Simplify the initialization dance by running qemu_init() in the main
thread before the Cocoa event loop starts.  The cocoa_display_init()
code that is post-applicationDidFinishLaunching: moves to the
application delegate itself, and the secondary thread only runs
the rest of qemu_main(), namely 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.  The newly-introduced assertions in the block layer
complain about this.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 softmmu/main.c |  12 +++--
 ui/cocoa.m     | 122 +++++++++++++++++++------------------------------
 2 files changed, 53 insertions(+), 81 deletions(-)

diff --git a/softmmu/main.c b/softmmu/main.c
index 639c67ff48..0c4384e980 100644
--- a/softmmu/main.c
+++ b/softmmu/main.c
@@ -39,16 +39,18 @@ int main(int argc, char **argv)
 #endif
 #endif /* CONFIG_SDL */
 
-#ifdef CONFIG_COCOA
-#undef main
-#define main qemu_main
-#endif /* CONFIG_COCOA */
-
+#ifndef CONFIG_COCOA
 int main(int argc, char **argv, char **envp)
 {
+    /*
+     * ui/cocoa.m relies on this being the exact content of main(),
+     * because it has to run everything after qemu_init in a secondary
+     * thread.
+     */
     qemu_init(argc, argv, envp);
     qemu_main_loop();
     qemu_cleanup();
 
     return 0;
 }
+#endif /* CONFIG_COCOA */
diff --git a/ui/cocoa.m b/ui/cocoa.m
index a8f1cdaf92..44d8ea7a39 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -95,14 +95,11 @@ static DisplayChangeListener dcl = {
 };
 static int last_buttons;
 static int cursor_hide = 1;
+static bool full_screen;
 
-static int gArgc;
-static char **gArgv;
 static bool stretch_video;
 static NSTextField *pauseLabel;
 
-static QemuSemaphore display_init_sem;
-static QemuSemaphore app_started_sem;
 static bool allow_events;
 
 static NSInteger cbchangecount = -1;
@@ -140,6 +137,32 @@ static bool bool_with_iothread_lock(BoolCodeBlock block)
     return val;
 }
 
+/*
+ * 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 qemu_main_loop():
+ *
+ * Initial thread:                    2nd thread:
+ * in main():
+ *  qemu_init()
+ *  create application, menus, etc
+ *  enter OSX run loop
+ * in applicationDidFinishLaunching:
+ *  fullscreen if needed
+ *  create main loop thread
+ *                                    call qemu_main_loop()
+ */
+
+static void *call_qemu_main_loop(void *opaque)
+{
+    COCOA_DEBUG("Second thread: calling qemu_main()\n");
+    qemu_main_loop();
+    COCOA_DEBUG("Second thread: qemu_main_loop() returned, exiting\n");
+    qemu_cleanup();
+    [cbowner release];
+    exit(0);
+}
+
 // Mac to QKeyCode conversion
 static const int mac_to_qkeycode_map[] = {
     [kVK_ANSI_A] = Q_KEY_CODE_A,
@@ -708,9 +731,7 @@ QemuCocoaView *cocoaView;
         /*
          * 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.)
+         * This may not be needed anymore?
          */
         return false;
     }
@@ -1185,8 +1206,19 @@ QemuCocoaView *cocoaView;
 {
     COCOA_DEBUG("QemuCocoaAppController: applicationDidFinishLaunching\n");
     allow_events = true;
-    /* Tell cocoa_display_init to proceed */
-    qemu_sem_post(&app_started_sem);
+
+    // register vga output callbacks
+    register_displaychangelistener(&dcl);
+
+    qemu_clipboard_peer_register(&cbpeer);
+    qemu_mutex_unlock_iothread();
+    qemu_thread_create(&thread, "qemu_main_loop", call_qemu_main_loop,
+                       NULL, QEMU_THREAD_DETACHED);
+
+    if (full_screen) {
+        [NSApp activateIgnoringOtherApps: YES];
+        [self toggleFullScreen: nil];
+    }
 }
 
 - (void)applicationWillTerminate:(NSNotification *)aNotification
