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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <01c74525-38b7-1e00-51ba-7cd793439f03@suse.de>
Date:   Thu, 9 Jun 2022 11:13:22 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Alex Williamson <alex.williamson@...hat.com>
Cc:     kvm@...r.kernel.org, airlied@...ux.ie,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        Laszlo Ersek <lersek@...hat.com>,
        Gerd Hoffmann <kraxel@...hat.com>
Subject: Re: [PATCH 2/2] vfio/pci: Remove console drivers

Hi

Am 08.06.22 um 16:04 schrieb Alex Williamson:
>>
>> 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").

What you're open coding within vfio is legacy code and we want to remove 
it as much as possible from the aperture helpers.

Tying the helpers to DRM was in mistake in retrospective. We wanted 
something tailored to the needs of DRM. Now that we've seen quite a few 
corner cases in the interaction among graphics subsystems, we need 
something else. The order of creating devices and loading driver modules 
is crucial to making the hand-over between drivers work reliably. So 
far, this luckily has only been a problem in theory, but not practice.

> 
>> 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,

Please have a look at the attached patch. It moves the aperture helpers 
to a location common to the various possible users (DRM, fbdev, vfio). 
The DRM interfaces remain untouched for now.  The patch should provide 
what you need in vfio and also serve our future use cases for graphics 
drivers. If possible, please create your patch on top of it.

Best regards
Thomas

> 
> Alex
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

View attachment "0001-drm-Implement-DRM-aperture-helpers-under-video.patch" of type "text/x-patch" (24431 bytes)

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)