[XEN PATCH v2 1/5] x86/Kconfig: introduce CENTAUR, HYGON & SHANGHAI config options

[XEN PATCH v2 1/5] x86/Kconfig: introduce CENTAUR, HYGON & SHANGHAI config options

[Sergiy Kibrik]

These options aim to represent what's currently supported by Xen, and later
to allow tuning for specific platform(s) only.

HYGON and SHANGHAI options depend on AMD and INTEL as there're build
dependencies on support code for AMD and Intel CPUs respectively.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/Kconfig.cpu  | 29 +++++++++++++++++++++++++++++
 xen/arch/x86/cpu/Makefile |  6 +++---
 xen/arch/x86/cpu/common.c |  6 ++++++
 3 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/Kconfig.cpu b/xen/arch/x86/Kconfig.cpu
index 5fb18db1aa..ac8f41d464 100644
--- a/xen/arch/x86/Kconfig.cpu
+++ b/xen/arch/x86/Kconfig.cpu
@@ -10,6 +10,25 @@ config AMD
 	  May be turned off in builds targetting other vendors.  Otherwise,
 	  must be enabled for Xen to work suitably on AMD platforms.
 
+config CENTAUR
+	bool "Support Centaur CPUs"
+	default y
+	help
+	  Detection, tunings and quirks for VIA platforms.
+
+	  May be turned off in builds targeting other vendors. Otherwise, must
+          be enabled for Xen to work suitably on VIA 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 targeting other vendors. Otherwise, must
+          be enabled for Xen to work suitably on Hygon platforms.
+
 config INTEL
 	bool "Support Intel CPUs"
 	default y
@@ -19,4 +38,14 @@ config INTEL
 	  May be turned off in builds targetting other vendors.  Otherwise,
 	  must be enabled for Xen to work suitably on Intel platforms.
 
+config SHANGHAI
+	bool "Support Shanghai CPUs"
+	depends on INTEL
+	default y
+	help
+	  Detection, tunings and quirks for Zhaoxin platforms.
+
+	  May be turned off in builds targeting other vendors. Otherwise, must
+          be enabled for Xen to work suitably on Zhaoxin platforms.
+
 endmenu
diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
index eafce5f204..80739d0256 100644
--- a/xen/arch/x86/cpu/Makefile
+++ b/xen/arch/x86/cpu/Makefile
@@ -3,13 +3,13 @@ obj-y += microcode/
 obj-y += mtrr/
 
 obj-y += amd.o
-obj-y += centaur.o
+obj-$(CONFIG_CENTAUR) += centaur.o
 obj-y += common.o
-obj-y += hygon.o
+obj-$(CONFIG_HYGON) += hygon.o
 obj-y += intel.o
 obj-y += intel_cacheinfo.o
 obj-y += mwait-idle.o
-obj-y += shanghai.o
+obj-$(CONFIG_SHANGHAI) += 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..dcc2753212 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -339,9 +339,15 @@ void __init early_cpu_init(bool verbose)
 	case X86_VENDOR_INTEL:    intel_unlock_cpuid_leaves(c);
 				  actual_cpu = intel_cpu_dev;    break;
 	case X86_VENDOR_AMD:      actual_cpu = amd_cpu_dev;      break;
+#ifdef CONFIG_CENTAUR
 	case X86_VENDOR_CENTAUR:  actual_cpu = centaur_cpu_dev;  break;
+#endif
+#ifdef CONFIG_SHANGHAI
 	case X86_VENDOR_SHANGHAI: actual_cpu = shanghai_cpu_dev; break;
+#endif
+#ifdef CONFIG_HYGON
 	case X86_VENDOR_HYGON:    actual_cpu = hygon_cpu_dev;    break;
+#endif
 	default:
 		actual_cpu = default_cpu;
 		if (!verbose)
-- 
2.25.1

[XEN PATCH v2 2/5] x86/amd: configurable handling of AMD-specific MSRs access

[Sergiy Kibrik]

Do not compile handlers of guest access to AMD-specific MSRs when CONFIG_AMD=n.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/msr.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 289cf10b78..4567de7fc8 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -219,6 +219,7 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
         *val = msrs->tsc_aux;
         break;
 
+#ifdef CONFIG_AMD
     case MSR_K8_SYSCFG:
     case MSR_K8_TOP_MEM1:
     case MSR_K8_TOP_MEM2:
@@ -281,6 +282,7 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
                                    ? 0 : (msr - MSR_AMD64_DR1_ADDRESS_MASK + 1),
                                    ARRAY_SIZE(msrs->dr_mask))];
         break;
+#endif /* CONFIG_AMD */
 
         /*
          * TODO: Implement when we have better topology representation.
@@ -552,6 +554,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
             wrmsr_tsc_aux(val);
         break;
 
+#ifdef CONFIG_AMD
     case MSR_VIRT_SPEC_CTRL:
         if ( !cp->extd.virt_ssbd )
             goto gp_fault;
@@ -598,6 +601,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
         if ( v == curr && (curr->arch.dr7 & DR7_ACTIVE_MASK) )
             wrmsrl(msr, val);
         break;
+#endif /* CONFIG_AMD */
 
     default:
         return X86EMUL_UNHANDLEABLE;
-- 
2.25.1

[XEN PATCH v2 3/5] x86/spec-ctrl: configurable Intlel/AMD-specific calculations

[Sergiy Kibrik]

Put platforms-specific code under #ifdef CONFIG_{AMD,INTEL} so that when
corresponding CPU support is disabled by configuration less dead code will end
up in the build.

This includes re-ordering of calls to ibpb_calculations() & div_calculations(),
but since they don't access common variables or feature bits it should be
safe to do.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/spec_ctrl.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 75a4177a75..ba6c3e80d2 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -1012,6 +1012,7 @@ static bool __init should_use_eager_fpu(void)
     }
 }
 
+#ifdef CONFIG_AMD
 /*
  * https://www.amd.com/content/dam/amd/en/documents/corporate/cr/speculative-return-stack-overflow-whitepaper.pdf
  */
@@ -1110,6 +1111,7 @@ static void __init div_calculations(bool hw_smt_enabled)
             "enabled.  Please assess your configuration and choose an\n"
             "explicit 'smt=<bool>' setting.  See XSA-439.\n");
 }
+#endif /* CONFIG_AMD */
 
 static void __init ibpb_calculations(void)
 {
@@ -1319,6 +1321,7 @@ static __init void l1tf_calculations(void)
                                             : (3UL << (paddr_bits - 2))));
 }
 
+#ifdef CONFIG_INTEL
 /* Calculate whether this CPU is vulnerable to MDS. */
 static __init void mds_calculations(void)
 {
@@ -1730,6 +1733,8 @@ static void __init bhi_calculations(void)
     }
 }
 
+#endif /* CONFIG_INTEL */
+
 void spec_ctrl_init_domain(struct domain *d)
 {
     bool pv = is_pv_domain(d);
@@ -2025,11 +2030,13 @@ void __init init_speculation_mitigations(void)
             default_scf |= SCF_ist_rsb;
     }
 
+#ifdef CONFIG_AMD
     srso_calculations(hw_smt_enabled);
 
-    ibpb_calculations();
-
     div_calculations(hw_smt_enabled);
+#endif
+
+    ibpb_calculations();
 
     /* Check whether Eager FPU should be enabled by default. */
     if ( opt_eager_fpu == -1 )
@@ -2136,9 +2143,10 @@ void __init init_speculation_mitigations(void)
      * - March 2023, for RFDS.  Enumerate RFDS_CLEAR to mean that VERW now
      *   scrubs non-architectural entries from certain register files.
      */
+#ifdef CONFIG_INTEL
     mds_calculations();
     rfds_calculations();
-
+#endif
     /*
      * Parts which enumerate FB_CLEAR are those with now-updated microcode
      * which weren't susceptible to the original MFBDS (and therefore didn't
@@ -2255,7 +2263,6 @@ void __init init_speculation_mitigations(void)
         opt_tsx = 0;
         tsx_init();
     }
-#endif
 
     /*
      * On some SRBDS-affected hardware, it may be safe to relax srb-lock by
@@ -2286,6 +2293,8 @@ void __init init_speculation_mitigations(void)
 
     bhi_calculations();
 
+#endif /* CONFIG_INTEL */
+
     print_details(thunk);
 
     /*
-- 
2.25.1

[XEN PATCH v2 4/5] 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>
CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
CC: Jan Beulich <jbeulich@suse.com>
---
changes in v2:
  - drop set_in_mcu_opt_ctrl() stub
---
 xen/arch/x86/cpu/Makefile            | 4 ++--
 xen/arch/x86/cpu/common.c            | 2 ++
 xen/arch/x86/include/asm/processor.h | 3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
index 80739d0256..eeb9ebe562 100644
--- a/xen/arch/x86/cpu/Makefile
+++ b/xen/arch/x86/cpu/Makefile
@@ -6,8 +6,8 @@ obj-y += amd.o
 obj-$(CONFIG_CENTAUR) += centaur.o
 obj-y += common.o
 obj-$(CONFIG_HYGON) += 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-$(CONFIG_SHANGHAI) += shanghai.o
 obj-y += vpmu.o
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index dcc2753212..580b01d6d5 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -336,8 +336,10 @@ 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;
+#endif
 	case X86_VENDOR_AMD:      actual_cpu = amd_cpu_dev;      break;
 #ifdef CONFIG_CENTAUR
 	case X86_VENDOR_CENTAUR:  actual_cpu = centaur_cpu_dev;  break;
diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h
index 66463f6a6d..a52f8b0a83 100644
--- a/xen/arch/x86/include/asm/processor.h
+++ b/xen/arch/x86/include/asm/processor.h
@@ -507,13 +507,14 @@ 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);
 #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) {}
 #endif
 
-void update_mcu_opt_ctrl(void);
 void set_in_mcu_opt_ctrl(uint32_t mask, uint32_t val);
 
 enum ap_boot_method {
-- 
2.25.1

[XEN PATCH v2 5/5] 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>
CC: Jan Beulich <jbeulich@suse.com>
---
changes in v2:
 - drop routines-stubs in amd.h, handle call sites instead
 - cpu_has_amd_erratum() return int instead of bool
---
 xen/arch/x86/cpu/Makefile      |  2 +-
 xen/arch/x86/cpu/common.c      |  4 +++-
 xen/arch/x86/hvm/svm/svm.c     |  6 ++++--
 xen/arch/x86/include/asm/amd.h | 20 ++++++++++++++++++--
 xen/arch/x86/spec_ctrl.c       |  2 ++
 5 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
index eeb9ebe562..2c34597136 100644
--- a/xen/arch/x86/cpu/Makefile
+++ b/xen/arch/x86/cpu/Makefile
@@ -2,7 +2,7 @@ obj-y += mcheck/
 obj-y += microcode/
 obj-y += mtrr/
 
-obj-y += amd.o
+obj-$(CONFIG_AMD) += amd.o
 obj-$(CONFIG_CENTAUR) += centaur.o
 obj-y += common.o
 obj-$(CONFIG_HYGON) += hygon.o
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 580b01d6d5..5930b712bf 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -194,7 +194,7 @@ void ctxt_switch_levelling(const struct vcpu *next)
 
 		if (cpu_has_cpuid_faulting)
 			set_cpuid_faulting(enable_cpuid_faulting);
-		else
+		else if ( IS_ENABLED(CONFIG_AMD) )
 			amd_set_cpuid_user_dis(enable_cpuid_faulting);
 
 		return;
@@ -340,7 +340,9 @@ void __init early_cpu_init(bool verbose)
 	case X86_VENDOR_INTEL:    intel_unlock_cpuid_leaves(c);
 				  actual_cpu = intel_cpu_dev;    break;
 #endif
+#ifdef CONFIG_AMD
 	case X86_VENDOR_AMD:      actual_cpu = amd_cpu_dev;      break;
+#endif
 #ifdef CONFIG_CENTAUR
 	case X86_VENDOR_CENTAUR:  actual_cpu = centaur_cpu_dev;  break;
 #endif
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 92bb10c504..88902e2d3a 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -919,7 +919,8 @@ static void cf_check svm_ctxt_switch_from(struct vcpu *v)
      * Possibly clear previous guest selection of SSBD if set.  Note that
      * SPEC_CTRL.SSBD is already handled by svm_vmexit_spec_ctrl.
      */
-    if ( v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
+    if ( IS_ENABLED(CONFIG_AMD) &&
+         v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
     {
         ASSERT(v->domain->arch.cpuid->extd.virt_ssbd);
         amd_set_legacy_ssbd(false);
@@ -953,7 +954,8 @@ static void cf_check svm_ctxt_switch_to(struct vcpu *v)
         wrmsr_tsc_aux(v->arch.msrs->tsc_aux);
 
     /* Load SSBD if set by the guest. */
-    if ( v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
+    if ( IS_ENABLED(CONFIG_AMD) &&
+         v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
     {
         ASSERT(v->domain->arch.cpuid->extd.virt_ssbd);
         amd_set_legacy_ssbd(true);
diff --git a/xen/arch/x86/include/asm/amd.h b/xen/arch/x86/include/asm/amd.h
index fa4e0fc766..da35b82d5a 100644
--- a/xen/arch/x86/include/asm/amd.h
+++ b/xen/arch/x86/include/asm/amd.h
@@ -158,20 +158,36 @@
 #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 0;
+}
+#endif
 
 extern s8 opt_allow_unsafe;
 
 void fam10h_check_enable_mmcfg(void);
 void check_enable_amd_mmconf_dmi(void);
 
-extern bool amd_acpi_c1e_quirk;
 void amd_check_disable_c1e(unsigned int port, u8 value);
 
 extern bool amd_legacy_ssbd;
-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);
 
+#ifdef CONFIG_AMD
+extern bool amd_acpi_c1e_quirk;
+extern bool amd_virt_spec_ctrl;
+#else
+
+#define amd_acpi_c1e_quirk (false)
+#define amd_virt_spec_ctrl (false)
+
+#endif
+
 #endif /* __AMD_H__ */
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index ba6c3e80d2..1964a417de 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -1893,10 +1893,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 v2 1/5] x86/Kconfig: introduce CENTAUR, HYGON & SHANGHAI config options

[Alejandro Vallejo]

On Fri Aug 16, 2024 at 12:10 PM BST, Sergiy Kibrik wrote:
> These options aim to represent what's currently supported by Xen, and later
> to allow tuning for specific platform(s) only.
>
> HYGON and SHANGHAI options depend on AMD and INTEL as there're build
> dependencies on support code for AMD and Intel CPUs respectively.
>
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> CC: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/arch/x86/Kconfig.cpu  | 29 +++++++++++++++++++++++++++++
>  xen/arch/x86/cpu/Makefile |  6 +++---
>  xen/arch/x86/cpu/common.c |  6 ++++++
>  3 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/Kconfig.cpu b/xen/arch/x86/Kconfig.cpu
> index 5fb18db1aa..ac8f41d464 100644
> --- a/xen/arch/x86/Kconfig.cpu
> +++ b/xen/arch/x86/Kconfig.cpu
> @@ -10,6 +10,25 @@ config AMD
>  	  May be turned off in builds targetting other vendors.  Otherwise,
>  	  must be enabled for Xen to work suitably on AMD platforms.
>  
> +config CENTAUR
> +	bool "Support Centaur CPUs"
> +	default y
> +	help
> +	  Detection, tunings and quirks for VIA platforms.
> +
> +	  May be turned off in builds targeting other vendors. Otherwise, must
> +          be enabled for Xen to work suitably on VIA 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 targeting other vendors. Otherwise, must
> +          be enabled for Xen to work suitably on Hygon platforms.
> +
>  config INTEL
>  	bool "Support Intel CPUs"
>  	default y
> @@ -19,4 +38,14 @@ config INTEL
>  	  May be turned off in builds targetting other vendors.  Otherwise,
>  	  must be enabled for Xen to work suitably on Intel platforms.
>  
> +config SHANGHAI
> +	bool "Support Shanghai CPUs"
> +	depends on INTEL
> +	default y
> +	help
> +	  Detection, tunings and quirks for Zhaoxin platforms.
> +
> +	  May be turned off in builds targeting other vendors. Otherwise, must
> +          be enabled for Xen to work suitably on Zhaoxin platforms.
> +
>  endmenu
> diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
> index eafce5f204..80739d0256 100644
> --- a/xen/arch/x86/cpu/Makefile
> +++ b/xen/arch/x86/cpu/Makefile
> @@ -3,13 +3,13 @@ obj-y += microcode/
>  obj-y += mtrr/
>  
>  obj-y += amd.o
> -obj-y += centaur.o
> +obj-$(CONFIG_CENTAUR) += centaur.o
>  obj-y += common.o
> -obj-y += hygon.o
> +obj-$(CONFIG_HYGON) += hygon.o
>  obj-y += intel.o
>  obj-y += intel_cacheinfo.o
>  obj-y += mwait-idle.o
> -obj-y += shanghai.o
> +obj-$(CONFIG_SHANGHAI) += 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..dcc2753212 100644
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -339,9 +339,15 @@ void __init early_cpu_init(bool verbose)
>  	case X86_VENDOR_INTEL:    intel_unlock_cpuid_leaves(c);
>  				  actual_cpu = intel_cpu_dev;    break;
>  	case X86_VENDOR_AMD:      actual_cpu = amd_cpu_dev;      break;
> +#ifdef CONFIG_CENTAUR
>  	case X86_VENDOR_CENTAUR:  actual_cpu = centaur_cpu_dev;  break;
> +#endif
> +#ifdef CONFIG_SHANGHAI
>  	case X86_VENDOR_SHANGHAI: actual_cpu = shanghai_cpu_dev; break;
> +#endif
> +#ifdef CONFIG_HYGON
>  	case X86_VENDOR_HYGON:    actual_cpu = hygon_cpu_dev;    break;
> +#endif
>  	default:
>  		actual_cpu = default_cpu;
>  		if (!verbose)

Reviewed-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

Cheers,
Alejandro

Re: [XEN PATCH v2 1/5] x86/Kconfig: introduce CENTAUR, HYGON & SHANGHAI config options

[Jan Beulich]

On 16.08.2024 13:10, Sergiy Kibrik wrote:
> --- a/xen/arch/x86/Kconfig.cpu
> +++ b/xen/arch/x86/Kconfig.cpu
> @@ -10,6 +10,25 @@ config AMD
>  	  May be turned off in builds targetting other vendors.  Otherwise,
>  	  must be enabled for Xen to work suitably on AMD platforms.
>  
> +config CENTAUR
> +	bool "Support Centaur CPUs"
> +	default y
> +	help
> +	  Detection, tunings and quirks for VIA platforms.
> +
> +	  May be turned off in builds targeting other vendors. Otherwise, must
> +          be enabled for Xen to work suitably on VIA 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 targeting other vendors. Otherwise, must
> +          be enabled for Xen to work suitably on Hygon platforms.
> +
>  config INTEL
>  	bool "Support Intel CPUs"
>  	default y
> @@ -19,4 +38,14 @@ config INTEL
>  	  May be turned off in builds targetting other vendors.  Otherwise,
>  	  must be enabled for Xen to work suitably on Intel platforms.
>  
> +config SHANGHAI
> +	bool "Support Shanghai CPUs"
> +	depends on INTEL
> +	default y
> +	help
> +	  Detection, tunings and quirks for Zhaoxin platforms.
> +
> +	  May be turned off in builds targeting other vendors. Otherwise, must
> +          be enabled for Xen to work suitably on Zhaoxin platforms.
> +
>  endmenu

Imo this re-raises the question of whether it is a good idea to leave out
"CPU" from the names: The more names there are, the more likely it'll become
that going forward we'll run into a naming collision. Andrew, iirc you
were the main proponent for omitting "CPU" - may I ask that you re-consider?

Furthermore I wonder whether "depends on" is appropriate here. This way one
won't be offered e.g. SHANGHAI if earlier on one de-selected INTEL.

Plus it is mere luck that the alphabetic ordering ends up with the dependents
after their dependencies (things would be somewhat odd the other way around).

Finally please check indentation of help text - there is one inconsistency
repeated for all three entries being added.

Jan

Re: [XEN PATCH v2 2/5] x86/amd: configurable handling of AMD-specific MSRs access

[Jan Beulich]

On 16.08.2024 13:12, Sergiy Kibrik wrote:
> Do not compile handlers of guest access to AMD-specific MSRs when CONFIG_AMD=n.

What I'm missing in the description is clarification on how boundaries were
drawn. In guest_rdmsr() there is, for example, also handling of MSR_AMD_PATCHLEVEL.
Which I'm okay to leave aside for now, as long as it's clear why that is.

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -219,6 +219,7 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>          *val = msrs->tsc_aux;
>          break;
>  
> +#ifdef CONFIG_AMD
>      case MSR_K8_SYSCFG:
>      case MSR_K8_TOP_MEM1:
>      case MSR_K8_TOP_MEM2:
> @@ -281,6 +282,7 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>                                     ? 0 : (msr - MSR_AMD64_DR1_ADDRESS_MASK + 1),
>                                     ARRAY_SIZE(msrs->dr_mask))];
>          break;
> +#endif /* CONFIG_AMD */
>  
>          /*
>           * TODO: Implement when we have better topology representation.
> @@ -552,6 +554,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>              wrmsr_tsc_aux(val);
>          break;
>  
> +#ifdef CONFIG_AMD
>      case MSR_VIRT_SPEC_CTRL:
>          if ( !cp->extd.virt_ssbd )
>              goto gp_fault;
> @@ -598,6 +601,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>          if ( v == curr && (curr->arch.dr7 & DR7_ACTIVE_MASK) )
>              wrmsrl(msr, val);
>          break;
> +#endif /* CONFIG_AMD */
>  
>      default:
>          return X86EMUL_UNHANDLEABLE;

Is just adding #ifdef-s actually correct? That results in different behavior on
e.g. Intel hardware, I think, depending on whether AMD=y or AMD=n. In the latter
case the function will now return X86EMUL_UNHANDLEABLE, while in the former case
it would return X86EMUL_EXCEPTION.

Jan

Re: [XEN PATCH v2 3/5] x86/spec-ctrl: configurable Intlel/AMD-specific calculations

[Jan Beulich]

On 16.08.2024 13:14, Sergiy Kibrik wrote:
> Put platforms-specific code under #ifdef CONFIG_{AMD,INTEL} so that when
> corresponding CPU support is disabled by configuration less dead code will end
> up in the build.
> 
> This includes re-ordering of calls to ibpb_calculations() & div_calculations(),
> but since they don't access common variables or feature bits it should be
> safe to do.
> 
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> CC: Jan Beulich <jbeulich@suse.com>

For one please consider adding Requested-by: or Suggested-by: tags.

> --- a/xen/arch/x86/spec_ctrl.c
> +++ b/xen/arch/x86/spec_ctrl.c
> @@ -1012,6 +1012,7 @@ static bool __init should_use_eager_fpu(void)
>      }
>  }
>  
> +#ifdef CONFIG_AMD
>  /*
>   * https://www.amd.com/content/dam/amd/en/documents/corporate/cr/speculative-return-stack-overflow-whitepaper.pdf
>   */
> @@ -1110,6 +1111,7 @@ static void __init div_calculations(bool hw_smt_enabled)
>              "enabled.  Please assess your configuration and choose an\n"
>              "explicit 'smt=<bool>' setting.  See XSA-439.\n");
>  }
> +#endif /* CONFIG_AMD */

And then no, I don't think we want to use #ifdef-ary here. IS_ENABLED()
inside the functions (where the vendor checks are) is not only making
sure the compiler will still parse all the code even when either vendor's
support was turned off, but will also help review (by having in context
what the actual vendor checks are in each function).

Jan

Re: [XEN PATCH v2 4/5] x86/intel: optional build of intel.c

[Jan Beulich]

On 16.08.2024 13:17, 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>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [XEN PATCH v2 5/5] x86/amd: optional build of amd.c

[Jan Beulich]

On 16.08.2024 13:19, Sergiy Kibrik wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -919,7 +919,8 @@ static void cf_check svm_ctxt_switch_from(struct vcpu *v)
>       * Possibly clear previous guest selection of SSBD if set.  Note that
>       * SPEC_CTRL.SSBD is already handled by svm_vmexit_spec_ctrl.
>       */
> -    if ( v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
> +    if ( IS_ENABLED(CONFIG_AMD) &&
> +         v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
>      {
>          ASSERT(v->domain->arch.cpuid->extd.virt_ssbd);
>          amd_set_legacy_ssbd(false);
> @@ -953,7 +954,8 @@ static void cf_check svm_ctxt_switch_to(struct vcpu *v)
>          wrmsr_tsc_aux(v->arch.msrs->tsc_aux);
>  
>      /* Load SSBD if set by the guest. */
> -    if ( v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
> +    if ( IS_ENABLED(CONFIG_AMD) &&
> +         v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
>      {
>          ASSERT(v->domain->arch.cpuid->extd.virt_ssbd);
>          amd_set_legacy_ssbd(true);

Instead of these changes, shouldn't AMD_SVM become dependent upon AMD in
Kconfig?

> +#ifdef CONFIG_AMD
> +extern bool amd_acpi_c1e_quirk;
> +extern bool amd_virt_spec_ctrl;
> +#else
> +
> +#define amd_acpi_c1e_quirk (false)
> +#define amd_virt_spec_ctrl (false)

As a remark, while there's nothing wrong with parenthesizing "false" here,
it also isn't really necessary. Omitting unnecessary parentheses generally
aids readability imo.

Jan

Re: [XEN PATCH v2 1/5] x86/Kconfig: introduce CENTAUR, HYGON & SHANGHAI config options

[Sergiy Kibrik]

19.08.24 11:53, Jan Beulich:
> On 16.08.2024 13:10, Sergiy Kibrik wrote:
>> --- a/xen/arch/x86/Kconfig.cpu
>> +++ b/xen/arch/x86/Kconfig.cpu
>> @@ -10,6 +10,25 @@ config AMD
>>   	  May be turned off in builds targetting other vendors.  Otherwise,
>>   	  must be enabled for Xen to work suitably on AMD platforms.
>>   
>> +config CENTAUR
>> +	bool "Support Centaur CPUs"
>> +	default y
>> +	help
>> +	  Detection, tunings and quirks for VIA platforms.
>> +
>> +	  May be turned off in builds targeting other vendors. Otherwise, must
>> +          be enabled for Xen to work suitably on VIA 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 targeting other vendors. Otherwise, must
>> +          be enabled for Xen to work suitably on Hygon platforms.
>> +
>>   config INTEL
>>   	bool "Support Intel CPUs"
>>   	default y
>> @@ -19,4 +38,14 @@ config INTEL
>>   	  May be turned off in builds targetting other vendors.  Otherwise,
>>   	  must be enabled for Xen to work suitably on Intel platforms.
>>   
>> +config SHANGHAI
>> +	bool "Support Shanghai CPUs"
>> +	depends on INTEL
>> +	default y
>> +	help
>> +	  Detection, tunings and quirks for Zhaoxin platforms.
>> +
>> +	  May be turned off in builds targeting other vendors. Otherwise, must
>> +          be enabled for Xen to work suitably on Zhaoxin platforms.
>> +
>>   endmenu
> Imo this re-raises the question of whether it is a good idea to leave out
> "CPU" from the names: The more names there are, the more likely it'll become
> that going forward we'll run into a naming collision. Andrew, iirc you
> were the main proponent for omitting "CPU" - may I ask that you re-consider?
> 
> Furthermore I wonder whether "depends on" is appropriate here. This way one
> won't be offered e.g. SHANGHAI if earlier on one de-selected INTEL.

I see, then probably I'll use 'select INTEL' here.
And also 'select AMD' for HYGON.

   -Sergiy

Re: [XEN PATCH v2 5/5] x86/amd: optional build of amd.c

[Sergiy Kibrik]

19.08.24 15:36, Jan Beulich:
> On 16.08.2024 13:19, Sergiy Kibrik wrote:
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -919,7 +919,8 @@ static void cf_check svm_ctxt_switch_from(struct vcpu *v)
>>        * Possibly clear previous guest selection of SSBD if set.  Note that
>>        * SPEC_CTRL.SSBD is already handled by svm_vmexit_spec_ctrl.
>>        */
>> -    if ( v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
>> +    if ( IS_ENABLED(CONFIG_AMD) &&
>> +         v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
>>       {
>>           ASSERT(v->domain->arch.cpuid->extd.virt_ssbd);
>>           amd_set_legacy_ssbd(false);
>> @@ -953,7 +954,8 @@ static void cf_check svm_ctxt_switch_to(struct vcpu *v)
>>           wrmsr_tsc_aux(v->arch.msrs->tsc_aux);
>>   
>>       /* Load SSBD if set by the guest. */
>> -    if ( v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
>> +    if ( IS_ENABLED(CONFIG_AMD) &&
>> +         v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
>>       {
>>           ASSERT(v->domain->arch.cpuid->extd.virt_ssbd);
>>           amd_set_legacy_ssbd(true);
> Instead of these changes, shouldn't AMD_SVM become dependent upon AMD in
> Kconfig?

It could be done earlier, yet I haven't done so since we briefly touched 
this before and decided not to link {AMD,INTEL} with {AMD_SVM,INTEL_VMX} 
then:

https://lore.kernel.org/xen-devel/9a973f18-e0af-456c-8b07-6869f044519e@citrix.com/

   -Sergiy

Re: [XEN PATCH v2 5/5] x86/amd: optional build of amd.c

[Jan Beulich]

On 29.08.2024 11:48, Sergiy Kibrik wrote:
> 19.08.24 15:36, Jan Beulich:
>> On 16.08.2024 13:19, Sergiy Kibrik wrote:
>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>> @@ -919,7 +919,8 @@ static void cf_check svm_ctxt_switch_from(struct vcpu *v)
>>>        * Possibly clear previous guest selection of SSBD if set.  Note that
>>>        * SPEC_CTRL.SSBD is already handled by svm_vmexit_spec_ctrl.
>>>        */
>>> -    if ( v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
>>> +    if ( IS_ENABLED(CONFIG_AMD) &&
>>> +         v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
>>>       {
>>>           ASSERT(v->domain->arch.cpuid->extd.virt_ssbd);
>>>           amd_set_legacy_ssbd(false);
>>> @@ -953,7 +954,8 @@ static void cf_check svm_ctxt_switch_to(struct vcpu *v)
>>>           wrmsr_tsc_aux(v->arch.msrs->tsc_aux);
>>>   
>>>       /* Load SSBD if set by the guest. */
>>> -    if ( v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
>>> +    if ( IS_ENABLED(CONFIG_AMD) &&
>>> +         v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
>>>       {
>>>           ASSERT(v->domain->arch.cpuid->extd.virt_ssbd);
>>>           amd_set_legacy_ssbd(true);
>> Instead of these changes, shouldn't AMD_SVM become dependent upon AMD in
>> Kconfig?
> 
> It could be done earlier, yet I haven't done so since we briefly touched 
> this before and decided not to link {AMD,INTEL} with {AMD_SVM,INTEL_VMX} 
> then:
> 
> https://lore.kernel.org/xen-devel/9a973f18-e0af-456c-8b07-6869f044519e@citrix.com/

Yet that only suggests that e.g HYGON also ought to select AMD_SVM. Which
will happen transitively with HYGON selecting AMD (in the earlier patch).

Jan

Re: [XEN PATCH v2 3/5] x86/spec-ctrl: configurable Intlel/AMD-specific calculations

[Andrew Cooper]

On 16/08/2024 12:14 pm, Sergiy Kibrik wrote:
> Put platforms-specific code under #ifdef CONFIG_{AMD,INTEL} so that when
> corresponding CPU support is disabled by configuration less dead code will end
> up in the build.
>
> This includes re-ordering of calls to ibpb_calculations() & div_calculations(),
> but since they don't access common variables or feature bits it should be
> safe to do.
>
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> CC: Jan Beulich <jbeulich@suse.com>

Sorry, but no.

This logic is security critical, highly fragile, gets chopped/changed
multiple times a year (as researchers keep on finding new things), and
all major work is done to it under embargo.

Just look at the history of the file.

The ifdefary around the tsx_init() call is bad enough and I intend to
revert it and do that differently.  I would have objected if I'd got to
the patch in time.


The only relevant cost in this file is whether I (and the other security
team members) can edit it correctly or not in staging and all prior
in-support branches.  You really don't want to know how many times
there's been a bug in backports...

Saving 451 lines from certification is not cheaper than the
problems/risks you're introducing with this change.

~Andrew

Re: [XEN PATCH v2 3/5] x86/spec-ctrl: configurable Intlel/AMD-specific calculations

[Jan Beulich]

On 29.08.2024 21:25, Andrew Cooper wrote:
> On 16/08/2024 12:14 pm, Sergiy Kibrik wrote:
>> Put platforms-specific code under #ifdef CONFIG_{AMD,INTEL} so that when
>> corresponding CPU support is disabled by configuration less dead code will end
>> up in the build.
>>
>> This includes re-ordering of calls to ibpb_calculations() & div_calculations(),
>> but since they don't access common variables or feature bits it should be
>> safe to do.
>>
>> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
>> CC: Jan Beulich <jbeulich@suse.com>
> 
> Sorry, but no.
> 
> This logic is security critical, highly fragile, gets chopped/changed
> multiple times a year (as researchers keep on finding new things), and
> all major work is done to it under embargo.
> 
> Just look at the history of the file.
> 
> The ifdefary around the tsx_init() call is bad enough and I intend to
> revert it and do that differently.  I would have objected if I'd got to
> the patch in time.
> 
> 
> The only relevant cost in this file is whether I (and the other security
> team members) can edit it correctly or not in staging and all prior
> in-support branches.  You really don't want to know how many times
> there's been a bug in backports...
> 
> Saving 451 lines from certification is not cheaper than the
> problems/risks you're introducing with this change.

Did you see my earlier reply? I don't think the issue is with hiding source
lines. We want to have the compiler DCE stuff wherever possible, hence why
I did respond asking to switch to IS_ENABLED(). That imo fits pretty well
with the vendor checks we have there already anyway.

Jan