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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.07.2025 21:39, Andrew Cooper wrote:
> 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:

Feel free to switch to ||. (The use of | producing worse code is clearly
a weakness of the compiler. Especially when used on non-adjacent fields
I expect | to be quite a bit better, first and foremost by ending up
with just a single conditional branch. Sadly I haven't seen compilers
do such a transformation for us.)

All of your reply doesn't address my remark regarding whether to check
->steppings here, though. (And no, whether to check it shouldn't be
[solely] justified by the compiler generating better code that way.)

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

Now that Intel has decided to use higher family numbers, hopefully yes.

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

Breaking? Or merely affecting code generation in a negative way?

Jan