NVIC_EnableIRQ missing barrier?

[kjbracey]

I've been getting paranoid about compiler barriers, particularly as people have been looking at Link-Time Optimisation.

NVIC_DisableIRQ has the DSB/ISB sequence needed by the architecture, and they act as the necessary "acquire" compiler barrier to keep following code in line - don't access RAM until that IRQ disable has completed.

But NVIC_EnableIRQ has nothing - it's just a volatile memory access. If code does something like:

NVIC_DisableIRQ(devx);
// modify some RAM accessed by devx IRQ handler
NVIC_EnableIRQ(devx);

Nothing is ensuring that RAM access occurs before the IRQ is enabled. volatile doesn't say anything about ordering of non-volatile accesses. Eg gcc manual:

Accesses to non-volatile objects are not ordered with respect to volatile accesses. You cannot use a volatile object as a memory barrier to order a sequence of writes to non-volatile memory.

If NVIC_DisableIRQ acts as a barrier, it makes sense that NVIC_EnableIRQ should also have one, so that the pair can function as a "lock/unlock" like above.

So my suggestion is:

_STATIC_INLINE void __NVIC_EnableIRQ(IRQn_Type IRQn)
{
  if ((int32_t)(IRQn) >= 0)
  {
    __COMPILER_BARRIER();
    NVIC->ISER[(((uint32_t)IRQn) >> 5UL)] = (uint32_t)(1UL << (((uint32_t)IRQn) & 0x1FUL));
    __COMPILER_BARRIER(); // not sure if 2nd needed - better safe than sorry
  }
}

Would need a barrier macro in the compiler-specific stuff.

[kjbracey]

I note that __enable_irq() and __disable_irq() also act as barriers, via their assembler memory clobber, so this would be consistent with those.

[ReinhardKeil]

Thanks for your comments. Please keep in mind, barrier instructions are not free. They consume both memory and execution time.

For an interrupt enable, the barrier instruction is not a strict requirement, as a potential interrupt that is triggered by the code that follows will be served perhaps, just a few cycles later or earlier.

For an interrupt disable, it is however critical that the interrupt is for sure disabled before you executed further instructions.

Did you discover a real-world problem that we have missed? Do you have a code example that does not work due to the fact that there are not barrier instructions for NVIC_Enable?

[kjbracey]

We don't need a barrier instruction - just a compiler barrier to stop the compiler re-ordering code.

For an interrupt enable it is potentially critical that previous memory stores have occurred before the interrupts are enabled, eg if we're modifying RAM variables used by an IRQ handler.

 NVIC_DisableIRQ(devx);
// modify some RAM accessed by devx IRQ handler
NVIC_EnableIRQ(devx);

Compiler could reorder to:

NVIC_DisableIRQ(devx);
NVIC_EnableIRQ(devx);
// modify some RAM accessed by devx IRQ handler

I've not hit a real-world problem, but I'll have a go at demonstrating that it can occur in compiler explorer.

[kjbracey]

Not been able to provoke it for real yet, but here's a half-example for GCC: https://godbolt.org/z/iL_oMi

If ENABLE_BARRIER is undefined to remove the barrier, the two enables will drift up into the function body. (I've done two enables just to make them easier to see, and to confirm volatile is doing its job in all cases).

Now, the cheat there is to make it happen I've had to declare NVIC as an object, rather than its usual "pointer to 0xE000xxxx".

It appears that the pointer form of NVIC is suppressing optimisations enough to stop the reordering, but I'm not totally sure why - GCC's "strict aliasing" should make it realise that even if my normal variables could overlap with 0xE000xxxx, short accesses to the state can't alias with uint32_t writes to the NVIC.

[ReinhardKeil]

@kjbracey-arm thanks for all your explanations. The "potential" problem is now understood. We will investigate for a solution that should prevent from such optimisation.

Looks like we need a new compiler intrinsic for "optimization barrier"

[kjbracey]

It appears that the pointer form of NVIC is suppressing optimisations enough to stop the reordering

I also managed to provoke reordering with a restricted pointer, assuring the compiler of no NVIC vs RAM aliasing. So it's definitely only the compiler's instinctive fear of pointer aliasing saving us at the moment. . The volatile doesn't affect that, and I don't know why it's being so cautious by default.

Looks like we need a new compiler intrinsic for "optimization barrier"

Indeed. That is actually one of my motivations for raising this - you would need to add that to CMSIS, then I could use it myself outside CMSIS.

See https://github.com/ARMmbed/mbed-os/pull/9247/files for my version of a local barrier implementation for Mbed OS (MBED_COMPILER_BARRIER). If your guys think that's wrong, or if there's something finer-grained that can be done, let me know. I'd be deferring to CMSIS the moment you have an implementation.

I believe it's best to stick a barrier both before and after the volatile write, as in my example in the first comment. That's consistent with atomic-store-to-RAM implementation, and produces the same "totally lock in position" effect as if it we did asm volatile("STR %0,[NVIC_ISER]":::"memory"), making its ordering consistent with __enable_irq.

[GorgonMeducer]

I think this issue can also be solved by the discussion we have in another issue. #494