Machine check recovery when kernel accesses poison

Copy the existing page fault fixup mechanisms to create a new table to be used when fixing machine checks. Note:
1) At this time we only provide a macro to annotate assembly code
2) We assume all fixups will in code builtin to the kernel.
3) Only for x86_64
4) New code under CONFIG_MCE_KERNEL_RECOVERY

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

---

arch/x86/mm/extable.c +2 -0

+#ifdef CONFIG_MCE_KERNEL_RECOVERY
+int fixup_mcexception(struct pt_regs *regs, u64 addr)

Copy the existing page fault fixup mechanisms to create a new table to be used when fixing machine checks. Note:
1) At this time we only provide a macro to annotate assembly code
2) We assume all fixups will in code builtin to the kernel.
3) Only for x86_64
4) New code under CONFIG_MCE_KERNEL_RECOVERY

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

---
 arch/x86/Kconfig                  |  4 ++++
 arch/x86/include/asm/asm.h        | 10 ++++++++--
 arch/x86/include/asm/uaccess.h    |  8 ++++++++
 arch/x86/mm/extable.c             | 19 +++++++++++++++++++
 include/asm-generic/vmlinux.lds.h |  6 ++++++
 include/linux/module.h            |  1 +
 kernel/extable.c                  | 20 ++++++++++++++++++++
 7 files changed, 66 insertions(+), 2 deletions(-)

arch/x86/Kconfig +0 -0

index 96d058a87100..db5c6e1d6e37 100644

arch/x86/Kconfig +1 -0

+
 config X86_THERMAL_VECTOR
 	def_bool y
 	depends on X86_MCE_INTEL

arch/x86/include/asm/asm.h +8 -2

index 189679aba703..a5d483ac11fa 100644
@@ -44,13 +44,19 @@
 
 /* Exception table entry */
 #ifdef __ASSEMBLY__
-# define _ASM_EXTABLE(from,to)					\
-	.pushsection "__ex_table","a" ;				\
+# define __ASM_EXTABLE(from, to, table)				\
+	.pushsection table, "a" ;				\
 	.balign 8 ;						\
 	.long (from) - . ;					\
 	.long (to) - . ;					\
 	.popsection
 
+# define _ASM_EXTABLE(from, to)					\
+	__ASM_EXTABLE(from, to, "__ex_table")
+
+# define _ASM_MCEXTABLE(from, to)				\
+	__ASM_EXTABLE(from, to, "__mcex_table")
+
 # define _ASM_EXTABLE_EX(from,to)				\
 	.pushsection "__ex_table","a" ;				\
 	.balign 8 ;						\

arch/x86/include/asm/uaccess.h +0 -0

index a8df874f3e88..7b02ca1991b4 100644
@@ -111,6 +111,14 @@ struct exception_table_entry {
 #define ARCH_HAS_SEARCH_EXTABLE
 

arch/x86/mm/extable.c +16 -0

+	const struct exception_table_entry *fixup;
+	unsigned long new_ip;
+
+	fixup = search_mcexception_tables(regs->ip);
+	if (fixup) {
+		new_ip = ex_fixup_addr(fixup);
+
+		regs->ip = new_ip;
+		regs->ax = BIT(63) | addr;
+		return 1;
+	}
+
+	return 0;
+}
+#endif
+
 /* Restricted version used during very early boot */
 int __init early_fixup_exception(unsigned long *ip)
 {

include/asm-generic/vmlinux.lds.h +0 -0

index 1781e54ea6d3..21bb20d1172a 100644

include/asm-generic/vmlinux.lds.h +0 -0

 	}
 
 /*

include/linux/module.h +1 -0

index 3a19c79918e0..ffecbfcc462c 100644
@@ -270,6 +270,7 @@ extern const typeof(name) __mod_##type##__##name##_device_table		\
 
 /* Given an address, look for it in the exception tables */
 const struct exception_table_entry *search_exception_tables(unsigned long add);
+const struct exception_table_entry *search_mcexception_tables(unsigned long a);
 
 struct notifier_block;
 

kernel/extable.c +9 -0

index e820ccee9846..7b224fbcb708 100644
@@ -34,6 +34,10 @@ DEFINE_MUTEX(text_mutex);
 
 extern struct exception_table_entry __start___ex_table[];
 extern struct exception_table_entry __stop___ex_table[];
+#ifdef CONFIG_MCE_KERNEL_RECOVERY
+extern struct exception_table_entry __start___mcex_table[];
+extern struct exception_table_entry __stop___mcex_table[];
+#endif
 
 /* Cleared by build time tools if the table is already sorted. */
 u32 __initdata __visible main_extable_sort_needed = 1;

@@ -45,6 +49,10 @@ void __init sort_main_extable(void)
 		pr_notice("Sorting __ex_table...\n");
 		sort_extable(__start___ex_table, __stop___ex_table);
 	}
+#ifdef CONFIG_MCE_KERNEL_RECOVERY
+	if (__stop___mcex_table > __start___mcex_table)
+		sort_extable(__start___mcex_table, __stop___mcex_table);
+#endif
 }
 
 /* Given an address, look for it in the exception tables. */

@@ -58,6 +66,18 @@ const struct exception_table_entry *search_exception_tables(unsigned long addr)
 	return e;
 }
 
+#ifdef CONFIG_MCE_KERNEL_RECOVERY

On Thu, Dec 10, 2015 at 01:58:04PM -0800, Tony Luck wrote:

kernel/extable.c const struct exception_table_entry *search_exception_tables(unsigned long addr) · line 79

Quoted 11 lines

kernel/extable.c +11 -0

+#ifdef CONFIG_MCE_KERNEL_RECOVERY
+/* Given an address, look for it in the machine check exception tables. */
+const struct exception_table_entry *search_mcexception_tables(
+                                 unsigned long addr)
+{
+     const struct exception_table_entry *e;
+
+     e = search_extable(__start___mcex_table, __stop___mcex_table-1, addr);
+     return e;
+}
+#endif

You can make this one a bit more readable by doing:

/* Given an address, look for it in the machine check exception tables. */ const struct exception_table_entry * search_mcexception_tables(unsigned long addr)
{

#ifdef CONFIG_MCE_KERNEL_RECOVERY
        return search_extable(__start___mcex_table,
                               __stop___mcex_table - 1, addr);

#endif
}

With this code if CONFIG_MCE_KERNEL_RECOVERY isn't defined you'll get a compiler error that the function doesn't have a return statement, right? I think we need an #else to return NULL, or to have the #ifdef encompass the whole function definition as it was in Tony's version.

arch/x86/Kconfig config X86_MCE_INJECT · line 1006

Quoted 3 lines

arch/x86/Kconfig +3 -0

+config MCE_KERNEL_RECOVERY
+	depends on X86_MCE && X86_64
+	def_bool y

Shouldn't that depend on NVDIMM or whatnot? Looks too generic now.

arch/x86/mm/extable.c int fixup_exception(struct pt_regs *regs) · line 54

Quoted 3 lines

arch/x86/mm/extable.c +3 -0

+#ifdef CONFIG_MCE_KERNEL_RECOVERY
+int fixup_mcexception(struct pt_regs *regs, u64 addr)
+{

If you move the #ifdef here, you can save yourself the ifdeffery in the header above.

Quoted 8 lines

include/asm-generic/vmlinux.lds.h +0 -0

index 1781e54ea6d3..21bb20d1172a 100644
@@ -473,6 +473,12 @@
 		VMLINUX_SYMBOL(__start___ex_table) = .;			\
 		*(__ex_table)						\
 		VMLINUX_SYMBOL(__stop___ex_table) = .;			\

include/asm-generic/vmlinux.lds.h line 481

Quoted 6 lines

include/asm-generic/vmlinux.lds.h +6 -0

+	}								\
+	. = ALIGN(align);						\
+	__mcex_table : AT(ADDR(__mcex_table) - LOAD_OFFSET) {		\
+		VMLINUX_SYMBOL(__start___mcex_table) = .;		\
+		*(__mcex_table)						\
+		VMLINUX_SYMBOL(__stop___mcex_table) = .;		\

Of all the places, this one is missing #ifdef CONFIG_MCE_KERNEL_RECOVERY.

You can make this one a bit more readable by doing:

/* Given an address, look for it in the machine check exception tables. */ const struct exception_table_entry * search_mcexception_tables(unsigned long addr)
{

#ifdef CONFIG_MCE_KERNEL_RECOVERY
        return search_extable(__start___mcex_table,
                               __stop___mcex_table - 1, addr);

#endif
}

Not sure what the "whatnot" would be though. Making it depend on X86_MCE should keep it out of the tiny configurations. By the time you have MCE support, this seems like a pretty small incremental change.

Is there some cpp magic to use an #ifdef inside a multi-line macro like this? Impact of not having the #ifdef is two extra symbols (the start/stop ones) in the symbol table of the final binary. If that's unacceptable I can fall back to an earlier unpublished version that had separate EXCEPTION_TABLE and MCEXCEPTION_TABLE macros with both invoked in the x86 vmlinux.lds.S file.

I think what is more important is that this should be in the x86-specific linker script, not in the generic one.

Extend the severity checking code to add a new context IN_KERN_RECOV which is used to indicate that the machine check was triggered by code in the kernel with a fixup entry.

Add code to check for this situation and respond by altering the return IP to the fixup address and changing the regs->ax so that the recovery code knows the physical address of the error. Note that we also set bit 63 because 0x0 is a legal physical address.

Major re-work to the tail code in do_machine_check() to make all this readable/maintainable. One functional change is that tolerant=3 no longer stops recovery actions. Revert to only skipping sending SIGBUS to the current process.

Extend the severity checking code to add a new context IN_KERN_RECOV which is used to indicate that the machine check was triggered by code in the kernel with a fixup entry.

Add code to check for this situation and respond by altering the return IP to the fixup address and changing the regs->ax so that the recovery code knows the physical address of the error. Note that we also set bit 63 because 0x0 is a legal physical address.

Major re-work to the tail code in do_machine_check() to make all this readable/maintainable. One functional change is that tolerant=3 no longer stops recovery actions. Revert to only skipping sending SIGBUS to the current process.

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

---
 arch/x86/kernel/cpu/mcheck/mce-severity.c | 22 +++++++++-
 arch/x86/kernel/cpu/mcheck/mce.c          | 69 ++++++++++++++++---------------
 2 files changed, 55 insertions(+), 36 deletions(-)

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

index 9c682c222071..ac7fbb0689fb 100644
@@ -12,6 +12,7 @@
 #include <linux/kernel.h>
 #include <linux/seq_file.h>
 #include <linux/init.h>
+#include <linux/module.h>
 #include <linux/debugfs.h>
 #include <asm/mce.h>
 

@@ -29,7 +30,7 @@
  * panic situations)
  */
 
-enum context { IN_KERNEL = 1, IN_USER = 2 };
+enum context { IN_KERNEL = 1, IN_USER = 2, IN_KERNEL_RECOV = 3 };
 enum ser { SER_REQUIRED = 1, NO_SER = 2 };
 enum exception { EXCP_CONTEXT = 1, NO_EXCP = 2 };
 

@@ -48,6 +49,7 @@ static struct severity {
 #define MCESEV(s, m, c...) { .sev = MCE_ ## s ## _SEVERITY, .msg = m, ## c }
 #define  KERNEL		.context = IN_KERNEL
 #define  USER		.context = IN_USER
+#define  KERNEL_RECOV	.context = IN_KERNEL_RECOV
 #define  SER		.ser = SER_REQUIRED
 #define  NOSER		.ser = NO_SER
 #define  EXCP		.excp = EXCP_CONTEXT

@@ -87,6 +89,10 @@ static struct severity {
 		EXCP, KERNEL, MCGMASK(MCG_STATUS_RIPV, 0)
 		),
 	MCESEV(
+		PANIC, "In kernel and no restart IP",
+		EXCP, KERNEL_RECOV, MCGMASK(MCG_STATUS_RIPV, 0)
+		),
+	MCESEV(
 		DEFERRED, "Deferred error",
 		NOSER, MASK(MCI_STATUS_UC|MCI_STATUS_DEFERRED|MCI_STATUS_POISON, MCI_STATUS_DEFERRED)
 		),

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

+		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR|MCI_ADDR|MCACOD, MCI_UC_SAR|MCI_ADDR|MCACOD_DATA),
+		KERNEL_RECOV
+		),
+	MCESEV(
 		AR, "Action required: data load error in a user process",
 		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR|MCI_ADDR|MCACOD, MCI_UC_SAR|MCI_ADDR|MCACOD_DATA),
 		USER

@@ -170,6 +181,9 @@ static struct severity {
 		)	/* always matches. keep at end */
 };
 
+#define mc_recoverable(mcg) (((mcg) & (MCG_STATUS_RIPV|MCG_STATUS_EIPV)) == \
+				(MCG_STATUS_RIPV|MCG_STATUS_EIPV))
+
 /*
  * If mcgstatus indicated that ip/cs on the stack were
  * no good, then "m->cs" will be zero and we will have

@@ -183,7 +197,11 @@ static struct severity {
  */
 static int error_context(struct mce *m)
 {
-	return ((m->cs & 3) == 3) ? IN_USER : IN_KERNEL;
+	if ((m->cs & 3) == 3)
+		return IN_USER;
+	if (mc_recoverable(m->mcgstatus) && search_mcexception_tables(m->ip))
+		return IN_KERNEL_RECOV;
+	return IN_KERNEL;
 }
 
 /*

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

index 9d014b82a124..f2f568ad6409 100644
@@ -31,6 +31,7 @@
 #include <linux/types.h>
 #include <linux/slab.h>
 #include <linux/init.h>
+#include <linux/module.h>
 #include <linux/kmod.h>
 #include <linux/poll.h>
 #include <linux/nmi.h>

@@ -958,6 +959,20 @@ static void mce_clear_state(unsigned long *toclear)
 	}
 }
 
+static int do_memory_failure(struct mce *m)
+{
+	int flags = MF_ACTION_REQUIRED;
+	int ret;
+
+	pr_err("Uncorrected hardware memory error in user-access at %llx", m->addr);
+	if (!(m->mcgstatus & MCG_STATUS_RIPV))
+		flags |= MF_MUST_KILL;
+	ret = memory_failure(m->addr >> PAGE_SHIFT, MCE_VECTOR, flags);
+	if (ret)
+		pr_err("Memory error not recovered");
+	return ret;
+}
+
 /*
  * The actual machine check handler. This only handles real
  * exceptions when something got corrupted coming in through int 18.

@@ -995,8 +1010,6 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 	DECLARE_BITMAP(toclear, MAX_NR_BANKS);
 	DECLARE_BITMAP(valid_banks, MAX_NR_BANKS);
 	char *msg = "Unknown";
-	u64 recover_paddr = ~0ull;
-	int flags = MF_ACTION_REQUIRED;
 	int lmce = 0;
 
 	ist_enter(regs);

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

+		kill_it = 0;
+	else if (no_way_out)
+		mce_panic("Fatal machine check on current CPU", &m, msg);
 
 	if (worst > 0)
 		mce_report_event(regs);

@@ -1146,25 +1150,22 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 out:
 	sync_core();
 
-	if (recover_paddr == ~0ull)
-		goto done;
+	/* Fault was in user mode and we need to take some action */
+	if ((m.cs & 3) == 3 && (worst == MCE_AR_SEVERITY || kill_it)) {
+		ist_begin_non_atomic(regs);
+		local_irq_enable();
 
-	pr_err("Uncorrected hardware memory error in user-access at %llx",
-		 recover_paddr);
-	/*
-	 * We must call memory_failure() here even if the current process is
-	 * doomed. We still need to mark the page as poisoned and alert any
-	 * other users of the page.
-	 */
-	ist_begin_non_atomic(regs);
-	local_irq_enable();
-	if (memory_failure(recover_paddr >> PAGE_SHIFT, MCE_VECTOR, flags) < 0) {
-		pr_err("Memory error not recovered");
-		force_sig(SIGBUS, current);
+		if (kill_it || do_memory_failure(&m))
+			force_sig(SIGBUS, current);
+		local_irq_disable();

Using __copy_user_nocache() as inspiration create a memory copy routine for use by kernel code with annotations to allow for recovery from machine checks.

Notes:
1) Unlike the original we make no attempt to copy all the bytes up to the faulting address. The original achieves that by re-executing the failing part as a byte-by-byte copy, which will take another page fault. We don't want to have a second machine check!
2) Likewise the return value for the original indicates exactly how many bytes were not copied. Instead we provide the physical address of the fault (thanks to help from do_machine_check()
3) Provide helpful macros to decode the return value.

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

---
 arch/x86/include/asm/uaccess_64.h |  5 +++
 arch/x86/kernel/x8664_ksyms_64.c  |  2 +
 arch/x86/lib/copy_user_64.S       | 91 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 98 insertions(+)

arch/x86/include/asm/uaccess_64.h +5 -0

index f2f9b39b274a..779cb0e77ecc 100644
@@ -216,6 +216,11 @@ __copy_to_user_inatomic(void __user *dst, const void *src, unsigned size)
 extern long __copy_user_nocache(void *dst, const void __user *src,
                                unsigned size, int zerorest);
 
