[PATCH] x86/uaccess: Improve __try_cmpxchg64_user_asm for x86_32

[PATCH] x86/uaccess: Improve __try_cmpxchg64_user_asm for x86_32

[Uros Bizjak]

Improve __try_cmpxcgh64_user_asm for !CONFIG_CC_HAS_ASM_GOTO_TIED_OUTPUT
by relaxing the output register constraint from "c" to "q" constraint,
which allows the compiler to choose between %ecx or %ebx register.

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/uaccess.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 35f222aa66bf..9fae2a1cc267 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -448,7 +448,7 @@ do {									\
 
 #ifdef CONFIG_X86_32
 /*
- * Unlike the normal CMPXCHG, hardcode ECX for both success/fail and error.
+ * Unlike the normal CMPXCHG, use output GPR for both success/fail and error.
  * There are only six GPRs available and four (EAX, EBX, ECX, and EDX) are
  * hardcoded by CMPXCHG8B, leaving only ESI and EDI.  If the compiler uses
  * both ESI and EDI for the memory operand, compilation will fail if the error
@@ -461,11 +461,12 @@ do {									\
 	__typeof__(*(_ptr)) __new = (_new);				\
 	asm volatile("\n"						\
 		     "1: " LOCK_PREFIX "cmpxchg8b %[ptr]\n"		\
-		     "mov $0, %%ecx\n\t"				\
-		     "setz %%cl\n"					\
+		     "mov $0, %[result]\n\t"				\
+		     "setz %b[result]\n"				\
 		     "2:\n"						\
-		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG, %%ecx) \
-		     : [result]"=c" (__result),				\
+		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG,	\
+					   %[result])			\
+		     : [result] "=q" (__result),			\
 		       "+A" (__old),					\
 		       [ptr] "+m" (*_ptr)				\
 		     : "b" ((u32)__new),				\
-- 
2.35.1

Re: [PATCH] x86/uaccess: Improve __try_cmpxchg64_user_asm for x86_32

[Sean Christopherson]

On Sun, May 15, 2022, Uros Bizjak wrote:
> Improve __try_cmpxcgh64_user_asm for !CONFIG_CC_HAS_ASM_GOTO_TIED_OUTPUT
> by relaxing the output register constraint from "c" to "q" constraint,
> which allows the compiler to choose between %ecx or %ebx register.
> 
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/include/asm/uaccess.h | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index 35f222aa66bf..9fae2a1cc267 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -448,7 +448,7 @@ do {									\
>  
>  #ifdef CONFIG_X86_32
>  /*
> - * Unlike the normal CMPXCHG, hardcode ECX for both success/fail and error.
> + * Unlike the normal CMPXCHG, use output GPR for both success/fail and error.
>   * There are only six GPRs available and four (EAX, EBX, ECX, and EDX) are
>   * hardcoded by CMPXCHG8B, leaving only ESI and EDI.  If the compiler uses
>   * both ESI and EDI for the memory operand, compilation will fail if the error
> @@ -461,11 +461,12 @@ do {									\
>  	__typeof__(*(_ptr)) __new = (_new);				\
>  	asm volatile("\n"						\
>  		     "1: " LOCK_PREFIX "cmpxchg8b %[ptr]\n"		\
> -		     "mov $0, %%ecx\n\t"				\
> -		     "setz %%cl\n"					\
> +		     "mov $0, %[result]\n\t"				\
> +		     "setz %b[result]\n"				\
>  		     "2:\n"						\
> -		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG, %%ecx) \
> -		     : [result]"=c" (__result),				\
> +		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG,	\
> +					   %[result])			\

Huh.  I remember trying this, but either I could never get one of these formats
correct (most likely) or I got sidetracked by the clang-13 bug and never circled
back to allowing EBX.

Regardless, clang-13 and gcc-11 are happy with this,

Reviewed-by: Sean Christopherson <seanjc@google.com>

> +		     : [result] "=q" (__result),			\
>  		       "+A" (__old),					\
>  		       [ptr] "+m" (*_ptr)				\
>  		     : "b" ((u32)__new),				\
> -- 
> 2.35.1
>