Re: [PATCH 13/29] KVM: implement vCPU creation for extra planes

[Sean Christopherson <seanjc@google.com>]

On Mon, Apr 21, 2025, Tom Lendacky wrote:
> On 4/1/25 11:10, Paolo Bonzini wrote:
> > For userspace to have fun with planes it is probably useful to let them
> > create vCPUs on the non-zero planes as well.  Since such vCPUs are backed
> > by the same struct kvm_vcpu, these are regular vCPU file descriptors except
> > that they only allow a small subset of ioctls (mostly get/set) and they
> > share some of the backing resources, notably vcpu->run.
> > 
> > TODO: prefault might be useful on non-default planes as well?
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  Documentation/virt/kvm/locking.rst |   3 +
> >  include/linux/kvm_host.h           |   4 +-
> >  include/uapi/linux/kvm.h           |   1 +
> >  virt/kvm/kvm_main.c                | 167 +++++++++++++++++++++++------
> >  4 files changed, 142 insertions(+), 33 deletions(-)
> > 
> 
> > @@ -4200,8 +4223,13 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
> >  	 * release semantics, which ensures the write is visible to kvm_get_vcpu().
> >  	 */
> >  	vcpu->plane = -1;
> > -	vcpu->vcpu_idx = atomic_read(&kvm->online_vcpus);
> > -	r = xa_insert(&kvm->planes[0]->vcpu_array, vcpu->vcpu_idx, vcpu, GFP_KERNEL_ACCOUNT);
> > +	if (plane->plane)
> > +		vcpu->vcpu_idx = atomic_read(&kvm->online_vcpus);
> > +	else
> > +		vcpu->vcpu_idx = plane0_vcpu->vcpu_idx;
> 
> Don't you want the atomic_read() for the plane0 vCPU and use the plane0
> vcpu->idx value for non-zero plane vCPUs?

+1, this looks backwards to me as well.

> 
> > +
> > +	r = xa_insert(&plane->vcpu_array, vcpu->vcpu_idx,
> > +		      vcpu, GFP_KERNEL_ACCOUNT);
> >  	WARN_ON_ONCE(r == -EBUSY);
> >  	if (r)
> >  		goto unlock_vcpu_destroy;
> > @@ -4220,13 +4248,14 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
> >  	if (r < 0)
> >  		goto kvm_put_xa_erase;
> >  
> > -	atomic_inc(&kvm->online_vcpus);
> > +	if (!plane0_vcpu)
> 
> It looks like plane0_vcpu will always have value, either from input or
> assigned in an else path earlier in the code. Should this be
> "!plane->plane" ?

Even if plane0_vcpu were viable, my vote is for !plane->plane, because that makes
it much more obvious that only plane0 bumps the count.