Fixup ARM MPU to use opaque types / Rebase of #500 #987
Conversation
agross-oss
requested review from
andrewboie,
dbkinder,
galak and
MaureenHelm
as code owners
|
|
||
| while (1) { | ||
| k_sleep(SLEEPTIME); | ||
| } | ||
| } | ||
|
|
||
| k_sleep(SCHEDTIME); |
There was a problem hiding this comment.
any particular reason why we need a delay here? if the messages from different threads are clobbering each other in the console output you could consider locking the scheduler around the printk()
There was a problem hiding this comment.
I have no idea why that was added. I'll check it out and see if that is the reason. If so, I'll change to be as you suggested.
There was a problem hiding this comment.
After looking at this more, it doesn't make any difference having it or not. So I am going to remove it.
| /* stack_guard_thread is a dynamic thread */ | ||
| void stack_guard_thread(void *dummy1, void *dummy2, void *dummy3) | ||
| { | ||
| ARG_UNUSED(dummy1); | ||
| ARG_UNUSED(dummy2); | ||
| ARG_UNUSED(dummy3); | ||
| while (1) | ||
| recursive_loop(0, (int)dummy1, dummy2); |
There was a problem hiding this comment.
Also you have ARG_UNUSED(dummy1) and ARG_UNUSED(dummy2), but then they now get used. This definitely needs to be changed.
| @@ -35,33 +35,40 @@ To build the test with the MPU enabled and the stack guard feature present: | |||
| Sample Output | |||
| ============= | |||
|
|
|||
| With the MPU disabled: | |||
| With the MPU enabled but the stack guard feature disabled: | |||
There was a problem hiding this comment.
since you used the phrase"stack guard feature disabled" here, should line 62 below say "stack guard feature enabled" to be consistent?
There was a problem hiding this comment.
yes, ill make that change.
include/arch/arm/arch.h
Outdated
| * @param nmemb Number of stacks to declare | ||
| * @param size Size of the stack memory region | ||
| */ | ||
|
|
There was a problem hiding this comment.
tradition has it this blank line shouldn't be here
|
As an aside, the test code is just a sample, and doesn't programmatically determine whether stack overflow protection is working correctly. We really need to convert this into a proper test case, my suggestion would be to merge this code with tests/kernel/fatal which also does some stack overflow scenarios and verifies that the expected exception type was triggered. Also despite its name, the test code isn't MPU specific; with PR #982 applied, it works great on qemu_x86. These changes to the test can happen another in another patch, but I thought I'd mention them. |
|
|
||
| while (1) { | ||
| k_sleep(SLEEPTIME); | ||
| } | ||
| } | ||
|
|
||
| k_sleep(SCHEDTIME); |
There was a problem hiding this comment.
I have no idea why that was added. I'll check it out and see if that is the reason. If so, I'll change to be as you suggested.
| /* stack_guard_thread is a dynamic thread */ | ||
| void stack_guard_thread(void *dummy1, void *dummy2, void *dummy3) | ||
| { | ||
| ARG_UNUSED(dummy1); | ||
| ARG_UNUSED(dummy2); | ||
| ARG_UNUSED(dummy3); | ||
| while (1) | ||
| recursive_loop(0, (int)dummy1, dummy2); |
|
|
||
| while (1) { | ||
| k_sleep(SLEEPTIME); | ||
| } | ||
| } | ||
|
|
||
| k_sleep(SCHEDTIME); |
There was a problem hiding this comment.
After looking at this more, it doesn't make any difference having it or not. So I am going to remove it.
The mimimum mpu size is 32 bytes, but requires mpu base address to be aligned on 32 bytes to work. Define architecture thread macro when MPU_STACK_GUARD config to allocate stack with 32 more bytes. Signed-off-by: Michel Jaouen <[email protected]>
This test launches several threads. Only last thread will overflow the stack and an mpu exception will occur. The other threads are regulary suspended, this triggers access to kthread structure. This tests is failed on st and nxp platform, if 32 bytes alignement is not set. Signed-off-by: Michel Jaouen <[email protected]>
Following test changes, log info have changes and previous log info with stack guard present shows an mpu error not caused by a stack overflow caught by mpu stack guard feature. Signed-off-by: Michel Jaouen <[email protected]>
This patch always defines the ARCH_THREAD_STACK_XXX macros/functions regardless of the MPU_STACK_GUARD usage. Only use MPU_STACK_GUARD when determining the minimum stack alignment. Signed-off-by: Andy Gross <[email protected]>
This patch adjusts the ARM MPU implementation to be compliant to the recent changes that introduced the opaque kernel data types. Signed-off-by: Andy Gross <[email protected]>
Fixes zephyrproject-rtos#977 Signed-off-by: Jimmy Huang <[email protected]>
This set of patches takes jamike's #500 and rebases them on top of master. Additionally, this fixes up the code to use the new opaque kernel types and fixes the outstanding review comments from #500.