arm: clean up MPU code for ARM and NXP #7902

andrewboie · 2018-05-25T00:19:36Z

We are now much better at not reserving unnecessary
system MPU regions based on configuration. The #defines
for intent are now an enumerated type. As a bonus, the
implementation of _get_region_index_by_type() is much
simpler. Previously we were wasting regions for stack guard
and application memory if they were not configured.
Certain parts of the MPU code are now properly ifdef'd
based on configuration.
THREAD_STACK_REGION and THREAD_STACK_USER_REGION was a
confusing construction and has now been replaced with
just THREAD_STACK_REGION, which represents the MPU region
for a user mode thread stack. Supervisor mode stacks
do not require an MPU region.
The bounds of CONFIG_APPLICATION_MEMORY never changes
and we just do it once during initialization instead of
every context switch.
Assertions have been added to catch out-of-bounds cases.

Signed-off-by: Andrew Boie andrew.p.boie@intel.com

codecov-io · 2018-05-25T00:47:42Z

Codecov Report

Merging #7902 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #7902   +/-   ##
=======================================
  Coverage   63.95%   63.95%           
=======================================
  Files         429      429           
  Lines       41281    41281           
  Branches     6957     6957           
=======================================
  Hits        26403    26403           
  Misses      11720    11720           
  Partials     3158     3158

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85c1522...7230593. Read the comment docs.

andrewboie · 2018-05-25T04:51:05Z

Passes my testing on sam_e70_xplained (ARM MPU) and frdm_k64f (NXP MPU)

ioannisg · 2018-05-25T09:23:26Z

Maybe, change the commit message, so it starts with "arch: arm:" :)

ioannisg · 2018-05-25T10:38:21Z

arch/arm/core/cortex_m/mpu/arm_mpu.c

-	}
+	u32_t region_index;
+
+	__ASSERT(type < THREAD_MPU_REGION_LAST,


Looks like, the first __ASSERT needed for the case when "type" is invalid, but region_index is valid. So, most likely, for wrap-around, correct?

this assert is to detect if an unknown value of type is passed in.

the second one is to determine if the computed region index exceeds the number of available regions.

ioannisg · 2018-05-25T10:40:13Z

arch/arm/core/cortex_m/mpu/arm_mpu.c

@@ -377,8 +329,33 @@ int arm_core_mpu_get_max_domain_partition_regions(void)
 		_get_region_index_by_type(THREAD_DOMAIN_PARTITION_REGION);
 }

+/**
+ * This internal function check if the region is user accessible or not


check --> checks

ioannisg · 2018-05-25T11:08:10Z

Good clean-up (reviewed all but the NXP part)

ioannisg · 2018-05-25T13:19:52Z

arch/arm/core/cortex_m/mpu/arm_mpu.c

 {
 	u32_t r_index;

-	/* ARM MPU supports up to 16 Regions */


This was probably wrong info: AFAIK, Cortex-M4 MPU supports up to 8 regions.

i saw this in a document somewhere. You can have more than 8 regions, it's not capped to 8 but it is implementation specific.

@agross-linaro I think you're right, ARMv7-m allows more regions. Didn't find the info that limits this. MPU_TYPE.DREGION is 8-b long, so it needs to be at most 256.

However, Cortex-M3,4 implement 8, so the comment was a bit confusing, for most of developers that work on M4 and M4

lpereira · 2018-05-25T18:08:44Z

arch/arm/core/cortex_m/mpu/nxp_mpu.c

@@ -44,6 +48,19 @@ static inline u8_t _get_num_regions(void)
 	return FSL_FEATURE_SYSMPU_DESCRIPTOR_COUNT;
 }

+static inline u8_t _get_num_usable_regions(void)
+{
+	int max = _get_num_regions();


_get_num_regions() returns an u8_t; please use it for the type of max to avoid confusions with signed/unsigned values, and implicit conversions between byte and int

lpereira · 2018-05-25T18:12:36Z

arch/arm/core/cortex_m/mpu/nxp_mpu.c

@@ -408,7 +390,7 @@ int arm_core_mpu_buffer_validate(void *addr, size_t size, int write)
 	u32_t r_index;

 	/* Iterate all mpu regions */
-	for (r_index = 0; r_index < _get_num_regions(); r_index++) {
+	for (r_index = 0; r_index < _get_num_usable_regions(); r_index++) {


_get_num_regions() or _get_num_usable_regions() unfortunately can't be marked with attribute((pure)), so the compiler will most likely keep calling them. Wouldn't be better to cache their value in an u8_t, and define r_index as an u8_t as well so that they're of the same type?

ioannisg · 2018-05-29T10:11:15Z

Should we try to get this in, for the upcoming release?

pfalcon · 2018-05-29T10:44:19Z

Looks like a massive change for a release which should happen in 2 days...

andrewboie · 2018-05-29T21:30:05Z

I'm OK with pushing to 1.13.

pizi-nordic · 2018-06-04T11:43:31Z

arch/arm/core/cortex_m/mpu/arm_mpu.c

+	r_ap = ARM_MPU_DEV->rasr & ACCESS_PERMS_MASK;
+
+	/* always return true if this is the thread stack region */
+	if (_get_region_index_by_type(THREAD_STACK_REGION) == r_index) {


Why this condition is needed? Will r_ap == P_RW_U_RW check fail there?

This might be a throwback to when the stack region index was the same for the privileged and unprivileged stack. The permissions on those are different and I believe this check would fail because it was stricter for the privileged stack. With the changes done to split out the privileged stack, we don't have to worry about the locality of the stack as it's in a completely different memory region and any user mode access to that memory would fail.

Is this function ever called when the thread is running privileged mode?

ioannisg · 2018-06-13T15:44:27Z

Could you please rebase this, @andrewboie , and see what is left of this PR? I think it overlaps in part with several PRs recently merged in master.

* We are now *much* better at not reserving unnecessary system MPU regions based on configuration. The #defines for intent are now an enumerated type. As a bonus, the implementation of _get_region_index_by_type() is much simpler. Previously we were wasting regions for stack guard and application memory if they were not configured. * NXP MPU doesn't reserve the last region if HW stack protection isn't enabled. * Certain parts of the MPU code are now properly ifdef'd based on configuration. * THREAD_STACK_REGION and THREAD_STACK_USER_REGION was a confusing construction and has now been replaced with just THREAD_STACK_REGION, which represents the MPU region for a user mode thread stack. Supervisor mode stacks do not require an MPU region. * The bounds of CONFIG_APPLICATION_MEMORY never changes and we just do it once during initialization instead of every context switch. * Assertions have been added to catch out-of-bounds cases. Fixes: zephyrproject-rtos#7384 Signed-off-by: Andrew Boie <andrew.p.boie@intel.com>

ioannisg · 2018-06-27T06:58:09Z

Thank you, @andrewboie , I suggest we try to merge this one before #8467, #8554.

ioannisg

Nice clean-up, I left some suggestions in arm_mpu.c, but same comments apply for the nxp_mpu.c

ioannisg · 2018-06-27T07:55:45Z

arch/arm/core/cortex_m/mpu/arm_mpu.c

@@ -60,21 +60,27 @@ static inline u32_t _size_to_mpu_rasr_size(u32_t size)
 */
 static inline u32_t _get_region_attr_by_type(u32_t type, u32_t size)
 {
+#if defined(CONFIG_USERSPACE) || defined(CONFIG_MPU_STACK_GUARD) || \


As far as I can see, _get_region_attr_by_type(..) can be placed, entirely, within this composite #if guard.
It is used in arm_mpu_configure_xxxx(..) functions, which depend on the same directives (e.g. CONFIG_USERSPACE, CONFIG_APPLICATION_MEMORY).

Note that: arm_core_mpu_configure(..) appears to be implemented unconditionally, but, in fact, it is used only if the CONFIG_MPU_STACK_GUARD is defined. So, that function can (and should) also be placed in the same #if guards.

can we make this additional refinement in another PR

ioannisg · 2018-06-27T08:39:43Z

arch/arm/core/cortex_m/mpu/arm_mpu.c

@@ -122,35 +128,18 @@ static void _region_init(u32_t index, u32_t region_addr,
 */
 static inline u32_t _get_region_index_by_type(u32_t type)


This function is greatly simplified.

ioannisg · 2018-06-27T08:47:07Z

arch/arm/core/cortex_m/mpu/arm_mpu.c

+	r_ap = ARM_MPU_DEV->rasr & MPU_RASR_AP_Msk;
+
+	/* always return true if this is the thread stack region */
+	if (_get_region_index_by_type(THREAD_STACK_REGION) == r_index) {


Here the answer is "by design"

ioannisg · 2018-06-27T08:47:31Z

arch/arm/core/cortex_m/mpu/arm_mpu.c

+	}
+
+	if (write) {
+		return r_ap == P_RW_U_RW;


Here the answer is "by checking hardware at runtime"

ioannisg · 2018-06-27T08:47:35Z

arch/arm/core/cortex_m/mpu/arm_mpu.c

+	}
+
+	/* For all user accessible permissions, their AP[1] bit is l */
+	return r_ap & (0x2 << MPU_RASR_AP_Pos);


Here the answer is "by checking hardware at runtime"

ioannisg · 2018-06-27T08:49:46Z

arch/arm/core/cortex_m/mpu/arm_mpu.c

+ * Note:
+ *   The caller must provide a valid region number.
+ */
+static inline int _is_user_accessible_region(u32_t r_index, int write)


I know that this change was just due to the relocation of the function, however, it give me the opportunity to bring up a question: _is_user_accessible_region(..) appears to mix up design with run-time hardware status, explained inline in following comments. So I wonder whether we could have a unified behavior, instead.

Basically, this boils down to the following question: Since the type of region determines the permissions, why do we need to ask the hardware in runtime?

can this change be done in another PR

ioannisg · 2018-06-27T08:52:14Z

arch/arm/core/cortex_m/mpu/arm_mpu.c

 /**
 * @brief validate the given buffer is user accessible or not
+ *
+ * Presumes the background mapping is NOT user accessible.


This is true by design in ARM MPU (@agross-linaro please correct if I am wrong), when the MPU is enabled. But I guess, we always enable MPU when we compile-in this file.

nobody is calling this function without the MPU turned on

ioannisg · 2018-06-27T08:56:42Z

arch/arm/core/cortex_m/mpu/arm_mpu.c

@@ -459,9 +437,11 @@ static void _arm_mpu_config(void)
 			mpu_config.num_regions,
 			_get_num_regions()
 		);
-		return;
+		return -1;


Isn't it better to use some standard Zephyr error code here, e.g. -EINVAL or some other?

I don't see the point as this gets called by init code which just checks success/failure

That's right

ioannisg

Approving this. Some suggested changes can be tracked on future PR

andrewboie added the DNM This PR should not be merged (Do Not Merge) label May 25, 2018

andrewboie requested review from galak and MaureenHelm as code owners May 25, 2018 00:19

andrewboie requested a review from agross-oss May 25, 2018 00:19

nashif requested a review from ioannisg May 25, 2018 01:07

andrewboie force-pushed the mpu-code-cleanup branch 2 times, most recently from f7e6895 to 3ceee05 Compare May 25, 2018 04:21

andrewboie removed the DNM This PR should not be merged (Do Not Merge) label May 25, 2018

andrewboie requested review from lpereira, nashif, andyross, carlescufi, pizi-nordic, rgundi and ramakrishnapallala May 25, 2018 04:51

andrewboie added bug The issue is a bug, or the PR is fixing a bug area: ARM ARM (32-bit) Architecture area: Memory Protection labels May 25, 2018

andrewboie force-pushed the mpu-code-cleanup branch from 3ceee05 to cb28c0a Compare May 25, 2018 05:13

ioannisg reviewed May 25, 2018

View reviewed changes

ioannisg requested changes May 25, 2018

View reviewed changes

ioannisg reviewed May 25, 2018

View reviewed changes

ruuddw requested review from finikorg, jfischer-no, jukkar and pfalcon as code owners May 25, 2018 14:50

ruuddw requested review from ruuddw, spoorthik and tbursztyka as code owners May 25, 2018 14:50

andrewboie force-pushed the mpu-code-cleanup branch 2 times, most recently from 31efbb1 to 8ba8056 Compare May 25, 2018 15:04

lpereira reviewed May 25, 2018

View reviewed changes

andrewboie force-pushed the mpu-code-cleanup branch from 8ba8056 to 4b98cf7 Compare May 25, 2018 20:39

andrewboie added this to the v1.13.0 milestone May 29, 2018

ioannisg mentioned this pull request May 30, 2018

Arch arm mpu assert additions and fixes #8028

Merged

andrewboie closed this May 31, 2018

andrewboie deleted the mpu-code-cleanup branch May 31, 2018 19:23

andrewboie restored the mpu-code-cleanup branch May 31, 2018 20:09

andrewboie reopened this May 31, 2018

pizi-nordic reviewed Jun 4, 2018

View reviewed changes

andrewboie force-pushed the mpu-code-cleanup branch from 4b98cf7 to 7230593 Compare June 26, 2018 20:18

ioannisg requested changes Jun 27, 2018

View reviewed changes

ioannisg approved these changes Jun 27, 2018

View reviewed changes

andrewboie merged commit 8bcffef into zephyrproject-rtos:master Jun 27, 2018

andrewboie deleted the mpu-code-cleanup branch June 27, 2018 19:56

ioannisg mentioned this pull request Jun 28, 2018

Arch arm v8m mpu reorg arm mpu #8603

Merged

		@@ -122,35 +128,18 @@ static void _region_init(u32_t index, u32_t region_addr,
		*/
		static inline u32_t _get_region_index_by_type(u32_t type)

arm: clean up MPU code for ARM and NXP #7902

arm: clean up MPU code for ARM and NXP #7902

Conversation

andrewboie commented May 25, 2018

codecov-io commented May 25, 2018 • edited Loading

Codecov Report

andrewboie commented May 25, 2018

ioannisg commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioannisg commented May 25, 2018

ioannisg May 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioannisg commented May 29, 2018

pfalcon commented May 29, 2018

andrewboie commented May 29, 2018

pizi-nordic Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioannisg commented Jun 13, 2018

ioannisg commented Jun 27, 2018

ioannisg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioannisg left a comment

Choose a reason for hiding this comment

codecov-io commented May 25, 2018 •

edited

Loading

ioannisg May 25, 2018 •

edited

Loading

pizi-nordic Jun 4, 2018 •

edited

Loading