Skip to content

Commit

Permalink
Fix data types around batch.{c,h}
Browse files Browse the repository at this point in the history 
Check that the value of dtmcs.abits is in the expected range.

Make corrections of data types in batch.{c,h} and in related code.
Some of the issues were found by activating "-Wconversion" in GCC,
others by inspecting the code manually.

This is an initial step towards being able to use "-Wconversion" on
RISC-V target code, which will give us bit more confidence when
refactoring or merging new patches.

Changes made:

- Check `dtmcs.abits` during examination.

- DMI address is no larger than 32-bits per the debug spec.
  Changed address parameters of multiple functions from uint64_t
  to uint32_t.

- The value passed to jtag_add_runtest() is now `unsigned int`,
  not `int`. No need for `assert(idle <= INT_MAX)` anymore.

- `get_delay()` in batch.c can return an unsigned value.

- Added few assertions around `field->num_bits` in batch.c.

Signed-off-by: Jan Matyas <[email protected]>
  • Loading branch information
@JanMatCodasip
JanMatCodasip committed Jan 9, 2025
1 parent f82c5a7 commit 3a139b8
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 40 deletions.
34 changes: 22 additions & 12 deletions src/target/riscv/batch.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@
#include "riscv.h"
#include "field_helpers.h"

// TODO: DTM_DMI_MAX_ADDRESS_LENGTH should be reduced to 32 (per the debug spec)
#define DTM_DMI_MAX_ADDRESS_LENGTH ((1<<DTM_DTMCS_ABITS_LENGTH)-1)
#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))

/* Reserve extra room in the batch (needed for the last NOP operation) */
Expand Down Expand Up @@ -127,11 +129,10 @@ static void add_idle_before_batch(const struct riscv_batch *batch, size_t start_
const unsigned int idle_change = new_delay - batch->last_scan_delay;
LOG_TARGET_DEBUG(batch->target, "Adding %u idle cycles before the batch.",
idle_change);
assert(idle_change <= INT_MAX);
jtag_add_runtest(idle_change, TAP_IDLE);
}

