Re: [PATCH v3 12/13] KVM: selftests: Dedup assembly code for VMLAUNCH and VMRESUME
From: Sean Christopherson
Date: Thu Aug 27 2026 - 14:14:49 EST
On Thu, Aug 27, 2026, Yosry Ahmed wrote:
> On Thu, Aug 27, 2026 at 10:33 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> >
> > On Thu, Aug 27, 2026, Yosry Ahmed wrote:
> > > On Thu, Aug 27, 2026 at 10:17 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > > > 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?
> >
> > ...
> >
> > > > 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?
> >
> > No, because then the encoding for VMWRITE needs to be:
> >
> > vmwrite %%rsp, %[host_rsp]
> >
> > but for MOV/eVMCS needs to be:
> >
> > mov %%rsp, (%[host_rsp])
> >
> > Have fun feeding the '(' and ')' into the asm blob :-)
>
> Semi-joking, what if we pass in the entire instruction instead?
Then there needs to be separate parameters for RSP vs. RIP, and we still need to
pass different operands, *and* it bleeds information into the callers since they
would need to hardcode use of %%rax and of the named constraints.
As ugly as the proposed code is, IMO the maintenance implications of the below
is far worse than the subtle 'r' vs. 'm' (and on principle, I dislike passing an
address in a register and then manually encoding a memory operand).
#define __BUILD_VMX_VM_ENTRY_HELPER(insn, prefix, vmwrite_rsp, vmwrite_rip, \
__host_rsp, __host_rip) \
static int __##prefix##_##insn(void) \
{ \
int ret; \
\
__asm__ __volatile__("push $0;" \
vmwrite_rsp \
"lea 1f(%%rip), %%rax;" \
vmwrite_rip \
VMX_SWITCH_GPRS_ASM \
__stringify(insn)";" \
"incq (%%rsp);" \
"1: ;" \
VMX_SWITCH_GPRS_ASM \
"pop %%rax;" \
: [ret]"=&a"(ret) \
: [host_rsp]"r"((u64)__host_rsp), \
[host_rip]"r"((u64)__host_rip), \
GUEST_REGS_OFFSETS \
: "memory", "cc"); \
return ret; \
}
#define BUILD_VMX_VM_ENTRY_HELPER(insn) \
__BUILD_VMX_VM_ENTRY_HELPER(insn, _, \
"vmwrite %%rsp, %[host_rsp];", \
"vmwrite %%rax, %[host_rip];", \
HOST_RSP, HOST_RIP) \
__BUILD_VMX_VM_ENTRY_HELPER(insn, __evmcs, \
"mov %%rsp, (%[host_rsp]);", \
"mov %%rsp, (%[host_rip]);", \
¤t_evmcs->host_rsp, ¤t_evmcs->host_rip)