Re: [PATCH v3 12/13] KVM: selftests: Dedup assembly code for VMLAUNCH and VMRESUME

[Yosry Ahmed]

On Thu, Aug 27, 2026 at 11:07 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> 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).

Yeah this isn't pretty either. I don't feel strongly either way tbh,
so whatever you think is best.

One other option is accepting some duplication, and letting the eVMCS
variants be separate. Basically keep your original patch as-is, and
just add a similar one for eVMCS variants.

Again, no strong preference, and I generally prefer your proposal for
deduplicating all of them if it wasn't for the subtle 'r' vs 'm'
thingy :/

>
> #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]);",                        \
>                                     &current_evmcs->host_rsp, &current_evmcs->host_rip)