Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add DCSR.MPRVEN support #1882

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion debug_rom/debug_rom.S
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,12 @@ _entry:
// This fence is required because the execution may have written something
// into the Abstract Data or Program Buffer registers.
fence
csrw CSR_DSCRATCH0, s0 // Save s0 to allow signaling MHARTID
csrw CSR_DSCRATCH0, s0 // Save s0 to allow signaling MHARTID

// We need to disable privilege modification to avoid
// address translation in debug_rom memory accesses
csrw CSR_DSCRATCH1, s1 // Save s1
csrrci s1, CSR_DCSR, DCSR_MPRVEN // Save DCSR and clear MPRVEN
Comment on lines +30 to +31
Copy link
Contributor

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?

Copy link
Author

@fk-sc fk-sc Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the use of register s1:
The register s1 is used within the debug ROM to save and restore the DCSR register, specifically to manage the MPRVEN bit. After the debug ROM execution, s1 is restored to its original value, ensuring that the debugger can access it as before. This approach maintains the integrity of s1 for debugger operations.

Clarification on MPRVEN bit management:
The MPRVEN bit is cleared within the debug ROM to prevent unwanted virtual address translations during ROM execution. However, it is crucial that this bit is set when the debugger initiates memory accesses. The debug ROM's role is to ensure a controlled environment, and the MPRVEN bit will have its intended effect when the debugger performs memory operations post-ROM execution.


// We continue to let the hart know that we are halted in order that
// a DM which was reset is still made aware that a hart is halted.
Expand All @@ -47,13 +52,16 @@ _exception:
// We need this in case the user tried an abstract write to a
// non-existent CSR.
csrr s0, CSR_DSCRATCH0
csrr s1, CSR_DSCRATCH1 // Restore s1
sw zero, DEBUG_ROM_EXCEPTION(zero) // Let debug module know you got an exception.
ebreak

going:
csrr s0, CSR_MHARTID
sw s0, DEBUG_ROM_GOING(zero) // When debug module sees this write, the GO flag is reset.
csrr s0, CSR_DSCRATCH0 // Restore s0 here
csrw CSR_DCSR, s1 // Restore DSCR
csrr s1, CSR_DSCRATCH1 // Restore s1
fence
fence.i
jalr zero, zero, %lo(whereto) // Debug module will put different instructions and data in the RAM,
Expand All @@ -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
Copy link
Contributor

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your feedback! I understand your concern about the debugger's ability to set bits like ebreakm and step in the DCSR register.

As I understand, the debugger modifies the DCSR only in whereto section (section for progbuf and abstract commands code). And we clear MPRVEN only inside of debug ROM which is not intersect with whereto. This means that restoring the entire DCSR register (including MPRVEN) should not interfere with the debugger's ability to set other bits like ebreakm and step during normal operation.

csrr s1, CSR_DSCRATCH1 // Restore s1
dret

// END OF ACTUAL "ROM" CONTENTS. BELOW IS JUST FOR LINKER SCRIPT.
Expand Down
22 changes: 12 additions & 10 deletions debug_rom/debug_rom.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
static const unsigned char debug_rom_raw[] = {
0x6f, 0x00, 0xc0, 0x00, 0x6f, 0x00, 0x00, 0x06, 0x6f, 0x00, 0x80, 0x03,
0x0f, 0x00, 0xf0, 0x0f, 0x73, 0x10, 0x24, 0x7b, 0x73, 0x24, 0x40, 0xf1,
0x23, 0x20, 0x80, 0x10, 0x03, 0x44, 0x04, 0x40, 0x13, 0x74, 0x14, 0x00,
0x63, 0x14, 0x04, 0x02, 0x73, 0x24, 0x40, 0xf1, 0x03, 0x44, 0x04, 0x40,
0x13, 0x74, 0x24, 0x00, 0x63, 0x18, 0x04, 0x02, 0x73, 0x00, 0x50, 0x10,
0x6f, 0xf0, 0x9f, 0xfd, 0x73, 0x24, 0x20, 0x7b, 0x23, 0x26, 0x00, 0x10,
0x6f, 0x00, 0xc0, 0x00, 0x6f, 0x00, 0x40, 0x07, 0x6f, 0x00, 0x00, 0x04,
0x0f, 0x00, 0xf0, 0x0f, 0x73, 0x10, 0x24, 0x7b, 0x73, 0x90, 0x34, 0x7b,
0xf3, 0x74, 0x08, 0x7b, 0x73, 0x24, 0x40, 0xf1, 0x23, 0x20, 0x80, 0x10,
0x03, 0x44, 0x04, 0x40, 0x13, 0x74, 0x14, 0x00, 0x63, 0x16, 0x04, 0x02,
0x73, 0x24, 0x40, 0xf1, 0x03, 0x44, 0x04, 0x40, 0x13, 0x74, 0x24, 0x00,
0x63, 0x1e, 0x04, 0x02, 0x73, 0x00, 0x50, 0x10, 0x6f, 0xf0, 0x9f, 0xfd,
0x73, 0x24, 0x20, 0x7b, 0xf3, 0x24, 0x30, 0x7b, 0x23, 0x26, 0x00, 0x10,
0x73, 0x00, 0x10, 0x00, 0x73, 0x24, 0x40, 0xf1, 0x23, 0x22, 0x80, 0x10,
0x73, 0x24, 0x20, 0x7b, 0x0f, 0x00, 0xf0, 0x0f, 0x0f, 0x10, 0x00, 0x00,
0x67, 0x00, 0x00, 0x30, 0x73, 0x24, 0x40, 0xf1, 0x23, 0x24, 0x80, 0x10,
0x73, 0x24, 0x20, 0x7b, 0x73, 0x00, 0x20, 0x7b
0x73, 0x24, 0x20, 0x7b, 0x73, 0x90, 0x04, 0x7b, 0xf3, 0x24, 0x30, 0x7b,
0x0f, 0x00, 0xf0, 0x0f, 0x0f, 0x10, 0x00, 0x00, 0x67, 0x00, 0x00, 0x30,
0x73, 0x24, 0x40, 0xf1, 0x23, 0x24, 0x80, 0x10, 0x73, 0x24, 0x20, 0x7b,
0x73, 0x90, 0x04, 0x7b, 0xf3, 0x24, 0x30, 0x7b, 0x73, 0x00, 0x20, 0x7b
};
static const unsigned int debug_rom_raw_len = 116;
static const unsigned int debug_rom_raw_len = 144;
3 changes: 3 additions & 0 deletions riscv/csrs.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1345,6 +1345,7 @@ dcsr_csr_t::dcsr_csr_t(processor_t* const proc, const reg_t addr):
ebreakvs(false),
ebreakvu(false),
v(false),
mprven(false),
cause(0),
ext_cause(0),
cetrig(0),
Expand Down Expand Up @@ -1374,6 +1375,7 @@ reg_t dcsr_csr_t::read() const noexcept {
result = set_field(result, DCSR_STEP, step);
result = set_field(result, DCSR_PRV, prv);
result = set_field(result, CSR_DCSR_V, v);
result = set_field(result, DCSR_MPRVEN, mprven);
result = set_field(result, DCSR_PELP, pelp);
return result;
}
Expand All @@ -1388,6 +1390,7 @@ bool dcsr_csr_t::unlogged_write(const reg_t val) noexcept {
ebreakvs = proc->extension_enabled('H') ? get_field(val, CSR_DCSR_EBREAKVS) : false;
ebreakvu = proc->extension_enabled('H') ? get_field(val, CSR_DCSR_EBREAKVU) : false;
v = proc->extension_enabled('H') ? get_field(val, CSR_DCSR_V) : false;
mprven = get_field(val, CSR_DCSR_MPRVEN);
pelp = proc->extension_enabled(EXT_ZICFILP) ?
static_cast<elp_t>(get_field(val, DCSR_PELP)) : elp_t::NO_LP_EXPECTED;
cetrig = proc->extension_enabled(EXT_SMDBLTRP) ? get_field(val, DCSR_CETRIG) : false;
Expand Down
1 change: 1 addition & 0 deletions riscv/csrs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,6 +697,7 @@ class dcsr_csr_t: public csr_t {
bool ebreakvs;
bool ebreakvu;
bool v;
bool mprven;
uint8_t cause;
uint8_t ext_cause;
bool cetrig;
Expand Down
2 changes: 1 addition & 1 deletion riscv/mmu.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ class mmu_t
{
return proc != nullptr
&& !(proc->state.mnstatus && !get_field(proc->state.mnstatus->read(), MNSTATUS_NMIE))
&& !proc->state.debug_mode
&& (!proc->state.debug_mode || get_field(proc->state.dcsr->read(), DCSR_MPRVEN))
&& get_field(proc->state.mstatus->read(), MSTATUS_MPRV);
}

Expand Down