Re: [PULL 09/17] hw/display: re-arrange memory region tracking

Thu, 05 Jun 2025 22:19:20 -0700 

On 2025/06/06 1:26, Alex Bennée wrote:

QOM objects can be embedded in other QOM objects and managed as part
of their lifetime but this isn't the case for
virtio_gpu_virgl_hostmem_region. However before we can split it out we
need some other way of associating the wider data structure with the
memory region.

Fortunately MemoryRegion has an opaque pointer. This is passed down to
MemoryRegionOps for device type regions but is unused in the
memory_region_init_ram_ptr() case. Use the opaque to carry the
reference and allow the final MemoryRegion object to be reaped when
its reference count is cleared.

Signed-off-by: Manos Pitsidianakis <manos.pitsidiana...@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org>
Message-Id: <20250410122643.1747913-2-manos.pitsidiana...@linaro.org>
Cc: qemu-sta...@nongnu.org
Signed-off-by: Alex Bennée <alex.ben...@linaro.org>
Message-ID: <20250603110204.838117-10-alex.ben...@linaro.org>
I have told you that you should address all comments before sending a series again a few times[1][2], but you haven't done that. 

For this patch, I raised a concern with [3]:
I pointed out it has no effect (fixing or improving something) other than adding a memory allocation, but you didn't make a reply to prove otherwise.
I also pointed out it leaks memory and you asked for a test case[4], but you made this pull request without giving me 24 hours to reply to it.
The situation of "[PULL 03/17] tests/tcg: make aarch64 boot.S handle different starting modes" is also similar. I added a comment about symbol naming and you gave a reasoning, but I didn't get time to review it either[5]. Besides, I also had a suggestion to make the code shorter for the past version, but it is also dismissed.
I also pointed out "[PULL 11/17] ui/gtk-gl-area: Remove extra draw call in refresh" has an undressed comment[2][7].
I would like to see improvements in how comments are addressed before a series is resent. 

Regards,
Akihiko Odaki
[1] https://lore.kernel.org/qemu-devel/e6af12bd-1c36-4e50-8bae-d8d80cad1...@daynix.com [2] https://lore.kernel.org/qemu-devel/e037e38c-dd8d-4f65-b2d5-2629be5f6...@daynix.com [3] https://lore.kernel.org/qemu-devel/1a86b86d-145a-44fc-9f87-2804767fb...@daynix.com/ 
[4] https://lore.kernel.org/qemu-devel/87o6v2764e....@draig.linaro.org/
[5] https://lore.kernel.org/qemu-devel/874iwu372j....@draig.linaro.org/
[6] https://lore.kernel.org/qemu-devel/7a76e746-9022-48cf-8216-775071e6d...@daynix.com/ [7] https://lore.kernel.org/qemu-devel/63911dcc-482b-45c5-9468-120ae3df6...@daynix.com/ 



diff --git a/include/system/memory.h b/include/system/memory.h
index fc35a0dcad..90715ff44a 100644
--- a/include/system/memory.h
+++ b/include/system/memory.h
@@ -784,6 +784,7 @@ struct MemoryRegion {
      DeviceState *dev;
const MemoryRegionOps *ops; 
+    /* opaque data, used by backends like @ops */
      void *opaque;
      MemoryRegion *container;
      int mapped_via_alias; /* Mapped via an alias, container might be NULL */
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 145a0b3879..71a7500de9 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -52,17 +52,11 @@ virgl_get_egl_display(G_GNUC_UNUSED void *cookie)
#if VIRGL_VERSION_MAJOR >= 1 
  struct virtio_gpu_virgl_hostmem_region {
-    MemoryRegion mr;
+    MemoryRegion *mr;
      struct VirtIOGPU *g;
      bool finish_unmapping;
  };
-static struct virtio_gpu_virgl_hostmem_region * 
-to_hostmem_region(MemoryRegion *mr)
-{
-    return container_of(mr, struct virtio_gpu_virgl_hostmem_region, mr);
-}
-
  static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque)
  {
      VirtIOGPU *g = opaque;
@@ -73,14 +67,12 @@ static void virtio_gpu_virgl_resume_cmdq_bh(void *opaque)
  static void virtio_gpu_virgl_hostmem_region_free(void *obj)
  {
      MemoryRegion *mr = MEMORY_REGION(obj);
-    struct virtio_gpu_virgl_hostmem_region *vmr;
+    struct virtio_gpu_virgl_hostmem_region *vmr = mr->opaque;
      VirtIOGPUBase *b;
      VirtIOGPUGL *gl;
- vmr = to_hostmem_region(mr); 
-    vmr->finish_unmapping = true;
-
      b = VIRTIO_GPU_BASE(vmr->g);
+    vmr->finish_unmapping = true;
      b->renderer_blocked--;
/* 
@@ -118,8 +110,8 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
vmr = g_new0(struct virtio_gpu_virgl_hostmem_region, 1); 
      vmr->g = g;
+    mr = g_new0(MemoryRegion, 1);
- mr = &vmr->mr; 
      memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
      memory_region_add_subregion(&b->hostmem, offset, mr);
      memory_region_set_enabled(mr, true);
@@ -131,7 +123,9 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
       * command processing until MR is fully unreferenced and freed.
       */
      OBJECT(mr)->free = virtio_gpu_virgl_hostmem_region_free;
+    mr->opaque = vmr;
+ vmr->mr = mr; 
      res->mr = mr;
return 0; 
@@ -142,16 +136,15 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
                                       struct virtio_gpu_virgl_resource *res,
                                       bool *cmd_suspended)
  {
-    struct virtio_gpu_virgl_hostmem_region *vmr;
      VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
      MemoryRegion *mr = res->mr;
+    struct virtio_gpu_virgl_hostmem_region *vmr;
      int ret;
if (!mr) { 
          return 0;
      }
-
-    vmr = to_hostmem_region(res->mr);
+    vmr = mr->opaque;
/* 
       * Perform async unmapping in 3 steps:

Reply via email to