[Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in HEST for corrected machine checks

[Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in HEST for corrected machine checks

[Max Asbock]

This patch adds code to the machine check handler to parse the 
APEI Hardware Error Source Table (HEST) and check if Firmware First mode
is set for the Correct Machine Check structure in HEST.
If Firmware First mode is set, the corrected machine checks are expected
to be reported through a Generic Hardware Error Source (GHES). Therefore in this
case CMCI will not be enabled to avoid interference with the firmware's
error reporting.

Signed-off-by: Max Asbock <masbock@linux.vnet.ibm.com>
---
 include/asm/mce.h                |    2 +
 kernel/cpu/mcheck/mce-apei.c     |   70 +++++++++++++++++++++++++++++++++++++++
 kernel/cpu/mcheck/mce-internal.h |    5 ++
 kernel/cpu/mcheck/mce_intel.c    |    6 ++-
 4 files changed, 81 insertions(+), 2 deletions(-)


diff -burpN linux-3.9/arch/x86/include/asm/mce.h linux-3.9.ff/arch/x86/include/asm/mce.h
--- linux-3.9/arch/x86/include/asm/mce.h	2013-05-02 09:38:16.000000000 -0700
+++ linux-3.9.ff/arch/x86/include/asm/mce.h	2013-05-03 13:20:36.000000000 -0700
@@ -144,12 +144,14 @@ DECLARE_PER_CPU(struct device *, mce_dev
 
 #ifdef CONFIG_X86_MCE_INTEL
 void mce_intel_feature_init(struct cpuinfo_x86 *c);
+void mce_intel_init_cmci(void);
 void cmci_clear(void);
 void cmci_reenable(void);
 void cmci_rediscover(void);
 void cmci_recheck(void);
 #else
 static inline void mce_intel_feature_init(struct cpuinfo_x86 *c) { }
+static inline void mce_intel_init_cmci(void) {}
 static inline void cmci_clear(void) {}
 static inline void cmci_reenable(void) {}
 static inline void cmci_rediscover(void) {}
diff -burpN linux-3.9/arch/x86/kernel/cpu/mcheck/mce-apei.c linux-3.9.ff/arch/x86/kernel/cpu/mcheck/mce-apei.c
--- linux-3.9/arch/x86/kernel/cpu/mcheck/mce-apei.c	2013-05-02 09:38:16.000000000 -0700
+++ linux-3.9.ff/arch/x86/kernel/cpu/mcheck/mce-apei.c	2013-05-06 14:25:48.000000000 -0700
@@ -32,6 +32,7 @@
 #include <linux/kernel.h>
 #include <linux/acpi.h>
 #include <linux/cper.h>
+#include <linux/init.h>
 #include <acpi/apei.h>
 #include <asm/mce.h>
 
@@ -148,3 +149,72 @@ int apei_clear_mce(u64 record_id)
 {
 	return erst_clear(record_id);
 }
+
+
+/* Support for Firmware First (FF) mode for Corrected Machine Checks
+ * as defined by APEI in the Hardware Error Source Table (HEST).
+ * (see section 18.3.2.2 in the ACPI spec, version 5.0)
+ *
+ * When Firmware First mode is specified in HEST for Corrected Machine Checks,
+ * these errors are expected to be reported by the firmware through a
+ * Generic Harware Event Source (GHES). In this case error reporting
+ * through CMCI should not be enabled so it won't interfere with the firmware.
+ *
+ * In the boot sequence HEST parsing is initialized after
+ * CPUs, therefore initially we don't know if Firmware First is set.
+ * We just assume yes, and enable CMCI in a late init call if FF is not set.
+ */
+static bool cmc_firmware_first;
+static bool cmc_firmware_first_checked;
+
+bool apei_cmc_firmware_first(void)
+{
+	if (!cmc_firmware_first_checked)
+		return true;
+	return cmc_firmware_first;
+}
+
+static int check_cmc_firmware_first(struct acpi_hest_header *hest_hdr, void *d)
+{
+	if (hest_hdr->type == ACPI_HEST_TYPE_IA32_CORRECTED_CHECK) {
+		struct acpi_hest_ia_corrected *cmc;
+
+		cmc = (struct acpi_hest_ia_corrected *)hest_hdr;
+
+		if (cmc->flags & ACPI_HEST_FIRMWARE_FIRST) {
+			cmc_firmware_first = true;
+			return 1;
+		}
+	}
+	return 0;
+}
+
+static void intel_late_init_cmci(void *data)
+{
+	if (!mce_available(__this_cpu_ptr(&cpu_info)))
+		return;
+
+	mce_intel_init_cmci();
+}
+
+static __init int honor_cmc_firmware_first(void)
+{
+	struct cpuinfo_x86 *c = __this_cpu_ptr(&cpu_info);
+
+	if (c->x86_vendor != X86_VENDOR_INTEL)
+		return 0;
+
+	apei_hest_parse(check_cmc_firmware_first, NULL);
+	cmc_firmware_first_checked = true;
+
+	if (cmc_firmware_first)
+		mca_cfg.cmci_disabled = true;
+
+	if (mca_cfg.cmci_disabled || mca_cfg.ignore_ce)
+		return 0;
+
+	on_each_cpu(intel_late_init_cmci, NULL, 1);
+	return 0;
+}
+
+late_initcall(honor_cmc_firmware_first);
diff -burpN linux-3.9/arch/x86/kernel/cpu/mcheck/mce_intel.c linux-3.9.ff/arch/x86/kernel/cpu/mcheck/mce_intel.c
--- linux-3.9/arch/x86/kernel/cpu/mcheck/mce_intel.c	2013-05-02 09:38:16.000000000 -0700
+++ linux-3.9.ff/arch/x86/kernel/cpu/mcheck/mce_intel.c	2013-05-03 13:21:53.000000000 -0700
@@ -315,7 +315,7 @@ void cmci_reenable(void)
 		cmci_discover(banks);
 }
 
-static void intel_init_cmci(void)
+void mce_intel_init_cmci(void)
 {
 	int banks;
 
@@ -337,5 +337,7 @@ static void intel_init_cmci(void)
 void mce_intel_feature_init(struct cpuinfo_x86 *c)
 {
 	intel_init_thermal(c);
-	intel_init_cmci();
+
+	if (!apei_cmc_firmware_first())
+		mce_intel_init_cmci();
 }
diff -burpN linux-3.9/arch/x86/kernel/cpu/mcheck/mce-internal.h linux-3.9.ff/arch/x86/kernel/cpu/mcheck/mce-internal.h
--- linux-3.9/arch/x86/kernel/cpu/mcheck/mce-internal.h	2013-05-02 09:38:16.000000000 -0700
+++ linux-3.9.ff/arch/x86/kernel/cpu/mcheck/mce-internal.h	2013-05-03 13:20:36.000000000 -0700
@@ -43,6 +43,7 @@ int apei_write_mce(struct mce *m);
 ssize_t apei_read_mce(struct mce *m, u64 *record_id);
 int apei_check_mce(void);
 int apei_clear_mce(u64 record_id);
+bool apei_cmc_firmware_first(void);
 #else
 static inline int apei_write_mce(struct mce *m)
 {
@@ -60,4 +61,8 @@ static inline int apei_clear_mce(u64 rec
 {
 	return -EINVAL;
 }
+static inline bool apei_cmc_firmware_first(void)
+{
+	return 0;
+}
 #endif

Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in HEST for corrected machine checks

[Borislav Petkov]

On Mon, May 06, 2013 at 03:58:22PM -0700, Max Asbock wrote:
> This patch adds code to the machine check handler to parse the 
> APEI Hardware Error Source Table (HEST) and check if Firmware First mode
> is set for the Correct Machine Check structure in HEST.
> If Firmware First mode is set, the corrected machine checks are expected
> to be reported through a Generic Hardware Error Source (GHES). Therefore in this
> case CMCI will not be enabled to avoid interference with the firmware's
> error reporting.
> 
> Signed-off-by: Max Asbock <masbock@linux.vnet.ibm.com>
> ---
>  include/asm/mce.h                |    2 +
>  kernel/cpu/mcheck/mce-apei.c     |   70 +++++++++++++++++++++++++++++++++++++++
>  kernel/cpu/mcheck/mce-internal.h |    5 ++
>  kernel/cpu/mcheck/mce_intel.c    |    6 ++-
>  4 files changed, 81 insertions(+), 2 deletions(-)
> 
> 
> diff -burpN linux-3.9/arch/x86/include/asm/mce.h linux-3.9.ff/arch/x86/include/asm/mce.h
> --- linux-3.9/arch/x86/include/asm/mce.h	2013-05-02 09:38:16.000000000 -0700
> +++ linux-3.9.ff/arch/x86/include/asm/mce.h	2013-05-03 13:20:36.000000000 -0700
> @@ -144,12 +144,14 @@ DECLARE_PER_CPU(struct device *, mce_dev
>  
>  #ifdef CONFIG_X86_MCE_INTEL
>  void mce_intel_feature_init(struct cpuinfo_x86 *c);
> +void mce_intel_init_cmci(void);
>  void cmci_clear(void);
>  void cmci_reenable(void);
>  void cmci_rediscover(void);
>  void cmci_recheck(void);
>  #else
>  static inline void mce_intel_feature_init(struct cpuinfo_x86 *c) { }
> +static inline void mce_intel_init_cmci(void) {}
>  static inline void cmci_clear(void) {}
>  static inline void cmci_reenable(void) {}
>  static inline void cmci_rediscover(void) {}
> diff -burpN linux-3.9/arch/x86/kernel/cpu/mcheck/mce-apei.c linux-3.9.ff/arch/x86/kernel/cpu/mcheck/mce-apei.c
> --- linux-3.9/arch/x86/kernel/cpu/mcheck/mce-apei.c	2013-05-02 09:38:16.000000000 -0700
> +++ linux-3.9.ff/arch/x86/kernel/cpu/mcheck/mce-apei.c	2013-05-06 14:25:48.000000000 -0700
> @@ -32,6 +32,7 @@
>  #include <linux/kernel.h>
>  #include <linux/acpi.h>
>  #include <linux/cper.h>
> +#include <linux/init.h>
>  #include <acpi/apei.h>
>  #include <asm/mce.h>
>  
> @@ -148,3 +149,72 @@ int apei_clear_mce(u64 record_id)
>  {
>  	return erst_clear(record_id);
>  }
> +
> +
> +/* Support for Firmware First (FF) mode for Corrected Machine Checks
> + * as defined by APEI in the Hardware Error Source Table (HEST).
> + * (see section 18.3.2.2 in the ACPI spec, version 5.0)
> + *
> + * When Firmware First mode is specified in HEST for Corrected Machine Checks,
> + * these errors are expected to be reported by the firmware through a
> + * Generic Harware Event Source (GHES). In this case error reporting
> + * through CMCI should not be enabled so it won't interfere with the firmware.
> + *
> + * In the boot sequence HEST parsing is initialized after
> + * CPUs, therefore initially we don't know if Firmware First is set.
> + * We just assume yes, and enable CMCI in a late init call if FF is not set.

I'm very sceptical about that assumption - I don't think that the
majority of boxes out there *now* have APEI and FF enabled (and those
working properly :)).

Besides, there's a bigger problem with this - in the window between
mce_intel_feature_init() and honor_cmc_firmware_first() you don't handle
any correctable errors, AFAICT.

> + */
> +static bool cmc_firmware_first;
> +static bool cmc_firmware_first_checked;
> +
> +bool apei_cmc_firmware_first(void)
> +{
> +	if (!cmc_firmware_first_checked)
> +		return true;
> +	return cmc_firmware_first;
> +}
> +
> +static int check_cmc_firmware_first(struct acpi_hest_header *hest_hdr, void *d)
> +{
> +	if (hest_hdr->type == ACPI_HEST_TYPE_IA32_CORRECTED_CHECK) {
> +		struct acpi_hest_ia_corrected *cmc;
> +
> +		cmc = (struct acpi_hest_ia_corrected *)hest_hdr;
> +
> +		if (cmc->flags & ACPI_HEST_FIRMWARE_FIRST) {
> +			cmc_firmware_first = true;
> +			return 1;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static void intel_late_init_cmci(void *data)
> +{
> +	if (!mce_available(__this_cpu_ptr(&cpu_info)))
> +		return;
> +
> +	mce_intel_init_cmci();
> +}
> +
> +static __init int honor_cmc_firmware_first(void)
> +{
> +	struct cpuinfo_x86 *c = __this_cpu_ptr(&cpu_info);
> +
> +	if (c->x86_vendor != X86_VENDOR_INTEL)
> +		return 0;
> +
> +	apei_hest_parse(check_cmc_firmware_first, NULL);
> +	cmc_firmware_first_checked = true;
> +
> +	if (cmc_firmware_first)
> +		mca_cfg.cmci_disabled = true;
> +
> +	if (mca_cfg.cmci_disabled || mca_cfg.ignore_ce)
> +		return 0;
> +
> +	on_each_cpu(intel_late_init_cmci, NULL, 1);
> +	return 0;
> +}
> +
> +late_initcall(honor_cmc_firmware_first);

This glue code should go to drivers/acpi/apei/ where the rest of the
APEI gunk lives.

> diff -burpN linux-3.9/arch/x86/kernel/cpu/mcheck/mce_intel.c linux-3.9.ff/arch/x86/kernel/cpu/mcheck/mce_intel.c
> --- linux-3.9/arch/x86/kernel/cpu/mcheck/mce_intel.c	2013-05-02 09:38:16.000000000 -0700
> +++ linux-3.9.ff/arch/x86/kernel/cpu/mcheck/mce_intel.c	2013-05-03 13:21:53.000000000 -0700
> @@ -315,7 +315,7 @@ void cmci_reenable(void)
>  		cmci_discover(banks);
>  }
>  
> -static void intel_init_cmci(void)
> +void mce_intel_init_cmci(void)
>  {
>  	int banks;
>  
> @@ -337,5 +337,7 @@ static void intel_init_cmci(void)
>  void mce_intel_feature_init(struct cpuinfo_x86 *c)
>  {
>  	intel_init_thermal(c);
> -	intel_init_cmci();
> +
> +	if (!apei_cmc_firmware_first())
> +		mce_intel_init_cmci();

Right, so I think the right approach should be to let CMCI get
initialized, then when apei gets to run, it calls cmci_clear() to turn
it off and continue its own FF handling. And this should be doable
without touching any MCA internals like mca_cfg and such. Simply
exporting the CMCI interfaces should be much cleaner.

Btw, we should keep the possibility open to be able to fall back to CMCI
at any point when, for some reason, it has been determined that APEI
functionality is out of order but we still want to have CMCI polling,
independent from the BIOS.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in HEST for corrected machine checks

[Max Asbock]

On Tue, 2013-05-07 at 01:25 +0200, Borislav Petkov wrote:
> On Mon, May 06, 2013 at 03:58:22PM -0700, Max Asbock wrote:
> > This patch adds code to the machine check handler to parse the 
> > APEI Hardware Error Source Table (HEST) and check if Firmware First mode
> > is set for the Correct Machine Check structure in HEST.
> > If Firmware First mode is set, the corrected machine checks are expected
> > to be reported through a Generic Hardware Error Source (GHES). Therefore in this
> > case CMCI will not be enabled to avoid interference with the firmware's
> > error reporting.
> > 
> > Signed-off-by: Max Asbock <masbock@linux.vnet.ibm.com>
> > ---
> >  include/asm/mce.h                |    2 +
> >  kernel/cpu/mcheck/mce-apei.c     |   70 +++++++++++++++++++++++++++++++++++++++
> >  kernel/cpu/mcheck/mce-internal.h |    5 ++
> >  kernel/cpu/mcheck/mce_intel.c    |    6 ++-
> >  4 files changed, 81 insertions(+), 2 deletions(-)
> > 
> > 
> > diff -burpN linux-3.9/arch/x86/include/asm/mce.h linux-3.9.ff/arch/x86/include/asm/mce.h
> > --- linux-3.9/arch/x86/include/asm/mce.h	2013-05-02 09:38:16.000000000 -0700
> > +++ linux-3.9.ff/arch/x86/include/asm/mce.h	2013-05-03 13:20:36.000000000 -0700
> > @@ -144,12 +144,14 @@ DECLARE_PER_CPU(struct device *, mce_dev
> >  
> >  #ifdef CONFIG_X86_MCE_INTEL
> >  void mce_intel_feature_init(struct cpuinfo_x86 *c);
> > +void mce_intel_init_cmci(void);
> >  void cmci_clear(void);
> >  void cmci_reenable(void);
> >  void cmci_rediscover(void);
> >  void cmci_recheck(void);
> >  #else
> >  static inline void mce_intel_feature_init(struct cpuinfo_x86 *c) { }
> > +static inline void mce_intel_init_cmci(void) {}
> >  static inline void cmci_clear(void) {}
> >  static inline void cmci_reenable(void) {}
> >  static inline void cmci_rediscover(void) {}
> > diff -burpN linux-3.9/arch/x86/kernel/cpu/mcheck/mce-apei.c linux-3.9.ff/arch/x86/kernel/cpu/mcheck/mce-apei.c
> > --- linux-3.9/arch/x86/kernel/cpu/mcheck/mce-apei.c	2013-05-02 09:38:16.000000000 -0700
> > +++ linux-3.9.ff/arch/x86/kernel/cpu/mcheck/mce-apei.c	2013-05-06 14:25:48.000000000 -0700
> > @@ -32,6 +32,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/acpi.h>
> >  #include <linux/cper.h>
> > +#include <linux/init.h>
> >  #include <acpi/apei.h>
> >  #include <asm/mce.h>
> >  
> > @@ -148,3 +149,72 @@ int apei_clear_mce(u64 record_id)
> >  {
> >  	return erst_clear(record_id);
> >  }
> > +
> > +
> > +/* Support for Firmware First (FF) mode for Corrected Machine Checks
> > + * as defined by APEI in the Hardware Error Source Table (HEST).
> > + * (see section 18.3.2.2 in the ACPI spec, version 5.0)
> > + *
> > + * When Firmware First mode is specified in HEST for Corrected Machine Checks,
> > + * these errors are expected to be reported by the firmware through a
> > + * Generic Harware Event Source (GHES). In this case error reporting
> > + * through CMCI should not be enabled so it won't interfere with the firmware.
> > + *
> > + * In the boot sequence HEST parsing is initialized after
> > + * CPUs, therefore initially we don't know if Firmware First is set.
> > + * We just assume yes, and enable CMCI in a late init call if FF is not set.
> 
> I'm very sceptical about that assumption - I don't think that the
> majority of boxes out there *now* have APEI and FF enabled (and those
> working properly :)).
> 
> Besides, there's a bigger problem with this - in the window between
> mce_intel_feature_init() and honor_cmc_firmware_first() you don't handle
> any correctable errors, AFAICT.

You bring up good points here. Most systems probably don't support APEI
and FF right now. Therefore a patch should not affect the behavior of
the majority of systems. And this current patch does leave a window
where CMCI is not enabled for all systems.

I actually have a different version of this patch where CMCI is always
enabled first and subsequently disabled if a HEST and FF is found, more
similar to what you suggest below. 

> 
> > + */
> > +static bool cmc_firmware_first;
> > +static bool cmc_firmware_first_checked;
> > +
> > +bool apei_cmc_firmware_first(void)
> > +{
> > +	if (!cmc_firmware_first_checked)
> > +		return true;
> > +	return cmc_firmware_first;
> > +}
> > +
> > +static int check_cmc_firmware_first(struct acpi_hest_header *hest_hdr, void *d)
> > +{
> > +	if (hest_hdr->type == ACPI_HEST_TYPE_IA32_CORRECTED_CHECK) {
> > +		struct acpi_hest_ia_corrected *cmc;
> > +
> > +		cmc = (struct acpi_hest_ia_corrected *)hest_hdr;
> > +
> > +		if (cmc->flags & ACPI_HEST_FIRMWARE_FIRST) {
> > +			cmc_firmware_first = true;
> > +			return 1;
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> > +static void intel_late_init_cmci(void *data)
> > +{
> > +	if (!mce_available(__this_cpu_ptr(&cpu_info)))
> > +		return;
> > +
> > +	mce_intel_init_cmci();
> > +}
> > +
> > +static __init int honor_cmc_firmware_first(void)
> > +{
> > +	struct cpuinfo_x86 *c = __this_cpu_ptr(&cpu_info);
> > +
> > +	if (c->x86_vendor != X86_VENDOR_INTEL)
> > +		return 0;
> > +
> > +	apei_hest_parse(check_cmc_firmware_first, NULL);
> > +	cmc_firmware_first_checked = true;
> > +
> > +	if (cmc_firmware_first)
> > +		mca_cfg.cmci_disabled = true;
> > +
> > +	if (mca_cfg.cmci_disabled || mca_cfg.ignore_ce)
> > +		return 0;
> > +
> > +	on_each_cpu(intel_late_init_cmci, NULL, 1);
> > +	return 0;
> > +}
> > +
> > +late_initcall(honor_cmc_firmware_first);
> 
> This glue code should go to drivers/acpi/apei/ where the rest of the
> APEI gunk lives.

But this is glue code between MCE and APEI, therefore I thought a file
named mce-apei.c would be a good place. If we put it into apei then we
might have to export some of the MCE interfaces, whereas
apei_hest_parse() is already exported.

> 
> > diff -burpN linux-3.9/arch/x86/kernel/cpu/mcheck/mce_intel.c linux-3.9.ff/arch/x86/kernel/cpu/mcheck/mce_intel.c
> > --- linux-3.9/arch/x86/kernel/cpu/mcheck/mce_intel.c	2013-05-02 09:38:16.000000000 -0700
> > +++ linux-3.9.ff/arch/x86/kernel/cpu/mcheck/mce_intel.c	2013-05-03 13:21:53.000000000 -0700
> > @@ -315,7 +315,7 @@ void cmci_reenable(void)
> >  		cmci_discover(banks);
> >  }
> >  
> > -static void intel_init_cmci(void)
> > +void mce_intel_init_cmci(void)
> >  {
> >  	int banks;
> >  
> > @@ -337,5 +337,7 @@ static void intel_init_cmci(void)
> >  void mce_intel_feature_init(struct cpuinfo_x86 *c)
> >  {
> >  	intel_init_thermal(c);
> > -	intel_init_cmci();
> > +
> > +	if (!apei_cmc_firmware_first())
> > +		mce_intel_init_cmci();
> 
> Right, so I think the right approach should be to let CMCI get
> initialized, then when apei gets to run, it calls cmci_clear() to turn
> it off and continue its own FF handling. And this should be doable
> without touching any MCA internals like mca_cfg and such. Simply
> exporting the CMCI interfaces should be much cleaner.

I can post another version of this patch where CMCI gets initialized
first. 

> 
> Btw, we should keep the possibility open to be able to fall back to CMCI
> at any point when, for some reason, it has been determined that APEI
> functionality is out of order but we still want to have CMCI polling,
> independent from the BIOS.

Agreed, it would be good to be able to override FF.

thanks,
Max

Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in HEST for corrected machine checks

[Borislav Petkov]

On Mon, May 06, 2013 at 08:32:46PM -0700, Max Asbock wrote:
> But this is glue code between MCE and APEI, therefore I thought a
> file named mce-apei.c would be a good place. If we put it into apei
> then we might have to export some of the MCE interfaces, whereas
> apei_hest_parse() is already exported.

Right, I don't want to expose any MCA internals to other subsystems
unless it is really necessary.

AFAICT, you need an ON and OFF switch for CMCI which is callable from
outside. So you can adapt/adjust the code in mce_intel.c to do so
without adding any other code.

Unless I'm missing something. But I don't think so, the high-level
sequence looks like this:

* MCA init
 -> CMCI init
* APEI init:
 -> if FF
    -> CMCI off

At a quick glance, simply doing:

	on_each_cpu(cmci_clear_func, NULL, 1);

should work. You'd need to define a proper cmci_clear_func prototype but
that should be trivial...

Pretty straight-forward, no?

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in HEST for corrected machine checks

[Max Asbock]

On Tue, 2013-05-07 at 15:19 +0200, Borislav Petkov wrote:
> On Mon, May 06, 2013 at 08:32:46PM -0700, Max Asbock wrote:
> > But this is glue code between MCE and APEI, therefore I thought a
> > file named mce-apei.c would be a good place. If we put it into apei
> > then we might have to export some of the MCE interfaces, whereas
> > apei_hest_parse() is already exported.
> 
> Right, I don't want to expose any MCA internals to other subsystems
> unless it is really necessary.
> 
> AFAICT, you need an ON and OFF switch for CMCI which is callable from
> outside. So you can adapt/adjust the code in mce_intel.c to do so
> without adding any other code.
> 
> Unless I'm missing something. But I don't think so, the high-level
> sequence looks like this:
> 
> * MCA init
>  -> CMCI init
> * APEI init:
>  -> if FF
>     -> CMCI off
> 
> At a quick glance, simply doing:
> 
> 	on_each_cpu(cmci_clear_func, NULL, 1);
> 
> should work. You'd need to define a proper cmci_clear_func prototype but
> that should be trivial...
> 

So something like the following patch might be closer:


Signed-off-by: Max Asbock <masbock@linux.vnet.ibm.com>
---
 mce-apei.c |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff -burpN linux-3.9/arch/x86/kernel/cpu/mcheck/mce-apei.c linux-3.9.ff2/arch/x86/kernel/cpu/mcheck/mce-apei.c
--- linux-3.9/arch/x86/kernel/cpu/mcheck/mce-apei.c	2013-05-02 09:38:16.000000000 -0700
+++ linux-3.9.ff2/arch/x86/kernel/cpu/mcheck/mce-apei.c	2013-05-07 08:12:16.000000000 -0700
@@ -32,6 +32,7 @@
 #include <linux/kernel.h>
 #include <linux/acpi.h>
 #include <linux/cper.h>
+#include <linux/init.h>
 #include <acpi/apei.h>
 #include <asm/mce.h>
 
@@ -148,3 +149,54 @@ int apei_clear_mce(u64 record_id)
 {
 	return erst_clear(record_id);
 }
+
+
+/* Support for Firmware First (FF) mode for Corrected Machine Checks
+ * as defined by APEI in the Hardware Error Source Table (HEST).
+ * (see section 18.3.2.2 in the ACPI spec, version 5.0)
+ *
+ * When Firmware First mode is specified in HEST for Corrected Machine Checks,
+ * these errors are expected to be reported by the firmware through a
+ * Generic Harware Event Source (GHES). In this case error reporting
+ * through CMCI should not be enabled so it won't interfere with the firmware.
+ *
+ * In the boot sequence HEST parsing is initialized after
+ * CPUs, therefore initially we don't know if Firmware First is set.
+ * We enable CMCI at first and then disable it in a late init call if FF is set.
+ */
+
+static bool cmc_firmware_first;
+
+static int check_cmc_firmware_first(struct acpi_hest_header *hest_hdr, void *d)
+{
+	if (hest_hdr->type == ACPI_HEST_TYPE_IA32_CORRECTED_CHECK) {
+		struct acpi_hest_ia_corrected *cmc;
+
+		cmc = (struct acpi_hest_ia_corrected *)hest_hdr;
+		if (cmc->flags & ACPI_HEST_FIRMWARE_FIRST) {
+			cmc_firmware_first = true;
+			return 1;
+		}
+	}
+	return 0;
+}
+
+static void disable_cmci(void *data)
+{
+	if (!mce_available(__this_cpu_ptr(&cpu_info)))
+		return;
+	cmci_clear();
+}
+
+static __init int honor_cmc_firmware_first(void)
+{
+	apei_hest_parse(check_cmc_firmware_first, NULL);
+
+	if (cmc_firmware_first && !mca_cfg.cmci_disabled) {
+		on_each_cpu(disable_cmci, NULL, 1);
+		mca_cfg.cmci_disabled = true;
+	}
+	return 0;
+}
+
+late_initcall(honor_cmc_firmware_first);

Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in HEST for corrected machine checks

[Borislav Petkov]

On Tue, May 07, 2013 at 08:40:14AM -0700, Max Asbock wrote:
> So something like the following patch might be closer:

Yep, we're getting there.

[ … ]

> +static bool cmc_firmware_first;

Why the global variable?

You can pass it as a pointer to apei_hest_parse so that
check_cmc_firmware_first can set its value. See below:

> +static int check_cmc_firmware_first(struct acpi_hest_header *hest_hdr, void *d)
> +{
> +	if (hest_hdr->type == ACPI_HEST_TYPE_IA32_CORRECTED_CHECK) {
> +		struct acpi_hest_ia_corrected *cmc;
> +
> +		cmc = (struct acpi_hest_ia_corrected *)hest_hdr;
> +		if (cmc->flags & ACPI_HEST_FIRMWARE_FIRST) {
> +			cmc_firmware_first = true;
> +			return 1;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static void disable_cmci(void *data)
> +{
> +	if (!mce_available(__this_cpu_ptr(&cpu_info)))
> +		return;
> +	cmci_clear();
> +}
> +
> +static __init int honor_cmc_firmware_first(void)
> +{
> +	apei_hest_parse(check_cmc_firmware_first, NULL);

> +
> +	if (cmc_firmware_first && !mca_cfg.cmci_disabled) {
> +		on_each_cpu(disable_cmci, NULL, 1);
> +		mca_cfg.cmci_disabled = true;

Well, in looking at the code more, there's set_cmci_disabled() in mce.c.
You can make a helper which contains only the meat of this function
without the buffer parsing and export it to users.

This way you don't need to touch anything else in mce.c - only this
helper.

IOW, I think it can be as simple as this:

static __init int honor_cmc_firmware_first(void)
{
	bool ff = false;

	/*
	 * XXX: you need to check error code here and return early accordingly
	 */
	apei_hest_parse(check_cmc_firmware_first, &ff);

	if (ff)
		disable_cmci();		/* <--- helper */

	/*
	 * Perhaps an informational message here:
	 */
	pr_info("APEI: Switching to Firmware-First mode.\n");

	return 0;
}
late_initcall(honor_cmc_firmware_first);

And this chunk should be in drivers/acpi/apei/ and not in arch/x86/.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in HEST for corrected machine checks

[Luck, Tony]

What happens when you succeed and turn off CMCI?  Will Linux
still poll the machine check banks looking for corrected errors?

-Tony

Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in HEST for corrected machine checks

[Borislav Petkov]

On Tue, May 07, 2013 at 09:34:28PM +0000, Luck, Tony wrote:
> What happens when you succeed and turn off CMCI?  Will Linux
> still poll the machine check banks looking for corrected errors?

Of course - there's the mce_timer. Which begs the question: is FF going
to take over *all* correctable MCEs or only a subset?

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in HEST for corrected machine checks

[Borislav Petkov]

On Tue, May 07, 2013 at 08:40:14AM -0700, Max Asbock wrote:
> + * When Firmware First mode is specified in HEST for Corrected
> + * Machine Checks, these errors are expected to be reported by the
> + * firmware through a Generic Harware Event Source (GHES). In this
> + * case error reporting through CMCI should not be enabled so it
> + * won't interfere with the firmware.

Ok, hold on: by accident I fell over the code in ghes_init() which
blubbers about APEI FF too. Isn't that the same thing?

Because if yes, you won't need any of the HEST header parsing here. So
what's up?

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

RE: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in HEST for corrected machine checks

[Luck, Tony]

> Because if yes, you won't need any of the HEST header parsing here. So
> what's up?

I think[1] HEST can tell you which banks report using APEI (as well as/instead of) CMCI.

So a full solution should take into account lots of possibilities:

1) Some banks may not support CMCI at all (bit 30 in MCi_CTL2 register ignores attempts to enable). For these banks Linux should poll.
2) BIOS may support generation of APEI records for certain classes of errors. HEST will say which banks are affected, and if we prefer APEI, we should disable CMCI for these banks (and not poll them).
3) Some banks may support CMCI, but don't have BIOS support to generate APEI records.  We should continue to enable CMCI for these.

We should also take advantage of more information provided in the APEI record.  We only look at mem_err->physical_addr for memory errors - but there are a ton of other potentially interesting things in there.

-Tony

[1] But I may be misreading ACPI spec - unfortunately it just has descriptions of *what* bits do, but no rationale as to *why* anyone would want to use them, or tradeoffs between different operating modes. Willing to be re-educated if this is all horribly wrong.

Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in HEST for corrected machine checks

[Borislav Petkov]

On Wed, May 08, 2013 at 09:55:28PM +0000, Luck, Tony wrote:
> > Because if yes, you won't need any of the HEST header parsing here. So
> > what's up?
> 
> I think[1] HEST can tell you which banks report using APEI (as well as/instead of) CMCI.
> 
> So a full solution should take into account lots of possibilities:

Great, more fun! :-)

> 1) Some banks may not support CMCI at all (bit 30 in MCi_CTL2 register
> ignores attempts to enable). For these banks Linux should poll.

That's the detection method, try setting bit 30 to see if it sticks?

> 2) BIOS may support generation of APEI records for certain classes of
> errors. HEST will say which banks are affected, and if we prefer APEI,
> we should disable CMCI for these banks (and not poll them).

Ok, so it looks like code should iterate over those and remove them from
the mce_banks list we pass on to machine_check_poll.

> 3) Some banks may support CMCI, but don't have BIOS support to
> generate APEI records. We should continue to enable CMCI for these.

How do you get that out of the HEST? The inability to generate APEI
records.

> We should also take advantage of more information provided in the APEI
> record. We only look at mem_err->physical_addr for memory errors - but
> there are a ton of other potentially interesting things in there.

Ok.

> [1] But I may be misreading ACPI spec - unfortunately it just has
> descriptions of *what* bits do, but no rationale as to *why* anyone
> would want to use them, or tradeoffs between different operating
> modes. Willing to be re-educated if this is all horribly wrong.

Me too. If you haven't noticed, I'm avoiding the APEI spec as much as
possible. :-)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

RE: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in HEST for corrected machine checks

[Luck, Tony]

>> 1) Some banks may not support CMCI at all (bit 30 in MCi_CTL2 register
>> ignores attempts to enable). For these banks Linux should poll.
>
> That's the detection method, try setting bit 30 to see if it sticks?

Yes. See Intel SDM Vol 3B section "15.3.2.5 IA32_MCi_CTL2 MSRs"

>> 2) BIOS may support generation of APEI records for certain classes of
>> errors. HEST will say which banks are affected, and if we prefer APEI,
>> we should disable CMCI for these banks (and not poll them).

> Ok, so it looks like code should iterate over those and remove them from
> the mce_banks list we pass on to machine_check_poll.

I think so.  the mce_banks bitmaps keep track of which cpu is responsible
for checking a bank (because banks can be shared by multiple cpus ... e.g.
socket level bank is same to all logical cpus on a package. Core level bank
is shared by just the two hyperthreads on a core).  If we trust APEI to handle
things, then the bank shouldn't be owned by any cpu.

>> 3) Some banks may support CMCI, but don't have BIOS support to
>> generate APEI records. We should continue to enable CMCI for these.
>
> How do you get that out of the HEST? The inability to generate APEI
> records.

Perhaps just by whatever banks you find left over after iterating the ones
that *are* in HEST?  There isn't an explicit list.

-Tony

RE: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in HEST for corrected machine checks

[Max Asbock]

I'll try to summarize the situation:

We proposed two iterations of a patch that would parse HEST for a
Corrected Machine Check entry and cause CMCI to be disabled if the
Firmware First flag was found to be on in that entry.
Several shortcomings of this approach were subsequently pointed out:
a) Disabling CMCI doesn't go far enough. If the firmware wants to
control corrected machine checks then we shouldn't even be polling the
MCi_STATUS registers. Therefore we need to disable CMCI and disable
polling if FF is set.

b) The firmware may take over only a subset of the possible corrected
machine check events. If we turn off CMCI (and polling) for all banks we
may miss out on some types of errors. Therefore we should not
indiscriminately disable CMCI on all banks.

The question arose whether the APEI spec allows to specify individual
machine check banks which fall under FF control. The answer appears to
be 'possibly'. The Corrected Machine Check (CMC) structure defined in
the APEI spec allows for a list of Machine Check Bank structures which
could be used to designate a set of banks falling under FF control.
However, the spec is silent on how the list of Machine Check Bank
structures in the CMC structure is be used. 

Further steps in this endeavor may depend on the interpretation of the
CMC structure in APEI an whether we can specify individual machine check
banks that fall under FF control.

- Max

Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in HEST for corrected machine checks

[Borislav Petkov]

And I was wondering why this is not reaching lkml. Fixed.

On Fri, May 10, 2013 at 10:59:04AM -0700, Max Asbock wrote:
> I'll try to summarize the situation:
> 
> We proposed two iterations of a patch that would parse HEST for a
> Corrected Machine Check entry and cause CMCI to be disabled if the
> Firmware First flag was found to be on in that entry.
> Several shortcomings of this approach were subsequently pointed out:
> a) Disabling CMCI doesn't go far enough. If the firmware wants to
> control corrected machine checks then we shouldn't even be polling the
> MCi_STATUS registers. Therefore we need to disable CMCI and disable
> polling if FF is set.
> 
> b) The firmware may take over only a subset of the possible corrected
> machine check events. If we turn off CMCI (and polling) for all banks we
> may miss out on some types of errors. Therefore we should not
> indiscriminately disable CMCI on all banks.
> 
> The question arose whether the APEI spec allows to specify individual
> machine check banks which fall under FF control. The answer appears to
> be 'possibly'. The Corrected Machine Check (CMC) structure defined in
> the APEI spec allows for a list of Machine Check Bank structures which
> could be used to designate a set of banks falling under FF control.
> However, the spec is silent on how the list of Machine Check Bank
> structures in the CMC structure is be used. 
> 
> Further steps in this endeavor may depend on the interpretation of the
> CMC structure in APEI an whether we can specify individual machine check
> banks that fall under FF control.

Right, so we need a complete solution where all FF banks are excluded
from polling. The question is, does the CMC table in APEI enumerate
*all* FF banks or we need to parse something else too.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[PATCH] Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in

[Naveen N. Rao]

HEST for corrected machine checks

Here's a patch that implements this technique. If the firmware advertises
support for firmware first mode in the CMC structure, we disable CMCI and
polling for all the MCA banks listed in the CMC structure.

- Naveen

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/x86/include/asm/mce.h             |    3 ++
 arch/x86/kernel/cpu/mcheck/mce_intel.c |   38 +++++++++++++++++++++++++++++++
 drivers/acpi/apei/hest.c               |   39 ++++++++++++++++++++++++++++++++
 3 files changed, 80 insertions(+)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index fa5f71e..9c91683 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -188,6 +188,9 @@ extern void register_mce_write_callback(ssize_t (*)(struct file *filp,
 				    const char __user *ubuf,
 				    size_t usize, loff_t *off));
 
+/* Disable CMCI/polling for MCA bank claimed by firmware */
+extern void mce_disable_bank(int bank);
+
 /*
  * Exception handler
  */
diff --git a/arch/x86/kernel/cpu/mcheck/mce_intel.c b/arch/x86/kernel/cpu/mcheck/mce_intel.c
index ae1697c..bc0307d 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_intel.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_intel.c
@@ -26,6 +26,9 @@
 
 static DEFINE_PER_CPU(mce_banks_t, mce_banks_owned);
 
+/* MCA banks controlled through firmware first */
+static mce_banks_t mce_banks_disabled;
+
 /*
  * cmci_discover_lock protects against parallel discovery attempts
  * which could race against each other.
@@ -191,6 +194,10 @@ static void cmci_discover(int banks)
 		if (test_bit(i, owned))
 			continue;
 
+		/* Skip banks in firmware first mode */
+		if (test_bit(i, mce_banks_disabled))
+			continue;
+
 		rdmsrl(MSR_IA32_MCx_CTL2(i), val);
 
 		/* Already owned by someone else? */
@@ -315,6 +322,37 @@ void cmci_reenable(void)
 		cmci_discover(banks);
 }
 
+static void cmci_disable_bank(void *arg)
+{
+	int banks;
+	unsigned long flags;
+	u64 val;
+	int bank = *((int *)arg);
+
+	/* Ensure we don't poll this bank */
+	__clear_bit(bank, __get_cpu_var(mce_poll_banks));
+
+	if (!cmci_supported(&banks))
+		return;
+
+	raw_spin_lock_irqsave(&cmci_discover_lock, flags);
+
+	/* Disable CMCI */
+	rdmsrl(MSR_IA32_MCx_CTL2(bank), val);
+	val &= ~MCI_CTL2_CMCI_EN;
+	wrmsrl(MSR_IA32_MCx_CTL2(bank), val);
+
+	__clear_bit(bank, __get_cpu_var(mce_banks_owned));
+
+	raw_spin_unlock_irqrestore(&cmci_discover_lock, flags);
+}
+
+void mce_disable_bank(int bank)
+{
+	set_bit(bank, mce_banks_disabled);
+	on_each_cpu(cmci_disable_bank, &bank, 1);
+}
+
 static void intel_init_cmci(void)
 {
 	int banks;
diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index f5ef5d5..765d8bf 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -36,6 +36,7 @@
 #include <linux/io.h>
 #include <linux/platform_device.h>
 #include <acpi/apei.h>
+#include <asm/mce.h>
 
 #include "apei-internal.h"
 
@@ -121,6 +122,42 @@ int apei_hest_parse(apei_hest_func_t func, void *data)
 }
 EXPORT_SYMBOL_GPL(apei_hest_parse);
 
+/*
+ * Check if firmware advertises firmware first mode. We need FF bit to be set
+ * along with a set of MC banks which work in FF mode.
+ */
+static int __init hest_parse_cmc(struct acpi_hest_header *hest_hdr, void *data)
+{
+	int i;
+	struct acpi_hest_ia_corrected *cmc;
+	struct acpi_hest_ia_error_bank *mc_bank;
+
+	if (hest_hdr->type != ACPI_HEST_TYPE_IA32_CORRECTED_CHECK)
+		return 0;
+
+	if (!((struct acpi_hest_generic *)hest_hdr)->enabled)
+		return 0;
+
+	cmc = (struct acpi_hest_ia_corrected *)hest_hdr;
+	if (!(cmc->flags & ACPI_HEST_FIRMWARE_FIRST))
+		return 0;
+
+	/*
+	 * We expect HEST to provide a list of MC banks that
+	 * report errors through firmware first mode.
+	 */
+	if (cmc->num_hardware_banks <= 0)
+		return 0;
+
+	pr_info("HEST: Enabling Firmware First mode for corrected errors\n");
+
+	mc_bank = (struct acpi_hest_ia_error_bank *)(cmc + 1);
+	for (i = 0; i < cmc->num_hardware_banks; i++, mc_bank++)
+		mce_disable_bank(mc_bank->bank_number);
+
+	return 0;
+}
+
 struct ghes_arr {
 	struct platform_device **ghes_devs;
 	unsigned int count;
@@ -227,6 +264,8 @@ void __init acpi_hest_init(void)
 		goto err;
 	}
 
+	apei_hest_parse(hest_parse_cmc, NULL);
+
 	if (!ghes_disable) {
 		rc = apei_hest_parse(hest_parse_ghes_count, &ghes_count);
 		if (rc)

Re: [PATCH] Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in

[Borislav Petkov]

On Fri, Jun 14, 2013 at 11:47:21PM +0530, Naveen N. Rao wrote:
> HEST for corrected machine checks
> 
> Here's a patch that implements this technique. If the firmware advertises
> support for firmware first mode in the CMC structure, we disable CMCI and
> polling for all the MCA banks listed in the CMC structure.

Yeah, this commit message needs a bit massaging. Don't be afraid to be
more verbose than you feel is necessary. :-)

> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/x86/include/asm/mce.h             |    3 ++
>  arch/x86/kernel/cpu/mcheck/mce_intel.c |   38 +++++++++++++++++++++++++++++++
>  drivers/acpi/apei/hest.c               |   39 ++++++++++++++++++++++++++++++++
>  3 files changed, 80 insertions(+)
> 
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index fa5f71e..9c91683 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -188,6 +188,9 @@ extern void register_mce_write_callback(ssize_t (*)(struct file *filp,
>  				    const char __user *ubuf,
>  				    size_t usize, loff_t *off));
>  
> +/* Disable CMCI/polling for MCA bank claimed by firmware */
> +extern void mce_disable_bank(int bank);
> +
>  /*
>   * Exception handler
>   */
> diff --git a/arch/x86/kernel/cpu/mcheck/mce_intel.c b/arch/x86/kernel/cpu/mcheck/mce_intel.c
> index ae1697c..bc0307d 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce_intel.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce_intel.c
> @@ -26,6 +26,9 @@
>  
>  static DEFINE_PER_CPU(mce_banks_t, mce_banks_owned);
>  
> +/* MCA banks controlled through firmware first */
> +static mce_banks_t mce_banks_disabled;
> +
>  /*
>   * cmci_discover_lock protects against parallel discovery attempts
>   * which could race against each other.
> @@ -191,6 +194,10 @@ static void cmci_discover(int banks)
>  		if (test_bit(i, owned))
>  			continue;
>  
> +		/* Skip banks in firmware first mode */
> +		if (test_bit(i, mce_banks_disabled))
> +			continue;
> +
>  		rdmsrl(MSR_IA32_MCx_CTL2(i), val);
>  
>  		/* Already owned by someone else? */
> @@ -315,6 +322,37 @@ void cmci_reenable(void)
>  		cmci_discover(banks);
>  }
>  
> +static void cmci_disable_bank(void *arg)
> +{
> +	int banks;
> +	unsigned long flags;
> +	u64 val;
> +	int bank = *((int *)arg);
> +
> +	/* Ensure we don't poll this bank */
> +	__clear_bit(bank, __get_cpu_var(mce_poll_banks));
> +
> +	if (!cmci_supported(&banks))
> +		return;

Hmm, so if CMCI is not supported, you just disabled polling of this bank
and returned here. Not good.

> +
> +	raw_spin_lock_irqsave(&cmci_discover_lock, flags);
> +
> +	/* Disable CMCI */
> +	rdmsrl(MSR_IA32_MCx_CTL2(bank), val);
> +	val &= ~MCI_CTL2_CMCI_EN;
> +	wrmsrl(MSR_IA32_MCx_CTL2(bank), val);
> +
> +	__clear_bit(bank, __get_cpu_var(mce_banks_owned));
> +
> +	raw_spin_unlock_irqrestore(&cmci_discover_lock, flags);

Almost the exact sequence is also in cmci_clear(). How about a static
function called __cmci_disable_bank which does that and the other
functions call it?

> +}
> +
> +void mce_disable_bank(int bank)
> +{
> +	set_bit(bank, mce_banks_disabled);
> +	on_each_cpu(cmci_disable_bank, &bank, 1);
> +}
> +
>  static void intel_init_cmci(void)
>  {
>  	int banks;
> diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
> index f5ef5d5..765d8bf 100644
> --- a/drivers/acpi/apei/hest.c
> +++ b/drivers/acpi/apei/hest.c
> @@ -36,6 +36,7 @@
>  #include <linux/io.h>
>  #include <linux/platform_device.h>
>  #include <acpi/apei.h>
> +#include <asm/mce.h>
>  
>  #include "apei-internal.h"
>  
> @@ -121,6 +122,42 @@ int apei_hest_parse(apei_hest_func_t func, void *data)
>  }
>  EXPORT_SYMBOL_GPL(apei_hest_parse);
>  
> +/*
> + * Check if firmware advertises firmware first mode. We need FF bit to be set
> + * along with a set of MC banks which work in FF mode.
> + */
> +static int __init hest_parse_cmc(struct acpi_hest_header *hest_hdr, void *data)
> +{
> +	int i;
> +	struct acpi_hest_ia_corrected *cmc;
> +	struct acpi_hest_ia_error_bank *mc_bank;
> +
> +	if (hest_hdr->type != ACPI_HEST_TYPE_IA32_CORRECTED_CHECK)
> +		return 0;
> +
> +	if (!((struct acpi_hest_generic *)hest_hdr)->enabled)
> +		return 0;
> +
> +	cmc = (struct acpi_hest_ia_corrected *)hest_hdr;

There is some crazy casting going on here: hest_hdr can be struct
acpi_hest_generic and struct acpi_hest_ia_corrected.

Since the ->enabled field overlaps in both structs and you want only it
as a struct acpi_hest_generic, and want the cmc later, why don't you do
this:

	struct acpi_hest_ia_corrected *cmc = (struct acpi_hest_ia_corrected *)hest_hdr;

	if (!cmc->enabled)
		...

Then this below:

> +	if (!(cmc->flags & ACPI_HEST_FIRMWARE_FIRST))
> +		return 0;

and so on... It should simplify the code a bit and drop the fun games
with casting.

> +
> +	/*
> +	 * We expect HEST to provide a list of MC banks that
> +	 * report errors through firmware first mode.
> +	 */
> +	if (cmc->num_hardware_banks <= 0)

->num_hardware_banks is unsigned char, so "== 0"

> +		return 0;
> +
> +	pr_info("HEST: Enabling Firmware First mode for corrected errors\n");

	pr_info(HEST_PFX "Enabling..." (fullstop at the end of the sentence).

Btw, this hest.c could use the standard pr_fmt mechanism.

> +
> +	mc_bank = (struct acpi_hest_ia_error_bank *)(cmc + 1);
> +	for (i = 0; i < cmc->num_hardware_banks; i++, mc_bank++)
> +		mce_disable_bank(mc_bank->bank_number);
> +
> +	return 0;
> +}
> +
>  struct ghes_arr {
>  	struct platform_device **ghes_devs;
>  	unsigned int count;
> @@ -227,6 +264,8 @@ void __init acpi_hest_init(void)
>  		goto err;
>  	}
>  
> +	apei_hest_parse(hest_parse_cmc, NULL);
> +
>  	if (!ghes_disable) {
>  		rc = apei_hest_parse(hest_parse_ghes_count, &ghes_count);
>  		if (rc)

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in

[Naveen N. Rao]

On 06/15/2013 08:18 PM, Borislav Petkov wrote:
> On Fri, Jun 14, 2013 at 11:47:21PM +0530, Naveen N. Rao wrote:
>> HEST for corrected machine checks
>>
>> Here's a patch that implements this technique. If the firmware advertises
>> support for firmware first mode in the CMC structure, we disable CMCI and
>> polling for all the MCA banks listed in the CMC structure.
>
> Yeah, this commit message needs a bit massaging. Don't be afraid to be
> more verbose than you feel is necessary. :-)

Yup, not to mention some stgit jitters with the subject ;)
I will re-write this.

>
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> ---
>>   arch/x86/include/asm/mce.h             |    3 ++
>>   arch/x86/kernel/cpu/mcheck/mce_intel.c |   38 +++++++++++++++++++++++++++++++
>>   drivers/acpi/apei/hest.c               |   39 ++++++++++++++++++++++++++++++++
>>   3 files changed, 80 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
>> index fa5f71e..9c91683 100644
>> --- a/arch/x86/include/asm/mce.h
>> +++ b/arch/x86/include/asm/mce.h
>> @@ -188,6 +188,9 @@ extern void register_mce_write_callback(ssize_t (*)(struct file *filp,
>>   				    const char __user *ubuf,
>>   				    size_t usize, loff_t *off));
>>
>> +/* Disable CMCI/polling for MCA bank claimed by firmware */
>> +extern void mce_disable_bank(int bank);
>> +
>>   /*
>>    * Exception handler
>>    */
>> diff --git a/arch/x86/kernel/cpu/mcheck/mce_intel.c b/arch/x86/kernel/cpu/mcheck/mce_intel.c
>> index ae1697c..bc0307d 100644
>> --- a/arch/x86/kernel/cpu/mcheck/mce_intel.c
>> +++ b/arch/x86/kernel/cpu/mcheck/mce_intel.c
>> @@ -26,6 +26,9 @@
>>
>>   static DEFINE_PER_CPU(mce_banks_t, mce_banks_owned);
>>
>> +/* MCA banks controlled through firmware first */
>> +static mce_banks_t mce_banks_disabled;
>> +
>>   /*
>>    * cmci_discover_lock protects against parallel discovery attempts
>>    * which could race against each other.
>> @@ -191,6 +194,10 @@ static void cmci_discover(int banks)
>>   		if (test_bit(i, owned))
>>   			continue;
>>
>> +		/* Skip banks in firmware first mode */
>> +		if (test_bit(i, mce_banks_disabled))
>> +			continue;
>> +
>>   		rdmsrl(MSR_IA32_MCx_CTL2(i), val);
>>
>>   		/* Already owned by someone else? */
>> @@ -315,6 +322,37 @@ void cmci_reenable(void)
>>   		cmci_discover(banks);
>>   }
>>
>> +static void cmci_disable_bank(void *arg)
>> +{
>> +	int banks;
>> +	unsigned long flags;
>> +	u64 val;
>> +	int bank = *((int *)arg);
>> +
>> +	/* Ensure we don't poll this bank */
>> +	__clear_bit(bank, __get_cpu_var(mce_poll_banks));
>> +
>> +	if (!cmci_supported(&banks))
>> +		return;
>
> Hmm, so if CMCI is not supported, you just disabled polling of this bank
> and returned here. Not good.

This is on purpose. If the bank doesn't support CMCI and we were polling 
it earlier, we want to disable that. The firmware should be notifying us 
of error events in either case.

>
>> +
>> +	raw_spin_lock_irqsave(&cmci_discover_lock, flags);
>> +
>> +	/* Disable CMCI */
>> +	rdmsrl(MSR_IA32_MCx_CTL2(bank), val);
>> +	val &= ~MCI_CTL2_CMCI_EN;
>> +	wrmsrl(MSR_IA32_MCx_CTL2(bank), val);
>> +
>> +	__clear_bit(bank, __get_cpu_var(mce_banks_owned));
>> +
>> +	raw_spin_unlock_irqrestore(&cmci_discover_lock, flags);
>
> Almost the exact sequence is also in cmci_clear(). How about a static
> function called __cmci_disable_bank which does that and the other
> functions call it?

Ok, will do.

>
>> +}
>> +
>> +void mce_disable_bank(int bank)
>> +{
>> +	set_bit(bank, mce_banks_disabled);
>> +	on_each_cpu(cmci_disable_bank, &bank, 1);
>> +}
>> +
>>   static void intel_init_cmci(void)
>>   {
>>   	int banks;
>> diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
>> index f5ef5d5..765d8bf 100644
>> --- a/drivers/acpi/apei/hest.c
>> +++ b/drivers/acpi/apei/hest.c
>> @@ -36,6 +36,7 @@
>>   #include <linux/io.h>
>>   #include <linux/platform_device.h>
>>   #include <acpi/apei.h>
>> +#include <asm/mce.h>
>>
>>   #include "apei-internal.h"
>>
>> @@ -121,6 +122,42 @@ int apei_hest_parse(apei_hest_func_t func, void *data)
>>   }
>>   EXPORT_SYMBOL_GPL(apei_hest_parse);
>>
>> +/*
>> + * Check if firmware advertises firmware first mode. We need FF bit to be set
>> + * along with a set of MC banks which work in FF mode.
>> + */
>> +static int __init hest_parse_cmc(struct acpi_hest_header *hest_hdr, void *data)
>> +{
>> +	int i;
>> +	struct acpi_hest_ia_corrected *cmc;
>> +	struct acpi_hest_ia_error_bank *mc_bank;
>> +
>> +	if (hest_hdr->type != ACPI_HEST_TYPE_IA32_CORRECTED_CHECK)
>> +		return 0;
>> +
>> +	if (!((struct acpi_hest_generic *)hest_hdr)->enabled)
>> +		return 0;
>> +
>> +	cmc = (struct acpi_hest_ia_corrected *)hest_hdr;
>
> There is some crazy casting going on here: hest_hdr can be struct
> acpi_hest_generic and struct acpi_hest_ia_corrected.
>
> Since the ->enabled field overlaps in both structs and you want only it
> as a struct acpi_hest_generic, and want the cmc later, why don't you do
> this:
>
> 	struct acpi_hest_ia_corrected *cmc = (struct acpi_hest_ia_corrected *)hest_hdr;
>
> 	if (!cmc->enabled)
> 		...
>
> Then this below:
>
>> +	if (!(cmc->flags & ACPI_HEST_FIRMWARE_FIRST))
>> +		return 0;
>
> and so on... It should simplify the code a bit and drop the fun games
> with casting.

Nice catch. I will simplify this.

>
>> +
>> +	/*
>> +	 * We expect HEST to provide a list of MC banks that
>> +	 * report errors through firmware first mode.
>> +	 */
>> +	if (cmc->num_hardware_banks <= 0)
>
> ->num_hardware_banks is unsigned char, so "== 0"

Ok.

>
>> +		return 0;
>> +
>> +	pr_info("HEST: Enabling Firmware First mode for corrected errors\n");
>
> 	pr_info(HEST_PFX "Enabling..." (fullstop at the end of the sentence).

Ok. Changed.

>
> Btw, this hest.c could use the standard pr_fmt mechanism.
>
>> +
>> +	mc_bank = (struct acpi_hest_ia_error_bank *)(cmc + 1);
>> +	for (i = 0; i < cmc->num_hardware_banks; i++, mc_bank++)
>> +		mce_disable_bank(mc_bank->bank_number);
>> +
>> +	return 0;
>> +}
>> +
>>   struct ghes_arr {
>>   	struct platform_device **ghes_devs;
>>   	unsigned int count;
>> @@ -227,6 +264,8 @@ void __init acpi_hest_init(void)
>>   		goto err;
>>   	}
>>
>> +	apei_hest_parse(hest_parse_cmc, NULL);
>> +
>>   	if (!ghes_disable) {
>>   		rc = apei_hest_parse(hest_parse_ghes_count, &ghes_count);
>>   		if (rc)
>
> Thanks.
>

Thanks,
Naveen

Re: [PATCH] Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in

[Borislav Petkov]

On Mon, Jun 17, 2013 at 12:30:05PM +0530, Naveen N. Rao wrote:
> >Hmm, so if CMCI is not supported, you just disabled polling of this bank
> >and returned here. Not good.
> 
> This is on purpose. If the bank doesn't support CMCI and we were
> polling it earlier, we want to disable that. The firmware should be
> notifying us of error events in either case.

Not for corrected errors - for those we poll since the hardware doesn't
generate an exception for them.

Basically, the absence of CMCI support shouldn't change the old polling
behavior.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in

[Naveen N. Rao]

On 2013/06/17 09:06AM, Borislav Petkov wrote:
> On Mon, Jun 17, 2013 at 12:30:05PM +0530, Naveen N. Rao wrote:
> > >Hmm, so if CMCI is not supported, you just disabled polling of this bank
> > >and returned here. Not good.
> > 
> > This is on purpose. If the bank doesn't support CMCI and we were
> > polling it earlier, we want to disable that. The firmware should be
> > notifying us of error events in either case.
> 
> Not for corrected errors - for those we poll since the hardware doesn't
> generate an exception for them.

Yes, we used to poll since we do not get notified via MCE/CMCI. However,
with firmware first set in CMC structure, the firmware is now controlling
all corrected error reporting for these banks. So, we should not be looking
at these MCA banks at all.

> 
> Basically, the absence of CMCI support shouldn't change the old polling
> behavior.

Again, this changes with firmware first mode. The firmware should now
notify us of all corrected errors through GHES.

Thanks,
Naveen

> 
> Thanks.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Sent from a fat crate under my desk. Formatting is fine.
> --
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in

[Borislav Petkov]

On Mon, Jun 17, 2013 at 01:41:03PM +0530, Naveen N. Rao wrote:
> Yes, we used to poll since we do not get notified via MCE/CMCI.
> However, with firmware first set in CMC structure, the firmware is
> now controlling all corrected error reporting for these banks. So, we
> should not be looking at these MCA banks at all.

Ok,

then the bank polling disabling part should be generic code (i.e., not
in mce_intel) since it is part of the APEI crap. The CMCI disabling
should be Intel-specific only as AMD has a similar mechanism which we'll
need to disable at some point too.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in

[Naveen N. Rao]

On 06/17/2013 01:51 PM, Borislav Petkov wrote:
> On Mon, Jun 17, 2013 at 01:41:03PM +0530, Naveen N. Rao wrote:
>> Yes, we used to poll since we do not get notified via MCE/CMCI.
>> However, with firmware first set in CMC structure, the firmware is
>> now controlling all corrected error reporting for these banks. So, we
>> should not be looking at these MCA banks at all.
>
> Ok,
>
> then the bank polling disabling part should be generic code (i.e., not
> in mce_intel) since it is part of the APEI crap. The CMCI disabling
> should be Intel-specific only as AMD has a similar mechanism which we'll
> need to disable at some point too.

Ok, I'll move mce_disable_bank() to mce.c and re-structure the patch 
around that.

Thanks,
Naveen

Re: [PATCH] Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in

[Naveen N. Rao]

The Corrected Machine Check structure (CMC) in HEST has a flag which can be
set by the firmware to indicate to the OS that it prefers to process the
corrected error events first. In this scenario, the OS is expected to not
monitor for corrected errors (through CMCI/polling). Instead, the firmware
notifies the OS on corrected error events through GHES.

Linux already has support for GHES. This patch adds support for parsing CMC
structure and to disable CMCI/polling if the firmware first flag is set.

Further, the list of machine check bank structures at the end of CMC is used
to determine which MCA banks function in FF mode, so that we continue to
monitor error events on the other banks.


- Naveen

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/x86/include/asm/mce.h                |    3 ++
 arch/x86/kernel/cpu/mcheck/mce-internal.h |    3 ++
 arch/x86/kernel/cpu/mcheck/mce.c          |   23 +++++++++++++++++
 arch/x86/kernel/cpu/mcheck/mce_intel.c    |   40 +++++++++++++++++++++++------
 drivers/acpi/apei/hest.c                  |   36 ++++++++++++++++++++++++++
 5 files changed, 97 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index fa5f71e..380fff8 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -188,6 +188,9 @@ extern void register_mce_write_callback(ssize_t (*)(struct file *filp,
 				    const char __user *ubuf,
 				    size_t usize, loff_t *off));
 
+/* Disable CMCI/polling for MCA bank claimed by firmware */
+extern void mce_disable_ce_bank(int bank);
+
 /*
  * Exception handler
  */
diff --git a/arch/x86/kernel/cpu/mcheck/mce-internal.h b/arch/x86/kernel/cpu/mcheck/mce-internal.h
index 5b7d4fa..193edc1 100644
--- a/arch/x86/kernel/cpu/mcheck/mce-internal.h
+++ b/arch/x86/kernel/cpu/mcheck/mce-internal.h
@@ -25,6 +25,9 @@ int mce_severity(struct mce *a, int tolerant, char **msg);
 struct dentry *mce_get_debugfs_dir(void);
 
 extern struct mce_bank *mce_banks;
+extern mce_banks_t mce_banks_ce_disabled;
+
+void cmci_disable_bank(int bank);
 
 #ifdef CONFIG_X86_MCE_INTEL
 unsigned long mce_intel_adjust_timer(unsigned long interval);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 9239504..4bd9973 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -94,6 +94,9 @@ DEFINE_PER_CPU(mce_banks_t, mce_poll_banks) = {
 	[0 ... BITS_TO_LONGS(MAX_NR_BANKS)-1] = ~0UL
 };
 
+/* MCA banks controlled through firmware first for corrected errors */
+mce_banks_t mce_banks_ce_disabled;
+
 static DEFINE_PER_CPU(struct work_struct, mce_work);
 
 static void (*quirk_no_way_out)(int bank, struct mce *m, struct pt_regs *regs);
@@ -1932,6 +1935,26 @@ static struct miscdevice mce_chrdev_device = {
 	&mce_chrdev_ops,
 };
 
+static void __mce_disable_ce_bank(void *arg)
+{
+	int bank = *((int *)arg);
+
+	/* Ensure we don't poll this bank */
+	__clear_bit(bank, __get_cpu_var(mce_poll_banks));
+	/* Disable CMCI */
+	cmci_disable_bank(bank);
+}
+
+void mce_disable_ce_bank(int bank)
+{
+	if (bank >= mca_cfg.banks) {
+		pr_info("mce_disable_bank: Invalid MCA bank %d ignored.\n", bank);
+		return;
+	}
+	set_bit(bank, mce_banks_ce_disabled);
+	on_each_cpu(__mce_disable_ce_bank, &bank, 1);
+}
+
 /*
  * mce=off Disables machine check
  * mce=no_cmci Disables CMCI
diff --git a/arch/x86/kernel/cpu/mcheck/mce_intel.c b/arch/x86/kernel/cpu/mcheck/mce_intel.c
index ae1697c..78256c0 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_intel.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_intel.c
@@ -191,6 +191,10 @@ static void cmci_discover(int banks)
 		if (test_bit(i, owned))
 			continue;
 
+		/* Skip banks in firmware first mode */
+		if (test_bit(i, mce_banks_ce_disabled))
+			continue;
+
 		rdmsrl(MSR_IA32_MCx_CTL2(i), val);
 
 		/* Already owned by someone else? */
@@ -259,6 +263,20 @@ void cmci_recheck(void)
 	local_irq_restore(flags);
 }
 
+/* Caller must hold the lock on cmci_discover_lock */
+static void __cmci_disable_bank(int bank)
+{
+	u64 val;
+
+	if (!test_bit(bank, __get_cpu_var(mce_banks_owned)))
+		return;
+	/* Disable CMCI */
+	rdmsrl(MSR_IA32_MCx_CTL2(bank), val);
+	val &= ~MCI_CTL2_CMCI_EN;
+	wrmsrl(MSR_IA32_MCx_CTL2(bank), val);
+	__clear_bit(bank, __get_cpu_var(mce_banks_owned));
+}
+
 /*
  * Disable CMCI on this CPU for all banks it owns when it goes down.
  * This allows other CPUs to claim the banks on rediscovery.
@@ -268,19 +286,12 @@ void cmci_clear(void)
 	unsigned long flags;
 	int i;
 	int banks;
-	u64 val;
 
 	if (!cmci_supported(&banks))
 		return;
 	raw_spin_lock_irqsave(&cmci_discover_lock, flags);
 	for (i = 0; i < banks; i++) {
-		if (!test_bit(i, __get_cpu_var(mce_banks_owned)))
-			continue;
-		/* Disable CMCI */
-		rdmsrl(MSR_IA32_MCx_CTL2(i), val);
-		val &= ~MCI_CTL2_CMCI_EN;
-		wrmsrl(MSR_IA32_MCx_CTL2(i), val);
-		__clear_bit(i, __get_cpu_var(mce_banks_owned));
+		__cmci_disable_bank(i);
 	}
 	raw_spin_unlock_irqrestore(&cmci_discover_lock, flags);
 }
@@ -315,6 +326,19 @@ void cmci_reenable(void)
 		cmci_discover(banks);
 }
 
