Add satp mode guard #807

KotorinMinami · 2025-03-21T04:42:58Z

From #796 , as the spec say

Implementations are not required to support all MODE settings, and if satp is written with an unsupported MODE, the entire write has no effect; no fields in satp are modified.

so adding extensionenabled and applying guard in legalize_satp I think might work.

jordancarlin

Need to think about whether extensionEnabled() makes sense for this. Technically only one of these extensions is ever enabled at a time (whichever is set in satp), but this may be the simplest way to implement this.

model/riscv_extensions.sail

github-actions · 2025-03-21T04:53:32Z

Test Results

400 tests ±0 400 ✅ ±0 1m 55s ⏱️ +2s
1 suites ±0 0 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit 03e322f. ± Comparison against base commit 8ff2a70.

♻️ This comment has been updated with latest results.

jrtc27 · 2025-03-21T05:13:47Z

Need to think about whether extensionEnabled() makes sense for this. Technically only one of these extensions is ever enabled at a time (whichever is set in satp), but this may be the simplest way to implement this.

No? The extension just means it's a supported option for satp.MODE, not that it's the currently active one.

jordancarlin · 2025-03-21T05:19:30Z

Need to think about whether extensionEnabled() makes sense for this. Technically only one of these extensions is ever enabled at a time (whichever is set in satp), but this may be the simplest way to implement this.

No? The extension just means it's a supported option for satp.MODE, not that it's the currently active one.

The way it's handled in the model right now, we use extensionEnabled() to mean an extension is active and sys_enable_XXX() to indicate it's a supported extension. For example, extensionEnabled(Ext_F) is only true when mstatus.FS is not set to off. It seems like the analogue would be for the extensionEnabled() clauses to be dependent on satp.MODE, but I'm not sure that scheme totally works for the VM mode extensions for the same reason you just mentioned.

jrtc27 · 2025-03-21T05:38:44Z

Need to think about whether extensionEnabled() makes sense for this. Technically only one of these extensions is ever enabled at a time (whichever is set in satp), but this may be the simplest way to implement this.

No? The extension just means it's a supported option for satp.MODE, not that it's the currently active one.

The way it's handled in the model right now, we use extensionEnabled() to mean an extension is active and sys_enable_XXX() to indicate it's a supported extension. For example, extensionEnabled(Ext_F) is only true when mstatus.FS is not set to off. It seems like the analogue would be for the extensionEnabled() clauses to be dependent on satp.MODE, but I'm not sure that scheme totally works for the VM mode extensions for the same reason you just mentioned.

The extension is always active, there's no way to turn it off. You just may not currently be using that specific mode in satp. It makes total sense for extensionEnabled to be true for each of the SvXX that the model is configured to support, because that's what the architecture says happens.

KotorinMinami · 2025-03-25T14:31:37Z

If I understand correctly, my current implementation should be fine, right? Going forward, we just need to add the sys_enable_svxx() function and use them in isextensionEnable().

model/riscv_sys_regs.sail

jordancarlin

LGTM now

arichardson

LGTM, but I think minor simplification is possible.

model/riscv_sys_regs.sail

jordancarlin

From section 3.1.6.4 of the privileged spec:

SUM is read-only 0 if S-mode is not supported or if satp.MODE is read-only 0.

Right now mstatus.SUM is guarded on S, but now that we are adding a way to disable writes to satp it needs to be guarded on that as well. Same for sstatus.SUM.

model/riscv_sys_regs.sail

jordancarlin

LGTM

model/riscv_sys_regs.sail

jordancarlin

Just needs to be updated for the new config system. Then should be good to go.

model/riscv_extensions.sail

jordancarlin · 2025-04-09T15:15:01Z

model/riscv_sys_regs.sail

@@ -160,6 +160,19 @@ function sys_enable_supervisor() -> bool = true
 function clause extensionEnabled(Ext_U) = misa[U] == 0b1
 function clause extensionEnabled(Ext_S) = misa[S] == 0b1

+// TODO: Make configurable
+function clause extensionEnabled(Ext_Svbare) = extensionEnabled(Ext_S)
+function clause extensionEnabled(Ext_Sv32) = extensionEnabled(Ext_S) & (xlen == 32)


Update

Suggested change

function clause extensionEnabled(Ext_Sv32) = extensionEnabled(Ext_S) & (xlen == 32)

function clause currentlyEnabled(Ext_Sv32) = currentlyEnabled(Ext_S) & (xlen == 32) & hartSupports(Ext_Sv32)

