Re: [RFC PATCH 01/11] x86: Add more granularity to the vendors in Kconfig

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.11.2025 17:44, Alejandro Vallejo wrote:
> This enables very aggressive DCE passes on single-vendor builds in later
> patches, as it will allow most vendor checks to become statically chosen
> branches. A lot of statics go away and a lot more inlining is allowed.
> 
> In order to allow x86_vendor_is() to fold into constants, expand Kconfig
> to have the full set of vendors. Adds Hygon, Centaur, Shanghai and the
> default path.
> 
> Have Hygon depend on AMD, and Centaur+Shanghai depend on Intel.
> 
> Not a functional change.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>
> ---
>  xen/arch/x86/Kconfig.cpu         | 45 ++++++++++++++++++++++++++++++++
>  xen/arch/x86/cpu/common.c        | 17 +++++++-----
>  xen/arch/x86/include/asm/cpuid.h |  7 +++++
>  3 files changed, 62 insertions(+), 7 deletions(-)

Shouldn't patch 5 be folded into here? Or, if there were some dependencies
on patches 2-4 (albeit I can't spot anything, as the files are all self-
contained), at least the parts which can be done right away?

> --- a/xen/arch/x86/Kconfig.cpu
> +++ b/xen/arch/x86/Kconfig.cpu
> @@ -19,4 +19,49 @@ config INTEL
>         May be turned off in builds targetting other vendors.  Otherwise,
>         must be enabled for Xen to work suitably on Intel platforms.
>  
> +config HYGON
> +     bool "Support Hygon CPUs"
> +     depends on AMD
> +     default y
> +     help
> +       Detection, tunings and quirks for Hygon platforms.
> +
> +       May be turned off in builds targetting other vendors.  Otherwise,
> +       must be enabled for Xen to work suitably on Hygon platforms.
> +
> +
> +config CENTAUR
> +     bool "Support Centaur CPUs"
> +     depends on INTEL
> +     default y
> +     help
> +       Detection, tunings and quirks for Centaur platforms.
> +
> +       May be turned off in builds targetting other vendors.  Otherwise,
> +       must be enabled for Xen to work suitably on Centaur platforms.
> +
> +config SHANGHAI
> +     bool "Support Shanghai CPUs"
> +     depends on INTEL
> +     default y
> +     help
> +       Detection, tunings and quirks for Shanghai platforms.
> +
> +       May be turned off in builds targetting other vendors.  Otherwise,
> +       must be enabled for Xen to work suitably on Shanghai platforms.
> +
> +config UNKNOWN_CPU
> +     bool "Support unknown CPUs"

"Unknown CPUs" can be of two kinds: Such of vendors we don't explicitly support,
and such of vendors we do explicitly support, but where we aren't aware of the
particular model. This needs to be unambiguous here, perhaps by it becoming
UNKNOWN_CPU_VENDOR (and the prompt changing accordingly).

> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -118,7 +118,7 @@ static void cf_check default_init(struct cpuinfo_x86 * c)
>       __clear_bit(X86_FEATURE_SEP, c->x86_capability);
>  }
>  
> -static const struct cpu_dev __initconst_cf_clobber __used default_cpu = {
> +static const struct cpu_dev __initconst_cf_clobber default_cpu = {

This change isn't explained in the description. __used here was introduced not
all this long ago together with __initconst_cf_clobber. Maybe this really was
a mistake, but if so it's correction should be explained.

> @@ -340,7 +340,8 @@ void __init early_cpu_init(bool verbose)
>       *(u32 *)&c->x86_vendor_id[8] = ecx;
>       *(u32 *)&c->x86_vendor_id[4] = edx;
>  
> -     c->x86_vendor = x86_cpuid_lookup_vendor(ebx, ecx, edx);
> +     c->x86_vendor = x86_cpuid_lookup_vendor(ebx, ecx, edx) &
> +                     X86_ENABLED_VENDORS;

May I suggest the & to move ...

>       switch (c->x86_vendor) {

... here? Yes, you panic() below, but I see no reason to store inaccurate
data when that's easy to avoid.

>       case X86_VENDOR_INTEL:    intel_unlock_cpuid_leaves(c);
>                                 actual_cpu = intel_cpu_dev;    break;
> @@ -349,12 +350,14 @@ void __init early_cpu_init(bool verbose)
>       case X86_VENDOR_SHANGHAI: actual_cpu = shanghai_cpu_dev; break;
>       case X86_VENDOR_HYGON:    actual_cpu = hygon_cpu_dev;    break;
>       default:
> +             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");

The text reads somewhat odd to me, "run in" in particular. Also nit: No full 
stop
please at the end of log messages, except maybe in extraordinary situations.

Jan