Add Sstc extension #570
Add Sstc extension #570
Conversation
|
This is a rebase of #395 by @ved-rivos. The model structure has changed a bit so it required fairly extensive changes. I also added a command line flag. |
|
Yes, I think having a |
Timmmm
force-pushed
the
user/timh/sstc
branch
from
25ebc9e
to
ef365f8
Compare
|
☝️ rebased and fixed a missing |
Timmmm
force-pushed
the
user/timh/sstc
branch
from
ef365f8
to
b0a1507
Compare
|
@billmcspadden-riscv any idea who we could get to review this from a spec correctness point of view? |
Timmmm
force-pushed
the
user/timh/sstc
branch
from
b0a1507
to
6c6fe58
Compare
|
Rebased to handle the CSR rejigging. I had to move some code around to make the dependencies work. I also removed the We've been using this code for a while now with no issues found. It seems like @jscheid-ventana was the original author of the spec - if you could take a look that would be great! Otherwise I think we should just merge this. |
Timmmm
force-pushed
the
user/timh/sstc
branch
from
6c6fe58
to
471a1b4
Compare
model/riscv_sstc.sail
Outdated
| function clause is_CSR_defined(0x14D) = extensionEnabled(Ext_S) & extensionEnabled(Ext_Sstc) | ||
| function clause is_CSR_defined(0x15D) = extensionEnabled(Ext_S) & extensionEnabled(Ext_Sstc) & xlen == 32 | ||
|
|
||
| function clause read_CSR(0x14D) = stimecmp[sizeof(xlen) - 1 .. 0] |
There was a problem hiding this comment.
Now we have this commit, I think this can just be xlen - 1?
There was a problem hiding this comment.
Good spot - updated.
model/riscv_sys_control.sail
Outdated
| function check_Stimecmp(csr : csreg, p : Privilege) -> bool = { | ||
| // Check if it is not stimecmp. | ||
| if csr != 0x14D & csr != 0x15D | ||
| then return true; |
There was a problem hiding this comment.
Nitpick: I think this would look better with the then on the preceding line like:
if csr != 0x14D & csr != 0x15D then {
return true
};
or maybe with the whole if as a single line.
There was a problem hiding this comment.
I never know how to format these - where's that autoformatter? :-D
I changed it to a single line.
|
I left some minor comments, but overall I also think this can just be merged. |
| @@ -61,8 +70,12 @@ function check_seed_CSR (csr : csreg, p : Privilege, isWrite : bool) -> bool = { | |||
| function check_CSR(csr : csreg, p : Privilege, isWrite : bool) -> bool = | |||
There was a problem hiding this comment.
Not for this PR, but wouldn't this be cleaner as a scattered function?
There was a problem hiding this comment.
I think this one shouldn't be a scattered function because check_CSR_access() is always the same, but the other functions should be moved into is_CSR_defined(), something like this:
function check_CSR(csr : csreg, p : Privilege, isWrite : bool) -> bool =
check_CSR_access(csrAccess(csr), csrPriv(csr), p, isWrite) & is_CSR_defined(csr, p, isWrite)
Or we could potentially leave isWrite just for the seed CSR. It's the only one that needs it and unfortunately it is actually specified that way:
Attempts to access the seed CSR using a read-only CSR-access instruction (CSRRS/CSRRC with rs1=x0 or CSRRSI/CSRRCI with uimm=0) raise an illegal instruction exception
I'll make a PR to clean this up after this one.
This adds the stimecmp[h] CSRs. It is enabled by default. The code is slightly unfortunately structured to handle CSRs that don't fit the standard permission checks. I added a TODO to improve it. Co-authored-by: Tim Hutt <timothy.hutt@codasip.com>
Timmmm
force-pushed
the
user/timh/sstc
branch
from
471a1b4
to
81287b2
Compare
Timmmm
added
the
will be merged
This adds the stimecmp[h] CSRs. It is enabled by default.
The code is slightly unfortunately structured to handle CSRs that don't fit the standard permission checks. I added a TODO to improve it.