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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/07/2025 9:11 am, Jan Beulich wrote:
> On 16.07.2025 19:31, Andrew Cooper wrote:
>> --- 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).

I do keep on saying that | like this is pure obfuscation.  This is an
excellent example.

It's looking for the {} entry, by looking for 0's in all of the metadata
fields.  A better check would be *(uint64_t *)m, or perhaps a unioned
metadata field, but..

This is also a good demonstration of binary | is a bad thing to use, not
only for legibility.  Swapping | for || lets the compiler do:

add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-76 (-76)
Function                                     old     new   delta
x86_match_cpu                                243     167     -76

and the code generation looks much better too:

https://termbin.com/c4m9

Although I'm a little confused as to why it's still done a split cmpw
$0x0,(%rax) and cmpq $0xffff,(%rax) for the loop entry condition, when
cmpq $0 would be the right one.


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

In Linux, x86_cpu_id is a module ABI and has wildcards on all fields,
because "please load me on any AMD Fam10 CPU" is something they want to
express.

In Xen, we only use it model/stepping specific lookup tables, so we
don't need wildcards for V/F/M like Linux does.

We do have a different layout of X86_VENDOR to Intel, and while that
would allow us to merge an AMD and a Hygon row, I don't think anything
good could come of trying.

One problem Linux has is that X86_VENDOR_INTEL is 0, so they introduced
a flags field with a VALID bit that now replaces the line of |'s.  I do
not see any need for that in Xen.



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

Hmm, yeah, that's not great grammar.  I think I prefer X86_STEPPING_ANY
to X86_STEPPINGS_ALL.

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

Interestingly, while it simplifies the C, it undoes most of the code
generation improvements from switching | to ||.

https://termbin.com/h0iu

By removing the "m->steppings &&", gcc has now hoisted the load of
c->stepping out of the loop (in fact, the whole 1U << c->stepping
calculation), but that's now resulted in a spill/restore of %rbx in the
loop, and also doubled up most of the loop.  I have no idea what it's
trying to do here...

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

It's the same size as cpuinfo_x86's field has been for 2 decades.

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

Again, this is the size of the field in cpuinfo_x86.  I don't think
0x10e is anything we're going to have to worry about any time soon.

>
>>      uint16_t model;
> Whereas the model is strictly limited to 8 bits.

There is space in here, if we need it, but you can't shrink it without
breaking the check for the NULL entry (going back to the first obfuscation).

~Andrew