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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 12 Jan 2021, Julien Grall wrote:
> > +    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.

OK


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

I agree

Technically, cpu_has_el0_32 works as is, because it is called after
boot_cpu_data has been updated. So we could just use it. What do you
think?


> >   #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.

OK