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

[Sairaj Kodilkar <sarunkod@amd.com>]

On 3/12/2026 5:16 PM, Paolo Bonzini wrote:
> 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"    \

Hi Paolo,

I think its better to keep two implementations separate because this
adds an additional complexity while passing the upper and lower half
of the "_new"
The cmpxchg8b expects ecx as "(u32)((u64)_new >> 32)" where as
cmpxchg16b expects ecx as "(u64)((u128)_new >> 64)"

Thanks
Sairaj