soc: mps2_an385: Added support for MPU #7035

AdithyaBaglody · 2018-04-12T09:54:56Z

Added MPU support for MPS2_AN385

codecov-io · 2018-04-12T10:24:45Z

Codecov Report

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

@@          Coverage Diff           @@
##           master   #7035   +/-   ##
======================================
  Coverage    52.5%   52.5%           
======================================
  Files         213     213           
  Lines       26140   26140           
  Branches     5634    5634           
======================================
  Hits        13726   13726           
  Misses      10159   10159           
  Partials     2255    2255

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 a013ce3...33af085. Read the comment docs.

galak · 2018-04-12T10:51:20Z

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

@@ -43,6 +43,13 @@ static inline u32_t _size_to_mpu_rasr_size(u32_t size)
 		return REGION_32B;
 	}

+#ifdef CONFIG_SOC_SERIES_MPS2


This is because of QEMU, not the actual hardware for MPS2_AN385

Could we perhaps make this a Kconfig instead? And set to 1024 in the qemu_cortex_m3 defconfig

How can I inform the build system that I am using qemu rather than a real hardware.
Is that I need to create a new board for this, say qemu_mps2_an385?

no, just change qemu_cortex_m3 to use this SOC definition instead of the other one.
And then ifdef this based on BOARD=qemu_cortex_m3

This causes build failures on all Bluetooth related tests/samples. This change would be backward incompatible.
So another solution may be something like BOARD=qemu_cortex_m3_mpu

galak

The various places that test against SOC_SERIES_MPS2 aren't correct, the actual hw is fine, its QEMU that has the 1k limitation.

andrewboie · 2018-04-12T19:33:25Z

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

@@ -43,6 +43,13 @@ static inline u32_t _size_to_mpu_rasr_size(u32_t size)
 		return REGION_32B;
 	}

+#ifdef CONFIG_SOC_SERIES_MPS2


Could we perhaps make this a Kconfig instead? And set to 1024 in the qemu_cortex_m3 defconfig

andrewboie · 2018-04-12T19:34:09Z

boards/arm/mps2_an385/mps2_an385_defconfig

+#userspace
+CONFIG_ARCH_HAS_USERSPACE=y
+CONFIG_ARM_MPU=y
+CONFIG_MAIN_STACK_SIZE=2048


Why do we need to override the main/idle/privileged stack size from defaults?

Qemu cant handle stacks lesser than 1024. This will be a region i need to congfigure in the MPU and the qemu has a 1k limitation.

then why aren't you changing this in qemu_cortex_m3 board definition?

These stack size changes need to be moved out of the mps2_an385 configuration, I can't merge this since this is really something that affects QEMU and not the actual mps2_an385 board.

@AdithyaBaglody you never addressed the comment above.

oh I had actually addressed it before. Now after enabling the userspace(in the last 2days) this came back in.
will fix it. Thanks for catching this.

andrewboie · 2018-04-12T19:34:32Z

scripts/gen_alignment_script.py

    if bit_len:
-        align_size = 1 << bit_len
+        if "CONFIG_SOC_SERIES_MPS2" in syms and bit_len < 10:


yeah another hack here -- let's just pull this out of kconfig

andrewboie · 2018-04-12T19:35:46Z

tests/kernel/mem_protect/userspace/src/main.c

@@ -324,7 +324,11 @@ static void pass_noperms_object(void)

 __kernel struct k_thread kthread_thread;

+#ifdef CONFIG_SOC_SERIES_MPS2
+#define STACKSIZE 1024


just make this 1024 for everyone IMO, 512 was selected at random IIRC and it's not like MPU-enabled devices have really tiny amounts of RAM

i agree. i always wondered about that. If people want to minimize that stuff they can add pare it back down sub <1k.

andrewboie

Getting close, but a few changes still needed.

Any details on the bluetooth issues you are seeing?

andrewboie · 2018-04-25T20:01:44Z

boards/arm/mps2_an385/mps2_an385_defconfig

+#userspace
+CONFIG_ARCH_HAS_USERSPACE=y
+CONFIG_ARM_MPU=y
+CONFIG_MAIN_STACK_SIZE=2048


These stack size changes need to be moved out of the mps2_an385 configuration, I can't merge this since this is really something that affects QEMU and not the actual mps2_an385 board.

andrewboie · 2018-04-25T20:02:04Z

scripts/gen_alignment_script.py

    else:
-        align_size = 32
+            align_size = min_region_size


indentation is incorrect here.

galak · 2018-04-26T18:35:06Z

arch/arm/soc/arm/mps2/arm_mpu_regions.c

+#include <soc.h>
+#include <arch/arm/cortex_m/mpu/arm_mpu.h>
+
+#define FLASH_BASE_ADDRESS (0x00000000)


Why aren't these coming from DT?

yes this should come from DTS

we do not have a DTS file for mps2_an385. So currently this will be kept here until we have one.

I'm going to defer to @galak on whether this is permissible.

we do not have a DTS file for mps2_an385. So currently this will be kept here until we have one.

boards/arm/mps2_an385/mps2_an385.dts ???

andrewboie

Dependencies: QEMU version 2.12

This should NOT be dependent on QEMU 2.12.
Separate out any work in QEMU 2.12 into another PR.
The mps3_an385 is a real piece of hardware with a working MPU.
Issues with QEMU are separate and need to be dealt with separately.

andrewboie · 2018-05-08T15:20:46Z

boards/arm/mps2_an385/mps2_an385_defconfig

+#userspace
+CONFIG_MAIN_STACK_SIZE=2048
+CONFIG_IDLE_STACK_SIZE=1024
+CONFIG_PRIVILEGED_STACK_SIZE=1024


this is unacceptable, you are hacking the stack sizes because of QEMU limitations and not the mps2_am385

we have quite a lot of memory on the actual board. So if someone wants to run it on qemu he shouldn't face the issue i did when i started out.

No, do not change the configuration for the real mps2_an385 board. This belongs in the qemu_cortex_m3 defconfig once it is updated to use the MPU.

andrewboie · 2018-05-08T15:21:10Z

samples/mpu/mem_domain_apis_test/src/main.c

@@ -10,7 +10,7 @@
 #include <misc/printk.h>

 /* size of stack area used by each thread */
-#define STACKSIZE 1024
+#define STACKSIZE CONFIG_MAIN_STACK_SIZE


why is this changed? this has the effect of making the stack size smaller on other boards.

This is one of the smiplest ways to get it to work on qemu. this sample needs 2k of stack and i didn't want to do a #if def CONFIG_BOARD_QEMU_m3. Maybe i will end up doing just that.

andrewboie · 2018-05-08T15:21:35Z

tests/benchmarks/sys_kernel/testcase.yaml

@@ -1,5 +1,6 @@
 tests:
  benchmark.kernel:
    arch_exclude: nios2 riscv32 xtensa
+    platform_exclude: qemu_cortex_m3


why is this removed?

the prj.conf streches the time between timer interrupts. This on qemu will overflow the counter.
And what is the point in running a benchmark on an emulation platform?

the prj.conf streches the time between timer interrupts. This on qemu will overflow the counter

Why is this not a problem on any of our other emulated platforms?

And what is the point in running a benchmark on an emulation platform?

Because the test still reports success or failure, and sanitycheck only runs on emulated platforms. If there is some functional issue that causes this test to crash we need to know about it.

Why is this not a problem on any of our other emulated platforms?

In arm we have only 24 bits to represent the count value. This maybe one reason. The other the clock speed on other platforms might be lesser. So it ends up streaching the timer Interrupts according to the prj.conf.

Because the test still reports success or failure, and sanitycheck only runs on emulated platforms. If there is some functional issue that causes this test to crash we need to know about it.

As mentioned above, this is to do with the timer interrupts. One workaround would be to create a new prj.conf for qemu_cortex_m3.

andrewboie · 2018-05-08T15:23:16Z

arch/arm/soc/arm/mps2/arm_mpu_regions.c

+#include <soc.h>
+#include <arch/arm/cortex_m/mpu/arm_mpu.h>
+
+#define FLASH_BASE_ADDRESS (0x00000000)


yes this should come from DTS

AdithyaBaglody · 2018-05-09T04:24:58Z

This should NOT be dependent on QEMU 2.12.
Separate out any work in QEMU 2.12 into another PR.
The mps3_an385 is a real piece of hardware with a working MPU.
Issues with QEMU are separate and need to be dealt with separately.

@andrewboie That description was a old one. I have already split the PR

andrewboie · 2018-05-09T18:26:39Z

samples/philosophers/src/main.c

@@ -83,7 +83,7 @@
 /* end - control behaviour of the demo */
 /***************************************/

-#define STACK_SIZE 768
+#define STACK_SIZE 1024


