diff mbox series

[08/19] target/ppc: Fix nip on power management instructions

Message ID 20190128094625.4428-9-clg@kaod.org (mailing list archive)
State New, archived
Headers show
Series ppc: support for the baremetal XIVE interrupt controller (POWER9) | expand

Commit Message

Cédric Le Goater Jan. 28, 2019, 9:46 a.m. UTC 
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Those instructions currently raise an exception from within
the helper. This tends to result in a bogus nip value in
the env context (typically the beginning of the TB). Such
a helper needs a gen_update_nip() first.

This fixes it with a different approach which is to throw
the exception from translate.c instead of the helper using
gen_exception_nip() which does the right thing.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 target/ppc/excp_helper.c |  1 -
 target/ppc/translate.c   | 12 ++++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

Comments

David Gibson Feb. 12, 2019, 6:02 a.m. UTC | #1 
On Mon, Jan 28, 2019 at 10:46:14AM +0100, Cédric Le Goater wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> Those instructions currently raise an exception from within
> the helper. This tends to result in a bogus nip value in
> the env context (typically the beginning of the TB). Such
> a helper needs a gen_update_nip() first.
> 
> This fixes it with a different approach which is to throw
> the exception from translate.c instead of the helper using
> gen_exception_nip() which does the right thing.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  target/ppc/excp_helper.c |  1 -
>  target/ppc/translate.c   | 12 ++++++++----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 751d759fcc1d..8407e0ade938 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -958,7 +958,6 @@ void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
>       * but this doesn't seem to be a problem.
>       */
>      env->msr |= (1ull << MSR_EE);
> -    raise_exception(env, EXCP_HLT);
>  }
>  #endif /* defined(TARGET_PPC64) */
>  
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 7d40a1fbe6bd..55281a8975e0 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -3571,7 +3571,8 @@ static void gen_doze(DisasContext *ctx)
>      t = tcg_const_i32(PPC_PM_DOZE);
>      gen_helper_pminsn(cpu_env, t);
>      tcg_temp_free_i32(t);
> -    gen_stop_exception(ctx);
> +    /* Stop translation, as the CPU is supposed to sleep from now */
> +    gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);

IIUC this also changes from EXCP_STOP to EXCP_HLT, is that intentional?

>  #endif /* defined(CONFIG_USER_ONLY) */
>  }
>  
> @@ -3586,7 +3587,8 @@ static void gen_nap(DisasContext *ctx)
>      t = tcg_const_i32(PPC_PM_NAP);
>      gen_helper_pminsn(cpu_env, t);
>      tcg_temp_free_i32(t);
> -    gen_stop_exception(ctx);
> +    /* Stop translation, as the CPU is supposed to sleep from now */
> +    gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
>  #endif /* defined(CONFIG_USER_ONLY) */
>  }
>  
> @@ -3606,7 +3608,8 @@ static void gen_sleep(DisasContext *ctx)
>      t = tcg_const_i32(PPC_PM_SLEEP);
>      gen_helper_pminsn(cpu_env, t);
>      tcg_temp_free_i32(t);
> -    gen_stop_exception(ctx);
> +    /* Stop translation, as the CPU is supposed to sleep from now */
> +    gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
>  #endif /* defined(CONFIG_USER_ONLY) */
>  }
>  
> @@ -3621,7 +3624,8 @@ static void gen_rvwinkle(DisasContext *ctx)
>      t = tcg_const_i32(PPC_PM_RVWINKLE);
>      gen_helper_pminsn(cpu_env, t);
>      tcg_temp_free_i32(t);
> -    gen_stop_exception(ctx);
> +    /* Stop translation, as the CPU is supposed to sleep from now */
> +    gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
>  #endif /* defined(CONFIG_USER_ONLY) */
>  }
>  #endif /* #if defined(TARGET_PPC64) */
Benjamin Herrenschmidt Feb. 13, 2019, 12:04 a.m. UTC | #2 
On Tue, 2019-02-12 at 17:02 +1100, David Gibson wrote:
> On Mon, Jan 28, 2019 at 10:46:14AM +0100, Cédric Le Goater wrote:
> > From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > 
> > Those instructions currently raise an exception from within
> > the helper. This tends to result in a bogus nip value in
> > the env context (typically the beginning of the TB). Such
> > a helper needs a gen_update_nip() first.
> > 
> > This fixes it with a different approach which is to throw
> > the exception from translate.c instead of the helper using
> > gen_exception_nip() which does the right thing.
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> >  target/ppc/excp_helper.c |  1 -
> >  target/ppc/translate.c   | 12 ++++++++----
> >  2 files changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > index 751d759fcc1d..8407e0ade938 100644
> > --- a/target/ppc/excp_helper.c
> > +++ b/target/ppc/excp_helper.c
> > @@ -958,7 +958,6 @@ void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
> >       * but this doesn't seem to be a problem.
> >       */
> >      env->msr |= (1ull << MSR_EE);
> > -    raise_exception(env, EXCP_HLT);
> >  }
> >  #endif /* defined(TARGET_PPC64) */
> >  
> > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > index 7d40a1fbe6bd..55281a8975e0 100644
> > --- a/target/ppc/translate.c
> > +++ b/target/ppc/translate.c
> > @@ -3571,7 +3571,8 @@ static void gen_doze(DisasContext *ctx)
> >      t = tcg_const_i32(PPC_PM_DOZE);
> >      gen_helper_pminsn(cpu_env, t);
> >      tcg_temp_free_i32(t);
> > -    gen_stop_exception(ctx);
> > +    /* Stop translation, as the CPU is supposed to sleep from now */
> > +    gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
> 
> IIUC this also changes from EXCP_STOP to EXCP_HLT, is that intentional?

Off the top of my head, it might be to break out of the outer exec
loop, but I don't remember off hand.

> >  #endif /* defined(CONFIG_USER_ONLY) */
> >  }
> >  
> > @@ -3586,7 +3587,8 @@ static void gen_nap(DisasContext *ctx)
> >      t = tcg_const_i32(PPC_PM_NAP);
> >      gen_helper_pminsn(cpu_env, t);
> >      tcg_temp_free_i32(t);
> > -    gen_stop_exception(ctx);
> > +    /* Stop translation, as the CPU is supposed to sleep from now */
> > +    gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
> >  #endif /* defined(CONFIG_USER_ONLY) */
> >  }
> >  
> > @@ -3606,7 +3608,8 @@ static void gen_sleep(DisasContext *ctx)
> >      t = tcg_const_i32(PPC_PM_SLEEP);
> >      gen_helper_pminsn(cpu_env, t);
> >      tcg_temp_free_i32(t);
> > -    gen_stop_exception(ctx);
> > +    /* Stop translation, as the CPU is supposed to sleep from now */
> > +    gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
> >  #endif /* defined(CONFIG_USER_ONLY) */
> >  }
> >  
> > @@ -3621,7 +3624,8 @@ static void gen_rvwinkle(DisasContext *ctx)
> >      t = tcg_const_i32(PPC_PM_RVWINKLE);
> >      gen_helper_pminsn(cpu_env, t);
> >      tcg_temp_free_i32(t);
> > -    gen_stop_exception(ctx);
> > +    /* Stop translation, as the CPU is supposed to sleep from now */
> > +    gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
> >  #endif /* defined(CONFIG_USER_ONLY) */
> >  }
> >  #endif /* #if defined(TARGET_PPC64) */
Cédric Le Goater Feb. 15, 2019, 3:30 p.m. UTC | #3 
On 2/13/19 1:04 AM, Benjamin Herrenschmidt wrote:
> On Tue, 2019-02-12 at 17:02 +1100, David Gibson wrote:
>> On Mon, Jan 28, 2019 at 10:46:14AM +0100, Cédric Le Goater wrote:
>>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>
>>> Those instructions currently raise an exception from within
>>> the helper. This tends to result in a bogus nip value in
>>> the env context (typically the beginning of the TB). Such
>>> a helper needs a gen_update_nip() first.
>>>
>>> This fixes it with a different approach which is to throw
>>> the exception from translate.c instead of the helper using
>>> gen_exception_nip() which does the right thing.
>>>
>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>  target/ppc/excp_helper.c |  1 -
>>>  target/ppc/translate.c   | 12 ++++++++----
>>>  2 files changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>>> index 751d759fcc1d..8407e0ade938 100644
>>> --- a/target/ppc/excp_helper.c
>>> +++ b/target/ppc/excp_helper.c
>>> @@ -958,7 +958,6 @@ void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
>>>       * but this doesn't seem to be a problem.
>>>       */
>>>      env->msr |= (1ull << MSR_EE);
>>> -    raise_exception(env, EXCP_HLT);
>>>  }
>>>  #endif /* defined(TARGET_PPC64) */
>>>  
>>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>>> index 7d40a1fbe6bd..55281a8975e0 100644
>>> --- a/target/ppc/translate.c
>>> +++ b/target/ppc/translate.c
>>> @@ -3571,7 +3571,8 @@ static void gen_doze(DisasContext *ctx)
>>>      t = tcg_const_i32(PPC_PM_DOZE);
>>>      gen_helper_pminsn(cpu_env, t);
>>>      tcg_temp_free_i32(t);
>>> -    gen_stop_exception(ctx);
>>> +    /* Stop translation, as the CPU is supposed to sleep from now */
>>> +    gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
>>
>> IIUC this also changes from EXCP_STOP to EXCP_HLT, is that intentional?
> 
> Off the top of my head, it might be to break out of the outer exec
> loop, but I don't remember off hand.

