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

[Alejandro Vallejo <alejandro.garciavallejo@amd.com>]

On Thu Nov 27, 2025 at 11:51 AM CET, Jan Beulich wrote:
> 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@amd.com>
>> ---
>>  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.

In this particular switch that's the case, but it's not so elsewhere.

Any switch would resolve to an unavoidable runtime check with compiled-out
branches left out (so long as the variable is ANDed with the enabled vendors
mask, which it currently isn't). However, this still forces the compiler to emit
a runtime check in case the vendor is actually zero. The conversion to if-elseif
ensures we can statically decide a branch at compile time and remove all others
(including the default one).

On a more stylistic note, though obviously this is all subjective, the patch
that migrates switch statements to if-elseif chains shaves 100 LoC and manages
to squash multiple checks onto single ones by making use of the bitmap nature of
the vendor field. The gains there are marginal, so I don't care that much, about
that but it's a measurable benefit in LoC and a (small) benefit in codegen.

Cheers,
Alejandro