Re: [PATCH v16 18/20] KVM: selftests: Add support for per-VM ucall ops on x86

[Sean Christopherson <seanjc@google.com>]

On Fri, Nov 06, 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] KVM: selftests: ucall_x86_ops is a host global instead of per-VM state
> --
> 
> commit 220d73be4cec0aafb094f8711c312a5a8ae361c9
> Author: Sean Christopherson <seanjc@google.com>
> 
> KVM: selftests: Add support for per-VM ucall ops on x86
> 
> This commit introduces an ops structure for x86 ucall handling so that
> different VM types, such as TDX VMs, can wire up their own do_ucall and
> get_ucall callbacks.
> 
> > diff --git a/tools/testing/selftests/kvm/lib/x86/ucall.c b/tools/testing/selftests/kvm/lib/x86/ucall.c
> > index 38050c60a0670..3aec4dd46d1b2 100644
> > --- a/tools/testing/selftests/kvm/lib/x86/ucall.c
> > +++ b/tools/testing/selftests/kvm/lib/x86/ucall.c
> [ ... ]
> > @@ -26,3 +26,26 @@ void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
> >  	}
> >  	return NULL;
> >  }
> > +
> > +static struct {
> > +	void (*do_ucall)(gva_t uc);
> > +	void *(*get_ucall)(struct kvm_vcpu *vcpu);
> > +} ucall_x86_ops = {
> > +	.do_ucall = ucall_x86_do_ucall,
> > +	.get_ucall = ucall_x86_get_ucall,
> > +};
> > +
> > +void ucall_arch_init(struct kvm_vm *vm, gpa_t mmio_gpa)
> > +{
> > +	sync_global_to_guest(vm, ucall_x86_ops);
> > +}
> > +
> > +void ucall_arch_do_ucall(gva_t uc)
> > +{
> > +	ucall_x86_ops.do_ucall(uc);
> > +}
> > +
> > +void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
> > +{
> > +	return ucall_x86_ops.get_ucall(vcpu);
> > +}
> 
> [Severity: Medium]
> As reported by Sashiko on v15 [1], ucall_x86_ops in
> tools/testing/selftests/kvm/lib/x86/ucall.c is defined as a single
> process-wide static variable on the host rather than per-VM state.
> 
> While ucall_arch_init() uses sync_global_to_guest(vm, ucall_x86_ops) to copy
> the structure into each guest VM's private address space for guest-side
> ucall_arch_do_ucall() execution, ucall_arch_get_ucall() executes on the host
> and dispatches through the single host-side ucall_x86_ops.get_ucall pointer
> instead of per-VM state associated with vcpu->vm.
> 
> In the following patch ("KVM: selftests: Add support for TDX ucalls, via
> TDVMCALL_REPORT_FATAL_ERROR"), ucall_arch_init() mutates the host
> ucall_x86_ops in place when is_tdx_vm(vm) is true without restoring the
> default x86 callbacks when is_tdx_vm(vm) is false. If a selftest process
> creates a TDX VM via __vm_create() -> ucall_init() -> ucall_arch_init() and
> concurrently or subsequently creates and runs a non-TDX VM, won't the
> non-TDX VM inherit the TDX ucall callbacks both in guest memory and on the
> host in get_ucall() -> ucall_arch_get_ucall()?

/facepalm

I over-engineered this.  OMG, and I complete forgot about write_guest_global(),
which I added specifically to make ucalls multi-VM friendly, i.e. we can even
make the initialization thread safe.  Hilarious.

We can either do:

  void (*x86_do_ucall)(gva_t uc);
  
  void ucall_arch_init(struct kvm_vm *vm, gpa_t mmio_gpa)
  {
    	write_guest_global(vm, x86_do_ucall, is_tdx_vm(vm) ? ucall_tdx_do_ucall :
  							     ucall_x86_do_ucall);
  }
  
  void ucall_arch_do_ucall(gva_t uc)
  {
  	x86_do_ucall(uc);
  }
  
  void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
  {
  	if (is_tdx_vm(vcpu->vm))
  		return ucall_tdx_get_ucall(vcpu);
  
  	return ucall_x86_get_ucall(vcpu);
  }
  
or:

  static bool ucall_is_tdx_vm;
  
  void ucall_arch_init(struct kvm_vm *vm, gpa_t mmio_gpa)
  {
  	write_guest_global(vm, ucall_is_tdx_vm, is_tdx_vm(vm));
  }
  
  void ucall_arch_do_ucall(gva_t uc)
  {
  	if (ucall_is_tdx_vm)
  		ucall_tdx_do_ucall(uc);
  	else
  		ucall_x86_do_ucall(uc);
  }
  
  void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
  {
  	if (is_tdx_vm(vcpu->vm))
  		return ucall_tdx_get_ucall(vcpu);
  
  	return ucall_x86_get_ucall(vcpu);
  }

or a very slight variant on option #2 to make the paths visually similar:

  static bool ucall_is_tdx_vm;
  
  void ucall_arch_init(struct kvm_vm *vm, gpa_t mmio_gpa)
  {
  	write_guest_global(vm, ucall_is_tdx_vm, is_tdx_vm(vm));
  }
  
  void ucall_arch_do_ucall(gva_t uc)
  {
  	if (ucall_is_tdx_vm)
  		return ucall_tdx_do_ucall(uc);
  
  	return ucall_x86_do_ucall(uc);
  }
  
  void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
  {
  	if (is_tdx_vm(vcpu->vm))
  		return ucall_tdx_get_ucall(vcpu);
  
  	return ucall_x86_get_ucall(vcpu);
  }

I strongly vote for the simple flag, with a preference for the double-return
variant.  I'm going to violate my own "don't do in-reply-to patches" rule and
post a v16.1 patch in response, to see if that's trigger another round of Sashiko
review.

P.S. I'VE GOT THE POWER:

# ./tdx_vm_test
Random seed: 0x26085169
TAP version 13
1..1
# Starting 1 tests from 1 test cases.
#  RUN           global.verify_td_lifecycle ...
#            OK  global.verify_td_lifecycle
ok 1 global.verify_td_lifecycle
# PASSED: 1 / 1 tests passed.
# Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0