Re: [PATCH 1/2] system/main: transfer replay mutex ownership from main thread to main loop thread

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 12/4/25 19:24, Pierrick Bouvier wrote:
> On 4/11/25 22:30, Nicholas Piggin wrote:
>> On Fri Apr 11, 2025 at 8:55 AM AEST, Pierrick Bouvier wrote:
>>> On MacOS, UI event loop has to be ran in the main thread of a process.
>>> Because of that restriction, on this platform, qemu main event loop is
>>> ran on another thread [1].
>>>
>>> This breaks record/replay feature, which expects thread running 
>>> qemu_init
>>> to initialize hold this lock, breaking associated functional tests on
>>> MacOS.
>>>
>>> Thus, as a generalization, and similar to how BQL is handled, we release
>>> it after init, and reacquire the lock before entering main event loop,
>>> avoiding a special case if a separate thread is used.
>>>
>>> Tested on MacOS with:
>>> $ meson test -C build --setup thorough --print-errorlogs \
>>> func-x86_64-x86_64_replay func-arm-arm_replay func-aarch64- 
>>> aarch64_replay
>>> $ ./build/qemu-system-x86_64 -nographic -icount 
>>> shift=auto,rr=record,rrfile=replay.log
>>> $ ./build/qemu-system-x86_64 -nographic -icount 
>>> shift=auto,rr=replay,rrfile=replay.log
>>>
>>> [1] https://gitlab.com/qemu-project/qemu/-/commit/ 
>>> f5ab12caba4f1656479c1feb5248beac1c833243
>>>
>>> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/2907
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> ---
>>>   system/main.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/system/main.c b/system/main.c
>>> index ecb12fd397c..1c022067349 100644
>>> --- a/system/main.c
>>> +++ b/system/main.c
>>> @@ -25,6 +25,7 @@
>>>   #include "qemu/osdep.h"
>>>   #include "qemu-main.h"
>>>   #include "qemu/main-loop.h"
>>> +#include "system/replay.h"
>>>   #include "system/system.h"
>>>   #ifdef CONFIG_SDL
>>> @@ -44,10 +45,12 @@ static void *qemu_default_main(void *opaque)
>>>   {
>>>       int status;
>>> +    replay_mutex_lock();
>>>       bql_lock();
>>>       status = qemu_main_loop();
>>>       qemu_cleanup(status);
>>>       bql_unlock();
>>> +    replay_mutex_unlock();
>>>       exit(status);
>>>   }
>>> @@ -67,6 +70,7 @@ int main(int argc, char **argv)
>>>   {
>>>       qemu_init(argc, argv);
>>>       bql_unlock();
>>> +    replay_mutex_unlock();
>>>       if (qemu_main) {
>>>           QemuThread main_loop_thread;
>>>           qemu_thread_create(&main_loop_thread, "qemu_main",
>>
>> Do we actually need to hold replay mutex (or even bql) over qemu_init()?
>> Both should get dropped before we return here. But as a simple fix, I
>> guess this is okay.
>>
> 
> For the bql, I don't know the exact reason.
> For replay lock, we need to hold it as clock gets saved as soon as the 
> devices are initialized, which happens before end of qemu_init.

Could be worth adding a comment with that information.

> 
>> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>