From 67a3d4fe7fbaf65b612a49c7d0a4f739eaa67be7 Mon Sep 17 00:00:00 2001 From: Jan Matyas Date: Tue, 6 Feb 2024 14:24:02 +0100 Subject: [PATCH 1/2] Fixes and cleanup in riscv batch and related functions Fixes: - Data types of address & data parameters in riscv_batch_add_*() and riscv*_fill_dm*() changed to uint64_t and uint32_t. - Corrected the comparison in riscv_batch_full(). - Corrected assertions in riscv_batch_get_dmi_read_op() and riscv_batch_get_dmi_read_data(). Cleanup: - Simplified calloc() fail handling in riscv_batch_alloc(). - Added explicit NULL assignments in riscv_batch_alloc() for clarity and readability. Don't rely on calloc(). - Removed suffix `_u64` from riscv_*_fill_dm*() since it does not have any meaning. - Renamed *dmi_write_u64_bits() to *get_dmi_scan_length() which better describes its purpose. Change-Id: Id70e5968528d64b2ee5476f1c00e08459a1e291d Signed-off-by: Jan Matyas --- src/target/riscv/batch.c | 91 +++++++++++++++++++----------------- src/target/riscv/batch.h | 4 +- src/target/riscv/riscv-013.c | 42 ++++++++--------- src/target/riscv/riscv.c | 16 +++---- src/target/riscv/riscv.h | 16 +++---- 5 files changed, 88 insertions(+), 81 deletions(-) diff --git a/src/target/riscv/batch.c b/src/target/riscv/batch.c index 290ce371be..2779535c7b 100644 --- a/src/target/riscv/batch.c +++ b/src/target/riscv/batch.c @@ -13,58 +13,62 @@ #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)) +#define BATCH_RESERVED_SCANS 4 + static void dump_field(int idle, const struct scan_field *field); struct riscv_batch *riscv_batch_alloc(struct target *target, size_t scans, size_t idle) { - scans += 4; + scans += BATCH_RESERVED_SCANS; struct riscv_batch *out = calloc(1, sizeof(*out)); - if (!out) - goto error0; + if (!out) { + LOG_ERROR("Failed to allocate struct riscv_batch"); + return NULL; + } + out->target = target; out->allocated_scans = scans; out->idle_count = idle; - out->data_out = malloc(sizeof(*out->data_out) * (scans) * DMI_SCAN_BUF_SIZE); + out->last_scan = RISCV_SCAN_TYPE_INVALID; + + out->data_out = NULL; + out->data_in = NULL; + out->fields = NULL; + out->bscan_ctxt = NULL; + out->read_keys = NULL; + + out->data_out = malloc(sizeof(*out->data_out) * scans * DMI_SCAN_BUF_SIZE); if (!out->data_out) { LOG_ERROR("Failed to allocate data_out in RISC-V batch."); - goto error1; + goto alloc_error; }; - out->data_in = malloc(sizeof(*out->data_in) * (scans) * DMI_SCAN_BUF_SIZE); + out->data_in = malloc(sizeof(*out->data_in) * scans * DMI_SCAN_BUF_SIZE); if (!out->data_in) { LOG_ERROR("Failed to allocate data_in in RISC-V batch."); - goto error2; + goto alloc_error; } - out->fields = malloc(sizeof(*out->fields) * (scans)); + out->fields = malloc(sizeof(*out->fields) * scans); if (!out->fields) { LOG_ERROR("Failed to allocate fields in RISC-V batch."); - goto error3; + goto alloc_error; } if (bscan_tunnel_ir_width != 0) { - out->bscan_ctxt = malloc(sizeof(*out->bscan_ctxt) * (scans)); + out->bscan_ctxt = malloc(sizeof(*out->bscan_ctxt) * scans); if (!out->bscan_ctxt) { LOG_ERROR("Failed to allocate bscan_ctxt in RISC-V batch."); - goto error4; + goto alloc_error; } } - out->last_scan = RISCV_SCAN_TYPE_INVALID; - out->read_keys = malloc(sizeof(*out->read_keys) * (scans)); + out->read_keys = malloc(sizeof(*out->read_keys) * scans); if (!out->read_keys) { LOG_ERROR("Failed to allocate read_keys in RISC-V batch."); - goto error5; + goto alloc_error; } + return out; -error5: - free(out->bscan_ctxt); -error4: - free(out->fields); -error3: - free(out->data_in); -error2: - free(out->data_out); -error1: - free(out); -error0: +alloc_error: + riscv_batch_free(out); return NULL; } @@ -80,7 +84,7 @@ void riscv_batch_free(struct riscv_batch *batch) bool riscv_batch_full(struct riscv_batch *batch) { - return batch->used_scans > (batch->allocated_scans - 4); + return batch->used_scans >= (batch->allocated_scans - BATCH_RESERVED_SCANS); } int riscv_batch_run(struct riscv_batch *batch) @@ -125,17 +129,17 @@ int riscv_batch_run(struct riscv_batch *batch) return ERROR_OK; } -void riscv_batch_add_dm_write(struct riscv_batch *batch, unsigned int address, uint64_t data, +void riscv_batch_add_dm_write(struct riscv_batch *batch, uint64_t address, uint32_t data, bool read_back) { assert(batch->used_scans < batch->allocated_scans); struct scan_field *field = batch->fields + batch->used_scans; - field->num_bits = riscv_dmi_write_u64_bits(batch->target); + 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_dm_write_u64(batch->target, (char *)field->out_value, address, data); + riscv_fill_dm_write(batch->target, (char *)field->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_u64(batch->target, (char *)field->in_value); + riscv_fill_dm_nop(batch->target, (char *)field->in_value); } else { field->in_value = NULL; } @@ -143,15 +147,15 @@ void riscv_batch_add_dm_write(struct riscv_batch *batch, unsigned int address, u batch->used_scans++; } -size_t riscv_batch_add_dm_read(struct riscv_batch *batch, unsigned int address) +size_t riscv_batch_add_dm_read(struct riscv_batch *batch, uint64_t address) { assert(batch->used_scans < batch->allocated_scans); struct scan_field *field = batch->fields + batch->used_scans; - field->num_bits = riscv_dmi_write_u64_bits(batch->target); + 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_read_u64(batch->target, (char *)field->out_value, address); - riscv_fill_dm_nop_u64(batch->target, (char *)field->in_value); + riscv_fill_dm_read(batch->target, (char *)field->out_value, address); + riscv_fill_dm_nop(batch->target, (char *)field->in_value); batch->last_scan = RISCV_SCAN_TYPE_READ; batch->used_scans++; @@ -163,17 +167,17 @@ unsigned int riscv_batch_get_dmi_read_op(const struct riscv_batch *batch, size_t { assert(key < batch->read_keys_used); size_t index = batch->read_keys[key]; - assert(index <= batch->used_scans); + assert(index < batch->used_scans); uint8_t *base = batch->data_in + DMI_SCAN_BUF_SIZE * index; /* extract "op" field from the DMI read result */ - return (unsigned)buf_get_u32(base, DTM_DMI_OP_OFFSET, DTM_DMI_OP_LENGTH); + return (unsigned int)buf_get_u32(base, DTM_DMI_OP_OFFSET, DTM_DMI_OP_LENGTH); } uint32_t riscv_batch_get_dmi_read_data(const struct riscv_batch *batch, size_t key) { assert(key < batch->read_keys_used); size_t index = batch->read_keys[key]; - assert(index <= batch->used_scans); + assert(index < batch->used_scans); uint8_t *base = batch->data_in + DMI_SCAN_BUF_SIZE * index; /* extract "data" field from the DMI read result */ return buf_get_u32(base, DTM_DMI_DATA_OFFSET, DTM_DMI_DATA_LENGTH); @@ -183,11 +187,11 @@ 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_dmi_write_u64_bits(batch->target); + 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_u64(batch->target, (char *)field->out_value); - riscv_fill_dm_nop_u64(batch->target, (char *)field->in_value); + riscv_fill_dm_nop(batch->target, (char *)field->out_value); + riscv_fill_dm_nop(batch->target, (char *)field->in_value); batch->last_scan = RISCV_SCAN_TYPE_NOP; batch->used_scans++; } @@ -226,13 +230,16 @@ static void dump_field(int idle, const struct scan_field *field) size_t riscv_batch_available_scans(struct riscv_batch *batch) { - return batch->allocated_scans - batch->used_scans - 4; + assert(batch->allocated_scans >= (batch->used_scans + BATCH_RESERVED_SCANS)); + return batch->allocated_scans - batch->used_scans - BATCH_RESERVED_SCANS; } bool riscv_batch_dmi_busy_encountered(const struct riscv_batch *batch) { - if (!batch->used_scans) + if (batch->used_scans == 0) + /* Empty batch */ return false; + assert(batch->last_scan == RISCV_SCAN_TYPE_NOP); const struct scan_field *field = batch->fields + batch->used_scans - 1; const uint64_t in = buf_get_u64(field->in_value, 0, field->num_bits); diff --git a/src/target/riscv/batch.h b/src/target/riscv/batch.h index 6ebc229e65..839e13e827 100644 --- a/src/target/riscv/batch.h +++ b/src/target/riscv/batch.h @@ -59,13 +59,13 @@ bool riscv_batch_full(struct riscv_batch *batch); int riscv_batch_run(struct riscv_batch *batch); /* Adds a DM register write to this batch. */ -void riscv_batch_add_dm_write(struct riscv_batch *batch, unsigned int address, uint64_t data, +void riscv_batch_add_dm_write(struct riscv_batch *batch, uint64_t address, uint32_t data, bool read_back); /* 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_dm_read(struct riscv_batch *batch, unsigned int address); +size_t riscv_batch_add_dm_read(struct riscv_batch *batch, uint64_t address); unsigned int riscv_batch_get_dmi_read_op(const struct riscv_batch *batch, size_t key); uint32_t riscv_batch_get_dmi_read_data(const struct riscv_batch *batch, size_t key); diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c index 0a68bfe98a..6acd3d70c9 100644 --- a/src/target/riscv/riscv-013.c +++ b/src/target/riscv/riscv-013.c @@ -55,13 +55,13 @@ 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_u64(struct target *target, char *buf, int a, uint64_t d); -static void riscv013_fill_dmi_read_u64(struct target *target, char *buf, int a); -static int riscv013_dmi_write_u64_bits(struct target *target); -static void riscv013_fill_dmi_nop_u64(struct target *target, char *buf); -static void riscv013_fill_dm_write_u64(struct target *target, char *buf, int a, uint64_t d); -static void riscv013_fill_dm_read_u64(struct target *target, char *buf, int a); -static void riscv013_fill_dm_nop_u64(struct target *target, char *buf); +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 void riscv013_fill_dmi_nop(struct target *target, char *buf); +static int riscv013_get_dmi_scan_length(struct target *target); +static void riscv013_fill_dm_write(struct target *target, char *buf, uint64_t a, uint32_t d); +static void riscv013_fill_dm_read(struct target *target, char *buf, uint64_t a); +static void riscv013_fill_dm_nop(struct target *target, char *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); @@ -2767,10 +2767,10 @@ static int init_target(struct command_context *cmd_ctx, generic_info->write_progbuf = &riscv013_write_progbuf; generic_info->execute_progbuf = &riscv013_execute_progbuf; generic_info->invalidate_cached_progbuf = &riscv013_invalidate_cached_progbuf; - generic_info->fill_dm_write_u64 = &riscv013_fill_dm_write_u64; - generic_info->fill_dm_read_u64 = &riscv013_fill_dm_read_u64; - generic_info->fill_dm_nop_u64 = &riscv013_fill_dm_nop_u64; - generic_info->dmi_write_u64_bits = &riscv013_dmi_write_u64_bits; + generic_info->fill_dm_write = &riscv013_fill_dm_write; + generic_info->fill_dm_read = &riscv013_fill_dm_read; + generic_info->fill_dm_nop = &riscv013_fill_dm_nop; + generic_info->get_dmi_scan_length = &riscv013_get_dmi_scan_length; generic_info->authdata_read = &riscv013_authdata_read; generic_info->authdata_write = &riscv013_authdata_write; generic_info->dmi_read = &dmi_read; @@ -5169,7 +5169,7 @@ static int riscv013_execute_progbuf(struct target *target, uint32_t *cmderr) return execute_abstract_command(target, run_program, cmderr); } -static void riscv013_fill_dmi_write_u64(struct target *target, char *buf, int a, uint64_t d) +static void riscv013_fill_dmi_write(struct target *target, char *buf, uint64_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); @@ -5177,7 +5177,7 @@ static void riscv013_fill_dmi_write_u64(struct target *target, char *buf, int a, buf_set_u64((unsigned char *)buf, DTM_DMI_ADDRESS_OFFSET, info->abits, a); } -static void riscv013_fill_dmi_read_u64(struct target *target, char *buf, int a) +static void riscv013_fill_dmi_read(struct target *target, char *buf, uint64_t a) { RISCV013_INFO(info); buf_set_u64((unsigned char *)buf, DTM_DMI_OP_OFFSET, DTM_DMI_OP_LENGTH, DMI_OP_READ); @@ -5185,7 +5185,7 @@ static void riscv013_fill_dmi_read_u64(struct target *target, char *buf, int a) buf_set_u64((unsigned char *)buf, DTM_DMI_ADDRESS_OFFSET, info->abits, a); } -static void riscv013_fill_dmi_nop_u64(struct target *target, char *buf) +static void riscv013_fill_dmi_nop(struct target *target, char *buf) { RISCV013_INFO(info); buf_set_u64((unsigned char *)buf, DTM_DMI_OP_OFFSET, DTM_DMI_OP_LENGTH, DMI_OP_NOP); @@ -5193,31 +5193,31 @@ static void riscv013_fill_dmi_nop_u64(struct target *target, char *buf) buf_set_u64((unsigned char *)buf, DTM_DMI_ADDRESS_OFFSET, info->abits, 0); } -static int riscv013_dmi_write_u64_bits(struct target *target) +static int riscv013_get_dmi_scan_length(struct target *target) { RISCV013_INFO(info); return info->abits + DTM_DMI_DATA_LENGTH + DTM_DMI_OP_LENGTH; } -void riscv013_fill_dm_write_u64(struct target *target, char *buf, int a, uint64_t d) +void riscv013_fill_dm_write(struct target *target, char *buf, uint64_t a, uint32_t d) { dm013_info_t *dm = get_dm(target); if (!dm) return; - riscv013_fill_dmi_write_u64(target, buf, a + dm->base, d); + riscv013_fill_dmi_write(target, buf, a + dm->base, d); } -void riscv013_fill_dm_read_u64(struct target *target, char *buf, int a) +void riscv013_fill_dm_read(struct target *target, char *buf, uint64_t a) { dm013_info_t *dm = get_dm(target); if (!dm) return; - riscv013_fill_dmi_read_u64(target, buf, a + dm->base); + riscv013_fill_dmi_read(target, buf, a + dm->base); } -void riscv013_fill_dm_nop_u64(struct target *target, char *buf) +void riscv013_fill_dm_nop(struct target *target, char *buf) { - riscv013_fill_dmi_nop_u64(target, buf); + riscv013_fill_dmi_nop(target, buf); } static int maybe_execute_fence_i(struct target *target) diff --git a/src/target/riscv/riscv.c b/src/target/riscv/riscv.c index 97de01703c..f3f77687d7 100644 --- a/src/target/riscv/riscv.c +++ b/src/target/riscv/riscv.c @@ -5354,28 +5354,28 @@ int riscv_execute_progbuf(struct target *target, uint32_t *cmderr) return r->execute_progbuf(target, cmderr); } -void riscv_fill_dm_write_u64(struct target *target, char *buf, int a, uint64_t d) +void riscv_fill_dm_write(struct target *target, char *buf, uint64_t a, uint32_t d) { RISCV_INFO(r); - r->fill_dm_write_u64(target, buf, a, d); + r->fill_dm_write(target, buf, a, d); } -void riscv_fill_dm_read_u64(struct target *target, char *buf, int a) +void riscv_fill_dm_read(struct target *target, char *buf, uint64_t a) { RISCV_INFO(r); - r->fill_dm_read_u64(target, buf, a); + r->fill_dm_read(target, buf, a); } -void riscv_fill_dm_nop_u64(struct target *target, char *buf) +void riscv_fill_dm_nop(struct target *target, char *buf) { RISCV_INFO(r); - r->fill_dm_nop_u64(target, buf); + r->fill_dm_nop(target, buf); } -int riscv_dmi_write_u64_bits(struct target *target) +int riscv_get_dmi_scan_length(struct target *target) { RISCV_INFO(r); - return r->dmi_write_u64_bits(target); + return r->get_dmi_scan_length(target); } static int check_if_trigger_exists(struct target *target, unsigned int index) diff --git a/src/target/riscv/riscv.h b/src/target/riscv/riscv.h index 8f28232f27..b7d97d1d4d 100644 --- a/src/target/riscv/riscv.h +++ b/src/target/riscv/riscv.h @@ -231,10 +231,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 (*dmi_write_u64_bits)(struct target *target); - void (*fill_dm_write_u64)(struct target *target, char *buf, int a, uint64_t d); - void (*fill_dm_read_u64)(struct target *target, char *buf, int a); - void (*fill_dm_nop_u64)(struct target *target, char *buf); + int (*get_dmi_scan_length)(struct target *target); + void (*fill_dm_write)(struct target *target, char *buf, uint64_t a, uint32_t d); + void (*fill_dm_read)(struct target *target, char *buf, uint64_t a); + void (*fill_dm_nop)(struct target *target, char *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); @@ -429,10 +429,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_u64(struct target *target, char *buf); -void riscv_fill_dm_write_u64(struct target *target, char *buf, int a, uint64_t d); -void riscv_fill_dm_read_u64(struct target *target, char *buf, int a); -int riscv_dmi_write_u64_bits(struct target *target); +void riscv_fill_dm_nop(struct target *target, char *buf); +void riscv_fill_dm_write(struct target *target, char *buf, uint64_t a, uint32_t d); +void riscv_fill_dm_read(struct target *target, char *buf, uint64_t a); +int riscv_get_dmi_scan_length(struct target *target); int riscv_enumerate_triggers(struct target *target); From 9bcbae13e086e6894265439e25098395f3e7e6df Mon Sep 17 00:00:00 2001 From: Jan Matyas Date: Fri, 9 Feb 2024 08:30:18 +0100 Subject: [PATCH 2/2] Fixes of review findings Change-Id: Ie9889d995d7b2a6e458ad5f66cc3d990888f54ec Signed-off-by: Jan Matyas --- src/target/riscv/batch.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/target/riscv/batch.c b/src/target/riscv/batch.c index 2779535c7b..faa92568b6 100644 --- a/src/target/riscv/batch.c +++ b/src/target/riscv/batch.c @@ -13,7 +13,8 @@ #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)) -#define BATCH_RESERVED_SCANS 4 +/* Reserve extra room in the batch (needed for the last NOP operation) */ +#define BATCH_RESERVED_SCANS 1 static void dump_field(int idle, const struct scan_field *field); @@ -37,6 +38,9 @@ struct riscv_batch *riscv_batch_alloc(struct target *target, size_t scans, size_ out->bscan_ctxt = NULL; out->read_keys = NULL; + /* FIXME: There is potential for memory usage reduction. We could allocate + * smaller buffers than DMI_SCAN_BUF_SIZE (that is, buffers that correspond to + * the real DR scan length on the given target) */ out->data_out = malloc(sizeof(*out->data_out) * scans * DMI_SCAN_BUF_SIZE); if (!out->data_out) { LOG_ERROR("Failed to allocate data_out in RISC-V batch."); @@ -84,7 +88,7 @@ void riscv_batch_free(struct riscv_batch *batch) bool riscv_batch_full(struct riscv_batch *batch) { - return batch->used_scans >= (batch->allocated_scans - BATCH_RESERVED_SCANS); + return riscv_batch_available_scans(batch) == 0; } int riscv_batch_run(struct riscv_batch *batch) @@ -98,7 +102,7 @@ int riscv_batch_run(struct riscv_batch *batch) for (size_t i = 0; i < batch->used_scans; ++i) { if (bscan_tunnel_ir_width != 0) - riscv_add_bscan_tunneled_scan(batch->target, batch->fields+i, batch->bscan_ctxt+i); + riscv_add_bscan_tunneled_scan(batch->target, batch->fields + i, batch->bscan_ctxt + i); else jtag_add_dr_scan(batch->target->tap, 1, batch->fields + i, TAP_IDLE);