Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clint interrupts virtualization #199

Closed
wants to merge 4 commits into from
Closed

Clint interrupts virtualization #199

wants to merge 4 commits into from

Conversation

Wesdcv
Copy link
Collaborator

@Wesdcv Wesdcv commented Sep 19, 2024

Draft PR: Hopefully partially solves #186 (MEI is yet to implement).

@Wesdcv Wesdcv force-pushed the clint-interrupts-virtualization branch 2 times, most recently from efee9d6 to fa21968 Compare September 20, 2024 11:42
@Wesdcv Wesdcv force-pushed the clint-interrupts-virtualization branch 2 times, most recently from 62d5a78 to b1934dc Compare September 30, 2024 05:21
@Wesdcv
This commit adds m-mode interrupts tests
20c08f6 
To verify correctness of interrupt virtualization, we should test some corner cases that are not currently addressed by firmware payloads
This tests verify that for virtualized m-mode interrupts from CLINT (timer&software) priority is right (e.g if two interrupts are pending simultaniously, MSI will be delivered before MTI), interrupt will be raised again if not cleared in trap handler, and that MSIs work overall (albeit that should already be covered by zephyr)
@Wesdcv
This is a first draft commit with virtualized interrupts MSI and MTI
ad3a40a 
Works in CI, doesn't work on the board
@Wesdcv
Refactored interrupt virtialization tests
c899b17 
Added timer tests to CI
Tidied up the code a bit
@Wesdcv
Fix timer virtualization bugs
460f09c 
This commit resolves two issues with timer virtualization:
1. Fixed a timer delay that was too large, ensuring test success regardless of log level.
2. Corrected a bug where the MPIE value was being overwritten, which for some reason affected only Spike.
Additionally, now firmware cannot delegate MSI and MTI.
@Wesdcv Wesdcv force-pushed the clint-interrupts-virtualization branch from b1934dc to 460f09c Compare September 30, 2024 05:29
@Wesdcv
Copy link
Collaborator Author

Wesdcv commented Sep 30, 2024 •
edited
Loading

Changes Made

  1. Filter Added in set_csr(): To prohibit the delegation of the MSI and MTI interrupts. However, MEI should be delegated for S-mode to receive interrupts due to specific PLIC logic. This aspect may need further revision to align with the overall strategy.

  2. MEI Value Update: Given the unique logic surrounding MEI, its value is currently updated without virtualization. The current approach implemented allows emulate_jump_trap_handler() to add pending M-mode interrupts, but not to clear them. While this solution isn't flawless, it functions correctly in both QEMU and Spike under the current non-virtualized MEI conditions.

  3. Pending Interrupt Check: Each write to the CLINT now checks if any vMXIs (MSI, MTI) should be cleared, ensuring proper interrupt management.

  4. MPIE Virtualization: I also believe the mstatus.MPIE virtualization used to be implemented without some corner case considerations, but issues only appeared in Spike.

Questions/Considerations

  • For CLINT, harts are expected to write to each other's MSIP. However, we might want to ensure that a hart does not attempt to write to another hart's MTIMECMP.
  • I left two options in the code for how interrupts should be injected; I would appreciate input on which direction would be best.
  • I have left the linux patch in the codebase, mainly because I have no idea what to do with it :)

I am open to any suggestions or notes for polishing the code, as it may still be a bit raw at this stage. Thank you for your review!

P.S. Checks fail at the past commits, i'll deal with it a bit later, otherwise ready for review.

@Wesdcv Wesdcv marked this pull request as ready for review September 30, 2024 05:46
@CharlyCst CharlyCst self-requested a review October 8, 2024 08:58
@CharlyCst
Copy link
Owner

Thanks for the work!
I have a deadline for the 18th (on the Tyche project), so I'm a bit busy right now, but I'll come back to work on that and do the review afterward :)

@CharlyCst CharlyCst mentioned this pull request Oct 28, 2024
Merged
@CharlyCst
Copy link
Owner

I spent some time working on the interrupts, I changed how we handle them in Miralis quite a bit (to add a proper virtualization layer, we don't re-inject the values from the trap-info anymore but we manually emulate the trap when the conditions are satisfied). I ended up using your code during the re-factoring so I will close this PR in favor of #257. I need to test on the board but I think we might finally have a version that works on the board without patching Linux :)

Thanks a lot for all of the work on this, the tests in particular were super helpful and helped me catch a lot of bugs :)
(I marked you as co-author on the commits using you code on the other PR)

@CharlyCst CharlyCst closed this Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CharlyCst CharlyCst Awaiting requested review from CharlyCst

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Wesdcv @CharlyCst