[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 Julien, > On 12 Jan 2021, at 12:46, Julien Grall <julien@xxxxxxx> wrote: > > > > 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. Right sorry missed that one. Cheers Bertrand > > Cheers, > > -- > Julien Grall
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |