[PATCH 14/18] cpus: properly kick CPUs out of inner execution loop

Paolo Bonzini posted 18 patches 1 week ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Cameron Esfahani <dirty@apple.com>, Roman Bolshakov <rbolshakov@ddn.com>, Phil Dennis-Jordan <phil@philjordan.eu>, Mads Ynddal <mads@ynddal.dk>, Riku Voipio <riku.voipio@iki.fi>, Warner Losh <imp@bsdimp.com>, Kyle Evans <kevans@freebsd.org>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Yanan Wang <wangyanan55@huawei.com>, Zhao Liu <zhao1.liu@intel.com>, Nicholas Piggin <npiggin@gmail.com>, Chinmay Rath <rathc@linux.ibm.com>, Harsh Prateek Bora <harshpb@linux.ibm.com>, Laurent Vivier <laurent@vivier.eu>, Brian Cain <brian.cain@oss.qualcomm.com>, "Alex Bennée" <alex.bennee@linaro.org>, Peter Maydell <peter.maydell@linaro.org>, Michael Rolnik <mrolnik@gmail.com>, Marcelo Tosatti <mtosatti@redhat.com>, Reinoud Zandijk <reinoud@netbsd.org>, Sunil Muthuswamy <sunilmut@microsoft.com>, Stafford Horne <shorne@gmail.com>, Yoshinori Sato <yoshinori.sato@nifty.com>, David Hildenbrand <david@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, Thomas Huth <thuth@redhat.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Artyom Tarasenko <atar4qemu@gmail.com>
[PATCH 14/18] cpus: properly kick CPUs out of inner execution loop
Posted by Paolo Bonzini 1 week ago
Now that cpu_exit() actually kicks all accelerators, use it whenever
the message to another thread is processed in qemu_wait_io_event().

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-common.c                | 3 ++-
 hw/ppc/ppc.c                | 2 ++
 hw/ppc/spapr_hcall.c        | 7 +++----
 hw/ppc/spapr_rtas.c         | 2 +-
 replay/replay-events.c      | 3 ++-
 system/cpu-timers.c         | 6 +++---
 system/cpus.c               | 5 +++--
 target/arm/tcg/mte_helper.c | 2 +-
 target/i386/kvm/hyperv.c    | 1 -
 9 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/cpu-common.c b/cpu-common.c
index ef5757d23bf..152661df8e9 100644
--- a/cpu-common.c
+++ b/cpu-common.c
@@ -137,7 +137,8 @@ static void queue_work_on_cpu(CPUState *cpu, struct qemu_work_item *wi)
     wi->done = false;
     qemu_mutex_unlock(&cpu->work_mutex);
 
-    qemu_cpu_kick(cpu);
+    /* exit the inner loop and reach qemu_wait_io_event_common().  */
+    cpu_exit(cpu);
 }
 
 void do_run_on_cpu(CPUState *cpu, run_on_cpu_func func, run_on_cpu_data data,
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 43d0d0e7553..3e436c70413 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -190,6 +190,7 @@ static void ppc970_set_irq(void *opaque, int pin, int level)
             if (level) {
                 trace_ppc_irq_cpu("stop");
                 cs->halted = 1;
+                cpu_exit(cs);
             } else {
                 trace_ppc_irq_cpu("restart");
                 cs->halted = 0;
@@ -386,6 +387,7 @@ static void ppc40x_set_irq(void *opaque, int pin, int level)
             if (level) {
                 trace_ppc_irq_cpu("stop");
                 cs->halted = 1;
+                cpu_exit(cs);
             } else {
                 trace_ppc_irq_cpu("restart");
                 cs->halted = 0;
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 51875e32a09..c594d4b916e 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -509,8 +509,8 @@ static target_ulong h_cede(PowerPCCPU *cpu, SpaprMachineState *spapr,
     if (!cpu_has_work(cs)) {
         cs->halted = 1;
         cs->exception_index = EXCP_HLT;
-        qatomic_set(&cs->exit_request, true);
         ppc_maybe_interrupt(env);
+        cpu_exit(cs);
     }
 
     return H_SUCCESS;
@@ -531,8 +531,8 @@ static target_ulong h_confer_self(PowerPCCPU *cpu)
     }
     cs->halted = 1;
     cs->exception_index = EXCP_HALTED;
-    qatomic_set(&cs->exit_request, true);
     ppc_maybe_interrupt(&cpu->env);
+    cpu_exit(cs);
 
     return H_SUCCESS;
 }
@@ -624,8 +624,7 @@ static target_ulong h_confer(PowerPCCPU *cpu, SpaprMachineState *spapr,
     }
 
     cs->exception_index = EXCP_YIELD;
-    qatomic_set(&cs->exit_request, true);
-    cpu_loop_exit(cs);
+    cpu_exit(cs);
 
     return H_SUCCESS;
 }
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 78309dbb09d..143bc8c3794 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -221,7 +221,7 @@ static void rtas_stop_self(PowerPCCPU *cpu, SpaprMachineState *spapr,
     cs->halted = 1;
     ppc_store_lpcr(cpu, env->spr[SPR_LPCR] & ~pcc->lpcr_pm);
     kvmppc_set_reg_ppc_online(cpu, 0);
-    qemu_cpu_kick(cs);
+    cpu_exit(cs);
 }
 
 static void rtas_ibm_suspend_me(PowerPCCPU *cpu, SpaprMachineState *spapr,
diff --git a/replay/replay-events.c b/replay/replay-events.c
index 8959da9f1fa..a96e47e7740 100644
--- a/replay/replay-events.c
+++ b/replay/replay-events.c
@@ -118,7 +118,8 @@ void replay_add_event(ReplayAsyncEventKind event_kind,
 
     g_assert(replay_mutex_locked());
     QTAILQ_INSERT_TAIL(&events_list, event, events);
-    qemu_cpu_kick(first_cpu);
+    /* Kick the TCG thread out of tcg_cpu_exec().  */
+    cpu_exit(first_cpu);
 }
 
 void replay_bh_schedule_event(QEMUBH *bh)
diff --git a/system/cpu-timers.c b/system/cpu-timers.c
index cb35fa62b8a..6a00691b8d5 100644
--- a/system/cpu-timers.c
+++ b/system/cpu-timers.c
@@ -246,14 +246,14 @@ void qemu_timer_notify_cb(void *opaque, QEMUClockType type)
 
     if (qemu_in_vcpu_thread()) {
         /*
-         * A CPU is currently running; kick it back out to the
+         * A CPU is currently running; kick it back out of the
          * tcg_cpu_exec() loop so it will recalculate its
          * icount deadline immediately.
          */
-        qemu_cpu_kick(current_cpu);
+        cpu_exit(current_cpu);
     } else if (first_cpu) {
         /*
-         * qemu_cpu_kick is not enough to kick a halted CPU out of
+         * cpu_exit() is not enough to kick a halted CPU out of
          * qemu_tcg_wait_io_event.  async_run_on_cpu, instead,
          * causes cpu_thread_is_idle to return false.  This way,
          * handle_icount_deadline can run.
diff --git a/system/cpus.c b/system/cpus.c
index 9bfbe2b0607..bb13942cbb7 100644
--- a/system/cpus.c
+++ b/system/cpus.c
@@ -604,7 +604,7 @@ void cpu_pause(CPUState *cpu)
         qemu_cpu_stop(cpu, true);
     } else {
         cpu->stop = true;
-        qemu_cpu_kick(cpu);
+        cpu_exit(cpu);
     }
 }
 
@@ -644,6 +644,7 @@ void pause_all_vcpus(void)
 
     while (!all_vcpus_paused()) {
         qemu_cond_wait(&qemu_pause_cond, &bql);
+        /* FIXME: is this needed? */
         CPU_FOREACH(cpu) {
             qemu_cpu_kick(cpu);
         }
@@ -672,7 +673,7 @@ void cpu_remove_sync(CPUState *cpu)
 {
     cpu->stop = true;
     cpu->unplug = true;
-    qemu_cpu_kick(cpu);
+    cpu_exit(cpu);
     bql_unlock();
     qemu_thread_join(cpu->thread);
     bql_lock();
diff --git a/target/arm/tcg/mte_helper.c b/target/arm/tcg/mte_helper.c
index 0efc18a181e..302e899287c 100644
--- a/target/arm/tcg/mte_helper.c
+++ b/target/arm/tcg/mte_helper.c
@@ -591,7 +591,7 @@ static void mte_async_check_fail(CPUARMState *env, uint64_t dirty_ptr,
      * which is rather sooner than "normal".  But the alternative
      * is waiting until the next syscall.
      */
-    qemu_cpu_kick(env_cpu(env));
+    cpu_exit(env_cpu(env));
 #endif
 }
 
diff --git a/target/i386/kvm/hyperv.c b/target/i386/kvm/hyperv.c
index 9865120cc43..f7a81bd2700 100644
--- a/target/i386/kvm/hyperv.c
+++ b/target/i386/kvm/hyperv.c
@@ -81,7 +81,6 @@ int kvm_hv_handle_exit(X86CPU *cpu, struct kvm_hyperv_exit *exit)
          * necessary because memory hierarchy is being changed
          */
         async_safe_run_on_cpu(CPU(cpu), async_synic_update, RUN_ON_CPU_NULL);
-        cpu_exit(CPU(cpu));
 
         return EXCP_INTERRUPT;
     case KVM_EXIT_HYPERV_HCALL: {
-- 
2.51.0
Re: [PATCH 14/18] cpus: properly kick CPUs out of inner execution loop
Posted by Richard Henderson 1 week ago
On 8/30/25 01:31, Paolo Bonzini wrote:
> @@ -624,8 +624,7 @@ static target_ulong h_confer(PowerPCCPU *cpu, SpaprMachineState *spapr,
>       }
>   
>       cs->exception_index = EXCP_YIELD;
> -    qatomic_set(&cs->exit_request, true);
> -    cpu_loop_exit(cs);
> +    cpu_exit(cs);
>   
>       return H_SUCCESS;
>   }

cpu_loop_exit does a longjmp; cpu_exit does not.

This may be a bug fix, but it's hard to tell.
If it is a bug fix, it should be separated.

I'm also having a hard time with e.g.

> +++ b/system/cpu-timers.c
> @@ -246,14 +246,14 @@ void qemu_timer_notify_cb(void *opaque, QEMUClockType type)
>  
>      if (qemu_in_vcpu_thread()) {
>          /*
> -         * A CPU is currently running; kick it back out to the
> +         * A CPU is currently running; kick it back out of the
>           * tcg_cpu_exec() loop so it will recalculate its
>           * icount deadline immediately.
>           */
> -        qemu_cpu_kick(current_cpu);
> +        cpu_exit(current_cpu);

where the comment still says kick and we're replacing kick with exit.

I guess the root of this problem is that "kick" isn't a precise term, we ought to name it 
something else, and we should paint the bike shed green.


r~
Re: [PATCH 14/18] cpus: properly kick CPUs out of inner execution loop
Posted by Paolo Bonzini 1 week ago
On Fri, Aug 29, 2025 at 11:57 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/30/25 01:31, Paolo Bonzini wrote:
> > @@ -624,8 +624,7 @@ static target_ulong h_confer(PowerPCCPU *cpu, SpaprMachineState *spapr,
> >       }
> >
> >       cs->exception_index = EXCP_YIELD;
> > -    qatomic_set(&cs->exit_request, true);
> > -    cpu_loop_exit(cs);
> > +    cpu_exit(cs);
> >
> >       return H_SUCCESS;
> >   }
>
> cpu_loop_exit does a longjmp; cpu_exit does not.
>
> This may be a bug fix, but it's hard to tell.
> If it is a bug fix, it should be separated.

The longjmp is overkill but works. Not doing the longjmp also works
because gen_sc() finishes the TB and then you go all the way (check
interrupt_request -> check exit_request -> write exception_index ->
cpu_exec_end) out of tcg_cpu_exec().

I like cpu_loop_exit() to signify that I am in the middle of the TB.
That said, I'm always conflicted between renaming badly-named
functions and keeping historical names. qemu_wait_io_event() is also
horrible.

> > +++ b/system/cpu-timers.c
> > @@ -246,14 +246,14 @@ void qemu_timer_notify_cb(void *opaque, QEMUClockType type)
> >
> >      if (qemu_in_vcpu_thread()) {
> >          /*
> > -         * A CPU is currently running; kick it back out to the
> > +         * A CPU is currently running; kick it back out of the
> >           * tcg_cpu_exec() loop so it will recalculate its
> >           * icount deadline immediately.
> >           */
> > -        qemu_cpu_kick(current_cpu);
> > +        cpu_exit(current_cpu);
>
> where the comment still says kick and we're replacing kick with exit.
>
> I guess the root of this problem is that "kick" isn't a precise term, we ought to name it
> something else, and we should paint the bike shed green.

Yes, I agree. "send it out of" can work too in this case, I'll chanbge it.

Paolo
Re: [PATCH 14/18] cpus: properly kick CPUs out of inner execution loop
Posted by Richard Henderson 1 week ago
On 8/30/25 09:05, Paolo Bonzini wrote:
> On Fri, Aug 29, 2025 at 11:57 PM Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 8/30/25 01:31, Paolo Bonzini wrote:
>>> @@ -624,8 +624,7 @@ static target_ulong h_confer(PowerPCCPU *cpu, SpaprMachineState *spapr,
>>>        }
>>>
>>>        cs->exception_index = EXCP_YIELD;
>>> -    qatomic_set(&cs->exit_request, true);
>>> -    cpu_loop_exit(cs);
>>> +    cpu_exit(cs);
>>>
>>>        return H_SUCCESS;
>>>    }
>>
>> cpu_loop_exit does a longjmp; cpu_exit does not.
>>
>> This may be a bug fix, but it's hard to tell.
>> If it is a bug fix, it should be separated.
> 
> The longjmp is overkill but works. Not doing the longjmp also works
> because gen_sc() finishes the TB and then you go all the way (check
> interrupt_request -> check exit_request -> write exception_index ->
> cpu_exec_end) out of tcg_cpu_exec().

Ok.

> 
> I like cpu_loop_exit() to signify that I am in the middle of the TB.

Agreed.  I suspect we over-use longjmp and could perhaps do better at simply returning up 
the call-stack.


> That said, I'm always conflicted between renaming badly-named
> functions and keeping historical names. qemu_wait_io_event() is also
> horrible.

...

> 
>>> +++ b/system/cpu-timers.c
>>> @@ -246,14 +246,14 @@ void qemu_timer_notify_cb(void *opaque, QEMUClockType type)
>>>
>>>       if (qemu_in_vcpu_thread()) {
>>>           /*
>>> -         * A CPU is currently running; kick it back out to the
>>> +         * A CPU is currently running; kick it back out of the
>>>            * tcg_cpu_exec() loop so it will recalculate its
>>>            * icount deadline immediately.
>>>            */
>>> -        qemu_cpu_kick(current_cpu);
>>> +        cpu_exit(current_cpu);
>>
>> where the comment still says kick and we're replacing kick with exit.
>>
>> I guess the root of this problem is that "kick" isn't a precise term, we ought to name it
>> something else, and we should paint the bike shed green.
> 
> Yes, I agree. "send it out of" can work too in this case, I'll chanbge it.

I was actually talking about renaming qemu_cpu_kick.
But per above, that's hard.  :-)

I guess just updating the comment is fine for now.


r~