target/riscv: implement abstract command cache

Implemented cache of unsupported abstract commands.
It's purpose is to replace set of caching variables to one.
So this commit provides one ac_not_supported_cache
instead of abstract_read_csr_supported,
abstract_write_csr_supported, abstract_read_fpr_supported,
abstract_write_fpr_supported, has_aampostincrement.

Dropped check for buggy aampostincrement

Fixes #1232

Change-Id: I75cae1481841f2cd0393d6cc80f0d913fbe34294
Signed-off-by: Farid Khaydari <[email protected]>

Author: fk-sc

src/target/riscv/riscv-013.c

@@ -145,6 +145,60 @@ typedef struct {
 	struct target *target;
 } target_list_t;
 
+struct ac_cache {
+	uint32_t *commands;
+	size_t size;
+};
+
+static int ac_cache_elem_comparator(const void *lhs, const void *rhs)
+{
+	return *(const uint32_t *)rhs - *(const uint32_t *)lhs;
+}
+
+static struct ac_cache ac_cache_construct(void)
+{
+	struct ac_cache cache = {
+		cache.commands = NULL,
+		cache.size = 0,
+	};
+	return cache;
+}
+
+static void ac_cache_free(struct ac_cache *cache)
+{
+	free(cache->commands);
+	cache->commands = NULL;
+	cache->size = 0;
+}
+
+static void ac_cache_insert(struct ac_cache *cache, uint32_t command)
+{
+	assert(cache);
+
+	size_t old_size = cache->size;
+	size_t new_size = old_size + 1;
+	size_t entry_size = sizeof(*cache->commands);
+
+	uint32_t *commands = realloc(cache->commands, new_size * entry_size);
+	if (!commands) {
+		LOG_ERROR("Reallocation to %zu bytes failed", new_size);
+		return;
+	}
+
+	commands[old_size] = command;
+	cache->commands = commands;
+	cache->size = new_size;
+
+	qsort(cache->commands, cache->size, entry_size,
+			ac_cache_elem_comparator);
+}
+
+static bool ac_cache_contains(const struct ac_cache *cache, uint32_t command)
+{
+	return bsearch(&command, cache->commands, cache->size,
+			sizeof(*cache->commands), ac_cache_elem_comparator);
+}
+
 typedef struct {
 	/* The indexed used to address this hart in its DM. */
 	unsigned int index;
@@ -175,12 +229,7 @@ typedef struct {
 	 */
 	struct riscv_scan_delays learned_delays;
 
-	bool abstract_read_csr_supported;
-	bool abstract_write_csr_supported;
-	bool abstract_read_fpr_supported;
-	bool abstract_write_fpr_supported;
-
-	yes_no_maybe_t has_aampostincrement;
+	struct ac_cache ac_not_supported_cache;
 
 	/* Some fields from hartinfo. */
 	uint8_t datasize;
@@ -685,6 +734,11 @@ int riscv013_execute_abstract_command(struct target *target, uint32_t command,
 	res = abstract_cmd_batch_check_and_clear_cmderr(target, batch,
 			abstractcs_read_key, cmderr);
 cleanup:
+	if (res != ERROR_OK && *cmderr == CMDERR_NOT_SUPPORTED) {
+		LOG_TARGET_DEBUG(target, "Command 0x%" PRIx32
+				"is cached as not supported, bailing out", command);
+		ac_cache_insert(&get_info(target)->ac_not_supported_cache, command);
+	}
 	riscv_batch_free(batch);
 	return res;
 }
@@ -834,33 +888,22 @@ static int register_read_abstract_with_size(struct target *target,
 {
 	RISCV013_INFO(info);
 
-	if (number >= GDB_REGNO_FPR0 && number <= GDB_REGNO_FPR31 &&
-			!info->abstract_read_fpr_supported)
-		return ERROR_FAIL;
-	if (number >= GDB_REGNO_CSR0 && number <= GDB_REGNO_CSR4095 &&
-			!info->abstract_read_csr_supported)
-		return ERROR_FAIL;
 	/* The spec doesn't define abstract register numbers for vector registers. */
 	if (number >= GDB_REGNO_V0 && number <= GDB_REGNO_V31)
 		return ERROR_FAIL;
 
 	uint32_t command = riscv013_access_register_command(target, number, size,
 			AC_ACCESS_REGISTER_TRANSFER);
+	if (ac_cache_contains(&info->ac_not_supported_cache, command)) {
+		LOG_TARGET_DEBUG(target, "Command is cached as not supported, bailing out");
+		LOG_DEBUG_REG(target, AC_ACCESS_REGISTER, command);
+		return ERROR_FAIL;
+	}
 
 	uint32_t cmderr;
 	int result = riscv013_execute_abstract_command(target, command, &cmderr);
-	if (result != ERROR_OK) {
-		if (cmderr == CMDERR_NOT_SUPPORTED) {
-			if (number >= GDB_REGNO_FPR0 && number <= GDB_REGNO_FPR31) {
-				info->abstract_read_fpr_supported = false;
-				LOG_TARGET_INFO(target, "Disabling abstract command reads from FPRs.");
-			} else if (number >= GDB_REGNO_CSR0 && number <= GDB_REGNO_CSR4095) {
-				info->abstract_read_csr_supported = false;
-				LOG_TARGET_INFO(target, "Disabling abstract command reads from CSRs.");
-			}
-		}
+	if (result != ERROR_OK)
 		return result;
-	}
 
 	if (value)
 		return read_abstract_arg(target, value, 0, size);
@@ -885,17 +928,16 @@ static int register_write_abstract(struct target *target, enum gdb_regno number,
 	if (!dm)
 		return ERROR_FAIL;
 
-	if (number >= GDB_REGNO_FPR0 && number <= GDB_REGNO_FPR31 &&
-			!info->abstract_write_fpr_supported)
-		return ERROR_FAIL;
-	if (number >= GDB_REGNO_CSR0 && number <= GDB_REGNO_CSR4095 &&
-			!info->abstract_write_csr_supported)
-		return ERROR_FAIL;
-
 	const unsigned int size_bits = register_size(target, number);
 	const uint32_t command = riscv013_access_register_command(target, number, size_bits,
 			AC_ACCESS_REGISTER_TRANSFER |
 			AC_ACCESS_REGISTER_WRITE);
+	if (ac_cache_contains(&info->ac_not_supported_cache, command)) {
+		LOG_TARGET_DEBUG(target, "Command is cached as not supported, bailing out");
+		LOG_DEBUG_REG(target, AC_ACCESS_REGISTER, command);
+		return ERROR_FAIL;
+	}
+
 	LOG_DEBUG_REG(target, AC_ACCESS_REGISTER, command);
 	assert(size_bits % 32 == 0);
 	const unsigned int size_in_words = size_bits / 32;
@@ -916,16 +958,10 @@ static int register_write_abstract(struct target *target, enum gdb_regno number,
 	res = abstract_cmd_batch_check_and_clear_cmderr(target, batch,
 			abstractcs_read_key, &cmderr);
 
-	if (res != ERROR_OK) {
-		if (cmderr == CMDERR_NOT_SUPPORTED) {
-			if (number >= GDB_REGNO_FPR0 && number <= GDB_REGNO_FPR31) {
-				info->abstract_write_fpr_supported = false;
-				LOG_TARGET_INFO(target, "Disabling abstract command writes to FPRs.");
-			} else if (number >= GDB_REGNO_CSR0 && number <= GDB_REGNO_CSR4095) {
-				info->abstract_write_csr_supported = false;
-				LOG_TARGET_INFO(target, "Disabling abstract command writes to CSRs.");
-			}
-		}
+	if (res != ERROR_OK && cmderr == CMDERR_NOT_SUPPORTED) {
+		LOG_TARGET_DEBUG(target, "Caching the command as not supported");
+		LOG_DEBUG_REG(target, AC_ACCESS_REGISTER, command);
+		ac_cache_insert(&info->ac_not_supported_cache, command);
 	}
 cleanup:
 	riscv_batch_free(batch);
@@ -1709,6 +1745,10 @@ static void deinit_target(struct target *target)
 	if (!info)
 		return;
 
+	riscv013_info_t *vsinfo = info->version_specific;
+	if (vsinfo)
+		ac_cache_free(&vsinfo->ac_not_supported_cache);
+
 	riscv013_dm_free(target);
 
 	free(info->version_specific);
@@ -2819,17 +2859,7 @@ static int init_target(struct command_context *cmd_ctx,
 	info->progbufsize = -1;
 	reset_learned_delays(target);
 
-	/* Assume all these abstract commands are supported until we learn
-	 * otherwise.
-	 * TODO: The spec allows eg. one CSR to be able to be accessed abstractly
-	 * while another one isn't. We don't track that this closely here, but in
-	 * the future we probably should. */
-	info->abstract_read_csr_supported = true;
-	info->abstract_write_csr_supported = true;
-	info->abstract_read_fpr_supported = true;
-	info->abstract_write_fpr_supported = true;
-
-	info->has_aampostincrement = YNM_MAYBE;
+	info->ac_not_supported_cache = ac_cache_construct();
 
 	return ERROR_OK;
 }
@@ -3720,15 +3750,19 @@ read_memory_abstract(struct target *target, const riscv_mem_access_args_t args)
 	assert(riscv_mem_access_is_read(args));
 
 	RISCV013_INFO(info);
-	bool use_aampostincrement = info->has_aampostincrement != YNM_NO;
-
 	memset(args.read_buffer, 0, args.count * args.size);
 
 	/* Convert the size (bytes) to width (bits) */
 	unsigned int width = args.size << 3;
 
 	/* Create the command (physical address, postincrement, read) */
-	uint32_t command = access_memory_command(target, false, width, use_aampostincrement, false);
+	uint32_t command = access_memory_command(target, false, width,
+			/* postincrement = */ true, /* is_write= */ false);
+	bool use_aampostincrement =
+			!ac_cache_contains(&info->ac_not_supported_cache, command);
+	if (!use_aampostincrement)
+		command = access_memory_command(target, false, width,
+				/* postincrement = */ false, /* is_write= */ false);
 
 	/* Execute the reads */
 	uint8_t *p = args.read_buffer;
@@ -3749,33 +3783,14 @@ read_memory_abstract(struct target *target, const riscv_mem_access_args_t args)
 		/* Execute the command */
 		uint32_t cmderr;
 		result = riscv013_execute_abstract_command(target, command, &cmderr);
-
-		/* TODO: we need to modify error handling here. */
-		/* NOTE: in case of timeout cmderr is set to CMDERR_NONE */
-		if (info->has_aampostincrement == YNM_MAYBE) {
-			if (result == ERROR_OK) {
-				/* Safety: double-check that the address was really auto-incremented */
-				riscv_reg_t new_address;
-				result = read_abstract_arg(target, &new_address, 1, riscv_xlen(target));
-				if (result != ERROR_OK)
-					return mem_access_result(MEM_ACCESS_FAILED_DM_ACCESS_FAILED);
-
-				if (new_address == args.address + args.size) {
-					LOG_TARGET_DEBUG(target, "aampostincrement is supported on this target.");
-					info->has_aampostincrement = YNM_YES;
-				} else {
-					LOG_TARGET_WARNING(target, "Buggy aampostincrement! Address not incremented correctly.");
-					info->has_aampostincrement = YNM_NO;
-				}
-			} else {
-				/* Try the same access but with postincrement disabled. */
-				command = access_memory_command(target, false, width, false, false);
-				result = riscv013_execute_abstract_command(target, command, &cmderr);
-				if (result == ERROR_OK) {
-					LOG_TARGET_DEBUG(target, "aampostincrement is not supported on this target.");
-					info->has_aampostincrement = YNM_NO;
-				}
-			}
+		if (use_aampostincrement && result != ERROR_OK &&
+					cmderr == CMDERR_NOT_SUPPORTED) {
+			LOG_TARGET_DEBUG(target, "aampostincrement is not supported on this target.");
+			use_aampostincrement = false;
+
+			/* Try the same access but with postincrement disabled. */
+			command = access_memory_command(target, false, width, false, false);
+			result = riscv013_execute_abstract_command(target, command, &cmderr);
 		}
 
 		/* TODO:
@@ -3791,7 +3806,7 @@ read_memory_abstract(struct target *target, const riscv_mem_access_args_t args)
 			return mem_access_result(MEM_ACCESS_FAILED_DM_ACCESS_FAILED);
 		buf_set_u64(p, 0, 8 * args.size, value);
 
-		if (info->has_aampostincrement == YNM_YES)
+		if (use_aampostincrement)
 			updateaddr = false;
 		p += args.size;
 	}
@@ -3811,13 +3826,18 @@ write_memory_abstract(struct target *target, const riscv_mem_access_args_t args)
 
 	RISCV013_INFO(info);
 	int result = ERROR_OK;
-	bool use_aampostincrement = info->has_aampostincrement != YNM_NO;
 
 	/* Convert the size (bytes) to width (bits) */
 	unsigned int width = args.size << 3;
 
 	/* Create the command (physical address, postincrement, write) */
-	uint32_t command = access_memory_command(target, false, width, use_aampostincrement, true);
+	uint32_t command = access_memory_command(target, false, width,
+			/* postincrement = */ true, /* is_write = */ true);
+	bool use_aampostincrement =
+			!ac_cache_contains(&info->ac_not_supported_cache, command);
+	if (!use_aampostincrement)
+		command = access_memory_command(target, false, width,
+				/* postincrement = */ false, /* is_write= */ false);
 
 	/* Execute the writes */
 	const uint8_t *p = args.write_buffer;
@@ -3844,33 +3864,14 @@ write_memory_abstract(struct target *target, const riscv_mem_access_args_t args)
 		/* Execute the command */
 		uint32_t cmderr;
 		result = riscv013_execute_abstract_command(target, command, &cmderr);
-
-		/* TODO: we need to modify error handling here. */
-		/* NOTE: in case of timeout cmderr is set to CMDERR_NONE */
-		if (info->has_aampostincrement == YNM_MAYBE) {
-			if (result == ERROR_OK) {
-				/* Safety: double-check that the address was really auto-incremented */
-				riscv_reg_t new_address;
-				result = read_abstract_arg(target, &new_address, 1, riscv_xlen(target));
-				if (result != ERROR_OK)
-					return mem_access_result(MEM_ACCESS_FAILED_DM_ACCESS_FAILED);
-
-				if (new_address == args.address + args.size) {
-					LOG_TARGET_DEBUG(target, "aampostincrement is supported on this target.");
-					info->has_aampostincrement = YNM_YES;
-				} else {
-					LOG_TARGET_WARNING(target, "Buggy aampostincrement! Address not incremented correctly.");
-					info->has_aampostincrement = YNM_NO;
-				}
-			} else {
-				/* Try the same access but with postincrement disabled. */
-				command = access_memory_command(target, false, width, false, true);
-				result = riscv013_execute_abstract_command(target, command, &cmderr);
-				if (result == ERROR_OK) {
-					LOG_TARGET_DEBUG(target, "aampostincrement is not supported on this target.");
-					info->has_aampostincrement = YNM_NO;
-				}
-			}
+		if (use_aampostincrement && result != ERROR_OK &&
+					cmderr == CMDERR_NOT_SUPPORTED) {
+			LOG_TARGET_DEBUG(target, "aampostincrement is not supported on this target.");
+			use_aampostincrement = false;
+
+			/* Try the same access but with postincrement disabled. */
+			command = access_memory_command(target, false, width, false, true);
+			result = riscv013_execute_abstract_command(target, command, &cmderr);
 		}
 
 		/* TODO:
@@ -3879,7 +3880,7 @@ write_memory_abstract(struct target *target, const riscv_mem_access_args_t args)
 		if (result != ERROR_OK)
 			return mem_access_result(MEM_ACCESS_SKIPPED_ABSTRACT_ACCESS_CMDERR);
 
-		if (info->has_aampostincrement == YNM_YES)
+		if (use_aampostincrement)
 			updateaddr = false;
 		p += args.size;
 	}