[PATCH v1 0/2] virtio-gpu: fix blob unmapping sequence

[PATCH v1 0/2] virtio-gpu: fix blob unmapping sequence

[Manos Pitsidianakis]

A hang was observed when running a small kernel that exercised VIRTIO 
GPU under TCG. This is an edge-case and won't happen under typical 
conditions.

When unmapping a blob object, its MemoryRegion's freeing is deferred to 
the RCU thread. The hang's cause was determined to be a busy main loop 
not allowing for the RCU thread to run because the kernel did not setup 
any timers or had any interrupts on the way. While fixing the RCU thread 
to run even if the guest CPU spins is a solution, it's easier to fix the 
reason why the MemoryRegion isn't freed from the main loop instead.

While at it, also restructure the 3 stage cleanup to immediately respond 
to the guest if the MR happened to have had no other reference.

PS: The hang can be reproduced by running this unikernel with TCG 

https://git.codelinaro.org/manos.pitsidianakis/virtio-tests/-/tree/8c0ebe9395827e24aa5711186d499bf5de87cf63/virtio-test-suite

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

 hw/display/virtio-gpu-virgl.c | 37 ++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)


base-commit: 0adf626718bc0ca9c46550249a76047f8e45da15
-- 
γαῖα πυρί μιχθήτω

[PATCH v1 1/2] virtio-gpu: fix hang under TCG when unmapping blob

[Manos Pitsidianakis]

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
has no other interrupts that 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 self-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.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.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 145a0b3879..ad600ccc9c 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -120,7 +120,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
     vmr->g = g;
 
     mr = &vmr->mr;
-    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);
 
-- 
γαῖα πυρί μιχθήτω

[PATCH v1 2/2] virtio-gpu: refactor async blob unmapping

[Manos Pitsidianakis]

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>
---
 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 ad600ccc9c..c75206114d 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -162,7 +162,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);
 
@@ -173,16 +198,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;
-- 
γαῖα πυρί μιχθήτω

Re: [PATCH v1 1/2] virtio-gpu: fix hang under TCG when unmapping blob

[Michael S. Tsirkin]

On Thu, Apr 03, 2025 at 03:17:01PM +0300, Manos Pitsidianakis wrote:
> 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
> has no other interrupts that 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 self-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.
> 
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>


Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  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 145a0b3879..ad600ccc9c 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -120,7 +120,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>      vmr->g = g;
>  
>      mr = &vmr->mr;
> -    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);
>  
> -- 
> γαῖα πυρί μιχθήτω

Re: [PATCH v1 0/2] virtio-gpu: fix blob unmapping sequence

[Alex Bennée]

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

> A hang was observed when running a small kernel that exercised VIRTIO 
> GPU under TCG. This is an edge-case and won't happen under typical 
> conditions.
>
> When unmapping a blob object, its MemoryRegion's freeing is deferred to 
> the RCU thread. The hang's cause was determined to be a busy main loop 
> not allowing for the RCU thread to run because the kernel did not setup 
> any timers or had any interrupts on the way. While fixing the RCU thread 
> to run even if the guest CPU spins is a solution, it's easier to fix the 
> reason why the MemoryRegion isn't freed from the main loop instead.
>
> While at it, also restructure the 3 stage cleanup to immediately respond 
> to the guest if the MR happened to have had no other reference.
>
> PS: The hang can be reproduced by running this unikernel with TCG 
>
> https://git.codelinaro.org/manos.pitsidianakis/virtio-tests/-/tree/8c0ebe9395827e24aa5711186d499bf5de87cf63/virtio-test-suite

Hmm these seems to regress the virtio-gpu tests:

🕙10:18:49 alex@toolbox:qemu.git/builds/virtio-gpu  on  HEAD (ae021f8) (REBASING 4/9) [$+?] took 7s
➜  echo $LD_LIBRARY_PATH
/home/alex/lsrc/qemu.git/builds/extra.libs/install/lib /home/alex/lsrc/qemu.git/builds/extra.libs/install/lib/x86_64-linux-gnu
🕙10:18:54 alex@toolbox:qemu.git/builds/virtio-gpu  on  HEAD (ae021f8) (REBASING 4/9) [$+?]
➜  echo $PKG_CONFIG_PATH
/home/alex/lsrc/qemu.git/builds/extra.libs/install/lib/pkgconfig /home/alex/lsrc/qemu.git/builds/extra.libs/install/lib/x86_64-linux-gnu/pkgconfig/
🕙10:18:58 alex@toolbox:qemu.git/builds/virtio-gpu  on  HEAD (ae021f8) (REBASING 4/9) [$+?]
➜  head config.log
# QEMU configure log Wed Apr  9 10:01:50 UTC 2025
# Configured with: '../../configure' '--disable-docs' '--enable-virglrenderer' '--target-list=aarch64-softmmu,x86_64-softmmu' '--enable-debug' '--skip-meson'

