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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

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

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

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

Jan