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 <evgeniy.naydanov@syntacore.com>

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
+	assert(rid != GDB_REGNO_PC && "'pc' should be read through 'dpc'");
-	 * version-independent section, but there is a conflict:
+	assert(rid != GDB_REGNO_PRIV && "'priv' should be read through 'dcsr'");
-	 * `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;
-	}
-
 	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));
 

src/target/riscv/riscv-013_reg.c

@@ -13,74 +13,452 @@
 #include "debug_defines.h"
 #include <helper/time_support.h>
 
-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. */
+static int send_riscv013_reg(const struct reg *reg, const uint8_t *buf)
-	if (reg->number > GDB_REGNO_XPR15 && reg->number <= GDB_REGNO_XPR31 &&
+{
-			riscv_supports_extension(target, 'E')) {
+	assert(reg->size <= 64);
-		buf_set_u64(reg->value, 0, reg->size, 0);
+	const riscv_reg_t value = buf_get_u64(buf, 0, reg->size);
-		return ERROR_OK;
+	return riscv013_set_register(riscv_reg_impl_get_target(reg),
+			reg->number, value);
+}
+
+/* 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);
+}
+
+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_<state>()" 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;
 	}
+	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;
+}
 
-	if (reg->number >= GDB_REGNO_V0 && reg->number <= GDB_REGNO_V31) {
+static void access_epilogue(const struct reg *reg, bool write)
-		if (riscv013_get_register_buf(target, reg->value, reg->number) != ERROR_OK)
+{
-			return ERROR_FAIL;
+	assert(riscv_reg_impl_is_initialized(reg));
-
+	assert(reg->exist);
-		reg->valid = riscv_reg_impl_gdb_regno_cacheable(reg->number, /* is write? */ false);
+	if (debug_level < LOG_LVL_DEBUG)
-	} else {
+		return;
-		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);
-	}
 	char *str = buf_to_hex_str(reg->value, reg->size);
-	LOG_TARGET_DEBUG(target, "Read 0x%s from %s (valid=%d).", str, reg->name,
+	LOG_TARGET_DEBUG(riscv_reg_impl_get_target(reg),
-			reg->valid);
+			"%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);
-	return ERROR_OK;
 }
 
-static int riscv013_reg_set(struct reg *reg, uint8_t *buf)
+static int noncaching_get(struct reg *reg, reg_fetcher fetch_reg)
 {
-	struct target *target = riscv_reg_impl_get_target(reg);
+	assert(riscv_reg_impl_is_initialized(reg));
-
+	int res = access_prelude(reg, /*write*/ false);
-	char *str = buf_to_hex_str(buf, reg->size);
+	if (res != ERROR_OK)
-	LOG_TARGET_DEBUG(target, "Write 0x%s to %s (valid=%d).", str, reg->name,
+		return res;
-			reg->valid);
+	res = fetch_reg(reg);
-	free(str);
+	mark_invalid(reg);
-
+	if (res != ERROR_OK)
-	/* TODO: Hack to deal with gdb that thinks these registers still exist. */
+		return res;
-	if (reg->number > GDB_REGNO_XPR15 && reg->number <= GDB_REGNO_XPR31 &&
+	access_epilogue(reg, /*write*/ false);
-			riscv_supports_extension(target, 'E') &&
-			buf_get_u64(buf, 0, reg->size) == 0)
-		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;
-
-		memcpy(reg->value, buf, DIV_ROUND_UP(reg->size, 8));
-		reg->valid = riscv_reg_impl_gdb_regno_cacheable(reg->number, /* is write? */ true);
-	} 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;
-	}
-
 	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;
+}
+
+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;
+}
+
+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 {
+		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 = {
+	if (regno != GDB_REGNO_ZERO && regno <= GDB_REGNO_XPR31)
-		.get = riscv013_reg_get,
+		return &xreg_type;
-		.set = riscv013_reg_set,
+	if (regno >= GDB_REGNO_FPR0 && regno <= GDB_REGNO_FPR31) {
-		.flush = NULL
+		/* For now "freg_type" is the same as "xreg_type", but it will be
-	};
+		 * changed soon */
-	return &riscv013_reg_type;
+		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 +737,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) {
+	assert(regid >= GDB_REGNO_ZERO && regid <= GDB_REGNO_XPR31);
-		LOG_TARGET_ERROR(target, "Can't save register %s on a hart that is not halted.",
+	struct reg *reg = riscv_reg_impl_cache_entry(target, regid);
-				 riscv_reg_gdb_regno_name(target, regid));
+	assert(riscv_reg_impl_is_initialized(reg));
-		return ERROR_FAIL;
-	}
-	assert(riscv_reg_impl_gdb_regno_cacheable(regid, /* is write? */ false) &&
-			"Only cacheable registers can be saved.");
 
 	RISCV_INFO(r);
-	riscv_reg_t value;
+	if (target->state != TARGET_HALTED) {
-	if (!target->reg_cache) {
+		LOG_TARGET_ERROR(target, "Can't save register %s on a hart that is not halted.",
-		assert(!target_was_examined(target));
+				reg->name);
-		/* To create register cache it is needed to examine the target first,
+		return ERROR_TARGET_NOT_HALTED;
-		 * therefore during examine, any changed register needs to be saved
-		 * and restored manually.
-		 */
-		return ERROR_OK;
 	}
 
-	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)
+	int res = reg->type->get(reg);
-		return ERROR_FAIL;
+	if (res != ERROR_OK)
+		return res;
 
 	assert(reg->valid &&
 			"The register is cacheable, so the cache entry must be valid now.");

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
+ * This function is used to save the value of a X-register in cache. The
- * is marked as dirty, and writeback is delayed for as long as possible.
+ * register is marked as dirty, and writeback is delayed for as long as
- * Generally used to save registers before program buffer execution.
+ * 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.
  */
 int riscv013_reg_save(struct target *target, enum gdb_regno regid);
 

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) {
+		if (reg->type->flush(reg) != ERROR_OK)
-			riscv_reg_t value = buf_get_u64(reg->value, 0, reg->size);
+			res = ERROR_FAIL;
-
-			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");
+	if (res == ERROR_OK)
-	return ERROR_OK;
+		LOG_TARGET_DEBUG(target, "Flush of register cache completed successfully.");
-}
+	else
-
+		LOG_TARGET_DEBUG(target, "Flush of register cache failed.");
-/**
+	return res;
- * 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;
-	}
-
-	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;
 }
 
 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,
+	struct reg * const reg = riscv_reg_impl_cache_entry(target, regid);
-			/* write_through */ false);
+	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,
+	int res = riscv_reg_set(target, regid, value);
-			/* write_through */ true);
+	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(value);
-	assert(r);
-	if (r->dtm_version == DTM_DTMCS_VERSION_0_11)
-		return riscv011_get_register(target, value, regid);
 
-	keep_alive();
+	struct reg * const reg = riscv_reg_impl_cache_entry(target, regid);
-
-	if (regid == GDB_REGNO_PC)
-		return riscv_reg_get(target, value, GDB_REGNO_DPC);
-
-	struct reg *reg = riscv_reg_impl_cache_entry(target, regid);
 	assert(riscv_reg_impl_is_initialized(reg));
-	if (!reg->exist) {
+	assert(reg->size <= sizeof(riscv_reg_t) * CHAR_BIT);
-		LOG_TARGET_DEBUG(target, "Register %s does not exist.", reg->name);
+	assert(reg->size <= 64);
-		return ERROR_FAIL;
+	int res = reg->type->get(reg);
-	}
+	if (res != ERROR_OK)
-
+		return res;
-	if (reg->valid) {
+	*value = buf_get_u64(reg->value, 0, reg->size);
-		*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);
 	return ERROR_OK;
 }

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 */