Re: [RFC PATCH] cpus: split qemu_init_vcpu and delay vCPU thread creation

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

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.bennee@linaro.org>
> Cc: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Cc: Philippe Mathieu-Daudé <philmd@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.

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

> +        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);