Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interrupt stack size unification + test #9092

Merged
merged 22 commits into from Jan 10, 2019

Conversation

Projects
None yet
@mprse
Copy link
Member

commented Dec 13, 2018

Description

This PR adds test which verifies sizes of: ISR stack, main thread stack, default user thread stack and updates linker scripts for ARM, GCC_ARM and IAR compilers to unify interrupt/boot stack size using framework provided in PR #8039.

Tested on the following boards:

K66F
K22F
EV_COG_AD4050LZ
NRF52_DK
NRF51_DK
NRF52840_DK
DISCO_L475VG_IOT01A
DISCO_F413ZH
NUCLEO_L476RG
NUCLEO_F207ZG
NUCLEO_F429ZI
LPC4088
FF_LPC546XX
EFM32GG_STK3700

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Dec 13, 2018

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

@mprse mprse force-pushed the mprse:stack_unification_sec_try branch 2 times, most recently Dec 13, 2018

@jeromecoutant

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2018

Hi
First quick test with L073:

target platform_name test suite result elapsed_time (sec) copy_method
NUCLEO_L073RZ-ARM NUCLEO_L073RZ tests-mbed_hal-stack_size_unification OK 18.91 default
NUCLEO_L073RZ-ARM NUCLEO_L073RZ tests-mbedmicro-rtos-mbed-heap_and_stack FAIL 19.96 default
NUCLEO_L073RZ-GCC_ARM NUCLEO_L073RZ tests-mbed_hal-stack_size_unification OK 21.21 default
NUCLEO_L073RZ-GCC_ARM NUCLEO_L073RZ tests-mbedmicro-rtos-mbed-heap_and_stack OK 22.09 default
NUCLEO_L073RZ-IAR NUCLEO_L073RZ tests-mbed_hal-stack_size_unification OK 18.84 default
NUCLEO_L073RZ-IAR NUCLEO_L073RZ tests-mbedmicro-rtos-mbed-heap_and_stack OK 19.62 default

Please check the failed test (OK in master branch)

@mprse

This comment has been minimized.

Copy link
Member Author

commented Dec 13, 2018

@jeromecoutant Thanks for checking this change. Unfortunately I don't have NUCLEO_L073RZ. Could you provide the test output?

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2018

@mprse - Will appreciate if you can split the PR into multiple based on targets (not necessary one per target but group of them). So many files have changed that even drop down button for files selection is not working, its extremely difficult to review.

@cmonr cmonr added the needs: work label Dec 13, 2018

@jeromecoutant

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

@mprse it seems that this test is FAIL for all the targets only with ARM

@mprse mprse force-pushed the mprse:stack_unification_sec_try branch Dec 14, 2018

@mprse

This comment has been minimized.

Copy link
Member Author

commented Dec 14, 2018

@mprse it seems that this test is FAIL for all the targets only with ARM

I pushed some fixes, but I was not able to test it well today. Tested only on NUCLEO_L476RG.

@mprse mprse force-pushed the mprse:stack_unification_sec_try branch 2 times, most recently Dec 17, 2018

@mprse

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2018

@mprse it seems that this test is FAIL for all the targets only with ARM

Few issues have been encountered while analysing the failing test. The first problem was that for the following targets isr/boot stack has been located in RW_IRAM2 instead RW_IRAM1:

targets/TARGET_STM/TARGET_STM32F4/TARGET_STM32F437xG/device/TOOLCHAIN_ARM_STD/stm32f437xx.sct
targets/TARGET_STM/TARGET_STM32F4/TARGET_STM32F439xI/device/TOOLCHAIN_ARM_STD/stm32f439xx.sct
targets/TARGET_STM/TARGET_STM32L4/TARGET_MTS_DRAGONFLY_L471QG/device/TOOLCHAIN_ARM_STD/stm32l471xx.sct
targets/TARGET_STM/TARGET_STM32L4/TARGET_STM32L443xC/device/TOOLCHAIN_ARM_STD/stm32l443xx.sct
targets/TARGET_STM/TARGET_STM32L4/TARGET_STM32L475xG/device/TOOLCHAIN_ARM_STD/stm32l475xx.sct
targets/TARGET_STM/TARGET_STM32L4/TARGET_STM32L486xG/device/TOOLCHAIN_ARM_STD/stm32l486xx.sct
targets/TARGET_STM/TARGET_STM32L4/TARGET_STM32L476xG/device/TOOLCHAIN_ARM_STD/stm32l476xx.sct

The problem has been fixed, but then I checked heap and stack locations and found another problem:

mbed_stack_isr_start: 0x20017C00
mbed_stack_isr_end:   0x20018000     <---- mbed_stack_isr_end == mbed_heap_end !!!
mbed_stack_isr_size:  0x400
mbed_heap_start:      0x200027F8
mbed_heap_end:        0x20018000     <---- mbed_stack_isr_end == mbed_heap_end !!!
mbed_heap_size:       0x15808

Isr/boot stack was located inside the heap (mbed_stack_isr_end == mbed_heap_end). Which is not good. It looks like the problem is in mbed_boot_arm_std.c file:

