| Message ID | 20241113142343.40832-2-phil@philjordan.eu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | macOS PV Graphics and new vmapple machine type | expand |
On 11/13/24 15:23, Phil Dennis-Jordan wrote: > 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. > > Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu> > Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com> I checked what GTK+ does and, either way, you have to create another thread: timers are handled with a CFRunLoopTimer, but file descriptors are polled in a separate thread and sent to the main thread with a single CFRunLoopSource. It's a bit nicer that the main thread is in charge, but it's more complex and probably slower too. As long as it's clear that any handlers that go through the CFRunLoop run outside the BQL, as is already the case for the Cocoa UI, I see no problem with this approach. Acked-by: Paolo Bonzini <pbonzini@redhat.com> Paolo
On Wed, 13 Nov 2024, Phil Dennis-Jordan wrote: > 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. > > Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu> > Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > > v5: > > * Simplified the way of setting/clearing the main loop by going back > to setting qemu_main directly, but narrowing the scope of what it > needs to do, and it can now be NULL. > > v6: > > * Folded function qemu_run_default_main_on_new_thread's code into > main() > * Removed whitespace changes left over on lines near code removed > between v4 and v5 > > v9: > > * Set qemu_main to NULL for GTK UI as well. > > v10: > > * Added comments clarifying the functionality and purpose of qemu_main. > > include/qemu-main.h | 21 ++++++++++++++-- > include/qemu/typedefs.h | 1 + > system/main.c | 50 ++++++++++++++++++++++++++++++++++---- > ui/cocoa.m | 54 ++++++++++------------------------------- > ui/gtk.c | 8 ++++++ > ui/sdl2.c | 4 +++ > 6 files changed, 90 insertions(+), 48 deletions(-) > > diff --git a/include/qemu-main.h b/include/qemu-main.h > index 940960a7dbc..fc70681c8b5 100644 > --- a/include/qemu-main.h > +++ b/include/qemu-main.h > @@ -5,7 +5,24 @@ > #ifndef QEMU_MAIN_H > #define QEMU_MAIN_H > > -int qemu_default_main(void); > -extern int (*qemu_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 a > + * specific thread. > + * 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.) > + */ > +extern qemu_main_fn qemu_main; qemu_main_fn is defined in typdefefs.h but not included here. Also coding style says typedefs should be CamelCase but maybe it's just not worth to define a type for this and you can just leave the existing line here only removing the qemu_default_main declaration and adding the comment. > #endif /* QEMU_MAIN_H */ > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h > index 3d84efcac47..b02cfe1f328 100644 > --- a/include/qemu/typedefs.h > +++ b/include/qemu/typedefs.h > @@ -131,5 +131,6 @@ typedef struct IRQState *qemu_irq; > * Function types > */ > typedef void (*qemu_irq_handler)(void *opaque, int n, int level); > +typedef int (*qemu_main_fn)(void); Then drop this change... > #endif /* QEMU_TYPEDEFS_H */ > diff --git a/system/main.c b/system/main.c > index 9b91d21ea8c..d9397a6d5d0 100644 > --- a/system/main.c > +++ b/system/main.c > @@ -24,13 +24,14 @@ > > #include "qemu/osdep.h" > #include "qemu-main.h" > +#include "qemu/main-loop.h" > #include "sysemu/sysemu.h" > > -#ifdef CONFIG_SDL > -#include <SDL.h> > +#ifdef CONFIG_DARWIN > +#include <CoreFoundation/CoreFoundation.h> > #endif > > -int qemu_default_main(void) > +static int qemu_default_main(void) > { > int status; > > @@ -40,10 +41,49 @@ int qemu_default_main(void) > return status; > } > > -int (*qemu_main)(void) = qemu_default_main; ...and leave this line here without init to define the global variable and only put assigning the init value in the #ifdef if you don't want to repeat the function pointer definition twice (but that wouldn't be a problem either in my opinion to do it in the #ifdef this way). > +/* > + * Various macOS system libraries, including the Cocoa UI and anything using > + * libdispatch, such as ParavirtualizedGraphics.framework, requires that the > + * main runloop, on the main (initial) thread be running or at least regularly > + * polled for events. A special mode is therefore supported, where the QEMU > + * main loop runs on a separate thread and the main thread handles the > + * CF/Cocoa runloop. > + */ > + > +static void *call_qemu_default_main(void *opaque) > +{ > + int status; > + > + bql_lock(); > + status = qemu_default_main(); > + bql_unlock(); > + > + exit(status); > +} > + > +#ifdef CONFIG_DARWIN > +static int os_darwin_cfrunloop_main(void) > +{ > + CFRunLoopRun(); > + abort(); Is it better to use g_assert_not_reached() here? > +} > + > +qemu_main_fn qemu_main = os_darwin_cfrunloop_main; > +#else > +qemu_main_fn qemu_main; > +#endif > > int main(int argc, char **argv) > { > + QemuThread main_loop_thread; > + > qemu_init(argc, argv); > - return qemu_main(); > + if (qemu_main) { > + qemu_thread_create(&main_loop_thread, "qemu_main", > + call_qemu_default_main, NULL, QEMU_THREAD_DETACHED); > + bql_unlock(); > + return qemu_main(); > + } else { > + qemu_default_main(); I think you need 'return qemu_default_main()' here but I'm a bit confused by all this wrapping of qemu_default_main in call_qemu_default_main. I see that may be needed because qemu_thread_create takes a different function but now that qemu_default main is static and not replaced externally could that be changed to the right type and avoid this confusion and simplify this a bit? Regards, BALATON Zoltan > + } > } > diff --git a/ui/cocoa.m b/ui/cocoa.m > index 4c2dd335323..30b8920d929 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); > @@ -1321,8 +1324,10 @@ - (void) dealloc > { > COCOA_DEBUG("QemuCocoaAppController: dealloc\n"); > > - if (cocoaView) > - [cocoaView release]; > + [cocoaView release]; > + [cbowner release]; > + cbowner = nil; > + > [super dealloc]; > } > > @@ -1938,8 +1943,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); > @@ -2002,43 +2005,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"); > @@ -2120,8 +2088,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 }; > @@ -2185,6 +2151,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 = { > diff --git a/ui/gtk.c b/ui/gtk.c > index bf9d3dd679a..fbf20161f36 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,13 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts) > #ifdef CONFIG_GTK_CLIPBOARD > gd_clipboard_init(s); > #endif /* CONFIG_GTK_CLIPBOARD */ > + > + /* > + * GTK+ calls must happen on the main thread at least on some platforms, > + * and on macOS the main runloop is polled via GTK+'s event handling. > + * Don't allow QEMU's event loop to be moved off 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 bd4f5a9da14..44ab2762262 100644 > --- a/ui/sdl2.c > +++ b/ui/sdl2.c > @@ -34,6 +34,7 @@ > #include "sysemu/sysemu.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 = { >
On Wed, Nov 13, 2024 at 7:16 PM BALATON Zoltan <balaton@eik.bme.hu> wrote: > > int main(int argc, char **argv) > > { > > + QemuThread main_loop_thread; > > + > > qemu_init(argc, argv); > > - return qemu_main(); > > + if (qemu_main) { > > + qemu_thread_create(&main_loop_thread, "qemu_main", > > + call_qemu_default_main, NULL, QEMU_THREAD_DETACHED); > > + bql_unlock(); > > + return qemu_main(); > > + } else { > > + qemu_default_main(); > > I think you need 'return qemu_default_main()' here but I'm a bit confused > by all this wrapping of qemu_default_main in call_qemu_default_main. I see > that may be needed because qemu_thread_create takes a different function > but now that qemu_default main is static and not replaced externally could > that be changed to the right type and avoid this confusion and simplify > this a bit? Note that qemu_default_main() expects the BQL to be locked, whereas qemu_main() and call_qemu_default_main() do not (because they run in a separate thread). But you're right, we could push bql_lock()/bql_unlock() into qemu_default_main(), and do bql_unlock(); if (qemu_main) { qemu_thread_create(&main_loop_thread, "qemu_main", call_qemu_default_main, NULL, QEMU_THREAD_DETACHED); return qemu_main(); } else { return qemu_default_main(); } Paolo > Regards, > BALATON Zoltan > > > + } > > } > > diff --git a/ui/cocoa.m b/ui/cocoa.m > > index 4c2dd335323..30b8920d929 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); > > @@ -1321,8 +1324,10 @@ - (void) dealloc > > { > > COCOA_DEBUG("QemuCocoaAppController: dealloc\n"); > > > > - if (cocoaView) > > - [cocoaView release]; > > + [cocoaView release]; > > + [cbowner release]; > > + cbowner = nil; > > + > > [super dealloc]; > > } > > > > @@ -1938,8 +1943,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); > > @@ -2002,43 +2005,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"); > > @@ -2120,8 +2088,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 }; > > @@ -2185,6 +2151,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 = { > > diff --git a/ui/gtk.c b/ui/gtk.c > > index bf9d3dd679a..fbf20161f36 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,13 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts) > > #ifdef CONFIG_GTK_CLIPBOARD > > gd_clipboard_init(s); > > #endif /* CONFIG_GTK_CLIPBOARD */ > > + > > + /* > > + * GTK+ calls must happen on the main thread at least on some platforms, > > + * and on macOS the main runloop is polled via GTK+'s event handling. > > + * Don't allow QEMU's event loop to be moved off 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 bd4f5a9da14..44ab2762262 100644 > > --- a/ui/sdl2.c > > +++ b/ui/sdl2.c > > @@ -34,6 +34,7 @@ > > #include "sysemu/sysemu.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 = { > > >
On Wed, 13 Nov 2024 at 19:36, Paolo Bonzini <pbonzini@redhat.com> wrote: > On Wed, Nov 13, 2024 at 7:16 PM BALATON Zoltan <balaton@eik.bme.hu> wrote: > > > int main(int argc, char **argv) > > > { > > > + QemuThread main_loop_thread; > > > + > > > qemu_init(argc, argv); > > > - return qemu_main(); > > > + if (qemu_main) { > > > + qemu_thread_create(&main_loop_thread, "qemu_main", > > > + call_qemu_default_main, NULL, > QEMU_THREAD_DETACHED); > > > + bql_unlock(); > > > + return qemu_main(); > > > + } else { > > > + qemu_default_main(); > > > > I think you need 'return qemu_default_main()' here but I'm a bit confused > > by all this wrapping of qemu_default_main in call_qemu_default_main. I > see > > that may be needed because qemu_thread_create takes a different function > > but now that qemu_default main is static and not replaced externally > could > > that be changed to the right type and avoid this confusion and simplify > > this a bit? > > Note that qemu_default_main() expects the BQL to be locked, whereas > qemu_main() and call_qemu_default_main() do not (because they run in a > separate thread). > > But you're right, we could push bql_lock()/bql_unlock() into > qemu_default_main(), and do > > bql_unlock(); > if (qemu_main) { > qemu_thread_create(&main_loop_thread, "qemu_main", > call_qemu_default_main, NULL, > QEMU_THREAD_DETACHED); > return qemu_main(); > } else { > return qemu_default_main(); > } > > Good point, if it's safe to drop the lock on thread 0 and re-lock it on another thread before running qemu_main_loop() there, it's also safe to momentarily drop the lock on thread 0 and re-take it before calling into qemu_main_loop(). I'll take that as a starting point and see how far I can simplify things. Thanks to both of you for the feedback! Paolo > > > Regards, > > BALATON Zoltan > > > > > + } > > > } > > > diff --git a/ui/cocoa.m b/ui/cocoa.m > > > index 4c2dd335323..30b8920d929 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); > > > @@ -1321,8 +1324,10 @@ - (void) dealloc > > > { > > > COCOA_DEBUG("QemuCocoaAppController: dealloc\n"); > > > > > > - if (cocoaView) > > > - [cocoaView release]; > > > + [cocoaView release]; > > > + [cbowner release]; > > > + cbowner = nil; > > > + > > > [super dealloc]; > > > } > > > > > > @@ -1938,8 +1943,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); > > > @@ -2002,43 +2005,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"); > > > @@ -2120,8 +2088,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 }; > > > @@ -2185,6 +2151,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 = { > > > diff --git a/ui/gtk.c b/ui/gtk.c > > > index bf9d3dd679a..fbf20161f36 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,13 @@ static void gtk_display_init(DisplayState *ds, > DisplayOptions *opts) > > > #ifdef CONFIG_GTK_CLIPBOARD > > > gd_clipboard_init(s); > > > #endif /* CONFIG_GTK_CLIPBOARD */ > > > + > > > + /* > > > + * GTK+ calls must happen on the main thread at least on some > platforms, > > > + * and on macOS the main runloop is polled via GTK+'s event > handling. > > > + * Don't allow QEMU's event loop to be moved off 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 bd4f5a9da14..44ab2762262 100644 > > > --- a/ui/sdl2.c > > > +++ b/ui/sdl2.c > > > @@ -34,6 +34,7 @@ > > > #include "sysemu/sysemu.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 = { > > > > > > > >
On Wed, 13 Nov 2024 at 17:38, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 11/13/24 15:23, Phil Dennis-Jordan wrote: > > 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. > > > > Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu> > > Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > I checked what GTK+ does and, either way, you have to create another > thread: timers are handled with a CFRunLoopTimer, but file descriptors > are polled in a separate thread and sent to the main thread with a > single CFRunLoopSource. It's a bit nicer that the main thread is in > charge, but it's more complex and probably slower too. > Just to clarify: is this supposed to be happening inside the GTK+ library itself? i.e. GTK should spawn its own thread to poll file descriptors that are owned by GTK? (As opposed to the file descriptors used by QEMU's own event loop - what on Linux are eventfds, but on macOS I think are just pipes*.) This doesn't describe what I'm seeing when I run with -display gtk on macOS. There's no extra thread created. There's a dock icon, but it's non-interactive ("Application not responding"), there aren't any menus, and there's no window. QEMU's own simulation is running in the background - I can reach a guest via the network. So I guess there's some function in GTK we're supposed to be calling that will make it crank the native event loop on macOS, but this isn't being done? Or do you mean there exists a global "block here forever" function in GTK we can call from the main thread and which will make everything spring into life, but that brings with it the same requirement as with the Cocoa UI: moving everything that was originally on the main thread onto a background thread. (I've done some searching for GTK docs and other background info but anything I've found has been rather thin on this topic. The ui/gtk*.c source is also not particularly enlightening - it seems to imply that something in the background ought to be handling events somewhere.) [* The event loop in QEMU on macOS probably ought to use a kqueue, with EVFILT_USER where eventfds are used on Linux, except the rough equivalent of ioeventfds are Mach ports which can be handled via EVFILT_MACHPORT, but I digress.] As long as it's clear that any handlers that go through the CFRunLoop > run outside the BQL, as is already the case for the Cocoa UI, I see no > problem with this approach. > I'm not entirely sure what you're getting at here, to be honest. The UI thread can definitely not assume to be holding the BQL all the time; we'd have to treat it more like an AIOContext. It could pave the way towards putting the display and UI subsystems on their own AIOContext or AIOContext-like-thing, rather than hogging the BQL for expensive image operations. (*By the sound of it, Win32 has an all-UI-calls-on-one-thread requirement as well which we may be violating to some degree via the GTK and/or SDL backends as well; my adventures with Win32 are almost 20 years back now so I'm a bit out of the loop there.) > Acked-by: Paolo Bonzini <pbonzini@redhat.com> > > Paolo > > >
On Wed, 13 Nov 2024 at 19:16, BALATON Zoltan <balaton@eik.bme.hu> wrote: > On Wed, 13 Nov 2024, Phil Dennis-Jordan wrote: > > 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. > > > > Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu> > > Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > --- > > > > v5: > > > > * Simplified the way of setting/clearing the main loop by going back > > to setting qemu_main directly, but narrowing the scope of what it > > needs to do, and it can now be NULL. > > > > v6: > > > > * Folded function qemu_run_default_main_on_new_thread's code into > > main() > > * Removed whitespace changes left over on lines near code removed > > between v4 and v5 > > > > v9: > > > > * Set qemu_main to NULL for GTK UI as well. > > > > v10: > > > > * Added comments clarifying the functionality and purpose of qemu_main. > > > > include/qemu-main.h | 21 ++++++++++++++-- > > include/qemu/typedefs.h | 1 + > > system/main.c | 50 ++++++++++++++++++++++++++++++++++---- > > ui/cocoa.m | 54 ++++++++++------------------------------- > > ui/gtk.c | 8 ++++++ > > ui/sdl2.c | 4 +++ > > 6 files changed, 90 insertions(+), 48 deletions(-) > > > > diff --git a/include/qemu-main.h b/include/qemu-main.h > > index 940960a7dbc..fc70681c8b5 100644 > > --- a/include/qemu-main.h > > +++ b/include/qemu-main.h > > @@ -5,7 +5,24 @@ > > #ifndef QEMU_MAIN_H > > #define QEMU_MAIN_H > > > > -int qemu_default_main(void); > > -extern int (*qemu_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 a > > + * specific thread. > > + * 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.) > > + */ > > +extern qemu_main_fn qemu_main; > > qemu_main_fn is defined in typdefefs.h but not included here. Also coding > style says typedefs should be CamelCase but maybe it's just not worth to > define a type for this and you can just leave the existing line here only > removing the qemu_default_main declaration and adding the comment. > > > #endif /* QEMU_MAIN_H */ > > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h > > index 3d84efcac47..b02cfe1f328 100644 > > --- a/include/qemu/typedefs.h > > +++ b/include/qemu/typedefs.h > > @@ -131,5 +131,6 @@ typedef struct IRQState *qemu_irq; > > * Function types > > */ > > typedef void (*qemu_irq_handler)(void *opaque, int n, int level); > > +typedef int (*qemu_main_fn)(void); > > Then drop this change... > > > #endif /* QEMU_TYPEDEFS_H */ > > diff --git a/system/main.c b/system/main.c > > index 9b91d21ea8c..d9397a6d5d0 100644 > > --- a/system/main.c > > +++ b/system/main.c > > @@ -24,13 +24,14 @@ > > > > #include "qemu/osdep.h" > > #include "qemu-main.h" > > +#include "qemu/main-loop.h" > > #include "sysemu/sysemu.h" > > > > -#ifdef CONFIG_SDL > > -#include <SDL.h> > > +#ifdef CONFIG_DARWIN > > +#include <CoreFoundation/CoreFoundation.h> > > #endif > > > > -int qemu_default_main(void) > > +static int qemu_default_main(void) > > { > > int status; > > > > @@ -40,10 +41,49 @@ int qemu_default_main(void) > > return status; > > } > > > > -int (*qemu_main)(void) = qemu_default_main; > > ...and leave this line here without init to define the global variable and > only put assigning the init value in the #ifdef if you don't want to > repeat the function pointer definition twice (but that wouldn't be a > problem either in my opinion to do it in the #ifdef this way). > > > +/* > > + * Various macOS system libraries, including the Cocoa UI and anything > using > > + * libdispatch, such as ParavirtualizedGraphics.framework, requires > that the > > + * main runloop, on the main (initial) thread be running or at least > regularly > > + * polled for events. A special mode is therefore supported, where the > QEMU > > + * main loop runs on a separate thread and the main thread handles the > > + * CF/Cocoa runloop. > > + */ > > + > > +static void *call_qemu_default_main(void *opaque) > > +{ > > + int status; > > + > > + bql_lock(); > > + status = qemu_default_main(); > > + bql_unlock(); > > + > > + exit(status); > > +} > > + > > +#ifdef CONFIG_DARWIN > > +static int os_darwin_cfrunloop_main(void) > > +{ > > + CFRunLoopRun(); > > + abort(); > > Is it better to use g_assert_not_reached() here? > > > +} > > + > > +qemu_main_fn qemu_main = os_darwin_cfrunloop_main; > > +#else > > +qemu_main_fn qemu_main; > > +#endif > > > > int main(int argc, char **argv) > > { > > + QemuThread main_loop_thread; > > + > > qemu_init(argc, argv); > > - return qemu_main(); > > + if (qemu_main) { > > + qemu_thread_create(&main_loop_thread, "qemu_main", > > + call_qemu_default_main, NULL, > QEMU_THREAD_DETACHED); > > + bql_unlock(); > > + return qemu_main(); > > + } else { > > + qemu_default_main(); > > I think you need 'return qemu_default_main()' here but I'm a bit confused > by all this wrapping of qemu_default_main in call_qemu_default_main. I see > that may be needed because qemu_thread_create takes a different function > but now that qemu_default main is static and not replaced externally could > that be changed to the right type and avoid this confusion and simplify > this a bit? > > Regards, > BALATON Zoltan > > Alright, I've gone ahead and worked through that, and it does indeed make things simpler. This is what I have for include/qemu-main.h and system/main.c now: diff --git a/include/qemu-main.h b/include/qemu-main.h index 940960a7dbc..e1041fe99b4 100644 --- a/include/qemu-main.h +++ b/include/qemu-main.h @@ -5,7 +5,29 @@ #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 a + * specific thread. + * In practice, this means that on macOS, it is initialised to a function + * that runs the Core Foundation runloop. The Cocoa UI "upgrades" this + * to the NSApplication-specific event processing variant. Other platforms + * currently leave it at NULL; the SDL and GTK UIs reset it to NULL even + * on macOS as they directly poll the runloop. + * 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.) + */ extern int (*qemu_main)(void); #endif /* QEMU_MAIN_H */ diff --git a/system/main.c b/system/main.c index 9b91d21ea8c..2d9d144ed46 100644 --- a/system/main.c +++ b/system/main.c @@ -24,26 +24,48 @@ #include "qemu/osdep.h" #include "qemu-main.h" +#include "qemu/main-loop.h" #include "sysemu/sysemu.h" -#ifdef CONFIG_SDL -#include <SDL.h> +#ifdef CONFIG_DARWIN +#include <CoreFoundation/CoreFoundation.h> #endif -int qemu_default_main(void) +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; +#ifdef CONFIG_DARWIN +static int os_darwin_cfrunloop_main(void) +{ + CFRunLoopRun(); + g_assert_not_reached(); +} + +int (*qemu_main)(void) = os_darwin_cfrunloop_main; +#else +int (*qemu_main)(void); +#endif int main(int argc, char **argv) { + QemuThread main_loop_thread; + qemu_init(argc, argv); - return qemu_main(); + bql_unlock(); + if (qemu_main) { + qemu_thread_create(&main_loop_thread, "qemu_main", + qemu_default_main, NULL, QEMU_THREAD_DETACHED); + return qemu_main(); + } else { + qemu_default_main(NULL); + } }
On Thu, Nov 14, 2024 at 11:32 AM Phil Dennis-Jordan <lists@philjordan.eu> wrote: >> I checked what GTK+ does and, either way, you have to create another >> thread: timers are handled with a CFRunLoopTimer, but file descriptors >> are polled in a separate thread and sent to the main thread with a >> single CFRunLoopSource. It's a bit nicer that the main thread is in >> charge, but it's more complex and probably slower too. > > > Just to clarify: is this supposed to be happening inside the GTK+ library itself? i.e. GTK should spawn its own thread to poll file descriptors that are owned by GTK? (As opposed to the file descriptors used by QEMU's own event loop - what on Linux are eventfds, but on macOS I think are just pipes*.) It's what I saw in the GTK+ source code. https://gitlab.gnome.org/GNOME/gtk/-/blob/main/gdk/macos/gdkmacoseventsource.c?ref_type=heads > This doesn't describe what I'm seeing when I run with -display gtk on macOS. There's no extra thread created. There's a dock icon, but it's non-interactive ("Application not responding"), there aren't any menus, and there's no window. QEMU's own simulation is running in the background - I can reach a guest via the network. So I guess there's some function in GTK we're supposed to be calling that will make it crank the native event loop on macOS, but this isn't being done? In theory it should work, with the event source added as soon as GTK+ is started... aha! We do not use the GMainContext's poll_func, we use qemu_poll_ns. That's the missing part: GDK replaces the poll_func with one that calls nextEventMatchingMask: https://gitlab.gnome.org/GNOME/gtk/-/blame/main/gdk/macos/gdkmacoseventsource.c?ref_type=heads#L767 There could be more issues, but I think for now it's better to block the GTK+ UI under macOS. [...] >> As long as it's clear that any handlers that go through the CFRunLoop >> run outside the BQL, as is already the case for the Cocoa UI, I see no >> problem with this approach. > > I'm not entirely sure what you're getting at here, to be honest. The UI thread can definitely not assume to be holding the BQL all the time; we'd have to treat it more like an AIOContext. It could pave the way towards putting the display and UI subsystems on their own AIOContext or AIOContext-like-thing, rather than hogging the BQL for expensive image operations. Don't worry, I was mostly talking to myself... The UI thread, and more specifically any handlers that are called from CFRunLoop instead of QEMU's main loop, will have to use mutexes or bql_lock()/bql_unlock(), like ui/cocoa.m already does. In other words, code that interacts with Apple's paravirtualized graphics needs to know if it runs from the CFRunLoop or from qemu_main(). > (*By the sound of it, Win32 has an all-UI-calls-on-one-thread requirement as well which we may be violating to some degree via the GTK and/or SDL backends as well; my adventures with Win32 are almost 20 years back now so I'm a bit out of the loop there.) Hmm, no I don't remember anything like that for Windows but it's also been many years for me. Paolo
On Thu, 14 Nov 2024, Phil Dennis-Jordan wrote: > On Wed, 13 Nov 2024 at 19:16, BALATON Zoltan <balaton@eik.bme.hu> wrote: >> On Wed, 13 Nov 2024, Phil Dennis-Jordan wrote: >>> 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. >>> >>> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu> >>> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>> --- >>> >>> v5: >>> >>> * Simplified the way of setting/clearing the main loop by going back >>> to setting qemu_main directly, but narrowing the scope of what it >>> needs to do, and it can now be NULL. >>> >>> v6: >>> >>> * Folded function qemu_run_default_main_on_new_thread's code into >>> main() >>> * Removed whitespace changes left over on lines near code removed >>> between v4 and v5 >>> >>> v9: >>> >>> * Set qemu_main to NULL for GTK UI as well. >>> >>> v10: >>> >>> * Added comments clarifying the functionality and purpose of qemu_main. >>> >>> include/qemu-main.h | 21 ++++++++++++++-- >>> include/qemu/typedefs.h | 1 + >>> system/main.c | 50 ++++++++++++++++++++++++++++++++++---- >>> ui/cocoa.m | 54 ++++++++++------------------------------- >>> ui/gtk.c | 8 ++++++ >>> ui/sdl2.c | 4 +++ >>> 6 files changed, 90 insertions(+), 48 deletions(-) >>> >>> diff --git a/include/qemu-main.h b/include/qemu-main.h >>> index 940960a7dbc..fc70681c8b5 100644 >>> --- a/include/qemu-main.h >>> +++ b/include/qemu-main.h >>> @@ -5,7 +5,24 @@ >>> #ifndef QEMU_MAIN_H >>> #define QEMU_MAIN_H >>> >>> -int qemu_default_main(void); >>> -extern int (*qemu_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 a >>> + * specific thread. >>> + * 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.) >>> + */ >>> +extern qemu_main_fn qemu_main; >> >> qemu_main_fn is defined in typdefefs.h but not included here. Also coding >> style says typedefs should be CamelCase but maybe it's just not worth to >> define a type for this and you can just leave the existing line here only >> removing the qemu_default_main declaration and adding the comment. >> >>> #endif /* QEMU_MAIN_H */ >>> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h >>> index 3d84efcac47..b02cfe1f328 100644 >>> --- a/include/qemu/typedefs.h >>> +++ b/include/qemu/typedefs.h >>> @@ -131,5 +131,6 @@ typedef struct IRQState *qemu_irq; >>> * Function types >>> */ >>> typedef void (*qemu_irq_handler)(void *opaque, int n, int level); >>> +typedef int (*qemu_main_fn)(void); >> >> Then drop this change... >> >>> #endif /* QEMU_TYPEDEFS_H */ >>> diff --git a/system/main.c b/system/main.c >>> index 9b91d21ea8c..d9397a6d5d0 100644 >>> --- a/system/main.c >>> +++ b/system/main.c >>> @@ -24,13 +24,14 @@ >>> >>> #include "qemu/osdep.h" >>> #include "qemu-main.h" >>> +#include "qemu/main-loop.h" >>> #include "sysemu/sysemu.h" >>> >>> -#ifdef CONFIG_SDL >>> -#include <SDL.h> >>> +#ifdef CONFIG_DARWIN >>> +#include <CoreFoundation/CoreFoundation.h> >>> #endif >>> >>> -int qemu_default_main(void) >>> +static int qemu_default_main(void) >>> { >>> int status; >>> >>> @@ -40,10 +41,49 @@ int qemu_default_main(void) >>> return status; >>> } >>> >>> -int (*qemu_main)(void) = qemu_default_main; >> >> ...and leave this line here without init to define the global variable and >> only put assigning the init value in the #ifdef if you don't want to >> repeat the function pointer definition twice (but that wouldn't be a >> problem either in my opinion to do it in the #ifdef this way). >> >>> +/* >>> + * Various macOS system libraries, including the Cocoa UI and anything >> using >>> + * libdispatch, such as ParavirtualizedGraphics.framework, requires >> that the >>> + * main runloop, on the main (initial) thread be running or at least >> regularly >>> + * polled for events. A special mode is therefore supported, where the >> QEMU >>> + * main loop runs on a separate thread and the main thread handles the >>> + * CF/Cocoa runloop. >>> + */ >>> + >>> +static void *call_qemu_default_main(void *opaque) >>> +{ >>> + int status; >>> + >>> + bql_lock(); >>> + status = qemu_default_main(); >>> + bql_unlock(); >>> + >>> + exit(status); >>> +} >>> + >>> +#ifdef CONFIG_DARWIN >>> +static int os_darwin_cfrunloop_main(void) >>> +{ >>> + CFRunLoopRun(); >>> + abort(); >> >> Is it better to use g_assert_not_reached() here? >> >>> +} >>> + >>> +qemu_main_fn qemu_main = os_darwin_cfrunloop_main; >>> +#else >>> +qemu_main_fn qemu_main; >>> +#endif >>> >>> int main(int argc, char **argv) >>> { >>> + QemuThread main_loop_thread; >>> + >>> qemu_init(argc, argv); >>> - return qemu_main(); >>> + if (qemu_main) { >>> + qemu_thread_create(&main_loop_thread, "qemu_main", >>> + call_qemu_default_main, NULL, >> QEMU_THREAD_DETACHED); >>> + bql_unlock(); >>> + return qemu_main(); >>> + } else { >>> + qemu_default_main(); >> >> I think you need 'return qemu_default_main()' here but I'm a bit confused >> by all this wrapping of qemu_default_main in call_qemu_default_main. I see >> that may be needed because qemu_thread_create takes a different function >> but now that qemu_default main is static and not replaced externally could >> that be changed to the right type and avoid this confusion and simplify >> this a bit? >> >> Regards, >> BALATON Zoltan >> >> > Alright, I've gone ahead and worked through that, and it does indeed make > things simpler. This is what I have for include/qemu-main.h and > system/main.c now: I think it looks better (can't tell if it will work so leave review to others who know this better). Maybe just a couple of more nits below. > diff --git a/include/qemu-main.h b/include/qemu-main.h > index 940960a7dbc..e1041fe99b4 100644 > --- a/include/qemu-main.h > +++ b/include/qemu-main.h > @@ -5,7 +5,29 @@ > #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 a > + * specific thread. > + * In practice, this means that on macOS, it is initialised to a function > + * that runs the Core Foundation runloop. The Cocoa UI "upgrades" this > + * to the NSApplication-specific event processing variant. Other platforms > + * currently leave it at NULL; the SDL and GTK UIs reset it to NULL even > + * on macOS as they directly poll the runloop. I'm not sure this last paragraph below belongs here or maybe better put it somewhere else (but if nobody else objects it can be left here for reminder and later clean up). I know I suggested to add a comment to note this but this comment is a bit long and with a lot of uncertainity so maybe it's enough to put this paragraph in the commit message, but it could be it will be burried there and nobody will see it later so a comment is more prominent place. I'm OK with it either way. > + * 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.) > + */ > extern int (*qemu_main)(void); > > #endif /* QEMU_MAIN_H */ > diff --git a/system/main.c b/system/main.c > index 9b91d21ea8c..2d9d144ed46 100644 > --- a/system/main.c > +++ b/system/main.c > @@ -24,26 +24,48 @@ > > #include "qemu/osdep.h" > #include "qemu-main.h" > +#include "qemu/main-loop.h" > #include "sysemu/sysemu.h" > > -#ifdef CONFIG_SDL > -#include <SDL.h> > +#ifdef CONFIG_DARWIN > +#include <CoreFoundation/CoreFoundation.h> > #endif > > -int qemu_default_main(void) > +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; You could leave the definition without value here (what's now in the #else) and then only assign it in the #if so this line would become -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; and this would be qemu_main = os_darwin_cfrunloop_main; and no need for #else. Also the name of this darwin main is a bit long, maybe qemu_darwin_main or qemu_default_main_darwin could be better? > +#else > +int (*qemu_main)(void); > +#endif > > int main(int argc, char **argv) > { > + QemuThread main_loop_thread; This could be moved within the if (qemu_main) below as it's not needed outside. > + > qemu_init(argc, argv); > - return qemu_main(); > + bql_unlock(); What locks the bql at this point? I could not find the part in qemu_init that makes it necessary to unlock it here. If nothing else is called before this, is it needed to take the lock and then unlock it here or can it be assumed to be unlocked yet? Regards, BALATON Zoltan > + if (qemu_main) { > + qemu_thread_create(&main_loop_thread, "qemu_main", > + qemu_default_main, NULL, QEMU_THREAD_DETACHED); > + return qemu_main(); > + } else { > + qemu_default_main(NULL); > + } > } >
On Thu, 14 Nov 2024 at 14:27, Paolo Bonzini <pbonzini@redhat.com> wrote: > On Thu, Nov 14, 2024 at 11:32 AM Phil Dennis-Jordan <lists@philjordan.eu> > wrote: > >> I checked what GTK+ does and, either way, you have to create another > >> thread: timers are handled with a CFRunLoopTimer, but file descriptors > >> are polled in a separate thread and sent to the main thread with a > >> single CFRunLoopSource. It's a bit nicer that the main thread is in > >> charge, but it's more complex and probably slower too. > > > > > > Just to clarify: is this supposed to be happening inside the GTK+ > library itself? i.e. GTK should spawn its own thread to poll file > descriptors that are owned by GTK? (As opposed to the file descriptors used > by QEMU's own event loop - what on Linux are eventfds, but on macOS I think > are just pipes*.) > > It's what I saw in the GTK+ source code. > > > https://gitlab.gnome.org/GNOME/gtk/-/blob/main/gdk/macos/gdkmacoseventsource.c?ref_type=heads > > > This doesn't describe what I'm seeing when I run with -display gtk on > macOS. There's no extra thread created. There's a dock icon, but it's > non-interactive ("Application not responding"), there aren't any menus, and > there's no window. QEMU's own simulation is running in the background - I > can reach a guest via the network. So I guess there's some function in GTK > we're supposed to be calling that will make it crank the native event loop > on macOS, but this isn't being done? > > In theory it should work, with the event source added as soon as GTK+ > is started... aha! > > We do not use the GMainContext's poll_func, we use qemu_poll_ns. > That's the missing part: GDK replaces the poll_func with one that > calls nextEventMatchingMask: > > > https://gitlab.gnome.org/GNOME/gtk/-/blame/main/gdk/macos/gdkmacoseventsource.c?ref_type=heads#L767 > Thanks for explaining. That makes sense - I was looking in the Qemu source for where exactly it was polling the GLib/GTK event handler, now I know why I couldn't find it. Tracing the poll_func setting, it looks like GTK expects g_main_context_iteration() to be called regularly, or the main thread needs to call and block inside g_main_loop_run. But in QEMU it's not as easy a fix as just going ahead and doing one of those two things because QEMU semi-replicates the GLib poll function, and we can't use both at the same time. Right? > There could be more issues, but I think for now it's better to block > the GTK+ UI under macOS. > OK, I've created a new issue on GitLab where any further decision making and action can be tracked: https://gitlab.com/qemu-project/qemu/-/issues/2676 I'm not sure this patchset is the best place for a patch blocking GTK on macOS, especially if you want it in 9.2. > [...] > > >> As long as it's clear that any handlers that go through the CFRunLoop > >> run outside the BQL, as is already the case for the Cocoa UI, I see no > >> problem with this approach. > > > > I'm not entirely sure what you're getting at here, to be honest. The UI > thread can definitely not assume to be holding the BQL all the time; we'd > have to treat it more like an AIOContext. It could pave the way towards > putting the display and UI subsystems on their own AIOContext or > AIOContext-like-thing, rather than hogging the BQL for expensive image > operations. > > Don't worry, I was mostly talking to myself... The UI thread, and more > specifically any handlers that are called from CFRunLoop instead of > QEMU's main loop, will have to use mutexes or bql_lock()/bql_unlock(), > like ui/cocoa.m already does. > > In other words, code that interacts with Apple's paravirtualized > graphics needs to know if it runs from the CFRunLoop or from > qemu_main(). > We already have extremely careful threading code in patch 02/15 for safely interfacing the threading models of the PV Graphics/libdispatch world and QEMU's world of BQL, AIOs, BHs, and so on. > > (*By the sound of it, Win32 has an all-UI-calls-on-one-thread > requirement as well which we may be violating to some degree via the GTK > and/or SDL backends as well; my adventures with Win32 are almost 20 years > back now so I'm a bit out of the loop there.) > > Hmm, no I don't remember anything like that for Windows but it's also > been many years for me. > Sorry, this was in reference to Daniel Berrangé's comment on this related bug: https://gitlab.com/qemu-project/qemu/-/issues/2537#note_2203183775 (QEMU's SDL UI also falls afoul of macOS's threading requirements in some specific cases.)
On Thu, 14 Nov 2024 at 14:38, BALATON Zoltan <balaton@eik.bme.hu> wrote: > On Thu, 14 Nov 2024, Phil Dennis-Jordan wrote: > > On Wed, 13 Nov 2024 at 19:16, BALATON Zoltan <balaton@eik.bme.hu> wrote: > >> On Wed, 13 Nov 2024, Phil Dennis-Jordan wrote: > >>> 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. > >>> > >>> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu> > >>> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com> > >>> --- > >>> > >>> v5: > >>> > >>> * Simplified the way of setting/clearing the main loop by going back > >>> to setting qemu_main directly, but narrowing the scope of what it > >>> needs to do, and it can now be NULL. > >>> > >>> v6: > >>> > >>> * Folded function qemu_run_default_main_on_new_thread's code into > >>> main() > >>> * Removed whitespace changes left over on lines near code removed > >>> between v4 and v5 > >>> > >>> v9: > >>> > >>> * Set qemu_main to NULL for GTK UI as well. > >>> > >>> v10: > >>> > >>> * Added comments clarifying the functionality and purpose of qemu_main. > >>> > >>> include/qemu-main.h | 21 ++++++++++++++-- > >>> include/qemu/typedefs.h | 1 + > >>> system/main.c | 50 ++++++++++++++++++++++++++++++++++---- > >>> ui/cocoa.m | 54 ++++++++++------------------------------- > >>> ui/gtk.c | 8 ++++++ > >>> ui/sdl2.c | 4 +++ > >>> 6 files changed, 90 insertions(+), 48 deletions(-) > >>> > >>> diff --git a/include/qemu-main.h b/include/qemu-main.h > >>> index 940960a7dbc..fc70681c8b5 100644 > >>> --- a/include/qemu-main.h > >>> +++ b/include/qemu-main.h > >>> @@ -5,7 +5,24 @@ > >>> #ifndef QEMU_MAIN_H > >>> #define QEMU_MAIN_H > >>> > >>> -int qemu_default_main(void); > >>> -extern int (*qemu_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 a > >>> + * specific thread. > >>> + * 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.) > >>> + */ > >>> +extern qemu_main_fn qemu_main; > >> > >> qemu_main_fn is defined in typdefefs.h but not included here. Also > coding > >> style says typedefs should be CamelCase but maybe it's just not worth to > >> define a type for this and you can just leave the existing line here > only > >> removing the qemu_default_main declaration and adding the comment. > >> > >>> #endif /* QEMU_MAIN_H */ > >>> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h > >>> index 3d84efcac47..b02cfe1f328 100644 > >>> --- a/include/qemu/typedefs.h > >>> +++ b/include/qemu/typedefs.h > >>> @@ -131,5 +131,6 @@ typedef struct IRQState *qemu_irq; > >>> * Function types > >>> */ > >>> typedef void (*qemu_irq_handler)(void *opaque, int n, int level); > >>> +typedef int (*qemu_main_fn)(void); > >> > >> Then drop this change... > >> > >>> #endif /* QEMU_TYPEDEFS_H */ > >>> diff --git a/system/main.c b/system/main.c > >>> index 9b91d21ea8c..d9397a6d5d0 100644 > >>> --- a/system/main.c > >>> +++ b/system/main.c > >>> @@ -24,13 +24,14 @@ > >>> > >>> #include "qemu/osdep.h" > >>> #include "qemu-main.h" > >>> +#include "qemu/main-loop.h" > >>> #include "sysemu/sysemu.h" > >>> > >>> -#ifdef CONFIG_SDL > >>> -#include <SDL.h> > >>> +#ifdef CONFIG_DARWIN > >>> +#include <CoreFoundation/CoreFoundation.h> > >>> #endif > >>> > >>> -int qemu_default_main(void) > >>> +static int qemu_default_main(void) > >>> { > >>> int status; > >>> > >>> @@ -40,10 +41,49 @@ int qemu_default_main(void) > >>> return status; > >>> } > >>> > >>> -int (*qemu_main)(void) = qemu_default_main; > >> > >> ...and leave this line here without init to define the global variable > and > >> only put assigning the init value in the #ifdef if you don't want to > >> repeat the function pointer definition twice (but that wouldn't be a > >> problem either in my opinion to do it in the #ifdef this way). > >> > >>> +/* > >>> + * Various macOS system libraries, including the Cocoa UI and anything > >> using > >>> + * libdispatch, such as ParavirtualizedGraphics.framework, requires > >> that the > >>> + * main runloop, on the main (initial) thread be running or at least > >> regularly > >>> + * polled for events. A special mode is therefore supported, where the > >> QEMU > >>> + * main loop runs on a separate thread and the main thread handles the > >>> + * CF/Cocoa runloop. > >>> + */ > >>> + > >>> +static void *call_qemu_default_main(void *opaque) > >>> +{ > >>> + int status; > >>> + > >>> + bql_lock(); > >>> + status = qemu_default_main(); > >>> + bql_unlock(); > >>> + > >>> + exit(status); > >>> +} > >>> + > >>> +#ifdef CONFIG_DARWIN > >>> +static int os_darwin_cfrunloop_main(void) > >>> +{ > >>> + CFRunLoopRun(); > >>> + abort(); > >> > >> Is it better to use g_assert_not_reached() here? > >> > >>> +} > >>> + > >>> +qemu_main_fn qemu_main = os_darwin_cfrunloop_main; > >>> +#else > >>> +qemu_main_fn qemu_main; > >>> +#endif > >>> > >>> int main(int argc, char **argv) > >>> { > >>> + QemuThread main_loop_thread; > >>> + > >>> qemu_init(argc, argv); > >>> - return qemu_main(); > >>> + if (qemu_main) { > >>> + qemu_thread_create(&main_loop_thread, "qemu_main", > >>> + call_qemu_default_main, NULL, > >> QEMU_THREAD_DETACHED); > >>> + bql_unlock(); > >>> + return qemu_main(); > >>> + } else { > >>> + qemu_default_main(); > >> > >> I think you need 'return qemu_default_main()' here but I'm a bit > confused > >> by all this wrapping of qemu_default_main in call_qemu_default_main. I > see > >> that may be needed because qemu_thread_create takes a different function > >> but now that qemu_default main is static and not replaced externally > could > >> that be changed to the right type and avoid this confusion and simplify > >> this a bit? > >> > >> Regards, > >> BALATON Zoltan > >> > >> > > Alright, I've gone ahead and worked through that, and it does indeed make > > things simpler. This is what I have for include/qemu-main.h and > > system/main.c now: > > I think it looks better (can't tell if it will work so leave review to > others who know this better). Maybe just a couple of more nits below. > > > diff --git a/include/qemu-main.h b/include/qemu-main.h > > index 940960a7dbc..e1041fe99b4 100644 > > --- a/include/qemu-main.h > > +++ b/include/qemu-main.h > > @@ -5,7 +5,29 @@ > > #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 a > > + * specific thread. > > + * In practice, this means that on macOS, it is initialised to a > function > > + * that runs the Core Foundation runloop. The Cocoa UI "upgrades" this > > + * to the NSApplication-specific event processing variant. Other > platforms > > + * currently leave it at NULL; the SDL and GTK UIs reset it to NULL even > > + * on macOS as they directly poll the runloop. > > I'm not sure this last paragraph below belongs here or maybe better put it > somewhere else (but if nobody else objects it can be left here for > reminder and later clean up). I know I suggested to add a comment to note > this but this comment is a bit long and with a lot of uncertainity so > maybe it's enough to put this paragraph in the commit message, but it > could be it will be burried there and nobody will see it later so a > comment is more prominent place. I'm OK with it either way. > You asked for a comment to be added. ;-) I guess I went with a brain dump of: 1. What this mechanism achieves. 2. How it's currently used. 3. What problems it doesn't solve yet. 4. Possible approaches for a more general mechanism in future. We could arguably remove (2) because every single use of the qemu_main variable actually now has a comment directly there or very nearby, so a simple code search by future code explorers should find those. > > + * 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.) > > + */ > > extern int (*qemu_main)(void); > > > > #endif /* QEMU_MAIN_H */ > > diff --git a/system/main.c b/system/main.c > > index 9b91d21ea8c..2d9d144ed46 100644 > > --- a/system/main.c > > +++ b/system/main.c > > @@ -24,26 +24,48 @@ > > > > #include "qemu/osdep.h" > > #include "qemu-main.h" > > +#include "qemu/main-loop.h" > > #include "sysemu/sysemu.h" > > > > -#ifdef CONFIG_SDL > > -#include <SDL.h> > > +#ifdef CONFIG_DARWIN > > +#include <CoreFoundation/CoreFoundation.h> > > #endif > > > > -int qemu_default_main(void) > > +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; > > You could leave the definition without value here (what's now in the > #else) and then only assign it in the #if so this line would become > > -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; > > and this would be > > qemu_main = os_darwin_cfrunloop_main; > > and no need for #else. Also the name of this darwin main is a bit long, > maybe qemu_darwin_main or qemu_default_main_darwin could be better? > I guess I wanted to clarify that it's using the CFRunloop variant of macOS event handling, in contrast to the Cocoa UI's, which uses [NSApp run]; which additionally performs all the Cocoa application bring-up before it starts handling events. > > +#else > > +int (*qemu_main)(void); > > +#endif > > > > int main(int argc, char **argv) > > { > > + QemuThread main_loop_thread; > > This could be moved within the if (qemu_main) below as it's not needed > outside. > > > + > > qemu_init(argc, argv); > > - return qemu_main(); > > + bql_unlock(); > > What locks the bql at this point? I could not find the part in qemu_init > that makes it necessary to unlock it here. If nothing else is called > before this, is it needed to take the lock and then unlock it here or can > it be assumed to be unlocked yet? > It looks like that happens in qemu_init_subsystems(), right after the BQL is created inside qemu_init_cpu_loop(): https://gitlab.com/qemu-project/qemu/-/blob/master/system/runstate.c?ref_type=heads#L866 > Regards, > BALATON Zoltan > > > + if (qemu_main) { > > + qemu_thread_create(&main_loop_thread, "qemu_main", > > + qemu_default_main, NULL, > QEMU_THREAD_DETACHED); > > + return qemu_main(); > > + } else { > > + qemu_default_main(NULL); > > + } > > } > > > >
diff --git a/include/qemu-main.h b/include/qemu-main.h index 940960a7dbc..fc70681c8b5 100644 --- a/include/qemu-main.h +++ b/include/qemu-main.h @@ -5,7 +5,24 @@ #ifndef QEMU_MAIN_H #define QEMU_MAIN_H -int qemu_default_main(void); -extern int (*qemu_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 a + * specific thread. + * 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.) + */ +extern qemu_main_fn qemu_main; #endif /* QEMU_MAIN_H */ diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h index 3d84efcac47..b02cfe1f328 100644 --- a/include/qemu/typedefs.h +++ b/include/qemu/typedefs.h @@ -131,5 +131,6 @@ typedef struct IRQState *qemu_irq; * Function types */ typedef void (*qemu_irq_handler)(void *opaque, int n, int level); +typedef int (*qemu_main_fn)(void); #endif /* QEMU_TYPEDEFS_H */ diff --git a/system/main.c b/system/main.c index 9b91d21ea8c..d9397a6d5d0 100644 --- a/system/main.c +++ b/system/main.c @@ -24,13 +24,14 @@ #include "qemu/osdep.h" #include "qemu-main.h" +#include "qemu/main-loop.h" #include "sysemu/sysemu.h" -#ifdef CONFIG_SDL -#include <SDL.h> +#ifdef CONFIG_DARWIN +#include <CoreFoundation/CoreFoundation.h> #endif -int qemu_default_main(void) +static int qemu_default_main(void) { int status; @@ -40,10 +41,49 @@ int qemu_default_main(void) return status; } -int (*qemu_main)(void) = qemu_default_main; +/* + * Various macOS system libraries, including the Cocoa UI and anything using + * libdispatch, such as ParavirtualizedGraphics.framework, requires that the + * main runloop, on the main (initial) thread be running or at least regularly + * polled for events. A special mode is therefore supported, where the QEMU + * main loop runs on a separate thread and the main thread handles the + * CF/Cocoa runloop. + */ + +static void *call_qemu_default_main(void *opaque) +{ + int status; + + bql_lock(); + status = qemu_default_main(); + bql_unlock(); + + exit(status); +} + +#ifdef CONFIG_DARWIN +static int os_darwin_cfrunloop_main(void) +{ + CFRunLoopRun(); + abort(); +} + +qemu_main_fn qemu_main = os_darwin_cfrunloop_main; +#else +qemu_main_fn qemu_main; +#endif int main(int argc, char **argv) { + QemuThread main_loop_thread; + qemu_init(argc, argv); - return qemu_main(); + if (qemu_main) { + qemu_thread_create(&main_loop_thread, "qemu_main", + call_qemu_default_main, NULL, QEMU_THREAD_DETACHED); + bql_unlock(); + return qemu_main(); + } else { + qemu_default_main(); + } } diff --git a/ui/cocoa.m b/ui/cocoa.m index 4c2dd335323..30b8920d929 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); @@ -1321,8 +1324,10 @@ - (void) dealloc { COCOA_DEBUG("QemuCocoaAppController: dealloc\n"); - if (cocoaView) - [cocoaView release]; + [cocoaView release]; + [cbowner release]; + cbowner = nil; + [super dealloc]; } @@ -1938,8 +1943,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); @@ -2002,43 +2005,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"); @@ -2120,8 +2088,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 }; @@ -2185,6 +2151,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 = { diff --git a/ui/gtk.c b/ui/gtk.c index bf9d3dd679a..fbf20161f36 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,13 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts) #ifdef CONFIG_GTK_CLIPBOARD gd_clipboard_init(s); #endif /* CONFIG_GTK_CLIPBOARD */ + + /* + * GTK+ calls must happen on the main thread at least on some platforms, + * and on macOS the main runloop is polled via GTK+'s event handling. + * Don't allow QEMU's event loop to be moved off 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 bd4f5a9da14..44ab2762262 100644 --- a/ui/sdl2.c +++ b/ui/sdl2.c @@ -34,6 +34,7 @@ #include "sysemu/sysemu.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 = {