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

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

Hi,

Thanks for taking the time to look at this

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

I didn't expect that to work before transforming the switch, but of course
the prior & X86_ENABLED_VENDORS is already telling the compiler which switch
branches are unreachable.

Will do and send soon as non-RFC if there's no objection to the full vendor
palette (including the unknown vendor case) being on display in Kconfig.

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

Right, what I do in this RFC is have compiled-out vendors fall back onto the
unknown vendor path. Because it really is unknown to the binary.

I could call it GENERIC_CPU_VENDOR, or anything else, but the main question
is whether a toggle for this seems acceptable upstream. I don't see obvious
drawbacks.

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

It wasn't clear to me why __used was there (I assume it shouldn't have been),
but it definitely clashes with the intent of having it gone when UNKNOWN_CPU=n.

If __used was misplaced to begin with I'm happy to get rid of it in a separate
patch. I don't think it warrants a Fixes tag, though.

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

That's intentional. Otherwise further checks of c->x86_vendor in other parts of
the code may not go through the same branch as the one here.

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

ack. As for the text, might as well be just "Unsupported hardware\n". The prior
printk already gives all relevant information. The particulars on the text can
be argued after I send a non-RFC version.

>
> Jan