Re: [RFC PATCH] softmmu: Fix async_run_on_cpu() use in tcg_commit_cpu()

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

On 7/9/23 18:28, Richard Henderson wrote:
> On 9/7/23 09:14, Philippe Mathieu-Daudé wrote:
>> 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
>>               ^^^^^^^^^^^^^^                                
>> ^^^^^^^^^^^^^^^
>>
> 
> Of course the function is called 512 times: you asked for 512 cpus, and 
> each has its own address space which needs initializing.

The AS are still initialized at the same time, but we defer the listener
callback until the vCPU is ready (what was expected first IIUC).

> If you skip the call before cpu->created, when exactly are you going to 
> do it?

With this patch tcg_commit_cpu() is only called by vCPU threads, in
their processing loop. i.e: comparing backtraces, now the first hit
is:
(lldb) bt
* thread #514, stop reason = breakpoint 4.2
   * frame #0: 0x1005d9d48 
qemu-system-aarch64`tcg_commit_cpu(cpu=0x173358000, data=...) at 
physmem.c:2493:63
     frame #1: 0x10000d684 
qemu-system-aarch64`process_queued_cpu_work(cpu=0x173358000) at 
cpus-common.c:360:13
     frame #2: 0x100297390 qemu-system-aarch64`qemu_wait_io_event 
[inlined] qemu_wait_io_event_common(cpu=<unavailable>) at cpus.c:412:5 
[artificial]
     frame #3: 0x100623b98 
qemu-system-aarch64`mttcg_cpu_thread_fn(arg=0x173358000) at 
tcg-accel-ops-mttcg.c:123:9
     frame #4: 0x10079f15c 
qemu-system-aarch64`qemu_thread_start(args=<unavailable>) at 
qemu-thread-posix.c:541:9
     frame #5: 0x18880ffa8 libsystem_pthread.dylib`_pthread_start + 148