Messages in this thread Patch in this message | | | Date | Thu, 18 Feb 2016 09:21:07 +0100 | From | Ingo Molnar <> | Subject | 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
| |