[QEMU PATCH v5 00/13] Support blob memory and venus on qemu

[QEMU PATCH v5 00/13] Support blob memory and venus on qemu

[Huang Rui]

Hi all,

Antonio Caggiano made the venus with QEMU on KVM platform last
September[1]. This series are inherited from his original work to support
the features of context init, hostmem, resource uuid, and blob resources
for venus.
At March of this year, we sent out the V1 version[2] for the review. But
those series are included both xen and virtio gpu. Right now, we would like
to divide into two parts, one is to continue the Antonio's work to upstream
virtio-gpu support for blob memory and venus, and another is to upstream
xen specific patches. This series is focusing on virtio-gpu, so we are
marking as V4 version here to continue Antonio's patches[1]. And we will
send xen specific patches separately, because they are hypervisor specific.
Besides of QEMU, these supports also included virglrenderer[3][4] and
mesa[5][6] as well. Right now, virglrenderer and mesa parts are all
accepted by upstream. In this qemu version, we try to address the concerns
around not proper cleanup during blob resource unmap and unref. Appreciate
it if you have any commments.

[1] https://lore.kernel.org/qemu-devel/20220926142422.22325-1-antonio.caggiano@collabora.com/
[2] V1: https://lore.kernel.org/qemu-devel/20230312092244.451465-1-ray.huang@amd.com
[3] https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1068
[4] https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1180
[5] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22108
[6] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/23680

Please note the first 4 patches 1 -> 4 are inlcuded in these series because
the series depends on them and not because we want them to be reviewed
since they are already in the process of review through the "rutabaga_gfx +
gfxstream" series.
- https://lore.kernel.org/qemu-devel/20230829003629.410-1-gurchetansingh@chromium.org/

V4: https://lore.kernel.org/qemu-devel/20230831093252.2461282-1-ray.huang@amd.com

Changes from V4 (virtio gpu V4) to V5

- Inverted patch 5 and 6 because we should configure
  HAVE_VIRGL_CONTEXT_INIT firstly.

- Validate owner of memory region to avoid slowing down DMA.

- Use memory_region_init_ram_ptr() instead of
  memory_region_init_ram_device_ptr().

- Adjust sequence to allocate gpu resource before virglrender resource
  creation

- Add virtio migration handling for uuid.

- Send kernel patch to define VIRTIO_GPU_CAPSET_VENUS.
  https://lore.kernel.org/lkml/20230915105918.3763061-1-ray.huang@amd.com/

- Add meson check to make sure unstable APIs defined from 0.9.0.

Changes from V1 to V2 (virtio gpu V4)

- Remove unused #include "hw/virtio/virtio-iommu.h"

- Add a local function, called virgl_resource_destroy(), that is used
  to release a vgpu resource on error paths and in resource_unref.

- Remove virtio_gpu_virgl_resource_unmap from
  virtio_gpu_cleanup_mapping(),
  since this function won't be called on blob resources and also because
  blob resources are unmapped via virgl_cmd_resource_unmap_blob().

- In virgl_cmd_resource_create_blob(), do proper cleanup in error paths
  and move QTAILQ_INSERT_HEAD(&g->reslist, res, next) after the resource
  has been fully initialized.

- Memory region has a different life-cycle from virtio gpu resources
  i.e. cannot be released synchronously along with the vgpu resource.
  So, here the field "region" was changed to a pointer and is allocated
  dynamically when the blob is mapped.
  Also, since the pointer can be used to indicate whether the blob
  is mapped, the explicite field "mapped" was removed.

- In virgl_cmd_resource_map_blob(), add check on the value of
  res->region, to prevent beeing called twice on the same resource.

- Add a patch to enable automatic deallocation of memory regions to resolve
  use-after-free memory corruption with a reference.

References

Demo with Venus:
- https://static.sched.com/hosted_files/xen2023/3f/xen_summit_2023_virtgpu_demo.mp4
QEMU repository:
- https://gitlab.freedesktop.org/rui/qemu-xen/-/commits/upstream-for-virtio-gpu

Thanks,
Ray

Antonio Caggiano (6):
  virtio-gpu: CONTEXT_INIT feature
  virtio-gpu: blob prep
  virtio-gpu: Handle resource blob commands
  virtio-gpu: Resource UUID
  virtio-gpu: Support Venus capset
  virtio-gpu: Initialize Venus

Dmitry Osipenko (1):
  virtio-gpu: Don't require udmabuf when blobs and virgl are enabled

Dr. David Alan Gilbert (1):
  virtio: Add shared memory capability

Gerd Hoffmann (1):
  virtio-gpu: hostmem

Huang Rui (3):
  virtio-gpu: Configure context init for virglrenderer
  virtio-gpu: Support context init feature with virglrenderer
  virtio-gpu: Enable virglrenderer render server flag for venus

Xenia Ragiadakou (1):
  softmmu/memory: enable automatic deallocation of memory regions

 hw/display/trace-events              |   1 +
 hw/display/virtio-gpu-base.c         |   5 +
 hw/display/virtio-gpu-pci.c          |  14 ++
 hw/display/virtio-gpu-virgl.c        | 272 ++++++++++++++++++++++++++-
 hw/display/virtio-gpu.c              |  78 +++++++-
 hw/display/virtio-vga.c              |  33 +++-
 hw/virtio/virtio-pci.c               |  18 ++
 include/hw/virtio/virtio-gpu-bswap.h |  15 ++
 include/hw/virtio/virtio-gpu.h       |  24 +++
 include/hw/virtio/virtio-pci.h       |   4 +
 meson.build                          |  13 ++
 softmmu/memory.c                     |   5 +
 12 files changed, 458 insertions(+), 24 deletions(-)

-- 
2.34.1

[QEMU PATCH v5 01/13] virtio: Add shared memory capability

[Huang Rui]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Define a new capability type 'VIRTIO_PCI_CAP_SHARED_MEMORY_CFG' to allow
defining shared memory regions with sizes and offsets of 2^32 and more.
Multiple instances of the capability are allowed and distinguished
by a device-specific 'id'.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
Tested-by: Alyssa Ross <hi@alyssa.is>
Tested-by: Huang Rui <ray.huang@amd.com>
Tested-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Acked-by: Huang Rui <ray.huang@amd.com>
Reviewed-by: Gurchetan Singh <gurchetansingh@chromium.org>
Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

This patch is already under review as part of the "rutabaga_gfx + gfxstream"
series (already in v13) and has been included here because of dependency.

 hw/virtio/virtio-pci.c         | 18 ++++++++++++++++++
 include/hw/virtio/virtio-pci.h |  4 ++++
 2 files changed, 22 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index edbc0daa18..da8c9ea12d 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1435,6 +1435,24 @@ static int virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy,
     return offset;
 }
 
+int virtio_pci_add_shm_cap(VirtIOPCIProxy *proxy,
+                           uint8_t bar, uint64_t offset, uint64_t length,
+                           uint8_t id)
+{
+    struct virtio_pci_cap64 cap = {
+        .cap.cap_len = sizeof cap,
+        .cap.cfg_type = VIRTIO_PCI_CAP_SHARED_MEMORY_CFG,
+    };
+
+    cap.cap.bar = bar;
+    cap.cap.length = cpu_to_le32(length);
+    cap.length_hi = cpu_to_le32(length >> 32);
+    cap.cap.offset = cpu_to_le32(offset);
+    cap.offset_hi = cpu_to_le32(offset >> 32);
+    cap.cap.id = id;
+    return virtio_pci_add_mem_cap(proxy, &cap.cap);
+}
+
 static uint64_t virtio_pci_common_read(void *opaque, hwaddr addr,
                                        unsigned size)
 {
diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
index ab2051b64b..5a3f182f99 100644
--- a/include/hw/virtio/virtio-pci.h
+++ b/include/hw/virtio/virtio-pci.h
@@ -264,4 +264,8 @@ unsigned virtio_pci_optimal_num_queues(unsigned fixed_queues);
 void virtio_pci_set_guest_notifier_fd_handler(VirtIODevice *vdev, VirtQueue *vq,
                                               int n, bool assign,
                                               bool with_irqfd);
+
+int virtio_pci_add_shm_cap(VirtIOPCIProxy *proxy, uint8_t bar, uint64_t offset,
+                           uint64_t length, uint8_t id);
+
 #endif
-- 
2.34.1

[QEMU PATCH v5 02/13] virtio-gpu: CONTEXT_INIT feature

[Huang Rui]

From: Antonio Caggiano <antonio.caggiano@collabora.com>

The feature can be enabled when a backend wants it.

Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
Tested-by: Alyssa Ross <hi@alyssa.is>
Tested-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Tested-by: Huang Rui <ray.huang@amd.com>
Acked-by: Huang Rui <ray.huang@amd.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

This patch is already under review as part of the "rutabaga_gfx + gfxstream"
series (already in v13) and has been included here because of dependency.

 hw/display/virtio-gpu-base.c   | 3 +++
 include/hw/virtio/virtio-gpu.h | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index ca1fb7b16f..4f2b0ba1f3 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -232,6 +232,9 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
     if (virtio_gpu_blob_enabled(g->conf)) {
         features |= (1 << VIRTIO_GPU_F_RESOURCE_BLOB);
     }
+    if (virtio_gpu_context_init_enabled(g->conf)) {
+        features |= (1 << VIRTIO_GPU_F_CONTEXT_INIT);
+    }
 
     return features;
 }
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 390c4642b8..8377c365ef 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -93,6 +93,7 @@ enum virtio_gpu_base_conf_flags {
     VIRTIO_GPU_FLAG_EDID_ENABLED,
     VIRTIO_GPU_FLAG_DMABUF_ENABLED,
     VIRTIO_GPU_FLAG_BLOB_ENABLED,
+    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED,
 };
 
 #define virtio_gpu_virgl_enabled(_cfg) \
@@ -105,6 +106,8 @@ enum virtio_gpu_base_conf_flags {
     (_cfg.flags & (1 << VIRTIO_GPU_FLAG_DMABUF_ENABLED))
 #define virtio_gpu_blob_enabled(_cfg) \
     (_cfg.flags & (1 << VIRTIO_GPU_FLAG_BLOB_ENABLED))
+#define virtio_gpu_context_init_enabled(_cfg) \
+    (_cfg.flags & (1 << VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED))
 
 struct virtio_gpu_base_conf {
     uint32_t max_outputs;
-- 
2.34.1

[QEMU PATCH v5 03/13] virtio-gpu: hostmem

[Huang Rui]

From: Gerd Hoffmann <kraxel@redhat.com>

Use VIRTIO_GPU_SHM_ID_HOST_VISIBLE as id for virtio-gpu.

Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Tested-by: Alyssa Ross <hi@alyssa.is>
Tested-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Tested-by: Huang Rui <ray.huang@amd.com>
Acked-by: Huang Rui <ray.huang@amd.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

This patch is already under review as part of the "rutabaga_gfx + gfxstream"
series (already in v13) and has been included here because of dependency.

 hw/display/virtio-gpu-pci.c    | 14 ++++++++++++++
 hw/display/virtio-gpu.c        |  1 +
 hw/display/virtio-vga.c        | 33 ++++++++++++++++++++++++---------
 include/hw/virtio/virtio-gpu.h |  5 +++++
 4 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
index 93f214ff58..da6a99f038 100644
--- a/hw/display/virtio-gpu-pci.c
+++ b/hw/display/virtio-gpu-pci.c
@@ -33,6 +33,20 @@ static void virtio_gpu_pci_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     DeviceState *vdev = DEVICE(g);
     int i;
 
+    if (virtio_gpu_hostmem_enabled(g->conf)) {
+        vpci_dev->msix_bar_idx = 1;
+        vpci_dev->modern_mem_bar_idx = 2;
+        memory_region_init(&g->hostmem, OBJECT(g), "virtio-gpu-hostmem",
+                           g->conf.hostmem);
+        pci_register_bar(&vpci_dev->pci_dev, 4,
+                         PCI_BASE_ADDRESS_SPACE_MEMORY |
+                         PCI_BASE_ADDRESS_MEM_PREFETCH |
+                         PCI_BASE_ADDRESS_MEM_TYPE_64,
+                         &g->hostmem);
+        virtio_pci_add_shm_cap(vpci_dev, 4, 0, g->conf.hostmem,
+                               VIRTIO_GPU_SHM_ID_HOST_VISIBLE);
+    }
+
     virtio_pci_force_virtio_1(vpci_dev);
     if (!qdev_realize(vdev, BUS(&vpci_dev->bus), errp)) {
         return;
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index bbd5c6561a..48ef0d9fad 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1509,6 +1509,7 @@ static Property virtio_gpu_properties[] = {
                      256 * MiB),
     DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
                     VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
+    DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
index e6fb0aa876..c8552ff760 100644
--- a/hw/display/virtio-vga.c
+++ b/hw/display/virtio-vga.c
@@ -115,17 +115,32 @@ static void virtio_vga_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     pci_register_bar(&vpci_dev->pci_dev, 0,
                      PCI_BASE_ADDRESS_MEM_PREFETCH, &vga->vram);
 
-    /*
-     * Configure virtio bar and regions
-     *
-     * We use bar #2 for the mmio regions, to be compatible with stdvga.
-     * virtio regions are moved to the end of bar #2, to make room for
-     * the stdvga mmio registers at the start of bar #2.
-     */
-    vpci_dev->modern_mem_bar_idx = 2;
-    vpci_dev->msix_bar_idx = 4;
     vpci_dev->modern_io_bar_idx = 5;
 
+    if (!virtio_gpu_hostmem_enabled(g->conf)) {
+        /*
+         * Configure virtio bar and regions
+         *
+         * We use bar #2 for the mmio regions, to be compatible with stdvga.
+         * virtio regions are moved to the end of bar #2, to make room for
+         * the stdvga mmio registers at the start of bar #2.
+         */
+        vpci_dev->modern_mem_bar_idx = 2;
+        vpci_dev->msix_bar_idx = 4;
+    } else {
+        vpci_dev->msix_bar_idx = 1;
+        vpci_dev->modern_mem_bar_idx = 2;
+        memory_region_init(&g->hostmem, OBJECT(g), "virtio-gpu-hostmem",
+                           g->conf.hostmem);
+        pci_register_bar(&vpci_dev->pci_dev, 4,
+                         PCI_BASE_ADDRESS_SPACE_MEMORY |
+                         PCI_BASE_ADDRESS_MEM_PREFETCH |
+                         PCI_BASE_ADDRESS_MEM_TYPE_64,
+                         &g->hostmem);
+        virtio_pci_add_shm_cap(vpci_dev, 4, 0, g->conf.hostmem,
+                               VIRTIO_GPU_SHM_ID_HOST_VISIBLE);
+    }
+
     if (!(vpci_dev->flags & VIRTIO_PCI_FLAG_PAGE_PER_VQ)) {
         /*
          * with page-per-vq=off there is no padding space we can use
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 8377c365ef..de4f624e94 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -108,12 +108,15 @@ enum virtio_gpu_base_conf_flags {
     (_cfg.flags & (1 << VIRTIO_GPU_FLAG_BLOB_ENABLED))
 #define virtio_gpu_context_init_enabled(_cfg) \
     (_cfg.flags & (1 << VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED))
+#define virtio_gpu_hostmem_enabled(_cfg) \
+    (_cfg.hostmem > 0)
 
 struct virtio_gpu_base_conf {
     uint32_t max_outputs;
     uint32_t flags;
     uint32_t xres;
     uint32_t yres;
+    uint64_t hostmem;
 };
 
 struct virtio_gpu_ctrl_command {
@@ -137,6 +140,8 @@ struct VirtIOGPUBase {
     int renderer_blocked;
     int enable;
 
+    MemoryRegion hostmem;
+
     struct virtio_gpu_scanout scanout[VIRTIO_GPU_MAX_SCANOUTS];
 
     int enabled_output_bitmask;
-- 
2.34.1

[QEMU PATCH v5 04/13] virtio-gpu: blob prep

[Huang Rui]

From: Antonio Caggiano <antonio.caggiano@collabora.com>

This adds preparatory functions needed to:

     - decode blob cmds
     - tracking iovecs

Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
Tested-by: Alyssa Ross <hi@alyssa.is>
Tested-by: Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org>
Tested-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Tested-by: Huang Rui <ray.huang@amd.com>
Acked-by: Huang Rui <ray.huang@amd.com>
Reviewed-by: Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org>
Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

This patch is already under review as part of the "rutabaga_gfx + gfxstream"
series (already in v13) and has been included here because of dependency.

 hw/display/virtio-gpu.c              | 10 +++-------
 include/hw/virtio/virtio-gpu-bswap.h | 15 +++++++++++++++
 include/hw/virtio/virtio-gpu.h       |  5 +++++
 3 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 48ef0d9fad..3e658f1fef 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -33,15 +33,11 @@
 
 #define VIRTIO_GPU_VM_VERSION 1
 
-static struct virtio_gpu_simple_resource*
-virtio_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id);
 static struct virtio_gpu_simple_resource *
 virtio_gpu_find_check_resource(VirtIOGPU *g, uint32_t resource_id,
                                bool require_backing,
                                const char *caller, uint32_t *error);
 
-static void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
-                                       struct virtio_gpu_simple_resource *res);
 static void virtio_gpu_reset_bh(void *opaque);
 
 void virtio_gpu_update_cursor_data(VirtIOGPU *g,
@@ -116,7 +112,7 @@ static void update_cursor(VirtIOGPU *g, struct virtio_gpu_update_cursor *cursor)
                   cursor->resource_id ? 1 : 0);
 }
 
-static struct virtio_gpu_simple_resource *
+struct virtio_gpu_simple_resource *
 virtio_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id)
 {
     struct virtio_gpu_simple_resource *res;
@@ -904,8 +900,8 @@ void virtio_gpu_cleanup_mapping_iov(VirtIOGPU *g,
     g_free(iov);
 }
 
-static void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
-                                       struct virtio_gpu_simple_resource *res)
+void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
+                                struct virtio_gpu_simple_resource *res)
 {
     virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt);
     res->iov = NULL;
diff --git a/include/hw/virtio/virtio-gpu-bswap.h b/include/hw/virtio/virtio-gpu-bswap.h
index 637a0585d0..dd1975e2d4 100644
--- a/include/hw/virtio/virtio-gpu-bswap.h
+++ b/include/hw/virtio/virtio-gpu-bswap.h
@@ -70,6 +70,21 @@ virtio_gpu_create_blob_bswap(struct virtio_gpu_resource_create_blob *cblob)
     le64_to_cpus(&cblob->size);
 }
 
+static inline void
+virtio_gpu_map_blob_bswap(struct virtio_gpu_resource_map_blob *mblob)
+{
+    virtio_gpu_ctrl_hdr_bswap(&mblob->hdr);
+    le32_to_cpus(&mblob->resource_id);
+    le64_to_cpus(&mblob->offset);
+}
+
+static inline void
+virtio_gpu_unmap_blob_bswap(struct virtio_gpu_resource_unmap_blob *ublob)
+{
+    virtio_gpu_ctrl_hdr_bswap(&ublob->hdr);
+    le32_to_cpus(&ublob->resource_id);
+}
+
 static inline void
 virtio_gpu_scanout_blob_bswap(struct virtio_gpu_set_scanout_blob *ssb)
 {
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index de4f624e94..55973e112f 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -257,6 +257,9 @@ void virtio_gpu_base_fill_display_info(VirtIOGPUBase *g,
 void virtio_gpu_base_generate_edid(VirtIOGPUBase *g, int scanout,
                                    struct virtio_gpu_resp_edid *edid);
 /* virtio-gpu.c */
+struct virtio_gpu_simple_resource *
+virtio_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id);
+
 void virtio_gpu_ctrl_response(VirtIOGPU *g,
                               struct virtio_gpu_ctrl_command *cmd,
                               struct virtio_gpu_ctrl_hdr *resp,
@@ -275,6 +278,8 @@ int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
                                   uint32_t *niov);
 void virtio_gpu_cleanup_mapping_iov(VirtIOGPU *g,
                                     struct iovec *iov, uint32_t count);
+void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
+                                struct virtio_gpu_simple_resource *res);
 void virtio_gpu_process_cmdq(VirtIOGPU *g);
 void virtio_gpu_device_realize(DeviceState *qdev, Error **errp);
 void virtio_gpu_reset(VirtIODevice *vdev);
-- 
2.34.1

[QEMU PATCH v5 05/13] virtio-gpu: Configure context init for virglrenderer

[Huang Rui]

Configure context init feature flag for virglrenderer.

Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

V4 -> V5:
    - Inverted patch 5 and 6 because we should configure
      HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)

 meson.build | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/meson.build b/meson.build
index 98e68ef0b1..ff20d3c249 100644
--- a/meson.build
+++ b/meson.build
@@ -1068,6 +1068,10 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
                                        prefix: '#include <virglrenderer.h>',
                                        dependencies: virgl))
   endif
+  config_host_data.set('HAVE_VIRGL_CONTEXT_INIT',
+                       cc.has_function('virgl_renderer_context_create_with_flags',
+                                       prefix: '#include <virglrenderer.h>',
+                                       dependencies: virgl))
 endif
 blkio = not_found
 if not get_option('blkio').auto() or have_block
-- 
2.34.1

Re: [QEMU PATCH v5 05/13] virtio-gpu: Configure context init for virglrenderer

[Marc-André Lureau]

Hi

On Fri, Sep 15, 2023 at 6:16 PM Huang Rui <ray.huang@amd.com> wrote:
>
> Configure context init feature flag for virglrenderer.
>
> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
>
> V4 -> V5:
>     - Inverted patch 5 and 6 because we should configure
>       HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
>
>  meson.build | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/meson.build b/meson.build
> index 98e68ef0b1..ff20d3c249 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1068,6 +1068,10 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
>                                         prefix: '#include <virglrenderer.h>',
>                                         dependencies: virgl))
>    endif
> +  config_host_data.set('HAVE_VIRGL_CONTEXT_INIT',
> +                       cc.has_function('virgl_renderer_context_create_with_flags',
> +                                       prefix: '#include <virglrenderer.h>',
> +                                       dependencies: virgl))

Move it under the "if virgl.found()" block above.

I suggest to name it after what is actually checked:
HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS for ex

>  endif
>  blkio = not_found
>  if not get_option('blkio').auto() or have_block
> --
> 2.34.1
>
>


-- 
Marc-André Lureau

Re: [QEMU PATCH v5 05/13] virtio-gpu: Configure context init for virglrenderer

[Huang Rui]

On Tue, Sep 19, 2023 at 04:17:43PM +0800, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Sep 15, 2023 at 6:16 PM Huang Rui <ray.huang@amd.com> wrote:
> >
> > Configure context init feature flag for virglrenderer.
> >
> > Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> >
> > V4 -> V5:
> >     - Inverted patch 5 and 6 because we should configure
> >       HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
> >
> >  meson.build | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/meson.build b/meson.build
> > index 98e68ef0b1..ff20d3c249 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -1068,6 +1068,10 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
> >                                         prefix: '#include <virglrenderer.h>',
> >                                         dependencies: virgl))
> >    endif
> > +  config_host_data.set('HAVE_VIRGL_CONTEXT_INIT',
> > +                       cc.has_function('virgl_renderer_context_create_with_flags',
> > +                                       prefix: '#include <virglrenderer.h>',
> > +                                       dependencies: virgl))
> 
> Move it under the "if virgl.found()" block above.
> 
> I suggest to name it after what is actually checked:
> HAVE_VIRGL_CONTEXT_CREATE_WITH_FLAGS for ex
> 

OK, will update it in V6.

Thanks,
Ray

> >  endif
> >  blkio = not_found
> >  if not get_option('blkio').auto() or have_block
> > --
> > 2.34.1
> >
> >
> 
> 
> -- 
> Marc-André Lureau

Re: [QEMU PATCH v5 05/13] virtio-gpu: Configure context init for virglrenderer

[Dmitry Osipenko]

On 9/15/23 14:11, Huang Rui wrote:
> Configure context init feature flag for virglrenderer.
> 
> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
> 
> V4 -> V5:
>     - Inverted patch 5 and 6 because we should configure
>       HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
> 
>  meson.build | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/meson.build b/meson.build
> index 98e68ef0b1..ff20d3c249 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1068,6 +1068,10 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
>                                         prefix: '#include <virglrenderer.h>',
>                                         dependencies: virgl))
>    endif
> +  config_host_data.set('HAVE_VIRGL_CONTEXT_INIT',
> +                       cc.has_function('virgl_renderer_context_create_with_flags',
> +                                       prefix: '#include <virglrenderer.h>',
> +                                       dependencies: virgl))
The "cc.has_function" doesn't work properly with PKG_CONFIG_PATH. It ignores the the given pkg and uses system includes. Antonio was aware about that problem [1].

[1] https://gitlab.freedesktop.org/Fahien/qemu/-/commit/ea1c252a707940983ccce71e92a292b49496bfcd

Given that virglrenderer 1.0 has been released couple weeks ago, can we make the v1.0 a mandatory requirement for qemu and remove all the ifdefs? I doubt that anyone is going to test newer qemu using older libviglrenderer, all that ifdef code will be bitrotting.

@@ -1060,6 +1060,7 @@ virgl = not_found
 have_vhost_user_gpu = have_tools and targetos == 'linux' and pixman.found()
 if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
   virgl = dependency('virglrenderer',
+                     version: '>=1.0.0',
                      method: 'pkg-config',
                      required: get_option('virglrenderer'))
   if virgl.found()


Best regards,
Dmitry

Re: [QEMU PATCH v5 05/13] virtio-gpu: Configure context init for virglrenderer

[Daniel P. Berrangé]

On Tue, Oct 10, 2023 at 02:41:22PM +0300, Dmitry Osipenko wrote:
> On 9/15/23 14:11, Huang Rui wrote:
> > Configure context init feature flag for virglrenderer.
> > 
> > Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> > 
> > V4 -> V5:
> >     - Inverted patch 5 and 6 because we should configure
> >       HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
> > 
> >  meson.build | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/meson.build b/meson.build
> > index 98e68ef0b1..ff20d3c249 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -1068,6 +1068,10 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
> >                                         prefix: '#include <virglrenderer.h>',
> >                                         dependencies: virgl))
> >    endif
> > +  config_host_data.set('HAVE_VIRGL_CONTEXT_INIT',
> > +                       cc.has_function('virgl_renderer_context_create_with_flags',
> > +                                       prefix: '#include <virglrenderer.h>',
> > +                                       dependencies: virgl))
> The "cc.has_function" doesn't work properly with PKG_CONFIG_PATH. It ignores the the given pkg and uses system includes. Antonio was aware about that problem [1].
> 
> [1] https://gitlab.freedesktop.org/Fahien/qemu/-/commit/ea1c252a707940983ccce71e92a292b49496bfcd
> 
> Given that virglrenderer 1.0 has been released couple weeks ago,
> can we make the v1.0 a mandatory requirement for qemu and remove
> all the ifdefs? I doubt that anyone is going to test newer qemu
> using older libviglrenderer, all that ifdef code will be bitrotting.

We cannot do that. If is is only weeks old, no distro will
have virglrenderer 1.0 present. QEMU has a defined set of
platforms that it targets compatibility with:

  https://www.qemu.org/docs/master/about/build-platforms.html

For newly added functionality we can set the min version to
something newer than the oldest QEMU target platform.

For existing functionality though, we must not regress wrt
the currently supported set of target platforms. So we can
only bump the min version to that present in the oldest
platform we target.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [QEMU PATCH v5 05/13] virtio-gpu: Configure context init for virglrenderer

[Dmitry Osipenko]

On 10/10/23 14:51, Daniel P. Berrangé wrote:
> On Tue, Oct 10, 2023 at 02:41:22PM +0300, Dmitry Osipenko wrote:
>> On 9/15/23 14:11, Huang Rui wrote:
>>> Configure context init feature flag for virglrenderer.
>>>
>>> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>> ---
>>>
>>> V4 -> V5:
>>>     - Inverted patch 5 and 6 because we should configure
>>>       HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
>>>
>>>  meson.build | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/meson.build b/meson.build
>>> index 98e68ef0b1..ff20d3c249 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -1068,6 +1068,10 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
>>>                                         prefix: '#include <virglrenderer.h>',
>>>                                         dependencies: virgl))
>>>    endif
>>> +  config_host_data.set('HAVE_VIRGL_CONTEXT_INIT',
>>> +                       cc.has_function('virgl_renderer_context_create_with_flags',
>>> +                                       prefix: '#include <virglrenderer.h>',
>>> +                                       dependencies: virgl))
>> The "cc.has_function" doesn't work properly with PKG_CONFIG_PATH. It ignores the the given pkg and uses system includes. Antonio was aware about that problem [1].
>>
>> [1] https://gitlab.freedesktop.org/Fahien/qemu/-/commit/ea1c252a707940983ccce71e92a292b49496bfcd
>>
>> Given that virglrenderer 1.0 has been released couple weeks ago,
>> can we make the v1.0 a mandatory requirement for qemu and remove
>> all the ifdefs? I doubt that anyone is going to test newer qemu
>> using older libviglrenderer, all that ifdef code will be bitrotting.
> 
> We cannot do that. If is is only weeks old, no distro will
> have virglrenderer 1.0 present. QEMU has a defined set of
> platforms that it targets compatibility with:
> 
>   https://www.qemu.org/docs/master/about/build-platforms.html
> 
> For newly added functionality we can set the min version to
> something newer than the oldest QEMU target platform.
> 
> For existing functionality though, we must not regress wrt
> the currently supported set of target platforms. So we can
> only bump the min version to that present in the oldest
> platform we target.

