From 198edca6d0516c979c787b30a99cc93d2342221a Mon Sep 17 00:00:00 2001 From: Parshintsev Anatoly Date: Fri, 4 Aug 2023 14:54:50 +0300 Subject: [PATCH] riscv: simplify state management during examine This also fixes a bug when, after `examine` completion, the target still has `unknown` status. To reproduce this one spike, it is enough to do the following: --- // make sure spike harts are halted openocd ... -c init -c 'echo "[targets]"' --- this behavior is quite dangerous and leads to segfaults in some cases Change-Id: I13915f7038ad6d0251d56d2d519fbad9a2f13c18 Signed-off-by: Parshintsev Anatoly --- src/target/riscv/riscv-013.c | 41 ++++++++++++++---------------------- 1 file changed, 16 insertions(+), 25 deletions(-) diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c index 59bd8690c..3862515e2 100644 --- a/src/target/riscv/riscv-013.c +++ b/src/target/riscv/riscv-013.c @@ -1870,8 +1870,12 @@ static int set_group(struct target *target, bool *supported, unsigned int group, static int examine(struct target *target) { - /* Don't need to select dbus, since the first thing we do is read dtmcontrol. */ + /* We reset target state in case if something goes wrong during examine: + * DTM/DM scans could fail or hart may fail to halt. */ + target->state = TARGET_UNKNOWN; + target->debug_reason = DBG_REASON_UNDEFINED; + /* Don't need to select dbus, since the first thing we do is read dtmcontrol. */ LOG_TARGET_DEBUG(target, "dbgbase=0x%x", target->dbgbase); uint32_t dtmcontrol = dtmcontrol_scan(target, 0); @@ -2033,35 +2037,22 @@ static int examine(struct target *target) if (dm013_select_hart(target, info->index) != ERROR_OK) return ERROR_FAIL; - enum riscv_hart_state state; - if (riscv_get_hart_state(target, &state) != ERROR_OK) + enum riscv_hart_state state_at_examine_start; + if (riscv_get_hart_state(target, &state_at_examine_start) != ERROR_OK) return ERROR_FAIL; - bool halted = (state == RISCV_STATE_HALTED); - if (!halted) { + const bool hart_halted_at_examine_start = state_at_examine_start == RISCV_STATE_HALTED; + if (!hart_halted_at_examine_start) { r->prepped = true; if (riscv013_halt_go(target) != ERROR_OK) { LOG_TARGET_ERROR(target, "Fatal: Hart %d failed to halt during %s", info->index, __func__); return ERROR_FAIL; } - target->state = TARGET_HALTED; - target->debug_reason = DBG_REASON_DBGRQ; - } - /* FIXME: This is needed since register_read_direct relies on target->state - * to work correctly, so, if target->state does not represent current state - * of target, e.g. if a target is halted, but target->state is - * TARGET_UNKNOWN, it can fail early, (e.g. accessing registers via program - * buffer can not be done atomically on a running hart becuse mstatus can't - * be prepared for a register access and then restored) - * See https://github.com/riscv/riscv-openocd/pull/842#discussion_r1179414089 - */ - const enum target_state saved_tgt_state = target->state; - const enum target_debug_reason saved_dbg_reason = target->debug_reason; - if (target->state != TARGET_HALTED) { - target->state = TARGET_HALTED; - target->debug_reason = DBG_REASON_DBGRQ; } + target->state = TARGET_HALTED; + target->debug_reason = hart_halted_at_examine_start ? DBG_REASON_UNDEFINED : DBG_REASON_DBGRQ; + /* Without knowing anything else we can at least mess with the * program buffer. */ r->debug_buffer_size = info->progbufsize; @@ -2134,13 +2125,13 @@ static int examine(struct target *target) if (set_dcsr_ebreak(target, false) != ERROR_OK) return ERROR_FAIL; - target->state = saved_tgt_state; - target->debug_reason = saved_dbg_reason; - - if (!halted) { + if (state_at_examine_start == RISCV_STATE_RUNNING) { riscv013_step_or_resume_current_hart(target, false); target->state = TARGET_RUNNING; target->debug_reason = DBG_REASON_NOTHALTED; + } else if (state_at_examine_start == RISCV_STATE_HALTED) { + target->state = TARGET_HALTED; + target->debug_reason = DBG_REASON_UNDEFINED; } if (target->smp) {