From 29e15f275b7d13bf00a8c18c39d733f31bd07fc3 Mon Sep 17 00:00:00 2001
From: Eliza Weisman <eliza@elizas.website>
Date: Mon, 30 Dec 2024 09:38:08 -0800
Subject: [PATCH] feat(hal_x86_64): be less stupid about ISR tracing

Tracing in interrupt service routines can easily deadlock the kernel
(c.f. c4183db5e6dcf55b3474790d298edfd842204788). Let's be less dumb
about it.

This commit does the following:

- Factor out the code for forcefully unlocking the mutices around
  tracing outputs (VGA and serial) into a separate, unsafe function, and
  make the ISRs that are (currently) always a kernel oops just call that
- Remove the trace in the "spurious interrupt" handler, as that seems
  like a good way to deadlock the universe. In future, let's have a
  way to track whether spurious interrupts have fired that doesn't
  involve emitting a trace (maybe counters?)
---
 hal-x86_64/src/interrupt.rs | 61 ++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 22 deletions(-)

diff --git a/hal-x86_64/src/interrupt.rs b/hal-x86_64/src/interrupt.rs
index c4cba185..23350ed8 100644
--- a/hal-x86_64/src/interrupt.rs
+++ b/hal-x86_64/src/interrupt.rs
@@ -523,11 +523,10 @@ impl hal_core::interrupt::Control for Idt {
             code: u64,
         ) {
             unsafe {
-                // Safety: who cares!
-                crate::vga::writer().force_unlock();
-                if let Some(com1) = crate::serial::com1() {
-                    com1.force_unlock();
-                }
+                // Safety: An invalid TSS exception is always an oops. Since we're
+                // not coming back from this, it's okay to forcefully unlock the
+                // tracing outputs.
+                force_unlock_tracing();
             }
             let selector = SelectorErrorCode(code as u16);
             tracing::error!(?selector, "invalid task-state segment!");
@@ -548,11 +547,10 @@ impl hal_core::interrupt::Control for Idt {
             code: u64,
         ) {
             unsafe {
-                // Safety: who cares!
-                crate::vga::writer().force_unlock();
-                if let Some(com1) = crate::serial::com1() {
-                    com1.force_unlock();
-                }
+                // Safety: An segment not present exception is always an oops.
+                // Since we're not coming back from this, it's okay to
+                // forcefully unlock the tracing outputs.
+                force_unlock_tracing();
             }
             let selector = SelectorErrorCode(code as u16);
             tracing::error!(?selector, "a segment was not present!");
@@ -573,11 +571,10 @@ impl hal_core::interrupt::Control for Idt {
             code: u64,
         ) {
             unsafe {
-                // Safety: who cares!
-                crate::vga::writer().force_unlock();
-                if let Some(com1) = crate::serial::com1() {
-                    com1.force_unlock();
-                }
+                // Safety: An stack-segment fault exeption is always an oops.
+                // Since we're not coming back from this, it's okay to
+                // forcefully unlock the tracing outputs.
+                force_unlock_tracing();
             }
             let selector = SelectorErrorCode(code as u16);
             tracing::error!(?selector, "a stack-segment fault is happening");
@@ -598,12 +595,17 @@ impl hal_core::interrupt::Control for Idt {
             code: u64,
         ) {
             unsafe {
-                // Safety: who cares!
-
-                crate::vga::writer().force_unlock();
-                if let Some(com1) = crate::serial::com1() {
-                    com1.force_unlock();
-                }
+                // Safety: A general protection fault is (currently) always an
+                // oops. Since we're not coming back from this, it's okay to
+                // forcefully unlock the tracing outputs.
+                //
+                // TODO(eliza): in the future, if we allow the kernel to
+                // recover from general protection faults in user mode programs,
+                // rather than treating them as invariably fatal, we should
+                // probably not always do this. Instead, we should just handle
+                // the user-mode GPF non-fatally, and only unlock the tracing
+                // stuff if we know we're going to do a kernel oops...
+                force_unlock_tracing();
             }
 
             let segment = if code > 0 {
@@ -625,7 +627,7 @@ impl hal_core::interrupt::Control for Idt {
         }
 
         extern "x86-interrupt" fn spurious_isr() {
-            tracing::trace!("spurious");
+            // TODO(eliza): do we need to actually do something here?
         }
 
         // === exceptions ===
@@ -675,6 +677,21 @@ impl hal_core::interrupt::Control for Idt {
     }
 }
 
+/// Forcefully unlock the VGA port and COM1 serial port (used by tracing), so
+/// that an ISR can log stuff without deadlocking.
+///
+/// # Safety
+///
+/// This forcefully unlocks a mutex, which is probably bad to do. Only do this
+/// in ISRs that definitely represent real actual faults, and not just because
+/// "you wanted to log something".
+unsafe fn force_unlock_tracing() {
+    crate::vga::writer().force_unlock();
+    if let Some(com1) = crate::serial::com1() {
+        com1.force_unlock();
+    }
+}
+
 impl fmt::Debug for Registers {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         let Self {