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

[Alex Bennée]

Peter Maydell <address@hidden> writes:

> On Mon, 28 Jan 2019 at 17:39, Alex Bennée <address@hidden> 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 <address@hidden>
>>
>> ---
>> 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.

Under a kernel I'm seeing numbers reported so I don't think it's totally
squashed:

  id_aa64dfr0_el1     : 0x0000000000000006
  id_aa64dfr1_el1     : 0x0000000000000000

Confusingly the kernel does:

static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
        ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, 36, 28, 0),
        ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, 
ID_AA64DFR0_PMSVER_SHIFT, 4, 0),
        ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 
ID_AA64DFR0_CTX_CMPS_SHIFT, 4, 0),
        ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 
ID_AA64DFR0_WRPS_SHIFT, 4, 0),
        ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 
ID_AA64DFR0_BRPS_SHIFT, 4, 0),
        /*
         * We can instantiate multiple PMU instances with different levels
         * of support.
         */
        S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_EXACT, 
ID_AA64DFR0_PMUVER_SHIFT, 4, 0),
        ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, 
ID_AA64DFR0_TRACEVER_SHIFT, 4, 0),
        ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, 
ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6),
        ARM64_FTR_END,
};

So I'm not sure if that is intentional as the line:

  ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, ID_AA64DFR0_DEBUGVER_SHIFT, 
4, 0x6)

implies the DEBUGVER is hidden but actually 0x6?

> 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.

Do you mean setting cpu->id_foo during cpu_init? I wasn't sure if that
might interact badly with the recent changes to base our features off
what's actually set in them.

>
> thanks
> -- PMM


--
Alex Bennée