8000 cortex-m: mpu: do not give all memory to process in certain case by bradjc · Pull Request #3200 · tock/tock · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

cortex-m: mpu: do not give all memory to process in certain case #3200

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 23, 2022
Merged

cortex-m: mpu: do not give all memory to process in certain case #3200

merged 1 commit into from
Sep 23, 2022

Conversation

bradjc
Copy link
Contributor
@bradjc bradjc commented Sep 7, 2022

The MPU should not make decisions about how much memory to give to a process, the kernel should decide.

It is not clear to me why this check if the kernel does not need any process memory then give the process all of the memory was included.

Testing Strategy

Quickly using hail.

TODO or Help Wanted

Does anyone know if this covers a specific use case? The caller should just request the full amount of RAM rather than relying on the MPU to make this decision.

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

@bradjc
cortex-m: mpu: do not give all memory to process
efb03ea 
The MPU should not make decisions about how much memory to give to a
process, the kernel should decide.

It is not clear to me why that check was included.
@bradjc
Copy link
Contributor Author
bradjc commented Sep 13, 2022

Ping? I don't think we want the MPU to decide how much memory apps should get. This also conflicts with the brk memop call.

@hudson-ayers
Copy link
Contributor

Yeah, this seems weird. It took me awhile to review this PR because it was not easy for me to understand the logic before/after the change. If I understand correctly, the current logic is:

  1. process::create() decides the amount of memory a process requires for userspace, and for its grant region
  2. MPU region size is determined as the closest power-of-2 size larger than the total requested memory for the process; or 256 if less than 256 bytes requested. Subregion size is just this / 8.
  3. Determine how many subregions to enable. If the grant region size is > 0, determine the number of enabled subregions for this process' MPU region via roundup(initial_app_memory_size / (region_size / 8)). Effectively, enable all the subregions containing any of the memory requested by the process. (The grant memory is at the end of the region, and thus that MPU subregion is never enabled by this process, and if it would be we just double the total memory requirement for that app to fix it.)
  4. Anytime an app requests more memory (via brk) we give it more memory by just enabling some more subregions, up until the point that doing so would touch the subregion that covers the grant memory, at which point we return an Error.
  5. If in step 3), the grant_region_size is == 0, we just enable all the subregions and let the app touch any of its memory it wants regardless of if it has called break, because we don't have to protect the grant region anyway since this app does not have one. This is a little weird, because it means that for apps with a grant region, you can only write a little bit past your app break before getting an MPU fault (specifically, you can write past your app break up until the next subregion boundary, wherever that is), whereas for apps with no grant region, you can write past your app break all the way up to the end of the entire MPU region without getting an MPU fault.

This PR just removes the edge case in step 5, such that the MPU is always configured to only allow apps to write to subregions they have specifically requested access to based on initial memory requirements in the tbf header + calls to brk().

That makes sense to me, I see no real upside from letting apps with no grant region write further past their break before generating a fault.

@bradjc
Copy link
Contributor Author
bradjc commented Sep 14, 2022

I believe you have captured the logic correctly.

That makes sense to me, I see no real upside from letting apps with no grant region

It might be worth noting that I don't think there has ever been or ever will be an app with no grant region, so I'm not sure why the check was added in the first place.

@ppannuto
Copy link
Member

bors r+

Hudson, thank you for the extremely helpful summary. I wonder if when we first started with the static array of process structs, if a board image didn't include any capsules that used grants (so no empty grant pointers) if it was possible to have an empty grant space? Not relevant any more however.

@bors
Copy link
Contributor
bors bot commented Sep 23, 2022

Build succeeded:

@bors bors bot merged commit a1f8ddc into master Sep 23, 2022
@bors bors bot deleted the cortexm-mpu-no-extra-process-ram branch September 23, 2022 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hudson-ayers hudson-ayers hudson-ayers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bradjc @hudson-ayers @ppannuto
0