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.coop...@citrix.com>
> In principle
> Reviewed-by: Jan Beulich <jbeul...@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
