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

 include/qemu-main.h     |  3 +--
 include/qemu/typedefs.h |  1 +
 system/main.c           | 50 ++++++++++++++++++++++++++++++++++----
 ui/cocoa.m              | 54 ++++++++++-------------------------------
 ui/sdl2.c               |  4 +++
 5 files changed, 64 insertions(+), 48 deletions(-)

diff --git a/include/qemu-main.h b/include/qemu-main.h
index 940960a7dbc..4bd0d667edc 100644
--- a/include/qemu-main.h
+++ b/include/qemu-main.h
@@ -5,7 +5,6 @@
 #ifndef QEMU_MAIN_H
 #define QEMU_MAIN_H
 
-int qemu_default_main(void);
-extern int (*qemu_main)(void);
+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/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 = {
-- 
2.39.3 (Apple Git-145)

On 2024/11/08 23:46, 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.

GTK should also do the same as SDL and requires treatment; I forgot to 
note that in previous reviews.

> 
> 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.
>   * 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
> 
>   include/qemu-main.h     |  3 +--
>   include/qemu/typedefs.h |  1 +
>   system/main.c           | 50 ++++++++++++++++++++++++++++++++++----
>   ui/cocoa.m              | 54 ++++++++++-------------------------------
>   ui/sdl2.c               |  4 +++
>   5 files changed, 64 insertions(+), 48 deletions(-)
> 
> diff --git a/include/qemu-main.h b/include/qemu-main.h
> index 940960a7dbc..4bd0d667edc 100644
> --- a/include/qemu-main.h
> +++ b/include/qemu-main.h
> @@ -5,7 +5,6 @@
>   #ifndef QEMU_MAIN_H
>   #define QEMU_MAIN_H
>   
> -int qemu_default_main(void);
> -extern int (*qemu_main)(void);
> +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/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 Sun 10. Nov 2024 at 08:01, Akihiko Odaki <akihiko.odaki@daynix.com>
wrote:

> On 2024/11/08 23:46, 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.
>
> GTK should also do the same as SDL and requires treatment; I forgot to
> note that in previous reviews.


Although it‘s possible to build Qemu with GTK support enabled on macOS,
that UI doesn’t actually work on macOS at all, and apparently hasn’t been
supported since 2018, see:
https://stackoverflow.com/a/51474795

I don’t think there’s any point making adjustments to the GTK code by
guessing what might be needed if someone did fix that to work with macOS at
some point.

>

> > 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.
> >   * 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
> >
> >   include/qemu-main.h     |  3 +--
> >   include/qemu/typedefs.h |  1 +
> >   system/main.c           | 50 ++++++++++++++++++++++++++++++++++----
> >   ui/cocoa.m              | 54 ++++++++++-------------------------------
> >   ui/sdl2.c               |  4 +++
> >   5 files changed, 64 insertions(+), 48 deletions(-)
> >
> > diff --git a/include/qemu-main.h b/include/qemu-main.h
> > index 940960a7dbc..4bd0d667edc 100644
> > --- a/include/qemu-main.h
> > +++ b/include/qemu-main.h
> > @@ -5,7 +5,6 @@
> >   #ifndef QEMU_MAIN_H
> >   #define QEMU_MAIN_H
> >
> > -int qemu_default_main(void);
> > -extern int (*qemu_main)(void);
> > +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/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 Sun, Nov 10, 2024 at 08:08:16AM +0100, Phil Dennis-Jordan wrote:
> On Sun 10. Nov 2024 at 08:01, Akihiko Odaki <akihiko.odaki@daynix.com>
> wrote:
> 
> > On 2024/11/08 23:46, 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.
> >
> > GTK should also do the same as SDL and requires treatment; I forgot to
> > note that in previous reviews.
> 
> 
> Although it‘s possible to build Qemu with GTK support enabled on macOS,
> that UI doesn’t actually work on macOS at all, and apparently hasn’t been
> supported since 2018, see:
> https://stackoverflow.com/a/51474795
> 
> I don’t think there’s any point making adjustments to the GTK code by
> guessing what might be needed if someone did fix that to work with macOS at
> some point.

If we don't support GTK on macOS, then we should update meson.build
to actively prevent users enabling GTK on macOS builds, rather than
letting them suffer random runtime crashes.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Mon, 11 Nov 2024 at 10:08, Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Sun, Nov 10, 2024 at 08:08:16AM +0100, Phil Dennis-Jordan wrote:
> > On Sun 10. Nov 2024 at 08:01, Akihiko Odaki <akihiko.odaki@daynix.com>
> > wrote:
> >
> > > On 2024/11/08 23:46, 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.
> > >
> > > GTK should also do the same as SDL and requires treatment; I forgot to
> > > note that in previous reviews.
> >
> >
> > Although it‘s possible to build Qemu with GTK support enabled on macOS,
> > that UI doesn’t actually work on macOS at all, and apparently hasn’t been
> > supported since 2018, see:
> > https://stackoverflow.com/a/51474795
> >
> > I don’t think there’s any point making adjustments to the GTK code by
> > guessing what might be needed if someone did fix that to work with macOS
> at
> > some point.
>
> If we don't support GTK on macOS, then we should update meson.build
> to actively prevent users enabling GTK on macOS builds, rather than
> letting them suffer random runtime crashes.
>
>
Agreed - I'm now more confused than ever though because
https://gitlab.com/qemu-project/qemu/-/issues/2539 sort of implies that
Philippe has previously been using it successfully. Or perhaps this was
created in response to https://gitlab.com/qemu-project/qemu/-/issues/2515 ?
But it seems like even the SDL implementation isn't perfect:
https://gitlab.com/qemu-project/qemu/-/issues/2537

Basically, it seems like Qemu's UI threading on macOS is currently a bit of
a mess, except in the Cocoa UI.

On Mon, 11 Nov 2024, Phil Dennis-Jordan wrote:
> On Mon, 11 Nov 2024 at 10:08, Daniel P. Berrangé <berrange@redhat.com>
> wrote:
>
>> On Sun, Nov 10, 2024 at 08:08:16AM +0100, Phil Dennis-Jordan wrote:
>>> On Sun 10. Nov 2024 at 08:01, Akihiko Odaki <akihiko.odaki@daynix.com>
>>> wrote:
>>>
>>>> On 2024/11/08 23:46, 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.
>>>>
>>>> GTK should also do the same as SDL and requires treatment; I forgot to
>>>> note that in previous reviews.
>>>
>>>
>>> Although it‘s possible to build Qemu with GTK support enabled on macOS,
>>> that UI doesn’t actually work on macOS at all, and apparently hasn’t been
>>> supported since 2018, see:
>>> https://stackoverflow.com/a/51474795
>>>
>>> I don’t think there’s any point making adjustments to the GTK code by
>>> guessing what might be needed if someone did fix that to work with macOS
>> at
>>> some point.
>>
>> If we don't support GTK on macOS, then we should update meson.build
>> to actively prevent users enabling GTK on macOS builds, rather than
>> letting them suffer random runtime crashes.
>>
>>
> Agreed - I'm now more confused than ever though because
> https://gitlab.com/qemu-project/qemu/-/issues/2539 sort of implies that
> Philippe has previously been using it successfully. Or perhaps this was
> created in response to https://gitlab.com/qemu-project/qemu/-/issues/2515 ?
> But it seems like even the SDL implementation isn't perfect:
> https://gitlab.com/qemu-project/qemu/-/issues/2537
>
> Basically, it seems like Qemu's UI threading on macOS is currently a bit of
> a mess, except in the Cocoa UI.

Maybe it worked with older MacOS X releases but broke around the same time 
when commit 5588840ff77800 was needed to fix the cocoa UI? Maybe gtk needs 
a similar fix or whatever cocoa was changed to use since somewhere in gtk 
or QEMU?

Also I find it strange to require UI backends to NULL init a global. If 
the cocoa UI is the only one that needs it could that also set it instead 
of doing it in /system? That would also confine macOS specific code to 
cocoa.m and the other UIs would not need any change that way.

Regards,
BALATON Zoltan

On Mon, 11 Nov 2024 at 13:41, BALATON Zoltan <balaton@eik.bme.hu> wrote:

> On Mon, 11 Nov 2024, Phil Dennis-Jordan wrote:
> > On Mon, 11 Nov 2024 at 10:08, Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> >> On Sun, Nov 10, 2024 at 08:08:16AM +0100, Phil Dennis-Jordan wrote:
> >>> On Sun 10. Nov 2024 at 08:01, Akihiko Odaki <akihiko.odaki@daynix.com>
> >>> wrote:
> >>>
> >>>> On 2024/11/08 23:46, 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.
> >>>>
> >>>> GTK should also do the same as SDL and requires treatment; I forgot to
> >>>> note that in previous reviews.
> >>>
> >>>
> >>> Although it‘s possible to build Qemu with GTK support enabled on macOS,
> >>> that UI doesn’t actually work on macOS at all, and apparently hasn’t
> been
> >>> supported since 2018, see:
> >>> https://stackoverflow.com/a/51474795
> >>>
> >>> I don’t think there’s any point making adjustments to the GTK code by
> >>> guessing what might be needed if someone did fix that to work with
> macOS
> >> at
> >>> some point.
> >>
> >> If we don't support GTK on macOS, then we should update meson.build
> >> to actively prevent users enabling GTK on macOS builds, rather than
> >> letting them suffer random runtime crashes.
> >>
> >>
> > Agreed - I'm now more confused than ever though because
> > https://gitlab.com/qemu-project/qemu/-/issues/2539 sort of implies that
> > Philippe has previously been using it successfully. Or perhaps this was
> > created in response to
> https://gitlab.com/qemu-project/qemu/-/issues/2515 ?
> > But it seems like even the SDL implementation isn't perfect:
> > https://gitlab.com/qemu-project/qemu/-/issues/2537
> >
> > Basically, it seems like Qemu's UI threading on macOS is currently a bit
> of
> > a mess, except in the Cocoa UI.
>
> Maybe it worked with older MacOS X releases but broke around the same time
> when commit 5588840ff77800 was needed to fix the cocoa UI? Maybe gtk needs
> a similar fix or whatever cocoa was changed to use since somewhere in gtk
> or QEMU?
>

Possible! Calling the Cocoa UI APIs from anything other than the main
thread has never officially been supported as far as I'm aware, but perhaps
some subset silently worked on earlier releases. Modern versions definitely
enforce the rule to some extent.

Also I find it strange to require UI backends to NULL init a global. If
> the cocoa UI is the only one that needs it could that also set it instead
> of doing it in /system? That would also confine macOS specific code to
> cocoa.m and the other UIs would not need any change that way.
>

Well, that's the whole point, it's not just the Cocoa UI - other macOS
system frameworks also need a runloop or libdispatch to be handling events
on thread 0. Relevant here are the ParavirtualizedGraphics.framework that's
integrated with patches 2-4 from this patch set, as well as the
Metal.framework, which PVG uses internally, and which we need to use
directly to some extent to complete the generated guest frames. These
frameworks internally use libdispatch, and they just don't work if nothing
is processing events on the main dispatch queue/runloop. So without patch
01/16, PV graphics will only work in conjunction with Cocoa or SDL, because
those do process the runloop on the main thread. But if you were to run
with -nographic, or VNC/SPICE-only, Mac PV graphics just wouldn't work. So
the idea is to uncouple the main thread's runloop from the Cocoa UI.

An then, the GTK+ and SDL libraries themselves call down into Cocoa on
macOS. Windows also has specific rules about its Win32 UI API and
threading. SDL says outright that everything needs to be done from the main
thread. GTK+ says everything needs to be called from a single thread on
Win32; it seems like the rule on macOS is the same, except that thread must
be thread 0. Both those UIs in Qemu seem to violate the threading rules of
the libraries they integrate, as evidenced by the bug reports listed above.
This brokenness is entirely independent of this patch set here, it's just
that we're bumping up against the same underlying issue of needing runloop
handling on thread 0 in macOS.

With regard to NULL'ing the qemu_main function pointer from the SDL or GTK
UIs in Qemu: I had implemented a different approach to solving the problem
in v4, where each UI declared up-front what kind of threading arrangement
it needed rather than each one just overwriting a global variable. This was
somewhat more complex than the current one though.
https://patchew.org/QEMU/20241024102813.9855-1-phil@philjordan.eu/20241024102813.9855-2-phil@philjordan.eu/

Ultimately, it seems like someone needs to take a look at the SDL and GTK
integrations in Qemu and rework them in a way that doesn't violate the SDL
and GTK+ libraries' own threading rules. Once we've figured out what
requirements fall out of that, we can tidy up the qemu_main arrangement.

But that's an undertaking that's out of scope for this patch - I see the
current patch as a step towards a global solution to the problem.
Definitely not the last word on the matter, but at least starting to get
away from the situation where each UI does whatever it wants with zero
regard for the rest of the code base.

On Mon, 11 Nov 2024, Phil Dennis-Jordan wrote:
> On Mon, 11 Nov 2024 at 13:41, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> On Mon, 11 Nov 2024, Phil Dennis-Jordan wrote:
>>> On Mon, 11 Nov 2024 at 10:08, Daniel P. Berrangé <berrange@redhat.com>
>>> wrote:
>>>
>>>> On Sun, Nov 10, 2024 at 08:08:16AM +0100, Phil Dennis-Jordan wrote:
>>>>> On Sun 10. Nov 2024 at 08:01, Akihiko Odaki <akihiko.odaki@daynix.com>
>>>>> wrote:
>>>>>
>>>>>> On 2024/11/08 23:46, 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.
>>>>>>
>>>>>> GTK should also do the same as SDL and requires treatment; I forgot to
>>>>>> note that in previous reviews.
>>>>>
>>>>>
>>>>> Although it‘s possible to build Qemu with GTK support enabled on macOS,
>>>>> that UI doesn’t actually work on macOS at all, and apparently hasn’t
>> been
>>>>> supported since 2018, see:
>>>>> https://stackoverflow.com/a/51474795
>>>>>
>>>>> I don’t think there’s any point making adjustments to the GTK code by
>>>>> guessing what might be needed if someone did fix that to work with
>> macOS
>>>> at
>>>>> some point.
>>>>
>>>> If we don't support GTK on macOS, then we should update meson.build
>>>> to actively prevent users enabling GTK on macOS builds, rather than
>>>> letting them suffer random runtime crashes.
>>>>
>>>>
>>> Agreed - I'm now more confused than ever though because
>>> https://gitlab.com/qemu-project/qemu/-/issues/2539 sort of implies that
>>> Philippe has previously been using it successfully. Or perhaps this was
>>> created in response to
>> https://gitlab.com/qemu-project/qemu/-/issues/2515 ?
>>> But it seems like even the SDL implementation isn't perfect:
>>> https://gitlab.com/qemu-project/qemu/-/issues/2537
>>>
>>> Basically, it seems like Qemu's UI threading on macOS is currently a bit
>> of
>>> a mess, except in the Cocoa UI.
>>
>> Maybe it worked with older MacOS X releases but broke around the same time
>> when commit 5588840ff77800 was needed to fix the cocoa UI? Maybe gtk needs
>> a similar fix or whatever cocoa was changed to use since somewhere in gtk
>> or QEMU?
>>
>
> Possible! Calling the Cocoa UI APIs from anything other than the main
> thread has never officially been supported as far as I'm aware, but perhaps
> some subset silently worked on earlier releases. Modern versions definitely
> enforce the rule to some extent.
>
> Also I find it strange to require UI backends to NULL init a global. If
>> the cocoa UI is the only one that needs it could that also set it instead
>> of doing it in /system? That would also confine macOS specific code to
>> cocoa.m and the other UIs would not need any change that way.
>>
>
> Well, that's the whole point, it's not just the Cocoa UI - other macOS
> system frameworks also need a runloop or libdispatch to be handling events
> on thread 0. Relevant here are the ParavirtualizedGraphics.framework that's
> integrated with patches 2-4 from this patch set, as well as the
> Metal.framework, which PVG uses internally, and which we need to use
> directly to some extent to complete the generated guest frames. These
> frameworks internally use libdispatch, and they just don't work if nothing
> is processing events on the main dispatch queue/runloop. So without patch
> 01/16, PV graphics will only work in conjunction with Cocoa or SDL, because
> those do process the runloop on the main thread. But if you were to run
> with -nographic, or VNC/SPICE-only, Mac PV graphics just wouldn't work. So
> the idea is to uncouple the main thread's runloop from the Cocoa UI.

OK, I think I got that now.

> An then, the GTK+ and SDL libraries themselves call down into Cocoa on
> macOS. Windows also has specific rules about its Win32 UI API and
> threading. SDL says outright that everything needs to be done from the main
> thread. GTK+ says everything needs to be called from a single thread on
> Win32; it seems like the rule on macOS is the same, except that thread must
> be thread 0. Both those UIs in Qemu seem to violate the threading rules of
> the libraries they integrate, as evidenced by the bug reports listed above.
> This brokenness is entirely independent of this patch set here, it's just
> that we're bumping up against the same underlying issue of needing runloop
> handling on thread 0 in macOS.
>
> With regard to NULL'ing the qemu_main function pointer from the SDL or GTK
> UIs in Qemu: I had implemented a different approach to solving the problem
> in v4, where each UI declared up-front what kind of threading arrangement
> it needed rather than each one just overwriting a global variable. This was
> somewhat more complex than the current one though.
> https://patchew.org/QEMU/20241024102813.9855-1-phil@philjordan.eu/20241024102813.9855-2-phil@philjordan.eu/

That does not seem to be better than the current version.

> Ultimately, it seems like someone needs to take a look at the SDL and GTK
> integrations in Qemu and rework them in a way that doesn't violate the SDL
> and GTK+ libraries' own threading rules. Once we've figured out what
> requirements fall out of that, we can tidy up the qemu_main arrangement.
>
> But that's an undertaking that's out of scope for this patch - I see the
> current patch as a step towards a global solution to the problem.
> Definitely not the last word on the matter, but at least starting to get
> away from the situation where each UI does whatever it wants with zero
> regard for the rest of the code base.

Agreed. As long as it's not more broken than it is already, i.e. the 
current behaviour is preserved, that should be good enough but I'm not a 
maintainer who has a word in this so that's just my opinion. Then maybe 
you could add a comment when the global variable needs to be set to NULL 
to note that this may not be the correct way and some hint of the above 
that this may need fixing later. I think we don't have a graphics 
maintainer at the moment who has time to look at this so then at least 
leave a comment to help whoever comes later who wants to fix this 
eventually.

Regards,
BALATON Zoltan

On 2024/11/10 16:08, Phil Dennis-Jordan wrote:
> 
> 
> On Sun 10. Nov 2024 at 08:01, Akihiko Odaki <akihiko.odaki@daynix.com 
> <mailto:akihiko.odaki@daynix.com>> wrote:
> 
>     On 2024/11/08 23:46, 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.
> 
>     GTK should also do the same as SDL and requires treatment; I forgot to
>     note that in previous reviews.
> 
> 
> Although it‘s possible to build Qemu with GTK support enabled on macOS, 
> that UI doesn’t actually work on macOS at all, and apparently hasn’t 
> been supported since 2018, see:
> https://stackoverflow.com/a/51474795 <https://stackoverflow.com/a/51474795>
> 
> I don’t think there’s any point making adjustments to the GTK code by 
> guessing what might be needed if someone did fix that to work with macOS 
> at some point.

But there is a GitLab issue saying it "sometimes" crashes, implying it 
works otherwise:
https://gitlab.com/qemu-project/qemu/-/issues/2539

On Sun, 10 Nov 2024 at 08:10, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/11/10 16:08, Phil Dennis-Jordan wrote:
> >
> >
> > On Sun 10. Nov 2024 at 08:01, Akihiko Odaki <akihiko.odaki@daynix.com
> > <mailto:akihiko.odaki@daynix.com>> wrote:
> >
> >     On 2024/11/08 23:46, 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.
> >
> >     GTK should also do the same as SDL and requires treatment; I forgot to
> >     note that in previous reviews.
> >
> >
> > Although it‘s possible to build Qemu with GTK support enabled on macOS,
> > that UI doesn’t actually work on macOS at all, and apparently hasn’t
> > been supported since 2018, see:
> > https://stackoverflow.com/a/51474795 <https://stackoverflow.com/a/51474795>
> >
> > I don’t think there’s any point making adjustments to the GTK code by
> > guessing what might be needed if someone did fix that to work with macOS
> > at some point.
>
> But there is a GitLab issue saying it "sometimes" crashes, implying it
> works otherwise:
> https://gitlab.com/qemu-project/qemu/-/issues/2539

I spent some time trying to get the GTK GUI working locally on macOS
with no success (with or without the pvg/vmapple patch set) until I
found various info online suggesting it was broken. But I guess I can
add a qemu_main = NULL; line to gtk_display_init(). This will retain
the existing behaviour both on other platforms (where qemu_main is
NULL anyway) and if anyone does manage to use it on macOS.