* [PATCH 07/15] drm/tidss: remove dependency on DRM simple helpers
From: Diogo Silva @ 2026-07-18 23:35 UTC (permalink / raw)
To: Jingoo Han, Inki Dae, Seung-Woo Kim, Kyungmin Park, David Airlie,
Simona Vetter, Krzysztof Kozlowski, Peter Griffin, Alim Akhtar,
Laurent Pinchart, Tomi Valkeinen, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, Michal Simek, Thierry Reding,
Mikko Perttunen, Jonathan Hunter, Stefan Agner, Alison Wang,
Anitha Chrisanthus, David Airlie, Gerd Hoffmann, Dmitry Osipenko,
Gurchetan Singh, Chia-I Wu, Jyri Sarha, Liu Ying, Frank Li,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Chun-Kuang Hu, Philipp Zabel, Matthias Brugger,
AngeloGioacchino Del Regno, Geert Uytterhoeven, Xinliang Liu,
Sumit Semwal, Yongqin Liu, John Stultz, Liviu Dudau,
Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl,
Jonathan Corbet, Shuah Khan
Cc: dri-devel, linux-arm-kernel, linux-samsung-soc, linux-kernel,
linux-tegra, virtualization, imx, linux-mediatek,
linux-renesas-soc, linux-amlogic, linux-doc, Diogo Silva
In-Reply-To: <20260719-drm_simple_encoder_init-v1-0-a78c509e3062@gmail.com>
The simple KMS helpers are deprecated because they only add an
intermediate layer between drivers and atomic modesetting.
Open-code drm_simple_encoder_init() by calling drm_encoder_init()
directly and providing driver-local drm_encoder_funcs.
Signed-off-by: Diogo Silva <diogompaissilva@gmail.com>
---
drivers/gpu/drm/tidss/tidss_encoder.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
index 698f8d964ca0..10dbcc6cdf6a 100644
--- a/drivers/gpu/drm/tidss/tidss_encoder.c
+++ b/drivers/gpu/drm/tidss/tidss_encoder.c
@@ -9,11 +9,11 @@
#include <drm/drm_atomic_helper.h>
#include <drm/drm_bridge.h>
#include <drm/drm_bridge_connector.h>
+#include <drm/drm_encoder.h>
#include <drm/drm_crtc.h>
#include <drm/drm_modeset_helper_vtables.h>
#include <drm/drm_panel.h>
#include <drm/drm_of.h>
-#include <drm/drm_simple_kms_helper.h>
#include "tidss_crtc.h"
#include "tidss_drv.h"
@@ -81,6 +81,10 @@ static const struct drm_bridge_funcs tidss_bridge_funcs = {
.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
};
+static const struct drm_encoder_funcs tidss_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
int tidss_encoder_create(struct tidss_device *tidss,
struct drm_bridge *next_bridge,
u32 encoder_type, u32 possible_crtcs)
@@ -95,8 +99,8 @@ int tidss_encoder_create(struct tidss_device *tidss,
if (IS_ERR(t_enc))
return PTR_ERR(t_enc);
- ret = drm_simple_encoder_init(&tidss->ddev, &t_enc->encoder,
- encoder_type);
+ ret = drm_encoder_init(&tidss->ddev, &t_enc->encoder,
+ &tidss_encoder_funcs, encoder_type, NULL);
if (ret)
return ret;
--
2.54.0
^ permalink raw reply related
* [PATCH 06/15] drm/virtio: remove dependency on DRM simple helpers
From: Diogo Silva @ 2026-07-18 23:35 UTC (permalink / raw)
To: Jingoo Han, Inki Dae, Seung-Woo Kim, Kyungmin Park, David Airlie,
Simona Vetter, Krzysztof Kozlowski, Peter Griffin, Alim Akhtar,
Laurent Pinchart, Tomi Valkeinen, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, Michal Simek, Thierry Reding,
Mikko Perttunen, Jonathan Hunter, Stefan Agner, Alison Wang,
Anitha Chrisanthus, David Airlie, Gerd Hoffmann, Dmitry Osipenko,
Gurchetan Singh, Chia-I Wu, Jyri Sarha, Liu Ying, Frank Li,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Chun-Kuang Hu, Philipp Zabel, Matthias Brugger,
AngeloGioacchino Del Regno, Geert Uytterhoeven, Xinliang Liu,
Sumit Semwal, Yongqin Liu, John Stultz, Liviu Dudau,
Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl,
Jonathan Corbet, Shuah Khan
Cc: dri-devel, linux-arm-kernel, linux-samsung-soc, linux-kernel,
linux-tegra, virtualization, imx, linux-mediatek,
linux-renesas-soc, linux-amlogic, linux-doc, Diogo Silva
In-Reply-To: <20260719-drm_simple_encoder_init-v1-0-a78c509e3062@gmail.com>
The simple KMS helpers are deprecated because they only add an
intermediate layer between drivers and atomic modesetting.
Open-code drm_simple_encoder_init() by calling drm_encoder_init()
directly and providing driver-local drm_encoder_funcs.
Also check the return value from drm_encoder_init() to avoid silent
failures.
Signed-off-by: Diogo Silva <diogompaissilva@gmail.com>
---
drivers/gpu/drm/virtio/virtgpu_display.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c
index 44ffffec550f..67023d91d40b 100644
--- a/drivers/gpu/drm/virtio/virtgpu_display.c
+++ b/drivers/gpu/drm/virtio/virtgpu_display.c
@@ -28,11 +28,11 @@
#include <drm/drm_atomic_helper.h>
#include <drm/drm_damage_helper.h>
#include <drm/drm_edid.h>
+#include <drm/drm_encoder.h>
#include <drm/drm_fourcc.h>
#include <drm/drm_gem_framebuffer_helper.h>
#include <drm/drm_print.h>
#include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
#include <drm/drm_vblank.h>
#include <drm/drm_vblank_helper.h>
@@ -232,6 +232,10 @@ static enum drm_mode_status virtio_gpu_conn_mode_valid(struct drm_connector *con
return MODE_BAD;
}
+static const struct drm_encoder_funcs virtio_gpu_enc_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
static const struct drm_encoder_helper_funcs virtio_gpu_enc_helper_funcs = {
.mode_set = virtio_gpu_enc_mode_set,
.enable = virtio_gpu_enc_enable,
@@ -306,7 +310,11 @@ static int vgdev_output_init(struct virtio_gpu_device *vgdev, int index)
if (vgdev->has_edid)
drm_connector_attach_edid_property(connector);
- drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_VIRTUAL);
+ ret = drm_encoder_init(dev, encoder, &virtio_gpu_enc_funcs,
+ DRM_MODE_ENCODER_VIRTUAL, NULL);
+ if (ret)
+ return ret;
+
drm_encoder_helper_add(encoder, &virtio_gpu_enc_helper_funcs);
encoder->possible_crtcs = 1 << index;
--
2.54.0
^ permalink raw reply related
* [PATCH 05/15] drm/kmb: remove dependency on DRM simple helpers
From: Diogo Silva @ 2026-07-18 23:35 UTC (permalink / raw)
To: Jingoo Han, Inki Dae, Seung-Woo Kim, Kyungmin Park, David Airlie,
Simona Vetter, Krzysztof Kozlowski, Peter Griffin, Alim Akhtar,
Laurent Pinchart, Tomi Valkeinen, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, Michal Simek, Thierry Reding,
Mikko Perttunen, Jonathan Hunter, Stefan Agner, Alison Wang,
Anitha Chrisanthus, David Airlie, Gerd Hoffmann, Dmitry Osipenko,
Gurchetan Singh, Chia-I Wu, Jyri Sarha, Liu Ying, Frank Li,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Chun-Kuang Hu, Philipp Zabel, Matthias Brugger,
AngeloGioacchino Del Regno, Geert Uytterhoeven, Xinliang Liu,
Sumit Semwal, Yongqin Liu, John Stultz, Liviu Dudau,
Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl,
Jonathan Corbet, Shuah Khan
Cc: dri-devel, linux-arm-kernel, linux-samsung-soc, linux-kernel,
linux-tegra, virtualization, imx, linux-mediatek,
linux-renesas-soc, linux-amlogic, linux-doc, Diogo Silva
In-Reply-To: <20260719-drm_simple_encoder_init-v1-0-a78c509e3062@gmail.com>
The simple KMS helpers are deprecated because they only add an
intermediate layer between drivers and atomic modesetting.
Open-code drm_simple_encoder_init() by calling drm_encoder_init()
directly and providing driver-local drm_encoder_funcs.
Signed-off-by: Diogo Silva <diogompaissilva@gmail.com>
---
drivers/gpu/drm/kmb/kmb_dsi.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/kmb/kmb_dsi.c b/drivers/gpu/drm/kmb/kmb_dsi.c
index 59d0e856392f..13adba96bc78 100644
--- a/drivers/gpu/drm/kmb/kmb_dsi.c
+++ b/drivers/gpu/drm/kmb/kmb_dsi.c
@@ -14,8 +14,8 @@
#include <drm/drm_atomic_helper.h>
#include <drm/drm_bridge.h>
#include <drm/drm_bridge_connector.h>
+#include <drm/drm_encoder.h>
#include <drm/drm_mipi_dsi.h>
-#include <drm/drm_simple_kms_helper.h>
#include <drm/drm_print.h>
#include <drm/drm_probe_helper.h>
@@ -1427,6 +1427,10 @@ struct kmb_dsi *kmb_dsi_init(struct platform_device *pdev)
return kmb_dsi;
}
+static const struct drm_encoder_funcs kmb_dsi_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
int kmb_dsi_encoder_init(struct drm_device *dev, struct kmb_dsi *kmb_dsi)
{
struct drm_encoder *encoder;
@@ -1437,7 +1441,8 @@ int kmb_dsi_encoder_init(struct drm_device *dev, struct kmb_dsi *kmb_dsi)
encoder->possible_crtcs = 1;
encoder->possible_clones = 0;
- ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DSI);
+ ret = drm_encoder_init(dev, encoder, &kmb_dsi_encoder_funcs,
+ DRM_MODE_ENCODER_DSI, NULL);
if (ret) {
dev_err(kmb_dsi->dev, "Failed to init encoder %d\n", ret);
return ret;
--
2.54.0
^ permalink raw reply related
* [PATCH 04/15] drm/fsl-dcu: remove dependency on DRM simple helpers
From: Diogo Silva @ 2026-07-18 23:35 UTC (permalink / raw)
To: Jingoo Han, Inki Dae, Seung-Woo Kim, Kyungmin Park, David Airlie,
Simona Vetter, Krzysztof Kozlowski, Peter Griffin, Alim Akhtar,
Laurent Pinchart, Tomi Valkeinen, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, Michal Simek, Thierry Reding,
Mikko Perttunen, Jonathan Hunter, Stefan Agner, Alison Wang,
Anitha Chrisanthus, David Airlie, Gerd Hoffmann, Dmitry Osipenko,
Gurchetan Singh, Chia-I Wu, Jyri Sarha, Liu Ying, Frank Li,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Chun-Kuang Hu, Philipp Zabel, Matthias Brugger,
AngeloGioacchino Del Regno, Geert Uytterhoeven, Xinliang Liu,
Sumit Semwal, Yongqin Liu, John Stultz, Liviu Dudau,
Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl,
Jonathan Corbet, Shuah Khan
Cc: dri-devel, linux-arm-kernel, linux-samsung-soc, linux-kernel,
linux-tegra, virtualization, imx, linux-mediatek,
linux-renesas-soc, linux-amlogic, linux-doc, Diogo Silva
In-Reply-To: <20260719-drm_simple_encoder_init-v1-0-a78c509e3062@gmail.com>
The simple KMS helpers are deprecated because they only add an
intermediate layer between drivers and atomic modesetting.
Open-code drm_simple_encoder_init() by calling drm_encoder_init()
directly and providing driver-local drm_encoder_funcs.
Signed-off-by: Diogo Silva <diogompaissilva@gmail.com>
---
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
index 84eff7519e32..c2b788bfa8f9 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c
@@ -11,14 +11,18 @@
#include <drm/drm_atomic_helper.h>
#include <drm/drm_bridge.h>
+#include <drm/drm_encoder.h>
#include <drm/drm_of.h>
#include <drm/drm_panel.h>
#include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
#include "fsl_dcu_drm_drv.h"
#include "fsl_tcon.h"
+static const struct drm_encoder_funcs fsl_dcu_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
int fsl_dcu_drm_encoder_create(struct fsl_dcu_drm_device *fsl_dev,
struct drm_crtc *crtc)
{
@@ -31,8 +35,8 @@ int fsl_dcu_drm_encoder_create(struct fsl_dcu_drm_device *fsl_dev,
if (fsl_dev->tcon)
fsl_tcon_bypass_enable(fsl_dev->tcon);
- ret = drm_simple_encoder_init(fsl_dev->drm, encoder,
- DRM_MODE_ENCODER_LVDS);
+ ret = drm_encoder_init(fsl_dev->drm, encoder, &fsl_dcu_encoder_funcs,
+ DRM_MODE_ENCODER_LVDS, NULL);
if (ret < 0)
return ret;
--
2.54.0
^ permalink raw reply related
* [PATCH 03/15] drm/tegra: remove dependency on DRM simple helpers
From: Diogo Silva @ 2026-07-18 23:35 UTC (permalink / raw)
To: Jingoo Han, Inki Dae, Seung-Woo Kim, Kyungmin Park, David Airlie,
Simona Vetter, Krzysztof Kozlowski, Peter Griffin, Alim Akhtar,
Laurent Pinchart, Tomi Valkeinen, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, Michal Simek, Thierry Reding,
Mikko Perttunen, Jonathan Hunter, Stefan Agner, Alison Wang,
Anitha Chrisanthus, David Airlie, Gerd Hoffmann, Dmitry Osipenko,
Gurchetan Singh, Chia-I Wu, Jyri Sarha, Liu Ying, Frank Li,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Chun-Kuang Hu, Philipp Zabel, Matthias Brugger,
AngeloGioacchino Del Regno, Geert Uytterhoeven, Xinliang Liu,
Sumit Semwal, Yongqin Liu, John Stultz, Liviu Dudau,
Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl,
Jonathan Corbet, Shuah Khan
Cc: dri-devel, linux-arm-kernel, linux-samsung-soc, linux-kernel,
linux-tegra, virtualization, imx, linux-mediatek,
linux-renesas-soc, linux-amlogic, linux-doc, Diogo Silva
In-Reply-To: <20260719-drm_simple_encoder_init-v1-0-a78c509e3062@gmail.com>
The simple KMS helpers are deprecated because they only add an
intermediate layer between drivers and atomic modesetting.
Open-code drm_simple_encoder_init() by calling drm_encoder_init()
directly and providing driver-local drm_encoder_funcs.
Also check the return value from drm_encoder_init() to avoid silent
failures.
Signed-off-by: Diogo Silva <diogompaissilva@gmail.com>
---
drivers/gpu/drm/tegra/dsi.c | 17 ++++++++++++++---
drivers/gpu/drm/tegra/rgb.c | 14 ++++++++++++--
2 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index e7fdd8c7ac12..840e118716b2 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -20,11 +20,11 @@
#include <drm/drm_atomic_helper.h>
#include <drm/drm_debugfs.h>
+#include <drm/drm_encoder.h>
#include <drm/drm_file.h>
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_panel.h>
#include <drm/drm_print.h>
-#include <drm/drm_simple_kms_helper.h>
#include "dc.h"
#include "drm.h"
@@ -1055,6 +1055,10 @@ tegra_dsi_encoder_atomic_check(struct drm_encoder *encoder,
return err;
}
+static const struct drm_encoder_funcs tegra_dsi_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
static const struct drm_encoder_helper_funcs tegra_dsi_encoder_helper_funcs = {
.disable = tegra_dsi_encoder_disable,
.enable = tegra_dsi_encoder_enable,
@@ -1078,8 +1082,15 @@ static int tegra_dsi_init(struct host1x_client *client)
&tegra_dsi_connector_helper_funcs);
dsi->output.connector.dpms = DRM_MODE_DPMS_OFF;
- drm_simple_encoder_init(drm, &dsi->output.encoder,
- DRM_MODE_ENCODER_DSI);
+ err = drm_encoder_init(drm, &dsi->output.encoder,
+ &tegra_dsi_encoder_funcs,
+ DRM_MODE_ENCODER_DSI, NULL);
+ if (err) {
+ dev_err(dsi->dev, "failed to initialize encoder: %d\n",
+ err);
+ return err;
+ }
+
drm_encoder_helper_add(&dsi->output.encoder,
&tegra_dsi_encoder_helper_funcs);
diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
index e67fbb2362e6..337925c30d67 100644
--- a/drivers/gpu/drm/tegra/rgb.c
+++ b/drivers/gpu/drm/tegra/rgb.c
@@ -9,7 +9,7 @@
#include <drm/drm_atomic_helper.h>
#include <drm/drm_bridge_connector.h>
-#include <drm/drm_simple_kms_helper.h>
+#include <drm/drm_encoder.h>
#include "drm.h"
#include "dc.h"
@@ -194,6 +194,10 @@ tegra_rgb_encoder_atomic_check(struct drm_encoder *encoder,
return err;
}
+static const struct drm_encoder_funcs tegra_rgb_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
static const struct drm_encoder_helper_funcs tegra_rgb_encoder_helper_funcs = {
.disable = tegra_rgb_encoder_disable,
.enable = tegra_rgb_encoder_enable,
@@ -305,7 +309,13 @@ int tegra_dc_rgb_init(struct drm_device *drm, struct tegra_dc *dc)
if (!dc->rgb)
return -ENODEV;
- drm_simple_encoder_init(drm, &output->encoder, DRM_MODE_ENCODER_LVDS);
+ err = drm_encoder_init(drm, &output->encoder, &tegra_rgb_encoder_funcs,
+ DRM_MODE_ENCODER_LVDS, NULL);
+ if (err) {
+ dev_err(output->dev, "failed to initialize encoder: %d\n", err);
+ return err;
+ }
+
drm_encoder_helper_add(&output->encoder,
&tegra_rgb_encoder_helper_funcs);
--
2.54.0
^ permalink raw reply related
* [PATCH 02/15] drm/xlnx/zynqmp_dpsub: remove dependency on DRM simple helpers
From: Diogo Silva @ 2026-07-18 23:35 UTC (permalink / raw)
To: Jingoo Han, Inki Dae, Seung-Woo Kim, Kyungmin Park, David Airlie,
Simona Vetter, Krzysztof Kozlowski, Peter Griffin, Alim Akhtar,
Laurent Pinchart, Tomi Valkeinen, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, Michal Simek, Thierry Reding,
Mikko Perttunen, Jonathan Hunter, Stefan Agner, Alison Wang,
Anitha Chrisanthus, David Airlie, Gerd Hoffmann, Dmitry Osipenko,
Gurchetan Singh, Chia-I Wu, Jyri Sarha, Liu Ying, Frank Li,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Chun-Kuang Hu, Philipp Zabel, Matthias Brugger,
AngeloGioacchino Del Regno, Geert Uytterhoeven, Xinliang Liu,
Sumit Semwal, Yongqin Liu, John Stultz, Liviu Dudau,
Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl,
Jonathan Corbet, Shuah Khan
Cc: dri-devel, linux-arm-kernel, linux-samsung-soc, linux-kernel,
linux-tegra, virtualization, imx, linux-mediatek,
linux-renesas-soc, linux-amlogic, linux-doc, Diogo Silva
In-Reply-To: <20260719-drm_simple_encoder_init-v1-0-a78c509e3062@gmail.com>
The simple KMS helpers are deprecated because they only add an
intermediate layer between drivers and atomic modesetting.
Open-code drm_simple_encoder_init() by calling drm_encoder_init()
directly and providing driver-local drm_encoder_funcs.
Also check the return value from drm_encoder_init() to avoid silent
failures.
Signed-off-by: Diogo Silva <diogompaissilva@gmail.com>
---
drivers/gpu/drm/xlnx/zynqmp_kms.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c b/drivers/gpu/drm/xlnx/zynqmp_kms.c
index d5f922450565..1d194c2824e3 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
@@ -30,7 +30,6 @@
#include <drm/drm_mode_config.h>
#include <drm/drm_plane.h>
#include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
#include <drm/drm_vblank.h>
#include <linux/clk.h>
@@ -417,6 +416,10 @@ static const struct drm_driver zynqmp_dpsub_drm_driver = {
.minor = 0,
};
+static const struct drm_encoder_funcs zynqmp_dpsub_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
static int zynqmp_dpsub_kms_init(struct zynqmp_dpsub *dpsub)
{
struct drm_encoder *encoder = &dpsub->drm->encoder;
@@ -436,7 +439,13 @@ static int zynqmp_dpsub_kms_init(struct zynqmp_dpsub *dpsub)
/* Create the encoder and attach the bridge. */
encoder->possible_crtcs |= drm_crtc_mask(&dpsub->drm->crtc);
- drm_simple_encoder_init(&dpsub->drm->dev, encoder, DRM_MODE_ENCODER_NONE);
+ ret = drm_encoder_init(&dpsub->drm->dev, encoder,
+ &zynqmp_dpsub_encoder_funcs,
+ DRM_MODE_ENCODER_NONE, NULL);
+ if (ret) {
+ dev_err(dpsub->dev, "failed to initialize encoder\n");
+ return ret;
+ }
ret = drm_bridge_attach(encoder, dpsub->bridge, NULL,
DRM_BRIDGE_ATTACH_NO_CONNECTOR);
--
2.54.0
^ permalink raw reply related
* [PATCH 01/15] drm/exynos: remove dependency on DRM simple helpers
From: Diogo Silva @ 2026-07-18 23:35 UTC (permalink / raw)
To: Jingoo Han, Inki Dae, Seung-Woo Kim, Kyungmin Park, David Airlie,
Simona Vetter, Krzysztof Kozlowski, Peter Griffin, Alim Akhtar,
Laurent Pinchart, Tomi Valkeinen, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, Michal Simek, Thierry Reding,
Mikko Perttunen, Jonathan Hunter, Stefan Agner, Alison Wang,
Anitha Chrisanthus, David Airlie, Gerd Hoffmann, Dmitry Osipenko,
Gurchetan Singh, Chia-I Wu, Jyri Sarha, Liu Ying, Frank Li,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Chun-Kuang Hu, Philipp Zabel, Matthias Brugger,
AngeloGioacchino Del Regno, Geert Uytterhoeven, Xinliang Liu,
Sumit Semwal, Yongqin Liu, John Stultz, Liviu Dudau,
Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl,
Jonathan Corbet, Shuah Khan
Cc: dri-devel, linux-arm-kernel, linux-samsung-soc, linux-kernel,
linux-tegra, virtualization, imx, linux-mediatek,
linux-renesas-soc, linux-amlogic, linux-doc, Diogo Silva
In-Reply-To: <20260719-drm_simple_encoder_init-v1-0-a78c509e3062@gmail.com>
The simple KMS helpers are deprecated because they only add an
intermediate layer between drivers and atomic modesetting.
Open-code drm_simple_encoder_init() by calling drm_encoder_init()
directly and providing driver-local drm_encoder_funcs.
Also check the return value from drm_encoder_init() to avoid silent
failures.
Signed-off-by: Diogo Silva <diogompaissilva@gmail.com>
---
drivers/gpu/drm/exynos/exynos_dp.c | 13 +++++++++++--
drivers/gpu/drm/exynos/exynos_drm_dpi.c | 14 ++++++++++++--
drivers/gpu/drm/exynos/exynos_drm_dsi.c | 11 +++++++++--
drivers/gpu/drm/exynos/exynos_drm_vidi.c | 14 ++++++++++++--
drivers/gpu/drm/exynos/exynos_hdmi.c | 15 +++++++++++++--
5 files changed, 57 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
index b80540328150..1598892c602b 100644
--- a/drivers/gpu/drm/exynos/exynos_dp.c
+++ b/drivers/gpu/drm/exynos/exynos_dp.c
@@ -24,11 +24,11 @@
#include <drm/drm_bridge.h>
#include <drm/drm_bridge_connector.h>
#include <drm/drm_crtc.h>
+#include <drm/drm_encoder.h>
#include <drm/drm_of.h>
#include <drm/drm_panel.h>
#include <drm/drm_print.h>
#include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
#include <drm/exynos_drm.h>
#include "exynos_drm_crtc.h"
@@ -79,6 +79,10 @@ static void exynos_dp_nop(struct drm_encoder *encoder)
/* do nothing */
}
+static const struct drm_encoder_funcs exynos_dp_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
static const struct drm_encoder_helper_funcs exynos_dp_encoder_helper_funcs = {
.mode_set = exynos_dp_mode_set,
.enable = exynos_dp_nop,
@@ -95,7 +99,12 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data)
dp->drm_dev = drm_dev;
- drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_TMDS);
+ ret = drm_encoder_init(drm_dev, encoder, &exynos_dp_encoder_funcs,
+ DRM_MODE_ENCODER_TMDS, NULL);
+ if (ret) {
+ dev_err(dp->dev, "Failed to initialize encoder\n");
+ return ret;
+ }
drm_encoder_helper_add(encoder, &exynos_dp_encoder_helper_funcs);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dpi.c b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
index 0dc36df6ada3..4e42a1da81d1 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dpi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
@@ -12,10 +12,10 @@
#include <linux/regulator/consumer.h>
#include <drm/drm_atomic_helper.h>
+#include <drm/drm_encoder.h>
#include <drm/drm_panel.h>
#include <drm/drm_print.h>
#include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
#include <video/of_videomode.h>
#include <video/videomode.h>
@@ -140,6 +140,10 @@ static void exynos_dpi_disable(struct drm_encoder *encoder)
}
}
+static const struct drm_encoder_funcs exynos_dpi_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
static const struct drm_encoder_helper_funcs exynos_dpi_encoder_helper_funcs = {
.mode_set = exynos_dpi_mode_set,
.enable = exynos_dpi_enable,
@@ -194,7 +198,13 @@ int exynos_dpi_bind(struct drm_device *dev, struct drm_encoder *encoder)
{
int ret;
- drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_TMDS);
+ ret = drm_encoder_init(dev, encoder, &exynos_dpi_encoder_funcs,
+ DRM_MODE_ENCODER_TMDS, NULL);
+ if (ret) {
+ DRM_DEV_ERROR(encoder_to_dpi(encoder)->dev,
+ "failed to create encoder ret = %d\n", ret);
+ return ret;
+ }
drm_encoder_helper_add(encoder, &exynos_dpi_encoder_helper_funcs);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index c4d098ab7863..6b7561ac9bb0 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -13,7 +13,7 @@
#include <drm/bridge/samsung-dsim.h>
#include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
+#include <drm/drm_encoder.h>
#include "exynos_drm_crtc.h"
#include "exynos_drm_drv.h"
@@ -22,6 +22,10 @@ struct exynos_dsi {
struct drm_encoder encoder;
};
+static const struct drm_encoder_funcs exynos_drm_dsi_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
static irqreturn_t exynos_dsi_te_irq_handler(struct samsung_dsim *dsim)
{
struct exynos_dsi *dsi = dsim->priv;
@@ -79,7 +83,10 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, void *data
struct drm_device *drm_dev = data;
int ret;
- drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_TMDS);
+ ret = drm_encoder_init(drm_dev, encoder, &exynos_drm_dsi_encoder_funcs,
+ DRM_MODE_ENCODER_TMDS, NULL);
+ if (ret)
+ return ret;
ret = exynos_drm_set_possible_crtcs(encoder, EXYNOS_DISPLAY_TYPE_LCD);
if (ret < 0)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index 67bbf9b8bc0e..59dea853d364 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -13,10 +13,10 @@
#include <drm/drm_atomic_helper.h>
#include <drm/drm_edid.h>
+#include <drm/drm_encoder.h>
#include <drm/drm_framebuffer.h>
#include <drm/drm_print.h>
#include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
#include <drm/drm_vblank.h>
#include <drm/exynos_drm.h>
@@ -403,6 +403,10 @@ static void exynos_vidi_disable(struct drm_encoder *encoder)
{
}
+static const struct drm_encoder_funcs exynos_vidi_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
static const struct drm_encoder_helper_funcs exynos_vidi_encoder_helper_funcs = {
.mode_set = exynos_vidi_mode_set,
.enable = exynos_vidi_enable,
@@ -445,7 +449,13 @@ static int vidi_bind(struct device *dev, struct device *master, void *data)
return PTR_ERR(ctx->crtc);
}
- drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_TMDS);
+ ret = drm_encoder_init(drm_dev, encoder, &exynos_vidi_encoder_funcs,
+ DRM_MODE_ENCODER_TMDS, NULL);
+ if (ret) {
+ DRM_DEV_ERROR(dev, "failed to initialize encoder ret = %d\n",
+ ret);
+ return ret;
+ }
drm_encoder_helper_add(encoder, &exynos_vidi_encoder_helper_funcs);
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 09b2cabb236f..f44586ce0fdf 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -36,9 +36,9 @@
#include <drm/drm_atomic_helper.h>
#include <drm/drm_bridge.h>
#include <drm/drm_edid.h>
+#include <drm/drm_encoder.h>
#include <drm/drm_print.h>
#include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
#include "exynos_drm_crtc.h"
#include "regs-hdmi.h"
@@ -1575,6 +1575,11 @@ static void hdmi_disable(struct drm_encoder *encoder)
mutex_unlock(&hdata->mutex);
}
+static const struct drm_encoder_funcs exynos_hdmi_encoder_funcs = {
+ .destroy = drm_encoder_cleanup,
+};
+
+
static const struct drm_encoder_helper_funcs exynos_hdmi_encoder_helper_funcs = {
.mode_fixup = hdmi_mode_fixup,
.enable = hdmi_enable,
@@ -1862,7 +1867,13 @@ static int hdmi_bind(struct device *dev, struct device *master, void *data)
hdata->phy_clk.enable = hdmiphy_clk_enable;
- drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_TMDS);
+ ret = drm_encoder_init(drm_dev, encoder, &exynos_hdmi_encoder_funcs,
+ DRM_MODE_ENCODER_TMDS, NULL);
+ if (ret) {
+ DRM_DEV_ERROR(dev, "failed to initialize encoder ret = %d\n",
+ ret);
+ return ret;
+ }
drm_encoder_helper_add(encoder, &exynos_hdmi_encoder_helper_funcs);
--
2.54.0
^ permalink raw reply related
* [PATCH 00/15] drm/drm_simple: remove drm_simple_encoder_init
From: Diogo Silva @ 2026-07-18 23:35 UTC (permalink / raw)
To: Jingoo Han, Inki Dae, Seung-Woo Kim, Kyungmin Park, David Airlie,
Simona Vetter, Krzysztof Kozlowski, Peter Griffin, Alim Akhtar,
Laurent Pinchart, Tomi Valkeinen, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, Michal Simek, Thierry Reding,
Mikko Perttunen, Jonathan Hunter, Stefan Agner, Alison Wang,
Anitha Chrisanthus, David Airlie, Gerd Hoffmann, Dmitry Osipenko,
Gurchetan Singh, Chia-I Wu, Jyri Sarha, Liu Ying, Frank Li,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Chun-Kuang Hu, Philipp Zabel, Matthias Brugger,
AngeloGioacchino Del Regno, Geert Uytterhoeven, Xinliang Liu,
Sumit Semwal, Yongqin Liu, John Stultz, Liviu Dudau,
Neil Armstrong, Kevin Hilman, Jerome Brunet, Martin Blumenstingl,
Jonathan Corbet, Shuah Khan
Cc: dri-devel, linux-arm-kernel, linux-samsung-soc, linux-kernel,
linux-tegra, virtualization, imx, linux-mediatek,
linux-renesas-soc, linux-amlogic, linux-doc, Diogo Silva
The simple KMS helpers are deprecated because they only add an
intermediate layer between drivers and atomic modesetting.
This series open-codes all remaining drm_simple_encoder_init() users by
calling drm_encoder_init() directly and providing driver-local
drm_encoder_funcs where needed. After the driver conversions, the helper
is removed and the completed DRM todo item is dropped.
Signed-off-by: Diogo Silva <diogompaissilva@gmail.com>
---
Diogo Silva (15):
drm/exynos: remove dependency on DRM simple helpers
drm/xlnx/zynqmp_dpsub: remove dependency on DRM simple helpers
drm/tegra: remove dependency on DRM simple helpers
drm/fsl-dcu: remove dependency on DRM simple helpers
drm/kmb: remove dependency on DRM simple helpers
drm/virtio: remove dependency on DRM simple helpers
drm/tidss: remove dependency on DRM simple helpers
drm/imx: remove dependency on DRM simple helpers
drm/mediatek: remove dependency on DRM simple helpers
drm/renesas/shmobile: remove dependency on DRM simple helpers
drm/hisilicon/kirin: remove dependency on DRM simple helpers
drm/arm/komeda: remove dependency on DRM simple helpers
drm/meson: remove dependency on DRM simple helpers
drm/drm_simple: remove deprecated drm_simple_encoder_init function
Documentation/gpu: remove completed drm_simple_encoder_init() todo
Documentation/gpu/todo.rst | 15 ---------------
drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 9 +++++++--
drivers/gpu/drm/drm_simple_kms_helper.c | 13 ++-----------
drivers/gpu/drm/exynos/exynos_dp.c | 13 +++++++++++--
drivers/gpu/drm/exynos/exynos_drm_dpi.c | 14 ++++++++++++--
drivers/gpu/drm/exynos/exynos_drm_dsi.c | 11 +++++++++--
drivers/gpu/drm/exynos/exynos_drm_vidi.c | 14 ++++++++++++--
drivers/gpu/drm/exynos/exynos_hdmi.c | 15 +++++++++++++--
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c | 10 +++++++---
drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 9 +++++++--
drivers/gpu/drm/imx/dc/dc-kms.c | 8 ++++++--
drivers/gpu/drm/kmb/kmb_dsi.c | 9 +++++++--
drivers/gpu/drm/mediatek/mtk_dsi.c | 10 +++++++---
drivers/gpu/drm/meson/meson_encoder_cvbs.c | 11 ++++++++---
drivers/gpu/drm/meson/meson_encoder_dsi.c | 11 ++++++++---
drivers/gpu/drm/meson/meson_encoder_hdmi.c | 11 ++++++++---
drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 10 +++++++---
drivers/gpu/drm/tegra/dsi.c | 17 ++++++++++++++---
drivers/gpu/drm/tegra/rgb.c | 14 ++++++++++++--
drivers/gpu/drm/tidss/tidss_encoder.c | 10 +++++++---
drivers/gpu/drm/virtio/virtgpu_display.c | 12 ++++++++++--
drivers/gpu/drm/xlnx/zynqmp_kms.c | 13 +++++++++++--
include/drm/drm_simple_kms_helper.h | 4 ----
23 files changed, 185 insertions(+), 78 deletions(-)
---
base-commit: e55c6b9a3522aaf1441a0662d534c1c7bfa9b860
change-id: 20260718-drm_simple_encoder_init-d069a4cc7f8b
Best regards,
--
Diogo Silva <diogompaissilva@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 1/4] virtio-mem: validate device-reported block size
From: Carlos Bilbao @ 2026-07-18 17:41 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Greg Kroah-Hartman, David Hildenbrand (Arm), Hari Mishal,
Jason Wang, Xuan Zhuo, Eugenio Pérez, virtualization,
linux-kernel, elena.reshetova, huster, mhollick, jiska.classen
In-Reply-To: <20260718131715-mutt-send-email-mst@kernel.org>
Hello Michael,
On 7/18/26 10:21, Michael S. Tsirkin wrote:
> On Sat, Jul 18, 2026 at 10:07:30AM -0700, Carlos Bilbao wrote:
>> On 7/17/26 22:29, Greg Kroah-Hartman wrote:
>>
>>> On Fri, Jul 17, 2026 at 08:31:09PM -0700, Carlos Bilbao wrote:
>>>> Historically, one of the biggest criticisms of coco, especially around
>>>> device hardening, was that there were too many values that a
>>>> malicious/buggy device could misreport, making it a losing battle. That is
>>>> no longer the case with LLMs, and we have the advantage (and challenge) of
>>>> open-source dev, which allows us to receive many of these fixes "for free".
>>>> If others want to burn their tokens, let them :)
>>> I have lots of tokens to burn :)
>>>
>>> So along those lines, any suggestions on how best to fuzz these code
>>> paths? Any workloads you all use for testing that I can take advantage
>>> of?
>>
>> We've the virtio-mem config struct layout and the kernel source, so for
>> obvious fixes like a NULL check, static analysis is better than fuzzing.
>> Claude took a few mins to find me two examples:
>>
>> Patch 1: virtio-mem: reject non-power-of-two device_block_size
>> This one is for virtio_mem_init() to check if
>> !is_power_of_2(vm->device_block_size)
>>
>> Patch 2: virto-mem: validate region_size and usable_region_size
>> THis one checks region_size != 0 and vm->usable_reion_size >
>> vm->region_size.
>>
>> An endless factory of "silly" checks like these are low hanging fruit.
> At the same time, these checks don't actually help within the coco
> threat model, do they?
>
>> Now, for harder bugs, looking around for fuzz options, VirtFuzz [1] looks
>> like a great candidate for those interested in pursuing this direction.
>>
>>
>> Their PoC fuzzes wireless/Bluetooth stack, but nothing our AI overlords
>> can't quickly adapt for virtio-mem and other virtio drivers; the JSON
>> definition to describe device behavior is easily extensible. Their threat
>> model [2] describes an external attacker, but in the context of coco, the
>> virtio device itself is the attacker.
> What we need, however, is to exclude DoS attacks - these are outside the
> threat model. If people try to address all DoS attacks uncritically we
> just get a churn of changes which just might introduce issues of their
> own.
>
> Example:
>
> BUG_ON(!is_power_of_2(....));
> panics, non exploitable.
>
> if(!is_power_of_2(....))
> goto error;
>
> can become exploitable if the cleanup is done wrong.
Yes, you are 100% technically right about the scope of the threat model.
DoS is out of scope because it is a fundamentally unreachable goal; the
cloud provider can always just "pull the plug". The dangers of
"vibe-coding" you point out are real, over-eager LLMs fixing up and down
will create new vulnerabilities in complex cleanup paths. Also, TBH, I
sympathize with a maintainer's disinterest in reviewing a million stupid
checks.
But, to play devil's advocate: this assumes a missing check
like is_power_of_2 only ever leads to a benign crash, rather than already
cascading into an unknown, exploitable state down the line.
So these checks are not _just_ to prevent DoS!
Anyhow, this is the exact justification for VirtFuzz. If your main concern
is that adding validation checks might introduce subtle exploit paths in
the error-cleanup code, VirtFuzz and tools like that, can fuzz those new
paths exhaustively. It gives the automated safety net needed to scale coco
device with as little regressions as possible.
>
>
>
>> Here's a vibe coded PR of what I mean:
>>
>> https://github.com/seemoo-lab/VirtFuzz/pull/7
>>
>> CCed the creators/authors, thanks for open sourcing this!
>>
>> Thanks,
>> Carlos
>>
>> [1] https://github.com/seemoo-lab/VirtFuzz
>>
>> On 7/17/26 22:29, Greg Kroah-Hartman wrote:
>>
>>> On Fri, Jul 17, 2026 at 08:31:09PM -0700, Carlos Bilbao wrote:
>>>> Historically, one of the biggest criticisms of coco, especially around
>>>> device hardening, was that there were too many values that a
>>>> malicious/buggy device could misreport, making it a losing battle. That is
>>>> no longer the case with LLMs, and we have the advantage (and challenge) of
>>>> open-source dev, which allows us to receive many of these fixes "for free".
>>>> If others want to burn their tokens, let them :)
>>> I have lots of tokens to burn :)
>>>
>>> So along those lines, any suggestions on how best to fuzz these code
>>> paths? Any workloads you all use for testing that I can take advantage
>>> of?
>>
>> We've the virto-mem config struct layout and the kernel source, so for
>> obvious fixes like a NULL check, static analysis is better than fuzzing.
>> Claude took a few mins to find me two examples:
>>
>> Patch 1: virtio-mem: reject non-power-of-two device_block_size
>> This one is for virtio_mem_init() to check if
>> !is_power_of_2(vm->device_block_size)
>>
>> Patch 2: virto-mem: validate region_size and usable_region_size
>> THis one checks region_size != 0 and vm->usable_reion_size >
>> vm->region_size.
>>
>> An endless factory of "silly" checks like these are low hanging fruit.
>>
>> Now, for harder bugs, looking around for fuzz options, VirtFuzz [1] looks
>> like a great candidate for those interested in pursuing this direction.
>>
>>
>> Their PoC fuzzes wireless/Bluetooth stack, but nothing our AI overlords
>> can't quickly adapt for virtio-mem and other virtio drivers; the JSON
>> definition to describe device behavior is easily extensible. Their threat
>> model [2] describes an external attacker, but in the context of coco, the
>> virtio device itself is the attacker. Here's a vibe coded PR of what I mean:
>>
>> https://github.com/seemoo-lab/VirtFuzz/pull/7
>>
>> CCed the creators/authors, thanks for open sourcing this!
>>
>> Thanks,
>> Carlos
>>
>> [1] https://github.com/seemoo-lab/VirtFuzz
>> [2] https://www.computer.org/csdl/proceedings-article/sp/2024/313000a024/1RjEa0y9RMQ
>>
>>
>>> thanks,
>>>
>>> greg k-h
>> [2] https://www.computer.org/csdl/proceedings-article/sp/2024/313000a024/1RjEa0y9RMQ
>>
>>
>>> thanks,
>>>
>>> greg k-h
Thanks,
Carlos
^ permalink raw reply
* Re: [PATCH v4 6/8] media: virtio: Add virtio_media_driver
From: Michael S. Tsirkin @ 2026-07-18 17:30 UTC (permalink / raw)
To: Brian Daniels
Cc: Mauro Carvalho Chehab, acourbot, adelva, aesteve, changyeon,
daniel.almeida, eperezma, gnurou, gurchetansingh, hverkuil,
jasowang, linux-kernel, linux-media, nicolas.dufresne,
virtualization, xuanzhuo
In-Reply-To: <20260625201850.2981130-1-briandaniels@google.com>
On Thu, Jun 25, 2026 at 04:18:48PM -0400, Brian Daniels wrote:
> > > From: Alexandre Courbot <gnurou@gmail.com>
> > >
> > > virtio_media_driver.c provides the expected driver hooks, and support
> > > for mmapping and polling.
> > >
> > > Signed-off-by: Alexandre Courbot <gnurou@gmail.com>
> > > Co-developed-by: Brian Daniels <briandaniels@google.com>
> > > Signed-off-by: Brian Daniels <briandaniels@google.com>
> > > ---
> > > drivers/media/virtio/virtio_media_driver.c | 959 +++++++++++++++++++++
> > > 1 file changed, 959 insertions(+)
> > > create mode 100644 drivers/media/virtio/virtio_media_driver.c
> > >
> > > diff --git a/drivers/media/virtio/virtio_media_driver.c b/drivers/media/virtio/virtio_media_driver.c
> > > new file mode 100644
> > > index 000000000..d6363c673
> > > --- /dev/null
> > > +++ b/drivers/media/virtio/virtio_media_driver.c
> > > @@ -0,0 +1,959 @@
> > > +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0+
> > > +
> > > +/*
> > > + * Virtio-media driver.
> > > + *
> > > + * Copyright (c) 2024-2025 Google LLC.
> > > + */
> > > +
> > > +#include <linux/delay.h>
> > > +#include <linux/device.h>
> > > +#include <linux/dev_printk.h>
> > > +#include <linux/mm.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/scatterlist.h>
> > > +#include <linux/types.h>
> > > +#include <linux/videodev2.h>
> > > +#include <linux/vmalloc.h>
> > > +#include <linux/wait.h>
> > > +#include <linux/workqueue.h>
> > > +#include <linux/module.h>
> > > +#include <linux/moduleparam.h>
> > > +#include <linux/virtio.h>
> > > +#include <linux/virtio_config.h>
> > > +#include <linux/virtio_ids.h>
> > > +
> > > +#include <media/frame_vector.h>
> > > +#include <media/v4l2-dev.h>
> > > +#include <media/v4l2-event.h>
> > > +#include <media/videobuf2-memops.h>
> > > +#include <media/v4l2-device.h>
> > > +#include <media/v4l2-ioctl.h>
> > > +
> > > +#include "protocol.h"
> > > +#include "session.h"
> > > +#include "virtio_media.h"
> > > +
> > > +#define VIRTIO_MEDIA_NUM_EVENT_BUFS 16
> > > +
> > > +/* ID of the SHM region into which MMAP buffer will be mapped. */
> > > +#define VIRTIO_MEDIA_SHM_MMAP 0
> > > +
> > > +/*
> > > + * Name of the driver to expose to user-space.
> > > + *
> > > + * This is configurable because v4l2-compliance has workarounds specific to
> > > + * some drivers. When proxying these directly from the host, this allows it to
> > > + * apply them as needed.
> > > + */
> > > +char *virtio_media_driver_name;
> > > +module_param_named(driver_name, virtio_media_driver_name, charp, 0660);
> >
> >
> > Um. What? Not how it should be handled.
>
> I can remove this module param. I didn't end up using this when compliance testing.
> Instead, I patched v4l-utils:
> https://lore.kernel.org/all/20260528163448.4031965-1-briandaniels@google.com/
>
> Let me know if you think the v4l-utils patch is a good approach, otherwise let
> me know how you'd prefer to address the v4l2-compliance driver-specific workounds
> when they're being proxied with virtio-media.
>
> > > +
> > > +/*
> > > + * Whether USERPTR buffers are allowed.
> > > + *
> > > + * This is disabled by default as USERPTR buffers are dangerous, but the option
> > > + * is left to enable them if desired.
> > > + */
> > > +bool virtio_media_allow_userptr;
> > > +module_param_named(allow_userptr, virtio_media_allow_userptr, bool, 0660);
> >
> >
> > is this kind of thing common?
>
> To be honest, I don't really know. I'm also not that familiar with the USERPTR
> issues. I see a few references online about their use being discouraged due to
> possible race conditions, perhaps that was the original motivation for this
> parameter (I'm not the original author of this driver).
>
> I'm open to alternatives, feel free to let me know if you have a preference.
>
> > > +
> > > +/**
> > > + * virtio_media_session_alloc - Allocate a new session.
> > > + * @vv: virtio-media device the session belongs to.
> > > + * @id: ID of the session.
> > > + * @nonblocking_dequeue: whether dequeuing of buffers should be blocking or
> > > + * not.
> > > + *
> > > + * The ``id`` and ``list`` fields must still be set by the caller.
> >
> > still in what sense?
>
> Based on the code below, I'm not so sure that the caller is responsible for
> setting these values. They seem to be initialized in the function.
>
> Perhaps Alexandre Courbot (the original author) would know more. Unless he
> says otherwise though I'm inclined to remove this comment.
>
> > > + */
> > > +static struct virtio_media_session *
> > > +virtio_media_session_alloc(struct virtio_media *vv, u32 id,
> > > + struct file *file)
> > > +{
> > > + struct virtio_media_session *session;
> > > + int i;
> > > + int ret;
> > > +
> > > + session = kzalloc_obj(*session, GFP_KERNEL);
> > > + if (!session)
> > > + goto err_session;
> > > +
> > > + session->shadow_buf = kzalloc(VIRTIO_SHADOW_BUF_SIZE, GFP_KERNEL);
> > > + if (!session->shadow_buf)
> > > + goto err_shadow_buf;
> > > +
> > > + ret = sg_alloc_table(&session->command_sgs, DESC_CHAIN_MAX_LEN,
> > > + GFP_KERNEL);
> > > + if (ret)
> > > + goto err_payload_sgs;
> > > +
> > > + session->id = id;
> > > + session->nonblocking_dequeue = file->f_flags & O_NONBLOCK;
> > > +
> > > + INIT_LIST_HEAD(&session->list);
> > > + v4l2_fh_init(&session->fh, &vv->video_dev);
> > > + virtio_media_session_fh_add(session, file);
> > > +
> > > + for (i = 0; i <= VIRTIO_MEDIA_LAST_QUEUE; i++)
> > > + INIT_LIST_HEAD(&session->queues[i].pending_dqbufs);
> > > + mutex_init(&session->queues_lock);
> > > +
> > > + init_waitqueue_head(&session->dqbuf_wait);
> > > +
> > > + mutex_lock(&vv->sessions_lock);
> > > + list_add_tail(&session->list, &vv->sessions);
> > > + mutex_unlock(&vv->sessions_lock);
> > > +
> > > + return session;
> > > +
> > > +err_payload_sgs:
> > > + kfree(session->shadow_buf);
> > > +err_shadow_buf:
> > > + kfree(session);
> > > +err_session:
> > > + return ERR_PTR(-ENOMEM);
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_session_free - Free all resources of a session.
> > > + * @vv: virtio-media device the session belongs to.
> > > + * @session: session to destroy.
> > > + *
> > > + * All the resources of @sesssion, as well as the backing memory of @session
> > > + * itself, are freed.
> >
> > why @ here and `` above? And typo in the name.
>
> The `@` here was an attempt to follow the guide here for referencing function
> parameters:
> https://docs.kernel.org/doc-guide/kernel-doc.html#highlights-and-cross-references
>
> That being said, I don't believe this file is 100% consistent with that. I will
> spend some time cleaning up the comments throughout this patch set to get them
> consistent for v5. Thanks!
>
> > > + */
> > > +static void virtio_media_session_free(struct virtio_media *vv,
> > > + struct virtio_media_session *session)
> > > +{
> > > + int i;
> > > +
> > > + mutex_lock(&vv->sessions_lock);
> > > + list_del(&session->list);
> > > + mutex_unlock(&vv->sessions_lock);
> > > +
> > > + virtio_media_session_fh_del(session);
> > > + v4l2_fh_exit(&session->fh);
> > > +
> > > + sg_free_table(&session->command_sgs);
> > > +
> > > + for (i = 0; i <= VIRTIO_MEDIA_LAST_QUEUE; i++)
> > > + vfree(session->queues[i].buffers);
> > > +
> > > + kfree(session->shadow_buf);
> > > + kfree(session);
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_session_close - Close and free a session.
> > > + * @vv: virtio-media device the session belongs to.
> > > + * @session: session to close and destroy.
> > > + *
> > > + * This send the ``VIRTIO_MEDIA_CMD_CLOSE`` command to the device, and frees
> >
> > sends
>
> Fix staged in v5
>
> > > + * all resources used by @session.
> > > + */
> > > +static int virtio_media_session_close(struct virtio_media *vv,
> > > + struct virtio_media_session *session)
> > > +{
> > > + struct virtio_media_cmd_close *cmd_close = &session->cmd.close;
> > > + struct scatterlist cmd_sg = {};
> > > + struct scatterlist *sgs[1] = { &cmd_sg };
> > > + int ret;
> > > +
> > > + mutex_lock(&vv->vlock);
> > > +
> > > + cmd_close->hdr.cmd = VIRTIO_MEDIA_CMD_CLOSE;
> > > + cmd_close->session_id = session->id;
> > > +
> > > + sg_set_buf(&cmd_sg, cmd_close, sizeof(*cmd_close));
> > > + sg_mark_end(&cmd_sg);
> > > +
> > > + ret = virtio_media_send_command(vv, sgs, 1, 0, 0, NULL);
> > > + mutex_unlock(&vv->vlock);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + virtio_media_session_free(vv, session);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_find_session - Lookup for the session with a given ID.
> >
> > a session
>
> Fix staged in v5
>
> > > + * @vv: virtio-media device to lookup the session from.
> > > + * @id: ID of the session to lookup.
> > > + */
> > > +static struct virtio_media_session *
> > > +virtio_media_find_session(struct virtio_media *vv, u32 id)
> > > +{
> > > + struct list_head *p;
> > > + struct virtio_media_session *session = NULL;
> > > +
> > > + mutex_lock(&vv->sessions_lock);
> > > + list_for_each(p, &vv->sessions) {
> > > + struct virtio_media_session *s =
> > > + list_entry(p, struct virtio_media_session, list);
> > > + if (s->id == id) {
> > > + session = s;
> > > + break;
> > > + }
> > > + }
> > > + mutex_unlock(&vv->sessions_lock);
> > > +
> > > + return session;
> > > +}
> > > +
> > > +/**
> > > + * struct virtio_media_cmd_callback_param - Callback parameters to the virtio
> > > + * command queue.
> > > + * @vv: virtio-media device in use.
> > > + * @done: flag to be switched once the command is completed.
> > > + * @resp_len: length of the received response from the command. Only valid
> > > + * after @done_flag has switched to ``true``.
> >
> > confusing indent
>
> Fix staged in v5
>
> > > + */
> > > +struct virtio_media_cmd_callback_param {
> > > + struct virtio_media *vv;
> > > + bool done;
> > > + size_t resp_len;
> > > +};
> > > +
> > > +/**
> > > + * commandq_callback: Callback for the command queue.
> > > + * @queue: command virtqueue.
> > > + *
> > > + * This just wakes up the thread that was waiting on the command to complete.
> > > + */
> > > +static void commandq_callback(struct virtqueue *queue)
> > > +{
> > > + unsigned int len;
> > > + struct virtio_media_cmd_callback_param *param;
> > > +
> > > +process_bufs:
> > > + while ((param = virtqueue_get_buf(queue, &len))) {
> > > + param->done = true;
> > > + param->resp_len = len;
> > > + wake_up(¶m->vv->wq);
> > > + }
> > > +
> > > + if (!virtqueue_enable_cb(queue)) {
> > > + virtqueue_disable_cb(queue);
> > > + goto process_bufs;
> > > + }
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_kick_command - send a command to the commandq.
> > > + * @vv: virtio-media device in use.
> > > + * @sgs: descriptor chain to send.
> > > + * @out_sgs: number of device-readable descriptors in @sgs.
> > > + * @in_sgs: number of device-writable descriptors in @sgs.
> > > + * @resp_len: output parameter. Upon success, contains the size of the response
> > > + * in bytes.
> >
> > confusing indent
> >
> > > + *
> >
> > why an empty line?
>
> Indent fixed and empty line removed in v5
>
> > > + */
> > > +static int virtio_media_kick_command(struct virtio_media *vv,
> > > + struct scatterlist **sgs,
> > > + const size_t out_sgs, const size_t in_sgs,
> > > + size_t *resp_len)
> > > +{
> > > + struct virtio_media_cmd_callback_param cb_param = {
> > > + .vv = vv,
> > > + .done = false,
> > > + .resp_len = 0,
> > > + };
> > > + struct virtio_media_resp_header *resp_header;
> > > + int ret;
> > > +
> > > + ret = virtqueue_add_sgs(vv->commandq, sgs, out_sgs, in_sgs, &cb_param,
> > > + GFP_ATOMIC);
> >
> >
> > can init with declaration.
>
> Thanks, added to v5
>
> > > + if (ret) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "failed to add sgs to command virtqueue\n");
> > > + return ret;
> > > + }
> > > +
> > > + if (!virtqueue_kick(vv->commandq)) {
> > > + v4l2_err(&vv->v4l2_dev, "failed to kick command virtqueue\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + /* Wait for the response. */
> > > + ret = wait_event_timeout(vv->wq, cb_param.done, 5 * HZ);
> > > + if (ret == 0) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "timed out waiting for response to command\n");
> > > + return -ETIMEDOUT;
> > > + }
> > > +
> > > + if (resp_len)
> > > + *resp_len = cb_param.resp_len;
> > > +
> > > + if (in_sgs > 0) {
> > > + /*
> > > + * If we expect a response, make sure we have at least a
> > > + * response header - anything shorter is invalid.
> > > + */
> > > + if (cb_param.resp_len < sizeof(*resp_header)) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "received response header is too short\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + resp_header = sg_virt(sgs[out_sgs]);
> > > + if (resp_header->status)
> > > + /* Host returns a positive error code. */
> > > + return -resp_header->status;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_send_command - Send a command to the device and wait for its
> > > + * response.
> > > + * @vv: virtio-media device in use.
> > > + * @sgs: descriptor chain to send.
> > > + * @out_sgs: number of device-readable descriptors in @sgs.
> > > + * @in_sgs: number of device-writable descriptors in @sgs.
> > > + * @minimum_resp_len: minimum length of the response expected by the caller
> > > + * when the command is successful. Anything shorter than that will result in
> > > + * ``-EINVAL`` being returned.
> > > + * @resp_len: output parameter. Upon success, contains the size of the response
> > > + * in bytes.
> > > + */
> > > +int virtio_media_send_command(struct virtio_media *vv, struct scatterlist **sgs,
> > > + const size_t out_sgs, const size_t in_sgs,
> > > + size_t minimum_resp_len, size_t *resp_len)
> > > +{
> > > + size_t local_resp_len = resp_len ? *resp_len : 0;
> > > + int ret = virtio_media_kick_command(vv, sgs, out_sgs, in_sgs,
> > > + &local_resp_len);
> > > + if (resp_len)
> > > + *resp_len = local_resp_len;
> > > +
> > > + /*
> > > + * If the host could not process the command, there is no valid
> > > + * response.
> > > + */
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + /* Make sure the host wrote a complete reply. */
> > > + if (local_resp_len < minimum_resp_len) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "received response is too short: received %zu, expected at least %zu\n",
> > > + local_resp_len, minimum_resp_len);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_send_event_buffer() - Sends an event buffer to the host so it
> > > + * can return it with an event.
> > > + * @vv: virtio-media device in use.
> > > + * @event_buffer: pointer to the event buffer to send to the device.
> > > + */
> > > +static int virtio_media_send_event_buffer(struct virtio_media *vv,
> > > + void *event_buffer)
> > > +{
> > > + struct scatterlist *sgs[1], vresp;
> > > + int ret;
> > > +
> > > + sg_init_one(&vresp, event_buffer, VIRTIO_MEDIA_EVENT_MAX_SIZE);
> > > + sgs[0] = &vresp;
> >
> > what is this convoluted thing? why not pass &vresp directly?
>
> `virtqueue_add_sgs` expects a `struct scatterlist **`. Passing `&vresp` is
> only a single pointer and it needs a double pointer.
>
> It could be written as:
>
> struct scatterlist *sgs, vresp;
>
> sg_init_one(&vresp, event_buffer, VIRTIO_MEDIA_EVENT_MAX_SIZE);
> sgs = &vresp;
> ret = virtqueue_add_sgs(vv->eventq, &sgs, 0, 1, event_buffer,
>
> But I suspect it's written as is to match `virtqueue_add_sgs`'s function
> declaration:
>
> int virtqueue_add_sgs(struct virtqueue *vq,
> struct scatterlist *sgs[],
> unsigned int out_sgs,
> unsigned int in_sgs,
> void *data,
> gfp_t gfp);
>
> > > +
> > > + ret = virtqueue_add_sgs(vv->eventq, sgs, 0, 1, event_buffer,
> > > + GFP_ATOMIC);
> >
> >
> > This does not work uness event_buffer is aligned for dma.
> > But it does not seem to be:
> >
> > for (i = 0; i < VIRTIO_MEDIA_NUM_EVENT_BUFS; i++) {
> > void *ebuf = vv->event_buffer + VIRTIO_MEDIA_EVENT_MAX_SIZE * i;
> >
> > ret = virtio_media_send_event_buffer(vv, ebuf);
> > if (ret)
> > goto err_send_event_buffer;
>
> Apologies if the following questions are bit basic, I'm still pretty new to
> this.
>
> Is the dma alignment requirement a general requirement for calling the
> `virtqueue_add_sgs` function? Or is the dma alignment requirement to allow the
> driver to support DMABUF transfers? If its the later, I should note this driver
> does not support DMABUF transfers yet when streaming frames.
>
> If it's the former, then how would I go about ensuring its dma aligned?
See Documentation/core-api/dma-api-howto.rst
> >From the code block you quoted above, `vv->event_buffer` is allocated like so:
>
> vv->event_buffer = devm_kzalloc(dev,
> VIRTIO_MEDIA_EVENT_MAX_SIZE *
> VIRTIO_MEDIA_NUM_EVENT_BUFS,
> GFP_KERNEL);
> if (!vv->event_buffer)
> return -ENOMEM;
>
> Do I need to call something else besides `devm_kzalloc()` to ensure every
> element within the vv->event_buffer array is dma aligned?
You need to either allocate each one separately or
add padding.
> >
> >
> > > + if (ret) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "failed to add sgs to event virtqueue\n");
> > > + return ret;
> > > + }
> > > +
> > > + if (!virtqueue_kick(vv->eventq)) {
> > > + v4l2_err(&vv->v4l2_dev, "failed to kick event virtqueue\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * eventq_callback() - Callback for the event queue.
> > > + * @queue: event virtqueue.
> > > + *
> > > + * This just schedules for event work to be run.
> > > + */
> > > +static void eventq_callback(struct virtqueue *queue)
> > > +{
> > > + struct virtio_media *vv = queue->vdev->priv;
> > > +
> > > + schedule_work(&vv->eventq_work);
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_process_dqbuf_event() - Process a dequeued event for a session.
> > > + * @vv: virtio-media device in use.
> > > + * @session: session the event is addressed to.
> > > + * @dqbuf_evt: the dequeued event to process.
> > > + *
> > > + * Invalid events are ignored with an error log.
> > > + */
> > > +static void
> > > +virtio_media_process_dqbuf_event(struct virtio_media *vv,
> > > + struct virtio_media_session *session,
> > > + struct virtio_media_event_dqbuf *dqbuf_evt)
> > > +{
> > > + struct virtio_media_buffer *dqbuf;
> > > + const enum v4l2_buf_type queue_type = dqbuf_evt->buffer.type;
> > > + struct virtio_media_queue_state *queue;
> > > + typeof(dqbuf->buffer.m) buffer_m;
> > > + typeof(dqbuf->buffer.m.planes[0].m) plane_m;
> > > + int i;
> > > +
> > > + if (queue_type >= ARRAY_SIZE(session->queues)) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "unmanaged queue %d passed to dqbuf event",
> > > + dqbuf_evt->buffer.type);
> > > + return;
> > > + }
> > > + queue = &session->queues[queue_type];
> > > +
> > > + if (dqbuf_evt->buffer.index >= queue->allocated_bufs) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "invalid buffer ID %d for queue %d in dqbuf event",
> > > + dqbuf_evt->buffer.index, dqbuf_evt->buffer.type);
> > > + return;
> > > + }
> > > +
> > > + dqbuf = &queue->buffers[dqbuf_evt->buffer.index];
> > > +
> > > + /*
> > > + * Preserve the 'm' union that was passed to us during QBUF so userspace
> > > + * gets back the information it submitted.
> > > + */
> > > + buffer_m = dqbuf->buffer.m;
> > > + memcpy(&dqbuf->buffer, &dqbuf_evt->buffer, sizeof(dqbuf->buffer));
> > > + dqbuf->buffer.m = buffer_m;
> > > + if (V4L2_TYPE_IS_MULTIPLANAR(dqbuf->buffer.type)) {
> > > + if (dqbuf->buffer.length > VIDEO_MAX_PLANES) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "invalid number of planes received from host for a multiplanar buffer\n");
> > > + return;
> > > + }
> > > + for (i = 0; i < dqbuf->buffer.length; i++) {
> > > + plane_m = dqbuf->planes[i].m;
> > > + memcpy(&dqbuf->planes[i], &dqbuf_evt->planes[i],
> > > + sizeof(struct v4l2_plane));
> > > + dqbuf->planes[i].m = plane_m;
> > > + }
> > > + }
> > > +
> > > + /* Set the DONE flag as the buffer is waiting for being dequeued. */
> > > + dqbuf->buffer.flags |= V4L2_BUF_FLAG_DONE;
> > > +
> > > + mutex_lock(&session->queues_lock);
> > > + list_add_tail(&dqbuf->list, &queue->pending_dqbufs);
> > > + queue->queued_bufs -= 1;
> > > + mutex_unlock(&session->queues_lock);
> > > +
> > > + wake_up(&session->dqbuf_wait);
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_process_events() - Process all pending events on a device.
> > > + * @vv: device which pending events we want to process.
> > > + *
> > > + * Retrieves all pending events on @vv's event queue and dispatch them to their
> > > + * corresponding session.
> > > + *
> > > + * Invalid events are ignored with an error log.
> > > + */
> > > +void virtio_media_process_events(struct virtio_media *vv)
> > > +{
> > > + struct virtio_media_event_error *error_evt;
> > > + struct virtio_media_event_dqbuf *dqbuf_evt;
> > > + struct virtio_media_event_event *event_evt;
> > > + struct virtio_media_session *session;
> > > + struct virtio_media_event_header *evt;
> > > + unsigned int len;
> > > +
> > > + mutex_lock(&vv->events_lock);
> > > +
> > > +process_bufs:
> > > + while ((evt = virtqueue_get_buf(vv->eventq, &len))) {
> > > + /* Make sure we received enough data */
> > > + if (len < sizeof(*evt)) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "event is too short: got %u, expected at least %zu\n",
> > > + len, sizeof(*evt));
> > > + goto end_of_event;
> > > + }
> > > +
> > > + session = virtio_media_find_session(vv, evt->session_id);
> > > + if (!session) {
> > > + v4l2_err(&vv->v4l2_dev, "cannot find session %d\n",
> > > + evt->session_id);
> > > + goto end_of_event;
> > > + }
> > > +
> > > + switch (evt->event) {
> > > + case VIRTIO_MEDIA_EVT_ERROR:
> > > + if (len < sizeof(*error_evt)) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "error event is too short: got %u, expected %zu\n",
> > > + len, sizeof(*error_evt));
> > > + break;
> > > + }
> > > + error_evt = (struct virtio_media_event_error *)evt;
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "received error %d for session %d",
> > > + error_evt->errno, error_evt->hdr.session_id);
> > > + virtio_media_session_close(vv, session);
> > > + break;
> > > +
> > > + /*
> > > + * Dequeued buffer: put it into the right queue so user-space
> > > + * can dequeue it.
> > > + */
> > > + case VIRTIO_MEDIA_EVT_DQBUF:
> > > + if (len < sizeof(*dqbuf_evt)) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "dqbuf event is too short: got %u, expected %zu\n",
> > > + len, sizeof(*dqbuf_evt));
> > > + break;
> > > + }
> > > + dqbuf_evt = (struct virtio_media_event_dqbuf *)evt;
> > > + virtio_media_process_dqbuf_event(vv, session,
> > > + dqbuf_evt);
> > > + break;
> > > +
> > > + case VIRTIO_MEDIA_EVT_EVENT:
> > > + if (len < sizeof(*event_evt)) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "session event is too short: got %u expected %zu\n",
> > > + len, sizeof(*event_evt));
> > > + break;
> > > + }
> > > +
> > > + event_evt = (struct virtio_media_event_event *)evt;
> > > + v4l2_event_queue_fh(&session->fh, &event_evt->event);
> > > + break;
> > > +
> > > + default:
> > > + v4l2_err(&vv->v4l2_dev, "unknown event type %d\n",
> > > + evt->event);
> > > + break;
> > > + }
> > > +
> > > +end_of_event:
> > > + virtio_media_send_event_buffer(vv, evt);
> > > + }
> > > +
> > > + if (!virtqueue_enable_cb(vv->eventq)) {
> > > + virtqueue_disable_cb(vv->eventq);
> > > + goto process_bufs;
> > > + }
> > > +
> > > + mutex_unlock(&vv->events_lock);
> > > +}
> > > +
> > > +static void virtio_media_event_work(struct work_struct *work)
> > > +{
> > > + struct virtio_media *vv =
> > > + container_of(work, struct virtio_media, eventq_work);
> > > +
> > > + virtio_media_process_events(vv);
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_device_open() - Create a new session from an opened file.
> > > + * @file: opened file for the session.
> > > + */
> > > +static int virtio_media_device_open(struct file *file)
> > > +{
> > > + struct video_device *video_dev = video_devdata(file);
> > > + struct virtio_media *vv = to_virtio_media(video_dev);
> > > + struct virtio_media_cmd_open *cmd_open = &vv->cmd.open;
> > > + struct virtio_media_resp_open *resp_open = &vv->resp.open;
> > > + struct scatterlist cmd_sg = {}, resp_sg = {};
> > > + struct scatterlist *sgs[2] = { &cmd_sg, &resp_sg };
> > > + struct virtio_media_session *session;
> > > + u32 session_id;
> > > + int ret;
> > > +
> > > + mutex_lock(&vv->vlock);
> > > +
> > > + sg_set_buf(&cmd_sg, cmd_open, sizeof(*cmd_open));
> > > + sg_mark_end(&cmd_sg);
> > > +
> > > + sg_set_buf(&resp_sg, resp_open, sizeof(*resp_open));
> > > + sg_mark_end(&resp_sg);
> > > +
> > > + cmd_open->hdr.cmd = VIRTIO_MEDIA_CMD_OPEN;
> > > + ret = virtio_media_send_command(vv, sgs, 1, 1, sizeof(*resp_open),
> > > + NULL);
> > > + session_id = resp_open->session_id;
> > > + mutex_unlock(&vv->vlock);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + session = virtio_media_session_alloc(vv, session_id, file);
> > > + if (IS_ERR(session))
> > > + return PTR_ERR(session);
> > > +
> > > + file->private_data = &session->fh;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_device_close() - Close a previously opened session.
> > > + * @file: file of the session to close.
> > > + *
> > > + * This sends to ``VIRTIO_MEDIA_CMD_CLOSE`` command to the device, and close
> > > + * the session on the driver side.
> > > + */
> > > +static int virtio_media_device_close(struct file *file)
> > > +{
> > > + struct video_device *video_dev = video_devdata(file);
> > > + struct virtio_media *vv = to_virtio_media(video_dev);
> > > + struct virtio_media_session *session =
> > > + fh_to_session(file->private_data);
> > > +
> > > + return virtio_media_session_close(vv, session);
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_device_poll() - Poll logic for a virtio-media device.
> > > + * @file: file of the session to poll.
> > > + * @wait: poll table to wait on.
> > > + */
> > > +static __poll_t virtio_media_device_poll(struct file *file, poll_table *wait)
> > > +{
> > > + struct virtio_media_session *session =
> > > + fh_to_session(file->private_data);
> > > + enum v4l2_buf_type capture_type =
> > > + session->uses_mplane ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE :
> > > + V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > > + enum v4l2_buf_type output_type =
> > > + session->uses_mplane ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE :
> > > + V4L2_BUF_TYPE_VIDEO_OUTPUT;
> > > + struct virtio_media_queue_state *capture_queue =
> > > + &session->queues[capture_type];
> > > + struct virtio_media_queue_state *output_queue =
> > > + &session->queues[output_type];
> > > + __poll_t req_events = poll_requested_events(wait);
> > > + __poll_t rc = 0;
> > > +
> > > + poll_wait(file, &session->dqbuf_wait, wait);
> > > + poll_wait(file, &session->fh.wait, wait);
> > > +
> > > + mutex_lock(&session->queues_lock);
> > > + if (req_events & (EPOLLIN | EPOLLRDNORM)) {
> > > + if (!capture_queue->streaming ||
> > > + (capture_queue->queued_bufs == 0 &&
> > > + list_empty(&capture_queue->pending_dqbufs)))
> > > + rc |= EPOLLERR;
> > > + else if (!list_empty(&capture_queue->pending_dqbufs))
> > > + rc |= EPOLLIN | EPOLLRDNORM;
> > > + }
> > > + if (req_events & (EPOLLOUT | EPOLLWRNORM)) {
> > > + if (!output_queue->streaming)
> > > + rc |= EPOLLERR;
> > > + else if (output_queue->queued_bufs <
> > > + output_queue->allocated_bufs)
> > > + rc |= EPOLLOUT | EPOLLWRNORM;
> > > + }
> > > + mutex_unlock(&session->queues_lock);
> > > +
> > > + if (v4l2_event_pending(&session->fh))
> > > + rc |= EPOLLPRI;
> > > +
> > > + return rc;
> > > +}
> > > +
> > > +static void virtio_media_vma_close_locked(struct vm_area_struct *vma)
> > > +{
> > > + struct virtio_media *vv = vma->vm_private_data;
> > > + struct virtio_media_cmd_munmap *cmd_munmap = &vv->cmd.munmap;
> > > + struct virtio_media_resp_munmap *resp_munmap = &vv->resp.munmap;
> > > + struct scatterlist cmd_sg = {}, resp_sg = {};
> > > + struct scatterlist *sgs[2] = { &cmd_sg, &resp_sg };
> > > + int ret;
> > > +
> > > + sg_set_buf(&cmd_sg, cmd_munmap, sizeof(*cmd_munmap));
> > > + sg_mark_end(&cmd_sg);
> > > +
> > > + sg_set_buf(&resp_sg, resp_munmap, sizeof(*resp_munmap));
> > > + sg_mark_end(&resp_sg);
> > > +
> > > + cmd_munmap->hdr.cmd = VIRTIO_MEDIA_CMD_MUNMAP;
> > > + cmd_munmap->driver_addr =
> > > + (vma->vm_pgoff << PAGE_SHIFT) - vv->mmap_region.addr;
> > > + ret = virtio_media_send_command(vv, sgs, 1, 1, sizeof(*resp_munmap),
> > > + NULL);
> > > + if (ret < 0) {
> > > + v4l2_err(&vv->v4l2_dev, "host failed to unmap buffer: %d\n",
> > > + ret);
> > > + }
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_vma_close() - Close a MMAP buffer mapping.
> > > + * @vma: VMA of the mapping to close.
> > > + *
> > > + * Inform the host that a previously created MMAP mapping is no longer needed
> > > + * and can be removed.
> > > + */
> > > +static void virtio_media_vma_close(struct vm_area_struct *vma)
> > > +{
> > > + struct virtio_media *vv = vma->vm_private_data;
> > > +
> > > + mutex_lock(&vv->vlock);
> > > + virtio_media_vma_close_locked(vma);
> > > + mutex_unlock(&vv->vlock);
> > > +}
> > > +
> > > +static const struct vm_operations_struct virtio_media_vm_ops = {
> > > + .close = virtio_media_vma_close,
> > > +};
> > > +
> > > +/**
> > > + * virtio_media_device_mmap - Perform a mmap request from userspace.
> > > + * @file: opened file of the session to map for.
> > > + * @vma: VM area struct describing the desired mapping.
> > > + *
> > > + * This requests the host to map a MMAP buffer for us, so we can then make that
> > > + * mapping visible into user-space address space.
> > > + */
> > > +static int virtio_media_device_mmap(struct file *file,
> > > + struct vm_area_struct *vma)
> > > +{
> > > + struct video_device *video_dev = video_devdata(file);
> > > + struct virtio_media *vv = to_virtio_media(video_dev);
> > > + struct virtio_media_session *session =
> > > + fh_to_session(file->private_data);
> > > + struct virtio_media_cmd_mmap *cmd_mmap = &session->cmd.mmap;
> > > + struct virtio_media_resp_mmap *resp_mmap = &session->resp.mmap;
> > > + struct scatterlist cmd_sg = {}, resp_sg = {};
> > > + struct scatterlist *sgs[2] = { &cmd_sg, &resp_sg };
> > > + int ret;
> > > +
> > > + if (!(vma->vm_flags & VM_SHARED))
> > > + return -EINVAL;
> > > + if (!(vma->vm_flags & (VM_READ | VM_WRITE)))
> > > + return -EINVAL;
> > > +
> > > + mutex_lock(&vv->vlock);
> > > +
> > > + cmd_mmap->hdr.cmd = VIRTIO_MEDIA_CMD_MMAP;
> > > + cmd_mmap->session_id = session->id;
> > > + cmd_mmap->flags =
> > > + (vma->vm_flags & VM_WRITE) ? VIRTIO_MEDIA_MMAP_FLAG_RW : 0;
> > > + cmd_mmap->offset = vma->vm_pgoff << PAGE_SHIFT;
> > > +
> > > + sg_set_buf(&cmd_sg, cmd_mmap, sizeof(*cmd_mmap));
> > > + sg_mark_end(&cmd_sg);
> > > +
> > > + sg_set_buf(&resp_sg, resp_mmap, sizeof(*resp_mmap));
> > > + sg_mark_end(&resp_sg);
> > > +
> > > + /*
> > > + * The host performs reference counting and is smart enough to return
> > > + * the same guest physical address if this is called several times on
> > > + * the same
> > > + * buffer.
> > > + */
> > > + ret = virtio_media_send_command(vv, sgs, 1, 1, sizeof(*resp_mmap),
> > > + NULL);
> > > + if (ret < 0)
> > > + goto end;
> > > +
> > > + vma->vm_private_data = vv;
> > > + /*
> > > + * Keep the guest address at which the buffer is mapped since we will
> > > + * use that to unmap.
> > > + */
> > > + vma->vm_pgoff = (resp_mmap->driver_addr + vv->mmap_region.addr) >>
> > > + PAGE_SHIFT;
> > > +
> > > + /*
> > > + * We cannot let the mapping be larger than the buffer.
> > > + */
> > > + if (vma->vm_end - vma->vm_start > PAGE_ALIGN(resp_mmap->len)) {
> > > + dev_dbg(&video_dev->dev,
> > > + "invalid MMAP, as it would overflow buffer length\n");
> > > + virtio_media_vma_close_locked(vma);
> > > + ret = -EINVAL;
> > > + goto end;
> > > + }
> > > +
> > > + ret = io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
> > > + vma->vm_end - vma->vm_start,
> > > + vma->vm_page_prot);
> > > + if (ret)
> > > + goto end;
> > > +
> > > + vma->vm_ops = &virtio_media_vm_ops;
> > > +
> > > +end:
> > > + mutex_unlock(&vv->vlock);
> > > + return ret;
> > > +}
> > > +
> > > +static const struct v4l2_file_operations virtio_media_fops = {
> > > + .owner = THIS_MODULE,
> > > + .open = virtio_media_device_open,
> > > + .release = virtio_media_device_close,
> > > + .poll = virtio_media_device_poll,
> > > + .unlocked_ioctl = virtio_media_device_ioctl,
> > > + .mmap = virtio_media_device_mmap,
> > > +};
> > > +
> > > +static int virtio_media_probe(struct virtio_device *virtio_dev)
> > > +{
> > > + struct device *dev = &virtio_dev->dev;
> > > + struct virtqueue *vqs[2];
> > > + static struct virtqueue_info vq_info[2] = {
> > > + {
> > > + .name = "command",
> > > + .callback = commandq_callback,
> > > + },
> > > + {
> > > + .name = "event",
> > > + .callback = eventq_callback,
> > > + },
> > > + };
> > > + struct virtio_media *vv;
> > > + struct video_device *vd;
> > > + int i;
> > > + int ret;
> > > +
> > > + vv = devm_kzalloc(dev, sizeof(*vv), GFP_KERNEL);
> > > + if (!vv)
> > > + return -ENOMEM;
> > > +
> > > + vv->event_buffer = devm_kzalloc(dev,
> > > + VIRTIO_MEDIA_EVENT_MAX_SIZE *
> > > + VIRTIO_MEDIA_NUM_EVENT_BUFS,
> > > + GFP_KERNEL);
> > > + if (!vv->event_buffer)
> > > + return -ENOMEM;
> > > +
> > > + INIT_LIST_HEAD(&vv->sessions);
> > > + mutex_init(&vv->sessions_lock);
> > > + mutex_init(&vv->events_lock);
> > > + mutex_init(&vv->vlock);
> > > +
> > > + vv->virtio_dev = virtio_dev;
> > > + virtio_dev->priv = vv;
> > > +
> > > + init_waitqueue_head(&vv->wq);
> > > +
> > > + ret = v4l2_device_register(dev, &vv->v4l2_dev);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = virtio_find_vqs(virtio_dev, 2, vqs, vq_info, NULL);
> > > + if (ret)
> > > + goto err_find_vqs;
> > > +
> > > + vv->commandq = vqs[0];
> > > + vv->eventq = vqs[1];
> > > + INIT_WORK(&vv->eventq_work, virtio_media_event_work);
> > > +
> > > + /* Get MMAP buffer mapping SHM region */
> > > + virtio_get_shm_region(virtio_dev, &vv->mmap_region,
> > > + VIRTIO_MEDIA_SHM_MMAP);
> > > +
> > > + vd = &vv->video_dev;
> > > +
> > > + vd->v4l2_dev = &vv->v4l2_dev;
> > > + vd->vfl_type = VFL_TYPE_VIDEO;
> > > + vd->ioctl_ops = &virtio_media_ioctl_ops;
> > > + vd->fops = &virtio_media_fops;
> > > + vd->device_caps = virtio_cread32(virtio_dev, 0);
> > > + if (vd->device_caps & (V4L2_CAP_VIDEO_M2M | V4L2_CAP_VIDEO_M2M_MPLANE))
> > > + vd->vfl_dir = VFL_DIR_M2M;
> > > + else if (vd->device_caps &
> > > + (V4L2_CAP_VIDEO_OUTPUT | V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE))
> > > + vd->vfl_dir = VFL_DIR_TX;
> > > + else
> > > + vd->vfl_dir = VFL_DIR_RX;
> > > + vd->release = video_device_release_empty;
> > > + strscpy(vd->name, "virtio-media", sizeof(vd->name));
> > > +
> > > + video_set_drvdata(vd, vv);
> > > +
> > > + ret = video_register_device(vd, virtio_cread32(virtio_dev, 4), 0);
> > > + if (ret)
> > > + goto err_register_device;
> > > +
> > > + for (i = 0; i < VIRTIO_MEDIA_NUM_EVENT_BUFS; i++) {
> > > + void *ebuf = vv->event_buffer + VIRTIO_MEDIA_EVENT_MAX_SIZE * i;
> > > +
> > > + ret = virtio_media_send_event_buffer(vv, ebuf);
> > > + if (ret)
> > > + goto err_send_event_buffer;
> > > + }
> > > +
> > > + virtio_device_ready(virtio_dev);
> > > +
> > > + return 0;
> > > +
> > > +err_send_event_buffer:
> > > + video_unregister_device(&vv->video_dev);
> > > +err_register_device:
> > > + virtio_dev->config->del_vqs(virtio_dev);
> > > +err_find_vqs:
> > > + v4l2_device_unregister(&vv->v4l2_dev);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static void virtio_media_remove(struct virtio_device *virtio_dev)
> > > +{
> > > + struct virtio_media *vv = virtio_dev->priv;
> > > + struct list_head *p, *n;
> > > +
> > > + cancel_work_sync(&vv->eventq_work);
> > > + virtio_reset_device(virtio_dev);
> > > +
> > > + v4l2_device_unregister(&vv->v4l2_dev);
> > > + virtio_dev->config->del_vqs(virtio_dev);
> > > + video_unregister_device(&vv->video_dev);
> > > +
> > > + list_for_each_safe(p, n, &vv->sessions) {
> > > + struct virtio_media_session *s =
> > > + list_entry(p, struct virtio_media_session, list);
> > > +
> > > + virtio_media_session_free(vv, s);
> > > + }
> > > +}
> > > +
> > > +static struct virtio_device_id id_table[] = {
> > > + { VIRTIO_ID_MEDIA, VIRTIO_DEV_ANY_ID },
> > > + { 0 },
> > > +};
> > > +
> > > +static unsigned int features[] = {};
> > > +
> > > +static struct virtio_driver virtio_media_driver = {
> > > + .feature_table = features,
> > > + .feature_table_size = ARRAY_SIZE(features),
> > > + .driver.name = VIRTIO_MEDIA_DEFAULT_DRIVER_NAME,
> > > + .driver.owner = THIS_MODULE,
> > > + .id_table = id_table,
> > > + .probe = virtio_media_probe,
> > > + .remove = virtio_media_remove,
> > > +};
> > > +
> > > +module_virtio_driver(virtio_media_driver);
> > > +
> > > +MODULE_DEVICE_TABLE(virtio, id_table);
> > > +MODULE_DESCRIPTION("virtio media driver");
> > > +MODULE_AUTHOR("Alexandre Courbot <gnurou@gmail.com>");
> > > +MODULE_LICENSE("Dual BSD/GPL");
^ permalink raw reply
* Re: [PATCH v4 6/8] media: virtio: Add virtio_media_driver
From: Michael S. Tsirkin @ 2026-07-18 17:28 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Brian Daniels, Mauro Carvalho Chehab, acourbot, adelva, aesteve,
changyeon, daniel.almeida, eperezma, gnurou, gurchetansingh,
hverkuil, jasowang, linux-kernel, linux-media, nicolas.dufresne,
virtualization, xuanzhuo
In-Reply-To: <20260712085726.19198fda@foz.lan>
On Sun, Jul 12, 2026 at 08:57:26AM +0200, Mauro Carvalho Chehab wrote:
> On Thu, 25 Jun 2026 16:18:48 -0400
> Brian Daniels <briandaniels@google.com> wrote:
>
> > > > From: Alexandre Courbot <gnurou@gmail.com>
> > > >
> > > > virtio_media_driver.c provides the expected driver hooks, and support
> > > > for mmapping and polling.
> > > >
> > > > Signed-off-by: Alexandre Courbot <gnurou@gmail.com>
> > > > Co-developed-by: Brian Daniels <briandaniels@google.com>
> > > > Signed-off-by: Brian Daniels <briandaniels@google.com>
> > > > ---
> > > > drivers/media/virtio/virtio_media_driver.c | 959 +++++++++++++++++++++
> > > > 1 file changed, 959 insertions(+)
> > > > create mode 100644 drivers/media/virtio/virtio_media_driver.c
> > > >
> > > > diff --git a/drivers/media/virtio/virtio_media_driver.c b/drivers/media/virtio/virtio_media_driver.c
> > > > new file mode 100644
> > > > index 000000000..d6363c673
> > > > --- /dev/null
> > > > +++ b/drivers/media/virtio/virtio_media_driver.c
> > > > @@ -0,0 +1,959 @@
> > > > +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0+
> > > > +
> > > > +/*
> > > > + * Virtio-media driver.
> > > > + *
> > > > + * Copyright (c) 2024-2025 Google LLC.
> > > > + */
> > > > +
> > > > +#include <linux/delay.h>
> > > > +#include <linux/device.h>
> > > > +#include <linux/dev_printk.h>
> > > > +#include <linux/mm.h>
> > > > +#include <linux/mutex.h>
> > > > +#include <linux/scatterlist.h>
> > > > +#include <linux/types.h>
> > > > +#include <linux/videodev2.h>
> > > > +#include <linux/vmalloc.h>
> > > > +#include <linux/wait.h>
> > > > +#include <linux/workqueue.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/moduleparam.h>
> > > > +#include <linux/virtio.h>
> > > > +#include <linux/virtio_config.h>
> > > > +#include <linux/virtio_ids.h>
> > > > +
> > > > +#include <media/frame_vector.h>
> > > > +#include <media/v4l2-dev.h>
> > > > +#include <media/v4l2-event.h>
> > > > +#include <media/videobuf2-memops.h>
> > > > +#include <media/v4l2-device.h>
> > > > +#include <media/v4l2-ioctl.h>
> > > > +
> > > > +#include "protocol.h"
> > > > +#include "session.h"
> > > > +#include "virtio_media.h"
> > > > +
> > > > +#define VIRTIO_MEDIA_NUM_EVENT_BUFS 16
> > > > +
> > > > +/* ID of the SHM region into which MMAP buffer will be mapped. */
> > > > +#define VIRTIO_MEDIA_SHM_MMAP 0
> > > > +
> > > > +/*
> > > > + * Name of the driver to expose to user-space.
> > > > + *
> > > > + * This is configurable because v4l2-compliance has workarounds specific to
> > > > + * some drivers. When proxying these directly from the host, this allows it to
> > > > + * apply them as needed.
> > > > + */
> > > > +char *virtio_media_driver_name;
> > > > +module_param_named(driver_name, virtio_media_driver_name, charp, 0660);
> > >
> > >
> > > Um. What? Not how it should be handled.
> >
> > I can remove this module param. I didn't end up using this when compliance testing.
> > Instead, I patched v4l-utils:
> > https://lore.kernel.org/all/20260528163448.4031965-1-briandaniels@google.com/
> >
> > Let me know if you think the v4l-utils patch is a good approach, otherwise let
> > me know how you'd prefer to address the v4l2-compliance driver-specific workounds
> > when they're being proxied with virtio-media.
>
> This kind of discussion should happen on a separate PR for v4l2-compliance,
> c/c to the proper developers and maintainers of it.
I wonder how migration can work when guest is tied to host driver model like
this.
> >
> > > > +
> > > > +/*
> > > > + * Whether USERPTR buffers are allowed.
> > > > + *
> > > > + * This is disabled by default as USERPTR buffers are dangerous, but the option
> > > > + * is left to enable them if desired.
> > > > + */
> > > > +bool virtio_media_allow_userptr;
> > > > +module_param_named(allow_userptr, virtio_media_allow_userptr, bool, 0660);
> > >
> > >
> > > is this kind of thing common?
>
> There is one old media device that has it (saa7134).
>
> >
> > To be honest, I don't really know. I'm also not that familiar with the USERPTR
> > issues. I see a few references online about their use being discouraged due to
> > possible race conditions, perhaps that was the original motivation for this
> > parameter (I'm not the original author of this driver).
> >
> > I'm open to alternatives, feel free to let me know if you have a preference.
>
> We tend to not implement USERPTR on newer drivers. I suggest you
> to place the logic with regards to V4L2_MEMORY_USERPTR on a separate
> patch for further discussions.
>
> >
> > > > +
> > > > +/**
> > > > + * virtio_media_session_alloc - Allocate a new session.
> > > > + * @vv: virtio-media device the session belongs to.
> > > > + * @id: ID of the session.
> > > > + * @nonblocking_dequeue: whether dequeuing of buffers should be blocking or
> > > > + * not.
> > > > + *
> > > > + * The ``id`` and ``list`` fields must still be set by the caller.
> > >
> > > still in what sense?
> >
> > Based on the code below, I'm not so sure that the caller is responsible for
> > setting these values. They seem to be initialized in the function.
> >
> > Perhaps Alexandre Courbot (the original author) would know more. Unless he
> > says otherwise though I'm inclined to remove this comment.
> >
> > > > + */
> > > > +static struct virtio_media_session *
> > > > +virtio_media_session_alloc(struct virtio_media *vv, u32 id,
> > > > + struct file *file)
> > > > +{
> > > > + struct virtio_media_session *session;
> > > > + int i;
> > > > + int ret;
> > > > +
> > > > + session = kzalloc_obj(*session, GFP_KERNEL);
> > > > + if (!session)
> > > > + goto err_session;
> > > > +
> > > > + session->shadow_buf = kzalloc(VIRTIO_SHADOW_BUF_SIZE, GFP_KERNEL);
> > > > + if (!session->shadow_buf)
> > > > + goto err_shadow_buf;
> > > > +
> > > > + ret = sg_alloc_table(&session->command_sgs, DESC_CHAIN_MAX_LEN,
> > > > + GFP_KERNEL);
> > > > + if (ret)
> > > > + goto err_payload_sgs;
> > > > +
> > > > + session->id = id;
> > > > + session->nonblocking_dequeue = file->f_flags & O_NONBLOCK;
> > > > +
> > > > + INIT_LIST_HEAD(&session->list);
> > > > + v4l2_fh_init(&session->fh, &vv->video_dev);
> > > > + virtio_media_session_fh_add(session, file);
> > > > +
> > > > + for (i = 0; i <= VIRTIO_MEDIA_LAST_QUEUE; i++)
> > > > + INIT_LIST_HEAD(&session->queues[i].pending_dqbufs);
> > > > + mutex_init(&session->queues_lock);
> > > > +
> > > > + init_waitqueue_head(&session->dqbuf_wait);
> > > > +
> > > > + mutex_lock(&vv->sessions_lock);
> > > > + list_add_tail(&session->list, &vv->sessions);
> > > > + mutex_unlock(&vv->sessions_lock);
> > > > +
> > > > + return session;
> > > > +
> > > > +err_payload_sgs:
> > > > + kfree(session->shadow_buf);
> > > > +err_shadow_buf:
> > > > + kfree(session);
> > > > +err_session:
> > > > + return ERR_PTR(-ENOMEM);
> > > > +}
> > > > +
> > > > +/**
> > > > + * virtio_media_session_free - Free all resources of a session.
> > > > + * @vv: virtio-media device the session belongs to.
> > > > + * @session: session to destroy.
> > > > + *
> > > > + * All the resources of @sesssion, as well as the backing memory of @session
> > > > + * itself, are freed.
> > >
> > > why @ here and `` above? And typo in the name.
> >
> > The `@` here was an attempt to follow the guide here for referencing function
> > parameters:
> > https://docs.kernel.org/doc-guide/kernel-doc.html#highlights-and-cross-references
>
> Yes. This is part of Linux Kernel kernel-doc markup: when referring to
> struct fields, you should use @field (or ``field`` if one wants to place an
> asterisk on it, like ``*field``).
>
> >
> > That being said, I don't believe this file is 100% consistent with that. I will
> > spend some time cleaning up the comments throughout this patch set to get them
> > consistent for v5. Thanks!
>
> Please use it on a consistent way along the driver.
>
>
> Thanks,
> Mauro
^ permalink raw reply
* Re: [PATCH v4 2/8] media: virtio: Add virtio-media driver structs and function declarations
From: Michael S. Tsirkin @ 2026-07-18 17:27 UTC (permalink / raw)
To: Brian Daniels
Cc: Mauro Carvalho Chehab, acourbot, adelva, aesteve, changyeon,
daniel.almeida, eperezma, gnurou, gurchetansingh, hverkuil,
jasowang, linux-kernel, linux-media, nicolas.dufresne,
virtualization, xuanzhuo
In-Reply-To: <CAD4i_GTBkZRAnBTohkzJSh2XTx+MHkTS_KokEMQ4s0-WP+DxcA@mail.gmail.com>
On Fri, Jul 17, 2026 at 03:46:36PM -0400, Brian Daniels wrote:
> On Mon, Jun 22, 2026 at 5:08 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Jun 22, 2026 at 04:43:37PM -0400, Brian Daniels wrote:
> > > From: Alexandre Courbot <gnurou@gmail.com>
> > >
> > > Add the structs and function declarations for the new virtio-media drvier.
> > >
> > > Signed-off-by: Alexandre Courbot <gnurou@gmail.com>
> > > Co-developed-by: Brian Daniels <briandaniels@google.com>
> > > Signed-off-by: Brian Daniels <briandaniels@google.com>
> > > ---
> > > drivers/media/virtio/virtio_media.h | 95 +++++++++++++++++++++++++++++
> > > 1 file changed, 95 insertions(+)
> > > create mode 100644 drivers/media/virtio/virtio_media.h
> > >
> > > diff --git a/drivers/media/virtio/virtio_media.h b/drivers/media/virtio/virtio_media.h
> > > new file mode 100644
> > > index 000000000..52809d4e9
> > > --- /dev/null
> > > +++ b/drivers/media/virtio/virtio_media.h
> > > @@ -0,0 +1,95 @@
> > > +/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0+ */
> > > +
> > > +/*
> > > + * Virtio-media structures & functions declarations.
> > > + *
> > > + * Copyright (c) 2024-2025 Google LLC.
> > > + */
> > > +
> > > +#ifndef __VIRTIO_MEDIA_H
> > > +#define __VIRTIO_MEDIA_H
> > > +
> > > +#include <linux/virtio_config.h>
> > > +#include <media/v4l2-device.h>
> > > +
> > > +#include "protocol.h"
> > > +
> > > +#define DESC_CHAIN_MAX_LEN SG_MAX_SINGLE_ALLOC
> > > +
> > > +#define VIRTIO_MEDIA_DEFAULT_DRIVER_NAME "virtio-media"
> > > +
> > > +extern char *virtio_media_driver_name;
> > > +extern bool virtio_media_allow_userptr;
> > > +
> > > +/**
> > > + * struct virtio_media - Virtio-media device.
> > > + * @v4l2_dev: v4l2_device for the media device.
> > > + * @video_dev: video_device for the media device.
> > > + * @virtio_dev: virtio device for the media device.
> > > + * @commandq: virtio command queue.
> > > + * @eventq: virtio event queue.
> > > + * @eventq_work: work to run when events are received on @eventq.
> > > + * @mmap_region: region into which MMAP buffers are mapped by the host.
> > > + * @event_buffer: buffer for event descriptors.
> > > + * @sessions: list of active sessions on the device.
> > > + * @sessions_lock: protects @sessions and ``virtio_media_session::list``.
> > > + * @events_lock: prevents concurrent processing of events.
> > > + * @cmd: union of the device commands "open" and "munmap". The other
> > > + * commands are handled by @struct virtio_media_session
> > > + * @resp: union of responses.to device commands "open" and "munmap". The
> > > + * other responses are handled by @struct virtio_media_session
> > > + * @vlock: serializes access to the command queue.
> > > + * @wq: waitqueue for host responses on the command queue.
> > > + */
> > > +struct virtio_media {
> > > + struct v4l2_device v4l2_dev;
> > > + struct video_device video_dev;
> > > +
> > > + struct virtio_device *virtio_dev;
> > > + struct virtqueue *commandq;
> > > + struct virtqueue *eventq;
> > > + struct work_struct eventq_work;
> > > +
> > > + struct virtio_shm_region mmap_region;
> > > +
> > > + void *event_buffer;
> > > +
> > > + struct list_head sessions;
> > > + struct mutex sessions_lock; /* protects sessions list */
> > > +
> > > + struct mutex events_lock; /* prevents concurrent event processing */
> > > +
> > > + union {
> > > + struct virtio_media_cmd_open open;
> > > + struct virtio_media_cmd_munmap munmap;
> > > + } cmd;
> > > +
> > > + union {
> > > + struct virtio_media_resp_open open;
> > > + struct virtio_media_resp_munmap munmap;
> > > + } resp;
> >
> >
> > You need DMA alignment padding for these things.
>
> Each member of the above unions is already padded to 64-bit alignment,
> is that sufficient?
>
> If not, could you tell me what else is necessary?
Read up "What memory is DMA'able?" and following chapters in
Documentation/core-api/dma-api-howto.rst
> > Which one can only see when I reads the actual driver 8 patches down.
> > Which is why it's not a sensible way to split patches.
> >
> > A sensible way is to have a driver then add functionality
> > in logical pieces gradually.
> >
>
> Appreciate the feedback. There was a previous comment requesting to
> split the patch into c/h file pairs, but I can try reformatting it as
> you suggest for v5.
>
> > > +
> > > + struct mutex vlock; /* serializes command queue access */
> > > + wait_queue_head_t wq;
> > > +};
> > > +
> > > +static inline struct virtio_media *
> > > +to_virtio_media(struct video_device *video_dev)
> > > +{
> > > + return container_of(video_dev, struct virtio_media, video_dev);
> > > +}
> > > +
> > > +/* virtio_media_driver.c */
> > > +
> > > +int virtio_media_send_command(struct virtio_media *vv, struct scatterlist **sgs,
> > > + const size_t out_sgs, const size_t in_sgs,
> > > + size_t minimum_resp_len, size_t *resp_len);
> > > +void virtio_media_process_events(struct virtio_media *vv);
> > > +
> > > +/* virtio_media_ioctls.c */
> > > +
> > > +long virtio_media_device_ioctl(struct file *file, unsigned int cmd,
> > > + unsigned long arg);
> > > +extern const struct v4l2_ioctl_ops virtio_media_ioctl_ops;
> > > +
> > > +#endif // __VIRTIO_MEDIA_H
> > > --
> > > 2.55.0.rc0.799.gd6f94ed593-goog
> >
^ permalink raw reply
* Re: [PATCH v2 1/4] virtio-mem: validate device-reported block size
From: Michael S. Tsirkin @ 2026-07-18 17:21 UTC (permalink / raw)
To: Carlos Bilbao
Cc: Greg Kroah-Hartman, David Hildenbrand (Arm), Hari Mishal,
Jason Wang, Xuan Zhuo, Eugenio Pérez, virtualization,
linux-kernel, elena.reshetova, huster, mhollick, jiska.classen
In-Reply-To: <ae5bc86b-cb8d-4530-9e58-d285007deac0@gmail.com>
On Sat, Jul 18, 2026 at 10:07:30AM -0700, Carlos Bilbao wrote:
> On 7/17/26 22:29, Greg Kroah-Hartman wrote:
>
> > On Fri, Jul 17, 2026 at 08:31:09PM -0700, Carlos Bilbao wrote:
> > > Historically, one of the biggest criticisms of coco, especially around
> > > device hardening, was that there were too many values that a
> > > malicious/buggy device could misreport, making it a losing battle. That is
> > > no longer the case with LLMs, and we have the advantage (and challenge) of
> > > open-source dev, which allows us to receive many of these fixes "for free".
> > > If others want to burn their tokens, let them :)
> > I have lots of tokens to burn :)
> >
> > So along those lines, any suggestions on how best to fuzz these code
> > paths? Any workloads you all use for testing that I can take advantage
> > of?
>
>
> We've the virtio-mem config struct layout and the kernel source, so for
> obvious fixes like a NULL check, static analysis is better than fuzzing.
> Claude took a few mins to find me two examples:
>
> Patch 1: virtio-mem: reject non-power-of-two device_block_size
> This one is for virtio_mem_init() to check if
> !is_power_of_2(vm->device_block_size)
>
> Patch 2: virto-mem: validate region_size and usable_region_size
> THis one checks region_size != 0 and vm->usable_reion_size >
> vm->region_size.
>
> An endless factory of "silly" checks like these are low hanging fruit.
At the same time, these checks don't actually help within the coco
threat model, do they?
> Now, for harder bugs, looking around for fuzz options, VirtFuzz [1] looks
> like a great candidate for those interested in pursuing this direction.
>
>
> Their PoC fuzzes wireless/Bluetooth stack, but nothing our AI overlords
> can't quickly adapt for virtio-mem and other virtio drivers; the JSON
> definition to describe device behavior is easily extensible. Their threat
> model [2] describes an external attacker, but in the context of coco, the
> virtio device itself is the attacker.
What we need, however, is to exclude DoS attacks - these are outside the
threat model. If people try to address all DoS attacks uncritically we
just get a churn of changes which just might introduce issues of their
own.
Example:
BUG_ON(!is_power_of_2(....));
panics, non exploitable.
if(!is_power_of_2(....))
goto error;
can become exploitable if the cleanup is done wrong.
> Here's a vibe coded PR of what I mean:
>
> https://github.com/seemoo-lab/VirtFuzz/pull/7
>
> CCed the creators/authors, thanks for open sourcing this!
>
> Thanks,
> Carlos
>
> [1] https://github.com/seemoo-lab/VirtFuzz
>
> On 7/17/26 22:29, Greg Kroah-Hartman wrote:
>
> > On Fri, Jul 17, 2026 at 08:31:09PM -0700, Carlos Bilbao wrote:
> > > Historically, one of the biggest criticisms of coco, especially around
> > > device hardening, was that there were too many values that a
> > > malicious/buggy device could misreport, making it a losing battle. That is
> > > no longer the case with LLMs, and we have the advantage (and challenge) of
> > > open-source dev, which allows us to receive many of these fixes "for free".
> > > If others want to burn their tokens, let them :)
> > I have lots of tokens to burn :)
> >
> > So along those lines, any suggestions on how best to fuzz these code
> > paths? Any workloads you all use for testing that I can take advantage
> > of?
>
>
> We've the virto-mem config struct layout and the kernel source, so for
> obvious fixes like a NULL check, static analysis is better than fuzzing.
> Claude took a few mins to find me two examples:
>
> Patch 1: virtio-mem: reject non-power-of-two device_block_size
> This one is for virtio_mem_init() to check if
> !is_power_of_2(vm->device_block_size)
>
> Patch 2: virto-mem: validate region_size and usable_region_size
> THis one checks region_size != 0 and vm->usable_reion_size >
> vm->region_size.
>
> An endless factory of "silly" checks like these are low hanging fruit.
>
> Now, for harder bugs, looking around for fuzz options, VirtFuzz [1] looks
> like a great candidate for those interested in pursuing this direction.
>
>
> Their PoC fuzzes wireless/Bluetooth stack, but nothing our AI overlords
> can't quickly adapt for virtio-mem and other virtio drivers; the JSON
> definition to describe device behavior is easily extensible. Their threat
> model [2] describes an external attacker, but in the context of coco, the
> virtio device itself is the attacker. Here's a vibe coded PR of what I mean:
>
> https://github.com/seemoo-lab/VirtFuzz/pull/7
>
> CCed the creators/authors, thanks for open sourcing this!
>
> Thanks,
> Carlos
>
> [1] https://github.com/seemoo-lab/VirtFuzz
> [2] https://www.computer.org/csdl/proceedings-article/sp/2024/313000a024/1RjEa0y9RMQ
>
>
> > thanks,
> >
> > greg k-h
>
> [2] https://www.computer.org/csdl/proceedings-article/sp/2024/313000a024/1RjEa0y9RMQ
>
>
> >
> > thanks,
> >
> > greg k-h
^ permalink raw reply
* Re: [PATCH v2 1/4] virtio-mem: validate device-reported block size
From: Carlos Bilbao @ 2026-07-18 17:07 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Michael S. Tsirkin, David Hildenbrand (Arm), Hari Mishal,
Jason Wang, Xuan Zhuo, Eugenio Pérez, virtualization,
linux-kernel, elena.reshetova, huster, mhollick, jiska.classen
In-Reply-To: <2026071803-passage-dares-8240@gregkh>
On 7/17/26 22:29, Greg Kroah-Hartman wrote:
> On Fri, Jul 17, 2026 at 08:31:09PM -0700, Carlos Bilbao wrote:
>> Historically, one of the biggest criticisms of coco, especially around
>> device hardening, was that there were too many values that a
>> malicious/buggy device could misreport, making it a losing battle. That is
>> no longer the case with LLMs, and we have the advantage (and challenge) of
>> open-source dev, which allows us to receive many of these fixes "for free".
>> If others want to burn their tokens, let them :)
> I have lots of tokens to burn :)
>
> So along those lines, any suggestions on how best to fuzz these code
> paths? Any workloads you all use for testing that I can take advantage
> of?
We've the virtio-mem config struct layout and the kernel source, so for
obvious fixes like a NULL check, static analysis is better than fuzzing.
Claude took a few mins to find me two examples:
Patch 1: virtio-mem: reject non-power-of-two device_block_size
This one is for virtio_mem_init() to check if
!is_power_of_2(vm->device_block_size)
Patch 2: virto-mem: validate region_size and usable_region_size
THis one checks region_size != 0 and vm->usable_reion_size >
vm->region_size.
An endless factory of "silly" checks like these are low hanging fruit.
Now, for harder bugs, looking around for fuzz options, VirtFuzz [1] looks
like a great candidate for those interested in pursuing this direction.
Their PoC fuzzes wireless/Bluetooth stack, but nothing our AI overlords
can't quickly adapt for virtio-mem and other virtio drivers; the JSON
definition to describe device behavior is easily extensible. Their threat
model [2] describes an external attacker, but in the context of coco, the
virtio device itself is the attacker. Here's a vibe coded PR of what I mean:
https://github.com/seemoo-lab/VirtFuzz/pull/7
CCed the creators/authors, thanks for open sourcing this!
Thanks,
Carlos
[1] https://github.com/seemoo-lab/VirtFuzz
On 7/17/26 22:29, Greg Kroah-Hartman wrote:
> On Fri, Jul 17, 2026 at 08:31:09PM -0700, Carlos Bilbao wrote:
>> Historically, one of the biggest criticisms of coco, especially around
>> device hardening, was that there were too many values that a
>> malicious/buggy device could misreport, making it a losing battle. That is
>> no longer the case with LLMs, and we have the advantage (and challenge) of
>> open-source dev, which allows us to receive many of these fixes "for free".
>> If others want to burn their tokens, let them :)
> I have lots of tokens to burn :)
>
> So along those lines, any suggestions on how best to fuzz these code
> paths? Any workloads you all use for testing that I can take advantage
> of?
We've the virto-mem config struct layout and the kernel source, so for
obvious fixes like a NULL check, static analysis is better than fuzzing.
Claude took a few mins to find me two examples:
Patch 1: virtio-mem: reject non-power-of-two device_block_size
This one is for virtio_mem_init() to check if
!is_power_of_2(vm->device_block_size)
Patch 2: virto-mem: validate region_size and usable_region_size
THis one checks region_size != 0 and vm->usable_reion_size >
vm->region_size.
An endless factory of "silly" checks like these are low hanging fruit.
Now, for harder bugs, looking around for fuzz options, VirtFuzz [1] looks
like a great candidate for those interested in pursuing this direction.
Their PoC fuzzes wireless/Bluetooth stack, but nothing our AI overlords
can't quickly adapt for virtio-mem and other virtio drivers; the JSON
definition to describe device behavior is easily extensible. Their threat
model [2] describes an external attacker, but in the context of coco, the
virtio device itself is the attacker. Here's a vibe coded PR of what I mean:
https://github.com/seemoo-lab/VirtFuzz/pull/7
CCed the creators/authors, thanks for open sourcing this!
Thanks,
Carlos
[1] https://github.com/seemoo-lab/VirtFuzz
[2]
https://www.computer.org/csdl/proceedings-article/sp/2024/313000a024/1RjEa0y9RMQ
> thanks,
>
> greg k-h
[2]
https://www.computer.org/csdl/proceedings-article/sp/2024/313000a024/1RjEa0y9RMQ
>
> thanks,
>
> greg k-h
^ permalink raw reply
* Re: [PATCH v2 1/4] virtio-mem: validate device-reported block size
From: Greg Kroah-Hartman @ 2026-07-18 5:29 UTC (permalink / raw)
To: Carlos Bilbao
Cc: Michael S. Tsirkin, David Hildenbrand (Arm), Hari Mishal,
Jason Wang, Xuan Zhuo, Eugenio Pérez, virtualization,
linux-kernel, elena.reshetova
In-Reply-To: <1fe328d1-edf9-4e72-a145-be74ede20e60@gmail.com>
On Fri, Jul 17, 2026 at 08:31:09PM -0700, Carlos Bilbao wrote:
> Historically, one of the biggest criticisms of coco, especially around
> device hardening, was that there were too many values that a
> malicious/buggy device could misreport, making it a losing battle. That is
> no longer the case with LLMs, and we have the advantage (and challenge) of
> open-source dev, which allows us to receive many of these fixes "for free".
> If others want to burn their tokens, let them :)
I have lots of tokens to burn :)
So along those lines, any suggestions on how best to fuzz these code
paths? Any workloads you all use for testing that I can take advantage
of?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] vhost-scsi: Prevent OOM from invalid protection SGL count
From: Jia Jia @ 2026-07-18 4:14 UTC (permalink / raw)
To: bvel, rtik, efwo, kodw, michael.christie
Cc: slf, mst, jasowang, pbonzini, stefanha, eperezma, virtualization,
kvm, linux-kernel
In-Reply-To: <8f5b5350-0be9-4b79-a6c9-069d418dee30@oracle.com>
Before sending my patch, I did a duplicate check against the upstream
linux-next tree, upstream history, publicly searchable mailing-list archives,
and Patchwork. I searched using the vhost-scsi and scatterlist function names,
T10-PI, invalid protection SGL counts, and the OOM/resource-exhaustion
symptoms. I did not find Linfeng's earlier patch.
Could you please point me to Linfeng's patch, or confirm what the additional
check covers? In particular, does it also validate:
prot_bytes <= exp_data_len
before subtracting prot_bytes from exp_data_len and entering the SGL
mapping path? I had identified this as a related PI length-mismatch path, but
did not include it in the patch I sent.
With a one-byte combined payload and
pi_bytesout=2, KGDB stopped at the following call stack:
#0 sg_alloc_table_chained(
table=0xffff88810bbc5108,
nents=0,
first_chunk=0xffff888110050000,
nents_first_chunk=2048)
at lib/sg_pool.c:117
#1 vhost_scsi_handle_vq+2295
#2 vhost worker task
#3 srso_alias_return_thunk
The stop was at the `BUG_ON(!nents)` instruction in
`sg_alloc_table_chained()`, and the kernel log recorded `kernel BUG at
lib/sg_pool.c:117!`. This confirms that the PI length check is needed before
the iterator adjustment and SGL mapping path.
If Linfeng's patch covers both the invalid protection SGL count and this PI
length check, I will not submit duplicate work.
Regarding the AI question: AI assistance was used during the source analysis
and test development. I also checked the relevant code path and the
host-side behavior.
^ permalink raw reply
* Re: [PATCH v2 1/4] virtio-mem: validate device-reported block size
From: Carlos Bilbao @ 2026-07-18 3:31 UTC (permalink / raw)
To: Michael S. Tsirkin, Greg Kroah-Hartman
Cc: David Hildenbrand (Arm), Hari Mishal, Jason Wang, Xuan Zhuo,
Eugenio Pérez, virtualization, linux-kernel, elena.reshetova
In-Reply-To: <20260717085838-mutt-send-email-mst@kernel.org>
Hey there,
On 7/17/26 06:08, Michael S. Tsirkin wrote:
> On Fri, Jul 17, 2026 at 02:07:50PM +0200, Greg Kroah-Hartman wrote:
>> On Fri, Jul 17, 2026 at 06:52:46AM -0400, Michael S. Tsirkin wrote:
>>> On Fri, Jul 17, 2026 at 12:46:52PM +0200, Greg Kroah-Hartman wrote:
>>>> On Fri, Jul 17, 2026 at 06:23:57AM -0400, Michael S. Tsirkin wrote:
>>>>> On Fri, Jul 17, 2026 at 12:15:09PM +0200, Greg Kroah-Hartman wrote:
>>>>>> On Fri, Jul 17, 2026 at 06:10:41AM -0400, Michael S. Tsirkin wrote:
>>>>>>> On Fri, Jul 17, 2026 at 11:14:23AM +0200, Greg Kroah-Hartman wrote:
>>>>>>>> On Fri, Jul 17, 2026 at 04:59:32AM -0400, Michael S. Tsirkin wrote:
>>>>>>>>> On Fri, Jul 17, 2026 at 10:39:40AM +0200, David Hildenbrand (Arm) wrote:
>>>>>>>>>> On 7/17/26 07:48, Michael S. Tsirkin wrote:
>>>>>>>>>>> On Thu, Jul 16, 2026 at 05:59:05PM +0200, David Hildenbrand (Arm) wrote:
>>>>>>>>>>>>> Or do we just always trust virtio mem devices explicitly?
>>>>>>>>>>>> It's hard for me to understand where we draw the line, really.
>>>>>>>>>>>>
>>>>>>>>>>>> But maybe MST can clarify what we care about in virtio world where the
>>>>>>>>>>>> hypervisor is fully in charge of the device,
>>>>>>>>>>> Generally:
>>>>>>>>>>> - The guest is expected to whitelist drivers (most drivers have not
>>>>>>>>>>> been audited).
>>>>>>>>>> But even if you audited your driver, who makes sure that we consider all ways
>>>>>>>>>> where the device could mess with us?
>>>>>>>>> A lot of this is up to a correct setup. For example, make sure all
>>>>>>>>> filesystems are encrypted and refuse to mount unencrypted ones.
>>>>>>>>>
>>>>>>>>>> Something feels off here.
>>>>>>>>>>
>>>>>>>>>> Handling selected out-of-spec scenarios like this feels like a band-aid. Happy
>>>>>>>>>> to be corrected.
>>>>>>>>> Well Documentation/security/snp-tdx-threat-model.rst puts it like this:
>>>>>>>>> It is important to note
>>>>>>>>> that this doesn’t imply that the host or VMM are intentionally
>>>>>>>>> malicious, but that there exists a security value in having a small CoCo
>>>>>>>>> VM TCB.
>>>>>>>>>
>>>>>>>>> and
>>>>>>>>>
>>>>>>>>> While traditionally the host has unlimited access to guest data and can
>>>>>>>>> leverage this access to attack the guest, the CoCo systems mitigate such
>>>>>>>>> attacks by adding security features like guest data confidentiality and
>>>>>>>>> integrity protection.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> now, when we are talking about "mitigation" it is indeed becoming a bit
>>>>>>>>> murky.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> For me, a rule of thumb I came up with is that if the validation happens
>>>>>>>>> to also be helful for users e.g. to work around buggy devices,
>>>>>>>>> or maybe because we feel failing gracefully is nice because this
>>>>>>>>> will allow to later make use of this config and old drivers will
>>>>>>>>> fail but at least not panic, then it is good to include.
>>>>>>>> Why not do what USB does? Don't trust the device until AFTER probe()
>>>>>>>> succeeds? All of the needed checking should happen before then, as that
>>>>>>>> is a "slow path" so lots of validation and the like can happen at that
>>>>>>>> point.
>>>>>>>>
>>>>>>>> After that, during the normal data paths, after the driver is bound,
>>>>>>>> trust it all you want as attempting to validate every single packet is
>>>>>>>> just going to be impossible.
>>>>>>>>
>>>>>>>> thanks,
>>>>>>>>
>>>>>>>> greg k-h
>>>>>>> People do expect that data path validation at this point.
>>>>>> Ok, so you want this patch :)
>>>>>>
>>>>>> And more, as you need to treat everything from the host as "untrusted",
>>>>>> and it must be "verified".
>>>>> Well. First it's not me) Second it's only specific configurations -
>>>>> for example there's no short term plan to validate filesystem code, people
>>>>> are expected to rely on encryption. The reasons have more to do
>>>>> with the available manpower than anything else.
>>>> Sure, but again, for subsystems, you have to define your threat model as
>>>> the LLMs are churning against the code base and coming up with lots of
>>>> crazy ideas if a device should or should not be trusted and spitting out
>>>> patches and reports like the ones that are in the first few patches of
>>>> this series.
>>>>
>>>> So please, pick a model, let's document it, and go with that. I am
>>>> getting directly conflicting responses here.
>>>>
>>>> thanks,
>>>>
>>>> greg k-h
>>> Supposed to be this one:
>>> Documentation/security/snp-tdx-threat-model.rst
>>>
>>> what is missing?
>> A policy decision that needs to be made. All that document does is
>> describe a bunch of different "threats" yet does not decide what to do
>> about them at all from what I can tell.
> That would be this section I think:
>
> The **Linux kernel CoCo VM security objectives** can be summarized as follows:
>
> it does, indeed, not go into detail about how to interact, safely,
> with untrusted entities. Does it really need to be spelled out?
>
>> And that's just for one subset of the CoC world, right? Is that
>> something that all virtio drivers need/want to care about?
> What is missing, and what you seem to be asking for, is an opinionated
> stance on which drivers we care about in this world?
> True.
> coco guys tried to annotate drivers at some point to do exactly that.
> this was rejected upstream from the position that this is not
> different from handling buggy hardware, and just to fix all drivers.
> so it's up to users, and I guess for virtio the answer is yes
> with some exceptions because we don't have a better answer right now.
>
>> So I don't see a real answer to the "does Linux trust the host to give
>> you good data or not" question in that file, am I missing it?
>>
>> thanks,
>>
>> greg k-h
> This? Note the last sentence.
>
> The **Linux CoCo VM attack surface** is any interface exposed from a CoCo
> guest Linux kernel towards an untrusted host that is not covered by the
> CoCo technology SW/HW protection. This includes any possible
> side-channels, as well as transient execution side channels. Examples of
> explicit (not side-channel) interfaces include accesses to port I/O, MMIO
> and DMA interfaces, access to PCI configuration space, VMM-specific
> hypercalls (towards Host-side VMM), access to shared memory pages,
> interrupts allowed to be injected into the guest kernel by the host, as
> well as CoCo technology-specific hypercalls, if present. Additionally, the
> host in a CoCo system typically controls the process of creating a CoCo
> guest: it has a method to load into a guest the firmware and bootloader
> images, the kernel image together with the kernel command line. All of this
> data should also be considered untrusted until its integrity and
> authenticity is established via attestation.
I'm glad you're finding this document helpful, it took us massive
back-and-forth to get somewhere everyone was happy.
Here's my 2 cents on this debate, if I may. I think defensive programming
is always a positive, and we don't just say, "the spec disallows it".
Historically, one of the biggest criticisms of coco, especially around
device hardening, was that there were too many values that a
malicious/buggy device could misreport, making it a losing battle. That is
no longer the case with LLMs, and we have the advantage (and challenge) of
open-source dev, which allows us to receive many of these fixes "for free".
If others want to burn their tokens, let them :)
Thanks,
Carlos
>
>
>
^ permalink raw reply
* Re: [PATCH v4 6/8] media: virtio: Add virtio_media_driver
From: Brian Daniels @ 2026-07-17 20:58 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Mauro Carvalho Chehab, acourbot, adelva, aesteve, changyeon,
daniel.almeida, eperezma, gnurou, gurchetansingh, hverkuil,
jasowang, linux-kernel, linux-media, nicolas.dufresne,
virtualization, xuanzhuo
In-Reply-To: <20260625201850.2981130-1-briandaniels@google.com>
On Thu, Jun 25, 2026 at 4:18 PM Brian Daniels <briandaniels@google.com> wrote:
>
> > > From: Alexandre Courbot <gnurou@gmail.com>
> > >
> > > virtio_media_driver.c provides the expected driver hooks, and support
> > > for mmapping and polling.
> > >
> > > Signed-off-by: Alexandre Courbot <gnurou@gmail.com>
> > > Co-developed-by: Brian Daniels <briandaniels@google.com>
> > > Signed-off-by: Brian Daniels <briandaniels@google.com>
> > > ---
> > > drivers/media/virtio/virtio_media_driver.c | 959 +++++++++++++++++++++
> > > 1 file changed, 959 insertions(+)
> > > create mode 100644 drivers/media/virtio/virtio_media_driver.c
> > >
> > > diff --git a/drivers/media/virtio/virtio_media_driver.c b/drivers/media/virtio/virtio_media_driver.c
> > > new file mode 100644
> > > index 000000000..d6363c673
> > > --- /dev/null
> > > +++ b/drivers/media/virtio/virtio_media_driver.c
> > > @@ -0,0 +1,959 @@
> > > +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0+
> > > +
> > > +/*
> > > + * Virtio-media driver.
> > > + *
> > > + * Copyright (c) 2024-2025 Google LLC.
> > > + */
> > > +
> > > +#include <linux/delay.h>
> > > +#include <linux/device.h>
> > > +#include <linux/dev_printk.h>
> > > +#include <linux/mm.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/scatterlist.h>
> > > +#include <linux/types.h>
> > > +#include <linux/videodev2.h>
> > > +#include <linux/vmalloc.h>
> > > +#include <linux/wait.h>
> > > +#include <linux/workqueue.h>
> > > +#include <linux/module.h>
> > > +#include <linux/moduleparam.h>
> > > +#include <linux/virtio.h>
> > > +#include <linux/virtio_config.h>
> > > +#include <linux/virtio_ids.h>
> > > +
> > > +#include <media/frame_vector.h>
> > > +#include <media/v4l2-dev.h>
> > > +#include <media/v4l2-event.h>
> > > +#include <media/videobuf2-memops.h>
> > > +#include <media/v4l2-device.h>
> > > +#include <media/v4l2-ioctl.h>
> > > +
> > > +#include "protocol.h"
> > > +#include "session.h"
> > > +#include "virtio_media.h"
> > > +
> > > +#define VIRTIO_MEDIA_NUM_EVENT_BUFS 16
> > > +
> > > +/* ID of the SHM region into which MMAP buffer will be mapped. */
> > > +#define VIRTIO_MEDIA_SHM_MMAP 0
> > > +
> > > +/*
> > > + * Name of the driver to expose to user-space.
> > > + *
> > > + * This is configurable because v4l2-compliance has workarounds specific to
> > > + * some drivers. When proxying these directly from the host, this allows it to
> > > + * apply them as needed.
> > > + */
> > > +char *virtio_media_driver_name;
> > > +module_param_named(driver_name, virtio_media_driver_name, charp, 0660);
> >
> >
> > Um. What? Not how it should be handled.
>
> I can remove this module param. I didn't end up using this when compliance testing.
> Instead, I patched v4l-utils:
> https://lore.kernel.org/all/20260528163448.4031965-1-briandaniels@google.com/
>
> Let me know if you think the v4l-utils patch is a good approach, otherwise let
> me know how you'd prefer to address the v4l2-compliance driver-specific workounds
> when they're being proxied with virtio-media.
>
> > > +
> > > +/*
> > > + * Whether USERPTR buffers are allowed.
> > > + *
> > > + * This is disabled by default as USERPTR buffers are dangerous, but the option
> > > + * is left to enable them if desired.
> > > + */
> > > +bool virtio_media_allow_userptr;
> > > +module_param_named(allow_userptr, virtio_media_allow_userptr, bool, 0660);
> >
> >
> > is this kind of thing common?
>
> To be honest, I don't really know. I'm also not that familiar with the USERPTR
> issues. I see a few references online about their use being discouraged due to
> possible race conditions, perhaps that was the original motivation for this
> parameter (I'm not the original author of this driver).
>
> I'm open to alternatives, feel free to let me know if you have a preference.
>
> > > +
> > > +/**
> > > + * virtio_media_session_alloc - Allocate a new session.
> > > + * @vv: virtio-media device the session belongs to.
> > > + * @id: ID of the session.
> > > + * @nonblocking_dequeue: whether dequeuing of buffers should be blocking or
> > > + * not.
> > > + *
> > > + * The ``id`` and ``list`` fields must still be set by the caller.
> >
> > still in what sense?
>
> Based on the code below, I'm not so sure that the caller is responsible for
> setting these values. They seem to be initialized in the function.
>
> Perhaps Alexandre Courbot (the original author) would know more. Unless he
> says otherwise though I'm inclined to remove this comment.
>
> > > + */
> > > +static struct virtio_media_session *
> > > +virtio_media_session_alloc(struct virtio_media *vv, u32 id,
> > > + struct file *file)
> > > +{
> > > + struct virtio_media_session *session;
> > > + int i;
> > > + int ret;
> > > +
> > > + session = kzalloc_obj(*session, GFP_KERNEL);
> > > + if (!session)
> > > + goto err_session;
> > > +
> > > + session->shadow_buf = kzalloc(VIRTIO_SHADOW_BUF_SIZE, GFP_KERNEL);
> > > + if (!session->shadow_buf)
> > > + goto err_shadow_buf;
> > > +
> > > + ret = sg_alloc_table(&session->command_sgs, DESC_CHAIN_MAX_LEN,
> > > + GFP_KERNEL);
> > > + if (ret)
> > > + goto err_payload_sgs;
> > > +
> > > + session->id = id;
> > > + session->nonblocking_dequeue = file->f_flags & O_NONBLOCK;
> > > +
> > > + INIT_LIST_HEAD(&session->list);
> > > + v4l2_fh_init(&session->fh, &vv->video_dev);
> > > + virtio_media_session_fh_add(session, file);
> > > +
> > > + for (i = 0; i <= VIRTIO_MEDIA_LAST_QUEUE; i++)
> > > + INIT_LIST_HEAD(&session->queues[i].pending_dqbufs);
> > > + mutex_init(&session->queues_lock);
> > > +
> > > + init_waitqueue_head(&session->dqbuf_wait);
> > > +
> > > + mutex_lock(&vv->sessions_lock);
> > > + list_add_tail(&session->list, &vv->sessions);
> > > + mutex_unlock(&vv->sessions_lock);
> > > +
> > > + return session;
> > > +
> > > +err_payload_sgs:
> > > + kfree(session->shadow_buf);
> > > +err_shadow_buf:
> > > + kfree(session);
> > > +err_session:
> > > + return ERR_PTR(-ENOMEM);
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_session_free - Free all resources of a session.
> > > + * @vv: virtio-media device the session belongs to.
> > > + * @session: session to destroy.
> > > + *
> > > + * All the resources of @sesssion, as well as the backing memory of @session
> > > + * itself, are freed.
> >
> > why @ here and `` above? And typo in the name.
>
> The `@` here was an attempt to follow the guide here for referencing function
> parameters:
> https://docs.kernel.org/doc-guide/kernel-doc.html#highlights-and-cross-references
>
> That being said, I don't believe this file is 100% consistent with that. I will
> spend some time cleaning up the comments throughout this patch set to get them
> consistent for v5. Thanks!
>
> > > + */
> > > +static void virtio_media_session_free(struct virtio_media *vv,
> > > + struct virtio_media_session *session)
> > > +{
> > > + int i;
> > > +
> > > + mutex_lock(&vv->sessions_lock);
> > > + list_del(&session->list);
> > > + mutex_unlock(&vv->sessions_lock);
> > > +
> > > + virtio_media_session_fh_del(session);
> > > + v4l2_fh_exit(&session->fh);
> > > +
> > > + sg_free_table(&session->command_sgs);
> > > +
> > > + for (i = 0; i <= VIRTIO_MEDIA_LAST_QUEUE; i++)
> > > + vfree(session->queues[i].buffers);
> > > +
> > > + kfree(session->shadow_buf);
> > > + kfree(session);
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_session_close - Close and free a session.
> > > + * @vv: virtio-media device the session belongs to.
> > > + * @session: session to close and destroy.
> > > + *
> > > + * This send the ``VIRTIO_MEDIA_CMD_CLOSE`` command to the device, and frees
> >
> > sends
>
> Fix staged in v5
>
> > > + * all resources used by @session.
> > > + */
> > > +static int virtio_media_session_close(struct virtio_media *vv,
> > > + struct virtio_media_session *session)
> > > +{
> > > + struct virtio_media_cmd_close *cmd_close = &session->cmd.close;
> > > + struct scatterlist cmd_sg = {};
> > > + struct scatterlist *sgs[1] = { &cmd_sg };
> > > + int ret;
> > > +
> > > + mutex_lock(&vv->vlock);
> > > +
> > > + cmd_close->hdr.cmd = VIRTIO_MEDIA_CMD_CLOSE;
> > > + cmd_close->session_id = session->id;
> > > +
> > > + sg_set_buf(&cmd_sg, cmd_close, sizeof(*cmd_close));
> > > + sg_mark_end(&cmd_sg);
> > > +
> > > + ret = virtio_media_send_command(vv, sgs, 1, 0, 0, NULL);
> > > + mutex_unlock(&vv->vlock);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + virtio_media_session_free(vv, session);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_find_session - Lookup for the session with a given ID.
> >
> > a session
>
> Fix staged in v5
>
> > > + * @vv: virtio-media device to lookup the session from.
> > > + * @id: ID of the session to lookup.
> > > + */
> > > +static struct virtio_media_session *
> > > +virtio_media_find_session(struct virtio_media *vv, u32 id)
> > > +{
> > > + struct list_head *p;
> > > + struct virtio_media_session *session = NULL;
> > > +
> > > + mutex_lock(&vv->sessions_lock);
> > > + list_for_each(p, &vv->sessions) {
> > > + struct virtio_media_session *s =
> > > + list_entry(p, struct virtio_media_session, list);
> > > + if (s->id == id) {
> > > + session = s;
> > > + break;
> > > + }
> > > + }
> > > + mutex_unlock(&vv->sessions_lock);
> > > +
> > > + return session;
> > > +}
> > > +
> > > +/**
> > > + * struct virtio_media_cmd_callback_param - Callback parameters to the virtio
> > > + * command queue.
> > > + * @vv: virtio-media device in use.
> > > + * @done: flag to be switched once the command is completed.
> > > + * @resp_len: length of the received response from the command. Only valid
> > > + * after @done_flag has switched to ``true``.
> >
> > confusing indent
>
> Fix staged in v5
>
> > > + */
> > > +struct virtio_media_cmd_callback_param {
> > > + struct virtio_media *vv;
> > > + bool done;
> > > + size_t resp_len;
> > > +};
> > > +
> > > +/**
> > > + * commandq_callback: Callback for the command queue.
> > > + * @queue: command virtqueue.
> > > + *
> > > + * This just wakes up the thread that was waiting on the command to complete.
> > > + */
> > > +static void commandq_callback(struct virtqueue *queue)
> > > +{
> > > + unsigned int len;
> > > + struct virtio_media_cmd_callback_param *param;
> > > +
> > > +process_bufs:
> > > + while ((param = virtqueue_get_buf(queue, &len))) {
> > > + param->done = true;
> > > + param->resp_len = len;
> > > + wake_up(¶m->vv->wq);
> > > + }
> > > +
> > > + if (!virtqueue_enable_cb(queue)) {
> > > + virtqueue_disable_cb(queue);
> > > + goto process_bufs;
> > > + }
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_kick_command - send a command to the commandq.
> > > + * @vv: virtio-media device in use.
> > > + * @sgs: descriptor chain to send.
> > > + * @out_sgs: number of device-readable descriptors in @sgs.
> > > + * @in_sgs: number of device-writable descriptors in @sgs.
> > > + * @resp_len: output parameter. Upon success, contains the size of the response
> > > + * in bytes.
> >
> > confusing indent
> >
> > > + *
> >
> > why an empty line?
>
> Indent fixed and empty line removed in v5
>
> > > + */
> > > +static int virtio_media_kick_command(struct virtio_media *vv,
> > > + struct scatterlist **sgs,
> > > + const size_t out_sgs, const size_t in_sgs,
> > > + size_t *resp_len)
> > > +{
> > > + struct virtio_media_cmd_callback_param cb_param = {
> > > + .vv = vv,
> > > + .done = false,
> > > + .resp_len = 0,
> > > + };
> > > + struct virtio_media_resp_header *resp_header;
> > > + int ret;
> > > +
> > > + ret = virtqueue_add_sgs(vv->commandq, sgs, out_sgs, in_sgs, &cb_param,
> > > + GFP_ATOMIC);
> >
> >
> > can init with declaration.
>
> Thanks, added to v5
>
> > > + if (ret) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "failed to add sgs to command virtqueue\n");
> > > + return ret;
> > > + }
> > > +
> > > + if (!virtqueue_kick(vv->commandq)) {
> > > + v4l2_err(&vv->v4l2_dev, "failed to kick command virtqueue\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + /* Wait for the response. */
> > > + ret = wait_event_timeout(vv->wq, cb_param.done, 5 * HZ);
> > > + if (ret == 0) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "timed out waiting for response to command\n");
> > > + return -ETIMEDOUT;
> > > + }
> > > +
> > > + if (resp_len)
> > > + *resp_len = cb_param.resp_len;
> > > +
> > > + if (in_sgs > 0) {
> > > + /*
> > > + * If we expect a response, make sure we have at least a
> > > + * response header - anything shorter is invalid.
> > > + */
> > > + if (cb_param.resp_len < sizeof(*resp_header)) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "received response header is too short\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + resp_header = sg_virt(sgs[out_sgs]);
> > > + if (resp_header->status)
> > > + /* Host returns a positive error code. */
> > > + return -resp_header->status;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_send_command - Send a command to the device and wait for its
> > > + * response.
> > > + * @vv: virtio-media device in use.
> > > + * @sgs: descriptor chain to send.
> > > + * @out_sgs: number of device-readable descriptors in @sgs.
> > > + * @in_sgs: number of device-writable descriptors in @sgs.
> > > + * @minimum_resp_len: minimum length of the response expected by the caller
> > > + * when the command is successful. Anything shorter than that will result in
> > > + * ``-EINVAL`` being returned.
> > > + * @resp_len: output parameter. Upon success, contains the size of the response
> > > + * in bytes.
> > > + */
> > > +int virtio_media_send_command(struct virtio_media *vv, struct scatterlist **sgs,
> > > + const size_t out_sgs, const size_t in_sgs,
> > > + size_t minimum_resp_len, size_t *resp_len)
> > > +{
> > > + size_t local_resp_len = resp_len ? *resp_len : 0;
> > > + int ret = virtio_media_kick_command(vv, sgs, out_sgs, in_sgs,
> > > + &local_resp_len);
> > > + if (resp_len)
> > > + *resp_len = local_resp_len;
> > > +
> > > + /*
> > > + * If the host could not process the command, there is no valid
> > > + * response.
> > > + */
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + /* Make sure the host wrote a complete reply. */
> > > + if (local_resp_len < minimum_resp_len) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "received response is too short: received %zu, expected at least %zu\n",
> > > + local_resp_len, minimum_resp_len);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_send_event_buffer() - Sends an event buffer to the host so it
> > > + * can return it with an event.
> > > + * @vv: virtio-media device in use.
> > > + * @event_buffer: pointer to the event buffer to send to the device.
> > > + */
> > > +static int virtio_media_send_event_buffer(struct virtio_media *vv,
> > > + void *event_buffer)
> > > +{
> > > + struct scatterlist *sgs[1], vresp;
> > > + int ret;
> > > +
> > > + sg_init_one(&vresp, event_buffer, VIRTIO_MEDIA_EVENT_MAX_SIZE);
> > > + sgs[0] = &vresp;
> >
> > what is this convoluted thing? why not pass &vresp directly?
>
> `virtqueue_add_sgs` expects a `struct scatterlist **`. Passing `&vresp` is
> only a single pointer and it needs a double pointer.
>
> It could be written as:
>
> struct scatterlist *sgs, vresp;
>
> sg_init_one(&vresp, event_buffer, VIRTIO_MEDIA_EVENT_MAX_SIZE);
> sgs = &vresp;
> ret = virtqueue_add_sgs(vv->eventq, &sgs, 0, 1, event_buffer,
>
> But I suspect it's written as is to match `virtqueue_add_sgs`'s function
> declaration:
>
> int virtqueue_add_sgs(struct virtqueue *vq,
> struct scatterlist *sgs[],
> unsigned int out_sgs,
> unsigned int in_sgs,
> void *data,
> gfp_t gfp);
>
> > > +
> > > + ret = virtqueue_add_sgs(vv->eventq, sgs, 0, 1, event_buffer,
> > > + GFP_ATOMIC);
> >
> >
> > This does not work uness event_buffer is aligned for dma.
> > But it does not seem to be:
> >
> > for (i = 0; i < VIRTIO_MEDIA_NUM_EVENT_BUFS; i++) {
> > void *ebuf = vv->event_buffer + VIRTIO_MEDIA_EVENT_MAX_SIZE * i;
> >
> > ret = virtio_media_send_event_buffer(vv, ebuf);
> > if (ret)
> > goto err_send_event_buffer;
>
> Apologies if the following questions are bit basic, I'm still pretty new to
> this.
>
> Is the dma alignment requirement a general requirement for calling the
> `virtqueue_add_sgs` function? Or is the dma alignment requirement to allow the
> driver to support DMABUF transfers? If its the later, I should note this driver
> does not support DMABUF transfers yet when streaming frames.
>
> If it's the former, then how would I go about ensuring its dma aligned?
>
> From the code block you quoted above, `vv->event_buffer` is allocated like so:
>
> vv->event_buffer = devm_kzalloc(dev,
> VIRTIO_MEDIA_EVENT_MAX_SIZE *
> VIRTIO_MEDIA_NUM_EVENT_BUFS,
> GFP_KERNEL);
> if (!vv->event_buffer)
> return -ENOMEM;
>
> Do I need to call something else besides `devm_kzalloc()` to ensure every
> element within the vv->event_buffer array is dma aligned?
Before I send out v5, would you mind addressing this question? I
didn't want it getting lost in all of the threads. Thanks!
> >
> >
> > > + if (ret) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "failed to add sgs to event virtqueue\n");
> > > + return ret;
> > > + }
> > > +
> > > + if (!virtqueue_kick(vv->eventq)) {
> > > + v4l2_err(&vv->v4l2_dev, "failed to kick event virtqueue\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * eventq_callback() - Callback for the event queue.
> > > + * @queue: event virtqueue.
> > > + *
> > > + * This just schedules for event work to be run.
> > > + */
> > > +static void eventq_callback(struct virtqueue *queue)
> > > +{
> > > + struct virtio_media *vv = queue->vdev->priv;
> > > +
> > > + schedule_work(&vv->eventq_work);
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_process_dqbuf_event() - Process a dequeued event for a session.
> > > + * @vv: virtio-media device in use.
> > > + * @session: session the event is addressed to.
> > > + * @dqbuf_evt: the dequeued event to process.
> > > + *
> > > + * Invalid events are ignored with an error log.
> > > + */
> > > +static void
> > > +virtio_media_process_dqbuf_event(struct virtio_media *vv,
> > > + struct virtio_media_session *session,
> > > + struct virtio_media_event_dqbuf *dqbuf_evt)
> > > +{
> > > + struct virtio_media_buffer *dqbuf;
> > > + const enum v4l2_buf_type queue_type = dqbuf_evt->buffer.type;
> > > + struct virtio_media_queue_state *queue;
> > > + typeof(dqbuf->buffer.m) buffer_m;
> > > + typeof(dqbuf->buffer.m.planes[0].m) plane_m;
> > > + int i;
> > > +
> > > + if (queue_type >= ARRAY_SIZE(session->queues)) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "unmanaged queue %d passed to dqbuf event",
> > > + dqbuf_evt->buffer.type);
> > > + return;
> > > + }
> > > + queue = &session->queues[queue_type];
> > > +
> > > + if (dqbuf_evt->buffer.index >= queue->allocated_bufs) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "invalid buffer ID %d for queue %d in dqbuf event",
> > > + dqbuf_evt->buffer.index, dqbuf_evt->buffer.type);
> > > + return;
> > > + }
> > > +
> > > + dqbuf = &queue->buffers[dqbuf_evt->buffer.index];
> > > +
> > > + /*
> > > + * Preserve the 'm' union that was passed to us during QBUF so userspace
> > > + * gets back the information it submitted.
> > > + */
> > > + buffer_m = dqbuf->buffer.m;
> > > + memcpy(&dqbuf->buffer, &dqbuf_evt->buffer, sizeof(dqbuf->buffer));
> > > + dqbuf->buffer.m = buffer_m;
> > > + if (V4L2_TYPE_IS_MULTIPLANAR(dqbuf->buffer.type)) {
> > > + if (dqbuf->buffer.length > VIDEO_MAX_PLANES) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "invalid number of planes received from host for a multiplanar buffer\n");
> > > + return;
> > > + }
> > > + for (i = 0; i < dqbuf->buffer.length; i++) {
> > > + plane_m = dqbuf->planes[i].m;
> > > + memcpy(&dqbuf->planes[i], &dqbuf_evt->planes[i],
> > > + sizeof(struct v4l2_plane));
> > > + dqbuf->planes[i].m = plane_m;
> > > + }
> > > + }
> > > +
> > > + /* Set the DONE flag as the buffer is waiting for being dequeued. */
> > > + dqbuf->buffer.flags |= V4L2_BUF_FLAG_DONE;
> > > +
> > > + mutex_lock(&session->queues_lock);
> > > + list_add_tail(&dqbuf->list, &queue->pending_dqbufs);
> > > + queue->queued_bufs -= 1;
> > > + mutex_unlock(&session->queues_lock);
> > > +
> > > + wake_up(&session->dqbuf_wait);
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_process_events() - Process all pending events on a device.
> > > + * @vv: device which pending events we want to process.
> > > + *
> > > + * Retrieves all pending events on @vv's event queue and dispatch them to their
> > > + * corresponding session.
> > > + *
> > > + * Invalid events are ignored with an error log.
> > > + */
> > > +void virtio_media_process_events(struct virtio_media *vv)
> > > +{
> > > + struct virtio_media_event_error *error_evt;
> > > + struct virtio_media_event_dqbuf *dqbuf_evt;
> > > + struct virtio_media_event_event *event_evt;
> > > + struct virtio_media_session *session;
> > > + struct virtio_media_event_header *evt;
> > > + unsigned int len;
> > > +
> > > + mutex_lock(&vv->events_lock);
> > > +
> > > +process_bufs:
> > > + while ((evt = virtqueue_get_buf(vv->eventq, &len))) {
> > > + /* Make sure we received enough data */
> > > + if (len < sizeof(*evt)) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "event is too short: got %u, expected at least %zu\n",
> > > + len, sizeof(*evt));
> > > + goto end_of_event;
> > > + }
> > > +
> > > + session = virtio_media_find_session(vv, evt->session_id);
> > > + if (!session) {
> > > + v4l2_err(&vv->v4l2_dev, "cannot find session %d\n",
> > > + evt->session_id);
> > > + goto end_of_event;
> > > + }
> > > +
> > > + switch (evt->event) {
> > > + case VIRTIO_MEDIA_EVT_ERROR:
> > > + if (len < sizeof(*error_evt)) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "error event is too short: got %u, expected %zu\n",
> > > + len, sizeof(*error_evt));
> > > + break;
> > > + }
> > > + error_evt = (struct virtio_media_event_error *)evt;
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "received error %d for session %d",
> > > + error_evt->errno, error_evt->hdr.session_id);
> > > + virtio_media_session_close(vv, session);
> > > + break;
> > > +
> > > + /*
> > > + * Dequeued buffer: put it into the right queue so user-space
> > > + * can dequeue it.
> > > + */
> > > + case VIRTIO_MEDIA_EVT_DQBUF:
> > > + if (len < sizeof(*dqbuf_evt)) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "dqbuf event is too short: got %u, expected %zu\n",
> > > + len, sizeof(*dqbuf_evt));
> > > + break;
> > > + }
> > > + dqbuf_evt = (struct virtio_media_event_dqbuf *)evt;
> > > + virtio_media_process_dqbuf_event(vv, session,
> > > + dqbuf_evt);
> > > + break;
> > > +
> > > + case VIRTIO_MEDIA_EVT_EVENT:
> > > + if (len < sizeof(*event_evt)) {
> > > + v4l2_err(&vv->v4l2_dev,
> > > + "session event is too short: got %u expected %zu\n",
> > > + len, sizeof(*event_evt));
> > > + break;
> > > + }
> > > +
> > > + event_evt = (struct virtio_media_event_event *)evt;
> > > + v4l2_event_queue_fh(&session->fh, &event_evt->event);
> > > + break;
> > > +
> > > + default:
> > > + v4l2_err(&vv->v4l2_dev, "unknown event type %d\n",
> > > + evt->event);
> > > + break;
> > > + }
> > > +
> > > +end_of_event:
> > > + virtio_media_send_event_buffer(vv, evt);
> > > + }
> > > +
> > > + if (!virtqueue_enable_cb(vv->eventq)) {
> > > + virtqueue_disable_cb(vv->eventq);
> > > + goto process_bufs;
> > > + }
> > > +
> > > + mutex_unlock(&vv->events_lock);
> > > +}
> > > +
> > > +static void virtio_media_event_work(struct work_struct *work)
> > > +{
> > > + struct virtio_media *vv =
> > > + container_of(work, struct virtio_media, eventq_work);
> > > +
> > > + virtio_media_process_events(vv);
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_device_open() - Create a new session from an opened file.
> > > + * @file: opened file for the session.
> > > + */
> > > +static int virtio_media_device_open(struct file *file)
> > > +{
> > > + struct video_device *video_dev = video_devdata(file);
> > > + struct virtio_media *vv = to_virtio_media(video_dev);
> > > + struct virtio_media_cmd_open *cmd_open = &vv->cmd.open;
> > > + struct virtio_media_resp_open *resp_open = &vv->resp.open;
> > > + struct scatterlist cmd_sg = {}, resp_sg = {};
> > > + struct scatterlist *sgs[2] = { &cmd_sg, &resp_sg };
> > > + struct virtio_media_session *session;
> > > + u32 session_id;
> > > + int ret;
> > > +
> > > + mutex_lock(&vv->vlock);
> > > +
> > > + sg_set_buf(&cmd_sg, cmd_open, sizeof(*cmd_open));
> > > + sg_mark_end(&cmd_sg);
> > > +
> > > + sg_set_buf(&resp_sg, resp_open, sizeof(*resp_open));
> > > + sg_mark_end(&resp_sg);
> > > +
> > > + cmd_open->hdr.cmd = VIRTIO_MEDIA_CMD_OPEN;
> > > + ret = virtio_media_send_command(vv, sgs, 1, 1, sizeof(*resp_open),
> > > + NULL);
> > > + session_id = resp_open->session_id;
> > > + mutex_unlock(&vv->vlock);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + session = virtio_media_session_alloc(vv, session_id, file);
> > > + if (IS_ERR(session))
> > > + return PTR_ERR(session);
> > > +
> > > + file->private_data = &session->fh;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_device_close() - Close a previously opened session.
> > > + * @file: file of the session to close.
> > > + *
> > > + * This sends to ``VIRTIO_MEDIA_CMD_CLOSE`` command to the device, and close
> > > + * the session on the driver side.
> > > + */
> > > +static int virtio_media_device_close(struct file *file)
> > > +{
> > > + struct video_device *video_dev = video_devdata(file);
> > > + struct virtio_media *vv = to_virtio_media(video_dev);
> > > + struct virtio_media_session *session =
> > > + fh_to_session(file->private_data);
> > > +
> > > + return virtio_media_session_close(vv, session);
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_device_poll() - Poll logic for a virtio-media device.
> > > + * @file: file of the session to poll.
> > > + * @wait: poll table to wait on.
> > > + */
> > > +static __poll_t virtio_media_device_poll(struct file *file, poll_table *wait)
> > > +{
> > > + struct virtio_media_session *session =
> > > + fh_to_session(file->private_data);
> > > + enum v4l2_buf_type capture_type =
> > > + session->uses_mplane ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE :
> > > + V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > > + enum v4l2_buf_type output_type =
> > > + session->uses_mplane ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE :
> > > + V4L2_BUF_TYPE_VIDEO_OUTPUT;
> > > + struct virtio_media_queue_state *capture_queue =
> > > + &session->queues[capture_type];
> > > + struct virtio_media_queue_state *output_queue =
> > > + &session->queues[output_type];
> > > + __poll_t req_events = poll_requested_events(wait);
> > > + __poll_t rc = 0;
> > > +
> > > + poll_wait(file, &session->dqbuf_wait, wait);
> > > + poll_wait(file, &session->fh.wait, wait);
> > > +
> > > + mutex_lock(&session->queues_lock);
> > > + if (req_events & (EPOLLIN | EPOLLRDNORM)) {
> > > + if (!capture_queue->streaming ||
> > > + (capture_queue->queued_bufs == 0 &&
> > > + list_empty(&capture_queue->pending_dqbufs)))
> > > + rc |= EPOLLERR;
> > > + else if (!list_empty(&capture_queue->pending_dqbufs))
> > > + rc |= EPOLLIN | EPOLLRDNORM;
> > > + }
> > > + if (req_events & (EPOLLOUT | EPOLLWRNORM)) {
> > > + if (!output_queue->streaming)
> > > + rc |= EPOLLERR;
> > > + else if (output_queue->queued_bufs <
> > > + output_queue->allocated_bufs)
> > > + rc |= EPOLLOUT | EPOLLWRNORM;
> > > + }
> > > + mutex_unlock(&session->queues_lock);
> > > +
> > > + if (v4l2_event_pending(&session->fh))
> > > + rc |= EPOLLPRI;
> > > +
> > > + return rc;
> > > +}
> > > +
> > > +static void virtio_media_vma_close_locked(struct vm_area_struct *vma)
> > > +{
> > > + struct virtio_media *vv = vma->vm_private_data;
> > > + struct virtio_media_cmd_munmap *cmd_munmap = &vv->cmd.munmap;
> > > + struct virtio_media_resp_munmap *resp_munmap = &vv->resp.munmap;
> > > + struct scatterlist cmd_sg = {}, resp_sg = {};
> > > + struct scatterlist *sgs[2] = { &cmd_sg, &resp_sg };
> > > + int ret;
> > > +
> > > + sg_set_buf(&cmd_sg, cmd_munmap, sizeof(*cmd_munmap));
> > > + sg_mark_end(&cmd_sg);
> > > +
> > > + sg_set_buf(&resp_sg, resp_munmap, sizeof(*resp_munmap));
> > > + sg_mark_end(&resp_sg);
> > > +
> > > + cmd_munmap->hdr.cmd = VIRTIO_MEDIA_CMD_MUNMAP;
> > > + cmd_munmap->driver_addr =
> > > + (vma->vm_pgoff << PAGE_SHIFT) - vv->mmap_region.addr;
> > > + ret = virtio_media_send_command(vv, sgs, 1, 1, sizeof(*resp_munmap),
> > > + NULL);
> > > + if (ret < 0) {
> > > + v4l2_err(&vv->v4l2_dev, "host failed to unmap buffer: %d\n",
> > > + ret);
> > > + }
> > > +}
> > > +
> > > +/**
> > > + * virtio_media_vma_close() - Close a MMAP buffer mapping.
> > > + * @vma: VMA of the mapping to close.
> > > + *
> > > + * Inform the host that a previously created MMAP mapping is no longer needed
> > > + * and can be removed.
> > > + */
> > > +static void virtio_media_vma_close(struct vm_area_struct *vma)
> > > +{
> > > + struct virtio_media *vv = vma->vm_private_data;
> > > +
> > > + mutex_lock(&vv->vlock);
> > > + virtio_media_vma_close_locked(vma);
> > > + mutex_unlock(&vv->vlock);
> > > +}
> > > +
> > > +static const struct vm_operations_struct virtio_media_vm_ops = {
> > > + .close = virtio_media_vma_close,
> > > +};
> > > +
> > > +/**
> > > + * virtio_media_device_mmap - Perform a mmap request from userspace.
> > > + * @file: opened file of the session to map for.
> > > + * @vma: VM area struct describing the desired mapping.
> > > + *
> > > + * This requests the host to map a MMAP buffer for us, so we can then make that
> > > + * mapping visible into user-space address space.
> > > + */
> > > +static int virtio_media_device_mmap(struct file *file,
> > > + struct vm_area_struct *vma)
> > > +{
> > > + struct video_device *video_dev = video_devdata(file);
> > > + struct virtio_media *vv = to_virtio_media(video_dev);
> > > + struct virtio_media_session *session =
> > > + fh_to_session(file->private_data);
> > > + struct virtio_media_cmd_mmap *cmd_mmap = &session->cmd.mmap;
> > > + struct virtio_media_resp_mmap *resp_mmap = &session->resp.mmap;
> > > + struct scatterlist cmd_sg = {}, resp_sg = {};
> > > + struct scatterlist *sgs[2] = { &cmd_sg, &resp_sg };
> > > + int ret;
> > > +
> > > + if (!(vma->vm_flags & VM_SHARED))
> > > + return -EINVAL;
> > > + if (!(vma->vm_flags & (VM_READ | VM_WRITE)))
> > > + return -EINVAL;
> > > +
> > > + mutex_lock(&vv->vlock);
> > > +
> > > + cmd_mmap->hdr.cmd = VIRTIO_MEDIA_CMD_MMAP;
> > > + cmd_mmap->session_id = session->id;
> > > + cmd_mmap->flags =
> > > + (vma->vm_flags & VM_WRITE) ? VIRTIO_MEDIA_MMAP_FLAG_RW : 0;
> > > + cmd_mmap->offset = vma->vm_pgoff << PAGE_SHIFT;
> > > +
> > > + sg_set_buf(&cmd_sg, cmd_mmap, sizeof(*cmd_mmap));
> > > + sg_mark_end(&cmd_sg);
> > > +
> > > + sg_set_buf(&resp_sg, resp_mmap, sizeof(*resp_mmap));
> > > + sg_mark_end(&resp_sg);
> > > +
> > > + /*
> > > + * The host performs reference counting and is smart enough to return
> > > + * the same guest physical address if this is called several times on
> > > + * the same
> > > + * buffer.
> > > + */
> > > + ret = virtio_media_send_command(vv, sgs, 1, 1, sizeof(*resp_mmap),
> > > + NULL);
> > > + if (ret < 0)
> > > + goto end;
> > > +
> > > + vma->vm_private_data = vv;
> > > + /*
> > > + * Keep the guest address at which the buffer is mapped since we will
> > > + * use that to unmap.
> > > + */
> > > + vma->vm_pgoff = (resp_mmap->driver_addr + vv->mmap_region.addr) >>
> > > + PAGE_SHIFT;
> > > +
> > > + /*
> > > + * We cannot let the mapping be larger than the buffer.
> > > + */
> > > + if (vma->vm_end - vma->vm_start > PAGE_ALIGN(resp_mmap->len)) {
> > > + dev_dbg(&video_dev->dev,
> > > + "invalid MMAP, as it would overflow buffer length\n");
> > > + virtio_media_vma_close_locked(vma);
> > > + ret = -EINVAL;
> > > + goto end;
> > > + }
> > > +
> > > + ret = io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
> > > + vma->vm_end - vma->vm_start,
> > > + vma->vm_page_prot);
> > > + if (ret)
> > > + goto end;
> > > +
> > > + vma->vm_ops = &virtio_media_vm_ops;
> > > +
> > > +end:
> > > + mutex_unlock(&vv->vlock);
> > > + return ret;
> > > +}
> > > +
> > > +static const struct v4l2_file_operations virtio_media_fops = {
> > > + .owner = THIS_MODULE,
> > > + .open = virtio_media_device_open,
> > > + .release = virtio_media_device_close,
> > > + .poll = virtio_media_device_poll,
> > > + .unlocked_ioctl = virtio_media_device_ioctl,
> > > + .mmap = virtio_media_device_mmap,
> > > +};
> > > +
> > > +static int virtio_media_probe(struct virtio_device *virtio_dev)
> > > +{
> > > + struct device *dev = &virtio_dev->dev;
> > > + struct virtqueue *vqs[2];
> > > + static struct virtqueue_info vq_info[2] = {
> > > + {
> > > + .name = "command",
> > > + .callback = commandq_callback,
> > > + },
> > > + {
> > > + .name = "event",
> > > + .callback = eventq_callback,
> > > + },
> > > + };
> > > + struct virtio_media *vv;
> > > + struct video_device *vd;
> > > + int i;
> > > + int ret;
> > > +
> > > + vv = devm_kzalloc(dev, sizeof(*vv), GFP_KERNEL);
> > > + if (!vv)
> > > + return -ENOMEM;
> > > +
> > > + vv->event_buffer = devm_kzalloc(dev,
> > > + VIRTIO_MEDIA_EVENT_MAX_SIZE *
> > > + VIRTIO_MEDIA_NUM_EVENT_BUFS,
> > > + GFP_KERNEL);
> > > + if (!vv->event_buffer)
> > > + return -ENOMEM;
> > > +
> > > + INIT_LIST_HEAD(&vv->sessions);
> > > + mutex_init(&vv->sessions_lock);
> > > + mutex_init(&vv->events_lock);
> > > + mutex_init(&vv->vlock);
> > > +
> > > + vv->virtio_dev = virtio_dev;
> > > + virtio_dev->priv = vv;
> > > +
> > > + init_waitqueue_head(&vv->wq);
> > > +
> > > + ret = v4l2_device_register(dev, &vv->v4l2_dev);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = virtio_find_vqs(virtio_dev, 2, vqs, vq_info, NULL);
> > > + if (ret)
> > > + goto err_find_vqs;
> > > +
> > > + vv->commandq = vqs[0];
> > > + vv->eventq = vqs[1];
> > > + INIT_WORK(&vv->eventq_work, virtio_media_event_work);
> > > +
> > > + /* Get MMAP buffer mapping SHM region */
> > > + virtio_get_shm_region(virtio_dev, &vv->mmap_region,
> > > + VIRTIO_MEDIA_SHM_MMAP);
> > > +
> > > + vd = &vv->video_dev;
> > > +
> > > + vd->v4l2_dev = &vv->v4l2_dev;
> > > + vd->vfl_type = VFL_TYPE_VIDEO;
> > > + vd->ioctl_ops = &virtio_media_ioctl_ops;
> > > + vd->fops = &virtio_media_fops;
> > > + vd->device_caps = virtio_cread32(virtio_dev, 0);
> > > + if (vd->device_caps & (V4L2_CAP_VIDEO_M2M | V4L2_CAP_VIDEO_M2M_MPLANE))
> > > + vd->vfl_dir = VFL_DIR_M2M;
> > > + else if (vd->device_caps &
> > > + (V4L2_CAP_VIDEO_OUTPUT | V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE))
> > > + vd->vfl_dir = VFL_DIR_TX;
> > > + else
> > > + vd->vfl_dir = VFL_DIR_RX;
> > > + vd->release = video_device_release_empty;
> > > + strscpy(vd->name, "virtio-media", sizeof(vd->name));
> > > +
> > > + video_set_drvdata(vd, vv);
> > > +
> > > + ret = video_register_device(vd, virtio_cread32(virtio_dev, 4), 0);
> > > + if (ret)
> > > + goto err_register_device;
> > > +
> > > + for (i = 0; i < VIRTIO_MEDIA_NUM_EVENT_BUFS; i++) {
> > > + void *ebuf = vv->event_buffer + VIRTIO_MEDIA_EVENT_MAX_SIZE * i;
> > > +
> > > + ret = virtio_media_send_event_buffer(vv, ebuf);
> > > + if (ret)
> > > + goto err_send_event_buffer;
> > > + }
> > > +
> > > + virtio_device_ready(virtio_dev);
> > > +
> > > + return 0;
> > > +
> > > +err_send_event_buffer:
> > > + video_unregister_device(&vv->video_dev);
> > > +err_register_device:
> > > + virtio_dev->config->del_vqs(virtio_dev);
> > > +err_find_vqs:
> > > + v4l2_device_unregister(&vv->v4l2_dev);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static void virtio_media_remove(struct virtio_device *virtio_dev)
> > > +{
> > > + struct virtio_media *vv = virtio_dev->priv;
> > > + struct list_head *p, *n;
> > > +
> > > + cancel_work_sync(&vv->eventq_work);
> > > + virtio_reset_device(virtio_dev);
> > > +
> > > + v4l2_device_unregister(&vv->v4l2_dev);
> > > + virtio_dev->config->del_vqs(virtio_dev);
> > > + video_unregister_device(&vv->video_dev);
> > > +
> > > + list_for_each_safe(p, n, &vv->sessions) {
> > > + struct virtio_media_session *s =
> > > + list_entry(p, struct virtio_media_session, list);
> > > +
> > > + virtio_media_session_free(vv, s);
> > > + }
> > > +}
> > > +
> > > +static struct virtio_device_id id_table[] = {
> > > + { VIRTIO_ID_MEDIA, VIRTIO_DEV_ANY_ID },
> > > + { 0 },
> > > +};
> > > +
> > > +static unsigned int features[] = {};
> > > +
> > > +static struct virtio_driver virtio_media_driver = {
> > > + .feature_table = features,
> > > + .feature_table_size = ARRAY_SIZE(features),
> > > + .driver.name = VIRTIO_MEDIA_DEFAULT_DRIVER_NAME,
> > > + .driver.owner = THIS_MODULE,
> > > + .id_table = id_table,
> > > + .probe = virtio_media_probe,
> > > + .remove = virtio_media_remove,
> > > +};
> > > +
> > > +module_virtio_driver(virtio_media_driver);
> > > +
> > > +MODULE_DEVICE_TABLE(virtio, id_table);
> > > +MODULE_DESCRIPTION("virtio media driver");
> > > +MODULE_AUTHOR("Alexandre Courbot <gnurou@gmail.com>");
> > > +MODULE_LICENSE("Dual BSD/GPL");
^ permalink raw reply
* Re: [PATCH v4 2/8] media: virtio: Add virtio-media driver structs and function declarations
From: Brian Daniels @ 2026-07-17 19:46 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Mauro Carvalho Chehab, acourbot, adelva, aesteve, changyeon,
daniel.almeida, eperezma, gnurou, gurchetansingh, hverkuil,
jasowang, linux-kernel, linux-media, nicolas.dufresne,
virtualization, xuanzhuo
In-Reply-To: <20260622170708-mutt-send-email-mst@kernel.org>
On Mon, Jun 22, 2026 at 5:08 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Jun 22, 2026 at 04:43:37PM -0400, Brian Daniels wrote:
> > From: Alexandre Courbot <gnurou@gmail.com>
> >
> > Add the structs and function declarations for the new virtio-media drvier.
> >
> > Signed-off-by: Alexandre Courbot <gnurou@gmail.com>
> > Co-developed-by: Brian Daniels <briandaniels@google.com>
> > Signed-off-by: Brian Daniels <briandaniels@google.com>
> > ---
> > drivers/media/virtio/virtio_media.h | 95 +++++++++++++++++++++++++++++
> > 1 file changed, 95 insertions(+)
> > create mode 100644 drivers/media/virtio/virtio_media.h
> >
> > diff --git a/drivers/media/virtio/virtio_media.h b/drivers/media/virtio/virtio_media.h
> > new file mode 100644
> > index 000000000..52809d4e9
> > --- /dev/null
> > +++ b/drivers/media/virtio/virtio_media.h
> > @@ -0,0 +1,95 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0+ */
> > +
> > +/*
> > + * Virtio-media structures & functions declarations.
> > + *
> > + * Copyright (c) 2024-2025 Google LLC.
> > + */
> > +
> > +#ifndef __VIRTIO_MEDIA_H
> > +#define __VIRTIO_MEDIA_H
> > +
> > +#include <linux/virtio_config.h>
> > +#include <media/v4l2-device.h>
> > +
> > +#include "protocol.h"
> > +
> > +#define DESC_CHAIN_MAX_LEN SG_MAX_SINGLE_ALLOC
> > +
> > +#define VIRTIO_MEDIA_DEFAULT_DRIVER_NAME "virtio-media"
> > +
> > +extern char *virtio_media_driver_name;
> > +extern bool virtio_media_allow_userptr;
> > +
> > +/**
> > + * struct virtio_media - Virtio-media device.
> > + * @v4l2_dev: v4l2_device for the media device.
> > + * @video_dev: video_device for the media device.
> > + * @virtio_dev: virtio device for the media device.
> > + * @commandq: virtio command queue.
> > + * @eventq: virtio event queue.
> > + * @eventq_work: work to run when events are received on @eventq.
> > + * @mmap_region: region into which MMAP buffers are mapped by the host.
> > + * @event_buffer: buffer for event descriptors.
> > + * @sessions: list of active sessions on the device.
> > + * @sessions_lock: protects @sessions and ``virtio_media_session::list``.
> > + * @events_lock: prevents concurrent processing of events.
> > + * @cmd: union of the device commands "open" and "munmap". The other
> > + * commands are handled by @struct virtio_media_session
> > + * @resp: union of responses.to device commands "open" and "munmap". The
> > + * other responses are handled by @struct virtio_media_session
> > + * @vlock: serializes access to the command queue.
> > + * @wq: waitqueue for host responses on the command queue.
> > + */
> > +struct virtio_media {
> > + struct v4l2_device v4l2_dev;
> > + struct video_device video_dev;
> > +
> > + struct virtio_device *virtio_dev;
> > + struct virtqueue *commandq;
> > + struct virtqueue *eventq;
> > + struct work_struct eventq_work;
> > +
> > + struct virtio_shm_region mmap_region;
> > +
> > + void *event_buffer;
> > +
> > + struct list_head sessions;
> > + struct mutex sessions_lock; /* protects sessions list */
> > +
> > + struct mutex events_lock; /* prevents concurrent event processing */
> > +
> > + union {
> > + struct virtio_media_cmd_open open;
> > + struct virtio_media_cmd_munmap munmap;
> > + } cmd;
> > +
> > + union {
> > + struct virtio_media_resp_open open;
> > + struct virtio_media_resp_munmap munmap;
> > + } resp;
>
>
> You need DMA alignment padding for these things.
Each member of the above unions is already padded to 64-bit alignment,
is that sufficient?
If not, could you tell me what else is necessary?
> Which one can only see when I reads the actual driver 8 patches down.
> Which is why it's not a sensible way to split patches.
>
> A sensible way is to have a driver then add functionality
> in logical pieces gradually.
>
Appreciate the feedback. There was a previous comment requesting to
split the patch into c/h file pairs, but I can try reformatting it as
you suggest for v5.
> > +
> > + struct mutex vlock; /* serializes command queue access */
> > + wait_queue_head_t wq;
> > +};
> > +
> > +static inline struct virtio_media *
> > +to_virtio_media(struct video_device *video_dev)
> > +{
> > + return container_of(video_dev, struct virtio_media, video_dev);
> > +}
> > +
> > +/* virtio_media_driver.c */
> > +
> > +int virtio_media_send_command(struct virtio_media *vv, struct scatterlist **sgs,
> > + const size_t out_sgs, const size_t in_sgs,
> > + size_t minimum_resp_len, size_t *resp_len);
> > +void virtio_media_process_events(struct virtio_media *vv);
> > +
> > +/* virtio_media_ioctls.c */
> > +
> > +long virtio_media_device_ioctl(struct file *file, unsigned int cmd,
> > + unsigned long arg);
> > +extern const struct v4l2_ioctl_ops virtio_media_ioctl_ops;
> > +
> > +#endif // __VIRTIO_MEDIA_H
> > --
> > 2.55.0.rc0.799.gd6f94ed593-goog
>
^ permalink raw reply
* Re: [PATCH] vhost-scsi: Prevent OOM from invalid protection SGL count
From: Michael S. Tsirkin @ 2026-07-17 16:31 UTC (permalink / raw)
To: Mike Christie
Cc: Jia Jia, jasowang, Linfeng Sun, pbonzini, stefanha, eperezma,
virtualization, kvm, netdev, linux-kernel, stable
In-Reply-To: <8f5b5350-0be9-4b79-a6c9-069d418dee30@oracle.com>
On Fri, Jul 17, 2026 at 10:13:36AM -0500, Mike Christie wrote:
> cc'ing Linfeng.
>
> Linfeng also reported this bug and provided a patch that has your check and
> an extra check to avoid calling into the function in the first place. It's
> been stuck in some other list.
>
> I think it might be best for Linfeng to re-submit their patch now so it can
> get merged. For that patch, I think there was one outstanding question left
> where Michael had asked if the issue was found with AI so the proper tag
> could be added. I don't think that question was responded to.
Right, was waiting for that.
>
> On 7/17/26 9:22 AM, Jia Jia wrote:
> > The protection SGL path passes the result of vhost_scsi_calc_sgls()
> > directly to sg_alloc_table_chained(). The helper returns a negative
> > errno when the iterator is invalid or the request exceeds the segment
> > limit. The negative errno is then treated as a very large unsigned count
> > and sends the request into the SGL allocation path with an invalid size.
> >
> > Repeated malformed T10-PI submissions from a host-side application caused
> > memory usage to rise sharply. MemAvailable fell to about 200 MB, and PSI
> > full avg10 reached about 1.46. The OOM killer terminated several userspace
> > processes before the endpoint cleanup completed. The kernel log included:
> >
> > [17036.451028] Out of memory: Killed process 2345 (systemd)
> > [17036.493325] Out of memory: Killed process 2349 (sd-pam)
> > [17078.265127] Out of memory: Killed process 1793 (networkd-dispat)
> >
> > Return the calculation error before setting up the protection SGL. This
> > keeps the protection path consistent with the data SGL path and prevents
> > the invalid count from entering the allocation path.
> >
> > Fixes: bca939d5bcd0 ("vhost-scsi: Dynamically allocate scatterlists")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Jia Jia <physicalmtea@gmail.com>
> > ---
> > drivers/vhost/scsi.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> > index 9a1253b9d..8486652fd 100644
> > --- a/drivers/vhost/scsi.c
> > +++ b/drivers/vhost/scsi.c
> > @@ -972,6 +972,9 @@ vhost_scsi_mapal(struct vhost_scsi *vs, struct vhost_scsi_cmd *cmd,
> > if (prot_bytes) {
> > sgl_count = vhost_scsi_calc_sgls(prot_iter, prot_bytes,
> > VHOST_SCSI_PREALLOC_PROT_SGLS);
> > + if (sgl_count < 0)
> > + return sgl_count;
> > +
> > cmd->prot_table.sgl = cmd->prot_sgl;
> > ret = sg_alloc_table_chained(&cmd->prot_table, sgl_count,
> > cmd->prot_table.sgl,
> > --
> > 2.43.0
> >
^ permalink raw reply
* Re: [PATCH v2 1/4] virtio-mem: validate device-reported block size
From: Michael S. Tsirkin @ 2026-07-17 16:30 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: David Hildenbrand (Arm), Hari Mishal, Jason Wang, Xuan Zhuo,
Eugenio Pérez, virtualization, linux-kernel, elena.reshetova,
carlos.bilbao.osdev
In-Reply-To: <20260717122729-mutt-send-email-mst@kernel.org>
On Fri, Jul 17, 2026 at 12:28:45PM -0400, Michael S. Tsirkin wrote:
> On Fri, Jul 17, 2026 at 04:31:03PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Jul 17, 2026 at 09:08:23AM -0400, Michael S. Tsirkin wrote:
> > > On Fri, Jul 17, 2026 at 02:07:50PM +0200, Greg Kroah-Hartman wrote:
> > > > On Fri, Jul 17, 2026 at 06:52:46AM -0400, Michael S. Tsirkin wrote:
> > > > > On Fri, Jul 17, 2026 at 12:46:52PM +0200, Greg Kroah-Hartman wrote:
> > > > > > On Fri, Jul 17, 2026 at 06:23:57AM -0400, Michael S. Tsirkin wrote:
> > > > > > > On Fri, Jul 17, 2026 at 12:15:09PM +0200, Greg Kroah-Hartman wrote:
> > > > > > > > On Fri, Jul 17, 2026 at 06:10:41AM -0400, Michael S. Tsirkin wrote:
> > > > > > > > > On Fri, Jul 17, 2026 at 11:14:23AM +0200, Greg Kroah-Hartman wrote:
> > > > > > > > > > On Fri, Jul 17, 2026 at 04:59:32AM -0400, Michael S. Tsirkin wrote:
> > > > > > > > > > > On Fri, Jul 17, 2026 at 10:39:40AM +0200, David Hildenbrand (Arm) wrote:
> > > > > > > > > > > > On 7/17/26 07:48, Michael S. Tsirkin wrote:
> > > > > > > > > > > > > On Thu, Jul 16, 2026 at 05:59:05PM +0200, David Hildenbrand (Arm) wrote:
> > > > > > > > > > > > >>> Or do we just always trust virtio mem devices explicitly?
> > > > > > > > > > > > >>
> > > > > > > > > > > > >> It's hard for me to understand where we draw the line, really.
> > > > > > > > > > > > >>
> > > > > > > > > > > > >> But maybe MST can clarify what we care about in virtio world where the
> > > > > > > > > > > > >> hypervisor is fully in charge of the device,
> > > > > > > > > > > > >
> > > > > > > > > > > > > Generally:
> > > > > > > > > > > > > - The guest is expected to whitelist drivers (most drivers have not
> > > > > > > > > > > > > been audited).
> > > > > > > > > > > >
> > > > > > > > > > > > But even if you audited your driver, who makes sure that we consider all ways
> > > > > > > > > > > > where the device could mess with us?
> > > > > > > > > > >
> > > > > > > > > > > A lot of this is up to a correct setup. For example, make sure all
> > > > > > > > > > > filesystems are encrypted and refuse to mount unencrypted ones.
> > > > > > > > > > >
> > > > > > > > > > > > Something feels off here.
> > > > > > > > > > > >
> > > > > > > > > > > > Handling selected out-of-spec scenarios like this feels like a band-aid. Happy
> > > > > > > > > > > > to be corrected.
> > > > > > > > > > >
> > > > > > > > > > > Well Documentation/security/snp-tdx-threat-model.rst puts it like this:
> > > > > > > > > > > It is important to note
> > > > > > > > > > > that this doesn’t imply that the host or VMM are intentionally
> > > > > > > > > > > malicious, but that there exists a security value in having a small CoCo
> > > > > > > > > > > VM TCB.
> > > > > > > > > > >
> > > > > > > > > > > and
> > > > > > > > > > >
> > > > > > > > > > > While traditionally the host has unlimited access to guest data and can
> > > > > > > > > > > leverage this access to attack the guest, the CoCo systems mitigate such
> > > > > > > > > > > attacks by adding security features like guest data confidentiality and
> > > > > > > > > > > integrity protection.
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > now, when we are talking about "mitigation" it is indeed becoming a bit
> > > > > > > > > > > murky.
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > For me, a rule of thumb I came up with is that if the validation happens
> > > > > > > > > > > to also be helful for users e.g. to work around buggy devices,
> > > > > > > > > > > or maybe because we feel failing gracefully is nice because this
> > > > > > > > > > > will allow to later make use of this config and old drivers will
> > > > > > > > > > > fail but at least not panic, then it is good to include.
> > > > > > > > > >
> > > > > > > > > > Why not do what USB does? Don't trust the device until AFTER probe()
> > > > > > > > > > succeeds? All of the needed checking should happen before then, as that
> > > > > > > > > > is a "slow path" so lots of validation and the like can happen at that
> > > > > > > > > > point.
> > > > > > > > > >
> > > > > > > > > > After that, during the normal data paths, after the driver is bound,
> > > > > > > > > > trust it all you want as attempting to validate every single packet is
> > > > > > > > > > just going to be impossible.
> > > > > > > > > >
> > > > > > > > > > thanks,
> > > > > > > > > >
> > > > > > > > > > greg k-h
> > > > > > > > >
> > > > > > > > > People do expect that data path validation at this point.
> > > > > > > >
> > > > > > > > Ok, so you want this patch :)
> > > > > > > >
> > > > > > > > And more, as you need to treat everything from the host as "untrusted",
> > > > > > > > and it must be "verified".
> > > > > > >
> > > > > > > Well. First it's not me) Second it's only specific configurations -
> > > > > > > for example there's no short term plan to validate filesystem code, people
> > > > > > > are expected to rely on encryption. The reasons have more to do
> > > > > > > with the available manpower than anything else.
> > > > > >
> > > > > > Sure, but again, for subsystems, you have to define your threat model as
> > > > > > the LLMs are churning against the code base and coming up with lots of
> > > > > > crazy ideas if a device should or should not be trusted and spitting out
> > > > > > patches and reports like the ones that are in the first few patches of
> > > > > > this series.
> > > > > >
> > > > > > So please, pick a model, let's document it, and go with that. I am
> > > > > > getting directly conflicting responses here.
> > > > > >
> > > > > > thanks,
> > > > > >
> > > > > > greg k-h
> > > > >
> > > > > Supposed to be this one:
> > > > > Documentation/security/snp-tdx-threat-model.rst
> > > > >
> > > > > what is missing?
> > > >
> > > > A policy decision that needs to be made. All that document does is
> > > > describe a bunch of different "threats" yet does not decide what to do
> > > > about them at all from what I can tell.
> > >
> > > That would be this section I think:
> > >
> > > The **Linux kernel CoCo VM security objectives** can be summarized as follows:
> > >
> > > it does, indeed, not go into detail about how to interact, safely,
> > > with untrusted entities. Does it really need to be spelled out?
> >
> > Seems like it as I didn't figure it out at all :)
> >
> > > > And that's just for one subset of the CoC world, right? Is that
> > > > something that all virtio drivers need/want to care about?
> > >
> > > What is missing, and what you seem to be asking for, is an opinionated
> > > stance on which drivers we care about in this world?
> > > True.
> >
> > Yes.
> >
> > > coco guys tried to annotate drivers at some point to do exactly that.
> > > this was rejected upstream from the position that this is not
> > > different from handling buggy hardware, and just to fix all drivers.
> > > so it's up to users, and I guess for virtio the answer is yes
> > > with some exceptions because we don't have a better answer right now.
> >
> > Ok, so back to the original question here:
> >
> > > > So I don't see a real answer to the "does Linux trust the host to give
> > > > you good data or not" question in that file, am I missing it?
> > > >
> > > > thanks,
> > > >
> > > > greg k-h
> > >
> > > This? Note the last sentence.
> > >
> > > The **Linux CoCo VM attack surface** is any interface exposed from a CoCo
> > > guest Linux kernel towards an untrusted host that is not covered by the
> > > CoCo technology SW/HW protection. This includes any possible
> > > side-channels, as well as transient execution side channels. Examples of
> > > explicit (not side-channel) interfaces include accesses to port I/O, MMIO
> > > and DMA interfaces, access to PCI configuration space, VMM-specific
> > > hypercalls (towards Host-side VMM), access to shared memory pages,
> > > interrupts allowed to be injected into the guest kernel by the host, as
> > > well as CoCo technology-specific hypercalls, if present. Additionally, the
> > > host in a CoCo system typically controls the process of creating a CoCo
> > > guest: it has a method to load into a guest the firmware and bootloader
> > > images, the kernel image together with the kernel command line. All of this
> > > data should also be considered untrusted until its integrity and
> > > authenticity is established via attestation.
> >
> > Great, so you are saying that we need to fix any bug found where a host
> > could be sending "bad" data over the virtio path before, and after, the
> > driver is bound to the device. That's a solid answer, and let's let the
> > LLMs run with that!
> >
> > Which also implies that the first 3 patches here are acceptable, right?
> > :)
> >
> > thanks,
> >
> > greg k-h
>
> The 1st patch has nothing to do with coco. Bad values it checks do not
> lead to any information leak. I'm fine with it as such
> but I
> would like the commit log to make that clearer.
Or to be more precise, I'd like the motivation to be documented,
given it is not coco, and then I can judge the merits.
>
> --
> MST
^ permalink raw reply
* Re: [PATCH v2 1/4] virtio-mem: validate device-reported block size
From: Michael S. Tsirkin @ 2026-07-17 16:28 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: David Hildenbrand (Arm), Hari Mishal, Jason Wang, Xuan Zhuo,
Eugenio Pérez, virtualization, linux-kernel, elena.reshetova,
carlos.bilbao.osdev
In-Reply-To: <2026071724-modulator-pliable-fa1c@gregkh>
On Fri, Jul 17, 2026 at 04:31:03PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Jul 17, 2026 at 09:08:23AM -0400, Michael S. Tsirkin wrote:
> > On Fri, Jul 17, 2026 at 02:07:50PM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Jul 17, 2026 at 06:52:46AM -0400, Michael S. Tsirkin wrote:
> > > > On Fri, Jul 17, 2026 at 12:46:52PM +0200, Greg Kroah-Hartman wrote:
> > > > > On Fri, Jul 17, 2026 at 06:23:57AM -0400, Michael S. Tsirkin wrote:
> > > > > > On Fri, Jul 17, 2026 at 12:15:09PM +0200, Greg Kroah-Hartman wrote:
> > > > > > > On Fri, Jul 17, 2026 at 06:10:41AM -0400, Michael S. Tsirkin wrote:
> > > > > > > > On Fri, Jul 17, 2026 at 11:14:23AM +0200, Greg Kroah-Hartman wrote:
> > > > > > > > > On Fri, Jul 17, 2026 at 04:59:32AM -0400, Michael S. Tsirkin wrote:
> > > > > > > > > > On Fri, Jul 17, 2026 at 10:39:40AM +0200, David Hildenbrand (Arm) wrote:
> > > > > > > > > > > On 7/17/26 07:48, Michael S. Tsirkin wrote:
> > > > > > > > > > > > On Thu, Jul 16, 2026 at 05:59:05PM +0200, David Hildenbrand (Arm) wrote:
> > > > > > > > > > > >>> Or do we just always trust virtio mem devices explicitly?
> > > > > > > > > > > >>
> > > > > > > > > > > >> It's hard for me to understand where we draw the line, really.
> > > > > > > > > > > >>
> > > > > > > > > > > >> But maybe MST can clarify what we care about in virtio world where the
> > > > > > > > > > > >> hypervisor is fully in charge of the device,
> > > > > > > > > > > >
> > > > > > > > > > > > Generally:
> > > > > > > > > > > > - The guest is expected to whitelist drivers (most drivers have not
> > > > > > > > > > > > been audited).
> > > > > > > > > > >
> > > > > > > > > > > But even if you audited your driver, who makes sure that we consider all ways
> > > > > > > > > > > where the device could mess with us?
> > > > > > > > > >
> > > > > > > > > > A lot of this is up to a correct setup. For example, make sure all
> > > > > > > > > > filesystems are encrypted and refuse to mount unencrypted ones.
> > > > > > > > > >
> > > > > > > > > > > Something feels off here.
> > > > > > > > > > >
> > > > > > > > > > > Handling selected out-of-spec scenarios like this feels like a band-aid. Happy
> > > > > > > > > > > to be corrected.
> > > > > > > > > >
> > > > > > > > > > Well Documentation/security/snp-tdx-threat-model.rst puts it like this:
> > > > > > > > > > It is important to note
> > > > > > > > > > that this doesn’t imply that the host or VMM are intentionally
> > > > > > > > > > malicious, but that there exists a security value in having a small CoCo
> > > > > > > > > > VM TCB.
> > > > > > > > > >
> > > > > > > > > > and
> > > > > > > > > >
> > > > > > > > > > While traditionally the host has unlimited access to guest data and can
> > > > > > > > > > leverage this access to attack the guest, the CoCo systems mitigate such
> > > > > > > > > > attacks by adding security features like guest data confidentiality and
> > > > > > > > > > integrity protection.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > now, when we are talking about "mitigation" it is indeed becoming a bit
> > > > > > > > > > murky.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > For me, a rule of thumb I came up with is that if the validation happens
> > > > > > > > > > to also be helful for users e.g. to work around buggy devices,
> > > > > > > > > > or maybe because we feel failing gracefully is nice because this
> > > > > > > > > > will allow to later make use of this config and old drivers will
> > > > > > > > > > fail but at least not panic, then it is good to include.
> > > > > > > > >
> > > > > > > > > Why not do what USB does? Don't trust the device until AFTER probe()
> > > > > > > > > succeeds? All of the needed checking should happen before then, as that
> > > > > > > > > is a "slow path" so lots of validation and the like can happen at that
> > > > > > > > > point.
> > > > > > > > >
> > > > > > > > > After that, during the normal data paths, after the driver is bound,
> > > > > > > > > trust it all you want as attempting to validate every single packet is
> > > > > > > > > just going to be impossible.
> > > > > > > > >
> > > > > > > > > thanks,
> > > > > > > > >
> > > > > > > > > greg k-h
> > > > > > > >
> > > > > > > > People do expect that data path validation at this point.
> > > > > > >
> > > > > > > Ok, so you want this patch :)
> > > > > > >
> > > > > > > And more, as you need to treat everything from the host as "untrusted",
> > > > > > > and it must be "verified".
> > > > > >
> > > > > > Well. First it's not me) Second it's only specific configurations -
> > > > > > for example there's no short term plan to validate filesystem code, people
> > > > > > are expected to rely on encryption. The reasons have more to do
> > > > > > with the available manpower than anything else.
> > > > >
> > > > > Sure, but again, for subsystems, you have to define your threat model as
> > > > > the LLMs are churning against the code base and coming up with lots of
> > > > > crazy ideas if a device should or should not be trusted and spitting out
> > > > > patches and reports like the ones that are in the first few patches of
> > > > > this series.
> > > > >
> > > > > So please, pick a model, let's document it, and go with that. I am
> > > > > getting directly conflicting responses here.
> > > > >
> > > > > thanks,
> > > > >
> > > > > greg k-h
> > > >
> > > > Supposed to be this one:
> > > > Documentation/security/snp-tdx-threat-model.rst
> > > >
> > > > what is missing?
> > >
> > > A policy decision that needs to be made. All that document does is
> > > describe a bunch of different "threats" yet does not decide what to do
> > > about them at all from what I can tell.
> >
> > That would be this section I think:
> >
> > The **Linux kernel CoCo VM security objectives** can be summarized as follows:
> >
> > it does, indeed, not go into detail about how to interact, safely,
> > with untrusted entities. Does it really need to be spelled out?
>
> Seems like it as I didn't figure it out at all :)
>
> > > And that's just for one subset of the CoC world, right? Is that
> > > something that all virtio drivers need/want to care about?
> >
> > What is missing, and what you seem to be asking for, is an opinionated
> > stance on which drivers we care about in this world?
> > True.
>
> Yes.
>
> > coco guys tried to annotate drivers at some point to do exactly that.
> > this was rejected upstream from the position that this is not
> > different from handling buggy hardware, and just to fix all drivers.
> > so it's up to users, and I guess for virtio the answer is yes
> > with some exceptions because we don't have a better answer right now.
>
> Ok, so back to the original question here:
>
> > > So I don't see a real answer to the "does Linux trust the host to give
> > > you good data or not" question in that file, am I missing it?
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > This? Note the last sentence.
> >
> > The **Linux CoCo VM attack surface** is any interface exposed from a CoCo
> > guest Linux kernel towards an untrusted host that is not covered by the
> > CoCo technology SW/HW protection. This includes any possible
> > side-channels, as well as transient execution side channels. Examples of
> > explicit (not side-channel) interfaces include accesses to port I/O, MMIO
> > and DMA interfaces, access to PCI configuration space, VMM-specific
> > hypercalls (towards Host-side VMM), access to shared memory pages,
> > interrupts allowed to be injected into the guest kernel by the host, as
> > well as CoCo technology-specific hypercalls, if present. Additionally, the
> > host in a CoCo system typically controls the process of creating a CoCo
> > guest: it has a method to load into a guest the firmware and bootloader
> > images, the kernel image together with the kernel command line. All of this
> > data should also be considered untrusted until its integrity and
> > authenticity is established via attestation.
>
> Great, so you are saying that we need to fix any bug found where a host
> could be sending "bad" data over the virtio path before, and after, the
> driver is bound to the device. That's a solid answer, and let's let the
> LLMs run with that!
>
> Which also implies that the first 3 patches here are acceptable, right?
> :)
>
> thanks,
>
> greg k-h
The 1st patch has nothing to do with coco. Bad values it checks do not
lead to any information leak. I'm fine with it as such but I
would like the commit log to make that clearer.
--
MST
^ permalink raw reply
* RE: [PATCH v2 1/4] virtio-mem: validate device-reported block size
From: Michael Kelley @ 2026-07-17 15:27 UTC (permalink / raw)
To: Greg Kroah-Hartman, Michael S. Tsirkin
Cc: David Hildenbrand (Arm), Hari Mishal, Jason Wang, Xuan Zhuo,
Eugenio Pérez, virtualization@lists.linux.dev,
linux-kernel@vger.kernel.org, elena.reshetova@intel.com,
carlos.bilbao.osdev@gmail.com
In-Reply-To: <2026071724-modulator-pliable-fa1c@gregkh>
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Sent: Friday, July 17, 2026 7:31 AM
> On Fri, Jul 17, 2026 at 09:08:23AM -0400, Michael S. Tsirkin wrote:
> > On Fri, Jul 17, 2026 at 02:07:50PM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Jul 17, 2026 at 06:52:46AM -0400, Michael S. Tsirkin wrote:
> > > > On Fri, Jul 17, 2026 at 12:46:52PM +0200, Greg Kroah-Hartman wrote:
> > > > > On Fri, Jul 17, 2026 at 06:23:57AM -0400, Michael S. Tsirkin wrote:
> > > > > > On Fri, Jul 17, 2026 at 12:15:09PM +0200, Greg Kroah-Hartman wrote:
> > > > > > > On Fri, Jul 17, 2026 at 06:10:41AM -0400, Michael S. Tsirkin wrote:
> > > > > > > > On Fri, Jul 17, 2026 at 11:14:23AM +0200, Greg Kroah-Hartman wrote:
> > > > > > > > > On Fri, Jul 17, 2026 at 04:59:32AM -0400, Michael S. Tsirkin wrote:
> > > > > > > > > > On Fri, Jul 17, 2026 at 10:39:40AM +0200, David Hildenbrand (Arm) wrote:
> > > > > > > > > > > On 7/17/26 07:48, Michael S. Tsirkin wrote:
> > > > > > > > > > > > On Thu, Jul 16, 2026 at 05:59:05PM +0200, David Hildenbrand (Arm) wrote:
> > > > > > > > > > > >>> Or do we just always trust virtio mem devices explicitly?
> > > > > > > > > > > >>
> > > > > > > > > > > >> It's hard for me to understand where we draw the line, really.
> > > > > > > > > > > >>
> > > > > > > > > > > >> But maybe MST can clarify what we care about in virtio world where the
> > > > > > > > > > > >> hypervisor is fully in charge of the device,
> > > > > > > > > > > >
> > > > > > > > > > > > Generally:
> > > > > > > > > > > > - The guest is expected to whitelist drivers (most drivers have not
> > > > > > > > > > > > been audited).
> > > > > > > > > > >
> > > > > > > > > > > But even if you audited your driver, who makes sure that we consider all ways
> > > > > > > > > > > where the device could mess with us?
> > > > > > > > > >
> > > > > > > > > > A lot of this is up to a correct setup. For example, make sure all
> > > > > > > > > > filesystems are encrypted and refuse to mount unencrypted ones.
> > > > > > > > > >
> > > > > > > > > > > Something feels off here.
> > > > > > > > > > >
> > > > > > > > > > > Handling selected out-of-spec scenarios like this feels like a band-aid. Happy
> > > > > > > > > > > to be corrected.
> > > > > > > > > >
> > > > > > > > > > Well Documentation/security/snp-tdx-threat-model.rst puts it like this:
> > > > > > > > > > It is important to note
> > > > > > > > > > that this doesn’t imply that the host or VMM are intentionally
> > > > > > > > > > malicious, but that there exists a security value in having a small CoCo
> > > > > > > > > > VM TCB.
> > > > > > > > > >
> > > > > > > > > > and
> > > > > > > > > >
> > > > > > > > > > While traditionally the host has unlimited access to guest data and can
> > > > > > > > > > leverage this access to attack the guest, the CoCo systems mitigate such
> > > > > > > > > > attacks by adding security features like guest data confidentiality and
> > > > > > > > > > integrity protection.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > now, when we are talking about "mitigation" it is indeed becoming a bit
> > > > > > > > > > murky.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > For me, a rule of thumb I came up with is that if the validation happens
> > > > > > > > > > to also be helful for users e.g. to work around buggy devices,
> > > > > > > > > > or maybe because we feel failing gracefully is nice because this
> > > > > > > > > > will allow to later make use of this config and old drivers will
> > > > > > > > > > fail but at least not panic, then it is good to include.
> > > > > > > > >
> > > > > > > > > Why not do what USB does? Don't trust the device until AFTER probe()
> > > > > > > > > succeeds? All of the needed checking should happen before then, as that
> > > > > > > > > is a "slow path" so lots of validation and the like can happen at that
> > > > > > > > > point.
> > > > > > > > >
> > > > > > > > > After that, during the normal data paths, after the driver is bound,
> > > > > > > > > trust it all you want as attempting to validate every single packet is
> > > > > > > > > just going to be impossible.
> > > > > > > > >
> > > > > > > > > thanks,
> > > > > > > > >
> > > > > > > > > greg k-h
> > > > > > > >
> > > > > > > > People do expect that data path validation at this point.
> > > > > > >
> > > > > > > Ok, so you want this patch :)
> > > > > > >
> > > > > > > And more, as you need to treat everything from the host as "untrusted",
> > > > > > > and it must be "verified".
> > > > > >
> > > > > > Well. First it's not me) Second it's only specific configurations -
> > > > > > for example there's no short term plan to validate filesystem code, people
> > > > > > are expected to rely on encryption. The reasons have more to do
> > > > > > with the available manpower than anything else.
> > > > >
> > > > > Sure, but again, for subsystems, you have to define your threat model as
> > > > > the LLMs are churning against the code base and coming up with lots of
> > > > > crazy ideas if a device should or should not be trusted and spitting out
> > > > > patches and reports like the ones that are in the first few patches of
> > > > > this series.
> > > > >
> > > > > So please, pick a model, let's document it, and go with that. I am
> > > > > getting directly conflicting responses here.
> > > > >
> > > > > thanks,
> > > > >
> > > > > greg k-h
> > > >
> > > > Supposed to be this one:
> > > > Documentation/security/snp-tdx-threat-model.rst
> > > >
> > > > what is missing?
> > >
> > > A policy decision that needs to be made. All that document does is
> > > describe a bunch of different "threats" yet does not decide what to do
> > > about them at all from what I can tell.
> >
> > That would be this section I think:
> >
> > The **Linux kernel CoCo VM security objectives** can be summarized as follows:
> >
> > it does, indeed, not go into detail about how to interact, safely,
> > with untrusted entities. Does it really need to be spelled out?
>
> Seems like it as I didn't figure it out at all :)
>
> > > And that's just for one subset of the CoC world, right? Is that
> > > something that all virtio drivers need/want to care about?
> >
> > What is missing, and what you seem to be asking for, is an opinionated
> > stance on which drivers we care about in this world?
> > True.
>
> Yes.
>
> > coco guys tried to annotate drivers at some point to do exactly that.
> > this was rejected upstream from the position that this is not
> > different from handling buggy hardware, and just to fix all drivers.
> > so it's up to users, and I guess for virtio the answer is yes
> > with some exceptions because we don't have a better answer right now.
>
> Ok, so back to the original question here:
>
> > > So I don't see a real answer to the "does Linux trust the host to give
> > > you good data or not" question in that file, am I missing it?
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > This? Note the last sentence.
> >
> > The **Linux CoCo VM attack surface** is any interface exposed from a CoCo
> > guest Linux kernel towards an untrusted host that is not covered by the
> > CoCo technology SW/HW protection. This includes any possible
> > side-channels, as well as transient execution side channels. Examples of
> > explicit (not side-channel) interfaces include accesses to port I/O, MMIO
> > and DMA interfaces, access to PCI configuration space, VMM-specific
> > hypercalls (towards Host-side VMM), access to shared memory pages,
> > interrupts allowed to be injected into the guest kernel by the host, as
> > well as CoCo technology-specific hypercalls, if present. Additionally, the
> > host in a CoCo system typically controls the process of creating a CoCo
> > guest: it has a method to load into a guest the firmware and bootloader
> > images, the kernel image together with the kernel command line. All of this
> > data should also be considered untrusted until its integrity and
> > authenticity is established via attestation.
>
> Great, so you are saying that we need to fix any bug found where a host
> could be sending "bad" data over the virtio path before, and after, the
> driver is bound to the device. That's a solid answer, and let's let the
> LLMs run with that!
>
FWIW, Hyper-V VMBus drivers in Linux use this same threat model,
with the same conclusions about data validation. The details are in
Documentation/virt/hyperv/coco.rst. See the Section entitled
" Guest communication with Hyper-V". Let me know if you've come
across potential data leaks or corruption modalities in the virtio code
that might also be applicable to the VMBus drivers, that aren't already
mentioned in that documentation.
Michael
^ permalink raw reply
* Re: [PATCH] vhost-scsi: Prevent OOM from invalid protection SGL count
From: Mike Christie @ 2026-07-17 15:13 UTC (permalink / raw)
To: Jia Jia, mst, jasowang, Linfeng Sun
Cc: pbonzini, stefanha, eperezma, virtualization, kvm, netdev,
linux-kernel, stable
In-Reply-To: <20260717142205.103515-1-physicalmtea@gmail.com>
cc'ing Linfeng.
Linfeng also reported this bug and provided a patch that has your check
and an extra check to avoid calling into the function in the first
place. It's been stuck in some other list.
I think it might be best for Linfeng to re-submit their patch now so it
can get merged. For that patch, I think there was one outstanding
question left where Michael had asked if the issue was found with AI so
the proper tag could be added. I don't think that question was responded to.
On 7/17/26 9:22 AM, Jia Jia wrote:
> The protection SGL path passes the result of vhost_scsi_calc_sgls()
> directly to sg_alloc_table_chained(). The helper returns a negative
> errno when the iterator is invalid or the request exceeds the segment
> limit. The negative errno is then treated as a very large unsigned count
> and sends the request into the SGL allocation path with an invalid size.
>
> Repeated malformed T10-PI submissions from a host-side application caused
> memory usage to rise sharply. MemAvailable fell to about 200 MB, and PSI
> full avg10 reached about 1.46. The OOM killer terminated several userspace
> processes before the endpoint cleanup completed. The kernel log included:
>
> [17036.451028] Out of memory: Killed process 2345 (systemd)
> [17036.493325] Out of memory: Killed process 2349 (sd-pam)
> [17078.265127] Out of memory: Killed process 1793 (networkd-dispat)
>
> Return the calculation error before setting up the protection SGL. This
> keeps the protection path consistent with the data SGL path and prevents
> the invalid count from entering the allocation path.
>
> Fixes: bca939d5bcd0 ("vhost-scsi: Dynamically allocate scatterlists")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jia Jia <physicalmtea@gmail.com>
> ---
> drivers/vhost/scsi.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 9a1253b9d..8486652fd 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -972,6 +972,9 @@ vhost_scsi_mapal(struct vhost_scsi *vs, struct vhost_scsi_cmd *cmd,
> if (prot_bytes) {
> sgl_count = vhost_scsi_calc_sgls(prot_iter, prot_bytes,
> VHOST_SCSI_PREALLOC_PROT_SGLS);
> + if (sgl_count < 0)
> + return sgl_count;
> +
> cmd->prot_table.sgl = cmd->prot_sgl;
> ret = sg_alloc_table_chained(&cmd->prot_table, sgl_count,
> cmd->prot_table.sgl,
> --
> 2.43.0
>
^ permalink raw reply
* [PATCH] virtio_console: take a kref in find_port_by_vq() to fix port UAF
From: Hari Mishal @ 2026-07-17 15:06 UTC (permalink / raw)
To: Amit Shah
Cc: Arnd Bergmann, Greg Kroah-Hartman, virtualization, linux-kernel,
Hari Mishal
find_port_by_vq() returns a raw struct port pointer without taking a
reference on it, unlike find_port_by_devt_in_portdev() which does.
find_port_by_vq()'s only two callers, in_intr() and out_intr(), run as
virtqueue interrupt callbacks, entirely independent of and possibly
concurrently with unplug_port(), which itself runs from a workqueue when
the host sends a VIRTIO_CONSOLE_PORT_REMOVE control message.
unplug_port() removes the port from portdev->ports under ports_lock,
then later drops its last reference with kref_put(), freeing it via
remove_port(). find_port_by_vq() also walks portdev->ports under
ports_lock, so if it finds the port still on the list, the list removal,
and therefore the eventual kref_put(), has not happened yet, and taking
a reference at that point is always safe. Without doing so,
in_intr()/out_intr() can be left holding a pointer to a port that
unplug_port() frees on another core before they are done using it.
Both triggers are host-controlled as the host decides when to send the
PORT_REMOVE control message and when to kick the port's data vq. So a
malicious backend could race the two on purpose, without any guest side
cooperation. The freed object is a generic kmalloc allocation containing
a wait_queue_head_t, which in_intr()/out_intr() pass to
wake_up_interruptible() after touching the stale pointer.
wake_up_interruptible() invokes a function pointer read out of the wait
queue's entries. If the freed slab slot is reclaimed with attacker
influenced content before that call, then this is an arbitrary function
call primitive rather than just undefined behaviour.
Take a reference in find_port_by_vq() while still holding ports_lock,
matching find_port_by_devt_in_portdev(), and release it in in_intr() and
out_intr() once they are done with the port.
Signed-off-by: Hari Mishal <harimishal1@gmail.com>
---
drivers/char/virtio_console.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index faef362dae85..1b63afe24e29 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -304,6 +304,12 @@ static struct port *find_port_by_id(struct ports_device *portdev, u32 id)
return port;
}
+/*
+ * Finds a port by the virtqueue and returns a pointer to struct port
+ * with the reference count incremented.
+ *
+ * Callers MUST decrement it when finished.
+ */
static struct port *find_port_by_vq(struct ports_device *portdev,
struct virtqueue *vq)
{
@@ -312,8 +318,10 @@ static struct port *find_port_by_vq(struct ports_device *portdev,
spin_lock_irqsave(&portdev->ports_lock, flags);
list_for_each_entry(port, &portdev->ports, list)
- if (port->in_vq == vq || port->out_vq == vq)
+ if (port->in_vq == vq || port->out_vq == vq) {
+ kref_get(&port->kref);
goto out;
+ }
port = NULL;
out:
spin_unlock_irqrestore(&portdev->ports_lock, flags);
@@ -1706,6 +1714,7 @@ static void out_intr(struct virtqueue *vq)
}
wake_up_interruptible(&port->waitqueue);
+ kref_put(&port->kref, remove_port);
}
static void in_intr(struct virtqueue *vq)
@@ -1723,6 +1732,7 @@ static void in_intr(struct virtqueue *vq)
if (!port->portdev) {
/* Port is being unplugged, ignore further data. */
spin_unlock_irqrestore(&port->inbuf_lock, flags);
+ kref_put(&port->kref, remove_port);
return;
}
port->inbuf = get_inbuf(port);
@@ -1756,6 +1766,8 @@ static void in_intr(struct virtqueue *vq)
if (is_console_port(port) && hvc_poll(port->cons.hvc))
hvc_kick();
+
+ kref_put(&port->kref, remove_port);
}
static void control_intr(struct virtqueue *vq)
--
2.43.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox