Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1336630 > unrolled thread

[PATCH v11 0/4] Machine check recovery when kernel accesses poison

Back to article view | Back to linux.kernel

Contents

  [PATCH v11 0/4] Machine check recovery when kernel accesses poison Tony Luck <tony.luck@intel.com> - 2016-02-17 19:30 +0100
    [PATCH v11 4/4] x86: Create a new synthetic cpu capability for  machine check recovery Tony Luck <tony.luck@intel.com> - 2016-02-17 19:30 +0100
      [tip:x86/asm] x86/cpufeature:   Create a new synthetic cpu capability for machine check recovery tip-bot for Tony Luck <tipbot@zytor.com> - 2016-02-18 11:30 +0100
    [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Tony Luck <tony.luck@intel.com> - 2016-02-17 19:30 +0100
      Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Ingo Molnar <mingo@kernel.org> - 2016-02-18 09:30 +0100
        Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Peter Zijlstra <peterz@infradead.org> - 2016-02-18 11:00 +0100
          Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Borislav Petkov <bp@alien8.de> - 2016-02-18 11:30 +0100
          Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Ingo Molnar <mingo@kernel.org> - 2016-02-18 11:30 +0100
            Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Peter Zijlstra <peterz@infradead.org> - 2016-02-18 11:40 +0100
              RE: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() "Luck, Tony" <tony.luck@intel.com> - 2016-02-18 16:00 +0100
          Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Ingo Molnar <mingo@kernel.org> - 2016-02-19 09:00 +0100
            Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Peter Zijlstra <peterz@infradead.org> - 2016-02-19 09:50 +0100
              Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Ingo Molnar <mingo@kernel.org> - 2016-02-19 11:00 +0100
        Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Ingo Molnar <mingo@kernel.org> - 2016-02-18 11:40 +0100
          Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Borislav Petkov <bp@alien8.de> - 2016-02-18 11:40 +0100
            Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Ingo Molnar <mingo@kernel.org> - 2016-02-18 19:50 +0100
        Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Borislav Petkov <bp@alien8.de> - 2016-02-18 11:40 +0100
        [PATCH v12] x86, mce: Add memcpy_trap() "Luck, Tony" <tony.luck@intel.com> - 2016-02-18 22:20 +0100
          Re: [PATCH v12] x86, mce: Add memcpy_trap() Ingo Molnar <mingo@kernel.org> - 2016-02-19 10:20 +0100
            Re: [PATCH v13] x86, mce: Add memcpy_trap() "Luck, Tony" <tony.luck@intel.com> - 2016-02-19 19:00 +0100
      Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-18 19:20 +0100
        Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() "Luck, Tony" <tony.luck@intel.com> - 2016-02-18 20:00 +0100
          Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Ingo Molnar <mingo@kernel.org> - 2016-02-18 21:20 +0100
            Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Dan Williams <dan.j.williams@intel.com> - 2016-02-18 22:40 +0100
        Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy() Ingo Molnar <mingo@kernel.org> - 2016-02-18 20:00 +0100

Page 1 of 2  [1] 2  Next page →

#1336630 — [PATCH v11 0/4] Machine check recovery when kernel accesses poison

[Resend of v11 with Boris' "Reviewed-by" tags added. For Ingo's workflow]

-Tony

Tony Luck (4):
  x86: Expand exception table to allow new handling options
  x86, mce: Check for faults tagged in EXTABLE_CLASS_FAULT exception
    table entries
  x86, mce: Add __mcsafe_copy()
  x86: Create a new synthetic cpu capability for machine check recovery

 Documentation/x86/exception-tables.txt    |  35 +++++++
 Documentation/x86/x86_64/boot-options.txt |   2 +
 arch/x86/include/asm/asm.h                |  40 ++++----
 arch/x86/include/asm/cpufeature.h         |   1 +
 arch/x86/include/asm/mce.h                |   1 +
 arch/x86/include/asm/string_64.h          |   8 ++
 arch/x86/include/asm/uaccess.h            |  16 ++--
 arch/x86/kernel/cpu/mcheck/mce-severity.c |  22 ++++-
 arch/x86/kernel/cpu/mcheck/mce.c          |  83 +++++++++-------
 arch/x86/kernel/kprobes/core.c            |   2 +-
 arch/x86/kernel/traps.c                   |   6 +-
 arch/x86/kernel/x8664_ksyms_64.c          |   2 +
 arch/x86/lib/memcpy_64.S                  | 151 ++++++++++++++++++++++++++++++
 arch/x86/mm/extable.c                     | 100 ++++++++++++++------
 arch/x86/mm/fault.c                       |   2 +-
 scripts/sortextable.c                     |  32 +++++++
 16 files changed, 410 insertions(+), 93 deletions(-)

-- 
2.5.0

[toc] | [next] | [standalone]

#1336631 — [PATCH v11 4/4] x86: Create a new synthetic cpu capability for machine check recovery

The Intel Software Developer Manual describes bit 24 in the MCG_CAP
MSR:
   MCG_SER_P (software error recovery support present) flag,
   bit 24 — Indicates (when set) that the processor supports
   software error recovery
But only some models with this capability bit set will actually
generate recoverable machine checks.

Check the model name and set a synthetic capability bit. Provide
a command line option to set this bit anyway in case the kernel
doesn't recognise the model name.

Reviewed-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 Documentation/x86/x86_64/boot-options.txt |  2 ++
 arch/x86/include/asm/cpufeature.h         |  1 +
 arch/x86/include/asm/mce.h                |  1 +
 arch/x86/kernel/cpu/mcheck/mce.c          | 13 +++++++++++++
 4 files changed, 17 insertions(+)

diff --git a/Documentation/x86/x86_64/boot-options.txt b/Documentation/x86/x86_64/boot-options.txt
index 68ed3114c363..0965a71f9942 100644
--- a/Documentation/x86/x86_64/boot-options.txt
+++ b/Documentation/x86/x86_64/boot-options.txt
@@ -60,6 +60,8 @@ Machine check
 		threshold to 1. Enabling this may make memory predictive failure
 		analysis less effective if the bios sets thresholds for memory
 		errors since we will not see details for all errors.
+   mce=recovery
+		Force-enable recoverable machine check code paths
 
    nomce (for compatibility with i386): same as mce=off
 
diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 7ad8c9464297..06c6c2d2fea0 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -106,6 +106,7 @@
 #define X86_FEATURE_APERFMPERF	( 3*32+28) /* APERFMPERF */
 #define X86_FEATURE_EAGER_FPU	( 3*32+29) /* "eagerfpu" Non lazy FPU restore */
 #define X86_FEATURE_NONSTOP_TSC_S3 ( 3*32+30) /* TSC doesn't stop in S3 state */
+#define X86_FEATURE_MCE_RECOVERY ( 3*32+31) /* cpu has recoverable machine checks */
 
 /* Intel-defined CPU features, CPUID level 0x00000001 (ecx), word 4 */
 #define X86_FEATURE_XMM3	( 4*32+ 0) /* "pni" SSE-3 */
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 2ea4527e462f..18d2ba9c8e44 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -113,6 +113,7 @@ struct mca_config {
 	bool ignore_ce;
 	bool disabled;
 	bool ser;
+	bool recovery;
 	bool bios_cmci_threshold;
 	u8 banks;
 	s8 bootlog;
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 905f3070f412..15ff6f07bd92 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1578,6 +1578,17 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
 
 		if (c->x86 == 6 && c->x86_model == 45)
 			quirk_no_way_out = quirk_sandybridge_ifu;
+		/*
+		 * MCG_CAP.MCG_SER_P is necessary but not sufficient to know
+		 * whether this processor will actually generate recoverable
+		 * machine checks. Check to see if this is an E7 model Xeon.
+		 * We can't do a model number check because E5 and E7 use the
+		 * same model number. E5 doesn't support recovery, E7 does.
+		 */
+		if (mca_cfg.recovery || (mca_cfg.ser &&
+			!strncmp(c->x86_model_id,
+				 "Intel(R) Xeon(R) CPU E7-", 24)))
+			set_cpu_cap(c, X86_FEATURE_MCE_RECOVERY);
 	}
 	if (cfg->monarch_timeout < 0)
 		cfg->monarch_timeout = 0;
@@ -2030,6 +2041,8 @@ static int __init mcheck_enable(char *str)
 		cfg->bootlog = (str[0] == 'b');
 	else if (!strcmp(str, "bios_cmci_threshold"))
 		cfg->bios_cmci_threshold = true;
+	else if (!strcmp(str, "recovery"))
+		cfg->recovery = true;
 	else if (isdigit(str[0])) {
 		if (get_option(&str, &cfg->tolerant) == 2)
 			get_option(&str, &(cfg->monarch_timeout));
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1337239 — [tip:x86/asm] x86/cpufeature: Create a new synthetic cpu capability for machine check recovery

Commit-ID:  0f68c088c0adb3c3bbeb487c4ebcde91fd5d34be
Gitweb:     http://git.kernel.org/tip/0f68c088c0adb3c3bbeb487c4ebcde91fd5d34be
Author:     Tony Luck <tony.luck@intel.com>
AuthorDate: Wed, 17 Feb 2016 10:20:13 -0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 18 Feb 2016 09:28:47 +0100

x86/cpufeature: Create a new synthetic cpu capability for machine check recovery

The Intel Software Developer Manual describes bit 24 in the MCG_CAP
MSR:

   MCG_SER_P (software error recovery support present) flag,
   bit 24 — Indicates (when set) that the processor supports
   software error recovery

But only some models with this capability bit set will actually
generate recoverable machine checks.

Check the model name and set a synthetic capability bit. Provide
a command line option to set this bit anyway in case the kernel
doesn't recognise the model name.

Signed-off-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Borislav Petkov <bp@suse.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/2e5bfb23c89800a036fb8a45fa97a74bb16bc362.1455732970.git.tony.luck@intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 Documentation/x86/x86_64/boot-options.txt |  2 ++
 arch/x86/include/asm/cpufeatures.h        |  1 +
 arch/x86/include/asm/mce.h                |  1 +
 arch/x86/kernel/cpu/mcheck/mce.c          | 13 +++++++++++++
 4 files changed, 17 insertions(+)

diff --git a/Documentation/x86/x86_64/boot-options.txt b/Documentation/x86/x86_64/boot-options.txt
index 68ed311..0965a71 100644
--- a/Documentation/x86/x86_64/boot-options.txt
+++ b/Documentation/x86/x86_64/boot-options.txt
@@ -60,6 +60,8 @@ Machine check
 		threshold to 1. Enabling this may make memory predictive failure
 		analysis less effective if the bios sets thresholds for memory
 		errors since we will not see details for all errors.
+   mce=recovery
+		Force-enable recoverable machine check code paths
 
    nomce (for compatibility with i386): same as mce=off
 
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 0ceb6ad..6663fae 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -106,6 +106,7 @@
 #define X86_FEATURE_APERFMPERF	( 3*32+28) /* APERFMPERF */
 #define X86_FEATURE_EAGER_FPU	( 3*32+29) /* "eagerfpu" Non lazy FPU restore */
 #define X86_FEATURE_NONSTOP_TSC_S3 ( 3*32+30) /* TSC doesn't stop in S3 state */
+#define X86_FEATURE_MCE_RECOVERY ( 3*32+31) /* cpu has recoverable machine checks */
 
 /* Intel-defined CPU features, CPUID level 0x00000001 (ecx), word 4 */
 #define X86_FEATURE_XMM3	( 4*32+ 0) /* "pni" SSE-3 */
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 2ea4527..18d2ba9 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -113,6 +113,7 @@ struct mca_config {
 	bool ignore_ce;
 	bool disabled;
 	bool ser;
+	bool recovery;
 	bool bios_cmci_threshold;
 	u8 banks;
 	s8 bootlog;
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index a006f4c..b5b187c 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1576,6 +1576,17 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
 
 		if (c->x86 == 6 && c->x86_model == 45)
 			quirk_no_way_out = quirk_sandybridge_ifu;
+		/*
+		 * MCG_CAP.MCG_SER_P is necessary but not sufficient to know
+		 * whether this processor will actually generate recoverable
+		 * machine checks. Check to see if this is an E7 model Xeon.
+		 * We can't do a model number check because E5 and E7 use the
+		 * same model number. E5 doesn't support recovery, E7 does.
+		 */
+		if (mca_cfg.recovery || (mca_cfg.ser &&
+			!strncmp(c->x86_model_id,
+				 "Intel(R) Xeon(R) CPU E7-", 24)))
+			set_cpu_cap(c, X86_FEATURE_MCE_RECOVERY);
 	}
 	if (cfg->monarch_timeout < 0)
 		cfg->monarch_timeout = 0;
@@ -2028,6 +2039,8 @@ static int __init mcheck_enable(char *str)
 		cfg->bootlog = (str[0] == 'b');
 	else if (!strcmp(str, "bios_cmci_threshold"))
 		cfg->bios_cmci_threshold = true;
+	else if (!strcmp(str, "recovery"))
+		cfg->recovery = true;
 	else if (isdigit(str[0])) {
 		if (get_option(&str, &cfg->tolerant) == 2)
 			get_option(&str, &(cfg->monarch_timeout));

[toc] | [prev] | [next] | [standalone]

#1336637 — [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

Make use of the EXTABLE_FAULT exception table entries. This routine
returns a structure to indicate the result of the copy:

struct mcsafe_ret {
        u64 trapnr;
        u64 remain;
};

If the copy is successful, then both 'trapnr' and 'remain' are zero.

If we faulted during the copy, then 'trapnr' will say which type
of trap (X86_TRAP_PF or X86_TRAP_MC) and 'remain' says how many
bytes were not copied.

Note that this is probably the first of several copy functions.
We can make new ones for non-temporal cache handling etc.

Reviewed-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/include/asm/string_64.h |   8 +++
 arch/x86/kernel/x8664_ksyms_64.c |   2 +
 arch/x86/lib/memcpy_64.S         | 151 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 161 insertions(+)

diff --git a/arch/x86/include/asm/string_64.h b/arch/x86/include/asm/string_64.h
index ff8b9a17dc4b..5b24039463a4 100644
--- a/arch/x86/include/asm/string_64.h
+++ b/arch/x86/include/asm/string_64.h
@@ -78,6 +78,14 @@ int strcmp(const char *cs, const char *ct);
 #define memset(s, c, n) __memset(s, c, n)
 #endif
 
+struct mcsafe_ret {
+	u64 trapnr;
+	u64 remain;
+};
+
+struct mcsafe_ret __mcsafe_copy(void *dst, const void __user *src, size_t cnt);
+extern void __mcsafe_copy_end(void);
+
 #endif /* __KERNEL__ */
 
 #endif /* _ASM_X86_STRING_64_H */
diff --git a/arch/x86/kernel/x8664_ksyms_64.c b/arch/x86/kernel/x8664_ksyms_64.c
index a0695be19864..fff245462a8c 100644
--- a/arch/x86/kernel/x8664_ksyms_64.c
+++ b/arch/x86/kernel/x8664_ksyms_64.c
@@ -37,6 +37,8 @@ EXPORT_SYMBOL(__copy_user_nocache);
 EXPORT_SYMBOL(_copy_from_user);
 EXPORT_SYMBOL(_copy_to_user);
 
+EXPORT_SYMBOL_GPL(__mcsafe_copy);
+
 EXPORT_SYMBOL(copy_page);
 EXPORT_SYMBOL(clear_page);
 
diff --git a/arch/x86/lib/memcpy_64.S b/arch/x86/lib/memcpy_64.S
index 16698bba87de..7f967a9ed0e4 100644
--- a/arch/x86/lib/memcpy_64.S
+++ b/arch/x86/lib/memcpy_64.S
@@ -177,3 +177,154 @@ ENTRY(memcpy_orig)
 .Lend:
 	retq
 ENDPROC(memcpy_orig)
+
+#ifndef CONFIG_UML
+/*
+ * __mcsafe_copy - 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.
+ */
+ENTRY(__mcsafe_copy)
+	cmpl $8,%edx
+	jb 20f		/* less then 8 bytes, go to byte copy loop */
+
+	/* check for bad alignment of source */
+	testl $7,%esi
+	/* already aligned */
+	jz 102f
+
+	/* copy one byte at a time until source is 8-byte aligned */
+	movl %esi,%ecx
+	andl $7,%ecx
+	subl $8,%ecx
+	negl %ecx
+	subl %ecx,%edx
+0:	movb (%rsi),%al
+	movb %al,(%rdi)
+	incq %rsi
+	incq %rdi
+	decl %ecx
+	jnz 0b
+
+102:
+	/* Figure out how many whole cache lines (64-bytes) to copy */
+	movl %edx,%ecx
+	andl $63,%edx
+	shrl $6,%ecx
+	jz 17f
+
+	/* Loop copying whole cache lines */
+1:	movq (%rsi),%r8
+2:	movq 1*8(%rsi),%r9
+3:	movq 2*8(%rsi),%r10
+4:	movq 3*8(%rsi),%r11
+	movq %r8,(%rdi)
+	movq %r9,1*8(%rdi)
+	movq %r10,2*8(%rdi)
+	movq %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
+	movq %r8,4*8(%rdi)
+	movq %r9,5*8(%rdi)
+	movq %r10,6*8(%rdi)
+	movq %r11,7*8(%rdi)
+	leaq 64(%rsi),%rsi
+	leaq 64(%rdi),%rdi
+	decl %ecx
+	jnz 1b
+
+	/* Are there any trailing 8-byte words? */
+17:	movl %edx,%ecx
+	andl $7,%edx
+	shrl $3,%ecx
+	jz 20f
+
+	/* Copy trailing words */
+18:	movq (%rsi),%r8
+	mov %r8,(%rdi)
+	leaq 8(%rsi),%rsi
+	leaq 8(%rdi),%rdi
+	decl %ecx
+	jnz 18b
+
+	/* Any trailing bytes? */
+20:	andl %edx,%edx
+	jz 23f
+
+	/* copy trailing bytes */
+	movl %edx,%ecx
+21:	movb (%rsi),%al
+	movb %al,(%rdi)
+	incq %rsi
+	incq %rdi
+	decl %ecx
+	jnz 21b
+
+	/* Copy successful. Return .remain = 0, .trapnr = 0 */
+23:	xorq %rax, %rax
+	xorq %rdx, %rdx
+	ret
+
+	.section .fixup,"ax"
+	/*
+	 * machine check handler loaded %rax with trap number
+	 * We just need to make sure %edx has the number of
+	 * bytes remaining
+	 */
+30:
+	add %ecx,%edx
+	ret
+31:
+	shl $6,%ecx
+	add %ecx,%edx
+	ret
+32:
+	shl $6,%ecx
+	lea -8(%ecx,%edx),%edx
+	ret
+33:
+	shl $6,%ecx
+	lea -16(%ecx,%edx),%edx
+	ret
+34:
+	shl $6,%ecx
+	lea -24(%ecx,%edx),%edx
+	ret
+35:
+	shl $6,%ecx
+	lea -32(%ecx,%edx),%edx
+	ret
+36:
+	shl $6,%ecx
+	lea -40(%ecx,%edx),%edx
+	ret
+37:
+	shl $6,%ecx
+	lea -48(%ecx,%edx),%edx
+	ret
+38:
+	shl $6,%ecx
+	lea -56(%ecx,%edx),%edx
+	ret
+39:
+	lea (%rdx,%rcx,8),%rdx
+	ret
+40:
+	mov %ecx,%edx
+	ret
+	.previous
+
+	_ASM_EXTABLE_FAULT(0b,30b)
+	_ASM_EXTABLE_FAULT(1b,31b)
+	_ASM_EXTABLE_FAULT(2b,32b)
+	_ASM_EXTABLE_FAULT(3b,33b)
+	_ASM_EXTABLE_FAULT(4b,34b)
+	_ASM_EXTABLE_FAULT(9b,35b)
+	_ASM_EXTABLE_FAULT(10b,36b)
+	_ASM_EXTABLE_FAULT(11b,37b)
+	_ASM_EXTABLE_FAULT(12b,38b)
+	_ASM_EXTABLE_FAULT(18b,39b)
+	_ASM_EXTABLE_FAULT(21b,40b)
+#endif
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1337138 — Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

* Tony Luck <tony.luck@intel.com> wrote:

> Make use of the EXTABLE_FAULT exception table entries. This routine
> returns a structure to indicate the result of the copy:

So the series looks good to me, but I have some (mostly readability) comments that 
went beyond what I usually fix up manually:

> struct mcsafe_ret {
>         u64 trapnr;
>         u64 remain;
> };

> +struct mcsafe_ret {
> +	u64 trapnr;
> +	u64 remain;
> +};

Yeah, so please change this to something like:

  struct mcsafe_ret {
          u64 trap_nr;
          u64 bytes_left;
  };

this makes it crystal clear what the fields are about and what their unit is. 
Readability is king and modern consoles are wide enough, no need to abbreviate 
excessively.

> +struct mcsafe_ret __mcsafe_copy(void *dst, const void __user *src, size_t cnt);
> +extern void __mcsafe_copy_end(void);

So this is a bad name I think. What kind of 'copy' is this? It's defined in 
asm/string_64.h - so people might thing it's a string copy. If it's a memcpy 
variant then name it so.

Also, I'd suggest we postfix the new mcsafe functions with '_mcsafe', not prefix 
them. Special properties of memcpy routines are usually postfixes - such as 
_nocache(), _toio(), etc.

> --- a/arch/x86/kernel/x8664_ksyms_64.c
> +++ b/arch/x86/kernel/x8664_ksyms_64.c
> @@ -37,6 +37,8 @@ EXPORT_SYMBOL(__copy_user_nocache);
>  EXPORT_SYMBOL(_copy_from_user);
>  EXPORT_SYMBOL(_copy_to_user);
>  
> +EXPORT_SYMBOL_GPL(__mcsafe_copy);
> +
>  EXPORT_SYMBOL(copy_page);
>  EXPORT_SYMBOL(clear_page);
>  
> diff --git a/arch/x86/lib/memcpy_64.S b/arch/x86/lib/memcpy_64.S
> index 16698bba87de..7f967a9ed0e4 100644
> --- a/arch/x86/lib/memcpy_64.S
> +++ b/arch/x86/lib/memcpy_64.S
> @@ -177,3 +177,154 @@ ENTRY(memcpy_orig)
>  .Lend:
>  	retq
>  ENDPROC(memcpy_orig)
> +
> +#ifndef CONFIG_UML

Why is this UML quirk needed? No other memcpy functions have it. Theoretically UML 
could introduce the notion of #MC interruption.

> +/*
> + * __mcsafe_copy - 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.
> + */
> +ENTRY(__mcsafe_copy)
> +	cmpl $8,%edx
> +	jb 20f		/* less then 8 bytes, go to byte copy loop */
> +
> +	/* check for bad alignment of source */
> +	testl $7,%esi
> +	/* already aligned */
> +	jz 102f
> +
> +	/* copy one byte at a time until source is 8-byte aligned */
> +	movl %esi,%ecx
> +	andl $7,%ecx
> +	subl $8,%ecx
> +	negl %ecx
> +	subl %ecx,%edx
> +0:	movb (%rsi),%al
> +	movb %al,(%rdi)
> +	incq %rsi
> +	incq %rdi
> +	decl %ecx
> +	jnz 0b
> +
> +102:
> +	/* Figure out how many whole cache lines (64-bytes) to copy */
> +	movl %edx,%ecx
> +	andl $63,%edx
> +	shrl $6,%ecx
> +	jz 17f

Please don't use numeric labels in new assembly code, use descriptively named 
local labels:

  .L_do_stuff:

numeric labels are generally unfriendly against future changes. They are the GOTO 
numeric labels of BASIC.

> +
> +	/* Loop copying whole cache lines */
> +1:	movq (%rsi),%r8
> +2:	movq 1*8(%rsi),%r9
> +3:	movq 2*8(%rsi),%r10
> +4:	movq 3*8(%rsi),%r11
> +	movq %r8,(%rdi)
> +	movq %r9,1*8(%rdi)
> +	movq %r10,2*8(%rdi)
> +	movq %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
> +	movq %r8,4*8(%rdi)
> +	movq %r9,5*8(%rdi)
> +	movq %r10,6*8(%rdi)
> +	movq %r11,7*8(%rdi)
> +	leaq 64(%rsi),%rsi
> +	leaq 64(%rdi),%rdi
> +	decl %ecx
> +	jnz 1b
> +
> +	/* Are there any trailing 8-byte words? */
> +17:	movl %edx,%ecx
> +	andl $7,%edx
> +	shrl $3,%ecx
> +	jz 20f
> +
> +	/* Copy trailing words */
> +18:	movq (%rsi),%r8
> +	mov %r8,(%rdi)
> +	leaq 8(%rsi),%rsi
> +	leaq 8(%rdi),%rdi
> +	decl %ecx
> +	jnz 18b
> +
> +	/* Any trailing bytes? */
> +20:	andl %edx,%edx
> +	jz 23f
> +
> +	/* copy trailing bytes */
> +	movl %edx,%ecx
> +21:	movb (%rsi),%al
> +	movb %al,(%rdi)
> +	incq %rsi
> +	incq %rdi
> +	decl %ecx
> +	jnz 21b
> +
> +	/* Copy successful. Return .remain = 0, .trapnr = 0 */
> +23:	xorq %rax, %rax
> +	xorq %rdx, %rdx
> +	ret
> +
> +	.section .fixup,"ax"
> +	/*
> +	 * machine check handler loaded %rax with trap number
> +	 * We just need to make sure %edx has the number of
> +	 * bytes remaining
> +	 */

Please use consistent capitalization in comments and punctuate sentences where 
there's more than one of them.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1337213 — Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

On Thu, Feb 18, 2016 at 09:21:07AM +0100, Ingo Molnar wrote:
> 
> * Tony Luck <tony.luck@intel.com> wrote:
> 
> > Make use of the EXTABLE_FAULT exception table entries. This routine
> > returns a structure to indicate the result of the copy:
> 
> So the series looks good to me, but I have some (mostly readability) comments that 
> went beyond what I usually fix up manually:
> 
> > struct mcsafe_ret {
> >         u64 trapnr;
> >         u64 remain;
> > };
> 
> > +struct mcsafe_ret {
> > +	u64 trapnr;
> > +	u64 remain;
> > +};
> 
> Yeah, so please change this to something like:
> 
>   struct mcsafe_ret {
>           u64 trap_nr;
>           u64 bytes_left;
>   };
> 
> this makes it crystal clear what the fields are about and what their unit is. 
> Readability is king and modern consoles are wide enough, no need to abbreviate 
> excessively.

I prefer to use my modern console width to display multiple columns of
text, instead of wasting it to display mostly whitespace. Therefore I
still very much prefer ~80 char wide code.

> > +struct mcsafe_ret __mcsafe_copy(void *dst, const void __user *src, size_t cnt);
> > +extern void __mcsafe_copy_end(void);
> 
> So this is a bad name I think. What kind of 'copy' is this? It's defined in 
> asm/string_64.h - so people might thing it's a string copy. If it's a memcpy 
> variant then name it so.
> 
> Also, I'd suggest we postfix the new mcsafe functions with '_mcsafe', not prefix 
> them. Special properties of memcpy routines are usually postfixes - such as 
> _nocache(), _toio(), etc.

I think the whole notion of mcsafe here is 'wrong'. This copy variant
simply reports the kind of trap that happened (#PF or #MC) and could
arguably be extended to include more types if the hardware were to
generate more.

[toc] | [prev] | [next] | [standalone]

#1337230 — Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

On Thu, Feb 18, 2016 at 11:19:42AM +0100, Ingo Molnar wrote:
> > I think the whole notion of mcsafe here is 'wrong'. This copy variant simply 
> > reports the kind of trap that happened (#PF or #MC) and could arguably be 
> > extended to include more types if the hardware were to generate more.

It is safe in the sense that when you get an MCE while shuffling data
here, in the kernel, you don't die but you recover. Thus the exception
handling games.

So _safe() really sounds fitting here.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1337238 — Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Feb 18, 2016 at 09:21:07AM +0100, Ingo Molnar wrote:
> > 
> > * Tony Luck <tony.luck@intel.com> wrote:
> > 
> > > Make use of the EXTABLE_FAULT exception table entries. This routine
> > > returns a structure to indicate the result of the copy:
> > 
> > So the series looks good to me, but I have some (mostly readability) comments that 
> > went beyond what I usually fix up manually:
> > 
> > > struct mcsafe_ret {
> > >         u64 trapnr;
> > >         u64 remain;
> > > };
> > 
> > > +struct mcsafe_ret {
> > > +	u64 trapnr;
> > > +	u64 remain;
> > > +};
> > 
> > Yeah, so please change this to something like:
> > 
> >   struct mcsafe_ret {
> >           u64 trap_nr;
> >           u64 bytes_left;
> >   };
> > 
> > this makes it crystal clear what the fields are about and what their unit is. 
> > Readability is king and modern consoles are wide enough, no need to abbreviate 
> > excessively.
> 
> I prefer to use my modern console width to display multiple columns of
> text, instead of wasting it to display mostly whitespace. Therefore I
> still very much prefer ~80 char wide code.

This naming won't hurt the col80 limit.

> > Also, I'd suggest we postfix the new mcsafe functions with '_mcsafe', not 
> > prefix them. Special properties of memcpy routines are usually postfixes - 
> > such as _nocache(), _toio(), etc.
> 
> I think the whole notion of mcsafe here is 'wrong'. This copy variant simply 
> reports the kind of trap that happened (#PF or #MC) and could arguably be 
> extended to include more types if the hardware were to generate more.

What would a better name be? memcpy_ret() or so?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1337244 — Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

On Thu, Feb 18, 2016 at 11:19:42AM +0100, Ingo Molnar wrote:
> > I think the whole notion of mcsafe here is 'wrong'. This copy variant simply 
> > reports the kind of trap that happened (#PF or #MC) and could arguably be 
> > extended to include more types if the hardware were to generate more.
> 
> What would a better name be? memcpy_ret() or so?

Yeah, uhmm.. naming. More options from the lack of inspiration department:

  memcpy_trap()
  memcpy_ex()

[toc] | [prev] | [next] | [standalone]

#1337435 — RE: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

> > > I think the whole notion of mcsafe here is 'wrong'. This copy variant simply 
> > > reports the kind of trap that happened (#PF or #MC) and could arguably be 
> > > extended to include more types if the hardware were to generate more.
> > 
> > What would a better name be? memcpy_ret() or so?
>
> Yeah, uhmm.. naming. More options from the lack of inspiration department:
>
>   memcpy_trap()
>   memcpy_ex()

I like "memcpy_trap" - thanks Peter.  I'll redo this part with all the other fixes
suggested by Ingo.

-Tony

[toc] | [prev] | [next] | [standalone]

#1337948 — Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

* Peter Zijlstra <peterz@infradead.org> wrote:

> > Yeah, so please change this to something like:
> > 
> >   struct mcsafe_ret {
> >           u64 trap_nr;
> >           u64 bytes_left;
> >   };
> > 
> > this makes it crystal clear what the fields are about and what their unit is. 
> > Readability is king and modern consoles are wide enough, no need to abbreviate 
> > excessively.
> 
> I prefer to use my modern console width to display multiple columns of text, 
> instead of wasting it to display mostly whitespace. Therefore I still very much 
> prefer ~80 char wide code.

Btw., the main reason I hate the col80 limit is that I see such patches 
frequently:

 void pcibios_add_bus(struct pci_bus *bus)
 {
+#ifdef CONFIG_DMI
+       const struct dmi_device *dmi;
+       struct dmi_dev_onboard *dslot;
+       char sname[128];
+
+       dmi = NULL;
+       while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEV_SLOT,
+                                     NULL, dmi)) != NULL) {
+               dslot = dmi->device_data;
+               if (dslot->segment == pci_domain_nr(bus) &&
+                   dslot->bus == bus->number) {
+                       dev_info(&bus->dev, "Found SMBIOS Slot %s\n",
+                                dslot->dev.name);
+                       snprintf(sname, sizeof(sname), "%s-%d",
+                                dslot->dev.name,
+                                dslot->instance);
+                       pci_create_slot(bus, dslot->devfn,
+                                       sname, NULL);
+               }
+       }
+#endif
        acpi_pci_add_bus(bus);

Which gobbledygook has 6 (!) col80 artifacts - and it's a pretty straightforward 
piece of code with just 2 levels of indentation.

It is IMHO much more readable in the following form:

 void pcibios_add_bus(struct pci_bus *bus)
 {
 #ifdef CONFIG_DMI
        const struct dmi_device *dmi;
        struct dmi_dev_onboard *dslot;
        char sname[128];

        dmi = NULL;
        while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEV_SLOT, NULL, dmi)) != NULL) {
                dslot = dmi->device_data;
                if (dslot->segment == pci_domain_nr(bus) && dslot->bus == bus->number) {
                        dev_info(&bus->dev, "Found SMBIOS Slot %s\n", dslot->dev.name);
                        snprintf(sname, sizeof(sname), "%s-%d", dslot->dev.name, dslot->instance);
                        pci_create_slot(bus, dslot->devfn, sname, NULL);
                }
        }
 #endif
        acpi_pci_add_bus(bus);

BYMMV.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1337999 — Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

On Fri, Feb 19, 2016 at 08:58:43AM +0100, Ingo Molnar wrote:
> > I prefer to use my modern console width to display multiple columns of text, 
> > instead of wasting it to display mostly whitespace. Therefore I still very much 
> > prefer ~80 char wide code.
> 
> Btw., the main reason I hate the col80 limit is that I see such patches 
> frequently:
> 
>  void pcibios_add_bus(struct pci_bus *bus)
>  {
> +#ifdef CONFIG_DMI
> +       const struct dmi_device *dmi;
> +       struct dmi_dev_onboard *dslot;
> +       char sname[128];
> +
> +       dmi = NULL;
> +       while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEV_SLOT,
> +                                     NULL, dmi)) != NULL) {
> +               dslot = dmi->device_data;
> +               if (dslot->segment == pci_domain_nr(bus) &&
> +                   dslot->bus == bus->number) {
> +                       dev_info(&bus->dev, "Found SMBIOS Slot %s\n",
> +                                dslot->dev.name);
> +                       snprintf(sname, sizeof(sname), "%s-%d",
> +                                dslot->dev.name,
> +                                dslot->instance);
> +                       pci_create_slot(bus, dslot->devfn,
> +                                       sname, NULL);
> +               }
> +       }
> +#endif
>         acpi_pci_add_bus(bus);
> 
> Which gobbledygook has 6 (!) col80 artifacts - and it's a pretty straightforward 
> piece of code with just 2 levels of indentation.
> 
> It is IMHO much more readable in the following form:
> 
>  void pcibios_add_bus(struct pci_bus *bus)
>  {
>  #ifdef CONFIG_DMI
>         const struct dmi_device *dmi;
>         struct dmi_dev_onboard *dslot;
>         char sname[128];
> 
>         dmi = NULL;
>         while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEV_SLOT, NULL, dmi)) != NULL) {
>                 dslot = dmi->device_data;
>                 if (dslot->segment == pci_domain_nr(bus) && dslot->bus == bus->number) {
>                         dev_info(&bus->dev, "Found SMBIOS Slot %s\n", dslot->dev.name);
>                         snprintf(sname, sizeof(sname), "%s-%d", dslot->dev.name, dslot->instance);
>                         pci_create_slot(bus, dslot->devfn, sname, NULL);
>                 }
>         }
>  #endif
>         acpi_pci_add_bus(bus);
> 
> BYMMV.

So I mostly agree, although your example does wrap on my normal display
width. The thing is though, we have to have a limit, otherwise people
will completely let loose and we'll end up with lines >200 chars wide
(I've worked on code like that in the past, and its a right pain).

And 80 has so far mostly worked just fine. Its just that people seem
unable to take guidelines as just than, and instead produce the most
horrible code just to adhere to checkpatch or whatnot.

And I'd much rather have an extra column of code than waste a lot of
screen-estate to display mostly whitespace.

Also, this 'artificial' limit on indentation level does in general
encourage people to write saner code.

[toc] | [prev] | [next] | [standalone]

#1338020 — Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Feb 19, 2016 at 08:58:43AM +0100, Ingo Molnar wrote:
> > > I prefer to use my modern console width to display multiple columns of text, 
> > > instead of wasting it to display mostly whitespace. Therefore I still very much 
> > > prefer ~80 char wide code.
> > 
> > Btw., the main reason I hate the col80 limit is that I see such patches 
> > frequently:
> > 
> >  void pcibios_add_bus(struct pci_bus *bus)
> >  {
> > +#ifdef CONFIG_DMI
> > +       const struct dmi_device *dmi;
> > +       struct dmi_dev_onboard *dslot;
> > +       char sname[128];
> > +
> > +       dmi = NULL;
> > +       while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEV_SLOT,
> > +                                     NULL, dmi)) != NULL) {
> > +               dslot = dmi->device_data;
> > +               if (dslot->segment == pci_domain_nr(bus) &&
> > +                   dslot->bus == bus->number) {
> > +                       dev_info(&bus->dev, "Found SMBIOS Slot %s\n",
> > +                                dslot->dev.name);
> > +                       snprintf(sname, sizeof(sname), "%s-%d",
> > +                                dslot->dev.name,
> > +                                dslot->instance);
> > +                       pci_create_slot(bus, dslot->devfn,
> > +                                       sname, NULL);
> > +               }
> > +       }
> > +#endif
> >         acpi_pci_add_bus(bus);
> > 
> > Which gobbledygook has 6 (!) col80 artifacts - and it's a pretty straightforward 
> > piece of code with just 2 levels of indentation.
> > 
> > It is IMHO much more readable in the following form:
> > 
> >  void pcibios_add_bus(struct pci_bus *bus)
> >  {
> >  #ifdef CONFIG_DMI
> >         const struct dmi_device *dmi;
> >         struct dmi_dev_onboard *dslot;
> >         char sname[128];
> > 
> >         dmi = NULL;
> >         while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEV_SLOT, NULL, dmi)) != NULL) {
> >                 dslot = dmi->device_data;
> >                 if (dslot->segment == pci_domain_nr(bus) && dslot->bus == bus->number) {
> >                         dev_info(&bus->dev, "Found SMBIOS Slot %s\n", dslot->dev.name);
> >                         snprintf(sname, sizeof(sname), "%s-%d", dslot->dev.name, dslot->instance);
> >                         pci_create_slot(bus, dslot->devfn, sname, NULL);
> >                 }
> >         }
> >  #endif
> >         acpi_pci_add_bus(bus);
> > 
> > BYMMV.
> 
> So I mostly agree, although your example does wrap on my normal display
> width. The thing is though, we have to have a limit, otherwise people
> will completely let loose and we'll end up with lines >200 chars wide
> (I've worked on code like that in the past, and its a right pain).

What I'm arguing for is to be, on average, _stricter_ than col80, but not use the 
absolute width as a metric.

Obviusly we have to have limits (to have a consistent coding style) - but I think 
it should be the level of indentation/nesting that should be the limit and the 
number of arguments to a function, while the absolute character count should be 
relaxed in certain cases (and should be made more strict in others!), such as 
printks and other 'leaf' functionality that has no primary side effects.

I'd be fine with only allowing up to 2-3 levels of nesting in typical code 
situations, and not having silly long names. I'd also maximize function arguments 
at about ~4 parameters for the typical case - anything longer should probably 
organize the parameters into helper structures.

But yeah, I can see the pragmatic power of a 'simple' guideline, such as col80.

> And 80 has so far mostly worked just fine. Its just that people seem
> unable to take guidelines as just than, and instead produce the most
> horrible code just to adhere to checkpatch or whatnot.

I think it should be made _stricter_ in many cases.

I.e. col80 is too easy to work around and is routinely worked around in 80% of the 
patches I get.

> And I'd much rather have an extra column of code than waste a lot of
> screen-estate to display mostly whitespace.

So I think that with my proposed rule we'd mostly have much shorter code than 80 
colums, with a few cases (such as printk() lines) where _you_ could easily accept 
automatic, editor generated line wraps instead of forcing the ugliness of manual 
line breaks on everyone else...

The fact is that in almost every patch I receieve these days I see col80 artifacts 
that could be avoided with better code structure - or that would look better if 
they were not manually line-broken.

I.e. I don't so much argue in favor of making lines longer than 80 cols, I argue 
against 'col80 line breaks' that are an easy workaround around the col80 rule.

> Also, this 'artificial' limit on indentation level does in general encourage 
> people to write saner code.

But that's not what we have - col80 is in fact too permissive when it comes to 
actual code complexity!

Let me give a random example - took me 20 seconds to find in kernel/*.c: 
kernel/cgroup.c's cgroup_subtree_control_write():

 - the function is way too big with 230+ lines - it should be split into 2-4
   helper functions.

 - the deepest C indentation it has is too much: 4 levels

 - the function has 11+ 'col80 artifacts' that I counted

 - the function has similar looking code patterns repeated over it

 - the control flow is messy at places - goto patterns mixed with open coded 
   unlock sequences.

 - some logic is completely undocumented - for example can you tell at a glance 
   what the first for_each_subsys() loop does? If it was in a helper function it 
   would be self-documenting to a large degree.

and it's a piece of code that is completely col80 compliant while it has obvious 
problems.

Most of the problems in this function would go away with two relatively simple 
rules, which are in fact stricter than col80 limits:

 - not going over 3 levels deep of nesting.

 - not allowing 'manual line breaks' for non-trivial functionality. This rule
   would flag ugly pieces of code like:

                cgroup_for_each_live_child(child, cgrp) {
                        if (css_enable & (1 << ssid))
                                ret = create_css(child, ss,
                                        cgrp->subtree_control & (1 << ssid));
                        else
                                ret = css_populate_dir(cgroup_css(child, ss),
                                                       NULL);
                        if (ret)
                                goto err_undo_css;
                }

these two rules would IMHO automatically limit the complexity of many functions 
that are too complex today.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1337250 — Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

* Borislav Petkov <bp@alien8.de> wrote:

> On Thu, Feb 18, 2016 at 09:21:07AM +0100, Ingo Molnar wrote:
> > > +#ifndef CONFIG_UML
> > 
> > Why is this UML quirk needed? No other memcpy functions have it. Theoretically UML 
> > could introduce the notion of #MC interruption.
> 
> https://lkml.kernel.org/r/56B7AEEE.5070504@nod.at

Does the build fail - or is it just an unused function? If the latter then I'd 
rather leave the #ifdef out.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1337255 — Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

On Thu, Feb 18, 2016 at 11:34:20AM +0100, Ingo Molnar wrote:
> Does the build fail - or is it just an unused function? If the latter
> then I'd rather leave the #ifdef out.

Yep, it does fail:

https://lkml.kernel.org/r/20160207165524.GF5862@pd.tnic

due to ex_handler_fault not being visible to UML.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1337621 — Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

* Borislav Petkov <bp@alien8.de> wrote:

> On Thu, Feb 18, 2016 at 11:34:20AM +0100, Ingo Molnar wrote:
> > Does the build fail - or is it just an unused function? If the latter
> > then I'd rather leave the #ifdef out.
> 
> Yep, it does fail:
> 
> https://lkml.kernel.org/r/20160207165524.GF5862@pd.tnic
> 
> due to ex_handler_fault not being visible to UML.

Ok, fair enough.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1337257 — Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

On Thu, Feb 18, 2016 at 09:21:07AM +0100, Ingo Molnar wrote:
> > +#ifndef CONFIG_UML
> 
> Why is this UML quirk needed? No other memcpy functions have it. Theoretically UML 
> could introduce the notion of #MC interruption.

https://lkml.kernel.org/r/56B7AEEE.5070504@nod.at

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1337741 — [PATCH v12] x86, mce: Add memcpy_trap()

Make use of the EXTABLE_FAULT exception table entries. This routine
returns a structure to indicate the result of the copy:

struct mcsafe_ret {
        u64 trap_nr;
        u64 bytes_left;
};

If the copy is successful, then both 'trap_nr' and 'bytes_left' are zero.

If we faulted during the copy, then 'trap_nr' will say which type
of trap (X86_TRAP_PF or X86_TRAP_MC) and 'bytes_left' says how many
bytes were not copied.

Note that this is probably the first of several copy functions.
We can make new ones for non-temporal cache handling etc.

Reviewed-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---

V12 (part3 only - parts 1,2,4 are in tip tree)

Ingo:	More meaningful names for fields of return structure
PeterZ:	Better name for copy function: memcpy_trap()
Ingo:	Don't use numeric labels in new asm code
Ingo:	Consistent capitalization in comments.
Ingo:	Periods between sentences in comments.

Not addressed: Linus' comment that perhaps we could try/catch
syntactic sugar instead of returning multiple values in a structure.

 arch/x86/include/asm/string_64.h |  26 +++++++
 arch/x86/kernel/x8664_ksyms_64.c |   2 +
 arch/x86/lib/memcpy_64.S         | 158 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 186 insertions(+)

diff --git a/arch/x86/include/asm/string_64.h b/arch/x86/include/asm/string_64.h
index ff8b9a17dc4b..65e5793b7590 100644
--- a/arch/x86/include/asm/string_64.h
+++ b/arch/x86/include/asm/string_64.h
@@ -78,6 +78,32 @@ int strcmp(const char *cs, const char *ct);
 #define memset(s, c, n) __memset(s, c, n)
 #endif
 
+/**
+ * struct memcpy_trap_ret - return value from memcpy_trap()
+ *
+ * @trap_nr	x86 trap number if the copy failed
+ * @bytes_left	zero for successful copy else number of bytes not copied
+ */
+struct memcpy_trap_ret {
+	u64 trap_nr;
+	u64 bytes_left;
+};
+
+/**
+ * memcpy_trap - copy memory with indication if a trap interrupted the copy
+ *
+ * @dst:	destination address
+ * @src:	source address
+ * @cnt:	number of bytes to copy
+ *
+ * Low level memory copy function that catches traps and indicates whether
+ * the copy succeeded and if not, why it failed.
+ *
+ * Return is struct memcpy_trap_ret which provides both the number of bytes
+ * not copied and the reason for the failure.
+ */
+struct memcpy_trap_ret memcpy_trap(void *dst, const void __user *src, size_t cnt);
+
 #endif /* __KERNEL__ */
 
 #endif /* _ASM_X86_STRING_64_H */
diff --git a/arch/x86/kernel/x8664_ksyms_64.c b/arch/x86/kernel/x8664_ksyms_64.c
index a0695be19864..40866e2cbcc4 100644
--- a/arch/x86/kernel/x8664_ksyms_64.c
+++ b/arch/x86/kernel/x8664_ksyms_64.c
@@ -37,6 +37,8 @@ EXPORT_SYMBOL(__copy_user_nocache);
 EXPORT_SYMBOL(_copy_from_user);
 EXPORT_SYMBOL(_copy_to_user);
 
+EXPORT_SYMBOL_GPL(memcpy_trap);
+
 EXPORT_SYMBOL(copy_page);
 EXPORT_SYMBOL(clear_page);
 
diff --git a/arch/x86/lib/memcpy_64.S b/arch/x86/lib/memcpy_64.S
index 16698bba87de..aecdfc41c114 100644
--- a/arch/x86/lib/memcpy_64.S
+++ b/arch/x86/lib/memcpy_64.S
@@ -177,3 +177,161 @@ ENTRY(memcpy_orig)
 .Lend:
 	retq
 ENDPROC(memcpy_orig)
+
+#ifndef CONFIG_UML
+/*
+ * memcpy_trap - 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.
+ */
+ENTRY(memcpy_trap)
+	cmpl $8,%edx
+	/* Less than 8 bytes? Go to byte copy loop */
+	jb .L_no_whole_words
+
+	/* Check for bad alignment of source */
+	testl $7,%esi
+	/* Already aligned */
+	jz .L_8byte_aligned
+
+	/* Copy one byte at a time until source is 8-byte aligned */
+	movl %esi,%ecx
+	andl $7,%ecx
+	subl $8,%ecx
+	negl %ecx
+	subl %ecx,%edx
+.L_copy_leading_bytes:
+	movb (%rsi),%al
+	movb %al,(%rdi)
+	incq %rsi
+	incq %rdi
+	decl %ecx
+	jnz .L_copy_leading_bytes
+
+.L_8byte_aligned:
+	/* Figure out how many whole cache lines (64-bytes) to copy */
+	movl %edx,%ecx
+	andl $63,%edx
+	shrl $6,%ecx
+	jz .L_no_whole_cache_lines
+
+	/* Loop copying whole cache lines */
+.L_cache_w0: movq (%rsi),%r8
+.L_cache_w1: movq 1*8(%rsi),%r9
+.L_cache_w2: movq 2*8(%rsi),%r10
+.L_cache_w3: movq 3*8(%rsi),%r11
+	movq %r8,(%rdi)
+	movq %r9,1*8(%rdi)
+	movq %r10,2*8(%rdi)
+	movq %r11,3*8(%rdi)
+.L_cache_w4: movq 4*8(%rsi),%r8
+.L_cache_w5: movq 5*8(%rsi),%r9
+.L_cache_w6: movq 6*8(%rsi),%r10
+.L_cache_w7: movq 7*8(%rsi),%r11
+	movq %r8,4*8(%rdi)
+	movq %r9,5*8(%rdi)
+	movq %r10,6*8(%rdi)
+	movq %r11,7*8(%rdi)
+	leaq 64(%rsi),%rsi
+	leaq 64(%rdi),%rdi
+	decl %ecx
+	jnz .L_cache_w0
+
+	/* Are there any trailing 8-byte words? */
+.L_no_whole_cache_lines:
+	movl %edx,%ecx
+	andl $7,%edx
+	shrl $3,%ecx
+	jz .L_no_whole_words
+
+	/* Copy trailing words */
+.L_copy_trailing_words:
+	movq (%rsi),%r8
+	mov %r8,(%rdi)
+	leaq 8(%rsi),%rsi
+	leaq 8(%rdi),%rdi
+	decl %ecx
+	jnz .L_copy_trailing_words
+
+	/* Any trailing bytes? */
+.L_no_whole_words:
+	andl %edx,%edx
+	jz .L_done_memcpy_trap
+
+	/* Copy trailing bytes */
+	movl %edx,%ecx
+.L_copy_trailing_bytes:
+	movb (%rsi),%al
+	movb %al,(%rdi)
+	incq %rsi
+	incq %rdi
+	decl %ecx
+	jnz .L_copy_trailing_bytes
+
+	/* Copy successful. Return .remain = 0, .trapnr = 0 */
+.L_done_memcpy_trap:
+	xorq %rax, %rax
+	xorq %rdx, %rdx
+	ret
+
+	.section .fixup,"ax"
+	/*
+	 * The machine check handler loaded %rax with trap number.
+	 * We just need to make sure %edx has the number of
+	 * bytes remaining.
+	 */
+.L_fix_leading_bytes:
+	add %ecx,%edx
+	ret
+.L_fix_cache_w0:
+	shl $6,%ecx
+	add %ecx,%edx
+	ret
+.L_fix_cache_w1:
+	shl $6,%ecx
+	lea -8(%ecx,%edx),%edx
+	ret
+.L_fix_cache_w2:
+	shl $6,%ecx
+	lea -16(%ecx,%edx),%edx
+	ret
+.L_fix_cache_w3:
+	shl $6,%ecx
+	lea -24(%ecx,%edx),%edx
+	ret
+.L_fix_cache_w4:
+	shl $6,%ecx
+	lea -32(%ecx,%edx),%edx
+	ret
+.L_fix_cache_w5:
+	shl $6,%ecx
+	lea -40(%ecx,%edx),%edx
+	ret
+.L_fix_cache_w6:
+	shl $6,%ecx
+	lea -48(%ecx,%edx),%edx
+	ret
+.L_fix_cache_w7:
+	shl $6,%ecx
+	lea -56(%ecx,%edx),%edx
+	ret
+.L_fix_trailing_words:
+	lea (%rdx,%rcx,8),%rdx
+	ret
+.L_fix_trailing_bytes:
+	mov %ecx,%edx
+	ret
+	.previous
+
+	_ASM_EXTABLE_FAULT(.L_copy_leading_bytes,.L_fix_leading_bytes)
+	_ASM_EXTABLE_FAULT(.L_cache_w0,.L_fix_cache_w0)
+	_ASM_EXTABLE_FAULT(.L_cache_w1,.L_fix_cache_w1)
+	_ASM_EXTABLE_FAULT(.L_cache_w3,.L_fix_cache_w2)
+	_ASM_EXTABLE_FAULT(.L_cache_w3,.L_fix_cache_w3)
+	_ASM_EXTABLE_FAULT(.L_cache_w4,.L_fix_cache_w4)
+	_ASM_EXTABLE_FAULT(.L_cache_w5,.L_fix_cache_w5)
+	_ASM_EXTABLE_FAULT(.L_cache_w6,.L_fix_cache_w6)
+	_ASM_EXTABLE_FAULT(.L_cache_w7,.L_fix_cache_w7)
+	_ASM_EXTABLE_FAULT(.L_copy_trailing_words,.L_fix_trailing_words)
+	_ASM_EXTABLE_FAULT(.L_copy_trailing_bytes,.L_fix_trailing_bytes)
+#endif
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1338008 — Re: [PATCH v12] x86, mce: Add memcpy_trap()

* Luck, Tony <tony.luck@intel.com> wrote:

> Make use of the EXTABLE_FAULT exception table entries. This routine
> returns a structure to indicate the result of the copy:
> 
> struct mcsafe_ret {
>         u64 trap_nr;
>         u64 bytes_left;
> };
> 
> If the copy is successful, then both 'trap_nr' and 'bytes_left' are zero.
> 
> If we faulted during the copy, then 'trap_nr' will say which type
> of trap (X86_TRAP_PF or X86_TRAP_MC) and 'bytes_left' says how many
> bytes were not copied.
> 
> Note that this is probably the first of several copy functions.
> We can make new ones for non-temporal cache handling etc.
> 
> Reviewed-by: Borislav Petkov <bp@suse.de>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> 
> V12 (part3 only - parts 1,2,4 are in tip tree)
> 
> Ingo:	More meaningful names for fields of return structure
> PeterZ:	Better name for copy function: memcpy_trap()
> Ingo:	Don't use numeric labels in new asm code
> Ingo:	Consistent capitalization in comments.
> Ingo:	Periods between sentences in comments.
> 
> Not addressed: Linus' comment that perhaps we could try/catch
> syntactic sugar instead of returning multiple values in a structure.
> 
>  arch/x86/include/asm/string_64.h |  26 +++++++
>  arch/x86/kernel/x8664_ksyms_64.c |   2 +
>  arch/x86/lib/memcpy_64.S         | 158 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 186 insertions(+)
> 
> diff --git a/arch/x86/include/asm/string_64.h b/arch/x86/include/asm/string_64.h
> index ff8b9a17dc4b..65e5793b7590 100644
> --- a/arch/x86/include/asm/string_64.h
> +++ b/arch/x86/include/asm/string_64.h
> @@ -78,6 +78,32 @@ int strcmp(const char *cs, const char *ct);
>  #define memset(s, c, n) __memset(s, c, n)
>  #endif
>  
> +/**
> + * struct memcpy_trap_ret - return value from memcpy_trap()
> + *
> + * @trap_nr	x86 trap number if the copy failed
> + * @bytes_left	zero for successful copy else number of bytes not copied
> + */
> +struct memcpy_trap_ret {
> +	u64 trap_nr;
> +	u64 bytes_left;
> +};
> +
> +/**
> + * memcpy_trap - copy memory with indication if a trap interrupted the copy
> + *
> + * @dst:	destination address
> + * @src:	source address
> + * @cnt:	number of bytes to copy
> + *
> + * Low level memory copy function that catches traps and indicates whether
> + * the copy succeeded and if not, why it failed.
> + *
> + * Return is struct memcpy_trap_ret which provides both the number of bytes
> + * not copied and the reason for the failure.
> + */
> +struct memcpy_trap_ret memcpy_trap(void *dst, const void __user *src, size_t cnt);
> +
>  #endif /* __KERNEL__ */
>  
>  #endif /* _ASM_X86_STRING_64_H */
> diff --git a/arch/x86/kernel/x8664_ksyms_64.c b/arch/x86/kernel/x8664_ksyms_64.c
> index a0695be19864..40866e2cbcc4 100644
> --- a/arch/x86/kernel/x8664_ksyms_64.c
> +++ b/arch/x86/kernel/x8664_ksyms_64.c
> @@ -37,6 +37,8 @@ EXPORT_SYMBOL(__copy_user_nocache);
>  EXPORT_SYMBOL(_copy_from_user);
>  EXPORT_SYMBOL(_copy_to_user);
>  
> +EXPORT_SYMBOL_GPL(memcpy_trap);
> +
>  EXPORT_SYMBOL(copy_page);
>  EXPORT_SYMBOL(clear_page);
>  
> diff --git a/arch/x86/lib/memcpy_64.S b/arch/x86/lib/memcpy_64.S
> index 16698bba87de..aecdfc41c114 100644
> --- a/arch/x86/lib/memcpy_64.S
> +++ b/arch/x86/lib/memcpy_64.S
> @@ -177,3 +177,161 @@ ENTRY(memcpy_orig)
>  .Lend:
>  	retq
>  ENDPROC(memcpy_orig)
> +
> +#ifndef CONFIG_UML
> +/*
> + * memcpy_trap - 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.
> + */
> +ENTRY(memcpy_trap)
> +	cmpl $8,%edx
> +	/* Less than 8 bytes? Go to byte copy loop */
> +	jb .L_no_whole_words
> +
> +	/* Check for bad alignment of source */
> +	testl $7,%esi
> +	/* Already aligned */
> +	jz .L_8byte_aligned
> +
> +	/* Copy one byte at a time until source is 8-byte aligned */
> +	movl %esi,%ecx
> +	andl $7,%ecx
> +	subl $8,%ecx
> +	negl %ecx
> +	subl %ecx,%edx
> +.L_copy_leading_bytes:
> +	movb (%rsi),%al
> +	movb %al,(%rdi)
> +	incq %rsi
> +	incq %rdi
> +	decl %ecx
> +	jnz .L_copy_leading_bytes
> +
> +.L_8byte_aligned:
> +	/* Figure out how many whole cache lines (64-bytes) to copy */
> +	movl %edx,%ecx
> +	andl $63,%edx
> +	shrl $6,%ecx
> +	jz .L_no_whole_cache_lines
> +
> +	/* Loop copying whole cache lines */
> +.L_cache_w0: movq (%rsi),%r8
> +.L_cache_w1: movq 1*8(%rsi),%r9
> +.L_cache_w2: movq 2*8(%rsi),%r10
> +.L_cache_w3: movq 3*8(%rsi),%r11
> +	movq %r8,(%rdi)
> +	movq %r9,1*8(%rdi)
> +	movq %r10,2*8(%rdi)
> +	movq %r11,3*8(%rdi)
> +.L_cache_w4: movq 4*8(%rsi),%r8
> +.L_cache_w5: movq 5*8(%rsi),%r9
> +.L_cache_w6: movq 6*8(%rsi),%r10
> +.L_cache_w7: movq 7*8(%rsi),%r11
> +	movq %r8,4*8(%rdi)
> +	movq %r9,5*8(%rdi)
> +	movq %r10,6*8(%rdi)
> +	movq %r11,7*8(%rdi)
> +	leaq 64(%rsi),%rsi
> +	leaq 64(%rdi),%rdi
> +	decl %ecx
> +	jnz .L_cache_w0
> +
> +	/* Are there any trailing 8-byte words? */
> +.L_no_whole_cache_lines:
> +	movl %edx,%ecx
> +	andl $7,%edx
> +	shrl $3,%ecx
> +	jz .L_no_whole_words
> +
> +	/* Copy trailing words */
> +.L_copy_trailing_words:
> +	movq (%rsi),%r8
> +	mov %r8,(%rdi)
> +	leaq 8(%rsi),%rsi
> +	leaq 8(%rdi),%rdi
> +	decl %ecx
> +	jnz .L_copy_trailing_words
> +
> +	/* Any trailing bytes? */
> +.L_no_whole_words:
> +	andl %edx,%edx
> +	jz .L_done_memcpy_trap
> +
> +	/* Copy trailing bytes */
> +	movl %edx,%ecx
> +.L_copy_trailing_bytes:
> +	movb (%rsi),%al
> +	movb %al,(%rdi)
> +	incq %rsi
> +	incq %rdi
> +	decl %ecx
> +	jnz .L_copy_trailing_bytes
> +
> +	/* Copy successful. Return .remain = 0, .trapnr = 0 */
> +.L_done_memcpy_trap:
> +	xorq %rax, %rax
> +	xorq %rdx, %rdx
> +	ret
> +
> +	.section .fixup,"ax"
> +	/*
> +	 * The machine check handler loaded %rax with trap number.
> +	 * We just need to make sure %edx has the number of
> +	 * bytes remaining.
> +	 */
> +.L_fix_leading_bytes:
> +	add %ecx,%edx
> +	ret
> +.L_fix_cache_w0:
> +	shl $6,%ecx
> +	add %ecx,%edx
> +	ret
> +.L_fix_cache_w1:
> +	shl $6,%ecx
> +	lea -8(%ecx,%edx),%edx
> +	ret
> +.L_fix_cache_w2:
> +	shl $6,%ecx
> +	lea -16(%ecx,%edx),%edx
> +	ret
> +.L_fix_cache_w3:
> +	shl $6,%ecx
> +	lea -24(%ecx,%edx),%edx
> +	ret
> +.L_fix_cache_w4:
> +	shl $6,%ecx
> +	lea -32(%ecx,%edx),%edx
> +	ret
> +.L_fix_cache_w5:
> +	shl $6,%ecx
> +	lea -40(%ecx,%edx),%edx
> +	ret
> +.L_fix_cache_w6:
> +	shl $6,%ecx
> +	lea -48(%ecx,%edx),%edx
> +	ret
> +.L_fix_cache_w7:
> +	shl $6,%ecx
> +	lea -56(%ecx,%edx),%edx
> +	ret
> +.L_fix_trailing_words:
> +	lea (%rdx,%rcx,8),%rdx
> +	ret
> +.L_fix_trailing_bytes:
> +	mov %ecx,%edx
> +	ret
> +	.previous
> +
> +	_ASM_EXTABLE_FAULT(.L_copy_leading_bytes,.L_fix_leading_bytes)
> +	_ASM_EXTABLE_FAULT(.L_cache_w0,.L_fix_cache_w0)
> +	_ASM_EXTABLE_FAULT(.L_cache_w1,.L_fix_cache_w1)
> +	_ASM_EXTABLE_FAULT(.L_cache_w3,.L_fix_cache_w2)
> +	_ASM_EXTABLE_FAULT(.L_cache_w3,.L_fix_cache_w3)
> +	_ASM_EXTABLE_FAULT(.L_cache_w4,.L_fix_cache_w4)
> +	_ASM_EXTABLE_FAULT(.L_cache_w5,.L_fix_cache_w5)
> +	_ASM_EXTABLE_FAULT(.L_cache_w6,.L_fix_cache_w6)
> +	_ASM_EXTABLE_FAULT(.L_cache_w7,.L_fix_cache_w7)
> +	_ASM_EXTABLE_FAULT(.L_copy_trailing_words,.L_fix_trailing_words)
> +	_ASM_EXTABLE_FAULT(.L_copy_trailing_bytes,.L_fix_trailing_bytes)
> +#endif

Ok, I absolutely love this assembly code, it's already a lot easier to read than 
95% of the x86 assembly code we have today!

There's two minor things I've noticed:

1) please put a space between instruction operands, i.e.:

-	shl $6,%ecx
+	shl $6, %ecx

2)

There's a way to make the exception fixup stubs more readable, by aligning them 
vertically, via something like:

.L_fix_cache_w0:	shl $6, %ecx; add %ecx, %edx;		ret
.L_fix_cache_w1:	shl $6, %ecx; lea  -8(%ecx,%edx), %edx;	ret
.L_fix_cache_w2:	shl $6, %ecx; lea -16(%ecx,%edx), %edx;	ret
.L_fix_cache_w3:	shl $6, %ecx; lea -24(%ecx,%edx), %edx; ret
.L_fix_cache_w4:	shl $6, %ecx; lea -32(%ecx,%edx), %edx; ret
.L_fix_cache_w5:	shl $6, %ecx; lea -40(%ecx,%edx), %edx; ret
.L_fix_cache_w6:	shl $6, %ecx; lea -48(%ecx,%edx), %edx; ret
.L_fix_cache_w7:	shl $6, %ecx; lea -56(%ecx,%edx), %edx; ret

this also makes it a lot easier to check the correctness of the fixup stubs. Also 
this layout makes it clear that the first fixup stub uses 'ADD', while the others 
use LEA, etc.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1338387 — Re: [PATCH v13] x86, mce: Add memcpy_trap()

Make use of the EXTABLE_FAULT exception table entries. This routine
returns a structure to indicate the result of the copy:

