You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mynewt.apache.org by Andrzej Kaczmarek <an...@codecoup.pl> on 2019/11/25 15:27:27 UTC
Cortex-M hardware stack limit checking
Hi,
I recently created a PR which adds support for hardware stack limit
checking on Cortex-M33. It was already approved by few people but
apparently there are different opinions of how this should be implemented
so I would like to get more opinions on this since we likely will have more
MCUs with this feature soon, so consider this as a voting.
PR: https://github.com/apache/mynewt-core/pull/2108
Here are proposed ways to do this:
1. use stack top and stack size to calculate PSPLIM on every context switch
this does not require any modification in os_task structure and adds 4
instructions on each context switch
2. add stack bottom to os_task
this extends os_task struct by 4 bytes and adds 2 instructions on each
context switch
3. change stack top to stack bottom in os_task
this does not change the size of os_task struct and adds 2 instructions on
each context switch, however it may break code which accesses os_task
structure directly (like mcumgr does - it should be modified to use proper
API anyway)
My personal preference is option 3 since it is more useful to have stack
bottom in os_task struct instead of stack top. For example, stack overflow
or usage is determined using stack bottom. Also when debugging it's easier
to inspect stack by using stack bottom rather than stack top. In general,
seems like you only need stack top when initializing it but this is done
only once (even os_task_init has 'stack_bottom' parameter, not 'stack_top').
Best,
Andrzej
Re: Cortex-M hardware stack limit checking
Posted by Andrzej Kaczmarek <an...@codecoup.pl>.
FYI: patch which replaces stack top with stack bottom and related changes
are already merged
Best,
Andrzej
On Wed, Nov 27, 2019 at 10:14 AM Marko Kiiskila <ma...@juul.com>
wrote:
> Option 3 sounds best to me as well.
>
> > On 26 Nov 2019, at 18.36, Łukasz Rymanowski <
> lukasz.rymanowski@codecoup.pl> wrote:
> >
> > option 3 looks good to me: +1
> >
> > Best
> > Łukasz
> >
> > On Mon, 25 Nov 2019 at 21:11, Jerzy Kasenberg
> > <je...@codecoup.pl> wrote:
> >>
> >> HI,
> >>
> >> I vote for option 3
> >>
> >> My reason, apart what Andrzej mentioned (usefulness and debugging),
> >> is that there is a notorious coverity warning popping up about usage of
> >> address &t_tasktop[stack_size].
> >>
> >> best regards
> >> Jerzy
> >>
> >> pon., 25 lis 2019 o 16:28 Andrzej Kaczmarek <
> andrzej.kaczmarek@codecoup.pl>
> >> napisał(a):
> >>
> >>> Hi,
> >>>
> >>> I recently created a PR which adds support for hardware stack limit
> >>> checking on Cortex-M33. It was already approved by few people but
> >>> apparently there are different opinions of how this should be
> implemented
> >>> so I would like to get more opinions on this since we likely will have
> more
> >>> MCUs with this feature soon, so consider this as a voting.
> >>>
> >>> PR: https://github.com/apache/mynewt-core/pull/2108
> >>>
> >>> Here are proposed ways to do this:
> >>> 1. use stack top and stack size to calculate PSPLIM on every context
> switch
> >>> this does not require any modification in os_task structure and adds 4
> >>> instructions on each context switch
> >>> 2. add stack bottom to os_task
> >>> this extends os_task struct by 4 bytes and adds 2 instructions on each
> >>> context switch
> >>> 3. change stack top to stack bottom in os_task
> >>> this does not change the size of os_task struct and adds 2
> instructions on
> >>> each context switch, however it may break code which accesses os_task
> >>> structure directly (like mcumgr does - it should be modified to use
> proper
> >>> API anyway)
> >>>
> >>> My personal preference is option 3 since it is more useful to have
> stack
> >>> bottom in os_task struct instead of stack top. For example, stack
> overflow
> >>> or usage is determined using stack bottom. Also when debugging it's
> easier
> >>> to inspect stack by using stack bottom rather than stack top. In
> general,
> >>> seems like you only need stack top when initializing it but this is
> done
> >>> only once (even os_task_init has 'stack_bottom' parameter, not
> >>> 'stack_top').
> >>>
> >>> Best,
> >>> Andrzej
> >>>
>
>
Re: Cortex-M hardware stack limit checking
Posted by Marko Kiiskila <ma...@juul.com>.
Option 3 sounds best to me as well.
> On 26 Nov 2019, at 18.36, Łukasz Rymanowski <lu...@codecoup.pl> wrote:
>
> option 3 looks good to me: +1
>
> Best
> Łukasz
>
> On Mon, 25 Nov 2019 at 21:11, Jerzy Kasenberg
> <je...@codecoup.pl> wrote:
>>
>> HI,
>>
>> I vote for option 3
>>
>> My reason, apart what Andrzej mentioned (usefulness and debugging),
>> is that there is a notorious coverity warning popping up about usage of
>> address &t_tasktop[stack_size].
>>
>> best regards
>> Jerzy
>>
>> pon., 25 lis 2019 o 16:28 Andrzej Kaczmarek <an...@codecoup.pl>
>> napisał(a):
>>
>>> Hi,
>>>
>>> I recently created a PR which adds support for hardware stack limit
>>> checking on Cortex-M33. It was already approved by few people but
>>> apparently there are different opinions of how this should be implemented
>>> so I would like to get more opinions on this since we likely will have more
>>> MCUs with this feature soon, so consider this as a voting.
>>>
>>> PR: https://github.com/apache/mynewt-core/pull/2108
>>>
>>> Here are proposed ways to do this:
>>> 1. use stack top and stack size to calculate PSPLIM on every context switch
>>> this does not require any modification in os_task structure and adds 4
>>> instructions on each context switch
>>> 2. add stack bottom to os_task
>>> this extends os_task struct by 4 bytes and adds 2 instructions on each
>>> context switch
>>> 3. change stack top to stack bottom in os_task
>>> this does not change the size of os_task struct and adds 2 instructions on
>>> each context switch, however it may break code which accesses os_task
>>> structure directly (like mcumgr does - it should be modified to use proper
>>> API anyway)
>>>
>>> My personal preference is option 3 since it is more useful to have stack
>>> bottom in os_task struct instead of stack top. For example, stack overflow
>>> or usage is determined using stack bottom. Also when debugging it's easier
>>> to inspect stack by using stack bottom rather than stack top. In general,
>>> seems like you only need stack top when initializing it but this is done
>>> only once (even os_task_init has 'stack_bottom' parameter, not
>>> 'stack_top').
>>>
>>> Best,
>>> Andrzej
>>>
Re: Cortex-M hardware stack limit checking
Posted by Łukasz Rymanowski <lu...@codecoup.pl>.
option 3 looks good to me: +1
Best
Łukasz
On Mon, 25 Nov 2019 at 21:11, Jerzy Kasenberg
<je...@codecoup.pl> wrote:
>
> HI,
>
> I vote for option 3
>
> My reason, apart what Andrzej mentioned (usefulness and debugging),
> is that there is a notorious coverity warning popping up about usage of
> address &t_tasktop[stack_size].
>
> best regards
> Jerzy
>
> pon., 25 lis 2019 o 16:28 Andrzej Kaczmarek <an...@codecoup.pl>
> napisał(a):
>
> > Hi,
> >
> > I recently created a PR which adds support for hardware stack limit
> > checking on Cortex-M33. It was already approved by few people but
> > apparently there are different opinions of how this should be implemented
> > so I would like to get more opinions on this since we likely will have more
> > MCUs with this feature soon, so consider this as a voting.
> >
> > PR: https://github.com/apache/mynewt-core/pull/2108
> >
> > Here are proposed ways to do this:
> > 1. use stack top and stack size to calculate PSPLIM on every context switch
> > this does not require any modification in os_task structure and adds 4
> > instructions on each context switch
> > 2. add stack bottom to os_task
> > this extends os_task struct by 4 bytes and adds 2 instructions on each
> > context switch
> > 3. change stack top to stack bottom in os_task
> > this does not change the size of os_task struct and adds 2 instructions on
> > each context switch, however it may break code which accesses os_task
> > structure directly (like mcumgr does - it should be modified to use proper
> > API anyway)
> >
> > My personal preference is option 3 since it is more useful to have stack
> > bottom in os_task struct instead of stack top. For example, stack overflow
> > or usage is determined using stack bottom. Also when debugging it's easier
> > to inspect stack by using stack bottom rather than stack top. In general,
> > seems like you only need stack top when initializing it but this is done
> > only once (even os_task_init has 'stack_bottom' parameter, not
> > 'stack_top').
> >
> > Best,
> > Andrzej
> >
Re: Cortex-M hardware stack limit checking
Posted by Jerzy Kasenberg <je...@codecoup.pl>.
HI,
I vote for option 3
My reason, apart what Andrzej mentioned (usefulness and debugging),
is that there is a notorious coverity warning popping up about usage of
address &t_tasktop[stack_size].
best regards
Jerzy
pon., 25 lis 2019 o 16:28 Andrzej Kaczmarek <an...@codecoup.pl>
napisał(a):
> Hi,
>
> I recently created a PR which adds support for hardware stack limit
> checking on Cortex-M33. It was already approved by few people but
> apparently there are different opinions of how this should be implemented
> so I would like to get more opinions on this since we likely will have more
> MCUs with this feature soon, so consider this as a voting.
>
> PR: https://github.com/apache/mynewt-core/pull/2108
>
> Here are proposed ways to do this:
> 1. use stack top and stack size to calculate PSPLIM on every context switch
> this does not require any modification in os_task structure and adds 4
> instructions on each context switch
> 2. add stack bottom to os_task
> this extends os_task struct by 4 bytes and adds 2 instructions on each
> context switch
> 3. change stack top to stack bottom in os_task
> this does not change the size of os_task struct and adds 2 instructions on
> each context switch, however it may break code which accesses os_task
> structure directly (like mcumgr does - it should be modified to use proper
> API anyway)
>
> My personal preference is option 3 since it is more useful to have stack
> bottom in os_task struct instead of stack top. For example, stack overflow
> or usage is determined using stack bottom. Also when debugging it's easier
> to inspect stack by using stack bottom rather than stack top. In general,
> seems like you only need stack top when initializing it but this is done
> only once (even os_task_init has 'stack_bottom' parameter, not
> 'stack_top').
>
> Best,
> Andrzej
>