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

["Alex Bennée" <alex.bennee@linaro.org>]

Akihiko Odaki <akihiko.odaki@daynix.com> writes:

> On 2025/06/05 20:57, Alex Bennée wrote:
>> 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?
>
> "De-referenced" is confusing and sounds like pointer dereferencing.
>
> OBJECT(mr)->free, which has virtio_gpu_virgl_hostmem_region_free() as
> its value, will be called to free mr when the references of mr are
> removed. This patch however does not add a corresponding g_free() call
> to virtio_gpu_virgl_hostmem_region_free(), leaking mr.
>
> AddressSanitizer will catch the memory leak.

Example invocation?

I ran the AddressSantizier against all the virtio-gpu tests yesterday
and it did not complain.

>
> Regards,
> Akihiko Odaki

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro