[PATCH v1 0/4] Support native debug icount trigger

[PATCH v1 0/4] Support native debug icount trigger

[LIU Zhiwei]

icount trigger set an instruction count. After one instruction retired,
the count will be decreased by 1. If the count decreased to 0, the icount
trigger will fire.

icount trigger is needed by single step ptrace system call and the native
GDB.

In this patch set, change the translation when icount trigger enabled in the
way that instruction executes one by one. After executing one instruction,
call a helper function to decrease the count for itrigger.


It also provides an accelebrated way. If QEMU executes with -icount parameter,
itrigger is simulated by a timer with the count in itrigger as the deadline.

Note the count in itrigger will only decrease when the priviledge matches, which
is also processed in this patch set.


After merging this patch set, QEMU will support type2/type6 trigger(needed by
breakpoint and watchpoint) and icount trigger(needed by single step),
which is enough for a PoC of native debug. 

LIU Zhiwei (4):
  target/riscv: Add itrigger support when icount is not enabled
  target/riscv: Add itrigger support when icount is enabled
  target/riscv: Enable native debug itrigger
  target/riscv: Add itrigger_enabled field to CPURISCVState

 target/riscv/cpu.h                            |   5 +
 target/riscv/cpu_helper.c                     |   8 +
 target/riscv/debug.c                          | 205 ++++++++++++++++++
 target/riscv/debug.h                          |  13 ++
 target/riscv/helper.h                         |   2 +
 .../riscv/insn_trans/trans_privileged.c.inc   |   4 +-
 target/riscv/insn_trans/trans_rvi.c.inc       |   8 +-
 target/riscv/insn_trans/trans_rvv.c.inc       |   4 +-
 target/riscv/machine.c                        |  15 ++
 target/riscv/translate.c                      |  33 ++-
 10 files changed, 286 insertions(+), 11 deletions(-)

-- 
2.17.1

[PATCH v1 1/4] target/riscv: Add itrigger support when icount is not enabled

[LIU Zhiwei]

When icount is not enabled, there is no API in QEMU that can get the
guest instruction number.

Translate the guest code in a way that each TB only has one instruction.
After executing the instruction, decrease the count by 1 until it reaches 0
where the itrigger fires.

Note that only when priviledge matches the itrigger configuration,
the count will decrease.

Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
 target/riscv/cpu.h                            |  2 +
 target/riscv/cpu_helper.c                     |  6 ++
 target/riscv/debug.c                          | 71 +++++++++++++++++++
 target/riscv/debug.h                          | 12 ++++
 target/riscv/helper.h                         |  2 +
 .../riscv/insn_trans/trans_privileged.c.inc   |  4 +-
 target/riscv/insn_trans/trans_rvi.c.inc       |  8 +--
 target/riscv/insn_trans/trans_rvv.c.inc       |  4 +-
 target/riscv/translate.c                      | 33 ++++++++-
 9 files changed, 131 insertions(+), 11 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index b131fa8c8e..24bafda27d 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -621,6 +621,8 @@ FIELD(TB_FLAGS, PM_MASK_ENABLED, 22, 1)
 FIELD(TB_FLAGS, PM_BASE_ENABLED, 23, 1)
 FIELD(TB_FLAGS, VTA, 24, 1)
 FIELD(TB_FLAGS, VMA, 25, 1)
+/* Native debug itrigger */
+FIELD(TB_FLAGS, ITRIGGER, 26, 1)
 
 #ifdef TARGET_RISCV32
 #define riscv_cpu_mxl(env)  ((void)(env), MXL_RV32)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 278d163803..263282f230 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -27,7 +27,9 @@
 #include "tcg/tcg-op.h"
 #include "trace.h"
 #include "semihosting/common-semi.h"
+#include "sysemu/cpu-timers.h"
 #include "cpu_bits.h"
+#include "debug.h"
 
 int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
 {
@@ -103,6 +105,10 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
         flags = FIELD_DP32(flags, TB_FLAGS, MSTATUS_HS_VS,
                            get_field(env->mstatus_hs, MSTATUS_VS));
     }
+    if (riscv_feature(env, RISCV_FEATURE_DEBUG) && !icount_enabled()) {
+        flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER,
+                           riscv_itrigger_enabled(env));
+    }
 #endif
 
     flags = FIELD_DP32(flags, TB_FLAGS, XL, env->xl);
diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index 26ea764407..45a3537d5c 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -29,6 +29,7 @@
 #include "cpu.h"
 #include "trace.h"
 #include "exec/exec-all.h"
+#include "exec/helper-proto.h"
 
 /*
  * The following M-mode trigger CSRs are implemented:
@@ -498,6 +499,76 @@ static void type6_reg_write(CPURISCVState *env, target_ulong index,
     return;
 }
 
+/* icount trigger type */
+static inline int
+itrigger_get_count(CPURISCVState *env, int index)
+{
+    return get_field(env->tdata1[index], ITRIGGER_COUNT);
+}
+
+static inline void
+itrigger_set_count(CPURISCVState *env, int index, int value)
+{
+    env->tdata1[index] = set_field(env->tdata1[index],
+                                   ITRIGGER_COUNT, value);
+}
+
+static bool check_itrigger_priv(CPURISCVState *env, int index)
+{
+    target_ulong tdata1 = env->tdata1[index];
+    if (riscv_cpu_virt_enabled(env)) {
+        /* check VU/VS bit against current privilege level */
+        return (get_field(tdata1, ITRIGGER_VS) == env->priv) ||
+               (get_field(tdata1, ITRIGGER_VU) == env->priv);
+    } else {
+        /* check U/S/M bit against current privilege level */
+        return (get_field(tdata1, ITRIGGER_M) == env->priv) ||
+               (get_field(tdata1, ITRIGGER_S) == env->priv) ||
+               (get_field(tdata1, ITRIGGER_U) == env->priv);
+    }
+}
+
+bool riscv_itrigger_enabled(CPURISCVState *env)
+{
+    int count;
+    for (int i = 0; i < RV_MAX_TRIGGERS; i++) {
+        if (get_trigger_type(env, i) != TRIGGER_TYPE_INST_CNT) {
+            continue;
+        }
+        if (check_itrigger_priv(env, i)) {
+            continue;
+        }
+        count = itrigger_get_count(env, i);
+        if (!count) {
+            continue;
+        }
+        return true;
+    }
+
+    return false;
+}
+
+void helper_itrigger_match(CPURISCVState *env)
+{
+    int count;
+    for (int i = 0; i < RV_MAX_TRIGGERS; i++) {
+        if (get_trigger_type(env, i) != TRIGGER_TYPE_INST_CNT) {
+            continue;
+        }
+        if (check_itrigger_priv(env, i)) {
+            continue;
+        }
+        count = itrigger_get_count(env, i);
+        if (!count) {
+            continue;
+        }
+        itrigger_set_count(env, i, count--);
+        if (!count) {
+            do_trigger_action(env, i);
+        }
+    }
+}
+
 target_ulong tdata_csr_read(CPURISCVState *env, int tdata_index)
 {
     switch (tdata_index) {
diff --git a/target/riscv/debug.h b/target/riscv/debug.h
index a1226b4d29..cc3358e69b 100644
--- a/target/riscv/debug.h
+++ b/target/riscv/debug.h
@@ -118,6 +118,17 @@ enum {
     SIZE_NUM = 16
 };
 
+/* itrigger filed masks */
+#define ITRIGGER_ACTION       0x3f
+#define ITRIGGER_U            BIT(6)
+#define ITRIGGER_S            BIT(7)
+#define ITRIGGER_PENDING      BIT(8)
+#define ITRIGGER_M            BIT(9)
+#define ITRIGGER_COUNT        (0x3fff << 10)
+#define ITRIGGER_HIT          BIT(24)
+#define ITRIGGER_VU           BIT(25)
+#define ITRIGGER_VS           BIT(26)
+
 bool tdata_available(CPURISCVState *env, int tdata_index);
 
 target_ulong tselect_csr_read(CPURISCVState *env);
@@ -134,4 +145,5 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp);
 
 void riscv_trigger_init(CPURISCVState *env);
 
+bool riscv_itrigger_enabled(CPURISCVState *env);
 #endif /* RISCV_DEBUG_H */
diff --git a/target/riscv/helper.h b/target/riscv/helper.h
index a03014fe67..227c7122ef 100644
--- a/target/riscv/helper.h
+++ b/target/riscv/helper.h
@@ -109,6 +109,8 @@ DEF_HELPER_1(sret, tl, env)
 DEF_HELPER_1(mret, tl, env)
 DEF_HELPER_1(wfi, void, env)
 DEF_HELPER_1(tlb_flush, void, env)
+/* Native Debug */
+DEF_HELPER_1(itrigger_match, void, env)
 #endif
 
 /* Hypervisor functions */
diff --git a/target/riscv/insn_trans/trans_privileged.c.inc b/target/riscv/insn_trans/trans_privileged.c.inc
index 3281408a87..59501b2780 100644
--- a/target/riscv/insn_trans/trans_privileged.c.inc
+++ b/target/riscv/insn_trans/trans_privileged.c.inc
@@ -78,7 +78,7 @@ static bool trans_sret(DisasContext *ctx, arg_sret *a)
     if (has_ext(ctx, RVS)) {
         decode_save_opc(ctx);
         gen_helper_sret(cpu_pc, cpu_env);
-        tcg_gen_exit_tb(NULL, 0); /* no chaining */
+        exit_tb(ctx); /* no chaining */
         ctx->base.is_jmp = DISAS_NORETURN;
     } else {
         return false;
@@ -94,7 +94,7 @@ static bool trans_mret(DisasContext *ctx, arg_mret *a)
 #ifndef CONFIG_USER_ONLY
     decode_save_opc(ctx);
     gen_helper_mret(cpu_pc, cpu_env);
-    tcg_gen_exit_tb(NULL, 0); /* no chaining */
+    exit_tb(ctx); /* no chaining */
     ctx->base.is_jmp = DISAS_NORETURN;
     return true;
 #else
diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
index c49dbec0eb..5c69b88d1e 100644
--- a/target/riscv/insn_trans/trans_rvi.c.inc
+++ b/target/riscv/insn_trans/trans_rvi.c.inc
@@ -66,7 +66,7 @@ static bool trans_jalr(DisasContext *ctx, arg_jalr *a)
     }
 
     gen_set_gpri(ctx, a->rd, ctx->pc_succ_insn);
-    tcg_gen_lookup_and_goto_ptr();
+    lookup_and_goto_ptr(ctx);
 
     if (misaligned) {
         gen_set_label(misaligned);
@@ -803,7 +803,7 @@ static bool trans_pause(DisasContext *ctx, arg_pause *a)
      * end the TB and return to main loop
      */
     gen_set_pc_imm(ctx, ctx->pc_succ_insn);
-    tcg_gen_exit_tb(NULL, 0);
+    exit_tb(ctx);
     ctx->base.is_jmp = DISAS_NORETURN;
 
     return true;
@@ -827,7 +827,7 @@ static bool trans_fence_i(DisasContext *ctx, arg_fence_i *a)
      * however we need to end the translation block
      */
     gen_set_pc_imm(ctx, ctx->pc_succ_insn);
-    tcg_gen_exit_tb(NULL, 0);
+    exit_tb(ctx);
     ctx->base.is_jmp = DISAS_NORETURN;
     return true;
 }
@@ -838,7 +838,7 @@ static bool do_csr_post(DisasContext *ctx)
     decode_save_opc(ctx);
     /* We may have changed important cpu state -- exit to main loop. */
     gen_set_pc_imm(ctx, ctx->pc_succ_insn);
-    tcg_gen_exit_tb(NULL, 0);
+    exit_tb(ctx);
     ctx->base.is_jmp = DISAS_NORETURN;
     return true;
 }
diff --git a/target/riscv/insn_trans/trans_rvv.c.inc b/target/riscv/insn_trans/trans_rvv.c.inc
index 4dea4413ae..d455acedbf 100644
--- a/target/riscv/insn_trans/trans_rvv.c.inc
+++ b/target/riscv/insn_trans/trans_rvv.c.inc
@@ -196,7 +196,7 @@ static bool do_vsetvl(DisasContext *s, int rd, int rs1, TCGv s2)
     mark_vs_dirty(s);
 
     gen_set_pc_imm(s, s->pc_succ_insn);
-    tcg_gen_lookup_and_goto_ptr();
+    lookup_and_goto_ptr(s);
     s->base.is_jmp = DISAS_NORETURN;
 
     if (rd == 0 && rs1 == 0) {
@@ -222,7 +222,7 @@ static bool do_vsetivli(DisasContext *s, int rd, TCGv s1, TCGv s2)
     gen_set_gpr(s, rd, dst);
     mark_vs_dirty(s);
     gen_set_pc_imm(s, s->pc_succ_insn);
-    tcg_gen_lookup_and_goto_ptr();
+    lookup_and_goto_ptr(s);
     s->base.is_jmp = DISAS_NORETURN;
 
     return true;
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index db123da5ec..d704265a37 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -111,6 +111,8 @@ typedef struct DisasContext {
     /* PointerMasking extension */
     bool pm_mask_enabled;
     bool pm_base_enabled;
+    /* Use icount trigger for native debug */
+    bool itrigger;
     /* TCG of the current insn_start */
     TCGOp *insn_start;
 } DisasContext;
@@ -252,15 +254,39 @@ static void gen_exception_inst_addr_mis(DisasContext *ctx)
     generate_exception(ctx, RISCV_EXCP_INST_ADDR_MIS);
 }
 
+static void lookup_and_goto_ptr(DisasContext *ctx)
+{
+#ifndef CONFIG_USER_ONLY
+    if (ctx->itrigger) {
+        gen_helper_itrigger_match(cpu_env);
+    }
+#endif
+    tcg_gen_lookup_and_goto_ptr();
+}
+
+static void exit_tb(DisasContext *ctx)
+{
+#ifndef CONFIG_USER_ONLY
+    if (ctx->itrigger) {
+        gen_helper_itrigger_match(cpu_env);
+    }
+#endif
+    tcg_gen_exit_tb(NULL, 0);
+}
+
 static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
 {
-    if (translator_use_goto_tb(&ctx->base, dest)) {
+     /*
+      * Under itrigger, instruction executes one by one like singlestep,
+      * direct block chain benefits will be small.
+      */
+    if (translator_use_goto_tb(&ctx->base, dest) && !ctx->itrigger) {
         tcg_gen_goto_tb(n);
         gen_set_pc_imm(ctx, dest);
         tcg_gen_exit_tb(ctx->base.tb, n);
     } else {
         gen_set_pc_imm(ctx, dest);
-        tcg_gen_lookup_and_goto_ptr();
+        lookup_and_goto_ptr(ctx);
     }
 }
 
@@ -1136,6 +1162,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     memset(ctx->ftemp, 0, sizeof(ctx->ftemp));
     ctx->pm_mask_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_MASK_ENABLED);
     ctx->pm_base_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_BASE_ENABLED);
+    ctx->itrigger = FIELD_EX32(tb_flags, TB_FLAGS, ITRIGGER);
     ctx->zero = tcg_constant_tl(0);
 }
 
@@ -1175,7 +1202,7 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
 
     /* Only the first insn within a TB is allowed to cross a page boundary. */
     if (ctx->base.is_jmp == DISAS_NEXT) {
-        if (!is_same_page(&ctx->base, ctx->base.pc_next)) {
+        if (ctx->itrigger || !is_same_page(&ctx->base, ctx->base.pc_next)) {
             ctx->base.is_jmp = DISAS_TOO_MANY;
         } else {
             unsigned page_ofs = ctx->base.pc_next & ~TARGET_PAGE_MASK;
-- 
2.17.1

Re: [PATCH v1 1/4] target/riscv: Add itrigger support when icount is not enabled

[Alistair Francis]

On Thu, Oct 13, 2022 at 4:32 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>
> When icount is not enabled, there is no API in QEMU that can get the
> guest instruction number.
>
> Translate the guest code in a way that each TB only has one instruction.

I don't think this is a great idea.

Why can't we just require icount be enabled if a user wants this? Or singlestep?

Alistair

> After executing the instruction, decrease the count by 1 until it reaches 0
> where the itrigger fires.
>
> Note that only when priviledge matches the itrigger configuration,
> the count will decrease.
>
> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> ---
>  target/riscv/cpu.h                            |  2 +
>  target/riscv/cpu_helper.c                     |  6 ++
>  target/riscv/debug.c                          | 71 +++++++++++++++++++
>  target/riscv/debug.h                          | 12 ++++
>  target/riscv/helper.h                         |  2 +
>  .../riscv/insn_trans/trans_privileged.c.inc   |  4 +-
>  target/riscv/insn_trans/trans_rvi.c.inc       |  8 +--
>  target/riscv/insn_trans/trans_rvv.c.inc       |  4 +-
>  target/riscv/translate.c                      | 33 ++++++++-
>  9 files changed, 131 insertions(+), 11 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index b131fa8c8e..24bafda27d 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -621,6 +621,8 @@ FIELD(TB_FLAGS, PM_MASK_ENABLED, 22, 1)
>  FIELD(TB_FLAGS, PM_BASE_ENABLED, 23, 1)
>  FIELD(TB_FLAGS, VTA, 24, 1)
>  FIELD(TB_FLAGS, VMA, 25, 1)
> +/* Native debug itrigger */
> +FIELD(TB_FLAGS, ITRIGGER, 26, 1)
>
>  #ifdef TARGET_RISCV32
>  #define riscv_cpu_mxl(env)  ((void)(env), MXL_RV32)
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 278d163803..263282f230 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -27,7 +27,9 @@
>  #include "tcg/tcg-op.h"
>  #include "trace.h"
>  #include "semihosting/common-semi.h"
> +#include "sysemu/cpu-timers.h"
>  #include "cpu_bits.h"
> +#include "debug.h"
>
>  int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
>  {
> @@ -103,6 +105,10 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
>          flags = FIELD_DP32(flags, TB_FLAGS, MSTATUS_HS_VS,
>                             get_field(env->mstatus_hs, MSTATUS_VS));
>      }
> +    if (riscv_feature(env, RISCV_FEATURE_DEBUG) && !icount_enabled()) {
> +        flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER,
> +                           riscv_itrigger_enabled(env));
> +    }
>  #endif
>
>      flags = FIELD_DP32(flags, TB_FLAGS, XL, env->xl);
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index 26ea764407..45a3537d5c 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -29,6 +29,7 @@
>  #include "cpu.h"
>  #include "trace.h"
>  #include "exec/exec-all.h"
> +#include "exec/helper-proto.h"
>
>  /*
>   * The following M-mode trigger CSRs are implemented:
> @@ -498,6 +499,76 @@ static void type6_reg_write(CPURISCVState *env, target_ulong index,
>      return;
>  }
>
> +/* icount trigger type */
> +static inline int
> +itrigger_get_count(CPURISCVState *env, int index)
> +{
> +    return get_field(env->tdata1[index], ITRIGGER_COUNT);
> +}
> +
> +static inline void
> +itrigger_set_count(CPURISCVState *env, int index, int value)
> +{
> +    env->tdata1[index] = set_field(env->tdata1[index],
> +                                   ITRIGGER_COUNT, value);
> +}
> +
> +static bool check_itrigger_priv(CPURISCVState *env, int index)
> +{
> +    target_ulong tdata1 = env->tdata1[index];
> +    if (riscv_cpu_virt_enabled(env)) {
> +        /* check VU/VS bit against current privilege level */
> +        return (get_field(tdata1, ITRIGGER_VS) == env->priv) ||
> +               (get_field(tdata1, ITRIGGER_VU) == env->priv);
> +    } else {
> +        /* check U/S/M bit against current privilege level */
> +        return (get_field(tdata1, ITRIGGER_M) == env->priv) ||
> +               (get_field(tdata1, ITRIGGER_S) == env->priv) ||
> +               (get_field(tdata1, ITRIGGER_U) == env->priv);
> +    }
> +}
> +
> +bool riscv_itrigger_enabled(CPURISCVState *env)
> +{
> +    int count;
> +    for (int i = 0; i < RV_MAX_TRIGGERS; i++) {
> +        if (get_trigger_type(env, i) != TRIGGER_TYPE_INST_CNT) {
> +            continue;
> +        }
> +        if (check_itrigger_priv(env, i)) {
> +            continue;
> +        }
> +        count = itrigger_get_count(env, i);
> +        if (!count) {
> +            continue;
> +        }
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +void helper_itrigger_match(CPURISCVState *env)
> +{
> +    int count;
> +    for (int i = 0; i < RV_MAX_TRIGGERS; i++) {
> +        if (get_trigger_type(env, i) != TRIGGER_TYPE_INST_CNT) {
> +            continue;
> +        }
> +        if (check_itrigger_priv(env, i)) {
> +            continue;
> +        }
> +        count = itrigger_get_count(env, i);
> +        if (!count) {
> +            continue;
> +        }
> +        itrigger_set_count(env, i, count--);
> +        if (!count) {
> +            do_trigger_action(env, i);
> +        }
> +    }
> +}
> +
>  target_ulong tdata_csr_read(CPURISCVState *env, int tdata_index)
>  {
>      switch (tdata_index) {
> diff --git a/target/riscv/debug.h b/target/riscv/debug.h
> index a1226b4d29..cc3358e69b 100644
> --- a/target/riscv/debug.h
> +++ b/target/riscv/debug.h
> @@ -118,6 +118,17 @@ enum {
>      SIZE_NUM = 16
>  };
>
> +/* itrigger filed masks */
> +#define ITRIGGER_ACTION       0x3f
> +#define ITRIGGER_U            BIT(6)
> +#define ITRIGGER_S            BIT(7)
> +#define ITRIGGER_PENDING      BIT(8)
> +#define ITRIGGER_M            BIT(9)
> +#define ITRIGGER_COUNT        (0x3fff << 10)
> +#define ITRIGGER_HIT          BIT(24)
> +#define ITRIGGER_VU           BIT(25)
> +#define ITRIGGER_VS           BIT(26)
> +
>  bool tdata_available(CPURISCVState *env, int tdata_index);
>
>  target_ulong tselect_csr_read(CPURISCVState *env);
> @@ -134,4 +145,5 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp);
>
>  void riscv_trigger_init(CPURISCVState *env);
>
> +bool riscv_itrigger_enabled(CPURISCVState *env);
>  #endif /* RISCV_DEBUG_H */
> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
> index a03014fe67..227c7122ef 100644
> --- a/target/riscv/helper.h
> +++ b/target/riscv/helper.h
> @@ -109,6 +109,8 @@ DEF_HELPER_1(sret, tl, env)
>  DEF_HELPER_1(mret, tl, env)
>  DEF_HELPER_1(wfi, void, env)
>  DEF_HELPER_1(tlb_flush, void, env)
> +/* Native Debug */
> +DEF_HELPER_1(itrigger_match, void, env)
>  #endif
>
>  /* Hypervisor functions */
> diff --git a/target/riscv/insn_trans/trans_privileged.c.inc b/target/riscv/insn_trans/trans_privileged.c.inc
> index 3281408a87..59501b2780 100644
> --- a/target/riscv/insn_trans/trans_privileged.c.inc
> +++ b/target/riscv/insn_trans/trans_privileged.c.inc
> @@ -78,7 +78,7 @@ static bool trans_sret(DisasContext *ctx, arg_sret *a)
>      if (has_ext(ctx, RVS)) {
>          decode_save_opc(ctx);
>          gen_helper_sret(cpu_pc, cpu_env);
> -        tcg_gen_exit_tb(NULL, 0); /* no chaining */
> +        exit_tb(ctx); /* no chaining */
>          ctx->base.is_jmp = DISAS_NORETURN;
>      } else {
>          return false;
> @@ -94,7 +94,7 @@ static bool trans_mret(DisasContext *ctx, arg_mret *a)
>  #ifndef CONFIG_USER_ONLY
>      decode_save_opc(ctx);
>      gen_helper_mret(cpu_pc, cpu_env);
> -    tcg_gen_exit_tb(NULL, 0); /* no chaining */
> +    exit_tb(ctx); /* no chaining */
>      ctx->base.is_jmp = DISAS_NORETURN;
>      return true;
>  #else
> diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
> index c49dbec0eb..5c69b88d1e 100644
> --- a/target/riscv/insn_trans/trans_rvi.c.inc
> +++ b/target/riscv/insn_trans/trans_rvi.c.inc
> @@ -66,7 +66,7 @@ static bool trans_jalr(DisasContext *ctx, arg_jalr *a)
>      }
>
>      gen_set_gpri(ctx, a->rd, ctx->pc_succ_insn);
> -    tcg_gen_lookup_and_goto_ptr();
> +    lookup_and_goto_ptr(ctx);
>
>      if (misaligned) {
>          gen_set_label(misaligned);
> @@ -803,7 +803,7 @@ static bool trans_pause(DisasContext *ctx, arg_pause *a)
>       * end the TB and return to main loop
>       */
>      gen_set_pc_imm(ctx, ctx->pc_succ_insn);
> -    tcg_gen_exit_tb(NULL, 0);
> +    exit_tb(ctx);
>      ctx->base.is_jmp = DISAS_NORETURN;
>
>      return true;
> @@ -827,7 +827,7 @@ static bool trans_fence_i(DisasContext *ctx, arg_fence_i *a)
>       * however we need to end the translation block
>       */
>      gen_set_pc_imm(ctx, ctx->pc_succ_insn);
> -    tcg_gen_exit_tb(NULL, 0);
> +    exit_tb(ctx);
>      ctx->base.is_jmp = DISAS_NORETURN;
>      return true;
>  }
> @@ -838,7 +838,7 @@ static bool do_csr_post(DisasContext *ctx)
>      decode_save_opc(ctx);
>      /* We may have changed important cpu state -- exit to main loop. */
>      gen_set_pc_imm(ctx, ctx->pc_succ_insn);
> -    tcg_gen_exit_tb(NULL, 0);
> +    exit_tb(ctx);
>      ctx->base.is_jmp = DISAS_NORETURN;
>      return true;
>  }
> diff --git a/target/riscv/insn_trans/trans_rvv.c.inc b/target/riscv/insn_trans/trans_rvv.c.inc
> index 4dea4413ae..d455acedbf 100644
> --- a/target/riscv/insn_trans/trans_rvv.c.inc
> +++ b/target/riscv/insn_trans/trans_rvv.c.inc
> @@ -196,7 +196,7 @@ static bool do_vsetvl(DisasContext *s, int rd, int rs1, TCGv s2)
>      mark_vs_dirty(s);
>
>      gen_set_pc_imm(s, s->pc_succ_insn);
> -    tcg_gen_lookup_and_goto_ptr();
> +    lookup_and_goto_ptr(s);
>      s->base.is_jmp = DISAS_NORETURN;
>
>      if (rd == 0 && rs1 == 0) {
> @@ -222,7 +222,7 @@ static bool do_vsetivli(DisasContext *s, int rd, TCGv s1, TCGv s2)
>      gen_set_gpr(s, rd, dst);
>      mark_vs_dirty(s);
>      gen_set_pc_imm(s, s->pc_succ_insn);
> -    tcg_gen_lookup_and_goto_ptr();
> +    lookup_and_goto_ptr(s);
>      s->base.is_jmp = DISAS_NORETURN;
>
>      return true;
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index db123da5ec..d704265a37 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -111,6 +111,8 @@ typedef struct DisasContext {
>      /* PointerMasking extension */
>      bool pm_mask_enabled;
>      bool pm_base_enabled;
> +    /* Use icount trigger for native debug */
> +    bool itrigger;
>      /* TCG of the current insn_start */
>      TCGOp *insn_start;
>  } DisasContext;
> @@ -252,15 +254,39 @@ static void gen_exception_inst_addr_mis(DisasContext *ctx)
>      generate_exception(ctx, RISCV_EXCP_INST_ADDR_MIS);
>  }
>
> +static void lookup_and_goto_ptr(DisasContext *ctx)
> +{
> +#ifndef CONFIG_USER_ONLY
> +    if (ctx->itrigger) {
> +        gen_helper_itrigger_match(cpu_env);
> +    }
> +#endif
> +    tcg_gen_lookup_and_goto_ptr();
> +}
> +
> +static void exit_tb(DisasContext *ctx)
> +{
> +#ifndef CONFIG_USER_ONLY
> +    if (ctx->itrigger) {
> +        gen_helper_itrigger_match(cpu_env);
> +    }
> +#endif
> +    tcg_gen_exit_tb(NULL, 0);
> +}
> +
>  static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
>  {
> -    if (translator_use_goto_tb(&ctx->base, dest)) {
> +     /*
> +      * Under itrigger, instruction executes one by one like singlestep,
> +      * direct block chain benefits will be small.
> +      */
> +    if (translator_use_goto_tb(&ctx->base, dest) && !ctx->itrigger) {
>          tcg_gen_goto_tb(n);
>          gen_set_pc_imm(ctx, dest);
>          tcg_gen_exit_tb(ctx->base.tb, n);
>      } else {
>          gen_set_pc_imm(ctx, dest);
> -        tcg_gen_lookup_and_goto_ptr();
> +        lookup_and_goto_ptr(ctx);
>      }
>  }
>
> @@ -1136,6 +1162,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>      memset(ctx->ftemp, 0, sizeof(ctx->ftemp));
>      ctx->pm_mask_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_MASK_ENABLED);
>      ctx->pm_base_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_BASE_ENABLED);
> +    ctx->itrigger = FIELD_EX32(tb_flags, TB_FLAGS, ITRIGGER);
>      ctx->zero = tcg_constant_tl(0);
>  }
>
> @@ -1175,7 +1202,7 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
>
>      /* Only the first insn within a TB is allowed to cross a page boundary. */
>      if (ctx->base.is_jmp == DISAS_NEXT) {
> -        if (!is_same_page(&ctx->base, ctx->base.pc_next)) {
> +        if (ctx->itrigger || !is_same_page(&ctx->base, ctx->base.pc_next)) {
>              ctx->base.is_jmp = DISAS_TOO_MANY;
>          } else {
>              unsigned page_ofs = ctx->base.pc_next & ~TARGET_PAGE_MASK;
> --
> 2.17.1
>
>

Re: [PATCH v1 1/4] target/riscv: Add itrigger support when icount is not enabled

[LIU Zhiwei]

On 2022/11/7 9:37, Alistair Francis wrote:
> On Thu, Oct 13, 2022 at 4:32 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>> When icount is not enabled, there is no API in QEMU that can get the
>> guest instruction number.
>>
>> Translate the guest code in a way that each TB only has one instruction.
> I don't think this is a great idea.
>
> Why can't we just require icount be enabled if a user wants this? Or singlestep?

This feature will only be used by users who want to  run the native gdb 
on Linux. If we run QEMU as a service,  after booting the kernel, we 
can't predicate whether the users will use native gdb.

Besides, icount can't be enabled on MTTCG currently (I am working on 
this problem) and I don't want to constraint the use of MTTCG even  when 
it is possible the users use native gdb (which may only occupy just a 
little time).


Thus, I give this fallback way to implement the itrigger.  The icount 
parameter can be used as an accelerated way.

Thanks,
Zhiwei

>
> Alistair
>
>> After executing the instruction, decrease the count by 1 until it reaches 0
>> where the itrigger fires.
>>
>> Note that only when priviledge matches the itrigger configuration,
>> the count will decrease.
>>
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>> ---
>>   target/riscv/cpu.h                            |  2 +
>>   target/riscv/cpu_helper.c                     |  6 ++
>>   target/riscv/debug.c                          | 71 +++++++++++++++++++
>>   target/riscv/debug.h                          | 12 ++++
>>   target/riscv/helper.h                         |  2 +
>>   .../riscv/insn_trans/trans_privileged.c.inc   |  4 +-
>>   target/riscv/insn_trans/trans_rvi.c.inc       |  8 +--
>>   target/riscv/insn_trans/trans_rvv.c.inc       |  4 +-
>>   target/riscv/translate.c                      | 33 ++++++++-
>>   9 files changed, 131 insertions(+), 11 deletions(-)
>>
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index b131fa8c8e..24bafda27d 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -621,6 +621,8 @@ FIELD(TB_FLAGS, PM_MASK_ENABLED, 22, 1)
>>   FIELD(TB_FLAGS, PM_BASE_ENABLED, 23, 1)
>>   FIELD(TB_FLAGS, VTA, 24, 1)
>>   FIELD(TB_FLAGS, VMA, 25, 1)
>> +/* Native debug itrigger */
>> +FIELD(TB_FLAGS, ITRIGGER, 26, 1)
>>
>>   #ifdef TARGET_RISCV32
>>   #define riscv_cpu_mxl(env)  ((void)(env), MXL_RV32)
>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> index 278d163803..263282f230 100644
>> --- a/target/riscv/cpu_helper.c
>> +++ b/target/riscv/cpu_helper.c
>> @@ -27,7 +27,9 @@
>>   #include "tcg/tcg-op.h"
>>   #include "trace.h"
>>   #include "semihosting/common-semi.h"
>> +#include "sysemu/cpu-timers.h"
>>   #include "cpu_bits.h"
>> +#include "debug.h"
>>
>>   int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
>>   {
>> @@ -103,6 +105,10 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
>>           flags = FIELD_DP32(flags, TB_FLAGS, MSTATUS_HS_VS,
>>                              get_field(env->mstatus_hs, MSTATUS_VS));
>>       }
>> +    if (riscv_feature(env, RISCV_FEATURE_DEBUG) && !icount_enabled()) {
>> +        flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER,
>> +                           riscv_itrigger_enabled(env));
>> +    }
>>   #endif
>>
>>       flags = FIELD_DP32(flags, TB_FLAGS, XL, env->xl);
>> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
>> index 26ea764407..45a3537d5c 100644
>> --- a/target/riscv/debug.c
>> +++ b/target/riscv/debug.c
>> @@ -29,6 +29,7 @@
>>   #include "cpu.h"
>>   #include "trace.h"
>>   #include "exec/exec-all.h"
>> +#include "exec/helper-proto.h"
>>
>>   /*
>>    * The following M-mode trigger CSRs are implemented:
>> @@ -498,6 +499,76 @@ static void type6_reg_write(CPURISCVState *env, target_ulong index,
>>       return;
>>   }
>>
>> +/* icount trigger type */
>> +static inline int
>> +itrigger_get_count(CPURISCVState *env, int index)
>> +{
>> +    return get_field(env->tdata1[index], ITRIGGER_COUNT);
>> +}
>> +
>> +static inline void
>> +itrigger_set_count(CPURISCVState *env, int index, int value)
>> +{
>> +    env->tdata1[index] = set_field(env->tdata1[index],
>> +                                   ITRIGGER_COUNT, value);
>> +}
>> +
>> +static bool check_itrigger_priv(CPURISCVState *env, int index)
>> +{
>> +    target_ulong tdata1 = env->tdata1[index];
>> +    if (riscv_cpu_virt_enabled(env)) {
>> +        /* check VU/VS bit against current privilege level */
>> +        return (get_field(tdata1, ITRIGGER_VS) == env->priv) ||
>> +               (get_field(tdata1, ITRIGGER_VU) == env->priv);
>> +    } else {
>> +        /* check U/S/M bit against current privilege level */
>> +        return (get_field(tdata1, ITRIGGER_M) == env->priv) ||
>> +               (get_field(tdata1, ITRIGGER_S) == env->priv) ||
>> +               (get_field(tdata1, ITRIGGER_U) == env->priv);
>> +    }
>> +}
>> +
>> +bool riscv_itrigger_enabled(CPURISCVState *env)
>> +{
>> +    int count;
>> +    for (int i = 0; i < RV_MAX_TRIGGERS; i++) {
>> +        if (get_trigger_type(env, i) != TRIGGER_TYPE_INST_CNT) {
>> +            continue;
>> +        }
>> +        if (check_itrigger_priv(env, i)) {
>> +            continue;
>> +        }
>> +        count = itrigger_get_count(env, i);
>> +        if (!count) {
>> +            continue;
>> +        }
>> +        return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +void helper_itrigger_match(CPURISCVState *env)
>> +{
>> +    int count;
>> +    for (int i = 0; i < RV_MAX_TRIGGERS; i++) {
>> +        if (get_trigger_type(env, i) != TRIGGER_TYPE_INST_CNT) {
>> +            continue;
>> +        }
>> +        if (check_itrigger_priv(env, i)) {
>> +            continue;
>> +        }
>> +        count = itrigger_get_count(env, i);
>> +        if (!count) {
>> +            continue;
>> +        }
>> +        itrigger_set_count(env, i, count--);
>> +        if (!count) {
>> +            do_trigger_action(env, i);
>> +        }
>> +    }
>> +}
>> +
>>   target_ulong tdata_csr_read(CPURISCVState *env, int tdata_index)
>>   {
>>       switch (tdata_index) {
>> diff --git a/target/riscv/debug.h b/target/riscv/debug.h
>> index a1226b4d29..cc3358e69b 100644
>> --- a/target/riscv/debug.h
>> +++ b/target/riscv/debug.h
>> @@ -118,6 +118,17 @@ enum {
>>       SIZE_NUM = 16
>>   };
>>
>> +/* itrigger filed masks */
>> +#define ITRIGGER_ACTION       0x3f
>> +#define ITRIGGER_U            BIT(6)
>> +#define ITRIGGER_S            BIT(7)
>> +#define ITRIGGER_PENDING      BIT(8)
>> +#define ITRIGGER_M            BIT(9)
>> +#define ITRIGGER_COUNT        (0x3fff << 10)
>> +#define ITRIGGER_HIT          BIT(24)
>> +#define ITRIGGER_VU           BIT(25)
>> +#define ITRIGGER_VS           BIT(26)
>> +
>>   bool tdata_available(CPURISCVState *env, int tdata_index);
>>
>>   target_ulong tselect_csr_read(CPURISCVState *env);
>> @@ -134,4 +145,5 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp);
>>
>>   void riscv_trigger_init(CPURISCVState *env);
>>
>> +bool riscv_itrigger_enabled(CPURISCVState *env);
>>   #endif /* RISCV_DEBUG_H */
>> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
>> index a03014fe67..227c7122ef 100644
>> --- a/target/riscv/helper.h
>> +++ b/target/riscv/helper.h
>> @@ -109,6 +109,8 @@ DEF_HELPER_1(sret, tl, env)
>>   DEF_HELPER_1(mret, tl, env)
>>   DEF_HELPER_1(wfi, void, env)
>>   DEF_HELPER_1(tlb_flush, void, env)
>> +/* Native Debug */
>> +DEF_HELPER_1(itrigger_match, void, env)
>>   #endif
>>
>>   /* Hypervisor functions */
>> diff --git a/target/riscv/insn_trans/trans_privileged.c.inc b/target/riscv/insn_trans/trans_privileged.c.inc
>> index 3281408a87..59501b2780 100644
>> --- a/target/riscv/insn_trans/trans_privileged.c.inc
>> +++ b/target/riscv/insn_trans/trans_privileged.c.inc
>> @@ -78,7 +78,7 @@ static bool trans_sret(DisasContext *ctx, arg_sret *a)
>>       if (has_ext(ctx, RVS)) {
>>           decode_save_opc(ctx);
>>           gen_helper_sret(cpu_pc, cpu_env);
>> -        tcg_gen_exit_tb(NULL, 0); /* no chaining */
>> +        exit_tb(ctx); /* no chaining */
>>           ctx->base.is_jmp = DISAS_NORETURN;
>>       } else {
>>           return false;
>> @@ -94,7 +94,7 @@ static bool trans_mret(DisasContext *ctx, arg_mret *a)
>>   #ifndef CONFIG_USER_ONLY
>>       decode_save_opc(ctx);
>>       gen_helper_mret(cpu_pc, cpu_env);
>> -    tcg_gen_exit_tb(NULL, 0); /* no chaining */
>> +    exit_tb(ctx); /* no chaining */
>>       ctx->base.is_jmp = DISAS_NORETURN;
>>       return true;
>>   #else
>> diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
>> index c49dbec0eb..5c69b88d1e 100644
>> --- a/target/riscv/insn_trans/trans_rvi.c.inc
>> +++ b/target/riscv/insn_trans/trans_rvi.c.inc
>> @@ -66,7 +66,7 @@ static bool trans_jalr(DisasContext *ctx, arg_jalr *a)
>>       }
>>
>>       gen_set_gpri(ctx, a->rd, ctx->pc_succ_insn);
>> -    tcg_gen_lookup_and_goto_ptr();
>> +    lookup_and_goto_ptr(ctx);
>>
>>       if (misaligned) {
>>           gen_set_label(misaligned);
>> @@ -803,7 +803,7 @@ static bool trans_pause(DisasContext *ctx, arg_pause *a)
>>        * end the TB and return to main loop
>>        */
>>       gen_set_pc_imm(ctx, ctx->pc_succ_insn);
>> -    tcg_gen_exit_tb(NULL, 0);
>> +    exit_tb(ctx);
>>       ctx->base.is_jmp = DISAS_NORETURN;
>>
>>       return true;
>> @@ -827,7 +827,7 @@ static bool trans_fence_i(DisasContext *ctx, arg_fence_i *a)
>>        * however we need to end the translation block
>>        */
>>       gen_set_pc_imm(ctx, ctx->pc_succ_insn);
>> -    tcg_gen_exit_tb(NULL, 0);
>> +    exit_tb(ctx);
>>       ctx->base.is_jmp = DISAS_NORETURN;
>>       return true;
>>   }
>> @@ -838,7 +838,7 @@ static bool do_csr_post(DisasContext *ctx)
>>       decode_save_opc(ctx);
>>       /* We may have changed important cpu state -- exit to main loop. */
>>       gen_set_pc_imm(ctx, ctx->pc_succ_insn);
>> -    tcg_gen_exit_tb(NULL, 0);
>> +    exit_tb(ctx);
>>       ctx->base.is_jmp = DISAS_NORETURN;
>>       return true;
>>   }
>> diff --git a/target/riscv/insn_trans/trans_rvv.c.inc b/target/riscv/insn_trans/trans_rvv.c.inc
>> index 4dea4413ae..d455acedbf 100644
>> --- a/target/riscv/insn_trans/trans_rvv.c.inc
>> +++ b/target/riscv/insn_trans/trans_rvv.c.inc
>> @@ -196,7 +196,7 @@ static bool do_vsetvl(DisasContext *s, int rd, int rs1, TCGv s2)
>>       mark_vs_dirty(s);
>>
>>       gen_set_pc_imm(s, s->pc_succ_insn);
>> -    tcg_gen_lookup_and_goto_ptr();
>> +    lookup_and_goto_ptr(s);
>>       s->base.is_jmp = DISAS_NORETURN;
>>
>>       if (rd == 0 && rs1 == 0) {
>> @@ -222,7 +222,7 @@ static bool do_vsetivli(DisasContext *s, int rd, TCGv s1, TCGv s2)
>>       gen_set_gpr(s, rd, dst);
>>       mark_vs_dirty(s);
>>       gen_set_pc_imm(s, s->pc_succ_insn);
>> -    tcg_gen_lookup_and_goto_ptr();
>> +    lookup_and_goto_ptr(s);
>>       s->base.is_jmp = DISAS_NORETURN;
>>
>>       return true;
>> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
>> index db123da5ec..d704265a37 100644
>> --- a/target/riscv/translate.c
>> +++ b/target/riscv/translate.c
>> @@ -111,6 +111,8 @@ typedef struct DisasContext {
>>       /* PointerMasking extension */
>>       bool pm_mask_enabled;
>>       bool pm_base_enabled;
>> +    /* Use icount trigger for native debug */
>> +    bool itrigger;
>>       /* TCG of the current insn_start */
>>       TCGOp *insn_start;
>>   } DisasContext;
>> @@ -252,15 +254,39 @@ static void gen_exception_inst_addr_mis(DisasContext *ctx)
>>       generate_exception(ctx, RISCV_EXCP_INST_ADDR_MIS);
>>   }
>>
>> +static void lookup_and_goto_ptr(DisasContext *ctx)
>> +{
>> +#ifndef CONFIG_USER_ONLY
>> +    if (ctx->itrigger) {
>> +        gen_helper_itrigger_match(cpu_env);
>> +    }
>> +#endif
>> +    tcg_gen_lookup_and_goto_ptr();
>> +}
>> +
>> +static void exit_tb(DisasContext *ctx)
>> +{
>> +#ifndef CONFIG_USER_ONLY
>> +    if (ctx->itrigger) {
>> +        gen_helper_itrigger_match(cpu_env);
>> +    }
>> +#endif
>> +    tcg_gen_exit_tb(NULL, 0);
>> +}
>> +
>>   static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
>>   {
>> -    if (translator_use_goto_tb(&ctx->base, dest)) {
>> +     /*
>> +      * Under itrigger, instruction executes one by one like singlestep,
>> +      * direct block chain benefits will be small.
>> +      */
>> +    if (translator_use_goto_tb(&ctx->base, dest) && !ctx->itrigger) {
>>           tcg_gen_goto_tb(n);
>>           gen_set_pc_imm(ctx, dest);
>>           tcg_gen_exit_tb(ctx->base.tb, n);
>>       } else {
>>           gen_set_pc_imm(ctx, dest);
>> -        tcg_gen_lookup_and_goto_ptr();
>> +        lookup_and_goto_ptr(ctx);
>>       }
>>   }
>>
>> @@ -1136,6 +1162,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>>       memset(ctx->ftemp, 0, sizeof(ctx->ftemp));
>>       ctx->pm_mask_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_MASK_ENABLED);
>>       ctx->pm_base_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_BASE_ENABLED);
>> +    ctx->itrigger = FIELD_EX32(tb_flags, TB_FLAGS, ITRIGGER);
>>       ctx->zero = tcg_constant_tl(0);
>>   }
>>
>> @@ -1175,7 +1202,7 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
>>
>>       /* Only the first insn within a TB is allowed to cross a page boundary. */
>>       if (ctx->base.is_jmp == DISAS_NEXT) {
>> -        if (!is_same_page(&ctx->base, ctx->base.pc_next)) {
>> +        if (ctx->itrigger || !is_same_page(&ctx->base, ctx->base.pc_next)) {
>>               ctx->base.is_jmp = DISAS_TOO_MANY;
>>           } else {
>>               unsigned page_ofs = ctx->base.pc_next & ~TARGET_PAGE_MASK;
>> --
>> 2.17.1
>>
>>

Re: [PATCH v1 1/4] target/riscv: Add itrigger support when icount is not enabled

[Alex Bennée]

LIU Zhiwei <zhiwei_liu@linux.alibaba.com> writes:

> On 2022/11/7 9:37, Alistair Francis wrote:
>> On Thu, Oct 13, 2022 at 4:32 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>>> When icount is not enabled, there is no API in QEMU that can get the
>>> guest instruction number.
>>>
>>> Translate the guest code in a way that each TB only has one instruction.
>> I don't think this is a great idea.
>>
>> Why can't we just require icount be enabled if a user wants this? Or singlestep?
>
> This feature will only be used by users who want to  run the native
> gdb on Linux. If we run QEMU as a service,  after booting the kernel,
> we can't predicate whether the users will use native gdb.
>
> Besides, icount can't be enabled on MTTCG currently (I am working on
> this problem)

I'm curious as to what your approach is going to be to solve this one?

-- 
Alex Bennée

Re: [PATCH v1 1/4] target/riscv: Add itrigger support when icount is not enabled

[LIU Zhiwei]

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


On 2022/11/7 23:58, Alex Bennée wrote:
> LIU Zhiwei<zhiwei_liu@linux.alibaba.com>  writes:
>
>> On 2022/11/7 9:37, Alistair Francis wrote:
>>> On Thu, Oct 13, 2022 at 4:32 PM LIU Zhiwei<zhiwei_liu@linux.alibaba.com>  wrote:
>>>> When icount is not enabled, there is no API in QEMU that can get the
>>>> guest instruction number.
>>>>
>>>> Translate the guest code in a way that each TB only has one instruction.
>>> I don't think this is a great idea.
>>>
>>> Why can't we just require icount be enabled if a user wants this? Or singlestep?
>> This feature will only be used by users who want to  run the native
>> gdb on Linux. If we run QEMU as a service,  after booting the kernel,
>> we can't predicate whether the users will use native gdb.
>>
>> Besides, icount can't be enabled on MTTCG currently (I am working on
>> this problem)
> I'm curious as to what your approach is going to be to solve this one?

Yes, I am interested in this problem.  But actually, I don't find a 
clear way.

For RR or MTTCG, timers using QEMU_CLOCK_VIRTUAL will set the total 
icount_budget.

  * For RR smp, every cpu has configured the total icount_budget.

  * For MTTCG smp, every cpu can't be configured the total
    icount_budget.  But we can split the icount_budget, such as divide
    by smp.cpus, to each core. If one core consumed its budget, it will
    wait for other cores.  Another way is to kick other cores and split
    the remaining icount_budget.

I am not sure if there are many other problems related.  It a difficult 
problem. Looking forward to your advice.

Thanks,
Zhiwei

>

[-- Attachment #2: Type: text/html, Size: 2805 bytes --]

Re: [PATCH v1 1/4] target/riscv: Add itrigger support when icount is not enabled

[Alistair Francis]

On Mon, Nov 7, 2022 at 12:01 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>
>
> On 2022/11/7 9:37, Alistair Francis wrote:
> > On Thu, Oct 13, 2022 at 4:32 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
> >> When icount is not enabled, there is no API in QEMU that can get the
> >> guest instruction number.
> >>
> >> Translate the guest code in a way that each TB only has one instruction.
> > I don't think this is a great idea.
> >
> > Why can't we just require icount be enabled if a user wants this? Or singlestep?
>
> This feature will only be used by users who want to  run the native gdb
> on Linux. If we run QEMU as a service,  after booting the kernel, we
> can't predicate whether the users will use native gdb.

I understand. It just seems like an invasive change. It does seem like
x86 does something similar though.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

>
> Besides, icount can't be enabled on MTTCG currently (I am working on
> this problem) and I don't want to constraint the use of MTTCG even  when
> it is possible the users use native gdb (which may only occupy just a
> little time).
>
>
> Thus, I give this fallback way to implement the itrigger.  The icount
> parameter can be used as an accelerated way.
>
> Thanks,
> Zhiwei
>
> >
> > Alistair
> >
> >> After executing the instruction, decrease the count by 1 until it reaches 0
> >> where the itrigger fires.
> >>
> >> Note that only when priviledge matches the itrigger configuration,
> >> the count will decrease.
> >>
> >> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> >> ---
> >>   target/riscv/cpu.h                            |  2 +
> >>   target/riscv/cpu_helper.c                     |  6 ++
> >>   target/riscv/debug.c                          | 71 +++++++++++++++++++
> >>   target/riscv/debug.h                          | 12 ++++
> >>   target/riscv/helper.h                         |  2 +
> >>   .../riscv/insn_trans/trans_privileged.c.inc   |  4 +-
> >>   target/riscv/insn_trans/trans_rvi.c.inc       |  8 +--
> >>   target/riscv/insn_trans/trans_rvv.c.inc       |  4 +-
> >>   target/riscv/translate.c                      | 33 ++++++++-
> >>   9 files changed, 131 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> >> index b131fa8c8e..24bafda27d 100644
> >> --- a/target/riscv/cpu.h
> >> +++ b/target/riscv/cpu.h
> >> @@ -621,6 +621,8 @@ FIELD(TB_FLAGS, PM_MASK_ENABLED, 22, 1)
> >>   FIELD(TB_FLAGS, PM_BASE_ENABLED, 23, 1)
> >>   FIELD(TB_FLAGS, VTA, 24, 1)
> >>   FIELD(TB_FLAGS, VMA, 25, 1)
> >> +/* Native debug itrigger */
> >> +FIELD(TB_FLAGS, ITRIGGER, 26, 1)
> >>
> >>   #ifdef TARGET_RISCV32
> >>   #define riscv_cpu_mxl(env)  ((void)(env), MXL_RV32)
> >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> >> index 278d163803..263282f230 100644
> >> --- a/target/riscv/cpu_helper.c
> >> +++ b/target/riscv/cpu_helper.c
> >> @@ -27,7 +27,9 @@
> >>   #include "tcg/tcg-op.h"
> >>   #include "trace.h"
> >>   #include "semihosting/common-semi.h"
> >> +#include "sysemu/cpu-timers.h"
> >>   #include "cpu_bits.h"
> >> +#include "debug.h"
> >>
> >>   int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
> >>   {
> >> @@ -103,6 +105,10 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
> >>           flags = FIELD_DP32(flags, TB_FLAGS, MSTATUS_HS_VS,
> >>                              get_field(env->mstatus_hs, MSTATUS_VS));
> >>       }
> >> +    if (riscv_feature(env, RISCV_FEATURE_DEBUG) && !icount_enabled()) {
> >> +        flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER,
> >> +                           riscv_itrigger_enabled(env));
> >> +    }
> >>   #endif
> >>
> >>       flags = FIELD_DP32(flags, TB_FLAGS, XL, env->xl);
> >> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> >> index 26ea764407..45a3537d5c 100644
> >> --- a/target/riscv/debug.c
> >> +++ b/target/riscv/debug.c
> >> @@ -29,6 +29,7 @@
> >>   #include "cpu.h"
> >>   #include "trace.h"
> >>   #include "exec/exec-all.h"
> >> +#include "exec/helper-proto.h"
> >>
> >>   /*
> >>    * The following M-mode trigger CSRs are implemented:
> >> @@ -498,6 +499,76 @@ static void type6_reg_write(CPURISCVState *env, target_ulong index,
> >>       return;
> >>   }
> >>
> >> +/* icount trigger type */
> >> +static inline int
> >> +itrigger_get_count(CPURISCVState *env, int index)
> >> +{
> >> +    return get_field(env->tdata1[index], ITRIGGER_COUNT);
> >> +}
> >> +
> >> +static inline void
> >> +itrigger_set_count(CPURISCVState *env, int index, int value)
> >> +{
> >> +    env->tdata1[index] = set_field(env->tdata1[index],
> >> +                                   ITRIGGER_COUNT, value);
> >> +}
> >> +
> >> +static bool check_itrigger_priv(CPURISCVState *env, int index)
> >> +{
> >> +    target_ulong tdata1 = env->tdata1[index];
> >> +    if (riscv_cpu_virt_enabled(env)) {
> >> +        /* check VU/VS bit against current privilege level */
> >> +        return (get_field(tdata1, ITRIGGER_VS) == env->priv) ||
> >> +               (get_field(tdata1, ITRIGGER_VU) == env->priv);
> >> +    } else {
> >> +        /* check U/S/M bit against current privilege level */
> >> +        return (get_field(tdata1, ITRIGGER_M) == env->priv) ||
> >> +               (get_field(tdata1, ITRIGGER_S) == env->priv) ||
> >> +               (get_field(tdata1, ITRIGGER_U) == env->priv);
> >> +    }
> >> +}
> >> +
> >> +bool riscv_itrigger_enabled(CPURISCVState *env)
> >> +{
> >> +    int count;
> >> +    for (int i = 0; i < RV_MAX_TRIGGERS; i++) {
> >> +        if (get_trigger_type(env, i) != TRIGGER_TYPE_INST_CNT) {
> >> +            continue;
> >> +        }
> >> +        if (check_itrigger_priv(env, i)) {
> >> +            continue;
> >> +        }
> >> +        count = itrigger_get_count(env, i);
> >> +        if (!count) {
> >> +            continue;
> >> +        }
> >> +        return true;
> >> +    }
> >> +
> >> +    return false;
> >> +}
> >> +
> >> +void helper_itrigger_match(CPURISCVState *env)
> >> +{
> >> +    int count;
> >> +    for (int i = 0; i < RV_MAX_TRIGGERS; i++) {
> >> +        if (get_trigger_type(env, i) != TRIGGER_TYPE_INST_CNT) {
> >> +            continue;
> >> +        }
> >> +        if (check_itrigger_priv(env, i)) {
> >> +            continue;
> >> +        }
> >> +        count = itrigger_get_count(env, i);
> >> +        if (!count) {
> >> +            continue;
> >> +        }
> >> +        itrigger_set_count(env, i, count--);
> >> +        if (!count) {
> >> +            do_trigger_action(env, i);
> >> +        }
> >> +    }
> >> +}
> >> +
> >>   target_ulong tdata_csr_read(CPURISCVState *env, int tdata_index)
> >>   {
> >>       switch (tdata_index) {
> >> diff --git a/target/riscv/debug.h b/target/riscv/debug.h
> >> index a1226b4d29..cc3358e69b 100644
> >> --- a/target/riscv/debug.h
> >> +++ b/target/riscv/debug.h
> >> @@ -118,6 +118,17 @@ enum {
> >>       SIZE_NUM = 16
> >>   };
> >>
> >> +/* itrigger filed masks */
> >> +#define ITRIGGER_ACTION       0x3f
> >> +#define ITRIGGER_U            BIT(6)
> >> +#define ITRIGGER_S            BIT(7)
> >> +#define ITRIGGER_PENDING      BIT(8)
> >> +#define ITRIGGER_M            BIT(9)
> >> +#define ITRIGGER_COUNT        (0x3fff << 10)
> >> +#define ITRIGGER_HIT          BIT(24)
> >> +#define ITRIGGER_VU           BIT(25)
> >> +#define ITRIGGER_VS           BIT(26)
> >> +
> >>   bool tdata_available(CPURISCVState *env, int tdata_index);
> >>
> >>   target_ulong tselect_csr_read(CPURISCVState *env);
> >> @@ -134,4 +145,5 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp);
> >>
> >>   void riscv_trigger_init(CPURISCVState *env);
> >>
> >> +bool riscv_itrigger_enabled(CPURISCVState *env);
> >>   #endif /* RISCV_DEBUG_H */
> >> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
> >> index a03014fe67..227c7122ef 100644
> >> --- a/target/riscv/helper.h
> >> +++ b/target/riscv/helper.h
> >> @@ -109,6 +109,8 @@ DEF_HELPER_1(sret, tl, env)
> >>   DEF_HELPER_1(mret, tl, env)
> >>   DEF_HELPER_1(wfi, void, env)
> >>   DEF_HELPER_1(tlb_flush, void, env)
> >> +/* Native Debug */
> >> +DEF_HELPER_1(itrigger_match, void, env)
> >>   #endif
> >>
> >>   /* Hypervisor functions */
> >> diff --git a/target/riscv/insn_trans/trans_privileged.c.inc b/target/riscv/insn_trans/trans_privileged.c.inc
> >> index 3281408a87..59501b2780 100644
> >> --- a/target/riscv/insn_trans/trans_privileged.c.inc
> >> +++ b/target/riscv/insn_trans/trans_privileged.c.inc
> >> @@ -78,7 +78,7 @@ static bool trans_sret(DisasContext *ctx, arg_sret *a)
> >>       if (has_ext(ctx, RVS)) {
> >>           decode_save_opc(ctx);
> >>           gen_helper_sret(cpu_pc, cpu_env);
> >> -        tcg_gen_exit_tb(NULL, 0); /* no chaining */
> >> +        exit_tb(ctx); /* no chaining */
> >>           ctx->base.is_jmp = DISAS_NORETURN;
> >>       } else {
> >>           return false;
> >> @@ -94,7 +94,7 @@ static bool trans_mret(DisasContext *ctx, arg_mret *a)
> >>   #ifndef CONFIG_USER_ONLY
> >>       decode_save_opc(ctx);
> >>       gen_helper_mret(cpu_pc, cpu_env);
> >> -    tcg_gen_exit_tb(NULL, 0); /* no chaining */
> >> +    exit_tb(ctx); /* no chaining */
> >>       ctx->base.is_jmp = DISAS_NORETURN;
> >>       return true;
> >>   #else
> >> diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
> >> index c49dbec0eb..5c69b88d1e 100644
> >> --- a/target/riscv/insn_trans/trans_rvi.c.inc
> >> +++ b/target/riscv/insn_trans/trans_rvi.c.inc
> >> @@ -66,7 +66,7 @@ static bool trans_jalr(DisasContext *ctx, arg_jalr *a)
> >>       }
> >>
> >>       gen_set_gpri(ctx, a->rd, ctx->pc_succ_insn);
> >> -    tcg_gen_lookup_and_goto_ptr();
> >> +    lookup_and_goto_ptr(ctx);
> >>
> >>       if (misaligned) {
> >>           gen_set_label(misaligned);
> >> @@ -803,7 +803,7 @@ static bool trans_pause(DisasContext *ctx, arg_pause *a)
> >>        * end the TB and return to main loop
> >>        */
> >>       gen_set_pc_imm(ctx, ctx->pc_succ_insn);
> >> -    tcg_gen_exit_tb(NULL, 0);
> >> +    exit_tb(ctx);
> >>       ctx->base.is_jmp = DISAS_NORETURN;
> >>
> >>       return true;
> >> @@ -827,7 +827,7 @@ static bool trans_fence_i(DisasContext *ctx, arg_fence_i *a)
> >>        * however we need to end the translation block
> >>        */
> >>       gen_set_pc_imm(ctx, ctx->pc_succ_insn);
> >> -    tcg_gen_exit_tb(NULL, 0);
> >> +    exit_tb(ctx);
> >>       ctx->base.is_jmp = DISAS_NORETURN;
> >>       return true;
> >>   }
> >> @@ -838,7 +838,7 @@ static bool do_csr_post(DisasContext *ctx)
> >>       decode_save_opc(ctx);
> >>       /* We may have changed important cpu state -- exit to main loop. */
> >>       gen_set_pc_imm(ctx, ctx->pc_succ_insn);
> >> -    tcg_gen_exit_tb(NULL, 0);
> >> +    exit_tb(ctx);
> >>       ctx->base.is_jmp = DISAS_NORETURN;
> >>       return true;
> >>   }
> >> diff --git a/target/riscv/insn_trans/trans_rvv.c.inc b/target/riscv/insn_trans/trans_rvv.c.inc
> >> index 4dea4413ae..d455acedbf 100644
> >> --- a/target/riscv/insn_trans/trans_rvv.c.inc
> >> +++ b/target/riscv/insn_trans/trans_rvv.c.inc
> >> @@ -196,7 +196,7 @@ static bool do_vsetvl(DisasContext *s, int rd, int rs1, TCGv s2)
> >>       mark_vs_dirty(s);
> >>
> >>       gen_set_pc_imm(s, s->pc_succ_insn);
> >> -    tcg_gen_lookup_and_goto_ptr();
> >> +    lookup_and_goto_ptr(s);
> >>       s->base.is_jmp = DISAS_NORETURN;
> >>
> >>       if (rd == 0 && rs1 == 0) {
> >> @@ -222,7 +222,7 @@ static bool do_vsetivli(DisasContext *s, int rd, TCGv s1, TCGv s2)
> >>       gen_set_gpr(s, rd, dst);
> >>       mark_vs_dirty(s);
> >>       gen_set_pc_imm(s, s->pc_succ_insn);
> >> -    tcg_gen_lookup_and_goto_ptr();
> >> +    lookup_and_goto_ptr(s);
> >>       s->base.is_jmp = DISAS_NORETURN;
> >>
> >>       return true;
> >> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> >> index db123da5ec..d704265a37 100644
> >> --- a/target/riscv/translate.c
> >> +++ b/target/riscv/translate.c
> >> @@ -111,6 +111,8 @@ typedef struct DisasContext {
> >>       /* PointerMasking extension */
> >>       bool pm_mask_enabled;
> >>       bool pm_base_enabled;
> >> +    /* Use icount trigger for native debug */
> >> +    bool itrigger;
> >>       /* TCG of the current insn_start */
> >>       TCGOp *insn_start;
> >>   } DisasContext;
> >> @@ -252,15 +254,39 @@ static void gen_exception_inst_addr_mis(DisasContext *ctx)
> >>       generate_exception(ctx, RISCV_EXCP_INST_ADDR_MIS);
> >>   }
> >>
> >> +static void lookup_and_goto_ptr(DisasContext *ctx)
> >> +{
> >> +#ifndef CONFIG_USER_ONLY
> >> +    if (ctx->itrigger) {
> >> +        gen_helper_itrigger_match(cpu_env);
> >> +    }
> >> +#endif
> >> +    tcg_gen_lookup_and_goto_ptr();
> >> +}
> >> +
> >> +static void exit_tb(DisasContext *ctx)
> >> +{
> >> +#ifndef CONFIG_USER_ONLY
> >> +    if (ctx->itrigger) {
> >> +        gen_helper_itrigger_match(cpu_env);
> >> +    }
> >> +#endif
> >> +    tcg_gen_exit_tb(NULL, 0);
> >> +}
> >> +
> >>   static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
> >>   {
> >> -    if (translator_use_goto_tb(&ctx->base, dest)) {
> >> +     /*
> >> +      * Under itrigger, instruction executes one by one like singlestep,
> >> +      * direct block chain benefits will be small.
> >> +      */
> >> +    if (translator_use_goto_tb(&ctx->base, dest) && !ctx->itrigger) {
> >>           tcg_gen_goto_tb(n);
> >>           gen_set_pc_imm(ctx, dest);
> >>           tcg_gen_exit_tb(ctx->base.tb, n);
> >>       } else {
> >>           gen_set_pc_imm(ctx, dest);
> >> -        tcg_gen_lookup_and_goto_ptr();
> >> +        lookup_and_goto_ptr(ctx);
> >>       }
> >>   }
> >>
> >> @@ -1136,6 +1162,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
> >>       memset(ctx->ftemp, 0, sizeof(ctx->ftemp));
> >>       ctx->pm_mask_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_MASK_ENABLED);
> >>       ctx->pm_base_enabled = FIELD_EX32(tb_flags, TB_FLAGS, PM_BASE_ENABLED);
> >> +    ctx->itrigger = FIELD_EX32(tb_flags, TB_FLAGS, ITRIGGER);
> >>       ctx->zero = tcg_constant_tl(0);
> >>   }
> >>
> >> @@ -1175,7 +1202,7 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
> >>
> >>       /* Only the first insn within a TB is allowed to cross a page boundary. */
> >>       if (ctx->base.is_jmp == DISAS_NEXT) {
> >> -        if (!is_same_page(&ctx->base, ctx->base.pc_next)) {
> >> +        if (ctx->itrigger || !is_same_page(&ctx->base, ctx->base.pc_next)) {
> >>               ctx->base.is_jmp = DISAS_TOO_MANY;
> >>           } else {
> >>               unsigned page_ofs = ctx->base.pc_next & ~TARGET_PAGE_MASK;
> >> --
> >> 2.17.1
> >>
> >>

[PATCH v1 2/4] target/riscv: Add itrigger support when icount is enabled

[LIU Zhiwei]

The max count in itrigger can be 0x3FFF, which will cause a no trivial
translation and execution overload.

When icount is enabled, QEMU provides API that can fetch guest
instruction number. Thus, we can set an timer for itrigger with
the count as deadline.

Only when timer expires or priviledge mode changes, do lazy update
to count.

Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
 target/riscv/cpu.h        |  2 ++
 target/riscv/cpu_helper.c |  3 ++
 target/riscv/debug.c      | 59 +++++++++++++++++++++++++++++++++++++++
 target/riscv/debug.h      |  1 +
 4 files changed, 65 insertions(+)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 24bafda27d..13ca0f20ae 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -329,6 +329,8 @@ struct CPUArchState {
     target_ulong tdata3[RV_MAX_TRIGGERS];
     struct CPUBreakpoint *cpu_breakpoint[RV_MAX_TRIGGERS];
     struct CPUWatchpoint *cpu_watchpoint[RV_MAX_TRIGGERS];
+    QEMUTimer *itrigger_timer[RV_MAX_TRIGGERS];
+    int64_t last_icount;
 
     /* machine specific rdtime callback */
     uint64_t (*rdtime_fn)(void *);
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 263282f230..7d8089b218 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -676,6 +676,9 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv)
     if (newpriv == PRV_H) {
         newpriv = PRV_U;
     }
+    if (icount_enabled() && newpriv != env->priv) {
+        riscv_itrigger_update_priv(env);
+    }
     /* tlb_flush is unnecessary as mode is contained in mmu_idx */
     env->priv = newpriv;
     env->xl = cpu_recompute_xl(env);
diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index 45a3537d5c..5ff70430a1 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -30,6 +30,7 @@
 #include "trace.h"
 #include "exec/exec-all.h"
 #include "exec/helper-proto.h"
+#include "sysemu/cpu-timers.h"
 
 /*
  * The following M-mode trigger CSRs are implemented:
@@ -569,6 +570,62 @@ void helper_itrigger_match(CPURISCVState *env)
     }
 }
 
+static void riscv_itrigger_update_count(CPURISCVState *env)
+{
+    int count, executed;
+    /*
+     * Record last icount, so that we can evaluate the executed instructions
+     * since last priviledge mode change or timer expire.
+     */
+    int64_t last_icount = env->last_icount, current_icount;
+    current_icount = env->last_icount = icount_get_raw();
+
+    for (int i = 0; i < RV_MAX_TRIGGERS; i++) {
+        if (get_trigger_type(env, i) != TRIGGER_TYPE_INST_CNT) {
+            continue;
+        }
+        count = itrigger_get_count(env, i);
+        if (!count) {
+            continue;
+        }
+        /*
+         * Only when priviledge is changed or itrigger timer expires,
+         * the count field in itrigger tdata1 register is updated.
+         * And the count field in itrigger only contains remaining value.
+         */
+        if (check_itrigger_priv(env, i)) {
+            /*
+             * If itrigger enabled in this priviledge mode, the number of
+             * executed instructions since last priviledge change
+             * should be reduced from current itrigger count.
+             */
+            executed = current_icount - last_icount;
+            itrigger_set_count(env, i, count - executed);
+            if (count == executed) {
+                do_trigger_action(env, i);
+            }
+        } else {
+            /*
+             * If itrigger is not enabled in this priviledge mode,
+             * the number of executed instructions will be discard and
+             * the count field in itrigger will not change.
+             */
+            timer_mod(env->itrigger_timer[i],
+                      current_icount + count);
+        }
+    }
+}
+
+static void riscv_itrigger_timer_cb(void *opaque)
+{
+    riscv_itrigger_update_count((CPURISCVState *)opaque);
+}
+
+void riscv_itrigger_update_priv(CPURISCVState *env)
+{
+    riscv_itrigger_update_count(env);
+}
+
 target_ulong tdata_csr_read(CPURISCVState *env, int tdata_index)
 {
     switch (tdata_index) {
@@ -798,5 +855,7 @@ void riscv_trigger_init(CPURISCVState *env)
         env->tdata3[i] = 0;
         env->cpu_breakpoint[i] = NULL;
         env->cpu_watchpoint[i] = NULL;
+        env->itrigger_timer[i] = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+                                              riscv_itrigger_timer_cb, env);
     }
 }
diff --git a/target/riscv/debug.h b/target/riscv/debug.h
index cc3358e69b..c471748d5a 100644
--- a/target/riscv/debug.h
+++ b/target/riscv/debug.h
@@ -146,4 +146,5 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp);
 void riscv_trigger_init(CPURISCVState *env);
 
 bool riscv_itrigger_enabled(CPURISCVState *env);
+void riscv_itrigger_update_priv(CPURISCVState *env);
 #endif /* RISCV_DEBUG_H */
-- 
2.17.1

Re: [PATCH v1 2/4] target/riscv: Add itrigger support when icount is enabled

[Alistair Francis]

On Thu, Oct 13, 2022 at 4:43 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>
> The max count in itrigger can be 0x3FFF, which will cause a no trivial
> translation and execution overload.
>
> When icount is enabled, QEMU provides API that can fetch guest
> instruction number. Thus, we can set an timer for itrigger with
> the count as deadline.
>
> Only when timer expires or priviledge mode changes, do lazy update
> to count.
>
> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu.h        |  2 ++
>  target/riscv/cpu_helper.c |  3 ++
>  target/riscv/debug.c      | 59 +++++++++++++++++++++++++++++++++++++++
>  target/riscv/debug.h      |  1 +
>  4 files changed, 65 insertions(+)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 24bafda27d..13ca0f20ae 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -329,6 +329,8 @@ struct CPUArchState {
>      target_ulong tdata3[RV_MAX_TRIGGERS];
>      struct CPUBreakpoint *cpu_breakpoint[RV_MAX_TRIGGERS];
>      struct CPUWatchpoint *cpu_watchpoint[RV_MAX_TRIGGERS];
> +    QEMUTimer *itrigger_timer[RV_MAX_TRIGGERS];
> +    int64_t last_icount;
>
>      /* machine specific rdtime callback */
>      uint64_t (*rdtime_fn)(void *);
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 263282f230..7d8089b218 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -676,6 +676,9 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv)
>      if (newpriv == PRV_H) {
>          newpriv = PRV_U;
>      }
> +    if (icount_enabled() && newpriv != env->priv) {
> +        riscv_itrigger_update_priv(env);
> +    }
>      /* tlb_flush is unnecessary as mode is contained in mmu_idx */
>      env->priv = newpriv;
>      env->xl = cpu_recompute_xl(env);
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index 45a3537d5c..5ff70430a1 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -30,6 +30,7 @@
>  #include "trace.h"
>  #include "exec/exec-all.h"
>  #include "exec/helper-proto.h"
> +#include "sysemu/cpu-timers.h"
>
>  /*
>   * The following M-mode trigger CSRs are implemented:
> @@ -569,6 +570,62 @@ void helper_itrigger_match(CPURISCVState *env)
>      }
>  }
>
> +static void riscv_itrigger_update_count(CPURISCVState *env)
> +{
> +    int count, executed;
> +    /*
> +     * Record last icount, so that we can evaluate the executed instructions
> +     * since last priviledge mode change or timer expire.
> +     */
> +    int64_t last_icount = env->last_icount, current_icount;
> +    current_icount = env->last_icount = icount_get_raw();
> +
> +    for (int i = 0; i < RV_MAX_TRIGGERS; i++) {
> +        if (get_trigger_type(env, i) != TRIGGER_TYPE_INST_CNT) {
> +            continue;
> +        }
> +        count = itrigger_get_count(env, i);
> +        if (!count) {
> +            continue;
> +        }
> +        /*
> +         * Only when priviledge is changed or itrigger timer expires,
> +         * the count field in itrigger tdata1 register is updated.
> +         * And the count field in itrigger only contains remaining value.
> +         */
> +        if (check_itrigger_priv(env, i)) {
> +            /*
> +             * If itrigger enabled in this priviledge mode, the number of
> +             * executed instructions since last priviledge change
> +             * should be reduced from current itrigger count.
> +             */
> +            executed = current_icount - last_icount;
> +            itrigger_set_count(env, i, count - executed);
> +            if (count == executed) {
> +                do_trigger_action(env, i);
> +            }
> +        } else {
> +            /*
> +             * If itrigger is not enabled in this priviledge mode,
> +             * the number of executed instructions will be discard and
> +             * the count field in itrigger will not change.
> +             */
> +            timer_mod(env->itrigger_timer[i],
> +                      current_icount + count);
> +        }
> +    }
> +}
> +
> +static void riscv_itrigger_timer_cb(void *opaque)
> +{
> +    riscv_itrigger_update_count((CPURISCVState *)opaque);
> +}
> +
> +void riscv_itrigger_update_priv(CPURISCVState *env)
> +{
> +    riscv_itrigger_update_count(env);
> +}
> +
>  target_ulong tdata_csr_read(CPURISCVState *env, int tdata_index)
>  {
>      switch (tdata_index) {
> @@ -798,5 +855,7 @@ void riscv_trigger_init(CPURISCVState *env)
>          env->tdata3[i] = 0;
>          env->cpu_breakpoint[i] = NULL;
>          env->cpu_watchpoint[i] = NULL;
> +        env->itrigger_timer[i] = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> +                                              riscv_itrigger_timer_cb, env);
>      }
>  }
> diff --git a/target/riscv/debug.h b/target/riscv/debug.h
> index cc3358e69b..c471748d5a 100644
> --- a/target/riscv/debug.h
> +++ b/target/riscv/debug.h
> @@ -146,4 +146,5 @@ bool riscv_cpu_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp);
>  void riscv_trigger_init(CPURISCVState *env);
>
>  bool riscv_itrigger_enabled(CPURISCVState *env);
> +void riscv_itrigger_update_priv(CPURISCVState *env);
>  #endif /* RISCV_DEBUG_H */
> --
> 2.17.1
>
>

