Re: [PATCH 8/9] vfio/pci: use x86 naming instead of igd

[Cornelia Huck <cohuck@redhat.com>]

On Mon, 1 Feb 2021 11:42:30 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Mon, 1 Feb 2021 12:49:12 -0500
> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
> 
> > On 2/1/21 12:14 PM, Cornelia Huck wrote:  
> > > On Mon, 1 Feb 2021 16:28:27 +0000
> > > Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
> > >     
> > >> This patch doesn't change any logic but only align to the concept of
> > >> vfio_pci_core extensions. Extensions that are related to a platform
> > >> and not to a specific vendor of PCI devices should be part of the core
> > >> driver. Extensions that are specific for PCI device vendor should go
> > >> to a dedicated vendor vfio-pci driver.    
> > > 
> > > My understanding is that igd means support for Intel graphics, i.e. a
> > > strict subset of x86. If there are other future extensions that e.g.
> > > only make sense for some devices found only on AMD systems, I don't
> > > think they should all be included under the same x86 umbrella.
> > > 
> > > Similar reasoning for nvlink, that only seems to cover support for some
> > > GPUs under Power, and is not a general platform-specific extension IIUC.
> > > 
> > > We can arguably do the zdev -> s390 rename (as zpci appears only on
> > > s390, and all PCI devices will be zpci on that platform), although I'm
> > > not sure about the benefit.    
> > 
> > As far as I can tell, there isn't any benefit for s390 it's just 
> > "re-branding" to match the platform name rather than the zdev moniker, 
> > which admittedly perhaps makes it more clear to someone outside of s390 
> > that any PCI device on s390 is a zdev/zpci type, and thus will use this 
> > extension to vfio_pci(_core).  This would still be true even if we added 
> > something later that builds atop it (e.g. a platform-specific device 
> > like ism-vfio-pci).  Or for that matter, mlx5 via vfio-pci on s390x uses 
> > these zdev extensions today and would need to continue using them in a 
> > world where mlx5-vfio-pci.ko exists.
> > 
> > I guess all that to say: if such a rename matches the 'grand scheme' of 
> > this design where we treat arch-level extensions to vfio_pci(_core) as 
> > "vfio_pci_(arch)" then I'm not particularly opposed to the rename.  But 
> > by itself it's not very exciting :)  
> 
> This all seems like the wrong direction to me.  The goal here is to
> modularize vfio-pci into a core library and derived vendor modules that
> make use of that core library.  If existing device specific extensions
> within vfio-pci cannot be turned into vendor modules through this
> support and are instead redefined as platform specific features of the
> new core library, that feels like we're already admitting failure of
> this core library to support known devices, let alone future devices.
> 
> IGD is a specific set of devices.  They happen to rely on some platform
> specific support, whose availability should be determined via the
> vendor module probe callback.  Packing that support into an "x86"
> component as part of the core feels not only short sighted, but also
> avoids addressing the issues around how userspace determines an optimal
> module to use for a device.

Hm, it seems that not all current extensions to the vfio-pci code are
created equal.

IIUC, we have igd and nvlink, which are sets of devices that only show
up on x86 or ppc, respectively, and may rely on some special features
of those architectures/platforms. The important point is that you have
a device identifier that you can match a driver against.

On the other side, we have the zdev support, which both requires s390
and applies to any pci device on s390. If we added special handling for
ISM on s390, ISM would be in a category similar to igd/nvlink.

Now, if somebody plugs a mlx5 device into an s390, we would want both
the zdev support and the specialized mlx5 driver. Maybe zdev should be
some kind of library that can be linked into normal vfio-pci, into
vfio-pci-mlx5, and a hypothetical vfio-pci-ism? You always want zdev on
s390 (if configured into the kernel).