Re: [PATCH 5/6] x86/match-cpu: Support matching on steppings

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.07.2025 19:31, Andrew Cooper wrote:
> Architecturally, stepping is a 4-bit field, so a uint16_t suffices for a
> bitmap of steppings.
> 
> In order to keep the size of struct x86_cpu_id the same, shrink the vendor and
> family fields, neither of which need to be uint16_t in Xen.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Linux supports all fields being optional.  This has lead to using
> X86_MATCH_CPU(ANY, ANY, ANY, ANY, FEATURE_FOO, NULL) in place of
> boot_cpu_has(), and is not a construct I think we want to encorage.

+1

> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -1003,13 +1003,15 @@ const struct x86_cpu_id *x86_match_cpu(const struct 
> x86_cpu_id table[])
>       const struct x86_cpu_id *m;
>       const struct cpuinfo_x86 *c = &boot_cpu_data;
>  
> -     for (m = table; m->vendor | m->family | m->model | m->feature; m++) {
> +     for (m = table; m->vendor | m->family | m->model | m->steppings | 
> m->feature; m++) {

Nit: Line length. But - do we need the change at all? It looks entirely
implausible to me to use ->steppings with all of vendor, family, and
model being *_ANY (if, as per below, they would be 0 in the first place).

Tangential: The ->feature check is slightly odd here. With everything
else being a wildcard (assuming these are 0; I can't find any X86_*_ANY
in the code base; INTEL_FAM6_ANY expands to X86_MODEL_ANY, but is itself
also not used anywhere), one wouldn't be able to use FPU, as that's
feature index 0. I notice though that ...

>               if (c->x86_vendor != m->vendor)
>                       continue;
>               if (c->x86 != m->family)
>                       continue;
>               if (c->x86_model != m->model)
>                       continue;

... X86_*_ANY also aren't catered for here. Hence it remains unclear
what value those constants would actually be meant to have.

Further tangential: The vendor check could in principle permit for
multiple vendors (e.g. AMD any Hygon at the same time), considering that
we use bit masks now. That would require the != there to change, though.

> --- a/xen/arch/x86/include/asm/match-cpu.h
> +++ b/xen/arch/x86/include/asm/match-cpu.h
> @@ -8,28 +8,32 @@
>  #include <asm/intel-family.h>
>  #include <asm/x86-vendors.h>
>  
> +#define X86_STEPPINGS_ANY 0

Given the (deliberate aiui) plural, maybe better X86_STEPPINGS_ALL?

Also perhaps use 0xffff as the value, allowing to drop part of the
conditional in x86_match_cpu()?

>  #define X86_FEATURE_ANY X86_FEATURE_LM
>  
>  struct x86_cpu_id {
> -    uint16_t vendor;
> -    uint16_t family;
> +    uint8_t vendor;

Is shrinking this to 8 bits a good idea? We use 5 of them already. (Of
course we can re-enlarge later, if and when the need arises.)

> +    uint8_t family;

The family formula allows the value to be up to 0x10e. The return type
of get_cpu_family() is therefore wrong too, strictly speaking. As is
struct cpuinfo_x86's x86 field.

>      uint16_t model;

Whereas the model is strictly limited to 8 bits.

Jan