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

[Peter Maydell]

On Mon, 4 Feb 2019 at 15:33, Alex Bennée <address@hidden> wrote:
> Peter Maydell <address@hidden> writes:
> > 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?

I think this is because the field in the register is architecturally
defined to have a restricted set of permitted values, of which 0 is
not one. (6 means "ARMv8 debug architecture" so is the effective minimum.)

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

No, I meant having something where we either:
 (a) have a separate set of cpreg structs for the userspace visible versions,
or (b) have some code which modifies the cpreg structs in v8_idregs[] (it's
not const) before handing it to define_arm_cp_regs().
(b) sounds better to me -- the ideal here I think would be something where we:
 * mark all the regs in the space the kernel covers as PL0U_R with a bit
   of code rather than by changing all the cpreg structs
 * default them to "no bits of the real value are exposed, all bits 0"
 * have a hopefully short list of overrides, probably defined in a special
   purpose array with entries like
 { /* ID_AA64DFR0_EL1 */, .crm = 5, .opc2 = 0, .passbits = 0x0,
.fixedbits = 0x6 },
Or we could match the override entries with cpreg values by register name,
which would let you say
 { "ID_AA64DFR0_EL1", .passbits = 0x0, .fixedbits = 0x6 },
and avoid having to repeat the encoding values, which might be neater.

thanks
-- PMM