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

diff --git a/xen/arch/x86/Kconfig.cpu b/xen/arch/x86/Kconfig.cpu
index 5fb18db1aa..aaf70fb37b 100644
--- 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"
+	default y
+	help
+	  This option prevents a panic on boot when the host CPU vendor isn't
+	  supported by going into a legacy compatibility mode and not applying
+	  any relevant tunings or quirks.
+
+	  Not selecting this options while selecting multiple vendors doesn't have
+	  any major effect on code size, but while selecting a single vendor
+	  it produces a smaller build especially optimised for size.
+
+	  If unsure, say Y.
+
 endmenu
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 37820a3a08..393c30227f 100644
--- 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 = {
 	.c_init	= default_init,
 };
 static struct cpu_dev __ro_after_init actual_cpu;
@@ -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;
 	switch (c->x86_vendor) {
 	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");
+
 		actual_cpu = default_cpu;
-		if (!verbose)
-			break;
-		printk(XENLOG_ERR
-		       "Unrecognised or unsupported CPU vendor '%.12s'\n",
-		       c->x86_vendor_id);
 	}
 
 	cpuid(0x00000001, &eax, &ebx, &ecx, &edx);
diff --git a/xen/arch/x86/include/asm/cpuid.h b/xen/arch/x86/include/asm/cpuid.h
index f1b9e37a42..bf1c635cdd 100644
--- a/xen/arch/x86/include/asm/cpuid.h
+++ b/xen/arch/x86/include/asm/cpuid.h
@@ -49,6 +49,13 @@ struct cpuid_leaf;
 void guest_cpuid(const struct vcpu *v, uint32_t leaf,
                  uint32_t subleaf, struct cpuid_leaf *res);
 
+#define X86_ENABLED_VENDORS \
+    ((IS_ENABLED(CONFIG_INTEL)    ? X86_VENDOR_INTEL    : 0) | \
+     (IS_ENABLED(CONFIG_AMD)      ? X86_VENDOR_AMD      : 0) | \
+     (IS_ENABLED(CONFIG_CENTAUR)  ? X86_VENDOR_CENTAUR  : 0) | \
+     (IS_ENABLED(CONFIG_SHANGHAI) ? X86_VENDOR_SHANGHAI : 0) | \
+     (IS_ENABLED(CONFIG_HYGON)    ? X86_VENDOR_HYGON    : 0))
+
 #endif /* !__X86_CPUID_H__ */
 
 /*
-- 
2.43.0

On 26/11/2025 4:44 pm, Alejandro Vallejo wrote:
> diff --git a/xen/arch/x86/Kconfig.cpu b/xen/arch/x86/Kconfig.cpu
> index 5fb18db1aa..aaf70fb37b 100644
> --- 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.

Minor nit, but all these 3 should be select AMD/INTEL, not depends. 
It's an implementation detail that they reach sideways into AMD/INTEL to
operate.

An end user in front of menuconfig wants to see all 5 to configure. 
It's unreasonable to require them to know that they need to enable AMD
in order to be able to configure Hygon.

> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
> index 37820a3a08..393c30227f 100644
> --- 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 = {

To follow up on a different question, the __used came from 660f8a75013
but it does look wrong on a second look.  It might have been a leftover
from an older revision of the patch.

~Andrew

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.

Oh, one more thing: There's x86_vendor_is() yet, so what it is going to be
needs at least roughly explaining here.

Jan

On Thu Nov 27, 2025 at 10:59 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.
>
> Oh, one more thing: There's x86_vendor_is() yet, so what it is going to be
> needs at least roughly explaining here.
>
> Jan

Fair enough.

Cheers,
Alejandro

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?

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

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

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

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

Jan

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

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

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

On 27.11.2025 14:44, Alejandro Vallejo wrote:
> 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?

Both have benefits and downsides, I think. The choice may need to be another
Kconfig or command line option. I'm also curious what other x86 maintainers
may think.

Jan