yes and POWERPC_EXCP_STOP is only used under linux-user, which is the 
wrong value to exit from the cpu exec loop.

I think this is correct.

C. 
 

>>>  #endif /* defined(CONFIG_USER_ONLY) */
>>>  }
>>>  
>>> @@ -3586,7 +3587,8 @@ static void gen_nap(DisasContext *ctx)
>>>      t = tcg_const_i32(PPC_PM_NAP);
>>>      gen_helper_pminsn(cpu_env, t);
>>>      tcg_temp_free_i32(t);
>>> -    gen_stop_exception(ctx);
>>> +    /* Stop translation, as the CPU is supposed to sleep from now */
>>> +    gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
>>>  #endif /* defined(CONFIG_USER_ONLY) */
>>>  }
>>>  
>>> @@ -3606,7 +3608,8 @@ static void gen_sleep(DisasContext *ctx)
>>>      t = tcg_const_i32(PPC_PM_SLEEP);
>>>      gen_helper_pminsn(cpu_env, t);
>>>      tcg_temp_free_i32(t);
>>> -    gen_stop_exception(ctx);
>>> +    /* Stop translation, as the CPU is supposed to sleep from now */
>>> +    gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
>>>  #endif /* defined(CONFIG_USER_ONLY) */
>>>  }
>>>  
>>> @@ -3621,7 +3624,8 @@ static void gen_rvwinkle(DisasContext *ctx)
>>>      t = tcg_const_i32(PPC_PM_RVWINKLE);
>>>      gen_helper_pminsn(cpu_env, t);
>>>      tcg_temp_free_i32(t);
>>> -    gen_stop_exception(ctx);
>>> +    /* Stop translation, as the CPU is supposed to sleep from now */
>>> +    gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
>>>  #endif /* defined(CONFIG_USER_ONLY) */
>>>  }
>>>  #endif /* #if defined(TARGET_PPC64) */
>
diff mbox series

Patch

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 751d759fcc1d..8407e0ade938 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -958,7 +958,6 @@  void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
      * but this doesn't seem to be a problem.
      */
     env->msr |= (1ull << MSR_EE);
-    raise_exception(env, EXCP_HLT);
 }
 #endif /* defined(TARGET_PPC64) */
 
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 7d40a1fbe6bd..55281a8975e0 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -3571,7 +3571,8 @@  static void gen_doze(DisasContext *ctx)
     t = tcg_const_i32(PPC_PM_DOZE);
     gen_helper_pminsn(cpu_env, t);
     tcg_temp_free_i32(t);
-    gen_stop_exception(ctx);
+    /* Stop translation, as the CPU is supposed to sleep from now */
+    gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
 #endif /* defined(CONFIG_USER_ONLY) */
 }
 
@@ -3586,7 +3587,8 @@  static void gen_nap(DisasContext *ctx)
     t = tcg_const_i32(PPC_PM_NAP);
     gen_helper_pminsn(cpu_env, t);
     tcg_temp_free_i32(t);
-    gen_stop_exception(ctx);
+    /* Stop translation, as the CPU is supposed to sleep from now */
+    gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
 #endif /* defined(CONFIG_USER_ONLY) */
 }
 
@@ -3606,7 +3608,8 @@  static void gen_sleep(DisasContext *ctx)
     t = tcg_const_i32(PPC_PM_SLEEP);
     gen_helper_pminsn(cpu_env, t);
     tcg_temp_free_i32(t);
-    gen_stop_exception(ctx);
+    /* Stop translation, as the CPU is supposed to sleep from now */
+    gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
 #endif /* defined(CONFIG_USER_ONLY) */
 }
 
@@ -3621,7 +3624,8 @@  static void gen_rvwinkle(DisasContext *ctx)
     t = tcg_const_i32(PPC_PM_RVWINKLE);
     gen_helper_pminsn(cpu_env, t);
     tcg_temp_free_i32(t);
-    gen_stop_exception(ctx);
+    /* Stop translation, as the CPU is supposed to sleep from now */
+    gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
 #endif /* defined(CONFIG_USER_ONLY) */
 }
 #endif /* #if defined(TARGET_PPC64) */