Skip to content
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.

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

Added the flag to disable the illegal instruction exception for undefined CSRs #696

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maria-rehman
Copy link

If the CSRs (Control and Status Registers) are not defined by the RISC-V ISA and are also not implemented, their behavior on access can vary, it’s implementation-dependent. Some systems may raise an illegal instruction exception, while others might not.

To handle this variability, I’ve added a new flag, disable_trap_undef_csr, to the Sail tool. When this flag is enabled, the tool will not generate an exception for undefined or unimplemented CSR accesses. However, if the flag is not set, the tool will generate an illegal instruction exception for such accesses.

@jrtc27
Copy link
Collaborator

jrtc27 commented Jan 22, 2025

Uh, no? My copy of the privileged spec very clearly reads:

Attempts to access a non-existent CSR raise an illegal instruction exception.

So it sounds like you have an implementation that's added a non-conforming custom extension (or whatever the current wording is for "tramples on encodings reserved for standard extensions") which defines every CSR encoding it doesn't implement as a read-only-zero CSR not even that, the implementation here makes it a NOP, it doesn't even write back 0 for reads?

@Timmmm
Copy link
Collaborator

Timmmm commented Jan 22, 2025

That was changed to this in recent specs (including the latest ratified one):

Instructions that access a non-existent CSR are reserved.

So I think we do need this flag.

@Timmmm
Copy link
Collaborator

Timmmm commented Jan 22, 2025

(or at least, it isn't wrong, and probably implementations will behave like this)

@jrtc27
Copy link
Collaborator

jrtc27 commented Jan 22, 2025

Ok, but treating it as a NOP rather than a read-zero-write-ignore register is probably wrong and likely to cause issues. Also I know BBL and OpenSBI rely on detecting the number of PMP regions by the fact that unimplemented CSRs trap. So I'm not sure this is a good idea to be doing...

@jrtc27
Copy link
Collaborator

jrtc27 commented Jan 22, 2025

(Ditto a few other CSRs; see csr_read_allowed and csr_write_allowed in OpenSBI)

@Alasdair
Copy link
Collaborator

Instructions that access a non-existent CSR are reserved

According to the unpriv spec decoding a reserved instruction is explictly 'UNSPECIFIED behaviour'. Should we really be doing anything other than raising an error that says you are now outside the bounds of what the spec says? (and therefore if you want explicit behaviour, you need to implement a custom extension that defines it).

@Timmmm
Copy link
Collaborator

Timmmm commented Jan 22, 2025

but treating it as a NOP rather than a read-zero-write-ignore register is probably wrong and likely to cause issues.

Ah yes I agree - I haven't actually read the code yet and assumed it was using read-only zero.

Should we really be doing anything other than raising an error that says you are now outside the bounds of what the spec says?

I think we should try and support common configurations. I have thought about having a mode where the model raises an error if it encounters any unspecified/reserved behaviour (technically spec compliant, if you take it to nasal dragons extremes), but I bet real software invokes UB all the time. Could be useful for testing software though.

However... I don't know if this actually is a common configuration. @maria-rehman did you write this because you are working with a real implementation that doesn't trap?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants