| 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
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: