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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hello Julien,

> On 12 Jan 2021, at 11:00 am, Julien Grall <julien@xxxxxxx> wrote:
> 
> 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 per my understanding please correct me if I am wrong, if AArch32 is allowed 
at an EL, it must be allowed all lower Exception levels. 

For example, if EL3 allows AArch32, then it must be allowed at all lower 
Exception levels.That means if we are checking the EL1 for AArch32 EL0 should 
also support AArch32. 

I think  "aarch32 = c->pfr64.el1 == 2” is correct to check.

Regards,
Rahul

> 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