[PATCH 0/3] target/ppc: Have gen_pause() actually pause vCPUs

[PATCH 0/3] target/ppc: Have gen_pause() actually pause vCPUs

[Philippe Mathieu-Daudé]

Probably due to a type, gen_pause() keeps vCPUs running.
Fix that.

Philippe Mathieu-Daudé (3):
  hw/ppc: Do not open-code cpu_resume() in spin_kick()
  target/ppc: Have gen_pause() actually pause vCPUs
  target/ppc: Re-use gen_pause() in gen_wait()

 hw/ppc/ppce500_spin.c  |  3 +--
 target/ppc/translate.c | 15 ++++++---------
 2 files changed, 7 insertions(+), 11 deletions(-)

-- 
2.51.0

[PATCH 1/3] hw/ppc: Do not open-code cpu_resume() in spin_kick()

[Philippe Mathieu-Daudé]

In order to make the code easier to follow / review,
use the cpu_resume() helper instead of open-coding it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ppc/ppce500_spin.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/ppc/ppce500_spin.c b/hw/ppc/ppce500_spin.c
index 2310f62a91e..bc70e50e926 100644
--- a/hw/ppc/ppce500_spin.c
+++ b/hw/ppc/ppce500_spin.c
@@ -99,8 +99,7 @@ static void spin_kick(CPUState *cs, run_on_cpu_data data)
 
     cs->halted = 0;
     cs->exception_index = -1;
-    cs->stopped = false;
-    qemu_cpu_kick(cs);
+    cpu_resume(cs);
 }
 
 static void spin_write(void *opaque, hwaddr addr, uint64_t value,
-- 
2.51.0

Re: [PATCH 1/3] hw/ppc: Do not open-code cpu_resume() in spin_kick()

[Richard Henderson]

On 9/24/25 10:30, Philippe Mathieu-Daudé wrote:
> In order to make the code easier to follow / review,
> use the cpu_resume() helper instead of open-coding it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/ppc/ppce500_spin.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/ppce500_spin.c b/hw/ppc/ppce500_spin.c
> index 2310f62a91e..bc70e50e926 100644
> --- a/hw/ppc/ppce500_spin.c
> +++ b/hw/ppc/ppce500_spin.c
> @@ -99,8 +99,7 @@ static void spin_kick(CPUState *cs, run_on_cpu_data data)
>   
>       cs->halted = 0;
>       cs->exception_index = -1;
> -    cs->stopped = false;
> -    qemu_cpu_kick(cs);
> +    cpu_resume(cs);
>   }
>   
>   static void spin_write(void *opaque, hwaddr addr, uint64_t value,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 2/3] target/ppc: Have gen_pause() actually pause vCPUs

[Philippe Mathieu-Daudé]

gen_pause() sets CPUState::halted = 0, effectively unhalting
(a.k.a. "running") the cpu. Correct by setting the '1' value
to really halt the cpu.

