Re: [PATCH v2 1/3] ui/cocoa: Run qemu_init in the main thread

[Peter Maydell]

On Fri, 15 Jul 2022 at 12:40, Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>
> This work is based on:
> https://patchew.org/QEMU/20220317125534.38706-1-philippe.mathieu.daude@gmail.com/
>
> Simplify the initialization dance by running qemu_init() in the main
> thread before the Cocoa event loop starts. The secondary thread only
> runs only qemu_main_loop() and qemu_cleanup().
>
> This fixes a case where addRemovableDevicesMenuItems() calls
> qmp_query_block() while expecting the main thread to still hold
> the BQL.
>
> Overriding the code after calling qemu_init() is done by dynamically
> replacing a function pointer variable, qemu_main when initializing
> ui/cocoa, which unifies the static implementation of main() for
> builds with ui/cocoa and ones without ui/cocoa.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>

> @@ -585,7 +583,7 @@ - (void) updateUIInfo
>          /*
>           * Don't try to tell QEMU about UI information in the application
>           * startup phase -- we haven't yet registered dcl with the QEMU UI
> -         * layer, and also trying to take the iothread lock would deadlock.
> +         * layer.
>           * When cocoa_display_init() does register the dcl, the UI layer
>           * will call cocoa_switch(), which will call updateUIInfo, so
>           * we don't lose any information here.

This comment says that we can't use the dcl while allow_events is false...

> @@ -778,16 +776,6 @@ - (void) handleMonitorInput:(NSEvent *)event
>
>  - (bool) handleEvent:(NSEvent *)event
>  {
> -    if(!allow_events) {
> -        /*
> -         * Just let OSX have all events that arrive before
> -         * applicationDidFinishLaunching.
> -         * This avoids a deadlock on the iothread lock, which 
> cocoa_display_init()
> -         * will not drop until after the app_started_sem is posted. (In 
> theory
> -         * there should not be any such events, but OSX Catalina now emits 
> some.)
> -         */
> -        return false;
> -    }

...so don't we want to also retain this check of allow_events ?
Much of the code in handleEventLocked assumes the dcl has been registered.

>      return bool_with_iothread_lock(^{
>          return [self handleEventLocked:event];
>      });

> @@ -1915,92 +1898,35 @@ 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 vl.c qemu_main():
> - *
> - * Initial thread:                    2nd thread:
> - * in main():
> - *  create qemu-main thread
> - *  wait on display_init semaphore
> - *                                    call qemu_main()
> - *                                    ...
> - *                                    in cocoa_display_init():
> - *                                     post the display_init semaphore
> - *                                     wait on app_started semaphore
> - *  create application, menus, etc
> - *  enter OSX run loop
> - * in applicationDidFinishLaunching:
> - *  post app_started semaphore
> - *                                     tell main thread to fullscreen if 
> needed
> - *                                    [...]
> - *                                    run qemu main-loop
> - *
> - * We do this in two stages so that we don't do the creation of the
> - * GUI application menus and so on for command line options like --help
> - * where we want to just print text to stdout and exit immediately.

Could we have an updated version of this diagram that explains the
new startup process, please ?

> + * need to start a second thread which runs the qemu_default_main().
>   */

Otherwise this looks good, and it's nice to get rid of that redefine-main
hack.

thanks
-- PMM