jordancarlin · 2025-04-14T15:29:04Z

config/default.json

@@ -183,6 +183,18 @@
            "supported" : true
        },
        "Svinval" : {
+            "supported" : false


This shouldn’t be disabling Svinval.

model/riscv_sys_regs.sail

arichardson

LGTM with my comment and https://github.com/riscv/sail-riscv/pull/807/files#r2042691915 addressed

arichardson · 2025-04-14T19:38:00Z

model/riscv_extensions.sail

+function clause hartSupports(Ext_Sv39) = config extensions.Sv39.supported
+enum clause extension = Ext_Sv48
+function clause hartSupports(Ext_Sv48) = config extensions.Sv48.supported
+enum clause extension = Ext_Sv57
+function clause hartSupports(Ext_Sv57) = config extensions.Sv57.supported


Should these have an xlen check (or a configuration sanity check that gives a fatal error if you try to enable them for RV32?)? That would allow us to remove the checks lower down.

They should definitely check xlen, similar to sv32.

We probably need a whole new file/function that checks for illegal configurations since there a number of other extensions that have the same issue. I’d hold off on that part in this PR.

model/riscv_sys_regs.sail

jordancarlin

Looks great!

Timmmm

LGTM. The only other thing I wonder is if we should enforce that Sv48 depends on Sv39 and Sv57 depends on Sv48? E.g. you can't just enable Sv57.

jordancarlin · 2025-04-15T14:03:04Z

LGTM. The only other thing I wonder is if we should enforce that Sv48 depends on Sv39 and Sv57 depends on Sv48? E.g. you can't just enable Sv57.

True, and maybe that is the easiest way for now.

Timmmm · 2025-04-28T14:31:19Z

Discussed in the meeting. We think we're going to eventually have a config validation function to check you don't enable Sv57 without Sv48 etc. so this is fine.

jordancarlin requested changes Mar 21, 2025

View reviewed changes

model/riscv_extensions.sail Show resolved Hide resolved

model/riscv_extensions.sail Outdated Show resolved Hide resolved

KotorinMinami force-pushed the satp_config branch from 69451f7 to 27d771d Compare March 25, 2025 14:27

jordancarlin reviewed Mar 25, 2025

View reviewed changes

model/riscv_sys_regs.sail Outdated Show resolved Hide resolved

jordancarlin requested changes Mar 25, 2025

View reviewed changes

model/riscv_sys_regs.sail Outdated Show resolved Hide resolved

KotorinMinami force-pushed the satp_config branch from 27d771d to 81708dc Compare March 26, 2025 07:40

jrtc27 reviewed Mar 26, 2025

View reviewed changes

model/riscv_sys_regs.sail Outdated Show resolved Hide resolved

KotorinMinami force-pushed the satp_config branch from 81708dc to bc8ff5f Compare March 27, 2025 00:29

jordancarlin approved these changes Mar 27, 2025

View reviewed changes

arichardson approved these changes Mar 27, 2025

View reviewed changes

model/riscv_sys_regs.sail Outdated Show resolved Hide resolved

model/riscv_sys_regs.sail Outdated Show resolved Hide resolved

KotorinMinami force-pushed the satp_config branch from bc8ff5f to 081f8d4 Compare March 28, 2025 16:59

jordancarlin approved these changes Mar 28, 2025

View reviewed changes

arichardson approved these changes Mar 28, 2025

View reviewed changes

model/riscv_sys_regs.sail Outdated Show resolved Hide resolved

jordancarlin requested changes Mar 29, 2025

View reviewed changes

jordancarlin linked an issue Mar 30, 2025 that may be closed by this pull request

Configuration of supported SATP Virtual Memory modes #796

Open

KotorinMinami force-pushed the satp_config branch from 081f8d4 to 8bca1db Compare April 2, 2025 08:25

jordancarlin reviewed Apr 2, 2025

View reviewed changes

model/riscv_sys_regs.sail Outdated Show resolved Hide resolved

model/riscv_sys_regs.sail Outdated Show resolved Hide resolved

KotorinMinami force-pushed the satp_config branch from 8bca1db to 4def74c Compare April 3, 2025 04:33

jordancarlin approved these changes Apr 3, 2025

View reviewed changes

Timmmm requested changes Apr 3, 2025

View reviewed changes

model/riscv_sys_regs.sail Outdated Show resolved Hide resolved

model/riscv_sys_regs.sail Outdated Show resolved Hide resolved

KotorinMinami force-pushed the satp_config branch from 4def74c to 1326854 Compare April 5, 2025 19:52

jordancarlin requested a review from Timmmm April 6, 2025 23:36

jordancarlin requested changes Apr 9, 2025

View reviewed changes

KotorinMinami force-pushed the satp_config branch from 1326854 to a1d52ef Compare April 10, 2025 07:19

jordancarlin added the configuration Additional configuration settings needed for the model label Apr 11, 2025

KotorinMinami force-pushed the satp_config branch from a1d52ef to f917603 Compare April 14, 2025 15:13

jordancarlin reviewed Apr 14, 2025

View reviewed changes

KotorinMinami force-pushed the satp_config branch from f917603 to 72e535f Compare April 14, 2025 18:19

arichardson approved these changes Apr 14, 2025

View reviewed changes

KotorinMinami force-pushed the satp_config branch from 72e535f to c37f0db Compare April 14, 2025 20:02

jordancarlin reviewed Apr 14, 2025

View reviewed changes

model/riscv_sys_regs.sail Outdated Show resolved Hide resolved

arichardson approved these changes Apr 14, 2025

View reviewed changes

KotorinMinami force-pushed the satp_config branch from c37f0db to 6dfd1ac Compare April 14, 2025 21:04

jordancarlin approved these changes Apr 14, 2025

View reviewed changes

jordancarlin added the will be merged Scheduled to be merged in a few days if nobody objects label Apr 15, 2025

Timmmm reviewed Apr 15, 2025

View reviewed changes

Timmmm added tgmm-agenda Tagged for the next Golden Model meeting agenda. and removed will be merged Scheduled to be merged in a few days if nobody objects labels Apr 16, 2025

Timmmm approved these changes Apr 28, 2025

View reviewed changes

Timmmm added the will be merged Scheduled to be merged in a few days if nobody objects label Apr 28, 2025

KotorinMinami force-pushed the satp_config branch from 6dfd1ac to abefe2a Compare April 28, 2025 16:54

add satp mode guard

03e322f

KotorinMinami force-pushed the satp_config branch from abefe2a to 03e322f Compare April 28, 2025 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add satp mode guard #807

Add satp mode guard #807

KotorinMinami commented Mar 21, 2025

jordancarlin left a comment

github-actions bot commented Mar 21, 2025 •

edited

Loading

jrtc27 commented Mar 21, 2025

jordancarlin commented Mar 21, 2025

jrtc27 commented Mar 21, 2025

KotorinMinami commented Mar 25, 2025

jordancarlin left a comment

arichardson left a comment

jordancarlin left a comment

jordancarlin left a comment

jordancarlin left a comment

jordancarlin Apr 9, 2025

jordancarlin Apr 14, 2025

arichardson left a comment

arichardson Apr 14, 2025

jordancarlin Apr 14, 2025

jordancarlin left a comment

Timmmm left a comment

jordancarlin commented Apr 15, 2025

Timmmm commented Apr 28, 2025

	function clause extensionEnabled(Ext_Sv32) = extensionEnabled(Ext_S) & (xlen == 32)
	function clause currentlyEnabled(Ext_Sv32) = currentlyEnabled(Ext_S) & (xlen == 32) & hartSupports(Ext_Sv32)

Add satp mode guard #807

Are you sure you want to change the base?

Add satp mode guard #807

Conversation

KotorinMinami commented Mar 21, 2025

jordancarlin left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 21, 2025 • edited Loading

Test Results

jrtc27 commented Mar 21, 2025

jordancarlin commented Mar 21, 2025

jrtc27 commented Mar 21, 2025

KotorinMinami commented Mar 25, 2025

jordancarlin left a comment

Choose a reason for hiding this comment

arichardson left a comment

Choose a reason for hiding this comment

jordancarlin left a comment

Choose a reason for hiding this comment

jordancarlin left a comment

Choose a reason for hiding this comment

jordancarlin left a comment

Choose a reason for hiding this comment

jordancarlin Apr 9, 2025

Choose a reason for hiding this comment

jordancarlin Apr 14, 2025

Choose a reason for hiding this comment

arichardson left a comment

Choose a reason for hiding this comment

arichardson Apr 14, 2025

Choose a reason for hiding this comment

jordancarlin Apr 14, 2025

Choose a reason for hiding this comment

jordancarlin left a comment

Choose a reason for hiding this comment

Timmmm left a comment

Choose a reason for hiding this comment

jordancarlin commented Apr 15, 2025

Timmmm commented Apr 28, 2025

github-actions bot commented Mar 21, 2025 •

edited

Loading