Stack unification and test #7238

mprse · 2018-06-18T11:36:12Z

Description

This PR provides test which verifies stack sizes:

ISR stack: exactly 1 kB,
Main thread stack: exactly 4 kB,
Default thread stack: exactly 4 kB,
RTOS-less thread stack: at least 4 kB - skipped for now since it looks like it is impossible to check RTOS-less thread (tests are built with RTOS).

Additionally PR provides fixes in linker scripts for some example targets. Can someone who has more experience with linker scripts check if these changes are correct? I could then proceed with the further targets.

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

bulislaw · 2018-06-19T10:24:11Z

@SeppoTakalo fyi

SeppoTakalo

Looks good.

SeppoTakalo · 2018-06-19T12:13:52Z

Can you also add checks for the boot time stack?

I believe those are the same as RTOS-less builds. For example IAR seems to define it as define symbol __ICFEDIT_size_cstack__ = 0x400;
and it is not the same as main thread stack, as main thread stack is allocated here: https://github.com/ARMmbed/mbed-os/blob/master/rtos/TARGET_CORTEX/mbed_boot.c#L189

So if we set stack size in linker file to 4kB it is used in boot, before RTX starts, and then we allocate another 4kB for main thread and kick the kernel to run.

However, RTX seems to suggest that 300 bytes should be enough: https://www.keil.com/pack/doc/CMSIS/RTOS/html/theory.html

SeppoTakalo · 2018-06-19T12:17:01Z

Or here: http://www.keil.com/support/man/docs/rlarm/rlarm_ar_technical_data.htm
it tells that 300 + 128 bytes for kernel.
which to me suggest, that within a linker file 428 should be the stack size we use. Not 4 kB

mprse · 2018-06-20T06:23:22Z

@SeppoTakalo In the test I am checking mbed_stack_isr_size which is defined based on CSTACK, ISR_STACK_SIZE symbols:

mbed-os/rtos/TARGET_CORTEX/mbed_boot.c

Lines 225 to 232 in eb3d3fd

    
           #if defined(__ICCARM__) 
        
               #pragma section="CSTACK" 
        
               #pragma section="HEAP" 
        
               #define HEAP_START          ((unsigned char*)__section_begin("HEAP")) 
        
               #define HEAP_SIZE           ((uint32_t)__section_size("HEAP")) 
        
               #define ISR_STACK_START     ((unsigned char*)__section_begin("CSTACK")) 
        
               #define ISR_STACK_SIZE      ((uint32_t)__section_size("CSTACK")) 
        
           #endif

mbed-os/rtos/TARGET_CORTEX/mbed_boot.c

Lines 266 to 275 in 19c795c

    
           #ifdef ISR_STACK_START 
        
               /* Interrupt stack explicitly specified */ 
        
               mbed_stack_isr_size = ISR_STACK_SIZE; 
        
               mbed_stack_isr_start = ISR_STACK_START; 
        
           #else 
        
               /* Interrupt stack -  reserve space at the end of the free block */ 
        
               mbed_stack_isr_size = ISR_STACK_SIZE < free_size ? ISR_STACK_SIZE : free_size; 
        
               mbed_stack_isr_start = free_start + free_size - mbed_stack_isr_size; 
        
               free_size -= mbed_stack_isr_size; 
        
           #endif

For example STM boards defines ISR_STACK_SIZE as follows (GCC_ARM):

mbed-os/targets/TARGET_STM/mbed_rtx.h

Line 136 in eb3d3fd

    
           #define ISR_STACK_SIZE        ((uint32_t)((uint32_t)__StackTop - (uint32_t)__StackLimit))

Now for specific TARGET_STM32L476xG family we have the following definitions:
IAR

mbed-os/targets/TARGET_STM/TARGET_STM32L4/TARGET_STM32L476xG/device/TOOLCHAIN_IAR/stm32l476xx.icf

