Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

riscv: simplify state management during examine #900

Merged
merged 1 commit into from
Aug 29, 2023
Merged

riscv: simplify state management during examine #900

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 16 additions & 25 deletions src/target/riscv/riscv-013.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
JanMatCodasip marked this conversation as resolved.
Show resolved Hide resolved
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;
Expand Down Expand Up @@ -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;
Comment on lines +2133 to +2134
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary? We know that the hart is halted on line 2054, and the state should reflect that.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary? We know that the hart is halted on line 2054, and the state should reflect that.

Yes, we know that the hart is halted. And we have target->state = TARGET_HALTED to indicate that. However, we don't know why hart was halted (since we've just connected) and thus we set the reason to UNDEFINED.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Thanks.

}

if (target->smp) {
Expand Down
Loading