[PATCH v1 3/4] target/riscv: Enable native debug itrigger

[LIU Zhiwei]

When QEMU is not in icount mode, execute instruction one by one. The
tdata1 can be read directly.

When QEMU is in icount mode, use a timer to simulate the itrigger. The
tdata1 may be not right because of lazy update of count in tdata1. Thus,
We should pack the adjusted count into tdata1 before read it back.

Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
 target/riscv/debug.c | 72 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index 5ff70430a1..db7745d4a3 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -626,10 +626,80 @@ void riscv_itrigger_update_priv(CPURISCVState *env)
     riscv_itrigger_update_count(env);
 }
 
+static target_ulong itrigger_validate(CPURISCVState *env,
+                                      target_ulong ctrl)
+{
+    target_ulong val;
+
+    /* validate the generic part first */
+    val = tdata1_validate(env, ctrl, TRIGGER_TYPE_INST_CNT);
+
+    /* validate unimplemented (always zero) bits */
+    warn_always_zero_bit(ctrl, ITRIGGER_ACTION, "action");
+    warn_always_zero_bit(ctrl, ITRIGGER_HIT, "hit");
+    warn_always_zero_bit(ctrl, ITRIGGER_PENDING, "pending");
+
+    /* keep the mode and attribute bits */
+    val |= ctrl & (ITRIGGER_VU | ITRIGGER_VS | ITRIGGER_U | ITRIGGER_S |
+                   ITRIGGER_M | ITRIGGER_COUNT);
+
+    return val;
+}
+
+static void itrigger_reg_write(CPURISCVState *env, target_ulong index,
+                               int tdata_index, target_ulong val)
+{
+    target_ulong new_val;
+
+    switch (tdata_index) {
+    case TDATA1:
+        /* set timer for icount */
+        new_val = itrigger_validate(env, val);
+        if (new_val != env->tdata1[index]) {
+            env->tdata1[index] = new_val;
+            if (icount_enabled()) {
+                env->last_icount = icount_get_raw();
+                /* set the count to timer */
+                timer_mod(env->itrigger_timer[index],
+                          env->last_icount + itrigger_get_count(env, index));
+            }
+        }
+        break;
+    case TDATA2:
+        qemu_log_mask(LOG_UNIMP,
+                      "tdata2 is not supported for icount trigger\n");
+        break;
+    case TDATA3:
+        qemu_log_mask(LOG_UNIMP,
+                      "tdata3 is not supported for icount trigger\n");
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    return;
+}
+
+static int itrigger_get_adjust_count(CPURISCVState *env)
+{
+    int count = itrigger_get_count(env, env->trigger_cur), executed;
+    if ((count != 0) && check_itrigger_priv(env, env->trigger_cur)) {
+        executed = icount_get_raw() - env->last_icount;
+        count += executed;
+    }
+    return count;
+}
+
 target_ulong tdata_csr_read(CPURISCVState *env, int tdata_index)
 {
+    int trigger_type;
     switch (tdata_index) {
     case TDATA1:
+        trigger_type = extract_trigger_type(env, env->tdata1[env->trigger_cur]);
+        if ((trigger_type == TRIGGER_TYPE_INST_CNT) && icount_enabled()) {
+            return deposit64(env->tdata1[env->trigger_cur], 10, 14,
+                             itrigger_get_adjust_count(env));
+        }
         return env->tdata1[env->trigger_cur];
     case TDATA2:
         return env->tdata2[env->trigger_cur];
@@ -658,6 +728,8 @@ void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val)
         type6_reg_write(env, env->trigger_cur, tdata_index, val);
         break;
     case TRIGGER_TYPE_INST_CNT:
+        itrigger_reg_write(env, env->trigger_cur, tdata_index, val);
+        break;
     case TRIGGER_TYPE_INT:
     case TRIGGER_TYPE_EXCP:
     case TRIGGER_TYPE_EXT_SRC:
-- 
2.17.1

Re: [PATCH v1 3/4] target/riscv: Enable native debug itrigger

[Alistair Francis]

On Thu, Oct 13, 2022 at 4:38 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>
> When QEMU is not in icount mode, execute instruction one by one. The
> tdata1 can be read directly.
>
> When QEMU is in icount mode, use a timer to simulate the itrigger. The
> tdata1 may be not right because of lazy update of count in tdata1. Thus,
> We should pack the adjusted count into tdata1 before read it back.
>
> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/debug.c | 72 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
>
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index 5ff70430a1..db7745d4a3 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -626,10 +626,80 @@ void riscv_itrigger_update_priv(CPURISCVState *env)
>      riscv_itrigger_update_count(env);
>  }
>
> +static target_ulong itrigger_validate(CPURISCVState *env,
> +                                      target_ulong ctrl)
> +{
> +    target_ulong val;
> +
> +    /* validate the generic part first */
> +    val = tdata1_validate(env, ctrl, TRIGGER_TYPE_INST_CNT);
> +
> +    /* validate unimplemented (always zero) bits */
> +    warn_always_zero_bit(ctrl, ITRIGGER_ACTION, "action");
> +    warn_always_zero_bit(ctrl, ITRIGGER_HIT, "hit");
> +    warn_always_zero_bit(ctrl, ITRIGGER_PENDING, "pending");
> +
> +    /* keep the mode and attribute bits */
> +    val |= ctrl & (ITRIGGER_VU | ITRIGGER_VS | ITRIGGER_U | ITRIGGER_S |
> +                   ITRIGGER_M | ITRIGGER_COUNT);
> +
> +    return val;
> +}
> +
> +static void itrigger_reg_write(CPURISCVState *env, target_ulong index,
> +                               int tdata_index, target_ulong val)
> +{
> +    target_ulong new_val;
> +
> +    switch (tdata_index) {
> +    case TDATA1:
> +        /* set timer for icount */
> +        new_val = itrigger_validate(env, val);
> +        if (new_val != env->tdata1[index]) {
> +            env->tdata1[index] = new_val;
> +            if (icount_enabled()) {
> +                env->last_icount = icount_get_raw();
> +                /* set the count to timer */
> +                timer_mod(env->itrigger_timer[index],
> +                          env->last_icount + itrigger_get_count(env, index));
> +            }
> +        }
> +        break;
> +    case TDATA2:
> +        qemu_log_mask(LOG_UNIMP,
> +                      "tdata2 is not supported for icount trigger\n");
> +        break;
> +    case TDATA3:
> +        qemu_log_mask(LOG_UNIMP,
> +                      "tdata3 is not supported for icount trigger\n");
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +
> +    return;
> +}
> +
> +static int itrigger_get_adjust_count(CPURISCVState *env)
> +{
> +    int count = itrigger_get_count(env, env->trigger_cur), executed;
> +    if ((count != 0) && check_itrigger_priv(env, env->trigger_cur)) {
> +        executed = icount_get_raw() - env->last_icount;
> +        count += executed;
> +    }
> +    return count;
> +}
> +
>  target_ulong tdata_csr_read(CPURISCVState *env, int tdata_index)
>  {
> +    int trigger_type;
>      switch (tdata_index) {
>      case TDATA1:
> +        trigger_type = extract_trigger_type(env, env->tdata1[env->trigger_cur]);
> +        if ((trigger_type == TRIGGER_TYPE_INST_CNT) && icount_enabled()) {
> +            return deposit64(env->tdata1[env->trigger_cur], 10, 14,
> +                             itrigger_get_adjust_count(env));
> +        }
>          return env->tdata1[env->trigger_cur];
>      case TDATA2:
>          return env->tdata2[env->trigger_cur];
> @@ -658,6 +728,8 @@ void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val)
>          type6_reg_write(env, env->trigger_cur, tdata_index, val);
>          break;
>      case TRIGGER_TYPE_INST_CNT:
> +        itrigger_reg_write(env, env->trigger_cur, tdata_index, val);
> +        break;
>      case TRIGGER_TYPE_INT:
>      case TRIGGER_TYPE_EXCP:
>      case TRIGGER_TYPE_EXT_SRC:
> --
> 2.17.1
>
>

[PATCH v1 4/4] target/riscv: Add itrigger_enabled field to CPURISCVState

[LIU Zhiwei]

Avoid calling riscv_itrigger_enabled() when calculate the tbflags.
As the itrigger enable status can only be changed when write
tdata1, migration load or itrigger fire, update env->itrigger_enabled
at these places.

Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
 target/riscv/cpu.h        |  1 +
 target/riscv/cpu_helper.c |  3 +--
 target/riscv/debug.c      |  3 +++
 target/riscv/machine.c    | 15 +++++++++++++++
 4 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 13ca0f20ae..44499df9da 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -331,6 +331,7 @@ struct CPUArchState {
     struct CPUWatchpoint *cpu_watchpoint[RV_MAX_TRIGGERS];
     QEMUTimer *itrigger_timer[RV_MAX_TRIGGERS];
     int64_t last_icount;
+    bool itrigger_enabled;
 
     /* machine specific rdtime callback */
     uint64_t (*rdtime_fn)(void *);
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 7d8089b218..95c766aec0 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -106,8 +106,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
                            get_field(env->mstatus_hs, MSTATUS_VS));
     }
     if (riscv_feature(env, RISCV_FEATURE_DEBUG) && !icount_enabled()) {
-        flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER,
-                           riscv_itrigger_enabled(env));
+        flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER, env->itrigger_enabled);
     }
 #endif
 
