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

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2025/06/07 0:02, Akihiko Odaki wrote:
> 
> 
> On 2025/06/06 19:16, Alex Bennée wrote:
>> 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.
> 
> The following command line triggered the memory leak. The image is a 
> clean Debian 12 installation. I booted the installation, and shut down 
> it by pressing the button on the booted GDM:
> 
> build/qemu-system-x86_64 -drive file=debian12.qcow2 -m 8G -smp 8 -device 
> virtio-vga-gl,blob=on,hostmem=1G -display egl-headless,gl=on -vnc :0 -M 
> q35,accel=kvm
> ==361968==WARNING: ASan doesn't fully support makecontext/swapcontext 
> functions and may produce false positives in some cases!
> ==361968==WARNING: ASan is ignoring requested __asan_handle_no_return: 
> stack type: default top: 0x7bf41d2b8380; bottom 0x7bf2e0f4c000; size: 
> 0x00013c36c380 (5305189248)
> False positive error reports may follow
> For details see https://github.com/google/sanitizers/issues/189
> 
> =================================================================
> ==361968==ERROR: LeakSanitizer: detected memory leaks
> 
> Direct leak of 816 byte(s) in 3 object(s) allocated from:
>      #0 0x7ff640f50a43 in calloc (/lib64/libasan.so.8+0xe6a43) (BuildId: 
> 6a82bb83b1f19d3f3a2118085acf79daa3b52371)
>      #1 0x7ff64077c901 in g_malloc0 (/lib64/libglib-2.0.so.0+0x48901) 
> (BuildId: 6827394d759bc44f207f57e7ab5f8e6b17e82c1c)
>      #2 0x557fa8080dc5 in virtio_gpu_virgl_map_resource_blob ../hw/ 
> display/virtio-gpu-virgl.c:113
>      #3 0x557fa8080dc5 in virgl_cmd_resource_map_blob ../hw/display/ 
> virtio-gpu-virgl.c:772
>      #4 0x557fa8080dc5 in virtio_gpu_virgl_process_cmd ../hw/display/ 
> virtio-gpu-virgl.c:952
> 
> SUMMARY: AddressSanitizer: 816 byte(s) leaked in 3 allocation(s).

The following command line also reproduced the issue:

LIBGL_ALWAYS_SOFTWARE=1 \
LSAN_OPTIONS=suppressions=<(echo leak:fontconfig) \
build/qemu-system-aarch64 -M virt,accel=kvm -cpu host -smp 8 -m 8G \
-device virtio-gpu-gl,blob=on,hostmem=256M -display gtk,gl=on \
-drive file=Fedora-Workstation-Live-42-1.1.aarch64.iso,format=raw \
-bios /usr/share/edk2/aarch64/QEMU_EFI-silent-pflash.raw \
-serial mon:stdio

Fedora's Live disk allows you to test without installation.

By the way, I tried TCG to reproduce the hang, but I couldn't. I'd 
appreciate if you tell:

- how to reproduce the issue
- whether the CPU usage saturates with 100 %
   (i.e., if the hang is a busy-loop or not).
- the stack traces of all the threads when the hang happens.

Hopefully they will provide more insights into the problem and your fix.

Regards,
Akihiko Odaki