NutExitCritical() behaviour on ARM Cortex-M

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

NutExitCritical() behaviour on ARM Cortex-M

Philipp Burch-2
Hi everyone,

I usually only work with critical sections that disable a specific
interrupt known to access the resource to be protected. But now I have
some code which may be accessed from different interrupts, so I would
like to have a critical section that disables interrupts globally on an
ARM Cortex-M.

For those processors, NutEnterCritical() and NutExitCritical() are
defined in arch/cm3/atom.h as follows:

#define NutEnterCritical() \
{ \
    asm volatile (  \
        "@ NutEnterCritical"    "\n\t" \
        "mrs     r0, PRIMASK"   "\n\t" \
        "cpsid   i"             "\n\t" \
        :::"r0" \
    ); \
}

#define NutExitCritical() \
    {\
        asm volatile ( \
        "@ NutExitCritical"     "\n\t" \
        "mrs     r0, PRIMASK"   "\n\t" \
        "cpsie   i"             "\n\t" \
        :::"r0" \
    ); \
}

But I really can't figure out how this is supposed to work.
NutEnterCritical() seems more or less reasonable. It moves state into r0
and then disables the interrupts. Only: What happens to the stored state
in r0? As far as I understand it, the clobber just tells the compiler
that the code modifies the contents of r0 and it needs to restore them
afterwards, but not to actually save the contents in r0 to be used
afterwards.
Now, what about NutExitCritical()? This seems to again move state *into*
r0 and then enables the interrupts. But isn't it the idea to read the
stored state and put it into PRIMASK to restore the interrupt flag to
what it was before NutEnterCritical()?
Additionally, the code disabling single interrupts required me to insert
__DSB(); __ISB(); to avoid spurious interrupts to occur at the beginning
of the critical section. Maybe these should be included.
https://electronics.stackexchange.com/a/415138 also suggests to add
"memory" and "cc" clobbers to the inline assembly code.

Am I just missing something in the current implementation?

Thank you and best regards,
Philipp
_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion
Reply | Threaded
Open this post in threaded view
|

Re: NutExitCritical() behaviour on ARM Cortex-M

Uwe Bonnes
Philipp Burch writes:
> Hi everyone,
...
> Am I just missing something in the current implementation?
>
Hi Philipp,

probably you do not miss something. There has been some discussion
about that subject from time to time. There was no conclusive
solution.

How do other OSes (RTOS, mbed, etc) handle that situation?

What is the use case.

Some discussion also revolved around stacking critical section. I was
not convinced that such stacking is really needed.

Cheers
--
Uwe Bonnes                [hidden email]

Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
--------- Tel. 06151 1623569 ------- Fax. 06151 1623305 ---------
_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion
Reply | Threaded
Open this post in threaded view
|

Re: NutExitCritical() behaviour on ARM Cortex-M

Dušan-2
In reply to this post by Philipp Burch-2
Hi Philipp,

I think the first instruction only reads current priority mask to the
function return code register.
Then all ints are globally disabled.

In our Coldfire Nut/OS port, we have e.g. uart interrupts, which can be
interrupted by ethernet ones.
We used to save interrupt priority, because you do not know at which
int. priority level you are running.
https://github.com/dferbas/NutOs/blob/master/nut/include/arch/m68k/coldfire/atom_mcf5.h

I realized later, when the Nut community rejected this approach, that if
you run a normal code,
you should be at zero priority level. So there is no need to store int
priority, except in drivers.

For drivers I found following 2 examples:

You can check e.g. Zephyr RTOS: https://github.com/zephyrproject-rtos/zephyr
irq_disable ~ arch/arm/core/irq_manage.c:arch_irq_disable ->
-> ext/hal/cmsis/Core/Include/core_cm4.h:NVIC_DisableIRQ(), which uses
NVIC->ICER instructions to disable ints.
I think no cpsid instruction.

Similar with FreeRTOS - https://github.com/jameswalmsley/FreeRTOS
Source/portable/GCC/ARM_CM4F/port.c:vPortEnterCritical() ->
portDISABLE_INTERRUPTS
vPortRaiseBASEPRI - I think this is called from normal context
     New priority is stored into basepri register,
     isb, dsb are used to enable immediate execution of pending ints

ulPortRaiseBASEPRI - here priority is first saved into a register (for
further restoral)

---------
Current Nut/OS cm3 port does not use isb instruction.
Can anyone say if it is needed?
As per
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dai0321a/BIHBFEIB.html

*Dušan*

On 5.12.2019 6:21, Philipp Burch wrote:

