On Fri, 29 Aug 2025 17:31:07 +0200
Paolo Bonzini <[email protected]> wrote:
> CPU threads write exit_request as a "note to self" that they need to
> go out to a slow path. This write happens out of the BQL and can be
> a data race with another threads' cpu_exit(); use atomic accesses
> consistently.
>
> While at it, change the source argument from int ("1") to bool ("true").
>
> Reviewed-by: Richard Henderson <[email protected]>
> Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
> Reviewed-by: Peter Xu <[email protected]>
> Signed-off-by: Paolo Bonzini <[email protected]>
Reviewed-by: Igor Mammedov <[email protected]>
> ---
> include/hw/core/cpu.h | 9 +++++++++
> accel/kvm/kvm-all.c | 2 +-
> accel/tcg/tcg-accel-ops-mttcg.c | 2 +-
> accel/tcg/tcg-accel-ops-rr.c | 4 ++--
> hw/ppc/spapr_hcall.c | 6 +++---
> target/i386/kvm/kvm.c | 6 +++---
> target/i386/nvmm/nvmm-accel-ops.c | 2 +-
> target/i386/nvmm/nvmm-all.c | 2 +-
> target/i386/whpx/whpx-all.c | 6 +++---
> 9 files changed, 24 insertions(+), 15 deletions(-)
>
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 8b57bcd92c9..338757e5254 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -422,6 +422,15 @@ struct qemu_work_item;
> * valid under cpu_list_lock.
> * @created: Indicates whether the CPU thread has been successfully created.
> * @halt_cond: condition variable sleeping threads can wait on.
> + * @exit_request: Another thread requests the CPU to call
> qemu_wait_io_event().
> + * Should be read only by CPU thread with load-acquire, to synchronize with
> + * other threads' store-release operation.
> + *
> + * In some cases, accelerator-specific code will write exit_request from
> + * within the same thread, to "bump" the effect of qemu_cpu_kick() to
> + * the one provided by cpu_exit(), especially when processing interrupt
> + * flags. In this case, the write and read happen in the same thread
> + * and the write therefore can use qemu_atomic_set().
> * @interrupt_request: Indicates a pending interrupt request.
> * Only used by system emulation.
> * @halted: Nonzero if the CPU is in suspended state.
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index bd9e5e3886d..e4167d94b4f 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3730,7 +3730,7 @@ int kvm_on_sigbus_vcpu(CPUState *cpu, int code, void
> *addr)
> have_sigbus_pending = true;
> pending_sigbus_addr = addr;
> pending_sigbus_code = code;
> - qatomic_set(&cpu->exit_request, 1);
> + qatomic_set(&cpu->exit_request, true);
> return 0;
> #else
> return 1;
> diff --git a/accel/tcg/tcg-accel-ops-mttcg.c b/accel/tcg/tcg-accel-ops-mttcg.c
> index 337b993d3da..b12b7a36b5d 100644
> --- a/accel/tcg/tcg-accel-ops-mttcg.c
> +++ b/accel/tcg/tcg-accel-ops-mttcg.c
> @@ -85,7 +85,7 @@ static void *mttcg_cpu_thread_fn(void *arg)
> qemu_guest_random_seed_thread_part2(cpu->random_seed);
>
> /* process any pending work */
> - cpu->exit_request = 1;
> + qatomic_set(&cpu->exit_request, true);
>
> do {
> if (cpu_can_run(cpu)) {
> diff --git a/accel/tcg/tcg-accel-ops-rr.c b/accel/tcg/tcg-accel-ops-rr.c
> index 1e551e92d6d..c2468d15d4f 100644
> --- a/accel/tcg/tcg-accel-ops-rr.c
> +++ b/accel/tcg/tcg-accel-ops-rr.c
> @@ -212,7 +212,7 @@ static void *rr_cpu_thread_fn(void *arg)
> cpu = first_cpu;
>
> /* process any pending work */
> - cpu->exit_request = 1;
> + qatomic_set(&cpu->exit_request, true);
>
> while (1) {
> /* Only used for icount_enabled() */
> @@ -286,7 +286,7 @@ static void *rr_cpu_thread_fn(void *arg)
> /* Does not need a memory barrier because a spurious wakeup is okay.
> */
> qatomic_set(&rr_current_cpu, NULL);
>
> - if (cpu && cpu->exit_request) {
> + if (cpu && qatomic_read(&cpu->exit_request)) {
> qatomic_set_mb(&cpu->exit_request, 0);
> }
>
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 1e936f35e44..51875e32a09 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -509,7 +509,7 @@ static target_ulong h_cede(PowerPCCPU *cpu,
> SpaprMachineState *spapr,
> if (!cpu_has_work(cs)) {
> cs->halted = 1;
> cs->exception_index = EXCP_HLT;
> - cs->exit_request = 1;
> + qatomic_set(&cs->exit_request, true);
> ppc_maybe_interrupt(env);
> }
>
> @@ -531,7 +531,7 @@ static target_ulong h_confer_self(PowerPCCPU *cpu)
> }
> cs->halted = 1;
> cs->exception_index = EXCP_HALTED;
> - cs->exit_request = 1;
> + qatomic_set(&cs->exit_request, true);
> ppc_maybe_interrupt(&cpu->env);
>
> return H_SUCCESS;
> @@ -624,7 +624,7 @@ static target_ulong h_confer(PowerPCCPU *cpu,
> SpaprMachineState *spapr,
> }
>
> cs->exception_index = EXCP_YIELD;
> - cs->exit_request = 1;
> + qatomic_set(&cs->exit_request, true);
> cpu_loop_exit(cs);
>
> return H_SUCCESS;
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 8420c4090ef..34e74f24470 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -5486,10 +5486,10 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run
> *run)
> if (cpu_test_interrupt(cpu, CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR)) {
> if (cpu_test_interrupt(cpu, CPU_INTERRUPT_INIT) &&
> !(env->hflags & HF_SMM_MASK)) {
> - qatomic_set(&cpu->exit_request, 1);
> + qatomic_set(&cpu->exit_request, true);
> }
> if (cpu_test_interrupt(cpu, CPU_INTERRUPT_TPR)) {
> - qatomic_set(&cpu->exit_request, 1);
> + qatomic_set(&cpu->exit_request, true);
> }
> }
>
> @@ -5604,7 +5604,7 @@ int kvm_arch_process_async_events(CPUState *cs)
> if (env->exception_nr == EXCP08_DBLE) {
> /* this means triple fault */
> qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> - cs->exit_request = 1;
> + qatomic_set(&cs->exit_request, true);
> return 0;
> }
> kvm_queue_exception(env, EXCP12_MCHK, 0, 0);
> diff --git a/target/i386/nvmm/nvmm-accel-ops.c
> b/target/i386/nvmm/nvmm-accel-ops.c
> index 3799260bbde..86869f133e9 100644
> --- a/target/i386/nvmm/nvmm-accel-ops.c
> +++ b/target/i386/nvmm/nvmm-accel-ops.c
> @@ -77,7 +77,7 @@ static void nvmm_start_vcpu_thread(CPUState *cpu)
> */
> static void nvmm_kick_vcpu_thread(CPUState *cpu)
> {
> - cpu->exit_request = 1;
> + qatomic_set(&cpu->exit_request, true);
> cpus_kick_thread(cpu);
> }
>
> diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
> index 10bd51d9b59..7e36c42fbb4 100644
> --- a/target/i386/nvmm/nvmm-all.c
> +++ b/target/i386/nvmm/nvmm-all.c
> @@ -414,7 +414,7 @@ nvmm_vcpu_pre_run(CPUState *cpu)
> * or commit pending TPR access.
> */
> if (cpu_test_interrupt(cpu, CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR)) {
> - cpu->exit_request = 1;
> + qatomic_set(&cpu->exit_request, true);
> }
>
> if (!has_event && cpu_test_interrupt(cpu, CPU_INTERRUPT_NMI)) {
> diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
> index 2106c29c3a0..00fb7e23100 100644
> --- a/target/i386/whpx/whpx-all.c
> +++ b/target/i386/whpx/whpx-all.c
> @@ -1489,10 +1489,10 @@ static void whpx_vcpu_pre_run(CPUState *cpu)
> if (cpu_test_interrupt(cpu, CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR)) {
> if (cpu_test_interrupt(cpu, CPU_INTERRUPT_INIT) &&
> !(env->hflags & HF_SMM_MASK)) {
> - cpu->exit_request = 1;
> + qatomic_set(&cpu->exit_request, true);
> }
> if (cpu_test_interrupt(cpu, CPU_INTERRUPT_TPR)) {
> - cpu->exit_request = 1;
> + qatomic_set(&cpu->exit_request, true);
> }
> }
>
> @@ -1539,7 +1539,7 @@ static void whpx_vcpu_pre_run(CPUState *cpu)
> if (tpr != vcpu->tpr) {
> vcpu->tpr = tpr;
> reg_values[reg_count].Reg64 = tpr;
> - cpu->exit_request = 1;
> + qatomic_set(&cpu->exit_request, true);
> reg_names[reg_count] = WHvX64RegisterCr8;
> reg_count += 1;
> }
