[PATCH 0/5] cpus: a few tweaks to CPU realization

[PATCH 0/5] cpus: a few tweaks to CPU realization

[Alex Bennée]

The recent IPS plugin exposed a race condition between vcpu_init
callbacks and the other vcpu state callbacks. I originally thought
there was some wider re-factoring to be done to clean this up but it
turns out things are broadly where they should be. However some of the
stuff allocated in the vCPU threads can clearly be done earlier so
I've moved enough from cpu_common_realizefn to cpu_common_initfn to
allow plugins to queue work before the threads start solving the race.

Please review.

Alex Bennée (5):
  hw/core: expand on the alignment of CPUState
  cpu: move Qemu[Thread|Cond] setup into common code
  cpu-target: don't set cpu->thread_id to bogus value
  plugins: remove special casing for cpu->realized
  core/cpu-common: initialise plugin state before thread creation

 include/hw/core/cpu.h             | 18 ++++++++++++++----
 accel/dummy-cpus.c                |  3 ---
 accel/hvf/hvf-accel-ops.c         |  4 ----
 accel/kvm/kvm-accel-ops.c         |  3 ---
 accel/tcg/tcg-accel-ops-mttcg.c   |  4 ----
 accel/tcg/tcg-accel-ops-rr.c      | 14 +++++++-------
 cpu-target.c                      |  1 -
 hw/core/cpu-common.c              | 25 +++++++++++++++++--------
 plugins/core.c                    |  6 +-----
 target/i386/nvmm/nvmm-accel-ops.c |  3 ---
 target/i386/whpx/whpx-accel-ops.c |  3 ---
 11 files changed, 39 insertions(+), 45 deletions(-)

-- 
2.39.2

[PATCH 1/5] hw/core: expand on the alignment of CPUState

[Alex Bennée]

Make the relationship between CPUState, ArchCPU and cpu_env a bit
clearer in the kdoc comments.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/core/cpu.h | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index bb398e8237..35d345371b 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -391,7 +391,8 @@ struct qemu_work_item;
 #define CPU_UNSET_NUMA_NODE_ID -1
 
 /**
- * CPUState:
+ * struct CPUState - common state of one CPU core or thread.
+ *
  * @cpu_index: CPU index (informative).
  * @cluster_index: Identifies which cluster this CPU is in.
  *   For boards which don't define clusters or for "loose" CPUs not assigned
@@ -439,10 +440,15 @@ struct qemu_work_item;
  * @kvm_fetch_index: Keeps the index that we last fetched from the per-vCPU
  *    dirty ring structure.
  *
- * State of one CPU core or thread.
+ * @neg_align: The CPUState is the common part of a concrete ArchCPU
+ * which is allocated when an individual CPU instance is created. As
+ * such care is taken is ensure there is no gap between between
+ * CPUState and CPUArchState within ArchCPU.
  *
- * Align, in order to match possible alignment required by CPUArchState,
- * and eliminate a hole between CPUState and CPUArchState within ArchCPU.
+ * @neg: The architectural register state ("cpu_env") immediately follows CPUState
+ * in ArchCPU and is passed to TCG code. The @neg structure holds some
+ * common TCG CPU variables which are accessed with a negative offset
+ * from cpu_env.
  */
 struct CPUState {
     /*< private >*/
-- 
2.39.2

Re: [PATCH 1/5] hw/core: expand on the alignment of CPUState

[Pierrick Bouvier]

On 5/30/24 12:42, Alex Bennée wrote:
> Make the relationship between CPUState, ArchCPU and cpu_env a bit
> clearer in the kdoc comments.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/hw/core/cpu.h | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index bb398e8237..35d345371b 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -391,7 +391,8 @@ struct qemu_work_item;
>   #define CPU_UNSET_NUMA_NODE_ID -1
>   
>   /**
> - * CPUState:
> + * struct CPUState - common state of one CPU core or thread.
> + *
>    * @cpu_index: CPU index (informative).
>    * @cluster_index: Identifies which cluster this CPU is in.
>    *   For boards which don't define clusters or for "loose" CPUs not assigned
> @@ -439,10 +440,15 @@ struct qemu_work_item;
>    * @kvm_fetch_index: Keeps the index that we last fetched from the per-vCPU
>    *    dirty ring structure.
>    *
> - * State of one CPU core or thread.
> + * @neg_align: The CPUState is the common part of a concrete ArchCPU
> + * which is allocated when an individual CPU instance is created. As
> + * such care is taken is ensure there is no gap between between
> + * CPUState and CPUArchState within ArchCPU.
>    *
> - * Align, in order to match possible alignment required by CPUArchState,
> - * and eliminate a hole between CPUState and CPUArchState within ArchCPU.
> + * @neg: The architectural register state ("cpu_env") immediately follows CPUState
> + * in ArchCPU and is passed to TCG code. The @neg structure holds some
> + * common TCG CPU variables which are accessed with a negative offset
> + * from cpu_env.
>    */
>   struct CPUState {
>       /*< private >*/

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 1/5] hw/core: expand on the alignment of CPUState

[Philippe Mathieu-Daudé]

On 30/5/24 21:42, Alex Bennée wrote:
> Make the relationship between CPUState, ArchCPU and cpu_env a bit
> clearer in the kdoc comments.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/hw/core/cpu.h | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 2/5] cpu: move Qemu[Thread|Cond] setup into common code

[Alex Bennée]

Aside from the round robin threads this is all common code. By
moving the halt_cond setup we also no longer need hacks to work around
the race between QOM object creation and thread creation.

It is a little ugly to free stuff up for the round robin thread but
better it deal with its own specialises than making the other
accelerators jump through hoops.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/core/cpu.h             |  4 ++++
 accel/dummy-cpus.c                |  3 ---
 accel/hvf/hvf-accel-ops.c         |  4 ----
 accel/kvm/kvm-accel-ops.c         |  3 ---
 accel/tcg/tcg-accel-ops-mttcg.c   |  4 ----
 accel/tcg/tcg-accel-ops-rr.c      | 14 +++++++-------
 hw/core/cpu-common.c              |  5 +++++
 target/i386/nvmm/nvmm-accel-ops.c |  3 ---
 target/i386/whpx/whpx-accel-ops.c |  3 ---
 9 files changed, 16 insertions(+), 27 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 35d345371b..a405119eda 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -404,10 +404,14 @@ struct qemu_work_item;
  * @tcg_cflags: Pre-computed cflags for this cpu.
  * @nr_cores: Number of cores within this CPU package.
  * @nr_threads: Number of threads within this CPU core.
