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

[Julien Grall <julien.grall.oss@xxxxxxxxx>]

On Tue, 12 Jan 2021 at 21:05, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
>
> On Tue, 12 Jan 2021, Julien Grall wrote:
> > On Tue, 12 Jan 2021 at 19:09, Stefano Stabellini <sstabellini@xxxxxxxxxx> 
> > wrote:
> > >
> > > 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?
> >
> > I thought about that when I wrote the first e-mail. However, this
> > would not work properly for heterogeneous platforms.
> > There is still a risk to read garbage (or UNDEF) if the boot CPU
> > supports AArch32 but the others.
>
> Yeah, but that is not the kind of thing that can be actually different
> in a heterogeneous platform, as far as I am aware?

Arm CPU vendors can do a lot of interesting mix. For Samsung released
in the past a big.LITTLE with a mix of Armv8.0 and Armv8.2 cores (see
[1]).
It turns out to be a massive blunder because they hack Linux to avoid
detecting a minimum feature set. So the userspace app was trying to
use LSE atomics on Armv8.0 (yes).

I wouldn't be surprised to see a phone with a mix of 64-bit only
processor and one with 32-bit EL0 to run legacy apps.

>
> In any case, a check that takes that into consideraion would be:
>
>     aarch32_el0 = cpu_has_el0_32 &&
>                   (boot_cpu_feature64(el0) == cpu_feature64(c, el0));

Why do you want to check the boot CPU feature? The per-cpu cpuinfo
should really contain a raw copy of the ID registers of that CPU.

Anything else will make our life very difficult when we try to look
for a common set of features or want to expose big.LITTLE to a guest
(this request comes back time to time).

>
> If you have something better in mind please feel free to suggest it and
> I'll add it to the patch. Otherwise, I'll send it with this later today
> with a note that if you want to make a change on commit you have my
> blessing :-)

Let me start by saying that I think the existing cpu_has_* are
misnamed because the name suggest it would check the features of the
current CPU but they only check the boot CPU. But I am not going to
suggest a renaming for now. The header cpufeature.h likely needs an
overhaul.

Instead I would suggest the following:

Pseudo-code:

#define cpu_feature64_has_el0(c) cpu_feature64(c, el0) == 2

And the the code would use:

aarch32_el0 = cpu_feature64_has_el0(c);

[1] https://github.com/golang/go/issues/28431