Change is_CSR_accessible return type to CSRAccessResult enum #1357

trdthg · 2025-10-22T03:23:18Z

No description provided.

arichardson · 2025-10-22T07:46:08Z

model/core/sys_regs.sail

-function clause is_CSR_accessible(0x142, _, _) = currentlyEnabled(Ext_S) // scause
-function clause is_CSR_accessible(0x143, _, _) = currentlyEnabled(Ext_S) // stval
+function clause is_CSR_accessible(0x140, _, _) = if currentlyEnabled(Ext_S) then CSRAccessSuccess else CSRNotExist // sscratch
+function clause is_CSR_accessible(0x142, _, _) = if currentlyEnabled(Ext_S) then CSRAccessSuccess else CSRNotExist // scause


Suggested change

function clause is_CSR_accessible(0x142, _, _) = if currentlyEnabled(Ext_S) then CSRAccessSuccess else CSRNotExist // scause

function clause is_CSR_accessible(0x142, _, _) = csrDefinedBy(Ext_S) // scause

How about using a helper function (not sure what the best name is though) for the simple case where it just depends on an extension being enabled?

existsIfEnabled(Ext_S) or accessibleIfEnabled(Ext_S)?

Either of these sound good to me. I think it might also make sense to have a variant for the *h CSRs to avoid complicated if then else

Timmmm · 2025-10-22T08:03:41Z

model/extensions/Zicsr/zicsr_insts.sail