+ * @thread: Host thread details, only live once @created is #true
+ * @sem: WIN32 only semaphore used only for qtest
+ * @thread_id: native thread id of vCPU, only live once @created is #true
  * @running: #true if CPU is currently running (lockless).
  * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;
  * valid under cpu_list_lock.
  * @created: Indicates whether the CPU thread has been successfully created.
+ * @halt_cond: condition variable sleeping threads can wait on.
  * @interrupt_request: Indicates a pending interrupt request.
  * @halted: Nonzero if the CPU is in suspended state.
  * @stop: Indicates a pending stop request.
diff --git a/accel/dummy-cpus.c b/accel/dummy-cpus.c
index 20519f1ea4..f32d8c8dc3 100644
--- a/accel/dummy-cpus.c
+++ b/accel/dummy-cpus.c
@@ -68,9 +68,6 @@ void dummy_start_vcpu_thread(CPUState *cpu)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];
 
-    cpu->thread = g_malloc0(sizeof(QemuThread));
-    cpu->halt_cond = g_malloc0(sizeof(QemuCond));
-    qemu_cond_init(cpu->halt_cond);
     snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/DUMMY",
              cpu->cpu_index);
     qemu_thread_create(cpu->thread, thread_name, dummy_cpu_thread_fn, cpu,
diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c
index 40d4187d9d..6f1e27ef46 100644
--- a/accel/hvf/hvf-accel-ops.c
+++ b/accel/hvf/hvf-accel-ops.c
@@ -463,10 +463,6 @@ static void hvf_start_vcpu_thread(CPUState *cpu)
      */
     assert(hvf_enabled());
 
-    cpu->thread = g_malloc0(sizeof(QemuThread));
-    cpu->halt_cond = g_malloc0(sizeof(QemuCond));
-    qemu_cond_init(cpu->halt_cond);
-
     snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HVF",
              cpu->cpu_index);
     qemu_thread_create(cpu->thread, thread_name, hvf_cpu_thread_fn,
diff --git a/accel/kvm/kvm-accel-ops.c b/accel/kvm/kvm-accel-ops.c
index 94c828ac8d..c239dfc87a 100644
--- a/accel/kvm/kvm-accel-ops.c
+++ b/accel/kvm/kvm-accel-ops.c
@@ -66,9 +66,6 @@ static void kvm_start_vcpu_thread(CPUState *cpu)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];
 
-    cpu->thread = g_malloc0(sizeof(QemuThread));
-    cpu->halt_cond = g_malloc0(sizeof(QemuCond));
-    qemu_cond_init(cpu->halt_cond);
     snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/KVM",
              cpu->cpu_index);
     qemu_thread_create(cpu->thread, thread_name, kvm_vcpu_thread_fn,
diff --git a/accel/tcg/tcg-accel-ops-mttcg.c b/accel/tcg/tcg-accel-ops-mttcg.c
index c552b45b8e..49814ec4af 100644
--- a/accel/tcg/tcg-accel-ops-mttcg.c
+++ b/accel/tcg/tcg-accel-ops-mttcg.c
@@ -137,10 +137,6 @@ void mttcg_start_vcpu_thread(CPUState *cpu)
     g_assert(tcg_enabled());
     tcg_cpu_init_cflags(cpu, current_machine->smp.max_cpus > 1);
 
-    cpu->thread = g_new0(QemuThread, 1);
-    cpu->halt_cond = g_malloc0(sizeof(QemuCond));
-    qemu_cond_init(cpu->halt_cond);
-
     /* create a thread per vCPU with TCG (MTTCG) */
     snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG",
              cpu->cpu_index);
diff --git a/accel/tcg/tcg-accel-ops-rr.c b/accel/tcg/tcg-accel-ops-rr.c
index 894e73e52c..84c36c1450 100644
--- a/accel/tcg/tcg-accel-ops-rr.c
+++ b/accel/tcg/tcg-accel-ops-rr.c
@@ -317,22 +317,22 @@ void rr_start_vcpu_thread(CPUState *cpu)
     tcg_cpu_init_cflags(cpu, false);
 
     if (!single_tcg_cpu_thread) {
-        cpu->thread = g_new0(QemuThread, 1);
-        cpu->halt_cond = g_new0(QemuCond, 1);
-        qemu_cond_init(cpu->halt_cond);
+        single_tcg_halt_cond = cpu->halt_cond;
+        single_tcg_cpu_thread = cpu->thread;
 
         /* share a single thread for all cpus with TCG */
         snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "ALL CPUs/TCG");
         qemu_thread_create(cpu->thread, thread_name,
                            rr_cpu_thread_fn,
                            cpu, QEMU_THREAD_JOINABLE);
-
-        single_tcg_halt_cond = cpu->halt_cond;
-        single_tcg_cpu_thread = cpu->thread;
     } else {
-        /* we share the thread */
+        /* we share the thread, dump spare data */
+        g_free(cpu->thread);
+        qemu_cond_destroy(cpu->halt_cond);
         cpu->thread = single_tcg_cpu_thread;
         cpu->halt_cond = single_tcg_halt_cond;
+
+        /* copy the stuff done at start of rr_cpu_thread_fn */
         cpu->thread_id = first_cpu->thread_id;
         cpu->neg.can_do_io = 1;
         cpu->created = true;
diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index 0f0a247f56..6cfc01593a 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -261,6 +261,11 @@ static void cpu_common_initfn(Object *obj)
     cpu->nr_threads = 1;
     cpu->cflags_next_tb = -1;
 
+    /* allocate storage for thread info, initialise condition variables */
+    cpu->thread = g_new0(QemuThread, 1);
+    cpu->halt_cond = g_new0(QemuCond, 1);
+    qemu_cond_init(cpu->halt_cond);
+
     qemu_mutex_init(&cpu->work_mutex);
     qemu_lockcnt_init(&cpu->in_ioctl_lock);
     QSIMPLEQ_INIT(&cpu->work_list);
diff --git a/target/i386/nvmm/nvmm-accel-ops.c b/target/i386/nvmm/nvmm-accel-ops.c
index 6b2bfd9b9c..0ba31201e2 100644
--- a/target/i386/nvmm/nvmm-accel-ops.c
+++ b/target/i386/nvmm/nvmm-accel-ops.c
@@ -64,9 +64,6 @@ static void nvmm_start_vcpu_thread(CPUState *cpu)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];
 
-    cpu->thread = g_new0(QemuThread, 1);
-    cpu->halt_cond = g_new0(QemuCond, 1);
-    qemu_cond_init(cpu->halt_cond);
     snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/NVMM",
              cpu->cpu_index);
     qemu_thread_create(cpu->thread, thread_name, qemu_nvmm_cpu_thread_fn,
diff --git a/target/i386/whpx/whpx-accel-ops.c b/target/i386/whpx/whpx-accel-ops.c
index 189ae0f140..1a2b4e1c43 100644
--- a/target/i386/whpx/whpx-accel-ops.c
+++ b/target/i386/whpx/whpx-accel-ops.c
@@ -64,9 +64,6 @@ static void whpx_start_vcpu_thread(CPUState *cpu)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];
 
-    cpu->thread = g_new0(QemuThread, 1);
-    cpu->halt_cond = g_new0(QemuCond, 1);
-    qemu_cond_init(cpu->halt_cond);
     snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/WHPX",
              cpu->cpu_index);
     qemu_thread_create(cpu->thread, thread_name, whpx_cpu_thread_fn,
-- 
2.39.2

Re: [PATCH 2/5] cpu: move Qemu[Thread|Cond] setup into common code

[Pierrick Bouvier]

On 5/30/24 12:42, Alex Bennée wrote:
> Aside from the round robin threads this is all common code. By
> moving the halt_cond setup we also no longer need hacks to work around
> the race between QOM object creation and thread creation.
> 
> It is a little ugly to free stuff up for the round robin thread but
> better it deal with its own specialises than making the other
> accelerators jump through hoops.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/hw/core/cpu.h             |  4 ++++
>   accel/dummy-cpus.c                |  3 ---
>   accel/hvf/hvf-accel-ops.c         |  4 ----
>   accel/kvm/kvm-accel-ops.c         |  3 ---
>   accel/tcg/tcg-accel-ops-mttcg.c   |  4 ----
>   accel/tcg/tcg-accel-ops-rr.c      | 14 +++++++-------
>   hw/core/cpu-common.c              |  5 +++++
>   target/i386/nvmm/nvmm-accel-ops.c |  3 ---
>   target/i386/whpx/whpx-accel-ops.c |  3 ---
>   9 files changed, 16 insertions(+), 27 deletions(-)
> 
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 35d345371b..a405119eda 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -404,10 +404,14 @@ struct qemu_work_item;
>    * @tcg_cflags: Pre-computed cflags for this cpu.
>    * @nr_cores: Number of cores within this CPU package.
>    * @nr_threads: Number of threads within this CPU core.
> + * @thread: Host thread details, only live once @created is #true
> + * @sem: WIN32 only semaphore used only for qtest
> + * @thread_id: native thread id of vCPU, only live once @created is #true
>    * @running: #true if CPU is currently running (lockless).
>    * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end;
>    * valid under cpu_list_lock.
>    * @created: Indicates whether the CPU thread has been successfully created.
> + * @halt_cond: condition variable sleeping threads can wait on.
>    * @interrupt_request: Indicates a pending interrupt request.
>    * @halted: Nonzero if the CPU is in suspended state.
>    * @stop: Indicates a pending stop request.
> diff --git a/accel/dummy-cpus.c b/accel/dummy-cpus.c
> index 20519f1ea4..f32d8c8dc3 100644
> --- a/accel/dummy-cpus.c
> +++ b/accel/dummy-cpus.c
> @@ -68,9 +68,6 @@ void dummy_start_vcpu_thread(CPUState *cpu)
>   {
>       char thread_name[VCPU_THREAD_NAME_SIZE];
>   
> -    cpu->thread = g_malloc0(sizeof(QemuThread));
> -    cpu->halt_cond = g_malloc0(sizeof(QemuCond));
> -    qemu_cond_init(cpu->halt_cond);
>       snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/DUMMY",
>                cpu->cpu_index);
>       qemu_thread_create(cpu->thread, thread_name, dummy_cpu_thread_fn, cpu,
> diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c
> index 40d4187d9d..6f1e27ef46 100644
> --- a/accel/hvf/hvf-accel-ops.c
> +++ b/accel/hvf/hvf-accel-ops.c
> @@ -463,10 +463,6 @@ static void hvf_start_vcpu_thread(CPUState *cpu)
>        */
>       assert(hvf_enabled());
>   
> -    cpu->thread = g_malloc0(sizeof(QemuThread));
> -    cpu->halt_cond = g_malloc0(sizeof(QemuCond));
> -    qemu_cond_init(cpu->halt_cond);
> -
>       snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HVF",
>                cpu->cpu_index);
>       qemu_thread_create(cpu->thread, thread_name, hvf_cpu_thread_fn,
> diff --git a/accel/kvm/kvm-accel-ops.c b/accel/kvm/kvm-accel-ops.c
> index 94c828ac8d..c239dfc87a 100644
> --- a/accel/kvm/kvm-accel-ops.c
> +++ b/accel/kvm/kvm-accel-ops.c
> @@ -66,9 +66,6 @@ static void kvm_start_vcpu_thread(CPUState *cpu)
>   {
>       char thread_name[VCPU_THREAD_NAME_SIZE];
>   
> -    cpu->thread = g_malloc0(sizeof(QemuThread));
> -    cpu->halt_cond = g_malloc0(sizeof(QemuCond));
> -    qemu_cond_init(cpu->halt_cond);
>       snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/KVM",
>                cpu->cpu_index);
>       qemu_thread_create(cpu->thread, thread_name, kvm_vcpu_thread_fn,
> diff --git a/accel/tcg/tcg-accel-ops-mttcg.c b/accel/tcg/tcg-accel-ops-mttcg.c
> index c552b45b8e..49814ec4af 100644
> --- a/accel/tcg/tcg-accel-ops-mttcg.c
> +++ b/accel/tcg/tcg-accel-ops-mttcg.c
> @@ -137,10 +137,6 @@ void mttcg_start_vcpu_thread(CPUState *cpu)
>       g_assert(tcg_enabled());
>       tcg_cpu_init_cflags(cpu, current_machine->smp.max_cpus > 1);
>   
> -    cpu->thread = g_new0(QemuThread, 1);
> -    cpu->halt_cond = g_malloc0(sizeof(QemuCond));
> -    qemu_cond_init(cpu->halt_cond);
> -
>       /* create a thread per vCPU with TCG (MTTCG) */
>       snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG",
>                cpu->cpu_index);
> diff --git a/accel/tcg/tcg-accel-ops-rr.c b/accel/tcg/tcg-accel-ops-rr.c
> index 894e73e52c..84c36c1450 100644
> --- a/accel/tcg/tcg-accel-ops-rr.c
> +++ b/accel/tcg/tcg-accel-ops-rr.c
> @@ -317,22 +317,22 @@ void rr_start_vcpu_thread(CPUState *cpu)
>       tcg_cpu_init_cflags(cpu, false);
>   
>       if (!single_tcg_cpu_thread) {
> -        cpu->thread = g_new0(QemuThread, 1);
> -        cpu->halt_cond = g_new0(QemuCond, 1);
> -        qemu_cond_init(cpu->halt_cond);
> +        single_tcg_halt_cond = cpu->halt_cond;
> +        single_tcg_cpu_thread = cpu->thread;
>   
>           /* share a single thread for all cpus with TCG */
>           snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "ALL CPUs/TCG");
>           qemu_thread_create(cpu->thread, thread_name,
>                              rr_cpu_thread_fn,
>                              cpu, QEMU_THREAD_JOINABLE);
> -
> -        single_tcg_halt_cond = cpu->halt_cond;
> -        single_tcg_cpu_thread = cpu->thread;
>       } else {
> -        /* we share the thread */
> +        /* we share the thread, dump spare data */
> +        g_free(cpu->thread);
> +        qemu_cond_destroy(cpu->halt_cond);
>           cpu->thread = single_tcg_cpu_thread;
>           cpu->halt_cond = single_tcg_halt_cond;
> +
> +        /* copy the stuff done at start of rr_cpu_thread_fn */
>           cpu->thread_id = first_cpu->thread_id;
>           cpu->neg.can_do_io = 1;
>           cpu->created = true;
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index 0f0a247f56..6cfc01593a 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -261,6 +261,11 @@ static void cpu_common_initfn(Object *obj)
>       cpu->nr_threads = 1;
>       cpu->cflags_next_tb = -1;
>   
> +    /* allocate storage for thread info, initialise condition variables */
> +    cpu->thread = g_new0(QemuThread, 1);
> +    cpu->halt_cond = g_new0(QemuCond, 1);
> +    qemu_cond_init(cpu->halt_cond);
> +
>       qemu_mutex_init(&cpu->work_mutex);
>       qemu_lockcnt_init(&cpu->in_ioctl_lock);
>       QSIMPLEQ_INIT(&cpu->work_list);
> diff --git a/target/i386/nvmm/nvmm-accel-ops.c b/target/i386/nvmm/nvmm-accel-ops.c
> index 6b2bfd9b9c..0ba31201e2 100644
> --- a/target/i386/nvmm/nvmm-accel-ops.c
> +++ b/target/i386/nvmm/nvmm-accel-ops.c
> @@ -64,9 +64,6 @@ static void nvmm_start_vcpu_thread(CPUState *cpu)
>   {
>       char thread_name[VCPU_THREAD_NAME_SIZE];
>   
> -    cpu->thread = g_new0(QemuThread, 1);
> -    cpu->halt_cond = g_new0(QemuCond, 1);
> -    qemu_cond_init(cpu->halt_cond);
>       snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/NVMM",
>                cpu->cpu_index);
>       qemu_thread_create(cpu->thread, thread_name, qemu_nvmm_cpu_thread_fn,
> diff --git a/target/i386/whpx/whpx-accel-ops.c b/target/i386/whpx/whpx-accel-ops.c
> index 189ae0f140..1a2b4e1c43 100644
> --- a/target/i386/whpx/whpx-accel-ops.c
> +++ b/target/i386/whpx/whpx-accel-ops.c
> @@ -64,9 +64,6 @@ static void whpx_start_vcpu_thread(CPUState *cpu)
>   {
>       char thread_name[VCPU_THREAD_NAME_SIZE];
>   
> -    cpu->thread = g_new0(QemuThread, 1);
> -    cpu->halt_cond = g_new0(QemuCond, 1);
> -    qemu_cond_init(cpu->halt_cond);
>       snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/WHPX",
>                cpu->cpu_index);
>       qemu_thread_create(cpu->thread, thread_name, whpx_cpu_thread_fn,

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 2/5] cpu: move Qemu[Thread|Cond] setup into common code

[Reinoud Zandijk]

On Thu, May 30, 2024 at 03:29:41PM -0700, Pierrick Bouvier wrote:
> On 5/30/24 12:42, Alex Bennée wrote:
> > Aside from the round robin threads this is all common code. By
> > moving the halt_cond setup we also no longer need hacks to work around
> > the race between QOM object creation and thread creation.
> > 
> > It is a little ugly to free stuff up for the round robin thread but
> > better it deal with its own specialises than making the other
> > accelerators jump through hoops.
> > 
> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
...
> > diff --git a/target/i386/nvmm/nvmm-accel-ops.c b/target/i386/nvmm/nvmm-accel-ops.c
> > index 6b2bfd9b9c..0ba31201e2 100644
> > --- a/target/i386/nvmm/nvmm-accel-ops.c
> > +++ b/target/i386/nvmm/nvmm-accel-ops.c
> > @@ -64,9 +64,6 @@ static void nvmm_start_vcpu_thread(CPUState *cpu)
> >   {
> >       char thread_name[VCPU_THREAD_NAME_SIZE];
> > -    cpu->thread = g_new0(QemuThread, 1);
> > -    cpu->halt_cond = g_new0(QemuCond, 1);
> > -    qemu_cond_init(cpu->halt_cond);
> >       snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/NVMM",
> >                cpu->cpu_index);
> >       qemu_thread_create(cpu->thread, thread_name, qemu_nvmm_cpu_thread_fn,

I haven't tested it since I don't have a recent qemu build but I doubt it will
give issues as its main qemu stuff.

Reinoud

Re: [PATCH 2/5] cpu: move Qemu[Thread|Cond] setup into common code

[Philippe Mathieu-Daudé]

On 30/5/24 21:42, Alex Bennée wrote:
> Aside from the round robin threads this is all common code. By
> moving the halt_cond setup we also no longer need hacks to work around
> the race between QOM object creation and thread creation.
> 
> It is a little ugly to free stuff up for the round robin thread but
> better it deal with its own specialises than making the other
> accelerators jump through hoops.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/hw/core/cpu.h             |  4 ++++
>   accel/dummy-cpus.c                |  3 ---
>   accel/hvf/hvf-accel-ops.c         |  4 ----
>   accel/kvm/kvm-accel-ops.c         |  3 ---
>   accel/tcg/tcg-accel-ops-mttcg.c   |  4 ----
>   accel/tcg/tcg-accel-ops-rr.c      | 14 +++++++-------
>   hw/core/cpu-common.c              |  5 +++++
>   target/i386/nvmm/nvmm-accel-ops.c |  3 ---
>   target/i386/whpx/whpx-accel-ops.c |  3 ---
>   9 files changed, 16 insertions(+), 27 deletions(-)


> diff --git a/accel/tcg/tcg-accel-ops-rr.c b/accel/tcg/tcg-accel-ops-rr.c
> index 894e73e52c..84c36c1450 100644
> --- a/accel/tcg/tcg-accel-ops-rr.c
> +++ b/accel/tcg/tcg-accel-ops-rr.c
> @@ -317,22 +317,22 @@ void rr_start_vcpu_thread(CPUState *cpu)
>       tcg_cpu_init_cflags(cpu, false);
>   
>       if (!single_tcg_cpu_thread) {
> -        cpu->thread = g_new0(QemuThread, 1);
> -        cpu->halt_cond = g_new0(QemuCond, 1);
> -        qemu_cond_init(cpu->halt_cond);
> +        single_tcg_halt_cond = cpu->halt_cond;
> +        single_tcg_cpu_thread = cpu->thread;
>   
>           /* share a single thread for all cpus with TCG */
>           snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "ALL CPUs/TCG");
>           qemu_thread_create(cpu->thread, thread_name,
>                              rr_cpu_thread_fn,
>                              cpu, QEMU_THREAD_JOINABLE);
> -
> -        single_tcg_halt_cond = cpu->halt_cond;
> -        single_tcg_cpu_thread = cpu->thread;
>       } else {
> -        /* we share the thread */
> +        /* we share the thread, dump spare data */

/* we share the thread, release allocations from cpu_common_initfn() */

> +        g_free(cpu->thread);
> +        qemu_cond_destroy(cpu->halt_cond);
>           cpu->thread = single_tcg_cpu_thread;
>           cpu->halt_cond = single_tcg_halt_cond;
> +
> +        /* copy the stuff done at start of rr_cpu_thread_fn */
>           cpu->thread_id = first_cpu->thread_id;
>           cpu->neg.can_do_io = 1;
>           cpu->created = true;

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 3/5] cpu-target: don't set cpu->thread_id to bogus value

[Alex Bennée]

The thread_id isn't valid until the threads are created. There is no
point setting it here. The only thing that cares about the thread_id
is qmp_query_cpus_fast.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 cpu-target.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/cpu-target.c b/cpu-target.c
index 5af120e8aa..499facf774 100644
--- a/cpu-target.c
+++ b/cpu-target.c
@@ -241,7 +241,6 @@ void cpu_exec_initfn(CPUState *cpu)
     cpu->num_ases = 0;
 
 #ifndef CONFIG_USER_ONLY
-    cpu->thread_id = qemu_get_thread_id();
     cpu->memory = get_system_memory();
     object_ref(OBJECT(cpu->memory));
 #endif
-- 
2.39.2

Re: [PATCH 3/5] cpu-target: don't set cpu->thread_id to bogus value

[Pierrick Bouvier]

On 5/30/24 12:42, Alex Bennée wrote:
> The thread_id isn't valid until the threads are created. There is no
> point setting it here. The only thing that cares about the thread_id
> is qmp_query_cpus_fast.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   cpu-target.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/cpu-target.c b/cpu-target.c
> index 5af120e8aa..499facf774 100644
> --- a/cpu-target.c
> +++ b/cpu-target.c
> @@ -241,7 +241,6 @@ void cpu_exec_initfn(CPUState *cpu)
>       cpu->num_ases = 0;
>   
>   #ifndef CONFIG_USER_ONLY
> -    cpu->thread_id = qemu_get_thread_id();
>       cpu->memory = get_system_memory();
>       object_ref(OBJECT(cpu->memory));
>   #endif

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 3/5] cpu-target: don't set cpu->thread_id to bogus value

[Philippe Mathieu-Daudé]

On 30/5/24 21:42, Alex Bennée wrote:
> The thread_id isn't valid until the threads are created. There is no
> point setting it here. The only thing that cares about the thread_id
> is qmp_query_cpus_fast.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   cpu-target.c | 1 -
>   1 file changed, 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 4/5] plugins: remove special casing for cpu->realized

[Alex Bennée]

Now the condition variable is initialised early on we don't need to go
through hoops to avoid calling async_run_on_cpu.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 plugins/core.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/plugins/core.c b/plugins/core.c
index 0726bc7f25..badede28cf 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -65,11 +65,7 @@ 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);
 
-    if (DEVICE(cpu)->realized) {
-        async_run_on_cpu(cpu, plugin_cpu_update__async, mask);
-    } else {
-        plugin_cpu_update__async(cpu, mask);
-    }
+    async_run_on_cpu(cpu, plugin_cpu_update__async, mask);
 }
 
 void plugin_unregister_cb__locked(struct qemu_plugin_ctx *ctx,
-- 
2.39.2

Re: [PATCH 4/5] plugins: remove special casing for cpu->realized

[Pierrick Bouvier]

On 5/30/24 12:42, Alex Bennée wrote:
> Now the condition variable is initialised early on we don't need to go
> through hoops to avoid calling async_run_on_cpu.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   plugins/core.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/plugins/core.c b/plugins/core.c
> index 0726bc7f25..badede28cf 100644
> --- a/plugins/core.c
> +++ b/plugins/core.c
> @@ -65,11 +65,7 @@ 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);
>   
> -    if (DEVICE(cpu)->realized) {
> -        async_run_on_cpu(cpu, plugin_cpu_update__async, mask);
> -    } else {
> -        plugin_cpu_update__async(cpu, mask);
> -    }
> +    async_run_on_cpu(cpu, plugin_cpu_update__async, mask);
>   }
>   
>   void plugin_unregister_cb__locked(struct qemu_plugin_ctx *ctx,

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 4/5] plugins: remove special casing for cpu->realized

[Philippe Mathieu-Daudé]

On 30/5/24 21:42, Alex Bennée wrote:
> Now the condition variable is initialised early on we don't need to go
> through hoops to avoid calling async_run_on_cpu.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   plugins/core.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/plugins/core.c b/plugins/core.c
> index 0726bc7f25..badede28cf 100644
> --- a/plugins/core.c
> +++ b/plugins/core.c
> @@ -65,11 +65,7 @@ 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);
>   
> -    if (DEVICE(cpu)->realized) {

We could assert() this to protect future refactors.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> -        async_run_on_cpu(cpu, plugin_cpu_update__async, mask);
> -    } else {
> -        plugin_cpu_update__async(cpu, mask);
> -    }
> +    async_run_on_cpu(cpu, plugin_cpu_update__async, mask);
>   }
>   
>   void plugin_unregister_cb__locked(struct qemu_plugin_ctx *ctx,

Re: [PATCH 4/5] plugins: remove special casing for cpu->realized

[Philippe Mathieu-Daudé]

On 3/6/24 13:31, Philippe Mathieu-Daudé wrote:
> On 30/5/24 21:42, Alex Bennée wrote:
>> Now the condition variable is initialised early on we don't need to go
>> through hoops to avoid calling async_run_on_cpu.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   plugins/core.c | 6 +-----
>>   1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/plugins/core.c b/plugins/core.c
>> index 0726bc7f25..badede28cf 100644
>> --- a/plugins/core.c
>> +++ b/plugins/core.c
>> @@ -65,11 +65,7 @@ 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);
>> -    if (DEVICE(cpu)->realized) {
> 
> We could assert() this to protect future refactors.

(No we can't because vCPU can still be unrealized at this point).

> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
>> -        async_run_on_cpu(cpu, plugin_cpu_update__async, mask);
>> -    } else {
>> -        plugin_cpu_update__async(cpu, mask);
>> -    }
>> +    async_run_on_cpu(cpu, plugin_cpu_update__async, mask);
>>   }
>>   void plugin_unregister_cb__locked(struct qemu_plugin_ctx *ctx,
> 

Re: [PATCH 4/5] plugins: remove special casing for cpu->realized

[Alex Bennée]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 30/5/24 21:42, Alex Bennée wrote:
>> Now the condition variable is initialised early on we don't need to go
>> through hoops to avoid calling async_run_on_cpu.
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   plugins/core.c | 6 +-----
>>   1 file changed, 1 insertion(+), 5 deletions(-)
>> diff --git a/plugins/core.c b/plugins/core.c
>> index 0726bc7f25..badede28cf 100644
>> --- a/plugins/core.c
>> +++ b/plugins/core.c
>> @@ -65,11 +65,7 @@ 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);
>>   -    if (DEVICE(cpu)->realized) {
>
> We could assert() this to protect future refactors.

No because the CPU can still not be realized but it will be able to
queue async work.

>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
>> -        async_run_on_cpu(cpu, plugin_cpu_update__async, mask);
>> -    } else {
>> -        plugin_cpu_update__async(cpu, mask);
>> -    }
>> +    async_run_on_cpu(cpu, plugin_cpu_update__async, mask);
>>   }
>>     void plugin_unregister_cb__locked(struct qemu_plugin_ctx *ctx,

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 5/5] core/cpu-common: initialise plugin state before thread creation

[Alex Bennée]

Originally I tried to move where vCPU thread initialisation to later
in realize. However pulling that thread (sic) got gnarly really
quickly. It turns out some steps of CPU realization need values that
can only be determined from the running vCPU thread.

However having moved enough out of the thread creation we can now
queue work before the thread starts (at least for TCG guests) and
avoid the race between vcpu_init and other vcpu states a plugin might
subscribe to.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/core/cpu-common.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index 6cfc01593a..bf1a7b8892 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -222,14 +222,6 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
         cpu_resume(cpu);
     }
 
-    /* Plugin initialization must wait until the cpu start executing code */
-#ifdef CONFIG_PLUGIN
-    if (tcg_enabled()) {
-        cpu->plugin_state = qemu_plugin_create_vcpu_state();
-        async_run_on_cpu(cpu, qemu_plugin_vcpu_init__async, RUN_ON_CPU_NULL);
-    }
-#endif
-
     /* NOTE: latest generic point where the cpu is fully realized */
 }
 
@@ -273,6 +265,18 @@ static void cpu_common_initfn(Object *obj)
     QTAILQ_INIT(&cpu->watchpoints);
 
     cpu_exec_initfn(cpu);
+
+    /*
+     * Plugin initialization must wait until the cpu start executing
+     * code, but we must queue this work before the threads are
+     * created to ensure we don't race.
+     */
+#ifdef CONFIG_PLUGIN
+    if (tcg_enabled()) {
+        cpu->plugin_state = qemu_plugin_create_vcpu_state();
+        async_run_on_cpu(cpu, qemu_plugin_vcpu_init__async, RUN_ON_CPU_NULL);
+    }
+#endif
 }
 
 static void cpu_common_finalize(Object *obj)
-- 
2.39.2

Re: [PATCH 5/5] core/cpu-common: initialise plugin state before thread creation

[Pierrick Bouvier]

On 5/30/24 12:42, Alex Bennée wrote:
> Originally I tried to move where vCPU thread initialisation to later
> in realize. However pulling that thread (sic) got gnarly really
> quickly. It turns out some steps of CPU realization need values that
> can only be determined from the running vCPU thread.
> 
> However having moved enough out of the thread creation we can now
> queue work before the thread starts (at least for TCG guests) and
> avoid the race between vcpu_init and other vcpu states a plugin might
> subscribe to.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   hw/core/cpu-common.c | 20 ++++++++++++--------
>   1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index 6cfc01593a..bf1a7b8892 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -222,14 +222,6 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>           cpu_resume(cpu);
>       }
>   
> -    /* Plugin initialization must wait until the cpu start executing code */
> -#ifdef CONFIG_PLUGIN
> -    if (tcg_enabled()) {
> -        cpu->plugin_state = qemu_plugin_create_vcpu_state();
> -        async_run_on_cpu(cpu, qemu_plugin_vcpu_init__async, RUN_ON_CPU_NULL);
> -    }
> -#endif
> -
>       /* NOTE: latest generic point where the cpu is fully realized */
>   }
>   
> @@ -273,6 +265,18 @@ static void cpu_common_initfn(Object *obj)
>       QTAILQ_INIT(&cpu->watchpoints);
>   
>       cpu_exec_initfn(cpu);
> +
> +    /*
> +     * Plugin initialization must wait until the cpu start executing
> +     * code, but we must queue this work before the threads are
> +     * created to ensure we don't race.
> +     */
> +#ifdef CONFIG_PLUGIN
> +    if (tcg_enabled()) {
> +        cpu->plugin_state = qemu_plugin_create_vcpu_state();
> +        async_run_on_cpu(cpu, qemu_plugin_vcpu_init__async, RUN_ON_CPU_NULL);
> +    }
> +#endif
>   }
>   
>   static void cpu_common_finalize(Object *obj)

Could you check it works for all combination?
- user-mode
- system-mode tcg
- system-mode mttcg

When I tried to move this code around, one of them didn't work correctly.

Else,
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 5/5] core/cpu-common: initialise plugin state before thread creation

[Alex Bennée]

Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> On 5/30/24 12:42, Alex Bennée wrote:
>> Originally I tried to move where vCPU thread initialisation to later
>> in realize. However pulling that thread (sic) got gnarly really
>> quickly. It turns out some steps of CPU realization need values that
>> can only be determined from the running vCPU thread.
>> However having moved enough out of the thread creation we can now
>> queue work before the thread starts (at least for TCG guests) and
>> avoid the race between vcpu_init and other vcpu states a plugin might
>> subscribe to.
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   hw/core/cpu-common.c | 20 ++++++++++++--------
>>   1 file changed, 12 insertions(+), 8 deletions(-)
>> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
>> index 6cfc01593a..bf1a7b8892 100644
>> --- a/hw/core/cpu-common.c
>> +++ b/hw/core/cpu-common.c
>> @@ -222,14 +222,6 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>>           cpu_resume(cpu);
>>       }
>>   -    /* Plugin initialization must wait until the cpu start
>> executing code */
>> -#ifdef CONFIG_PLUGIN
>> -    if (tcg_enabled()) {
>> -        cpu->plugin_state = qemu_plugin_create_vcpu_state();
>> -        async_run_on_cpu(cpu, qemu_plugin_vcpu_init__async, RUN_ON_CPU_NULL);
>> -    }
>> -#endif
>> -
>>       /* NOTE: latest generic point where the cpu is fully realized */
>>   }
>>   @@ -273,6 +265,18 @@ static void cpu_common_initfn(Object *obj)
>>       QTAILQ_INIT(&cpu->watchpoints);
>>         cpu_exec_initfn(cpu);
>> +
>> +    /*
>> +     * Plugin initialization must wait until the cpu start executing
>> +     * code, but we must queue this work before the threads are
>> +     * created to ensure we don't race.
>> +     */
>> +#ifdef CONFIG_PLUGIN
>> +    if (tcg_enabled()) {
>> +        cpu->plugin_state = qemu_plugin_create_vcpu_state();
>> +        async_run_on_cpu(cpu, qemu_plugin_vcpu_init__async, RUN_ON_CPU_NULL);
>> +    }
>> +#endif
>>   }
>>     static void cpu_common_finalize(Object *obj)
>
> Could you check it works for all combination?
> - user-mode
> - system-mode tcg
> - system-mode mttcg

I was hand testing against the record replay tests in avocado (rr single
thread tcg) and general system and user mode stuff. I haven't run a full
pipeline yet, although I did apply your IPS patches ontop and run that:

  https://gitlab.com/stsquad/qemu/-/pipelines/1312869352

but all those failures are user-mode build failures due to the missing
time stubs AFAICT.

>
> When I tried to move this code around, one of them didn't work correctly.
>
> Else,
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 5/5] core/cpu-common: initialise plugin state before thread creation

[Philippe Mathieu-Daudé]

On 30/5/24 21:42, Alex Bennée wrote:
> Originally I tried to move where vCPU thread initialisation to later
> in realize. However pulling that thread (sic) got gnarly really
> quickly. It turns out some steps of CPU realization need values that
> can only be determined from the running vCPU thread.

FYI: 
https://lore.kernel.org/qemu-devel/20240528145953.65398-6-philmd@linaro.org/

> However having moved enough out of the thread creation we can now
> queue work before the thread starts (at least for TCG guests) and
> avoid the race between vcpu_init and other vcpu states a plugin might
> subscribe to.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   hw/core/cpu-common.c | 20 ++++++++++++--------
>   1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index 6cfc01593a..bf1a7b8892 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -222,14 +222,6 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>           cpu_resume(cpu);
>       }
>   
> -    /* Plugin initialization must wait until the cpu start executing code */
> -#ifdef CONFIG_PLUGIN
> -    if (tcg_enabled()) {
> -        cpu->plugin_state = qemu_plugin_create_vcpu_state();
> -        async_run_on_cpu(cpu, qemu_plugin_vcpu_init__async, RUN_ON_CPU_NULL);
> -    }
> -#endif
> -
>       /* NOTE: latest generic point where the cpu is fully realized */
>   }
>   
> @@ -273,6 +265,18 @@ static void cpu_common_initfn(Object *obj)
>       QTAILQ_INIT(&cpu->watchpoints);
>   
>       cpu_exec_initfn(cpu);
> +
> +    /*
> +     * Plugin initialization must wait until the cpu start executing
> +     * code, but we must queue this work before the threads are
> +     * created to ensure we don't race.
> +     */
> +#ifdef CONFIG_PLUGIN
> +    if (tcg_enabled()) {
> +        cpu->plugin_state = qemu_plugin_create_vcpu_state();

Per https://etherpad.opendev.org/p/QEMU_vCPU_life, plugin_state could
be initialized in AccelCPUClass::cpu_instance_init (although this
callback is called at CPUClass::instance_post_init which I haven't
yet figured why).

> +        async_run_on_cpu(cpu, qemu_plugin_vcpu_init__async, RUN_ON_CPU_NULL);
> +    }
> +#endif
>   }
>   
>   static void cpu_common_finalize(Object *obj)

