[Qemu-devel] [PATCH] MIPS/system: MTC0 single-stepping PC update fix

[Qemu-devel] [PATCH] MIPS/system: MTC0 single-stepping PC update fix

[Maciej W. Rozycki]

 Some MTC0 (and possibly other) instructions switch to the BS_STOP state
to terminate the current translation block, so that the state transition
of the simulated CPU resulting from the CP0 operation takes effect with
the following instruction.  This happens for "mtc0 <reg>,c0_config" for
example.

 While single-stepping this has a side-effect of not advancing the PC past
the instruction just executed; subsequent single-step traps will stop at
the same instruction repeatedly.  This is obviously incorrect and (with my
limited understanding of QEMU internals) is fixed easily as below, making
the old PC be kept only for the BS_EXCP (exception condition) state.

 Example:

(gdb) stepi
0x80004d24 in _start ()
5: x/i $pc
=> 0x80004d24 <_start+364>:     mfc0    t1,c0_config
(gdb)
0x80004d28 in _start ()
5: x/i $pc
=> 0x80004d28 <_start+368>:     li      at,-8
(gdb)
0x80004d2c in _start ()
5: x/i $pc
=> 0x80004d2c <_start+372>:     and     t1,t1,at
(gdb)
0x80004d30 in _start ()
5: x/i $pc
=> 0x80004d30 <_start+376>:     ori     t1,t1,0x3
(gdb)
0x80004d34 in _start ()
5: x/i $pc
=> 0x80004d34 <_start+380>:     mtc0    t1,c0_config
(gdb)
0x80004d34 in _start ()
5: x/i $pc
=> 0x80004d34 <_start+380>:     mtc0    t1,c0_config
(gdb)
0x80004d34 in _start ()
5: x/i $pc
=> 0x80004d34 <_start+380>:     mtc0    t1,c0_config
(gdb)
0x80004d34 in _start ()
5: x/i $pc
=> 0x80004d34 <_start+380>:     mtc0    t1,c0_config
(gdb)

-- oops!

Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
---

 Please apply,

  Maciej

