Add Zawrs extension #398
Add Zawrs extension #398
Conversation
ved-rivos
force-pushed
the
zawrs
branch
2 times, most recently
from
fa4acb6
to
1e5cbd4
Compare
|
|
||
| platform_start_wrs_timeout(if op == WRS_STO then true else false); | ||
|
|
||
| while reservation_set_valid() & not(interrupts_pending()) & not(platform_is_wrs_timed_out()) do (); |
There was a problem hiding this comment.
I'm not sure if the best way to handle this is via waiting in the actual execute function. WFI doesn't work like this (to be fair it doesn't work at all because there's no proper interface for injecting interrupts currently).
Maybe the model should be a state machine and the step() function handled the WFI and WRS states explicitly?
I dunno, we probably need to think about this properly. Maybe this is fine for now though since the platform callbacks are just hard-coded to time out immediately.
There was a problem hiding this comment.
The intent here is two folds:
- As a precise documentation of the ISA - this shows that the execution stalls while the instruction waits for these three conditions a) a snoop that invalidates the reservation set due to a store from another hart or a device b) an interrupt becomes pending c) the wait times out.
- Allows the platform test bench to inject these into the mode - a snoop invalidation or the platform specific timer timeout. This would also include advancing cycles and time.
We cannot step() as it is presently as we cannot fetch or execute instructions when stalled.
There was a problem hiding this comment.
Yeah I agree it is nice to document the stalls like this. But documentation isn't the only purpose of the model. I think we want to be a bit careful about this because this is the first bit of code in the model that assumes that step() can block.
We cannot step() as it is presently as we cannot fetch or execute instructions when stalled.
My feeling is that step() should really mean "step the model state", not "fetch and execute an instruction". At the moment those are exactly the same because none of the execute functions call platform callbacks to wait for an event, including WFI. If step() blocks you need threads in lots of situations (e.g. multiple harts, cosimulation), which are quite a pain in C++ and even worse in C.
But it's true that turning step() into an explicit state machine would make the code less clear. Hmm.
There was a problem hiding this comment.
Thats why I wrote "as it is presently". I dont think it would make the code less clear as a state machine. We would need instead of RETIRE_SUCCESS, something like STALL_UNTIL and the code should setup the conditions for "until". The model would then want the execution to resume after the STALL_UNTIL - STALL_UNTIL then becomes sort of like a swapcontext().
There was a problem hiding this comment.
something like STALL_UNTIL
Yeah exactly! We also use RETIRE_DEBUG_TRIGGER in Retired, and we should really have RETIRE_ILLEGAL too - see #412.
There was a problem hiding this comment.
there's no proper interface for injecting interrupts currently
#395 provides timer interrupts.
Spec: https://github.com/riscv/riscv-zawrs
Test: riscv-non-isa/riscv-arch-test#432