+extern u64 mcsafe_memcpy(void *dst, const void __user *src,
+                               unsigned size);
+#define COPY_HAD_MCHECK(ret)   ((ret) & BIT(63))
+#define        COPY_MCHECK_PADDR(ret)  ((ret) & ~BIT(63))
+
 static inline int
 __copy_from_user_nocache(void *dst, const void __user *src, unsigned size)
 {

arch/x86/kernel/x8664_ksyms_64.c +2 -0

index a0695be19864..ec988c92c055 100644
@@ -37,6 +37,8 @@ EXPORT_SYMBOL(__copy_user_nocache);
 EXPORT_SYMBOL(_copy_from_user);
 EXPORT_SYMBOL(_copy_to_user);
 
+EXPORT_SYMBOL(mcsafe_memcpy);
+
 EXPORT_SYMBOL(copy_page);
 EXPORT_SYMBOL(clear_page);
 

arch/x86/lib/copy_user_64.S +81 -0

index 982ce34f4a9b..ffce93cbc9a5 100644
@@ -319,3 +319,94 @@ ENTRY(__copy_user_nocache)
        _ASM_EXTABLE(21b,50b)
        _ASM_EXTABLE(22b,50b)
 ENDPROC(__copy_user_nocache)
+
+/*
+ * mcsafe_memcpy - Uncached memory copy with machine check exception handling
+ * Note that we only catch machine checks when reading the source addresses.
+ * Writes to target are posted and don't generate machine checks.
+ * This will force destination/source out of cache for more performance.
+ */
+ENTRY(mcsafe_memcpy)
+       cmpl $8,%edx
+       jb 20f          /* less then 8 bytes, go to byte copy loop */
+
+       /* check for bad alignment of destination */
+       movl %edi,%ecx
+       andl $7,%ecx
+       jz 102f                         /* already aligned */
+       subl $8,%ecx
+       negl %ecx
+       subl %ecx,%edx
+0:     movb (%rsi),%al
+       movb %al,(%rdi)
+       incq %rsi
+       incq %rdi
+       decl %ecx
+       jnz 100b
+102:
+       movl %edx,%ecx
+       andl $63,%edx
+       shrl $6,%ecx
+       jz 17f
+1:     movq (%rsi),%r8
+2:     movq 1*8(%rsi),%r9
+3:     movq 2*8(%rsi),%r10
+4:     movq 3*8(%rsi),%r11
+       movnti %r8,(%rdi)
+       movnti %r9,1*8(%rdi)
+       movnti %r10,2*8(%rdi)
+       movnti %r11,3*8(%rdi)
+9:     movq 4*8(%rsi),%r8
+10:    movq 5*8(%rsi),%r9
+11:    movq 6*8(%rsi),%r10
+12:    movq 7*8(%rsi),%r11
+       movnti %r8,4*8(%rdi)
+       movnti %r9,5*8(%rdi)
+       movnti %r10,6*8(%rdi)
+       movnti %r11,7*8(%rdi)
+       leaq 64(%rsi),%rsi
+       leaq 64(%rdi),%rdi
+       decl %ecx
+       jnz 1b
+17:    movl %edx,%ecx
+       andl $7,%edx
+       shrl $3,%ecx
+       jz 20f
+18:    movq (%rsi),%r8
+       movnti %r8,(%rdi)
+       leaq 8(%rsi),%rsi
+       leaq 8(%rdi),%rdi
+       decl %ecx
+       jnz 18b
+20:    andl %edx,%edx
+       jz 23f
+       movl %edx,%ecx
+21:    movb (%rsi),%al
+       movb %al,(%rdi)
+       incq %rsi
+       incq %rdi
+       decl %ecx
+       jnz 21b
+23:    xorl %eax,%eax
+       sfence
+       ret
+
+       .section .fixup,"ax"
+30:
+       sfence
+       /* do_machine_check() sets %eax return value */
+       ret
+       .previous
+
+       _ASM_MCEXTABLE(0b,30b)
+       _ASM_MCEXTABLE(1b,30b)

Quoted 1 line

I still don't get the BIT(63) thing. Can you explain it?

It will be more obvious when I get around to writing copy_from_user().

Then we will have a function that can take page faults if there are pages that are not present. If the page faults can't be fixed we have a -EFAULT condition. We can also take machine checks if we reads from a location with an uncorrected error.

We need to distinguish these two cases because the action we take is different. For the unresolved page fault we already have the ABI that the copy_to/from_user() functions return zero for success, and a non-zero return is the number of not-copied bytes.

So for my new case I'm setting bit63 ... this is never going to be set for a failed page fault.

copy_from_user() conceptually will look like this:

int copy_from_user(void *to, void *from, unsigned long n)

{
	u64 ret = mcsafe_memcpy(to, from, n);

	if (COPY_HAD_MCHECK(r)) {
		if (memory_failure(COPY_MCHECK_PADDR(ret) >> PAGE_SIZE, ...))
			force_sig(SIGBUS, current);
		return something;
	} else
		return ret;
}

-Tony
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm

Quoted 1 line

I still don't get the BIT(63) thing. Can you explain it?

It will be more obvious when I get around to writing copy_from_user().

Then we will have a function that can take page faults if there are pages that are not present. If the page faults can't be fixed we have a -EFAULT condition. We can also take machine checks if we reads from a location with an uncorrected error.

We need to distinguish these two cases because the action we take is different. For the unresolved page fault we already have the ABI that the copy_to/from_user() functions return zero for success, and a non-zero return is the number of not-copied bytes.

I'm missing something, though. The normal fixup_exception path doesn't touch rax at all. The memory_failure path does. But couldn't you distinguish them by just pointing the exception handlers at different landing pads?

Also, would it be more straightforward if the mcexception landing pad looked up the va -> pa mapping by itself? Or is that somehow not reliable?

Quoted 3 lines

Also, would it be more straightforward if the mcexception landing pad looked up the va -> pa mapping by itself? Or is that somehow not reliable?

If we did get all the above right, then we could have target use virt_to_phys() to convert to physical ... I don't see that this part would be a problem.

Quoted 4 lines · Tony

Quoted 3 lines

Also, would it be more straightforward if the mcexception landing pad looked up the va -> pa mapping by itself? Or is that somehow not reliable?

Quoted 3 lines

If we did get all the above right, then we could have target use virt_to_phys() to convert to physical ... I don't see that this part would be a problem.

virt_to_phys() implies a linear address. In the case of the use in the pmem driver we'll be using an ioremap()'d address off somewherein vmalloc space.

Quoted 2 lines

Also, are there really PCOMMIT-capable CPUs that still forcibly broadcast MCE? If, so, that's unfortunate.

PCOMMIT and LMCE arrive together ... though BIOS is in the decision path to enable LMCE, so it is possible that some systems could still broadcast if the BIOS writer decides to not allow local.

But a machine check safe copy_from_user() would be useful current generation cpus that broadcast all the time.

Quoted 4 lines

Quoted 2 lines

But a machine check safe copy_from_user() would be useful current generation cpus that broadcast all the time.

Fair enough.

Thanks for spending the time to look at this. Coaxing me to re-write the tail of do_machine_check() has made that code much better. Too many years of one patch on top of another without looking at the whole context.

Cogitate on this series over the weekend and see if you can give me an Acked-by or Reviewed-by (I'll be adding a #define for BIT(63)).

I still think it would be better if you get rid of BIT(63) and use a pair of landing pads, though. They could be as simple as:

.Lpage_fault_goes_here:
xorq %rax, %rax
jmp .Lbad

.Lmce_goes_here:

    /* set high bit of rax or whatever */
    /* fall through */

.Lbad:

    /* deal with it */

That way the magic is isolated to the function that needs the magic.

Quoted 3 lines

    /* deal with it */

That way the magic is isolated to the function that needs the magic.

Seconded - this is the usual pattern we use in all assembly functions.

Quoted 5 lines · Ingo Molnar

Quoted 3 lines

    /* deal with it */

That way the magic is isolated to the function that needs the magic.

Seconded - this is the usual pattern we use in all assembly functions.

Ok - you want me to write some x86 assembly code (you may regret that).

Initial question ... here's the fixup for __copy_user_nocache()

                .section .fixup,"ax"
        30:     shll $6,%ecx
                addl %ecx,%edx
                jmp 60f
        40:     lea (%rdx,%rcx,8),%rdx
                jmp 60f
        50:     movl %ecx,%edx
        60:     sfence
                jmp copy_user_handle_tail
                .previous

Are %ecx and %rcx synonyms for the same register? Is there some super subtle reason we use the 'r' names in the "40" fixup, but the 'e' names everywhere else in this code (and the 'e' names in the body of the original function)?

Using __copy_user_nocache() as inspiration create a memory copy routine for use by kernel code with annotations to allow for recovery from machine checks.

Notes:
1) Unlike the original we make no attempt to copy all the bytes up to the faulting address. The original achieves that by re-executing the failing part as a byte-by-byte copy, which will take another page fault. We don't want to have a second machine check!
2) Likewise the return value for the original indicates exactly how many bytes were not copied. Instead we provide the physical address of the fault (thanks to help from do_machine_check()
3) Provide helpful macros to decode the return value.

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

