Added support of Smepmp #601

KotorinMinami · 2024-10-19T23:39:59Z

added command line support for Smepmp. command line switch: --enable-smepmp. Sail function: haveSmepmp

Added support of Smepmp

This pr was written by @HamzaKh01, merged with the command-line switch code provided by Bill, and adjusted by myself

github-actions · 2024-10-19T23:53:14Z

Test Results

392 tests ±0 392 ✅ ±0 1m 20s ⏱️ -1s
1 suites ±0 0 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit cf51e2e. ± Comparison against base commit 2dfc4ff.

♻️ This comment has been updated with latest results.

Timmmm · 2024-10-31T14:58:57Z

Specification is here

Timmmm

Still working on reading all the boolean logic...

Timmmm · 2024-10-31T15:32:22Z

model/riscv_pmp_regs.sail

+}
+
+register mseccfg  : Mseccfg_ent
+register mseccfgh : bits(32)


Normally we don't treat this as a separate register, we just make bitfield Mseccfg : bits(64) and then read/write the appropriate bits of it.

We have an implementation of mseccfg like that internally; I'll make a PR for it.

Timmmm · 2024-10-31T15:38:40Z

model/riscv_sys_regs.sail

+/* Enable Smepmp */
+val sys_enable_smepmp = pure "sys_enable_smepmp" : unit -> bool
+/* TODO: for extension smmpm and zkr to enable */
+function sys_has_mseccfg() -> bool = sys_enable_smepmp()


You can implement mseccfg even if you don't have any of the extensions that use it (and that isn't a weird edge case). So ideally this would be

// Callback to C val sys_have_mseccfg : "sys_have_mseccfg" unit -> bool function have_mseccfg() -> bool = sys_have_mseccfg() | sys_enable_smepmp() | sys_enable_zkr() ...

You mean we need to add an extra command line parameter to act as an extra switch to control the presence of mseccfg to make sure that even if we don't implement extensions that use this register, we can still implement this register, right ?

Yeah exactly. I don't think we need to actually bother hooking it up to a CLI arguments at the moment - we can wait until Alasdair's new config system exists and then it will be a whole lot less tedious. But we can just have a function that returns true as a placeholder so we don't forget.

Timmmm · 2024-10-31T15:39:33Z

model/riscv_pmp_control.sail

@@ -48,14 +48,43 @@ function pmpCheckRWX(ent, acc) = {
  }
 }

-// this needs to be called with the effective current privilege.
+/* this needs to be called with the effective current privilege */


// style comments are nicer IMO. I don't see why we would change them back?

This is the part that was changed by @HamzaKh01, I didn't check this as I was too focused on the code implementation part, we can change it back if needed, but there are many one line comments like /* ... */ in Sail, we might need to declare it better in the codestyle after some discussion?

Yeah there is an experimental auto-formatter in the Sail compiler but I don't know what state it got to and I'd guess it doesn't change this sort of thing.

KotorinMinami · 2024-12-09T05:12:01Z

So does someone review this code and merge it in?

Timmmm

I did a bit more reviewing. I'll do more when it's been updated.

Timmmm · 2025-02-25T14:25:09Z

model/riscv_pmp_control.sail

-               then pmpCheckRWX(ent, acc)
-               else true,
-    _       => pmpCheckRWX(ent, acc)
+  if sys_has_mseccfg() & mseccfg[MML] == 0b1


If a CPU doesn't have mseccfg then I think we can just assume it behaves as if all bits are 0, so I would remove this check here (and in the other places). Instead it should be used for is_CSR_defined.

Timmmm · 2025-02-25T14:55:31Z

model/riscv_pmp_control.sail

