Re: [PATCH v4 01/15] ui & main loop: Redesign of system-specific main thread event handling

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2024/10/24 19:27, 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 formalises main thread handling. UI (Display) and OS
> platform implementations can declare requirements or preferences:
> 
>   * The Cocoa UI specifies that Qemu's main loop must run on a
>     background thread and provides a function to run on the main thread
>     which runs the NSApplication event handling runloop.
>   * The SDL UI specifies 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.
>   * The Darwin platform provides a default function to run on the
>     main thread, which runs the main CFRunLoop.
>   * Other OSes do not provide their own default main function and thus
>     fall back to running Qemu's main loop on the main thread, as usual.
> 
> 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.
> 
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>

This patch adds a few indirections but I don' see their utilities.
Namely, we can initialize qemu_main with os_darwin_cfrunloop_main ifdef 
CONFIG_DARWIN instead of defining and calling 
os_non_loop_main_thread_fn(). ui/cocoa and ui/sdl2 can also assign 
qemu_main directly.

Regards,
Akihiko Odaki

> ---
>   include/qemu-main.h       |  3 +--
>   include/qemu/typedefs.h   |  1 +
>   include/sysemu/os-posix.h |  2 ++
>   include/sysemu/os-win32.h |  2 ++
>   include/ui/console.h      | 12 +++++++++
>   os-posix.c                | 20 ++++++++++++++
>   system/main.c             | 45 +++++++++++++++++++++++++++-----
>   system/vl.c               |  2 ++
>   ui/cocoa.m                | 55 +++++++++------------------------------
>   ui/console.c              | 32 +++++++++++++++++++++--
>   ui/sdl2.c                 |  2 ++
>   ui/trace-events           |  1 +
>   12 files changed, 123 insertions(+), 54 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/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
> index b881ac6c6f7..51bbb5370e0 100644
> --- a/include/sysemu/os-posix.h
> +++ b/include/sysemu/os-posix.h
> @@ -26,6 +26,7 @@
>   #ifndef QEMU_OS_POSIX_H
>   #define QEMU_OS_POSIX_H
>   
> +#include "qemu/typedefs.h"
>   #include <sys/mman.h>
>   #include <sys/socket.h>
>   #include <netinet/in.h>
> @@ -54,6 +55,7 @@ void os_set_chroot(const char *path);
>   void os_setup_limits(void);
>   void os_setup_post(void);
>   int os_mlock(void);
> +qemu_main_fn os_non_loop_main_thread_fn(void);
>   
>   /**
>    * qemu_alloc_stack:
> diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
> index b82a5d3ad93..db0daba9a52 100644
> --- a/include/sysemu/os-win32.h
> +++ b/include/sysemu/os-win32.h
> @@ -26,6 +26,7 @@
>   #ifndef QEMU_OS_WIN32_H
>   #define QEMU_OS_WIN32_H
>   
> +#include "qemu/typedefs.h"
>   #include <winsock2.h>
>   #include <windows.h>
>   #include <ws2tcpip.h>
> @@ -105,6 +106,7 @@ void os_set_line_buffering(void);
>   void os_setup_early_signal_handling(void);
>   
>   int getpagesize(void);
> +static inline qemu_main_fn os_non_loop_main_thread_fn(void) { return NULL; }
>   
>   #if !defined(EPROTONOSUPPORT)
>   # define EPROTONOSUPPORT EINVAL
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 5832d52a8a6..4e3dc7da146 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -440,6 +440,18 @@ typedef struct QemuDisplay QemuDisplay;
>   
>   struct QemuDisplay {
>       DisplayType type;
> +    /*
> +     * Some UIs have special requirements, for the qemu_main event loop running
> +     * on either the process's initial (main) thread ('Off'), or on an
> +     * explicitly created background thread ('On') because of platform-specific
> +     * event handling.
> +     * The default, 'Auto', indicates the display will work with both setups.
> +     * If 'On', either a qemu_main_thread_fn must be supplied, or it must be
> +     * ensured that all applicable host OS platforms supply a default main.
> +     * (via os_non_loop_main_thread_fn())
> +     */
> +    OnOffAuto qemu_main_on_bg_thread;
> +    qemu_main_fn qemu_main_thread_fn;
>       void (*early_init)(DisplayOptions *opts);
>       void (*init)(DisplayState *ds, DisplayOptions *opts);
>       const char *vc;
> diff --git a/os-posix.c b/os-posix.c
> index 43f9a43f3fe..a173c026f6c 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -37,6 +37,8 @@
>   
>   #ifdef CONFIG_LINUX
>   #include <sys/prctl.h>
> +#elif defined(CONFIG_DARWIN)
> +#include <CoreFoundation/CoreFoundation.h>
>   #endif
>   
>   
> @@ -342,3 +344,21 @@ int os_mlock(void)
>       return -ENOSYS;
>   #endif
>   }
> +
> +#ifdef CONFIG_DARWIN
> +static int os_darwin_cfrunloop_main(void)
> +{
> +    CFRunLoopRun();
> +    abort();
> +}
> +#endif
> +
> +qemu_main_fn os_non_loop_main_thread_fn(void)
> +{
> +#ifdef CONFIG_DARWIN
> +    /* By default, run the OS's event runloop on the main thread. */
> +    return os_darwin_cfrunloop_main;
> +#else
> +    return NULL;
> +#endif
> +}
> diff --git a/system/main.c b/system/main.c
> index 9b91d21ea8c..358eab281b0 100644
> --- a/system/main.c
> +++ b/system/main.c
> @@ -24,13 +24,10 @@
>   
>   #include "qemu/osdep.h"
>   #include "qemu-main.h"
> +#include "qemu/main-loop.h"
>   #include "sysemu/sysemu.h"
>   
> -#ifdef CONFIG_SDL
> -#include <SDL.h>
> -#endif
> -
> -int qemu_default_main(void)
> +static int qemu_default_main(void)
>   {
>       int status;
>   
> @@ -40,10 +37,44 @@ 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);
> +}
> +
> +static void qemu_run_default_main_on_new_thread(void)
> +{
> +    QemuThread thread;
> +
> +    qemu_thread_create(&thread, "qemu_main", call_qemu_default_main,
> +                       NULL, QEMU_THREAD_DETACHED);
> +}
> +
> +qemu_main_fn qemu_main;
>   
>   int main(int argc, char **argv)
>   {
>       qemu_init(argc, argv);
> -    return qemu_main();
> +    if (qemu_main) {
> +        qemu_run_default_main_on_new_thread();
> +        bql_unlock();
> +        return qemu_main();
> +    } else {
> +        qemu_default_main();
> +    }
>   }
> diff --git a/system/vl.c b/system/vl.c
> index e83b3b2608b..c1db20dbee9 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -134,6 +134,7 @@
>   #include "sysemu/iothread.h"
>   #include "qemu/guest-random.h"
>   #include "qemu/keyval.h"
> +#include "qemu-main.h"
>   
>   #define MAX_VIRTIO_CONSOLES 1
>   
> @@ -3667,6 +3668,7 @@ void qemu_init(int argc, char **argv)
>       trace_init_file();
>   
>       qemu_init_main_loop(&error_fatal);
> +    qemu_main = os_non_loop_main_thread_fn();
>       cpu_timers_init();
>   
>       user_register_global_props();
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index 4c2dd335323..393b3800491 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 };
> @@ -2188,8 +2154,11 @@ static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
>   }
>   
>   static QemuDisplay qemu_display_cocoa = {
> -    .type       = DISPLAY_TYPE_COCOA,
> -    .init       = cocoa_display_init,
> +    .type                   = DISPLAY_TYPE_COCOA,
> +    .init                   = cocoa_display_init,
> +    /* The Cocoa UI will run the NSApplication runloop on the main thread.*/
> +    .qemu_main_on_bg_thread = ON_OFF_AUTO_ON,
> +    .qemu_main_thread_fn    = cocoa_main,
>   };
>   
>   static void register_cocoa(void)
> diff --git a/ui/console.c b/ui/console.c
> index 5165f171257..1599d8b7095 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -33,6 +33,7 @@
>   #include "qemu/main-loop.h"
>   #include "qemu/module.h"
>   #include "qemu/option.h"
> +#include "qemu-main.h"
>   #include "chardev/char.h"
>   #include "trace.h"
>   #include "exec/memory.h"
> @@ -1569,12 +1570,39 @@ void qemu_display_early_init(DisplayOptions *opts)
>   
>   void qemu_display_init(DisplayState *ds, DisplayOptions *opts)
>   {
> +    QemuDisplay *display;
> +    bool bg_main_loop;
> +
>       assert(opts->type < DISPLAY_TYPE__MAX);
>       if (opts->type == DISPLAY_TYPE_NONE) {
>           return;
>       }
> -    assert(dpys[opts->type] != NULL);
> -    dpys[opts->type]->init(ds, opts);
> +    display = dpys[opts->type];
> +    assert(display != NULL);
> +    display->init(ds, opts);
> +
> +    switch (display->qemu_main_on_bg_thread) {
> +    case ON_OFF_AUTO_OFF:
> +        bg_main_loop = false;
> +        qemu_main = NULL;
> +        break;
> +    case ON_OFF_AUTO_ON:
> +        bg_main_loop = true;
> +        break;
> +    case ON_OFF_AUTO_AUTO:
> +    default:
> +        bg_main_loop = qemu_main;
> +        break;
> +    }
> +
> +    trace_qemu_display_init_main_thread(
> +        DisplayType_str(display->type), display->qemu_main_thread_fn, qemu_main,
> +        OnOffAuto_lookup.array[display->qemu_main_on_bg_thread],
> +        display->qemu_main_on_bg_thread, bg_main_loop);
> +    if (bg_main_loop && display->qemu_main_thread_fn) {
> +        qemu_main = display->qemu_main_thread_fn;
> +    }
> +    assert(!bg_main_loop || qemu_main);
>   }
>   
>   const char *qemu_display_get_vc(DisplayOptions *opts)
> diff --git a/ui/sdl2.c b/ui/sdl2.c
> index bd4f5a9da14..35e22785119 100644
> --- a/ui/sdl2.c
> +++ b/ui/sdl2.c
> @@ -971,6 +971,8 @@ static QemuDisplay qemu_display_sdl2 = {
>       .type       = DISPLAY_TYPE_SDL,
>       .early_init = sdl2_display_early_init,
>       .init       = sdl2_display_init,
> +    /* SDL must poll for events (via dpy_refresh) on main thread */
> +    .qemu_main_on_bg_thread = ON_OFF_AUTO_OFF,
>   };
>   
>   static void register_sdl1(void)
> diff --git a/ui/trace-events b/ui/trace-events
> index 3da0d5e2800..1e72c967399 100644
> --- a/ui/trace-events
> +++ b/ui/trace-events
> @@ -16,6 +16,7 @@ displaysurface_free(void *display_surface) "surface=%p"
>   displaychangelistener_register(void *dcl, const char *name) "%p [ %s ]"
>   displaychangelistener_unregister(void *dcl, const char *name) "%p [ %s ]"
>   ppm_save(int fd, void *image) "fd=%d image=%p"
> +qemu_display_init_main_thread(const char *display_name, bool qemu_display_sets_main_fn, bool qemu_main_is_set, const char *display_bg_main_loop_preference, int preference, bool bg_main_loop) "display '%s': sets main thread function: %d, platform provides main function: %d, display background main loop preference: %s (%d); main loop will run on background thread: %d"
>   
>   # gtk-egl.c
>   # gtk-gl-area.c