static int get_delay(const struct riscv_batch *batch, size_t scan_idx,
static unsigned int get_delay(const struct riscv_batch *batch, size_t scan_idx,
const struct riscv_scan_delays *delays, bool resets_delays,
size_t reset_delays_after)
{
Expand All @@ -142,7 +143,6 @@ static int get_delay(const struct riscv_batch *batch, size_t scan_idx,
const enum riscv_scan_delay_class delay_class =
batch->delay_classes[scan_idx];
const unsigned int delay = riscv_scan_get_delay(delays, delay_class);
assert(delay <= INT_MAX);
return delays_were_reset ? 0 : delay;
}

Expand Down Expand Up @@ -199,9 +199,10 @@ static void log_batch(const struct riscv_batch *batch, size_t start_idx,
return;

const unsigned int scan_bits = batch->fields->num_bits;
assert(scan_bits == (unsigned int)riscv_get_dmi_scan_length(batch->target));
const unsigned int abits = scan_bits - DTM_DMI_OP_LENGTH
- DTM_DMI_DATA_LENGTH;
assert(scan_bits == riscv_get_dmi_scan_length(batch->target));
assert(scan_bits <= DMI_SCAN_MAX_BIT_LENGTH);
const unsigned int abits = (unsigned int)(scan_bits - DTM_DMI_OP_LENGTH
- DTM_DMI_DATA_LENGTH);

/* Determine the "op" and "address" of the scan that preceded the first
* executed scan.
Expand All @@ -211,7 +212,7 @@ static void log_batch(const struct riscv_batch *batch, size_t start_idx,
* would be a more robust solution.
*/
bool last_scan_was_read = false;
uint32_t last_scan_address = -1 /* to silence maybe-uninitialized */;
uint32_t last_scan_address = (uint32_t)(-1) /* to silence maybe-uninitialized */;
if (start_idx > 0) {
const struct scan_field * const field = &batch->fields[start_idx - 1];
assert(field->out_value);
Expand All @@ -224,7 +225,7 @@ static void log_batch(const struct riscv_batch *batch, size_t start_idx,
/* Decode and log every executed scan */
for (size_t i = start_idx; i < batch->used_scans; ++i) {
static const char * const op_string[] = {"-", "r", "w", "?"};
const int delay = get_delay(batch, i, delays, resets_delays,
const unsigned int delay = get_delay(batch, i, delays, resets_delays,
reset_delays_after);
const struct scan_field * const field = &batch->fields[i];

Expand All @@ -247,15 +248,15 @@ static void log_batch(const struct riscv_batch *batch, size_t start_idx,
DTM_DMI_ADDRESS_OFFSET, abits);

LOG_DEBUG("%db %s %08" PRIx32 " @%02" PRIx32
" -> %s %08" PRIx32 " @%02" PRIx32 "; %di",
" -> %s %08" PRIx32 " @%02" PRIx32 "; %ui",
field->num_bits, op_string[out_op], out_data, out_address,
status_string[in_op], in_data, in_address, delay);

if (last_scan_was_read && in_op == DTM_DMI_OP_SUCCESS)
log_dmi_decoded(batch, /*write*/ false,
last_scan_address, in_data);
} else {
LOG_DEBUG("%db %s %08" PRIx32 " @%02" PRIx32 " -> ?; %di",
LOG_DEBUG("%db %s %08" PRIx32 " @%02" PRIx32 " -> ?; %ui",
field->num_bits, op_string[out_op], out_data, out_address,
delay);
}
Expand Down Expand Up @@ -321,12 +322,16 @@ int riscv_batch_run_from(struct riscv_batch *batch, size_t start_idx,
return ERROR_OK;
}

void riscv_batch_add_dmi_write(struct riscv_batch *batch, uint64_t address, uint32_t data,
void riscv_batch_add_dmi_write(struct riscv_batch *batch, uint32_t address, uint32_t data,
bool read_back, enum riscv_scan_delay_class delay_class)
{
// TODO: Check that the bit width of "address" is no more than dtmcs.abits,
// otherwise return an error (during batch creation or when the batch is executed).

assert(batch->used_scans < batch->allocated_scans);
struct scan_field *field = batch->fields + batch->used_scans;
field->num_bits = riscv_get_dmi_scan_length(batch->target);
assert(field->num_bits <= DMI_SCAN_MAX_BIT_LENGTH);
field->out_value = (void *)(batch->data_out + batch->used_scans * DMI_SCAN_BUF_SIZE);
riscv_fill_dmi_write(batch->target, (char *)field->out_value, address, data);
if (read_back) {
Expand All @@ -340,12 +345,16 @@ void riscv_batch_add_dmi_write(struct riscv_batch *batch, uint64_t address, uint
batch->used_scans++;
}

size_t riscv_batch_add_dmi_read(struct riscv_batch *batch, uint64_t address,
size_t riscv_batch_add_dmi_read(struct riscv_batch *batch, uint32_t address,
enum riscv_scan_delay_class delay_class)
{
// TODO: Check that the bit width of "address" is no more than dtmcs.abits,
// otherwise return an error (during batch creation or when the batch is executed).

assert(batch->used_scans < batch->allocated_scans);
struct scan_field *field = batch->fields + batch->used_scans;
field->num_bits = riscv_get_dmi_scan_length(batch->target);
assert(field->num_bits <= DMI_SCAN_MAX_BIT_LENGTH);
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_dmi_read(batch->target, (char *)field->out_value, address);
Expand Down Expand Up @@ -383,6 +392,7 @@ 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_get_dmi_scan_length(batch->target);
assert(field->num_bits <= DMI_SCAN_MAX_BIT_LENGTH);
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(batch->target, (char *)field->out_value);
Expand Down
8 changes: 4 additions & 4 deletions src/target/riscv/batch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,11 @@ int riscv_batch_run_from(struct riscv_batch *batch, size_t start_idx,
size_t riscv_batch_finished_scans(const struct riscv_batch *batch);

/* Adds a DM register write to this batch. */
void riscv_batch_add_dmi_write(struct riscv_batch *batch, uint64_t address, uint32_t data,
void riscv_batch_add_dmi_write(struct riscv_batch *batch, uint32_t address, uint32_t data,
bool read_back, enum riscv_scan_delay_class delay_class);

static inline void
riscv_batch_add_dm_write(struct riscv_batch *batch, uint64_t address, uint32_t data,
riscv_batch_add_dm_write(struct riscv_batch *batch, uint32_t address, uint32_t data,
bool read_back, enum riscv_scan_delay_class delay_type)
{
return riscv_batch_add_dmi_write(batch,
Expand All @@ -205,11 +205,11 @@ riscv_batch_add_dm_write(struct riscv_batch *batch, uint64_t address, uint32_t d
/* 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_dmi_read(struct riscv_batch *batch, uint64_t address,
size_t riscv_batch_add_dmi_read(struct riscv_batch *batch, uint32_t address,
enum riscv_scan_delay_class delay_class);

static inline size_t
riscv_batch_add_dm_read(struct riscv_batch *batch, uint64_t address,
riscv_batch_add_dm_read(struct riscv_batch *batch, uint32_t address,
enum riscv_scan_delay_class delay_type)
{
return riscv_batch_add_dmi_read(batch,
Expand Down
53 changes: 38 additions & 15 deletions src/target/riscv/riscv-013.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@ 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(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 int riscv013_get_dmi_scan_length(struct target *target);
static void riscv013_fill_dmi_write(struct target *target, char *buf, uint32_t a, uint32_t d);
static void riscv013_fill_dmi_read(struct target *target, char *buf, uint32_t a);
static unsigned int riscv013_get_dmi_scan_length(struct target *target);
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,
Expand Down Expand Up @@ -1941,6 +1941,29 @@ static int examine(struct target *target)
info->abits = get_field(dtmcontrol, DTM_DTMCS_ABITS);
info->dtmcs_idle = get_field(dtmcontrol, DTM_DTMCS_IDLE);

if (info->abits > RISCV013_DTMCS_ABITS_MAX) {
/* Max. address width given by the debug specification is exceeded */
LOG_TARGET_ERROR(target, "The target's debug bus (DMI) address width exceeds "
"the maximum:");
LOG_TARGET_ERROR(target, " found dtmcs.abits = %d; maximum is abits = %d.",
info->abits, RISCV013_DTMCS_ABITS_MAX);
return ERROR_FAIL;
}

if (info->abits < RISCV013_DTMCS_ABITS_MIN) {
/* The requirement for minimum DMI address width of 7 bits is part of
* the RISC-V Debug spec since Jan-20-2017 (commit 03df6ee7). However,
* implementations exist that implement narrower DMI address. For example
* Spike as of Q1/2025 uses dmi.abits = 6.
*
* For that reason, warn the user but continue.
*/
LOG_TARGET_WARNING(target, "The target's debug bus (DMI) address width is "
"lower than the minimum:");
LOG_TARGET_WARNING(target, " found dtmcs.abits = %d; minimum is abits = %d.",
info->abits, RISCV013_DTMCS_ABITS_MIN);
}

if (!check_dbgbase_exists(target)) {
LOG_TARGET_ERROR(target, "Could not find debug module with DMI base address (dbgbase) = 0x%x", target->dbgbase);
return ERROR_FAIL;
Expand Down Expand Up @@ -5390,31 +5413,31 @@ static int riscv013_execute_progbuf(struct target *target, uint32_t *cmderr)
return riscv013_execute_abstract_command(target, run_program, cmderr);
}

static void riscv013_fill_dmi_write(struct target *target, char *buf, uint64_t a, uint32_t d)
static void riscv013_fill_dmi_write(struct target *target, char *buf, uint32_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);
buf_set_u64((unsigned char *)buf, DTM_DMI_DATA_OFFSET, DTM_DMI_DATA_LENGTH, d);
buf_set_u64((unsigned char *)buf, DTM_DMI_ADDRESS_OFFSET, info->abits, a);
buf_set_u32((unsigned char *)buf, DTM_DMI_OP_OFFSET, DTM_DMI_OP_LENGTH, DMI_OP_WRITE);
buf_set_u32((unsigned char *)buf, DTM_DMI_DATA_OFFSET, DTM_DMI_DATA_LENGTH, d);
buf_set_u32((unsigned char *)buf, DTM_DMI_ADDRESS_OFFSET, info->abits, a);
}

static void riscv013_fill_dmi_read(struct target *target, char *buf, uint64_t a)
static void riscv013_fill_dmi_read(struct target *target, char *buf, uint32_t a)
{
RISCV013_INFO(info);
buf_set_u64((unsigned char *)buf, DTM_DMI_OP_OFFSET, DTM_DMI_OP_LENGTH, DMI_OP_READ);
buf_set_u64((unsigned char *)buf, DTM_DMI_DATA_OFFSET, DTM_DMI_DATA_LENGTH, 0);
buf_set_u64((unsigned char *)buf, DTM_DMI_ADDRESS_OFFSET, info->abits, a);
buf_set_u32((unsigned char *)buf, DTM_DMI_OP_OFFSET, DTM_DMI_OP_LENGTH, DMI_OP_READ);
buf_set_u32((unsigned char *)buf, DTM_DMI_DATA_OFFSET, DTM_DMI_DATA_LENGTH, 0);
buf_set_u32((unsigned char *)buf, DTM_DMI_ADDRESS_OFFSET, info->abits, a);
}

static void riscv013_fill_dm_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);
buf_set_u64((unsigned char *)buf, DTM_DMI_DATA_OFFSET, DTM_DMI_DATA_LENGTH, 0);
buf_set_u64((unsigned char *)buf, DTM_DMI_ADDRESS_OFFSET, info->abits, 0);
buf_set_u32((unsigned char *)buf, DTM_DMI_OP_OFFSET, DTM_DMI_OP_LENGTH, DMI_OP_NOP);
buf_set_u32((unsigned char *)buf, DTM_DMI_DATA_OFFSET, DTM_DMI_DATA_LENGTH, 0);
buf_set_u32((unsigned char *)buf, DTM_DMI_ADDRESS_OFFSET, info->abits, 0);
}

static int riscv013_get_dmi_scan_length(struct target *target)
static unsigned int riscv013_get_dmi_scan_length(struct target *target)
{
RISCV013_INFO(info);
return info->abits + DTM_DMI_DATA_LENGTH + DTM_DMI_OP_LENGTH;
Expand Down
6 changes: 3 additions & 3 deletions src/target/riscv/riscv.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5890,13 +5890,13 @@ int riscv_execute_progbuf(struct target *target, uint32_t *cmderr)
return r->execute_progbuf(target, cmderr);
}

void riscv_fill_dmi_write(struct target *target, char *buf, uint64_t a, uint32_t d)
void riscv_fill_dmi_write(struct target *target, char *buf, uint32_t a, uint32_t d)
{
RISCV_INFO(r);
r->fill_dmi_write(target, buf, a, d);
}

void riscv_fill_dmi_read(struct target *target, char *buf, uint64_t a)
void riscv_fill_dmi_read(struct target *target, char *buf, uint32_t a)
{
RISCV_INFO(r);
r->fill_dmi_read(target, buf, a);
Expand All @@ -5908,7 +5908,7 @@ void riscv_fill_dm_nop(struct target *target, char *buf)
r->fill_dm_nop(target, buf);
}

int riscv_get_dmi_scan_length(struct target *target)
unsigned int riscv_get_dmi_scan_length(struct target *target)
{
RISCV_INFO(r);
return r->get_dmi_scan_length(target);
Expand Down
15 changes: 9 additions & 6 deletions src/target/riscv/riscv.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,9 @@ typedef struct {
#define DTM_DTMCS_VERSION_UNKNOWN ((unsigned int)-1)
#define RISCV_TINFO_VERSION_UNKNOWN (-1)

#define RISCV013_DTMCS_ABITS_MIN 7
#define RISCV013_DTMCS_ABITS_MAX 32

struct reg_name_table {
unsigned int num_entries;
char **reg_names;
Expand Down Expand Up @@ -275,9 +278,9 @@ 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 (*get_dmi_scan_length)(struct target *target);
void (*fill_dmi_write)(struct target *target, char *buf, uint64_t a, uint32_t d);
void (*fill_dmi_read)(struct target *target, char *buf, uint64_t a);
unsigned int (*get_dmi_scan_length)(struct target *target);
void (*fill_dmi_write)(struct target *target, char *buf, uint32_t a, uint32_t d);
void (*fill_dmi_read)(struct target *target, char *buf, uint32_t a);
void (*fill_dm_nop)(struct target *target, char *buf);

int (*authdata_read)(struct target *target, uint32_t *value, unsigned int index);
Expand Down Expand Up @@ -463,9 +466,9 @@ 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(struct target *target, char *buf);
void riscv_fill_dmi_write(struct target *target, char *buf, uint64_t a, uint32_t d);
void riscv_fill_dmi_read(struct target *target, char *buf, uint64_t a);
int riscv_get_dmi_scan_length(struct target *target);
void riscv_fill_dmi_write(struct target *target, char *buf, uint32_t a, uint32_t d);
void riscv_fill_dmi_read(struct target *target, char *buf, uint32_t a);
unsigned int riscv_get_dmi_scan_length(struct target *target);

uint32_t riscv_get_dmi_address(const struct target *target, uint32_t dm_address);

Expand Down

0 comments on commit 3a139b8

Please sign in to comment.