Philippe Mathieu-Daudé <phi...@linaro.org> writes:
> Hi Alex,
>
> On 29/5/24 17:22, Alex Bennée wrote:
>> This ensures we don't start the thread until cpu_common_realizefn has
>> finished. This ensures that plugins will always run
>> qemu_plugin_vcpu_init__async first before any other states. It doesn't
>> totally eliminate the race that plugin_cpu_update__locked has to work
>> around though. I found this while reviewing the ips plugin which makes
>> heavy use of the vcpu phase callbacks.
>> An alternative might be to move the explicit creation of vCPU
>> threads
>> to qdev_machine_creation_done()? It doesn't affect user-mode which
>> already has a thread to execute in and ensures the QOM object has
>> completed creation in cpu_create() before continuing.
>> Signed-off-by: Alex Bennée <alex.ben...@linaro.org>
>> Cc: Pierrick Bouvier <pierrick.bouv...@linaro.org>
>> Cc: Philippe Mathieu-Daudé <phi...@linaro.org>
>> ---
>> include/hw/core/cpu.h | 8 ++++++++
>> accel/tcg/user-exec-stub.c | 5 +++++
>> hw/core/cpu-common.c | 7 ++++++-
>> plugins/core.c | 5 +++++
>> system/cpus.c | 15 ++++++++++-----
>> 5 files changed, 34 insertions(+), 6 deletions(-)
>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>> index bb398e8237..6920699585 100644
>> --- a/include/hw/core/cpu.h
>> +++ b/include/hw/core/cpu.h
>> @@ -1041,6 +1041,14 @@ void end_exclusive(void);
>> */
>> void qemu_init_vcpu(CPUState *cpu);
>> +/**
>> + * qemu_start_vcpu:
>> + * @cpu: The vCPU to start.
>> + *
>> + * Create the vCPU thread and start it running.
>> + */
>> +void qemu_start_vcpu(CPUState *cpu);
>> +
>> #define SSTEP_ENABLE 0x1 /* Enable simulated HW single stepping */
>> #define SSTEP_NOIRQ 0x2 /* Do not use IRQ while single stepping */
>> #define SSTEP_NOTIMER 0x4 /* Do not Timers while single stepping */
>> diff --git a/accel/tcg/user-exec-stub.c b/accel/tcg/user-exec-stub.c
>> index 4fbe2dbdc8..162bb72bbe 100644
>> --- a/accel/tcg/user-exec-stub.c
>> +++ b/accel/tcg/user-exec-stub.c
>> @@ -18,6 +18,11 @@ void cpu_exec_reset_hold(CPUState *cpu)
>> {
>> }
>> +void qemu_start_vcpu(CPUState *cpu)
>> +{
>> + /* NOP for user-mode, we already have a thread */
>> +}
>> +
>> /* User mode emulation does not support record/replay yet. */
>> bool replay_exception(void)
>> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
>> index 0f0a247f56..68895ddd59 100644
>> --- a/hw/core/cpu-common.c
>> +++ b/hw/core/cpu-common.c
>> @@ -230,7 +230,12 @@ static void cpu_common_realizefn(DeviceState *dev,
>> Error **errp)
>> }
>> #endif
>> - /* NOTE: latest generic point where the cpu is fully realized
>> */
>> + /*
>> + * With everything set up we can finally start the vCPU thread.
>> + * This is a NOP for linux-user.
>> + * NOTE: latest generic point where the cpu is fully realized
>> + */
>> + qemu_start_vcpu(cpu);
>> }
>> static void cpu_common_unrealizefn(DeviceState *dev)
>> diff --git a/plugins/core.c b/plugins/core.c
>> index 0726bc7f25..1e5da7853b 100644
>> --- a/plugins/core.c
>> +++ b/plugins/core.c
>> @@ -65,6 +65,11 @@ static void plugin_cpu_update__locked(gpointer k,
>> gpointer v, gpointer udata)
>> CPUState *cpu = container_of(k, CPUState, cpu_index);
>> run_on_cpu_data mask = RUN_ON_CPU_HOST_ULONG(*plugin.mask);
>> + /*
>> + * There is a race condition between the starting of the vCPU
>> + * thread at the end of cpu_common_realizefn and when realized is
>> + * finally set.
>> + */
>
> I'd like we simply assert(DEVICE(cpu)->realized) here;
> I still don't understand when this can be called while
> the vcpu isn't yet realized.
It will be shortly but as the comment says we don't set it until we come
out of cpu_common_realizefn and return to QOM so you get:
**
ERROR:../../plugins/core.c:73:plugin_cpu_update__locked: assertion failed:
(DEVICE(cpu)->realized)
Bail out! ERROR:../../plugins/core.c:73:plugin_cpu_update__locked: assertion
failed: (DEVICE(cpu)->realized)
Thread 4 "qemu-system-aar" received signal SIGABRT, Aborted.
[Switching to Thread 0x7fffe5e006c0 (LWP 1000969)]
__pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6,
no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
44 ./nptl/pthread_kill.c: No such file or directory.
(gdb) bt
#0 __pthread_kill_implementation (threadid=<optimized out>,
signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
#1 0x00007ffff4ca9e8f in __pthread_kill_internal (signo=6,
threadid=<optimized out>) at ./nptl/pthread_kill.c:78
#2 0x00007ffff4c5afb2 in __GI_raise (sig=sig@entry=6) at
../sysdeps/posix/raise.c:26
#3 0x00007ffff4c45472 in __GI_abort () at ./stdlib/abort.c:79
#4 0x00007ffff6e46ec8 in () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#5 0x00007ffff6ea6e1a in g_assertion_message_expr () at
/lib/x86_64-linux-gnu/libglib-2.0.so.0
#6 0x000055555600e2c8 in plugin_cpu_update__locked (k=0x5555579e9ee8,
v=<optimized out>, udata=<optimized out>) at ../../plugins/core.c:73
#7 0x000055555600e5fc in qemu_plugin_vcpu_init_hook (cpu=0x5555579e9c20) at
../../plugins/core.c:261
#8 0x00005555558ff106 in process_queued_cpu_work (cpu=0x5555579e9c20) at
../../cpu-common.c:360
#9 0x00005555560100f6 in mttcg_cpu_thread_fn (arg=arg@entry=0x5555579e9c20)
at ../../accel/tcg/tcg-accel-ops-mttcg.c:118
#10 0x00005555561bcce8 in qemu_thread_start (args=0x555557a55d70) at
../../util/qemu-thread-posix.c:541
#11 0x00007ffff4ca8134 in start_thread (arg=<optimized out>) at
./nptl/pthread_create.c:442
#12 0x00007ffff4d287dc in clone3 () at
../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
Arguably I think we should just wait until machine is created I think.
>
>> if (DEVICE(cpu)->realized) {
>> async_run_on_cpu(cpu, plugin_cpu_update__async, mask);
>> } else {
>> diff --git a/system/cpus.c b/system/cpus.c
>> index d3640c9503..7dd8464c5e 100644
>> --- a/system/cpus.c
>> +++ b/system/cpus.c
>> @@ -488,11 +488,13 @@ void cpus_kick_thread(CPUState *cpu)
>> void qemu_cpu_kick(CPUState *cpu)
>> {
>> - qemu_cond_broadcast(cpu->halt_cond);
>> - if (cpus_accel->kick_vcpu_thread) {
>> - cpus_accel->kick_vcpu_thread(cpu);
>> - } else { /* default */
>> - cpus_kick_thread(cpu);
>> + if (cpu->halt_cond) {
>
> cpu->halt_cond = NULL is a bug, why kicking a vcpu not
> yet fully created?
We are queuing work for when it is ready but we don't create
cpu->halt_cond until in the thread (this is mainly to workaround the
fact the rr_thread shares its context between multiple CPUStates).
>
>> + qemu_cond_broadcast(cpu->halt_cond);
>> + if (cpus_accel->kick_vcpu_thread) {
>> + cpus_accel->kick_vcpu_thread(cpu);
>> + } else { /* default */
>> + cpus_kick_thread(cpu);
>> + }
>> }
>> }
>> @@ -674,7 +676,10 @@ void qemu_init_vcpu(CPUState *cpu)
>> cpu->num_ases = 1;
>> cpu_address_space_init(cpu, 0, "cpu-memory", cpu->memory);
>> }
>> +}
>> +void qemu_start_vcpu(CPUState *cpu)
>> +{
>> /* accelerators all implement the AccelOpsClass */
>> g_assert(cpus_accel != NULL && cpus_accel->create_vcpu_thread != NULL);
>> cpus_accel->create_vcpu_thread(cpu);
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
