Ensure read_memory() only reads each address once.

Previously it might read an address multiple times if an abstract
command took longer to execute than expected.

The new implementations reads from the target how far it has gotten
along reading memory, and resumes from there if cmderr=busy.

This ended up being a bigger change than I envisioned, but in the end it
deleted more lines than it added, so I'm happy. :-)

src/target/riscv/riscv-013.c

@@ -1248,6 +1248,31 @@ static int deassert_reset(struct target *target)
 	return ERROR_OK;
 }
 
+static void write_to_buf(uint8_t *buffer, uint64_t value, unsigned size)
+{
+	switch (size) {
+		case 8:
+			buffer[7] = value >> 56;
+			buffer[6] = value >> 48;
+			buffer[5] = value >> 40;
+			buffer[4] = value >> 32;
+		case 4:
+			buffer[3] = value >> 24;
+			buffer[2] = value >> 16;
+		case 2:
+			buffer[1] = value >> 8;
+		case 1:
+			buffer[0] = value;
+			break;
+		default:
+			assert(false);
+	}
+}
+
+/**
+ * Read the requested memory, taking care to execute every read exactly once,
+ * even if cmderr=busy is encountered.
+ */
 static int read_memory(struct target *target, target_addr_t address,
 		uint32_t size, uint32_t count, uint8_t *buffer)
 {
@@ -1314,6 +1339,9 @@ static int read_memory(struct target *target, target_addr_t address,
 		return ERROR_FAIL;
 	}
 
+	// Program has been executed once. d_addr contains address+size, and d_data
+	// contains *address.
+
 	/* The rest of this program is designed to be fast so it reads various
 	 * DMI registers directly. */
 	int d_data = (r_data - riscv_debug_buffer_addr(target)) / 4;
@@ -1325,15 +1353,17 @@ static int read_memory(struct target *target, target_addr_t address,
 	 * case we need to back off a bit and try again.  There's two
 	 * termination conditions to this loop: a non-BUSY error message, or
 	 * the data was all copied. */
-	riscv_addr_t cur_addr = address;
+	riscv_addr_t cur_addr = riscv_read_debug_buffer_x(target, d_addr);
 	riscv_addr_t fin_addr = address + (count * size);
-	bool this_is_last_read = false;
 	LOG_DEBUG("reading until final address 0x%" PRIx64, fin_addr);
-	while (!this_is_last_read) {
+	while (cur_addr < fin_addr) {
-		riscv_addr_t start = (cur_addr - address) / size;
+		// Invariant:
-		LOG_DEBUG("reading burst at address 0x%" TARGET_PRIxADDR
+		// d_data contains *addr
-				"; start=0x%"
+		// d_addr contains addr + size
-				TARGET_PRIxADDR, cur_addr, start);
+
+		unsigned start = (cur_addr - address) / size;
+		LOG_DEBUG("creating burst to read address 0x%" TARGET_PRIxADDR
+				" up to 0x%" TARGET_PRIxADDR "; start=0x%d", cur_addr, fin_addr, start);
 		assert(cur_addr >= address && cur_addr < fin_addr);
 		struct riscv_batch *batch = riscv_batch_alloc(
 			target,
@@ -1341,23 +1371,12 @@ static int read_memory(struct target *target, target_addr_t address,
 			info->dmi_busy_delay + info->ac_busy_delay);
 
 		size_t reads = 0;
-		size_t rereads = reads;
+		for (riscv_addr_t addr = cur_addr; addr < fin_addr; addr += size) {
-		for (riscv_addr_t i = start; i < count; ++i) {
+			size_t const index =
-			if (i == count - 1) {
+				riscv_batch_add_dmi_read(
-				// don't do actual read in this batch,
-				// we will do it later after we disable autoexec
-				//
-				// this is done to avoid reading more memory than requested
-				// which in some special cases(like reading stack located
-				// at the very top of RAM) may cause an exception
-				this_is_last_read = true;
-			} else {
-				size_t const index =
-					riscv_batch_add_dmi_read(
 						batch,
 						riscv013_debug_buffer_register(target, r_data));
-				assert(index == reads);
+			assert(index == reads);
-			}
 
 			reads++;
 			if (riscv_batch_full(batch))
@@ -1366,27 +1385,21 @@ static int read_memory(struct target *target, target_addr_t address,
 
 		riscv_batch_run(batch);
 
-		// Note that if the scan resulted in a Busy DMI response, it
+		// Wait for the target to finish performing the last abstract command,
-		// is this read to abstractcs that will cause the dmi_busy_delay
+		// and update our copy of cmderr.
-		// to be incremented if necessary. The loop condition above
-		// catches the case where no writes went through at all.
-
-		bool retry_batch_transaction = false;
 		uint32_t abstractcs = dmi_read(target, DMI_ABSTRACTCS);
 		while (get_field(abstractcs, DMI_ABSTRACTCS_BUSY))
 			abstractcs = dmi_read(target, DMI_ABSTRACTCS);
 		info->cmderr = get_field(abstractcs, DMI_ABSTRACTCS_CMDERR);
+
 		switch (info->cmderr) {
 		case CMDERR_NONE:
 			LOG_DEBUG("successful (partial?) memory read");
 			break;
 		case CMDERR_BUSY:
-			LOG_DEBUG("memory read resulted in busy response; "
+			LOG_DEBUG("memory read resulted in busy response");
-					"this_is_last_read=%d", this_is_last_read);
 			increase_ac_busy_delay(target);
 			riscv013_clear_abstract_error(target);
-			retry_batch_transaction = true;
-			riscv_batch_free(batch);
 			break;
 		default:
 			LOG_ERROR("error when reading memory, abstractcs=0x%08lx", (long)abstractcs);
@@ -1397,82 +1410,45 @@ static int read_memory(struct target *target, target_addr_t address,
 			riscv_batch_free(batch);
 			return ERROR_FAIL;
 		}
-		if (retry_batch_transaction) {
-			this_is_last_read = false;
 
-			switch (riscv_xlen(target)) {
+		// Figure out how far we managed to read.
-				case 64:
+		riscv_addr_t next_addr = riscv_read_debug_buffer_x(target, d_addr);
-					riscv013_write_debug_buffer(target, d_addr + 1, cur_addr >> 32);
+		LOG_DEBUG("Batch read [0x%" TARGET_PRIxADDR ", 0x%" TARGET_PRIxADDR
-				case 32:
+				"); reads=%d", cur_addr, next_addr, (unsigned) reads);
-					riscv013_write_debug_buffer(target, d_addr, cur_addr);
+		assert(next_addr >= address && next_addr <= fin_addr);
-					break;
+		assert(info->cmderr != CMDERR_NONE ||
-				default:
+				next_addr == cur_addr + reads * size);
-					LOG_ERROR("unknown XLEN %d", riscv_xlen(target));
-					return ERROR_FAIL;
-			}
 
-			if (riscv013_execute_debug_buffer(target) != ERROR_OK) {
+		// Now read whatever we got out of the batch.
-				uint32_t acs = dmi_read(target, DMI_ABSTRACTCS);
+		unsigned rereads = 0;
-				LOG_ERROR("failed to execute program, abstractcs=0x%08x", acs);
+		for (riscv_addr_t addr = cur_addr - size; addr < next_addr - size;
-				riscv013_clear_abstract_error(target);
+				addr += size) {
-				riscv_set_register(target, GDB_REGNO_S0, s0);
+			riscv_addr_t offset = addr - address;
-				riscv_set_register(target, GDB_REGNO_S1, s1);
-				LOG_ERROR("  exiting with ERROR_FAIL");
-				return ERROR_FAIL;
-			}
 
-			continue;
+			uint64_t dmi_out = riscv_batch_get_dmi_read(batch, rereads);
-		}
+			uint32_t value = get_field(dmi_out, DTM_DMI_DATA);
+			write_to_buf(buffer + offset, value, size);
 
-		for (size_t i = start; i < start + reads; ++i) {
-			riscv_addr_t offset = size*i;
-			riscv_addr_t t_addr = address + offset;
-			uint8_t *t_buffer = buffer + offset;
-			uint32_t value;
-
-			if (this_is_last_read && i == start + reads - 1) {
-				riscv013_set_autoexec(target, d_data, 0);
-
-				// Access debug buffer without executing a program. This
-				// address logic was taken from program.c.
-				int const off = (r_data -
-						riscv_debug_buffer_addr(program.target)) /
-					sizeof(program.debug_buffer[0]);
-				value = riscv_read_debug_buffer(target, off);
-			} else {
-				uint64_t dmi_out = riscv_batch_get_dmi_read(batch, rereads);
-				value = get_field(dmi_out, DTM_DMI_DATA);
-			}
-			
 			rereads++;
 
-			switch (size) {
+			LOG_DEBUG("M[0x%" TARGET_PRIxADDR "] reads 0x%08x", addr, value);
-			case 1:
-				t_buffer[0] = value;
-				break;
-			case 2:
-				t_buffer[0] = value;
-				t_buffer[1] = value >> 8;
-				break;
-			case 4:
-				t_buffer[0] = value;
-				t_buffer[1] = value >> 8;
-				t_buffer[2] = value >> 16;
-				t_buffer[3] = value >> 24;
-				break;
-			default:
-				LOG_ERROR("unsupported access size: %d", size);
-				return ERROR_FAIL;
-			}
-
-			LOG_DEBUG("M[0x%08lx] reads 0x%08x", (long)t_addr, value);
 		}
 		riscv_batch_free(batch);
 
-		cur_addr += reads * size;
+		cur_addr = next_addr;
 	}
 
 	riscv013_set_autoexec(target, d_data, 0);
+
+	// Read the last word.
+
+	// Access debug buffer without executing a program. This
+	// address logic was taken from program.c.
+	uint32_t value = riscv013_read_debug_buffer(target, d_data);
+	riscv_addr_t addr = cur_addr - size;
+	write_to_buf(buffer + addr - address, value, size);
+	LOG_DEBUG("M[0x%" TARGET_PRIxADDR "] reads 0x%08x", addr, value);
+
 	riscv_set_register(target, GDB_REGNO_S0, s0);
 	riscv_set_register(target, GDB_REGNO_S1, s1);
 	return ERROR_OK;