[Qemu-devel] Single stepping for PPC broken?

[Qemu-devel] Single stepping for PPC broken?

[Marius Groeger]

Hello all,

I'm having problems with qemu's (-M prep, -cpu 604) handling of the 
MSR_SE bit. My gdbstub can successfully step along regular code, but 
qemu chokes when stepping over a branch instruction like "blr". 
(Needless to say, that same gdbstub works fine on real hardware). I 
tried older versions of qemu and found that the code base 8 months ago 
worked fine.

I'd like to try this with Linux and gdbserver (or a native gdb) but I 
don't have any boot images handy and oszoo.org seems to be down right 
now.

Any ideas? Did perhaps the PPC440 additions add some regression here?
Can someone try booting Linux on qemu-system-ppc and try 
gdb/gdbserver?

(I haven't been following this list lately, so please bear with me if 
I missed something critical. I've searched the archives, of course, 
but to no avail.)

Regards and TIA,
Marius

-- 
Marius Groeger <mgroeger@sysgo.com>
SYSGO AG                      Embedded and Real-Time Software
Voice: +49 6136 9948 0                  FAX: +49 6136 9948 10
www.sysgo.com | www.elinos.com | www.osek.de | www.pikeos.com

Re: [Qemu-devel] Single stepping for PPC broken!

[Marius Groeger]

On Wed, 9 Jan 2008, Marius Groeger wrote:

> I'm having problems with qemu's (-M prep, -cpu 604) handling of the 
> MSR_SE bit. My gdbstub can successfully step along regular code, but 
> qemu chokes when stepping over a branch instruction like "blr". 
> (Needless to say, that same gdbstub works fine on real hardware). I 
> tried older versions of qemu and found that the code base 8 months ago 
> worked fine.

I have now verified with booting a Linux image into qemu-system-ppc - same
problem. When stepi'ing over the following sequence, the system chokes on a
"bl" instruction:

  / # gdb testprg
  GNU gdb 6.3.50.20050810
  Copyright 2004 Free Software Foundation, Inc.
  GDB is free software, covered by the GNU General Public License, and you are
  welcome to change it and/or distribute copies of it under certain conditions.
  Type "show copying" to see the conditions.
  There is absolutely no warranty for GDB.  Type "show warranty" for details.
  This GDB was configured as "powerpc-linux"...Using host libthread_db library
  "/lib/libthread_db.so.1".
  
  (gdb) b main
  Breakpoint 1 at 0x10000520: file testprg.c, line 26.
  (gdb) run
  Starting program: testprg
  Breakpoint 1, main () at testprg.c:26
  26  testprg.c: No such file or directory.
  in testprg.c
  (gdb) disassemble
  Dump of assembler code for function main:
  0x1000050c <main+0>:stwu    r1,-32(r1)
  0x10000510 <main+4>:mflr    r0
  0x10000514 <main+8>:stw     r31,28(r1)
  0x10000518 <main+12>:stw     r0,36(r1)
  0x1000051c <main+16>:mr      r31,r1
  0x10000520 <main+20>:lis     r9,4096
  0x10000524 <main+24>:addi    r3,r9,2376
  0x10000528 <main+28>:crclr   4*cr1+eq
  0x1000052c <main+32>:bl      0x10010ad8 <printf>
  0x10000530 <main+36>:lis     r9,4096
  ...
  (gdb) stepi
  0x10000524   26 in testprg.c
  (gdb) stepi
  0x10000528   26 in testprg.c
  (gdb) stepi
  0x1000052c   26 in testprg.c
  (gdb) stepi
  <<< QEMU HANGS! >>>

> Any ideas? Did perhaps the PPC440 additions add some regression here?

?!

Regards and TIA,
Marius

-- 
Marius Groeger <mgroeger@sysgo.com>
SYSGO AG                      Embedded and Real-Time Software
Voice: +49 6136 9948 0                  FAX: +49 6136 9948 10
www.sysgo.com | www.elinos.com | www.osek.de | www.pikeos.com

Re: [PATCH][Qemu-devel] Single stepping for PPC broken!

[Marius Groeger]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1098 bytes --]

On Wed, 9 Jan 2008, Marius Groeger wrote:

> On Wed, 9 Jan 2008, Marius Groeger wrote:
> 
> > I'm having problems with qemu's (-M prep, -cpu 604) handling of the 
> > MSR_SE bit. My gdbstub can successfully step along regular code, but 
> > qemu chokes when stepping over a branch instruction like "blr". 
> > (Needless to say, that same gdbstub works fine on real hardware). I 
> > tried older versions of qemu and found that the code base 8 months ago 
> > worked fine.
> 
> I have now verified with booting a Linux image into qemu-system-ppc - same
> problem. When stepi'ing over the following sequence, the system chokes on a
> "bl" instruction:

The attached patch fixes the problem, but I have to admit I can't tell 
for sure if this doesn't break other things (such as qemu's built-in 
GDB server). Could some QEMU ppc expert please comment on this?

Thanks
Marius

-- 
Marius Groeger <mgroeger@sysgo.com>
SYSGO AG                      Embedded and Real-Time Software
Voice: +49 6136 9948 0                  FAX: +49 6136 9948 10
www.sysgo.com | www.elinos.com | www.osek.de | www.pikeos.com

[-- Attachment #2: Type: TEXT/PLAIN, Size: 987 bytes --]

Index: target-ppc/translate.c
===================================================================
RCS file: /sources/qemu/qemu/target-ppc/translate.c,v
retrieving revision 1.115
diff -u -r1.115 translate.c
--- target-ppc/translate.c	24 Nov 2007 02:03:55 -0000	1.115
+++ target-ppc/translate.c	10 Jan 2008 13:54:36 -0000
@@ -2811,8 +2811,6 @@
 #endif
             gen_op_b_T1();
         gen_op_set_T0((long)tb + n);
-        if (ctx->singlestep_enabled)
-            gen_op_debug();
         gen_op_exit_tb();
     } else {
         gen_set_T1(dest);
@@ -2823,8 +2821,6 @@
 #endif
             gen_op_b_T1();
         gen_op_reset_T0();
-        if (ctx->singlestep_enabled)
-            gen_op_debug();
         gen_op_exit_tb();
     }
 }
@@ -3007,8 +3003,6 @@
             gen_op_btest_T1(ctx->nip);
         gen_op_reset_T0();
     no_test:
-        if (ctx->singlestep_enabled)
-            gen_op_debug();
         gen_op_exit_tb();
     }
  out:

Re: [PATCH][Qemu-devel] Single stepping for PPC broken!

[Rob Landley]

On Thursday 10 January 2008 07:57:50 Marius Groeger wrote:
> The attached patch fixes the problem, but I have to admit I can't tell
> for sure if this doesn't break other things (such as qemu's built-in
> GDB server). Could some QEMU ppc expert please comment on this?

Looks fine to me, but I don't see it in the git mirror I follow...

Did anybody notice this patch?

Rob
-- 
"One of my most productive days was throwing away 1000 lines of code."
  - Ken Thompson.

Re: [PATCH][Qemu-devel] Single stepping for PPC broken!

[Marius Groeger]

On Mon, 11 Feb 2008, Rob Landley wrote:

> On Thursday 10 January 2008 07:57:50 Marius Groeger wrote:
> > The attached patch fixes the problem, but I have to admit I can't tell
> > for sure if this doesn't break other things (such as qemu's built-in
> > GDB server). Could some QEMU ppc expert please comment on this?
> 
> Looks fine to me, but I don't see it in the git mirror I follow...
> 
> Did anybody notice this patch?

Apparently not :-)

I just checked if it still applies, and it doesn't. Checking why I ran 
into the following strangeness in target-ppc/translate.c:gen_goto_tb() 
which appeared during the TCG migration:

  ..
  if ((tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK) &&
     !ctx->singlestep_enabled) {
  ..
  } else {
    gen_set_T1(dest);
#if defined(TARGET_PPC64)
    if (ctx->sf_mode)
      gen_op_b_T1_64();
     else
#endif
      gen_op_b_T1();
    if (ctx->singlestep_enabled)
      gen_op_debug()
  }

It seems to me that the second if (ctx->singlestep_enabled) is 
rendundant.

I'll see if I can find some time to see if the patch is still needed 
and if so, update it to the current HEAD.

Thanks
Marius

-- 
Marius Groeger
SYSGO AG                      Embedded and Real-Time Software
Voice: +49 6136 9948 0                  FAX: +49 6136 9948 10
www.sysgo.com | www.elinos.com | www.osek.de | www.pikeos.com