Well, then alternatively we could specify supported libvirglrender
features based on the lib version to avoid those pkgconfig+meson troubles.

-- 
Best regards,
Dmitry

[QEMU PATCH v5 06/13] virtio-gpu: Support context init feature with virglrenderer

[Huang Rui]

Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init
feature flags.
We would like to enable the feature with virglrenderer, so add to create
virgl renderer context with flags using context_id when valid.

Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

V4 -> V5:
    - Inverted patch 5 and 6 because we should configure
      HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)

 hw/display/virtio-gpu-virgl.c | 13 +++++++++++--
 hw/display/virtio-gpu.c       |  2 ++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 8bb7a2c21f..312953ec16 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
     trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
                                     cc.debug_name);
 
-    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
-                                  cc.debug_name);
+    if (cc.context_init) {
+#ifdef HAVE_VIRGL_CONTEXT_INIT
+        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
+                                                 cc.context_init,
+                                                 cc.nlen,
+                                                 cc.debug_name);
+        return;
+#endif
+    }
+
+    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name);
 }
 
 static void virgl_cmd_context_destroy(VirtIOGPU *g,
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 3e658f1fef..a66cbd9930 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1506,6 +1506,8 @@ static Property virtio_gpu_properties[] = {
     DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
                     VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
     DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
+    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
+                    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.34.1

Re: [QEMU PATCH v5 06/13] virtio-gpu: Support context init feature with virglrenderer

[Akihiko Odaki]

On 2023/09/15 20:11, Huang Rui wrote:
> Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init
> feature flags.
> We would like to enable the feature with virglrenderer, so add to create
> virgl renderer context with flags using context_id when valid.
> 
> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
> 
> V4 -> V5:
>      - Inverted patch 5 and 6 because we should configure
>        HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
> 
>   hw/display/virtio-gpu-virgl.c | 13 +++++++++++--
>   hw/display/virtio-gpu.c       |  2 ++
>   2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 8bb7a2c21f..312953ec16 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
>       trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
>                                       cc.debug_name);
>   
> -    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
> -                                  cc.debug_name);
> +    if (cc.context_init) {
> +#ifdef HAVE_VIRGL_CONTEXT_INIT
> +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
> +                                                 cc.context_init,
> +                                                 cc.nlen,
> +                                                 cc.debug_name);
> +        return;
> +#endif

This should deal with the case when context_init is set while 
HAVE_VIRGL_CONTEXT_INIT is not defined.

Re: [QEMU PATCH v5 06/13] virtio-gpu: Support context init feature with virglrenderer

[Huang Rui]

On Fri, Sep 15, 2023 at 11:20:46PM +0800, Akihiko Odaki wrote:
> On 2023/09/15 20:11, Huang Rui wrote:
> > Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init
> > feature flags.
> > We would like to enable the feature with virglrenderer, so add to create
> > virgl renderer context with flags using context_id when valid.
> > 
> > Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> > 
> > V4 -> V5:
> >      - Inverted patch 5 and 6 because we should configure
> >        HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
> > 
> >   hw/display/virtio-gpu-virgl.c | 13 +++++++++++--
> >   hw/display/virtio-gpu.c       |  2 ++
> >   2 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> > index 8bb7a2c21f..312953ec16 100644
> > --- a/hw/display/virtio-gpu-virgl.c
> > +++ b/hw/display/virtio-gpu-virgl.c
> > @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
> >       trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
> >                                       cc.debug_name);
> >   
> > -    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
> > -                                  cc.debug_name);
> > +    if (cc.context_init) {
> > +#ifdef HAVE_VIRGL_CONTEXT_INIT
> > +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
> > +                                                 cc.context_init,
> > +                                                 cc.nlen,
> > +                                                 cc.debug_name);
> > +        return;
> > +#endif
> 
> This should deal with the case when context_init is set while 
> HAVE_VIRGL_CONTEXT_INIT is not defined.

Actually, I received the comment below before:

https://lore.kernel.org/qemu-devel/32588d0e-a1f2-30c4-5e9f-e6e7c4190b65@linaro.org/

At original patch set, I have the case while HAVE_VIRGL_CONTEXT_INIT is set
but HAVE_VIRGL_CONTEXT_INIT is not defined. But I think we may encounter
the case that virgl_renderer_context_create_with_flags is not defined in
virglrenderer early version. Should I bring the error message back?

Thanks,
Ray

Re: [QEMU PATCH v5 06/13] virtio-gpu: Support context init feature with virglrenderer

[Akihiko Odaki]

On 2023/09/16 19:32, Huang Rui wrote:
> On Fri, Sep 15, 2023 at 11:20:46PM +0800, Akihiko Odaki wrote:
>> On 2023/09/15 20:11, Huang Rui wrote:
>>> Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init
>>> feature flags.
>>> We would like to enable the feature with virglrenderer, so add to create
>>> virgl renderer context with flags using context_id when valid.
>>>
>>> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>> ---
>>>
>>> V4 -> V5:
>>>       - Inverted patch 5 and 6 because we should configure
>>>         HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
>>>
>>>    hw/display/virtio-gpu-virgl.c | 13 +++++++++++--
>>>    hw/display/virtio-gpu.c       |  2 ++
>>>    2 files changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>>> index 8bb7a2c21f..312953ec16 100644
>>> --- a/hw/display/virtio-gpu-virgl.c
>>> +++ b/hw/display/virtio-gpu-virgl.c
>>> @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
>>>        trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
>>>                                        cc.debug_name);
>>>    
>>> -    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
>>> -                                  cc.debug_name);
>>> +    if (cc.context_init) {
>>> +#ifdef HAVE_VIRGL_CONTEXT_INIT
>>> +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
>>> +                                                 cc.context_init,
>>> +                                                 cc.nlen,
>>> +                                                 cc.debug_name);
>>> +        return;
>>> +#endif
>>
>> This should deal with the case when context_init is set while
>> HAVE_VIRGL_CONTEXT_INIT is not defined.
> 
> Actually, I received the comment below before:
> 
> https://lore.kernel.org/qemu-devel/32588d0e-a1f2-30c4-5e9f-e6e7c4190b65@linaro.org/
> 
> At original patch set, I have the case while HAVE_VIRGL_CONTEXT_INIT is set
> but HAVE_VIRGL_CONTEXT_INIT is not defined. But I think we may encounter
> the case that virgl_renderer_context_create_with_flags is not defined in
> virglrenderer early version. Should I bring the error message back?
> 
> Thanks,
> Ray

I suggest checking VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED instead of 
reporting an error here. Perhaps it may be easier to add #ifdef around:
 > +    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
 > +                    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, false),

Re: [QEMU PATCH v5 06/13] virtio-gpu: Support context init feature with virglrenderer

[Huang Rui]

On Sat, Sep 16, 2023 at 06:42:04PM +0800, Akihiko Odaki wrote:
> On 2023/09/16 19:32, Huang Rui wrote:
> > On Fri, Sep 15, 2023 at 11:20:46PM +0800, Akihiko Odaki wrote:
> >> On 2023/09/15 20:11, Huang Rui wrote:
> >>> Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init
> >>> feature flags.
> >>> We would like to enable the feature with virglrenderer, so add to create
> >>> virgl renderer context with flags using context_id when valid.
> >>>
> >>> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> >>> Signed-off-by: Huang Rui <ray.huang@amd.com>
> >>> ---
> >>>
> >>> V4 -> V5:
> >>>       - Inverted patch 5 and 6 because we should configure
> >>>         HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
> >>>
> >>>    hw/display/virtio-gpu-virgl.c | 13 +++++++++++--
> >>>    hw/display/virtio-gpu.c       |  2 ++
> >>>    2 files changed, 13 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> >>> index 8bb7a2c21f..312953ec16 100644
> >>> --- a/hw/display/virtio-gpu-virgl.c
> >>> +++ b/hw/display/virtio-gpu-virgl.c
> >>> @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
> >>>        trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
> >>>                                        cc.debug_name);
> >>>    
> >>> -    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
> >>> -                                  cc.debug_name);
> >>> +    if (cc.context_init) {
> >>> +#ifdef HAVE_VIRGL_CONTEXT_INIT
> >>> +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
> >>> +                                                 cc.context_init,
> >>> +                                                 cc.nlen,
> >>> +                                                 cc.debug_name);
> >>> +        return;
> >>> +#endif
> >>
> >> This should deal with the case when context_init is set while
> >> HAVE_VIRGL_CONTEXT_INIT is not defined.
> > 
> > Actually, I received the comment below before:
> > 
> > https://lore.kernel.org/qemu-devel/32588d0e-a1f2-30c4-5e9f-e6e7c4190b65@linaro.org/
> > 
> > At original patch set, I have the case while HAVE_VIRGL_CONTEXT_INIT is set
> > but HAVE_VIRGL_CONTEXT_INIT is not defined. But I think we may encounter
> > the case that virgl_renderer_context_create_with_flags is not defined in
> > virglrenderer early version. Should I bring the error message back?
> > 
> > Thanks,
> > Ray
> 
> I suggest checking VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED instead of 
> reporting an error here. Perhaps it may be easier to add #ifdef around:
>  > +    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
>  > +                    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, false),

How about below changes:

---
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 8bb7a2c21f..54a3cfe136 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -106,8 +106,15 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
     trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
                                     cc.debug_name);

-    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
-                                  cc.debug_name);
+    if (cc.context_init && virtio_gpu_context_init_enabled(g->conf)) {
+        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
+                                                 cc.context_init,
+                                                 cc.nlen,
+                                                 cc.debug_name);
+        return;
+    }
+
+    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name);
 }

 static void virgl_cmd_context_destroy(VirtIOGPU *g,
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index be16efbd38..6ff2c8e92d 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1508,6 +1508,10 @@ static Property virtio_gpu_properties[] = {
     DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
                     VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
     DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
+#ifdef HAVE_VIRGL_CONTEXT_INIT
+    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
+                    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, true),
+#endif
     DEFINE_PROP_END_OF_LIST(),
 };

Re: [QEMU PATCH v5 06/13] virtio-gpu: Support context init feature with virglrenderer

[Akihiko Odaki]

On 2023/09/17 14:45, Huang Rui wrote:
> On Sat, Sep 16, 2023 at 06:42:04PM +0800, Akihiko Odaki wrote:
>> On 2023/09/16 19:32, Huang Rui wrote:
>>> On Fri, Sep 15, 2023 at 11:20:46PM +0800, Akihiko Odaki wrote:
>>>> On 2023/09/15 20:11, Huang Rui wrote:
>>>>> Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init
>>>>> feature flags.
>>>>> We would like to enable the feature with virglrenderer, so add to create
>>>>> virgl renderer context with flags using context_id when valid.
>>>>>
>>>>> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>>>> ---
>>>>>
>>>>> V4 -> V5:
>>>>>        - Inverted patch 5 and 6 because we should configure
>>>>>          HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
>>>>>
>>>>>     hw/display/virtio-gpu-virgl.c | 13 +++++++++++--
>>>>>     hw/display/virtio-gpu.c       |  2 ++
>>>>>     2 files changed, 13 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>>>>> index 8bb7a2c21f..312953ec16 100644
>>>>> --- a/hw/display/virtio-gpu-virgl.c
>>>>> +++ b/hw/display/virtio-gpu-virgl.c
>>>>> @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
>>>>>         trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
>>>>>                                         cc.debug_name);
>>>>>     
>>>>> -    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
>>>>> -                                  cc.debug_name);
>>>>> +    if (cc.context_init) {
>>>>> +#ifdef HAVE_VIRGL_CONTEXT_INIT
>>>>> +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
>>>>> +                                                 cc.context_init,
>>>>> +                                                 cc.nlen,
>>>>> +                                                 cc.debug_name);
>>>>> +        return;
>>>>> +#endif
>>>>
>>>> This should deal with the case when context_init is set while
>>>> HAVE_VIRGL_CONTEXT_INIT is not defined.
>>>
>>> Actually, I received the comment below before:
>>>
>>> https://lore.kernel.org/qemu-devel/32588d0e-a1f2-30c4-5e9f-e6e7c4190b65@linaro.org/
>>>
>>> At original patch set, I have the case while HAVE_VIRGL_CONTEXT_INIT is set
>>> but HAVE_VIRGL_CONTEXT_INIT is not defined. But I think we may encounter
>>> the case that virgl_renderer_context_create_with_flags is not defined in
>>> virglrenderer early version. Should I bring the error message back?
>>>
>>> Thanks,
>>> Ray
>>
>> I suggest checking VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED instead of
>> reporting an error here. Perhaps it may be easier to add #ifdef around:
>>   > +    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
>>   > +                    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, false),
> 
> How about below changes: >
> ---
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 8bb7a2c21f..54a3cfe136 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -106,8 +106,15 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
>       trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
>                                       cc.debug_name);
> 
> -    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
> -                                  cc.debug_name);
> +    if (cc.context_init && virtio_gpu_context_init_enabled(g->conf)) {
> +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
> +                                                 cc.context_init,
> +                                                 cc.nlen,
> +                                                 cc.debug_name);
> +        return;
> +    }
> +
> +    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name);
>   }
> 
>   static void virgl_cmd_context_destroy(VirtIOGPU *g,
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index be16efbd38..6ff2c8e92d 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1508,6 +1508,10 @@ static Property virtio_gpu_properties[] = {
>       DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
>                       VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
>       DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
> +#ifdef HAVE_VIRGL_CONTEXT_INIT
> +    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
> +                    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, true),
> +#endif
>       DEFINE_PROP_END_OF_LIST(),
>   };
> 

It looks better, but not having #ifdef around 
virgl_renderer_context_create_with_flags() will result in a link error 
with old virglrenderer.

Re: [QEMU PATCH v5 06/13] virtio-gpu: Support context init feature with virglrenderer

[Huang Rui]

On Sun, Sep 17, 2023 at 01:49:19PM +0800, Akihiko Odaki wrote:
> On 2023/09/17 14:45, Huang Rui wrote:
> > On Sat, Sep 16, 2023 at 06:42:04PM +0800, Akihiko Odaki wrote:
> >> On 2023/09/16 19:32, Huang Rui wrote:
> >>> On Fri, Sep 15, 2023 at 11:20:46PM +0800, Akihiko Odaki wrote:
> >>>> On 2023/09/15 20:11, Huang Rui wrote:
> >>>>> Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init
> >>>>> feature flags.
> >>>>> We would like to enable the feature with virglrenderer, so add to create
> >>>>> virgl renderer context with flags using context_id when valid.
> >>>>>
> >>>>> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> >>>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
> >>>>> ---
> >>>>>
> >>>>> V4 -> V5:
> >>>>>        - Inverted patch 5 and 6 because we should configure
> >>>>>          HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
> >>>>>
> >>>>>     hw/display/virtio-gpu-virgl.c | 13 +++++++++++--
> >>>>>     hw/display/virtio-gpu.c       |  2 ++
> >>>>>     2 files changed, 13 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> >>>>> index 8bb7a2c21f..312953ec16 100644
> >>>>> --- a/hw/display/virtio-gpu-virgl.c
> >>>>> +++ b/hw/display/virtio-gpu-virgl.c
> >>>>> @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
> >>>>>         trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
> >>>>>                                         cc.debug_name);
> >>>>>     
> >>>>> -    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
> >>>>> -                                  cc.debug_name);
> >>>>> +    if (cc.context_init) {
> >>>>> +#ifdef HAVE_VIRGL_CONTEXT_INIT
> >>>>> +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
> >>>>> +                                                 cc.context_init,
> >>>>> +                                                 cc.nlen,
> >>>>> +                                                 cc.debug_name);
> >>>>> +        return;
> >>>>> +#endif
> >>>>
> >>>> This should deal with the case when context_init is set while
> >>>> HAVE_VIRGL_CONTEXT_INIT is not defined.
> >>>
> >>> Actually, I received the comment below before:
> >>>
> >>> https://lore.kernel.org/qemu-devel/32588d0e-a1f2-30c4-5e9f-e6e7c4190b65@linaro.org/
> >>>
> >>> At original patch set, I have the case while HAVE_VIRGL_CONTEXT_INIT is set
> >>> but HAVE_VIRGL_CONTEXT_INIT is not defined. But I think we may encounter
> >>> the case that virgl_renderer_context_create_with_flags is not defined in
> >>> virglrenderer early version. Should I bring the error message back?
> >>>
> >>> Thanks,
> >>> Ray
> >>
> >> I suggest checking VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED instead of
> >> reporting an error here. Perhaps it may be easier to add #ifdef around:
> >>   > +    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
> >>   > +                    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, false),
> > 
> > How about below changes: >
> > ---
> > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> > index 8bb7a2c21f..54a3cfe136 100644
> > --- a/hw/display/virtio-gpu-virgl.c
> > +++ b/hw/display/virtio-gpu-virgl.c
> > @@ -106,8 +106,15 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
> >       trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
> >                                       cc.debug_name);
> > 
> > -    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
> > -                                  cc.debug_name);
> > +    if (cc.context_init && virtio_gpu_context_init_enabled(g->conf)) {
> > +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
> > +                                                 cc.context_init,
> > +                                                 cc.nlen,
> > +                                                 cc.debug_name);
> > +        return;
> > +    }
> > +
> > +    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name);
> >   }
> > 
> >   static void virgl_cmd_context_destroy(VirtIOGPU *g,
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index be16efbd38..6ff2c8e92d 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -1508,6 +1508,10 @@ static Property virtio_gpu_properties[] = {
> >       DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
> >                       VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
> >       DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
> > +#ifdef HAVE_VIRGL_CONTEXT_INIT
> > +    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
> > +                    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, true),
> > +#endif
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> > 
> 
> It looks better, but not having #ifdef around 
> virgl_renderer_context_create_with_flags() will result in a link error 
> with old virglrenderer.

Hmm, right, it seems that we have to have a "#ifdef" around here. Or do you
have any better idea?

Thanks,
Ray

Re: [QEMU PATCH v5 06/13] virtio-gpu: Support context init feature with virglrenderer

[Akihiko Odaki]

On 2023/09/18 14:43, Huang Rui wrote:
> On Sun, Sep 17, 2023 at 01:49:19PM +0800, Akihiko Odaki wrote:
>> On 2023/09/17 14:45, Huang Rui wrote:
>>> On Sat, Sep 16, 2023 at 06:42:04PM +0800, Akihiko Odaki wrote:
>>>> On 2023/09/16 19:32, Huang Rui wrote:
>>>>> On Fri, Sep 15, 2023 at 11:20:46PM +0800, Akihiko Odaki wrote:
>>>>>> On 2023/09/15 20:11, Huang Rui wrote:
>>>>>>> Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init
>>>>>>> feature flags.
>>>>>>> We would like to enable the feature with virglrenderer, so add to create
>>>>>>> virgl renderer context with flags using context_id when valid.
>>>>>>>
>>>>>>> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
>>>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>>>>>> ---
>>>>>>>
>>>>>>> V4 -> V5:
>>>>>>>         - Inverted patch 5 and 6 because we should configure
>>>>>>>           HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
>>>>>>>
>>>>>>>      hw/display/virtio-gpu-virgl.c | 13 +++++++++++--
>>>>>>>      hw/display/virtio-gpu.c       |  2 ++
>>>>>>>      2 files changed, 13 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>>>>>>> index 8bb7a2c21f..312953ec16 100644
>>>>>>> --- a/hw/display/virtio-gpu-virgl.c
>>>>>>> +++ b/hw/display/virtio-gpu-virgl.c
>>>>>>> @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
>>>>>>>          trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
>>>>>>>                                          cc.debug_name);
>>>>>>>      
>>>>>>> -    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
>>>>>>> -                                  cc.debug_name);
>>>>>>> +    if (cc.context_init) {
>>>>>>> +#ifdef HAVE_VIRGL_CONTEXT_INIT
>>>>>>> +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
>>>>>>> +                                                 cc.context_init,
>>>>>>> +                                                 cc.nlen,
>>>>>>> +                                                 cc.debug_name);
>>>>>>> +        return;
>>>>>>> +#endif
>>>>>>
>>>>>> This should deal with the case when context_init is set while
>>>>>> HAVE_VIRGL_CONTEXT_INIT is not defined.
>>>>>
>>>>> Actually, I received the comment below before:
>>>>>
>>>>> https://lore.kernel.org/qemu-devel/32588d0e-a1f2-30c4-5e9f-e6e7c4190b65@linaro.org/
>>>>>
>>>>> At original patch set, I have the case while HAVE_VIRGL_CONTEXT_INIT is set
>>>>> but HAVE_VIRGL_CONTEXT_INIT is not defined. But I think we may encounter
>>>>> the case that virgl_renderer_context_create_with_flags is not defined in
>>>>> virglrenderer early version. Should I bring the error message back?
>>>>>
>>>>> Thanks,
>>>>> Ray
>>>>
>>>> I suggest checking VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED instead of
>>>> reporting an error here. Perhaps it may be easier to add #ifdef around:
>>>>    > +    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
>>>>    > +                    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, false),
>>>
>>> How about below changes: >
>>> ---
>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>>> index 8bb7a2c21f..54a3cfe136 100644
>>> --- a/hw/display/virtio-gpu-virgl.c
>>> +++ b/hw/display/virtio-gpu-virgl.c
>>> @@ -106,8 +106,15 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
>>>        trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
>>>                                        cc.debug_name);
>>>
>>> -    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
>>> -                                  cc.debug_name);
>>> +    if (cc.context_init && virtio_gpu_context_init_enabled(g->conf)) {
>>> +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
>>> +                                                 cc.context_init,
>>> +                                                 cc.nlen,
>>> +                                                 cc.debug_name);
>>> +        return;
>>> +    }
>>> +
>>> +    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name);
>>>    }
>>>
>>>    static void virgl_cmd_context_destroy(VirtIOGPU *g,
>>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>>> index be16efbd38..6ff2c8e92d 100644
>>> --- a/hw/display/virtio-gpu.c
>>> +++ b/hw/display/virtio-gpu.c
>>> @@ -1508,6 +1508,10 @@ static Property virtio_gpu_properties[] = {
>>>        DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
>>>                        VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
>>>        DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
>>> +#ifdef HAVE_VIRGL_CONTEXT_INIT
>>> +    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
>>> +                    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, true),
>>> +#endif
>>>        DEFINE_PROP_END_OF_LIST(),
>>>    };
>>>
>>
>> It looks better, but not having #ifdef around
>> virgl_renderer_context_create_with_flags() will result in a link error
>> with old virglrenderer.
> 
> Hmm, right, it seems that we have to have a "#ifdef" around here. Or do you
> have any better idea?

Having #ifdef is the right direction.

Re: [QEMU PATCH v5 06/13] virtio-gpu: Support context init feature with virglrenderer

[Huang Rui]

On Mon, Sep 18, 2023 at 02:07:25PM +0800, Akihiko Odaki wrote:
> On 2023/09/18 14:43, Huang Rui wrote:
> > On Sun, Sep 17, 2023 at 01:49:19PM +0800, Akihiko Odaki wrote:
> >> On 2023/09/17 14:45, Huang Rui wrote:
> >>> On Sat, Sep 16, 2023 at 06:42:04PM +0800, Akihiko Odaki wrote:
> >>>> On 2023/09/16 19:32, Huang Rui wrote:
> >>>>> On Fri, Sep 15, 2023 at 11:20:46PM +0800, Akihiko Odaki wrote:
> >>>>>> On 2023/09/15 20:11, Huang Rui wrote:
> >>>>>>> Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init
> >>>>>>> feature flags.
> >>>>>>> We would like to enable the feature with virglrenderer, so add to create
> >>>>>>> virgl renderer context with flags using context_id when valid.
> >>>>>>>
> >>>>>>> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> >>>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
> >>>>>>> ---
> >>>>>>>
> >>>>>>> V4 -> V5:
> >>>>>>>         - Inverted patch 5 and 6 because we should configure
> >>>>>>>           HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
> >>>>>>>
> >>>>>>>      hw/display/virtio-gpu-virgl.c | 13 +++++++++++--
> >>>>>>>      hw/display/virtio-gpu.c       |  2 ++
> >>>>>>>      2 files changed, 13 insertions(+), 2 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> >>>>>>> index 8bb7a2c21f..312953ec16 100644
> >>>>>>> --- a/hw/display/virtio-gpu-virgl.c
> >>>>>>> +++ b/hw/display/virtio-gpu-virgl.c
> >>>>>>> @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
> >>>>>>>          trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
> >>>>>>>                                          cc.debug_name);
> >>>>>>>      
> >>>>>>> -    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
> >>>>>>> -                                  cc.debug_name);
> >>>>>>> +    if (cc.context_init) {
> >>>>>>> +#ifdef HAVE_VIRGL_CONTEXT_INIT
> >>>>>>> +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
> >>>>>>> +                                                 cc.context_init,
> >>>>>>> +                                                 cc.nlen,
> >>>>>>> +                                                 cc.debug_name);
> >>>>>>> +        return;
> >>>>>>> +#endif
> >>>>>>
> >>>>>> This should deal with the case when context_init is set while
> >>>>>> HAVE_VIRGL_CONTEXT_INIT is not defined.
> >>>>>
> >>>>> Actually, I received the comment below before:
> >>>>>
> >>>>> https://lore.kernel.org/qemu-devel/32588d0e-a1f2-30c4-5e9f-e6e7c4190b65@linaro.org/
> >>>>>
> >>>>> At original patch set, I have the case while HAVE_VIRGL_CONTEXT_INIT is set
> >>>>> but HAVE_VIRGL_CONTEXT_INIT is not defined. But I think we may encounter
> >>>>> the case that virgl_renderer_context_create_with_flags is not defined in
> >>>>> virglrenderer early version. Should I bring the error message back?
> >>>>>
> >>>>> Thanks,
> >>>>> Ray
> >>>>
> >>>> I suggest checking VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED instead of
> >>>> reporting an error here. Perhaps it may be easier to add #ifdef around:
> >>>>    > +    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
> >>>>    > +                    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, false),
> >>>
> >>> How about below changes: >
> >>> ---
> >>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> >>> index 8bb7a2c21f..54a3cfe136 100644
> >>> --- a/hw/display/virtio-gpu-virgl.c
> >>> +++ b/hw/display/virtio-gpu-virgl.c
> >>> @@ -106,8 +106,15 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
> >>>        trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
> >>>                                        cc.debug_name);
> >>>
> >>> -    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
> >>> -                                  cc.debug_name);
> >>> +    if (cc.context_init && virtio_gpu_context_init_enabled(g->conf)) {
> >>> +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
> >>> +                                                 cc.context_init,
> >>> +                                                 cc.nlen,
> >>> +                                                 cc.debug_name);
> >>> +        return;
> >>> +    }
> >>> +
> >>> +    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name);
> >>>    }
> >>>
> >>>    static void virgl_cmd_context_destroy(VirtIOGPU *g,
> >>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> >>> index be16efbd38..6ff2c8e92d 100644
> >>> --- a/hw/display/virtio-gpu.c
> >>> +++ b/hw/display/virtio-gpu.c
> >>> @@ -1508,6 +1508,10 @@ static Property virtio_gpu_properties[] = {
> >>>        DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
> >>>                        VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
> >>>        DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
> >>> +#ifdef HAVE_VIRGL_CONTEXT_INIT
> >>> +    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
> >>> +                    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, true),
> >>> +#endif
> >>>        DEFINE_PROP_END_OF_LIST(),
> >>>    };
> >>>
> >>
> >> It looks better, but not having #ifdef around
> >> virgl_renderer_context_create_with_flags() will result in a link error
> >> with old virglrenderer.
> > 
> > Hmm, right, it seems that we have to have a "#ifdef" around here. Or do you
> > have any better idea?
> 
> Having #ifdef is the right direction.

OK, so we can use cc.context_init and make sure context_init function
enabled. Please check below:

---
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 8bb7a2c21f..8363674ebc 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
     trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
                                     cc.debug_name);

-    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
-                                  cc.debug_name);
+    if (cc.context_init && virtio_gpu_context_init_enabled(g->parent_obj.conf)) {
+#ifdef HAVE_VIRGL_CONTEXT_INIT
+        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
+                                                 cc.context_init,
+                                                 cc.nlen,
+                                                 cc.debug_name);
+        return;
+#endif
+    }
+
+    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name);
 }

 static void virgl_cmd_context_destroy(VirtIOGPU *g,
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index be16efbd38..6ff2c8e92d 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1508,6 +1508,10 @@ static Property virtio_gpu_properties[] = {
     DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
                     VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
     DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
+#ifdef HAVE_VIRGL_CONTEXT_INIT
+    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
+                    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, true),
+#endif
     DEFINE_PROP_END_OF_LIST(),
 };

Re: [QEMU PATCH v5 06/13] virtio-gpu: Support context init feature with virglrenderer

[Akihiko Odaki]

On 2023/09/18 15:20, Huang Rui wrote:
> On Mon, Sep 18, 2023 at 02:07:25PM +0800, Akihiko Odaki wrote:
>> On 2023/09/18 14:43, Huang Rui wrote:
>>> On Sun, Sep 17, 2023 at 01:49:19PM +0800, Akihiko Odaki wrote:
>>>> On 2023/09/17 14:45, Huang Rui wrote:
>>>>> On Sat, Sep 16, 2023 at 06:42:04PM +0800, Akihiko Odaki wrote:
>>>>>> On 2023/09/16 19:32, Huang Rui wrote:
>>>>>>> On Fri, Sep 15, 2023 at 11:20:46PM +0800, Akihiko Odaki wrote:
>>>>>>>> On 2023/09/15 20:11, Huang Rui wrote:
>>>>>>>>> Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init
>>>>>>>>> feature flags.
>>>>>>>>> We would like to enable the feature with virglrenderer, so add to create
>>>>>>>>> virgl renderer context with flags using context_id when valid.
>>>>>>>>>
>>>>>>>>> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
>>>>>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> V4 -> V5:
>>>>>>>>>          - Inverted patch 5 and 6 because we should configure
>>>>>>>>>            HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
>>>>>>>>>
>>>>>>>>>       hw/display/virtio-gpu-virgl.c | 13 +++++++++++--
>>>>>>>>>       hw/display/virtio-gpu.c       |  2 ++
>>>>>>>>>       2 files changed, 13 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>>>>>>>>> index 8bb7a2c21f..312953ec16 100644
>>>>>>>>> --- a/hw/display/virtio-gpu-virgl.c
>>>>>>>>> +++ b/hw/display/virtio-gpu-virgl.c
>>>>>>>>> @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
>>>>>>>>>           trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
>>>>>>>>>                                           cc.debug_name);
>>>>>>>>>       
>>>>>>>>> -    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
>>>>>>>>> -                                  cc.debug_name);
>>>>>>>>> +    if (cc.context_init) {
>>>>>>>>> +#ifdef HAVE_VIRGL_CONTEXT_INIT
>>>>>>>>> +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
>>>>>>>>> +                                                 cc.context_init,
>>>>>>>>> +                                                 cc.nlen,
>>>>>>>>> +                                                 cc.debug_name);
>>>>>>>>> +        return;
>>>>>>>>> +#endif
>>>>>>>>
>>>>>>>> This should deal with the case when context_init is set while
>>>>>>>> HAVE_VIRGL_CONTEXT_INIT is not defined.
>>>>>>>
>>>>>>> Actually, I received the comment below before:
>>>>>>>
>>>>>>> https://lore.kernel.org/qemu-devel/32588d0e-a1f2-30c4-5e9f-e6e7c4190b65@linaro.org/
>>>>>>>
>>>>>>> At original patch set, I have the case while HAVE_VIRGL_CONTEXT_INIT is set
>>>>>>> but HAVE_VIRGL_CONTEXT_INIT is not defined. But I think we may encounter
>>>>>>> the case that virgl_renderer_context_create_with_flags is not defined in
>>>>>>> virglrenderer early version. Should I bring the error message back?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Ray
>>>>>>
>>>>>> I suggest checking VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED instead of
>>>>>> reporting an error here. Perhaps it may be easier to add #ifdef around:
>>>>>>     > +    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
>>>>>>     > +                    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, false),
>>>>>
>>>>> How about below changes: >
>>>>> ---
>>>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>>>>> index 8bb7a2c21f..54a3cfe136 100644
>>>>> --- a/hw/display/virtio-gpu-virgl.c
>>>>> +++ b/hw/display/virtio-gpu-virgl.c
>>>>> @@ -106,8 +106,15 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
>>>>>         trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
>>>>>                                         cc.debug_name);
>>>>>
>>>>> -    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
>>>>> -                                  cc.debug_name);
>>>>> +    if (cc.context_init && virtio_gpu_context_init_enabled(g->conf)) {
>>>>> +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
>>>>> +                                                 cc.context_init,
>>>>> +                                                 cc.nlen,
>>>>> +                                                 cc.debug_name);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name);
>>>>>     }
>>>>>
>>>>>     static void virgl_cmd_context_destroy(VirtIOGPU *g,
>>>>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>>>>> index be16efbd38..6ff2c8e92d 100644
>>>>> --- a/hw/display/virtio-gpu.c
>>>>> +++ b/hw/display/virtio-gpu.c
>>>>> @@ -1508,6 +1508,10 @@ static Property virtio_gpu_properties[] = {
>>>>>         DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
>>>>>                         VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
>>>>>         DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
>>>>> +#ifdef HAVE_VIRGL_CONTEXT_INIT
>>>>> +    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
>>>>> +                    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, true),
>>>>> +#endif
>>>>>         DEFINE_PROP_END_OF_LIST(),
>>>>>     };
>>>>>
>>>>
>>>> It looks better, but not having #ifdef around
>>>> virgl_renderer_context_create_with_flags() will result in a link error
>>>> with old virglrenderer.
>>>
>>> Hmm, right, it seems that we have to have a "#ifdef" around here. Or do you
>>> have any better idea?
>>
>> Having #ifdef is the right direction.
> 
> OK, so we can use cc.context_init and make sure context_init function
> enabled. Please check below:
> 
> ---
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 8bb7a2c21f..8363674ebc 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
>       trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
>                                       cc.debug_name);
> 
> -    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
> -                                  cc.debug_name);
> +    if (cc.context_init && virtio_gpu_context_init_enabled(g->parent_obj.conf)) {
> +#ifdef HAVE_VIRGL_CONTEXT_INIT
> +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
> +                                                 cc.context_init,
> +                                                 cc.nlen,
> +                                                 cc.debug_name);
> +        return;
> +#endif
> +    }

You can put the if-statement into #ifdef. Otherwise it looks good to me.

Re: [QEMU PATCH v5 06/13] virtio-gpu: Support context init feature with virglrenderer

[Akihiko Odaki]

On 2023/09/15 20:11, Huang Rui wrote:
> Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init
> feature flags.
> We would like to enable the feature with virglrenderer, so add to create
> virgl renderer context with flags using context_id when valid.
> 
> Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
> 
> V4 -> V5:
>      - Inverted patch 5 and 6 because we should configure
>        HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
> 
>   hw/display/virtio-gpu-virgl.c | 13 +++++++++++--
>   hw/display/virtio-gpu.c       |  2 ++
>   2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 8bb7a2c21f..312953ec16 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
>       trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
>                                       cc.debug_name);
>   
> -    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
> -                                  cc.debug_name);
> +    if (cc.context_init) {
> +#ifdef HAVE_VIRGL_CONTEXT_INIT
> +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
> +                                                 cc.context_init,
> +                                                 cc.nlen,
> +                                                 cc.debug_name);
> +        return;
> +#endif
> +    }
> +
> +    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name);
>   }
>   
>   static void virgl_cmd_context_destroy(VirtIOGPU *g,
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 3e658f1fef..a66cbd9930 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1506,6 +1506,8 @@ static Property virtio_gpu_properties[] = {
>       DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
>                       VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
>       DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
> +    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
> +                    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, false),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   

I think it's more convenient if this feature is enabled by default.

Re: [QEMU PATCH v5 06/13] virtio-gpu: Support context init feature with virglrenderer

[Huang Rui]

On Sat, Sep 16, 2023 at 12:58:31AM +0800, Akihiko Odaki wrote:
> On 2023/09/15 20:11, Huang Rui wrote:
> > Patch "virtio-gpu: CONTEXT_INIT feature" has added the context_init
> > feature flags.
> > We would like to enable the feature with virglrenderer, so add to create
> > virgl renderer context with flags using context_id when valid.
> > 
> > Originally-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> > 
> > V4 -> V5:
> >      - Inverted patch 5 and 6 because we should configure
> >        HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
> > 
> >   hw/display/virtio-gpu-virgl.c | 13 +++++++++++--
> >   hw/display/virtio-gpu.c       |  2 ++
> >   2 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> > index 8bb7a2c21f..312953ec16 100644
> > --- a/hw/display/virtio-gpu-virgl.c
> > +++ b/hw/display/virtio-gpu-virgl.c
> > @@ -106,8 +106,17 @@ static void virgl_cmd_context_create(VirtIOGPU *g,
> >       trace_virtio_gpu_cmd_ctx_create(cc.hdr.ctx_id,
> >                                       cc.debug_name);
> >   
> > -    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen,
> > -                                  cc.debug_name);
> > +    if (cc.context_init) {
> > +#ifdef HAVE_VIRGL_CONTEXT_INIT
> > +        virgl_renderer_context_create_with_flags(cc.hdr.ctx_id,
> > +                                                 cc.context_init,
> > +                                                 cc.nlen,
> > +                                                 cc.debug_name);
> > +        return;
> > +#endif
> > +    }
> > +
> > +    virgl_renderer_context_create(cc.hdr.ctx_id, cc.nlen, cc.debug_name);
> >   }
> >   
> >   static void virgl_cmd_context_destroy(VirtIOGPU *g,
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index 3e658f1fef..a66cbd9930 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -1506,6 +1506,8 @@ static Property virtio_gpu_properties[] = {
> >       DEFINE_PROP_BIT("blob", VirtIOGPU, parent_obj.conf.flags,
> >                       VIRTIO_GPU_FLAG_BLOB_ENABLED, false),
> >       DEFINE_PROP_SIZE("hostmem", VirtIOGPU, parent_obj.conf.hostmem, 0),
> > +    DEFINE_PROP_BIT("context_init", VirtIOGPU, parent_obj.conf.flags,
> > +                    VIRTIO_GPU_FLAG_CONTEXT_INIT_ENABLED, false),
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> >   
> 
> I think it's more convenient if this feature is enabled by default.

Yes, I will update it in next version.

Thanks,
Ray

[QEMU PATCH v5 07/13] softmmu/memory: enable automatic deallocation of memory regions

[Huang Rui]

From: Xenia Ragiadakou <xenia.ragiadakou@amd.com>

When the memory region has a different life-cycle from that of her parent,
could be automatically released, once has been unparent and once all of her
references have gone away, via the object's free callback.

However, currently, references to the memory region are held by its owner
without first incrementing the memory region object's reference count.
As a result, the automatic deallocation of the object, not taking into
account those references, results in use-after-free memory corruption.

This patch increases the reference count of an owned memory region object
on each memory_region_ref() and decreases it on each memory_region_unref().

Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

V4 -> V5:
    - ref/unref only owned memory regions (Akihiko)

 softmmu/memory.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 7d9494ce70..15e1699750 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1800,6 +1800,9 @@ void memory_region_ref(MemoryRegion *mr)
     /* MMIO callbacks most likely will access data that belongs
      * to the owner, hence the need to ref/unref the owner whenever
      * the memory region is in use.
+     * Likewise, the owner keeps references to the memory region,
+     * hence the need to ref/unref the memory region object to prevent
+     * its automatic deallocation while still referenced by its owner.
      *
      * The memory region is a child of its owner.  As long as the
      * owner doesn't call unparent itself on the memory region,
@@ -1808,6 +1811,7 @@ void memory_region_ref(MemoryRegion *mr)
      * we do not ref/unref them because it slows down DMA sensibly.
      */
     if (mr && mr->owner) {
+        object_ref(OBJECT(mr));
         object_ref(mr->owner);
     }
 }
@@ -1816,6 +1820,7 @@ void memory_region_unref(MemoryRegion *mr)
 {
     if (mr && mr->owner) {
         object_unref(mr->owner);
+        object_unref(OBJECT(mr));
     }
 }
 
-- 
2.34.1

Re: [QEMU PATCH v5 07/13] softmmu/memory: enable automatic deallocation of memory regions

[Akihiko Odaki]

On 2023/09/15 20:11, Huang Rui wrote:
> From: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> 
> When the memory region has a different life-cycle from that of her parent,
> could be automatically released, once has been unparent and once all of her
> references have gone away, via the object's free callback.
> 
> However, currently, references to the memory region are held by its owner
> without first incrementing the memory region object's reference count.
> As a result, the automatic deallocation of the object, not taking into
> account those references, results in use-after-free memory corruption.
> 
> This patch increases the reference count of an owned memory region object
> on each memory_region_ref() and decreases it on each memory_region_unref().
> 
> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
> 
> V4 -> V5:
>      - ref/unref only owned memory regions (Akihiko)
> 
>   softmmu/memory.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 7d9494ce70..15e1699750 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1800,6 +1800,9 @@ void memory_region_ref(MemoryRegion *mr)
>       /* MMIO callbacks most likely will access data that belongs
>        * to the owner, hence the need to ref/unref the owner whenever
>        * the memory region is in use.
> +     * Likewise, the owner keeps references to the memory region,
> +     * hence the need to ref/unref the memory region object to prevent
> +     * its automatic deallocation while still referenced by its owner.

This comment does not make sense. Traditionally no such automatic 
deallocation happens so the owner has been always required to free the 
memory region when it gets finalized.

"[QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands" 
introduces a different kind of memory region, which can be freed anytime 
before the device gets finalized. Even in this case, the owner removes 
the reference to the memory owner by doing res->region = NULL;

Regards,
Akihiko Odaki

Re: [QEMU PATCH v5 07/13] softmmu/memory: enable automatic deallocation of memory regions

[Xenia Ragiadakou]

On 15/9/23 18:11, Akihiko Odaki wrote:
> On 2023/09/15 20:11, Huang Rui wrote:
>> From: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>>
>> When the memory region has a different life-cycle from that of her 
>> parent,
>> could be automatically released, once has been unparent and once all 
>> of her
>> references have gone away, via the object's free callback.
>>
>> However, currently, references to the memory region are held by its 
>> owner
>> without first incrementing the memory region object's reference count.
>> As a result, the automatic deallocation of the object, not taking into
>> account those references, results in use-after-free memory corruption.
>>
>> This patch increases the reference count of an owned memory region 
>> object
>> on each memory_region_ref() and decreases it on each 
>> memory_region_unref().
>>
>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>> ---
>>
>> V4 -> V5:
>>      - ref/unref only owned memory regions (Akihiko)
>>
>>   softmmu/memory.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/softmmu/memory.c b/softmmu/memory.c
>> index 7d9494ce70..15e1699750 100644
>> --- a/softmmu/memory.c
>> +++ b/softmmu/memory.c
>> @@ -1800,6 +1800,9 @@ void memory_region_ref(MemoryRegion *mr)
>>       /* MMIO callbacks most likely will access data that belongs
>>        * to the owner, hence the need to ref/unref the owner whenever
>>        * the memory region is in use.
>> +     * Likewise, the owner keeps references to the memory region,
>> +     * hence the need to ref/unref the memory region object to prevent
>> +     * its automatic deallocation while still referenced by its owner.
>
> This comment does not make sense. Traditionally no such automatic 
> deallocation happens so the owner has been always required to free the 
> memory region when it gets finalized.
>
> "[QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands" 
> introduces a different kind of memory region, which can be freed 
> anytime before the device gets finalized. Even in this case, the owner 
> removes the reference to the memory owner by doing res->region = NULL;

Hi Akihiko,

You are right, the word "owner" is not correct. The issue observed was 
due to the references kept in flatview ranges and the fact that 
flatview_destroy() is asynchronous and was called after memory region's 
destruction.

If I replace the word "owner" with "memory subsystem" in the commit 
message and drop the comment, would that be ok with you? or do want to 
suggest something else?

Xenia

>
>
> Regards,
> Akihiko Odaki

Re: [QEMU PATCH v5 07/13] softmmu/memory: enable automatic deallocation of memory regions

[Akihiko Odaki]

On 2023/09/19 19:28, Xenia Ragiadakou wrote:
> 
> On 15/9/23 18:11, Akihiko Odaki wrote:
>> On 2023/09/15 20:11, Huang Rui wrote:
>>> From: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>>>
>>> When the memory region has a different life-cycle from that of her 
>>> parent,
>>> could be automatically released, once has been unparent and once all 
>>> of her
>>> references have gone away, via the object's free callback.
>>>
>>> However, currently, references to the memory region are held by its 
>>> owner
>>> without first incrementing the memory region object's reference count.
>>> As a result, the automatic deallocation of the object, not taking into
>>> account those references, results in use-after-free memory corruption.
>>>
>>> This patch increases the reference count of an owned memory region 
>>> object
>>> on each memory_region_ref() and decreases it on each 
>>> memory_region_unref().
>>>
>>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>> ---
>>>
>>> V4 -> V5:
>>>      - ref/unref only owned memory regions (Akihiko)
>>>
>>>   softmmu/memory.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/softmmu/memory.c b/softmmu/memory.c
>>> index 7d9494ce70..15e1699750 100644
>>> --- a/softmmu/memory.c
>>> +++ b/softmmu/memory.c
>>> @@ -1800,6 +1800,9 @@ void memory_region_ref(MemoryRegion *mr)
>>>       /* MMIO callbacks most likely will access data that belongs
>>>        * to the owner, hence the need to ref/unref the owner whenever
>>>        * the memory region is in use.
>>> +     * Likewise, the owner keeps references to the memory region,
>>> +     * hence the need to ref/unref the memory region object to prevent
>>> +     * its automatic deallocation while still referenced by its owner.
>>
>> This comment does not make sense. Traditionally no such automatic 
>> deallocation happens so the owner has been always required to free the 
>> memory region when it gets finalized.
>>
>> "[QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands" 
>> introduces a different kind of memory region, which can be freed 
>> anytime before the device gets finalized. Even in this case, the owner 
>> removes the reference to the memory owner by doing res->region = NULL;
> 
> Hi Akihiko,
> 
> You are right, the word "owner" is not correct. The issue observed was 
> due to the references kept in flatview ranges and the fact that 
> flatview_destroy() is asynchronous and was called after memory region's 
> destruction.
> 
> If I replace the word "owner" with "memory subsystem" in the commit 
> message and drop the comment, would that be ok with you? or do want to 
> suggest something else?

This will extend the lifetime of the memory region, but the underlying 
memory is still synchronously freed. Can you show that the flatview 
range will not be used to read the freed memory?

Regards,
Akihiko Odaki

Re: [QEMU PATCH v5 07/13] softmmu/memory: enable automatic deallocation of memory regions

[Xenia Ragiadakou]

On 19/9/23 13:44, Akihiko Odaki wrote:
> On 2023/09/19 19:28, Xenia Ragiadakou wrote:
>>
>> On 15/9/23 18:11, Akihiko Odaki wrote:
>>> On 2023/09/15 20:11, Huang Rui wrote:
>>>> From: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>>>>
>>>> When the memory region has a different life-cycle from that of her 
>>>> parent,
>>>> could be automatically released, once has been unparent and once 
>>>> all of her
>>>> references have gone away, via the object's free callback.
>>>>
>>>> However, currently, references to the memory region are held by its 
>>>> owner
>>>> without first incrementing the memory region object's reference count.
>>>> As a result, the automatic deallocation of the object, not taking into
>>>> account those references, results in use-after-free memory corruption.
>>>>
>>>> This patch increases the reference count of an owned memory region 
>>>> object
>>>> on each memory_region_ref() and decreases it on each 
>>>> memory_region_unref().
>>>>
>>>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>>> ---
>>>>
>>>> V4 -> V5:
>>>>      - ref/unref only owned memory regions (Akihiko)
>>>>
>>>>   softmmu/memory.c | 5 +++++
>>>>   1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/softmmu/memory.c b/softmmu/memory.c
>>>> index 7d9494ce70..15e1699750 100644
>>>> --- a/softmmu/memory.c
>>>> +++ b/softmmu/memory.c
>>>> @@ -1800,6 +1800,9 @@ void memory_region_ref(MemoryRegion *mr)
>>>>       /* MMIO callbacks most likely will access data that belongs
>>>>        * to the owner, hence the need to ref/unref the owner whenever
>>>>        * the memory region is in use.
>>>> +     * Likewise, the owner keeps references to the memory region,
>>>> +     * hence the need to ref/unref the memory region object to 
>>>> prevent
>>>> +     * its automatic deallocation while still referenced by its 
>>>> owner.
>>>
>>> This comment does not make sense. Traditionally no such automatic 
>>> deallocation happens so the owner has been always required to free 
>>> the memory region when it gets finalized.
>>>
>>> "[QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands" 
>>> introduces a different kind of memory region, which can be freed 
>>> anytime before the device gets finalized. Even in this case, the 
>>> owner removes the reference to the memory owner by doing res->region 
>>> = NULL;
>>
>> Hi Akihiko,
>>
>> You are right, the word "owner" is not correct. The issue observed 
>> was due to the references kept in flatview ranges and the fact that 
>> flatview_destroy() is asynchronous and was called after memory 
>> region's destruction.
>>
>> If I replace the word "owner" with "memory subsystem" in the commit 
>> message and drop the comment, would that be ok with you? or do want 
>> to suggest something else?
>
> This will extend the lifetime of the memory region, but the underlying 
> memory is still synchronously freed. Can you show that the flatview 
> range will not be used to read the freed memory?

Yes, the intention of this patch is to delay the mr object finalization 
until all memory_region_unref() on this mr have been taken place.

What do you mean by "the underlying memory is still synchronously freed"?

Re: [QEMU PATCH v5 07/13] softmmu/memory: enable automatic deallocation of memory regions

[Akihiko Odaki]

On 2023/09/19 23:21, Xenia Ragiadakou wrote:
> 
> On 19/9/23 13:44, Akihiko Odaki wrote:
>> On 2023/09/19 19:28, Xenia Ragiadakou wrote:
>>>
>>> On 15/9/23 18:11, Akihiko Odaki wrote:
>>>> On 2023/09/15 20:11, Huang Rui wrote:
>>>>> From: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>>>>>
>>>>> When the memory region has a different life-cycle from that of her 
>>>>> parent,
>>>>> could be automatically released, once has been unparent and once 
>>>>> all of her
>>>>> references have gone away, via the object's free callback.
>>>>>
>>>>> However, currently, references to the memory region are held by its 
>>>>> owner
>>>>> without first incrementing the memory region object's reference count.
>>>>> As a result, the automatic deallocation of the object, not taking into
>>>>> account those references, results in use-after-free memory corruption.
>>>>>
>>>>> This patch increases the reference count of an owned memory region 
>>>>> object
>>>>> on each memory_region_ref() and decreases it on each 
>>>>> memory_region_unref().
>>>>>
>>>>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>>>> ---
>>>>>
>>>>> V4 -> V5:
>>>>>      - ref/unref only owned memory regions (Akihiko)
>>>>>
>>>>>   softmmu/memory.c | 5 +++++
>>>>>   1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/softmmu/memory.c b/softmmu/memory.c
>>>>> index 7d9494ce70..15e1699750 100644
>>>>> --- a/softmmu/memory.c
>>>>> +++ b/softmmu/memory.c
>>>>> @@ -1800,6 +1800,9 @@ void memory_region_ref(MemoryRegion *mr)
>>>>>       /* MMIO callbacks most likely will access data that belongs
>>>>>        * to the owner, hence the need to ref/unref the owner whenever
>>>>>        * the memory region is in use.
>>>>> +     * Likewise, the owner keeps references to the memory region,
>>>>> +     * hence the need to ref/unref the memory region object to 
>>>>> prevent
>>>>> +     * its automatic deallocation while still referenced by its 
>>>>> owner.
>>>>
>>>> This comment does not make sense. Traditionally no such automatic 
>>>> deallocation happens so the owner has been always required to free 
>>>> the memory region when it gets finalized.
>>>>
>>>> "[QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands" 
>>>> introduces a different kind of memory region, which can be freed 
>>>> anytime before the device gets finalized. Even in this case, the 
>>>> owner removes the reference to the memory owner by doing res->region 
>>>> = NULL;
>>>
>>> Hi Akihiko,
>>>
>>> You are right, the word "owner" is not correct. The issue observed 
>>> was due to the references kept in flatview ranges and the fact that 
>>> flatview_destroy() is asynchronous and was called after memory 
>>> region's destruction.
>>>
>>> If I replace the word "owner" with "memory subsystem" in the commit 
>>> message and drop the comment, would that be ok with you? or do want 
>>> to suggest something else?
>>
>> This will extend the lifetime of the memory region, but the underlying 
>> memory is still synchronously freed. Can you show that the flatview 
>> range will not be used to read the freed memory?
> 
> Yes, the intention of this patch is to delay the mr object finalization 
> until all memory_region_unref() on this mr have been taken place.
> 
> What do you mean by "the underlying memory is still synchronously freed"?
> 

A pointer is passed to memory_region_init_ram_ptr() with the ptr 
parameter when initializing the memory region and the memory region 
keeps the pointer.

In virtio_gpu_virgl_resource_unmap(), the memory pointed with the 
pointer is unmapped with virgl_renderer_resource_unmap() and makes the 
pointer kept by the memory region dangling though the lifetime of the 
memory region is extended with this patch. Can you show that the 
dangling pointer the memory region has will never be referenced?

Re: [QEMU PATCH v5 07/13] softmmu/memory: enable automatic deallocation of memory regions

[Xenia Ragiadakou]

On 20/9/23 01:18, Akihiko Odaki wrote:
> On 2023/09/19 23:21, Xenia Ragiadakou wrote:
>>
>> On 19/9/23 13:44, Akihiko Odaki wrote:
>>> On 2023/09/19 19:28, Xenia Ragiadakou wrote:
>>>>
>>>> On 15/9/23 18:11, Akihiko Odaki wrote:
>>>>> On 2023/09/15 20:11, Huang Rui wrote:
>>>>>> From: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>>>>>>
>>>>>> When the memory region has a different life-cycle from that of 
>>>>>> her parent,
>>>>>> could be automatically released, once has been unparent and once 
>>>>>> all of her
>>>>>> references have gone away, via the object's free callback.
>>>>>>
>>>>>> However, currently, references to the memory region are held by 
>>>>>> its owner
>>>>>> without first incrementing the memory region object's reference 
>>>>>> count.
>>>>>> As a result, the automatic deallocation of the object, not taking 
>>>>>> into
>>>>>> account those references, results in use-after-free memory 
>>>>>> corruption.
>>>>>>
>>>>>> This patch increases the reference count of an owned memory 
>>>>>> region object
>>>>>> on each memory_region_ref() and decreases it on each 
>>>>>> memory_region_unref().
>>>>>>
>>>>>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>>>>> ---
>>>>>>
>>>>>> V4 -> V5:
>>>>>>      - ref/unref only owned memory regions (Akihiko)
>>>>>>
>>>>>>   softmmu/memory.c | 5 +++++
>>>>>>   1 file changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git a/softmmu/memory.c b/softmmu/memory.c
>>>>>> index 7d9494ce70..15e1699750 100644
>>>>>> --- a/softmmu/memory.c
>>>>>> +++ b/softmmu/memory.c
>>>>>> @@ -1800,6 +1800,9 @@ void memory_region_ref(MemoryRegion *mr)
>>>>>>       /* MMIO callbacks most likely will access data that belongs
>>>>>>        * to the owner, hence the need to ref/unref the owner 
>>>>>> whenever
>>>>>>        * the memory region is in use.
>>>>>> +     * Likewise, the owner keeps references to the memory region,
>>>>>> +     * hence the need to ref/unref the memory region object to 
>>>>>> prevent
>>>>>> +     * its automatic deallocation while still referenced by its 
>>>>>> owner.
>>>>>
>>>>> This comment does not make sense. Traditionally no such automatic 
>>>>> deallocation happens so the owner has been always required to free 
>>>>> the memory region when it gets finalized.
>>>>>
>>>>> "[QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands" 
>>>>> introduces a different kind of memory region, which can be freed 
>>>>> anytime before the device gets finalized. Even in this case, the 
>>>>> owner removes the reference to the memory owner by doing 
>>>>> res->region = NULL;
>>>>
>>>> Hi Akihiko,
>>>>
>>>> You are right, the word "owner" is not correct. The issue observed 
>>>> was due to the references kept in flatview ranges and the fact that 
>>>> flatview_destroy() is asynchronous and was called after memory 
>>>> region's destruction.
>>>>
>>>> If I replace the word "owner" with "memory subsystem" in the commit 
>>>> message and drop the comment, would that be ok with you? or do want 
>>>> to suggest something else?
>>>
>>> This will extend the lifetime of the memory region, but the 
>>> underlying memory is still synchronously freed. Can you show that 
>>> the flatview range will not be used to read the freed memory?
>>
>> Yes, the intention of this patch is to delay the mr object 
>> finalization until all memory_region_unref() on this mr have been 
>> taken place.
>>
>> What do you mean by "the underlying memory is still synchronously 
>> freed"?
>>
>
> A pointer is passed to memory_region_init_ram_ptr() with the ptr 
> parameter when initializing the memory region and the memory region 
> keeps the pointer.
>
> In virtio_gpu_virgl_resource_unmap(), the memory pointed with the 
> pointer is unmapped with virgl_renderer_resource_unmap() and makes the 
> pointer kept by the memory region dangling though the lifetime of the 
> memory region is extended with this patch. Can you show that the 
> dangling pointer the memory region has will never be referenced?

I see your point but I think it is not directly related to this patch. 
IMHO, it is related to the implementation of 
virtio_gpu_virgl_resource_unmap(). Maybe the unmapping should be done in 
the free callback. However, I would expect the pointer to a disabled 
memory region to not be used, not sure though.

Re: [QEMU PATCH v5 07/13] softmmu/memory: enable automatic deallocation of memory regions

[Akihiko Odaki]

On 2023/09/20 17:57, Xenia Ragiadakou wrote:
> 
> On 20/9/23 01:18, Akihiko Odaki wrote:
>> On 2023/09/19 23:21, Xenia Ragiadakou wrote:
>>>
>>> On 19/9/23 13:44, Akihiko Odaki wrote:
>>>> On 2023/09/19 19:28, Xenia Ragiadakou wrote:
>>>>>
>>>>> On 15/9/23 18:11, Akihiko Odaki wrote:
>>>>>> On 2023/09/15 20:11, Huang Rui wrote:
>>>>>>> From: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>>>>>>>
>>>>>>> When the memory region has a different life-cycle from that of 
>>>>>>> her parent,
>>>>>>> could be automatically released, once has been unparent and once 
>>>>>>> all of her
>>>>>>> references have gone away, via the object's free callback.
>>>>>>>
>>>>>>> However, currently, references to the memory region are held by 
>>>>>>> its owner
>>>>>>> without first incrementing the memory region object's reference 
>>>>>>> count.
>>>>>>> As a result, the automatic deallocation of the object, not taking 
>>>>>>> into
>>>>>>> account those references, results in use-after-free memory 
>>>>>>> corruption.
>>>>>>>
>>>>>>> This patch increases the reference count of an owned memory 
>>>>>>> region object
>>>>>>> on each memory_region_ref() and decreases it on each 
>>>>>>> memory_region_unref().
>>>>>>>
>>>>>>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>>>>>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>>>>>> ---
>>>>>>>
>>>>>>> V4 -> V5:
>>>>>>>      - ref/unref only owned memory regions (Akihiko)
>>>>>>>
>>>>>>>   softmmu/memory.c | 5 +++++
>>>>>>>   1 file changed, 5 insertions(+)
>>>>>>>
>>>>>>> diff --git a/softmmu/memory.c b/softmmu/memory.c
>>>>>>> index 7d9494ce70..15e1699750 100644
>>>>>>> --- a/softmmu/memory.c
>>>>>>> +++ b/softmmu/memory.c
>>>>>>> @@ -1800,6 +1800,9 @@ void memory_region_ref(MemoryRegion *mr)
>>>>>>>       /* MMIO callbacks most likely will access data that belongs
>>>>>>>        * to the owner, hence the need to ref/unref the owner 
>>>>>>> whenever
>>>>>>>        * the memory region is in use.
>>>>>>> +     * Likewise, the owner keeps references to the memory region,
>>>>>>> +     * hence the need to ref/unref the memory region object to 
>>>>>>> prevent
>>>>>>> +     * its automatic deallocation while still referenced by its 
>>>>>>> owner.
>>>>>>
>>>>>> This comment does not make sense. Traditionally no such automatic 
>>>>>> deallocation happens so the owner has been always required to free 
>>>>>> the memory region when it gets finalized.
>>>>>>
>>>>>> "[QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands" 
>>>>>> introduces a different kind of memory region, which can be freed 
>>>>>> anytime before the device gets finalized. Even in this case, the 
>>>>>> owner removes the reference to the memory owner by doing 
>>>>>> res->region = NULL;
>>>>>
>>>>> Hi Akihiko,
>>>>>
>>>>> You are right, the word "owner" is not correct. The issue observed 
>>>>> was due to the references kept in flatview ranges and the fact that 
>>>>> flatview_destroy() is asynchronous and was called after memory 
>>>>> region's destruction.
>>>>>
>>>>> If I replace the word "owner" with "memory subsystem" in the commit 
>>>>> message and drop the comment, would that be ok with you? or do want 
>>>>> to suggest something else?
>>>>
>>>> This will extend the lifetime of the memory region, but the 
>>>> underlying memory is still synchronously freed. Can you show that 
>>>> the flatview range will not be used to read the freed memory?
>>>
>>> Yes, the intention of this patch is to delay the mr object 
>>> finalization until all memory_region_unref() on this mr have been 
>>> taken place.
>>>
>>> What do you mean by "the underlying memory is still synchronously 
>>> freed"?
>>>
>>
>> A pointer is passed to memory_region_init_ram_ptr() with the ptr 
>> parameter when initializing the memory region and the memory region 
>> keeps the pointer.
>>
>> In virtio_gpu_virgl_resource_unmap(), the memory pointed with the 
>> pointer is unmapped with virgl_renderer_resource_unmap() and makes the 
>> pointer kept by the memory region dangling though the lifetime of the 
>> memory region is extended with this patch. Can you show that the 
>> dangling pointer the memory region has will never be referenced?
> 
> I see your point but I think it is not directly related to this patch. 
> IMHO, it is related to the implementation of 
> virtio_gpu_virgl_resource_unmap(). Maybe the unmapping should be done in 
> the free callback. However, I would expect the pointer to a disabled 
> memory region to not be used, not sure though.

Unmapping in the free callback sounds good.

[QEMU PATCH v5 08/13] virtio-gpu: Don't require udmabuf when blobs and virgl are enabled

[Huang Rui]

From: Dmitry Osipenko <dmitry.osipenko@collabora.com>

The udmabuf usage is mandatory when virgl is disabled and blobs feature
enabled in the Qemu machine configuration. If virgl and blobs are enabled,
then udmabuf requirement is optional. Since udmabuf isn't widely supported
by a popular Linux distros today, let's relax the udmabuf requirement for
blobs=on,virgl=on. Now, a full-featured virtio-gpu acceleration is
available to Qemu users without a need to have udmabuf available in the
system.

Reviewed-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---
 hw/display/virtio-gpu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index a66cbd9930..5b7a7eab4f 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1361,7 +1361,8 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
     VirtIOGPU *g = VIRTIO_GPU(qdev);
 
     if (virtio_gpu_blob_enabled(g->parent_obj.conf)) {
-        if (!virtio_gpu_have_udmabuf()) {
+        if (!virtio_gpu_virgl_enabled(g->parent_obj.conf) &&
+            !virtio_gpu_have_udmabuf()) {
             error_setg(errp, "cannot enable blob resources without udmabuf");
             return;
         }
-- 
2.34.1

[QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands

[Huang Rui]

From: Antonio Caggiano <antonio.caggiano@collabora.com>

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 <antonio.caggiano@collabora.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

V4 -> V5:
    - Use memory_region_init_ram_ptr() instead of
      memory_region_init_ram_device_ptr() (Akihiko)

 hw/display/virtio-gpu-virgl.c  | 213 +++++++++++++++++++++++++++++++++
 hw/display/virtio-gpu.c        |   4 +-
 include/hw/virtio/virtio-gpu.h |   5 +
 meson.build                    |   4 +
 4 files changed, 225 insertions(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 312953ec16..563a6f2f58 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -17,6 +17,7 @@
 #include "trace.h"
 #include "hw/virtio/virtio.h"
 #include "hw/virtio/virtio-gpu.h"
+#include "hw/virtio/virtio-gpu-bswap.h"
 
 #include "ui/egl-helpers.h"
 
@@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
     virgl_renderer_resource_create(&args, NULL, 0);
 }
 
+static void virgl_resource_destroy(VirtIOGPU *g,
+                                   struct virtio_gpu_simple_resource *res)
+{
+    if (!res)
+        return;
+
+    QTAILQ_REMOVE(&g->reslist, res, next);
+
+    virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt);
+    g_free(res->addrs);
+
+    g_free(res);
+}
+
 static void virgl_cmd_resource_unref(VirtIOGPU *g,
                                      struct virtio_gpu_ctrl_command *cmd)
 {
+    struct virtio_gpu_simple_resource *res;
     struct virtio_gpu_resource_unref unref;
     struct iovec *res_iovs = NULL;
     int num_iovs = 0;
@@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
     VIRTIO_GPU_FILL_CMD(unref);
     trace_virtio_gpu_cmd_res_unref(unref.resource_id);
 
+    res = virtio_gpu_find_resource(g, unref.resource_id);
+
     virgl_renderer_resource_detach_iov(unref.resource_id,
                                        &res_iovs,
                                        &num_iovs);
     if (res_iovs != NULL && num_iovs != 0) {
         virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
+        if (res) {
+            res->iov = NULL;
+            res->iov_cnt = 0;
+        }
     }
+
     virgl_renderer_resource_unref(unref.resource_id);
+
+    virgl_resource_destroy(g, res);
 }
 
 static void virgl_cmd_context_create(VirtIOGPU *g,
@@ -426,6 +451,183 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
     g_free(resp);
 }
 
+#ifdef HAVE_VIRGL_RESOURCE_BLOB
+
+static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
+                                           struct virtio_gpu_ctrl_command *cmd)
+{
+    struct virtio_gpu_simple_resource *res;
+    struct virtio_gpu_resource_create_blob cblob;
+    struct virgl_renderer_resource_create_blob_args virgl_args = { 0 };
+    int ret;
+
+    VIRTIO_GPU_FILL_CMD(cblob);
+    virtio_gpu_create_blob_bswap(&cblob);
+    trace_virtio_gpu_cmd_res_create_blob(cblob.resource_id, cblob.size);
+
+    if (cblob.resource_id == 0) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n",
+                      __func__);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+        return;
+    }
+
+    res = virtio_gpu_find_resource(g, cblob.resource_id);
+    if (res) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
+                      __func__, cblob.resource_id);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+        return;
+    }
+
+    res = g_new0(struct virtio_gpu_simple_resource, 1);
+    if (!res) {
+        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
+        return;
+    }
+
+    res->resource_id = cblob.resource_id;
+    res->blob_size = cblob.size;
+
+    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) {
+        ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
+                                            cmd, &res->addrs, &res->iov,
+                                            &res->iov_cnt);
+        if (!ret) {
+            g_free(res);
+            cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
+            return;
+        }
+    }
+
+    QTAILQ_INSERT_HEAD(&g->reslist, res, next);
+
+    virgl_args.res_handle = cblob.resource_id;
+    virgl_args.ctx_id = cblob.hdr.ctx_id;
+    virgl_args.blob_mem = cblob.blob_mem;
+    virgl_args.blob_id = cblob.blob_id;
+    virgl_args.blob_flags = cblob.blob_flags;
+    virgl_args.size = cblob.size;
+    virgl_args.iovecs = res->iov;
+    virgl_args.num_iovs = res->iov_cnt;
+
+    ret = virgl_renderer_resource_create_blob(&virgl_args);
+    if (ret) {
+        virgl_resource_destroy(g, res);
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: virgl blob create error: %s\n",
+                      __func__, strerror(-ret));
+        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
+    }
+}
+
+static void virgl_cmd_resource_map_blob(VirtIOGPU *g,
+                                        struct virtio_gpu_ctrl_command *cmd)
+{
+    struct virtio_gpu_simple_resource *res;
+    struct virtio_gpu_resource_map_blob mblob;
+    int ret;
+    void *data;
+    uint64_t size;
+    struct virtio_gpu_resp_map_info resp;
+    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
+
+    VIRTIO_GPU_FILL_CMD(mblob);
+    virtio_gpu_map_blob_bswap(&mblob);
+
+    if (mblob.resource_id == 0) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n",
+                      __func__);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+        return;
+    }
+
+    res = virtio_gpu_find_resource(g, mblob.resource_id);
+    if (!res) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n",
+                      __func__, mblob.resource_id);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+        return;
+    }
+    if (res->region) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already mapped %d\n",
+		      __func__, mblob.resource_id);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+        return;
+    }
+
+    ret = virgl_renderer_resource_map(res->resource_id, &data, &size);
+    if (ret) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n",
+                      __func__, strerror(-ret));
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+        return;
+    }
+
+    res->region = g_new0(MemoryRegion, 1);
+    if (!res->region) {
+        virgl_renderer_resource_unmap(res->resource_id);
+        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
+        return;
+    }
+    memory_region_init_ram_ptr(res->region, OBJECT(g), NULL, size, data);
+    OBJECT(res->region)->free = g_free;
+    memory_region_add_subregion(&b->hostmem, mblob.offset, res->region);
+    memory_region_set_enabled(res->region, true);
+
+    memset(&resp, 0, sizeof(resp));
+    resp.hdr.type = VIRTIO_GPU_RESP_OK_MAP_INFO;
+    virgl_renderer_resource_get_map_info(mblob.resource_id, &resp.map_info);
+    virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
+}
+
+static int virtio_gpu_virgl_resource_unmap(VirtIOGPU *g,
+                                           struct virtio_gpu_simple_resource
+                                           *res)
+{
+    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
+
+    if (!res) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already unmapped %d\n",
+                      __func__, res->resource_id);
+        return VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+    }
+
+    memory_region_set_enabled(res->region, false);
+    memory_region_del_subregion(&b->hostmem, res->region);
+    object_unparent(OBJECT(res->region));
+    res->region = NULL;
+
+    return virgl_renderer_resource_unmap(res->resource_id);
+}
+
+static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g,
+                                          struct virtio_gpu_ctrl_command *cmd)
+{
+    struct virtio_gpu_simple_resource *res;
+    struct virtio_gpu_resource_unmap_blob ublob;
+    VIRTIO_GPU_FILL_CMD(ublob);
+    virtio_gpu_unmap_blob_bswap(&ublob);
+
+    if (ublob.resource_id == 0) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n",
+                      __func__);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+        return;
+    }
+
+    res = virtio_gpu_find_resource(g, ublob.resource_id);
+    if (!res) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n",
+                      __func__, ublob.resource_id);
+        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
+        return;
+    }
+
+    virtio_gpu_virgl_resource_unmap(g, res);
+}
+
+#endif /* HAVE_VIRGL_RESOURCE_BLOB */
+
 void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
                                       struct virtio_gpu_ctrl_command *cmd)
 {
@@ -492,6 +694,17 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
     case VIRTIO_GPU_CMD_GET_EDID:
         virtio_gpu_get_edid(g, cmd);
         break;
+#ifdef HAVE_VIRGL_RESOURCE_BLOB
+    case VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB:
+        virgl_cmd_resource_create_blob(g, cmd);
+        break;
+    case VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB:
+        virgl_cmd_resource_map_blob(g, cmd);
+        break;
+    case VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB:
+        virgl_cmd_resource_unmap_blob(g, cmd);
+        break;
+#endif /* HAVE_VIRGL_RESOURCE_BLOB */
     default:
         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
         break;
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 5b7a7eab4f..cc4c1f81bb 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1367,10 +1367,12 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
             return;
         }
 
+#ifndef HAVE_VIRGL_RESOURCE_BLOB
         if (virtio_gpu_virgl_enabled(g->parent_obj.conf)) {
-            error_setg(errp, "blobs and virgl are not compatible (yet)");
+            error_setg(errp, "Linked virglrenderer does not support blob resources");
             return;
         }
+#endif
     }
 
     if (!virtio_gpu_base_device_realize(qdev,
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 55973e112f..b9adc28071 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -58,6 +58,11 @@ struct virtio_gpu_simple_resource {
     int dmabuf_fd;
     uint8_t *remapped;
 
+#ifdef HAVE_VIRGL_RESOURCE_BLOB
+    /* only blob resource needs this region to be mapped as guest mmio */
+    MemoryRegion *region;
+#endif
+
     QTAILQ_ENTRY(virtio_gpu_simple_resource) next;
 };
 
diff --git a/meson.build b/meson.build
index ff20d3c249..f7b744ab82 100644
--- a/meson.build
+++ b/meson.build
@@ -1072,6 +1072,10 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
                        cc.has_function('virgl_renderer_context_create_with_flags',
                                        prefix: '#include <virglrenderer.h>',
                                        dependencies: virgl))
+  config_host_data.set('HAVE_VIRGL_RESOURCE_BLOB',
+                       cc.has_function('virgl_renderer_resource_create_blob',
+                                       prefix: '#include <virglrenderer.h>',
+                                       dependencies: virgl))
 endif
 blkio = not_found
 if not get_option('blkio').auto() or have_block
-- 
2.34.1

Re: [QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands

[Akihiko Odaki]

On 2023/09/15 20:11, Huang Rui wrote:
> From: Antonio Caggiano <antonio.caggiano@collabora.com>
> 
> 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 <antonio.caggiano@collabora.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
> 
> V4 -> V5:
>      - Use memory_region_init_ram_ptr() instead of
>        memory_region_init_ram_device_ptr() (Akihiko)
> 
>   hw/display/virtio-gpu-virgl.c  | 213 +++++++++++++++++++++++++++++++++
>   hw/display/virtio-gpu.c        |   4 +-
>   include/hw/virtio/virtio-gpu.h |   5 +
>   meson.build                    |   4 +
>   4 files changed, 225 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 312953ec16..563a6f2f58 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -17,6 +17,7 @@
>   #include "trace.h"
>   #include "hw/virtio/virtio.h"
>   #include "hw/virtio/virtio-gpu.h"
> +#include "hw/virtio/virtio-gpu-bswap.h"
>   
>   #include "ui/egl-helpers.h"
>   
> @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
>       virgl_renderer_resource_create(&args, NULL, 0);
>   }
>   
> +static void virgl_resource_destroy(VirtIOGPU *g,
> +                                   struct virtio_gpu_simple_resource *res)
> +{
> +    if (!res)
> +        return;
> +
> +    QTAILQ_REMOVE(&g->reslist, res, next);
> +
> +    virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt);
> +    g_free(res->addrs);
> +
> +    g_free(res);
> +}
> +
>   static void virgl_cmd_resource_unref(VirtIOGPU *g,
>                                        struct virtio_gpu_ctrl_command *cmd)
>   {
> +    struct virtio_gpu_simple_resource *res;
>       struct virtio_gpu_resource_unref unref;
>       struct iovec *res_iovs = NULL;
>       int num_iovs = 0;
> @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
>       VIRTIO_GPU_FILL_CMD(unref);
>       trace_virtio_gpu_cmd_res_unref(unref.resource_id);
>   
> +    res = virtio_gpu_find_resource(g, unref.resource_id);
> +
>       virgl_renderer_resource_detach_iov(unref.resource_id,
>                                          &res_iovs,
>                                          &num_iovs);
>       if (res_iovs != NULL && num_iovs != 0) {
>           virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
> +        if (res) {
> +            res->iov = NULL;
> +            res->iov_cnt = 0;
> +        }
>       }
> +
>       virgl_renderer_resource_unref(unref.resource_id);
> +
> +    virgl_resource_destroy(g, res);
>   }
>   
>   static void virgl_cmd_context_create(VirtIOGPU *g,
> @@ -426,6 +451,183 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
>       g_free(resp);
>   }
>   
> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> +
> +static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
> +                                           struct virtio_gpu_ctrl_command *cmd)
> +{
> +    struct virtio_gpu_simple_resource *res;
> +    struct virtio_gpu_resource_create_blob cblob;
> +    struct virgl_renderer_resource_create_blob_args virgl_args = { 0 };
> +    int ret;
> +
> +    VIRTIO_GPU_FILL_CMD(cblob);
> +    virtio_gpu_create_blob_bswap(&cblob);
> +    trace_virtio_gpu_cmd_res_create_blob(cblob.resource_id, cblob.size);
> +
> +    if (cblob.resource_id == 0) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n",
> +                      __func__);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +
> +    res = virtio_gpu_find_resource(g, cblob.resource_id);
> +    if (res) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
> +                      __func__, cblob.resource_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +
> +    res = g_new0(struct virtio_gpu_simple_resource, 1);
> +    if (!res) {
> +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> +        return;
> +    }
> +
> +    res->resource_id = cblob.resource_id;
> +    res->blob_size = cblob.size;
> +
> +    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) {
> +        ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
> +                                            cmd, &res->addrs, &res->iov,
> +                                            &res->iov_cnt);
> +        if (!ret) {
> +            g_free(res);
> +            cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> +            return;
> +        }
> +    }
> +
> +    QTAILQ_INSERT_HEAD(&g->reslist, res, next);
> +
> +    virgl_args.res_handle = cblob.resource_id;
> +    virgl_args.ctx_id = cblob.hdr.ctx_id;
> +    virgl_args.blob_mem = cblob.blob_mem;
> +    virgl_args.blob_id = cblob.blob_id;
> +    virgl_args.blob_flags = cblob.blob_flags;
> +    virgl_args.size = cblob.size;
> +    virgl_args.iovecs = res->iov;
> +    virgl_args.num_iovs = res->iov_cnt;
> +
> +    ret = virgl_renderer_resource_create_blob(&virgl_args);
> +    if (ret) {
> +        virgl_resource_destroy(g, res);
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: virgl blob create error: %s\n",
> +                      __func__, strerror(-ret));
> +        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> +    }
> +}
> +
> +static void virgl_cmd_resource_map_blob(VirtIOGPU *g,
> +                                        struct virtio_gpu_ctrl_command *cmd)
> +{
> +    struct virtio_gpu_simple_resource *res;
> +    struct virtio_gpu_resource_map_blob mblob;
> +    int ret;
> +    void *data;
> +    uint64_t size;
> +    struct virtio_gpu_resp_map_info resp;
> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> +
> +    VIRTIO_GPU_FILL_CMD(mblob);
> +    virtio_gpu_map_blob_bswap(&mblob);
> +
> +    if (mblob.resource_id == 0) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n",
> +                      __func__);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +
> +    res = virtio_gpu_find_resource(g, mblob.resource_id);
> +    if (!res) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n",
> +                      __func__, mblob.resource_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +    if (res->region) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already mapped %d\n",
> +		      __func__, mblob.resource_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +
> +    ret = virgl_renderer_resource_map(res->resource_id, &data, &size);
> +    if (ret) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n",
> +                      __func__, strerror(-ret));
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +
> +    res->region = g_new0(MemoryRegion, 1);
> +    if (!res->region) {
> +        virgl_renderer_resource_unmap(res->resource_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> +        return;
> +    }
> +    memory_region_init_ram_ptr(res->region, OBJECT(g), NULL, size, data);
> +    OBJECT(res->region)->free = g_free;
> +    memory_region_add_subregion(&b->hostmem, mblob.offset, res->region);
> +    memory_region_set_enabled(res->region, true);
> +
> +    memset(&resp, 0, sizeof(resp));
> +    resp.hdr.type = VIRTIO_GPU_RESP_OK_MAP_INFO;
> +    virgl_renderer_resource_get_map_info(mblob.resource_id, &resp.map_info);
> +    virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
> +}
> +
> +static int virtio_gpu_virgl_resource_unmap(VirtIOGPU *g,
> +                                           struct virtio_gpu_simple_resource
> +                                           *res)
> +{
> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> +
> +    if (!res) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already unmapped %d\n",
> +                      __func__, res->resource_id);
> +        return VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;

This returns enum virtio_gpu_ctrl_type while 
virgl_renderer_resource_unmap() returns -errno and the returned error 
code is ignored.

I think you can just inline this function into 
virgl_cmd_resource_unmap_blob(). It will be more concise and consistent 
with virgl_cmd_resource_map_blob().

