| Message ID | 47d32ecff7b915bd23b6d13b76cedf4b39db71a2.1714640459.git.Sergiy_Kibrik@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: make Intel/AMD vPMU & MCE support configurable | expand |
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
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
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
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;
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(-)