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

["Nicholas Piggin" <npiggin@gmail.com>]

On Tue Apr 15, 2025 at 1:24 AM AEST, Pierrick Bouvier wrote:
> On 4/14/25 03:25, Philippe Mathieu-Daudé wrote:
>> 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.
>> 
>
> In case someone is curious about it, changing default state of lock can 
> answer why it's needed, as it crashes immediately on an assert.

That all sounds reasonable enough and good info. I'm not suggesting to
remove the lock from qemu_init() by assuming we are in init and init is
single threaded (I agree it's good practice to keep locking consistent).

My question was more that we should move the locks tighter around
the operations that require them. Move the unlock into qemu_init().

Commit f5ab12caba4f1 didn't introduce this problem, cocoa_main()
already immediatey called bql_unlock() so effectively the issue is
still there. The original design before cocoa I guess was that qemu_init
would init things under the same critical section as qemu_main_loop() is
then called, which is reasonable and conservative. It would have been
good to see this bql split get a specific patch to epxlain why it's not
needed across qemu_init and qemu_main_loop, but no big deal now.

The patch is fine for a fix, could I suggest another patch that
moves the lock narrower and perhaps adds a few words of comment?

Thanks,
Nick