On 4/14/25 19:41, Nicholas Piggin wrote:
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.bouv...@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.
Looking more closely, bql_lock ensure vcpus don't start executing
anything before init is completed. So we really want to hold the lock
through all qemu_init().
Concerning replay_lock, during init, icount_configure calls
qemu_clock_get_ns, that calls replay_save_clock, which expects to have
the lock. Thus, we should hold the lock, at least during icount
configuration.
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?
We would still need to acquire locks in qemu_default_main() anyway.
For bql, we definitely want to hold it anytime through init, so the
scope is end of init.
For replay_lock, it could be moved around parts that expect it during
initialization, but what would be the benefit, considering only one
thread is running during init?
Moving locks narrower is usually made to allow more concurrency, at the
price of increased complexity. In init phase, only one thread runs
anyway, so there is no benefit to do anything around here.
What we could eventually do is move those unlock at the end of
qemu_init, but IMHO, it's more readable to see the lock/unlock scheme in
a single place, in system/main.c.
As well, I think it's better to have a single code path for lock/unlock,
whether we use a background thread or not (vs adding a bool parameter to
qemu_default_main() saying if we are in the same thread, or a different
one).
Thanks,
Nick
