All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/8] target/riscv: Improve RISC-V Debug support
@ 2022-09-09 13:42 Bin Meng
  2022-09-09 13:42 ` [PATCH v2 1/8] target/riscv: debug: Determine the trigger type from tdata1.type Bin Meng
                   ` (8 more replies)
  0 siblings, 9 replies; 17+ messages in thread
From: Bin Meng @ 2022-09-09 13:42 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv

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.

This is a v2 respin of previous patch originally done by Frank Chang
at SiFive. I've incorperated my review comments in v2 and rebased
against QEMU mainline.

Changes in v2:
- fixed MXL_RV128 case
- moved macros to patch#2
- added log guest errors for TRIGGER_TYPE_{NO_EXIST,UNAVAIL}
- moved RV{32,64}_DATA_MASK definition to this patch
- add handling of the DBG_ACTION_NONE case in do_trigger_action()
- drop patch: "target/riscv: debug: Return 0 if previous value written to tselect >= number of triggers"

Frank Chang (8):
  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: Add initial support of type 6 trigger

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

-- 
2.34.1



^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 1/8] target/riscv: debug: Determine the trigger type from tdata1.type
  2022-09-09 13:42 [PATCH v2 0/8] target/riscv: Improve RISC-V Debug support Bin Meng
@ 2022-09-09 13:42 ` Bin Meng
  2022-09-16  1:53   ` LIU Zhiwei
  2022-09-16  2:42   ` LIU Zhiwei
  2022-09-09 13:42 ` [PATCH v2 2/8] target/riscv: debug: Introduce build_tdata1() to build tdata1 register content Bin Meng
                   ` (7 subsequent siblings)
  8 siblings, 2 replies; 17+ messages in thread
From: Bin Meng @ 2022-09-09 13:42 UTC (permalink / raw)
  To: qemu-devel
  Cc: Frank Chang, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv

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>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
[bmeng: fixed MXL_RV128 case, and moved macros to the following patch]
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

---

Changes in v2:
- fixed MXL_RV128 case
- moved macros to patch#2
- added log guest errors for TRIGGER_TYPE_{NO_EXIST,UNAVAIL}

 target/riscv/cpu.h     |   2 +-
 target/riscv/debug.h   |  13 +--
 target/riscv/csr.c     |   2 +-
 target/riscv/debug.c   | 188 +++++++++++++++++++++++++++++------------
 target/riscv/machine.c |   2 +-
 5 files changed, 140 insertions(+), 67 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 06751e1e3e..4d82a3250b 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -324,7 +324,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/debug.h b/target/riscv/debug.h
index 27b9cac6b4..72e4edcd8c 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,7 +51,7 @@ 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)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index b96db1b62b..3d0d8e0340 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3065,7 +3065,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..9dd468753a 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,27 @@ 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:
+    case MXL_RV128:
+        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 +117,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 +167,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 +292,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 +310,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 +338,64 @@ 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);
+
+    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:
+        qemu_log_mask(LOG_GUEST_ERROR, "trigger type: %d does not exit\n",
+                      trigger_type);
+        break;
+    default:
+        g_assert_not_reached();
+    }
 
-    return read_func(env, env->trigger_cur, tdata_index);
+    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;
 
-    return write_func(env, env->trigger_cur, tdata_index, val);
+    if (tdata_index == TDATA1) {
+        trigger_type = extract_trigger_type(env, val);
+    } else {
+        trigger_type = get_trigger_type(env, env->trigger_cur);
+    }
+
+    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:
+        qemu_log_mask(LOG_GUEST_ERROR, "trigger type: %d does not exit\n",
+                      trigger_type);
+        break;
+    default:
+        g_assert_not_reached();
+    }
 }
 
 void riscv_cpu_debug_excp_handler(CPUState *cs)
@@ -364,18 +422,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 +457,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 ((wp->flags & flags) && (wp->vaddr == addr)) {
-            /* check U/S/M bit against current privilege level */
-            if ((ctrl >> 3) & BIT(env->priv)) {
-                return true;
+            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;
+                }
             }
+            break;
+        default:
+            /* other trigger types are not supported */
+            break;
         }
     }
 
@@ -417,11 +495,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 +513,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/machine.c b/target/riscv/machine.c
index 41098f6ad0..b8173394a2 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -247,7 +247,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.34.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 2/8] target/riscv: debug: Introduce build_tdata1() to build tdata1 register content
  2022-09-09 13:42 [PATCH v2 0/8] target/riscv: Improve RISC-V Debug support Bin Meng
  2022-09-09 13:42 ` [PATCH v2 1/8] target/riscv: debug: Determine the trigger type from tdata1.type Bin Meng
