[PATCH 0/9] Improve RISC-V Debug support

[PATCH 0/9] Improve RISC-V Debug support

[frank.chang]

From: Frank Chang <frank.chang@sifive.com>

This patchset refactors RISC-V Debug support to allow more types of
triggers to be extended.

The initial support of type 6 trigger, which is similar to type 2
trigger with additional functionality, is also introduced in this
patchset.

Frank Chang (9):
  target/riscv: debug: Determine the trigger type from tdata1.type
  target/riscv: debug: Introduce build_tdata1() to build tdata1 register
    content
  target/riscv: debug: Introduce tdata1, tdata2, and tdata3 CSRs
  target/riscv: debug: Restrict the range of tselect value can be
    written
  target/riscv: debug: Introduce tinfo CSR
  target/riscv: debug: Create common trigger actions function
  target/riscv: debug: Check VU/VS modes for type 2 trigger
  target/riscv: debug: Return 0 if previous value written to tselect >=
    number of triggers
  target/riscv: debug: Add initial support of type 6 trigger

 target/riscv/cpu.h      |   7 +-
 target/riscv/cpu_bits.h |   1 +
 target/riscv/csr.c      |  10 +-
 target/riscv/debug.c    | 483 ++++++++++++++++++++++++++++++++--------
 target/riscv/debug.h    |  55 +++--
 target/riscv/machine.c  |  20 +-
 6 files changed, 445 insertions(+), 131 deletions(-)

--
2.36.1

[PATCH 1/9] target/riscv: debug: Determine the trigger type from tdata1.type

[frank.chang]

From: Frank Chang <frank.chang@sifive.com>

Current RISC-V debug assumes that only type 2 trigger is supported.
To allow more types of triggers to be supported in the future
(e.g. type 6 trigger, which is similar to type 2 trigger with additional
 functionality), we should determine the trigger type from tdata1.type.

RV_MAX_TRIGGERS is also introduced in replacement of TRIGGER_TYPE2_NUM.

Signed-off-by: Frank Chang <frank.chang@sifive.com>
---
 target/riscv/cpu.h     |   2 +-
 target/riscv/csr.c     |   2 +-
 target/riscv/debug.c   | 183 ++++++++++++++++++++++++++++-------------
 target/riscv/debug.h   |  15 ++--
 target/riscv/machine.c |   2 +-
 5 files changed, 137 insertions(+), 67 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 7d6397acdf..535123a989 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -289,7 +289,7 @@ struct CPUArchState {
 
     /* trigger module */
     target_ulong trigger_cur;
-    type2_trigger_t type2_trig[TRIGGER_TYPE2_NUM];
+    type2_trigger_t type2_trig[RV_MAX_TRIGGERS];
 
     /* machine specific rdtime callback */
     uint64_t (*rdtime_fn)(void *);
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 6dbe9b541f..005ae31a01 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -2776,7 +2776,7 @@ static RISCVException read_tdata(CPURISCVState *env, int csrno,
                                  target_ulong *val)
 {
     /* return 0 in tdata1 to end the trigger enumeration */
-    if (env->trigger_cur >= TRIGGER_NUM && csrno == CSR_TDATA1) {
+    if (env->trigger_cur >= RV_MAX_TRIGGERS && csrno == CSR_TDATA1) {
         *val = 0;
         return RISCV_EXCP_NONE;
     }
diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index fc6e13222f..abbcd38a17 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -52,8 +52,15 @@
 /* tdata availability of a trigger */
 typedef bool tdata_avail[TDATA_NUM];
 
-static tdata_avail tdata_mapping[TRIGGER_NUM] = {
-    [TRIGGER_TYPE2_IDX_0 ... TRIGGER_TYPE2_IDX_1] = { true, true, false },
+static tdata_avail tdata_mapping[TRIGGER_TYPE_NUM] = {
+    [TRIGGER_TYPE_NO_EXIST] = { false, false, false },
+    [TRIGGER_TYPE_AD_MATCH] = { true, true, true },
+    [TRIGGER_TYPE_INST_CNT] = { true, false, true },
+    [TRIGGER_TYPE_INT] = { true, true, true },
+    [TRIGGER_TYPE_EXCP] = { true, true, true },
+    [TRIGGER_TYPE_AD_MATCH6] = { true, true, true },
+    [TRIGGER_TYPE_EXT_SRC] = { true, false, false },
+    [TRIGGER_TYPE_UNAVAIL] = { true, true, true }
 };
 
 /* only breakpoint size 1/2/4/8 supported */
@@ -67,6 +74,26 @@ static int access_size[SIZE_NUM] = {
     [6 ... 15] = -1,
 };
 
+static inline target_ulong extract_trigger_type(CPURISCVState *env,
+                                                target_ulong tdata1)
+{
+    switch (riscv_cpu_mxl(env)) {
+    case MXL_RV32:
+        return extract32(tdata1, 28, 4);
+    case MXL_RV64:
+        return extract64(tdata1, 60, 4);
+    default:
+        g_assert_not_reached();
+    }
+}
+
+static inline target_ulong get_trigger_type(CPURISCVState *env,
+                                            target_ulong trigger_index)
+{
+    target_ulong tdata1 = env->type2_trig[trigger_index].mcontrol;
+    return extract_trigger_type(env, tdata1);
+}
+
 static inline target_ulong trigger_type(CPURISCVState *env,
                                         trigger_type_t type)
 {
@@ -89,15 +116,17 @@ static inline target_ulong trigger_type(CPURISCVState *env,
 
 bool tdata_available(CPURISCVState *env, int tdata_index)
 {
+    int trigger_type = get_trigger_type(env, env->trigger_cur);
+
     if (unlikely(tdata_index >= TDATA_NUM)) {
         return false;
     }
 
-    if (unlikely(env->trigger_cur >= TRIGGER_NUM)) {
+    if (unlikely(env->trigger_cur >= RV_MAX_TRIGGERS)) {
         return false;
     }
 
-    return tdata_mapping[env->trigger_cur][tdata_index];
+    return tdata_mapping[trigger_type][tdata_index];
 }
 
 target_ulong tselect_csr_read(CPURISCVState *env)
@@ -137,6 +166,7 @@ static target_ulong tdata1_validate(CPURISCVState *env, target_ulong val,
         qemu_log_mask(LOG_GUEST_ERROR,
                       "ignoring type write to tdata1 register\n");
     }
+
     if (dmode != 0) {
         qemu_log_mask(LOG_UNIMP, "debug mode is not supported\n");
     }
@@ -261,9 +291,8 @@ static void type2_breakpoint_remove(CPURISCVState *env, target_ulong index)
 }
 
 static target_ulong type2_reg_read(CPURISCVState *env,
-                                   target_ulong trigger_index, int tdata_index)
+                                   target_ulong index, int tdata_index)
 {
-    uint32_t index = trigger_index - TRIGGER_TYPE2_IDX_0;
     target_ulong tdata;
 
     switch (tdata_index) {
@@ -280,10 +309,9 @@ static target_ulong type2_reg_read(CPURISCVState *env,
     return tdata;
 }
 
-static void type2_reg_write(CPURISCVState *env, target_ulong trigger_index,
+static void type2_reg_write(CPURISCVState *env, target_ulong index,
                             int tdata_index, target_ulong val)
 {
-    uint32_t index = trigger_index - TRIGGER_TYPE2_IDX_0;
     target_ulong new_val;
 
     switch (tdata_index) {
@@ -309,35 +337,60 @@ static void type2_reg_write(CPURISCVState *env, target_ulong trigger_index,
     return;
 }
 
-typedef target_ulong (*tdata_read_func)(CPURISCVState *env,
-                                        target_ulong trigger_index,
-                                        int tdata_index);
-
-static tdata_read_func trigger_read_funcs[TRIGGER_NUM] = {
-    [TRIGGER_TYPE2_IDX_0 ... TRIGGER_TYPE2_IDX_1] = type2_reg_read,
-};
-
-typedef void (*tdata_write_func)(CPURISCVState *env,
-                                 target_ulong trigger_index,
-                                 int tdata_index,
-                                 target_ulong val);
-
-static tdata_write_func trigger_write_funcs[TRIGGER_NUM] = {
-    [TRIGGER_TYPE2_IDX_0 ... TRIGGER_TYPE2_IDX_1] = type2_reg_write,
-};
-
 target_ulong tdata_csr_read(CPURISCVState *env, int tdata_index)
 {
-    tdata_read_func read_func = trigger_read_funcs[env->trigger_cur];
+    int trigger_type = get_trigger_type(env, env->trigger_cur);
 
-    return read_func(env, env->trigger_cur, tdata_index);
+    switch (trigger_type) {
+    case TRIGGER_TYPE_AD_MATCH:
+        return type2_reg_read(env, env->trigger_cur, tdata_index);
+        break;
+    case TRIGGER_TYPE_INST_CNT:
+    case TRIGGER_TYPE_INT:
+    case TRIGGER_TYPE_EXCP:
+    case TRIGGER_TYPE_AD_MATCH6:
+    case TRIGGER_TYPE_EXT_SRC:
+        qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n",
+                      trigger_type);
+        break;
+    case TRIGGER_TYPE_NO_EXIST:
+    case TRIGGER_TYPE_UNAVAIL:
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    return 0;
 }
 
 void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val)
 {
-    tdata_write_func write_func = trigger_write_funcs[env->trigger_cur];
+    int trigger_type;
+
+    if (tdata_index == TDATA1) {
+        trigger_type = extract_trigger_type(env, val);
+    } else {
+        trigger_type = get_trigger_type(env, env->trigger_cur);
+    }
 
-    return write_func(env, env->trigger_cur, tdata_index, val);
+    switch (trigger_type) {
+    case TRIGGER_TYPE_AD_MATCH:
+        type2_reg_write(env, env->trigger_cur, tdata_index, val);
+        break;
+    case TRIGGER_TYPE_INST_CNT:
+    case TRIGGER_TYPE_INT:
+    case TRIGGER_TYPE_EXCP:
+    case TRIGGER_TYPE_AD_MATCH6:
+    case TRIGGER_TYPE_EXT_SRC:
+        qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n",
+                      trigger_type);
+        break;
+    case TRIGGER_TYPE_NO_EXIST:
+    case TRIGGER_TYPE_UNAVAIL:
+        break;
+    default:
+        g_assert_not_reached();
+    }
 }
 
 void riscv_cpu_debug_excp_handler(CPUState *cs)
@@ -364,18 +417,28 @@ bool riscv_cpu_debug_check_breakpoint(CPUState *cs)
     CPUBreakpoint *bp;
     target_ulong ctrl;
     target_ulong pc;
+    int trigger_type;
     int i;
 
     QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
-        for (i = 0; i < TRIGGER_TYPE2_NUM; i++) {
-            ctrl = env->type2_trig[i].mcontrol;
-            pc = env->type2_trig[i].maddress;
-
-            if ((ctrl & TYPE2_EXEC) && (bp->pc == pc)) {
-                /* check U/S/M bit against current privilege level */
-                if ((ctrl >> 3) & BIT(env->priv)) {
-                    return true;
+        for (i = 0; i < RV_MAX_TRIGGERS; i++) {
+            trigger_type = get_trigger_type(env, i);
+
+            switch (trigger_type) {
+            case TRIGGER_TYPE_AD_MATCH:
+                ctrl = env->type2_trig[i].mcontrol;
+                pc = env->type2_trig[i].maddress;
+
+                if ((ctrl & TYPE2_EXEC) && (bp->pc == pc)) {
+                    /* check U/S/M bit against current privilege level */
+                    if ((ctrl >> 3) & BIT(env->priv)) {
+                        return true;
+                    }
                 }
+                break;
+            default:
+                /* other trigger types are not supported or irrelevant */
+                break;
             }
         }
     }
@@ -389,26 +452,36 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
     CPURISCVState *env = &cpu->env;
     target_ulong ctrl;
     target_ulong addr;
+    int trigger_type;
     int flags;
     int i;
 
-    for (i = 0; i < TRIGGER_TYPE2_NUM; i++) {
-        ctrl = env->type2_trig[i].mcontrol;
-        addr = env->type2_trig[i].maddress;
-        flags = 0;
+    for (i = 0; i < RV_MAX_TRIGGERS; i++) {
+        trigger_type = get_trigger_type(env, i);
 
-        if (ctrl & TYPE2_LOAD) {
-            flags |= BP_MEM_READ;
-        }
-        if (ctrl & TYPE2_STORE) {
-            flags |= BP_MEM_WRITE;
-        }
+        switch (trigger_type) {
+        case TRIGGER_TYPE_AD_MATCH:
+            ctrl = env->type2_trig[i].mcontrol;
+            addr = env->type2_trig[i].maddress;
+            flags = 0;
+
+            if (ctrl & TYPE2_LOAD) {
+                flags |= BP_MEM_READ;
+            }
+            if (ctrl & TYPE2_STORE) {
+                flags |= BP_MEM_WRITE;
+            }
 
-        if ((wp->flags & flags) && (wp->vaddr == addr)) {
-            /* check U/S/M bit against current privilege level */
-            if ((ctrl >> 3) & BIT(env->priv)) {
-                return true;
+            if ((wp->flags & flags) && (wp->vaddr == addr)) {
+                /* check U/S/M bit against current privilege level */
+                if ((ctrl >> 3) & BIT(env->priv)) {
+                    return true;
+                }
             }
+            break;
+        default:
+            /* other trigger types are not supported */
+            break;
         }
     }
 
@@ -417,11 +490,11 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
 
 void riscv_trigger_init(CPURISCVState *env)
 {
-    target_ulong type2 = trigger_type(env, TRIGGER_TYPE_AD_MATCH);
+    target_ulong tdata1 = trigger_type(env, TRIGGER_TYPE_AD_MATCH);
     int i;
 
-    /* type 2 triggers */
-    for (i = 0; i < TRIGGER_TYPE2_NUM; i++) {
+    /* init to type 2 triggers */
+    for (i = 0; i < RV_MAX_TRIGGERS; i++) {
         /*
          * type = TRIGGER_TYPE_AD_MATCH
          * dmode = 0 (both debug and M-mode can write tdata)
@@ -435,7 +508,7 @@ void riscv_trigger_init(CPURISCVState *env)
          * chain = 0 (unimplemented, always 0)
          * match = 0 (always 0, when any compare value equals tdata2)
          */
-        env->type2_trig[i].mcontrol = type2;
+        env->type2_trig[i].mcontrol = tdata1;
         env->type2_trig[i].maddress = 0;
         env->type2_trig[i].bp = NULL;
         env->type2_trig[i].wp = NULL;
diff --git a/target/riscv/debug.h b/target/riscv/debug.h
index 27b9cac6b4..c422553c27 100644
--- a/target/riscv/debug.h
+++ b/target/riscv/debug.h
@@ -22,13 +22,7 @@
 #ifndef RISCV_DEBUG_H
 #define RISCV_DEBUG_H
 
-/* trigger indexes implemented */
-enum {
-    TRIGGER_TYPE2_IDX_0 = 0,
-    TRIGGER_TYPE2_IDX_1,
-    TRIGGER_TYPE2_NUM,
-    TRIGGER_NUM = TRIGGER_TYPE2_NUM
-};
+#define RV_MAX_TRIGGERS         2
 
 /* register index of tdata CSRs */
 enum {
@@ -46,7 +40,8 @@ typedef enum {
     TRIGGER_TYPE_EXCP = 5,          /* exception trigger */
     TRIGGER_TYPE_AD_MATCH6 = 6,     /* new address/data match trigger */
     TRIGGER_TYPE_EXT_SRC = 7,       /* external source trigger */
-    TRIGGER_TYPE_UNAVAIL = 15       /* trigger exists, but unavailable */
+    TRIGGER_TYPE_UNAVAIL = 15,      /* trigger exists, but unavailable */
+    TRIGGER_TYPE_NUM
 } trigger_type_t;
 
 typedef struct {
@@ -56,14 +51,16 @@ typedef struct {
     struct CPUWatchpoint *wp;
 } type2_trigger_t;
 
-/* tdata field masks */
+/* tdata1 field masks */
 
 #define RV32_TYPE(t)    ((uint32_t)(t) << 28)
 #define RV32_TYPE_MASK  (0xf << 28)
 #define RV32_DMODE      BIT(27)
+#define RV32_DATA_MASK  0x7ffffff
 #define RV64_TYPE(t)    ((uint64_t)(t) << 60)
 #define RV64_TYPE_MASK  (0xfULL << 60)
 #define RV64_DMODE      BIT_ULL(59)
+#define RV64_DATA_MASK  0x7ffffffffffffff
 
 /* mcontrol field masks */
 
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index 2a437b29a1..54e523c26c 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -246,7 +246,7 @@ static const VMStateDescription vmstate_debug = {
     .needed = debug_needed,
     .fields = (VMStateField[]) {
         VMSTATE_UINTTL(env.trigger_cur, RISCVCPU),
-        VMSTATE_STRUCT_ARRAY(env.type2_trig, RISCVCPU, TRIGGER_TYPE2_NUM,
+        VMSTATE_STRUCT_ARRAY(env.type2_trig, RISCVCPU, RV_MAX_TRIGGERS,
                              0, vmstate_debug_type2, type2_trigger_t),
         VMSTATE_END_OF_LIST()
     }
-- 
2.36.1

Re: [PATCH 1/9] target/riscv: debug: Determine the trigger type from tdata1.type

[Bin Meng]

On Fri, Jun 10, 2022 at 1:20 PM <frank.chang@sifive.com> wrote:
>
> From: Frank Chang <frank.chang@sifive.com>
>
> Current RISC-V debug assumes that only type 2 trigger is supported.
> To allow more types of triggers to be supported in the future
> (e.g. type 6 trigger, which is similar to type 2 trigger with additional
>  functionality), we should determine the trigger type from tdata1.type.
>
> RV_MAX_TRIGGERS is also introduced in replacement of TRIGGER_TYPE2_NUM.
>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> ---
>  target/riscv/cpu.h     |   2 +-
>  target/riscv/csr.c     |   2 +-
>  target/riscv/debug.c   | 183 ++++++++++++++++++++++++++++-------------
>  target/riscv/debug.h   |  15 ++--
>  target/riscv/machine.c |   2 +-
>  5 files changed, 137 insertions(+), 67 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 7d6397acdf..535123a989 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -289,7 +289,7 @@ struct CPUArchState {
>
>      /* trigger module */
>      target_ulong trigger_cur;
> -    type2_trigger_t type2_trig[TRIGGER_TYPE2_NUM];
> +    type2_trigger_t type2_trig[RV_MAX_TRIGGERS];
>
>      /* machine specific rdtime callback */
>      uint64_t (*rdtime_fn)(void *);
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 6dbe9b541f..005ae31a01 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -2776,7 +2776,7 @@ static RISCVException read_tdata(CPURISCVState *env, int csrno,
>                                   target_ulong *val)
>  {
>      /* return 0 in tdata1 to end the trigger enumeration */
> -    if (env->trigger_cur >= TRIGGER_NUM && csrno == CSR_TDATA1) {
> +    if (env->trigger_cur >= RV_MAX_TRIGGERS && csrno == CSR_TDATA1) {
>          *val = 0;
>          return RISCV_EXCP_NONE;
>      }
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index fc6e13222f..abbcd38a17 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -52,8 +52,15 @@
>  /* tdata availability of a trigger */
>  typedef bool tdata_avail[TDATA_NUM];
>
> -static tdata_avail tdata_mapping[TRIGGER_NUM] = {
> -    [TRIGGER_TYPE2_IDX_0 ... TRIGGER_TYPE2_IDX_1] = { true, true, false },
> +static tdata_avail tdata_mapping[TRIGGER_TYPE_NUM] = {
> +    [TRIGGER_TYPE_NO_EXIST] = { false, false, false },
> +    [TRIGGER_TYPE_AD_MATCH] = { true, true, true },
> +    [TRIGGER_TYPE_INST_CNT] = { true, false, true },
> +    [TRIGGER_TYPE_INT] = { true, true, true },
> +    [TRIGGER_TYPE_EXCP] = { true, true, true },
> +    [TRIGGER_TYPE_AD_MATCH6] = { true, true, true },
> +    [TRIGGER_TYPE_EXT_SRC] = { true, false, false },
> +    [TRIGGER_TYPE_UNAVAIL] = { true, true, true }
>  };
>
>  /* only breakpoint size 1/2/4/8 supported */
> @@ -67,6 +74,26 @@ static int access_size[SIZE_NUM] = {
>      [6 ... 15] = -1,
>  };
>
> +static inline target_ulong extract_trigger_type(CPURISCVState *env,
> +                                                target_ulong tdata1)
> +{
> +    switch (riscv_cpu_mxl(env)) {
> +    case MXL_RV32:
> +        return extract32(tdata1, 28, 4);
> +    case MXL_RV64:
> +        return extract64(tdata1, 60, 4);
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
> +static inline target_ulong get_trigger_type(CPURISCVState *env,
> +                                            target_ulong trigger_index)
> +{
> +    target_ulong tdata1 = env->type2_trig[trigger_index].mcontrol;
> +    return extract_trigger_type(env, tdata1);
> +}
> +
>  static inline target_ulong trigger_type(CPURISCVState *env,
>                                          trigger_type_t type)
>  {
> @@ -89,15 +116,17 @@ static inline target_ulong trigger_type(CPURISCVState *env,
>
>  bool tdata_available(CPURISCVState *env, int tdata_index)
>  {
> +    int trigger_type = get_trigger_type(env, env->trigger_cur);
> +
>      if (unlikely(tdata_index >= TDATA_NUM)) {
>          return false;
>      }
>
> -    if (unlikely(env->trigger_cur >= TRIGGER_NUM)) {
> +    if (unlikely(env->trigger_cur >= RV_MAX_TRIGGERS)) {
>          return false;
>      }
>
> -    return tdata_mapping[env->trigger_cur][tdata_index];
> +    return tdata_mapping[trigger_type][tdata_index];
>  }
>
>  target_ulong tselect_csr_read(CPURISCVState *env)
> @@ -137,6 +166,7 @@ static target_ulong tdata1_validate(CPURISCVState *env, target_ulong val,
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "ignoring type write to tdata1 register\n");
>      }
> +
>      if (dmode != 0) {
>          qemu_log_mask(LOG_UNIMP, "debug mode is not supported\n");
>      }
> @@ -261,9 +291,8 @@ static void type2_breakpoint_remove(CPURISCVState *env, target_ulong index)
>  }
>
>  static target_ulong type2_reg_read(CPURISCVState *env,
> -                                   target_ulong trigger_index, int tdata_index)
> +                                   target_ulong index, int tdata_index)
>  {
> -    uint32_t index = trigger_index - TRIGGER_TYPE2_IDX_0;
>      target_ulong tdata;
>
>      switch (tdata_index) {
> @@ -280,10 +309,9 @@ static target_ulong type2_reg_read(CPURISCVState *env,
>      return tdata;
>  }
>
> -static void type2_reg_write(CPURISCVState *env, target_ulong trigger_index,
> +static void type2_reg_write(CPURISCVState *env, target_ulong index,
>                              int tdata_index, target_ulong val)
>  {
> -    uint32_t index = trigger_index - TRIGGER_TYPE2_IDX_0;
>      target_ulong new_val;
>
>      switch (tdata_index) {
> @@ -309,35 +337,60 @@ static void type2_reg_write(CPURISCVState *env, target_ulong trigger_index,
>      return;
>  }
>
> -typedef target_ulong (*tdata_read_func)(CPURISCVState *env,
> -                                        target_ulong trigger_index,
> -                                        int tdata_index);
> -
> -static tdata_read_func trigger_read_funcs[TRIGGER_NUM] = {
> -    [TRIGGER_TYPE2_IDX_0 ... TRIGGER_TYPE2_IDX_1] = type2_reg_read,
> -};
> -
> -typedef void (*tdata_write_func)(CPURISCVState *env,
> -                                 target_ulong trigger_index,
> -                                 int tdata_index,
> -                                 target_ulong val);
> -
> -static tdata_write_func trigger_write_funcs[TRIGGER_NUM] = {
> -    [TRIGGER_TYPE2_IDX_0 ... TRIGGER_TYPE2_IDX_1] = type2_reg_write,
> -};
> -
>  target_ulong tdata_csr_read(CPURISCVState *env, int tdata_index)
>  {
> -    tdata_read_func read_func = trigger_read_funcs[env->trigger_cur];
> +    int trigger_type = get_trigger_type(env, env->trigger_cur);
>
> -    return read_func(env, env->trigger_cur, tdata_index);
> +    switch (trigger_type) {
> +    case TRIGGER_TYPE_AD_MATCH:
> +        return type2_reg_read(env, env->trigger_cur, tdata_index);
> +        break;
> +    case TRIGGER_TYPE_INST_CNT:
> +    case TRIGGER_TYPE_INT:
> +    case TRIGGER_TYPE_EXCP:
> +    case TRIGGER_TYPE_AD_MATCH6:
> +    case TRIGGER_TYPE_EXT_SRC:
> +        qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n",
> +                      trigger_type);
> +        break;
> +    case TRIGGER_TYPE_NO_EXIST:
> +    case TRIGGER_TYPE_UNAVAIL:
> +        break;

Should we log guest errors for these 2 types?

> +    default:
> +        g_assert_not_reached();
> +    }
> +
> +    return 0;
>  }
>
>  void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val)
>  {
> -    tdata_write_func write_func = trigger_write_funcs[env->trigger_cur];
> +    int trigger_type;
> +
> +    if (tdata_index == TDATA1) {
> +        trigger_type = extract_trigger_type(env, val);
> +    } else {
> +        trigger_type = get_trigger_type(env, env->trigger_cur);
> +    }
>
> -    return write_func(env, env->trigger_cur, tdata_index, val);
> +    switch (trigger_type) {
> +    case TRIGGER_TYPE_AD_MATCH:
> +        type2_reg_write(env, env->trigger_cur, tdata_index, val);
> +        break;
> +    case TRIGGER_TYPE_INST_CNT:
> +    case TRIGGER_TYPE_INT:
> +    case TRIGGER_TYPE_EXCP:
> +    case TRIGGER_TYPE_AD_MATCH6:
> +    case TRIGGER_TYPE_EXT_SRC:
> +        qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n",
> +                      trigger_type);
> +        break;
> +    case TRIGGER_TYPE_NO_EXIST:
> +    case TRIGGER_TYPE_UNAVAIL:
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
>  }
>
>  void riscv_cpu_debug_excp_handler(CPUState *cs)
> @@ -364,18 +417,28 @@ bool riscv_cpu_debug_check_breakpoint(CPUState *cs)
>      CPUBreakpoint *bp;
>      target_ulong ctrl;
>      target_ulong pc;
> +    int trigger_type;
>      int i;
>
>      QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -        for (i = 0; i < TRIGGER_TYPE2_NUM; i++) {
> -            ctrl = env->type2_trig[i].mcontrol;
> -            pc = env->type2_trig[i].maddress;
> -
> -            if ((ctrl & TYPE2_EXEC) && (bp->pc == pc)) {
> -                /* check U/S/M bit against current privilege level */
> -                if ((ctrl >> 3) & BIT(env->priv)) {
> -                    return true;
> +        for (i = 0; i < RV_MAX_TRIGGERS; i++) {
> +            trigger_type = get_trigger_type(env, i);
> +
> +            switch (trigger_type) {
> +            case TRIGGER_TYPE_AD_MATCH:
> +                ctrl = env->type2_trig[i].mcontrol;
> +                pc = env->type2_trig[i].maddress;
> +
> +                if ((ctrl & TYPE2_EXEC) && (bp->pc == pc)) {
> +                    /* check U/S/M bit against current privilege level */
> +                    if ((ctrl >> 3) & BIT(env->priv)) {
> +                        return true;
> +                    }
>                  }
> +                break;
> +            default:
> +                /* other trigger types are not supported or irrelevant */
> +                break;
>              }
>          }
>      }
> @@ -389,26 +452,36 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
>      CPURISCVState *env = &cpu->env;
>      target_ulong ctrl;
>      target_ulong addr;
> +    int trigger_type;
>      int flags;
>      int i;
>
> -    for (i = 0; i < TRIGGER_TYPE2_NUM; i++) {
> -        ctrl = env->type2_trig[i].mcontrol;
> -        addr = env->type2_trig[i].maddress;
> -        flags = 0;
> +    for (i = 0; i < RV_MAX_TRIGGERS; i++) {
> +        trigger_type = get_trigger_type(env, i);
>
> -        if (ctrl & TYPE2_LOAD) {
> -            flags |= BP_MEM_READ;
> -        }
> -        if (ctrl & TYPE2_STORE) {
> -            flags |= BP_MEM_WRITE;
> -        }
> +        switch (trigger_type) {
> +        case TRIGGER_TYPE_AD_MATCH:
> +            ctrl = env->type2_trig[i].mcontrol;
> +            addr = env->type2_trig[i].maddress;
> +            flags = 0;
> +
> +            if (ctrl & TYPE2_LOAD) {
> +                flags |= BP_MEM_READ;
> +            }
> +            if (ctrl & TYPE2_STORE) {
> +                flags |= BP_MEM_WRITE;
> +            }
>
> -        if ((wp->flags & flags) && (wp->vaddr == addr)) {
> -            /* check U/S/M bit against current privilege level */
> -            if ((ctrl >> 3) & BIT(env->priv)) {
> -                return true;
> +            if ((wp->flags & flags) && (wp->vaddr == addr)) {
> +                /* check U/S/M bit against current privilege level */
> +                if ((ctrl >> 3) & BIT(env->priv)) {
> +                    return true;
> +                }
>              }
> +            break;
> +        default:
> +            /* other trigger types are not supported */
> +            break;
>          }
>      }
>
> @@ -417,11 +490,11 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
>
>  void riscv_trigger_init(CPURISCVState *env)
>  {
> -    target_ulong type2 = trigger_type(env, TRIGGER_TYPE_AD_MATCH);
> +    target_ulong tdata1 = trigger_type(env, TRIGGER_TYPE_AD_MATCH);
>      int i;
>
> -    /* type 2 triggers */
> -    for (i = 0; i < TRIGGER_TYPE2_NUM; i++) {
> +    /* init to type 2 triggers */
> +    for (i = 0; i < RV_MAX_TRIGGERS; i++) {
>          /*
>           * type = TRIGGER_TYPE_AD_MATCH
>           * dmode = 0 (both debug and M-mode can write tdata)
> @@ -435,7 +508,7 @@ void riscv_trigger_init(CPURISCVState *env)
>           * chain = 0 (unimplemented, always 0)
>           * match = 0 (always 0, when any compare value equals tdata2)
>           */
> -        env->type2_trig[i].mcontrol = type2;
> +        env->type2_trig[i].mcontrol = tdata1;
>          env->type2_trig[i].maddress = 0;
>          env->type2_trig[i].bp = NULL;
>          env->type2_trig[i].wp = NULL;
> diff --git a/target/riscv/debug.h b/target/riscv/debug.h
> index 27b9cac6b4..c422553c27 100644
> --- a/target/riscv/debug.h
> +++ b/target/riscv/debug.h
> @@ -22,13 +22,7 @@
>  #ifndef RISCV_DEBUG_H
>  #define RISCV_DEBUG_H
>
> -/* trigger indexes implemented */
> -enum {
> -    TRIGGER_TYPE2_IDX_0 = 0,
> -    TRIGGER_TYPE2_IDX_1,
> -    TRIGGER_TYPE2_NUM,
> -    TRIGGER_NUM = TRIGGER_TYPE2_NUM
> -};
> +#define RV_MAX_TRIGGERS         2
>
>  /* register index of tdata CSRs */
>  enum {
> @@ -46,7 +40,8 @@ typedef enum {
>      TRIGGER_TYPE_EXCP = 5,          /* exception trigger */
>      TRIGGER_TYPE_AD_MATCH6 = 6,     /* new address/data match trigger */
>      TRIGGER_TYPE_EXT_SRC = 7,       /* external source trigger */
> -    TRIGGER_TYPE_UNAVAIL = 15       /* trigger exists, but unavailable */
> +    TRIGGER_TYPE_UNAVAIL = 15,      /* trigger exists, but unavailable */
> +    TRIGGER_TYPE_NUM
>  } trigger_type_t;
>
>  typedef struct {
> @@ -56,14 +51,16 @@ typedef struct {
>      struct CPUWatchpoint *wp;
>  } type2_trigger_t;
>
> -/* tdata field masks */
> +/* tdata1 field masks */
>
>  #define RV32_TYPE(t)    ((uint32_t)(t) << 28)
>  #define RV32_TYPE_MASK  (0xf << 28)
>  #define RV32_DMODE      BIT(27)
> +#define RV32_DATA_MASK  0x7ffffff
>  #define RV64_TYPE(t)    ((uint64_t)(t) << 60)
>  #define RV64_TYPE_MASK  (0xfULL << 60)
>  #define RV64_DMODE      BIT_ULL(59)
> +#define RV64_DATA_MASK  0x7ffffffffffffff
>
>  /* mcontrol field masks */
>
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index 2a437b29a1..54e523c26c 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -246,7 +246,7 @@ static const VMStateDescription vmstate_debug = {
>      .needed = debug_needed,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINTTL(env.trigger_cur, RISCVCPU),
> -        VMSTATE_STRUCT_ARRAY(env.type2_trig, RISCVCPU, TRIGGER_TYPE2_NUM,
> +        VMSTATE_STRUCT_ARRAY(env.type2_trig, RISCVCPU, RV_MAX_TRIGGERS,
>                               0, vmstate_debug_type2, type2_trigger_t),
>          VMSTATE_END_OF_LIST()
>      }
> --

Overall LGTM,
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH 1/9] target/riscv: debug: Determine the trigger type from tdata1.type

[Bin Meng]

On Fri, Jun 10, 2022 at 1:20 PM <frank.chang@sifive.com> wrote:
>
> From: Frank Chang <frank.chang@sifive.com>
>
> Current RISC-V debug assumes that only type 2 trigger is supported.
> To allow more types of triggers to be supported in the future
> (e.g. type 6 trigger, which is similar to type 2 trigger with additional
>  functionality), we should determine the trigger type from tdata1.type.
>
> RV_MAX_TRIGGERS is also introduced in replacement of TRIGGER_TYPE2_NUM.
>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> ---
>  target/riscv/cpu.h     |   2 +-
>  target/riscv/csr.c     |   2 +-
>  target/riscv/debug.c   | 183 ++++++++++++++++++++++++++++-------------
>  target/riscv/debug.h   |  15 ++--
>  target/riscv/machine.c |   2 +-
>  5 files changed, 137 insertions(+), 67 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 7d6397acdf..535123a989 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -289,7 +289,7 @@ struct CPUArchState {
>
>      /* trigger module */
>      target_ulong trigger_cur;
> -    type2_trigger_t type2_trig[TRIGGER_TYPE2_NUM];
> +    type2_trigger_t type2_trig[RV_MAX_TRIGGERS];
>
>      /* machine specific rdtime callback */
>      uint64_t (*rdtime_fn)(void *);
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 6dbe9b541f..005ae31a01 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -2776,7 +2776,7 @@ static RISCVException read_tdata(CPURISCVState *env, int csrno,
>                                   target_ulong *val)
>  {
>      /* return 0 in tdata1 to end the trigger enumeration */
> -    if (env->trigger_cur >= TRIGGER_NUM && csrno == CSR_TDATA1) {
> +    if (env->trigger_cur >= RV_MAX_TRIGGERS && csrno == CSR_TDATA1) {
>          *val = 0;
>          return RISCV_EXCP_NONE;
>      }
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index fc6e13222f..abbcd38a17 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -52,8 +52,15 @@
>  /* tdata availability of a trigger */
>  typedef bool tdata_avail[TDATA_NUM];
>
> -static tdata_avail tdata_mapping[TRIGGER_NUM] = {
> -    [TRIGGER_TYPE2_IDX_0 ... TRIGGER_TYPE2_IDX_1] = { true, true, false },
> +static tdata_avail tdata_mapping[TRIGGER_TYPE_NUM] = {
> +    [TRIGGER_TYPE_NO_EXIST] = { false, false, false },
> +    [TRIGGER_TYPE_AD_MATCH] = { true, true, true },
> +    [TRIGGER_TYPE_INST_CNT] = { true, false, true },
> +    [TRIGGER_TYPE_INT] = { true, true, true },
> +    [TRIGGER_TYPE_EXCP] = { true, true, true },
> +    [TRIGGER_TYPE_AD_MATCH6] = { true, true, true },
> +    [TRIGGER_TYPE_EXT_SRC] = { true, false, false },
> +    [TRIGGER_TYPE_UNAVAIL] = { true, true, true }
>  };
>
>  /* only breakpoint size 1/2/4/8 supported */
> @@ -67,6 +74,26 @@ static int access_size[SIZE_NUM] = {
>      [6 ... 15] = -1,
>  };
>
> +static inline target_ulong extract_trigger_type(CPURISCVState *env,
> +                                                target_ulong tdata1)
> +{
> +    switch (riscv_cpu_mxl(env)) {
> +    case MXL_RV32:
> +        return extract32(tdata1, 28, 4);
> +    case MXL_RV64:
> +        return extract64(tdata1, 60, 4);

I guess we should add a "case MXL_RV128" here so that it won't break rv128.
See commit d1d8541217ce8a23e9e751cd868c7d618817134a

> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
> +static inline target_ulong get_trigger_type(CPURISCVState *env,
> +                                            target_ulong trigger_index)
> +{
> +    target_ulong tdata1 = env->type2_trig[trigger_index].mcontrol;
> +    return extract_trigger_type(env, tdata1);
> +}
> +
>  static inline target_ulong trigger_type(CPURISCVState *env,
>                                          trigger_type_t type)
>  {
> @@ -89,15 +116,17 @@ static inline target_ulong trigger_type(CPURISCVState *env,
>
>  bool tdata_available(CPURISCVState *env, int tdata_index)
>  {
> +    int trigger_type = get_trigger_type(env, env->trigger_cur);
> +
>      if (unlikely(tdata_index >= TDATA_NUM)) {
>          return false;
>      }
>
> -    if (unlikely(env->trigger_cur >= TRIGGER_NUM)) {
> +    if (unlikely(env->trigger_cur >= RV_MAX_TRIGGERS)) {
>          return false;
>      }
>
> -    return tdata_mapping[env->trigger_cur][tdata_index];
> +    return tdata_mapping[trigger_type][tdata_index];
>  }
>
>  target_ulong tselect_csr_read(CPURISCVState *env)
> @@ -137,6 +166,7 @@ static target_ulong tdata1_validate(CPURISCVState *env, target_ulong val,
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "ignoring type write to tdata1 register\n");
>      }
> +
>      if (dmode != 0) {
>          qemu_log_mask(LOG_UNIMP, "debug mode is not supported\n");
>      }
> @@ -261,9 +291,8 @@ static void type2_breakpoint_remove(CPURISCVState *env, target_ulong index)
>  }
>
>  static target_ulong type2_reg_read(CPURISCVState *env,
> -                                   target_ulong trigger_index, int tdata_index)
> +                                   target_ulong index, int tdata_index)
>  {
> -    uint32_t index = trigger_index - TRIGGER_TYPE2_IDX_0;
>      target_ulong tdata;
>
>      switch (tdata_index) {
> @@ -280,10 +309,9 @@ static target_ulong type2_reg_read(CPURISCVState *env,
>      return tdata;
>  }
>
> -static void type2_reg_write(CPURISCVState *env, target_ulong trigger_index,
> +static void type2_reg_write(CPURISCVState *env, target_ulong index,
>                              int tdata_index, target_ulong val)
>  {
> -    uint32_t index = trigger_index - TRIGGER_TYPE2_IDX_0;
>      target_ulong new_val;
>
>      switch (tdata_index) {
> @@ -309,35 +337,60 @@ static void type2_reg_write(CPURISCVState *env, target_ulong trigger_index,
>      return;
>  }
>
> -typedef target_ulong (*tdata_read_func)(CPURISCVState *env,
> -                                        target_ulong trigger_index,
> -                                        int tdata_index);
> -
> -static tdata_read_func trigger_read_funcs[TRIGGER_NUM] = {
> -    [TRIGGER_TYPE2_IDX_0 ... TRIGGER_TYPE2_IDX_1] = type2_reg_read,
> -};
> -
> -typedef void (*tdata_write_func)(CPURISCVState *env,
> -                                 target_ulong trigger_index,
> -                                 int tdata_index,
> -                                 target_ulong val);
> -
> -static tdata_write_func trigger_write_funcs[TRIGGER_NUM] = {
> -    [TRIGGER_TYPE2_IDX_0 ... TRIGGER_TYPE2_IDX_1] = type2_reg_write,
> -};
> -
>  target_ulong tdata_csr_read(CPURISCVState *env, int tdata_index)
>  {
> -    tdata_read_func read_func = trigger_read_funcs[env->trigger_cur];
> +    int trigger_type = get_trigger_type(env, env->trigger_cur);
>
> -    return read_func(env, env->trigger_cur, tdata_index);
> +    switch (trigger_type) {
> +    case TRIGGER_TYPE_AD_MATCH:
> +        return type2_reg_read(env, env->trigger_cur, tdata_index);
> +        break;
> +    case TRIGGER_TYPE_INST_CNT:
> +    case TRIGGER_TYPE_INT:
> +    case TRIGGER_TYPE_EXCP:
> +    case TRIGGER_TYPE_AD_MATCH6:
> +    case TRIGGER_TYPE_EXT_SRC:
> +        qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n",
> +                      trigger_type);
> +        break;
> +    case TRIGGER_TYPE_NO_EXIST:
> +    case TRIGGER_TYPE_UNAVAIL:
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +
> +    return 0;
>  }
>
>  void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val)
>  {
> -    tdata_write_func write_func = trigger_write_funcs[env->trigger_cur];
> +    int trigger_type;
> +
> +    if (tdata_index == TDATA1) {
> +        trigger_type = extract_trigger_type(env, val);
> +    } else {
> +        trigger_type = get_trigger_type(env, env->trigger_cur);
> +    }
>
> -    return write_func(env, env->trigger_cur, tdata_index, val);
> +    switch (trigger_type) {
> +    case TRIGGER_TYPE_AD_MATCH:
> +        type2_reg_write(env, env->trigger_cur, tdata_index, val);
> +        break;
> +    case TRIGGER_TYPE_INST_CNT:
> +    case TRIGGER_TYPE_INT:
> +    case TRIGGER_TYPE_EXCP:
> +    case TRIGGER_TYPE_AD_MATCH6:
> +    case TRIGGER_TYPE_EXT_SRC:
> +        qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n",
> +                      trigger_type);
> +        break;
> +    case TRIGGER_TYPE_NO_EXIST:
> +    case TRIGGER_TYPE_UNAVAIL:
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
>  }
>
>  void riscv_cpu_debug_excp_handler(CPUState *cs)
> @@ -364,18 +417,28 @@ bool riscv_cpu_debug_check_breakpoint(CPUState *cs)
>      CPUBreakpoint *bp;
>      target_ulong ctrl;
>      target_ulong pc;
> +    int trigger_type;
>      int i;
>
>      QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -        for (i = 0; i < TRIGGER_TYPE2_NUM; i++) {
> -            ctrl = env->type2_trig[i].mcontrol;
> -            pc = env->type2_trig[i].maddress;
> -
> -            if ((ctrl & TYPE2_EXEC) && (bp->pc == pc)) {
> -                /* check U/S/M bit against current privilege level */
> -                if ((ctrl >> 3) & BIT(env->priv)) {
> -                    return true;
> +        for (i = 0; i < RV_MAX_TRIGGERS; i++) {
> +            trigger_type = get_trigger_type(env, i);
> +
> +            switch (trigger_type) {
> +            case TRIGGER_TYPE_AD_MATCH:
> +                ctrl = env->type2_trig[i].mcontrol;
> +                pc = env->type2_trig[i].maddress;
> +
> +                if ((ctrl & TYPE2_EXEC) && (bp->pc == pc)) {
> +                    /* check U/S/M bit against current privilege level */
> +                    if ((ctrl >> 3) & BIT(env->priv)) {
> +                        return true;
> +                    }
>                  }
> +                break;
> +            default:
> +                /* other trigger types are not supported or irrelevant */
> +                break;
>              }
>          }
>      }
> @@ -389,26 +452,36 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
>      CPURISCVState *env = &cpu->env;
>      target_ulong ctrl;
>      target_ulong addr;
> +    int trigger_type;
>      int flags;
>      int i;
>
> -    for (i = 0; i < TRIGGER_TYPE2_NUM; i++) {
> -        ctrl = env->type2_trig[i].mcontrol;
> -        addr = env->type2_trig[i].maddress;
> -        flags = 0;
> +    for (i = 0; i < RV_MAX_TRIGGERS; i++) {
> +        trigger_type = get_trigger_type(env, i);
>
> -        if (ctrl & TYPE2_LOAD) {
> -            flags |= BP_MEM_READ;
> -        }
> -        if (ctrl & TYPE2_STORE) {
> -            flags |= BP_MEM_WRITE;
> -        }
> +        switch (trigger_type) {
> +        case TRIGGER_TYPE_AD_MATCH:
> +            ctrl = env->type2_trig[i].mcontrol;
> +            addr = env->type2_trig[i].maddress;
> +            flags = 0;
> +
> +            if (ctrl & TYPE2_LOAD) {
> +                flags |= BP_MEM_READ;
> +            }
> +            if (ctrl & TYPE2_STORE) {
> +                flags |= BP_MEM_WRITE;
> +            }
>
> -        if ((wp->flags & flags) && (wp->vaddr == addr)) {
> -            /* check U/S/M bit against current privilege level */
> -            if ((ctrl >> 3) & BIT(env->priv)) {
> -                return true;
> +            if ((wp->flags & flags) && (wp->vaddr == addr)) {
> +                /* check U/S/M bit against current privilege level */
> +                if ((ctrl >> 3) & BIT(env->priv)) {
> +                    return true;
> +                }
>              }
> +            break;
> +        default:
> +            /* other trigger types are not supported */
> +            break;
>          }
>      }
>
> @@ -417,11 +490,11 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
>
>  void riscv_trigger_init(CPURISCVState *env)
>  {
> -    target_ulong type2 = trigger_type(env, TRIGGER_TYPE_AD_MATCH);
> +    target_ulong tdata1 = trigger_type(env, TRIGGER_TYPE_AD_MATCH);
>      int i;
>
> -    /* type 2 triggers */
> -    for (i = 0; i < TRIGGER_TYPE2_NUM; i++) {
> +    /* init to type 2 triggers */
> +    for (i = 0; i < RV_MAX_TRIGGERS; i++) {
>          /*
>           * type = TRIGGER_TYPE_AD_MATCH
>           * dmode = 0 (both debug and M-mode can write tdata)
> @@ -435,7 +508,7 @@ void riscv_trigger_init(CPURISCVState *env)
>           * chain = 0 (unimplemented, always 0)
>           * match = 0 (always 0, when any compare value equals tdata2)
>           */
> -        env->type2_trig[i].mcontrol = type2;
> +        env->type2_trig[i].mcontrol = tdata1;
>          env->type2_trig[i].maddress = 0;
>          env->type2_trig[i].bp = NULL;
>          env->type2_trig[i].wp = NULL;
> diff --git a/target/riscv/debug.h b/target/riscv/debug.h
> index 27b9cac6b4..c422553c27 100644
> --- a/target/riscv/debug.h
> +++ b/target/riscv/debug.h
> @@ -22,13 +22,7 @@
>  #ifndef RISCV_DEBUG_H
>  #define RISCV_DEBUG_H
>
> -/* trigger indexes implemented */
> -enum {
> -    TRIGGER_TYPE2_IDX_0 = 0,
> -    TRIGGER_TYPE2_IDX_1,
> -    TRIGGER_TYPE2_NUM,
> -    TRIGGER_NUM = TRIGGER_TYPE2_NUM
> -};
> +#define RV_MAX_TRIGGERS         2
>
>  /* register index of tdata CSRs */
>  enum {
> @@ -46,7 +40,8 @@ typedef enum {
>      TRIGGER_TYPE_EXCP = 5,          /* exception trigger */
>      TRIGGER_TYPE_AD_MATCH6 = 6,     /* new address/data match trigger */
>      TRIGGER_TYPE_EXT_SRC = 7,       /* external source trigger */
> -    TRIGGER_TYPE_UNAVAIL = 15       /* trigger exists, but unavailable */
> +    TRIGGER_TYPE_UNAVAIL = 15,      /* trigger exists, but unavailable */
> +    TRIGGER_TYPE_NUM
>  } trigger_type_t;
>
>  typedef struct {
> @@ -56,14 +51,16 @@ typedef struct {
>      struct CPUWatchpoint *wp;
>  } type2_trigger_t;
>
> -/* tdata field masks */
> +/* tdata1 field masks */
>
>  #define RV32_TYPE(t)    ((uint32_t)(t) << 28)
>  #define RV32_TYPE_MASK  (0xf << 28)
>  #define RV32_DMODE      BIT(27)
> +#define RV32_DATA_MASK  0x7ffffff
>  #define RV64_TYPE(t)    ((uint64_t)(t) << 60)
>  #define RV64_TYPE_MASK  (0xfULL << 60)
>  #define RV64_DMODE      BIT_ULL(59)
> +#define RV64_DATA_MASK  0x7ffffffffffffff

It looks the 2 macros added here were used in patch2, so please move
the macro definition in patch 2.

>
>  /* mcontrol field masks */
>
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index 2a437b29a1..54e523c26c 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -246,7 +246,7 @@ static const VMStateDescription vmstate_debug = {
>      .needed = debug_needed,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINTTL(env.trigger_cur, RISCVCPU),
> -        VMSTATE_STRUCT_ARRAY(env.type2_trig, RISCVCPU, TRIGGER_TYPE2_NUM,
> +        VMSTATE_STRUCT_ARRAY(env.type2_trig, RISCVCPU, RV_MAX_TRIGGERS,
>                               0, vmstate_debug_type2, type2_trigger_t),
>          VMSTATE_END_OF_LIST()
>      }
> --

Regards,
Bin

[PATCH 2/9] target/riscv: debug: Introduce build_tdata1() to build tdata1 register content

[frank.chang]

From: Frank Chang <frank.chang@sifive.com>

Introduce build_tdata1() to build tdata1 register content, which can be
shared among all types of triggers.

Signed-off-by: Frank Chang <frank.chang@sifive.com>
---
 target/riscv/debug.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index abbcd38a17..089aae0696 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -94,18 +94,23 @@ static inline target_ulong get_trigger_type(CPURISCVState *env,
     return extract_trigger_type(env, tdata1);
 }
 
-static inline target_ulong trigger_type(CPURISCVState *env,
-                                        trigger_type_t type)
+static inline target_ulong build_tdata1(CPURISCVState *env,
+                                        trigger_type_t type,
+                                        bool dmode, target_ulong data)
 {
     target_ulong tdata1;
 
     switch (riscv_cpu_mxl(env)) {
     case MXL_RV32:
-        tdata1 = RV32_TYPE(type);
+        tdata1 = RV32_TYPE(type) |
+                 (dmode ? RV32_DMODE : 0) |
+                 (data & RV32_DATA_MASK);
         break;
     case MXL_RV64:
     case MXL_RV128:
-        tdata1 = RV64_TYPE(type);
+        tdata1 = RV64_TYPE(type) |
+                 (dmode ? RV64_DMODE : 0) |
+                 (data & RV64_DATA_MASK);
         break;
     default:
         g_assert_not_reached();
@@ -490,7 +495,7 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
 
 void riscv_trigger_init(CPURISCVState *env)
 {
-    target_ulong tdata1 = trigger_type(env, TRIGGER_TYPE_AD_MATCH);
+    target_ulong tdata1 = build_tdata1(env, TRIGGER_TYPE_AD_MATCH, 0, 0);
     int i;
 
     /* init to type 2 triggers */
-- 
2.36.1

Re: [PATCH 2/9] target/riscv: debug: Introduce build_tdata1() to build tdata1 register content

[Bin Meng]

On Fri, Jun 10, 2022 at 1:14 PM <frank.chang@sifive.com> wrote:
>
> From: Frank Chang <frank.chang@sifive.com>
>
> Introduce build_tdata1() to build tdata1 register content, which can be
> shared among all types of triggers.
>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> ---
>  target/riscv/debug.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index abbcd38a17..089aae0696 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -94,18 +94,23 @@ static inline target_ulong get_trigger_type(CPURISCVState *env,
>      return extract_trigger_type(env, tdata1);
>  }
>
> -static inline target_ulong trigger_type(CPURISCVState *env,
> -                                        trigger_type_t type)
> +static inline target_ulong build_tdata1(CPURISCVState *env,
> +                                        trigger_type_t type,
> +                                        bool dmode, target_ulong data)
>  {
>      target_ulong tdata1;
>
>      switch (riscv_cpu_mxl(env)) {
>      case MXL_RV32:
> -        tdata1 = RV32_TYPE(type);
> +        tdata1 = RV32_TYPE(type) |
> +                 (dmode ? RV32_DMODE : 0) |
> +                 (data & RV32_DATA_MASK);

RV32_DATA_MASK should be introduced in this patch

>          break;
>      case MXL_RV64:
>      case MXL_RV128:
> -        tdata1 = RV64_TYPE(type);
> +        tdata1 = RV64_TYPE(type) |
> +                 (dmode ? RV64_DMODE : 0) |
> +                 (data & RV64_DATA_MASK);

ditto

>          break;
>      default:
>          g_assert_not_reached();
> @@ -490,7 +495,7 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
>
>  void riscv_trigger_init(CPURISCVState *env)
>  {
> -    target_ulong tdata1 = trigger_type(env, TRIGGER_TYPE_AD_MATCH);
> +    target_ulong tdata1 = build_tdata1(env, TRIGGER_TYPE_AD_MATCH, 0, 0);
>      int i;
>
>      /* init to type 2 triggers */
> --
>

Otherwise,
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 3/9] target/riscv: debug: Introduce tdata1, tdata2, and tdata3 CSRs

[frank.chang]

From: Frank Chang <frank.chang@sifive.com>

Replace type2_trigger_t with the real tdata1, tdata2, and tdata3 CSRs,
which allows us to support more types of triggers in the future.

Signed-off-by: Frank Chang <frank.chang@sifive.com>
---
 target/riscv/cpu.h     |   6 ++-
 target/riscv/debug.c   | 101 ++++++++++++++++-------------------------
 target/riscv/debug.h   |   7 ---
 target/riscv/machine.c |  20 ++------
 4 files changed, 48 insertions(+), 86 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 535123a989..bac5f00722 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -289,7 +289,11 @@ struct CPUArchState {
 
     /* trigger module */
     target_ulong trigger_cur;
-    type2_trigger_t type2_trig[RV_MAX_TRIGGERS];
+    target_ulong tdata1[RV_MAX_TRIGGERS];
+    target_ulong tdata2[RV_MAX_TRIGGERS];
+    target_ulong tdata3[RV_MAX_TRIGGERS];
+    struct CPUBreakpoint *cpu_breakpoint[RV_MAX_TRIGGERS];
+    struct CPUWatchpoint *cpu_watchpoint[RV_MAX_TRIGGERS];
 
     /* machine specific rdtime callback */
     uint64_t (*rdtime_fn)(void *);
diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index 089aae0696..6913682f75 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -90,8 +90,7 @@ static inline target_ulong extract_trigger_type(CPURISCVState *env,
 static inline target_ulong get_trigger_type(CPURISCVState *env,
                                             target_ulong trigger_index)
 {
-    target_ulong tdata1 = env->type2_trig[trigger_index].mcontrol;
-    return extract_trigger_type(env, tdata1);
+    return extract_trigger_type(env, env->tdata1[trigger_index]);
 }
 
 static inline target_ulong build_tdata1(CPURISCVState *env,
@@ -187,6 +186,8 @@ static inline void warn_always_zero_bit(target_ulong val, target_ulong mask,
     }
 }
 
+/* type 2 trigger */
+
 static uint32_t type2_breakpoint_size(CPURISCVState *env, target_ulong ctrl)
 {
     uint32_t size, sizelo, sizehi = 0;
@@ -246,8 +247,8 @@ static target_ulong type2_mcontrol_validate(CPURISCVState *env,
 
 static void type2_breakpoint_insert(CPURISCVState *env, target_ulong index)
 {
-    target_ulong ctrl = env->type2_trig[index].mcontrol;
-    target_ulong addr = env->type2_trig[index].maddress;
+    target_ulong ctrl = env->tdata1[index];
+    target_ulong addr = env->tdata2[index];
     bool enabled = type2_breakpoint_enabled(ctrl);
     CPUState *cs = env_cpu(env);
     int flags = BP_CPU | BP_STOP_BEFORE_ACCESS;
@@ -258,7 +259,7 @@ static void type2_breakpoint_insert(CPURISCVState *env, target_ulong index)
     }
 
     if (ctrl & TYPE2_EXEC) {
-        cpu_breakpoint_insert(cs, addr, flags, &env->type2_trig[index].bp);
+        cpu_breakpoint_insert(cs, addr, flags, &env->cpu_breakpoint[index]);
     }
 
     if (ctrl & TYPE2_LOAD) {
@@ -272,10 +273,10 @@ static void type2_breakpoint_insert(CPURISCVState *env, target_ulong index)
         size = type2_breakpoint_size(env, ctrl);
         if (size != 0) {
             cpu_watchpoint_insert(cs, addr, size, flags,
-                                  &env->type2_trig[index].wp);
+                                  &env->cpu_watchpoint[index]);
         } else {
             cpu_watchpoint_insert(cs, addr, 8, flags,
-                                  &env->type2_trig[index].wp);
+                                  &env->cpu_watchpoint[index]);
         }
     }
 }
@@ -284,34 +285,15 @@ static void type2_breakpoint_remove(CPURISCVState *env, target_ulong index)
 {
     CPUState *cs = env_cpu(env);
 
-    if (env->type2_trig[index].bp) {
-        cpu_breakpoint_remove_by_ref(cs, env->type2_trig[index].bp);
-        env->type2_trig[index].bp = NULL;
+    if (env->cpu_breakpoint[index]) {
+        cpu_breakpoint_remove_by_ref(cs, env->cpu_breakpoint[index]);
+        env->cpu_breakpoint[index] = NULL;
     }
 
-    if (env->type2_trig[index].wp) {
-        cpu_watchpoint_remove_by_ref(cs, env->type2_trig[index].wp);
-        env->type2_trig[index].wp = NULL;
-    }
-}
-
-static target_ulong type2_reg_read(CPURISCVState *env,
-                                   target_ulong index, int tdata_index)
-{
-    target_ulong tdata;
-
-    switch (tdata_index) {
-    case TDATA1:
-        tdata = env->type2_trig[index].mcontrol;
-        break;
-    case TDATA2:
-        tdata = env->type2_trig[index].maddress;
-        break;
-    default:
-        g_assert_not_reached();
+    if (env->cpu_watchpoint[index]) {
+        cpu_watchpoint_remove_by_ref(cs, env->cpu_watchpoint[index]);
+        env->cpu_watchpoint[index] = NULL;
     }
-
-    return tdata;
 }
 
 static void type2_reg_write(CPURISCVState *env, target_ulong index,
@@ -322,19 +304,23 @@ static void type2_reg_write(CPURISCVState *env, target_ulong index,
     switch (tdata_index) {
     case TDATA1:
         new_val = type2_mcontrol_validate(env, val);
-        if (new_val != env->type2_trig[index].mcontrol) {
-            env->type2_trig[index].mcontrol = new_val;
+        if (new_val != env->tdata1[index]) {
+            env->tdata1[index] = new_val;
             type2_breakpoint_remove(env, index);
             type2_breakpoint_insert(env, index);
         }
         break;
     case TDATA2:
-        if (val != env->type2_trig[index].maddress) {
-            env->type2_trig[index].maddress = val;
+        if (val != env->tdata2[index]) {
+            env->tdata2[index] = val;
             type2_breakpoint_remove(env, index);
             type2_breakpoint_insert(env, index);
         }
         break;
+    case TDATA3:
+        qemu_log_mask(LOG_UNIMP,
+                      "tdata3 is not supported for type 2 trigger\n");
+        break;
     default:
         g_assert_not_reached();
     }
@@ -344,28 +330,16 @@ static void type2_reg_write(CPURISCVState *env, target_ulong index,
 
 target_ulong tdata_csr_read(CPURISCVState *env, int tdata_index)
 {
-    int trigger_type = get_trigger_type(env, env->trigger_cur);
-
-    switch (trigger_type) {
-    case TRIGGER_TYPE_AD_MATCH:
-        return type2_reg_read(env, env->trigger_cur, tdata_index);
-        break;
-    case TRIGGER_TYPE_INST_CNT:
-    case TRIGGER_TYPE_INT:
-    case TRIGGER_TYPE_EXCP:
-    case TRIGGER_TYPE_AD_MATCH6:
-    case TRIGGER_TYPE_EXT_SRC:
-        qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n",
-                      trigger_type);
-        break;
-    case TRIGGER_TYPE_NO_EXIST:
-    case TRIGGER_TYPE_UNAVAIL:
-        break;
+    switch (tdata_index) {
+    case TDATA1:
+        return env->tdata1[env->trigger_cur];
+    case TDATA2:
+        return env->tdata2[env->trigger_cur];
+    case TDATA3:
+        return env->tdata3[env->trigger_cur];
     default:
         g_assert_not_reached();
     }
-
-    return 0;
 }
 
 void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val)
@@ -431,8 +405,8 @@ bool riscv_cpu_debug_check_breakpoint(CPUState *cs)
 
             switch (trigger_type) {
             case TRIGGER_TYPE_AD_MATCH:
-                ctrl = env->type2_trig[i].mcontrol;
-                pc = env->type2_trig[i].maddress;
+                ctrl = env->tdata1[i];
+                pc = env->tdata2[i];
 
                 if ((ctrl & TYPE2_EXEC) && (bp->pc == pc)) {
                     /* check U/S/M bit against current privilege level */
@@ -466,8 +440,8 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
 
         switch (trigger_type) {
         case TRIGGER_TYPE_AD_MATCH:
-            ctrl = env->type2_trig[i].mcontrol;
-            addr = env->type2_trig[i].maddress;
+            ctrl = env->tdata1[i];
+            addr = env->tdata2[i];
             flags = 0;
 
             if (ctrl & TYPE2_LOAD) {
@@ -513,9 +487,10 @@ void riscv_trigger_init(CPURISCVState *env)
          * chain = 0 (unimplemented, always 0)
          * match = 0 (always 0, when any compare value equals tdata2)
          */
-        env->type2_trig[i].mcontrol = tdata1;
-        env->type2_trig[i].maddress = 0;
-        env->type2_trig[i].bp = NULL;
-        env->type2_trig[i].wp = NULL;
+        env->tdata1[i] = tdata1;
+        env->tdata2[i] = 0;
+        env->tdata3[i] = 0;
+        env->cpu_breakpoint[i] = NULL;
+        env->cpu_watchpoint[i] = NULL;
     }
 }
diff --git a/target/riscv/debug.h b/target/riscv/debug.h
index c422553c27..76146f373a 100644
--- a/target/riscv/debug.h
+++ b/target/riscv/debug.h
@@ -44,13 +44,6 @@ typedef enum {
     TRIGGER_TYPE_NUM
 } trigger_type_t;
 
-typedef struct {
-    target_ulong mcontrol;
-    target_ulong maddress;
-    struct CPUBreakpoint *bp;
-    struct CPUWatchpoint *wp;
-} type2_trigger_t;
-
 /* tdata1 field masks */
 
 #define RV32_TYPE(t)    ((uint32_t)(t) << 28)
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index 54e523c26c..a1db8b9559 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -228,26 +228,16 @@ static bool debug_needed(void *opaque)
     return riscv_feature(env, RISCV_FEATURE_DEBUG);
 }
 
-static const VMStateDescription vmstate_debug_type2 = {
-    .name = "cpu/debug/type2",
-    .version_id = 1,
-    .minimum_version_id = 1,
-    .fields = (VMStateField[]) {
-        VMSTATE_UINTTL(mcontrol, type2_trigger_t),
-        VMSTATE_UINTTL(maddress, type2_trigger_t),
-        VMSTATE_END_OF_LIST()
-   }
-};
-
 static const VMStateDescription vmstate_debug = {
     .name = "cpu/debug",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .needed = debug_needed,
     .fields = (VMStateField[]) {
         VMSTATE_UINTTL(env.trigger_cur, RISCVCPU),
-        VMSTATE_STRUCT_ARRAY(env.type2_trig, RISCVCPU, RV_MAX_TRIGGERS,
-                             0, vmstate_debug_type2, type2_trigger_t),
+        VMSTATE_UINTTL_ARRAY(env.tdata1, RISCVCPU, RV_MAX_TRIGGERS),
+        VMSTATE_UINTTL_ARRAY(env.tdata2, RISCVCPU, RV_MAX_TRIGGERS),
+        VMSTATE_UINTTL_ARRAY(env.tdata3, RISCVCPU, RV_MAX_TRIGGERS),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.36.1

Re: [PATCH 3/9] target/riscv: debug: Introduce tdata1, tdata2, and tdata3 CSRs

[Bin Meng]

On Fri, Jun 10, 2022 at 1:15 PM <frank.chang@sifive.com> wrote:
>
> From: Frank Chang <frank.chang@sifive.com>
>
> Replace type2_trigger_t with the real tdata1, tdata2, and tdata3 CSRs,
> which allows us to support more types of triggers in the future.
>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> ---
>  target/riscv/cpu.h     |   6 ++-
>  target/riscv/debug.c   | 101 ++++++++++++++++-------------------------
>  target/riscv/debug.h   |   7 ---
>  target/riscv/machine.c |  20 ++------
>  4 files changed, 48 insertions(+), 86 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 535123a989..bac5f00722 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -289,7 +289,11 @@ struct CPUArchState {
>
>      /* trigger module */
>      target_ulong trigger_cur;
> -    type2_trigger_t type2_trig[RV_MAX_TRIGGERS];
> +    target_ulong tdata1[RV_MAX_TRIGGERS];
> +    target_ulong tdata2[RV_MAX_TRIGGERS];
> +    target_ulong tdata3[RV_MAX_TRIGGERS];
> +    struct CPUBreakpoint *cpu_breakpoint[RV_MAX_TRIGGERS];
> +    struct CPUWatchpoint *cpu_watchpoint[RV_MAX_TRIGGERS];

I believe the breakpoint and watchpoint here does not make sense to
every type of trigger. It only makes sense to type 2, 6. Type 3 only
has breakpoint.

>
>      /* machine specific rdtime callback */
>      uint64_t (*rdtime_fn)(void *);
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index 089aae0696..6913682f75 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -90,8 +90,7 @@ static inline target_ulong extract_trigger_type(CPURISCVState *env,
>  static inline target_ulong get_trigger_type(CPURISCVState *env,
>                                              target_ulong trigger_index)
>  {
> -    target_ulong tdata1 = env->type2_trig[trigger_index].mcontrol;
> -    return extract_trigger_type(env, tdata1);
> +    return extract_trigger_type(env, env->tdata1[trigger_index]);
>  }
>
>  static inline target_ulong build_tdata1(CPURISCVState *env,
> @@ -187,6 +186,8 @@ static inline void warn_always_zero_bit(target_ulong val, target_ulong mask,
>      }
>  }
>
> +/* type 2 trigger */
> +
>  static uint32_t type2_breakpoint_size(CPURISCVState *env, target_ulong ctrl)
>  {
>      uint32_t size, sizelo, sizehi = 0;
> @@ -246,8 +247,8 @@ static target_ulong type2_mcontrol_validate(CPURISCVState *env,
>
>  static void type2_breakpoint_insert(CPURISCVState *env, target_ulong index)
>  {
> -    target_ulong ctrl = env->type2_trig[index].mcontrol;
> -    target_ulong addr = env->type2_trig[index].maddress;
> +    target_ulong ctrl = env->tdata1[index];
> +    target_ulong addr = env->tdata2[index];
>      bool enabled = type2_breakpoint_enabled(ctrl);
>      CPUState *cs = env_cpu(env);
>      int flags = BP_CPU | BP_STOP_BEFORE_ACCESS;
> @@ -258,7 +259,7 @@ static void type2_breakpoint_insert(CPURISCVState *env, target_ulong index)
>      }
>
>      if (ctrl & TYPE2_EXEC) {
> -        cpu_breakpoint_insert(cs, addr, flags, &env->type2_trig[index].bp);
> +        cpu_breakpoint_insert(cs, addr, flags, &env->cpu_breakpoint[index]);
>      }
>
>      if (ctrl & TYPE2_LOAD) {
> @@ -272,10 +273,10 @@ static void type2_breakpoint_insert(CPURISCVState *env, target_ulong index)
>          size = type2_breakpoint_size(env, ctrl);
>          if (size != 0) {
>              cpu_watchpoint_insert(cs, addr, size, flags,
> -                                  &env->type2_trig[index].wp);
> +                                  &env->cpu_watchpoint[index]);
>          } else {
>              cpu_watchpoint_insert(cs, addr, 8, flags,
> -                                  &env->type2_trig[index].wp);
> +                                  &env->cpu_watchpoint[index]);
>          }
>      }
>  }
> @@ -284,34 +285,15 @@ static void type2_breakpoint_remove(CPURISCVState *env, target_ulong index)
>  {
>      CPUState *cs = env_cpu(env);
>
> -    if (env->type2_trig[index].bp) {
> -        cpu_breakpoint_remove_by_ref(cs, env->type2_trig[index].bp);
> -        env->type2_trig[index].bp = NULL;
> +    if (env->cpu_breakpoint[index]) {
> +        cpu_breakpoint_remove_by_ref(cs, env->cpu_breakpoint[index]);
> +        env->cpu_breakpoint[index] = NULL;
>      }
>
> -    if (env->type2_trig[index].wp) {
> -        cpu_watchpoint_remove_by_ref(cs, env->type2_trig[index].wp);
> -        env->type2_trig[index].wp = NULL;
> -    }
> -}
> -
> -static target_ulong type2_reg_read(CPURISCVState *env,
> -                                   target_ulong index, int tdata_index)
> -{
> -    target_ulong tdata;
> -
> -    switch (tdata_index) {
> -    case TDATA1:
> -        tdata = env->type2_trig[index].mcontrol;
> -        break;
> -    case TDATA2:
> -        tdata = env->type2_trig[index].maddress;
> -        break;
> -    default:
> -        g_assert_not_reached();
> +    if (env->cpu_watchpoint[index]) {
> +        cpu_watchpoint_remove_by_ref(cs, env->cpu_watchpoint[index]);
> +        env->cpu_watchpoint[index] = NULL;
>      }
> -
> -    return tdata;
>  }
>
>  static void type2_reg_write(CPURISCVState *env, target_ulong index,
> @@ -322,19 +304,23 @@ static void type2_reg_write(CPURISCVState *env, target_ulong index,
>      switch (tdata_index) {
>      case TDATA1:
>          new_val = type2_mcontrol_validate(env, val);
> -        if (new_val != env->type2_trig[index].mcontrol) {
> -            env->type2_trig[index].mcontrol = new_val;
> +        if (new_val != env->tdata1[index]) {
> +            env->tdata1[index] = new_val;
>              type2_breakpoint_remove(env, index);
>              type2_breakpoint_insert(env, index);
>          }
>          break;
>      case TDATA2:
> -        if (val != env->type2_trig[index].maddress) {
> -            env->type2_trig[index].maddress = val;
> +        if (val != env->tdata2[index]) {
> +            env->tdata2[index] = val;
>              type2_breakpoint_remove(env, index);
>              type2_breakpoint_insert(env, index);
>          }
>          break;
> +    case TDATA3:
> +        qemu_log_mask(LOG_UNIMP,
> +                      "tdata3 is not supported for type 2 trigger\n");
> +        break;
>      default:
>          g_assert_not_reached();
>      }
> @@ -344,28 +330,16 @@ static void type2_reg_write(CPURISCVState *env, target_ulong index,
>
>  target_ulong tdata_csr_read(CPURISCVState *env, int tdata_index)
>  {
> -    int trigger_type = get_trigger_type(env, env->trigger_cur);
> -
> -    switch (trigger_type) {
> -    case TRIGGER_TYPE_AD_MATCH:
> -        return type2_reg_read(env, env->trigger_cur, tdata_index);
> -        break;
> -    case TRIGGER_TYPE_INST_CNT:
> -    case TRIGGER_TYPE_INT:
> -    case TRIGGER_TYPE_EXCP:
> -    case TRIGGER_TYPE_AD_MATCH6:
> -    case TRIGGER_TYPE_EXT_SRC:
> -        qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n",
> -                      trigger_type);
> -        break;
> -    case TRIGGER_TYPE_NO_EXIST:
> -    case TRIGGER_TYPE_UNAVAIL:
> -        break;
> +    switch (tdata_index) {
> +    case TDATA1:
> +        return env->tdata1[env->trigger_cur];
> +    case TDATA2:
> +        return env->tdata2[env->trigger_cur];
> +    case TDATA3:
> +        return env->tdata3[env->trigger_cur];
>      default:
>          g_assert_not_reached();
>      }
> -
> -    return 0;
>  }
>
>  void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val)
> @@ -431,8 +405,8 @@ bool riscv_cpu_debug_check_breakpoint(CPUState *cs)
>
>              switch (trigger_type) {
>              case TRIGGER_TYPE_AD_MATCH:
> -                ctrl = env->type2_trig[i].mcontrol;
> -                pc = env->type2_trig[i].maddress;
> +                ctrl = env->tdata1[i];
> +                pc = env->tdata2[i];
>
>                  if ((ctrl & TYPE2_EXEC) && (bp->pc == pc)) {
>                      /* check U/S/M bit against current privilege level */
> @@ -466,8 +440,8 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
>
>          switch (trigger_type) {
>          case TRIGGER_TYPE_AD_MATCH:
> -            ctrl = env->type2_trig[i].mcontrol;
> -            addr = env->type2_trig[i].maddress;
> +            ctrl = env->tdata1[i];
> +            addr = env->tdata2[i];
>              flags = 0;
>
>              if (ctrl & TYPE2_LOAD) {
> @@ -513,9 +487,10 @@ void riscv_trigger_init(CPURISCVState *env)
>           * chain = 0 (unimplemented, always 0)
>           * match = 0 (always 0, when any compare value equals tdata2)
>           */
> -        env->type2_trig[i].mcontrol = tdata1;
> -        env->type2_trig[i].maddress = 0;
> -        env->type2_trig[i].bp = NULL;
> -        env->type2_trig[i].wp = NULL;
> +        env->tdata1[i] = tdata1;
> +        env->tdata2[i] = 0;
> +        env->tdata3[i] = 0;
> +        env->cpu_breakpoint[i] = NULL;
> +        env->cpu_watchpoint[i] = NULL;
>      }
>  }
> diff --git a/target/riscv/debug.h b/target/riscv/debug.h
> index c422553c27..76146f373a 100644
> --- a/target/riscv/debug.h
> +++ b/target/riscv/debug.h
> @@ -44,13 +44,6 @@ typedef enum {
>      TRIGGER_TYPE_NUM
>  } trigger_type_t;
>
> -typedef struct {
> -    target_ulong mcontrol;
> -    target_ulong maddress;
> -    struct CPUBreakpoint *bp;
> -    struct CPUWatchpoint *wp;
> -} type2_trigger_t;
> -
>  /* tdata1 field masks */
>
>  #define RV32_TYPE(t)    ((uint32_t)(t) << 28)
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index 54e523c26c..a1db8b9559 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -228,26 +228,16 @@ static bool debug_needed(void *opaque)
>      return riscv_feature(env, RISCV_FEATURE_DEBUG);
>  }
>
> -static const VMStateDescription vmstate_debug_type2 = {
> -    .name = "cpu/debug/type2",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> -    .fields = (VMStateField[]) {
> -        VMSTATE_UINTTL(mcontrol, type2_trigger_t),
> -        VMSTATE_UINTTL(maddress, type2_trigger_t),
> -        VMSTATE_END_OF_LIST()
> -   }
> -};
> -
>  static const VMStateDescription vmstate_debug = {
>      .name = "cpu/debug",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .needed = debug_needed,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINTTL(env.trigger_cur, RISCVCPU),
> -        VMSTATE_STRUCT_ARRAY(env.type2_trig, RISCVCPU, RV_MAX_TRIGGERS,
> -                             0, vmstate_debug_type2, type2_trigger_t),
> +        VMSTATE_UINTTL_ARRAY(env.tdata1, RISCVCPU, RV_MAX_TRIGGERS),
> +        VMSTATE_UINTTL_ARRAY(env.tdata2, RISCVCPU, RV_MAX_TRIGGERS),
> +        VMSTATE_UINTTL_ARRAY(env.tdata3, RISCVCPU, RV_MAX_TRIGGERS),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> --
>

Regards,
Bin

[PATCH 4/9] target/riscv: debug: Restrict the range of tselect value can be written

[frank.chang]

From: Frank Chang <frank.chang@sifive.com>

The value of tselect CSR can be written should be limited within the
range of supported triggers number.

Signed-off-by: Frank Chang <frank.chang@sifive.com>
---
 target/riscv/debug.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index 6913682f75..296192ffc4 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -126,10 +126,6 @@ bool tdata_available(CPURISCVState *env, int tdata_index)
         return false;
     }
 
-    if (unlikely(env->trigger_cur >= RV_MAX_TRIGGERS)) {
-        return false;
-    }
-
     return tdata_mapping[trigger_type][tdata_index];
 }
 
@@ -140,8 +136,9 @@ target_ulong tselect_csr_read(CPURISCVState *env)
 
 void tselect_csr_write(CPURISCVState *env, target_ulong val)
 {
-    /* all target_ulong bits of tselect are implemented */
-    env->trigger_cur = val;
+    if (val < RV_MAX_TRIGGERS) {
+        env->trigger_cur = val;
+    }
 }
 
 static target_ulong tdata1_validate(CPURISCVState *env, target_ulong val,
-- 
2.36.1

Re: [PATCH 4/9] target/riscv: debug: Restrict the range of tselect value can be written

[Bin Meng]

On Fri, Jun 10, 2022 at 1:14 PM <frank.chang@sifive.com> wrote:
>
> From: Frank Chang <frank.chang@sifive.com>
>
> The value of tselect CSR can be written should be limited within the
> range of supported triggers number.
>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> ---
>  target/riscv/debug.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 5/9] target/riscv: debug: Introduce tinfo CSR

[frank.chang]

From: Frank Chang <frank.chang@sifive.com>

tinfo.info:
  One bit for each possible type enumerated in tdata1.
  If the bit is set, then that type is supported by the currently
  selected trigger.

Signed-off-by: Frank Chang <frank.chang@sifive.com>
---
 target/riscv/cpu_bits.h |  1 +
 target/riscv/csr.c      |  8 ++++++++
 target/riscv/debug.c    | 10 +++++++---
 target/riscv/debug.h    |  2 ++
 4 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 4d04b20d06..666b4d69ca 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -331,6 +331,7 @@
 #define CSR_TDATA1          0x7a1
 #define CSR_TDATA2          0x7a2
 #define CSR_TDATA3          0x7a3
+#define CSR_TINFO           0x7a4
 
 /* Debug Mode Registers */
 #define CSR_DCSR            0x7b0
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 005ae31a01..823b6bd520 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -2800,6 +2800,13 @@ static RISCVException write_tdata(CPURISCVState *env, int csrno,
     return RISCV_EXCP_NONE;
 }
 
+static RISCVException read_tinfo(CPURISCVState *env, int csrno,
+                                 target_ulong *val)
+{
+    *val = tinfo_csr_read(env);
+    return RISCV_EXCP_NONE;
+}
+
 /*
  * Functions to access Pointer Masking feature registers
  * We have to check if current priv lvl could modify
@@ -3588,6 +3595,7 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
     [CSR_TDATA1]    =  { "tdata1",  debug, read_tdata,   write_tdata   },
     [CSR_TDATA2]    =  { "tdata2",  debug, read_tdata,   write_tdata   },
     [CSR_TDATA3]    =  { "tdata3",  debug, read_tdata,   write_tdata   },
+    [CSR_TINFO]     =  { "tinfo",   debug, read_tinfo,   write_ignore  },
 
     /* User Pointer Masking */
     [CSR_UMTE]    =    { "umte",    pointer_masking, read_umte,    write_umte    },
diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index 296192ffc4..1668b8abda 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -37,9 +37,7 @@
  * - tdata1
  * - tdata2
  * - tdata3
- *
- * We don't support writable 'type' field in the tdata1 register, so there is
- * no need to implement the "tinfo" CSR.
+ * - tinfo
  *
  * The following triggers are implemented:
  *
@@ -369,6 +367,12 @@ void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val)
     }
 }
 
+target_ulong tinfo_csr_read(CPURISCVState *env)
+{
+    /* assume all triggers support the same types of triggers */
+    return BIT(TRIGGER_TYPE_AD_MATCH);
+}
+
 void riscv_cpu_debug_excp_handler(CPUState *cs)
 {
     RISCVCPU *cpu = RISCV_CPU(cs);
diff --git a/target/riscv/debug.h b/target/riscv/debug.h
index 76146f373a..9f69c64591 100644
--- a/target/riscv/debug.h
+++ b/target/riscv/debug.h
@@ -95,6 +95,8 @@ void tselect_csr_write(CPURISCVState *env, target_ulong val);
 target_ulong tdata_csr_read(CPURISCVState *env, int tdata_index);
 void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val);
 
+target_ulong tinfo_csr_read(CPURISCVState *env);
+
 void riscv_cpu_debug_excp_handler(CPUState *cs);
 bool riscv_cpu_debug_check_breakpoint(CPUState *cs);
 bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp);
-- 
2.36.1

Re: [PATCH 5/9] target/riscv: debug: Introduce tinfo CSR

[Bin Meng]

On Fri, Jun 10, 2022 at 1:21 PM <frank.chang@sifive.com> wrote:
>
> From: Frank Chang <frank.chang@sifive.com>
>
> tinfo.info:
>   One bit for each possible type enumerated in tdata1.
>   If the bit is set, then that type is supported by the currently
>   selected trigger.
>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> ---
>  target/riscv/cpu_bits.h |  1 +
>  target/riscv/csr.c      |  8 ++++++++
>  target/riscv/debug.c    | 10 +++++++---
>  target/riscv/debug.h    |  2 ++
>  4 files changed, 18 insertions(+), 3 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 6/9] target/riscv: debug: Create common trigger actions function

[frank.chang]

From: Frank Chang <frank.chang@sifive.com>

Trigger actions are shared among all triggers. Extract to a common
function.

Signed-off-by: Frank Chang <frank.chang@sifive.com>
---
 target/riscv/debug.c | 55 ++++++++++++++++++++++++++++++++++++++++++--
 target/riscv/debug.h | 13 +++++++++++
 2 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index 1668b8abda..ab23566113 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -91,6 +91,35 @@ static inline target_ulong get_trigger_type(CPURISCVState *env,
     return extract_trigger_type(env, env->tdata1[trigger_index]);
 }
 
+static trigger_action_t get_trigger_action(CPURISCVState *env,
+                                           target_ulong trigger_index)
+{
+    target_ulong tdata1 = env->tdata1[trigger_index];
+    int trigger_type = get_trigger_type(env, trigger_index);
+    trigger_action_t action = DBG_ACTION_NONE;
+
+    switch (trigger_type) {
+    case TRIGGER_TYPE_AD_MATCH:
+        action = (tdata1 & TYPE2_ACTION) >> 12;
+        break;
+    case TRIGGER_TYPE_INST_CNT:
+    case TRIGGER_TYPE_INT:
+    case TRIGGER_TYPE_EXCP:
+    case TRIGGER_TYPE_AD_MATCH6:
+    case TRIGGER_TYPE_EXT_SRC:
+        qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n",
+                      trigger_type);
+        break;
+    case TRIGGER_TYPE_NO_EXIST:
+    case TRIGGER_TYPE_UNAVAIL:
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    return action;
+}
+
 static inline target_ulong build_tdata1(CPURISCVState *env,
                                         trigger_type_t type,
                                         bool dmode, target_ulong data)
@@ -181,6 +210,28 @@ static inline void warn_always_zero_bit(target_ulong val, target_ulong mask,
     }
 }
 
+static void do_trigger_action(CPURISCVState *env, target_ulong trigger_index)
+{
+    trigger_action_t action = get_trigger_action(env, trigger_index);
+
+    switch (action) {
+    case DBG_ACTION_BP:
+        riscv_raise_exception(env, RISCV_EXCP_BREAKPOINT, 0);
+        break;
+    case DBG_ACTION_DBG_MODE:
+    case DBG_ACTION_TRACE0:
+    case DBG_ACTION_TRACE1:
+    case DBG_ACTION_TRACE2:
+    case DBG_ACTION_TRACE3:
+    case DBG_ACTION_EXT_DBG0:
+    case DBG_ACTION_EXT_DBG1:
+        qemu_log_mask(LOG_UNIMP, "action: %d is not supported\n", action);
+        break;
+    default:
+        g_assert_not_reached();
+    }
+}
+
 /* type 2 trigger */
 
 static uint32_t type2_breakpoint_size(CPURISCVState *env, target_ulong ctrl)
@@ -381,11 +432,11 @@ void riscv_cpu_debug_excp_handler(CPUState *cs)
     if (cs->watchpoint_hit) {
         if (cs->watchpoint_hit->flags & BP_CPU) {
             cs->watchpoint_hit = NULL;
-            riscv_raise_exception(env, RISCV_EXCP_BREAKPOINT, 0);
+            do_trigger_action(env, DBG_ACTION_BP);
         }
     } else {
         if (cpu_breakpoint_test(cs, env->pc, BP_CPU)) {
-            riscv_raise_exception(env, RISCV_EXCP_BREAKPOINT, 0);
+            do_trigger_action(env, DBG_ACTION_BP);
         }
     }
 }
diff --git a/target/riscv/debug.h b/target/riscv/debug.h
index 9f69c64591..0e4859cf74 100644
--- a/target/riscv/debug.h
+++ b/target/riscv/debug.h
@@ -44,6 +44,19 @@ typedef enum {
     TRIGGER_TYPE_NUM
 } trigger_type_t;
 
+/* actions */
+typedef enum {
+    DBG_ACTION_NONE = -1,           /* sentinel value */
+    DBG_ACTION_BP = 0,
+    DBG_ACTION_DBG_MODE,
+    DBG_ACTION_TRACE0,
+    DBG_ACTION_TRACE1,
+    DBG_ACTION_TRACE2,
+    DBG_ACTION_TRACE3,
+    DBG_ACTION_EXT_DBG0 = 8,
+    DBG_ACTION_EXT_DBG1
+} trigger_action_t;
+
 /* tdata1 field masks */
 
 #define RV32_TYPE(t)    ((uint32_t)(t) << 28)
-- 
2.36.1

Re: [PATCH 6/9] target/riscv: debug: Create common trigger actions function

[Bin Meng]

On Fri, Jun 10, 2022 at 1:21 PM <frank.chang@sifive.com> wrote:
>
> From: Frank Chang <frank.chang@sifive.com>
>
> Trigger actions are shared among all triggers. Extract to a common
> function.
>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> ---
>  target/riscv/debug.c | 55 ++++++++++++++++++++++++++++++++++++++++++--
>  target/riscv/debug.h | 13 +++++++++++
>  2 files changed, 66 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index 1668b8abda..ab23566113 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -91,6 +91,35 @@ static inline target_ulong get_trigger_type(CPURISCVState *env,
>      return extract_trigger_type(env, env->tdata1[trigger_index]);
>  }
>
> +static trigger_action_t get_trigger_action(CPURISCVState *env,
> +                                           target_ulong trigger_index)
> +{
> +    target_ulong tdata1 = env->tdata1[trigger_index];
> +    int trigger_type = get_trigger_type(env, trigger_index);
> +    trigger_action_t action = DBG_ACTION_NONE;
> +
> +    switch (trigger_type) {
> +    case TRIGGER_TYPE_AD_MATCH:
> +        action = (tdata1 & TYPE2_ACTION) >> 12;
> +        break;
> +    case TRIGGER_TYPE_INST_CNT:
> +    case TRIGGER_TYPE_INT:
> +    case TRIGGER_TYPE_EXCP:
> +    case TRIGGER_TYPE_AD_MATCH6:
> +    case TRIGGER_TYPE_EXT_SRC:
> +        qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n",
> +                      trigger_type);
> +        break;
> +    case TRIGGER_TYPE_NO_EXIST:
> +    case TRIGGER_TYPE_UNAVAIL:
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +
> +    return action;
> +}
> +
>  static inline target_ulong build_tdata1(CPURISCVState *env,
>                                          trigger_type_t type,
>                                          bool dmode, target_ulong data)
> @@ -181,6 +210,28 @@ static inline void warn_always_zero_bit(target_ulong val, target_ulong mask,
>      }
>  }
>
> +static void do_trigger_action(CPURISCVState *env, target_ulong trigger_index)
> +{
> +    trigger_action_t action = get_trigger_action(env, trigger_index);
> +
> +    switch (action) {
> +    case DBG_ACTION_BP:
> +        riscv_raise_exception(env, RISCV_EXCP_BREAKPOINT, 0);
> +        break;
> +    case DBG_ACTION_DBG_MODE:
> +    case DBG_ACTION_TRACE0:
> +    case DBG_ACTION_TRACE1:
> +    case DBG_ACTION_TRACE2:
> +    case DBG_ACTION_TRACE3:
> +    case DBG_ACTION_EXT_DBG0:
> +    case DBG_ACTION_EXT_DBG1:
> +        qemu_log_mask(LOG_UNIMP, "action: %d is not supported\n", action);
> +        break;

case DBG_ACTION_NONE should be added here as get_trigger_action() may
return such value

> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
>  /* type 2 trigger */
>
>  static uint32_t type2_breakpoint_size(CPURISCVState *env, target_ulong ctrl)
> @@ -381,11 +432,11 @@ void riscv_cpu_debug_excp_handler(CPUState *cs)
>      if (cs->watchpoint_hit) {
>          if (cs->watchpoint_hit->flags & BP_CPU) {
>              cs->watchpoint_hit = NULL;
> -            riscv_raise_exception(env, RISCV_EXCP_BREAKPOINT, 0);
> +            do_trigger_action(env, DBG_ACTION_BP);
>          }
>      } else {
>          if (cpu_breakpoint_test(cs, env->pc, BP_CPU)) {
> -            riscv_raise_exception(env, RISCV_EXCP_BREAKPOINT, 0);
> +            do_trigger_action(env, DBG_ACTION_BP);
>          }
>      }
>  }
> diff --git a/target/riscv/debug.h b/target/riscv/debug.h
> index 9f69c64591..0e4859cf74 100644
> --- a/target/riscv/debug.h
> +++ b/target/riscv/debug.h
> @@ -44,6 +44,19 @@ typedef enum {
>      TRIGGER_TYPE_NUM
>  } trigger_type_t;
>
> +/* actions */
> +typedef enum {
> +    DBG_ACTION_NONE = -1,           /* sentinel value */
> +    DBG_ACTION_BP = 0,
> +    DBG_ACTION_DBG_MODE,
> +    DBG_ACTION_TRACE0,
> +    DBG_ACTION_TRACE1,
> +    DBG_ACTION_TRACE2,
> +    DBG_ACTION_TRACE3,
> +    DBG_ACTION_EXT_DBG0 = 8,
> +    DBG_ACTION_EXT_DBG1
> +} trigger_action_t;
> +
>  /* tdata1 field masks */
>
>  #define RV32_TYPE(t)    ((uint32_t)(t) << 28)

Regards,
Bin

[PATCH 7/9] target/riscv: debug: Check VU/VS modes for type 2 trigger

[frank.chang]

From: Frank Chang <frank.chang@sifive.com>

Type 2 trigger cannot be fired in VU/VS modes.

Signed-off-by: Frank Chang <frank.chang@sifive.com>
---
 target/riscv/debug.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index ab23566113..ce9ff15d75 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -457,6 +457,11 @@ bool riscv_cpu_debug_check_breakpoint(CPUState *cs)
 
             switch (trigger_type) {
             case TRIGGER_TYPE_AD_MATCH:
+                /* type 2 trigger cannot be fired in VU/VS mode */
+                if (riscv_cpu_virt_enabled(env)) {
+                    return false;
+                }
+
                 ctrl = env->tdata1[i];
                 pc = env->tdata2[i];
 
@@ -492,6 +497,11 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
 
         switch (trigger_type) {
         case TRIGGER_TYPE_AD_MATCH:
+            /* type 2 trigger cannot be fired in VU/VS mode */
+            if (riscv_cpu_virt_enabled(env)) {
+                return false;
+            }
+
             ctrl = env->tdata1[i];
             addr = env->tdata2[i];
             flags = 0;
-- 
2.36.1

Re: [PATCH 7/9] target/riscv: debug: Check VU/VS modes for type 2 trigger

[Bin Meng]

On Fri, Jun 10, 2022 at 1:25 PM <frank.chang@sifive.com> wrote:
>
> From: Frank Chang <frank.chang@sifive.com>
>
> Type 2 trigger cannot be fired in VU/VS modes.
>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> ---
>  target/riscv/debug.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 8/9] target/riscv: debug: Return 0 if previous value written to tselect >= number of triggers

[frank.chang]

From: Frank Chang <frank.chang@sifive.com>

If the value written to tselect is greater than or equal to the number
of supported triggers, then the following reads of tselect would return
value 0.

Signed-off-by: Frank Chang <frank.chang@sifive.com>
---
 target/riscv/cpu.h   | 1 +
 target/riscv/debug.c | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index bac5f00722..c7ee3f80e6 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -289,6 +289,7 @@ struct CPUArchState {
 
     /* trigger module */
     target_ulong trigger_cur;
+    target_ulong trigger_prev;
     target_ulong tdata1[RV_MAX_TRIGGERS];
     target_ulong tdata2[RV_MAX_TRIGGERS];
     target_ulong tdata3[RV_MAX_TRIGGERS];
diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index ce9ff15d75..83b72fa1b9 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -158,6 +158,10 @@ bool tdata_available(CPURISCVState *env, int tdata_index)
 
 target_ulong tselect_csr_read(CPURISCVState *env)
 {
+    if (env->trigger_prev >= RV_MAX_TRIGGERS) {
+        return 0;
+    }
+
     return env->trigger_cur;
 }
 
@@ -166,6 +170,8 @@ void tselect_csr_write(CPURISCVState *env, target_ulong val)
     if (val < RV_MAX_TRIGGERS) {
         env->trigger_cur = val;
     }
+
+    env->trigger_prev = val;
 }
 
 static target_ulong tdata1_validate(CPURISCVState *env, target_ulong val,
-- 
2.36.1

Re: [PATCH 8/9] target/riscv: debug: Return 0 if previous value written to tselect >= number of triggers

[Bin Meng]

On Fri, Jun 10, 2022 at 1:24 PM <frank.chang@sifive.com> wrote:
>
> From: Frank Chang <frank.chang@sifive.com>
>
> If the value written to tselect is greater than or equal to the number
> of supported triggers, then the following reads of tselect would return
> value 0.

Where is this behavior documented?

>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> ---
>  target/riscv/cpu.h   | 1 +
>  target/riscv/debug.c | 6 ++++++
>  2 files changed, 7 insertions(+)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index bac5f00722..c7ee3f80e6 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -289,6 +289,7 @@ struct CPUArchState {
>
>      /* trigger module */
>      target_ulong trigger_cur;
> +    target_ulong trigger_prev;
>      target_ulong tdata1[RV_MAX_TRIGGERS];
>      target_ulong tdata2[RV_MAX_TRIGGERS];
>      target_ulong tdata3[RV_MAX_TRIGGERS];
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index ce9ff15d75..83b72fa1b9 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -158,6 +158,10 @@ bool tdata_available(CPURISCVState *env, int tdata_index)
>
>  target_ulong tselect_csr_read(CPURISCVState *env)
>  {
> +    if (env->trigger_prev >= RV_MAX_TRIGGERS) {
> +        return 0;
> +    }
> +
>      return env->trigger_cur;
>  }
>
> @@ -166,6 +170,8 @@ void tselect_csr_write(CPURISCVState *env, target_ulong val)
>      if (val < RV_MAX_TRIGGERS) {
>          env->trigger_cur = val;
>      }
> +
> +    env->trigger_prev = val;
>  }
>
>  static target_ulong tdata1_validate(CPURISCVState *env, target_ulong val,
> --

The spec mentions "implementations which have 2^n triggers only need
to implement n bits of tselect", so in QEMU we can always implement
2^n triggers and have tselect implement just n bit.

In such way, writing tselect can be: env->trigger_cur = val &
(RV_MAX_TRIGGERS - 1).

and I believe you can squash this patch into patch 4 "target/riscv:
debug: Restrict the range of tselect value can be written" because in
patch 4 you changed the actual tselect range while the original
implementation allowed all bits to be set.

Regards,
Bin

[PATCH 9/9] target/riscv: debug: Add initial support of type 6 trigger

[frank.chang]

From: Frank Chang <frank.chang@sifive.com>

Type 6 trigger is similar to a type 2 trigger, but provides additional
functionality and should be used instead of type 2 in newer
implementations.

Signed-off-by: Frank Chang <frank.chang@sifive.com>
---
 target/riscv/debug.c | 174 ++++++++++++++++++++++++++++++++++++++++++-
 target/riscv/debug.h |  18 +++++
 2 files changed, 188 insertions(+), 4 deletions(-)

diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index 83b72fa1b9..43ee10c1c3 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -39,7 +39,7 @@
  * - tdata3
  * - tinfo
  *
- * The following triggers are implemented:
+ * The following triggers are initialized by default:
  *
  * Index | Type |          tdata mapping | Description
  * ------+------+------------------------+------------
@@ -102,10 +102,12 @@ static trigger_action_t get_trigger_action(CPURISCVState *env,
     case TRIGGER_TYPE_AD_MATCH:
         action = (tdata1 & TYPE2_ACTION) >> 12;
         break;
+    case TRIGGER_TYPE_AD_MATCH6:
+        action = (tdata1 & TYPE6_ACTION) >> 12;
+        break;
     case TRIGGER_TYPE_INST_CNT:
     case TRIGGER_TYPE_INT:
     case TRIGGER_TYPE_EXCP:
-    case TRIGGER_TYPE_AD_MATCH6:
     case TRIGGER_TYPE_EXT_SRC:
         qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n",
                       trigger_type);
@@ -380,6 +382,123 @@ static void type2_reg_write(CPURISCVState *env, target_ulong index,
     return;
 }
 
+/* type 6 trigger */
+
+static inline bool type6_breakpoint_enabled(target_ulong ctrl)
+{
+    bool mode = !!(ctrl & (TYPE6_VU | TYPE6_VS | TYPE6_U | TYPE6_S | TYPE6_M));
+    bool rwx = !!(ctrl & (TYPE6_LOAD | TYPE6_STORE | TYPE6_EXEC));
+
+    return mode && rwx;
+}
+
+static target_ulong type6_mcontrol6_validate(CPURISCVState *env,
+                                             target_ulong ctrl)
+{
+    target_ulong val;
+    uint32_t size;
+
+    /* validate the generic part first */
+    val = tdata1_validate(env, ctrl, TRIGGER_TYPE_AD_MATCH6);
+
+    /* validate unimplemented (always zero) bits */
+    warn_always_zero_bit(ctrl, TYPE6_MATCH, "match");
+    warn_always_zero_bit(ctrl, TYPE6_CHAIN, "chain");
+    warn_always_zero_bit(ctrl, TYPE6_ACTION, "action");
+    warn_always_zero_bit(ctrl, TYPE6_TIMING, "timing");
+    warn_always_zero_bit(ctrl, TYPE6_SELECT, "select");
+    warn_always_zero_bit(ctrl, TYPE6_HIT, "hit");
+
+    /* validate size encoding */
+    size = extract32(ctrl, 16, 4);
+    if (access_size[size] == -1) {
+        qemu_log_mask(LOG_UNIMP, "access size %d is not supported, using SIZE_ANY\n",
+                      size);
+    } else {
+        val |= (ctrl & TYPE6_SIZE);
+    }
+
+    /* keep the mode and attribute bits */
+    val |= (ctrl & (TYPE6_VU | TYPE6_VS | TYPE6_U | TYPE6_S | TYPE6_M |
+                    TYPE6_LOAD | TYPE6_STORE | TYPE6_EXEC));
+
+    return val;
+}
+
+static void type6_breakpoint_insert(CPURISCVState *env, target_ulong index)
+{
+    target_ulong ctrl = env->tdata1[index];
+    target_ulong addr = env->tdata2[index];
+    bool enabled = type6_breakpoint_enabled(ctrl);
+    CPUState *cs = env_cpu(env);
+    int flags = BP_CPU | BP_STOP_BEFORE_ACCESS;
+    uint32_t size;
+
+    if (!enabled) {
+        return;
+    }
+
+    if (ctrl & TYPE6_EXEC) {
+        cpu_breakpoint_insert(cs, addr, flags, &env->cpu_breakpoint[index]);
+    }
+
+    if (ctrl & TYPE6_LOAD) {
+        flags |= BP_MEM_READ;
+    }
+
+    if (ctrl & TYPE6_STORE) {
+        flags |= BP_MEM_WRITE;
+    }
+
+    if (flags & BP_MEM_ACCESS) {
+        size = extract32(ctrl, 16, 4);
+        if (size != 0) {
+            cpu_watchpoint_insert(cs, addr, size, flags,
+                                  &env->cpu_watchpoint[index]);
+        } else {
+            cpu_watchpoint_insert(cs, addr, 8, flags,
+                                  &env->cpu_watchpoint[index]);
+        }
+    }
+}
+
+static void type6_breakpoint_remove(CPURISCVState *env, target_ulong index)
+{
+    type2_breakpoint_remove(env, index);
+}
+
+static void type6_reg_write(CPURISCVState *env, target_ulong index,
+                            int tdata_index, target_ulong val)
+{
+    target_ulong new_val;
+
+    switch (tdata_index) {
+    case TDATA1:
+        new_val = type6_mcontrol6_validate(env, val);
+        if (new_val != env->tdata1[index]) {
+            env->tdata1[index] = new_val;
+            type6_breakpoint_remove(env, index);
+            type6_breakpoint_insert(env, index);
+        }
+        break;
+    case TDATA2:
+        if (val != env->tdata2[index]) {
+            env->tdata2[index] = val;
+            type6_breakpoint_remove(env, index);
+            type6_breakpoint_insert(env, index);
+        }
+        break;
+    case TDATA3:
+        qemu_log_mask(LOG_UNIMP,
+                      "tdata3 is not supported for type 6 trigger\n");
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    return;
+}
+
 target_ulong tdata_csr_read(CPURISCVState *env, int tdata_index)
 {
     switch (tdata_index) {
@@ -408,10 +527,12 @@ void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val)
     case TRIGGER_TYPE_AD_MATCH:
         type2_reg_write(env, env->trigger_cur, tdata_index, val);
         break;
+    case TRIGGER_TYPE_AD_MATCH6:
+        type6_reg_write(env, env->trigger_cur, tdata_index, val);
+        break;
     case TRIGGER_TYPE_INST_CNT:
     case TRIGGER_TYPE_INT:
     case TRIGGER_TYPE_EXCP:
-    case TRIGGER_TYPE_AD_MATCH6:
     case TRIGGER_TYPE_EXT_SRC:
         qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n",
                       trigger_type);
@@ -427,7 +548,8 @@ void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val)
 target_ulong tinfo_csr_read(CPURISCVState *env)
 {
     /* assume all triggers support the same types of triggers */
-    return BIT(TRIGGER_TYPE_AD_MATCH);
+    return BIT(TRIGGER_TYPE_AD_MATCH) |
+           BIT(TRIGGER_TYPE_AD_MATCH6);
 }
 
 void riscv_cpu_debug_excp_handler(CPUState *cs)
@@ -478,6 +600,24 @@ bool riscv_cpu_debug_check_breakpoint(CPUState *cs)
                     }
                 }
                 break;
+            case TRIGGER_TYPE_AD_MATCH6:
+                ctrl = env->tdata1[i];
+                pc = env->tdata2[i];
+
+                if ((ctrl & TYPE6_EXEC) && (bp->pc == pc)) {
+                    if (riscv_cpu_virt_enabled(env)) {
+                        /* check VU/VS bit against current privilege level */
+                        if ((ctrl >> 23) & BIT(env->priv)) {
+                            return true;
+                        }
+                    } else {
+                        /* check U/S/M bit against current privilege level */
+                        if ((ctrl >> 3) & BIT(env->priv)) {
+                            return true;
+                        }
+                    }
+                }
+                break;
             default:
                 /* other trigger types are not supported or irrelevant */
                 break;
@@ -526,6 +666,32 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
                 }
             }
             break;
+        case TRIGGER_TYPE_AD_MATCH6:
+            ctrl = env->tdata1[i];
+            addr = env->tdata2[i];
+            flags = 0;
+
+            if (ctrl & TYPE6_LOAD) {
+                flags |= BP_MEM_READ;
+            }
+            if (ctrl & TYPE6_STORE) {
+                flags |= BP_MEM_WRITE;
+            }
+
+            if ((wp->flags & flags) && (wp->vaddr == addr)) {
+                if (riscv_cpu_virt_enabled(env)) {
+                    /* check VU/VS bit against current privilege level */
+                    if ((ctrl >> 23) & BIT(env->priv)) {
+                        return true;
+                    }
+                } else {
+                    /* check U/S/M bit against current privilege level */
+                    if ((ctrl >> 3) & BIT(env->priv)) {
+                        return true;
+                    }
+                }
+            }
+            break;
         default:
             /* other trigger types are not supported */
             break;
diff --git a/target/riscv/debug.h b/target/riscv/debug.h
index 0e4859cf74..a1226b4d29 100644
--- a/target/riscv/debug.h
+++ b/target/riscv/debug.h
@@ -85,6 +85,24 @@ typedef enum {
 #define TYPE2_HIT       BIT(20)
 #define TYPE2_SIZEHI    (0x3 << 21) /* RV64 only */
 
+/* mcontrol6 field masks */
+
+#define TYPE6_LOAD      BIT(0)
+#define TYPE6_STORE     BIT(1)
+#define TYPE6_EXEC      BIT(2)
+#define TYPE6_U         BIT(3)
+#define TYPE6_S         BIT(4)
+#define TYPE6_M         BIT(6)
+#define TYPE6_MATCH     (0xf << 7)
+#define TYPE6_CHAIN     BIT(11)
+#define TYPE6_ACTION    (0xf << 12)
+#define TYPE6_SIZE      (0xf << 16)
+#define TYPE6_TIMING    BIT(20)
+#define TYPE6_SELECT    BIT(21)
+#define TYPE6_HIT       BIT(22)
+#define TYPE6_VU        BIT(23)
+#define TYPE6_VS        BIT(24)
+
 /* access size */
 enum {
     SIZE_ANY = 0,
-- 
2.36.1

Re: [PATCH 9/9] target/riscv: debug: Add initial support of type 6 trigger

[Bin Meng]

On Fri, Jun 10, 2022 at 1:25 PM <frank.chang@sifive.com> wrote:
>
> From: Frank Chang <frank.chang@sifive.com>
>
> Type 6 trigger is similar to a type 2 trigger, but provides additional
> functionality and should be used instead of type 2 in newer
> implementations.
>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> ---
>  target/riscv/debug.c | 174 ++++++++++++++++++++++++++++++++++++++++++-
>  target/riscv/debug.h |  18 +++++
>  2 files changed, 188 insertions(+), 4 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>