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 <[email protected]>
> ---
> 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
