| Message ID | 20250603110204.838117-10-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Maintainer updates for May (testing, plugins, virtio-gpu) - pre-PR | expand |
On 2025/06/03 20:01, 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.pitsidianakis@linaro.org> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Message-Id: <20250410122643.1747913-2-manos.pitsidianakis@linaro.org> > Cc: qemu-stable@nongnu.org > --- > include/system/memory.h | 1 + > hw/display/virtio-gpu-virgl.c | 23 ++++++++--------------- > 2 files changed, 9 insertions(+), 15 deletions(-) > > 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); This patch does nothing more than adding a separate allocation for MemoryRegion. Besides there is no corresponding g_free(). This patch can be simply dropped. Regards, Akihiko Odaki > > - 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:
Akihiko Odaki <akihiko.odaki@daynix.com> writes: > On 2025/06/03 20:01, 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.pitsidianakis@linaro.org> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> Message-Id: <20250410122643.1747913-2-manos.pitsidianakis@linaro.org> >> Cc: qemu-stable@nongnu.org >> --- >> include/system/memory.h | 1 + >> hw/display/virtio-gpu-virgl.c | 23 ++++++++--------------- >> 2 files changed, 9 insertions(+), 15 deletions(-) >> 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); > > This patch does nothing more than adding a separate allocation for > MemoryRegion. Besides there is no corresponding g_free(). This patch > can be simply dropped. As the patch says the MemoryRegion is now free'd when it is de-referenced. Do you have a test case showing it leaking?
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: