track down and fix spinlocks being reentered within an interrupt causing an infinite spin wait

Author: braindigitalis

include/apic.h

@@ -249,4 +249,35 @@ void wake_cpu(uint8_t logical_cpu_id);
  * @see APIC_SVR
  * @see APIC_TPR
  */
-void apic_setup_ap();
+void apic_setup_ap();
+
+/**
+ * @brief Serialises all pending CPU operations.
+ *
+ * This function enforces a strict ordering of all memory, I/O, and MSR
+ * operations issued by the CPU prior to the call. It ensures that all writes
+ * and reads complete and are globally visible before any instructions issued
+ * after the call are allowed to execute.
+ *
+ * Implementation details:
+ * - `mfence` is used to flush and order all pending memory operations,
+ *   including MMIO writes to devices such as the LAPIC or IOAPIC.
+ * - `cpuid` is a serialising instruction which drains the CPU pipeline,
+ *   guaranteeing that no later instructions are executed until all earlier
+ *   ones have fully completed. It also serves as a convenient barrier in
+ *   emulators such as QEMU, which may otherwise defer committing MMIO state.
+ *
+ * This is particularly important when programming interrupt controllers
+ * (APIC, IOAPIC, PIC), as their state machines may lose or misroute
+ * interrupts if configuration writes have not yet taken effect before
+ * enabling interrupts with `sti`.
+ *
+ * @note This function clobbers the general-purpose registers EAX, EBX, ECX,
+ *       and EDX as required by the `cpuid` instruction. It does not return
+ *       any values.
+ */
+static inline void cpu_serialise(void)
+{
+	__asm__ volatile("mfence; cpuid"
+		::: "eax", "ebx", "ecx", "edx");
+}

include/input.h

@@ -1,5 +1,5 @@
 /**
- * @file kinput.h
+ * @file input.h
  * @author Craig Edwards ([email protected])
  * @brief Console line input API for Retro Rocket.
  *

include/spinlock.h

@@ -16,9 +16,11 @@
 
 #pragma once
 
+#include <io.h>
 #include <stdint.h>
 #include <stdbool.h>
 
+
 /**
  * @typedef spinlock_t
  * @brief Opaque 32-bit integer used as a spinlock state.
@@ -76,3 +78,96 @@ static inline void unlock_spinlock(spinlock_t* lock) {
 static inline bool try_lock_spinlock(spinlock_t* lock) {
 	return !__atomic_test_and_set(lock, __ATOMIC_ACQUIRE);
 }
+
+/**
+ * @brief Read the current RFLAGS register.
+ *
+ * Captures the full 64-bit RFLAGS value from the CPU.
+ *
+ * @return The current value of the RFLAGS register.
+ */
+static inline uint64_t read_rflags(void) {
+	uint64_t flags;
+	__asm__ volatile("pushfq; pop %0" : "=r"(flags));
+	return flags;
+}
+
+/**
+ * @brief Write a value into the RFLAGS register.
+ *
+ * Restores the CPU's RFLAGS register to the specified value.
+ * Typically used to restore saved interrupt flag state.
+ *
+ * @param flags The 64-bit value to load into RFLAGS.
+ */
+static inline void write_rflags(uint64_t flags) {
+	__asm__ volatile("push %0; popfq" :: "r"(flags) : "memory", "cc");
+}
+
+/**
+ * @brief Query whether interrupts are currently enabled.
+ *
+ * Uses the IF (Interrupt Flag) in the RFLAGS register to determine
+ * if maskable interrupts are permitted.
+ *
+ * @return true if interrupts are enabled (IF = 1), false otherwise.
+ */
+static inline bool are_interrupts_enabled(void) {
+	return (read_rflags() & (1ULL << 9)) != 0;
+}
+
+/**
+ * @brief Attempt to acquire a spinlock once (non-blocking).
+ *
+ * Performs an atomic compare-and-swap on the lock.
+ *
+ * @param lock Pointer to the spinlock variable.
+ * @return true if the lock was acquired, false otherwise.
+ */
+static inline bool try_lock(spinlock_t *lock) {
+	uint32_t expected = 0;
+	return __atomic_compare_exchange_n(lock, &expected, 1, false, __ATOMIC_ACQUIRE, __ATOMIC_RELAXED);
+}
+
+/**
+ * @brief Busy-wait until a spinlock is acquired.
+ *
+ * Continuously calls try_lock() until success.
+ * Uses the `pause` instruction to reduce contention on hyper-threaded CPUs.
+ *
+ * @param lock Pointer to the spinlock variable.
+ */
+static inline void spin_until_locked(spinlock_t *lock) {
+	while (!try_lock(lock)) {
+		__asm__ volatile("pause"); // reduce contention
+	}
+}
+
+/**
+ * @brief Acquire a spinlock while disabling interrupts.
+ *
+ * Saves the current interrupt flag (IF) state into @p flags,
+ * disables interrupts, then spins until the lock is acquired.
+ *
+ * @param lock  Pointer to the spinlock variable.
+ * @param flags Pointer to a uint64_t used to store the saved RFLAGS register.
+ */
+static inline void lock_spinlock_irq(spinlock_t *lock, uint64_t *flags) {
+	*flags = read_rflags();   // save current IF
+	interrupts_off();         // mask interrupts
+	spin_until_locked(lock);  // acquire
+}
+
+/**
+ * @brief Release a spinlock and restore interrupts.
+ *
+ * Releases the given spinlock and restores the CPU interrupt flag (IF)
+ * from the previously saved @p flags value.
+ *
+ * @param lock  Pointer to the spinlock variable.
+ * @param flags RFLAGS value saved by lock_spinlock_irq().
+ */
+static inline void unlock_spinlock_irq(spinlock_t *lock, uint64_t flags) {
+	__atomic_store_n(lock, 0, __ATOMIC_RELEASE); // unlock
+	write_rflags(flags);                         // restore IF exactly as before
+}

