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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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.

~Andrew