Skip to content

Commit

Permalink
feat(x86_64): real abstraction for ISA IRQs, unfuck I/O APIC (#490)
Browse files Browse the repository at this point in the history 
Basically, the current way you interact with ISA standard PC interrupts
in the Mycelium x86_64 HAL is nonsensical, in several ways:

1. For some reason, we were mapping the I/O APIC ISA IRQs and the PIC
   ISA IRQs to separate regions in the IDT, even though you can only
   ever have either an I/O APIC *or* a PIC. Because of this, we were
   creating separate entries for each ISA interrupt's ISR in the PIC
   region and the I/O APIC region of the IDT. This doesn't actually
   break anything, but it's stupid and dumb and I don't know why I did
   that.

2. I made an attempt to have an interface for controlling ISA interrupts
   that abstracts over the I/O APIC and PIC interrupt models. But,
   currently, you can't actually *use* this thing to do anything you
   might want to do in an abstract way (mask/unmask/acknowledge an
   interrupt). Instead, you have to actually inspect which interrupt
   model you have. So, it's pointless.

3. Apparently the motherboard manufacturer gets to route ISA interrupts
   to I/O APIC pins in "whatever random order they want to". Currently,
   we hard-code the PS/2 keyboard and PIT timer interrupts to the
   vectors they *happen* to be on when I run `qemu-system-x86_64 -cpu
   qemu64` on *my* computer, but in real life, they could be anything.
   There's a thing in the ACPI tables that tells you how these are
   routed, but we're not using it. This is bad.

4. Also there's a bug where apparently when we unmask the PIT interrupt,
   it might fire spuriously, even if we haven't actually done an `sti`.
   I don't get this and it seems wrong, but it results in the IRQ being
   raised *before* configuring the static that tells you whether you
   have the APIC or PIC interrupt model so when you try to actually send
   an EOI, you dereference uninitialized memory and the kernel just
   hangs. This happens occasionally when running in QEMU with the
   default settings, and seems to happen consistently with `-machine
   accel=kvm`, so that suggests it is maybe worth worrying about.

This branch, uh, fixes all of that stuff. I made the abstraction for ISA
interrupts actually usable, and you can now mask/unmask/acknowledge them
from ISRs or anywhere else by interrupt name, instead of having to know
what interrupt model we have. Also, we now do the correct thing with
regard to MADT interrupt source overrides (read: we actually know about
them), and I fixed the spooky spurious interrupt behavior with the PIT
by not unmasking it until after the interrupt controller static is
initialized.

Please clap.
  • Loading branch information
@hawkw
hawkw authored Dec 29, 2024
1 parent 68a8cd2 commit 2cefeb1
Show file tree
Hide file tree
Showing 5 changed files with 411 additions and 128 deletions.
189 changes: 130 additions & 59 deletions hal-x86_64/src/interrupt.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::{cpu, mm, segment, time, PAddr, VAddr};
use crate::{cpu, mm, segment, time, VAddr};
use core::{arch::asm, marker::PhantomData, time::Duration};
use hal_core::interrupt::Control;
use hal_core::interrupt::{ctx, Handlers};
Expand All @@ -15,7 +15,7 @@ pub mod apic;
pub mod idt;
pub mod pic;

use self::apic::{IoApic, LocalApic};
use self::apic::{IoApicSet, LocalApic};
pub use idt::Idt;
pub use pic::CascadedPic;

Expand Down Expand Up @@ -69,10 +69,7 @@ enum InterruptModel {
/// [apics]: apic
Apic {
local: apic::LocalApic,
// TODO(eliza): allow further configuration of the I/O APIC (e.g.
// masking/unmasking stuff...)
#[allow(dead_code)]
io: Mutex<apic::IoApic, Spinlock>,
io: apic::IoApicSet,
},
}

Expand Down Expand Up @@ -134,6 +131,67 @@ pub struct Registers {
static IDT: Mutex<idt::Idt, Spinlock> = Mutex::new_with_raw_mutex(idt::Idt::new(), Spinlock::new());
static INTERRUPT_CONTROLLER: InitOnce<Controller> = InitOnce::uninitialized();

pub enum MaskError {
NotHwIrq,
}

/// ISA interrupt vectors
///
/// See: [the other wiki](https://wiki.osdev.org/Interrupts#General_IBM-PC_Compatible_Interrupt_Information)
#[derive(Copy, Clone, Debug)]
#[repr(u8)]
pub enum IsaInterrupt {
/// Programmable Interval Timer (PIT) timer interrupt.
PitTimer = 0,
/// PS/2 keyboard controller interrupt.
Ps2Keyboard = 1,
// IRQ 2 is reserved for the PIC cascade interrupt and isn't user accessible!
/// COM2 / COM4 serial port interrupt.
Com2 = 3,
/// COM1 / COM3 serial port interrupt.
Com1 = 4,
/// LPT2 parallel port interrupt.
Lpt2 = 5,
/// Floppy disk
Floppy = 6,
/// LPT1 parallel port interrupt, or spurious.
Lpt1 = 7,
/// CMOS real-time clock.
CmosRtc = 8,
/// Free for peripherals/SCSI/NIC
Periph1 = 9,
Periph2 = 10,
Periph3 = 11,
/// PS/2 Mouse
Ps2Mouse = 12,
/// FPU
Fpu = 13,
/// Primary ATA hard disk
AtaPrimary = 14,
/// Secondary ATA hard disk
AtaSecondary = 15,
}

impl IsaInterrupt {
pub const ALL: [IsaInterrupt; 15] = [
IsaInterrupt::PitTimer,
IsaInterrupt::Ps2Keyboard,
IsaInterrupt::Com2,
IsaInterrupt::Com1,
IsaInterrupt::Lpt2,
IsaInterrupt::Floppy,
IsaInterrupt::Lpt1,
IsaInterrupt::CmosRtc,
IsaInterrupt::Periph1,
IsaInterrupt::Periph2,
IsaInterrupt::Periph3,
IsaInterrupt::Ps2Mouse,
IsaInterrupt::Fpu,
IsaInterrupt::AtaPrimary,
IsaInterrupt::AtaSecondary,
];
}

impl Controller {
// const DEFAULT_IOAPIC_BASE_PADDR: u64 = 0xFEC00000;

Expand All @@ -152,6 +210,31 @@ impl Controller {
}
}

pub fn mask_isa_irq(&self, irq: IsaInterrupt) {
match self.model {
InterruptModel::Pic(ref pics) => pics.lock().mask(irq),
InterruptModel::Apic { ref io, .. } => io.set_isa_masked(irq, true),
}
}

pub fn unmask_isa_irq(&self, irq: IsaInterrupt) {
match self.model {
InterruptModel::Pic(ref pics) => pics.lock().unmask(irq),
InterruptModel::Apic { ref io, .. } => io.set_isa_masked(irq, false),
}
}

/// # Safety
///
/// Calling this when there isn't actually an ISA interrupt pending can do
/// arbitrary bad things (which I think is basically just faulting the CPU).
pub unsafe fn end_isa_irq(&self, irq: IsaInterrupt) {
match self.model {
InterruptModel::Pic(ref pics) => pics.lock().end_interrupt(irq),
InterruptModel::Apic { ref local, .. } => local.end_interrupt(),
}
}

pub fn enable_hardware_interrupts(
acpi: Option<&acpi::InterruptModel>,
frame_alloc: &impl hal_core::mem::page::Alloc<mm::size::Size4Kb>,
Expand All @@ -169,7 +252,7 @@ impl Controller {
pics.set_irq_address(Idt::PIC_BIG_START as u8, Idt::PIC_LITTLE_START as u8);
}

let model = match acpi {
let controller = match acpi {
Some(acpi::InterruptModel::Apic(apic_info)) => {
tracing::info!("detected APIC interrupt model");

Expand All @@ -181,38 +264,17 @@ impl Controller {
}
tracing::info!("disabled 8259 PICs");

// configure the I/O APIC
let mut io = {
// TODO(eliza): consider actually using other I/O APICs? do
// we need them for anything??
tracing::trace!(?apic_info.io_apics, "found {} IO APICs", apic_info.io_apics.len());

let io_apic = &apic_info.io_apics[0];
let addr = PAddr::from_u64(io_apic.address as u64);

tracing::debug!(ioapic.paddr = ?addr, "IOAPIC");
IoApic::new(addr, &mut pagectrl, frame_alloc)
};

// map the standard ISA hardware interrupts to I/O APIC
// redirection entries.
io.map_isa_irqs(Idt::IOAPIC_START as u8);

// unmask the PIT timer vector --- we'll need this for calibrating
// the local APIC timer...
io.set_masked(IoApic::PIT_TIMER_IRQ, false);

// unmask the PS/2 keyboard interrupt as well.
io.set_masked(IoApic::PS2_KEYBOARD_IRQ, false);
// configure the I/O APIC(s)
let io = IoApicSet::new(apic_info, frame_alloc, &mut pagectrl, Idt::ISA_BASE as u8);

// enable the local APIC
let local = LocalApic::new(&mut pagectrl, frame_alloc);
local.enable(Idt::LOCAL_APIC_SPURIOUS as u8);

InterruptModel::Apic {
local,
io: Mutex::new_with_raw_mutex(io, Spinlock::new()),
}
let model = InterruptModel::Apic { local, io };

tracing::trace!(interrupt_model = ?model);
INTERRUPT_CONTROLLER.init(Self { model })
}
model => {
if model.is_none() {
Expand All @@ -229,21 +291,37 @@ impl Controller {
// clear for you, the reader, that at this point they are definitely intentionally enabled.
pics.enable();
}
InterruptModel::Pic(Mutex::new_with_raw_mutex(pics, Spinlock::new()))
INTERRUPT_CONTROLLER.init(Self {
model: InterruptModel::Pic(Mutex::new_with_raw_mutex(pics, Spinlock::new())),
})
}
};
tracing::trace!(interrupt_model = ?model);

let controller = INTERRUPT_CONTROLLER.init(Self { model });

// `sti` may not be called until the interrupt controller static is
// fully initialized, as an interrupt that occurs before it is
// initialized may attempt to access the static to finish the interrupt!
core::sync::atomic::fence(core::sync::atomic::Ordering::SeqCst);
unsafe {
crate::cpu::intrinsics::sti();
}

// There's a weird behavior in QEMU where, apparently, when we unmask
// the PIT interrupt, it might fire spuriously *as soon as its
// unmasked*, even if we haven't actually done an `sti`. I don't get
// this and it seems wrong, but it seems to happen occasionally with the
// default QEMU acceleration, and pretty consistently with `-machine
// accel=kvm`, so *maybe* it can also happen on a real computer?
//
// Anyway, because of this, we can't unmask the PIT interrupt until
// after we've actually initialized the interrupt controller static.
// Otherwise, if we unmask it before initializing the static (like we
// used to), the interrupt gets raised immediately, and when the ISR
// tries to actually send an EOI to ack it, it dereferences
// uninitialized memory and the kernel just hangs. So, we wait to do it
// until here.
//
// The fact that this behavior exists at all makes me really, profoundly
// uncomfortable: shouldn't `cli` like, actually do what it's supposed
// to? But, we can just choose to unmask the PIT later and it's fine, I
// guess...
controller.unmask_isa_irq(IsaInterrupt::PitTimer);
controller.unmask_isa_irq(IsaInterrupt::Ps2Keyboard);
controller
}

Expand Down Expand Up @@ -405,12 +483,9 @@ impl hal_core::interrupt::Control for Idt {
tracing::trace!("PIT sleep completed");
}
unsafe {
match INTERRUPT_CONTROLLER.get_unchecked().model {
InterruptModel::Pic(ref pics) => {
pics.lock().end_interrupt(Idt::PIC_PIT_TIMER as u8)
}
InterruptModel::Apic { ref local, .. } => local.end_interrupt(),
}
INTERRUPT_CONTROLLER
.get_unchecked()
.end_isa_irq(IsaInterrupt::PitTimer);
}
}

Expand All @@ -432,12 +507,9 @@ impl hal_core::interrupt::Control for Idt {
let scancode = unsafe { PORT.readb() };
H::ps2_keyboard(scancode);
unsafe {
match INTERRUPT_CONTROLLER.get_unchecked().model {
InterruptModel::Pic(ref pics) => {
pics.lock().end_interrupt(Idt::PIC_PS2_KEYBOARD as u8)
}
InterruptModel::Apic { ref local, .. } => local.end_interrupt(),
}
INTERRUPT_CONTROLLER
.get_unchecked()
.end_isa_irq(IsaInterrupt::Ps2Keyboard);
}
}

Expand Down Expand Up @@ -591,11 +663,10 @@ impl hal_core::interrupt::Control for Idt {
// === hardware interrupts ===
// ISA standard hardware interrupts mapped on both the PICs and IO APIC
// interrupt models.
self.register_isr(Self::PIC_PIT_TIMER, pit_timer_isr::<H> as *const ());
self.register_isr(Self::IOAPIC_PIT_TIMER, pit_timer_isr::<H> as *const ());
self.register_isr(Self::PIC_PS2_KEYBOARD, keyboard_isr::<H> as *const ());
self.register_isr(Self::IOAPIC_PS2_KEYBOARD, keyboard_isr::<H> as *const ());
// local APIC specific hardware itnerrupts
self.register_isa_isr(IsaInterrupt::PitTimer, pit_timer_isr::<H> as *const ());
self.register_isa_isr(IsaInterrupt::Ps2Keyboard, keyboard_isr::<H> as *const ());

// local APIC specific hardware interrupts
self.register_isr(Self::LOCAL_APIC_SPURIOUS, spurious_isr as *const ());
self.register_isr(Self::LOCAL_APIC_TIMER, apic_timer_isr::<H> as *const ());

Expand Down
2 changes: 1 addition & 1 deletion hal-x86_64/src/interrupt/apic.rs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! Advanced Programmable Interrupt Controller (APIC).
pub mod io;
pub mod local;
pub use io::IoApic;
pub use io::{IoApic, IoApicSet};
pub use local::LocalApic;

use mycelium_util::bits::enum_from_bits;
Expand Down
Loading

0 comments on commit 2cefeb1

Please sign in to comment.