@@ -1859,60 +1891,14 @@ 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.
- */
-
-static void *call_qemu_main(void *opaque)
-{
-    int status;
-
-    COCOA_DEBUG("Second thread: calling qemu_main()\n");
-    status = qemu_main(gArgc, gArgv, *_NSGetEnviron());
-    COCOA_DEBUG("Second thread: qemu_main() returned, exiting\n");
-    [cbowner release];
-    exit(status);
-}
-
 int main (int argc, char **argv) {
     QemuThread thread;
 
     COCOA_DEBUG("Entered main()\n");
-    gArgc = argc;
-    gArgv = argv;
+    qemu_event_init(&cbevent, false);
 
-    qemu_sem_init(&display_init_sem, 0);
-    qemu_sem_init(&app_started_sem, 0);
-
-    qemu_thread_create(&thread, "qemu_main", call_qemu_main,
-                       NULL, QEMU_THREAD_DETACHED);
-
-    COCOA_DEBUG("Main thread: waiting for display_init_sem\n");
-    qemu_sem_wait(&display_init_sem);
-    COCOA_DEBUG("Main thread: initializing app\n");
+    /* Takes iothread lock, released in applicationDidFinishLaunching:.  */
+    qemu_init(argc, argv);
 
     NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init];
 
@@ -1934,6 +1920,7 @@ int main (int argc, char **argv) {
      */
     add_console_menu_entries();
     addRemovableDevicesMenuItems();
+    cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init];
 
     // Create an Application controller
     QemuCocoaAppController *appController = [[QemuCocoaAppController alloc] init];
@@ -2034,29 +2021,12 @@ static void cocoa_refresh(DisplayChangeListener *dcl)
 static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
 {
     COCOA_DEBUG("qemu_cocoa: cocoa_display_init\n");
-
-    /* Tell main thread to go ahead and create the app and enter the run loop */
-    qemu_sem_post(&display_init_sem);
-    qemu_sem_wait(&app_started_sem);
-    COCOA_DEBUG("cocoa_display_init: app start completed\n");
-
-    /* if fullscreen mode is to be used */
     if (opts->has_full_screen && opts->full_screen) {
-        dispatch_async(dispatch_get_main_queue(), ^{
-            [NSApp activateIgnoringOtherApps: YES];
-            [(QemuCocoaAppController *)[[NSApplication sharedApplication] delegate] toggleFullScreen: nil];
-        });
+        full_screen = 1;
     }
     if (opts->has_show_cursor && opts->show_cursor) {
         cursor_hide = 0;
     }
-
-    // register vga output callbacks
-    register_displaychangelistener(&dcl);
-
-    qemu_event_init(&cbevent, false);
-    cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init];
-    qemu_clipboard_peer_register(&cbpeer);
 }
 
 static QemuDisplay qemu_display_cocoa = {
-- 
2.34.1

On 2022/03/08 0:10, Paolo Bonzini wrote:
> Simplify the initialization dance by running qemu_init() in the main
> thread before the Cocoa event loop starts.  The cocoa_display_init()
> code that is post-applicationDidFinishLaunching: moves to the
> application delegate itself, and the secondary thread only runs
> the rest of qemu_main(), namely 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.  The newly-introduced assertions in the block layer
> complain about this.

Hi,

Thanks for this interesting suggestion. However I don't think this 
improves the situation much. The main contribution of this change is 
that elimination of display_init_sem but it is still necessary for 
command line usage of the executable.

display_init_sem is kind of overloaded has two roles. One is to tell 
that the QEMU is ready to initialize the display. The other is to tell 
if it is going to initialize the display, which would not happen when it 
is used entirely in the command line. The former role can be eliminated 
by waiting for qemu_init, but the latter cannot be.

Regards,
Akihiko Odaki

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   softmmu/main.c |  12 +++--
>   ui/cocoa.m     | 122 +++++++++++++++++++------------------------------
>   2 files changed, 53 insertions(+), 81 deletions(-)
> 
> diff --git a/softmmu/main.c b/softmmu/main.c
> index 639c67ff48..0c4384e980 100644
> --- a/softmmu/main.c
> +++ b/softmmu/main.c
> @@ -39,16 +39,18 @@ int main(int argc, char **argv)
>   #endif
>   #endif /* CONFIG_SDL */
>   
> -#ifdef CONFIG_COCOA
> -#undef main
> -#define main qemu_main
> -#endif /* CONFIG_COCOA */
> -
> +#ifndef CONFIG_COCOA
>   int main(int argc, char **argv, char **envp)
>   {
> +    /*
> +     * ui/cocoa.m relies on this being the exact content of main(),
> +     * because it has to run everything after qemu_init in a secondary
> +     * thread.
> +     */
>       qemu_init(argc, argv, envp);
>       qemu_main_loop();
>       qemu_cleanup();
>   
>       return 0;
>   }
> +#endif /* CONFIG_COCOA */
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index a8f1cdaf92..44d8ea7a39 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -95,14 +95,11 @@ static DisplayChangeListener dcl = {
>   };
>   static int last_buttons;
>   static int cursor_hide = 1;
> +static bool full_screen;
>   
> -static int gArgc;
> -static char **gArgv;
>   static bool stretch_video;
>   static NSTextField *pauseLabel;
>   
> -static QemuSemaphore display_init_sem;
> -static QemuSemaphore app_started_sem;
>   static bool allow_events;
>   
>   static NSInteger cbchangecount = -1;
> @@ -140,6 +137,32 @@ static bool bool_with_iothread_lock(BoolCodeBlock block)
>       return val;
>   }
>   
> +/*
> + * 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 qemu_main_loop():
> + *
> + * Initial thread:                    2nd thread:
> + * in main():
> + *  qemu_init()
> + *  create application, menus, etc
> + *  enter OSX run loop
> + * in applicationDidFinishLaunching:
> + *  fullscreen if needed
> + *  create main loop thread
> + *                                    call qemu_main_loop()
> + */
> +
> +static void *call_qemu_main_loop(void *opaque)
> +{
> +    COCOA_DEBUG("Second thread: calling qemu_main()\n");
> +    qemu_main_loop();
> +    COCOA_DEBUG("Second thread: qemu_main_loop() returned, exiting\n");
> +    qemu_cleanup();
> +    [cbowner release];
> +    exit(0);
> +}
> +
>   // Mac to QKeyCode conversion
>   static const int mac_to_qkeycode_map[] = {
>       [kVK_ANSI_A] = Q_KEY_CODE_A,
> @@ -708,9 +731,7 @@ QemuCocoaView *cocoaView;
>           /*
>            * 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.)
> +         * This may not be needed anymore?
>            */
>           return false;
>       }
> @@ -1185,8 +1206,19 @@ QemuCocoaView *cocoaView;
>   {
>       COCOA_DEBUG("QemuCocoaAppController: applicationDidFinishLaunching\n");
>       allow_events = true;
> -    /* Tell cocoa_display_init to proceed */
> -    qemu_sem_post(&app_started_sem);
> +
> +    // register vga output callbacks
> +    register_displaychangelistener(&dcl);
> +
> +    qemu_clipboard_peer_register(&cbpeer);
> +    qemu_mutex_unlock_iothread();
> +    qemu_thread_create(&thread, "qemu_main_loop", call_qemu_main_loop,
> +                       NULL, QEMU_THREAD_DETACHED);
> +
> +    if (full_screen) {
> +        [NSApp activateIgnoringOtherApps: YES];
> +        [self toggleFullScreen: nil];
> +    }
>   }
>   
>   - (void)applicationWillTerminate:(NSNotification *)aNotification
> @@ -1859,60 +1891,14 @@ 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.
> - */
> -
> -static void *call_qemu_main(void *opaque)
> -{
> -    int status;
> -
> -    COCOA_DEBUG("Second thread: calling qemu_main()\n");
> -    status = qemu_main(gArgc, gArgv, *_NSGetEnviron());
> -    COCOA_DEBUG("Second thread: qemu_main() returned, exiting\n");
> -    [cbowner release];
> -    exit(status);
> -}
> -
>   int main (int argc, char **argv) {
>       QemuThread thread;
>   
>       COCOA_DEBUG("Entered main()\n");
> -    gArgc = argc;
> -    gArgv = argv;
> +    qemu_event_init(&cbevent, false);
>   
> -    qemu_sem_init(&display_init_sem, 0);
> -    qemu_sem_init(&app_started_sem, 0);
> -
> -    qemu_thread_create(&thread, "qemu_main", call_qemu_main,
> -                       NULL, QEMU_THREAD_DETACHED);
> -
> -    COCOA_DEBUG("Main thread: waiting for display_init_sem\n");
> -    qemu_sem_wait(&display_init_sem);
> -    COCOA_DEBUG("Main thread: initializing app\n");
> +    /* Takes iothread lock, released in applicationDidFinishLaunching:.  */
> +    qemu_init(argc, argv);
>   
>       NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init];
>   
> @@ -1934,6 +1920,7 @@ int main (int argc, char **argv) {
>        */
>       add_console_menu_entries();
>       addRemovableDevicesMenuItems();
> +    cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init];
>   
>       // Create an Application controller
>       QemuCocoaAppController *appController = [[QemuCocoaAppController alloc] init];
> @@ -2034,29 +2021,12 @@ static void cocoa_refresh(DisplayChangeListener *dcl)
>   static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
>   {
>       COCOA_DEBUG("qemu_cocoa: cocoa_display_init\n");
> -
> -    /* Tell main thread to go ahead and create the app and enter the run loop */
> -    qemu_sem_post(&display_init_sem);
> -    qemu_sem_wait(&app_started_sem);
> -    COCOA_DEBUG("cocoa_display_init: app start completed\n");
> -
> -    /* if fullscreen mode is to be used */
>       if (opts->has_full_screen && opts->full_screen) {
> -        dispatch_async(dispatch_get_main_queue(), ^{
> -            [NSApp activateIgnoringOtherApps: YES];
> -            [(QemuCocoaAppController *)[[NSApplication sharedApplication] delegate] toggleFullScreen: nil];
> -        });
> +        full_screen = 1;
>       }
>       if (opts->has_show_cursor && opts->show_cursor) {
>           cursor_hide = 0;
>       }
> -
> -    // register vga output callbacks
> -    register_displaychangelistener(&dcl);
> -
> -    qemu_event_init(&cbevent, false);
> -    cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init];
> -    qemu_clipboard_peer_register(&cbpeer);
>   }
>   
>   static QemuDisplay qemu_display_cocoa = {

On Mon, 7 Mar 2022 at 15:34, Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>
> On 2022/03/08 0:10, Paolo Bonzini wrote:
> > Simplify the initialization dance by running qemu_init() in the main
> > thread before the Cocoa event loop starts.  The cocoa_display_init()
> > code that is post-applicationDidFinishLaunching: moves to the
> > application delegate itself, and the secondary thread only runs
> > the rest of qemu_main(), namely 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.  The newly-introduced assertions in the block layer
> > complain about this.
>
> Hi,
>
> Thanks for this interesting suggestion. However I don't think this
> improves the situation much. The main contribution of this change is
> that elimination of display_init_sem but it is still necessary for
> command line usage of the executable.

The main benefit of Paolo's suggestion from my point of view is
that it entirely eliminates the odd situation where cocoa.m wants to
make calls back into QEMU code where it does not itself hold
the iothread lock in the current thread, but instead knows that
some other thread does and is waiting for it. Instead we end up
with a much more straightforward situation of "every time we
call into QEMU code we hold the iothread lock directly ourselves".

thanks
-- PMM

On Tue, Mar 8, 2022 at 1:14 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 7 Mar 2022 at 15:34, Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
> >
> > On 2022/03/08 0:10, Paolo Bonzini wrote:
> > > Simplify the initialization dance by running qemu_init() in the main
> > > thread before the Cocoa event loop starts.  The cocoa_display_init()
> > > code that is post-applicationDidFinishLaunching: moves to the
> > > application delegate itself, and the secondary thread only runs
> > > the rest of qemu_main(), namely 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.  The newly-introduced assertions in the block layer
> > > complain about this.
> >
> > Hi,
> >
> > Thanks for this interesting suggestion. However I don't think this
> > improves the situation much. The main contribution of this change is
> > that elimination of display_init_sem but it is still necessary for
> > command line usage of the executable.
>
> The main benefit of Paolo's suggestion from my point of view is
> that it entirely eliminates the odd situation where cocoa.m wants to
> make calls back into QEMU code where it does not itself hold
> the iothread lock in the current thread, but instead knows that
> some other thread does and is waiting for it. Instead we end up
> with a much more straightforward situation of "every time we
> call into QEMU code we hold the iothread lock directly ourselves".
>
> thanks
> -- PMM

The current cocoa.m somehows calls back into QEMU code in main, but
that is not necessary as demonstrated in:
https://patchew.org/QEMU/20220307134946.61407-1-akihiko.odaki@gmail.com/

With the code is moved, it becomes only a difference of the place
where the code without iothread is located, in main or in
[-QemuCocoaAppController applicationDidFinishLaunching:].

Regards,
Akihiko Odaki

On Mon, 7 Mar 2022 at 16:27, Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
> On Tue, Mar 8, 2022 at 1:14 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> > The main benefit of Paolo's suggestion from my point of view is
> > that it entirely eliminates the odd situation where cocoa.m wants to
> > make calls back into QEMU code where it does not itself hold
> > the iothread lock in the current thread, but instead knows that
> > some other thread does and is waiting for it. Instead we end up
> > with a much more straightforward situation of "every time we
> > call into QEMU code we hold the iothread lock directly ourselves".

> The current cocoa.m somehows calls back into QEMU code in main, but
> that is not necessary as demonstrated in:
> https://patchew.org/QEMU/20220307134946.61407-1-akihiko.odaki@gmail.com/
>
> With the code is moved, it becomes only a difference of the place
> where the code without iothread is located, in main or in
> [-QemuCocoaAppController applicationDidFinishLaunching:].

That series doesn't remove the general design that has quite a bit
of "we know some other thread holds the lock and waits for us" code.
It also gives us the opposite problem that we're now calling a lot
of Cocoa APIs from something other than the main Cocoa thread.

thanks
-- PMM

On Tue, Mar 8, 2022 at 1:35 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 7 Mar 2022 at 16:27, Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
> > On Tue, Mar 8, 2022 at 1:14 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> > > The main benefit of Paolo's suggestion from my point of view is
> > > that it entirely eliminates the odd situation where cocoa.m wants to
> > > make calls back into QEMU code where it does not itself hold
> > > the iothread lock in the current thread, but instead knows that
> > > some other thread does and is waiting for it. Instead we end up
> > > with a much more straightforward situation of "every time we
> > > call into QEMU code we hold the iothread lock directly ourselves".
>
> > The current cocoa.m somehows calls back into QEMU code in main, but
> > that is not necessary as demonstrated in:
> > https://patchew.org/QEMU/20220307134946.61407-1-akihiko.odaki@gmail.com/
> >
> > With the code is moved, it becomes only a difference of the place
> > where the code without iothread is located, in main or in
> > [-QemuCocoaAppController applicationDidFinishLaunching:].
>
> That series doesn't remove the general design that has quite a bit
> of "we know some other thread holds the lock and waits for us" code.

Well, I don't think so. main no longer calls back QEMU code (and it
should never do so in my opinion).

> It also gives us the opposite problem that we're now calling a lot
> of Cocoa APIs from something other than the main Cocoa thread.

Basically NSView is the only thing prohibited from calling from
another thread so it shouldn't be a problem.

Regards,
Akihiko Odaki

>
> thanks
> -- PMM

On 3/7/22 17:41, Akihiko Odaki wrote:
>> That series doesn't remove the general design that has quite a bit
>> of "we know some other thread holds the lock and waits for us" code.
> Well, I don't think so. main no longer calls back QEMU code (and it
> should never do so in my opinion).

That's an arbitrary limitation.  With my patch, until [NSApp run] only 
the main thread has the lock and therefore it can do anything.  In my 
patch I decided to minimize the changes, but if 
register_displaychangelistener() and qemu_clipboard_peer_register() can 
be moved earlier, then the iothread could even be unlocked before [NSApp 
run].

There's also the advantage that the flow is a lot more similar between 
"-display cocoa" and "-display none", and also between --enable-cocoa 
and --disable-cocoa builds.

Then main() remains as

     /* doesn't hurt in the !cocoa case */
     NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init];

     qemu_init(argc, argv);
     if (!have_cocoa_display) {
         qemu_main_loop();
         qemu_cleanup();
     } else {
         QemuCocoaAppController *appController = 
[[QemuCocoaAppController alloc] init];
         [NSApp setDelegate:appController];
         [NSApp run];
         [appController release];
     }
     [pool release];
     return 0;

