* [PATCH v3 0/2] Add support for cmpxchg16b emulation @ 2026-08-26 7:00 Sairaj Kodilkar 2026-08-26 7:00 ` [PATCH v3 1/2] x86/uaccess: Extend CMPXCHG user helpers to 128-bit operands Sairaj Kodilkar 2026-08-26 7:00 ` [PATCH v3 2/2] KVM: x86: Add support for cmpxchg16b emulation Sairaj Kodilkar 0 siblings, 2 replies; 12+ messages in thread From: Sairaj Kodilkar @ 2026-08-26 7:00 UTC (permalink / raw) To: H. Peter Anvin, Peter Zijlstra (Intel), Borislav Petkov, Dave Hansen, Ingo Molnar, Mathieu Desnoyers, Paolo Bonzini, Sairaj Kodilkar, Sean Christopherson, Thomas Gleixner, Uros Bizjak, kvm, linux-kernel, x86 Cc: vasant.hegde, suravee.suthikulpanit This series adds 128-bit compare-exchange support needed for KVM to emulate guest cmpxchg16b instructions. The AMD IOMMU driver updates 256-bit device table entries with two 128-bit cmpxchg operations. For hardware-accelerated vIOMMU, QEMU traps those DTE accesses to install nested page tables [1]. KVM must emulate guest cmpxchg16b on the trapped MMIO regions; without that, DTE access emulation fails. Patch 1: extends the x86 user CMPXCHG helpers with __try_cmpxchg128_user_asm() (cmpxchg16b on x86-64), wired into unsafe_try_cmpxchg_user(). Patch 2: extends KVM CMPXCHG8B emulation to 16-byte operands (REX.W=1) and uses the new helper for atomic 16-byte compare-exchange on guest RAM via emulator_cmpxchg_emulated(). You can find the KUT to test this series at [2]. [1] https://github.com/AMDESE/qemu-iommu/blob/wip/for_iommufd_hw_queue-v8_amd_viommu_20260106/hw/i386/amd_viommu.c#L517 [2] https://lore.kernel.org/kvm/20260706062153.346-1-sarunkod@amd.com/T/#u Changes since v2: https://lore.kernel.org/all/20260706063035.1139-1-sarunkod@amd.com/ Patch 1: Trigger build failure if __try_cmpxchg128_user_asm is called on 32 bit machines [Sean] Sairaj Kodilkar (2): x86/uaccess: Extend CMPXCHG user helpers to 128-bit operands KVM: x86: Add support for cmpxchg16b emulation arch/x86/include/asm/uaccess.h | 56 +++++++++++++++++++++++++++++++++- arch/x86/kvm/emulate.c | 50 ++++++++++++++++++++---------- arch/x86/kvm/kvm_emulate.h | 6 ++++ arch/x86/kvm/x86.c | 7 ++++- 4 files changed, 101 insertions(+), 18 deletions(-) base-commit: 45c13f3f9e3bb15fd89ff2864c6f627a3b4b4229 -- 2.34.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 1/2] x86/uaccess: Extend CMPXCHG user helpers to 128-bit operands 2026-08-26 7:00 [PATCH v3 0/2] Add support for cmpxchg16b emulation Sairaj Kodilkar @ 2026-08-26 7:00 ` Sairaj Kodilkar 2026-08-26 7:23 ` sashiko-bot 2026-08-26 12:30 ` Dave Hansen 2026-08-26 7:00 ` [PATCH v3 2/2] KVM: x86: Add support for cmpxchg16b emulation Sairaj Kodilkar 1 sibling, 2 replies; 12+ messages in thread From: Sairaj Kodilkar @ 2026-08-26 7:00 UTC (permalink / raw) To: H. Peter Anvin, Peter Zijlstra (Intel), Borislav Petkov, Dave Hansen, Ingo Molnar, Mathieu Desnoyers, Paolo Bonzini, Sairaj Kodilkar, Sean Christopherson, Thomas Gleixner, Uros Bizjak, kvm, linux-kernel, x86 Cc: vasant.hegde, suravee.suthikulpanit Extend the existing user CMPXCHG helpers to support 16-byte operands on x86-64, using LOCK_PREFIX "cmpxchg16b". This mirrors the existing __try_cmpxchg64_user_asm() / cmpxchg8b path provided for 32-bit kernels, where KVM needs an atomic compare-exchange wider than the generic cmpxchg helper can provide. On 32-bit kernels, stub the helper to always return failure because cmpxchg16b requires 64-bit GPRs and is not available. KVM uses this to atomically emulate guest cmpxchg16b on guest RAM mapped via userspace addresses. Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com> --- arch/x86/include/asm/uaccess.h | 56 +++++++++++++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h index 3a0dd3c2b233..39bcf327664e 100644 --- a/arch/x86/include/asm/uaccess.h +++ b/arch/x86/include/asm/uaccess.h @@ -407,6 +407,25 @@ do { \ if (unlikely(!success)) \ *_old = __old; \ likely(success); }) +#else // !CONFIG_X86_32 +#define __try_cmpxchg128_user_asm(_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 "cmpxchg16b %[ptr]\n" \ + _ASM_EXTABLE_UA(1b, %l[label]) \ + : "=@ccz" (success), \ + "+A" (__old), \ + [ptr] "+m" (*_ptr) \ + : "b" ((u64)__new), \ + "c" ((u64)((u128)__new >> 64)) \ + : "memory" \ + : label); \ + 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) ({ \ @@ -463,6 +482,30 @@ do { \ if (unlikely(!__result)) \ *_old = __old; \ likely(__result); }) +#else //!CONFIG_X86_32 +#define __try_cmpxchg128_user_asm(_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 "cmpxchg16b %[ptr]\n" \ + "mov $0, %[result]\n\t" \ + "setz %b[result]\n" \ + "2:\n" \ + _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG, \ + %[result]) \ + : [result] "=q" (__result), \ + "+A" (__old), \ + [ptr] "+m" (*_ptr) \ + : "b" ((u64)__new), \ + "c" ((u64)((u128)__new >> 64)) \ + : "memory", "cc"); \ + if (unlikely(__result < 0)) \ + goto label; \ + if (unlikely(!__result)) \ + *_old = __old; \ + likely(__result); }) #endif // CONFIG_X86_32 #endif // CONFIG_CC_HAS_ASM_GOTO_TIED_OUTPUT @@ -551,11 +594,18 @@ do { \ extern void __try_cmpxchg_user_wrong_size(void); -#ifndef CONFIG_X86_32 +#ifdef CONFIG_X86_32 +/* Always fail on 32 bit arch as it do not support 128 cmpxchg (i.e. cmpxchg16b + * instruction). + */ +#define __try_cmpxchg128_user_asm(_ptr, _pold, _new, label) ({ BUILD_BUG_ON(1); 0; }) +#else #define __try_cmpxchg64_user_asm(_ptr, _oldp, _nval, _label) \ __try_cmpxchg_user_asm("q", "r", (_ptr), (_oldp), (_nval), _label) + #endif + /* * Force the pointer to u<size> to match the size expected by the asm helper. * clang/LLVM compiles all cases and only discards the unused paths after @@ -580,6 +630,10 @@ 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; }) -- 2.34.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] x86/uaccess: Extend CMPXCHG user helpers to 128-bit operands 2026-08-26 7:00 ` [PATCH v3 1/2] x86/uaccess: Extend CMPXCHG user helpers to 128-bit operands Sairaj Kodilkar @ 2026-08-26 7:23 ` sashiko-bot 2026-08-26 8:54 ` Sairaj Kodilkar 2026-08-26 12:30 ` Dave Hansen 1 sibling, 1 reply; 12+ messages in thread From: sashiko-bot @ 2026-08-26 7:23 UTC (permalink / raw) To: Sairaj Kodilkar; +Cc: kvm Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] The inline assembly for 128-bit cmpxchg uses the unreliable `+A` constraint on x86-64, risking silent memory corruption or build failures. - [Medium] The 32-bit stub for `__try_cmpxchg128_user_asm` relies on `BUILD_BUG_ON(1)`, which will break the 32-bit build for all callers of `unsafe_try_cmpxchg_user`. -- commit 778924f40bf568ddfaeffc09239d3c0e2a09bba6 Author: Sairaj Kodilkar <sarunkod@amd.com> x86/uaccess: Extend CMPXCHG user helpers to 128-bit operands This commit extends the existing user CMPXCHG helpers to support 16-byte operands on x86-64 using the "cmpxchg16b" instruction. On 32-bit kernels, it stubs the helper to fail, as cmpxchg16b requires 64-bit registers. KVM uses this to atomically emulate guest cmpxchg16b on guest RAM mapped via userspace addresses. > diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h > index 3a0dd3c2b233e..39bcf327664ea 100644 > --- a/arch/x86/include/asm/uaccess.h > +++ b/arch/x86/include/asm/uaccess.h > @@ -407,6 +407,25 @@ do { \ [ ... ] > +#else // !CONFIG_X86_32 > +#define __try_cmpxchg128_user_asm(_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 "cmpxchg16b %[ptr]\n" \ > + _ASM_EXTABLE_UA(1b, %l[label]) \ > + : "=@ccz" (success), \ > + "+A" (__old), \ [Severity: High] Does using the "+A" constraint for a 128-bit value provide the strict rdx:rax register ordering required by the cmpxchg16b instruction? The kernel documents in arch/x86/include/asm/asm.h that the "A" constraint on x86-64 means rax or rdx, rather than the rdx:rax pair. If the compiler does not guarantee the rdx:rax ordering for __int128/u128 with "+A", could this lead to silent memory corruption with swapped operands, or outright build failures on compilers like Clang? The standard pattern used elsewhere (like __arch_cmpxchg128 in arch/x86/include/asm/cmpxchg_64.h) splits the 128-bit value into a union of two 64-bit halves and explicitly binds them to "+a" (rax) and "+d" (rdx). Would it be safer to use that pattern here? > + [ptr] "+m" (*_ptr) \ > + : "b" ((u64)__new), \ > + "c" ((u64)((u128)__new >> 64)) \ > + : "memory" \ > + : label); \ > + if (unlikely(!success)) \ > + *_old = __old; \ > + likely(success); }) > #endif // CONFIG_X86_32 > #else // !CONFIG_CC_HAS_ASM_GOTO_TIED_OUTPUT [ ... ] > +#else //!CONFIG_X86_32 > +#define __try_cmpxchg128_user_asm(_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 "cmpxchg16b %[ptr]\n" \ > + "mov $0, %[result]\n\t" \ > + "setz %b[result]\n" \ > + "2:\n" \ > + _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG, \ > + %[result]) \ > + : [result] "=q" (__result), \ > + "+A" (__old), \ [Severity: High] Similar to the asm_goto_output block above, this fallback path also binds the 128-bit __old variable to the "+A" constraint. > + [ptr] "+m" (*_ptr) \ > + : "b" ((u64)__new), \ > + "c" ((u64)((u128)__new >> 64)) \ > + : "memory", "cc"); \ > + if (unlikely(__result < 0)) \ > + goto label; \ > + if (unlikely(!__result)) \ > + *_old = __old; \ > + likely(__result); }) > #endif // CONFIG_X86_32 > #endif // CONFIG_CC_HAS_ASM_GOTO_TIED_OUTPUT [ ... ] > @@ -551,11 +594,18 @@ do { \ > > extern void __try_cmpxchg_user_wrong_size(void); > > -#ifndef CONFIG_X86_32 > +#ifdef CONFIG_X86_32 > +/* Always fail on 32 bit arch as it do not support 128 cmpxchg (i.e. cmpxchg16b > + * instruction). > + */ > +#define __try_cmpxchg128_user_asm(_ptr, _pold, _new, label) ({ BUILD_BUG_ON(1); 0; }) [Severity: Medium] Will this BUILD_BUG_ON(1) break 32-bit x86 builds that call the unsafe_try_cmpxchg_user() macro for perfectly valid 32-bit or 64-bit sizes? Because unsafe_try_cmpxchg_user() dispatches via a switch(sizeof(*(_ptr))) statement, compilers like Clang/LLVM eagerly evaluate all branches during semantic analysis, including the 16-byte branch. If evaluated on 32-bit builds, BUILD_BUG_ON(1) will trigger a compile-time error regardless of whether the 16-byte path is actually reachable. Since the commit message says "stub the helper to always return failure", would it be better to return a runtime error code here instead of using a build bug? > +#else > #define __try_cmpxchg64_user_asm(_ptr, _oldp, _nval, _label) \ > __try_cmpxchg_user_asm("q", "r", (_ptr), (_oldp), (_nval), _label) -- Sashiko AI review · https://sashiko.dev/#/patchset/20260826070004.8100-1-sarunkod@amd.com?part=1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] x86/uaccess: Extend CMPXCHG user helpers to 128-bit operands 2026-08-26 7:23 ` sashiko-bot @ 2026-08-26 8:54 ` Sairaj Kodilkar 0 siblings, 0 replies; 12+ messages in thread From: Sairaj Kodilkar @ 2026-08-26 8:54 UTC (permalink / raw) To: sashiko-reviews; +Cc: sarunkod, kvm On 8/26/2026 12:53 PM, sashiko-bot@kernel.org wrote: > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: > - [High] The inline assembly for 128-bit cmpxchg uses the unreliable `+A` constraint on x86-64, risking silent memory corruption or build failures. > - [Medium] The 32-bit stub for `__try_cmpxchg128_user_asm` relies on `BUILD_BUG_ON(1)`, which will break the 32-bit build for all callers of `unsafe_try_cmpxchg_user`. > -- > > commit 778924f40bf568ddfaeffc09239d3c0e2a09bba6 > Author: Sairaj Kodilkar <sarunkod@amd.com> > > x86/uaccess: Extend CMPXCHG user helpers to 128-bit operands > > This commit extends the existing user CMPXCHG helpers to support 16-byte > operands on x86-64 using the "cmpxchg16b" instruction. On 32-bit kernels, > it stubs the helper to fail, as cmpxchg16b requires 64-bit registers. KVM > uses this to atomically emulate guest cmpxchg16b on guest RAM mapped via > userspace addresses. > >> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h >> index 3a0dd3c2b233e..39bcf327664ea 100644 >> --- a/arch/x86/include/asm/uaccess.h >> +++ b/arch/x86/include/asm/uaccess.h >> @@ -407,6 +407,25 @@ do { \ > [ ... ] >> +#else // !CONFIG_X86_32 >> +#define __try_cmpxchg128_user_asm(_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 "cmpxchg16b %[ptr]\n" \ >> + _ASM_EXTABLE_UA(1b, %l[label]) \ >> + : "=@ccz" (success), \ >> + "+A" (__old), \ > > [Severity: High] > Does using the "+A" constraint for a 128-bit value provide the strict > rdx:rax register ordering required by the cmpxchg16b instruction? > > The kernel documents in arch/x86/include/asm/asm.h that the "A" constraint > on x86-64 means rax or rdx, rather than the rdx:rax pair. If the compiler > does not guarantee the rdx:rax ordering for __int128/u128 with "+A", could > this lead to silent memory corruption with swapped operands, or outright > build failures on compilers like Clang? > > The standard pattern used elsewhere (like __arch_cmpxchg128 in > arch/x86/include/asm/cmpxchg_64.h) splits the 128-bit value into a union > of two 64-bit halves and explicitly binds them to "+a" (rax) and "+d" (rdx). > Would it be safer to use that pattern here? It is easy to misread/misunderstand the official gcc documentations [1]. It specifically mentions that "Single word values will be allocated either in ax or dx". Here on x86_64 arch, the __old is 128 bit (i.e. it cannot fit in single register) so the compiler will always split it in rdx:rax pair. >> + [ptr] "+m" (*_ptr) \ >> + : "b" ((u64)__new), \ >> + "c" ((u64)((u128)__new >> 64)) \ >> + : "memory" \ >> + : label); \ >> + if (unlikely(!success)) \ >> + *_old = __old; \ >> + likely(success); }) >> #endif // CONFIG_X86_32 >> #else // !CONFIG_CC_HAS_ASM_GOTO_TIED_OUTPUT > [ ... ] >> +#else //!CONFIG_X86_32 >> +#define __try_cmpxchg128_user_asm(_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 "cmpxchg16b %[ptr]\n" \ >> + "mov $0, %[result]\n\t" \ >> + "setz %b[result]\n" \ >> + "2:\n" \ >> + _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG, \ >> + %[result]) \ >> + : [result] "=q" (__result), \ >> + "+A" (__old), \ > > [Severity: High] > Similar to the asm_goto_output block above, this fallback path also binds > the 128-bit __old variable to the "+A" constraint. > >> + [ptr] "+m" (*_ptr) \ >> + : "b" ((u64)__new), \ >> + "c" ((u64)((u128)__new >> 64)) \ >> + : "memory", "cc"); \ >> + if (unlikely(__result < 0)) \ >> + goto label; \ >> + if (unlikely(!__result)) \ >> + *_old = __old; \ >> + likely(__result); }) >> #endif // CONFIG_X86_32 >> #endif // CONFIG_CC_HAS_ASM_GOTO_TIED_OUTPUT > [ ... ] >> @@ -551,11 +594,18 @@ do { \ >> >> extern void __try_cmpxchg_user_wrong_size(void); >> >> -#ifndef CONFIG_X86_32 >> +#ifdef CONFIG_X86_32 >> +/* Always fail on 32 bit arch as it do not support 128 cmpxchg (i.e. cmpxchg16b >> + * instruction). >> + */ >> +#define __try_cmpxchg128_user_asm(_ptr, _pold, _new, label) ({ BUILD_BUG_ON(1); 0; }) > > [Severity: Medium] > Will this BUILD_BUG_ON(1) break 32-bit x86 builds that call the > unsafe_try_cmpxchg_user() macro for perfectly valid 32-bit or 64-bit sizes? > > Because unsafe_try_cmpxchg_user() dispatches via a switch(sizeof(*(_ptr))) > statement, compilers like Clang/LLVM eagerly evaluate all branches during > semantic analysis, including the 16-byte branch. If evaluated on 32-bit > builds, BUILD_BUG_ON(1) will trigger a compile-time error regardless of > whether the 16-byte path is actually reachable. > > Since the commit message says "stub the helper to always return failure", > would it be better to return a runtime error code here instead of using > a build bug? > The BUILD_BUG_ON(1) will only cause compile failure if there the code is getting used. As sean mentioned in the previous version, compiler elimiates this code if it is not used. Thanks Sairaj >> +#else >> #define __try_cmpxchg64_user_asm(_ptr, _oldp, _nval, _label) \ >> __try_cmpxchg_user_asm("q", "r", (_ptr), (_oldp), (_nval), _label) > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] x86/uaccess: Extend CMPXCHG user helpers to 128-bit operands 2026-08-26 7:00 ` [PATCH v3 1/2] x86/uaccess: Extend CMPXCHG user helpers to 128-bit operands Sairaj Kodilkar 2026-08-26 7:23 ` sashiko-bot @ 2026-08-26 12:30 ` Dave Hansen 2026-08-26 13:19 ` Uros Bizjak 1 sibling, 1 reply; 12+ messages in thread From: Dave Hansen @ 2026-08-26 12:30 UTC (permalink / raw) To: Sairaj Kodilkar, H. Peter Anvin, Peter Zijlstra (Intel), Borislav Petkov, Dave Hansen, Ingo Molnar, Mathieu Desnoyers, Paolo Bonzini, Sean Christopherson, Thomas Gleixner, Uros Bizjak, kvm, linux-kernel, x86 Cc: vasant.hegde, suravee.suthikulpanit On 8/26/26 00:00, Sairaj Kodilkar wrote: > Extend the existing user CMPXCHG helpers to support 16-byte operands on > x86-64, using LOCK_PREFIX "cmpxchg16b". This mirrors the existing > __try_cmpxchg64_user_asm() / cmpxchg8b path provided for 32-bit kernels, > where KVM needs an atomic compare-exchange wider than the generic > cmpxchg helper can provide. Please take a good look at the Sashiko review: https://sashiko.dev/#/patchset/20260826070004.8100-2-sarunkod%40amd.com It looks like the "A" constraint isn't one that you can cleanly mirror from cmpxchg8b => cmpxchg16b. Uros, any chance you can give these a good once-over? This seems to be just the kind of thing you've been fixing up lately. It would be nice to get them right the first time. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] x86/uaccess: Extend CMPXCHG user helpers to 128-bit operands 2026-08-26 12:30 ` Dave Hansen @ 2026-08-26 13:19 ` Uros Bizjak 2026-08-26 13:28 ` Sairaj Kodilkar 2026-08-27 9:01 ` David Laight 0 siblings, 2 replies; 12+ messages in thread From: Uros Bizjak @ 2026-08-26 13:19 UTC (permalink / raw) To: Dave Hansen Cc: Sairaj Kodilkar, H. Peter Anvin, Peter Zijlstra (Intel), Borislav Petkov, Dave Hansen, Ingo Molnar, Mathieu Desnoyers, Paolo Bonzini, Sean Christopherson, Thomas Gleixner, kvm, linux-kernel, x86, vasant.hegde, suravee.suthikulpanit On Wed, Aug 26, 2026 at 2:30 PM Dave Hansen <dave.hansen@intel.com> wrote: > > On 8/26/26 00:00, Sairaj Kodilkar wrote: > > Extend the existing user CMPXCHG helpers to support 16-byte operands on > > x86-64, using LOCK_PREFIX "cmpxchg16b". This mirrors the existing > > __try_cmpxchg64_user_asm() / cmpxchg8b path provided for 32-bit kernels, > > where KVM needs an atomic compare-exchange wider than the generic > > cmpxchg helper can provide. > > Please take a good look at the Sashiko review: > > https://sashiko.dev/#/patchset/20260826070004.8100-2-sarunkod%40amd.com > > It looks like the "A" constraint isn't one that you can cleanly mirror > from cmpxchg8b => cmpxchg16b. Actually, "+A" will work for 64bit targets, as long as the variable is 128-bit. The comment in asm.h applies to 64-bit values, where on 32-bit targets they fit in eax *and* edx, while on 64-bit targets, the 64-bit values fit into rax *or* rdx. This is documented in GCC documentation: ‘A’ The ‘a’ and ‘d’ registers. This class is used for instructions that return double word results in the ‘ax:dx’ register pair. Single word values will be allocated either in ‘ax’ or ‘dx’. And can be confirmed with e.g.: __int128 val; void foo (void) { __int128 _v = val; asm volatile ("" : "+A" (_v)); val = _v; } which will fail compilation with -ffixed-rax or -ffixed-rdx. That said, the approach with union of two 64-bit halves can lead to slightly better code, because the compiler splits the value earlier in the compilation pipeline. > Uros, any chance you can give these a good once-over? This seems to be > just the kind of thing you've been fixing up lately. It would be nice to > get them right the first time. Based on the above explanation, these *can* be copied from 32-bit asm patterns. Even "q" constraint will include all integer registers on 64-bit targets. Uros. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] x86/uaccess: Extend CMPXCHG user helpers to 128-bit operands 2026-08-26 13:19 ` Uros Bizjak @ 2026-08-26 13:28 ` Sairaj Kodilkar 2026-08-27 9:01 ` David Laight 1 sibling, 0 replies; 12+ messages in thread From: Sairaj Kodilkar @ 2026-08-26 13:28 UTC (permalink / raw) To: Uros Bizjak, Dave Hansen Cc: sarunkod, H. Peter Anvin, Peter Zijlstra (Intel), Borislav Petkov, Dave Hansen, Ingo Molnar, Mathieu Desnoyers, Paolo Bonzini, Sean Christopherson, Thomas Gleixner, kvm, linux-kernel, x86, vasant.hegde, suravee.suthikulpanit On 8/26/2026 6:49 PM, Uros Bizjak wrote: > [You don't often get email from ubizjak@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > On Wed, Aug 26, 2026 at 2:30 PM Dave Hansen <dave.hansen@intel.com> wrote: >> >> On 8/26/26 00:00, Sairaj Kodilkar wrote: >>> Extend the existing user CMPXCHG helpers to support 16-byte operands on >>> x86-64, using LOCK_PREFIX "cmpxchg16b". This mirrors the existing >>> __try_cmpxchg64_user_asm() / cmpxchg8b path provided for 32-bit kernels, >>> where KVM needs an atomic compare-exchange wider than the generic >>> cmpxchg helper can provide. >> >> Please take a good look at the Sashiko review: >> >> https://sashiko.dev/#/patchset/20260826070004.8100-2-sarunkod%40amd.com >> >> It looks like the "A" constraint isn't one that you can cleanly mirror >> from cmpxchg8b => cmpxchg16b. > > Actually, "+A" will work for 64bit targets, as long as the variable is > 128-bit. The comment in asm.h applies to 64-bit values, where on > 32-bit targets they fit in eax *and* edx, while on 64-bit targets, the > 64-bit values fit into rax *or* rdx. > > This is documented in GCC documentation: > > ‘A’ > The ‘a’ and ‘d’ registers. This class is used for > instructions that return double word results in the ‘ax:dx’ > register pair. Single word values will be allocated either in > ‘ax’ or ‘dx’. > > And can be confirmed with e.g.: > > __int128 val; > > void foo (void) > { > __int128 _v = val; > asm volatile ("" : "+A" (_v)); > val = _v; > } > > which will fail compilation with -ffixed-rax or -ffixed-rdx. > > That said, the approach with union of two 64-bit halves can lead to > slightly better code, because the compiler splits the value earlier in > the compilation pipeline. > >> Uros, any chance you can give these a good once-over? This seems to be >> just the kind of thing you've been fixing up lately. It would be nice to >> get them right the first time. > > Based on the above explanation, these *can* be copied from 32-bit asm > patterns. Even "q" constraint will include all integer registers on > 64-bit targets. Yep, this is what I was trying to say in my reply to sashiko. Basically +A can be safely used for 64 bit values on 32 bit machines and 128 bit values on 64 bit machines. Also if you prefer split one, I can do that as well. Sairaj > > Uros. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/2] x86/uaccess: Extend CMPXCHG user helpers to 128-bit operands 2026-08-26 13:19 ` Uros Bizjak 2026-08-26 13:28 ` Sairaj Kodilkar @ 2026-08-27 9:01 ` David Laight 1 sibling, 0 replies; 12+ messages in thread From: David Laight @ 2026-08-27 9:01 UTC (permalink / raw) To: Uros Bizjak Cc: Dave Hansen, Sairaj Kodilkar, H. Peter Anvin, Peter Zijlstra (Intel), Borislav Petkov, Dave Hansen, Ingo Molnar, Mathieu Desnoyers, Paolo Bonzini, Sean Christopherson, Thomas Gleixner, kvm, linux-kernel, x86, vasant.hegde, suravee.suthikulpanit On Wed, 26 Aug 2026 15:19:57 +0200 Uros Bizjak <ubizjak@gmail.com> wrote: > On Wed, Aug 26, 2026 at 2:30 PM Dave Hansen <dave.hansen@intel.com> wrote: > > > > On 8/26/26 00:00, Sairaj Kodilkar wrote: > > > Extend the existing user CMPXCHG helpers to support 16-byte operands on > > > x86-64, using LOCK_PREFIX "cmpxchg16b". This mirrors the existing > > > __try_cmpxchg64_user_asm() / cmpxchg8b path provided for 32-bit kernels, > > > where KVM needs an atomic compare-exchange wider than the generic > > > cmpxchg helper can provide. > > > > Please take a good look at the Sashiko review: > > > > https://sashiko.dev/#/patchset/20260826070004.8100-2-sarunkod%40amd.com > > > > It looks like the "A" constraint isn't one that you can cleanly mirror > > from cmpxchg8b => cmpxchg16b. > > Actually, "+A" will work for 64bit targets, as long as the variable is > 128-bit. The comment in asm.h applies to 64-bit values, where on > 32-bit targets they fit in eax *and* edx, while on 64-bit targets, the > 64-bit values fit into rax *or* rdx. > ... > > That said, the approach with union of two 64-bit halves can lead to > slightly better code, because the compiler splits the value earlier in > the compilation pipeline. I think I agree... From experiments I did with 64bit values on 32bit it is more the case that the value never gets assigned to a 64bit (on 32bit) 'virtual' register. If that ever happens all the operations are initially done with the 'wide' register and then later split (rather than being generated as a pair of 32bit ops). This causes excessive register pressure and even spilling of constant zero values to stack. You do seem to 'get away' with returning hi << 32 | lo. I'd guess the same happens for 128bit values on 64bit. (This is gcc, clang does a lot better.) David > > > Uros, any chance you can give these a good once-over? This seems to be > > just the kind of thing you've been fixing up lately. It would be nice to > > get them right the first time. > > Based on the above explanation, these *can* be copied from 32-bit asm > patterns. Even "q" constraint will include all integer registers on > 64-bit targets. > > Uros. > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 2/2] KVM: x86: Add support for cmpxchg16b emulation 2026-08-26 7:00 [PATCH v3 0/2] Add support for cmpxchg16b emulation Sairaj Kodilkar 2026-08-26 7:00 ` [PATCH v3 1/2] x86/uaccess: Extend CMPXCHG user helpers to 128-bit operands Sairaj Kodilkar @ 2026-08-26 7:00 ` Sairaj Kodilkar 2026-08-26 7:31 ` sashiko-bot 2026-08-26 12:50 ` Mathieu Desnoyers 1 sibling, 2 replies; 12+ messages in thread From: Sairaj Kodilkar @ 2026-08-26 7:00 UTC (permalink / raw) To: H. Peter Anvin, Peter Zijlstra (Intel), Borislav Petkov, Dave Hansen, Ingo Molnar, Mathieu Desnoyers, Paolo Bonzini, Sairaj Kodilkar, Sean Christopherson, Thomas Gleixner, Uros Bizjak, kvm, linux-kernel, x86 Cc: vasant.hegde, suravee.suthikulpanit AMD and Intel both provides support for 128 bit cmpxchg operands using cmpxchg8b/cmpxchg16b instructions (opcode 0FC7). However, kvm does not support emulating cmpxchg16b (i.e when destination memory is 128 bit and REX.W = 1) which causes emulation failure when QEMU guest performs a cmpxchg16b on a memory region setup as a IO. This failure is seen on the AMD IOMMU driver which writes 256-bit device table entries with two 128-bit cmpxchg operations. For guests using hardware-accelerated vIOMMU, QEMU traps device table accesses to set up nested page tables (see [1]). Without 128-bit cmpxchg emulation, KVM cannot handle these traps and DTE access emulation fails. Hence extend cmpxchg8b to perform cmpxchg16b when the destination memory is 128 bit. [1] https://github.com/AMDESE/qemu-iommu/blob/wip/for_iommufd_hw_queue-v8_amd_viommu_20260106/hw/i386/amd_viommu.c#L517 Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com> --- arch/x86/kvm/emulate.c | 50 ++++++++++++++++++++++++++------------ arch/x86/kvm/kvm_emulate.h | 6 +++++ arch/x86/kvm/x86.c | 7 +++++- 3 files changed, 46 insertions(+), 17 deletions(-) diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c index c1b21282187f..f38c4f1a99c3 100644 --- a/arch/x86/kvm/emulate.c +++ b/arch/x86/kvm/emulate.c @@ -2184,24 +2184,36 @@ static int em_call_near_abs(struct x86_emulate_ctxt *ctxt) return rc; } +#define em_cmpxchg8b_16b(__c, rbits, mbits)\ +do { \ + u##mbits old = __c->dst.orig_val##mbits; \ + \ + BUILD_BUG_ON(rbits * 2 != mbits); \ + \ + if (((u##rbits) (old >> 0) != (u##rbits) reg_read(ctxt, VCPU_REGS_RAX)) || \ + ((u##rbits) (old >> rbits) != (u##rbits) reg_read(ctxt, VCPU_REGS_RDX))) { \ + *reg_write(ctxt, VCPU_REGS_RAX) = (u##rbits) (old >> 0); \ + *reg_write(ctxt, VCPU_REGS_RDX) = (u##rbits) (old >> rbits); \ + ctxt->eflags &= ~X86_EFLAGS_ZF; \ + } else { \ + ctxt->dst.val##mbits = ((u##mbits)reg_read(ctxt, VCPU_REGS_RCX) << rbits) | \ + (u##rbits) reg_read(ctxt, VCPU_REGS_RBX); \ + \ + ctxt->eflags |= X86_EFLAGS_ZF; \ + } \ +} while(0) + static int em_cmpxchg8b(struct x86_emulate_ctxt *ctxt) { - u64 old = ctxt->dst.orig_val64; - - if (ctxt->dst.bytes == 16) + if (WARN_ON_ONCE(8 + !!(ctxt->rex_bits & REX_W) * 8 != ctxt->dst.bytes)) return X86EMUL_UNHANDLEABLE; - if (((u32) (old >> 0) != (u32) reg_read(ctxt, VCPU_REGS_RAX)) || - ((u32) (old >> 32) != (u32) reg_read(ctxt, VCPU_REGS_RDX))) { - *reg_write(ctxt, VCPU_REGS_RAX) = (u32) (old >> 0); - *reg_write(ctxt, VCPU_REGS_RDX) = (u32) (old >> 32); - ctxt->eflags &= ~X86_EFLAGS_ZF; - } else { - ctxt->dst.val64 = ((u64)reg_read(ctxt, VCPU_REGS_RCX) << 32) | - (u32) reg_read(ctxt, VCPU_REGS_RBX); - - ctxt->eflags |= X86_EFLAGS_ZF; - } + if (!(ctxt->rex_bits & REX_W)) + em_cmpxchg8b_16b(ctxt, 32, 64); +#ifdef CONFIG_X86_64 + else + em_cmpxchg8b_16b(ctxt, 64, 128); +#endif return X86EMUL_CONTINUE; } @@ -5414,8 +5426,14 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt, bool check_intercepts) goto done; } } - /* Copy full 64-bit value for CMPXCHG8B. */ - ctxt->dst.orig_val64 = ctxt->dst.val64; + /* Copy full 64/128-bit value for CMPXCHG8B. */ + +#ifdef CONFIG_X86_64 + if (ctxt->dst.bytes == 16) + ctxt->dst.orig_val128 = ctxt->dst.val128; + else +#endif + ctxt->dst.orig_val64 = ctxt->dst.val64; special_insn: diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h index 3e375af15c03..89911845233d 100644 --- a/arch/x86/kvm/kvm_emulate.h +++ b/arch/x86/kvm/kvm_emulate.h @@ -263,6 +263,9 @@ struct operand { union { unsigned long orig_val; u64 orig_val64; +#ifdef CONFIG_X86_64 + u128 orig_val128; +#endif }; union { unsigned long *reg; @@ -276,6 +279,9 @@ struct operand { union { unsigned long val; u64 val64; +#ifdef CONFIG_X86_64 + u128 val128; +#endif char valptr[sizeof(avx256_t)]; sse128_t vec_val; avx256_t vec_val2; diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 79468ddfe473..60be16f05d79 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -5271,7 +5271,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); @@ -5311,6 +5311,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(); } -- 2.34.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/2] KVM: x86: Add support for cmpxchg16b emulation 2026-08-26 7:00 ` [PATCH v3 2/2] KVM: x86: Add support for cmpxchg16b emulation Sairaj Kodilkar @ 2026-08-26 7:31 ` sashiko-bot 2026-08-26 9:17 ` Sairaj Kodilkar 2026-08-26 12:50 ` Mathieu Desnoyers 1 sibling, 1 reply; 12+ messages in thread From: sashiko-bot @ 2026-08-26 7:31 UTC (permalink / raw) To: Sairaj Kodilkar; +Cc: kvm Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: New issues: - [High] Emulation of CMPXCHG16B lacks the mandatory 16-byte alignment check, leading to a guest-triggerable VM crash. - [Low] The macro `em_cmpxchg8b_16b` incorrectly captures the variable `ctxt` from the surrounding scope instead of solely using its argument `__c`. Pre-existing issues: - [Critical] TOCTOU vulnerability in atomic CMPXCHG emulation leads to silent data corruption in the guest when a native compare fails. -- commit f6a8a67f38b03c4522f119ac54d63d97691e1289 Author: Sairaj Kodilkar <sarunkod@amd.com> KVM: x86: Add support for cmpxchg16b emulation This patch adds support for emulating cmpxchg16b when the destination memory is 128-bit and REX.W = 1. This prevents QEMU guests from hitting emulation failures during 256-bit device table entries accesses on AMD vIOMMU. > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c > index c1b21282187f3..f38c4f1a99c31 100644 > --- a/arch/x86/kvm/emulate.c > +++ b/arch/x86/kvm/emulate.c [ ... ] > @@ -2184,24 +2184,36 @@ static int em_call_near_abs(struct x86_emulate_ctxt *ctxt) > return rc; > } > > +#define em_cmpxchg8b_16b(__c, rbits, mbits)\ > +do { \ > + u##mbits old = __c->dst.orig_val##mbits; \ > + \ > + BUILD_BUG_ON(rbits * 2 != mbits); \ > + \ > + if (((u##rbits) (old >> 0) != (u##rbits) reg_read(ctxt, VCPU_REGS_RAX)) || \ [Severity: Low] Does this macro intentionally reference the unpassed variable ctxt instead of the provided argument __c? This uses ctxt to read registers, which makes the macro rely on the specific variable name ctxt in the caller's scope instead of its parameter. > + ((u##rbits) (old >> rbits) != (u##rbits) reg_read(ctxt, VCPU_REGS_RDX))) { \ > + *reg_write(ctxt, VCPU_REGS_RAX) = (u##rbits) (old >> 0); \ > + *reg_write(ctxt, VCPU_REGS_RDX) = (u##rbits) (old >> rbits); \ > + ctxt->eflags &= ~X86_EFLAGS_ZF; \ > + } else { \ > + ctxt->dst.val##mbits = ((u##mbits)reg_read(ctxt, VCPU_REGS_RCX) << rbits) | \ > + (u##rbits) reg_read(ctxt, VCPU_REGS_RBX); \ > + \ > + ctxt->eflags |= X86_EFLAGS_ZF; \ > + } \ > +} while(0) > + > static int em_cmpxchg8b(struct x86_emulate_ctxt *ctxt) > { > - u64 old = ctxt->dst.orig_val64; > - > - if (ctxt->dst.bytes == 16) > + if (WARN_ON_ONCE(8 + !!(ctxt->rex_bits & REX_W) * 8 != ctxt->dst.bytes)) > return X86EMUL_UNHANDLEABLE; > > - if (((u32) (old >> 0) != (u32) reg_read(ctxt, VCPU_REGS_RAX)) || > - ((u32) (old >> 32) != (u32) reg_read(ctxt, VCPU_REGS_RDX))) { > - *reg_write(ctxt, VCPU_REGS_RAX) = (u32) (old >> 0); > - *reg_write(ctxt, VCPU_REGS_RDX) = (u32) (old >> 32); > - ctxt->eflags &= ~X86_EFLAGS_ZF; > - } else { > - ctxt->dst.val64 = ((u64)reg_read(ctxt, VCPU_REGS_RCX) << 32) | > - (u32) reg_read(ctxt, VCPU_REGS_RBX); > - > - ctxt->eflags |= X86_EFLAGS_ZF; > - } > + if (!(ctxt->rex_bits & REX_W)) > + em_cmpxchg8b_16b(ctxt, 32, 64); > +#ifdef CONFIG_X86_64 > + else > + em_cmpxchg8b_16b(ctxt, 64, 128); > +#endif [Severity: High] Are we missing a 16-byte alignment check for the destination memory when executing CMPXCHG16B here? The x86 architecture strictly mandates that the memory operand for CMPXCHG16B must be 16-byte aligned. If it is not, a #GP(0) exception is generated unconditionally. Without this check, KVM will later attempt a native unaligned lock cmpxchg16b, which will raise a host #GP(0). The host exception table catches this and returns -EFAULT, translating to X86EMUL_UNHANDLEABLE, which immediately crashes the VM via KVM_EXIT_INTERNAL_ERROR. Can this regression be used by a guest to trigger a denial of service? > return X86EMUL_CONTINUE; > } [ ... ] > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 79468ddfe4736..60be16f05d794 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c [ ... ] > @@ -5311,6 +5311,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 [Severity: Critical] This is a pre-existing issue, but does the emulation of atomic CMPXCHG suffer from a TOCTOU race leading to silent data corruption in the guest? When emulator_try_cmpxchg_user() fails due to concurrent memory modification, emulator_cmpxchg_emulated() returns X86EMUL_CMPXCHG_FAILED. However, in x86_emulate_insn(), this failure appears to be silently swallowed: arch/x86/kvm/emulate.c:x86_emulate_insn() { ... if (rc == X86EMUL_CONTINUE) writeback_registers(ctxt); return (rc == X86EMUL_UNHANDLEABLE) ? EMULATION_FAILED : EMULATION_OK; } Because rc is not X86EMUL_CONTINUE, writeback_registers() is skipped and the guest registers are not updated. Because EMULATION_OK is returned, KVM retires the instruction as successful, advancing RIP and leaving the guest with a stale ZF=1 flag. Doesn't this cause the guest to mistakenly believe it acquired a lock? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260826070004.8100-1-sarunkod@amd.com?part=2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/2] KVM: x86: Add support for cmpxchg16b emulation 2026-08-26 7:31 ` sashiko-bot @ 2026-08-26 9:17 ` Sairaj Kodilkar 0 siblings, 0 replies; 12+ messages in thread From: Sairaj Kodilkar @ 2026-08-26 9:17 UTC (permalink / raw) To: sashiko-reviews; +Cc: sarunkod, kvm On 8/26/2026 1:01 PM, sashiko-bot@kernel.org wrote: > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: > > New issues: > - [High] Emulation of CMPXCHG16B lacks the mandatory 16-byte alignment check, leading to a guest-triggerable VM crash. > - [Low] The macro `em_cmpxchg8b_16b` incorrectly captures the variable `ctxt` from the surrounding scope instead of solely using its argument `__c`. > > Pre-existing issues: > - [Critical] TOCTOU vulnerability in atomic CMPXCHG emulation leads to silent data corruption in the guest when a native compare fails. > -- > > commit f6a8a67f38b03c4522f119ac54d63d97691e1289 > Author: Sairaj Kodilkar <sarunkod@amd.com> > > KVM: x86: Add support for cmpxchg16b emulation > > This patch adds support for emulating cmpxchg16b when the destination memory > is 128-bit and REX.W = 1. This prevents QEMU guests from hitting emulation > failures during 256-bit device table entries accesses on AMD vIOMMU. > >> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c >> index c1b21282187f3..f38c4f1a99c31 100644 >> --- a/arch/x86/kvm/emulate.c >> +++ b/arch/x86/kvm/emulate.c > > [ ... ] > >> @@ -2184,24 +2184,36 @@ static int em_call_near_abs(struct x86_emulate_ctxt *ctxt) >> return rc; >> } >> >> +#define em_cmpxchg8b_16b(__c, rbits, mbits)\ >> +do { \ >> + u##mbits old = __c->dst.orig_val##mbits; \ >> + \ >> + BUILD_BUG_ON(rbits * 2 != mbits); \ >> + \ >> + if (((u##rbits) (old >> 0) != (u##rbits) reg_read(ctxt, VCPU_REGS_RAX)) || \ > > [Severity: Low] > Does this macro intentionally reference the unpassed variable ctxt instead of > the provided argument __c? > > This uses ctxt to read registers, which makes the macro rely on the specific > variable name ctxt in the caller's scope instead of its parameter. > Its a bug, the macro should use __c instead of ctxt everywhere. >> + ((u##rbits) (old >> rbits) != (u##rbits) reg_read(ctxt, VCPU_REGS_RDX))) { \ >> + *reg_write(ctxt, VCPU_REGS_RAX) = (u##rbits) (old >> 0); \ >> + *reg_write(ctxt, VCPU_REGS_RDX) = (u##rbits) (old >> rbits); \ >> + ctxt->eflags &= ~X86_EFLAGS_ZF; \ >> + } else { \ >> + ctxt->dst.val##mbits = ((u##mbits)reg_read(ctxt, VCPU_REGS_RCX) << rbits) | \ >> + (u##rbits) reg_read(ctxt, VCPU_REGS_RBX); \ >> + \ >> + ctxt->eflags |= X86_EFLAGS_ZF; \ >> + } \ >> +} while(0) >> + >> static int em_cmpxchg8b(struct x86_emulate_ctxt *ctxt) >> { >> - u64 old = ctxt->dst.orig_val64; >> - >> - if (ctxt->dst.bytes == 16) >> + if (WARN_ON_ONCE(8 + !!(ctxt->rex_bits & REX_W) * 8 != ctxt->dst.bytes)) >> return X86EMUL_UNHANDLEABLE; >> >> - if (((u32) (old >> 0) != (u32) reg_read(ctxt, VCPU_REGS_RAX)) || >> - ((u32) (old >> 32) != (u32) reg_read(ctxt, VCPU_REGS_RDX))) { >> - *reg_write(ctxt, VCPU_REGS_RAX) = (u32) (old >> 0); >> - *reg_write(ctxt, VCPU_REGS_RDX) = (u32) (old >> 32); >> - ctxt->eflags &= ~X86_EFLAGS_ZF; >> - } else { >> - ctxt->dst.val64 = ((u64)reg_read(ctxt, VCPU_REGS_RCX) << 32) | >> - (u32) reg_read(ctxt, VCPU_REGS_RBX); >> - >> - ctxt->eflags |= X86_EFLAGS_ZF; >> - } >> + if (!(ctxt->rex_bits & REX_W)) >> + em_cmpxchg8b_16b(ctxt, 32, 64); >> +#ifdef CONFIG_X86_64 >> + else >> + em_cmpxchg8b_16b(ctxt, 64, 128); >> +#endif > > [Severity: High] > Are we missing a 16-byte alignment check for the destination memory when > executing CMPXCHG16B here? > > The x86 architecture strictly mandates that the memory operand for CMPXCHG16B > must be 16-byte aligned. If it is not, a #GP(0) exception is generated > unconditionally. > > Without this check, KVM will later attempt a native unaligned lock cmpxchg16b, > which will raise a host #GP(0). The host exception table catches this and > returns -EFAULT, translating to X86EMUL_UNHANDLEABLE, which immediately > crashes the VM via KVM_EXIT_INTERNAL_ERROR. Can this regression be used by a > guest to trigger a denial of service? insn_alignment checks for any alignment issues. Its comment explicitly mentions that "Also included is CMPXCHG16B which is not a vector instruction, yet it is subject to the same check. FXSAVE and FXRSTOR are checked here too as their 512 bytes of data must be aligned to a 16 byte boundary." Hence kvm will raise an #GP(0) to guest before it performs cmpxchg8b. Thanks Sairaj > >> return X86EMUL_CONTINUE; >> } > > [ ... ] > >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 79468ddfe4736..60be16f05d794 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c > > [ ... ] > >> @@ -5311,6 +5311,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 > > [Severity: Critical] > This is a pre-existing issue, but does the emulation of atomic CMPXCHG suffer > from a TOCTOU race leading to silent data corruption in the guest? > > When emulator_try_cmpxchg_user() fails due to concurrent memory modification, > emulator_cmpxchg_emulated() returns X86EMUL_CMPXCHG_FAILED. > > However, in x86_emulate_insn(), this failure appears to be silently swallowed: > > arch/x86/kvm/emulate.c:x86_emulate_insn() { > ... > if (rc == X86EMUL_CONTINUE) > writeback_registers(ctxt); > > return (rc == X86EMUL_UNHANDLEABLE) ? EMULATION_FAILED : EMULATION_OK; > } > > Because rc is not X86EMUL_CONTINUE, writeback_registers() is skipped and the > guest registers are not updated. Because EMULATION_OK is returned, KVM retires > the instruction as successful, advancing RIP and leaving the guest with a > stale ZF=1 flag. Doesn't this cause the guest to mistakenly believe it > acquired a lock? > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/2] KVM: x86: Add support for cmpxchg16b emulation 2026-08-26 7:00 ` [PATCH v3 2/2] KVM: x86: Add support for cmpxchg16b emulation Sairaj Kodilkar 2026-08-26 7:31 ` sashiko-bot @ 2026-08-26 12:50 ` Mathieu Desnoyers 1 sibling, 0 replies; 12+ messages in thread From: Mathieu Desnoyers @ 2026-08-26 12:50 UTC (permalink / raw) To: Sairaj Kodilkar, H. Peter Anvin, Peter Zijlstra (Intel), Borislav Petkov, Dave Hansen, Ingo Molnar, Paolo Bonzini, Sean Christopherson, Thomas Gleixner, Uros Bizjak, kvm, linux-kernel, x86 Cc: vasant.hegde, suravee.suthikulpanit On 2026-08-26 03:00, Sairaj Kodilkar wrote: > AMD and Intel both provides support for 128 bit cmpxchg operands using > cmpxchg8b/cmpxchg16b instructions (opcode 0FC7). However, kvm does not > support emulating cmpxchg16b (i.e when destination memory is 128 bit and > REX.W = 1) which causes emulation failure when QEMU guest performs a > cmpxchg16b on a memory region setup as a IO. > > This failure is seen on the AMD IOMMU driver which writes 256-bit device > table entries with two 128-bit cmpxchg operations. For guests using > hardware-accelerated vIOMMU, QEMU traps device table accesses to set up > nested page tables (see [1]). Without 128-bit cmpxchg emulation, KVM > cannot handle these traps and DTE access emulation fails. > > Hence extend cmpxchg8b to perform cmpxchg16b when the destination memory > is 128 bit. > > [1] https://github.com/AMDESE/qemu-iommu/blob/wip/for_iommufd_hw_queue-v8_amd_viommu_20260106/hw/i386/amd_viommu.c#L517 > You may want to test for the X86_FEATURE_CX16 cpu feature and only enable this on 64-bit x86 CPUs implementing this instruction. Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. https://www.efficios.com ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2026-08-27 9:01 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-26 7:00 [PATCH v3 0/2] Add support for cmpxchg16b emulation Sairaj Kodilkar 2026-08-26 7:00 ` [PATCH v3 1/2] x86/uaccess: Extend CMPXCHG user helpers to 128-bit operands Sairaj Kodilkar 2026-08-26 7:23 ` sashiko-bot 2026-08-26 8:54 ` Sairaj Kodilkar 2026-08-26 12:30 ` Dave Hansen 2026-08-26 13:19 ` Uros Bizjak 2026-08-26 13:28 ` Sairaj Kodilkar 2026-08-27 9:01 ` David Laight 2026-08-26 7:00 ` [PATCH v3 2/2] KVM: x86: Add support for cmpxchg16b emulation Sairaj Kodilkar 2026-08-26 7:31 ` sashiko-bot 2026-08-26 9:17 ` Sairaj Kodilkar 2026-08-26 12:50 ` Mathieu Desnoyers
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.