smp-ready msi routing

Author: braindigitalis

include/idt.h

@@ -26,6 +26,25 @@ typedef struct idt_entry_t {
 
 extern volatile idt_ptr_t idt64;
 
+/**
+ * @brief Function pointer type for interrupt and IRQ handlers.
+ *
+ * This defines the signature of an interrupt service routine (ISR) that may be
+ * registered via `register_interrupt_handler`. It will be called whenever a
+ * matching interrupt or IRQ occurs.
+ *
+ * All handlers receive the interrupt vector number, error code (if any),
+ * the IRQ number (for hardware IRQs, this is `isrnumber - 32`), and a user-defined
+ * opaque pointer that was passed during registration. This allows for stateful
+ * or device-specific behaviour in shared or reentrant interrupt contexts.
+ *
+ * @param isrnumber The IDT entry number (0–255), corresponding to the triggered interrupt vector.
+ * @param errorcode A CPU-generated error code for certain exceptions (e.g. page faults). Always 0 for IRQs.
+ * @param irqnumber For IRQs, this is typically `isrnumber - 32`; otherwise 0.
+ * @param opaque The same pointer that was passed to `register_interrupt_handler`; used for context or state.
+ */
+typedef void (*isr_t)(uint8_t isrnumber, uint64_t errorcode, uint64_t irqnumber, void* opaque);
+
 /**
  * @brief Initialise the IDT and enable interrupts.
  *
@@ -77,31 +96,38 @@ void pic_eoi(int irq);
 void io_wait(void);
 
 /**
- * @brief Allocate a free MSI interrupt vector.
+ * @brief Allocate a free MSI interrupt vector on a given CPU.
  *
  * Allocates an IDT vector in the range 64–255 for use with
- * Message Signalled Interrupts (MSI/MSI-X).
+ * Message Signalled Interrupts (MSI/MSI-X), associated with
+ * the specified Local APIC ID.
+ *
+ * @param cpu Local APIC ID of the target CPU.
  *
  * @return Vector number (64–255) on success,
- *         -1 if no free vector is available.
+ *         -1 if no free vector is available on that CPU.
  *
  * @note After allocation, the driver must program the device’s
- *       MSI capability structure and register an interrupt
- *       handler for the vector.
+ *       MSI/MSI-X capability structure with the returned vector
+ *       and Local APIC ID, and register an interrupt handler
+ *       for the vector on the specified CPU.
  */
-int alloc_msi_vector(void);
+int alloc_msi_vector(uint8_t cpu);
 
 /**
- * @brief Free a previously allocated MSI interrupt vector.
+ * @brief Free a previously allocated MSI interrupt vector on a given CPU.
  *
- * Marks the given MSI vector as available for reuse.
+ * Marks the given MSI vector as available for reuse on the specified
+ * Local APIC ID.
  *
- * @param vec The MSI vector to free (64–255).
+ * @param cpu Local APIC ID of the CPU that the vector belongs to.
+ * @param vec    The MSI vector to free (64–255).
  *
  * @warning Behaviour is undefined if freeing a vector that
- *          was never allocated or is still in use.
+ *          was never allocated on the given CPU, or is still in use.
  *
  * @note Drivers should call this during teardown to avoid
- *       leaking interrupt vectors.
+ *       leaking interrupt vectors on that CPU.
  */
-void free_msi_vector(int vec);
+void free_msi_vector(uint8_t cpu, int vec);
+

include/interrupt.h

@@ -38,27 +38,6 @@ enum irq_number_t {
 	IRQ16 = 50,  ///< Local APIC timer vector (used instead of IRQ0 in APIC mode)
 };
 
-
-/**
- * @brief Function pointer type for interrupt and IRQ handlers.
- *
- * This defines the signature of an interrupt service routine (ISR) that may be
- * registered via `register_interrupt_handler`. It will be called whenever a
- * matching interrupt or IRQ occurs.
- *
- * All handlers receive the interrupt vector number, error code (if any),
- * the IRQ number (for hardware IRQs, this is `isrnumber - 32`), and a user-defined
- * opaque pointer that was passed during registration. This allows for stateful
- * or device-specific behaviour in shared or reentrant interrupt contexts.
- *
- * @param isrnumber The IDT entry number (0–255), corresponding to the triggered interrupt vector.
- * @param errorcode A CPU-generated error code for certain exceptions (e.g. page faults). Always 0 for IRQs.
- * @param irqnumber For IRQs, this is typically `isrnumber - 32`; otherwise 0.
- * @param opaque The same pointer that was passed to `register_interrupt_handler`; used for context or state.
- */
-typedef void (*isr_t)(uint8_t isrnumber, uint64_t errorcode, uint64_t irqnumber, void* opaque);
-
-
 /**
  * @brief Represents a single handler in a chain of handlers attached to a shared ISR.
  *

include/pci.h

@@ -87,8 +87,13 @@ extern pci_dev_t dev_zero;
 #define PCI_MSI_EDGETRIGGER	 (1 << 15)
 #define PCI_MSI_ENABLE		 (1 << 16)
 
-#define PCI_BAR_TYPE_MEMORY      0x00
-#define PCI_BAR_TYPE_IOPORT      0x01
+#define PCI_CAPABILITY_MSIX	 0x11
+
+#define PCI_MSIX_ENABLE		 (1 << 15)
+#define PCI_MSIX_FUNCTION_MASK	 (1 << 14)
+
+#define PCI_BAR_TYPE_MEMORY       0x00
+#define PCI_BAR_TYPE_IOPORT       0x01
 
 #define PCI_HEADER_TYPE_DEVICE  0
 #define PCI_HEADER_TYPE_BRIDGE  1
@@ -101,23 +106,187 @@ extern pci_dev_t dev_zero;
 #define DEVICE_PER_BUS           32
 #define FUNCTION_PER_DEVICE      32
 
+/**
+ * @brief Read a value from a PCI configuration space field.
+ * @param dev PCI device descriptor.
+ * @param field Offset of the configuration field.
+ * @return The value read (size determined by field).
+ */
 uint32_t pci_read(pci_dev_t dev, uint32_t field);
+
+/**
+ * @brief Write a value into a PCI configuration space field.
+ * @param dev PCI device descriptor.
+ * @param field Offset of the configuration field.
+ * @param value Value to write (size determined by field).
+ */
 void pci_write(pci_dev_t dev, uint32_t field, uint32_t value);
+
+/**
+ * @brief Get the PCI device class/subclass identifier.
+ * @param dev PCI device descriptor.
+ * @return Encoded device type (class << 8 | subclass).
+ */
 uint32_t get_device_type(pci_dev_t dev);
+
+/**
+ * @brief Get the secondary bus number from a PCI bridge device.
+ * @param dev PCI bridge device descriptor.
+ * @return Secondary bus number.
+ */
 uint32_t get_secondary_bus(pci_dev_t dev);
+
+/**
+ * @brief Determine if the device is a single-function endpoint.
+ * @param dev PCI device descriptor.
+ * @return Non-zero if endpoint, zero if multifunction/bridge.
+ */
 uint32_t pci_reach_end(pci_dev_t dev);
-pci_dev_t pci_scan_function(uint16_t vendor_id, uint16_t device_id, uint32_t bus, uint32_t device, uint32_t function, int device_type);
-pci_dev_t pci_scan_device(uint16_t vendor_id, uint16_t device_id, uint32_t bus, uint32_t device, int device_type);
-pci_dev_t pci_scan_bus(uint16_t vendor_id, uint16_t device_id, uint32_t bus, int device_type);
-pci_dev_t pci_get_device(uint16_t vendor_id, uint16_t device_id, int device_type);
+
+/**
+ * @brief Scan a specific PCI function for matching IDs or type.
+ * @param vendor_id Vendor ID to match (0 = wildcard).
+ * @param device_id Device ID to match (0 = wildcard).
+ * @param bus Bus number.
+ * @param device Device slot number.
+ * @param function Function number.
+ * @param device_type Device class/type to match (-1 = any).
+ * @return Matching PCI device or dev_zero if none.
+ */
+pci_dev_t pci_scan_function(uint16_t vendor_id, uint16_t device_id,
+			    uint32_t bus, uint32_t device,
+			    uint32_t function, int device_type);
+
+/**
+ * @brief Scan all functions of a given PCI device slot.
+ * @param vendor_id Vendor ID to match (0 = wildcard).
+ * @param device_id Device ID to match (0 = wildcard).
+ * @param bus Bus number.
+ * @param device Device slot number.
+ * @param device_type Device class/type to match (-1 = any).
+ * @return Matching PCI device or dev_zero if none.
+ */
+pci_dev_t pci_scan_device(uint16_t vendor_id, uint16_t device_id,
+			  uint32_t bus, uint32_t device,
+			  int device_type);
+
+/**
+ * @brief Scan all devices on a given PCI bus.
+ * @param vendor_id Vendor ID to match (0 = wildcard).
+ * @param device_id Device ID to match (0 = wildcard).
+ * @param bus Bus number.
+ * @param device_type Device class/type to match (-1 = any).
+ * @return First matching PCI device or dev_zero if none.
+ */
+pci_dev_t pci_scan_bus(uint16_t vendor_id, uint16_t device_id,
+		       uint32_t bus, int device_type);
+
+/**
+ * @brief Find a device by vendor, device, or type.
+ * @param vendor_id Vendor ID to match (0 = any).
+ * @param device_id Device ID to match (0 = any).
+ * @param device_type Device class/type to match (-1 = any).
+ * @return Matching PCI device or dev_zero if none.
+ */
+pci_dev_t pci_get_device(uint16_t vendor_id, uint16_t device_id,
+			 int device_type);
+
+/**
+ * @brief Initialise the PCI subsystem and enumerate devices.
+ */
 void init_pci();
+
+/**
+ * @brief Enable bus mastering on a PCI device.
+ * @param device PCI device descriptor.
+ * @return True if bus mastering was newly enabled, false if already set.
+ */
 bool pci_bus_master(pci_dev_t device);
+
+/**
+ * @brief Get the type of a PCI Base Address Register (BAR).
+ * @param field Raw BAR field value.
+ * @return 0 = memory, 1 = I/O.
+ */
 uint8_t pci_bar_type(uint32_t field);
+
+/**
+ * @brief Extract an I/O base address from a BAR.
+ * @param field Raw BAR field value.
+ * @return I/O port base address.
+ */
 uint16_t pci_io_base(uint32_t field);
+
+/**
+ * @brief Extract a memory base address from a BAR.
+ * @param field Raw BAR field value.
+ * @return Memory-mapped base address.
+ */
 uint32_t pci_mem_base(uint32_t field);
+
+/**
+ * @brief Test whether a device descriptor represents 'not found'.
+ * @param device PCI device descriptor.
+ * @return True if no device, false otherwise.
+ */
 bool pci_not_found(pci_dev_t device);
 
-bool pci_enable_msi(pci_dev_t device, uint32_t vector);
+/**
+ * @brief Enable MSI (Message Signalled Interrupts) on a device.
+ * @param device PCI device descriptor.
+ * @param vector APIC interrupt vector to use.
+ * @param lapic_id local APIC ID of the CPU to route the vector to
+ * @return True if MSI was enabled successfully.
+ */
+bool pci_enable_msi(pci_dev_t device, uint32_t vector, uint32_t lapic_id);
+
+/**
+ * @brief Display enumerated PCI devices to the system log.
+ */
 void pci_display_device_list();
+
+/**
+ * @brief Get a list of all enumerated PCI devices.
+ * @param list Pointer to array of devices (output).
+ * @return Number of devices in the list.
+ */
 size_t pci_get_device_list(pci_dev_t** list);
-void pci_interrupt_enable(pci_dev_t device, bool enable);
+
+/**
+ * @brief Enable or disable legacy INTx interrupt signalling.
+ * @param device PCI device descriptor.
+ * @param enable True = enable, False = disable.
+ */
+void pci_interrupt_enable(pci_dev_t device, bool enable);
+
+/**
+ * @brief Enable MSI-X (extended message signalled interrupts).
+ * @param device PCI device descriptor.
+ * @param vector APIC interrupt vector to assign.
+ * @param entry MSI-X table entry to configure.
+ * @param lapic_id local APIC ID of the CPU to route the vector to
+ * @return True if MSI-X was enabled successfully.
+ */
+bool pci_enable_msix(pci_dev_t device, uint32_t vector, uint16_t entry, uint32_t lapic_id);
+
+/**
+ * @brief Configure an interrupt for a PCI device (MSI/MSI-X if available).
+ *
+ * Attempts to enable MSI/MSI-X for the given PCI device, routing the
+ * interrupt to the specified Local APIC ID. If the device does not support
+ * MSI/MSI-X, falls back to legacy INTx routing.
+ *
+ * @param name     Human-readable device name (used in logs/debug output).
+ * @param dev      The PCI device handle.
+ * @param lapic_id Local APIC ID of the target CPU that should receive the
+ *                 interrupt.
+ * @param handler  Interrupt service routine to register.
+ * @param context  Opaque pointer passed to the ISR when the interrupt fires.
+ *
+ * @return The assigned interrupt vector (64–255 if MSI/MSI-X was used,
+ *         or IRQ_START + line if falling back to legacy INTx).
+ *
+ * @note Drivers should always check the returned vector to confirm which
+ *       interrupt mechanism was actually configured.
+ */
+uint32_t pci_setup_interrupt(const char* name, pci_dev_t dev, uint8_t lapic_id, isr_t handler, void *context);

src/ahci.c

@@ -594,21 +594,8 @@ void init_ahci()
 	}
 
 	ahci_base = pci_mem_base(ahci_base);
