CPUState::halt_cond is an accelerator specific pointer, used in particular by TCG (which tcg_commit() is about). The pointer is set by the AccelOpsClass::create_vcpu_thread() handler. AccelOpsClass::create_vcpu_thread() is called by the generic qemu_init_vcpu(), which expect the accelerator handler to eventually call cpu_thread_signal_created() which is protected with a QemuCond. It is safer to check the vCPU is created with this field rather than the 'halt_cond' pointer set in create_vcpu_thread() before the vCPU thread is initialized.
This avoids calling tcg_commit() until all CPUs are realized. Here we can see for a machine with N CPUs, tcg_commit() is called N times before the 'machine_creation_done' event: (lldb) settings set -- target.run-args "-M" "virt" "-smp" "512" "-display" "none" (lldb) breakpoint set --name qemu_machine_creation_done --one-shot true (lldb) breakpoint set --name tcg_commit_cpu --auto-continue true (lldb) run Process 84089 launched: 'qemu-system-aarch64' (arm64) Process 84089 stopped * thread #1, queue = 'com.apple.main-thread', stop reason = one-shot breakpoint 2 (lldb) breakpoint list --brief Current breakpoints: 2: name = 'tcg_commit_cpu', locations = 2, resolved = 2, hit count = 512 Options: enabled auto-continue ^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^ Having the following backtrace: (lldb) bt * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 2.1 * frame #0: 0x1005d0fe0 qemu-system-aarch64`tcg_commit [inlined] tcg_commit_cpu(cpu=0x108460000, data=(host_ptr = 0x600003b05c00, target_ptr = 105553178156032)) at physmem.c:2493:63 frame #1: 0x1005d0fe0 qemu-system-aarch64`tcg_commit(listener=<unavailable>) at physmem.c:2527:9 frame #2: 0x1005cd220 qemu-system-aarch64`memory_listener_register [inlined] listener_add_address_space(listener=0x600003b05c18, as=<unavailable>) at memory.c:3014:9 frame #3: 0x1005cd148 qemu-system-aarch64`memory_listener_register(listener=0x600003b05c18, as=0x16fdfe720) at memory.c:3077:5 frame #4: 0x1005d0f40 qemu-system-aarch64`cpu_address_space_init(cpu=<unavailable>, asidx=<unavailable>, prefix=<unavailable>, mr=<unavailable>) at physmem.c:773:9 [artificial] frame #5: 0x100389a64 qemu-system-aarch64`arm_cpu_realizefn(dev=0x108460000, errp=0x16fdfe720) at cpu.c:2244:5 frame #6: 0x10062af28 qemu-system-aarch64`device_set_realized(obj=<unavailable>, value=<unavailable>, errp=0x16fdfe7d8) at qdev.c:510:13 frame #7: 0x100632518 qemu-system-aarch64`property_set_bool(obj=0x108460000, v=<unavailable>, name=<unavailable>, opaque=0x600000013e50, errp=0x16fdfe7d8) at object.c:2285:5 frame #8: 0x100630808 qemu-system-aarch64`object_property_set(obj=0x108460000, name="realized", v=0x600003e02100, errp=0x16fdfe7d8) at object.c:1420:5 frame #9: 0x1006345ac qemu-system-aarch64`object_property_set_qobject(obj=<unavailable>, name=<unavailable>, value=<unavailable>, errp=<unavailable>) at qom-qobject.c:28:10 frame #10: 0x100630c80 qemu-system-aarch64`object_property_set_bool(obj=<unavailable>, name=<unavailable>, value=<unavailable>, errp=<unavailable>) at object.c:1489:15 frame #11: 0x10062a188 qemu-system-aarch64`qdev_realize(dev=<unavailable>, bus=<unavailable>, errp=<unavailable>) at qdev.c:292:12 [artificial] frame #12: 0x100319c30 qemu-system-aarch64`machvirt_init(machine=0x103562480) at virt.c:2248:9 frame #13: 0x100090edc qemu-system-aarch64`machine_run_board_init(machine=0x103562480, mem_path=<unavailable>, errp=<unavailable>) at machine.c:1469:5 frame #14: 0x1002a2684 qemu-system-aarch64`qmp_x_exit_preconfig [inlined] qemu_init_board at vl.c:2543:5 frame #15: 0x1002a2650 qemu-system-aarch64`qmp_x_exit_preconfig(errp=<unavailable>) at vl.c:2634:5 frame #16: 0x1002a5dd4 qemu-system-aarch64`qemu_init(argc=<unavailable>, argv=<unavailable>) at vl.c:3642:9 frame #17: 0x100627d64 qemu-system-aarch64`main(argc=<unavailable>, argv=<unavailable>) at main.c:47:5 When can then invert the if ladders for clarity: in the unlikely case of the caller being executed on the vCPU thread, directly dispatch, otherwise defer until quiescence. Cc: qemu-sta...@nongnu.org Fixes: 0d58c66068 ("softmmu: Use async_run_on_cpu in tcg_commit") Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1866 Signed-off-by: Philippe Mathieu-Daudé <phi...@linaro.org> --- RFC: I tried my best to understand and explain, but this is still black magic to me... --- softmmu/physmem.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/softmmu/physmem.c b/softmmu/physmem.c index 18277ddd67..12ef9d7d27 100644 --- a/softmmu/physmem.c +++ b/softmmu/physmem.c @@ -2505,22 +2505,27 @@ static void tcg_commit(MemoryListener *listener) cpuas = container_of(listener, CPUAddressSpace, tcg_as_listener); cpu = cpuas->cpu; - /* - * Defer changes to as->memory_dispatch until the cpu is quiescent. - * Otherwise we race between (1) other cpu threads and (2) ongoing - * i/o for the current cpu thread, with data cached by mmu_lookup(). - * - * In addition, queueing the work function will kick the cpu back to - * the main loop, which will end the RCU critical section and reclaim - * the memory data structures. - * - * That said, the listener is also called during realize, before - * all of the tcg machinery for run-on is initialized: thus halt_cond. - */ - if (cpu->halt_cond) { - async_run_on_cpu(cpu, tcg_commit_cpu, RUN_ON_CPU_HOST_PTR(cpuas)); - } else { + if (!cpu->created) { + /* + * The listener is also called during realize, before + * all of the tcg machinery for run-on is initialized. + */ + return; + } + + if (unlikely(qemu_cpu_is_self(cpu))) { tcg_commit_cpu(cpu, RUN_ON_CPU_HOST_PTR(cpuas)); + } else { + /* + * Defer changes to as->memory_dispatch until the cpu is quiescent. + * Otherwise we race between (1) other cpu threads and (2) ongoing + * i/o for the current cpu thread, with data cached by mmu_lookup(). + * + * In addition, queueing the work function will kick the cpu back to + * the main loop, which will end the RCU critical section and reclaim + * the memory data structures. + */ + async_run_on_cpu(cpu, tcg_commit_cpu, RUN_ON_CPU_HOST_PTR(cpuas)); } } -- 2.41.0