diff --git a/issues/kernel/a-safe-mmio-window-reaches-any-physical-address.md b/issues/kernel/a-safe-mmio-window-reaches-any-physical-address.md new file mode 100644 index 00000000000..cdec64c0905 --- /dev/null +++ b/issues/kernel/a-safe-mmio-window-reaches-any-physical-address.md @@ -0,0 +1,20 @@ +--- +status: open +kind: defect +opened: 2026-10-01 +--- + +# A safe `Mmio` window reaches any physical address + +`Mmio::new` (`kernel/src/mm/mmio.rs`) is a safe `pub(crate)` constructor over +any `DirectMap`, `DirectMap::from_phys` (`kernel/src/mm/mod.rs`) is safe for +any address, and `Mmio::write_u32` is safe: any kernel module can write any +physical address, RAM included, with no `unsafe` to answer for. So no type +can make a device's registers its driver's alone: a module that builds a +window over `arch::console_uart::frame()` writes the PL011 without the +`serial::Registers` that `arch::console_uart::write_byte` asks for. + +**Evidence:** the code. Its callers are `map_mmio` on both architectures, the +AArch64 GIC redistributor's window and the PL011's. + +**Exit:** no safe call builds a window over memory it was not handed. diff --git a/issues/kernel/a-safe-port-read-reaches-any-io-port.md b/issues/kernel/a-safe-port-read-reaches-any-io-port.md new file mode 100644 index 00000000000..a2b386c14ab --- /dev/null +++ b/issues/kernel/a-safe-port-read-reaches-any-io-port.md @@ -0,0 +1,22 @@ +--- +status: open +kind: defect +opened: 2026-10-01 +--- + +# A safe port read reaches any I/O port + +`inb` and `inw` (`kernel/src/arch/x86_64/cpu.rs`) are safe `pub fn`s over +any port, "safe because a read has no value a caller can get wrong". A port +read has effects of its own: a read of COM1's data register takes the 16550's +received byte, a read of its line status clears the error bits, and a read of +the i8042's data port takes its output byte. So any kernel module takes a +received console byte with `crate::arch::cpu::inb(0x3f8)`, with no +`serial::Registers` and no `unsafe`, past the type that makes the UART's +registers their holder's alone. `outb` and `outw` are `unsafe`, under a +contract that the caller owns the port; the reads carry none. + +**Evidence:** the code. `inb`'s callers are `arch::console_uart`, `arch::rtc` +and `arch::i8042`; `inw`'s is `arch::watchdog`. + +**Exit:** no safe call reads a port it was not handed. diff --git a/issues/kernel/the-aarch64-boot-cpus-id-read-is-held-against-nothing.md b/issues/kernel/the-aarch64-boot-cpus-id-read-is-held-against-nothing.md new file mode 100644 index 00000000000..9ea36cb3ad0 --- /dev/null +++ b/issues/kernel/the-aarch64-boot-cpus-id-read-is-held-against-nothing.md @@ -0,0 +1,23 @@ +--- +status: open +kind: defect +opened: 2026-10-01 +--- + +# The AArch64 boot CPU's id read is held against nothing + +`Roster::commit` (`kernel/src/smp_roster.rs`) holds every AP's `arch::cpu::hardware_id` +against the MADT MPIDR it was started by, and x86-64's `boot_aps` holds the +boot CPU's against its LAPIC id. AArch64's `smp::start` makes the boot CPU's +read its roster slot (`ROSTER.set_bsp(me)`), and `irqchip::init` picks the +boot CPU's redistributor by it, so nothing independent of the read holds it. +A read naming no MADT CPU panics in `Gic::redistributor`. One naming another +CPU brings that CPU's redistributor up as the boot CPU's and skips that CPU, +then sends `CPU_ON` to the boot CPU, which PSCI refuses as already on, so +every CPU after it in the MADT stays off. + +**Evidence:** the code (`kernel/src/arch/aarch64/smp.rs`, +`kernel/src/arch/aarch64/irqchip.rs`). + +**Exit:** the boot CPU's read is held, before the roster takes it, against an +id it did not produce. diff --git a/issues/panic-path/a-fatal-path-inside-its-own-cpus-console-burst-waits-out-the-bound.md b/issues/panic-path/a-fatal-path-inside-its-own-cpus-console-burst-waits-out-the-bound.md new file mode 100644 index 00000000000..12bf6e76cc0 --- /dev/null +++ b/issues/panic-path/a-fatal-path-inside-its-own-cpus-console-burst-waits-out-the-bound.md @@ -0,0 +1,20 @@ +--- +status: open +kind: defect +opened: 2026-10-01 +--- + +# A fatal path inside its own CPU's console burst waits out the bound + +A fatal path takes the console registers by `BackendLock::seize` +(`kernel/src/drivers/serial_lock.rs`), which finds a hold its own CPU's fatal +path left and gives up on any other after `PANIC_LOCK_SPIN_LIMIT` tries. A +burst holder — `uart_write_fifo`, virtio-console's publish and its looks, a +byte read — holds them as `LIVE`, which names no CPU. A fault, a panic or a +nested NMI that lands inside one waits the whole bound for a holder that never +runs again, says so, and writes over it; every later `panic_registers` on +that path waits it again. + +**Evidence:** the code. + +**Exit:** no fatal path waits out a hold its own CPU left. diff --git a/issues/panic-path/a-panic-reentry-inside-a-fatal-paths-console-hold-halts-with-it-held.md b/issues/panic-path/a-panic-reentry-inside-a-fatal-paths-console-hold-halts-with-it-held.md new file mode 100644 index 00000000000..36f98514757 --- /dev/null +++ b/issues/panic-path/a-panic-reentry-inside-a-fatal-paths-console-hold-halts-with-it-held.md @@ -0,0 +1,20 @@ +--- +status: open +kind: defect +opened: 2026-10-01 +--- + +# A panic reentry inside a fatal path's console hold halts with it held + +`panic::last_words` holds the console registers (`serial::panic_registers`) +for its report, and `serial::panic_flush` for its drain. On the DOUBLE PANIC +path and the first `panic_flush` of an ordinary panic (`kernel/src/main.rs`), +that is before `halt_all_cpus` has stopped the other CPUs. A panic inside the +hold reaches the reentry guard, which writes under it and halts this CPU +alone, so the hold is never let go: every other CPU's next console burst +waits for it with interrupts off (`BackendLock::lock`), and the console takes +no burst again. + +**Evidence:** the code. + +**Exit:** no CPU halts holding the console registers while another still runs. diff --git a/issues/panic-path/a-panic-write-between-two-console-bursts-can-split-a-character.md b/issues/panic-path/a-panic-write-between-two-console-bursts-can-split-a-character.md new file mode 100644 index 00000000000..5e772355d02 --- /dev/null +++ b/issues/panic-path/a-panic-write-between-two-console-bursts-can-split-a-character.md @@ -0,0 +1,28 @@ +--- +status: open +kind: defect +opened: 2026-10-01 +--- + +# A panic-path write between two console bursts can split a character + +`serial::uart_write_fifo` cuts a line into `TX_BURST`-byte bursts wherever the +count falls, so a multibyte character, such as the `—` many of the kernel's +records carry, can straddle two of them. The panic path takes the registers +between a wire holder's bursts (`panic_flush`'s drain, `nested_nmi`'s report), +so its bytes can land between that character's lead byte and the rest, and +the console line they cut is not UTF-8. + +The harness's reader stops at the first console line that is not UTF-8 +(`publish_line` in `tests/common/qemu.rs`), so the fatal report after it never +reaches an assertion, and the boot waits for QEMU to exit. + +**Evidence:** the code. The harness's half was recorded with the unlocked +nested-NMI report as the splitter: main's nightly `guest` lane at `06788146b`, +run 36843762360, job 110374194368, where `nested_nmi_is_loud` ended `QEMU died +before NESTED NMI (status 0)` after 63 s; `PANIC_BOUND_MS` is 60 s. Its capture +stops in the line before the `—` of cpu1's `i8042:` record, and holds neither +the rest of the report nor the panic flush's records. + +**Exit:** no console line the panic path writes into carries a split +character. diff --git a/kernel-loom/src/lib.rs b/kernel-loom/src/lib.rs index adfd920c320..afcade99636 100644 --- a/kernel-loom/src/lib.rs +++ b/kernel-loom/src/lib.rs @@ -101,6 +101,26 @@ pub mod arch { pub fn poll() {} } + /// Which CPU a model thread is, which the console lock's fatal word and the + /// roster's echo name. + /// The model says, through [`become_cpu`]; the kernel asks the CPU. Per + /// model thread for `scheduler`'s reason: loom's threads share an OS one. + #[cfg(feature = "loom")] + pub mod cpu { + loom::thread_local! { + static CPU: core::cell::Cell> = core::cell::Cell::new(None); + } + + /// Say which CPU the current model thread is. No kernel counterpart. + pub fn become_cpu(id: u32) { + CPU.with(|cpu| cpu.set(Some(id))); + } + + pub fn hardware_id() -> u32 { + CPU.with(|cpu| cpu.get()).expect("a model thread asked which cpu it is before saying") + } + } + /// **A strictly stronger model than the instruction, and the direction is /// the whole argument.** /// @@ -148,7 +168,9 @@ pub mod sync; #[path = "../../kernel/src/shootdown.rs"] pub mod shootdown; -/// The CPU roster and the release/answer word, driven by `tests/smp_bringup.rs`. +/// The CPU roster and the release/answer word, driven by `tests/smp_bringup.rs`; +/// under `loom` alone, as is the [`arch::cpu`] shim it names. +#[cfg(feature = "loom")] #[path = "../../kernel/src/smp_roster.rs"] pub mod smp_roster; @@ -352,6 +374,8 @@ pub mod log_ring; #[path = "../../kernel/src/drivers/panic_console/published.rs"] pub mod panic_console_published; -/// The console backend's lock, driven by `tests/serial_lock.rs`. +/// The console backend's lock, driven by `tests/serial_lock.rs`; under `loom` +/// alone, as is the [`arch::cpu`] shim it names. +#[cfg(feature = "loom")] #[path = "../../kernel/src/drivers/serial_lock.rs"] pub mod serial_lock; diff --git a/kernel-loom/tests/serial_lock.rs b/kernel-loom/tests/serial_lock.rs index 58613469a37..eb66a2b4c7d 100644 --- a/kernel-loom/tests/serial_lock.rs +++ b/kernel-loom/tests/serial_lock.rs @@ -1,13 +1,15 @@ -//! Loom: a lost `try_lock` on the console backend leaves its holder holding. +//! Loom: a lost `try_lock` on the console backend leaves its holder holding; +//! a fatal path's `seize` waits for a holder that lets go, gives up on one that +//! never does, and has its own CPU's hold at once; and a burst beneath its own +//! CPU's fatal path is refused rather than spun on. //! -//! The console drain takes the backend with `try_lock` from every CPU that -//! logs, so a losing attempt is the common case beside another CPU's write. //! A loss that released the lock let a third writer in under the holder. The //! negative control is the `serial-try-lock-then-some` feature, which must red //! this file. #![cfg(feature = "loom")] -use kernel_loom::serial_lock::BackendLock; +use kernel_loom::arch::cpu::become_cpu; +use kernel_loom::serial_lock::{within, BackendLock, Seized}; use loom::cell::UnsafeCell; use loom::sync::Arc; use loom::thread; @@ -49,3 +51,141 @@ fn two_writers_never_overlap() { other.join().unwrap(); }); } + +/// The bound is the count asked for: an attempt that never answers is asked +/// exactly `tries` times. In a model only because the pause is loom's. +#[test] +fn within_asks_a_silent_attempt_exactly_its_tries() { + for tries in [0, 1, 100] { + loom::model(move || { + let mut asked = 0; + let answer = within(tries, || { + asked += 1; + None::<()> + }); + assert!(answer.is_none(), "an attempt that never answered was answered"); + assert_eq!(asked, tries, "a wait bounded at {tries} tries asked {asked} times"); + }); + } +} + +loom::lazy_static! { + /// A lock whose burst can be handed to another thread: [`Held`] borrows it. + static ref LOCK: BackendLock = BackendLock::new(); +} + +/// The wait inside the bound: a burst held when a seize begins, and let go on +/// another CPU, is taken. Each time the seize runs again before the holder has +/// let go, which loom counts as a preemption of the holder, costs it a try, so +/// `k` of them leave `k + 2` tries enough; unbounded, no count would be. +#[test] +fn a_seize_takes_a_burst_let_go_inside_the_bound() { + for preemptions in 0..=2u8 { + let mut model = loom::model::Builder::new(); + model.preemption_bound = Some(usize::from(preemptions)); + model.check(move || { + become_cpu(0); + let burst = LOCK.try_lock().expect("an unheld lock refused its first taker"); + let holder = thread::spawn(move || drop(burst)); + let taken = matches!(LOCK.seize(u64::from(preemptions) + 2), Seized::Taken(_)); + // Joined first: a holder never run still owns the burst, whose + // drop outside the model would abort the binary rather than fail. + holder.join().unwrap(); + assert!(taken, "a fatal path gave up on a burst that let go inside the bound"); + }); + } +} + +/// Nothing on a fatal path waits for ever, and a holder that never lets go is +/// what it would wait for. +#[test] +fn a_seize_gives_up_on_a_holder_that_never_lets_go() { + loom::model(|| { + become_cpu(0); + let lock = BackendLock::new(); + let _burst = lock.try_lock().expect("an unheld lock refused its first taker"); + assert!(matches!(lock.seize(3), Seized::Expired), "a seize took a lock its holder kept"); + }); +} + +/// A fatal path entered on top of its own CPU's has the lock at once, and +/// another CPU's still waits the holder out. +#[test] +fn a_seize_reenters_its_own_cpus_hold() { + loom::model(|| { + let lock = Arc::new(BackendLock::new()); + become_cpu(1); + let Seized::Taken(held) = lock.seize(1) else { panic!("a free lock refused a seize") }; + assert!(matches!(lock.seize(1), Seized::Reentered), "a fatal path waited out its own CPU's hold"); + assert!(lock.try_lock().is_none(), "a burst took a fatal path's hold"); + let other = { + let lock = lock.clone(); + thread::spawn(move || { + become_cpu(2); + matches!(lock.seize(1), Seized::Expired) + }) + }; + assert!(other.join().unwrap(), "a fatal path took another CPU's hold"); + drop(held); + assert!(matches!(lock.seize(1), Seized::Taken(_)), "a released lock stayed held"); + }); +} + +/// A fatal path and a burst on two CPUs: whoever gets in writes alone. +#[test] +fn a_seize_and_a_burst_never_overlap() { + loom::model(|| { + let lock = Arc::new(BackendLock::new()); + let line = Arc::new(UnsafeCell::new(0u32)); + let burst = { + let (lock, line) = (lock.clone(), line.clone()); + thread::spawn(move || { + if let Some(held) = lock.try_lock() { + // SAFETY: the backend is held, so no other writer is in here. + line.with_mut(|n| unsafe { *n += 1 }); + drop(held); + } + }) + }; + become_cpu(0); + if let Seized::Taken(held) = lock.seize(2) { + // SAFETY: as above. + line.with_mut(|n| unsafe { *n += 1 }); + drop(held); + } + burst.join().unwrap(); + }); +} + +/// A burst on the CPU whose fatal path holds the registers would wait for a +/// frame beneath it: it is refused, loudly. +#[test] +#[should_panic(expected = "serial: a burst asked for the console registers this cpu's own fatal path holds")] +fn a_burst_beneath_its_own_cpus_fatal_path_is_refused() { + loom::model(|| { + become_cpu(1); + let lock = BackendLock::new(); + let _fatal = lock.seize(1); + drop(lock.lock()); + }); +} + +/// A burst on another CPU waits a fatal path's hold out, and has the lock +/// once it is let go. +#[test] +fn a_burst_waits_out_another_cpus_fatal_path() { + loom::model(|| { + let lock = Arc::new(BackendLock::new()); + become_cpu(1); + let Seized::Taken(fatal) = lock.seize(1) else { panic!("a free lock refused a seize") }; + let burst = { + let lock = lock.clone(); + thread::spawn(move || { + become_cpu(2); + drop(lock.lock()); + }) + }; + drop(fatal); + burst.join().unwrap(); + }); +} diff --git a/kernel-loom/tests/smp_bringup.rs b/kernel-loom/tests/smp_bringup.rs index d89aa87a63f..50f5b313825 100644 --- a/kernel-loom/tests/smp_bringup.rs +++ b/kernel-loom/tests/smp_bringup.rs @@ -1,4 +1,5 @@ -//! Loom: AP bring-up's two ordering edges, invisible to a guest test on x86 TSO. +//! Loom: AP bring-up's two ordering edges, invisible to a guest test on x86 TSO, +//! and its commit's refusal of an AP that reads its own hardware id as another. //! `--features roster-commit-relaxed` reds publication (a reader sees a count over //! an unfilled slot); `--features smp-ready-split` reds release (a CPU sees the //! machine released but not yet answering, so a shootdown skips a joined AP). The @@ -7,14 +8,19 @@ #![cfg(feature = "loom")] +use std::cell::Cell; use std::sync::atomic::{AtomicBool, Ordering::SeqCst}; +use kernel_loom::arch::cpu::become_cpu; use kernel_loom::smp_roster::Roster; use loom::sync::Arc; /// A hardware id no uncommitted slot holds (the roster fills a slot with `u32::MAX`). const HARDWARE_ID: u32 = 7; +/// What an AP started as [`HARDWARE_ID`] reads as its own when it reads wrong. +const MISREAD: u32 = 5; + /// Bounded for loom (an unbounded spin never finishes); each turn a scheduling point. const POLLS: usize = 4; @@ -29,6 +35,8 @@ fn a_committed_count_never_outruns_its_slot() { let r = Arc::new(Roster::new()); let attempt = r.begin_attempt().expect("a slot is free"); assert_eq!(attempt.id(), 1, "cpu1 is the first AP"); + become_cpu(HARDWARE_ID); + r.echo(attempt.token()); let committer = { let r = r.clone(); @@ -91,3 +99,35 @@ fn a_released_machine_is_answering() { "no interleaving observed the release, so the assertion never ran", ); } + +/// The boot CPU that sees an AP's echo refuses to commit it under an id other +/// than the one the AP echoed as its own read. +#[test] +#[should_panic( + expected = "smp: cpu1 reads its own hardware id as 0x5, and its roster slot and every IPI name it 0x7" +)] +fn an_ap_that_reads_another_id_is_refused() { + loom::model(|| { + let r = Arc::new(Roster::new()); + let attempt = r.begin_attempt().expect("a slot is free"); + + let ap = { + let r = r.clone(); + loom::thread::spawn(move || { + become_cpu(MISREAD); + r.echo(attempt.token()); + }) + }; + + let polls = Cell::new(0); + let echoed = r.await_echo(attempt, || { + polls.set(polls.get() + 1); + loom::thread::yield_now(); + polls.get() > POLLS + }); + ap.join().unwrap(); + if echoed { + r.commit(attempt, HARDWARE_ID); + } + }); +} diff --git a/kernel/src/arch/aarch64/console_uart.rs b/kernel/src/arch/aarch64/console_uart.rs index a0b235f0762..91bf79de17b 100644 --- a/kernel/src/arch/aarch64/console_uart.rs +++ b/kernel/src/arch/aarch64/console_uart.rs @@ -9,6 +9,7 @@ use core::sync::atomic::{AtomicU64, Ordering}; use toyos_acpi::{SerialInterface, GAS_SYSTEM_MEMORY}; use crate::drivers::acpi::direct_phys; +use crate::drivers::serial::Registers; use crate::log; use crate::mm::{DirectMap, Mmio}; @@ -41,7 +42,9 @@ fn regs() -> Mmio { } /// Find the UART SPCR names and answer whether it is one this file drives. -pub fn init(rsdp_addr: u64) -> bool { +/// Takes the registers for the call it shares with the 16550's `init`, which +/// programs its UART; this one touches none. +pub fn init(_: &mut Registers, rsdp_addr: u64) -> bool { let spcr = match toyos_acpi::spcr(direct_phys(), rsdp_addr) { Ok(spcr) => spcr, Err(e) => { @@ -67,21 +70,21 @@ pub fn init(rsdp_addr: u64) -> bool { } /// Whether a received byte waits. -pub fn rx_ready() -> bool { +pub fn rx_ready(_: &mut Registers) -> bool { regs().read_u32(FR) & FR_RXFE == 0 } /// The received byte; only after [`rx_ready`] said one waits. -pub fn read_byte() -> u8 { +pub fn read_byte(_: &mut Registers) -> u8 { regs().read_u32(DR) as u8 } /// Whether the transmitter will take a byte. -pub fn tx_ready() -> bool { +pub fn tx_ready(_: &mut Registers) -> bool { regs().read_u32(FR) & FR_TXFF == 0 } /// Put one byte in the transmitter; only after [`tx_ready`], or the byte may be lost. -pub fn write_byte(byte: u8) { +pub fn write_byte(_: &mut Registers, byte: u8) { regs().write_u32(DR, u32::from(byte)); } diff --git a/kernel/src/arch/x86_64/console_uart.rs b/kernel/src/arch/x86_64/console_uart.rs index ef1658e84ea..4a25cdb308f 100644 --- a/kernel/src/arch/x86_64/console_uart.rs +++ b/kernel/src/arch/x86_64/console_uart.rs @@ -2,6 +2,7 @@ //! kernel boots puts one if it has one. use super::cpu::{inb, outb}; +use crate::drivers::serial::Registers; use crate::log; const PORT: u16 = 0x3f8; // COM1 @@ -23,10 +24,11 @@ pub const TX_BURST: usize = 16; // Every register is `PORT + n`; the identity op keeps that pattern uniform // across all eight lines instead of special-casing the data register. #[allow(clippy::identity_op)] -pub fn init(_rsdp_addr: u64) -> bool { +pub fn init(_: &mut Registers, _rsdp_addr: u64) -> bool { // SAFETY: `outb`/`inb` require the caller to own the port and the byte; - // every port here is `PORT + n` for `n` in 0..=4, inside COM1's own - // register block, and the writes are the 16550's documented init sequence. + // the caller holds the registers, every port here is `PORT + n` for `n` + // in 0..=4, inside COM1's own register block, and the writes are the + // 16550's documented init sequence. // Order matters: DLAB must precede the divisor writes and loopback mode // must precede the probe, or the sequence misprograms the chip. let loopback = unsafe { @@ -54,23 +56,24 @@ pub fn init(_rsdp_addr: u64) -> bool { } /// Whether a received byte waits. -pub fn rx_ready() -> bool { +pub fn rx_ready(_: &mut Registers) -> bool { inb(LSR) & LSR_DATA_READY != 0 } /// The received byte; only after [`rx_ready`] said one waits. -pub fn read_byte() -> u8 { +pub fn read_byte(_: &mut Registers) -> u8 { inb(PORT) } /// Whether the transmitter will take a byte. -pub fn tx_ready() -> bool { +pub fn tx_ready(_: &mut Registers) -> bool { inb(LSR) & LSR_THR_EMPTY != 0 } /// Put one byte in the transmitter; only after [`tx_ready`], or the byte may be lost. -pub fn write_byte(byte: u8) { - // SAFETY: `outb` requires ownership of the port and the byte; `PORT` is - // COM1's own data register, and the byte is console output only. +pub fn write_byte(_: &mut Registers, byte: u8) { + // SAFETY: `outb` requires ownership of the port and the byte; the caller + // holds the registers, `PORT` is COM1's own data register, and the byte is + // console output only. unsafe { outb(PORT, byte) }; } diff --git a/kernel/src/arch/x86_64/idt/nmi.rs b/kernel/src/arch/x86_64/idt/nmi.rs index 2ef04370a92..a9a8ab28e95 100644 --- a/kernel/src/arch/x86_64/idt/nmi.rs +++ b/kernel/src/arch/x86_64/idt/nmi.rs @@ -11,8 +11,7 @@ //! the one bound a CPU with `IF` clear is under: the frame's `rip`, `rsp` and //! `rflags` are what a machine whose every CPU stopped taking interrupts has //! left to say. That path may seal a record and reset from here and never -//! return, and it obeys this file's discipline exactly — no lock, no -//! allocation, nothing that logs. +//! return. use core::arch::naked_asm; @@ -137,15 +136,18 @@ fn stage_nested_if_armed() { /// A second NMI on a stack the first is still standing on. /// Logs nothing: the interrupted context may be mid-publish of its own record, and one from here would garble the ring `halt_all_cpus` reads. `src/sourcegate.rs`'s `nmi_does_not_log` is the gate. -/// `panic_raw` takes no lock, so it can't be blocked by whatever either context held. extern "sysv64" fn nested_nmi(rip: u64, rsp: u64) -> ! { - let serial = crate::drivers::serial::panic_raw; - serial(b"\n[nmi] NESTED NMI on cpu "); - crate::drivers::serial::panic_raw_dec(u64::from(crate::arch::percpu::cpu_id())); - serial(b": a second NMI entered while IST2 was still in use.\n[nmi] rip="); - crate::drivers::serial::panic_raw_hex(rip); - serial(b" rsp="); - crate::drivers::serial::panic_raw_hex(rsp); - serial(b"\n[nmi] the outer handler's frame is gone; the machine stops here.\n"); + // Let go of before the halt: its flush, finding this CPU's own fatal path + // holding the registers, would drain raw and leave virtio-console out. + { + let mut uart = crate::drivers::serial::panic_registers(); + uart.write(b"\n[nmi] NESTED NMI on cpu "); + uart.dec(u64::from(crate::arch::percpu::cpu_id())); + uart.write(b": a second NMI entered while IST2 was still in use.\n[nmi] rip="); + uart.hex(rip); + uart.write(b" rsp="); + uart.hex(rsp); + uart.write(b"\n[nmi] the outer handler's frame is gone; the machine stops here.\n"); + } crate::panic::halt_all_cpus() } diff --git a/kernel/src/arch/x86_64/percpu.rs b/kernel/src/arch/x86_64/percpu.rs index c19deac2368..113329f0cc6 100644 --- a/kernel/src/arch/x86_64/percpu.rs +++ b/kernel/src/arch/x86_64/percpu.rs @@ -461,11 +461,12 @@ pub fn ist1_report() { * 8; let used = IST_STACK_SIZE - untouched; - crate::drivers::serial::panic_raw(b"\n[ist1] used "); - crate::drivers::serial::panic_raw_dec(used as u64); - crate::drivers::serial::panic_raw(b" of "); - crate::drivers::serial::panic_raw_dec(IST_STACK_SIZE as u64); - crate::drivers::serial::panic_raw(if intact { + let mut uart = crate::drivers::serial::panic_registers(); + uart.write(b"\n[ist1] used "); + uart.dec(used as u64); + uart.write(b" of "); + uart.dec(IST_STACK_SIZE as u64); + uart.write(if intact { b" bytes, guard intact\n" } else { b" bytes, GUARD CORRUPTED\n" diff --git a/kernel/src/arch/x86_64/smp.rs b/kernel/src/arch/x86_64/smp.rs index e16c55118e5..721f93de39d 100644 --- a/kernel/src/arch/x86_64/smp.rs +++ b/kernel/src/arch/x86_64/smp.rs @@ -165,6 +165,12 @@ fn build_trampoline_data() -> TrampolineData { /// Boot all Application Processors found in the MADT, using `boot_cr3` — the bootloader's identity+high-half PML4 — until each AP switches to the kernel PML4 in `ap_entry`. pub fn boot_aps(madt: &MadtInfo, boot_cr3: u64) { let bsp_id = apic::id(); + let read = cpu::hardware_id(); + assert_eq!( + read, + bsp_id, + "smp: the boot CPU reads its own hardware id as {read:#x}, and its roster slot and every IPI name it {bsp_id:#x}" + ); ROSTER.set_bsp(bsp_id); copy_trampoline(); diff --git a/kernel/src/drivers/serial.rs b/kernel/src/drivers/serial.rs index 10a9acd2f5f..c1f9c14c6d9 100644 --- a/kernel/src/drivers/serial.rs +++ b/kernel/src/drivers/serial.rs @@ -6,13 +6,13 @@ //! burst: what the UART's transmitter takes at once; the publish of a //! transmit buffer to virtio-console, and each look for its completion, which //! the host takes at its own pace; a byte read. The panic path takes the -//! registers alone, and bypasses them once they stay held. Nothing that holds -//! a kernel lock formats here. +//! registers alone, as a [`PanicUart`], and bypasses them once they stay held. +//! Nothing that holds a kernel lock formats here. use core::sync::atomic::{AtomicBool, Ordering}; use crate::arch::IrqGuard; use crate::log; -use super::serial_lock::{BackendLock, Held}; +use super::serial_lock::{self, BackendLock, Held, Seized}; use crate::scheduler::Parkable; use crate::sleeplock::{SleepGuard, SleepLock}; @@ -25,7 +25,10 @@ static UART_PRESENT: AtomicBool = AtomicBool::new(false); /// Find and program the console UART, off the firmware tables at `rsdp_addr` /// where the architecture places it by them. pub fn init(rsdp_addr: u64) { - UART_PRESENT.store(uart::init(rsdp_addr), Ordering::Relaxed); + // What the architecture logs under the lock is not drained under it: + // `has_console` is false until the store below. + let present = uart::init(&mut BackendGuard::lock().registers, rsdp_addr); + UART_PRESENT.store(present, Ordering::Relaxed); console_changed(); } @@ -66,49 +69,48 @@ pub fn backend() -> Backend { static BACKEND: BackendLock = BackendLock::new(); +/// The console UART's registers, asked for: what every `arch::console_uart` +/// function that touches the UART takes. Only this file builds one, for a +/// [`BackendGuard`] and a [`PanicUart`]. +pub struct Registers(()); + /// Exclusive access to the serial backend; interrupts are off for as long as the guard lives. /// Same-CPU re-entry from an IRQ handler deadlocks the spin. pub struct BackendGuard { // Fields drop in order: the backend is released before interrupts reopen. _held: Held<'static>, _irq: IrqGuard, + registers: Registers, } impl BackendGuard { pub fn lock() -> Self { let irq = IrqGuard::close(); - Self { _held: BACKEND.lock(), _irq: irq } - } - - /// Non-blocking acquire: `None` if another CPU already holds the backend. - pub fn try_lock() -> Option { - let irq = IrqGuard::close(); - let held = BACKEND.try_lock()?; - Some(Self { _held: held, _irq: irq }) + Self { _held: BACKEND.lock(), _irq: irq, registers: Registers(()) } } /// Writes raw bytes with no escape stripping. pub fn write_raw(&mut self, bytes: &[u8]) { match backend() { - Backend::Virtio => super::virtio_console::write_bytes_locked(bytes), - Backend::Uart => uart_write_bytes(bytes), + Backend::Virtio => super::virtio_console::write_bytes_locked(self, bytes), + Backend::Uart => uart_write_bytes(&mut self.registers, bytes), Backend::None => {} } } - pub fn has_data(&self) -> bool { + pub fn has_data(&mut self) -> bool { if super::virtio_console::is_ready() { - super::virtio_console::has_data_locked() + super::virtio_console::has_data_locked(self) } else { - uart_present() && uart::rx_ready() + uart_present() && uart::rx_ready(&mut self.registers) } } pub fn try_read_byte(&mut self) -> Option { if super::virtio_console::is_ready() { - super::virtio_console::try_read_byte_locked() - } else if uart_present() && uart::rx_ready() { - Some(uart::read_byte()) + super::virtio_console::try_read_byte_locked(self) + } else if uart_present() && uart::rx_ready(&mut self.registers) { + Some(uart::read_byte(&mut self.registers)) } else { None } @@ -116,7 +118,7 @@ impl BackendGuard { } pub fn has_data() -> bool { - let g = BackendGuard::lock(); + let mut g = BackendGuard::lock(); g.has_data() } @@ -128,11 +130,100 @@ pub fn try_read_byte() -> Option { /// ~1s of spin, long enough for a live guard holder to release and short enough not to hang panic. const PANIC_LOCK_SPIN_LIMIT: u64 = 100_000_000; +/// The console UART's registers for a fatal path, from [`panic_registers`]. +/// A `log!` under one before `klogd` runs may drain inline on this CPU, as a +/// burst that `BackendLock::lock` refuses while this CPU's fatal path holds +/// the registers. +pub struct PanicUart(Hold); + +enum Hold { + /// Taken from nobody, or from a holder that let go inside the bound. + Held(BackendGuard), + /// This CPU's own fatal path holds the registers, underneath this one. + Reentered(Over), + /// Another holder kept them through the whole bound, which was said. + Expired(Over), +} + +/// The registers, written over whoever holds them. +struct Over { + registers: Registers, + _irq: IrqGuard, +} + +/// What a fatal path that waited out another holder writes first. +const WRITTEN_OVER: &[u8] = + b"\n[serial] the console registers stayed held through the bound; written over their holder\n"; + +/// What a flush that found its own CPU's fatal path holding the registers writes first. +const DRAINED_RAW: &[u8] = + b"\n[serial] this cpu's own fatal path held the console registers; drained raw\n"; + +/// The registers for a fatal path: waited for while another holder may still +/// let them go, and had at once where this CPU's fatal path holds them. +pub fn panic_registers() -> PanicUart { + let irq = IrqGuard::close(); + PanicUart(match BACKEND.seize(PANIC_LOCK_SPIN_LIMIT) { + Seized::Taken(held) => Hold::Held(BackendGuard { _held: held, _irq: irq, registers: Registers(()) }), + Seized::Reentered => Hold::Reentered(Over { registers: Registers(()), _irq: irq }), + Seized::Expired => { + let mut over = Over { registers: Registers(()), _irq: irq }; + uart_write_bytes(&mut over.registers, WRITTEN_OVER); + Hold::Expired(over) + } + }) +} + +impl PanicUart { + fn registers(&mut self) -> &mut Registers { + match &mut self.0 { + Hold::Held(guard) => &mut guard.registers, + Hold::Reentered(over) | Hold::Expired(over) => &mut over.registers, + } + } + + /// Straight to the UART, never virtio-console: no allocation, bounded per byte. + pub fn write(&mut self, bytes: &[u8]) { + uart_write_bytes(self.registers(), bytes); + } + + /// An address, formatted as `{:#018x}` to match the rest of the crash report. + pub fn hex(&mut self, v: u64) { + let mut out = [b'0'; 18]; + out[1] = b'x'; + for (i, byte) in out[2..].iter_mut().enumerate() { + let nibble = (v >> (60 - 4 * i)) as u8 & 0xF; + *byte = if nibble < 10 { b'0' + nibble } else { b'a' + nibble - 10 }; + } + uart_write_bytes(self.registers(), &out); + } + + /// A number, since the callers cannot format one. + pub fn dec(&mut self, mut v: u64) { + let mut digits = [0u8; 20]; + let mut n = 0; + loop { + digits[n] = b'0' + (v % 10) as u8; + n += 1; + v /= 10; + if v == 0 || n == digits.len() { + break; + } + } + let mut out = [0u8; 20]; + for i in 0..n { + out[i] = digits[n - 1 - i]; + } + uart_write_bytes(self.registers(), &out[..n]); + } +} + /// Flushes pending logs on the panic path. /// /// Waits for a live guard holder to release before bypassing it — bypassing /// immediately would race its live ring/virtqueue mutation — and only -/// bypasses a holder that never releases. +/// bypasses a holder that never releases, or its own CPU's fatal path, which +/// may have stopped inside a virtqueue publish. /// /// # Safety /// Panic context only: the bypass reads the drain position with no lock held. @@ -142,12 +233,13 @@ pub unsafe fn panic_flush() { if !has_console() { return; } - for _ in 0..PANIC_LOCK_SPIN_LIMIT { - if let Some(mut g) = BackendGuard::try_lock() { - crate::log::console::drain_locked(&mut g); - return; - } - core::hint::spin_loop(); + let mut uart = panic_registers(); + if let Hold::Held(guard) = &mut uart.0 { + crate::log::console::drain_locked(guard); + return; + } + if matches!(uart.0, Hold::Reentered(_)) { + uart.write(DRAINED_RAW); } // Disables virtio-console first: a half-submitted TX queue would panic // recursively if a bypassing write reached it. @@ -155,9 +247,10 @@ pub unsafe fn panic_flush() { return; } super::virtio_console::disable(); - // SAFETY: the bounded wait above found no clean handoff; the holder is - // wedged and will not publish, so reading its position unlocked is safe. - unsafe { crate::log::console::drain_bypassed() }; + // SAFETY: the registers are a wedged holder's or a fatal path's beneath + // this one, neither of which runs again to publish, so reading the + // position unlocked is safe. + unsafe { crate::log::console::drain_bypassed(&mut uart) }; } /// Drains the ring before the machine powers off, so the tail of a shutdown @@ -168,12 +261,9 @@ pub unsafe fn panic_flush() { /// nothing else runs. Losing the tail is better than not powering off, and /// the black box says it was lost, since the console cannot. pub fn flush_final() { - for _ in 0..PANIC_LOCK_SPIN_LIMIT { - if let Some(wire) = try_wire() { - crate::log::console::drain_all(&wire); - return; - } - core::hint::spin_loop(); + if let Some(wire) = serial_lock::within(PANIC_LOCK_SPIN_LIMIT, try_wire) { + crate::log::console::drain_all(&wire); + return; } crate::blackbox::append(|lines| { let _ = writeln!( @@ -230,11 +320,11 @@ fn uart_write_fifo(bytes: &[u8]) { for chunk in bytes.chunks(uart::TX_BURST) { let mut asked = 0; loop { - let burst = BackendGuard::lock(); - if uart::tx_ready() { + let mut burst = BackendGuard::lock(); + if uart::tx_ready(&mut burst.registers) { // A chunk is no more than the transmitter takes once ready. for &b in chunk { - uart::write_byte(b); + uart::write_byte(&mut burst.registers, b); } break; } @@ -364,52 +454,17 @@ pub const MAX_CONSOLE_LINE: usize = 1024; /// forever here, on `panic_flush`'s bypass path where nothing else can help. const THRE_SPIN_LIMIT: u32 = 100_000; -fn uart_write_bytes(bytes: &[u8]) { +fn uart_write_bytes(registers: &mut Registers, bytes: &[u8]) { if !uart_present() { return; } for &b in bytes { for _ in 0..THRE_SPIN_LIMIT { - if uart::tx_ready() { + if uart::tx_ready(registers) { break; } core::hint::spin_loop(); } - uart::write_byte(b); - } -} - -/// Writes straight to the UART, bypassing the ring, the lock and virtio-console: no allocation, bounded per byte. -pub fn panic_raw(bytes: &[u8]) { - uart_write_bytes(bytes); -} - -/// `panic_raw` for an address, formatted as `{:#018x}` to match the rest of the crash report. -pub fn panic_raw_hex(v: u64) { - let mut out = [b'0'; 18]; - out[1] = b'x'; - for (i, byte) in out[2..].iter_mut().enumerate() { - let nibble = (v >> (60 - 4 * i)) as u8 & 0xF; - *byte = if nibble < 10 { b'0' + nibble } else { b'a' + nibble - 10 }; - } - uart_write_bytes(&out); -} - -/// `panic_raw` for a number, since the callers cannot format one. -pub fn panic_raw_dec(mut v: u64) { - let mut digits = [0u8; 20]; - let mut n = 0; - loop { - digits[n] = b'0' + (v % 10) as u8; - n += 1; - v /= 10; - if v == 0 || n == digits.len() { - break; - } - } - let mut out = [0u8; 20]; - for i in 0..n { - out[i] = digits[n - 1 - i]; + uart::write_byte(registers, b); } - uart_write_bytes(&out[..n]); } diff --git a/kernel/src/drivers/serial_lock.rs b/kernel/src/drivers/serial_lock.rs index 6928a93ce80..931b3f207f2 100644 --- a/kernel/src/drivers/serial_lock.rs +++ b/kernel/src/drivers/serial_lock.rs @@ -1,16 +1,31 @@ -//! The console backend's one lock: a flag that only the [`Held`] a won +//! The console backend's one lock: a word that only the [`Held`] a won //! exchange returns ever clears, by its drop. A lost `try_lock` builds no //! [`Held`], so it can never release the lock another CPU holds. -//! No `crate::` references: `kernel-loom` compiles this file directly under `feature = "loom"`. +//! A fatal path's hold names its CPU ([`BackendLock::seize`]): a fatal path +//! that CPU enters on top of it finds the lock its own, where waiting would +//! wait for a frame that never runs again, and a burst it enters is refused. +//! Of the kernel it names only `crate::arch::cpu::hardware_id`, which +//! `kernel-loom` supplies to compile this file. #[cfg(not(feature = "loom"))] -use core::sync::atomic::{AtomicBool, Ordering}; +use core::{hint::spin_loop, sync::atomic::{AtomicU64, Ordering}}; #[cfg(feature = "loom")] -use loom::sync::atomic::{AtomicBool, Ordering}; +use loom::{hint::spin_loop, sync::atomic::{AtomicU64, Ordering}}; pub struct BackendLock { - locked: AtomicBool, + /// [`FREE`], [`LIVE`], or [`FATAL`] plus the CPU whose fatal path holds it. + word: AtomicU64, +} + +const FREE: u64 = 0; +/// A holder that lets go once its burst, look or read is done. +const LIVE: u64 = 1; +const FATAL: u64 = 2; + +/// The word this CPU's fatal path holds the lock as. +fn fatal_here() -> u64 { + FATAL + u64::from(crate::arch::cpu::hardware_id()) } /// The lock, held for as long as this lives. @@ -18,30 +33,46 @@ pub struct Held<'a> { lock: &'a BackendLock, } +/// What a fatal path's [`BackendLock::seize`] came back with. +pub enum Seized<'a> { + /// Free, or let go of inside the bound. + Taken(Held<'a>), + /// This CPU's own fatal path holds it, underneath the one asking. + Reentered, + /// Another holder kept it through every try. + Expired, +} + impl BackendLock { /// Must stay `const`: the backend's lock is a `static`. #[cfg(not(feature = "loom"))] pub const fn new() -> Self { - Self { locked: AtomicBool::new(false) } + Self { word: AtomicU64::new(FREE) } } #[allow(clippy::new_without_default)] #[cfg(feature = "loom")] pub fn new() -> Self { - Self { locked: AtomicBool::new(false) } + Self { word: AtomicU64::new(FREE) } } + /// The lock for a burst; refused on a CPU whose own fatal path holds it, + /// since that holder is beneath this caller and cannot let go while it waits. pub fn lock(&self) -> Held<'_> { - while self - .locked - .compare_exchange_weak(false, true, Ordering::Acquire, Ordering::Relaxed) - .is_err() - { - while self.locked.load(Ordering::Relaxed) { - core::hint::spin_loop(); + loop { + if let Some(held) = self.try_lock() { + return held; + } + let mut word = self.word.load(Ordering::Relaxed); + while word != FREE { + assert!( + word == LIVE || word != fatal_here(), + "serial: a burst asked for the console registers this cpu's own fatal path holds" + ); + spin_loop(); + word = self.word.load(Ordering::Relaxed); } } - Held { lock: self } } /// `None` if another holder has it. @@ -49,7 +80,7 @@ impl BackendLock { // Not `then_some`: its argument is built whether or not the exchange // won, and a `Held` built on a loss drops, and its drop releases the // lock another CPU holds. - let won = self.locked.compare_exchange(false, true, Ordering::Acquire, Ordering::Relaxed).is_ok(); + let won = self.word.compare_exchange(FREE, LIVE, Ordering::Acquire, Ordering::Relaxed).is_ok(); #[cfg(feature = "serial-try-lock-then-some")] return won.then_some(Held { lock: self }); #[cfg(not(feature = "serial-try-lock-then-some"))] @@ -59,10 +90,35 @@ impl BackendLock { None } } + + /// The lock for the fatal path running on this CPU, asked for at most + /// `tries` times: nothing on that path may wait for ever. + pub fn seize(&self, tries: u64) -> Seized<'_> { + let mine = fatal_here(); + within(tries, || match self.word.compare_exchange(FREE, mine, Ordering::Acquire, Ordering::Relaxed) { + Ok(_) => Some(Seized::Taken(Held { lock: self })), + // Only this CPU stores `mine`, so seeing it needs no edge. + Err(word) if word == mine => Some(Seized::Reentered), + Err(_) => None, + }) + .unwrap_or(Seized::Expired) + } +} + +/// `attempt` until it answers, or `tries` times: the one bounded wait for a +/// console lock. +pub fn within(tries: u64, mut attempt: impl FnMut() -> Option) -> Option { + for _ in 0..tries { + if let Some(answer) = attempt() { + return Some(answer); + } + spin_loop(); + } + None } impl Drop for Held<'_> { fn drop(&mut self) { - self.lock.locked.store(false, Ordering::Release); + self.lock.word.store(FREE, Ordering::Release); } } diff --git a/kernel/src/drivers/virtio_console.rs b/kernel/src/drivers/virtio_console.rs index 2cd1319951a..dc6cb55ee9f 100644 --- a/kernel/src/drivers/virtio_console.rs +++ b/kernel/src/drivers/virtio_console.rs @@ -8,6 +8,7 @@ use core::mem::MaybeUninit; use core::sync::atomic::{AtomicBool, Ordering}; use super::pci::PciDevice; +use super::serial::BackendGuard; use super::virtio::{wait_used, BufDir, DescSlot, Virtqueue, VirtioDevice, VIRTIO_F_VERSION_1}; use super::DmaPool; use crate::log; @@ -91,8 +92,8 @@ fn refill_rx(c: &mut VConsole, buf_idx: usize, slot: DescSlot) { c.desc_to_rx[desc_id as usize] = buf_idx as u8; } -/// Synchronous: blocks until the host consumes each chunk. Caller must hold `serial::BackendGuard` with IRQs disabled. -pub fn write_bytes_locked(bytes: &[u8]) { +/// Synchronous: blocks until the host consumes each chunk. +pub fn write_bytes_locked(_: &mut BackendGuard, bytes: &[u8]) { with_console(|c| { let mut off = 0; while off < bytes.len() { @@ -165,8 +166,8 @@ pub fn write_burst(bytes: &[u8]) { }); } -/// Read one byte from RX. Caller must hold `serial::BackendGuard` with IRQs disabled. -pub fn try_read_byte_locked() -> Option { +/// Read one byte from RX. +pub fn try_read_byte_locked(_: &mut BackendGuard) -> Option { with_console(|c| { if c.rx_pending.is_none() { // `slot`/`len` are bounded by `poll_used`: id indexes `desc_to_rx` @@ -190,8 +191,7 @@ pub fn try_read_byte_locked() -> Option { .flatten() } -/// Caller must hold `serial::BackendGuard` with IRQs disabled. -pub fn has_data_locked() -> bool { +pub fn has_data_locked(_: &BackendGuard) -> bool { with_console(|c| c.rx_pending.is_some() || c.rx.has_used()).unwrap_or(false) } diff --git a/kernel/src/log/console.rs b/kernel/src/log/console.rs index c40699f20c7..79213270791 100644 --- a/kernel/src/log/console.rs +++ b/kernel/src/log/console.rs @@ -18,7 +18,7 @@ use toyos_abi::log::LogRecord; use toyos_sched::task::{WaitClass, WakeCause, WakeReason}; use toyos_sched::park::notify; -use crate::drivers::serial::{self, BackendGuard, MAX_CONSOLE_LINE}; +use crate::drivers::serial::{self, BackendGuard, PanicUart, MAX_CONSOLE_LINE}; use crate::hw::HW; use crate::sched::driver::{cpus, irq_off}; use crate::sched::kthread; @@ -152,13 +152,13 @@ fn drain_records(wire: &SleepGuard<'_, ()>, budget: u64) -> u64 { records } -/// Drain with no lock at all, straight to the 16550. +/// Drain straight to the 16550, over whoever holds its registers. /// /// # Safety -/// Panic path only, after `serial::panic_flush`'s bounded wait for a clean handoff fails; the position is unsynchronised and a record may reach the wire twice. -pub unsafe fn drain_bypassed() { +/// Panic path only, once `serial::panic_flush` found no clean handoff; the position is unsynchronised and a record may reach the wire twice. +pub unsafe fn drain_bypassed(uart: &mut PanicUart) { let mut cursor = DRAINED.take(); - let mut sink = Raw; + let mut sink = Raw { uart }; drain_ordered(&mut cursor, &mut sink); DRAINED.put(&cursor); } @@ -396,12 +396,14 @@ impl RecordSink for Discard { } } -/// Records straight to the 16550, for the bypass. No lock, bounded per byte. -struct Raw; +/// Records straight to the 16550, for the bypass. Bounded per byte. +struct Raw<'a> { + uart: &'a mut PanicUart, +} -impl RecordSink for Raw { +impl RecordSink for Raw<'_> { fn put(&mut self, record: &LogRecord) -> bool { - write_line(record, serial::panic_raw); + write_line(record, |bytes| self.uart.write(bytes)); true } } diff --git a/kernel/src/panic.rs b/kernel/src/panic.rs index dcd72e0cd3e..5481315d3db 100644 --- a/kernel/src/panic.rs +++ b/kernel/src/panic.rs @@ -268,50 +268,54 @@ pub fn last_words( }; let second_message = second.message().as_str().unwrap_or(NOT_CAPTURED); - let raw: fn(&[u8]) = serial::panic_raw; - raw(b"\n!!! "); - raw(header.as_bytes()); - raw(b" !!! (apic "); - serial::panic_raw_dec(u64::from(cpu::hardware_id())); - if let Some(prev) = prev { - raw(b", the cpu was already in "); - raw(state_name(prev).as_bytes()); - } - raw(b")\n first (apic "); - serial::panic_raw_dec(u64::from(slot.apic.load(Ordering::Relaxed))); - raw(b"): "); - match kind { - Kind::Panic => { - raw(b"panic at "); - raw(cut.as_bytes()); - raw(file.as_bytes()); - raw(b":"); - serial::panic_raw_dec(u64::from(line)); - raw(b":"); - serial::panic_raw_dec(u64::from(column)); - raw(b": "); - raw(message.as_bytes()); + // Let go of before the record below: before `klogd` runs, its drain may be + // a burst on this CPU, which this hold would refuse. + { + let mut uart = serial::panic_registers(); + uart.write(b"\n!!! "); + uart.write(header.as_bytes()); + uart.write(b" !!! (apic "); + uart.dec(u64::from(cpu::hardware_id())); + if let Some(prev) = prev { + uart.write(b", the cpu was already in "); + uart.write(state_name(prev).as_bytes()); } - Kind::Fault => { - raw(message.as_bytes()); - raw(b" rip="); - serial::panic_raw_hex(rip); - raw(b" cr2="); - serial::panic_raw_hex(cr2); - raw(b" err="); - serial::panic_raw_hex(error_code); + uart.write(b")\n first (apic "); + uart.dec(u64::from(slot.apic.load(Ordering::Relaxed))); + uart.write(b"): "); + match kind { + Kind::Panic => { + uart.write(b"panic at "); + uart.write(cut.as_bytes()); + uart.write(file.as_bytes()); + uart.write(b":"); + uart.dec(u64::from(line)); + uart.write(b":"); + uart.dec(u64::from(column)); + uart.write(b": "); + uart.write(message.as_bytes()); + } + Kind::Fault => { + uart.write(message.as_bytes()); + uart.write(b" rip="); + uart.hex(rip); + uart.write(b" cr2="); + uart.hex(cr2); + uart.write(b" err="); + uart.hex(error_code); + } + Kind::None => uart.write(b"nothing captured on this cpu"), } - Kind::None => raw(b"nothing captured on this cpu"), + uart.write(b"\n second: panic at "); + uart.write(second_file.as_bytes()); + uart.write(b":"); + uart.dec(u64::from(second_line)); + uart.write(b":"); + uart.dec(u64::from(second_column)); + uart.write(b": "); + uart.write(second_message.as_bytes()); + uart.write(b"\n"); } - raw(b"\n second: panic at "); - raw(second_file.as_bytes()); - raw(b":"); - serial::panic_raw_dec(u64::from(second_line)); - raw(b":"); - serial::panic_raw_dec(u64::from(second_column)); - raw(b": "); - raw(second_message.as_bytes()); - raw(b"\n"); if !on_the_record { return; diff --git a/kernel/src/panic_reboot.rs b/kernel/src/panic_reboot.rs index 4b9cbe9eda4..65bda7dbe21 100644 --- a/kernel/src/panic_reboot.rs +++ b/kernel/src/panic_reboot.rs @@ -114,7 +114,7 @@ pub fn arm(on_the_record: bool) -> Bound { source.named() ); } else { - serial::panic_raw(b"panic: rebooting unless a key is pressed\n"); + serial::panic_registers().write(b"panic: rebooting unless a key is pressed\n"); } Bound::At(cycles) } @@ -126,7 +126,7 @@ pub fn arm(on_the_record: bool) -> Bound { source.named() ); } else { - serial::panic_raw(b"panic: holding this panel: no reset register\n"); + serial::panic_registers().write(b"panic: holding this panel: no reset register\n"); } Bound::Held } @@ -137,7 +137,7 @@ pub fn arm(on_the_record: bool) -> Bound { nothing here can time a wait" ); } else { - serial::panic_raw(b"panic: holding this panel: no clock\n"); + serial::panic_registers().write(b"panic: holding this panel: no clock\n"); } Bound::Held } @@ -147,7 +147,7 @@ pub fn arm(on_the_record: bool) -> Bound { /// Return the machine to firmware. The second of this path's two lines, and it /// goes out raw: the log has already been flushed and drained by here. pub fn reboot_now() -> ! { - serial::panic_raw( + serial::panic_registers().write( b"\npanic: no key inside the bound, so nobody is here: returning this machine to \ firmware\n", ); diff --git a/kernel/src/smp_roster.rs b/kernel/src/smp_roster.rs index c881952f1d6..7436176bcbf 100644 --- a/kernel/src/smp_roster.rs +++ b/kernel/src/smp_roster.rs @@ -1,13 +1,14 @@ //! The CPU roster and the one release/answer word, two invariants held as a type: //! an id commits only after its AP's handshake and `commit` publishes the slot //! before the count, so `0..count()` has no dead slot; and the word `release` sets -//! is the word `answering` reads. Compiled into `kernel-loom/`, so no `crate::`. +//! is the word `answering` reads. Of the kernel it names only +//! `crate::arch::cpu::hardware_id`, which `kernel-loom` supplies to compile this file. #[cfg(not(feature = "loom"))] -use core::sync::atomic::{AtomicBool, AtomicU32, Ordering}; +use core::sync::atomic::{AtomicBool, AtomicU32, AtomicU64, Ordering}; #[cfg(feature = "loom")] -use loom::sync::atomic::{AtomicBool, AtomicU32, Ordering}; +use loom::sync::atomic::{AtomicBool, AtomicU32, AtomicU64, Ordering}; /// Matches `sched::MAX_CPUS`; the roster refuses an id at or above it. pub const MAX_CPUS: usize = 8; @@ -43,8 +44,9 @@ pub struct Roster { /// Source of per-attempt tokens; `0` is "no attempt". next_token: AtomicU32, /// The token the latest-started AP echoed, not a flag, so a stale AP cannot - /// be read as this one; `0` means none has. - echoed: AtomicU32, + /// be read as this one; `0` means none has. Its low half is the hardware id + /// that AP reads as its own, one store with the token. + echoed: AtomicU64, } impl Roster { @@ -58,7 +60,7 @@ impl Roster { #[cfg(feature = "smp-ready-split")] answer: AtomicBool::new(false), next_token: AtomicU32::new(1), - echoed: AtomicU32::new(0), + echoed: AtomicU64::new(0), } } @@ -72,7 +74,7 @@ impl Roster { #[cfg(feature = "smp-ready-split")] answer: AtomicBool::new(false), next_token: AtomicU32::new(1), - echoed: AtomicU32::new(0), + echoed: AtomicU64::new(0), } } @@ -101,16 +103,18 @@ impl Roster { Some(Attempt { id, token }) } - /// The AP's half of the handshake, once it can take its first interrupt: - /// the token of the attempt that started it. + /// The AP's half of the handshake, run on that AP once it can take its + /// first interrupt: the token of the attempt that started it, and the + /// hardware id this CPU reads as its own. pub fn echo(&self, token: u32) { - self.echoed.store(token, Ordering::Release); + let read = crate::arch::cpu::hardware_id(); + self.echoed.store((u64::from(token) << 32) | u64::from(read), Ordering::Release); } /// Whether `at`'s AP has echoed; an acquire, so what the AP did before its /// echo is visible after. pub fn echoed(&self, at: Attempt) -> bool { - self.echoed.load(Ordering::Acquire) == at.token + self.echoed.load(Ordering::Acquire) >> 32 == u64::from(at.token) } /// Whether `at`'s AP echoed before `spent` said the BSP's budget for it is gone. @@ -127,9 +131,23 @@ impl Roster { } /// Fill a started AP's slot, then publish the count that covers it. Only the - /// BSP calls this, one at a time, so `at.id` is the current count. + /// BSP calls this, one at a time and once `at` has echoed, so `at.id` is the + /// current count and the echo is `at`'s. + /// + /// Refuses an AP that reads its own hardware id as other than + /// `hardware_id`, the id its slot and every IPI name it by: its fatal paths + /// and the console lock name it by its read. The BSP refuses, because before + /// the release an AP's own panic stops no other CPU. pub fn commit(&self, at: Attempt, hardware_id: u32) { debug_assert!(at.id == self.count.load(Ordering::Relaxed)); + // Relaxed: the read is the echo's own word, which `await_echo` acquired. + let read = self.echoed.load(Ordering::Relaxed) as u32; + assert_eq!( + read, + hardware_id, + "smp: cpu{} reads its own hardware id as {read:#x}, and its roster slot and every IPI name it {hardware_id:#x}", + at.id + ); self.hardware_ids[at.id as usize].store(hardware_id, Ordering::Relaxed); // Release: the slot store above lands before the count exposes it; the // control drops it to relaxed and the model finds the unfilled slot. diff --git a/tests/checks.rs b/tests/checks.rs index 05f9544e77d..180ca9a8ebd 100644 --- a/tests/checks.rs +++ b/tests/checks.rs @@ -274,6 +274,59 @@ mod checks { Ok(()) } + /// `nested_nmi_is_loud`'s verdict on a whole report, on the splice CI's KVM + /// lane recorded before the report held the console registers, on another + /// CPU's burst inside each of the report's three lines, and on each line + /// the kernel writes when they were not clean — held to the kernel's own + /// words, which nothing links this crate to. + #[test] + fn nested_nmi_verdict() -> Result<(), String> { + const WHOLE: &str = "[kernel 0.385 cpu1] CPU 1: jo\n\ + [nmi] NESTED NMI on cpu 0: a second NMI entered while IST2 was still in use.\n\ + [nmi] rip=0xffffffff8012d3a0 rsp=0xffff80000017df50\n\ + [nmi] the outer handler's frame is gone; the machine stops here.\n\ + ining scheduler\n\ + [kernel 0.390 cpu0] panic: rebooting in 60 s unless a key is pressed\n"; + const SPLICED: &str = "[[kenrnmel i0.38]5 cpNu1E] CSPUT 1E: Djo inNiMngI s choednule r\n\ + c[pkeurn el0 0:.3 85a cp u1s] sechcedo: ncpud=1 rNeaMdyI=0 deyinngt=0e srtoeppded= 0 wpahrkield=0e c urrIentS=NTon2e trwipas=s1\n \ + stil[lke rnieln 0 .3u87s cep.u1\n\ + ] [i8n04m2:i ar]me d a t r37i2mps,= i0dlex fatf 38f7mfs,8 0 0in0te0r7rubpt3s 9\u{2014} 1th6e cp5in hras snevper= as0sxerftefd f(kbfd 8GSI0 10, a0ux0 G0SI 612)0\n\ + df50\n\ + [nmi] the outer handler's frame is gone; the machine stops here.\n\ + [kernel 0.390 cpu0] panic: rebooting in 60 s unless a key is pressed\n"; + if faults::report(WHOLE)? != WHOLE.lines().nth(1).unwrap_or_default() { + return Err("a whole report's first line was not the one answered".into()); + } + if faults::report(SPLICED).is_ok() { + return Err("the recorded splice was read as a whole report".into()); + } + // A burst of another CPU's line with no line end of its own, cut into + // the middle of each report line in turn (on the first, behind the + // prefix the report is found by): no line moves, so only the cut + // line's own shape can refuse it. + let lines: Vec<&str> = WHOLE.lines().collect(); + for at in 1..=3 { + let (head, tail) = lines[at].split_at(lines[at].len() / 2); + let cut = format!("{head}[kernel 0.386 cp{tail}"); + let mut spliced = lines.clone(); + spliced[at] = &cut; + if faults::report(&spliced.join("\n")).is_ok() { + return Err(format!("a burst inside the report's line {at} was read as a whole report")); + } + } + let serial = Path::new(env!("CARGO_MANIFEST_DIR")).join("kernel/src/drivers/serial.rs"); + let source = std::fs::read_to_string(&serial).map_err(|e| format!("{}: {e}", serial.display()))?; + for said in faults::UNCLEAN { + if !source.contains(said) { + return Err(format!("{} writes no {said:?}, so the test refusing it refuses nothing", serial.display())); + } + if faults::report(&format!("\n{said}; and so on\n{WHOLE}")).is_ok() { + return Err(format!("a capture saying {said:?} was read as clean")); + } + } + Ok(()) + } + /// [`control_regs`] against machines this host cannot boot, with no guest. /// /// What is here is the states no actuator reaches — a CPU that differs from diff --git a/tests/common/faults.rs b/tests/common/faults.rs index 4b7052fa154..bf0b63cb70c 100644 --- a/tests/common/faults.rs +++ b/tests/common/faults.rs @@ -3,9 +3,20 @@ use std::path::Path; use super::qemu::{self, BootOptions, QemuInstance}; use super::serial::Serial; -/// The line `nested_nmi` writes to the UART, and this boot's ready marker: the -/// machine halts on it. -const NESTED: &str = "NESTED NMI"; +/// The first and the last line of the report `nested_nmi` writes to the UART. +const NESTED: &str = "[nmi] NESTED NMI on cpu "; +const STOPS: &str = "[nmi] the outer handler's frame is gone; the machine stops here."; + +/// The halt's flush ends on the reboot's arm line, which is this boot's ready +/// marker. +const ARMED: &str = "panic: rebooting in"; + +/// What `kernel/src/drivers/serial.rs` writes when the report or the halt's +/// flush could not have the console registers clean. +pub const UNCLEAN: [&str; 2] = [ + "[serial] the console registers stayed held through the bound", + "[serial] this cpu's own fatal path held the console registers", +]; pub fn nested_nmi_is_loud(test_config: &Path) -> Result<(), String> { let qemu = QemuInstance::boot_with_options( @@ -19,15 +30,46 @@ pub fn nested_nmi_is_loud(test_config: &Path) -> Result<(), String> { // (`arch::idt::nmi`) — so on any other profile it lands on a UART // nothing here is reading. profile: qemu::Profile::Metal, - ready_marker: NESTED, + ready_marker: ARMED, ..Default::default() }, ); - let loud = Serial::boot(&qemu).must_say(NESTED)?.to_string(); - eprintln!(" [nmi] nested: {}", loud.trim()); + let serial = Serial::boot(&qemu); + let first = report(serial.text())?; + eprintln!(" [nmi] nested: {first}"); Ok(()) } +/// `nested_nmi`'s first line, once its three are whole and back to back and +/// nothing in `capture` says the registers were not clean. +pub fn report(capture: &str) -> Result<&str, String> { + let lines: Vec<&str> = capture.lines().collect(); + let at = lines.iter().position(|l| l.starts_with(NESTED)); + let Some(report) = at.and_then(|at| lines.get(at..at + 3)).filter(|r| whole(r)) else { + return Err(format!("the nested-NMI report reached the console spliced:\n{capture}")); + }; + if let Some(unclean) = lines.iter().find(|l| UNCLEAN.iter().any(|said| l.contains(said))) { + return Err(format!("the report's registers were not clean: {unclean}\n{capture}")); + } + Ok(report[0]) +} + +/// `nested_nmi`'s three lines, each exactly as it writes them: a burst another +/// CPU put on the 16550 inside the report splits one. +fn whole(report: &[&str]) -> bool { + let [first, registers, last] = report else { return false }; + let hex = |v: &str| v.len() == 18 && v.starts_with("0x") && v[2..].bytes().all(|b| b.is_ascii_hexdigit()); + first + .strip_prefix(NESTED) + .and_then(|rest| rest.strip_suffix(": a second NMI entered while IST2 was still in use.")) + .is_some_and(|cpu| cpu.parse::().is_ok()) + && registers + .strip_prefix("[nmi] rip=") + .and_then(|rest| rest.split_once(" rsp=")) + .is_some_and(|(rip, rsp)| hex(rip) && hex(rsp)) + && *last == STOPS +} + /// The blocked-task dump's NMI probe: a CPU that ignores a kick is named, and /// then asked where it is with the one interrupt it cannot mask. /// diff --git a/tests/common/serial.rs b/tests/common/serial.rs index edc8bcaf92b..dc37aeca40e 100644 --- a/tests/common/serial.rs +++ b/tests/common/serial.rs @@ -108,7 +108,7 @@ pub(crate) const DEATHS: &[(&str, Died, Died)] = &[ // capture can carry it twice. ("DOUBLE PANIC", Died::Kernel, Died::Kernel), // kernel/src/main.rs — the reentry guard, written straight out the UART - // port with no lock and therefore with no prefix. It reaches the 16550 log + // port with no prefix. It reaches the 16550 log // rather than the console, and is here so that a capture carrying it is // never read as anything else. ("PANIC REENTRY", Died::Kernel, Died::Kernel),