ARM64 system calls are entered with interrupts masked

[andyross]

I just submitted #35301, which will assert on the case where a thread with interrupts locked tries to context switch. This is an error, but it has traditionally worked in Zephyr and we haven't been able to detect it. Now we can.

But it broke in one spot in the test suite on qemu_cortex_a53_smp. And the symptom turned out to be that the ztest "1cpu" setup routine (which is a system call when USERSPACE=y) was coming out of the trap handler with masked interrupts. The function then creates a highest priority thread to "hold" a CPU, which I guess wants to preempt the current thread[1] and thus tries to context switch. Which hits the new warning.

It's a little suspicious that this got hit only in that one spot out of the whole test suite[2], so there may be more complexity here than I've diagnosed. But the symptom seems really clear. At the top of the system call I can do e.g. __ASSERT(arch_irq_unlocked(arch_irq_lock()), "") and watch it fail. Which is definitely 100% wrong. System calls need to operate with interrupts unmasked, otherwise the kernel becomes a giant latency trap.

[1] This might be something to look at too: on x86 and xtensa SMP, the IPI announcing that new thread to the other CPU almost always gets picked up synchronously before the current thread reaches the schedule point. It's extremely rare to find a high priority thread spawned that tries to preempt the current thread. Maybe IPIs aren't being delivered promptly?

[2] And in fact that use of 1cpu on the test case was needless anyway, so I removed it. We don't even need to work around this ARM64 issue right now.

[carlocaione]

I think the title is a bit misleading. I looked briefly at this issue and I don't think that the system calls are entered with interrupt masked but sometimes the IRQs are disable during a syscall by some other arch code (are you sure that the __ASSERT() you are hitting is at the top of the system call and not at the end?). I have a suspicion about what is causing that but nothing certain so far.

[npitre]

On ARM64 there is a peculiarity with the FPU support. Everything that ends
up invoking va_start() (i.e. all the test logging facility) does imply FP
access. va_start() will make a copy of all the FPU registers used to pass
FP arguments just in case.

Because the FP context is huge, we have save areas only for actual threads
in their normal state. When FP is involved in exception state (which
syscalls are) then the FPU access is trapped, the FP context is flushed to
the interrupted context and FPU access is granted to the exception context
but with IRQ disabled because at that point we have no storage to preserve that context if the exception code
itself were to be interrupted due to FPU access.

So if you don't do any floating point and you don't encounter a va_start()
in your syscalls (the case for most real life usage scenarios) then
interrupts will remain unmasked.

Now I'm thinking of a way to avoid this IRQ masking in the future, at least
for most of the va_start() cases where the FP arguments end up being all
zeroes anyway because the FPU access wasn't activated for anything else,
and no FP arguments are going to be consumed. Maybe emulating and skipping
over the corresponding str instructions could be done cheaply enough to
be worth it.

[andyross]

Ah.... OK, thanks. I think that mostly explains this, though my original diagnosis that the interrupts were wrong at syscall entry doesn't quite fit. But then it's not impossible I was just wrong.

This definitely defeats that assert I just added, though. But in this case the interrupt state doesn't represent a "lock" being broken, so as long as a z_swap() call doesn't lose FPU registers while in this state (it doesn't, right?) there's no reason to bother trying to bail. I think that's justification enough to just #if out ARM64 in that warning, and link here for the explanation.

As far as the interrupt trick, that definitely seems like something we should address as a performance bug, though. Being unable to interrupt kernel code in the general case is a pretty severe latency problem. But maybe that's an "enhancement" kind of thing and not a "bug".

[galak]

Closing this since it seems like from the discussion this isn't a bug. Please open an enhancement for the peformance improvement.

[npitre]

The proposed enhancement, and most likely the fix for the failing test mentioned here, is in PR #35410.