User Mode and remaining mstatus fields #729
Conversation
coreblocks/func_blocks/csr/csr.py
Outdated
| # Temporary, until privileged spec is implemented | ||
| priv_level = Signal(PrivilegeLevel, reset=PrivilegeLevel.MACHINE) | ||
| priv_level = Signal(PrivilegeLevel) | ||
| with Transaction().body(m): |
There was a problem hiding this comment.
This is an always running transaction, right? We'll need a way to assert that easily in Transactron. For now, maybe state this in a comment?
There was a problem hiding this comment.
Call moved to other internal transaction.
Side note: It could be relatively easily asserted with naming transaction and asserting .grant. But is the assertion really needed for single-use transaction only to read nonexclusive and always-ready by definition CSR method?
There was a problem hiding this comment.
Call moved to other internal transaction.
But this will make critical path longer :(
There was a problem hiding this comment.
But is the assertion really needed for single-use transaction only to read nonexclusive and always-ready by definition CSR method?
It is not, but in time code gets modified. It could happen that someone added another, potentially blocking, method call to the transaction, which could then create some kind of a heisenbug. Assertions help protect against future mistakes.
Because it's not the first time something like this happens, maybe some short way for asserting this is needed - e.g. using a keyword argument for Transaction().body.
There was a problem hiding this comment.
But this will make critical path longer :(
Why?
There was a problem hiding this comment.
Because now it is under Switch.
There was a problem hiding this comment.
But the output of the read method does not depend combinationally on the run signal.
coreblocks/priv/csr/csr_instances.py
Outdated
| self.priv_mode = CSRRegister( | ||
| None, | ||
| gen_params, | ||
| width=2, |
There was a problem hiding this comment.
Not something for this PR, but what about accepting arbitrary shapes in CSR definitions? This would allow, e.g., using strongly typed Enum instead of weakly typed IntEnum more.
For now, I would write:
| width=2, | |
| width=Shape.cast(PrivilegeLevel), |
There was a problem hiding this comment.
I'm not sure if that would be too beneficial, as CSR can be written with arbitrary values from functional unit and typing could be wrong in some cases.
|
|
||
|
|
||
| @unique | ||
| class MstatusFieldOffsets(IntEnum): |
There was a problem hiding this comment.
Why there is no SD field?
There was a problem hiding this comment.
because SD field has variable position (last bit of mstatus different on 32/64), but now I added it with value -1, so position is calculated from MstatusFieldOffsets.SD % mstatus.width
coreblocks/priv/csr/csr_instances.py
Outdated
| with m.Case(PrivilegeLevel.USER): | ||
| m.d.comb += legal.eq(gen_params.user_mode) | ||
| with m.Default(): | ||
| pass |
There was a problem hiding this comment.
Shouldn't be 0? So that Supervisor mode will be illegal?
There was a problem hiding this comment.
0 is a default value, updated to set it explicitely
No description provided.