| 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
| ||
|
Message-ID: <9631930e-3826-ded7-1a45-1d0a285c5195@suse.de>
Date: Tue, 28 Feb 2023 12:37:38 +0100
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Javier Martinez Canillas <javierm@...hat.com>,
Gerd Hoffmann <kraxel@...hat.com>,
Rob Clark <robdclark@...il.com>
Cc: Rob Clark <robdclark@...omium.org>,
Dmitry Osipenko <dmitry.osipenko@...labora.com>,
open list <linux-kernel@...r.kernel.org>,
dri-devel@...ts.freedesktop.org,
Gurchetan Singh <gurchetansingh@...omium.org>,
Ryan Neph <ryanneph@...omium.org>,
David Airlie <airlied@...hat.com>,
"open list:VIRTIO GPU DRIVER"
<virtualization@...ts.linux-foundation.org>
Subject: Re: [PATCH] drm/virtio: Add option to disable KMS support
Hi
Am 28.02.23 um 10:19 schrieb Javier Martinez Canillas:
> Gerd Hoffmann <kraxel@...hat.com> writes:
[...]
>>
>> I think it is a bad idea to make that a compile time option, I'd suggest
>> a runtime switch instead, for example a module parameter to ask the
>> driver to ignore any scanouts.
>>
>
> I don't think there's a need for a new module parameter, there's already
> the virtio-gpu 'modeset' module parameter to enable/disable modsetting
> and the global 'nomodeset' kernel cmdline parameter to do it for all DRM
> drivers.
>
> Currently, many drivers just fail to probe when 'nomodeset' is present,
> but others only disable modsetting but keep the rendering part. In fact,
> most DRM only drivers just ignore the 'nomodeset' parameter.
Do you have a list of these drivers? Maybe we need to adjust semantics
slightly. Please see my comment below.
> We could make virtio-gpu driver to only disable KMS with these params,
> something like the following (untested) patch:
>
> From 9cddee7f696f37c34d80d6160e87827f3d7a0237 Mon Sep 17 00:00:00 2001
> From: Javier Martinez Canillas <javierm@...hat.com>
> Date: Tue, 28 Feb 2023 10:09:11 +0100
> Subject: [PATCH] drm/virtio: Only disable KMS with nomodeset
>
> The virtio-gpu driver currently fails to probe if either the "nomodeset"
> kernel cmdline parameter is used or the module "modeset" parameter used.
>
> But there may be cases where the rendering part of the driver is needed
> and only the mode setting part needs to be disabled. So let's change the
> logic to only disable the KMS part but still keep the DRM side of it.
>
> Signed-off-by: Javier Martinez Canillas <javierm@...hat.com>
> ---
> drivers/gpu/drm/virtio/virtgpu_display.c | 16 +++++++++++++++
> drivers/gpu/drm/virtio/virtgpu_drv.c | 23 ++++++++++++++--------
> drivers/gpu/drm/virtio/virtgpu_kms.c | 25 +-----------------------
> 3 files changed, 32 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c
> index 9ea7611a9e0f..e176e5e8c1a0 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_display.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_display.c
> @@ -335,6 +335,22 @@ static const struct drm_mode_config_funcs virtio_gpu_mode_funcs = {
> int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
> {
> int i, ret;
> + u32 num_scanouts;
> +
> + if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_EDID)) {
> + vgdev->has_edid = true;
> + }
> +
> + /* get display info */
> + virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
> + num_scanouts, &num_scanouts);
> + vgdev->num_scanouts = min_t(uint32_t, num_scanouts,
> + VIRTIO_GPU_MAX_SCANOUTS);
> + if (!vgdev->num_scanouts) {
> + DRM_ERROR("num_scanouts is zero\n");
> + return -EINVAL;
> + }
> + DRM_INFO("number of scanouts: %d\n", num_scanouts);
>
> ret = drmm_mode_config_init(vgdev->ddev);
> if (ret)
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index ae97b98750b6..979b5b177f49 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -40,7 +40,7 @@
>
> #include "virtgpu_drv.h"
>
> -static const struct drm_driver driver;
> +static struct drm_driver driver;
>
> static int virtio_gpu_modeset = -1;
>
> @@ -69,13 +69,12 @@ static int virtio_gpu_pci_quirk(struct drm_device *dev)
> static int virtio_gpu_probe(struct virtio_device *vdev)
> {
> struct drm_device *dev;
> + struct virtio_gpu_device *vgdev;
> int ret;
>
> - if (drm_firmware_drivers_only() && virtio_gpu_modeset == -1)
> - return -EINVAL;
> -
> - if (virtio_gpu_modeset == 0)
> - return -EINVAL;
> + if ((drm_firmware_drivers_only() && virtio_gpu_modeset == -1) ||
> + (virtio_gpu_modeset == 0))
> + driver.driver_features &= ~(DRIVER_MODESET | DRIVER_ATOMIC);
The kernel-wide option 'nomodeset' affects system behavior. It's a
misnomer, as it actually means 'don't replace the firmware-provided
framebuffer.' So if you just set these flags here, virtio-gpu would
later remove the firmware driver via aperture helpers. Therefore, if
drm_formware_drivers_only() returns true, we should fail probing with
-ENODEV.
But we could try to repurpose the module's 'modeset' option. It's
already obsoleted by nomodeset anyway. I'd try to make modeset it a
boolean that controls modesetting vs render-only. It will then be about
the driver's feature set, rather than system behavior.
Best regards
Thomas
>
> /*
> * The virtio-gpu device is a virtual device that doesn't have DMA
> @@ -98,11 +97,19 @@ static int virtio_gpu_probe(struct virtio_device *vdev)
> if (ret)
> goto err_free;
>
> + if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> + vgdev = dev->dev_private;
> + ret = virtio_gpu_modeset_init(vgdev);
> + if (ret)
> + goto err_deinit;
> + }
> +
> ret = drm_dev_register(dev, 0);
> if (ret)
> goto err_deinit;
>
> - drm_fbdev_generic_setup(vdev->priv, 32);
> + if (drm_core_check_feature(dev, DRIVER_MODESET))
> + drm_fbdev_generic_setup(vdev->priv, 32);
> return 0;
>
> err_deinit:
> @@ -171,7 +178,7 @@ MODULE_AUTHOR("Alon Levy");
>
> DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops);
>
> -static const struct drm_driver driver = {
> +static struct drm_driver driver = {
> .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC,
> .open = virtio_gpu_driver_open,
> .postclose = virtio_gpu_driver_postclose,
> diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
> index 27b7f14dae89..2f5f2aac6b71 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
> @@ -122,7 +122,7 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
> struct virtio_gpu_device *vgdev;
> /* this will expand later */
> struct virtqueue *vqs[2];
> - u32 num_scanouts, num_capsets;
> + u32 num_capsets;
> int ret = 0;
>
> if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
> @@ -161,9 +161,6 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
> if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_VIRGL))
> vgdev->has_virgl_3d = true;
> #endif
> - if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_EDID)) {
> - vgdev->has_edid = true;
> - }
> if (virtio_has_feature(vgdev->vdev, VIRTIO_RING_F_INDIRECT_DESC)) {
> vgdev->has_indirect = true;
> }
> @@ -218,28 +215,10 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
> goto err_vbufs;
> }
>
> - /* get display info */
> - virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
> - num_scanouts, &num_scanouts);
> - vgdev->num_scanouts = min_t(uint32_t, num_scanouts,
> - VIRTIO_GPU_MAX_SCANOUTS);
> - if (!vgdev->num_scanouts) {
> - DRM_ERROR("num_scanouts is zero\n");
> - ret = -EINVAL;
> - goto err_scanouts;
> - }
> - DRM_INFO("number of scanouts: %d\n", num_scanouts);
> -
> virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
> num_capsets, &num_capsets);
> DRM_INFO("number of cap sets: %d\n", num_capsets);
>
> - ret = virtio_gpu_modeset_init(vgdev);
> - if (ret) {
> - DRM_ERROR("modeset init failed\n");
> - goto err_scanouts;
> - }
> -
> virtio_device_ready(vgdev->vdev);
>
> if (num_capsets)
> @@ -252,8 +231,6 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
> 5 * HZ);
> return 0;
>
> -err_scanouts:
> - virtio_gpu_free_vbufs(vgdev);
> err_vbufs:
> vgdev->vdev->config->del_vqs(vgdev->vdev);
> err_vqs:
--
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
Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)
Powered by blists - more mailing lists