Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 S-mode debug CSR for debug #60
- Add S-mode debug CSR for debug #60
Changes from 1 commit
ba58c30
8147fce
714f4e3
90ed221
dc5d161
cc346dc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 would say this differently.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to clear the scratch register whenever
mdbgen
orsdedbalw
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
See comment above