Re: [PATCH 2/2] vfio/pci: Remove console drivers

[Alex Williamson <alex.williamson@redhat.com>]

Hi Thomas,

On Wed, 8 Jun 2022 13:11:21 +0200
Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Hi Alex
> 
> Am 06.06.22 um 19:53 schrieb Alex Williamson:
> > Console drivers can create conflicts with PCI resources resulting in
> > userspace getting mmap failures to memory BARs.  This is especially evident
> > when trying to re-use the system primary console for userspace drivers.
> > Attempt to remove all nature of conflicting drivers as part of our VGA
> > initialization.  
> 
> First a dumb question about your use case.  You want to assign a PCI 
> graphics card to a virtual machine and need to remove the generic driver 
> from the framebuffer?

Exactly.
 
> > Reported-by: Laszlo Ersek <lersek@redhat.com>
> > Tested-by: Laszlo Ersek <lersek@redhat.com>
> > Suggested-by: Gerd Hoffmann <kraxel@redhat.com>
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >   drivers/vfio/pci/vfio_pci_core.c |   17 +++++++++++++++++
> >   1 file changed, 17 insertions(+)
> > 
> > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> > index a0d69ddaf90d..e0cbcbc2aee1 100644
> > --- a/drivers/vfio/pci/vfio_pci_core.c
> > +++ b/drivers/vfio/pci/vfio_pci_core.c
> > @@ -13,6 +13,7 @@
> >   #include <linux/device.h>
> >   #include <linux/eventfd.h>
> >   #include <linux/file.h>
> > +#include <linux/fb.h>
> >   #include <linux/interrupt.h>
> >   #include <linux/iommu.h>
> >   #include <linux/module.h>
> > @@ -29,6 +30,8 @@
> >   
> >   #include <linux/vfio_pci_core.h>
> >   
> > +#include <drm/drm_aperture.h>
> > +
> >   #define DRIVER_AUTHOR   "Alex Williamson <alex.williamson@redhat.com>"
> >   #define DRIVER_DESC "core driver for VFIO based PCI devices"
> >   
> > @@ -1793,6 +1796,20 @@ static int vfio_pci_vga_init(struct vfio_pci_core_device *vdev)
> >   	if (!vfio_pci_is_vga(pdev))
> >   		return 0;
> >   
> > +#if IS_REACHABLE(CONFIG_DRM)
> > +	drm_aperture_detach_platform_drivers(pdev);
> > +#endif
> > +
> > +#if IS_REACHABLE(CONFIG_FB)
> > +	ret = remove_conflicting_pci_framebuffers(pdev, vdev->vdev.ops->name);
> > +	if (ret)
> > +		return ret;
> > +#endif
> > +
> > +	ret = vga_remove_vgacon(pdev);
> > +	if (ret)
> > +		return ret;
> > +  
> 
> You shouldn't have to copy any of the implementation of the aperture 
> helpers.
> 
> If you call drm_aperture_remove_conflicting_pci_framebuffers() it should 
> work correctly. The only reason why it requires a DRM driver structure 
> as second argument is for the driver's name. [1] And that name is only 
> used for printing an info message. [2]

vfio-pci is not dependent on CONFIG_DRM, therefore we need to open code
this regardless.  The only difference if we were to use the existing
function would be something like:

#if IS_REACHABLE(CONFIG_DRM)
	ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &dummy_drm_driver);
	if (ret)
		return ret;
#else
#if IS_REACHABLE(CONFIG_FB)
	ret = remove_conflicting_pci_framebuffers(pdev, vdev->vdev.ops->name);
	if (ret)
		return ret;
#endif
	ret = vga_remove_vgacon(pdev);
	if (ret)
		return ret;
#endif

It's also bad practice to create a dummy DRM driver struct with some
assumption which fields are used.  If the usage is later expanded, we'd
only discover it by users getting segfaults.  If DRM wanted to make
such an API guarantee, then we shouldn't have commit 97c9bfe3f660
("drm/aperture: Pass DRM driver structure instead of driver name").

> The plan forward would be to drop patch 1 entirely.
> 
> For patch 2, the most trivial workaround is to instanciate struct 
> drm_driver here and set the name field to 'vdev->vdev.ops->name'. In the 
> longer term, the aperture helpers will be moved out of DRM and into a 
> more prominent location. That workaround will be cleaned up then.

Trivial in execution, but as above, this is poor practice and should be
avoided.

> Alternatively, drm_aperture_remove_conflicting_pci_framebuffers() could 
> be changed to accept the name string as second argument, but that's 
> quite a bit of churn within the DRM code.

The series as presented was exactly meant to provide the most correct
solution with the least churn to the DRM code.  The above referenced
commit precludes us from calling the existing DRM function directly
without resorting to poor practices of assuming the usage of the DRM
driver struct.  Using the existing DRM function also does not prevent
us from open coding the remainder of the function to avoid a CONFIG_DRM
dependency.  Thanks,

Alex