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
