Skip to content

[dv] CSR testbench fixes #2465

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.

Sign up for GitHub

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

Open
SamuelRiedel wants to merge 3 commits into
base: master
Choose a base branch
from
+49 −7
Open

[dv] CSR testbench fixes #2465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ce31521
[dv] Implement SMEPMP write-suppression rule 4b in tb
SamuelRiedel Jul 29, 2026
f33a30c
[tb] Fix rst_ni perpetually low under Verilator
SamuelRiedel Jul 29, 2026
fedd44d
[dv] Allow modifying SMEPMP when RLB is set
SamuelRiedel Jul 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 42 additions & 7 deletions dv/cs_registers/model/base_register.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,12 +151,34 @@ uint32_t PmpCfgRegister::GetLockMask() {
return lock_mask;
}

// 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)}.

@elliotb-lowrisc elliotb-lowrisc Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@elliotb-lowrisc elliotb-lowrisc Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found some other things confusing, but perhaps that was because I am had little prior knowledge of PMP.

  1. 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".
  2. 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.

uint32_t PmpCfgRegister::GetMmlSuppressMask(uint32_t candidate) {
BaseRegister *mseccfg = GetRegisterFromMap(kCSRMSeccfg);
assert(mseccfg);
uint32_t mseccfg_val = mseccfg->RegisterRead();
if (!(mseccfg_val & kMSeccfgMml) || (mseccfg_val & kMSeccfgRlb))
return 0;
uint32_t suppress_mask = 0;
for (int i = 0; i < 4; i++) {
uint8_t byte_val = (candidate >> (8 * i)) & 0xFF;
uint8_t xwr = byte_val & 0x7;
if ((byte_val & 0x80) &&
(xwr == 0x4 || xwr == 0x2 || xwr == 0x6 || xwr == 0x5)) {
suppress_mask |= (0xFFu << (8 * i));
}
}
return suppress_mask;
}

uint32_t PmpCfgRegister::RegisterWrite(uint32_t newval) {
uint32_t lock_mask = GetLockMask();
uint32_t read_value = register_value_;
uint32_t suppress_mask = GetMmlSuppressMask(newval);

register_value_ &= lock_mask;
register_value_ |= (newval & ~lock_mask);
register_value_ &= (lock_mask | suppress_mask);
register_value_ |= (newval & ~lock_mask & ~suppress_mask);
register_value_ = HandleReservedVals(register_value_);

return read_value;
Expand All @@ -165,8 +187,10 @@ uint32_t PmpCfgRegister::RegisterWrite(uint32_t newval) {
uint32_t PmpCfgRegister::RegisterSet(uint32_t newval) {
uint32_t lock_mask = GetLockMask();
uint32_t read_value = register_value_;
uint32_t candidate = register_value_ | (newval & ~lock_mask);
uint32_t suppress_mask = GetMmlSuppressMask(candidate);

register_value_ |= (newval & ~lock_mask);
register_value_ |= (newval & ~lock_mask & ~suppress_mask);
register_value_ = HandleReservedVals(register_value_);

return read_value;
Expand All @@ -175,8 +199,11 @@ uint32_t PmpCfgRegister::RegisterSet(uint32_t newval) {
uint32_t PmpCfgRegister::RegisterClear(uint32_t newval) {
uint32_t lock_mask = GetLockMask();
uint32_t read_value = register_value_;
uint32_t candidate = register_value_ & (~newval | lock_mask);
uint32_t suppress_mask = GetMmlSuppressMask(candidate);

register_value_ &= (~newval | lock_mask);
register_value_ = (register_value_ & (lock_mask | suppress_mask)) |
(candidate & ~lock_mask & ~suppress_mask);
Comment on lines +205 to +206

@elliotb-lowrisc elliotb-lowrisc Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

register_value_ = HandleReservedVals(register_value_);

return read_value;
Expand Down Expand Up @@ -204,11 +231,19 @@ uint32_t PmpCfgRegister::HandleReservedVals(uint32_t cfg_val) {
}

uint32_t PmpAddrRegister::GetLockMask() {
BaseRegister *mseccfg = GetRegisterFromMap(kCSRMSeccfg);
assert(mseccfg);

// When RLB=1, locked entries can still be modified (SMEPMP).
if (mseccfg->RegisterRead() & kMSeccfgRlb) {
return 0;
}

// Calculate which region this is
uint32_t pmp_region = (register_address_ & 0xF);
// Form the address of the corresponding CFG register
uint32_t pmp_cfg_addr = 0x3A0 + (pmp_region / 4);
// Form the address of the CFG registerfor the next region
// Form the address of the CFG register for the next region
// For region 15, this will point to a non-existant register, which is fine
uint32_t pmp_cfg_plus1_addr = 0x3A0 + ((pmp_region + 1) / 4);
uint32_t cfg_value = 0;
Expand All @@ -225,8 +260,8 @@ uint32_t PmpAddrRegister::GetLockMask() {
// Shift to the relevant bits in the CFG registers
cfg_value >>= ((pmp_region & 0x3) * 8);
cfg_plus1_value >>= (((pmp_region + 1) & 0x3) * 8);
// Locked if the lock bit is set, or the next region is TOR
if ((cfg_value & 0x80) || ((cfg_plus1_value & 0x18) == 0x8)) {
// Locked if own L=1, or next region is BOTH locked (L=1) and TOR mode.
if ((cfg_value & 0x80) || ((cfg_plus1_value & 0x98) == 0x88)) {
return 0xFFFFFFFF;
} else {
return 0;
Expand Down
1 change: 1 addition & 0 deletions dv/cs_registers/model/base_register.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ class PmpCfgRegister : public BaseRegister {
uint32_t RegisterClear(uint32_t newval);

private:
uint32_t GetMmlSuppressMask(uint32_t candidate);
uint32_t HandleReservedVals(uint32_t cfg_val);
const uint32_t raz_mask_ = 0x9F9F9F9F;
};
Expand Down
6 changes: 6 additions & 0 deletions dv/cs_registers/tb/tb_cs_registers.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,13 @@ module tb_cs_registers #(
//-----------------
// Allow reset to be toggled by the top-level (in Verilator)
// or a DPI call
// 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).
Comment on lines +46 to +47

@elliotb-lowrisc elliotb-lowrisc Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is eval_initial?

`ifdef VERILATOR
assign rst_ni = dpi_rst_ni;
`else
assign rst_ni = in_rst_ni & dpi_rst_ni;
`endif

//----------------------------------------
// Clock generation (not used in Verilator
Expand Down
Loading