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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.
>>
>> 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.