Re: [PATCH v2] x86: Remove x86 prefixed names from cpuinfo for intel.c

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.11.2025 21:27, Kevin Lampis wrote:
> struct cpuinfo_x86
>   .x86        => .family
>   .x86_vendor => .vendor
>   .x86_model  => .model
>   .x86_mask   => .stepping
> 
> No functional change.
> 
> Signed-off-by: Kevin Lampis <kevin.lampis@xxxxxxxxxx>
> ---
> Changes in v2:
> - Convert the two switch statements in probe_masking_msrs()
>   and check_memory_type_self_snoop_errata()
> - Requested style changes

As to the latter - not quite, see below.

> --- a/xen/arch/x86/cpu/intel.c
> +++ b/xen/arch/x86/cpu/intel.c
> @@ -86,18 +86,19 @@ static void __init 
> check_memory_type_self_snoop_errata(void)
>       if (!boot_cpu_has(X86_FEATURE_SS))
>               return;
>  
> -     switch (boot_cpu_data.x86_model) {
> -     case 0x0f: /* Merom */
> -     case 0x16: /* Merom L */
> -     case 0x17: /* Penryn */
> -     case 0x1d: /* Dunnington */
> -     case 0x1e: /* Nehalem */
> -     case 0x1f: /* Auburndale / Havendale */
> -     case 0x1a: /* Nehalem EP */
> -     case 0x2e: /* Nehalem EX */
> -     case 0x25: /* Westmere */
> -     case 0x2c: /* Westmere EP */
> -     case 0x2a: /* SandyBridge */
> +     switch ( boot_cpu_data.vfm )
> +     {
> +     case INTEL_CORE2_MEROM:
> +     case INTEL_CORE2_MEROM_L:
> +     case INTEL_CORE2_PENRYN:
> +     case INTEL_CORE2_DUNNINGTON:
> +     case INTEL_NEHALEM:
> +     case INTEL_NEHALEM_G:
> +     case INTEL_NEHALEM_EP:
> +     case INTEL_NEHALEM_EX:
> +     case INTEL_WESTMERE:
> +     case INTEL_WESTMERE_EP:
> +     case INTEL_SANDYBRIDGE:

The ordering here would imo ...

> @@ -137,28 +138,29 @@ static void __init probe_masking_msrs(void)
>       unsigned int exp_msr_basic, exp_msr_ext, exp_msr_xsave;
>  
>       /* Only family 6 supports this feature. */
> -     if (c->x86 != 6)
> +     if (c->family != 6)
>               return;
>  
> -     switch (c->x86_model) {
> -     case 0x17: /* Yorkfield, Wolfdale, Penryn, Harpertown(DP) */
> -     case 0x1d: /* Dunnington(MP) */
> +     switch ( c->vfm )
> +     {
> +     case INTEL_CORE2_PENRYN:
> +     case INTEL_CORE2_DUNNINGTON:
>               msr_basic = MSR_INTEL_MASK_V1_CPUID1;
>               break;
>  
> -     case 0x1a: /* Bloomfield, Nehalem-EP(Gainestown) */
> -     case 0x1e: /* Clarksfield, Lynnfield, Jasper Forest */
> -     case 0x1f: /* Something Nehalem-based - perhaps Auburndale/Havendale? */
> -     case 0x25: /* Arrandale, Clarksdale */
> -     case 0x2c: /* Gulftown, Westmere-EP */
> -     case 0x2e: /* Nehalem-EX(Beckton) */
> -     case 0x2f: /* Westmere-EX */
> +     case INTEL_NEHALEM_EP:
> +     case INTEL_NEHALEM:
> +     case INTEL_NEHALEM_G:
> +     case INTEL_WESTMERE:
> +     case INTEL_WESTMERE_EP:
> +     case INTEL_NEHALEM_EX:
> +     case INTEL_WESTMERE_EX:

... best also be followed here, even if that means some re-ordering
compared to what original code had.

> @@ -572,8 +580,13 @@ static void intel_log_freq(const struct cpuinfo_x86 *c)
>              factor = core_factors[msrval];
>              break;
>  
> -        case 0x1a: case 0x1e: case 0x1f: case 0x2e: /* Nehalem */
> -        case 0x25: case 0x2c: case 0x2f: /* Westmere */
> +        case INTEL_NEHALEM_EP:
> +        case INTEL_NEHALEM:
> +        case INTEL_NEHALEM_G:
> +        case INTEL_NEHALEM_EX:
> +        case INTEL_WESTMERE:
> +        case INTEL_WESTMERE_EP:
> +        case INTEL_WESTMERE_EX:
>              factor = 13333;
>              break;

Same here. (This iirc also wasn't there in v1, but isn't mentioned in
the changelog.)

> @@ -657,14 +670,17 @@ static void cf_check init_intel(struct cpuinfo_x86 *c)
>       /* Work around errata */
>       Intel_errata_workarounds(c);
>  
> -     if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
> -             (c->x86 == 0x6 && c->x86_model >= 0x0e))
> -             __set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability);
> -     if (cpu_has(c, X86_FEATURE_ITSC)) {
> -             __set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability);
> -             __set_bit(X86_FEATURE_NONSTOP_TSC, c->x86_capability);
> -             __set_bit(X86_FEATURE_TSC_RELIABLE, c->x86_capability);
> -     }
> +    if ( ( c->family == 15 && c->model >= 0x03 ) ||
> +         ( c->family == 6 && c->model >= 0x0e ) )
> +        __set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability);
> +
> +    if ( cpu_has(c, X86_FEATURE_ITSC) )
> +    {
> +        __set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability);
> +        __set_bit(X86_FEATURE_NONSTOP_TSC, c->x86_capability);
> +        __set_bit(X86_FEATURE_TSC_RELIABLE, c->x86_capability);
> +    }
> +
>       if ( opt_arat &&
>            ( c->cpuid_level >= 0x00000006 ) &&
>            ( cpuid_eax(0x00000006) & (1u<<2) ) )

>From your v1 reply I concluded that you understood that this isn't the way
to go. Within a function, indentation shouldn't vary like this. I suggest
anyway that you really wait with submitting a new version until discussion
has settled.

Jan