[v2,3/5] target/mips: Exit after enabling interrupts
diff mbox

Message ID 20170614194821.8754-4-rth@twiddle.net
State New
Headers show

Commit Message

Richard Henderson June 14, 2017, 7:48 p.m. UTC
From: Paolo Bonzini <pbonzini@redhat.com>

Exit to cpu loop so we reevaluate cpu_mips_hw_interrupts.

Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Yongbok Kim <yongbok.kim@imgtec.com>
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target/mips/translate.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Alex Bennée June 15, 2017, 8:57 a.m. UTC | #1
Richard Henderson <rth@twiddle.net> writes:

> From: Paolo Bonzini <pbonzini@redhat.com>
>
> Exit to cpu loop so we reevaluate cpu_mips_hw_interrupts.
>
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Yongbok Kim <yongbok.kim@imgtec.com>
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  target/mips/translate.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index 559f8fe..891f14b 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -13403,9 +13403,11 @@ static void gen_pool32axf (CPUMIPSState *env, DisasContext *ctx, int rt, int rs)
>                  save_cpu_state(ctx, 1);
>                  gen_helper_ei(t0, cpu_env);
>                  gen_store_gpr(t0, rs);
> -                /* Stop translation as we may have switched the execution mode */
> -                ctx->bstate = BS_STOP;
>                  tcg_temp_free(t0);
> +                /* BS_STOP isn't good enough here;
> +                   reevaluate cpu_mips_hw_interrupts_enabled.  */

nit: technically we want to ensure mips_cpu_exec_interrupt is run (which
calls cpu_mips_hw_interrupts_enabled)

> +                gen_save_pc(ctx->pc + 4);
> +                ctx->bstate = BS_EXCP;
>              }
>              break;
>          default:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée
Paolo Bonzini June 15, 2017, 2:16 p.m. UTC | #2
On 15/06/2017 10:57, Alex Bennée wrote:
>> +                /* BS_STOP isn't good enough here;
>> +                   reevaluate cpu_mips_hw_interrupts_enabled.  */
> nit: technically we want to ensure mips_cpu_exec_interrupt is run (which
> calls cpu_mips_hw_interrupts_enabled)

Right, that's why it's "reevaluate" not "reexecute". :)  Just a nit of
course.

Paolo
Aurelien Jarno June 15, 2017, 9:19 p.m. UTC | #3
On 2017-06-14 12:48, Richard Henderson wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> Exit to cpu loop so we reevaluate cpu_mips_hw_interrupts.
> 
> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Cc: Yongbok Kim <yongbok.kim@imgtec.com>
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  target/mips/translate.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index 559f8fe..891f14b 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -13403,9 +13403,11 @@ static void gen_pool32axf (CPUMIPSState *env, DisasContext *ctx, int rt, int rs)
>                  save_cpu_state(ctx, 1);
>                  gen_helper_ei(t0, cpu_env);
>                  gen_store_gpr(t0, rs);
> -                /* Stop translation as we may have switched the execution mode */
> -                ctx->bstate = BS_STOP;
>                  tcg_temp_free(t0);
> +                /* BS_STOP isn't good enough here;
> +                   reevaluate cpu_mips_hw_interrupts_enabled.  */
> +                gen_save_pc(ctx->pc + 4);
> +                ctx->bstate = BS_EXCP;
>              }
>              break;
>          default:

While the above looks correct, it's not complete. It only fixes the
microMIPS EI instruction. The MIPS one also has to be fixed.

For what I understood, anything that can change the result of
cpu_mips_hw_interrupts_enabled has to stop the translation. In that case
I checked that ERET/ERETNC and MTC0/DMTC0 to the Status register are
already correct, that said it might be a good idea to update the
comments to mention it.

I can work on a better patch, but I doubt I'll have time before the
week-end.

Aurelien
Richard Henderson June 15, 2017, 9:29 p.m. UTC | #4
On 06/15/2017 02:19 PM, Aurelien Jarno wrote:
> While the above looks correct, it's not complete. It only fixes the
> microMIPS EI instruction. The MIPS one also has to be fixed.
> 
> For what I understood, anything that can change the result of
> cpu_mips_hw_interrupts_enabled has to stop the translation. In that case
> I checked that ERET/ERETNC and MTC0/DMTC0 to the Status register are
> already correct, that said it might be a good idea to update the
> comments to mention it.
> 
> I can work on a better patch, but I doubt I'll have time before the
> week-end.

Ok, I'll drop this one for now.  It's not urgent.


r~

Patch
diff mbox

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 559f8fe..891f14b 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -13403,9 +13403,11 @@  static void gen_pool32axf (CPUMIPSState *env, DisasContext *ctx, int rt, int rs)
                 save_cpu_state(ctx, 1);
                 gen_helper_ei(t0, cpu_env);
                 gen_store_gpr(t0, rs);
-                /* Stop translation as we may have switched the execution mode */
-                ctx->bstate = BS_STOP;
                 tcg_temp_free(t0);
+                /* BS_STOP isn't good enough here;
+                   reevaluate cpu_mips_hw_interrupts_enabled.  */
+                gen_save_pc(ctx->pc + 4);
+                ctx->bstate = BS_EXCP;
             }
             break;
         default: