Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 18 Aug 2022 16:00:40 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Andrew Jones <andrew.jones@linux.dev>
Cc:     Peter Gonda <pgonda@google.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, marcorr@google.com,
        michael.roth@amd.com, thomas.lendacky@amd.com, joro@8bytes.org,
        mizhang@google.com, pbonzini@redhat.com, vannapurve@google.com
Subject: Re: [V3 10/11] KVM: selftests: Add ucall pool based implementation
Message-ID: <Yv5iKJbjW5VseagS@google.com>
References: <20220810152033.946942-1-pgonda@google.com>
 <20220810152033.946942-11-pgonda@google.com>
 <20220816161350.b7x5brnyz5pyi7te@kamzik>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220816161350.b7x5brnyz5pyi7te@kamzik>
Precedence: bulk

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 *))".

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.

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.