From 919a98a05b8d55ab807a77f789fa35f42e46731b Mon Sep 17 00:00:00 2001 From: Evgeniy Naydanov Date: Mon, 24 Apr 2023 13:59:54 +0300 Subject: [PATCH 1/5] target/riscv: fix register cache flushing Since writing a register can make some GPRs dirty (e.g. S0, S1), registers should be flushed in reverse order, so GPRs are flushed last. Change-Id: Ice352a4df4ae064619c0f9905db634a7b57e4711 Signed-off-by: Evgeniy Naydanov --- src/target/riscv/riscv.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/src/target/riscv/riscv.c b/src/target/riscv/riscv.c index 609f8c3b3..6c694df9e 100644 --- a/src/target/riscv/riscv.c +++ b/src/target/riscv/riscv.c @@ -1495,6 +1495,15 @@ static int old_or_new_riscv_poll(struct target *target) return riscv_openocd_poll(target); } +static struct reg *get_reg_cache_entry(struct target *target, + unsigned int number) +{ + assert(target->reg_cache); + assert(target->reg_cache->reg_list); + assert(number < target->reg_cache->num_regs); + return &target->reg_cache->reg_list[number]; +} + int riscv_flush_registers(struct target *target) { RISCV_INFO(r); @@ -1502,21 +1511,25 @@ int riscv_flush_registers(struct target *target) if (!target->reg_cache) return ERROR_OK; - LOG_DEBUG("[%s]", target_name(target)); + LOG_TARGET_DEBUG(target, ""); - for (uint32_t number = 0; number < target->reg_cache->num_regs; number++) { - struct reg *reg = &target->reg_cache->reg_list[number]; + /* Writing non-GPR registers may require progbuf execution, and some GPRs + * may become dirty in the process (e.g. S0, S1). For that reason, flush + * registers in reverse order, so that GPRs are flushed last. + */ + for (unsigned int number = target->reg_cache->num_regs; number-- > 0; ) { + struct reg *reg = get_reg_cache_entry(target, number); if (reg->valid && reg->dirty) { - uint64_t value = buf_get_u64(reg->value, 0, reg->size); - LOG_DEBUG("[%s] %s is dirty; write back 0x%" PRIx64, - target_name(target), reg->name, value); - int result = r->set_register(target, number, value); - if (result != ERROR_OK) + riscv_reg_t value = buf_get_u64(reg->value, 0, reg->size); + + LOG_TARGET_DEBUG(target, "%s is dirty; write back 0x%" PRIx64, + reg->name, value); + if (r->set_register(target, number, value) != ERROR_OK) return ERROR_FAIL; reg->dirty = false; } } - + LOG_TARGET_DEBUG(target, "Flush of register cache completed"); return ERROR_OK; } From 7a181e8bbcb5fa20305c136e8fe77a8f3a871539 Mon Sep 17 00:00:00 2001 From: Evgeniy Naydanov Date: Mon, 24 Apr 2023 14:09:08 +0300 Subject: [PATCH 2/5] target/riscv: use `riscv_reg_t` and `enum gbb_regno` consistently Change-Id: Ia476251e835fa5fd129ae6b679c6049c5c60c716 Signed-off-by: Evgeniy Naydanov --- src/target/riscv/riscv-011.c | 9 +++-- src/target/riscv/riscv-013.c | 74 +++++++++++++++++++----------------- src/target/riscv/riscv.h | 11 ++++-- 3 files changed, 52 insertions(+), 42 deletions(-) diff --git a/src/target/riscv/riscv-011.c b/src/target/riscv/riscv-011.c index e23a89d93..e4e6e43b4 100644 --- a/src/target/riscv/riscv-011.c +++ b/src/target/riscv/riscv-011.c @@ -212,7 +212,8 @@ typedef struct { static int poll_target(struct target *target, bool announce); static int riscv011_poll(struct target *target); -static int get_register(struct target *target, riscv_reg_t *value, int regid); +static int get_register(struct target *target, riscv_reg_t *value, + enum gdb_regno regid); /*** Utility functions. ***/ @@ -1324,7 +1325,8 @@ static int register_write(struct target *target, unsigned int number, return ERROR_OK; } -static int get_register(struct target *target, riscv_reg_t *value, int regid) +static int get_register(struct target *target, riscv_reg_t *value, + enum gdb_regno regid) { riscv011_info_t *info = get_info(target); @@ -1368,7 +1370,8 @@ static int get_register(struct target *target, riscv_reg_t *value, int regid) return ERROR_OK; } -static int set_register(struct target *target, int regid, uint64_t value) +static int set_register(struct target *target, enum gdb_regno regid, + riscv_reg_t value) { return register_write(target, regid, value); } diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c index ebe4951c3..0c94728de 100644 --- a/src/target/riscv/riscv-013.c +++ b/src/target/riscv/riscv-013.c @@ -36,8 +36,9 @@ static void riscv013_clear_abstract_error(struct target *target); /* Implementations of the functions in struct riscv_info. */ static int riscv013_get_register(struct target *target, - riscv_reg_t *value, int rid); -static int riscv013_set_register(struct target *target, int regid, uint64_t value); + riscv_reg_t *value, enum gdb_regno rid); +static int riscv013_set_register(struct target *target, enum gdb_regno regid, + riscv_reg_t value); static int dm013_select_hart(struct target *target, int hart_index); static int riscv013_halt_prep(struct target *target); static int riscv013_halt_go(struct target *target); @@ -57,9 +58,10 @@ static void riscv013_fill_dmi_write_u64(struct target *target, char *buf, int a, static void riscv013_fill_dmi_read_u64(struct target *target, char *buf, int a); static int riscv013_dmi_write_u64_bits(struct target *target); static void riscv013_fill_dmi_nop_u64(struct target *target, char *buf); -static int register_read_direct(struct target *target, uint64_t *value, uint32_t number); -static int register_write_direct(struct target *target, unsigned number, - uint64_t value); +static int register_read_direct(struct target *target, riscv_reg_t *value, + enum gdb_regno number); +static int register_write_direct(struct target *target, enum gdb_regno number, + riscv_reg_t value); static int read_memory(struct target *target, target_addr_t address, uint32_t size, uint32_t count, uint8_t *buffer, uint32_t increment); static int write_memory(struct target *target, target_addr_t address, @@ -875,8 +877,8 @@ static uint32_t access_register_command(struct target *target, uint32_t number, return command; } -static int register_read_abstract(struct target *target, uint64_t *value, - uint32_t number, unsigned size) +static int register_read_abstract(struct target *target, riscv_reg_t *value, + enum gdb_regno number, unsigned int size) { RISCV013_INFO(info); @@ -913,8 +915,8 @@ static int register_read_abstract(struct target *target, uint64_t *value, return ERROR_OK; } -static int register_write_abstract(struct target *target, uint32_t number, - uint64_t value, unsigned size) +static int register_write_abstract(struct target *target, enum gdb_regno number, + riscv_reg_t value, unsigned int size) { RISCV013_INFO(info); @@ -1043,7 +1045,7 @@ static int examine_progbuf(struct target *target) return ERROR_OK; } -static int is_fpu_reg(uint32_t gdb_regno) +static int is_fpu_reg(enum gdb_regno gdb_regno) { return (gdb_regno >= GDB_REGNO_FPR0 && gdb_regno <= GDB_REGNO_FPR31) || (gdb_regno == GDB_REGNO_CSR0 + CSR_FFLAGS) || @@ -1051,7 +1053,7 @@ static int is_fpu_reg(uint32_t gdb_regno) (gdb_regno == GDB_REGNO_CSR0 + CSR_FCSR); } -static int is_vector_reg(uint32_t gdb_regno) +static int is_vector_reg(enum gdb_regno gdb_regno) { return (gdb_regno >= GDB_REGNO_V0 && gdb_regno <= GDB_REGNO_V31) || gdb_regno == GDB_REGNO_VSTART || @@ -1063,8 +1065,8 @@ static int is_vector_reg(uint32_t gdb_regno) gdb_regno == GDB_REGNO_VLENB; } -static int prep_for_register_access(struct target *target, uint64_t *mstatus, - int regno) +static int prep_for_register_access(struct target *target, riscv_reg_t *mstatus, + enum gdb_regno regno) { if (is_fpu_reg(regno) || is_vector_reg(regno)) { if (register_read_direct(target, mstatus, GDB_REGNO_MSTATUS) != ERROR_OK) @@ -1085,7 +1087,7 @@ static int prep_for_register_access(struct target *target, uint64_t *mstatus, } static int cleanup_after_register_access(struct target *target, - uint64_t mstatus, int regno) + riscv_reg_t mstatus, enum gdb_regno regno) { if ((is_fpu_reg(regno) && (mstatus & MSTATUS_FS) == 0) || (is_vector_reg(regno) && (mstatus & MSTATUS_VS) == 0)) @@ -1254,7 +1256,7 @@ static int scratch_write64(struct target *target, scratch_mem_t *scratch, } /** Return register size in bits. */ -static unsigned register_size(struct target *target, unsigned number) +static unsigned int register_size(struct target *target, enum gdb_regno number) { /* If reg_cache hasn't been initialized yet, make a guess. We need this for * when this function is called during examine(). */ @@ -1276,12 +1278,12 @@ static bool has_sufficient_progbuf(struct target *target, unsigned size) * Immediately write the new value to the requested register. This mechanism * bypasses any caches. */ -static int register_write_direct(struct target *target, unsigned number, - uint64_t value) +static int register_write_direct(struct target *target, enum gdb_regno number, + riscv_reg_t value) { LOG_TARGET_DEBUG(target, "%s <- 0x%" PRIx64, gdb_regno_name(number), value); - uint64_t mstatus; + riscv_reg_t mstatus; if (prep_for_register_access(target, &mstatus, number) != ERROR_OK) return ERROR_FAIL; @@ -1378,12 +1380,13 @@ static int register_write_direct(struct target *target, unsigned number, } /** Actually read registers from the target right now. */ -static int register_read_direct(struct target *target, uint64_t *value, uint32_t number) +static int register_read_direct(struct target *target, riscv_reg_t *value, + enum gdb_regno number) { if (dm013_select_target(target) != ERROR_OK) return ERROR_FAIL; - uint64_t mstatus; + riscv_reg_t mstatus; if (prep_for_register_access(target, &mstatus, number) != ERROR_OK) return ERROR_FAIL; @@ -1910,8 +1913,8 @@ static COMMAND_HELPER(riscv013_print_info, struct target *target) return 0; } -static int prep_for_vector_access(struct target *target, uint64_t *saved_vtype, - uint64_t *saved_vl, unsigned *debug_vl, unsigned *debug_vsew) +static int prep_for_vector_access(struct target *target, riscv_reg_t *saved_vtype, + riscv_reg_t *saved_vl, unsigned int *debug_vl, unsigned int *debug_vsew) { RISCV_INFO(r); /* TODO: this continuous save/restore is terrible for performance. */ @@ -1964,8 +1967,8 @@ static int prep_for_vector_access(struct target *target, uint64_t *saved_vtype, return ERROR_OK; } -static int cleanup_after_vector_access(struct target *target, uint64_t vtype, - uint64_t vl) +static int cleanup_after_vector_access(struct target *target, riscv_reg_t vtype, + riscv_reg_t vl) { /* Restore vtype and vl. */ if (register_write_direct(target, GDB_REGNO_VTYPE, vtype) != ERROR_OK) @@ -1976,7 +1979,7 @@ static int cleanup_after_vector_access(struct target *target, uint64_t vtype, } static int riscv013_get_register_buf(struct target *target, - uint8_t *value, int regno) + uint8_t *value, enum gdb_regno regno) { assert(regno >= GDB_REGNO_V0 && regno <= GDB_REGNO_V31); @@ -1986,11 +1989,11 @@ static int riscv013_get_register_buf(struct target *target, if (riscv_save_register(target, GDB_REGNO_S0) != ERROR_OK) return ERROR_FAIL; - uint64_t mstatus; + riscv_reg_t mstatus; if (prep_for_register_access(target, &mstatus, regno) != ERROR_OK) return ERROR_FAIL; - uint64_t vtype, vl; + riscv_reg_t vtype, vl; unsigned int debug_vl, debug_vsew; if (prep_for_vector_access(target, &vtype, &vl, &debug_vl, &debug_vsew) != ERROR_OK) return ERROR_FAIL; @@ -2014,7 +2017,7 @@ static int riscv013_get_register_buf(struct target *target, * so messed up that attempting to restore isn't going to help. */ result = riscv_program_exec(&program, target); if (result == ERROR_OK) { - uint64_t v; + riscv_reg_t v; if (register_read_direct(target, &v, GDB_REGNO_S0) != ERROR_OK) return ERROR_FAIL; buf_set_u64(value, debug_vsew * i, debug_vsew, v); @@ -2035,7 +2038,7 @@ static int riscv013_get_register_buf(struct target *target, } static int riscv013_set_register_buf(struct target *target, - int regno, const uint8_t *value) + enum gdb_regno regno, const uint8_t *value) { assert(regno >= GDB_REGNO_V0 && regno <= GDB_REGNO_V31); @@ -2045,11 +2048,11 @@ static int riscv013_set_register_buf(struct target *target, if (riscv_save_register(target, GDB_REGNO_S0) != ERROR_OK) return ERROR_FAIL; - uint64_t mstatus; + riscv_reg_t mstatus; if (prep_for_register_access(target, &mstatus, regno) != ERROR_OK) return ERROR_FAIL; - uint64_t vtype, vl; + riscv_reg_t vtype, vl; unsigned int debug_vl, debug_vsew; if (prep_for_vector_access(target, &vtype, &vl, &debug_vl, &debug_vsew) != ERROR_OK) return ERROR_FAIL; @@ -4142,7 +4145,7 @@ struct target_type riscv013_target = { /*** 0.13-specific implementations of various RISC-V helper functions. ***/ static int riscv013_get_register(struct target *target, - riscv_reg_t *value, int rid) + riscv_reg_t *value, enum gdb_regno rid) { LOG_DEBUG("[%s] reading register %s", target_name(target), gdb_regno_name(rid)); @@ -4170,7 +4173,8 @@ static int riscv013_get_register(struct target *target, return result; } -static int riscv013_set_register(struct target *target, int rid, uint64_t value) +static int riscv013_set_register(struct target *target, enum gdb_regno rid, + riscv_reg_t value) { if (dm013_select_target(target) != ERROR_OK) return ERROR_FAIL; @@ -4182,7 +4186,7 @@ static int riscv013_set_register(struct target *target, int rid, uint64_t value) } else if (rid == GDB_REGNO_PC) { LOG_TARGET_DEBUG(target, "writing PC to DPC: 0x%" PRIx64, value); register_write_direct(target, GDB_REGNO_DPC, value); - uint64_t actual_value; + riscv_reg_t actual_value; register_read_direct(target, &actual_value, GDB_REGNO_DPC); LOG_TARGET_DEBUG(target, " actual DPC written: 0x%016" PRIx64, actual_value); if (value != actual_value) { @@ -4191,7 +4195,7 @@ static int riscv013_set_register(struct target *target, int rid, uint64_t value) return ERROR_FAIL; } } else if (rid == GDB_REGNO_PRIV) { - uint64_t dcsr; + riscv_reg_t dcsr; register_read_direct(target, &dcsr, GDB_REGNO_DCSR); dcsr = set_field(dcsr, CSR_DCSR_PRV, get_field(value, VIRT_PRIV_PRV)); dcsr = set_field(dcsr, CSR_DCSR_V, get_field(value, VIRT_PRIV_V)); diff --git a/src/target/riscv/riscv.h b/src/target/riscv/riscv.h index 2d5e82696..fbaf15606 100644 --- a/src/target/riscv/riscv.h +++ b/src/target/riscv/riscv.h @@ -178,10 +178,13 @@ struct riscv_info { /* Helper functions that target the various RISC-V debug spec * implementations. */ - int (*get_register)(struct target *target, riscv_reg_t *value, int regid); - int (*set_register)(struct target *target, int regid, uint64_t value); - int (*get_register_buf)(struct target *target, uint8_t *buf, int regno); - int (*set_register_buf)(struct target *target, int regno, + int (*get_register)(struct target *target, riscv_reg_t *value, + enum gdb_regno regno); + int (*set_register)(struct target *target, enum gdb_regno regno, + riscv_reg_t value); + int (*get_register_buf)(struct target *target, uint8_t *buf, + enum gdb_regno regno); + int (*set_register_buf)(struct target *target, enum gdb_regno regno, const uint8_t *buf); int (*select_target)(struct target *target); int (*get_hart_state)(struct target *target, enum riscv_hart_state *state); From 8f3a617dc74b867735aaa78bddd92b8a2fe57579 Mon Sep 17 00:00:00 2001 From: Evgeniy Naydanov Date: Mon, 24 Apr 2023 15:02:21 +0300 Subject: [PATCH 3/5] target/riscv: improve register caching (riscv_write_register) This commit introduces a new function, which can be used to reduce number of register accesses. Change-Id: I125809726eb7797b11121175c3ad66bedb66dd0d Signed-off-by: Evgeniy Naydanov --- src/target/riscv/riscv.c | 224 +++++++++++++++++++++++++++------------ src/target/riscv/riscv.h | 10 +- 2 files changed, 164 insertions(+), 70 deletions(-) diff --git a/src/target/riscv/riscv.c b/src/target/riscv/riscv.c index 6c694df9e..b7b742a3f 100644 --- a/src/target/riscv/riscv.c +++ b/src/target/riscv/riscv.c @@ -4632,96 +4632,166 @@ static bool gdb_regno_cacheable(enum gdb_regno regno, bool is_write) } /** - * This function is called when the debug user wants to change the value of a - * register. The new value may be cached, and may not be written until the hart - * is resumed. */ -int riscv_set_register(struct target *target, enum gdb_regno regid, riscv_reg_t value) + * This function is used internally by functions that change register values. + * If `write_through` is true, it is ensured that the value of the target's + * register is set to be equal to the `value` argument. The cached value is + * updated if the register is cacheable. + */ +static int riscv_set_or_write_register(struct target *target, + enum gdb_regno regid, riscv_reg_t value, bool write_through) { RISCV_INFO(r); - LOG_DEBUG("[%s] %s <- %" PRIx64, target_name(target), gdb_regno_name(regid), value); assert(r->set_register); keep_alive(); - /* TODO: Hack to deal with gdb that thinks these registers still exist. */ - if (regid > GDB_REGNO_XPR15 && regid <= GDB_REGNO_XPR31 && value == 0 && - riscv_supports_extension(target, 'E')) - return ERROR_OK; - - struct reg *reg = &target->reg_cache->reg_list[regid]; - buf_set_u64(reg->value, 0, reg->size, value); - - if (gdb_regno_cacheable(regid, true)) { - reg->valid = true; - reg->dirty = true; - } else { - if (r->set_register(target, regid, value) != ERROR_OK) - return ERROR_FAIL; + if (!target->reg_cache) { + assert(!target_was_examined(target)); + LOG_TARGET_DEBUG(target, + "No cache, writing to target: %s <- 0x%" PRIx64, + gdb_regno_name(regid), value); + return r->set_register(target, regid, value); } - LOG_DEBUG("[%s] wrote 0x%" PRIx64 " to %s valid=%d", - target_name(target), value, reg->name, reg->valid); + struct reg *reg = get_reg_cache_entry(target, regid); + + if (!reg->exist) { + LOG_TARGET_DEBUG(target, "Register %s does not exist.", reg->name); + return ERROR_FAIL; + } + + if (target->state != TARGET_HALTED) { + LOG_TARGET_DEBUG(target, + "Target not halted, writing to target: %s <- 0x%" PRIx64, + reg->name, value); + return r->set_register(target, regid, value); + } + + const bool need_to_write = !reg->valid || reg->dirty || + value != buf_get_u64(reg->value, 0, reg->size); + const bool cacheable = gdb_regno_cacheable(regid, need_to_write); + + if (!cacheable || (write_through && need_to_write)) { + LOG_TARGET_DEBUG(target, + "Writing to target: %s <- 0x%" PRIx64 " (cacheable=%s, valid=%s, dirty=%s)", + reg->name, value, cacheable ? "true" : "false", + reg->valid ? "true" : "false", + reg->dirty ? "true" : "false"); + if (r->set_register(target, regid, value) != ERROR_OK) + return ERROR_FAIL; + + reg->dirty = false; + } else { + reg->dirty = need_to_write; + } + + buf_set_u64(reg->value, 0, reg->size, value); + reg->valid = cacheable; + + LOG_TARGET_DEBUG(target, + "Wrote 0x%" PRIx64 " to %s (cacheable=%s, valid=%s, dirty=%s)", + value, reg->name, cacheable ? "true" : "false", + reg->valid ? "true" : "false", + reg->dirty ? "true" : "false"); return ERROR_OK; } +/** + * This function is used to change the value of a register. The new value may + * be cached, and may not be written until the hart is resumed. + */ +int riscv_set_register(struct target *target, enum gdb_regno regid, + riscv_reg_t value) +{ + return riscv_set_or_write_register(target, regid, value, + /* write_through */ false); +} + +/** + * This function is used to change the value of a register. The new value may + * be cached, but it will be written to hart immediately. + */ +int riscv_write_register(struct target *target, enum gdb_regno regid, + riscv_reg_t value) +{ + return riscv_set_or_write_register(target, regid, value, + /* write_through */ true); +} + +/** + * This function is used to get the value of a register. If possible, the value + * in cache will be updated. + */ int riscv_get_register(struct target *target, riscv_reg_t *value, enum gdb_regno regid) { RISCV_INFO(r); + assert(r->get_register); keep_alive(); - struct reg *reg = &target->reg_cache->reg_list[regid]; + if (!target->reg_cache) { + assert(!target_was_examined(target)); + LOG_TARGET_DEBUG(target, "No cache, reading %s from target", + gdb_regno_name(regid)); + return r->get_register(target, value, regid); + } + + struct reg *reg = get_reg_cache_entry(target, regid); if (!reg->exist) { - LOG_DEBUG("[%s] %s does not exist.", - target_name(target), gdb_regno_name(regid)); + LOG_TARGET_DEBUG(target, "Register %s does not exist.", reg->name); return ERROR_FAIL; } - if (reg && reg->valid) { + if (reg->valid) { *value = buf_get_u64(reg->value, 0, reg->size); - LOG_DEBUG("[%s] %s: %" PRIx64 " (cached)", target_name(target), - gdb_regno_name(regid), *value); + LOG_TARGET_DEBUG(target, "Read %s: 0x%" PRIx64 " (cached)", reg->name, + *value); return ERROR_OK; } - /* TODO: Hack to deal with gdb that thinks these registers still exist. */ - if (regid > GDB_REGNO_XPR15 && regid <= GDB_REGNO_XPR31 && - riscv_supports_extension(target, 'E')) { - *value = 0; - return ERROR_OK; - } + LOG_TARGET_DEBUG(target, "Reading %s from target", reg->name); + if (r->get_register(target, value, regid) != ERROR_OK) + return ERROR_FAIL; - int result = r->get_register(target, value, regid); + buf_set_u64(reg->value, 0, reg->size, *value); + reg->valid = gdb_regno_cacheable(regid, /* is write? */ false); + reg->dirty = false; - if (result == ERROR_OK) { - /* Update the cache in case we're called from - * riscv_save_register(). */ - buf_set_u64(reg->value, 0, reg->size, *value); - reg->valid = gdb_regno_cacheable(regid, false); - } - - LOG_DEBUG("[%s] %s: %" PRIx64, target_name(target), - gdb_regno_name(regid), *value); - return result; + LOG_TARGET_DEBUG(target, "Read %s: 0x%" PRIx64, reg->name, *value); + return ERROR_OK; } +/** + * This function is used to save the value of a register in cache. The register + * is marked as dirty, and writeback is delayed for as long as possible. + */ int riscv_save_register(struct target *target, enum gdb_regno regid) { + assert(target->state == TARGET_HALTED && + "Doesn't make sense to populate register cache on non-halted targets."); + assert(gdb_regno_cacheable(regid, /* is write? */ false) && + "Only cacheable registers can be saved."); + RISCV_INFO(r); riscv_reg_t value; if (!target->reg_cache) { assert(!target_was_examined(target)); + /* To create register cache it is needed to examine the target first, + * therefore during examine, any changed register needs to be saved + * and restored manually. + */ return ERROR_OK; } - struct reg *reg = &target->reg_cache->reg_list[regid]; - LOG_DEBUG("[%s] save %s", target_name(target), reg->name); + struct reg *reg = get_reg_cache_entry(target, regid); + + LOG_TARGET_DEBUG(target, "Saving %s", reg->name); if (riscv_get_register(target, &value, regid) != ERROR_OK) return ERROR_FAIL; - if (!reg->valid) - return ERROR_FAIL; + assert(reg->valid && + "The register is cacheable, so the cache entry must be valid now."); /* Mark the register dirty. We assume that this function is called * because the caller is about to mess with the underlying value of the * register. */ @@ -5080,21 +5150,34 @@ const char *gdb_regno_name(enum gdb_regno regno) } } + +/** + * This function is the handler of user's request to read a register. + */ static int register_get(struct reg *reg) { - riscv_reg_info_t *reg_info = reg->arch_info; - struct target *target = reg_info->target; + struct target *target = ((riscv_reg_info_t *)reg->arch_info)->target; RISCV_INFO(r); + /* TODO: Hack to deal with gdb that thinks these registers still exist. */ + if (reg->number > GDB_REGNO_XPR15 && reg->number <= GDB_REGNO_XPR31 && + riscv_supports_extension(target, 'E')) { + buf_set_u64(reg->value, 0, reg->size, 0); + return ERROR_OK; + } + if (reg->number >= GDB_REGNO_V0 && reg->number <= GDB_REGNO_V31) { if (!r->get_register_buf) { - LOG_ERROR("Reading register %s not supported on this RISC-V target.", - gdb_regno_name(reg->number)); + LOG_TARGET_ERROR(target, + "Reading register %s not supported on this target.", + reg->name); return ERROR_FAIL; } if (r->get_register_buf(target, reg->value, reg->number) != ERROR_OK) return ERROR_FAIL; + + reg->valid = gdb_regno_cacheable(reg->number, /* is write? */ false); } else { uint64_t value; int result = riscv_get_register(target, &value, reg->number); @@ -5102,33 +5185,32 @@ static int register_get(struct reg *reg) return result; buf_set_u64(reg->value, 0, reg->size, value); } - reg->valid = gdb_regno_cacheable(reg->number, false); char *str = buf_to_hex_str(reg->value, reg->size); - LOG_DEBUG("[%s] read 0x%s from %s (valid=%d)", target_name(target), - str, reg->name, reg->valid); + LOG_TARGET_DEBUG(target, "read 0x%s from %s (valid=%d)", str, reg->name, + reg->valid); free(str); return ERROR_OK; } +/** + * This function is the handler of user's request to write a register. + */ static int register_set(struct reg *reg, uint8_t *buf) { - riscv_reg_info_t *reg_info = reg->arch_info; - struct target *target = reg_info->target; + struct target *target = ((riscv_reg_info_t *)reg->arch_info)->target; RISCV_INFO(r); char *str = buf_to_hex_str(buf, reg->size); - LOG_DEBUG("[%s] write 0x%s to %s (valid=%d)", target_name(target), - str, reg->name, reg->valid); + LOG_TARGET_DEBUG(target, "write 0x%s to %s (valid=%d)", str, reg->name, + reg->valid); free(str); - /* Exit early for writing x0, which on the hardware would be ignored, and we - * don't want to update our cache. */ - if (reg->number == GDB_REGNO_ZERO) + /* TODO: Hack to deal with gdb that thinks these registers still exist. */ + if (reg->number > GDB_REGNO_XPR15 && reg->number <= GDB_REGNO_XPR31 && + riscv_supports_extension(target, 'E') && + buf_get_u64(buf, 0, reg->size) == 0) return ERROR_OK; - memcpy(reg->value, buf, DIV_ROUND_UP(reg->size, 8)); - reg->valid = gdb_regno_cacheable(reg->number, true); - if (reg->number == GDB_REGNO_TDATA1 || reg->number == GDB_REGNO_TDATA2) { r->manual_hwbp_set = true; @@ -5142,15 +5224,19 @@ static int register_set(struct reg *reg, uint8_t *buf) if (reg->number >= GDB_REGNO_V0 && reg->number <= GDB_REGNO_V31) { if (!r->set_register_buf) { - LOG_ERROR("Writing register %s not supported on this RISC-V target.", - gdb_regno_name(reg->number)); + LOG_TARGET_ERROR(target, + "Writing register %s not supported on this target.", + reg->name); return ERROR_FAIL; } - if (r->set_register_buf(target, reg->number, reg->value) != ERROR_OK) + if (r->set_register_buf(target, reg->number, buf) != ERROR_OK) return ERROR_FAIL; + + memcpy(reg->value, buf, DIV_ROUND_UP(reg->size, 8)); + reg->valid = gdb_regno_cacheable(reg->number, /* is write? */ true); } else { - uint64_t value = buf_get_u64(buf, 0, reg->size); + const riscv_reg_t value = buf_get_u64(buf, 0, reg->size); if (riscv_set_register(target, reg->number, value) != ERROR_OK) return ERROR_FAIL; } diff --git a/src/target/riscv/riscv.h b/src/target/riscv/riscv.h index fbaf15606..a9dedbb68 100644 --- a/src/target/riscv/riscv.h +++ b/src/target/riscv/riscv.h @@ -376,8 +376,16 @@ int riscv_current_hartid(const struct target *target); * consecutive and start with mhartid=0. */ unsigned int riscv_count_harts(struct target *target); -/** Set register, updating the cache. */ +/** + * Set the register value. For cacheable registers, only the cache is updated + * (write-back mode). + */ int riscv_set_register(struct target *target, enum gdb_regno i, riscv_reg_t v); +/** + * Set the register value and immediately write it to the target + * (write-through mode). + */ +int riscv_write_register(struct target *target, enum gdb_regno i, riscv_reg_t v); /** Get register, from the cache if it's in there. */ int riscv_get_register(struct target *target, riscv_reg_t *value, enum gdb_regno r); From c822dc8194d750726e60431f711285ef94bcc8ad Mon Sep 17 00:00:00 2001 From: Evgeniy Naydanov Date: Mon, 24 Apr 2023 15:11:11 +0300 Subject: [PATCH 4/5] target/riscv: improve register caching (prep_*, cleanup_*) Introduce riscv_write_register to prep_for_register/vector_access and cleanup_after_register/vector_access. Change-Id: I77a0a06ac6f12eceec309f0aff94aa77bd56ff55 Signed-off-by: Evgeniy Naydanov --- src/target/riscv/riscv-013.c | 197 +++++++++++++++++++---------------- 1 file changed, 107 insertions(+), 90 deletions(-) diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c index 0c94728de..ca80e8a01 100644 --- a/src/target/riscv/riscv-013.c +++ b/src/target/riscv/riscv-013.c @@ -1065,35 +1065,54 @@ static int is_vector_reg(enum gdb_regno gdb_regno) gdb_regno == GDB_REGNO_VLENB; } -static int prep_for_register_access(struct target *target, riscv_reg_t *mstatus, - enum gdb_regno regno) +static int prep_for_register_access(struct target *target, + riscv_reg_t *orig_mstatus, enum gdb_regno regno) { - if (is_fpu_reg(regno) || is_vector_reg(regno)) { - if (register_read_direct(target, mstatus, GDB_REGNO_MSTATUS) != ERROR_OK) - return ERROR_FAIL; - if (is_fpu_reg(regno) && (*mstatus & MSTATUS_FS) == 0) { - if (register_write_direct(target, GDB_REGNO_MSTATUS, - set_field(*mstatus, MSTATUS_FS, 1)) != ERROR_OK) - return ERROR_FAIL; - } else if (is_vector_reg(regno) && (*mstatus & MSTATUS_VS) == 0) { - if (register_write_direct(target, GDB_REGNO_MSTATUS, - set_field(*mstatus, MSTATUS_VS, 1)) != ERROR_OK) - return ERROR_FAIL; - } - } else { - *mstatus = 0; - } + assert(orig_mstatus); + + if (!is_fpu_reg(regno) && !is_vector_reg(regno)) + /* No special preparation needed */ + return ERROR_OK; + + LOG_TARGET_DEBUG(target, "Preparing mstatus to access %s", + gdb_regno_name(regno)); + + /* FIXME: On a running target, there is no way to make sure mstatus won't + * change between reading and writing it, so + * if target->state != TARGET_HALTED an error should be returned here. + * However, this would not allow access to FPU registers on the running + * target. + * See https://github.com/riscv/riscv-openocd/pull/842#discussion_r1178500114 + */ + + if (riscv_get_register(target, orig_mstatus, GDB_REGNO_MSTATUS) != ERROR_OK) + return ERROR_FAIL; + + riscv_reg_t new_mstatus = *orig_mstatus; + riscv_reg_t field_mask = is_fpu_reg(regno) ? MSTATUS_FS : MSTATUS_VS; + + if ((new_mstatus & field_mask) != 0) + return ERROR_OK; + + new_mstatus = set_field(new_mstatus, field_mask, 1); + + if (riscv_write_register(target, GDB_REGNO_MSTATUS, new_mstatus) != ERROR_OK) + return ERROR_FAIL; + + LOG_TARGET_DEBUG(target, "Prepared to access %s (mstatus=0x%" PRIx64 ")", + gdb_regno_name(regno), new_mstatus); return ERROR_OK; } static int cleanup_after_register_access(struct target *target, riscv_reg_t mstatus, enum gdb_regno regno) { - if ((is_fpu_reg(regno) && (mstatus & MSTATUS_FS) == 0) || - (is_vector_reg(regno) && (mstatus & MSTATUS_VS) == 0)) - if (register_write_direct(target, GDB_REGNO_MSTATUS, mstatus) != ERROR_OK) - return ERROR_FAIL; - return ERROR_OK; + if (!is_fpu_reg(regno) && !is_vector_reg(regno)) + /* Mstatus was not changed for this register access. No need to restore it. */ + return ERROR_OK; + + LOG_TARGET_DEBUG(target, "Restoring mstatus to 0x%" PRIx64, mstatus); + return riscv_write_register(target, GDB_REGNO_MSTATUS, mstatus); } typedef enum { @@ -1913,69 +1932,78 @@ static COMMAND_HELPER(riscv013_print_info, struct target *target) return 0; } -static int prep_for_vector_access(struct target *target, riscv_reg_t *saved_vtype, - riscv_reg_t *saved_vl, unsigned int *debug_vl, unsigned int *debug_vsew) +static int try_set_vsew(struct target *target, unsigned int *debug_vsew) { RISCV_INFO(r); - /* TODO: this continuous save/restore is terrible for performance. */ - /* Write vtype and vl. */ + unsigned int encoded_vsew = + (riscv_xlen(target) == 64 && r->vsew64_supported != YNM_NO) ? 3 : 2; + + /* Set standard element width to match XLEN, for vmv instruction to move + * the least significant bits into a GPR. + */ + if (riscv_write_register(target, GDB_REGNO_VTYPE, encoded_vsew << 3) != ERROR_OK) + return ERROR_FAIL; + + if (encoded_vsew == 3 && r->vsew64_supported == YNM_MAYBE) { + /* Check that it's supported. */ + riscv_reg_t vtype; + + if (riscv_get_register(target, &vtype, GDB_REGNO_VTYPE) != ERROR_OK) + return ERROR_FAIL; + if (vtype >> (riscv_xlen(target) - 1)) { + r->vsew64_supported = YNM_NO; + /* Try again. */ + return try_set_vsew(target, debug_vsew); + } + r->vsew64_supported = YNM_YES; + } + *debug_vsew = encoded_vsew == 3 ? 64 : 32; + return ERROR_OK; +} + +static int prep_for_vector_access(struct target *target, + riscv_reg_t *orig_mstatus, riscv_reg_t *orig_vtype, riscv_reg_t *orig_vl, + unsigned int *debug_vl, unsigned int *debug_vsew) +{ + assert(orig_mstatus); + assert(orig_vtype); + assert(orig_vl); + assert(debug_vl); + assert(debug_vsew); + + RISCV_INFO(r); + if (target->state != TARGET_HALTED) { + LOG_TARGET_ERROR(target, + "Unable to access vector register: target not halted"); + return ERROR_FAIL; + } + if (prep_for_register_access(target, orig_mstatus, GDB_REGNO_VL) != ERROR_OK) + return ERROR_FAIL; /* Save vtype and vl. */ - if (register_read_direct(target, saved_vtype, GDB_REGNO_VTYPE) != ERROR_OK) + if (riscv_get_register(target, orig_vtype, GDB_REGNO_VTYPE) != ERROR_OK) return ERROR_FAIL; - if (register_read_direct(target, saved_vl, GDB_REGNO_VL) != ERROR_OK) + if (riscv_get_register(target, orig_vl, GDB_REGNO_VL) != ERROR_OK) return ERROR_FAIL; - while (true) { - unsigned int encoded_vsew; - if (riscv_xlen(target) == 64 && r->vsew64_supported != YNM_NO) { - encoded_vsew = 3; - *debug_vsew = 64; - } else { - encoded_vsew = 2; - *debug_vsew = 32; - } - - /* Set standard element width to match XLEN, for vmv instruction to move - * the least significant bits into a GPR. */ - if (register_write_direct(target, GDB_REGNO_VTYPE, encoded_vsew << 3) != ERROR_OK) - return ERROR_FAIL; - - if (*debug_vsew == 64 && r->vsew64_supported == YNM_MAYBE) { - /* Check that it's supported. */ - uint64_t vtype; - if (register_read_direct(target, &vtype, GDB_REGNO_VTYPE) != ERROR_OK) - return ERROR_FAIL; - if (vtype >> (riscv_xlen(target) - 1)) { - r->vsew64_supported = YNM_NO; - /* Try again. */ - continue; - } else { - r->vsew64_supported = YNM_YES; - } - } - break; - } - + if (try_set_vsew(target, debug_vsew) != ERROR_OK) + return ERROR_FAIL; /* Set the number of elements to be updated with results from a vector * instruction, for the vslide1down instruction. * Set it so the entire V register is updated. */ *debug_vl = DIV_ROUND_UP(r->vlenb * 8, *debug_vsew); - if (register_write_direct(target, GDB_REGNO_VL, *debug_vl) != ERROR_OK) - return ERROR_FAIL; - - return ERROR_OK; + return riscv_write_register(target, GDB_REGNO_VL, *debug_vl); } -static int cleanup_after_vector_access(struct target *target, riscv_reg_t vtype, - riscv_reg_t vl) +static int cleanup_after_vector_access(struct target *target, + riscv_reg_t mstatus, riscv_reg_t vtype, riscv_reg_t vl) { /* Restore vtype and vl. */ - if (register_write_direct(target, GDB_REGNO_VTYPE, vtype) != ERROR_OK) + if (riscv_write_register(target, GDB_REGNO_VTYPE, vtype) != ERROR_OK) return ERROR_FAIL; - if (register_write_direct(target, GDB_REGNO_VL, vl) != ERROR_OK) + if (riscv_write_register(target, GDB_REGNO_VL, vl) != ERROR_OK) return ERROR_FAIL; - return ERROR_OK; + return cleanup_after_register_access(target, mstatus, GDB_REGNO_VL); } static int riscv013_get_register_buf(struct target *target, @@ -1989,13 +2017,10 @@ static int riscv013_get_register_buf(struct target *target, if (riscv_save_register(target, GDB_REGNO_S0) != ERROR_OK) return ERROR_FAIL; - riscv_reg_t mstatus; - if (prep_for_register_access(target, &mstatus, regno) != ERROR_OK) - return ERROR_FAIL; - - riscv_reg_t vtype, vl; + riscv_reg_t mstatus, vtype, vl; unsigned int debug_vl, debug_vsew; - if (prep_for_vector_access(target, &vtype, &vl, &debug_vl, &debug_vsew) != ERROR_OK) + if (prep_for_vector_access(target, &mstatus, &vtype, &vl, + &debug_vl, &debug_vsew) != ERROR_OK) return ERROR_FAIL; unsigned vnum = regno - GDB_REGNO_V0; @@ -2022,16 +2047,14 @@ static int riscv013_get_register_buf(struct target *target, return ERROR_FAIL; buf_set_u64(value, debug_vsew * i, debug_vsew, v); } else { - LOG_TARGET_ERROR(target, "Failed to execute vmv/vslide1down while reading %s", - gdb_regno_name(regno)); + LOG_TARGET_ERROR(target, + "Failed to execute vmv/vslide1down while reading %s", + gdb_regno_name(regno)); break; } } - if (cleanup_after_vector_access(target, vtype, vl) != ERROR_OK) - return ERROR_FAIL; - - if (cleanup_after_register_access(target, mstatus, regno) != ERROR_OK) + if (cleanup_after_vector_access(target, mstatus, vtype, vl) != ERROR_OK) return ERROR_FAIL; return result; @@ -2048,13 +2071,10 @@ static int riscv013_set_register_buf(struct target *target, if (riscv_save_register(target, GDB_REGNO_S0) != ERROR_OK) return ERROR_FAIL; - riscv_reg_t mstatus; - if (prep_for_register_access(target, &mstatus, regno) != ERROR_OK) - return ERROR_FAIL; - - riscv_reg_t vtype, vl; + riscv_reg_t mstatus, vtype, vl; unsigned int debug_vl, debug_vsew; - if (prep_for_vector_access(target, &vtype, &vl, &debug_vl, &debug_vsew) != ERROR_OK) + if (prep_for_vector_access(target, &mstatus, &vtype, &vl, + &debug_vl, &debug_vsew) != ERROR_OK) return ERROR_FAIL; unsigned vnum = regno - GDB_REGNO_V0; @@ -2072,10 +2092,7 @@ static int riscv013_set_register_buf(struct target *target, break; } - if (cleanup_after_vector_access(target, vtype, vl) != ERROR_OK) - return ERROR_FAIL; - - if (cleanup_after_register_access(target, mstatus, regno) != ERROR_OK) + if (cleanup_after_vector_access(target, mstatus, vtype, vl) != ERROR_OK) return ERROR_FAIL; return result; From 5a29a7399fb0aeccab4d03946ff5d982a32c0747 Mon Sep 17 00:00:00 2001 From: Evgeniy Naydanov Date: Mon, 24 Apr 2023 15:28:25 +0300 Subject: [PATCH 5/5] target/riscv: refactor register accesses Change-Id: I45731d501f6261c4142c70afacf3fbbe42cf2806 Signed-off-by: Evgeniy Naydanov --- src/target/riscv/riscv-013.c | 404 ++++++++++++++++++++--------------- src/target/riscv/riscv.c | 6 +- 2 files changed, 236 insertions(+), 174 deletions(-) diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c index ca80e8a01..b4e165595 100644 --- a/src/target/riscv/riscv-013.c +++ b/src/target/riscv/riscv-013.c @@ -58,6 +58,7 @@ static void riscv013_fill_dmi_write_u64(struct target *target, char *buf, int a, static void riscv013_fill_dmi_read_u64(struct target *target, char *buf, int a); static int riscv013_dmi_write_u64_bits(struct target *target); static void riscv013_fill_dmi_nop_u64(struct target *target, char *buf); +static unsigned int register_size(struct target *target, enum gdb_regno number); static int register_read_direct(struct target *target, riscv_reg_t *value, enum gdb_regno number); static int register_write_direct(struct target *target, enum gdb_regno number, @@ -877,8 +878,8 @@ static uint32_t access_register_command(struct target *target, uint32_t number, return command; } -static int register_read_abstract(struct target *target, riscv_reg_t *value, - enum gdb_regno number, unsigned int size) +static int register_read_abstract_with_size(struct target *target, + riscv_reg_t *value, enum gdb_regno number, unsigned int size) { RISCV013_INFO(info); @@ -915,10 +916,19 @@ static int register_read_abstract(struct target *target, riscv_reg_t *value, return ERROR_OK; } +static int register_read_abstract(struct target *target, riscv_reg_t *value, + enum gdb_regno number) +{ + const unsigned int size = register_size(target, number); + + return register_read_abstract_with_size(target, value, number, size); +} + static int register_write_abstract(struct target *target, enum gdb_regno number, - riscv_reg_t value, unsigned int size) + riscv_reg_t value) { RISCV013_INFO(info); + const unsigned int size = register_size(target, number); if (number >= GDB_REGNO_FPR0 && number <= GDB_REGNO_FPR31 && !info->abstract_write_fpr_supported) @@ -1294,22 +1304,43 @@ static bool has_sufficient_progbuf(struct target *target, unsigned size) } /** - * Immediately write the new value to the requested register. This mechanism - * bypasses any caches. + * This function is used to read a 64-bit value from a register by executing a + * program. + * The program stores a register to address located in S0. + * The caller should save S0. */ -static int register_write_direct(struct target *target, enum gdb_regno number, - riscv_reg_t value) +static int internal_register_read64_progbuf_scratch(struct target *target, + struct riscv_program *program, riscv_reg_t *value) { - LOG_TARGET_DEBUG(target, "%s <- 0x%" PRIx64, gdb_regno_name(number), value); + scratch_mem_t scratch; - riscv_reg_t mstatus; - if (prep_for_register_access(target, &mstatus, number) != ERROR_OK) + if (scratch_reserve(target, &scratch, program, 8) != ERROR_OK) return ERROR_FAIL; - int result = register_write_abstract(target, number, value, - register_size(target, number)); - if (result == ERROR_OK || !has_sufficient_progbuf(target, 2)) - return result; + if (register_write_abstract(target, GDB_REGNO_S0, scratch.hart_address) + != ERROR_OK) { + scratch_release(target, &scratch); + return ERROR_FAIL; + } + if (riscv_program_exec(program, target) != ERROR_OK) { + scratch_release(target, &scratch); + return ERROR_FAIL; + } + + int result = scratch_read64(target, &scratch, value); + + scratch_release(target, &scratch); + return result; +} + +/** + * This function reads a register by writing a program to program buffer and + * executing it. + */ +static int register_read_progbuf(struct target *target, uint64_t *value, + enum gdb_regno number) +{ + assert(target->state == TARGET_HALTED); struct riscv_program program; riscv_program_init(&program, target); @@ -1317,170 +1348,183 @@ static int register_write_direct(struct target *target, enum gdb_regno number, if (riscv_save_register(target, GDB_REGNO_S0) != ERROR_OK) return ERROR_FAIL; - scratch_mem_t scratch; - bool use_scratch = false; - if (number >= GDB_REGNO_FPR0 && number <= GDB_REGNO_FPR31 && - riscv_supports_extension(target, 'D') && - riscv_xlen(target) < 64) { - /* There are no instructions to move all the bits from a register, so - * we need to use some scratch RAM. */ - use_scratch = true; - if (riscv_program_insert(&program, fld(number - GDB_REGNO_FPR0, S0, 0)) - != ERROR_OK) - return ERROR_FAIL; + if (number >= GDB_REGNO_FPR0 && number <= GDB_REGNO_FPR31) { + const unsigned int freg = number - GDB_REGNO_FPR0; - if (scratch_reserve(target, &scratch, &program, 8) != ERROR_OK) - return ERROR_FAIL; - - if (register_write_direct(target, GDB_REGNO_S0, scratch.hart_address) - != ERROR_OK) { - scratch_release(target, &scratch); - return ERROR_FAIL; + if (riscv_supports_extension(target, 'D') && riscv_xlen(target) < 64) { + /* There are no instructions to move all the bits from a + * register, so we need to use some scratch RAM. + */ + if (riscv_program_insert(&program, fsd(freg, S0, 0)) != ERROR_OK) + return ERROR_FAIL; + return internal_register_read64_progbuf_scratch(target, &program, + value); } - - if (scratch_write64(target, &scratch, value) != ERROR_OK) { - scratch_release(target, &scratch); + if (riscv_program_insert(&program, + riscv_supports_extension(target, 'D') ? + fmv_x_d(S0, freg) : fmv_x_w(S0, freg)) != ERROR_OK) + return ERROR_FAIL; + } else if (number >= GDB_REGNO_CSR0 && number <= GDB_REGNO_CSR4095) { + if (riscv_program_csrr(&program, S0, number) != ERROR_OK) return ERROR_FAIL; - } - } else { - if (number >= GDB_REGNO_FPR0 && number <= GDB_REGNO_FPR31) { - if (riscv_supports_extension(target, 'D')) { - if (riscv_program_insert(&program, - fmv_d_x(number - GDB_REGNO_FPR0, S0)) != ERROR_OK) - return ERROR_FAIL; - } else { - if (riscv_program_insert(&program, - fmv_w_x(number - GDB_REGNO_FPR0, S0)) != ERROR_OK) - return ERROR_FAIL; - } - } else if (number == GDB_REGNO_VTYPE) { - if (riscv_save_register(target, GDB_REGNO_S1) != ERROR_OK) - return ERROR_FAIL; - if (riscv_program_insert(&program, csrr(S1, CSR_VL)) != ERROR_OK) - return ERROR_FAIL; - if (riscv_program_insert(&program, vsetvl(ZERO, S1, S0)) != ERROR_OK) - return ERROR_FAIL; - } else if (number == GDB_REGNO_VL) { - /* "The XLEN-bit-wide read-only vl CSR can only be updated by the - * vsetvli and vsetvl instructions, and the fault-only-rst vector - * load instruction variants." */ - if (riscv_save_register(target, GDB_REGNO_S1) != ERROR_OK) - return ERROR_FAIL; - if (riscv_program_insert(&program, csrr(S1, CSR_VTYPE)) != ERROR_OK) - return ERROR_FAIL; - if (riscv_program_insert(&program, vsetvl(ZERO, S0, S1)) != ERROR_OK) - return ERROR_FAIL; - } else if (number >= GDB_REGNO_CSR0 && number <= GDB_REGNO_CSR4095) { - if (riscv_program_csrw(&program, S0, number) != ERROR_OK) - return ERROR_FAIL; - } else { - LOG_ERROR("Unsupported register (enum gdb_regno)(%d)", number); - return ERROR_FAIL; - } - if (register_write_direct(target, GDB_REGNO_S0, value) != ERROR_OK) - return ERROR_FAIL; + LOG_ERROR("Unsupported register: %s", gdb_regno_name(number)); + return ERROR_FAIL; } - int exec_out = riscv_program_exec(&program, target); - /* Don't message on error. Probably the register doesn't exist. */ - if (exec_out == ERROR_OK && target->reg_cache) { - struct reg *reg = &target->reg_cache->reg_list[number]; - buf_set_u64(reg->value, 0, reg->size, value); - } + if (riscv_program_exec(&program, target) != ERROR_OK) + return ERROR_FAIL; - if (use_scratch) + return register_read_abstract(target, value, GDB_REGNO_S0) != ERROR_OK; +} + +/** + * This function is used to write a 64-bit value to a register by executing a + * program. + * The program loads a value from address located in S0 to a register. + * The caller should save S0. + */ +static int internal_register_write64_progbuf_scratch(struct target *target, + struct riscv_program *program, riscv_reg_t value) +{ + scratch_mem_t scratch; + + if (scratch_reserve(target, &scratch, program, 8) != ERROR_OK) + return ERROR_FAIL; + + if (register_write_abstract(target, GDB_REGNO_S0, scratch.hart_address) + != ERROR_OK) { scratch_release(target, &scratch); + return ERROR_FAIL; + } + if (scratch_write64(target, &scratch, value) != ERROR_OK) { + scratch_release(target, &scratch); + return ERROR_FAIL; + } + int result = riscv_program_exec(program, target); + + scratch_release(target, &scratch); + return result; +} + +/** + * This function writes a register by writing a program to program buffer and + * executing it. + */ +static int register_write_progbuf(struct target *target, enum gdb_regno number, + riscv_reg_t value) +{ + assert(target->state == TARGET_HALTED); + + struct riscv_program program; + + riscv_program_init(&program, target); + + if (riscv_save_register(target, GDB_REGNO_S0) != ERROR_OK) + return ERROR_FAIL; + + if (number >= GDB_REGNO_FPR0 && number <= GDB_REGNO_FPR31 && + riscv_supports_extension(target, 'D') && riscv_xlen(target) < 64) { + /* There are no instructions to move all the bits from a register, + * so we need to use some scratch RAM. + */ + const unsigned int freg = number - GDB_REGNO_FPR0; + + if (riscv_program_insert(&program, fld(freg, S0, 0)) != ERROR_OK) + return ERROR_FAIL; + return internal_register_write64_progbuf_scratch(target, &program, + value); + } + + if (register_write_abstract(target, GDB_REGNO_S0, value) != ERROR_OK) + return ERROR_FAIL; + + if (number >= GDB_REGNO_FPR0 && number <= GDB_REGNO_FPR31) { + const unsigned int freg = number - GDB_REGNO_FPR0; + + if (riscv_program_insert(&program, + riscv_supports_extension(target, 'D') ? + fmv_d_x(freg, S0) : fmv_w_x(freg, S0)) != ERROR_OK) + return ERROR_FAIL; + } else if (number == GDB_REGNO_VTYPE) { + if (riscv_save_register(target, GDB_REGNO_S1) != ERROR_OK) + return ERROR_FAIL; + if (riscv_program_insert(&program, csrr(S1, CSR_VL)) != ERROR_OK) + return ERROR_FAIL; + if (riscv_program_insert(&program, vsetvl(ZERO, S1, S0)) != ERROR_OK) + return ERROR_FAIL; + } else if (number == GDB_REGNO_VL) { + /* "The XLEN-bit-wide read-only vl CSR can only be updated by the + * vsetvli and vsetvl instructions, and the fault-only-rst vector + * load instruction variants." + */ + if (riscv_save_register(target, GDB_REGNO_S1) != ERROR_OK) + return ERROR_FAIL; + if (riscv_program_insert(&program, csrr(S1, CSR_VTYPE)) != ERROR_OK) + return ERROR_FAIL; + if (riscv_program_insert(&program, vsetvl(ZERO, S0, S1)) != ERROR_OK) + return ERROR_FAIL; + } else if (number >= GDB_REGNO_CSR0 && number <= GDB_REGNO_CSR4095) { + if (riscv_program_csrw(&program, S0, number) != ERROR_OK) + return ERROR_FAIL; + } else { + LOG_ERROR("Unsupported register (enum gdb_regno)(%d)", number); + return ERROR_FAIL; + } + return riscv_program_exec(&program, target); +} + +/** + * Immediately write the new value to the requested register. This mechanism + * bypasses any caches. + */ +static int register_write_direct(struct target *target, enum gdb_regno number, + riscv_reg_t value) +{ + LOG_TARGET_DEBUG(target, "Writing 0x%" PRIx64 " to %s", value, + gdb_regno_name(number)); + + riscv_reg_t mstatus; + if (prep_for_register_access(target, &mstatus, number) != ERROR_OK) + return ERROR_FAIL; + + int result = register_write_abstract(target, number, value); + + if (result != ERROR_OK && target->state == TARGET_HALTED) + result = register_write_progbuf(target, number, value); if (cleanup_after_register_access(target, mstatus, number) != ERROR_OK) return ERROR_FAIL; - return exec_out; + if (result == ERROR_OK) + LOG_TARGET_DEBUG(target, "%s <- 0x%" PRIx64, gdb_regno_name(number), + value); + + return result; } /** Actually read registers from the target right now. */ static int register_read_direct(struct target *target, riscv_reg_t *value, enum gdb_regno number) { - if (dm013_select_target(target) != ERROR_OK) - return ERROR_FAIL; + LOG_TARGET_DEBUG(target, "Reading %s", gdb_regno_name(number)); riscv_reg_t mstatus; + if (prep_for_register_access(target, &mstatus, number) != ERROR_OK) return ERROR_FAIL; - int result = register_read_abstract(target, value, number, - register_size(target, number)); + int result = register_read_abstract(target, value, number); - if (result != ERROR_OK && - has_sufficient_progbuf(target, 2) && - number > GDB_REGNO_XPR31) { - struct riscv_program program; - riscv_program_init(&program, target); - - scratch_mem_t scratch; - bool use_scratch = false; - - if (riscv_save_register(target, GDB_REGNO_S0) != ERROR_OK) - return ERROR_FAIL; - - /* Write program to move data into s0. */ - - if (number >= GDB_REGNO_FPR0 && number <= GDB_REGNO_FPR31) { - if (riscv_supports_extension(target, 'D') - && riscv_xlen(target) < 64) { - /* There are no instructions to move all the bits from a - * register, so we need to use some scratch RAM. */ - if (riscv_program_insert(&program, - fsd(number - GDB_REGNO_FPR0, S0, 0)) != ERROR_OK) - return ERROR_FAIL; - if (scratch_reserve(target, &scratch, &program, 8) != ERROR_OK) - return ERROR_FAIL; - use_scratch = true; - - if (register_write_direct(target, GDB_REGNO_S0, - scratch.hart_address) != ERROR_OK) { - scratch_release(target, &scratch); - return ERROR_FAIL; - } - } else if (riscv_supports_extension(target, 'D')) { - if (riscv_program_insert(&program, fmv_x_d(S0, number - GDB_REGNO_FPR0)) != - ERROR_OK) - return ERROR_FAIL; - } else { - if (riscv_program_insert(&program, fmv_x_w(S0, number - GDB_REGNO_FPR0)) != - ERROR_OK) - return ERROR_FAIL; - } - } else if (number >= GDB_REGNO_CSR0 && number <= GDB_REGNO_CSR4095) { - if (riscv_program_csrr(&program, S0, number) != ERROR_OK) - return ERROR_FAIL; - } else { - LOG_ERROR("Unsupported register: %s", gdb_regno_name(number)); - return ERROR_FAIL; - } - - /* Execute program. */ - result = riscv_program_exec(&program, target); - /* Don't message on error. Probably the register doesn't exist. */ - - if (use_scratch) { - result = scratch_read64(target, &scratch, value); - scratch_release(target, &scratch); - if (result != ERROR_OK) - return result; - } else { - /* Read S0 */ - if (register_read_direct(target, value, GDB_REGNO_S0) != ERROR_OK) - return ERROR_FAIL; - } - } + if (result != ERROR_OK && target->state == TARGET_HALTED) + result = register_read_progbuf(target, value, number); if (cleanup_after_register_access(target, mstatus, number) != ERROR_OK) return ERROR_FAIL; if (result == ERROR_OK) - LOG_TARGET_DEBUG(target, "%s = 0x%" PRIx64, gdb_regno_name(number), *value); + LOG_TARGET_DEBUG(target, "%s = 0x%" PRIx64, gdb_regno_name(number), + *value); return result; } @@ -1710,13 +1754,29 @@ static int examine(struct target *target) info->index); 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; } /* Without knowing anything else we can at least mess with the * program buffer. */ r->debug_buffer_size = info->progbufsize; - int result = register_read_abstract(target, NULL, GDB_REGNO_S0, 64); + int result = register_read_abstract_with_size(target, NULL, GDB_REGNO_S0, 64); if (result == ERROR_OK) r->xlen = 64; else @@ -1725,11 +1785,11 @@ static int examine(struct target *target) /* Save s0 and s1. The register cache hasn't be initialized yet so we * need to take care of this manually. */ uint64_t s0, s1; - if (register_read_direct(target, &s0, GDB_REGNO_S0) != ERROR_OK) { + if (register_read_abstract(target, &s0, GDB_REGNO_S0) != ERROR_OK) { LOG_TARGET_ERROR(target, "Fatal: Failed to read s0."); return ERROR_FAIL; } - if (register_read_direct(target, &s1, GDB_REGNO_S1) != ERROR_OK) { + if (register_read_abstract(target, &s1, GDB_REGNO_S1) != ERROR_OK) { LOG_TARGET_ERROR(target, "Fatal: Failed to read s1."); return ERROR_FAIL; } @@ -1781,6 +1841,9 @@ static int examine(struct target *target) return ERROR_FAIL; } + target->state = saved_tgt_state; + target->debug_reason = saved_dbg_reason; + if (!halted) { riscv013_step_or_resume_current_hart(target, false); target->state = TARGET_RUNNING; @@ -2019,14 +2082,15 @@ static int riscv013_get_register_buf(struct target *target, riscv_reg_t mstatus, vtype, vl; unsigned int debug_vl, debug_vsew; + if (prep_for_vector_access(target, &mstatus, &vtype, &vl, &debug_vl, &debug_vsew) != ERROR_OK) return ERROR_FAIL; - unsigned vnum = regno - GDB_REGNO_V0; + unsigned int vnum = regno - GDB_REGNO_V0; int result = ERROR_OK; - for (unsigned i = 0; i < debug_vl; i++) { + for (unsigned int i = 0; i < debug_vl; i++) { /* Can't reuse the same program because riscv_program_exec() adds * ebreak to the end every time. */ struct riscv_program program; @@ -2073,17 +2137,18 @@ static int riscv013_set_register_buf(struct target *target, riscv_reg_t mstatus, vtype, vl; unsigned int debug_vl, debug_vsew; + if (prep_for_vector_access(target, &mstatus, &vtype, &vl, &debug_vl, &debug_vsew) != ERROR_OK) return ERROR_FAIL; - unsigned vnum = regno - GDB_REGNO_V0; + unsigned int vnum = regno - GDB_REGNO_V0; struct riscv_program program; riscv_program_init(&program, target); riscv_program_insert(&program, vslide1down_vx(vnum, vnum, S0, true)); int result = ERROR_OK; - for (unsigned i = 0; i < debug_vl; i++) { + for (unsigned int i = 0; i < debug_vl; i++) { if (register_write_direct(target, GDB_REGNO_S0, buf_get_u64(value, debug_vsew * i, debug_vsew)) != ERROR_OK) return ERROR_FAIL; @@ -4164,8 +4229,7 @@ struct target_type riscv013_target = { static int riscv013_get_register(struct target *target, riscv_reg_t *value, enum gdb_regno rid) { - LOG_DEBUG("[%s] reading register %s", target_name(target), - gdb_regno_name(rid)); + LOG_TARGET_DEBUG(target, "reading register %s", gdb_regno_name(rid)); if (dm013_select_target(target) != ERROR_OK) return ERROR_FAIL; @@ -4178,7 +4242,8 @@ static int riscv013_get_register(struct target *target, } else if (rid == GDB_REGNO_PRIV) { uint64_t dcsr; /* TODO: move this into riscv.c. */ - result = register_read_direct(target, &dcsr, GDB_REGNO_DCSR); + if (register_read_direct(target, &dcsr, GDB_REGNO_DCSR) != ERROR_OK) + return ERROR_FAIL; *value = set_field(0, VIRT_PRIV_V, get_field(dcsr, CSR_DCSR_V)); *value = set_field(*value, VIRT_PRIV_PRV, get_field(dcsr, CSR_DCSR_PRV)); } else { @@ -4193,27 +4258,22 @@ static int riscv013_get_register(struct target *target, static int riscv013_set_register(struct target *target, enum gdb_regno rid, riscv_reg_t value) { - if (dm013_select_target(target) != ERROR_OK) - return ERROR_FAIL; LOG_TARGET_DEBUG(target, "writing 0x%" PRIx64 " to register %s", value, gdb_regno_name(rid)); + if (dm013_select_target(target) != ERROR_OK) + return ERROR_FAIL; + if (rid <= GDB_REGNO_XPR31) { return register_write_direct(target, rid, value); } else if (rid == GDB_REGNO_PC) { LOG_TARGET_DEBUG(target, "writing PC to DPC: 0x%" PRIx64, value); - register_write_direct(target, GDB_REGNO_DPC, value); - riscv_reg_t actual_value; - register_read_direct(target, &actual_value, GDB_REGNO_DPC); - LOG_TARGET_DEBUG(target, " actual DPC written: 0x%016" PRIx64, actual_value); - if (value != actual_value) { - LOG_ERROR("Written PC (0x%" PRIx64 ") does not match read back " - "value (0x%" PRIx64 ")", value, actual_value); - return ERROR_FAIL; - } + return register_write_direct(target, GDB_REGNO_DPC, value); } else if (rid == GDB_REGNO_PRIV) { riscv_reg_t dcsr; - register_read_direct(target, &dcsr, GDB_REGNO_DCSR); + + if (register_read_direct(target, &dcsr, GDB_REGNO_DCSR) != ERROR_OK) + return ERROR_FAIL; dcsr = set_field(dcsr, CSR_DCSR_PRV, get_field(value, VIRT_PRIV_PRV)); dcsr = set_field(dcsr, CSR_DCSR_V, get_field(value, VIRT_PRIV_V)); return register_write_direct(target, GDB_REGNO_DCSR, dcsr); @@ -4544,7 +4604,7 @@ static int riscv013_on_step_or_resume(struct target *target, bool step) /* We want to twiddle some bits in the debug CSR so debugging works. */ riscv_reg_t dcsr; - int result = register_read_direct(target, &dcsr, GDB_REGNO_DCSR); + int result = riscv_get_register(target, &dcsr, GDB_REGNO_DCSR); if (result != ERROR_OK) return result; dcsr = set_field(dcsr, CSR_DCSR_STEP, step); diff --git a/src/target/riscv/riscv.c b/src/target/riscv/riscv.c index b7b742a3f..db2620d20 100644 --- a/src/target/riscv/riscv.c +++ b/src/target/riscv/riscv.c @@ -1533,8 +1533,10 @@ int riscv_flush_registers(struct target *target) return ERROR_OK; } -/* Convert: RISC-V hart's halt reason --> OpenOCD's generic debug reason */ -static int set_debug_reason(struct target *target, enum riscv_halt_reason halt_reason) +/** + * Set OpenOCD's generic debug reason from the RISC-V halt reason. + */ +int set_debug_reason(struct target *target, enum riscv_halt_reason halt_reason) { RISCV_INFO(r); r->trigger_hit = -1;