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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.07.2025 12:29, Andrew Cooper wrote:
> On 18/07/2025 6:53 am, Jan Beulich wrote:
>> 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.)
> 
> Well, as stated: "It's looking for the {} entry, by looking for 0's in
> all of the metadata fields."
> 
> The intended usage of ->steppings, or ->feature for that matter, is not
> relevant to the loop termination condition, which is simply "is all the
> metadata 0".
> 
>>>>>      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?
> 
> Shrinking model without adding (and checking) a new field would mean the
> loop condition no longer covers all metadata.

And it doesn't strictly need to. It needs to check enough to not mistake a
valid entry for a sentinel one.

Jan