+void cmci_disable_bank(int bank)
+{
+	int banks;
+	unsigned long flags;
+
+	if (!cmci_supported(&banks))
+		return;
+
+	raw_spin_lock_irqsave(&cmci_discover_lock, flags);
+	__cmci_disable_bank(bank);
+	raw_spin_unlock_irqrestore(&cmci_discover_lock, flags);
+}
+
 static void intel_init_cmci(void)
 {
 	int banks;
diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index f5ef5d5..d8c69ba 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -36,6 +36,7 @@
 #include <linux/io.h>
 #include <linux/platform_device.h>
 #include <acpi/apei.h>
+#include <asm/mce.h>
 
 #include "apei-internal.h"
 
@@ -121,6 +122,39 @@ int apei_hest_parse(apei_hest_func_t func, void *data)
 }
 EXPORT_SYMBOL_GPL(apei_hest_parse);
 
+/*
+ * Check if firmware advertises firmware first mode. We need FF bit to be set
+ * along with a set of MC banks which work in FF mode.
+ */
+static int __init hest_parse_cmc(struct acpi_hest_header *hest_hdr, void *data)
+{
+	int i;
+	struct acpi_hest_ia_corrected *cmc;
+	struct acpi_hest_ia_error_bank *mc_bank;
+
+	if (hest_hdr->type != ACPI_HEST_TYPE_IA32_CORRECTED_CHECK)
+		return 0;
+
+	cmc = (struct acpi_hest_ia_corrected *)hest_hdr;
+	if (!cmc->enabled || !(cmc->flags & ACPI_HEST_FIRMWARE_FIRST))
+		return 0;
+
+	/*
+	 * We expect HEST to provide a list of MC banks that
+	 * report errors through firmware first mode.
+	 */
+	if (cmc->num_hardware_banks == 0)
+		return 0;
+
+	pr_info(HEST_PFX "Enabling Firmware First mode for corrected errors.\n");
+
+	mc_bank = (struct acpi_hest_ia_error_bank *)(cmc + 1);
+	for (i = 0; i < cmc->num_hardware_banks; i++, mc_bank++)
+		mce_disable_ce_bank(mc_bank->bank_number);
+
+	return 0;
+}
+
 struct ghes_arr {
 	struct platform_device **ghes_devs;
 	unsigned int count;
@@ -227,6 +261,8 @@ void __init acpi_hest_init(void)
 		goto err;
 	}
 
