Feature: RV32I progbuf-based memory access support #2185

Please move this to the RISC-V debug header so it's not duplicated with the core architecture implementation

As I will need DM registers 0x20 and later 0x18, can I move them from riscv_debug.c? Otherwise I only minimally touch that file, only to hook mem_read functions.

diff --git a/src/target/riscv_debug.h b/src/target/riscv_debug.h
index 4aec5432..9c04cae2 100644
--- a/src/target/riscv_debug.h
+++ b/src/target/riscv_debug.h
@@ -181,6 +181,8 @@ typedef struct riscv_hart {
 #define RV_DM_DATA3 0x07U
 #define RV_DM_ABST_CTRLSTATUS 0x16U
 #define RV_DM_ABST_COMMAND 0x17U
+#define RV_DM_ABST_AUTO 0x18U
+#define RV_DM_PROGBUF_BASE 0x20U
 #define RV_DM_SYSBUS_CTRLSTATUS 0x38U
 #define RV_DM_SYSBUS_ADDR0 0x39U
 #define RV_DM_SYSBUS_ADDR1 0x3aU

There are 3 more DM registers mentioned in riscv_debug.c, but they come with bitfields, and I don't need them.

Yes, please do - that would be ideal.

Please move these #define's up to the top of the file so they're more easily discovered.

There should be no more #defines left if I move stuff to riscv_debug.h. Would you like me to remove the RV_EBREAK from riscv_debug.c:131? (between CSRR instructions and semihosting entry-exit NOPs)

It would be best if the instruction is moved either to the top of the file, or.. if you expect it'll wind up used in other RISC-V contexts such as riscv64.c or riscv_debug.c, into the header.

This function has essentially all the same notes as the read one above, so please consider all those notes as applying here too.

We would suggest moving the progbuf blocks here up out of the function - there's no particular reason to shield these static constant arrays from the rest of the implementation, but it does make discovering them more difficult. We greatly appreciate the comments defining what the encoded instructions are and mean though - that's greatly appreciated!

They were 4+1 slots long, I hoped to also use 8+1 slots initially, with content differing in multiple of instructions.
If the plan is to use 1+impebreak slot, then the arrays serve no purpose, in fact the second instructon in all 6 of them is identical. I would like to size-optimize the pointer to them, too (it can be a load from function-bottom literal constant pool). If I move them up, I would have to trust GCC to see they're only used once.

For a variety of reasons, you're already having to trust GCC to notice that - where they're defined in the file won't make a codegen difference.

We note this doesn't use the second instruction in each of the above buffers - so no automatic address increment.. if this is intentional, perhaps drop the extra instruction from them? Otherwise please adjust this code to do the right thing.

Auto increment is not guaranteed, as you say, by the Debug Spec some implementors could have omitted it. I can drop the extra instruction, bend over backwards and size-optimise this code; however it will be brought back in some form by the time of PR2225. If you intend to accept both, I don't see the benefit in undoing changes and then redoing it in the follow-up. I do see the benefit in shrinking the size-diff.

The benefit is in the evolutionary narative it creates in the commit history.. this doesn't use that functionality.. it writes one instruction into progbuf even though it guarantees at least 2 are available, it looks.. odd.. as it stands and thus our request. Removing the unused bits here and then introducing them in that other PR provides a more understandable evolution even though it might seem like more work.

We'd greatly prefer that this be RV_GPR_A1 so the assumption that they're next to each other isn't having to be made and the code says more what is intended, which isn't "the register after A0", but rather "the register A1".

Accepted. The macro now exists in recently merged PR2115, but again in another TU. What is the strategy, extract them to riscv_debug.h? Or move all new progbuf-related code from riscv32.c to riscv_debug.c so that riscv64.c can potentially benefit from it? (that's a larger change than I'd like)

Extract to the header. we can do the swizzling needed to make the memory access primitives available on 64-bit RISC-V in a follow-up PR. There's a lot more work involved in that swizzle than just moving this new code.

Given this is made of only constants, please just inline it in the riscv_dm_write() call below - we're not gaining anything from it being a const-marked variable like this.

The separate constant makes it more readable to me, the bitfield macros are very long and clang-format wants to soft-wrap them. I rely on the optimizing compiler to substitute the value, which is used only once. I can comply with the style used in the rest of functions calling riscv_dm_write(), but then it's harder for me to read the long argument and count the AAR modificators.

Please follow the style of the rest of the code.. unless you were naming this something more meaningful, it's just creating noise for us to maintain, and inviting people to violate the exact promise you're wanting to rely on with the compiler.

Why is there conditional compilation here? What code path actually needs to be taken and why? Please clean up so we can review the right half of the branch.

I first wrote it as a manual {DM reg Abstract Command} manipulation, looking at how other BMD code uses it, and then noticed It's almost a duplicate of riscv_csr_read(). The intent was to do it all without CSR access and secondary progbuf manipulation, dirtying its contents -- GPR A1 to DATA0 is supposed to always work via AAR. In the optimal case I could trust that progbuf content stays preserved between adjacent mem_read() calls.
If I can trust that any logic complexity in riscv_csr_read() does not fire or mess with this operation, then I can just delegate to it and have less code to maintain. Especially if error handling ends up being the same.

You're in 32-bit RISC-V land, none of the additonal complexity of the CSR function will fire. You're guaranteed a 32-bit access and thus the 64- and 128-bit checks fail.

Flipping it to #if 0 and reading RV_CSR_A1 I now get a DEBUG_TARGET spam of Reading 32-bit CSR 100b. But otherwise correct compare-sections data. How do I suppress that? Demote to DEBUG_PROTO, which riscv32_mem_read uses and which I would like to see instead? That also spams with riscv_dmi_read.

You.. don't.. and that's not a problem - for now, let BMDA "spam" the console, and we'll look at improving the diagnostics around CSR reads too help make it less obnoxious. They're different PR concerns.

Given the CSR write in the block above is just this call, does it not make mroe sense to use the function that exists to do the write?

Yes. I wanted to make sure than any errors only happen from my newly added code, and not from existing other functions. I can likewise replace the boilerplate of first write with a single call.

Please drop this line - we know it needs postexec (progbuf) execution, so there's no point to having this be here.

Because of additional modifier I will drop the commented-out call (which I can't use because I need postexec). I will note that they all look very similar.

U on the end of the constant please.

For 1 slot it should be >= 1U, and any autoincrement will be dealt with later.

The same note as before applies to all these defines - please move them to the top of the file with the rest of them.

Moved relevant defines up (discarded unused defines), added a couple comments to them.

U on the end of this and the next line's constant please.

Applied four s/1/1U/ as fixup+rebase.

Ditto here.