| 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?
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. Regards, Akihiko Odaki
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
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). Regards, Akihiko Odaki
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
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: