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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 19 Jul 2021, at 09:58, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> 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.

I understand the potential need (even though supporting to assign guest CPUs 
depending on features needed would be something complex to achieve).

> 
>> So I am wondering if we should not reduce a bit the amount of global data 
>> and:
> 
> How much are we talking?

We are declaring cpuinfo for all possible cores in smpboot:
struct cpuinfo_arm cpu_data[NR_CPUS];
And default value for NR_CPUS is 128 so that makes around 9k of data.

This is not that much I agree.


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

Agree and with the actual size reduction being quite small this does not make 
sense.

Thanks a lot for the feedback, I will go on with the system_cpuinfo and is.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall