Implement S-mode traps#884
Conversation
tilk
added
enhancement
qbojj
force-pushed
the
feat-s-mode-traps
branch
3 times, most recently
from
f6a1859 to
22dd467
Compare
qbojj
force-pushed
the
feat-s-mode-traps
branch
from
22dd467 to
e5d8da4
Compare
Benchmarks summaryPerformance benchmarks
You can view all the metrics here. Synthesis benchmarks (basic)
Synthesis benchmarks (full)
|
Benchmarks summaryPerformance benchmarks
You can view all the metrics here. Synthesis benchmarks (basic)
Synthesis benchmarks (full)
|
Benchmarks summaryPerformance benchmarks
You can view all the metrics here. Synthesis benchmarks (basic)
Synthesis benchmarks (full)
|
Benchmarks summaryPerformance benchmarks
You can view all the metrics here. Synthesis benchmarks (basic)
Synthesis benchmarks (full)
|
This comment has been minimized.
This comment has been minimized.
awariac
left a comment
awariac
left a comment
There was a problem hiding this comment.
Very nice work!
3/4 part of review - some comments and discussions:
| if self.medelegh: | ||
| m.d.av_comb += edeleg.eq(Cat(self.medeleg.read(m).data, self.medelegh.read(m).data)) | ||
| else: | ||
| m.d.av_comb += edeleg.eq(self.medeleg.read(m).data) |
There was a problem hiding this comment.
I think we need some wraper for RV32 to group *h registers together and provide single read.
Out of scope here.
| @def_method(m, self.get_trap_target_priv) | ||
| def _(cause): |
There was a problem hiding this comment.
I don't like that its feeding back already computed cause (but only for interrupts), but I don't have ideas how to improve it. I see this is necessary for exceptions, and to be done at last possible point.
Some things I was thinking about - maybe trap_entry should return target priv instead of requiring it? Or maybe this would fit more into ExceptionInformationRegister that we need to call too?
The split about ways of exceptions and interrupts is probelmatic here.
If we don't come up with any better ideas, this could be left as-is.
Also in case of interrupts this is duplicating computation of top interrupt. Could this be unified?
There was a problem hiding this comment.
I have redone the interface - now entry returns the privilege. There is still duplication of interrupt target logic - I was wandering if we could just have it saved like interrupt_cause, but I'm not sure if it would not be some sort of TOC-TOU broblem
There was a problem hiding this comment.
I'm also a bit worried about TOCTOU issues here.
The only part I was referring to was that we already do selected_pending computation based on m_interrupt_insert and that's equivalent to the code here.
get_interrupt_cause happens at the same time in the retirement as trap_entry and represents the state in the same cycle.
However, I have no idea how to connect that cleanly, and hardware cost is not that large either (one xlenwide mux).
The retirement implementation could change in the future as well.
It's fine to leave it as its now, its much better.
| JumpComponent(), | ||
| ExceptionUnitComponent(), | ||
| PrivilegedUnitComponent(), | ||
| PrivilegedUnitComponent(supervisor_enable=True), |
There was a problem hiding this comment.
It's not very convenient; this should be inferred form gen_params
There was a problem hiding this comment.
I had a problem with get_decoder_manager not getting gen params
There was a problem hiding this comment.
Huh. The mechanism with get_decoder_manager was designed with more local effects in mind (e.g. ALU: parameter added -> new instructions supported -> new extension automatically inferred). I see two ways of avoiding redundancy in config:
- Treat this like extensions - declaring
supervisor_enableon anyPrivilegedUnitautomatically makes the whole core support supervisor mode. - Pass
gen_paramstoget_decoder_manager, and therefore allow the set of supported instructions to change after enabling supervisor mode in the configuration.
Don't have a strong opinion on this, maybe I like the second option slightly more.
There was a problem hiding this comment.
second one would be more consistent with user_mode.
Maybe it could accept core_config instead of gen_params?
The problem is gen_params's ISA string is determined from FU configs, so that's kind of circular dependency. It's not a problem in this case, but it could create problems for other units if we add this to standard interface.
@tilk what do you think about that?
There was a problem hiding this comment.
You are right about the circular depdendency, passing the configuration seems like the best choice here.
There was a problem hiding this comment.
the problem is that this object is the part of the configuration...
There was a problem hiding this comment.
ok, I think you can add assert to PrivilegedUnitComponent that supervisor_mode == gen_params.supervisor_mode and maybe we will come up with something better later
|
there are also architectural tests for supervisor mode that need to be enabled. you can do it here on in other PR, but its worth checking for correctness |
This comment has been minimized.
This comment has been minimized.
|
There are some macros in |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I think I will leave riscof to another PR |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Benchmarks summaryPerformance benchmarks
You can view all the metrics here. Synthesis benchmarks (basic)
Synthesis benchmarks (full)
|
| JumpComponent(), | ||
| ExceptionUnitComponent(), | ||
| PrivilegedUnitComponent(), | ||
| PrivilegedUnitComponent(supervisor_enable=True), |
There was a problem hiding this comment.
ok, I think you can add assert to PrivilegedUnitComponent that supervisor_mode == gen_params.supervisor_mode and maybe we will come up with something better later
| @def_method(m, self.get_trap_target_priv) | ||
| def _(cause): |
There was a problem hiding this comment.
I'm also a bit worried about TOCTOU issues here.
The only part I was referring to was that we already do selected_pending computation based on m_interrupt_insert and that's equivalent to the code here.
get_interrupt_cause happens at the same time in the retirement as trap_entry and represents the state in the same cycle.
However, I have no idea how to connect that cleanly, and hardware cost is not that large either (one xlenwide mux).
The retirement implementation could change in the future as well.
It's fine to leave it as its now, its much better.
|
It looks like S-mode changes themselves pass riscof tests #921. The only problem seems to be the Zifencei |
3 participants
This builds up on #878 and implements trap handling.
This PR implements:
SRETand blankSFENCE.VMA