[XEN PATCH v1 0/2] x86/CPU: optional build of Intel/AMD CPUs support

[XEN PATCH v1 0/2] x86/CPU: optional build of Intel/AMD CPUs support

[Sergiy Kibrik]

This series is one more step towards separation of Intel/AMD support in Xen.
With all preparation work is mostly done now, it becomes possible to make build
of arch/x86/cpu/{amd.c,intel.c} files optional, depending on whether
CONFIG_{AMD,INTEL} are enabled or not.

This can be useful for builds tuned for specific platform -- by reducing dead
code that won't ever be executed on that platform anyway.

Note: this series (namely its Intel part) depends on optional PSR support patch:
https://lore.kernel.org/xen-devel/20240801084453.1163506-1-Sergiy_Kibrik@epam.com/

  -Sergiy
  
Sergiy Kibrik (2):
  x86/intel: optional build of intel.c
  x86/amd: optional build of amd.c

 xen/arch/x86/cpu/Makefile            | 10 +++++-----
 xen/arch/x86/cpu/common.c            |  8 ++++++--
 xen/arch/x86/include/asm/amd.h       | 22 ++++++++++++++++++++++
 xen/arch/x86/include/asm/processor.h |  7 ++++---
 xen/arch/x86/spec_ctrl.c             |  2 ++
 5 files changed, 39 insertions(+), 10 deletions(-)

-- 
2.25.1

[XEN PATCH v1 1/2] x86/intel: optional build of intel.c

[Sergiy Kibrik]

With specific config option INTEL in place and most of the code that depends
on intel.c now can be optionally enabled/disabled it's now possible to put
the whole intel.c under INTEL option as well. This will allow for a Xen build
without Intel CPU support.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
---
 xen/arch/x86/cpu/Makefile            | 6 +++---
 xen/arch/x86/cpu/common.c            | 4 +++-
 xen/arch/x86/include/asm/processor.h | 7 ++++---
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
index eafce5f204..020c86bda3 100644
--- a/xen/arch/x86/cpu/Makefile
+++ b/xen/arch/x86/cpu/Makefile
@@ -6,10 +6,10 @@ obj-y += amd.o
 obj-y += centaur.o
 obj-y += common.o
 obj-y += hygon.o
-obj-y += intel.o
-obj-y += intel_cacheinfo.o
+obj-$(CONFIG_INTEL) += intel.o
+obj-$(CONFIG_INTEL) += intel_cacheinfo.o
 obj-y += mwait-idle.o
