Hi Stefano,
> On 12 Jan 2021, at 23:44, Stefano Stabellini <sstabell...@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-aef41a279...@suse.com/
> Suggested-by: Julien Grall <jul...@xen.org>
> Signed-off-by: Stefano Stabellini <stefano.stabell...@xilinx.com>
with this added:
Reviewed-by: Bertrand Marquis <bertrand.marq...@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
>