---
 arch/x86/include/asm/uaccess_64.h |  5 +++
 arch/x86/kernel/x8664_ksyms_64.c  |  2 +
 arch/x86/lib/copy_user_64.S       | 91 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 98 insertions(+)

+ * mcsafe_memcpy - Uncached memory copy with machine check exception handling
+ * Note that we only catch machine checks when reading the source addresses.
+ * Writes to target are posted and don't generate machine checks.
+ * This will force destination/source out of cache for more performance.

Quoted 21 lines · Tony Luck

Using __copy_user_nocache() as inspiration create a memory copy routine for use by kernel code with annotations to allow for recovery from machine checks.

Notes:
1) Unlike the original we make no attempt to copy all the bytes up to the faulting address. The original achieves that by re-executing the failing part as a byte-by-byte copy, which will take another page fault. We don't want to have a second machine check!
2) Likewise the return value for the original indicates exactly how many bytes were not copied. Instead we provide the physical address of the fault (thanks to help from do_machine_check()
3) Provide helpful macros to decode the return value.

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

---
 arch/x86/include/asm/uaccess_64.h |  5 +++
 arch/x86/kernel/x8664_ksyms_64.c  |  2 +
 arch/x86/lib/copy_user_64.S       | 91 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 98 insertions(+)

...

Quoted 4 lines