diff --git a/target/riscv/debug.c b/target/riscv/debug.c
index db7745d4a3..2c0c8b18db 100644
--- a/target/riscv/debug.c
+++ b/target/riscv/debug.c
@@ -565,6 +565,7 @@ void helper_itrigger_match(CPURISCVState *env)
         }
         itrigger_set_count(env, i, count--);
         if (!count) {
+            env->itrigger_enabled = riscv_itrigger_enabled(env);
             do_trigger_action(env, i);
         }
     }
@@ -662,6 +663,8 @@ static void itrigger_reg_write(CPURISCVState *env, target_ulong index,
                 /* set the count to timer */
                 timer_mod(env->itrigger_timer[index],
                           env->last_icount + itrigger_get_count(env, index));
+            } else {
+                env->itrigger_enabled = riscv_itrigger_enabled(env);
             }
         }
         break;
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index c2a94a82b3..cd32a52e19 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -21,6 +21,8 @@
 #include "qemu/error-report.h"
 #include "sysemu/kvm.h"
 #include "migration/cpu.h"
+#include "sysemu/cpu-timers.h"
+#include "debug.h"
 
 static bool pmp_needed(void *opaque)
 {
@@ -229,11 +231,24 @@ static bool debug_needed(void *opaque)
     return riscv_feature(env, RISCV_FEATURE_DEBUG);
 }
 
+static int debug_post_load(void *opaque, int version_id)
+{
+    RISCVCPU *cpu = opaque;
+    CPURISCVState *env = &cpu->env;
+
+    if (icount_enabled()) {
+        env->itrigger_enabled = riscv_itrigger_enabled(env);
+    }
+
+    return 0;
+}
+
 static const VMStateDescription vmstate_debug = {
     .name = "cpu/debug",
     .version_id = 2,
     .minimum_version_id = 2,
     .needed = debug_needed,
+    .post_load = debug_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINTTL(env.trigger_cur, RISCVCPU),
         VMSTATE_UINTTL_ARRAY(env.tdata1, RISCVCPU, RV_MAX_TRIGGERS),
-- 
2.17.1

Re: [PATCH v1 4/4] target/riscv: Add itrigger_enabled field to CPURISCVState

[Alistair Francis]

On Thu, Oct 13, 2022 at 4:51 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>
> Avoid calling riscv_itrigger_enabled() when calculate the tbflags.
> As the itrigger enable status can only be changed when write
> tdata1, migration load or itrigger fire, update env->itrigger_enabled
> at these places.
>
> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> ---
>  target/riscv/cpu.h        |  1 +
>  target/riscv/cpu_helper.c |  3 +--
>  target/riscv/debug.c      |  3 +++
>  target/riscv/machine.c    | 15 +++++++++++++++
>  4 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 13ca0f20ae..44499df9da 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -331,6 +331,7 @@ struct CPUArchState {
>      struct CPUWatchpoint *cpu_watchpoint[RV_MAX_TRIGGERS];
>      QEMUTimer *itrigger_timer[RV_MAX_TRIGGERS];
>      int64_t last_icount;
> +    bool itrigger_enabled;
>
>      /* machine specific rdtime callback */
>      uint64_t (*rdtime_fn)(void *);
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 7d8089b218..95c766aec0 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -106,8 +106,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
>                             get_field(env->mstatus_hs, MSTATUS_VS));
>      }
>      if (riscv_feature(env, RISCV_FEATURE_DEBUG) && !icount_enabled()) {
> -        flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER,
> -                           riscv_itrigger_enabled(env));
> +        flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER, env->itrigger_enabled);
>      }
>  #endif
>
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index db7745d4a3..2c0c8b18db 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -565,6 +565,7 @@ void helper_itrigger_match(CPURISCVState *env)
>          }
>          itrigger_set_count(env, i, count--);
>          if (!count) {
> +            env->itrigger_enabled = riscv_itrigger_enabled(env);
>              do_trigger_action(env, i);
>          }
>      }
> @@ -662,6 +663,8 @@ static void itrigger_reg_write(CPURISCVState *env, target_ulong index,
>                  /* set the count to timer */
>                  timer_mod(env->itrigger_timer[index],
>                            env->last_icount + itrigger_get_count(env, index));
> +            } else {
> +                env->itrigger_enabled = riscv_itrigger_enabled(env);
>              }
>          }
>          break;
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index c2a94a82b3..cd32a52e19 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -21,6 +21,8 @@
>  #include "qemu/error-report.h"
>  #include "sysemu/kvm.h"
>  #include "migration/cpu.h"
> +#include "sysemu/cpu-timers.h"
> +#include "debug.h"
>
>  static bool pmp_needed(void *opaque)
>  {
> @@ -229,11 +231,24 @@ static bool debug_needed(void *opaque)
>      return riscv_feature(env, RISCV_FEATURE_DEBUG);
>  }
>
> +static int debug_post_load(void *opaque, int version_id)
> +{
> +    RISCVCPU *cpu = opaque;
> +    CPURISCVState *env = &cpu->env;
> +
> +    if (icount_enabled()) {
> +        env->itrigger_enabled = riscv_itrigger_enabled(env);
> +    }
> +
> +    return 0;
> +}
> +
>  static const VMStateDescription vmstate_debug = {
>      .name = "cpu/debug",
>      .version_id = 2,
>      .minimum_version_id = 2,

The versions here should be bumped

Alistair

>      .needed = debug_needed,
> +    .post_load = debug_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINTTL(env.trigger_cur, RISCVCPU),
>          VMSTATE_UINTTL_ARRAY(env.tdata1, RISCVCPU, RV_MAX_TRIGGERS),
> --
> 2.17.1
>
>

Re: [PATCH v1 4/4] target/riscv: Add itrigger_enabled field to CPURISCVState

[LIU Zhiwei]

On 2022/11/10 6:55, Alistair Francis wrote:
> On Thu, Oct 13, 2022 at 4:51 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>> Avoid calling riscv_itrigger_enabled() when calculate the tbflags.
>> As the itrigger enable status can only be changed when write
>> tdata1, migration load or itrigger fire, update env->itrigger_enabled
>> at these places.
>>
>> Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>> ---
>>   target/riscv/cpu.h        |  1 +
>>   target/riscv/cpu_helper.c |  3 +--
>>   target/riscv/debug.c      |  3 +++
>>   target/riscv/machine.c    | 15 +++++++++++++++
>>   4 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 13ca0f20ae..44499df9da 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -331,6 +331,7 @@ struct CPUArchState {
>>       struct CPUWatchpoint *cpu_watchpoint[RV_MAX_TRIGGERS];
>>       QEMUTimer *itrigger_timer[RV_MAX_TRIGGERS];
>>       int64_t last_icount;
>> +    bool itrigger_enabled;
>>
>>       /* machine specific rdtime callback */
>>       uint64_t (*rdtime_fn)(void *);
>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> index 7d8089b218..95c766aec0 100644
>> --- a/target/riscv/cpu_helper.c
>> +++ b/target/riscv/cpu_helper.c
>> @@ -106,8 +106,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
>>                              get_field(env->mstatus_hs, MSTATUS_VS));
>>       }
>>       if (riscv_feature(env, RISCV_FEATURE_DEBUG) && !icount_enabled()) {
>> -        flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER,
>> -                           riscv_itrigger_enabled(env));
>> +        flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER, env->itrigger_enabled);
>>       }
>>   #endif
>>
>> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
>> index db7745d4a3..2c0c8b18db 100644
>> --- a/target/riscv/debug.c
>> +++ b/target/riscv/debug.c
>> @@ -565,6 +565,7 @@ void helper_itrigger_match(CPURISCVState *env)
>>           }
>>           itrigger_set_count(env, i, count--);
>>           if (!count) {
>> +            env->itrigger_enabled = riscv_itrigger_enabled(env);
>>               do_trigger_action(env, i);
>>           }
>>       }
>> @@ -662,6 +663,8 @@ static void itrigger_reg_write(CPURISCVState *env, target_ulong index,
>>                   /* set the count to timer */
>>                   timer_mod(env->itrigger_timer[index],
>>                             env->last_icount + itrigger_get_count(env, index));
>> +            } else {
>> +                env->itrigger_enabled = riscv_itrigger_enabled(env);
>>               }
>>           }
>>           break;
>> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
>> index c2a94a82b3..cd32a52e19 100644
>> --- a/target/riscv/machine.c
>> +++ b/target/riscv/machine.c
>> @@ -21,6 +21,8 @@
>>   #include "qemu/error-report.h"
>>   #include "sysemu/kvm.h"
>>   #include "migration/cpu.h"
>> +#include "sysemu/cpu-timers.h"
>> +#include "debug.h"
>>
>>   static bool pmp_needed(void *opaque)
>>   {
>> @@ -229,11 +231,24 @@ static bool debug_needed(void *opaque)
>>       return riscv_feature(env, RISCV_FEATURE_DEBUG);
>>   }
>>
>> +static int debug_post_load(void *opaque, int version_id)
>> +{
>> +    RISCVCPU *cpu = opaque;
>> +    CPURISCVState *env = &cpu->env;
>> +
>> +    if (icount_enabled()) {
>> +        env->itrigger_enabled = riscv_itrigger_enabled(env);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static const VMStateDescription vmstate_debug = {
>>       .name = "cpu/debug",
>>       .version_id = 2,
>>       .minimum_version_id = 2,
> The versions here should be bumped

Hi Alistair and Richard,

I am not sure if we should bump versions when just add post_load 
callback without adding new fields.  I once upstreamed a patch
with a similar change but not bumping version.

Could you give some principles for bumping versions?

Thanks,
Zhiwei

> Alistair
>
>>       .needed = debug_needed,
>> +    .post_load = debug_post_load,
>>       .fields = (VMStateField[]) {
>>           VMSTATE_UINTTL(env.trigger_cur, RISCVCPU),
>>           VMSTATE_UINTTL_ARRAY(env.tdata1, RISCVCPU, RV_MAX_TRIGGERS),
>> --
>> 2.17.1
>>
>>

Re: [PATCH v1 4/4] target/riscv: Add itrigger_enabled field to CPURISCVState

[Richard Henderson]

On 11/10/22 13:15, LIU Zhiwei wrote:
>>> +static int debug_post_load(void *opaque, int version_id)
>>> +{
>>> +    RISCVCPU *cpu = opaque;
>>> +    CPURISCVState *env = &cpu->env;
>>> +
>>> +    if (icount_enabled()) {
>>> +        env->itrigger_enabled = riscv_itrigger_enabled(env);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static const VMStateDescription vmstate_debug = {
>>>       .name = "cpu/debug",
>>>       .version_id = 2,
>>>       .minimum_version_id = 2,
>> The versions here should be bumped
> 
> Hi Alistair and Richard,
> 
> I am not sure if we should bump versions when just add post_load callback without adding 
> new fields.  I once upstreamed a patch
> with a similar change but not bumping version.

Simply adding a post_load does not require a version bump.


r~

Re: [PATCH v1 4/4] target/riscv: Add itrigger_enabled field to CPURISCVState

[Alistair Francis]

On Thu, Nov 10, 2022 at 3:35 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 11/10/22 13:15, LIU Zhiwei wrote:
> >>> +static int debug_post_load(void *opaque, int version_id)
> >>> +{
> >>> +    RISCVCPU *cpu = opaque;
> >>> +    CPURISCVState *env = &cpu->env;
> >>> +
> >>> +    if (icount_enabled()) {
> >>> +        env->itrigger_enabled = riscv_itrigger_enabled(env);
> >>> +    }
> >>> +
> >>> +    return 0;
> >>> +}
> >>> +
> >>>   static const VMStateDescription vmstate_debug = {
> >>>       .name = "cpu/debug",
> >>>       .version_id = 2,
> >>>       .minimum_version_id = 2,
> >> The versions here should be bumped
> >
> > Hi Alistair and Richard,
> >
> > I am not sure if we should bump versions when just add post_load callback without adding
> > new fields.  I once upstreamed a patch
> > with a similar change but not bumping version.
>
> Simply adding a post_load does not require a version bump.

Ah, my mistake then

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

>
>
> r~

Re: [PATCH v1 0/4] Support native debug icount trigger

[Alistair Francis]

On Thu, Oct 13, 2022 at 4:34 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>
> icount trigger set an instruction count. After one instruction retired,
> the count will be decreased by 1. If the count decreased to 0, the icount
> trigger will fire.
>
> icount trigger is needed by single step ptrace system call and the native
> GDB.
>
> In this patch set, change the translation when icount trigger enabled in the
> way that instruction executes one by one. After executing one instruction,
> call a helper function to decrease the count for itrigger.
>
>
> It also provides an accelebrated way. If QEMU executes with -icount parameter,
> itrigger is simulated by a timer with the count in itrigger as the deadline.
>
> Note the count in itrigger will only decrease when the priviledge matches, which
> is also processed in this patch set.
>
>
> After merging this patch set, QEMU will support type2/type6 trigger(needed by
> breakpoint and watchpoint) and icount trigger(needed by single step),
> which is enough for a PoC of native debug.
>
> LIU Zhiwei (4):
>   target/riscv: Add itrigger support when icount is not enabled
>   target/riscv: Add itrigger support when icount is enabled
>   target/riscv: Enable native debug itrigger
>   target/riscv: Add itrigger_enabled field to CPURISCVState

Thanks!

Applied to riscv-to-apply.next

Alistair

>
>  target/riscv/cpu.h                            |   5 +
>  target/riscv/cpu_helper.c                     |   8 +
>  target/riscv/debug.c                          | 205 ++++++++++++++++++
>  target/riscv/debug.h                          |  13 ++
>  target/riscv/helper.h                         |   2 +
>  .../riscv/insn_trans/trans_privileged.c.inc   |   4 +-
>  target/riscv/insn_trans/trans_rvi.c.inc       |   8 +-
>  target/riscv/insn_trans/trans_rvv.c.inc       |   4 +-
>  target/riscv/machine.c                        |  15 ++
>  target/riscv/translate.c                      |  33 ++-
>  10 files changed, 286 insertions(+), 11 deletions(-)
>
> --
> 2.17.1
>
>