Philippe Mathieu-Daudé <phi...@linaro.org> writes:
> On 10/4/25 14:26, Manos Pitsidianakis wrote:
>> From: Alex Bennée <alex.ben...@linaro.org>
>> 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.
>
> It is unclear to me what layer is supposed to set/consume it. So far
> in memory_region_init_io/ram/rom it is kind of internal to the memory
> layer, but MemoryRegionOps aren't. Yeah well, OK then.
>
> Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org>
>
>> 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: Alex Bennée <alex.ben...@linaro.org>
>> Signed-off-by: Manos Pitsidianakis <manos.pitsidiana...@linaro.org>
>> ---
>> include/exec/memory.h | 1 +
>> hw/display/virtio-gpu-virgl.c | 23 ++++++++---------------
>> 2 files changed, 9 insertions(+), 15 deletions(-)
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index d09af58c97..bb735a3c7e 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/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;
>
> Same same but different? ;)
Hmm I think I just reflexively put unassigned variables at the bottom of
the list of declarations so they stand out more.
>
>> int ret;
>> if (!mr) {
>> return 0;
>> }
>> -
>> - vmr = to_hostmem_region(res->mr);
>> + vmr = mr->opaque;
>> /*
>> * Perform async unmapping in 3 steps:
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
