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

[Sean Christopherson <seanjc@google.com>]

On Wed, Aug 26, 2026, Yosry Ahmed wrote:
> On Wed, Aug 26, 2026 at 4:39 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > Dedup the assembly code from VMLAUNCH vs. VMRESUME, the difference is
> > literally only the actual VM-Enter instruction.
> 
> You couldn't resist macrofiying this too.

It's either that or throw a branch in the asm blob, which is surprisingly
difficult because VMX_SWITCH_GPRS_ASM subtly clobbers RFLAGS and obviously
clobbers GPRs.  Which basically leaves pushing the value on the stack, which is
totally doable, but still annoying, and needs to be done immediately, before
vmcs.HOST_RSP is loaded.  Branching at runtime also makes the generated code
harder to read, e.g. when debugging, as it's not immediately obvious which
instruction was actually attempted.

That said, looking at this again made me realize evmcs_{vmlaunch,vmresume}() have
the same core copy+paste mess.  Blech.  Macrofying the standard flows but not the
eVMCS flows is rather silly.

Inlining these blobs *twice* (VMCS vs. eVMCS) at every VM-Enter is also ridiculous.
E.g. at a glance, it's responsible for something ~100k bytes of code in the compiled
state_test.  While the code footprint of selftests isn't a priority, that's still
absurd.

So instead of macrofying everything, how about this?  Depending on how one feels
about macro shenanigans, it's either horrific or amazing.  Or both.  But IMO it's
worth eliminating all of the copy+paste, and it makes the control flow much easier
to read, which in practice is likely what most people care about?  At least until
the entry/exit sequence fails and they have to debug macro hell :-)

vmx.c:

  
  #define __BUILD_VMX_VM_ENTRY_HELPER(insn, prefix, vmwrite_insn, vmwrite_operand,	\
  				    __host_rsp, __host_rip)				\
  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)
  
  BUILD_VMX_VM_ENTRY_HELPER(vmlaunch)
  BUILD_VMX_VM_ENTRY_HELPER(vmresume)
  
  int __vmlaunch(void)
  {
  	if (enable_evmcs) {
  		current_evmcs->hv_clean_fields = 0;
  		return ____evmcs_vmlaunch();
  	}
  
  	return ____vmlaunch();
  }
  
  int __vmresume(void)
  {
  	if (enable_evmcs) {
  		/* HOST_RIP */
  		current_evmcs->hv_clean_fields &= ~HV_VMX_ENLIGHTENED_CLEAN_FIELD_HOST_GRP1;
  		/* HOST_RSP */
  		current_evmcs->hv_clean_fields &= ~HV_VMX_ENLIGHTENED_CLEAN_FIELD_HOST_POINTER;
  		return ____evmcs_vmresume();
  	}
  
  	return ____vmresume();
  }

vmx.h:

  int __vmlaunch(void);
  int __vmresume(void);
  
  static inline int vmlaunch(void)
  {
  	return __vmlaunch();
  }
  
  static inline int vmresume(void)
  {
  	return __vmresume();
  }

And then the next patch becomes:

    KVM: selftests: Assert success in vmlaunch() and vmresume()
    
    Assert success in the "outer" APIs for VMLAUNCH and VMRESUME to dedup a
    pile of code, and switch to the double-underscores versions as necessary,
    to make it more obvious which paths expect VM-Enter *failure*.
    
    Signed-off-by: Sean Christopherson <seanjc@google.com>

diff --git tools/testing/selftests/kvm/include/x86/vmx.h tools/testing/selftests/kvm/include/x86/vmx.h
index d0ac8e128d98..1419043b41dd 100644
--- tools/testing/selftests/kvm/include/x86/vmx.h
+++ tools/testing/selftests/kvm/include/x86/vmx.h
@@ -362,14 +362,14 @@ static inline u64 vmptrst(void)
 int __vmlaunch(void);
 int __vmresume(void);
 
-static inline int vmlaunch(void)
+static inline void vmlaunch(void)
 {
-	return __vmlaunch();
+	__GUEST_ASSERT(!__vmlaunch(), "vmlaunch hit VM-Fail");
 }
 
-static inline int vmresume(void)
+static inline void vmresume(void)
 {
-	return __vmresume();
+	__GUEST_ASSERT(!__vmresume(), "vmresume hit VM-Fail");
 }
 
 static inline void vmcall(void)

> > +#define BUILD_VMX_VM_ENTRY_HELPERS(insn)                               \
> 
> s/HELPERS/HELPER?
> 
> or maybe better: DEFINE_VMX_VM_ENTRY_HELPER()?

Because after the next patch, there are two helpers for each instruction, vmresume()
and __vmresume(), and churning the macros to add the 'S' felt silly.  But, with the
above, this is a moot point.

> With the rename:
> 
> Reviewed-by: Yosry Ahmed <yosry@kernel.org>
> 
> (Who's f734222792@gmail.com?)

An anonymous bug reporter: https://bugzilla.kernel.org/show_bug.cgi?id=221841