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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

Jan