On 5/29/24 08: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.
+ */
Could we simply have an active wait here?
while (!DEVICE(cpu)->realized) {}
We have a guarantee it will be realized shortly, and if it's too hard to
have a proper synchronization mechanism (introduce a realize_cond?),
then waiting for the proper state does not seem too bad.
It's a bit strange for me to document an existing race condition,
instead of finding a solution.
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) {
+ 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);