Re: [PATCH 5/5] core/cpu-common: initialise plugin state before thread creation

[Alex Bennée]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 30/5/24 21:42, Alex Bennée wrote:
>> Originally I tried to move where vCPU thread initialisation to later
>> in realize. However pulling that thread (sic) got gnarly really
>> quickly. It turns out some steps of CPU realization need values that
>> can only be determined from the running vCPU thread.
>
> FYI:
> https://lore.kernel.org/qemu-devel/20240528145953.65398-6-philmd@linaro.org/

But this still has it in realize which would still race as the threads
are started before we call the common realize functions.

>
>> However having moved enough out of the thread creation we can now
>> queue work before the thread starts (at least for TCG guests) and
>> avoid the race between vcpu_init and other vcpu states a plugin might
>> subscribe to.
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   hw/core/cpu-common.c | 20 ++++++++++++--------
>>   1 file changed, 12 insertions(+), 8 deletions(-)
>> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
>> index 6cfc01593a..bf1a7b8892 100644
>> --- a/hw/core/cpu-common.c
>> +++ b/hw/core/cpu-common.c
>> @@ -222,14 +222,6 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>>           cpu_resume(cpu);
>>       }
>>   -    /* Plugin initialization must wait until the cpu start
>> executing code */
>> -#ifdef CONFIG_PLUGIN
>> -    if (tcg_enabled()) {
>> -        cpu->plugin_state = qemu_plugin_create_vcpu_state();
>> -        async_run_on_cpu(cpu, qemu_plugin_vcpu_init__async, RUN_ON_CPU_NULL);
>> -    }
>> -#endif
>> -
>>       /* NOTE: latest generic point where the cpu is fully realized */
>>   }
>>   @@ -273,6 +265,18 @@ static void cpu_common_initfn(Object *obj)
>>       QTAILQ_INIT(&cpu->watchpoints);
>>         cpu_exec_initfn(cpu);
>> +
>> +    /*
>> +     * Plugin initialization must wait until the cpu start executing
>> +     * code, but we must queue this work before the threads are
>> +     * created to ensure we don't race.
>> +     */
>> +#ifdef CONFIG_PLUGIN
>> +    if (tcg_enabled()) {
>> +        cpu->plugin_state = qemu_plugin_create_vcpu_state();
>
> Per https://etherpad.opendev.org/p/QEMU_vCPU_life, plugin_state could
> be initialized in AccelCPUClass::cpu_instance_init (although this
> callback is called at CPUClass::instance_post_init which I haven't
> yet figured why).

Why are x86 and RiscV special in terms of calling this function?

>
>> +        async_run_on_cpu(cpu, qemu_plugin_vcpu_init__async, RUN_ON_CPU_NULL);
>> +    }
>> +#endif
>>   }
>>     static void cpu_common_finalize(Object *obj)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 5/5] core/cpu-common: initialise plugin state before thread creation

[Philippe Mathieu-Daudé]

On 31/5/24 10:47, Alex Bennée wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> On 30/5/24 21:42, Alex Bennée wrote:
>>> Originally I tried to move where vCPU thread initialisation to later
>>> in realize. However pulling that thread (sic) got gnarly really
>>> quickly. It turns out some steps of CPU realization need values that
>>> can only be determined from the running vCPU thread.
>>
>> FYI:
>> https://lore.kernel.org/qemu-devel/20240528145953.65398-6-philmd@linaro.org/
> 
> But this still has it in realize which would still race as the threads
> are started before we call the common realize functions.
> 
>>
>>> However having moved enough out of the thread creation we can now
>>> queue work before the thread starts (at least for TCG guests) and
>>> avoid the race between vcpu_init and other vcpu states a plugin might
>>> subscribe to.
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>>    hw/core/cpu-common.c | 20 ++++++++++++--------
>>>    1 file changed, 12 insertions(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 0/5] cpus: a few tweaks to CPU realization

[Philippe Mathieu-Daudé]

On 30/5/24 21:42, Alex Bennée wrote:
> The recent IPS plugin exposed a race condition between vcpu_init
> callbacks and the other vcpu state callbacks. I originally thought
> there was some wider re-factoring to be done to clean this up but it
> turns out things are broadly where they should be. However some of the
> stuff allocated in the vCPU threads can clearly be done earlier so
> I've moved enough from cpu_common_realizefn to cpu_common_initfn to
> allow plugins to queue work before the threads start solving the race.
> 
> Please review.
> 
> Alex Bennée (5):
>    hw/core: expand on the alignment of CPUState
>    cpu: move Qemu[Thread|Cond] setup into common code
>    cpu-target: don't set cpu->thread_id to bogus value
>    plugins: remove special casing for cpu->realized

Thanks, patches 1-4 queued so far.

Re: [PATCH 0/5] cpus: a few tweaks to CPU realization

[Philippe Mathieu-Daudé]

On 3/6/24 14:47, Philippe Mathieu-Daudé wrote:
> On 30/5/24 21:42, Alex Bennée wrote:
>> The recent IPS plugin exposed a race condition between vcpu_init
>> callbacks and the other vcpu state callbacks. I originally thought
>> there was some wider re-factoring to be done to clean this up but it
>> turns out things are broadly where they should be. However some of the
>> stuff allocated in the vCPU threads can clearly be done earlier so
>> I've moved enough from cpu_common_realizefn to cpu_common_initfn to
>> allow plugins to queue work before the threads start solving the race.
>>
>> Please review.
>>
>> Alex Bennée (5):
>>    hw/core: expand on the alignment of CPUState
>>    cpu: move Qemu[Thread|Cond] setup into common code
>>    cpu-target: don't set cpu->thread_id to bogus value
>>    plugins: remove special casing for cpu->realized
> 
> Thanks, patches 1-4 queued so far.

Now patch 5 also queued ;)