#
cc -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
cc -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
config-temp/qemu-conf.c:2:2: error: #error __i386__ not defined
    2 | #error __i386__ not defined
      |  ^~~~~
cc -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
🕙10:19:02 alex@toolbox:qemu.git/builds/virtio-gpu  on  HEAD (ae021f8) (REBASING 4/9) [$+?]
➜  make -j30
/home/alex/lsrc/qemu.git/builds/virtio-gpu/pyvenv/bin/meson introspect --targets --tests --benchmarks | /home/alex/lsrc/qemu.git/builds/virtio-gpu/pyvenv/bin/python3 -B scripts/mtest2make.py > Makefile.mtest
[1/19] Generating qemu-version.h with a custom command (wrapped by meson to capture output)
🕙10:19:09 alex@toolbox:qemu.git/builds/virtio-gpu  on  HEAD (ae021f8) (REBASING 4/9) [$+?]
➜  ./pyvenv/bin/meson test --setup thorough --suite func-thorough func-aarch64-aarch64_virt_gpu -v
ninja: Entering directory `/home/alex/lsrc/qemu.git/builds/virtio-gpu'
[1/6] Generating qemu-version.h with a custom command (wrapped by meson to capture output)
1/1 qemu:func-thorough+func-aarch64-thorough+thorough / func-aarch64-aarch64_virt_gpu        RUNNING
>>> QEMU_BUILD_ROOT=/home/alex/lsrc/qemu.git/builds/virtio-gpu G_TEST_SLOW=1 QEMU_TEST_QEMU_BINARY=/home/alex/lsrc/qemu.git/builds/virtio-gpu/qemu-system-aarch64 MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 MALLOC_PERTURB_=52 PYTHONPATH=/home/alex/lsrc/qemu.git/python:/home/alex/lsrc/qemu.git/tests/functional RUST_BACKTRACE=1 LD_LIBRARY_PATH=/home/alex/lsrc/qemu.git/builds/virtio-gpu/tests/tcg/plugins:/home/alex/lsrc/qemu.git/builds/virtio-gpu/contrib/plugins:/home/alex/lsrc/qemu.git/builds/extra.libs/install/lib:/home/alex/lsrc/qemu.git/builds/extra.libs/install/lib/x86_64-linux-gnu SPEED=thorough ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 QEMU_TEST_QEMU_IMG=/home/alex/lsrc/qemu.git/builds/virtio-gpu/qemu-img MESON_TEST_ITERATION=1 /home/alex/lsrc/qemu.git/builds/virtio-gpu/pyvenv/bin/python3 /home/alex/lsrc/qemu.git/tests/functional/test_aarch64_virt_gpu.py
▶ 1/1 test_aarch64_virt_gpu.Aarch64VirtGPUMachine.test_aarch64_virt_with_virgl_blobs_gpu     OK
▶ 1/1 test_aarch64_virt_gpu.Aarch64VirtGPUMachine.test_aarch64_virt_with_virgl_gpu           OK
▶ 1/1 test_aarch64_virt_gpu.Aarch64VirtGPUMachine.test_aarch64_virt_with_vulkan_gpu          OK
1/1 qemu:func-thorough+func-aarch64-thorough+thorough / func-aarch64-aarch64_virt_gpu        OK             207.15s   3 subtests passed


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

Full log written to /home/alex/lsrc/qemu.git/builds/virtio-gpu/meson-logs/testlog-thorough.txt
🕙10:22:41 alex@toolbox:qemu.git/builds/virtio-gpu  on  HEAD (ae021f8) (REBASING 4/9) [$+?] took 3m27s
➜  make -j30
[1/21] Generating qemu-version.h with a custom command (wrapped by meson to capture output)
[2/21] Compiling C object libcommon.a.p/hw_display_virtio-gpu-virgl.c.o
[3/21] Compiling C object libcommon.a.p/hw_virtio_virtio-pci.c.o
[4/21] Compiling C object qemu-vmsr-helper.p/tools_i386_qemu-vmsr-helper.c.o
[5/21] Compiling C object qemu-pr-helper.p/scsi_qemu-pr-helper.c.o
[6/21] Compiling C object qemu-io.p/qemu-io.c.o
[7/21] Linking target qemu-vmsr-helper
[8/21] Compiling C object qga/qemu-ga.p/main.c.o
[9/21] Compiling C object qemu-nbd.p/qemu-nbd.c.o
[10/21] Compiling C object libqmp.a.p/monitor_qmp-cmds-control.c.o
[11/21] Compiling C object storage-daemon/qemu-storage-daemon.p/qemu-storage-daemon.c.o
[12/21] Linking target qemu-pr-helper
[13/21] Linking target qga/qemu-ga
[14/21] Linking target qemu-io
[15/21] Linking target qemu-nbd
[16/21] Linking target storage-daemon/qemu-storage-daemon
[17/21] Compiling C object qemu-img.p/qemu-img.c.o
[18/21] Compiling C object libcommon.a.p/system_vl.c.o
[19/21] Linking target qemu-img
[20/21] Linking target qemu-system-x86_64
[21/21] Linking target qemu-system-aarch64
🕙10:23:08 alex@toolbox:qemu.git/builds/virtio-gpu  on  virtio/virtio-blk-fallback [$?]
➜  ./pyvenv/bin/meson test --setup thorough --suite func-thorough func-aarch64-aarch64_virt_gpu -v
ninja: Entering directory `/home/alex/lsrc/qemu.git/builds/virtio-gpu'
[1/6] Generating qemu-version.h with a custom command (wrapped by meson to capture output)
1/1 qemu:func-thorough+func-aarch64-thorough+thorough / func-aarch64-aarch64_virt_gpu        RUNNING
>>> PYTHONPATH=/home/alex/lsrc/qemu.git/python:/home/alex/lsrc/qemu.git/tests/functional QEMU_BUILD_ROOT=/home/alex/lsrc/qemu.git/builds/virtio-gpu ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1 MESON_TEST_ITERATION=1 SPEED=thorough QEMU_TEST_QEMU_BINARY=/home/alex/lsrc/qemu.git/builds/virtio-gpu/qemu-system-aarch64 G_TEST_SLOW=1 UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 MALLOC_PERTURB_=7 MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print_stacktrace=1 RUST_BACKTRACE=1 LD_LIBRARY_PATH=/home/alex/lsrc/qemu.git/builds/virtio-gpu/tests/tcg/plugins:/home/alex/lsrc/qemu.git/builds/virtio-gpu/contrib/plugins:/home/alex/lsrc/qemu.git/builds/extra.libs/install/lib:/home/alex/lsrc/qemu.git/builds/extra.libs/install/lib/x86_64-linux-gnu QEMU_TEST_QEMU_IMG=/home/alex/lsrc/qemu.git/builds/virtio-gpu/qemu-img /home/alex/lsrc/qemu.git/builds/virtio-gpu/pyvenv/bin/python3 /home/alex/lsrc/qemu.git/tests/functional/test_aarch64_virt_gpu.py
qemu:func-thorough+func-aarch64-thorough+thorough / func-aarch64-aarch64_virt_gpu time out (After 480 seconds)
1/1 qemu:func-thorough+func-aarch64-thorough+thorough / func-aarch64-aarch64_virt_gpu        TIMEOUT        480.01s   killed by signal 15 SIGTERM


Summary of Failures:

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

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

Full log written to /home/alex/lsrc/qemu.git/builds/virtio-gpu/meson-logs/testlog-thorough.txt
🕙10:31:12 alex@toolbox:qemu.git/builds/virtio-gpu  on  virtio/virtio-blk-fallback [$?] took 8m [🔴 ERROR]


>
> Manos Pitsidianakis (2):
>   virtio-gpu: fix hang under TCG when unmapping blob
>   virtio-gpu: refactor async blob unmapping
>
>  hw/display/virtio-gpu-virgl.c | 37 ++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
>
>
> base-commit: 0adf626718bc0ca9c46550249a76047f8e45da15

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro