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

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

On Thu Nov 27, 2025 at 2:19 PM CET, Jan Beulich wrote:
> On 27.11.2025 13:36, Alejandro Vallejo wrote:
>> On Thu Nov 27, 2025 at 10:43 AM CET, Jan Beulich wrote:
>>> On 26.11.2025 17:44, Alejandro Vallejo wrote:
>>>> --- 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.
>
> I'd recommend against "generic" or anything alike, as it'll rather suggest any
> vendor's CPU will work reasonably. I'm fine with "unknown", just that the nature
> of the unknown-ness needs making unambiguous.

Got it, if UNKNOWN_CPU_VENDOR sounds better I'm fine with that.

What are your thoughts on the panic-on-compiled-out-vendor vs use-unknown?

>
>>>> --- 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.
>
> I can only vaguely reconstruct that it may have been put there so the
> .init.rodata.cf_clobber entry wouldn't go away. But as long as the compiler
> also eliminates the function pointed at, that would be of no concern.

ack.

>
>>>> @@ -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.
>
> Hmm. I would kind of expect x86_vendor_is() to be capable of dealing with
> that.

Sure, but that's not done until the end of the series. and otherwise I'd be
introducing the fallback behaviour in some places and not others. I could
remove this AND at the end. Or remove it altogether if we go for a panic on
compiled-out vendor strategy.

Cheers,
Aljandro