Intel have run out of model space in Family 6 and will start using Family 19
starting with Diamond Rapids. Xen, like Linux, has model checking logic which
will malfunction owing to bad assumptions about the family field.
Reorder the family, vendor and model fields so they can be accessed together
as a single vfm field.
As we're cleaning up the logic, take the opportunity to introduce better
names, dropping the x86 prefix.
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>
---
xen/arch/x86/include/asm/cpufeature.h | 28 +++++++++++++++++++++++----
xen/arch/x86/setup.c | 4 +++-
2 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h
index 3c2ac964e410..707b134c09c7 100644
--- a/xen/arch/x86/include/asm/cpufeature.h
+++ b/xen/arch/x86/include/asm/cpufeature.h
@@ -20,10 +20,30 @@
#ifndef __ASSEMBLY__
struct cpuinfo_x86 {
- unsigned char x86; /* CPU family */
- unsigned char x86_vendor; /* CPU vendor */
- unsigned char x86_model;
- unsigned char x86_mask;
+ /* TODO: Phase out the x86 prefixed names. */
+ union {
+ struct {
+ union {
+ uint8_t x86_model;
+ uint8_t model;
+ };
+ union {
+ uint8_t x86;
+ uint8_t family;
+ };
+ union {
+ uint8_t x86_vendor;
+ uint8_t vendor;
+ };
+ uint8_t _rsvd;
+ };
+ uint32_t vfm; /* Vendor Family Model */
+ };
+ union {
+ uint8_t x86_mask;
+ uint8_t stepping;
+ };
+
unsigned int cpuid_level; /* Maximum supported CPUID level */
unsigned int extended_cpuid_level; /* Maximum supported CPUID extended level */
unsigned int x86_capability[NCAPINTS];
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 24e4f5ac7f5d..37421ac9d05b 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -178,7 +178,9 @@ void *stack_start = cpu0_stack + STACK_SIZE - sizeof(struct cpu_info);
/* Used by the boot asm and EFI to stash the multiboot_info paddr. */
unsigned int __initdata multiboot_ptr;
-struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 0, 0, -1 };
+struct cpuinfo_x86 __read_mostly boot_cpu_data = {
+ .cpuid_level = -1,
+};
unsigned long __read_mostly mmu_cr4_features = XEN_MINIMAL_CR4;
--
2.39.5
On 16.07.2025 15:28, Andrew Cooper wrote:
> Intel have run out of model space in Family 6 and will start using Family 19
> starting with Diamond Rapids. Xen, like Linux, has model checking logic which
> will malfunction owing to bad assumptions about the family field.
>
> Reorder the family, vendor and model fields so they can be accessed together
> as a single vfm field.
>
> As we're cleaning up the logic, take the opportunity to introduce better
> names, dropping the x86 prefix.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
In principle
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Two remarks, though:
> --- a/xen/arch/x86/include/asm/cpufeature.h
> +++ b/xen/arch/x86/include/asm/cpufeature.h
> @@ -20,10 +20,30 @@
> #ifndef __ASSEMBLY__
>
> struct cpuinfo_x86 {
> - unsigned char x86; /* CPU family */
> - unsigned char x86_vendor; /* CPU vendor */
> - unsigned char x86_model;
> - unsigned char x86_mask;
> + /* TODO: Phase out the x86 prefixed names. */
> + union {
> + struct {
> + union {
> + uint8_t x86_model;
> + uint8_t model;
> + };
> + union {
> + uint8_t x86;
> + uint8_t family;
> + };
> + union {
> + uint8_t x86_vendor;
> + uint8_t vendor;
> + };
> + uint8_t _rsvd;
Can we perhaps name this e.g. _zero, so it's clear that it cannot be
repurposed?
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -178,7 +178,9 @@ void *stack_start = cpu0_stack + STACK_SIZE - sizeof(struct cpu_info);
> /* Used by the boot asm and EFI to stash the multiboot_info paddr. */
> unsigned int __initdata multiboot_ptr;
>
> -struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 0, 0, -1 };
> +struct cpuinfo_x86 __read_mostly boot_cpu_data = {
> + .cpuid_level = -1,
> +};
So you retain the bogus setting of this field. Would you mind taking a
look at [1], one of the many things that I never heard back on? I'm
deliberately purging that non-sense there as a (side-)effect. Plus
really I'm getting tired of having to re-base my long-pending changes
over ones you are helped getting in pretty quickly. No matter that this
one's going to be one of the easy ones (I hope).
Jan
[1] https://lists.xen.org/archives/html/xen-devel/2024-02/msg00726.html
On 16/07/2025 2:47 pm, Jan Beulich wrote:
> On 16.07.2025 15:28, Andrew Cooper wrote:
>> Intel have run out of model space in Family 6 and will start using Family 19
>> starting with Diamond Rapids. Xen, like Linux, has model checking logic which
>> will malfunction owing to bad assumptions about the family field.
>>
>> Reorder the family, vendor and model fields so they can be accessed together
>> as a single vfm field.
>>
>> As we're cleaning up the logic, take the opportunity to introduce better
>> names, dropping the x86 prefix.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> In principle
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Two remarks, though:
>
>> --- a/xen/arch/x86/include/asm/cpufeature.h
>> +++ b/xen/arch/x86/include/asm/cpufeature.h
>> @@ -20,10 +20,30 @@
>> #ifndef __ASSEMBLY__
>>
>> struct cpuinfo_x86 {
>> - unsigned char x86; /* CPU family */
>> - unsigned char x86_vendor; /* CPU vendor */
>> - unsigned char x86_model;
>> - unsigned char x86_mask;
>> + /* TODO: Phase out the x86 prefixed names. */
>> + union {
>> + struct {
>> + union {
>> + uint8_t x86_model;
>> + uint8_t model;
>> + };
>> + union {
>> + uint8_t x86;
>> + uint8_t family;
>> + };
>> + union {
>> + uint8_t x86_vendor;
>> + uint8_t vendor;
>> + };
>> + uint8_t _rsvd;
> Can we perhaps name this e.g. _zero, so it's clear that it cannot be
> repurposed?
It can be repurposed; it just must be done in coordination with VFM_MAKE().
I can add a comment to this effect, but it would need to be in the
subsequent patch when VFM_MAKE() is introduced.
>
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -178,7 +178,9 @@ void *stack_start = cpu0_stack + STACK_SIZE - sizeof(struct cpu_info);
>> /* Used by the boot asm and EFI to stash the multiboot_info paddr. */
>> unsigned int __initdata multiboot_ptr;
>>
>> -struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 0, 0, -1 };
>> +struct cpuinfo_x86 __read_mostly boot_cpu_data = {
>> + .cpuid_level = -1,
>> +};
> So you retain the bogus setting of this field. Would you mind taking a
> look at [1], one of the many things that I never heard back on? I'm
> deliberately purging that non-sense there as a (side-)effect. Plus
> really I'm getting tired of having to re-base my long-pending changes
> over ones you are helped getting in pretty quickly. No matter that this
> one's going to be one of the easy ones (I hope).
>
> Jan
>
> [1] https://lists.xen.org/archives/html/xen-devel/2024-02/msg00726.html
I can rebase.
~Andrew
On 16.07.2025 17:45, Andrew Cooper wrote:
> On 16/07/2025 2:47 pm, Jan Beulich wrote:
>> On 16.07.2025 15:28, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/include/asm/cpufeature.h
>>> +++ b/xen/arch/x86/include/asm/cpufeature.h
>>> @@ -20,10 +20,30 @@
>>> #ifndef __ASSEMBLY__
>>>
>>> struct cpuinfo_x86 {
>>> - unsigned char x86; /* CPU family */
>>> - unsigned char x86_vendor; /* CPU vendor */
>>> - unsigned char x86_model;
>>> - unsigned char x86_mask;
>>> + /* TODO: Phase out the x86 prefixed names. */
>>> + union {
>>> + struct {
>>> + union {
>>> + uint8_t x86_model;
>>> + uint8_t model;
>>> + };
>>> + union {
>>> + uint8_t x86;
>>> + uint8_t family;
>>> + };
>>> + union {
>>> + uint8_t x86_vendor;
>>> + uint8_t vendor;
>>> + };
>>> + uint8_t _rsvd;
>> Can we perhaps name this e.g. _zero, so it's clear that it cannot be
>> repurposed?
>
> It can be repurposed; it just must be done in coordination with VFM_MAKE().
Hmm, true.
> I can add a comment to this effect, but it would need to be in the
> subsequent patch when VFM_MAKE() is introduced.
Okay, and thanks.
Jan
© 2016 - 2025 Red Hat, Inc.