Re: [RFC PATCH 04/11] x86: Refactor early vendor lookup code to use x86_vendor_is()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.11.2025 17:44, Alejandro Vallejo wrote:
> Move the "unknown" vendor ahead of all others and have it NOT rely
> on x86_vendor_is(), as that would yield incorrect values for the
> single-vendor+no-fallback case when running in incorrect hardware
> (because x86_vendor_is() becomes a folded constant of the single
> compiled-in vendor).
> 
> This is one of the two places where x86_vendor_is() cannot be used,
> along with the compatibility check on loaded guest CPU policies.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>
> ---
>  xen/arch/x86/cpu/common.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
> index 393c30227f..c0c3606dd2 100644
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -342,23 +342,38 @@ void __init early_cpu_init(bool verbose)
>  
>       c->x86_vendor = x86_cpuid_lookup_vendor(ebx, ecx, edx) &
>                       X86_ENABLED_VENDORS;
> -     switch (c->x86_vendor) {
> -     case X86_VENDOR_INTEL:    intel_unlock_cpuid_leaves(c);
> -                               actual_cpu = intel_cpu_dev;    break;
> -     case X86_VENDOR_AMD:      actual_cpu = amd_cpu_dev;      break;
> -     case X86_VENDOR_CENTAUR:  actual_cpu = centaur_cpu_dev;  break;
> -     case X86_VENDOR_SHANGHAI: actual_cpu = shanghai_cpu_dev; break;
> -     case X86_VENDOR_HYGON:    actual_cpu = hygon_cpu_dev;    break;
> -     default:
> +
> +     /*
> +      * We can't rely on x86_vendor_is() here due to the single-vendor
> +      * optimisation. It makes x86_vendor_is(x, y) rely on the constant `y`
> +      * matching the single vendor Xen was compiled for and ignore the
> +      * runtime variable `x`. In order to preserve sanity we must assert here
> +      * that we never boot such a build in a CPU from another vendor, or
> +      * major chaos would ensue.
> +      */
> +     if (c->x86_vendor == X86_VENDOR_UNKNOWN)
> +     {

Nit: No mix of styles please. Here it wants to be Linux style.

>               if (verbose || !IS_ENABLED(CONFIG_UNKNOWN_CPU))
>                       printk(XENLOG_ERR
>                              "Unrecognised or unsupported CPU vendor 
> '%.12s'\n",
>                              c->x86_vendor_id);
> +
>               if (!IS_ENABLED(CONFIG_UNKNOWN_CPU))
>                       panic("Cannot run in unknown/compiled-out CPU 
> vendor.\n");
>  
>               actual_cpu = default_cpu;
>       }
> +     else if (x86_vendor_is(c->x86_vendor, X86_VENDOR_INTEL)) {
> +             intel_unlock_cpuid_leaves(c);
> +             actual_cpu = intel_cpu_dev;
> +     } else if (x86_vendor_is(c->x86_vendor, X86_VENDOR_AMD))
> +             actual_cpu = amd_cpu_dev;
> +     else if (x86_vendor_is(c->x86_vendor, X86_VENDOR_CENTAUR))
> +             actual_cpu = centaur_cpu_dev;
> +     else if (x86_vendor_is(c->x86_vendor, X86_VENDOR_SHANGHAI))
> +             actual_cpu = shanghai_cpu_dev;
> +     else if (x86_vendor_is(c->x86_vendor, X86_VENDOR_HYGON))
> +             actual_cpu = hygon_cpu_dev;

If it needs to be like this, then so be it, but I view it as a downside to
not be able to use switch() anymore. It's not quite clear to me though what
extra gains the transformation brings. The masking by X86_ENABLED_VENDORS
already does most of what you want, and X86_VENDOR_UNKNOWN continues to be
handled separately anyway.

Jan