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

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Feb 08, 2022 at 12:26:24PM -0700, Alex Williamson wrote:

> > 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.
> 
> I agree, but I've resigned that I've lost that battle.  Both mdev vGPU
> vendors make specific assumptions about running on a VM. 

The vGPU's are not as egregious though, are they?

> > 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.
> 
> As I understand the discussion here:
> 
> https://lore.kernel.org/all/20220204211536.321475-15-mjrosato@linux.ibm.com/
> 
> The assumption is that there is no non-KVM userspace currently.  This
> seems like a regression to me.

Indeed, I definitely don't like it either. This is not VFIO if is
just driving KVM.

I would prefer they add a function to get the 'struct device *' from a
VFIO device fd and drive more of this from kvm, as appropriate.

> > > 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?
> 
> Essentially just:
> 
> static const struct vfio_device_ops vfio_pci_ops = {
>         .name           = "vfio-pci",
> #ifdef CONFIG_S390
>         .open_device    = vfio_zpci_open_device,
>         .close_device   = vfio_zpci_close_device,
>         .ioctl          = vfio_zpci_ioctl,
> #else
>         .open_device    = vfio_pci_open_device,
>         .close_device   = vfio_pci_core_close_device,
>         .ioctl          = vfio_pci_core_ioctl,
> #endif
>         .read           = vfio_pci_core_read,
>         .write          = vfio_pci_core_write,
>         .mmap           = vfio_pci_core_mmap,
>         .request        = vfio_pci_core_request,
>         .match          = vfio_pci_core_match,
> };
> 
> It would at least provide more validation/exercise of the core/vendor
> split.  Thanks,

This would have to be in every pci driver - this is not just code the
universal vfio-pci has to enable, but every migration driver/etc too.

And we will need it again in vfio-cxl for s390 in 10 years too..

So, I think this approach is the right one, asided from the
philosophical question of being so tightly linking s390 vfio to KVM.

Jason