[PATCH 0/4] target/m68k: implement m68k "any instruction" trace mode

[PATCH 0/4] target/m68k: implement m68k "any instruction" trace mode

[Mark Cave-Ayland]

This patchset implements the m68k "any instruction" tracing mode which is used
by the NetBSD kernel debugger to implement single-stepping. It is based upon
reading through the M68000 PRM and looking at how the ARM target handles both
gdbstub and architectural single-step exceptions.

With this patchset it becomes possible to single-step the NetBSD kernel using
the in-built kernel debugger:

Stopped in pid 0.1 (system) at  netbsd:cpu_Debugger+0x6:        unlk    a6
db> s
Stopped in pid 0.1 (system) at  netbsd:cpu_Debugger+0x8:        rts
db> 
Stopped in pid 0.1 (system) at  netbsd:main+0x2c:       jsr     kernel_lock_init
        [addr:0x1a38f6 ]
db> 
Stopped in pid 0.1 (system) at  netbsd:kernel_lock_init:        linkw   a6,#0
db> 
Stopped in pid 0.1 (system) at  netbsd:kernel_lock_init+0x4:    clrb    kernel_l
ock     [addr:0x33f400 ]
db> 
Stopped in pid 0.1 (system) at  netbsd:kernel_lock_init+0xa:    clrb    kernel_l
ock_dodebug     [addr:0x35b48c ]
db> 
Stopped in pid 0.1 (system) at  netbsd:kernel_lock_init+0x10:   unlk    a6
db> 
Stopped in pid 0.1 (system) at  netbsd:kernel_lock_init+0x12:   rts
db> c
Copyright (c) 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005,
[   1.0000000]     2006, 2007, 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017,
[   1.0000000]     2018, 2019, 2020 The NetBSD Foundation, Inc.  All rights reserved.
[   1.0000000] Copyright (c) 1982, 1986, 1989, 1991, 1993
[   1.0000000]     The Regents of the University of California.  All rights reserved.

[   1.0000000] NetBSD 9.1 (GENERIC) #0: Sun Oct 18 19:24:30 UTC 2020
... etc ...


Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

[q800-macos-upstream patchset series: 2]

Mark Cave-Ayland (4):
  target/m68k: introduce is_singlestepping() function
  target/m68k: call gen_raise_exception() directly if single-stepping in
    gen_jmp_tb()
  target/m68k: introduce gen_singlestep_exception() function
  target/m68k: implement m68k "any instruction" trace mode

 target/m68k/cpu.h       |  8 +++++++
 target/m68k/translate.c | 51 ++++++++++++++++++++++++++++++++++-------
 2 files changed, 51 insertions(+), 8 deletions(-)

-- 
2.20.1

[PATCH 1/4] target/m68k: introduce is_singlestepping() function

[Mark Cave-Ayland]

The m68k translator currently checks the DisasContextBase singlestep_enabled
boolean directly to determine whether to single-step execution. Soon
single-stepping may also be triggered by setting the appropriate bits in the
SR register so centralise the check into a single is_singlestepping()
function.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target/m68k/translate.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 200018ae6a..c774f2e8f0 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -194,6 +194,17 @@ static void do_writebacks(DisasContext *s)
     }
 }
 
+static bool is_singlestepping(DisasContext *s)
+{
+    /*
+     * Return true if we are singlestepping either because of QEMU gdbstub
+     * singlestep. This does not include the command line '-singlestep' mode
+     * which is rather misnamed as it only means "one instruction per TB" and
+     * doesn't affect the code we generate.
+     */
+    return s->base.singlestep_enabled;
+}
+
 /* is_jmp field values */
 #define DISAS_JUMP      DISAS_TARGET_0 /* only pc was modified dynamically */
 #define DISAS_EXIT      DISAS_TARGET_1 /* cpu state was modified dynamically */