-
-	uint32_t irq_num = pci_read(ahci_device, PCI_INTERRUPT_LINE);
-	uint32_t irq_pin = pci_read(ahci_device, PCI_INTERRUPT_PIN);
-	uint32_t vector = alloc_msi_vector();
-	bool msi_ok = pci_enable_msi(ahci_device, vector);
-	if (msi_ok) {
-		register_interrupt_handler(vector, ahci_handler, ahci_device, (void *) ahci_base);
-		kprintf("AHCI: MSI on, INT %d\n", vector);
-	} else {
-		free_msi_vector(vector);
-		register_interrupt_handler(IRQ_START + irq_num, ahci_handler, ahci_device, (void *) ahci_base);
-		kprintf("AHCI: MSI off, INT %d\n", irq_num);
-	}
-	pci_interrupt_enable(ahci_device, true);
-	dprintf("AHCI base MMIO: %08x INT %d PIN#%c\n", ahci_base, irq_num, irq_pin + 'A' - 1);
+	uint32_t irq_num = pci_setup_interrupt("AHCI", ahci_device, cpu_id(), ahci_handler, ahci_base);
+	dprintf("AHCI base MMIO: %08x INT %d\n", ahci_base, irq_num);
 
 	probe_port((ahci_hba_mem_t*)ahci_base, ahci_device);
 }

src/e1000.c

@@ -319,17 +319,36 @@ bool e1000_start(pci_dev_t *pci_device) {
 	}
 
 	if (e1000_device_id == E1000_82540EM) {
-		uint8_t vector = alloc_msi_vector();
-		bool msi_ok = pci_enable_msi(*pci_device, vector);
-		if (msi_ok) {
-			kprintf("e1000: MSI enabled, INT %d\n", vector);
-			register_interrupt_handler(vector, e1000_handler, *pci_device, NULL);
-		} else {
-			free_msi_vector(vector);
-			uint32_t irq_num = pci_read(*pci_device, PCI_INTERRUPT_LINE);
-			register_interrupt_handler(IRQ_START + irq_num, e1000_handler, *pci_device, NULL);
-		}
+		/* Attempting MSI setup is safe here */
+		pci_setup_interrupt("e1000", *pci_device, cpu_id(), e1000_handler, NULL);
 	} else {
+		/* But not here! The 82541PI actively torpedoes the system if you enable MSI, see its errata:
+		 *
+		 * 82541PI GIGABIT ETHERNET CONTROLLER SPECIFICATION UPDATE
+		 *
+		 * 7. Message Signaled Interrupt Feature May Corrupt Write Transactions
+		 *
+		 * Problem: The problem is with the implementation of the Message Signaled Interrupt (MSI) feature in the Ethernet
+		 * controllers. During MSI writes, the controller incorrectly accesses the write data FIFO. If there are pending write
+		 * transactions when this occurs, these transactions may become corrupted, which may cause the network
+		 * controller to lock up and become unresponsive.
+		 *
+		 * For a normal PCI write transaction, the controller’s PCI logic receives data to be written from an internal FIFO.
+		 * Once the controller is given bus ownership, the PCI logic pulls the data out of this FIFO and performs the write
+		 * transaction.
+		 *
+		 * For systems using MSI writes, the data, which is constant, should be pulled from the controller’s PCI
+		 * Configuration Space rather than the internal FIFO. The affected devices are not pulling this data from PCI
+		 * Configuration Space. Instead, they are pulling data from the internal FIFO.
+		 *
+		 * Implication: If the affected products are used with a future OS that uses Message Signal Interrupts and no accommodations
+		 * are made to mitigate the use of these interrupts, data integrity issues may occur.
+		 *
+		 * Workaround: For PCI systems, advertisement of the MSI capability can be turned off by setting the MSI Disable bit in the
+		 * EEPROM (Init Control Word 2, bit 7).
+		 *
+		 * Status: Intel does not plan to resolve this erratum in the 82541 Gigabit Ethernet controller.
+		 */
 		uint32_t irq_num = pci_read(*pci_device, PCI_INTERRUPT_LINE);
 		register_interrupt_handler(IRQ_START + irq_num, e1000_handler, *pci_device, NULL);