[riscv] refactor functions that register read/write via progbuf

[aap-sc]

The motivation for this refactor is to fixup error handling for some corner cases. These *_progbuf functions unconditionally attempt to cache S0 register and only then perform a bunch of extra checks to figure out if the requested register is a valid one. The problem is that there are few corner cases when _*progbuf functions could receive a GPR as an input. For example, an abstract read could fail (for whatever reason) leading to infinite recursion:

save S0 -> read S0 -> save S0 -> read S0 -> ...

The case described above could be fixed by adding extra sanitity checks, however I decided to make these functions more modular since I find self-contained functions easier to read.

Change-Id: I01f57bf474ca45ebb67a30cd4d8fdef21f307c7d

[aap-sc]

The issue mentioned above can be reproduced on spike if #900 is NOT applied (apparently, #900 hides this problem).

Reproduction (I've tried to reduce it and this is what I've got):

1. Target configuration file:

proc init_targets {} {
        jtag newtap riscv tap -irlen 5
        target create cpu0.riscv riscv -endian little -chain-position riscv.tap -coreid 0
        target create cpu1.riscv riscv -endian little -chain-position riscv.tap -coreid 1
}

2. Test script:

proc run_test {} {
  foreach t [target names] {
    targets $t
    halt
  }
  foreach t [target names] {
    targets $t
    resume
  }
  foreach t [target names] {
    targets $t
    halt
  }
  # entry point
  reg pc 0x80000000
  shutdown
}

3. Spike command line:

# make sure that entry point of the elf is at 0x80000000
spike \
  --rbb-port=33627 \
  --isa=rv64gc \
  --dm-progsize=6 \
  --dm-sba=0 \
  --dm-abstract-rti=0 \
  -p4 \
  -m0x80000000:0xa00000 \
  <some_elf_file>

4. OpenOCD invocation:

openocd -c "adapter driver remote_bitbang" -c "remote_bitbang port 33271" -c "remote_bitbang host 127.0.0.1" \
  -f config.cfg  -d2 -c init -f run_test.tcl -c run_test
openocd -c "adapter driver remote_bitbang" -c "remote_bitbang port 33271" -c "remote_bitbang host 127.0.0.1" \
  -f config.cfg  -d2 -c init -f run_test.tcl -c run_test

During the second invocation OpenOCD will crash due to stack overflow (because of infinite recursion).

[aap-sc]

Note: the intention of this patch is only to fix infinite recursion. The reason we have such a situation on spike with no apparent reason is another matter.

[JanMatCodasip]

I will need to return to this one again for a more thorough review. However, overall, this looks very good.

These *_progbuf functions unconditionally attempt to cache S0 register and only then perform a bunch of extra checks to figure out if the requested register is a valid one. The problem is that there are few corner cases when _*progbuf functions could receive a GPR as an input.

I fully agree. The *_progbuf functions should refuse invalid input argument (in this case, unsupported register type) at the very beginning, before attempting any further actions.

I also support the split of the original register_{read,write}_progbuf to the smaller functions.

[JanMatCodasip]

I tried to reword this to make it more understandable for users:

Suggested change

	LOG_ERROR("Unsupported register: %s", gdb_regno_name(number));
	LOG_TARGET_ERROR(target, "Write to register %s via program buffer not supported.", gdb_regno_name(number));

[EccoTheDolphin]

yes, will address. Thanks!

[aap-sc]

addressed, though, I've changed the message a little bit.

[JanMatCodasip]

(For clarity of the message:)

Suggested change

	LOG_TARGET_ERROR(target, "Unsupported register: %s", gdb_regno_name(number));
	LOG_TARGET_ERROR(target, "Reading register %s via program buffer is not supported.", gdb_regno_name(number));

[aap-sc]

this one is addressed too

[timsifive]

Looks good overall.