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

Groups > linux.kernel > #1305099 > unrolled thread

[PATCH v8 3/3] x86, mce: Add __mcsafe_copy()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v8 3/3] x86, mce: Add __mcsafe_copy() Tony Luck <tony.luck@intel.com> - 2016-01-09 01:20 +0100
    Re: [PATCH v8 3/3] x86, mce: Add __mcsafe_copy() Andy Lutomirski <luto@amacapital.net> - 2016-01-09 02:50 +0100
      Re: [PATCH v8 3/3] x86, mce: Add __mcsafe_copy() Tony Luck <tony.luck@gmail.com> - 2016-01-09 18:50 +0100
        Re: [PATCH v8 3/3] x86, mce: Add __mcsafe_copy() Andy Lutomirski <luto@amacapital.net> - 2016-01-09 19:00 +0100
          Re: [PATCH v8 3/3] x86, mce: Add __mcsafe_copy() Tony Luck <tony.luck@gmail.com> - 2016-01-09 20:50 +0100
            Re: [PATCH v8 3/3] x86, mce: Add __mcsafe_copy() Dan Williams <dan.j.williams@intel.com> - 2016-01-09 23:20 +0100
              Re: [PATCH v8 3/3] x86, mce: Add __mcsafe_copy() Andy Lutomirski <luto@amacapital.net> - 2016-01-09 23:40 +0100
                Re: [PATCH v8 3/3] x86, mce: Add __mcsafe_copy() Dan Williams <dan.j.williams@intel.com> - 2016-01-10 01:30 +0100
                  Re: [PATCH v8 3/3] x86, mce: Add __mcsafe_copy() Tony Luck <tony.luck@gmail.com> - 2016-01-10 02:50 +0100
                    Re: [PATCH v8 3/3] x86, mce: Add __mcsafe_copy() Borislav Petkov <bp@alien8.de> - 2016-01-10 12:30 +0100
                      Re: [PATCH v8 3/3] x86, mce: Add __mcsafe_copy() Ingo Molnar <mingo@kernel.org> - 2016-01-11 11:50 +0100
                        Re: [PATCH v8 3/3] x86, mce: Add __mcsafe_copy() Tony Luck <tony.luck@gmail.com> - 2016-01-14 00:30 +0100
                          Re: [PATCH v8 3/3] x86, mce: Add __mcsafe_copy() Borislav Petkov <bp@alien8.de> - 2016-01-14 05:50 +0100
      Re: [PATCH v8 3/3] x86, mce: Add __mcsafe_copy() "Luck, Tony" <tony.luck@intel.com> - 2016-01-12 01:30 +0100
        Re: [PATCH v8 3/3] x86, mce: Add __mcsafe_copy() Andy Lutomirski <luto@amacapital.net> - 2016-01-12 01:40 +0100
        Re: [PATCH v8 3/3] x86, mce: Add __mcsafe_copy() Andy Lutomirski <luto@amacapital.net> - 2016-01-12 01:40 +0100

#1305099 — [PATCH v8 3/3] 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.

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         | 133 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 143 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..96434edd7430 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(__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..195ff0144152 100644
--- a/arch/x86/lib/memcpy_64.S
+++ b/arch/x86/lib/memcpy_64.S
@@ -177,3 +177,136 @@ ENTRY(memcpy_orig)
 .Lend:
 	retq
 ENDPROC(memcpy_orig)
+
+/*
+ * __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 */
+	movl %esi,%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 0b
+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
+	mov %r8,(%rdi)
+	mov %r9,1*8(%rdi)
+	mov %r10,2*8(%rdi)
+	mov %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
+	mov %r8,4*8(%rdi)
+	mov %r9,5*8(%rdi)
+	mov %r10,6*8(%rdi)
+	mov %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
+	mov %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:	xorq %rax, %rax
+	xorq %rdx, %rdx
+	sfence
+	/* copy successful. return 0 */
+	ret
+
+	.section .fixup,"ax"
+	/* fixups for machine check */
+30:
+	add %ecx,%edx
+	jmp 100f
+31:
+	shl $6,%ecx
+	add %ecx,%edx
+	jmp 100f
+32:
+	shl $6,%ecx
+	lea -8(%ecx,%edx),%edx
+	jmp 100f
+33:
+	shl $6,%ecx
+	lea -16(%ecx,%edx),%edx
+	jmp 100f
+34:
+	shl $6,%ecx
+	lea -24(%ecx,%edx),%edx
+	jmp 100f
+35:
+	shl $6,%ecx
+	lea -32(%ecx,%edx),%edx
+	jmp 100f
+36:
+	shl $6,%ecx
+	lea -40(%ecx,%edx),%edx
+	jmp 100f
+37:
+	shl $6,%ecx
+	lea -48(%ecx,%edx),%edx
+	jmp 100f
+38:
+	shl $6,%ecx
+	lea -56(%ecx,%edx),%edx
+	jmp 100f
+39:
+	lea (%rdx,%rcx,8),%rdx
+	jmp 100f
+40:
+	mov %ecx,%edx
+100:
+	sfence
+
+	/* %rax set the fault number in fixup_exception() */
+	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)
-- 
2.1.4

[toc] | [next] | [standalone]

#1305133

On Jan 8, 2016 4:19 PM, "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:

Perhaps this is silly, but could we make this feature depend on ERMS
and thus make the code a lot simpler?

Also, what's the sfence for?  You don't seem to be using any
non-temporal operations.

--Andy

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

#1305336

On Fri, Jan 8, 2016 at 5:49 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Jan 8, 2016 4:19 PM, "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:
>
> Perhaps this is silly, but could we make this feature depend on ERMS
> and thus make the code a lot simpler?

ERMS?

> Also, what's the sfence for?  You don't seem to be using any
> non-temporal operations.

Ah - left over from the original function that this
was cloned from (which did use non-temporal
operations).  Will delete.

-Tony

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

#1305338

On Sat, Jan 9, 2016 at 9:48 AM, Tony Luck <tony.luck@gmail.com> wrote:
> On Fri, Jan 8, 2016 at 5:49 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Jan 8, 2016 4:19 PM, "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:
>>
>> Perhaps this is silly, but could we make this feature depend on ERMS
>> and thus make the code a lot simpler?
>
> ERMS?

It's the fast string extension, aka Enhanced REP MOV STOS.  On CPUs
with that feature (and not disabled via MSR), plain ol' rep movs is
the fastest way to copy bytes.  I think this includes all Intel CPUs
from SNB onwards.

--Andy

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

#1305352

On Sat, Jan 9, 2016 at 9:57 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Sat, Jan 9, 2016 at 9:48 AM, Tony Luck <tony.luck@gmail.com> wrote:
>> ERMS?
>
> It's the fast string extension, aka Enhanced REP MOV STOS.  On CPUs
> with that feature (and not disabled via MSR), plain ol' rep movs is
> the fastest way to copy bytes.  I think this includes all Intel CPUs
> from SNB onwards.

Ah ... very fast at copying .. but currently not machine check recoverable.

-Tony

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

#1305373

On Sat, Jan 9, 2016 at 11:39 AM, Tony Luck <tony.luck@gmail.com> wrote:
> On Sat, Jan 9, 2016 at 9:57 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Sat, Jan 9, 2016 at 9:48 AM, Tony Luck <tony.luck@gmail.com> wrote:
>>> ERMS?
>>
>> It's the fast string extension, aka Enhanced REP MOV STOS.  On CPUs
>> with that feature (and not disabled via MSR), plain ol' rep movs is
>> the fastest way to copy bytes.  I think this includes all Intel CPUs
>> from SNB onwards.
>
> Ah ... very fast at copying .. but currently not machine check recoverable.

Hmm, I assume for the pmem driver I'll want to check at runtime if the
cpu has machine check recovery and fall back to the faster copy if
it's not available?

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

#1305377

On Sat, Jan 9, 2016 at 2:15 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Sat, Jan 9, 2016 at 11:39 AM, Tony Luck <tony.luck@gmail.com> wrote:
>> On Sat, Jan 9, 2016 at 9:57 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>> On Sat, Jan 9, 2016 at 9:48 AM, Tony Luck <tony.luck@gmail.com> wrote:
>>>> ERMS?
>>>
>>> It's the fast string extension, aka Enhanced REP MOV STOS.  On CPUs
>>> with that feature (and not disabled via MSR), plain ol' rep movs is
>>> the fastest way to copy bytes.  I think this includes all Intel CPUs
>>> from SNB onwards.
>>
>> Ah ... very fast at copying .. but currently not machine check recoverable.
>
> Hmm, I assume for the pmem driver I'll want to check at runtime if the
> cpu has machine check recovery and fall back to the faster copy if
> it's not available?

Shouldn't that logic live in the mcsafe_copy routine itself rather
than being delegated to callers?

--Andy

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

#1305398

On Sat, Jan 9, 2016 at 2:33 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Sat, Jan 9, 2016 at 2:15 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>> On Sat, Jan 9, 2016 at 11:39 AM, Tony Luck <tony.luck@gmail.com> wrote:
>>> On Sat, Jan 9, 2016 at 9:57 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>>> On Sat, Jan 9, 2016 at 9:48 AM, Tony Luck <tony.luck@gmail.com> wrote:
>>>>> ERMS?
>>>>
>>>> It's the fast string extension, aka Enhanced REP MOV STOS.  On CPUs
>>>> with that feature (and not disabled via MSR), plain ol' rep movs is
>>>> the fastest way to copy bytes.  I think this includes all Intel CPUs
>>>> from SNB onwards.
>>>
>>> Ah ... very fast at copying .. but currently not machine check recoverable.
>>
>> Hmm, I assume for the pmem driver I'll want to check at runtime if the
>> cpu has machine check recovery and fall back to the faster copy if
>> it's not available?
>
> Shouldn't that logic live in the mcsafe_copy routine itself rather
> than being delegated to callers?
>

Yes, please.

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

#1305406

On Sat, Jan 9, 2016 at 4:23 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Sat, Jan 9, 2016 at 2:33 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> Shouldn't that logic live in the mcsafe_copy routine itself rather
>> than being delegated to callers?
>>
>
> Yes, please.

Yes - we should have some of that fancy self-patching code that
redirects to the optimal routine for the cpu model we are running
on.

BUT ... it's all going to be very messy.  We don't have any CPUID
capability bits to say whether we support recovery, or which instructions
are good/bad choices for recovery. You might think that MCG_CAP{24}
which is described as "software error recovery" (or some such) would
be a good clue, but you'd be wrong. The bit got a little overloaded and
there are cpus that set it, but won't recover.

Only Intel(R) Xeon(R) branded cpus can recover, but not all. The story so far:

Nehalem, Westmere: E7 models support SRAO recovery (patrol scrub,
cache eviction). Not relevant for this e-mail thread.

Sandy Bridge: Some "advanced RAS" skus will recover from poison reads
(these have E5 model names, there was no E7 in this generation)

Ivy Bridge: Xeon E5-* models do not recover. E7-* models do recover.
Note E5 and E7 have the same CPUID model number.

Haswell: Same as Ivy Bridge

Broadwell/Sky Lake: Xeon not released yet ... can't talk about them.

Linux code recently got some recovery bits for AMD cpus ... I don't
know what the story is on which models support this,

-Tony

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

#1305541

On Sat, Jan 09, 2016 at 05:40:05PM -0800, Tony Luck wrote:
> BUT ... it's all going to be very messy.  We don't have any CPUID
> capability bits to say whether we support recovery, or which instructions
> are good/bad choices for recovery.

We can always define synthetic ones and set them after having checked
MCA capability bits, f/m/s, etc., maybe even based on the list you're
supplying...

> Linux code recently got some recovery bits for AMD cpus ... I don't
> know what the story is on which models support this,

You mean this?

                /*
                 * overflow_recov is supported for F15h Models 00h-0fh
                 * even though we don't have a CPUID bit for it.
                 */
                if (c->x86 == 0x15 && c->x86_model <= 0xf)
                        mce_flags.overflow_recov = 1;

If so, that's just an improvement which makes MCi_STATUS[Overflow] MCEs
non-fatal.

-- 
Regards/Gruss,
    Boris.

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

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

#1306052

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

> On Sat, Jan 09, 2016 at 05:40:05PM -0800, Tony Luck wrote:
> > BUT ... it's all going to be very messy.  We don't have any CPUID
> > capability bits to say whether we support recovery, or which instructions
> > are good/bad choices for recovery.
> 
> We can always define synthetic ones and set them after having checked
> MCA capability bits, f/m/s, etc., maybe even based on the list you're
> supplying...

So such a synthetic CPUID bit would definitely be useful.

Also, knowing whether a memcpy function is recoverable or not, should not be 
delegated to callers: there should be the regular memcpy APIs, plus new APIs that 
do everything they can to provide recoverable memory copies. Whether it's achieved 
via flag checking, a function pointer or code patching is an implementation detail 
that's not visible to drivers making use of the new facility.

I'd go for the simplest, most robust solution initially, also perhaps with boot 
time messages to make sure users know which variant is used and now.

Thanks,

	Ingo

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

#1308895

On Mon, Jan 11, 2016 at 2:44 AM, Ingo Molnar <mingo@kernel.org> wrote:
> So such a synthetic CPUID bit would definitely be useful.
>
> Also, knowing whether a memcpy function is recoverable or not, should not be
> delegated to callers: there should be the regular memcpy APIs, plus new APIs that
> do everything they can to provide recoverable memory copies. Whether it's achieved
> via flag checking, a function pointer or code patching is an implementation detail
> that's not visible to drivers making use of the new facility.
>
> I'd go for the simplest, most robust solution initially, also perhaps with boot
> time messages to make sure users know which variant is used and now.

Are there some examples of synthetic CPUID bits?  I grepped around and
found a handful of places making ad hoc decisions based on sub-strings of
x86_model_id[] ... but didn't find any systematic approach.

-Tony

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

#1308990

On Wed, Jan 13, 2016 at 03:22:58PM -0800, Tony Luck wrote:
> Are there some examples of synthetic CPUID bits?

X86_FEATURE_ALWAYS is one. The others got renamed into X86_BUG_* ones,
the remaining mechanism is the same, though.

-- 
Regards/Gruss,
    Boris.

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

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

#1306851

On Fri, Jan 08, 2016 at 05:49:30PM -0800, Andy Lutomirski wrote:
> Also, what's the sfence for?  You don't seem to be using any
> non-temporal operations.

So I deleted the "sfence" and now I just have a comment
at the 100: label.

37:
        shl $6,%ecx
        lea -48(%ecx,%edx),%edx
        jmp 100f
38:
        shl $6,%ecx
        lea -56(%ecx,%edx),%edx
        jmp 100f
39:
        lea (%rdx,%rcx,8),%rdx
        jmp 100f
40:
        mov %ecx,%edx
100:
        /* %rax set the fault number in fixup_exception() */
        ret

Should I just change all the "jmp 100f" into "ret"?  There
aren't any tools that will be confused that the function
has 10 returns, are there?

-Tony

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

#1306861

On Mon, Jan 11, 2016 at 4:26 PM, Luck, Tony <tony.luck@intel.com> wrote:
> On Fri, Jan 08, 2016 at 05:49:30PM -0800, Andy Lutomirski wrote:
>> Also, what's the sfence for?  You don't seem to be using any
>> non-temporal operations.
>
> So I deleted the "sfence" and now I just have a comment
> at the 100: label.
>
> 37:
>         shl $6,%ecx
>         lea -48(%ecx,%edx),%edx
>         jmp 100f
> 38:
>         shl $6,%ecx
>         lea -56(%ecx,%edx),%edx
>         jmp 100f
> 39:
>         lea (%rdx,%rcx,8),%rdx
>         jmp 100f
> 40:
>         mov %ecx,%edx
> 100:
>         /* %rax set the fault number in fixup_exception() */
>         ret
>
> Should I just change all the "jmp 100f" into "ret"?  There
> aren't any tools that will be confused that the function
> has 10 returns, are there?
>

Given that gcc does that too, it should be fine.

--Andy\

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

#1306863

On Mon, Jan 11, 2016 at 4:26 PM, Luck, Tony <tony.luck@intel.com> wrote:
> On Fri, Jan 08, 2016 at 05:49:30PM -0800, Andy Lutomirski wrote:
>> Also, what's the sfence for?  You don't seem to be using any
>> non-temporal operations.
>
> So I deleted the "sfence" and now I just have a comment
> at the 100: label.
>
> 37:
>         shl $6,%ecx
>         lea -48(%ecx,%edx),%edx
>         jmp 100f
> 38:
>         shl $6,%ecx
>         lea -56(%ecx,%edx),%edx
>         jmp 100f
> 39:
>         lea (%rdx,%rcx,8),%rdx
>         jmp 100f
> 40:
>         mov %ecx,%edx
> 100:
>         /* %rax set the fault number in fixup_exception() */
>         ret
>
> Should I just change all the "jmp 100f" into "ret"?  There
> aren't any tools that will be confused that the function
> has 10 returns, are there?
>

Given that gcc does that too, it should be fine.

--Andy\

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web