which is in my opinion the best of both worlds.  But my patch (assuming 
it works) really fixes the threading model.  Without it, moving menus to 
the iothread makes it even more complicated.

>> It also gives us the opposite problem that we're now calling a lot
>> of Cocoa APIs from something other than the main Cocoa thread.
> Basically NSView is the only thing prohibited from calling from
> another thread so it shouldn't be a problem.

I'd rather have a single thread that does all things related to Cocoa. 
If my patch doesn't work, I would just call qmp_query_block(NULL) from 
cocoa_display_init() and save the result in a global variable.

Paolo

On Tue, Mar 8, 2022 at 2:21 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 3/7/22 17:41, Akihiko Odaki wrote:
> >> That series doesn't remove the general design that has quite a bit
> >> of "we know some other thread holds the lock and waits for us" code.
> > Well, I don't think so. main no longer calls back QEMU code (and it
> > should never do so in my opinion).
>
> That's an arbitrary limitation.  With my patch, until [NSApp run] only
> the main thread has the lock and therefore it can do anything.  In my
> patch I decided to minimize the changes, but if
> register_displaychangelistener() and qemu_clipboard_peer_register() can
> be moved earlier, then the iothread could even be unlocked before [NSApp
> run].

I meant the thread without iothread lock shouldn't call QEMU functions
even if it is clear that another thread is holding the lock and
waiting. The rule should apply after unlocking the mutex in
[-QemuCocoaAppController applicationDidFinishLaunching:].