> Hi everyone,
>
> I usually only work with critical sections that disable a specific
> interrupt known to access the resource to be protected. But now I have
> some code which may be accessed from different interrupts, so I would
> like to have a critical section that disables interrupts globally on an
> ARM Cortex-M.
>
> For those processors, NutEnterCritical() and NutExitCritical() are
> defined in arch/cm3/atom.h as follows:
>
> #define NutEnterCritical() \
> { \
>      asm volatile (  \
>          "@ NutEnterCritical"    "\n\t" \
>          "mrs     r0, PRIMASK"   "\n\t" \
>          "cpsid   i"             "\n\t" \
>          :::"r0" \
>      ); \
> }
>
> #define NutExitCritical() \
>      {\
>          asm volatile ( \
>          "@ NutExitCritical"     "\n\t" \
>          "mrs     r0, PRIMASK"   "\n\t" \
>          "cpsie   i"             "\n\t" \
>          :::"r0" \
>      ); \
> }
>
> But I really can't figure out how this is supposed to work.
> NutEnterCritical() seems more or less reasonable. It moves state into r0
> and then disables the interrupts. Only: What happens to the stored state
> in r0? As far as I understand it, the clobber just tells the compiler
> that the code modifies the contents of r0 and it needs to restore them
> afterwards, but not to actually save the contents in r0 to be used
> afterwards.
> Now, what about NutExitCritical()? This seems to again move state *into*
> r0 and then enables the interrupts. But isn't it the idea to read the
> stored state and put it into PRIMASK to restore the interrupt flag to
> what it was before NutEnterCritical()?
> Additionally, the code disabling single interrupts required me to insert
> __DSB(); __ISB(); to avoid spurious interrupts to occur at the beginning
> of the critical section. Maybe these should be included.
> https://electronics.stackexchange.com/a/415138 also suggests to add
> "memory" and "cc" clobbers to the inline assembly code.
>
> Am I just missing something in the current implementation?
>
> Thank you and best regards,
> Philipp
> _______________________________________________
> http://lists.egnite.de/mailman/listinfo/en-nut-discussion
>
_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion
Reply | Threaded
Open this post in threaded view
|

Re: NutExitCritical() behaviour on ARM Cortex-M

Uwe Bonnes
Dušan writes:
...
> I realized later, when the Nut community rejected this approach, that if
> you run a normal code,
> you should be at zero priority level. So there is no need to store int
> priority, except in drivers.

Probably the patch as such got not "rejected". Few people with commit
rights are still active, mostly I do commits. Perhaps I did not fully
understand the code or the need for that feature or it got lost with
other work. Most probably I feared collateral damage.

And as you tell
" that if you run a normal code, you should be at zero priority level.
  So there is no need to store int,except in drivers."

So make that a new API, like NutXxxxCriticalIrq(). That way we do not
mess up with NutEnterCritical() as used in the base ethernut code. That
code would only be arch specific with much less chance for collateral
damage. Philipp, get inspired by the other OSes. Aggree on an API with
Dusan and send patches!

> priority, except in drivers.

> For drivers I found following 2 examples:
>
> You can check e.g. Zephyr RTOS: https://github.com/zephyrproject-rtos/zephyr
> irq_disable ~ arch/arm/core/irq_manage.c:arch_irq_disable ->
> -> ext/hal/cmsis/Core/Include/core_cm4.h:NVIC_DisableIRQ(), which uses
> NVIC->ICER instructions to disable ints.
> I think no cpsid instruction.
>
> Similar with FreeRTOS - https://github.com/jameswalmsley/FreeRTOS
> Source/portable/GCC/ARM_CM4F/port.c:vPortEnterCritical() ->
> portDISABLE_INTERRUPTS
> vPortRaiseBASEPRI - I think this is called from normal context
>      New priority is stored into basepri register,
>      isb, dsb are used to enable immediate execution of pending ints
>
> ulPortRaiseBASEPRI - here priority is first saved into a register (for
> further restoral)
>
> ---------
> Current Nut/OS cm3 port does not use isb instruction.
> Can anyone say if it is needed?

SNV Head uses __DSB() in stm32f2_4_flash.c.

ISB would probably be needed in the CM3 NutXxxxCriticalIrq() implementation.

In the arch/cm3/ directory, I only find stm32_owitim.c using
a critical section in a interrupt routine. Here any interruption would confuse
timing and so  all interrupts are blocked, but this should probably
get NutXxxxCriticalIrq() when the API exists.

Cheers


--
Uwe Bonnes                [hidden email]

Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
--------- Tel. 06151 1623569 ------- Fax. 06151 1623305 ---------
_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion
Reply | Threaded
Open this post in threaded view
|

Re: NutExitCritical() behaviour on ARM Cortex-M

Philipp Burch-2
In reply to this post by Uwe Bonnes
Hi Uwe, hi Dušan!

On 05.12.19 10:46, [hidden email] wrote:

