| Message ID | 20220317115644.37276-3-philippe.mathieu.daude@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cocoa: run qemu_init in the main thread | expand |
On 17/3/22 12:56, Philippe Mathieu-Daudé wrote: > From: Paolo Bonzini <pbonzini@redhat.com> > > 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> > Message-Id: <20220307151004.578069-1-pbonzini@redhat.com> > [PMD: Fixed trivial build failures & rebased] > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > softmmu/main.c | 12 +++--- > ui/cocoa.m | 114 ++++++++++++++++++++----------------------------- > 2 files changed, 54 insertions(+), 72 deletions(-) > diff --git a/ui/cocoa.m b/ui/cocoa.m > index 027c3053f7..2e5130d805 100644 > --- a/ui/cocoa.m > +++ b/ui/cocoa.m > @@ -100,14 +100,13 @@ static int last_buttons; > static int cursor_hide = 1; > static int left_command_key_enabled = 1; > static bool swap_opt_cmd; > +static bool full_screen; > +static bool full_grab; > +static bool have_cocoa_ui; > > -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; > @@ -115,6 +114,7 @@ static QemuClipboardInfo *cbinfo; > static QemuClipboardPeer cbpeer; > static QemuCocoaPasteboardTypeOwner *cbowner; > static QemuEvent cbevent; > +static QemuThread main_thread; This variable doesn't need to be in global scope, I'll move it to applicationDidFinishLaunching(). > @@ -1323,8 +1310,22 @@ static CGEventRef handleTapEvent(CGEventTapProxy proxy, CGEventType type, CGEven > { > 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(&main_thread, "qemu_main_loop", call_qemu_main_loop, > + NULL, QEMU_THREAD_DETACHED); > + > + if (full_screen) { > + [NSApp activateIgnoringOtherApps: YES]; > + [self toggleFullScreen: nil]; > + } > + if (full_grab) { > + [self setFullGrab: nil]; > + } > }
On Thu, 17 Mar 2022 at 11:57, Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> wrote: > > From: Paolo Bonzini <pbonzini@redhat.com> > > 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> > Message-Id: <20220307151004.578069-1-pbonzini@redhat.com> > [PMD: Fixed trivial build failures & rebased] > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> (with or without the tweak you just mentioned for the main_thread variable) thanks -- PMM
On Thu, Mar 17, 2022 at 8:57 PM Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> wrote: > > From: Paolo Bonzini <pbonzini@redhat.com> > > 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> > Message-Id: <20220307151004.578069-1-pbonzini@redhat.com> > [PMD: Fixed trivial build failures & rebased] > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > softmmu/main.c | 12 +++--- > ui/cocoa.m | 114 ++++++++++++++++++++----------------------------- > 2 files changed, 54 insertions(+), 72 deletions(-) I realized this actually breaks the "runas" option with ui/cocoa. [+NSApplication sharedApplication] calls issetugid() to see if setgid() or setuid() is called before and calls exit() if it evaluates true. It does not evaluate true without this patch since setgid() and setuid() are called after [+NSApplication sharedApplication]. This patch, however, changes the order and triggers the check. There are two options to solve the problem: 1. Move setgid and setuid calls after [+NSApplication sharedApplication] to let NSApplication initialize as the original user. 2. Do: [[NSUserDefaults standardUserDefaults] setBool:YES forKey:@"_NSAppAllowsNonTrustedUGID"] Option 2 would be preferred in terms of practicality since nobody would want to initialize NSApplication as the original user (usually superuser). However, _NSAppAllowsNonTrustedUGID is not documented by Apple. > > 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 027c3053f7..2e5130d805 100644 > --- a/ui/cocoa.m > +++ b/ui/cocoa.m > @@ -100,14 +100,13 @@ static int last_buttons; > static int cursor_hide = 1; > static int left_command_key_enabled = 1; > static bool swap_opt_cmd; > +static bool full_screen; > +static bool full_grab; > +static bool have_cocoa_ui; > > -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; > @@ -115,6 +114,7 @@ static QemuClipboardInfo *cbinfo; > static QemuClipboardPeer cbpeer; > static QemuCocoaPasteboardTypeOwner *cbowner; > static QemuEvent cbevent; > +static QemuThread main_thread; > > // Utility functions to run specified code block with iothread lock held > typedef void (^CodeBlock)(void); > @@ -150,39 +150,28 @@ static bool bool_with_iothread_lock(BoolCodeBlock block) > /* > * 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(): > + * need to start a second thread which runs qemu_main_loop(): > * > * 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 > + * qemu_init() > * 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. > + * fullscreen if needed > + * create main loop thread > + * call qemu_main_loop() > */ > > -static void *call_qemu_main(void *opaque) > +static void *call_qemu_main_loop(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"); > + COCOA_DEBUG("Second thread: calling qemu_main_loop()\n"); > + qemu_mutex_lock_iothread(); > + qemu_main_loop(); > + COCOA_DEBUG("Second thread: qemu_main_loop() returned, exiting\n"); > + qemu_cleanup(); > [cbowner release]; > - exit(status); > + exit(0); > } > > // Mac to QKeyCode conversion > @@ -823,9 +812,7 @@ static CGEventRef handleTapEvent(CGEventTapProxy proxy, CGEventType type, CGEven > /* > * 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; > } > @@ -1323,8 +1310,22 @@ static CGEventRef handleTapEvent(CGEventTapProxy proxy, CGEventType type, CGEven > { > 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(&main_thread, "qemu_main_loop", call_qemu_main_loop, > + NULL, QEMU_THREAD_DETACHED); > + > + if (full_screen) { > + [NSApp activateIgnoringOtherApps: YES]; > + [self toggleFullScreen: nil]; > + } > + if (full_grab) { > + [self setFullGrab: nil]; > + } > } > > - (void)applicationWillTerminate:(NSNotification *)aNotification > @@ -1941,22 +1942,18 @@ static void cocoa_clipboard_request(QemuClipboardInfo *info, > } > } > > -int main (int argc, char **argv) { > - QemuThread thread; > - > +int main(int argc, char **argv, char **envp) > +{ > 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, envp); > + if (!have_cocoa_ui) { > + qemu_main_loop(); > + qemu_cleanup(); > + return 0; > + } > > NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init]; > > @@ -1978,6 +1975,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]; > @@ -2071,24 +2069,13 @@ 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; > > - /* 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"); > - > - QemuCocoaAppController *controller = (QemuCocoaAppController *)[[NSApplication sharedApplication] delegate]; > - /* if fullscreen mode is to be used */ > if (opts->has_full_screen && opts->full_screen) { > - dispatch_async(dispatch_get_main_queue(), ^{ > - [NSApp activateIgnoringOtherApps: YES]; > - [controller toggleFullScreen: nil]; > - }); > + full_screen = 1; In case you missed an earlier comment: you may simply store opts and use it later. sdl2 already does this. Regards, Akihiko Odaki > } > if (opts->u.cocoa.has_full_grab && opts->u.cocoa.full_grab) { > - dispatch_async(dispatch_get_main_queue(), ^{ > - [controller setFullGrab: nil]; > - }); > + full_grab = 1; > } > > if (opts->has_show_cursor && opts->show_cursor) { > @@ -2101,13 +2088,6 @@ static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts) > if (opts->u.cocoa.has_left_command_key && !opts->u.cocoa.left_command_key) { > left_command_key_enabled = 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 Thu, Mar 17, 2022 at 8:57 PM Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> wrote: > > From: Paolo Bonzini <pbonzini@redhat.com> > > 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> > Message-Id: <20220307151004.578069-1-pbonzini@redhat.com> > [PMD: Fixed trivial build failures & rebased] > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > softmmu/main.c | 12 +++--- > ui/cocoa.m | 114 ++++++++++++++++++++----------------------------- > 2 files changed, 54 insertions(+), 72 deletions(-) The following lines which will be out-dated. > /* > * Create the menu entries which depend on QEMU state (for consoles > * and removeable devices). These make calls back into QEMU functions, > * which is OK because at this point we know that the second thread > * holds the iothread lock and is synchronously waiting for us to > * finish. > */ Regards, Akihiko Odaki
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 027c3053f7..2e5130d805 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -100,14 +100,13 @@ static int last_buttons; static int cursor_hide = 1; static int left_command_key_enabled = 1; static bool swap_opt_cmd; +static bool full_screen; +static bool full_grab; +static bool have_cocoa_ui; -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; @@ -115,6 +114,7 @@ static QemuClipboardInfo *cbinfo; static QemuClipboardPeer cbpeer; static QemuCocoaPasteboardTypeOwner *cbowner; static QemuEvent cbevent; +static QemuThread main_thread; // Utility functions to run specified code block with iothread lock held typedef void (^CodeBlock)(void); @@ -150,39 +150,28 @@ static bool bool_with_iothread_lock(BoolCodeBlock block) /* * 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(): + * need to start a second thread which runs qemu_main_loop(): * * 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 + * qemu_init() * 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. + * fullscreen if needed + * create main loop thread + * call qemu_main_loop() */ -static void *call_qemu_main(void *opaque) +static void *call_qemu_main_loop(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"); + COCOA_DEBUG("Second thread: calling qemu_main_loop()\n"); + qemu_mutex_lock_iothread(); + qemu_main_loop(); + COCOA_DEBUG("Second thread: qemu_main_loop() returned, exiting\n"); + qemu_cleanup(); [cbowner release]; - exit(status); + exit(0); } // Mac to QKeyCode conversion @@ -823,9 +812,7 @@ static CGEventRef handleTapEvent(CGEventTapProxy proxy, CGEventType type, CGEven /* * 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; } @@ -1323,8 +1310,22 @@ static CGEventRef handleTapEvent(CGEventTapProxy proxy, CGEventType type, CGEven { 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(&main_thread, "qemu_main_loop", call_qemu_main_loop, + NULL, QEMU_THREAD_DETACHED); + + if (full_screen) { + [NSApp activateIgnoringOtherApps: YES]; + [self toggleFullScreen: nil]; + } + if (full_grab) { + [self setFullGrab: nil]; + } } - (void)applicationWillTerminate:(NSNotification *)aNotification @@ -1941,22 +1942,18 @@ static void cocoa_clipboard_request(QemuClipboardInfo *info, } } -int main (int argc, char **argv) { - QemuThread thread; - +int main(int argc, char **argv, char **envp) +{ 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, envp); + if (!have_cocoa_ui) { + qemu_main_loop(); + qemu_cleanup(); + return 0; + } NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init]; @@ -1978,6 +1975,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]; @@ -2071,24 +2069,13 @@ 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; - /* 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"); - - QemuCocoaAppController *controller = (QemuCocoaAppController *)[[NSApplication sharedApplication] delegate]; - /* if fullscreen mode is to be used */ if (opts->has_full_screen && opts->full_screen) { - dispatch_async(dispatch_get_main_queue(), ^{ - [NSApp activateIgnoringOtherApps: YES]; - [controller toggleFullScreen: nil]; - }); + full_screen = 1; } if (opts->u.cocoa.has_full_grab && opts->u.cocoa.full_grab) { - dispatch_async(dispatch_get_main_queue(), ^{ - [controller setFullGrab: nil]; - }); + full_grab = 1; } if (opts->has_show_cursor && opts->show_cursor) { @@ -2101,13 +2088,6 @@ static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts) if (opts->u.cocoa.has_left_command_key && !opts->u.cocoa.left_command_key) { left_command_key_enabled = 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 = {