qemu-mips-mtc0-step.diff
Index: qemu-git-trunk/target-mips/translate.c
===================================================================
--- qemu-git-trunk.orig/target-mips/translate.c	2012-06-04 04:16:57.755560324 +0100
+++ qemu-git-trunk/target-mips/translate.c	2012-06-04 05:01:42.435594656 +0100
@@ -12494,7 +12494,7 @@ gen_intermediate_code_internal (CPUMIPSS
     if (tb->cflags & CF_LAST_IO)
         gen_io_end();
     if (env->singlestep_enabled && ctx.bstate != BS_BRANCH) {
-        save_cpu_state(&ctx, ctx.bstate == BS_NONE);
+        save_cpu_state(&ctx, ctx.bstate != BS_EXCP);
         gen_helper_0i(raise_exception, EXCP_DEBUG);
     } else {
         switch (ctx.bstate) {

Re: [Qemu-devel] [PATCH] MIPS/system: MTC0 single-stepping PC update fix

[Richard Henderson]

On 2012-06-07 18:05, Maciej W. Rozycki wrote:
>      if (env->singlestep_enabled && ctx.bstate != BS_BRANCH) {
> -        save_cpu_state(&ctx, ctx.bstate == BS_NONE);
> +        save_cpu_state(&ctx, ctx.bstate != BS_EXCP);
>          gen_helper_0i(raise_exception, EXCP_DEBUG);

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

[Qemu-devel] [PATCH RESEND] mips: Ensure PC update with MTC0 single-stepping

[Maciej W. Rozycki]

Correct the way PC is updated when single-stepping instructions, by 
keeping the old PC only for the BS_EXCP (exception condition) state.

Some MTC0 (and possibly other) instructions switch to the BS_STOP state 
to terminate the current translation block, so that the state transition 
of the simulated CPU resulting from the CP0 operation takes effect with 
the following instruction.  This happens with `mtc0 <reg>,c0_config' for 
example, typically used to set KSEG0 cacheability.

While single-stepping this has a side-effect of not advancing the PC 
past the instruction just executed; subsequent single-step traps will 
stop at the same instruction repeatedly.  Example:

(gdb) stepi
0x80004d24 in _start ()
5: x/i $pc
=> 0x80004d24 <_start+364>:     mfc0    t1,c0_config
(gdb)
0x80004d28 in _start ()
5: x/i $pc
=> 0x80004d28 <_start+368>:     li      at,-8
(gdb)
0x80004d2c in _start ()
5: x/i $pc
=> 0x80004d2c <_start+372>:     and     t1,t1,at
(gdb)
0x80004d30 in _start ()
5: x/i $pc
=> 0x80004d30 <_start+376>:     ori     t1,t1,0x3
(gdb)
0x80004d34 in _start ()
5: x/i $pc
=> 0x80004d34 <_start+380>:     mtc0    t1,c0_config
(gdb)
0x80004d34 in _start ()
5: x/i $pc
=> 0x80004d34 <_start+380>:     mtc0    t1,c0_config
(gdb)
0x80004d34 in _start ()
5: x/i $pc
=> 0x80004d34 <_start+380>:     mtc0    t1,c0_config
(gdb)
0x80004d34 in _start ()
5: x/i $pc
=> 0x80004d34 <_start+380>:     mtc0    t1,c0_config
(gdb)

-- oops!

Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
---
 It's been lost and waited for too long now, the original submission has 
been archived here:

http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg01227.html

I have verified with a manual check that the issue is still there and 
that the fix still works.  Please apply.

  Maciej

qemu-mips-mtc0-step.diff
Index: qemu-git-trunk/target-mips/translate.c
===================================================================
--- qemu-git-trunk.orig/target-mips/translate.c	2014-11-02 18:51:10.838947420 +0000
+++ qemu-git-trunk/target-mips/translate.c	2014-11-02 18:51:14.838939198 +0000
@@ -17522,7 +17522,7 @@ gen_intermediate_code_internal(MIPSCPU *
         gen_io_end();
     }
     if (cs->singlestep_enabled && ctx.bstate != BS_BRANCH) {
-        save_cpu_state(&ctx, ctx.bstate == BS_NONE);
+        save_cpu_state(&ctx, ctx.bstate != BS_EXCP);
         gen_helper_0e0i(raise_exception, EXCP_DEBUG);
     } else {
         switch (ctx.bstate) {

Re: [Qemu-devel] [PATCH RESEND] mips: Ensure PC update with MTC0 single-stepping

[Leon Alrae]

On 06/11/2014 20:38, Maciej W. Rozycki wrote:
> Correct the way PC is updated when single-stepping instructions, by 
> keeping the old PC only for the BS_EXCP (exception condition) state.
> 
> Some MTC0 (and possibly other) instructions switch to the BS_STOP state 
> to terminate the current translation block, so that the state transition 
> of the simulated CPU resulting from the CP0 operation takes effect with 
> the following instruction.  This happens with `mtc0 <reg>,c0_config' for 
> example, typically used to set KSEG0 cacheability.
> 
> While single-stepping this has a side-effect of not advancing the PC 
> past the instruction just executed; subsequent single-step traps will 
> stop at the same instruction repeatedly.  Example:
> 
> (gdb) stepi
> 0x80004d24 in _start ()
> 5: x/i $pc
> => 0x80004d24 <_start+364>:     mfc0    t1,c0_config
> (gdb)
> 0x80004d28 in _start ()
> 5: x/i $pc
> => 0x80004d28 <_start+368>:     li      at,-8
> (gdb)
> 0x80004d2c in _start ()
> 5: x/i $pc
> => 0x80004d2c <_start+372>:     and     t1,t1,at
> (gdb)
> 0x80004d30 in _start ()
> 5: x/i $pc
> => 0x80004d30 <_start+376>:     ori     t1,t1,0x3
> (gdb)
> 0x80004d34 in _start ()
> 5: x/i $pc
> => 0x80004d34 <_start+380>:     mtc0    t1,c0_config
> (gdb)
> 0x80004d34 in _start ()
> 5: x/i $pc
> => 0x80004d34 <_start+380>:     mtc0    t1,c0_config
> (gdb)
> 0x80004d34 in _start ()
> 5: x/i $pc
> => 0x80004d34 <_start+380>:     mtc0    t1,c0_config
> (gdb)
> 0x80004d34 in _start ()
> 5: x/i $pc
> => 0x80004d34 <_start+380>:     mtc0    t1,c0_config
> (gdb)
> 
> -- oops!
> 
> Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
> ---
>  It's been lost and waited for too long now, the original submission has 
> been archived here:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg01227.html
> 
> I have verified with a manual check that the issue is still there and 
> that the fix still works.  Please apply.
> 
>   Maciej
> 
> qemu-mips-mtc0-step.diff
> Index: qemu-git-trunk/target-mips/translate.c
> ===================================================================
> --- qemu-git-trunk.orig/target-mips/translate.c	2014-11-02 18:51:10.838947420 +0000
> +++ qemu-git-trunk/target-mips/translate.c	2014-11-02 18:51:14.838939198 +0000
> @@ -17522,7 +17522,7 @@ gen_intermediate_code_internal(MIPSCPU *
>          gen_io_end();
>      }
>      if (cs->singlestep_enabled && ctx.bstate != BS_BRANCH) {
> -        save_cpu_state(&ctx, ctx.bstate == BS_NONE);
> +        save_cpu_state(&ctx, ctx.bstate != BS_EXCP);
>          gen_helper_0e0i(raise_exception, EXCP_DEBUG);
>      } else {
>          switch (ctx.bstate) {
> 

Good fix for 2.2, thanks.

Reviewed-by: Leon Alrae <leon.alrae@imgtec.com>

Regards,
Leon