From 02e2daef7ba8de0c70ebd69bcca2218118d18607 Mon Sep 17 00:00:00 2001 From: Evgeniy Naydanov Date: Tue, 10 Sep 2024 14:37:23 +0300 Subject: [PATCH] target/riscv: access registers via `reg->type` * `int riscv_reg_get()` and `int riscv_reg_set()` are implemented in terms of `reg->type->get/set` instead of the other way around. This makes it easier to support custom behavior for some registers. * Cacheability is determined by `reg->type` instead of `riscv_reg_impl_gdb_regno_cacheable()`. * Issues with redirection of `priv` -> `dcsr` and `pc` -> `dpc` are addressed at the "topmost" level. - `priv` and `pc` are alvais invalid. - Fixed some issues, e.g. the first `pc` write printed-out an uninitialized value: ``` > reg pc 0 pc (/64): 0x000075da6b33db20 ``` Change-Id: I514547f455d62b289fb5dee62753bf5d9aa3b8ae Signed-off-by: Evgeniy Naydanov --- src/target/riscv/riscv-013.c | 17 +- src/target/riscv/riscv-013_reg.c | 501 ++++++++++++++++++++++++++---- src/target/riscv/riscv-013_reg.h | 9 +- src/target/riscv/riscv_reg.c | 183 ++--------- src/target/riscv/riscv_reg_impl.h | 50 --- 5 files changed, 471 insertions(+), 289 deletions(-) diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c index c399de7e4..1780e1f6b 100644 --- a/src/target/riscv/riscv-013.c +++ b/src/target/riscv/riscv-013.c @@ -5064,19 +5064,8 @@ struct target_type riscv013_target = { int riscv013_get_register(struct target *target, riscv_reg_t *value, enum gdb_regno rid) { - /* It would be beneficial to move this redirection to the - * version-independent section, but there is a conflict: - * `dcsr[5]` is `dcsr.v` in current spec, but it is `dcsr.debugint` in 0.11. - */ - if (rid == GDB_REGNO_PRIV) { - uint64_t dcsr; - if (riscv_reg_get(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)); - return ERROR_OK; - } - + assert(rid != GDB_REGNO_PC && "'pc' should be read through 'dpc'" ); + assert(rid != GDB_REGNO_PRIV && "'priv' should be read through 'dcsr'" ); LOG_TARGET_DEBUG(target, "reading register %s", riscv_reg_gdb_regno_name(target, rid)); if (dm013_select_target(target) != ERROR_OK) @@ -5093,6 +5082,8 @@ int riscv013_get_register(struct target *target, int riscv013_set_register(struct target *target, enum gdb_regno rid, riscv_reg_t value) { + assert(rid != GDB_REGNO_PC && "'pc' should be written through 'dpc'" ); + assert(rid != GDB_REGNO_PRIV && "'priv' should be written through 'dcsr'" ); LOG_TARGET_DEBUG(target, "writing 0x%" PRIx64 " to register %s", value, riscv_reg_gdb_regno_name(target, rid)); diff --git a/src/target/riscv/riscv-013_reg.c b/src/target/riscv/riscv-013_reg.c index f1fea7401..999b3303e 100644 --- a/src/target/riscv/riscv-013_reg.c +++ b/src/target/riscv/riscv-013_reg.c @@ -13,74 +13,454 @@ #include "debug_defines.h" #include -static int riscv013_reg_get(struct reg *reg) +/* Start of register fetch/send definitions. + * - "reg_fetcher" (named "fetch_*") -- loads the value from target into reg + * cache entry. Modifyes only "reg->value". + * - "reg_sender" (named "send_*") -- stores the value from "buf" to the + * target. Does not modify "reg" at all. + */ +typedef int (*reg_fetcher)(struct reg *reg); +typedef int (*reg_sender)(const struct reg *reg, const uint8_t *buf); + +/* TODO: Introduce separate "fetch_*" and "send_*" functions for register + * classes (xregs, fregs, csrs). + */ +static int fetch_riscv013_reg(struct reg *reg) { - struct target *target = riscv_reg_impl_get_target(reg); + riscv_reg_t value; + int res = riscv013_get_register(riscv_reg_impl_get_target(reg), + &value, reg->number); + if (res != ERROR_OK) + return res; + buf_set_u64(reg->value, 0, reg->size, value); + return res; +} - /* 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; - } +static int send_riscv013_reg(const struct reg *reg, const uint8_t *buf) +{ + assert(reg->size <= 64); + const riscv_reg_t value = buf_get_u64(buf, 0, reg->size); + return riscv013_set_register(riscv_reg_impl_get_target(reg), + reg->number, value); +} - if (reg->number >= GDB_REGNO_V0 && reg->number <= GDB_REGNO_V31) { - if (riscv013_get_register_buf(target, reg->value, reg->number) != ERROR_OK) - return ERROR_FAIL; +/* TODO: Inline "riscv013_*_register_buf" into "vreg" accessors. + */ +static int fetch_vreg(struct reg *reg) +{ + return riscv013_get_register_buf(riscv_reg_impl_get_target(reg), + reg->value, reg->number); +} - reg->valid = riscv_reg_impl_gdb_regno_cacheable(reg->number, /* is write? */ false); - } else { - uint64_t value; - int result = riscv_reg_get(target, &value, reg->number); - if (result != ERROR_OK) - return result; - buf_set_u64(reg->value, 0, reg->size, value); +static int send_vreg(const struct reg *reg, const uint8_t *buf) +{ + return riscv013_set_register_buf(riscv_reg_impl_get_target(reg), + reg->number, reg->value); +} + +/* End of register fetch/send definitions. */ +/* Start of cache entry utils. */ + +static void set_cache_value(struct reg *reg, const uint8_t *buf) +{ + assert(reg); + assert(reg->value); + assert(reg->size); + buf_cpy(buf, reg->value, reg->size); +} + +/** + * Note: there are only 3 states a cahce entry can be in: + * |: state :|: "reg->valid" :|: "reg->dirty" :| + * | "invalid" | false | false | + * | "valid" | true | false | + * | "dirty" | true | true | + * + * The state "reg->valid == dirty && reg->valid == false" should be unreacheable. + * To achieve this, "reg->valid" and "reg->dirty" are not assigned directly + * outside of "mark_()" functions. + */ +static void mark_invalid(struct reg *reg) +{ + assert(riscv_reg_impl_is_initialized(reg)); + assert(reg->exist); + reg->valid = false; + reg->dirty = false; +} + +static void mark_clean(struct reg *reg) +{ + assert(riscv_reg_impl_is_initialized(reg)); + assert(reg->exist); + assert(riscv_reg_impl_get_target(reg)->state == TARGET_HALTED); + reg->valid = true; + reg->dirty = false; +} + +static void mark_dirty(struct reg *reg) +{ + assert(riscv_reg_impl_is_initialized(reg)); + assert(reg->exist); + assert(riscv_reg_impl_get_target(reg)->state == TARGET_HALTED); + reg->valid = true; + reg->dirty = true; +} + +static int access_prelude(const struct reg *reg, bool write) +{ + assert(riscv_reg_impl_is_initialized(reg)); + const struct target * const target = riscv_reg_impl_get_target(reg); + if (!reg->exist) { + LOG_TARGET_DEBUG(target, "Register %s does not exist.", + reg->name); + return ERROR_FAIL; } - char *str = buf_to_hex_str(reg->value, reg->size); - LOG_TARGET_DEBUG(target, "Read 0x%s from %s (valid=%d).", str, reg->name, - reg->valid); - free(str); + LOG_TARGET_DEBUG(target, "%s %s (valid=%s, dirty=%s).", + write ? "Writing" : "Reading", reg->name, + reg->valid ? "true" : "false", + reg->dirty ? "true" : "false"); return ERROR_OK; } -static int riscv013_reg_set(struct reg *reg, uint8_t *buf) +static void access_epilogue(const struct reg *reg, bool write) { - struct target *target = riscv_reg_impl_get_target(reg); - - char *str = buf_to_hex_str(buf, reg->size); - LOG_TARGET_DEBUG(target, "Write 0x%s to %s (valid=%d).", str, reg->name, - reg->valid); + assert(riscv_reg_impl_is_initialized(reg)); + assert(reg->exist); + if (debug_level < LOG_LVL_DEBUG) + return; + char *str = buf_to_hex_str(reg->value, reg->size); + LOG_TARGET_DEBUG(riscv_reg_impl_get_target(reg), + "%s 0x%s %s %s (valid=%s, dirty=%s).", + write ? "Wrote" : "Read", str, + write ? "to" : "from", reg->name, + reg->valid ? "true" : "false", + reg->dirty ? "true" : "false"); free(str); +} - /* 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) +static int noncaching_get(struct reg *reg, reg_fetcher fetch_reg) +{ + assert(riscv_reg_impl_is_initialized(reg)); + int res = access_prelude(reg, /*write*/ false); + if (res != ERROR_OK) + return res; + res = fetch_reg(reg); + mark_invalid(reg); + if (res != ERROR_OK) + return res; + access_epilogue(reg, /*write*/ false); + return ERROR_OK; +} + +static int caching_get(struct reg *reg, reg_fetcher fetch_reg) +{ + assert(riscv_reg_impl_is_initialized(reg)); + int res = access_prelude(reg, /*write*/ false); + if (res != ERROR_OK) + return res; + if (reg->valid) { + LOG_TARGET_DEBUG(riscv_reg_impl_get_target(reg), + "Reading %s from cache.", reg->name); return ERROR_OK; + } + res = fetch_reg(reg); + if (res != ERROR_OK) { + mark_invalid(reg); + return res; + } + if (riscv_reg_impl_get_target(reg)->state == TARGET_HALTED) + mark_clean(reg); + else + mark_invalid(reg); + access_epilogue(reg, /*write*/ false); + return ERROR_OK; +} - if (reg->number >= GDB_REGNO_V0 && reg->number <= GDB_REGNO_V31) { - if (riscv013_set_register_buf(target, reg->number, buf) != ERROR_OK) - return ERROR_FAIL; +static int noncaching_set(struct reg *reg, const uint8_t *buf, + reg_sender send_reg) +{ + int res = access_prelude(reg, /*write*/ true); + if (res != ERROR_OK) + return res; + res = send_reg(reg, buf); + mark_invalid(reg); + if (res != ERROR_OK) + return res; + set_cache_value(reg, buf); + access_epilogue(reg, /*write*/ true); + return ERROR_OK; +} - memcpy(reg->value, buf, DIV_ROUND_UP(reg->size, 8)); - reg->valid = riscv_reg_impl_gdb_regno_cacheable(reg->number, /* is write? */ true); +static int caching_set(struct reg *reg, const uint8_t *buf, + reg_sender send_reg) +{ + assert(riscv_reg_impl_is_initialized(reg)); + const struct target * const target = riscv_reg_impl_get_target(reg); + if (target->state != TARGET_HALTED) { + LOG_TARGET_DEBUG(target, "Not caching the write to %s.", reg->name); + return noncaching_set(reg, buf, send_reg); + } + int res = access_prelude(reg, /*write*/ true); + if (res != ERROR_OK) + return res; + if (reg->valid && buf_eq(reg->value, buf, reg->size)) { + LOG_TARGET_DEBUG(riscv_reg_impl_get_target(reg), + "Writing the same value to %s.", reg->name); } else { - const riscv_reg_t value = buf_get_u64(buf, 0, reg->size); - if (riscv_reg_set(target, reg->number, value) != ERROR_OK) - return ERROR_FAIL; + LOG_TARGET_DEBUG(target, "Caching the write to %s.", reg->name); + set_cache_value(reg, buf); + mark_dirty(reg); } + access_epilogue(reg, /*write*/ true); + return ERROR_OK; +} +static int flush_reg(struct reg *reg, reg_sender send_reg) +{ + if (!reg->dirty) + return ERROR_OK; + int res = access_prelude(reg, /*write*/ true); + if (res != ERROR_OK) + return res; + assert(reg->dirty); + assert(reg->valid); + const struct target * const target = riscv_reg_impl_get_target(reg); + if (target->state != TARGET_HALTED) { + LOG_TARGET_ERROR(target, + "BUG: register %s is dirty while the target is not halted.", + reg->name); + return ERROR_TARGET_NOT_HALTED; + } + res = send_reg(reg, reg->value); + if (res != ERROR_OK) { + /* Register is not marked "invalid" here (like it's done on failure in + * "caching_set") since it is "dirty", i.e. the most recent value + * is currently in the cache. */ + return res; + } + mark_clean(reg); + access_epilogue(reg, /*write*/ true); + return res; +} + +/* End of cache entry utils. */ +/* Start of "reg_arch_type" method definitions. */ + +static int riscv013_noncaching_get(struct reg *reg) +{ + return noncaching_get(reg, fetch_riscv013_reg); +} + +static int riscv013_caching_get(struct reg *reg) +{ + return caching_get(reg, fetch_riscv013_reg); +} + +static int riscv013_noncaching_set(struct reg *reg, uint8_t *buf) +{ + return noncaching_set(reg, buf, send_riscv013_reg); +} + +static int riscv013_caching_set(struct reg *reg, uint8_t *buf) +{ + return caching_set(reg, buf, send_riscv013_reg); +} + +static int riscv013_reg_flush(struct reg *reg) +{ + return flush_reg(reg, send_riscv013_reg); +} + +static int flush_uncacheable(struct reg *reg) +{ + assert(!reg->dirty); + return ERROR_OK; +} + +static int get_vreg(struct reg *reg) +{ + assert(reg->number >= GDB_REGNO_V0 && reg->number <= GDB_REGNO_V31); + return caching_get(reg, fetch_vreg); +} + +static int set_vreg(struct reg *reg, uint8_t *buf) +{ + assert(reg->number >= GDB_REGNO_V0 && reg->number <= GDB_REGNO_V31); + return caching_set(reg, buf, send_vreg); +} + +static int flush_vreg(struct reg *reg) +{ + assert(reg->number >= GDB_REGNO_V0 && reg->number <= GDB_REGNO_V31); + return flush_reg(reg, send_vreg); +} + +static int get_pc(struct reg *pc) +{ + assert(pc->number == GDB_REGNO_PC); + assert(!pc->valid); + assert(!pc->dirty); + struct reg * const dpc = + riscv_reg_impl_cache_entry(riscv_reg_impl_get_target(pc), + GDB_REGNO_DPC); + int res = dpc->type->get(dpc); + if (res == ERROR_OK) + set_cache_value(pc, dpc->value); + return res; +} + +static int set_pc(struct reg *pc, uint8_t *buf) +{ + assert(pc->number == GDB_REGNO_PC); + assert(!pc->valid); + assert(!pc->dirty); + struct reg * const dpc = + riscv_reg_impl_cache_entry(riscv_reg_impl_get_target(pc), + GDB_REGNO_DPC); + int res = dpc->type->set(dpc, buf); + if (res == ERROR_OK) + set_cache_value(pc, dpc->value); + return res; +} + +static int get_priv(struct reg *priv) +{ + assert(priv->number == GDB_REGNO_PRIV); + assert(!priv->valid); + assert(!priv->dirty); + struct reg * const dcsr = + riscv_reg_impl_cache_entry(riscv_reg_impl_get_target(priv), + GDB_REGNO_DCSR); + assert(dcsr->size == 32); + int res = dcsr->type->get(dcsr); + if (res != ERROR_OK) + return res; + buf_set_u32(priv->value, VIRT_PRIV_PRV_OFFSET, VIRT_PRIV_PRV_LENGTH, + buf_get_u32(dcsr->value, CSR_DCSR_PRV_OFFSET, CSR_DCSR_PRV_LENGTH)); + buf_set_u32(priv->value, VIRT_PRIV_V_OFFSET, VIRT_PRIV_V_LENGTH, + buf_get_u32(dcsr->value, CSR_DCSR_V_OFFSET, CSR_DCSR_V_LENGTH)); return ERROR_OK; } +static int set_priv(struct reg *priv, uint8_t *priv_buf) +{ + assert(priv->number == GDB_REGNO_PRIV); + assert(!priv->valid); + assert(!priv->dirty); + struct reg * const dcsr = riscv_reg_impl_cache_entry(riscv_reg_impl_get_target(priv), + GDB_REGNO_DCSR); + int res = dcsr->type->get(dcsr); + if (res != ERROR_OK) + return res; + assert(dcsr->size == 32); + uint8_t dcsr_buf[32 / 8]; + buf_cpy(dcsr->value, dcsr_buf, dcsr->size); + buf_set_u32(dcsr_buf, CSR_DCSR_PRV_OFFSET, CSR_DCSR_PRV_LENGTH, + buf_get_u32(priv_buf, VIRT_PRIV_PRV_OFFSET, VIRT_PRIV_PRV_LENGTH)); + buf_set_u32(dcsr_buf, CSR_DCSR_V_OFFSET, CSR_DCSR_V_LENGTH, + buf_get_u32(priv_buf, VIRT_PRIV_V_OFFSET, VIRT_PRIV_V_LENGTH)); + res = dcsr->type->set(dcsr, dcsr_buf); + if (res == ERROR_OK) + set_cache_value(priv, priv_buf); + return res; +} + +/* End of "reg_arch_type" method definitions. */ + +static const struct reg_arch_type zero_type = { + .get = riscv013_caching_get, + /* "zero" is read-only, but there is nothig wrong with attempting to + * write it (e.g. for the purpose of HW testing). */ + .set = riscv013_noncaching_set, + .flush = riscv013_reg_flush, +}; + +static const struct reg_arch_type xreg_type = { + .get = riscv013_caching_get, + .set = riscv013_caching_set, + .flush = riscv013_reg_flush, +}; + +static const struct reg_arch_type freg_type = { + .get = riscv013_caching_get, + .set = riscv013_caching_set, + .flush = riscv013_reg_flush, +}; + +static const struct reg_arch_type vreg_type = { + .get = get_vreg, + .set = set_vreg, + .flush = flush_vreg +}; + +static const struct reg_arch_type pc_type = { + .get = get_pc, + .set = set_pc, + .flush = flush_uncacheable +}; + +static const struct reg_arch_type priv_type = { + .get = get_priv, + .set = set_priv, + .flush = flush_uncacheable +}; + +static const struct reg_arch_type warl_csr_type = { + .get = riscv013_caching_get, + .set = riscv013_noncaching_set, + .flush = riscv013_reg_flush +}; + +static const struct reg_arch_type uncacheable_type = { + .get = riscv013_noncaching_get, + .set = riscv013_noncaching_set, + .flush = flush_uncacheable +}; + static const struct reg_arch_type *riscv013_gdb_regno_reg_type(uint32_t regno) { - static const struct reg_arch_type riscv013_reg_type = { - .get = riscv013_reg_get, - .set = riscv013_reg_set, - .flush = NULL - }; - return &riscv013_reg_type; + if (regno != GDB_REGNO_ZERO && regno <= GDB_REGNO_XPR31) { + return &xreg_type; + } + if (regno >= GDB_REGNO_FPR0 && regno <= GDB_REGNO_FPR31) { + /* For now "freg_type" is the same as "xreg_type", but it will be + * changed soon */ + return &freg_type; + } + if (regno >= GDB_REGNO_V0 && regno <= GDB_REGNO_V31) { + return &vreg_type; + } + switch (regno) { + case GDB_REGNO_ZERO: + return &zero_type; + case GDB_REGNO_PC: + return &pc_type; + case GDB_REGNO_PRIV: + return &priv_type; + case GDB_REGNO_VLENB: + /* "vlenb" is read-only, but there is nothig wrong with attempting to + * write it (e.g. for the purpose of HW testing). */ + case GDB_REGNO_DPC: + case GDB_REGNO_VSTART: + case GDB_REGNO_VXSAT: + case GDB_REGNO_VXRM: + case GDB_REGNO_VL: + case GDB_REGNO_VTYPE: + case GDB_REGNO_MISA: + case GDB_REGNO_DCSR: + case GDB_REGNO_DSCRATCH0: + case GDB_REGNO_MSTATUS: + case GDB_REGNO_MEPC: + case GDB_REGNO_MCAUSE: + case GDB_REGNO_SATP: + return &warl_csr_type; + /* TODO: Sdtrig register should be WARL. */ + case GDB_REGNO_TSELECT: + case GDB_REGNO_TDATA1: + case GDB_REGNO_TDATA2: + default: + return &uncacheable_type; + } } static int init_cache_entry(struct target *target, uint32_t regno) @@ -359,30 +739,21 @@ int riscv013_reg_examine_all(struct target *target) */ int riscv013_reg_save(struct target *target, enum gdb_regno regid) { - if (target->state != TARGET_HALTED) { - LOG_TARGET_ERROR(target, "Can't save register %s on a hart that is not halted.", - riscv_reg_gdb_regno_name(target, regid)); - return ERROR_FAIL; - } - assert(riscv_reg_impl_gdb_regno_cacheable(regid, /* is write? */ false) && - "Only cacheable registers can be saved."); + assert(regid >= GDB_REGNO_ZERO && regid <= GDB_REGNO_XPR31); + struct reg *reg = riscv_reg_impl_cache_entry(target, regid); + assert(riscv_reg_impl_is_initialized(reg)); 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; + if (target->state != TARGET_HALTED) { + LOG_TARGET_ERROR(target, "Can't save register %s on a hart that is not halted.", + reg->name); + return ERROR_TARGET_NOT_HALTED; } - struct reg *reg = riscv_reg_impl_cache_entry(target, regid); - LOG_TARGET_DEBUG(target, "Saving %s", reg->name); - if (riscv_reg_get(target, &value, regid) != ERROR_OK) - return ERROR_FAIL; + int res = reg->type->get(reg); + if (res != ERROR_OK) + return res; assert(reg->valid && "The register is cacheable, so the cache entry must be valid now."); diff --git a/src/target/riscv/riscv-013_reg.h b/src/target/riscv/riscv-013_reg.h index e542a3501..4f0b80b9b 100644 --- a/src/target/riscv/riscv-013_reg.h +++ b/src/target/riscv/riscv-013_reg.h @@ -20,12 +20,9 @@ int riscv013_reg_examine_all(struct target *target); /** - * 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. - * Generally used to save registers before program buffer execution. - * - * TODO: The interface should be restricted in such a way that only GPRs can be - * saved. + * This function is used to save the value of a X-register in cache. The + * register is marked as dirty, and writeback is delayed for as long as + * possible. Generally used to save registers before program buffer execution. */ int riscv013_reg_save(struct target *target, enum gdb_regno regid); diff --git a/src/target/riscv/riscv_reg.c b/src/target/riscv/riscv_reg.c index 29d8a2b65..39d41b281 100644 --- a/src/target/riscv/riscv_reg.c +++ b/src/target/riscv/riscv_reg.c @@ -8,19 +8,6 @@ #include "riscv.h" #include "riscv_reg.h" #include "riscv_reg_impl.h" -/** - * TODO: Currently `reg->get/set` is implemented in terms of - * `riscv_get/set_register`. However, the intention behind - * `riscv_get/set_register` is to work with the cache, therefore it accesses - * and modifyes register cache directly. The idea is to implement - * `riscv_get/set_register` in terms of `riscv_reg_impl_cache_entry` and - * `reg->get/set`. - * Once this is done, the following includes should be removed. - */ -#include "debug_defines.h" -#include "riscv-011.h" -#include "riscv-013.h" -#include "field_helpers.h" static const char * const default_reg_names[GDB_REGNO_COUNT] = { [GDB_REGNO_ZERO] = "zero", @@ -795,97 +782,17 @@ int riscv_reg_flush_all(struct target *target) * may become dirty in the process (e.g. S0, S1). For that reason, flush * registers in reverse order, so that GPRs are flushed last. */ + int res = ERROR_OK; for (unsigned int number = target->reg_cache->num_regs; number-- > 0; ) { struct reg *reg = riscv_reg_impl_cache_entry(target, number); - if (reg->valid && reg->dirty) { - 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 (riscv_reg_write(target, number, value) != ERROR_OK) - return ERROR_FAIL; - } - } - LOG_TARGET_DEBUG(target, "Flush of register cache completed"); - return ERROR_OK; -} - -/** - * 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. - * TODO: Currently `reg->get/set` is implemented in terms of - * `riscv_get/set_register`. However, the intention behind - * `riscv_get/set_register` is to work with the cache, therefore it accesses - * and modifyes register cache directly. The idea is to implement - * `riscv_get/set_register` in terms of `riscv_reg_impl_cache_entry` and - * `reg->get/set`. - */ -static int riscv_set_or_write_register(struct target *target, - enum gdb_regno regid, riscv_reg_t value, bool write_through) -{ - RISCV_INFO(r); - assert(r); - if (r->dtm_version == DTM_DTMCS_VERSION_0_11) - return riscv011_set_register(target, regid, value); - - keep_alive(); - - if (regid == GDB_REGNO_PC) { - return riscv_set_or_write_register(target, GDB_REGNO_DPC, value, write_through); - } else if (regid == GDB_REGNO_PRIV) { - riscv_reg_t dcsr; - - if (riscv_reg_get(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 riscv_set_or_write_register(target, GDB_REGNO_DCSR, dcsr, write_through); - } - - struct reg *reg = riscv_reg_impl_cache_entry(target, regid); - assert(riscv_reg_impl_is_initialized(reg)); - - 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 riscv013_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 = riscv_reg_impl_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 (riscv013_set_register(target, regid, value) != ERROR_OK) - return ERROR_FAIL; - - reg->dirty = false; - } else { - reg->dirty = need_to_write; + if (reg->type->flush(reg) != ERROR_OK) + res = ERROR_FAIL; } - - 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; + if (res == ERROR_OK) + LOG_TARGET_DEBUG(target, "Flush of register cache completed successfully."); + else + LOG_TARGET_DEBUG(target, "Flush of register cache failed."); + return res; } bool riscv_reg_cache_any_dirty(const struct target *target, int log_level) @@ -919,83 +826,49 @@ void riscv_reg_cache_invalidate_all(struct target *target) /** * 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. - * TODO: Currently `reg->get/set` is implemented in terms of - * `riscv_get/set_register`. However, the intention behind - * `riscv_get/set_register` is to work with the cache, therefore it accesses - * and modifyes register cache directly. The idea is to implement - * `riscv_get/set_register` in terms of `riscv_reg_impl_cache_entry` and - * `reg->get/set`. */ int riscv_reg_set(struct target *target, enum gdb_regno regid, riscv_reg_t value) { - return riscv_set_or_write_register(target, regid, value, - /* write_through */ false); + struct reg * const reg = riscv_reg_impl_cache_entry(target, regid); + assert(riscv_reg_impl_is_initialized(reg)); + assert(reg->size <= sizeof(riscv_reg_t) * CHAR_BIT); + assert(reg->size <= 64); + uint8_t buf[64 / 8]; + buf_set_u64(buf, 0, reg->size, value); + return reg->type->set(reg, buf); } /** * 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. - * TODO: Currently `reg->get/set` is implemented in terms of - * `riscv_get/set_register`. However, the intention behind - * `riscv_get/set_register` is to work with the cache, therefore it accesses - * and modifyes register cache directly. The idea is to implement - * `riscv_get/set_register` in terms of `riscv_reg_impl_cache_entry` and - * `reg->get/set`. */ int riscv_reg_write(struct target *target, enum gdb_regno regid, riscv_reg_t value) { - return riscv_set_or_write_register(target, regid, value, - /* write_through */ true); + int res = riscv_reg_set(target, regid, value); + if (res != ERROR_OK) + return res; + struct reg * const reg = riscv_reg_impl_cache_entry(target, regid); + return reg->type->flush(reg); } /** * This function is used to get the value of a register. If possible, the value * in cache will be updated. - * TODO: Currently `reg->get/set` is implemented in terms of - * `riscv_get/set_register`. However, the intention behind - * `riscv_get/set_register` is to work with the cache, therefore it accesses - * and modifyes register cache directly. The idea is to implement - * `riscv_get/set_register` in terms of `riscv_reg_impl_cache_entry` and - * `reg->get/set`. */ int riscv_reg_get(struct target *target, riscv_reg_t *value, enum gdb_regno regid) { - RISCV_INFO(r); - assert(r); - if (r->dtm_version == DTM_DTMCS_VERSION_0_11) - return riscv011_get_register(target, value, regid); - - keep_alive(); + assert(value); - if (regid == GDB_REGNO_PC) - return riscv_reg_get(target, value, GDB_REGNO_DPC); - - struct reg *reg = riscv_reg_impl_cache_entry(target, regid); + struct reg * const reg = riscv_reg_impl_cache_entry(target, regid); assert(riscv_reg_impl_is_initialized(reg)); - if (!reg->exist) { - LOG_TARGET_DEBUG(target, "Register %s does not exist.", reg->name); - return ERROR_FAIL; - } - - if (reg->valid) { - *value = buf_get_u64(reg->value, 0, reg->size); - LOG_TARGET_DEBUG(target, "Read %s: 0x%" PRIx64 " (cached)", reg->name, - *value); - return ERROR_OK; - } - - LOG_TARGET_DEBUG(target, "Reading %s from target", reg->name); - if (riscv013_get_register(target, value, regid) != ERROR_OK) - return ERROR_FAIL; - - buf_set_u64(reg->value, 0, reg->size, *value); - reg->valid = riscv_reg_impl_gdb_regno_cacheable(regid, /* is write? */ false) && - target->state == TARGET_HALTED; - reg->dirty = false; - - LOG_TARGET_DEBUG(target, "Read %s: 0x%" PRIx64, reg->name, *value); + assert(reg->size <= sizeof(riscv_reg_t) * CHAR_BIT); + assert(reg->size <= 64); + int res = reg->type->get(reg); + if (res != ERROR_OK) + return res; + *value = buf_get_u64(reg->value, 0, reg->size); return ERROR_OK; } diff --git a/src/target/riscv/riscv_reg_impl.h b/src/target/riscv/riscv_reg_impl.h index 7a483adc2..133797fe1 100644 --- a/src/target/riscv/riscv_reg_impl.h +++ b/src/target/riscv/riscv_reg_impl.h @@ -171,54 +171,4 @@ int riscv_reg_impl_expose_csrs(const struct target *target); /** Hide additional CSRs, as specified by `riscv_info_t::hide_csr` list. */ void riscv_reg_impl_hide_csrs(const struct target *target); -/** - * If write is true: - * return true iff we are guaranteed that the register will contain exactly - * the value we just wrote when it's read. - * If write is false: - * return true iff we are guaranteed that the register will read the same - * value in the future as the value we just read. - */ -static inline bool riscv_reg_impl_gdb_regno_cacheable(enum gdb_regno regno, - bool is_write) -{ - if (regno == GDB_REGNO_ZERO) - return !is_write; - - /* GPRs, FPRs, vector registers are just normal data stores. */ - if (regno <= GDB_REGNO_XPR31 || - (regno >= GDB_REGNO_FPR0 && regno <= GDB_REGNO_FPR31) || - (regno >= GDB_REGNO_V0 && regno <= GDB_REGNO_V31)) - return true; - - /* Most CSRs won't change value on us, but we can't assume it about arbitrary - * CSRs. */ - switch (regno) { - case GDB_REGNO_DPC: - case GDB_REGNO_VSTART: - case GDB_REGNO_VXSAT: - case GDB_REGNO_VXRM: - case GDB_REGNO_VLENB: - case GDB_REGNO_VL: - case GDB_REGNO_VTYPE: - case GDB_REGNO_MISA: - case GDB_REGNO_DCSR: - case GDB_REGNO_DSCRATCH0: - case GDB_REGNO_MSTATUS: - case GDB_REGNO_MEPC: - case GDB_REGNO_MCAUSE: - case GDB_REGNO_SATP: - /* - * WARL registers might not contain the value we just wrote, but - * these ones won't spontaneously change their value either. * - */ - return !is_write; - - case GDB_REGNO_TSELECT: /* I think this should be above, but then it doesn't work. */ - case GDB_REGNO_TDATA1: /* Changes value when tselect is changed. */ - case GDB_REGNO_TDATA2: /* Changes value when tselect is changed. */ - default: - return false; - } -} #endif /* OPENOCD_TARGET_RISCV_RISCV_REG_IMPL_H */