-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
arch: arm: cortex_m: maintainability improvement by reducing assembly surface #65071
arch: arm: cortex_m: maintainability improvement by reducing assembly surface #65071
Conversation
9ca25ac
to
56315ad
Compare
56315ad
to
3a84306
Compare
3a84306
to
c48b0e3
Compare
Merging since the maintainer is the author. |
@ithinuel this PR seems to break some ARM targets like the nRF5340. I haven't tested other platforms just yet, but at least the Bluetooth IPC driver seems to not work after this. |
It causes also a rolling reboot in tests of MCUboot. |
when added
instead of just returning main, it works (no rolling reboot) |
@gchwier can you please open an issue as well? |
Interestingly there are no issues for non-Bluetooth applications, so may be related to the HCI driver or something else. |
There are issues for multiple tests (not only Bluetooth), eg.
|
With that fix there is no rolling-boot, but there is also no printed messages on the console, and shell is not available |
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.
I am a bit late to the party here ... but is converting the core arch implementations written in ASM to C really something we want/should be doing?
The _isr_wrapper
function in isr_wrapper.S
, for instance, is called for every interrupt and just a few instructions added to it can lead to a significant performance degradation. Same goes for z_arm_pendsv
, which is called for every context switch.
Places like these are where we want complete control over the exact instructions used and hence why they were written in ASM.
Also, it is more difficult to control data and instruction barriers by switching to the C implementation (especially using the CMSIS functions), and this can lead to unnecessary and redundant pipeline flushes and hence performance degradation.
Moreover, the existing ASM implementation was fairly clean, well commented, and easy enough to read.
Given that this has already caused many issues for us and will likely cause more in the future, should we look into reverting it and getting back to the good old ASM implementation?
/* ISB is not strictly necessary here (stack pointer is not being | ||
* touched), but it's recommended to avoid executing pre-fetched | ||
* instructions with the previous privilege. | ||
*/ | ||
isb |
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.
Important bits like these get lost when translating to C.
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.
That’s a good point. The ISB is included in __set_CONTROL
, so the barrier is not lost, but it would be good to transfer the comment about pre-fetched instructions.
Thank you for your valuable feedback. I understand all your points and agree with most of them. However, since its introduction, the code has evolved to include a significant amount of conditional features inclusion which compounds with accommodations for the various architectural variants (v6m, v7m, v8m.main, v8m.base etc) makes some of the code difficult to read and keep track of. In the various comparison I made, the generated code was on par or better than the previous solution. The most recent comparison I did was built with In a number of places, the generated code is/was either suboptimal or incorrect. Eg in this instance (which hasn’t been changed as part of this PR), it would fail to build for Cortex-M23 with A notable pain point is around manual in-lining of irq locking/unlocking which makes it hard to keep them all consistent and to introduce changes. Eg, the implementation of SMP on Armv6-m will require to Due to the nature of the change, I was expecting some breakage to have sipped through the checks (all CI passed). I am still ready to investigate and document the cause of any issue that may still arise. Hopefully, this hasn’t inconvenienced too many people and I appreciate the patience of those who have been. So I don’t think reverting would be an appropriate move and I remain convinced this is a good thing for the maintainability. |
CMSIS is actually a problem: #57246, I hope it gets some attention. |
This PR simplifies maintenance and improves portability by relying on C where asm is not strictly required.
This requires: