Re: [PATCH] xen/arm: don't read aarch32 regs when aarch32 isn't available

[Julien Grall <julien@xxxxxxx>]

On 12/01/2021 10:50, Bertrand Marquis wrote:
> Hi Stefano,
>
> > On 12 Jan 2021, at 00:16, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> >
> > Don't read aarch32 system registers at boot time when the aarch32 state
> > is not available. They are UNKNOWN, so it is not useful to read them.
> > Moreover, on Cavium ThunderX reading ID_PFR2_EL1 causes a Xen crash.
> > Instead, only read them when aarch32 is available.
> >
> > Leave the corresponding fields in struct cpuinfo_arm so that they
> > are read-as-zero from a guest.
> I agree with the fact that all users of identify_cpu are currently using spaces
> which are 0 but to make the core a bit more robust we could do a memset(0)
> of the structure at the beginning of the function.
>
> > Since we are editing identify_cpu, also fix the indentation: 4 spaces
> > instead of 8.
> >
> > Fixes: 9cfdb489af81 ("xen/arm: Add ID registers and complete cpuinfo")
> > Link: https://marc.info/?l=xen-devel&m=161035501118086
> > Link: 
> > http://logs.test-lab.xenproject.org/osstest/logs/158293/test-arm64-arm64-xl-xsm/info.html
> > Suggested-by: Julien Grall <julien@xxxxxxx>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > ---
> > xen/arch/arm/cpufeature.c | 35 +++++++++++++++++++++--------------
> > 1 file changed, 21 insertions(+), 14 deletions(-)
> >
> > diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
> > index 698bfa0201..b1c82ade49 100644
> > --- a/xen/arch/arm/cpufeature.c
> > +++ b/xen/arch/arm/cpufeature.c
> > @@ -101,29 +101,35 @@ int enable_nonboot_cpu_caps(const struct 
> > arm_cpu_capabilities *caps)
> >
> > void identify_cpu(struct cpuinfo_arm *c)
> > {
> > -        c->midr.bits = READ_SYSREG(MIDR_EL1);
> > -        c->mpidr.bits = READ_SYSREG(MPIDR_EL1);
> > +    bool aarch32 = true;
> > +
> > +    c->midr.bits = READ_SYSREG(MIDR_EL1);
> > +    c->mpidr.bits = READ_SYSREG(MPIDR_EL1);
> >
> > #ifdef CONFIG_ARM_64
> > -        c->pfr64.bits[0] = READ_SYSREG(ID_AA64PFR0_EL1);
> > -        c->pfr64.bits[1] = READ_SYSREG(ID_AA64PFR1_EL1);
> > +    c->pfr64.bits[0] = READ_SYSREG(ID_AA64PFR0_EL1);
> > +    c->pfr64.bits[1] = READ_SYSREG(ID_AA64PFR1_EL1);
> > +
> > +    c->dbg64.bits[0] = READ_SYSREG(ID_AA64DFR0_EL1);
> > +    c->dbg64.bits[1] = READ_SYSREG(ID_AA64DFR1_EL1);
> >
> > -        c->dbg64.bits[0] = READ_SYSREG(ID_AA64DFR0_EL1);
> > -        c->dbg64.bits[1] = READ_SYSREG(ID_AA64DFR1_EL1);
> > +    c->aux64.bits[0] = READ_SYSREG(ID_AA64AFR0_EL1);
> > +    c->aux64.bits[1] = READ_SYSREG(ID_AA64AFR1_EL1);
> >
> > -        c->aux64.bits[0] = READ_SYSREG(ID_AA64AFR0_EL1);
> > -        c->aux64.bits[1] = READ_SYSREG(ID_AA64AFR1_EL1);
> > +    c->mm64.bits[0]  = READ_SYSREG(ID_AA64MMFR0_EL1);
> > +    c->mm64.bits[1]  = READ_SYSREG(ID_AA64MMFR1_EL1);
> > +    c->mm64.bits[2]  = READ_SYSREG(ID_AA64MMFR2_EL1);
> >
> > -        c->mm64.bits[0]  = READ_SYSREG(ID_AA64MMFR0_EL1);
> > -        c->mm64.bits[1]  = READ_SYSREG(ID_AA64MMFR1_EL1);
> > -        c->mm64.bits[2]  = READ_SYSREG(ID_AA64MMFR2_EL1);
> > +    c->isa64.bits[0] = READ_SYSREG(ID_AA64ISAR0_EL1);
> > +    c->isa64.bits[1] = READ_SYSREG(ID_AA64ISAR1_EL1);
> >
> > -        c->isa64.bits[0] = READ_SYSREG(ID_AA64ISAR0_EL1);
> > -        c->isa64.bits[1] = READ_SYSREG(ID_AA64ISAR1_EL1);
> > +    c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
> >
> > -        c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
> > +    aarch32 = c->pfr64.el1 == 2;
> I would put some () around the test.
>
> > #endif
> >
> > +    if ( aarch32 )
> > +    {
> >          c->pfr32.bits[0] = READ_SYSREG(ID_PFR0_EL1);
> >          c->pfr32.bits[1] = READ_SYSREG(ID_PFR1_EL1);
> >          c->pfr32.bits[2] = READ_SYSREG(ID_PFR2_EL1);
> > @@ -153,6 +159,7 @@ void identify_cpu(struct cpuinfo_arm *c)
> > #ifndef MVFR2_MAYBE_UNDEFINED
> >          c->mvfr.bits[2] = READ_SYSREG(MVFR2_EL1);
> > #endif
> If we test for aarch32, the ifndef here should not be needed anymore.
>  > Is your previous patch really still needed if we go with the aarch32 
path ?

There were two undefs discovered:
   1) On Armv7 when accessing MVFR2_EL1
   2) On Cavium Thunder-X (Armv8) when accessing ID_PFR2_EL1

If you remove the #ifdef, then you will re-introduce the UNDEF on Armv7.

Cheers,

--
Julien Grall