target/riscv: use batch interface in read_memory_bus_v1()

[en-sc]

Fixes #1080

[en-sc]

This PR is based on #1073

[JanMatCodasip]

Overall this change looks all right - thanks. I like how the use of the batch API simplifies the code in read_memory_bus_v1() and makes it easier to reason about.

Two things:

• I have not really grasped where exactly in the original code was the problem causing read_memory_bus_v1() is broken #1080, and what had to happen for it to occur. Could you please explain it.
• I believe the order of sbdata0 reads should be different - please see my comments.

[en-sc]

I have not really grasped where exactly in the original code was the problem causing read_memory_bus_v1() is broken #1080, and what had to happen for it to occur. Could you please explain it.

How the issue occurs?

• The delay is not enough, so the first read results in a busy response:
  

  riscv-openocd/src/target/riscv/riscv-013.c

  Line 3700 in c6bb902

  if (get_field(sbcs_read, DM_SBCS_SBBUSYERROR)) {

• next_address = 0x1004 (read from sbaddress*).
  https://github.com/riscv-collab/riscv-openocd/blob/riscv/src/target/riscv/riscv-013.c#L3708
• The first element is read from sbdata correctly.
  

  riscv-openocd/src/target/riscv/riscv-013.c

  Lines 3714 to 3715 in c6bb902

  	if (read_memory_bus_word(target, current_address, size,
  	buffer + current_address - address) != ERROR_OK)

Debug: 647 10 riscv-013.c:3379 log_memory_access64(): M[0x1000] reads 0x00000297

• continue transfers control to the start of the outermost loop:
  

  riscv-openocd/src/target/riscv/riscv-013.c

  Line 3722 in c6bb902

  continue;

  
  

  riscv-openocd/src/target/riscv/riscv-013.c

  Line 3582 in c6bb902

  while (next_address < end_address) {

• On this iteration, the loop that reads an element is not entered, since next_address - address == 4, i = (next_address - address) / size == 1, i < (count - 1 == 1) is false.:
  

  riscv-openocd/src/target/riscv/riscv-013.c

  Line 3615 in c6bb902

  for (uint32_t i = (next_address - address) / size; i < count - 1; i++) {

• However, DMI NOP is executed outside the loop, since the count is greater than 1:
  

  riscv-openocd/src/target/riscv/riscv-013.c

  Lines 3660 to 3675 in c6bb902

  	if (count > 1) {
  	unsigned attempt = 0;
  	while (1) {
  	if (attempt++ > 100) {
  	LOG_TARGET_ERROR(target, "DMI keeps being busy in while reading memory"
  	" just past " TARGET_ADDR_FMT, next_read);
  	return ERROR_FAIL;
  	}
  	dmi_status_t status = dmi_scan(target, NULL, &sbvalue[0], DMI_OP_NOP, 0, 0, false);
  	if (status == DMI_STATUS_BUSY)
  	increase_dmi_busy_delay(target);
  	else if (status == DMI_STATUS_SUCCESS)
  	break;
  	else
  	return ERROR_FAIL;
  	}

• This NOP returns some garbage (the previous DMI OP was a NOP) and this garbage is assumed to be the result of the read (and overrides the previous correct result):

Debug: 661 11 riscv-013.c:3379 log_memory_access64(): M[0xfff] reads 0x00000000

• The reported address is equal to 0xfff due to initialization of next_read.
  

  riscv-openocd/src/target/riscv/riscv-013.c

  Line 3612 in c6bb902

  target_addr_t next_read = address - 1;

TL;DR:

The condition

riscv-openocd/src/target/riscv/riscv-013.c

Line 3660 in c6bb902

if (count > 1) {

Shoudl guarantee that the body of the loop

riscv-openocd/src/target/riscv/riscv-013.c

Line 3615 in c6bb902

for (uint32_t i = (next_address - address) / size; i < count - 1; i++) {

is executed at least once for the read value to be correct.

How the commit fixes the issue?

The execution of the NOP is moved into the loop (the last operation in the batch).

[JanMatCodasip]

@en-sc Thank you for explaining it in detail 👍. I have understood the original problem now.

LGTM!

[en-sc]

@JanMatCodasip, please, take a look once again. There was a conflict with aa9a3fa.

[aap-sc]

@en-sc you have an error here. size can be 8 here.

#7  0x000055555609d04c in buf_set_u32 (_buffer=0x5555569ca720 "\277", first=0, num=64, value=191)                           
    at /home/nikita/projects/openocd/src/helper/binarybuffer.h:43 
#8  0x00005555560b1d13 in read_memory_bus_v1 (target=0x5555569e5390, address=2147491969, size=8, count=2, 
    buffer=0x5555569ca720 "\277", increment=8) at ../openocd/src/target/riscv/riscv-013.c:3410
#9  0x00005555560b58c0 in read_memory (target=0x5555569e5390, address=2147491969, size=8, count=2, 
    buffer=0x5555569ca720 "\277", increment=8) at ../openocd/src/target/riscv/riscv-013.c:4397
#10 0x0000555555c9b5c8 in riscv_read_memory (target=0x5555569e5390, address=2147491969, size=8, count=2, 
    buffer=0x5555569ca720 "\277") at ../openocd/src/target/riscv/riscv.c:3088
#11 0x0000555555c2daee in target_read_memory (target=0x5555569e5390, address=2147491969, size=8, count=2, 
    buffer=0x5555569ca720 "\277") at ../openocd/src/target/target.c:1245

[JanMatCodasip]

(For reference, in case anyone wonders.)

This is being fixed in #1109