From bee84bc9b5792a60f09e29164c18dd332c416560 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 27 Aug 2026 10:41:29 +0200 Subject: [PATCH 01/14] Every gs: displacement is derived, and the 20 asserts go with the last literal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The five naked entry stubs were the last GS accesses in the kernel spelling a displacement as a number: eight in `arch::syscall`'s entry (the stack switch, its three diagnostic stores and the preempt bracket), four in the Ring 0 timer's re-arm and its need_resched/fire-count stores, and two each in `device_irq`, `common_entry` and the shootdown entry. Each now takes the matching `arch::percpu::OFF_*` as a `const` operand, which a naked stub accepts exactly as an ordinary `asm!` does — `arch::idt::nmi` had been doing it since it was written. `OFF_KERNEL_RSP` is new, for the one field no Rust access named. With no literal left, the 20 `const _: () = assert!(OFF_x == N)` protect nothing and are deleted with the 27 `// offset N` field comments — a third copy of the same numbers. The hazard those existed for was never the reorder they catch: it was a field edit made *with* the asserts updated, which left the 18 asm sites reading the wrong bytes with no diagnostic at all. `_pad200` goes too. It was reserved because "dropping these 8 bytes shifts all of them" — every field below it was reached by a literal — and now nothing is. Its removal is what proves the unification rather than asserting it. Negative control, measured: this whole change reverted onto its base, with `_pad200` deleted and the 20 asserts updated to the shifted offsets — the careful edit the old shape invites — reds. The syscall entry's `lock add dword ptr gs:[240]` then lands on `last_seen_ring0_fires`, and init dies in boot with `scheduler entered while a lock is held: preempt depth 4294966880, baseline 0` at `syscall_entry+0x88`. `ALONE syscall_cost: red again, the same failure both times`. Independent oracle, the emitted machine code rather than the source: the 33 `gs:` instructions across `syscall_entry`, `common_entry`, `timer_entry`, `tlb_flush_entry` and the six `device_irq` stubs, disassembled before and after with llvm-objdump, are the same instructions in the same order with every displacement moved by exactly the 8 bytes `_pad200` occupied — 0xf0 to 0xe8 for the preempt count, 0xd8 to 0xd0 for `syscall_rip`, 0x10 and 0x18 unmoved because they sit above the padding. No instruction changed shape, so the `const` operand really is the immediate-displacement form the literal was. Green: syscall_cost, irq_census_conservation, sched_stress, futex_wake_counts, cargo test --lib. Closes issues/design-debt/percpu-asm-contract-is-unbound.md's first half; the two PerCpu riders that entry says must not precede this follow. --- kernel/src/arch/idt/device_irq.rs | 5 +- kernel/src/arch/idt/mod.rs | 5 +- kernel/src/arch/idt/timer.rs | 13 ++-- kernel/src/arch/idt/tlb.rs | 5 +- kernel/src/arch/percpu.rs | 118 +++++++++++++----------------- kernel/src/arch/syscall/gate.rs | 29 +++++--- kernel/src/preempt.rs | 22 +++--- 7 files changed, 93 insertions(+), 104 deletions(-) diff --git a/kernel/src/arch/idt/device_irq.rs b/kernel/src/arch/idt/device_irq.rs index b302d4ce43e..1f24b1b833e 100644 --- a/kernel/src/arch/idt/device_irq.rs +++ b/kernel/src/arch/idt/device_irq.rs @@ -36,13 +36,13 @@ macro_rules! device_irq_entry { "push r10", "push r11", "push rbp", - "lock add dword ptr gs:[240], 1", + "lock add dword ptr gs:[{preempt_count}], 1", // Ring 0 entry has unknown rsp alignment; align via the rbp save. "mov rbp, rsp", "and rsp, -16", "call {handler}", "mov rsp, rbp", - "lock sub dword ptr gs:[240], 1", + "lock sub dword ptr gs:[{preempt_count}], 1", "test dword ptr [rsp + 88], 3", // CS = 10 GPRs + RIP above "jz 1f", // Ring 3: run the deferred-preempt epilogue with the user @@ -65,6 +65,7 @@ macro_rules! device_irq_entry { "iretq", handler = sym $handler, exit_to_user = sym crate::arch::idt::kernel_exit_to_user_check, + preempt_count = const $crate::arch::percpu::OFF_PREEMPT_COUNT, ); } }; diff --git a/kernel/src/arch/idt/mod.rs b/kernel/src/arch/idt/mod.rs index 60b5da21112..1cd7511c948 100644 --- a/kernel/src/arch/idt/mod.rs +++ b/kernel/src/arch/idt/mod.rs @@ -422,11 +422,11 @@ extern "sysv64" fn common_entry() { "push r11", "push r10", "push r9", "push r8", "push rbp", "push rdi", "push rsi", "push rdx", "push rcx", "push rbx", "push rax", - "lock add dword ptr gs:[240], 1", + "lock add dword ptr gs:[{preempt_count}], 1", "mov rdi, rsp", save_user_state!(), "call {dispatch}", - "lock sub dword ptr gs:[240], 1", + "lock sub dword ptr gs:[{preempt_count}], 1", // Run exit-to-user epilogue before restoring GPRs — the call clobbers // scratch regs, which would otherwise leak kernel state into user. "mov r11, [rsp + {fp_bytes}]", @@ -444,6 +444,7 @@ extern "sysv64" fn common_entry() { "iretq", dispatch = sym trap_dispatch, exit_to_user = sym kernel_exit_to_user_check, + preempt_count = const percpu::OFF_PREEMPT_COUNT, ); } diff --git a/kernel/src/arch/idt/timer.rs b/kernel/src/arch/idt/timer.rs index 334d4a6e772..73243b45daf 100644 --- a/kernel/src/arch/idt/timer.rs +++ b/kernel/src/arch/idt/timer.rs @@ -35,9 +35,8 @@ pub(super) extern "sysv64" fn timer_entry() { // Re-arm before Rust runs so the timer survives even if the handler // path panics before scheduler::do_preempt → arm_one_shot. - // gs:[260] = PerCpu.last_armed_ticks (per-CPU one-shot re-arm value). "mov ecx, 0x838", - "mov eax, dword ptr gs:[260]", + "mov eax, dword ptr gs:[{armed_ticks}]", "xor edx, edx", "wrmsr", @@ -73,17 +72,21 @@ pub(super) extern "sysv64" fn timer_entry() { "xor edx, edx", "wrmsr", "mov ecx, 0x838", // X2APIC_TIMER_INIT — re-arm with last value; - "mov eax, dword ptr gs:[260]", // PerCpu.last_armed_ticks; 0 = disabled. + "mov eax, dword ptr gs:[{armed_ticks}]", // 0 = disabled. "xor edx, edx", "wrmsr", - "mov byte ptr gs:[244], 1", // need_resched - "inc dword ptr gs:[248]", // ring0_timer_fires (no lock: single writer, IF=0) + "mov byte ptr gs:[{need_resched}], 1", + // No lock on the fire count: single writer, IF=0. + "inc dword ptr gs:[{ring0_fires}]", "pop rdx", "pop rcx", "pop rax", "iretq", handler = sym timer_handler, exit_to_user = sym crate::arch::idt::kernel_exit_to_user_check, + armed_ticks = const crate::arch::percpu::OFF_LAST_ARMED_TICKS, + need_resched = const crate::arch::percpu::OFF_NEED_RESCHED, + ring0_fires = const crate::arch::percpu::OFF_RING0_TIMER_FIRES, irq_total = const crate::irq_census::slot_offset(crate::irq_census::TOTAL), irq_timer = const crate::irq_census::slot_offset( 1 + crate::irq_census::Source::Timer as usize diff --git a/kernel/src/arch/idt/tlb.rs b/kernel/src/arch/idt/tlb.rs index a2415122864..5fb876f81b0 100644 --- a/kernel/src/arch/idt/tlb.rs +++ b/kernel/src/arch/idt/tlb.rs @@ -22,7 +22,7 @@ pub(super) extern "sysv64" fn tlb_flush_entry() { "push r10", "push r11", "push rbp", - "lock add dword ptr gs:[240], 1", + "lock add dword ptr gs:[{preempt_count}], 1", "mov rbp, rsp", "and rsp, -16", "call {flush}", @@ -31,7 +31,7 @@ pub(super) extern "sysv64" fn tlb_flush_entry() { "xor eax, eax", "xor edx, edx", "wrmsr", - "lock sub dword ptr gs:[240], 1", + "lock sub dword ptr gs:[{preempt_count}], 1", "test dword ptr [rsp + 88], 3", "jz 1f", "cli", @@ -52,6 +52,7 @@ pub(super) extern "sysv64" fn tlb_flush_entry() { "iretq", flush = sym flush, exit_to_user = sym crate::arch::idt::kernel_exit_to_user_check, + preempt_count = const crate::arch::percpu::OFF_PREEMPT_COUNT, ); } diff --git a/kernel/src/arch/percpu.rs b/kernel/src/arch/percpu.rs index 79123093d05..2fa6671d13d 100644 --- a/kernel/src/arch/percpu.rs +++ b/kernel/src/arch/percpu.rs @@ -72,54 +72,52 @@ pub enum CpuFaultState { Panic = 3, // panic handler running } -/// Per-CPU data. Accessed via GS segment in kernel mode. -/// Field offsets are hardcoded in assembly — do not reorder. +/// Per-CPU data, reached through the GS segment. Every access — Rust and +/// assembly alike — names an `OFF_*` constant below, so the layout is this +/// type's alone and a field that moves moves its accessors with it. #[repr(C)] pub struct PerCpu { - self_ptr: u64, // offset 0: points to self (for gs:0 self-reference) - cpu_id: u32, // offset 8 - lapic_id: u32, // offset 12 - pub kernel_rsp: u64, // offset 16: syscall entry loads this as kernel stack - pub user_rsp: u64, // offset 24: syscall entry saves user RSP here - pub tss: Tss, // offset 32 (104 bytes) - current_tid: u32, // offset 136: TID of thread running on this CPU (u32::MAX = idle) - current_pid: u32, // offset 140: PID of process running on this CPU (u32::MAX = idle) - gdt: [u64; 7], // offset 144 (56 bytes) - // offset 200: reserved, and reclaiming it is not a local change. Every field - // below is reached by a `gs:[NNN]` *literal* in a naked stub — - // `syscall_rip`/`syscall_num`/`syscall_rbp` at 216/224/232 and - // `preempt_count` at 240 (`arch::syscall`), and `need_resched` at 244, - // `ring0_timer_fires` at 248 and `last_armed_ticks` at 260 (`arch::idt`'s - // timer/tlb stubs). Dropping these 8 bytes shifts all of them. - _pad200: [u8; 8], // offset 200 - idle_stack_top: u64, // offset 208: top of per-CPU idle stack + self_ptr: u64, + cpu_id: u32, + lapic_id: u32, + /// The syscall entry loads this as its kernel stack. + pub kernel_rsp: u64, + /// …and parks the user's RSP here across the switch. + pub user_rsp: u64, + pub tss: Tss, + /// TID of the thread running on this CPU; `u32::MAX` when none is. + current_tid: u32, + /// PID of the process running on this CPU; `u32::MAX` when none is. + current_pid: u32, + gdt: [u64; 7], + idle_stack_top: u64, /// Saved user RIP at last syscall entry (for panic diagnostics). - pub syscall_rip: u64, // offset 216 + pub syscall_rip: u64, /// Saved syscall number (for panic diagnostics). - pub syscall_num: u64, // offset 224 + pub syscall_num: u64, /// Saved user RBP at last syscall entry (for panic diagnostics). - pub syscall_rbp: u64, // offset 232 + pub syscall_rbp: u64, /// `lock add/sub` because IRQ entry/exit and Rust kernel code mutate it /// on the same CPU. - pub preempt_count: AtomicU32, // offset 240 - pub need_resched: AtomicU8, // offset 244 - _pad245: [u8; 3], // offset 245..248 + pub preempt_count: AtomicU32, + pub need_resched: AtomicU8, + _pad_after_need_resched: [u8; 3], /// Writes use plain `inc`: only the Ring 0 timer stub writes, with IF=0. - pub ring0_timer_fires: AtomicU32, // offset 248 - pub last_seen_ring0_fires: u32, // offset 252 - fault_state: u8, // offset 256 - _pad257: [u8; 3], // offset 257..260 - /// Ticks the Ring 0 timer asm re-arms with (gs:[260]). Per-CPU: one-shot + pub ring0_timer_fires: AtomicU32, + pub last_seen_ring0_fires: u32, + fault_state: u8, + _pad_after_fault_state: [u8; 3], + /// Ticks the Ring 0 timer asm re-arms with. Per-CPU: one-shot /// timers are armed independently on every CPU; a shared value would let /// any CPU's arm/stop clobber every other CPU's re-arm fallback. - pub last_armed_ticks: AtomicU32, // offset 260 + pub last_armed_ticks: AtomicU32, /// This CPU's [`log::Shard`], reached by [`reserve_log_slot`]. /// /// **Never null on a live CPU**: [`alloc_percpu`] fills it for cpu0 and for /// every AP, and the BSP allocates an AP's whole `PerCpu` before that AP /// executes an instruction. That is why `emit` needs no check — an absent /// shard is not a state this field can be in. - log_shard: u64, // offset 264 + log_shard: u64, /// Non-zero while this CPU is inside its NMI handler, written by /// `arch::idt::nmi`'s entry and by nothing else. /// @@ -129,8 +127,8 @@ pub struct PerCpu { /// handler's `iretq` (SDM Vol. 3A §6.7.1), and the handler cannot fault, so /// no such entry exists — and this word is what turns that argument into an /// observation rather than an assumption. - nmi_active: u32, // offset 272 - _pad276: [u8; 4], // offset 276..280 + nmi_active: u32, + _pad_after_nmi_active: [u8; 4], /// Interrupt deliveries this CPU has taken: the machine's total, then one /// counter per `irq_census::Source`. /// @@ -141,7 +139,7 @@ pub struct PerCpu { /// module header states. Last on purpose: the array's length is /// `irq_census::SLOTS`, so a new `Source` grows it without moving any /// other field. - pub irq_counts: [AtomicU64; crate::irq_census::SLOTS], // offset 280 + pub irq_counts: [AtomicU64; crate::irq_census::SLOTS], } // GDT layout: @@ -221,25 +219,27 @@ impl PerCpu { // Where each field this kernel reaches through `gs:` sits inside `PerCpu`. // -// **Derived from the type and asserted against the number the assembly -// hardcodes.** `arch::syscall`'s entry and `arch::idt`'s stubs — the Ring 0 -// timer's re-arm and the preempt-count opens and closes among them — write the -// displacement as a literal, so the assertion is the whole of what keeps the -// two sides in step — a reordered or resized field would otherwise move only -// the Rust half. Every GS access written in Rust names one of these constants; -// none of them names a number. +// **Derived from the type, and the only spelling any access has.** Every GS +// access — the primitives below, `preempt`, `irq_census`, and the five naked +// entry stubs of `arch::syscall` and `arch::idt` — feeds one of these in as a +// `const` operand, a naked stub exactly as an ordinary `asm!`. So no +// displacement is written down twice and none can be left behind by a field +// edit. const OFF_SELF_PTR: u32 = offset_of!(PerCpu, self_ptr) as u32; const OFF_CPU_ID: u32 = offset_of!(PerCpu, cpu_id) as u32; -const OFF_USER_RSP: u32 = offset_of!(PerCpu, user_rsp) as u32; +/// The kernel stack `arch::syscall`'s entry switches to, and the one field of +/// this type the hardware never reads for itself. +pub(crate) const OFF_KERNEL_RSP: u32 = offset_of!(PerCpu, kernel_rsp) as u32; +pub(crate) const OFF_USER_RSP: u32 = offset_of!(PerCpu, user_rsp) as u32; const OFF_CURRENT_TID: u32 = offset_of!(PerCpu, current_tid) as u32; const OFF_CURRENT_PID: u32 = offset_of!(PerCpu, current_pid) as u32; const OFF_IDLE_STACK_TOP: u32 = offset_of!(PerCpu, idle_stack_top) as u32; -const OFF_SYSCALL_RIP: u32 = offset_of!(PerCpu, syscall_rip) as u32; -const OFF_SYSCALL_NUM: u32 = offset_of!(PerCpu, syscall_num) as u32; -const OFF_SYSCALL_RBP: u32 = offset_of!(PerCpu, syscall_rbp) as u32; -const OFF_RING0_TIMER_FIRES: u32 = offset_of!(PerCpu, ring0_timer_fires) as u32; +pub(crate) const OFF_SYSCALL_RIP: u32 = offset_of!(PerCpu, syscall_rip) as u32; +pub(crate) const OFF_SYSCALL_NUM: u32 = offset_of!(PerCpu, syscall_num) as u32; +pub(crate) const OFF_SYSCALL_RBP: u32 = offset_of!(PerCpu, syscall_rbp) as u32; +pub(crate) const OFF_RING0_TIMER_FIRES: u32 = offset_of!(PerCpu, ring0_timer_fires) as u32; const OFF_LAST_SEEN_RING0_FIRES: u32 = offset_of!(PerCpu, last_seen_ring0_fires) as u32; -const OFF_LAST_ARMED_TICKS: u32 = offset_of!(PerCpu, last_armed_ticks) as u32; +pub(crate) const OFF_LAST_ARMED_TICKS: u32 = offset_of!(PerCpu, last_armed_ticks) as u32; /// `reserve_log_slot`'s naked read of this CPU's [`log::Shard`] pointer names /// this rather than an inline `offset_of!`, so no GS access spells a raw field. const OFF_LOG_SHARD: u32 = offset_of!(PerCpu, log_shard) as u32; @@ -260,26 +260,6 @@ pub(crate) const OFF_NMI_ACTIVE: u32 = offset_of!(PerCpu, nmi_active) as u32; /// instrument names no number of its own. pub const OFF_IRQ_COUNTS: u32 = offset_of!(PerCpu, irq_counts) as u32; -const _: () = assert!(OFF_SELF_PTR == 0); -const _: () = assert!(OFF_CPU_ID == 8); -const _: () = assert!(offset_of!(PerCpu, kernel_rsp) == 16); -const _: () = assert!(OFF_USER_RSP == 24); -const _: () = assert!(offset_of!(PerCpu, tss) == 32); -const _: () = assert!(OFF_CURRENT_TID == 136); -const _: () = assert!(OFF_CURRENT_PID == 140); -const _: () = assert!(OFF_IDLE_STACK_TOP == 208); -const _: () = assert!(OFF_SYSCALL_RIP == 216); -const _: () = assert!(OFF_SYSCALL_NUM == 224); -const _: () = assert!(OFF_SYSCALL_RBP == 232); -const _: () = assert!(OFF_PREEMPT_COUNT == 240); -const _: () = assert!(OFF_NEED_RESCHED == 244); -const _: () = assert!(OFF_RING0_TIMER_FIRES == 248); -const _: () = assert!(OFF_LAST_SEEN_RING0_FIRES == 252); -const _: () = assert!(OFF_FAULT_STATE == 256); -const _: () = assert!(OFF_LAST_ARMED_TICKS == 260); -const _: () = assert!(OFF_LOG_SHARD == 264); -const _: () = assert!(OFF_NMI_ACTIVE == 272); -const _: () = assert!(OFF_IRQ_COUNTS == 280); /// Every GS-relative access this kernel makes, as `const`-generic primitives. /// @@ -374,7 +354,7 @@ pub(crate) mod gs { /// /// Its own primitive rather than [`write_u8`] called with a constant, /// because the instruction differs: this is the one-instruction - /// `mov byte ptr gs:[244], 1`, where the register form would first + /// `mov byte ptr gs:[{off}], 1`, where the register form would first /// materialise the value. #[inline] pub fn write_u8_imm() { diff --git a/kernel/src/arch/syscall/gate.rs b/kernel/src/arch/syscall/gate.rs index 42902d3e41c..b12058ee692 100644 --- a/kernel/src/arch/syscall/gate.rs +++ b/kernel/src/arch/syscall/gate.rs @@ -62,7 +62,7 @@ pub fn init() { // The single-step trap after a `popfq` that set `TF` is deferred by // exactly one instruction, and if that instruction is `syscall` the // `#DB` is taken at `LSTAR` with CPL already 0 and `rsp` still the - // *user* stack, because the entry has not reached `mov rsp, gs:[16]`. + // *user* stack, because the entry has not reached its stack switch. // The `#DB` gate has no IST, so the CPU builds its frame there — a // supervisor write to a user page, which SMAP refuses — and the `#PF` // lands on the same stack and escalates. Measured on this tree before @@ -86,15 +86,14 @@ pub fn init() { } // Syscall entry: GS permanently points to kernel per-CPU data (no swapgs needed). -// PerCpu layout: offset 16 = kernel_rsp, offset 24 = user_rsp. // // The bracket spans the handler *and* the exit-to-user epilogue, because both // can context-switch. The epilogue used to run with the user state already put // back, so a switch there returned to Ring 3 carrying whatever the task that // ran in between had left in the registers. // -// **`SYSCALL` switches no stack, so the instructions before `mov rsp, gs:[16]` -// run at CPL 0 on the user's stack — and that is the whole of the window an +// **`SYSCALL` switches no stack, so the instructions before `mov rsp, +// gs:[{kernel_rsp}]` run at CPL 0 on the user's stack — and that is the whole of the window an // exception may not land in.** It was six instructions and it is three: the // three diagnostic stores below it — `syscall_rip`, `syscall_num`, // `syscall_rbp` — are reads of `rcx`, `rdi` and `rbp`, which the stack switch @@ -109,12 +108,12 @@ pub fn init() { #[unsafe(naked)] extern "sysv64" fn syscall_entry() { ring3_naked_asm!( - "mov gs:[24], rsp", // save user RSP to percpu.user_rsp - "mov rsp, gs:[16]", // load kernel RSP from percpu.kernel_rsp - "mov gs:[216], rcx", // save user RIP to percpu.syscall_rip - "mov gs:[224], rdi", // save syscall number to percpu.syscall_num - "mov gs:[232], rbp", // save user RBP to percpu.syscall_rbp - "push gs:[24]", // user RSP on kernel stack + "mov gs:[{user_rsp}], rsp", + "mov rsp, gs:[{kernel_rsp}]", + "mov gs:[{syscall_rip}], rcx", + "mov gs:[{syscall_num}], rdi", + "mov gs:[{syscall_rbp}], rbp", + "push gs:[{user_rsp}]", // user RSP on kernel stack "push rcx", // return RIP "push r11", // return RFLAGS "push rdi", @@ -126,11 +125,11 @@ extern "sysv64" fn syscall_entry() { save_user_state!(), - "lock add dword ptr gs:[240], 1", // preempt_count++ + "lock add dword ptr gs:[{preempt_count}], 1", "call {handler}", - "lock sub dword ptr gs:[240], 1", // preempt_count-- + "lock sub dword ptr gs:[{preempt_count}], 1", // cli before exit_to_user and pop rsp / sysretq: an interrupt after // pop rsp would land on the user RSP as a kernel stack. Helper // preserves IF=0 across its return. @@ -159,6 +158,12 @@ extern "sysv64" fn syscall_entry() { "sysretq", handler = sym syscall_handler, exit_to_user = sym crate::arch::idt::kernel_exit_to_user_check, + kernel_rsp = const percpu::OFF_KERNEL_RSP, + user_rsp = const percpu::OFF_USER_RSP, + syscall_rip = const percpu::OFF_SYSCALL_RIP, + syscall_num = const percpu::OFF_SYSCALL_NUM, + syscall_rbp = const percpu::OFF_SYSCALL_RBP, + preempt_count = const percpu::OFF_PREEMPT_COUNT, ); } diff --git a/kernel/src/preempt.rs b/kernel/src/preempt.rs index b8e8d77809f..c21c3865914 100644 --- a/kernel/src/preempt.rs +++ b/kernel/src/preempt.rs @@ -1,12 +1,12 @@ //! Linux-style deferred preemption primitives. //! //! Two per-CPU words drive the model (defined in `arch::percpu::PerCpu`): -//! - `preempt_count` @ gs:[240] — incremented by every IRQ entry and by +//! - `preempt_count` — incremented by every IRQ entry and by //! `disable()`. Read-modify-writes are `lock`-prefixed because both kernel //! code and IRQ entries mutate it on the same CPU; `set_count`'s plain //! store needs no prefix (a naturally aligned 32-bit store, and a same-CPU //! IRQ cannot land inside one instruction). -//! - `need_resched` @ gs:[244] — set by the timer ISR (and future wake +//! - `need_resched` — set by the timer ISR (and future wake //! paths), cleared by the deferred-preempt epilogue. Single-byte stores //! are naturally atomic on x86 — no `lock` prefix needed. //! @@ -17,13 +17,13 @@ //! `const`-generic primitives — `read_u32`, `write_u32`, `read_u8`, //! `write_u8_imm`, `lock_inc_u32`, `lock_dec_u32` — rather than a hand-written //! `asm!` string per accessor. The offset is a `const` operand, so each still -//! assembles to the immediate-displacement form (`lock addl $1, %gs:240`) the -//! entry stubs in `arch::syscall` and `arch::idt` open and close the same count -//! with. **They live in `arch::percpu` and not here**: that module declares -//! `PerCpu`, asserts every offset against the number the assembly hardcodes, and -//! reaches the same fields itself — this file had a second copy of both the -//! primitives and the three offsets, and a `gs:` string at the crate root is -//! also x86 in a file that is not `arch/`. +//! assembles to the immediate-displacement form the entry stubs in +//! `arch::syscall` and `arch::idt` open and close the same count with — and +//! those stubs feed it the same constant, so no spelling of this word is a +//! number. **They live in `arch::percpu` and not here**: that module declares +//! `PerCpu` and derives every offset from it — this file had a second copy of +//! both the primitives and the three offsets, and a `gs:` string at the crate +//! root is also x86 in a file that is not `arch/`. //! //! The word is per-CPU but the depth it holds belongs to the running *context*, //! so `Hw::switch` swaps it with the incoming context's saved depth. Without @@ -118,9 +118,7 @@ pub fn enable() { /// Whether this CPU is inside a fault or panic report. /// -/// `gs:[256]` is `PerCpu::fault_state`, non-zero for PageFault/Fatal/Panic and -/// asserted at that offset in `percpu.rs` alongside the other raw offsets this -/// module uses. +/// `PerCpu::fault_state` is non-zero for PageFault/Fatal/Panic. /// /// A CPU inside a report is not reschedulable, so a `fault_state` never /// returned to Normal costs that CPU its preemption for the rest of the boot: From 891c5ac146bf32f129e92f240056179e61260408 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 27 Aug 2026 10:43:34 +0200 Subject: [PATCH 02/14] PerCpu is written whole, and the field with no readers goes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two riders the entry made wait for the unification, now that a field edit moves nothing but the field. `lapic_id` had zero readers: written once and never read, while every other `lapic_id` in the kernel is the *parameter* of `alloc_percpu`, `init_bsp` or `alloc_ap`. The field goes, and with it the parameter of the two functions that carried it only to store it — `init_bsp` keeps its own, which its identity log line prints, and `smp`'s AP loop still names the lapic id it sent the INIT-SIPI-SIPI to. `alloc_percpu` set 8 of the struct's fields and left the other 14 to `alloc_zeroed`. It is one `ptr::write(PerCpu { .. })` now, so a field added tomorrow does not compile until somebody says what it starts as — which is the question `current_tid`/`current_pid` answer with `u32::MAX` and a zeroed allocation would have answered with thread 0 of process 0. `init_tss_descriptor` stays after the write, because a TSS descriptor holds the address the block was allocated at. Green: sched_stress (an SMP boot, so every AP takes this path), irq_census_conservation, syscall_cost, cargo test --lib. Closes issues/design-debt/percpu-asm-contract-is-unbound.md. No citation of the slug or the path exists anywhere in the tree, and `src/redlist.rs` has no row sourced to it. What the entry was for now lives where it is enforced: the `OFF_*` block's own header in `arch/percpu.rs`, which says every access spells a constant and none spells a number. --- .../percpu-asm-contract-is-unbound.md | 69 ------------------ kernel/src/arch/percpu.rs | 72 ++++++++++++++----- kernel/src/arch/smp.rs | 2 +- 3 files changed, 55 insertions(+), 88 deletions(-) delete mode 100644 issues/design-debt/percpu-asm-contract-is-unbound.md diff --git a/issues/design-debt/percpu-asm-contract-is-unbound.md b/issues/design-debt/percpu-asm-contract-is-unbound.md deleted file mode 100644 index c46a895345d..00000000000 --- a/issues/design-debt/percpu-asm-contract-is-unbound.md +++ /dev/null @@ -1,69 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-08-08 ---- - -# 18 `gs:[N]` literals in five entry stubs are still bound to nothing - -The owner's own review note, and the one finding in it that names a hazard -rather than a shape: *"this verifies against constants but doesnt gaurantee -that the constants are the same used in for example preempt.rs."* He is right, -and he is right about less of the tree than he was. - -**The fix shape has landed for everything written in Rust.** -`arch/percpu.rs:234-264` declares 18 `OFF_*` constants, each -`offset_of!(PerCpu, f)`, and every GS access this kernel writes in Rust now -feeds one into the assembly as a `const` operand — the `const`-generic -primitives at `:323-422`, `reserve_log_slot`'s four-word read at `:671-674`, -`preempt`'s six accessors, `irq_census::irq_took!` deriving both of its -displacements from `OFF_IRQ_COUNTS`, and `arch/idt/nmi.rs`'s **naked** entry -reaching `nmi_active` as `gs:[{active}]` with `active = const OFF_NMI_ACTIVE`. -That last one is the proof the rest is reachable: a naked stub takes `const` -operands, and `ring3_naked_asm!` is `($($body:tt)*)` — it passes its body -through and appends two of its own, so a caller may write one at the site with -no macro plumbing at all. - -**What is left is the entry stubs, and they are the paths that punish it.** -Measured 2026-08-24 at `f62a6443`: 54 `gs:[` lines across ten files, of which -19 are `const` operands, 17 are prose, and **18 are hand-written literal -displacements across five files** — - -| file | literals | offsets | -|---|---|---| -| `arch/syscall.rs:268-289` | 8 | 16, 24 (twice), 216, 224, 232, 240 (twice) | -| `arch/idt/timer.rs:40-80` | 4 | 244, 248, 260 (twice) | -| `arch/idt/device_irq.rs:39,45` | 2 | 240 | -| `arch/idt/mod.rs:413,417` | 2 | 240 | -| `arch/idt/tlb.rs:25,34` | 2 | 240 | - -nine distinct offsets in all. These are the `SYSCALL` entry's stack switch and -its three diagnostic stores, the Ring 0 timer's re-arm and `need_resched`, and -every IRQ entry's preempt-count open and close. - -`arch/percpu.rs:266-285` carries **20** `const _: () = assert!(...)` — the count -grew with the constants rather than shrinking. Eighteen assert an `OFF_*` against -a literal; two (`kernel_rsp == 16`, `tss == 32`) assert `offset_of!` directly, -because no constant is declared for a field no Rust GS access names. A third -copy of the same numbers still lives in the field comments (`cpu_id: u32, // -offset 8`): 27 `// offset` lines at `:79-147`. - -So the hazard is unchanged in kind and smaller in extent. Reordering a field -trips the asserts; changing one asserted literal and its field together does -not, and the 18 remaining asm sites then read the wrong bytes with no -diagnostic at all. The fix is the same one, finished: feed `offset_of!` into -those five stubs as `const` operands, and the 20 asserts delete with the last -literal. - -Two smaller PerCpu items ride this and **must not precede it**, because field -surgery before the unification is what the remaining copies punish. Both -re-verified 2026-08-24: - -- `lapic_id` (`percpu.rs:81`) has zero readers. Written at `:585`; every other - `lapic_id` in the kernel is the *parameter* of `alloc_percpu`, `init_bsp` or - `alloc_ap`, or `:947` logging that parameter. Delete the field. -- `alloc_percpu` (`:567`) sets 8 of `PerCpu`'s 22 real fields — 26 declarations, - four of them padding — and relies on `alloc_zeroed` for the other 14. One - total `ptr::write(PerCpu { .. })` says what the struct is. The - `current_tid`/`current_pid` `u32::MAX` sentinel stays — it is an asm wire - format and is already `Option`-decoded at the boundary. diff --git a/kernel/src/arch/percpu.rs b/kernel/src/arch/percpu.rs index 2fa6671d13d..26f829525b4 100644 --- a/kernel/src/arch/percpu.rs +++ b/kernel/src/arch/percpu.rs @@ -79,7 +79,6 @@ pub enum CpuFaultState { pub struct PerCpu { self_ptr: u64, cpu_id: u32, - lapic_id: u32, /// The syscall entry loads this as its kernel stack. pub kernel_rsp: u64, /// …and parks the user's RSP here across the switch. @@ -506,7 +505,14 @@ const STACK_FILL: u8 = 0xA5; const STACK_FILL_WORD: u64 = u64::from_ne_bytes([STACK_FILL; 8]); /// Allocate and initialize PerCpu for a CPU. Returns a raw pointer (lives forever). -fn alloc_percpu(cpu_id: u32, lapic_id: u32) -> *mut PerCpu { +/// +/// **One `write` of the whole struct, so the allocator zeroes nothing this +/// function means.** A field added to [`PerCpu`] has to be given a value here or +/// the initialiser does not compile; the partial form this replaced named 8 of +/// the fields and left the rest to `alloc_zeroed`, where a new field's default +/// is silently whatever zero means for it — and two of these fields have a +/// non-zero idle state. +fn alloc_percpu(cpu_id: u32) -> *mut PerCpu { let layout = Layout::from_size_align(size_of::(), 16).unwrap(); // SAFETY: `size_of::()` is non-zero and 16 is a power of two, which // is the whole of `alloc_zeroed`'s contract. Irreducible because the block @@ -516,21 +522,51 @@ fn alloc_percpu(cpu_id: u32, lapic_id: u32) -> *mut PerCpu { let ptr = unsafe { alloc_zeroed(layout) } as *mut PerCpu; assert!(!ptr.is_null(), "percpu: alloc failed"); - // SAFETY: the allocation above succeeded (asserted), is `size_of::()` - // bytes at 16-byte alignment, and is zeroed — which is a valid `PerCpu`, - // every field being an integer, an array of them or an atomic over one. It - // is not published anywhere until this function returns, so this `&mut` is - // the only reference to it in the machine. + // SAFETY: the allocation above succeeded (asserted) and is + // `size_of::()` bytes at 16-byte alignment, so it is a writable, + // aligned, uninhabited-by-anything-else place for one `PerCpu`. Nothing has + // a reference to it and nothing reads it until this function returns. + unsafe { + core::ptr::write( + ptr, + PerCpu { + self_ptr: ptr as u64, + cpu_id, + kernel_rsp: 0, + user_rsp: 0, + tss: Tss::new(), + // The idle sentinel, and an asm wire format: `current_tid` and + // `current_pid` are decoded to `Option` at this module's + // boundary, so zero here would name thread 0 of process 0. + current_tid: u32::MAX, + current_pid: u32::MAX, + gdt: GDT_ENTRIES, + idle_stack_top: 0, + syscall_rip: 0, + syscall_num: 0, + syscall_rbp: 0, + preempt_count: AtomicU32::new(0), + need_resched: AtomicU8::new(0), + _pad_after_need_resched: [0; 3], + ring0_timer_fires: AtomicU32::new(0), + last_seen_ring0_fires: 0, + fault_state: CpuFaultState::Normal as u8, + _pad_after_fault_state: [0; 3], + last_armed_ticks: AtomicU32::new(0), + log_shard: alloc_log_shard(cpu_id), + nmi_active: 0, + _pad_after_nmi_active: [0; 4], + irq_counts: [const { AtomicU64::new(0) }; crate::irq_census::SLOTS], + }, + ); + } + + // SAFETY: the write above initialised the allocation, which nothing else + // references, so this `&mut` is the only one in the machine. let percpu = unsafe { &mut *ptr }; - percpu.self_ptr = ptr as u64; - percpu.cpu_id = cpu_id; - percpu.lapic_id = lapic_id; - percpu.current_tid = u32::MAX; - percpu.current_pid = u32::MAX; - percpu.tss = Tss::new(); - percpu.gdt = GDT_ENTRIES; + // After the write and not inside it: the descriptor holds the *address* of + // the TSS, which is a property of where this block was allocated. percpu.init_tss_descriptor(); - percpu.log_shard = alloc_log_shard(cpu_id); // The counters themselves are reached through `gs:`; this is what lets a // *sibling* read them, and it is published here for the same reason the log // shard is — the whole block exists before the CPU it belongs to has run an @@ -850,7 +886,7 @@ fn words(base: u64, len: usize) -> impl Iterator { /// Initialize per-CPU data for the BSP. Call after paging + allocator but before IDT/syscall. pub fn init_bsp(lapic_id: u32) { - let ptr = alloc_percpu(0, lapic_id); + let ptr = alloc_percpu(0); // SAFETY: `alloc_percpu` just returned a live, initialised, never-freed // `PerCpu` that nothing else has a reference to — it is not published into // `IA32_GS_BASE` until the `wrmsr` below. @@ -890,8 +926,8 @@ pub fn init_bsp(lapic_id: u32) { /// Allocate percpu for an AP on the BSP. Returns the raw pointer for the trampoline /// to write into IA32_GS_BASE before loading the IDT. -pub fn alloc_ap(cpu_id: u32, lapic_id: u32) -> *mut PerCpu { - let ptr = alloc_percpu(cpu_id, lapic_id); +pub fn alloc_ap(cpu_id: u32) -> *mut PerCpu { + let ptr = alloc_percpu(cpu_id); // SAFETY: `init_bsp`'s argument — a live, never-freed `PerCpu` nothing else // references. This one runs on the BSP for an AP that has not been sent its // INIT-SIPI yet, so the CPU it belongs to has executed no instruction. diff --git a/kernel/src/arch/smp.rs b/kernel/src/arch/smp.rs index 24c889b3a60..ee6372786cb 100644 --- a/kernel/src/arch/smp.rs +++ b/kernel/src/arch/smp.rs @@ -244,7 +244,7 @@ pub fn boot_aps(madt: &MadtInfo, boot_cr3: u64) { let ap_cpu_id = next_cpu_id; next_cpu_id += 1; CPU_APIC_IDS[ap_cpu_id as usize].store(ap_id, Ordering::Relaxed); - let ap_percpu = percpu::alloc_ap(ap_cpu_id, ap_id); + let ap_percpu = percpu::alloc_ap(ap_cpu_id); data.stack_top = stack_base as u64 + AP_STACK_SIZE as u64; data.entry = ap_entry as *const () as u64; From 56a9065b59d95e4906b780e70de42fb575816bf2 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 27 Aug 2026 10:55:17 +0200 Subject: [PATCH 03/14] "In a syscall" is an identity the CPU records, not a word nobody clears MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The panic handler recovered — killed the current process and rejoined the scheduler — where `syscall_rip() != 0 && current_tid().is_some()`. Nothing ever cleared `syscall_rip`, so on any CPU that had served one syscall the first clause was permanently true, and every panic in IRQ context with a task current read as that task's syscall panic. `PerCpu::syscall_task` is what the question actually needs: the pid and tid of the task this CPU entered a syscall for, or a sentinel. `syscall_handler` brackets the dispatch with it, and `in_syscall()` holds only while the recorded identity is the one this CPU is running. Pid *and* tid, because a tid is per-process and `Tid(0)` is the main thread of every process on the machine. It is deliberately not a guard type: a kernel panic does not unwind, and the panic handler must find the bracket still open. A thread that leaves through `SYS_EXIT` and never returns leaves an identity no live task has, which reads false the moment anything else runs. The predicate errs one way and it is the safe one — a syscall that parked and resumed on a CPU that finished somebody else's in between answers false, so a panic there halts and reports rather than recovering. The `false` direction costs a report; the `true` direction kills an innocent process. Making that case answer true means saving and restoring the word across a context switch as `preempt_count` is, which is `hw::KernelHw::switch`'s to do and not this. The crash report's `Syscall:` block moves to the same predicate. That is the second half of the entry: the block was printed off the stale word and then `user_backtrace` walked an equally stale `syscall_rbp` through the *current* address space. Negative control, measured — a `panic!` staged in `timer_handler`, five Ring 3 timer fires in, on both arms of the same session: base: the process is killed and the machine runs on — `FAIL rs::allocator_stress: exit code -1` — and the report carries `Syscall: num=63 user_rip=0x1000003a036` with a user backtrace into `dlmalloc::malloc+0x776`: a syscall that had already returned, named as the context of a timer interrupt. fixed: `PANIC ... md2 control` with no `Syscall:` block, and every CPU halted — "the guest went quiet because every CPU is halted". Independent oracle: the recorded real failure this entry was reopened on (2026-08-20, a 12-wide `bootable.img` boot-storm capture of a kernel death), where the same stale block printed `Syscall: num=90 user_rip=0x1000003d458` and the backtrace off `syscall_rbp` faulted — `FAULT rip=... cr2=0x0 ... RECURSIVE` — losing the rest of the report. The control arm above reproduces its first half from a staged panic rather than from a boot storm. Green: syscall_cost, allocator_stress, handle_kill_policy, cargo test --lib. `src/prose-ledger` gains three rows deliberately: `arch/percpu.rs` 501 -> 535 (this field, its four accessors, and the whole-struct initialiser two commits back, which is this pull request's), `arch/idt/exceptions.rs` 192 -> 195 and `arch/syscall/gate.rs` 90 -> 95. Closes issues/panic-path/syscall-rip-never-cleared.md. Its two citations went with it: `arch/idt/exceptions.rs` and `sched/kthread.rs`, both of which now state the predicate rather than the defect. No `src/redlist.rs` row is sourced to it. --- .../panic-path/syscall-rip-never-cleared.md | 40 ---------- kernel/src/arch/idt/exceptions.rs | 17 ++-- kernel/src/arch/percpu.rs | 80 ++++++++++++++++--- kernel/src/arch/syscall/gate.rs | 11 ++- kernel/src/main.rs | 17 ++-- kernel/src/sched/kthread.rs | 16 ++-- src/prose-ledger | 6 +- 7 files changed, 104 insertions(+), 83 deletions(-) delete mode 100644 issues/panic-path/syscall-rip-never-cleared.md diff --git a/issues/panic-path/syscall-rip-never-cleared.md b/issues/panic-path/syscall-rip-never-cleared.md deleted file mode 100644 index 82897fa6a3a..00000000000 --- a/issues/panic-path/syscall-rip-never-cleared.md +++ /dev/null @@ -1,40 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-07-31 ---- - -# `percpu.syscall_rip` is never cleared, so "in syscall context" is a guess - -`syscall_entry` stores the user RIP at `gs:[216]` on every SYSCALL and nothing -ever zeroes it. The panic handler's recovery predicate is `syscall_rip() != 0 -&& current_tid().is_some()` (`main.rs`), so on any CPU that has ever served a -syscall the first half is permanently true. A panic in IRQ context — a timer -tick, a scheduler assert — with any task current is therefore treated as a -syscall panic: `try_recover_from_panic` poisons that task, kills the process -and rejoins the scheduler. - -The consequence is backwards from fail-fast: a kernel bug with nothing to do -with the current process kills an innocent process and lets the machine run on, -instead of halting and reporting. `crash_report_panic` prints a "Syscall: -num=... user_rip=..." block off the same stale value, so the report also names -a syscall that is not running. Clearing it on syscall return is one store; the -honest predicate is a per-CPU "in syscall" depth. - -## 2026-08-20: the block does not merely lie, it truncates the report - -Measured, in a 12-wide `bootable.img` boot storm capture of a kernel death: - -``` -[kernel 0.557 cpu0 tid=1] Syscall: num=90 user_rip=0x1000003d458 user_rsp=0xfffe8007b0 -[kernel 0.557 cpu0 tid=1] User backtrace: -[kernel 0.558 cpu0 tid=1] FAULT rip=0xffff80007cb37262 cr2=0x0 err=0x0 … RECURSIVE -``` - -The block is not only printed off a stale value — `user_backtrace` then walks a -stale `syscall_rbp` through the *current* address space, faults, and everything -the crash report had left to say goes with it. That is the last section of -`crash_report_panic`, so on this occasion nothing was lost; anything added after -it would be. The fix for that ordering is already taken (`report_contexts` is -emitted ahead of both backtraces), which is a placement and not a repair: the -stale word is still what sends the walk into a page that is not there. diff --git a/kernel/src/arch/idt/exceptions.rs b/kernel/src/arch/idt/exceptions.rs index 65ef0401ae7..e84c1938c66 100644 --- a/kernel/src/arch/idt/exceptions.rs +++ b/kernel/src/arch/idt/exceptions.rs @@ -328,13 +328,14 @@ fn crash_report_exception(ctx: &ExceptionContext) { } else { kernel_backtrace(ctx.frame.rbp, 32); - // The `Syscall:` line below is the faulting thread's *last* syscall and - // says nothing about where the fault is: `syscall_rip` is never cleared - // (`issues/panic-path/syscall-rip-never-cleared.md`), so it can name a return address - // many syscalls old. Reading it as the fault site cost the AMD `#GP` - // investigation its first day. + // The `Syscall:` line below is where the faulting thread *called in + // from*, not where the fault is — reading it as the fault site cost the + // AMD `#GP` investigation its first day. It is printed only while this + // CPU is inside that thread's own syscall, because the words are its + // entry's and nobody else's; a stale `syscall_rbp` walked through the + // current address space faults and takes the rest of the report with it. let user_rip = percpu::syscall_rip(); - if user_rip != 0 && pid.is_some() { + if percpu::in_syscall() && pid.is_some() { log!(" Syscall: num={} user_rip={:#x} user_rsp={:#x}", percpu::syscall_num(), user_rip, percpu::user_rsp()); log!(" User backtrace:"); @@ -405,8 +406,10 @@ fn crash_report_panic(info: &core::panic::PanicInfo, rbp: u64) { log!(" [Process: PROCESS_TABLE locked, skipping]"); } + // `in_syscall` and not a non-zero word: the three diagnostics belong to + // the entry of the task named above, and are a lie about any other. let user_rip = percpu::syscall_rip(); - if user_rip != 0 { + if percpu::in_syscall() { log!(" Syscall: num={} user_rip={:#x} user_rsp={:#x}", percpu::syscall_num(), user_rip, percpu::user_rsp()); log!(" User backtrace:"); diff --git a/kernel/src/arch/percpu.rs b/kernel/src/arch/percpu.rs index 26f829525b4..1a062086f58 100644 --- a/kernel/src/arch/percpu.rs +++ b/kernel/src/arch/percpu.rs @@ -96,6 +96,10 @@ pub struct PerCpu { pub syscall_num: u64, /// Saved user RBP at last syscall entry (for panic diagnostics). pub syscall_rbp: u64, + /// The task whose syscall this CPU is inside, packed pid:tid, or + /// [`NO_SYSCALL`] — which is what makes the three words above readable: + /// they are diagnostics of *that* task's entry and of no other. + syscall_task: u64, /// `lock add/sub` because IRQ entry/exit and Rust kernel code mutate it /// on the same CPU. pub preempt_count: AtomicU32, @@ -236,6 +240,7 @@ const OFF_IDLE_STACK_TOP: u32 = offset_of!(PerCpu, idle_stack_top) as u32; pub(crate) const OFF_SYSCALL_RIP: u32 = offset_of!(PerCpu, syscall_rip) as u32; pub(crate) const OFF_SYSCALL_NUM: u32 = offset_of!(PerCpu, syscall_num) as u32; pub(crate) const OFF_SYSCALL_RBP: u32 = offset_of!(PerCpu, syscall_rbp) as u32; +const OFF_SYSCALL_TASK: u32 = offset_of!(PerCpu, syscall_task) as u32; pub(crate) const OFF_RING0_TIMER_FIRES: u32 = offset_of!(PerCpu, ring0_timer_fires) as u32; const OFF_LAST_SEEN_RING0_FIRES: u32 = offset_of!(PerCpu, last_seen_ring0_fires) as u32; pub(crate) const OFF_LAST_ARMED_TICKS: u32 = offset_of!(PerCpu, last_armed_ticks) as u32; @@ -326,6 +331,17 @@ pub(crate) mod gs { } } + /// One naturally aligned per-CPU `u64` store. No `lock` prefix, for + /// [`write_u32`]'s reason at eight bytes. + #[inline] + pub fn write_u64(v: u64) { + // SAFETY: `write_u32`'s argument, for eight bytes. + unsafe { + asm!("mov gs:[{off}], {v}", off = const OFF, v = in(reg) v, + options(nostack, preserves_flags)); + } + } + /// One per-CPU byte load. #[inline] pub fn read_u8() -> u8 { @@ -507,11 +523,8 @@ const STACK_FILL_WORD: u64 = u64::from_ne_bytes([STACK_FILL; 8]); /// Allocate and initialize PerCpu for a CPU. Returns a raw pointer (lives forever). /// /// **One `write` of the whole struct, so the allocator zeroes nothing this -/// function means.** A field added to [`PerCpu`] has to be given a value here or -/// the initialiser does not compile; the partial form this replaced named 8 of -/// the fields and left the rest to `alloc_zeroed`, where a new field's default -/// is silently whatever zero means for it — and two of these fields have a -/// non-zero idle state. +/// function means.** A field added to [`PerCpu`] does not compile until it is +/// given a value here, and three of these have a non-zero idle state. fn alloc_percpu(cpu_id: u32) -> *mut PerCpu { let layout = Layout::from_size_align(size_of::(), 16).unwrap(); // SAFETY: `size_of::()` is non-zero and 16 is a power of two, which @@ -523,9 +536,9 @@ fn alloc_percpu(cpu_id: u32) -> *mut PerCpu { assert!(!ptr.is_null(), "percpu: alloc failed"); // SAFETY: the allocation above succeeded (asserted) and is - // `size_of::()` bytes at 16-byte alignment, so it is a writable, - // aligned, uninhabited-by-anything-else place for one `PerCpu`. Nothing has - // a reference to it and nothing reads it until this function returns. + // `size_of::()` bytes at 16-byte alignment, so it is an aligned, + // writable place for one `PerCpu` that nothing else references or reads + // until this function returns. unsafe { core::ptr::write( ptr, @@ -535,9 +548,7 @@ fn alloc_percpu(cpu_id: u32) -> *mut PerCpu { kernel_rsp: 0, user_rsp: 0, tss: Tss::new(), - // The idle sentinel, and an asm wire format: `current_tid` and - // `current_pid` are decoded to `Option` at this module's - // boundary, so zero here would name thread 0 of process 0. + // The idle sentinel: zero would name thread 0 of process 0. current_tid: u32::MAX, current_pid: u32::MAX, gdt: GDT_ENTRIES, @@ -545,6 +556,7 @@ fn alloc_percpu(cpu_id: u32) -> *mut PerCpu { syscall_rip: 0, syscall_num: 0, syscall_rbp: 0, + syscall_task: NO_SYSCALL, preempt_count: AtomicU32::new(0), need_resched: AtomicU8::new(0), _pad_after_need_resched: [0; 3], @@ -1061,7 +1073,51 @@ pub fn idle_stack_top() -> u64 { gs::read_u64::() } -/// User RIP saved at last syscall entry (for panic diagnostics). +/// No task on this CPU is inside a syscall. Not an identity anything can hold: +/// [`pack_task`] never produces it, because no id map issues `u32::MAX`. +const NO_SYSCALL: u64 = u64::MAX; + +/// The identity a syscall bracket records — the pid and the tid together, +/// because a tid is per-process and `Tid(0)` is the main thread of every +/// process on the machine. +fn pack_task(pid: u32, tid: u32) -> u64 { + ((pid as u64) << 32) | tid as u64 +} + +/// Enter this CPU's syscall bracket. `arch::syscall` is the only caller. +pub fn enter_syscall() { + gs::write_u64::(pack_task( + gs::read_u32::(), + gs::read_u32::(), + )); +} + +/// …and leave it. +pub fn leave_syscall() { + gs::write_u64::(NO_SYSCALL); +} + +/// Whether the task this CPU is running is inside a syscall right now. +/// +/// **An identity and not a flag**, because the word is per-CPU while the +/// question is about a thread: the comparison is what rules out a thread in +/// Ring 3 on a CPU that once served a syscall, and a thread in Ring 3 while a +/// sibling sits parked inside one. +/// +/// It errs one way, the safe one: a syscall that parked and resumed on a CPU +/// that finished somebody else's in between answers `false`, so a panic there +/// halts and reports instead of recovering. The other answer needs the word +/// saved and restored across a switch, as `preempt_count` is +/// (`hw::KernelHw::switch`). +pub fn in_syscall() -> bool { + let recorded = gs::read_u64::(); + recorded != NO_SYSCALL + && recorded + == pack_task(gs::read_u32::(), gs::read_u32::()) +} + +/// User RIP saved at last syscall entry, meaningful only while +/// [`in_syscall`] holds. pub fn syscall_rip() -> u64 { gs::read_u64::() } diff --git a/kernel/src/arch/syscall/gate.rs b/kernel/src/arch/syscall/gate.rs index b12058ee692..29faf3b1960 100644 --- a/kernel/src/arch/syscall/gate.rs +++ b/kernel/src/arch/syscall/gate.rs @@ -167,8 +167,17 @@ extern "sysv64" fn syscall_entry() { ); } +/// The syscall bracket, which is what makes the entry's three diagnostic +/// stores readable afterwards. +/// +/// **Not a guard type**: a panic inside the dispatch does not unwind, and that +/// is the case that must find the bracket still open — the panic handler asks +/// [`percpu::in_syscall`] whether killing this process is the honest answer. extern "sysv64" fn syscall_handler(num: u64, a1: u64, a2: u64, _: u64, a3: u64, a4: u64) -> u64 { #[cfg(feature = "df-witness")] cpu::df_witness("syscall_handler"); - syscall_dispatch(num, a1, a2, a3, a4) + percpu::enter_syscall(); + let out = syscall_dispatch(num, a1, a2, a3, a4); + percpu::leave_syscall(); + out } diff --git a/kernel/src/main.rs b/kernel/src/main.rs index 8598d489510..8846fd1c753 100644 --- a/kernel/src/main.rs +++ b/kernel/src/main.rs @@ -239,16 +239,13 @@ fn panic(info: &core::panic::PanicInfo) -> ! { // is fully handled — reset the reentry guard so a future, independent // panic on this CPU still reports. // - // **A kernel thread answers from its own row and never from the two words - // below**, because for one of them the words do not merely give the wrong - // answer — they give a *nondeterministic* one. `syscall_rip` is never - // cleared (`issues/panic-path/syscall-rip-never-cleared.md`), so a - // kernel task reads whatever user thread last ran on this CPU left behind: - // the same panic on the same build would recover or halt depending on which - // CPU work stealing had put the thread on. `sched::kthread` is where the - // answer is a property of the thread instead. - let recoverable = sched::kthread::panic_recovers_here() - .unwrap_or_else(|| percpu::syscall_rip() != 0 && percpu::current_tid().is_some()); + // **A kernel thread answers from its own row**, because the question below + // is not about it: it has no syscall to be inside, and `sched::kthread` is + // where the answer is a property of the thread. Everything else recovers + // only where the panicking task is the one this CPU is inside a syscall + // for — the one context `try_recover_from_panic` can abandon, since killing + // its process is what abandoning it means. + let recoverable = sched::kthread::panic_recovers_here().unwrap_or_else(percpu::in_syscall); if recoverable { depth.store(0, core::sync::atomic::Ordering::SeqCst); // The captured report dies with the panic it belongs to. Left set, it diff --git a/kernel/src/sched/kthread.rs b/kernel/src/sched/kthread.rs index 572b038f6b2..8661d008a31 100644 --- a/kernel/src/sched/kthread.rs +++ b/kernel/src/sched/kthread.rs @@ -74,16 +74,12 @@ const CLAIMING: u64 = u64::MAX - 1; /// One kernel thread, and whether a panic inside it may be recovered from. /// -/// **The column exists because the ordinary predicate is not merely wrong for -/// a kernel thread, it is nondeterministic.** `main.rs`'s panic handler -/// recovers when `percpu::syscall_rip() != 0 && percpu::current_tid().is_some()` -/// — and `syscall_rip` is *never cleared* -/// (`issues/panic-path/syscall-rip-never-cleared.md`, and -/// `arch/idt/exceptions.rs` says so in its own comment). A kernel thread has a -/// tid, so the second clause holds; the first reads whatever user thread last -/// ran on *this* CPU left behind. The same panic on the same build therefore -/// recovers or halts depending on which CPU the thread happened to be -/// scheduled on. The row is what makes the answer a property of the thread. +/// **The column exists because the ordinary predicate is not about a kernel +/// thread at all.** `main.rs`'s panic handler recovers where +/// `percpu::in_syscall()` holds, and a kernel thread has no syscall to be +/// inside: there is no process to kill and no user frame to return to, so +/// every one of them would answer `false` and halt the machine. The row is +/// what makes the answer a property of the thread. /// /// **[`Row::task`] is the last word written and the first word read**, and that /// is the whole publication protocol. A reader searches on the identity, so diff --git a/src/prose-ledger b/src/prose-ledger index 529358c6210..538abbdbbc0 100644 --- a/src/prose-ledger +++ b/src/prose-ledger @@ -29,7 +29,7 @@ kernel/src/arch/entry.rs 96 0 kernel/src/arch/fpu.rs 81 0 kernel/src/arch/idt/device_irq.rs 25 0 kernel/src/arch/idt/dma_fault.rs 9 0 -kernel/src/arch/idt/exceptions.rs 192 1 +kernel/src/arch/idt/exceptions.rs 195 1 kernel/src/arch/idt/hda.rs 4 0 kernel/src/arch/idt/i8042.rs 3 0 kernel/src/arch/idt/log_nest.rs 15 0 @@ -43,13 +43,13 @@ kernel/src/arch/idt/xhci.rs 6 0 kernel/src/arch/mod.rs 118 0 kernel/src/arch/mtrr.rs 54 0 kernel/src/arch/pat.rs 74 0 -kernel/src/arch/percpu.rs 501 0 +kernel/src/arch/percpu.rs 535 0 kernel/src/arch/smp.rs 125 0 kernel/src/arch/syscall/debug.rs 64 0 kernel/src/arch/syscall/device.rs 59 0 kernel/src/arch/syscall/dispatch.rs 229 1 kernel/src/arch/syscall/fs.rs 76 0 -kernel/src/arch/syscall/gate.rs 90 0 +kernel/src/arch/syscall/gate.rs 95 0 kernel/src/arch/syscall/handles.rs 75 0 kernel/src/arch/syscall/io.rs 51 0 kernel/src/arch/syscall/ipc.rs 133 0 From 5382539fd5fc92cf31cd6c6db9b4e53027c25d23 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 27 Aug 2026 11:09:18 +0200 Subject: [PATCH 04/14] The vector this kernel names in the SVR has a gate, and it is asked before it acknowledges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `apic::enable_x2apic` writes `0xFF` into the spurious-interrupt vector field on the BSP and on every AP, and the IDT left slot 0xFF `IdtEntry::EMPTY` — `P = 0`. A vector the CPU can deliver through a non-present gate is a contributory fault and the CPU escalates to `#DF`, which halts the machine: the rule `9bd7a9e` wrote above `idt_vectors!` for the range Intel names, on the one vector the platform names instead. `arch/idt/spurious.rs` is the gate. It is `ring0` — it reaches no task, touches no preempt count and reschedules nothing — and it does not log, for `arch/idt/nmi.rs`'s reason: it can arrive inside the log's own commit bracket. What it does is the census's single `add` and then the difficult part, which is the acknowledgment: a genuine spurious interrupt sets no ISR bit (SDM Vol. 3A §11.9), so an unconditional `eoi()` would clear an unrelated interrupt's bit and lose it, while the same vector reached by an IPI does go through the IRR and does need one. The handler reads the in-service register and acknowledges only what is in service. `irq_census::Source::Spurious` is where the count lands, because the handler cannot say anything itself. A non-zero column on a machine that staged nothing is an interrupt-routing defect, and this is its only witness. The gate is exercised on every run rather than shipped unentered. Nothing on this host raises the vector by itself — the SDM's classic condition needs a task-priority register this kernel never writes, and every device here is MSI or MSI-X — so `lapic-spurious-selftest` raises it deliberately and reports three things: that the delivery arrived and was counted, that the vector is no longer in service afterwards, and that the CPU went on taking interrupts. Two negative controls, each measured on its own arm of the same session: the gate removed, the delivery still staged: the guest never reaches `===READY===` and the harness reports `the console carried: nothing at all` — the halted machine with no name on it, which is exactly the defect. the gate kept, the acknowledgment removed: `LAPIC: spurious selftest FAILED — vector 0xff is still in service, so nothing below priority 0xF can be delivered on cpu0 again`, from the kernel's own arm. Independent oracle: Intel SDM Vol. 3A — §11.9 for what a spurious interrupt is and that it needs no EOI, §11.8.4 for an in-service bit blocking every lower priority, and §6.14's contributory-fault table for the escalation a `P = 0` gate produces. The tree's own recorded failure is the same mechanism on the exception range: `9bd7a9e`'s `div` by zero took the whole guest down before every Intel-named vector had a gate. `lapic_spurious_vector` is a new registered name, `Sched::Parallel`, `Tier::Fast`, committed to `tests/test-durations` with the `UNMEASURED` marker — the one measured run it buys is by design, and the price verdict lands on the run that measures it. Green: lapic_spurious_vector, irq_census_conservation (whose host-side `SOURCES` gains the same column), cargo test --lib. `src/prose-ledger`: `arch/idt/spurious.rs` enters at 50; `arch/idt/mod.rs` 223 -> 235, `irq_census.rs` 117 -> 125, `main.rs` 328 -> 329, `actuator.rs` 436 -> 442, `arch/apic.rs` 251 -> 263, `tests/toyos.rs` 4777 -> 4794. The dated column is untouched. Closes issues/kernel/the-lapic-spurious-vector-has-no-gate.md. Its one citation, in `a-double-fault-on-cpu-1-under-a-wide-suite.md`, is updated in this commit: that reading is closed and was never the claim. The entry's last paragraph — the other 235 empty slots — is not this vector's defect and is filed as issues/kernel/an-unclaimed-vector-halts-the-machine-with-no-name.md, with the control above as its evidence. No `src/redlist.rs` row is sourced to either. --- ...ouble-fault-on-cpu-1-under-a-wide-suite.md | 20 +-- ...d-vector-halts-the-machine-with-no-name.md | 39 +++++ .../the-lapic-spurious-vector-has-no-gate.md | 87 ---------- kernel/src/actuator.rs | 8 + kernel/src/arch/apic.rs | 20 ++- kernel/src/arch/idt/mod.rs | 14 ++ kernel/src/arch/idt/spurious.rs | 151 ++++++++++++++++++ kernel/src/irq_census.rs | 23 ++- kernel/src/main.rs | 8 + src/prose-ledger | 13 +- tests/common/irqcensus.rs | 4 +- tests/test-durations | 1 + tests/toyos.rs | 46 ++++++ 13 files changed, 327 insertions(+), 107 deletions(-) create mode 100644 issues/kernel/an-unclaimed-vector-halts-the-machine-with-no-name.md delete mode 100644 issues/kernel/the-lapic-spurious-vector-has-no-gate.md create mode 100644 kernel/src/arch/idt/spurious.rs diff --git a/issues/kernel/a-double-fault-on-cpu-1-under-a-wide-suite.md b/issues/kernel/a-double-fault-on-cpu-1-under-a-wide-suite.md index ee8f2be3fcc..7c890d63307 100644 --- a/issues/kernel/a-double-fault-on-cpu-1-under-a-wide-suite.md +++ b/issues/kernel/a-double-fault-on-cpu-1-under-a-wide-suite.md @@ -162,15 +162,17 @@ What survives of that class was audited here, in code: every redirection entry firmware left, so neither can deliver a vector at all. **Closed.** - every vector Intel names for 64-bit mode has a gate. **Closed by `9bd7a9e`.** -- **the LAPIC's spurious-interrupt vector is `0xFF` and the IDT has no entry at - `0xFF`.** That is a defect on its own terms and is filed as - `issues/kernel/the-lapic-spurious-vector-has-no-gate.md`. **It is not - claimed as the cause of this sighting** — a spurious interrupt leaves nothing - behind, and what would deliver one on *this* configuration is itself - unestablished (that file has the reading). It is recorded here because - a `#DF` naming a live pid is precisely what it would look like, and because - closing it removes one of the three readings the next sighting has to be - weighed against. +- the LAPIC's spurious-interrupt vector is `0xFF`, and the IDT had no entry at + `0xFF`. **Closed**: `arch/idt/spurious.rs` is that gate, and + `lapic_spurious_vector` raises the vector on purpose on every run. **It was + never claimed as the cause of this sighting** — a spurious interrupt leaves + nothing behind, and what would have delivered one on *this* configuration was + itself unestablished. It is recorded here because a `#DF` naming a live pid is + precisely what it would have looked like, and because closing it removes one + of the three readings the next sighting has to be weighed against. +- **the other 235 `IdtEntry::EMPTY` slots are unchanged**, and each turns an + interrupt nobody expected into the same halt — filed as + `issues/kernel/an-unclaimed-vector-halts-the-machine-with-no-name.md`. ## Reproduction diff --git a/issues/kernel/an-unclaimed-vector-halts-the-machine-with-no-name.md b/issues/kernel/an-unclaimed-vector-halts-the-machine-with-no-name.md new file mode 100644 index 00000000000..b24a4c4c510 --- /dev/null +++ b/issues/kernel/an-unclaimed-vector-halts-the-machine-with-no-name.md @@ -0,0 +1,39 @@ +--- +status: open +kind: defect +opened: 2026-08-27 +--- + +# 235 IDT entries are `P = 0`, and a delivery through one halts the machine saying nothing + +`kernel/src/arch/idt/mod.rs` fills the table from `idt_vectors!` and leaves +every other slot `IdtEntry::EMPTY`, whose `type_attr` is `0`. A vector delivered +through a gate with `P = 0` is not a fault the process takes: the CPU treats the +missing gate as a second, contributory fault and escalates to `#DF`, which +`double_fault_handler` answers with `halt_all_cpus`. + +## Measured, on this tree + +Taken as the negative control of the spurious-vector gate (2026-08-27): with +that gate's row removed and the delivery still staged, the guest never reaches +`===READY===` and the harness reports **`the console carried: nothing at all`**. +Not a report, not a `DOUBLE FAULT` line, not a panic — a machine that stopped +with no name on it, which is the whole of what this entry is about. With the row +in place the same boot is green. + +## What would close it + +A single naked entry installed in every otherwise-unfilled slot, which counts +the vector it took and reports it — the shape `arch/idt/spurious.rs` already +has, minus the conditional EOI question, which it inherits: an unexpected +vector may or may not be in service and the handler has to ask before it +acknowledges. + +The reachable ones are not hypothetical. A stale MSI-X table entry left by a +driver reconfiguration names a vector nothing gated; so does firmware that left +an I/O APIC redirection entry pointing somewhere this kernel does not, which +`ioapic::init` masks precisely because of it. + +**What it costs to get wrong**: a gate that absorbs a vector silently is a +machine hiding an interrupt-routing defect, so the count and the report are the +point, not the survival. diff --git a/issues/kernel/the-lapic-spurious-vector-has-no-gate.md b/issues/kernel/the-lapic-spurious-vector-has-no-gate.md deleted file mode 100644 index e277dfc73b4..00000000000 --- a/issues/kernel/the-lapic-spurious-vector-has-no-gate.md +++ /dev/null @@ -1,87 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-08-18 ---- - -# The kernel programs the LAPIC to deliver vector `0xFF` and installs no gate for it - -`kernel/src/arch/apic.rs`, `enable_x2apic`, on the BSP and on every AP: - -```rust -let svr = cpu::rdmsr(X2APIC_SVR); -cpu::wrmsr(X2APIC_SVR, svr | (1 << 8) | 0xFF); -``` - -so this machine's spurious-interrupt vector is `0xFF`. `kernel/src/arch/idt/mod.rs` -declares gates for the exception vectors Intel names, for NMI, for `0x20..=0x26`, -for `0xFD` (halt IPI) and for `0xFE` (TLB flush), plus `0x27` in a kernel built -with `boot-actuators`. Every other entry of the 256 is `IdtEntry::EMPTY`, whose -`type_attr` is `0` — **P=0**. - -**A fault or interrupt delivered through a gate with P=0 is a contributory fault -and the CPU escalates to `#DF`, which `double_fault_handler` answers with -`halt_all_cpus`.** That is not a deduction from the manual: this tree reproduced -it and fixed it for the exception range in `9bd7a9e` — *"`div` by zero in any -Ring 3 process took the whole guest down … the CPU escalated to #DF … Reproduced -before the fix: `DOUBLE FAULT on CPU 0 (pid=Some(Pid(3)))` and a timed-out test, -wide and alone."* The comment that commit left above `idt_vectors!` states the -rule the SVR write breaks: - -> Every vector Intel names for 64-bit mode has a gate, because a vector without -> one does not fault the process: the CPU takes the missing gate as a second, -> contributory fault and escalates to #DF, which halts the machine. - -The spurious vector is not one Intel names as an exception; it is one **this -kernel names**, by writing it into SVR. So it is the same rule and the same -mechanism, on the one vector the fix did not reach. - -## What is *not* claimed - -That this caused -`issues/kernel/a-double-fault-on-cpu-1-under-a-wide-suite.md`. It cannot -be claimed: a spurious interrupt leaves nothing behind, and that sighting's -report was never printed. This file exists because the hole is provable from the -source with no appeal to that sighting at all. - -Nor is the delivery rate known, and what can be read of it here is narrow: - -- The task-priority register is never written — `X2APIC_TPR` (`0x808`) appears - nowhere in the tree — so the SDM's classic spurious condition, an interrupt - masked by a TPR raised between assertion and `INTA`, cannot arise from this - kernel's own doing. -- `stop_timer` writes `0` to `X2APIC_TIMER_INIT` and does **not** mask the LVT, - so an already-latched timer interrupt is still delivered on `0x20`. -- The only LVT mask is `wrmsr(X2APIC_LVT_TIMER, 1 << 16)` during BSP - calibration. -- Every device on this machine is MSI or MSI-X, and the two ISA lines are - edge-triggered. - -So the honest statement is: **the gate is missing and its absence is lethal; -what would deliver it here is not established.** On the T14 — different -firmware, different interrupt topology, a real IOAPIC with level-triggered -lines — that last clause is worth a great deal less. - -## What a fix has to get right - -Not simply an `iretq`. A genuine spurious interrupt is **not** acknowledged — -the LAPIC set no ISR bit, and an EOI from the handler would clear an unrelated -interrupt's bit instead. But the same vector reached by a self-IPI or by an ICR -send *does* go through the IRR and *does* need one. The handler therefore has to -read the ISR (x2APIC MSRs `0x810..=0x817`) and EOI only when the vector is -actually in service — which is also what makes the fix stageable: `apic::send_self` -already exists (`log/nested.rs` uses it), so a test can raise the vector on -purpose and assert the machine survives, the count moved, and no interrupt after -it was lost. - -The count is the second half. A spurious interrupt that is absorbed silently is -a machine hiding an interrupt-routing defect, and the handler may not `log!` — -it can arrive inside the log commit path, which is the whole reason -`LOG_NEST_VECTOR` exists. `kernel/src/arch/idt/nmi.rs` is the shape: a lock-free -per-CPU slot written by the handler and read from ordinary context. - -While somebody is there: **236 of the 256 IDT entries are `EMPTY`**, so every -one of them turns an interrupt nobody expected into a halted machine with no -name on it. `9bd7a9e` closed the range Intel defines; the range the platform -defines is still open, and a stale MSI-X entry left by a driver reconfiguration -lands in it. diff --git a/kernel/src/actuator.rs b/kernel/src/actuator.rs index 7e75b5f8c5a..94416f34e35 100644 --- a/kernel/src/actuator.rs +++ b/kernel/src/actuator.rs @@ -336,6 +336,14 @@ actuators! { /// kernel. See `drivers/virtio.rs`'s `used_selftest`. virtio_used_selftest = "virtio-used-selftest"; + /// Raise the local APIC's spurious vector on this CPU on purpose, once, and + /// report whether the machine survived it, the handler counted it, and the + /// next interrupt still arrived. Nothing outside the guest reaches that + /// vector: this CPU's own LAPIC delivers it, its classic condition needs a + /// task-priority register this kernel never writes, and no QEMU device, + /// machine property or `-cpu` flag produces one. See `arch/idt/spurious.rs`. + lapic_spurious_selftest = "lapic-spurious-selftest"; + /// Leave every AP holding the CR0 and CR4 that INIT left it: caching /// disabled, WP clear, NE clear. A control register is written by the guest /// and read by nobody outside it, so no QEMU device, machine property or diff --git a/kernel/src/arch/apic.rs b/kernel/src/arch/apic.rs index 9c22e990ce5..897a26f9594 100644 --- a/kernel/src/arch/apic.rs +++ b/kernel/src/arch/apic.rs @@ -28,6 +28,9 @@ enum Reg { Id = 0x802, Eoi = 0x80B, Svr = 0x80F, + /// The first of the eight in-service words, 0x810..=0x817. Read through + /// [`in_service`], which is where a vector picks one of the eight. + Isr0 = 0x810, Icr = 0x830, LvtTimer = 0x832, TimerInit = 0x838, @@ -69,8 +72,11 @@ fn enable_x2apic() { base |= (1 << 11) | (1 << 10); Reg::ApicBase.write(base); + // Bit 8 software-enables the APIC; the low byte is the vector it delivers + // when it takes an interrupt back, and `arch::idt::spurious` is the gate + // that number has to have. let svr = Reg::Svr.read(); - Reg::Svr.write(svr | (1 << 8) | 0xFF); + Reg::Svr.write(svr | (1 << 8) | super::idt::spurious::SPURIOUS_VECTOR as u64); } /// Initialize the BSP's Local APIC in x2APIC mode. @@ -106,6 +112,18 @@ pub fn eoi() { Reg::Eoi.write(0); } +/// Whether `vector` is in service on this CPU — the one question that separates +/// an interrupt the LAPIC delivered from one it took back. +/// +/// The in-service register is eight 32-bit words (SDM Vol. 3A §12.8.4): the +/// vector's high five bits pick the word and its low five the bit. `Isr0` plus +/// an offset rather than eight variants, because the eight *are* one array and +/// `vector >> 5` cannot leave it. +pub fn in_service(vector: u8) -> bool { + let word = cpu::rdmsr(Reg::Isr0 as u32 + (vector as u32 >> 5)); + (word >> (vector & 31)) & 1 != 0 +} + /// Send an IPI to **this** CPU (self shorthand), for the one caller that needs /// an interrupt whose delivery time is decided by `IF` alone. /// diff --git a/kernel/src/arch/idt/mod.rs b/kernel/src/arch/idt/mod.rs index 1cd7511c948..68434401ffa 100644 --- a/kernel/src/arch/idt/mod.rs +++ b/kernel/src/arch/idt/mod.rs @@ -6,6 +6,7 @@ mod i8042; #[cfg(feature = "boot-actuators")] mod log_nest; mod nmi; +pub(crate) mod spurious; mod timer; mod tlb; mod virtio_net; @@ -350,6 +351,19 @@ idt_vectors! { // Ring 0 because it never returns: `cli; hlt` forever. ring0 HaltAll = 0xFD, stub_halt_all; ring3 TlbFlush = 0xFE, tlb::tlb_flush_entry; + // The vector this kernel names by writing it into the SVR, which is + // why it is on a table whose rule is Intel's names: a vector the CPU + // can deliver through a `P = 0` slot is the escalation above, and the + // platform names this one. Ring 0 because the handler reaches no task — + // it counts the delivery and acknowledges it only if the in-service + // register says it is one that may be acknowledged. + ring0 Spurious = 0xFF, spurious::spurious_entry; + // The vector this kernel names by writing it into the SVR, which is + // why it is on a table whose rule is Intel's names: a vector the CPU + // can deliver through a `P = 0` slot is the escalation above, and the + // platform names this one. Ring 0 because the handler reaches no task — + // it counts the delivery and acknowledges it only if the in-service + // register says it is one that may be acknowledged. } } diff --git a/kernel/src/arch/idt/spurious.rs b/kernel/src/arch/idt/spurious.rs new file mode 100644 index 00000000000..b8577571a9f --- /dev/null +++ b/kernel/src/arch/idt/spurious.rs @@ -0,0 +1,151 @@ +//! Vector 0xFF, which this kernel names by writing it into the SVR. +//! +//! `apic::enable_x2apic` puts `0xFF` in the spurious-interrupt vector field on +//! the BSP and on every AP, so the local APIC will deliver that vector whenever +//! it takes back an interrupt it had already signalled. A vector the CPU can +//! deliver and the IDT leaves `P = 0` is not a fault: the missing gate is a +//! second, contributory fault and the CPU escalates to `#DF`, which +//! `double_fault_handler` answers by halting the machine. That is the rule +//! `idt_vectors!`' own comment states for the exception range, and this is the +//! one vector the platform — rather than Intel — names. +//! +//! **The EOI is conditional, and that is the whole of the handler's +//! difficulty.** A genuine spurious interrupt sets no ISR bit (SDM Vol. 3A +//! §11.9), so an unconditional `eoi()` here would clear some *other* +//! interrupt's in-service bit and lose it. The same vector reached by a +//! deliberate IPI does go through the IRR and does need one — and without it +//! the ISR bit at priority 0xF blocks every lower-priority vector on this CPU +//! for the rest of the boot, the timer included. So the handler asks the ISR +//! which of the two it is. +//! +//! **It does not log**, for `idt::nmi`'s reason: it can arrive inside the log's +//! own commit bracket. It records one delivery in the interrupt census, which +//! is a single `add` to this CPU's own counter block and reaches no lock. + +use core::arch::naked_asm; + +use crate::arch::apic; + +/// The vector `apic::enable_x2apic` writes into the SVR. Public because the +/// gate's row and the register write are two places, and only one may decide. +pub const SPURIOUS_VECTOR: u8 = 0xFF; + +#[unsafe(naked)] +pub(super) extern "sysv64" fn spurious_entry() { + naked_asm!( + // The `cld` every Ring 0 entry owes itself (`arch::entry`), at a gate + // that is not routed through `ring3_naked_asm!`: this vector can arrive + // between any two instructions, `memmove`'s `std` … `cld` window + // included, and `took` is a `sysv64` call. + "cld", + "push rax", + "push rcx", + "push rdx", + "push rsi", + "push rdi", + "push r8", + "push r9", + "push r10", + "push r11", + "push rbp", + "mov rbp, rsp", + "and rsp, -16", + "call {took}", + "mov rsp, rbp", + "pop rbp", + "pop r11", + "pop r10", + "pop r9", + "pop r8", + "pop rdi", + "pop rsi", + "pop rdx", + "pop rcx", + "pop rax", + "iretq", + took = sym took, + ); +} + +/// One delivery of the spurious vector, counted and acknowledged if it is one +/// of the deliveries that can be. +extern "sysv64" fn took() { + crate::irq_census::irq_took!(Spurious); + if apic::in_service(SPURIOUS_VECTOR) { + apic::eoi(); + } +} + +/// Raise the spurious vector on this CPU on purpose, and check the three things +/// a boot cannot otherwise certify: that the machine survives the delivery, +/// that the handler ran, and that the interrupt after it is not lost. +/// +/// **Nothing on this host produces a genuine spurious interrupt.** The SDM's +/// classic condition is an interrupt masked by a task-priority register raised +/// between assertion and `INTA`, and this kernel never writes `TPR`; every +/// device on the machine is MSI or MSI-X. So without this the gate would ship +/// never having been entered — and the third assertion is the one that matters, +/// because a handler that acknowledged nothing would leave an ISR bit set at +/// priority 0xF and starve every vector below it, the timer included. +#[cfg(feature = "boot-actuators")] +pub fn selftest() { + use crate::irq_census::Source; + + let cpu = crate::arch::percpu::cpu_id(); + let Some(before) = crate::irq_census::deliveries(cpu, Source::Spurious) else { + crate::log!("LAPIC: spurious selftest FAILED — this CPU publishes no census block"); + return; + }; + let taken_before = crate::irq_census::deliveries_total(cpu).unwrap_or(0); + + apic::send_self(SPURIOUS_VECTOR); + + // A self-IPI is delivered as soon as `IF` allows, which is now; the budget + // is what turns "it never arrived" into a verdict instead of a hang. + const ARRIVES: crate::time::Budget = crate::time::Budget::of( + crate::time::Duration::from_millis(50), + "the delivery is reported as never having arrived", + ); + let delivered = crate::clock::settles(ARRIVES.nanos(), || { + crate::irq_census::deliveries(cpu, Source::Spurious).unwrap_or(before) > before + }); + if !delivered { + crate::log!("LAPIC: spurious selftest FAILED — vector {SPURIOUS_VECTOR:#x} never arrived"); + return; + } + + // Acknowledged. A missing EOI does not fault: it leaves the bit set, this + // CPU's in-service priority at 0xF, and every lower vector — the timer + // included — undeliverable for the rest of the boot (SDM Vol. 3A §11.8.4). + if apic::in_service(SPURIOUS_VECTOR) { + crate::log!( + "LAPIC: spurious selftest FAILED — vector {SPURIOUS_VECTOR:#x} is still in service, so \ + nothing below priority 0xF can be delivered on cpu{cpu} again" + ); + return; + } + + // …and the machine takes interrupts after it, which is that argument's + // observable half. Any source will do: what is being ruled out is a CPU + // that has gone deaf, not a particular device. + let ran_on = crate::clock::settles(ARRIVES.nanos(), || { + crate::irq_census::deliveries_total(cpu).unwrap_or(taken_before) > taken_before + }); + if !ran_on { + crate::log!( + "LAPIC: spurious selftest FAILED — cpu{cpu} took no interrupt at all after the \ + spurious one" + ); + return; + } + + let after = crate::irq_census::deliveries(cpu, Source::Spurious).unwrap_or(before); + crate::log!( + "LAPIC: spurious selftest 3/3 — vector {:#x} delivered on cpu{} ({} -> {}), acknowledged, \ + and the CPU took interrupts after it", + SPURIOUS_VECTOR, + cpu, + before, + after, + ); +} diff --git a/kernel/src/irq_census.rs b/kernel/src/irq_census.rs index e25c8adf1e3..fc80c1a3456 100644 --- a/kernel/src/irq_census.rs +++ b/kernel/src/irq_census.rs @@ -90,16 +90,21 @@ pub enum Source { Tlb, /// Vector 0x02, and `sched::dump` is its only sender. Nmi, + /// Vector 0xFF, the local APIC's spurious vector — an interrupt it + /// signalled and then took back. A non-zero count on a machine that staged + /// nothing is an interrupt-routing defect this census is the only witness + /// to, because the handler may not log. + Spurious, } impl Source { - pub const COUNT: usize = 9; + pub const COUNT: usize = 10; /// The census's field names, in variant order. Read by the host side, so /// they are part of what a capture means: `tests/toyos.rs`'s /// `irq_census_conservation` parses them back. pub const NAMES: [&'static str; Self::COUNT] = - ["timer", "xhci", "net", "sound", "i8042", "dmafault", "hda", "tlb", "nmi"]; + ["timer", "xhci", "net", "sound", "i8042", "dmafault", "hda", "tlb", "nmi", "spurious"]; } /// How many `u64`s one CPU's counter block holds: the total, then one per @@ -195,6 +200,20 @@ impl fmt::Display for Fields<'_> { } } +/// What one CPU has taken from one source, or `None` if that CPU has never +/// been built. The only read of a single counter: everything else prints the +/// whole census, and a caller that wants a *difference* needs one number twice. +#[cfg(feature = "boot-actuators")] +pub fn deliveries(cpu: u32, source: Source) -> Option { + read(cpu).map(|counts| counts[1 + source as usize]) +} + +/// …and everything one CPU has taken, from the counter written beside them. +#[cfg(feature = "boot-actuators")] +pub fn deliveries_total(cpu: u32) -> Option { + read(cpu).map(|counts| counts[TOTAL]) +} + /// One `irq: cpuN total=… =… …` line per online CPU. /// /// The counts are cumulative since boot, so the last such line a capture holds diff --git a/kernel/src/main.rs b/kernel/src/main.rs index 8846fd1c753..f347029fb3f 100644 --- a/kernel/src/main.rs +++ b/kernel/src/main.rs @@ -691,6 +691,14 @@ unsafe fn kernel_main(kernel_args: &KernelArgs) -> ! { drivers::virtio::used_selftest(); } + // Here because it needs interrupts on and a timer already ticking: the last + // of its three assertions is that the interrupt *after* the spurious one + // still arrives. + #[cfg(feature = "boot-actuators")] + if actuator::lapic_spurious_selftest() { + arch::idt::spurious::selftest(); + } + virtio_console::init(&pci_devices); virtio_net::init(&pci_devices); diff --git a/src/prose-ledger b/src/prose-ledger index 538abbdbbc0..475b7481800 100644 --- a/src/prose-ledger +++ b/src/prose-ledger @@ -21,8 +21,8 @@ kernel-loom/tests/reap_gate.rs 53 1 kernel-loom/tests/sleep_lock.rs 80 1 kernel-loom/tests/ticket_lock.rs 31 1 kernel-loom/tests/tlb_shootdown.rs 93 2 -kernel/src/actuator.rs 436 0 -kernel/src/arch/apic.rs 251 0 +kernel/src/actuator.rs 442 0 +kernel/src/arch/apic.rs 263 0 kernel/src/arch/control_regs.rs 233 0 kernel/src/arch/cpu.rs 220 0 kernel/src/arch/entry.rs 96 0 @@ -33,8 +33,9 @@ kernel/src/arch/idt/exceptions.rs 195 1 kernel/src/arch/idt/hda.rs 4 0 kernel/src/arch/idt/i8042.rs 3 0 kernel/src/arch/idt/log_nest.rs 15 0 -kernel/src/arch/idt/mod.rs 223 0 +kernel/src/arch/idt/mod.rs 235 0 kernel/src/arch/idt/nmi.rs 117 0 +kernel/src/arch/idt/spurious.rs 50 0 kernel/src/arch/idt/timer.rs 36 0 kernel/src/arch/idt/tlb.rs 9 0 kernel/src/arch/idt/virtio_net.rs 6 0 @@ -110,7 +111,7 @@ kernel/src/iommu/vtd/fault.rs 103 0 kernel/src/iommu/vtd/mod.rs 188 0 kernel/src/iommu/vtd/queue.rs 49 0 kernel/src/iommu/vtd/table.rs 132 0 -kernel/src/irq_census.rs 117 0 +kernel/src/irq_census.rs 125 0 kernel/src/irq_ring.rs 52 0 kernel/src/keyboard.rs 71 0 kernel/src/loader/mod.rs 246 0 @@ -125,7 +126,7 @@ kernel/src/log/registry.rs 53 0 kernel/src/log/shard.rs 281 0 kernel/src/log/storm.rs 105 2 kernel/src/log/user.rs 97 0 -kernel/src/main.rs 328 0 +kernel/src/main.rs 329 0 kernel/src/mm/alloc.rs 343 0 kernel/src/mm/dma.rs 190 0 kernel/src/mm/mmio.rs 62 0 @@ -346,7 +347,7 @@ tests/toyos-rust-tests/tls-dlopen-lib/src/lib.rs 10 0 tests/toyos-rust-tests/tls-lib/src/lib.rs 0 0 tests/toyos-rust-tests/tls-multi-crate/dep/src/lib.rs 3 0 tests/toyos-rust-tests/tls-multi-crate/src/lib.rs 10 0 -tests/toyos.rs 4777 23 +tests/toyos.rs 4794 23 toyos-abi/src/audio.rs 21 0 toyos-abi/src/boot.rs 90 0 toyos-abi/src/handle.rs 107 0 diff --git a/tests/common/irqcensus.rs b/tests/common/irqcensus.rs index d3c852395d5..638815e687c 100644 --- a/tests/common/irqcensus.rs +++ b/tests/common/irqcensus.rs @@ -22,8 +22,8 @@ use std::sync::Mutex; /// and [`Census::parse`] refuses a line whose fields are not exactly these, so /// a source added on one side and not the other is a red rather than a silently /// dropped column. -pub const SOURCES: [&str; 9] = - ["timer", "xhci", "net", "sound", "i8042", "dmafault", "hda", "tlb", "nmi"]; +pub const SOURCES: [&str; 10] = + ["timer", "xhci", "net", "sound", "i8042", "dmafault", "hda", "tlb", "nmi", "spurious"]; /// The sources whose delivery CPU is chosen by the interrupt controller rather /// than by the CPU that took the work — every device vector, in other words. diff --git a/tests/test-durations b/tests/test-durations index 7af42a2e6d1..9d6c2ece827 100644 --- a/tests/test-durations +++ b/tests/test-durations @@ -207,6 +207,7 @@ kernel_log_file 13152 keyboard_claim_close_spares_stdin 4580 kill_while_blocked 44 klogd_hosted 17870 +lapic_spurious_vector 18446744073709551615 late_storage_connect 6455 launcher_refusals 2106 locale_detect 5588 diff --git a/tests/toyos.rs b/tests/toyos.rs index 0b8d7852daa..3d78a0ef9e0 100644 --- a/tests/toyos.rs +++ b/tests/toyos.rs @@ -513,6 +513,11 @@ const MACHINE_TESTS: &[(&str, Sched, Tier)] = &[ // One boot, and its verdict is a line the kernel printed before any device // was brought up. No clock and no device in it. ("virtio_used_ring", Sched::Parallel, Tier::Fast), + // One boot whose verdict is three lines of kernel log and a census column. + // The two waits inside the guest are bounded and report rather than hang, so + // no host clock decides anything. Carrying `UNMEASURED_MS` until the shards + // price it. + ("lapic_spurious_vector", Sched::Parallel, Tier::Fast), ("xhci_many_devices", Sched::Parallel, Tier::Fast), // Its whole assertion is that a keystroke injected from the host crossed a // USB keyboard on the *second* controller, and `input_events_run` sends @@ -10620,6 +10625,47 @@ fn run_machine_test( } Ok(()) } + "lapic_spurious_vector" => { + // `apic::enable_x2apic` writes 0xFF into the SVR on every CPU, so + // the platform names a vector the IDT has to gate: delivery through + // a `P = 0` slot is a contributory fault and the CPU escalates to + // `#DF`, which halts the machine. Nothing on this host raises one by + // itself — the SDM's classic condition needs a task-priority + // register this kernel never writes, and every device here is MSI or + // MSI-X — so the kernel raises it on purpose under this parameter. + let qemu = QemuInstance::boot_with_options( + test_config, + c_bins, + rust_bins, + BootOptions { + kernel_params: &["lapic-spurious-selftest"], + ..Default::default() + }, + ); + let log = qemu.boot_log().to_string(); + if let Some(bad) = log.lines().find(|l| l.contains("spurious selftest FAILED")) { + return Err(format!("{bad}\n{log}")); + } + let Some(verdict) = log.lines().find(|l| l.contains("spurious selftest")) else { + return Err(format!("the spurious vector was never raised:\n{log}")); + }; + // `3/3`, not the absence of a FAILED line: a self-test that never + // ran satisfies that absence just as well. + if !verdict.contains("3/3") { + return Err(format!("the self-test did not reach its verdict: {verdict}")); + } + // The two numbers are the interrupt census's own column — the + // handler may not log, so that column is the only report a delivery + // has — and both are asserted: nothing raised this vector before the + // staged one, and exactly one arrived. + if !verdict.contains("(0 -> 1)") { + return Err(format!( + "the census did not count exactly the staged delivery: {verdict}" + )); + } + eprintln!(" [lapic] {}", verdict.trim()); + Ok(()) + } "virtio_used_ring" => { // Both fields of a virtqueue used-ring element are written by the // device, and on virtio-sound's control and event queues the ring From a93767933d33465ab12ac947e247d4ddcf4112ff Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 27 Aug 2026 11:17:40 +0200 Subject: [PATCH 05/14] A BOT command names the region its data phase lands in, not an address and a hope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `bot` took `(data_phys: u64, data_len: u32)` and asserted `data_len <= MSC_DATA_LEN` — 32 KiB — while four of its five call sites pointed at `MSC_SCRATCH`, which is 64 bytes. The bound was in the right place with the wrong operand: nothing related the length the device was told to move to the buffer it was told to move it into. The pair is one `DataPhase = Option>` now. The CBW's `dCBWDataTransferLength` is the region's own `size()`, so a command cannot name a length its destination does not have, and `None` is a command with no data phase rather than a zero somebody has to read as one. `MSC_DATA_LEN` is still asserted and now says the only thing it can: this driver rings at most 32 KiB per transfer. The two scratch users take a 64-byte region and narrow it — `Dma::subview` is what refuses `read_scratch`'s `want` at the buffer, in the type that owns the bound, rather than against a constant declared somewhere else. Negative control, measured, both arms in one session: an INQUIRY staged to ask for a 4,096-byte data phase into the 64-byte scratch. base: `usb_storage_gate` **green**. The transfer was programmed, the stick bound, the volume mounted, and nothing anywhere said a word. fixed: `DMA: 4096 byte(s) at 0x0 run past a region of 0x40, in the region at 0xffff800001612080` — refused at the buffer, before anything reached a ring. Independent oracle, USB Mass Storage Bulk-Only Transport 1.0: §5.1 makes `dCBWDataTransferLength` the number of bytes the *host* expects to transfer, and §6.7.2 licenses the device to send exactly that many before the CSW. So a host that names a length its buffer does not have has authorised the overflow; the device declining to use it is the device's choice, not the driver's bound. That is also why the base arm is green — QEMU's stick answers INQUIRY with 36 bytes and short-packets — and why no test in this tree could have caught it. Green: usb_storage_gate, usb_storage_shapes, usb_transport_break (all nightly tier), usb_short_read, usb_storage_write_error, cargo test --lib. `src/prose-ledger` raises `drivers/xhci/wait/msc.rs` 548 -> 561 deliberately. Closes issues/filesystem/bot-length-assertion-binds-another-buffer.md — the USB storage type-safety audit's F6. No citation of the slug or the path exists elsewhere in the tree, and no `src/redlist.rs` row is sourced to it. --- ...t-length-assertion-binds-another-buffer.md | 17 ------ kernel/src/drivers/xhci/wait/msc.rs | 56 +++++++++++++------ src/prose-ledger | 2 +- 3 files changed, 41 insertions(+), 34 deletions(-) delete mode 100644 issues/filesystem/bot-length-assertion-binds-another-buffer.md diff --git a/issues/filesystem/bot-length-assertion-binds-another-buffer.md b/issues/filesystem/bot-length-assertion-binds-another-buffer.md deleted file mode 100644 index 4e2e5fd0f5a..00000000000 --- a/issues/filesystem/bot-length-assertion-binds-another-buffer.md +++ /dev/null @@ -1,17 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-08-02 ---- - -# `bot`'s length assertion names `MSC_DATA_LEN` and binds a different buffer - -Filed, not fixed. `bot` asserts `data_len as usize <= MSC_DATA_LEN` (32 KiB), -and four of its five call sites point at `MSC_SCRATCH`, whose length is 64. -The assertion permits a 32,768-byte transfer into a 64-byte buffer. Today's -largest is 36 (INQUIRY) so there is no live bug; the next command added is -where it becomes one, and the assertion is what the person adding it will read -to decide the buffer is big enough. Same shape as `IpcPayload`: a bound in the -right place with the wrong operand. The fix is to give `bot` the *region* -rather than a physical address it cannot reason about. The USB storage -type-safety audit's finding F6. diff --git a/kernel/src/drivers/xhci/wait/msc.rs b/kernel/src/drivers/xhci/wait/msc.rs index 25b5f7a1e3b..d7044f53bd3 100644 --- a/kernel/src/drivers/xhci/wait/msc.rs +++ b/kernel/src/drivers/xhci/wait/msc.rs @@ -44,6 +44,17 @@ use super::super::{CC_SUCCESS, CC_STALL, CC_SHORT_PACKET, TRB_NORMAL, OFF_INPUT_ use super::super::{MSC_IN_RING, MSC_OUT_RING, MSC_CBW, MSC_CSW, MSC_SCRATCH, MSC_SCRATCH_LEN}; use super::super::{MSC_DATA, MSC_DATA_LEN, MSC_MAX_BLOCKS}; +/// Where a command's data phase lands, or `None` for a command that has none. +/// +/// **A region and not an address, which is the whole of what makes the length +/// checkable.** The CBW tells the device how many bytes to move, and that +/// number is now the region's own size — so no command can name a length its +/// destination does not have. It was a `(u64, u32)` pair, and four of the five +/// call sites pointed at a 64-byte scratch buffer while the bound above them +/// was the 32 KiB data buffer's. +type DataPhase = Option>; + + /// The block size the layer above this one is written in. A device that /// addresses in anything this does not divide by is unimplemented, not /// unsupported-but-approximated — see `bring_up`. @@ -626,7 +637,7 @@ impl XhciController { // LBA 0, block count 0: the whole medium, which is the only thing // a cache flush above a block device can mean. let cdb = [0x35u8, 0, 0, 0, 0, 0, 0, 0, 0, 0]; - let issued = ctrl.scsi(dev, &cdb, 10, 0, 0, false, until); + let issued = ctrl.scsi(dev, &cdb, 10, None, false, until); let outcome = match flush_sense() { Some((key, asc, ascq)) => Scsi::Refused { key, asc, ascq }, None => issued, @@ -695,7 +706,7 @@ impl XhciController { } let dma = self.dma(); - let data_phys = dma.phys() + (dev.block + MSC_DATA) as u64; + let data = dma.subview(dev.block + MSC_DATA, MSC_DATA_LEN); let mut done = 0u32; while done < count { let batch = (count - done).min(MSC_MAX_BLOCKS); @@ -725,7 +736,7 @@ impl XhciController { dma.copy_from(dev.block + MSC_DATA, &src[offset..offset + bytes]); } - match self.scsi(dev, &cdb, 10, data_phys, bytes as u32, !write, until) { + match self.scsi(dev, &cdb, 10, Some(data.subview(0, bytes)), !write, until) { Scsi::Ok { delivered } if delivered as usize == bytes => {} // Short of what was asked, and reported as success. Nothing // above here has a way to say "these blocks arrived and those @@ -799,8 +810,7 @@ impl XhciController { dev: &mut MscDevice, cdb: &[u8], cdb_len: u8, - data_phys: u64, - data_len: u32, + data: DataPhase, data_in: bool, until: Deadline, ) -> Scsi { @@ -821,7 +831,7 @@ impl XhciController { // `/bin/logd` give up a volume on a stick that is answering. return Scsi::Budget; } - match self.bot(dev, cdb, cdb_len, data_phys, data_len, data_in) { + match self.bot(dev, cdb, cdb_len, data, data_in) { Ok(Bot::Done { delivered }) => { if attempt > 1 { log!("usb-storage: {slot} SCSI {opcode:#04x} completed on attempt \ @@ -855,8 +865,8 @@ impl XhciController { /// zeroes fall on the failing side of every such decision. fn request_sense(&mut self, dev: &mut MscDevice) -> (u8, u8, u8) { let dma = self.dma(); - let phys = dma.phys() + (dev.block + MSC_SCRATCH) as u64; - super::super::zero_dma(dma, dev.block + MSC_SCRATCH, MSC_SCRATCH_LEN); + let scratch = dma.subview(dev.block + MSC_SCRATCH, MSC_SCRATCH_LEN); + scratch.zero(); let cdb = [0x03u8, 0, 0, 0, 18, 0]; // Recursion is not possible: a failing REQUEST SENSE goes through // `bot` directly, so it cannot ask for sense data about itself. @@ -864,7 +874,7 @@ impl XhciController { // what makes all three readable. Short of that they are whatever the // zeroing above left, and zero ASC/ASCQ is exactly the value // [`Scsi::unimplemented`] tests for. - match self.bot(dev, &cdb, 6, phys, 18, true) { + match self.bot(dev, &cdb, 6, Some(scratch.subview(0, 18)), true) { Ok(Bot::Done { delivered }) if delivered >= 14 => { let mut resp = [0u8; 18]; dma.copy_to(dev.block + MSC_SCRATCH, &mut resp); @@ -908,13 +918,24 @@ impl XhciController { dev: &mut MscDevice, cdb: &[u8], cdb_len: u8, - data_phys: u64, - data_len: u32, + data: DataPhase, data_in: bool, ) -> Result { // The CDBs are this file's own, so their shape is a kernel invariant. assert!(cdb_len as usize <= cdb.len() && cdb_len <= 16); - assert!(data_len as usize <= MSC_DATA_LEN); + // The length the device is told to move is the region's own, so the + // only bound left to state is this driver's largest transfer. + let (data_phys, data_len) = match data { + Some(region) => { + assert!( + region.size() <= MSC_DATA_LEN, + "usb-storage: a {} B data phase, past the {MSC_DATA_LEN} B this driver rings", + region.size(), + ); + (region.phys(), region.size() as u32) + } + None => (0, 0), + }; let dma = self.dma(); let tag = dev.next_tag(); @@ -1288,7 +1309,7 @@ fn bring_up(ctrl: &mut XhciController, dev: &mut MscDevice) -> bool { let mut sense = (0u8, 0u8, 0u8); let mut ready = false; loop { - match ctrl.bot(dev, &[0x00u8; 6], 6, 0, 0, false) { + match ctrl.bot(dev, &[0x00u8; 6], 6, None, false) { Ok(Bot::Done { .. }) => { ready = true; break; @@ -1312,7 +1333,7 @@ fn bring_up(ctrl: &mut XhciController, dev: &mut MscDevice) -> bool { } let dma = ctrl.dma(); - let scratch_phys = dma.phys() + (dev.block + MSC_SCRATCH) as u64; + let scratch = dma.subview(dev.block + MSC_SCRATCH, MSC_SCRATCH_LEN); // **No caller's budget here, and that is not an omission.** // [`crate::block::OPERATION`] bounds one *block-device operation*, and a // bring-up is not one: nobody has asked for anything yet, there is no @@ -1328,8 +1349,11 @@ fn bring_up(ctrl: &mut XhciController, dev: &mut MscDevice) -> bool { cdb_len: u8, want: u32, out: &mut [u8]| { - super::super::zero_dma(dma, dev.block + MSC_SCRATCH, MSC_SCRATCH_LEN); - match ctrl.scsi(dev, cdb, cdb_len, scratch_phys, want, true, until) { + scratch.zero(); + // `subview` is what refuses a command asking for more than the scratch + // buffer holds, at the buffer rather than against a constant somewhere + // else. + match ctrl.scsi(dev, cdb, cdb_len, Some(scratch.subview(0, want as usize)), true, until) { Scsi::Ok { delivered } if delivered as usize >= out.len() => { dma.copy_to(dev.block + MSC_SCRATCH, out); true diff --git a/src/prose-ledger b/src/prose-ledger index 475b7481800..14809b543b3 100644 --- a/src/prose-ledger +++ b/src/prose-ledger @@ -89,7 +89,7 @@ kernel/src/drivers/xhci/mod.rs 948 0 kernel/src/drivers/xhci/usbd.rs 53 0 kernel/src/drivers/xhci/wait/boot.rs 232 0 kernel/src/drivers/xhci/wait/mod.rs 219 0 -kernel/src/drivers/xhci/wait/msc.rs 548 0 +kernel/src/drivers/xhci/wait/msc.rs 561 0 kernel/src/elf/cache.rs 73 0 kernel/src/elf/index.rs 50 0 kernel/src/elf/mod.rs 214 0 From cee5063c67c533bc063a8bc1267373c22b370cf2 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 27 Aug 2026 11:25:06 +0200 Subject: [PATCH 06/14] A refused device gives its slot back where it is refused, not where it is pulled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Device Slot is the controller's from the moment Enable Slot answers, and only Disable Slot gives one back. The unplug half was closed: `teardown_port` disables the slot a port carries. The half that stayed open is a device that is refused and *stays plugged in* — a hub, a camera, a fingerprint reader, a disk with no bulk pair, one the pool has no block for, one whose bring-up fails — which kept its slot for the life of the boot, on eleven paths. `device::refuse` is the one exit those paths take now: it marks the port attached with no slot and submits Disable Slot with `AfterSlot::Refused`. Two things it deliberately does not do. The port stays *attached*, which is `let_go`'s answer one stage earlier — a port that read as unattached would enumerate the same refused device again on every debounce. And the pool blocks stay with the port until the unplug, because the port still has a device in it and `teardown_port` is where they are released; only the slot is a resource the controller is short of. Where a class driver is the one refusing, it has to say so, so `msc::bind` and `bind_hid` answer `bool`: SET_CONFIGURATION failing, Configure Endpoint failing for a bulk pair or for a HID interrupt endpoint, a pointer past `PointerSource::claim`'s table, and a stick that never becomes ready are refusals that used to end in the same `finish` a bound device does. Gate and negative control are the same test, `xhci_slot_exhaustion`, which stages a controller clamped to one device block on a six-device bus. It gains one assertion — that every refused device's slot is given back — and that assertion is measured in both directions in one session: base: `FAIL xhci_slot_exhaustion: 0 slot(s) disabled for 5 refused device(s)` — the test that describes the leak was its largest producer. fixed: green, 5 for 5. Independent oracle, xHCI 1.2: §4.6.4 makes Disable Slot legal from any slot state, which is what lets a refusal issue it against a device still in its port and is why no path here has to reason about what state the device reached; §4.5.1 and the Enable Slot semantics in §4.6.3 make the slot allocated at the command's completion whatever becomes of the device. Every path between a successful Enable Slot and a bound device was enumerated by the USB storage type-safety audit (F12) rather than by this change, and the count is the same eleven. Green: xhci_slot_exhaustion, xhci_many_devices, xhci_full_speed_device, xhci_hotplug, usb_storage_gate, usb_refused_disk_first, cargo test --lib. `src/prose-ledger` raises four rows deliberately: `xhci/device.rs` 262 -> 279, `xhci/mod.rs` 948 -> 956, `xhci/wait/msc.rs` 561 -> 562, `tests/toyos.rs` 4794 -> 4799. Closes issues/hardware/xhci-slot-never-given-back.md. No citation of the slug or the path exists elsewhere in the tree and no `src/redlist.rs` row is sourced to it. --- issues/hardware/xhci-slot-never-given-back.md | 52 ------------------ kernel/src/drivers/xhci/device.rs | 54 ++++++++++++++----- kernel/src/drivers/xhci/mod.rs | 9 ++++ kernel/src/drivers/xhci/wait/msc.rs | 6 ++- src/prose-ledger | 8 +-- tests/toyos.rs | 11 ++++ 6 files changed, 68 insertions(+), 72 deletions(-) delete mode 100644 issues/hardware/xhci-slot-never-given-back.md diff --git a/issues/hardware/xhci-slot-never-given-back.md b/issues/hardware/xhci-slot-never-given-back.md deleted file mode 100644 index 72a9ea5d73b..00000000000 --- a/issues/hardware/xhci-slot-never-given-back.md +++ /dev/null @@ -1,52 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-08-03 ---- - -# A device still plugged in after a refused enumeration keeps its slot, on eleven paths - -**A slot is now given back when its device is unplugged, and only then.** -`0ed2bc1` added Disable Slot and made `device::configure` carry the slot id out -of *every* path below the successful Enable Slot, including the eleven refusals -below — so the port remembers the slot whether or not a device came of it, and -`teardown_port` disables it. `xhci_hotplug` shows the controller handing the -same slot id straight back to the next device plugged into that controller. - -What that closes is the hotplug half, which was the half that grows: without it -every plug cycle cost a slot and 64 of them exhausted a PCH controller. What it -does not close is a device that is **still plugged in** after a refused -enumeration — a hub, a camera, a fingerprint reader, or any of the eleven paths -— which keeps its slot until it is pulled. That is the rest of this entry, -unchanged, and the count is still 11. - -`init_device` enables a slot for every connected port and issues no Disable -Slot, on any path: not for the devices it walks past (a hub, camera or -fingerprint reader), not when Address Device fails, not when the descriptor -fetch fails, and not when the slot id comes back past the pool's device blocks -(the `layout.device()` `None` branch). Each of those keeps a slot for a device -the driver will never talk to again. Mass storage added three more: a disk -whose interface has no bulk pair, one the pool has no mass-storage block for, -and one that fails `bring_up` — and the boot stick came *off* the list, since -it now binds. - -The fourth is the one with a test behind it: `xhci_slot_exhaustion` leaves five -slots enabled with a zero DCBAA entry every run, which makes the entry's own -test the largest producer of the leak it describes. - -**The count is 11, not four plus three**, enumerated by the USB storage -type-safety audit (finding F12) by reading every path between the -successful Enable Slot and a bound device. Three of them are named nowhere -else: SET_CONFIGURATION failing (`device.rs`), Configure Endpoint failing for -the bulk pair (`msc.rs`) and for the HID interrupt endpoint (`device.rs`), plus -`PointerSource::claim` running out. A fix that adds Disable Slot to the four -named above leaves seven behind, and this entry is what somebody will work -from. - -Harmless where slots outnumber ports, which is every machine in reach: QEMU -reports 64, Intel's PCH controllers 32 or more, and no root hub has that many -ports. It stops being harmless on a controller whose slot count is below its -device count, where a HID on a later port loses its slot to a hub on an earlier -one. `xhci_slot_exhaustion` is what would catch the regression — it proves the -machine survives the shortage and that the one device which fit was enumerated -to completion, not that the right devices win it. diff --git a/kernel/src/drivers/xhci/device.rs b/kernel/src/drivers/xhci/device.rs index 8ba164f2913..5cb2b262013 100644 --- a/kernel/src/drivers/xhci/device.rs +++ b/kernel/src/drivers/xhci/device.rs @@ -395,7 +395,7 @@ pub(super) fn slot_answered( let Some(block) = ctrl.layout.device(slot_id) else { log!("xHCI: slot {} is beyond the pool's {} device blocks, dropping port {}", slot_id, ctrl.layout.dev_blocks, port_idx + 1); - return finish(ctrl, port_idx, Some(slot_id)); + return refuse(ctrl, port_idx, slot_id); }; log!("xHCI: slot {} enabled (dma +{:#x})", slot_id, block); @@ -423,7 +423,7 @@ pub(super) fn stepped(ctrl: &mut XhciController, mut state: Enumerating, outcome Act::Command(cmd) => { if !outcome.succeeded() { log!("xHCI: {} on port {port}: {}", command_name(cmd), Answer(outcome)); - return finish(ctrl, state.port_idx, Some(state.slot_id)); + return refuse(ctrl, state.port_idx, state.slot_id); } match cmd { enumerate::Command::AddressDevice => log!("xHCI: device addressed"), @@ -450,11 +450,11 @@ pub(super) fn stepped(ctrl: &mut XhciController, mut state: Enumerating, outcome }; let Some(delivered) = delivered(outcome, want) else { log!("xHCI: {} on port {port}: {}", request_name(request), Answer(outcome)); - return finish(ctrl, state.port_idx, Some(state.slot_id)); + return refuse(ctrl, state.port_idx, state.slot_id); }; match read_back(ctrl, &mut state, request, delivered) { Ok(learnt) => learnt, - Err(()) => return finish(ctrl, state.port_idx, Some(state.slot_id)), + Err(()) => return refuse(ctrl, state.port_idx, state.slot_id), } } }; @@ -469,7 +469,7 @@ fn advance(ctrl: &mut XhciController, state: Enumerating, learnt: Learnt) { Next::Refuse => { log!("xHCI: no HID boot interface found on port {}, skipping it", state.port_idx + 1); - finish(ctrl, state.port_idx, Some(state.slot_id)) + refuse(ctrl, state.port_idx, state.slot_id) } } } @@ -483,7 +483,7 @@ fn perform(ctrl: &mut XhciController, mut state: Enumerating, act: Act) { Act::Request(request) => Some(control(ctrl, &mut state, request)), }; let Some((on, stages)) = submitted else { - return finish(ctrl, state.port_idx, Some(state.slot_id)); + return refuse(ctrl, state.port_idx, state.slot_id); }; ctrl.outstanding.submit(What::Enumerating(state), on, stages, deadline()); } @@ -783,27 +783,33 @@ fn hid_input_context( fn bind(ctrl: &mut XhciController, state: Enumerating) { let (_, function) = state.parsed.expect("a configuration named a function"); let rings = state.rings.expect("Configure Endpoint named this device's rings"); - match (function, rings) { + // Whether a device came of it, because that is what decides who keeps the + // slot: a class driver that refused this device leaves the controller + // holding a slot for something nothing will ever talk to. + let bound = match (function, rings) { (Function::Msc(info), Rings::Msc(msc)) => { - super::msc::bind(ctrl, state.ep0_ring, state.slot_id, state.block, msc, &info); - } - (Function::Hid(info), Rings::Hid(int_ring)) => { - bind_hid(ctrl, &state, &info, int_ring); + super::msc::bind(ctrl, state.ep0_ring, state.slot_id, state.block, msc, &info) } + (Function::Hid(info), Rings::Hid(int_ring)) => bind_hid(ctrl, &state, &info, int_ring), // The rings are built from the function two acts earlier and nothing // between the two can change it, so a mismatch is a driver that lost // track of which device it is enumerating. _ => unreachable!("the rings were built for another function"), + }; + if bound { + finish(ctrl, state.port_idx, Some(state.slot_id)); + } else { + refuse(ctrl, state.port_idx, state.slot_id); } - finish(ctrl, state.port_idx, Some(state.slot_id)); } +/// `true` if a device came of it — the caller gives the slot back if not. fn bind_hid( ctrl: &mut XhciController, state: &Enumerating, info: &HidInterfaceInfo, int_ring: TrbRing, -) { +) -> bool { let report = ctrl.dma().subview(state.block + DEV_REPORT, 8); let report_size = match info.protocol { HidType::Keyboard => 8, @@ -820,7 +826,7 @@ fn bind_hid( None => { log!("xHCI: slot {} is past the pointers this machine can number, dropping it", state.slot_id); - return; + return false; } }, }; @@ -855,6 +861,7 @@ fn bind_hid( log!("xHCI: pointer on slot {} merges as source {}", state.slot_id, source.id()); } ctrl.devices.push(dev); + true } /// The enumeration is over, however it went. @@ -873,6 +880,25 @@ pub(super) fn finish(ctrl: &mut XhciController, port_idx: u8, slot: Option) ctrl.acknowledge_port_read(port_idx); } +/// The enumeration is over and the device is refused, with the device still in +/// its port. +/// +/// **The slot goes back here rather than at the unplug.** A slot is the +/// controller's resource from the moment Enable Slot answers, and a device that +/// is refused and stays plugged in — a hub, a camera, a fingerprint reader, a +/// disk with no bulk pair — kept one for the life of the boot. On a controller +/// with fewer slots than the machine has devices, that is a later device losing +/// its slot to an earlier one nothing will ever talk to. +/// +/// The port is left *attached with no slot*, which is `let_go`'s answer one +/// stage earlier: a port that read as unattached would enumerate the same +/// refused device again every debounce. +pub(super) fn refuse(ctrl: &mut XhciController, port_idx: u8, slot_id: u8) { + ctrl.ports[port_idx as usize].enumerated(None); + ctrl.acknowledge_port_read(port_idx); + ctrl.submit_disable_slot(slot_id, super::AfterSlot::Refused); +} + /// Drop an enumeration outstanding for a port whose device has gone, for the /// reason a recovery is dropped: the device it is talking to will not answer, /// and the teardown behind it would spend a deadline per remaining act finding diff --git a/kernel/src/drivers/xhci/mod.rs b/kernel/src/drivers/xhci/mod.rs index 97756c91706..b499d18d78d 100644 --- a/kernel/src/drivers/xhci/mod.rs +++ b/kernel/src/drivers/xhci/mod.rs @@ -248,6 +248,12 @@ enum AfterSlot { /// A device this driver gave up on while it is still in its port, so the /// port stays marked attached — see [`XhciController::let_go`]. LetGo, + /// An enumeration that ended in a refusal, with the device still plugged + /// in. Same shape as [`LetGo`](Self::LetGo) one stage earlier: the slot is + /// the controller's again at once rather than at the unplug, and the port + /// stays attached so the driver does not re-enumerate the device it just + /// refused on every debounce. + Refused, } /// The earlier of two instants something wants to be looked at again. @@ -1822,6 +1828,9 @@ impl XhciController { // of the boot — and two of those is a machine with no disks at all, // boot stick included. A controller that will not disable a slot is // already past what this driver can repair. + // A refusal keeps its pool blocks until the device is pulled, which is + // when `teardown_port` releases them: the port is still attached, so + // the blocks have an owner and the slot does not. if let AfterSlot::Teardown(port_idx) = then { self.release_blocks(port_idx); self.ports[port_idx as usize].torn_down(); diff --git a/kernel/src/drivers/xhci/wait/msc.rs b/kernel/src/drivers/xhci/wait/msc.rs index d7044f53bd3..a2eefacbcff 100644 --- a/kernel/src/drivers/xhci/wait/msc.rs +++ b/kernel/src/drivers/xhci/wait/msc.rs @@ -1250,6 +1250,7 @@ pub(in crate::drivers::xhci) fn prepare( /// No return value: every failure path below logs, so a `bool` would carry /// nothing the one caller wants — and it would be dropped in statement /// position, silently, because Rust does not warn about a discarded `bool`. +/// `true` if a disk came of it — the caller gives the slot back if not. pub(in crate::drivers::xhci) fn bind( ctrl: &mut XhciController, ep0_ring: TrbRing, @@ -1257,7 +1258,7 @@ pub(in crate::drivers::xhci) fn bind( dev_block: usize, rings: MscRings, info: &MscInterface, -) { +) -> bool { let MscRings { at, block, in_ring, out_ring } = rings; let mut dev = MscDevice { slot_id, @@ -1280,7 +1281,7 @@ pub(in crate::drivers::xhci) fn bind( }; if !bring_up(ctrl, &mut dev) { - return; + return false; } // The machine-wide number, taken here because here is where there is a disk // to give one to: it is what `usb_storage::open` indexes by and what a mount @@ -1296,6 +1297,7 @@ pub(in crate::drivers::xhci) fn bind( block ); ctrl.msc[at].disk = Some(Disk { index, dev }); + true } /// TEST UNIT READY, INQUIRY and READ CAPACITY: everything between a configured diff --git a/src/prose-ledger b/src/prose-ledger index 14809b543b3..51e257a3381 100644 --- a/src/prose-ledger +++ b/src/prose-ledger @@ -82,14 +82,14 @@ kernel/src/drivers/virtio_console.rs 94 0 kernel/src/drivers/virtio_gpu.rs 70 0 kernel/src/drivers/virtio_net.rs 100 1 kernel/src/drivers/virtio_sound.rs 181 1 -kernel/src/drivers/xhci/device.rs 262 0 +kernel/src/drivers/xhci/device.rs 279 0 kernel/src/drivers/xhci/hid.rs 103 0 kernel/src/drivers/xhci/legacy.rs 99 0 -kernel/src/drivers/xhci/mod.rs 948 0 +kernel/src/drivers/xhci/mod.rs 956 0 kernel/src/drivers/xhci/usbd.rs 53 0 kernel/src/drivers/xhci/wait/boot.rs 232 0 kernel/src/drivers/xhci/wait/mod.rs 219 0 -kernel/src/drivers/xhci/wait/msc.rs 561 0 +kernel/src/drivers/xhci/wait/msc.rs 562 0 kernel/src/elf/cache.rs 73 0 kernel/src/elf/index.rs 50 0 kernel/src/elf/mod.rs 214 0 @@ -347,7 +347,7 @@ tests/toyos-rust-tests/tls-dlopen-lib/src/lib.rs 10 0 tests/toyos-rust-tests/tls-lib/src/lib.rs 0 0 tests/toyos-rust-tests/tls-multi-crate/dep/src/lib.rs 3 0 tests/toyos-rust-tests/tls-multi-crate/src/lib.rs 10 0 -tests/toyos.rs 4794 23 +tests/toyos.rs 4799 23 toyos-abi/src/audio.rs 21 0 toyos-abi/src/boot.rs 90 0 toyos-abi/src/handle.rs 107 0 diff --git a/tests/toyos.rs b/tests/toyos.rs index 3d78a0ef9e0..68c2e145c08 100644 --- a/tests/toyos.rs +++ b/tests/toyos.rs @@ -9999,6 +9999,17 @@ fn run_machine_test( if slots != [1] { return Err(format!("slots {slots:?} got a block, want just slot 1:\n{log}")); } + // And every one of them gave its slot straight back. A slot is the + // controller's from the moment Enable Slot answers, so a device + // refused and left plugged in used to keep one for the life of the + // boot — which is this test's own bus five times over, on a + // controller the shortage is staged on. + let given_back = log.matches("disabled").count(); + if given_back != over { + return Err(format!( + "{given_back} slot(s) disabled for {over} refused device(s):\n{log}" + )); + } // The one device that did get the block was enumerated to // completion, which is what makes "the extra devices and nothing From fe5fb1cd02c5052358638038f5b08364e07c2b0e Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 27 Aug 2026 12:56:31 +0200 Subject: [PATCH 07/14] A stalled control transfer leaves EP0 running, on both the paths that stall one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There was no `restart_endpoint` for a control endpoint, so a stall halted EP0 for good. Two places reach that state and both are now answered. **The enumeration.** QEMU's `usb-wacom-tablet` stalls SET_PROTOCOL on every boot of the full-speed bus, and the driver binds it anyway — that tolerance is deliberate and stays. What did not stay is going on with EP0 halted: `toyos_xhci::enumerate` learns `Stalled` from the act and answers with two commands of its own, Reset Endpoint and Set TR Dequeue Pointer on DCI 1. Both, because Reset Endpoint alone leaves the controller's dequeue pointer on the TRB that stalled and the next control transfer re-runs it. No packet goes out: the device clears its own half on the next SETUP. **Bulk-Only Reset Recovery.** If the class request itself stalls, EP0 is halted and the two CLEAR_FEATUREs behind it are control transfers that can no longer run — so a disk that broke once could never be recovered. `control_transfer` now recovers EP0 before it reports a stall, which is the only place that knows one happened, and every blocking caller inherits it. `run_recovery` is the loop `quiesce_endpoint` had, lifted so EP0 reaches the same `Recovery` sequence as the bulk and interrupt endpoints; `restart_control_endpoint` is the entry point that drops the device half. `EP0_DCI` is declared once. Every device's EP0 ring is at `DEV_EP0_RING` inside its own device block, which is what lets a caller supply the block and nothing else. Gate and negative control are the same test, `xhci_full_speed_device`, which boots the bus the stall really happens on. It gains two assertions — that exactly one SET_PROTOCOL stalled, and that EP0 ran again after it — measured in both directions in one session: base: `FAIL xhci_full_speed_device: EP0 was left halted behind the stall` fixed: green, and the driver says `EP0 on port 6 runs again after the stall`. The sequence half is gated where it is decided: two host tests in `toyos-xhci/src/enumerate.rs` assert that a stalled SET_PROTOCOL produces exactly `ResetEp0`, `SetEp0Dequeue`, `ConfigureEndpoint` in that order, and that a device which answered pays for neither. 41 pass in that crate. Independent oracle, two specifications rather than this tree's reasoning: USB 2.0 §9.4.5 does not define the Halt feature for the default control pipe and §8.5.3.4 has the device clear the condition on the next SETUP — which is why there is no CLEAR_FEATURE here and why sending one would be a request over the endpoint that is halted. xHCI 1.2 §4.6.8 puts a stalled endpoint in Halted and names Reset Endpoint as what leaves it, and §4.6.10 is why Set TR Dequeue Pointer has to follow. Green: xhci_full_speed_device, xhci_slot_exhaustion, xhci_hid_break, usb_short_read, usb_transport_break, usb_storage_gate, `cargo test` in toyos-xhci, cargo test --lib. `src/prose-ledger` raises five rows deliberately: `xhci/device.rs` 279 -> 289, `xhci/mod.rs` 956 -> 959, `xhci/wait/mod.rs` 219 -> 243, `tests/common/usb.rs` 948 -> 954 (its dated column is untouched at 4), `toyos-xhci/src/enumerate.rs` 106 -> 126. Closes issues/filesystem/control-stall-halts-ep0.md. No citation of the slug or the path exists elsewhere in the tree, and no `src/redlist.rs` row is sourced to it. --- issues/filesystem/control-stall-halts-ep0.md | 21 ---- kernel/src/drivers/xhci/device.rs | 40 +++++++- kernel/src/drivers/xhci/mod.rs | 5 + kernel/src/drivers/xhci/wait/mod.rs | 100 ++++++++++++++++--- kernel/src/drivers/xhci/wait/msc.rs | 8 +- src/prose-ledger | 10 +- tests/common/usb.rs | 19 +++- toyos-xhci/src/enumerate.rs | 58 ++++++++++- 8 files changed, 215 insertions(+), 46 deletions(-) delete mode 100644 issues/filesystem/control-stall-halts-ep0.md diff --git a/issues/filesystem/control-stall-halts-ep0.md b/issues/filesystem/control-stall-halts-ep0.md deleted file mode 100644 index cedee8fcc57..00000000000 --- a/issues/filesystem/control-stall-halts-ep0.md +++ /dev/null @@ -1,21 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-08-02 ---- - -# A control transfer that stalls during enumeration leaves EP0 halted for good - -Filed, not fixed, and visible on any boot of `Profile::MetalFullSpeed`: -QEMU's `usb-wacom-tablet` stalls SET_PROTOCOL, and the driver logs -`xHCI: SET_PROTOCOL on port 6: status stage completion code 6 (Stall Error)` and -carries on. -A stall halts EP0, and nothing clears it — there is no `restart_endpoint` for a -control endpoint. Harmless today because enumeration issues no further control -transfer to that device and the interrupt endpoint is configured afterwards -regardless, so the tablet binds and delivers. It stops being harmless the moment -anything wants to talk to a bound HID over EP0, which is what the mass-storage -path already does on its recovery path. - -The same hole one level up: if `reset_recovery`'s Bulk-Only Reset request itself -stalls, EP0 is halted and only the *bulk* endpoints are restarted. diff --git a/kernel/src/drivers/xhci/device.rs b/kernel/src/drivers/xhci/device.rs index 5cb2b262013..028bd08d5e3 100644 --- a/kernel/src/drivers/xhci/device.rs +++ b/kernel/src/drivers/xhci/device.rs @@ -6,9 +6,10 @@ use toyos_xhci::enumerate::{ }; use toyos_xhci::job::{Await, Outcome, Stages}; use toyos_xhci::port::{self, Reset}; +use toyos_xhci::recovery; use super::{deadline, Answer, Trb, TrbRing, What, XhciController, PAGE}; use super::{OFF_INPUT_CTX, OFF_DATA_BUF}; -use super::{DEV_INT_RING, DEV_EP0_RING, DEV_OUT_CTX, DEV_REPORT}; +use super::{DEV_INT_RING, DEV_EP0_RING, DEV_OUT_CTX, DEV_REPORT, EP0_DCI}; use super::{TRB_ENABLE_SLOT, TRB_ADDRESS_DEVICE, TRB_CONFIGURE_EP, TRB_EVALUATE_CONTEXT}; use super::{enqueue_control, CC_SUCCESS}; @@ -428,7 +429,12 @@ pub(super) fn stepped(ctrl: &mut XhciController, mut state: Enumerating, outcome match cmd { enumerate::Command::AddressDevice => log!("xHCI: device addressed"), enumerate::Command::ConfigureEndpoint => log!("xHCI: endpoint configured"), - enumerate::Command::EnableSlot | enumerate::Command::EvaluateEp0 => {} + enumerate::Command::SetEp0Dequeue => { + log!("xHCI: EP0 on port {port} runs again after the stall") + } + enumerate::Command::EnableSlot + | enumerate::Command::EvaluateEp0 + | enumerate::Command::ResetEp0 => {} } Learnt::Nothing } @@ -439,8 +445,15 @@ pub(super) fn stepped(ctrl: &mut XhciController, mut state: Enumerating, outcome Act::Request(Request::SetProtocol) => { if !outcome.succeeded() { log!("xHCI: SET_PROTOCOL on port {port}: {}", Answer(outcome)); + // Going on is the decision; leaving EP0 halted behind it is + // not. The sequence answers with the controller's two + // recovery commands, which is the whole of what a stalled + // control endpoint owes — the device clears its own half on + // the next SETUP (USB 2.0 §8.5.3.4). + Learnt::Stalled + } else { + Learnt::Nothing } - Learnt::Nothing } Act::Request(request) => { let want = match request { @@ -504,6 +517,25 @@ fn command( Some(evaluate_ep0_trb(ctrl, state.slot_id, state.packet)) } enumerate::Command::ConfigureEndpoint => configure_endpoint_trb(ctrl, state), + // EP0's own recovery, which the sequence owes after an act the device + // stalled and it went on from. `recovery_trb` is the same builder the + // bulk and interrupt endpoints recover through, and its Set TR Dequeue + // arm is what re-initialises the ring — the controller is otherwise + // still pointing at the TRB that stalled. + enumerate::Command::ResetEp0 => Some(ctrl.recovery_trb( + recovery::Command::ResetEndpoint, + state.slot_id, + EP0_DCI, + &mut state.ep0_ring, + state.block + DEV_EP0_RING, + )), + enumerate::Command::SetEp0Dequeue => Some(ctrl.recovery_trb( + recovery::Command::SetDequeue, + state.slot_id, + EP0_DCI, + &mut state.ep0_ring, + state.block + DEV_EP0_RING, + )), } } @@ -659,6 +691,8 @@ fn command_name(cmd: enumerate::Command) -> &'static str { enumerate::Command::AddressDevice => "Address Device", enumerate::Command::EvaluateEp0 => "Evaluate Context (EP0 packet size)", enumerate::Command::ConfigureEndpoint => "Configure Endpoint", + enumerate::Command::ResetEp0 => "Reset Endpoint (EP0)", + enumerate::Command::SetEp0Dequeue => "Set TR Dequeue Pointer (EP0)", } } diff --git a/kernel/src/drivers/xhci/mod.rs b/kernel/src/drivers/xhci/mod.rs index b499d18d78d..80fae0d396c 100644 --- a/kernel/src/drivers/xhci/mod.rs +++ b/kernel/src/drivers/xhci/mod.rs @@ -730,6 +730,11 @@ const SHARED_SIZE: usize = 6 * PAGE; // needs to talk to a device *after* boot — Clear-Feature(HALT) and Bulk-Only // Reset are control transfers on the recovery path — so the ring has to belong // to the device rather than to the enumeration. +/// The Device Context Index of the default control pipe, which is 1 for every +/// device: DCI is `2 * endpoint + direction` and EP0 is bidirectional, so it +/// has one context rather than a pair (xHCI 1.2 §4.5.1). +const EP0_DCI: u8 = 1; + const DEV_INT_RING: usize = 0; // 256 TRBs, exactly one page const DEV_EP0_RING: usize = PAGE; // likewise const DEV_OUT_CTX: usize = 2 * PAGE; // 32 contexts, 2 KiB at ctx_size 64 diff --git a/kernel/src/drivers/xhci/wait/mod.rs b/kernel/src/drivers/xhci/wait/mod.rs index 876782803f8..9ddd97ee15f 100644 --- a/kernel/src/drivers/xhci/wait/mod.rs +++ b/kernel/src/drivers/xhci/wait/mod.rs @@ -257,30 +257,46 @@ impl XhciController { /// [`Self::step_recovery`] is the same route stepped across passes. fn restart_endpoint(&mut self, mut ep: Restart<'_>) -> bool { let owed = self.quiesce_endpoint(&mut ep); - self.clear_endpoint_halt(ep.slot_id, ep.ep0_ring, owed) + self.clear_endpoint_halt(ep.slot_id, ep.ctx_block, ep.ep0_ring, owed) } /// The half of one endpoint's recovery the **controller** answers: every /// command [`Recovery`] owes, up to the point where the sequence would /// speak to the device. fn quiesce_endpoint(&mut self, ep: &mut Restart<'_>) -> Owed { - let slot = self.slot(ep.slot_id); - let state = self.endpoint_state(ep.ctx_block, ep.dci); - log!("xHCI: {slot} endpoint {} is {state}, recovering", ep.dci); + self.run_recovery(ep.slot_id, ep.dci, ep.ctx_block, ep.ring, ep.ring_at, ep.ep_addr) + } + + /// [`Recovery`] run to whatever it owes the device, one blocking command at + /// a time. Every endpoint reaches this, EP0 included — what differs is who + /// answers the [`Owed`] it ends with. + #[allow(clippy::too_many_arguments)] + fn run_recovery( + &mut self, + slot_id: u8, + dci: u8, + ctx_block: usize, + ring: &mut TrbRing, + ring_at: usize, + ep_addr: u8, + ) -> Owed { + let slot = self.slot(slot_id); + let state = self.endpoint_state(ctx_block, dci); + log!("xHCI: {slot} endpoint {dci} is {state}, recovering"); let (mut seq, mut act) = match Recovery::begin(state) { Ok(begun) => begun, Err(NeedsConfigure(state)) => { - log_unrecoverable(slot, ep.dci, state); + log_unrecoverable(slot, dci, state); return Owed::Failed; } }; loop { let cmd = match act { Act::Running => return Owed::Nothing, - Act::ClearHalt => return Owed::ClearHalt { ep_addr: ep.ep_addr }, + Act::ClearHalt => return Owed::ClearHalt { ep_addr }, Act::Command(cmd) => cmd, }; - let trb = self.recovery_trb(cmd, ep.slot_id, ep.dci, ep.ring, ep.ring_at); + let trb = self.recovery_trb(cmd, slot_id, dci, ring, ring_at); if !self.run_command(trb, cmd.name()) { return Owed::Failed; } @@ -288,16 +304,52 @@ impl XhciController { } } + /// The default control pipe, back to a state that runs TRBs. + /// + /// **Its own entry point because the device half does not exist.** USB 2.0 + /// §9.4.5 does not define the Halt feature for the default pipe and §8.5.3.4 + /// has the device clear the condition itself on the next SETUP — and a + /// CLEAR_FEATURE asking for it would have to go out over the very endpoint + /// that is halted. What is left is the controller's half, which is Reset + /// Endpoint and Set TR Dequeue Pointer (xHCI 1.2 §4.6.8): without the + /// second, the controller's dequeue pointer is still on the TRB that + /// stalled and the next transfer re-runs it. + /// + /// Every device's EP0 ring is at `DEV_EP0_RING` inside its own device + /// block, which is what makes the block the whole of what a caller supplies. + fn restart_control_endpoint( + &mut self, + slot_id: u8, + ctx_block: usize, + ring: &mut TrbRing, + ) -> bool { + let owed = self.run_recovery( + slot_id, + super::EP0_DCI, + ctx_block, + ring, + ctx_block + super::DEV_EP0_RING, + 0, + ); + !matches!(owed, Owed::Failed) + } + /// The half the **device** answers, which is the only packet a recovery /// puts on the bus. - fn clear_endpoint_halt(&mut self, slot_id: u8, ep0_ring: &mut TrbRing, owed: Owed) -> bool { + fn clear_endpoint_halt( + &mut self, + slot_id: u8, + ctx_block: usize, + ep0_ring: &mut TrbRing, + owed: Owed, + ) -> bool { let ep_addr = match owed { Owed::Nothing => return true, Owed::Failed => return false, Owed::ClearHalt { ep_addr } => ep_addr, }; - let cleared = - self.control_transfer(slot_id, ep0_ring, 0x02, 0x01, 0, ep_addr as u16, None, 0); + let cleared = self + .control_transfer(slot_id, ctx_block, ep0_ring, 0x02, 0x01, 0, ep_addr as u16, None, 0); if !cleared.done() { log!("xHCI: {} would not clear the halt on endpoint {ep_addr:#04x}: {cleared}", self.slot(slot_id)); @@ -450,6 +502,7 @@ impl XhciController { fn control_transfer( &mut self, slot: u8, + ctx_block: usize, ring: &mut TrbRing, bm_request_type: u8, b_request: u8, @@ -475,15 +528,38 @@ impl XhciController { // The status stage is deliberately not waited for. An errored // data stage halts EP0, so the TRB behind it never runs, and // waiting would spend the whole transfer budget learning that. - Ok((code, _)) => return Control::Failed { stage: "data", code }, + Ok((code, _)) => { + self.recover_after(slot, ctx_block, ring, code); + return Control::Failed { stage: "data", code }; + } Err(why) => return Control::Silent { stage: "data", why }, } } match self.wait_transfer(slot, 1, trbs.status) { Ok((CC_SUCCESS, _)) => Control::Done { delivered }, - Ok((code, _)) => Control::Failed { stage: "status", code }, + Ok((code, _)) => { + self.recover_after(slot, ctx_block, ring, code); + Control::Failed { stage: "status", code } + } Err(why) => Control::Silent { stage: "status", why }, } } + /// Take EP0 back out of Halted where `code` says the device stalled the + /// transfer, before the failure is reported to a caller that will very + /// likely send another one. + /// + /// **Here rather than at each caller**, because this is the only place that + /// knows a control transfer stalled — and a stall the caller answers with + /// another control transfer, which is what Bulk-Only Reset Recovery does + /// twice, is a transfer whose TRBs the controller never runs. + fn recover_after(&mut self, slot: u8, ctx_block: usize, ring: &mut TrbRing, code: u32) { + if code != super::CC_STALL { + return; + } + if !self.restart_control_endpoint(slot, ctx_block, ring) { + log!("xHCI: {} EP0 stayed halted after the stall", self.slot(slot)); + } + } + } diff --git a/kernel/src/drivers/xhci/wait/msc.rs b/kernel/src/drivers/xhci/wait/msc.rs index a2eefacbcff..862ee7b0fd8 100644 --- a/kernel/src/drivers/xhci/wait/msc.rs +++ b/kernel/src/drivers/xhci/wait/msc.rs @@ -1148,7 +1148,9 @@ impl XhciController { fn reset_the_device(&mut self, dev: &mut MscDevice, in_ep: Owed, out_ep: Owed) -> bool { let slot = dev.slot_id; let iface = dev.iface as u16; - let reset = self.control_transfer(slot, &mut dev.ep0_ring, 0x21, 0xFF, 0, iface, None, 0); + let block = dev.dev_block; + let reset = + self.control_transfer(slot, block, &mut dev.ep0_ring, 0x21, 0xFF, 0, iface, None, 0); if !reset.done() { log!("usb-storage: slot {slot} would not take a Bulk-Only Reset: {reset}"); } @@ -1156,8 +1158,8 @@ impl XhciController { // endpoints are what the next command touches, and leaving one halted // because another step failed turns a recoverable device into a // permanently offline one. - let cleared_in = self.clear_endpoint_halt(slot, &mut dev.ep0_ring, in_ep); - let cleared_out = self.clear_endpoint_halt(slot, &mut dev.ep0_ring, out_ep); + let cleared_in = self.clear_endpoint_halt(slot, block, &mut dev.ep0_ring, in_ep); + let cleared_out = self.clear_endpoint_halt(slot, block, &mut dev.ep0_ring, out_ep); reset.done() && cleared_in && cleared_out } } diff --git a/src/prose-ledger b/src/prose-ledger index 51e257a3381..1237668f264 100644 --- a/src/prose-ledger +++ b/src/prose-ledger @@ -82,13 +82,13 @@ kernel/src/drivers/virtio_console.rs 94 0 kernel/src/drivers/virtio_gpu.rs 70 0 kernel/src/drivers/virtio_net.rs 100 1 kernel/src/drivers/virtio_sound.rs 181 1 -kernel/src/drivers/xhci/device.rs 279 0 +kernel/src/drivers/xhci/device.rs 289 0 kernel/src/drivers/xhci/hid.rs 103 0 kernel/src/drivers/xhci/legacy.rs 99 0 -kernel/src/drivers/xhci/mod.rs 956 0 +kernel/src/drivers/xhci/mod.rs 959 0 kernel/src/drivers/xhci/usbd.rs 53 0 kernel/src/drivers/xhci/wait/boot.rs 232 0 -kernel/src/drivers/xhci/wait/mod.rs 219 0 +kernel/src/drivers/xhci/wait/mod.rs 243 0 kernel/src/drivers/xhci/wait/msc.rs 562 0 kernel/src/elf/cache.rs 73 0 kernel/src/elf/index.rs 50 0 @@ -225,7 +225,7 @@ tests/common/serial.rs 260 1 tests/common/stats.rs 44 0 tests/common/storage.rs 57 0 tests/common/toybox.rs 74 0 -tests/common/usb.rs 948 4 +tests/common/usb.rs 954 4 tests/common/volumes.rs 556 3 tests/common/wallclock.rs 144 0 tests/toyos-rust-tests/src/bin/abuse_connect_flood.rs 41 0 @@ -552,7 +552,7 @@ toyos-xhci/sim/tests/scenarios.rs 43 0 toyos-xhci/sim/tests/superspeed.rs 31 0 toyos-xhci/sim/tests/teardown.rs 43 0 toyos-xhci/sim/tests/teeth.rs 11 0 -toyos-xhci/src/enumerate.rs 106 0 +toyos-xhci/src/enumerate.rs 126 0 toyos-xhci/src/invariants.rs 26 0 toyos-xhci/src/job.rs 166 1 toyos-xhci/src/lib.rs 6 0 diff --git a/tests/common/usb.rs b/tests/common/usb.rs index b060a31207d..7d7ebf1ad1e 100644 --- a/tests/common/usb.rs +++ b/tests/common/usb.rs @@ -1889,11 +1889,28 @@ pub fn xhci_full_speed_device( if !log.contains("Boot: complete") { return Err(format!("the boot did not finish\n{log}")); } + + // The tablet stalls SET_PROTOCOL: QEMU's `usb-wacom-tablet` reports a boot + // protocol it will not select, and the driver binds it anyway. What it may + // not do is bind it with EP0 still halted, because a halted control + // endpoint runs no TRB — so a device whose interrupt endpoint later needs a + // CLEAR_FEATURE could never be recovered. This is the one bus in the suite + // where a device stalls a request the driver goes on from. + let stalled = log.lines().filter(|l| l.contains("SET_PROTOCOL")).count(); + if stalled != 1 { + return Err(format!( + "{stalled} stalled SET_PROTOCOL(s); this gate needs the tablet's one\n{log}" + )); + } + if !log.contains("runs again after the stall") { + return Err(format!("EP0 was left halted behind the stall\n{log}")); + } serial::Serial::named("boot console", log.as_str()).must_be_clean()?; eprintln!( " [xhci] two full-speed devices enumerated: one EP0 resized to 64 from the reader's \ - own bMaxPacketSize0 and the tablet's 8 left alone, both identities read off the wire" + own bMaxPacketSize0 and the tablet's 8 left alone, both identities read off the wire, \ + and the tablet's stalled SET_PROTOCOL left EP0 running" ); Ok(()) } diff --git a/toyos-xhci/src/enumerate.rs b/toyos-xhci/src/enumerate.rs index 63c57a39510..63d5618c4e0 100644 --- a/toyos-xhci/src/enumerate.rs +++ b/toyos-xhci/src/enumerate.rs @@ -63,6 +63,14 @@ pub enum Command { /// a device that is not yet configured and says far more than is meant. EvaluateEp0, ConfigureEndpoint, + /// Reset Endpoint on DCI 1, which is what takes the default control pipe + /// out of Halted after the device stalled a request the sequence went on + /// from (xHCI 1.2 §4.6.8). + ResetEp0, + /// …and Set TR Dequeue Pointer for the same endpoint, because the + /// controller's dequeue pointer is still on the TRB that stalled: without + /// it the next control transfer re-runs the stalled one (xHCI 1.2 §4.6.10). + SetEp0Dequeue, } /// A control request the enumeration issues on EP0. @@ -112,6 +120,10 @@ pub enum Learnt { Ep0PacketWrong, /// The configuration descriptor named a function this driver can bind. Function(Function), + /// The device stalled the act, and the sequence goes on regardless — which + /// is a decision only [`Request::SetProtocol`] has. The stall left EP0 + /// halted, so what is owed before the next act is EP0's own recovery. + Stalled, } /// Where the sequence goes after the act that has just completed. @@ -144,6 +156,8 @@ enum At { Config, Configuration, Protocol, + Ep0Reset, + Ep0Dequeue, Endpoints, } @@ -189,7 +203,15 @@ impl Enumeration { At::Configuration if self.boot_protocol => { (At::Protocol, Act::Request(Request::SetProtocol)) } - At::Configuration | At::Protocol => { + // A tolerated stall halts EP0 at the controller, and the device + // clears its own half on the next SETUP (USB 2.0 §8.5.3.4). So what + // is owed is the controller's two commands and no packet on the bus + // — which is why this branch is here and not a `Request`. + At::Protocol if learnt == Learnt::Stalled => { + (At::Ep0Reset, Act::Command(Command::ResetEp0)) + } + At::Ep0Reset => (At::Ep0Dequeue, Act::Command(Command::SetEp0Dequeue)), + At::Configuration | At::Protocol | At::Ep0Dequeue => { (At::Endpoints, Act::Command(Command::ConfigureEndpoint)) } At::Endpoints => return Next::Bind, @@ -292,6 +314,40 @@ mod tests { assert_eq!(route.count(Act::Command(Command::EvaluateEp0)), 1); } + /// The one act the sequence goes on from after a stall, and what a stall + /// leaves behind: EP0 halted at the controller, recovered by two commands + /// and no packet on the bus. + #[test] + fn a_stalled_set_protocol_recovers_ep0_before_the_endpoints_are_configured() { + let route = route(|act| match act { + Act::Request(Request::SetProtocol) => Learnt::Stalled, + other => keyboard(other), + }); + assert_eq!(route.end, Next::Bind); + assert_eq!( + route.acts()[6..], + [ + Act::Request(Request::SetProtocol), + Act::Command(Command::ResetEp0), + Act::Command(Command::SetEp0Dequeue), + Act::Command(Command::ConfigureEndpoint), + ] + ); + // Both, and in that order: Reset Endpoint alone leaves the controller's + // dequeue pointer on the TRB that stalled, so the next control transfer + // runs it again. + assert_eq!(route.count(Act::Command(Command::ResetEp0)), 1); + assert_eq!(route.count(Act::Command(Command::SetEp0Dequeue)), 1); + } + + /// …and a device that did not stall pays nothing for it. + #[test] + fn a_device_that_answered_set_protocol_recovers_nothing() { + let route = route(keyboard); + assert_eq!(route.count(Act::Command(Command::ResetEp0)), 0); + assert_eq!(route.count(Act::Command(Command::SetEp0Dequeue)), 0); + } + /// A disk and a tablet have no boot protocol to select, and asking for one /// is a request the device may stall for. #[test] From 03b9e28eee1680cd394bb7ca23bbb7cdc6e7f43e Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 27 Aug 2026 13:07:12 +0200 Subject: [PATCH 08/14] set_resolution replaces every description of the mode it changed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The entry was filed about the panic console, whose cached framebuffer address a driver frees under it, and that half had already moved into `gpu::set_resolution` — it blinds the console across the window and rearms it if the driver refused. What the entry says next is the half that was still open: "the pattern is simply unguarded for anything that caches the address", and two things do. `device::set_framebuffer_info` holds the description the *next* framebuffer claim is answered with — its regions and its geometry — and it was written once, at registration, by a driver that then changed the mode without it. Its own comment says the second consumer out loud: the absolute pointer's per-axis scale "is a function of the screen and has to follow a mode change", and nothing made it. So after a successful resize the next claimant is handed the regions of the buffer that is no longer scanned out, at the geometry before last, and a tablet maps its coordinates onto a screen that is not there. `gpu::set_resolution` calls it now, which is what owning the invalidation means: a caller doing it for itself is a caller that has to know the driver freed something. `gpu::screen` is the one constructor of that description, so registration and a mode change cannot describe the same screen differently — `register_gpu` was the second copy and is now three lines. Negative control, measured, both arms in one session. No configuration the harness boots can resize at all — measured, and it is why nothing here caught this: `md2 probe: the resize answered Err(NotSupported)`, because every guest takes the UEFI GOP path and GOP cannot change mode after boot services exit, while virtio-gpu — the one driver that can — is on no profile in `tests/common/qemu.rs`. So the control stages a GOP that accepts a mode change and asks the registry what the next claimant would be told: base: `the resize answered Ok((800, 600)) and the registry says Some((2048, 2048))` fixed: `the resize answered Ok((800, 600)) and the registry says Some((800, 600))` Independent oracle, an in-tree differential rather than an argument: the two paths that answer "what is on this screen" are the resize's own return value, which the compositor reads (`userland/compositor/src/session.rs`), and the device registry, which every later claim reads. They are answers to one question and the control above is them disagreeing. **No committed gate, and that is a property of the harness rather than a choice**: a successful resize is unreachable from any guest it can boot, which the first measurement above establishes. Filed as issues/diagnostics/no-guest-can-change-the-display-mode.md so the gap is tracked rather than implied. Green: diskless_boot, screen_log_absent, metal_sim_input, cargo test --lib. `src/prose-ledger` raises `kernel/src/gpu.rs` 19 -> 33 deliberately. Closes issues/design-debt/set-resolution-frees-a-live-framebuffer.md. No citation of the slug or the path exists elsewhere in the tree, and no `src/redlist.rs` row is sourced to it. --- ...set-resolution-frees-a-live-framebuffer.md | 16 -------- .../no-guest-can-change-the-display-mode.md | 38 +++++++++++++++++++ kernel/src/gpu.rs | 35 ++++++++++++++++- kernel/src/main.rs | 15 -------- src/prose-ledger | 2 +- 5 files changed, 72 insertions(+), 34 deletions(-) delete mode 100644 issues/design-debt/set-resolution-frees-a-live-framebuffer.md create mode 100644 issues/diagnostics/no-guest-can-change-the-display-mode.md diff --git a/issues/design-debt/set-resolution-frees-a-live-framebuffer.md b/issues/design-debt/set-resolution-frees-a-live-framebuffer.md deleted file mode 100644 index edee435b93e..00000000000 --- a/issues/design-debt/set-resolution-frees-a-live-framebuffer.md +++ /dev/null @@ -1,16 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-07-31 ---- - -# `gpu::set_resolution` frees the old framebuffer while consumers may hold pointers to it - -`kernel/src/gpu.rs:59-76` calls into the driver, and virtio's implementation -allocates a new framebuffer and frees the old one. Today the only consumer -re-reads `GpuInfo` afterwards, so nothing breaks; the pattern is simply -unguarded for anything that caches the address. The panic console is the first -thing that would have cached one, and it handles the window explicitly — -`detach()` before the call, `rearm()` if the driver refused — which is a -per-caller workaround, not a fix. The fix is for `set_resolution` to own the -invalidation. diff --git a/issues/diagnostics/no-guest-can-change-the-display-mode.md b/issues/diagnostics/no-guest-can-change-the-display-mode.md new file mode 100644 index 00000000000..6ad6b7664b2 --- /dev/null +++ b/issues/diagnostics/no-guest-can-change-the-display-mode.md @@ -0,0 +1,38 @@ +--- +status: open +kind: defect +opened: 2026-08-27 +--- + +# `SYS_GPU_SET_RESOLUTION`'s success path runs on no machine the suite boots + +Every guest `tests/common/qemu.rs` describes takes the UEFI GOP display path: +no profile attaches a virtio-gpu device, and `Shape::vga` is `"std"` throughout. +`GopGpu::set_resolution` answers `NotSupported` — GOP cannot change mode after +boot services exit — so the whole of what a successful resize does is unexecuted +by every test in this tree. + +## Measured + +Staged at `register_gpu` on 2026-08-27 and read off the boot log of +`diskless_boot`: + +``` +md2 probe: the resize answered Err(NotSupported) and the registry says Some((2048, 2048)) +``` + +## What that leaves uncovered + +Everything past the driver's refusal: the new framebuffer's allocation and the +old one's release (`virtio_gpu.rs`), the panic console's detach-and-rearm window, +the mode-change update of `device::set_framebuffer_info` and of the pointer's +per-axis scale, and the compositor's own re-read of the returned `GpuInfo`. A +defect in any of those is invisible here and visible on the owner's desktop, +which is the one machine that runs virtio-gpu. + +## What would close it + +A profile that attaches `virtio-gpu-pci`, and a guest that claims the +framebuffer, resizes, and compares what the call returned against what a second +claim is told. It is a new registered name and a new profile, which is why it is +filed rather than done beside the fix that needed it. diff --git a/kernel/src/gpu.rs b/kernel/src/gpu.rs index 9e027124dec..8adcb328deb 100644 --- a/kernel/src/gpu.rs +++ b/kernel/src/gpu.rs @@ -37,7 +37,31 @@ pub trait Gpu: Send { static GPU: Lock>> = Lock::new(None); static INFO: Lock> = Lock::new(None); +/// What the machine outside this module derives from the current mode. +/// +/// One constructor, because the two callers are the two moments it changes — +/// the driver registering and a resolution being set — and a second copy is how +/// one of them ends up describing the mode before last. +pub fn screen(info: &GpuInfo) -> crate::device::Screen { + crate::device::Screen { + // A description carries handles into whichever process reads it, and + // that process does not exist yet: `try_claim` mints them. + info: toyos_abi::FramebufferInfo { + scanout: [toyos_abi::HANDLE_INVALID; 2], + cursor: toyos_abi::HANDLE_INVALID, + width: info.width, + height: info.height, + stride: info.stride, + pixel_format: info.pixel_format, + flags: info.flags, + }, + scanout: info.scanout.clone(), + cursor: info.cursor.clone(), + } +} + pub fn register(gpu: Box, info: GpuInfo) { + crate::device::set_framebuffer_info(screen(&info)); *INFO.lock() = Some(info); *GPU.lock() = Some(gpu); } @@ -79,8 +103,7 @@ pub fn set_resolution(width: u32, height: u32) -> Result } result? }; - let mut info = INFO.lock(); - *info = Some(GpuInfo { + *INFO.lock() = Some(GpuInfo { scanout: new_info.scanout.clone(), cursor: new_info.cursor.clone(), width: new_info.width, @@ -89,6 +112,14 @@ pub fn set_resolution(width: u32, height: u32) -> Result pixel_format: new_info.pixel_format, flags: new_info.flags, }); + // **Every cached description of the old mode is this function's to + // replace**, which is what it means for it to own the invalidation: the + // registry answers the *next* framebuffer claim out of these regions, and + // the absolute pointer's per-axis scale is a function of this geometry. A + // caller doing it for itself is a caller that has to know the driver freed + // something, and the panic console — the one consumer that caches an + // address rather than a region — is blinded above for the same reason. + crate::device::set_framebuffer_info(screen(&new_info)); Ok(new_info) } diff --git a/kernel/src/main.rs b/kernel/src/main.rs index f347029fb3f..ae919b0c5e3 100644 --- a/kernel/src/main.rs +++ b/kernel/src/main.rs @@ -288,21 +288,6 @@ pub unsafe extern "sysv64" fn _start(_kernel_args: &KernelArgs) -> ! { } fn register_gpu(driver: Box, info: gpu::GpuInfo) { - crate::device::set_framebuffer_info(crate::device::Screen { - // A description carries handles into whichever process reads it, and - // that process does not exist yet: `try_claim` mints them. - info: toyos_abi::FramebufferInfo { - scanout: [toyos_abi::HANDLE_INVALID; 2], - cursor: toyos_abi::HANDLE_INVALID, - width: info.width, - height: info.height, - stride: info.stride, - pixel_format: info.pixel_format, - flags: info.flags, - }, - scanout: info.scanout.clone(), - cursor: info.cursor.clone(), - }); gpu::register(driver, info); } diff --git a/src/prose-ledger b/src/prose-ledger index 1237668f264..100c2112c35 100644 --- a/src/prose-ledger +++ b/src/prose-ledger @@ -98,7 +98,7 @@ kernel/src/fat32_adapter.rs 576 0 kernel/src/file_backing.rs 77 0 kernel/src/file_cache.rs 224 0 kernel/src/gpt.rs 104 0 -kernel/src/gpu.rs 19 0 +kernel/src/gpu.rs 33 0 kernel/src/heartbeat.rs 135 0 kernel/src/hw.rs 347 0 kernel/src/id_map.rs 8 0 From b72440bdc4685696e070034e9bd83fd442c43fd4 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 27 Aug 2026 13:13:41 +0200 Subject: [PATCH 09/14] SYS_FUTEX_WAIT answers the timeout the ABI has always documented MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `toyos-abi`'s `futex_wait` says "Returns 0 on wake, 1 on timeout" and the kernel could produce only the first: both arms of `process::futex_wait` returned 0, and under them `scheduler::futex_wait` returned a bare `true` for everything — `completion::wait_until` answers `Ok(())` for a satisfied predicate and for an expired deadline alike. So a caller could not tell a timeout from a wake, and it broke silently, because the honest answer and the wrong one were the same number. The two are told apart by the word itself, which is what the wait was armed on: after `wait_until` returns, the predicate runs once more. A word that no longer holds `expected` is the wake this wait was for — or a writer that got there first, or the frame going away — and a word that still holds it is a wait nothing it was armed for has ended, which leaves the caller's own deadline. `scheduler::futex_wait` answers a named `FutexEnd` rather than the `bool` it had, which claimed "it parked": a question no caller asks, answered `true` whether or not it had. This is the entry's second option, and its own words about it are right — "the word may have changed and changed back". That reads as a wake, which is also what the ABI answers a caller who was woken and found the word back where it was: a `futex_wait` return is never proof of anything but that the caller must look again. It costs one re-evaluation of a predicate the wait has already run at least once, against `completion::wait_until` growing an outcome and eleven call sites having to say what they do with it. The gate is the third question in `futex_wake_counts`, which already owns this syscall pair, so no new name is registered. Three assertions, because any one of them passes on a kernel answering a constant: a timed wait nobody wakes answers 1, a wait whose word never matched answers 0, and a wait that is woken answers 0. Only the first is timed; the woken arm waits forever and is woken by the test itself, so no margin decides a verdict. Negative control, measured, this kernel change reverted under the same test: `FAIL rs::futex_wake_counts: a futex wait nobody woke answered 0, wanted the timeout`. Independent oracle: the ABI's own line, written before this and never met — `toyos-abi/src/syscall.rs`, "Returns 0 on wake, 1 on timeout" — and behind it POSIX's `pthread_cond_timedwait`, whose `ETIMEDOUT` is the caller this exists for and which cannot be built on a primitive that answers one number. Green: futex_wake_counts, abuse_kernel_addr, sched_stress, cargo test --lib. `src/prose-ledger` raises three rows deliberately: `scheduler.rs` 642 -> 659, `process.rs` 920 -> 922, `futex_wake_counts.rs` 152 -> 167. Closes two entries, which are one defect seen twice: issues/kernel/futex-wait-cannot-report-a-timeout.md and issues/kernel/futex-wait-never-returns-its-timeout-code.md. The second says so itself — it is the doc-comment half of the first. No citation of either slug or path exists elsewhere in the tree, and no `src/redlist.rs` row is sourced to either. --- .../futex-wait-cannot-report-a-timeout.md | 43 ------------------- ...tex-wait-never-returns-its-timeout-code.md | 38 ---------------- kernel/src/process.rs | 14 +++--- kernel/src/scheduler.rs | 35 +++++++++++++-- src/prose-ledger | 6 +-- .../src/bin/futex_wake_counts.rs | 34 +++++++++++++++ 6 files changed, 77 insertions(+), 93 deletions(-) delete mode 100644 issues/kernel/futex-wait-cannot-report-a-timeout.md delete mode 100644 issues/kernel/futex-wait-never-returns-its-timeout-code.md diff --git a/issues/kernel/futex-wait-cannot-report-a-timeout.md b/issues/kernel/futex-wait-cannot-report-a-timeout.md deleted file mode 100644 index 257b4d50cb1..00000000000 --- a/issues/kernel/futex-wait-cannot-report-a-timeout.md +++ /dev/null @@ -1,43 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-08-16 ---- - -# `SYS_FUTEX_WAIT` answers 0 for a timeout and 0 for a wake - -`toyos-abi/src/syscall.rs`'s `futex_wait` declares its contract in one line: - -> Block if `*addr == expected`. Returns 0 on wake, 1 on timeout. - -`kernel/src/process.rs`'s `futex_wait` cannot produce the second value. Both of -its arms return `0` — one commented "blocked and woken", the other "value -mismatch, returned immediately" — and neither is the timeout the ABI names. One -layer down, `scheduler::futex_wait` returns a bare `true` unconditionally, so -there is nothing for the caller to distinguish a timeout with either: the -`completion::wait_until` it wraps answers `Ok(())` for a satisfied predicate and -`Ok(())` again for an expired deadline, which is the shape every other timed -caller wants and the one this one cannot use. - -**Pre-existing, and older than the completion cutover.** It is recorded here -because the sibling half of the same syscall pair *was* this branch's to fix — -`futex_wake` returned 0 for every call in the machine, and -`futex_wake_counts` is the gate that now holds it — and the two were found -together. Nothing in this chunk touched the wait's return. - -**What it costs today is small and not zero.** No in-tree caller reads the -value: `userland/libc/src/pthread.rs` discards it, and the std fork's -condvar/rwlock paths re-derive their own predicate after every return, which is -what `scheduler-core-spec.md` invariant 10 requires of them anyway. What it -breaks is any future caller that treats a timed `futex_wait` as answering -whether its own deadline was reached — a `pthread_cond_timedwait` that reports -`ETIMEDOUT`, most obviously — and it breaks it silently, because the honest -answer and the wrong answer are the same number. - -Closing it means one of two things, and the choice belongs with whoever lands -`pthread_cond_timedwait`: - -- `completion::wait_until` reports whether it returned on the predicate or on - the deadline, which is a signature change with eleven call sites; or -- `futex_wait` re-reads the word itself after the wait and answers from that, - which is cheaper and weaker — the word may have changed and changed back. diff --git a/issues/kernel/futex-wait-never-returns-its-timeout-code.md b/issues/kernel/futex-wait-never-returns-its-timeout-code.md deleted file mode 100644 index aa658af51f3..00000000000 --- a/issues/kernel/futex-wait-never-returns-its-timeout-code.md +++ /dev/null @@ -1,38 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-08-19 ---- - -# `process::futex_wait` documents a timeout return it cannot produce - -`kernel/src/process.rs`'s `futex_wait` says: - -> Returns 0 if woken normally, 1 if timed out, an error if `addr` names no word -> this process may have. - -No path returns 1. It calls `scheduler::futex_wait`, which is `-> bool` and -answers `false` only for the value-mismatch case — the timeout and the normal -wake are both `block_on(ticket, deadline)` returning, after which it answers -`true`. So a caller that passes a timeout cannot tell a timeout from a wake, and -the one number that would have told it is documented but never produced. - -Found while clearing default clippy's `if_same_then_else` at that site -(`0 // blocked and woken` and `0 // value mismatch, returned immediately` were -two arms of one `if`). The lint is now clear — the branch is gone and one -comment says both outcomes are the same answer — but that fix was deliberately -neutral, and the mismatch between the doc and the code is what is left. - -Two things could be true, and which one decides the fix: - -- **The doc is stale.** Userland re-checks the futex word after every wake, so a - timeout that reads as a wake costs one extra check and nothing else. Then the - fix is deleting the sentence and the returns stay `0`. -- **The caller needs it.** `SYS_FUTEX_WAIT`'s return reaches userland, and a - `futex_wait` with a timeout that cannot report the timeout is a primitive - nobody can build a bounded wait on. Then `scheduler::futex_wait` has to - distinguish the two, which means `block_on` has to say which of the two woke - it. - -Nothing measured which. The syscall's userland callers are the evidence to -gather first. diff --git a/kernel/src/process.rs b/kernel/src/process.rs index 9b2dc14f916..70e1933331a 100644 --- a/kernel/src/process.rs +++ b/kernel/src/process.rs @@ -1718,11 +1718,15 @@ pub fn futex_wait(addr: UserAddr, expected: u32, timeout_ns: u64) -> u64 { return toyos_abi::syscall::SyscallError::BadAddress.to_u64(); }; - // Both outcomes answer 0: a thread that blocked and was woken and one whose - // word did not match and never blocked are the same answer to the caller, - // which re-checks the word either way. - scheduler::futex_wait(addr, phys_addr, expected, deadline); - 0 + // The ABI's two answers, and the kernel can produce both now: 0 wherever + // the word no longer holds `expected` — woken, or never blocked at all, + // which are one answer to a caller that re-checks the word either way — + // and 1 where it still does, which nothing but the caller's own deadline + // can have ended. + match scheduler::futex_wait(addr, phys_addr, expected, deadline) { + scheduler::FutexEnd::Changed => 0, + scheduler::FutexEnd::Timeout => 1, + } } /// Wake up to `count` threads blocked on the same physical address as `addr`. diff --git a/kernel/src/scheduler.rs b/kernel/src/scheduler.rs index 0a0cbdcbcaa..b7f75f0b7c6 100644 --- a/kernel/src/scheduler.rs +++ b/kernel/src/scheduler.rs @@ -684,7 +684,8 @@ pub fn set_current_rt(enable: bool) { driver::set_current_rt(enable); } -/// Block on a futex word unless it already changed. Returns whether it parked. +/// Block on a futex word unless it already changed, and answer which of the two +/// things the ABI names ended the wait. /// /// Registering before reading the word is the whole protocol: a `futex_wake` /// that runs after the registration either claims the ticket or finds the @@ -702,7 +703,7 @@ pub fn futex_wait( phys_addr: DirectMap, expected: u32, deadline: Deadline, -) -> bool { +) -> FutexEnd { let parkable = Parkable::at_entry(); // The value check is the predicate, and it runs *after* the arm — which is // the same ordering the registration gave it, and why no wake-generation @@ -755,9 +756,35 @@ pub fn futex_wait( completion::Token::new(phys_addr.phys()), WaitClass::Futex, deadline, - read, + &read, ); - true + // **The predicate again, which is the whole of how the two are told + // apart.** `wait_until` answers `Ok(())` for a satisfied predicate and for + // an expired deadline alike, so what separates them is the word itself: it + // is the thing this wait was armed on, and the caller asked to sleep for as + // long as it held `expected`. A word that changed and changed back reads as + // a wake, which is the ABI's answer too — a `futex_wait` return is never + // proof of anything but that the caller must look again. + if read() { + FutexEnd::Changed + } else { + FutexEnd::Timeout + } +} + +/// Which of the two things `SYS_FUTEX_WAIT`'s ABI names ended a wait. +/// +/// A named pair rather than the `bool` this returned, because the `bool` said +/// "it parked" — a question no caller asks and which it answered `true` to +/// whether or not it had. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum FutexEnd { + /// The word no longer holds `expected` — the wake this wait was for, a + /// writer that got there first, or the word's own frame going away. + Changed, + /// The word still holds `expected`, so nothing this wait was armed for has + /// happened and the caller's own deadline is what ended it. + Timeout, } /// Wake up to `count` waiters on this futex word, and answer how many. diff --git a/src/prose-ledger b/src/prose-ledger index 100c2112c35..555b0d2091f 100644 --- a/src/prose-ledger +++ b/src/prose-ledger @@ -156,7 +156,7 @@ kernel/src/page_cache.rs 88 0 kernel/src/panic.rs 177 0 kernel/src/pipe.rs 96 0 kernel/src/preempt.rs 66 0 -kernel/src/process.rs 920 0 +kernel/src/process.rs 922 0 kernel/src/rtc.rs 123 0 kernel/src/sched/driver.rs 716 0 kernel/src/sched/dump.rs 241 0 @@ -166,7 +166,7 @@ kernel/src/sched/payload.rs 124 0 kernel/src/sched/reap_gate.rs 71 1 kernel/src/sched/waitqs.rs 62 0 kernel/src/sched_gate.rs 45 0 -kernel/src/scheduler.rs 642 0 +kernel/src/scheduler.rs 659 0 kernel/src/shootdown.rs 95 0 kernel/src/sleeplock.rs 176 0 kernel/src/symbols.rs 135 0 @@ -275,7 +275,7 @@ tests/toyos-rust-tests/src/bin/fault_gates.rs 27 0 tests/toyos-rust-tests/src/bin/fpu_isolation.rs 101 0 tests/toyos-rust-tests/src/bin/fs_large_file.rs 16 0 tests/toyos-rust-tests/src/bin/fs_truncate_persist.rs 20 0 -tests/toyos-rust-tests/src/bin/futex_wake_counts.rs 152 0 +tests/toyos-rust-tests/src/bin/futex_wake_counts.rs 167 0 tests/toyos-rust-tests/src/bin/handle_basic.rs 121 2 tests/toyos-rust-tests/src/bin/handle_kill_policy.rs 207 4 tests/toyos-rust-tests/src/bin/handle_lifetime.rs 94 1 diff --git a/tests/toyos-rust-tests/src/bin/futex_wake_counts.rs b/tests/toyos-rust-tests/src/bin/futex_wake_counts.rs index 56dfc824b2a..9f53cad6f70 100644 --- a/tests/toyos-rust-tests/src/bin/futex_wake_counts.rs +++ b/tests/toyos-rust-tests/src/bin/futex_wake_counts.rs @@ -101,9 +101,43 @@ fn main() { counts(); claim_semantics(); orphaned_by_unmap(); + timeout_is_its_own_answer(); println!("futex_wake respects its count, names its word, says how many it woke, and ends"); } +/// The wait's own two answers, which the ABI names and the kernel could not +/// produce: `0` on a wake and `1` on a timeout. +/// +/// **Both arms, because either one alone passes on a kernel that answers a +/// constant** — which is what this was: every return was 0, so a +/// `pthread_cond_timedwait` built on it could never report `ETIMEDOUT`, and the +/// honest answer and the wrong one were the same number. +/// +/// The timeout is a real span and the wake is not raced against one: the woken +/// arm waits forever and is woken by this thread after the word changes, so no +/// margin decides anything. What the timed arm asserts is only that a wait +/// nobody wakes ends saying so — a slow host makes it later, never wrong. +fn timeout_is_its_own_answer() { + static TIMED: AtomicU32 = AtomicU32::new(7); + static WOKEN: AtomicU32 = AtomicU32::new(7); + + let timed_out = unsafe { syscall::futex_wait(TIMED.as_ptr(), 7, Some(50_000_000)) }; + assert_eq!(timed_out, 1, "a futex wait nobody woke answered {timed_out}, wanted the timeout"); + + // …and the same call with the word already changed is the other answer, + // which is what rules out a kernel that has started answering 1 to + // everything. + let changed = unsafe { syscall::futex_wait(TIMED.as_ptr(), 9, Some(50_000_000)) }; + assert_eq!(changed, 0, "a futex wait whose word did not match answered {changed}"); + + let waiter = thread::spawn(|| unsafe { syscall::futex_wait(WOKEN.as_ptr(), 7, None) }); + thread::sleep(PARK_MARGIN); + WOKEN.store(8, Ordering::SeqCst); + unsafe { syscall::futex_wake(WOKEN.as_ptr(), 1) }; + let woken = waiter.join().expect("the woken waiter panicked"); + assert_eq!(woken, 0, "a futex wait that was woken answered {woken}, wanted the wake"); +} + /// A count-limited wake names its word and answers how many it woke. fn counts() { let waiters: Vec<_> = (0..2) From 50542cf289220143faba5f6a373d8894e6bf66bf Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 27 Aug 2026 15:17:53 +0200 Subject: [PATCH 10/14] The two-controller collision is staged with a device that keeps its slot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `MetalXhciBoth` exists to put both pointers on the same xHCI slot id of their own controller, because a slot id was once used as a machine-wide name for a device and is not one. It balanced the boot stick on the first controller with a `usb-hub` on the second — and a hub is walked past, so what it contributed was a *leaked* slot. The commit before last gives that slot back at the refusal, so the second controller's devices moved down one and the premise went with them: FAIL xhci_two_controllers: the two pointers are on slots 3 and 2, so a slot-keyed merge would not have collided and this test proves nothing That is the test declining to certify rather than a defect it found, and the staging is what is wrong. A second keyboard on the second controller balances the stick with a device that *binds*, which is the only kind whose slot survives its enumeration. The hub stays, walked past as before — and now also exercising the refusal path that gives a slot back on a bus where something else is enumerated behind it. Measured, alone, three arms in one session: on `16c05999` (this branch's base) both pointers land on slot 3 and the test is green; on the branch with the old device list they land on 3 and 2 and it reds; with the list rebalanced they are on slot 3 again and merge as sources 1 and 2. The machine-wide totals the test asserts move with the device list: 5 HID devices and three keyboards. `src/prose-ledger` raises `tests/common/qemu.rs` 1588 -> 1591 deliberately. --- src/prose-ledger | 2 +- tests/common/qemu.rs | 7 ++++++- tests/toyos.rs | 8 ++++---- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/prose-ledger b/src/prose-ledger index 555b0d2091f..e1669caa683 100644 --- a/src/prose-ledger +++ b/src/prose-ledger @@ -219,7 +219,7 @@ tests/common/lane.rs 29 0 tests/common/logread.rs 143 2 tests/common/mod.rs 0 0 tests/common/passcost.rs 237 4 -tests/common/qemu.rs 1588 11 +tests/common/qemu.rs 1591 11 tests/common/screen.rs 89 0 tests/common/serial.rs 260 1 tests/common/stats.rs 44 0 diff --git a/tests/common/qemu.rs b/tests/common/qemu.rs index b0f8b9fa3b3..989b8330188 100644 --- a/tests/common/qemu.rs +++ b/tests/common/qemu.rs @@ -1168,7 +1168,11 @@ pub enum Profile { /// bus, and it is a claim nothing could test: with one controller, an /// xHCI slot id was a machine-wide name for a device. It is not — the /// device lists here are shaped so both pointers land on the same slot id - /// of their own controller. + /// of their own controller — with a *bound* device, because a refused one + /// gives its slot back the moment it is refused and shifts nothing after + /// it. The hub on the second controller is still there and is still walked + /// past; what balances the boot stick on the first is the second keyboard + /// beside it. MetalXhciBoth, /// The HID controller has no MSI-X, and nothing else can drain its ring. /// @@ -1779,6 +1783,7 @@ impl Profile { "usb-mouse,bus=xhci.0", "usb-hub,bus=xhci1.0", "usb-kbd,bus=xhci1.0", + "usb-kbd,bus=xhci1.0", "usb-mouse,bus=xhci1.0", ], nvme_bytes: NVME_SMALL, diff --git a/tests/toyos.rs b/tests/toyos.rs index 68c2e145c08..c133136cea4 100644 --- a/tests/toyos.rs +++ b/tests/toyos.rs @@ -8614,13 +8614,13 @@ fn run_machine_test( if found != 2 { return Err(format!("{found} controller(s) initialised, want 2:\n{boot}")); } - if !boot.contains("xHCI: 2 controller(s), 4 HID device(s)") { + if !boot.contains("xHCI: 2 controller(s), 5 HID device(s)") { return Err(format!( - "the machine-wide totals are not 2 controllers and 4 HID devices:\n{boot}" + "the machine-wide totals are not 2 controllers and 5 HID devices:\n{boot}" )); } let binds = parse_xhci_binds(&boot); - for (want, count) in [("keyboard", 2), ("mouse", 2)] { + for (want, count) in [("keyboard", 3), ("mouse", 2)] { let got = binds.iter().filter(|b| b.kind == want).count(); if got != count { return Err(format!("{got} {want}(s) bound, want {count}: {binds:?}\n{boot}")); @@ -8652,7 +8652,7 @@ fn run_machine_test( } serial::Serial::named("boot console", boot.as_str()).must_be_clean()?; eprintln!( - " [xhci] 2 controllers, 4 HID; both pointers on slot {}, merging as sources {} \ + " [xhci] 2 controllers, 5 HID; both pointers on slot {}, merging as sources {} \ and {}", pointers[0].0, pointers[0].1, pointers[1].1 ); From 04b7577a24bc8def0433cfcbefdc531b31466300 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 27 Aug 2026 15:19:21 +0200 Subject: [PATCH 11/14] A red at 220x its price is adjudicated, not re-run away MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `syscall_window_nmi` reds in the wide phase of this branch's `cargo test` — at 1,505 s against a committed 6,825 ms, on a host carrying 92 guests and a second worktree's suite — and its isolated re-run in the same session is green in 5 s with the storm reported in full. `--known-red` answered NOT ON THE LIST, so the next agent to meet it would have read it as theirs. The row is `Instrument::DevHostLoaded`, `Finding::Seen`, no rate: one sighting has no denominator, and this tree does not write a number it did not measure. What the entry beside it says is what would settle it. It is not this branch's: the syscall entry's displacements changed *spelling* and are byte-identical machine code, and the two per-CPU stores the panic path's bracket adds are not a 220x wall stretch. The same tip runs the test green alone. --- ...all-window-nmi-reds-under-a-shared-host.md | 51 +++++++++++++++++++ src/redlist.rs | 17 +++++++ 2 files changed, 68 insertions(+) create mode 100644 issues/build/syscall-window-nmi-reds-under-a-shared-host.md diff --git a/issues/build/syscall-window-nmi-reds-under-a-shared-host.md b/issues/build/syscall-window-nmi-reds-under-a-shared-host.md new file mode 100644 index 00000000000..70b16677140 --- /dev/null +++ b/issues/build/syscall-window-nmi-reds-under-a-shared-host.md @@ -0,0 +1,51 @@ +--- +status: open +kind: defect +opened: 2026-08-27 +--- + +# `syscall_window_nmi` reds when the host is somebody else's too + +One sighting, dev host, 2026-08-27, a 288-name `cargo test` run at 92 guests +with a second worktree's suite on the same machine: + +``` +FAIL syscall_window_nmi: the storm never reported — is `syscall-window-nmi` on? + FAIL syscall_window_nmi (1505s) + ALONE syscall_window_nmi: GREEN +``` + +The isolated re-run in the same session took **5 s** and reported the storm in +full — `3000 sent, 3000 taken, 43 in the window, 140 in Ring 3, 663 syscalls +made under the storm`. The committed price is 6,825 ms, so the wide-run reading +is a **220x wall stretch**: the guest was still working and had not finished, +which is what its own message says when the storm line has not arrived yet. + +## Why this is filed rather than re-classified + +`ALONE: GREEN` is the harness naming a *hypothesis* — that the name's +`Sched::Parallel` is wrong — and this file is not that claim. +`tests/CLAUDE.md` is explicit: the harness suggests scheduling, the mechanism +decides, and nothing here has measured a mechanism. What is measured is one +red at 220x its price and one green at 1x. + +`cargo run -- --known-red syscall_window_nmi` answered **NOT ON THE LIST** when +this was opened; `src/redlist.rs` now carries a row sourced here so the next +agent who meets it is told whose it is. + +## What it is not + +Not the branch it was found on. That branch changed the syscall entry's +displacement *spelling* (`const` operands for the same immediates, byte-identical +machine code) and added two per-CPU stores per syscall for the panic path's +`in_syscall` bracket. Neither moves a 6.8 s test to 1,505 s, and the same tip +runs it green alone in 5 s. + +## What would settle it + +A rate. One sighting has no denominator, which is why there is no number in the +row: the same suite run repeatedly on a host with and without a second +worktree's build on it is what turns this into either a contention class the +harness should schedule around or a defect in the storm's own pacing. +`issues/build/parallel-tests-red-under-other-suites.md` is where the family +lives, and this name is not on it. diff --git a/src/redlist.rs b/src/redlist.rs index 7ab72025ab6..642a13ee5c2 100644 --- a/src/redlist.rs +++ b/src/redlist.rs @@ -2228,6 +2228,23 @@ pub const KNOWN_RED: &[Red] = &[ source: "issues/build/parallel-tests-red-under-other-suites.md", measured: "2026-08-20", }, + Red { + test: "syscall_window_nmi", + instrument: Instrument::DevHostLoaded, + finding: Finding::Seen, + standing: Standing::Stands, + what: "`the storm never reported — is `syscall-window-nmi` on?` at 1,505 s against a \ + committed 6,825 ms, in a 288-name run at 92 guests with a second worktree's \ + suite on the same host. A 220x wall stretch, and the guest's own message for \ + a storm line that has not arrived yet. The isolated re-run in the same \ + session was green in 5 s and reported `3000 sent, 3000 taken, 43 in the \ + window`. First sighting, no denominator; `--known-red` answered NOT ON THE \ + LIST. **Not about the diff it was found on**", + evidence: "dev host, 2026-08-27, the `cargo test` run of the md2 defect-fix branch; \ + `exit_wait_storm` reds in the same phase and is already on this list", + source: "issues/build/syscall-window-nmi-reds-under-a-shared-host.md", + measured: "2026-08-27", + }, Red { test: "exit_wait_storm", instrument: Instrument::Ci, From 24c20c273dc2419383dc371e04a8217cd2689853 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 27 Aug 2026 15:32:16 +0200 Subject: [PATCH 12/14] tests/CLAUDE.md: a premise arranged by a defect passes for the wrong reason Placed by the orchestrator with the slot fix that revealed it: MetalXhciBoth balanced its buses with a leaked hub slot, so the fix moved both pointers and the test declined to certify. A staging device must own a resource that survives its own enumeration. --- tests/CLAUDE.md | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/CLAUDE.md b/tests/CLAUDE.md index 746692f2822..015bed62de4 100644 --- a/tests/CLAUDE.md +++ b/tests/CLAUDE.md @@ -26,4 +26,5 @@ The mechanics live where the work is: profiles and shapes in `tests/common/`, re - **A crafted-input test asserts the harm before the return value, and never Debug-prints a refused value** — an unrefused one is as large as the input asked for, so the arm that asserts the error first panics on the value and never reports the damage (a 2 GB assert log and a hidden root-node clobber, both measured 2026-08-23). - **A test that delivers its stimulus through a channel that can silently lose it verifies the stimulus arrived before asserting on the effect** — otherwise the failure message names whatever the test was watching, never the thing that broke. QEMU's PS/2 queue is 16 bytes and drops the seventeenth silently, so typed input paces against the panel's echo, burst by burst (`console_type_line`); both recorded `screen_console_panic` "lost screen" reds were commands that never fully arrived (2026-08-24). A guest's console reaches the host as whole lines only — userland's stdout is line-buffered, so a shell's echo of a partial line exists on no channel; a host pacing an injection needs the panel, or waits for the newline that flushes the line (2026-08-26). - **A harness field that can be silently inert is this suite's worst defect class** — an arm that does nothing, a run that is green, and every negative control staged through it proving nothing (`kernel_params` beside a staged `boot_image` was one until 2026-08-22). Where two options can describe the same guest, they refuse each other by name rather than one quietly winning; an image is asked what it is armed with. +- **A test whose premise is arranged by a defect passes for the wrong reason, and the fix is what reveals it** — `MetalXhciBoth` balanced its two buses with a `usb-hub` whose slot the driver leaked, so giving the slot back moved both pointers and the test declined to certify; a staging device has to be one whose resource survives its own enumeration (2026-08-27). - **Host suites** run with plain `cargo test` inside `toyos-sched/`, `toyos-ps2/`, `toyos-gpt/`, `toyos-elf/`, `toyos-cc/`, `toyos-ld/`, `toyos-hda/`, `toyos-pci/`, `toyos-xhci/`, `toyos-desktop/`, `toyos-keymap/`, `bcachefs/`, `kernel-loom/`, `toyos-userbound/`, `toyos-elide/`, `toyos-fat32/`, `toyos-fat32-check/`, `toyos-abi/`, `toyos-manifest/`, `toyos-wallclock/`, `toyos-mixer/`, `toyos-dma/`; `userland/sshd`, `userland/calc` and `userland/soundd` cross-compile and need the host triple (`cargo test --target "$(rustc -vV | sed -n 's/^host: //p')"`) — any userland crate is host-testable this way despite the `toyos` SDK, which is how soundd's seven tests ran nowhere until 2026-08-15; `calc`'s arithmetic is all decided there, so its 64 tests are the whole of what that program is gated by. `kernel-loom/` and `toyos-sched/loom` are the memory-ordering checks — x86 TSO hides a missing acquire edge from every guest test. `toyos/` is host-testable too — plain `cargo test` inside it, its `lib.rs` being `cfg_attr(not(test), no_std)` — so an SDK decision that is a function of a kernel word has a host gate and needs no guest (the `netd error` mapping ran nowhere until 2026-08-22). From d2e34f4ae8804b97c0775eed157367996c645eb2 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 27 Aug 2026 16:30:13 +0200 Subject: [PATCH 13/14] tests: price lapic_spurious_vector from the hosted run The UNMEASURED marker bought hosted run 33077119849 (guest partition green); it measured lapic_spurious_vector at 6829ms, under the Fast commitment, so the declared tier stands. The rest of the profile is unchanged. --- tests/test-durations | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test-durations b/tests/test-durations index 9d6c2ece827..0f219bbc64c 100644 --- a/tests/test-durations +++ b/tests/test-durations @@ -207,7 +207,7 @@ kernel_log_file 13152 keyboard_claim_close_spares_stdin 4580 kill_while_blocked 44 klogd_hosted 17870 -lapic_spurious_vector 18446744073709551615 +lapic_spurious_vector 6829 late_storage_connect 6455 launcher_refusals 2106 locale_detect 5588 From 9b9129551a6a9e31661f9cda9deaef8f6446afa9 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 27 Aug 2026 20:34:31 +0200 Subject: [PATCH 14/14] scheduler: the borrow the merge union grew comes back off The merge of main brought the reshaped placement signature beside this branch's call and the union borrowed a generic argument the callee takes by value; CI clippy refused it, and this time the kernel clippy ran locally before the push. --- kernel/src/scheduler.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/src/scheduler.rs b/kernel/src/scheduler.rs index b7f75f0b7c6..6790ddfec8a 100644 --- a/kernel/src/scheduler.rs +++ b/kernel/src/scheduler.rs @@ -756,7 +756,7 @@ pub fn futex_wait( completion::Token::new(phys_addr.phys()), WaitClass::Futex, deadline, - &read, + read, ); // **The predicate again, which is the whole of how the two are told // apart.** `wait_until` answers `Ok(())` for a satisfied predicate and for