Lines 25 to 28 in eb3d3fd

    
           define symbol __size_cstack__ = 0x7e00; 
        
           define symbol __size_heap__   = 0x10000; 
        
           define block CSTACK    with alignment = 8, size = __size_cstack__   { }; 
        
           define block HEAP      with alignment = 8, size = __size_heap__     { };

GCC_ARM

mbed-os/targets/TARGET_STM/TARGET_STM32L4/TARGET_STM32L476xG/device/TOOLCHAIN_GCC_ARM/STM32L476XX.ld

Lines 158 to 165 in eb3d3fd

    
           /* Set stack top to end of RAM, and stack limit move down by 
        
            * size of stack_dummy section */ 
        
           __StackTop = ORIGIN(SRAM2) + LENGTH(SRAM2); 
        
           _estack = __StackTop; 
        
           __StackLimit = __StackTop - SIZEOF(.stack_dummy); 
        
           PROVIDE(__stack = __StackTop); 
        
           /* Check if stack exceeds RAM2 limit */ 
        
           ASSERT((ORIGIN(SRAM2)+LENGTH(SRAM2)) >= __StackLimit, "SRAM2 overflow")

ARM
Not defined

According to the requirements I'm going to modify ISR stack for all targets to 1 kB. At least for GCC and IAR. I can't see how ISR stack size is defined for ARM compiler.
As far as I understand you suggest to set ISR stack size to ~0,5 kB?

deepikabhavnani · 2018-06-20T15:23:02Z

According to the requirements I'm going to modify ISR stack for all targets to 1 kB

Why set it to 1KB only? Some devices might need higher ISR stack can we have some good range instead?

deepikabhavnani · 2018-06-20T15:33:49Z

ARM Not defined

Below files show where ISR stack is set for ARM. Look for what is set in ISR_STACK_START for all targets. Many of them use same define "ARM_LIB_STACK" in scatter files

mbed-os/targets/TARGET_NUVOTON/mbed_rtx.h

Line 31 in 7b42891

#define ISR_STACK_START ((unsigned char*)Image$$ARM_LIB_STACK$$ZI$$Base)

mbed-os/targets/TARGET_NUVOTON/TARGET_NANO100/device/TOOLCHAIN_ARM_STD/NANO130.sct

Line 13 in 0977206

