virtio-gpu waits for the main thread to destroy resources and replace surfaces, but it occasionally results in deadlock, so remove the code to wait. In particular, when running a test case[1] the main thread may wait for the vCPUs to pause during shut down while a vCPU may be concurrently resetting virtio-gpu. vCPU actually does not need to perform resource destruction and surface replacement synchronously, but it only needs to ensure correct ordering among virtio-gpu operations. virtio-gpu-gl already exploits this fact to ensure that virglrenderer is reset on the main thread; instead of synchronously resetting virglrenderer when the device is being reset, it resets virglrenderer just before processing the first command after the device reset arrives. Take advantage of this fact by simply removing synchronization between the main thread and the resetting vCPU thread. The request to destroy resources and replace surfaces is scheduled earlier than any virtio-gpu command that may be queued after resetting ensures correct ordering, so we do not need to make additional effort to ensure ordering. [1] https://lore.kernel.org/qemu-devel/20251014111234.3190346-9-alex.bennee@linaro.org/ Fixes: a41e2d97f92b ("virtio-gpu: reset gfx resources in main thread") Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> --- include/hw/virtio/virtio-gpu.h | 2 -- hw/display/virtio-gpu.c | 9 --------- 2 files changed, 11 deletions(-) diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/virtio/virtio-gpu.h +++ b/include/hw/virtio/virtio-gpu.h @@ -XXX,XX +XXX,XX @@ struct VirtIOGPU { QEMUBH *ctrl_bh; QEMUBH *cursor_bh; QEMUBH *reset_bh; - QemuCond reset_cond; - bool reset_finished; QTAILQ_HEAD(, virtio_gpu_simple_resource) reslist; QTAILQ_HEAD(, virtio_gpu_ctrl_command) cmdq; diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index XXXXXXX..XXXXXXX 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -XXX,XX +XXX,XX @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) g->ctrl_bh = virtio_bh_new_guarded(qdev, virtio_gpu_ctrl_bh, g); g->cursor_bh = virtio_bh_new_guarded(qdev, virtio_gpu_cursor_bh, g); g->reset_bh = qemu_bh_new(virtio_gpu_reset_bh, g); - qemu_cond_init(&g->reset_cond); QTAILQ_INIT(&g->reslist); QTAILQ_INIT(&g->cmdq); QTAILQ_INIT(&g->fenceq); @@ -XXX,XX +XXX,XX @@ static void virtio_gpu_device_unrealize(DeviceState *qdev) g_clear_pointer(&g->ctrl_bh, qemu_bh_delete); g_clear_pointer(&g->cursor_bh, qemu_bh_delete); g_clear_pointer(&g->reset_bh, qemu_bh_delete); - qemu_cond_destroy(&g->reset_cond); virtio_gpu_base_device_unrealize(qdev); } @@ -XXX,XX +XXX,XX @@ static void virtio_gpu_reset_bh(void *opaque) for (i = 0; i < g->parent_obj.conf.max_outputs; i++) { dpy_gfx_replace_surface(g->parent_obj.scanout[i].con, NULL); } - - g->reset_finished = true; - qemu_cond_signal(&g->reset_cond); } void virtio_gpu_reset(VirtIODevice *vdev) @@ -XXX,XX +XXX,XX @@ void virtio_gpu_reset(VirtIODevice *vdev) struct virtio_gpu_ctrl_command *cmd; if (qemu_in_vcpu_thread()) { - g->reset_finished = false; qemu_bh_schedule(g->reset_bh); - while (!g->reset_finished) { - qemu_cond_wait_bql(&g->reset_cond); - } } else { aio_bh_call(g->reset_bh); } --- base-commit: 36076d24f04ea9dc3357c0fbe7bb14917375819c change-id: 20251029-gpu-c3f45747f7ba Best regards, -- Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> This fixes a deadlock I previously observed with the test in [1]. However, I can no longer reproduce the issue reliably with that test, so I used Codex, a coding agent, to write a more reliable local test case, shown below. I applied to Codex for Open Source to get access. The test case is not intended for merge: current policy prohibits that, and it is probably not worth carrying anyway because race-condition tests are inherently fragile. The remaining patches were written by me. [1] https://lore.kernel.org/qemu-devel/20251014111234.3190346-9-alex.bennee@linaro.org/ To: qemu-devel@nongnu.org Cc: Alex Bennée <alex.bennee@linaro.org> Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com> Cc: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> Below is the Codex-written test case: diff --git a/tests/functional/aarch64/test_gpu_blob.py b/tests/functional/aarch64/test_gpu_blob.py index XXXXXXX..XXXXXXX 100755 --- a/tests/functional/aarch64/test_gpu_blob.py +++ b/tests/functional/aarch64/test_gpu_blob.py @@ -XXX,XX +XXX,XX @@ # # SPDX-License-Identifier: GPL-2.0-or-later -from qemu.machine.machine import VMLaunchFailure +import subprocess + +from qemu.machine.machine import AbnormalShutdown, VMLaunchFailure from qemu_test import Asset from qemu_test import wait_for_console_pattern @@ -XXX,XX +XXX,XX @@ class Aarch64VirtBlobTest(LinuxKernelTest): 'download?path=%2Fblob-test&files=qemu-880.bin', '2f6ab85d0b156c94fcedd2c4c821c5cbd52925a2de107f8e2d569ea2e34e42eb') - def test_virtio_gpu_blob(self): - + def launch_blob_test(self): self.set_machine('virt') self.require_accelerator("tcg") @@ -XXX,XX +XXX,XX @@ def test_virtio_gpu_blob(self): self.log.info("unhandled launch failure: %s", excp.output) raise excp + def test_virtio_gpu_blob(self): + self.launch_blob_test() + self.wait_for_console_pattern('[INFO] virtio-gpu test finished') # the test should cleanly exit + def test_virtio_gpu_blob_shutdown_race(self): + self.launch_blob_test() + + self.wait_for_console_pattern('[INFO] unmapping blob object resource') + + try: + self.vm.shutdown(timeout=10) + except AbnormalShutdown as excp: + if isinstance(excp.__cause__, subprocess.TimeoutExpired): + raise AssertionError( + "QEMU failed to exit while virtio-gpu reset was racing " + "with shutdown") from excp + self.log.info("QEMU exited before the shutdown request completed: %s", + excp) + if __name__ == '__main__': LinuxKernelTest.main() --- Changes in v2: - Added the patch "virtio-gpu: Run reset cleanup in the same BH". - My assumption about the ordering was incorrect, so I changed the patch to follow the approach used by virtio-gpu-gl. - Link to v1: https://lore.kernel.org/qemu-devel/20251029-gpu-v1-1-e3e3c7eebc9e@rsg.ci.i.u-tokyo.ac.jp --- Akihiko Odaki (2): virtio-gpu: Run reset cleanup in the same BH virtio-gpu: Do not wait for the main thread during reset include/hw/virtio/virtio-gpu.h | 4 +-- hw/display/virtio-gpu.c | 60 ++++++++++++++++++++---------------------- 2 files changed, 30 insertions(+), 34 deletions(-) --- base-commit: 14f38a63b9adc02c0ebe3b5ada1f1208abaf21ea change-id: 20251029-gpu-c3f45747f7ba Best regards, -- Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>