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 <pbonz...@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~
