[PATCH 0/8] virtio-gpu/next - misc fixes and MR handling

[PATCH 0/8] virtio-gpu/next - misc fixes and MR handling

[Alex Bennée]

I still have a number of patches that addressed a lock-up but were
never merged due to objections. However there is a bunch of discussion
around re-factoring the MemoryRegion code so I'm re-posting with an
additional functional test which demonstrates the lock-up is fixed.
Whatever the final solution for more cleanly handling the binding
between MemoryRegions and blobs it will at least need to pass the
test.

Blob memory issues aside I'll send a PR next week to merge the
per-head and ui/gtk-gl-area changes.

Discuss,

Alex.

Alex Bennée (4):
  system/memory: add memory_region_finalize tracepoint
  hw/display: add blob map/unmap trace events
  hw/display: re-arrange memory region tracking
  tests/functional: add GPU blob allocation test

Andrew Keesler (1):
  Support per-head resolutions with virtio-gpu

Dongwon Kim (1):
  ui/gtk-gl-area: Remove extra draw call in refresh

Manos Pitsidianakis (2):
  virtio-gpu: refactor async blob unmapping
  virtio-gpu: fix hang under TCG when unmapping blob

 qapi/virtio.json                          | 10 +++-
 include/system/memory.h                   |  1 +
 hw/display/virtio-gpu-base.c              | 10 ++++
 hw/display/virtio-gpu-virgl.c             | 62 +++++++++++--------
 system/memory.c                           |  5 ++
 ui/gtk-gl-area.c                          |  1 -
 hw/display/trace-events                   |  2 +
 system/trace-events                       |  1 +
 tests/functional/aarch64/meson.build      |  1 +
 tests/functional/aarch64/test_gpu_blob.py | 73 +++++++++++++++++++++++
 10 files changed, 138 insertions(+), 28 deletions(-)
 create mode 100755 tests/functional/aarch64/test_gpu_blob.py

-- 
2.47.3

[PATCH 1/8] Support per-head resolutions with virtio-gpu

[Alex Bennée]

From: Andrew Keesler <ankeesler@google.com>

In 454f4b0f, we started down the path of supporting separate
configurations per display head (e.g., you have 2 heads - one with
EDID name "AAA" and the other with EDID name "BBB").

In this change, we add resolution to this configuration surface (e.g.,
you have 2 heads - one with resolution 111x222 and the other with
resolution 333x444).

  -display vnc=localhost:0,id=aaa,display=vga,head=0 \
  -display vnc=localhost:1,id=bbb,display=vga,head=1 \
  -device '{"driver":"virtio-vga",
            "max_outputs":2,
            "id":"vga",
            "outputs":[
              {
                 "name":"AAA",
                 "xres":111,
                 "yres":222
              },
              {
                 "name":"BBB",
                 "xres":333,
                 "yres":444
              }
            ]}'

Here is the behavior matrix of the current resolution configuration
surface (xres/yres) with the new resolution configuration surface
(outputs[i].xres/yres).

Case: !(xres || yres) && !(outputs[i].has_xres && outputs[i].has_yres)
Behavior: current behavior - outputs[0] enabled with default xres/yres

Case: (xres || yres) && !(outputs[i].has_xres && outputs[i].has_yres)
Behavior: current behavior - outputs[0] enabled with xres/yres

Case: !(xres || yres) && (outputs[i].has_xres && outputs[i].has_yres)
Behavior: new behavior - outputs[i] enabled with outputs[i].xres/yres

Case: (xres || yres) && (outputs[i].has_xres && outputs[i].has_yres)
Behavior: new behavior - outputs[i] enabled with outputs[i].xres/yres
Signed-off-by: Andrew Keesler <ankeesler@google.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-ID: <20250902141312.750525-2-ankeesler@google.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 qapi/virtio.json             | 10 ++++++++--
 hw/display/virtio-gpu-base.c | 10 ++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/qapi/virtio.json b/qapi/virtio.json
index 05295ab6655..0ce789bb22f 100644
--- a/qapi/virtio.json
+++ b/qapi/virtio.json
@@ -971,15 +971,21 @@
 ##
 # @VirtIOGPUOutput:
 #
-# Describes configuration of a VirtIO GPU output.
+# Describes configuration of a VirtIO GPU output. If both xres and
+# yres are set, they take precedence over root virtio-gpu
+# resolution configuration and enable the corresponding output.
 #
 # @name: the name of the output
 #
+# @xres: horizontal resolution of the output in pixels (since 10.2)
+#
+# @yres: vertical resolution of the output in pixels (since 10.2)
+#
 # Since: 10.1
 ##
 
 { 'struct': 'VirtIOGPUOutput',
-  'data': { 'name': 'str' } }
+  'data': { 'name': 'str', '*xres': 'uint16', '*yres': 'uint16' } }
 
 ##
 # @DummyVirtioForceArrays:
diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index 7269477a1c8..6adb5312a40 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -233,6 +233,16 @@ virtio_gpu_base_device_realize(DeviceState *qdev,
     g->req_state[0].width = g->conf.xres;
     g->req_state[0].height = g->conf.yres;
 
+    for (output_idx = 0, node = g->conf.outputs;
+         node && output_idx < g->conf.max_outputs;
+         output_idx++, node = node->next) {
+        if (node->value->has_xres && node->value->has_yres) {
+            g->enabled_output_bitmask |= (1 << output_idx);
+            g->req_state[output_idx].width = node->value->xres;
+            g->req_state[output_idx].height = node->value->yres;
+        }
+    }
+
     g->hw_ops = &virtio_gpu_ops;
     for (i = 0; i < g->conf.max_outputs; i++) {
         g->scanout[i].con =
-- 
2.47.3

Re: [PATCH 1/8] Support per-head resolutions with virtio-gpu

[Akihiko Odaki]

On 2025/10/14 20:12, Alex Bennée wrote:
> From: Andrew Keesler <ankeesler@google.com>
> 
> In 454f4b0f, we started down the path of supporting separate
> configurations per display head (e.g., you have 2 heads - one with
> EDID name "AAA" and the other with EDID name "BBB").
> 
> In this change, we add resolution to this configuration surface (e.g.,
> you have 2 heads - one with resolution 111x222 and the other with
> resolution 333x444).
> 
>    -display vnc=localhost:0,id=aaa,display=vga,head=0 \
>    -display vnc=localhost:1,id=bbb,display=vga,head=1 \
>    -device '{"driver":"virtio-vga",
>              "max_outputs":2,
>              "id":"vga",
>              "outputs":[
>                {
>                   "name":"AAA",
>                   "xres":111,
>                   "yres":222
>                },
>                {
>                   "name":"BBB",
>                   "xres":333,
>                   "yres":444
>                }
>              ]}'
> 
> Here is the behavior matrix of the current resolution configuration
> surface (xres/yres) with the new resolution configuration surface
> (outputs[i].xres/yres).
> 
> Case: !(xres || yres) && !(outputs[i].has_xres && outputs[i].has_yres)
> Behavior: current behavior - outputs[0] enabled with default xres/yres
> 
> Case: (xres || yres) && !(outputs[i].has_xres && outputs[i].has_yres)
> Behavior: current behavior - outputs[0] enabled with xres/yres
> 
> Case: !(xres || yres) && (outputs[i].has_xres && outputs[i].has_yres)
> Behavior: new behavior - outputs[i] enabled with outputs[i].xres/yres
> 
> Case: (xres || yres) && (outputs[i].has_xres && outputs[i].has_yres)
> Behavior: new behavior - outputs[i] enabled with outputs[i].xres/yres
> Signed-off-by: Andrew Keesler <ankeesler@google.com>

Nitpick: it is better to have a blank line between Behavior: and 
Signed-off-by: to clarify the beginning of tags.

> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Message-ID: <20250902141312.750525-2-ankeesler@google.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   qapi/virtio.json             | 10 ++++++++--
>   hw/display/virtio-gpu-base.c | 10 ++++++++++
>   2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/virtio.json b/qapi/virtio.json
> index 05295ab6655..0ce789bb22f 100644
> --- a/qapi/virtio.json
> +++ b/qapi/virtio.json
> @@ -971,15 +971,21 @@
>   ##
>   # @VirtIOGPUOutput:
>   #
> -# Describes configuration of a VirtIO GPU output.
> +# Describes configuration of a VirtIO GPU output. If both xres and
> +# yres are set, they take precedence over root virtio-gpu
> +# resolution configuration and enable the corresponding output.
>   #
>   # @name: the name of the output
>   #
> +# @xres: horizontal resolution of the output in pixels (since 10.2)
> +#
> +# @yres: vertical resolution of the output in pixels (since 10.2)
> +#
>   # Since: 10.1
>   ##
>   
>   { 'struct': 'VirtIOGPUOutput',
> -  'data': { 'name': 'str' } }
> +  'data': { 'name': 'str', '*xres': 'uint16', '*yres': 'uint16' } }
>   
>   ##
>   # @DummyVirtioForceArrays:
> diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
> index 7269477a1c8..6adb5312a40 100644
> --- a/hw/display/virtio-gpu-base.c
> +++ b/hw/display/virtio-gpu-base.c
> @@ -233,6 +233,16 @@ virtio_gpu_base_device_realize(DeviceState *qdev,
>       g->req_state[0].width = g->conf.xres;
>       g->req_state[0].height = g->conf.yres;
>   
> +    for (output_idx = 0, node = g->conf.outputs;
> +         node && output_idx < g->conf.max_outputs;

output_idx < g->conf.max_outputs is redundant as it is already enforced 
with the first for-loop that enumerates outputs.

The condition can be simply removed, but I think merging this loop to 
the earlier loop will make the code a bit more concise.

Aside this redundancy of the code, the logic this patch implements looks 
good to me.

> +         output_idx++, node = node->next) {
> +        if (node->value->has_xres && node->value->has_yres) {
> +            g->enabled_output_bitmask |= (1 << output_idx);
> +            g->req_state[output_idx].width = node->value->xres;
> +            g->req_state[output_idx].height = node->value->yres;
> +        }
> +    }
> +
>       g->hw_ops = &virtio_gpu_ops;
>       for (i = 0; i < g->conf.max_outputs; i++) {
>           g->scanout[i].con =

Re: [PATCH 1/8] Support per-head resolutions with virtio-gpu

[Alex Bennée]

Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:

> On 2025/10/14 20:12, Alex Bennée wrote:
>> From: Andrew Keesler <ankeesler@google.com>
>> In 454f4b0f, we started down the path of supporting separate
>> configurations per display head (e.g., you have 2 heads - one with
>> EDID name "AAA" and the other with EDID name "BBB").
<snip>
>> diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
>> index 7269477a1c8..6adb5312a40 100644
>> --- a/hw/display/virtio-gpu-base.c
>> +++ b/hw/display/virtio-gpu-base.c
>> @@ -233,6 +233,16 @@ virtio_gpu_base_device_realize(DeviceState *qdev,
>>       g->req_state[0].width = g->conf.xres;
>>       g->req_state[0].height = g->conf.yres;
>>   +    for (output_idx = 0, node = g->conf.outputs;
>> +         node && output_idx < g->conf.max_outputs;
>
> output_idx < g->conf.max_outputs is redundant as it is already
> enforced with the first for-loop that enumerates outputs.
>
> The condition can be simply removed, but I think merging this loop to
> the earlier loop will make the code a bit more concise.

I've dropped the extra condition check but I've left the loop where it
is as I don't want to change the effect of:

    g->req_state[0].width = g->conf.xres;
    g->req_state[0].height = g->conf.yres;

above. 

>
> Aside this redundancy of the code, the logic this patch implements
> looks good to me.
>
>> +         output_idx++, node = node->next) {
>> +        if (node->value->has_xres && node->value->has_yres) {
>> +            g->enabled_output_bitmask |= (1 << output_idx);
>> +            g->req_state[output_idx].width = node->value->xres;
>> +            g->req_state[output_idx].height = node->value->yres;
>> +        }
>> +    }
>> +
>>       g->hw_ops = &virtio_gpu_ops;
>>       for (i = 0; i < g->conf.max_outputs; i++) {
>>           g->scanout[i].con =

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 2/8] system/memory: add memory_region_finalize tracepoint

[Alex Bennée]

This only traces named memory regions as it is otherwise quite noisy
every time the address map changes.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 system/memory.c     | 5 +++++
 system/trace-events | 1 +
 2 files changed, 6 insertions(+)

diff --git a/system/memory.c b/system/memory.c
index 8b84661ae36..fd7c3192ed4 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1821,6 +1821,11 @@ static void memory_region_finalize(Object *obj)
      * memory_region_set_enabled instead could trigger a transaction and
      * cause an infinite loop.
      */
+
+    if (mr->name) {
+        trace_memory_region_finalize(mr, mr->name);
+    }
+
     mr->enabled = false;
     memory_region_transaction_begin();
     if (mr->container) {
diff --git a/system/trace-events b/system/trace-events
index 82856e44f2e..a8ef2326e14 100644
--- a/system/trace-events
+++ b/system/trace-events
@@ -23,6 +23,7 @@ memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, uint64_t v
 memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
 memory_region_sync_dirty(const char *mr, const char *listener, int global) "mr '%s' listener '%s' synced (global=%d)"
+memory_region_finalize(void *mr, const char *name) "mr %p, %s"
 flatview_new(void *view, void *root) "%p (root %p)"
 flatview_destroy(void *view, void *root) "%p (root %p)"
 flatview_destroy_rcu(void *view, void *root) "%p (root %p)"
-- 
2.47.3

Re: [PATCH 2/8] system/memory: add memory_region_finalize tracepoint

[Akihiko Odaki]

On 2025/10/14 20:12, Alex Bennée wrote:
> This only traces named memory regions as it is otherwise quite noisy
> every time the address map changes.

Checking for name does not seem effective to reduce noises. I used 
Coccinelle and found there are only three instances of unnamed memory 
regions. The three instances are:

- memory_region_init_alias() in xen_gnttab_realize()
- memory_region_init_io() in subpage_init()
- memory_region_init_io() in io_mem_init()

The command line I used is as follows:
spatch --macro-file scripts/cocci-macro-file.h --sp-file 
./scripts/coccinelle/a.cocci --keep-comments --in-place --use-gitgrep 
--dir .

Below is the content of scripts/coccinelle/a.cocci:

@filter@
expression a, b, c, d;
position p;
@@

(
  memory_region_init@p(a, b, NULL, ...);
|
  memory_region_init_io@p(a, b, c, d, NULL, ...);
|
  memory_region_init_ram_nomigrate@p(a, b, NULL, ...);
|
  memory_region_init_ram_flags_nomigrate@p(a, b, NULL, ...);
|
  memory_region_init_resizeable_ram@p(a, b, NULL, ...);
|
  memory_region_init_ram_from_file@p(a, b, NULL, ...);
|
  memory_region_init_ram_from_fd@p(a, b, NULL, ...);
|
  memory_region_init_ram_ptr@p(a, b, NULL, ...);
|
  memory_region_init_ram_device_ptr@p(a, b, NULL, ...);
|
  memory_region_init_alias@p(a, b, NULL, ...);
|
  memory_region_init_rom_nomigrate@p(a, b, NULL, ...);
|
  memory_region_init_rom_device_nomigrate@p(a, b, c, d, NULL, ...);
|
  memory_region_init_iommu@p(a, b, c, d, NULL, ...);
|
  memory_region_init_ram@p(a, b, NULL, ...);
|
  memory_region_init_ram_guest_memfd@p(a, b, NULL, ...);
|
  memory_region_init_rom@p(a, b, NULL, ...);
|
  memory_region_init_rom_device@p(a, b, c, d, NULL, ...);
)

@script:python@
p << filter.p;
@@

cocci.print_main("found", p)


> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   system/memory.c     | 5 +++++
>   system/trace-events | 1 +
>   2 files changed, 6 insertions(+)
> 
> diff --git a/system/memory.c b/system/memory.c
> index 8b84661ae36..fd7c3192ed4 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -1821,6 +1821,11 @@ static void memory_region_finalize(Object *obj)
>        * memory_region_set_enabled instead could trigger a transaction and
>        * cause an infinite loop.
>        */
> +
> +    if (mr->name) {
> +        trace_memory_region_finalize(mr, mr->name);
> +    }
> +
>       mr->enabled = false;
>       memory_region_transaction_begin();
>       if (mr->container) {
> diff --git a/system/trace-events b/system/trace-events
> index 82856e44f2e..a8ef2326e14 100644
> --- a/system/trace-events
> +++ b/system/trace-events
> @@ -23,6 +23,7 @@ memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, uint64_t v
>   memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
>   memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
>   memory_region_sync_dirty(const char *mr, const char *listener, int global) "mr '%s' listener '%s' synced (global=%d)"
> +memory_region_finalize(void *mr, const char *name) "mr %p, %s"
>   flatview_new(void *view, void *root) "%p (root %p)"
>   flatview_destroy(void *view, void *root) "%p (root %p)"
>   flatview_destroy_rcu(void *view, void *root) "%p (root %p)"

[PATCH 3/8] ui/gtk-gl-area: Remove extra draw call in refresh

[Alex Bennée]

From: Dongwon Kim <dongwon.kim@intel.com>

This partially reverts commit 77bf310084dad38b3a2badf01766c659056f1cf2
which causes some guest display corruption when gtk-gl-area
is used for GTK rendering (e.g. Wayland Compositor) possibly due to
simulataneous accesses on the guest frame buffer by host compositor
and the guest.

Fixes: 77bf310084 ("ui/gtk: Draw guest frame at refresh cycle")
Reported-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Reported-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
Message-Id: <20250214170813.2234754-1-dongwon.kim@intel.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-ID: <20250603110204.838117-12-alex.bennee@linaro.org>
Cc: qemu-stable@nongnu.org
---
 ui/gtk-gl-area.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
index 05fc38096ec..9a11c9b4d18 100644
--- a/ui/gtk-gl-area.c
+++ b/ui/gtk-gl-area.c
@@ -165,7 +165,6 @@ void gd_gl_area_refresh(DisplayChangeListener *dcl)
 
     if (vc->gfx.guest_fb.dmabuf &&
         qemu_dmabuf_get_draw_submitted(vc->gfx.guest_fb.dmabuf)) {
-        gd_gl_area_draw(vc);
         return;
     }
 
-- 
2.47.3

Re: [PATCH 3/8] ui/gtk-gl-area: Remove extra draw call in refresh

[Akihiko Odaki]

On 2025/10/14 20:12, Alex Bennée wrote:
> From: Dongwon Kim <dongwon.kim@intel.com>
> 
> This partially reverts commit 77bf310084dad38b3a2badf01766c659056f1cf2
> which causes some guest display corruption when gtk-gl-area
> is used for GTK rendering (e.g. Wayland Compositor) possibly due to
> simulataneous accesses on the guest frame buffer by host compositor
> and the guest.
> 
> Fixes: 77bf310084 ("ui/gtk: Draw guest frame at refresh cycle")
> Reported-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Reported-by: Alex Bennée <alex.bennee@linaro.org>
> Tested-by: Alex Bennée <alex.bennee@linaro.org>
> Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
> Message-Id: <20250214170813.2234754-1-dongwon.kim@intel.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-ID: <20250603110204.838117-12-alex.bennee@linaro.org>
> Cc: qemu-stable@nongnu.org
> ---
>   ui/gtk-gl-area.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
> index 05fc38096ec..9a11c9b4d18 100644
> --- a/ui/gtk-gl-area.c
> +++ b/ui/gtk-gl-area.c
> @@ -165,7 +165,6 @@ void gd_gl_area_refresh(DisplayChangeListener *dcl)
>   
>       if (vc->gfx.guest_fb.dmabuf &&
>           qemu_dmabuf_get_draw_submitted(vc->gfx.guest_fb.dmabuf)) {
> -        gd_gl_area_draw(vc);


I suggested adding code comment for the lack of gd_gl_area_draw() here a 
while ago but it seems it is missed since then:
https://lore.kernel.org/qemu-devel/63911dcc-482b-45c5-9468-120ae3df691b@daynix.com/

The removal of this function call itself looks good to me.

>           return;
>       }
>   

Re: [PATCH 3/8] ui/gtk-gl-area: Remove extra draw call in refresh

[Alex Bennée]

Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:

> On 2025/10/14 20:12, Alex Bennée wrote:
>> From: Dongwon Kim <dongwon.kim@intel.com>
>> This partially reverts commit
>> 77bf310084dad38b3a2badf01766c659056f1cf2
>> which causes some guest display corruption when gtk-gl-area
>> is used for GTK rendering (e.g. Wayland Compositor) possibly due to
>> simulataneous accesses on the guest frame buffer by host compositor
>> and the guest.
>> Fixes: 77bf310084 ("ui/gtk: Draw guest frame at refresh cycle")
>> Reported-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> Reported-by: Alex Bennée <alex.bennee@linaro.org>
>> Tested-by: Alex Bennée <alex.bennee@linaro.org>
>> Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
>> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
>> Message-Id: <20250214170813.2234754-1-dongwon.kim@intel.com>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Message-ID: <20250603110204.838117-12-alex.bennee@linaro.org>
>> Cc: qemu-stable@nongnu.org
>> ---
>>   ui/gtk-gl-area.c | 1 -
>>   1 file changed, 1 deletion(-)
>> diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
>> index 05fc38096ec..9a11c9b4d18 100644
>> --- a/ui/gtk-gl-area.c
>> +++ b/ui/gtk-gl-area.c
>> @@ -165,7 +165,6 @@ void gd_gl_area_refresh(DisplayChangeListener *dcl)
>>         if (vc->gfx.guest_fb.dmabuf &&
>>           qemu_dmabuf_get_draw_submitted(vc->gfx.guest_fb.dmabuf)) {
>> -        gd_gl_area_draw(vc);
>
>
> I suggested adding code comment for the lack of gd_gl_area_draw() here
> a while ago but it seems it is missed since then:
> https://lore.kernel.org/qemu-devel/63911dcc-482b-45c5-9468-120ae3df691b@daynix.com/
>
> The removal of this function call itself looks good to me.

What comment would you like - its not clear from the thread what I
should add.

>
>>           return;
>>       }
>>   

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 3/8] ui/gtk-gl-area: Remove extra draw call in refresh

[Akihiko Odaki]

On 2025/10/15 19:45, Alex Bennée wrote:
> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:
> 
>> On 2025/10/14 20:12, Alex Bennée wrote:
>>> From: Dongwon Kim <dongwon.kim@intel.com>
>>> This partially reverts commit
>>> 77bf310084dad38b3a2badf01766c659056f1cf2
>>> which causes some guest display corruption when gtk-gl-area
>>> is used for GTK rendering (e.g. Wayland Compositor) possibly due to
>>> simulataneous accesses on the guest frame buffer by host compositor
>>> and the guest.
>>> Fixes: 77bf310084 ("ui/gtk: Draw guest frame at refresh cycle")
>>> Reported-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>> Reported-by: Alex Bennée <alex.bennee@linaro.org>
>>> Tested-by: Alex Bennée <alex.bennee@linaro.org>
>>> Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
>>> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
>>> Message-Id: <20250214170813.2234754-1-dongwon.kim@intel.com>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Message-ID: <20250603110204.838117-12-alex.bennee@linaro.org>
>>> Cc: qemu-stable@nongnu.org
>>> ---
>>>    ui/gtk-gl-area.c | 1 -
>>>    1 file changed, 1 deletion(-)
>>> diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
>>> index 05fc38096ec..9a11c9b4d18 100644
>>> --- a/ui/gtk-gl-area.c
>>> +++ b/ui/gtk-gl-area.c
>>> @@ -165,7 +165,6 @@ void gd_gl_area_refresh(DisplayChangeListener *dcl)
>>>          if (vc->gfx.guest_fb.dmabuf &&
>>>            qemu_dmabuf_get_draw_submitted(vc->gfx.guest_fb.dmabuf)) {
>>> -        gd_gl_area_draw(vc);
>>
>>
>> I suggested adding code comment for the lack of gd_gl_area_draw() here
>> a while ago but it seems it is missed since then:
>> https://lore.kernel.org/qemu-devel/63911dcc-482b-45c5-9468-120ae3df691b@daynix.com/
>>
>> The removal of this function call itself looks good to me.
> 
> What comment would you like - its not clear from the thread what I
> should add.

Below is an idea of comment:

===

gd_egl_refresh() calls gd_egl_draw() if a DMA-BUF draw has already
been submitted, but this function does not call gd_gl_area_draw() in
such a case due to display corruption.

Calling gd_gl_area_draw() is necessary to prevent a situation where
there is a scheduled draw event but it won't happen bacause the window
is currently in inactive state (minimized or tabified). If draw is not
done for a long time, gl_block timeout and/or fence timeout (on the
guest) will happen eventually.

However, it is found that calling gd_gl_area_draw() here causes guest
display corruption on a Wayland Compositor. The display corruption is
more serious than the possible fence timeout so gd_gl_area_draw() is
omitted for now.

===

In the thread, it was discussed that the removal of gd_gl_area_draw() 
can cause a regression but it is necessary to fix a bigger problem 
(display corruption on Wayland), and I suggested to note the regression 
so that we won't lose track of it.

To remind of the regression, the comment first describes it omits 
gd_gl_area_draw(), and then explains why omitting gd_gl_area_draw() can 
cause a problem, which once fixed by the reverted change. Finally, it 
explains that omitting gd_gl_area_draw() is necessary to avoid display 
corruption on Wayland.

Regards,
Akihiko Odaki

[PATCH 4/8] hw/display: add blob map/unmap trace events

[Alex Bennée]

As these events happen dynamically as the guest does various things
they are quite handy to trace.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/display/virtio-gpu-virgl.c | 4 ++++
 hw/display/trace-events       | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 94ddc01f91c..07f6355ad62 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -134,6 +134,8 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
 
     res->mr = mr;
 
+    trace_virtio_gpu_cmd_res_map_blob(res->base.resource_id, vmr, mr);
+
     return 0;
 }
 
@@ -153,6 +155,8 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
 
     vmr = to_hostmem_region(res->mr);
 
+    trace_virtio_gpu_cmd_res_unmap_blob(res->base.resource_id, mr, vmr->finish_unmapping);
+
     /*
      * Perform async unmapping in 3 steps:
      *
diff --git a/hw/display/trace-events b/hw/display/trace-events
index 52786e6e184..e323a82cff2 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -38,6 +38,8 @@ virtio_gpu_cmd_set_scanout_blob(uint32_t id, uint32_t res, uint32_t w, uint32_t
 virtio_gpu_cmd_res_create_2d(uint32_t res, uint32_t fmt, uint32_t w, uint32_t h) "res 0x%x, fmt 0x%x, w %d, h %d"
 virtio_gpu_cmd_res_create_3d(uint32_t res, uint32_t fmt, uint32_t w, uint32_t h, uint32_t d) "res 0x%x, fmt 0x%x, w %d, h %d, d %d"
 virtio_gpu_cmd_res_create_blob(uint32_t res, uint64_t size) "res 0x%x, size %" PRId64
+virtio_gpu_cmd_res_map_blob(uint32_t res, void *vmr, void *mr) "res 0x%x, vmr %p, mr %p"
+virtio_gpu_cmd_res_unmap_blob(uint32_t res, void *mr, bool finish_unmapping) "res 0x%x, mr %p, finish_unmapping %d"
 virtio_gpu_cmd_res_unref(uint32_t res) "res 0x%x"
 virtio_gpu_cmd_res_back_attach(uint32_t res) "res 0x%x"
 virtio_gpu_cmd_res_back_detach(uint32_t res) "res 0x%x"
-- 
2.47.3

Re: [PATCH 4/8] hw/display: add blob map/unmap trace events

[Akihiko Odaki]

On 2025/10/14 20:12, Alex Bennée wrote:
> As these events happen dynamically as the guest does various things
> they are quite handy to trace.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>

I think you can include this in the pull request for per-head and 
ui/gtk-gl-area changes you are going to send.

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

[Alex Bennée]

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 */
     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

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

[Peter Xu]

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

[PATCH 6/8] virtio-gpu: refactor async blob unmapping

[Alex Bennée]

From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Change the 3 part async cleanup of a blob memory mapping to check if the
unmapping has finished already after deleting the subregion; this
condition allows us to skip suspending the command and responding to the
guest right away.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20250410122643.1747913-4-manos.pitsidianakis@linaro.org>
Cc: qemu-stable@nongnu.org
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-ID: <20250603110204.838117-11-alex.bennee@linaro.org>
---
 hw/display/virtio-gpu-virgl.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 0ef0b2743fe..ca7c607bf67 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -159,7 +159,32 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
      *    asynchronously by virtio_gpu_virgl_hostmem_region_free().
      * 3. Finish the unmapping with final virgl_renderer_resource_unmap().
      */
+
+    /* 1. Check if we should start unmapping now */
+    if (!vmr->finish_unmapping) {
+        /* begin async unmapping. render will be unblocked once MR is freed */
+        b->renderer_blocked++;
+
+        memory_region_set_enabled(mr, false);
+        memory_region_del_subregion(&b->hostmem, mr);
+        object_unparent(OBJECT(mr));
+        /*
+         * The unmapping might have already finished at this point if no one
+         * else held a reference to the MR; if yes, we can skip suspending the
+         * command and unmap the resource right away.
+         */
+        *cmd_suspended = !vmr->finish_unmapping;
+    }
+
+    /*
+     * 2. if virtio_gpu_virgl_hostmem_region_free hasn't been executed yet, we
+     * have marked the command to be re-processed later by setting
+     * cmd_suspended to true. The freeing callback will be called from RCU
+     * context later.
+     */
+
     if (vmr->finish_unmapping) {
+        /* 3. MemoryRegion has been freed, so finish unmapping */
         res->mr = NULL;
         g_free(vmr);
 
@@ -170,16 +195,6 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
                           __func__, strerror(-ret));
             return ret;
         }
-    } else {
-        *cmd_suspended = true;
-
-        /* render will be unblocked once MR is freed */
-        b->renderer_blocked++;
-
-        /* memory region owns self res->mr object and frees it by itself */
-        memory_region_set_enabled(mr, false);
-        memory_region_del_subregion(&b->hostmem, mr);
-        object_unparent(OBJECT(mr));
     }
 
     return 0;
-- 
2.47.3

[PATCH 7/8] virtio-gpu: fix hang under TCG when unmapping blob

[Alex Bennée]

From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

This commit fixes an indefinite hang when using VIRTIO GPU blob objects
under TCG in certain conditions.

The VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB VIRTIO command creates a
MemoryRegion and attaches it to an offset on a PCI BAR of the
VirtIOGPUdevice. The VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB command unmaps
it.

Because virglrenderer commands are not thread-safe they are only
called on the main context and QEMU performs the cleanup in three steps
to prevent a use-after-free scenario where the guest can access the
region after it’s unmapped:

1. From the main context, the region’s field finish_unmapping is false
   by default, so it sets a variable cmd_suspended, increases the
   renderer_blocked variable, deletes the blob subregion, and unparents
   the blob subregion causing its reference count to decrement.

2. From an RCU context, the MemoryView gets freed, the FlatView gets
   recalculated, the free callback of the blob region
   virtio_gpu_virgl_hostmem_region_free is called which sets the
   region’s field finish_unmapping to true, allowing the main thread
   context to finish replying to the command

3. From the main context, the command is processed again, but this time
   finish_unmapping is true, so virgl_renderer_resource_unmap can be
   called and a response is sent to the guest.

It happens so that under TCG, if the guest has no timers configured (and
thus no interrupt will cause the CPU to exit), the RCU thread does not
have enough time to grab the locks and recalculate the FlatView.

That’s not a big problem in practice since most guests will assume a
response will happen later in time and go on to do different things,
potentially triggering interrupts and allowing the RCU context to run.
If the guest waits for the unmap command to complete though, it blocks
indefinitely. Attaching to the QEMU monitor and force quitting the guest
allows the cleanup to continue.

There's no reason why the FlatView recalculation can't occur right away
when we delete the blob subregion, however. It does not, because when we
create the subregion we set the object as its own parent:

    memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);

The extra reference is what prevents freeing the memory region object in
the memory transaction of deleting the subregion.

This commit changes the owner object to the device, which removes the
extra owner reference in the memory region and causes the MR to be
freed right away in the main context.

Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20250410122643.1747913-3-manos.pitsidianakis@linaro.org>
Cc: qemu-stable@nongnu.org
---
 hw/display/virtio-gpu-virgl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index ca7c607bf67..d880adc8ab0 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -112,7 +112,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
     vmr->g = g;
     mr = g_new0(MemoryRegion, 1);
 
-    memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data);
+    memory_region_init_ram_ptr(mr, OBJECT(g), "blob", size, data);
     memory_region_add_subregion(&b->hostmem, offset, mr);
     memory_region_set_enabled(mr, true);
 
-- 
2.47.3

[PATCH 8/8] tests/functional: add GPU blob allocation test

[Alex Bennée]

This is a very short microkernel test that initialises and then maps
and unmaps a blob resource.

Without the other fixes in this series it causes QEMU to hang on the
unhandled unmap.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/functional/aarch64/meson.build      |  1 +
 tests/functional/aarch64/test_gpu_blob.py | 73 +++++++++++++++++++++++
 2 files changed, 74 insertions(+)
 create mode 100755 tests/functional/aarch64/test_gpu_blob.py

diff --git a/tests/functional/aarch64/meson.build b/tests/functional/aarch64/meson.build
index 5ad52f93e1d..f6ca33b2be4 100644
--- a/tests/functional/aarch64/meson.build
+++ b/tests/functional/aarch64/meson.build
@@ -26,6 +26,7 @@ tests_aarch64_system_thorough = [
   'aspeed_ast2700',
   'aspeed_ast2700fc',
   'device_passthrough',
+  'gpu_blob',
   'hotplug_pci',
   'imx8mp_evk',
   'kvm',
diff --git a/tests/functional/aarch64/test_gpu_blob.py b/tests/functional/aarch64/test_gpu_blob.py
new file mode 100755
index 00000000000..a913d3b29c8
--- /dev/null
+++ b/tests/functional/aarch64/test_gpu_blob.py
@@ -0,0 +1,73 @@
+#!/usr/bin/env python3
+#
+# Functional tests for GPU blob support. This is a directed test to
+# exercise the blob creation and removal features of virtio-gpu. You
+# can find the source code for microkernel test here:
+#   https://gitlab.com/epilys/qemu-880-repro
+#
+# Copyright (c) 2025 Linaro Ltd.
+#
+# Authors:
+#  Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
+#  Alex Bennée <alex.bennee@linaro.org>
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+from qemu.machine.machine import VMLaunchFailure
+
+from qemu_test import Asset
+from qemu_test import wait_for_console_pattern
+from qemu_test.linuxkernel import LinuxKernelTest
+
+class Aarch64VirtBlobTest(LinuxKernelTest):
+
+    ASSET_BLOB = Asset('https://fileserver.linaro.org/s/kE4nCFLdQcoBF9t/'
+                       'download?path=%2Fblob-test&files=qemu-880.bin',
+                       '2f6ab85d0b156c94fcedd2c4c821c5cbd52925a2de107f8e2d569ea2e34e42eb')
+
+    def test_virtio_gpu_blob(self):
+
+        self.set_machine('virt')
+        self.require_accelerator("tcg")
+
+        blob = self.ASSET_BLOB.fetch()
+
+        self.vm.set_console()
+
+        self.vm.add_args("-machine", "virt,memory-backend=mem0,accel=tcg",
+                         '-m', '4G',
+                         '-cpu', 'max',
+                         '-kernel', blob,
+                         '-object', 'memory-backend-memfd,share=on,id=mem0,size=4G',
+                         '-global', 'virtio-mmio.force-legacy=false',
+                         '-nic', 'none',
+                         '-device',
+                         'virtio-gpu-gl,hostmem=128M,blob=true,venus=true',
+                         '-display', 'egl-headless,gl=on',
+                         '-d', 'guest_errors')
+
+        try:
+            self.vm.launch()
+        except VMLaunchFailure as excp:
+            if "old virglrenderer, blob resources unsupported" in excp.output:
+                self.skipTest("No blob support for virtio-gpu")
+            elif "old virglrenderer, venus unsupported" in excp.output:
+                self.skipTest("No venus support for virtio-gpu")
+            elif "egl: no drm render node available" in excp.output:
+                self.skipTest("Can't access host DRM render node")
+            elif "'type' does not accept value 'egl-headless'" in excp.output:
+                self.skipTest("egl-headless support is not available")
+            elif "'type' does not accept value 'dbus'" in excp.output:
+                self.skipTest("dbus display support is not available")
+            elif "eglInitialize failed: EGL_NOT_INITIALIZED" in excp.output:
+                self.skipTest("EGL failed to initialize on this host")
+            else:
+                self.log.info("unhandled launch failure: %s", excp.output)
+                raise excp
+
+        self.wait_for_console_pattern('[INFO] virtio-gpu test finished')
+        # the test should cleanly exit
+
+
+if __name__ == '__main__':
+    LinuxKernelTest.main()
-- 
2.47.3

Re: [PATCH 8/8] tests/functional: add GPU blob allocation test

[Akihiko Odaki]

On 2025/10/14 20:12, Alex Bennée wrote:
> This is a very short microkernel test that initialises and then maps
> and unmaps a blob resource.
> 
> Without the other fixes in this series it causes QEMU to hang on the
> unhandled unmap.

Thank you for the reproduction case.

I don't have time to look into it this and the next week due to 
attendance to a conference (MICRO-58), but I'll definitely do so after that.

> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   tests/functional/aarch64/meson.build      |  1 +
>   tests/functional/aarch64/test_gpu_blob.py | 73 +++++++++++++++++++++++
>   2 files changed, 74 insertions(+)
>   create mode 100755 tests/functional/aarch64/test_gpu_blob.py
> 
> diff --git a/tests/functional/aarch64/meson.build b/tests/functional/aarch64/meson.build
> index 5ad52f93e1d..f6ca33b2be4 100644
> --- a/tests/functional/aarch64/meson.build
> +++ b/tests/functional/aarch64/meson.build
> @@ -26,6 +26,7 @@ tests_aarch64_system_thorough = [
>     'aspeed_ast2700',
>     'aspeed_ast2700fc',
>     'device_passthrough',
> +  'gpu_blob',
>     'hotplug_pci',
>     'imx8mp_evk',
>     'kvm',
> diff --git a/tests/functional/aarch64/test_gpu_blob.py b/tests/functional/aarch64/test_gpu_blob.py
> new file mode 100755
> index 00000000000..a913d3b29c8
> --- /dev/null
> +++ b/tests/functional/aarch64/test_gpu_blob.py
> @@ -0,0 +1,73 @@
> +#!/usr/bin/env python3
> +#
> +# Functional tests for GPU blob support. This is a directed test to
> +# exercise the blob creation and removal features of virtio-gpu. You
> +# can find the source code for microkernel test here:
> +#   https://gitlab.com/epilys/qemu-880-repro

Nice. I appreciate the effort to creat the microkernel; hopefully it 
will be useful also to exercise this part of virtio-gpu for debugging in 
the future.

> +#
> +# Copyright (c) 2025 Linaro Ltd.
> +#
> +# Authors:
> +#  Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> +#  Alex Bennée <alex.bennee@linaro.org>
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +from qemu.machine.machine import VMLaunchFailure
> +
> +from qemu_test import Asset
> +from qemu_test import wait_for_console_pattern
> +from qemu_test.linuxkernel import LinuxKernelTest
> +
> +class Aarch64VirtBlobTest(LinuxKernelTest):
> +
> +    ASSET_BLOB = Asset('https://fileserver.linaro.org/s/kE4nCFLdQcoBF9t/'
> +                       'download?path=%2Fblob-test&files=qemu-880.bin',
> +                       '2f6ab85d0b156c94fcedd2c4c821c5cbd52925a2de107f8e2d569ea2e34e42eb')
> +
> +    def test_virtio_gpu_blob(self):
> +
> +        self.set_machine('virt')
> +        self.require_accelerator("tcg")
> +
> +        blob = self.ASSET_BLOB.fetch()
> +
> +        self.vm.set_console()
> +
> +        self.vm.add_args("-machine", "virt,memory-backend=mem0,accel=tcg",
> +                         '-m', '4G',
> +                         '-cpu', 'max',
> +                         '-kernel', blob,
> +                         '-object', 'memory-backend-memfd,share=on,id=mem0,size=4G',
> +                         '-global', 'virtio-mmio.force-legacy=false',
> +                         '-nic', 'none',
> +                         '-device',
> +                         'virtio-gpu-gl,hostmem=128M,blob=true,venus=true',

venus is not exercised with this test case so can be removed.

This test somehow hung on my laptop even with the all other patches in 
this series applied, and removing venus=true fixed the hung.

I suppose the hang is unrelated to the problem fixed in this series and 
but rather is a problem specific to my environment (Asahi Linux). 
Anyway, I think removing venus=true is a safer option to avoid problems 
with the host graphics stack.

Below is a part of build/meson-logs/testlog.txt:

==================================== 1/1 
=====================================
test:         qemu:func-thorough+func-aarch64-thorough+thorough / 
func-aarch64-gpu_blob
start time:   02:25:49
duration:     90.01s
result:       killed by signal 15 SIGTERM
command: 
MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 
MALLOC_PERTURB_=12 RUST_BACKTRACE=1 
QEMU_TEST_QEMU_BINARY=/home/me/q/var/qemu/build/qemu-system-aarch64 
UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 
PYTHONPATH=/home/me/q/var/qemu/python:/home/me/q/var/qemu/tests/functional 
ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 
MESON_TEST_ITERATION=1 QEMU_BUILD_ROOT=/home/me/q/var/qemu/build 
QEMU_TEST_QEMU_IMG=/home/me/q/var/qemu/build/qemu-img 
LD_LIBRARY_PATH=/home/me/q/var/qemu/build/contrib/plugins:/home/me/q/var/qemu/build/tests/tcg/plugins 
/home/me/q/var/qemu/build/pyvenv/bin/python3 
/home/me/q/var/qemu/tests/functional/aarch64/test_gpu_blob.py
----------------------------------- stderr 
-----------------------------------
==31639==WARNING: ASan doesn't fully support makecontext/swapcontext 
functions and may produce false positives in some cases!
==============================================================================


Summary of Failures:

1/1 qemu:func-thorough+func-aarch64-thorough+thorough / 
func-aarch64-gpu_blob TIMEOUT        90.01s   killed by signal 15 SIGTERM

Ok:                 0
Expected Fail:      0
Fail:               0
Unexpected Pass:    0
Skipped:            0
Timeout:            1

> +                         '-display', 'egl-headless,gl=on',
> +                         '-d', 'guest_errors')
> +
> +        try:
> +            self.vm.launch()
> +        except VMLaunchFailure as excp:
> +            if "old virglrenderer, blob resources unsupported" in excp.output:
> +                self.skipTest("No blob support for virtio-gpu")
> +            elif "old virglrenderer, venus unsupported" in excp.output:
> +                self.skipTest("No venus support for virtio-gpu")
> +            elif "egl: no drm render node available" in excp.output:
> +                self.skipTest("Can't access host DRM render node")

This condition is unnecessary as DRM render node is not used.

> +            elif "'type' does not accept value 'egl-headless'" in excp.output:
> +                self.skipTest("egl-headless support is not available")
> +            elif "'type' does not accept value 'dbus'" in excp.output:
> +                self.skipTest("dbus display support is not available")

This can be removed too.

> +            elif "eglInitialize failed: EGL_NOT_INITIALIZED" in excp.output:
> +                self.skipTest("EGL failed to initialize on this host")
> +            else:
> +                self.log.info("unhandled launch failure: %s", excp.output)
> +                raise excp
> +
> +        self.wait_for_console_pattern('[INFO] virtio-gpu test finished')
> +        # the test should cleanly exit
> +
> +
> +if __name__ == '__main__':
> +    LinuxKernelTest.main()

Re: [PATCH 8/8] tests/functional: add GPU blob allocation test

[Akihiko Odaki]

On 2025/10/15 11:41, Akihiko Odaki wrote:
> On 2025/10/14 20:12, Alex Bennée wrote:
>> This is a very short microkernel test that initialises and then maps
>> and unmaps a blob resource.
>>
>> Without the other fixes in this series it causes QEMU to hang on the
>> unhandled unmap.
> 
> Thank you for the reproduction case.
> 
> I don't have time to look into it this and the next week due to 
> attendance to a conference (MICRO-58), but I'll definitely do so after 
> that.
> 
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   tests/functional/aarch64/meson.build      |  1 +
>>   tests/functional/aarch64/test_gpu_blob.py | 73 +++++++++++++++++++++++
>>   2 files changed, 74 insertions(+)
>>   create mode 100755 tests/functional/aarch64/test_gpu_blob.py
>>
>> diff --git a/tests/functional/aarch64/meson.build b/tests/functional/ 
>> aarch64/meson.build
>> index 5ad52f93e1d..f6ca33b2be4 100644
>> --- a/tests/functional/aarch64/meson.build
>> +++ b/tests/functional/aarch64/meson.build
>> @@ -26,6 +26,7 @@ tests_aarch64_system_thorough = [
>>     'aspeed_ast2700',
>>     'aspeed_ast2700fc',
>>     'device_passthrough',
>> +  'gpu_blob',
>>     'hotplug_pci',
>>     'imx8mp_evk',
>>     'kvm',
>> diff --git a/tests/functional/aarch64/test_gpu_blob.py b/tests/ 
>> functional/aarch64/test_gpu_blob.py
>> new file mode 100755
>> index 00000000000..a913d3b29c8
>> --- /dev/null
>> +++ b/tests/functional/aarch64/test_gpu_blob.py
>> @@ -0,0 +1,73 @@
>> +#!/usr/bin/env python3
>> +#
>> +# Functional tests for GPU blob support. This is a directed test to
>> +# exercise the blob creation and removal features of virtio-gpu. You
>> +# can find the source code for microkernel test here:
>> +#   https://gitlab.com/epilys/qemu-880-repro
> 
> Nice. I appreciate the effort to creat the microkernel; hopefully it 
> will be useful also to exercise this part of virtio-gpu for debugging in 
> the future.
> 
>> +#
>> +# Copyright (c) 2025 Linaro Ltd.
>> +#
>> +# Authors:
>> +#  Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>> +#  Alex Bennée <alex.bennee@linaro.org>
>> +#
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +from qemu.machine.machine import VMLaunchFailure
>> +
>> +from qemu_test import Asset
>> +from qemu_test import wait_for_console_pattern

This import of wait_for_console_pattern is unused. There is a call of 
self.wait_for_console_pattern(), but it prefixed with "self." so doesn't 
use this import.

>> +from qemu_test.linuxkernel import LinuxKernelTest
>> +
>> +class Aarch64VirtBlobTest(LinuxKernelTest):
>> +
>> +    ASSET_BLOB = Asset('https://fileserver.linaro.org/s/ 
>> kE4nCFLdQcoBF9t/'
>> +                       'download?path=%2Fblob-test&files=qemu-880.bin',
>> +                       
>> '2f6ab85d0b156c94fcedd2c4c821c5cbd52925a2de107f8e2d569ea2e34e42eb')
>> +
>> +    def test_virtio_gpu_blob(self):
>> +
>> +        self.set_machine('virt')
>> +        self.require_accelerator("tcg")
>> +
>> +        blob = self.ASSET_BLOB.fetch()
>> +
>> +        self.vm.set_console()
>> +
>> +        self.vm.add_args("-machine", "virt,memory- 
>> backend=mem0,accel=tcg",
>> +                         '-m', '4G',
>> +                         '-cpu', 'max',
>> +                         '-kernel', blob,
>> +                         '-object', 'memory-backend- 
>> memfd,share=on,id=mem0,size=4G',
>> +                         '-global', 'virtio-mmio.force-legacy=false',
>> +                         '-nic', 'none',
>> +                         '-device',
>> +                         'virtio-gpu- 
>> gl,hostmem=128M,blob=true,venus=true',
> 
> venus is not exercised with this test case so can be removed.
> 
> This test somehow hung on my laptop even with the all other patches in 
> this series applied, and removing venus=true fixed the hung.
> 
> I suppose the hang is unrelated to the problem fixed in this series and 
> but rather is a problem specific to my environment (Asahi Linux). 
> Anyway, I think removing venus=true is a safer option to avoid problems 
> with the host graphics stack.
> 
> Below is a part of build/meson-logs/testlog.txt:
> 
> ==================================== 1/1 
> =====================================
> test:         qemu:func-thorough+func-aarch64-thorough+thorough / func- 
> aarch64-gpu_blob
> start time:   02:25:49
> duration:     90.01s
> result:       killed by signal 15 SIGTERM
> command: 
> MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 MALLOC_PERTURB_=12 RUST_BACKTRACE=1 QEMU_TEST_QEMU_BINARY=/home/me/q/var/qemu/build/qemu-system-aarch64 UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 PYTHONPATH=/home/me/q/var/qemu/python:/home/me/q/var/qemu/tests/functional ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 MESON_TEST_ITERATION=1 QEMU_BUILD_ROOT=/home/me/q/var/qemu/build QEMU_TEST_QEMU_IMG=/home/me/q/var/qemu/build/qemu-img LD_LIBRARY_PATH=/home/me/q/var/qemu/build/contrib/plugins:/home/me/q/var/qemu/build/tests/tcg/plugins /home/me/q/var/qemu/build/pyvenv/bin/python3 /home/me/q/var/qemu/tests/functional/aarch64/test_gpu_blob.py
> ----------------------------------- stderr 
> -----------------------------------
> ==31639==WARNING: ASan doesn't fully support makecontext/swapcontext 
> functions and may produce false positives in some cases!
> ==============================================================================
> 
> 
> Summary of Failures:
> 
> 1/1 qemu:func-thorough+func-aarch64-thorough+thorough / func-aarch64- 
> gpu_blob TIMEOUT        90.01s   killed by signal 15 SIGTERM
> 
> Ok:                 0
> Expected Fail:      0
> Fail:               0
> Unexpected Pass:    0
> Skipped:            0
> Timeout:            1
> 
>> +                         '-display', 'egl-headless,gl=on',
>> +                         '-d', 'guest_errors')
>> +
>> +        try:
>> +            self.vm.launch()
>> +        except VMLaunchFailure as excp:
>> +            if "old virglrenderer, blob resources unsupported" in 
>> excp.output:
>> +                self.skipTest("No blob support for virtio-gpu")
>> +            elif "old virglrenderer, venus unsupported" in excp.output:
>> +                self.skipTest("No venus support for virtio-gpu")
>> +            elif "egl: no drm render node available" in excp.output:
>> +                self.skipTest("Can't access host DRM render node")
> 
> This condition is unnecessary as DRM render node is not used.
> 
>> +            elif "'type' does not accept value 'egl-headless'" in 
>> excp.output:
>> +                self.skipTest("egl-headless support is not available")
>> +            elif "'type' does not accept value 'dbus'" in excp.output:
>> +                self.skipTest("dbus display support is not available")
> 
> This can be removed too.
> 
>> +            elif "eglInitialize failed: EGL_NOT_INITIALIZED" in 
>> excp.output:
>> +                self.skipTest("EGL failed to initialize on this host")
>> +            else:
>> +                self.log.info("unhandled launch failure: %s", 
>> excp.output)
>> +                raise excp
>> +
>> +        self.wait_for_console_pattern('[INFO] virtio-gpu test finished')
>> +        # the test should cleanly exit
>> +
>> +
>> +if __name__ == '__main__':
>> +    LinuxKernelTest.main()
>