Re: [PATCH][Qemu-devel] Single stepping for PPC broken!

[Daniel Jacobowitz]

On Wed, Feb 13, 2008 at 09:46:44AM +0100, Marius Groeger wrote:
>   if ((tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK) &&
>      !ctx->singlestep_enabled) {
>   ..
>   } else {
>     gen_set_T1(dest);
> #if defined(TARGET_PPC64)
>     if (ctx->sf_mode)
>       gen_op_b_T1_64();
>      else
> #endif
>       gen_op_b_T1();
>     if (ctx->singlestep_enabled)
>       gen_op_debug()
>   }
> 
> It seems to me that the second if (ctx->singlestep_enabled) is 
> rendundant.

No, if you've gone to a different page without single step then you
don't need the debug trap.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH][Qemu-devel] Single stepping for PPC broken!

[Marius Groeger]

On Wed, 13 Feb 2008, Daniel Jacobowitz wrote:

> On Wed, Feb 13, 2008 at 09:46:44AM +0100, Marius Groeger wrote:
> >   if ((tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK) &&
> >      !ctx->singlestep_enabled) {
> >   ..
> >   } else {
> >     gen_set_T1(dest);
> > #if defined(TARGET_PPC64)
> >     if (ctx->sf_mode)
> >       gen_op_b_T1_64();
> >      else
> > #endif
> >       gen_op_b_T1();
> >     if (ctx->singlestep_enabled)
> >       gen_op_debug()
> >   }
> > 
> > It seems to me that the second if (ctx->singlestep_enabled) is 
> > rendundant.
> 
> No, if you've gone to a different page without single step then you
> don't need the debug trap.

Hm, so you mean betweeen the first "if .. !ctx->singlestep_enabled" 
and the second one in the evaluation of ctx->singlestep_enabled 
changes? What I meant is simply that the "else" clause already implies 
that ctx->singlestep_enabled is true.

Regards
Marius

-- 
Marius Groeger
SYSGO AG                      Embedded and Real-Time Software
Voice: +49 6136 9948 0                  FAX: +49 6136 9948 10
www.sysgo.com | www.elinos.com | www.osek.de | www.pikeos.com

Re: [PATCH][Qemu-devel] Single stepping for PPC broken!

[Daniel Jacobowitz]

On Wed, Feb 13, 2008 at 04:52:22PM +0100, Marius Groeger wrote:
> On Wed, 13 Feb 2008, Daniel Jacobowitz wrote:
> 
> > On Wed, Feb 13, 2008 at 09:46:44AM +0100, Marius Groeger wrote:
> > >   if ((tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK) &&
> > >      !ctx->singlestep_enabled) {

> > No, if you've gone to a different page without single step then you
> > don't need the debug trap.
> 
> Hm, so you mean betweeen the first "if .. !ctx->singlestep_enabled" 
> and the second one in the evaluation of ctx->singlestep_enabled 
> changes? What I meant is simply that the "else" clause already implies 
> that ctx->singlestep_enabled is true.

No it doesn't.

if (A && !B) {
   ...
} else {
   ...
}

The else block will be entered if !A, or if A && B.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH][Qemu-devel] Single stepping for PPC broken!

[Marius Groeger]

On Wed, 13 Feb 2008, Daniel Jacobowitz wrote:

> The else block will be entered if !A, or if A && B.

Yeah - oops - sorry :-)

Regards
Marius

-- 
Marius Groeger
SYSGO AG                      Embedded and Real-Time Software
Voice: +49 6136 9948 0                  FAX: +49 6136 9948 10
www.sysgo.com | www.elinos.com | www.osek.de | www.pikeos.com

Re: [PATCH][Qemu-devel] Single stepping for PPC broken!

[Jason Wessel]

[-- Attachment #1: Type: text/plain, Size: 1376 bytes --]

Marius Groeger wrote:
> On Wed, 9 Jan 2008, Marius Groeger wrote:
>
>   
>> On Wed, 9 Jan 2008, Marius Groeger wrote:
>>
>>     
>>> I'm having problems with qemu's (-M prep, -cpu 604) handling of the 
>>> MSR_SE bit. My gdbstub can successfully step along regular code, but 
>>> qemu chokes when stepping over a branch instruction like "blr". 
>>> (Needless to say, that same gdbstub works fine on real hardware). I 
>>> tried older versions of qemu and found that the code base 8 months ago 
>>> worked fine.
>>>       
>> I have now verified with booting a Linux image into qemu-system-ppc - same
>> problem. When stepi'ing over the following sequence, the system chokes on a
>> "bl" instruction:
>>     
>
> The attached patch fixes the problem, but I have to admit I can't tell 
> for sure if this doesn't break other things (such as qemu's built-in 
> GDB server). Could some QEMU ppc expert please comment on this?
>
> Thanks
> Marius
>
>   

The patch you originally attached definitely breaks the back end
debugger connection for qemu.  It does point to the heart of the problem
though.  The back end debugger uses the same variable to control the
single stepping state as the MSR_SE uses.

Attached is a patch that fixes the issue, as well as a generic problem
in cvs latest where the backend debugger is occasionally missing debug
exceptions on all archs.


Jason.

[-- Attachment #2: ppc_system_single_step.patch --]
[-- Type: text/x-patch, Size: 2964 bytes --]


- Fix generic single step problem in vl.c
  * Overwriting the ret code when there was
    and interrupt pending causes the debugger
    to miss exceptions

- For ppc, split run-time single stepping from the
  debugger stub single stepping
  * This fixes the hang problems when using single
    stepping via the msr_se


Signed-off-by: Jason Wessel <jason.wessel@windriver.com>

---
 target-ppc/translate.c |   14 ++++++++++++--
 vl.c                   |    4 ++--
 2 files changed, 14 insertions(+), 4 deletions(-)

--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -150,6 +150,7 @@ typedef struct DisasContext {
     int spe_enabled;
     ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
     int singlestep_enabled;
+    int sys_sstep_enabled;
     int dcache_line_size;
 } DisasContext;
 
@@ -2802,8 +2803,10 @@ static always_inline void gen_goto_tb (D
         else
 #endif
             gen_op_b_T1();
-        if (ctx->singlestep_enabled)
+       if (unlikely(ctx->sys_sstep_enabled)) {
+            gen_update_nip(ctx, ctx->nip);
             gen_op_debug();
+        }
         tcg_gen_exit_tb(0);
     }
 }
@@ -2984,8 +2987,10 @@ static always_inline void gen_bcond (Dis
 #endif
             gen_op_btest_T1(ctx->nip);
     no_test:
-        if (ctx->singlestep_enabled)
+        if (ctx->sys_sstep_enabled) {
+            gen_update_nip(ctx, ctx->nip);
             gen_op_debug();
+        }
         tcg_gen_exit_tb(0);
     }
  out:
@@ -6190,6 +6195,7 @@ static always_inline int gen_intermediat
         branch_step = 1;
     else
         branch_step = 0;
+    ctx.sys_sstep_enabled = env->singlestep_enabled;
     ctx.singlestep_enabled = env->singlestep_enabled || single_step == 1;
 #if defined (DO_SINGLE_STEP) && 0
     /* Single step trace mode */
@@ -6306,6 +6312,10 @@ static always_inline int gen_intermediat
     if (ctx.exception == POWERPC_EXCP_NONE) {
         gen_goto_tb(&ctx, 0, ctx.nip);
     } else if (ctx.exception != POWERPC_EXCP_BRANCH) {
+        if (unlikely(ctx.sys_sstep_enabled)) {
+            gen_update_nip(&ctx, ctx.nip);
+            gen_op_debug();
+        }
         /* Generate the return instruction */
         tcg_gen_exit_tb(0);
     }
--- a/vl.c
+++ b/vl.c
@@ -7523,7 +7523,7 @@ static int main_loop(void)
                 qemu_time += profile_getclock() - ti;
 #endif
                 next_cpu = env->next_cpu ?: first_cpu;
-                if (event_pending) {
+                if (event_pending && likely(ret != EXCP_DEBUG)) {
                     ret = EXCP_INTERRUPT;
                     event_pending = 0;
                     break;
@@ -7555,7 +7555,7 @@ static int main_loop(void)
 		qemu_system_powerdown();
                 ret = EXCP_INTERRUPT;
             }
-            if (ret == EXCP_DEBUG) {
+            if (unlikely(ret == EXCP_DEBUG)) {
                 vm_stop(EXCP_DEBUG);
             }
             /* If all cpus are halted then wait until the next IRQ */