Re: [PATCH v3 24/30] vfio-pci/zdev: wire up group notifier

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Feb 08, 2022 at 10:43:19AM -0700, Alex Williamson wrote:
> On Fri,  4 Feb 2022 16:15:30 -0500
> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
> 
> > KVM zPCI passthrough device logic will need a reference to the associated
> > kvm guest that has access to the device.  Let's register a group notifier
> > for VFIO_GROUP_NOTIFY_SET_KVM to catch this information in order to create
> > an association between a kvm guest and the host zdev.
> > 
> > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> >  arch/s390/include/asm/kvm_pci.h  |  2 ++
> >  drivers/vfio/pci/vfio_pci_core.c |  2 ++
> >  drivers/vfio/pci/vfio_pci_zdev.c | 46 ++++++++++++++++++++++++++++++++
> >  include/linux/vfio_pci_core.h    | 10 +++++++
> >  4 files changed, 60 insertions(+)
> > 
> > diff --git a/arch/s390/include/asm/kvm_pci.h b/arch/s390/include/asm/kvm_pci.h
> > index e4696f5592e1..16290b4cf2a6 100644
> > +++ b/arch/s390/include/asm/kvm_pci.h
> > @@ -16,6 +16,7 @@
> >  #include <linux/kvm.h>
> >  #include <linux/pci.h>
> >  #include <linux/mutex.h>
> > +#include <linux/notifier.h>
> >  #include <asm/pci_insn.h>
> >  #include <asm/pci_dma.h>
> >  
> > @@ -32,6 +33,7 @@ struct kvm_zdev {
> >  	u64 rpcit_count;
> >  	struct kvm_zdev_ioat ioat;
> >  	struct zpci_fib fib;
> > +	struct notifier_block nb;
> >  };
> >  
> >  int kvm_s390_pci_dev_open(struct zpci_dev *zdev);
> > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> > index f948e6cd2993..fc57d4d0abbe 100644
> > +++ b/drivers/vfio/pci/vfio_pci_core.c
> > @@ -452,6 +452,7 @@ void vfio_pci_core_close_device(struct vfio_device *core_vdev)
> >  
> >  	vfio_pci_vf_token_user_add(vdev, -1);
> >  	vfio_spapr_pci_eeh_release(vdev->pdev);
> > +	vfio_pci_zdev_release(vdev);
> >  	vfio_pci_core_disable(vdev);
> >  
> >  	mutex_lock(&vdev->igate);
> > @@ -470,6 +471,7 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_close_device);
> >  void vfio_pci_core_finish_enable(struct vfio_pci_core_device *vdev)
> >  {
> >  	vfio_pci_probe_mmaps(vdev);
> > +	vfio_pci_zdev_open(vdev);
> >  	vfio_spapr_pci_eeh_open(vdev->pdev);
> >  	vfio_pci_vf_token_user_add(vdev, 1);
> >  }
> 
> If this handling were for a specific device, I think we'd be suggesting
> this is the point at which we cross over to a vendor variant making use
> of vfio-pci-core rather than hooking directly into the core code. 

Personally, I think it is wrong layering for VFIO to be aware of KVM
like this. This marks the first time that VFIO core code itself is
being made aware of the KVM linkage.

It copies the same kind of design the s390 specific mdev use of
putting VFIO in charge of KVM functionality. If we are doing this we
should just give up and admit that KVM is a first-class part of struct
vfio_device and get rid of the notifier stuff too, at least for s390.

Reading the patches and descriptions pretty much everything is boiling
down to 'use vfio to tell the kvm architecture code to do something' -
which I think needs to be handled through a KVM side ioctl.

Or, at the very least, everything needs to be described in some way
that makes it clear what is happening to userspace, without kvm,
through these ioctls.

This seems especially true now that it seems s390 PCI support is
almost truely functional, with actual new userspace instructions to
issue MMIO operations that work outside of KVM. 

I'm not sure how this all fits together, but I would expect an outcome
where DPDK could run on these new systems and not have to know
anything more about s390 beyond using the proper MMIO instructions via
some compilation time enablement.

(I've been reviewing s390 patches updating rdma for a parallel set of
stuff)

> this is meant to extend vfio-pci proper for the whole arch.  Is there a
> compromise in using #ifdefs in vfio_pci_ops to call into zpci specific
> code that implements these arch specific hooks and the core for
> everything else?  SPAPR code could probably converted similarly, it
> exists here for legacy reasons. [Cc Jason]

I'm not sure I get what you are suggesting? Where would these ifdefs
be?

> Also, please note the DEVICE_FEATURE generalizations in the latest
> series from NVIDIA for mlx5 migration support:
 
> https://lore.kernel.org/all/20220207172216.206415-8-yishaih@nvidia.com/

Yes, please don't implement a bunch of new FEATURE code without taking
the cleanup patches for feature support from that series too.

I can put them on a branch for you if you needed.

Jason