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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 12 Jan 2021, at 14:27, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 12/01/2021 13:28, Rahul Singh wrote:
>> Hello Julien,
> 
> Hi Rahul,
> 
>>> 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.
> 
> This statement is correct.
> 
>> 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.
> 
> I agree that if EL1 supports AArch32, then it means EL0 will not support.
> 
> However, if EL1 doesn't support AArch32, then it doesn't imply that EL0 will 
> not support AArch32.
> 
> Therefore, the check suggested would not be correct because it would prevent 
> 32-bit userspace running on HW (such as IIRC Cortex-A76) that only support 
> AArch32 in EL0.

I agree with this, we should check aarch32 at EL0 as if it is not supported 
there it is not supported at all and the the registers should not be needed.
If it is supported in EL0, wether or not it is supported at lower level, we 
need to read those registers.

So we should check pfr64.el0 instead.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall