[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] xen/arm: don't read aarch32 regs when aarch32 isn't available
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 ? Cheers Bertrand > + } > } > > /* > -- > 2.17.1 >
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |