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

[Peter Maydell]

On Fri, 15 Jul 2022 at 14:19, Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>
> On 2022/07/15 22:10, Peter Maydell wrote:
> > 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 ?
>
> I don't think the diagram is appropriate anymore. It was necessary to
> describe the synchronization between the initial thread and the second
> thread, but they do no longer synchronize at all.

But there are still two threads, and the sequence of events is
not exactly obvious given that things happen in several different
functions. A summary of the expected sequence of events during
startup is still useful to have, I think.

thanks
-- PMM