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

[Julien Grall <julien@xxxxxxx>]

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 <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;
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