x86/mce: Various fixes and cleanups for MCE handling

Hi Boris.

This is the 2nd iteration of a smallish series with some fixes/cleanups for the handling of MCEs. It should apply cleanly to your ras/core branch.

The first iteration can be found here:
https://lore.kernel.org/linux-edac/[email protected]/T/

Changes v1 -> v2:
- dropped patches 3, 5, 6 as you already cherry-picked them into ras/core or ras/urgent (this renumbers patch 4 in v1 to patch 3 in v2);
- addressed remaining comments on patches 1-3;
- added patch 5 as per Yazen's comment that the SRAO notifier shall not be used on AMD for now;
- added patch 4 as a prerequisite for the given realization of patch 5;
- added patch 6 as an example, what else can be done due to patch 4.

See individual patches 1-3 for more detailed comments on changes.

I'm not yet convinced, that patch 6 is an entirely good idea. I've still included it for discussion. If we end up not doing something like it, we can as well rewrite patch 5 to be just another "if" within srao_decode_notifier()/uc_decode_notifier().

Regards
Jan

Jan H. Schönherr (6):
x86/mce: Take action on UCNA/Deferred errors again x86/mce: Make mce=nobootlog work again
x86/mce: Fix use of uninitialized MCE message string x86/mce: Allow a variable number of internal MCE decode notifiers x86/mce: Do not take action on SRAO/Deferred errors on AMD for now x86/mce: Dynamically register default MCE handler

 arch/x86/include/asm/mce.h     |   2 +-
 arch/x86/kernel/cpu/mce/core.c | 145 ++++++++++++++++++---------------
 2 files changed, 81 insertions(+), 66 deletions(-)

2.22.0.3.gb49bb57c8208.dirty

Commit fa92c5869426 ("x86, mce: Support memory error recovery for both UCNA and Deferred error in machine_check_poll") added handling of UCNA and Deferred errors by adding them to the ring for SRAO errors.

Later, commit fd4cf79fcc4b ("x86/mce: Remove the MCE ring for Action Optional errors") switched storage from the SRAO ring to the unified pool that is still in use today. In order to only act on the intended errors, a filter for MCE_AO_SEVERITY is used -- effectively removing handling of UCNA/Deferred errors again.

Extend the severity filter to include UCNA/Deferred errors again. Also, generalize the naming of the notifier from SRAO to UC to capture the extended scope.

Note, that this change may cause a message like the following to appear, as the same address may be reported as SRAO and as UCNA:

Memory failure: 0x5fe3284: already hardware poisoned

Technically, this is a return to previous behavior.

---
Changes v1->v2:
- rename notifier from SRAO to UC (as requested by Tony)
- extend commit message (per remark from Tony)
- don't mention Linux versions (per remark from Boris)

There was some discussion on v1, whether the SRAO/UC notifier does the right thing or not. While it seems to be correct as is for Intel (per Tony), there were some concerns for AMD (per Yazen). Hence, there is a new patch 5 in this series, which disables the notifier on AMD.

---
 arch/x86/include/asm/mce.h     |  2 +-
 arch/x86/kernel/cpu/mce/core.c | 31 ++++++++++++++++---------------
 2 files changed, 17 insertions(+), 16 deletions(-)

index dc2d4b206ab7..c8ff6f6750ef 100644
@@ -144,7 +144,7 @@ struct mce_log_buffer {
 
 enum mce_notifier_prios {
 	MCE_PRIO_FIRST		= INT_MAX,
-	MCE_PRIO_SRAO		= INT_MAX - 1,
+	MCE_PRIO_UC		= INT_MAX - 1,
 	MCE_PRIO_EXTLOG		= INT_MAX - 2,
 	MCE_PRIO_NFIT		= INT_MAX - 3,
 	MCE_PRIO_EDAC		= INT_MAX - 4,

index 8994fe7751a4..16134ce587fd 100644
@@ -156,10 +156,8 @@ void mce_log(struct mce *m)
 }
 EXPORT_SYMBOL_GPL(mce_log);
 
-static struct notifier_block mce_srao_nb;
-
 /*
- * We run the default notifier if we have only the SRAO, the first and the
+ * We run the default notifier if we have only the UC, the first and the
  * default notifier registered. I.e., the mandatory NUM_DEFAULT_NOTIFIERS
  * notifiers registered on the chain.
  */

@@ -580,26 +578,29 @@ static struct notifier_block first_nb = {
 	.priority	= MCE_PRIO_FIRST,
 };
 
-static int srao_decode_notifier(struct notifier_block *nb, unsigned long val,
-				void *data)
+static int uc_decode_notifier(struct notifier_block *nb, unsigned long val,
+			      void *data)
 {
 	struct mce *mce = (struct mce *)data;
 	unsigned long pfn;
 
-	if (!mce)
+	if (!mce || !mce_usable_address(mce))
 		return NOTIFY_DONE;
 
-	if (mce_usable_address(mce) && (mce->severity == MCE_AO_SEVERITY)) {
-		pfn = mce->addr >> PAGE_SHIFT;
-		if (!memory_failure(pfn, 0))
-			set_mce_nospec(pfn);
-	}
+	if (mce->severity != MCE_AO_SEVERITY &&
+	    mce->severity != MCE_DEFERRED_SEVERITY)
+		return NOTIFY_DONE;
+
+	pfn = mce->addr >> PAGE_SHIFT;
+	if (!memory_failure(pfn, 0))
+		set_mce_nospec(pfn);
 
 	return NOTIFY_OK;
 }
-static struct notifier_block mce_srao_nb = {
-	.notifier_call	= srao_decode_notifier,
-	.priority	= MCE_PRIO_SRAO,
+
+static struct notifier_block mce_uc_nb = {
+	.notifier_call	= uc_decode_notifier,
+	.priority	= MCE_PRIO_UC,
 };
 
 static int mce_default_notifier(struct notifier_block *nb, unsigned long val,

@@ -1970,7 +1971,7 @@ int __init mcheck_init(void)
 {
 	mcheck_intel_therm_init();
 	mce_register_decode_chain(&first_nb);
-	mce_register_decode_chain(&mce_srao_nb);
+	mce_register_decode_chain(&mce_uc_nb);
 	mce_register_decode_chain(&mce_default_nb);
 	mcheck_vendor_init_severity();
 

2.22.0.3.gb49bb57c8208.dirty

Tony, ACK?

Also, do you want it in stable@ so that it gets backported?

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

That's a tricky question. We have changing behavior (UCNA pages offlined, then a few kernel versions stopped doing this, now we are going to start doing it again. But is it really a _BUG_ that needs backporting to stable? I'm leaning towards "no it isn't". But could perhaps be convinced to change my mind if somebody has a good reason for wanting it there.

Is there something to put in the tags to stop this being autoselected for backport because it has a Fixes: tag?

I totally agree that counting notifiers is clumsy. Also less than ideal is the concept that any notifier on the chain can declare:
"I fixed it"
and prevent any other notifiers from even seeing it. Well the concept is good, but it is overused.

I think we may do better with a field in the "struct mce" that is being passed to each where notifiers can wiggle some bits (semantics to be defined later) which can tell subsequent notifiers what sort of actions have been taken.
E.g. the SRAO/UCNA notifier can say "I took this page offline"
the dev_mcelog one can say "I think I handed to a process that has /dev/mcelog open" EDAC drivers can say "I decoded the address and printed something"
CEC can say: "I silently counted this corrected error", or "error exceeded threshold and I took the page offline".

The default notifier can print to console if nobody set a bit to say that the error had been somehow logged.

-Tony

I think the same, so let's not. We can always backport it later, if it turns out that it is needed.

Yah.

I don't think so.

What we could do is write the "Fixes:" tag in free text in the commit message so that tools don't pick up on it.

I'll reply to the other thing below in a separate mail.

Thx.

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

But why can't we use it?

Don't get me wrong: I'm simply following my KISS approach to do the simplest scheme required. So, do you see a use case where the whole error handling chain would need more sophisticated handling?

That idea is good and I'll gladly take patches for it so if you wanna do it...

Thx.

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Since commit 8b38937b7ab5 ("x86/mce: Do not enter deferred errors into the generic pool twice") the mce=nobootlog option has become mostly ineffective (after being only slightly ineffective before), as the code is taking actions on MCEs left over from boot when they have a usable address.

Move the check for MCP_DONTLOG a bit outward to make it effective again.

Also, since commit 011d82611172 ("RAS: Add a Corrected Errors Collector") the two branches of the remaining "if" the bottom of machine_check_poll() do same. Unify them.

---
Changes v1->v2:
- remove an indentation level in favor of a goto (requested by Boris)
- don't mention Linux version (per remark from Boris)

---
 arch/x86/kernel/cpu/mce/core.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

index 16134ce587fd..0ccd6cf3402d 100644
@@ -750,26 +750,22 @@ bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
 log_it:
 		error_seen = true;
 
-		mce_read_aux(&m, i);
+		if (flags & MCP_DONTLOG)
+			goto clear_it;
 
+		mce_read_aux(&m, i);
 		m.severity = mce_severity(&m, mca_cfg.tolerant, NULL, false);
-
 		/*
 		 * Don't get the IP here because it's unlikely to
 		 * have anything to do with the actual error location.
 		 */
-		if (!(flags & MCP_DONTLOG) && !mca_cfg.dont_log_ce)
-			mce_log(&m);
-		else if (mce_usable_address(&m)) {
-			/*
-			 * Although we skipped logging this, we still want
-			 * to take action. Add to the pool so the registered
-			 * notifiers will see it.
-			 */
-			if (!mce_gen_pool_add(&m))
-				mce_schedule_work();
-		}
 
+		if (mca_cfg.dont_log_ce && !mce_usable_address(&m))
+			goto clear_it;
+
+		mce_log(&m);
+
+clear_it:
 		/*
 		 * Clear state for this bank.
 		 */

2.22.0.3.gb49bb57c8208.dirty

The function mce_severity() is not required to update its msg argument. In fact, mce_severity_amd() does not, which makes mce_no_way_out() return uninitialized data, which may be used later for printing.

Assuming that implementations of mce_severity() either always or never update the msg argument (which is currently the case), it is sufficient to initialize the temporary variable in mce_no_way_out().

While at it, avoid printing a useless "Unknown".

---
Changes v1->v2:
- simplify fix by assuming that mce_severity() either always or never updates the msg argument -- as opposed to mce_severity() having the freedom to decide on a case by case basis (requested by Boris);
- stop printing "Unknown" (requested by Boris).

---
 arch/x86/kernel/cpu/mce/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 0ccd6cf3402d..1d91ce956772 100644
@@ -790,7 +790,7 @@ EXPORT_SYMBOL_GPL(machine_check_poll);
 static int mce_no_way_out(struct mce *m, char **msg, unsigned long *validp,
 			  struct pt_regs *regs)
 {
-	char *tmp;
+	char *tmp = *msg;
 	int i;
 
 	for (i = 0; i < this_cpu_read(mce_num_banks); i++) {

@@ -1209,8 +1209,8 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 	DECLARE_BITMAP(toclear, MAX_NR_BANKS);
 	struct mca_config *cfg = &mca_cfg;
 	int cpu = smp_processor_id();
-	char *msg = "Unknown";
 	struct mce m, *final;
+	char *msg = NULL;
 	int worst = 0;
 
 	/*

2.22.0.3.gb49bb57c8208.dirty

Get rid of the compile time constant of internal (or mandatory) MCE decode notifiers in preparation for future changes. Instead, distinguish explicitly between internal and external MCE decode notifiers.

---
New in v2, preparation for patches 5 and 6.

---
 arch/x86/kernel/cpu/mce/core.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

index 1d91ce956772..d48deb127071 100644
@@ -157,21 +157,24 @@ void mce_log(struct mce *m)
 EXPORT_SYMBOL_GPL(mce_log);
 
 /*
- * We run the default notifier if we have only the UC, the first and the
- * default notifier registered. I.e., the mandatory NUM_DEFAULT_NOTIFIERS
+ * We run the default notifier as long as we have no external
  * notifiers registered on the chain.
  */
-#define NUM_DEFAULT_NOTIFIERS	3
 static atomic_t num_notifiers;
 
-void mce_register_decode_chain(struct notifier_block *nb)
+static void mce_register_decode_chain_internal(struct notifier_block *nb)
 {
 	if (WARN_ON(nb->priority > MCE_PRIO_MCELOG && nb->priority < MCE_PRIO_EDAC))
 		return;
 
+	blocking_notifier_chain_register(&x86_mce_decoder_chain, nb);
+}
+
+void mce_register_decode_chain(struct notifier_block *nb)
+{
 	atomic_inc(&num_notifiers);
 
-	blocking_notifier_chain_register(&x86_mce_decoder_chain, nb);
+	mce_register_decode_chain_internal(nb);
 }
 EXPORT_SYMBOL_GPL(mce_register_decode_chain);
 

@@ -611,7 +614,7 @@ static int mce_default_notifier(struct notifier_block *nb, unsigned long val,
 	if (!m)
 		return NOTIFY_DONE;
 
-	if (atomic_read(&num_notifiers) > NUM_DEFAULT_NOTIFIERS)
+	if (atomic_read(&num_notifiers))
 		return NOTIFY_DONE;
 
 	__print_mce(m);

@@ -1966,9 +1969,9 @@ __setup("mce", mcheck_enable);
 int __init mcheck_init(void)
 {
 	mcheck_intel_therm_init();
-	mce_register_decode_chain(&first_nb);
-	mce_register_decode_chain(&mce_uc_nb);
-	mce_register_decode_chain(&mce_default_nb);
+	mce_register_decode_chain_internal(&first_nb);
+	mce_register_decode_chain_internal(&mce_uc_nb);
+	mce_register_decode_chain_internal(&mce_default_nb);
 	mcheck_vendor_init_severity();
 
 	INIT_WORK(&mce_work, mce_gen_pool_process);

2.22.0.3.gb49bb57c8208.dirty

Per Yazen Ghannam we should not use the UC notifier for the time being on AMD.

---
New in v2. This is due to a remark from Yazen on v1, that we shouldn't be handling neither SRAO nor Deferred errors in that handler.

An alternative implementation would do the architecture "if" directly within uc_decode_notifier(), in which case we could decide to not apply patch 4.

---
 arch/x86/kernel/cpu/mce/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index d48deb127071..d8fe5b048ee7 100644
@@ -1970,7 +1970,8 @@ int __init mcheck_init(void)
 {
 	mcheck_intel_therm_init();
 	mce_register_decode_chain_internal(&first_nb);
-	mce_register_decode_chain_internal(&mce_uc_nb);
+	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
+		mce_register_decode_chain_internal(&mce_uc_nb);
 	mce_register_decode_chain_internal(&mce_default_nb);
 	mcheck_vendor_init_severity();
 

2.22.0.3.gb49bb57c8208.dirty

The default MCE handler takes action, when no external MCE handler is registered. Instead of checking for external handlers within the default MCE handler, only register the default MCE handler when there are no external handlers in the first place.

---
New in v2. This is something that became possible due to patch 4. I'm not entirely happy with it.

One the one hand, I'm wondering whether there's a nicer way to handle (de-)registration races.

On the other hand, I'm starting to question the whole logic to "only print the MCE if nothing else in the kernel has a handler registered". Is that really how it should be? For example, there are handlers that filter for a specific subset of MCEs. If one of those is registered, we're losing all information for MCEs that don't match.

A possible solution to the latter would be to have a "handled" or "printed" flag within "struct mce" and print the MCE based on that in the default handler. What do you think?

---
 arch/x86/kernel/cpu/mce/core.c | 90 ++++++++++++++++++++--------------
 1 file changed, 52 insertions(+), 38 deletions(-)

index d8fe5b048ee7..3b6e37c5252f 100644
@@ -156,36 +156,6 @@ void mce_log(struct mce *m)
 }
 EXPORT_SYMBOL_GPL(mce_log);
 
-/*
- * We run the default notifier as long as we have no external
- * notifiers registered on the chain.
- */
-static atomic_t num_notifiers;
-
-static void mce_register_decode_chain_internal(struct notifier_block *nb)
-{
-	if (WARN_ON(nb->priority > MCE_PRIO_MCELOG && nb->priority < MCE_PRIO_EDAC))
-		return;
-
-	blocking_notifier_chain_register(&x86_mce_decoder_chain, nb);
-}
-
-void mce_register_decode_chain(struct notifier_block *nb)
-{
-	atomic_inc(&num_notifiers);
-
-	mce_register_decode_chain_internal(nb);
-}
-EXPORT_SYMBOL_GPL(mce_register_decode_chain);
-
-void mce_unregister_decode_chain(struct notifier_block *nb)
-{
-	atomic_dec(&num_notifiers);
-
-	blocking_notifier_chain_unregister(&x86_mce_decoder_chain, nb);
-}
-EXPORT_SYMBOL_GPL(mce_unregister_decode_chain);
-
 static inline u32 ctl_reg(int bank)
 {
 	return MSR_IA32_MCx_CTL(bank);

@@ -606,18 +576,19 @@ static struct notifier_block mce_uc_nb = {
 	.priority	= MCE_PRIO_UC,
 };
 
+/*
+ * We run the default notifier as long as we have no external
+ * notifiers registered on the chain.
+ */
+static atomic_t num_notifiers;
+
 static int mce_default_notifier(struct notifier_block *nb, unsigned long val,
 				void *data)
 {
 	struct mce *m = (struct mce *)data;
 
-	if (!m)
-		return NOTIFY_DONE;
-
-	if (atomic_read(&num_notifiers))
-		return NOTIFY_DONE;
-
-	__print_mce(m);
+	if (m)
+		__print_mce(m);
 
 	return NOTIFY_DONE;
 }

@@ -628,6 +599,49 @@ static struct notifier_block mce_default_nb = {
 	.priority	= MCE_PRIO_LOWEST,
 };
 
+static void update_default_notifier_registration(void)
+{
+	bool has_notifiers = !!atomic_read(&num_notifiers);
+
+retry:
+	if (has_notifiers)
+		blocking_notifier_chain_unregister(&x86_mce_decoder_chain,
+						   &mce_default_nb);
+	else
+		blocking_notifier_chain_cond_register(&x86_mce_decoder_chain,
+						      &mce_default_nb);
+
+	if (has_notifiers != !!atomic_read(&num_notifiers)) {
+		has_notifiers = !has_notifiers;
+		goto retry;
+	}
+}
+
+static void mce_register_decode_chain_internal(struct notifier_block *nb)
+{
+	if (WARN_ON(nb->priority > MCE_PRIO_MCELOG &&
+		    nb->priority < MCE_PRIO_EDAC))
+		return;
+
+	blocking_notifier_chain_register(&x86_mce_decoder_chain, nb);
+}
+
+void mce_register_decode_chain(struct notifier_block *nb)
+{
+	atomic_inc(&num_notifiers);
+	mce_register_decode_chain_internal(nb);
+	update_default_notifier_registration();
+}
+EXPORT_SYMBOL_GPL(mce_register_decode_chain);
+
+void mce_unregister_decode_chain(struct notifier_block *nb)
+{
+	atomic_dec(&num_notifiers);
+	update_default_notifier_registration();
+	blocking_notifier_chain_unregister(&x86_mce_decoder_chain, nb);
+}
+EXPORT_SYMBOL_GPL(mce_unregister_decode_chain);
+
 /*
  * Read ADDR and MISC registers.
  */

@@ -1972,7 +1986,7 @@ int __init mcheck_init(void)
 	mce_register_decode_chain_internal(&first_nb);
 	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
 		mce_register_decode_chain_internal(&mce_uc_nb);
-	mce_register_decode_chain_internal(&mce_default_nb);
+	update_default_notifier_registration();
 	mcheck_vendor_init_severity();
 
 	INIT_WORK(&mce_work, mce_gen_pool_process);

2.22.0.3.gb49bb57c8208.dirty

Instead of unregistering/registering the default notifier depending on whether there are other notifiers, couldn't you just make the default notifier check to see if it should print. E.g.

static int mce_default_notifier(struct notifier_block *nb, unsigned long val,
void *data)

{
	struct mce *m = (struct mce *)data;

	if (m && !atomic_read(&num_notifiers))
		__print_mce(m);

	return NOTIFY_DONE;
}

Maybe put control of this into the hands of the notifiers ... if a notifier thinks that it does something useful with the log that makes the console log redundant, then it could call a function to bump the count to suppress the __print_mce(). Simple filter functions on the chain wouldn't do that.

If we go this path the variable should be named something like "suppress_console_mce" rather than num_notifiers.

Might also be useful to have some command line option or debugfs knob to force printing for those cases where bad stuff is happening and we don't see what was logged before a crash drops all the bits on the floor.

-Tony

Yes, it should be this way: if there are no consumers, all error information should go to dmesg so that it gets printed at least.

Probably but I don't think there's an example of an actual system where there are no other MCE consumers registered. Not if its users care about RAS. This default fallback was added for the hypothetical case anyway.

Before we go and fix whatever, we need to define what exactly we're fixing. Is there an actual system you're seeing this on or is this something that would never happen in reality? Because if the latter, I don't really care TBH. As in, there's more important stuff to take care of first.

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

I've encountered an issue where EDAC didn't load (either due to a bug or missing hardware enablement) and the MCE got swallowed by the mcelog notifier. The mcelog utility wasn't in use, so there was no record of the MCE. This can be considered a system configuration issue though that can be resolved with a bit of tweaking. But maybe we can find a solution to prevent something like this?

Thanks,
Yazen

Can you reproduce this using the injector?

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Yes, I was just able to do it. Here's what I did.

1) Unload EDAC decoder modules.
2) Inject a corrected MCE using mce-inject.
3) Observe "Machine check events logged" message and no other MCE info.
4) Manually load mcelog and find MCE info.

It seems to me that the issue is the mcelog notifier counts toward the number of notifiers, so the default notifier doesn't print anything.

Of course, this can be avoided if the EDAC modules are loaded, or if mcelog is disabled in the kernel config, or if rasdaemon is used, etc.

Thanks,
Yazen

If we gave a API to the notifiers to say whether to suppress printing, then the dev_mcelog() code could do the suppression only if some process had /dev/mcelog open. So if mcelog(8) wasn't running, you'd still see the console message.

-Tony

I briefly looked into that.

There is the issue that mcelog code buffers MCEs unconditionally. And we probably don't want to deactivate that, so that MCEs during boot can be queried a bit later via /dev/mcelog.

We would get a bit of duplicate logging, if we let mcelog report "supress printing" only if there is an actual consumer. (Or if there was a consumer once, in case there are periodically polling consumers.)

Regards
Jan

--

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879