diff mbox series

[PULL,23/29] ui & main loop: Redesign of system-specific main thread event handling

Message ID 20241231202228.28819-24-philmd@linaro.org
State New
Headers show
Series [PULL,01/29] hw/pci-host/gpex: Allow more than 4 legacy IRQs | expand

Commit Message

Philippe Mathieu-Daudé Dec. 31, 2024, 8:22 p.m. UTC 
From: Phil Dennis-Jordan <phil@philjordan.eu>

macOS's Cocoa event handling must be done on the initial (main) thread
of the process. Furthermore, if library or application code uses
libdispatch, the main dispatch queue must be handling events on the main
thread as well.

So far, this has affected Qemu in both the Cocoa and SDL UIs, although
in different ways: the Cocoa UI replaces the default qemu_main function
with one that spins Qemu's internal main event loop off onto a
background thread. SDL (which uses Cocoa internally) on the other hand
uses a polling approach within Qemu's main event loop. Events are
polled during the SDL UI's dpy_refresh callback, which happens to run
on the main thread by default.

As UIs are mutually exclusive, this works OK as long as nothing else
needs platform-native event handling. In the next patch, a new device is
introduced based on the ParavirtualizedGraphics.framework in macOS.
This uses libdispatch internally, and only works when events are being
handled on the main runloop. With the current system, it works when
using either the Cocoa or the SDL UI. However, it does not when running
headless. Moreover, any attempt to install a similar scheme to the
Cocoa UI's main thread replacement fails when combined with the SDL
UI.

This change tidies up main thread management to be more flexible.

 * The qemu_main global function pointer is a custom function for the
   main thread, and it may now be NULL. When it is, the main thread
   runs the main Qemu loop. This represents the traditional setup.
 * When non-null, spawning the main Qemu event loop on a separate
   thread is now done centrally rather than inside the Cocoa UI code.
 * For most platforms, qemu_main is indeed NULL by default, but on
   Darwin, it defaults to a function that runs the CFRunLoop.
 * The Cocoa UI sets qemu_main to a function which runs the
   NSApplication event handling runloop, as is usual for a Cocoa app.
 * The SDL UI overrides the qemu_main function to NULL, thus
   specifying that Qemu's main loop must run on the main
   thread.
 * The GTK UI also overrides the qemu_main function to NULL.
 * For other UIs, or in the absence of UIs, the platform's default
   behaviour is followed.

This means that on macOS, the platform's runloop events are always
handled, regardless of chosen UI. The new PV graphics device will
thus work in all configurations. There is no functional change on other
operating systems.

Implementing this via a global function pointer variable is a bit
ugly, but it's probably worth investigating the existing UI thread rule
violations in the SDL (e.g. #2537) and GTK+ back-ends. Fixing those
issues might precipitate requirements similar but not identical to those
of the Cocoa UI; hopefully we'll see some kind of pattern emerge, which
can then be used as a basis for an overhaul. (In fact, it may turn
out to be simplest to split the UI/native platform event thread from the
QEMU main event loop on all platforms, with any UI or even none at all.)

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Tested-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Message-ID: <20241223221645.29911-2-phil@philjordan.eu>
[PMD: Declare 'qemu_main' symbol in tests/qtest/fuzz/fuzz.c,
      add missing g_assert_not_reached() call in main()]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/qemu-main.h     | 14 ++++++++++-
 system/main.c           | 38 ++++++++++++++++++++++++++---
 tests/qtest/fuzz/fuzz.c |  1 +
 ui/gtk.c                |  4 +++
 ui/sdl2.c               |  4 +++
 ui/cocoa.m              | 54 ++++++++++-------------------------------
 6 files changed, 69 insertions(+), 46 deletions(-)

Comments

David Woodhouse Jan. 8, 2025, 1:51 p.m. UTC | #1 
On Tue, 2024-12-31 at 21:22 +0100, Philippe Mathieu-Daudé wrote:
> 
> This change tidies up main thread management to be more flexible.
> 
>  * The qemu_main global function pointer is a custom function for the
>    main thread, and it may now be NULL. When it is, the main thread
>    runs the main Qemu loop. This represents the traditional setup.
>  * When non-null, spawning the main Qemu event loop on a separate
>    thread is now done centrally rather than inside the Cocoa UI code.
>  * For most platforms, qemu_main is indeed NULL by default, but on
>    Darwin, it defaults to a function that runs the CFRunLoop.
>  * The Cocoa UI sets qemu_main to a function which runs the
>    NSApplication event handling runloop, as is usual for a Cocoa app.
>  * The SDL UI overrides the qemu_main function to NULL, thus
>    specifying that Qemu's main loop must run on the main
>    thread.
>  * The GTK UI also overrides the qemu_main function to NULL.
>  * For other UIs, or in the absence of UIs, the platform's default
>    behaviour is followed.

When exiting an emulated Xen guest with <Ctrl-a x> on the console, I
now see:

(gdb) run
Starting program: /home/dwmw2/git/qemu/ball/qemu-system-x86_64 -display none -vga none -serial mon:stdio -machine q35 -accel kvm,xen-version=0x4000a,kernel-irqchip=split -smp 2 -kernel /home/dwmw2/avocado/data/cache/by_name/bzImage -append printk.time=0\ root=/dev/xvda\ console=ttyS0\ xen_emul_unplug=ide-disks\ xen_no_vector_callback\ noapic\ loglevel=0 -drive file=/home/dwmw2/avocado/data/cache/by_name/rootfs.ext4,if=none,snapshot=on,format=raw,id=drv0 -device xen-disk,drive=drv0,vdev=xvda -device virtio-net-pci,netdev=unet -netdev user,id=unet,hostfwd=:127.0.0.1:0-:22

Starting syslogd: OK
Starting klogd: OK
Running sysctl: OK
Saving 256 bits of non-creditable seed for next boot
Starting network: OK
Starting dhcpcd...
no such user dhcpcd
dhcpcd-9.4.1 starting
no interfaces have a carrier
forked to background, child pid 111
Starting dropbear sshd: OK
# 
QEMU: Terminated
qemu-system-x86_64: ../block/block-backend.c:1290: blk_in_drain: Assertion `qemu_in_main_thread()' failed.

Thread 1 "qemu-system-x86" received signal SIGABRT, Aborted.
0x00007ffff5ffc724 in __pthread_kill_implementation () from /lib64/libc.so.6
(gdb) bt
#0  0x00007ffff5ffc724 in __pthread_kill_implementation () at /lib64/libc.so.6
#1  0x00007ffff5fa3d1e in raise () at /lib64/libc.so.6
#2  0x00007ffff5f8b942 in abort () at /lib64/libc.so.6
#3  0x00007ffff5f8b85e in __assert_fail_base.cold () at /lib64/libc.so.6
#4  0x00007ffff5f9be47 in __assert_fail () at /lib64/libc.so.6
#5  0x0000555555abf911 in blk_in_drain (blk=0x555557ca6680)
    at ../block/block-backend.c:1290
#6  0x000055555593410c in xen_block_dataplane_stop (dataplane=0x555558982950)
    at ../hw/block/dataplane/xen-block.c:695
#7  0x000055555593441a in xen_block_dataplane_stop (dataplane=<optimized out>)
    at ../hw/block/dataplane/xen-block.c:689
#8  0x00005555555dfd5e in xen_block_disconnect
    (xendev=xendev@entry=0x55555880aa60, errp=<optimized out>)
    at ../hw/block/xen-block.c:172
#9  0x00005555555dfeca in xen_block_unrealize (xendev=0x55555880aa60)
    at ../hw/block/xen-block.c:282
#10 0x000055555578ef71 in xen_device_unrealize (dev=<optimized out>)
    at ../hw/xen/xen-bus.c:978
#11 0x0000555555bfe65f in notifier_list_notify (list=<optimized out>, data=0x0)
    at ../util/notify.c:39
#12 0x00007ffff5fa6461 in __run_exit_handlers () at /lib64/libc.so.6
#13 0x00007ffff5fa652e in exit () at /lib64/libc.so.6
#14 0x0000555555b523dc in qemu_default_main (opaque=opaque@entry=0x0)
#15 0x00005555555609c0 in main (argc=<optimized out>, argv=<optimized out>) at ../system/main.c:76
Philippe Mathieu-Daudé April 1, 2025, 11:18 a.m. UTC | #2 
Hi David,

On 8/1/25 14:51, David Woodhouse wrote:
> On Tue, 2024-12-31 at 21:22 +0100, Philippe Mathieu-Daudé wrote:
>>
>> This change tidies up main thread management to be more flexible.
>>
>>   * The qemu_main global function pointer is a custom function for the
>>     main thread, and it may now be NULL. When it is, the main thread
>>     runs the main Qemu loop. This represents the traditional setup.
>>   * When non-null, spawning the main Qemu event loop on a separate
>>     thread is now done centrally rather than inside the Cocoa UI code.
>>   * For most platforms, qemu_main is indeed NULL by default, but on
>>     Darwin, it defaults to a function that runs the CFRunLoop.
>>   * The Cocoa UI sets qemu_main to a function which runs the
>>     NSApplication event handling runloop, as is usual for a Cocoa app.
>>   * The SDL UI overrides the qemu_main function to NULL, thus
>>     specifying that Qemu's main loop must run on the main
>>     thread.
>>   * The GTK UI also overrides the qemu_main function to NULL.
>>   * For other UIs, or in the absence of UIs, the platform's default
>>     behaviour is followed.
> 
> When exiting an emulated Xen guest with <Ctrl-a x> on the console, I
> now see:
> 
> (gdb) run
> Starting program: /home/dwmw2/git/qemu/ball/qemu-system-x86_64 -display none -vga none -serial mon:stdio -machine q35 -accel kvm,xen-version=0x4000a,kernel-irqchip=split -smp 2 -kernel /home/dwmw2/avocado/data/cache/by_name/bzImage -append printk.time=0\ root=/dev/xvda\ console=ttyS0\ xen_emul_unplug=ide-disks\ xen_no_vector_callback\ noapic\ loglevel=0 -drive file=/home/dwmw2/avocado/data/cache/by_name/rootfs.ext4,if=none,snapshot=on,format=raw,id=drv0 -device xen-disk,drive=drv0,vdev=xvda -device virtio-net-pci,netdev=unet -netdev user,id=unet,hostfwd=:127.0.0.1:0-:22
> 
> Starting syslogd: OK
> Starting klogd: OK
> Running sysctl: OK
> Saving 256 bits of non-creditable seed for next boot
> Starting network: OK
> Starting dhcpcd...
> no such user dhcpcd
> dhcpcd-9.4.1 starting
> no interfaces have a carrier
> forked to background, child pid 111
> Starting dropbear sshd: OK
> #
> QEMU: Terminated
> qemu-system-x86_64: ../block/block-backend.c:1290: blk_in_drain: Assertion `qemu_in_main_thread()' failed.
> 
> Thread 1 "qemu-system-x86" received signal SIGABRT, Aborted.
> 0x00007ffff5ffc724 in __pthread_kill_implementation () from /lib64/libc.so.6
> (gdb) bt
> #0  0x00007ffff5ffc724 in __pthread_kill_implementation () at /lib64/libc.so.6
> #1  0x00007ffff5fa3d1e in raise () at /lib64/libc.so.6
> #2  0x00007ffff5f8b942 in abort () at /lib64/libc.so.6
> #3  0x00007ffff5f8b85e in __assert_fail_base.cold () at /lib64/libc.so.6
> #4  0x00007ffff5f9be47 in __assert_fail () at /lib64/libc.so.6
> #5  0x0000555555abf911 in blk_in_drain (blk=0x555557ca6680)
>      at ../block/block-backend.c:1290
> #6  0x000055555593410c in xen_block_dataplane_stop (dataplane=0x555558982950)
>      at ../hw/block/dataplane/xen-block.c:695
> #7  0x000055555593441a in xen_block_dataplane_stop (dataplane=<optimized out>)
>      at ../hw/block/dataplane/xen-block.c:689
> #8  0x00005555555dfd5e in xen_block_disconnect
>      (xendev=xendev@entry=0x55555880aa60, errp=<optimized out>)
>      at ../hw/block/xen-block.c:172
> #9  0x00005555555dfeca in xen_block_unrealize (xendev=0x55555880aa60)
>      at ../hw/block/xen-block.c:282
> #10 0x000055555578ef71 in xen_device_unrealize (dev=<optimized out>)
>      at ../hw/xen/xen-bus.c:978
> #11 0x0000555555bfe65f in notifier_list_notify (list=<optimized out>, data=0x0)
>      at ../util/notify.c:39
> #12 0x00007ffff5fa6461 in __run_exit_handlers () at /lib64/libc.so.6
> #13 0x00007ffff5fa652e in exit () at /lib64/libc.so.6
> #14 0x0000555555b523dc in qemu_default_main (opaque=opaque@entry=0x0)
> #15 0x00005555555609c0 in main (argc=<optimized out>, argv=<optimized out>) at ../system/main.c:76
> 

Is this still an issue?
Philippe Mathieu-Daudé April 1, 2025, 11:30 a.m. UTC | #3 
On 1/4/25 13:18, Philippe Mathieu-Daudé wrote:
> Hi David,
> 
> On 8/1/25 14:51, David Woodhouse wrote:
>> On Tue, 2024-12-31 at 21:22 +0100, Philippe Mathieu-Daudé wrote:
>>>
>>> This change tidies up main thread management to be more flexible.
>>>
>>>   * The qemu_main global function pointer is a custom function for the
>>>     main thread, and it may now be NULL. When it is, the main thread
>>>     runs the main Qemu loop. This represents the traditional setup.
>>>   * When non-null, spawning the main Qemu event loop on a separate
>>>     thread is now done centrally rather than inside the Cocoa UI code.
>>>   * For most platforms, qemu_main is indeed NULL by default, but on
>>>     Darwin, it defaults to a function that runs the CFRunLoop.
>>>   * The Cocoa UI sets qemu_main to a function which runs the
>>>     NSApplication event handling runloop, as is usual for a Cocoa app.
>>>   * The SDL UI overrides the qemu_main function to NULL, thus
>>>     specifying that Qemu's main loop must run on the main
>>>     thread.
>>>   * The GTK UI also overrides the qemu_main function to NULL.
>>>   * For other UIs, or in the absence of UIs, the platform's default
>>>     behaviour is followed.
>>
>> When exiting an emulated Xen guest with <Ctrl-a x> on the console, I
>> now see:
>>
>> (gdb) run
>> Starting program: /home/dwmw2/git/qemu/ball/qemu-system-x86_64 - 
>> display none -vga none -serial mon:stdio -machine q35 -accel kvm,xen- 
>> version=0x4000a,kernel-irqchip=split -smp 2 -kernel /home/dwmw2/ 
>> avocado/data/cache/by_name/bzImage -append printk.time=0\ root=/dev/ 
>> xvda\ console=ttyS0\ xen_emul_unplug=ide-disks\ 
>> xen_no_vector_callback\ noapic\ loglevel=0 -drive file=/home/dwmw2/ 
>> avocado/data/cache/by_name/ 
>> rootfs.ext4,if=none,snapshot=on,format=raw,id=drv0 -device xen- 
>> disk,drive=drv0,vdev=xvda -device virtio-net-pci,netdev=unet -netdev 
>> user,id=unet,hostfwd=:127.0.0.1:0-:22
>>
>> Starting syslogd: OK
>> Starting klogd: OK
>> Running sysctl: OK
>> Saving 256 bits of non-creditable seed for next boot
>> Starting network: OK
>> Starting dhcpcd...
>> no such user dhcpcd
>> dhcpcd-9.4.1 starting
>> no interfaces have a carrier
>> forked to background, child pid 111
>> Starting dropbear sshd: OK
>> #
>> QEMU: Terminated
>> qemu-system-x86_64: ../block/block-backend.c:1290: blk_in_drain: 
>> Assertion `qemu_in_main_thread()' failed.
>>
>> Thread 1 "qemu-system-x86" received signal SIGABRT, Aborted.
>> 0x00007ffff5ffc724 in __pthread_kill_implementation () from /lib64/ 
>> libc.so.6
>> (gdb) bt
>> #0  0x00007ffff5ffc724 in __pthread_kill_implementation () at /lib64/ 
>> libc.so.6
>> #1  0x00007ffff5fa3d1e in raise () at /lib64/libc.so.6
>> #2  0x00007ffff5f8b942 in abort () at /lib64/libc.so.6
>> #3  0x00007ffff5f8b85e in __assert_fail_base.cold () at /lib64/libc.so.6
>> #4  0x00007ffff5f9be47 in __assert_fail () at /lib64/libc.so.6
>> #5  0x0000555555abf911 in blk_in_drain (blk=0x555557ca6680)
>>      at ../block/block-backend.c:1290
>> #6  0x000055555593410c in xen_block_dataplane_stop 
>> (dataplane=0x555558982950)
>>      at ../hw/block/dataplane/xen-block.c:695
>> #7  0x000055555593441a in xen_block_dataplane_stop 
>> (dataplane=<optimized out>)
>>      at ../hw/block/dataplane/xen-block.c:689
>> #8  0x00005555555dfd5e in xen_block_disconnect
>>      (xendev=xendev@entry=0x55555880aa60, errp=<optimized out>)
>>      at ../hw/block/xen-block.c:172
>> #9  0x00005555555dfeca in xen_block_unrealize (xendev=0x55555880aa60)
>>      at ../hw/block/xen-block.c:282
>> #10 0x000055555578ef71 in xen_device_unrealize (dev=<optimized out>)
>>      at ../hw/xen/xen-bus.c:978
>> #11 0x0000555555bfe65f in notifier_list_notify (list=<optimized out>, 
>> data=0x0)
>>      at ../util/notify.c:39
>> #12 0x00007ffff5fa6461 in __run_exit_handlers () at /lib64/libc.so.6
>> #13 0x00007ffff5fa652e in exit () at /lib64/libc.so.6
>> #14 0x0000555555b523dc in qemu_default_main (opaque=opaque@entry=0x0)
>> #15 0x00005555555609c0 in main (argc=<optimized out>, argv=<optimized 
>> out>) at ../system/main.c:76
>>
> 
> Is this still an issue?

Likely fixed by commit e7bc0204 ("system/runstate:
Fix regression, clarify BQL status of exit notifiers"), so
I'm closing https://gitlab.com/qemu-project/qemu/-/issues/2771.
David Woodhouse April 1, 2025, 11:43 a.m. UTC | #4 
On Tue, 2025-04-01 at 13:30 +0200, Philippe Mathieu-Daudé wrote:
> On 1/4/25 13:18, Philippe Mathieu-Daudé wrote:
> > Hi David,
> > 
> > On 8/1/25 14:51, David Woodhouse wrote:
> > > On Tue, 2024-12-31 at 21:22 +0100, Philippe Mathieu-Daudé wrote:
> > > > 
> > > > This change tidies up main thread management to be more
> > > > flexible.
> > > > 
> > > >   * The qemu_main global function pointer is a custom function
> > > > for the
> > > >     main thread, and it may now be NULL. When it is, the main
> > > > thread
> > > >     runs the main Qemu loop. This represents the traditional
> > > > setup.
> > > >   * When non-null, spawning the main Qemu event loop on a
> > > > separate
> > > >     thread is now done centrally rather than inside the Cocoa
> > > > UI code.
> > > >   * For most platforms, qemu_main is indeed NULL by default,
> > > > but on
> > > >     Darwin, it defaults to a function that runs the CFRunLoop.
> > > >   * The Cocoa UI sets qemu_main to a function which runs the
> > > >     NSApplication event handling runloop, as is usual for a
> > > > Cocoa app.
> > > >   * The SDL UI overrides the qemu_main function to NULL, thus
> > > >     specifying that Qemu's main loop must run on the main
> > > >     thread.
> > > >   * The GTK UI also overrides the qemu_main function to NULL.
> > > >   * For other UIs, or in the absence of UIs, the platform's
> > > > default
> > > >     behaviour is followed.
> > > 
> > > When exiting an emulated Xen guest with <Ctrl-a x> on the
> > > console, I
> > > now see:
> > > 
> > > (gdb) run
> > > Starting program: /home/dwmw2/git/qemu/ball/qemu-system-x86_64 - 
> > > display none -vga none -serial mon:stdio -machine q35 -accel
> > > kvm,xen- 
> > > version=0x4000a,kernel-irqchip=split -smp 2 -kernel /home/dwmw2/ 
> > > avocado/data/cache/by_name/bzImage -append printk.time=0\
> > > root=/dev/ 
> > > xvda\ console=ttyS0\ xen_emul_unplug=ide-disks\ 
> > > xen_no_vector_callback\ noapic\ loglevel=0 -drive
> > > file=/home/dwmw2/ 
> > > avocado/data/cache/by_name/ 
> > > rootfs.ext4,if=none,snapshot=on,format=raw,id=drv0 -device xen- 
> > > disk,drive=drv0,vdev=xvda -device virtio-net-pci,netdev=unet -
> > > netdev 
> > > user,id=unet,hostfwd=:127.0.0.1:0-:22
> > > 
> > > Starting syslogd: OK
> > > Starting klogd: OK
> > > Running sysctl: OK
> > > Saving 256 bits of non-creditable seed for next boot
> > > Starting network: OK
> > > Starting dhcpcd...
> > > no such user dhcpcd
> > > dhcpcd-9.4.1 starting
> > > no interfaces have a carrier
> > > forked to background, child pid 111
> > > Starting dropbear sshd: OK
> > > #
> > > QEMU: Terminated
> > > qemu-system-x86_64: ../block/block-backend.c:1290: blk_in_drain: 
> > > Assertion `qemu_in_main_thread()' failed.
> > > 
> > > Thread 1 "qemu-system-x86" received signal SIGABRT, Aborted.
> > > 0x00007ffff5ffc724 in __pthread_kill_implementation () from
> > > /lib64/ 
> > > libc.so.6
> > > (gdb) bt
> > > #0  0x00007ffff5ffc724 in __pthread_kill_implementation () at
> > > /lib64/ 
> > > libc.so.6
> > > #1  0x00007ffff5fa3d1e in raise () at /lib64/libc.so.6
> > > #2  0x00007ffff5f8b942 in abort () at /lib64/libc.so.6
> > > #3  0x00007ffff5f8b85e in __assert_fail_base.cold () at
> > > /lib64/libc.so.6
> > > #4  0x00007ffff5f9be47 in __assert_fail () at /lib64/libc.so.6
> > > #5  0x0000555555abf911 in blk_in_drain (blk=0x555557ca6680)
> > >      at ../block/block-backend.c:1290
> > > #6  0x000055555593410c in xen_block_dataplane_stop 
> > > (dataplane=0x555558982950)
> > >      at ../hw/block/dataplane/xen-block.c:695
> > > #7  0x000055555593441a in xen_block_dataplane_stop 
> > > (dataplane=<optimized out>)
> > >      at ../hw/block/dataplane/xen-block.c:689
> > > #8  0x00005555555dfd5e in xen_block_disconnect
> > >      (xendev=xendev@entry=0x55555880aa60, errp=<optimized out>)
> > >      at ../hw/block/xen-block.c:172
> > > #9  0x00005555555dfeca in xen_block_unrealize
> > > (xendev=0x55555880aa60)
> > >      at ../hw/block/xen-block.c:282
> > > #10 0x000055555578ef71 in xen_device_unrealize (dev=<optimized
> > > out>)
> > >      at ../hw/xen/xen-bus.c:978
> > > #11 0x0000555555bfe65f in notifier_list_notify (list=<optimized
> > > out>, 
> > > data=0x0)
> > >      at ../util/notify.c:39
> > > #12 0x00007ffff5fa6461 in __run_exit_handlers () at
> > > /lib64/libc.so.6
> > > #13 0x00007ffff5fa652e in exit () at /lib64/libc.so.6
> > > #14 0x0000555555b523dc in qemu_default_main
> > > (opaque=opaque@entry=0x0)
> > > #15 0x00005555555609c0 in main (argc=<optimized out>,
> > > argv=<optimized 
> > > out>) at ../system/main.c:76
> > > 
> > 
> > Is this still an issue?
> 
> Likely fixed by commit e7bc0204 ("system/runstate:
> Fix regression, clarify BQL status of exit notifiers"), so
> I'm closing https://gitlab.com/qemu-project/qemu/-/issues/2771.

Indeed. I think I was assuming the explicit Resolves: tag in that
commit was going to magically close the issue when it was merged.
Should I have closed it manually?
Philippe Mathieu-Daudé April 1, 2025, 11:58 a.m. UTC | #5 
On 1/4/25 13:43, David Woodhouse wrote:
> On Tue, 2025-04-01 at 13:30 +0200, Philippe Mathieu-Daudé wrote:
>> On 1/4/25 13:18, Philippe Mathieu-Daudé wrote:
>>> Hi David,
>>>
>>> On 8/1/25 14:51, David Woodhouse wrote:
>>>> On Tue, 2024-12-31 at 21:22 +0100, Philippe Mathieu-Daudé wrote:
>>>>>
>>>>> This change tidies up main thread management to be more
>>>>> flexible.
>>>>>
>>>>>    * The qemu_main global function pointer is a custom function
>>>>> for the
>>>>>      main thread, and it may now be NULL. When it is, the main
>>>>> thread
>>>>>      runs the main Qemu loop. This represents the traditional
>>>>> setup.
>>>>>    * When non-null, spawning the main Qemu event loop on a
>>>>> separate
>>>>>      thread is now done centrally rather than inside the Cocoa
>>>>> UI code.
>>>>>    * For most platforms, qemu_main is indeed NULL by default,
>>>>> but on
>>>>>      Darwin, it defaults to a function that runs the CFRunLoop.
>>>>>    * The Cocoa UI sets qemu_main to a function which runs the
>>>>>      NSApplication event handling runloop, as is usual for a
>>>>> Cocoa app.
>>>>>    * The SDL UI overrides the qemu_main function to NULL, thus
>>>>>      specifying that Qemu's main loop must run on the main
>>>>>      thread.
>>>>>    * The GTK UI also overrides the qemu_main function to NULL.
>>>>>    * For other UIs, or in the absence of UIs, the platform's
>>>>> default
>>>>>      behaviour is followed.
>>>>
>>>> When exiting an emulated Xen guest with <Ctrl-a x> on the
>>>> console, I
>>>> now see:
>>>>
>>>> (gdb) run
>>>> Starting program: /home/dwmw2/git/qemu/ball/qemu-system-x86_64 -
>>>> display none -vga none -serial mon:stdio -machine q35 -accel
>>>> kvm,xen-
>>>> version=0x4000a,kernel-irqchip=split -smp 2 -kernel /home/dwmw2/
>>>> avocado/data/cache/by_name/bzImage -append printk.time=0\
>>>> root=/dev/
>>>> xvda\ console=ttyS0\ xen_emul_unplug=ide-disks\
>>>> xen_no_vector_callback\ noapic\ loglevel=0 -drive
>>>> file=/home/dwmw2/
>>>> avocado/data/cache/by_name/
>>>> rootfs.ext4,if=none,snapshot=on,format=raw,id=drv0 -device xen-
>>>> disk,drive=drv0,vdev=xvda -device virtio-net-pci,netdev=unet -
>>>> netdev
>>>> user,id=unet,hostfwd=:127.0.0.1:0-:22
>>>>
>>>> Starting syslogd: OK
>>>> Starting klogd: OK
>>>> Running sysctl: OK
>>>> Saving 256 bits of non-creditable seed for next boot
>>>> Starting network: OK
>>>> Starting dhcpcd...
>>>> no such user dhcpcd
>>>> dhcpcd-9.4.1 starting
>>>> no interfaces have a carrier
>>>> forked to background, child pid 111
>>>> Starting dropbear sshd: OK
>>>> #
>>>> QEMU: Terminated
>>>> qemu-system-x86_64: ../block/block-backend.c:1290: blk_in_drain:
>>>> Assertion `qemu_in_main_thread()' failed.
>>>>
>>>> Thread 1 "qemu-system-x86" received signal SIGABRT, Aborted.
>>>> 0x00007ffff5ffc724 in __pthread_kill_implementation () from
>>>> /lib64/
>>>> libc.so.6
>>>> (gdb) bt
>>>> #0  0x00007ffff5ffc724 in __pthread_kill_implementation () at
>>>> /lib64/
>>>> libc.so.6
>>>> #1  0x00007ffff5fa3d1e in raise () at /lib64/libc.so.6
>>>> #2  0x00007ffff5f8b942 in abort () at /lib64/libc.so.6
>>>> #3  0x00007ffff5f8b85e in __assert_fail_base.cold () at
>>>> /lib64/libc.so.6
>>>> #4  0x00007ffff5f9be47 in __assert_fail () at /lib64/libc.so.6
>>>> #5  0x0000555555abf911 in blk_in_drain (blk=0x555557ca6680)
>>>>       at ../block/block-backend.c:1290
>>>> #6  0x000055555593410c in xen_block_dataplane_stop
>>>> (dataplane=0x555558982950)
>>>>       at ../hw/block/dataplane/xen-block.c:695
>>>> #7  0x000055555593441a in xen_block_dataplane_stop
>>>> (dataplane=<optimized out>)
>>>>       at ../hw/block/dataplane/xen-block.c:689
>>>> #8  0x00005555555dfd5e in xen_block_disconnect
>>>>       (xendev=xendev@entry=0x55555880aa60, errp=<optimized out>)
>>>>       at ../hw/block/xen-block.c:172
>>>> #9  0x00005555555dfeca in xen_block_unrealize
>>>> (xendev=0x55555880aa60)
>>>>       at ../hw/block/xen-block.c:282
>>>> #10 0x000055555578ef71 in xen_device_unrealize (dev=<optimized
>>>> out>)
>>>>       at ../hw/xen/xen-bus.c:978
>>>> #11 0x0000555555bfe65f in notifier_list_notify (list=<optimized
>>>> out>,
>>>> data=0x0)
>>>>       at ../util/notify.c:39
>>>> #12 0x00007ffff5fa6461 in __run_exit_handlers () at
>>>> /lib64/libc.so.6
>>>> #13 0x00007ffff5fa652e in exit () at /lib64/libc.so.6
>>>> #14 0x0000555555b523dc in qemu_default_main
>>>> (opaque=opaque@entry=0x0)
>>>> #15 0x00005555555609c0 in main (argc=<optimized out>,
>>>> argv=<optimized
>>>> out>) at ../system/main.c:76
>>>>
>>>
>>> Is this still an issue?
>>
>> Likely fixed by commit e7bc0204 ("system/runstate:
>> Fix regression, clarify BQL status of exit notifiers"), so
>> I'm closing https://gitlab.com/qemu-project/qemu/-/issues/2771.
> 
> Indeed. I think I was assuming the explicit Resolves: tag in that
> commit was going to magically close the issue when it was merged.
> Should I have closed it manually?

It should have been closed automatically. Maybe some transient
problem on GitLab side ¯\_(ツ)_/¯
David Woodhouse April 1, 2025, noon UTC | #6 
On Tue, 2025-04-01 at 13:58 +0200, Philippe Mathieu-Daudé wrote:
> On 1/4/25 13:43, David Woodhouse wrote:
> > On Tue, 2025-04-01 at 13:30 +0200, Philippe Mathieu-Daudé wrote:
> > > On 1/4/25 13:18, Philippe Mathieu-Daudé wrote:
> > > > Hi David,
> > > > 
> > > > On 8/1/25 14:51, David Woodhouse wrote:
> > > > > On Tue, 2024-12-31 at 21:22 +0100, Philippe Mathieu-Daudé
> > > > > wrote:
> > > > > > 
> > > > > > This change tidies up main thread management to be more
> > > > > > flexible.
> > > > > > 
> > > > > >    * The qemu_main global function pointer is a custom
> > > > > > function
> > > > > > for the
> > > > > >      main thread, and it may now be NULL. When it is, the
> > > > > > main
> > > > > > thread
> > > > > >      runs the main Qemu loop. This represents the
> > > > > > traditional
> > > > > > setup.
> > > > > >    * When non-null, spawning the main Qemu event loop on a
> > > > > > separate
> > > > > >      thread is now done centrally rather than inside the
> > > > > > Cocoa
> > > > > > UI code.
> > > > > >    * For most platforms, qemu_main is indeed NULL by
> > > > > > default,
> > > > > > but on
> > > > > >      Darwin, it defaults to a function that runs the
> > > > > > CFRunLoop.
> > > > > >    * The Cocoa UI sets qemu_main to a function which runs
> > > > > > the
> > > > > >      NSApplication event handling runloop, as is usual for
> > > > > > a
> > > > > > Cocoa app.
> > > > > >    * The SDL UI overrides the qemu_main function to NULL,
> > > > > > thus
> > > > > >      specifying that Qemu's main loop must run on the main
> > > > > >      thread.
> > > > > >    * The GTK UI also overrides the qemu_main function to
> > > > > > NULL.
> > > > > >    * For other UIs, or in the absence of UIs, the
> > > > > > platform's
> > > > > > default
> > > > > >      behaviour is followed.
> > > > > 
> > > > > When exiting an emulated Xen guest with <Ctrl-a x> on the
> > > > > console, I
> > > > > now see:
> > > > > 
> > > > > (gdb) run
> > > > > Starting program: /home/dwmw2/git/qemu/ball/qemu-system-
> > > > > x86_64 -
> > > > > display none -vga none -serial mon:stdio -machine q35 -accel
> > > > > kvm,xen-
> > > > > version=0x4000a,kernel-irqchip=split -smp 2 -kernel
> > > > > /home/dwmw2/
> > > > > avocado/data/cache/by_name/bzImage -append printk.time=0\
> > > > > root=/dev/
> > > > > xvda\ console=ttyS0\ xen_emul_unplug=ide-disks\
> > > > > xen_no_vector_callback\ noapic\ loglevel=0 -drive
> > > > > file=/home/dwmw2/
> > > > > avocado/data/cache/by_name/
> > > > > rootfs.ext4,if=none,snapshot=on,format=raw,id=drv0 -device
> > > > > xen-
> > > > > disk,drive=drv0,vdev=xvda -device virtio-net-pci,netdev=unet
> > > > > -
> > > > > netdev
> > > > > user,id=unet,hostfwd=:127.0.0.1:0-:22
> > > > > 
> > > > > Starting syslogd: OK
> > > > > Starting klogd: OK
> > > > > Running sysctl: OK
> > > > > Saving 256 bits of non-creditable seed for next boot
> > > > > Starting network: OK
> > > > > Starting dhcpcd...
> > > > > no such user dhcpcd
> > > > > dhcpcd-9.4.1 starting
> > > > > no interfaces have a carrier
> > > > > forked to background, child pid 111
> > > > > Starting dropbear sshd: OK
> > > > > #
> > > > > QEMU: Terminated
> > > > > qemu-system-x86_64: ../block/block-backend.c:1290:
> > > > > blk_in_drain:
> > > > > Assertion `qemu_in_main_thread()' failed.
> > > > > 
> > > > > Thread 1 "qemu-system-x86" received signal SIGABRT, Aborted.
> > > > > 0x00007ffff5ffc724 in __pthread_kill_implementation () from
> > > > > /lib64/
> > > > > libc.so.6
> > > > > (gdb) bt
> > > > > #0  0x00007ffff5ffc724 in __pthread_kill_implementation () at
> > > > > /lib64/
> > > > > libc.so.6
> > > > > #1  0x00007ffff5fa3d1e in raise () at /lib64/libc.so.6
> > > > > #2  0x00007ffff5f8b942 in abort () at /lib64/libc.so.6
> > > > > #3  0x00007ffff5f8b85e in __assert_fail_base.cold () at
> > > > > /lib64/libc.so.6
> > > > > #4  0x00007ffff5f9be47 in __assert_fail () at
> > > > > /lib64/libc.so.6
> > > > > #5  0x0000555555abf911 in blk_in_drain (blk=0x555557ca6680)
> > > > >       at ../block/block-backend.c:1290
> > > > > #6  0x000055555593410c in xen_block_dataplane_stop
> > > > > (dataplane=0x555558982950)
> > > > >       at ../hw/block/dataplane/xen-block.c:695
> > > > > #7  0x000055555593441a in xen_block_dataplane_stop
> > > > > (dataplane=<optimized out>)
> > > > >       at ../hw/block/dataplane/xen-block.c:689
> > > > > #8  0x00005555555dfd5e in xen_block_disconnect
> > > > >       (xendev=xendev@entry=0x55555880aa60, errp=<optimized
> > > > > out>)
> > > > >       at ../hw/block/xen-block.c:172
> > > > > #9  0x00005555555dfeca in xen_block_unrealize
> > > > > (xendev=0x55555880aa60)
> > > > >       at ../hw/block/xen-block.c:282
> > > > > #10 0x000055555578ef71 in xen_device_unrealize
> > > > > (dev=<optimized
> > > > > out>)
> > > > >       at ../hw/xen/xen-bus.c:978
> > > > > #11 0x0000555555bfe65f in notifier_list_notify
> > > > > (list=<optimized
> > > > > out>,
> > > > > data=0x0)
> > > > >       at ../util/notify.c:39
> > > > > #12 0x00007ffff5fa6461 in __run_exit_handlers () at
> > > > > /lib64/libc.so.6
> > > > > #13 0x00007ffff5fa652e in exit () at /lib64/libc.so.6
> > > > > #14 0x0000555555b523dc in qemu_default_main
> > > > > (opaque=opaque@entry=0x0)
> > > > > #15 0x00005555555609c0 in main (argc=<optimized out>,
> > > > > argv=<optimized
> > > > > out>) at ../system/main.c:76
> > > > > 
> > > > 
> > > > Is this still an issue?
> > > 
> > > Likely fixed by commit e7bc0204 ("system/runstate:
> > > Fix regression, clarify BQL status of exit notifiers"), so
> > > I'm closing https://gitlab.com/qemu-project/qemu/-/issues/2771.
> > 
> > Indeed. I think I was assuming the explicit Resolves: tag in that
> > commit was going to magically close the issue when it was merged.
> > Should I have closed it manually?
> 
> It should have been closed automatically. Maybe some transient
> problem on GitLab side ¯\_(ツ)_/¯

Ack. Thanks for confirming.
diff mbox series

Patch

diff --git a/include/qemu-main.h b/include/qemu-main.h
index 940960a7dbc..2ee83bedff3 100644
--- a/include/qemu-main.h
+++ b/include/qemu-main.h
@@ -5,7 +5,19 @@ 
 #ifndef QEMU_MAIN_H
 #define QEMU_MAIN_H
 
-int qemu_default_main(void);
+/*
+ * The function to run on the main (initial) thread of the process.
+ * NULL means QEMU's main event loop.
+ * When non-NULL, QEMU's main event loop will run on a purposely created
+ * thread, after which the provided function pointer will be invoked on
+ * the initial thread.
+ * This is useful on platforms which treat the main thread as special
+ * (macOS/Darwin) and/or require all UI API calls to occur from the main
+ * thread. Those platforms can initialise it to a specific function,
+ * while UI implementations may reset it to NULL during their init if they
+ * will handle system and UI events on the main thread via QEMU's own main
+ * event loop.
+ */
 extern int (*qemu_main)(void);
 
 #endif /* QEMU_MAIN_H */
diff --git a/system/main.c b/system/main.c
index 4923520741c..ecb12fd397c 100644
--- a/system/main.c
+++ b/system/main.c
@@ -24,26 +24,56 @@ 
 
 #include "qemu/osdep.h"
 #include "qemu-main.h"
+#include "qemu/main-loop.h"
 #include "system/system.h"
 
 #ifdef CONFIG_SDL
+/*
+ * SDL insists on wrapping the main() function with its own implementation on
+ * some platforms; it does so via a macro that renames our main function, so
+ * <SDL.h> must be #included here even with no SDL code called from this file.
+ */
 #include <SDL.h>
 #endif
 
-int qemu_default_main(void)
+#ifdef CONFIG_DARWIN
+#include <CoreFoundation/CoreFoundation.h>
+#endif
+
+static void *qemu_default_main(void *opaque)
 {
     int status;
 
+    bql_lock();
     status = qemu_main_loop();
     qemu_cleanup(status);
+    bql_unlock();
 
-    return status;
+    exit(status);
 }
 
-int (*qemu_main)(void) = qemu_default_main;
+int (*qemu_main)(void);
+
+#ifdef CONFIG_DARWIN
+static int os_darwin_cfrunloop_main(void)
+{
+    CFRunLoopRun();
+    g_assert_not_reached();
+}
+int (*qemu_main)(void) = os_darwin_cfrunloop_main;
+#endif
 
 int main(int argc, char **argv)
 {
     qemu_init(argc, argv);
-    return qemu_main();
+    bql_unlock();
+    if (qemu_main) {
+        QemuThread main_loop_thread;
+        qemu_thread_create(&main_loop_thread, "qemu_main",
+                           qemu_default_main, NULL, QEMU_THREAD_DETACHED);
+        return qemu_main();
+    } else {
+        qemu_default_main(NULL);
+        g_assert_not_reached();
+    }
 }
diff --git a/tests/qtest/fuzz/fuzz.c b/tests/qtest/fuzz/fuzz.c
index 8274000bd55..ca248a51a6c 100644
--- a/tests/qtest/fuzz/fuzz.c
+++ b/tests/qtest/fuzz/fuzz.c
@@ -41,6 +41,7 @@  static FuzzTargetList *fuzz_target_list;
 static FuzzTarget *fuzz_target;
 static QTestState *fuzz_qts;
 
+int (*qemu_main)(void);
 
 
 void flush_events(QTestState *s)
diff --git a/ui/gtk.c b/ui/gtk.c
index 0d38c070e40..c0237431489 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -38,6 +38,7 @@ 
 #include "qemu/cutils.h"
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
+#include "qemu-main.h"
 
 #include "ui/console.h"
 #include "ui/gtk.h"
@@ -2485,6 +2486,9 @@  static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
 #ifdef CONFIG_GTK_CLIPBOARD
     gd_clipboard_init(s);
 #endif /* CONFIG_GTK_CLIPBOARD */
+
+    /* GTK's event polling must happen on the main thread. */
+    qemu_main = NULL;
 }
 
 static void early_gtk_display_init(DisplayOptions *opts)
diff --git a/ui/sdl2.c b/ui/sdl2.c
index 1fb72f67a6f..445eb1dd9f9 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -34,6 +34,7 @@ 
 #include "system/system.h"
 #include "ui/win32-kbd-hook.h"
 #include "qemu/log.h"
+#include "qemu-main.h"
 
 static int sdl2_num_outputs;
 static struct sdl2_console *sdl2_console;
@@ -965,6 +966,9 @@  static void sdl2_display_init(DisplayState *ds, DisplayOptions *o)
     }
 
     atexit(sdl_cleanup);
+
+    /* SDL's event polling (in dpy_refresh) must happen on the main thread. */
+    qemu_main = NULL;
 }
 
 static QemuDisplay qemu_display_sdl2 = {
diff --git a/ui/cocoa.m b/ui/cocoa.m
index 3a88535374d..23b7a736d70 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -73,6 +73,8 @@ 
     int height;
 } QEMUScreen;
 
+@class QemuCocoaPasteboardTypeOwner;
+
 static void cocoa_update(DisplayChangeListener *dcl,
                          int x, int y, int w, int h);
 
@@ -107,6 +109,7 @@  static void cocoa_switch(DisplayChangeListener *dcl,
 static NSInteger cbchangecount = -1;
 static QemuClipboardInfo *cbinfo;
 static QemuEvent cbevent;
+static QemuCocoaPasteboardTypeOwner *cbowner;
 
 // Utility functions to run specified code block with the BQL held
 typedef void (^CodeBlock)(void);
@@ -1326,8 +1329,10 @@  - (void) dealloc
 {
     COCOA_DEBUG("QemuCocoaAppController: dealloc\n");
 
-    if (cocoaView)
-        [cocoaView release];
+    [cocoaView release];
+    [cbowner release];
+    cbowner = nil;
+
     [super dealloc];
 }
 
@@ -1943,8 +1948,6 @@  - (void)pasteboard:(NSPasteboard *)sender provideDataForType:(NSPasteboardType)t
 
 @end
 
-static QemuCocoaPasteboardTypeOwner *cbowner;
-
 static void cocoa_clipboard_notify(Notifier *notifier, void *data);
 static void cocoa_clipboard_request(QemuClipboardInfo *info,
                                     QemuClipboardType type);
@@ -2007,43 +2010,8 @@  static void cocoa_clipboard_request(QemuClipboardInfo *info,
     }
 }
 
-/*
- * The startup process for the OSX/Cocoa UI is complicated, because
- * OSX insists that the UI runs on the initial main thread, and so we
- * need to start a second thread which runs the qemu_default_main():
- * in main():
- *  in cocoa_display_init():
- *   assign cocoa_main to qemu_main
- *   create application, menus, etc
- *  in cocoa_main():
- *   create qemu-main thread
- *   enter OSX run loop
- */
-
-static void *call_qemu_main(void *opaque)
-{
-    int status;
-
-    COCOA_DEBUG("Second thread: calling qemu_default_main()\n");
-    bql_lock();
-    status = qemu_default_main();
-    bql_unlock();
-    COCOA_DEBUG("Second thread: qemu_default_main() returned, exiting\n");
-    [cbowner release];
-    exit(status);
-}
-
 static int cocoa_main(void)
 {
-    QemuThread thread;
-
-    COCOA_DEBUG("Entered %s()\n", __func__);
-
-    bql_unlock();
-    qemu_thread_create(&thread, "qemu_main", call_qemu_main,
-                       NULL, QEMU_THREAD_DETACHED);
-
-    // Start the main event loop
     COCOA_DEBUG("Main thread: entering OSX run loop\n");
     [NSApp run];
     COCOA_DEBUG("Main thread: left OSX run loop, which should never happen\n");
@@ -2125,8 +2093,6 @@  static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
 
     COCOA_DEBUG("qemu_cocoa: cocoa_display_init\n");
 
-    qemu_main = cocoa_main;
-
     // Pull this console process up to being a fully-fledged graphical
     // app with a menubar and Dock icon
     ProcessSerialNumber psn = { 0, kCurrentProcess };
@@ -2190,6 +2156,12 @@  static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
     qemu_clipboard_peer_register(&cbpeer);
 
     [pool release];
+
+    /*
+     * The Cocoa UI will run the NSApplication runloop on the main thread
+     * rather than the default Core Foundation one.
+     */
+    qemu_main = cocoa_main;
 }
 
 static QemuDisplay qemu_display_cocoa = {