target/riscv: stop using register_get/set for 0.11 targets

Caching is somewhat handled in `riscv-011.c`. Handling it additionaly in `riscv.c` may cause problems. Sice there is no simulator that supports RISC-V Debug Specification v0.11, so it is not feaseable to automate testing. This commit separates 0.11 register accesses and unlocks further development in this area. Change-Id: I73ff17ef85106c4ababa38319f446f6c384a1750 Signed-off-by: Evgeniy Naydanov <[email protected]>
en-sc · May 27, 2024 · 10db38f · 10db38f
1 parent 3dbf1e6
commit 10db38f
Show file tree

Hide file tree

Showing 2 changed files with 107 additions and 13 deletions.
diff --git a/src/target/riscv/riscv-011.c b/src/target/riscv/riscv-011.c
@@ -1261,7 +1261,7 @@ static int register_read(struct target *target, riscv_reg_t *value, int regnum)
 	return ERROR_OK;
 }
 
-/* Write the register. No caching or games. */
+/* Write the register. */
 static int register_write(struct target *target, unsigned int number,
 		uint64_t value)
 {
@@ -1337,6 +1337,10 @@ static int get_register(struct target *target, riscv_reg_t *value,
 	maybe_write_tselect(target);
 
 	if (regid <= GDB_REGNO_XPR31) {
+		/* FIXME: Here the implementation assumes that the value
+		 * written to GPR will be the same as the value read back. This
+		 * is not true for a write of a non-zero value to x0.
+		 */
 		*value = reg_cache_get(target, regid);
 	} else if (regid == GDB_REGNO_PC || regid == GDB_REGNO_DPC) {
 		*value = info->dpc;
@@ -1374,10 +1378,34 @@ static int get_register(struct target *target, riscv_reg_t *value,
 	return ERROR_OK;
 }
 
+/* This function is intended to handle accesses to registers through register
+ * cache. */
 static int set_register(struct target *target, enum gdb_regno regid,
 		riscv_reg_t value)
 {
-	return register_write(target, regid, value);
+	assert(target->reg_cache);
+	assert(target->reg_cache->reg_list);
+	struct reg * const reg = &target->reg_cache->reg_list[regid];
+	assert(reg);
+	/* On RISC-V 0.11 targets valid value of some registers (e.g. `dcsr`)
+	 * is stored in `riscv011_info_t` itself, not in register cache. This
+	 * complicates register cache implementation.
+	 * Therefore, for now, caching registers in register cache is disabled
+	 * for all registers, except for GPRs.
+	 * FIXME: x0 (zero) should not be cached on writes.
+	 */
+	const bool cacheable = regid <= GDB_REGNO_XPR31;
+	if (cacheable) {
+		reg->dirty = true;
+	} else {
+		assert(!reg->dirty);
+		int result = register_write(target, regid, value);
+		if (result != ERROR_OK)
+			return result;
+	}
+	reg_cache_set(target, regid, value);
+	reg->valid = cacheable;
+	return ERROR_OK;
 }
 
 static int halt(struct target *target)

diff --git a/src/target/riscv/riscv.c b/src/target/riscv/riscv.c
@@ -5594,7 +5594,10 @@ 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);
 	assert(r->set_register);
+	if (r->dtm_version == DTM_DTMCS_VERSION_0_11)
+		return r->set_register(target, regid, value);
 
 	keep_alive();
 
@@ -5691,7 +5694,10 @@ int riscv_get_register(struct target *target, riscv_reg_t *value,
 		enum gdb_regno regid)
 {
 	RISCV_INFO(r);
+	assert(r);
 	assert(r->get_register);
+	if (r->dtm_version == DTM_DTMCS_VERSION_0_11)
+		return r->get_register(target, value, regid);
 
 	keep_alive();
 
@@ -6147,13 +6153,17 @@ const char *gdb_regno_name(const struct target *target, enum gdb_regno regno)
 	return NULL;
 }
 
+static struct target *get_target_from_reg(const struct reg *reg);
+
 /**
  * This function is the handler of user's request to read a register.
  */
-static int register_get(struct reg *reg)
+static int riscv013_reg_get(struct reg *reg)
 {
-	struct target *target = ((riscv_reg_info_t *)reg->arch_info)->target;
+	struct target *target = get_target_from_reg(reg);
 	RISCV_INFO(r);
+	assert(r);
+	assert(r->dtm_version == DTM_DTMCS_VERSION_1_0);
 
 	/* TODO: Hack to deal with gdb that thinks these registers still exist. */
 	if (reg->number > GDB_REGNO_XPR15 && reg->number <= GDB_REGNO_XPR31 &&
@@ -6191,10 +6201,12 @@ static int register_get(struct reg *reg)
 /**
  * This function is the handler of user's request to write a register.
  */
-static int register_set(struct reg *reg, uint8_t *buf)
+static int riscv013_reg_set(struct reg *reg, uint8_t *buf)
 {
-	struct target *target = ((riscv_reg_info_t *)reg->arch_info)->target;
+	struct target *target = get_target_from_reg(reg);
 	RISCV_INFO(r);
+	assert(r);
+	assert(r->dtm_version == DTM_DTMCS_VERSION_1_0);
 
 	char *str = buf_to_hex_str(buf, reg->size);
 	LOG_TARGET_DEBUG(target, "Write 0x%s to %s (valid=%d).",	str, reg->name,
@@ -6240,11 +6252,6 @@ static int register_set(struct reg *reg, uint8_t *buf)
 	return ERROR_OK;
 }
 
-static struct reg_arch_type riscv_reg_arch_type = {
-	.get = register_get,
-	.set = register_set
-};
-
 static int init_custom_register_names(struct list_head *expose_custom,
 		struct reg_name_table *custom_register_names)
 {
@@ -6295,7 +6302,7 @@ static bool is_known_standard_csr(unsigned int csr_num)
 	return is_csr_in_buf[csr_num];
 }
 
-bool reg_is_initialized(const struct reg *reg)
+static bool reg_is_initialized(const struct reg *reg)
 {
 	assert(reg);
 	if (!reg->feature) {
@@ -6310,6 +6317,65 @@ bool reg_is_initialized(const struct reg *reg)
 	return true;
 }
 
+static struct target *get_target_from_reg(const struct reg *reg)
+{
+	assert(reg_is_initialized(reg));
+	return ((const riscv_reg_info_t *)reg->arch_info)->target;
+}
+
+static int riscv011_reg_get(struct reg *reg)
+{
+	struct target * const target = get_target_from_reg(reg);
+	RISCV_INFO(r);
+	assert(r);
+	assert(r->dtm_version == DTM_DTMCS_VERSION_0_11);
+	riscv_reg_t value;
+	const int result = r->get_register(target, &value, reg->number);
+	if (result != ERROR_OK)
+		return result;
+	buf_set_u64(reg->value, 0, reg->size, value);
+	return ERROR_OK;
+}
+
+static int riscv011_reg_set(struct reg *reg, uint8_t *buf)
+{
+	const riscv_reg_t value = buf_get_u64(buf, 0, reg->size);
+	struct target * const target = get_target_from_reg(reg);
+	RISCV_INFO(r);
+	assert(r);
+	assert(r->dtm_version == DTM_DTMCS_VERSION_0_11);
+	if (reg->number == GDB_REGNO_TDATA1 || reg->number == GDB_REGNO_TDATA2) {
+		r->manual_hwbp_set = true;
+		/* When enumerating triggers, we clear any triggers with DMODE set,
+		 * assuming they were left over from a previous debug session. So make
+		 * sure that is done before a user might be setting their own triggers.
+		 */
+		if (riscv_enumerate_triggers(target) != ERROR_OK)
+			return ERROR_FAIL;
+	}
+	return r->set_register(target, reg->number, value);
+}
+
+static struct reg_arch_type *gdb_regno_reg_type(const struct target *target,
+		uint32_t regno)
+{
+	RISCV_INFO(info);
+	assert(info);
+	if (info->dtm_version == DTM_DTMCS_VERSION_0_11) {
+		static struct reg_arch_type riscv011_reg_type = {
+			.get = riscv011_reg_get,
+			.set = riscv011_reg_set
+		};
+		return &riscv011_reg_type;
+	}
+
+	static struct reg_arch_type riscv013_reg_type = {
+		.get = riscv013_reg_get,
+		.set = riscv013_reg_set
+	};
+	return &riscv013_reg_type;
+}
+
 static struct reg_feature *gdb_regno_feature(uint32_t regno)
 {
 	if (regno <= GDB_REGNO_XPR31 || regno == GDB_REGNO_PC) {
@@ -6683,7 +6749,7 @@ static int init_reg(struct target *target, uint32_t regno)
 	if (reg_is_initialized(reg))
 		return ERROR_OK;
 	reg->number = regno;
-	reg->type = &riscv_reg_arch_type;
+	reg->type = gdb_regno_reg_type(target, regno);
 	reg->dirty = false;
 	reg->valid = false;
 	reg->hidden = false;