-obj-y += shanghai.o
+obj-$(CONFIG_INTEL) += shanghai.o
 obj-y += vpmu.o
 obj-$(CONFIG_AMD) += vpmu_amd.o
 obj-$(CONFIG_INTEL) += vpmu_intel.o
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index ff4cd22897..50ce13f81c 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -336,11 +336,13 @@ void __init early_cpu_init(bool verbose)
 
 	c->x86_vendor = x86_cpuid_lookup_vendor(ebx, ecx, edx);
 	switch (c->x86_vendor) {
+#ifdef CONFIG_INTEL
 	case X86_VENDOR_INTEL:    intel_unlock_cpuid_leaves(c);
 				  actual_cpu = intel_cpu_dev;    break;
+	case X86_VENDOR_SHANGHAI: actual_cpu = shanghai_cpu_dev; break;
+#endif
 	case X86_VENDOR_AMD:      actual_cpu = amd_cpu_dev;      break;
 	case X86_VENDOR_CENTAUR:  actual_cpu = centaur_cpu_dev;  break;
-	case X86_VENDOR_SHANGHAI: actual_cpu = shanghai_cpu_dev; break;
 	case X86_VENDOR_HYGON:    actual_cpu = hygon_cpu_dev;    break;
 	default:
 		actual_cpu = default_cpu;
diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h
index 66463f6a6d..a88d45252b 100644
--- a/xen/arch/x86/include/asm/processor.h
+++ b/xen/arch/x86/include/asm/processor.h
@@ -507,15 +507,16 @@ static inline uint8_t get_cpu_family(uint32_t raw, uint8_t *model,
 extern int8_t opt_tsx;
 extern bool rtm_disabled;
 void tsx_init(void);
+void update_mcu_opt_ctrl(void);
+void set_in_mcu_opt_ctrl(uint32_t mask, uint32_t val);
 #else
 #define opt_tsx      0     /* explicitly indicate TSX is off */
 #define rtm_disabled false /* RTM was not force-disabled */
 static inline void tsx_init(void) {}
+static inline void update_mcu_opt_ctrl(void) {}
+static inline void set_in_mcu_opt_ctrl(uint32_t mask, uint32_t val) {}
 #endif
 
-void update_mcu_opt_ctrl(void);
-void set_in_mcu_opt_ctrl(uint32_t mask, uint32_t val);
-
 enum ap_boot_method {
     AP_BOOT_NORMAL,
     AP_BOOT_SKINIT,
-- 
2.25.1

[XEN PATCH v1 2/2] x86/amd: optional build of amd.c

[Sergiy Kibrik]

Similar to making Intel CPU support optional -- as we've got CONFIG_AMD option
now, we can put arch/x86/cpu/amd.c under it and make it possible to build
Xen without AMD CPU support. One possible use case is to dispose of dead code
in Intel-only systems.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
---
 xen/arch/x86/cpu/Makefile      |  4 ++--
 xen/arch/x86/cpu/common.c      |  4 +++-
 xen/arch/x86/include/asm/amd.h | 22 ++++++++++++++++++++++
 xen/arch/x86/spec_ctrl.c       |  2 ++
 4 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
index 020c86bda3..5efd87be38 100644
--- a/xen/arch/x86/cpu/Makefile
+++ b/xen/arch/x86/cpu/Makefile
@@ -2,10 +2,10 @@ obj-y += mcheck/
 obj-y += microcode/
 obj-y += mtrr/
 
-obj-y += amd.o
+obj-$(CONFIG_AMD) += amd.o
 obj-y += centaur.o
 obj-y += common.o
-obj-y += hygon.o
+obj-$(CONFIG_AMD) += hygon.o
 obj-$(CONFIG_INTEL) += intel.o
 obj-$(CONFIG_INTEL) += intel_cacheinfo.o
 obj-y += mwait-idle.o
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 50ce13f81c..7be689c2e3 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -341,9 +341,11 @@ void __init early_cpu_init(bool verbose)
 				  actual_cpu = intel_cpu_dev;    break;
 	case X86_VENDOR_SHANGHAI: actual_cpu = shanghai_cpu_dev; break;
 #endif
+#ifdef CONFIG_AMD
 	case X86_VENDOR_AMD:      actual_cpu = amd_cpu_dev;      break;
-	case X86_VENDOR_CENTAUR:  actual_cpu = centaur_cpu_dev;  break;
 	case X86_VENDOR_HYGON:    actual_cpu = hygon_cpu_dev;    break;
+#endif
+	case X86_VENDOR_CENTAUR:  actual_cpu = centaur_cpu_dev;  break;
 	default:
 		actual_cpu = default_cpu;
 		if (!verbose)
diff --git a/xen/arch/x86/include/asm/amd.h b/xen/arch/x86/include/asm/amd.h
index fa4e0fc766..a2481eddc7 100644
--- a/xen/arch/x86/include/asm/amd.h
+++ b/xen/arch/x86/include/asm/amd.h
@@ -158,13 +158,21 @@
 #define is_zen4_uarch()   boot_cpu_has(X86_FEATURE_AUTO_IBRS)
 
 struct cpuinfo_x86;
+#ifdef CONFIG_AMD
 int cpu_has_amd_erratum(const struct cpuinfo_x86 *cpu, int osvw_id, ...);
+#else
+static inline int cpu_has_amd_erratum(const struct cpuinfo_x86 *cpu, int osvw_id, ...)
+{
+    return false;
+}
+#endif
 
 extern s8 opt_allow_unsafe;
 
 void fam10h_check_enable_mmcfg(void);
 void check_enable_amd_mmconf_dmi(void);
 
+#ifdef CONFIG_AMD
 extern bool amd_acpi_c1e_quirk;
 void amd_check_disable_c1e(unsigned int port, u8 value);
 
@@ -173,5 +181,19 @@ extern bool amd_virt_spec_ctrl;
 bool amd_setup_legacy_ssbd(void);
 void amd_set_legacy_ssbd(bool enable);
 void amd_set_cpuid_user_dis(bool enable);
+#else
+static inline void amd_set_cpuid_user_dis(bool enable) {}
+static inline void amd_set_legacy_ssbd(bool enable) {}
+static inline bool amd_setup_legacy_ssbd(void)
+{
+    return false;
+}
+
+#define amd_acpi_c1e_quirk (false)
+#define amd_virt_spec_ctrl (false)
+#define amd_legacy_ssbd (false)
+
+static inline void amd_check_disable_c1e(unsigned int port, u8 value) {}
+#endif
 
 #endif /* __AMD_H__ */
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 92405b8be7..8231515c80 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -1884,10 +1884,12 @@ void __init init_speculation_mitigations(void)
             setup_force_cpu_cap(X86_FEATURE_SC_MSR_HVM);
     }
 
+#ifdef CONFIG_AMD
     /* Support VIRT_SPEC_CTRL.SSBD if AMD_SSBD is not available. */
     if ( opt_msr_sc_hvm && !cpu_has_amd_ssbd &&
          (cpu_has_virt_ssbd || (amd_legacy_ssbd && amd_setup_legacy_ssbd())) )
         amd_virt_spec_ctrl = true;
+#endif
 
     /* Figure out default_xen_spec_ctrl. */
     if ( has_spec_ctrl && ibrs )
-- 
2.25.1

Re: [XEN PATCH v1 1/2] x86/intel: optional build of intel.c

[Alejandro Vallejo]

On Fri Aug 9, 2024 at 11:09 AM BST, Sergiy Kibrik wrote:
> With specific config option INTEL in place and most of the code that depends
> on intel.c now can be optionally enabled/disabled it's now possible to put
> the whole intel.c under INTEL option as well. This will allow for a Xen build
> without Intel CPU support.
>
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> ---
>  xen/arch/x86/cpu/Makefile            | 6 +++---
>  xen/arch/x86/cpu/common.c            | 4 +++-
>  xen/arch/x86/include/asm/processor.h | 7 ++++---
>  3 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
> index eafce5f204..020c86bda3 100644
> --- a/xen/arch/x86/cpu/Makefile
> +++ b/xen/arch/x86/cpu/Makefile
> @@ -6,10 +6,10 @@ obj-y += amd.o
>  obj-y += centaur.o
>  obj-y += common.o
>  obj-y += hygon.o
> -obj-y += intel.o
> -obj-y += intel_cacheinfo.o
> +obj-$(CONFIG_INTEL) += intel.o
> +obj-$(CONFIG_INTEL) += intel_cacheinfo.o
>  obj-y += mwait-idle.o
> -obj-y += shanghai.o
> +obj-$(CONFIG_INTEL) += shanghai.o

Why pick this one too? It's based on VIA IP, aiui.

>  obj-y += vpmu.o
>  obj-$(CONFIG_AMD) += vpmu_amd.o
>  obj-$(CONFIG_INTEL) += vpmu_intel.o
> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
> index ff4cd22897..50ce13f81c 100644
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -336,11 +336,13 @@ void __init early_cpu_init(bool verbose)
>  
>  	c->x86_vendor = x86_cpuid_lookup_vendor(ebx, ecx, edx);
>  	switch (c->x86_vendor) {
> +#ifdef CONFIG_INTEL
>  	case X86_VENDOR_INTEL:    intel_unlock_cpuid_leaves(c);
>  				  actual_cpu = intel_cpu_dev;    break;
> +	case X86_VENDOR_SHANGHAI: actual_cpu = shanghai_cpu_dev; break;
> +#endif
>  	case X86_VENDOR_AMD:      actual_cpu = amd_cpu_dev;      break;
>  	case X86_VENDOR_CENTAUR:  actual_cpu = centaur_cpu_dev;  break;
> -	case X86_VENDOR_SHANGHAI: actual_cpu = shanghai_cpu_dev; break;
>  	case X86_VENDOR_HYGON:    actual_cpu = hygon_cpu_dev;    break;
>  	default:
>  		actual_cpu = default_cpu;
> diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h
> index 66463f6a6d..a88d45252b 100644
> --- a/xen/arch/x86/include/asm/processor.h
> +++ b/xen/arch/x86/include/asm/processor.h
> @@ -507,15 +507,16 @@ static inline uint8_t get_cpu_family(uint32_t raw, uint8_t *model,
>  extern int8_t opt_tsx;
>  extern bool rtm_disabled;
>  void tsx_init(void);
> +void update_mcu_opt_ctrl(void);
> +void set_in_mcu_opt_ctrl(uint32_t mask, uint32_t val);
>  #else
>  #define opt_tsx      0     /* explicitly indicate TSX is off */
>  #define rtm_disabled false /* RTM was not force-disabled */
>  static inline void tsx_init(void) {}
> +static inline void update_mcu_opt_ctrl(void) {}
> +static inline void set_in_mcu_opt_ctrl(uint32_t mask, uint32_t val) {}
>  #endif
>  
> -void update_mcu_opt_ctrl(void);
> -void set_in_mcu_opt_ctrl(uint32_t mask, uint32_t val);
> -
>  enum ap_boot_method {
>      AP_BOOT_NORMAL,
>      AP_BOOT_SKINIT,


Cheers,
Alejandro

Re: [XEN PATCH v1 1/2] x86/intel: optional build of intel.c

[Sergiy Kibrik]

09.08.24 13:36, Alejandro Vallejo:
> On Fri Aug 9, 2024 at 11:09 AM BST, Sergiy Kibrik wrote:
>> With specific config option INTEL in place and most of the code that depends
>> on intel.c now can be optionally enabled/disabled it's now possible to put
>> the whole intel.c under INTEL option as well. This will allow for a Xen build
>> without Intel CPU support.
>>
>> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
>> ---
>>   xen/arch/x86/cpu/Makefile            | 6 +++---
>>   xen/arch/x86/cpu/common.c            | 4 +++-
>>   xen/arch/x86/include/asm/processor.h | 7 ++++---
>>   3 files changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
>> index eafce5f204..020c86bda3 100644
>> --- a/xen/arch/x86/cpu/Makefile
>> +++ b/xen/arch/x86/cpu/Makefile
>> @@ -6,10 +6,10 @@ obj-y += amd.o
>>   obj-y += centaur.o
>>   obj-y += common.o
>>   obj-y += hygon.o
>> -obj-y += intel.o
>> -obj-y += intel_cacheinfo.o
>> +obj-$(CONFIG_INTEL) += intel.o
>> +obj-$(CONFIG_INTEL) += intel_cacheinfo.o
>>   obj-y += mwait-idle.o
>> -obj-y += shanghai.o
>> +obj-$(CONFIG_INTEL) += shanghai.o
> 
> Why pick this one too? It's based on VIA IP, aiui.

shanghai.c and intel.c both use init_intel_cacheinfo() routine, so 
there's build dependency on Intel code.

   -Sergiy

Re: [XEN PATCH v1 1/2] x86/intel: optional build of intel.c

[Jan Beulich]

On 12.08.2024 11:40, Sergiy Kibrik wrote:
> 09.08.24 13:36, Alejandro Vallejo:
>> On Fri Aug 9, 2024 at 11:09 AM BST, Sergiy Kibrik wrote:
>>> --- a/xen/arch/x86/cpu/Makefile
>>> +++ b/xen/arch/x86/cpu/Makefile
>>> @@ -6,10 +6,10 @@ obj-y += amd.o
>>>   obj-y += centaur.o
>>>   obj-y += common.o
>>>   obj-y += hygon.o
>>> -obj-y += intel.o
>>> -obj-y += intel_cacheinfo.o
>>> +obj-$(CONFIG_INTEL) += intel.o
>>> +obj-$(CONFIG_INTEL) += intel_cacheinfo.o
>>>   obj-y += mwait-idle.o
>>> -obj-y += shanghai.o
>>> +obj-$(CONFIG_INTEL) += shanghai.o
>>
>> Why pick this one too? It's based on VIA IP, aiui.
> 
> shanghai.c and intel.c both use init_intel_cacheinfo() routine, so 
> there's build dependency on Intel code.

Yet Shanghai isn't as directly a clone of Intel CPUs as Hygon ones are
for AMD. So at the very least you want to justify your choice in the
description. After all there's also the alternative of having a separate
SHANGHAI Kconfig setting, which would merely have "select INTEL" or
"depends on INTEL".

Jan

Re: [XEN PATCH v1 1/2] x86/intel: optional build of intel.c

[Alejandro Vallejo]

On Mon Aug 12, 2024 at 10:58 AM BST, Jan Beulich wrote:
> On 12.08.2024 11:40, Sergiy Kibrik wrote:
> > 09.08.24 13:36, Alejandro Vallejo:
> >> On Fri Aug 9, 2024 at 11:09 AM BST, Sergiy Kibrik wrote:
> >>> --- a/xen/arch/x86/cpu/Makefile
> >>> +++ b/xen/arch/x86/cpu/Makefile
> >>> @@ -6,10 +6,10 @@ obj-y += amd.o
> >>>   obj-y += centaur.o
> >>>   obj-y += common.o
> >>>   obj-y += hygon.o
> >>> -obj-y += intel.o
> >>> -obj-y += intel_cacheinfo.o
> >>> +obj-$(CONFIG_INTEL) += intel.o
> >>> +obj-$(CONFIG_INTEL) += intel_cacheinfo.o
> >>>   obj-y += mwait-idle.o
> >>> -obj-y += shanghai.o
> >>> +obj-$(CONFIG_INTEL) += shanghai.o
> >>
> >> Why pick this one too? It's based on VIA IP, aiui.
> > 
> > shanghai.c and intel.c both use init_intel_cacheinfo() routine, so 
> > there's build dependency on Intel code.

My point is that the use of Intel functions on Shanghai and not Centaur is
accidental. If shanghai goes under Intel so should Centaur (imo).

>
> Yet Shanghai isn't as directly a clone of Intel CPUs as Hygon ones are
> for AMD. So at the very least you want to justify your choice in the
> description. After all there's also the alternative of having a separate
> SHANGHAI Kconfig setting, which would merely have "select INTEL" or
> "depends on INTEL".
>
> Jan

That's one option, another is for the Kconfig options to explicitly state which
vendors they apply to. I'd be fine with either. It's less fine for CONFIG_INTEL
to cover a VIA derivative and not the other.

Cheers,
Alejandro

Re: [XEN PATCH v1 1/2] x86/intel: optional build of intel.c

[Sergiy Kibrik]

12.08.24 15:24, Alejandro Vallejo:
> On Mon Aug 12, 2024 at 10:58 AM BST, Jan Beulich wrote:
>> On 12.08.2024 11:40, Sergiy Kibrik wrote:
>>> 09.08.24 13:36, Alejandro Vallejo:
>>>> On Fri Aug 9, 2024 at 11:09 AM BST, Sergiy Kibrik wrote:
>>>>> --- a/xen/arch/x86/cpu/Makefile
>>>>> +++ b/xen/arch/x86/cpu/Makefile
>>>>> @@ -6,10 +6,10 @@ obj-y += amd.o
>>>>>    obj-y += centaur.o
>>>>>    obj-y += common.o
>>>>>    obj-y += hygon.o
>>>>> -obj-y += intel.o
>>>>> -obj-y += intel_cacheinfo.o
>>>>> +obj-$(CONFIG_INTEL) += intel.o
>>>>> +obj-$(CONFIG_INTEL) += intel_cacheinfo.o
>>>>>    obj-y += mwait-idle.o
>>>>> -obj-y += shanghai.o
>>>>> +obj-$(CONFIG_INTEL) += shanghai.o
>>>>
>>>> Why pick this one too? It's based on VIA IP, aiui.
>>>
>>> shanghai.c and intel.c both use init_intel_cacheinfo() routine, so
>>> there's build dependency on Intel code.
> 
> My point is that the use of Intel functions on Shanghai and not Centaur is
> accidental. If shanghai goes under Intel so should Centaur (imo).
> 
>>
>> Yet Shanghai isn't as directly a clone of Intel CPUs as Hygon ones are
>> for AMD. So at the very least you want to justify your choice in the
>> description. After all there's also the alternative of having a separate
>> SHANGHAI Kconfig setting, which would merely have "select INTEL" or
>> "depends on INTEL".
>>
>> Jan
> 
> That's one option, another is for the Kconfig options to explicitly state which
> vendors they apply to. I'd be fine with either. It's less fine for CONFIG_INTEL
> to cover a VIA derivative and not the other.
> 

I think I'll go with separate kconfig options -- for Centaur, Shanghai & 
Hygon, as we already got a separate submenu for CPUs & everything.
Also this way it's more up to the task of config tuning.

Thanks for clarification!

    -Sergiy

Re: [XEN PATCH v1 1/2] x86/intel: optional build of intel.c

[Jan Beulich]

On 09.08.2024 12:09, Sergiy Kibrik wrote:
> With specific config option INTEL in place and most of the code that depends
> on intel.c now can be optionally enabled/disabled it's now possible to put
> the whole intel.c under INTEL option as well. This will allow for a Xen build
> without Intel CPU support.
> 
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> ---
>  xen/arch/x86/cpu/Makefile            | 6 +++---
>  xen/arch/x86/cpu/common.c            | 4 +++-
>  xen/arch/x86/include/asm/processor.h | 7 ++++---
>  3 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
> index eafce5f204..020c86bda3 100644
> --- a/xen/arch/x86/cpu/Makefile
> +++ b/xen/arch/x86/cpu/Makefile
> @@ -6,10 +6,10 @@ obj-y += amd.o
>  obj-y += centaur.o
>  obj-y += common.o
>  obj-y += hygon.o
> -obj-y += intel.o
> -obj-y += intel_cacheinfo.o
> +obj-$(CONFIG_INTEL) += intel.o
> +obj-$(CONFIG_INTEL) += intel_cacheinfo.o
>  obj-y += mwait-idle.o
> -obj-y += shanghai.o
> +obj-$(CONFIG_INTEL) += shanghai.o
>  obj-y += vpmu.o
>  obj-$(CONFIG_AMD) += vpmu_amd.o
>  obj-$(CONFIG_INTEL) += vpmu_intel.o
> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
> index ff4cd22897..50ce13f81c 100644
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -336,11 +336,13 @@ void __init early_cpu_init(bool verbose)
>  
>  	c->x86_vendor = x86_cpuid_lookup_vendor(ebx, ecx, edx);
>  	switch (c->x86_vendor) {
> +#ifdef CONFIG_INTEL
>  	case X86_VENDOR_INTEL:    intel_unlock_cpuid_leaves(c);
>  				  actual_cpu = intel_cpu_dev;    break;
> +	case X86_VENDOR_SHANGHAI: actual_cpu = shanghai_cpu_dev; break;
> +#endif
>  	case X86_VENDOR_AMD:      actual_cpu = amd_cpu_dev;      break;
>  	case X86_VENDOR_CENTAUR:  actual_cpu = centaur_cpu_dev;  break;
> -	case X86_VENDOR_SHANGHAI: actual_cpu = shanghai_cpu_dev; break;
>  	case X86_VENDOR_HYGON:    actual_cpu = hygon_cpu_dev;    break;
>  	default:
>  		actual_cpu = default_cpu;
> diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h
> index 66463f6a6d..a88d45252b 100644
> --- a/xen/arch/x86/include/asm/processor.h
> +++ b/xen/arch/x86/include/asm/processor.h
> @@ -507,15 +507,16 @@ static inline uint8_t get_cpu_family(uint32_t raw, uint8_t *model,
>  extern int8_t opt_tsx;
>  extern bool rtm_disabled;
>  void tsx_init(void);
> +void update_mcu_opt_ctrl(void);
> +void set_in_mcu_opt_ctrl(uint32_t mask, uint32_t val);
>  #else
>  #define opt_tsx      0     /* explicitly indicate TSX is off */
>  #define rtm_disabled false /* RTM was not force-disabled */
>  static inline void tsx_init(void) {}
> +static inline void update_mcu_opt_ctrl(void) {}
> +static inline void set_in_mcu_opt_ctrl(uint32_t mask, uint32_t val) {}
>  #endif
>  
> -void update_mcu_opt_ctrl(void);
> -void set_in_mcu_opt_ctrl(uint32_t mask, uint32_t val);

I'm okay-ish with the simple stubbing out for update_mcu_opt_ctrl(), but
set_in_mcu_opt_ctrl() imo requires more work. The call sites in spec_ctrl.c
shouldn't give the wrong impression of having some effect. Imo in
init_speculation_mitigations() an #endif wants to move down, while all of
gds_calculations() wants to be compiled out. And that likely extends to
further Intel-only functions there (with an early bail-out keyed to
boot_cpu_data.x86_vendor != X86_VENDOR_INTEL). Which overall likely means
there wants to be another separate patch dealing with that. (And then
maybe a counterpart one for AMD.)

Jan

Re: [XEN PATCH v1 2/2] x86/amd: optional build of amd.c

[Jan Beulich]

On 09.08.2024 12:11, Sergiy Kibrik wrote:
> --- a/xen/arch/x86/include/asm/amd.h
> +++ b/xen/arch/x86/include/asm/amd.h
> @@ -158,13 +158,21 @@
>  #define is_zen4_uarch()   boot_cpu_has(X86_FEATURE_AUTO_IBRS)
>  
>  struct cpuinfo_x86;
> +#ifdef CONFIG_AMD
>  int cpu_has_amd_erratum(const struct cpuinfo_x86 *cpu, int osvw_id, ...);
> +#else
> +static inline int cpu_has_amd_erratum(const struct cpuinfo_x86 *cpu, int osvw_id, ...)

Nit: Too long line.

> +{
> +    return false;

I wouldn't mind if you consistently changed the function to return
bool, but as long as it returns int I don't think it should be returning
false.

> @@ -173,5 +181,19 @@ extern bool amd_virt_spec_ctrl;
>  bool amd_setup_legacy_ssbd(void);
>  void amd_set_legacy_ssbd(bool enable);
>  void amd_set_cpuid_user_dis(bool enable);
> +#else
> +static inline void amd_set_cpuid_user_dis(bool enable) {}
> +static inline void amd_set_legacy_ssbd(bool enable) {}
> +static inline bool amd_setup_legacy_ssbd(void)
> +{
> +    return false;
> +}

Nit: Would be a little nicer if these were in the same order as their
corresponding declarations. However, along the lines of one of my
comments on the Intel counterpart patch ...

> +#define amd_acpi_c1e_quirk (false)
> +#define amd_virt_spec_ctrl (false)
> +#define amd_legacy_ssbd (false)
> +
> +static inline void amd_check_disable_c1e(unsigned int port, u8 value) {}
> +#endif

... question overall is how many of these stubs are really required,
once clearly AMD-only code is properly taken care of (perhaps not just
in spec_ctrl.c).

Jan

Re: [XEN PATCH v1 2/2] x86/amd: optional build of amd.c

[Sergiy Kibrik]

13.08.24 10:50, Jan Beulich:
> On 09.08.2024 12:11, Sergiy Kibrik wrote:
>> --- a/xen/arch/x86/include/asm/amd.h
>> +++ b/xen/arch/x86/include/asm/amd.h
>> @@ -158,13 +158,21 @@
>>   #define is_zen4_uarch()   boot_cpu_has(X86_FEATURE_AUTO_IBRS)
>>   
>>   struct cpuinfo_x86;
>> +#ifdef CONFIG_AMD
>>   int cpu_has_amd_erratum(const struct cpuinfo_x86 *cpu, int osvw_id, ...);
>> +#else
>> +static inline int cpu_has_amd_erratum(const struct cpuinfo_x86 *cpu, int osvw_id, ...)
> 
> Nit: Too long line.
> 
>> +{
>> +    return false;
> 
> I wouldn't mind if you consistently changed the function to return
> bool, but as long as it returns int I don't think it should be returning
> false.
> 

indeed, it should return 0

>> @@ -173,5 +181,19 @@ extern bool amd_virt_spec_ctrl;
>>   bool amd_setup_legacy_ssbd(void);
>>   void amd_set_legacy_ssbd(bool enable);
>>   void amd_set_cpuid_user_dis(bool enable);
>> +#else
>> +static inline void amd_set_cpuid_user_dis(bool enable) {}
>> +static inline void amd_set_legacy_ssbd(bool enable) {}
>> +static inline bool amd_setup_legacy_ssbd(void)
>> +{
>> +    return false;
>> +}
> 
> Nit: Would be a little nicer if these were in the same order as their
> corresponding declarations. However, along the lines of one of my
> comments on the Intel counterpart patch ...
> 
>> +#define amd_acpi_c1e_quirk (false)
>> +#define amd_virt_spec_ctrl (false)
>> +#define amd_legacy_ssbd (false)
>> +
>> +static inline void amd_check_disable_c1e(unsigned int port, u8 value) {}
>> +#endif
> 
> ... question overall is how many of these stubs are really required,
> once clearly AMD-only code is properly taken care of (perhaps not just
> in spec_ctrl.c).
> 

most of these functions-stubs can go away, though it'll require more 
CONFIG_AMD checks at call sites, and more patches probably.

   -Sergiy

Re: [XEN PATCH v1 1/2] x86/intel: optional build of intel.c

[Sergiy Kibrik]

13.08.24 10:40, Jan Beulich:
> I'm okay-ish with the simple stubbing out for update_mcu_opt_ctrl(), but
> set_in_mcu_opt_ctrl() imo requires more work. The call sites in spec_ctrl.c
> shouldn't give the wrong impression of having some effect. Imo in
> init_speculation_mitigations() an #endif wants to move down, while all of
> gds_calculations() wants to be compiled out. And that likely extends to
> further Intel-only functions there (with an early bail-out keyed to
> boot_cpu_data.x86_vendor != X86_VENDOR_INTEL). Which overall likely means
> there wants to be another separate patch dealing with that. (And then
> maybe a counterpart one for AMD.)

Thanks for a suggestion, indeed there are many routines there that can 
be put under CONFIG_{AMD,INTEL}.

   -Sergiy