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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aEIelSM6viCxaHj7@google.com>
Date: Thu, 5 Jun 2025 15:47:49 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Tom Lendacky <thomas.lendacky@....com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, linux-kernel@...r.kernel.org, kvm@...r.kernel.org, 
	roy.hopkins@...e.com, ashish.kalra@....com, michael.roth@....com, 
	jroedel@...e.de, nsaenz@...zon.com, anelkz@...zon.de, 
	James.Bottomley@...senpartnership.com
Subject: Re: [PATCH 13/29] KVM: implement vCPU creation for extra planes

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@...hat.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.