src/basic/console.c

@@ -125,11 +125,12 @@ void print_statement(struct basic_ctx* ctx)
 	accept_or_return(PRINT, ctx);
 	const char* out = printable_syntax(ctx);
 	if (out) {
-		lock_spinlock(&console_spinlock);
+		uint64_t flags;
+		lock_spinlock_irq(&console_spinlock, &flags);
 		lock_spinlock(&debug_console_spinlock);
 		putstring((console*)ctx->cons, out);
-		unlock_spinlock(&console_spinlock);
 		unlock_spinlock(&debug_console_spinlock);
+		unlock_spinlock_irq(&console_spinlock, flags);
 	}
 }
 

src/e1000.c

@@ -176,7 +176,7 @@ void e1000_transmit_init() {
 void e1000_handle_receive() {
 
 	if (!rx_descs[rx_cur]) {
-		kprintf("No rx_desc[rx_cur]\n");
+		dprintf("No rx_desc[rx_cur]\n");
 		return;
 	}
 	netdev_t* dev = get_active_network_device();
@@ -188,7 +188,7 @@ void e1000_handle_receive() {
 		uint8_t *buf = (uint8_t *) rx_descs[rx_cur]->addr;
 		uint16_t len = rx_descs[rx_cur]->length;
 		if (!buf) {
-			kprintf("No buf\n");
+			dprintf("No buf\n");
 			return;
 		}
 

src/hashmap.c

@@ -94,7 +94,7 @@ struct hashmap *hashmap_new_with_allocator(
 	}
 	// hashmap + spare + edata
 	size_t size = sizeof(struct hashmap)+bucketsz*2;
-	struct hashmap *map = _malloc(size);
+	struct hashmap *map = kmalloc(size);
 	if (!map) {
 		return NULL;
 	}
@@ -120,10 +120,10 @@ struct hashmap *hashmap_new_with_allocator(
 	memset(map->buckets, 0, map->bucketsz*map->nbuckets);
 	map->growat = map->nbuckets*0.75;
 	map->shrinkat = map->nbuckets*0.10;
-    	map->malloc = _malloc;
-	map->realloc = _realloc;
-	map->free = _free;
-	return map;  
+    	map->malloc = kmalloc;
+	map->realloc = krealloc;
+	map->free = kfree;
+	return map;
 }
 
 
@@ -144,14 +144,7 @@ struct hashmap *hashmap_new_with_allocator(
 // The hashmap must be freed with hashmap_free(). 
 // Param `elfree` is a function that frees a specific item. This should be NULL
 // unless you're storing some kind of reference data in the hash.
-struct hashmap *hashmap_new(size_t elsize, size_t cap, 
-							uint64_t seed0, uint64_t seed1,
-							uint64_t (*hash)(const void *item, 
-											 uint64_t seed0, uint64_t seed1),
-							int (*compare)(const void *a, const void *b, 
-										   void *udata),
-							void (*elfree)(const void *item),
-							void *udata)
+struct hashmap *hashmap_new(size_t elsize, size_t cap, uint64_t seed0, uint64_t seed1, uint64_t (*hash)(const void *item, uint64_t seed0, uint64_t seed1), int (*compare)(const void *a, const void *b, void *udata), void (*elfree)(const void *item), void *udata)
 {
 	return hashmap_new_with_allocator(
 		(_malloc?_malloc:kmalloc),

src/idt.c

@@ -103,8 +103,12 @@ void init_idt() {
 	pic_remap(0x20, 0x28);
 #ifdef USE_IOAPIC
 	pic_disable();
+	cpu_serialise();
 	remap_irqs_to_ioapic();
+	cpu_serialise();
 	init_lapic_timer(100);
+	cpu_serialise();
+	apic_setup_ap();
 #else
 	pic_enable();
 #endif

src/input.c

@@ -26,7 +26,8 @@ size_t kinput(size_t maxlen, console* cons)
 		cons->bufcnt = 0;
 	}
 
-	lock_spinlock(&console_spinlock);
+	uint64_t flags;
+	lock_spinlock_irq(&console_spinlock, &flags);
 	lock_spinlock(&debug_console_spinlock);
 	switch (cons->last) {
 		case KEY_UP:
@@ -61,8 +62,8 @@ size_t kinput(size_t maxlen, console* cons)
 			}
 		break;
 	}
-	unlock_spinlock(&console_spinlock);
 	unlock_spinlock(&debug_console_spinlock);
+	unlock_spinlock_irq(&console_spinlock, flags);
 	/* Terminate string */
 	*cons->buffer = 0;
 

src/interrupt.c

@@ -83,36 +83,33 @@ void local_apic_clear_interrupt()
 
 /* Both the Interrupt() and ISR() functions are dispatched from the assembly code trampoline via a pre-set IDT */
 
-void Interrupt(uint64_t isrnumber, uint64_t errorcode)
-{
-	uint8_t fx[512];
+void Interrupt(uint64_t isrnumber, uint64_t errorcode) {
+	__attribute__((aligned(16))) uint8_t fx[512];
 	__builtin_ia32_fxsave64(&fx);
-	for (shared_interrupt_t* si = shared_interrupt[isrnumber]; si; si = si->next) {
+
+	for (shared_interrupt_t *si = shared_interrupt[isrnumber]; si; si = si->next) {
 		/* There is no shared interrupt routing on these interrupts,
 		 * they are purely routed to interested handlers
 		 */
 		if (si->interrupt_handler) {
-			si->interrupt_handler((uint8_t)isrnumber, errorcode, 0, si->opaque);
+			si->interrupt_handler((uint8_t) isrnumber, errorcode, 0, si->opaque);
 		}
 	}
 
 	if (isrnumber < 32) {
 		/* This simple error handler is replaced by a more complex debugger once the system is up */
-		kprintf("CPU %d halted with exception %016lx, error code %016lx: %s.\n", logical_cpu_id(), isrnumber, errorcode, error_table[isrnumber]);
+		kprintf("CPU %d halted with exception %016lx, error code %016lx: %s.\n", logical_cpu_id(), isrnumber,
+			errorcode, error_table[isrnumber]);
 		wait_forever();
 	}
 
-#ifdef USE_IOAPIC
 	local_apic_clear_interrupt();
-#else
-	pic_eoi(isrnumber);
-#endif
 	__builtin_ia32_fxrstor64(&fx);
 }
 
 void IRQ(uint64_t isrnumber, uint64_t irqnum)
 {
-	uint8_t fx[512];
+	__attribute__((aligned(16))) uint8_t fx[512];
 	__builtin_ia32_fxsave64(&fx);
 	for (shared_interrupt_t* si = shared_interrupt[isrnumber]; si; si = si->next) {
 		if (si->device.bits == 0 && si->interrupt_handler) {
@@ -127,14 +124,7 @@ void IRQ(uint64_t isrnumber, uint64_t irqnum)
 			}
 		}
 	}
-#ifdef USE_IOAPIC
+
 	local_apic_clear_interrupt();
-#else
-	/* IRQ7 is the PIC spurious interrupt, we never acknowledge it */
-	if (irqnum != IRQ7) {
-		pic_eoi(isrnumber);
-	}
-#endif
 	__builtin_ia32_fxrstor64(&fx);
 }
-

src/ioapic.c

@@ -86,7 +86,8 @@ void ioapic_redir_set(uint32_t irq, uint32_t vector, uint32_t del_mode, uint32_t
 		return;
 	}
 	uint32_t lower = (vector & 0xff) | (del_mode << 8) | (dest_mode << 11) | (intpol << 13) | (trigger_mode << 15) | (mask << 16);
-	uint32_t upper = (dest_mode << 24);
+	uint32_t bsp_apic_id = get_lapic_id_from_cpu_id(logical_cpu_id());
+	uint32_t upper = (bsp_apic_id << 24);
 	ioapic_write_gsi(ioapic, gsi, lower, upper);
 	dprintf("Remapping IRQ %d -> GSI %d -> Vector %d (trigger mode: %s, polarity: %s)\n", irq, gsi, vector,
 		triggering_str(trigger_mode), polarity_str(trigger_mode));