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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi,

On 23/12/24 23:16, 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.
> 
> Implementing this via a global function pointer variable is a bit
> ugly, but it's probably worth investigating the existing UI thread rule
> violations in the SDL (e.g. #2537) and GTK+ back-ends. Fixing those
> issues might precipitate requirements similar but not identical to those
> of the Cocoa UI; hopefully we'll see some kind of pattern emerge, which
> can then be used as a basis for an overhaul. (In fact, it may turn
> out to be simplest to split the UI/native platform event thread from the
> QEMU main event loop on all platforms, with any UI or even none at all.)
> 
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Tested-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---


>   include/qemu-main.h | 14 +++++++++++-
>   system/main.c       | 37 +++++++++++++++++++++++++++----
>   ui/cocoa.m          | 54 +++++++++++----------------------------------
>   ui/gtk.c            |  4 ++++
>   ui/sdl2.c           |  4 ++++
>   5 files changed, 67 insertions(+), 46 deletions(-)
> 
> diff --git a/include/qemu-main.h b/include/qemu-main.h
> index 940960a7db..2ee83bedff 100644
> --- a/include/qemu-main.h
> +++ b/include/qemu-main.h
> @@ -5,7 +5,19 @@
>   #ifndef QEMU_MAIN_H
>   #define QEMU_MAIN_H
>   
> -int qemu_default_main(void);
> +/*
> + * The function to run on the main (initial) thread of the process.
> + * NULL means QEMU's main event loop.
> + * When non-NULL, QEMU's main event loop will run on a purposely created
> + * thread, after which the provided function pointer will be invoked on
> + * the initial thread.
> + * This is useful on platforms which treat the main thread as special
> + * (macOS/Darwin) and/or require all UI API calls to occur from the main
> + * thread. Those platforms can initialise it to a specific function,
> + * while UI implementations may reset it to NULL during their init if they
> + * will handle system and UI events on the main thread via QEMU's own main
> + * event loop.
> + */
>   extern int (*qemu_main)(void);
>   
>   #endif /* QEMU_MAIN_H */


> diff --git a/ui/gtk.c b/ui/gtk.c
> index 0d38c070e4..c023743148 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -38,6 +38,7 @@
>   #include "qemu/cutils.h"
>   #include "qemu/error-report.h"
>   #include "qemu/main-loop.h"
> +#include "qemu-main.h"
>   
>   #include "ui/console.h"
>   #include "ui/gtk.h"
> @@ -2485,6 +2486,9 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
>   #ifdef CONFIG_GTK_CLIPBOARD
>       gd_clipboard_init(s);
>   #endif /* CONFIG_GTK_CLIPBOARD */
> +
> +    /* GTK's event polling must happen on the main thread. */
> +    qemu_main = NULL;
>   }
>   
>   static void early_gtk_display_init(DisplayOptions *opts)
> diff --git a/ui/sdl2.c b/ui/sdl2.c
> index 1fb72f67a6..445eb1dd9f 100644
> --- a/ui/sdl2.c
> +++ b/ui/sdl2.c
> @@ -34,6 +34,7 @@
>   #include "system/system.h"
>   #include "ui/win32-kbd-hook.h"
>   #include "qemu/log.h"
> +#include "qemu-main.h"
>   
>   static int sdl2_num_outputs;
>   static struct sdl2_console *sdl2_console;
> @@ -965,6 +966,9 @@ static void sdl2_display_init(DisplayState *ds, DisplayOptions *o)
>       }
>   
>       atexit(sdl_cleanup);
> +
> +    /* SDL's event polling (in dpy_refresh) must happen on the main thread. */
> +    qemu_main = NULL;
>   }
>   
>   static QemuDisplay qemu_display_sdl2 = {

This fails the build-oss-fuzz job as:

     /usr/bin/ld: libcommon.a.p/ui_gtk.c.o: in function `gtk_display_init':
     ../ui/gtk.c:2491:(.text+0x3df9): undefined reference to `qemu_main'
     /usr/bin/ld: ../ui/gtk.c:2491:(.text+0x45f1): undefined reference 
to `qemu_main'
     /usr/bin/ld: libcommon.a.p/ui_sdl2.c.o: in function 
`sdl2_display_init':
     ../ui/sdl2.c:971:(.text+0x2e8c): undefined reference to `qemu_main'
     /usr/bin/ld: ../ui/sdl2.c:971:(.text+0x30bf): undefined reference 
to `qemu_main'
     clang: error: linker command failed with exit code 1 (use -v to see 
invocation)

Therefore I'm squashing:

-- >8 --
diff --git a/tests/qtest/fuzz/fuzz.c b/tests/qtest/fuzz/fuzz.c
index 8274000bd55..ca248a51a6c 100644
--- a/tests/qtest/fuzz/fuzz.c
+++ b/tests/qtest/fuzz/fuzz.c
@@ -41,6 +41,7 @@ static FuzzTargetList *fuzz_target_list;
  static FuzzTarget *fuzz_target;
  static QTestState *fuzz_qts;

+int (*qemu_main)(void);


  void flush_events(QTestState *s)
---

Regards,

Phil.