diff --git a/Kernel/Arch/x86/Processor.h b/Kernel/Arch/x86/Processor.h index 307bda41e8..42ebcc82b8 100644 --- a/Kernel/Arch/x86/Processor.h +++ b/Kernel/Arch/x86/Processor.h @@ -339,17 +339,13 @@ public: write_gs_ptr(__builtin_offsetof(Processor, m_in_critical), critical); } - ALWAYS_INLINE static void enter_critical(u32& prev_flags) + ALWAYS_INLINE static void enter_critical() { - prev_flags = cpu_flags(); - cli(); - // NOTE: Up until this point we *could* have been preempted. - // Now interrupts are disabled, so calling current() is safe. - AK::atomic_fetch_add(¤t().m_in_critical, 1u, AK::MemoryOrder::memory_order_relaxed); + write_gs_ptr(__builtin_offsetof(Processor, m_in_critical), in_critical() + 1); } private: - ALWAYS_INLINE void do_leave_critical(u32 prev_flags) + ALWAYS_INLINE void do_leave_critical() { VERIFY(m_in_critical > 0); if (m_in_critical == 1) { @@ -363,52 +359,31 @@ private: } else { m_in_critical = m_in_critical - 1; } - if (prev_flags & 0x200) - sti(); - else - cli(); } public: - ALWAYS_INLINE static void leave_critical(u32 prev_flags) + ALWAYS_INLINE static void leave_critical() { - cli(); // Need to prevent IRQs from interrupting us here! - // NOTE: Up until this point we *could* have been preempted! - // Now interrupts are disabled, so calling current() is safe - current().do_leave_critical(prev_flags); + current().do_leave_critical(); } - ALWAYS_INLINE static u32 clear_critical(u32& prev_flags, bool enable_interrupts) + ALWAYS_INLINE static u32 clear_critical() { - cli(); - // NOTE: Up until this point we *could* have been preempted! - // Now interrupts are disabled, so calling current() is safe - // This doesn't have to be atomic, and it's also fine if we - // were to be preempted in between these steps (which should - // not happen due to the cli call), but if we moved to another - // processors m_in_critical would move along with us - auto prev_critical = read_gs_ptr(__builtin_offsetof(Processor, m_in_critical)); + auto prev_critical = in_critical(); write_gs_ptr(__builtin_offsetof(Processor, m_in_critical), 0); auto& proc = current(); if (!proc.m_in_irq) proc.check_invoke_scheduler(); - if (enable_interrupts || (prev_flags & 0x200)) - sti(); return prev_critical; } - ALWAYS_INLINE static void restore_critical(u32 prev_critical, u32 prev_flags) + ALWAYS_INLINE static void restore_critical(u32 prev_critical) { // NOTE: This doesn't have to be atomic, and it's also fine if we // get preempted in between these steps. If we move to another // processors m_in_critical will move along with us. And if we // are preempted, we would resume with the same flags. write_gs_ptr(__builtin_offsetof(Processor, m_in_critical), prev_critical); - VERIFY(!prev_critical || !(prev_flags & 0x200)); - if (prev_flags & 0x200) - sti(); - else - cli(); } ALWAYS_INLINE static u32 in_critical() diff --git a/Kernel/Arch/x86/ScopedCritical.h b/Kernel/Arch/x86/ScopedCritical.h index 66fcdc09bd..3c75490144 100644 --- a/Kernel/Arch/x86/ScopedCritical.h +++ b/Kernel/Arch/x86/ScopedCritical.h @@ -28,15 +28,13 @@ public: } ScopedCritical(ScopedCritical&& from) - : m_prev_flags(exchange(from.m_prev_flags, 0)) - , m_valid(exchange(from.m_valid, false)) + : m_valid(exchange(from.m_valid, false)) { } ScopedCritical& operator=(ScopedCritical&& from) { if (&from != this) { - m_prev_flags = exchange(from.m_prev_flags, 0); m_valid = exchange(from.m_valid, false); } return *this; @@ -46,18 +44,17 @@ public: { VERIFY(m_valid); m_valid = false; - Processor::leave_critical(m_prev_flags); + Processor::leave_critical(); } void enter() { VERIFY(!m_valid); m_valid = true; - Processor::enter_critical(m_prev_flags); + Processor::enter_critical(); } private: - u32 m_prev_flags { 0 }; bool m_valid { false }; }; diff --git a/Kernel/Arch/x86/common/Processor.cpp b/Kernel/Arch/x86/common/Processor.cpp index d829a5644c..8cc84f2314 100644 --- a/Kernel/Arch/x86/common/Processor.cpp +++ b/Kernel/Arch/x86/common/Processor.cpp @@ -658,9 +658,9 @@ void Processor::exit_trap(TrapFrame& trap) void Processor::check_invoke_scheduler() { + InterruptDisabler disabler; VERIFY(!m_in_irq); VERIFY(!m_in_critical); - VERIFY_INTERRUPTS_DISABLED(); VERIFY(&Processor::current() == this); if (m_invoke_scheduler_async && m_scheduler_initialized) { m_invoke_scheduler_async = false; @@ -732,7 +732,7 @@ ProcessorMessage& Processor::smp_get_from_pool() u32 Processor::smp_wake_n_idle_processors(u32 wake_count) { - VERIFY(Processor::in_critical()); + VERIFY_INTERRUPTS_DISABLED(); VERIFY(wake_count > 0); if (!s_smp_enabled) return 0; @@ -817,8 +817,7 @@ bool Processor::smp_process_pending_messages() VERIFY(s_smp_enabled); bool did_process = false; - u32 prev_flags; - enter_critical(prev_flags); + enter_critical(); if (auto pending_msgs = m_message_queue.exchange(nullptr, AK::MemoryOrder::memory_order_acq_rel)) { // We pulled the stack of pending messages in LIFO order, so we need to reverse the list first @@ -882,7 +881,7 @@ bool Processor::smp_process_pending_messages() halt_this(); } - leave_critical(prev_flags); + leave_critical(); return did_process; } diff --git a/Kernel/Locking/SpinLock.h b/Kernel/Locking/SpinLock.h index a312914b5f..0f83375f4c 100644 --- a/Kernel/Locking/SpinLock.h +++ b/Kernel/Locking/SpinLock.h @@ -23,8 +23,9 @@ public: ALWAYS_INLINE u32 lock() { - u32 prev_flags; - Processor::enter_critical(prev_flags); + u32 prev_flags = cpu_flags(); + Processor::enter_critical(); + cli(); while (m_lock.exchange(1, AK::memory_order_acquire) != 0) { Processor::wait_check(); } @@ -35,7 +36,11 @@ public: { VERIFY(is_locked()); m_lock.store(0, AK::memory_order_release); - Processor::leave_critical(prev_flags); + if (prev_flags & 0x200) + sti(); + else + cli(); + Processor::leave_critical(); } [[nodiscard]] ALWAYS_INLINE bool is_locked() const @@ -63,8 +68,9 @@ public: { auto& proc = Processor::current(); FlatPtr cpu = FlatPtr(&proc); - u32 prev_flags; - Processor::enter_critical(prev_flags); + u32 prev_flags = cpu_flags(); + Processor::enter_critical(); + cli(); FlatPtr expected = 0; while (!m_lock.compare_exchange_strong(expected, cpu, AK::memory_order_acq_rel)) { if (expected == cpu) @@ -82,7 +88,11 @@ public: VERIFY(m_lock.load(AK::memory_order_relaxed) == FlatPtr(&Processor::current())); if (--m_recursions == 0) m_lock.store(0, AK::memory_order_release); - Processor::leave_critical(prev_flags); + if (prev_flags & 0x200) + sti(); + else + cli(); + Processor::leave_critical(); } [[nodiscard]] ALWAYS_INLINE bool is_locked() const diff --git a/Kernel/Syscalls/execve.cpp b/Kernel/Syscalls/execve.cpp index b4b328fe9a..4b55c7f2e7 100644 --- a/Kernel/Syscalls/execve.cpp +++ b/Kernel/Syscalls/execve.cpp @@ -630,7 +630,9 @@ KResult Process::do_exec(NonnullRefPtr main_program_description // We enter a critical section here because we don't want to get interrupted between do_exec() // and Processor::assume_context() or the next context switch. // If we used an InterruptDisabler that sti()'d on exit, we might timer tick'd too soon in exec(). - Processor::current().enter_critical(prev_flags); + Processor::enter_critical(); + prev_flags = cpu_flags(); + cli(); // NOTE: Be careful to not trigger any page faults below! @@ -927,7 +929,9 @@ KResult Process::exec(String path, Vector arguments, Vector envi VERIFY_NOT_REACHED(); } - Processor::leave_critical(prev_flags); + if (prev_flags & 0x200) + sti(); + Processor::leave_critical(); return KSuccess; } diff --git a/Kernel/Thread.cpp b/Kernel/Thread.cpp index 71313e8774..4a72194090 100644 --- a/Kernel/Thread.cpp +++ b/Kernel/Thread.cpp @@ -388,8 +388,7 @@ void Thread::die_if_needed() // Now leave the critical section so that we can also trigger the // actual context switch - u32 prev_flags; - Processor::clear_critical(prev_flags, false); + Processor::clear_critical(); dbgln("die_if_needed returned from clear_critical!!! in irq: {}", Processor::current().in_irq()); // We should never get here, but the scoped scheduler lock // will be released by Scheduler::context_switch again @@ -420,10 +419,9 @@ void Thread::yield_assuming_not_holding_big_lock() // Disable interrupts here. This ensures we don't accidentally switch contexts twice InterruptDisabler disable; Scheduler::yield(); // flag a switch - u32 prev_flags; - u32 prev_crit = Processor::clear_critical(prev_flags, true); + u32 prev_critical = Processor::clear_critical(); // NOTE: We may be on a different CPU now! - Processor::restore_critical(prev_crit, prev_flags); + Processor::restore_critical(prev_critical); } void Thread::yield_and_release_relock_big_lock() @@ -451,13 +449,12 @@ void Thread::relock_process(LockMode previous_locked, u32 lock_count_to_restore) // flagged by calling Scheduler::yield above. // We have to do it this way because we intentionally // leave the critical section here to be able to switch contexts. - u32 prev_flags; - u32 prev_crit = Processor::clear_critical(prev_flags, true); + u32 prev_critical = Processor::clear_critical(); // CONTEXT SWITCH HAPPENS HERE! // NOTE: We may be on a different CPU now! - Processor::restore_critical(prev_crit, prev_flags); + Processor::restore_critical(prev_critical); if (previous_locked != LockMode::Unlocked) { // We've unblocked, relock the process if needed and carry on.