x86, mce: machine check recovery for applications

There is only one caller of memory_failure(), all other users call __memory_failure() and pass in the flags argument explicitly. The lone user of memory_failure() will soon need to pass flags too.

Add flags argument to the callsite in mce.c. Delete the old memory_failure() function, and then rename __memory_failure() without the leading "__".

Signed-off-by Tony Luck <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c |    9 +++++--
 drivers/base/memory.c            |    2 +-
 include/linux/mm.h               |    3 +-
 mm/hwpoison-inject.c             |    4 +-
 mm/madvise.c                     |    2 +-
 mm/memory-failure.c              |   46 +++++++++++++++++--------------------
 6 files changed, 32 insertions(+), 34 deletions(-)

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

index 2af127d..265139d 100644

arch/x86/kernel/cpu/mcheck/mce.c out: · line 1052

Quoted 6 lines

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

-/* dummy to break dependency. actual code is in mm/memory-failure.c */
-void __attribute__((weak)) memory_failure(unsigned long pfn, int vector)
+#ifndef CONFIG_MEMORY_FAILURE
+int memory_failure(unsigned long pfn, int vector, int flags)
 {
 	printk(KERN_ERR "Action optional memory failure at %lx ignored\n", pfn);

Btw., while at it, could we phrase this message in a more obvious way to users, such as 'Non-fatal memory failure at %lx ignored'?

Quoted 10 lines · Ingo Molnar

arch/x86/kernel/cpu/mcheck/mce.c out: · line 1052

Quoted 6 lines

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

-/* dummy to break dependency. actual code is in mm/memory-failure.c */
-void __attribute__((weak)) memory_failure(unsigned long pfn, int vector)
+#ifndef CONFIG_MEMORY_FAILURE
+int memory_failure(unsigned long pfn, int vector, int flags)
 {
 	printk(KERN_ERR "Action optional memory failure at %lx ignored\n", pfn);

Btw., while at it, could we phrase this message in a more obvious way to users, such as 'Non-fatal memory failure at %lx ignored'?

Yeah, that's might not be as correct as we want it to be. AO means it is an uncorrectable error, i.e. it will become fatal if we'd consumed it, but it isn't that now because we just saw it passing by in the cacheline...

Maybe "Fatal, unconsumed error ignored..."

Quoted 5 lines

Quoted 1 line

printk(KERN_ERR "Action optional memory failure at %lx ignored\n", pfn);

Btw., while at it, could we phrase this message in a more obvious way to users, such as 'Non-fatal memory failure at %lx ignored'?

Yeah, that's might not be as correct as we want it to be. AO means it is an uncorrectable error, i.e. it will become fatal if we'd consumed it, but it isn't that now because we just saw it passing by in the cacheline...

Maybe "Fatal, unconsumed error ignored..."

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

printk(KERN_ERR "Action optional memory failure at %lx ignored\n", pfn);

Btw., while at it, could we phrase this message in a more obvious way to users, such as 'Non-fatal memory failure at %lx ignored'?

Quoted 6 lines

Yeah, that's might not be as correct as we want it to be. AO means it is an uncorrectable error, i.e. it will become fatal if we'd consumed it, but it isn't that now because we just saw it passing by in the cacheline...

Maybe "Fatal, unconsumed error ignored..."

The overall meaning is "land mine seen but not stepped on yet"

Quoted 1 line

The overall meaning is "land mine seen but not stepped on yet"

Perfect message!

Quoted 3 lines · Ingo Molnar

Quoted 1 line

The overall meaning is "land mine seen but not stepped on yet"

Perfect message!

How about:

Uncorrected memory error in page 0x%lx ignored
Rebuild kernel with CONFIG_MEMORY_FAILURE=y for smarter handling

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

index 265139d..43f22c8 100644
@@ -492,6 +492,27 @@ static void mce_report_event(struct pt_regs *regs)
 	irq_work_queue(&__get_cpu_var(mce_irq_work));
 }
 
+/*
+ * Read ADDR and MISC registers.
+ */
+static void mce_read_aux(struct mce *m, int i)
+{

Machine checks on Intel cpus interrupt execution on all cpus, regardless of interrupt masking. We have a need to save some data about the cause of the machine check (physical address) in the machine check handler that can be retrieved later to attempt recovery in a more flexible execution state.

Signed-off-by Tony Luck <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c |   51 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)

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

index 43f22c8..9b83b7d 100644

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

+
+	mce_panic("Too many concurrent recoverable errors", NULL, NULL);
+}
+
+static int mce_find_info(__u64 *paddr)
+{
+	int	i;
+
+	for (i = 0; i < MAX_MCE_INFO; i++)
+		if (atomic_read(&mce_info[i].inuse) &&
+		    mce_info[i].t == current) {
+			*paddr = mce_info[i].paddr;
+			return 1;
+		}

All non-urgent actions (reporting low severity errors and handling "action-optional" errors) are now handled by a work queue. This means that TIF_MCE_NOTIFY can be used to block execution for a thread experiencing an "action-required" fault until we get all cpus out of the machine check handler (and the thread that hit the fault into mce_notify_process().

We use the new mce_{save,find,clear}_info() API to get information from do_machine_check() to mce_notify_process(), and then use the newly improved memory_failure(..., MF_ACTION_REQUIRED) to handle the error (possibly signalling the process).

Signed-off-by Tony Luck<[email protected]>

---
  arch/x86/kernel/cpu/mcheck/mce.c |   64 ++++++++++++++++++++++---------------
  1 files changed, 38 insertions(+), 26 deletions(-)

arch/x86/kernel/cpu/mcheck/mce.c +3 -7

index 9b83b7d..66e3bfb 100644
@@ -1044,12 +1044,6 @@ void do_machine_check(struct pt_regs *regs, long error_code)
  			continue;
  		}
 
-		/*
-		 * Kill on action required.
-		 */
-		if (severity == MCE_AR_SEVERITY)
-			kill_it = 1;
-
  		mce_read_aux(&m, i);
 
  		/*

@@ -1070,6 +1064,8 @@ void do_machine_check(struct pt_regs *regs, long error_code)
  		}
  	}
 
+	m = *final;
+
  	if (!no_way_out)
  		mce_clear_state(toclear);
 

@@ -1088,7 +1084,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
  	 * support MCE broadcasting or it has been disabled.
  	 */
  	if (no_way_out&&  tolerant<  3)
-		mce_panic("Fatal machine check on current CPU", final, msg);
+		mce_panic("Fatal machine check on current CPU",&m, msg);
 
  	/*
  	 * If the error seems to be unrecoverable, something should be

arch/x86/kernel/cpu/mcheck/mce.c +5 -9

+	}
 
  	if (worst>  0)
  		mce_report_event(regs);

@@ -1115,34 +1113,50 @@ EXPORT_SYMBOL_GPL(do_machine_check);
  #ifndef CONFIG_MEMORY_FAILURE
  int memory_failure(unsigned long pfn, int vector, int flags)
  {
-	printk(KERN_ERR "Action optional memory failure at %lx ignored\n", pfn);
+	if (flags&  MF_ACTION_REQUIRED)
+		return -ENXIO; /* panic? */
+	else
+		printk(KERN_ERR "Action optional memory failure at %lx ignored\n", pfn);
 
  	return 0;
  }
  #endif
 
  /*
- * Called after mce notification in process context. This code
- * is allowed to sleep. Call the high level VM handler to process
- * any corrupted pages.
- * Assume that the work queue code only calls this one at a time
- * per CPU.
- * Note we don't disable preemption, so this code might run on the wrong
- * CPU. In this case the event is picked up by the scheduled work queue.
- * This is merely a fast path to expedite processing in some common

arch/x86/kernel/cpu/mcheck/mce.c +7 -3

  	unsigned long pfn;
-	mce_notify_irq();
-	while (mce_ring_get(&pfn))
-		memory_failure(pfn, MCE_VECTOR, 0);
+
+	if (!mce_find_info(&paddr))
+		mce_panic("Lost address", NULL, NULL);
+	pfn = paddr>>  PAGE_SHIFT;
+
+	clear_thread_flag(TIF_MCE_NOTIFY);
+

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

  static void mce_process_work(struct work_struct *dummy)
  {
-	mce_notify_process();
+	unsigned long pfn;
+
+	while (mce_ring_get(&pfn))
+		memory_failure(pfn, MCE_VECTOR, 0);
  }
 
  #ifdef CONFIG_X86_MCE_INTEL

@@ -1232,8 +1246,6 @@ int mce_notify_irq(void)
  	/* Not more than two messages every minute */
  	static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2);
 
