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

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