hal/armvXm: use cpu_context_t as exc_context_t #685
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @etiaro, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refactors the exception context handling for ARMv7m and ARMv8m targets. By aliasing exc_context_t to cpu_context_t, it standardizes how exception contexts are managed, ensuring they reside at the end of the kernel stack even during fault handling. This change is motivated by the goal of simplifying unified coredump generation across different targets.
Highlights
- Unified Exception Context: The
exc_context_ttype definition has been changed to directly usecpu_context_tacross both ARMv7m and ARMv8m architectures, streamlining context management. - Context Member Renaming: References to
excretandmspctxwithin the exception context structure have been updated toirq_retandhwctxrespectively, aligning with the newcpu_context_tstructure. - Assembly Context Handling Adjustments: The assembly code for exception dispatch (
_exceptions_dispatch) in various ARMv7m and ARMv8m target initialisation files (_init.S) has been modified to correctly save and restore the updatedcpu_context_tstructure, including adjustments for FPU context and stack alignment. - FPU Context Size Update: The
SIZE_FPUCTXmacro inhal/armv7m/exceptions.chas been increased to accommodate the full FPU context size.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request refactors exception handling on armv7m and armv8m to use cpu_context_t directly as exc_context_t, which is a good change for consistency and simplifying coredump generation. The changes in the header files and C code are correct and consistent with this goal. However, I've identified a recurring issue in several assembly files where a register (r2) is saved to the exception context without being initialized first. This leads to uninitialized data in the context structure, which can cause unpredictable behavior. I've added specific comments with suggestions to fix this in each affected file.
etiaro
force-pushed
the
etiaro/armvXm-excctx
branch
from
1a68372 to
ff4062b
Compare
hal/armv7m/imxrt/_init.S
Outdated
| /* if we came from userspace, make space for hw-saved regs */ | ||
| tst lr, #(1 << 2) | ||
| it ne | ||
| subne sp, sp, #(26 * 4) /* space for hw-saved ctx */ |
There was a problem hiding this comment.
I think the comment is somewhat misleading - we're aligning the cpu_context_t struct, not making space for the ctx - it's never put in there. I would like to also suggest writing the size out more explicitly - #((8 + 18) * 4)
There was a problem hiding this comment.
Also, I think we can skip this aliment - part of the struct will point to the trash, but it's never accessed. On the other hand we'll save some stack space (that is used extensively in the exception handler)
There was a problem hiding this comment.
Sure, the comment could be better indeed.
The problem shows up when we start accesing this context as kstack + kstacksz - sizeof(cpu_context_t):
Even if we don't want to deliver signals from exceptions on this target, it's still useful for coredump to have single kind of context
There was a problem hiding this comment.
I'm also wondering if we shouldn't move s0-s15, fpscr, pad1 from cpu_context_t to cpu_hwContext_t for more clarity, as it seems to me like these are also hardware-saved.
Or am I missing something here?
etiaro
force-pushed
the
etiaro/armvXm-excctx
branch
from
ff4062b to
2df7005
Compare
etiaro
force-pushed
the
etiaro/armvXm-excctx
branch
from
2df7005 to
790747f
Compare
JIRA: RTOS-1062
JIRA: RTOS-1062
etiaro
force-pushed
the
etiaro/armvXm-excctx
branch
from
790747f to
c076305
Compare
3 participants
Description
This way armv7m & armv8m targets follow convention of having cpu_context_t as substructure of exc_context_t and residing at the end of kernel stack when being in kernel space even during fault handling.
Also adjusted SIZE_FPUCTX to include fpscr and padding as described in ARM docs.
Currently tested only on imxrt117x
Motivation and Context
This assumption simplifies producing coredump in unified way among all targets.
Types of changes
How Has This Been Tested?
Checklist:
Special treatment