Fixes: b68e60e6f0d ("ppc: Get out of emulation on SMT "OR" ops")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 27f90c3cc56..a1a97e0fd2e 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -1985,7 +1985,7 @@ static inline void gen_op_arith_subf(DisasContext *ctx, TCGv ret, TCGv arg1,
 #if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
 static void gen_pause(DisasContext *ctx)
 {
-    TCGv_i32 t0 = tcg_constant_i32(0);
+    TCGv_i32 t0 = tcg_constant_i32(1);
     tcg_gen_st_i32(t0, tcg_env,
                    -offsetof(PowerPCCPU, env) + offsetof(CPUState, halted));
 
-- 
2.51.0

Re: [PATCH 2/3] target/ppc: Have gen_pause() actually pause vCPUs

[Richard Henderson]

On 9/24/25 10:30, Philippe Mathieu-Daudé wrote:
> gen_pause() sets CPUState::halted = 0, effectively unhalting
> (a.k.a. "running") the cpu. Correct by setting the '1' value
> to really halt the cpu.
> 
> Fixes: b68e60e6f0d ("ppc: Get out of emulation on SMT "OR" ops")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/ppc/translate.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 27f90c3cc56..a1a97e0fd2e 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -1985,7 +1985,7 @@ static inline void gen_op_arith_subf(DisasContext *ctx, TCGv ret, TCGv arg1,
>   #if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>   static void gen_pause(DisasContext *ctx)
>   {
> -    TCGv_i32 t0 = tcg_constant_i32(0);
> +    TCGv_i32 t0 = tcg_constant_i32(1);
>       tcg_gen_st_i32(t0, tcg_env,
>                      -offsetof(PowerPCCPU, env) + offsetof(CPUState, halted));
>   

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 2/3] target/ppc: Have gen_pause() actually pause vCPUs

[Benjamin Herrenschmidt]

On Wed, 2025-09-24 at 19:30 +0200, Philippe Mathieu-Daudé wrote:
> gen_pause() sets CPUState::halted = 0, effectively unhalting
> (a.k.a. "running") the cpu. Correct by setting the '1' value
> to really halt the cpu.

What will resume it though ? The smt_low() case isn't meant to *halt*
the CPUs permanently. smt_*() levels are about SMT thread priorities.
Using a "pause" that just gets out of TCG (and back in), is a way to 
"yield" to another thread, thus enabling more forward progress when a
thread is spinning on an smt_low() loop. This happens in firmware and
in some spinlock cases.

This isn't about stopping until some external event resumes it.

Cheers,
Ben.

Re: [PATCH 2/3] target/ppc: Have gen_pause() actually pause vCPUs

[Philippe Mathieu-Daudé]

On 29/9/25 06:28, Benjamin Herrenschmidt wrote:
> On Wed, 2025-09-24 at 19:30 +0200, Philippe Mathieu-Daudé wrote:
>> gen_pause() sets CPUState::halted = 0, effectively unhalting
>> (a.k.a. "running") the cpu. Correct by setting the '1' value
>> to really halt the cpu.
> 
> What will resume it though ? The smt_low() case isn't meant to *halt*
> the CPUs permanently. smt_*() levels are about SMT thread priorities.
> Using a "pause" that just gets out of TCG (and back in), is a way to
> "yield" to another thread, thus enabling more forward progress when a

What you describe can be achieved with a helper doing:

   cs->exception_index = EXCP_YIELD;
   cpu_loop_exit(cs);

Is that what you wanted?

> thread is spinning on an smt_low() loop. This happens in firmware and
> in some spinlock cases.
> 
> This isn't about stopping until some external event resumes it.
> 
> Cheers,
> Ben.
> 

Re: [PATCH 2/3] target/ppc: Have gen_pause() actually pause vCPUs

[Benjamin Herrenschmidt]

On Mon, 2025-09-29 at 09:51 +0200, Philippe Mathieu-Daudé wrote:
> > What will resume it though ? The smt_low() case isn't meant to
> > *halt*
> > the CPUs permanently. smt_*() levels are about SMT thread
> > priorities.
> > Using a "pause" that just gets out of TCG (and back in), is a way
> > to
> > "yield" to another thread, thus enabling more forward progress when
> > a
> 
> What you describe can be achieved with a helper doing:
> 
>    cs->exception_index = EXCP_YIELD;
>    cpu_loop_exit(cs);
> 
> Is that what you wanted?

I suppose so... this was many years ago and I don't have much context
anymore, so I don't know why I didn't do it that way back then.

Cheers,
Ben.

Re: [PATCH 2/3] target/ppc: Have gen_pause() actually pause vCPUs

[Philippe Mathieu-Daudé]

(Cc'ing Aaron for commit 9e196938aa1 "target-ppc: gen_pause for
instructions: yield, mdoio, mdoom, miso")

On 30/9/25 00:59, Benjamin Herrenschmidt wrote:
> On Mon, 2025-09-29 at 09:51 +0200, Philippe Mathieu-Daudé wrote:
>>> What will resume it though ? The smt_low() case isn't meant to
>>> *halt*
>>> the CPUs permanently. smt_*() levels are about SMT thread
>>> priorities.
>>> Using a "pause" that just gets out of TCG (and back in), is a way
>>> to
>>> "yield" to another thread, thus enabling more forward progress when
>>> a
>>
>> What you describe can be achieved with a helper doing:
>>
>>     cs->exception_index = EXCP_YIELD;
>>     cpu_loop_exit(cs);
>>
>> Is that what you wanted?
> 
> I suppose so... this was many years ago and I don't have much context
> anymore, so I don't know why I didn't do it that way back then.

I *think* Nick implemented something similar for sPAPR in commit
e8ce0e40ee9 ("spapr: Implement H_CONFER"):

     /*
      * The targeted confer does not do anything special beyond yielding
      * the current vCPU, but even this should be better than nothing.
      * At least for single-threaded tcg, it gives the target a chance to
      * run before we run again. Multi-threaded tcg does not really do
      * anything with EXCP_YIELD yet.
      */

     cs->exception_index = EXCP_YIELD;
     cpu_exit(cs);

Nick, would that make sense to implement smt_low?

[PATCH 3/3] target/ppc: Re-use gen_pause() in gen_wait()

[Philippe Mathieu-Daudé]

Avoid open-coding gen_pause() in gen_wait().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/ppc/translate.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index a1a97e0fd2e..1b5d2521deb 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -1982,17 +1982,18 @@ static inline void gen_op_arith_subf(DisasContext *ctx, TCGv ret, TCGv arg1,
 
 /***                            Integer logical                            ***/
 
-#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
 static void gen_pause(DisasContext *ctx)
 {
     TCGv_i32 t0 = tcg_constant_i32(1);
     tcg_gen_st_i32(t0, tcg_env,
                    -offsetof(PowerPCCPU, env) + offsetof(CPUState, halted));
 
-    /* Stop translation, this gives other CPUs a chance to run */
+    /*
+     * Stop translation, as the CPU is supposed to sleep from now,
+     * giving other CPUs a chance to run
+     */
     gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
 }
-#endif /* defined(TARGET_PPC64) */
 
 /***                             Integer rotate                            ***/
 
@@ -3368,11 +3369,7 @@ static void gen_wait(DisasContext *ctx)
      * to occur.
      */
     if (wc == 0) {
-        TCGv_i32 t0 = tcg_constant_i32(1);
-        tcg_gen_st_i32(t0, tcg_env,
-                       -offsetof(PowerPCCPU, env) + offsetof(CPUState, halted));
-        /* Stop translation, as the CPU is supposed to sleep from now */
-        gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
+        gen_pause(ctx);
     }
 
     /*
-- 
2.51.0

Re: [PATCH 3/3] target/ppc: Re-use gen_pause() in gen_wait()

[Richard Henderson]

On 9/24/25 10:30, Philippe Mathieu-Daudé wrote:
> Avoid open-coding gen_pause() in gen_wait().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/ppc/translate.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 0/3] target/ppc: Have gen_pause() actually pause vCPUs

[Philippe Mathieu-Daudé]

On 24/9/25 19:30, Philippe Mathieu-Daudé wrote:
> Probably due to a type, gen_pause() keeps vCPUs running.
> Fix that.
> 
> Philippe Mathieu-Daudé (3):
>    hw/ppc: Do not open-code cpu_resume() in spin_kick()

Patch #1 queued, thanks.