[dv] CSR testbench fixes - #2465
Conversation
Fixes lowRISC#2242 Signed-off-by: Samuel Riedel <sriedel@lowrisc.org>
Signed-off-by: Samuel Riedel <sriedel@lowrisc.org>
Signed-off-by: Samuel Riedel <sriedel@lowrisc.org>
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document. By submitting this pull request comment, I am hereby confirming my acceptance of the terms of the CLA Document and my agreement to be legally bound by its terms. |
|
LGTM 👏🏻 |
| // Note: under Verilator, in_rst_ni is an inout that eval_initial forces to 0, | ||
| // so we rely solely on the DPI reset driver (dpi_rst_ni). |
There was a problem hiding this comment.
What is eval_initial?
|
|
||
| // Returns a per-byte mask for bytes whose write must be suppressed under | ||
| // SMEPMP rule 4b: MML=1, RLB=0, and the candidate byte has lock=1 with | ||
| // {X,W,R} in {0x4(X), 0x2(W), 0x6(W+X), 0x5(X+R)}. |
There was a problem hiding this comment.
I suspect the latest official release of the RISC-V Priv spec (version 20260120) doesn't have "SMEPMP rule 4b" as such anymore. Some content that may have been it seems to have been moved into a "Historical Rationale for Extensions" appendix (A.1.4.b), which doesn't seem as strong a thing to reference. Perhaps clarify or replace this reference?
There was a problem hiding this comment.
I found some other things confusing, but perhaps that was because I am had little prior knowledge of PMP.
- I was confused that if this only restricted locked cfgs then it seemed to be redundant w.r.t.
PmpCfgRegister::GetLockMask(), until I realised that this checked the value being written, rather than the existing value. Just me not catching the intended meaning of "candidate". - I was additionally confused for some time about why these particular combinations of permissions were included, until I read 3.1.19 "Adding a rule with executable privileges that either is M-mode-only or a locked Shared-Region..." and the table in 6.2 together closely.
A larger explanatory comment here would be nice to avoid any future misunderstanding, but ultimately anyone making changes here should have a good understanding of the spec (which I apparently didn't) already.
| register_value_ = (register_value_ & (lock_mask | suppress_mask)) | | ||
| (candidate & ~lock_mask & ~suppress_mask); |
There was a problem hiding this comment.
I don't think this is quite right. If both masks are all zeros (allow any bit to be cleared), then the existing register value will be entirely lost.
How about:
register_value_ &= (~newval | lock_mask | suppress_mask);
3 participants
Fix tb_cs_registers golden model and testbench:
@ZhongYic00 and @brdr-life, does this resolve #2242 for you?
Closes #2242