+ * mcsafe_memcpy - Uncached memory copy with machine check exception handling
+ * Note that we only catch machine checks when reading the source addresses.
+ * Writes to target are posted and don't generate machine checks.
+ * This will force destination/source out of cache for more performance.

... and the non-temporal version is the optimal one even though we're defaulting to copy_user_enhanced_fast_string for memcpy on modern Intel CPUs...?

My current generation cpu has a bit of an issue with recovering from a machine check in a "rep mov" ... so I'm working with a version of memcpy that unrolls into individual mov instructions for now.

I can drop the "nti" from the destination moves. Does "nti" work on the load from source address side to avoid cache allocation?

On another topic raised by Boris ... is there some CONFIG_PMEM* that I should use as a dependency to enable all this?

Quoted 3 lines

Quoted 3 lines

... and the non-temporal version is the optimal one even though we're defaulting to copy_user_enhanced_fast_string for memcpy on modern Intel CPUs...?

My current generation cpu has a bit of an issue with recovering from a machine check in a "rep mov" ... so I'm working with a version of memcpy that unrolls into individual mov instructions for now.

Quoted 6 lines

At least the pmem driver use case does not want caching of the source-buffer since that is the raw "disk" media. I.e. in pmem_do_bvec() we'd use this to implement memcpy_from_pmem(). However, caching the destination-buffer may prove beneficial since that data is likely to be consumed immediately by the thread that submitted the i/o.

I can drop the "nti" from the destination moves. Does "nti" work on the load from source address side to avoid cache allocation?

On another topic raised by Boris ... is there some CONFIG_PMEM* that I should use as a dependency to enable all this?

Quoted 15 lines · Tony

Quoted 3 lines

Quoted 3 lines

... and the non-temporal version is the optimal one even though we're defaulting to copy_user_enhanced_fast_string for memcpy on modern Intel CPUs...?

My current generation cpu has a bit of an issue with recovering from a machine check in a "rep mov" ... so I'm working with a version of memcpy that unrolls into individual mov instructions for now.

Quoted 6 lines

At least the pmem driver use case does not want caching of the source-buffer since that is the raw "disk" media. I.e. in pmem_do_bvec() we'd use this to implement memcpy_from_pmem(). However, caching the destination-buffer may prove beneficial since that data is likely to be consumed immediately by the thread that submitted the i/o.

Quoted 2 lines

I can drop the "nti" from the destination moves. Does "nti" work on the load from source address side to avoid cache allocation?

My mistake, I don't think we have an uncached load capability, only store.

Correction we have MOVNTDQA, but that requires saving the fpu state and marking the memory as WC, i.e. probably not worth it.

-----Original Message-----
From: Linux-nvdimm [mailto:[email protected]] On Behalf Of Borislav Petkov
Sent: Tuesday, December 15, 2015 12:39 PM
To: Dan Williams <[email protected]>
Cc: Luck, Tony <[email protected]>; linux-nvdimm <linux- [email protected]>; X86 ML <[email protected]>; linux- [email protected]; Linux MM <[email protected]>; Andy Lutomirski <[email protected]>; Andrew Morton <[email protected]>; Ingo Molnar <[email protected]>
Subject: Re: [PATCHV2 3/3] x86, ras: Add mcsafe_memcpy() function to recover from machine checks

Quoted 2 lines · Dan Williams

Correction we have MOVNTDQA, but that requires saving the fpu state and marking the memory as WC, i.e. probably not worth it.

Not really. Last time I tried an SSE3 memcpy in the kernel like glibc does, it wasn't worth it. The enhanced REP; MOVSB is hands down faster.

Correction we have MOVNTDQA, but that requires
saving the fpu state and marking the memory as WC
i.e. probably not worth it.

Due to the historic long latency of storage devices, applications don't re-read from storage again; they save the results.
So, the streaming-load instructions are beneficial:

This isn't exactly what you're looking for, but here is an example of fio doing reads from pmem devices (reading from NVDIMMs, writing to DIMMs) with various transfer sizes.

At 256 KiB, all the main memory buffers fit in the CPU caches, so no write traffic appears on DDR (just the reads from the NVDIMMs). At 1 MiB, the data spills out of the caches, and writes to the DIMMs end up on DDR.

Although DDR is busier, fio gets a lot less work done:
* 256 KiB: 90 GiB/s by fio
* 1 MiB: 49 GiB/s by fio

We could try modifying pmem to use its own non-temporal memcpy functions (I've posted experimental patches before that did this) to see if that transition point shifts. We can also watch the CPU cache statistics while running.

Here are statistics from Intel's pcm-memory.x
(pardon the wide formatting):

256 KiB
=======
pmem0: (groupid=0, jobs=40): err= 0: pid=20867: Tue Nov 24 18:20:08 2015
read : io=5219.1GB, bw=89079MB/s, iops=356314, runt= 60006msec
cpu : usr=1.74%, sys=96.16%, ctx=49576, majf=0, minf=21997

Run status group 0 (all jobs):

   READ: io=5219.1GB, aggrb=89079MB/s, minb=89079MB/s, maxb=89079MB/s, mint=60006msec, maxt=60006msec

|---------------------------------------||---------------------------------------|
|--             Socket  0             --||--             Socket  1             --|
|---------------------------------------||---------------------------------------|
|--     Memory Channel Monitoring     --||--     Memory Channel Monitoring     --|
|---------------------------------------||---------------------------------------|
|-- Mem Ch  0: Reads (MB/s): 11778.11 --||-- Mem Ch  0: Reads (MB/s): 11743.99 --|
|--            Writes(MB/s):    51.83 --||--            Writes(MB/s):    43.25 --|
|-- Mem Ch  1: Reads (MB/s): 11779.90 --||-- Mem Ch  1: Reads (MB/s): 11736.06 --|
|--            Writes(MB/s):    48.73 --||--            Writes(MB/s):    37.86 --|
|-- Mem Ch  4: Reads (MB/s): 11784.79 --||-- Mem Ch  4: Reads (MB/s): 11746.94 --|
|--            Writes(MB/s):    52.90 --||--            Writes(MB/s):    43.73 --|
|-- Mem Ch  5: Reads (MB/s): 11778.48 --||-- Mem Ch  5: Reads (MB/s): 11741.55 --|
|--            Writes(MB/s):    47.62 --||--            Writes(MB/s):    37.80 --|
|-- NODE 0 Mem Read (MB/s) : 47121.27 --||-- NODE 1 Mem Read (MB/s) : 46968.53 --|
|-- NODE 0 Mem Write(MB/s) :   201.08 --||-- NODE 1 Mem Write(MB/s) :   162.65 --|
|-- NODE 0 P. Write (T/s):     190927 --||-- NODE 1 P. Write (T/s):     182961 --|

|-- NODE 0 Memory (MB/s):    47322.36 --||-- NODE 1 Memory (MB/s):    47131.17 --|
|---------------------------------------||---------------------------------------|
|---------------------------------------||---------------------------------------|
|--                   System Read Throughput(MB/s):  94089.80                  --|
|--                  System Write Throughput(MB/s):    363.73                  --|
|--                 System Memory Throughput(MB/s):  94453.52                  --|
|---------------------------------------||---------------------------------------|

1 MiB
=====

|---------------------------------------||---------------------------------------|
|--             Socket  0             --||--             Socket  1             --|
|---------------------------------------||---------------------------------------|
|--     Memory Channel Monitoring     --||--     Memory Channel Monitoring     --|
|---------------------------------------||---------------------------------------|
|-- Mem Ch  0: Reads (MB/s):  7227.83 --||-- Mem Ch  0: Reads (MB/s):  7047.45 --|
|--            Writes(MB/s):  5894.47 --||--            Writes(MB/s):  6010.66 --|
|-- Mem Ch  1: Reads (MB/s):  7229.32 --||-- Mem Ch  1: Reads (MB/s):  7041.79 --|
|--            Writes(MB/s):  5891.38 --||--            Writes(MB/s):  6003.19 --|
|-- Mem Ch  4: Reads (MB/s):  7230.70 --||-- Mem Ch  4: Reads (MB/s):  7052.44 --|
|--            Writes(MB/s):  5888.63 --||--            Writes(MB/s):  6012.49 --|
|-- Mem Ch  5: Reads (MB/s):  7229.16 --||-- Mem Ch  5: Reads (MB/s):  7047.19 --|
|--            Writes(MB/s):  5882.45 --||--            Writes(MB/s):  6008.11 --|
|-- NODE 0 Mem Read (MB/s) : 28917.01 --||-- NODE 1 Mem Read (MB/s) : 28188.87 --|
|-- NODE 0 Mem Write(MB/s) : 23556.93 --||-- NODE 1 Mem Write(MB/s) : 24034.46 --|

|-- NODE 0 P. Write (T/s):     238713 --||-- NODE 1 P. Write (T/s):     228040 --|
|-- NODE 0 Memory (MB/s):    52473.94 --||-- NODE 1 Memory (MB/s):    52223.33 --|
|---------------------------------------||---------------------------------------|
|---------------------------------------||---------------------------------------|
|--                   System Read Throughput(MB/s):  57105.87                  --|
|--                  System Write Throughput(MB/s):  47591.39                  --|
|--                 System Memory Throughput(MB/s): 104697.27                  --|
|---------------------------------------||---------------------------------------|