mce: acpi/apei: Honour Firmware First for MCA banks listed in APEI HEST CMC

PATCH Superseded Naveen N. Rao 4 participants 42 messages Jun 19, 2013 – Jul 1, 2013

Subsystems (7)

Memory management Documentation x86 ACPI +3 more subsystems

lore

Share & tools

lore

Naveen N. Rao

author

2013-06-19 17:57 UTC #0 lore raw

PATCH v2 1/2 mce: acpi/apei: Honour Firmware First for MCA banks listed in APEI HEST CMC

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

--
Changes:
- Incorporated comments from Boris and Tony from the previous thread at
http://thread.gmane.org/gmane.linux.acpi.devel/60802
- Added patch to disable firmware first mode through a boot option.

Signed-off-by Naveen N. Rao <[email protected]>

---
 arch/x86/include/asm/mce.h                |    3 ++
 arch/x86/kernel/cpu/mcheck/mce-internal.h |    3 ++
 arch/x86/kernel/cpu/mcheck/mce.c          |   25 ++++++++++++++++++
 arch/x86/kernel/cpu/mcheck/mce_intel.c    |   40 +++++++++++++++++++++++------
 drivers/acpi/apei/hest.c                  |   36 ++++++++++++++++++++++++++
 5 files changed, 99 insertions(+), 8 deletions(-)

arch/x86/include/asm/mce.h +3 -0

index fa5f71e..380fff8 100644
@@ -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
  */

arch/x86/kernel/cpu/mcheck/mce-internal.h +3 -0

index 5b7d4fa..193edc1 100644
@@ -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);

arch/x86/kernel/cpu/mcheck/mce.c +25 -0

index 9239504..9512034 100644
@@ -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,28 @@ 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_warning(FW_BUG 
+			"Ignoring request to disable invalid MCA bank %d.\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

arch/x86/kernel/cpu/mcheck/mce_intel.c +32 -8

index ae1697c..78256c0 100644
@@ -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;

drivers/acpi/apei/hest.c +36 -0

index f5ef5d5..d8c69ba 100644
@@ -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)

Naveen N. Rao

author

2013-06-19 17:57 UTC #1 lore raw

PATCH v2 2/2 mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

Add a boot option to disable firmware first mode for corrected errors.

Signed-off-by Naveen N. Rao <[email protected]>

---
 Documentation/x86/x86_64/boot-options.txt |    5 +++++
 arch/x86/include/asm/acpi.h               |    2 ++
 arch/x86/kernel/acpi/boot.c               |    5 +++++
 drivers/acpi/apei/hest.c                  |    3 ++-
 4 files changed, 14 insertions(+), 1 deletion(-)

Documentation/x86/x86_64/boot-options.txt +5 -0

index e9e8ddb..1228b22 100644
@@ -176,6 +176,11 @@ ACPI
 
   acpi=noirq	Don't route interrupts
 
+  acpi=nocmcff	Disable firmware first mode for corrected errors. This
+		disables parsing the HEST CMC error source to check if
+		firmware has set the FF flag. This may result in
+		duplicate corrected error reports.
+
 PCI
 
   pci=off		Don't use PCI

arch/x86/include/asm/acpi.h +2 -0

index b31bf97..42db2b8 100644
@@ -86,6 +86,7 @@ extern int acpi_pci_disabled;
 extern int acpi_skip_timer_override;
 extern int acpi_use_timer_override;
 extern int acpi_fix_pin2_polarity;
+extern int acpi_disable_cmcff;
 
 extern u8 acpi_sci_flags;
 extern int acpi_sci_override_gsi;

@@ -168,6 +169,7 @@ static inline void arch_acpi_set_pdc_bits(u32 *buf)
 
 #define acpi_lapic 0
 #define acpi_ioapic 0
+#define acpi_disable_cmcff 0
 static inline void acpi_noirq_set(void) { }
 static inline void acpi_disable_pci(void) { }
 static inline void disable_acpi(void) { }

arch/x86/kernel/acpi/boot.c +5 -0

index 230c8ea..d1998d5 100644
@@ -66,6 +66,7 @@ EXPORT_SYMBOL(acpi_pci_disabled);
 int acpi_lapic;
 int acpi_ioapic;
 int acpi_strict;
+int acpi_disable_cmcff;
 
 u8 acpi_sci_flags __initdata;
 int acpi_sci_override_gsi __initdata;

@@ -1619,6 +1620,10 @@ static int __init parse_acpi(char *arg)
 	/* "acpi=copy_dsdt" copys DSDT */
 	else if (strcmp(arg, "copy_dsdt") == 0) {
 		acpi_gbl_copy_dsdt_locally = 1;
+	}
+	/* "acpi=nocmcff" disables FF mode for corrected errors */
+	else if (strcmp(arg, "nocmcff") == 0) {
+		acpi_disable_cmcff = 1;
 	} else {
 		/* Core will printk when we return error. */
 		return -EINVAL;

drivers/acpi/apei/hest.c +2 -1

index d8c69ba..f8b1115 100644
@@ -261,7 +261,8 @@ void __init acpi_hest_init(void)
 		goto err;
 	}
 
-	apei_hest_parse(hest_parse_cmc, NULL);
+	if (!acpi_disable_cmcff)
+		apei_hest_parse(hest_parse_cmc, NULL);
 
 	if (!ghes_disable) {
 		rc = apei_hest_parse(hest_parse_ghes_count, &ghes_count);

Borislav Petkov

maintainer

2013-06-19 18:04 UTC #2 lore raw

Re: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

Quoted 15 lines · Naveen N. Rao

Add a boot option to disable firmware first mode for corrected errors.
Signed-off-by Naveen N. Rao <[email protected]>
---
 Documentation/x86/x86_64/boot-options.txt |    5 +++++
 arch/x86/include/asm/acpi.h               |    2 ++
 arch/x86/kernel/acpi/boot.c               |    5 +++++
 drivers/acpi/apei/hest.c                  |    3 ++-
 4 files changed, 14 insertions(+), 1 deletion(-)
Documentation/x86/x86_64/boot-options.txt +0 -0
index e9e8ddb..1228b22 100644

Documentation/x86/x86_64/boot-options.txt ACPI · line 182

Quoted 7 lines

Documentation/x86/x86_64/boot-options.txt +4 -0

@@ -176,6 +176,11 @@ ACPI
 
   acpi=noirq	Don't route interrupts
 
+  acpi=nocmcff	Disable firmware first mode for corrected errors. This
+		disables parsing the HEST CMC error source to check if
+		firmware has set the FF flag. This may result in
+		duplicate corrected error reports.

Interesting, why? Why would we even need such an option? My impression is, if ACPI tells us FF, MCE code doesn't poll those banks anymore. So where do the duplicated reports come from?

Thanks.

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Naveen N. Rao

author

2013-06-19 18:17 UTC #3 lore raw

Re: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

Quoted 19 lines · Borislav Petkov

Quoted 18 lines · Naveen N. Rao
Add a boot option to disable firmware first mode for corrected errors.
Signed-off-by Naveen N. Rao <[email protected]>
---
  Documentation/x86/x86_64/boot-options.txt |    5 +++++
  arch/x86/include/asm/acpi.h               |    2 ++
  arch/x86/kernel/acpi/boot.c               |    5 +++++
  drivers/acpi/apei/hest.c                  |    3 ++-
  4 files changed, 14 insertions(+), 1 deletion(-)
Documentation/x86/x86_64/boot-options.txt +0 -0
index e9e8ddb..1228b22 100644
@@ -176,6 +176,11 @@ ACPI
 
    acpi=noirq	Don't route interrupts
 

Quoted 8 lines

Documentation/x86/x86_64/boot-options.txt ACPI · line 182
Quoted 4 lines
Documentation/x86/x86_64/boot-options.txt +4 -0
+  acpi=nocmcff	Disable firmware first mode for corrected errors. This
+		disables parsing the HEST CMC error source to check if
+		firmware has set the FF flag. This may result in
+		duplicate corrected error reports.
Interesting, why? Why would we even need such an option? My impression is, if ACPI tells us FF, MCE code doesn't poll those banks anymore. So where do the duplicated reports come from?

This option is a way to revert to the existing behavior where we continue to enable CMCI/poll. If we enable this option, then we will receive error events from CMCI/polling as well as from the firmware through GHES.

Thanks,
Naveen

Luck, Tony

reviewer

2013-06-19 18:19 UTC #4 lore raw

RE: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

In reply to Borislav Petkov

Quoted 3 lines

Interesting, why? Why would we even need such an option? My impression is, if ACPI tells us FF, MCE code doesn't poll those banks anymore. So where do the duplicated reports come from?

The option is only disabling the Linux side of firmware first ... the BIOS will still be doing it and generating records to feed to the OS using APEI.

So Linux may see the error in a bank and report it, and BIOS may report the same error. Though I'd expect that to be rare as whoever saw it first would most likely clear the bank before the other could see it.

I asked for the option because I'm nervous about just skipping some banks on the say-so of the BIOS ... what if the BIOS did something wrong. This option gives us a way to return to the way things were before this patch.

These parts are now looking good ... but we still need to tackle what Linux does when it does get the CPER record. I suspect we need to preserve the existing "fake an mcelog entry with just the address" on old platforms, but need to do something smarter on new ones.

-Tony

Borislav Petkov

maintainer

2013-06-19 18:36 UTC #5 lore raw

Re: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

Quoted 7 lines · Tony

Quoted 3 lines
Interesting, why? Why would we even need such an option? My impression is, if ACPI tells us FF, MCE code doesn't poll those banks anymore. So where do the duplicated reports come from?
The option is only disabling the Linux side of firmware first ... the BIOS will still be doing it and generating records to feed to the OS using APEI.

Quoted 7 lines

So Linux may see the error in a bank and report it, and BIOS may report the same error. Though I'd expect that to be rare as whoever saw it first would most likely clear the bank before the other could see it.
I asked for the option because I'm nervous about just skipping some banks on the say-so of the BIOS ... what if the BIOS did something wrong. This option gives us a way to return to the way things were before this patch.

Yeah, the code I saw only disables the banks in the HEST:

	mce_disable_ce_bank(mc_bank->bank_number)

and leaving the rest in poll mode. But I agree, we need this as a fallback if BIOS is doing other crack smoking exercises and thus we want to ignore FF completely.

Quoted 4 lines

These parts are now looking good ... but we still need to tackle what Linux does when it does get the CPER record. I suspect we need to preserve the existing "fake an mcelog entry with just the address" on old platforms, but need to do something smarter on new ones.

Why, fill out struct mce and do mce_log(mce) does not suffice?

I'll take a look at the rest of the stuff tomorrow, on a clear head.

Thanks.

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Luck, Tony

reviewer

2013-06-19 19:05 UTC #6 lore raw

RE: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

Quoted 1 line

Why, fill out struct mce and do mce_log(mce) does not suffice?

There is (or should be) a lot more interesting stuff in the CPER than just the address. Stuff that we don't have fields for in the existing mcelog structure. We also need to treat filtered records from modern APEI implementations a bit differently from the old stuff. The original user of this code was Westmere-EX, which used it as a workaround for a missing address in MCi_ADDR for corrected errors. So in that scenario we had every error being reported and mcelog(8) deamon doing the threshold analysis to decide when to take action.

In this new modern world - Naveen wants to have the BIOS decide the threshold, so we'd like Linux to take some action as soon as it sees just one CPER.

-Tony

Borislav Petkov

maintainer

2013-06-19 20:14 UTC #7 lore raw

Re: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

Quoted 3 lines · Tony

Quoted 1 line
Why, fill out struct mce and do mce_log(mce) does not suffice?
There is (or should be)

Ha!

Quoted 4 lines

a lot more interesting stuff in the CPER than just the address. Stuff that we don't have fields for in the existing mcelog structure. We also need to treat filtered records from modern APEI implementations a bit differently from the old stuff.

Great, the CPER record is described in the UEFI spec. Those BIOS people are all like a mafia.

Ok, seriously: so the situation should still be fine, FF reported errors get the CPER format while the rest, the "old" MCE format.

cper.c is doing printk so I'm guessing it would need to get its own tracepoint and carry that to userspace.

Concerning the RAS daemon, Robert and I are making good progress so once we have the persistent events in perf, we can read that tracepoint in userspace and do whatever we want with the error info.

Quoted 8 lines

The original user of this code was Westmere-EX, which used it as a workaround for a missing address in MCi_ADDR for corrected errors. So in that scenario we had every error being reported and mcelog(8) deamon doing the threshold analysis to decide when to take action.
In this new modern world - Naveen wants to have the BIOS decide the threshold, so we'd like Linux to take some action as soon as it sees just one CPER.

Why would Linux have to intervene if it is doing FF - wasn't the deal behind Firmware First for the firmware to get the error first and handle accordingly?

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Luck, Tony

reviewer

2013-06-19 20:33 UTC #8 lore raw

RE: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

Quoted 3 lines

Quoted 1 line
There is (or should be)
Ha!

Oh ye of little faith - I'm sure the BIOS will get this right this time :-)

Quoted 5 lines

Ok, seriously: so the situation should still be fine, FF reported errors get the CPER format while the rest, the "old" MCE format.
cper.c is doing printk so I'm guessing it would need to get its own tracepoint and carry that to userspace.

Yes - a tracepoint is the right answer here for all the new stuff.

Quoted 3 lines

Concerning the RAS daemon, Robert and I are making good progress so once we have the persistent events in perf, we can read that tracepoint in userspace and do whatever we want with the error info.

Mauro has a rasdaemon in progress
git://git.fedorahosted.org/rasdaemon.git
just picks up perf/events and logs to a sqlite database.

Quoted 7 lines

Quoted 3 lines
In this new modern world - Naveen wants to have the BIOS decide the threshold, so we'd like Linux to take some action as soon as it sees just one CPER.
Why would Linux have to intervene if it is doing FF - wasn't the deal behind Firmware First for the firmware to get the error first and handle accordingly?

Because Linux can do runtime things that the BIOS can't - like offline a 4K page. Idea here is that BIOS does whatever the OEM thinks is the right level of threshholding - not bothering the OS with petty details of random corrected erorrs that mean nothing. But if there is some repeated error (like a stuck bit) then the BIOS can provide a CPER to the OS telling it that it would be a good idea to stop using that page.

And this is where the semantics of a CPER change between the original WSM-EX implementation ... where Linux expects to see all the errors and do its own thresholding only taking a page offline if it sees a lot of CPER refer to the same page; and now - where the BIOS does the counting and tells Linux just once to take the page offline.

-Tony

Borislav Petkov

maintainer

2013-06-19 21:07 UTC #9 lore raw

Re: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

Quoted 15 lines · Tony

Quoted 3 lines
Quoted 1 line
There is (or should be)
Ha!
Oh ye of little faith - I'm sure the BIOS will get this right this time :-)
Quoted 5 lines
Ok, seriously: so the situation should still be fine, FF reported errors get the CPER format while the rest, the "old" MCE format.
cper.c is doing printk so I'm guessing it would need to get its own tracepoint and carry that to userspace.
Yes - a tracepoint is the right answer here for all the new stuff.

Quoted 7 lines

Quoted 3 lines
Concerning the RAS daemon, Robert and I are making good progress so once we have the persistent events in perf, we can read that tracepoint in userspace and do whatever we want with the error info.
Mauro has a rasdaemon in progress
git://git.fedorahosted.org/rasdaemon.git
just picks up perf/events and logs to a sqlite database.

Actually it uses ftrace's facilities but it is a tracepoint in the end.

And I asked him nicely not to call it rasdaemon because I already have a RAS daemon but hey, whatever. The more confusion, the better.

Quoted 7 lines

Because Linux can do runtime things that the BIOS can't - like offline a 4K page. Idea here is that BIOS does whatever the OEM thinks is the right level of threshholding - not bothering the OS with petty details of random corrected erorrs that mean nothing. But if there is some repeated error (like a stuck bit) then the BIOS can provide a CPER to the OS telling it that it would be a good idea to stop using that page.

Ok, where is that semantics? What in a CPER record does say "this error should tell you that you need to offline the containing page and I'm telling you this exactly only once"? Error Severity 0, i.e. Recoverable?

Quoted 5 lines

And this is where the semantics of a CPER change between the original WSM-EX implementation ... where Linux expects to see all the errors and do its own thresholding only taking a page offline if it sees a lot of CPER refer to the same page; and now - where the BIOS does the counting and tells Linux just once to take the page offline.

Ok, we're talking about the S in RAS now. Do we have error recovery strategies specified anywhere? Are they per-platform or generic? Is this CPER strategy above, for example, only valid for some platforms or for all APEI-using hardware?

Questions over questions...

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Luck, Tony

reviewer

2013-06-19 21:28 UTC #10 lore raw

RE: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

Quoted 3 lines

Ok, where is that semantics? What in a CPER record does say "this error should tell you that you need to offline the containing page and I'm telling you this exactly only once"? Error Severity 0, i.e. Recoverable?

Naveen - this one is for you (or for your BIOS team). Can you get us a sample CPER that you plan to provide when the BIOS decides that its threshold has
been exceeded? How will it be different from what old WSM-EX platforms
were sending to us? Hopefully the answer is encoded in the CPER record
and not in some code we have to put in Linux to say "if (IBMplatform) do_thing_1(); else ... "

Quoted 4 lines

Ok, we're talking about the S in RAS now. Do we have error recovery strategies specified anywhere? Are they per-platform or generic? Is this CPER strategy above, for example, only valid for some platforms or for all APEI-using hardware?

mcelog(8) daemon has been doing this for years ... but it used the "predictive failure analysis" buzzwords that were popular way back then (today the marketing people seem to prefer "self healing" ). Whatever the name, the concept is the same ... take some set of corrected event reports and infer from them that something worse may happen soon, and use that information to try to avoid the (possibly) impending crash.

Quoted 1 line

Questions over questions...

Questions are good - they help fill out gaps

-Tony

Borislav Petkov

maintainer

2013-06-19 21:41 UTC #11 lore raw

Re: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

Quoted 9 lines · Tony

Quoted 3 lines
Ok, where is that semantics? What in a CPER record does say "this error should tell you that you need to offline the containing page and I'm telling you this exactly only once"? Error Severity 0, i.e. Recoverable?
Naveen - this one is for you (or for your BIOS team). Can you get us a sample CPER that you plan to provide when the BIOS decides that its threshold has
been exceeded? How will it be different from what old WSM-EX platforms
were sending to us? Hopefully the answer is encoded in the CPER record
and not in some code we have to put in Linux to say "if (IBMplatform) do_thing_1(); else ... "

If we're going to be vendor-specific (which I'm pretty sure we will), we'd gonna have to export knobs to userspace which each vendor can tweak for themselves. Otherwise we'd get the DMI quirks ugliness all over again and we better nip it in the bud, while we can.

Quoted 11 lines

Quoted 4 lines
Ok, we're talking about the S in RAS now. Do we have error recovery strategies specified anywhere? Are they per-platform or generic? Is this CPER strategy above, for example, only valid for some platforms or for all APEI-using hardware?
mcelog(8) daemon has been doing this for years ... but it used the "predictive failure analysis" buzzwords that were popular way back then (today the marketing people seem to prefer "self healing" ). Whatever the name, the concept is the same ... take some set of corrected event reports and infer from them that something worse may happen soon, and use that information to try to avoid the (possibly) impending crash.

Ok, so some sort of userspace is enforcing policy based on collected data/heuristics.

The above question about what to do *without* going to userspace and back is maybe more interesting and we'd need a clean design there... we'll see.

Quoted 3 lines

Quoted 1 line
Questions over questions...
Questions are good - they help fill out gaps

I know - that's why I'm trying to poke holes early...

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Luck, Tony

reviewer

2013-06-19 22:09 UTC #12 lore raw

RE: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

Quoted 3 lines

The above question about what to do *without* going to userspace and back is maybe more interesting and we'd need a clean design there... we'll see.

Yes - this case (where the BIOS did all the threshold math and made the decision) should be one where Linux kernel could just implement the action directly. Perhaps controlled by a knob to say whether we really trust the BIOS that much.

But we will also have cases where a smart user agent can correlate data from multiple sources to identify the real root cause (e.g. some temperature anomalies around the same time as some memory errors that occur at 10am on the third Tuesday each month -> cause is air conditioner maintenance guy that shuts down the a/c for 10 minutes to change the filter).

I'll leave writing an agent that smart as an exercise for the concerned data center manager :-)

-Tony

Borislav Petkov

maintainer

2013-06-20 05:35 UTC #13 lore raw

Re: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

Quoted 8 lines · Tony

Quoted 3 lines
The above question about what to do *without* going to userspace and back is maybe more interesting and we'd need a clean design there... we'll see.
Yes - this case (where the BIOS did all the threshold math and made the decision) should be one where Linux kernel could just implement the action directly. Perhaps controlled by a knob to say whether we really trust the BIOS that much.

Quoted 5 lines

But we will also have cases where a smart user agent can correlate data from multiple sources to identify the real root cause (e.g. some temperature anomalies around the same time as some memory errors that occur at 10am on the third Tuesday each month -> cause is air conditioner maintenance guy that shuts down the a/c for 10 minutes to change the filter).

Surely we cannot put that in the kernel. For that we'd need userspace to decide and only turn knobs in the kernel.

Quoted 2 lines

I'll leave writing an agent that smart as an exercise for the concerned data center manager :-)

Me too, as long as it stays in userspace and it only turns knobs/interfaces in the kernel.

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Naveen N. Rao

author

2013-06-20 21:21 UTC #14 lore raw

Re: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

In reply to Luck, Tony

Quoted 9 lines · Tony

Quoted 3 lines
Ok, where is that semantics? What in a CPER record does say "this error should tell you that you need to offline the containing page and I'm telling you this exactly only once"? Error Severity 0, i.e. Recoverable?
Naveen - this one is for you (or for your BIOS team). Can you get us a sample CPER that you plan to provide when the BIOS decides that its threshold has
been exceeded? How will it be different from what old WSM-EX platforms
were sending to us? Hopefully the answer is encoded in the CPER record
and not in some code we have to put in Linux to say "if (IBMplatform) do_thing_1(); else ... "

Looking at the specs, there might be a few ways we can do this:
- One, Error threshold value of 1 in the Hardware Error Notification structure of CMC. This field is described as the number of error events before OS considers this as an error event. With a threshold value of 1, we are essentially asking the OS not to threshold further.
- Two, the Generic Error Data Entry (aka UEFI Section Descriptor) has a flag which indicates 'Error Threshold Exceeded'. From the UEFI spec, it looks like we could consider this as an indication to offline the page; though I am not sure if/how this relates to the threshold value above.

Thoughts?

Thanks,
Naveen

Luck, Tony

reviewer

2013-06-20 22:11 UTC #15 lore raw

RE: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

Quoted 4 lines

- Two, the Generic Error Data Entry (aka UEFI Section Descriptor) has a flag which indicates 'Error Threshold Exceeded'. From the UEFI spec, it looks like we could consider this as an indication to offline the page; though I am not sure if/how this relates to the threshold value above.

This one sounds to make sense ... the flag description sounds exactly what we want - I won't feel embarrassed explaining to people why Linux takes action when it sees a record like this.

-Tony

Borislav Petkov

maintainer

2013-06-21 07:27 UTC #16 lore raw

Re: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

Quoted 8 lines · Tony

Quoted 4 lines
- Two, the Generic Error Data Entry (aka UEFI Section Descriptor) has a flag which indicates 'Error Threshold Exceeded'. From the UEFI spec, it looks like we could consider this as an indication to offline the page; though I am not sure if/how this relates to the threshold value above.
This one sounds to make sense ... the flag description sounds exactly what we want - I won't feel embarrassed explaining to people why Linux takes action when it sees a record like this.

Yep, and firmware can set that flag when it wants to, so decision when to offline a page is left to the platform.

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Naveen N. Rao

author

2013-06-21 16:44 UTC #17 lore raw

Re: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

Quoted 6 lines · Borislav Petkov

Quoted 5 lines · Tony
Quoted 4 lines
- Two, the Generic Error Data Entry (aka UEFI Section Descriptor) has a flag which indicates 'Error Threshold Exceeded'. From the UEFI spec, it looks like we could consider this as an indication to offline the page; though I am not sure if/how this relates to the threshold value above.

Quoted 7 lines

Quoted 3 lines
This one sounds to make sense ... the flag description sounds exactly what we want - I won't feel embarrassed explaining to people why Linux takes action when it sees a record like this.
Yep, and firmware can set that flag when it wants to, so decision when to offline a page is left to the platform.

Ok, I will work towards a patch which does this.

Thanks,
Naveen

Naveen N. Rao

author

2013-06-28 12:04 UTC #18 lore raw

Re: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

In reply to Borislav Petkov

Hi Tony, Boris,
Here is a patch which implements this technique. Kindly take a look and let me know what you think.

Thanks,
Naveen

--
If the firmware indicates in GHES error data entry that the error threshold has exceeded for a corrected error event, then we try to soft-offline the page.

Signed-off-by Naveen N. Rao <[email protected]>

---
 drivers/acpi/apei/ghes.c |    7 ++++++
 include/linux/mm.h       |    1 +
 mm/memory-failure.c      |   53 ++++++++++++++++++++++++++++++----------------
 3 files changed, 43 insertions(+), 18 deletions(-)

drivers/acpi/apei/ghes.c +7 -0

index fcd7d91..17137cf 100644
@@ -422,6 +422,13 @@ static void ghes_do_proc(struct ghes *ghes,
 				 CPER_SEC_PLATFORM_MEM)) {
 			struct cper_sec_mem_err *mem_err;
 			mem_err = (struct cper_sec_mem_err *)(gdata+1);
+			if (sec_sev == GHES_SEV_CORRECTED &&
+			    (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED) &&
+			    (mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS)) {
+				unsigned long pfn;
+				pfn = mem_err->physical_addr >> PAGE_SHIFT;
+				soft_memory_failure_queue(pfn, 0, 0);
+			}
 			ghes_edac_report_mem_error(ghes, sev, mem_err);
 
 #ifdef CONFIG_X86_MCE

include/linux/mm.h +1 -0

index e0c8528..f9907d2 100644
@@ -1787,6 +1787,7 @@ enum mf_flags {
 };
 extern int memory_failure(unsigned long pfn, int trapno, int flags);
 extern void memory_failure_queue(unsigned long pfn, int trapno, int flags);
+extern void soft_memory_failure_queue(unsigned long pfn, int trapno, int flags);
 extern int unpoison_memory(unsigned long pfn);
 extern int sysctl_memory_failure_early_kill;
 extern int sysctl_memory_failure_recovery;

mm/memory-failure.c +35 ~~-18~~

82 lines not on this page · raw message

Tony Luck

2013-06-28 17:31 UTC #19 lore raw

Re: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

drivers/acpi/apei/ghes.c static void ghes_do_proc(struct ghes *ghes, · line 429

Quoted 5 lines

drivers/acpi/apei/ghes.c +5 -0

+                       if (sec_sev == GHES_SEV_CORRECTED &&
+                           (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED) &&
+                           (mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS)) {
+                               unsigned long pfn;
+                               pfn = mem_err->physical_addr >> PAGE_SHIFT;

As Reagan said "Trust ... but verify" ... we should make sure BIOS gave us a good pfn

                                    if (pfn_valid(pfn))
                                         soft_memory_failure_queue(pfn, 0, 0);
                                    else
                                         printk( ...something about

BIOS giving us bad pfn = %lu\n", pfn);

Quoted 1 line

+ }

Naveen N. Rao

author

2013-07-01 15:07 UTC #20 lore raw

Re: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

Quoted 6 lines · Tony Luck

drivers/acpi/apei/ghes.c static void ghes_do_proc(struct ghes *ghes, · line 429

Quoted 5 lines

drivers/acpi/apei/ghes.c +5 -0

+                       if (sec_sev == GHES_SEV_CORRECTED &&
+                           (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED) &&
+                           (mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS)) {
+                               unsigned long pfn;
+                               pfn = mem_err->physical_addr >> PAGE_SHIFT;

Quoted 7 lines

As Reagan said "Trust ... but verify" ... we should make sure BIOS gave us a good pfn

                                     if (pfn_valid(pfn))
                                          soft_memory_failure_queue(pfn, 0, 0);
                                     else
                                          printk( ...something about

BIOS giving us bad pfn = %lu\n", pfn);

Ah, nice catch - I thought soft_offline_page() takes care of this, but it sure is good to point a finger at the firmware.

Thanks!
- Naveen

Quoted 2 lines

Quoted 1 line
+ }

Borislav Petkov

maintainer

2013-07-01 15:38 UTC #21 lore raw

Re: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

Quoted 15 lines · Naveen N. Rao

Quoted 6 lines · Tony Luck

drivers/acpi/apei/ghes.c static void ghes_do_proc(struct ghes *ghes, · line 429

Quoted 5 lines

drivers/acpi/apei/ghes.c +5 -0

+                       if (sec_sev == GHES_SEV_CORRECTED &&
+                           (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED) &&
+                           (mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS)) {
+                               unsigned long pfn;
+                               pfn = mem_err->physical_addr >> PAGE_SHIFT;

Quoted 7 lines

As Reagan said "Trust ... but verify" ... we should make sure BIOS gave us a good pfn

                                    if (pfn_valid(pfn))
                                         soft_memory_failure_queue(pfn, 0, 0);
                                    else
                                         printk( ...something about

BIOS giving us bad pfn = %lu\n", pfn);

Quoted 2 lines

Ah, nice catch - I thought soft_offline_page() takes care of this, but it sure is good to point a finger at the firmware.

While at it maybe make it pr_warning(FW_BUG or FW_WARN...

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Naveen N. Rao

author

2013-07-01 15:41 UTC #22 lore raw

Re: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

Quoted 16 lines · Borislav Petkov

Quoted 15 lines · Naveen N. Rao

Quoted 13 lines · Tony Luck

drivers/acpi/apei/ghes.c static void ghes_do_proc(struct ghes *ghes, · line 429

Quoted 5 lines

drivers/acpi/apei/ghes.c +5 -0

+                       if (sec_sev == GHES_SEV_CORRECTED &&
+                           (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED) &&
+                           (mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS)) {
+                               unsigned long pfn;
+                               pfn = mem_err->physical_addr >> PAGE_SHIFT;

As Reagan said "Trust ... but verify" ... we should make sure BIOS gave us a good pfn

                                     if (pfn_valid(pfn))
                                          soft_memory_failure_queue(pfn, 0, 0);
                                     else
                                          printk( ...something about

BIOS giving us bad pfn = %lu\n", pfn);

Quoted 4 lines

Quoted 2 lines
Ah, nice catch - I thought soft_offline_page() takes care of this, but it sure is good to point a finger at the firmware.
While at it maybe make it pr_warning(FW_BUG or FW_WARN...

Yup - I've made it a pr_warning(FW_WARN... I just sent out a new version with these changes.

Thanks,
Naveen

Borislav Petkov

maintainer

2013-06-20 07:48 UTC #23 lore raw

Re: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

In reply to Naveen N. Rao

Quoted 15 lines · Naveen N. Rao

Add a boot option to disable firmware first mode for corrected errors.
Signed-off-by Naveen N. Rao <[email protected]>
---
 Documentation/x86/x86_64/boot-options.txt |    5 +++++
 arch/x86/include/asm/acpi.h               |    2 ++
 arch/x86/kernel/acpi/boot.c               |    5 +++++
 drivers/acpi/apei/hest.c                  |    3 ++-
 4 files changed, 14 insertions(+), 1 deletion(-)
Documentation/x86/x86_64/boot-options.txt +0 -0
index e9e8ddb..1228b22 100644

Documentation/x86/x86_64/boot-options.txt ACPI · line 182

Quoted 7 lines

Documentation/x86/x86_64/boot-options.txt +4 -0

@@ -176,6 +176,11 @@ ACPI
 
   acpi=noirq	Don't route interrupts
 
+  acpi=nocmcff	Disable firmware first mode for corrected errors. This
+		disables parsing the HEST CMC error source to check if
+		firmware has set the FF flag. This may result in
+		duplicate corrected error reports.

Ok, this option should be called something more vendor-agnostic like

acpi=noff

for example.

CMCI is Intel-specific and Firmware First is a generic way to say that firmware would like to look at the errors first.

AMD has similar thing called error thresholding so...

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Naveen N. Rao

author

2013-06-20 19:02 UTC #24 lore raw

Re: [PATCH v2 2/2] mce: acpi/apei: Add a boot option to disable ff mode for corrected errors

Quoted 26 lines · Borislav Petkov

Quoted 15 lines · Naveen N. Rao
Add a boot option to disable firmware first mode for corrected errors.
Signed-off-by Naveen N. Rao <[email protected]>
---
  Documentation/x86/x86_64/boot-options.txt |    5 +++++
  arch/x86/include/asm/acpi.h               |    2 ++
  arch/x86/kernel/acpi/boot.c               |    5 +++++
  drivers/acpi/apei/hest.c                  |    3 ++-
  4 files changed, 14 insertions(+), 1 deletion(-)
Documentation/x86/x86_64/boot-options.txt +0 -0
index e9e8ddb..1228b22 100644
Documentation/x86/x86_64/boot-options.txt ACPI · line 182
Quoted 7 lines
Documentation/x86/x86_64/boot-options.txt +4 -0
@@ -176,6 +176,11 @@ ACPI
 
    acpi=noirq	Don't route interrupts
 
+  acpi=nocmcff	Disable firmware first mode for corrected errors. This
+		disables parsing the HEST CMC error source to check if
+		firmware has set the FF flag. This may result in
+		duplicate corrected error reports.
Ok, this option should be called something more vendor-agnostic like

Quoted 9 lines

acpi=noff
for example.
CMCI is Intel-specific and Firmware First is a generic way to say that firmware would like to look at the errors first.
AMD has similar thing called error thresholding so...

nocmcff actually refers to CMC (Corrected Machine Check) Error source which is APEI terminology, so this is not actually Intel-specific and should apply equally well for AMD. I chose nocmcff over noff since there is also a firmware first flag for MCE Error source in APEI and this option only disables FF for CMC.

Thanks,
Naveen

Borislav Petkov

maintainer

2013-06-20 07:39 UTC #25 lore raw

In reply to Naveen N. Rao

Quoted 65 lines · 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
--
Changes:
- Incorporated comments from Boris and Tony from the previous thread at
http://thread.gmane.org/gmane.linux.acpi.devel/60802
- Added patch to disable firmware first mode through a boot option.
Signed-off-by Naveen N. Rao <[email protected]>
---
 arch/x86/include/asm/mce.h                |    3 ++
 arch/x86/kernel/cpu/mcheck/mce-internal.h |    3 ++
 arch/x86/kernel/cpu/mcheck/mce.c          |   25 ++++++++++++++++++
 arch/x86/kernel/cpu/mcheck/mce_intel.c    |   40 +++++++++++++++++++++++------
 drivers/acpi/apei/hest.c                  |   36 ++++++++++++++++++++++++++
 5 files changed, 99 insertions(+), 8 deletions(-)
arch/x86/include/asm/mce.h +3 -0
index fa5f71e..380fff8 100644
@@ -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
  */
arch/x86/kernel/cpu/mcheck/mce-internal.h +3 -0
index 5b7d4fa..193edc1 100644
@@ -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);
arch/x86/kernel/cpu/mcheck/mce.c +0 -0
index 9239504..9512034 100644

arch/x86/kernel/cpu/mcheck/mce.c DEFINE_PER_CPU(mce_banks_t, mce_poll_banks) = { · line 98

Quoted 5 lines

arch/x86/kernel/cpu/mcheck/mce.c +2 -0

@@ -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;

Why the second bitfield? The cleared bits in mce_poll_banks should be the banks which are handled in FF mode, no?

arch/x86/kernel/cpu/mcheck/mce.c static struct miscdevice mce_chrdev_device = { · line 1942

Quoted 13 lines

arch/x86/kernel/cpu/mcheck/mce.c +6 -0

+
 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,28 @@ 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 */

No need for that comment.

arch/x86/kernel/cpu/mcheck/mce.c static struct miscdevice mce_chrdev_device = { · line 1944

Quoted 2 lines

arch/x86/kernel/cpu/mcheck/mce.c +2 -0

+	__clear_bit(bank, __get_cpu_var(mce_poll_banks));
+	/* Disable CMCI */

No need for that comment either - function name cannot be more descriptive :-)

arch/x86/kernel/cpu/mcheck/mce.c static struct miscdevice mce_chrdev_device = { · line 1948

Quoted 4 lines

arch/x86/kernel/cpu/mcheck/mce.c +4 -0

+	cmci_disable_bank(bank);
+}
+
+void mce_disable_ce_bank(int bank)

Yeah, let's call it ...disable_poll_bank because we're disabling polling for those banks. And yes, we poll for errors for which no MCE exception is generated and those happen to be corrected but still...

Quoted 59 lines

arch/x86/kernel/cpu/mcheck/mce.c +11 -0

+{
+	if (bank >= mca_cfg.banks) {
+		pr_warning(FW_BUG 
+			"Ignoring request to disable invalid MCA bank %d.\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

arch/x86/kernel/cpu/mcheck/mce_intel.c +18 -1

index ae1697c..78256c0 100644
@@ -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);

arch/x86/kernel/cpu/mcheck/mce_intel.c void cmci_clear(void) · line 295

Quoted 10 lines

arch/x86/kernel/cpu/mcheck/mce_intel.c +1 -7

 	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);
 	}

This leaves only one line so no need for the {} braces anymore.

Quoted 8 lines

arch/x86/kernel/cpu/mcheck/mce_intel.c +2 -0

 	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)
+{

arch/x86/kernel/cpu/mcheck/mce_intel.c void cmci_reenable(void) · line 340

Quoted 10 lines

arch/x86/kernel/cpu/mcheck/mce_intel.c +10 -0

+	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);
+}

You need a prototype for that if you're going to call it from mce.c but CONFIG_X86_MCE_INTEL is not set:

arch/x86/built-in.o: In function `__mce_disable_ce_bank':
/home/boris/kernel/linux-2.6/arch/x86/kernel/cpu/mcheck/mce.c:1945: undefined reference to `cmci_disable_bank'
make: *** [vmlinux] Error 1

Quoted 30 lines

arch/x86/kernel/cpu/mcheck/mce_intel.c +1 -0

+
 static void intel_init_cmci(void)
 {
 	int banks;

drivers/acpi/apei/hest.c +11 -0

index f5ef5d5..d8c69ba 100644
@@ -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;
+

drivers/acpi/apei/hest.c int apei_hest_parse(apei_hest_func_t func, void *data) · line 144

Quoted 10 lines

drivers/acpi/apei/hest.c +10 -0

+	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.

... in firmware first mode.

drivers/acpi/apei/hest.c int apei_hest_parse(apei_hest_func_t func, void *data) · line 146

Quoted 2 lines

drivers/acpi/apei/hest.c +2 -0
+	 */
+	if (cmc->num_hardware_banks == 0)

	if (!cmc->num_hardware_banks)

drivers/acpi/apei/hest.c int apei_hest_parse(apei_hest_func_t func, void *data) · line 147

Quoted 1 line

drivers/acpi/apei/hest.c +1 -0
+		return 0;

Err, why does this function return 0 when the sanity checks above fail? apei_hest_parse actually looks at the retval and advances the hest_hdr.

drivers/acpi/apei/hest.c int apei_hest_parse(apei_hest_func_t func, void *data) · line 155

Quoted 8 lines

drivers/acpi/apei/hest.c +8 -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;

IOW, we should return 0 only here.

[ … ]

Signature · 9 lines

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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Naveen N. Rao

author

2013-06-20 19:08 UTC #26 lore raw

Quoted 69 lines · Borislav Petkov

Quoted 68 lines · 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
--
Changes:
- Incorporated comments from Boris and Tony from the previous thread at
http://thread.gmane.org/gmane.linux.acpi.devel/60802
- Added patch to disable firmware first mode through a boot option.
Signed-off-by Naveen N. Rao <[email protected]>
---
  arch/x86/include/asm/mce.h                |    3 ++
  arch/x86/kernel/cpu/mcheck/mce-internal.h |    3 ++
  arch/x86/kernel/cpu/mcheck/mce.c          |   25 ++++++++++++++++++
  arch/x86/kernel/cpu/mcheck/mce_intel.c    |   40 +++++++++++++++++++++++------
  drivers/acpi/apei/hest.c                  |   36 ++++++++++++++++++++++++++
  5 files changed, 99 insertions(+), 8 deletions(-)
arch/x86/include/asm/mce.h +3 -0
index fa5f71e..380fff8 100644
@@ -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
   */
arch/x86/kernel/cpu/mcheck/mce-internal.h +3 -0
index 5b7d4fa..193edc1 100644
@@ -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);
arch/x86/kernel/cpu/mcheck/mce.c +0 -0
index 9239504..9512034 100644
@@ -94,6 +94,9 @@ DEFINE_PER_CPU(mce_banks_t, mce_poll_banks) = {
  	[0 ... BITS_TO_LONGS(MAX_NR_BANKS)-1] = ~0UL
  };
 

Quoted 5 lines

arch/x86/kernel/cpu/mcheck/mce.c DEFINE_PER_CPU(mce_banks_t, mce_poll_banks) = { · line 98
Quoted 2 lines
arch/x86/kernel/cpu/mcheck/mce.c +2 -0
+/* MCA banks controlled through firmware first for corrected errors */
+mce_banks_t mce_banks_ce_disabled;
Why the second bitfield? The cleared bits in mce_poll_banks should be the banks which are handled in FF mode, no?

We need this bitfield to prevent enabling CMCI in future cmci_discover() invocations. See usage in cmci_discover() further below.

Quoted 17 lines

Quoted 6 lines
arch/x86/kernel/cpu/mcheck/mce.c +1 -0
+
  static DEFINE_PER_CPU(struct work_struct, mce_work);
 
  static void (*quirk_no_way_out)(int bank, struct mce *m, struct pt_regs *regs);
arch/x86/kernel/cpu/mcheck/mce.c static struct miscdevice mce_chrdev_device = { · line 1942
Quoted 8 lines
arch/x86/kernel/cpu/mcheck/mce.c +5 -0
@@ -1932,6 +1935,28 @@ 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 */
No need for that comment.

Quoted 5 lines

arch/x86/kernel/cpu/mcheck/mce.c static struct miscdevice mce_chrdev_device = { · line 1944
Quoted 2 lines
arch/x86/kernel/cpu/mcheck/mce.c +2 -0
+	__clear_bit(bank, __get_cpu_var(mce_poll_banks));
+	/* Disable CMCI */
No need for that comment either - function name cannot be more descriptive :-)

Sure, will remove these comments.

Quoted 9 lines

arch/x86/kernel/cpu/mcheck/mce.c static struct miscdevice mce_chrdev_device = { · line 1948
Quoted 5 lines
arch/x86/kernel/cpu/mcheck/mce.c +4 -0
+	cmci_disable_bank(bank);
+}
+
+void mce_disable_ce_bank(int bank)
Yeah, let's call it ...disable_poll_bank because we're disabling polling for those banks. And yes, we poll for errors for which no MCE exception is generated and those happen to be corrected but still...

We actually also disable CMCI here. So, in essence, we are disabling these banks for all sorts of direct corrected error reporting. I thought of naming this disable_ce_on_bank() or disable_ce_bank(), but felt that the mce_ prefix would be good to have. If that isn't necessary, I can rename this to disable_ce_on_bank() which sounds more accurate to me. Is that ok?

Quoted 57 lines

arch/x86/kernel/cpu/mcheck/mce.c +11 -0

+{
+	if (bank >= mca_cfg.banks) {
+		pr_warning(FW_BUG
+			"Ignoring request to disable invalid MCA bank %d.\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

arch/x86/kernel/cpu/mcheck/mce_intel.c +18 -1

index ae1697c..78256c0 100644
@@ -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;

Quoted 15 lines

arch/x86/kernel/cpu/mcheck/mce_intel.c void cmci_clear(void) · line 295

Quoted 13 lines

arch/x86/kernel/cpu/mcheck/mce_intel.c +1 -7

  	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);
  	}

This leaves only one line so no need for the {} braces anymore.

Ah, ok.

Quoted 23 lines

Quoted 9 lines
arch/x86/kernel/cpu/mcheck/mce_intel.c +2 -0
  	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)
+{
arch/x86/kernel/cpu/mcheck/mce_intel.c void cmci_reenable(void) · line 340
Quoted 10 lines
arch/x86/kernel/cpu/mcheck/mce_intel.c +10 -0
+	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);
+}
You need a prototype for that if you're going to call it from mce.c but CONFIG_X86_MCE_INTEL is not set:

Quoted 3 lines

arch/x86/built-in.o: In function `__mce_disable_ce_bank':
/home/boris/kernel/linux-2.6/arch/x86/kernel/cpu/mcheck/mce.c:1945: undefined reference to `cmci_disable_bank'
make: *** [vmlinux] Error 1

Good catch - missed this. Will address this.

Quoted 35 lines

arch/x86/kernel/cpu/mcheck/mce_intel.c +1 -0

+
  static void intel_init_cmci(void)
  {
  	int banks;

drivers/acpi/apei/hest.c +15 -0

index f5ef5d5..d8c69ba 100644
@@ -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;

Quoted 8 lines

drivers/acpi/apei/hest.c int apei_hest_parse(apei_hest_func_t func, void *data) · line 144
Quoted 6 lines
drivers/acpi/apei/hest.c +6 -0
+	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.
... in firmware first mode.

Ok.

Quoted 5 lines

drivers/acpi/apei/hest.c int apei_hest_parse(apei_hest_func_t func, void *data) · line 146
Quoted 3 lines
drivers/acpi/apei/hest.c +2 -0
+	 */
+	if (cmc->num_hardware_banks == 0)
	if (!cmc->num_hardware_banks)

Ok.

Quoted 6 lines

drivers/acpi/apei/hest.c int apei_hest_parse(apei_hest_func_t func, void *data) · line 147
Quoted 2 lines
drivers/acpi/apei/hest.c +1 -0
+		return 0;
Err, why does this function return 0 when the sanity checks above fail? apei_hest_parse actually looks at the retval and advances the hest_hdr.

Quoted 10 lines

drivers/acpi/apei/hest.c int apei_hest_parse(apei_hest_func_t func, void *data) · line 155
Quoted 8 lines
drivers/acpi/apei/hest.c +8 -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;
IOW, we should return 0 only here.

Quite the contrary actually. We want apei_hest_parse() to continue to the next error source structure _until_ we find CMC error source. This requires us to return 0 at the very first check we have for error source type. After that, we should actually return a non-zero value so we don't continue looking at further entries (as per APEI, only one error source of type CMC is permitted in HEST.) I will make this change.

As always, thanks for the detailed review!
- Naveen

Borislav Petkov

maintainer

2013-06-20 19:29 UTC #27 lore raw

Quoted 3 lines · Naveen N. Rao

We need this bitfield to prevent enabling CMCI in future cmci_discover() invocations. See usage in cmci_discover() further below.

So?!

	/* Skip banks in firmware first mode */
	if (!test_bit(i, __get_cpu_var(mce_poll_banks))
		continue;

...

Quoted 10 lines

Quoted 3 lines
Yeah, let's call it ...disable_poll_bank because we're disabling polling for those banks. And yes, we poll for errors for which no MCE exception is generated and those happen to be corrected but still...
We actually also disable CMCI here. So, in essence, we are disabling these banks for all sorts of direct corrected error reporting. I thought of naming this disable_ce_on_bank() or disable_ce_bank(), but felt that the mce_ prefix would be good to have. If that isn't necessary, I can rename this to disable_ce_on_bank() which sounds more accurate to me. Is that ok?

No, mce_disable_bank() removes the respective bank from the polling bitfield and cmci_disable_bank() actually disables CMCI which is Intel-only. So leave it at mce_disable_bank and that should be fine.

Thanks.

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Naveen N. Rao

author

2013-06-20 20:14 UTC #28 lore raw

Quoted 10 lines · Borislav Petkov

Quoted 3 lines · Naveen N. Rao
We need this bitfield to prevent enabling CMCI in future cmci_discover() invocations. See usage in cmci_discover() further below.
So?!
	/* Skip banks in firmware first mode */
	if (!test_bit(i, __get_cpu_var(mce_poll_banks))
		continue;

This won't work across cpu offline/online, right? We will end up _not_ enabling CMCI on certain banks where we should have.

Quoted 14 lines

...
Quoted 10 lines
Quoted 3 lines
Yeah, let's call it ...disable_poll_bank because we're disabling polling for those banks. And yes, we poll for errors for which no MCE exception is generated and those happen to be corrected but still...
We actually also disable CMCI here. So, in essence, we are disabling these banks for all sorts of direct corrected error reporting. I thought of naming this disable_ce_on_bank() or disable_ce_bank(), but felt that the mce_ prefix would be good to have. If that isn't necessary, I can rename this to disable_ce_on_bank() which sounds more accurate to me. Is that ok?

Quoted 3 lines

No, mce_disable_bank() removes the respective bank from the polling bitfield and cmci_disable_bank() actually disables CMCI which is Intel-only. So leave it at mce_disable_bank and that should be fine.

Ok. I will rename this to mce_disable_bank() from mce_disable_ce_bank().

Another thing: for hest_parse_cmc(), does the below look good?

        cmc = (struct acpi_hest_ia_corrected *)hest_hdr;
        if (!cmc->enabled)
                return 0;

#define ACPI_HEST_PARSING_DONE 1
        /*
         * We expect HEST to provide a list of MC banks that
         * report errors in firmware first mode.
         */
        if (!(cmc->flags & ACPI_HEST_FIRMWARE_FIRST) || 
!cmc->num_hardware_banks)
                return ACPI_HEST_PARSING_DONE;

The return value doesn't really matter since we don't check it, but returning an error looked like the wrong thing to do as well.

Thanks,
Naveen

Borislav Petkov

maintainer

2013-06-20 20:57 UTC #29 lore raw

Quoted 2 lines · Naveen N. Rao

This won't work across cpu offline/online, right? We will end up _not_ enabling CMCI on certain banks where we should have.

Huh, don't understand. cmci_discover runs on each CPU. After you've run hest_parse_cmc early during boot and cleared the mce_poll_banks bits, nothing will set them again so CPU hotplug doesn't matter...

Quoted 15 lines

Another thing: for hest_parse_cmc(), does the below look good?

        cmc = (struct acpi_hest_ia_corrected *)hest_hdr;
        if (!cmc->enabled)
                return 0;

#define ACPI_HEST_PARSING_DONE 1
        /*
         * We expect HEST to provide a list of MC banks that
         * report errors in firmware first mode.
         */
        if (!(cmc->flags & ACPI_HEST_FIRMWARE_FIRST) ||
!cmc->num_hardware_banks)
                return ACPI_HEST_PARSING_DONE;

Quoted 2 lines

The return value doesn't really matter since we don't check it, but returning an error looked like the wrong thing to do as well.

I'd add a comment above the "return 1" statement to explain why I'm doing this. It is much more verbose even than a well-named macro :)

Thanks.

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Naveen N. Rao

author

2013-06-20 21:22 UTC #30 lore raw

Quoted 7 lines · Borislav Petkov

Quoted 2 lines · Naveen N. Rao
This won't work across cpu offline/online, right? We will end up _not_ enabling CMCI on certain banks where we should have.
Huh, don't understand. cmci_discover runs on each CPU. After you've run hest_parse_cmc early during boot and cleared the mce_poll_banks bits, nothing will set them again so CPU hotplug doesn't matter...

Exactly, but mce_poll_banks also doesn't have bits set for banks on which CMCI is enabled.

Let's say we have a cpu with 2 banks (not shared), none of which work in FF mode. Both these banks support CMCI, so mce_poll_banks won't have these bits set.

On cpu offline, we call cmci_clear() which disables CMCI on these two banks before offlining it. When this cpu is brought online again, we call cmci_discover() which sees that mce_poll_banks doesn't have these two banks enabled and will skip enabling CMCI thinking these are in FF.

Right?

Quoted 16 lines

Another thing: for hest_parse_cmc(), does the below look good?

         cmc = (struct acpi_hest_ia_corrected *)hest_hdr;
         if (!cmc->enabled)
                 return 0;

#define ACPI_HEST_PARSING_DONE 1
         /*
          * We expect HEST to provide a list of MC banks that
          * report errors in firmware first mode.
          */
         if (!(cmc->flags & ACPI_HEST_FIRMWARE_FIRST) ||
!cmc->num_hardware_banks)
                 return ACPI_HEST_PARSING_DONE;

Quoted 5 lines

Quoted 2 lines
The return value doesn't really matter since we don't check it, but returning an error looked like the wrong thing to do as well.
I'd add a comment above the "return 1" statement to explain why I'm doing this. It is much more verbose even than a well-named macro :)

Okay :)

Thanks,
Naveen

Borislav Petkov

maintainer

2013-06-21 07:34 UTC #31 lore raw

Quoted 7 lines · Naveen N. Rao

Exactly, but mce_poll_banks also doesn't have bits set for banks on which CMCI is enabled.
Let's say we have a cpu with 2 banks (not shared), none of which work in FF mode. Both these banks support CMCI, so mce_poll_banks won't have these bits set.

Quoted 5 lines

On cpu offline, we call cmci_clear() which disables CMCI on these two banks before offlining it. When this cpu is brought online again, we call cmci_discover() which sees that mce_poll_banks doesn't have these two banks enabled and will skip enabling CMCI thinking these are in FF.

Hmm, mce_intel has yet another bitfield - mce_banks_owned. (Btw, this is why I have a problem with adding yet another bitfield).

The way I understand it is, if a bit is set in the owned bitfield, those banks belong to CMCI and are not polled.

Now, can we use both mce_banks_owned and mce_poll_banks? If a bit in both bifields is cleared, the corresponding bank is not polled *and* is not owned by CMCI => it is in FF mode.

Makes sense?

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Naveen N. Rao

author

2013-06-21 07:47 UTC #32 lore raw

Quoted 17 lines · Borislav Petkov

Quoted 7 lines · Naveen N. Rao
Exactly, but mce_poll_banks also doesn't have bits set for banks on which CMCI is enabled.
Let's say we have a cpu with 2 banks (not shared), none of which work in FF mode. Both these banks support CMCI, so mce_poll_banks won't have these bits set.
Quoted 5 lines
On cpu offline, we call cmci_clear() which disables CMCI on these two banks before offlining it. When this cpu is brought online again, we call cmci_discover() which sees that mce_poll_banks doesn't have these two banks enabled and will skip enabling CMCI thinking these are in FF.
Hmm, mce_intel has yet another bitfield - mce_banks_owned. (Btw, this is why I have a problem with adding yet another bitfield).

Quoted 9 lines

The way I understand it is, if a bit is set in the owned bitfield, those banks belong to CMCI and are not polled.
Now, can we use both mce_banks_owned and mce_poll_banks? If a bit in both bifields is cleared, the corresponding bank is not polled *and* is not owned by CMCI => it is in FF mode.
Makes sense?

Yes, but I'm afraid this won't work either - mce_banks_owned is cleared during cpu offline. This is necessary since a cmci rediscover is triggered on cpu offline, so that if this bank is shared across cores, a different cpu can claim ownership of this bank.

The difference between the new bitfield and the existing bitfields is that the new one is not per-cpu. This is a global list of banks across cpus that we do not want enabled.

Thanks,
Naveen

Borislav Petkov

maintainer

2013-06-21 08:36 UTC #33 lore raw

Quoted 5 lines · Naveen N. Rao

Yes, but I'm afraid this won't work either - mce_banks_owned is cleared during cpu offline. This is necessary since a cmci rediscover is triggered on cpu offline, so that if this bank is shared across cores, a different cpu can claim ownership of this bank.

What for? Sounds strange to me.

Anyway, the reason for which mce_banks_owned won't work is because it is in mce_intel and we need a mce.c global bitfield for banks which are excluded from CE handling.

So ok, I'm persuaded, yet another bitfield it is ... :-\

Thanks.

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Naveen N. Rao

author

2013-06-21 09:32 UTC #34 lore raw

Quoted 8 lines · Borislav Petkov

Quoted 5 lines · Naveen N. Rao
Yes, but I'm afraid this won't work either - mce_banks_owned is cleared during cpu offline. This is necessary since a cmci rediscover is triggered on cpu offline, so that if this bank is shared across cores, a different cpu can claim ownership of this bank.
What for? Sounds strange to me.

Look at section "15.5.1 CMCI Local APIC Interface" from Intel SDM Vol. 3, and the subsequent section on "System Software Recommendation for Managing CMCI and Machine Check Resources":
"For example, if a corrected bit error in a cache shared by two logical processors caused a CMCI, the interrupt will be delivered to both logical processors sharing that microarchitectural sub-system."

In other words, some of the MC banks are shared across logical cpus in a core and some across all cores in a package. During initialization, the first cpu in a core ends up owning most of the banks specific to the core/package. When this cpu is offlined, we would want the second cpu in that core to discover and enable CMCI for those MC banks which it shares with the first cpu.

As an example, consider a hypothetical single-core Intel processor with Hyperthreading. On init, let's say the first cpu ends up owning banks 1, 2, 3 and 4; and the second cpu ends up owning banks 1 and 2. This would mean that MC banks 1 and 2 are "hyperthread"-specific, while banks 3 and 4 are shared. Now, if we offline the first cpu, it disables CMCI on all 4 banks. However, banks 3 and 4 are shared. So, if we now do a cmci rediscovery, the second cpu will see that banks 3 and 4 don't have CMCI enabled and will then claim ownership of those so that we can continue to receive and process CMCIs from those subsystems.

Makes sense now?

Thanks,
Naveen

Borislav Petkov

maintainer

2013-06-21 14:08 UTC #35 lore raw

Quoted 6 lines · Naveen N. Rao

As an example, consider a hypothetical single-core Intel processor with Hyperthreading. On init, let's say the first cpu ends up owning banks 1, 2, 3 and 4; and the second cpu ends up owning banks 1 and
2. This would mean that MC banks 1 and 2 are "hyperthread"-specific, while banks 3 and 4 are shared. Now, if we offline the first cpu, it disables CMCI on all 4 banks.

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This is what I find strange - having to disable CMCI, especially on a shared bank, just to reenable it right back on the next core. But I guess this is a constraint dictated by the hardware...

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Tony Luck

2013-06-21 16:47 UTC #36 lore raw

In reply to Borislav Petkov

Quoted 1 line · Borislav Petkov

So ok, I'm persuaded, yet another bitfield it is ... :-\

Let's add some more comments on what each of these bitfields mean. Otherwise we will be going back over this next time we have a patch that touches one of them and we've all forgotten the subtle details explained in this e-mail thread.

-Tony

Borislav Petkov

maintainer

2013-06-21 17:41 UTC #37 lore raw

Quoted 7 lines · Tony Luck

Quoted 1 line · Borislav Petkov
So ok, I'm persuaded, yet another bitfield it is ... :-\
Let's add some more comments on what each of these bitfields mean. Otherwise we will be going back over this next time we have a patch that touches one of them and we've all forgotten the subtle details explained in this e-mail thread.

Very good point - it's like you're reading my mind! I thought about suggesting exactly that but then it slipped my mind with all the details and commotion here.

:-)

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Naveen N. Rao

author

2013-06-25 17:46 UTC #38 lore raw

Tony, Boris,
Can you please see if the comments in the below patch include the details you were expecting?

Thanks,
Naveen

--
Add comments to clarify usage of the various bitfields in the MCA subsystem

Signed-off-by Naveen N. Rao <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c       |    5 ++++-
 arch/x86/kernel/cpu/mcheck/mce_intel.c |    9 +++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

arch/x86/kernel/cpu/mcheck/mce.c +4 -1

index 9239504..bf49cdb 100644
@@ -89,7 +89,10 @@ static DECLARE_WAIT_QUEUE_HEAD(mce_chrdev_wait);
 static DEFINE_PER_CPU(struct mce, mces_seen);
 static int			cpu_missing;
 
-/* MCA banks polled by the period polling timer for corrected events */
+/*
+ * MCA banks polled by the period polling timer for corrected events.
+ * With Intel CMCI, this only has MCA banks which do not support CMCI (if any).
+ */
 DEFINE_PER_CPU(mce_banks_t, mce_poll_banks) = {
 	[0 ... BITS_TO_LONGS(MAX_NR_BANKS)-1] = ~0UL
 };

arch/x86/kernel/cpu/mcheck/mce_intel.c +9 -0

index ae1697c..2816f53 100644
@@ -24,6 +24,15 @@
  * Also supports reliable discovery of shared banks.
  */
 
+/*
+ * Indicates MCA banks controlled by the current cpu for CMCI. Note that this
+ * can change when a cpu is offlined or brought online since some MCA banks
+ * are shared across cpus. When a cpu is offlined, cmci_clear() disables CMCI
+ * on all banks owned by the cpu and clears this bitfield. At this point,
+ * cmci_rediscover() kicks in and a different cpu may end up taking
+ * ownership of some of the shared MCA banks that were previously owned
+ * by the offlined cpu.
+ */
 static DEFINE_PER_CPU(mce_banks_t, mce_banks_owned);
 
 /*

Borislav Petkov

maintainer Acked-by

2013-06-25 17:53 UTC #39 lore raw

Quoted 40 lines · Naveen N. Rao

Tony, Boris,
Can you please see if the comments in the below patch include the details you were expecting?

Thanks,
Naveen

--
Add comments to clarify usage of the various bitfields in the MCA subsystem

Signed-off-by Naveen N. Rao <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c       |    5 ++++-
 arch/x86/kernel/cpu/mcheck/mce_intel.c |    9 +++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

arch/x86/kernel/cpu/mcheck/mce.c +4 -1

index 9239504..bf49cdb 100644
@@ -89,7 +89,10 @@ static DECLARE_WAIT_QUEUE_HEAD(mce_chrdev_wait);
 static DEFINE_PER_CPU(struct mce, mces_seen);
 static int			cpu_missing;
 
-/* MCA banks polled by the period polling timer for corrected events */
+/*
+ * MCA banks polled by the period polling timer for corrected events.
+ * With Intel CMCI, this only has MCA banks which do not support CMCI (if any).
+ */
 DEFINE_PER_CPU(mce_banks_t, mce_poll_banks) = {
 	[0 ... BITS_TO_LONGS(MAX_NR_BANKS)-1] = ~0UL
 };

arch/x86/kernel/cpu/mcheck/mce_intel.c +0 -0

index ae1697c..2816f53 100644
@@ -24,6 +24,15 @@
  * Also supports reliable discovery of shared banks.
  */

arch/x86/kernel/cpu/mcheck/mce_intel.c line 36

Quoted 10 lines

arch/x86/kernel/cpu/mcheck/mce_intel.c +9 -0

+/*
+ * Indicates MCA banks controlled by the current cpu for CMCI. Note that this
+ * can change when a cpu is offlined or brought online since some MCA banks
+ * are shared across cpus. When a cpu is offlined, cmci_clear() disables CMCI
+ * on all banks owned by the cpu and clears this bitfield. At this point,
+ * cmci_rediscover() kicks in and a different cpu may end up taking
+ * ownership of some of the shared MCA banks that were previously owned
+ * by the offlined cpu.
+ */
 static DEFINE_PER_CPU(mce_banks_t, mce_banks_owned);

Yep, it looks ok to me.

Acked-by Borislav Petkov <[email protected]>

Thanks.

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Luck, Tony

reviewer

2013-06-25 17:55 UTC #40 lore raw

In reply to Naveen N. Rao

+/*
+ * Indicates MCA banks controlled by the current cpu for CMCI. Note that this
+ * can change when a cpu is offlined or brought online since some MCA banks
+ * are shared across cpus. When a cpu is offlined, cmci_clear() disables CMCI
+ * on all banks owned by the cpu and clears this bitfield. At this point,
+ * cmci_rediscover() kicks in and a different cpu may end up taking
+ * ownership of some of the shared MCA banks that were previously owned
+ * by the offlined cpu.
+ */
static DEFINE_PER_CPU(mce_banks_t, mce_banks_owned);

Maybe an extra sentence or two at the beginning to say *why* we need this. E.g.

/*
* CMCI can be delivered to multiple cpus that share a machine check bank
* so we need to designate a single cpu to process errors logged in each bank
* in the interrupt handler (otherwise we would have many races and potential
* double reporting of the same error.
*/
...

-Tony

Naveen N. Rao

author Acked-by Borislav Petkov

2013-06-25 18:29 UTC #41 lore raw

Tony,
Thanks - I have included your text in the patch. I wasn't sure if I should add your Signed-off-by. Kindly review and do the needful.

Thanks,
Naveen

--
Add comments to clarify usage of the various bitfields in the MCA subsystem

Signed-off-by Naveen N. Rao <[email protected]>

Acked-by Borislav Petkov <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c       |    5 ++++-
 arch/x86/kernel/cpu/mcheck/mce_intel.c |   12 ++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

arch/x86/kernel/cpu/mcheck/mce.c +4 -1

index 9239504..bf49cdb 100644
@@ -89,7 +89,10 @@ static DECLARE_WAIT_QUEUE_HEAD(mce_chrdev_wait);
 static DEFINE_PER_CPU(struct mce, mces_seen);
 static int			cpu_missing;
 
-/* MCA banks polled by the period polling timer for corrected events */
+/*
+ * MCA banks polled by the period polling timer for corrected events.
+ * With Intel CMCI, this only has MCA banks which do not support CMCI (if any).
+ */
 DEFINE_PER_CPU(mce_banks_t, mce_poll_banks) = {
 	[0 ... BITS_TO_LONGS(MAX_NR_BANKS)-1] = ~0UL
 };

arch/x86/kernel/cpu/mcheck/mce_intel.c +12 -0

index ae1697c..d5640530 100644
@@ -24,6 +24,18 @@
  * Also supports reliable discovery of shared banks.
  */
 
+/*
+ * CMCI can be delivered to multiple cpus that share a machine check bank
+ * so we need to designate a single cpu to process errors logged in each bank
+ * in the interrupt handler (otherwise we would have many races and potential
+ * double reporting of the same error).
+ * Note that this can change when a cpu is offlined or brought online since
+ * some MCA banks are shared across cpus. When a cpu is offlined, cmci_clear()
+ * disables CMCI on all banks owned by the cpu and clears this bitfield. At
+ * this point, cmci_rediscover() kicks in and a different cpu may end up
+ * taking ownership of some of the shared MCA banks that were previously
+ * owned by the offlined cpu.
+ */
 static DEFINE_PER_CPU(mce_banks_t, mce_banks_owned);
 
 /*