1. Patchew
  2. Xen
  3. View series

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

Stefano Stabellini posted 1 patch 3 years, 3 months ago
Failed in applying to current master (apply log)
xen/arch/arm/cpufeature.c        | 35 +++++++++++++++++++-------------
xen/include/asm-arm/cpufeature.h |  2 ++
2 files changed, 23 insertions(+), 14 deletions(-)
[PATCH v2] xen/arm: don't read aarch32 regs when aarch32 isn't available
Posted by Stefano Stabellini 3 years, 3 months ago 
Don't read aarch32 system registers at boot time when the aarch32 state
is not available at EL0. 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.

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://lore.kernel.org/xen-devel/f90e40ee-b042-6cc5-a08d-aef41a279527@suse.com/
Suggested-by: Julien Grall <julien@xen.org>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
Changes in v2:
- check el0 instead of el1 for aarch32 support
- clarify EL0 in commit message
- remove temporary link in commit message
- use lore.kernel.org in commit message
- introduce cpu_feature64_has_el0_32
- rename aarch32 to aarch32_el0
---
 xen/arch/arm/cpufeature.c        | 35 +++++++++++++++++++-------------
 xen/include/asm-arm/cpufeature.h |  2 ++
 2 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
index 698bfa0201..99fe4db280 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_el0 = 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_el0 = cpu_feature64_has_el0_32(c);
 #endif
 
+    if ( aarch32_el0 )
+    {
         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
+    }
 }
 
 /*
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index 6058744c18..13a2739a69 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -5,6 +5,8 @@
 #define cpu_feature64(c, feat)         ((c)->pfr64.feat)
 #define boot_cpu_feature64(feat)       (boot_cpu_data.pfr64.feat)
 
+#define cpu_feature64_has_el0_32(c)    (cpu_feature64(c, el0) == 2)
+
 #define cpu_has_el0_32    (boot_cpu_feature64(el0) == 2)
 #define cpu_has_el0_64    (boot_cpu_feature64(el0) >= 1)
 #define cpu_has_el1_32    (boot_cpu_feature64(el1) == 2)
-- 
2.17.1
Re: [PATCH v2] xen/arm: don't read aarch32 regs when aarch32 isn't available
Posted by Bertrand Marquis 3 years, 3 months ago 
Hi Stefano,

> On 12 Jan 2021, at 23:44, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> Don't read aarch32 system registers at boot time when the aarch32 state
> is not available at EL0. 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.

Might be a good idea to say that on Cavium ID_PFR2_EL1 is generating an
unsupported exception causing a Xen crash on boot.

> 
> Leave the corresponding fields in struct cpuinfo_arm so that they
> are read-as-zero from a guest.
> 
> 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://lore.kernel.org/xen-devel/f90e40ee-b042-6cc5-a08d-aef41a279527@suse.com/
> Suggested-by: Julien Grall <julien@xen.org>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
with this added:

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> Changes in v2:
> - check el0 instead of el1 for aarch32 support
> - clarify EL0 in commit message
> - remove temporary link in commit message
> - use lore.kernel.org in commit message
> - introduce cpu_feature64_has_el0_32
> - rename aarch32 to aarch32_el0
> ---
> xen/arch/arm/cpufeature.c        | 35 +++++++++++++++++++-------------
> xen/include/asm-arm/cpufeature.h |  2 ++
> 2 files changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
> index 698bfa0201..99fe4db280 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_el0 = 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_el0 = cpu_feature64_has_el0_32(c);
> #endif
> 
> +    if ( aarch32_el0 )
> +    {
>         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
> +    }
> }
> 
> /*
> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> index 6058744c18..13a2739a69 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -5,6 +5,8 @@
> #define cpu_feature64(c, feat)         ((c)->pfr64.feat)
> #define boot_cpu_feature64(feat)       (boot_cpu_data.pfr64.feat)
> 
> +#define cpu_feature64_has_el0_32(c)    (cpu_feature64(c, el0) == 2)
> +
> #define cpu_has_el0_32    (boot_cpu_feature64(el0) == 2)
> #define cpu_has_el0_64    (boot_cpu_feature64(el0) >= 1)
> #define cpu_has_el1_32    (boot_cpu_feature64(el1) == 2)
> -- 
> 2.17.1
>
Re: [PATCH v2] xen/arm: don't read aarch32 regs when aarch32 isn't available
Posted by Julien Grall 3 years, 3 months ago 
Hi Stefano,

I think you forgot to update the title to say the state is EL0.

On 12/01/2021 23:44, Stefano Stabellini wrote:
> Don't read aarch32 system registers at boot time when the aarch32 state
> is not available at EL0. 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.
> 
> 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://lore.kernel.org/xen-devel/f90e40ee-b042-6cc5-a08d-aef41a279527@suse.com/
> Suggested-by: Julien Grall <julien@xen.org>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.