sail-riscv icon indicating copy to clipboard operation
sail-riscv copied to clipboard

Fix handling of mstatus.MIE in M-mode only case.

Open rmn30 opened this issue 3 years ago • 6 comments

Previously if neither user or supervisor mode are present dispatchInterrupt ignored mstatus.MIE leading to an infinite interrupt loop.

rmn30 avatar Nov 04 '22 09:11 rmn30

Unit Test Results

712 tests  ±0   712 :heavy_check_mark: ±0   0s :stopwatch: ±0s     6 suites ±0       0 :zzz: ±0      1 files   ±0       0 :x: ±0 

Results for commit 1e983ea8. ± Comparison against base commit 9547a30b.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Nov 04 '22 10:11 github-actions[bot]

Any objections to this?

rmn30 avatar Jan 12 '23 11:01 rmn30

Yes, that is more elegant. I will push an alternative version.

rmn30 avatar Jan 24 '23 11:01 rmn30

That does sound like the correct version to determine that an interrupt cannot be delegated, irrespective of delegation bits and enabling

On Wed, Jan 25, 2023 at 2:32 AM Robert Norton @.***> wrote:

@.**** commented on this pull request.

In model/riscv_sys_control.sail https://github.com/riscv/sail-riscv/pull/190#discussion_r1086463844:

@@ -342,9 +342,9 @@ function dispatchInterrupt(priv : Privilege) -> option((InterruptType, Privilege if (~ (haveUsrMode())) | ((~ (haveSupMode())) & (~ (haveNExt()))) then { assert(priv == Machine, "invalid current privilege");

On the subject of that condition I think it could also be expressed as ~(haveSupMode()) | (haveUsrMode() & ~haveNExt()) which might more clearly express the intent?

The above is incorrect but a more concise version might be ~(haveSupMode() | haveNExt())?

— Reply to this email directly, view it on GitHub https://github.com/riscv/sail-riscv/pull/190#discussion_r1086463844, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHPXVJUAG6EYCA2UQXFCOSTWUD6KXANCNFSM6AAAAAARW772AA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

allenjbaum avatar Jan 25 '23 18:01 allenjbaum

OK, I'll revise this again to remove the assertion. I'll also flip the if so we don't need to invert the condition.

rmn30 avatar Jan 26 '23 10:01 rmn30

Please correct the typo (msatus) in the commit message and PR title.

scottj97 avatar Jan 26 '23 14:01 scottj97

Also this whole check is only a performance optimisation. Maybe it's better just to remove it (it probably doesn't make too much difference because it's all xlenbits so GMP won't get involved).

We ended up doing this so the behaviour should be correct now. Please open an issue if it isn't!

Timmmm avatar Dec 16 '24 14:12 Timmmm