Architecturally, stepping is a 4-bit field, so a uint16_t suffices for a
bitmap of steppings.
In order to keep the size of struct x86_cpu_id the same, shrink the vendor and
family fields, neither of which need to be uint16_t in Xen.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
Linux supports all fields being optional. This has lead to using
X86_MATCH_CPU(ANY, ANY, ANY, ANY, FEATURE_FOO, NULL) in place of
boot_cpu_has(), and is not a construct I think we want to encorage.
---
xen/arch/x86/cpu/common.c | 4 +++-
xen/arch/x86/include/asm/match-cpu.h | 12 ++++++++----
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index cc004fc976f5..fc25935d3109 100644
--- 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++) {
if (c->x86_vendor != m->vendor)
continue;
if (c->x86 != m->family)
continue;
if (c->x86_model != m->model)
continue;
+ if (m->steppings && !(m->steppings & (1U << c->stepping)))
+ continue;
if (!cpu_has(c, m->feature))
continue;
return m;
diff --git a/xen/arch/x86/include/asm/match-cpu.h b/xen/arch/x86/include/asm/match-cpu.h
index dcdc50a70d14..3862e766ccfc 100644
--- 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
#define X86_FEATURE_ANY X86_FEATURE_LM
struct x86_cpu_id {
- uint16_t vendor;
- uint16_t family;
+ uint8_t vendor;
+ uint8_t family;
uint16_t model;
+ uint16_t steppings; /* Stepping bitmap, or X86_STEPPINGS_ANY */
uint16_t feature; /* X86_FEATURE_*, or X86_FEATURE_ANY */
const void *driver_data;
};
-#define X86_MATCH_CPU(v, f, m, feat, data) \
+#define X86_MATCH_CPU(v, f, m, steps, feat, data) \
{ \
.vendor = (v), \
.family = (f), \
.model = (m), \
+ .steppings = (steps), \
.feature = (feat), \
.driver_data = (const void *)(unsigned long)(data), \
}
#define X86_MATCH_VFM(vfm, data) \
X86_MATCH_CPU(VFM_VENDOR(vfm), VFM_FAMILY(vfm), \
- VFM_MODEL(vfm), X86_FEATURE_ANY, data)
+ VFM_MODEL(vfm), X86_STEPPINGS_ANY, \
+ X86_FEATURE_ANY, data)
/*
* x86_match_cpu() - match the CPU against an array of x86_cpu_ids[]
--
2.39.5
On 16.07.2025 19:31, Andrew Cooper wrote:
> Architecturally, stepping is a 4-bit field, so a uint16_t suffices for a
> bitmap of steppings.
>
> In order to keep the size of struct x86_cpu_id the same, shrink the vendor and
> family fields, neither of which need to be uint16_t in Xen.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
>
> Linux supports all fields being optional. This has lead to using
> X86_MATCH_CPU(ANY, ANY, ANY, ANY, FEATURE_FOO, NULL) in place of
> boot_cpu_has(), and is not a construct I think we want to encorage.
+1
> --- 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).
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.
> --- 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?
Also perhaps use 0xffff as the value, allowing to drop part of the
conditional in x86_match_cpu()?
> #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.)
> + 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.
> uint16_t model;
Whereas the model is strictly limited to 8 bits.
Jan
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
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
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
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
On 18/07/2025 2:28 pm, Jan Beulich wrote: >>>>>> 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. I've found a nicer way of doing this, but it needs another prereq patch. I'm preparing a v2 with the remainder. It also addresses the horrible code generation. ~Andrew
© 2016 - 2025 Red Hat, Inc.