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/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/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/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/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/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/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 72c5bda5fde..933b7528f5d 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 @@ -163,15 +163,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/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/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/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/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/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/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/idt/mod.rs b/kernel/src/arch/idt/mod.rs index 60b5da21112..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. } } @@ -422,11 +436,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 +458,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/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/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..1a062086f58 100644 --- a/kernel/src/arch/percpu.rs +++ b/kernel/src/arch/percpu.rs @@ -72,54 +72,55 @@ 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, + /// 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, + /// 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, // 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 +130,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 +142,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 +222,28 @@ 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; +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; -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 +264,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. /// @@ -347,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 { @@ -374,7 +369,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() { @@ -526,7 +521,11 @@ 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`] 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 // is the whole of `alloc_zeroed`'s contract. Irreducible because the block @@ -536,21 +535,50 @@ 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 an aligned, + // writable place for one `PerCpu` that nothing else references or reads + // 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: zero 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, + syscall_task: NO_SYSCALL, + 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 @@ -870,7 +898,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. @@ -910,8 +938,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. @@ -1045,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/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; diff --git a/kernel/src/arch/syscall/gate.rs b/kernel/src/arch/syscall/gate.rs index 42902d3e41c..29faf3b1960 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,11 +158,26 @@ 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, ); } +/// 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/drivers/xhci/device.rs b/kernel/src/drivers/xhci/device.rs index 8ba164f2913..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}; @@ -395,7 +396,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,12 +424,17 @@ 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"), 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 { @@ -450,11 +463,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 +482,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 +496,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()); } @@ -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)", } } @@ -783,27 +817,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 +860,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 +895,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 +914,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..80fae0d396c 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. @@ -724,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 @@ -1822,6 +1833,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/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 25b5f7a1e3b..862ee7b0fd8 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(); @@ -1127,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}"); } @@ -1135,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 } } @@ -1229,6 +1252,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, @@ -1236,7 +1260,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, @@ -1259,7 +1283,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 @@ -1275,6 +1299,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 @@ -1288,7 +1313,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 +1337,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 +1353,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/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/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 8598d489510..ae919b0c5e3 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 @@ -291,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); } @@ -694,6 +676,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/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: 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/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/kernel/src/scheduler.rs b/kernel/src/scheduler.rs index 0a0cbdcbcaa..6790ddfec8a 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 @@ -757,7 +758,33 @@ pub fn futex_wait( deadline, 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 a0c50c63988..aa7ed85f8ed 100644 --- a/src/prose-ledger +++ b/src/prose-ledger @@ -21,20 +21,21 @@ 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 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 -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 @@ -43,13 +44,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 @@ -81,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 289 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 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/msc.rs 548 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 kernel/src/elf/mod.rs 214 0 @@ -97,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 @@ -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 @@ -155,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 717 0 kernel/src/sched/dump.rs 241 0 @@ -165,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 @@ -218,13 +219,13 @@ 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 1603 11 +tests/common/qemu.rs 1607 11 tests/common/screen.rs 89 0 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 @@ -274,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 @@ -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 4828 23 +tests/toyos.rs 4848 23 toyos-abi/src/audio.rs 21 0 toyos-abi/src/boot.rs 90 0 toyos-abi/src/handle.rs 107 0 @@ -551,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/src/redlist.rs b/src/redlist.rs index 35a96f0e73f..2e11f60d067 100644 --- a/src/redlist.rs +++ b/src/redlist.rs @@ -2232,6 +2232,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, diff --git a/tests/CLAUDE.md b/tests/CLAUDE.md index d8bd95683f1..0893b4876dc 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). 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/common/qemu.rs b/tests/common/qemu.rs index 42d29c0e0d6..6257c5cd79e 100644 --- a/tests/common/qemu.rs +++ b/tests/common/qemu.rs @@ -1228,7 +1228,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. /// @@ -1839,6 +1843,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/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/tests/test-durations b/tests/test-durations index 7dab096a0ef..a7729b7e598 100644 --- a/tests/test-durations +++ b/tests/test-durations @@ -215,6 +215,7 @@ kernel_log_file 13152 keyboard_claim_close_spares_stdin 4580 kill_while_blocked 44 klogd_hosted 17870 +lapic_spurious_vector 6829 late_storage_connect 6455 launcher_refusals 2106 locale_detect 5588 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) diff --git a/tests/toyos.rs b/tests/toyos.rs index 92403d38fe2..200e134008c 100644 --- a/tests/toyos.rs +++ b/tests/toyos.rs @@ -544,6 +544,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 @@ -8712,13 +8717,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}")); @@ -8750,7 +8755,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 ); @@ -10097,6 +10102,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 @@ -10723,6 +10739,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 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]