Skip to content
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

Reduce ROM impact of MPU code #9030

Merged
merged 5 commits into from Dec 11, 2018
Merged

Reduce ROM impact of MPU code #9030

merged 5 commits into from Dec 11, 2018

Conversation

kjbracey
Copy link
Contributor

@kjbracey kjbracey commented Dec 10, 2018 •
edited

Description

  • Exclude MPU manager if MPU disabled
  • Save ROM by specifying initial MPU state
  • Use MBED_ASSERT in MPU manager

Saves 400+ bytes from an image using FlashIAP with DEVICE_MPU disabled - the MPU management code is totally eliminated.

Saves around 100 bytes from develop images using FlashIAP with DEVICE_MPU enabled - asserts are smaller than errors. Saves more from release builds by omitting the asserts.

Saves around 90 bytes from images not manipulating the MPU with DEVICE_MPU enabled - don't need to always include the mbed_mpu_enable_ram_xn and mbed_mpu_enable_rom_wn calls.

Fixes #9007.

Pull request type

[ ] Fix
[X] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@kjbracey
Exclude MPU manager code if MPU disabled
e2f6025 
Save around 400 bytes from images using FlashIAP by omitting MPU manager
reference counting if DEVICE_MPU is 0.
@kjbracey
Save ROM by specifying initial MPU state
842587d 
We can omit the need for the "change MPU state" calls from simple images
by specifying the initial state at init.
@kjbracey
Use MBED_ASSERT in MPU manager
1289406 
MPU lock under- or overflow is not a likely random runtime failure - it
will be a mismatched lock/unlock programming error, so should be
detected during development.  Make the checks asserts so they can be
left out from release builds.  MBED_ASSERT is also a bit smaller than
MBED_ERROR in develop builds.
@bulislaw
Copy link
Member

What are the final numbers for all the cases you describe savings for? (On "random" platform)

c1728p9
c1728p9 reviewed Dec 10, 2018
View reviewed changes
hal/mpu_api.h Show resolved Hide resolved
@kjbracey
Copy link
Contributor Author

kjbracey commented Dec 10, 2018 •
edited

Enabling standard ARMv7M DEVICE_MPU now costs 208 bytes in GCC_ARM develop build.

Using the scoped locks, as FlashIAP does, pulls in another 451 bytes in a develop build (including the asserts), making a total of 659 bytes of MPU code, plus 4 bytes of RAM. Plus there will be extra code in the callsites.

All MPU-related code is fully eliminated if DEVICE_MPU is 0.

@bulislaw
Copy link
Member

659 is a lot still, client guys won't be happy.

@adbridge
Copy link
Contributor

659 is a lot still, client guys won't be happy.

@bulislaw So do we want to take this to RC3 or not ?

@c1728p9
Copy link
Contributor

c1728p9 commented Dec 10, 2018

@adbridge you'll want this PR either way. A separate PR will be needed to turn off the MPU by default if this is too much rom for client.

bulislaw
bulislaw approved these changes Dec 10, 2018
View reviewed changes
Copy link
Member

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@c1728p9 can we make sure docs are clear on how to disable it, also could we send an email to @JanneKiiskila and @yogpan01 to warn them it's coming.

@cmonr
Copy link
Contributor

cmonr commented Dec 10, 2018

CI started

@donatieng
Copy link
Contributor

Thanks for pushing this one in, I'm very not eager to disable MPU by default as it would really hurt our security promise - however if footprint is an issue I'm happy to discuss it further

@cmonr
Copy link
Contributor

cmonr commented Dec 10, 2018

@kjbracey-arm If you're around, this is about to fail CI with build errors.
@c1728p9 We might need you to take this over US time.

@mbed-ci
Copy link

mbed-ci commented Dec 10, 2018

Test run: FAILED

Summary: 3 of 7 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-IAR

@c1728p9
Add header to fix compile failures
390ef30 
Add the header platform/mbed_assert.h to fix build errors with
platform/mbed_mpu_mgmt.c.
@cmonr
Copy link
Contributor

cmonr commented Dec 10, 2018

CI started

@mbed-ci
Copy link

mbed-ci commented Dec 10, 2018

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

@c1728p9 c1728p9 mentioned this pull request Dec 10, 2018
Merged
@c1728p9
Copy link
Contributor

c1728p9 commented Dec 10, 2018

@c1728p9 can we make sure docs are clear on how to disable it, also could we send an email to @JanneKiiskila and @yogpan01 to warn them it's coming.

@bulislaw I created #9040 which adds a platform config option to turn off the MPU. Prior to this it had to be turned off either at runtime or by removing device_has from the current target.

I added docs for this config in ARMmbed/mbed-os-5-docs#868.

@cmonr cmonr merged commit 61f1c1c into ARMmbed:master Dec 11, 2018
@cmonr cmonr removed the needs: CI label Dec 11, 2018
@kjbracey kjbracey deleted the mpu_size branch December 11, 2018 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c1728p9 c1728p9 c1728p9 approved these changes

@bulislaw bulislaw bulislaw approved these changes

Assignees
No one assigned
Labels
release-version: 5.11.0-rc3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@kjbracey @bulislaw @adbridge @c1728p9 @cmonr @donatieng @mbed-ci @0xc0170