cpu/cortex-m: turn MPU support into a feature by kaspar030 · Pull Request #13391 · RIOT-OS/RIOT

kaspar030 · 2020-02-17T11:56:46Z

Contribution description

This turns the MPU of Cortex-M into a feature.

Testing procedure

Issues/PRs references

benpicco · 2020-02-17T12:39:24Z

cpu/cortexm_common/Makefile.features

+ifneq ($(CPU_ARCH),cortex-m0)
+  FEATURES_PROVIDED += cortexm_mpu
+endif


According to Wikipedia, the MPU is always optional. It's up to the chip vendor whether to include it or not.

Hmok. Do all our non-cm0 CPUs have an MCU?
I'll add a check to the mpu test application.

I've added a check. Seems like all our non-Cortex-M0 boards have an MPU, or at leastm __MPU_PRESENT set.

Either __MPU_PRESENT is not undefined, or all our Cortex-M boards do have the optional MPU, even the CM0. @aabadie Could you verify the test succeeds e.g., on a stm32f031k6?

@aabadie could you take a look at this? I don't have a M0 board...

I tried on nucleo-f072rb, which is an M0, and the tests/mpu_stack_guard is correctly skipped by the build system.

The problem here is that, according to the __MPU_PRESENT macro in vendor files, stm32f1 and also some stm32f3 (so not all) CPUs doesn't provide an MPU. It's also unclear if stm32l0 provides an MPU or not (I'm wondering if the vendor files were manually altered for this family).

So, as suggested by @benpicco, I think this feature should be provided by default less widely. Maybe just for M4/M7 by default, and enable on a case-by-case basis for other CPU families (M0/M0plus,M3).

I just checked the data sheets, neither samd2x nor saml2x (cm0p) implement an MPU.

aabadie · 2020-02-17T13:43:03Z

tests/mpu_stack_guard/Makefile.ci

@@ -1,3 +1,4 @@
 BOARD_INSUFFICIENT_MEMORY := \
+    nucleo-f031k6 \


This board (and stm32f030f4-demo) is already excluded by the FEATURES_REQUIRED += cortexm_mpu and should already be marked as unsupported by the CI. Why do you add it here ?

Hm, seems like the feature guard doesn't work:

[kaspar@ng riot.tmp/tests/mpu_stack_guard (mpu_feature)]$ make info-boards-supported | grep -o nucleo-f031k6 nucleo-f031k6 [kaspar@ng riot.tmp/tests/mpu_stack_guard (mpu_feature)]$

[kaspar@ng riot.tmp/tests/mpu_stack_guard (mpu_feature)]$ BOARD=nucleo-f031k6 make info-debug-variable-CPU_ARCH cortex-m0 [kaspar@ng riot.tmp/tests/mpu_stack_guard (mpu_feature)]$

Seems like CPU_ARCH is not available in Makefile.features.

It seems that this PR requires some changes from #13236 where CPU_ARCH is moved to Makefile.features. I can cut this part out of #13236 if it also helps here.

I can cut this part out of #13236 if it also helps here.

That would be awesome.

Here it is: #13392 :)

Thanks a bunch.

[kaspar@ng riot.tmp/tests/mpu_stack_guard (mpu_feature)]$ make info-boards-supported | grep -o nucleo-f031k6 [kaspar@ng riot.tmp/tests/mpu_stack_guard (mpu_feature)]$

benpicco · 2020-02-18T15:02:17Z

So this needs a rebase now.

kaspar030 · 2020-02-18T15:47:31Z

So this needs a rebase now.

Done. Removed an unrelated ssp change, and renamed the WIP commit.

kaspar030 · 2020-02-18T16:15:30Z

fixed long commit msg

aabadie

I think this PR cannot be merged in the current state: we should check more precisely which CPUs (family or even specific CPUs) provide an MPU or not. It seems some families doesn't provide an MPU (M0) or even not all CPUs of a given family (M3). For M4/M7, all CPUs seems to provide an MPU.

I also don't think the check on __MPU_PRESENT macro is needed as it seems to be defined to 0 if not already defined (can be verified with git grep __MPU_PRESENT).

aabadie · 2020-02-25T09:21:33Z

cpu/cortexm_common/Makefile.features

+ifneq ($(CPU_ARCH),cortex-m0)
+  FEATURES_PROVIDED += cortexm_mpu
+endif


I tried on nucleo-f072rb, which is an M0, and the tests/mpu_stack_guard is correctly skipped by the build system.

The problem here is that, according to the __MPU_PRESENT macro in vendor files, stm32f1 and also some stm32f3 (so not all) CPUs doesn't provide an MPU. It's also unclear if stm32l0 provides an MPU or not (I'm wondering if the vendor files were manually altered for this family).

So, as suggested by @benpicco, I think this feature should be provided by default less widely. Maybe just for M4/M7 by default, and enable on a case-by-case basis for other CPU families (M0/M0plus,M3).

kaspar030 · 2020-02-25T10:47:39Z

So, as suggested by @benpicco, I think this feature should be provided by default less widely. Maybe just for M4/M7 by default, and enable on a case-by-case basis for other CPU families (M0/M0plus,M3).

Even for M4 this seems to not not always be available (e.g., stm32f302r8 is an M4, but doesn't set __MPU_PRESENT). Seaveing through all CPU's is quite a hassle.

I'm inclined to define this per-board, as that can be done quickly (by, e.g., use the buildtest output of all cortex-m, then add the feature to all board Makefile.features where it succeeded).
We can then move it to more common places later on.

@aabadie what do you think?

aabadie · 2020-02-25T10:51:24Z

what do you think?

I agree that the feature should be provided more finely. Maybe provide the feature in whitelisted board as a first step ? (and no need to wait for buildtest ;))

benpicco · 2020-02-25T10:52:43Z

Seaveing through all CPU's is quite a hassle.

We can just add it to known-good CPU families for now, it's easy to fix omissions later.
E.g. I think it's save to assume no stm32f3 has a MPU.

I'm inclined to define this per-board

But this is not a board feature. We should get rid of MCU features in boards, not add more of those.

kaspar030 · 2020-02-25T11:49:46Z

E.g. I think it's save to assume no stm32f3 has a MPU.

Hmm,

(e.g., stm32f302r8 is an M4, but doesn't set __MPU_PRESENT).

kaspar030 · 2020-02-25T11:56:45Z

But this is not a board feature. We should get rid of MCU features in boards, not add more of those.

Agreed, but the way our build configuration is modeled, there's no way to print the tree of dependencies (board -> cpu -> cpu_fam -> cpu_arch), and there are many layers of includes with non-standard names.

I can do the manual equivalent of make buildtest | grep succeeded | cut -d" " -f3 to get a list of boards that work, and with that I can do for board in $BOARDS; do echo "FEATURES_PROVIDED += cortexm_mpu" >> boards/$BOARD/Makefile.features, then cleanup the result.

Doing it "the right way" will take maybe a day? We'd have to go through all ~180 board Makefiles, figure out which CPU family they're in, try to find commonalities, build, refine.

benpicco · 2020-02-25T12:10:11Z

E.g. I think it's save to assume no stm32f3 has a MPU.

Hmm,

(e.g., stm32f302r8 is an M4, but doesn't set __MPU_PRESENT).

I see no contradiction 😉

Doing it "the right way" will take maybe a day? We'd have to go through all ~180 board Makefiles, figure out which CPU family they're in, try to find commonalities, build, refine.

I'm not advocating for a complete list, just adding FEATURES_PROVIDED += cortexm_mpu to the hand full of families where we know it's present.
Sure we'll miss some, but if people want to use the MPU on these boards they can add the FEATURES_PROVIDED += cortexm_mpu.

kaspar030 · 2020-02-25T12:19:13Z

I'm not advocating for a complete list, just adding FEATURES_PROVIDED += cortexm_mpu to the hand full of families where we know it's present.

Makes sense. Which ones are those?

benpicco · 2020-02-25T12:26:10Z

Which ones are those?

stm32f4 (checked the smallest member of the family), stm32l4 (dito), nrf51, nrf52, samd5x, cc26x2_cc13x2, lm4f120, sam3, saml1x, cc2538

kaspar030 · 2020-03-02T14:08:50Z

stm32f4 (checked the smallest member of the family), stm32l4 (dito), nrf51, nrf52, samd5x, cc26x2_cc13x2, lm4f120, sam3, saml1x, cc2538

Done. Removed nrf51, they don't have an MPU.

benpicco · 2020-03-02T14:15:22Z

Removed nrf51, they don't have an MPU.

The Data Sheet has a chapter about an MPU though.

kaspar030 · 2020-03-02T14:25:09Z

The Data Sheet has a chapter about an MPU though.

Hm, maybe it is non-standard. The CMSIS headers define __MPU_PRESENT to 0, and if forced to 1, compilation fails, e.g.,:

/home/kaspar/src/riot/cpu/cortexm_common/mpu.c: In function 'mpu_disable':
/home/kaspar/src/riot/cpu/cortexm_common/mpu.c:28:5: error: 'MPU' undeclared (first use in this function); did you mean 'MPU_H'?
   28 |     MPU->CTRL &= ~MPU_CTRL_ENABLE_Msk;
      |     ^~~                                                                                                                  
      |     MPU_H

Seems like "MPU" is what Nordic calls flash protection:

The memory protection unit can be configured to protect all flash memory on the device from readback, or to protect blocks of flash from over-write or erase.

aabadie

Could FEATURES_PROVIDED += cortexm_mpu be also added to stm32f1, stm32f2 and stm32l1 ?
(initially iotlab-*, nucleo-l152re and nucleo-207zg were whitelisted in the test).

kaspar030 · 2020-03-03T13:37:27Z

Could FEATURES_PROVIDED += cortexm_mpu be also added to stm32f1, stm32f2 and stm32l1 ?
(initially iotlab-*, nucleo-l152re and nucleo-207zg were whitelisted in the test).

Added the stm32f2 and l1. The others don't define __MPU_PRESENT.

tests/mpu_stack_guard/main.c

aabadie · 2020-03-03T15:12:55Z

Added the stm32f2 and l1

Did you forget to push ?

kaspar030 · 2020-03-03T15:48:57Z

Did you forget to push ?

yup :)

kaspar030 · 2020-03-03T20:58:09Z

There are some missing that where whitelisted before and actually work. Will add them.

kaspar030 · 2020-03-03T21:58:06Z

Now I got them all. Special cases were: some stm32f303 and some stm32l052 have an MPU.
The PR now covers all CPU's in master.
It was not so bad, thanks for pushing @benpicco @aabadie.

aabadie

ACK and go!

kaspar030 requested review from MichelRottleuthner, MrKevinWeiss, aabadie, fjmolinas, leandrolanzieri, miri64 and smlng as code owners February 17, 2020 11:56

kaspar030 mentioned this pull request Feb 17, 2020

Add optional support for executable space protections #13387

Merged

kaspar030 changed the title ~~cpu/corrtex-m: turn MPU support into a feature [WIP]~~ cpu/cortex-m: turn MPU support into a feature [WIP] Feb 17, 2020

benpicco reviewed Feb 17, 2020

View reviewed changes

kaspar030 force-pushed the mpu_feature branch from a38de2d to 8b79ccc Compare February 17, 2020 12:55

aabadie reviewed Feb 17, 2020

View reviewed changes

aabadie mentioned this pull request Feb 17, 2020

cpu/cortexm: move CPU_ARCH/FAM to Makefile.features #13392

Merged

cpu/cortex-m: MPU: turn into feature "cortexm_mpu"

950a206

kaspar030 force-pushed the mpu_feature branch from 8b79ccc to 5a9fff6 Compare February 18, 2020 15:43

kaspar030 changed the title ~~cpu/cortex-m: turn MPU support into a feature [WIP]~~ cpu/cortex-m: turn MPU support into a feature Feb 18, 2020

kaspar030 force-pushed the mpu_feature branch from 5a9fff6 to d77ea67 Compare February 18, 2020 16:15

benpicco mentioned this pull request Feb 18, 2020

code: relax max line length to 120 #13400

Closed

benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 20, 2020

kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 24, 2020

aabadie self-assigned this Feb 25, 2020

aabadie requested changes Feb 25, 2020

View reviewed changes

aabadie reviewed Mar 3, 2020

View reviewed changes

tests/mpu_stack_guard/main.c Outdated Show resolved Hide resolved

kaspar030 added 2 commits March 3, 2020 22:58

tests/mpu_stack_guard: error unless __MPU_PRESENT

b154784

tests/mpu_stack_guard: update Makefile.ci

cbca43b

kaspar030 force-pushed the mpu_feature branch from 1140f50 to c6c865a Compare March 3, 2020 21:58

cpu/*: add cortex_mpu to known-to-support CPU families

a3c527f

kaspar030 force-pushed the mpu_feature branch from c6c865a to a3c527f Compare March 3, 2020 21:59

aabadie approved these changes Mar 4, 2020

View reviewed changes

aabadie merged commit 68ec8b2 into RIOT-OS:master Mar 4, 2020

leandrolanzieri added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Mar 13, 2020

leandrolanzieri added this to the Release 2020.04 milestone Mar 13, 2020

kaspar030 deleted the mpu_feature branch March 30, 2020 19:28

		@@ -1,3 +1,4 @@
		BOARD_INSUFFICIENT_MEMORY := \
		nucleo-f031k6 \

Conversation

kaspar030 commented Feb 17, 2020

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

benpicco Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benpicco commented Feb 18, 2020

Uh oh!

kaspar030 commented Feb 18, 2020

Uh oh!

kaspar030 commented Feb 18, 2020

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Feb 25, 2020

Uh oh!

aabadie commented Feb 25, 2020

Uh oh!

benpicco commented Feb 25, 2020

Uh oh!

kaspar030 commented Feb 25, 2020

Uh oh!

kaspar030 commented Feb 25, 2020

Uh oh!

benpicco commented Feb 25, 2020

Uh oh!

kaspar030 commented Feb 25, 2020

Uh oh!

benpicco commented Feb 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaspar030 commented Mar 2, 2020

Uh oh!

benpicco commented Mar 2, 2020

Uh oh!

kaspar030 commented Mar 2, 2020

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Mar 3, 2020

Uh oh!

Uh oh!

aabadie commented Mar 3, 2020

Uh oh!

benpicco Feb 17, 2020 •

edited

Loading

aabadie Feb 17, 2020 •

edited

Loading

aabadie Feb 17, 2020 •

edited

Loading

benpicco commented Feb 25, 2020 •

edited

Loading