From 21e3fc86f24da704abdfeef10c6cb99d177fcdd1 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 1 Oct 2026 15:45:30 +0200 Subject: [PATCH 01/10] The nested-NMI report holds the console's registers, so another CPU's burst never lands inside it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `nested_nmi` wrote its report through `serial::panic_raw`, which takes no lock, before `halt_all_cpus` stops any CPU. Under KVM in CI the report on cpu0 and klogd's records on cpu1 went onto the 16550 into each other a byte at a time, and `nested_nmi_is_loud` went red twice: on #671's `guest` check (run 36863809437) it timed out with `NESTED NMI` never whole, and on main's nightly `guest` lane at 06788146b (run 36843762360) the capture stopped short of the report's tail, just before a `—` the splice could cut apart (the harness's reader stops at a line that is not UTF-8), and the boot waited out the panic bound until QEMU exited. The first spliced line of the first run is exactly a byte interleaving of `[kernel 0.385 cpu1] CPU 1: joining scheduler` and `[nmi] NESTED NMI on `: a shuffle check over the captured bytes says so, and says no with either string one byte off. The console's contract was the kernel's to keep, not the harness's to read around: the registers' lock is held for one burst, klogd writes every UART byte under it, and the panic path takes the registers alone and bypasses them only once they stay held. `panic_flush` did that; the report did not. A raw report carries no tag a reader could sort by, and a line spliced byte by byte is nothing a reader can undo. - `serial::panic_registers` is `panic_flush`'s bounded wait for the registers, lifted out unchanged: `None` once the bound runs out. `panic_flush` calls it. - `nested_nmi` holds what it returns for the whole report, and drops it before `halt_all_cpus`, whose flush takes the registers again. A hold the context it interrupted left on its own CPU delays the report by the bound and does not stop it. - `nested_nmi_is_loud` waits for the report's last line and asserts all three lines whole and back to back. It asserted only that `NESTED NMI` appeared, and the first red carried the last line whole and the first spliced, which it took a 39-second timeout to say. Filed, not fixed here: - issues/panic-path/a-fatal-report-written-raw-can-splice-into-another-cpus-console-burst.md: `panic::last_words` and the double fault's `[ist1]` line still write raw while another CPU can write, and `panic_registers` as it stands is not their fix. - issues/panic-path/a-panic-write-between-two-console-bursts-can-split-a-character.md: a burst can end inside a multibyte character, and the panic path writes between bursts. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- ...-splice-into-another-cpus-console-burst.md | 30 ++++++++++++++++ ...wo-console-bursts-can-split-a-character.md | 28 +++++++++++++++ kernel/src/arch/x86_64/idt/nmi.rs | 6 +++- kernel/src/drivers/serial.rs | 21 ++++++++---- tests/common/faults.rs | 34 +++++++++++++++---- 5 files changed, 106 insertions(+), 13 deletions(-) create mode 100644 issues/panic-path/a-fatal-report-written-raw-can-splice-into-another-cpus-console-burst.md create mode 100644 issues/panic-path/a-panic-write-between-two-console-bursts-can-split-a-character.md diff --git a/issues/panic-path/a-fatal-report-written-raw-can-splice-into-another-cpus-console-burst.md b/issues/panic-path/a-fatal-report-written-raw-can-splice-into-another-cpus-console-burst.md new file mode 100644 index 00000000000..90aaf87400a --- /dev/null +++ b/issues/panic-path/a-fatal-report-written-raw-can-splice-into-another-cpus-console-burst.md @@ -0,0 +1,30 @@ +--- +status: open +kind: defect +opened: 2026-10-01 +--- + +# A fatal report written raw can splice into another CPU's console burst + +`serial::panic_raw` takes no lock, so a report written through it while +another CPU writes the console lands on the 16550 inside that CPU's burst, byte +by byte. `nested_nmi` holds the registers for its report +(`serial::panic_registers`); these do not: + +- `panic::last_words`, on both of its arms, `PANIC REENTRY` and `DOUBLE + PANIC`, written before any CPU is stopped. The reentry arm never stops them. +- `percpu::ist1_report`, after `halt_all_cpus`' flush. A CPU that spun for the + registers with `IF` clear through the flush takes them as the flush ends, + and its burst goes out beside the `[ist1]` line before it takes the halt IPI. + +Taking them in `last_words` with `panic_registers` is not the fix: a reentry +inside `panic_flush`'s drain would find them held by its own CPU's outer frame, +and wait the whole bound for a holder that never runs again. `panic_flush` and +`nested_nmi` already pay that wait when the hold is their own CPU's. + +**Evidence:** the code. CI's KVM `guest` lane recorded the mechanism on +`nested_nmi`'s report before it held the registers: run 36863809437, job +110375742604. + +**Exit:** every raw report holds the registers for its whole length, and no +fatal path waits out a hold its own CPU left. 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/src/arch/x86_64/idt/nmi.rs b/kernel/src/arch/x86_64/idt/nmi.rs index 2ef04370a92..65b61a94de3 100644 --- a/kernel/src/arch/x86_64/idt/nmi.rs +++ b/kernel/src/arch/x86_64/idt/nmi.rs @@ -137,8 +137,10 @@ 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) -> ! { + // Held for the whole report, so no other CPU's console burst lands inside it; bounded, so a + // hold the interrupted context left only delays it. + let registers = crate::drivers::serial::panic_registers(); 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())); @@ -147,5 +149,7 @@ extern "sysv64" fn nested_nmi(rip: u64, rsp: u64) -> ! { 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"); + // Before the halt, whose flush waits for the registers. + drop(registers); crate::panic::halt_all_cpus() } diff --git a/kernel/src/drivers/serial.rs b/kernel/src/drivers/serial.rs index 10a9acd2f5f..8c469651a02 100644 --- a/kernel/src/drivers/serial.rs +++ b/kernel/src/drivers/serial.rs @@ -128,6 +128,18 @@ 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 registers for the panic path: a live burst holder is waited for, and +/// one that never releases them is bypassed, which is `None`. +pub fn panic_registers() -> Option { + for _ in 0..PANIC_LOCK_SPIN_LIMIT { + if let Some(g) = BackendGuard::try_lock() { + return Some(g); + } + core::hint::spin_loop(); + } + None +} + /// Flushes pending logs on the panic path. /// /// Waits for a live guard holder to release before bypassing it — bypassing @@ -142,12 +154,9 @@ 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(); + if let Some(mut g) = panic_registers() { + crate::log::console::drain_locked(&mut g); + return; } // Disables virtio-console first: a half-submitted TX queue would panic // recursively if a bypassing write reached it. diff --git a/tests/common/faults.rs b/tests/common/faults.rs index 4b7052fa154..e1d87b01815 100644 --- a/tests/common/faults.rs +++ b/tests/common/faults.rs @@ -3,9 +3,9 @@ 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 last line of the report `nested_nmi` writes to the UART, and this boot's +/// ready marker: the machine halts on it. +const STOPS: &str = "[nmi] the outer handler's frame is gone; the machine stops here."; pub fn nested_nmi_is_loud(test_config: &Path) -> Result<(), String> { let qemu = QemuInstance::boot_with_options( @@ -19,15 +19,37 @@ 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: STOPS, ..Default::default() }, ); - let loud = Serial::boot(&qemu).must_say(NESTED)?.to_string(); - eprintln!(" [nmi] nested: {}", loud.trim()); + let serial = Serial::boot(&qemu); + let lines: Vec<&str> = serial.text().lines().collect(); + // The capture ends at the marker. + let report = &lines[lines.len().saturating_sub(3)..]; + if !whole(report) { + return Err(format!("the nested-NMI report reached the console spliced:\n{}", serial.text())); + } + eprintln!(" [nmi] nested: {}", report[0]); Ok(()) } +/// `nested_nmi`'s three lines back to back, 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("[nmi] NESTED NMI on cpu ") + .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. /// From f9835df2fac5bed481a65c7628bd0cdde566be56 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 1 Oct 2026 16:50:15 +0200 Subject: [PATCH 02/10] Every fatal path writes the console through registers it asked for, and re-enters its own CPU's hold Review round 2 of #675. The type. `serial::PanicUart` is the one way a fatal path writes the console UART, and `serial::panic_registers` is its one constructor: `panic_raw`, `panic_raw_hex` and `panic_raw_dec` are gone. `panic::last_words` (both dead ends), `percpu::ist1_report`, `panic_reboot::arm`'s three raw lines and `panic_reboot::reboot_now` now hold the registers for their lines, beside `nested_nmi` and `panic_flush`. A write that does not ask no longer compiles. The lock. The bounded acquire moved into `serial_lock.rs` as `BackendLock::seize`, where kernel-loom drives it. The word now says who holds it: free, a burst holder (`LIVE`), or a fatal path's CPU. A fatal path entered on top of its own CPU's fatal hold re-enters at once instead of waiting for a frame that never runs again; that was a reentry inside `panic_flush`'s drain, and the report-then-halt sequence. Another holder is waited out for `PANIC_LOCK_SPIN_LIMIT` tries, and the expiry now says itself in one raw line. `within` is the console's one bounded wait, so `flush_final`'s copy of the loop goes. `lock()` is built on `try_lock()`, which lost its last other caller. `panic_flush` drains through virtio-console only under registers it took clean. Under its own CPU's fatal hold the outer path may have stopped inside a virtqueue publish, so it says so and drains raw, as it does past the bound. `nested_nmi` lets its report's registers go before the halt for that reason. The test. `nested_nmi_is_loud` now reads to the halt's flush, ending on the reboot's arm line, and refuses both lines the registers say when they were not clean. Its verdict is `faults::report`, which `tests/checks.rs` runs on a whole report, on the splice CI's KVM lane recorded before the report held the registers (run 36863809437, job 110375742604), and on each refused line, holding the refused words to `kernel/src/drivers/serial.rs`. Mutations, each a checked patch, built, run and restored: - `seize` takes the lock unbounded (the review's `Some(BackendGuard::lock())` at its new home): build 0, kernel-loom `serial_lock` 101. - `within` loses its bound: build 0, 101. - `seize` loses its re-entry arm: build 0, 101. - the kernel's re-entry line drifts from the harness's: the check 101. Issues: `a-fatal-report-written-raw-can-splice-into-another-cpus-console-burst` closes, since every raw report now holds the registers; what it left is filed as `a-fatal-path-inside-its-own-cpus-console-burst-waits-out-the-bound`: a burst holder names no CPU. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- ...-cpus-console-burst-waits-out-the-bound.md | 20 +++ ...-splice-into-another-cpus-console-burst.md | 30 ---- kernel-loom/tests/serial_lock.rs | 57 ++++++- kernel/src/arch/x86_64/idt/nmi.rs | 28 ++-- kernel/src/arch/x86_64/percpu.rs | 11 +- kernel/src/drivers/serial.rs | 152 ++++++++++-------- kernel/src/drivers/serial_lock.rs | 71 ++++++-- kernel/src/log/console.rs | 20 +-- kernel/src/panic.rs | 85 +++++----- kernel/src/panic_reboot.rs | 8 +- tests/checks.rs | 38 +++++ tests/common/faults.rs | 46 ++++-- tests/common/serial.rs | 2 +- 13 files changed, 368 insertions(+), 200 deletions(-) create mode 100644 issues/panic-path/a-fatal-path-inside-its-own-cpus-console-burst-waits-out-the-bound.md delete mode 100644 issues/panic-path/a-fatal-report-written-raw-can-splice-into-another-cpus-console-burst.md 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-fatal-report-written-raw-can-splice-into-another-cpus-console-burst.md b/issues/panic-path/a-fatal-report-written-raw-can-splice-into-another-cpus-console-burst.md deleted file mode 100644 index 90aaf87400a..00000000000 --- a/issues/panic-path/a-fatal-report-written-raw-can-splice-into-another-cpus-console-burst.md +++ /dev/null @@ -1,30 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-10-01 ---- - -# A fatal report written raw can splice into another CPU's console burst - -`serial::panic_raw` takes no lock, so a report written through it while -another CPU writes the console lands on the 16550 inside that CPU's burst, byte -by byte. `nested_nmi` holds the registers for its report -(`serial::panic_registers`); these do not: - -- `panic::last_words`, on both of its arms, `PANIC REENTRY` and `DOUBLE - PANIC`, written before any CPU is stopped. The reentry arm never stops them. -- `percpu::ist1_report`, after `halt_all_cpus`' flush. A CPU that spun for the - registers with `IF` clear through the flush takes them as the flush ends, - and its burst goes out beside the `[ist1]` line before it takes the halt IPI. - -Taking them in `last_words` with `panic_registers` is not the fix: a reentry -inside `panic_flush`'s drain would find them held by its own CPU's outer frame, -and wait the whole bound for a holder that never runs again. `panic_flush` and -`nested_nmi` already pay that wait when the hold is their own CPU's. - -**Evidence:** the code. CI's KVM `guest` lane recorded the mechanism on -`nested_nmi`'s report before it held the registers: run 36863809437, job -110375742604. - -**Exit:** every raw report holds the registers for its whole length, and no -fatal path waits out a hold its own CPU left. diff --git a/kernel-loom/tests/serial_lock.rs b/kernel-loom/tests/serial_lock.rs index 58613469a37..22cc26fd452 100644 --- a/kernel-loom/tests/serial_lock.rs +++ b/kernel-loom/tests/serial_lock.rs @@ -1,4 +1,6 @@ -//! 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, +//! and a fatal path's `seize` gives up on a holder that never lets go but not +//! on its own CPU's. //! //! 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. @@ -7,7 +9,7 @@ //! this file. #![cfg(feature = "loom")] -use kernel_loom::serial_lock::BackendLock; +use kernel_loom::serial_lock::{BackendLock, Seized}; use loom::cell::UnsafeCell; use loom::sync::Arc; use loom::thread; @@ -49,3 +51,54 @@ fn two_writers_never_overlap() { other.join().unwrap(); }); } + +/// 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(|| { + let lock = BackendLock::new(); + let _burst = lock.try_lock().expect("an unheld lock refused its first taker"); + assert!(matches!(lock.seize(0, 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 = BackendLock::new(); + let Seized::Taken(held) = lock.seize(1, 1) else { panic!("a free lock refused a seize") }; + assert!(matches!(lock.seize(1, 1), Seized::Reentered), "a fatal path waited out its own CPU's hold"); + assert!(matches!(lock.seize(2, 1), Seized::Expired), "a fatal path took another CPU's hold"); + assert!(lock.try_lock().is_none(), "a burst took a fatal path's hold"); + drop(held); + assert!(matches!(lock.seize(2, 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); + } + }) + }; + if let Seized::Taken(held) = lock.seize(0, 2) { + // SAFETY: as above. + line.with_mut(|n| unsafe { *n += 1 }); + drop(held); + } + burst.join().unwrap(); + }); +} diff --git a/kernel/src/arch/x86_64/idt/nmi.rs b/kernel/src/arch/x86_64/idt/nmi.rs index 65b61a94de3..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; @@ -138,18 +137,17 @@ 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. extern "sysv64" fn nested_nmi(rip: u64, rsp: u64) -> ! { - // Held for the whole report, so no other CPU's console burst lands inside it; bounded, so a - // hold the interrupted context left only delays it. - let registers = crate::drivers::serial::panic_registers(); - 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"); - // Before the halt, whose flush waits for the registers. - drop(registers); + // 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/drivers/serial.rs b/kernel/src/drivers/serial.rs index 8c469651a02..7594d1b2a3c 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}; @@ -80,13 +80,6 @@ impl BackendGuard { 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 }) - } - /// Writes raw bytes with no escape stripping. pub fn write_raw(&mut self, bytes: &[u8]) { match backend() { @@ -128,23 +121,85 @@ 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 registers for the panic path: a live burst holder is waited for, and -/// one that never releases them is bypassed, which is `None`. -pub fn panic_registers() -> Option { - for _ in 0..PANIC_LOCK_SPIN_LIMIT { - if let Some(g) = BackendGuard::try_lock() { - return Some(g); +/// The console UART on a fatal path, and the one way a fatal path writes it. +/// Never held across a `log!`: before `klogd` runs, a record drains inline, +/// on this CPU, through these 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 { _irq: IrqGuard }, + /// Another holder kept them through the whole bound, which was said. + Expired { _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(crate::arch::cpu::hardware_id(), PANIC_LOCK_SPIN_LIMIT) { + Seized::Taken(held) => Hold::Held(BackendGuard { _held: held, _irq: irq }), + Seized::Reentered => Hold::Reentered { _irq: irq }, + Seized::Expired => { + uart_write_bytes(WRITTEN_OVER); + Hold::Expired { _irq: irq } + } + }) +} + +impl PanicUart { + /// Straight to the UART, never virtio-console: no allocation, bounded per byte. + pub fn write(&mut self, bytes: &[u8]) { + uart_write_bytes(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(&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]; } - core::hint::spin_loop(); + uart_write_bytes(&out[..n]); } - None } /// 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. @@ -154,19 +209,24 @@ pub unsafe fn panic_flush() { if !has_console() { return; } - if let Some(mut g) = panic_registers() { - crate::log::console::drain_locked(&mut g); + let mut uart = panic_registers(); + if let Hold::Held(registers) = &mut uart.0 { + crate::log::console::drain_locked(registers); 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. if !uart_present() { 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 @@ -177,12 +237,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!( @@ -387,38 +444,3 @@ fn uart_write_bytes(bytes: &[u8]) { 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_bytes(&out[..n]); -} diff --git a/kernel/src/drivers/serial_lock.rs b/kernel/src/drivers/serial_lock.rs index 6928a93ce80..f690f9e43be 100644 --- a/kernel/src/drivers/serial_lock.rs +++ b/kernel/src/drivers/serial_lock.rs @@ -1,47 +1,64 @@ -//! 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. +//! 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. //! No `crate::` references: `kernel-loom` compiles this file directly under `feature = "loom"`. #[cfg(not(feature = "loom"))] -use core::sync::atomic::{AtomicBool, Ordering}; +use core::sync::atomic::{AtomicU64, Ordering}; #[cfg(feature = "loom")] -use loom::sync::atomic::{AtomicBool, Ordering}; +use loom::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 lock, held for as long as this lives. 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) } } 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) { + loop { + if let Some(held) = self.try_lock() { + return held; + } + while self.word.load(Ordering::Relaxed) != FREE { core::hint::spin_loop(); } } - Held { lock: self } } /// `None` if another holder has it. @@ -49,7 +66,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 +76,34 @@ impl BackendLock { None } } + + /// The lock for the fatal path running on `cpu`, asked for at most `tries` + /// times: nothing on that path may wait for ever. + pub fn seize(&self, cpu: u32, tries: u64) -> Seized<'_> { + let mine = FATAL + u64::from(cpu); + 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 console's one bounded wait. +pub fn within(tries: u64, mut attempt: impl FnMut() -> Option) -> Option { + for _ in 0..tries { + if let Some(answer) = attempt() { + return Some(answer); + } + core::hint::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/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..790da630bbc 100644 --- a/kernel/src/panic.rs +++ b/kernel/src/panic.rs @@ -268,50 +268,53 @@ 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. + { + 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/tests/checks.rs b/tests/checks.rs index 05f9544e77d..eaf83642f4e 100644 --- a/tests/checks.rs +++ b/tests/checks.rs @@ -274,6 +274,44 @@ 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, 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()); + } + 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 e1d87b01815..bf0b63cb70c 100644 --- a/tests/common/faults.rs +++ b/tests/common/faults.rs @@ -3,10 +3,21 @@ use std::path::Path; use super::qemu::{self, BootOptions, QemuInstance}; use super::serial::Serial; -/// The last line of the report `nested_nmi` writes to the UART, and this boot's -/// ready marker: the machine halts on it. +/// 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( test_config, @@ -19,28 +30,37 @@ 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: STOPS, + ready_marker: ARMED, ..Default::default() }, ); let serial = Serial::boot(&qemu); - let lines: Vec<&str> = serial.text().lines().collect(); - // The capture ends at the marker. - let report = &lines[lines.len().saturating_sub(3)..]; - if !whole(report) { - return Err(format!("the nested-NMI report reached the console spliced:\n{}", serial.text())); - } - eprintln!(" [nmi] nested: {}", report[0]); + let first = report(serial.text())?; + eprintln!(" [nmi] nested: {first}"); Ok(()) } -/// `nested_nmi`'s three lines back to back, each exactly as it writes them: a -/// burst another CPU put on the 16550 inside the report splits one. +/// `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("[nmi] NESTED NMI on cpu ") + .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 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), From 7d6a15233777890ea599066b2686c195c270ea1a Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 1 Oct 2026 17:55:51 +0200 Subject: [PATCH 03/10] Review round 3: the console lock reads its own CPU and refuses a burst under its fatal hold; the UART moves a byte only with Registers The round-2 review sent back six findings. Each is answered here. The wait inside the bound now has a model. `a_seize_takes_a_burst_let_go_inside_the_bound` holds a burst when the seize begins and lets it go on another thread. Under `feature = "loom"`, `within`'s pause is `loom::hint::spin_loop`, imported beside the loom atomics, so the holder runs between two looks. The review asked for `seize(_, 2)` with `Taken` in every execution, and under loom's default exploration that cannot hold for any count of tries. Measured: with `LOOM_MAX_PREEMPTIONS=0` it is green, and with 1 or 2 it is red. Loom's trace of the red execution shows the seize looking, yielding to the holder, and then running again before the holder's store executes. That is the holder preempted by the waiter, and each such preemption costs the seize one try. So the model runs at preemption bounds 0, 1 and 2, with k + 2 tries at bound k. The console lock reads which CPU it is on through `crate::arch::cpu::hardware_id`. kernel-loom supplies that function as a per-model-thread shim (`arch::cpu::become_cpu`). With this, no call site passes a CPU id, and the read the review mutated now lives in the file the models compile. Mutated to 0 there, it fails two models, which put two CPUs on one fatal hold. `BackendLock::lock` refuses, by assertion, a burst on a CPU whose own fatal path holds the registers. Holding `PanicUart` across a `log!` before klogd runs therefore panics into the reentry guard's report instead of spinning for ever on the CPU's own FATAL word. The patch the review named, braces dropped in `last_words`, still compiles. Under it the hang becomes that loud stop. The refusal is held by `a_burst_beneath_its_own_cpus_fatal_path_is_refused`. The other CPUs' side is held by `a_burst_waits_out_another_cpus_fatal_path`. `arch::console_uart::{write_byte, read_byte}` take a `&mut serial::Registers`. Only serial.rs builds one, for a `BackendGuard` or a `PanicUart`. virtio-console's three `*_locked` functions take the `BackendGuard` itself, which closes the review's NOTE by the same mechanism. `nested_nmi_verdict` now also reads another CPU's burst cut into each of the report's three lines, so `whole` is held on the host. The false sentence in kernel-loom/tests/serial_lock.rs's header is deleted. Filed: - issues/kernel/a-safe-mmio-window-reaches-any-physical-address.md - issues/panic-path/a-panic-reentry-inside-a-fatal-paths-console-hold-halts-with-it-held.md Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- ...mio-window-reaches-any-physical-address.md | 20 ++++ ...l-paths-console-hold-halts-with-it-held.md | 20 ++++ kernel-loom/src/lib.rs | 23 +++++ kernel-loom/tests/serial_lock.rs | 94 ++++++++++++++++--- kernel/src/arch/aarch64/console_uart.rs | 5 +- kernel/src/arch/x86_64/console_uart.rs | 5 +- kernel/src/drivers/serial.rs | 73 +++++++++----- kernel/src/drivers/serial_lock.rs | 39 +++++--- kernel/src/drivers/virtio_console.rs | 12 +-- kernel/src/panic.rs | 3 +- tests/checks.rs | 19 +++- 11 files changed, 249 insertions(+), 64 deletions(-) create mode 100644 issues/kernel/a-safe-mmio-window-reaches-any-physical-address.md create mode 100644 issues/panic-path/a-panic-reentry-inside-a-fatal-paths-console-hold-halts-with-it-held.md 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/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/kernel-loom/src/lib.rs b/kernel-loom/src/lib.rs index adfd920c320..d20776a4419 100644 --- a/kernel-loom/src/lib.rs +++ b/kernel-loom/src/lib.rs @@ -101,6 +101,29 @@ pub mod arch { pub fn poll() {} } + /// Which CPU a model thread is, which the console lock's fatal word names. + /// 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. + pub mod cpu { + #[cfg(feature = "loom")] + loom::thread_local! { + static CPU: core::cell::Cell> = core::cell::Cell::new(None); + } + #[cfg(not(feature = "loom"))] + std::thread_local! { + static CPU: core::cell::Cell> = const { 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.** /// diff --git a/kernel-loom/tests/serial_lock.rs b/kernel-loom/tests/serial_lock.rs index 22cc26fd452..7d4e8b2e10e 100644 --- a/kernel-loom/tests/serial_lock.rs +++ b/kernel-loom/tests/serial_lock.rs @@ -1,14 +1,14 @@ -//! Loom: a lost `try_lock` on the console backend leaves its holder holding, -//! and a fatal path's `seize` gives up on a holder that never lets go but not -//! on its own CPU's. +//! 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::arch::cpu::become_cpu; use kernel_loom::serial_lock::{BackendLock, Seized}; use loom::cell::UnsafeCell; use loom::sync::Arc; @@ -52,14 +52,42 @@ fn two_writers_never_overlap() { }); } +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)); + assert!( + matches!(LOCK.seize(u64::from(preemptions) + 2), Seized::Taken(_)), + "a fatal path gave up on a burst that let go inside the bound" + ); + holder.join().unwrap(); + }); + } +} + /// 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(0, 3), Seized::Expired), "a seize took a lock its holder kept"); + assert!(matches!(lock.seize(3), Seized::Expired), "a seize took a lock its holder kept"); }); } @@ -68,13 +96,21 @@ fn a_seize_gives_up_on_a_holder_that_never_lets_go() { #[test] fn a_seize_reenters_its_own_cpus_hold() { loom::model(|| { - let lock = BackendLock::new(); - let Seized::Taken(held) = lock.seize(1, 1) else { panic!("a free lock refused a seize") }; - assert!(matches!(lock.seize(1, 1), Seized::Reentered), "a fatal path waited out its own CPU's hold"); - assert!(matches!(lock.seize(2, 1), Seized::Expired), "a fatal path took another CPU's hold"); + 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(2, 1), Seized::Taken(_)), "a released lock stayed held"); + assert!(matches!(lock.seize(1), Seized::Taken(_)), "a released lock stayed held"); }); } @@ -94,7 +130,8 @@ fn a_seize_and_a_burst_never_overlap() { } }) }; - if let Seized::Taken(held) = lock.seize(0, 2) { + become_cpu(0); + if let Seized::Taken(held) = lock.seize(2) { // SAFETY: as above. line.with_mut(|n| unsafe { *n += 1 }); drop(held); @@ -102,3 +139,36 @@ fn a_seize_and_a_burst_never_overlap() { 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/src/arch/aarch64/console_uart.rs b/kernel/src/arch/aarch64/console_uart.rs index a0b235f0762..a3578da0277 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}; @@ -72,7 +73,7 @@ pub fn rx_ready() -> bool { } /// 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 } @@ -82,6 +83,6 @@ pub fn tx_ready() -> bool { } /// 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..7b1269dfb88 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 @@ -59,7 +60,7 @@ pub fn rx_ready() -> bool { } /// The received byte; only after [`rx_ready`] said one waits. -pub fn read_byte() -> u8 { +pub fn read_byte(_: &mut Registers) -> u8 { inb(PORT) } @@ -69,7 +70,7 @@ pub fn tx_ready() -> bool { } /// 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) { // SAFETY: `outb` requires ownership of the port and the byte; `PORT` is // COM1's own data register, and the byte is console output only. unsafe { outb(PORT, byte) }; diff --git a/kernel/src/drivers/serial.rs b/kernel/src/drivers/serial.rs index 7594d1b2a3c..e8e107c6bd5 100644 --- a/kernel/src/drivers/serial.rs +++ b/kernel/src/drivers/serial.rs @@ -66,32 +66,38 @@ pub fn backend() -> Backend { static BACKEND: BackendLock = BackendLock::new(); +/// The console UART's registers, asked for: what `arch::console_uart` moves a +/// byte with. 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 } + 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 { 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() } @@ -99,9 +105,9 @@ impl BackendGuard { pub fn try_read_byte(&mut self) -> Option { if super::virtio_console::is_ready() { - super::virtio_console::try_read_byte_locked() + super::virtio_console::try_read_byte_locked(self) } else if uart_present() && uart::rx_ready() { - Some(uart::read_byte()) + Some(uart::read_byte(&mut self.registers)) } else { None } @@ -121,18 +127,25 @@ 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 on a fatal path, and the one way a fatal path writes it. -/// Never held across a `log!`: before `klogd` runs, a record drains inline, -/// on this CPU, through these registers. +/// 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 { _irq: IrqGuard }, + Reentered(Over), /// Another holder kept them through the whole bound, which was said. - Expired { _irq: IrqGuard }, + 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. @@ -147,20 +160,28 @@ const DRAINED_RAW: &[u8] = /// 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(crate::arch::cpu::hardware_id(), PANIC_LOCK_SPIN_LIMIT) { - Seized::Taken(held) => Hold::Held(BackendGuard { _held: held, _irq: irq }), - Seized::Reentered => Hold::Reentered { _irq: irq }, + 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 => { - uart_write_bytes(WRITTEN_OVER); - Hold::Expired { _irq: irq } + 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(bytes); + uart_write_bytes(self.registers(), bytes); } /// An address, formatted as `{:#018x}` to match the rest of the crash report. @@ -171,7 +192,7 @@ impl PanicUart { 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); + uart_write_bytes(self.registers(), &out); } /// A number, since the callers cannot format one. @@ -190,7 +211,7 @@ impl PanicUart { for i in 0..n { out[i] = digits[n - 1 - i]; } - uart_write_bytes(&out[..n]); + uart_write_bytes(self.registers(), &out[..n]); } } @@ -210,11 +231,11 @@ pub unsafe fn panic_flush() { return; } let mut uart = panic_registers(); - if let Hold::Held(registers) = &mut uart.0 { - crate::log::console::drain_locked(registers); + if let Hold::Held(guard) = &mut uart.0 { + crate::log::console::drain_locked(guard); return; } - if matches!(uart.0, Hold::Reentered { .. }) { + if matches!(uart.0, Hold::Reentered(_)) { uart.write(DRAINED_RAW); } // Disables virtio-console first: a half-submitted TX queue would panic @@ -296,11 +317,11 @@ fn uart_write_fifo(bytes: &[u8]) { for chunk in bytes.chunks(uart::TX_BURST) { let mut asked = 0; loop { - let burst = BackendGuard::lock(); + let mut burst = BackendGuard::lock(); if uart::tx_ready() { // 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; } @@ -430,7 +451,7 @@ 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; } @@ -441,6 +462,6 @@ fn uart_write_bytes(bytes: &[u8]) { } core::hint::spin_loop(); } - uart::write_byte(b); + uart::write_byte(registers, b); } } diff --git a/kernel/src/drivers/serial_lock.rs b/kernel/src/drivers/serial_lock.rs index f690f9e43be..931b3f207f2 100644 --- a/kernel/src/drivers/serial_lock.rs +++ b/kernel/src/drivers/serial_lock.rs @@ -3,14 +3,15 @@ //! [`Held`], so it can never release the lock another CPU holds. //! 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. -//! No `crate::` references: `kernel-loom` compiles this file directly under `feature = "loom"`. +//! 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::{AtomicU64, Ordering}; +use core::{hint::spin_loop, sync::atomic::{AtomicU64, Ordering}}; #[cfg(feature = "loom")] -use loom::sync::atomic::{AtomicU64, Ordering}; +use loom::{hint::spin_loop, sync::atomic::{AtomicU64, Ordering}}; pub struct BackendLock { /// [`FREE`], [`LIVE`], or [`FATAL`] plus the CPU whose fatal path holds it. @@ -22,6 +23,11 @@ const FREE: u64 = 0; 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. pub struct Held<'a> { lock: &'a BackendLock, @@ -50,13 +56,21 @@ impl BackendLock { 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<'_> { loop { if let Some(held) = self.try_lock() { return held; } - while self.word.load(Ordering::Relaxed) != FREE { - core::hint::spin_loop(); + 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); } } } @@ -77,10 +91,10 @@ impl BackendLock { } } - /// The lock for the fatal path running on `cpu`, asked for at most `tries` - /// times: nothing on that path may wait for ever. - pub fn seize(&self, cpu: u32, tries: u64) -> Seized<'_> { - let mine = FATAL + u64::from(cpu); + /// 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. @@ -91,13 +105,14 @@ impl BackendLock { } } -/// `attempt` until it answers, or `tries` times: the console's one bounded wait. +/// `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); } - core::hint::spin_loop(); + spin_loop(); } None } 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/panic.rs b/kernel/src/panic.rs index 790da630bbc..5481315d3db 100644 --- a/kernel/src/panic.rs +++ b/kernel/src/panic.rs @@ -268,7 +268,8 @@ pub fn last_words( }; let second_message = second.message().as_str().unwrap_or(NOT_CAPTURED); - // Let go of before the record below. + // 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!!! "); diff --git a/tests/checks.rs b/tests/checks.rs index eaf83642f4e..ca20ea6ff72 100644 --- a/tests/checks.rs +++ b/tests/checks.rs @@ -275,9 +275,10 @@ mod checks { } /// `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, 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. + /// 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\ @@ -299,6 +300,18 @@ mod checks { if faults::report(SPLICED).is_ok() { return Err("the recorded splice was read as a whole report".into()); } + // Cut at the middle, which on the first line is behind the prefix the + // report is found by, so each cut is judged by the line's own shape. + 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 cpu1] CPU 1: joining scheduler\n{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 { From 4a3e601d18aa583d6c2aad69f2e9175be490340f Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 1 Oct 2026 17:59:40 +0200 Subject: [PATCH 04/10] nested_nmi_verdict cuts a burst with no line end into each report line, so each of whole's clauses is held alone The burst the previous commit cut into each report line carried its own line end. That pushed the next line into the cut line's place, and a later clause of `whole` refused it. So the check passed with the first-line clause removed and also with the registers clause removed (each run exit 0, measured as mutations m4b and m4c). A burst with no line end leaves every line where it was, and only the cut line's own clause can refuse it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- tests/checks.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/checks.rs b/tests/checks.rs index ca20ea6ff72..180ca9a8ebd 100644 --- a/tests/checks.rs +++ b/tests/checks.rs @@ -300,12 +300,14 @@ mod checks { if faults::report(SPLICED).is_ok() { return Err("the recorded splice was read as a whole report".into()); } - // Cut at the middle, which on the first line is behind the prefix the - // report is found by, so each cut is judged by the line's own shape. + // 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 cpu1] CPU 1: joining scheduler\n{tail}"); + 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() { From f71a5fd1981659d8b418ebf8917e85723972816c Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 1 Oct 2026 18:01:34 +0200 Subject: [PATCH 05/10] The seize model joins its holder before it asserts, so a seize that gives up fails the test instead of aborting it Under mutations m2a (`Err(_) => Some(Seized::Expired)`) and m2c (`within`'s pause as `core::hint::spin_loop`), the seize gave up before the holder thread had run. The assertion then unwound while loom still had the holder's closure queued. That closure owns the burst, whose drop stores to a loom atomic outside the model. That is a panic in a destructor during cleanup, and the binary died on SIGABRT. The run was red (exit 101), but its other tests were cut short with it. Joining the holder first runs that drop inside the model, so the failure is the assertion's own. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- kernel-loom/tests/serial_lock.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/kernel-loom/tests/serial_lock.rs b/kernel-loom/tests/serial_lock.rs index 7d4e8b2e10e..26b0732e3fe 100644 --- a/kernel-loom/tests/serial_lock.rs +++ b/kernel-loom/tests/serial_lock.rs @@ -70,11 +70,11 @@ fn a_seize_takes_a_burst_let_go_inside_the_bound() { become_cpu(0); let burst = LOCK.try_lock().expect("an unheld lock refused its first taker"); let holder = thread::spawn(move || drop(burst)); - assert!( - matches!(LOCK.seize(u64::from(preemptions) + 2), Seized::Taken(_)), - "a fatal path gave up on a burst that let go inside the bound" - ); + 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"); }); } } From 1f88a38299c322aac6221f343d18c982dec45c67 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 1 Oct 2026 18:56:51 +0200 Subject: [PATCH 06/10] Review round 4: each CPU's id read is held against its roster id at bring-up, and every UART touch takes the registers - kernel-loom compiles `serial_lock` and its `arch::cpu` shim under `loom` alone. Neither not-loom test (`log_zeroed_init`, `log_body_words`) names them, so the shim's not-loom arm, which no clippy shape built, is deleted rather than given a shape. - `within`'s count has a model: an attempt that never answers is asked exactly `tries` times, at 0, 1 and 100. With `0..tries.min(4)` every model passed before this one. - `arch::console_uart`'s `init`, `rx_ready` and `tx_ready` take `&mut serial::Registers` on both architectures, as `read_byte` and `write_byte` do. `serial::init` takes the registers' lock for the call. What the architecture logs under it is not drained there: `drain_inline` returns while `has_console` is false, which it is until `UART_PRESENT` is stored from the call's answer. AArch64's `init` takes them for the call it shares with the 16550's and touches no register. - `smp::echo` and `smp::commit`: a started AP says the hardware id it reads as its own with its echo, and the boot CPU refuses to commit it under any other id. x86-64's `boot_aps` holds the boot CPU's own read against its LAPIC id before the roster takes it. On AArch64 the boot CPU's roster slot is its read. Each refusal is the boot CPU's panic, before `smp::set_ready`, so a wrong read stops the boot. - Filed issues/kernel/a-safe-port-read-reaches-any-io-port.md: `inb(0x3f8)` takes a received console byte with no `Registers` and no `unsafe`. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- .../a-safe-port-read-reaches-any-io-port.md | 22 +++++++++++++++ kernel-loom/src/lib.rs | 10 +++---- kernel-loom/tests/serial_lock.rs | 26 ++++++++++++++--- kernel/src/arch/aarch64/console_uart.rs | 8 ++++-- kernel/src/arch/aarch64/smp.rs | 4 +-- kernel/src/arch/x86_64/console_uart.rs | 16 ++++++----- kernel/src/arch/x86_64/smp.rs | 13 +++++++-- kernel/src/drivers/serial.rs | 23 ++++++++------- kernel/src/smp.rs | 28 ++++++++++++++++++- 9 files changed, 114 insertions(+), 36 deletions(-) create mode 100644 issues/kernel/a-safe-port-read-reaches-any-io-port.md 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/kernel-loom/src/lib.rs b/kernel-loom/src/lib.rs index d20776a4419..a9d2740304a 100644 --- a/kernel-loom/src/lib.rs +++ b/kernel-loom/src/lib.rs @@ -104,15 +104,11 @@ pub mod arch { /// Which CPU a model thread is, which the console lock's fatal word names. /// 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 { - #[cfg(feature = "loom")] loom::thread_local! { static CPU: core::cell::Cell> = core::cell::Cell::new(None); } - #[cfg(not(feature = "loom"))] - std::thread_local! { - static CPU: core::cell::Cell> = const { core::cell::Cell::new(None) }; - } /// Say which CPU the current model thread is. No kernel counterpart. pub fn become_cpu(id: u32) { @@ -375,6 +371,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 26b0732e3fe..84b56d62ae6 100644 --- a/kernel-loom/tests/serial_lock.rs +++ b/kernel-loom/tests/serial_lock.rs @@ -1,7 +1,8 @@ //! 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. +//! `within` asks an attempt exactly the tries it was given; 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. //! //! 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 @@ -9,7 +10,7 @@ #![cfg(feature = "loom")] use kernel_loom::arch::cpu::become_cpu; -use kernel_loom::serial_lock::{BackendLock, Seized}; +use kernel_loom::serial_lock::{within, BackendLock, Seized}; use loom::cell::UnsafeCell; use loom::sync::Arc; use loom::thread; @@ -52,6 +53,23 @@ fn two_writers_never_overlap() { }); } +/// 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(); diff --git a/kernel/src/arch/aarch64/console_uart.rs b/kernel/src/arch/aarch64/console_uart.rs index a3578da0277..91bf79de17b 100644 --- a/kernel/src/arch/aarch64/console_uart.rs +++ b/kernel/src/arch/aarch64/console_uart.rs @@ -42,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) => { @@ -68,7 +70,7 @@ 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 } @@ -78,7 +80,7 @@ pub fn read_byte(_: &mut Registers) -> 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 } diff --git a/kernel/src/arch/aarch64/smp.rs b/kernel/src/arch/aarch64/smp.rs index 0f294097f43..0461b8b4129 100644 --- a/kernel/src/arch/aarch64/smp.rs +++ b/kernel/src/arch/aarch64/smp.rs @@ -67,7 +67,7 @@ pub fn start(gic: &irqchip::Gic, psci: Option) { log!("SMP: cpu{} mpidr={:#x} did not echo within {AP_START}; the rest stay off", attempt.id(), gicc.mpidr); break; } - ROSTER.commit(attempt, packed_affinity(gicc.mpidr)); + smp::commit(attempt, packed_affinity(gicc.mpidr)); log!("SMP: cpu{} mpidr={:#x} online", attempt.id(), gicc.mpidr); } log!("SMP: {} of {} MADT CPUs online", smp::cpu_count(), gic.cpus.len()); @@ -82,6 +82,6 @@ pub(super) extern "C" fn ap_entry(start: &'static ApStart, el: u64) -> ! { paging::check_joined(); control_regs::check(el); irqchip::init_cpu(start.redistributor); - ROSTER.echo(start.token); + smp::echo(start.token); crate::process::ap_idle(); } diff --git a/kernel/src/arch/x86_64/console_uart.rs b/kernel/src/arch/x86_64/console_uart.rs index 7b1269dfb88..4a25cdb308f 100644 --- a/kernel/src/arch/x86_64/console_uart.rs +++ b/kernel/src/arch/x86_64/console_uart.rs @@ -24,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 { @@ -55,7 +56,7 @@ 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 } @@ -65,13 +66,14 @@ pub fn read_byte(_: &mut Registers) -> u8 { } /// 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(_: &mut Registers, 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. + // 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/smp.rs b/kernel/src/arch/x86_64/smp.rs index e16c55118e5..2169e829e97 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(); @@ -220,7 +226,7 @@ pub fn boot_aps(madt: &MadtInfo, boot_cr3: u64) { if tsc_inside(attempt.id(), bracket_lo, bracket_hi) { bracketed += 1; } - ROSTER.commit(attempt, ap_id); + smp::commit(attempt, ap_id); log!("SMP: AP cpu{} lapic={} online", attempt.id(), ap_id); } else { // Neither the id nor the trampoline is reused after a failure: stop here. @@ -287,8 +293,9 @@ extern "C" fn ap_entry() -> ! { // Calibration is a one-time BSP measurement; nothing left for an AP to do here. apic::init_ap(); - // Echo this attempt's token, so the BSP counts this AP for its own attempt. - ROSTER.echo(percpu::ap_token()); + // Echo this attempt's token, so the BSP counts this AP for its own attempt, + // with the hardware id this AP reads as its own. + smp::echo(percpu::ap_token()); process::ap_idle(); } diff --git a/kernel/src/drivers/serial.rs b/kernel/src/drivers/serial.rs index e8e107c6bd5..c1f9c14c6d9 100644 --- a/kernel/src/drivers/serial.rs +++ b/kernel/src/drivers/serial.rs @@ -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,9 +69,9 @@ pub fn backend() -> Backend { static BACKEND: BackendLock = BackendLock::new(); -/// The console UART's registers, asked for: what `arch::console_uart` moves a -/// byte with. Only this file builds one, for a [`BackendGuard`] and a -/// [`PanicUart`]. +/// 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. @@ -95,18 +98,18 @@ impl BackendGuard { } } - 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(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(self) - } else if uart_present() && uart::rx_ready() { + } else if uart_present() && uart::rx_ready(&mut self.registers) { Some(uart::read_byte(&mut self.registers)) } else { None @@ -115,7 +118,7 @@ impl BackendGuard { } pub fn has_data() -> bool { - let g = BackendGuard::lock(); + let mut g = BackendGuard::lock(); g.has_data() } @@ -318,7 +321,7 @@ fn uart_write_fifo(bytes: &[u8]) { let mut asked = 0; loop { let mut burst = BackendGuard::lock(); - if uart::tx_ready() { + 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(&mut burst.registers, b); @@ -457,7 +460,7 @@ fn uart_write_bytes(registers: &mut Registers, bytes: &[u8]) { } for &b in bytes { for _ in 0..THRE_SPIN_LIMIT { - if uart::tx_ready() { + if uart::tx_ready(registers) { break; } core::hint::spin_loop(); diff --git a/kernel/src/smp.rs b/kernel/src/smp.rs index 6c9c667a625..131c2d285e0 100644 --- a/kernel/src/smp.rs +++ b/kernel/src/smp.rs @@ -3,10 +3,11 @@ //! and what starting an AP takes on either architecture. use core::mem::size_of; +use core::sync::atomic::{AtomicU32, Ordering}; use alloc::boxed::Box; -use crate::smp_roster::{Roster, MAX_CPUS}; +use crate::smp_roster::{Attempt, Roster, MAX_CPUS}; pub static ROSTER: Roster = Roster::new(); @@ -23,6 +24,31 @@ pub fn hardware_id(cpu: u32) -> u32 { ROSTER.hardware_id(cpu) } +/// The hardware id the AP being started reads as its own, said with its echo. +static ECHOED_ID: AtomicU32 = AtomicU32::new(0); + +/// The started AP's half of the handshake: the hardware id it reads as its +/// own, then [`Roster::echo`]. +pub fn echo(token: u32) { + ECHOED_ID.store(crate::arch::cpu::hardware_id(), Ordering::Relaxed); + ROSTER.echo(token); +} + +/// Commit the AP that echoed `at` under `hardware_id`, the id its roster slot +/// and every IPI name it by, which must be the one it reads as its own: its +/// fatal paths and the console lock name it by that read. +pub fn commit(at: Attempt, hardware_id: u32) { + // Ordered by the echo, whose acquire `Roster::await_echo` took. + let read = ECHOED_ID.load(Ordering::Relaxed); + 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() + ); + ROSTER.commit(at, hardware_id); +} + /// True once a shootdown must wait for siblings; the word the APs are released by. pub fn answering() -> bool { ROSTER.answering() From 53dc49db5519b99f9493cdcc8c59a936fa7e7bcd Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 1 Oct 2026 19:06:22 +0200 Subject: [PATCH 07/10] File that the AArch64 boot CPU's id read is held against nothing `smp::commit` holds every AP's read against the MPIDR it was started by, and x86-64 holds its boot CPU's against the LAPIC id. On AArch64 the boot CPU's read is its roster slot and picks its redistributor, so no second id holds it: a read naming another CPU brings that CPU's redistributor up as the boot CPU's, and `CPU_ON` to the boot CPU is refused, which stops every CPU after it in the MADT. Recorded rather than fixed: the ruling for this round was to show what the read feeds there. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- ...ot-cpus-id-read-is-held-against-nothing.md | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 issues/kernel/the-aarch64-boot-cpus-id-read-is-held-against-nothing.md 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..52f647dc9e3 --- /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 + +`smp::commit` (`kernel/src/smp.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. From 1a9380fedbee8a311d83f6a85c0b85486d502bd1 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 1 Oct 2026 19:10:56 +0200 Subject: [PATCH 08/10] smp::commit says why the boot CPU refuses an AP's id read, not the AP The review's cheapest red asserted on each CPU itself. An AP's own panic before `smp::set_ready` stops no other CPU: `stop_other_cpus` sends nothing before the release, on both architectures, and the boot CPU's `await_echo` runs out and boots on with the CPUs that came up. So the refusal is the boot CPU's, at the commit, where a wrong read stops the boot. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- kernel/src/smp.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/kernel/src/smp.rs b/kernel/src/smp.rs index 131c2d285e0..6cef7f5167b 100644 --- a/kernel/src/smp.rs +++ b/kernel/src/smp.rs @@ -36,7 +36,8 @@ pub fn echo(token: u32) { /// Commit the AP that echoed `at` under `hardware_id`, the id its roster slot /// and every IPI name it by, which must be the one it reads as its own: its -/// fatal paths and the console lock name it by that read. +/// fatal paths and the console lock name it by that read. Refused here, on the +/// boot CPU, because before the release an AP's own panic stops no other CPU. pub fn commit(at: Attempt, hardware_id: u32) { // Ordered by the echo, whose acquire `Roster::await_echo` took. let read = ECHOED_ID.load(Ordering::Relaxed); From d31faab90c593bdb43e2c47700de4e541495e14f Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 1 Oct 2026 19:49:00 +0200 Subject: [PATCH 09/10] Review round 5: an AP's id read rides in the roster's own echo, and Roster::commit refuses a mismatch Round 4 kept the AP's read in a second word, `smp::ECHOED_ID`, stored before the echo and loaded after `await_echo`. Its order rested on a comment that no model drove, and `ROSTER.commit` stayed a public way around the check. - `Roster::echo(token, read)` stores the token and the read as one `AtomicU64`, token in the high half, so the read cannot land after the token. `Roster::echoed` compares the token half. - `Roster::commit` loads that word and refuses a read other than the id it commits the AP under, with round 4's message. Only the boot CPU commits, before the release, so the boot CPU is the one that refuses. - `ECHOED_ID`, `smp::echo` and `smp::commit` go. Both architectures echo `cpu::hardware_id()` with the token and commit through `ROSTER.commit`, so `kernel/src/smp.rs` is main's again. - `kernel-loom/tests/smp_bringup.rs`, `an_ap_that_reads_another_id_is_refused`: an AP echoes 5, the boot CPU awaits that echo and commits the AP as 7, and the model must panic with the refusal. The count model echoes before it commits, since `commit` now reads the echo. - The review's two REMOVEs: the `within` clause in `serial_lock.rs`'s header, and the clause on the read in x86-64 `ap_entry`'s comment. - The AArch64 boot-CPU issue names `Roster::commit`. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- ...ot-cpus-id-read-is-held-against-nothing.md | 2 +- kernel-loom/tests/serial_lock.rs | 7 ++-- kernel-loom/tests/smp_bringup.rs | 37 +++++++++++++++++- kernel/src/arch/aarch64/smp.rs | 4 +- kernel/src/arch/x86_64/smp.rs | 7 ++-- kernel/src/smp.rs | 29 +------------- kernel/src/smp_roster.rs | 38 +++++++++++++------ 7 files changed, 73 insertions(+), 51 deletions(-) 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 index 52f647dc9e3..9ea36cb3ad0 100644 --- 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 @@ -6,7 +6,7 @@ opened: 2026-10-01 # The AArch64 boot CPU's id read is held against nothing -`smp::commit` (`kernel/src/smp.rs`) holds every AP's `arch::cpu::hardware_id` +`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 diff --git a/kernel-loom/tests/serial_lock.rs b/kernel-loom/tests/serial_lock.rs index 84b56d62ae6..eb66a2b4c7d 100644 --- a/kernel-loom/tests/serial_lock.rs +++ b/kernel-loom/tests/serial_lock.rs @@ -1,8 +1,7 @@ //! Loom: a lost `try_lock` on the console backend leaves its holder holding; -//! `within` asks an attempt exactly the tries it was given; 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. +//! 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. //! //! 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 diff --git a/kernel-loom/tests/smp_bringup.rs b/kernel-loom/tests/smp_bringup.rs index d89aa87a63f..17d4d10ac1f 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,6 +8,7 @@ #![cfg(feature = "loom")] +use std::cell::Cell; use std::sync::atomic::{AtomicBool, Ordering::SeqCst}; use kernel_loom::smp_roster::Roster; @@ -15,6 +17,9 @@ 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 +34,7 @@ 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"); + r.echo(attempt.token(), HARDWARE_ID); let committer = { let r = r.clone(); @@ -91,3 +97,32 @@ 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 || r.echo(attempt.token(), MISREAD)) + }; + + 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/smp.rs b/kernel/src/arch/aarch64/smp.rs index 0461b8b4129..81cec294fc5 100644 --- a/kernel/src/arch/aarch64/smp.rs +++ b/kernel/src/arch/aarch64/smp.rs @@ -67,7 +67,7 @@ pub fn start(gic: &irqchip::Gic, psci: Option) { log!("SMP: cpu{} mpidr={:#x} did not echo within {AP_START}; the rest stay off", attempt.id(), gicc.mpidr); break; } - smp::commit(attempt, packed_affinity(gicc.mpidr)); + ROSTER.commit(attempt, packed_affinity(gicc.mpidr)); log!("SMP: cpu{} mpidr={:#x} online", attempt.id(), gicc.mpidr); } log!("SMP: {} of {} MADT CPUs online", smp::cpu_count(), gic.cpus.len()); @@ -82,6 +82,6 @@ pub(super) extern "C" fn ap_entry(start: &'static ApStart, el: u64) -> ! { paging::check_joined(); control_regs::check(el); irqchip::init_cpu(start.redistributor); - smp::echo(start.token); + ROSTER.echo(start.token, cpu::hardware_id()); crate::process::ap_idle(); } diff --git a/kernel/src/arch/x86_64/smp.rs b/kernel/src/arch/x86_64/smp.rs index 2169e829e97..4f310e51f8e 100644 --- a/kernel/src/arch/x86_64/smp.rs +++ b/kernel/src/arch/x86_64/smp.rs @@ -226,7 +226,7 @@ pub fn boot_aps(madt: &MadtInfo, boot_cr3: u64) { if tsc_inside(attempt.id(), bracket_lo, bracket_hi) { bracketed += 1; } - smp::commit(attempt, ap_id); + ROSTER.commit(attempt, ap_id); log!("SMP: AP cpu{} lapic={} online", attempt.id(), ap_id); } else { // Neither the id nor the trampoline is reused after a failure: stop here. @@ -293,9 +293,8 @@ extern "C" fn ap_entry() -> ! { // Calibration is a one-time BSP measurement; nothing left for an AP to do here. apic::init_ap(); - // Echo this attempt's token, so the BSP counts this AP for its own attempt, - // with the hardware id this AP reads as its own. - smp::echo(percpu::ap_token()); + // Echo this attempt's token, so the BSP counts this AP for its own attempt. + ROSTER.echo(percpu::ap_token(), cpu::hardware_id()); process::ap_idle(); } diff --git a/kernel/src/smp.rs b/kernel/src/smp.rs index 6cef7f5167b..6c9c667a625 100644 --- a/kernel/src/smp.rs +++ b/kernel/src/smp.rs @@ -3,11 +3,10 @@ //! and what starting an AP takes on either architecture. use core::mem::size_of; -use core::sync::atomic::{AtomicU32, Ordering}; use alloc::boxed::Box; -use crate::smp_roster::{Attempt, Roster, MAX_CPUS}; +use crate::smp_roster::{Roster, MAX_CPUS}; pub static ROSTER: Roster = Roster::new(); @@ -24,32 +23,6 @@ pub fn hardware_id(cpu: u32) -> u32 { ROSTER.hardware_id(cpu) } -/// The hardware id the AP being started reads as its own, said with its echo. -static ECHOED_ID: AtomicU32 = AtomicU32::new(0); - -/// The started AP's half of the handshake: the hardware id it reads as its -/// own, then [`Roster::echo`]. -pub fn echo(token: u32) { - ECHOED_ID.store(crate::arch::cpu::hardware_id(), Ordering::Relaxed); - ROSTER.echo(token); -} - -/// Commit the AP that echoed `at` under `hardware_id`, the id its roster slot -/// and every IPI name it by, which must be the one it reads as its own: its -/// fatal paths and the console lock name it by that read. Refused here, on the -/// boot CPU, because before the release an AP's own panic stops no other CPU. -pub fn commit(at: Attempt, hardware_id: u32) { - // Ordered by the echo, whose acquire `Roster::await_echo` took. - let read = ECHOED_ID.load(Ordering::Relaxed); - 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() - ); - ROSTER.commit(at, hardware_id); -} - /// True once a shootdown must wait for siblings; the word the APs are released by. pub fn answering() -> bool { ROSTER.answering() diff --git a/kernel/src/smp_roster.rs b/kernel/src/smp_roster.rs index c881952f1d6..dfd5890d31f 100644 --- a/kernel/src/smp_roster.rs +++ b/kernel/src/smp_roster.rs @@ -4,10 +4,10 @@ //! is the word `answering` reads. Compiled into `kernel-loom/`, so no `crate::`. #[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 +43,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 +59,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 +73,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), } } @@ -102,15 +103,16 @@ impl Roster { } /// The AP's half of the handshake, once it can take its first interrupt: - /// the token of the attempt that started it. - pub fn echo(&self, token: u32) { - self.echoed.store(token, Ordering::Release); + /// the token of the attempt that started it, and its `read` of its own + /// hardware id. + pub fn echo(&self, token: u32, read: u32) { + 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 +129,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. From 3b961d5bfc369c5e63d90e653e3081a33865e0fc Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 1 Oct 2026 20:31:28 +0200 Subject: [PATCH 10/10] Review round 6: Roster::echo reads its own CPU's hardware id `Roster::echo(token, read)` took the AP's id read from its caller, so two mistakes at either architecture's call site passed every QEMU boot, where cpu N's token and hardware id are both N: the two arguments swapped, and x86-64 echoing `apic::id()` instead of `cpu::hardware_id()`, which would leave `Roster::commit`'s refusal holding nothing. `echo(token)` now reads `crate::arch::cpu::hardware_id()` itself, as the console lock's `fatal_here` does, so neither can be written: there is one argument, and the read sits in code both architectures share. kernel-loom supplies that function as its per-model-thread `arch::cpu` shim, which exists under `loom` alone, so `smp_roster` is now compiled there under `loom` alone, as `serial_lock` is; neither not-loom test names it. The two `smp_bringup` models that echo say which CPU they are with `become_cpu` first: the AP that misreads is CPU 5, and the slot model's echo is CPU 7. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016t9wjdQkB8SH7bmfUoiy6L --- kernel-loom/src/lib.rs | 7 +++++-- kernel-loom/tests/smp_bringup.rs | 9 +++++++-- kernel/src/arch/aarch64/smp.rs | 2 +- kernel/src/arch/x86_64/smp.rs | 2 +- kernel/src/smp_roster.rs | 12 +++++++----- 5 files changed, 21 insertions(+), 11 deletions(-) diff --git a/kernel-loom/src/lib.rs b/kernel-loom/src/lib.rs index a9d2740304a..afcade99636 100644 --- a/kernel-loom/src/lib.rs +++ b/kernel-loom/src/lib.rs @@ -101,7 +101,8 @@ pub mod arch { pub fn poll() {} } - /// Which CPU a model thread is, which the console lock's fatal word names. + /// 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")] @@ -167,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; diff --git a/kernel-loom/tests/smp_bringup.rs b/kernel-loom/tests/smp_bringup.rs index 17d4d10ac1f..50f5b313825 100644 --- a/kernel-loom/tests/smp_bringup.rs +++ b/kernel-loom/tests/smp_bringup.rs @@ -11,6 +11,7 @@ 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; @@ -34,7 +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"); - r.echo(attempt.token(), HARDWARE_ID); + become_cpu(HARDWARE_ID); + r.echo(attempt.token()); let committer = { let r = r.clone(); @@ -111,7 +113,10 @@ fn an_ap_that_reads_another_id_is_refused() { let ap = { let r = r.clone(); - loom::thread::spawn(move || r.echo(attempt.token(), MISREAD)) + loom::thread::spawn(move || { + become_cpu(MISREAD); + r.echo(attempt.token()); + }) }; let polls = Cell::new(0); diff --git a/kernel/src/arch/aarch64/smp.rs b/kernel/src/arch/aarch64/smp.rs index 81cec294fc5..0f294097f43 100644 --- a/kernel/src/arch/aarch64/smp.rs +++ b/kernel/src/arch/aarch64/smp.rs @@ -82,6 +82,6 @@ pub(super) extern "C" fn ap_entry(start: &'static ApStart, el: u64) -> ! { paging::check_joined(); control_regs::check(el); irqchip::init_cpu(start.redistributor); - ROSTER.echo(start.token, cpu::hardware_id()); + ROSTER.echo(start.token); crate::process::ap_idle(); } diff --git a/kernel/src/arch/x86_64/smp.rs b/kernel/src/arch/x86_64/smp.rs index 4f310e51f8e..721f93de39d 100644 --- a/kernel/src/arch/x86_64/smp.rs +++ b/kernel/src/arch/x86_64/smp.rs @@ -294,7 +294,7 @@ extern "C" fn ap_entry() -> ! { apic::init_ap(); // Echo this attempt's token, so the BSP counts this AP for its own attempt. - ROSTER.echo(percpu::ap_token(), cpu::hardware_id()); + ROSTER.echo(percpu::ap_token()); process::ap_idle(); } diff --git a/kernel/src/smp_roster.rs b/kernel/src/smp_roster.rs index dfd5890d31f..7436176bcbf 100644 --- a/kernel/src/smp_roster.rs +++ b/kernel/src/smp_roster.rs @@ -1,7 +1,8 @@ //! 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, AtomicU64, Ordering}; @@ -102,10 +103,11 @@ 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, and its `read` of its own - /// hardware id. - pub fn echo(&self, token: u32, read: u32) { + /// 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) { + let read = crate::arch::cpu::hardware_id(); self.echoed.store((u64::from(token) << 32) | u64::from(read), Ordering::Release); }