Re: [PATCH 1/3] x86: Rearrange struct cpuinfo_x86 to introduce a vfm field

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx>
> In principle
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> 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