#if !defined(HEAP_START)
/* Defined by linker script */
extern uint32_t Image$$RW_IRAM1$$ZI$$Limit[];
#define HEAP_START ((unsigned char*)Image$$RW_IRAM1$$ZI$$Limit)
#define HEAP_SIZE ((uint32_t)((uint32_t)INITIAL_SP - (uint32_t)HEAP_START))
#endif
/*
* Note - Overriding the function __cpp_initialize__aeabi_ for the Arm
* standard library causes a crash when there are no global constructors.
* To avoid this do not override __cpp_initialize__aeabi_ with the super$$
* sub$$ mechanism for the Arm standard library. The uARM library is not
* effected by this problem.
*/
/*
* mbed entry point for the ARM standard toolchain
*
* Override the ARM standard library function __rt_entry to run code earlier in
* the boot sequence. This is after scatter loading has taken place but before
* the C library has been initialized.
*/
void __rt_entry(void)
{
unsigned char *free_start = HEAP_START;
uint32_t free_size = HEAP_SIZE;
#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
/* Heap - everything else */
mbed_heap_size = free_size;
mbed_heap_start = free_start;
mbed_init();
_platform_post_stackheap_init();
mbed_rtos_start();
}

This PR adds definition of ARM_LIB_STACK to all scatter files and based on ARM_LIB_STACK defines ISR_STACK_START and ISR_STACK_SIZE (40ff7f1).
This means that __rt_entry() function will use now explicitly specified interrupt stack (line 58 in above code snippet). It looks like the problem is now with heap definition. Heap is defined based on INITIAL_SP which points to the top of the RAM (line 34). Heap defined in this way actually defines heap+stack region - which is ok for case when ISR_STACK_START is not specified, because interrupt/boot stack will be reserved at the end of the free block. In case when ISR_STACK_START is specified heap definition should be changed to define only real heap size. Commit f40bb45 fixes the problem.

Final results:

mbed_stack_isr_start: 0x20017C00     <--- stack_isr_start == mbed_heap_end 
mbed_stack_isr_end:   0x20018000
mbed_stack_isr_size:  0x400
mbed_heap_start:      0x200027F8
mbed_heap_end:        0x20017C00     <--- stack_isr_start == mbed_heap_end 
mbed_heap_size:       0x15408

Now isr/boot stack is located after heap and isr/boot stack size is 1KB - as expected.

@c1728p9 Could you check if above modifications make sense?

@mprse

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2018

@mprse - Will appreciate if you can split the PR into multiple based on targets (not necessary one per target but group of them). So many files have changed that even drop down button for files selection is not working, its extremely difficult to review.

Can I leave one PR and create commit for each vendor?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Dec 17, 2018

Can I leave one PR and create commit for each vendor?

That should help and also simplify rebases as targets are changing constantly

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

Can I leave one PR and create commit for each vendor?

Commit per vendor will help with the review as well.
From my past experience with changing linker files, it was worth splitting the PR's between vendors. Avoided rebase few times and some got merged pretty fast. Your call to have separate commits or PR.

@c1728p9

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2018

Hi @mprse, your modifications make sense. I think you'll need to make the same modifications for GCC.

With this change will all targets be setting heap/stack info through their linker script? Once that is the case we should remove the configurable macro values HEAP_START,HEAP_SIZE,ISR_STACK_START,ISR_STACK_SIZE and use the linker symbols directly in the respective toolchain boot file - "mbed_boot_<toolchain>.c". That may be outside the scope of this PR though.

@mprse mprse force-pushed the mprse:stack_unification_sec_try branch to 1b931dc Dec 19, 2018

@0xc0170

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

CI started to get early errors. Still needs reviews

@mprse

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

Summary: 1 of 11 test jobs failed
Build number : 9
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

Seems that greentea-test has passed (no failures).

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

@ARMmbed/mbed-os-maintainers Any objections for this one landing soon?

@cmonr cmonr merged commit 2454b25 into ARMmbed:master Jan 10, 2019

21 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(-8 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9281 cycles (-763 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Jan 10, 2019

cmonr added a commit to cmonr/mbed-os that referenced this pull request Jan 11, 2019

Revert "Merge pull request ARMmbed#9092 from mprse/stack_unification_…
…sec_try"

This reverts commit 2454b25, reversing
changes made to 12980f4.

mprse added a commit to mprse/mbed-os that referenced this pull request Feb 1, 2019

Use 2-region memory model in ARM rtos-less builds.
The following commits: ARMmbed#8039, ARMmbed#9092 added Boot/ISR stack definition to all scatter files (ARM_LIB_STACK).

This has changed memory model for RTOS-less builds to 2-region memory model and caused failure in case of rtos less builds.
This PR defines valid heap/stack regions for rtos-less builds.

deepikabhavnani added a commit to deepikabhavnani/mbed-os that referenced this pull request Feb 11, 2019

Use 2-region memory model in ARM rtos-less builds.
The following commits: ARMmbed#8039, ARMmbed#9092 added Boot/ISR stack definition to all scatter files (ARM_LIB_STACK).

This has changed memory model for RTOS-less builds to 2-region memory model and caused failure in case of rtos less builds.
This PR defines valid heap/stack regions for rtos-less builds.

deepikabhavnani added a commit to mprse/mbed-os that referenced this pull request Feb 19, 2019

Use 2-region memory model in ARM rtos-less builds.
The following commits: ARMmbed#8039, ARMmbed#9092 added Boot/ISR stack definition to all scatter files (ARM_LIB_STACK).

This has changed memory model for RTOS-less builds to 2-region memory model and caused failure in case of rtos less builds.
This PR defines valid heap/stack regions for rtos-less builds.

orenc17 added a commit to kfnta/mbed-os that referenced this pull request Feb 20, 2019

Use 2-region memory model in ARM rtos-less builds.
The following commits: ARMmbed#8039, ARMmbed#9092 added Boot/ISR stack definition to all scatter files (ARM_LIB_STACK).

This has changed memory model for RTOS-less builds to 2-region memory model and caused failure in case of rtos less builds.
This PR defines valid heap/stack regions for rtos-less builds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.