Atomic sections via masking global interrupts

PX4 Autopilot System Architecture 🏗
1

Hi,

Apologies if this topic has already been discussed, I could not find any references touching this subject.

I’m in the process of implementing kernel-/userspace separation, known in NuttX as protected build. This has already been done for fmu-v5 but I’m doing it for a different platform and architecture altogether (RISC-V).

After fixing some minor/trivial build issues, I encountered one major architectural issue, which is the usage of enter_critical_section() / leave_critical_section() from unprivileged mode, which is a big no-no. On RISC-V this causes a full system crash due to illegal instruction (trying to run Machine ISA from userspace). On ARM the macros “sort of” work, because the privileged mode instruction which controls the processor state e.g. global interrupt enable is silently ignored.

According to the ARM documentation:
Use CPS only from privileged software. It has no effect if used in unprivileged software.
I used the term “sort of”, because interrupts are not masked in places where this is expected, so it can cause nasty race / sync issues which will be impossible to debug without knowing the quoted line from the ARM documentation.

But back to my issue and my solution proposal. I see that e.g. px4::atomic<> uses critical sections with NuttX and the usage of px4::atomic<> is ubiquitous. This means (IMO) the patch has to be made either on px4::atomic or preferably on (px4_)enter-/leave_critical_section. My suggestion is to create a new class “px4::critical” or something, that would act as a common interface for atomic sections in user- and kernelspace. In userspace a mutex/semaphore lock would be used, while in kernelspace the implementation can be inlined to call enter-/leave_critical_section() directly. Two separate source modules would be compiled & archived and the correct one must be selected for user-/kernelspace.

So why did I make this post about it ? My understanding of the px4 architecture (even the basic modules to be honest) is quite limited and I would first like to understand a few things:

  • Is px4::atomic used from ISR context ? If not is it expected to be ISR compatible ?
  • Does my solution make any sense whatsoever ?
  • Have you guys with more experience already considered how this could be fixed ?

Any comments and help on this will be greatly appreciated!

2

Hi
Thanks for the detailed write-up.

Adding px4::critical as a generic abstraction makes sense.

px4::atomic is used from IRQ handlers. The only case where enter_critical_section is needed (as it’s not lock-free), is for drivers setting a 64bit timestamp from an IRQ handler. So it should be enough to change the defines to:

#if defined(__PX4_NUTTX) && (defined (__KERNEL__) || defined(CONFIG_BUILD_FLAT))

Then in user-space, GCC will automatically add the locking for non-lock-free calls to __atomic_* (we could still handle that explicitly if we want to).

3

Thanks for the answer, I did not notice I got a reply here until now. The fix for using px4::atomic from user space can be found here: nuttx/atomic: Don't call enter-/exit_critical_section() from user code by pussuw · Pull Request #20170 · PX4/PX4-Autopilot · GitHub

4

I made a solution for this generic px4::critical abstraction as well, but the issue with that is that it ended up using almost 6K of flash (via .data section) and as many targets are already very short on available memory, I decided to do the flagging approach instead.

Let’s just hope the KERNEL flag is set correctly for kernel modules, or more critically, it is never set for user space modules. At least the patch 20170 works for the RISC-V target I’m working on!