hal/arm: pmap_switch full MPU reconfiguration #718
Conversation
Summary of ChangesHello @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 significantly refactors the Memory Protection Unit (MPU) management across various ARM architectures to enable more flexible and robust memory mapping for processes. By implementing a full MPU reconfiguration during every context switch, the system can now support more complex and spatially separated memory configurations, which is a foundational step towards a partitioning mechanism. This change moves away from a region bitmask approach to a comprehensive MPU state loading, improving both flexibility and potentially performance for Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the MPU handling on ARM targets to allow for more flexible memory map configurations. The core idea of moving to a full MPU reconfiguration on context switch is sound. However, I've identified a critical issue in the pmap_switch function for armv7m and armv8m architectures that could lead to a buffer over-read and system instability. Additionally, there's a minor inefficiency in pmap_isAllowed for armv7m and armv7r. My review provides suggestions to fix these issues, prioritizing correctness.
etiaro
force-pushed
the
etiaro/mpu-partitioning
branch
from
2ac3247 to
544225a
Compare
etiaro
requested review from
Darchiv,
agkaminski,
jmaksymowicz and
winderdoot
This commit introduces full MPU regions reconfiguration on context switch, allowing for more flexibile configuration of memory maps on MPU targets. Performed tests show no memory coherence problems and minor improvements in pmap_switch performance. According to ARM documentation, cache maintenance is not required, as long as memory maps are not overlapping, and that assumption is already present in Phoenix-RTOS. Changes include * additional hal_syspage_prog_t structure, initialized in loader, containing program configuration of MPU regions in form of ready-to-copy register values * pmap_t structure contain pointer to above structure instead of regions bitmask * pmap_switch disables MPU and performs full reconfiguration, optimized with LDMIA/STMIA assembly operations * handling of process's kernel-code access is moved to loader * hal/pmap.c functions update to the new pmap_t structure JIRA: RTOS-1149
etiaro
force-pushed
the
etiaro/mpu-partitioning
branch
from
544225a to
17b5b7b
Compare
This commit introduces full MPU regions reconfiguration on context switch, allowing for more flexibile configuration of memory maps on MPU targets. Performed tests show no memory coherence problems and minor improvements in pmap_switch performance. According to ARM documentation, cache maintenance is not required, as long as memory maps are not overlapping, and that assumption is already present in Phoenix-RTOS.
Changes include
JIRA: RTOS-1149
Description
Motivation and Context
This change enables users to define more memory maps, allowing more flexible configuration and better spatial separation of processes, which is first stage of development of partitioning mechanism.
The unchanged restriction is that memory maps are disjoint and have fixed attributes, which allows for a quick context switch without costly cache maintenance operations (hardware tests shown it works slightly faster than before).
Note: Correctness of this change is based on observaiton, that there is no documented influence on memory attributes (caching, shareability, accessibility) by disabled MPU region. Thanks to that, if manual cache maintenance wasn't required before, it shouldn't be required here either.
The only moment of doubt, is during pmap_switch itself, when MPU is disabled and accesses to kernel memory are made to configure it. This is similar to situation in previous implementation, when switching between processes without access to kernel data map. This should not cause problems as long as kernel map has the same attributes as in the system address map (which is also used as MPU "background" map for priviliged accesses).
Types of changes
How Has This Been Tested?
Checklist:
Special treatment