| Message ID | 20250410225550.46807-2-pierrick.bouvier@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | fix record/replay on MacOS | expand |
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. Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
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. > Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
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> >
On 4/12/25 19:24, Pierrick Bouvier wrote: > On 4/11/25 22:30, Nicholas Piggin wrote: >> 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. In general it's better to assume that there can be other threads (and therefore you need BQL). Also, Rust code panics if you access a BqlCell or BqlRefCell without holding the lock. Paolo
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. >> >>> Reviewed-by: 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
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",
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(+)