[PATCH-for-7.0,v2] qemu/main-loop: Disable block backend global state assertion on Cocoa

Commit Message

Philippe Mathieu-Daudé March 22, 2022, 7:54 a.m. UTC

From: Philippe Mathieu-Daudé <[e-mail]>

Since commit 0439c5a462 ("block/block-backend.c: assertions for
block-backend") QEMU crashes when using Cocoa on Darwin hosts.

Example on macOS:

  $ qemu-system-i386
  Assertion failed: (qemu_in_main_thread()), function blk_all_next, file block-backend.c, line 552.
  Abort trap: 6

Looking with lldb:

  Assertion failed: (qemu_in_main_thread()), function blk_all_next, file block-backend.c, line 552.
  Process 76914 stopped
  * thread #1, queue = 'com.apple.main-thread', stop reason = hit program assert
     frame #4: 0x000000010057c2d4 qemu-system-i386`blk_all_next.cold.1
  at block-backend.c:552:5 [opt]
      549    */
      550   BlockBackend *blk_all_next(BlockBackend *blk)
      551   {
  --> 552       GLOBAL_STATE_CODE();
      553       return blk ? QTAILQ_NEXT(blk, link)
      554                  : QTAILQ_FIRST(&block_backends);
      555   }
  Target 1: (qemu-system-i386) stopped.

  (lldb) bt
  * thread #1, queue = 'com.apple.main-thread', stop reason = hit program assert
     frame #0: 0x00000001908c99b8 libsystem_kernel.dylib`__pthread_kill + 8
     frame #1: 0x00000001908fceb0 libsystem_pthread.dylib`pthread_kill + 288
     frame #2: 0x000000019083a314 libsystem_c.dylib`abort + 164
     frame #3: 0x000000019083972c libsystem_c.dylib`__assert_rtn + 300
   * frame #4: 0x000000010057c2d4 qemu-system-i386`blk_all_next.cold.1 at block-backend.c:552:5 [opt]
     frame #5: 0x00000001003c00b4 qemu-system-i386`blk_all_next(blk=<unavailable>) at block-backend.c:552:5 [opt]
     frame #6: 0x00000001003d8f04 qemu-system-i386`qmp_query_block(errp=0x0000000000000000) at qapi.c:591:16 [opt]
     frame #7: 0x000000010003ab0c qemu-system-i386`main [inlined] addRemovableDevicesMenuItems at cocoa.m:1756:21 [opt]
     frame #8: 0x000000010003ab04 qemu-system-i386`main(argc=<unavailable>, argv=<unavailable>) at cocoa.m:1980:5 [opt]
     frame #9: 0x00000001012690f4 dyld`start + 520

As we are in passed release 7.0 hard freeze, disable the block
backend assertion which, while being valuable during development,
is not helpful to users. We'll restore this assertion immediately
once 7.0 is released and work on a fix.

Cc: Kevin Wolf <[e-mail]>
Cc: Paolo Bonzini <[e-mail]>
Cc: Peter Maydell <[e-mail]>
Cc: Emanuele Giuseppe Esposito <[e-mail]>
Suggested-by: Akihiko Odaki <[e-mail]>
Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
---
Supersedes: <[e-mail]>
---
 include/qemu/main-loop.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Akihiko Odaki March 22, 2022, 8:23 a.m. UTC | #1

On 2022/03/22 16:54, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé <[e-mail]>
> 
> Since commit 0439c5a462 ("block/block-backend.c: assertions for
> block-backend") QEMU crashes when using Cocoa on Darwin hosts.
> 
> Example on macOS:
> 
>    $ qemu-system-i386
>    Assertion failed: (qemu_in_main_thread()), function blk_all_next, file block-backend.c, line 552.
>    Abort trap: 6
> 
> Looking with lldb:
> 
>    Assertion failed: (qemu_in_main_thread()), function blk_all_next, file block-backend.c, line 552.
>    Process 76914 stopped
>    * thread #1, queue = 'com.apple.main-thread', stop reason = hit program assert
>       frame #4: 0x000000010057c2d4 qemu-system-i386`blk_all_next.cold.1
>    at block-backend.c:552:5 [opt]
>        549    */
>        550   BlockBackend *blk_all_next(BlockBackend *blk)
>        551   {
>    --> 552       GLOBAL_STATE_CODE();
>        553       return blk ? QTAILQ_NEXT(blk, link)
>        554                  : QTAILQ_FIRST(&block_backends);
>        555   }
>    Target 1: (qemu-system-i386) stopped.
> 
>    (lldb) bt
>    * thread #1, queue = 'com.apple.main-thread', stop reason = hit program assert
>       frame #0: 0x00000001908c99b8 libsystem_kernel.dylib`__pthread_kill + 8
>       frame #1: 0x00000001908fceb0 libsystem_pthread.dylib`pthread_kill + 288
>       frame #2: 0x000000019083a314 libsystem_c.dylib`abort + 164
>       frame #3: 0x000000019083972c libsystem_c.dylib`__assert_rtn + 300
>     * frame #4: 0x000000010057c2d4 qemu-system-i386`blk_all_next.cold.1 at block-backend.c:552:5 [opt]
>       frame #5: 0x00000001003c00b4 qemu-system-i386`blk_all_next(blk=<unavailable>) at block-backend.c:552:5 [opt]
>       frame #6: 0x00000001003d8f04 qemu-system-i386`qmp_query_block(errp=0x0000000000000000) at qapi.c:591:16 [opt]
>       frame #7: 0x000000010003ab0c qemu-system-i386`main [inlined] addRemovableDevicesMenuItems at cocoa.m:1756:21 [opt]
>       frame #8: 0x000000010003ab04 qemu-system-i386`main(argc=<unavailable>, argv=<unavailable>) at cocoa.m:1980:5 [opt]
>       frame #9: 0x00000001012690f4 dyld`start + 520
> 
> As we are in passed release 7.0 hard freeze, disable the block
> backend assertion which, while being valuable during development,
> is not helpful to users. We'll restore this assertion immediately
> once 7.0 is released and work on a fix.
> 
> Cc: Kevin Wolf <[e-mail]>
> Cc: Paolo Bonzini <[e-mail]>
> Cc: Peter Maydell <[e-mail]>
> Cc: Emanuele Giuseppe Esposito <[e-mail]>
> Suggested-by: Akihiko Odaki <[e-mail]>
> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
> ---
> Supersedes: <[e-mail]>
> ---
>   include/qemu/main-loop.h | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
> index 7a4d6a0920..48061f736b 100644
> --- a/include/qemu/main-loop.h
> +++ b/include/qemu/main-loop.h
> @@ -270,10 +270,22 @@ bool qemu_mutex_iothread_locked(void);
>   bool qemu_in_main_thread(void);
>   
>   /* Mark and check that the function is part of the global state API. */
> +#ifdef CONFIG_COCOA
> +/*
> + * When using Cocoa ui, addRemovableDevicesMenuItems() calls qmp_query_block()
> + * while expecting the main thread to still hold the BQL, triggering this
> + * assertions in the block layer (commit 0439c5a462). As the Cocoa fix is not
> + * trivial, disable this assertion for the v7.0.0 release when using Cocoa; it
> + * will be restored immediately after the release. This issue is tracked as
> + * https://gitlab.com/qemu-project/qemu/-/issues/926

It is better to state that addRemovableDevicesMenuItems() is called from 
a thread different from the QEMU main thread and the main thread is 
waiting for that.

> + */
> +#define GLOBAL_STATE_CODE()
> +#else
>   #define GLOBAL_STATE_CODE()                                         \
>       do {                                                            \
>           assert(qemu_in_main_thread());                              \
>       } while (0)
> +#endif /* CONFIG_DARWIN */

It still says CONFIG_DARWIN ;)

Regards,
Akihiko Odaki

>   
>   /* Mark and check that the function is part of the I/O API. */
>   #define IO_CODE()                                                   \

Paolo Bonzini March 22, 2022, 8:32 a.m. UTC | #2

On 3/22/22 08:54, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé <[e-mail]>
> 
> Since commit 0439c5a462 ("block/block-backend.c: assertions for
> block-backend") QEMU crashes when using Cocoa on Darwin hosts.
> 
> Example on macOS:
> 
>    $ qemu-system-i386
>    Assertion failed: (qemu_in_main_thread()), function blk_all_next, file block-backend.c, line 552.
>    Abort trap: 6
> 
> Looking with lldb:
> 
>    Assertion failed: (qemu_in_main_thread()), function blk_all_next, file block-backend.c, line 552.
>    Process 76914 stopped
>    * thread #1, queue = 'com.apple.main-thread', stop reason = hit program assert
>       frame #4: 0x000000010057c2d4 qemu-system-i386`blk_all_next.cold.1
>    at block-backend.c:552:5 [opt]
>        549    */
>        550   BlockBackend *blk_all_next(BlockBackend *blk)
>        551   {
>    --> 552       GLOBAL_STATE_CODE();
>        553       return blk ? QTAILQ_NEXT(blk, link)
>        554                  : QTAILQ_FIRST(&block_backends);
>        555   }
>    Target 1: (qemu-system-i386) stopped.
> 
>    (lldb) bt
>    * thread #1, queue = 'com.apple.main-thread', stop reason = hit program assert
>       frame #0: 0x00000001908c99b8 libsystem_kernel.dylib`__pthread_kill + 8
>       frame #1: 0x00000001908fceb0 libsystem_pthread.dylib`pthread_kill + 288
>       frame #2: 0x000000019083a314 libsystem_c.dylib`abort + 164
>       frame #3: 0x000000019083972c libsystem_c.dylib`__assert_rtn + 300
>     * frame #4: 0x000000010057c2d4 qemu-system-i386`blk_all_next.cold.1 at block-backend.c:552:5 [opt]
>       frame #5: 0x00000001003c00b4 qemu-system-i386`blk_all_next(blk=<unavailable>) at block-backend.c:552:5 [opt]
>       frame #6: 0x00000001003d8f04 qemu-system-i386`qmp_query_block(errp=0x0000000000000000) at qapi.c:591:16 [opt]
>       frame #7: 0x000000010003ab0c qemu-system-i386`main [inlined] addRemovableDevicesMenuItems at cocoa.m:1756:21 [opt]
>       frame #8: 0x000000010003ab04 qemu-system-i386`main(argc=<unavailable>, argv=<unavailable>) at cocoa.m:1980:5 [opt]
>       frame #9: 0x00000001012690f4 dyld`start + 520
> 
> As we are in passed release 7.0 hard freeze, disable the block
> backend assertion which, while being valuable during development,
> is not helpful to users. We'll restore this assertion immediately
> once 7.0 is released and work on a fix.
> 
> Cc: Kevin Wolf <[e-mail]>
> Cc: Paolo Bonzini <[e-mail]>
> Cc: Peter Maydell <[e-mail]>
> Cc: Emanuele Giuseppe Esposito <[e-mail]>
> Suggested-by: Akihiko Odaki <[e-mail]>
> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
> ---
> Supersedes: <[e-mail]>
> ---
>   include/qemu/main-loop.h | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
> index 7a4d6a0920..48061f736b 100644
> --- a/include/qemu/main-loop.h
> +++ b/include/qemu/main-loop.h
> @@ -270,10 +270,22 @@ bool qemu_mutex_iothread_locked(void);
>   bool qemu_in_main_thread(void);
>   
>   /* Mark and check that the function is part of the global state API. */
> +#ifdef CONFIG_COCOA
> +/*
> + * When using Cocoa ui, addRemovableDevicesMenuItems() calls qmp_query_block()
> + * while expecting the main thread to still hold the BQL, triggering this
> + * assertions in the block layer (commit 0439c5a462). As the Cocoa fix is not
> + * trivial, disable this assertion for the v7.0.0 release when using Cocoa; it
> + * will be restored immediately after the release. This issue is tracked as
> + * https://gitlab.com/qemu-project/qemu/-/issues/926
> + */
> +#define GLOBAL_STATE_CODE()
> +#else
>   #define GLOBAL_STATE_CODE()                                         \
>       do {                                                            \
>           assert(qemu_in_main_thread());                              \
>       } while (0)
> +#endif /* CONFIG_DARWIN */
>   
>   /* Mark and check that the function is part of the I/O API. */
>   #define IO_CODE()                                                   \

I don't know, it seems to me that the reorganized initialization code 
had only advantages.

For now, it fixes the regression and makes the Cocoa build much more 
similar to the others.  There is an easy way to fix the -runas 
regression, by moving the code up to the call of -sharedApplication in 
cocoa_display_init.

Later, in 7.1, you have time to move more code out of 
-applicationDidFinishLaunching:, remove the bools that are copied out of 
the display options, etc.

Paolo

Philippe Mathieu-Daudé March 22, 2022, 9:20 a.m. UTC | #3

On 22/3/22 09:32, Paolo Bonzini wrote:
> On 3/22/22 08:54, Philippe Mathieu-Daudé wrote:
>> From: Philippe Mathieu-Daudé <[e-mail]>
>>
>> Since commit 0439c5a462 ("block/block-backend.c: assertions for
>> block-backend") QEMU crashes when using Cocoa on Darwin hosts.
>>
>> Example on macOS:
>>
>>    $ qemu-system-i386
>>    Assertion failed: (qemu_in_main_thread()), function blk_all_next, 
>> file block-backend.c, line 552.
>>    Abort trap: 6
>>
>> Looking with lldb:
>>
>>    Assertion failed: (qemu_in_main_thread()), function blk_all_next, 
>> file block-backend.c, line 552.
>>    Process 76914 stopped
>>    * thread #1, queue = 'com.apple.main-thread', stop reason = hit 
>> program assert
>>       frame #4: 0x000000010057c2d4 qemu-system-i386`blk_all_next.cold.1
>>    at block-backend.c:552:5 [opt]
>>        549    */
>>        550   BlockBackend *blk_all_next(BlockBackend *blk)
>>        551   {
>>    --> 552       GLOBAL_STATE_CODE();
>>        553       return blk ? QTAILQ_NEXT(blk, link)
>>        554                  : QTAILQ_FIRST(&block_backends);
>>        555   }
>>    Target 1: (qemu-system-i386) stopped.
>>
>>    (lldb) bt
>>    * thread #1, queue = 'com.apple.main-thread', stop reason = hit 
>> program assert
>>       frame #0: 0x00000001908c99b8 
>> libsystem_kernel.dylib`__pthread_kill + 8
>>       frame #1: 0x00000001908fceb0 
>> libsystem_pthread.dylib`pthread_kill + 288
>>       frame #2: 0x000000019083a314 libsystem_c.dylib`abort + 164
>>       frame #3: 0x000000019083972c libsystem_c.dylib`__assert_rtn + 300
>>     * frame #4: 0x000000010057c2d4 
>> qemu-system-i386`blk_all_next.cold.1 at block-backend.c:552:5 [opt]
>>       frame #5: 0x00000001003c00b4 
>> qemu-system-i386`blk_all_next(blk=<unavailable>) at 
>> block-backend.c:552:5 [opt]
>>       frame #6: 0x00000001003d8f04 
>> qemu-system-i386`qmp_query_block(errp=0x0000000000000000) at 
>> qapi.c:591:16 [opt]
>>       frame #7: 0x000000010003ab0c qemu-system-i386`main [inlined] 
>> addRemovableDevicesMenuItems at cocoa.m:1756:21 [opt]
>>       frame #8: 0x000000010003ab04 
>> qemu-system-i386`main(argc=<unavailable>, argv=<unavailable>) at 
>> cocoa.m:1980:5 [opt]
>>       frame #9: 0x00000001012690f4 dyld`start + 520
>>
>> As we are in passed release 7.0 hard freeze, disable the block
>> backend assertion which, while being valuable during development,
>> is not helpful to users. We'll restore this assertion immediately
>> once 7.0 is released and work on a fix.
>>
>> Cc: Kevin Wolf <[e-mail]>
>> Cc: Paolo Bonzini <[e-mail]>
>> Cc: Peter Maydell <[e-mail]>
>> Cc: Emanuele Giuseppe Esposito <[e-mail]>
>> Suggested-by: Akihiko Odaki <[e-mail]>
>> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
>> ---
>> Supersedes: <[e-mail]>
>> ---
>>   include/qemu/main-loop.h | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
>> index 7a4d6a0920..48061f736b 100644
>> --- a/include/qemu/main-loop.h
>> +++ b/include/qemu/main-loop.h
>> @@ -270,10 +270,22 @@ bool qemu_mutex_iothread_locked(void);
>>   bool qemu_in_main_thread(void);
>>   /* Mark and check that the function is part of the global state API. */
>> +#ifdef CONFIG_COCOA
>> +/*
>> + * When using Cocoa ui, addRemovableDevicesMenuItems() calls 
>> qmp_query_block()
>> + * while expecting the main thread to still hold the BQL, triggering 
>> this
>> + * assertions in the block layer (commit 0439c5a462). As the Cocoa 
>> fix is not
>> + * trivial, disable this assertion for the v7.0.0 release when using 
>> Cocoa; it
>> + * will be restored immediately after the release. This issue is 
>> tracked as
>> + * https://gitlab.com/qemu-project/qemu/-/issues/926
>> + */
>> +#define GLOBAL_STATE_CODE()
>> +#else
>>   #define GLOBAL_STATE_CODE()                                         \
>>       do {                                                            \
>>           assert(qemu_in_main_thread());                              \
>>       } while (0)
>> +#endif /* CONFIG_DARWIN */
>>   /* Mark and check that the function is part of the I/O API. */
>>   #define IO_CODE()                                                   \
> 
> I don't know, it seems to me that the reorganized initialization code 
> had only advantages.

Yeah, I just don't want to break the "no new code after soft-freeze"
rule.

> For now, it fixes the regression and makes the Cocoa build much more 
> similar to the others.  There is an easy way to fix the -runas 
> regression, by moving the code up to the call of -sharedApplication in 
> cocoa_display_init.

That worked for me, I'll respin and wait to see if Akihiko is happy with
the change and Peter won't object to the patchset past hard-freeze.

> Later, in 7.1, you have time to move more code out of 
> -applicationDidFinishLaunching:, remove the bools that are copied out of 
> the display options, etc.
> 
> Paolo

Peter Maydell March 22, 2022, 9:24 a.m. UTC | #4

On Tue, 22 Mar 2022 at 09:20, Philippe Mathieu-Daudé
<[e-mail]> wrote:
>
> On 22/3/22 09:32, Paolo Bonzini wrote:
> > For now, it fixes the regression and makes the Cocoa build much more
> > similar to the others.  There is an easy way to fix the -runas
> > regression, by moving the code up to the call of -sharedApplication in
> > cocoa_display_init.
>
> That worked for me, I'll respin and wait to see if Akihiko is happy with
> the change and Peter won't object to the patchset past hard-freeze.

Cocoa UI is currently completely broken, so we should have some
fix to it. I'm happy to go with whatever you all think is a
reasonable approach.

-- PMM

Philippe Mathieu-Daudé March 22, 2022, 9:35 a.m. UTC | #5

On 22/3/22 09:32, Paolo Bonzini wrote:
> On 3/22/22 08:54, Philippe Mathieu-Daudé wrote:
>> From: Philippe Mathieu-Daudé <[e-mail]>
>>
>> Since commit 0439c5a462 ("block/block-backend.c: assertions for
>> block-backend") QEMU crashes when using Cocoa on Darwin hosts.
>>
>> Example on macOS:
>>
>>    $ qemu-system-i386
>>    Assertion failed: (qemu_in_main_thread()), function blk_all_next, 
>> file block-backend.c, line 552.
>>    Abort trap: 6
>>
>> Looking with lldb:
>>
>>    Assertion failed: (qemu_in_main_thread()), function blk_all_next, 
>> file block-backend.c, line 552.
>>    Process 76914 stopped
>>    * thread #1, queue = 'com.apple.main-thread', stop reason = hit 
>> program assert
>>       frame #4: 0x000000010057c2d4 qemu-system-i386`blk_all_next.cold.1
>>    at block-backend.c:552:5 [opt]
>>        549    */
>>        550   BlockBackend *blk_all_next(BlockBackend *blk)
>>        551   {
>>    --> 552       GLOBAL_STATE_CODE();
>>        553       return blk ? QTAILQ_NEXT(blk, link)
>>        554                  : QTAILQ_FIRST(&block_backends);
>>        555   }
>>    Target 1: (qemu-system-i386) stopped.
>>
>>    (lldb) bt
>>    * thread #1, queue = 'com.apple.main-thread', stop reason = hit 
>> program assert
>>       frame #0: 0x00000001908c99b8 
>> libsystem_kernel.dylib`__pthread_kill + 8
>>       frame #1: 0x00000001908fceb0 
>> libsystem_pthread.dylib`pthread_kill + 288
>>       frame #2: 0x000000019083a314 libsystem_c.dylib`abort + 164
>>       frame #3: 0x000000019083972c libsystem_c.dylib`__assert_rtn + 300
>>     * frame #4: 0x000000010057c2d4 
>> qemu-system-i386`blk_all_next.cold.1 at block-backend.c:552:5 [opt]
>>       frame #5: 0x00000001003c00b4 
>> qemu-system-i386`blk_all_next(blk=<unavailable>) at 
>> block-backend.c:552:5 [opt]
>>       frame #6: 0x00000001003d8f04 
>> qemu-system-i386`qmp_query_block(errp=0x0000000000000000) at 
>> qapi.c:591:16 [opt]
>>       frame #7: 0x000000010003ab0c qemu-system-i386`main [inlined] 
>> addRemovableDevicesMenuItems at cocoa.m:1756:21 [opt]
>>       frame #8: 0x000000010003ab04 
>> qemu-system-i386`main(argc=<unavailable>, argv=<unavailable>) at 
>> cocoa.m:1980:5 [opt]
>>       frame #9: 0x00000001012690f4 dyld`start + 520
>>
>> As we are in passed release 7.0 hard freeze, disable the block
>> backend assertion which, while being valuable during development,
>> is not helpful to users. We'll restore this assertion immediately
>> once 7.0 is released and work on a fix.
>>
>> Cc: Kevin Wolf <[e-mail]>
>> Cc: Paolo Bonzini <[e-mail]>
>> Cc: Peter Maydell <[e-mail]>
>> Cc: Emanuele Giuseppe Esposito <[e-mail]>
>> Suggested-by: Akihiko Odaki <[e-mail]>
>> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
>> ---
>> Supersedes: <[e-mail]>
>> ---
>>   include/qemu/main-loop.h | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
>> index 7a4d6a0920..48061f736b 100644
>> --- a/include/qemu/main-loop.h
>> +++ b/include/qemu/main-loop.h
>> @@ -270,10 +270,22 @@ bool qemu_mutex_iothread_locked(void);
>>   bool qemu_in_main_thread(void);
>>   /* Mark and check that the function is part of the global state API. */
>> +#ifdef CONFIG_COCOA
>> +/*
>> + * When using Cocoa ui, addRemovableDevicesMenuItems() calls 
>> qmp_query_block()
>> + * while expecting the main thread to still hold the BQL, triggering 
>> this
>> + * assertions in the block layer (commit 0439c5a462). As the Cocoa 
>> fix is not
>> + * trivial, disable this assertion for the v7.0.0 release when using 
>> Cocoa; it
>> + * will be restored immediately after the release. This issue is 
>> tracked as
>> + * https://gitlab.com/qemu-project/qemu/-/issues/926
>> + */
>> +#define GLOBAL_STATE_CODE()
>> +#else
>>   #define GLOBAL_STATE_CODE()                                         \
>>       do {                                                            \
>>           assert(qemu_in_main_thread());                              \
>>       } while (0)
>> +#endif /* CONFIG_DARWIN */
>>   /* Mark and check that the function is part of the I/O API. */
>>   #define IO_CODE()                                                   \
> 
> I don't know, it seems to me that the reorganized initialization code 
> had only advantages.
> 
> For now, it fixes the regression and makes the Cocoa build much more 
> similar to the others.  There is an easy way to fix the -runas 
> regression, by moving the code up to the call of -sharedApplication in 
> cocoa_display_init.

So the options are:

#1 disabling the assert for cocoa (this patch)

#2 run qemu_init() in the main thread​ from Paolo [*] with this (?)
    patch on top:

-- >8 --
diff --git a/ui/cocoa.m b/ui/cocoa.m
index e69ce97f44..867c222e18 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -1946,7 +1946,6 @@ static void 
cocoa_clipboard_request(QemuClipboardInfo *info,
  int main(int argc, char **argv, char **envp)
  {
      COCOA_DEBUG("Entered main()\n");
-    qemu_event_init(&cbevent, false);

      /* Takes iothread lock, released in 
applicationDidFinishLaunching:.  */
      qemu_init(argc, argv, envp);
@@ -1958,13 +1957,6 @@ int main(int argc, char **argv, char **envp)

      NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init];

-    // Pull this console process up to being a fully-fledged graphical
-    // app with a menubar and Dock icon
-    ProcessSerialNumber psn = { 0, kCurrentProcess };
-    TransformProcessType(&psn, kProcessTransformToForegroundApplication);
-
-    [QemuApplication sharedApplication];
-
      create_initial_menus();

      /*
@@ -1976,7 +1968,6 @@ int main(int argc, char **argv, char **envp)
       */
      add_console_menu_entries();
      addRemovableDevicesMenuItems();
-    cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init];

      // Create an Application controller
      QemuCocoaAppController *appController = [[QemuCocoaAppController 
alloc] init];
@@ -2089,6 +2080,17 @@ 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;
      }
+
+    qemu_event_init(&cbevent, false);
+
+    cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init];
+
+    // Pull this console process up to being a fully-fledged graphical
+    // app with a menubar and Dock icon
+    ProcessSerialNumber psn = { 0, kCurrentProcess };
+    TransformProcessType(&psn, kProcessTransformToForegroundApplication);
+
+    [QemuApplication sharedApplication];
  }

---

# 3 "Create menus in iothread" approach from Akihiko:
https://lore.kernel.org/qemu-devel/[e-mail]/

Is that correct? (#2 patch and the 3 different options)

What is preferred between #2 and #3? I don't have enough knowledge to
take the decision, which is why I suggested the chicken-hearted "disable
assert" option #1.

Thanks,

Phil.

[*] 
https://lore.kernel.org/qemu-devel/[e-mail]/

Akihiko Odaki March 22, 2022, 10:56 a.m. UTC | #6

On 2022/03/22 18:35, Philippe Mathieu-Daudé wrote:
> On 22/3/22 09:32, Paolo Bonzini wrote:
>> On 3/22/22 08:54, Philippe Mathieu-Daudé wrote:
>>> From: Philippe Mathieu-Daudé <[e-mail]>
>>>
>>> Since commit 0439c5a462 ("block/block-backend.c: assertions for
>>> block-backend") QEMU crashes when using Cocoa on Darwin hosts.
>>>
>>> Example on macOS:
>>>
>>>    $ qemu-system-i386
>>>    Assertion failed: (qemu_in_main_thread()), function blk_all_next, 
>>> file block-backend.c, line 552.
>>>    Abort trap: 6
>>>
>>> Looking with lldb:
>>>
>>>    Assertion failed: (qemu_in_main_thread()), function blk_all_next, 
>>> file block-backend.c, line 552.
>>>    Process 76914 stopped
>>>    * thread #1, queue = 'com.apple.main-thread', stop reason = hit 
>>> program assert
>>>       frame #4: 0x000000010057c2d4 qemu-system-i386`blk_all_next.cold.1
>>>    at block-backend.c:552:5 [opt]
>>>        549    */
>>>        550   BlockBackend *blk_all_next(BlockBackend *blk)
>>>        551   {
>>>    --> 552       GLOBAL_STATE_CODE();
>>>        553       return blk ? QTAILQ_NEXT(blk, link)
>>>        554                  : QTAILQ_FIRST(&block_backends);
>>>        555   }
>>>    Target 1: (qemu-system-i386) stopped.
>>>
>>>    (lldb) bt
>>>    * thread #1, queue = 'com.apple.main-thread', stop reason = hit 
>>> program assert
>>>       frame #0: 0x00000001908c99b8 
>>> libsystem_kernel.dylib`__pthread_kill + 8
>>>       frame #1: 0x00000001908fceb0 
>>> libsystem_pthread.dylib`pthread_kill + 288
>>>       frame #2: 0x000000019083a314 libsystem_c.dylib`abort + 164
>>>       frame #3: 0x000000019083972c libsystem_c.dylib`__assert_rtn + 300
>>>     * frame #4: 0x000000010057c2d4 
>>> qemu-system-i386`blk_all_next.cold.1 at block-backend.c:552:5 [opt]
>>>       frame #5: 0x00000001003c00b4 
>>> qemu-system-i386`blk_all_next(blk=<unavailable>) at 
>>> block-backend.c:552:5 [opt]
>>>       frame #6: 0x00000001003d8f04 
>>> qemu-system-i386`qmp_query_block(errp=0x0000000000000000) at 
>>> qapi.c:591:16 [opt]
>>>       frame #7: 0x000000010003ab0c qemu-system-i386`main [inlined] 
>>> addRemovableDevicesMenuItems at cocoa.m:1756:21 [opt]
>>>       frame #8: 0x000000010003ab04 
>>> qemu-system-i386`main(argc=<unavailable>, argv=<unavailable>) at 
>>> cocoa.m:1980:5 [opt]
>>>       frame #9: 0x00000001012690f4 dyld`start + 520
>>>
>>> As we are in passed release 7.0 hard freeze, disable the block
>>> backend assertion which, while being valuable during development,
>>> is not helpful to users. We'll restore this assertion immediately
>>> once 7.0 is released and work on a fix.
>>>
>>> Cc: Kevin Wolf <[e-mail]>
>>> Cc: Paolo Bonzini <[e-mail]>
>>> Cc: Peter Maydell <[e-mail]>
>>> Cc: Emanuele Giuseppe Esposito <[e-mail]>
>>> Suggested-by: Akihiko Odaki <[e-mail]>
>>> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
>>> ---
>>> Supersedes: <[e-mail]>
>>> ---
>>>   include/qemu/main-loop.h | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
>>> index 7a4d6a0920..48061f736b 100644
>>> --- a/include/qemu/main-loop.h
>>> +++ b/include/qemu/main-loop.h
>>> @@ -270,10 +270,22 @@ bool qemu_mutex_iothread_locked(void);
>>>   bool qemu_in_main_thread(void);
>>>   /* Mark and check that the function is part of the global state 
>>> API. */
>>> +#ifdef CONFIG_COCOA
>>> +/*
>>> + * When using Cocoa ui, addRemovableDevicesMenuItems() calls 
>>> qmp_query_block()
>>> + * while expecting the main thread to still hold the BQL, triggering 
>>> this
>>> + * assertions in the block layer (commit 0439c5a462). As the Cocoa 
>>> fix is not
>>> + * trivial, disable this assertion for the v7.0.0 release when using 
>>> Cocoa; it
>>> + * will be restored immediately after the release. This issue is 
>>> tracked as
>>> + * https://gitlab.com/qemu-project/qemu/-/issues/926
>>> + */
>>> +#define GLOBAL_STATE_CODE()
>>> +#else
>>>   #define GLOBAL_STATE_CODE()                                         \
>>>       do {                                                            \
>>>           assert(qemu_in_main_thread());                              \
>>>       } while (0)
>>> +#endif /* CONFIG_DARWIN */
>>>   /* Mark and check that the function is part of the I/O API. */
>>>   #define IO_CODE()                                                   \
>>
>> I don't know, it seems to me that the reorganized initialization code 
>> had only advantages.
>>
>> For now, it fixes the regression and makes the Cocoa build much more 
>> similar to the others.  There is an easy way to fix the -runas 
>> regression, by moving the code up to the call of -sharedApplication in 
>> cocoa_display_init.
> 
> So the options are:
> 
> #1 disabling the assert for cocoa (this patch)
> 
> #2 run qemu_init() in the main thread​ from Paolo [*] with this (?)
>     patch on top:
> 
> -- >8 --
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index e69ce97f44..867c222e18 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -1946,7 +1946,6 @@ static void 
> cocoa_clipboard_request(QemuClipboardInfo *info,
>   int main(int argc, char **argv, char **envp)
>   {
>       COCOA_DEBUG("Entered main()\n");
> -    qemu_event_init(&cbevent, false);
> 
>       /* Takes iothread lock, released in 
> applicationDidFinishLaunching:.  */
>       qemu_init(argc, argv, envp);
> @@ -1958,13 +1957,6 @@ int main(int argc, char **argv, char **envp)
> 
>       NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init];
> 
> -    // Pull this console process up to being a fully-fledged graphical
> -    // app with a menubar and Dock icon
> -    ProcessSerialNumber psn = { 0, kCurrentProcess };
> -    TransformProcessType(&psn, kProcessTransformToForegroundApplication);
> -
> -    [QemuApplication sharedApplication];
> -
>       create_initial_menus();
> 
>       /*
> @@ -1976,7 +1968,6 @@ int main(int argc, char **argv, char **envp)
>        */
>       add_console_menu_entries();
>       addRemovableDevicesMenuItems();
> -    cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init];
> 
>       // Create an Application controller
>       QemuCocoaAppController *appController = [[QemuCocoaAppController 
> alloc] init];
> @@ -2089,6 +2080,17 @@ 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;
>       }
> +
> +    qemu_event_init(&cbevent, false);
> +
> +    cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init];
> +
> +    // Pull this console process up to being a fully-fledged graphical
> +    // app with a menubar and Dock icon
> +    ProcessSerialNumber psn = { 0, kCurrentProcess };
> +    TransformProcessType(&psn, kProcessTransformToForegroundApplication);
> +
> +    [QemuApplication sharedApplication];

I think the changes for cbevent and cbowner are irrelevant, but 
basically this should do.

>   }
> 
> ---
> 
> # 3 "Create menus in iothread" approach from Akihiko:
> https://lore.kernel.org/qemu-devel/[e-mail]/ 
> 
> 
> Is that correct? (#2 patch and the 3 different options)
> 
> What is preferred between #2 and #3? I don't have enough knowledge to
> take the decision, which is why I suggested the chicken-hearted "disable
> assert" option #1.

#2 can help allowing ui/cocoa to live in the same binary with ui/gtk and 
ui/sdl2 in the future and I believe it should be eventually (possible 
after 7.0) done, but requires relatively large code change.

#3 aims to solve only the problem with iothread dependency when creating 
menus and its scope of modification is minimal. It is concerned that #3 
may confuse developers since it touches Cocoa interfaces from iothread 
although it is still according to its API convention.

I'm pretty sure both of #2 and #3 work, but I would like to rather 
insist #1 as the safest. As a developer, I don't want see such an ugly 
workaround like #1 of course, but such feeling does not matter much in 
practice. On the other hand, having the entire next release cycle for 
testing #2 is a real advantage even if the possibility of discovering a 
new problem is very tiny.

Regards,
Akihiko Odaki

> 
> Thanks,
> 
> Phil.
> 
> [*] 
> https://lore.kernel.org/qemu-devel/[e-mail]/ 
>

Peter Maydell March 25, 2022, 10:03 a.m. UTC | #7

On Tue, 22 Mar 2022 at 09:35, Philippe Mathieu-Daudé
<[e-mail]> wrote:
>
> What is preferred between #2 and #3? I don't have enough knowledge to
> take the decision, which is why I suggested the chicken-hearted "disable
> assert" option #1.

I don't have a strong opinion, but I do strongly think we need
to get a fix of some kind into the tree before rc2, and we
are rapidly running out of time for that. Pick something,
and send patches/pullreq as appropriate, please.

thanks
-- PMM

Patch

diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index 7a4d6a0920..48061f736b 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -270,10 +270,22 @@  bool qemu_mutex_iothread_locked(void);
 bool qemu_in_main_thread(void);
 
 /* Mark and check that the function is part of the global state API. */
+#ifdef CONFIG_COCOA
+/*
+ * When using Cocoa ui, addRemovableDevicesMenuItems() calls qmp_query_block()
+ * while expecting the main thread to still hold the BQL, triggering this
+ * assertions in the block layer (commit 0439c5a462). As the Cocoa fix is not
+ * trivial, disable this assertion for the v7.0.0 release when using Cocoa; it
+ * will be restored immediately after the release. This issue is tracked as
+ * https://gitlab.com/qemu-project/qemu/-/issues/926
+ */
+#define GLOBAL_STATE_CODE()
+#else
 #define GLOBAL_STATE_CODE()                                         \
     do {                                                            \
         assert(qemu_in_main_thread());                              \
     } while (0)
+#endif /* CONFIG_DARWIN */
 
 /* Mark and check that the function is part of the I/O API. */
 #define IO_CODE()                                                   \