From 10030038e910237816b6a181deb69a88d15d6550 Mon Sep 17 00:00:00 2001 From: Timon Kruiper Date: Thu, 6 Apr 2023 12:01:18 +0200 Subject: [PATCH] Kernel/aarch64: Make sure no reordering of DAIF::read is possible We were crashing on the VERIFY_INTERRUPTS_DISABLED() in RecursiveSpinlock::unlock, which was caused by the compiler reordering instructions in `sys$get_root_session_id`. In this function, a SpinLock is locked and quickly unlocked again, and since the lock and unlock functions were inlined into `sys$get_root_session_id` and the DAIF::read was missing the `volatile` keyword, the compiler was free to reorder the reads from the DAIF register to the top of this function. This caused the CPU to read the interrupts state at the beginning of the function, and storing the result on the stack, which in turn caused the VERIFY_INTERRUPTS_DISABLED() assertion to fail. By adding the `volatile` modifier to the inline assembly, the compiler will not reorder the instructions. In aa40cef2b7, I mistakenly assumed that the crash was related to the initial interrupts state of the kernel threads, but it turns out that the missing `volatile` keyword was the actual problem. This commit also removes that code again. --- Kernel/Arch/aarch64/Registers.h | 4 ++-- Kernel/WorkQueue.cpp | 7 ------- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/Kernel/Arch/aarch64/Registers.h b/Kernel/Arch/aarch64/Registers.h index b84aa9363b..264c4ab177 100644 --- a/Kernel/Arch/aarch64/Registers.h +++ b/Kernel/Arch/aarch64/Registers.h @@ -1287,8 +1287,8 @@ struct DAIF { { DAIF daif; - asm("mrs %[value], daif" - : [value] "=r"(daif)); + asm volatile("mrs %[value], daif" + : [value] "=r"(daif)); return daif; } diff --git a/Kernel/WorkQueue.cpp b/Kernel/WorkQueue.cpp index dc99835c89..7c0492737b 100644 --- a/Kernel/WorkQueue.cpp +++ b/Kernel/WorkQueue.cpp @@ -28,13 +28,6 @@ UNMAP_AFTER_INIT WorkQueue::WorkQueue(StringView name) if (name_kstring.is_error()) TODO(); auto [_, thread] = Process::create_kernel_process(name_kstring.release_value(), [this] { -#if ARCH(AARCH64) - // FIXME: This function expects to be executed with interrupts disabled, however on - // aarch64 we spawn (kernel) threads with interrupts enabled, so we need to disable them. - // This code should be written in a way that it is able to be executed with interrupts enabled. - Processor::disable_interrupts(); -#endif - for (;;) { WorkItem* item; bool have_more;