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

[Matthew Rosato]

On 4/27/22 11:14 AM, Jason Gunthorpe wrote:
> On Tue, Apr 26, 2022 at 04:08:36PM -0400, Matthew Rosato wrote:
>
> > +int kvm_s390_pci_register_kvm(struct zpci_dev *zdev, struct kvm *kvm)
> > +{
> > +	if (!zdev)
> > +		return 0;
> > +
> > +	/*
> > +	 * Register device with this KVM (or remove the KVM association if 0).
> > +	 * If interpetation facilities are available, enable them and let
> > +	 * userspace indicate whether or not they will be used (specify SHM bit
> > +	 * to disable).
> > +	 */
> > +	if (kvm)
> > +		return register_kvm(zdev, kvm);
> > +	else
> > +		return unregister_kvm(zdev);
> > +}
> > +EXPORT_SYMBOL_GPL(kvm_s390_pci_register_kvm);
>
> I think it is cleaner to expose both the register/unregister APIs and
> not multiplex them like this

OK

> > +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 -- 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.

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

Then, since we know each device will get closed (either by userspace or 
by kvm), I don't need something like kvm_s390_pci_clear_list at all.

> If you keep it like this then the locking in register/unregister looks
> not broad enough and has to cover the zdev->kzdev also.

But I would still need to revisit the locking with the above idea.

> Overall I think it is OK designed like this, aside from the ugly
> symbol_get in vfio which I hope you can resolve.
>
> Jason