riscv / riscv-fast-interrupt

Proposal for a RISC-V Core-Local Interrupt Controller (CLIC)
https://jira.riscv.org/browse/RVG-63
Creative Commons Attribution 4.0 International
237 stars 49 forks source link

xintthresh checks on entry from lower privilege levels #295

Closed MarkHillHuawei closed 1 year ago

MarkHillHuawei commented 1 year ago

The spec says that "The interrupt-level threshold is only valid when running in associated privilege mode and not in other modes. This is because interrupts for lower privilege modes are always disabled, whereas interrupts for higher privilege modes are always enabled. For example, machine-mode interrupts will not be masked by machine-mode threshold setting when running in user mode".

However xmnxti has no such check on the previous privilege level so there is an inconsistency that vectored interrupts will be taken regardless of xintthresh but direct entry via a xtvec->xmnxti sequence will fail and risks getting into a continuous loop of exit/-re-entry of the ISR without servicing the interrupt.

For the direct/indirect behaviour to be consistent I would suggest a change to the xnxti so that this check is in the pseudo code:

clic.level > mcause.pil && clic.level > mintthresh.th

becomes:

(mcause.mpp<M || (clic.level > mcause.pil && clic.level > mintthresh.th))

I can't think of a good reason why you would want to enter a lower level privilege level with a non-zero intthresh but if the situation does arise it seems safer/more consistent to handle this scenario the same way in both cases.

kasanovic commented 1 year ago

The two scenarios, both of which have a threshold set in M-mode (say M-thresh), and where we are originally running in user-mode 1) M-mode has hardware vectoring: we hardware vector to the handler regardless of M-thresh 2) M-mode has software vectoring: we jump to common handler and use mnxti to handle interrupt, which ignores interrupt if below M-thresh. This is an inconsistency but is well defined. The assumption being that software should have cleared mintthresh before scheduling a lower-privilege task (mintthresh is intended for short atomic sections at the same privilege mode). We don't want to add the additional hardware logic to fix a software bug.

The specification does need more clarification about this issue, and we will add additional text.

MarkHillHuawei commented 1 year ago

My view would be that a handful of gates is worth paying to get the architectural consistency of both cases being treated the same way.

kasanovic commented 1 year ago

We came up with a new proposal to have hardware automatically clear *intthresh when going to a lower mode via SRET and MRET. This is similar to the mstatus.mprv change in priv 1.12. This avoids the issue and also removes a potential source of bugs. Text to be written and added to spec.

MarkHillHuawei commented 1 year ago

Thanks Krste, yes that is a better solution. In fact we have just realised that the additional check against mcause.mpp won't work because when you are handling a sequence of horizontal interrupts any pre-emption to a higher privilege level will cause mpp to be cleared (because executing an mret always does this) potentially resulting in lower interrupt level interrupts being processed when only horizontal ones should be.

dansmathers commented 1 year ago

closed with pull #310

dansmathers commented 1 year ago

adding markhillhuawei comment here instead of on the pull:

I suggest that the equivalent change is made in the debug spec for when dprv lowers the privilege level. Note, the change to mprv has already been mirrored: https://github.com/riscv/riscv-debug-spec/pull/503.

dansmathers commented 1 year ago

closed with pull #313