Re: [PATCH v2] xen/arm: do not read MVFR2 when is not defined

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 8 Jan 2021, Julien Grall wrote:
> Hi Stefano,
> 
> On 08/01/2021 19:22, Stefano Stabellini wrote:
> > MVFR2 is not available on ARMv7. It is available on ARMv8 aarch32 and
> > aarch64. If Xen reads MVFR2 on ARMv7 it could crash.
> > 
> > Avoid the issue by doing the following:
> > 
> > - define MVFR2_MAYBE_UNDEFINED on arm32
> > - if MVFR2_MAYBE_UNDEFINED, do not attempt to read MVFR2 in Xen
> > - keep the 3rd register_t in struct cpuinfo_arm.mvfr on arm32 so that a
> >    guest read to the register returns '0' instead of crashing the guest.
> > 
> > '0' is an appropriate value to return to the guest because it is defined
> > as "no support for miscellaneous features".
> > 
> > Aarch64 Xen is not affected by this patch.
> > 
> > Fixes: 9cfdb489af81 ("xen/arm: Add ID registers and complete cpuinfo")
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> 
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
> 
> With one question below:

Thanks!


> >   xen/arch/arm/cpufeature.c           | 2 ++
> >   xen/include/asm-arm/arm32/sysregs.h | 3 +++
> >   2 files changed, 5 insertions(+)
> > 
> > diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
> > index 1f6a85aafe..698bfa0201 100644
> > --- a/xen/arch/arm/cpufeature.c
> > +++ b/xen/arch/arm/cpufeature.c
> > @@ -150,7 +150,9 @@ void identify_cpu(struct cpuinfo_arm *c)
> >             c->mvfr.bits[0] = READ_SYSREG(MVFR0_EL1);
> >           c->mvfr.bits[1] = READ_SYSREG(MVFR1_EL1);
> > +#ifndef MVFR2_MAYBE_UNDEFINED
> >           c->mvfr.bits[2] = READ_SYSREG(MVFR2_EL1);
> > +#endif
> 
> Is there any guarantee that c->mvfr.bits[2] will be zeroed by default?

It is coming from one of the following:

- xen/arch/arm/setup.c: struct cpuinfo_arm __read_mostly boot_cpu_data;
- xen/arch/arm/smpboot.c: struct cpuinfo_arm cpu_data[NR_CPUS];

Both are global variables so they should be both zeroed.