Support for Smepmp #652

MuhammadHammad001 · 2025-04-23T11:07:38Z

Issue #219

…cfg and mseccfgh

MuhammadHammad001 · 2025-04-24T05:31:10Z

Hi @ThinkOpenly @AFOliveira Can you please review the initial code while I work on adding the IDL?

AFOliveira

I am not the most qualified to review this, so I left my comments more as questions than definitives, feel free to push back if you think I am wrong in any (or all :) )

AFOliveira · 2025-04-24T07:03:54Z

arch/csr/mseccfgh.yaml

+    - name: Sm
+      version: ">=1.12"
+    - name: Smepmp
+      version: ">= 1.0.0"


I am not super sure about this. Obviously, we need machine mode to have Smepmp, but isn't that a relationship in-between the extensions rather than something this CSR should define? Likely @ThinkOpenly has a better view of this than me

Yes, "Smepmp" only, please.

Also, it's better to use the "compatible" operator for the version requirement here:

- name: Smepmp version: ~> 1.0.0

That's equivalent to >= 1.0.0 until some version comes along that isn't backwards-compatible with 1.0.0

AFOliveira · 2025-04-24T07:06:52Z

arch/ext/Smepmp.yaml

+      - name: RISC-V TEE Task Group
+
+description: |
+  Being able to access the memory of a process running at a high privileged execution mode, such as the


Here, we should take much more of the spec rather just the introductory paragraph, right? If things like the threat model are not described here, they won't be anywhere else

Sure, I will add everything from the spec.

There are somethings which may be repetitions of the CSRs, but we definitely need more than this. Also, there is an image we should also add, can you add it? I am not sure what the process for that is

Yeah, I was also thinking about the image and was not sure, whether we should add this or not. But, I will add that in the next commit and move/remove that after the review.

I'm not sure if we already did add some image, it may be something not possible to do yet, but give it a try and let me know. If it needs some extra ruby code, I can help you. I don't think it is fully required to merge this PR since it is a bit out of your scope

This gets in to the topic of what we are trying to describe in the extension YAML files. Two directions:

A concise summary of the extension

The full extension text

We have both intermixed right now in the data. This is a UDB SIG topic

AFOliveira · 2025-04-24T07:08:27Z

arch/ext/Smepmp.yaml

+kind: extension
+name: Smepmp
+type: privileged
+long_name: Personal Physical Memory Protection (PMP) Enhancements for memory access and execution prevention on Machine mode


Personal should be removed and I don't think there is a need to redefine Physical Memory Protection (PMP) , so I would opt only for PMP

Sorry, personal was added by mistake. Sure, I will update this.

AFOliveira · 2025-04-24T07:10:42Z

arch/csr/mseccfg.yaml

 definedBy:
-  name: Sm


why are we removing Sm here? There is no way we can implement this without M-Mode

I actually added defined by both the Sm and Smepmp.

My bad, you are right, you did. The same comment then applies to both definedBy, but let's have Paul take a look at it

The spec looks like the just dumped the "Smepmp" extension proposal into the doc, as 6.2 is literally titled "Proposal". :-/

Anyway, that section says:

Machine Security Configuration (mseccfg) is a new RW Machine mode CSR...

Based on that, Smepmp defined the CSR, and should be the only one listed here. "definedBy" is serving a different purpose than "requires"... just the extension that literally defines the CSR.

ThinkOpenly · 2025-04-24T16:14:51Z

arch/csr/mseccfg.yaml

-length: 64
-description: Machine Security Configuration
+length: MXLEN
+description: Machine Security Configuration register is used for configuring various security mechanisms present on the hart and only accessible in Machine mode.


Remove "and only accessible in Machine Mode", since it's redundant with the "priv_mode", above.

ThinkOpenly · 2025-04-24T16:18:09Z

arch/csr/mseccfg.yaml

 definedBy:
-  name: Sm


The spec looks like the just dumped the "Smepmp" extension proposal into the doc, as 6.2 is literally titled "Proposal". :-/

Anyway, that section says:

Machine Security Configuration (mseccfg) is a new RW Machine mode CSR...

Based on that, Smepmp defined the CSR, and should be the only one listed here. "definedBy" is serving a different purpose than "requires"... just the extension that literally defines the CSR.

ThinkOpenly · 2025-04-24T16:23:36Z

arch/csr/mseccfg.yaml

+  MML:
+    location: 0
+    description: |
+      Machine Mode Lockdown (mseccfg.MML) enforces strong isolation between Machine Mode and lower-privilege modes. This is a _sticky bit_


Where did this sentence come from? A general guideline is to use text from the spec and only text from the spec, preferably unmodified, if possible, to provide content that is accurate and complete.

Reading ahead, the content differs from the spec in some minor areas, or we're looking at different specs. I'm looking at _The RISC-V Instruction Set Manual: Volume II: Privileged Architecture" dated 20240411. I think this is the lastest version... please use the text from the latest version.

I actually was confused whether to add a summary of the orignal spec or completely copy/paste everything. But, now onward, in the next commit, I will replace with the original spec content.
Also, the comment from Derek related to your comment as well?

This gets in to the topic of what we are trying to describe in the extension YAML files. Two directions:

A concise summary of the extension

The full extension text

We have both intermixed right now in the data. This is a UDB SIG topic

My preference is text directly from the spec that fully describes the extension and does not belong elsewhere. So, descriptions of instructions, CSRs/fields is not needed in the extension, because that content goes in other YAML files.

ThinkOpenly · 2025-04-24T16:29:55Z

arch/csr/mseccfg.yaml

