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

[Jason Wessel]

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.

- 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 <address@hidden>

---
 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 */