+    }
+  }
+  else{
+    match priv {


I think this can be simplified and clarified to

priv == Machine & not(pmpLocked(ent)) | pmpCheckRWX(ent, acc)

I feel like I made this comment somewhere else but I guess it hasn't been merged wherever it was.

Oh yeah look it was me... #738 :-D

I've merged that now.

Timmmm · 2025-02-25T15:00:17Z

model/riscv_pmp_control.sail

+      (_, _, _, _) =>
+        if (priv == Machine) == pmpLockBit(ent)
+        then pmpCheckRWX(ent, acc)
+        else false


I would capture the lock bit from the match:

(L, _, _, _) => ((priv == machine) == (L == 0b1)) & pmpCheckRWX(ent, acc),

Timmmm · 2025-02-25T15:01:39Z

model/riscv_pmp_control.sail

-  if priv == Machine then None() else Some(accessToFault(acc))
+  if priv == Machine
+  then {
+    if sys_has_mseccfg() & mseccfg[MMWP] == 0b1                /* Make Read, Write and execute denied by default, if condition meets for M mode */


Can you move the comments to their own line with //. These are too long, and end-of-line comments are kind of awkward.

Timmmm · 2025-02-25T15:11:15Z

model/riscv_pmp_control.sail

+    }
+    else None()
+  }
+  else Some(accessToFault(acc))


I think this could be a lot simpler:

if priv == Machine & mseccfg[MMWP] != 0b1 & not(mseccfg[MML] == 0b1 & isExecute(acc)) then None() else Some(accesstoFault(acc)

(add isExecute() as a helper)

Timmmm · 2025-02-25T15:12:20Z

model/riscv_pmp_regs.sail

@@ -89,32 +98,48 @@ function pmpReadAddrReg(n : range(0, 63)) -> xlenbits = {
 }

 /* Helpers to handle locked entries */
-function pmpLocked(cfg: Pmpcfg_ent) -> bool =
-   cfg[L] == 0b1
+function pmpLockBit(cfg: Pmpcfg_ent) -> bool = cfg[L] == 0b1


I think I would just delete this and access cfg[L] directly.

Timmmm · 2025-02-25T15:13:07Z

model/riscv_pmp_regs.sail

+  /* To prevent adding a rule with execution privileges if MML is enabled unless RLB is set */
+  else {
+    /* Constructing legal Pmpcfg_ent by making bit 5 and 6 zero */
+    let legal_v : Pmpcfg_ent = Mk_Pmpcfg_ent(v & 0x9f);


Not your fault but we should use [v with 6 .. 5 = zeros()].

jordancarlin · 2025-02-26T07:00:57Z

model/riscv_pmp_regs.sail

+  reg
+}
+
+mapping clause csr_name_map = 0x747  <-> "mseccfg"


All of this mseccfg related code should probably move to sys_regs or another similar file because this CSR is not exclusive to pmp

Oh, that can be awkward, because mseccfg's RLB field depends on pmpNcfg's L field, and pmpNcfg's L field depends on the RLB field, and their highly coupled relationship would be difficult to handle if they were to be written separately

Yeah this is an ongoing problem with these "global" CSRs (also menvcfg, mstatus, etc.). But I agree with @jordancarlin we should move it.

To solve the circular dependency you can forward-declare functions using val. We'll hopefully come up with a better way of dealing with this after the new project system is in place.

Okay, I've pushed the moved version.

…smepmp. Sail function: haveSmepmp Added support of Smepmp Co-authored-by: William McSpaddden <bill@riscv.org> Co-authored-by: Hamza Khan <hamza.khan@10xengineers.ai>

KotorinMinami mentioned this pull request Oct 20, 2024

Added support of Smepmp in Sail RISCV #196

Open

UmerShahidengr mentioned this pull request Oct 29, 2024

ePMP SAIL riscv-admin/dev-partners#12

Open

13 tasks

KotorinMinami force-pushed the smepmp branch from 2a69d43 to ad5a9ed Compare October 30, 2024 23:48

Timmmm reviewed Oct 31, 2024

View reviewed changes

Timmmm added the extension Adds support for a RISC-V extension label Jan 6, 2025

Timmmm requested changes Feb 25, 2025

View reviewed changes

KotorinMinami force-pushed the smepmp branch from 64e2143 to 1dc9cbb Compare February 26, 2025 06:40

KotorinMinami requested a review from Timmmm February 26, 2025 06:55

jordancarlin reviewed Feb 26, 2025

View reviewed changes

added command line support for Smepmp. command line switch: --enable-…

cf51e2e

…smepmp. Sail function: haveSmepmp Added support of Smepmp Co-authored-by: William McSpaddden <bill@riscv.org> Co-authored-by: Hamza Khan <hamza.khan@10xengineers.ai>

KotorinMinami force-pushed the smepmp branch from 1dc9cbb to cf51e2e Compare March 3, 2025 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support of Smepmp #601

Added support of Smepmp #601

KotorinMinami commented Oct 19, 2024

github-actions bot commented Oct 19, 2024 •

edited

Loading

Timmmm commented Oct 31, 2024

Timmmm left a comment

Timmmm Oct 31, 2024

Timmmm Oct 31, 2024

KotorinMinami Oct 31, 2024

Timmmm Oct 31, 2024

Timmmm Oct 31, 2024

KotorinMinami Oct 31, 2024

Timmmm Oct 31, 2024

KotorinMinami commented Dec 9, 2024

Timmmm left a comment

Timmmm Feb 25, 2025

Timmmm Feb 25, 2025

Timmmm Feb 25, 2025

Timmmm Feb 25, 2025

Timmmm Feb 25, 2025

Timmmm Feb 25, 2025

Timmmm Feb 25, 2025

Timmmm Feb 25, 2025

jordancarlin Feb 26, 2025

KotorinMinami Feb 27, 2025

Timmmm Feb 28, 2025

KotorinMinami Mar 3, 2025

Added support of Smepmp #601

Are you sure you want to change the base?

Added support of Smepmp #601

Conversation

KotorinMinami commented Oct 19, 2024

github-actions bot commented Oct 19, 2024 • edited Loading

Test Results

Timmmm commented Oct 31, 2024

Timmmm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KotorinMinami commented Dec 9, 2024

Timmmm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 19, 2024 •

edited

Loading