Fix data types around batch.{c,h}

Check that the value of dtmcs.abits is in the expected range.

Make corrections of data types in batch.{c,h} and in related code.
Some of the issues were found by activating "-Wconversion" in GCC,
others by inspecting the code manually.

This is an initial step towards being able to use "-Wconversion" on
RISC-V target code, which will give us bit more confidence when
refactoring or merging new patches.

Changes made:

- Check `dtmcs.abits` during examination.

- DMI address is no larger than 32-bits per the debug spec.
  Changed address parameters of multiple functions from uint64_t
  to uint32_t.

- The value passed to jtag_add_runtest() is now `unsigned int`,
  not `int`. No need for `assert(idle <= INT_MAX)` anymore.

- `get_delay()` in batch.c can return an unsigned value.

- Added few assertions around `field->num_bits` in batch.c.

Change-Id: Ibfccd62d552063df6ab9b5a2d4ea4ed23617d3db
Signed-off-by: Jan Matyas <[email protected]>

Author: JanMatCodasip

src/target/riscv/batch.c

@@ -10,13 +10,24 @@
 #include "riscv.h"
 #include "field_helpers.h"
 
+// TODO: DTM_DMI_MAX_ADDRESS_LENGTH should be reduced to 32 (per the debug spec)
 #define DTM_DMI_MAX_ADDRESS_LENGTH	((1<<DTM_DTMCS_ABITS_LENGTH)-1)
 #define DMI_SCAN_MAX_BIT_LENGTH (DTM_DMI_MAX_ADDRESS_LENGTH + DTM_DMI_DATA_LENGTH + DTM_DMI_OP_LENGTH)
+
 #define DMI_SCAN_BUF_SIZE (DIV_ROUND_UP(DMI_SCAN_MAX_BIT_LENGTH, 8))
 
 /* Reserve extra room in the batch (needed for the last NOP operation) */
 #define BATCH_RESERVED_SCANS 1
 
+static unsigned int get_dmi_scan_length(const struct target *target)
+{
+	const unsigned int abits = riscv_get_dmi_address_bits(target);
+	assert(abits > 0);
+	assert(abits <= DTM_DMI_MAX_ADDRESS_LENGTH);
+
+	return abits + DTM_DMI_DATA_LENGTH + DTM_DMI_OP_LENGTH;
+}
+
 struct riscv_batch *riscv_batch_alloc(struct target *target, size_t scans)
 {
 	scans += BATCH_RESERVED_SCANS;
@@ -127,11 +138,10 @@ static void add_idle_before_batch(const struct riscv_batch *batch, size_t start_
 	const unsigned int idle_change = new_delay - batch->last_scan_delay;
 	LOG_TARGET_DEBUG(batch->target, "Adding %u idle cycles before the batch.",
 			idle_change);
-	assert(idle_change <= INT_MAX);
 	jtag_add_runtest(idle_change, TAP_IDLE);
 }
 
-static int get_delay(const struct riscv_batch *batch, size_t scan_idx,
+static unsigned int get_delay(const struct riscv_batch *batch, size_t scan_idx,
 		const struct riscv_scan_delays *delays, bool resets_delays,
 		size_t reset_delays_after)
 {
@@ -142,7 +152,6 @@ static int get_delay(const struct riscv_batch *batch, size_t scan_idx,
 	const enum riscv_scan_delay_class delay_class =
 		batch->delay_classes[scan_idx];
 	const unsigned int delay = riscv_scan_get_delay(delays, delay_class);
-	assert(delay <= INT_MAX);
 	return delays_were_reset ? 0 : delay;
 }
 
@@ -198,10 +207,7 @@ static void log_batch(const struct riscv_batch *batch, size_t start_idx,
 	if (debug_level < LOG_LVL_DEBUG)
 		return;
 
-	const unsigned int scan_bits = batch->fields->num_bits;
-	assert(scan_bits == (unsigned int)riscv_get_dmi_scan_length(batch->target));
-	const unsigned int abits = scan_bits - DTM_DMI_OP_LENGTH
-			- DTM_DMI_DATA_LENGTH;
+	const unsigned int abits = riscv_get_dmi_address_bits(batch->target);
 
 	/* Determine the "op" and "address" of the scan that preceded the first
 	 * executed scan.
@@ -211,7 +217,7 @@ static void log_batch(const struct riscv_batch *batch, size_t start_idx,
 	 * would be a more robust solution.
 	 */
 	bool last_scan_was_read = false;
-	uint32_t last_scan_address = -1 /* to silence maybe-uninitialized */;
+	uint32_t last_scan_address = (uint32_t)(-1) /* to silence maybe-uninitialized */;
 	if (start_idx > 0) {
 		const struct scan_field * const field = &batch->fields[start_idx - 1];
 		assert(field->out_value);
@@ -224,7 +230,7 @@ static void log_batch(const struct riscv_batch *batch, size_t start_idx,
 	/* Decode and log every executed scan */
 	for (size_t i = start_idx; i < batch->used_scans; ++i) {
 		static const char * const op_string[] = {"-", "r", "w", "?"};
-		const int delay = get_delay(batch, i, delays, resets_delays,
+		const unsigned int delay = get_delay(batch, i, delays, resets_delays,
 				reset_delays_after);
 		const struct scan_field * const field = &batch->fields[i];
 
@@ -247,15 +253,15 @@ static void log_batch(const struct riscv_batch *batch, size_t start_idx,
 					DTM_DMI_ADDRESS_OFFSET, abits);
 
 			LOG_DEBUG("%db %s %08" PRIx32 " @%02" PRIx32
-					" -> %s %08" PRIx32 " @%02" PRIx32 "; %di",
+					" -> %s %08" PRIx32 " @%02" PRIx32 "; %ui",
 					field->num_bits, op_string[out_op], out_data, out_address,
 					status_string[in_op], in_data, in_address, delay);
 
 			if (last_scan_was_read && in_op == DTM_DMI_OP_SUCCESS)
 				log_dmi_decoded(batch, /*write*/ false,
 						last_scan_address, in_data);
 		} else {
-			LOG_DEBUG("%db %s %08" PRIx32 " @%02" PRIx32 " -> ?; %di",
+			LOG_DEBUG("%db %s %08" PRIx32 " @%02" PRIx32 " -> ?; %ui",
 					field->num_bits, op_string[out_op], out_data, out_address,
 					delay);
 		}
@@ -321,35 +327,56 @@ int riscv_batch_run_from(struct riscv_batch *batch, size_t start_idx,
 	return ERROR_OK;
 }
 
-void riscv_batch_add_dmi_write(struct riscv_batch *batch, uint64_t address, uint32_t data,
+void riscv_batch_add_dmi_write(struct riscv_batch *batch, uint32_t address, uint32_t data,
 		bool read_back, enum riscv_scan_delay_class delay_class)
 {
+	// TODO: Check that the bit width of "address" is no more than dtmcs.abits,
+	// otherwise return an error (during batch creation or when the batch is executed).
+
 	assert(batch->used_scans < batch->allocated_scans);
 	struct scan_field *field = batch->fields + batch->used_scans;
-	field->num_bits = riscv_get_dmi_scan_length(batch->target);
-	field->out_value = (void *)(batch->data_out + batch->used_scans * DMI_SCAN_BUF_SIZE);
-	riscv_fill_dmi_write(batch->target, (char *)field->out_value, address, data);
+
+	field->num_bits = get_dmi_scan_length(batch->target);
+	assert(field->num_bits <= DMI_SCAN_MAX_BIT_LENGTH);
+
+	uint8_t *out_value = batch->data_out + batch->used_scans * DMI_SCAN_BUF_SIZE;
+	uint8_t *in_value = batch->data_in + batch->used_scans * DMI_SCAN_BUF_SIZE;
+
+	field->out_value = out_value;
+	riscv_fill_dmi_write(batch->target, out_value, address, data);
+
 	if (read_back) {
-		field->in_value = (void *)(batch->data_in + batch->used_scans * DMI_SCAN_BUF_SIZE);
-		riscv_fill_dm_nop(batch->target, (char *)field->in_value);
+		field->in_value = in_value;
+		riscv_fill_dm_nop(batch->target, in_value);
 	} else {
 		field->in_value = NULL;
 	}
+
 	batch->delay_classes[batch->used_scans] = delay_class;
 	batch->last_scan = RISCV_SCAN_TYPE_WRITE;
 	batch->used_scans++;
 }
 
-size_t riscv_batch_add_dmi_read(struct riscv_batch *batch, uint64_t address,
+size_t riscv_batch_add_dmi_read(struct riscv_batch *batch, uint32_t address,
 		enum riscv_scan_delay_class delay_class)
 {
+	// TODO: Check that the bit width of "address" is no more than dtmcs.abits,
+	// otherwise return an error (during batch creation or when the batch is executed).
+
 	assert(batch->used_scans < batch->allocated_scans);
 	struct scan_field *field = batch->fields + batch->used_scans;
-	field->num_bits = riscv_get_dmi_scan_length(batch->target);
-	field->out_value = (void *)(batch->data_out + batch->used_scans * DMI_SCAN_BUF_SIZE);
-	field->in_value  = (void *)(batch->data_in  + batch->used_scans * DMI_SCAN_BUF_SIZE);
-	riscv_fill_dmi_read(batch->target, (char *)field->out_value, address);
-	riscv_fill_dm_nop(batch->target, (char *)field->in_value);
+
+	field->num_bits = get_dmi_scan_length(batch->target);
+	assert(field->num_bits <= DMI_SCAN_MAX_BIT_LENGTH);
+
+	uint8_t *out_value = batch->data_out + batch->used_scans * DMI_SCAN_BUF_SIZE;
+	uint8_t *in_value = batch->data_in + batch->used_scans * DMI_SCAN_BUF_SIZE;
+
+	field->out_value = out_value;
+	field->in_value = in_value;
+	riscv_fill_dmi_read(batch->target, out_value, address);
+	riscv_fill_dm_nop(batch->target, in_value);
+
 	batch->delay_classes[batch->used_scans] = delay_class;
 	batch->last_scan = RISCV_SCAN_TYPE_READ;
 	batch->used_scans++;
@@ -382,11 +409,18 @@ void riscv_batch_add_nop(struct riscv_batch *batch)
 {
 	assert(batch->used_scans < batch->allocated_scans);
 	struct scan_field *field = batch->fields + batch->used_scans;
-	field->num_bits = riscv_get_dmi_scan_length(batch->target);
-	field->out_value = (void *)(batch->data_out + batch->used_scans * DMI_SCAN_BUF_SIZE);
-	field->in_value  = (void *)(batch->data_in  + batch->used_scans * DMI_SCAN_BUF_SIZE);
-	riscv_fill_dm_nop(batch->target, (char *)field->out_value);
-	riscv_fill_dm_nop(batch->target, (char *)field->in_value);
+
+	field->num_bits = get_dmi_scan_length(batch->target);
+	assert(field->num_bits <= DMI_SCAN_MAX_BIT_LENGTH);
+
+	uint8_t *out_value = batch->data_out + batch->used_scans * DMI_SCAN_BUF_SIZE;
+	uint8_t *in_value = batch->data_in + batch->used_scans * DMI_SCAN_BUF_SIZE;
+
+	field->out_value = out_value;
+	field->in_value = in_value;
+	riscv_fill_dm_nop(batch->target, out_value);
+	riscv_fill_dm_nop(batch->target, in_value);
+
 	/* DMI NOP never triggers any debug module operation,
 	 * so the shortest (base) delay can be used. */
 	batch->delay_classes[batch->used_scans] = RISCV_DELAY_BASE;

src/target/riscv/batch.h

@@ -190,11 +190,11 @@ int riscv_batch_run_from(struct riscv_batch *batch, size_t start_idx,
 size_t riscv_batch_finished_scans(const struct riscv_batch *batch);
 
 /* Adds a DM register write to this batch. */
-void riscv_batch_add_dmi_write(struct riscv_batch *batch, uint64_t address, uint32_t data,
+void riscv_batch_add_dmi_write(struct riscv_batch *batch, uint32_t address, uint32_t data,
 	bool read_back, enum riscv_scan_delay_class delay_class);
 
 static inline void
-riscv_batch_add_dm_write(struct riscv_batch *batch, uint64_t address, uint32_t data,
+riscv_batch_add_dm_write(struct riscv_batch *batch, uint32_t address, uint32_t data,
 	bool read_back, enum riscv_scan_delay_class delay_type)
 {
 	return riscv_batch_add_dmi_write(batch,
@@ -205,11 +205,11 @@ riscv_batch_add_dm_write(struct riscv_batch *batch, uint64_t address, uint32_t d
 /* DM register reads must be handled in two parts: the first one schedules a read and
  * provides a key, the second one actually obtains the result of the read -
  * status (op) and the actual data. */
-size_t riscv_batch_add_dmi_read(struct riscv_batch *batch, uint64_t address,
+size_t riscv_batch_add_dmi_read(struct riscv_batch *batch, uint32_t address,
 		enum riscv_scan_delay_class delay_class);
 
 static inline size_t
-riscv_batch_add_dm_read(struct riscv_batch *batch, uint64_t address,
+riscv_batch_add_dm_read(struct riscv_batch *batch, uint32_t address,
 		enum riscv_scan_delay_class delay_type)
 {
 	return riscv_batch_add_dmi_read(batch,

src/target/riscv/riscv-013.c

@@ -56,10 +56,10 @@ static riscv_insn_t riscv013_read_progbuf(struct target *target, unsigned int
 		index);
 static int riscv013_invalidate_cached_progbuf(struct target *target);
 static int riscv013_execute_progbuf(struct target *target, uint32_t *cmderr);
-static void riscv013_fill_dmi_write(struct target *target, char *buf, uint64_t a, uint32_t d);
-static void riscv013_fill_dmi_read(struct target *target, char *buf, uint64_t a);
-static int riscv013_get_dmi_scan_length(struct target *target);
-static void riscv013_fill_dm_nop(struct target *target, char *buf);
+static void riscv013_fill_dmi_write(const struct target *target, uint8_t *buf, uint32_t a, uint32_t d);
+static void riscv013_fill_dmi_read(const struct target *target, uint8_t *buf, uint32_t a);
+static unsigned int riscv013_get_dmi_address_bits(const struct target *target);
+static void riscv013_fill_dm_nop(const struct target *target, uint8_t *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);
@@ -1941,6 +1941,29 @@ static int examine(struct target *target)
 	info->abits = get_field(dtmcontrol, DTM_DTMCS_ABITS);
 	info->dtmcs_idle = get_field(dtmcontrol, DTM_DTMCS_IDLE);
 
+	if (info->abits > RISCV013_DTMCS_ABITS_MAX) {
+		/* Max. address width given by the debug specification is exceeded */
+		LOG_TARGET_ERROR(target, "The target's debug bus (DMI) address width exceeds "
+			"the maximum:");
+		LOG_TARGET_ERROR(target, " found dtmcs.abits = %d; maximum is abits = %d.",
+			info->abits, RISCV013_DTMCS_ABITS_MAX);
+		return ERROR_FAIL;
+	}
+
+	if (info->abits < RISCV013_DTMCS_ABITS_MIN) {
+		/* The requirement for minimum DMI address width of 7 bits is part of
+		 * the RISC-V Debug spec since Jan-20-2017 (commit 03df6ee7). However,
+		 * implementations exist that implement narrower DMI address. For example
+		 * Spike as of Q1/2025 uses dmi.abits = 6.
+		 *
+		 * For that reason, warn the user but continue.
+		 */
+		LOG_TARGET_WARNING(target, "The target's debug bus (DMI) address width is "
+			"lower than the minimum:");
+		LOG_TARGET_WARNING(target, " found dtmcs.abits = %d; minimum is abits = %d.",
+			info->abits, RISCV013_DTMCS_ABITS_MIN);
+	}
+
 	if (!check_dbgbase_exists(target)) {
 		LOG_TARGET_ERROR(target, "Could not find debug module with DMI base address (dbgbase) = 0x%x", target->dbgbase);
 		return ERROR_FAIL;
@@ -2771,7 +2794,7 @@ static int init_target(struct command_context *cmd_ctx,
 	generic_info->fill_dmi_write = &riscv013_fill_dmi_write;
 	generic_info->fill_dmi_read = &riscv013_fill_dmi_read;
 	generic_info->fill_dm_nop = &riscv013_fill_dm_nop;
-	generic_info->get_dmi_scan_length = &riscv013_get_dmi_scan_length;
+	generic_info->get_dmi_address_bits = &riscv013_get_dmi_address_bits;
 	generic_info->authdata_read = &riscv013_authdata_read;
 	generic_info->authdata_write = &riscv013_authdata_write;
 	generic_info->dmi_read = &dmi_read;
@@ -5390,34 +5413,34 @@ static int riscv013_execute_progbuf(struct target *target, uint32_t *cmderr)
 	return riscv013_execute_abstract_command(target, run_program, cmderr);
 }
 
-static void riscv013_fill_dmi_write(struct target *target, char *buf, uint64_t a, uint32_t d)
+static void riscv013_fill_dmi_write(const struct target *target, uint8_t *buf, uint32_t a, uint32_t d)
 {
 	RISCV013_INFO(info);
-	buf_set_u64((unsigned char *)buf, DTM_DMI_OP_OFFSET, DTM_DMI_OP_LENGTH, DMI_OP_WRITE);
-	buf_set_u64((unsigned char *)buf, DTM_DMI_DATA_OFFSET, DTM_DMI_DATA_LENGTH, d);
-	buf_set_u64((unsigned char *)buf, DTM_DMI_ADDRESS_OFFSET, info->abits, a);
+	buf_set_u32(buf, DTM_DMI_OP_OFFSET, DTM_DMI_OP_LENGTH, DMI_OP_WRITE);
+	buf_set_u32(buf, DTM_DMI_DATA_OFFSET, DTM_DMI_DATA_LENGTH, d);
+	buf_set_u32(buf, DTM_DMI_ADDRESS_OFFSET, info->abits, a);
 }
 
-static void riscv013_fill_dmi_read(struct target *target, char *buf, uint64_t a)
+static void riscv013_fill_dmi_read(const struct target *target, uint8_t *buf, uint32_t a)
 {
 	RISCV013_INFO(info);
-	buf_set_u64((unsigned char *)buf, DTM_DMI_OP_OFFSET, DTM_DMI_OP_LENGTH, DMI_OP_READ);
-	buf_set_u64((unsigned char *)buf, DTM_DMI_DATA_OFFSET, DTM_DMI_DATA_LENGTH, 0);
-	buf_set_u64((unsigned char *)buf, DTM_DMI_ADDRESS_OFFSET, info->abits, a);
+	buf_set_u32(buf, DTM_DMI_OP_OFFSET, DTM_DMI_OP_LENGTH, DMI_OP_READ);
+	buf_set_u32(buf, DTM_DMI_DATA_OFFSET, DTM_DMI_DATA_LENGTH, 0);
+	buf_set_u32(buf, DTM_DMI_ADDRESS_OFFSET, info->abits, a);
 }
 
-static void riscv013_fill_dm_nop(struct target *target, char *buf)
+static void riscv013_fill_dm_nop(const struct target *target, uint8_t *buf)
 {
 	RISCV013_INFO(info);
-	buf_set_u64((unsigned char *)buf, DTM_DMI_OP_OFFSET, DTM_DMI_OP_LENGTH, DMI_OP_NOP);
-	buf_set_u64((unsigned char *)buf, DTM_DMI_DATA_OFFSET, DTM_DMI_DATA_LENGTH, 0);
-	buf_set_u64((unsigned char *)buf, DTM_DMI_ADDRESS_OFFSET, info->abits, 0);
+	buf_set_u32(buf, DTM_DMI_OP_OFFSET, DTM_DMI_OP_LENGTH, DMI_OP_NOP);
+	buf_set_u32(buf, DTM_DMI_DATA_OFFSET, DTM_DMI_DATA_LENGTH, 0);
+	buf_set_u32(buf, DTM_DMI_ADDRESS_OFFSET, info->abits, 0);
 }
 
-static int riscv013_get_dmi_scan_length(struct target *target)
+static unsigned int riscv013_get_dmi_address_bits(const struct target *target)
 {
 	RISCV013_INFO(info);
-	return info->abits + DTM_DMI_DATA_LENGTH + DTM_DMI_OP_LENGTH;
+	return info->abits;
 }
 
 /* Helper Functions. */

src/target/riscv/riscv.c

@@ -5890,28 +5890,28 @@ int riscv_execute_progbuf(struct target *target, uint32_t *cmderr)
 	return r->execute_progbuf(target, cmderr);
 }
 
-void riscv_fill_dmi_write(struct target *target, char *buf, uint64_t a, uint32_t d)
+void riscv_fill_dmi_write(const struct target *target, uint8_t *buf, uint32_t a, uint32_t d)
 {
 	RISCV_INFO(r);
 	r->fill_dmi_write(target, buf, a, d);
 }
 
-void riscv_fill_dmi_read(struct target *target, char *buf, uint64_t a)
+void riscv_fill_dmi_read(const struct target *target, uint8_t *buf, uint32_t a)
 {
 	RISCV_INFO(r);
 	r->fill_dmi_read(target, buf, a);
 }
 
-void riscv_fill_dm_nop(struct target *target, char *buf)
+void riscv_fill_dm_nop(const struct target *target, uint8_t *buf)
 {
 	RISCV_INFO(r);
 	r->fill_dm_nop(target, buf);
 }
 
-int riscv_get_dmi_scan_length(struct target *target)
+unsigned int riscv_get_dmi_address_bits(const struct target *target)
 {
 	RISCV_INFO(r);
-	return r->get_dmi_scan_length(target);
+	return r->get_dmi_address_bits(target);
 }
 
 static int check_if_trigger_exists(struct target *target, unsigned int index)

src/target/riscv/riscv.h

@@ -125,6 +125,9 @@ typedef struct {
 #define DTM_DTMCS_VERSION_UNKNOWN ((unsigned int)-1)
 #define RISCV_TINFO_VERSION_UNKNOWN (-1)
 
+#define RISCV013_DTMCS_ABITS_MIN 7
+#define RISCV013_DTMCS_ABITS_MAX 32
+
 struct reg_name_table {
 	unsigned int num_entries;
 	char **reg_names;
@@ -275,10 +278,10 @@ struct riscv_info {
 	riscv_insn_t (*read_progbuf)(struct target *target, unsigned int index);
 	int (*execute_progbuf)(struct target *target, uint32_t *cmderr);
 	int (*invalidate_cached_progbuf)(struct target *target);
-	int (*get_dmi_scan_length)(struct target *target);
-	void (*fill_dmi_write)(struct target *target, char *buf, uint64_t a, uint32_t d);
-	void (*fill_dmi_read)(struct target *target, char *buf, uint64_t a);
-	void (*fill_dm_nop)(struct target *target, char *buf);
+	unsigned int (*get_dmi_address_bits)(const struct target *target);
+	void (*fill_dmi_write)(const struct target *target, uint8_t *buf, uint32_t a, uint32_t d);
+	void (*fill_dmi_read)(const struct target *target, uint8_t *buf, uint32_t a);
+	void (*fill_dm_nop)(const struct target *target, uint8_t *buf);
 
 	int (*authdata_read)(struct target *target, uint32_t *value, unsigned int index);
 	int (*authdata_write)(struct target *target, uint32_t value, unsigned int index);
@@ -462,10 +465,10 @@ riscv_insn_t riscv_read_progbuf(struct target *target, int index);
 int riscv_write_progbuf(struct target *target, int index, riscv_insn_t insn);
 int riscv_execute_progbuf(struct target *target, uint32_t *cmderr);
 
-void riscv_fill_dm_nop(struct target *target, char *buf);
-void riscv_fill_dmi_write(struct target *target, char *buf, uint64_t a, uint32_t d);
-void riscv_fill_dmi_read(struct target *target, char *buf, uint64_t a);
-int riscv_get_dmi_scan_length(struct target *target);
+void riscv_fill_dm_nop(const struct target *target, uint8_t *buf);
+void riscv_fill_dmi_write(const struct target *target, uint8_t *buf, uint32_t a, uint32_t d);
+void riscv_fill_dmi_read(const struct target *target, uint8_t *buf, uint32_t a);
+unsigned int riscv_get_dmi_address_bits(const struct target *target);
 
 uint32_t riscv_get_dmi_address(const struct target *target, uint32_t dm_address);