On Mon, 4 Feb 2019 at 15:33, Alex Bennée <alex.ben...@linaro.org> wrote:
> Peter Maydell <peter.mayd...@linaro.org> 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
