-
Notifications
You must be signed in to change notification settings - Fork 76
fix: implement Zawrs in IDL #1131
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
base: main
Are you sure you want to change the base?
Conversation
if (CSR[misa].H == 1) { | ||
if ((CSR[hstatus].VTW == 1'b1)) { | ||
if ((mode() == PrivilegeMode::VS) || (mode() == PrivilegeMode::VU)) { | ||
raise (ExceptionCode::VirtualInstruction, mode(), $encoding); |
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.
We need to account for the "implementation-specific bounded time limit" in:
When the TW (Timeout Wait) bit in mstatus is set and WRS.NTO is executed in any privilege mode other than M mode, and it does not complete within an implementation-specific bounded time limit, the WRS.NTO instruction will cause an illegal-instruction exception.
So, I suggest a couple of things:
- A new configuration parameter specifying that "bounded time limit" for NTO case.
- Some sort of parameterization for the
wait_on_reservation_set
function to accept that time bound for NTO case and some indicator of the STO case, possibly the same parameter if done carefully.
The wording in the spec seems to imply that the "short" duration for STO is not something formal / predicatable / parameterizable, and probably needs to be left undefined?
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 was thinking of adding it but I noticed that the spec also mentions an implementation specific time limit for WFI, but nothing specific was written for it in IDL.
in VS-mode, attempts to execute WFI when hstatus .VTW=1 and mstatus .TW=0, unless the instruction
completes within an implementation-specific, bounded time;
Therefore I didn't include it in Zawrs as well. But, I'll write it as it seems like the right approach.
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.
With this change I'll have to represent the permission checks within description of wait_on_reservation_set
, right?
Should I write it in code format or just simply describe when it'll fault? What do you suggest?
If we take the code-format approach, I could use the function read_mcycle
to keep a count of cycles and compare it with BOUNDED_TIME_LIMIT
.
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.
Yea these situations are tricky to model because (a) IDL has no concept of time and (b) implementations are allowed to do anything, including having "bounded amount time" be variable.
We can (should) try to make this apparent to a reader of the IDL code even if we may never be able to accurately model it. Something like:
if (mode() == PrivilegeMode::M) {
wait_on_reservation(...);
} else {
if (try_wait_on_reservation(...) == false) {
raise(...);
}
}
Within (try_)wait_on_reservation, it will have to eventually call a builtin to let the "SoC" decide what to do. We can have two variants of that, one for a bounded wait and one for an unbounded wait.
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.
So basically try_wait_on_reservation
for wrs.nto
could have a description like
Temporarily stall execution in a low-power state as long as:
a. The reservation set is valid
b. Doesn't exceed implementation defined BOUNDED_TIME_LIMIT
c. No pending interrupt is observed
Returns False if BOUNDED_TIME_LIMIT exceeded; else True
Please review the new changes. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1131 +/- ##
=======================================
Coverage 46.05% 46.05%
=======================================
Files 11 11
Lines 4942 4942
Branches 1345 1345
=======================================
Hits 2276 2276
Misses 2666 2666
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@henrikg-qc, this one needs ISS implementation of two builtins |
Implemented Zawrs in IDL (Issue #224). I am new to IDL and wrote this while keeping wfi as a reference.