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

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.

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.
> 

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.

(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?

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~