sail-riscv
sail-riscv copied to clipboard
riscv
Added the flag to disable the illegal instruction exception for undefined CSRs
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.
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?
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.
(or at least, it isn't wrong, and probably implementations will behave like this)
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...
(Ditto a few other CSRs; see csr_read_allowed and csr_write_allowed in OpenSBI)
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).
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?
I'll close this for now. @maria-rehman if you have an implementation that actually does treat all unimplemented CSRs as read-only zero then feel free to make a new PR.
It should be a lot easier with the new config system. I would do it by adding an optional catch-all CSR clause, something like this at the start of riscv_csr_end.sail:
// Catch-all CSR; some implementations treat all unimplemented CSRs as read-only zero.
function clause is_CSR_accessible(_) if config csr.catchall_csr_read_only_zero = true
function clause read_CSR(_) if config csr.catchall_csr_read_only_zero = zeros()
function clause read_CSR(_, _) if config csr.catchall_csr_read_only_zero = Ok(zeros())