From ab97974d1b0d65bde3263596a27cfdbecda6c280 Mon Sep 17 00:00:00 2001 From: Farid Khaydari Date: Tue, 25 Mar 2025 13:07:36 +0300 Subject: [PATCH] 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: I9690d9d79e3d1f593b63740b989074dcf0285637 Signed-off-by: Farid Khaydari --- src/target/riscv/riscv-013.c | 275 ++++++++++++++++++++--------------- 1 file changed, 154 insertions(+), 121 deletions(-) diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c index 23c0aa62c4..af1c1eb77f 100644 --- a/src/target/riscv/riscv-013.c +++ b/src/target/riscv/riscv-013.c @@ -145,6 +145,66 @@ 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 *p_lhs, const void *p_rhs) +{ + uint32_t lhs = *(const uint32_t *)p_lhs; + uint32_t rhs = *(const uint32_t *)p_rhs; + if (lhs < rhs) + return -1; + if (lhs > rhs) + return 1; + return 0; +} + +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 * entry_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 +235,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; @@ -651,6 +706,30 @@ static int abstract_cmd_batch_check_and_clear_cmderr(struct target *target, return res; } +enum riscv_debug_reg_ordinal get_cmdtype(uint32_t command) +{ + switch (get_field(command, DM_COMMAND_CMDTYPE)) { + case 0: + return AC_ACCESS_REGISTER_ORDINAL; + case 1: + return AC_QUICK_ACCESS_ORDINAL; + case 2: + return AC_ACCESS_MEMORY_ORDINAL; + default: + assert(false && "Unknown command type value"); + return 0; + } +} + +static void mark_command_as_unsupported(struct target *target, uint32_t command) +{ + LOG_TARGET_DEBUG(target, "Caching the abstract " + "command 0x%" PRIx32 " as not supported", command); + log_debug_reg(target, get_cmdtype(command), + command, __FILE__, __LINE__, __func__); + ac_cache_insert(&get_info(target)->ac_not_supported_cache, command); +} + int riscv013_execute_abstract_command(struct target *target, uint32_t command, uint32_t *cmderr) { @@ -684,6 +763,9 @@ 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); + if (res != ERROR_OK && *cmderr == CMDERR_NOT_SUPPORTED) + mark_command_as_unsupported(target, command); + cleanup: riscv_batch_free(batch); return res; @@ -829,38 +911,35 @@ uint32_t riscv013_access_register_command(struct target *target, uint32_t number return command; } +static bool is_command_unsupported(struct target *target, uint32_t command) +{ + bool unsupported = ac_cache_contains(&get_info(target)->ac_not_supported_cache, command); + if (!unsupported) + return false; + + LOG_TARGET_DEBUG(target, "Abstract command 0x%" + PRIx32 " is cached as not supported", command); + log_debug_reg(target, get_cmdtype(command), + command, __FILE__, __LINE__, __func__); + return true; +} + static int register_read_abstract_with_size(struct target *target, riscv_reg_t *value, enum gdb_regno number, unsigned int size) { - 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 (is_command_unsupported(target, 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); @@ -879,23 +958,17 @@ static int register_read_abstract(struct target *target, riscv_reg_t *value, static int register_write_abstract(struct target *target, enum gdb_regno number, riscv_reg_t value) { - RISCV013_INFO(info); - dm013_info_t *dm = get_dm(target); 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 (is_command_unsupported(target, 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; @@ -915,18 +988,9 @@ static int register_write_abstract(struct target *target, enum gdb_regno number, uint32_t cmderr; res = abstract_cmd_batch_check_and_clear_cmderr(target, batch, abstractcs_read_key, &cmderr); + if (res != ERROR_OK && cmderr == CMDERR_NOT_SUPPORTED) + mark_command_as_unsupported(target, command); - 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."); - } - } - } cleanup: riscv_batch_free(batch); return res; @@ -1709,6 +1773,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 +2887,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; } @@ -3719,16 +3777,22 @@ 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, /* virtual = */ false, + width, /* postincrement = */ true, /* is_write = */ false); + bool use_aampostincrement = !is_command_unsupported(target, command); + if (!use_aampostincrement) + /* It is already known that this abstract memory + * access with aampostincrement=1 is not supported. + * So try aampostincrement=0 right away. + * + * TODO: check if new command is supported */ + command = access_memory_command(target, /* virtual = */ false, + width, /* postincrement = */ false, /* is_write = */ false); /* Execute the reads */ uint8_t *p = args.read_buffer; @@ -3749,33 +3813,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, "Trying the same abstract memory " + "read command, but without aampostincrement"); + use_aampostincrement = false; + command = access_memory_command(target, /* virtual = */ false, + width, /* postincrement = */ false, /* is_write = */ false); + result = riscv013_execute_abstract_command(target, command, &cmderr); } /* TODO: @@ -3791,7 +3836,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; } @@ -3809,15 +3854,22 @@ write_memory_abstract(struct target *target, const riscv_mem_access_args_t args) { assert(riscv_mem_access_is_write(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, /* virtual = */ false, + width, /* postincrement = */ true, /* is_write = */ true); + bool use_aampostincrement = !is_command_unsupported(target, command); + if (!use_aampostincrement) + /* It is already known that this abstract memory + * access with aampostincrement=1 is not supported. + * So try aampostincrement=0 right away. + * + * TODO: check if new command is supported */ + command = access_memory_command(target, /* virtual = */ false, + width, /* postincrement = */ false, /* is_write = */ true); /* Execute the writes */ const uint8_t *p = args.write_buffer; @@ -3844,33 +3896,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, "Trying the same abstract memory " + "write command, but without aampostincrement"); + use_aampostincrement = false; + command = access_memory_command(target, /* virtual = */ false, + width, /* postincrement = */ false, /* is_write = */ true); + result = riscv013_execute_abstract_command(target, command, &cmderr); } /* TODO: @@ -3879,7 +3912,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; }