ARM_LIB_STACK 0x20000000 EMPTY 0x800 {

SeppoTakalo · 2018-06-20T16:00:48Z

@mprse

As far as I understand you suggest to set ISR stack size to ~0,5 kB?

No, this was based on my original understanding that boot time stack/main stack and ISR stacks are different.

But according to Cortex-M3 User Guide Reference Material: 2.1.2. Stacks
In Thread mode, the CONTROL register controls whether the processor uses the main stack or the process stack, see CONTROL register. In Handler mode, the processor always uses the main stack.

So clearly "boot" stack and ISR stacks are the same. And I tried to find, where we set that pointer, but it seems to be the first vector, so its the linker file + startup_*.S

According the Cortex-M3 Devices Generic User Guide: Stack Pointer

The Stack Pointer (SP) is register R13. In Thread mode, bit[1] of the CONTROL register indicates the stack pointer to use:
0 = Main Stack Pointer (MSP). This is the reset value.
1 = Process Stack Pointer (PSP).
On reset, the processor loads the MSP with the value from address 0x00000000.

So I was wrong assuming that checking ISR stack would not be enough, but those are the same.

Should we actually change the name, because my understanding is now that both RTX kernel and ISR both use this "main stack" where MSP register points, and each thread have their own stack where PSP points?

And @deepikabhavnani why would some devices need 4kB of ISR stack? Sound like a lot compared to RTX kernel requiring only 400 bytes. Do you know some examples?

At least within Mbed OS it is very limited what you can do in ISR, so the call path would be usually very short.

pan- · 2018-06-20T16:36:24Z

...ARGET_NORDIC/TARGET_NRF5x/TARGET_NRF52/TARGET_MCU_NRF52832/device/TOOLCHAIN_IAR/nRF52832.icf

@@ -40,7 +40,7 @@ export symbol __ICFEDIT_region_RAM_end__;

 /*-Sizes-*/
 /*Heap 1/4 of ram and stack 1/8*/
-define symbol __ICFEDIT_size_cstack__   = 0x800;


That would break BLE.

pan- · 2018-06-20T16:36:41Z

...F51/TARGET_MCU_NRF51822_UNIFIED/device/TOOLCHAIN_IAR/TARGET_MCU_NORDIC_32K/nRF51822_QFAA.icf

@@ -11,8 +11,8 @@ define symbol __ICFEDIT_region_RAM_end__   = 0x20007FFF;
 export symbol __ICFEDIT_region_RAM_start__;
 export symbol __ICFEDIT_region_RAM_end__;
 /*-Sizes-*/
-/*Heap 1/4 of ram and stack 1/8*/
-define symbol __ICFEDIT_size_cstack__   = 0x800;


That would break BLE.

pan-

IRQ Stack must be at least 2K large on Nordic targets. This is not an option to change it.

deepikabhavnani · 2018-06-20T16:53:35Z

And @deepikabhavnani why would some devices need 4kB of ISR stack

I never meant 4KB stack size as requirement. I would suggest to have a range based on analysis / input from target devices instead of hard-code 1KB.

mprse · 2018-06-21T09:20:41Z

Thanks for all your comments. They are really helpful!

Should we actually change the name, because my understanding is now that both RTX kernel and ISR both use this "main stack" where MSP register points, and each thread have their own stack where PSP points?

I agree that the name ("Interrupt stack") is misleading and it would be better to be consistent with the documentation and use names "main stack" (interrupt and boot time stack) and "process stack" (threads).

I never meant 4KB stack size as requirement. I would suggest to have a range based on analysis / input from target devices instead of hard-code 1KB.

I also agree that setting main stack/isr stack size to 1 KB for all boards might not be a good solution. For many cases it should be ok, but for sure there are cases when it is impossible (like BLE support on NRF boards pointed by @pan).

I found the following problematic cases:

Renesas targets are based on Cortex-A and stack is organised in the different way. We have different exception modes like Undefined instruction, Supervisor call, Abort, IRQ, FIQ and for each of them separate stack is used. Total stack size is defined as a sum of all these stacks which gives 92 KB.

mbed-os/targets/TARGET_RENESAS/TARGET_RZ_A1XX/TARGET_RZ_A1H/device/TOOLCHAIN_ARM_STD/MBRZA1H.sct

Line 44 in 443e37b

    
           ARM_LIB_STACK (__RAM_BASE + __NM_RAM_SIZE) EMPTY -__STACK_SIZE      ; Stack region growing down

mbed-os/targets/TARGET_RENESAS/TARGET_RZ_A1XX/TARGET_RZ_A1H/device/TOOLCHAIN_ARM_STD/mem_RZ_A1H.h

Lines 58 to 63 in 443e37b

    
           #define __UND_STACK_SIZE 0x00000100 
        
           #define __SVC_STACK_SIZE 0x00008000 
        
           #define __ABT_STACK_SIZE 0x00000100 
        
           #define __FIQ_STACK_SIZE 0x00000100 
        
           #define __IRQ_STACK_SIZE 0x0000F000 
        
           #define __STACK_SIZE     (__UND_STACK_SIZE + __SVC_STACK_SIZE + __ABT_STACK_SIZE + __FIQ_STACK_SIZE + __IRQ_STACK_SIZE)

mbed-os/targets/TARGET_RENESAS/TARGET_RZ_A1XX/TARGET_RZ_A1H/device/TOOLCHAIN_ARM_STD/startup_RZ_A1H.c

Lines 116 to 139 in 443e37b

    
           //Enter Undefined Instruction Mode and set its Stack Pointer  
        
           CPS    #UND_MODE 
        
           MOV    SP, R0 
        
           SUB    R0, R0, #__UND_STACK_SIZE 
        
           // Enter Abort Mode and set its Stack Pointer  
        
           CPS    #ABT_MODE 
        
           MOV    SP, R0 
        
           SUB    R0, R0, #__ABT_STACK_SIZE 
        
           // Enter FIQ Mode and set its Stack Pointer  
        
           CPS    #FIQ_MODE 
        
           MOV    SP, R0 
        
           SUB    R0, R0, #__FIQ_STACK_SIZE 
        
           // Enter IRQ Mode and set its Stack Pointer  
        
           CPS    #IRQ_MODE 
        
           MOV    SP, R0 
        
           SUB    R0, R0, #__IRQ_STACK_SIZE 
        
           // Enter Supervisor Mode and set its Stack Pointer  
        
           CPS    #SVC_MODE 
        
           MOV    SP, R0 
        
           SUB    R0, R0, #__SVC_STACK_SIZE

On some boards with very small amount of RAM the main stack/isr stack size is currently less than 1 KB.
TARGET_KL05Z: main stack/isr stack size is set to 512 B. This board has only 4 KB RAM.

mbed-os/targets/TARGET_Freescale/TARGET_KLXX/TARGET_KL05Z/device/TOOLCHAIN_IAR/MKL05Z4.icf

Lines 14 to 17 in 443e37b

    
           /*Heap 1/4 of ram and stack 1/8*/ 
        
           define symbol __ICFEDIT_size_cstack__ = 0x200; 
        
           define symbol __ICFEDIT_size_heap__   = 0x400; 
        
           /**** End of ICF editor section. ###ICF###*/

TARGET_NCS36510: main stack/isr stack size is set to 512 B. This board has 48 KB RAM.

mbed-os/targets/TARGET_ONSEMI/TARGET_NCS36510/device/TOOLCHAIN_IAR/NCS36510.icf

Lines 31 to 33 in 443e37b

    
           /*-Sizes-*/ 
        
           define symbol __ICFEDIT_size_cstack__ = 0x200; 
        
           define symbol __ICFEDIT_size_heap__   = 0x4000;

TARGET_LPC810: main stack/isr stack size is set to 256 B. This micro-controller has only 4 KB RAM.

mbed-os/targets/TARGET_NXP/TARGET_LPC81X/TARGET_LPC810/device/TOOLCHAIN_IAR/LPC810.icf

Lines 13 to 15 in 443e37b

    
           /*-Sizes-*/ 
        
           define symbol __ICFEDIT_size_cstack__   = 0x100; 
        
           define symbol __ICFEDIT_size_heap__     = 0x200;

I'm not sure if increasing main stack/isr stack size for boards which have only 4KB or 8KB RAM is possible.

@bulislaw I think we should consider changing the requirement. I still did not checked all targets, but it looks like I can not simply unify main stack/isr stack size to 1KB for all targets. At least some exceptions are needed.

bulislaw · 2018-06-21T15:53:44Z

Yeah that's fair, NRF is special case due to the SoftDevices. TARGET_LPC810 and TARGET_KL05Z is not supported in mbed 5. We may also make an exception for Cortex A platform, but we need to document it well (your comment above gives nice explanation).

mprse · 2018-06-26T11:23:30Z

@pan Can you check if current form is acceptable for NORDIC target?

Unify ISR stack size for targets which support MBED 5 (or MBED 2 and MBED 5). MBED 5 : 3 boards ---------------- TMPM3H6 TMPM46B TMPM066 MBED 2, 5 : 0 boards ---------------- MBED 2 : 0 boards (skipped) ----------------

Unify ISR stack size for targets which support MBED 5 (or MBED 2 and MBED 5). MBED 5 : 0 boards ---------------- MBED 2, 5 : 3 boards ---------------- WIZWIKI_W7500ECO WIZWIKI_W7500P WIZWIKI_W7500 MBED 2 : 0 boards (skipped) ----------------

Unify ISR stack size for targets which support MBED 5 (or MBED 2 and MBED 5).

mprse · 2018-08-28T07:19:31Z

Rebased.

The following Mbed 5 boards were processed: NUCLEO_F091RC NUCLEO_F070RB NUCLEO_F072RB NUCLEO_F103RB NUCLEO_F207ZG DISCO_F303VC NUCLEO_F303ZE NUCLEO_F303RE --- only boards marked with "-" of NUCLEO_F3 family were processed UBLOX_C030_U201 MTB_UBLOX_ODIN_W2 MTB_MXCHIP_EMW3166 MBED_CONNECT_ODIN UBLOX_C030_N211 UBLOX_EVK_ODIN_W2 USI_WM_BN_BM_22 UBLOX_C030_R410M NUCLEO_F411RE NUCLEO_F413ZH DISCO_F407VG - NUCLEO_F401RE NUCLEO_F429ZI B96B_F446VE DISCO_F413ZH NUCLEO_F439ZI DISCO_F429ZI - MTS_DRAGONFLY_F411RE NUCLEO_F446ZE DISCO_F469NI NUCLEO_F446RE NUCLEO_F410RB MTS_MDOT_F411RE WIO_3G STEVAL_3DP001V1 - MTB_MTS_DRAGONFLY NUCLEO_F412ZG

jeromecoutant · 2018-09-26T07:32:10Z

Hi

Maybe we can push a little bit this PR as more and more PR on the same topic comes ?
#8225 #8252 #7909 #7289 ...

Thx

jeromecoutant · 2018-09-26T07:34:03Z

Maybe should we implement proposition from #8252:

if (!isdefinedsymbol(MBED_APP_HEAP_SIZE)) {
define symbol size_heap = ;
} else {
define symbol size_heap = MBED_APP_HEAP_SIZE;
}

mprse · 2018-09-26T07:48:59Z

I will have to mainly review all files again when framework for configuring boot stack size will go in (PR #8039).

Set the ISR stack to be 1KB. ARMmbed#7238 Set the heap size to 3KB(2KB + overhead + spare) so that atleast 2KB free ram is available for testing. With dynamic heap size, explicit size is not required. IAR 7.8 supports static heap, hence the change is needed in IAR linker files.

cmonr · 2018-10-18T15:40:48Z

@mprse Is this still in progress? #8039 has since been merged

Set the ISR stack to be 1KB. #7238 Set the heap size to 3KB(2KB + overhead + spare) so that atleast 2KB free ram is available for testing. With dynamic heap size, explicit size is not required. IAR 7.8 supports static heap, hence the change is needed in IAR linker files.

0xc0170 · 2018-10-25T07:32:47Z

@mprse Let us know the status. We shall close it and reopen with an update? Or is there expecting completion anytime soon?

mprse · 2018-10-25T08:03:10Z

@mprse Let us know the status. We shall close it and reopen with an update? Or is there expecting completion anytime soon?

I'm planning to continue this one when I'm done with SPI testing. Since we have new Framework added by @c1728p9 (PR #8039) I will have to go through all linker scripts again, so I think we should close this one and I'll open new one instead later.

0xc0170 · 2018-10-25T09:11:41Z

Sounds good to me (reference this previous work if needed any history). I'll close this one

mprse force-pushed the stack_unification branch from dea671e to 9ccd700 Compare June 18, 2018 11:44

0xc0170 added the needs: review label Jun 18, 2018

0xc0170 requested review from a team June 18, 2018 11:49

cmonr added the release-version: 5.9.2 label Jun 18, 2018

bulislaw previously approved these changes Jun 19, 2018

View reviewed changes

SeppoTakalo previously approved these changes Jun 19, 2018

View reviewed changes

cmonr added needs: work and removed needs: review labels Jun 19, 2018

pan- reviewed Jun 20, 2018

View reviewed changes

pan- requested changes Jun 20, 2018

View reviewed changes

mprse dismissed stale reviews from SeppoTakalo and bulislaw via 93a141b June 22, 2018 09:16

mprse force-pushed the stack_unification branch from 9ccd700 to 93a141b Compare June 22, 2018 09:16

bulislaw mentioned this pull request Jun 22, 2018

On STM32F439xI IAR linker file decreased stack size and increased heap #7289

Merged

mprse force-pushed the stack_unification branch 3 times, most recently from d9eba95 to a119085 Compare June 26, 2018 11:21

mprse added 3 commits August 28, 2018 09:16

TARGET_TOSHIBA: ISR stack size unification

eb31883

Unify ISR stack size for targets which support MBED 5 (or MBED 2 and MBED 5). MBED 5 : 3 boards ---------------- TMPM3H6 TMPM46B TMPM066 MBED 2, 5 : 0 boards ---------------- MBED 2 : 0 boards (skipped) ----------------

TARGET_WIZNET: ISR stack size unification

da3d347

Unify ISR stack size for targets which support MBED 5 (or MBED 2 and MBED 5). MBED 5 : 0 boards ---------------- MBED 2, 5 : 3 boards ---------------- WIZWIKI_W7500ECO WIZWIKI_W7500P WIZWIKI_W7500 MBED 2 : 0 boards (skipped) ----------------

TARGET_Silicon_Labs: ISR stack size unification

22399bb

Unify ISR stack size for targets which support MBED 5 (or MBED 2 and MBED 5).

mprse force-pushed the stack_unification branch from 7cfa089 to 22399bb Compare August 28, 2018 07:18

0xc0170 mentioned this pull request Aug 29, 2018

Fix Wiznet: IAR heap memory problem #7921

Merged

c1728p9 mentioned this pull request Sep 7, 2018

Add framework for configuring boot stack size #8039

Merged

cmonr added the needs: preceding PR label Sep 7, 2018

kjbracey mentioned this pull request Sep 10, 2018

Adjust STMF411xE IAR linker file to mbed-os memory needs. #7909

Merged

0xc0170 mentioned this pull request Sep 24, 2018

Adjust stack & heap for IAR on STM32F412xG #8225

Merged

0xc0170 mentioned this pull request Oct 2, 2018

Add MTS dragonfly, MTS dragonfly l471 #7304

Merged

SeppoTakalo mentioned this pull request Oct 23, 2018

Fhss timer dev #8478

Merged

0xc0170 closed this Oct 25, 2018

0xc0170 removed the needs: work label Oct 25, 2018

cmonr removed needs: preceding PR release-version: 5.11.0-rc1 labels Oct 25, 2018

Stack unification and test #7238

Stack unification and test #7238

Uh oh!

Conversation

mprse commented Jun 18, 2018

Description

Pull request type

Uh oh!

bulislaw commented Jun 19, 2018

Uh oh!

SeppoTakalo left a comment

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo commented Jun 19, 2018

Uh oh!

SeppoTakalo commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mprse commented Jun 20, 2018

Uh oh!

deepikabhavnani commented Jun 20, 2018

Uh oh!

deepikabhavnani commented Jun 20, 2018

Uh oh!

SeppoTakalo commented Jun 20, 2018

Uh oh!

pan- Jun 20, 2018

Choose a reason for hiding this comment

Uh oh!

pan- Jun 20, 2018

Choose a reason for hiding this comment

Uh oh!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

deepikabhavnani commented Jun 20, 2018

Uh oh!

mprse commented Jun 21, 2018

Uh oh!

bulislaw commented Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mprse commented Jun 26, 2018

Uh oh!

mprse commented Aug 28, 2018

Uh oh!

jeromecoutant commented Sep 26, 2018

Uh oh!

jeromecoutant commented Sep 26, 2018

Uh oh!

mprse commented Sep 26, 2018

Uh oh!

cmonr commented Oct 18, 2018

Uh oh!

0xc0170 commented Oct 25, 2018

Uh oh!

mprse commented Oct 25, 2018

Uh oh!

0xc0170 commented Oct 25, 2018

Uh oh!

Uh oh!

SeppoTakalo commented Jun 19, 2018 •

edited

Loading

bulislaw commented Jun 21, 2018 •

edited

Loading