Re: [PATCH] KVM: x86: Add support for cmpxchg16b emulation

[Paolo Bonzini <pbonzini@redhat.com>]

On 3/6/26 16:38, Sean Christopherson wrote:
>> -	if (ctxt->dst.bytes == 16)
>> +	/* Use of the REX.W prefix promotes operation to 128 bits */
>> +	if (ctxt->rex_bits & REX_W)
> 
> Uh, yeah, and the caller specifically pivoted on this size.  I'm a-ok with a
> sanity check, but it should be exactly that, e.g.
> 
> 	if (WARN_ON_ONCE(8 + (ctxt->rex_bits & REX_W) * 8 != ctxt->dst.bytes))
> 		return X86EMUL_UNHANDLEABLE;


> And then emulator_cmpxchg_emulated() should be taught to do cmpxchg16b itself:
> 
> 	/* guests cmpxchg8b have to be emulated atomically */
> 	if (bytes > 8 || (bytes & (bytes - 1)))
> 		goto emul_write;
> 
> That might be a big lift, e.g. to get a 16-byte uaccess version.  If it's
> unreasonably difficult, we should reject emulation of LOCK CMPXCHG16B.

It's actually pretty easy because a kind soul already wrote the
cmpxchg8b version of the macros.  Compile-tested only (and checked
that the asm does have the instruction):

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 3a7755c1a4410..f40e815263233 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -388,14 +388,13 @@ do {									\
  		*_old = __old;						\
  	likely(success);					})
  
-#ifdef CONFIG_X86_32
-#define __try_cmpxchg64_user_asm(_ptr, _pold, _new, label)	({	\
+#define __try_cmpxchg8b_16b_user_asm(_b, _ptr, _pold, _new, label)({	\
  	bool success;							\
  	__typeof__(_ptr) _old = (__typeof__(_ptr))(_pold);		\
  	__typeof__(*(_ptr)) __old = *_old;				\
  	__typeof__(*(_ptr)) __new = (_new);				\
  	asm_goto_output("\n"						\
-		     "1: " LOCK_PREFIX "cmpxchg8b %[ptr]\n"		\
+		     "1: " LOCK_PREFIX "cmpxchg" #_b "b %[ptr]\n"	\
  		     _ASM_EXTABLE_UA(1b, %l[label])			\
  		     : CC_OUT(z) (success),				\
  		       "+A" (__old),					\
@@ -407,7 +406,6 @@ do {									\
  	if (unlikely(!success))						\
  		*_old = __old;						\
  	likely(success);					})
-#endif // CONFIG_X86_32
  #else  // !CONFIG_CC_HAS_ASM_GOTO_TIED_OUTPUT
  #define __try_cmpxchg_user_asm(itype, ltype, _ptr, _pold, _new, label)	({ \
  	int __err = 0;							\
@@ -433,7 +431,6 @@ do {									\
  		*_old = __old;						\
  	likely(success);					})
  
-#ifdef CONFIG_X86_32
  /*
   * 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
@@ -441,13 +438,13 @@ do {									\
   * both ESI and EDI for the memory operand, compilation will fail if the error
   * is an input+output as there will be no register available for input.
   */
-#define __try_cmpxchg64_user_asm(_ptr, _pold, _new, label)	({	\
+#define __try_cmpxchg8b_16b_user_asm(_b, _ptr, _pold, _new, label)	({	\
  	int __result;							\
  	__typeof__(_ptr) _old = (__typeof__(_ptr))(_pold);		\
  	__typeof__(*(_ptr)) __old = *_old;				\
  	__typeof__(*(_ptr)) __new = (_new);				\
  	asm volatile("\n"						\
-		     "1: " LOCK_PREFIX "cmpxchg8b %[ptr]\n"		\
+		     "1: " LOCK_PREFIX "cmpxchg" #_b "b %[ptr]\n"	\
  		     "mov $0, %[result]\n\t"				\
  		     "setz %b[result]\n"				\
  		     "2:\n"						\
@@ -464,7 +461,6 @@ do {									\
  	if (unlikely(!__result))					\
  		*_old = __old;						\
  	likely(__result);					})
-#endif // CONFIG_X86_32
  #endif // CONFIG_CC_HAS_ASM_GOTO_TIED_OUTPUT
  
  /* FIXME: this hack is definitely wrong -AK */
@@ -552,9 +549,15 @@ do {										\
  
  extern void __try_cmpxchg_user_wrong_size(void);
  
-#ifndef CONFIG_X86_32
+#ifdef CONFIG_X86_32
+#define __try_cmpxchg64_user_asm(_ptr, _pold, _new, label)		\
+	__try_cmpxchg8b_16b_user_asm(8, _ptr, _pold, _new, label)
+#else
  #define __try_cmpxchg64_user_asm(_ptr, _oldp, _nval, _label)		\
  	__try_cmpxchg_user_asm("q", "r", (_ptr), (_oldp), (_nval), _label)
+
+#define __try_cmpxchg128_user_asm(_ptr, _pold, _new, label)		\
+	__try_cmpxchg8b_16b_user_asm(16, _ptr, _pold, _new, label)
  #endif
  
  /*
@@ -581,6 +584,9 @@ extern void __try_cmpxchg_user_wrong_size(void);
  	case 8:	__ret = __try_cmpxchg64_user_asm((__force u64 *)(_ptr), (_oldp),\
  						 (_nval), _label);		\
  		break;								\
+	case 16:__ret = __try_cmpxchg128_user_asm((__force u128 *)(_ptr), (_oldp),\
+						 (_nval), _label);		\
+		break;								\
  	default: __try_cmpxchg_user_wrong_size();				\
  	}									\
  	__ret;						})
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f2db04aa6a17f..3e1bd9cdd75e6 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7973,7 +7973,7 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
  	int r;
  
  	/* guests cmpxchg8b have to be emulated atomically */
-	if (bytes > 8 || (bytes & (bytes - 1)))
+	if (bytes > 2 * sizeof(unsigned long) || (bytes & (bytes - 1)))
  		goto emul_write;
  
  	gpa = kvm_mmu_gva_to_gpa_write(vcpu, addr, NULL);
@@ -8013,6 +8013,11 @@ static int emulator_cmpxchg_emulated(struct x86_emulate_ctxt *ctxt,
  	case 8:
  		r = emulator_try_cmpxchg_user(u64, hva, old, new);
  		break;
+#ifdef CONFIG_X86_64
+	case 16:
+		r = emulator_try_cmpxchg_user(u128, hva, old, new);
+		break;
+#endif
  	default:
  		BUG();
  	}


(with the uaccess.h part to be split in a separate patch, of course).

Paolo