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

[Alex Williamson]

On Mon, 1 Feb 2021 12:49:12 -0500
Matthew Rosato <mjrosato@xxxxxxxxxxxxx> wrote:

> On 2/1/21 12:14 PM, Cornelia Huck wrote:
> > On Mon, 1 Feb 2021 16:28:27 +0000
> > Max Gurtovoy <mgurtovoy@xxxxxxxxxx> 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.  Thanks,

Alex

> >> For now, x86 extensions will include only igd.
> >>
> >> Signed-off-by: Max Gurtovoy <mgurtovoy@xxxxxxxxxx>
> >> ---
> >>   drivers/vfio/pci/Kconfig                            | 13 ++++++-------
> >>   drivers/vfio/pci/Makefile                           |  2 +-
> >>   drivers/vfio/pci/vfio_pci_core.c                    |  2 +-
> >>   drivers/vfio/pci/vfio_pci_private.h                 |  2 +-
> >>   drivers/vfio/pci/{vfio_pci_igd.c => vfio_pci_x86.c} |  0
> >>   5 files changed, 9 insertions(+), 10 deletions(-)
> >>   rename drivers/vfio/pci/{vfio_pci_igd.c => vfio_pci_x86.c} (100%)  
> > 
> > (...)
> >   
> >> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> >> index c559027def2d..e0e258c37fb5 100644
> >> --- a/drivers/vfio/pci/vfio_pci_core.c
> >> +++ b/drivers/vfio/pci/vfio_pci_core.c
> >> @@ -328,7 +328,7 @@ static int vfio_pci_enable(struct vfio_pci_device *vdev)
> >>   
> >>   	if (vfio_pci_is_vga(pdev) &&
> >>   	    pdev->vendor == PCI_VENDOR_ID_INTEL &&
> >> -	    IS_ENABLED(CONFIG_VFIO_PCI_IGD)) {
> >> +	    IS_ENABLED(CONFIG_VFIO_PCI_X86)) {
> >>   		ret = vfio_pci_igd_init(vdev);  
> > 
> > This one explicitly checks for Intel devices, so I'm not sure why you
> > want to generalize this to x86?
> >   
> >>   		if (ret && ret != -ENODEV) {
> >>   			pci_warn(pdev, "Failed to setup Intel IGD regions\n");  
> >   
>