Re: [RFC PATCH 02/19] system/cpus: Only kick running vCPUs

["Alex Bennée" <alex.bennee@linaro.org>]

Philippe Mathieu-Daudé <philmd@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é <philmd@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-philmd@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