-
Notifications
You must be signed in to change notification settings - Fork 367
Adding direct vector table hooking support for RISC-V's #621
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
Conversation
|
@onsdagens So sorry for the delay in reviewing this PR, I was on vacation and somehow missed this! Will review today. |
MabezDev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Just a comment about the cfgs/features, lets try and keep it to one feature and leverage the already defined symbols. Could you also rebase on main?
|
I followed your suggestion and moved the helpers out to esp-hal-common, letting us reuse the plic flag. I've also added untested support for the H2 along with an example. Thanks for having a look! |
MabezDev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for those changes. I've tested on my esp32c6/h2 and it isn't working, the interrupt isn't firing for some reason, any ideas on why that might be the case? I'll take a look regardless, but hints would be appreciated.
I'm not sure right off the bat, i'll let it marinate overnight. |
I've figured it out, it was missing a call to |
Great, thank you for looking into it. I've fixed it now. |
|
I added a preemption flag to the RT as you suggested. This made me think that maybe the preemption functionality (or at least the calls to the helpers) should be moved out of esp-hal-common and into esp-riscv-rt entirely. I've done some local tests, and it seems that it would only require some tweaks to the feature gates already present in the RT, but i also think it should probably be a separate PR. What do you think? |
MabezDev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @onsdagens, I appreciate your patience! This looks good to me :).
Oops, missed this. I think its fine in esp-hal-common for now, maybe we can separate it in a future PR if it makes sense. |
|
and of course, CI has died 🙃, I'll check back on this later and merge it then. |
* direct vectoring support added * provide minimal handlers for hooking the vector table directly * changed direct vectoring interrupt enable interface to map to CPU interrupt * direct vectoring interrupt nesting * removed unused dependency * added tentative c2 and c6 support for direct vector table hooking * added direct vectoring examples * added direct vectoring examples * updated changelog * added direct vectoring to CI * Added H2 support and example, moved helpers to esp-hal-common * Added H2 direct vectoring example to CI * Removed remnants of removed feature * C6 and H2 examples fixed * C6 and H2 examples fixed * C6 and H2 examples fixed * Comment fixed * Added preemption flag to RT --------- Co-authored-by: Scott Mabin <[email protected]>
Hi!
This PR adds a feature enabling an alternative interrupt flow, eg. hooking the vector table directly instead of passing through the generic HAL handler. An example benchmark for the esp32-c3 here shows an 85% decrease in interrupt latency when compared to the default approach.
The PR comes in three parts - firstly, i've added a small handler for each of the vector table entries, which move the program flow after the context stacking routine to a weakly linked and overridable symbol instead of the generic HAL handler.
Secondly, i've added a priority threshold set/restore routine to the trap handler to enable proper interrupt nesting.
Thirdly, i've added an alternative
enable()toesp-hal-common/interrupt/riscv, which additionaly requires a CPU interrupt to be provided when enabling an interrupt, and maps the peripheral interrupt accordingly. This is needed since the vector table hooks CPU interrupts, so without explicit control over the mapping, this feature is sort of pointless.All of this is feature gated, the program flow should remain the same if the
direct-vectoringfeature is not enabled.Currently, if a CPU interrupt is in use, the corresponding weak symbol mentioned in part 1 is meant to be overriden, and points to a
loop{}by default (eg. the default HAL handler is only used in case of an exception). However, i think the default behavior could actually be changed to loading the HAL handler address. In this case, if only CPU interrupts 16+ are using the new feature, one could potentially have their cake and eat it too, but i think i'd like to save that for later, since this PR has grown quite large as it is.The changes are tested on the ESP32-C3. I've tried implementing them for the C2 and C6 as well, and the examples build, but since i don't own them, i would appreciate some help with testing.
Thank you!
Thank you for your contribution.
Please make sure that your submission includes the following:
Must
errorsorwarnings.cargo fmtwas run.CHANGELOG.mdin the proper section.Nice to have