@ 2022-09-09 13:42 ` Bin Meng
  2022-09-16  1:55   ` LIU Zhiwei
  2022-09-09 13:42 ` [PATCH v2 3/8] target/riscv: debug: Introduce tdata1, tdata2, and tdata3 CSRs Bin Meng
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Bin Meng @ 2022-09-09 13:42 UTC (permalink / raw)
  To: qemu-devel
  Cc: Frank Chang, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv

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>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
[bmeng: moved RV{32,64}_DATA_MASK definition to this patch]
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

---

Changes in v2:
- moved RV{32,64}_DATA_MASK definition to this patch

 target/riscv/debug.h |  2 ++
 target/riscv/debug.c | 15 ++++++++++-----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/target/riscv/debug.h b/target/riscv/debug.h
index 72e4edcd8c..c422553c27 100644
--- a/target/riscv/debug.h
+++ b/target/riscv/debug.h
@@ -56,9 +56,11 @@ typedef struct {
 #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/debug.c b/target/riscv/debug.c
index 9dd468753a..45aae87ec3 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -95,18 +95,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();
@@ -495,7 +500,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.34.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 3/8] target/riscv: debug: Introduce tdata1, tdata2, and tdata3 CSRs
  2022-09-09 13:42 [PATCH v2 0/8] target/riscv: Improve RISC-V Debug support Bin Meng
  2022-09-09 13:42 ` [PATCH v2 1/8] target/riscv: debug: Determine the trigger type from tdata1.type Bin Meng
  2022-09-09 13:42 ` [PATCH v2 2/8] target/riscv: debug: Introduce build_tdata1() to build tdata1 register content Bin Meng
@ 2022-09-09 13:42 ` Bin Meng
  2022-09-16  1:58   ` LIU Zhiwei
  2022-09-09 13:42 ` [PATCH v2 4/8] target/riscv: debug: Restrict the range of tselect value can be written Bin Meng
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Bin Meng @ 2022-09-09 13:42 UTC (permalink / raw)
  To: qemu-devel
  Cc: Frank Chang, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv

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>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

(no changes since v1)

 target/riscv/cpu.h     |   6 ++-
 target/riscv/debug.h   |   7 ---
 target/riscv/debug.c   | 103 +++++++++++++++--------------------------
 target/riscv/machine.c |  20 ++------
 4 files changed, 48 insertions(+), 88 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 4d82a3250b..b0b05c19ca 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -324,7 +324,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.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/debug.c b/target/riscv/debug.c
index 45aae87ec3..06feef7d67 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -91,8 +91,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,
@@ -188,6 +187,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;
@@ -247,8 +248,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;
@@ -259,7 +260,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) {
@@ -273,10 +274,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]);
         }
     }
 }
@@ -285,36 +286,17 @@ 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;
+    if (env->cpu_watchpoint[index]) {
+        cpu_watchpoint_remove_by_ref(cs, env->cpu_watchpoint[index]);
+        env->cpu_watchpoint[index] = 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();
-    }
-
-    return tdata;
-}
-
 static void type2_reg_write(CPURISCVState *env, target_ulong index,
                             int tdata_index, target_ulong val)
 {
@@ -323,19 +305,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();
     }
@@ -345,30 +331,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:
-        qemu_log_mask(LOG_GUEST_ERROR, "trigger type: %d does not exit\n",
-                      trigger_type);
-        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)
@@ -436,8 +408,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 */
@@ -471,8 +443,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) {
@@ -518,9 +490,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/machine.c b/target/riscv/machine.c
index b8173394a2..cb1c4b83b7 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -229,26 +229,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.34.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 4/8] target/riscv: debug: Restrict the range of tselect value can be written
  2022-09-09 13:42 [PATCH v2 0/8] target/riscv: Improve RISC-V Debug support Bin Meng
                   ` (2 preceding siblings ...)
  2022-09-09 13:42 ` [PATCH v2 3/8] target/riscv: debug: Introduce tdata1, tdata2, and tdata3 CSRs Bin Meng
@ 2022-09-09 13:42 ` Bin Meng
  2022-09-16  1:59   ` LIU Zhiwei
  2022-09-09 13:42 ` [PATCH v2 5/8] target/riscv: debug: Introduce tinfo CSR Bin Meng
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Bin Meng @ 2022-09-09 13:42 UTC (permalink / raw)
  To: qemu-devel
  Cc: Frank Chang, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv

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>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

(no changes since v1)

 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 06feef7d67..d6666164cd 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -127,10 +127,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];
 }
 
@@ -141,8 +137,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.34.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 5/8] target/riscv: debug: Introduce tinfo CSR
  2022-09-09 13:42 [PATCH v2 0/8] target/riscv: Improve RISC-V Debug support Bin Meng
                   ` (3 preceding siblings ...)
  2022-09-09 13:42 ` [PATCH v2 4/8] target/riscv: debug: Restrict the range of tselect value can be written Bin Meng
@ 2022-09-09 13:42 ` Bin Meng
  2022-09-16  2:26   ` LIU Zhiwei
  2022-09-09 13:42 ` [PATCH v2 6/8] target/riscv: debug: Create common trigger actions function Bin Meng
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Bin Meng @ 2022-09-09 13:42 UTC (permalink / raw)
  To: qemu-devel
  Cc: Frank Chang, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv

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>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

(no changes since v1)

 target/riscv/cpu_bits.h |  1 +
 target/riscv/debug.h    |  2 ++
 target/riscv/csr.c      |  8 ++++++++
 target/riscv/debug.c    | 10 +++++++---
 4 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 7be12cac2e..1972aee3bb 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -321,6 +321,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/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);
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 3d0d8e0340..e66019048d 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3089,6 +3089,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
@@ -3893,6 +3900,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 d6666164cd..7d546ace42 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:
  *
@@ -372,6 +370,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);
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 6/8] target/riscv: debug: Create common trigger actions function
  2022-09-09 13:42 [PATCH v2 0/8] target/riscv: Improve RISC-V Debug support Bin Meng
                   ` (4 preceding siblings ...)
  2022-09-09 13:42 ` [PATCH v2 5/8] target/riscv: debug: Introduce tinfo CSR Bin Meng
@ 2022-09-09 13:42 ` Bin Meng
  2022-09-16  2:40   ` LIU Zhiwei
  2022-09-09 13:42 ` [PATCH v2 7/8] target/riscv: debug: Check VU/VS modes for type 2 trigger Bin Meng
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Bin Meng @ 2022-09-09 13:42 UTC (permalink / raw)
  To: qemu-devel
  Cc: Frank Chang, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv

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>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
[bmeng: handle the DBG_ACTION_NONE case]
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

---

Changes in v2:
- add handling of the DBG_ACTION_NONE case in do_trigger_action()

 target/riscv/debug.h | 13 ++++++++++
 target/riscv/debug.c | 59 ++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 70 insertions(+), 2 deletions(-)

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)
diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index 7d546ace42..7a8910f980 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -92,6 +92,37 @@ 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:
+        qemu_log_mask(LOG_GUEST_ERROR, "trigger type: %d does not exit\n",
+                      trigger_type);
+        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)
@@ -182,6 +213,30 @@ 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_NONE:
+        break;
+    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)
@@ -384,11 +439,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);
         }
     }
 }
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 7/8] target/riscv: debug: Check VU/VS modes for type 2 trigger
  2022-09-09 13:42 [PATCH v2 0/8] target/riscv: Improve RISC-V Debug support Bin Meng
                   ` (5 preceding siblings ...)
  2022-09-09 13:42 ` [PATCH v2 6/8] target/riscv: debug: Create common trigger actions function Bin Meng
@ 2022-09-09 13:42 ` Bin Meng
  2022-09-09 13:42 ` [PATCH v2 8/8] target/riscv: debug: Add initial support of type 6 trigger Bin Meng
  2022-09-23  4:46 ` [PATCH v2 0/8] target/riscv: Improve RISC-V Debug support Alistair Francis
  8 siblings, 0 replies; 17+ messages in thread
From: Bin Meng @ 2022-09-09 13:42 UTC (permalink / raw)
  To: qemu-devel
  Cc: Frank Chang, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv

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>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

(no changes since v1)

 target/riscv/debug.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index 7a8910f980..e16d5c070a 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -464,6 +464,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];
 
@@ -499,6 +504,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.34.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 8/8] target/riscv: debug: Add initial support of type 6 trigger
  2022-09-09 13:42 [PATCH v2 0/8] target/riscv: Improve RISC-V Debug support Bin Meng
                   ` (6 preceding siblings ...)
  2022-09-09 13:42 ` [PATCH v2 7/8] target/riscv: debug: Check VU/VS modes for type 2 trigger Bin Meng
@ 2022-09-09 13:42 ` Bin Meng
  2022-09-23  4:46 ` [PATCH v2 0/8] target/riscv: Improve RISC-V Debug support Alistair Francis
  8 siblings, 0 replies; 17+ messages in thread
From: Bin Meng @ 2022-09-09 13:42 UTC (permalink / raw)
  To: qemu-devel
  Cc: Frank Chang, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv

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>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

---

(no changes since v1)

 target/riscv/debug.h |  18 +++++
 target/riscv/debug.c | 174 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 188 insertions(+), 4 deletions(-)

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,
diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index e16d5c070a..26ea764407 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
  * ------+------+------------------------+------------
