From 2c00a087daddbc478f2b9d691bc9706a60f0681b Mon Sep 17 00:00:00 2001 From: Parshintsev Anatoly Date: Mon, 22 Apr 2024 21:33:33 +0300 Subject: target/riscv: fix halt reason for targets that do not support hit bit on triggers Before this patch the following behavior is observed on targets that do not support hit bit: ``` bp 0x80000004 4 hw resume 0x80000000 riscv.cpu halted due to watchpoint ``` This happens because the current implementation relies on the presence of hit bit way too much. While working on this patch few defects in hit bit-based trigger detection were discovered, added appropriate TODOs. --- src/target/riscv/riscv.c | 101 +++++++++++++++++++++++++++++++++++++++++------ src/target/riscv/riscv.h | 4 +- 2 files changed, 90 insertions(+), 15 deletions(-) diff --git a/src/target/riscv/riscv.c b/src/target/riscv/riscv.c index 26fd365..2454d54 100644 --- a/src/target/riscv/riscv.c +++ b/src/target/riscv/riscv.c @@ -34,6 +34,8 @@ #define DBUS 0x11 +#define RISCV_TRIGGER_HIT_NOT_FOUND ((int64_t)-1) + static uint8_t ir_dtmcontrol[4] = {DTMCONTROL}; struct scan_field select_dtmcontrol = { .in_value = NULL, @@ -1550,17 +1552,24 @@ int riscv_remove_watchpoint(struct target *target, /** * 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 - * ~0, no match was found. + * RISCV_TRIGGER_HIT_NOT_FOUND, no match was found. */ -static int riscv_hit_trigger_hit_bit(struct target *target, uint32_t *unique_id) + +static int riscv_trigger_detect_hit_bits(struct target *target, int64_t *unique_id) { + /* 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 + * matched but did not fire. Such targets will receive erroneous results. + */ + + // FIXME: Add hit bits support detection and caching RISCV_INFO(r); riscv_reg_t tselect; if (riscv_get_register(target, &tselect, GDB_REGNO_TSELECT) != ERROR_OK) return ERROR_FAIL; - *unique_id = ~0; + *unique_id = RISCV_TRIGGER_HIT_NOT_FOUND; for (unsigned int i = 0; i < r->trigger_count; i++) { if (r->trigger_unique_id[i] == -1) continue; @@ -1594,15 +1603,15 @@ static int riscv_hit_trigger_hit_bit(struct target *target, uint32_t *unique_id) hit_mask = CSR_ETRIGGER_HIT(riscv_xlen(target)); break; default: - LOG_TARGET_DEBUG(target, "Trigger %d has unknown type %d", i, type); + LOG_TARGET_DEBUG(target, "Trigger %u has unknown type %d", i, type); continue; } - /* Note: If we ever use chained triggers, then this logic needs - * to be changed to ignore triggers that are not the last one in - * the chain. */ + /* 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 %d (unique_id=%d) has hit bit set.", i, r->trigger_unique_id[i]); + LOG_TARGET_DEBUG(target, "Trigger %u (unique_id=%" PRIi64 ") has hit bit set.", + i, r->trigger_unique_id[i]); if (riscv_set_register(target, GDB_REGNO_TDATA1, tdata1 & ~hit_mask) != ERROR_OK) return ERROR_FAIL; @@ -2268,6 +2277,33 @@ int riscv_flush_registers(struct target *target) return ERROR_OK; } +static enum target_debug_reason +derive_debug_reason_without_hitbit(const struct target *target, riscv_reg_t dpc) +{ + /* TODO: if we detect that etrigger/itrigger/icount is set, we should + * just report DBG_REASON_UNKNOWN, since we can't disctiguish these + * triggers from BP/WP or from other triggers of such type. However, + * currently this renders existing testsuite as failing. We need to + * fix the testsuite first + */ + // TODO: the code below does not handle context-aware trigger types + for (const struct breakpoint *bp = target->breakpoints; bp; bp = bp->next) { + // TODO: investigate if we need to handle bp length + if (bp->type == BKPT_HARD && bp->is_set && bp->address == dpc) { + // FIXME: bp->linked_brp is uninitialized + if (bp->asid) { + LOG_TARGET_ERROR(target, + "can't derive debug reason for context-aware breakpoint: " + "unique_id = %" PRIu32 ", address = %" TARGET_PRIxADDR + ", asid = %" PRIx32 ", linked = %d", + bp->unique_id, bp->address, bp->asid, bp->linked_brp); + return DBG_REASON_UNDEFINED; + } + return DBG_REASON_BREAKPOINT; + } + } + return DBG_REASON_WATCHPOINT; +} /** * Set OpenOCD's generic debug reason from the RISC-V halt reason. */ @@ -2280,13 +2316,52 @@ static int set_debug_reason(struct target *target, enum riscv_halt_reason halt_r target->debug_reason = DBG_REASON_BREAKPOINT; break; case RISCV_HALT_TRIGGER: - if (riscv_hit_trigger_hit_bit(target, &r->trigger_hit) != ERROR_OK) + target->debug_reason = DBG_REASON_UNDEFINED; + if (riscv_trigger_detect_hit_bits(target, &r->trigger_hit) != ERROR_OK) return ERROR_FAIL; - target->debug_reason = DBG_REASON_WATCHPOINT; - /* Check if we hit a hardware breakpoint. */ - for (struct breakpoint *bp = target->breakpoints; bp; bp = bp->next) { - if (bp->unique_id == r->trigger_hit) + // FIXME: handle multiple hit bits + if (r->trigger_hit != RISCV_TRIGGER_HIT_NOT_FOUND) { + /* We scan for breakpoints first. If no breakpoints are found we still + * assume that debug reason is DBG_REASON_BREAKPOINT, unless + * there is a watchpoint match - This is to take + * ETrigger/ITrigger/ICount into account + */ + LOG_TARGET_DEBUG(target, + "Active hit bit is detected, trying to find trigger owner."); + for (struct breakpoint *bp = target->breakpoints; bp; bp = bp->next) { + if (bp->unique_id == r->trigger_hit) { + target->debug_reason = DBG_REASON_BREAKPOINT; + LOG_TARGET_DEBUG(target, + "Breakpoint with unique_id = %" PRIu32 " owns the trigger.", + bp->unique_id); + } + } + if (target->debug_reason == DBG_REASON_UNDEFINED) { + // by default we report all triggers as breakpoints target->debug_reason = DBG_REASON_BREAKPOINT; + for (struct watchpoint *wp = target->watchpoints; wp; wp = wp->next) { + if (wp->unique_id == r->trigger_hit) { + target->debug_reason = DBG_REASON_WATCHPOINT; + LOG_TARGET_DEBUG(target, + "Watchpoint with unique_id = %" PRIu32 " owns the trigger.", + wp->unique_id); + } + } + } + } else { + LOG_TARGET_DEBUG(target, + "No trigger hit found, deriving debug reason without it."); + riscv_reg_t dpc; + if (riscv_get_register(target, &dpc, GDB_REGNO_DPC) != ERROR_OK) + return ERROR_FAIL; + /* Here we don't have the hit bit set (likely, HW does not support it). + * We are trying to guess the state. But here comes the problem: + * if we have etrigger/itrigger/icount raised - we can't really + * distinguish it from the breakpoint or watchpoint. There is not + * much we can do here, except for checking current PC against pending + * breakpoints and hope for the best) + */ + target->debug_reason = derive_debug_reason_without_hitbit(target, dpc); } break; case RISCV_HALT_INTERRUPT: diff --git a/src/target/riscv/riscv.h b/src/target/riscv/riscv.h index 7a95af6..d3703d1 100644 --- a/src/target/riscv/riscv.h +++ b/src/target/riscv/riscv.h @@ -159,11 +159,11 @@ struct riscv_info { * >= 0: unique_id of the breakpoint/watchpoint that is using it. * Note that in RTOS mode the triggers are the same across all harts the * target controls, while otherwise only a single hart is controlled. */ - int trigger_unique_id[RISCV_MAX_HWBPS]; + int64_t trigger_unique_id[RISCV_MAX_HWBPS]; /* The unique id of the trigger that caused the most recent halt. If the * most recent halt was not caused by a trigger, then this is -1. */ - uint32_t trigger_hit; + int64_t trigger_hit; /* The number of entries in the program buffer. */ int progbuf_size; -- cgit v1.1