From 80ef54dba2411f9354b3793d5832c5d8ad871b4b Mon Sep 17 00:00:00 2001 From: Tim Newsome Date: Thu, 7 Feb 2019 13:24:44 -0800 Subject: [PATCH] Rtos riscv (#350) * Implement riscv_get_thread_reg(). This is necessary because riscv_get_gdb_reg_list() now reads all registers, which ended up causing `-rtos riscv` to read all registers whenever one was requested (because the register cache is wiped every time we switch to a different hart). CustomRegisterTest went from 1329s to 106s. Change-Id: I8e9918b7a532d44bca927f67aae5ac34954a8d32 * Also implement riscv_set_reg(). Now all the `-rtos riscv` tests pass again, at regular speed. Change-Id: I55164224672d9dcc9eb4d1184b47258ff3c2cff1 * Better error messages. Change-Id: I4125f9a54750d9d0ee22c4fa84b9dd3f5af203f5 * Add target_get_gdb_reg_list_noread(). Being explicit about what's expected gets `-rtos riscv` back to `-rtos hwthread` time. Change-Id: I6e57390c2fe79b5e6799bfda980d89697e2e29f7 * Revert a change I made that has no effect. I don't understand exactly what all this test protects against, and I shouldn't change it unless I do. Change-Id: Ib329d4e34d65d2b38559b89b7afb3678f439ad2c --- src/rtos/hwthread.c | 13 ++++++--- src/rtos/riscv_debug.c | 52 +++++++++++++++++++++++++++++------- src/rtos/rtos.c | 16 ++++++----- src/server/gdb_server.c | 10 +++---- src/target/riscv/riscv-013.c | 6 ++--- src/target/riscv/riscv.c | 49 +++++++++++++++++++-------------- src/target/target.c | 11 ++++++++ src/target/target.h | 10 +++++++ src/target/target_type.h | 7 +++++ 9 files changed, 127 insertions(+), 47 deletions(-) diff --git a/src/rtos/hwthread.c b/src/rtos/hwthread.c index 8a271fb54..1ad9e61d7 100644 --- a/src/rtos/hwthread.c +++ b/src/rtos/hwthread.c @@ -270,15 +270,22 @@ static int hwthread_get_thread_reg(struct rtos *rtos, int64_t thread_id, struct target *target = rtos->target; struct target *curr = find_thread(target, thread_id); - if (curr == NULL) + if (curr == NULL) { + LOG_ERROR("Couldn't find RTOS thread for id %" PRId64 ".", thread_id); return ERROR_FAIL; + } - if (!target_was_examined(curr)) + if (!target_was_examined(curr)) { + LOG_ERROR("Target %d hasn't been examined yet.", curr->coreid); return ERROR_FAIL; + } struct reg *reg = register_get_by_number(curr->reg_cache, reg_num, true); - if (!reg) + if (!reg) { + LOG_ERROR("Couldn't find register %d in thread %" PRId64 ".", reg_num, + thread_id); return ERROR_FAIL; + } if (reg->type->get(reg) != ERROR_OK) return ERROR_FAIL; diff --git a/src/rtos/riscv_debug.c b/src/rtos/riscv_debug.c index d5cd4a1bf..4695e4e48 100644 --- a/src/rtos/riscv_debug.c +++ b/src/rtos/riscv_debug.c @@ -3,9 +3,11 @@ #endif #include "riscv_debug.h" +#include "target/register.h" #include "target/target.h" #include "target/riscv/riscv.h" #include "server/gdb_server.h" +#include "helper/binarybuffer.h" static int riscv_gdb_thread_packet(struct connection *connection, const char *packet, int packet_size); static int riscv_gdb_v_packet(struct connection *connection, const char *packet, int packet_size); @@ -174,6 +176,7 @@ static int riscv_gdb_thread_packet(struct connection *connection, const char *pa break; default: riscv_set_rtos_hartid(target, tid - 1); + rtos->current_threadid = tid; break; } @@ -270,6 +273,27 @@ static int riscv_gdb_v_packet(struct connection *connection, const char *packet, return GDB_THREAD_PACKET_NOT_CONSUMED; } +static int riscv_get_thread_reg(struct rtos *rtos, int64_t thread_id, + uint32_t reg_num, struct rtos_reg *rtos_reg) +{ + LOG_DEBUG("thread_id=%" PRId64 ", reg_num=%d", thread_id, reg_num); + + struct target *target = rtos->target; + struct reg *reg = register_get_by_number(target->reg_cache, reg_num, true); + if (!reg) + return ERROR_FAIL; + + uint64_t reg_value = 0; + if (riscv_get_register_on_hart(rtos->target, ®_value, thread_id - 1, + reg_num) != ERROR_OK) + return ERROR_FAIL; + + buf_set_u64(rtos_reg->value, 0, 64, reg_value); + rtos_reg->number = reg->number; + rtos_reg->size = reg->size; + return ERROR_OK; +} + static int riscv_get_thread_reg_list(struct rtos *rtos, int64_t thread_id, struct rtos_reg **reg_list, int *num_regs) { @@ -277,11 +301,10 @@ static int riscv_get_thread_reg_list(struct rtos *rtos, int64_t thread_id, /* We return just the GPRs here. */ - *num_regs = 32; + *num_regs = 33; int xlen = riscv_xlen_of_hart(rtos->target, thread_id - 1); *reg_list = calloc(*num_regs, sizeof(struct rtos_reg)); - *reg_list = 0; for (int i = 0; i < *num_regs; ++i) { uint64_t reg_value; if (riscv_get_register_on_hart(rtos->target, ®_value, thread_id - 1, @@ -290,18 +313,25 @@ static int riscv_get_thread_reg_list(struct rtos *rtos, int64_t thread_id, (*reg_list)[i].number = i; (*reg_list)[i].size = xlen; - (*reg_list)[i].value[0] = reg_value & 0xff; - (*reg_list)[i].value[1] = (reg_value >> 8) & 0xff; - (*reg_list)[i].value[2] = (reg_value >> 16) & 0xff; - (*reg_list)[i].value[3] = (reg_value >> 24) & 0xff; - (*reg_list)[i].value[4] = (reg_value >> 32) & 0xff; - (*reg_list)[i].value[5] = (reg_value >> 40) & 0xff; - (*reg_list)[i].value[6] = (reg_value >> 48) & 0xff; - (*reg_list)[i].value[7] = (reg_value >> 56) & 0xff; + buf_set_u64((*reg_list)[i].value, 0, 64, reg_value); } return JIM_OK; } +static int riscv_set_reg(struct rtos *rtos, uint32_t reg_num, + uint8_t *reg_value) +{ + struct target *target = rtos->target; + struct reg *reg = register_get_by_number(target->reg_cache, reg_num, true); + if (!reg) + return ERROR_FAIL; + + int hartid = rtos->current_threadid - 1; + uint64_t value = buf_get_u64(reg_value, 0, reg->size); + + return riscv_set_register_on_hart(target, hartid, reg_num, value); +} + static int riscv_get_symbol_list_to_lookup(symbol_table_elem_t *symbol_list[]) { *symbol_list = calloc(1, sizeof(symbol_table_elem_t)); @@ -315,6 +345,8 @@ const struct rtos_type riscv_rtos = { .detect_rtos = riscv_detect_rtos, .create = riscv_create_rtos, .update_threads = riscv_update_threads, + .get_thread_reg = riscv_get_thread_reg, .get_thread_reg_list = riscv_get_thread_reg_list, .get_symbol_list_to_lookup = riscv_get_symbol_list_to_lookup, + .set_reg = riscv_set_reg, }; diff --git a/src/rtos/rtos.c b/src/rtos/rtos.c index b055f91de..a7c28f9c4 100644 --- a/src/rtos/rtos.c +++ b/src/rtos/rtos.c @@ -475,8 +475,8 @@ int rtos_get_gdb_reg(struct connection *connection, int reg_num) struct rtos_reg *reg_list; int num_regs; - LOG_DEBUG("RTOS: getting register %d for thread 0x%" PRIx64 - ", target->rtos->current_thread=0x%" PRIx64 "\r\n", + LOG_DEBUG("getting register %d for thread 0x%" PRIx64 + ", target->rtos->current_thread=0x%" PRIx64, reg_num, current_threadid, target->rtos->current_thread); @@ -487,15 +487,19 @@ int rtos_get_gdb_reg(struct connection *connection, int reg_num) num_regs = 1; retval = target->rtos->type->get_thread_reg(target->rtos, current_threadid, reg_num, ®_list[0]); + if (retval != ERROR_OK) { + LOG_ERROR("RTOS: failed to get register %d", reg_num); + return retval; + } } else { retval = target->rtos->type->get_thread_reg_list(target->rtos, current_threadid, ®_list, &num_regs); - } - if (retval != ERROR_OK) { - LOG_ERROR("RTOS: failed to get register list"); - return retval; + if (retval != ERROR_OK) { + LOG_ERROR("RTOS: failed to get register list"); + return retval; + } } for (int i = 0; i < num_regs; ++i) { diff --git a/src/server/gdb_server.c b/src/server/gdb_server.c index b63000173..8f1fceeaa 100644 --- a/src/server/gdb_server.c +++ b/src/server/gdb_server.c @@ -96,7 +96,7 @@ struct gdb_connection { char *thread_list; }; -#if 1 +#if 0 #define _DEBUG_GDB_IO_ #endif @@ -1307,7 +1307,7 @@ static int gdb_get_register_packet(struct connection *connection, if ((target->rtos != NULL) && (ERROR_OK == rtos_get_gdb_reg(connection, reg_num))) return ERROR_OK; - retval = target_get_gdb_reg_list(target, ®_list, ®_list_size, + retval = target_get_gdb_reg_list_noread(target, ®_list, ®_list_size, REG_CLASS_ALL); if (retval != ERROR_OK) return gdb_error(connection, retval); @@ -1367,7 +1367,7 @@ static int gdb_set_register_packet(struct connection *connection, return ERROR_OK; } - retval = target_get_gdb_reg_list(target, ®_list, ®_list_size, + retval = target_get_gdb_reg_list_noread(target, ®_list, ®_list_size, REG_CLASS_ALL); if (retval != ERROR_OK) { free(bin_buf); @@ -2221,7 +2221,7 @@ static int gdb_generate_target_description(struct target *target, char **tdesc_o arch_defined_types = calloc(1, sizeof(char *)); - retval = target_get_gdb_reg_list(target, ®_list, + retval = target_get_gdb_reg_list_noread(target, ®_list, ®_list_size, REG_CLASS_ALL); if (retval != ERROR_OK) { @@ -2409,7 +2409,7 @@ static int gdb_target_description_supported(struct target *target, int *supporte char const *architecture = target_get_gdb_arch(target); - retval = target_get_gdb_reg_list(target, ®_list, + retval = target_get_gdb_reg_list_noread(target, ®_list, ®_list_size, REG_CLASS_ALL); if (retval != ERROR_OK) { LOG_ERROR("get register list failed"); diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c index fe1d4cb0a..b0a70ebe9 100644 --- a/src/target/riscv/riscv-013.c +++ b/src/target/riscv/riscv-013.c @@ -1138,7 +1138,7 @@ static int register_write_direct(struct target *target, unsigned number, RISCV013_INFO(info); RISCV_INFO(r); - LOG_DEBUG("[%d] reg[0x%x] <- 0x%" PRIx64, riscv_current_hartid(target), + LOG_DEBUG("{%d} reg[0x%x] <- 0x%" PRIx64, riscv_current_hartid(target), number, value); int result = register_write_abstract(target, number, value, @@ -1321,7 +1321,7 @@ static int register_read_direct(struct target *target, uint64_t *value, uint32_t } if (result == ERROR_OK) { - LOG_DEBUG("[%d] reg[0x%x] = 0x%" PRIx64, riscv_current_hartid(target), + LOG_DEBUG("{%d} reg[0x%x] = 0x%" PRIx64, riscv_current_hartid(target), number, *value); } @@ -2988,7 +2988,7 @@ static enum riscv_halt_reason riscv013_halt_reason(struct target *target) * already set when we connected. Force enumeration now, which has the * side effect of clearing any triggers we did not set. */ riscv_enumerate_triggers(target); - LOG_DEBUG("[%d] halted because of trigger", target->coreid); + LOG_DEBUG("{%d} halted because of trigger", target->coreid); return RISCV_HALT_TRIGGER; case CSR_DCSR_CAUSE_STEP: return RISCV_HALT_SINGLESTEP; diff --git a/src/target/riscv/riscv.c b/src/target/riscv/riscv.c index 01b84bfda..b3464c4b3 100644 --- a/src/target/riscv/riscv.c +++ b/src/target/riscv/riscv.c @@ -919,13 +919,13 @@ static int riscv_write_memory(struct target *target, target_addr_t address, return tt->write_memory(target, address, size, count, buffer); } -static int riscv_get_gdb_reg_list(struct target *target, +static int riscv_get_gdb_reg_list_internal(struct target *target, struct reg **reg_list[], int *reg_list_size, - enum target_register_class reg_class) + enum target_register_class reg_class, bool read) { RISCV_INFO(r); - LOG_DEBUG("reg_class=%d", reg_class); - LOG_DEBUG("rtos_hartid=%d current_hartid=%d", r->rtos_hartid, r->current_hartid); + LOG_DEBUG("rtos_hartid=%d, current_hartid=%d, reg_class=%d, read=%d", + r->rtos_hartid, r->current_hartid, reg_class, read); if (!target->reg_cache) { LOG_ERROR("Target not initialized. Return ERROR_FAIL."); @@ -951,24 +951,36 @@ static int riscv_get_gdb_reg_list(struct target *target, if (!*reg_list) return ERROR_FAIL; - bool read = true; for (int i = 0; i < *reg_list_size; i++) { assert(!target->reg_cache->reg_list[i].valid || target->reg_cache->reg_list[i].size > 0); (*reg_list)[i] = &target->reg_cache->reg_list[i]; if (read && !target->reg_cache->reg_list[i].valid) { - /* This function gets called from - * gdb_target_description_supported(), and we end up failing in - * that case. Allow failures for now. */ if (target->reg_cache->reg_list[i].type->get( &target->reg_cache->reg_list[i]) != ERROR_OK) - read = false; + return ERROR_FAIL; } } return ERROR_OK; } +static int riscv_get_gdb_reg_list_noread(struct target *target, + struct reg **reg_list[], int *reg_list_size, + enum target_register_class reg_class) +{ + return riscv_get_gdb_reg_list_internal(target, reg_list, reg_list_size, + reg_class, false); +} + +static int riscv_get_gdb_reg_list(struct target *target, + struct reg **reg_list[], int *reg_list_size, + enum target_register_class reg_class) +{ + return riscv_get_gdb_reg_list_internal(target, reg_list, reg_list_size, + reg_class, true); +} + static int riscv_arch_state(struct target *target) { struct target_type *tt = get_target_type(target); @@ -1960,6 +1972,7 @@ struct target_type riscv_target = { .checksum_memory = riscv_checksum_memory, .get_gdb_reg_list = riscv_get_gdb_reg_list, + .get_gdb_reg_list_noread = riscv_get_gdb_reg_list_noread, .add_breakpoint = riscv_add_breakpoint, .remove_breakpoint = riscv_remove_breakpoint, @@ -2140,6 +2153,7 @@ void riscv_invalidate_register_cache(struct target *target) { RISCV_INFO(r); + LOG_DEBUG("[%d]", target->coreid); register_cache_invalidate(target->reg_cache); for (size_t i = 0; i < target->reg_cache->num_regs; ++i) { struct reg *reg = &target->reg_cache->reg_list[i]; @@ -2196,7 +2210,7 @@ int riscv_set_register_on_hart(struct target *target, int hartid, enum gdb_regno regid, uint64_t value) { RISCV_INFO(r); - LOG_DEBUG("[%d] %s <- %" PRIx64, hartid, gdb_regno_name(regid), value); + LOG_DEBUG("{%d} %s <- %" PRIx64, hartid, gdb_regno_name(regid), value); assert(r->set_register); return r->set_register(target, hartid, regid, value); } @@ -2213,21 +2227,16 @@ int riscv_get_register_on_hart(struct target *target, riscv_reg_t *value, { RISCV_INFO(r); - if (hartid != riscv_current_hartid(target)) - riscv_invalidate_register_cache(target); - struct reg *reg = &target->reg_cache->reg_list[regid]; - if (reg && reg->valid) { + + if (reg && reg->valid && hartid == riscv_current_hartid(target)) { *value = buf_get_u64(reg->value, 0, reg->size); return ERROR_OK; } int result = r->get_register(target, value, hartid, regid); - if (hartid != riscv_current_hartid(target)) - riscv_invalidate_register_cache(target); - - LOG_DEBUG("[%d] %s: %" PRIx64, hartid, gdb_regno_name(regid), *value); + LOG_DEBUG("{%d} %s: %" PRIx64, hartid, gdb_regno_name(regid), *value); return result; } @@ -2441,7 +2450,7 @@ static int register_get(struct reg *reg) (reg->number >= GDB_REGNO_FPR0 && reg->number <= GDB_REGNO_FPR31) || reg->number == GDB_REGNO_PC) reg->valid = true; - LOG_DEBUG("[%d,%d] read 0x%" PRIx64 " from %s (valid=%d)", + LOG_DEBUG("[%d]{%d} read 0x%" PRIx64 " from %s (valid=%d)", target->coreid, riscv_current_hartid(target), value, reg->name, reg->valid); return ERROR_OK; @@ -2454,7 +2463,7 @@ static int register_set(struct reg *reg, uint8_t *buf) uint64_t value = buf_get_u64(buf, 0, reg->size); - LOG_DEBUG("[%d,%d] write 0x%" PRIx64 " to %s (valid=%d)", + LOG_DEBUG("[%d]{%d} write 0x%" PRIx64 " to %s (valid=%d)", target->coreid, riscv_current_hartid(target), value, reg->name, reg->valid); struct reg *r = &target->reg_cache->reg_list[reg->number]; diff --git a/src/target/target.c b/src/target/target.c index ffd82fb59..bc7cba67c 100644 --- a/src/target/target.c +++ b/src/target/target.c @@ -1223,6 +1223,17 @@ int target_get_gdb_reg_list(struct target *target, return target->type->get_gdb_reg_list(target, reg_list, reg_list_size, reg_class); } +int target_get_gdb_reg_list_noread(struct target *target, + struct reg **reg_list[], int *reg_list_size, + enum target_register_class reg_class) +{ + if (target->type->get_gdb_reg_list_noread && + target->type->get_gdb_reg_list_noread(target, reg_list, + reg_list_size, reg_class) == ERROR_OK) + return ERROR_OK; + return target_get_gdb_reg_list(target, reg_list, reg_list_size, reg_class); +} + bool target_supports_gdb_connection(struct target *target) { /* diff --git a/src/target/target.h b/src/target/target.h index fb9d71465..7c9db3090 100644 --- a/src/target/target.h +++ b/src/target/target.h @@ -496,6 +496,16 @@ int target_get_gdb_reg_list(struct target *target, struct reg **reg_list[], int *reg_list_size, enum target_register_class reg_class); +/** + * Obtain the registers for GDB, but don't read register values from the + * target. + * + * This routine is a wrapper for target->type->get_gdb_reg_list_noread. + */ +int target_get_gdb_reg_list_noread(struct target *target, + struct reg **reg_list[], int *reg_list_size, + enum target_register_class reg_class); + /** * Check if @a target allows GDB connections. * diff --git a/src/target/target_type.h b/src/target/target_type.h index a8928911f..b825c7bb6 100644 --- a/src/target/target_type.h +++ b/src/target/target_type.h @@ -111,6 +111,13 @@ struct target_type { int (*get_gdb_reg_list)(struct target *target, struct reg **reg_list[], int *reg_list_size, enum target_register_class reg_class); + /** + * Same as get_gdb_reg_list, but doesn't read the register values. + * */ + int (*get_gdb_reg_list_noread)(struct target *target, + struct reg **reg_list[], int *reg_list_size, + enum target_register_class reg_class); + /* target memory access * size: 1 = byte (8bit), 2 = half-word (16bit), 4 = word (32bit) * count: number of items of