Re: [PATCH v6 15/21] KVM: s390: pci: add routines to start/stop interpretive execution

From: Jason Gunthorpe <jgg@nvidia.com>
To: Matthew Rosato <mjrosato@linux.ibm.com>
Cc: linux-s390@vger.kernel.org, alex.williamson@redhat.com,
	cohuck@redhat.com, schnelle@linux.ibm.com, farman@linux.ibm.com,
	pmorel@linux.ibm.com, borntraeger@linux.ibm.com,
	hca@linux.ibm.com, gor@linux.ibm.com,
	gerald.schaefer@linux.ibm.com, agordeev@linux.ibm.com,
	svens@linux.ibm.com, frankja@linux.ibm.com, david@redhat.com,
	imbrenda@linux.ibm.com, vneethv@linux.ibm.com,
	oberpar@linux.ibm.com, freude@linux.ibm.com, thuth@redhat.com,
	pasic@linux.ibm.com, pbonzini@redhat.com, corbet@lwn.net,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-doc@vger.kernel.org
Subject: Re: [PATCH v6 15/21] KVM: s390: pci: add routines to start/stop interpretive execution
Date: Thu, 28 Apr 2022 09:28:45 -0300	[thread overview]
Message-ID: <20220428122845.GC8364@nvidia.com> (raw)
In-Reply-To: <b9575614-a234-0c36-7601-9c09d159c3b0@linux.ibm.com>

On Wed, Apr 27, 2022 at 04:20:10PM -0400, Matthew Rosato wrote:
> > > +void kvm_s390_pci_clear_list(struct kvm *kvm)
> > > +{
> > > +	struct kvm_zdev *tmp, *kzdev;
> > > +	LIST_HEAD(remove);
> > > +
> > > +	spin_lock(&kvm->arch.kzdev_list_lock);
> > > +	list_for_each_entry_safe(kzdev, tmp, &kvm->arch.kzdev_list, entry)
> > > +		list_move_tail(&kzdev->entry, &remove);
> > > +	spin_unlock(&kvm->arch.kzdev_list_lock);
> > > +
> > > +	list_for_each_entry_safe(kzdev, tmp, &remove, entry)
> > > +		unregister_kvm(kzdev->zdev);
> > 
> > Hum, I wonder if this is a mistake in kvm:
> > 
> > static void kvm_destroy_vm(struct kvm *kvm)
> > {
> > [..]
> > 	kvm_arch_destroy_vm(kvm);
> > 	kvm_destroy_devices(kvm);
> > 
> > kvm_destroy_devices() triggers the VFIO notifier with NULL. Indeed for
> > correctness I would expect the VFIO users to have been notified to
> > release the kvm before the kvm object becomes partially destroyed?
> > 
> > Maybe you should investigate re-ordering this at the KVM side and just
> > WARN_ON(!list_empty) in the arch code?
> > 
> > (vfio has this odd usage model where it should use the kvm pointer
> > without taking a ref on it so long as the unregister hasn't been
> > called)
> > 
> 
> The issue there is that I am unregistering the notifier during close_device
> for each zPCI dev, which will have already happened

And at that moment you have to clean up the arch stuff too, it
shouldn't be left floating around once the driver that installed it
looses access to the kvm.

> -- so by the time we get to kvm_destroy_devices(), whether it's
> before or after kvm_arch_destroy_vm, there are no longer any zPCI
> notifiers registered that will trigger.

I don't think that is strictly true, there is no enforced linkage
between the lifetime of the kvm FD and the lifetime of the VFIO device
FD. qemu probably orders them the way you say.

> One way to solve this is to have the zpci close_device hook also trigger the
> work that a KVM_DEV_VFIO_GROUP_DEL would (if the device is being closed, the
> KVM association for that device isn't applicable anymore so go ahead and
> clean up).

That makes the most sense - but think about what happens if the KVM fd
is closed while the device FD is still open..

Jason