>
> There's also the advantage that the flow is a lot more similar between
> "-display cocoa" and "-display none", and also between --enable-cocoa
> and --disable-cocoa builds.
>
> Then main() remains as
>
>      /* doesn't hurt in the !cocoa case */
>      NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init];
>
>      qemu_init(argc, argv);
>      if (!have_cocoa_display) {
>          qemu_main_loop();
>          qemu_cleanup();
>      } else {
>          QemuCocoaAppController *appController =
> [[QemuCocoaAppController alloc] init];
>          [NSApp setDelegate:appController];
>          [NSApp run];
>          [appController release];
>      }
>      [pool release];
>      return 0;
>
> which is in my opinion the best of both worlds.  But my patch (assuming
> it works) really fixes the threading model.  Without it, moving menus to
> the iothread makes it even more complicated.

I don't think the threading model is broken in the current code. There
is always the iothread and main thread and ui/cocoa has to be aware of
that. We could unify them when initializing but it is exceptional. The
broken part is the menu creation code which touches iothread.

The nice part of this change is that it gives control on how to model
threading. It can provide a conventional model when running without
cocoa, and leave a room for changes like moving all of the
initialization code before [+NSApp run].

>
> >> It also gives us the opposite problem that we're now calling a lot
> >> of Cocoa APIs from something other than the main Cocoa thread.
> > Basically NSView is the only thing prohibited from calling from
> > another thread so it shouldn't be a problem.
>
> I'd rather have a single thread that does all things related to Cocoa.
> If my patch doesn't work, I would just call qmp_query_block(NULL) from
> cocoa_display_init() and save the result in a global variable.

The distinction between Cocoa and other platform APIs is arbitrary.
The APIs of Cocoa are mostly thread-independent and NSView is rather
exceptional.

Regards,
Akihiko Odaki

>
> Paolo

On 3/7/22 16:34, Akihiko Odaki wrote:
> Thanks for this interesting suggestion. However I don't think this 
> improves the situation much. The main contribution of this change is 
> that elimination of display_init_sem but it is still necessary for 
> command line usage of the executable.
> 
> display_init_sem is kind of overloaded has two roles. One is to tell 
> that the QEMU is ready to initialize the display. The other is to tell 
> if it is going to initialize the display, which would not happen when it 
> is used entirely in the command line. The former role can be eliminated 
> by waiting for qemu_init, but the latter cannot be.

This is easy to account for.  On top of this patch:

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 44d8ea7a39..3903fa4b9b 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -96,6 +96,7 @@ static DisplayChangeListener dcl = {
  static int last_buttons;
  static int cursor_hide = 1;
  static bool full_screen;
+static bool have_cocoa_ui;
  
  static bool stretch_video;
  static NSTextField *pauseLabel;
@@ -1899,6 +1900,11 @@ int main (int argc, char **argv) {
  
      /* Takes iothread lock, released in applicationDidFinishLaunching:.  */
      qemu_init(argc, argv);
+    if (!have_cocoa_ui) {
+         qemu_main_loop();
+         qemu_cleanup();
+         return 0;
+    }
  
      NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init];
  
@@ -2021,6 +2027,7 @@ static void cocoa_refresh(DisplayChangeListener *dcl)
  static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
  {
      COCOA_DEBUG("qemu_cocoa: cocoa_display_init\n");
+    have_cocoa_ui = 1;
      if (opts->has_full_screen && opts->full_screen) {
          full_screen = 1;
      }

Paolo

On Tue, Mar 8, 2022 at 1:39 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 3/7/22 16:34, Akihiko Odaki wrote:
> > Thanks for this interesting suggestion. However I don't think this
> > improves the situation much. The main contribution of this change is
> > that elimination of display_init_sem but it is still necessary for
> > command line usage of the executable.
> >
> > display_init_sem is kind of overloaded has two roles. One is to tell
> > that the QEMU is ready to initialize the display. The other is to tell
> > if it is going to initialize the display, which would not happen when it
> > is used entirely in the command line. The former role can be eliminated
> > by waiting for qemu_init, but the latter cannot be.
>
> This is easy to account for.  On top of this patch:
>
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index 44d8ea7a39..3903fa4b9b 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -96,6 +96,7 @@ static DisplayChangeListener dcl = {
>   static int last_buttons;
>   static int cursor_hide = 1;
>   static bool full_screen;
> +static bool have_cocoa_ui;
>
>   static bool stretch_video;
>   static NSTextField *pauseLabel;
> @@ -1899,6 +1900,11 @@ int main (int argc, char **argv) {
>
>       /* Takes iothread lock, released in applicationDidFinishLaunching:.  */
>       qemu_init(argc, argv);
> +    if (!have_cocoa_ui) {
> +         qemu_main_loop();
> +         qemu_cleanup();
> +         return 0;
> +    }
>
>       NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init];
>
> @@ -2021,6 +2027,7 @@ static void cocoa_refresh(DisplayChangeListener *dcl)
>   static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
>   {
>       COCOA_DEBUG("qemu_cocoa: cocoa_display_init\n");
> +    have_cocoa_ui = 1;
>       if (opts->has_full_screen && opts->full_screen) {
>           full_screen = 1;
>       }
>
> Paolo

This would allow to have gtk and sdl2 in the same binary.

Regards,
Akihiko Odaki

On 3/7/22 16:10, Paolo Bonzini wrote:
> +    COCOA_DEBUG("Second thread: calling qemu_main()\n");
> +    qemu_main_loop();

This should do qemu_mutex_lock_iothread() before calling qemu_main_loop().

Paolo

On 7/3/22 16:10, Paolo Bonzini wrote:
> Simplify the initialization dance by running qemu_init() in the main
> thread before the Cocoa event loop starts.  The cocoa_display_init()
> code that is post-applicationDidFinishLaunching: moves to the
> application delegate itself, and the secondary thread only runs
> the rest of qemu_main(), namely 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.  The newly-introduced assertions in the block layer
> complain about this.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   softmmu/main.c |  12 +++--
>   ui/cocoa.m     | 122 +++++++++++++++++++------------------------------
>   2 files changed, 53 insertions(+), 81 deletions(-)

I got this building and it indeed fixes the issue reported here:
https://lore.kernel.org/qemu-devel/cecef6bd-951a-aab6-e603-96e3551e3e9e@gmail.com/

I will post as v2 and let you iterate :)