Re: [PATCH 06/12] x86: Migrate switch vendor checks to cpu_vendor()

[Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>]

On Thu Feb 12, 2026 at 12:06 PM CET, Jan Beulich wrote:
> On 06.02.2026 17:15, Alejandro Vallejo wrote:
>> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
>> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
>> @@ -133,7 +133,7 @@ static int __init cf_check cpufreq_driver_init(void)
>>  
>>          ret = -ENOENT;
>>  
>> -        switch ( boot_cpu_data.x86_vendor )
>> +        switch( cpu_vendor() )
>
> Nit: Please avoid screwing up style.

bah, yes.

>
>> @@ -141,12 +141,10 @@ static int __init cf_check cpufreq_driver_init(void)
>>                  switch ( cpufreq_xen_opts[i] )
>>                  {
>>                  case CPUFREQ_xen:
>> -                    ret = IS_ENABLED(CONFIG_INTEL) ?
>> -                          acpi_cpufreq_register() : -ENODEV;
>> +                    ret = acpi_cpufreq_register();
>>                      break;
>>                  case CPUFREQ_hwp:
>> -                    ret = IS_ENABLED(CONFIG_INTEL) ?
>> -                          hwp_register_driver() : -ENODEV;
>> +                    ret = hwp_register_driver();
>>                      break;
>>                  case CPUFREQ_none:
>>                      ret = 0;
>
> This of course is a neat (side) effect.
>
>> @@ -165,7 +163,6 @@ static int __init cf_check cpufreq_driver_init(void)
>>  
>>          case X86_VENDOR_AMD:
>>          case X86_VENDOR_HYGON:
>> -#ifdef CONFIG_AMD
>>              for ( i = 0; i < cpufreq_xen_cnt; i++ )
>>              {
>>                  switch ( cpufreq_xen_opts[i] )
>> @@ -191,9 +188,6 @@ static int __init cf_check cpufreq_driver_init(void)
>>                  if ( !ret || ret == -EBUSY )
>>                      break;
>>              }
>> -#else
>> -            ret = -ENODEV;
>> -#endif /* CONFIG_AMD */
>>              break;
>>  
>>          default:
>
> There's a change to the function's return value, though: When reaching 
> default:,
> -ENOENT will result, when previously -ENODEV would have been returned for
> compiled-out cases. It may well be that there is a dependency somewhere on the
> particular return value - did you thoroughly check that?

It's a presmp_initcall. The specific ret value is inconsequential.

>
> Of course this may well apply elsewhere as well; I did not go through and 
> check
> every of the switch()es you alter.

It may. I did check, but that doesn't mean I didn't miss any.

>
>> --- a/xen/arch/x86/cpu/mcheck/mce.h
>> +++ b/xen/arch/x86/cpu/mcheck/mce.h
>> @@ -137,7 +137,7 @@ void x86_mcinfo_dump(struct mc_info *mi);
>>  
>>  static inline int mce_vendor_bank_msr(const struct vcpu *v, uint32_t msr)
>>  {
>> -    switch (boot_cpu_data.x86_vendor) {
>> +    switch (cpu_vendor()) {
>
> Would be nice if style was updated while touching such.

sure.

>
>> --- a/xen/arch/x86/cpu/vpmu.c
>> +++ b/xen/arch/x86/cpu/vpmu.c
>> @@ -815,7 +815,7 @@ static struct notifier_block cpu_nfb = {
>>  
>>  static int __init cf_check vpmu_init(void)
>>  {
>> -    int vendor = current_cpu_data.x86_vendor;
>> +    int vendor = cpu_vendor();
>
> It is only seeing the plain int here that I notice that cpu_vendor() returns
> uint8_t. I don't think that's necessary, and hence as per ./CODING_STYLE it
> should rather be unsigned int. Which is then waht would want using here as
> well.

Hmm, I need to check whether it affects codegen. Probably not seeing how its
all inline. Will do unless it has terrible side effects.

>
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -319,7 +319,7 @@ void domain_cpu_policy_changed(struct domain *d)
>>              if ( cpu_has_htt )
>>                  edx |= cpufeat_mask(X86_FEATURE_HTT);
>>  
>> -            switch ( boot_cpu_data.x86_vendor )
>> +            switch( cpu_vendor() )
>>              {
>>              case X86_VENDOR_INTEL:
>>                  /*
>> @@ -427,7 +427,7 @@ void domain_cpu_policy_changed(struct domain *d)
>>              if ( !(p->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) )
>>                  edx &= ~CPUID_COMMON_1D_FEATURES;
>>  
>> -            switch ( boot_cpu_data.x86_vendor )
>> +            switch( cpu_vendor() )
> As they recur, I wonder where these bogus style adjustments are coming from.
> It's not like ...
>
>> --- a/xen/arch/x86/guest/xen/xen.c
>> +++ b/xen/arch/x86/guest/xen/xen.c
>> @@ -63,7 +63,7 @@ void asmlinkage __init early_hypercall_setup(void)
>>                    x86_cpuid_vendor_to_str(boot_cpu_data.x86_vendor));
>>      }
>>  
>> -    switch ( boot_cpu_data.x86_vendor )
>> +    switch ( cpu_vendor() )
>
> ... you would have used a bad sed pattern globally, as here style remains
> intact. Further down it breaks again.

This patch used to be several. One of the primordial commits seems to suffer
from this. Will fix globally. Thanks.

Alejandro