I'm trying to understand something here.
IIRC, On ARM, the guard region for stacks is taken from the existing stack size. It is not added like on x86.
Wouldn't a stack size of 1024 be entirely consumed by the guard area, leaving no room for the actual stack buffer?

yes. if the MPU guard is enabled it will consume 1K out of the stack size.
but I want to create a warning that will inform the users of this limitation when we refactor qemu_cortex_m3.

You lost me.
If 1K is taken out of the stack, and we only have 1K for STACK_SIZE, how does this work??

MPU Stack guard is not enabled by default. Hence this works.

MPU stack guard is now enabled by default for tests.
We don't have the 1K limitation any more, correct?

nashif · 2018-05-17T13:52:40Z

tests/benchmarks/sys_kernel/testcase.yaml

@@ -3,3 +3,7 @@ tests:
    arch_exclude: nios2 riscv32 xtensa
    min_ram: 32
    tags: benchmark
+  benchmark.kernel.qemu_cortex_m3:


not sure how this is related to this PR.

Some of these patches are need to keep the sanity when we move to the new QEMU.
So such patches need to be ready before we update the SDK with the new QEMU.

nashif · 2018-05-17T13:53:05Z

tests/benchmarks/sys_kernel/prj_1.conf

@@ -0,0 +1,8 @@
+CONFIG_TEST=y


do not use number in file names, call it with something meaningful

nashif · 2018-08-27T16:40:49Z

@AdithyaBaglody 2 tests are failing for me:

tests/kernel/mem_pool/mem_pool_threadsafe/kernel.memory_pool
and
tests/kernel/mem_protect/syscalls/kernel.memory_protection.syscalls

AdithyaBaglody · 2018-08-28T10:15:08Z

@AdithyaBaglody 2 tests are failing for me:
tests/kernel/mem_pool/mem_pool_threadsafe/kernel.memory_pool
and
tests/kernel/mem_protect/syscalls/kernel.memory_protection.syscalls

Anas can you please check with this rebase. I am not seeing this issue on my end.

nashif · 2018-08-28T22:06:46Z

tests/kernel/mem_pool/mem_pool_threadsafe still fails, the syscalls one is probably expected to fault, we see this in other places. @dcpleung

dcpleung · 2018-08-29T01:13:15Z

I ran sanitycheck with QEMU 3.0.0 couple times in a row. Every time it fails of different set of tests. Maybe it's a QEMU issue?

nashif · 2018-08-29T02:31:48Z

I ran sanitycheck with QEMU 3.0.0 couple times in a row. Every time it fails of different set of tests. Maybe it's a QEMU issue?

i am running qemu from head, so post 3.0.0

AdithyaBaglody · 2018-08-29T05:05:38Z

@dcpleung You need to take the latest code i.e the repo head. Needed for MPU to work correctly.
@nashif I am using a qemu that is 1/2 week old. There it seems to work just fine. Let me check with the latest repo head.

dcpleung · 2018-08-29T20:59:29Z

I grabbed QEMU from git this morning (v3.0.0-614-g19b599f766), and it still gave me different set of failures between sanitycheck runs. Some of them are related to timers (e.g. too many ticks elapsed waiting for a task). However, I got a few USAGE FAULTs complaining about illegal use of EPSR and a few BUS FAULTS also.

AdithyaBaglody · 2018-09-10T08:10:51Z

So if you want to run it on qemu. you need to add the following to the defconfig
CONFIG_MAIN_STACK_SIZE=1024
CONFIG_IDLE_STACK_SIZE=1024
CONFIG_PRIVILEGED_STACK_SIZE=1024
CONFIG_TEST_EXTRA_STACKSIZE=1024

After adding these changes on the latest qemu and it is working fine.

nashif · 2018-09-15T03:30:55Z

@AdithyaBaglody this now only failed on one test:

samples/basic/userspace/shared_mem

QEMU: ***** Booting Zephyr OS zephyr-v1.13.0-146-gaa839c2245 *****
QEMU: init partitions complete
QEMU: init app memory complete
QEMU: ENC Thread Created 200008b0
QEMU: init domain complete
QEMU: Partitions added to dom1
QEMU: dom1 Created
QEMU: PT Thread Created 20000a00
QEMU: dom0 Created
QEMU: CT Thread Created 20000974
QEMU: dom2 Created
QEMU: ***** MPU FAULT *****
QEMU: Data Access Violation
QEMU: MMFAR Address: 0x200000a0
QEMU: ***** Hardware exception *****
QEMU: Current thread ID = 0x200008b0
QEMU: Faulting instruction address = 0x4f6
QEMU: Fatal fault in thread 0x200008b0! Aborting.
QEMU: ENC thread started
QEMU: PT thread started
QEMU complete (timeout) after 60.059631 seconds
MAKE: 'sanity_test_finished mps2_an385/samples/basic/userspace/shared_mem/kernel.memory_protection.userspace'

AdithyaBaglody · 2018-09-19T06:16:08Z

@nashif this was not a problem on qemu. It seems like it caught an issue which was unnoticed till now.
Created a PR for the solution #10087

nashif · 2018-09-20T15:35:45Z

tests/benchmarks/sys_kernel/prj_qemu_cortex_m3.conf

@@ -0,0 +1,8 @@
+CONFIG_TEST=y


Please remove this patch from this PR, not related to mps2_an385

nashif · 2018-09-20T15:37:43Z

tests/benchmarks/sys_kernel/testcase.yaml

remove this change to this file, not sure why we have qemu_cortex_m3 in the tags

Increasing the stack size to 1024. Signed-off-by: Adithya Baglody <[email protected]>

Added required files/support for the MPU in MPS2-AN358 (Cortex-M3). Signed-off-by: Adithya Baglody <[email protected]> Signed-off-by: Anas Nashif <[email protected]>

The code assumes that when the systick counter hits zero, the timer interrupt will be taken before the loop can read the LOAD/VAL registers, but this is not architecturally guaranteed, and so the code can see a post-reload SysTick->VAL and a pre-reload clock_accumulated_count, which causes it to return an incorrectly small cycle count. By adding a ISB we overcome this issue. Signed-off-by: Adithya Baglody <[email protected]>

This patch will enable the MPU in MPS2_AN385 soc. Signed-off-by: Adithya Baglody <[email protected]> Signed-off-by: Anas Nashif <[email protected]>

This patch will enable the userspace. Signed-off-by: Adithya Baglody <[email protected]>

Now the stack size is a function of CONFIG_TEST_EXTRA_STACKSIZE. Signed-off-by: Adithya Baglody <[email protected]>

This patch updates the alignment for the memory domain partitions. Also update the stack size for qemu_cortex_m3. Signed-off-by: Adithya Baglody <[email protected]>

The test case was supposed to access the privileged stack area but instead it was accessing the stack guard region. Signed-off-by: Adithya Baglody <[email protected]>

The total number of loops being executed were taking far too long to finish executing the test case. This was causing the timer handler to execute in the middle of the test case. Thereby causing a test case failure. Now the number of loops have been reduced and this will make sure the test case completes. This change has no effect on the time being calculated by the benchmark. Signed-off-by: Adithya Baglody <[email protected]>

The header should check if the macro _MPU_PRESENT is defined and create it only if not defined. Signed-off-by: Adithya Baglody <[email protected]>

AdithyaBaglody added the area: Memory Protection label Apr 12, 2018

AdithyaBaglody requested review from nashif, andrewboie and agross-oss April 12, 2018 09:54

carlescufi requested a review from ioannisg April 12, 2018 10:42

galak reviewed Apr 12, 2018

View reviewed changes

galak requested changes Apr 12, 2018

View reviewed changes

andrewboie suggested changes Apr 12, 2018

View reviewed changes

AdithyaBaglody force-pushed the qemu_cortex_mpu branch from 69c8ba2 to eafab3e Compare April 25, 2018 06:12

andrewboie suggested changes Apr 25, 2018

View reviewed changes

galak previously requested changes Apr 26, 2018

View reviewed changes

AdithyaBaglody force-pushed the qemu_cortex_mpu branch from eafab3e to ce81a11 Compare May 8, 2018 05:58

This was referenced May 8, 2018

soc: mps2_an385: Enable Memory Protection unit. #7398

Closed

boards: arm: qemu_cortex_m3: Refactor to use mps2_an385. #7399

Closed

AdithyaBaglody force-pushed the qemu_cortex_mpu branch 3 times, most recently from 4a2b732 to b1ebc50 Compare May 8, 2018 09:08

andrewboie suggested changes May 8, 2018

View reviewed changes

AdithyaBaglody force-pushed the qemu_cortex_mpu branch from 15ba70d to 74befe7 Compare May 9, 2018 04:22

AdithyaBaglody requested a review from MaureenHelm as a code owner May 9, 2018 04:22