+	apei_hest_parse(hest_parse_cmc, NULL);
+
 	if (!ghes_disable) {
 		rc = apei_hest_parse(hest_parse_ghes_count, &ghes_count);
 		if (rc)

Re: [PATCH] Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in

[Tony Luck]

On Mon, Jun 17, 2013 at 11:43 PM, Naveen N. Rao
<naveen.n.rao@linux.vnet.ibm.com> wrote:
> +       if (bank >= mca_cfg.banks) {
> +               pr_info("mce_disable_bank: Invalid MCA bank %d ignored.\n", bank);

Let's have a FW_BUG in that message to point a finger at the source of
the problem.


+       apei_hest_parse(hest_parse_cmc, NULL);

I think we want a boot command line option to opt out of this. "nohestcmc"??

-Tony

Re: [PATCH] Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in

[Naveen N. Rao]

On 06/19/2013 03:59 AM, Tony Luck wrote:
> On Mon, Jun 17, 2013 at 11:43 PM, Naveen N. Rao
> <naveen.n.rao@linux.vnet.ibm.com> wrote:
>> +       if (bank >= mca_cfg.banks) {
>> +               pr_info("mce_disable_bank: Invalid MCA bank %d ignored.\n", bank);
>
> Let's have a FW_BUG in that message to point a finger at the source of
> the problem.

Ok.

>
>
> +       apei_hest_parse(hest_parse_cmc, NULL);
>
> I think we want a boot command line option to opt out of this. "nohestcmc"??

Ok, I will add a boot option for this.


Thanks,
Naveen

Re: [PATCH] Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in

[Borislav Petkov]

On Fri, Jun 14, 2013 at 11:47:21PM +0530, Naveen N. Rao wrote:
> +static int __init hest_parse_cmc(struct acpi_hest_header *hest_hdr, void *data)
> +{
> +	int i;
> +	struct acpi_hest_ia_corrected *cmc;
> +	struct acpi_hest_ia_error_bank *mc_bank;
> +
> +	if (hest_hdr->type != ACPI_HEST_TYPE_IA32_CORRECTED_CHECK)
> +		return 0;
> +
> +	if (!((struct acpi_hest_generic *)hest_hdr)->enabled)
> +		return 0;
> +
> +	cmc = (struct acpi_hest_ia_corrected *)hest_hdr;
> +	if (!(cmc->flags & ACPI_HEST_FIRMWARE_FIRST))
> +		return 0;
> +
> +	/*
> +	 * We expect HEST to provide a list of MC banks that
> +	 * report errors through firmware first mode.
> +	 */
> +	if (cmc->num_hardware_banks <= 0)
> +		return 0;
> +
> +	pr_info("HEST: Enabling Firmware First mode for corrected errors\n");
> +
> +	mc_bank = (struct acpi_hest_ia_error_bank *)(cmc + 1);
> +	for (i = 0; i < cmc->num_hardware_banks; i++, mc_bank++)
> +		mce_disable_bank(mc_bank->bank_number);

One more thing: we never trust the BIOS so mce_disable_bank() should
sanity-check this mc_bank->bank_number against the number of the
actually available banks on the system before disabling anything.

Thanks.

Re: [PATCH] Re: [Patch] MCE, APEI: Don't enable CMCI when Firmware First mode is set in

[Naveen N. Rao]

On 06/16/2013 05:50 PM, Borislav Petkov wrote:
> On Fri, Jun 14, 2013 at 11:47:21PM +0530, Naveen N. Rao wrote:
>> +static int __init hest_parse_cmc(struct acpi_hest_header *hest_hdr, void *data)
>> +{
>> +	int i;
>> +	struct acpi_hest_ia_corrected *cmc;
>> +	struct acpi_hest_ia_error_bank *mc_bank;
>> +
>> +	if (hest_hdr->type != ACPI_HEST_TYPE_IA32_CORRECTED_CHECK)
>> +		return 0;
>> +
>> +	if (!((struct acpi_hest_generic *)hest_hdr)->enabled)
>> +		return 0;
>> +
>> +	cmc = (struct acpi_hest_ia_corrected *)hest_hdr;
>> +	if (!(cmc->flags & ACPI_HEST_FIRMWARE_FIRST))
>> +		return 0;
>> +
>> +	/*
>> +	 * We expect HEST to provide a list of MC banks that
>> +	 * report errors through firmware first mode.
>> +	 */
>> +	if (cmc->num_hardware_banks <= 0)
>> +		return 0;
>> +
>> +	pr_info("HEST: Enabling Firmware First mode for corrected errors\n");
>> +
>> +	mc_bank = (struct acpi_hest_ia_error_bank *)(cmc + 1);
>> +	for (i = 0; i < cmc->num_hardware_banks; i++, mc_bank++)
>> +		mce_disable_bank(mc_bank->bank_number);
>
> One more thing: we never trust the BIOS so mce_disable_bank() should
> sanity-check this mc_bank->bank_number against the number of the
> actually available banks on the system before disabling anything.

Agreed. Will add the check.

>
> Thanks.
>

Thanks,
Naveen