+      remains 0 and any further modifications to mseccfg.RLB are ignored until a PMP reset.
+    type: RW
+    definedBy: Smepmp
+    reset_value: UNDEFINED_LEGAL


This PR has a couple of files where GitHub complains about the end of the file

ThinkOpenly · 2025-04-24T16:30:26Z

arch/csr/mseccfgh.yaml

+    - name: Sm
+      version: ">=1.12"
+    - name: Smepmp
+      version: ">= 1.0.0"


Yes, "Smepmp" only, please.

dhower-qc · 2025-04-28T20:07:14Z

arch/ext/Smepmp.yaml

+  Terms:
+
+  - *PMP Entry*: A pair of `pmpcfg[i]` / `pmpaddr[i]` registers.
+
+  - *PMP Rule*: The contents of a pmpcfg register and its associated pmpaddr register(s),
+    that encode a valid protected physical memory region, where `pmpcfg[i].A != OFF`, and
+    if `pmpcfg[i].A == TOR`, `pmpaddr[i-1] < pmpaddr[i]`.
+  - *Ignored*: Any permissions set by a matching PMP rule are ignored, and all accesses to
+    the requested address range are allowed.
+  - *Enforced*: Only access types configured in the PMP rule matching the requested address
+    range are allowed; failures will cause an access-fault exception.
+  - *Denied*: Any permissions set by a matching PMP rule are ignored, and no accesses to the
+    requested address range are allowed.; failures will cause an access-fault exception.
+  - *Locked*: A PMP rule/entry where the `pmpcfg.L` bit is set.
+  - *PMP reset*: A reset process where all PMP settings of the hart, including locked
+    rules/settings, are re-initialized to a set of safe defaults, before releasing the hart (back)
+    to the firmware / OS / application.


I think this is for the glossary, which @kersten1 is working on separately.

okay, I will remove it.

dhower-qc · 2025-04-28T20:19:17Z

arch/csr/mseccfg.yaml

+    location: 2
+    description: |
+      Rule Locking Bypass (mseccfg.RLB). This field can be set to 1 and once it is set back to 0, then it cannot be changed until the PMP Reset.
+
+      When 1, locked PMP rules may be removed/modified and locked PMP enteries may be edited.
+
+      When 0, with `pmpcfg.L=1` in any rule or entry (including disabled enteries), then mseccfg.RLB
+      remains 0 and any further modifications to mseccfg.RLB are ignored until a PMP reset.
+    type: RW
+    definedBy: Smepmp
+    reset_value: UNDEFINED_LEGAL


mseccfg.RLB is WARL and may be hardwired to zero. Thus, the type of this field is configuration-dependent, and the Smepmp extension needs a new parameter to control it.

For example

Smepmp.yaml

parameters: MUTABLE_MSECCFG_RLB: schema: type: boolean description: | When set, mseccfg.RLB is writable. When clear, mseccfg.RLB is read-only-0.

mseccfg.yaml

# instead of `type: RW` type(): return MUTABLE_MSECCFG_RLB ? CsrFieldType::RW : CsrFieldType:RO;

dhower-qc · 2025-04-28T20:28:29Z

arch/csr/mseccfg.yaml

+      even if it is not explicitly covered by a PMP rule.
+
+    type: RW
+    definedBy: Smepmp


mseccfg.MMWP is WARL, so it needs a new configuration parameter in Smepmp. In this case, there are four possibilities: MMWP is read-only-0, read-only-1, reset-to-0 (reset-to-1 is the same as read-only-1 since the bit is sticky). In cases like these, we've been using string parameters like so:

parameters: MSECCFG_MMWP_TYPE: schema: enum: [read-only-0, read-only-1, reset-to-0, reset-to-1] description: | Determines the behavior of the mseccfg.MMWP bit: * "read-only-0": Bit is hardwired to 0 * "read-only-1": Bit is hardwired to 1 * "reset-to-0": Bit comes out of reset cleared, and can be set once before becoming sticky

The type of the field is parameter dependent, e.g.,:

type(): | if (MSECCFG_MMWP_TYPE == "read-only-0") { return CsrFieldType::RO; } else if (...)

And there needs to a sw_write function since the bit is sticky:

sw_write(csr_value): return csr_value.MMWP | CSR[mseccfg].MMWP;

dhower-qc · 2025-04-28T20:29:15Z

arch/csr/mseccfg.yaml

+    - name: Smepmp
+      version: ">= 1.0.0"
+fields:
+  MML:


See comments for MMWP; similar situation for this bit.

Add yaml for mseccfg

e90d730

MuhammadHammad001 requested a review from dhower-qc as a code owner April 23, 2025 11:07

MuhammadHammad001 marked this pull request as draft April 23, 2025 11:33

MuhammadHammad001 added 2 commits April 23, 2025 16:56

Initial yaml for Smepmp extension and update definedby field for msec…

72709d1

…cfg and mseccfgh

Remove wrong param field

4ee10a8

AFOliveira requested changes Apr 24, 2025

View reviewed changes

AFOliveira assigned MuhammadHammad001 Apr 24, 2025

This was linked to issues Apr 24, 2025

Add Smepmp #219

Open

Add sub-types to existing instruction formats #655

Open

AFOliveira removed a link to an issue Apr 24, 2025

Add sub-types to existing instruction formats #655

Open

ThinkOpenly requested changes Apr 24, 2025

View reviewed changes

dhower-qc requested changes Apr 28, 2025

View reviewed changes

Support for Smepmp #652

Are you sure you want to change the base?

Support for Smepmp #652

Uh oh!

Conversation

MuhammadHammad001 commented Apr 23, 2025

Uh oh!

MuhammadHammad001 commented Apr 24, 2025

Uh oh!

AFOliveira left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!