Re: [V3 10/11] KVM: selftests: Add ucall pool based implementation

[Andrew Jones <andrew.jones@linux.dev>]

On Thu, Aug 18, 2022 at 04:00:40PM +0000, Sean Christopherson wrote:
> On Tue, Aug 16, 2022, Andrew Jones wrote:
> > On Wed, Aug 10, 2022 at 08:20:32AM -0700, Peter Gonda wrote:
> > > diff --git a/tools/testing/selftests/kvm/lib/aarch64/ucall.c b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> > > index 132c0e98bf49..ee70531e8e51 100644
> > > --- a/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> > > +++ b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> > > @@ -81,12 +81,16 @@ void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
> > >  
> > >  	if (run->exit_reason == KVM_EXIT_MMIO &&
> > >  	    run->mmio.phys_addr == (uint64_t)ucall_exit_mmio_addr) {
> > > -		vm_vaddr_t gva;
> > > +		uint64_t ucall_addr;
> > 
> > Why change this vm_vaddr_t to a uint64_t? We shouldn't, because...
> > 
> > >  
> > >  		TEST_ASSERT(run->mmio.is_write && run->mmio.len == 8,
> > >  			    "Unexpected ucall exit mmio address access");
> > > -		memcpy(&gva, run->mmio.data, sizeof(gva));
> > > -		return addr_gva2hva(vcpu->vm, gva);
> > > +		memcpy(&ucall_addr, run->mmio.data, sizeof(ucall_addr));
> > 
> > ...here we assume it's a vm_vaddr_t and only...
> > 
> > > +
> > > +		if (vcpu->vm->use_ucall_pool)
> > > +			return (void *)ucall_addr;
> > 
> > ...here do we know otherwise. So only here should be any casting.
> 
> It technically should be a union, because if sizeof(vm_vaddr_t) < sizeof(void *)
> then declaring it as a vm_addr_t will do the wrong thing.  But then it's possible
> that this could read too many bytes and inducs failure.  So I guess what we really
> need is a "static_assert(sizeof(vm_vaddr_t) == sizeof(void *))".

ack

> 
> But why is "use_ucall_pool" optional?  Unless there's a use case that fundamentally
> conflicts with the pool approach, let's make the pool approach the _only_ approach.
> IIRC, ARM's implementation isn't thread safe, i.e. there's at least one other use
> case that _needs_ the pool implementation.

Really? The ucall structure is on the vcpu's stack like the other
architectures. Ah, you're probably thinking about the shared address used
to exit to userspace. The address doesn't matter as long as no VM maps
it, but, yes, a multi-VM test where the VMs have different maps could end
up breaking ucalls for one or more VMs. It wouldn't be hard to make that
address per-VM, though, if ever necessary.

> 
> By supporting both, we are signing ourselves up for extra maintenance and pain,
> e.g. inevitably we'll break whichever option isn't the default and not notice for
> quite some time.

uc pools are currently limited to a single VM. That could be changed, but
at the expense of even more code to maintain. The simple uc implementation
is, well, simple, and also supports multiple VMs. I'd prefer we keep that
one and keep it as the default.

Thanks,
drew