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
