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

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

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 :-)