-
Notifications
You must be signed in to change notification settings - Fork 882
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add DCSR.MPRVEN support #1882
base: master
Are you sure you want to change the base?
Add DCSR.MPRVEN support #1882
Conversation
I've also added tests targeting this feature: riscv-software-src/riscv-tests#599 |
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. * Decreases the number of required abstract commands to read virtual memory thus improving the user experience. 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 in 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 Signed-off-by: Farid Khaydari <[email protected]>
372448d
to
95af514
Compare
@aswaterman, @rtwfroody, would you kindly take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had thought that not using dscratch1 was a goal of this sample debug ROM, but I'll defer to @rtwfroody
@aswaterman Can you approve the workflows for this change? |
Yep. |
csrw CSR_DSCRATCH1, s1 // Save s1 | ||
csrrci s1, CSR_DCSR, DCSR_MPRVEN // Save DCSR and clear MPRVEN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This clobbers s1. Now the debugger can't access it anymore.
Also you're going through a bunch of effort here to clear dcsr.mprven while in debug mode, but if you're clearing it then when does the bit actually have an effect?
@@ -64,6 +72,8 @@ _resume: | |||
csrr s0, CSR_MHARTID | |||
sw s0, DEBUG_ROM_RESUMING(zero) // When Debug Module sees this write, the RESUME flag is reset. | |||
csrr s0, CSR_DSCRATCH0 // Restore s0 | |||
csrw CSR_DCSR, s1 // Restore DSCR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can the debugger set bits in dcsr if here you overwrite it with a previously saved value? The debugger has to be able to set bits like ebreakm and step.
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:
Commit's changes:
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: