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

[Paolo Bonzini]

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