Re: [PATCH 8/8] accel: make all calls to qemu_wait_io_event look the same

[Richard Henderson <richard.henderson@linaro.org>]

On 8/9/25 04:59, Paolo Bonzini wrote:
> There is no reason for some accelerators to use qemu_wait_io_event_common
> (which is specifically separated for round robin).  They can also check
> on the first pass through the loop as well directly, without setting
> cpu->exit_request for no particular reason.
> 
> There is also no need to use qatomic_set_mb() because the ordering of
> the communication between I/O and vCPU threads is always the same.
> In the I/O thread:
> 
> (a) store other memory locations that will be checked if cpu->exit_request
>      or cpu->interrupt_request is 1 (for example cpu->stop or cpu->work_list
>      for cpu->exit_request)
> 
> (b) cpu_exit(): store-release cpu->exit_request, or
> (b) cpu_interrupt(): store-release cpu->interrupt_request
> 
>>>> at this point, cpu->halt_cond is broadcast and the BQL released
> 
> (c) do the accelerator-specific kick (e.g. write icount_decr for TCG,
>      pthread_kill for KVM, etc.)
> 
> In the vCPU thread instead the opposite order is respected:
> 
> (c) the accelerator's execution loop exits thanks to the kick
> 
> (b) then the inner execution loop checks cpu->interrupt_request
>      and cpu->exit_request.  If needed cpu->interrupt_request is
>      converted into cpu->exit_request when work is needed outside
>      the execution loop.
> 
> (a) then the other memory locations are checked.  Some may need
>      to be read under the BQL, and the vCPU thread may also take
>      for the vCPU thread can sleep on cpu->halt_cond; but in
>      principle this step is correct even without the BQL.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   accel/dummy-cpus.c                |  2 +-
>   accel/hvf/hvf-accel-ops.c         |  2 +-
>   accel/kvm/kvm-accel-ops.c         |  3 ++-
>   accel/kvm/kvm-all.c               |  2 --
>   accel/tcg/cpu-exec.c              |  1 -
>   accel/tcg/tcg-accel-ops-mttcg.c   |  7 ++----
>   accel/tcg/tcg-accel-ops-rr.c      | 38 ++++++++++++++++---------------
>   accel/tcg/tcg-accel-ops.c         |  2 --
>   system/cpus.c                     |  1 +
>   target/i386/nvmm/nvmm-accel-ops.c |  6 ++---
>   target/i386/nvmm/nvmm-all.c       |  2 --
>   target/i386/whpx/whpx-accel-ops.c |  6 ++---
>   target/i386/whpx/whpx-all.c       |  2 --
>   13 files changed, 31 insertions(+), 43 deletions(-)

I think this is doing two separate things: rearranging the qemu_wait_io_event, and ...


> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 57e35960125..db95e06e768 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3155,7 +3155,6 @@ int kvm_cpu_exec(CPUState *cpu)
>       trace_kvm_cpu_exec();
>   
>       if (kvm_arch_process_async_events(cpu)) {
> -        qatomic_set(&cpu->exit_request, 0);
>           return EXCP_HLT;
>       }
>   
> @@ -3345,7 +3344,6 @@ int kvm_cpu_exec(CPUState *cpu)
>           vm_stop(RUN_STATE_INTERNAL_ERROR);
>       }
>   
> -    qatomic_set(&cpu->exit_request, 0);
>       return ret;
>   }
>   
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index b9da2e3770e..f474ccb37f5 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -871,7 +871,6 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>        * 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.c b/accel/tcg/tcg-accel-ops.c
> index f4d5372866a..ad3f29107e1 100644
> --- a/accel/tcg/tcg-accel-ops.c
> +++ b/accel/tcg/tcg-accel-ops.c
> @@ -82,8 +82,6 @@ int tcg_cpu_exec(CPUState *cpu)
>       ret = cpu_exec(cpu);
>       cpu_exec_end(cpu);
>   
> -    qatomic_set_mb(&cpu->exit_request, 0);
> -
>       return ret;
>   }
>   
> diff --git a/system/cpus.c b/system/cpus.c
> index d2cfa2a9c4e..0cc14eae6a0 100644
> --- a/system/cpus.c
> +++ b/system/cpus.c
> @@ -458,6 +458,7 @@ void qemu_wait_io_event(CPUState *cpu)
>   {
>       bool slept = false;
>   
> +    qatomic_set(&cpu->exit_request, false);
>       while (cpu_thread_is_idle(cpu)) {
>           if (!slept) {
>               slept = true;
> diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
> index d2d90f38976..09839d45b92 100644
> --- a/target/i386/nvmm/nvmm-all.c
> +++ b/target/i386/nvmm/nvmm-all.c
> @@ -817,8 +817,6 @@ nvmm_vcpu_loop(CPUState *cpu)
>       cpu_exec_end(cpu);
>       bql_lock();
>   
> -    qatomic_set(&cpu->exit_request, false);
> -
>       return ret < 0;
>   }
>   
> diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
> index 9b07716121a..2e248a0a6d5 100644
> --- a/target/i386/whpx/whpx-all.c
> +++ b/target/i386/whpx/whpx-all.c
> @@ -2050,8 +2050,6 @@ static int whpx_vcpu_run(CPUState *cpu)
>           whpx_last_vcpu_stopping(cpu);
>       }
>   
> -    qatomic_set(&cpu->exit_request, false);
> -
>       return ret < 0;
>   }
>   

... sinking the clear of exit_request.

It would be nice to actually unify the run loop that you're manipulating here.  But I 
suppose that can be a follow-up.


r~