Skip to content

New optional feature: PLIC external interrupt handling with virq #49

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.

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

Closed
wants to merge 29 commits into from
Closed

Conversation

steewbsd
Copy link
Contributor

This pull request adds a new optional feature: virq (vectorized interrupts).
When enabled, any of the 52 PLIC external interrupts are automatically handled by an already provided MachineExternal function, which in turn calls the appropriate handler for the interrupt source.
Every interrupt is redirected to a handler with a name equal to theirs, like shown below:
GPIO4 -> GPIO4().
As every handler is declared as a weak symbol in interrupts.x, the user can override any of these 52 functions with a simple

#[no_mangle]
fn GPIO4() {
/* ... */
} 

which will override the default handler, thus providing a very simple way of customizing behaviors per-interrupt.
An example usage of this feature is provided in the hifive1 repository, currently residing in a personal fork, virq.rs
which will also be pull requested to the upstream repository.
That pull request would be needed in order to use this feature, as the interrupts.x file provided is included from their hifive1-link.x file, as to not disrupt any kind of linker script dependency.
As a side note, the GPIO0 as well as the PLIC peripherals would very much benefit from some abstractions to manage their respective priorities, such as having a clear_priorities function in PLIC, or an interrupt_enable() function in GPIO to enable GPIO interrupts. A further pull request will possibly be made addressing a proposal for such functions.
As an explanation for why this was not implemented in e310x instead, it all boils down on the MachineExternal function depending directly on CorePeripherals, so it was more convenient to implement here.
Lastly, I noticed there is a convenience macro in e310x/interrupts.rs , interrupt! which would achieve a similar function to what this pull request implements, but it currently has no implementation and is not working. I would like your feedback on somewhat merging the two efforts to achieve a common goal.

@romancardenas
Copy link
Contributor

Any news on this PR? @almindor we would like to know what you think about this contribution and how we could improve it if needed

@almindor
Copy link
Collaborator

Hey, sorry I just didn't have the time to look into this yet. It looks ok on first glance, could you fix the conflicts please? I'll let the tests run (we probably need to also make the CI builds to take this feature into account using the --features flag)

@romancardenas
Copy link
Contributor

romancardenas commented Feb 15, 2023

Great! We are working on it. We are currently facing a conceptual issue regarding the linking of the new parts. I'll try to explain the problem the best I can so you could give us some advice.

In e310x, you have a few lines regarding interrupts. Basically, you only define the external interrupt sources. We initially thought that it would be a good idea to implement the vectored IRQ table in that crate. However, e310x does not provide functionalities to work with the PLIC etc., which are particularly useful for implementing the MachineExternal function in this vectored version.

That is why we decided to move all the contribution here. The problem is that we now have a new linker file: interrupts.x. We also need to implement a build.rs file for this crate. It may not be an issue, but it increases the pieces of code to maintain.

Next, we must update the hifive1 BSP so it now includes interrupts.x in hifive1-link.x. This step should be straightforward.

Before resolving all the conflicts and the CI stuff, we wanted to know your point of view on how to structure the code among these three crates. Then, we will put all our efforts to integrate our feature following your style.

@almindor
Copy link
Collaborator

Could you please split this into two PRs? One for just the lint fixes so we get those out of the way without mixing things up and one for the actual interrupt additions.

@romancardenas
Copy link
Contributor

Sure, my bad! We're working on it

@steewbsd
Copy link
Contributor Author

Hello, I have separated the changes into a new Pull Request in #51 . If you deem it appropriate I can close this now.

@steewbsd steewbsd closed this Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants