Merge pull request #991 from en-sc/en-sc/dm-dmi-address-conversion

target/riscv: fix DM register address checks in `dm_read`/`dm_write`
riscv-collab · Jan 18, 2024 · e6e9fbe · e6e9fbe
2 parents 4fc0d86 + bb4c117
commit e6e9fbe
Show file tree

Hide file tree

Showing 3 changed files with 122 additions and 112 deletions.
diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c
@@ -694,6 +694,16 @@ static int dmi_write_exec(struct target *target, uint32_t address,
 	return dmi_op(target, NULL, NULL, DMI_OP_WRITE, address, value, true, ensure_success);
 }
 
+static uint32_t riscv013_get_dmi_address(const struct target *target, uint32_t address)
+{
+	assert(target);
+	uint32_t base = 0;
+	RISCV013_INFO(info);
+	if (info && info->dm)
+		base = info->dm->base;
+	return address + base;
+}
+
 static int dm_op_timeout(struct target *target, uint32_t *data_in,
 		bool *dmi_busy_encountered, int op, uint32_t address,
 		uint32_t data_out, int timeout_sec, bool exec, bool ensure_success)
@@ -2772,8 +2782,7 @@ static int init_target(struct command_context *cmd_ctx,
 	generic_info->authdata_write = &riscv013_authdata_write;
 	generic_info->dmi_read = &dmi_read;
 	generic_info->dmi_write = &dmi_write;
-	generic_info->dm_read = &dm_read;
-	generic_info->dm_write = &dm_write;
+	generic_info->get_dmi_address = &riscv013_get_dmi_address;
 	generic_info->read_memory = read_memory;
 	generic_info->hart_count = &riscv013_hart_count;
 	generic_info->data_bits = &riscv013_data_bits;

diff --git a/src/target/riscv/riscv.c b/src/target/riscv/riscv.c
@@ -3690,143 +3690,138 @@ COMMAND_HANDLER(riscv_authdata_write)
 	return r->authdata_write(target, value, index);
 }
 
-COMMAND_HANDLER(riscv_dmi_read)
+static uint32_t riscv_get_dmi_address(const struct target *target, uint32_t dm_address)
 {
-	if (CMD_ARGC != 1) {
-		LOG_ERROR("Command takes 1 parameter");
-		return ERROR_COMMAND_SYNTAX_ERROR;
-	}
+	assert(target);
+	RISCV_INFO(r);
+	if (!r || !r->get_dmi_address)
+		return dm_address;
+	return r->get_dmi_address(target, dm_address);
+}
 
-	struct target *target = get_current_target(CMD_CTX);
+static int riscv_dmi_read(struct target *target, uint32_t *value, uint32_t address)
+{
 	if (!target) {
 		LOG_ERROR("target is NULL!");
 		return ERROR_FAIL;
 	}
-
 	RISCV_INFO(r);
 	if (!r) {
 		LOG_TARGET_ERROR(target, "riscv_info is NULL!");
 		return ERROR_FAIL;
 	}
-
-	if (r->dmi_read) {
-		uint32_t address, value;
-		COMMAND_PARSE_NUMBER(u32, CMD_ARGV[0], address);
-		if (r->dmi_read(target, &value, address) != ERROR_OK)
-			return ERROR_FAIL;
-		command_print(CMD, "0x%" PRIx32, value);
-	} else {
-		LOG_TARGET_ERROR(target, "dmi_read is not implemented for this target.");
+	if (!r->dmi_read) {
+		LOG_TARGET_ERROR(target, "dmi_read is not implemented.");
 		return ERROR_FAIL;
 	}
-	return ERROR_OK;
+	return r->dmi_read(target, value, address);
 }
 
-COMMAND_HANDLER(riscv_dmi_write)
+static int riscv_dmi_write(struct target *target, uint32_t dmi_address, uint32_t value)
 {
-	if (CMD_ARGC != 2) {
-		LOG_ERROR("Command takes exactly 2 arguments");
-		return ERROR_COMMAND_SYNTAX_ERROR;
-	}
-
-	struct target *target = get_current_target(CMD_CTX);
-	RISCV_INFO(r);
-
-	uint32_t address, value;
-	COMMAND_PARSE_NUMBER(u32, CMD_ARGV[0], address);
-	COMMAND_PARSE_NUMBER(u32, CMD_ARGV[1], value);
-
-	if (r->dmi_write) {
-		/* Perform the DMI write */
-		int retval = r->dmi_write(target, address, value);
-
-		/* Invalidate our cached progbuf copy:
-		   - if the user tinkered directly with a progbuf register
-		   - if debug module was reset, in which case progbuf registers
-		     may not retain their value.
-		*/
-		bool progbuf_touched = (address >= DM_PROGBUF0 && address <= DM_PROGBUF15);
-		bool dm_deactivated = (address == DM_DMCONTROL && (value & DM_DMCONTROL_DMACTIVE) == 0);
-		if (progbuf_touched || dm_deactivated) {
-			if (r->invalidate_cached_progbuf)
-				r->invalidate_cached_progbuf(target);
-		}
-
-		return retval;
-	}
-
-	LOG_TARGET_ERROR(target, "dmi_write is not implemented for this target.");
-	return ERROR_FAIL;
-}
-
-
-COMMAND_HANDLER(riscv_dm_read)
-{
-	if (CMD_ARGC != 1) {
-		LOG_ERROR("Command takes 1 parameter");
-		return ERROR_COMMAND_SYNTAX_ERROR;
-	}
-
-	struct target *target = get_current_target(CMD_CTX);
 	if (!target) {
 		LOG_ERROR("target is NULL!");
 		return ERROR_FAIL;
 	}
-
 	RISCV_INFO(r);
 	if (!r) {
 		LOG_TARGET_ERROR(target, "riscv_info is NULL!");
 		return ERROR_FAIL;
 	}
-
-	if (r->dm_read) {
-		uint32_t address, value;
-		COMMAND_PARSE_NUMBER(u32, CMD_ARGV[0], address);
-		if (r->dm_read(target, &value, address) != ERROR_OK)
-			return ERROR_FAIL;
-		command_print(CMD, "0x%" PRIx32, value);
-	} else {
-		LOG_TARGET_ERROR(target, "dm_read is not implemented for this target.");
+	if (!r->dmi_write) {
+		LOG_TARGET_ERROR(target, "dmi_write is not implemented.");
 		return ERROR_FAIL;
 	}
-	return ERROR_OK;
+	const int result = r->dmi_write(target, dmi_address, value);
+	/* Invalidate our cached progbuf copy:
+	 * - if the user tinkered directly with a progbuf register
+	 * - if debug module was reset, in which case progbuf registers
+	 * may not retain their value.
+	 * FIXME: If there are multiple DMs on a single TAP, it is possible to
+	 * clobber progbuf or reset the DM of another target.
+	 */
+	const bool progbuf_touched =
+			(dmi_address >= riscv_get_dmi_address(target, DM_PROGBUF0) &&
+			dmi_address <= riscv_get_dmi_address(target, DM_PROGBUF15));
+	const bool dm_deactivated =
+			(dmi_address == riscv_get_dmi_address(target, DM_DMCONTROL) &&
+			(value & DM_DMCONTROL_DMACTIVE) == 0);
+	if (progbuf_touched || dm_deactivated) {
+		if (r->invalidate_cached_progbuf) {
+			/* Here the return value of invalidate_cached_progbuf()
+			 * is ignored. It is okay to do so for now, since the
+			 * only case an error is returned is a failure to
+			 * assign a DM to the target, which would have already
+			 * caused an error during dmi_write().
+			 * FIXME: invalidate_cached_progbuf() should be void.
+			 */
+			r->invalidate_cached_progbuf(target);
+		} else {
+			LOG_TARGET_DEBUG(target,
+					"invalidate_cached_progbuf() is not implemented.");
+		}
+	}
+	return result;
 }
 
-COMMAND_HANDLER(riscv_dm_write)
+COMMAND_HANDLER(handle_riscv_dmi_read)
 {
-	if (CMD_ARGC != 2) {
-		LOG_ERROR("Command takes exactly 2 arguments");
+	if (CMD_ARGC != 1)
 		return ERROR_COMMAND_SYNTAX_ERROR;
-	}
 
-	struct target *target = get_current_target(CMD_CTX);
-	RISCV_INFO(r);
+	uint32_t dmi_address;
+	COMMAND_PARSE_NUMBER(u32, CMD_ARGV[0], dmi_address);
+
+	struct target * const target = get_current_target(CMD_CTX);
+	uint32_t value;
+	const int result = riscv_dmi_read(target, &value, dmi_address);
+	if (result == ERROR_OK)
+		command_print(CMD, "0x%" PRIx32, value);
+	return result;
+}
 
-	uint32_t address, value;
-	COMMAND_PARSE_NUMBER(u32, CMD_ARGV[0], address);
+COMMAND_HANDLER(handle_riscv_dmi_write)
+{
+	if (CMD_ARGC != 2)
+		return ERROR_COMMAND_SYNTAX_ERROR;
+
+	uint32_t dmi_address, value;
+	COMMAND_PARSE_NUMBER(u32, CMD_ARGV[0], dmi_address);
 	COMMAND_PARSE_NUMBER(u32, CMD_ARGV[1], value);
 
-	if (r->dm_write) {
-		/* Perform the DM write */
-		int retval = r->dm_write(target, address, value);
-
-		/* Invalidate our cached progbuf copy:
-		   - if the user tinkered directly with a progbuf register
-		   - if debug module was reset, in which case progbuf registers
-		     may not retain their value.
-		*/
-		bool progbuf_touched = (address >= DM_PROGBUF0 && address <= DM_PROGBUF15);
-		bool dm_deactivated = (address == DM_DMCONTROL && (value & DM_DMCONTROL_DMACTIVE) == 0);
-		if (progbuf_touched || dm_deactivated) {
-			if (r->invalidate_cached_progbuf)
-				r->invalidate_cached_progbuf(target);
-		}
+	struct target * const target = get_current_target(CMD_CTX);
+	return riscv_dmi_write(target, dmi_address, value);
+}
 
-		return retval;
-	}
+COMMAND_HANDLER(handle_riscv_dm_read)
+{
+	if (CMD_ARGC != 1)
+		return ERROR_COMMAND_SYNTAX_ERROR;
 
-	LOG_TARGET_ERROR(target, "dm_write is not implemented for this target.");
-	return ERROR_FAIL;
+	uint32_t dm_address;
+	COMMAND_PARSE_NUMBER(u32, CMD_ARGV[0], dm_address);
+
+	struct target * const target = get_current_target(CMD_CTX);
+	uint32_t value;
+	const int result = riscv_dmi_read(target, &value,
+			riscv_get_dmi_address(target, dm_address));
+	if (result == ERROR_OK)
+		command_print(CMD, "0x%" PRIx32, value);
+	return result;
+}
+
+COMMAND_HANDLER(handle_riscv_dm_write)
+{
+	if (CMD_ARGC != 2)
+		return ERROR_COMMAND_SYNTAX_ERROR;
+
+	uint32_t dm_address, value;
+	COMMAND_PARSE_NUMBER(u32, CMD_ARGV[0], dm_address);
+	COMMAND_PARSE_NUMBER(u32, CMD_ARGV[1], value);
+
+	struct target * const target = get_current_target(CMD_CTX);
+	return riscv_dmi_write(target, riscv_get_dmi_address(target, dm_address),
+					value);
 }
 
 COMMAND_HANDLER(riscv_reset_delays)
@@ -4653,31 +4648,34 @@ static const struct command_registration riscv_exec_command_handlers[] = {
 	},
 	{
 		.name = "dmi_read",
-		.handler = riscv_dmi_read,
+		.handler = handle_riscv_dmi_read,
 		.mode = COMMAND_ANY,
 		.usage = "address",
-		.help = "Perform a 32-bit DMI read at address, returning the value."
+		.help = "Read and return 32-bit value from the given address on the "
+				"RISC-V DMI bus."
 	},
 	{
 		.name = "dmi_write",
-		.handler = riscv_dmi_write,
+		.handler = handle_riscv_dmi_write,
 		.mode = COMMAND_ANY,
 		.usage = "address value",
-		.help = "Perform a 32-bit DMI write of value at address."
+		.help = "Write a 32-bit value to the given address on the RISC-V DMI bus."
 	},
 	{
 		.name = "dm_read",
-		.handler = riscv_dm_read,
+		.handler = handle_riscv_dm_read,
 		.mode = COMMAND_ANY,
 		.usage = "reg_address",
-		.help = "Perform a 32-bit read from DM register at reg_address, returning the value."
+		.help = "Read and return 32-bit value from a debug module's register "
+				"at reg_address."
 	},
 	{
 		.name = "dm_write",
-		.handler = riscv_dm_write,
+		.handler = handle_riscv_dm_write,
 		.mode = COMMAND_ANY,
 		.usage = "reg_address value",
-		.help = "Write a 32-bit value to the DM register at reg_address."
+		.help = "Write a 32-bit value to the debug module's register at "
+				"reg_address."
 	},
 	{
 		.name = "reset_delays",

diff --git a/src/target/riscv/riscv.h b/src/target/riscv/riscv.h
@@ -242,8 +242,11 @@ struct riscv_info {
 	int (*dmi_read)(struct target *target, uint32_t *value, uint32_t address);
 	int (*dmi_write)(struct target *target, uint32_t address, uint32_t value);
 
-	int (*dm_read)(struct target *target, uint32_t *value, uint32_t address);
-	int (*dm_write)(struct target *target, uint32_t address, uint32_t value);
+	/* Get the DMI address of target's DM's register.
+	 * The function should return the passed address
+	 * if the target is not assigned a DM yet.
+	 */
+	uint32_t (*get_dmi_address)(const struct target *target, uint32_t dm_address);
 
 	int (*sample_memory)(struct target *target,
 						 struct riscv_sample_buf *buf,