ui/cocoa: Create menus in iothread

Commit 0439c5a4623d674efa0c72abd62ca6e98bb7cf87 introduced an
assertion that blk_all_next is called in the main thread. The function
is called in the following chain:
- blk_all_next
- qmp_query_block
- addRemovableDevicesMenuItems
- main

This change moves the menu creation to the iothread. This also changes
the menu creation procedure to construct the entire menu tree before
setting to NSApp, which is necessary because a menu set once cannot be
modified if NSApp is already running.

v2: Separate a change moving create_initial_menus (Peter Maydell)

Akihiko Odaki (2):
  ui/cocoa: Move create_initial_menus
  ui/cocoa: Create menus in iothread

 ui/cocoa.m | 209 +++++++++++++++++++++++++----------------------------
 1 file changed, 98 insertions(+), 111 deletions(-)

-- 
2.32.0 (Apple Git-132)

On 3/7/22 14:49, Akihiko Odaki wrote:
> ui/cocoa: Create menus in iothread
> 
> Commit 0439c5a4623d674efa0c72abd62ca6e98bb7cf87 introduced an
> assertion that blk_all_next is called in the main thread. The function
> is called in the following chain:
> - blk_all_next
> - qmp_query_block
> - addRemovableDevicesMenuItems
> - main
> 
> This change moves the menu creation to the iothread. This also changes
> the menu creation procedure to construct the entire menu tree before
> setting to NSApp, which is necessary because a menu set once cannot be
> modified if NSApp is already running.

I wonder if you actually need the iothread/secondary thread separation 
during initialization.  It's needed to run the "secondary" main loop, 
but until that point nobody should care what thread things run on. 
cocoa_display_init() is close enough to the end of qemu_init() that I 
think you can just do:

   main()
     qemu_init()
       /* takes iothread lock */
       cocoa_display_init()
         /* just save a few values from opts */
     ... create menus ...
     [NSApp run]
       applicationDidFinishLaunching:
         /* do what was in cocoa_display_init() */
         qemu_unlock_mutex_iothread();
         qemu_thread_create(call_qemu_main_loop)
                                                    call_qemu_main_loop()
                                                      qemu_main_loop()

This might even obsolete the allow_events hack, because now the main 
thread has the iothread lock until applicationDidFinishLaunching:.

Paolo

> v2: Separate a change moving create_initial_menus (Peter Maydell)
> 
> Akihiko Odaki (2):
>    ui/cocoa: Move create_initial_menus
>    ui/cocoa: Create menus in iothread
> 
>   ui/cocoa.m | 209 +++++++++++++++++++++++++----------------------------
>   1 file changed, 98 insertions(+), 111 deletions(-)
>

On Tue, Mar 8, 2022 at 12:32 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 3/7/22 14:49, Akihiko Odaki wrote:
> > ui/cocoa: Create menus in iothread
> >
> > Commit 0439c5a4623d674efa0c72abd62ca6e98bb7cf87 introduced an
> > assertion that blk_all_next is called in the main thread. The function
> > is called in the following chain:
> > - blk_all_next
> > - qmp_query_block
> > - addRemovableDevicesMenuItems
> > - main
> >
> > This change moves the menu creation to the iothread. This also changes
> > the menu creation procedure to construct the entire menu tree before
> > setting to NSApp, which is necessary because a menu set once cannot be
> > modified if NSApp is already running.
>
> I wonder if you actually need the iothread/secondary thread separation
> during initialization.  It's needed to run the "secondary" main loop,
> but until that point nobody should care what thread things run on.
> cocoa_display_init() is close enough to the end of qemu_init() that I
> think you can just do:
>
>    main()
>      qemu_init()
>        /* takes iothread lock */
>        cocoa_display_init()
>          /* just save a few values from opts */
>      ... create menus ...
>      [NSApp run]
>        applicationDidFinishLaunching:
>          /* do what was in cocoa_display_init() */
>          qemu_unlock_mutex_iothread();
>          qemu_thread_create(call_qemu_main_loop)
>                                                     call_qemu_main_loop()
>                                                       qemu_main_loop()
>
> This might even obsolete the allow_events hack, because now the main
> thread has the iothread lock until applicationDidFinishLaunching:.

allow_events was necessary not because of the separation of the
thread, but because cocoa_display_init waits the main thread for
finishing the initialization. It can be simply removed if it doesn't
wait for app_started_sem.

Regards,
Akihiko Odaki

>
> Paolo
>
> > v2: Separate a change moving create_initial_menus (Peter Maydell)
> >
> > Akihiko Odaki (2):
> >    ui/cocoa: Move create_initial_menus
> >    ui/cocoa: Create menus in iothread
> >
> >   ui/cocoa.m | 209 +++++++++++++++++++++++++----------------------------
> >   1 file changed, 98 insertions(+), 111 deletions(-)
> >
>

On 7/3/22 14:49, Akihiko Odaki wrote:
> ui/cocoa: Create menus in iothread
> 
> Commit 0439c5a4623d674efa0c72abd62ca6e98bb7cf87 introduced an
> assertion that blk_all_next is called in the main thread. The function
> is called in the following chain:
> - blk_all_next
> - qmp_query_block
> - addRemovableDevicesMenuItems
> - main
> 
> This change moves the menu creation to the iothread. This also changes
> the menu creation procedure to construct the entire menu tree before
> setting to NSApp, which is necessary because a menu set once cannot be
> modified if NSApp is already running.
> 
> v2: Separate a change moving create_initial_menus (Peter Maydell)
> 
> Akihiko Odaki (2):
>    ui/cocoa: Move create_initial_menus
>    ui/cocoa: Create menus in iothread

Patch #2 doesn't apply anymore, can you respin?