Re: [PATCH v5] drm/virtio: Add option to disable KMS support

2023-03-02 Thread Rob Clark
On Wed, Mar 1, 2023 at 11:25 PM Gerd Hoffmann  wrote:
>
> On Thu, Mar 02, 2023 at 12:39:33AM +0300, Dmitry Osipenko wrote:
> > On 3/1/23 21:54, Rob Clark wrote:
> > >  /* virtgpu_display.c */
> > > +#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
> > >  int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
> > >  void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
> > > +#else
> > > +static inline int virtio_gpu_modeset_init(struct virtio_gpu_device 
> > > *vgdev)
> > > +{
> > > +   return 0;
> > > +}
> > > +static inline void virtio_gpu_modeset_fini(struct virtio_gpu_device 
> > > *vgdev)
> > > +{
> > > +}
> > > +#endif
> >
> > In v4 Gerd wanted to keep building the virtgpu_display.o and instead add
> > the KSM config check to virtio_gpu_modeset_init/fini().
>
> The main point is that the code workflow should be the same in both
> cases.  The patch does that for virtio_gpu_modeset_init() but doesn't
> for virtio_gpu_modeset_fini().
>
> Return early in the functions (and drop the #ifdef here) is how I would
> implement this, but I wouldn't insist on that, there are other ways to
> solve this too ;)

Ahh, true, I guess omitting that one file doesn't save anything and
early return makes for a bit simpler/smaller patch

BR,
-R


Re: [PATCH v5] drm/virtio: Add option to disable KMS support

2023-03-01 Thread Gerd Hoffmann
On Thu, Mar 02, 2023 at 12:39:33AM +0300, Dmitry Osipenko wrote:
> On 3/1/23 21:54, Rob Clark wrote:
> >  /* virtgpu_display.c */
> > +#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
> >  int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
> >  void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
> > +#else
> > +static inline int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
> > +{
> > +   return 0;
> > +}
> > +static inline void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev)
> > +{
> > +}
> > +#endif
> 
> In v4 Gerd wanted to keep building the virtgpu_display.o and instead add
> the KSM config check to virtio_gpu_modeset_init/fini().

The main point is that the code workflow should be the same in both
cases.  The patch does that for virtio_gpu_modeset_init() but doesn't
for virtio_gpu_modeset_fini().

Return early in the functions (and drop the #ifdef here) is how I would
implement this, but I wouldn't insist on that, there are other ways to
solve this too ;)

take care,
  Gerd



Re: [PATCH v5] drm/virtio: Add option to disable KMS support

2023-03-01 Thread Gerd Hoffmann
  Hi,

> + if (vgdev->num_scanouts) {
> + ret = virtio_gpu_modeset_init(vgdev);

The call to virtio_gpu_modeset_fini() in the unregister
code path needs this too.

Otherwise the patch looks good to me, so with that fixed:
Acked-by: Gerd Hoffmann 

take care,
  Gerd



Re: [PATCH v5] drm/virtio: Add option to disable KMS support

2023-03-01 Thread Dmitry Osipenko
On 3/1/23 21:54, Rob Clark wrote:
>  /* virtgpu_display.c */
> +#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
>  int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
>  void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
> +#else
> +static inline int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
> +{
> + return 0;
> +}
> +static inline void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev)
> +{
> +}
> +#endif

In v4 Gerd wanted to keep building the virtgpu_display.o and instead add
the KSM config check to virtio_gpu_modeset_init/fini(). Is there any
reason why you didn't do that?

-- 
Best regards,
Dmitry



[PATCH v5] drm/virtio: Add option to disable KMS support

2023-03-01 Thread Rob Clark
From: Rob Clark 

Add a build option to disable modesetting support.  This is useful in
cases where the guest only needs to use the GPU in a headless mode, or
(such as in the CrOS usage) window surfaces are proxied to a host
compositor.

As the modesetting ioctls are a big surface area for potential security
bugs to be found (it's happened in the past, we should assume it will
again in the future), it makes sense to have a build option to disable
those ioctls in cases where they serve no legitimate purpose.