@@ -103,10 +103,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);
@@ -379,6 +381,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) {
@@ -407,10 +526,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);
@@ -428,7 +549,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)
@@ -479,6 +601,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;
@@ -527,6 +667,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;
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/8] target/riscv: debug: Determine the trigger type from tdata1.type
  2022-09-09 13:42 ` [PATCH v2 1/8] target/riscv: debug: Determine the trigger type from tdata1.type Bin Meng
@ 2022-09-16  1:53   ` LIU Zhiwei
  2022-09-16  2:42   ` LIU Zhiwei
  1 sibling, 0 replies; 17+ messages in thread
From: LIU Zhiwei @ 2022-09-16  1:53 UTC (permalink / raw)
  To: Bin Meng, qemu-devel
  Cc: Frank Chang, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

On 2022/9/9 21:42, Bin Meng 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>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> [bmeng: fixed MXL_RV128 case, and moved macros to the following patch]
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> ---
>
> Changes in v2:
> - fixed MXL_RV128 case
> - moved macros to patch#2
> - added log guest errors for TRIGGER_TYPE_{NO_EXIST,UNAVAIL}
>
>   target/riscv/cpu.h     |   2 +-
>   target/riscv/debug.h   |  13 +--
>   target/riscv/csr.c     |   2 +-
>   target/riscv/debug.c   | 188 +++++++++++++++++++++++++++++------------
>   target/riscv/machine.c |   2 +-
>   5 files changed, 140 insertions(+), 67 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 06751e1e3e..4d82a3250b 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -324,7 +324,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/debug.h b/target/riscv/debug.h
> index 27b9cac6b4..72e4edcd8c 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,7 +51,7 @@ 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)
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index b96db1b62b..3d0d8e0340 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3065,7 +3065,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..9dd468753a 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,27 @@ 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:
> +    case MXL_RV128:
> +        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 +117,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 +167,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 +292,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 +310,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 +338,64 @@ 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);
> +
> +    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:
> +        qemu_log_mask(LOG_GUEST_ERROR, "trigger type: %d does not exit\n",
> +                      trigger_type);
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
>   
> -    return read_func(env, env->trigger_cur, tdata_index);
> +    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;
>   
> -    return write_func(env, env->trigger_cur, tdata_index, val);
> +    if (tdata_index == TDATA1) {
> +        trigger_type = extract_trigger_type(env, val);
> +    } else {
> +        trigger_type = get_trigger_type(env, env->trigger_cur);
> +    }
> +
> +    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:
> +        qemu_log_mask(LOG_GUEST_ERROR, "trigger type: %d does not exit\n",
> +                      trigger_type);
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
>   }
>   
>   void riscv_cpu_debug_excp_handler(CPUState *cs)
> @@ -364,18 +422,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 +457,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 ((wp->flags & flags) && (wp->vaddr == addr)) {
> -            /* check U/S/M bit against current privilege level */
> -            if ((ctrl >> 3) & BIT(env->priv)) {
> -                return true;
> +            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;
> +                }
>               }
> +            break;
> +        default:
> +            /* other trigger types are not supported */
> +            break;
>           }
>       }
>   
> @@ -417,11 +495,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 +513,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/machine.c b/target/riscv/machine.c
> index 41098f6ad0..b8173394a2 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -247,7 +247,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()
>       }


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 2/8] target/riscv: debug: Introduce build_tdata1() to build tdata1 register content
  2022-09-09 13:42 ` [PATCH v2 2/8] target/riscv: debug: Introduce build_tdata1() to build tdata1 register content Bin Meng
@ 2022-09-16  1:55   ` LIU Zhiwei
  0 siblings, 0 replies; 17+ messages in thread
From: LIU Zhiwei @ 2022-09-16  1:55 UTC (permalink / raw)
  To: Bin Meng, qemu-devel
  Cc: Frank Chang, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

On 2022/9/9 21:42, Bin Meng 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>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> [bmeng: moved RV{32,64}_DATA_MASK definition to this patch]
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> ---
>
> Changes in v2:
> - moved RV{32,64}_DATA_MASK definition to this patch
>
>   target/riscv/debug.h |  2 ++
>   target/riscv/debug.c | 15 ++++++++++-----
>   2 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/target/riscv/debug.h b/target/riscv/debug.h
> index 72e4edcd8c..c422553c27 100644
> --- a/target/riscv/debug.h
> +++ b/target/riscv/debug.h
> @@ -56,9 +56,11 @@ typedef struct {
>   #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/debug.c b/target/riscv/debug.c
> index 9dd468753a..45aae87ec3 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -95,18 +95,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();
> @@ -495,7 +500,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 */


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 3/8] target/riscv: debug: Introduce tdata1, tdata2, and tdata3 CSRs
  2022-09-09 13:42 ` [PATCH v2 3/8] target/riscv: debug: Introduce tdata1, tdata2, and tdata3 CSRs Bin Meng
@ 2022-09-16  1:58   ` LIU Zhiwei
  0 siblings, 0 replies; 17+ messages in thread
From: LIU Zhiwei @ 2022-09-16  1:58 UTC (permalink / raw)
  To: Bin Meng, qemu-devel
  Cc: Frank Chang, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv


On 2022/9/9 21:42, Bin Meng 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>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
> (no changes since v1)
>
>   target/riscv/cpu.h     |   6 ++-
>   target/riscv/debug.h   |   7 ---
>   target/riscv/debug.c   | 103 +++++++++++++++--------------------------
>   target/riscv/machine.c |  20 ++------
>   4 files changed, 48 insertions(+), 88 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 4d82a3250b..b0b05c19ca 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -324,7 +324,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];

These fields should not packed into env.  As it had already existed  
before this patch set,

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>   
>       /* machine specific rdtime callback */
>       uint64_t (*rdtime_fn)(void *);
> 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/debug.c b/target/riscv/debug.c
> index 45aae87ec3..06feef7d67 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -91,8 +91,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,
> @@ -188,6 +187,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;
> @@ -247,8 +248,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;
> @@ -259,7 +260,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) {
> @@ -273,10 +274,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]);
>           }
>       }
>   }
> @@ -285,36 +286,17 @@ 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;
> +    if (env->cpu_watchpoint[index]) {
> +        cpu_watchpoint_remove_by_ref(cs, env->cpu_watchpoint[index]);
> +        env->cpu_watchpoint[index] = 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();
> -    }
> -
> -    return tdata;
> -}
> -
>   static void type2_reg_write(CPURISCVState *env, target_ulong index,
>                               int tdata_index, target_ulong val)
>   {
> @@ -323,19 +305,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();
>       }
> @@ -345,30 +331,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:
> -        qemu_log_mask(LOG_GUEST_ERROR, "trigger type: %d does not exit\n",
> -                      trigger_type);
> -        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)
> @@ -436,8 +408,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 */
> @@ -471,8 +443,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) {
> @@ -518,9 +490,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/machine.c b/target/riscv/machine.c
> index b8173394a2..cb1c4b83b7 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -229,26 +229,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()
>       }
>   };


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 4/8] target/riscv: debug: Restrict the range of tselect value can be written
  2022-09-09 13:42 ` [PATCH v2 4/8] target/riscv: debug: Restrict the range of tselect value can be written Bin Meng
@ 2022-09-16  1:59   ` LIU Zhiwei
  0 siblings, 0 replies; 17+ messages in thread
From: LIU Zhiwei @ 2022-09-16  1:59 UTC (permalink / raw)
  To: Bin Meng, qemu-devel
  Cc: Frank Chang, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

On 2022/9/9 21:42, Bin Meng 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>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
> (no changes since v1)
>
>   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 06feef7d67..d6666164cd 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -127,10 +127,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];
>   }
>   
> @@ -141,8 +137,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,


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 5/8] target/riscv: debug: Introduce tinfo CSR
  2022-09-09 13:42 ` [PATCH v2 5/8] target/riscv: debug: Introduce tinfo CSR Bin Meng
@ 2022-09-16  2:26   ` LIU Zhiwei
  0 siblings, 0 replies; 17+ messages in thread
From: LIU Zhiwei @ 2022-09-16  2:26 UTC (permalink / raw)
  To: Bin Meng, qemu-devel
  Cc: Frank Chang, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

On 2022/9/9 21:42, Bin Meng 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>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
> (no changes since v1)
>
>   target/riscv/cpu_bits.h |  1 +
>   target/riscv/debug.h    |  2 ++
>   target/riscv/csr.c      |  8 ++++++++
>   target/riscv/debug.c    | 10 +++++++---
>   4 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 7be12cac2e..1972aee3bb 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -321,6 +321,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/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);
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 3d0d8e0340..e66019048d 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3089,6 +3089,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
> @@ -3893,6 +3900,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 d6666164cd..7d546ace42 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:
>    *
> @@ -372,6 +370,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);


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 6/8] target/riscv: debug: Create common trigger actions function
  2022-09-09 13:42 ` [PATCH v2 6/8] target/riscv: debug: Create common trigger actions function Bin Meng
@ 2022-09-16  2:40   ` LIU Zhiwei
  0 siblings, 0 replies; 17+ messages in thread
From: LIU Zhiwei @ 2022-09-16  2:40 UTC (permalink / raw)
  To: Bin Meng, qemu-devel
  Cc: Frank Chang, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

On 2022/9/9 21:42, Bin Meng 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>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> [bmeng: handle the DBG_ACTION_NONE case]
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> ---
>
> Changes in v2:
> - add handling of the DBG_ACTION_NONE case in do_trigger_action()
>
>   target/riscv/debug.h | 13 ++++++++++
>   target/riscv/debug.c | 59 ++++++++++++++++++++++++++++++++++++++++++--
>   2 files changed, 70 insertions(+), 2 deletions(-)
>
> 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)
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index 7d546ace42..7a8910f980 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -92,6 +92,37 @@ 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:
> +        qemu_log_mask(LOG_GUEST_ERROR, "trigger type: %d does not exit\n",
> +                      trigger_type);
> +        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)
> @@ -182,6 +213,30 @@ 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_NONE:
> +        break;
> +    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)
> @@ -384,11 +439,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);
>           }
>       }
>   }


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/8] target/riscv: debug: Determine the trigger type from tdata1.type
  2022-09-09 13:42 ` [PATCH v2 1/8] target/riscv: debug: Determine the trigger type from tdata1.type Bin Meng
  2022-09-16  1:53   ` LIU Zhiwei
@ 2022-09-16  2:42   ` LIU Zhiwei
  1 sibling, 0 replies; 17+ messages in thread
From: LIU Zhiwei @ 2022-09-16  2:42 UTC (permalink / raw)
  To: Bin Meng, qemu-devel
  Cc: Frank Chang, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv


On 2022/9/9 21:42, Bin Meng 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>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> [bmeng: fixed MXL_RV128 case, and moved macros to the following patch]
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> ---
>
> Changes in v2:
> - fixed MXL_RV128 case
> - moved macros to patch#2
> - added log guest errors for TRIGGER_TYPE_{NO_EXIST,UNAVAIL}
>
>   target/riscv/cpu.h     |   2 +-
>   target/riscv/debug.h   |  13 +--
>   target/riscv/csr.c     |   2 +-
>   target/riscv/debug.c   | 188 +++++++++++++++++++++++++++++------------
>   target/riscv/machine.c |   2 +-
>   5 files changed, 140 insertions(+), 67 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 06751e1e3e..4d82a3250b 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -324,7 +324,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/debug.h b/target/riscv/debug.h
> index 27b9cac6b4..72e4edcd8c 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,7 +51,7 @@ 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)
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index b96db1b62b..3d0d8e0340 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3065,7 +3065,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..9dd468753a 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,27 @@ 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:
> +    case MXL_RV128:
> +        return extract64(tdata1, 60, 4);
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +

Maybe

get_type_from_tdata

> +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);
> +}
> +

and

get_type_from_index

>   static inline target_ulong trigger_type(CPURISCVState *env,
>                                           trigger_type_t type)
>   {
> @@ -89,15 +117,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 +167,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 +292,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 +310,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 +338,64 @@ 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);
> +
> +    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:
> +        qemu_log_mask(LOG_GUEST_ERROR, "trigger type: %d does not exit\n",
> +                      trigger_type);
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
>   
> -    return read_func(env, env->trigger_cur, tdata_index);
> +    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;
>   
> -    return write_func(env, env->trigger_cur, tdata_index, val);
> +    if (tdata_index == TDATA1) {
> +        trigger_type = extract_trigger_type(env, val);
> +    } else {
> +        trigger_type = get_trigger_type(env, env->trigger_cur);
> +    }
> +
> +    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:
> +        qemu_log_mask(LOG_GUEST_ERROR, "trigger type: %d does not exit\n",
> +                      trigger_type);
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
>   }
>   
>   void riscv_cpu_debug_excp_handler(CPUState *cs)
> @@ -364,18 +422,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 +457,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 ((wp->flags & flags) && (wp->vaddr == addr)) {
> -            /* check U/S/M bit against current privilege level */
> -            if ((ctrl >> 3) & BIT(env->priv)) {
> -                return true;
> +            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;
> +                }
>               }
> +            break;
> +        default:
> +            /* other trigger types are not supported */
> +            break;
>           }
>       }
>   
> @@ -417,11 +495,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 +513,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/machine.c b/target/riscv/machine.c
> index 41098f6ad0..b8173394a2 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -247,7 +247,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()
>       }


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 0/8] target/riscv: Improve RISC-V Debug support
  2022-09-09 13:42 [PATCH v2 0/8] target/riscv: Improve RISC-V Debug support Bin Meng
                   ` (7 preceding siblings ...)
  2022-09-09 13:42 ` [PATCH v2 8/8] target/riscv: debug: Add initial support of type 6 trigger Bin Meng
