You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/03/26 19:17:30 UTC
[GitHub] [incubator-nuttx] hartmannathan opened a new pull request #3198: Stm32h7 fix heap clobber
hartmannathan opened a new pull request #3198:
URL: https://github.com/apache/incubator-nuttx/pull/3198
## Summary
Normally, statically allocated data goes in .bss, followed by the initial stack, followed by HEAP. However, when data is statically allocated in SRAM4 with: `__attribute__ ((section (".sram4")))` it will clobber, and be clobbered by, the HEAP.
In one case, this manifested as a failure of NSH to start, because a statically allocated buffer for SPI6 BDMA, which has to be in SRAM4, clobbered the `/dev/console` inode structs, which the OS allocated earlier.
References:
- See the dev@nuttx.a.o mailing list discussion started 2021/03/25: "How to ensure HEAP will not overlap static DMA buffer?" [https://lists.apache.org/thread.html/recf2bb9043f8c9f53c10917e2adb2ec64fe35dc5e6f9a695a7ac6ecc%40%3Cdev.nuttx.apache.org%3E](https://lists.apache.org/thread.html/recf2bb9043f8c9f53c10917e2adb2ec64fe35dc5e6f9a695a7ac6ecc%40%3Cdev.nuttx.apache.org%3E)
- See `arm_addregion()` in `arch/arm/src/stm32h7/stm32_allocateheap.c`
This is a proposed stopgap fix, while discussion takes place to determine a more general fix for this class of problems.
This fix creates a "reserved" portion of SRAM4 which must meet both of these requirements:
- The .sram4_reserve section must be origined at SRAM4_BEGIN
- The portion of SRAM4 which is added to the heap must be in the range SRAM4_RESERVE_END to SRAM4_END.
Also, this commit adds a new Kconfig, `CONFIG_STM32H7_SRAM4EXCLUDE`, to allow optionally excluding the entire SRAM4 from the heap, similar to what the pre-existing Kconfig, `CONFIG_STM32H7_DTCMEXCLUDE`, does for the DTCM region.
**Caveats:** This requires every STM32H7 linker script to be modified to include:
```
.sram4_reserve :
{
_sram4_reserve_start = ABSOLUTE(.);
*(.sram4)
_sram4_reserve_end = ABSOLUTE(.);
} > sram4
```
Or link will fail with: `undefined reference to `_sram4_reserve_end'`. This PR updates all in-tree STM32H7 board linker scripts as required by the previous commit. The Release Notes should make a note of this because out-of-tree boards must make a similar change.
## Impact
Prevents serious problem where HEAP-allocated objects and statically-allocated objects placed in SRAM4 may clobber each other, leading to all sorts of undefined behavior. In particular, if configured to use SPI6 with DMA, the SPI6 BDMA Tx and Rx buffers will have this problem. There may be other such statically allocated buffers.
## Testing
Tested with custom board.
Also performed build testing and successfully built all of the following configurations:
- nucleo-h743zi:elf
- nucleo-h743zi:netnsh
- nucleo-h743zi:nsh
- nucleo-h743zi:nxlines_oled
- nucleo-h743zi:otg_fs_host
- nucleo-h743zi:pwm
- stm32h747i-disco:nsh
nxstyle
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #3198: Stm32h7 fix heap clobber
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #3198:
URL: https://github.com/apache/incubator-nuttx/pull/3198#discussion_r602714473
##########
File path: boards/arm/stm32h7/nucleo-h743zi/scripts/flash.ld
##########
@@ -172,9 +172,12 @@ SECTIONS
/* Emit the the D3 power domain section for locating BDMA data */
- .sram4 :
- {
- } > sram4
+ .sram4_reserve :
Review comment:
```suggestion
.sram4_reserve (NOLOAD) :
```
A OK Once enabled
38000800 A _sram4_reserve_end
38000000 A _sram4_reserve_start
38000400 00000400 d g_spi6_rxbuf
38000000 00000400 d g_spi6_txbuf
Except:
-rwxrwxr-x 1 david_s5 david_s5 805308416 Mar 27 05:34 nuttx.bin
Yikes!
With **NOLOAD**
108 -rwxrwxr-x 1 david_s5 david_s5 108708 Mar 27 05:39 nuttx.bin
24000074 00000048 d g_spi6dev
38000400 00000400 b g_spi6_rxbuf
38000000 00000400 b g_spi6_txbuf
0800207e 0000001a T stm32_spi6select
08002098 00000018 T stm32_spi6status
38000800 A _sram4_reserve_end
38000000 A _sram4_reserve_start
##########
File path: boards/arm/stm32h7/nucleo-h743zi/scripts/flash.ld
##########
@@ -172,9 +172,12 @@ SECTIONS
/* Emit the the D3 power domain section for locating BDMA data */
- .sram4 :
- {
- } > sram4
+ .sram4_reserve :
+ {
+ _sram4_reserve_start = ABSOLUTE(.);
Review comment:
I am wondering if we could use a name here that says what it is.
```
_sram4_heapexclude_start = ABSOLUTE(.);
*(.sram4)
_sram4_heapexclude_end = ABSOLUTE(.);
_sram4_heap_start = ABSOLUTE(.);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on a change in pull request #3198: Stm32h7 fix heap clobber
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on a change in pull request #3198:
URL: https://github.com/apache/incubator-nuttx/pull/3198#discussion_r603394267
##########
File path: boards/arm/stm32h7/nucleo-h743zi/scripts/flash.ld
##########
@@ -172,9 +172,12 @@ SECTIONS
/* Emit the the D3 power domain section for locating BDMA data */
- .sram4 :
- {
- } > sram4
+ .sram4_reserve :
+ {
+ _sram4_reserve_start = ABSOLUTE(.);
Review comment:
Agreed. Better naming makes it far more clear what this is for.
I think it also needs a `. = ALIGN(4)` in case of oddly sized data being placed here.
Right now I have it as this:
```
.sram4_reserve (NOLOAD) :
{
_sram4_heapexclude_start = ABSOLUTE(.);
*(.sram4)
. = ALIGN(4);
_sram4_heapexclude_end = ABSOLUTE(.);
_sram4_heap_start = ABSOLUTE(.);
} > sram4
```
But I think it can be improved/simplified further:
_sram4_heapexclude_start really serves no purpose other than documentation.
Also, because _sram4_heapexclude_end and _sram4_heap_start are the same address, we could remove _sram4_heapexclude_end and keep just _sram4_heap_start.
This will simplify the code changes in stm32_allocateheap.c as well (by removing a bunch of unnecessary symbols).
I'll make this change... let me know what you think. I can always change it back if we don't like it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis merged pull request #3198: Stm32h7 fix heap clobber
Posted by GitBox <gi...@apache.org>.
acassis merged pull request #3198:
URL: https://github.com/apache/incubator-nuttx/pull/3198
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on a change in pull request #3198: Stm32h7 fix heap clobber
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on a change in pull request #3198:
URL: https://github.com/apache/incubator-nuttx/pull/3198#discussion_r603389785
##########
File path: boards/arm/stm32h7/nucleo-h743zi/scripts/flash.ld
##########
@@ -172,9 +172,12 @@ SECTIONS
/* Emit the the D3 power domain section for locating BDMA data */
- .sram4 :
- {
- } > sram4
+ .sram4_reserve :
Review comment:
Ouch! Thanks for reviewing. I'm glad you caught that!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org