Re: [PATCH 09/18] accel: use store_release/load_acquire for cross-thread exit_request

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 29 Aug 2025 17:31:06 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Reads and writes cpu->exit_request do not use a load-acquire/store-release
> pair right now, but this means that cpu_exit() may not write cpu->exit_request
> after any flags that are read by the vCPU thread.
> 
> Probably everything is protected one way or the other by the BQL, because
> cpu->exit_request leads to the slow path, where the CPU thread often takes
> the BQL (for example, to go to sleep by waiting on the BQL-protected
> cpu->halt_cond); but it's not clear, so use load-acquire/store-release
> consistently.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

nit below:
> ---
>  accel/kvm/kvm-all.c          | 19 +++++++++----------
>  accel/tcg/cpu-exec.c         |  7 +++++--
>  accel/tcg/tcg-accel-ops-rr.c |  2 +-
>  hw/core/cpu-common.c         |  3 ++-
>  target/i386/nvmm/nvmm-all.c  |  5 ++---
>  target/i386/whpx/whpx-all.c  |  3 ++-
>  6 files changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index f36dfe33492..bd9e5e3886d 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3029,10 +3029,6 @@ static void kvm_eat_signals(CPUState *cpu)
>  
>      if (kvm_immediate_exit) {
>          qatomic_set(&cpu->kvm_run->immediate_exit, 0);
> -        /* Write kvm_run->immediate_exit before the cpu->exit_request
> -         * write in kvm_cpu_exec.
> -         */
> -        smp_wmb();
>          return;
>      }
>  
> @@ -3187,7 +3183,8 @@ int kvm_cpu_exec(CPUState *cpu)
>          }
>  
>          kvm_arch_pre_run(cpu, run);
> -        if (qatomic_read(&cpu->exit_request)) {
> +        /* Corresponding store-release is in cpu_exit. */
> +        if (qatomic_load_acquire(&cpu->exit_request)) {
>              trace_kvm_interrupt_exit_request();
>              /*
>               * KVM requires us to reenter the kernel after IO exits to complete
> @@ -3197,13 +3194,15 @@ int kvm_cpu_exec(CPUState *cpu)
>              kvm_cpu_kick_self();
>          }
>  
> -        /* Read cpu->exit_request before KVM_RUN reads run->immediate_exit.
> -         * Matching barrier in kvm_eat_signals.
> -         */
> -        smp_rmb();
> -
>          run_ret = kvm_vcpu_ioctl(cpu, KVM_RUN, 0);
>  
> +        /*
> +         * After writing cpu->exit_request, cpu_exit() sends a signal that writes
> +         * kvm->run->immediate_exit.  The signal is already happening after the
> +         * write to cpu->exit_request so, if KVM read kvm->run->immediate_exit
> +         * as true, cpu->exit_request will always read as true.
> +         */
> +
>          attrs = kvm_arch_post_run(cpu, run);
>  
>  #ifdef KVM_HAVE_MCE_INJECTION
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 508d2d2d9e2..f838535d111 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -851,8 +851,11 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>      }
>  #endif /* !CONFIG_USER_ONLY */
>  
> -    /* Finally, check if we need to exit to the main loop.  */
> -    if (unlikely(qatomic_read(&cpu->exit_request)) || icount_exit_request(cpu)) {
> +    /*
> +     * Finally, check if we need to exit to the main loop.
> +     * The corresponding store-release is in cpu_exit.
> +     */
> +    if (unlikely(qatomic_load_acquire(&cpu->exit_request)) || icount_exit_request(cpu)) {
>          qatomic_set(&cpu->exit_request, 0);
>          if (cpu->exception_index == -1) {
>              cpu->exception_index = EXCP_INTERRUPT;
> diff --git a/accel/tcg/tcg-accel-ops-rr.c b/accel/tcg/tcg-accel-ops-rr.c
> index 6eec5c9eee9..1e551e92d6d 100644
> --- a/accel/tcg/tcg-accel-ops-rr.c
> +++ b/accel/tcg/tcg-accel-ops-rr.c
> @@ -242,7 +242,7 @@ static void *rr_cpu_thread_fn(void *arg)
>              cpu = first_cpu;
>          }
>  

should we have here a similar comment as in the previous hunk?

> -        while (cpu && cpu_work_list_empty(cpu) && !cpu->exit_request) {
> +        while (cpu && cpu_work_list_empty(cpu) && !qatomic_load_acquire(&cpu->exit_request)) {
>              /* Store rr_current_cpu before evaluating cpu_can_run().  */
>              qatomic_set_mb(&rr_current_cpu, cpu);
>  
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index 9ea1f3764a8..ca00accd162 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -74,7 +74,8 @@ void cpu_reset_interrupt(CPUState *cpu, int mask)
>  
>  void cpu_exit(CPUState *cpu)
>  {
> -    qatomic_set(&cpu->exit_request, 1);
> +    /* Ensure cpu_exec will see the reason why the exit request was set.  */
> +    qatomic_store_release(&cpu->exit_request, true);
>      /* Ensure cpu_exec will see the exit request after TCG has exited.  */
>      smp_wmb();
>      qatomic_set(&cpu->neg.icount_decr.u16.high, -1);
> diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
> index e1151b04c6e..10bd51d9b59 100644
> --- a/target/i386/nvmm/nvmm-all.c
> +++ b/target/i386/nvmm/nvmm-all.c
> @@ -743,7 +743,8 @@ nvmm_vcpu_loop(CPUState *cpu)
>  
>          nvmm_vcpu_pre_run(cpu);
>  
> -        if (qatomic_read(&cpu->exit_request)) {
> +        /* Corresponding store-release is in cpu_exit. */
> +        if (qatomic_load_acquire(&cpu->exit_request)) {
>  #if NVMM_USER_VERSION >= 2
>              nvmm_vcpu_stop(vcpu);
>  #else
> @@ -751,8 +752,6 @@ nvmm_vcpu_loop(CPUState *cpu)
>  #endif
>          }
>  
> -        /* Read exit_request before the kernel reads the immediate exit flag */
> -        smp_rmb();
>          ret = nvmm_vcpu_run(mach, vcpu);
>          if (ret == -1) {
>              error_report("NVMM: Failed to exec a virtual processor,"
> diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
> index c09a0a64f22..2106c29c3a0 100644
> --- a/target/i386/whpx/whpx-all.c
> +++ b/target/i386/whpx/whpx-all.c
> @@ -1714,7 +1714,8 @@ static int whpx_vcpu_run(CPUState *cpu)
>          if (exclusive_step_mode == WHPX_STEP_NONE) {
>              whpx_vcpu_pre_run(cpu);
>  
> -            if (qatomic_read(&cpu->exit_request)) {
> +            /* Corresponding store-release is in cpu_exit. */
> +            if (qatomic_load_acquire(&cpu->exit_request)) {
>                  whpx_vcpu_kick(cpu);
>              }
>          }