serv icon indicating copy to clipboard operation
serv copied to clipboard

Allow mpie bit to be writeable in mstatus

Open btashton opened this issue 4 years ago • 5 comments

This bit is currently not r/w but this means that it is not possible to change the mstatus mie state from within an exception handler. The mie bit is always set to its previous state when mret is called. This is an issue with an RTOS where ecall may be called from a new task creation which is in a critical section. This mean that interrupts including the system tick will not be triggered.

This functionality was tested with the code in this GitHub gist https://gist.github.com/btashton/4aa7ed42bc81ff4716821636997d9df9

btashton avatar Jun 29 '20 05:06 btashton

When I targeted the icebreaker there did not appear to be any change in resource usage. The logic change is quite minor since the wire for the mpie bit is already used for the mie CSR so I was not too surprised. I left the read unimplemented, I think that would add a fair bit more logic, and I don't know that it is that useful.

btashton avatar Jun 29 '20 06:06 btashton

I am realizing that the OS does in fact expect to be able to read the MPIE bit correctly for exactly the case of returning back to critical section. It would be bad if a blocked task restored into the critical section with the MPIE set and interrupts were then restored. I have not looked at what is required to support the read path for this bit.

btashton avatar Jun 30 '20 06:06 btashton

I have also noticed another issue with MPIE that is unexpected. Here is a trace where the interrupt handler is triggered because of the timer. We see a pulse on the trap, and for mstatus MIE goes low and MPIE goes high as expected. However at instruction 0xa4 csrr s0, mstatus MPIE is brought low. So not only can we not read out MPIE to restore it, but reading it will cause its state to change.
image

btashton avatar Jul 01 '20 07:07 btashton

I prefer your fix if we can do without reading, but if I understand your follow-up comments correctly, we do need to read it as well.

olofk avatar Jul 03 '20 21:07 olofk

@olofk I don't know a way around the read issue since we need to store all the register state including mstatus to restore. I'll take a look later this next week, I'm bringing up support for the EOS S3 and I want to get that in "workable" state before switching back to this. I'll also look at Zephyr and see how they are handling the context save/restore. My guess is that there is a lingering issue there as well, but maybe ecall is not used for context switch?

btashton avatar Jul 04 '20 18:07 btashton