I've also added tests targeting this feature: riscv-software-src/riscv-tests#599

@aswaterman, @rtwfroody, would you kindly take a look?

@aswaterman Can you approve the workflows for this change?

Yep.

@rtwfroody would you kindly take a look again?

@rtwfroody ping)

@aswaterman could you please consider merging this? I already have tests for OpenOCD in riscv_tests repository that depend on this patch: pr599

There are some concerns from @wissygh that indicate that this may break compatibility (I'm not sure that there was one, to be fair) with rocket-chip project for some configurations of their designs. This compatibility was never intended to be in the scope of this MR, though. If this is an issue - someone could try to address this in a subsequent commit. I can participate too if there is a request for it.

@aswaterman could you please consider merging this? I already have tests for OpenOCD in riscv_tests repository that depend on this patch: pr599

There are some concerns from @wissygh that indicate that this may break compatibility (I'm not sure that there was one, to be fair) with rocket-chip project for some configurations of their designs. This compatibility was never intended to be in the scope of this MR, though. If this is an issue - someone could try to address this in a subsequent commit. I can participate too if there is a request for it.

What concerns me is whether Spike will continue to increase its use of dscratchX if similar issues arise in the future. Additionally, Spike is considered an important reference when designing the microarchitecture. What I'm trying to say is, is there a better way to solve this problem?

@wissygh

Additionally, Spike is considered an important reference when designing the microarchitecture.

The thing is that this patch extends existing functionality. The fact that the new ROM becomes incompatible with some older rocket-chip design is a direct consequence of that (we introduce a new micro architectural feature that should be taken into account).

What concerns me is whether Spike will continue to increase its use of dscratchX if similar issues arise in the future

I don't think that we require to increase the number of dsccrach registers in future. It could be possible to avoid it. However, I don't think that we can reduce the number of used scratch registers in this specific case.

1. It is possible to allocate an extra per-hart storage and store whatever information we need there. However, this will require us to modify the code of the simulator and re-design the way storage space for debug mode is allocated. I'm not sure if this can be classified as "better way", though.
2. Even if the above change is made we still need 2 things in order to operate: mhartid (to uniquely identify hart and it's associated storage) and the current state of of DCSR (to disable mprven). The latter allows for ROM code to perform access to physical addresses allowing to use per-hart storage as usual.

What I'm trying to say is, is there a better way to solve this problem?

Given the summary above I don't think there is. However, if in future we need to take into account some new issue - it should be possible to implement a solution that won't require more scratch registers.

I share @wissygh's concern but don't have a great solution. But one improvement I'd recommend is to use #ifdef USE_MPRVEN guards (or pick a better name if you have one) so that dscratch1 is only needed if the code is assembled with -DUSE_MPRVEN. I'm willing to merge it after we make that improvement.

I think you can avoid using dscratch1 by duplicating some code. You'd write something like:

if (dcsr.mprven) {
   clear_mprven();
   // do all the usual things, but make sure to set_mprven() in a few places where necessary
} else {
  // do all the usual things, don't touch mprven
}

Thinking of this more, the comment says we disable mprven to avoid address translation when fetching code from the debug_rom. What about fetching code while mprven is set? E.g. the few instructions before you disable mprven, and the fetching of instructions orchestrated pointed at by whereto?

Avoiding use of dscratch1 by restructuring the code would be an improvement, IMO.

I noticed that when virt2phys_mode is set to hw, OpenOCD assert dcsr.mprven before memory access and deassert dcsr.mprven after memory access through the progbuffer, respectively. Given that this method is already supported, is it still necessary to separately write to dcsr.mprven? @aap-sc @fk-sc @rtwfroody @aswaterman

riscv-collab/riscv-openocd#386

I noticed that when virt2phys_mode is set to hw, OpenOCD assert dcsr.mprven before memory access and deassert dcsr.mprven after memory access through the progbuffer, respectively. Given that this method is already supported, is it still necessary to separately write to dcsr.mprven?

When running OpenOCD with Spike, the simulator ignores writes to dcsr.mprven (it behaves like it hardwired to zero). Consequently, hardware-assisted virtual-to-physical address translation doesn't work properly, and "virtual" address accesses directly map to physical memory. OpenOCD’s automated mprven toggling thus has no effect in this environment. The patch corrects this behavior by allowing Spike to honor dcsr.mprven writes, enabling proper translation. This is the reason this MR exists. @wissygh

I noticed that when virt2phys_mode is set to hw, OpenOCD assert dcsr.mprven before memory access and deassert dcsr.mprven after memory access through the progbuffer, respectively. Given that this method is already supported, is it still necessary to separately write to dcsr.mprven?

When running OpenOCD with Spike, the simulator ignores writes to dcsr.mprven (it behaves like it hardwired to zero). Consequently, hardware-assisted virtual-to-physical address translation doesn't work properly, and "virtual" address accesses directly map to physical memory. OpenOCD’s automated mprven toggling thus has no effect in this environment. The patch corrects this behavior by allowing Spike to honor dcsr.mprven writes, enabling proper translation. This is the reason this MR exists. @wissygh

Yes, therefore I believe that Spike only needs to add support for the implementation of mprven and does not require modifications to the debug_rom.

Yes, therefore I believe that Spike only needs to add support for the implementation of mprven and does not require modifications to the debug_rom.

@wissygh

As soon as we add support for mprven we have to update debug rom because the debug rom itself performs memory accesses and expects these memory accesses to avoid memory translation

@rtwfroody, I will try to implement your idea. If successful, I’ll create a separate PR. For now, I’ll use the temporary #ifdef solution suggested by @aswaterman.

Regarding your concerns about instruction fetch: the RISC-V Privileged Specification (section 3.1.6.3, Memory Privilege in mstatus Register) states that MPRV should not affect instruction address translation. I’ve double-checked Spike’s implementation, and it behaves correctly.

@wissygh, are you okay with the approach suggested by @aswaterman?

I would strongly prefer to wait until you've gotten @rtwfroody's scheme working, rather than using the #ifdef scheme as a temporary solution.

I noticed that when virt2phys_mode is set to hw, OpenOCD assert dcsr.mprven before memory access and deassert dcsr.mprven after memory access through the progbuffer, respectively. Given that this method is already supported, is it still necessary to separately write to dcsr.mprven?

When running OpenOCD with Spike, the simulator ignores writes to dcsr.mprven (it behaves like it hardwired to zero). Consequently, hardware-assisted virtual-to-physical address translation doesn't work properly, and "virtual" address accesses directly map to physical memory. OpenOCD’s automated mprven toggling thus has no effect in this environment. The patch corrects this behavior by allowing Spike to honor dcsr.mprven writes, enabling proper translation. This is the reason this MR exists. @wissygh

Given that this method is already supported, is it still necessary to separately write to dcsr.mprven?

What I want to express here is that if the debugger is only capable of debugging hardware address translation by setting virt2phys_mode to hw, perhaps there's no need to modify the debug_rom. However, I realized that I might have overlooked an issue: the progbuffer's memory access instructions can also cause exceptions, which means the instruction to clear mprven might not be executed, and this could still affect the execution of memory access instructions in the debug_rom.

@wissygh, are you okay with the approach suggested by @aswaterman?

OK!

@wissygh

the progbuffer's memory access instructions can also cause exceptions, which means the instruction to clear mprven might not be executed, and this could still affect the execution of memory access instructions in the debug_rom.

Aside from that there is also "abstract memory access" command mechanism that does not use progbuf at all (just FYI).