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

Jason Gunthorpe
2022-02-08T22:23:24Z

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