-	clear_thread_flag(TIF_MCE_NOTIFY);
-
  	if (test_and_clear_bit(0,&mce_need_notify)) {
  		/* wake processes polling /dev/mcelog */
  		wake_up_interruptible(&mce_chrdev_wait);

Quoted 4 lines

-       if (kill_it&&  tolerant<  3)

+       if (worst != MCE_AR_SEVERITY&&  kill_it&&  tolerant<  3) force_sig(SIGBUS, current);

I think here it should add more comments to clarify why not killing *AR* case.
Such as: "for SRAR errors, such as DCU/IFU error, on affected logical processors, it is reasonable that RIPV is 0."

Quoted 3 lines

-       /* notify userspace ASAP */
-       set_thread_flag(TIF_MCE_NOTIFY);
+       if (worst == MCE_AR_SEVERITY) {

how about adding one more condition check: mce_usable_address(&m) here?

Quoted 2 lines

+               mce_save_info(m.addr);
+               set_thread_flag(TIF_MCE_NOTIFY);

Here only SRAR error are flagged with TIF_MCE_NOTIFY, which means only SRAR error is handled in the function do_notify_resume. If so, SRAO error will only be handled in work_queue mce_work. If so, I think some related function names should be updated too. Otherwise, it will confuse people not touching these codes before.

Quoted 3 lines

 void mce_notify_process(void)
 {
+       __u64   paddr = paddr;

you mean "__u64 paddr = 0;"?

Does there exist some possibility that in the same process there are more than
one error triggered? If so, maybe mce_find_info/mce_clear_info should be changed
to loop-style, because here TIF_MCE_NOTIFY is cleared in the handler.

Or it is impossible because overwritten will be covered by following condition:

Quoted 10 lines · Chen Gong

arch/x86/kernel/cpu/mcheck/mce.c void do_machine_check(struct pt_regs *regs, long error_code) · line 1097

Quoted 4 lines

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

-       if (kill_it&&    tolerant<    3)
 
+       if (worst != MCE_AR_SEVERITY&&    kill_it&&    tolerant<    3)
                force_sig(SIGBUS, current);

I think here it should add more comments to clarify why not killing *AR* case.
Such as: "for SRAR errors, such as DCU/IFU error, on affected logical processors, it is reasonable that RIPV is 0."

I'll look at this - the reason to not kill for AR is that we want to try to recover
first (e.g. page could be re-read from disk into a different physical page). In some cases we can recover transparently to the application.

- * Called after mce notification in process context. This code
- * is allowed to sleep. Call the high level VM handler to process
- * any corrupted pages.
- * Assume that the work queue code only calls this one at a time
- * per CPU.
- * Note we don't disable preemption, so this code might run on the wrong
- * CPU. In this case the event is picked up by the scheduled work queue.
- * This is merely a fast path to expedite processing in some common
- * cases.
+ * Called in process context that interrupted by MCE and marked with
+ * TIF_MCE_NOTFY, just before returning to errorneous userland.
+ * This code is allowed to sleep.
+ * Attempt possible recovery such as calling the high level VM handler to
+ * process any corrupted pages, and kill/signal current process if required. */
void mce_notify_process(void)
{

arch/x86/kernel/cpu/mcheck/mce.c +5 -3

+	}
 }
 
 static void mce_process_work(struct work_struct *dummy)
 {
-	mce_notify_process();
+	unsigned long pfn;
+
+	while (mce_ring_get(&pfn))
+		memory_failure(pfn, MCE_VECTOR, 0);
 }
 
 #ifdef CONFIG_X86_MCE_INTEL

@@ -1232,8 +1246,6 @@ int mce_notify_irq(void)
 	/* Not more than two messages every minute */
 	static DEFINE_RATELIMIT_STATE(ratelimit, 60*HZ, 2);
 
-	clear_thread_flag(TIF_MCE_NOTIFY);
-
 	if (test_and_clear_bit(0, &mce_need_notify)) {
 		/* wake processes polling /dev/mcelog */
 		wake_up_interruptible(&mce_chrdev_wait);

Signature · 1 line

1.7.3.1

arch/x86/kernel/cpu/mcheck/mce.c EXPORT_SYMBOL_GPL(do_machine_check); · line 1138

Quoted 2 lines

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

+	if (!mce_find_info(&paddr))
+		mce_panic("Lost address", NULL, NULL);

Wouldn't it be good to return struct mce_info *mi here in addition to &paddr...

so that you don't need to iterate again over the mce_info array but do:

	mce_clear_info(mi);

This assumes, of course, that you have only one AR MCE per task, per return to userspace. I guess this is fine for now.

Action required data path signature is defined in table 15-19 of SDM:

+-----------------------------------------------------------------------------+
| SRAR Error | Valid | OVER | UC | EN | MISCV | ADDRV | PCC | S | AR | MCACOD |
| Data Load  |     1 |    0 |  1 |  1 |     1 |     1 |   0 | 1 |  1 |  0x134 |
+-----------------------------------------------------------------------------+

Recognise this, and pass MCE_AR_SEVERITY code back to do_machine_check()

Signed-off-by Tony Luck <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce-severity.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

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

index 7395d5f..c4d8b24 100644
@@ -54,6 +54,7 @@ static struct severity {
 #define  MASK(x, y)	.mask = x, .result = y
 #define MCI_UC_S (MCI_STATUS_UC|MCI_STATUS_S)
 #define MCI_UC_SAR (MCI_STATUS_UC|MCI_STATUS_S|MCI_STATUS_AR)
+#define	MCI_ADDR (MCI_STATUS_ADDRV|MCI_STATUS_MISCV)
 #define MCACOD 0xffff
 
 	MCESEV(

@@ -102,11 +103,22 @@ static struct severity {
 		SER, BITCLR(MCI_STATUS_S)
 		),
 
-	/* AR add known MCACODs here */

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

+		),
+	MCESEV(
+		AR, "Action required: data load error",
+		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR|MCI_ADDR|MCACOD, MCI_UC_SAR|MCI_ADDR|0x0134),
+		USER
+		),
 	MCESEV(
 		PANIC, "Action required: unknown MCACOD",
 		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR, MCI_UC_SAR)

Signature · 1 line

1.7.3.1