+  match check_CSR(csr, cur_privilege, is_CSR_Write) {
+    CSRAccessSuccess => (),
+    CSRAccessIllegal => return Illegal_Instruction(),
+    CSRNotExist => return Illegal_Instruction(),


So currently these have exactly the same effect? What will the difference be in future? I think we need a comment for that.

Timmmm · 2025-10-22T08:05:59Z

model/core/sys_regs.sail

-function clause is_CSR_accessible(0x10A, _, _) = currentlyEnabled(Ext_S) // senvcfg
+function clause is_CSR_accessible(0x30A, _, _) = if currentlyEnabled(Ext_U) then CSRAccessSuccess else CSRNotExist // menvcfg
+function clause is_CSR_accessible(0x31A, _, _) = if currentlyEnabled(Ext_U) & (xlen == 32) then CSRAccessSuccess else CSRNotExist // menvcfgh
+function clause is_CSR_accessible(0x10A, _, _) = if currentlyEnabled(Ext_S) then CSRAccessSuccess else CSRNotExist // senvcfg


CSRNotExist at least doesn't feel like the right name here, even if it is the right behaviour. The CSR can still exist, but just be temporarily inaccessible if supervisor mode is disabled for example.

I guess you could say it effectively doesn't exist at that time... maybe we can think of a better name. Probably the names should refer to the actual effect that returning them has, but as it is the same at the moment I don't know what to suggest.

then we use CSRNotEffective and CSREffectiveIfEnabled(Ext_S)? And only use CSRNotExist in fallback.

trdthg · 2025-11-19T09:58:56Z

I just split CSRNotExist into these two

CSRNotDefined: CSR is not defined in spec or not implemented by sail-model yet
- maybe we can also have a CSRNotImplemented, but this cannot be distinguished.
CSRNotEnabled: CSR is disabled by config or misa

I have also categorized things like xlen == 32 to CSRNotEnabled, because according to the manual, you can modify the width of a CSR by changing SXLEN at runtime.

I also considered whether CSRNotEnabled needs to be further divided into

CSRNotSupported(like hartSupported): CSR is implemented but disabled by config which can't be modified at runtime
CSRNotEnabled: CSR is disabled by things like misa that can be modified at runtine

But you have to do a lot of work, for example

function clause currentlyEnabled(Ext_U) = hartSupports(Ext_U) & misa[U] == 0b1 & currentlyEnabled(Ext_Zicsr)
function clause is_CSR_accessible(0x30A, _, _) = currentlyEnabled(Ext_U) // menvcfg

currentlyEnabled(Ext_U)
    |
    v
if hartSupports(Ext_U) & hartSupports(Ext_Zicsr) then 
	// Here you can just check `misa[U] == 0b1`, but you must carefully check
    // the intersection of each `currentlyEnabled` and `hartSupports`.
	if currentlyEnabled(Ext_U) & currentlyEnabled(Ext_Zicsr)
	then CSRNotEnabled
	else CSRAccessSuccess
else CSRNotSupported

Considering we don't need such a detailed division at the moment, I don't really plan to do this.

github-actions · 2025-11-19T10:31:26Z

Test Results

2 115 tests ±0 2 115 ✅ ±0 20m 51s ⏱️ +2s
1 suites ±0 0 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit ab7fc10. ± Comparison against base commit 91178c8.

arichardson · 2025-11-19T17:21:38Z

model/core/csr_begin.sail

+  //    "When MXLEN=32, the SXL and UXL fields do not exist, and SXLEN=32 and UXLEN=32.
+  //     When MXLEN=64, if S-mode is not supported, then SXL is read-only zero. Otherwise,
+  //     it is a WARL field that encodes the current value of SXLEN."
+  CSRNotEnabled,


How is this different from CSRAccessIllegal? I think those two cases could be merged?

I am just thinking out loud here but CSRAccessIllegal could mean in the current context that U-mode is accessing an M-mode CSR or that the CSR does not exist, or writing into a read-only CSR. CSRNotEnabled would apply if FP or vector CSRs are currently disabled, so the CSR exists but access is not permitted at the moment. But my thoughts do not match the comments in the code above.

I guess both of those are just a special case of the "illegal" case. Do we need to distinguish this for anything other than improved logging? Looking at the current diff, handling those specializations adds significant extra logic that could be simpler otherwise:

function clause is_CSR_accessible(0xC02, priv, _) = if currentlyEnabled(Ext_Zicntr) then if currentlyEnabled(Ext_Zicntr) & counter_enabled(2, priv) then CSRAccessSuccess else CSRAccessIllegal else CSRNotEnabled // instret

Could just be

function clause is_CSR_accessible(0xC02, priv, _) = CSREnabledIf(Ext_Zicntr, counter_enabled(2, priv)) // instret

I do think it's valuable to differentiate CSRAccessIllegal and CSRNotDefined, since the trace can then show "access to unknown CSR" and for CSRAccessIllegal "access constraints not met" or similar.

Another reason for the distinction is that there are a few CSRs that are explicitly defined to throw illegal instruction exceptions, but the general case of accessing an undefined CSR is implementation dependent. Right now the model throws an illegal instruction exception, but it is equally legal to not take a trap and just continue execution.

I agree this is confusing and I'm not sure it makes sense to distinguish these cases until we actually do do something different with them (e.g. log them differently or abort on reserved behaviour).

Alternatively perhaps the behavioural and non-behavioural information could be split by turning it into an enum where the Illegal variant has a reason enum, or something like that.

I initially assumed that an external debugger would differentiate between these cases (see riscv/riscv-debug-spec#1140), but that does not seem to be the case (even though we could propagate this kind of information to an external debugger that certain CSR's are currently disabled if openocd plays along). Currently, we do not distinguish at all between not enabled and not implemented. I believe it would be beneficial to differentiate them for potential future use cases, logging as you described (or cases like the one @jordancarlin described).

I'm just a bit worried that because RISC-V itself doesn't make any distinction between them, and if we also don't make use of the distinction, then it's going to be really hard to tell what it actually means.

Adding a reason string would be a great solution. I've debugged issues in. Qemu in the past where a csr was incorrectly marked as inaccessible and if I'd had a trace telling me why I'm getting the illegal instruction fault that would have been so much easier to diagnose.

I'm just a bit worried that because RISC-V itself doesn't make any distinction between them, and if we also don't make use of the distinction, then it's going to be really hard to tell what it actually means.

Yeah I agree it could get messy pretty quickly.

I like your idea of passing the reason as an enum we could use internally/logging, without tying it to the spec itself.

trdthg · 2025-11-20T08:26:46Z

Using a separate enum to represent the reason is indeed a good idea, now the code looks much better

enum CSRAccessIllegalReason = {
  // CSR is not defined in spec or not implemented by sail-model yet
  CSRNotDefined,

  // other
  CSRNotAccessible,
}

union CSRAccessResult = {
  CSRAccessSuccess : unit,
  CSRAccessIllegal : CSRAccessIllegalReason,
}

Now CSRNotAccessible is all cases that can raise Illegal_Instruction except for CSRNotDefined

In addition, I just wrote a more aggressive version that distinguishes various unaccessible reasons in detail. see #1402

arichardson

looks good to me

arichardson · 2025-11-21T23:02:04Z

model/core/csr_begin.sail


+function CSRDefinedIf(exist : bool) -> CSRAccessResult = if exist then CSRAccessSuccess() else CSRAccessIllegal(CSRNotDefined)
+
+function CSRAccessibleIfBool(accessible : bool) -> CSRAccessResult = if accessible then CSRAccessSuccess() else CSRAccessIllegal(CSRNotAccessible)


Suggested change

function CSRAccessibleIfBool(accessible : bool) -> CSRAccessResult = if accessible then CSRAccessSuccess() else CSRAccessIllegal(CSRNotAccessible)

function CSRAccessibleIfBool(accessible : bool) -> CSRAccessResult =

if accessible then CSRAccessSuccess()

else CSRAccessIllegal(CSRNotAccessible)

For consistency with the other functions?

nadime15 · 2025-12-01T17:53:19Z

model/sys/sys_control.sail

+  privLevel_to_bits(p) >=_u csrPriv(csr) & not(csrAccess(csr) == 0b11)

 // Check that the CSR access isn't a write to a read-only CSR.
 function check_CSR_access(csr : csreg, access_type : CSRAccessType) -> bool =
-  not((access_type == CSRWrite | access_type == CSRReadWrite) & (csrAccess(csr) == 0b11))
+  (access_type == CSRWrite | access_type == CSRReadWrite)


Maybe it's just me but I dont think we should check whether we access a read-only CSR (while performing a write, etc.) in check_CSR_priv(). This functions (according to the name) should only check whether we have the sufficient privileges. It might make sense to define a new function to check that?

nadime15 · 2025-12-01T17:59:18Z

model/core/csr_begin.sail

+function CSRDefinedIf(exist : bool) -> CSRAccessResult =
+  if exist then CSRAccessSuccess()
+  else CSRAccessIllegal(CSRNotDefined)
+
+function CSRAccessibleIfBool(accessible : bool) -> CSRAccessResult =
+  if accessible then CSRAccessSuccess()
+  else CSRAccessIllegal(CSRNotAccessible)


I am curious what others think, but I feel it might be a bit unfortunate that we have two functions with the same signature, one overloaded and the other not. I understand the reasoning behind it, but I am not sure it would be immediately obvious to someone new contributing to the project.

They might have to dive deep into the code to notice the difference.

But to be honest, I dont have an alternative for now :D

arichardson reviewed Oct 22, 2025

View reviewed changes

Timmmm reviewed Oct 22, 2025

View reviewed changes

trdthg mentioned this pull request Oct 22, 2025

Update is_CSR_accessible signature to accept CSRAccessType instead of isWrite #1358

Closed

trdthg force-pushed the is_CSR_accessible_return_enum branch from 303d8e2 to ab7fc10 Compare November 19, 2025 09:57

arichardson reviewed Nov 19, 2025

View reviewed changes

trdthg force-pushed the is_CSR_accessible_return_enum branch 2 times, most recently from b88da4b to d362a9e Compare November 20, 2025 08:12

trdthg mentioned this pull request Nov 20, 2025

Enhance CSR access result granularity and report illegal access reasons #1402

Open

arichardson approved these changes Nov 21, 2025

View reviewed changes

Change is_CSR_accessible return type to enum CSRAccessResult

e6777ea

trdthg force-pushed the is_CSR_accessible_return_enum branch from d362a9e to e6777ea Compare December 1, 2025 16:01

nadime15 reviewed Dec 1, 2025

View reviewed changes

	function clause is_CSR_accessible(0x142, _, _) = if currentlyEnabled(Ext_S) then CSRAccessSuccess else CSRNotExist // scause
	function clause is_CSR_accessible(0x142, _, _) = csrDefinedBy(Ext_S) // scause


		function CSRDefinedIf(exist : bool) -> CSRAccessResult = if exist then CSRAccessSuccess() else CSRAccessIllegal(CSRNotDefined)

		function CSRAccessibleIfBool(accessible : bool) -> CSRAccessResult = if accessible then CSRAccessSuccess() else CSRAccessIllegal(CSRNotAccessible)

Change is_CSR_accessible return type to CSRAccessResult enum #1357

Are you sure you want to change the base?

Change is_CSR_accessible return type to CSRAccessResult enum #1357

Uh oh!

Conversation

trdthg commented Oct 22, 2025

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!

trdthg Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trdthg commented Nov 19, 2025

Uh oh!

github-actions bot commented Nov 19, 2025

Test Results

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!

trdthg commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arichardson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nadime15 Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

trdthg Oct 22, 2025 •

edited

Loading

trdthg commented Nov 20, 2025 •

edited

Loading

nadime15 Dec 1, 2025 •

edited

Loading