From 73ba776ca0c651431a4af9a97f45a71ba524b335 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 4 Sep 2024 09:24:39 -0700 Subject: [PATCH 01/16] docs: link to changelogs in published crate READMEs (#485) This just seemed kinda nice to add. --- bitfield/README.md | 4 ++++ cordyceps/README.md | 6 +++++- maitake-sync/README.md | 6 +++++- 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/bitfield/README.md b/bitfield/README.md index 1551ac41..239b2c50 100644 --- a/bitfield/README.md +++ b/bitfield/README.md @@ -3,6 +3,7 @@ 🍄 bitfield utilities, courtesy of [Mycelium]. [![crates.io][crates-badge]][crates-url] +[![Changelog][changelog-badge]][changelog-url] [![Documentation][docs-badge]][docs-url] [![Documentation (HEAD)][docs-main-badge]][docs-main-url] [![MIT licensed][mit-badge]][mit-url] @@ -11,6 +12,9 @@ [crates-badge]: https://img.shields.io/crates/v/mycelium-bitfield.svg [crates-url]: https://crates.io/crates/mycelium-bitfield +[changelog-badge]: + https://img.shields.io/crates/v/mycelium-bitfield?label=changelog&color=blue +[changelog-url]: https://github.com/hawkw/mycelium/blob/main/mycelium-bitfield/CHANGELOG.md [docs-badge]: https://docs.rs/mycelium-bitfield/badge.svg [docs-url]: https://docs.rs/mycelium-bitfield [docs-main-badge]: https://img.shields.io/netlify/3ec00bb5-251a-4f83-ac7f-3799d95db0e6?label=docs%20%28main%20branch%29 diff --git a/cordyceps/README.md b/cordyceps/README.md index 8ed3f2ce..ccb71a5b 100644 --- a/cordyceps/README.md +++ b/cordyceps/README.md @@ -3,6 +3,7 @@ 🍄 the [Mycelium] intrusive data structures library. [![crates.io][crates-badge]][crates-url] +[![Changelog][changelog-badge]][changelog-url] [![Documentation][docs-badge]][docs-url] [![Documentation (HEAD)][docs-main-badge]][docs-main-url] [![MIT licensed][mit-badge]][mit-url] @@ -11,6 +12,9 @@ [crates-badge]: https://img.shields.io/crates/v/cordyceps.svg [crates-url]: https://crates.io/crates/cordyceps +[changelog-badge]: + https://img.shields.io/crates/v/cordyceps?label=changelog&color=blue +[changelog-url]: https://github.com/hawkw/mycelium/blob/main/cordyceps/CHANGELOG.md [docs-badge]: https://docs.rs/cordyceps/badge.svg [docs-url]: https://docs.rs/cordyceps [docs-main-badge]: https://img.shields.io/netlify/3ec00bb5-251a-4f83-ac7f-3799d95db0e6?label=docs%20%28main%20branch%29 @@ -106,4 +110,4 @@ The following features are available (this list is incomplete; you can help by [ [queue]: https://docs.rs/cordyceps/latest/cordyceps/mpsc_queue/struct.MpscQueue.html [`Linked`]: https://docs.rs/cordyceps/latest/cordyceps/trait.Linked.html [`liballoc`]: https://doc.rust-lang.org/alloc/ -[`libstd`]: https://doc.rust-lang.org/std/ \ No newline at end of file +[`libstd`]: https://doc.rust-lang.org/std/ diff --git a/maitake-sync/README.md b/maitake-sync/README.md index e8897d20..3e35ec52 100644 --- a/maitake-sync/README.md +++ b/maitake-sync/README.md @@ -4,6 +4,7 @@ primitives from [`maitake`] [![crates.io][crates-badge]][crates-url] +[![Changelog][changelog-badge]][changelog-url] [![Documentation][docs-badge]][docs-url] [![Documentation (HEAD)][docs-main-badge]][docs-main-url] [![MIT licensed][mit-badge]][mit-url] @@ -11,7 +12,10 @@ primitives from [`maitake`] [![Sponsor @hawkw on GitHub Sponsors][sponsor-badge]][sponsor-url] [crates-badge]: https://img.shields.io/crates/v/maitake-sync.svg -[crates-url]: https://crates.io/crates/maitake-sync-sync +[crates-url]: https://crates.io/crates/maitake-sync +[changelog-badge]: + https://img.shields.io/crates/v/maitake-sync?label=changelog&color=blue +[changelog-url]: https://github.com/hawkw/mycelium/blob/main/maitake-sync/CHANGELOG.md [docs-badge]: https://docs.rs/maitake-sync/badge.svg [docs-url]: https://docs.rs/maitake-sync [docs-main-badge]: https://img.shields.io/netlify/3ec00bb5-251a-4f83-ac7f-3799d95db0e6?label=docs%20%28main%20branch%29 From ba56bb4d02f46fb59754b7b88bddc2e8ca99c1f5 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 8 Oct 2024 10:37:49 -0700 Subject: [PATCH 02/16] feat(x86_64): make raw IDT methods public (#487) MnemOS would like us to provide lower-level access to the IDT in `hal-x86_64` so that they can work around the nice, high level abstractions we worked so hard to provide for them (see tosc-rs/mnemos#337). This PR capitulates to mnemOS' demands by making the IDT methods for registering raw ISR functions public. I hope mnemOS doesn't shoot themselves in the foot using this :) --- hal-x86_64/src/interrupt.rs | 28 +++++------ hal-x86_64/src/interrupt/idt.rs | 89 +++++++++++++++++++++++++++++---- 2 files changed, 93 insertions(+), 24 deletions(-) diff --git a/hal-x86_64/src/interrupt.rs b/hal-x86_64/src/interrupt.rs index c66f3f89..5bb7a521 100644 --- a/hal-x86_64/src/interrupt.rs +++ b/hal-x86_64/src/interrupt.rs @@ -349,7 +349,7 @@ impl hal_core::interrupt::Control for Idt { ($self:ident, $h:ty, $($vector:path => fn $name:ident($($rest:tt)+),)+) => { $( gen_code_faults! {@ $name($($rest)+); } - $self.set_isr($vector, $name::<$h> as *const ()); + $self.register_isr($vector, $name::<$h> as *const ()); )+ }; (@ $name:ident($kind:literal);) => { @@ -575,32 +575,32 @@ impl hal_core::interrupt::Control for Idt { } // other exceptions, not mapped to the "code fault" handler - self.set_isr(Self::PAGE_FAULT, page_fault_isr:: as *const ()); - self.set_isr(Self::INVALID_TSS, invalid_tss_isr:: as *const ()); - self.set_isr( + self.register_isr(Self::PAGE_FAULT, page_fault_isr:: as *const ()); + self.register_isr(Self::INVALID_TSS, invalid_tss_isr:: as *const ()); + self.register_isr( Self::SEGMENT_NOT_PRESENT, segment_not_present_isr:: as *const (), ); - self.set_isr( + self.register_isr( Self::STACK_SEGMENT_FAULT, stack_segment_isr:: as *const (), ); - self.set_isr(Self::GENERAL_PROTECTION_FAULT, gpf_isr:: as *const ()); - self.set_isr(Self::DOUBLE_FAULT, double_fault_isr:: as *const ()); + self.register_isr(Self::GENERAL_PROTECTION_FAULT, gpf_isr:: as *const ()); + self.register_isr(Self::DOUBLE_FAULT, double_fault_isr:: as *const ()); // === hardware interrupts === // ISA standard hardware interrupts mapped on both the PICs and IO APIC // interrupt models. - self.set_isr(Self::PIC_PIT_TIMER, pit_timer_isr:: as *const ()); - self.set_isr(Self::IOAPIC_PIT_TIMER, pit_timer_isr:: as *const ()); - self.set_isr(Self::PIC_PS2_KEYBOARD, keyboard_isr:: as *const ()); - self.set_isr(Self::IOAPIC_PS2_KEYBOARD, keyboard_isr:: as *const ()); + self.register_isr(Self::PIC_PIT_TIMER, pit_timer_isr:: as *const ()); + self.register_isr(Self::IOAPIC_PIT_TIMER, pit_timer_isr:: as *const ()); + self.register_isr(Self::PIC_PS2_KEYBOARD, keyboard_isr:: as *const ()); + self.register_isr(Self::IOAPIC_PS2_KEYBOARD, keyboard_isr:: as *const ()); // local APIC specific hardware itnerrupts - self.set_isr(Self::LOCAL_APIC_SPURIOUS, spurious_isr as *const ()); - self.set_isr(Self::LOCAL_APIC_TIMER, apic_timer_isr:: as *const ()); + self.register_isr(Self::LOCAL_APIC_SPURIOUS, spurious_isr as *const ()); + self.register_isr(Self::LOCAL_APIC_TIMER, apic_timer_isr:: as *const ()); // vector 69 (nice) is reserved by the HAL for testing the IDT. - self.set_isr(69, test_isr:: as *const ()); + self.register_isr(69, test_isr:: as *const ()); Ok(()) } diff --git a/hal-x86_64/src/interrupt/idt.rs b/hal-x86_64/src/interrupt/idt.rs index 13f20dd7..116a1646 100644 --- a/hal-x86_64/src/interrupt/idt.rs +++ b/hal-x86_64/src/interrupt/idt.rs @@ -102,7 +102,7 @@ impl bits::FromBits for GateKind { // === impl Idt === impl Idt { - pub(super) const NUM_VECTORS: usize = 256; + pub const NUM_VECTORS: usize = 256; /// Divide-by-zero interrupt (#D0) pub const DIVIDE_BY_ZERO: usize = 0; @@ -155,18 +155,87 @@ impl Idt { /// Chosen by fair die roll, guaranteed to be random. pub const DOUBLE_FAULT_IST_OFFSET: usize = 4; + /// PIC Programmable Interval Timer (PIT) interrupt vector mapped by + /// [`Controller::enable_hardware_interrupts`]. + /// + /// Systems which do not use that function to initialize the PICs may + /// map this interrupt to a different IDT vector. + /// + /// [`Controller::enable_hardware_interrupts`]: super::Controller::enable_hardware_interrupts pub const PIC_PIT_TIMER: usize = Self::PIC_BIG_START; + + /// PIC PS/2 interrupt vector mapped by + /// [`Controller::enable_hardware_interrupts`]. + /// + /// Systems which do not use that function to initialize the PICs may + /// map this interrupt to a different IDT vector. + /// + /// [`Controller::enable_hardware_interrupts`]: super::Controller::enable_hardware_interrupts pub const PIC_PS2_KEYBOARD: usize = Self::PIC_BIG_START + 1; - pub(super) const LOCAL_APIC_TIMER: usize = (Self::NUM_VECTORS - 2); - pub(super) const LOCAL_APIC_SPURIOUS: usize = (Self::NUM_VECTORS - 1); - pub(super) const PIC_BIG_START: usize = 0x20; - pub(super) const PIC_LITTLE_START: usize = 0x28; + /// Local APIC timer interrupt vector mapped by + /// [`Controller::enable_hardware_interrupts`]. + /// + /// Systems which do not use that function to initialize the local APIC may + /// map this interrupt to a different IDT vector. + /// + /// [`Controller::enable_hardware_interrupts`]: super::Controller::enable_hardware_interrupts + pub const LOCAL_APIC_TIMER: usize = (Self::NUM_VECTORS - 2); + + /// Local APIC spurious interrupt vector mapped by + /// [`Controller::enable_hardware_interrupts`]. + /// + /// Systems which do not use that function to initialize the local APIC may + /// map this interrupt to a different IDT vector. + /// + /// [`Controller::enable_hardware_interrupts`]: super::Controller::enable_hardware_interrupts + pub const LOCAL_APIC_SPURIOUS: usize = (Self::NUM_VECTORS - 1); + + /// Base of the primary PIC's interrupt vector region mapped by + /// [`Controller::enable_hardware_interrupts`]. + /// + /// Systems which do not use that function to initialize the PICs may + /// map this interrupt to a different IDT vector. + /// + /// [`Controller::enable_hardware_interrupts`]: super::Controller::enable_hardware_interrupts + pub const PIC_BIG_START: usize = 0x20; + + /// Base of the secondary PIC's interrupt vector region mapped by + /// [`Controller::enable_hardware_interrupts`]. + /// + /// Systems which do not use that function to initialize the PICs may + /// map this interrupt to a different IDT vector. + /// + /// [`Controller::enable_hardware_interrupts`]: super::Controller::enable_hardware_interrupts + pub const PIC_LITTLE_START: usize = 0x28; // put the IOAPIC right after the PICs - pub(super) const IOAPIC_START: usize = 0x30; - pub(super) const IOAPIC_PIT_TIMER: usize = Self::IOAPIC_START + IoApic::PIT_TIMER_IRQ as usize; - pub(super) const IOAPIC_PS2_KEYBOARD: usize = - Self::IOAPIC_START + IoApic::PS2_KEYBOARD_IRQ as usize; + + /// Base of the IOAPIC's interrupt vector region mapped by + /// [`Controller::enable_hardware_interrupts`]. + /// + /// Systems which do not use that function to initialize the IOAPIC may + /// map this interrupt to a different IDT vector. + /// + /// [`Controller::enable_hardware_interrupts`]: super::Controller::enable_hardware_interrupts + pub const IOAPIC_START: usize = 0x30; + + /// IOAPIC Programmable Interval Timer (PIT) interrupt vector region mapped + /// by [`Controller::enable_hardware_interrupts`]. + /// + /// Systems which do not use that function to initialize the IOAPIC may + /// map this interrupt to a different IDT vector. + /// + /// [`Controller::enable_hardware_interrupts`]: super::Controller::enable_hardware_interrupts + pub const IOAPIC_PIT_TIMER: usize = Self::IOAPIC_START + IoApic::PIT_TIMER_IRQ as usize; + + /// IOAPIC PS/2 keyboard interrupt vector mapped by + /// [`Controller::enable_hardware_interrupts`]. + /// + /// Systems which do not use that function to initialize the IOAPIC may + /// map this interrupt to a different IDT vector. + /// + /// [`Controller::enable_hardware_interrupts`]: super::Controller::enable_hardware_interrupts + pub const IOAPIC_PS2_KEYBOARD: usize = Self::IOAPIC_START + IoApic::PS2_KEYBOARD_IRQ as usize; pub const fn new() -> Self { Self { @@ -174,7 +243,7 @@ impl Idt { } } - pub(super) fn set_isr(&mut self, vector: usize, isr: *const ()) { + pub fn register_isr(&mut self, vector: usize, isr: *const ()) { let descr = self.descriptors[vector].set_handler(isr); if vector == Self::DOUBLE_FAULT { descr.set_ist_offset(Self::DOUBLE_FAULT_IST_OFFSET as u8); From 68a8cd2573b4ca4f26881363746c03ed862e9f3e Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Sat, 28 Dec 2024 20:32:38 -0800 Subject: [PATCH 03/16] chore(ci): remove mycelium-util loom tests turns out we don't need those anymore and the CI job now fails about it. --- .github/workflows/ci.yml | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 334fb16a..1daea575 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -301,27 +301,6 @@ jobs: - name: run Miri tests run: just miri maitake - ### mycelium-util ### - - # run loom tests - util_loom: - needs: changed_paths - if: needs.changed_paths.outputs.should_skip != 'true' || !fromJSON(needs.changed_paths.outputs.paths_result).util.should_skip - runs-on: ubuntu-latest - name: Loom tests (mycelium-util) - steps: - - name: install rust toolchain - run: rustup show - - uses: actions/checkout@v4 - - name: install Just - uses: extractions/setup-just@v1 - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - - name: install nextest - uses: taiki-e/install-action@nextest - - name: run loom tests - run: just loom mycelium-util - # dummy job that depends on all required checks all_systems_go: needs: @@ -337,7 +316,6 @@ jobs: - maitake_no_default_features - maitake_loom - maitake_miri - - util_loom runs-on: ubuntu-latest steps: - run: exit 0 From 2cefeb1d7767641db980fd33496df5f7cc91ee29 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Sat, 28 Dec 2024 20:55:43 -0800 Subject: [PATCH 04/16] feat(x86_64): real abstraction for ISA IRQs, unfuck I/O APIC (#490) 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. --- hal-x86_64/src/interrupt.rs | 189 +++++++++++++++------- hal-x86_64/src/interrupt/apic.rs | 2 +- hal-x86_64/src/interrupt/apic/io.rs | 240 +++++++++++++++++++++++++--- hal-x86_64/src/interrupt/idt.rs | 61 ++----- hal-x86_64/src/interrupt/pic.rs | 47 +++++- 5 files changed, 411 insertions(+), 128 deletions(-) diff --git a/hal-x86_64/src/interrupt.rs b/hal-x86_64/src/interrupt.rs index 5bb7a521..17200169 100644 --- a/hal-x86_64/src/interrupt.rs +++ b/hal-x86_64/src/interrupt.rs @@ -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}; @@ -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; @@ -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, + io: apic::IoApicSet, }, } @@ -134,6 +131,67 @@ pub struct Registers { static IDT: Mutex = Mutex::new_with_raw_mutex(idt::Idt::new(), Spinlock::new()); static INTERRUPT_CONTROLLER: InitOnce = 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; @@ -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, @@ -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"); @@ -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() { @@ -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 } @@ -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); } } @@ -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); } } @@ -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:: as *const ()); - self.register_isr(Self::IOAPIC_PIT_TIMER, pit_timer_isr:: as *const ()); - self.register_isr(Self::PIC_PS2_KEYBOARD, keyboard_isr:: as *const ()); - self.register_isr(Self::IOAPIC_PS2_KEYBOARD, keyboard_isr:: as *const ()); - // local APIC specific hardware itnerrupts + self.register_isa_isr(IsaInterrupt::PitTimer, pit_timer_isr:: as *const ()); + self.register_isa_isr(IsaInterrupt::Ps2Keyboard, keyboard_isr:: 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:: as *const ()); diff --git a/hal-x86_64/src/interrupt/apic.rs b/hal-x86_64/src/interrupt/apic.rs index 54687809..3c15b998 100644 --- a/hal-x86_64/src/interrupt/apic.rs +++ b/hal-x86_64/src/interrupt/apic.rs @@ -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; diff --git a/hal-x86_64/src/interrupt/apic/io.rs b/hal-x86_64/src/interrupt/apic/io.rs index 3c1b28b9..0a0426df 100644 --- a/hal-x86_64/src/interrupt/apic/io.rs +++ b/hal-x86_64/src/interrupt/apic/io.rs @@ -1,12 +1,22 @@ use super::{PinPolarity, TriggerMode}; use crate::{ cpu::FeatureNotSupported, + interrupt::IsaInterrupt, mm::{self, page, size::Size4Kb, PhysPage, VirtPage}, }; use hal_core::PAddr; -use mycelium_util::bits::{bitfield, enum_from_bits}; +use mycelium_util::{ + bits::{bitfield, enum_from_bits}, + sync::blocking::Mutex, +}; use volatile::Volatile; +#[derive(Debug)] +pub struct IoApicSet { + ioapics: alloc::vec::Vec>, + isa_map: [IsaOverride; 16], +} + #[derive(Debug)] #[must_use] pub struct IoApic { @@ -81,11 +91,219 @@ struct MmioRegisters { data: u32, } +#[derive(Copy, Clone, Debug)] +struct IsaOverride { + apic: u8, + vec: u8, +} + +// === impl IoApicSet === + +impl IoApicSet { + pub fn new( + madt: &acpi::platform::interrupt::Apic, + frame_alloc: &impl hal_core::mem::page::Alloc, + pagectrl: &mut crate::mm::PageCtrl, + isa_base: u8, + ) -> Self { + // The ACPI Multiple APIC Descriptor Table (MADT) tells us where to find + // the I/O APICs, as well as information about how the ISA standard + // interrupts are routed to I/O APIC inputs on this system. + // + // See: https://wiki.osdev.org/MADT + + // There may be multiple I/O APICs in the system, so we'll collect them + // all into a `Vec`. We're also going to build a table of how the ISA + // standard interrupts are mapped across those I/O APICs, so we can look + // up which one a particular interrupt lives on. + let n_ioapics = madt.io_apics.len(); + tracing::trace!(?madt.io_apics, "found {n_ioapics} IO APICs", ); + assert_ne!( + n_ioapics, 0, + "why would a computer have the APIC interrupt model but not \ + have any IO APICs???" + ); + let mut this = IoApicSet { + ioapics: alloc::vec::Vec::with_capacity(n_ioapics), + isa_map: [IsaOverride { apic: 0, vec: 0 }; 16], + }; + + for (n, ioapic) in madt.io_apics.iter().enumerate() { + let addr = PAddr::from_u64(ioapic.address as u64); + tracing::debug!(ioapic.paddr = ?addr, "IOAPIC {n}"); + this.ioapics + .push(Mutex::new(IoApic::new(addr, pagectrl, frame_alloc))); + } + + // Okay, so here's where it gets ~*weird*~. + // + // On the Platonic Ideal Normal Computer, the ISA PC interrupts would be + // mapped to I/O APIC input pins by number, so ISA IRQ 0 would go to pin + // 0 on I/O APIC 0, and so on. + // + // However, motherboard manufacturers can do whatever they like when + // routing these interrupts, so they could go to different pins, + // potentially on different I/O APICs (if the system has more than one). + // Also, some of these interrupts might have different pin polarity or + // edge/level-triggered-iness than what we might expect. + // + // Fortunately, ACPI is here to help (statements dreamed up by the + // utterly deranged). + // + // The MADT includes a list of "interrupt source overrides" that + // describe any ISA interrupts that are not mapped to I/O APIC pins in + // numeric order. Each entry in the interrupt source overrides list will + // contain: + // - an ISA IRQ number (naturally), + // - the "global system interrupt", which is NOT the IDT vector for that + // interrupt but instead the I/O APIC input pin number that the IRQ is + // routed to, + // - the polarity and trigger mode of the interrupt pin, if these are + // different from the default bus trigger mode. + // + // For each of the 16 ISA interrupt vectors, we'll configure the + // redirection entry in the appropriate I/O APIC and record which I/O + // APIC (and which pin on that I/O APIC) the interrupt is routed to. We + // do this by first checking if the MADT contains an override matching + // that ISA interrupt, using that if so, and if not, falling back to the + // ISA interrupt number. + // + // Yes, this is a big pile of nested loops. But, consider the following: + // + // - the MADT override entries can probably come in any order, if the + // motherboard firmware chooses to be maximally perverse, so we have + // to scan the whole list of them to find out if each ISA interrupt is + // overridden. + // - there are only ever 16 ISA interrupts, so the outer loop iterates + // exactly 16 times; and there can't be *more* overrides than there + // are ISA interrupts (and there's generally substantially fewer). + // similarly, if there's more than 1-8 I/O APICs, you probably have + // some kind of really weird computer and should tell me about it + // because i bet it's awesome. + // so, neither inner loop actually loops that many times. + // - finally, we only do this once on boot, so who cares? + + let base_entry = RedirectionEntry::new() + .with(RedirectionEntry::DELIVERY, DeliveryMode::Normal) + .with(RedirectionEntry::REMOTE_IRR, false) + .with(RedirectionEntry::MASKED, true) + .with(RedirectionEntry::DESTINATION, 0xff); + for irq in IsaInterrupt::ALL { + // Assume the IRQ is mapped to the I/O APIC pin corresponding to + // that ISA IRQ number, and is active-high and edge-triggered. + let mut global_system_interrupt = irq as u8; + let mut polarity = PinPolarity::High; + let mut trigger = TriggerMode::Edge; + // Is there an override for this IRQ? If there is, clobber the + // assumed defaults with "whatever the override says". + if let Some(src_override) = madt + .interrupt_source_overrides + .iter() + .find(|o| o.isa_source == irq as u8) + { + // put the defaults through an extruder that maybe messes with + // them. + use acpi::platform::interrupt::{ + Polarity as AcpiPolarity, TriggerMode as AcpiTriggerMode, + }; + tracing::debug!( + ?irq, + ?src_override.global_system_interrupt, + ?src_override.polarity, + ?src_override.trigger_mode, + "ISA interrupt {irq:?} is overridden by MADT" + ); + match src_override.polarity { + AcpiPolarity::ActiveHigh => polarity = PinPolarity::High, + AcpiPolarity::ActiveLow => polarity = PinPolarity::Low, + // TODO(eliza): if the MADT override entry says that the pin + // polarity is "same as bus", we should probably actually + // make it be the same as the bus, instead of just assuming + // that it's active high. But...just assuming that "same as + // bus" means active high seems to basically work so far... + AcpiPolarity::SameAsBus => {} + } + match src_override.trigger_mode { + AcpiTriggerMode::Edge => trigger = TriggerMode::Edge, + AcpiTriggerMode::Level => trigger = TriggerMode::Level, + // TODO(eliza): As above, when the MADT says this is "same + // as bus", we should make it be the same as the bus instead + // of going "ITS EDGE TRIGGERED LOL LMAO" which is what + // we're currently doing. But, just like above, this Seems + // To Work? + AcpiTriggerMode::SameAsBus => {} + } + global_system_interrupt = src_override.global_system_interrupt.try_into().expect( + "if this exceeds u8::MAX, what the fuck! \ + that's bigger than the entire IDT...", + ); + } + // Now, scan to find which I/O APIC this IRQ corresponds to. if the + // system only has one I/O APIC, this will always be 0, but we gotta + // handle systems with more than one. So, we'll this by traversing + // the list of I/O APICs, seeing if the GSI number is less than the + // max number of IRQs handled by that I/O APIC, and if it is, we'll + // stick it in there. If not, keep searching for the next one, + // subtracting the max number of interrupts handled by the I/O APIC + // we just looked at. + let mut entry_idx = global_system_interrupt; + let mut got_him = false; + 'apic_scan: for (apic_idx, apic) in this.ioapics.iter_mut().enumerate() { + let apic = apic.get_mut(); + let max_entries = apic.max_entries(); + if entry_idx > max_entries { + entry_idx -= max_entries; + continue; + } + + // Ladies and gentlemen...we got him! + got_him = true; + tracing::debug!( + ?irq, + ?global_system_interrupt, + ?apic_idx, + ?entry_idx, + "found IOAPIC for ISA interrupt" + ); + let entry = base_entry + .with(RedirectionEntry::POLARITY, polarity) + .with(RedirectionEntry::TRIGGER, trigger) + .with(RedirectionEntry::VECTOR, isa_base + irq as u8); + apic.set_entry(entry_idx, entry); + this.isa_map[irq as usize] = IsaOverride { + apic: apic_idx as u8, + vec: entry_idx, + }; + break 'apic_scan; + } + + assert!( + got_him, + "somehow, we didn't find an I/O APIC for MADT global system \ + interrupt {global_system_interrupt} (ISA IRQ {irq:?})!\n \ + this probably means the MADT is corrupted somehow, or maybe \ + your motherboard is just super weird? i have no idea what to \ + do in this situation, so i guess i'll die." + ); + } + + this + } + + fn for_isa_irq(&self, irq: IsaInterrupt) -> (&Mutex, u8) { + let isa_override = self.isa_map[irq as usize]; + (&self.ioapics[isa_override.apic as usize], isa_override.vec) + } + + pub fn set_isa_masked(&self, irq: IsaInterrupt, masked: bool) { + let (ioapic, vec) = self.for_isa_irq(irq); + ioapic.with_lock(|ioapic| ioapic.set_masked(vec, masked)); + } +} + // === impl IoApic === impl IoApic { - pub(crate) const PS2_KEYBOARD_IRQ: u8 = 0x1; - pub(crate) const PIT_TIMER_IRQ: u8 = 0x2; const REDIRECTION_ENTRY_BASE: u32 = 0x10; /// Try to construct an `IoApic`. @@ -150,22 +368,6 @@ impl IoApic { Self::try_new(addr, pagectrl, frame_alloc).unwrap() } - /// Map all ISA interrupts starting at `base`. - #[tracing::instrument(level = tracing::Level::DEBUG, skip(self))] - pub fn map_isa_irqs(&mut self, base: u8) { - let flags = RedirectionEntry::new() - .with(RedirectionEntry::DELIVERY, DeliveryMode::Normal) - .with(RedirectionEntry::POLARITY, PinPolarity::High) - .with(RedirectionEntry::REMOTE_IRR, false) - .with(RedirectionEntry::TRIGGER, TriggerMode::Edge) - .with(RedirectionEntry::MASKED, true) - .with(RedirectionEntry::DESTINATION, 0xff); - for irq in 0..16 { - let entry = flags.with(RedirectionEntry::VECTOR, base + irq); - self.set_entry(irq, entry); - } - } - /// Returns the IO APIC's ID. #[must_use] pub fn id(&mut self) -> u8 { diff --git a/hal-x86_64/src/interrupt/idt.rs b/hal-x86_64/src/interrupt/idt.rs index 116a1646..73acdf92 100644 --- a/hal-x86_64/src/interrupt/idt.rs +++ b/hal-x86_64/src/interrupt/idt.rs @@ -1,4 +1,4 @@ -use super::apic::IoApic; +use super::IsaInterrupt; use crate::{cpu, segment}; use mycelium_util::{bits, fmt}; @@ -155,23 +155,10 @@ impl Idt { /// Chosen by fair die roll, guaranteed to be random. pub const DOUBLE_FAULT_IST_OFFSET: usize = 4; - /// PIC Programmable Interval Timer (PIT) interrupt vector mapped by - /// [`Controller::enable_hardware_interrupts`]. - /// - /// Systems which do not use that function to initialize the PICs may - /// map this interrupt to a different IDT vector. - /// - /// [`Controller::enable_hardware_interrupts`]: super::Controller::enable_hardware_interrupts - pub const PIC_PIT_TIMER: usize = Self::PIC_BIG_START; + pub const MAX_CPU_EXCEPTION: usize = Self::SECURITY_EXCEPTION; - /// PIC PS/2 interrupt vector mapped by - /// [`Controller::enable_hardware_interrupts`]. - /// - /// Systems which do not use that function to initialize the PICs may - /// map this interrupt to a different IDT vector. - /// - /// [`Controller::enable_hardware_interrupts`]: super::Controller::enable_hardware_interrupts - pub const PIC_PS2_KEYBOARD: usize = Self::PIC_BIG_START + 1; + /// Base offset for ISA hardware interrupts. + pub const ISA_BASE: usize = 0x20; /// Local APIC timer interrupt vector mapped by /// [`Controller::enable_hardware_interrupts`]. @@ -198,7 +185,7 @@ impl Idt { /// map this interrupt to a different IDT vector. /// /// [`Controller::enable_hardware_interrupts`]: super::Controller::enable_hardware_interrupts - pub const PIC_BIG_START: usize = 0x20; + pub const PIC_BIG_START: usize = Self::ISA_BASE; /// Base of the secondary PIC's interrupt vector region mapped by /// [`Controller::enable_hardware_interrupts`]. @@ -207,48 +194,28 @@ impl Idt { /// map this interrupt to a different IDT vector. /// /// [`Controller::enable_hardware_interrupts`]: super::Controller::enable_hardware_interrupts - pub const PIC_LITTLE_START: usize = 0x28; + pub const PIC_LITTLE_START: usize = Self::ISA_BASE + 8; // put the IOAPIC right after the PICs - /// Base of the IOAPIC's interrupt vector region mapped by - /// [`Controller::enable_hardware_interrupts`]. - /// - /// Systems which do not use that function to initialize the IOAPIC may - /// map this interrupt to a different IDT vector. - /// - /// [`Controller::enable_hardware_interrupts`]: super::Controller::enable_hardware_interrupts - pub const IOAPIC_START: usize = 0x30; - - /// IOAPIC Programmable Interval Timer (PIT) interrupt vector region mapped - /// by [`Controller::enable_hardware_interrupts`]. - /// - /// Systems which do not use that function to initialize the IOAPIC may - /// map this interrupt to a different IDT vector. - /// - /// [`Controller::enable_hardware_interrupts`]: super::Controller::enable_hardware_interrupts - pub const IOAPIC_PIT_TIMER: usize = Self::IOAPIC_START + IoApic::PIT_TIMER_IRQ as usize; - - /// IOAPIC PS/2 keyboard interrupt vector mapped by - /// [`Controller::enable_hardware_interrupts`]. - /// - /// Systems which do not use that function to initialize the IOAPIC may - /// map this interrupt to a different IDT vector. - /// - /// [`Controller::enable_hardware_interrupts`]: super::Controller::enable_hardware_interrupts - pub const IOAPIC_PS2_KEYBOARD: usize = Self::IOAPIC_START + IoApic::PS2_KEYBOARD_IRQ as usize; - pub const fn new() -> Self { Self { descriptors: [Descriptor::null(); Self::NUM_VECTORS], } } + /// Register an interrupt service routine (ISR) for the given ISA standard + /// PC interrupt. + pub fn register_isa_isr(&mut self, irq: IsaInterrupt, isr: *const ()) { + let vector = irq as usize + Self::ISA_BASE; + self.register_isr(vector, isr); + } + pub fn register_isr(&mut self, vector: usize, isr: *const ()) { let descr = self.descriptors[vector].set_handler(isr); if vector == Self::DOUBLE_FAULT { descr.set_ist_offset(Self::DOUBLE_FAULT_IST_OFFSET as u8); } - tracing::debug!(vector, ?isr, ?descr, "set isr"); + tracing::debug!(vector, ?isr, ?descr, "set ISR"); } pub fn load(&'static self) { diff --git a/hal-x86_64/src/interrupt/pic.rs b/hal-x86_64/src/interrupt/pic.rs index da37c42f..e61150a0 100644 --- a/hal-x86_64/src/interrupt/pic.rs +++ b/hal-x86_64/src/interrupt/pic.rs @@ -1,3 +1,4 @@ +use super::IsaInterrupt; use crate::cpu; use hal_core::interrupt::{Handlers, RegistrationError}; @@ -16,6 +17,24 @@ impl Pic { data: cpu::Port::at(data), } } + + /// Mask the provided interrupt number. + unsafe fn mask(&mut self, num: u8) { + debug_assert!(num < 8); + // read the current value of the Interrupt Mask Register (IMR). + let imr = self.data.readb(); + // set the bit corresponding to the interrupt number to 1. + self.data.writeb(imr | (1 << num)); + } + + /// Unmask the provided interrupt number. + unsafe fn unmask(&mut self, num: u8) { + debug_assert!(num < 8); + // read the current value of the Interrupt Mask Register (IMR). + let imr = self.data.readb(); + // clear the bit corresponding to the interrupt number. + self.data.writeb(imr & !(1 << num)); + } } #[derive(Debug)] @@ -50,9 +69,33 @@ impl CascadedPic { } } - pub(crate) fn end_interrupt(&mut self, num: u8) { + pub(crate) fn mask(&mut self, irq: IsaInterrupt) { + let (pic, num) = self.pic_for_irq(irq); + unsafe { + pic.mask(num); + } + } + + pub(crate) fn unmask(&mut self, irq: IsaInterrupt) { + let (pic, num) = self.pic_for_irq(irq); + unsafe { + pic.unmask(num); + } + } + + fn pic_for_irq(&mut self, irq: IsaInterrupt) -> (&mut Pic, u8) { + let num = irq as u8; + if num >= 8 { + (&mut self.sisters.little, num - 8) + } else { + (&mut self.sisters.big, num) + } + } + + pub(crate) fn end_interrupt(&mut self, irq: IsaInterrupt) { const END_INTERRUPT: u8 = 0x20; // from osdev wiki - if num >= self.sisters.little.address && num < self.sisters.little.address + 8 { + let num = irq as u8; + if num >= 8 { unsafe { self.sisters.little.command.writeb(END_INTERRUPT); } From a6f9ba0b986f37dc12ebb320cb6168de56370c45 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Sun, 29 Dec 2024 09:31:24 -0800 Subject: [PATCH 05/16] chore(nix): remove unnecessary LD_LIBRARY_PATH --- default.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/default.nix b/default.nix index faf941c4..5bbf29d0 100644 --- a/default.nix +++ b/default.nix @@ -28,6 +28,6 @@ pkgs.buildEnv { CURL_CA_BUNDLE = "${cacert}/etc/ca-bundle.crt"; CARGO_TERM_COLOR = "always"; RUST_BACKTRACE = "full"; - LD_LIBRARY_PATH = "${lib.makeLibraryPath [ zlib ]}"; + # LD_LIBRARY_PATH = "${lib.makeLibraryPath [ zlib ]}"; }; } From 8c2cc1042118fcee97bea0274463c21e1392e607 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Sun, 29 Dec 2024 11:37:40 -0800 Subject: [PATCH 06/16] feat(x86_64): debug commands to print CPUID & MSRs (#492) This commit adds kernel debug shell commands `dump arch msr` and dump arch cpuid`, to print the value of x86 model-specific registers and CPUID leaves, respectively. I also added a bunch of argument-parsing utilities to the kernel debug shell. Closes #491 ![image](https://github.com/user-attachments/assets/815118d5-90f1-4a6f-ac76-30290f870cac) --- src/arch/x86_64/shell.rs | 73 +++++++++++++- src/shell.rs | 200 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 260 insertions(+), 13 deletions(-) diff --git a/src/arch/x86_64/shell.rs b/src/arch/x86_64/shell.rs index e7f94500..25d531a5 100644 --- a/src/arch/x86_64/shell.rs +++ b/src/arch/x86_64/shell.rs @@ -1,4 +1,5 @@ -use crate::shell::Command; +use crate::shell::{Command, NumberFormat}; +use mycelium_util::fmt; pub const DUMP_ARCH: Command = Command::new("arch") .with_help("dump architecture-specific structures") @@ -17,4 +18,74 @@ pub const DUMP_ARCH: Command = Command::new("arch") tracing::info!(IDT = ?idt); Ok(()) }), + Command::new("msr") + .with_help( + "print the value of the specified model-specific register (MSR)\n \ + MSR_NUM: the MSR number in hexadecimal or binary\n \ + -f, --fmt : format the value of the MSR in hexadecimal, \ + decimal, or binary.", + ) + .with_usage("[-f|--fmt] ") + .with_fn(|mut ctx| { + let fmt = ctx + .parse_optional_flag::(&["-f", "--fmt"])? + .unwrap_or(NumberFormat::Hex); + let num = ctx.parse_u32_hex_or_dec("")?; + + let msr = hal_x86_64::cpu::msr::Msr::try_new(num).ok_or_else(|| { + ctx.other_error( + "CPU does not support model-specific registers (must be pre-pentium...)", + ) + })?; + + let val = msr.read_raw(); + match fmt { + NumberFormat::Binary => tracing::info!("MSR {num:#x} = {val:#b}"), + NumberFormat::Decimal => tracing::info!("MSR {num:#x} = {val}"), + NumberFormat::Hex => tracing::info!("MSR {num:#x} = {val:#x}"), + } + Ok(()) + }), + Command::new("cpuid") + .with_help( + "print the value of the specified CPUID leaf (and subleaf)\n \ + LEAF: the CPUID leaf number in hexadecimal or binary\n \ + SUBLEAF: the CPUID subleaf number in hexadecimal or binary\n \ + -f, --fmt : format the values of the CPUID registers in hexadecimal, \ + decimal, or binary.", + ) + .with_usage("[-f|--fmt] [SUBLEAF]") + .with_fn(|mut ctx| { + use core::arch::x86_64::{CpuidResult, __cpuid_count}; + let fmt = ctx + .parse_optional_flag::(&["-f", "--fmt"])? + .unwrap_or(NumberFormat::Hex); + let leaf = ctx.parse_u32_hex_or_dec("")?; + let subleaf = ctx.parse_optional_u32_hex_or_dec("[SUBLEAF]")?.unwrap_or(0); + + let CpuidResult { eax, ebx, ecx, edx } = unsafe { __cpuid_count(leaf, subleaf) }; + match fmt { + NumberFormat::Binary => tracing::info!( + target: "shell", + eax = fmt::bin(eax), + ebx = fmt::bin(ebx), + ecx = fmt::bin(ecx), + edx = fmt::bin(edx), + "CPUID {leaf:#x}:{subleaf:x}", + ), + NumberFormat::Decimal => tracing::info!( + target: "shell", eax, ebx, ecx, edx, + "CPUID {leaf:#x}:{subleaf:x}", + ), + NumberFormat::Hex => tracing::info!( + target: "shell", + eax = fmt::hex(eax), + ebx = fmt::hex(ebx), + ecx = fmt::hex(ecx), + edx = fmt::hex(edx), + "CPUID {leaf:#x}:{subleaf:x}", + ), + } + Ok(()) + }), ]); diff --git a/src/shell.rs b/src/shell.rs index 5c34bd00..286bc814 100644 --- a/src/shell.rs +++ b/src/shell.rs @@ -2,6 +2,7 @@ //! purposes. //! use crate::rt; +use core::str::FromStr; use mycelium_util::fmt::{self, Write}; /// Defines a shell command, including its name, help text, and how the command @@ -21,10 +22,10 @@ pub struct Error<'a> { kind: ErrorKind<'a>, } -pub type Result<'a> = core::result::Result<(), Error<'a>>; +pub type CmdResult<'a> = core::result::Result<(), Error<'a>>; pub trait Run: Send + Sync { - fn run<'ctx>(&'ctx self, ctx: Context<'ctx>) -> Result<'ctx>; + fn run<'ctx>(&'ctx self, ctx: Context<'ctx>) -> CmdResult<'ctx>; } #[derive(Debug)] @@ -32,12 +33,19 @@ enum ErrorKind<'a> { UnknownCommand(&'a [Command<'a>]), SubcommandRequired(&'a [Command<'a>]), - InvalidArguments { help: &'a str, arg: &'a str }, + InvalidArguments { + help: &'a str, + arg: &'a str, + flag: Option<&'a str>, + }, + FlagRequired { + flags: &'a [&'a str], + }, Other(&'static str), } enum RunKind<'a> { - Fn(fn(Context<'_>) -> Result<'_>), + Fn(fn(Context<'_>) -> CmdResult<'_>), Runnable(&'a dyn Run), } @@ -75,7 +83,7 @@ pub struct Context<'cmd> { current: &'cmd str, } -pub fn handle_command<'cmd>(ctx: Context<'cmd>, commands: &'cmd [Command]) -> Result<'cmd> { +pub fn handle_command<'cmd>(ctx: Context<'cmd>, commands: &'cmd [Command]) -> CmdResult<'cmd> { let chunk = ctx.current.trim(); for cmd in commands { if let Some(current) = chunk.strip_prefix(cmd.name) { @@ -310,7 +318,7 @@ impl<'cmd> Command<'cmd> { /// If [`Command::with_fn`] or [`Command::with_runnable`] was previously /// called, this overwrites the previously set value. #[must_use] - pub const fn with_fn(self, func: fn(Context<'_>) -> Result<'_>) -> Self { + pub const fn with_fn(self, func: fn(Context<'_>) -> CmdResult<'_>) -> Self { Self { run: Some(RunKind::Fn(func)), ..self @@ -332,7 +340,7 @@ impl<'cmd> Command<'cmd> { } /// Run this command in the provided [`Context`]. - pub fn run<'ctx>(&'cmd self, ctx: Context<'ctx>) -> Result<'ctx> + pub fn run<'ctx>(&'cmd self, ctx: Context<'ctx>) -> CmdResult<'ctx> where 'cmd: 'ctx, { @@ -414,6 +422,17 @@ impl fmt::Display for Error<'_> { .chain(core::iter::once("help")) } + fn fmt_flag_names(f: &mut fmt::Formatter<'_>, flags: &[&str]) -> fmt::Result { + let mut names = flags.iter(); + if let Some(name) = names.next() { + f.write_str(name)?; + for name in names { + write!(f, "|{name}")?; + } + } + Ok(()) + } + let Self { line, kind } = self; match kind { ErrorKind::UnknownCommand(commands) => { @@ -421,8 +440,12 @@ impl fmt::Display for Error<'_> { fmt::comma_delimited(&mut f, command_names(commands))?; f.write_char(']')?; } - ErrorKind::InvalidArguments { arg, help } => { - write!(f, "invalid argument {arg:?}: {help}")? + ErrorKind::InvalidArguments { arg, help, flag } => { + f.write_str("invalid argument")?; + if let Some(flag) = flag { + write!(f, " {flag}")?; + } + write!(f, " {arg:?}: {help}")?; } ErrorKind::SubcommandRequired(subcommands) => { writeln!( @@ -432,6 +455,11 @@ impl fmt::Display for Error<'_> { fmt::comma_delimited(&mut f, command_names(subcommands))?; f.write_char(']')?; } + ErrorKind::FlagRequired { flags } => { + write!(f, "the '{line}' command requires the ")?; + fmt_flag_names(f, flags)?; + write!(f, " flag")?; + } ErrorKind::Other(msg) => write!(f, "could not execute {line:?}: {msg}")?, } @@ -472,6 +500,18 @@ impl<'cmd> Context<'cmd> { line: self.line, kind: ErrorKind::InvalidArguments { arg: self.current, + flag: None, + help, + }, + } + } + + pub fn invalid_argument_named(&self, name: &'static str, help: &'static str) -> Error<'cmd> { + Error { + line: self.line, + kind: ErrorKind::InvalidArguments { + arg: self.current, + flag: Some(name), help, }, } @@ -483,13 +523,130 @@ impl<'cmd> Context<'cmd> { kind: ErrorKind::Other(msg), } } + + pub fn parse_bool_flag(&mut self, flag: &str) -> bool { + if let Some(rest) = self.command().trim().strip_prefix(flag) { + self.current = rest.trim(); + true + } else { + false + } + } + + pub fn parse_optional_u32_hex_or_dec( + &mut self, + name: &'static str, + ) -> Result, Error<'cmd>> { + let (chunk, rest) = match self.command().split_once(" ") { + Some((chunk, rest)) => (chunk.trim(), rest), + None => (self.command(), ""), + }; + + if chunk.is_empty() { + return Ok(None); + } + + let val = if let Some(hex_num) = chunk.strip_prefix("0x") { + u32::from_str_radix(hex_num.trim(), 16).map_err(|_| Error { + line: self.line, + kind: ErrorKind::InvalidArguments { + arg: chunk, + flag: Some(name), + help: "expected a 32-bit hex number", + }, + })? + } else { + u32::from_str(chunk).map_err(|_| Error { + line: self.line, + kind: ErrorKind::InvalidArguments { + arg: chunk, + flag: Some(name), + help: "expected a 32-bit decimal number", + }, + })? + }; + + self.current = rest; + Ok(Some(val)) + } + + pub fn parse_u32_hex_or_dec(&mut self, name: &'static str) -> Result> { + self.parse_optional_u32_hex_or_dec(name).and_then(|val| { + val.ok_or_else(|| self.invalid_argument_named(name, "expected a number")) + }) + } + + pub fn parse_optional_flag( + &mut self, + names: &'static [&'static str], + ) -> Result, Error<'cmd>> + where + T: FromStr, + T::Err: core::fmt::Display, + { + for name in names { + if let Some(rest) = self.command().strip_prefix(name) { + let (chunk, rest) = match rest.trim().split_once(" ") { + Some((chunk, rest)) => (chunk.trim(), rest), + None => (rest, ""), + }; + + if chunk.is_empty() { + return Err(Error { + line: self.line, + kind: ErrorKind::InvalidArguments { + arg: chunk, + flag: Some(name), + help: "expected a value", + }, + }); + } + + match chunk.parse() { + Ok(val) => { + self.current = rest; + return Ok(Some(val)); + } + Err(e) => { + tracing::warn!(target: "shell", "invalid value {chunk:?} for flag {name}: {e}"); + return Err(Error { + line: self.line, + kind: ErrorKind::InvalidArguments { + arg: chunk, + flag: Some(name), + help: "invalid value", + }, + }); + } + } + } + } + + Ok(None) + } + + pub fn parse_required_flag( + &mut self, + names: &'static [&'static str], + ) -> Result> + where + T: FromStr, + T::Err: core::fmt::Display, + { + self.parse_optional_flag(names).and_then(|val| { + val.ok_or_else(|| Error { + line: self.line, + kind: ErrorKind::FlagRequired { flags: names }, + }) + }) + } } // === impl RunKind === impl RunKind<'_> { #[inline] - fn run<'ctx>(&'ctx self, ctx: Context<'ctx>) -> Result<'ctx> { + fn run<'ctx>(&'ctx self, ctx: Context<'ctx>) -> CmdResult<'ctx> { match self { Self::Fn(func) => func(ctx), Self::Runnable(runnable) => runnable.run(ctx), @@ -513,9 +670,9 @@ impl fmt::Debug for RunKind<'_> { impl Run for F where - F: Fn(Context<'_>) -> Result<'_> + Send + Sync, + F: Fn(Context<'_>) -> CmdResult<'_> + Send + Sync, { - fn run<'ctx>(&'ctx self, ctx: Context<'ctx>) -> Result<'ctx> { + fn run<'ctx>(&'ctx self, ctx: Context<'ctx>) -> CmdResult<'ctx> { self(ctx) } } @@ -527,3 +684,22 @@ fn print_help(parent_cmd: &str, commands: &[Command]) { } tracing::info!(target: "shell", " {parent_cmd}{parent_cmd_pad}help --- prints this help message"); } + +#[derive(Copy, Clone, Debug, Eq, PartialEq)] +pub enum NumberFormat { + Binary, + Hex, + Decimal, +} + +impl FromStr for NumberFormat { + type Err = &'static str; + fn from_str(s: &str) -> Result { + match s.trim() { + "b" | "bin" | "binary" => Ok(Self::Binary), + "h" | "hex" => Ok(Self::Hex), + "d" | "dec" | "decimal" => Ok(Self::Decimal), + _ => Err("expected one of: [b, bin, binary, h, hex, d, decimal]"), + } + } +} From c4183db5e6dcf55b3474790d298edfd842204788 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Sun, 29 Dec 2024 12:35:38 -0800 Subject: [PATCH 07/16] fix(hal_x86_64): remove tracing in timer ISR this makes the kernel deadlock because im a fucking idiot --- hal-x86_64/src/interrupt.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/hal-x86_64/src/interrupt.rs b/hal-x86_64/src/interrupt.rs index 17200169..c4cba185 100644 --- a/hal-x86_64/src/interrupt.rs +++ b/hal-x86_64/src/interrupt.rs @@ -479,8 +479,6 @@ impl hal_core::interrupt::Control for Idt { extern "x86-interrupt" fn pit_timer_isr>(_regs: Registers) { if crate::time::Pit::handle_interrupt() { H::timer_tick() - } else { - tracing::trace!("PIT sleep completed"); } unsafe { INTERRUPT_CONTROLLER From e23a00d4afb9f3154add50812ec83afb1799a485 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 30 Dec 2024 10:01:39 -0800 Subject: [PATCH 08/16] feat(hal_x86_64): be less stupid about ISR tracing (#496) 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 { From 05c7aabb326e0e8163b7a44515a5d789d06c80ef Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 30 Dec 2024 09:43:48 -0800 Subject: [PATCH 09/16] refac(hal_x86_64): rename `apic::io` to `ioapic` (#497) I don't like that the module is just called "io", that feels weird. --- hal-x86_64/src/interrupt/apic.rs | 4 ++-- hal-x86_64/src/interrupt/apic/{io.rs => ioapic.rs} | 0 2 files changed, 2 insertions(+), 2 deletions(-) rename hal-x86_64/src/interrupt/apic/{io.rs => ioapic.rs} (100%) diff --git a/hal-x86_64/src/interrupt/apic.rs b/hal-x86_64/src/interrupt/apic.rs index 3c15b998..846a5613 100644 --- a/hal-x86_64/src/interrupt/apic.rs +++ b/hal-x86_64/src/interrupt/apic.rs @@ -1,7 +1,7 @@ //! Advanced Programmable Interrupt Controller (APIC). -pub mod io; +pub mod ioapic; pub mod local; -pub use io::{IoApic, IoApicSet}; +pub use ioapic::{IoApic, IoApicSet}; pub use local::LocalApic; use mycelium_util::bits::enum_from_bits; diff --git a/hal-x86_64/src/interrupt/apic/io.rs b/hal-x86_64/src/interrupt/apic/ioapic.rs similarity index 100% rename from hal-x86_64/src/interrupt/apic/io.rs rename to hal-x86_64/src/interrupt/apic/ioapic.rs From b0d028fbf13f8c746e1af8ce2b1eeddadfec9945 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 30 Dec 2024 10:02:24 -0800 Subject: [PATCH 10/16] refac(hal_x86_64): define ISRs at the top level (#497) Currently, all the interrupt service routines provided by the HAL are defined inside of the `Idt::register_handlers` method. I did this initially because they're only referenced inside that method. However, this has the unfortunate side effect of making the *debug symbols* for ISRs quite unpleasant, because `register_handlers` is generic. In GDB, the symbol name for the PIT timer ISR is ```rust hal_x86_64::interrupt::::register_handlers::pit_timer_isr ```` For an example, consider [this stack trace][1], which is horrible. So, if you want to set a breakpoint on that ISR, you have to type all of that, and also remember that this is the symbol name for the ISR, and not something normal. This is annoying. Therefore, this commit moves the ISR definitions out of this method and into a top-level `hal_x86_64::interrupt::isr` module. Now, the names of these symbols should be more like this: ```rust hal_x86_64::interrupt::isr::pit_timer ``` Which seems a *lot* less unpleasant. There should be no actual functional change here besides getting nicer debug symbols. [1]: https://github.com/hawkw/mycelium/commit/c4183db5e6dcf55b3474790d298edfd842204788#commitcomment-150799709 --- hal-x86_64/src/interrupt.rs | 469 +++++++++++++++++++----------------- 1 file changed, 243 insertions(+), 226 deletions(-) diff --git a/hal-x86_64/src/interrupt.rs b/hal-x86_64/src/interrupt.rs index 23350ed8..a4eaf012 100644 --- a/hal-x86_64/src/interrupt.rs +++ b/hal-x86_64/src/interrupt.rs @@ -423,255 +423,54 @@ impl hal_core::interrupt::Control for Idt { where H: Handlers, { - macro_rules! gen_code_faults { - ($self:ident, $h:ty, $($vector:path => fn $name:ident($($rest:tt)+),)+) => { - $( - gen_code_faults! {@ $name($($rest)+); } - $self.register_isr($vector, $name::<$h> as *const ()); - )+ - }; - (@ $name:ident($kind:literal);) => { - extern "x86-interrupt" fn $name>(mut registers: Registers) { - let code = CodeFault { - error_code: None, - kind: $kind, - }; - H::code_fault(Context { registers: &mut registers, code }); - } - }; - (@ $name:ident($kind:literal, code);) => { - extern "x86-interrupt" fn $name>( - mut registers: Registers, - code: u64, - ) { - let code = CodeFault { - error_code: Some(&code), - kind: $kind, - }; - H::code_fault(Context { registers: &mut registers, code }); - } - }; - } - let span = tracing::debug_span!("Idt::register_handlers"); let _enter = span.enter(); - extern "x86-interrupt" fn page_fault_isr>( - mut registers: Registers, - code: PageFaultCode, - ) { - H::page_fault(Context { - registers: &mut registers, - code, - }); - } - - extern "x86-interrupt" fn double_fault_isr>( - mut registers: Registers, - code: u64, - ) { - H::double_fault(Context { - registers: &mut registers, - code, - }); - } - - extern "x86-interrupt" fn pit_timer_isr>(_regs: Registers) { - if crate::time::Pit::handle_interrupt() { - H::timer_tick() - } - unsafe { - INTERRUPT_CONTROLLER - .get_unchecked() - .end_isa_irq(IsaInterrupt::PitTimer); - } - } - - extern "x86-interrupt" fn apic_timer_isr>(_regs: Registers) { - H::timer_tick(); - unsafe { - match INTERRUPT_CONTROLLER.get_unchecked().model { - InterruptModel::Pic(_) => unreachable!(), - InterruptModel::Apic { ref local, .. } => local.end_interrupt(), - } - } - } - - extern "x86-interrupt" fn keyboard_isr>(_regs: Registers) { - // 0x60 is a magic PC/AT number. - static PORT: cpu::Port = cpu::Port::at(0x60); - // load-bearing read - if we don't read from the keyboard controller it won't - // send another interrupt on later keystrokes. - let scancode = unsafe { PORT.readb() }; - H::ps2_keyboard(scancode); - unsafe { - INTERRUPT_CONTROLLER - .get_unchecked() - .end_isa_irq(IsaInterrupt::Ps2Keyboard); - } - } - - extern "x86-interrupt" fn test_isr>(mut registers: Registers) { - H::test_interrupt(Context { - registers: &mut registers, - code: (), - }); - } - - extern "x86-interrupt" fn invalid_tss_isr>( - mut registers: Registers, - code: u64, - ) { - unsafe { - // 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!"); - - let msg = selector.named("task-state segment (TSS)"); - let code = CodeFault { - error_code: Some(&msg), - kind: "Invalid TSS (0xA)", - }; - H::code_fault(Context { - registers: &mut registers, - code, - }); - } - - extern "x86-interrupt" fn segment_not_present_isr>( - mut registers: Registers, - code: u64, - ) { - unsafe { - // 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!"); - - let msg = selector.named("stack segment"); - let code = CodeFault { - error_code: Some(&msg), - kind: "Segment Not Present (0xB)", - }; - H::code_fault(Context { - registers: &mut registers, - code, - }); - } - - extern "x86-interrupt" fn stack_segment_isr>( - mut registers: Registers, - code: u64, - ) { - unsafe { - // 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"); - - let msg = selector.named("stack segment"); - let code = CodeFault { - error_code: Some(&msg), - kind: "Stack-Segment Fault (0xC)", - }; - H::code_fault(Context { - registers: &mut registers, - code, - }); - } - - extern "x86-interrupt" fn gpf_isr>( - mut registers: Registers, - code: u64, - ) { - unsafe { - // 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 { - Some(SelectorErrorCode(code as u16)) - } else { - None - }; - - tracing::error!(?segment, "lmao, a general protection fault is happening"); - let error_code = segment.map(|seg| seg.named("selector")); - let code = CodeFault { - error_code: error_code.as_ref().map(|code| code as &dyn fmt::Display), - kind: "General Protection Fault (0xD)", - }; - H::code_fault(Context { - registers: &mut registers, - code, - }); - } - - extern "x86-interrupt" fn spurious_isr() { - // TODO(eliza): do we need to actually do something here? - } - // === exceptions === // these exceptions are mapped to the HAL `Handlers` trait's "code // fault" handler, and indicate that the code that was executing did a // Bad Thing - gen_code_faults! { - self, H, - Self::DIVIDE_BY_ZERO => fn div_0_isr("Divide-By-Zero (0x0)"), - Self::OVERFLOW => fn overflow_isr("Overflow (0x4)"), - Self::BOUND_RANGE_EXCEEDED => fn br_isr("Bound Range Exceeded (0x5)"), - Self::INVALID_OPCODE => fn ud_isr("Invalid Opcode (0x6)"), - Self::DEVICE_NOT_AVAILABLE => fn no_fpu_isr("Device (FPU) Not Available (0x7)"), - Self::ALIGNMENT_CHECK => fn alignment_check_isr("Alignment Check (0x11)", code), - Self::SIMD_FLOATING_POINT => fn simd_fp_exn_isr("SIMD Floating-Point Exception (0x13)"), - Self::X87_FPU_EXCEPTION => fn x87_exn_isr("x87 Floating-Point Exception (0x10)"), - } + self.register_isr(Self::DIVIDE_BY_ZERO, isr::div_0:: as *const ()); + self.register_isr(Self::OVERFLOW, isr::overflow:: as *const ()); + self.register_isr(Self::BOUND_RANGE_EXCEEDED, isr::br:: as *const ()); + self.register_isr(Self::INVALID_OPCODE, isr::ud:: as *const ()); + self.register_isr(Self::DEVICE_NOT_AVAILABLE, isr::no_fpu:: as *const ()); + self.register_isr( + Self::ALIGNMENT_CHECK, + isr::alignment_check:: as *const (), + ); + self.register_isr( + Self::SIMD_FLOATING_POINT, + isr::simd_fp_exn:: as *const (), + ); + self.register_isr(Self::X87_FPU_EXCEPTION, isr::x87_exn:: as *const ()); // other exceptions, not mapped to the "code fault" handler - self.register_isr(Self::PAGE_FAULT, page_fault_isr:: as *const ()); - self.register_isr(Self::INVALID_TSS, invalid_tss_isr:: as *const ()); + self.register_isr(Self::PAGE_FAULT, isr::page_fault:: as *const ()); + self.register_isr(Self::INVALID_TSS, isr::invalid_tss:: as *const ()); self.register_isr( Self::SEGMENT_NOT_PRESENT, - segment_not_present_isr:: as *const (), + isr::segment_not_present:: as *const (), ); self.register_isr( Self::STACK_SEGMENT_FAULT, - stack_segment_isr:: as *const (), + isr::stack_segment:: as *const (), ); - self.register_isr(Self::GENERAL_PROTECTION_FAULT, gpf_isr:: as *const ()); - self.register_isr(Self::DOUBLE_FAULT, double_fault_isr:: as *const ()); + self.register_isr(Self::GENERAL_PROTECTION_FAULT, isr::gpf:: as *const ()); + self.register_isr(Self::DOUBLE_FAULT, isr::double_fault:: as *const ()); // === hardware interrupts === // ISA standard hardware interrupts mapped on both the PICs and IO APIC // interrupt models. - self.register_isa_isr(IsaInterrupt::PitTimer, pit_timer_isr:: as *const ()); - self.register_isa_isr(IsaInterrupt::Ps2Keyboard, keyboard_isr:: as *const ()); + self.register_isa_isr(IsaInterrupt::PitTimer, isr::pit_timer:: as *const ()); + self.register_isa_isr(IsaInterrupt::Ps2Keyboard, isr::keyboard:: 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:: as *const ()); + self.register_isr(Self::LOCAL_APIC_SPURIOUS, isr::spurious as *const ()); + self.register_isr(Self::LOCAL_APIC_TIMER, isr::apic_timer:: as *const ()); // vector 69 (nice) is reserved by the HAL for testing the IDT. - self.register_isr(69, test_isr:: as *const ()); + self.register_isr(69, isr::test:: as *const ()); Ok(()) } @@ -772,6 +571,224 @@ impl fmt::Display for NamedSelectorErrorCode { } } +mod isr { + use super::*; + + macro_rules! gen_code_faults { + ($(fn $name:ident($($rest:tt)+),)+) => { + $( + gen_code_faults! {@ $name($($rest)+); } + )+ + }; + (@ $name:ident($kind:literal);) => { + pub(super) extern "x86-interrupt" fn $name>(mut registers: Registers) { + let code = CodeFault { + error_code: None, + kind: $kind, + }; + H::code_fault(Context { registers: &mut registers, code }); + } + }; + (@ $name:ident($kind:literal, code);) => { + pub(super) extern "x86-interrupt" fn $name>( + mut registers: Registers, + code: u64, + ) { + let code = CodeFault { + error_code: Some(&code), + kind: $kind, + }; + H::code_fault(Context { registers: &mut registers, code }); + } + }; + } + + gen_code_faults! { + fn div_0("Divide-By-Zero (0x0)"), + fn overflow("Overflow (0x4)"), + fn br("Bound Range Exceeded (0x5)"), + fn ud("Invalid Opcode (0x6)"), + fn no_fpu("Device (FPU) Not Available (0x7)"), + fn alignment_check("Alignment Check (0x11)", code), + fn simd_fp_exn("SIMD Floating-Point Exception (0x13)"), + fn x87_exn("x87 Floating-Point Exception (0x10)"), + } + + pub(super) extern "x86-interrupt" fn page_fault>( + mut registers: Registers, + code: PageFaultCode, + ) { + H::page_fault(Context { + registers: &mut registers, + code, + }); + } + + pub(super) extern "x86-interrupt" fn double_fault>( + mut registers: Registers, + code: u64, + ) { + H::double_fault(Context { + registers: &mut registers, + code, + }); + } + + pub(super) extern "x86-interrupt" fn pit_timer>(_regs: Registers) { + if crate::time::Pit::handle_interrupt() { + H::timer_tick() + } + unsafe { + INTERRUPT_CONTROLLER + .get_unchecked() + .end_isa_irq(IsaInterrupt::PitTimer); + } + } + + pub(super) extern "x86-interrupt" fn apic_timer>(_regs: Registers) { + H::timer_tick(); + unsafe { + match INTERRUPT_CONTROLLER.get_unchecked().model { + InterruptModel::Pic(_) => unreachable!(), + InterruptModel::Apic { ref local, .. } => local.end_interrupt(), + } + } + } + + pub(super) extern "x86-interrupt" fn keyboard>(_regs: Registers) { + // 0x60 is a magic PC/AT number. + static PORT: cpu::Port = cpu::Port::at(0x60); + // load-bearing read - if we don't read from the keyboard controller it won't + // send another interrupt on later keystrokes. + let scancode = unsafe { PORT.readb() }; + H::ps2_keyboard(scancode); + unsafe { + INTERRUPT_CONTROLLER + .get_unchecked() + .end_isa_irq(IsaInterrupt::Ps2Keyboard); + } + } + + pub(super) extern "x86-interrupt" fn test>(mut registers: Registers) { + H::test_interrupt(Context { + registers: &mut registers, + code: (), + }); + } + + pub(super) extern "x86-interrupt" fn invalid_tss>( + mut registers: Registers, + code: u64, + ) { + unsafe { + // 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!"); + + let msg = selector.named("task-state segment (TSS)"); + let code = CodeFault { + error_code: Some(&msg), + kind: "Invalid TSS (0xA)", + }; + H::code_fault(Context { + registers: &mut registers, + code, + }); + } + + pub(super) extern "x86-interrupt" fn segment_not_present>( + mut registers: Registers, + code: u64, + ) { + unsafe { + // 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!"); + + let msg = selector.named("stack segment"); + let code = CodeFault { + error_code: Some(&msg), + kind: "Segment Not Present (0xB)", + }; + H::code_fault(Context { + registers: &mut registers, + code, + }); + } + + pub(super) extern "x86-interrupt" fn stack_segment>( + mut registers: Registers, + code: u64, + ) { + unsafe { + // 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"); + + let msg = selector.named("stack segment"); + let code = CodeFault { + error_code: Some(&msg), + kind: "Stack-Segment Fault (0xC)", + }; + H::code_fault(Context { + registers: &mut registers, + code, + }); + } + + pub(super) extern "x86-interrupt" fn gpf>( + mut registers: Registers, + code: u64, + ) { + unsafe { + // 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 { + Some(SelectorErrorCode(code as u16)) + } else { + None + }; + + tracing::error!(?segment, "lmao, a general protection fault is happening"); + let error_code = segment.map(|seg| seg.named("selector")); + let code = CodeFault { + error_code: error_code.as_ref().map(|code| code as &dyn fmt::Display), + kind: "General Protection Fault (0xD)", + }; + H::code_fault(Context { + registers: &mut registers, + code, + }); + } + + pub(super) extern "x86-interrupt" fn spurious() { + // TODO(eliza): do we need to actually do something here? + } +} + #[cfg(test)] mod tests { use super::*; From 7d5dd73bbd5c792729882115c0ead0a5414bc96b Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 31 Dec 2024 10:06:51 -0800 Subject: [PATCH 11/16] feat(util): add `Deferred` (#499) This commit adds a `Deferred` utility to `mycelium-util` for making drop guards. I wanted to use this to add a quick "`cli` at the top of a scope and `sti` when exiting the scope" utility in the `hal-x86_64` interrupt module. --- util/src/deferred.rs | 51 ++++++++++++++++++++++++++++++++++++++++++++ util/src/lib.rs | 2 ++ 2 files changed, 53 insertions(+) create mode 100644 util/src/deferred.rs diff --git a/util/src/deferred.rs b/util/src/deferred.rs new file mode 100644 index 00000000..b2b689a3 --- /dev/null +++ b/util/src/deferred.rs @@ -0,0 +1,51 @@ +//! Deferred closure execution (a.k.a. scope guards). +//! +//! See the [`Deferred`] type and the [`defer`] function for more information. + +/// Defers execution of a closure until a scope is exited. +/// +/// As seen in "The Go Programming Language".. +pub struct Deferred(Option); + +impl Deferred { + /// Defer execution of `f` until this scope is exited. + #[inline] + pub const fn new(f: F) -> Self { + Self(Some(f)) + } + + /// Cancel the deferred execution.of the closure passed to `defer`. + /// + /// Calling this method will prevent the closure from being executed when + /// this `Deferred` guard is dropped. + #[inline] + pub fn cancel(&mut self) { + self.0 = None; + } +} + +impl Drop for Deferred { + #[inline] + fn drop(&mut self) { + if let Some(f) = self.0.take() { + f() + } + } +} + +impl core::fmt::Debug for Deferred { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + f.debug_tuple("Deferred") + .field(&format_args!("{}", core::any::type_name::())) + .finish() + } +} + +/// Defer execution of `f` until this scope is exited. +/// +/// This is equivalent to calling `Deferred::new(f)`. See [`Deferred`] for more +/// details. +#[inline(always)] +pub fn defer(f: F) -> Deferred { + Deferred::new(f) +} diff --git a/util/src/lib.rs b/util/src/lib.rs index 24b61dee..f30ca142 100644 --- a/util/src/lib.rs +++ b/util/src/lib.rs @@ -9,6 +9,8 @@ extern crate alloc; #[macro_use] mod macros; +pub use deferred::defer; +pub mod deferred; pub mod error; pub mod fmt; pub mod io; From 555862ad4444ab54eac65ab3f17e77ba784cd6e8 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 31 Dec 2024 10:34:44 -0800 Subject: [PATCH 12/16] feat(hal-x86_64): put local APIC in local storage (#499) As described in #495, each CPU core in a SMP x86 system has its own local APIC. Currently, the mycelium HAL's interrupt code will just only ever know about the boot processor's local APIC. This is wrong. This commit reworks this to use the `%GS` local storage mechanism to store the state of the local APIC separately for each CPU core. I've added code for initializing the local APIC state on non-boot processors, which we will call when we bring up other cores. Fixes #495 --- hal-x86_64/src/interrupt.rs | 77 ++++++++++++++++++++++---- hal-x86_64/src/interrupt/apic/local.rs | 55 +++++++++++++++++- util/src/deferred.rs | 4 +- 3 files changed, 123 insertions(+), 13 deletions(-) diff --git a/hal-x86_64/src/interrupt.rs b/hal-x86_64/src/interrupt.rs index a4eaf012..43cfadf7 100644 --- a/hal-x86_64/src/interrupt.rs +++ b/hal-x86_64/src/interrupt.rs @@ -2,6 +2,7 @@ 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}; +use hal_core::mem::page; use mycelium_util::{ bits, fmt, sync::{ @@ -44,7 +45,8 @@ pub type Isr = extern "x86-interrupt" fn(&mut Context); #[derive(Debug)] pub enum PeriodicTimerError { Pit(time::PitError), - Apic(time::InvalidDuration), + InvalidDuration(time::InvalidDuration), + Apic(apic::local::LocalApicError), } #[derive(Debug)] @@ -68,8 +70,8 @@ enum InterruptModel { /// [I/O]: apic::IoApic /// [apics]: apic Apic { - local: apic::LocalApic, io: apic::IoApicSet, + local: apic::local::Handle, }, } @@ -192,6 +194,16 @@ impl IsaInterrupt { ]; } +#[must_use] +fn disable_scoped() -> impl Drop + Send + Sync { + unsafe { + crate::cpu::intrinsics::cli(); + } + mycelium_util::defer(|| unsafe { + crate::cpu::intrinsics::sti(); + }) +} + impl Controller { // const DEFAULT_IOAPIC_BASE_PADDR: u64 = 0xFEC00000; @@ -224,6 +236,36 @@ impl Controller { } } + fn local_apic_handle(&self) -> Result<&apic::local::Handle, apic::local::LocalApicError> { + match self.model { + InterruptModel::Pic(_) => Err(apic::local::LocalApicError::NoApic), + InterruptModel::Apic { ref local, .. } => Ok(local), + } + } + + pub fn with_local_apic( + &self, + f: impl FnOnce(&LocalApic) -> T, + ) -> Result { + self.local_apic_handle()?.with(f) + } + + /// This should *not* be called by the boot processor + pub fn initialize_local_apic( + &self, + frame_alloc: &A, + pagectrl: &mut impl page::Map, + ) -> Result<(), apic::local::LocalApicError> + where + A: page::Alloc, + { + let _deferred = disable_scoped(); + let hdl = self.local_apic_handle()?; + unsafe { + hdl.initialize(frame_alloc, pagectrl, Idt::LOCAL_APIC_SPURIOUS as u8); + } + Ok(()) + } /// # Safety /// /// Calling this when there isn't actually an ISA interrupt pending can do @@ -231,13 +273,14 @@ impl Controller { 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(), + InterruptModel::Apic { ref local, .. } => local.with(|apic| unsafe { apic.end_interrupt() }) + .expect("interrupts should not be handled on this core until the local APIC is initialized") } } pub fn enable_hardware_interrupts( acpi: Option<&acpi::InterruptModel>, - frame_alloc: &impl hal_core::mem::page::Alloc, + frame_alloc: &impl page::Alloc, ) -> &'static Self { let mut pics = pic::CascadedPic::new(); // regardless of whether APIC or PIC interrupt handling will be used, @@ -267,13 +310,16 @@ impl Controller { // 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); + // configure and initialize the local APIC on the boot processor + let local = apic::local::Handle::new(); + unsafe { + local.initialize(frame_alloc, &mut pagectrl, Idt::LOCAL_APIC_SPURIOUS as u8); + } let model = InterruptModel::Apic { local, io }; tracing::trace!(interrupt_model = ?model); + INTERRUPT_CONTROLLER.init(Self { model }) } model => { @@ -334,8 +380,11 @@ impl Controller { .start_periodic_timer(interval) .map_err(PeriodicTimerError::Pit), InterruptModel::Apic { ref local, .. } => local - .start_periodic_timer(interval, Idt::LOCAL_APIC_TIMER as u8) - .map_err(PeriodicTimerError::Apic), + .with(|apic| { + apic.start_periodic_timer(interval, Idt::LOCAL_APIC_TIMER as u8) + .map_err(PeriodicTimerError::InvalidDuration) + }) + .map_err(PeriodicTimerError::Apic)?, } } } @@ -650,7 +699,15 @@ mod isr { unsafe { match INTERRUPT_CONTROLLER.get_unchecked().model { InterruptModel::Pic(_) => unreachable!(), - InterruptModel::Apic { ref local, .. } => local.end_interrupt(), + InterruptModel::Apic { ref local, .. } => { + match local.with(|apic| apic.end_interrupt()) { + Ok(_) => {} + Err(e) => unreachable!( + "the local APIC timer will not have fired if the \ + local APIC is uninitialized on this core! {e:?}", + ), + } + } } } } diff --git a/hal-x86_64/src/interrupt/apic/local.rs b/hal-x86_64/src/interrupt/apic/local.rs index c5c3d18d..516e35df 100644 --- a/hal-x86_64/src/interrupt/apic/local.rs +++ b/hal-x86_64/src/interrupt/apic/local.rs @@ -1,10 +1,10 @@ use super::{PinPolarity, TriggerMode}; use crate::{ - cpu::{FeatureNotSupported, Msr}, + cpu::{local, FeatureNotSupported, Msr}, mm::{self, page, size::Size4Kb, PhysPage, VirtPage}, time::{Duration, InvalidDuration}, }; -use core::{convert::TryInto, marker::PhantomData, num::NonZeroU32}; +use core::{cell::RefCell, convert::TryInto, marker::PhantomData, num::NonZeroU32}; use hal_core::{PAddr, VAddr}; use mycelium_util::fmt; use raw_cpuid::CpuId; @@ -24,6 +24,9 @@ pub struct LocalApicRegister { _ty: PhantomData, } +#[derive(Debug)] +pub(in crate::interrupt) struct Handle(local::LocalKey>>); + pub trait RegisterAccess { type Access; type Target; @@ -32,6 +35,16 @@ pub trait RegisterAccess { ) -> Volatile<&'static mut Self::Target, Self::Access>; } +#[derive(Debug, Eq, PartialEq)] +pub enum LocalApicError { + /// The system is configured to use the PIC interrupt model rather than the + /// APIC interrupt model. + NoApic, + + /// The local APIC is uninitialized. + Uninitialized, +} + impl LocalApic { const BASE_PADDR_MASK: u64 = 0xffff_ffff_f000; @@ -293,6 +306,44 @@ impl LocalApic { } } +impl Handle { + pub(in crate::interrupt) const fn new() -> Self { + Self(local::LocalKey::new(|| RefCell::new(None))) + } + + pub(in crate::interrupt) fn with( + &self, + f: impl FnOnce(&LocalApic) -> T, + ) -> Result { + self.0.with(|apic| { + Ok(f(apic + .borrow() + .as_ref() + .ok_or(LocalApicError::Uninitialized)?)) + }) + } + + pub(in crate::interrupt) unsafe fn initialize( + &self, + frame_alloc: &A, + pagectrl: &mut impl page::Map, + spurious_vector: u8, + ) where + A: page::Alloc, + { + self.0.with(|slot| { + let mut slot = slot.borrow_mut(); + if slot.is_some() { + // already initialized, bail. + return; + } + let apic = LocalApic::new(pagectrl, frame_alloc); + apic.enable(spurious_vector); + *slot = Some(apic); + }) + } +} + pub mod register { use super::*; use mycelium_util::bits::{bitfield, enum_from_bits}; diff --git a/util/src/deferred.rs b/util/src/deferred.rs index b2b689a3..73bdf157 100644 --- a/util/src/deferred.rs +++ b/util/src/deferred.rs @@ -4,7 +4,9 @@ /// Defers execution of a closure until a scope is exited. /// -/// As seen in "The Go Programming Language".. +/// As seen in "The Go Programming Language". +#[must_use = "dropping a `Deferred` guard immediately executes \ + the deferred function"] pub struct Deferred(Option); impl Deferred { From 4909f6f1482f699b2727887e2da8d084780d7968 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 31 Dec 2024 10:40:11 -0800 Subject: [PATCH 13/16] fix(hal-x86_64): don't log every MSR read (#499) Currently, we have some code for reading MSRs that emits a tracing event for every `rdmsr` instruction you execute. This was a stupid thing to do, and I only did it because I was being excessively cutesy. Now that we read the `FS_GS_BASE` MSR in ISRs to access local data, this can deadlock the kernel :upside_down: So, this commit removes it. --- hal-x86_64/src/cpu/msr.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/hal-x86_64/src/cpu/msr.rs b/hal-x86_64/src/cpu/msr.rs index 69547ed5..abfb8126 100644 --- a/hal-x86_64/src/cpu/msr.rs +++ b/hal-x86_64/src/cpu/msr.rs @@ -281,9 +281,7 @@ impl Msr { options(nomem, nostack, preserves_flags) ); } - let result = (hi as u64) << 32 | (lo as u64); - tracing::trace!(rdmsr = %self, value = fmt::hex(result)); - result + (hi as u64) << 32 | (lo as u64) } /// Writes the given raw `u64` value to this MSR. From 0363e82476f528feae5c9929bbdcde717ff1b439 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 31 Dec 2024 11:06:38 -0800 Subject: [PATCH 14/16] fix(hal-x86_64): fix wrong LAPIC frequency calc (#499) 1 Hz = 1 second. 1 ms = 1/1000 second. So we should multiply here instead of dividing, lol. The divide accidentally makes us consider the LAPIC frequency to be 0, and then we just "don't ever have a timer". Whoopsie. --- hal-x86_64/src/interrupt/apic/local.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/hal-x86_64/src/interrupt/apic/local.rs b/hal-x86_64/src/interrupt/apic/local.rs index 516e35df..386340cf 100644 --- a/hal-x86_64/src/interrupt/apic/local.rs +++ b/hal-x86_64/src/interrupt/apic/local.rs @@ -132,15 +132,15 @@ impl LocalApic { let cpuid = CpuId::new(); - if let Some(undivided_freq_khz) = cpuid.get_hypervisor_info().and_then(|hypervisor| { + if let Some(freq_khz) = cpuid.get_hypervisor_info().and_then(|hypervisor| { tracing::trace!("CPUID contains hypervisor info"); let freq = hypervisor.apic_frequency(); - tracing::trace!(hypervisor.apic_frequency = ?freq); + tracing::trace!(hypervisor.apic_frequency_khz = ?freq); NonZeroU32::new(freq?) }) { // the hypervisor info CPUID leaf expresses the frequency in kHz, // and the frequency is not divided by the target timer divisor. - let frequency_hz = undivided_freq_khz.get() / 1000 / Self::TIMER_DIVISOR; + let frequency_hz = (freq_khz.get() * 1000) / Self::TIMER_DIVISOR; tracing::debug!( frequency_hz, "determined APIC frequency from CPUID hypervisor info" @@ -148,6 +148,8 @@ impl LocalApic { return frequency_hz; } + // XXX ELIZA THIS IS TSC FREQUENCY, SO IDK IF THAT'S RIGHT? + /* if let Some(undivided_freq_hz) = cpuid.get_tsc_info().and_then(|tsc| { tracing::trace!("CPUID contains TSC info"); let freq = tsc.nominal_frequency(); @@ -161,6 +163,7 @@ impl LocalApic { ); return frequency_hz; } + */ // CPUID didn't help, so fall back to calibrating the APIC frequency // using the PIT. From d9dac1df29d5074bc88c10a3b80405d373cde6bb Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 31 Dec 2024 11:57:46 -0800 Subject: [PATCH 15/16] feat(hal-x86_64): crosscheck PIT/CPUID LAPIC freq (#499) This commit changes the local APIC calibration code to attempt PIT calibration even when we find CPUID leaves indicating the frequency. This way, we can cross-check them and see how sloppy the PIT calibration is. It turns out its pretty bad. --- hal-x86_64/src/interrupt/apic/local.rs | 134 +++++++++++++++++-------- 1 file changed, 91 insertions(+), 43 deletions(-) diff --git a/hal-x86_64/src/interrupt/apic/local.rs b/hal-x86_64/src/interrupt/apic/local.rs index 386340cf..f16733b8 100644 --- a/hal-x86_64/src/interrupt/apic/local.rs +++ b/hal-x86_64/src/interrupt/apic/local.rs @@ -127,11 +127,64 @@ impl LocalApic { tracing::info!(base = ?self.base, spurious_vector, "local APIC enabled"); } - fn timer_frequency_hz(&self) -> u32 { + /// Calibrate the timer frequency using the PIT. + #[inline(always)] + fn calibrate_frequency_hz_pit(&self) -> u32 { use register::*; + tracing::debug!("calibrating APIC timer frequency using PIT..."); - let cpuid = CpuId::new(); + // lock the PIT now, before actually starting the timer IRQ, so that we + // don't include any time spent waiting for the PIT lock. + // + // since we only run this code on startup, before any other cores have + // been started, this probably never actually waits for a lock. but...we + // should do it the right way, anyway. + let mut pit = crate::time::PIT.lock(); + + unsafe { + // start the timer + + // set timer divisor to 16 + self.write_register(TIMER_DIVISOR, 0b11); + self.write_register( + LVT_TIMER, + LvtTimer::new() + .with(LvtTimer::MODE, TimerMode::OneShot) + .with(LvtTimer::MASKED, false), + ); + // set initial count to -1 + self.write_register(TIMER_INITIAL_COUNT, -1i32 as u32); + } + + // use the PIT to sleep for 10ms + pit.sleep_blocking(Duration::from_millis(10)) + .expect("the PIT should be able to send a 10ms interrupt..."); + + unsafe { + // stop the timer + self.write_register( + LVT_TIMER, + LvtTimer::new() + .with(LvtTimer::MODE, TimerMode::OneShot) + .with(LvtTimer::MASKED, true), + ); + } + + let elapsed_ticks = unsafe { self.register(TIMER_CURRENT_COUNT).read() }; + // since we slept for ten milliseconds, each tick is 10 kHz. we don't + // need to account for the divisor since we ran the timer at that + // divisor already. + let ticks_per_10ms = (-1i32 as u32).wrapping_sub(elapsed_ticks); + tracing::debug!(?ticks_per_10ms); + // convert the frequency to Hz. + let frequency_hz = ticks_per_10ms * 100; + tracing::debug!(frequency_hz, "calibrated local APIC timer using PIT"); + frequency_hz + } + #[inline(always)] + fn calibrate_frequency_hz_cpuid() -> Option { + let cpuid = CpuId::new(); if let Some(freq_khz) = cpuid.get_hypervisor_info().and_then(|hypervisor| { tracing::trace!("CPUID contains hypervisor info"); let freq = hypervisor.apic_frequency(); @@ -143,9 +196,9 @@ impl LocalApic { let frequency_hz = (freq_khz.get() * 1000) / Self::TIMER_DIVISOR; tracing::debug!( frequency_hz, - "determined APIC frequency from CPUID hypervisor info" + "determined APIC timer frequency from CPUID hypervisor info" ); - return frequency_hz; + return Some(frequency_hz); } // XXX ELIZA THIS IS TSC FREQUENCY, SO IDK IF THAT'S RIGHT? @@ -167,51 +220,46 @@ impl LocalApic { // CPUID didn't help, so fall back to calibrating the APIC frequency // using the PIT. - tracing::debug!("calibrating APIC timer frequency using PIT..."); - - // lock the PIT now, before actually starting the timer IRQ, so that we - // don't include any time spent waiting for the PIT lock. - // - // since we only run this code on startup, before any other cores have - // been started, this probably never actually waits for a lock. but...we - // should do it the right way, anyway. - let mut pit = crate::time::PIT.lock(); - - unsafe { - // set timer divisor to 16 - self.write_register(TIMER_DIVISOR, 0b11); - // set initial count to -1 - self.write_register(TIMER_INITIAL_COUNT, -1i32 as u32); + None + } - // start the timer - self.write_register( - LVT_TIMER, - LvtTimer::new().with(LvtTimer::MODE, TimerMode::OneShot), + fn timer_frequency_hz(&self) -> u32 { + // How sloppy do we expect the PIT frequency calibration to be? + // If the delta between the CPUID frequency and the frequency we + // determined using PIT calibration is > this value, we'll yell about + // it. + const PIT_SLOPPINESS: u32 = 1000; + + // Start out by calibrating the APIC frequency using the PIT. + let pit_frequency_hz = self.calibrate_frequency_hz_pit(); + + // See if we can get something from CPUID. + let Some(cpuid_frequency_hz) = Self::calibrate_frequency_hz_cpuid() else { + tracing::info!( + pit.frequency_hz = pit_frequency_hz, + "CPUID does not indicate APIC timer frequency; using PIT \ + calibration only" ); - } - - // use the PIT to sleep for 10ms - pit.sleep_blocking(Duration::from_millis(10)) - .expect("the PIT should be able to send a 10ms interrupt..."); + return pit_frequency_hz; + }; - unsafe { - // stop the timer - self.write_register( - LVT_TIMER, - LvtTimer::new().with(LvtTimer::MODE, TimerMode::Periodic), + // Cross-check the PIT calibration result and CPUID value. + let distance = if pit_frequency_hz > cpuid_frequency_hz { + pit_frequency_hz - cpuid_frequency_hz + } else { + cpuid_frequency_hz - pit_frequency_hz + }; + if distance > PIT_SLOPPINESS { + tracing::warn!( + pit.frequency_hz = pit_frequency_hz, + cpuid.frequency_hz = cpuid_frequency_hz, + distance, + "APIC timer frequency from PIT calibration differs substantially \ + from CPUID!" ); } - let elapsed_ticks = unsafe { self.register(TIMER_CURRENT_COUNT).read() }; - // since we slept for ten milliseconds, each tick is 10 kHz. we don't - // need to account for the divisor since we ran the timer at that - // divisor already. - let ticks_per_10ms = (-1i32 as u32).wrapping_sub(elapsed_ticks); - tracing::debug!(?ticks_per_10ms); - // convert the frequency to Hz. - let frequency_hz = ticks_per_10ms * 100; - tracing::debug!(frequency_hz, "calibrated local APIC timer using PIT"); - frequency_hz + cpuid_frequency_hz } #[tracing::instrument( From e58cdd151e0dd98f8829e398162907ee512d4a7a Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 31 Dec 2024 11:59:30 -0800 Subject: [PATCH 16/16] fix(hal-x86_64): remove tracing from PIT sleep (#499) The current code for sleeping using the PIT one-shot timer has a bunch of tracing in it. This adds overhead which makes the sleep duration way less accurate, making PIT calibration for the local APIC timer way sloppier. This commit removes the tracing. PIT calibration accuracy still sucks, but it sucks less now. --- hal-x86_64/src/time/pit.rs | 26 +++----------------------- 1 file changed, 3 insertions(+), 23 deletions(-) diff --git a/hal-x86_64/src/time/pit.rs b/hal-x86_64/src/time/pit.rs index df2e5731..6ecaf3a9 100644 --- a/hal-x86_64/src/time/pit.rs +++ b/hal-x86_64/src/time/pit.rs @@ -271,19 +271,14 @@ impl Pit { /// /// [`Controller::init`]: crate::interrupt::Controller::init /// [`interrupt`]: crate::interrupt - #[tracing::instrument( - name = "Pit::sleep_blocking" - level = tracing::Level::DEBUG, - skip(self), - fields(?duration), - err, - )] pub fn sleep_blocking(&mut self, duration: Duration) -> Result<(), PitError> { SLEEPING .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) .map_err(|_| PitError::SleepInProgress)?; self.interrupt_in(duration) .map_err(PitError::InvalidDuration)?; + + // Tracing here is fine, because we are already sleeping... tracing::debug!("started PIT sleep"); // spin until the sleep interrupt fires. @@ -291,11 +286,8 @@ impl Pit { cpu::wait_for_interrupt(); } - tracing::info!(?duration, "slept using PIT channel 0"); - // if we were previously in periodic mode, re-enable it. if let Some(interval) = self.channel0_interval { - tracing::debug!("restarting PIT periodic timer"); self.start_periodic_timer(interval)?; } @@ -366,13 +358,6 @@ impl Pit { /// This configures the PIT in mode 0 (oneshot mode). Once the interrupt has /// fired, in order to use the periodic timer, the pit must be put back into /// periodic mode by calling [`Pit::start_periodic_timer`]. - #[tracing::instrument( - name = "Pit::interrupt_in" - level = tracing::Level::DEBUG, - skip(self), - fields(?duration), - err, - )] pub fn interrupt_in(&mut self, duration: Duration) -> Result<(), InvalidDuration> { let duration_ms = usize::try_from(duration.as_millis()).map_err(|_| { InvalidDuration::new( @@ -388,8 +373,6 @@ impl Pit { ) })?; - tracing::trace!(?duration, duration_ms, target_time, "Pit::interrupt_in"); - let command = Command::new() // use the binary counter .with(Command::BCD_BINARY, false) @@ -410,19 +393,16 @@ impl Pit { } fn set_divisor(&mut self, divisor: u16) { - tracing::trace!(divisor = &fmt::hex(divisor), "Pit::set_divisor"); let low = divisor as u8; let high = (divisor >> 8) as u8; unsafe { self.channel0.writeb(low); // write the low byte - tracing::trace!(lo = &fmt::hex(low), "pit.channel0.writeb(lo)"); self.channel0.writeb(high); // write the high byte - tracing::trace!(hi = &fmt::hex(high), "pit.channel0.writeb(hi)"); } } fn send_command(&self, command: Command) { - tracing::debug!(?command, "Pit::send_command"); + // tracing::debug!(?command, "Pit::send_command"); unsafe { self.command.writeb(command.bits()); }