@@ -1506,7 +1517,7 @@ static inline bool use_goto_tb(DisasContext *s, uint32_t dest)
 /* Generate a jump to an immediate address.  */
 static void gen_jmp_tb(DisasContext *s, int n, uint32_t dest)
 {
-    if (unlikely(s->base.singlestep_enabled)) {
+    if (unlikely(is_singlestepping(s))) {
         gen_exception(s, dest, EXCP_DEBUG);
     } else if (use_goto_tb(s, dest)) {
         tcg_gen_goto_tb(n);
@@ -6245,7 +6256,7 @@ static void m68k_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
         break;
     case DISAS_TOO_MANY:
         update_cc_op(dc);
-        if (dc->base.singlestep_enabled) {
+        if (is_singlestepping(dc)) {
             tcg_gen_movi_i32(QREG_PC, dc->pc);
             gen_raise_exception(EXCP_DEBUG);
         } else {
@@ -6254,7 +6265,7 @@ static void m68k_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
         break;
     case DISAS_JUMP:
         /* We updated CC_OP and PC in gen_jmp/gen_jmp_im.  */
-        if (dc->base.singlestep_enabled) {
+        if (is_singlestepping(dc)) {
             gen_raise_exception(EXCP_DEBUG);
         } else {
             tcg_gen_lookup_and_goto_ptr();
@@ -6265,7 +6276,7 @@ static void m68k_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
          * We updated CC_OP and PC in gen_exit_tb, but also modified
          * other state that may require returning to the main loop.
          */
-        if (dc->base.singlestep_enabled) {
+        if (is_singlestepping(dc)) {
             gen_raise_exception(EXCP_DEBUG);
         } else {
             tcg_gen_exit_tb(NULL, 0);
-- 
2.20.1

Re: [PATCH 1/4] target/m68k: introduce is_singlestepping() function

[Richard Henderson]

On 5/19/21 9:29 AM, Mark Cave-Ayland wrote:
> The m68k translator currently checks the DisasContextBase singlestep_enabled
> boolean directly to determine whether to single-step execution. Soon
> single-stepping may also be triggered by setting the appropriate bits in the
> SR register so centralise the check into a single is_singlestepping()
> function.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 2/4] target/m68k: call gen_raise_exception() directly if single-stepping in gen_jmp_tb()

[Mark Cave-Ayland]

In order to consolidate the single-step exception handling into a single
helper, change gen_jmp_tb() so that it calls gen_raise_exception() directly
instead of gen_exception(). This ensures that all single-step exceptions are
now handled directly by gen_raise_exception().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target/m68k/translate.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index c774f2e8f0..f14ecab5a5 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -1518,7 +1518,9 @@ static inline bool use_goto_tb(DisasContext *s, uint32_t dest)
 static void gen_jmp_tb(DisasContext *s, int n, uint32_t dest)
 {
     if (unlikely(is_singlestepping(s))) {
-        gen_exception(s, dest, EXCP_DEBUG);
+        update_cc_op(s);
+        tcg_gen_movi_i32(QREG_PC, dest);
+        gen_raise_exception(EXCP_DEBUG);
     } else if (use_goto_tb(s, dest)) {
         tcg_gen_goto_tb(n);
         tcg_gen_movi_i32(QREG_PC, dest);
-- 
2.20.1

Re: [PATCH 2/4] target/m68k: call gen_raise_exception() directly if single-stepping in gen_jmp_tb()

[Richard Henderson]

On 5/19/21 9:29 AM, Mark Cave-Ayland wrote:
> In order to consolidate the single-step exception handling into a single
> helper, change gen_jmp_tb() so that it calls gen_raise_exception() directly
> instead of gen_exception(). This ensures that all single-step exceptions are
> now handled directly by gen_raise_exception().
> 
> Signed-off-by: Mark Cave-Ayland<mark.cave-ayland@ilande.co.uk>
> ---
>   target/m68k/translate.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 3/4] target/m68k: introduce gen_singlestep_exception() function

[Mark Cave-Ayland]

Introduce a new gen_singlestep_exception() function to be called when generating
the EXCP_DEBUG exception in single-step mode rather than calling
gen_raise_exception(EXCP_DEBUG) directly. This allows for the single-step
exception behaviour for all callers to be managed in a single place.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target/m68k/translate.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index f14ecab5a5..10e8aba42e 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -319,6 +319,15 @@ static void gen_exception(DisasContext *s, uint32_t dest, int nr)
     s->base.is_jmp = DISAS_NORETURN;
 }
 
+static void gen_singlestep_exception(DisasContext *s)
+{
+    /*
+     * Generate the right kind of exception for singlestep, which is
+     * EXCP_DEBUG for QEMU's gdb singlestepping.
+     */
+    gen_raise_exception(EXCP_DEBUG);
+}
+
 static inline void gen_addr_fault(DisasContext *s)
 {
     gen_exception(s, s->base.pc_next, EXCP_ADDRESS);
@@ -1520,7 +1529,7 @@ static void gen_jmp_tb(DisasContext *s, int n, uint32_t dest)
     if (unlikely(is_singlestepping(s))) {
         update_cc_op(s);
         tcg_gen_movi_i32(QREG_PC, dest);
-        gen_raise_exception(EXCP_DEBUG);
+        gen_singlestep_exception(s);
     } else if (use_goto_tb(s, dest)) {
         tcg_gen_goto_tb(n);
         tcg_gen_movi_i32(QREG_PC, dest);
@@ -6260,7 +6269,7 @@ static void m68k_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
         update_cc_op(dc);
         if (is_singlestepping(dc)) {
             tcg_gen_movi_i32(QREG_PC, dc->pc);
-            gen_raise_exception(EXCP_DEBUG);
+            gen_singlestep_exception(dc);
         } else {
             gen_jmp_tb(dc, 0, dc->pc);
         }
@@ -6268,7 +6277,7 @@ static void m68k_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
     case DISAS_JUMP:
         /* We updated CC_OP and PC in gen_jmp/gen_jmp_im.  */
         if (is_singlestepping(dc)) {
-            gen_raise_exception(EXCP_DEBUG);
+            gen_singlestep_exception(dc);
         } else {
             tcg_gen_lookup_and_goto_ptr();
         }
@@ -6279,7 +6288,7 @@ static void m68k_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
          * other state that may require returning to the main loop.
          */
         if (is_singlestepping(dc)) {
-            gen_raise_exception(EXCP_DEBUG);
+            gen_singlestep_exception(dc);
         } else {
             tcg_gen_exit_tb(NULL, 0);
         }
-- 
2.20.1

Re: [PATCH 3/4] target/m68k: introduce gen_singlestep_exception() function

[Richard Henderson]

On 5/19/21 9:29 AM, Mark Cave-Ayland wrote:
> Introduce a new gen_singlestep_exception() function to be called when generating
> the EXCP_DEBUG exception in single-step mode rather than calling
> gen_raise_exception(EXCP_DEBUG) directly. This allows for the single-step
> exception behaviour for all callers to be managed in a single place.
> 
> Signed-off-by: Mark Cave-Ayland<mark.cave-ayland@ilande.co.uk>
> ---
>   target/m68k/translate.c | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 4/4] target/m68k: implement m68k "any instruction" trace mode

[Mark Cave-Ayland]

The m68k trace mode is controlled by the top 2 bits in the SR register. Implement
the m68k "any instruction" trace mode where bit T1=1 and bit T0=0 in which the CPU
generates an EXCP_TRACE exception (vector 9 or offset 0x24) after executing each
instruction.

This functionality is used by the NetBSD kernel debugger to allow single-stepping
on m68k architectures.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target/m68k/cpu.h       |  8 ++++++++
 target/m68k/translate.c | 27 ++++++++++++++++++++-------
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
index 402c86c876..997d588911 100644
--- a/target/m68k/cpu.h
+++ b/target/m68k/cpu.h
@@ -230,6 +230,9 @@ typedef enum {
 #define SR_T_SHIFT 14
 #define SR_T  0xc000
 
+#define M68K_SR_TRACE(sr) ((sr & SR_T) >> SR_T_SHIFT)
+#define M68K_SR_TRACE_ANY_INS 0x2
+
 #define M68K_SSP    0
 #define M68K_USP    1
 #define M68K_ISP    2
@@ -590,6 +593,8 @@ typedef M68kCPU ArchCPU;
 #define TB_FLAGS_SFC_S          (1 << TB_FLAGS_SFC_S_BIT)
 #define TB_FLAGS_DFC_S_BIT      15
 #define TB_FLAGS_DFC_S          (1 << TB_FLAGS_DFC_S_BIT)
+#define TB_FLAGS_TRACE          16
+#define TB_FLAGS_TRACE_BIT      (1 << TB_FLAGS_TRACE)
 
 static inline void cpu_get_tb_cpu_state(CPUM68KState *env, target_ulong *pc,
                                         target_ulong *cs_base, uint32_t *flags)
@@ -602,6 +607,9 @@ static inline void cpu_get_tb_cpu_state(CPUM68KState *env, target_ulong *pc,
         *flags |= (env->sfc << (TB_FLAGS_SFC_S_BIT - 2)) & TB_FLAGS_SFC_S;
         *flags |= (env->dfc << (TB_FLAGS_DFC_S_BIT - 2)) & TB_FLAGS_DFC_S;
     }
+    if (M68K_SR_TRACE(env->sr) == M68K_SR_TRACE_ANY_INS) {
+        *flags |= TB_FLAGS_TRACE;
+    }
 }
 
 void dump_mmu(CPUM68KState *env);
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 10e8aba42e..f0c5bf9154 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -124,6 +124,7 @@ typedef struct DisasContext {
 #define MAX_TO_RELEASE 8
     int release_count;
     TCGv release[MAX_TO_RELEASE];
+    bool ss_active;
 } DisasContext;
 
 static void init_release_array(DisasContext *s)
@@ -197,12 +198,13 @@ static void do_writebacks(DisasContext *s)
 static bool is_singlestepping(DisasContext *s)
 {
     /*
-     * Return true if we are singlestepping either because of QEMU gdbstub
-     * singlestep. This does not include the command line '-singlestep' mode
-     * which is rather misnamed as it only means "one instruction per TB" and
-     * doesn't affect the code we generate.
+     * Return true if we are singlestepping either because of
+     * architectural singlestep or QEMU gdbstub singlestep. This does
+     * not include the command line '-singlestep' mode which is rather
+     * misnamed as it only means "one instruction per TB" and doesn't
+     * affect the code we generate.
      */
-    return s->base.singlestep_enabled;
+    return s->base.singlestep_enabled || s->ss_active;
 }
 
 /* is_jmp field values */
@@ -323,9 +325,14 @@ static void gen_singlestep_exception(DisasContext *s)
 {
     /*
      * Generate the right kind of exception for singlestep, which is
-     * EXCP_DEBUG for QEMU's gdb singlestepping.
+     * either the architectural singlestep or EXCP_DEBUG for QEMU's
+     * gdb singlestepping.
      */
-    gen_raise_exception(EXCP_DEBUG);
+    if (s->ss_active) {
+        gen_raise_exception(EXCP_TRACE);
+    } else {
+        gen_raise_exception(EXCP_DEBUG);
+    }
 }
 
 static inline void gen_addr_fault(DisasContext *s)
@@ -6194,6 +6201,12 @@ static void m68k_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cpu)
     dc->done_mac = 0;
     dc->writeback_mask = 0;
     init_release_array(dc);
+
+    dc->ss_active = (M68K_SR_TRACE(env->sr) == M68K_SR_TRACE_ANY_INS);
+    /* If architectural single step active, limit to 1 */
+    if (is_singlestepping(dc)) {
+        dc->base.max_insns = 1;
+    }
 }
 
 static void m68k_tr_tb_start(DisasContextBase *dcbase, CPUState *cpu)
-- 
2.20.1

Re: [PATCH 4/4] target/m68k: implement m68k "any instruction" trace mode

[Richard Henderson]

On 5/19/21 9:29 AM, Mark Cave-Ayland wrote:
> The m68k trace mode is controlled by the top 2 bits in the SR register. Implement
> the m68k "any instruction" trace mode where bit T1=1 and bit T0=0 in which the CPU
> generates an EXCP_TRACE exception (vector 9 or offset 0x24) after executing each
> instruction.
> 
> This functionality is used by the NetBSD kernel debugger to allow single-stepping
> on m68k architectures.
> 
> Signed-off-by: Mark Cave-Ayland<mark.cave-ayland@ilande.co.uk>
> ---
>   target/m68k/cpu.h       |  8 ++++++++
>   target/m68k/translate.c | 27 ++++++++++++++++++++-------
>   2 files changed, 28 insertions(+), 7 deletions(-)

It wouldn't be difficult to handle 'trace on change of flow' as well, if you 
wanted.  But this is certainly good.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 0/4] target/m68k: implement m68k "any instruction" trace mode

[Laurent Vivier]

Le 19/05/2021 à 16:29, Mark Cave-Ayland a écrit :
> This patchset implements the m68k "any instruction" tracing mode which is used
> by the NetBSD kernel debugger to implement single-stepping. It is based upon
> reading through the M68000 PRM and looking at how the ARM target handles both
> gdbstub and architectural single-step exceptions.
> 
> With this patchset it becomes possible to single-step the NetBSD kernel using
> the in-built kernel debugger:
> 
> Stopped in pid 0.1 (system) at  netbsd:cpu_Debugger+0x6:        unlk    a6
> db> s
> Stopped in pid 0.1 (system) at  netbsd:cpu_Debugger+0x8:        rts
> db> 
> Stopped in pid 0.1 (system) at  netbsd:main+0x2c:       jsr     kernel_lock_init
>         [addr:0x1a38f6 ]
> db> 
> Stopped in pid 0.1 (system) at  netbsd:kernel_lock_init:        linkw   a6,#0
> db> 
> Stopped in pid 0.1 (system) at  netbsd:kernel_lock_init+0x4:    clrb    kernel_l
> ock     [addr:0x33f400 ]
> db> 
> Stopped in pid 0.1 (system) at  netbsd:kernel_lock_init+0xa:    clrb    kernel_l
> ock_dodebug     [addr:0x35b48c ]
> db> 
> Stopped in pid 0.1 (system) at  netbsd:kernel_lock_init+0x10:   unlk    a6
> db> 
> Stopped in pid 0.1 (system) at  netbsd:kernel_lock_init+0x12:   rts
> db> c
> Copyright (c) 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005,
> [   1.0000000]     2006, 2007, 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017,
> [   1.0000000]     2018, 2019, 2020 The NetBSD Foundation, Inc.  All rights reserved.
> [   1.0000000] Copyright (c) 1982, 1986, 1989, 1991, 1993
> [   1.0000000]     The Regents of the University of California.  All rights reserved.
> 
> [   1.0000000] NetBSD 9.1 (GENERIC) #0: Sun Oct 18 19:24:30 UTC 2020
> ... etc ...
> 
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> [q800-macos-upstream patchset series: 2]
> 
> Mark Cave-Ayland (4):
>   target/m68k: introduce is_singlestepping() function
>   target/m68k: call gen_raise_exception() directly if single-stepping in
>     gen_jmp_tb()
>   target/m68k: introduce gen_singlestep_exception() function
>   target/m68k: implement m68k "any instruction" trace mode
> 
>  target/m68k/cpu.h       |  8 +++++++
>  target/m68k/translate.c | 51 ++++++++++++++++++++++++++++++++++-------
>  2 files changed, 51 insertions(+), 8 deletions(-)
> 

Applied to my m68k-for-6.1 tree.

Thanks,
Laurent