> Philipp Burch writes:
>> Hi everyone,
> ...
>> Am I just missing something in the current implementation?
>>
> Hi Philipp,
>
> probably you do not miss something. There has been some discussion
> about that subject from time to time. There was no conclusive
> solution.

It is mostly that placing of PRIMASK into r0 from *both* macros that I
don't understand. What is the point of placing some read data in a
register that is not used afterwards?

>
> How do other OSes (RTOS, mbed, etc) handle that situation?
>
> What is the use case.

From my understanding, the idea of a critical section is that it can be
nested safely, especially when used inside generic code (be it for
disabling a single interrupt or all interrupts). This is also what the
comment in sys/atom.h says:

 * NutExitCritical()
 * Finalize a critical section. Must be used only at the end of a
critical section
 * to restore the global interrupt flag to the state saved by
NutEnterCritical()

My use case leading me to digging into this was that I have some code
that accesses hardware resources which should be usable both from
interrupt context as well as from thread context. The current
implementation should not cause a problem in my case, but I got confused
by the code and the comment.

>
> Some discussion also revolved around stacking critical section. I was
> not convinced that such stacking is really needed.

I agree that nesting critical sections (or calling functions from a
critical section in general) may not be a very good idea in the first
place. But sometimes it may lead to somewhat cleaner code (when building
an API to access shared resources while offering different levels of
abstraction for example). Combined with the comment that actually says
something else than what the code does, this could indeed lead to subtle
and hard-to-debug bugs in the code.

I also agree that changing the behaviour of such a low-level function
may cause collateral damage. So it is certainly sensible to add a new
API instead of updating the present one; but the comment should be fixed
to avoid being misleading.

A new interface could be named something like
NutEnterCriticalSectionNestable(). In my opinion, it should return an
unsigned int containing the previous state of the interrupt enable. This
could then passed to NutExitCriticalSectionNestable() to restore the
state. A typical use case would then look like this:

unsigned int iestate = NutEnterCriticalSectionNestable();
/* Perform shared resource access. */
NutExitCriticalSectionNestable(iestate);

DSB() and ISB() could also be integrated for ARM code. What do you think?

The FreeRTOS way with the global nesting counter would be another option
without the need to supply a local variable. But personally, I'd prefer
the explicit version over integrated global state...

> I think the first instruction only reads current priority mask to the function return code register.
> Then all ints are globally disabled.

Correct me if I'm wrong, but with the current implementation, isn't the
value stored in r0 lost anyway? r0 may be the function return value
register, but the code is implemented as a macro, so the using C code
can't really access the stored value. Or can it? I'm not too familiar
with inline assembly...

Best regards,
Philipp
_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion
Reply | Threaded
Open this post in threaded view
|

Re: NutExitCritical() behaviour on ARM Cortex-M

Dušan-2
Hi Philipp,

when I wrote about "driver" code,
I meant everything, which works with a low level, peripheral part of
your application.

C compiler mostly uses r0 to pass return value to a calling code.
On avr it is r0, r1 for int as int is 16 bit there. On 32 bit cpus, I
think it is the r0.
You can check assembler listings, how it is compiled.
E.g. in Linux kernel or U-Boot, you can insert in a makefile cflags-y +=
-Wa,-ahlms=$(@:.o=.lst)
to generate listings, to see how compiler compiles to instructions.

So r0 was probably intended to define something like int
NutEnterCritical(void),
i.e. a function, which returns a value of interrupt priority mask,
which is modified with cpsid, cpsie instructions.
As per
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dai0321a/BIHBFEIB.html,
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0552a/CHDBIBGJ.html
this should return 1 for disabled ints and 0 for enabled ints.

---
I suggest to redefine NutEnterCritical to become an inline function.
This allows to stay with its definition in a header.
It will be nice to allow compiler to check arguments type for case, when
we will add an argument.

I think we can stay with current cm3 code at first with primask,
later we can remove the (I think) unneeded mrs r0,PRIMASK.
For levels, we need to operate with basemask.
https://github.com/jameswalmsley/FreeRTOS/blob/a7152a969b2b49fce50d759b3972f17bf3b18ed7/FreeRTOS/Source/portable/GCC/ARM_CM4F/portmacro.h

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0552a/CHDBIBGJ.html,
link to PRIMASK register explanation
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dai0321a/BIHBFEIB.html

Something like below.
I do not know if we need to know a prio mask level when we return from
protected part.
In this case, we can redefine NutExitProtectedLevel not to read basepri
and return a value.

I only wrote the code, I did not compile it,
so I apologize if there is a mistake.

Usage
     NutUseProtected();

NutEnterProtected(PRIO_MASK_LEVEL_UART);
...
NutExitProtected();

Definition

static inline void NutEnterCritical(void) {
     asm volatile (
         "@ NutEnterCritical"    "\n\t" \
         "mrs     r0, PRIMASK"   "\n\t" \
         "cpsid   i"             "\n\t" \
         :::"r0" \
     );
}

static inline NutExitCritical(void) {
     asm volatile (
         "@ NutExitCritical"     "\n\t" \
         "mrs     r0, PRIMASK"   "\n\t" \
         "cpsie   i"             "\n\t" \
         "isb"                   "\n\t" \
         "dsb"                   "\n\t" \
         :::"r0" \
     );
}

static inline uint32_t NutEnterProtectedLevel(uint32_t
requested_prio_mask) {
     uint32_t prev_prio_mask, new_prio_mask;
     asm volatile (
         "@ NutEnterCritical"    "\n\t" \
         "mrs     %0, basepri"   "\n\t" \
         "mov     %1, %2"        "\n\t" \
         "msr     basepri,%1"    "\n\t" \
         "isb"                   "\n\t" \
         "dsb"                   "\n\t" \
         :"=r" (prev_prio_mask), "=r" (new_prio_mask): "rm"
(requested_prio_mask) \
     );
     return prev_prio_mask;
}

#define NutExitProtectedLevel(mask) (void)NutEnterProtectedLevel(mask)

#define NutUseProtected() \
     uint32_t register _saved_prio_mask;

#define NutEnterProtected(mask) \
     _saved_prio_mask = NutEnterProtectedLevel(mask)

#define NutExitProtected() \
     NutExitProtectedLevel(_saved_prio_mask)


*Dušan*

On 5.12.2019 19:54, Philipp Burch wrote:

> Hi Uwe, hi Dušan!
>
> On 05.12.19 10:46, [hidden email] wrote:
>> Philipp Burch writes:
>>> Hi everyone,
>> ...
>>> Am I just missing something in the current implementation?
>>>
>> Hi Philipp,
>>
>> probably you do not miss something. There has been some discussion
>> about that subject from time to time. There was no conclusive
>> solution.
> It is mostly that placing of PRIMASK into r0 from *both* macros that I
> don't understand. What is the point of placing some read data in a
> register that is not used afterwards?
>
>> How do other OSes (RTOS, mbed, etc) handle that situation?
>>
>> What is the use case.
>  From my understanding, the idea of a critical section is that it can be
> nested safely, especially when used inside generic code (be it for
> disabling a single interrupt or all interrupts). This is also what the
> comment in sys/atom.h says:
>
>   * NutExitCritical()
>   * Finalize a critical section. Must be used only at the end of a
> critical section
>   * to restore the global interrupt flag to the state saved by
> NutEnterCritical()
>
> My use case leading me to digging into this was that I have some code
> that accesses hardware resources which should be usable both from
> interrupt context as well as from thread context. The current
> implementation should not cause a problem in my case, but I got confused
> by the code and the comment.
>
>> Some discussion also revolved around stacking critical section. I was
>> not convinced that such stacking is really needed.
> I agree that nesting critical sections (or calling functions from a
> critical section in general) may not be a very good idea in the first
> place. But sometimes it may lead to somewhat cleaner code (when building
> an API to access shared resources while offering different levels of
> abstraction for example). Combined with the comment that actually says
> something else than what the code does, this could indeed lead to subtle
> and hard-to-debug bugs in the code.
>
> I also agree that changing the behaviour of such a low-level function
> may cause collateral damage. So it is certainly sensible to add a new
> API instead of updating the present one; but the comment should be fixed
> to avoid being misleading.
>
> A new interface could be named something like
> NutEnterCriticalSectionNestable(). In my opinion, it should return an
> unsigned int containing the previous state of the interrupt enable. This
> could then passed to NutExitCriticalSectionNestable() to restore the
> state. A typical use case would then look like this:
>
> unsigned int iestate = NutEnterCriticalSectionNestable();
> /* Perform shared resource access. */
> NutExitCriticalSectionNestable(iestate);
>
> DSB() and ISB() could also be integrated for ARM code. What do you think?
>
> The FreeRTOS way with the global nesting counter would be another option
> without the need to supply a local variable. But personally, I'd prefer
> the explicit version over integrated global state...
>
>> I think the first instruction only reads current priority mask to the function return code register.
>> Then all ints are globally disabled.
> Correct me if I'm wrong, but with the current implementation, isn't the
> value stored in r0 lost anyway? r0 may be the function return value
> register, but the code is implemented as a macro, so the using C code
> can't really access the stored value. Or can it? I'm not too familiar
> with inline assembly...
>
> Best regards,
> Philipp
> _______________________________________________
> http://lists.egnite.de/mailman/listinfo/en-nut-discussion
_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion