Rtos riscv (#350)

* Implement riscv_get_thread_reg().

This is necessary because riscv_get_gdb_reg_list() now reads all
registers, which ended up causing `-rtos riscv` to read all registers
whenever one was requested (because the register cache is wiped every
time we switch to a different hart).

CustomRegisterTest went from 1329s to 106s.

Change-Id: I8e9918b7a532d44bca927f67aae5ac34954a8d32

* Also implement riscv_set_reg().

Now all the `-rtos riscv` tests pass again, at regular speed.

Change-Id: I55164224672d9dcc9eb4d1184b47258ff3c2cff1

* Better error messages.

Change-Id: I4125f9a54750d9d0ee22c4fa84b9dd3f5af203f5

* Add target_get_gdb_reg_list_noread().

Being explicit about what's expected gets `-rtos riscv` back to `-rtos
hwthread` time.

Change-Id: I6e57390c2fe79b5e6799bfda980d89697e2e29f7

* Revert a change I made that has no effect.

I don't understand exactly what all this test protects against, and I
shouldn't change it unless I do.

Change-Id: Ib329d4e34d65d2b38559b89b7afb3678f439ad2c

src/rtos/hwthread.c

@@ -270,15 +270,22 @@ static int hwthread_get_thread_reg(struct rtos *rtos, int64_t thread_id,
 	struct target *target = rtos->target;
 
 	struct target *curr = find_thread(target, thread_id);
-	if (curr == NULL)
+	if (curr == NULL) {
+		LOG_ERROR("Couldn't find RTOS thread for id %" PRId64 ".", thread_id);
 		return ERROR_FAIL;
+	}
 
-	if (!target_was_examined(curr))
+	if (!target_was_examined(curr)) {
+		LOG_ERROR("Target %d hasn't been examined yet.", curr->coreid);
 		return ERROR_FAIL;
+	}
 
 	struct reg *reg = register_get_by_number(curr->reg_cache, reg_num, true);
-	if (!reg)
+	if (!reg) {
+		LOG_ERROR("Couldn't find register %d in thread %" PRId64 ".", reg_num,
+				thread_id);
 		return ERROR_FAIL;
+	}
 
 	if (reg->type->get(reg) != ERROR_OK)
 		return ERROR_FAIL;

src/rtos/riscv_debug.c

@@ -3,9 +3,11 @@
 #endif
 
 #include "riscv_debug.h"
+#include "target/register.h"
 #include "target/target.h"
 #include "target/riscv/riscv.h"
 #include "server/gdb_server.h"
+#include "helper/binarybuffer.h"
 
 static int riscv_gdb_thread_packet(struct connection *connection, const char *packet, int packet_size);
 static int riscv_gdb_v_packet(struct connection *connection, const char *packet, int packet_size);
@@ -174,6 +176,7 @@ static int riscv_gdb_thread_packet(struct connection *connection, const char *pa
 			break;
 		default:
 			riscv_set_rtos_hartid(target, tid - 1);
+			rtos->current_threadid = tid;
 			break;
 		}
 
@@ -270,6 +273,27 @@ static int riscv_gdb_v_packet(struct connection *connection, const char *packet,
 	return GDB_THREAD_PACKET_NOT_CONSUMED;
 }
 
+static int riscv_get_thread_reg(struct rtos *rtos, int64_t thread_id,
+		uint32_t reg_num, struct rtos_reg *rtos_reg)
+{
+	LOG_DEBUG("thread_id=%" PRId64 ", reg_num=%d", thread_id, reg_num);
+
+	struct target *target = rtos->target;
+	struct reg *reg = register_get_by_number(target->reg_cache, reg_num, true);
+	if (!reg)
+		return ERROR_FAIL;
+
+	uint64_t reg_value = 0;
+	if (riscv_get_register_on_hart(rtos->target, &reg_value, thread_id - 1,
+				reg_num) != ERROR_OK)
+		return ERROR_FAIL;
+
+	buf_set_u64(rtos_reg->value, 0, 64, reg_value);
+	rtos_reg->number = reg->number;
+	rtos_reg->size = reg->size;
+	return ERROR_OK;
+}
+
 static int riscv_get_thread_reg_list(struct rtos *rtos, int64_t thread_id,
 		struct rtos_reg **reg_list, int *num_regs)
 {
@@ -277,11 +301,10 @@ static int riscv_get_thread_reg_list(struct rtos *rtos, int64_t thread_id,
 
 	/* We return just the GPRs here. */
 
-	*num_regs = 32;
+	*num_regs = 33;
 	int xlen = riscv_xlen_of_hart(rtos->target, thread_id - 1);
 
 	*reg_list = calloc(*num_regs, sizeof(struct rtos_reg));
-	*reg_list = 0;
 	for (int i = 0; i < *num_regs; ++i) {
 		uint64_t reg_value;
 		if (riscv_get_register_on_hart(rtos->target, &reg_value, thread_id - 1,
@@ -290,18 +313,25 @@ static int riscv_get_thread_reg_list(struct rtos *rtos, int64_t thread_id,
 
 		(*reg_list)[i].number = i;
 		(*reg_list)[i].size = xlen;
-		(*reg_list)[i].value[0] = reg_value & 0xff;
+		buf_set_u64((*reg_list)[i].value, 0, 64, reg_value);
-		(*reg_list)[i].value[1] = (reg_value >> 8) & 0xff;
-		(*reg_list)[i].value[2] = (reg_value >> 16) & 0xff;
-		(*reg_list)[i].value[3] = (reg_value >> 24) & 0xff;
-		(*reg_list)[i].value[4] = (reg_value >> 32) & 0xff;
-		(*reg_list)[i].value[5] = (reg_value >> 40) & 0xff;
-		(*reg_list)[i].value[6] = (reg_value >> 48) & 0xff;
-		(*reg_list)[i].value[7] = (reg_value >> 56) & 0xff;
 	}
 	return JIM_OK;
 }
 
+static int riscv_set_reg(struct rtos *rtos, uint32_t reg_num,
+		uint8_t *reg_value)
+{
+	struct target *target = rtos->target;
+	struct reg *reg = register_get_by_number(target->reg_cache, reg_num, true);
+	if (!reg)
+		return ERROR_FAIL;
+
+	int hartid = rtos->current_threadid - 1;
+	uint64_t value = buf_get_u64(reg_value, 0, reg->size);
+
+	return riscv_set_register_on_hart(target, hartid, reg_num, value);
+}
+
 static int riscv_get_symbol_list_to_lookup(symbol_table_elem_t *symbol_list[])
 {
 	*symbol_list = calloc(1, sizeof(symbol_table_elem_t));
@@ -315,6 +345,8 @@ const struct rtos_type riscv_rtos = {
 	.detect_rtos = riscv_detect_rtos,
 	.create = riscv_create_rtos,
 	.update_threads = riscv_update_threads,
+	.get_thread_reg = riscv_get_thread_reg,
 	.get_thread_reg_list = riscv_get_thread_reg_list,
 	.get_symbol_list_to_lookup = riscv_get_symbol_list_to_lookup,
+	.set_reg = riscv_set_reg,
 };

src/rtos/rtos.c

@@ -475,8 +475,8 @@ int rtos_get_gdb_reg(struct connection *connection, int reg_num)
 		struct rtos_reg *reg_list;
 		int num_regs;
 
-		LOG_DEBUG("RTOS: getting register %d for thread 0x%" PRIx64
+		LOG_DEBUG("getting register %d for thread 0x%" PRIx64
-				  ", target->rtos->current_thread=0x%" PRIx64 "\r\n",
+				  ", target->rtos->current_thread=0x%" PRIx64,
 										reg_num,
 										current_threadid,
 										target->rtos->current_thread);
@@ -487,15 +487,19 @@ int rtos_get_gdb_reg(struct connection *connection, int reg_num)
 			num_regs = 1;
 			retval = target->rtos->type->get_thread_reg(target->rtos,
 					current_threadid, reg_num, &reg_list[0]);
+			if (retval != ERROR_OK) {
+				LOG_ERROR("RTOS: failed to get register %d", reg_num);
+				return retval;
+			}
 		} else {
 			retval = target->rtos->type->get_thread_reg_list(target->rtos,
 					current_threadid,
 					&reg_list,
 					&num_regs);
-		}
+			if (retval != ERROR_OK) {
-		if (retval != ERROR_OK) {
+				LOG_ERROR("RTOS: failed to get register list");
-			LOG_ERROR("RTOS: failed to get register list");
+				return retval;
-			return retval;
+			}
 		}
 
 		for (int i = 0; i < num_regs; ++i) {

src/server/gdb_server.c

@@ -96,7 +96,7 @@ struct gdb_connection {
 	char *thread_list;
 };
 
-#if 1
+#if 0
 #define _DEBUG_GDB_IO_
 #endif
 
@@ -1307,7 +1307,7 @@ static int gdb_get_register_packet(struct connection *connection,
 	if ((target->rtos != NULL) && (ERROR_OK == rtos_get_gdb_reg(connection, reg_num)))
 		return ERROR_OK;
 
-	retval = target_get_gdb_reg_list(target, &reg_list, &reg_list_size,
+	retval = target_get_gdb_reg_list_noread(target, &reg_list, &reg_list_size,
 			REG_CLASS_ALL);
 	if (retval != ERROR_OK)
 		return gdb_error(connection, retval);
@@ -1367,7 +1367,7 @@ static int gdb_set_register_packet(struct connection *connection,
 		return ERROR_OK;
 	}
 
-	retval = target_get_gdb_reg_list(target, &reg_list, &reg_list_size,
+	retval = target_get_gdb_reg_list_noread(target, &reg_list, &reg_list_size,
 			REG_CLASS_ALL);
 	if (retval != ERROR_OK) {
 		free(bin_buf);
@@ -2221,7 +2221,7 @@ static int gdb_generate_target_description(struct target *target, char **tdesc_o
 
 	arch_defined_types = calloc(1, sizeof(char *));
 
-	retval = target_get_gdb_reg_list(target, &reg_list,
+	retval = target_get_gdb_reg_list_noread(target, &reg_list,
 			&reg_list_size, REG_CLASS_ALL);
 
 	if (retval != ERROR_OK) {
@@ -2409,7 +2409,7 @@ static int gdb_target_description_supported(struct target *target, int *supporte
 
 	char const *architecture = target_get_gdb_arch(target);
 
-	retval = target_get_gdb_reg_list(target, &reg_list,
+	retval = target_get_gdb_reg_list_noread(target, &reg_list,
 			&reg_list_size, REG_CLASS_ALL);
 	if (retval != ERROR_OK) {
 		LOG_ERROR("get register list failed");

src/target/riscv/riscv-013.c

@@ -1138,7 +1138,7 @@ static int register_write_direct(struct target *target, unsigned number,
 	RISCV013_INFO(info);
 	RISCV_INFO(r);
 
-	LOG_DEBUG("[%d] reg[0x%x] <- 0x%" PRIx64, riscv_current_hartid(target),
+	LOG_DEBUG("{%d} reg[0x%x] <- 0x%" PRIx64, riscv_current_hartid(target),
 			number, value);
 
 	int result = register_write_abstract(target, number, value,
@@ -1321,7 +1321,7 @@ static int register_read_direct(struct target *target, uint64_t *value, uint32_t
 	}
 
 	if (result == ERROR_OK) {
-		LOG_DEBUG("[%d] reg[0x%x] = 0x%" PRIx64, riscv_current_hartid(target),
+		LOG_DEBUG("{%d} reg[0x%x] = 0x%" PRIx64, riscv_current_hartid(target),
 				number, *value);
 	}
 
@@ -2988,7 +2988,7 @@ static enum riscv_halt_reason riscv013_halt_reason(struct target *target)
 		 * already set when we connected. Force enumeration now, which has the
 		 * side effect of clearing any triggers we did not set. */
 		riscv_enumerate_triggers(target);
-		LOG_DEBUG("[%d] halted because of trigger", target->coreid);
+		LOG_DEBUG("{%d} halted because of trigger", target->coreid);
 		return RISCV_HALT_TRIGGER;
 	case CSR_DCSR_CAUSE_STEP:
 		return RISCV_HALT_SINGLESTEP;

src/target/riscv/riscv.c

@@ -919,13 +919,13 @@ static int riscv_write_memory(struct target *target, target_addr_t address,
 	return tt->write_memory(target, address, size, count, buffer);
 }
 
-static int riscv_get_gdb_reg_list(struct target *target,
+static int riscv_get_gdb_reg_list_internal(struct target *target,
 		struct reg **reg_list[], int *reg_list_size,
-		enum target_register_class reg_class)
+		enum target_register_class reg_class, bool read)
 {
 	RISCV_INFO(r);
-	LOG_DEBUG("reg_class=%d", reg_class);
+	LOG_DEBUG("rtos_hartid=%d, current_hartid=%d, reg_class=%d, read=%d",
-	LOG_DEBUG("rtos_hartid=%d current_hartid=%d", r->rtos_hartid, r->current_hartid);
+			r->rtos_hartid, r->current_hartid, reg_class, read);
 
 	if (!target->reg_cache) {
 		LOG_ERROR("Target not initialized. Return ERROR_FAIL.");
@@ -951,24 +951,36 @@ static int riscv_get_gdb_reg_list(struct target *target,
 	if (!*reg_list)
 		return ERROR_FAIL;
 
-	bool read = true;
 	for (int i = 0; i < *reg_list_size; i++) {
 		assert(!target->reg_cache->reg_list[i].valid ||
 				target->reg_cache->reg_list[i].size > 0);
 		(*reg_list)[i] = &target->reg_cache->reg_list[i];
 		if (read && !target->reg_cache->reg_list[i].valid) {
-			/* This function gets called from
-			 * gdb_target_description_supported(), and we end up failing in
-			 * that case. Allow failures for now. */
 			if (target->reg_cache->reg_list[i].type->get(
 						&target->reg_cache->reg_list[i]) != ERROR_OK)
-				read = false;
+				return ERROR_FAIL;
 		}
 	}
 
 	return ERROR_OK;
 }
 
+static int riscv_get_gdb_reg_list_noread(struct target *target,
+		struct reg **reg_list[], int *reg_list_size,
+		enum target_register_class reg_class)
+{
+	return riscv_get_gdb_reg_list_internal(target, reg_list, reg_list_size,
+			reg_class, false);
+}
+
+static int riscv_get_gdb_reg_list(struct target *target,
+		struct reg **reg_list[], int *reg_list_size,
+		enum target_register_class reg_class)
+{
+	return riscv_get_gdb_reg_list_internal(target, reg_list, reg_list_size,
+			reg_class, true);
+}
+
 static int riscv_arch_state(struct target *target)
 {
 	struct target_type *tt = get_target_type(target);
@@ -1960,6 +1972,7 @@ struct target_type riscv_target = {
 	.checksum_memory = riscv_checksum_memory,
 
 	.get_gdb_reg_list = riscv_get_gdb_reg_list,
+	.get_gdb_reg_list_noread = riscv_get_gdb_reg_list_noread,
 
 	.add_breakpoint = riscv_add_breakpoint,
 	.remove_breakpoint = riscv_remove_breakpoint,
@@ -2140,6 +2153,7 @@ void riscv_invalidate_register_cache(struct target *target)
 {
 	RISCV_INFO(r);
 
+	LOG_DEBUG("[%d]", target->coreid);
 	register_cache_invalidate(target->reg_cache);
 	for (size_t i = 0; i < target->reg_cache->num_regs; ++i) {
 		struct reg *reg = &target->reg_cache->reg_list[i];
@@ -2196,7 +2210,7 @@ int riscv_set_register_on_hart(struct target *target, int hartid,
 		enum gdb_regno regid, uint64_t value)
 {
 	RISCV_INFO(r);
-	LOG_DEBUG("[%d] %s <- %" PRIx64, hartid, gdb_regno_name(regid), value);
+	LOG_DEBUG("{%d} %s <- %" PRIx64, hartid, gdb_regno_name(regid), value);
 	assert(r->set_register);
 	return r->set_register(target, hartid, regid, value);
 }
@@ -2213,21 +2227,16 @@ int riscv_get_register_on_hart(struct target *target, riscv_reg_t *value,
 {
 	RISCV_INFO(r);
 
-	if (hartid != riscv_current_hartid(target))
-		riscv_invalidate_register_cache(target);
-
 	struct reg *reg = &target->reg_cache->reg_list[regid];
-	if (reg && reg->valid) {
+
+	if (reg && reg->valid && hartid == riscv_current_hartid(target)) {
 		*value = buf_get_u64(reg->value, 0, reg->size);
 		return ERROR_OK;
 	}
 
 	int result = r->get_register(target, value, hartid, regid);
 
-	if (hartid != riscv_current_hartid(target))
+	LOG_DEBUG("{%d} %s: %" PRIx64, hartid, gdb_regno_name(regid), *value);
-		riscv_invalidate_register_cache(target);
-
-	LOG_DEBUG("[%d] %s: %" PRIx64, hartid, gdb_regno_name(regid), *value);
 	return result;
 }
 
@@ -2441,7 +2450,7 @@ static int register_get(struct reg *reg)
 			(reg->number >= GDB_REGNO_FPR0 && reg->number <= GDB_REGNO_FPR31) ||
 			reg->number == GDB_REGNO_PC)
 		reg->valid = true;
-	LOG_DEBUG("[%d,%d] read 0x%" PRIx64 " from %s (valid=%d)",
+	LOG_DEBUG("[%d]{%d} read 0x%" PRIx64 " from %s (valid=%d)",
 			target->coreid, riscv_current_hartid(target), value, reg->name,
 			reg->valid);
 	return ERROR_OK;
@@ -2454,7 +2463,7 @@ static int register_set(struct reg *reg, uint8_t *buf)
 
 	uint64_t value = buf_get_u64(buf, 0, reg->size);
 
-	LOG_DEBUG("[%d,%d] write 0x%" PRIx64 " to %s (valid=%d)",
+	LOG_DEBUG("[%d]{%d} write 0x%" PRIx64 " to %s (valid=%d)",
 			target->coreid, riscv_current_hartid(target), value, reg->name,
 			reg->valid);
 	struct reg *r = &target->reg_cache->reg_list[reg->number];

src/target/target.c

@@ -1223,6 +1223,17 @@ int target_get_gdb_reg_list(struct target *target,
 	return target->type->get_gdb_reg_list(target, reg_list, reg_list_size, reg_class);
 }
 
+int target_get_gdb_reg_list_noread(struct target *target,
+		struct reg **reg_list[], int *reg_list_size,
+		enum target_register_class reg_class)
+{
+	if (target->type->get_gdb_reg_list_noread &&
+			target->type->get_gdb_reg_list_noread(target, reg_list,
+				reg_list_size, reg_class) == ERROR_OK)
+		return ERROR_OK;
+	return target_get_gdb_reg_list(target, reg_list, reg_list_size, reg_class);
+}
+
 bool target_supports_gdb_connection(struct target *target)
 {
 	/*

src/target/target.h

@@ -496,6 +496,16 @@ int target_get_gdb_reg_list(struct target *target,
 		struct reg **reg_list[], int *reg_list_size,
 		enum target_register_class reg_class);
 
+/**
+ * Obtain the registers for GDB, but don't read register values from the
+ * target.
+ *
+ * This routine is a wrapper for target->type->get_gdb_reg_list_noread.
+ */
+int target_get_gdb_reg_list_noread(struct target *target,
+		struct reg **reg_list[], int *reg_list_size,
+		enum target_register_class reg_class);
+
 /**
  * Check if @a target allows GDB connections.
  *

src/target/target_type.h

@@ -111,6 +111,13 @@ struct target_type {
 	int (*get_gdb_reg_list)(struct target *target, struct reg **reg_list[],
 			int *reg_list_size, enum target_register_class reg_class);
 
+	/**
+	 * Same as get_gdb_reg_list, but doesn't read the register values.
+	 * */
+	int (*get_gdb_reg_list_noread)(struct target *target,
+			struct reg **reg_list[], int *reg_list_size,
+			enum target_register_class reg_class);
+
 	/* target memory access
 	* size: 1 = byte (8bit), 2 = half-word (16bit), 4 = word (32bit)
 	* count: number of items of <size>