> +    }
> +
> +    memory_region_set_enabled(res->region, false);
> +    memory_region_del_subregion(&b->hostmem, res->region);
> +    object_unparent(OBJECT(res->region));
> +    res->region = NULL;
> +
> +    return virgl_renderer_resource_unmap(res->resource_id);
> +}
> +
> +static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g,
> +                                          struct virtio_gpu_ctrl_command *cmd)
> +{
> +    struct virtio_gpu_simple_resource *res;
> +    struct virtio_gpu_resource_unmap_blob ublob;
> +    VIRTIO_GPU_FILL_CMD(ublob);
> +    virtio_gpu_unmap_blob_bswap(&ublob);
> +
> +    if (ublob.resource_id == 0) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n",
> +                      __func__);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +
> +    res = virtio_gpu_find_resource(g, ublob.resource_id);
> +    if (!res) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n",
> +                      __func__, ublob.resource_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +
> +    virtio_gpu_virgl_resource_unmap(g, res);
> +}
> +
> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
> +
>   void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>                                         struct virtio_gpu_ctrl_command *cmd)
>   {
> @@ -492,6 +694,17 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>       case VIRTIO_GPU_CMD_GET_EDID:
>           virtio_gpu_get_edid(g, cmd);
>           break;
> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> +    case VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB:
> +        virgl_cmd_resource_create_blob(g, cmd);
> +        break;
> +    case VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB:
> +        virgl_cmd_resource_map_blob(g, cmd);
> +        break;
> +    case VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB:
> +        virgl_cmd_resource_unmap_blob(g, cmd);
> +        break;
> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
>       default:
>           cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>           break;
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 5b7a7eab4f..cc4c1f81bb 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1367,10 +1367,12 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
>               return;
>           }
>   
> +#ifndef HAVE_VIRGL_RESOURCE_BLOB
>           if (virtio_gpu_virgl_enabled(g->parent_obj.conf)) {
> -            error_setg(errp, "blobs and virgl are not compatible (yet)");
> +            error_setg(errp, "Linked virglrenderer does not support blob resources");
>               return;
>           }
> +#endif
>       }
>   
>       if (!virtio_gpu_base_device_realize(qdev,
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index 55973e112f..b9adc28071 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -58,6 +58,11 @@ struct virtio_gpu_simple_resource {
>       int dmabuf_fd;
>       uint8_t *remapped;
>   
> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> +    /* only blob resource needs this region to be mapped as guest mmio */
> +    MemoryRegion *region;
> +#endif
> +

Since this is specific to virgl, let's create a struct that embeds 
virtio_gpu_simple_resource and use it to avoid modifying virtio-gpu 
common code.

Re: [QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands

[Akihiko Odaki]

On 2023/09/16 1:04, Akihiko Odaki wrote:
> On 2023/09/15 20:11, Huang Rui wrote:
>> From: Antonio Caggiano <antonio.caggiano@collabora.com>
>>
>> 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 <antonio.caggiano@collabora.com>
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>> ---
>>
>> V4 -> V5:
>>      - Use memory_region_init_ram_ptr() instead of
>>        memory_region_init_ram_device_ptr() (Akihiko)
>>
>>   hw/display/virtio-gpu-virgl.c  | 213 +++++++++++++++++++++++++++++++++
>>   hw/display/virtio-gpu.c        |   4 +-
>>   include/hw/virtio/virtio-gpu.h |   5 +
>>   meson.build                    |   4 +
>>   4 files changed, 225 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/display/virtio-gpu-virgl.c 
>> b/hw/display/virtio-gpu-virgl.c
>> index 312953ec16..563a6f2f58 100644
>> --- a/hw/display/virtio-gpu-virgl.c
>> +++ b/hw/display/virtio-gpu-virgl.c
>> @@ -17,6 +17,7 @@
>>   #include "trace.h"
>>   #include "hw/virtio/virtio.h"
>>   #include "hw/virtio/virtio-gpu.h"
>> +#include "hw/virtio/virtio-gpu-bswap.h"
>>   #include "ui/egl-helpers.h"
>> @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
>>       virgl_renderer_resource_create(&args, NULL, 0);
>>   }
>> +static void virgl_resource_destroy(VirtIOGPU *g,
>> +                                   struct virtio_gpu_simple_resource 
>> *res)
>> +{
>> +    if (!res)
>> +        return;
>> +
>> +    QTAILQ_REMOVE(&g->reslist, res, next);
>> +
>> +    virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt);
>> +    g_free(res->addrs);
>> +
>> +    g_free(res);
>> +}
>> +
>>   static void virgl_cmd_resource_unref(VirtIOGPU *g,
>>                                        struct virtio_gpu_ctrl_command 
>> *cmd)
>>   {
>> +    struct virtio_gpu_simple_resource *res;
>>       struct virtio_gpu_resource_unref unref;
>>       struct iovec *res_iovs = NULL;
>>       int num_iovs = 0;
>> @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
>>       VIRTIO_GPU_FILL_CMD(unref);
>>       trace_virtio_gpu_cmd_res_unref(unref.resource_id);
>> +    res = virtio_gpu_find_resource(g, unref.resource_id);
>> +
>>       virgl_renderer_resource_detach_iov(unref.resource_id,
>>                                          &res_iovs,
>>                                          &num_iovs);
>>       if (res_iovs != NULL && num_iovs != 0) {
>>           virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
>> +        if (res) {
>> +            res->iov = NULL;
>> +            res->iov_cnt = 0;
>> +        }
>>       }
>> +
>>       virgl_renderer_resource_unref(unref.resource_id);
>> +
>> +    virgl_resource_destroy(g, res);

This may leak memory region.

Re: [QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands

[Huang Rui via]

On Sat, Sep 16, 2023 at 12:37:29AM +0800, Akihiko Odaki wrote:
> On 2023/09/16 1:04, Akihiko Odaki wrote:
> > On 2023/09/15 20:11, Huang Rui wrote:
> >> From: Antonio Caggiano <antonio.caggiano@collabora.com>
> >>
> >> 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 <antonio.caggiano@collabora.com>
> >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> >> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> >> Signed-off-by: Huang Rui <ray.huang@amd.com>
> >> ---
> >>
> >> V4 -> V5:
> >>      - Use memory_region_init_ram_ptr() instead of
> >>        memory_region_init_ram_device_ptr() (Akihiko)
> >>
> >>   hw/display/virtio-gpu-virgl.c  | 213 +++++++++++++++++++++++++++++++++
> >>   hw/display/virtio-gpu.c        |   4 +-
> >>   include/hw/virtio/virtio-gpu.h |   5 +
> >>   meson.build                    |   4 +
> >>   4 files changed, 225 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/display/virtio-gpu-virgl.c 
> >> b/hw/display/virtio-gpu-virgl.c
> >> index 312953ec16..563a6f2f58 100644
> >> --- a/hw/display/virtio-gpu-virgl.c
> >> +++ b/hw/display/virtio-gpu-virgl.c
> >> @@ -17,6 +17,7 @@
> >>   #include "trace.h"
> >>   #include "hw/virtio/virtio.h"
> >>   #include "hw/virtio/virtio-gpu.h"
> >> +#include "hw/virtio/virtio-gpu-bswap.h"
> >>   #include "ui/egl-helpers.h"
> >> @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
> >>       virgl_renderer_resource_create(&args, NULL, 0);
> >>   }
> >> +static void virgl_resource_destroy(VirtIOGPU *g,
> >> +                                   struct virtio_gpu_simple_resource 
> >> *res)
> >> +{
> >> +    if (!res)
> >> +        return;
> >> +
> >> +    QTAILQ_REMOVE(&g->reslist, res, next);
> >> +
> >> +    virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt);
> >> +    g_free(res->addrs);
> >> +
> >> +    g_free(res);
> >> +}
> >> +
> >>   static void virgl_cmd_resource_unref(VirtIOGPU *g,
> >>                                        struct virtio_gpu_ctrl_command 
> >> *cmd)
> >>   {
> >> +    struct virtio_gpu_simple_resource *res;
> >>       struct virtio_gpu_resource_unref unref;
> >>       struct iovec *res_iovs = NULL;
> >>       int num_iovs = 0;
> >> @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
> >>       VIRTIO_GPU_FILL_CMD(unref);
> >>       trace_virtio_gpu_cmd_res_unref(unref.resource_id);
> >> +    res = virtio_gpu_find_resource(g, unref.resource_id);
> >> +
> >>       virgl_renderer_resource_detach_iov(unref.resource_id,
> >>                                          &res_iovs,
> >>                                          &num_iovs);
> >>       if (res_iovs != NULL && num_iovs != 0) {
> >>           virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
> >> +        if (res) {
> >> +            res->iov = NULL;
> >> +            res->iov_cnt = 0;
> >> +        }
> >>       }
> >> +
> >>       virgl_renderer_resource_unref(unref.resource_id);
> >> +
> >> +    virgl_resource_destroy(g, res);
> 
> This may leak memory region.

The memory region should be freed under virgl_cmd_resource_unmap_blob()
which is calling memory_region_del_subregion(&b->hostmem, res->region).
Because this region is created by map_blob(). Do we have the case to call
virgl_cmd_resource_unref() without calling virgl_cmd_resource_unmap_blob()
for blob memory?

Thanks,
Ray

Re: [QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands

[Akihiko Odaki]

On 2023/09/20 14:50, Huang Rui wrote:
> On Sat, Sep 16, 2023 at 12:37:29AM +0800, Akihiko Odaki wrote:
>> On 2023/09/16 1:04, Akihiko Odaki wrote:
>>> On 2023/09/15 20:11, Huang Rui wrote:
>>>> From: Antonio Caggiano <antonio.caggiano@collabora.com>
>>>>
>>>> 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 <antonio.caggiano@collabora.com>
>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>>> ---
>>>>
>>>> V4 -> V5:
>>>>       - Use memory_region_init_ram_ptr() instead of
>>>>         memory_region_init_ram_device_ptr() (Akihiko)
>>>>
>>>>    hw/display/virtio-gpu-virgl.c  | 213 +++++++++++++++++++++++++++++++++
>>>>    hw/display/virtio-gpu.c        |   4 +-
>>>>    include/hw/virtio/virtio-gpu.h |   5 +
>>>>    meson.build                    |   4 +
>>>>    4 files changed, 225 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/display/virtio-gpu-virgl.c
>>>> b/hw/display/virtio-gpu-virgl.c
>>>> index 312953ec16..563a6f2f58 100644
>>>> --- a/hw/display/virtio-gpu-virgl.c
>>>> +++ b/hw/display/virtio-gpu-virgl.c
>>>> @@ -17,6 +17,7 @@
>>>>    #include "trace.h"
>>>>    #include "hw/virtio/virtio.h"
>>>>    #include "hw/virtio/virtio-gpu.h"
>>>> +#include "hw/virtio/virtio-gpu-bswap.h"
>>>>    #include "ui/egl-helpers.h"
>>>> @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
>>>>        virgl_renderer_resource_create(&args, NULL, 0);
>>>>    }
>>>> +static void virgl_resource_destroy(VirtIOGPU *g,
>>>> +                                   struct virtio_gpu_simple_resource
>>>> *res)
>>>> +{
>>>> +    if (!res)
>>>> +        return;
>>>> +
>>>> +    QTAILQ_REMOVE(&g->reslist, res, next);
>>>> +
>>>> +    virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt);
>>>> +    g_free(res->addrs);
>>>> +
>>>> +    g_free(res);
>>>> +}
>>>> +
>>>>    static void virgl_cmd_resource_unref(VirtIOGPU *g,
>>>>                                         struct virtio_gpu_ctrl_command
>>>> *cmd)
>>>>    {
>>>> +    struct virtio_gpu_simple_resource *res;
>>>>        struct virtio_gpu_resource_unref unref;
>>>>        struct iovec *res_iovs = NULL;
>>>>        int num_iovs = 0;
>>>> @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
>>>>        VIRTIO_GPU_FILL_CMD(unref);
>>>>        trace_virtio_gpu_cmd_res_unref(unref.resource_id);
>>>> +    res = virtio_gpu_find_resource(g, unref.resource_id);
>>>> +
>>>>        virgl_renderer_resource_detach_iov(unref.resource_id,
>>>>                                           &res_iovs,
>>>>                                           &num_iovs);
>>>>        if (res_iovs != NULL && num_iovs != 0) {
>>>>            virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
>>>> +        if (res) {
>>>> +            res->iov = NULL;
>>>> +            res->iov_cnt = 0;
>>>> +        }
>>>>        }
>>>> +
>>>>        virgl_renderer_resource_unref(unref.resource_id);
>>>> +
>>>> +    virgl_resource_destroy(g, res);
>>
>> This may leak memory region.
> 
> The memory region should be freed under virgl_cmd_resource_unmap_blob()
> which is calling memory_region_del_subregion(&b->hostmem, res->region).
> Because this region is created by map_blob(). Do we have the case to call
> virgl_cmd_resource_unref() without calling virgl_cmd_resource_unmap_blob()
> for blob memory?

Calling virgl_cmd_resource_unmap_blob() and virgl_cmd_resource_unref() 
in order is a guest's responsibility, and we are required to prepare for 
broken guests.

Re: [QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands

[Huang Rui via]

On Wed, Sep 20, 2023 at 01:54:03PM +0800, Akihiko Odaki wrote:
> On 2023/09/20 14:50, Huang Rui wrote:
> > On Sat, Sep 16, 2023 at 12:37:29AM +0800, Akihiko Odaki wrote:
> >> On 2023/09/16 1:04, Akihiko Odaki wrote:
> >>> On 2023/09/15 20:11, Huang Rui wrote:
> >>>> From: Antonio Caggiano <antonio.caggiano@collabora.com>
> >>>>
> >>>> 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 <antonio.caggiano@collabora.com>
> >>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> >>>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> >>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
> >>>> ---
> >>>>
> >>>> V4 -> V5:
> >>>>       - Use memory_region_init_ram_ptr() instead of
> >>>>         memory_region_init_ram_device_ptr() (Akihiko)
> >>>>
> >>>>    hw/display/virtio-gpu-virgl.c  | 213 +++++++++++++++++++++++++++++++++
> >>>>    hw/display/virtio-gpu.c        |   4 +-
> >>>>    include/hw/virtio/virtio-gpu.h |   5 +
> >>>>    meson.build                    |   4 +
> >>>>    4 files changed, 225 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/hw/display/virtio-gpu-virgl.c
> >>>> b/hw/display/virtio-gpu-virgl.c
> >>>> index 312953ec16..563a6f2f58 100644
> >>>> --- a/hw/display/virtio-gpu-virgl.c
> >>>> +++ b/hw/display/virtio-gpu-virgl.c
> >>>> @@ -17,6 +17,7 @@
> >>>>    #include "trace.h"
> >>>>    #include "hw/virtio/virtio.h"
> >>>>    #include "hw/virtio/virtio-gpu.h"
> >>>> +#include "hw/virtio/virtio-gpu-bswap.h"
> >>>>    #include "ui/egl-helpers.h"
> >>>> @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
> >>>>        virgl_renderer_resource_create(&args, NULL, 0);
> >>>>    }
> >>>> +static void virgl_resource_destroy(VirtIOGPU *g,
> >>>> +                                   struct virtio_gpu_simple_resource
> >>>> *res)
> >>>> +{
> >>>> +    if (!res)
> >>>> +        return;
> >>>> +
> >>>> +    QTAILQ_REMOVE(&g->reslist, res, next);
> >>>> +
> >>>> +    virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt);
> >>>> +    g_free(res->addrs);
> >>>> +
> >>>> +    g_free(res);
> >>>> +}
> >>>> +
> >>>>    static void virgl_cmd_resource_unref(VirtIOGPU *g,
> >>>>                                         struct virtio_gpu_ctrl_command
> >>>> *cmd)
> >>>>    {
> >>>> +    struct virtio_gpu_simple_resource *res;
> >>>>        struct virtio_gpu_resource_unref unref;
> >>>>        struct iovec *res_iovs = NULL;
> >>>>        int num_iovs = 0;
> >>>> @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
> >>>>        VIRTIO_GPU_FILL_CMD(unref);
> >>>>        trace_virtio_gpu_cmd_res_unref(unref.resource_id);
> >>>> +    res = virtio_gpu_find_resource(g, unref.resource_id);
> >>>> +
> >>>>        virgl_renderer_resource_detach_iov(unref.resource_id,
> >>>>                                           &res_iovs,
> >>>>                                           &num_iovs);
> >>>>        if (res_iovs != NULL && num_iovs != 0) {
> >>>>            virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
> >>>> +        if (res) {
> >>>> +            res->iov = NULL;
> >>>> +            res->iov_cnt = 0;
> >>>> +        }
> >>>>        }
> >>>> +
> >>>>        virgl_renderer_resource_unref(unref.resource_id);
> >>>> +
> >>>> +    virgl_resource_destroy(g, res);
> >>
> >> This may leak memory region.
> > 
> > The memory region should be freed under virgl_cmd_resource_unmap_blob()
> > which is calling memory_region_del_subregion(&b->hostmem, res->region).
> > Because this region is created by map_blob(). Do we have the case to call
> > virgl_cmd_resource_unref() without calling virgl_cmd_resource_unmap_blob()
> > for blob memory?
> 
> Calling virgl_cmd_resource_unmap_blob() and virgl_cmd_resource_unref() 
> in order is a guest's responsibility, and we are required to prepare for 
> broken guests.

OK, no problem. I will update this in V6.

Thanks,
Ray

Re: [QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands

[Huang Rui]

On Sat, Sep 16, 2023 at 12:04:17AM +0800, Akihiko Odaki wrote:
> On 2023/09/15 20:11, Huang Rui wrote:
> > From: Antonio Caggiano <antonio.caggiano@collabora.com>
> > 
> > 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 <antonio.caggiano@collabora.com>
> > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> > Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> > 
> > V4 -> V5:
> >      - Use memory_region_init_ram_ptr() instead of
> >        memory_region_init_ram_device_ptr() (Akihiko)
> > 
> >   hw/display/virtio-gpu-virgl.c  | 213 +++++++++++++++++++++++++++++++++
> >   hw/display/virtio-gpu.c        |   4 +-
> >   include/hw/virtio/virtio-gpu.h |   5 +
> >   meson.build                    |   4 +
> >   4 files changed, 225 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> > index 312953ec16..563a6f2f58 100644
> > --- a/hw/display/virtio-gpu-virgl.c
> > +++ b/hw/display/virtio-gpu-virgl.c
> > @@ -17,6 +17,7 @@
> >   #include "trace.h"
> >   #include "hw/virtio/virtio.h"
> >   #include "hw/virtio/virtio-gpu.h"
> > +#include "hw/virtio/virtio-gpu-bswap.h"
> >   
> >   #include "ui/egl-helpers.h"
> >   
> > @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
> >       virgl_renderer_resource_create(&args, NULL, 0);
> >   }
> >   
> > +static void virgl_resource_destroy(VirtIOGPU *g,
> > +                                   struct virtio_gpu_simple_resource *res)
> > +{
> > +    if (!res)
> > +        return;
> > +
> > +    QTAILQ_REMOVE(&g->reslist, res, next);
> > +
> > +    virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt);
> > +    g_free(res->addrs);
> > +
> > +    g_free(res);
> > +}
> > +
> >   static void virgl_cmd_resource_unref(VirtIOGPU *g,
> >                                        struct virtio_gpu_ctrl_command *cmd)
> >   {
> > +    struct virtio_gpu_simple_resource *res;
> >       struct virtio_gpu_resource_unref unref;
> >       struct iovec *res_iovs = NULL;
> >       int num_iovs = 0;
> > @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
> >       VIRTIO_GPU_FILL_CMD(unref);
> >       trace_virtio_gpu_cmd_res_unref(unref.resource_id);
> >   
> > +    res = virtio_gpu_find_resource(g, unref.resource_id);
> > +
> >       virgl_renderer_resource_detach_iov(unref.resource_id,
> >                                          &res_iovs,
> >                                          &num_iovs);
> >       if (res_iovs != NULL && num_iovs != 0) {
> >           virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
> > +        if (res) {
> > +            res->iov = NULL;
> > +            res->iov_cnt = 0;
> > +        }
> >       }
> > +
> >       virgl_renderer_resource_unref(unref.resource_id);
> > +
> > +    virgl_resource_destroy(g, res);
> >   }
> >   
> >   static void virgl_cmd_context_create(VirtIOGPU *g,
> > @@ -426,6 +451,183 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
> >       g_free(resp);
> >   }
> >   
> > +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> > +
> > +static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
> > +                                           struct virtio_gpu_ctrl_command *cmd)
> > +{
> > +    struct virtio_gpu_simple_resource *res;
> > +    struct virtio_gpu_resource_create_blob cblob;
> > +    struct virgl_renderer_resource_create_blob_args virgl_args = { 0 };
> > +    int ret;
> > +
> > +    VIRTIO_GPU_FILL_CMD(cblob);
> > +    virtio_gpu_create_blob_bswap(&cblob);
> > +    trace_virtio_gpu_cmd_res_create_blob(cblob.resource_id, cblob.size);
> > +
> > +    if (cblob.resource_id == 0) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n",
> > +                      __func__);
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > +        return;
> > +    }
> > +
> > +    res = virtio_gpu_find_resource(g, cblob.resource_id);
> > +    if (res) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
> > +                      __func__, cblob.resource_id);
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > +        return;
> > +    }
> > +
> > +    res = g_new0(struct virtio_gpu_simple_resource, 1);
> > +    if (!res) {
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> > +        return;
> > +    }
> > +
> > +    res->resource_id = cblob.resource_id;
> > +    res->blob_size = cblob.size;
> > +
> > +    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) {
> > +        ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
> > +                                            cmd, &res->addrs, &res->iov,
> > +                                            &res->iov_cnt);
> > +        if (!ret) {
> > +            g_free(res);
> > +            cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> > +            return;
> > +        }
> > +    }
> > +
> > +    QTAILQ_INSERT_HEAD(&g->reslist, res, next);
> > +
> > +    virgl_args.res_handle = cblob.resource_id;
> > +    virgl_args.ctx_id = cblob.hdr.ctx_id;
> > +    virgl_args.blob_mem = cblob.blob_mem;
> > +    virgl_args.blob_id = cblob.blob_id;
> > +    virgl_args.blob_flags = cblob.blob_flags;
> > +    virgl_args.size = cblob.size;
> > +    virgl_args.iovecs = res->iov;
> > +    virgl_args.num_iovs = res->iov_cnt;
> > +
> > +    ret = virgl_renderer_resource_create_blob(&virgl_args);
> > +    if (ret) {
> > +        virgl_resource_destroy(g, res);
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: virgl blob create error: %s\n",
> > +                      __func__, strerror(-ret));
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> > +    }
> > +}
> > +
> > +static void virgl_cmd_resource_map_blob(VirtIOGPU *g,
> > +                                        struct virtio_gpu_ctrl_command *cmd)
> > +{
> > +    struct virtio_gpu_simple_resource *res;
> > +    struct virtio_gpu_resource_map_blob mblob;
> > +    int ret;
> > +    void *data;
> > +    uint64_t size;
> > +    struct virtio_gpu_resp_map_info resp;
> > +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> > +
> > +    VIRTIO_GPU_FILL_CMD(mblob);
> > +    virtio_gpu_map_blob_bswap(&mblob);
> > +
> > +    if (mblob.resource_id == 0) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n",
> > +                      __func__);
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > +        return;
> > +    }
> > +
> > +    res = virtio_gpu_find_resource(g, mblob.resource_id);
> > +    if (!res) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n",
> > +                      __func__, mblob.resource_id);
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > +        return;
> > +    }
> > +    if (res->region) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already mapped %d\n",
> > +		      __func__, mblob.resource_id);
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > +        return;
> > +    }
> > +
> > +    ret = virgl_renderer_resource_map(res->resource_id, &data, &size);
> > +    if (ret) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n",
> > +                      __func__, strerror(-ret));
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > +        return;
> > +    }
> > +
> > +    res->region = g_new0(MemoryRegion, 1);
> > +    if (!res->region) {
> > +        virgl_renderer_resource_unmap(res->resource_id);
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> > +        return;
> > +    }
> > +    memory_region_init_ram_ptr(res->region, OBJECT(g), NULL, size, data);
> > +    OBJECT(res->region)->free = g_free;
> > +    memory_region_add_subregion(&b->hostmem, mblob.offset, res->region);
> > +    memory_region_set_enabled(res->region, true);
> > +
> > +    memset(&resp, 0, sizeof(resp));
> > +    resp.hdr.type = VIRTIO_GPU_RESP_OK_MAP_INFO;
> > +    virgl_renderer_resource_get_map_info(mblob.resource_id, &resp.map_info);
> > +    virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
> > +}
> > +
> > +static int virtio_gpu_virgl_resource_unmap(VirtIOGPU *g,
> > +                                           struct virtio_gpu_simple_resource
> > +                                           *res)
> > +{
> > +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> > +
> > +    if (!res) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already unmapped %d\n",
> > +                      __func__, res->resource_id);
> > +        return VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> 
> This returns enum virtio_gpu_ctrl_type while 
> virgl_renderer_resource_unmap() returns -errno and the returned error 
> code is ignored.
> 
> I think you can just inline this function into 
> virgl_cmd_resource_unmap_blob(). It will be more concise and consistent 
> with virgl_cmd_resource_map_blob().

OK, will update in V2.

> 
> > +    }
> > +
> > +    memory_region_set_enabled(res->region, false);
> > +    memory_region_del_subregion(&b->hostmem, res->region);
> > +    object_unparent(OBJECT(res->region));
> > +    res->region = NULL;
> > +
> > +    return virgl_renderer_resource_unmap(res->resource_id);
> > +}
> > +
> > +static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g,
> > +                                          struct virtio_gpu_ctrl_command *cmd)
> > +{
> > +    struct virtio_gpu_simple_resource *res;
> > +    struct virtio_gpu_resource_unmap_blob ublob;
> > +    VIRTIO_GPU_FILL_CMD(ublob);
> > +    virtio_gpu_unmap_blob_bswap(&ublob);
> > +
> > +    if (ublob.resource_id == 0) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n",
> > +                      __func__);
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > +        return;
> > +    }
> > +
> > +    res = virtio_gpu_find_resource(g, ublob.resource_id);
> > +    if (!res) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n",
> > +                      __func__, ublob.resource_id);
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > +        return;
> > +    }
> > +
> > +    virtio_gpu_virgl_resource_unmap(g, res);
> > +}
> > +
> > +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
> > +
> >   void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> >                                         struct virtio_gpu_ctrl_command *cmd)
> >   {
> > @@ -492,6 +694,17 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> >       case VIRTIO_GPU_CMD_GET_EDID:
> >           virtio_gpu_get_edid(g, cmd);
> >           break;
> > +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> > +    case VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB:
> > +        virgl_cmd_resource_create_blob(g, cmd);
> > +        break;
> > +    case VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB:
> > +        virgl_cmd_resource_map_blob(g, cmd);
> > +        break;
> > +    case VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB:
> > +        virgl_cmd_resource_unmap_blob(g, cmd);
> > +        break;
> > +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
> >       default:
> >           cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> >           break;
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index 5b7a7eab4f..cc4c1f81bb 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -1367,10 +1367,12 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
> >               return;
> >           }
> >   
> > +#ifndef HAVE_VIRGL_RESOURCE_BLOB
> >           if (virtio_gpu_virgl_enabled(g->parent_obj.conf)) {
> > -            error_setg(errp, "blobs and virgl are not compatible (yet)");
> > +            error_setg(errp, "Linked virglrenderer does not support blob resources");
> >               return;
> >           }
> > +#endif
> >       }
> >   
> >       if (!virtio_gpu_base_device_realize(qdev,
> > diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> > index 55973e112f..b9adc28071 100644
> > --- a/include/hw/virtio/virtio-gpu.h
> > +++ b/include/hw/virtio/virtio-gpu.h
> > @@ -58,6 +58,11 @@ struct virtio_gpu_simple_resource {
> >       int dmabuf_fd;
> >       uint8_t *remapped;
> >   
> > +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> > +    /* only blob resource needs this region to be mapped as guest mmio */
> > +    MemoryRegion *region;
> > +#endif
> > +
> 
> Since this is specific to virgl, let's create a struct that embeds 
> virtio_gpu_simple_resource and use it to avoid modifying virtio-gpu 
> common code.

I am thinking should I modify the whole virtio-gpu-virgl.c to use the new
struct like virgl_gpu_resource which has virtio_gpu_simple_resource
embedded? Then in virgl, we can use this structure to indicate the resource
for virglrenderer.

Thanks,
Ray

Re: [QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands

[Akihiko Odaki]

On 2023/09/18 17:36, Huang Rui wrote:
> On Sat, Sep 16, 2023 at 12:04:17AM +0800, Akihiko Odaki wrote:
>> On 2023/09/15 20:11, Huang Rui wrote:
>>> From: Antonio Caggiano <antonio.caggiano@collabora.com>
>>>
>>> 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 <antonio.caggiano@collabora.com>
>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
>>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>>> ---
>>>
>>> V4 -> V5:
>>>       - Use memory_region_init_ram_ptr() instead of
>>>         memory_region_init_ram_device_ptr() (Akihiko)
>>>
>>>    hw/display/virtio-gpu-virgl.c  | 213 +++++++++++++++++++++++++++++++++
>>>    hw/display/virtio-gpu.c        |   4 +-
>>>    include/hw/virtio/virtio-gpu.h |   5 +
>>>    meson.build                    |   4 +
>>>    4 files changed, 225 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>>> index 312953ec16..563a6f2f58 100644
>>> --- a/hw/display/virtio-gpu-virgl.c
>>> +++ b/hw/display/virtio-gpu-virgl.c
>>> @@ -17,6 +17,7 @@
>>>    #include "trace.h"
>>>    #include "hw/virtio/virtio.h"
>>>    #include "hw/virtio/virtio-gpu.h"
>>> +#include "hw/virtio/virtio-gpu-bswap.h"
>>>    
>>>    #include "ui/egl-helpers.h"
>>>    
>>> @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
>>>        virgl_renderer_resource_create(&args, NULL, 0);
>>>    }
>>>    
>>> +static void virgl_resource_destroy(VirtIOGPU *g,
>>> +                                   struct virtio_gpu_simple_resource *res)
>>> +{
>>> +    if (!res)
>>> +        return;
>>> +
>>> +    QTAILQ_REMOVE(&g->reslist, res, next);
>>> +
>>> +    virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt);
>>> +    g_free(res->addrs);
>>> +
>>> +    g_free(res);
>>> +}
>>> +
>>>    static void virgl_cmd_resource_unref(VirtIOGPU *g,
>>>                                         struct virtio_gpu_ctrl_command *cmd)
>>>    {
>>> +    struct virtio_gpu_simple_resource *res;
>>>        struct virtio_gpu_resource_unref unref;
>>>        struct iovec *res_iovs = NULL;
>>>        int num_iovs = 0;
>>> @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
>>>        VIRTIO_GPU_FILL_CMD(unref);
>>>        trace_virtio_gpu_cmd_res_unref(unref.resource_id);
>>>    
>>> +    res = virtio_gpu_find_resource(g, unref.resource_id);
>>> +
>>>        virgl_renderer_resource_detach_iov(unref.resource_id,
>>>                                           &res_iovs,
>>>                                           &num_iovs);
>>>        if (res_iovs != NULL && num_iovs != 0) {
>>>            virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
>>> +        if (res) {
>>> +            res->iov = NULL;
>>> +            res->iov_cnt = 0;
>>> +        }
>>>        }
>>> +
>>>        virgl_renderer_resource_unref(unref.resource_id);
>>> +
>>> +    virgl_resource_destroy(g, res);
>>>    }
>>>    
>>>    static void virgl_cmd_context_create(VirtIOGPU *g,
>>> @@ -426,6 +451,183 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
>>>        g_free(resp);
>>>    }
>>>    
>>> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
>>> +
>>> +static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
>>> +                                           struct virtio_gpu_ctrl_command *cmd)
>>> +{
>>> +    struct virtio_gpu_simple_resource *res;
>>> +    struct virtio_gpu_resource_create_blob cblob;
>>> +    struct virgl_renderer_resource_create_blob_args virgl_args = { 0 };
>>> +    int ret;
>>> +
>>> +    VIRTIO_GPU_FILL_CMD(cblob);
>>> +    virtio_gpu_create_blob_bswap(&cblob);
>>> +    trace_virtio_gpu_cmd_res_create_blob(cblob.resource_id, cblob.size);
>>> +
>>> +    if (cblob.resource_id == 0) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n",
>>> +                      __func__);
>>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
>>> +        return;
>>> +    }
>>> +
>>> +    res = virtio_gpu_find_resource(g, cblob.resource_id);
>>> +    if (res) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
>>> +                      __func__, cblob.resource_id);
>>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
>>> +        return;
>>> +    }
>>> +
>>> +    res = g_new0(struct virtio_gpu_simple_resource, 1);
>>> +    if (!res) {
>>> +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
>>> +        return;
>>> +    }
>>> +
>>> +    res->resource_id = cblob.resource_id;
>>> +    res->blob_size = cblob.size;
>>> +
>>> +    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) {
>>> +        ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
>>> +                                            cmd, &res->addrs, &res->iov,
>>> +                                            &res->iov_cnt);
>>> +        if (!ret) {
>>> +            g_free(res);
>>> +            cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>>> +            return;
>>> +        }
>>> +    }
>>> +
>>> +    QTAILQ_INSERT_HEAD(&g->reslist, res, next);
>>> +
>>> +    virgl_args.res_handle = cblob.resource_id;
>>> +    virgl_args.ctx_id = cblob.hdr.ctx_id;
>>> +    virgl_args.blob_mem = cblob.blob_mem;
>>> +    virgl_args.blob_id = cblob.blob_id;
>>> +    virgl_args.blob_flags = cblob.blob_flags;
>>> +    virgl_args.size = cblob.size;
>>> +    virgl_args.iovecs = res->iov;
>>> +    virgl_args.num_iovs = res->iov_cnt;
>>> +
>>> +    ret = virgl_renderer_resource_create_blob(&virgl_args);
>>> +    if (ret) {
>>> +        virgl_resource_destroy(g, res);
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: virgl blob create error: %s\n",
>>> +                      __func__, strerror(-ret));
>>> +        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>>> +    }
>>> +}
>>> +
>>> +static void virgl_cmd_resource_map_blob(VirtIOGPU *g,
>>> +                                        struct virtio_gpu_ctrl_command *cmd)
>>> +{
>>> +    struct virtio_gpu_simple_resource *res;
>>> +    struct virtio_gpu_resource_map_blob mblob;
>>> +    int ret;
>>> +    void *data;
>>> +    uint64_t size;
>>> +    struct virtio_gpu_resp_map_info resp;
>>> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
>>> +
>>> +    VIRTIO_GPU_FILL_CMD(mblob);
>>> +    virtio_gpu_map_blob_bswap(&mblob);
>>> +
>>> +    if (mblob.resource_id == 0) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n",
>>> +                      __func__);
>>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
>>> +        return;
>>> +    }
>>> +
>>> +    res = virtio_gpu_find_resource(g, mblob.resource_id);
>>> +    if (!res) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n",
>>> +                      __func__, mblob.resource_id);
>>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
>>> +        return;
>>> +    }
>>> +    if (res->region) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already mapped %d\n",
>>> +		      __func__, mblob.resource_id);
>>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
>>> +        return;
>>> +    }
>>> +
>>> +    ret = virgl_renderer_resource_map(res->resource_id, &data, &size);
>>> +    if (ret) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n",
>>> +                      __func__, strerror(-ret));
>>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
>>> +        return;
>>> +    }
>>> +
>>> +    res->region = g_new0(MemoryRegion, 1);
>>> +    if (!res->region) {
>>> +        virgl_renderer_resource_unmap(res->resource_id);
>>> +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
>>> +        return;
>>> +    }
>>> +    memory_region_init_ram_ptr(res->region, OBJECT(g), NULL, size, data);
>>> +    OBJECT(res->region)->free = g_free;
>>> +    memory_region_add_subregion(&b->hostmem, mblob.offset, res->region);
>>> +    memory_region_set_enabled(res->region, true);
>>> +
>>> +    memset(&resp, 0, sizeof(resp));
>>> +    resp.hdr.type = VIRTIO_GPU_RESP_OK_MAP_INFO;
>>> +    virgl_renderer_resource_get_map_info(mblob.resource_id, &resp.map_info);
>>> +    virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
>>> +}
>>> +
>>> +static int virtio_gpu_virgl_resource_unmap(VirtIOGPU *g,
>>> +                                           struct virtio_gpu_simple_resource
>>> +                                           *res)
>>> +{
>>> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
>>> +
>>> +    if (!res) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already unmapped %d\n",
>>> +                      __func__, res->resource_id);
>>> +        return VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
>>
>> This returns enum virtio_gpu_ctrl_type while
>> virgl_renderer_resource_unmap() returns -errno and the returned error
>> code is ignored.
>>
>> I think you can just inline this function into
>> virgl_cmd_resource_unmap_blob(). It will be more concise and consistent
>> with virgl_cmd_resource_map_blob().
> 
> OK, will update in V2.
> 
>>
>>> +    }
>>> +
>>> +    memory_region_set_enabled(res->region, false);
>>> +    memory_region_del_subregion(&b->hostmem, res->region);
>>> +    object_unparent(OBJECT(res->region));
>>> +    res->region = NULL;
>>> +
>>> +    return virgl_renderer_resource_unmap(res->resource_id);
>>> +}
>>> +
>>> +static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g,
>>> +                                          struct virtio_gpu_ctrl_command *cmd)
>>> +{
>>> +    struct virtio_gpu_simple_resource *res;
>>> +    struct virtio_gpu_resource_unmap_blob ublob;
>>> +    VIRTIO_GPU_FILL_CMD(ublob);
>>> +    virtio_gpu_unmap_blob_bswap(&ublob);
>>> +
>>> +    if (ublob.resource_id == 0) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n",
>>> +                      __func__);
>>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
>>> +        return;
>>> +    }
>>> +
>>> +    res = virtio_gpu_find_resource(g, ublob.resource_id);
>>> +    if (!res) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n",
>>> +                      __func__, ublob.resource_id);
>>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
>>> +        return;
>>> +    }
>>> +
>>> +    virtio_gpu_virgl_resource_unmap(g, res);
>>> +}
>>> +
>>> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
>>> +
>>>    void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>>>                                          struct virtio_gpu_ctrl_command *cmd)
>>>    {
>>> @@ -492,6 +694,17 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>>>        case VIRTIO_GPU_CMD_GET_EDID:
>>>            virtio_gpu_get_edid(g, cmd);
>>>            break;
>>> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
>>> +    case VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB:
>>> +        virgl_cmd_resource_create_blob(g, cmd);
>>> +        break;
>>> +    case VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB:
>>> +        virgl_cmd_resource_map_blob(g, cmd);
>>> +        break;
>>> +    case VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB:
>>> +        virgl_cmd_resource_unmap_blob(g, cmd);
>>> +        break;
>>> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
>>>        default:
>>>            cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>>>            break;
>>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>>> index 5b7a7eab4f..cc4c1f81bb 100644
>>> --- a/hw/display/virtio-gpu.c
>>> +++ b/hw/display/virtio-gpu.c
>>> @@ -1367,10 +1367,12 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
>>>                return;
>>>            }
>>>    
>>> +#ifndef HAVE_VIRGL_RESOURCE_BLOB
>>>            if (virtio_gpu_virgl_enabled(g->parent_obj.conf)) {
>>> -            error_setg(errp, "blobs and virgl are not compatible (yet)");
>>> +            error_setg(errp, "Linked virglrenderer does not support blob resources");
>>>                return;
>>>            }
>>> +#endif
>>>        }
>>>    
>>>        if (!virtio_gpu_base_device_realize(qdev,
>>> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
>>> index 55973e112f..b9adc28071 100644
>>> --- a/include/hw/virtio/virtio-gpu.h
>>> +++ b/include/hw/virtio/virtio-gpu.h
>>> @@ -58,6 +58,11 @@ struct virtio_gpu_simple_resource {
>>>        int dmabuf_fd;
>>>        uint8_t *remapped;
>>>    
>>> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
>>> +    /* only blob resource needs this region to be mapped as guest mmio */
>>> +    MemoryRegion *region;
>>> +#endif
>>> +
>>
>> Since this is specific to virgl, let's create a struct that embeds
>> virtio_gpu_simple_resource and use it to avoid modifying virtio-gpu
>> common code.
> 
> I am thinking should I modify the whole virtio-gpu-virgl.c to use the new
> struct like virgl_gpu_resource which has virtio_gpu_simple_resource
> embedded? Then in virgl, we can use this structure to indicate the resource
> for virglrenderer.

Yes, that's what I suggest.

Re: [QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands

[Marc-André Lureau]

Hi

On Fri, Sep 15, 2023 at 3:14 PM Huang Rui <ray.huang@amd.com> wrote:
>
> From: Antonio Caggiano <antonio.caggiano@collabora.com>
>
> 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 <antonio.caggiano@collabora.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
>
> V4 -> V5:
>     - Use memory_region_init_ram_ptr() instead of
>       memory_region_init_ram_device_ptr() (Akihiko)
>
>  hw/display/virtio-gpu-virgl.c  | 213 +++++++++++++++++++++++++++++++++
>  hw/display/virtio-gpu.c        |   4 +-
>  include/hw/virtio/virtio-gpu.h |   5 +
>  meson.build                    |   4 +
>  4 files changed, 225 insertions(+), 1 deletion(-)
>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 312953ec16..563a6f2f58 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -17,6 +17,7 @@
>  #include "trace.h"
>  #include "hw/virtio/virtio.h"
>  #include "hw/virtio/virtio-gpu.h"
> +#include "hw/virtio/virtio-gpu-bswap.h"
>
>  #include "ui/egl-helpers.h"
>
> @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
>      virgl_renderer_resource_create(&args, NULL, 0);
>  }
>
> +static void virgl_resource_destroy(VirtIOGPU *g,
> +                                   struct virtio_gpu_simple_resource *res)
> +{
> +    if (!res)
> +        return;
> +

QEMU coding style imposes braces

> +    QTAILQ_REMOVE(&g->reslist, res, next);
> +
> +    virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt);
> +    g_free(res->addrs);
> +
> +    g_free(res);

If you embed virtio_gpu_simple_resource in a new struct, we should
instead call the existing virtio_gpu_resource_destroy() I guess.

> +}
> +
>  static void virgl_cmd_resource_unref(VirtIOGPU *g,
>                                       struct virtio_gpu_ctrl_command *cmd)
>  {
> +    struct virtio_gpu_simple_resource *res;
>      struct virtio_gpu_resource_unref unref;
>      struct iovec *res_iovs = NULL;
>      int num_iovs = 0;
> @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
>      VIRTIO_GPU_FILL_CMD(unref);
>      trace_virtio_gpu_cmd_res_unref(unref.resource_id);
>
> +    res = virtio_gpu_find_resource(g, unref.resource_id);
> +
>      virgl_renderer_resource_detach_iov(unref.resource_id,
>                                         &res_iovs,
>                                         &num_iovs);
>      if (res_iovs != NULL && num_iovs != 0) {
>          virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
> +        if (res) {
> +            res->iov = NULL;
> +            res->iov_cnt = 0;
> +        }
>      }
> +
>      virgl_renderer_resource_unref(unref.resource_id);
> +
> +    virgl_resource_destroy(g, res);
>  }
>
>  static void virgl_cmd_context_create(VirtIOGPU *g,
> @@ -426,6 +451,183 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
>      g_free(resp);
>  }
>
> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> +
> +static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
> +                                           struct virtio_gpu_ctrl_command *cmd)
> +{
> +    struct virtio_gpu_simple_resource *res;
> +    struct virtio_gpu_resource_create_blob cblob;
> +    struct virgl_renderer_resource_create_blob_args virgl_args = { 0 };
> +    int ret;
> +
> +    VIRTIO_GPU_FILL_CMD(cblob);
> +    virtio_gpu_create_blob_bswap(&cblob);
> +    trace_virtio_gpu_cmd_res_create_blob(cblob.resource_id, cblob.size);
> +
> +    if (cblob.resource_id == 0) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n",
> +                      __func__);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +
> +    res = virtio_gpu_find_resource(g, cblob.resource_id);
> +    if (res) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
> +                      __func__, cblob.resource_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +
> +    res = g_new0(struct virtio_gpu_simple_resource, 1);
> +    if (!res) {
> +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> +        return;

needless error handling, glib will abort() on OOM.

> +    }
> +
> +    res->resource_id = cblob.resource_id;
> +    res->blob_size = cblob.size;
> +
> +    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) {
> +        ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
> +                                            cmd, &res->addrs, &res->iov,
> +                                            &res->iov_cnt);
> +        if (!ret) {
> +            g_free(res);
> +            cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> +            return;
> +        }
> +    }
> +
> +    QTAILQ_INSERT_HEAD(&g->reslist, res, next);
> +
> +    virgl_args.res_handle = cblob.resource_id;
> +    virgl_args.ctx_id = cblob.hdr.ctx_id;
> +    virgl_args.blob_mem = cblob.blob_mem;
> +    virgl_args.blob_id = cblob.blob_id;
> +    virgl_args.blob_flags = cblob.blob_flags;
> +    virgl_args.size = cblob.size;
> +    virgl_args.iovecs = res->iov;
> +    virgl_args.num_iovs = res->iov_cnt;
> +
> +    ret = virgl_renderer_resource_create_blob(&virgl_args);
> +    if (ret) {
> +        virgl_resource_destroy(g, res);
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: virgl blob create error: %s\n",
> +                      __func__, strerror(-ret));
> +        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> +    }
> +}
> +
> +static void virgl_cmd_resource_map_blob(VirtIOGPU *g,
> +                                        struct virtio_gpu_ctrl_command *cmd)
> +{
> +    struct virtio_gpu_simple_resource *res;
> +    struct virtio_gpu_resource_map_blob mblob;
> +    int ret;
> +    void *data;
> +    uint64_t size;
> +    struct virtio_gpu_resp_map_info resp;
> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> +
> +    VIRTIO_GPU_FILL_CMD(mblob);
> +    virtio_gpu_map_blob_bswap(&mblob);
> +
> +    if (mblob.resource_id == 0) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n",
> +                      __func__);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +
> +    res = virtio_gpu_find_resource(g, mblob.resource_id);
> +    if (!res) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n",
> +                      __func__, mblob.resource_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +    if (res->region) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already mapped %d\n",
> +                     __func__, mblob.resource_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +
> +    ret = virgl_renderer_resource_map(res->resource_id, &data, &size);
> +    if (ret) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n",
> +                      __func__, strerror(-ret));
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +
> +    res->region = g_new0(MemoryRegion, 1);
> +    if (!res->region) {

no need for OOM handling here too

> +        virgl_renderer_resource_unmap(res->resource_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> +        return;
> +    }
> +    memory_region_init_ram_ptr(res->region, OBJECT(g), NULL, size, data);
> +    OBJECT(res->region)->free = g_free;
> +    memory_region_add_subregion(&b->hostmem, mblob.offset, res->region);
> +    memory_region_set_enabled(res->region, true);
> +
> +    memset(&resp, 0, sizeof(resp));
> +    resp.hdr.type = VIRTIO_GPU_RESP_OK_MAP_INFO;
> +    virgl_renderer_resource_get_map_info(mblob.resource_id, &resp.map_info);
> +    virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
> +}
> +
> +static int virtio_gpu_virgl_resource_unmap(VirtIOGPU *g,
> +                                           struct virtio_gpu_simple_resource
> +                                           *res)
> +{
> +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> +
> +    if (!res) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already unmapped %d\n",
> +                      __func__, res->resource_id);
> +        return VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +    }
> +
> +    memory_region_set_enabled(res->region, false);
> +    memory_region_del_subregion(&b->hostmem, res->region);
> +    object_unparent(OBJECT(res->region));
> +    res->region = NULL;
> +
> +    return virgl_renderer_resource_unmap(res->resource_id);
> +}
> +
> +static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g,
> +                                          struct virtio_gpu_ctrl_command *cmd)
> +{
> +    struct virtio_gpu_simple_resource *res;
> +    struct virtio_gpu_resource_unmap_blob ublob;
> +    VIRTIO_GPU_FILL_CMD(ublob);
> +    virtio_gpu_unmap_blob_bswap(&ublob);
> +
> +    if (ublob.resource_id == 0) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n",
> +                      __func__);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +
> +    res = virtio_gpu_find_resource(g, ublob.resource_id);
> +    if (!res) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n",
> +                      __func__, ublob.resource_id);
> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> +        return;
> +    }
> +
> +    virtio_gpu_virgl_resource_unmap(g, res);
> +}
> +
> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
> +
>  void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>                                        struct virtio_gpu_ctrl_command *cmd)
>  {
> @@ -492,6 +694,17 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>      case VIRTIO_GPU_CMD_GET_EDID:
>          virtio_gpu_get_edid(g, cmd);
>          break;
> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> +    case VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB:
> +        virgl_cmd_resource_create_blob(g, cmd);
> +        break;
> +    case VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB:
> +        virgl_cmd_resource_map_blob(g, cmd);
> +        break;
> +    case VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB:
> +        virgl_cmd_resource_unmap_blob(g, cmd);
> +        break;
> +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
>      default:
>          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>          break;
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 5b7a7eab4f..cc4c1f81bb 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1367,10 +1367,12 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
>              return;
>          }
>
> +#ifndef HAVE_VIRGL_RESOURCE_BLOB
>          if (virtio_gpu_virgl_enabled(g->parent_obj.conf)) {
> -            error_setg(errp, "blobs and virgl are not compatible (yet)");
> +            error_setg(errp, "Linked virglrenderer does not support blob resources");
>              return;
>          }
> +#endif
>      }
>
>      if (!virtio_gpu_base_device_realize(qdev,
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index 55973e112f..b9adc28071 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -58,6 +58,11 @@ struct virtio_gpu_simple_resource {
>      int dmabuf_fd;
>      uint8_t *remapped;
>
> +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> +    /* only blob resource needs this region to be mapped as guest mmio */
> +    MemoryRegion *region;
> +#endif
> +
>      QTAILQ_ENTRY(virtio_gpu_simple_resource) next;
>  };
>
> diff --git a/meson.build b/meson.build
> index ff20d3c249..f7b744ab82 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1072,6 +1072,10 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
>                         cc.has_function('virgl_renderer_context_create_with_flags',
>                                         prefix: '#include <virglrenderer.h>',
>                                         dependencies: virgl))
> +  config_host_data.set('HAVE_VIRGL_RESOURCE_BLOB',
> +                       cc.has_function('virgl_renderer_resource_create_blob',
> +                                       prefix: '#include <virglrenderer.h>',
> +                                       dependencies: virgl))

better moved under the if virgl.found() block

>  endif
>  blkio = not_found
>  if not get_option('blkio').auto() or have_block
> --
> 2.34.1
>
>


-- 
Marc-André Lureau

Re: [QEMU PATCH v5 09/13] virtio-gpu: Handle resource blob commands

[Huang Rui]

On Tue, Sep 19, 2023 at 04:44:01PM +0800, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Sep 15, 2023 at 3:14 PM Huang Rui <ray.huang@amd.com> wrote:
> >
> > From: Antonio Caggiano <antonio.caggiano@collabora.com>
> >
> > 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 <antonio.caggiano@collabora.com>
> > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> > Signed-off-by: Xenia Ragiadakou <xenia.ragiadakou@amd.com>
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> >
> > V4 -> V5:
> >     - Use memory_region_init_ram_ptr() instead of
> >       memory_region_init_ram_device_ptr() (Akihiko)
> >
> >  hw/display/virtio-gpu-virgl.c  | 213 +++++++++++++++++++++++++++++++++
> >  hw/display/virtio-gpu.c        |   4 +-
> >  include/hw/virtio/virtio-gpu.h |   5 +
> >  meson.build                    |   4 +
> >  4 files changed, 225 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> > index 312953ec16..563a6f2f58 100644
> > --- a/hw/display/virtio-gpu-virgl.c
> > +++ b/hw/display/virtio-gpu-virgl.c
> > @@ -17,6 +17,7 @@
> >  #include "trace.h"
> >  #include "hw/virtio/virtio.h"
> >  #include "hw/virtio/virtio-gpu.h"
> > +#include "hw/virtio/virtio-gpu-bswap.h"
> >
> >  #include "ui/egl-helpers.h"
> >
> > @@ -78,9 +79,24 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
> >      virgl_renderer_resource_create(&args, NULL, 0);
> >  }
> >
> > +static void virgl_resource_destroy(VirtIOGPU *g,
> > +                                   struct virtio_gpu_simple_resource *res)
> > +{
> > +    if (!res)
> > +        return;
> > +
> 
> QEMU coding style imposes braces
> 

Thanks for reminder.

> > +    QTAILQ_REMOVE(&g->reslist, res, next);
> > +
> > +    virtio_gpu_cleanup_mapping_iov(g, res->iov, res->iov_cnt);
> > +    g_free(res->addrs);
> > +
> > +    g_free(res);
> 
> If you embed virtio_gpu_simple_resource in a new struct, we should
> instead call the existing virtio_gpu_resource_destroy() I guess.

Yes, but I didn't call virtio_gpu_resource_destroy() here, I am using the
virgl_resource_destroy() and the input should be the virgl resource in next
version. May I know whether I miss understood your point?

> 
> > +}
> > +
> >  static void virgl_cmd_resource_unref(VirtIOGPU *g,
> >                                       struct virtio_gpu_ctrl_command *cmd)
> >  {
> > +    struct virtio_gpu_simple_resource *res;
> >      struct virtio_gpu_resource_unref unref;
> >      struct iovec *res_iovs = NULL;
> >      int num_iovs = 0;
> > @@ -88,13 +104,22 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
> >      VIRTIO_GPU_FILL_CMD(unref);
> >      trace_virtio_gpu_cmd_res_unref(unref.resource_id);
> >
> > +    res = virtio_gpu_find_resource(g, unref.resource_id);
> > +
> >      virgl_renderer_resource_detach_iov(unref.resource_id,
> >                                         &res_iovs,
> >                                         &num_iovs);
> >      if (res_iovs != NULL && num_iovs != 0) {
> >          virtio_gpu_cleanup_mapping_iov(g, res_iovs, num_iovs);
> > +        if (res) {
> > +            res->iov = NULL;
> > +            res->iov_cnt = 0;
> > +        }
> >      }
> > +
> >      virgl_renderer_resource_unref(unref.resource_id);
> > +
> > +    virgl_resource_destroy(g, res);
> >  }
> >
> >  static void virgl_cmd_context_create(VirtIOGPU *g,
> > @@ -426,6 +451,183 @@ static void virgl_cmd_get_capset(VirtIOGPU *g,
> >      g_free(resp);
> >  }
> >
> > +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> > +
> > +static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
> > +                                           struct virtio_gpu_ctrl_command *cmd)
> > +{
> > +    struct virtio_gpu_simple_resource *res;
> > +    struct virtio_gpu_resource_create_blob cblob;
> > +    struct virgl_renderer_resource_create_blob_args virgl_args = { 0 };
> > +    int ret;
> > +
> > +    VIRTIO_GPU_FILL_CMD(cblob);
> > +    virtio_gpu_create_blob_bswap(&cblob);
> > +    trace_virtio_gpu_cmd_res_create_blob(cblob.resource_id, cblob.size);
> > +
> > +    if (cblob.resource_id == 0) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n",
> > +                      __func__);
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > +        return;
> > +    }
> > +
> > +    res = virtio_gpu_find_resource(g, cblob.resource_id);
> > +    if (res) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
> > +                      __func__, cblob.resource_id);
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > +        return;
> > +    }
> > +
> > +    res = g_new0(struct virtio_gpu_simple_resource, 1);
> > +    if (!res) {
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> > +        return;
> 
> needless error handling, glib will abort() on OOM.

OK, I will remove all simliar error handling in next version.

> 
> > +    }
> > +
> > +    res->resource_id = cblob.resource_id;
> > +    res->blob_size = cblob.size;
> > +
> > +    if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) {
> > +        ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
> > +                                            cmd, &res->addrs, &res->iov,
> > +                                            &res->iov_cnt);
> > +        if (!ret) {
> > +            g_free(res);
> > +            cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> > +            return;
> > +        }
> > +    }
> > +
> > +    QTAILQ_INSERT_HEAD(&g->reslist, res, next);
> > +
> > +    virgl_args.res_handle = cblob.resource_id;
> > +    virgl_args.ctx_id = cblob.hdr.ctx_id;
> > +    virgl_args.blob_mem = cblob.blob_mem;
> > +    virgl_args.blob_id = cblob.blob_id;
> > +    virgl_args.blob_flags = cblob.blob_flags;
> > +    virgl_args.size = cblob.size;
> > +    virgl_args.iovecs = res->iov;
> > +    virgl_args.num_iovs = res->iov_cnt;
> > +
> > +    ret = virgl_renderer_resource_create_blob(&virgl_args);
> > +    if (ret) {
> > +        virgl_resource_destroy(g, res);
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: virgl blob create error: %s\n",
> > +                      __func__, strerror(-ret));
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> > +    }
> > +}
> > +
> > +static void virgl_cmd_resource_map_blob(VirtIOGPU *g,
> > +                                        struct virtio_gpu_ctrl_command *cmd)
> > +{
> > +    struct virtio_gpu_simple_resource *res;
> > +    struct virtio_gpu_resource_map_blob mblob;
> > +    int ret;
> > +    void *data;
> > +    uint64_t size;
> > +    struct virtio_gpu_resp_map_info resp;
> > +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> > +
> > +    VIRTIO_GPU_FILL_CMD(mblob);
> > +    virtio_gpu_map_blob_bswap(&mblob);
> > +
> > +    if (mblob.resource_id == 0) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n",
> > +                      __func__);
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > +        return;
> > +    }
> > +
> > +    res = virtio_gpu_find_resource(g, mblob.resource_id);
> > +    if (!res) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n",
> > +                      __func__, mblob.resource_id);
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > +        return;
> > +    }
> > +    if (res->region) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already mapped %d\n",
> > +                     __func__, mblob.resource_id);
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > +        return;
> > +    }
> > +
> > +    ret = virgl_renderer_resource_map(res->resource_id, &data, &size);
> > +    if (ret) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource map error: %s\n",
> > +                      __func__, strerror(-ret));
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > +        return;
> > +    }
> > +
> > +    res->region = g_new0(MemoryRegion, 1);
> > +    if (!res->region) {
> 
> no need for OOM handling here too
> 
> > +        virgl_renderer_resource_unmap(res->resource_id);
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> > +        return;
> > +    }
> > +    memory_region_init_ram_ptr(res->region, OBJECT(g), NULL, size, data);
> > +    OBJECT(res->region)->free = g_free;
> > +    memory_region_add_subregion(&b->hostmem, mblob.offset, res->region);
> > +    memory_region_set_enabled(res->region, true);
> > +
> > +    memset(&resp, 0, sizeof(resp));
> > +    resp.hdr.type = VIRTIO_GPU_RESP_OK_MAP_INFO;
> > +    virgl_renderer_resource_get_map_info(mblob.resource_id, &resp.map_info);
> > +    virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
> > +}
> > +
> > +static int virtio_gpu_virgl_resource_unmap(VirtIOGPU *g,
> > +                                           struct virtio_gpu_simple_resource
> > +                                           *res)
> > +{
> > +    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
> > +
> > +    if (!res) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already unmapped %d\n",
> > +                      __func__, res->resource_id);
> > +        return VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > +    }
> > +
> > +    memory_region_set_enabled(res->region, false);
> > +    memory_region_del_subregion(&b->hostmem, res->region);
> > +    object_unparent(OBJECT(res->region));
> > +    res->region = NULL;
> > +
> > +    return virgl_renderer_resource_unmap(res->resource_id);
> > +}
> > +
> > +static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g,
> > +                                          struct virtio_gpu_ctrl_command *cmd)
> > +{
> > +    struct virtio_gpu_simple_resource *res;
> > +    struct virtio_gpu_resource_unmap_blob ublob;
> > +    VIRTIO_GPU_FILL_CMD(ublob);
> > +    virtio_gpu_unmap_blob_bswap(&ublob);
> > +
> > +    if (ublob.resource_id == 0) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource id 0 is not allowed\n",
> > +                      __func__);
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > +        return;
> > +    }
> > +
> > +    res = virtio_gpu_find_resource(g, ublob.resource_id);
> > +    if (!res) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: resource does not exist %d\n",
> > +                      __func__, ublob.resource_id);
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> > +        return;
> > +    }
> > +
> > +    virtio_gpu_virgl_resource_unmap(g, res);
> > +}
> > +
> > +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
> > +
> >  void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> >                                        struct virtio_gpu_ctrl_command *cmd)
> >  {
> > @@ -492,6 +694,17 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> >      case VIRTIO_GPU_CMD_GET_EDID:
> >          virtio_gpu_get_edid(g, cmd);
> >          break;
> > +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> > +    case VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB:
> > +        virgl_cmd_resource_create_blob(g, cmd);
> > +        break;
> > +    case VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB:
> > +        virgl_cmd_resource_map_blob(g, cmd);
> > +        break;
> > +    case VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB:
> > +        virgl_cmd_resource_unmap_blob(g, cmd);
> > +        break;
> > +#endif /* HAVE_VIRGL_RESOURCE_BLOB */
> >      default:
> >          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> >          break;
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index 5b7a7eab4f..cc4c1f81bb 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -1367,10 +1367,12 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
> >              return;
> >          }
> >
> > +#ifndef HAVE_VIRGL_RESOURCE_BLOB
> >          if (virtio_gpu_virgl_enabled(g->parent_obj.conf)) {
> > -            error_setg(errp, "blobs and virgl are not compatible (yet)");
> > +            error_setg(errp, "Linked virglrenderer does not support blob resources");
> >              return;
> >          }
> > +#endif
> >      }
> >
> >      if (!virtio_gpu_base_device_realize(qdev,
> > diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> > index 55973e112f..b9adc28071 100644
> > --- a/include/hw/virtio/virtio-gpu.h
> > +++ b/include/hw/virtio/virtio-gpu.h
> > @@ -58,6 +58,11 @@ struct virtio_gpu_simple_resource {
> >      int dmabuf_fd;
> >      uint8_t *remapped;
> >
> > +#ifdef HAVE_VIRGL_RESOURCE_BLOB
> > +    /* only blob resource needs this region to be mapped as guest mmio */
> > +    MemoryRegion *region;
> > +#endif
> > +
> >      QTAILQ_ENTRY(virtio_gpu_simple_resource) next;
> >  };
> >
> > diff --git a/meson.build b/meson.build
> > index ff20d3c249..f7b744ab82 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -1072,6 +1072,10 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
> >                         cc.has_function('virgl_renderer_context_create_with_flags',
> >                                         prefix: '#include <virglrenderer.h>',
> >                                         dependencies: virgl))
> > +  config_host_data.set('HAVE_VIRGL_RESOURCE_BLOB',
> > +                       cc.has_function('virgl_renderer_resource_create_blob',
> > +                                       prefix: '#include <virglrenderer.h>',
> > +                                       dependencies: virgl))
> 
> better moved under the if virgl.found() block

Yes, will update.

> 
> >  endif
> >  blkio = not_found
> >  if not get_option('blkio').auto() or have_block
> > --
> > 2.34.1
> >
> >
> 
> 
> -- 
> Marc-André Lureau

[QEMU PATCH v5 10/13] virtio-gpu: Resource UUID

[Huang Rui]

From: Antonio Caggiano <antonio.caggiano@collabora.com>

Enable resource UUID feature and implement command resource assign UUID.
This is done by introducing a hash table to map resource IDs to their
UUIDs.

Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

V4 -> V5:
    - Add virtio migration handling for uuid (Akihiko)
    - Adjust sequence to allocate gpu resource before virglrender resource
      creation (Akihiko)
    - Clean up (Akihiko)

 hw/display/trace-events        |  1 +
 hw/display/virtio-gpu-base.c   |  2 ++
 hw/display/virtio-gpu-virgl.c  | 21 ++++++++++++
 hw/display/virtio-gpu.c        | 58 ++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio-gpu.h |  6 ++++
 5 files changed, 88 insertions(+)

diff --git a/hw/display/trace-events b/hw/display/trace-events
index 2336a0ca15..54d6894c59 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -41,6 +41,7 @@ virtio_gpu_cmd_res_create_blob(uint32_t res, uint64_t size) "res 0x%x, size %" P
 virtio_gpu_cmd_res_unref(uint32_t res) "res 0x%x"
 virtio_gpu_cmd_res_back_attach(uint32_t res) "res 0x%x"
 virtio_gpu_cmd_res_back_detach(uint32_t res) "res 0x%x"
+virtio_gpu_cmd_res_assign_uuid(uint32_t res) "res 0x%x"
 virtio_gpu_cmd_res_xfer_toh_2d(uint32_t res) "res 0x%x"
 virtio_gpu_cmd_res_xfer_toh_3d(uint32_t res) "res 0x%x"
 virtio_gpu_cmd_res_xfer_fromh_3d(uint32_t res) "res 0x%x"
diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index 4f2b0ba1f3..f44388715c 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -236,6 +236,8 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
         features |= (1 << VIRTIO_GPU_F_CONTEXT_INIT);
     }
 
+    features |= (1 << VIRTIO_GPU_F_RESOURCE_UUID);
+
     return features;
 }
 
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 563a6f2f58..8a017dbeb4 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -36,11 +36,20 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
 {
     struct virtio_gpu_resource_create_2d c2d;
     struct virgl_renderer_resource_create_args args;
+    struct virtio_gpu_simple_resource *res;
 
     VIRTIO_GPU_FILL_CMD(c2d);
     trace_virtio_gpu_cmd_res_create_2d(c2d.resource_id, c2d.format,
                                        c2d.width, c2d.height);
 
+    res = g_new0(struct virtio_gpu_simple_resource, 1);
+    if (!res) {
+        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
+        return;
+    }
+    res->resource_id = c2d.resource_id;
+    QTAILQ_INSERT_HEAD(&g->reslist, res, next);
+
     args.handle = c2d.resource_id;
     args.target = 2;
     args.format = c2d.format;
@@ -60,11 +69,20 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
 {
     struct virtio_gpu_resource_create_3d c3d;
     struct virgl_renderer_resource_create_args args;
+    struct virtio_gpu_simple_resource *res;
 
     VIRTIO_GPU_FILL_CMD(c3d);
     trace_virtio_gpu_cmd_res_create_3d(c3d.resource_id, c3d.format,
                                        c3d.width, c3d.height, c3d.depth);
 
+    res = g_new0(struct virtio_gpu_simple_resource, 1);
+    if (!res) {
+        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
+        return;
+    }
+    res->resource_id = c3d.resource_id;
+    QTAILQ_INSERT_HEAD(&g->reslist, res, next);
+
     args.handle = c3d.resource_id;
     args.target = c3d.target;
     args.format = c3d.format;
@@ -682,6 +700,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
         /* TODO add security */
         virgl_cmd_ctx_detach_resource(g, cmd);
         break;
+    case VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID:
+        virtio_gpu_resource_assign_uuid(g, cmd);
+        break;
     case VIRTIO_GPU_CMD_GET_CAPSET_INFO:
         virgl_cmd_get_capset_info(g, cmd);
         break;
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index cc4c1f81bb..44414c1c5e 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -966,6 +966,38 @@ virtio_gpu_resource_detach_backing(VirtIOGPU *g,
     virtio_gpu_cleanup_mapping(g, res);
 }
 
+void virtio_gpu_resource_assign_uuid(VirtIOGPU *g,
+                                     struct virtio_gpu_ctrl_command *cmd)
+{
+    struct virtio_gpu_simple_resource *res;
+    struct virtio_gpu_resource_assign_uuid assign;
+    struct virtio_gpu_resp_resource_uuid resp;
+    QemuUUID *uuid;
+
+    VIRTIO_GPU_FILL_CMD(assign);
+    virtio_gpu_bswap_32(&assign, sizeof(assign));
+    trace_virtio_gpu_cmd_res_assign_uuid(assign.resource_id);
+
+    res = virtio_gpu_find_check_resource(g, assign.resource_id, false, __func__, &cmd->error);
+    if (!res) {
+        return;
+    }
+
+    memset(&resp, 0, sizeof(resp));
+    resp.hdr.type = VIRTIO_GPU_RESP_OK_RESOURCE_UUID;
+
+    uuid = g_hash_table_lookup(g->resource_uuids, GUINT_TO_POINTER(assign.resource_id));
+    if (!uuid) {
+        uuid = g_new(QemuUUID, 1);
+        qemu_uuid_generate(uuid);
+        g_hash_table_insert(g->resource_uuids, GUINT_TO_POINTER(assign.resource_id), uuid);
+        res->has_uuid = true;
+    }
+
+    memcpy(resp.uuid, uuid, sizeof(QemuUUID));
+    virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
+}
+
 void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
                                    struct virtio_gpu_ctrl_command *cmd)
 {
@@ -1014,6 +1046,9 @@ void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
     case VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING:
         virtio_gpu_resource_detach_backing(g, cmd);
         break;
+    case VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID:
+        virtio_gpu_resource_assign_uuid(g, cmd);
+        break;
     default:
         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
         break;
@@ -1208,6 +1243,7 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size,
     VirtIOGPU *g = opaque;
     struct virtio_gpu_simple_resource *res;
     int i;
+    QemuUUID *uuid;
 
     /* in 2d mode we should never find unprocessed commands here */
     assert(QTAILQ_EMPTY(&g->cmdq));
@@ -1224,9 +1260,17 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size,
         }
         qemu_put_buffer(f, (void *)pixman_image_get_data(res->image),
                         pixman_image_get_stride(res->image) * res->height);
+
+        qemu_put_byte(f, res->has_uuid);
+        if (res->has_uuid) {
+            uuid = g_hash_table_lookup(g->resource_uuids, GUINT_TO_POINTER(res->resource_id));
+            qemu_put_buffer(f, (void *)uuid, sizeof(QemuUUID));
+        }
     }
     qemu_put_be32(f, 0); /* end of list */
 
+    g_hash_table_destroy(g->resource_uuids);
+
     return vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL);
 }
 
@@ -1239,9 +1283,12 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
     uint32_t resource_id, pformat;
     void *bits = NULL;
     int i;
+    QemuUUID *uuid = NULL;
 
     g->hostmem = 0;
 
+    g->resource_uuids = g_hash_table_new_full(NULL, NULL, NULL, g_free);
+
     resource_id = qemu_get_be32(f);
     while (resource_id != 0) {
         res = virtio_gpu_find_resource(g, resource_id);
@@ -1292,6 +1339,12 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
         qemu_get_buffer(f, (void *)pixman_image_get_data(res->image),
                         pixman_image_get_stride(res->image) * res->height);
 
+        res->has_uuid = qemu_get_byte(f);
+        if (res->has_uuid) {
+            qemu_get_buffer(f, (void *)uuid, sizeof(QemuUUID));
+            g_hash_table_insert(g->resource_uuids, GUINT_TO_POINTER(res->resource_id), uuid);
+        }
+
         /* restore mapping */
         for (i = 0; i < res->iov_cnt; i++) {
             hwaddr len = res->iov[i].iov_len;
@@ -1393,12 +1446,15 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
     QTAILQ_INIT(&g->reslist);
     QTAILQ_INIT(&g->cmdq);
     QTAILQ_INIT(&g->fenceq);
+
+    g->resource_uuids = g_hash_table_new_full(NULL, NULL, NULL, g_free);
 }
 
 static void virtio_gpu_device_unrealize(DeviceState *qdev)
 {
     VirtIOGPU *g = VIRTIO_GPU(qdev);
 
+    g_hash_table_destroy(g->resource_uuids);
     g_clear_pointer(&g->ctrl_bh, qemu_bh_delete);
     g_clear_pointer(&g->cursor_bh, qemu_bh_delete);
     g_clear_pointer(&g->reset_bh, qemu_bh_delete);
@@ -1452,6 +1508,8 @@ void virtio_gpu_reset(VirtIODevice *vdev)
         g_free(cmd);
     }
 
+    g_hash_table_remove_all(g->resource_uuids);
+
     virtio_gpu_base_reset(VIRTIO_GPU_BASE(vdev));
 }
 
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index b9adc28071..67b39fccec 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -63,6 +63,8 @@ struct virtio_gpu_simple_resource {
     MemoryRegion *region;
 #endif
 
+    bool has_uuid;
+
     QTAILQ_ENTRY(virtio_gpu_simple_resource) next;
 };
 
@@ -208,6 +210,8 @@ struct VirtIOGPU {
         QTAILQ_HEAD(, VGPUDMABuf) bufs;
         VGPUDMABuf *primary[VIRTIO_GPU_MAX_SCANOUTS];
     } dmabuf;
+
+    GHashTable *resource_uuids;
 };
 
 struct VirtIOGPUClass {
@@ -285,6 +289,8 @@ void virtio_gpu_cleanup_mapping_iov(VirtIOGPU *g,
                                     struct iovec *iov, uint32_t count);
 void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
                                 struct virtio_gpu_simple_resource *res);
+void virtio_gpu_resource_assign_uuid(VirtIOGPU *g,
+                                     struct virtio_gpu_ctrl_command *cmd);
 void virtio_gpu_process_cmdq(VirtIOGPU *g);
 void virtio_gpu_device_realize(DeviceState *qdev, Error **errp);
 void virtio_gpu_reset(VirtIODevice *vdev);
-- 
2.34.1

Re: [QEMU PATCH v5 10/13] virtio-gpu: Resource UUID

[Akihiko Odaki]

On 2023/09/15 20:11, Huang Rui wrote:
> From: Antonio Caggiano <antonio.caggiano@collabora.com>
> 
> Enable resource UUID feature and implement command resource assign UUID.
> This is done by introducing a hash table to map resource IDs to their
> UUIDs.
> 
> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
> 
> V4 -> V5:
>      - Add virtio migration handling for uuid (Akihiko)
>      - Adjust sequence to allocate gpu resource before virglrender resource
>        creation (Akihiko)
>      - Clean up (Akihiko)
> 
>   hw/display/trace-events        |  1 +
>   hw/display/virtio-gpu-base.c   |  2 ++
>   hw/display/virtio-gpu-virgl.c  | 21 ++++++++++++
>   hw/display/virtio-gpu.c        | 58 ++++++++++++++++++++++++++++++++++
>   include/hw/virtio/virtio-gpu.h |  6 ++++
>   5 files changed, 88 insertions(+)
> 
> diff --git a/hw/display/trace-events b/hw/display/trace-events
> index 2336a0ca15..54d6894c59 100644
> --- a/hw/display/trace-events
> +++ b/hw/display/trace-events
> @@ -41,6 +41,7 @@ virtio_gpu_cmd_res_create_blob(uint32_t res, uint64_t size) "res 0x%x, size %" P
>   virtio_gpu_cmd_res_unref(uint32_t res) "res 0x%x"
>   virtio_gpu_cmd_res_back_attach(uint32_t res) "res 0x%x"
>   virtio_gpu_cmd_res_back_detach(uint32_t res) "res 0x%x"
> +virtio_gpu_cmd_res_assign_uuid(uint32_t res) "res 0x%x"
>   virtio_gpu_cmd_res_xfer_toh_2d(uint32_t res) "res 0x%x"
>   virtio_gpu_cmd_res_xfer_toh_3d(uint32_t res) "res 0x%x"
>   virtio_gpu_cmd_res_xfer_fromh_3d(uint32_t res) "res 0x%x"
> diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
> index 4f2b0ba1f3..f44388715c 100644
> --- a/hw/display/virtio-gpu-base.c
> +++ b/hw/display/virtio-gpu-base.c
> @@ -236,6 +236,8 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
>           features |= (1 << VIRTIO_GPU_F_CONTEXT_INIT);
>       }
>   
> +    features |= (1 << VIRTIO_GPU_F_RESOURCE_UUID);
> +
>       return features;
>   }
>   
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 563a6f2f58..8a017dbeb4 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -36,11 +36,20 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
>   {
>       struct virtio_gpu_resource_create_2d c2d;
>       struct virgl_renderer_resource_create_args args;
> +    struct virtio_gpu_simple_resource *res;
>   
>       VIRTIO_GPU_FILL_CMD(c2d);
>       trace_virtio_gpu_cmd_res_create_2d(c2d.resource_id, c2d.format,
>                                          c2d.width, c2d.height);
>   
> +    res = g_new0(struct virtio_gpu_simple_resource, 1);
> +    if (!res) {
> +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> +        return;
> +    }
> +    res->resource_id = c2d.resource_id;
> +    QTAILQ_INSERT_HEAD(&g->reslist, res, next);
> +

The struct virtio_gpu_simple_resource for a resource created with 
virgl_cmd_create_resource_2d() and virgl_resource_attach_backing() will 
not have iov and iov_cnt set, which is inconsistent with 
virgl_cmd_resource_create_blob().

>       args.handle = c2d.resource_id;
>       args.target = 2;
>       args.format = c2d.format;
> @@ -60,11 +69,20 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
>   {
>       struct virtio_gpu_resource_create_3d c3d;
>       struct virgl_renderer_resource_create_args args;
> +    struct virtio_gpu_simple_resource *res;
>   
>       VIRTIO_GPU_FILL_CMD(c3d);
>       trace_virtio_gpu_cmd_res_create_3d(c3d.resource_id, c3d.format,
>                                          c3d.width, c3d.height, c3d.depth);
>   
> +    res = g_new0(struct virtio_gpu_simple_resource, 1);
> +    if (!res) {
> +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> +        return;
> +    }
> +    res->resource_id = c3d.resource_id;
> +    QTAILQ_INSERT_HEAD(&g->reslist, res, next);
> +
>       args.handle = c3d.resource_id;
>       args.target = c3d.target;
>       args.format = c3d.format;
> @@ -682,6 +700,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>           /* TODO add security */
>           virgl_cmd_ctx_detach_resource(g, cmd);
>           break;
> +    case VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID:
> +        virtio_gpu_resource_assign_uuid(g, cmd);
> +        break;
>       case VIRTIO_GPU_CMD_GET_CAPSET_INFO:
>           virgl_cmd_get_capset_info(g, cmd);
>           break;
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index cc4c1f81bb..44414c1c5e 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -966,6 +966,38 @@ virtio_gpu_resource_detach_backing(VirtIOGPU *g,
>       virtio_gpu_cleanup_mapping(g, res);
>   }
>   
> +void virtio_gpu_resource_assign_uuid(VirtIOGPU *g,
> +                                     struct virtio_gpu_ctrl_command *cmd)
> +{
> +    struct virtio_gpu_simple_resource *res;
> +    struct virtio_gpu_resource_assign_uuid assign;
> +    struct virtio_gpu_resp_resource_uuid resp;
> +    QemuUUID *uuid;
> +
> +    VIRTIO_GPU_FILL_CMD(assign);
> +    virtio_gpu_bswap_32(&assign, sizeof(assign));
> +    trace_virtio_gpu_cmd_res_assign_uuid(assign.resource_id);
> +
> +    res = virtio_gpu_find_check_resource(g, assign.resource_id, false, __func__, &cmd->error);
> +    if (!res) {
> +        return;
> +    }
> +
> +    memset(&resp, 0, sizeof(resp));
> +    resp.hdr.type = VIRTIO_GPU_RESP_OK_RESOURCE_UUID;
> +
> +    uuid = g_hash_table_lookup(g->resource_uuids, GUINT_TO_POINTER(assign.resource_id));
> +    if (!uuid) {
> +        uuid = g_new(QemuUUID, 1);
> +        qemu_uuid_generate(uuid);
> +        g_hash_table_insert(g->resource_uuids, GUINT_TO_POINTER(assign.resource_id), uuid);
> +        res->has_uuid = true;
> +    }

What about embedding uuid into struct virtio_gpu_simple_resource? You 
will not need to maintain another table in this way.

Re: [QEMU PATCH v5 10/13] virtio-gpu: Resource UUID

[Huang Rui]

On Sat, Sep 16, 2023 at 12:48:14AM +0800, Akihiko Odaki wrote:
> On 2023/09/15 20:11, Huang Rui wrote:
> > From: Antonio Caggiano <antonio.caggiano@collabora.com>
> > 
> > Enable resource UUID feature and implement command resource assign UUID.
> > This is done by introducing a hash table to map resource IDs to their
> > UUIDs.
> > 
> > Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> > 
> > V4 -> V5:
> >      - Add virtio migration handling for uuid (Akihiko)
> >      - Adjust sequence to allocate gpu resource before virglrender resource
> >        creation (Akihiko)
> >      - Clean up (Akihiko)
> > 
> >   hw/display/trace-events        |  1 +
> >   hw/display/virtio-gpu-base.c   |  2 ++
> >   hw/display/virtio-gpu-virgl.c  | 21 ++++++++++++
> >   hw/display/virtio-gpu.c        | 58 ++++++++++++++++++++++++++++++++++
> >   include/hw/virtio/virtio-gpu.h |  6 ++++
> >   5 files changed, 88 insertions(+)
> > 
> > diff --git a/hw/display/trace-events b/hw/display/trace-events
> > index 2336a0ca15..54d6894c59 100644
> > --- a/hw/display/trace-events
> > +++ b/hw/display/trace-events
> > @@ -41,6 +41,7 @@ virtio_gpu_cmd_res_create_blob(uint32_t res, uint64_t size) "res 0x%x, size %" P
> >   virtio_gpu_cmd_res_unref(uint32_t res) "res 0x%x"
> >   virtio_gpu_cmd_res_back_attach(uint32_t res) "res 0x%x"
> >   virtio_gpu_cmd_res_back_detach(uint32_t res) "res 0x%x"
> > +virtio_gpu_cmd_res_assign_uuid(uint32_t res) "res 0x%x"
> >   virtio_gpu_cmd_res_xfer_toh_2d(uint32_t res) "res 0x%x"
> >   virtio_gpu_cmd_res_xfer_toh_3d(uint32_t res) "res 0x%x"
> >   virtio_gpu_cmd_res_xfer_fromh_3d(uint32_t res) "res 0x%x"
> > diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
> > index 4f2b0ba1f3..f44388715c 100644
> > --- a/hw/display/virtio-gpu-base.c
> > +++ b/hw/display/virtio-gpu-base.c
> > @@ -236,6 +236,8 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
> >           features |= (1 << VIRTIO_GPU_F_CONTEXT_INIT);
> >       }
> >   
> > +    features |= (1 << VIRTIO_GPU_F_RESOURCE_UUID);
> > +
> >       return features;
> >   }
> >   
> > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> > index 563a6f2f58..8a017dbeb4 100644
> > --- a/hw/display/virtio-gpu-virgl.c
> > +++ b/hw/display/virtio-gpu-virgl.c
> > @@ -36,11 +36,20 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
> >   {
> >       struct virtio_gpu_resource_create_2d c2d;
> >       struct virgl_renderer_resource_create_args args;
> > +    struct virtio_gpu_simple_resource *res;
> >   
> >       VIRTIO_GPU_FILL_CMD(c2d);
> >       trace_virtio_gpu_cmd_res_create_2d(c2d.resource_id, c2d.format,
> >                                          c2d.width, c2d.height);
> >   
> > +    res = g_new0(struct virtio_gpu_simple_resource, 1);
> > +    if (!res) {
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> > +        return;
> > +    }
> > +    res->resource_id = c2d.resource_id;
> > +    QTAILQ_INSERT_HEAD(&g->reslist, res, next);
> > +
> 
> The struct virtio_gpu_simple_resource for a resource created with 
> virgl_cmd_create_resource_2d() and virgl_resource_attach_backing() will 
> not have iov and iov_cnt set, which is inconsistent with 
> virgl_cmd_resource_create_blob().
> 

OK, so we should find out the resource in virgl_resource_attach_backing(),
and set the related iov and iov_cnt for this resource to align with blob
memory.

> >       args.handle = c2d.resource_id;
> >       args.target = 2;
> >       args.format = c2d.format;
> > @@ -60,11 +69,20 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
> >   {
> >       struct virtio_gpu_resource_create_3d c3d;
> >       struct virgl_renderer_resource_create_args args;
> > +    struct virtio_gpu_simple_resource *res;
> >   
> >       VIRTIO_GPU_FILL_CMD(c3d);
> >       trace_virtio_gpu_cmd_res_create_3d(c3d.resource_id, c3d.format,
> >                                          c3d.width, c3d.height, c3d.depth);
> >   
> > +    res = g_new0(struct virtio_gpu_simple_resource, 1);
> > +    if (!res) {
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> > +        return;
> > +    }
> > +    res->resource_id = c3d.resource_id;
> > +    QTAILQ_INSERT_HEAD(&g->reslist, res, next);
> > +
> >       args.handle = c3d.resource_id;
> >       args.target = c3d.target;
> >       args.format = c3d.format;
> > @@ -682,6 +700,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> >           /* TODO add security */
> >           virgl_cmd_ctx_detach_resource(g, cmd);
> >           break;
> > +    case VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID:
> > +        virtio_gpu_resource_assign_uuid(g, cmd);
> > +        break;
> >       case VIRTIO_GPU_CMD_GET_CAPSET_INFO:
> >           virgl_cmd_get_capset_info(g, cmd);
> >           break;
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index cc4c1f81bb..44414c1c5e 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -966,6 +966,38 @@ virtio_gpu_resource_detach_backing(VirtIOGPU *g,
> >       virtio_gpu_cleanup_mapping(g, res);
> >   }
> >   
> > +void virtio_gpu_resource_assign_uuid(VirtIOGPU *g,
> > +                                     struct virtio_gpu_ctrl_command *cmd)
> > +{
> > +    struct virtio_gpu_simple_resource *res;
> > +    struct virtio_gpu_resource_assign_uuid assign;
> > +    struct virtio_gpu_resp_resource_uuid resp;
> > +    QemuUUID *uuid;
> > +
> > +    VIRTIO_GPU_FILL_CMD(assign);
> > +    virtio_gpu_bswap_32(&assign, sizeof(assign));
> > +    trace_virtio_gpu_cmd_res_assign_uuid(assign.resource_id);
> > +
> > +    res = virtio_gpu_find_check_resource(g, assign.resource_id, false, __func__, &cmd->error);
> > +    if (!res) {
> > +        return;
> > +    }
> > +
> > +    memset(&resp, 0, sizeof(resp));
> > +    resp.hdr.type = VIRTIO_GPU_RESP_OK_RESOURCE_UUID;
> > +
> > +    uuid = g_hash_table_lookup(g->resource_uuids, GUINT_TO_POINTER(assign.resource_id));
> > +    if (!uuid) {
> > +        uuid = g_new(QemuUUID, 1);
> > +        qemu_uuid_generate(uuid);
> > +        g_hash_table_insert(g->resource_uuids, GUINT_TO_POINTER(assign.resource_id), uuid);
> > +        res->has_uuid = true;
> > +    }
> 
> What about embedding uuid into struct virtio_gpu_simple_resource? You 
> will not need to maintain another table in this way.

Yes, agree. Will update it in V6.

Thanks,
Ray

Re: [QEMU PATCH v5 10/13] virtio-gpu: Resource UUID

[Marc-André Lureau]

Hi

On Fri, Sep 15, 2023 at 3:14 PM Huang Rui <ray.huang@amd.com> wrote:
>
> From: Antonio Caggiano <antonio.caggiano@collabora.com>
>
> Enable resource UUID feature and implement command resource assign UUID.
> This is done by introducing a hash table to map resource IDs to their
> UUIDs.
>
> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
>
> V4 -> V5:
>     - Add virtio migration handling for uuid (Akihiko)
>     - Adjust sequence to allocate gpu resource before virglrender resource
>       creation (Akihiko)
>     - Clean up (Akihiko)
>
>  hw/display/trace-events        |  1 +
>  hw/display/virtio-gpu-base.c   |  2 ++
>  hw/display/virtio-gpu-virgl.c  | 21 ++++++++++++
>  hw/display/virtio-gpu.c        | 58 ++++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio-gpu.h |  6 ++++
>  5 files changed, 88 insertions(+)
>
> diff --git a/hw/display/trace-events b/hw/display/trace-events
> index 2336a0ca15..54d6894c59 100644
> --- a/hw/display/trace-events
> +++ b/hw/display/trace-events
> @@ -41,6 +41,7 @@ virtio_gpu_cmd_res_create_blob(uint32_t res, uint64_t size) "res 0x%x, size %" P
>  virtio_gpu_cmd_res_unref(uint32_t res) "res 0x%x"
>  virtio_gpu_cmd_res_back_attach(uint32_t res) "res 0x%x"
>  virtio_gpu_cmd_res_back_detach(uint32_t res) "res 0x%x"
> +virtio_gpu_cmd_res_assign_uuid(uint32_t res) "res 0x%x"
>  virtio_gpu_cmd_res_xfer_toh_2d(uint32_t res) "res 0x%x"
>  virtio_gpu_cmd_res_xfer_toh_3d(uint32_t res) "res 0x%x"
>  virtio_gpu_cmd_res_xfer_fromh_3d(uint32_t res) "res 0x%x"
> diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
> index 4f2b0ba1f3..f44388715c 100644
> --- a/hw/display/virtio-gpu-base.c
> +++ b/hw/display/virtio-gpu-base.c
> @@ -236,6 +236,8 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
>          features |= (1 << VIRTIO_GPU_F_CONTEXT_INIT);
>      }
>
> +    features |= (1 << VIRTIO_GPU_F_RESOURCE_UUID);

This changes what is exposed to the guest. We should have an option
for it, and set it to false on older machine types (similar to what is
done for edid).

> +
>      return features;
>  }
>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 563a6f2f58..8a017dbeb4 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -36,11 +36,20 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
>  {
>      struct virtio_gpu_resource_create_2d c2d;
>      struct virgl_renderer_resource_create_args args;
> +    struct virtio_gpu_simple_resource *res;
>
>      VIRTIO_GPU_FILL_CMD(c2d);
>      trace_virtio_gpu_cmd_res_create_2d(c2d.resource_id, c2d.format,
>                                         c2d.width, c2d.height);
>
> +    res = g_new0(struct virtio_gpu_simple_resource, 1);
> +    if (!res) {

needless OOM here too

> +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> +        return;
> +    }
> +    res->resource_id = c2d.resource_id;
> +    QTAILQ_INSERT_HEAD(&g->reslist, res, next);
> +
>      args.handle = c2d.resource_id;
>      args.target = 2;
>      args.format = c2d.format;
> @@ -60,11 +69,20 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
>  {
>      struct virtio_gpu_resource_create_3d c3d;
>      struct virgl_renderer_resource_create_args args;
> +    struct virtio_gpu_simple_resource *res;
>
>      VIRTIO_GPU_FILL_CMD(c3d);
>      trace_virtio_gpu_cmd_res_create_3d(c3d.resource_id, c3d.format,
>                                         c3d.width, c3d.height, c3d.depth);
>
> +    res = g_new0(struct virtio_gpu_simple_resource, 1);
> +    if (!res) {

same

> +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> +        return;
> +    }
> +    res->resource_id = c3d.resource_id;
> +    QTAILQ_INSERT_HEAD(&g->reslist, res, next);
> +
>      args.handle = c3d.resource_id;
>      args.target = c3d.target;
>      args.format = c3d.format;
> @@ -682,6 +700,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>          /* TODO add security */
>          virgl_cmd_ctx_detach_resource(g, cmd);
>          break;
> +    case VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID:
> +        virtio_gpu_resource_assign_uuid(g, cmd);
> +        break;
>      case VIRTIO_GPU_CMD_GET_CAPSET_INFO:
>          virgl_cmd_get_capset_info(g, cmd);
>          break;
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index cc4c1f81bb..44414c1c5e 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -966,6 +966,38 @@ virtio_gpu_resource_detach_backing(VirtIOGPU *g,
>      virtio_gpu_cleanup_mapping(g, res);
>  }
>
> +void virtio_gpu_resource_assign_uuid(VirtIOGPU *g,
> +                                     struct virtio_gpu_ctrl_command *cmd)
> +{
> +    struct virtio_gpu_simple_resource *res;
> +    struct virtio_gpu_resource_assign_uuid assign;
> +    struct virtio_gpu_resp_resource_uuid resp;
> +    QemuUUID *uuid;
> +
> +    VIRTIO_GPU_FILL_CMD(assign);
> +    virtio_gpu_bswap_32(&assign, sizeof(assign));
> +    trace_virtio_gpu_cmd_res_assign_uuid(assign.resource_id);
> +
> +    res = virtio_gpu_find_check_resource(g, assign.resource_id, false, __func__, &cmd->error);
> +    if (!res) {
> +        return;
> +    }
> +
> +    memset(&resp, 0, sizeof(resp));
> +    resp.hdr.type = VIRTIO_GPU_RESP_OK_RESOURCE_UUID;
> +
> +    uuid = g_hash_table_lookup(g->resource_uuids, GUINT_TO_POINTER(assign.resource_id));
> +    if (!uuid) {
> +        uuid = g_new(QemuUUID, 1);
> +        qemu_uuid_generate(uuid);
> +        g_hash_table_insert(g->resource_uuids, GUINT_TO_POINTER(assign.resource_id), uuid);
> +        res->has_uuid = true;
> +    }
> +
> +    memcpy(resp.uuid, uuid, sizeof(QemuUUID));
> +    virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
> +}
> +
>  void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
>                                     struct virtio_gpu_ctrl_command *cmd)
>  {
> @@ -1014,6 +1046,9 @@ void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
>      case VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING:
>          virtio_gpu_resource_detach_backing(g, cmd);
>          break;
> +    case VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID:
> +        virtio_gpu_resource_assign_uuid(g, cmd);
> +        break;
>      default:
>          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>          break;
> @@ -1208,6 +1243,7 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size,
>      VirtIOGPU *g = opaque;
>      struct virtio_gpu_simple_resource *res;
>      int i;
> +    QemuUUID *uuid;
>
>      /* in 2d mode we should never find unprocessed commands here */
>      assert(QTAILQ_EMPTY(&g->cmdq));
> @@ -1224,9 +1260,17 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size,
>          }
>          qemu_put_buffer(f, (void *)pixman_image_get_data(res->image),
>                          pixman_image_get_stride(res->image) * res->height);
> +
> +        qemu_put_byte(f, res->has_uuid);
> +        if (res->has_uuid) {
> +            uuid = g_hash_table_lookup(g->resource_uuids, GUINT_TO_POINTER(res->resource_id));
> +            qemu_put_buffer(f, (void *)uuid, sizeof(QemuUUID));
> +        }

This is breaking the migration format. It will need to be handled with
an optional subsection instead.

>      }
>      qemu_put_be32(f, 0); /* end of list */
>
> +    g_hash_table_destroy(g->resource_uuids);
> +
>      return vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL);
>  }
>
> @@ -1239,9 +1283,12 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
>      uint32_t resource_id, pformat;
>      void *bits = NULL;
>      int i;
> +    QemuUUID *uuid = NULL;
>
>      g->hostmem = 0;
>
> +    g->resource_uuids = g_hash_table_new_full(NULL, NULL, NULL, g_free);

Since it maps int -> uuid, it should probably use g_int_hash() /
g_int_qual() instead of the default (g_direct_hash()).

