Re: [PATCH 1/2] x86/lib/copy_user_64.S: Handle 4-byte uncached copy

[Ingo Molnar]

* Toshi Kani <toshi.kani@xxxxxxx> wrote:

> Data corruption issues were observed in tests which initiated
> a system crash while accessing BTT devices.  This problem is
> reproducible.
> 
> The BTT driver calls pmem_rw_bytes() to update data in pmem
> devices.  This interface calls __copy_user_nocache(), which
> uses non-temporal stores so that the stores to pmem are
> persistent.
> 
> __copy_user_nocache() uses non-temporal stores when a request
> size is 8 bytes or larger (and is aligned by 8 bytes).  The
> BTT driver updates the BTT map table, which entry size is
> 4 bytes.  Therefore, updates to the map table entries remain
> cached, and are not written to pmem after a crash.
> 
> Change __copy_user_nocache() to use non-temporal store when
> a request size is 4 bytes.  The change extends the byte-copy
> path for a less-than-8-bytes request, and does not add any
> overhead to the regular path.
> 
> Also add comments to clarify the cases cached copy is used.
> 
> Reported-and-tested-by: Micah Parrish <micah.parrish@xxxxxxx>
> Reported-and-tested-by: Brian Boylston <brian.boylston@xxxxxxx>
> Signed-off-by: Toshi Kani <toshi.kani@xxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: H. Peter Anvin <hpa@xxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Cc: Ross Zwisler <ross.zwisler@xxxxxxxxxxxxxxx>
> Cc: Vishal Verma <vishal.l.verma@xxxxxxxxx>
> ---
>  arch/x86/lib/copy_user_64.S |   44 ++++++++++++++++++++++++++++++++-----------
>  1 file changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/lib/copy_user_64.S b/arch/x86/lib/copy_user_64.S
> index 982ce34..84b5578 100644
> --- a/arch/x86/lib/copy_user_64.S
> +++ b/arch/x86/lib/copy_user_64.S
> @@ -232,12 +232,17 @@ ENDPROC(copy_user_enhanced_fast_string)
>  
>  /*
>   * copy_user_nocache - Uncached memory copy with exception handling
> - * This will force destination/source out of cache for more performance.
> + * This will force destination out of cache for more performance.
> + *
> + * Note: Cached memory copy is used when destination or size is not
> + * naturally aligned. That is:
> + *  - Require 8-byte alignment when size is 8 bytes or larger.
> + *  - Require 4-byte alignment when size is 4 bytes.
>   */
>  ENTRY(__copy_user_nocache)
>  	ASM_STAC
>  	cmpl $8,%edx
> -	jb 20f		/* less then 8 bytes, go to byte copy loop */
> +	jb 20f
>  	ALIGN_DESTINATION
>  	movl %edx,%ecx
>  	andl $63,%edx
> @@ -274,15 +279,28 @@ ENTRY(__copy_user_nocache)
>  	decl %ecx
>  	jnz 18b
>  20:	andl %edx,%edx
> -	jz 23f
> +	jz 26f
> +	movl %edi,%ecx
> +	andl $3,%ecx
> +	jnz 23f
>  	movl %edx,%ecx
> -21:	movb (%rsi),%al
> -22:	movb %al,(%rdi)
> +	andl $3,%edx
> +	shrl $2,%ecx
> +	jz 23f
> +21:	movl (%rsi),%r8d
> +22:	movnti %r8d,(%rdi)
> +	leaq 4(%rsi),%rsi
> +	leaq 4(%rdi),%rdi
> +	andl %edx,%edx
> +	jz 26f
> +23:	movl %edx,%ecx
> +24:	movb (%rsi),%al
> +25:	movb %al,(%rdi)

So at minimum this patch needs to add quite a few comments to explain the 
alignment dependent control flow.

Assembly code is hard enough to read as-is. Adding 20 more lines with zero in-line 
comments is a mistake.

Btw., while at it, please add comments for the control flow of the whole function. 
Above a certain complexity that is a must for assembly functions.

Thanks,

	Ingo