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

[Matthew Rosato]

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 :)

> > 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");