Philippe Mathieu-Daudé <phi...@linaro.org> writes:
> On 7/6/25 15:23, Richard Henderson wrote:
>> On 6/6/25 17:44, Philippe Mathieu-Daudé wrote:
>>> As an optimization, avoid kicking stopped vCPUs.
This also breaks gdbstub:
pause_all_vcpus() -> cpu_pause(sets cpu->stop) -> qemu_cpu_kick(skips kicking)
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <phi...@linaro.org>
>>> ---
>>> system/cpus.c | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/system/cpus.c b/system/cpus.c
>>> index d16b0dff989..4835e5ced48 100644
>>> --- a/system/cpus.c
>>> +++ b/system/cpus.c
>>> @@ -494,6 +494,11 @@ void cpus_kick_thread(CPUState *cpu)
>>> void qemu_cpu_kick(CPUState *cpu)
>>> {
>>> qemu_cond_broadcast(cpu->halt_cond);
>>> +
>>> + if (!cpu_can_run(cpu)) {
>>> + return;
>>> + }
>>> +
>> This would appear to be a race condition. The evaluation of
>> cpu_can_run should be done within the context of 'cpu', not here,
>> and not *after* we've already woken 'cpu' via the broadcast.
>
> OK.
>
> Still I don't understand something, when putting this assertion:
>
> -- >8 --
> diff --git a/system/cpus.c b/system/cpus.c
> index d16b0dff989..0631015f754 100644
> --- a/system/cpus.c
> +++ b/system/cpus.c
> @@ -493,7 +493,10 @@ void cpus_kick_thread(CPUState *cpu)
>
> void qemu_cpu_kick(CPUState *cpu)
> {
> + assert(cpu_can_run(cpu));
> +
> qemu_cond_broadcast(cpu->halt_cond);
> if (cpus_accel->kick_vcpu_thread) {
> cpus_accel->kick_vcpu_thread(cpu);
> } else { /* default */
> ---
>
> I get:
>
> (lldb) bt
> * thread #1, queue = 'com.apple.main-thread', stop reason = hit
> program assert
> frame #0: 0x000000018a669388 libsystem_kernel.dylib`__pthread_kill + 8
> frame #1: 0x000000018a6a288c libsystem_pthread.dylib`pthread_kill + 296
> frame #2: 0x000000018a5abc60 libsystem_c.dylib`abort + 124
> frame #3: 0x000000018a5aaeec libsystem_c.dylib`__assert_rtn + 284
> * frame #4: 0x000000010057ddc4 qemu_cpu_kick(cpu=0x0000000130218000)
> at cpus.c:496:5
> frame #5: 0x00000001000106ec
> queue_work_on_cpu(cpu=0x0000000130218000, wi=0x000060000038c000)
> at cpu-common.c:140:5
> frame #6: 0x0000000100010780
> async_run_on_cpu(cpu=0x0000000130218000, func=(tcg_commit_cpu at
> physmem.c:2758), data=(host_int = 60885632, host_ulong =
> 105553177152128, host_ptr = 0x0000600003a10a80, target_ptr =
> 105553177152128)) at cpu-common.c:177:5
> frame #7: 0x000000010059ad34
> tcg_commit(listener=0x0000600003a10a98) at physmem.c:2789:9
> frame #8: 0x0000000100591240
> listener_add_address_space(listener=0x0000600003a10a98,
> as=0x0000600003611980) at memory.c:3082:9
> frame #9: 0x0000000100590f48
> memory_listener_register(listener=0x0000600003a10a98,
> as=0x0000600003611980) at memory.c:3170:5
> frame #10: 0x000000010059abe4
> cpu_address_space_init(cpu=0x0000000130218000, asidx=0,
> prefix="cpu-memory", mr=0x000000012b1faba0) at physmem.c:813:9
> frame #11: 0x0000000100750c40
> arm_cpu_realizefn(dev=0x0000000130218000, errp=0x000000016fdfe2c0)
> at cpu.c:2572:5
> frame #12: 0x0000000100b7ed9c
> device_set_realized(obj=0x0000000130218000, value=true,
> errp=0x000000016fdfe388) at qdev.c:494:13
> frame #13: 0x0000000100b8a880
> property_set_bool(obj=0x0000000130218000, v=0x0000600003f12d00,
> name="realized", opaque=0x000060000010c1d0,
> errp=0x000000016fdfe388) at object.c:2375:5
> frame #14: 0x0000000100b87acc
> object_property_set(obj=0x0000000130218000, name="realized",
> v=0x0000600003f12d00, errp=0x000000016fdfe388) at object.c:1450:5
> frame #15: 0x0000000100b8f14c
> object_property_set_qobject(obj=0x0000000130218000,
> name="realized", value=0x0000600000386920,
> errp=0x0000000101e39e28) at qom-qobject.c:28:10
> frame #16: 0x0000000100b882f8
> object_property_set_bool(obj=0x0000000130218000, name="realized",
> value=true, errp=0x0000000101e39e28) at object.c:1520:15
> frame #17: 0x0000000100b7d240 qdev_realize(dev=0x0000000130218000,
> bus=0x0000000000000000, errp=0x0000000101e39e28) at qdev.c:276:12
> frame #18: 0x000000010083a81c
> machvirt_init(machine=0x000000012b1fa710) at virt.c:2329:9
> frame #19: 0x0000000100136a40
> machine_run_board_init(machine=0x000000012b1fa710,
> mem_path=0x0000000000000000, errp=0x000000016fdfe6a8) at
> machine.c:1669:5
> frame #20: 0x0000000100571384 qemu_init_board at vl.c:2714:5
> frame #21: 0x0000000100571154
> qmp_x_exit_preconfig(errp=0x0000000101e39e28) at vl.c:2808:5
> frame #22: 0x0000000100573a14 qemu_init(argc=17,
> argv=0x000000016fdff138) at vl.c:3844:9
> frame #23: 0x0000000100d036e0 main(argc=17,
> argv=0x000000016fdff138) at main.c:71:5
> frame #24: 0x000000018a302b98 dyld`start + 6076
> (lldb)
>
> I expect a vCPU to be in a "stable" state and usable *after* it is
> realized, as we are calling various hooks in many places. Here we are
> processing the pending work queue while the vCPU isn't fully realized,
> so some hooks might not have been called yet...
>
> Git history of tcg_commit() points to commit 0d58c660689 ("softmmu: Use
> async_run_on_cpu in tcg_commit").
> This isn't the first time I ends there, see also:
> https://lore.kernel.org/qemu-devel/20230907161415.6102-1-phi...@linaro.org/.
> Using the same reasoning of this patch, adding:
>
> -- >8 --
> diff --git a/system/physmem.c b/system/physmem.c
> index a8a9ca309ea..479a7a88037 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -2773,6 +2774,14 @@ static void tcg_commit(MemoryListener *listener)
> cpuas = container_of(listener, CPUAddressSpace, tcg_as_listener);
> cpu = cpuas->cpu;
>
> + if (!qdev_is_realized(DEVICE(cpu))) {
> + /*
> + * The listener is also called during realize, before
> + * all of the tcg machinery for run-on is initialized.
> + */
> + return;
> + }
> +
> /*
> * Defer changes to as->memory_dispatch until the cpu is quiescent.
> * Otherwise we race between (1) other cpu threads and (2) ongoing
> ---
>
> makes my issues disappear; tcg_commit_cpu() calls are run on realized
> vCPUs, and the order of pre-realize vcpu hooks doesn't alter anything.
>
> I don't remember why I wrote this "The listener is also called during
> realize, before all of the tcg machinery for run-on is initialized"
> comment, it could be better to call memory_region_transaction_commit()
> after CpuRealize, maybe in CpuReset.
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
