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

[Ingo Molnar]

* Tony Luck <tony.luck@xxxxxxxxx> 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