riscv-software-src
Add DCSR.MPRVEN support
Adds DCSR.MPRVEN bit support, as specified in RISC-V External Debug Support Version 1.0.0-rc4 (https://github.com/riscv/riscv-debug-spec/releases/tag/1.0.0-rc4, see 4.9.1 Debug Control and Status).
This bit allows to enable hardware virtual address translation when memory access is initiated by the debugger (see 4.1 Debug Mode, clause 2).
This change:
- Increases debug specification coverage, allows more detailed testing of external debuggers with Spike.
- DCSR.MPRVEN decreases the number of required abstract commands to read virtual memory.
Commit's changes:
- Added MPRVEN field to DCSR register
- Updated debug_rom.S to turn off MPRVEN while executing ROM
To avoid unwanted address translation while debug_rom.S executed DCSR.MPRVEN bit has to be cleared on entry and restored on exit.
Updated version of debug_rom.S does the following:
- On _entry: clears DCSR.MPRVEN bit, stores previous DCSR value to S1 and stores previous S1 value to DSCRATCH01
- On _exception: restores S1 value from DSCRATCH01
- On _resume/going: restores S1 and DCSR values
I've also added tests targeting this feature: https://github.com/riscv-software-src/riscv-tests/pull/599
@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.
- 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.
- 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
https://github.com/riscv-collab/riscv-openocd/pull/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.
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
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).
@rtwfroody, @aswaterman, @wissygh, @aap-sc, what do you think about following proposal: Instead of relying on the dscratch register, could we allocate a dedicated memory region to track the MPRVEN bit state for each hart? It is going to be similar to DEBUG_ROM_FLAGS
That seems similarly hacky, given that we already believe there is a valid solution that adds no new architectural state.
@rtwfroody, @aswaterman, @wissygh, @aap-sc, I have updated the implementation. Could you please review it? Now, the S0 register is not clobbered, and the MPRVEN bit is cleared before every memory access and restored afterward
Thank you for taking the time to restructure this in a way that avoids dependence on
dscratch1. Looks good to me.Could someone else (@wissygh or @rtwfroody) provide another pair of eyes on the debug ROM changes, then ping me when it is ready to merge?
LGTM.