[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





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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.