Re: [PATCH v13 11/13] virtio-gpu: Handle resource blob commands
On 2024/06/03 14:32, Dmitry Osipenko wrote:
On 6/2/24 08:45, Akihiko Odaki wrote:
...
+ case HOSTMEM_MR_FINISH_UNMAPPING:
+ ret = virgl_renderer_resource_unmap(res->base.resource_id);
+ if (ret) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: failed to unmap virgl resource: %s\n",
+ __func__, strerror(-ret));
+ return ret;
+ }
+ res->mr = NULL;
+ g_free(vmr);
+ break;
+ case HOSTMEM_MR_UNMAPPING:
+ *cmd_suspended = true;
This code path should be unreachable since the command processing is
blocked while unmapping.
Will change to abort()
I don't think abort() call is needed here. You can just do what you do
for HOSTMEM_MR_MAPPED; the reference counter will automatically catch
the double-free condition and abort.
+ if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) {
+ ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries,
sizeof(cblob),
+ cmd, &res->base.addrs,
+ &res->base.iov,
&res->base.iov_cnt);
+ if (!ret) {
+ g_free(res);
As noted for an earlier version:
Use g_autofree instead of writing duplicate g_free() calls. See
docs/devel/style.rst for details.
The g_autofree isn't appropriate for this code. It's intended to be used
if you allocate a tmp variable that should be freed in all code paths.
This is not the case here, the res variable isn't temporal and shall not
be freed on success.
You can assign NULL to res after QTAILQ_INSERT_HEAD(). See
rutabaga_cmd_resource_create_blob() for example.
Usually g_steal_pointer() should be used in such a situation but
unfortunately it is not possible in this case due to how
QTAILQ_INSERT_HEAD() macro is implemented.
Re: [PATCH v13 11/13] virtio-gpu: Handle resource blob commands
On 6/2/24 08:45, Akihiko Odaki wrote:
...
>> + case HOSTMEM_MR_FINISH_UNMAPPING:
>> + ret = virgl_renderer_resource_unmap(res->base.resource_id);
>> + if (ret) {
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "%s: failed to unmap virgl resource: %s\n",
>> + __func__, strerror(-ret));
>> + return ret;
>> + }
>> + res->mr = NULL;
>> + g_free(vmr);
>> + break;
>> + case HOSTMEM_MR_UNMAPPING:
>> + *cmd_suspended = true;
>
> This code path should be unreachable since the command processing is
> blocked while unmapping.
Will change to abort()
>> + if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) {
>> + ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries,
>> sizeof(cblob),
>> + cmd, &res->base.addrs,
>> + &res->base.iov,
>> &res->base.iov_cnt);
>> + if (!ret) {
>> + g_free(res);
>
> As noted for an earlier version:
>> Use g_autofree instead of writing duplicate g_free() calls. See
>> docs/devel/style.rst for details.
The g_autofree isn't appropriate for this code. It's intended to be used
if you allocate a tmp variable that should be freed in all code paths.
This is not the case here, the res variable isn't temporal and shall not
be freed on success.
--
Best regards,
Dmitry
Re: [PATCH v13 11/13] virtio-gpu: Handle resource blob commands
On 2024/05/27 12:02, Dmitry Osipenko wrote:
From: Antonio Caggiano
Support BLOB resources creation, mapping and unmapping by calling the
new stable virglrenderer 0.10 interface. Only enabled when available and
via the blob config. E.g. -device virtio-vga-gl,blob=true
Signed-off-by: Antonio Caggiano
Signed-off-by: Xenia Ragiadakou
Signed-off-by: Huang Rui
Signed-off-by: Dmitry Osipenko
---
hw/display/virtio-gpu-virgl.c | 320 -
hw/display/virtio-gpu.c| 4 +-
include/hw/virtio/virtio-gpu.h | 2 +
3 files changed, 322 insertions(+), 4 deletions(-)
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 7f45b4fa5fd7..0c73d9ba65f9 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -26,6 +26,7 @@
struct virtio_gpu_virgl_resource {
struct virtio_gpu_simple_resource base;
+MemoryRegion *mr;
};
static struct virtio_gpu_virgl_resource *
@@ -49,6 +50,159 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
}
#endif
+#ifdef HAVE_VIRGL_RESOURCE_BLOB
+typedef enum {
+HOSTMEM_MR_MAPPED,
+HOSTMEM_MR_UNMAPPING,
+HOSTMEM_MR_FINISH_UNMAPPING,
+} HostmemMRState;
+
+struct virtio_gpu_virgl_hostmem_region {
+MemoryRegion mr;
+struct VirtIOGPU *g;
+HostmemMRState state;
+};
+
+static struct virtio_gpu_virgl_hostmem_region *
+to_hostmem_region(MemoryRegion *mr)
+{
+return container_of(mr, struct virtio_gpu_virgl_hostmem_region, mr);
+}
+
+static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque)
+{
+VirtIOGPU *g = opaque;
+
+virtio_gpu_process_cmdq(g);
+}
+
+static void virtio_gpu_virgl_hostmem_region_free(void *obj)
+{
+MemoryRegion *mr = MEMORY_REGION(obj);
+struct virtio_gpu_virgl_hostmem_region *vmr;
+VirtIOGPUBase *b;
+VirtIOGPUGL *gl;
+
+vmr = to_hostmem_region(mr);
+vmr->state = HOSTMEM_MR_FINISH_UNMAPPING;
+
+b = VIRTIO_GPU_BASE(vmr->g);
+b->renderer_blocked--;
+
+/*
+ * memory_region_unref() is executed from RCU thread context, while
+ * virglrenderer works only on the main-loop thread that's holding GL
+ * context.
+ */
+gl = VIRTIO_GPU_GL(vmr->g);
+qemu_bh_schedule(gl->cmdq_resume_bh);
+}
+
+static int
+virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
+ struct virtio_gpu_virgl_resource *res,
+ uint64_t offset)
+{
+struct virtio_gpu_virgl_hostmem_region *vmr;
+VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
+MemoryRegion *mr;
+uint64_t size;
+void *data;
+int ret;
+
+if (!virtio_gpu_hostmem_enabled(b->conf)) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: hostmem disabled\n", __func__);
+return -EOPNOTSUPP;
+}
+
+ret = virgl_renderer_resource_map(res->base.resource_id, &data, &size);
+if (ret) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map virgl resource:
%s\n",
+ __func__, strerror(-ret));
+return ret;
+}
+
+vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
+vmr->g = g;
+
+mr = &vmr->mr;
+memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
+memory_region_add_subregion(&b->hostmem, offset, mr);
+memory_region_set_enabled(mr, true);
+
+/*
+ * MR could outlive the resource if MR's reference is held outside of
+ * virtio-gpu. In order to prevent unmapping resource while MR is alive,
+ * and thus, making the data pointer invalid, we will block virtio-gpu
+ * command processing until MR is fully unreferenced and freed.
+ */
+OBJECT(mr)->free = virtio_gpu_virgl_hostmem_region_free;
+
+res->mr = mr;
+
+return 0;
+}
+
+static int
+virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
+ struct virtio_gpu_virgl_resource *res,
+ bool *cmd_suspended)
+{
+struct virtio_gpu_virgl_hostmem_region *vmr;
+VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
+MemoryRegion *mr = res->mr;
+int ret;
+
+if (!mr) {
+return 0;
+}
+
+vmr = to_hostmem_region(res->mr);
+
+/*
+ * Perform async unmapping in 3 steps:
+ *
+ * 1. Begin async unmapping with memory_region_del_subregion()
+ *and suspend/block cmd processing.
+ * 2. Wait for res->mr to be freed and cmd processing resumed
+ *asynchronously by virtio_gpu_virgl_hostmem_region_free().
+ * 3. Finish the unmapping with final virgl_renderer_resource_unmap().
+ */
+
+switch (vmr->state) {
+case HOSTMEM_MR_MAPPED:
+vmr->state = HOSTMEM_MR_UNMAPPING;
+
+*cmd_suspended = true;
+
+/* render will be unblocked once MR is freed */
+b->renderer_blocked++;
+
+/* memory region owns self res->mr object and frees it by itself */
+memory_region_set_enabled(mr, false);
+memory_region_del_subregion(&b->hostmem, mr);
+object_unparent(OBJECT(m
[PATCH v13 11/13] virtio-gpu: Handle resource blob commands
From: Antonio Caggiano
Support BLOB resources creation, mapping and unmapping by calling the
new stable virglrenderer 0.10 interface. Only enabled when available and
via the blob config. E.g. -device virtio-vga-gl,blob=true
Signed-off-by: Antonio Caggiano
Signed-off-by: Xenia Ragiadakou
Signed-off-by: Huang Rui
Signed-off-by: Dmitry Osipenko
---
hw/display/virtio-gpu-virgl.c | 320 -
hw/display/virtio-gpu.c| 4 +-
include/hw/virtio/virtio-gpu.h | 2 +
3 files changed, 322 insertions(+), 4 deletions(-)
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 7f45b4fa5fd7..0c73d9ba65f9 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -26,6 +26,7 @@
struct virtio_gpu_virgl_resource {
struct virtio_gpu_simple_resource base;
+MemoryRegion *mr;
};
static struct virtio_gpu_virgl_resource *
@@ -49,6 +50,159 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
}
#endif
+#ifdef HAVE_VIRGL_RESOURCE_BLOB
+typedef enum {
+HOSTMEM_MR_MAPPED,
+HOSTMEM_MR_UNMAPPING,
+HOSTMEM_MR_FINISH_UNMAPPING,
+} HostmemMRState;
+
+struct virtio_gpu_virgl_hostmem_region {
+MemoryRegion mr;
+struct VirtIOGPU *g;
+HostmemMRState state;
+};
+
+static struct virtio_gpu_virgl_hostmem_region *
+to_hostmem_region(MemoryRegion *mr)
+{
+return container_of(mr, struct virtio_gpu_virgl_hostmem_region, mr);
+}
+
+static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque)
+{
+VirtIOGPU *g = opaque;
+
+virtio_gpu_process_cmdq(g);
+}
+
+static void virtio_gpu_virgl_hostmem_region_free(void *obj)
+{
+MemoryRegion *mr = MEMORY_REGION(obj);
+struct virtio_gpu_virgl_hostmem_region *vmr;
+VirtIOGPUBase *b;
+VirtIOGPUGL *gl;
+
+vmr = to_hostmem_region(mr);
+vmr->state = HOSTMEM_MR_FINISH_UNMAPPING;
+
+b = VIRTIO_GPU_BASE(vmr->g);
+b->renderer_blocked--;
+
+/*
+ * memory_region_unref() is executed from RCU thread context, while
+ * virglrenderer works only on the main-loop thread that's holding GL
+ * context.
+ */
+gl = VIRTIO_GPU_GL(vmr->g);
+qemu_bh_schedule(gl->cmdq_resume_bh);
+}
+
+static int
+virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
+ struct virtio_gpu_virgl_resource *res,
+ uint64_t offset)
+{
+struct virtio_gpu_virgl_hostmem_region *vmr;
+VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
+MemoryRegion *mr;
+uint64_t size;
+void *data;
+int ret;
+
+if (!virtio_gpu_hostmem_enabled(b->conf)) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: hostmem disabled\n", __func__);
+return -EOPNOTSUPP;
+}
+
+ret = virgl_renderer_resource_map(res->base.resource_id, &data, &size);
+if (ret) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to map virgl resource:
%s\n",
+ __func__, strerror(-ret));
+return ret;
+}
+
+vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1);
+vmr->g = g;
+
+mr = &vmr->mr;
+memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
+memory_region_add_subregion(&b->hostmem, offset, mr);
+memory_region_set_enabled(mr, true);
+
+/*
+ * MR could outlive the resource if MR's reference is held outside of
+ * virtio-gpu. In order to prevent unmapping resource while MR is alive,
+ * and thus, making the data pointer invalid, we will block virtio-gpu
+ * command processing until MR is fully unreferenced and freed.
+ */
+OBJECT(mr)->free = virtio_gpu_virgl_hostmem_region_free;
+
+res->mr = mr;
+
+return 0;
+}
+
+static int
+virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
+ struct virtio_gpu_virgl_resource *res,
+ bool *cmd_suspended)
+{
+struct virtio_gpu_virgl_hostmem_region *vmr;
+VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
+MemoryRegion *mr = res->mr;
+int ret;
+
+if (!mr) {
+return 0;
+}
+
+vmr = to_hostmem_region(res->mr);
+
+/*
+ * Perform async unmapping in 3 steps:
+ *
+ * 1. Begin async unmapping with memory_region_del_subregion()
+ *and suspend/block cmd processing.
+ * 2. Wait for res->mr to be freed and cmd processing resumed
+ *asynchronously by virtio_gpu_virgl_hostmem_region_free().
+ * 3. Finish the unmapping with final virgl_renderer_resource_unmap().
+ */
+
+switch (vmr->state) {
+case HOSTMEM_MR_MAPPED:
+vmr->state = HOSTMEM_MR_UNMAPPING;
+
+*cmd_suspended = true;
+
+/* render will be unblocked once MR is freed */
+b->renderer_blocked++;
+
+/* memory region owns self res->mr object and frees it by itself */
+memory_region_set_enabled(mr, false);
+memory_region_del_subregion(&b->hostmem, mr);
+object_unparent(OBJECT(mr));
+break;
+case HOSTMEM_MR_FINISH_UNMAPPING:
+
