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
