Re: [PATCH 5/8] hw/display: re-arrange memory region tracking

[Peter Xu <peterx@redhat.com>]

On Tue, Oct 14, 2025 at 12:12:31PM +0100, 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>
> Message-Id: <20250410122643.1747913-2-manos.pitsidianakis@linaro.org>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-ID: <20250603110204.838117-10-alex.bennee@linaro.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 3bd5ffa5e0d..3349a5185a0 100644
> --- a/include/system/memory.h
> +++ b/include/system/memory.h
> @@ -843,6 +843,7 @@ struct MemoryRegion {
>      DeviceState *dev;
>  
>      const MemoryRegionOps *ops;
> +    /* opaque data, used by backends like @ops */

IMHO this comment isn't very helpful..  Maybe it should provide a list of
things that are using the opaque, in this case memory_region_init_ram_ptr
is the new addition.

Having memory_region_init() directly taking opaque as a parameter would be
nice but it might be an overkill indeed..  Still, there's option to at
least start to have opaque passed into memory_region_init_ram_ptr() so
people when reading memory.c will also know when opaque is used.  Otherwise
it can easily break virtio-gpu if someone decides to reuse opaque for the
API, without noticing it's used in virtio-gpu (like the ->free use..).

No strong feelings, but if anyone agrees it could be at least something to
be considered to be worked on top.

Thanks,

>      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 07f6355ad62..0ef0b2743fe 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;
>  
>      trace_virtio_gpu_cmd_res_map_blob(res->base.resource_id, vmr, mr);
> @@ -144,16 +138,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;
>  
>      trace_virtio_gpu_cmd_res_unmap_blob(res->base.resource_id, mr, vmr->finish_unmapping);
>  
> -- 
> 2.47.3
> 

-- 
Peter Xu