Re: [RFC PATCH 3/4] xen/arm: Sanitize cpuinfo ID registers fields

[Julien Grall <julien@xxxxxxx>]

On 16/07/2021 18:14, Bertrand Marquis wrote:
> Hi Julien
Hi Bertrand,

> […]
> > > +
> > > +    if ( old_reg != new_reg )
> > > +        printk(XENLOG_DEBUG "SANITY DIF: %s 0x%"PRIx64" -> 0x%"PRIx64"\n",
> > > +               reg_name, old_reg, new_reg);
> > > +    if ( old_reg != *cur_reg )
> > > +        printk(XENLOG_DEBUG "SANITY FIX: %s 0x%"PRIx64" -> 0x%"PRIx64"\n",
> > > +               reg_name, old_reg, *cur_reg);
> > > +
> > > +    if ( taint )
> > > +    {
> > > +        printk(XENLOG_WARNING "SANITY CHECK: Unexpected variation in %s.\n",
> > > +                reg_name);
> > > +        add_taint(TAINT_CPU_OUT_OF_SPEC);
> > > +    }
> > > +}
> > > +
> > > +
> > > +/*
> > > + * This function should be called on secondary cores to sanitize the boot cpu
> > > + * cpuinfo.
> > Can we renamed boot_cpu_data to system_cpuinfo (or something similar)? This 
> > would make clear this is not only the features for the boot CPU?
> While looking at this request, I checked a bit how boot_cpu_data and cpu_data 
> overall are used:
> - boot_cpu_data is only used in setup.c, by boot_cpu_features macros, in 
> smpboot to retrieve the bootcpu midr, in p2m and by cpufeatures
> - cpu_data[] is used in smpboot, in errata handling to test for csv2, and in 
> vcpreg to access the midr
>
> So we have a bunch of cpuinfo structures as global variables but most of them 
> are not really used or did I miss something ?
While I agree this is not useful today, the idea is we can find easily 
what features each processor supports. This could be useful if we wanted 
to expose big.LITTLE to the guest.
For instance, imagine you have a system where some processor may support 
32-bit EL1 only on some processor. With a global approach, we would say 
"32-bit EL1 is not supported". That would prevent a user to use the 
system to its full advantage.
Note that I am not asking to implement such things today... This is more 
to show that we will likely want to keep the per-CPU info around.
The system_cpuinfo could be used for system wide decision in Xen (e.g. 
P2M size, cacheline size....) while the per-CPU could be used to enable 
features only used by a couple of CPUs.
> So I am wondering if we should not reduce a bit the amount of global data and:
How much are we talking?

> - introduce a global system_cpuinfo
> - remove cpu_data[] and use a temp structure in the stack of the cpu booting
> - read midr directly in vcpreg
> - use boot_cpu_data in errata for csv2
This would not be quite the same. You may have a system where not all 
processors have ID_AA64PFR0_EL1.CSV2 is set, yet we want to avoid 
setting the hardening vector on process with the bit set to reduce the 
overhead.
Cheers,

--
Julien Grall