- Add S-mode debug CSR for debug #60
Conversation
chapter2.adoc
Outdated
|
|
||
| ==== Debug Control and Status (dcsr) | ||
| The `sdcsr` register tracks the current debug state of the hart, formatted as shown in <<sdcsr32>> and <<sdcsr64>>. When `mdbgen` is 0 and `sdedbgalw` is set to 1, the `prv` and `v` fields indicate the the privilege level at which the hart was executing before entering Debug Mode. Meanwhile, the `sdpc` is updated with the address of the next instruction to executed upon entry into Debug Mode. When resuming from Debug Mode, the privilege level of the hart is restored to the values in `prv` and `v`, while the hart's PC is updated wth the address in `sdpc`. |
There was a problem hiding this comment.
I would say this differently.
The
sdcsr,sdpc,sdscratch0, andsdscratch1registers are accessible only from Debug Mode, and provide supervisor access todcsr,dpc,dscratch0, anddscratch1, respectively. Thedscratch0anddscratch1registers are optional, andsdscratch0andsdscratch1are only required ifdscratch0anddscratch1are implemented.The
sdcsrregister provides masked access todcsr, to prevent supervisor access to machine state. Whendcsris accessed throughsdcsr, the following state modifications apply:
mprvenandebreakmare read-only 0prv[1]is read-only 0, which prevents prv from being set to M-mode- <...>
This way you aren't duplicating fields defined in the Debug spec, which risks the two specs getting out of sync. In fact, that already happened, you didn't include the new dcsr.cetrig bit. But it also makes clear that sdcsr is simply a masked alias of dcsr, not a new physical register. Much the way stselect is an alias (albeit unmasked) of tselect. So you only have to define how accesses to sdcsr differ from accesses to dcsr, you don't need to name all the fields that are accessed normally (e.g., v, cause, ebreak{s,u,vs,vu}, etc).
There was a problem hiding this comment.
I think sdpc, sdscratch* should have separate physical registers unlike sdcsr. Assuming there is lifecycle control, the contents in dpc, dscratch* might be leaked to S-mode debugger. cetrig is masked in sdcsr on purpose, since it shouldn't be configured by S-mode. I would say any new field in dcsr need requires revisit of sdcsr, and it is equivalent to have a diagram to show unmasked field or describe the masked fields in narrative words.
There was a problem hiding this comment.
Hmm, I have to think about that leakage case. So your worry is that an M-mode external debugger may populate dpc and dscratch*, then later an S-mode external debugger may be able to read those values if sdpc and sdscratch* are just aliases? So there is no reset between the use of the two debuggers? I would think a reset would be required for a change to the lifecycle value, but maybe not for an M-mode change to sdedbgalw. And I guess M-mode can't then clear dpc/dscratch*, since that can only be done in debug mode. Though won't entering debug mode always overwrite dpc? So I don't think that one could leak anything. But I suppose in theory dscratch* could.
Maybe separate sdscratch* regs is reasonable, though it seems inefficient. We could also say that changes to sdedbgalw reset dscratch*, not sure if that's better though.
There was a problem hiding this comment.
As for the sdcsr field specification, I guess you can do it either way, but if you plan to list all the unmasked fields then you probably want a non-normative comment describing why the remaining fields were masked. That way it's clear which fields you have considered, in case future dscr fields are added.
There was a problem hiding this comment.
Maybe separate sdscratch* regs is reasonable, though it seems inefficient. We could also say that changes to sdedbgalw reset dscratch*, not sure if that's better though.
I think it makes sense to clear the scratch register whenever mdbgen or sdedbalw is cleared. Beside the scenario where the M-mode debugger is deprivileged to S-mode, there could also be cases where multiple supervisor domains are debuggable while others are not. Therefore, it would be more secure to clear the scratch registers during context switch and requires no separate physical registers anymore.
@pdonahue-ventana do you think this will break any debug use cases?
There was a problem hiding this comment.
Makes me wonder why we ever standardized dscratch* in the first place.
Similarly, the dret instruction was standardized in the normative part of Debug 0.13. I argued in 2020 that this was a microarchitecture thing so we moved dret to the appendix in 1.0.
The dscratch* CSRs might also be details from the initial implementation that were incorrectly elevated to architecture status, though nscratch does kind of make them available architecturally.
There was a problem hiding this comment.
I agree with what you say. I just think that we should think about how you might do a ROM-based implementation. We shouldn't define the architecture in a way that somehow prevents ROM-based implementations. Here's one way to do it:
- define a custom CSR romcontrol that is only accessible in Debug mode (similar to dcsr/dpc/dscratch)
- software can clear romcontrol[0] but cannot set it. Hardware can set it.
- romcontrol[0]=1 means that debug mode can access any CSRs that the ROM needs to access (e.g. mhartid, dscratch0) and it ignores MPRV.
- romcontrol[0]=0 means that debug mode cannot access those CSRs and MPRV behaves as defined by mprven.
- upon entry to debug mode, hardware sets romcontrol[0] (so the ROM can access what it needs)
- ROM does a
csrrci x0, romcontrol, 1right before jumping to the program buffer- then the program buffer cannot access these things and it also cannot elevate its own permission due to the rule above about writing romcontrol
Of course, this is just a thought exercise and it's not mandatory to implement it like this. The only important thing is that it's possible to have a ROM-based implementation.
If you agree with the above then I don't think that we need smhartid and we probably don't need sdscratch*. What do you think?
One note on this proposal: this would mean that the hart would need to recognise when it leaves the program buffer code to go back to the ROM park loop, and at that point set romcontrol[0] back to 1. Perhaps that could be added to the ebreak behaviour in ROM-based implementations with program buffers?
There was a problem hiding this comment.
You're right. I was implicitly considering the ebreak to be a re-entry to debug mode in the fifth bullet. It's kind of like when you're in M-mode and you execute an ebreak and you trap to the M-mode handler. Of course, different implementations may do things differently and the only important thing is that the bit gets set whenever you go to the park loop.
There was a problem hiding this comment.
IIUC, the romcontrol and its behavior shall not be normative part, it is very implementation-specific. I assume it can be included in appendix as an implementation suggestion.
There was a problem hiding this comment.
I think we all agree that this CSR based approach is just thought exercise to understand ROM-based execution is possible. There are also other ways and they are implementation specific. They will not be in the normative part of the spec.
chapter2.adoc
Outdated
|
|
||
| The dcsr is always accessible in Debug Mode. The access rules for field `prv` and `v` are addressed in subsection <<prvvacc>>. Beside `prv` and `v`, the access condition of remaining fields are listed in the following table. | ||
| When the access conditions are met, they are read/write accessible. When access is disallowed, writes to these fields are ignored, and reads return zero. | ||
| The `sdscratch0` and `sdscratch1` are optional scratch registers. |
AoteJin
requested review from
bcstrongx,
pdonahue-ventana and
gokhankaplayan
- Remove sdcsr64 - Add placeholder in appendix for exe-based imp
chapter2.adoc
Outdated
| @@ -148,10 +148,10 @@ The `sdcsr`, `sdpc` provides supervisor read/write access to the `dcsr`, `dpc`. | |||
| | 0xaaa | sdpc | Supervisor debug program counter. | |||
| |============================================================================================ | |||
|
|
|||
| The `sdcsr` is a subset of the `dcsr` formatted as shown in <<sdcsr32>> and <<sdcsr64>>, while the `sdpc` has full access to the `dpc`. | |||
| The `sdcsr` register exposes a subset of the `dcsr`, formatted as shown in <<sdcsr32>>, while the `sdpc` provides full access to the `dpc`. | |||
There was a problem hiding this comment.
Nit: I would only put "the" in front of a CSR name if the name is followed by "register". So one could refer to "the dcsr register", or simply to dcsr.
chapter3.adoc
Outdated
| @@ -57,7 +57,7 @@ Trusted entities like RoT should configure IOPMP or equivalent protection before | |||
|
|
|||
| === Security Fault Error Reporting | |||
|
|
|||
| A dedicated error code, security fault error (cmderr 6), is included in `cmderr` of abstractcs (at 0x16 in Debug Module). Misconfigurations of the dcsr and issuance of abstract commands under disallowed circumstance can signify such an error. Additionally, the bus security fault error (sberror 6) is introduced in `sberror` of sbcs (at 0x38 in Debug Module) to denote errors related to system bus access. | |||
| A dedicated error code, security fault error (cmderr 6), is included in `cmderr` of abstractcs (at 0x16 in Debug Module). Issuance of abstract commands under disallowed circumstance sets `cmderr` to 6. Additionally, the bus security fault error (sberror 6) is introduced in `sberror` of sbcs (at 0x38 in Debug Module) to denote errors related to system bus access. | |||
There was a problem hiding this comment.
Missing some backticks on register names (abstractcs, sbcs). CSR field names don't need to be in backticks, they are generally all caps but the debug spec predates this convention. So fine to keep the backticks on those if you prefer that.
|
Thanks all for you valuable inputs! Merge and close the PR. |
No description provided.