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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> --- 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