> +
>      resource_id = qemu_get_be32(f);
>      while (resource_id != 0) {
>          res = virtio_gpu_find_resource(g, resource_id);
> @@ -1292,6 +1339,12 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
>          qemu_get_buffer(f, (void *)pixman_image_get_data(res->image),
>                          pixman_image_get_stride(res->image) * res->height);
>
> +        res->has_uuid = qemu_get_byte(f);
> +        if (res->has_uuid) {
> +            qemu_get_buffer(f, (void *)uuid, sizeof(QemuUUID));
> +            g_hash_table_insert(g->resource_uuids, GUINT_TO_POINTER(res->resource_id), uuid);
> +        }
> +
>          /* restore mapping */
>          for (i = 0; i < res->iov_cnt; i++) {
>              hwaddr len = res->iov[i].iov_len;
> @@ -1393,12 +1446,15 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
>      QTAILQ_INIT(&g->reslist);
>      QTAILQ_INIT(&g->cmdq);
>      QTAILQ_INIT(&g->fenceq);
> +
> +    g->resource_uuids = g_hash_table_new_full(NULL, NULL, NULL, g_free);
>  }
>
>  static void virtio_gpu_device_unrealize(DeviceState *qdev)
>  {
>      VirtIOGPU *g = VIRTIO_GPU(qdev);
>
> +    g_hash_table_destroy(g->resource_uuids);
>      g_clear_pointer(&g->ctrl_bh, qemu_bh_delete);
>      g_clear_pointer(&g->cursor_bh, qemu_bh_delete);
>      g_clear_pointer(&g->reset_bh, qemu_bh_delete);
> @@ -1452,6 +1508,8 @@ void virtio_gpu_reset(VirtIODevice *vdev)
>          g_free(cmd);
>      }
>
> +    g_hash_table_remove_all(g->resource_uuids);
> +
>      virtio_gpu_base_reset(VIRTIO_GPU_BASE(vdev));
>  }
>
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index b9adc28071..67b39fccec 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -63,6 +63,8 @@ struct virtio_gpu_simple_resource {
>      MemoryRegion *region;
>  #endif
>
> +    bool has_uuid;
> +
>      QTAILQ_ENTRY(virtio_gpu_simple_resource) next;
>  };
>
> @@ -208,6 +210,8 @@ struct VirtIOGPU {
>          QTAILQ_HEAD(, VGPUDMABuf) bufs;
>          VGPUDMABuf *primary[VIRTIO_GPU_MAX_SCANOUTS];
>      } dmabuf;
> +
> +    GHashTable *resource_uuids;
>  };
>
>  struct VirtIOGPUClass {
> @@ -285,6 +289,8 @@ void virtio_gpu_cleanup_mapping_iov(VirtIOGPU *g,
>                                      struct iovec *iov, uint32_t count);
>  void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
>                                  struct virtio_gpu_simple_resource *res);
> +void virtio_gpu_resource_assign_uuid(VirtIOGPU *g,
> +                                     struct virtio_gpu_ctrl_command *cmd);
>  void virtio_gpu_process_cmdq(VirtIOGPU *g);
>  void virtio_gpu_device_realize(DeviceState *qdev, Error **errp);
>  void virtio_gpu_reset(VirtIODevice *vdev);
> --
> 2.34.1
>
>


-- 
Marc-André Lureau

Re: [QEMU PATCH v5 10/13] virtio-gpu: Resource UUID

[Huang Rui]

On Tue, Sep 19, 2023 at 05:00:05PM +0800, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Sep 15, 2023 at 3:14 PM Huang Rui <ray.huang@amd.com> wrote:
> >
> > From: Antonio Caggiano <antonio.caggiano@collabora.com>
> >
> > Enable resource UUID feature and implement command resource assign UUID.
> > This is done by introducing a hash table to map resource IDs to their
> > UUIDs.
> >
> > Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> >
> > V4 -> V5:
> >     - Add virtio migration handling for uuid (Akihiko)
> >     - Adjust sequence to allocate gpu resource before virglrender resource
> >       creation (Akihiko)
> >     - Clean up (Akihiko)
> >
> >  hw/display/trace-events        |  1 +
> >  hw/display/virtio-gpu-base.c   |  2 ++
> >  hw/display/virtio-gpu-virgl.c  | 21 ++++++++++++
> >  hw/display/virtio-gpu.c        | 58 ++++++++++++++++++++++++++++++++++
> >  include/hw/virtio/virtio-gpu.h |  6 ++++
> >  5 files changed, 88 insertions(+)
> >
> > diff --git a/hw/display/trace-events b/hw/display/trace-events
> > index 2336a0ca15..54d6894c59 100644
> > --- a/hw/display/trace-events
> > +++ b/hw/display/trace-events
> > @@ -41,6 +41,7 @@ virtio_gpu_cmd_res_create_blob(uint32_t res, uint64_t size) "res 0x%x, size %" P
> >  virtio_gpu_cmd_res_unref(uint32_t res) "res 0x%x"
> >  virtio_gpu_cmd_res_back_attach(uint32_t res) "res 0x%x"
> >  virtio_gpu_cmd_res_back_detach(uint32_t res) "res 0x%x"
> > +virtio_gpu_cmd_res_assign_uuid(uint32_t res) "res 0x%x"
> >  virtio_gpu_cmd_res_xfer_toh_2d(uint32_t res) "res 0x%x"
> >  virtio_gpu_cmd_res_xfer_toh_3d(uint32_t res) "res 0x%x"
> >  virtio_gpu_cmd_res_xfer_fromh_3d(uint32_t res) "res 0x%x"
> > diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
> > index 4f2b0ba1f3..f44388715c 100644
> > --- a/hw/display/virtio-gpu-base.c
> > +++ b/hw/display/virtio-gpu-base.c
> > @@ -236,6 +236,8 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, uint64_t features,
> >          features |= (1 << VIRTIO_GPU_F_CONTEXT_INIT);
> >      }
> >
> > +    features |= (1 << VIRTIO_GPU_F_RESOURCE_UUID);
> 
> This changes what is exposed to the guest. We should have an option
> for it, and set it to false on older machine types (similar to what is
> done for edid).
> 

Did you mean we can add "resource_uuid" in virtio_gpu_properties[] like
below, user can set it to false on older types with the param?

DEFINE_PROP_BIT("resource_uuid", VirtIOGPU, parent_obj.conf.flags,
                VIRTIO_GPU_FLAG_RESOURCE_UUID, true);

> > +
> >      return features;
> >  }
> >
> > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> > index 563a6f2f58..8a017dbeb4 100644
> > --- a/hw/display/virtio-gpu-virgl.c
> > +++ b/hw/display/virtio-gpu-virgl.c
> > @@ -36,11 +36,20 @@ static void virgl_cmd_create_resource_2d(VirtIOGPU *g,
> >  {
> >      struct virtio_gpu_resource_create_2d c2d;
> >      struct virgl_renderer_resource_create_args args;
> > +    struct virtio_gpu_simple_resource *res;
> >
> >      VIRTIO_GPU_FILL_CMD(c2d);
> >      trace_virtio_gpu_cmd_res_create_2d(c2d.resource_id, c2d.format,
> >                                         c2d.width, c2d.height);
> >
> > +    res = g_new0(struct virtio_gpu_simple_resource, 1);
> > +    if (!res) {
> 
> needless OOM here too
> 
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> > +        return;
> > +    }
> > +    res->resource_id = c2d.resource_id;
> > +    QTAILQ_INSERT_HEAD(&g->reslist, res, next);
> > +
> >      args.handle = c2d.resource_id;
> >      args.target = 2;
> >      args.format = c2d.format;
> > @@ -60,11 +69,20 @@ static void virgl_cmd_create_resource_3d(VirtIOGPU *g,
> >  {
> >      struct virtio_gpu_resource_create_3d c3d;
> >      struct virgl_renderer_resource_create_args args;
> > +    struct virtio_gpu_simple_resource *res;
> >
> >      VIRTIO_GPU_FILL_CMD(c3d);
> >      trace_virtio_gpu_cmd_res_create_3d(c3d.resource_id, c3d.format,
> >                                         c3d.width, c3d.height, c3d.depth);
> >
> > +    res = g_new0(struct virtio_gpu_simple_resource, 1);
> > +    if (!res) {
> 
> same
> 
> > +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> > +        return;
> > +    }
> > +    res->resource_id = c3d.resource_id;
> > +    QTAILQ_INSERT_HEAD(&g->reslist, res, next);
> > +
> >      args.handle = c3d.resource_id;
> >      args.target = c3d.target;
> >      args.format = c3d.format;
> > @@ -682,6 +700,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
> >          /* TODO add security */
> >          virgl_cmd_ctx_detach_resource(g, cmd);
> >          break;
> > +    case VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID:
> > +        virtio_gpu_resource_assign_uuid(g, cmd);
> > +        break;
> >      case VIRTIO_GPU_CMD_GET_CAPSET_INFO:
> >          virgl_cmd_get_capset_info(g, cmd);
> >          break;
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index cc4c1f81bb..44414c1c5e 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -966,6 +966,38 @@ virtio_gpu_resource_detach_backing(VirtIOGPU *g,
> >      virtio_gpu_cleanup_mapping(g, res);
> >  }
> >
> > +void virtio_gpu_resource_assign_uuid(VirtIOGPU *g,
> > +                                     struct virtio_gpu_ctrl_command *cmd)
> > +{
> > +    struct virtio_gpu_simple_resource *res;
> > +    struct virtio_gpu_resource_assign_uuid assign;
> > +    struct virtio_gpu_resp_resource_uuid resp;
> > +    QemuUUID *uuid;
> > +
> > +    VIRTIO_GPU_FILL_CMD(assign);
> > +    virtio_gpu_bswap_32(&assign, sizeof(assign));
> > +    trace_virtio_gpu_cmd_res_assign_uuid(assign.resource_id);
> > +
> > +    res = virtio_gpu_find_check_resource(g, assign.resource_id, false, __func__, &cmd->error);
> > +    if (!res) {
> > +        return;
> > +    }
> > +
> > +    memset(&resp, 0, sizeof(resp));
> > +    resp.hdr.type = VIRTIO_GPU_RESP_OK_RESOURCE_UUID;
> > +
> > +    uuid = g_hash_table_lookup(g->resource_uuids, GUINT_TO_POINTER(assign.resource_id));
> > +    if (!uuid) {
> > +        uuid = g_new(QemuUUID, 1);
> > +        qemu_uuid_generate(uuid);
> > +        g_hash_table_insert(g->resource_uuids, GUINT_TO_POINTER(assign.resource_id), uuid);
> > +        res->has_uuid = true;
> > +    }
> > +
> > +    memcpy(resp.uuid, uuid, sizeof(QemuUUID));
> > +    virtio_gpu_ctrl_response(g, cmd, &resp.hdr, sizeof(resp));
> > +}
> > +
> >  void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
> >                                     struct virtio_gpu_ctrl_command *cmd)
> >  {
> > @@ -1014,6 +1046,9 @@ void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
> >      case VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING:
> >          virtio_gpu_resource_detach_backing(g, cmd);
> >          break;
> > +    case VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID:
> > +        virtio_gpu_resource_assign_uuid(g, cmd);
> > +        break;
> >      default:
> >          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> >          break;
> > @@ -1208,6 +1243,7 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size,
> >      VirtIOGPU *g = opaque;
> >      struct virtio_gpu_simple_resource *res;
> >      int i;
> > +    QemuUUID *uuid;
> >
> >      /* in 2d mode we should never find unprocessed commands here */
> >      assert(QTAILQ_EMPTY(&g->cmdq));
> > @@ -1224,9 +1260,17 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size,
> >          }
> >          qemu_put_buffer(f, (void *)pixman_image_get_data(res->image),
> >                          pixman_image_get_stride(res->image) * res->height);
> > +
> > +        qemu_put_byte(f, res->has_uuid);
> > +        if (res->has_uuid) {
> > +            uuid = g_hash_table_lookup(g->resource_uuids, GUINT_TO_POINTER(res->resource_id));
> > +            qemu_put_buffer(f, (void *)uuid, sizeof(QemuUUID));
> > +        }
> 
> This is breaking the migration format. It will need to be handled with
> an optional subsection instead.

I am using one byte to store res->has_uuid after pixman data, if check this
field is true, then have UUID data.

Would you mind to elaborate the optional subsection?

> 
> >      }
> >      qemu_put_be32(f, 0); /* end of list */
> >
> > +    g_hash_table_destroy(g->resource_uuids);
> > +
> >      return vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL);
> >  }
> >
> > @@ -1239,9 +1283,12 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
> >      uint32_t resource_id, pformat;
> >      void *bits = NULL;
> >      int i;
> > +    QemuUUID *uuid = NULL;
> >
> >      g->hostmem = 0;
> >
> > +    g->resource_uuids = g_hash_table_new_full(NULL, NULL, NULL, g_free);
> 
> Since it maps int -> uuid, it should probably use g_int_hash() /
> g_int_qual() instead of the default (g_direct_hash()).
> 

Thanks to reminder, will update it in V6.

> > +
> >      resource_id = qemu_get_be32(f);
> >      while (resource_id != 0) {
> >          res = virtio_gpu_find_resource(g, resource_id);
> > @@ -1292,6 +1339,12 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
> >          qemu_get_buffer(f, (void *)pixman_image_get_data(res->image),
> >                          pixman_image_get_stride(res->image) * res->height);
> >
> > +        res->has_uuid = qemu_get_byte(f);
> > +        if (res->has_uuid) {
> > +            qemu_get_buffer(f, (void *)uuid, sizeof(QemuUUID));
> > +            g_hash_table_insert(g->resource_uuids, GUINT_TO_POINTER(res->resource_id), uuid);
> > +        }
> > +
> >          /* restore mapping */
> >          for (i = 0; i < res->iov_cnt; i++) {
> >              hwaddr len = res->iov[i].iov_len;
> > @@ -1393,12 +1446,15 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
> >      QTAILQ_INIT(&g->reslist);
> >      QTAILQ_INIT(&g->cmdq);
> >      QTAILQ_INIT(&g->fenceq);
> > +
> > +    g->resource_uuids = g_hash_table_new_full(NULL, NULL, NULL, g_free);
> >  }
> >
> >  static void virtio_gpu_device_unrealize(DeviceState *qdev)
> >  {
> >      VirtIOGPU *g = VIRTIO_GPU(qdev);
> >
> > +    g_hash_table_destroy(g->resource_uuids);
> >      g_clear_pointer(&g->ctrl_bh, qemu_bh_delete);
> >      g_clear_pointer(&g->cursor_bh, qemu_bh_delete);
> >      g_clear_pointer(&g->reset_bh, qemu_bh_delete);
> > @@ -1452,6 +1508,8 @@ void virtio_gpu_reset(VirtIODevice *vdev)
> >          g_free(cmd);
> >      }
> >
> > +    g_hash_table_remove_all(g->resource_uuids);
> > +
> >      virtio_gpu_base_reset(VIRTIO_GPU_BASE(vdev));
> >  }
> >
> > diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> > index b9adc28071..67b39fccec 100644
> > --- a/include/hw/virtio/virtio-gpu.h
> > +++ b/include/hw/virtio/virtio-gpu.h
> > @@ -63,6 +63,8 @@ struct virtio_gpu_simple_resource {
> >      MemoryRegion *region;
> >  #endif
> >
> > +    bool has_uuid;
> > +
> >      QTAILQ_ENTRY(virtio_gpu_simple_resource) next;
> >  };
> >
> > @@ -208,6 +210,8 @@ struct VirtIOGPU {
> >          QTAILQ_HEAD(, VGPUDMABuf) bufs;
> >          VGPUDMABuf *primary[VIRTIO_GPU_MAX_SCANOUTS];
> >      } dmabuf;
> > +
> > +    GHashTable *resource_uuids;
> >  };
> >
> >  struct VirtIOGPUClass {
> > @@ -285,6 +289,8 @@ void virtio_gpu_cleanup_mapping_iov(VirtIOGPU *g,
> >                                      struct iovec *iov, uint32_t count);
> >  void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
> >                                  struct virtio_gpu_simple_resource *res);
> > +void virtio_gpu_resource_assign_uuid(VirtIOGPU *g,
> > +                                     struct virtio_gpu_ctrl_command *cmd);
> >  void virtio_gpu_process_cmdq(VirtIOGPU *g);
> >  void virtio_gpu_device_realize(DeviceState *qdev, Error **errp);
> >  void virtio_gpu_reset(VirtIODevice *vdev);
> > --
> > 2.34.1
> >
> >
> 
> 
> -- 
> Marc-André Lureau

[QEMU PATCH v5 11/13] virtio-gpu: Support Venus capset

[Huang Rui]

From: Antonio Caggiano <antonio.caggiano@collabora.com>

Add support for the Venus capset, which enables Vulkan support through
the Venus Vulkan driver for virtio-gpu.

Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

V4 -> V5:
    - Send kernel patch to define VIRTIO_GPU_CAPSET_VENUS and will use
      another patch to sync up linux headers. (Akihiko)
    - https://lore.kernel.org/lkml/20230915105918.3763061-1-ray.huang@amd.com/

 hw/display/virtio-gpu-virgl.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 8a017dbeb4..7f95490e90 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -437,6 +437,11 @@ static void virgl_cmd_get_capset_info(VirtIOGPU *g,
         virgl_renderer_get_cap_set(resp.capset_id,
                                    &resp.capset_max_version,
                                    &resp.capset_max_size);
+    } else if (info.capset_index == 2) {
+        resp.capset_id = VIRTIO_GPU_CAPSET_VENUS;
+        virgl_renderer_get_cap_set(resp.capset_id,
+                                   &resp.capset_max_version,
+                                   &resp.capset_max_size);
     } else {
         resp.capset_max_version = 0;
         resp.capset_max_size = 0;
@@ -901,10 +906,18 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
 
 int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
 {
-    uint32_t capset2_max_ver, capset2_max_size;
+    uint32_t capset2_max_ver, capset2_max_size, num_capsets;
+    num_capsets = 1;
+
     virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VIRGL2,
-                              &capset2_max_ver,
-                              &capset2_max_size);
+                               &capset2_max_ver,
+                               &capset2_max_size);
+    num_capsets += capset2_max_ver ? 1 : 0;
+
+    virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VENUS,
+                               &capset2_max_ver,
+                               &capset2_max_size);
+    num_capsets += capset2_max_size ? 1 : 0;
 
-    return capset2_max_ver ? 2 : 1;
+    return num_capsets;
 }
-- 
2.34.1

Re: [QEMU PATCH v5 11/13] virtio-gpu: Support Venus capset

[Marc-André Lureau]

Hi

On Fri, Sep 15, 2023 at 3:14 PM Huang Rui <ray.huang@amd.com> wrote:
>
> From: Antonio Caggiano <antonio.caggiano@collabora.com>
>
> Add support for the Venus capset, which enables Vulkan support through
> the Venus Vulkan driver for virtio-gpu.
>
> Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
>
> V4 -> V5:
>     - Send kernel patch to define VIRTIO_GPU_CAPSET_VENUS and will use
>       another patch to sync up linux headers. (Akihiko)
>     - https://lore.kernel.org/lkml/20230915105918.3763061-1-ray.huang@amd.com/

Ok but in the meantime, you should have that header update patch in
the series too, otherwise we can't compile it :)

thanks

>
>  hw/display/virtio-gpu-virgl.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 8a017dbeb4..7f95490e90 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -437,6 +437,11 @@ static void virgl_cmd_get_capset_info(VirtIOGPU *g,
>          virgl_renderer_get_cap_set(resp.capset_id,
>                                     &resp.capset_max_version,
>                                     &resp.capset_max_size);
> +    } else if (info.capset_index == 2) {
> +        resp.capset_id = VIRTIO_GPU_CAPSET_VENUS;
> +        virgl_renderer_get_cap_set(resp.capset_id,
> +                                   &resp.capset_max_version,
> +                                   &resp.capset_max_size);
>      } else {
>          resp.capset_max_version = 0;
>          resp.capset_max_size = 0;
> @@ -901,10 +906,18 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
>
>  int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
>  {
> -    uint32_t capset2_max_ver, capset2_max_size;
> +    uint32_t capset2_max_ver, capset2_max_size, num_capsets;
> +    num_capsets = 1;
> +
>      virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VIRGL2,
> -                              &capset2_max_ver,
> -                              &capset2_max_size);
> +                               &capset2_max_ver,
> +                               &capset2_max_size);
> +    num_capsets += capset2_max_ver ? 1 : 0;
> +
> +    virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VENUS,
> +                               &capset2_max_ver,
> +                               &capset2_max_size);
> +    num_capsets += capset2_max_size ? 1 : 0;
>
> -    return capset2_max_ver ? 2 : 1;
> +    return num_capsets;
>  }
> --
> 2.34.1
>
>


-- 
Marc-André Lureau

Re: [QEMU PATCH v5 11/13] virtio-gpu: Support Venus capset

[Huang Rui]

On Tue, Sep 19, 2023 at 05:02:36PM +0800, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Sep 15, 2023 at 3:14 PM Huang Rui <ray.huang@amd.com> wrote:
> >
> > From: Antonio Caggiano <antonio.caggiano@collabora.com>
> >
> > Add support for the Venus capset, which enables Vulkan support through
> > the Venus Vulkan driver for virtio-gpu.
> >
> > Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> >
> > V4 -> V5:
> >     - Send kernel patch to define VIRTIO_GPU_CAPSET_VENUS and will use
> >       another patch to sync up linux headers. (Akihiko)
> >     - https://lore.kernel.org/lkml/20230915105918.3763061-1-ray.huang@amd.com/
> 
> Ok but in the meantime, you should have that header update patch in
> the series too, otherwise we can't compile it :)
> 

In fact, it's in my repo. :-)
I am waiting for it to be merged into kernel mainline and then update the
commit id.

https://gitlab.freedesktop.org/rui/qemu-xen/-/commit/4ae9d078f9242890769c98162caf32f95df42529

I will include it in next series.

Thanks,
Ray

> thanks
> 
> >
> >  hw/display/virtio-gpu-virgl.c | 21 +++++++++++++++++----
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> > index 8a017dbeb4..7f95490e90 100644
> > --- a/hw/display/virtio-gpu-virgl.c
> > +++ b/hw/display/virtio-gpu-virgl.c
> > @@ -437,6 +437,11 @@ static void virgl_cmd_get_capset_info(VirtIOGPU *g,
> >          virgl_renderer_get_cap_set(resp.capset_id,
> >                                     &resp.capset_max_version,
> >                                     &resp.capset_max_size);
> > +    } else if (info.capset_index == 2) {
> > +        resp.capset_id = VIRTIO_GPU_CAPSET_VENUS;
> > +        virgl_renderer_get_cap_set(resp.capset_id,
> > +                                   &resp.capset_max_version,
> > +                                   &resp.capset_max_size);
> >      } else {
> >          resp.capset_max_version = 0;
> >          resp.capset_max_size = 0;
> > @@ -901,10 +906,18 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
> >
> >  int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g)
> >  {
> > -    uint32_t capset2_max_ver, capset2_max_size;
> > +    uint32_t capset2_max_ver, capset2_max_size, num_capsets;
> > +    num_capsets = 1;
> > +
> >      virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VIRGL2,
> > -                              &capset2_max_ver,
> > -                              &capset2_max_size);
> > +                               &capset2_max_ver,
> > +                               &capset2_max_size);
> > +    num_capsets += capset2_max_ver ? 1 : 0;
> > +
> > +    virgl_renderer_get_cap_set(VIRTIO_GPU_CAPSET_VENUS,
> > +                               &capset2_max_ver,
> > +                               &capset2_max_size);
> > +    num_capsets += capset2_max_size ? 1 : 0;
> >
> > -    return capset2_max_ver ? 2 : 1;
> > +    return num_capsets;
> >  }
> > --
> > 2.34.1
> >
> >
> 
> 
> -- 
> Marc-André Lureau

[QEMU PATCH v5 12/13] virtio-gpu: Initialize Venus

[Huang Rui]

From: Antonio Caggiano <antonio.caggiano@collabora.com>

Request Venus when initializing VirGL.

Signed-off-by: Antonio Caggiano <antonio.caggiano@collabora.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
---

V4 -> V5:
    - Add meson check to make sure unstable APIs defined from 0.9.0. (Antonio)

 hw/display/virtio-gpu-virgl.c | 4 ++++
 meson.build                   | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 7f95490e90..39c04d730c 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -887,6 +887,10 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
     }
 #endif
 
+#ifdef VIRGL_RENDERER_VENUS
+    flags |= VIRGL_RENDERER_VENUS;
+#endif
+
     ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs);
     if (ret != 0) {
         error_report("virgl could not be initialized: %d", ret);
diff --git a/meson.build b/meson.build
index f7b744ab82..e4004d05b1 100644
--- a/meson.build
+++ b/meson.build
@@ -1076,6 +1076,11 @@ if not get_option('virglrenderer').auto() or have_system or have_vhost_user_gpu
                        cc.has_function('virgl_renderer_resource_create_blob',
                                        prefix: '#include <virglrenderer.h>',
                                        dependencies: virgl))
+  if virgl.version().version_compare('>= 0.9.0') and virgl.version().version_compare('< 1.0.0')
+    message('Enabling virglrenderer unstable APIs')
+    virgl = declare_dependency(compile_args: '-DVIRGL_RENDERER_UNSTABLE_APIS',
+                               dependencies: virgl)
+  endif
 endif
 blkio = not_found
 if not get_option('blkio').auto() or have_block
-- 
2.34.1

Re: [QEMU PATCH v5 12/13] virtio-gpu: Initialize Venus

[Dmitry Osipenko]

On 9/15/23 14:11, Huang Rui wrote:
> +  if virgl.version().version_compare('>= 0.9.0') and virgl.version().version_compare('< 1.0.0')
> +    message('Enabling virglrenderer unstable APIs')
> +    virgl = declare_dependency(compile_args: '-DVIRGL_RENDERER_UNSTABLE_APIS',
> +                               dependencies: virgl)
> +  endif

All the required APIs has been stabilized in virglrenderer 1.0, we
shouldn't use the VIRGL_RENDERER_UNSTABLE_APIS anymore. The unstable
APIs were used for development purposes and shouldn't present in the
final version.

Please remove the VIRGL_RENDERER_UNSTABLE_APIS in v6.

-- 
Best regards,
Dmitry

[QEMU PATCH v5 13/13] virtio-gpu: Enable virglrenderer render server flag for venus

[Huang Rui]

Venus in virglrenderer has required render server support.

Signed-off-by: Huang Rui <ray.huang@amd.com>
---
 hw/display/virtio-gpu-virgl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 39c04d730c..65ffce85a8 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -888,7 +888,7 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
 #endif
 
 #ifdef VIRGL_RENDERER_VENUS
-    flags |= VIRGL_RENDERER_VENUS;
+    flags |= VIRGL_RENDERER_VENUS | VIRGL_RENDERER_RENDER_SERVER;
 #endif
 
     ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs);
-- 
2.34.1

Re: [QEMU PATCH v5 13/13] virtio-gpu: Enable virglrenderer render server flag for venus

[Dmitry Osipenko]

On 9/15/23 14:11, Huang Rui wrote:
> Venus in virglrenderer has required render server support.
> 
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
>  hw/display/virtio-gpu-virgl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 39c04d730c..65ffce85a8 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -888,7 +888,7 @@ int virtio_gpu_virgl_init(VirtIOGPU *g)
>  #endif
>  
>  #ifdef VIRGL_RENDERER_VENUS
> -    flags |= VIRGL_RENDERER_VENUS;
> +    flags |= VIRGL_RENDERER_VENUS | VIRGL_RENDERER_RENDER_SERVER;
>  #endif
>  
>      ret = virgl_renderer_init(g, flags, &virtio_gpu_3d_cbs);

This change should be squashed into the previous "Initialize Venus" patch.

-- 
Best regards,
Dmitry

Re: [QEMU PATCH v5 00/13] Support blob memory and venus on qemu

[Dmitry Osipenko]

On 9/15/23 14:11, Huang Rui wrote:
> Hi all,
> 
> Antonio Caggiano made the venus with QEMU on KVM platform last
> September[1]. This series are inherited from his original work to support
> the features of context init, hostmem, resource uuid, and blob resources
> for venus.
> At March of this year, we sent out the V1 version[2] for the review. But
> those series are included both xen and virtio gpu. Right now, we would like
> to divide into two parts, one is to continue the Antonio's work to upstream
> virtio-gpu support for blob memory and venus, and another is to upstream
> xen specific patches. This series is focusing on virtio-gpu, so we are
> marking as V4 version here to continue Antonio's patches[1]. And we will
> send xen specific patches separately, because they are hypervisor specific.
> Besides of QEMU, these supports also included virglrenderer[3][4] and
> mesa[5][6] as well. Right now, virglrenderer and mesa parts are all
> accepted by upstream. In this qemu version, we try to address the concerns
> around not proper cleanup during blob resource unmap and unref. Appreciate
> it if you have any commments.

Venus uses blob buffers. In order to display Venus/VK application using
GL display server, the VIRTIO_GPU_CMD_SET_SCANOUT_BLOB must be
supported. Otherwise Venus itself isn't practically useful because you
can't run VK application with a usual Linux DE because Venus will error
out due to the unsupported command.

The VIRTIO_GPU_CMD_SET_SCANOUT_BLOB support needs to be a part of this
series, IMO. Can we add it to the next patchset version?

-- 
Best regards,
Dmitry