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:
https://termbin.com/c4m9
Although I'm a little confused as to why it's still done a split cmpw
$0x0,(%rax) and cmpq $0xffff,(%rax) for the loop entry condition, when
cmpq $0 would be the right one.
>
> Tangential: The ->feature check is slightly odd here. With everything
> else being a wildcard (assuming these are 0; I can't find any X86_*_ANY
> in the code base; INTEL_FAM6_ANY expands to X86_MODEL_ANY, but is itself
> also not used anywhere), one wouldn't be able to use FPU, as that's
> feature index 0. I notice though that ...
>
>> if (c->x86_vendor != m->vendor)
>> continue;
>> if (c->x86 != m->family)
>> continue;
>> if (c->x86_model != m->model)
>> continue;
> ... X86_*_ANY also aren't catered for here. Hence it remains unclear
> what value those constants would actually be meant to have.
>
> Further tangential: The vendor check could in principle permit for
> multiple vendors (e.g. AMD any Hygon at the same time), considering that
> we use bit masks now. That would require the != there to change, though.
In Linux, x86_cpu_id is a module ABI and has wildcards on all fields,
because "please load me on any AMD Fam10 CPU" is something they want to
express.
In Xen, we only use it model/stepping specific lookup tables, so we
don't need wildcards for V/F/M like Linux does.
We do have a different layout of X86_VENDOR to Intel, and while that
would allow us to merge an AMD and a Hygon row, I don't think anything
good could come of trying.
One problem Linux has is that X86_VENDOR_INTEL is 0, so they introduced
a flags field with a VALID bit that now replaces the line of |'s. I do
not see any need for that in Xen.
>
>> --- a/xen/arch/x86/include/asm/match-cpu.h
>> +++ b/xen/arch/x86/include/asm/match-cpu.h
>> @@ -8,28 +8,32 @@
>> #include <asm/intel-family.h>
>> #include <asm/x86-vendors.h>
>>
>> +#define X86_STEPPINGS_ANY 0
> Given the (deliberate aiui) plural, maybe better X86_STEPPINGS_ALL?
Hmm, yeah, that's not great grammar. I think I prefer X86_STEPPING_ANY
to X86_STEPPINGS_ALL.
> Also perhaps use 0xffff as the value, allowing to drop part of the
> conditional in x86_match_cpu()?
Interestingly, while it simplifies the C, it undoes most of the code
generation improvements from switching | to ||.
https://termbin.com/h0iu
By removing the "m->steppings &&", gcc has now hoisted the load of
c->stepping out of the loop (in fact, the whole 1U << c->stepping
calculation), but that's now resulted in a spill/restore of %rbx in the
loop, and also doubled up most of the loop. I have no idea what it's
trying to do here...
>
>> #define X86_FEATURE_ANY X86_FEATURE_LM
>>
>> 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.
>
>> 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).
~Andrew
