diff --git a/src/target/riscv/riscv.c b/src/target/riscv/riscv.c index 59717fd14..a4e851812 100644 --- a/src/target/riscv/riscv.c +++ b/src/target/riscv/riscv.c @@ -623,12 +623,12 @@ static int find_first_trigger_by_id(struct target *target, int unique_id) static unsigned int count_trailing_ones(riscv_reg_t reg) { - assert(sizeof(riscv_reg_t) * 8 == 64); - for (unsigned int i = 0; i < 64; i++) { + const unsigned int riscv_reg_bits = sizeof(riscv_reg_t) * CHAR_BIT; + for (unsigned int i = 0; i < riscv_reg_bits; i++) { if ((1 & (reg >> i)) == 0) return i; } - return 64; + return riscv_reg_bits; } static int set_trigger(struct target *target, unsigned int idx, riscv_reg_t tdata1, riscv_reg_t tdata2) @@ -1561,13 +1561,69 @@ int riscv_remove_watchpoint(struct target *target, return ERROR_OK; } +typedef enum { + M6_HIT_ERROR, + M6_HIT_NOT_SUPPORTED, + M6_NOT_HIT, + M6_HIT_BEFORE, + M6_HIT_AFTER, + M6_HIT_IMM_AFTER +} mctrl6hitstatus; + +/* TODO Record info:mcontrol6_supports_hit, This way + * the number of reads and writes of TDATA1 will be + * greatly reduced. + */ +static mctrl6hitstatus check_mcontrol6_hit_status(struct target *target, + riscv_reg_t tdata1, uint64_t hit_mask) +{ + const uint32_t hit0 = get_field(tdata1, CSR_MCONTROL6_HIT0); + const uint32_t hit1 = get_field(tdata1, CSR_MCONTROL6_HIT1); + const uint32_t hit_info = (hit1 << 1) | hit0; + if (hit_info == CSR_MCONTROL6_HIT0_BEFORE) + return M6_HIT_BEFORE; + + if (hit_info == CSR_MCONTROL6_HIT0_AFTER) + return M6_HIT_AFTER; + + if (hit_info == CSR_MCONTROL6_HIT0_IMMEDIATELY_AFTER) + return M6_HIT_IMM_AFTER; + + if (hit_info == CSR_MCONTROL6_HIT0_FALSE) { + /* hit[1..0] equals 0, which can mean one of the following: + * - "hit" bits are supported and this trigger has not fired + * - "hit" bits are not supported on this trigger + * To distinguish these two cases, try writing all non-zero bit + * patterns to hit[1..0] to determine if the "hit" bits are supported: + */ + riscv_reg_t tdata1_tests[] = { + set_field(tdata1, CSR_MCONTROL6_HIT0, 1), + set_field(tdata1, CSR_MCONTROL6_HIT1, 1), + set_field(tdata1, CSR_MCONTROL6_HIT0, 1) | field_value(CSR_MCONTROL6_HIT1, 1) + }; + riscv_reg_t tdata1_test_rb; + for (uint64_t i = 0; i < ARRAY_SIZE(tdata1_tests); ++i) { + if (riscv_reg_set(target, GDB_REGNO_TDATA1, tdata1_tests[i]) != ERROR_OK) + return M6_HIT_ERROR; + if (riscv_reg_get(target, &tdata1_test_rb, GDB_REGNO_TDATA1) != ERROR_OK) + return M6_HIT_ERROR; + if (riscv_reg_set(target, GDB_REGNO_TDATA1, tdata1_test_rb & ~hit_mask) != ERROR_OK) + return M6_HIT_ERROR; + if (tdata1_test_rb == tdata1_tests[i]) + return M6_NOT_HIT; + } + } + return M6_HIT_NOT_SUPPORTED; +} + /** * Look at the trigger hit bits to find out which trigger is the reason we're * halted. Sets *unique_id to the unique ID of that trigger. If *unique_id is * RISCV_TRIGGER_HIT_NOT_FOUND, no match was found. */ -static int riscv_trigger_detect_hit_bits(struct target *target, int64_t *unique_id) +static int riscv_trigger_detect_hit_bits(struct target *target, int64_t *unique_id, + bool *need_single_step) { /* FIXME: this function assumes that we have only one trigger that can * have hit bit set. Debug spec allows hit bit to bit set if a trigger has @@ -1601,9 +1657,21 @@ static int riscv_trigger_detect_hit_bits(struct target *target, int64_t *unique_ break; case CSR_TDATA1_TYPE_MCONTROL: hit_mask = CSR_MCONTROL_HIT; + r->need_single_step = true; break; case CSR_TDATA1_TYPE_MCONTROL6: hit_mask = CSR_MCONTROL6_HIT0 | CSR_MCONTROL6_HIT1; + if (r->trigger_tinfo_version[i] == CSR_TINFO_VERSION_0) { + r->need_single_step = true; + } else if (r->trigger_tinfo_version[i] == ERROR_TARGET_RESOURCE_NOT_AVAILABLE + || r->trigger_tinfo_version[i] == CSR_TINFO_VERSION_1) { + mctrl6hitstatus hits_status = check_mcontrol6_hit_status(target, + tdata1, hit_mask); + if (hits_status == M6_HIT_ERROR) + return ERROR_FAIL; + if (hits_status == M6_HIT_BEFORE || hits_status == M6_HIT_NOT_SUPPORTED) + r->need_single_step = true; + } break; case CSR_TDATA1_TYPE_ICOUNT: hit_mask = CSR_ICOUNT_HIT; @@ -1622,8 +1690,9 @@ static int riscv_trigger_detect_hit_bits(struct target *target, int64_t *unique_ /* FIXME: this logic needs to be changed to ignore triggers that are not * the last one in the chain. */ if (tdata1 & hit_mask) { - LOG_TARGET_DEBUG(target, "Trigger %u (unique_id=%" PRIi64 ") has hit bit set.", - i, r->trigger_unique_id[i]); + LOG_TARGET_DEBUG(target, "Trigger %u (unique_id=%" PRIi64 "" + ", need_single_step=%" PRId32 ") has hit bit set.", + i, r->trigger_unique_id[i], r->need_single_step); if (riscv_reg_set(target, GDB_REGNO_TDATA1, tdata1 & ~hit_mask) != ERROR_OK) return ERROR_FAIL; @@ -2285,13 +2354,15 @@ static int set_debug_reason(struct target *target, enum riscv_halt_reason halt_r { RISCV_INFO(r); r->trigger_hit = -1; + r->need_single_step = false; switch (halt_reason) { case RISCV_HALT_EBREAK: target->debug_reason = DBG_REASON_BREAKPOINT; break; case RISCV_HALT_TRIGGER: target->debug_reason = DBG_REASON_UNDEFINED; - if (riscv_trigger_detect_hit_bits(target, &r->trigger_hit) != ERROR_OK) + if (riscv_trigger_detect_hit_bits(target, &r->trigger_hit, + &r->need_single_step) != ERROR_OK) return ERROR_FAIL; // FIXME: handle multiple hit bits if (r->trigger_hit != RISCV_TRIGGER_HIT_NOT_FOUND) { @@ -2553,10 +2624,19 @@ static int resume_prep(struct target *target, int current, if (handle_breakpoints) { /* To be able to run off a trigger, we perform a step operation and then * resume. If handle_breakpoints is true then step temporarily disables - * pending breakpoints so we can safely perform the step. */ - if (old_or_new_riscv_step_impl(target, current, address, handle_breakpoints, - false /* callbacks are not called */) != ERROR_OK) - return ERROR_FAIL; + * pending breakpoints so we can safely perform the step. + * + * Two cases where single step is needed before resuming: + * 1. ebreak used in software breakpoint; + * 2. a trigger that is taken just before the instruction that triggered it is retired. + */ + if (target->debug_reason == DBG_REASON_BREAKPOINT + || (target->debug_reason == DBG_REASON_WATCHPOINT + && r->need_single_step)) { + if (old_or_new_riscv_step_impl(target, current, address, handle_breakpoints, + false /* callbacks are not called */) != ERROR_OK) + return ERROR_FAIL; + } } if (r->get_hart_state) { @@ -5698,12 +5778,13 @@ static int check_if_trigger_exists(struct target *target, unsigned int index) * to determine trigger types supported by a trigger. * It is assumed that the trigger is already selected via writing `tselect`. */ -static int get_trigger_types(struct target *target, unsigned int *trigger_tinfo, - riscv_reg_t tdata1) +static int get_trigger_types_and_version(struct target *target, unsigned int *trigger_tinfo, + int64_t *trigger_tinfo_version, riscv_reg_t tdata1) { assert(trigger_tinfo); riscv_reg_t tinfo; if (riscv_reg_get(target, &tinfo, GDB_REGNO_TINFO) == ERROR_OK) { + *trigger_tinfo_version = get_field(tinfo, CSR_TINFO_VERSION); /* tinfo.INFO == 1: trigger doesn’t exist * tinfo == 0 or tinfo.INFO != 1 and tinfo LSB is set: invalid tinfo */ if (tinfo == 0 || tinfo & 0x1) @@ -5711,6 +5792,7 @@ static int get_trigger_types(struct target *target, unsigned int *trigger_tinfo, *trigger_tinfo = tinfo; return ERROR_OK; } + *trigger_tinfo_version = ERROR_TARGET_RESOURCE_NOT_AVAILABLE; const unsigned int type = get_field(tdata1, CSR_TDATA1_TYPE(riscv_xlen(target))); if (type == 0) return ERROR_TARGET_RESOURCE_NOT_AVAILABLE; @@ -5794,7 +5876,8 @@ int riscv_enumerate_triggers(struct target *target) if (riscv_reg_get(target, &tdata1, GDB_REGNO_TDATA1) != ERROR_OK) return ERROR_FAIL; - result = get_trigger_types(target, &r->trigger_tinfo[t], tdata1); + result = get_trigger_types_and_version(target, &r->trigger_tinfo[t], + &r->trigger_tinfo_version[t], tdata1); if (result == ERROR_FAIL) return ERROR_FAIL; if (result == ERROR_TARGET_RESOURCE_NOT_AVAILABLE) diff --git a/src/target/riscv/riscv.h b/src/target/riscv/riscv.h index 4ac10fa76..82fe45b26 100644 --- a/src/target/riscv/riscv.h +++ b/src/target/riscv/riscv.h @@ -152,6 +152,17 @@ struct riscv_info { /* record the tinfo of each trigger */ unsigned int trigger_tinfo[RISCV_MAX_TRIGGERS]; + /* record the tinfo_version of each trigger */ + int64_t trigger_tinfo_version[RISCV_MAX_TRIGGERS]; + + /* Record if single-step is needed prior to resuming + * from a software breakpoint or trigger. + * Single-step is needed if the instruction that + * caused the halt was not retired. That is, + * when we halted "before" that instruction. + */ + bool need_single_step; + /* For each physical trigger contains: * -1: the hwbp is available * -4: The trigger is used by the itrigger command