Hi Daniel,
On 2026-05-23T05:50:00, Daniel Palmer <[email protected]> wrote:
> virtio: Add driver for virtio gpu
>
> Add a basic driver for virtio gpu. This allows for a video on
> qemu virt machines.
>
> Signed-off-by: Daniel Palmer <[email protected]>
>
> drivers/virtio/Kconfig | 19 ++
> drivers/virtio/Makefile | 1 +
> drivers/virtio/virtio-uclass.c | 1 +
> drivers/virtio/virtio_gpu.c | 284 +++++++++++++++++++++++++
> drivers/virtio/virtio_gpu.h | 461
> +++++++++++++++++++++++++++++++++++++++++
> include/virtio.h | 10 +-
> 6 files changed, 772 insertions(+), 4 deletions(-)
Thanks for doing this!
> diff --git a/drivers/virtio/virtio_gpu.c b/drivers/virtio/virtio_gpu.c
> @@ -0,0 +1,284 @@
> +U_BOOT_DRIVER(virtio_gpu) = {
> + .name = VIRTIO_GPU_DRV_NAME,
> + .id = UCLASS_VIDEO,
> + .ops = &virtio_gpu_ops,
> + .probe = virtio_gpu_probe,
> + .flags = DM_FLAG_ACTIVE_DMA,
> + .priv_auto = sizeof(struct virtio_gpu_priv),
> +};
Two things are missing here per doc/develop/driver-model/virtio.rst
- a bind() that calls virtio_driver_features_init() for feature negotiation
- a '.remove = virtio_reset' so the device is reset before jumping to
the OS; without it the host may keep scanning out of a buffer U-Boot
has freed.
> diff --git a/drivers/virtio/virtio_gpu.c b/drivers/virtio/virtio_gpu.c
> @@ -0,0 +1,284 @@
> + /* virtio GPU apparently doesn't care about alignment */
> + plat->size = (width * height) * 4;
> + plat->base = (ulong)malloc(plat->size);
> + if (!plat->base)
> + return -ENOMEM;
> +
> + ret = virtio_gpu_resource_attach_backing(vq,
> + FRAMEBUFFER_RESOURCE_ID,
> + plat->base,
> + round_up(plat->size, 4096));
The length advertised to the host is round_up(plat->size, 4096) but
only plat->size bytes are allocated, so the host may read past the
end. Allocate the rounded-up size or pass plat->size
More fundamentally, video drivers declare plat->size (and plat->align
if needed) in bind(), and video_reserve() places the framebuffer in
the reserved region below the U-Boot relocation address - see
drivers/video/bochs.c for the standard pattern. So you should not need
the malloc() in probe() ?
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> @@ -77,4 +77,23 @@ config VIRTIO_RNG
> +if VIRTIO_GPU
> +config VIRTIO_DEFAULT_WIDTH
> + int "default width"
> + default 1024
> +
> +config VIRTIO_DEFAULT_HEIGHT
> + int "default height"
> + default 768
> +endif
These sit in the VIRTIO namespace but are GPU-specific - please rename
to VIRTIO_GPU_DEFAULT_WIDTH / VIRTIO_GPU_DEFAULT_HEIGHT
The prompts also do not say what they configure; something like
'Default framebuffer width' would be clearer. BTW, are these ever
used?
> diff --git a/drivers/virtio/virtio_gpu.c b/drivers/virtio/virtio_gpu.c
> @@ -0,0 +1,284 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * virtio GPU driver
> + * 2025 Daniel Palmer <[email protected]>
> + */
> +
> +#include <dm.h>
> +#include <malloc.h>
> +#include <video.h>
> +#include <virtio_types.h>
> +#include <virtio.h>
> +#include <virtio_ring.h>
Please add #define LOG_CATEGORY UCLASS_VIRTIO at the top - also good
to use log_msg_ret() / log_debug() on the error paths. Several
goto-err paths currently swallow the error silently.
> diff --git a/drivers/virtio/virtio_gpu.c b/drivers/virtio/virtio_gpu.c
> @@ -0,0 +1,284 @@
> + while (!virtqueue_get_buf(vq, NULL))
> + ;
> +
> + /* Some basic error handling */
> + switch (le32_to_cpu(resp_hdr->type)) {
> + case VIRTIO_GPU_RESP_ERR_UNSPEC:
> + case VIRTIO_GPU_RESP_ERR_INVALID_SCANOUT_ID:
> + case VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID:
> + case VIRTIO_GPU_RESP_ERR_INVALID_CONTEXT_ID:
> + case VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER:
> + return -EINVAL;
> + case VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY:
> + return -ENOMEM;
> + }
> +
> + return 0;
This returns success for any response not in the listed error codes,
so a future spec error (or garbage) is silently treated as OK. Please
invert the check — require resp_hdr->type to be one of
VIRTIO_GPU_RESP_OK_* and return -EIO otherwise.
> diff --git a/drivers/virtio/virtio_gpu.c b/drivers/virtio/virtio_gpu.c
> @@ -0,0 +1,284 @@
> + virtio: Add driver for virtio gpu
> +
> + Add a basic driver for virtio gpu. This allows for a video on
> + qemu virt machines.
> +
> + Signed-off-by: Daniel Palmer <[email protected]>
The commit message is thin. Please mention the motivation (no current
display driver works on QEMU virt for arm64/riscv?), what is and isn't
supported (2D only, single scanout, no cursor, no EDID, no feature
negotiation) and how the user enables it. Also 'allows for a video'
reads oddly - perhaps 'enables video output on QEMU virt machines'.
Regards,
Simon
