From 848062d0d11679de25be573981df45e2c4880db8 Mon Sep 17 00:00:00 2001 From: Tim Newsome Date: Mon, 12 Mar 2018 17:26:29 -0700 Subject: [PATCH] Propagate errors in more places Change-Id: I5a7594d4b44c524537827f403348d0c10814546f --- src/target/riscv/riscv-013.c | 113 +++++++++++++++-------------------- src/target/riscv/riscv.c | 76 ++++++++++++++--------- src/target/riscv/riscv.h | 7 ++- 3 files changed, 99 insertions(+), 97 deletions(-) diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c index 2af2221d5..990379e84 100644 --- a/src/target/riscv/riscv-013.c +++ b/src/target/riscv/riscv-013.c @@ -39,7 +39,7 @@ static void riscv013_clear_abstract_error(struct target *target); static int riscv013_get_register(struct target *target, riscv_reg_t *value, int hid, int rid); static int riscv013_set_register(struct target *target, int hartid, int regid, uint64_t value); -static void riscv013_select_current_hart(struct target *target); +static int riscv013_select_current_hart(struct target *target); static int riscv013_halt_current_hart(struct target *target); static int riscv013_resume_current_hart(struct target *target); static int riscv013_step_current_hart(struct target *target); @@ -479,7 +479,8 @@ static dmi_status_t dmi_scan(struct target *target, uint32_t *address_in, return buf_get_u32(in, DTM_DMI_OP_OFFSET, DTM_DMI_OP_LENGTH); } -static int dmi_read(struct target *target, uint32_t *value, uint32_t address) +static int dmi_op(struct target *target, uint32_t *data_in, int dmi_op, + uint32_t address, uint32_t data_out) { select_dmi(target); @@ -488,49 +489,66 @@ static int dmi_read(struct target *target, uint32_t *value, uint32_t address) unsigned i = 0; - /* This first loop ensures that the read request was actually sent - * to the target. Note that if for some reason this stays busy, - * it is actually due to the previous dmi_read or dmi_write. */ + const char *op_name; + switch (dmi_op) { + case DMI_OP_NOP: + op_name = "nop"; + break; + case DMI_OP_READ: + op_name = "read"; + break; + case DMI_OP_WRITE: + op_name = "write"; + break; + default: + LOG_ERROR("Invalid DMI operation: %d", dmi_op); + return ERROR_FAIL; + } + + /* This first loop performs the request. Note that if for some reason this + * stays busy, it is actually due to the previous access. */ for (i = 0; i < 256; i++) { - status = dmi_scan(target, NULL, NULL, DMI_OP_READ, address, 0, + status = dmi_scan(target, NULL, NULL, dmi_op, address, data_out, false); if (status == DMI_STATUS_BUSY) { increase_dmi_busy_delay(target); } else if (status == DMI_STATUS_SUCCESS) { break; } else { - LOG_ERROR("failed read from 0x%x, status=%d", address, status); + LOG_ERROR("failed %s at 0x%x, status=%d", op_name, address, status); return ERROR_FAIL; } } if (status != DMI_STATUS_SUCCESS) { - LOG_ERROR("Failed read from 0x%x; status=%d", address, status); + LOG_ERROR("Failed %s at 0x%x; status=%d", op_name, address, status); return ERROR_FAIL; } - /* This second loop ensures that we got the read - * data back. Note that NOP can result in a 'busy' result as well, but - * that would be noticed on the next DMI access we do. */ + /* This second loop ensures the request succeeded, and gets back data. + * Note that NOP can result in a 'busy' result as well, but that would be + * noticed on the next DMI access we do. */ for (i = 0; i < 256; i++) { - status = dmi_scan(target, &address_in, value, DMI_OP_NOP, address, 0, + status = dmi_scan(target, &address_in, data_in, DMI_OP_NOP, address, 0, false); if (status == DMI_STATUS_BUSY) { increase_dmi_busy_delay(target); } else if (status == DMI_STATUS_SUCCESS) { break; } else { - LOG_ERROR("failed read (NOP) at 0x%x, status=%d", address, status); + LOG_ERROR("failed %s (NOP) at 0x%x, status=%d", op_name, address, + status); return ERROR_FAIL; } } if (status != DMI_STATUS_SUCCESS) { - if (status == DMI_STATUS_FAILED) { - LOG_ERROR("Failed read (NOP) from 0x%x; status=%d", address, status); + if (status == DMI_STATUS_FAILED || !data_in) { + LOG_ERROR("Failed %s (NOP) at 0x%x; status=%d", op_name, address, + status); } else { - LOG_ERROR("Failed read (NOP) from 0x%x; value=0x%x, status=%d", - address, *value, status); + LOG_ERROR("Failed %s (NOP) at 0x%x; value=0x%x, status=%d", + op_name, address, *data_in, status); } return ERROR_FAIL; } @@ -538,53 +556,14 @@ static int dmi_read(struct target *target, uint32_t *value, uint32_t address) return ERROR_OK; } +static int dmi_read(struct target *target, uint32_t *value, uint32_t address) +{ + return dmi_op(target, value, DMI_OP_READ, address, 0); +} + static int dmi_write(struct target *target, uint32_t address, uint32_t value) { - select_dmi(target); - dmi_status_t status = DMI_STATUS_BUSY; - unsigned i = 0; - - /* The first loop ensures that we successfully sent the write request. */ - for (i = 0; i < 256; i++) { - status = dmi_scan(target, NULL, NULL, DMI_OP_WRITE, address, value, - address == DMI_COMMAND); - if (status == DMI_STATUS_BUSY) { - increase_dmi_busy_delay(target); - } else if (status == DMI_STATUS_SUCCESS) { - break; - } else { - LOG_ERROR("failed write to 0x%x, status=%d", address, status); - break; - } - } - - if (status != DMI_STATUS_SUCCESS) { - LOG_ERROR("Failed write to 0x%x;, status=%d", - address, status); - return ERROR_FAIL; - } - - /* The second loop isn't strictly necessary, but would ensure that the - * write is complete/ has no non-busy errors before returning from this - * function. */ - for (i = 0; i < 256; i++) { - status = dmi_scan(target, NULL, NULL, DMI_OP_NOP, address, 0, - false); - if (status == DMI_STATUS_BUSY) { - increase_dmi_busy_delay(target); - } else if (status == DMI_STATUS_SUCCESS) { - break; - } else { - LOG_ERROR("failed write (NOP) at 0x%x, status=%d", address, status); - break; - } - } - if (status != DMI_STATUS_SUCCESS) { - LOG_ERROR("failed to write (NOP) 0x%x to 0x%x; status=%d", value, address, status); - return ERROR_FAIL; - } - - return ERROR_OK; + return dmi_op(target, NULL, DMI_OP_WRITE, address, value); } int dmstatus_read(struct target *target, uint32_t *dmstatus, @@ -1352,7 +1331,8 @@ static int examine(struct target *target) continue; r->current_hartid = i; - riscv013_select_current_hart(target); + if (riscv013_select_current_hart(target) != ERROR_OK) + return ERROR_FAIL; uint32_t s; if (dmstatus_read(target, &s, true) != ERROR_OK) @@ -2480,14 +2460,15 @@ static int riscv013_set_register(struct target *target, int hid, int rid, uint64 return ERROR_OK; } -static void riscv013_select_current_hart(struct target *target) +static int riscv013_select_current_hart(struct target *target) { RISCV_INFO(r); uint32_t dmcontrol; - dmi_read(target, &dmcontrol, DMI_DMCONTROL); + if (dmi_read(target, &dmcontrol, DMI_DMCONTROL) != ERROR_OK) + return ERROR_FAIL; dmcontrol = set_field(dmcontrol, hartsel_mask(target), r->current_hartid); - dmi_write(target, DMI_DMCONTROL, dmcontrol); + return dmi_write(target, DMI_DMCONTROL, dmcontrol); } static int riscv013_halt_current_hart(struct target *target) diff --git a/src/target/riscv/riscv.c b/src/target/riscv/riscv.c index 9f16a400a..15ac8318c 100644 --- a/src/target/riscv/riscv.c +++ b/src/target/riscv/riscv.c @@ -737,19 +737,20 @@ static int old_or_new_riscv_resume( return riscv_openocd_resume(target, current, address, handle_breakpoints, debug_execution); } -static void riscv_select_current_hart(struct target *target) +static int riscv_select_current_hart(struct target *target) { RISCV_INFO(r); if (r->rtos_hartid != -1 && riscv_rtos_enabled(target)) - riscv_set_current_hartid(target, r->rtos_hartid); + return riscv_set_current_hartid(target, r->rtos_hartid); else - riscv_set_current_hartid(target, target->coreid); + return riscv_set_current_hartid(target, target->coreid); } static int riscv_read_memory(struct target *target, target_addr_t address, uint32_t size, uint32_t count, uint8_t *buffer) { - riscv_select_current_hart(target); + if (riscv_select_current_hart(target) != ERROR_OK) + return ERROR_FAIL; struct target_type *tt = get_target_type(target); return tt->read_memory(target, address, size, count, buffer); } @@ -757,7 +758,8 @@ static int riscv_read_memory(struct target *target, target_addr_t address, static int riscv_write_memory(struct target *target, target_addr_t address, uint32_t size, uint32_t count, const uint8_t *buffer) { - riscv_select_current_hart(target); + if (riscv_select_current_hart(target) != ERROR_OK) + return ERROR_FAIL; struct target_type *tt = get_target_type(target); return tt->write_memory(target, address, size, count, buffer); } @@ -775,7 +777,8 @@ static int riscv_get_gdb_reg_list(struct target *target, return ERROR_FAIL; } - riscv_select_current_hart(target); + if (riscv_select_current_hart(target) != ERROR_OK) + return ERROR_FAIL; switch (reg_class) { case REG_CLASS_GENERAL: @@ -964,24 +967,29 @@ int riscv_blank_check_memory(struct target *target, /*** OpenOCD Helper Functions ***/ -/* 0 means nothing happened, 1 means the hart's state changed (and thus the - * poll should terminate), and -1 means there was an error. */ -static int riscv_poll_hart(struct target *target, int hartid) +enum riscv_poll_hart { + RPH_NO_CHANGE, + RPH_CHANGE, + RPH_ERROR +}; +static enum riscv_poll_hart riscv_poll_hart(struct target *target, int hartid) { RISCV_INFO(r); - riscv_set_current_hartid(target, hartid); + if (riscv_set_current_hartid(target, hartid) != ERROR_OK) + return RPH_ERROR; - LOG_DEBUG("polling hart %d, target->state=%d (TARGET_HALTED=%d)", hartid, target->state, TARGET_HALTED); + LOG_DEBUG("polling hart %d, target->state=%d (TARGET_HALTED=%d)", hartid, + target->state, TARGET_HALTED); - /* If OpenOCD this we're running but this hart is halted then it's time + /* If OpenOCD thinks we're running but this hart is halted then it's time * to raise an event. */ if (target->state != TARGET_HALTED && riscv_is_halted(target)) { LOG_DEBUG(" triggered a halt"); r->on_halt(target); - return 1; + return RPH_CHANGE; } - return 0; + return RPH_NO_CHANGE; } /*** OpenOCD Interface ***/ @@ -992,14 +1000,14 @@ int riscv_openocd_poll(struct target *target) if (riscv_rtos_enabled(target)) { /* Check every hart for an event. */ for (int i = 0; i < riscv_count_harts(target); ++i) { - int out = riscv_poll_hart(target, i); + enum riscv_poll_hart out = riscv_poll_hart(target, i); switch (out) { - case 0: + case RPH_NO_CHANGE: continue; - case 1: + case RPH_CHANGE: triggered_hart = i; break; - case -1: + case RPH_ERROR: return ERROR_FAIL; } } @@ -1018,8 +1026,12 @@ int riscv_openocd_poll(struct target *target) for (int i = 0; i < riscv_count_harts(target); ++i) riscv_halt_one_hart(target, i); } else { - if (riscv_poll_hart(target, riscv_current_hartid(target)) == 0) + enum riscv_poll_hart out = riscv_poll_hart(target, + riscv_current_hartid(target)); + if (out == RPH_NO_CHANGE) return ERROR_OK; + else if (out == RPH_ERROR) + return ERROR_FAIL; triggered_hart = riscv_current_hartid(target); LOG_DEBUG(" hart %d halted", triggered_hart); @@ -1042,6 +1054,8 @@ int riscv_openocd_poll(struct target *target) case RISCV_HALT_UNKNOWN: target->debug_reason = DBG_REASON_UNDEFINED; break; + case RISCV_HALT_ERROR: + return ERROR_FAIL; } if (riscv_rtos_enabled(target)) { @@ -1562,7 +1576,8 @@ int riscv_halt_one_hart(struct target *target, int hartid) { RISCV_INFO(r); LOG_DEBUG("halting hart %d", hartid); - riscv_set_current_hartid(target, hartid); + if (riscv_set_current_hartid(target, hartid) != ERROR_OK) + return ERROR_FAIL; if (riscv_is_halted(target)) { LOG_DEBUG(" hart %d requested halt, but was already halted", hartid); return ERROR_OK; @@ -1588,7 +1603,8 @@ int riscv_resume_one_hart(struct target *target, int hartid) { RISCV_INFO(r); LOG_DEBUG("resuming hart %d", hartid); - riscv_set_current_hartid(target, hartid); + if (riscv_set_current_hartid(target, hartid) != ERROR_OK) + return ERROR_FAIL; if (!riscv_is_halted(target)) { LOG_DEBUG(" hart %d requested resume, but was already resumed", hartid); return ERROR_OK; @@ -1609,7 +1625,8 @@ int riscv_step_rtos_hart(struct target *target) hartid = 0; } } - riscv_set_current_hartid(target, hartid); + if (riscv_set_current_hartid(target, hartid) != ERROR_OK) + return ERROR_FAIL; LOG_DEBUG("stepping hart %d", hartid); if (!riscv_is_halted(target)) { @@ -1659,33 +1676,35 @@ bool riscv_rtos_enabled(const struct target *target) return target->rtos != NULL; } -void riscv_set_current_hartid(struct target *target, int hartid) +int riscv_set_current_hartid(struct target *target, int hartid) { RISCV_INFO(r); if (!r->select_current_hart) - return; + return ERROR_FAIL; int previous_hartid = riscv_current_hartid(target); r->current_hartid = hartid; assert(riscv_hart_enabled(target, hartid)); LOG_DEBUG("setting hartid to %d, was %d", hartid, previous_hartid); - r->select_current_hart(target); + if (r->select_current_hart(target) != ERROR_OK) + return ERROR_FAIL; /* This might get called during init, in which case we shouldn't be * setting up the register cache. */ if (!target_was_examined(target)) - return; + return ERROR_OK; /* Avoid invalidating the register cache all the time. */ if (r->registers_initialized && (!riscv_rtos_enabled(target) || (previous_hartid == hartid)) && target->reg_cache->reg_list[GDB_REGNO_ZERO].size == (unsigned)riscv_xlen(target) && (!riscv_rtos_enabled(target) || (r->rtos_hartid != -1))) { - return; + return ERROR_OK; } else LOG_DEBUG("Initializing registers: xlen=%d", riscv_xlen(target)); riscv_invalidate_register_cache(target); + return ERROR_OK; } void riscv_invalidate_register_cache(struct target *target) @@ -1775,7 +1794,8 @@ bool riscv_is_halted(struct target *target) enum riscv_halt_reason riscv_halt_reason(struct target *target, int hartid) { RISCV_INFO(r); - riscv_set_current_hartid(target, hartid); + if (riscv_set_current_hartid(target, hartid) != ERROR_OK) + return RISCV_HALT_ERROR; if (!riscv_is_halted(target)) { LOG_ERROR("Hart is not halted!"); return RISCV_HALT_UNKNOWN; diff --git a/src/target/riscv/riscv.h b/src/target/riscv/riscv.h index 626b71f83..1e6ffb94b 100644 --- a/src/target/riscv/riscv.h +++ b/src/target/riscv/riscv.h @@ -31,7 +31,8 @@ enum riscv_halt_reason { RISCV_HALT_BREAKPOINT, RISCV_HALT_SINGLESTEP, RISCV_HALT_TRIGGER, - RISCV_HALT_UNKNOWN + RISCV_HALT_UNKNOWN, + RISCV_HALT_ERROR }; typedef struct { @@ -93,7 +94,7 @@ typedef struct { riscv_reg_t *value, int hid, int rid); int (*set_register)(struct target *, int hartid, int regid, uint64_t value); - void (*select_current_hart)(struct target *); + int (*select_current_hart)(struct target *); bool (*is_halted)(struct target *target); int (*halt_current_hart)(struct target *); int (*resume_current_hart)(struct target *target); @@ -191,7 +192,7 @@ bool riscv_rtos_enabled(const struct target *target); /* Sets the current hart, which is the hart that will actually be used when * issuing debug commands. */ -void riscv_set_current_hartid(struct target *target, int hartid); +int riscv_set_current_hartid(struct target *target, int hartid); int riscv_current_hartid(const struct target *target); /*** Support functions for the RISC-V 'RTOS', which provides multihart support