Re: [Qemu-devel] [PATCH v1 2/4] target/arm: expose CPUID registers to userspace

[Peter Maydell <peter.maydell@linaro.org>]

On Mon, 28 Jan 2019 at 17:39, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> A number of CPUID registers are exposed to userspace by modern Linux
> kernels thanks to the "ARM64 CPU Feature Registers" ABI. For QEMU's
> user-mode emulation we don't need to emulate the kernels trap but just
> return the value the trap would have done. For this we use the PL0U_R
> permission mask which allows this access in CONFIG_USER mode.
>
> Some registers only return a subset of their contents so we need
> specific CONFIG_USER_ONLY logic to do this.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v4
>   - tweak commit message
>   - use PL0U_R instead of PL1U_R to be less confusing
>   - more CONFIG_USER logic for special cases
>   - mask a bunch of bits for some registers
> ---
>  target/arm/helper.c | 51 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 36 insertions(+), 15 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 42c1c0b144..68808e7293 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3543,7 +3543,7 @@ static uint64_t mpidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>  static const ARMCPRegInfo mpidr_cp_reginfo[] = {
>      { .name = "MPIDR", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 5,
> -      .access = PL1_R, .readfn = mpidr_read, .type = ARM_CP_NO_RAW },
> +      .access = PL0U_R, .readfn = mpidr_read, .type = ARM_CP_NO_RAW },
>      REGINFO_SENTINEL
>  };
>
> @@ -5488,6 +5488,7 @@ static uint64_t id_pfr1_read(CPUARMState *env, const ARMCPRegInfo *ri)
>      return pfr1;
>  }
>
> +#ifndef CONFIG_USER_ONLY
>  static uint64_t id_aa64pfr0_read(CPUARMState *env, const ARMCPRegInfo *ri)
>  {
>      ARMCPU *cpu = arm_env_get_cpu(env);
> @@ -5498,6 +5499,7 @@ static uint64_t id_aa64pfr0_read(CPUARMState *env, const ARMCPRegInfo *ri)
>      }
>      return pfr0;
>  }
> +#endif
>
>  /* Shared logic between LORID and the rest of the LOR* registers.
>   * Secure state has already been delt with.
> @@ -5799,18 +5801,26 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>           * define new registers here.
>           */
>          ARMCPRegInfo v8_idregs[] = {
> -            /* ID_AA64PFR0_EL1 is not a plain ARM_CP_CONST because we don't
> -             * know the right value for the GIC field until after we
> -             * define these regs.
> +            /* ID_AA64PFR0_EL1 is not a plain ARM_CP_CONST for system
> +             * emulation because we don't know the right value for the
> +             * GIC field until after we define these regs. For
> +             * user-mode HWCAP_CPUID emulation the GIC bits are masked
> +             * anyway.
>               */
>              { .name = "ID_AA64PFR0_EL1", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 0,
> +#ifndef CONFIG_USER_ONLY
>                .access = PL1_R, .type = ARM_CP_NO_RAW,
>                .readfn = id_aa64pfr0_read,
> -              .writefn = arm_cp_write_ignore },
> +              .writefn = arm_cp_write_ignore
> +#else
> +              .access = PL0U_R, .type = ARM_CP_CONST,
> +              .resetvalue = cpu->isar.id_aa64pfr0 & 0x000f000f0ff0000ULL
> +#endif
> +            },
>              { .name = "ID_AA64PFR1_EL1", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 1,
> -              .access = PL1_R, .type = ARM_CP_CONST,
> +              .access = PL0U_R, .type = ARM_CP_CONST,
>                .resetvalue = cpu->isar.id_aa64pfr1},
>              { .name = "ID_AA64PFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 2,
> @@ -5839,11 +5849,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>                .resetvalue = 0 },
>              { .name = "ID_AA64DFR0_EL1", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 0,
> -              .access = PL1_R, .type = ARM_CP_CONST,
> +              .access = PL0U_R, .type = ARM_CP_CONST,
>                .resetvalue = cpu->id_aa64dfr0 },

This doesn't seem right -- I think ID_AA64DFR0_EL1 should be exposed
as zero, not whatever the underlying register value is.

More generally, this ifdeffing of "maybe mask the values" doesn't
seem very future-proof. If we add something to one of the cpu-id_*
values, that should be masked out to zero by default for linux-user,
not defaulting to passed through.

thanks
-- PMM