Re: [PATCH v3 12/13] KVM: selftests: Dedup assembly code for VMLAUNCH and VMRESUME
From: Yosry Ahmed
Date: Thu Aug 27 2026 - 13:22:08 EST
On Thu, Aug 27, 2026 at 10:17 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Thu, Aug 27, 2026, Yosry Ahmed wrote:
> > On Thu, Aug 27, 2026 at 9:41 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > > #define __BUILD_VMX_VM_ENTRY_HELPER(insn, prefix, vmwrite_insn, vmwrite_operand, \
> > > __host_rsp, __host_rip) \
> >
> > The only part I really hate is vmwrite_operand. Why do we need it? Seems
> > like all the current helpers use "r", I feel like I am missing something.
>
> ...
>
> > > static int __##prefix##_##insn(void) \
> > > { \
> > > int ret; \
> > > \
> > > __asm__ __volatile__("push $0;" \
> > > __stringify(vmwrite_insn) " %%rsp, %[host_rsp];" \
> > > "lea 1f(%%rip), %%rax;" \
> > > __stringify(vmwrite_insn) " %%rax, %[host_rip];" \
> > > VMX_SWITCH_GPRS_ASM \
> > > __stringify(insn)";" \
> > > "incq (%%rsp);" \
> > > "1: ;" \
> > > VMX_SWITCH_GPRS_ASM \
> > > "pop %%rax;" \
> > > : [ret]"=&a"(ret) \
> > > : [host_rsp]__stringify(vmwrite_operand)(__host_rsp), \
> > > [host_rip]__stringify(vmwrite_operand)(__host_rip), \
> > > GUEST_REGS_OFFSETS \
> > > : "memory", "cc"); \
> > > return ret; \
> > > }
> > >
> > > #define BUILD_VMX_VM_ENTRY_HELPER(insn) \
> > > __BUILD_VMX_VM_ENTRY_HELPER(insn, _, vmwrite, r, (u64)HOST_RSP, (u64)HOST_RIP) \
> > > __BUILD_VMX_VM_ENTRY_HELPER(insn, __evmcs, mov, m, \
> > > current_evmcs->host_rsp, current_evmcs->host_rip)
>
> eVMCS is a memory operand, i.e. it's MOV RSP, [evmcs->host_rip]. The existing
> evmcs_{vmresume,vmlaunch}() code is rather stupid and loads the address into a
> register, and then manually encodes MOV RSP, [<reg>].
Can't we keep it as a register operand just to avoid vmwrite_operand?
Does it actually hurt in any way?
>
> P.S. just in case you didn't already think this code is evil, the '&' in the
> above "[ret]"=&a"(ret)" is an earlyclobber that tells the compiler that
> RAX may be clobbered before all inputs and processed, i.e. prevents the
> compiler from using RAX for VMWRITE's register operand, or from using RAX
> to compute the memory operand for eVMCS's MOV.
TIL.
>
> P.P.S. even though the prototype for VMWRITE is "VMWRITE r64, r/m64", Intel is
> ass-backwards when it comes to syntax, i.e r/m64 is the source, not the
> dest, and so the field encoding *must* be a register operand. I.e. we
> can't do something like this:
Right, I was suggested using register operands for everything.
>
> diff --git tools/testing/selftests/kvm/lib/x86/vmx.c tools/testing/selftests/kvm/lib/x86/vmx.c
> index 1a8515de42b0..b5efd10950c7 100644
> --- tools/testing/selftests/kvm/lib/x86/vmx.c
> +++ tools/testing/selftests/kvm/lib/x86/vmx.c
> @@ -179,7 +179,10 @@ void load_vmcs(struct vmx_pages *vmx)
> vmclear(vmx->shadow_vmcs_gpa);
> }
>
> -#define __BUILD_VMX_VM_ENTRY_HELPER(insn, prefix, vmwrite_insn, vmwrite_operand, \
> +const u64 HOST_RSP_ENCODING = HOST_RSP;
> +const u64 HOST_RIP_ENCODING = HOST_RIP;
> +
> +#define __BUILD_VMX_VM_ENTRY_HELPER(insn, prefix, vmwrite_insn, \
> __host_rsp, __host_rip) \
> static int __##prefix##_##insn(void) \
> { \
> @@ -196,16 +199,17 @@ static int __##prefix##_##insn(void) \
> VMX_SWITCH_GPRS_ASM \
> "pop %%rax;" \
> : [ret]"=&a"(ret) \
> - : [host_rsp]__stringify(vmwrite_operand)(__host_rsp), \
> - [host_rip]__stringify(vmwrite_operand)(__host_rip), \
> + : [host_rsp]"m"(__host_rsp), \
> + [host_rip]"m"(__host_rip), \
I suppose we can use "r" here though?
> GUEST_REGS_OFFSETS \
> : "memory", "cc"); \
> return ret; \
> }
>
> #define BUILD_VMX_VM_ENTRY_HELPER(insn) \
> - __BUILD_VMX_VM_ENTRY_HELPER(insn, _, vmwrite, r, (u64)HOST_RSP, (u64)HOST_RIP) \
> - __BUILD_VMX_VM_ENTRY_HELPER(insn, __evmcs, mov, m, \
> + __BUILD_VMX_VM_ENTRY_HELPER(insn, _, vmwrite, \
> + HOST_RSP_ENCODING, HOST_RIP_ENCODING) \
> + __BUILD_VMX_VM_ENTRY_HELPER(insn, __evmcs, mov, \
> current_evmcs->host_rsp, current_evmcs->host_rip)
>
> BUILD_VMX_VM_ENTRY_HELPER(vmlaunch)
>