[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 1/4] target/riscv: Add itrigger support when icount is not
|
From: |
Alistair Francis |
|
Subject: |
Re: [PATCH v1 1/4] target/riscv: Add itrigger support when icount is not enabled |
|
Date: |
Thu, 10 Nov 2022 08:33:17 +1000 |
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
> >>
> >>