[XEN PATCH v2 0/5] x86: make Intel/AMD vPMU & MCE support configurable

[XEN PATCH v2 0/5] x86: make Intel/AMD vPMU & MCE support configurable

[Sergiy Kibrik]

Here's a second attempt to separate support of Intel & AMD CPUs in Xen build.
The code to drive both platforms used to be built unconditionally, until recent
introduction of CONFIG_{AMD,INTEL} Kconfig options.

This series extends coverage of these options on vpmu and mcheck subsystems,
which allows not to build Intel or AMD vpmu/mcheck support if CPU vendor's support
was explicitly disabled.

Since v1 series one patch had been merged independently, and one more got to be
squashed, so series became shorter. Specific changes since v1 are provided per-patch.

v1 series here:
https://lore.kernel.org/xen-devel/cover.1713860310.git.Sergiy_Kibrik@epam.com/

  -Sergiy

Sergiy Kibrik (5):
  x86/vpmu: separate amd/intel vPMU code
  x86/intel: move vmce_has_lmce() routine to header
  x86/MCE: guard access to Intel/AMD-specific MCA MSRs
  x86/MCE: guard {intel/amd}_mcheck_init() calls
  x86/MCE: optional build of AMD/Intel MCE code

 xen/arch/x86/cpu/Makefile           |  4 +++-
 xen/arch/x86/cpu/mcheck/Makefile    |  8 ++++----
 xen/arch/x86/cpu/mcheck/mce.c       |  6 ++++--
 xen/arch/x86/cpu/mcheck/mce.h       | 13 +++++++++++++
 xen/arch/x86/cpu/mcheck/mce_intel.c |  4 ----
 xen/arch/x86/cpu/mcheck/non-fatal.c |  6 ++++++
 xen/arch/x86/cpu/mcheck/vmce.c      | 17 ++++++++++-------
 xen/arch/x86/cpu/vpmu.c             |  6 ++++--
 xen/arch/x86/include/asm/mce.h      |  1 -
 xen/arch/x86/msr.c                  |  2 ++
 10 files changed, 46 insertions(+), 21 deletions(-)

-- 
2.25.1

[XEN PATCH v2 1/5] x86/vpmu: separate amd/intel vPMU code

[Sergiy Kibrik]

Build AMD vPMU when CONFIG_AMD is on, and Intel vPMU when CONFIG_INTEL
is on respectively, allowing for a plaftorm-specific build.

No functional change intended.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>

---
changes in v2:
 - drop static inline stubs, use #idef/#endif in vpmu_init)()
changes in v1:
 - switch to CONFIG_{AMD,INTEL} instead of CONFIG_{SVM,VMX}
---
 xen/arch/x86/cpu/Makefile | 4 +++-
 xen/arch/x86/cpu/vpmu.c   | 6 ++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
index 35561fe51d..eafce5f204 100644
--- a/xen/arch/x86/cpu/Makefile
+++ b/xen/arch/x86/cpu/Makefile
@@ -10,4 +10,6 @@ obj-y += intel.o
 obj-y += intel_cacheinfo.o
 obj-y += mwait-idle.o
 obj-y += shanghai.o
-obj-y += vpmu.o vpmu_amd.o vpmu_intel.o
+obj-y += vpmu.o
+obj-$(CONFIG_AMD) += vpmu_amd.o
+obj-$(CONFIG_INTEL) += vpmu_intel.o
diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
index b2e9881e06..3db90b7839 100644
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -827,6 +827,7 @@ static int __init cf_check vpmu_init(void)
 
     switch ( vendor )
     {
+#ifdef CONFIG_AMD
     case X86_VENDOR_AMD:
         ops = amd_vpmu_init();
         break;
@@ -834,11 +835,12 @@ static int __init cf_check vpmu_init(void)
     case X86_VENDOR_HYGON:
         ops = hygon_vpmu_init();
         break;
-
+#endif
+#ifdef CONFIG_INTEL
     case X86_VENDOR_INTEL:
         ops = core2_vpmu_init();
         break;
-
+#endif
     default:
         printk(XENLOG_WARNING "VPMU: Unknown CPU vendor: %d. "
                "Turning VPMU off.\n", vendor);
-- 
2.25.1

Re: [XEN PATCH v2 1/5] x86/vpmu: separate amd/intel vPMU code

[Jan Beulich]

On 02.05.2024 11:12, Sergiy Kibrik wrote:
> Build AMD vPMU when CONFIG_AMD is on, and Intel vPMU when CONFIG_INTEL
> is on respectively, allowing for a plaftorm-specific build.
> 
> No functional change intended.
> 
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

I can only guess that Stefano is likely fine with

> changes in v2:
>  - drop static inline stubs, use #idef/#endif in vpmu_init)()

this.

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

> --- a/xen/arch/x86/cpu/vpmu.c
> +++ b/xen/arch/x86/cpu/vpmu.c
> @@ -827,6 +827,7 @@ static int __init cf_check vpmu_init(void)
>  
>      switch ( vendor )
>      {
> +#ifdef CONFIG_AMD
>      case X86_VENDOR_AMD:
>          ops = amd_vpmu_init();
>          break;
> @@ -834,11 +835,12 @@ static int __init cf_check vpmu_init(void)
>      case X86_VENDOR_HYGON:
>          ops = hygon_vpmu_init();
>          break;
> -
> +#endif
> +#ifdef CONFIG_INTEL
>      case X86_VENDOR_INTEL:
>          ops = core2_vpmu_init();
>          break;
> -
> +#endif
>      default:
>          printk(XENLOG_WARNING "VPMU: Unknown CPU vendor: %d. "
>                 "Turning VPMU off.\n", vendor);

... neither of the blank lines dropped.

Jan

[XEN PATCH v2 2/5] x86/intel: move vmce_has_lmce() routine to header

[Sergiy Kibrik]

Moving this function out of mce_intel.c would make it possible to disable
build of Intel MCE code later on, because the function gets called from
common x86 code.

Add internal check for CONFIG_INTEL option, as MCG_LMCE_P bit is currently
specific to Intel CPUs only.

Also replace boilerplate code that checks for MCG_LMCE_P flag with
vmce_has_lmce(), which might contribute to readability a bit.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
CC: Jan Beulich <jbeulich@suse.com>
---
changes in v2:
 - move vmce_has_lmce() to cpu/mcheck/mce.h
 - move IS_ENABLED(CONFIG_INTEL) check inside vmce_has_lmce()
 - changed description
---
 xen/arch/x86/cpu/mcheck/mce.h       | 5 +++++
 xen/arch/x86/cpu/mcheck/mce_intel.c | 4 ----
 xen/arch/x86/cpu/mcheck/vmce.c      | 5 ++---
 xen/arch/x86/include/asm/mce.h      | 1 -
 xen/arch/x86/msr.c                  | 2 ++
 5 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.h b/xen/arch/x86/cpu/mcheck/mce.h
index 4806405f96..d6d56aa232 100644
--- a/xen/arch/x86/cpu/mcheck/mce.h
+++ b/xen/arch/x86/cpu/mcheck/mce.h
@@ -170,6 +170,11 @@ static inline int mce_bank_msr(const struct vcpu *v, uint32_t msr)
     return 0;
 }
 
+static inline bool vmce_has_lmce(const struct vcpu *v)
+{
+    return IS_ENABLED(CONFIG_INTEL) && v->arch.vmce.mcg_cap & MCG_LMCE_P;
+}
+
 struct mce_callbacks {
     void (*handler)(const struct cpu_user_regs *regs);
     bool (*check_addr)(uint64_t status, uint64_t misc, int addr_type);
diff --git a/xen/arch/x86/cpu/mcheck/mce_intel.c b/xen/arch/x86/cpu/mcheck/mce_intel.c
index 3f5199b531..af43281cc6 100644
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -1050,7 +1050,3 @@ int vmce_intel_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
     return 1;
 }
 
-bool vmce_has_lmce(const struct vcpu *v)
-{
-    return v->arch.vmce.mcg_cap & MCG_LMCE_P;
-}
diff --git a/xen/arch/x86/cpu/mcheck/vmce.c b/xen/arch/x86/cpu/mcheck/vmce.c
index 353d4f19b2..94d1f021e1 100644
--- a/xen/arch/x86/cpu/mcheck/vmce.c
+++ b/xen/arch/x86/cpu/mcheck/vmce.c
@@ -199,7 +199,7 @@ int vmce_rdmsr(uint32_t msr, uint64_t *val)
          * bits are always set in guest MSR_IA32_FEATURE_CONTROL by Xen, so it
          * does not need to check them here.
          */
-        if ( cur->arch.vmce.mcg_cap & MCG_LMCE_P )
+        if ( vmce_has_lmce(cur) )
         {
             *val = cur->arch.vmce.mcg_ext_ctl;
             mce_printk(MCE_VERBOSE, "MCE: %pv: rd MCG_EXT_CTL %#"PRIx64"\n",
@@ -324,8 +324,7 @@ int vmce_wrmsr(uint32_t msr, uint64_t val)
         break;
 
     case MSR_IA32_MCG_EXT_CTL:
-        if ( (cur->arch.vmce.mcg_cap & MCG_LMCE_P) &&
-             !(val & ~MCG_EXT_CTL_LMCE_EN) )
+        if ( vmce_has_lmce(cur) && !(val & ~MCG_EXT_CTL_LMCE_EN) )
             cur->arch.vmce.mcg_ext_ctl = val;
         else
             ret = -1;
diff --git a/xen/arch/x86/include/asm/mce.h b/xen/arch/x86/include/asm/mce.h
index 6ce56b5b85..2ec47a71ae 100644
--- a/xen/arch/x86/include/asm/mce.h
+++ b/xen/arch/x86/include/asm/mce.h
@@ -41,7 +41,6 @@ extern void vmce_init_vcpu(struct vcpu *v);
 extern int vmce_restore_vcpu(struct vcpu *v, const struct hvm_vmce_vcpu *ctxt);
 extern int vmce_wrmsr(uint32_t msr, uint64_t val);
 extern int vmce_rdmsr(uint32_t msr, uint64_t *val);
-extern bool vmce_has_lmce(const struct vcpu *v);
 extern int vmce_enable_mca_cap(struct domain *d, uint64_t cap);
 
 DECLARE_PER_CPU(unsigned int, nr_mce_banks);
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 9babd441f9..b0ec96f021 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -24,6 +24,8 @@
 
 #include <public/hvm/params.h>
 
+#include "cpu/mcheck/mce.h"
+
 DEFINE_PER_CPU(uint32_t, tsc_aux);
 
 int init_vcpu_msr_policy(struct vcpu *v)
-- 
2.25.1

Re: [XEN PATCH v2 2/5] x86/intel: move vmce_has_lmce() routine to header

[Jan Beulich]

On 02.05.2024 11:14, Sergiy Kibrik wrote:
> Moving this function out of mce_intel.c would make it possible to disable
> build of Intel MCE code later on, because the function gets called from
> common x86 code.
> 
> Add internal check for CONFIG_INTEL option, as MCG_LMCE_P bit is currently
> specific to Intel CPUs only.

My previously voiced concern regarding this was not addressed. If ...

> --- a/xen/arch/x86/cpu/mcheck/mce.h
> +++ b/xen/arch/x86/cpu/mcheck/mce.h
> @@ -170,6 +170,11 @@ static inline int mce_bank_msr(const struct vcpu *v, uint32_t msr)
>      return 0;
>  }
>  
> +static inline bool vmce_has_lmce(const struct vcpu *v)
> +{
> +    return IS_ENABLED(CONFIG_INTEL) && v->arch.vmce.mcg_cap & MCG_LMCE_P;

... the IS_ENABLED() indeed is to stay, the & wants parenthesizing.

Jan

Re: [XEN PATCH v2 2/5] x86/intel: move vmce_has_lmce() routine to header

[Sergiy Kibrik]

06.05.24 14:18, Jan Beulich:
> On 02.05.2024 11:14, Sergiy Kibrik wrote:
>> Moving this function out of mce_intel.c would make it possible to disable
>> build of Intel MCE code later on, because the function gets called from
>> common x86 code.
>>
>> Add internal check for CONFIG_INTEL option, as MCG_LMCE_P bit is currently
>> specific to Intel CPUs only.
> My previously voiced concern regarding this was not addressed. If ...

I misunderstood you comment to v1 patch.
I'll drop checks for CONFIG_INTEL, I see now that we don't really need one.

   -Sergiy

[XEN PATCH v2 3/5] x86/MCE: guard access to Intel/AMD-specific MCA MSRs

[Sergiy Kibrik]

Add build-time checks for newly introduced INTEL/AMD config options when
calling vmce_{intel/amd}_{rdmsr/wrmsr}() routines.
This way a platform-specific code can be omitted in vmce code, if this
platform is disabled in config.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
CC: Jan Beulich <jbeulich@suse.com>
---
changes in v2:
 - use #ifdef/#endif in switch instead of IS_ENABLED
 - fallback to returning default 0 if vendor not recognized
---
 xen/arch/x86/cpu/mcheck/vmce.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/vmce.c b/xen/arch/x86/cpu/mcheck/vmce.c
index 94d1f021e1..373a8e2452 100644
--- a/xen/arch/x86/cpu/mcheck/vmce.c
+++ b/xen/arch/x86/cpu/mcheck/vmce.c
@@ -138,17 +138,19 @@ static int bank_mce_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
     default:
         switch ( boot_cpu_data.x86_vendor )
         {
+#ifdef CONFIG_INTEL
         case X86_VENDOR_CENTAUR:
         case X86_VENDOR_SHANGHAI:
         case X86_VENDOR_INTEL:
             ret = vmce_intel_rdmsr(v, msr, val);
             break;
-
+#endif
+#ifdef CONFIG_AMD
         case X86_VENDOR_AMD:
         case X86_VENDOR_HYGON:
             ret = vmce_amd_rdmsr(v, msr, val);
             break;
-
+#endif
         default:
             ret = 0;
             break;
@@ -271,15 +273,17 @@ static int bank_mce_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
     default:
         switch ( boot_cpu_data.x86_vendor )
         {
+#ifdef CONFIG_INTEL
         case X86_VENDOR_INTEL:
             ret = vmce_intel_wrmsr(v, msr, val);
             break;
-
+#endif
+#ifdef CONFIG_AMD
         case X86_VENDOR_AMD:
         case X86_VENDOR_HYGON:
             ret = vmce_amd_wrmsr(v, msr, val);
             break;
-
+#endif
         default:
             ret = 0;
             break;
-- 
2.25.1

Re: [XEN PATCH v2 3/5] x86/MCE: guard access to Intel/AMD-specific MCA MSRs

[Jan Beulich]

On 02.05.2024 11:16, Sergiy Kibrik wrote:
> Add build-time checks for newly introduced INTEL/AMD config options when
> calling vmce_{intel/amd}_{rdmsr/wrmsr}() routines.
> This way a platform-specific code can be omitted in vmce code, if this
> platform is disabled in config.
> 
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

With the same remark and the same constraint as on patch 1:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

[XEN PATCH v2 4/5] x86/MCE: guard {intel/amd}_mcheck_init() calls

[Sergiy Kibrik]

Guard calls to CPU-specific mcheck init routines in common MCE code
using new INTEL/AMD config options.

The purpose is not to build platform-specific mcheck code and calls to it,
if this platform is disabled in config.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
CC: Jan Beulich <jbeulich@suse.com>
---
changes in v2:
 - use #ifdef/#endif in switch instead of IS_ENABLED
---
 xen/arch/x86/cpu/mcheck/mce.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index d179e6b068..fb943addae 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -760,11 +760,13 @@ void mcheck_init(struct cpuinfo_x86 *c, bool bsp)
 
     switch ( c->x86_vendor )
     {
+#ifdef CONFIG_AMD
     case X86_VENDOR_AMD:
     case X86_VENDOR_HYGON:
         inited = amd_mcheck_init(c, bsp);
         break;
-
+#endif
+#ifdef CONFIG_INTEL
     case X86_VENDOR_INTEL:
         switch ( c->x86 )
         {
@@ -774,7 +776,7 @@ void mcheck_init(struct cpuinfo_x86 *c, bool bsp)
             break;
         }
         break;
-
+#endif
     default:
         break;
     }
-- 
2.25.1

Re: [XEN PATCH v2 4/5] x86/MCE: guard {intel/amd}_mcheck_init() calls

[Jan Beulich]

On 02.05.2024 11:18, Sergiy Kibrik wrote:
> Guard calls to CPU-specific mcheck init routines in common MCE code
> using new INTEL/AMD config options.
> 
> The purpose is not to build platform-specific mcheck code and calls to it,
> if this platform is disabled in config.
> 
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Once again with the same remark and the same constraint as on patches 1 and 3:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

[XEN PATCH v2 5/5] x86/MCE: optional build of AMD/Intel MCE code

[Sergiy Kibrik]

Separate Intel/AMD-specific MCE code using CONFIG_{INTEL,AMD} config options.
Now we can avoid build of mcheck code if support for specific platform is
intentionally disabled by configuration.

Add default return value to init_nonfatal_mce_checker() routine -- in case
of a build with both AMD and INTEL options are off (e.g. randconfig).

Also global Intel-specific variables lmce_support & cmci_support have to be
redefined if !INTEL, as they get checked in common code.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
CC: Jan Beulich <jbeulich@suse.com>
---
changes in v2:
 - fallback to original ordering in Makefile
 - redefine lmce_support & cmci_support global vars to false when !INTEL
 - changed patch description
---
 xen/arch/x86/cpu/mcheck/Makefile    | 8 ++++----
 xen/arch/x86/cpu/mcheck/mce.h       | 8 ++++++++
 xen/arch/x86/cpu/mcheck/non-fatal.c | 6 ++++++
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/Makefile b/xen/arch/x86/cpu/mcheck/Makefile
index f927f10b4d..e6cb4dd503 100644
--- a/xen/arch/x86/cpu/mcheck/Makefile
+++ b/xen/arch/x86/cpu/mcheck/Makefile
@@ -1,12 +1,12 @@
-obj-y += amd_nonfatal.o
-obj-y += mce_amd.o
+obj-$(CONFIG_AMD) += amd_nonfatal.o
+obj-$(CONFIG_AMD) += mce_amd.o
 obj-y += mcaction.o
 obj-y += barrier.o
-obj-y += intel-nonfatal.o
+obj-$(CONFIG_INTEL) += intel-nonfatal.o
 obj-y += mctelem.o
 obj-y += mce.o
 obj-y += mce-apei.o
-obj-y += mce_intel.o
+obj-$(CONFIG_INTEL) += mce_intel.o
 obj-y += non-fatal.o
 obj-y += util.o
 obj-y += vmce.o
diff --git a/xen/arch/x86/cpu/mcheck/mce.h b/xen/arch/x86/cpu/mcheck/mce.h
index d6d56aa232..7fbf1fa2ae 100644
--- a/xen/arch/x86/cpu/mcheck/mce.h
+++ b/xen/arch/x86/cpu/mcheck/mce.h
@@ -40,7 +40,11 @@ enum mcheck_type {
 };
 
 extern uint8_t cmci_apic_vector;
+#ifdef CONFIG_INTEL
 extern bool lmce_support;
+#else
+#define lmce_support (false)
+#endif
 
 /* Init functions */
 enum mcheck_type amd_mcheck_init(const struct cpuinfo_x86 *c, bool bsp);
@@ -120,7 +124,11 @@ DECLARE_PER_CPU(struct mca_banks *, poll_bankmask);
 DECLARE_PER_CPU(struct mca_banks *, no_cmci_banks);
 DECLARE_PER_CPU(struct mca_banks *, mce_clear_banks);
 
+#ifdef CONFIG_INTEL
 extern bool cmci_support;
+#else
+#define cmci_support (false)
+#endif
 extern bool is_mc_panic;
 extern bool mce_broadcast;
 extern void mcheck_mca_clearbanks(struct mca_banks *bankmask);
diff --git a/xen/arch/x86/cpu/mcheck/non-fatal.c b/xen/arch/x86/cpu/mcheck/non-fatal.c
index 33cacd15c2..2d91a3b1e0 100644
--- a/xen/arch/x86/cpu/mcheck/non-fatal.c
+++ b/xen/arch/x86/cpu/mcheck/non-fatal.c
@@ -24,14 +24,20 @@ static int __init cf_check init_nonfatal_mce_checker(void)
 	 * Check for non-fatal errors every MCE_RATE s
 	 */
 	switch (c->x86_vendor) {
+#ifdef CONFIG_AMD
 	case X86_VENDOR_AMD:
 	case X86_VENDOR_HYGON:
 		/* Assume we are on K8 or newer AMD or Hygon CPU here */
 		amd_nonfatal_mcheck_init(c);
 		break;
+#endif
+#ifdef CONFIG_INTEL
 	case X86_VENDOR_INTEL:
 		intel_nonfatal_mcheck_init(c);
 		break;
+#endif
+	default:
+		return -ENODEV;
 	}
 	printk(KERN_INFO "mcheck_poll: Machine check polling timer started.\n");
 	return 0;
-- 
2.25.1

Re: [XEN PATCH v2 5/5] x86/MCE: optional build of AMD/Intel MCE code

[Jan Beulich]

On 02.05.2024 11:21, Sergiy Kibrik wrote:
> Separate Intel/AMD-specific MCE code using CONFIG_{INTEL,AMD} config options.
> Now we can avoid build of mcheck code if support for specific platform is
> intentionally disabled by configuration.
> 
> Add default return value to init_nonfatal_mce_checker() routine -- in case
> of a build with both AMD and INTEL options are off (e.g. randconfig).

I'm afraid that, as before, I can't accept this as a justification for the
addition. The addition likely is wanted, but perhaps in a separate up-front
patch and explaining what's wrong when that's missing.

> Also global Intel-specific variables lmce_support & cmci_support have to be
> redefined if !INTEL, as they get checked in common code.

Them being checked in common code may have different resolution strategies.
The justification here imo is that, right now, both variables are only ever
written by mce_intel.c. As mentioned for vmce_has_lmce(), there's nothing
fundamentally preventing MCG_CAP from having respective bits set on a non-
Intel CPU.

> --- a/xen/arch/x86/cpu/mcheck/mce.h
> +++ b/xen/arch/x86/cpu/mcheck/mce.h
> @@ -40,7 +40,11 @@ enum mcheck_type {
>  };
>  
>  extern uint8_t cmci_apic_vector;
> +#ifdef CONFIG_INTEL
>  extern bool lmce_support;
> +#else
> +#define lmce_support (false)

Nit: Neither here nor ...

> @@ -120,7 +124,11 @@ DECLARE_PER_CPU(struct mca_banks *, poll_bankmask);
>  DECLARE_PER_CPU(struct mca_banks *, no_cmci_banks);
>  DECLARE_PER_CPU(struct mca_banks *, mce_clear_banks);
>  
> +#ifdef CONFIG_INTEL
>  extern bool cmci_support;
> +#else
> +#define cmci_support (false)

... here parentheses are really needed.

> --- a/xen/arch/x86/cpu/mcheck/non-fatal.c
> +++ b/xen/arch/x86/cpu/mcheck/non-fatal.c
> @@ -24,14 +24,20 @@ static int __init cf_check init_nonfatal_mce_checker(void)
>  	 * Check for non-fatal errors every MCE_RATE s
>  	 */
>  	switch (c->x86_vendor) {
> +#ifdef CONFIG_AMD
>  	case X86_VENDOR_AMD:
>  	case X86_VENDOR_HYGON:
>  		/* Assume we are on K8 or newer AMD or Hygon CPU here */
>  		amd_nonfatal_mcheck_init(c);
>  		break;
> +#endif
> +#ifdef CONFIG_INTEL
>  	case X86_VENDOR_INTEL:
>  		intel_nonfatal_mcheck_init(c);
>  		break;
> +#endif
> +	default:
> +		return -ENODEV;
>  	}
>  	printk(KERN_INFO "mcheck_poll: Machine check polling timer started.\n");
>  	return 0;

Along the lines of remarks on earlier patches, it would be a good opportunity
here to add missing blank lines between non-fall-through case blocks.

Jan

Re: [XEN PATCH v2 5/5] x86/MCE: optional build of AMD/Intel MCE code

[Sergiy Kibrik]

06.05.24 14:32, Jan Beulich:
> On 02.05.2024 11:21, Sergiy Kibrik wrote:
>> Separate Intel/AMD-specific MCE code using CONFIG_{INTEL,AMD} config options.
>> Now we can avoid build of mcheck code if support for specific platform is
>> intentionally disabled by configuration.
>>
>> Add default return value to init_nonfatal_mce_checker() routine -- in case
>> of a build with both AMD and INTEL options are off (e.g. randconfig).
> 
> I'm afraid that, as before, I can't accept this as a justification for the
> addition. The addition likely is wanted, but perhaps in a separate up-front
> patch and explaining what's wrong when that's missing.

sure, I'll do separate patch for that.

> 
>> Also global Intel-specific variables lmce_support & cmci_support have to be
>> redefined if !INTEL, as they get checked in common code.
> 
> Them being checked in common code may have different resolution strategies.
> The justification here imo is that, right now, both variables are only ever
> written by mce_intel.c. As mentioned for vmce_has_lmce(), there's nothing
> fundamentally preventing MCG_CAP from having respective bits set on a non-
> Intel CPU.
> 

so could these global variables just be moved to common code then? Like 
arch/x86/cpu/mcheck/mce.c ?

   -Sergiy

Re: [XEN PATCH v2 5/5] x86/MCE: optional build of AMD/Intel MCE code

[Jan Beulich]

On 13.05.2024 11:11, Sergiy Kibrik wrote:
> 06.05.24 14:32, Jan Beulich:
>> On 02.05.2024 11:21, Sergiy Kibrik wrote:
>>> Also global Intel-specific variables lmce_support & cmci_support have to be
>>> redefined if !INTEL, as they get checked in common code.
>>
>> Them being checked in common code may have different resolution strategies.
>> The justification here imo is that, right now, both variables are only ever
>> written by mce_intel.c. As mentioned for vmce_has_lmce(), there's nothing
>> fundamentally preventing MCG_CAP from having respective bits set on a non-
>> Intel CPU.
>>
> 
> so could these global variables just be moved to common code then? Like 
> arch/x86/cpu/mcheck/mce.c ?

That would likely be a better approach, yes.

Jan