struct mcsafe_ret {
        u64 trap_nr;
        u64 bytes_left;
};

If the copy is successful, then both 'trap_nr' and 'bytes_left' are zero.

If we faulted during the copy, then 'trap_nr' will say which type
of trap (X86_TRAP_PF or X86_TRAP_MC) and 'bytes_left' says how many
bytes were not copied.

Note that this is probably the first of several copy functions.
We can make new ones for non-temporal cache handling etc.

Reviewed-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
V12-V13
Ingo: Separate instruction arguments with a ", "
	Note that I didn't add spaces after "," within an argument.
	E.g. "lea (%rdx,%rcx,8), %rdx"
	Did you want them there too? I don't think they help as much there.
Ingo: More readable layout for fixup stubs

 arch/x86/include/asm/string_64.h |  26 ++++++++
 arch/x86/kernel/x8664_ksyms_64.c |   2 +
 arch/x86/lib/memcpy_64.S         | 128 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 156 insertions(+)

diff --git a/arch/x86/include/asm/string_64.h b/arch/x86/include/asm/string_64.h
index ff8b9a17dc4b..65e5793b7590 100644
--- a/arch/x86/include/asm/string_64.h
+++ b/arch/x86/include/asm/string_64.h
@@ -78,6 +78,32 @@ int strcmp(const char *cs, const char *ct);
 #define memset(s, c, n) __memset(s, c, n)
 #endif
 
+/**
+ * struct memcpy_trap_ret - return value from memcpy_trap()
+ *
+ * @trap_nr	x86 trap number if the copy failed
+ * @bytes_left	zero for successful copy else number of bytes not copied
+ */
+struct memcpy_trap_ret {
+	u64 trap_nr;
+	u64 bytes_left;
+};
+
+/**
+ * memcpy_trap - copy memory with indication if a trap interrupted the copy
+ *
+ * @dst:	destination address
+ * @src:	source address
+ * @cnt:	number of bytes to copy
+ *
+ * Low level memory copy function that catches traps and indicates whether
+ * the copy succeeded and if not, why it failed.
+ *
+ * Return is struct memcpy_trap_ret which provides both the number of bytes
+ * not copied and the reason for the failure.
+ */
+struct memcpy_trap_ret memcpy_trap(void *dst, const void __user *src, size_t cnt);
+
 #endif /* __KERNEL__ */
 
 #endif /* _ASM_X86_STRING_64_H */
diff --git a/arch/x86/kernel/x8664_ksyms_64.c b/arch/x86/kernel/x8664_ksyms_64.c
index a0695be19864..40866e2cbcc4 100644
--- a/arch/x86/kernel/x8664_ksyms_64.c
+++ b/arch/x86/kernel/x8664_ksyms_64.c
@@ -37,6 +37,8 @@ EXPORT_SYMBOL(__copy_user_nocache);
 EXPORT_SYMBOL(_copy_from_user);
 EXPORT_SYMBOL(_copy_to_user);
 
+EXPORT_SYMBOL_GPL(memcpy_trap);
+
 EXPORT_SYMBOL(copy_page);
 EXPORT_SYMBOL(clear_page);
 
diff --git a/arch/x86/lib/memcpy_64.S b/arch/x86/lib/memcpy_64.S
index 16698bba87de..b8dccda0575d 100644
--- a/arch/x86/lib/memcpy_64.S
+++ b/arch/x86/lib/memcpy_64.S
@@ -177,3 +177,131 @@ ENTRY(memcpy_orig)
 .Lend:
 	retq
 ENDPROC(memcpy_orig)
+
+#ifndef CONFIG_UML
+/*
+ * memcpy_trap - 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.
+ */
+ENTRY(memcpy_trap)
+	cmpl $8, %edx
+	/* Less than 8 bytes? Go to byte copy loop */
+	jb .L_no_whole_words
+
+	/* Check for bad alignment of source */
+	testl $7, %esi
+	/* Already aligned */
+	jz .L_8byte_aligned
+
+	/* Copy one byte at a time until source is 8-byte aligned */
+	movl %esi, %ecx
+	andl $7, %ecx
+	subl $8, %ecx
+	negl %ecx
+	subl %ecx, %edx
+.L_copy_leading_bytes:
+	movb (%rsi), %al
+	movb %al, (%rdi)
+	incq %rsi
+	incq %rdi
+	decl %ecx
+	jnz .L_copy_leading_bytes
+
+.L_8byte_aligned:
+	/* Figure out how many whole cache lines (64-bytes) to copy */
+	movl %edx, %ecx
+	andl $63, %edx
+	shrl $6, %ecx
+	jz .L_no_whole_cache_lines
+
+	/* Loop copying whole cache lines */
+.L_cache_w0: movq (%rsi), %r8
+.L_cache_w1: movq 1*8(%rsi), %r9
+.L_cache_w2: movq 2*8(%rsi), %r10
+.L_cache_w3: movq 3*8(%rsi), %r11
+	movq %r8, (%rdi)
+	movq %r9, 1*8(%rdi)
+	movq %r10, 2*8(%rdi)
+	movq %r11, 3*8(%rdi)
+.L_cache_w4: movq 4*8(%rsi), %r8
+.L_cache_w5: movq 5*8(%rsi), %r9
+.L_cache_w6: movq 6*8(%rsi), %r10
+.L_cache_w7: movq 7*8(%rsi), %r11
+	movq %r8, 4*8(%rdi)
+	movq %r9, 5*8(%rdi)
+	movq %r10, 6*8(%rdi)
+	movq %r11, 7*8(%rdi)
+	leaq 64(%rsi), %rsi
+	leaq 64(%rdi), %rdi
+	decl %ecx
+	jnz .L_cache_w0
+
+	/* Are there any trailing 8-byte words? */
+.L_no_whole_cache_lines:
+	movl %edx, %ecx
+	andl $7, %edx
+	shrl $3, %ecx
+	jz .L_no_whole_words
+
+	/* Copy trailing words */
+.L_copy_trailing_words:
+	movq (%rsi), %r8
+	mov %r8, (%rdi)
+	leaq 8(%rsi), %rsi
+	leaq 8(%rdi), %rdi
+	decl %ecx
+	jnz .L_copy_trailing_words
+
+	/* Any trailing bytes? */
+.L_no_whole_words:
+	andl %edx, %edx
+	jz .L_done_memcpy_trap
+
+	/* Copy trailing bytes */
+	movl %edx, %ecx
+.L_copy_trailing_bytes:
+	movb (%rsi), %al
+	movb %al, (%rdi)
+	incq %rsi
+	incq %rdi
+	decl %ecx
+	jnz .L_copy_trailing_bytes
+
+	/* Copy successful. Return .remain = 0, .trapnr = 0 */
+.L_done_memcpy_trap:
+	xorq %rax, %rax
+	xorq %rdx, %rdx
+	ret
+
+	.section .fixup, "ax"
+	/*
+	 * The machine check handler loaded %rax with trap number.
+	 * We just need to make sure %edx has the number of
+	 * bytes remaining.
+	 */
+.L_fix_leading_bytes:	add %ecx, %edx; ret
+.L_fix_cache_w0:	shl $6, %ecx; add %ecx, %edx; ret
+.L_fix_cache_w1:	shl $6, %ecx; lea -8(%ecx,%edx), %edx; ret
+.L_fix_cache_w2:	shl $6, %ecx; lea -16(%ecx,%edx), %edx; ret
+.L_fix_cache_w3:	shl $6, %ecx; lea -24(%ecx,%edx), %edx; ret
+.L_fix_cache_w4:	shl $6, %ecx; lea -32(%ecx,%edx), %edx; ret
+.L_fix_cache_w5:	shl $6, %ecx; lea -40(%ecx,%edx), %edx; ret
+.L_fix_cache_w6:	shl $6, %ecx; lea -48(%ecx,%edx), %edx; ret
+.L_fix_cache_w7:	shl $6, %ecx; lea -56(%ecx,%edx), %edx; ret
+.L_fix_trailing_words:	lea (%rdx,%rcx,8), %rdx; ret
+.L_fix_trailing_bytes:	mov %ecx, %edx; ret
+	.previous
+
+	_ASM_EXTABLE_FAULT(.L_copy_leading_bytes, .L_fix_leading_bytes)
+	_ASM_EXTABLE_FAULT(.L_cache_w0, .L_fix_cache_w0)
+	_ASM_EXTABLE_FAULT(.L_cache_w1, .L_fix_cache_w1)
+	_ASM_EXTABLE_FAULT(.L_cache_w3, .L_fix_cache_w2)
+	_ASM_EXTABLE_FAULT(.L_cache_w3, .L_fix_cache_w3)
+	_ASM_EXTABLE_FAULT(.L_cache_w4, .L_fix_cache_w4)
+	_ASM_EXTABLE_FAULT(.L_cache_w5, .L_fix_cache_w5)
+	_ASM_EXTABLE_FAULT(.L_cache_w6, .L_fix_cache_w6)
+	_ASM_EXTABLE_FAULT(.L_cache_w7, .L_fix_cache_w7)
+	_ASM_EXTABLE_FAULT(.L_copy_trailing_words, .L_fix_trailing_words)
+	_ASM_EXTABLE_FAULT(.L_copy_trailing_bytes, .L_fix_trailing_bytes)
+#endif
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web