v2: Use more if (IS_ENABLED(...))
v3: Also permit the host to advertise no scanouts
v4: Spiff out commit msg
v5: Make num_scanouts==0 and DRM_VIRTIO_GPU_KMS=n behave the same

Signed-off-by: Rob Clark 
Reviewed-by: Dmitry Osipenko 
Reviewed-by: Javier Martinez Canillas 
---
 drivers/gpu/drm/virtio/Kconfig   | 11 ++
 drivers/gpu/drm/virtio/Makefile  |  5 -
 drivers/gpu/drm/virtio/virtgpu_drv.c |  4 
 drivers/gpu/drm/virtio/virtgpu_drv.h | 10 +
 drivers/gpu/drm/virtio/virtgpu_kms.c | 33 +---
 5 files changed, 49 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/virtio/Kconfig b/drivers/gpu/drm/virtio/Kconfig
index 51ec7c3240c9..ea06ff2aa4b4 100644
--- a/drivers/gpu/drm/virtio/Kconfig
+++ b/drivers/gpu/drm/virtio/Kconfig
@@ -11,3 +11,14 @@ config DRM_VIRTIO_GPU
   QEMU based VMMs (like KVM or Xen).
 
   If unsure say M.
+
+config DRM_VIRTIO_GPU_KMS
+   bool "Virtio GPU driver modesetting support"
+   depends on DRM_VIRTIO_GPU
+   default y
+   help
+  Enable modesetting support for virtio GPU driver.  This can be
+  disabled in cases where only "headless" usage of the GPU is
+  required.
+
+  If unsure, say Y.
diff --git a/drivers/gpu/drm/virtio/Makefile b/drivers/gpu/drm/virtio/Makefile
index b99fa4a73b68..24c7ebe87032 100644
--- a/drivers/gpu/drm/virtio/Makefile
+++ b/drivers/gpu/drm/virtio/Makefile
@@ -4,8 +4,11 @@
 # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
 
 virtio-gpu-y := virtgpu_drv.o virtgpu_kms.o virtgpu_gem.o virtgpu_vram.o \
-   virtgpu_display.o virtgpu_vq.o \
+   virtgpu_vq.o \
virtgpu_fence.o virtgpu_object.o virtgpu_debugfs.o virtgpu_plane.o \
virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o
 
+virtio-gpu-$(CONFIG_DRM_VIRTIO_GPU_KMS) += \
+   virtgpu_display.o
+
 obj-$(CONFIG_DRM_VIRTIO_GPU) += virtio-gpu.o
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c 
b/drivers/gpu/drm/virtio/virtgpu_drv.c
index ae97b98750b6..add075681e18 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -172,6 +172,10 @@ MODULE_AUTHOR("Alon Levy");
 DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops);
 
 static const struct drm_driver driver = {
+   /*
+* If KMS is disabled DRIVER_MODESET and DRIVER_ATOMIC are masked
+* out via drm_device::driver_features:
+*/
.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_drv.h 
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index af6ffb696086..ffe8faf67247 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -426,8 +426,18 @@ virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device 
*vgdev,
uint32_t x, uint32_t y);
 
 /* virtgpu_display.c */
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
 int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
 void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
+#else
+static inline int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
+{
+   return 0;
+}
+static inline void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev)
+{
+}
+#endif
 
 /* virtgpu_plane.c */
 uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc);
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c 
b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 27b7f14dae89..61a1afe2c8c8 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -223,21 +223,26 @@ int virtio_gpu_init(struct virtio_device *vdev, struct 
drm_device *dev)
num_scanouts, _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;
+
+   if (IS_ENABLED(CONFIG_DRM_VIRTIO_GPU_KMS) || !vgdev->num_scanouts) {
+   DRM_INFO("KMS disabled\n");
+   vgdev->num_scanouts = 0;
+   vgdev->has_edid = false;
+   dev->driver_features &= ~(DRIVER_MODESET | DRIVER_ATOMIC);
+   } else {
+   DRM_INFO("number of scanouts: %d\n", num_scanouts);
}
-