Hi Stefano,
On 12/01/2021 00:16, Stefano Stabellini 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.
AArch32 may be supported in EL0 but not in EL1. So I think you want to
clarify in the commit message/title which EL you are referring to.
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.
I was going to ask to split that in a separate patch. But then, I
noticed that it avoids to change the indentation of the if body twice.
So I am ok with that.
Fixes: 9cfdb489af81 ("xen/arm: Add ID registers and complete cpuinfo")
Link: https://marc.info/?l=xen-devel&m=161035501118086
NIT: I would suggest to use lore.kernel.org just because the link
contains the message-id. So if the website goes down, it is still
possible to track the original discussion.
Link:
http://logs.test-lab.xenproject.org/osstest/logs/158293/test-arm64-arm64-xl-xsm/info.html
IIRC we only keep the logs around for a couple of weeks. So this is
going to be break quickly. Therefore, I would suggest to remove this link.
Suggested-by: Julien Grall <jul...@xen.org>
Signed-off-by: Stefano Stabellini <stefano.stabell...@xilinx.com>
---
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;
This is checking that AArch32 is available in EL1. However, it may not
be the case yet it would be available in EL0.
As a consequence, 32-bit userspace wouldn't work properly after this patch.
The Arm Arm mandates that if AArch32 is available at EL(n), then it must
be available at EL(n - 1).
So we should check that AArch32 is available at EL0 because this would
cover the case where AArch32 is enabled at EL1.
Furthermore, I would also like to avoid hardcoding value in the code as
it is less readable. We already define cpu_has_el0_32 which use the boot
CPU feature. Maybe we want to expand the macro or split it?
#endif
+ if ( aarch32 )
I read this check as "If AArch32 is not available at any EL". But you
are checking whether it is available at a given level. So I would
suggest to suffix with the EL for clarification.
In this case, I think you will want to call it 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
+ }
}
/*
Cheers,
--
Julien Grall