@ 2022-09-23  4:46 ` Alistair Francis
  8 siblings, 0 replies; 17+ messages in thread
From: Alistair Francis @ 2022-09-23  4:46 UTC (permalink / raw)
  To: Bin Meng
  Cc: qemu-devel@nongnu.org Developers, Alistair Francis, Bin Meng,
	Palmer Dabbelt, open list:RISC-V

On Fri, Sep 9, 2022 at 11:48 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> 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.
>
> This is a v2 respin of previous patch originally done by Frank Chang
> at SiFive. I've incorperated my review comments in v2 and rebased
> against QEMU mainline.
>
> Changes in v2:
> - fixed MXL_RV128 case
> - moved macros to patch#2
> - added log guest errors for TRIGGER_TYPE_{NO_EXIST,UNAVAIL}
> - moved RV{32,64}_DATA_MASK definition to this patch
> - add handling of the DBG_ACTION_NONE case in do_trigger_action()
> - drop patch: "target/riscv: debug: Return 0 if previous value written to tselect >= number of triggers"
>
> Frank Chang (8):
>   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: Add initial support of type 6 trigger

Thanks!

Applied to riscv-to-apply.next

Alistair

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2022-09-23  4:49 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-09 13:42 [PATCH v2 0/8] target/riscv: Improve RISC-V Debug support Bin Meng
2022-09-09 13:42 ` [PATCH v2 1/8] target/riscv: debug: Determine the trigger type from tdata1.type Bin Meng
2022-09-16  1:53   ` LIU Zhiwei
2022-09-16  2:42   ` LIU Zhiwei
2022-09-09 13:42 ` [PATCH v2 2/8] target/riscv: debug: Introduce build_tdata1() to build tdata1 register content Bin Meng
2022-09-16  1:55   ` LIU Zhiwei
2022-09-09 13:42 ` [PATCH v2 3/8] target/riscv: debug: Introduce tdata1, tdata2, and tdata3 CSRs Bin Meng
2022-09-16  1:58   ` LIU Zhiwei
2022-09-09 13:42 ` [PATCH v2 4/8] target/riscv: debug: Restrict the range of tselect value can be written Bin Meng
2022-09-16  1:59   ` LIU Zhiwei
2022-09-09 13:42 ` [PATCH v2 5/8] target/riscv: debug: Introduce tinfo CSR Bin Meng
2022-09-16  2:26   ` LIU Zhiwei
2022-09-09 13:42 ` [PATCH v2 6/8] target/riscv: debug: Create common trigger actions function Bin Meng
2022-09-16  2:40   ` LIU Zhiwei
2022-09-09 13:42 ` [PATCH v2 7/8] target/riscv: debug: Check VU/VS modes for type 2 trigger Bin Meng
2022-09-09 13:42 ` [PATCH v2 8/8] target/riscv: debug: Add initial support of type 6 trigger Bin Meng
2022-09-23  4:46 ` [PATCH v2 0/8] target/riscv: Improve RISC-V Debug support Alistair Francis

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.