AdithyaBaglody force-pushed the qemu_cortex_mpu branch from 74befe7 to 9beba80 Compare May 9, 2018 09:04

andrewboie reviewed May 9, 2018

View reviewed changes

AdithyaBaglody force-pushed the qemu_cortex_mpu branch 3 times, most recently from bc79c66 to 31b15d1 Compare May 10, 2018 08:25

nashif requested changes May 17, 2018

View reviewed changes

AdithyaBaglody mentioned this pull request May 30, 2018

qemu_cortex_m3: enable MPU #4015

Closed

AdithyaBaglody force-pushed the qemu_cortex_mpu branch from 31b15d1 to 82ae816 Compare June 4, 2018 07:56

AdithyaBaglody force-pushed the qemu_cortex_mpu branch from 0fff2c6 to f3e6d2e Compare August 28, 2018 10:13

nashif added this to the v1.14.0 milestone Aug 29, 2018

nashif force-pushed the qemu_cortex_mpu branch 2 times, most recently from 3aab0b3 to aa839c2 Compare September 15, 2018 03:16

nashif approved these changes Sep 15, 2018

View reviewed changes

AdithyaBaglody force-pushed the qemu_cortex_mpu branch from aa839c2 to f6f4f8b Compare September 19, 2018 06:20

nashif requested changes Sep 20, 2018

View reviewed changes

AdithyaBaglody added 10 commits September 20, 2018 17:27

tests: userspace: Update the required stack size for mps2_an385

e8d9618

Increasing the stack size to 1024. Signed-off-by: Adithya Baglody <[email protected]>

soc: mps2_an385: Added support for MPU

ae96868

Added required files/support for the MPU in MPS2-AN358 (Cortex-M3). Signed-off-by: Adithya Baglody <[email protected]> Signed-off-by: Anas Nashif <[email protected]>

soc: mps2_an385: Enable Memory Protection unit.

379ab95

This patch will enable the MPU in MPS2_AN385 soc. Signed-off-by: Adithya Baglody <[email protected]> Signed-off-by: Anas Nashif <[email protected]>

boards: mps2_an385: Enable userspace for MPS2_AN385.

0d876fd

This patch will enable the userspace. Signed-off-by: Adithya Baglody <[email protected]>

tests: kernel: pipe : Update the stack size.

ab945bc

Now the stack size is a function of CONFIG_TEST_EXTRA_STACKSIZE. Signed-off-by: Adithya Baglody <[email protected]>

tests: mem_protect: mem_protect: Update the stack size.

23cc0eb

This patch updates the alignment for the memory domain partitions. Also update the stack size for qemu_cortex_m3. Signed-off-by: Adithya Baglody <[email protected]>

tests: userspace: Incorrect location to the privileged stack.

47a458e

The test case was supposed to access the privileged stack area but instead it was accessing the stack guard region. Signed-off-by: Adithya Baglody <[email protected]>

include: arm: CMSIS header redefining MPU_PRESENT.

33af085

The header should check if the macro _MPU_PRESENT is defined and create it only if not defined. Signed-off-by: Adithya Baglody <[email protected]>

nashif force-pushed the qemu_cortex_mpu branch from f6f4f8b to 33af085 Compare September 20, 2018 22:36

nashif approved these changes Sep 21, 2018

View reviewed changes

nashif merged commit 31c8817 into zephyrproject-rtos:master Sep 21, 2018

soc: mps2_an385: Added support for MPU #7035

soc: mps2_an385: Added support for MPU #7035

Conversation

AdithyaBaglody commented Apr 12, 2018 • edited Loading

codecov-io commented Apr 12, 2018 • edited Loading

Codecov Report

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

galak 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

andrewboie 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

andrewboie 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

AdithyaBaglody commented May 9, 2018

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

nashif commented Aug 27, 2018

AdithyaBaglody commented Aug 28, 2018

nashif commented Aug 28, 2018

dcpleung commented Aug 29, 2018

nashif commented Aug 29, 2018

AdithyaBaglody commented Aug 29, 2018

dcpleung commented Aug 29, 2018

AdithyaBaglody commented Sep 10, 2018 • edited Loading

nashif commented Sep 15, 2018

AdithyaBaglody commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdithyaBaglody commented Apr 12, 2018 •

edited

Loading

codecov-io commented Apr 12, 2018 •

edited

Loading

AdithyaBaglody commented Sep 10, 2018 •

edited

Loading