From 9fe48dbe74a3697899f4a513c0d81a9b994aae1e Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 5 Nov 2026 14:27:27 +0100 Subject: [PATCH 01/12] A claimed function interrupts at most once per read of its record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A function a process drives could raise its claim's interrupt on cpu0 as fast as its holder made it send, and nothing bounded how often. Now the handler masks the claim's message after it counts one, and the holder's read that takes the count delivers it again: one interrupt per read, with no rate to choose. Each side changes the record's count first and then, under the claim's gate, puts hardware in the state the record owes (masked exactly while a count waits), read from the word and not from its own change. The side that takes the gate last sees both changes, so a mask that lands after the read that re-armed the claim cannot strand it masked. kernel/loom's device_irq_gate models it; the device-irq-gate-intent control, where each side applies its own intent, reds both models. The masks, each published before it returns: - VT-d: the entry not present with FPD set (Rev. 4.1 §9.9, §5.1.4.1 Table 13: the blocked message's fault is qualified and not recorded), one 64-bit write, then an index-selective interrupt-entry-cache invalidation (§6.4, §6.5.2.7), which also drains what the unit already remapped. - AMD-Vi: RemapEn clear and SupIOPF set (48882 rev 3.11 Table 22; target abort, §2.1.3.1), then INVALIDATE_INTERRUPT_TABLE (§2.4.5). - ITS: the LPI's configuration byte disabled, then INV and SYNC; the message stays pending until it is enabled again. Release enables it first. The handler cannot take the existing IOMMU locks: VT-d's map takes TABLES under UNITS and reaches the page allocator. So each unit's invalidation queue (VT-d's queue, AMD-Vi's command buffer, the ITS command queue) moves to a leaf lock of its own, taken with interrupts masked and holding nothing else. Every invalidation's completion wait now runs with interrupts off. Drivers must take their record before servicing the device, stated at toyos::PciDev::irq; all ten already do. Co-Authored-By: Claude Opus 5.5 --- build/ci.rs | 4 + .../a-claimed-function-can-storm-its-cpu.md | 34 ------- kernel/Cargo.toml | 4 + kernel/amdvi/src/irte.rs | 10 ++ kernel/amdvi/tests/encodings.rs | 11 +++ kernel/loom/Cargo.toml | 11 +++ kernel/loom/tests/device_irq.rs | 6 +- kernel/loom/tests/device_irq_gate.rs | 76 +++++++++++++++ kernel/src/arch/aarch64/irqchip/its.rs | 95 ++++++++++++++----- kernel/src/arch/x86_64/amdvi/domain.rs | 3 +- kernel/src/arch/x86_64/amdvi/interrupt.rs | 45 +++++++-- kernel/src/arch/x86_64/amdvi/mod.rs | 54 +++++++---- kernel/src/arch/x86_64/iommu_unit.rs | 36 +++++-- kernel/src/arch/x86_64/vtd/domain.rs | 3 +- kernel/src/arch/x86_64/vtd/interrupt.rs | 43 ++++++++- kernel/src/arch/x86_64/vtd/mod.rs | 72 ++++++++++---- kernel/src/arch/x86_64/vtd/queue.rs | 60 +++++------- kernel/src/arch/x86_64/vtd/table.rs | 5 + kernel/src/iommu/mod.rs | 20 ++++ kernel/src/isa.rs | 2 +- kernel/src/pcidev/mod.rs | 67 +++++++++++-- kernel/src/pcidev/record.rs | 40 +++++++- sdk/abi/src/pci.rs | 5 + sdk/toyos/src/device.rs | 5 + 24 files changed, 542 insertions(+), 169 deletions(-) delete mode 100644 issues/a-claimed-function-can-storm-its-cpu.md create mode 100644 kernel/loom/tests/device_irq_gate.rs diff --git a/build/ci.rs b/build/ci.rs index 206b42276c8..cfd6062fd94 100644 --- a/build/ci.rs +++ b/build/ci.rs @@ -606,6 +606,10 @@ pub(crate) const CONTROLS: &[Control] = &[ red(KERNEL_LOOM, "device-irq-lossy", Some("device_irq"), &[ Fails("every_message_is_counted_once"), ]), + red(KERNEL_LOOM, "device-irq-gate-intent", Some("device_irq_gate"), &[ + Fails("hardware_ends_as_the_record_owes"), + Fails("hardware_ends_as_the_record_owes_after_two_messages"), + ]), Control { krate: KERNEL_LOOM, feature: "no-preempt-guard", diff --git a/issues/a-claimed-function-can-storm-its-cpu.md b/issues/a-claimed-function-can-storm-its-cpu.md deleted file mode 100644 index 91a7e744bfd..00000000000 --- a/issues/a-claimed-function-can-storm-its-cpu.md +++ /dev/null @@ -1,34 +0,0 @@ ---- -status: open -kind: defect -severity: [kernel-from-userland] -opened: 2026-10-04 ---- - -# A claimed function can storm its CPU - -A function a process drives raises its claim's interrupt on cpu0 for -every message it sends — on x86-64 the claim's vector -(`kernel/src/arch/x86_64/idt/user_dev.rs`), on AArch64 the function's LPI through -the ITS (`kernel/src/arch/aarch64/irqchip/its.rs`) — and `pcidev::isr` -(`kernel/src/pcidev/mod.rs`) takes each one; nothing bounds how often. Its -holder programs the device through the BARs it maps, so a hostile or broken -holder can make the device send messages as fast as the bus carries -them, and each one is an interrupt cpu0 takes before anything else it runs. -The kernel's own xHCI measured what that does to a CPU for a kernel -driver's source, ten timer interrupts in thirty-four seconds on T14 run 20, -before that driver left the kernel; a claim is the same source with its -holder outside the kernel, which every xHCI controller now is. - -The lever is the claim's own translation: on x86-64 its remapping entry, which -made not present stops the function's messages at the unit; on AArch64 its -LPI's configuration byte, which cleared and followed by an `INV` disables -the LPI at the redistributor. Masking through the device would need the -holder's cooperation. What rate is a storm is not decided, and a number -chosen without a measurement behind it is the silent decision this file -exists to refuse. - -Owner: the IOMMU track. Exit: a claimed function's messages past a bound the -kernel derives and states are stopped at its entry and recorded against its -claim, and a T14 row with a holder that provokes them shows cpu0's timer and -other sources still served. diff --git a/kernel/Cargo.toml b/kernel/Cargo.toml index 20fd3581617..caa3327f8da 100644 --- a/kernel/Cargo.toml +++ b/kernel/Cargo.toml @@ -62,6 +62,10 @@ shard-publish-relaxed = [] # `device_irq` reds. They *are* the record's design, so turning them off reverts # the whole of what the model claims. device-irq-lossy = [] +# `src/pcidev/record.rs`'s gate follows each side's intent instead of the +# record, so a mask landing after the read that opened it strands the claim, and +# `device_irq_gate` reds. +device-irq-gate-intent = [] # The same arrangement for the CPU roster's publication edge: `src/smp_roster.rs`'s # `commit` publishes the count with a `Relaxed` store instead of the release the # slot store needs, so a reader can see a count over an unfilled slot, and diff --git a/kernel/amdvi/src/irte.rs b/kernel/amdvi/src/irte.rs index 575ed966f2e..62c6a5b3722 100644 --- a/kernel/amdvi/src/irte.rs +++ b/kernel/amdvi/src/irte.rs @@ -5,6 +5,9 @@ //! entry is the message's own vector. const REMAP_EN: u64 = 1 << 0; +/// `SupIOPF`: no `IO_PAGE_FAULT` is logged for a message through the entry, +/// whatever `RemapEn` says (Table 22). +const SUP_IOPF: u64 = 1 << 1; const DESTINATION_SHIFT: u32 = 8; /// `Vector`, bits 71:64: bits 7:0 of the second doubleword. const VECTOR_SHIFT: u32 = 0; @@ -30,6 +33,13 @@ pub const fn entry(vector: u8, destination: Destination) -> [u64; 2] { pub const ABSENT: [u64; 2] = [0, 0]; +/// `entry` refusing its messages silently: `RemapEn` clear, so each is target +/// aborted (Table 22, §2.1.3.1), and `SupIOPF` set, so none is logged. The +/// rest of the entry is kept for the [`entry`] it was to deliver again. +pub const fn masked(entry: [u64; 2]) -> [u64; 2] { + [entry[0] & !REMAP_EN | SUP_IOPF, entry[1]] +} + /// Whether an entry read back delivers anything. pub const fn present(words: [u64; 2]) -> bool { words[0] & REMAP_EN != 0 diff --git a/kernel/amdvi/tests/encodings.rs b/kernel/amdvi/tests/encodings.rs index a0ad5895673..fdf6ad3f16b 100644 --- a/kernel/amdvi/tests/encodings.rs +++ b/kernel/amdvi/tests/encodings.rs @@ -244,6 +244,17 @@ fn a_remapping_entry_delivers_its_vector_to_its_destination() { assert!(!irte::present(irte::ABSENT)); } +/// Table 22: a masked entry has `RemapEn` [0] clear and `SupIOPF` [1] set, +/// and every other bit of the entry it masks. +#[test] +fn a_masked_entry_aborts_silently_and_keeps_its_fields() { + let delivering = irte::entry(0xFB, Destination::Extended(0x1234_5678)); + let masked = irte::masked(delivering); + assert_eq!(masked, [delivering[0] & !1 | 1 << 1, delivering[1]]); + assert!(!irte::present(masked)); + assert_eq!((irte::vector(masked), irte::destination(masked)), (0xFB, 0x1234_5678)); +} + /// Figure 48: `INVALIDATE_INTERRUPT_TABLE`, code 05h, the id at [15:0]. #[test] fn invalidating_an_interrupt_table_names_its_requester() { diff --git a/kernel/loom/Cargo.toml b/kernel/loom/Cargo.toml index bf01b3134fd..f9d8c0a3446 100644 --- a/kernel/loom/Cargo.toml +++ b/kernel/loom/Cargo.toml @@ -142,6 +142,17 @@ shard-publish-relaxed = [] # Never on by default and never reachable from a kernel build, which declares # the same name only so `cfg` checking knows it. device-irq-lossy = [] +# The negative control for a claimed function's interrupt gate. The ISR masks +# and the read opens by their own intent, not by what the record owes, so a +# mask that lands after the read opened the gate strands the claim, and +# `device_irq_gate.rs` must red: +# +# cargo test --manifest-path kernel/loom/Cargo.toml --features device-irq-gate-intent \ +# --test device_irq_gate +# +# Never on by default and never reachable from a kernel build, which declares +# the same name only so `cfg` checking knows it. +device-irq-gate-intent = [] # The negative control for a program's log ring, `sdk/toyos/src/log/ring.rs`. It # makes the store that publishes a record — the shared ring's `published` word # and a lane's `head` — `Relaxed`, so a reader can find the word with the body diff --git a/kernel/loom/tests/device_irq.rs b/kernel/loom/tests/device_irq.rs index b098aebdb6a..24a6c044819 100644 --- a/kernel/loom/tests/device_irq.rs +++ b/kernel/loom/tests/device_irq.rs @@ -117,12 +117,12 @@ fn an_idle_record_answers_nothing() { loom::model(|| { let irq = Interrupt::new(); assert!(irq.take().is_none(), "an untouched record answered a reader"); - assert!(!irq.armed(), "an untouched record read ready"); + assert!(!irq.pending(), "an untouched record read ready"); irq.took(7); - assert!(irq.armed(), "a record with a message in it read empty"); + assert!(irq.pending(), "a record with a message in it read empty"); let record = irq.take().expect("a record with a message in it answered nothing"); assert_eq!((record.count, record.first_nanos, record.last_nanos), (1, 7, 7)); assert!(irq.take().is_none(), "the same message was answered twice"); - assert!(!irq.armed(), "a drained record still reads ready"); + assert!(!irq.pending(), "a drained record still reads ready"); }); } diff --git a/kernel/loom/tests/device_irq_gate.rs b/kernel/loom/tests/device_irq_gate.rs new file mode 100644 index 00000000000..0f8fd57e391 --- /dev/null +++ b/kernel/loom/tests/device_irq_gate.rs @@ -0,0 +1,76 @@ +//! Loom: a claimed function's interrupt, masked while its record holds a +//! count and opened by the read that takes it (`pcidev/record.rs`'s [`Gate`]). +//! +//! **The invariant is that hardware ends in the state the record owes it: +//! masked exactly while a count waits.** A claim left masked with nothing to +//! read never interrupts again, and its driver waits for a device that has +//! spoken; one left open with a count waiting is the storm the mask exists to +//! stop. What makes it hold is that each side reads the record under the +//! gate's lock after its own change to the count, so the negative control is +//! each side applying its own intent instead (the ISR masks, the read opens), +//! where a mask that lands after the read opened the gate strands the claim: +//! +//! ```text +//! cargo test --manifest-path kernel/loom/Cargo.toml --features device-irq-gate-intent \ +//! --test device_irq_gate +//! ``` +//! +//! and [`hardware_ends_as_the_record_owes`] must red. + +use kernel_loom::device_irq::{Gate, Interrupt}; +use loom::sync::{Arc, Mutex}; + +/// The claim's interrupt as hardware has it, behind the gate's lock. +struct Line(Mutex); + +impl Gate for Line { + fn hold(&self, masked: impl FnOnce() -> bool) { + let mut hardware = self.0.lock().unwrap(); + *hardware = masked(); + } +} + +/// Messages against one read that may run at any point among them, and a +/// last read once they have all landed: after each, hardware is masked +/// exactly while the record holds a count. +fn model(messages: u64) { + loom::model(move || { + let irq = Arc::new(Interrupt::new()); + let line = Arc::new(Line(Mutex::new(false))); + + let isr = { + let (irq, line) = (irq.clone(), line.clone()); + loom::thread::spawn(move || { + for at in 1..=messages { + irq.took_masking(at, &*line); + } + }) + }; + + let taken = irq.take_opening(&*line).map_or(0, |record| u64::from(record.count)); + isr.join().unwrap(); + let masked = *line.0.lock().unwrap(); + assert_eq!( + masked, + irq.pending(), + "the read took {taken} of {messages} message(s) and hardware is {} with {} waiting", + if masked { "masked" } else { "open" }, + if irq.pending() { "a count" } else { "nothing" }, + ); + + let left = irq.take_opening(&*line).map_or(0, |record| u64::from(record.count)); + assert_eq!(taken + left, messages, "the reads took {taken} and {left} of {messages}"); + assert!(!*line.0.lock().unwrap(), "every message was read and the claim is still masked"); + }); +} + +#[test] +fn hardware_ends_as_the_record_owes() { + model(1); +} + +/// A second message landing while the first is masked or being read. +#[test] +fn hardware_ends_as_the_record_owes_after_two_messages() { + model(2); +} diff --git a/kernel/src/arch/aarch64/irqchip/its.rs b/kernel/src/arch/aarch64/irqchip/its.rs index db18f02443c..fef70016c0e 100644 --- a/kernel/src/arch/aarch64/irqchip/its.rs +++ b/kernel/src/arch/aarch64/irqchip/its.rs @@ -28,6 +28,11 @@ //! does not consume within [`TIMEOUT_PER_SECOND`]'s bound, is a kernel //! defect, and panics. //! +//! **A claim's LPI is masked by its configuration byte's enable** (Table 5-1), +//! reread at the redistributor on an `INV` (§5.3.6): a disabled LPI's message +//! stays pending and is taken once it is enabled again. Every LPI is enabled +//! whenever no claim has masked it. +//! //! **A claim's DeviceID is the one the SMMUv3's own routes carry** //! (`super::super::smmu::its_device`): a function the unit would not put on a //! domain is refused before its claim reaches the unit. @@ -44,7 +49,7 @@ use crate::log; use crate::mm::pmm::{self, PhysPage}; use crate::mm::policy::MmioPolicy; use crate::mm::{DirectMap, Mmio, PAGE_2M}; -use crate::sync::Lock; +use crate::sync::{Lock, Masked}; /// A command is consumed within a second, and a register settles in one: /// `settles` takes the bound as a rate. @@ -89,19 +94,29 @@ const ITT_ALIGN: u64 = 256; static ITS: Lock> = Lock::new(None); -/// The ITS this kernel drives, armed. -struct Live { +/// The armed ITS's command queue: a leaf, taken with interrupts masked and +/// holding nothing else, because a claim's interrupt handler masks its LPI +/// through it ([`set_masked`]) while any other context may hold it. +static QUEUE: Masked> = Masked::new(None); + +struct Queue { regs: Mmio, - /// The MADT's id for it, which the IORT names it by. - id: u32, - /// `GITS_TRANSLATER`'s physical address: every claim's message address. - doorbell: u32, queue: Mmio, commands: CommandQueue, /// `GITS_CWRITER` as last written. cwriter: u64, /// The boot CPU's redistributor, as this ITS names it. target: Target, +} + +/// The ITS this kernel drives, armed. +struct Live { + /// The MADT's id for it, which the IORT names it by. + id: u32, + /// `GITS_TRANSLATER`'s physical address: every claim's message address. + doorbell: u32, + /// The redistributor's LPI configuration table. + configuration: Mmio, collection: Collection, events: EventBits, space: lpi::Space, @@ -127,9 +142,17 @@ impl Live { self.itts.push((device, table)); Some(table) } +} + +/// `commands`, then a `SYNC` to the boot CPU's redistributor, consumed: +/// every one has taken effect when this returns (§5.3.15). +fn issue(commands: &[Command]) { + let closed = crate::arch::IrqGuard::close(); + let mut held = QUEUE.lock(&closed); + held.as_mut().expect("ITS: a command issued with no ITS armed").issue(commands); +} - /// `commands`, then a `SYNC` to the boot CPU's redistributor, consumed: - /// every one has taken effect when this returns (§5.3.15). +impl Queue { fn issue(&mut self, commands: &[Command]) { let (regs, queue) = (self.regs, self.commands); for command in commands.iter().copied().chain([Command::Sync(self.target)]) { @@ -346,14 +369,15 @@ fn bring_up(id: u32, base: u64, frame: u64) -> Result Result) -> Result impl core::fmt::Display { alloc::format!("LPI {}", self.lpi.intid()) } + + pub fn line(&self) -> Line { + self.line + } +} + +/// A claim's LPI, which [`set_masked`] masks; named only while its +/// [`Claimed`] is held. +#[derive(Clone, Copy)] +pub struct Line { + /// The LPI's configuration byte. + configuration: Mmio, + device: u32, + event: toyos_its::Event, +} + +/// `line`'s LPI disabled, or enabled again, at the redistributor before this +/// returns. Takes only the command queue. +pub fn set_masked(line: Line, masked: bool) { + line.configuration.write_u8(0, lpi::configuration(PRIORITY, !masked)); + issue(&[Command::Reconfigure { device: line.device, event: line.event }]); } /// Map `source`'s DeviceID to its translation table, its one event to `pcidev` @@ -486,24 +529,28 @@ pub fn claim(function: usize, source: StreamId) -> Result { let table = live.table_of(device).ok_or(Refused::NoMemory)?; let event = live.events.event(EVENT).expect("event 0 is in every table"); let (collection, events) = (live.collection, live.events); - live.issue(&[ + issue(&[ Command::MapDevice { device, table, events }, Command::MapEvent { device, event, lpi, collection }, ]); live.held.push(device); log!("ITS: {source}, DeviceID {device:#x}, event {EVENT} is LPI {}", lpi.intid()); - Ok(Claimed { msi: Msi { address: live.doorbell, data: EVENT }, device, lpi }) + let configuration = live.configuration.subregion(lpi.configuration_index() as u64, 1); + Ok(Claimed { msi: Msi { address: live.doorbell, data: EVENT }, line: Line { configuration, device, event }, lpi }) } /// A claim's DeviceID unmapped, and anything it left pending dropped, before /// this returns. +/// +/// Its LPI is enabled again first, where its claim left it masked, so the +/// next claim on the function finds it as every unclaimed LPI is. pub fn release(claimed: &Claimed, source: StreamId) { - let device = claimed.device; + let Line { device, event, configuration } = claimed.line; let mut held = ITS.lock(); let live = held.as_mut().unwrap_or_else(|| panic!("ITS: {source} released with no ITS armed")); let at = live.held.iter().position(|held| *held == device); live.held.swap_remove(at.unwrap_or_else(|| panic!("ITS: DeviceID {device:#x} released unmapped"))); - let event = live.events.event(EVENT).expect("event 0 is in every table"); - live.issue(&[Command::Discard { device, event }, Command::UnmapDevice { device }]); + configuration.write_u8(0, lpi::configuration(PRIORITY, true)); + issue(&[Command::Reconfigure { device, event }, Command::Discard { device, event }, Command::UnmapDevice { device }]); log!("ITS: {source}, DeviceID {device:#x}, unmapped from LPI {}", claimed.lpi.intid()); } diff --git a/kernel/src/arch/x86_64/amdvi/domain.rs b/kernel/src/arch/x86_64/amdvi/domain.rs index 42d8d2b5f21..49a9792e06b 100644 --- a/kernel/src/arch/x86_64/amdvi/domain.rs +++ b/kernel/src/arch/x86_64/amdvi/domain.rs @@ -11,7 +11,8 @@ //! A requester an IVMD names stays on the identity domain: firmware declares //! memory it must keep reaching, which a domain of its own would not map. //! -//! `DOMAINS` is taken before `UNITS` or `TABLES`, and those two are never held together. +//! `DOMAINS` is taken before `UNITS` or `TABLES`, and those two are never held together; +//! a unit's slot in `UNITS` is a leaf. use alloc::vec::Vec; diff --git a/kernel/src/arch/x86_64/amdvi/interrupt.rs b/kernel/src/arch/x86_64/amdvi/interrupt.rs index 46657eabb61..9e60fb756df 100644 --- a/kernel/src/arch/x86_64/amdvi/interrupt.rs +++ b/kernel/src/arch/x86_64/amdvi/interrupt.rs @@ -13,6 +13,11 @@ //! `GuestMode` 0 (Table 21, Figure 17); its destination is 32 bits wide under //! `XTEn` (§2.2.5.3, Figure 19), which an x2APIC machine needs. //! +//! **A claim's entry is masked with `RemapEn` clear and `SupIOPF` set** +//! ([`irte::masked`]): its messages are target aborted, which the host bridge +//! completes with no error reported (§2.1.3.1), and none is logged. Each +//! change is published with `INVALIDATE_INTERRUPT_TABLE` (§2.4.5). +//! //! This module's lock is taken before `DOMAINS`, `UNITS` and `TABLES`, never //! after any of them. @@ -123,21 +128,44 @@ fn report(source: StreamId, vector: u8, read: [u64; 2]) { ); } +/// A claim's entry, held until [`release`]: whose table it is in, where, +/// and what it holds while it delivers. +#[derive(Clone, Copy)] +pub struct Line { + requester: u16, + vector: u8, + slot: crate::mm::Mmio, + delivers: [u64; 2], +} + /// `source`'s entry at `vector`, delivering it to this kernel's CPU. -fn arm(source: StreamId, vector: u8) { +fn arm(source: StreamId, vector: u8) -> Line { let mut remap = REMAP.lock(); let destination = match remap.extended { Some(true) => Destination::Extended(MSG_DEST), _ => Destination::Narrow(MSG_DEST_NARROW), }; - let read = remap.write(source.requester(), vector, irte::entry(vector, destination)); + let requester = source.requester(); + let delivers = remap.write(requester, vector, irte::entry(vector, destination)); + let table = remap.tables[&requester]; drop(remap); - report(source, vector, read); + report(source, vector, delivers); + Line { requester, vector, slot: memory(table.get() + u64::from(vector) * 16, 16), delivers } +} + +/// A claim's entry masked or delivering again, gone from every unit's cache +/// before this returns. One 64-bit write: `RemapEn` and `SupIOPF` are both +/// in the low half. Takes only the units' buffers, so a claim's interrupt +/// handler may call it. +pub(in crate::arch::x86_64) fn set_masked(line: Line, masked: bool) { + let entry = if masked { irte::masked(line.delivers) } else { line.delivers }; + line.slot.write_u64(0, entry[0]); + submit(&[Command::InvalidateInterrupts(line.requester)]); } /// A claim's entry at `vector` for `source`, whose entry from now on passes /// nothing firmware declared for it and gives it no `SysMgt`. -pub(in crate::arch::x86_64) fn claim(vector: u8, source: StreamId) -> Result { +pub(in crate::arch::x86_64) fn claim(vector: u8, source: StreamId) -> Result<(Msi, Line), Refused> { // A requester the platform speaks as is refused before here // (`iommu::isolates`), and one a kernel driver holds by `pcidev::claim`. // A table already written is a re-claim's, whose setting is `CLAIMED`; @@ -147,15 +175,14 @@ pub(in crate::arch::x86_64) fn claim(vector: u8, source: StreamId) -> Result = Lock::new(Tables::new()); -/// Every translating unit, and the single owner of each one's command buffer: -/// a buffer has one tail, so a second holder would write behind its back. -static UNITS: Lock> = Lock::new(Vec::new()); +/// Each translating unit's command buffer, [`PUBLISHED`] of them: a buffer +/// has one tail, so its slot is the one owner. A leaf, taken with interrupts +/// masked and holding nothing else, because a claim's interrupt handler +/// masks its entry through it ([`interrupt::set_masked`]) while any other +/// context may hold it. +static UNITS: [Masked>; MAX_UNITS] = [const { Masked::new(None) }; MAX_UNITS]; +static PUBLISHED: AtomicUsize = AtomicUsize::new(0); + +/// Each translating unit's SMI filter, until [`settle`] writes it. +static SMI: Lock>)>> = Lock::new(Vec::new()); /// `bytes` at `phys`, through the direct map. fn memory(phys: u64, bytes: u64) -> Mmio { @@ -476,8 +483,6 @@ struct Live { tail: usize, done: Address<12>, waited: u64, - /// [`Ready::smi`], until [`settle`] writes it. - smi: Option>, } impl Live { @@ -518,8 +523,9 @@ impl Live { /// `commands` on every translating unit, finished on each before this returns. fn submit(commands: &[Command]) { - for unit in UNITS.lock().iter_mut() { - unit.submit(commands); + for slot in &UNITS[..PUBLISHED.load(Ordering::Acquire)] { + let closed = crate::arch::IrqGuard::close(); + slot.lock(&closed).as_mut().expect("amdvi: a published unit has its buffer").submit(commands); } } @@ -568,7 +574,7 @@ fn program(unit: Ready, devices: &[PciDevice], remap: Option) { || regs.read_u32(reg::STATUS) & running == running, || format!("iommu: amdvi unit{index} never ran its command buffer and event log: STATUS={:#010x}", regs.read_u32(reg::STATUS)), ); - let mut live = Live { index, regs, commands: memory(commands.get(), PAGE_SIZE), tail: 0, done, waited: 0, smi }; + let mut live = Live { index, regs, commands: memory(commands.get(), PAGE_SIZE), tail: 0, done, waited: 0 }; // §2.4.8 needs `INVALIDATE_IOMMU_ALL` alone. QEMU 11.1.1 moves a function // off pass-through only on an `INVALIDATE_DEVTAB_ENTRY` that names it // (`issues/qemus-invalidate-iommu-all-leaves-every-function-untranslated.md`), @@ -585,34 +591,40 @@ fn program(unit: Ready, devices: &[PciDevice], remap: Option) { events.get(), devices.len(), ); - UNITS.lock().push(live); + let mut settling = SMI.lock(); + let at = PUBLISHED.load(Ordering::Relaxed); + { + let closed = crate::arch::IrqGuard::close(); + *UNITS[at].lock(&closed) = Some(live); + } + PUBLISHED.store(at + 1, Ordering::Release); + settling.push((index, regs, smi)); } /// Every unit's SMI filter on, once every function this kernel drives is out /// of firmware's hands. A refused SMI is logged as an `IO_PAGE_FAULT` /// (§2.1.5.1) for [`fault`]. pub fn settle() { - let mut units = UNITS.lock(); + let mut units = SMI.lock(); let mut everywhere = !units.is_empty(); - for unit in units.iter_mut() { - let Some(filter) = unit.smi.take() else { + for (index, regs, smi) in units.iter_mut() { + let Some(filter) = smi.take() else { everywhere = false; continue; }; for (i, register) in (0u64..).zip(&filter) { - if !SmiFilter(unit.regs.read_u64(reg::SMI_FILTER + 8 * i)).locked() { - unit.regs.write_u64(reg::SMI_FILTER + 8 * i, register.0); + if !SmiFilter(regs.read_u64(reg::SMI_FILTER + 8 * i)).locked() { + regs.write_u64(reg::SMI_FILTER + 8 * i, register.0); } } - unit.regs.write_u64(reg::CONTROL, unit.regs.read_u64(reg::CONTROL) | control::SMIF_EN | control::SMIF_LOG_EN); + regs.write_u64(reg::CONTROL, regs.read_u64(reg::CONTROL) | control::SMIF_EN | control::SMIF_LOG_EN); let passes: Vec = (0..filter.len() as u64) - .filter_map(|i| SmiFilter(unit.regs.read_u64(reg::SMI_FILTER + 8 * i)).passes()) + .filter_map(|i| SmiFilter(regs.read_u64(reg::SMI_FILTER + 8 * i)).passes()) .map(|id| format!("{}", requester(id))) .collect(); log!( - "iommu: amdvi unit{} SMI filter on control={:#018x}, passing {}", - unit.index, - unit.regs.read_u64(reg::CONTROL), + "iommu: amdvi unit{index} SMI filter on control={:#018x}, passing {}", + regs.read_u64(reg::CONTROL), passes.join(" ") ); } diff --git a/kernel/src/arch/x86_64/iommu_unit.rs b/kernel/src/arch/x86_64/iommu_unit.rs index 4a5143161aa..73efa480410 100644 --- a/kernel/src/arch/x86_64/iommu_unit.rs +++ b/kernel/src/arch/x86_64/iommu_unit.rs @@ -282,8 +282,7 @@ pub mod interrupt { pub struct Claimed { pub msi: Msi, vector: ClaimVector, - /// VT-d's entry index; AMD-Vi's entry is the vector's own. - entry: Option, + line: Line, } impl Claimed { @@ -291,6 +290,27 @@ pub mod interrupt { pub fn interrupt(&self) -> impl core::fmt::Display { self.vector } + + pub fn line(&self) -> Line { + self.line + } + } + + /// A claim's remapping entry, which [`set_masked`] masks; named only + /// while its [`Claimed`] is held. + #[derive(Clone, Copy)] + pub enum Line { + Vtd(vtd::interrupt::Line), + AmdVi(amdvi::interrupt::Line), + } + + /// `line`'s messages refused silently at the unit, or delivered again, + /// before this returns. Takes only the units' invalidation queues. + pub fn set_masked(line: Line, masked: bool) { + match line { + Line::Vtd(line) => vtd::interrupt::set_masked(line, masked), + Line::AmdVi(line) => amdvi::interrupt::set_masked(line, masked), + } } /// Whether a claimed function's every message reaches only its own @@ -321,13 +341,13 @@ pub mod interrupt { pub fn claim(function: usize, source: StreamId) -> Result { let vector = user_dev::take(function).ok_or(Refused::NoVector)?; let made = match backend() { - Backend::AmdVi => amdvi::interrupt::claim(vector.vector(), source).map(|msi| (msi, None)), + Backend::AmdVi => amdvi::interrupt::claim(vector.vector(), source).map(|(msi, line)| (msi, Line::AmdVi(line))), Backend::Vtd => vtd::interrupt::claim(vector.vector(), source) - .map(|entry| (vtd::interrupt::message(entry), Some(entry))), + .map(|line| (vtd::interrupt::message(line.index()), Line::Vtd(line))), Backend::None => unreachable!("iommu: a claim's message made where nothing remaps"), }; match made { - Ok((msi, entry)) => Ok(Claimed { msi, vector, entry }), + Ok((msi, line)) => Ok(Claimed { msi, vector, line }), Err(why) => { user_dev::give_back(vector); Err(why) @@ -337,9 +357,9 @@ pub mod interrupt { /// A claim's entry gone from every unit, and then its vector free. pub fn release(claimed: &Claimed, source: StreamId) { - match claimed.entry { - Some(entry) => vtd::interrupt::release(entry, source), - None => amdvi::interrupt::release(claimed.vector.vector(), source), + match claimed.line { + Line::Vtd(line) => vtd::interrupt::release(line, source), + Line::AmdVi(line) => amdvi::interrupt::release(line, source), } user_dev::give_back(claimed.vector); } diff --git a/kernel/src/arch/x86_64/vtd/domain.rs b/kernel/src/arch/x86_64/vtd/domain.rs index bb7af2bec6d..d4a0e71b19a 100644 --- a/kernel/src/arch/x86_64/vtd/domain.rs +++ b/kernel/src/arch/x86_64/vtd/domain.rs @@ -13,7 +13,8 @@ //! 6.1: with caching mode reported, software invalidates after *every* change, //! a mapping becoming present included. //! -//! Lock order here is `DOMAINS`, `REMAP`, `UNITS`, `TABLES`, never the reverse. +//! Lock order here is `DOMAINS`, `REMAP`, `UNITS`, `TABLES`, never the reverse, +//! and a unit's invalidation queue last of all. use crate::log; use alloc::vec::Vec; diff --git a/kernel/src/arch/x86_64/vtd/interrupt.rs b/kernel/src/arch/x86_64/vtd/interrupt.rs index b0706752daf..a4a2903e592 100644 --- a/kernel/src/arch/x86_64/vtd/interrupt.rs +++ b/kernel/src/arch/x86_64/vtd/interrupt.rs @@ -29,6 +29,12 @@ //! the boot, one per arming, and by a claim until its release, when it goes //! not present and is free again. //! +//! **A claim's entry is masked not present with `FPD` set** (§9.9 Figure 9-9, +//! `FPD` bit 1, evaluated whatever `P` says): a message through it is blocked +//! (§5.1.4), and its fault, `22h`, is a qualified one (§5.1.4.1 Table 13), +//! recorded only where `FPD` is clear. Each change is followed by the entry's +//! interrupt-entry-cache invalidation (§6.4, §6.5.2.7). +//! //! This module's lock is taken before `UNITS` and before `TABLES`, never after //! either. @@ -57,6 +63,7 @@ const ENTRIES: usize = 256; const EXTENDED_INTERRUPT_MODE: u64 = 1 << 11; const PRESENT: u64 = 1; +const FAULT_PROCESSING_DISABLE: u64 = 1 << 1; const TRIGGER_LEVEL: u64 = 1 << 4; const VECTOR_SHIFT: u64 = 16; const DESTINATION_SHIFT: u64 = 32; @@ -212,15 +219,43 @@ pub(in crate::arch::x86_64) fn message(index: usize) -> Msi { } } +/// A claim's entry, held until [`release`]: where it is and what its low +/// half holds while it delivers. +#[derive(Clone, Copy)] +pub struct Line { + table: Table, + index: usize, + delivers: u64, +} + +impl Line { + pub(in crate::arch::x86_64) fn index(self) -> usize { + self.index + } +} + /// An entry delivering a claim's `vector`, written for `source` and published -/// to every unit before this returns; its index, held until [`release`]. -pub(in crate::arch::x86_64) fn claim(vector: u8, source: StreamId) -> Result { - allocate(source, vector, MSG_DEST, false) +/// to every unit before this returns. +pub(in crate::arch::x86_64) fn claim(vector: u8, source: StreamId) -> Result { + let index = allocate(source, vector, MSG_DEST, false)?; + let table = REMAP.lock().table.expect("iommu: a claim's entry was written with a table armed"); + Ok(Line { table, index, delivers: table.read_pair(index).0 }) +} + +/// A claim's entry masked or delivering again, gone from every remapping +/// unit's cache before this returns. One 64-bit write: `P` and `FPD` are both +/// in the low half, so the unit reads the old entry or the new one. Takes +/// only the units' queues, so a claim's interrupt handler may call it. +pub(in crate::arch::x86_64) fn set_masked(line: Line, masked: bool) { + let low = if masked { line.delivers & !PRESENT | FAULT_PROCESSING_DISABLE } else { line.delivers }; + line.table.write_low(line.index, low); + super::remapping_queues(|queue| queue.invalidate_interrupt(line.index)); } /// A claim's entry not present again and gone from every unit's cache, and /// then free. -pub(in crate::arch::x86_64) fn release(entry: usize, source: StreamId) { +pub(in crate::arch::x86_64) fn release(line: Line, source: StreamId) { + let entry = line.index; let lo = { let remap = REMAP.lock(); let Some(table) = remap.table else { diff --git a/kernel/src/arch/x86_64/vtd/mod.rs b/kernel/src/arch/x86_64/vtd/mod.rs index 6f013910a3a..0daa6e73510 100644 --- a/kernel/src/arch/x86_64/vtd/mod.rs +++ b/kernel/src/arch/x86_64/vtd/mod.rs @@ -26,8 +26,9 @@ use crate::drivers::acpi::TableError; use crate::drivers::pci::PciDevice; use crate::iommu::{AddressWidth, StreamId}; use crate::mm::Mmio; -use crate::sync::Lock; +use crate::sync::{Lock, Masked}; use crate::time::{Duration, Tripwire}; +use core::sync::atomic::{AtomicUsize, Ordering}; use super::iommu_unit::{yn, MAX_UNITS}; use dmar::{Dmar, Malformed, Scope, Scopes, Structure}; @@ -88,19 +89,47 @@ const COMMAND_TIMEOUT: Tripwire = Tripwire::absurd( /// Remapping tables outlive `init`: the unit keeps reading them while it is on. static TABLES: Lock = Lock::new(Tables::new()); -/// Every translating unit, and the single owner of each one's invalidation -/// queue: a queue has one tail, so a second holder would submit behind its back. +/// Every translating unit. static UNITS: Lock> = Lock::new(Vec::new()); -/// A unit past its hand-over: its window, its queue, and the root table its -/// context entries live in. -pub(super) struct Live { - regs: Mmio, +/// Each translating unit's invalidation queue, [`PUBLISHED`] of them, in the +/// order [`UNITS`] holds them: a queue has one tail, so its slot is the one +/// owner. A leaf, taken with interrupts masked and holding nothing else, +/// because a claim's interrupt handler masks its entry through it +/// ([`interrupt::set_masked`]) while any other context may hold it. +static QUEUES: [Masked>; MAX_UNITS] = [const { Masked::new(None) }; MAX_UNITS]; +static PUBLISHED: AtomicUsize = AtomicUsize::new(0); + +struct Published { queue: Queue, - root: Table, /// A descriptor type a unit does not implement stalls its queue, so the /// interrupt-entry-cache type goes only to a unit that remaps. remaps: bool, +} + +/// `f` on the queue of the unit at `index` in [`QUEUES`]. +fn with_queue(index: usize, f: impl FnOnce(&mut Published) -> R) -> R { + let closed = crate::arch::IrqGuard::close(); + let mut held = QUEUES[index].lock(&closed); + f(held.as_mut().expect("vtd: a published unit has its queue")) +} + +/// `f` on every remapping unit's queue. +fn remapping_queues(mut f: impl FnMut(&mut Queue)) { + for index in 0..PUBLISHED.load(Ordering::Acquire) { + with_queue(index, |unit| { + if unit.remaps { + f(&mut unit.queue); + } + }); + } +} + +/// A unit past its hand-over: its queue's place in [`QUEUES`], and the root +/// table its context entries live in. +pub(super) struct Live { + queue: usize, + root: Table, /// [`Unit::pci_all`]. pci_all: bool, /// [`Unit::hierarchies`]. @@ -109,22 +138,20 @@ pub(super) struct Live { impl Live { pub(super) fn invalidate_domain(&mut self, domain: u16) { - self.queue.invalidate_domain(self.regs, domain); + with_queue(self.queue, |unit| unit.queue.invalidate_domain(domain)); } /// Moves `stream` onto `domain` in this unit's tables, and out of /// whatever this unit still holds for it under its old one. pub(super) fn attach(&mut self, stream: StreamId, id: u16, space: table::Space) { let displaced = table::bind(&mut TABLES.lock(), self.root, stream, id, space); - self.queue.invalidate_context(self.regs, displaced); + with_queue(self.queue, |unit| unit.queue.invalidate_context(displaced)); } } /// Every remapping unit's interrupt-entry cache, gone. pub(super) fn invalidate_interrupt_entries() { - for unit in UNITS.lock().iter_mut().filter(|u| u.remaps) { - unit.queue.invalidate_interrupts(unit.regs); - } + remapping_queues(Queue::invalidate_interrupts); } /// Every region firmware reserved and the interrupt range are what no domain's @@ -464,12 +491,12 @@ impl Unit { self.command(QUEUED_INVALIDATION_ENABLE, true, "queued invalidation"); self.regs.write_u64(RTADDR_REG, root.phys()); self.command(SET_ROOT_TABLE_POINTER, true, "the root table pointer"); - queue.invalidate_all(self.regs); + queue.invalidate_all(); self.command(TRANSLATION_ENABLE, true, "translation"); if self.caps.interrupt_remapping() { self.regs.write_u64(interrupt::IRTA_REG, remap.phys()); self.command(interrupt::SET_TABLE_POINTER, true, "the interrupt remap table pointer"); - queue.invalidate_interrupts(self.regs); + queue.invalidate_interrupts(); self.command(interrupt::INTERRUPT_REMAPPING_ENABLE, true, "interrupt remapping"); let status = self.regs.read_u32(GSTS_REG) & PERSISTENT; self.regs.write_u32(GCMD_REG, status | interrupt::COMPATIBILITY_FORMAT); @@ -676,7 +703,7 @@ fn translate(unit: Unit, plan: Plan, root: Table, via: Invalidation) -> Translat unit.regs.write_u64(RTADDR_REG, root.phys()); unit.command(SET_ROOT_TABLE_POINTER, true, "the root table pointer"); match queue.as_mut() { - Some(queue) => queue.invalidate_all(unit.regs), + Some(queue) => queue.invalidate_all(), None => unit.invalidate_through_registers(), } unit.command(TRANSLATION_ENABLE, true, "translation"); @@ -701,12 +728,21 @@ fn finish(translating: Translating, devices: &[PciDevice], remap: Option) if let Some(irta) = irta { unit.regs.write_u64(interrupt::IRTA_REG, irta); unit.command(interrupt::SET_TABLE_POINTER, true, "the interrupt remap table pointer"); - queue.invalidate_interrupts(unit.regs); + queue.invalidate_interrupts(); unit.command(interrupt::INTERRUPT_REMAPPING_ENABLE, true, "interrupt remapping"); } // After `IRE`, so nothing is invalidated on a unit that is not yet reading // the table, and after `TE`, so nothing reaches a unit that is not on. - UNITS.lock().push(Live { regs: unit.regs, queue, root, remaps: remap.is_some(), pci_all: unit.pci_all, hierarchies: unit.hierarchies.clone() }); + { + let mut units = UNITS.lock(); + let at = PUBLISHED.load(Ordering::Relaxed); + { + let closed = crate::arch::IrqGuard::close(); + *QUEUES[at].lock(&closed) = Some(Published { queue, remaps: remap.is_some() }); + } + PUBLISHED.store(at + 1, Ordering::Release); + units.push(Live { queue: at, root, pci_all: unit.pci_all, hierarchies: unit.hierarchies.clone() }); + } let gsts = unit.regs.read_u32(GSTS_REG); log!( diff --git a/kernel/src/arch/x86_64/vtd/queue.rs b/kernel/src/arch/x86_64/vtd/queue.rs index 22230e98582..11b857fffea 100644 --- a/kernel/src/arch/x86_64/vtd/queue.rs +++ b/kernel/src/arch/x86_64/vtd/queue.rs @@ -18,6 +18,10 @@ const CONTEXT_CACHE: u64 = 0x1; const IOTLB: u64 = 0x2; /// Type 4h, Section 6.5.2.7 — granularity inverted against the two above: clear is global. const INTERRUPT_ENTRY_CACHE: u64 = 0x4; +/// `G`, bit 4 of an interrupt-entry-cache descriptor: set, one index (§6.5.2.7). +const INDEX_SELECTIVE: u64 = 1 << 4; +/// `IIDX`, bits 47:32; `IM` 31:27 left 0 names that index alone. +const INDEX_SHIFT: u64 = 32; const WAIT: u64 = 0x5; /// Granularity field, bits 5:4: `01` is global — every domain, every entry. @@ -48,6 +52,7 @@ const ACK_TIMEOUT: Tripwire = Tripwire::absurd( ); pub struct Queue { + regs: Mmio, descriptors: Table, /// Own page: the ring uses its full 4 KiB, so an inline status word would collide with a descriptor slot. status: Table, @@ -57,7 +62,7 @@ pub struct Queue { impl Queue { /// The caller enables `GCMD.QIE` only after this returns, since a queue the unit has not been pointed at is a queue it never reads. pub fn new(tables: &mut Tables, regs: Mmio) -> Self { - let queue = Self { descriptors: tables.alloc(), status: tables.alloc(), tail: 0 }; + let queue = Self { regs, descriptors: tables.alloc(), status: tables.alloc(), tail: 0 }; // Descriptor width 0, queue size 0: the register is the base address alone. regs.write_u64(IQA_REG, queue.descriptors.phys()); regs.write_u64(IQT_REG, 0); @@ -66,58 +71,43 @@ impl Queue { /// Every cached translation and context entry, gone, acknowledged by the /// unit before this returns. - pub fn invalidate_all(&mut self, regs: Mmio) { - self.submit( - regs, - &[ - (CONTEXT_CACHE | GLOBAL, 0), - (IOTLB | GLOBAL | DRAIN_WRITES | DRAIN_READS, 0), - ], - ); + pub fn invalidate_all(&mut self) { + self.submit(&[(CONTEXT_CACHE | GLOBAL, 0), (IOTLB | GLOBAL | DRAIN_WRITES | DRAIN_READS, 0)]); } /// Every translation cached for one domain, gone. Owed after any change to /// its second-level tables, a mapping becoming present included, because /// `CAP.CM` says the unit may have cached the absence. - pub fn invalidate_domain(&mut self, regs: Mmio, domain: u16) { - self.submit( - regs, - &[( - IOTLB | DOMAIN | DRAIN_WRITES | DRAIN_READS - | ((domain as u64) << DOMAIN_SHIFT), - 0, - )], - ); + pub fn invalidate_domain(&mut self, domain: u16) { + self.submit(&[(IOTLB | DOMAIN | DRAIN_WRITES | DRAIN_READS | ((domain as u64) << DOMAIN_SHIFT), 0)]); } /// A replaced context entry, then everything cached under the domain it /// named, in Section 6.5.2.1's order: the unit looks a translation up by /// requester and keeps it under that id (Section 6.2.1). - pub fn invalidate_context(&mut self, regs: Mmio, displaced: Displaced) { + pub fn invalidate_context(&mut self, displaced: Displaced) { let domain = (displaced.domain() as u64) << DOMAIN_SHIFT; - self.submit( - regs, - &[ - ( - CONTEXT_CACHE - | DEVICE - | domain - | ((displaced.requester() as u64) << SOURCE_SHIFT), - 0, - ), - (IOTLB | DOMAIN | DRAIN_WRITES | DRAIN_READS | domain, 0), - ], - ); + self.submit(&[ + (CONTEXT_CACHE | DEVICE | domain | ((displaced.requester() as u64) << SOURCE_SHIFT), 0), + (IOTLB | DOMAIN | DRAIN_WRITES | DRAIN_READS | domain, 0), + ]); } /// Every cached interrupt remapping entry, gone. Owed after `SIRTP` where /// `CAP.ESIRTPS` is clear (Section 6.7); sent only to a unit that remaps, /// since a type it does not implement stalls its queue. - pub fn invalidate_interrupts(&mut self, regs: Mmio) { - self.submit(regs, &[(INTERRUPT_ENTRY_CACHE, 0)]); + pub fn invalidate_interrupts(&mut self) { + self.submit(&[(INTERRUPT_ENTRY_CACHE, 0)]); + } + + /// Interrupt remapping entry `index`, gone from the cache, and every + /// message the unit already remapped through it delivered (§6.5.2.7). + pub fn invalidate_interrupt(&mut self, index: usize) { + self.submit(&[(INTERRUPT_ENTRY_CACHE | INDEX_SELECTIVE | (index as u64) << INDEX_SHIFT, 0)]); } - fn submit(&mut self, regs: Mmio, descriptors: &[(u64, u64)]) { + fn submit(&mut self, descriptors: &[(u64, u64)]) { + let regs = self.regs; assert!( descriptors.len() + 1 < QUEUE_ENTRIES, "iommu: {} descriptors do not fit a {QUEUE_ENTRIES}-entry queue", diff --git a/kernel/src/arch/x86_64/vtd/table.rs b/kernel/src/arch/x86_64/vtd/table.rs index a16fd3f530b..e5bc5c26e9e 100644 --- a/kernel/src/arch/x86_64/vtd/table.rs +++ b/kernel/src/arch/x86_64/vtd/table.rs @@ -112,6 +112,11 @@ impl Table { self.write(index * 2, lo); } + /// Rewrites only the low half of the 16-byte entry at `index`. + pub fn write_low(self, index: usize, lo: u64) { + self.write(index * 2, lo); + } + /// Zeroes a 16-byte entry low half first: the low half holds `P`, and an /// entry whose high half went first would for a moment be present over a /// field it no longer means. diff --git a/kernel/src/iommu/mod.rs b/kernel/src/iommu/mod.rs index e9fb3037b2b..d272a6e6649 100644 --- a/kernel/src/iommu/mod.rs +++ b/kernel/src/iommu/mod.rs @@ -306,6 +306,11 @@ impl Remapped { self.claimed.interrupt() } + /// What masks the message, for as long as this is held. + pub fn line(&self) -> MessageLine { + MessageLine(self.claimed.line()) + } + fn stream(&self) -> StreamId { StreamId::pci(self.function.bus, self.function.dev, self.function.func) } @@ -317,6 +322,21 @@ impl Drop for Remapped { } } +/// What masks a claimed function's message at the unit that translates it: +/// its remapping entry, or its LPI. Valid while its [`Remapped`] is held. +#[derive(Clone, Copy)] +pub struct MessageLine(messages::Line); + +impl MessageLine { + /// The function's messages refused, or delivered again, before this + /// returns: a refused one on x86-64 is dropped at the unit and recorded + /// nowhere, and on AArch64 left pending until it is delivered. Takes only + /// the units' invalidation queues, so an interrupt handler may call it. + pub fn set_masked(self, masked: bool) { + messages::set_masked(self.0, masked); + } +} + /// A message for `pcidev` function `number`, raising an interrupt taken for /// it and translated for `function` alone. pub fn claim_msi( diff --git a/kernel/src/isa.rs b/kernel/src/isa.rs index b8b65c38f48..aa25d9bc3b9 100644 --- a/kernel/src/isa.rs +++ b/kernel/src/isa.rs @@ -250,7 +250,7 @@ pub fn take_record(row: &Row) -> Option { } pub fn has_irq(row: &Row) -> bool { - IRQ[row.0].armed() + IRQ[row.0].pending() } /// Records one interrupt, masks the row's level lines and posts the claim's diff --git a/kernel/src/pcidev/mod.rs b/kernel/src/pcidev/mod.rs index 88a8e727cd3..71e317f2704 100644 --- a/kernel/src/pcidev/mod.rs +++ b/kernel/src/pcidev/mod.rs @@ -136,12 +136,12 @@ use toyos_pci::{af, aperture, bar, express, hotplug, pages, pm}; use crate::device::{Claim, ClaimError}; use crate::drivers::pci::{arm_claimed_msi, arm_claimed_msix, mask_claimed_msix, NoCapability, NoEntry, PciDevice}; -use crate::iommu::{IommuError, OwnSpace, Refused, Remapped}; +use crate::iommu::{IommuError, MessageLine, OwnSpace, Refused, Remapped}; use kernel::claimspace::{Span, Window}; use crate::mm::policy::CachePolicy; use crate::mm::align_2m; use crate::object::shm::SharedMemObject; -use crate::sync::Lock; +use crate::sync::{Lock, Masked}; use crate::inbox::PollEntry; use crate::watch::IrqWatch; @@ -189,6 +189,8 @@ struct Function { /// its fault is never this kernel's ([`note_fault`]). claimed: AtomicBool, irq: Interrupt, + /// What masks its claim's message while [`Self::irq`] holds a count. + gate: Gate, /// What its holder's poll waits on: two processes each driving a function /// must not learn when the other's device is busy. watch: IrqWatch, @@ -249,6 +251,7 @@ fn new_record(pci: PciDevice, arrival: Arrival) -> Function { held: AtomicBool::new(false), claimed: AtomicBool::new(false), irq: Interrupt::new(), + gate: Gate(Masked::new(None)), watch: IrqWatch::new(), space: Lock::new(None), bound: Lock::new(None), @@ -1044,7 +1047,9 @@ pub fn claim(arrival: Arrival, firmware: FirmwareWindows) -> Result<(PciFunction bound.armed.message().interrupt(), ); let function = binding.function(); + let line = bound.armed.message().line(); *function.bound.lock() = Some(bound); + function.gate.arm(line); function.irq.clear(); changed(); Ok((info, Claim::pci(binding))) @@ -1522,6 +1527,7 @@ fn release(number: usize, arrival: Arrival) { /// that maps nothing and a message that is not present. fn tear_down_departed(function: &Function, bound: Bound) { let Bound { space, armed, grants, id, .. } = bound; + function.gate.disarm(); drop(armed); for grant in grants.iter() { if let Err(why) = space.unmap(grant.at, grant.bytes) { @@ -1586,6 +1592,7 @@ fn tear_down(function: &Function, mut bound: Bound) { // Once the function no longer speaks, and before it can be claimed again: // its translation goes, and its interrupt is free. bound.pci.drain_messages(); + function.gate.disarm(); drop(message); // **The reset before the domain gives anything back.** With mastering off // the function starts nothing new; its grants stay mapped until it is @@ -1970,7 +1977,8 @@ pub fn config_read(binding: &Binding, at: Register, width: RegWidth) -> Result Result, SyscallError> { @@ -1981,7 +1989,7 @@ pub fn take_record(binding: &Binding) -> Result, Syscall if function.irq.faulted() { return Err(SyscallError::Io); } - let taken = function.irq.take(); + let taken = function.irq.take_opening(&function.gate); if taken.is_some() && function.irq.take_unannounced() { let bound = function.bound.lock(); let bound = bound.as_ref().expect("pcidev: a claim's binding has its function bound"); @@ -1994,13 +2002,15 @@ pub fn take_record(binding: &Binding) -> Result, Syscall /// refusal is. pub fn has_irq(binding: &Binding) -> bool { let function = binding.function(); - !binding.here() || function.irq.armed() || function.irq.faulted() + !binding.here() || function.irq.pending() || function.irq.faulted() } -/// Records one message and posts the claim's watch. Called from the +/// Records one message, masks the claim's message until its holder reads +/// the record, and posts the claim's watch: a function interrupts at most +/// once per read, whatever its holder makes it send. Called from the /// interrupt's handler for function `number`, so it allocates nothing; -/// `record.rs` owns the counting, and `kernel-loom` models it against a -/// concurrent reader. +/// `record.rs` owns the counting and the mask's rule, and `kernel-loom` +/// models both against a concurrent reader. /// /// **The handler holds the preempt count raised across it**: the post lets go /// of a count of its own, and at depth zero that would run a scheduler pass @@ -2011,10 +2021,49 @@ pub fn isr(number: usize) { if let Some(slot) = slot::of(function) { return slot.isr(); } - function.irq.took(crate::clock::nanos_since_boot()); + function.irq.took_masking(crate::clock::nanos_since_boot(), &function.gate); function.watch.post_in_place(); } +/// A claim's message line and whether it is masked, while the claim holds +/// it. A leaf but for the units' invalidation queues, taken with interrupts +/// masked: the handler takes it too. +struct Gate(Masked>); + +struct Gated { + line: MessageLine, + masked: bool, +} + +impl Gate { + /// A claim's line, which its arming left delivering. + fn arm(&self, line: MessageLine) { + let closed = crate::arch::IrqGuard::close(); + let previous = self.0.lock(&closed).replace(Gated { line, masked: false }); + assert!(previous.is_none(), "pcidev: a claim's line was armed over another's"); + } + + /// Before the claim's message is given back, so nothing masks an entry + /// another claim may be given. + fn disarm(&self) { + let closed = crate::arch::IrqGuard::close(); + self.0.lock(&closed).take(); + } +} + +impl record::Gate for Gate { + fn hold(&self, masked: impl FnOnce() -> bool) { + let closed = crate::arch::IrqGuard::close(); + let mut held = self.0.lock(&closed); + let Some(gated) = held.as_mut() else { return }; + let masked = masked(); + if masked != gated.masked { + gated.line.set_masked(masked); + gated.masked = masked; + } + } +} + /// The unit refused requester `who` an access: `true` where a claim holds it, /// a claim held it, or it has left the device set, so that no driver of this /// kernel's is to blame, and its bus mastering is then cleared. diff --git a/kernel/src/pcidev/record.rs b/kernel/src/pcidev/record.rs index 47694d97228..9db1be3974a 100644 --- a/kernel/src/pcidev/record.rs +++ b/kernel/src/pcidev/record.rs @@ -27,6 +27,14 @@ //! the first can take read `n + 1` and let read `n + 2`'s first message into //! the slot the first is loading; that costs the racing holder its own time, //! never a count. +//! +//! **A claim's interrupt is masked while a count waits** ([`Gate`]): the ISR +//! masks it after it counts, and the holder's read opens it after it takes, so +//! a function interrupts at most once per read. Each side changes the count +//! first and then, under the gate's lock, puts hardware in the state the +//! record then owes, read from the word and not from its own change: the side +//! that takes the lock last sees both changes, so a mask that lands after the +//! read that opened the gate cannot leave a claim masked with nothing to read. #[cfg(not(feature = "loom"))] use core::sync::atomic::{AtomicBool, AtomicU64, Ordering}; @@ -177,10 +185,32 @@ impl Interrupt { /// Whether a message is waiting, for a readiness check that consumes /// nothing. - pub fn armed(&self) -> bool { + pub fn pending(&self) -> bool { self.word.load(ORDER) & COUNT != 0 } + /// [`Self::took`], then the claim's interrupt masked behind `gate`. + pub fn took_masking(&self, now: u64, gate: &impl Gate) { + self.took(now); + #[cfg(feature = "device-irq-gate-intent")] + gate.hold(|| true); + #[cfg(not(feature = "device-irq-gate-intent"))] + gate.hold(|| self.pending()); + } + + /// [`Self::take`], then the claim's interrupt opened behind `gate` where + /// the read took something. + pub fn take_opening(&self, gate: &impl Gate) -> Option { + let taken = self.take(); + if taken.is_some() { + #[cfg(feature = "device-irq-gate-intent")] + gate.hold(|| false); + #[cfg(not(feature = "device-irq-gate-intent"))] + gate.hold(|| self.pending()); + } + taken + } + /// Whether this is the first message this claim has taken. Answers `true` /// once per claim and `false` ever after, so a caller may log on it. /// @@ -208,3 +238,11 @@ impl Interrupt { self.unannounced.store(true, ORDER); } } + +/// What masks a claim's interrupt in hardware, behind a lock both sides of +/// its record take. +pub trait Gate { + /// Under the lock, put hardware in the state `masked` answers, where it + /// is not already in it. + fn hold(&self, masked: impl FnOnce() -> bool); +} diff --git a/sdk/abi/src/pci.rs b/sdk/abi/src/pci.rs index a1629f7d5a1..af8952e88f1 100644 --- a/sdk/abi/src/pci.rs +++ b/sdk/abi/src/pci.rs @@ -204,6 +204,11 @@ const _: () = assert!(core::mem::size_of::() == 8 + 8); /// slow reader to overflow. The count says how many messages arrived, never /// what any of them meant: that is in the device's own rings. /// +/// **A claimed PCI function's interrupt is masked from a message until the +/// read that takes it**, so it interrupts at most once per read: a message +/// sent meanwhile is refused at the unit on x86-64, and left pending until +/// the read on AArch64. A count above one is only what was in flight. +/// /// **The two times are when messages landed, which a driver reading its rings /// later cannot recover**: a driver clocked by its device — soundserver's DLL — /// is clocked by these and not by when it woke. Both are nanoseconds since diff --git a/sdk/toyos/src/device.rs b/sdk/toyos/src/device.rs index 56b02ded5cb..852235163ab 100644 --- a/sdk/toyos/src/device.rs +++ b/sdk/toyos/src/device.rs @@ -192,6 +192,11 @@ impl PciDev { } /// The interrupts since the last read, or `Err(WouldBlock)` for none. + /// + /// **Take the record before servicing the device.** From a message to the + /// read that takes it, the function's interrupt is masked, and what the + /// device signals meanwhile raises nothing: a driver that serviced first + /// and read after could miss an event that arrived in between. pub fn irq(&self) -> Result { irq_record(&self.0) } From 29ffc6521d23f6a3ab0398c0eee11f5e176b2e2e Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 5 Nov 2026 14:27:28 +0100 Subject: [PATCH 02/12] i219_msi: a storm of forced causes stops at the claim's entry 10,000 RXSEQ causes forced back to back with ITR off and the record left unread, each read off ICR as asserted: at most 64 messages may land, and the next forced cause after the read is exactly one. Without the mask every cause lands, so the kernel change reverted reds it. Co-Authored-By: Claude Opus 5.5 --- tests/guest/rust-tests/src/bin/i219_msi.rs | 45 +++++++++++++++++++++- tests/toyos.rs | 6 ++- 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/tests/guest/rust-tests/src/bin/i219_msi.rs b/tests/guest/rust-tests/src/bin/i219_msi.rs index 78382fa9033..5eb2a0eaf01 100644 --- a/tests/guest/rust-tests/src/bin/i219_msi.rs +++ b/tests/guest/rust-tests/src/bin/i219_msi.rs @@ -1,8 +1,8 @@ //! The T14's I219, judged on what `toyos-i219` rests on where its interrupts //! are concerned, on one claim: a capability list with MSI and no MSI-X, //! causes forced through `ICS` counted on the claim's vector unmasked and -//! masked, an asserted read of `ICR` under auto-mask, and then the driver -//! itself under traffic. Every reading is printed; the exit is whether each +//! masked, a storm of them with the record left unread, an asserted read of +//! `ICR` under auto-mask, and then the driver itself under traffic. Every reading is printed; the exit is whether each //! held. //! //! The PCH datasheet (631120 §8.1) and the 82574's (§10.2.4) are what each @@ -36,6 +36,15 @@ const RXSEQ: u32 = 1 << 3; const FORCES: u32 = 4; /// How long a forced cause is given to arrive, and a masked one to stay away. const WINDOW_NANOS: u64 = toyos_i219::DELIVERY_DEADLINE_NANOS; +/// Causes forced back to back with the claim's record left unread: a holder +/// making its function speak as fast as its own register writes go. +const STORM: u32 = 10_000; +/// The most of them that may land: the first masks the claim's entry, and +/// what follows is only what was already past the unit when the handler's +/// invalidation completed (VT-d §6.5.2.7 drains it). The handler finishes +/// within tens of microseconds, and each forced cause here is a register +/// write and read, so a few dozen at most; unmasked, every one lands. +const STORM_BOUND: u32 = 64; /// How long the link is given to come up after the driver's reset. const LINK_NANOS: u64 = 15_000_000_000; /// How long frames are listened for after the DHCP discover. @@ -140,6 +149,24 @@ fn force(regs: &R, claim: &Claim, forced: u32, unmask: u32) -> (u3 (count, after, icr) } +/// `STORM` causes forced with `ITR` off and the record unread, each read off +/// `ICR` as asserted, then the messages that landed: the claim's interrupt +/// is masked from the first until this read. +fn storm(regs: &R, claim: &Claim) -> (u32, u32) { + regs.write32(regs::IMC, u32::MAX); + regs.write32(regs::ITR, 0); + let _ = regs.read32(regs::ICR); + let _ = claim.taken(); + regs.write32(regs::IMS, RXSEQ); + let mut asserted = 0; + for _ in 0..STORM { + regs.write32(regs::ICS, RXSEQ); + asserted += u32::from(regs.read32(regs::ICR) & (cause::INT_ASSERTED | RXSEQ) == cause::INT_ASSERTED | RXSEQ); + } + regs.write32(regs::IMC, u32::MAX); + (asserted, messages(claim, WINDOW_NANOS)) +} + /// One read of `ICR` with an interrupt asserted under auto-mask, as every /// pass of the driver makes it: the read, `IMS` after it, and a second read. /// §10.2.4.1's case 2 says the read clears `ICR` and writes `IAM` into `IMC`. @@ -290,6 +317,20 @@ fn main() { fail("a forced cause was not exactly one message unmasked and none masked"); } + let (asserted, landed) = storm(®s, &claim); + say(&format!("storm: {asserted} of {STORM} forced causes asserted, {landed} message(s) landed unread")); + if asserted != STORM { + fail("the storm did not assert every cause it forced"); + } + if landed == 0 || landed > STORM_BOUND { + fail(&format!("a storm landed {landed} message(s) on an unread claim, outside 1..={STORM_BOUND}")); + } + let (count, _, _) = force(®s, &claim, RXSEQ, RXSEQ); + say(&format!("after the storm's read, RXSEQ forced unmasked: {count} message(s)")); + if count != 1 { + fail("the read that took the storm's record did not open the claim's interrupt again"); + } + let (first, mask, second) = auto_masked_read(®s, &claim); say(&format!( "IAME on: the asserted ICR read {first:#010x}, IMS after it {mask:#010x}, the second read {second:#010x}" diff --git a/tests/toyos.rs b/tests/toyos.rs index 7df43da25b1..3711286314d 100644 --- a/tests/toyos.rs +++ b/tests/toyos.rs @@ -1546,8 +1546,10 @@ const METAL: &[(&str, metal::Metal)] = &[ ( // The I219's interrupts on the function itself, which QEMU's e1000e // is not: no MSI-X capability, causes forced through `ICS` counted on - // the claim's MSI vector unmasked and masked, `ICR`'s asserted read - // under auto-mask, and the driver under traffic on the bench's wire. + // the claim's MSI vector unmasked and masked, a storm of them stopped + // at the claim's entry until its record is read, `ICR`'s asserted + // read under auto-mask, and the driver under traffic on the bench's + // wire. // It shares the boot with `claim_reuses_its_remapping_entry`, whose // judge reads only its own job's records. "i219_messages_on_its_msi_vector", From 77f7a77d9175eddcd249c47af01774fc0f1b3f24 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 5 Nov 2026 14:57:04 +0100 Subject: [PATCH 03/12] amdvi: a unit waiting for its SMI filter is a named type Clippy refused the tuple as too complex. Co-Authored-By: Claude Opus 5.5 --- kernel/src/arch/x86_64/amdvi/mod.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/kernel/src/arch/x86_64/amdvi/mod.rs b/kernel/src/arch/x86_64/amdvi/mod.rs index ade626ea697..6e8cd000af5 100644 --- a/kernel/src/arch/x86_64/amdvi/mod.rs +++ b/kernel/src/arch/x86_64/amdvi/mod.rs @@ -120,7 +120,14 @@ static UNITS: [Masked>; MAX_UNITS] = [const { Masked::new(None) }; static PUBLISHED: AtomicUsize = AtomicUsize::new(0); /// Each translating unit's SMI filter, until [`settle`] writes it. -static SMI: Lock>)>> = Lock::new(Vec::new()); +static SMI: Lock> = Lock::new(Vec::new()); + +struct Unfiltered { + index: usize, + regs: Mmio, + /// [`Ready::smi`]. + filter: Option>, +} /// `bytes` at `phys`, through the direct map. fn memory(phys: u64, bytes: u64) -> Mmio { @@ -598,7 +605,7 @@ fn program(unit: Ready, devices: &[PciDevice], remap: Option) { *UNITS[at].lock(&closed) = Some(live); } PUBLISHED.store(at + 1, Ordering::Release); - settling.push((index, regs, smi)); + settling.push(Unfiltered { index, regs, filter: smi }); } /// Every unit's SMI filter on, once every function this kernel drives is out @@ -607,8 +614,8 @@ fn program(unit: Ready, devices: &[PciDevice], remap: Option) { pub fn settle() { let mut units = SMI.lock(); let mut everywhere = !units.is_empty(); - for (index, regs, smi) in units.iter_mut() { - let Some(filter) = smi.take() else { + for Unfiltered { index, regs, filter } in units.iter_mut() { + let Some(filter) = filter.take() else { everywhere = false; continue; }; From b23299d8be6218c62aa29a0e343e2707aa607f16 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 5 Nov 2026 18:05:58 +0100 Subject: [PATCH 04/12] review r1 of #1193: the storm bound derived, the issue narrowed, the count cleared before the gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - i219_msi's storm bound is 2: the handler masks and drains before its end of interrupt, and a local APIC holds one request per vector beside the one in service (Intel SDM Vol. 3A §11.8.4). Unmasked, every forced cause lands here, as the A arm measured (10,000 of 10,000): the ICR read between forces outlasts the handler. - issues/a-claimed-function-can-storm-its-cpu.md stays open, narrowed to what is left: a holder that loops force-and-read paces cpu0 and its invalidation waits, a refused message is counted nowhere, and no T14 row shows cpu0's timer served under such a holder. - A claim clears its record before it arms the gate, so the order no longer rests on mastering being off. - The gate's loom header is one sentence. Co-Authored-By: Claude Opus 5.5 --- .../a-claimed-function-can-storm-its-cpu.md | 35 +++++++++++++++++++ kernel/loom/tests/device_irq_gate.rs | 21 ++--------- kernel/src/pcidev/mod.rs | 2 +- tests/guest/rust-tests/src/bin/i219_msi.rs | 13 +++---- 4 files changed, 46 insertions(+), 25 deletions(-) create mode 100644 issues/a-claimed-function-can-storm-its-cpu.md diff --git a/issues/a-claimed-function-can-storm-its-cpu.md b/issues/a-claimed-function-can-storm-its-cpu.md new file mode 100644 index 00000000000..5d7344bd7cc --- /dev/null +++ b/issues/a-claimed-function-can-storm-its-cpu.md @@ -0,0 +1,35 @@ +--- +status: open +kind: defect +severity: [kernel-from-userland] +opened: 2026-10-04 +--- + +# A claimed function can storm its CPU + +A claim's message is masked from the message its handler takes until its +holder reads the record (`kernel/src/pcidev/record.rs`), so a function +interrupts at most once per read. That bounds it per read and not per unit of +time, and three things remain. + +**A holder paces cpu0.** A holder that loops on forcing a cause through its +own registers and reading its record, on any CPU, still raises one cpu0 +interrupt per iteration. Each one now also costs cpu0 an invalidation +completion wait with interrupts off: VT-d's interrupt-entry-cache +invalidation, AMD-Vi's completion wait, or the ITS's `SYNC`. Both are charged +to whatever cpu0 interrupted, not to the holder. + +**A refused message is recorded nowhere.** While masked, a message is target +aborted (AMD-Vi) or blocked with fault processing disabled (VT-d), and +neither the unit nor the claim's record counts it. On AArch64 a disabled LPI +stays pending and folds into one. + +**No T14 row shows cpu0's other sources served under a holder that +provokes its function.** `i219_msi`'s storm phase shows a forced storm on +an unread claim stopped at the entry, and not the timer. + +Owner: the IOMMU track. Exit: cpu0's interrupts from a claim are bounded in +time, and the work they cost is charged to their holder, by a bound the +kernel derives and states; a message refused past it is counted against +the claim; and a T14 row with a holder that loops force-and-read shows +cpu0's timer and other sources still served. diff --git a/kernel/loom/tests/device_irq_gate.rs b/kernel/loom/tests/device_irq_gate.rs index 0f8fd57e391..ee85c90d5c7 100644 --- a/kernel/loom/tests/device_irq_gate.rs +++ b/kernel/loom/tests/device_irq_gate.rs @@ -1,21 +1,6 @@ -//! Loom: a claimed function's interrupt, masked while its record holds a -//! count and opened by the read that takes it (`pcidev/record.rs`'s [`Gate`]). -//! -//! **The invariant is that hardware ends in the state the record owes it: -//! masked exactly while a count waits.** A claim left masked with nothing to -//! read never interrupts again, and its driver waits for a device that has -//! spoken; one left open with a count waiting is the storm the mask exists to -//! stop. What makes it hold is that each side reads the record under the -//! gate's lock after its own change to the count, so the negative control is -//! each side applying its own intent instead (the ISR masks, the read opens), -//! where a mask that lands after the read opened the gate strands the claim: -//! -//! ```text -//! cargo test --manifest-path kernel/loom/Cargo.toml --features device-irq-gate-intent \ -//! --test device_irq_gate -//! ``` -//! -//! and [`hardware_ends_as_the_record_owes`] must red. +//! Loom: a claimed function's interrupt ends masked exactly while its record +//! holds a count, however the handler and a read interleave +//! (`pcidev/record.rs`); the `device-irq-gate-intent` control must red it. use kernel_loom::device_irq::{Gate, Interrupt}; use loom::sync::{Arc, Mutex}; diff --git a/kernel/src/pcidev/mod.rs b/kernel/src/pcidev/mod.rs index 71e317f2704..a6ddf13c9b8 100644 --- a/kernel/src/pcidev/mod.rs +++ b/kernel/src/pcidev/mod.rs @@ -1049,8 +1049,8 @@ pub fn claim(arrival: Arrival, firmware: FirmwareWindows) -> Result<(PciFunction let function = binding.function(); let line = bound.armed.message().line(); *function.bound.lock() = Some(bound); - function.gate.arm(line); function.irq.clear(); + function.gate.arm(line); changed(); Ok((info, Claim::pci(binding))) } diff --git a/tests/guest/rust-tests/src/bin/i219_msi.rs b/tests/guest/rust-tests/src/bin/i219_msi.rs index 5eb2a0eaf01..0c82b51bc83 100644 --- a/tests/guest/rust-tests/src/bin/i219_msi.rs +++ b/tests/guest/rust-tests/src/bin/i219_msi.rs @@ -39,12 +39,13 @@ const WINDOW_NANOS: u64 = toyos_i219::DELIVERY_DEADLINE_NANOS; /// Causes forced back to back with the claim's record left unread: a holder /// making its function speak as fast as its own register writes go. const STORM: u32 = 10_000; -/// The most of them that may land: the first masks the claim's entry, and -/// what follows is only what was already past the unit when the handler's -/// invalidation completed (VT-d §6.5.2.7 drains it). The handler finishes -/// within tens of microseconds, and each forced cause here is a register -/// write and read, so a few dozen at most; unmasked, every one lands. -const STORM_BOUND: u32 = 64; +/// The most of them that may land: the first masks the claim's entry and +/// drains what the unit already remapped before its end of interrupt, and a +/// local APIC holds one request per vector beside the one in service, so +/// whatever was drained is one more delivery (Intel SDM Vol. 3A §11.8.4, +/// "Interrupt Acceptance for Fixed Interrupts"). Unmasked, every one lands: +/// the `ICR` read between forces outlasts the handler. +const STORM_BOUND: u32 = 2; /// How long the link is given to come up after the driver's reset. const LINK_NANOS: u64 = 15_000_000_000; /// How long frames are listened for after the DHCP discover. From 8e07c9ec809df88c06466c93ab5403e6c2041fb9 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 5 Nov 2026 18:17:27 +0100 Subject: [PATCH 05/12] A claim's interrupt is masked from the second message before a read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Masking on every delivery cost each device interrupt a remapping-entry write and an invalidation wait, +33 us median on the T14's I219 (A/B r1). The gate now masks only when a message lands while the record still holds an unread count, so a driver that reads between messages never pays it, and a storm between two reads stops at what lands before the mask takes: the second message masks, and its handler's drain leaves at most one more request in the local APIC (Intel SDM Vol. 3A, 325384-093, §13.8.4). The gate, the backends and the loom model's shape are unchanged; the owed state is `count >= MASKED_FROM`, read by both sides under the gate. i219_msi's storm bound is 3 accordingly. The citation in the previous commit's message, §11.8.4, was that section's number in older revisions. Co-Authored-By: Claude Opus 5.5 --- build/ci.rs | 1 - .../a-claimed-function-can-storm-its-cpu.md | 17 ++++----- kernel/loom/tests/device_irq_gate.rs | 19 +++++----- kernel/src/pcidev/mod.rs | 7 ++-- kernel/src/pcidev/record.rs | 36 +++++++++++++------ sdk/abi/src/pci.rs | 9 ++--- sdk/toyos/src/device.rs | 9 ++--- tests/guest/rust-tests/src/bin/i219_msi.rs | 10 +++--- 8 files changed, 64 insertions(+), 44 deletions(-) diff --git a/build/ci.rs b/build/ci.rs index cfd6062fd94..32a8297bd99 100644 --- a/build/ci.rs +++ b/build/ci.rs @@ -607,7 +607,6 @@ pub(crate) const CONTROLS: &[Control] = &[ Fails("every_message_is_counted_once"), ]), red(KERNEL_LOOM, "device-irq-gate-intent", Some("device_irq_gate"), &[ - Fails("hardware_ends_as_the_record_owes"), Fails("hardware_ends_as_the_record_owes_after_two_messages"), ]), Control { diff --git a/issues/a-claimed-function-can-storm-its-cpu.md b/issues/a-claimed-function-can-storm-its-cpu.md index 5d7344bd7cc..304c57a8f1a 100644 --- a/issues/a-claimed-function-can-storm-its-cpu.md +++ b/issues/a-claimed-function-can-storm-its-cpu.md @@ -7,17 +7,18 @@ opened: 2026-10-04 # A claimed function can storm its CPU -A claim's message is masked from the message its handler takes until its -holder reads the record (`kernel/src/pcidev/record.rs`), so a function -interrupts at most once per read. That bounds it per read and not per unit of -time, and three things remain. +A claim's message is masked from the second message that lands before its +holder reads the record until that read (`kernel/src/pcidev/record.rs`), so +what lands between two reads stops at what arrives before the mask takes. +That bounds it per read and not per unit of time, and three things remain. **A holder paces cpu0.** A holder that loops on forcing a cause through its own registers and reading its record, on any CPU, still raises one cpu0 -interrupt per iteration. Each one now also costs cpu0 an invalidation -completion wait with interrupts off: VT-d's interrupt-entry-cache -invalidation, AMD-Vi's completion wait, or the ITS's `SYNC`. Both are charged -to whatever cpu0 interrupted, not to the holder. +interrupt per iteration; one that forces two per read also costs cpu0 an +invalidation completion wait with interrupts off, at the mask and again at +its read (VT-d's interrupt-entry-cache invalidation, AMD-Vi's completion +wait, or the ITS's `SYNC`). All of it is charged to whatever cpu0 +interrupted, not to the holder. **A refused message is recorded nowhere.** While masked, a message is target aborted (AMD-Vi) or blocked with fault processing disabled (VT-d), and diff --git a/kernel/loom/tests/device_irq_gate.rs b/kernel/loom/tests/device_irq_gate.rs index ee85c90d5c7..20bc51d4274 100644 --- a/kernel/loom/tests/device_irq_gate.rs +++ b/kernel/loom/tests/device_irq_gate.rs @@ -1,6 +1,7 @@ //! Loom: a claimed function's interrupt ends masked exactly while its record -//! holds a count, however the handler and a read interleave -//! (`pcidev/record.rs`); the `device-irq-gate-intent` control must red it. +//! owes a mask, from the second unread message, however the handler and a +//! read interleave (`pcidev/record.rs`); the `device-irq-gate-intent` control +//! must red the two-message model. use kernel_loom::device_irq::{Gate, Interrupt}; use loom::sync::{Arc, Mutex}; @@ -17,7 +18,7 @@ impl Gate for Line { /// Messages against one read that may run at any point among them, and a /// last read once they have all landed: after each, hardware is masked -/// exactly while the record holds a count. +/// exactly while the record owes it. fn model(messages: u64) { loom::model(move || { let irq = Arc::new(Interrupt::new()); @@ -37,10 +38,10 @@ fn model(messages: u64) { let masked = *line.0.lock().unwrap(); assert_eq!( masked, - irq.pending(), - "the read took {taken} of {messages} message(s) and hardware is {} with {} waiting", + irq.owes_mask(), + "the read took {taken} of {messages} message(s) and hardware is {} where the record owes it {}", if masked { "masked" } else { "open" }, - if irq.pending() { "a count" } else { "nothing" }, + if irq.owes_mask() { "masked" } else { "open" }, ); let left = irq.take_opening(&*line).map_or(0, |record| u64::from(record.count)); @@ -49,12 +50,14 @@ fn model(messages: u64) { }); } +/// One message per read never masks. #[test] -fn hardware_ends_as_the_record_owes() { +fn one_message_per_read_never_masks() { model(1); } -/// A second message landing while the first is masked or being read. +/// A second message before a read masks, and the read that takes them opens +/// it, wherever the read lands among them. #[test] fn hardware_ends_as_the_record_owes_after_two_messages() { model(2); diff --git a/kernel/src/pcidev/mod.rs b/kernel/src/pcidev/mod.rs index a6ddf13c9b8..e4db0f2f2d8 100644 --- a/kernel/src/pcidev/mod.rs +++ b/kernel/src/pcidev/mod.rs @@ -1978,7 +1978,7 @@ pub fn config_read(binding: &Binding, at: Register, width: RegWidth) -> Result Result, SyscallError> { @@ -2006,8 +2006,9 @@ pub fn has_irq(binding: &Binding) -> bool { } /// Records one message, masks the claim's message until its holder reads -/// the record, and posts the claim's watch: a function interrupts at most -/// once per read, whatever its holder makes it send. Called from the +/// the record where this is the second unread one, and posts the claim's +/// watch: a function's interrupts between two reads stop at what lands +/// before the mask, whatever its holder makes it send. Called from the /// interrupt's handler for function `number`, so it allocates nothing; /// `record.rs` owns the counting and the mask's rule, and `kernel-loom` /// models both against a concurrent reader. diff --git a/kernel/src/pcidev/record.rs b/kernel/src/pcidev/record.rs index 9db1be3974a..8a0500da5db 100644 --- a/kernel/src/pcidev/record.rs +++ b/kernel/src/pcidev/record.rs @@ -28,13 +28,14 @@ //! the slot the first is loading; that costs the racing holder its own time, //! never a count. //! -//! **A claim's interrupt is masked while a count waits** ([`Gate`]): the ISR -//! masks it after it counts, and the holder's read opens it after it takes, so -//! a function interrupts at most once per read. Each side changes the count -//! first and then, under the gate's lock, puts hardware in the state the -//! record then owes, read from the word and not from its own change: the side -//! that takes the lock last sees both changes, so a mask that lands after the -//! read that opened the gate cannot leave a claim masked with nothing to read. +//! **A claim's interrupt is masked from the second message before a read** +//! ([`Gate`], [`MASKED_FROM`]): a driver that reads between messages never +//! pays for a mask, and one that falls behind is held to what lands before +//! the mask takes. Each side changes the count first and then, under the +//! gate's lock, puts hardware in the state the record then owes, read from +//! the word and not from its own change: the side that takes the lock last +//! sees both changes, so a mask that lands after the read that opened the +//! gate cannot leave a claim masked with nothing to read. #[cfg(not(feature = "loom"))] use core::sync::atomic::{AtomicBool, AtomicU64, Ordering}; @@ -50,6 +51,10 @@ const ORDER: Ordering = Ordering::Relaxed; /// The count is the word's low half and the reads that took one its high half. const COUNT: u64 = 0xFFFF_FFFF; + +/// The count from which a claim's interrupt is masked until its holder reads: +/// a message landing while one already waits unread. +pub const MASKED_FROM: u64 = 2; const READ: u64 = 1 << 32; /// The negative control: each compare-exchange on the word becomes a load and @@ -189,13 +194,17 @@ impl Interrupt { self.word.load(ORDER) & COUNT != 0 } - /// [`Self::took`], then the claim's interrupt masked behind `gate`. + /// [`Self::took`], then the claim's interrupt masked behind `gate` where + /// this message found one waiting unread. pub fn took_masking(&self, now: u64, gate: &impl Gate) { self.took(now); #[cfg(feature = "device-irq-gate-intent")] - gate.hold(|| true); + { + let seen = self.owes_mask(); + gate.hold(|| seen); + } #[cfg(not(feature = "device-irq-gate-intent"))] - gate.hold(|| self.pending()); + gate.hold(|| self.owes_mask()); } /// [`Self::take`], then the claim's interrupt opened behind `gate` where @@ -206,11 +215,16 @@ impl Interrupt { #[cfg(feature = "device-irq-gate-intent")] gate.hold(|| false); #[cfg(not(feature = "device-irq-gate-intent"))] - gate.hold(|| self.pending()); + gate.hold(|| self.owes_mask()); } taken } + /// Whether the record owes its claim's interrupt masked. + pub fn owes_mask(&self) -> bool { + self.word.load(ORDER) & COUNT >= MASKED_FROM + } + /// Whether this is the first message this claim has taken. Answers `true` /// once per claim and `false` ever after, so a caller may log on it. /// diff --git a/sdk/abi/src/pci.rs b/sdk/abi/src/pci.rs index af8952e88f1..7d9de4e77d8 100644 --- a/sdk/abi/src/pci.rs +++ b/sdk/abi/src/pci.rs @@ -204,10 +204,11 @@ const _: () = assert!(core::mem::size_of::() == 8 + 8); /// slow reader to overflow. The count says how many messages arrived, never /// what any of them meant: that is in the device's own rings. /// -/// **A claimed PCI function's interrupt is masked from a message until the -/// read that takes it**, so it interrupts at most once per read: a message -/// sent meanwhile is refused at the unit on x86-64, and left pending until -/// the read on AArch64. A count above one is only what was in flight. +/// **A claimed PCI function's interrupt is masked from the second message +/// that lands before a read until that read**: a message sent meanwhile is +/// refused at the unit on x86-64, and left pending until the read on AArch64. +/// So a count of one is every message, and a count of two or more is a +/// lower bound. /// /// **The two times are when messages landed, which a driver reading its rings /// later cannot recover**: a driver clocked by its device — soundserver's DLL — diff --git a/sdk/toyos/src/device.rs b/sdk/toyos/src/device.rs index 852235163ab..eb765d92a82 100644 --- a/sdk/toyos/src/device.rs +++ b/sdk/toyos/src/device.rs @@ -193,10 +193,11 @@ impl PciDev { /// The interrupts since the last read, or `Err(WouldBlock)` for none. /// - /// **Take the record before servicing the device.** From a message to the - /// read that takes it, the function's interrupt is masked, and what the - /// device signals meanwhile raises nothing: a driver that serviced first - /// and read after could miss an event that arrived in between. + /// **Take the record before servicing the device.** From a second + /// message before a read to that read, the function's interrupt is + /// masked, and what the device signals meanwhile raises nothing: a driver + /// that serviced first and read after could miss an event that arrived in + /// between. pub fn irq(&self) -> Result { irq_record(&self.0) } diff --git a/tests/guest/rust-tests/src/bin/i219_msi.rs b/tests/guest/rust-tests/src/bin/i219_msi.rs index 0c82b51bc83..c68ba4ffae8 100644 --- a/tests/guest/rust-tests/src/bin/i219_msi.rs +++ b/tests/guest/rust-tests/src/bin/i219_msi.rs @@ -39,13 +39,13 @@ const WINDOW_NANOS: u64 = toyos_i219::DELIVERY_DEADLINE_NANOS; /// Causes forced back to back with the claim's record left unread: a holder /// making its function speak as fast as its own register writes go. const STORM: u32 = 10_000; -/// The most of them that may land: the first masks the claim's entry and -/// drains what the unit already remapped before its end of interrupt, and a -/// local APIC holds one request per vector beside the one in service, so -/// whatever was drained is one more delivery (Intel SDM Vol. 3A §11.8.4, +/// The most of them that may land: the first never masks the claim's entry +/// and the second does, draining what the unit already remapped before its +/// end of interrupt, which a local APIC holds as one more request for the +/// vector beside the one in service (Intel SDM Vol. 3A, 325384-093, §13.8.4, /// "Interrupt Acceptance for Fixed Interrupts"). Unmasked, every one lands: /// the `ICR` read between forces outlasts the handler. -const STORM_BOUND: u32 = 2; +const STORM_BOUND: u32 = 3; /// How long the link is given to come up after the driver's reset. const LINK_NANOS: u64 = 15_000_000_000; /// How long frames are listened for after the DHCP discover. From b6833aa4ca031c3640cc26d6440b5df398e50d3c Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 5 Nov 2026 18:41:05 +0100 Subject: [PATCH 06/12] toyos-abi states the count from which a claim's interrupt is masked A record below MASKED_FROM counts every message, and one at or above it is a lower bound; the kernel's gate and a driver that needs the difference read the same constant. Co-Authored-By: Claude Opus 5.5 --- kernel/src/pcidev/record.rs | 7 ++----- sdk/abi/src/pci.rs | 8 ++++++-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/kernel/src/pcidev/record.rs b/kernel/src/pcidev/record.rs index 8a0500da5db..52a95a9992d 100644 --- a/kernel/src/pcidev/record.rs +++ b/kernel/src/pcidev/record.rs @@ -43,7 +43,7 @@ use core::sync::atomic::{AtomicBool, AtomicU64, Ordering}; #[cfg(feature = "loom")] use loom::sync::atomic::{AtomicBool, AtomicU64, Ordering}; -use toyos_abi::pci::DeviceIrqRecord; +use toyos_abi::pci::{DeviceIrqRecord, MASKED_FROM}; /// The words that say only what they hold: the latch, the fault and the /// newest stamp, whose reader takes it after the exchange that orders it. @@ -52,9 +52,6 @@ const ORDER: Ordering = Ordering::Relaxed; /// The count is the word's low half and the reads that took one its high half. const COUNT: u64 = 0xFFFF_FFFF; -/// The count from which a claim's interrupt is masked until its holder reads: -/// a message landing while one already waits unread. -pub const MASKED_FROM: u64 = 2; const READ: u64 = 1 << 32; /// The negative control: each compare-exchange on the word becomes a load and @@ -222,7 +219,7 @@ impl Interrupt { /// Whether the record owes its claim's interrupt masked. pub fn owes_mask(&self) -> bool { - self.word.load(ORDER) & COUNT >= MASKED_FROM + self.word.load(ORDER) & COUNT >= u64::from(MASKED_FROM) } /// Whether this is the first message this claim has taken. Answers `true` diff --git a/sdk/abi/src/pci.rs b/sdk/abi/src/pci.rs index 7d9de4e77d8..88d5cf68c9e 100644 --- a/sdk/abi/src/pci.rs +++ b/sdk/abi/src/pci.rs @@ -207,8 +207,8 @@ const _: () = assert!(core::mem::size_of::() == 8 + 8); /// **A claimed PCI function's interrupt is masked from the second message /// that lands before a read until that read**: a message sent meanwhile is /// refused at the unit on x86-64, and left pending until the read on AArch64. -/// So a count of one is every message, and a count of two or more is a -/// lower bound. +/// So a count below [`MASKED_FROM`] is every message, and one at it or above +/// is a lower bound. /// /// **The two times are when messages landed, which a driver reading its rings /// later cannot recover**: a driver clocked by its device — soundserver's DLL — @@ -228,6 +228,10 @@ pub struct DeviceIrqRecord { pub last_nanos: u64, } +/// The count from which a claimed PCI function's interrupt is masked until +/// the read that takes it. +pub const MASKED_FROM: u32 = 2; + impl DeviceIrqRecord { pub const SIZE: usize = core::mem::size_of::(); } From 04f33e023357b72a31f3aeb6e469e26e8a906eeb Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 5 Nov 2026 18:48:15 +0100 Subject: [PATCH 07/12] issues: i219_msi's burst does not fill the T14's ring, a sighting On #1188's and #1193's T14 runs the wire sends the burst faster than the driver queues it, so the phase's tx_full requirement is never met; recorded under the issue whose exit that premise is. Co-Authored-By: Claude Opus 5.5 --- ...ntel-nics-room-and-wake-are-unread-on-hardware.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/issues/the-intel-nics-room-and-wake-are-unread-on-hardware.md b/issues/the-intel-nics-room-and-wake-are-unread-on-hardware.md index 86660fb6fc1..72b44f94130 100644 --- a/issues/the-intel-nics-room-and-wake-are-unread-on-hardware.md +++ b/issues/the-intel-nics-room-and-wake-are-unread-on-hardware.md @@ -35,6 +35,18 @@ write-back policy this path's. What either part does with its ring across a link change is not in this exit: `issues/no-machine-has-read-an-intel-nic-across-a-link-change.md`. +**Sighting, 2026-11-05: `i219_msi`'s burst does not fill the T14's ring +either.** Its traffic phase queues `4 * TX_RING` frames back to back and +then requires `tx_full` and a taken wake, and on the bench's link the wire +sends them faster than the driver queues them: "the ring full 0 time(s), 0 +wake(s) asked and 0 taken, 0 stranded; the MAC counted 65 sent", both on +#1188's T14 run, where the job failed first on its receive check +(https://github.com/ToyOSOrg/evidence/blob/72d7fa82557c6680fb33008665e717ddc83cb849/pr-1188/r3/t14-kernel-excerpt.log, +line 43), and on #1193's B arm, where it is the job's failure +(https://github.com/ToyOSOrg/evidence/tree/3bfbd7de150de5686df1edc749efc33b07722354/pr-1193/r1). +That phase's `tx_full` requirement is this exit's premise and the burst does +not establish it. + ## Owner The T14's outbound rows of `issues/toyos-has-its-own-network-stack.md`: the From d380f476dacb7b01179e255db71958e516accd0a Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 5 Nov 2026 18:50:34 +0100 Subject: [PATCH 08/12] virtio-sound: a record that dropped notifications owes nothing, and the next is not clocked (option b) With #1193's gate a claim's interrupt masks at the second notification before a read, and what lands past it is dropped, so a record at or above toyos_abi::pci::MASKED_FROM is a lower bound. The notifications it holds still answer the first periods in order, so they clock as before; what it leaves owed is unknown and is taken as none, and the next record clocks nothing, since its first notification may be for a period already read. Today's accounting kept the dropped notifications owed, so no later record clocked a period again and the mix loop's DLL stopped being corrected. Cost: one record's anchor after each record that dropped notifications; its periods carry into the DLL's next update. A wrong anchor needs that race twice running. a_record_that_dropped_notifications_does_not_stop_the_clock holds it; today's accounting reds it, clocking [0, 0, 0]. Co-Authored-By: Claude Opus 5.5 --- system/soundserver/src/virtio.rs | 8 +++- system/soundserver/virtio-sound/src/lib.rs | 40 +++++++++++++++++--- system/soundserver/virtio-sound/src/tests.rs | 32 +++++++++++++++- 3 files changed, 70 insertions(+), 10 deletions(-) diff --git a/system/soundserver/src/virtio.rs b/system/soundserver/src/virtio.rs index 8964c3a543c..71c1efd337f 100644 --- a/system/soundserver/src/virtio.rs +++ b/system/soundserver/src/virtio.rs @@ -26,7 +26,7 @@ use toyos_abi::syscall::SyscallError; use toyos_pci_claim::virtio::{negotiate, Negotiated}; use toyos_pci_claim::{Bar, Grant, KernelRefused}; use toyos_virtio::pci::{Live, NO_VECTOR}; -use toyos_virtio_sound::{Queues, Sound, PERIOD_BYTES, STREAM_ID}; +use toyos_virtio_sound::{Notified, Queues, Sound, PERIOD_BYTES, STREAM_ID}; use crate::backend::Completion; @@ -153,7 +153,11 @@ impl Virtio { let record = record?; let played = self .sound - .played(Some(record.count)) + .played(Some(if record.count >= toyos_abi::pci::MASKED_FROM { + Notified::AtLeast(record.count) + } else { + Notified::All(record.count) + })) .unwrap_or_else(|why| panic!("soundserver: virtio-sound cannot be driven on — {why}"))?; Some(Completion { mask: played.mask, diff --git a/system/soundserver/virtio-sound/src/lib.rs b/system/soundserver/virtio-sound/src/lib.rs index b1249293175..68994ab7462 100644 --- a/system/soundserver/virtio-sound/src/lib.rs +++ b/system/soundserver/virtio-sound/src/lib.rs @@ -235,6 +235,16 @@ impl Queues { } } +/// How many notifications a record of the claim's interrupt says landed. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Notified { + /// Every one since the last record. + All(u32), + /// At least this many: the claim's interrupt was masked before the read, + /// and what landed past the mask was dropped (`toyos_abi::pci`). + AtLeast(u32), +} + /// The periods one record answers for. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub struct Played { @@ -262,6 +272,9 @@ pub struct Sound { running: bool, /// Periods a read counted before their notification landed. unnotified: u32, + /// The last record dropped notifications, so the next one's may be for + /// a period an earlier read already counted. + doubted: bool, } impl Sound { @@ -272,7 +285,7 @@ impl Sound { if streams == 0 { return Err(Refusal::NoStream); } - let mut sound = Self { mem, bell, queues, running: false, unnotified: 0 }; + let mut sound = Self { mem, bell, queues, running: false, unnotified: 0, doubted: false }; for buffer in 0..EVENT_BUFS { sound.post_event(buffer); } @@ -347,15 +360,30 @@ impl Sound { /// record**: notifications answer periods in the order they came back, the /// ones an earlier read counted first, and [`Played::clocked`] counts the /// periods of this mask the record's notifications reach. - pub fn played(&mut self, notified: Option) -> Result, Refusal> { + /// + /// **A record of [`Notified::AtLeast`] dropped the notifications past its + /// mask**, so the ones it holds still answer the first periods in order, + /// and what it leaves owed is not known: a period this read found may have + /// had its notification dropped, or have it still to land. + /// It is taken as none, and the next record clocks nothing, since its + /// first notification may be one such period's. + pub fn played(&mut self, notified: Option) -> Result, Refusal> { let Some(notified) = notified else { return Ok(None) }; let mask = self.completed()?; let periods = mask.count_ones(); - let fresh = notified.saturating_sub(self.unnotified); - let clocked = fresh.min(periods); + let (count, dropped) = match notified { + Notified::All(count) => (count, false), + Notified::AtLeast(count) => (count, true), + }; + let fresh = count.saturating_sub(self.unnotified).min(periods); + let clocked = if self.doubted { 0 } else { fresh }; // No more can be owed than the ring holds periods. - self.unnotified = - (self.unnotified.saturating_sub(notified) + periods - clocked).min(PERIODS as u32); + self.unnotified = if dropped { + 0 + } else { + (self.unnotified.saturating_sub(count) + periods - fresh).min(PERIODS as u32) + }; + self.doubted = dropped; Ok((mask != 0).then_some(Played { mask, clocked })) } diff --git a/system/soundserver/virtio-sound/src/tests.rs b/system/soundserver/virtio-sound/src/tests.rs index 1d3a26ad92e..337619c8ca3 100644 --- a/system/soundserver/virtio-sound/src/tests.rs +++ b/system/soundserver/virtio-sound/src/tests.rs @@ -13,7 +13,7 @@ use toyos_virtio::queue::{Published, UsedRefusal}; use crate::wire::{self, Event, Failed, PcmInfo, Request}; use crate::{ - Played, + Notified, Played, Doorbell, Queues, Refusal, Sound, Stream, CONTROL_PARTS, EVENTS, EVENT_PARTS, PERIODS, PERIOD_BYTES, TX_PARTS, }; @@ -625,7 +625,7 @@ fn no_period_is_stranded_between_its_used_element_and_its_notification() { let mut taken: Option<(u32, u64)> = None; let mut back: Vec<(Played, u64)> = Vec::new(); let read = |sound: &mut Sound<_, _>, record: Option<(u32, u64)>, back: &mut Vec<(Played, u64)>| { - let played = sound.played(record.map(|(count, _)| count)).expect("a whole period"); + let played = sound.played(record.map(|(count, _)| Notified::All(count))).expect("a whole period"); back.extend(played.zip(record.map(|(_, at)| at))); }; for n in 0..8 { @@ -664,3 +664,31 @@ fn no_period_is_stranded_between_its_used_element_and_its_notification() { } assert_eq!(orders, 70); } + +/// A driver that fell behind is clocked again once it catches up: three +/// periods complete before a read, the claim's interrupt masks at the second +/// notification and drops the third (`toyos_abi::pci`), and the record is +/// [`Notified::AtLeast`]; then the device and the driver alternate, one +/// period, one notification and one read at a time, and the second steady read after it is clocked again, the first being doubted. +/// +/// Today's accounting reds this: it keeps the dropped notification owed, and +/// no record after it ever clocks a period. +#[test] +fn a_record_that_dropped_notifications_does_not_stop_the_clock() { + let model = Model::new(); + let mut sound = opened(&model); + for period in 0..PERIODS { + sound.submit(period).expect("a period goes out"); + } + model.play(3); + let behind = sound.played(Some(Notified::AtLeast(2))).expect("whole periods").expect("three periods came back"); + assert_eq!((behind.mask.count_ones(), behind.clocked), (3, 2), "the two notifications that landed clock the first two"); + let mut clocked = Vec::new(); + for _ in 0..3 { + model.play(1); + let played = sound.played(Some(Notified::All(1))).expect("a whole period").expect("one period came back"); + assert_eq!(played.mask.count_ones(), 1); + clocked.push(played.clocked); + } + assert_eq!(clocked, [0, 1, 1], "how many periods each steady read clocked"); +} From eb475222cdeaec980500d3fcfd489c57750c7dc9 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 5 Nov 2026 20:51:02 +0100 Subject: [PATCH 09/12] virtio-sound counts periods from its used ring alone, and a record is a wake and a time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A record's count is of interrupts the kernel took, not of messages the device sent: the local APIC folds a message that arrives while its vector is pending (Intel SDM Vol. 3A, 325384-093, §13.8.4), an LPI folds while pending, and the claim's mask drops what lands past it. virtio-sound inferred periods from that count, so a record of one interrupt over two periods left a notification owed that never landed, and no later record clocked a period again: the mix loop's DLL stopped being corrected. That stall predates #1193. Now the used ring alone says which periods played, on every read, and the record's newest time anchors the clock only where it can be no other period's: one interrupt taken and one period read. Any other read anchors nothing, and its periods carry into the DLL's next update, which it already does. The notification debt and option (b)'s Notified type are gone. a_record_folded_over_two_periods_does_not_stop_the_clock feeds a count of 1 over two periods and then three steady reads: [0, 1, 1, 1]. #1188's accounting, restored as the control, reads [1, 0, 0, 0]. Review r2 of #1193 besides: the ABI says what the count is; i219_msi's storm comment names the second interrupt as the one that masks; take_record's doc, record.rs's word constants and the issue's account of who pays a read's invalidation wait. Co-Authored-By: Claude Opus 5.5 --- .../a-claimed-function-can-storm-its-cpu.md | 11 +-- kernel/src/pcidev/mod.rs | 8 +-- kernel/src/pcidev/record.rs | 1 - sdk/abi/src/pci.rs | 24 +++---- sdk/toyos/src/device.rs | 2 +- system/soundserver/src/virtio.rs | 8 +-- system/soundserver/virtio-sound/src/lib.rs | 70 +++++-------------- system/soundserver/virtio-sound/src/tests.rs | 28 +++----- tests/guest/rust-tests/src/bin/i219_msi.rs | 2 +- 9 files changed, 54 insertions(+), 100 deletions(-) diff --git a/issues/a-claimed-function-can-storm-its-cpu.md b/issues/a-claimed-function-can-storm-its-cpu.md index 304c57a8f1a..e153145f22f 100644 --- a/issues/a-claimed-function-can-storm-its-cpu.md +++ b/issues/a-claimed-function-can-storm-its-cpu.md @@ -14,11 +14,12 @@ That bounds it per read and not per unit of time, and three things remain. **A holder paces cpu0.** A holder that loops on forcing a cause through its own registers and reading its record, on any CPU, still raises one cpu0 -interrupt per iteration; one that forces two per read also costs cpu0 an -invalidation completion wait with interrupts off, at the mask and again at -its read (VT-d's interrupt-entry-cache invalidation, AMD-Vi's completion -wait, or the ITS's `SYNC`). All of it is charged to whatever cpu0 -interrupted, not to the holder. +interrupt per iteration, charged to whatever cpu0 interrupted and not to the +holder. One that forces two per read also costs cpu0 an invalidation +completion wait with interrupts off at the mask (VT-d's interrupt-entry-cache +invalidation, AMD-Vi's completion wait, or the ITS's `SYNC`); the read's own +wait runs in the holder's call, and reaches cpu0 only as a spin on the unit's +queue behind it. **A refused message is recorded nowhere.** While masked, a message is target aborted (AMD-Vi) or blocked with fault processing disabled (VT-d), and diff --git a/kernel/src/pcidev/mod.rs b/kernel/src/pcidev/mod.rs index e4db0f2f2d8..ee0962f69ac 100644 --- a/kernel/src/pcidev/mod.rs +++ b/kernel/src/pcidev/mod.rs @@ -1977,10 +1977,10 @@ pub fn config_read(binding: &Binding, at: Register, width: RegWidth) -> Result Result, SyscallError> { let function = binding.function(); if !binding.here() { diff --git a/kernel/src/pcidev/record.rs b/kernel/src/pcidev/record.rs index 52a95a9992d..88e76f7145f 100644 --- a/kernel/src/pcidev/record.rs +++ b/kernel/src/pcidev/record.rs @@ -51,7 +51,6 @@ const ORDER: Ordering = Ordering::Relaxed; /// The count is the word's low half and the reads that took one its high half. const COUNT: u64 = 0xFFFF_FFFF; - const READ: u64 = 1 << 32; /// The negative control: each compare-exchange on the word becomes a load and diff --git a/sdk/abi/src/pci.rs b/sdk/abi/src/pci.rs index 88d5cf68c9e..f57ad2bd366 100644 --- a/sdk/abi/src/pci.rs +++ b/sdk/abi/src/pci.rs @@ -197,18 +197,18 @@ pub struct DmaMapping { const _: () = assert!(core::mem::size_of::() == 8 + 8); -/// The interrupts that landed since the last read of a claim. +/// The interrupts the kernel took for a claim since its last read. /// /// One record and not a queue: the kernel accumulates, so a driver that slept /// through several is told about all of them at once and there is no ring for a -/// slow reader to overflow. The count says how many messages arrived, never -/// what any of them meant: that is in the device's own rings. +/// slow reader to overflow. The count is of interrupts the kernel took, never +/// of messages the device sent or what any of them meant: the interrupt +/// controller folds messages that arrive while one is pending, and what they +/// meant is in the device's own rings. /// -/// **A claimed PCI function's interrupt is masked from the second message -/// that lands before a read until that read**: a message sent meanwhile is -/// refused at the unit on x86-64, and left pending until the read on AArch64. -/// So a count below [`MASKED_FROM`] is every message, and one at it or above -/// is a lower bound. +/// **A claimed PCI function's interrupt is masked from the second interrupt +/// taken before a read until that read**: a message sent meanwhile is refused +/// at the unit on x86-64, and left pending until the read on AArch64. /// /// **The two times are when messages landed, which a driver reading its rings /// later cannot recover**: a driver clocked by its device — soundserver's DLL — @@ -220,11 +220,11 @@ pub struct DeviceIrqRecord { /// Never 0 in a record that was answered; an empty count is `WouldBlock`. pub count: u32, pub _pad: u32, - /// When the first message `count` holds landed. + /// When the first interrupt `count` holds was taken. pub first_nanos: u64, - /// When the newest landed: never older than the newest message `count` - /// holds, and newer only by one that lands as the read is made, which the - /// next read counts. + /// When the newest was: never older than the newest interrupt `count` + /// holds, and newer only by one taken as the read is made, which the next + /// read counts. pub last_nanos: u64, } diff --git a/sdk/toyos/src/device.rs b/sdk/toyos/src/device.rs index eb765d92a82..b637a3056b1 100644 --- a/sdk/toyos/src/device.rs +++ b/sdk/toyos/src/device.rs @@ -194,7 +194,7 @@ impl PciDev { /// The interrupts since the last read, or `Err(WouldBlock)` for none. /// /// **Take the record before servicing the device.** From a second - /// message before a read to that read, the function's interrupt is + /// interrupt before a read to that read, the function's interrupt is /// masked, and what the device signals meanwhile raises nothing: a driver /// that serviced first and read after could miss an event that arrived in /// between. diff --git a/system/soundserver/src/virtio.rs b/system/soundserver/src/virtio.rs index 71c1efd337f..8964c3a543c 100644 --- a/system/soundserver/src/virtio.rs +++ b/system/soundserver/src/virtio.rs @@ -26,7 +26,7 @@ use toyos_abi::syscall::SyscallError; use toyos_pci_claim::virtio::{negotiate, Negotiated}; use toyos_pci_claim::{Bar, Grant, KernelRefused}; use toyos_virtio::pci::{Live, NO_VECTOR}; -use toyos_virtio_sound::{Notified, Queues, Sound, PERIOD_BYTES, STREAM_ID}; +use toyos_virtio_sound::{Queues, Sound, PERIOD_BYTES, STREAM_ID}; use crate::backend::Completion; @@ -153,11 +153,7 @@ impl Virtio { let record = record?; let played = self .sound - .played(Some(if record.count >= toyos_abi::pci::MASKED_FROM { - Notified::AtLeast(record.count) - } else { - Notified::All(record.count) - })) + .played(Some(record.count)) .unwrap_or_else(|why| panic!("soundserver: virtio-sound cannot be driven on — {why}"))?; Some(Completion { mask: played.mask, diff --git a/system/soundserver/virtio-sound/src/lib.rs b/system/soundserver/virtio-sound/src/lib.rs index 68994ab7462..d1b084e8c6c 100644 --- a/system/soundserver/virtio-sound/src/lib.rs +++ b/system/soundserver/virtio-sound/src/lib.rs @@ -235,23 +235,14 @@ impl Queues { } } -/// How many notifications a record of the claim's interrupt says landed. -#[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub enum Notified { - /// Every one since the last record. - All(u32), - /// At least this many: the claim's interrupt was masked before the read, - /// and what landed past the mask was dropped (`toyos_abi::pci`). - AtLeast(u32), -} - /// The periods one record answers for. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub struct Played { /// The periods the device gave back, one bit each. pub mask: u32, - /// How many of them, in the order they came back, the record's newest - /// notification is the time of; the rest are a later record's. + /// 1 where the record's newest notification is the time of the one + /// period this read found, and 0 where it may not be: these periods are + /// then clocked by the next record that is. pub clocked: u32, } @@ -270,11 +261,6 @@ pub struct Sound { bell: D, queues: Queues, running: bool, - /// Periods a read counted before their notification landed. - unnotified: u32, - /// The last record dropped notifications, so the next one's may be for - /// a period an earlier read already counted. - doubted: bool, } impl Sound { @@ -285,7 +271,7 @@ impl Sound { if streams == 0 { return Err(Refusal::NoStream); } - let mut sound = Self { mem, bell, queues, running: false, unnotified: 0, doubted: false }; + let mut sound = Self { mem, bell, queues, running: false }; for buffer in 0..EVENT_BUFS { sound.post_event(buffer); } @@ -346,44 +332,22 @@ impl Sound { Ok(()) } - /// The periods the device has given back since a record of `notified` - /// notifications was taken; nothing without a record. + /// The periods the device has given back since the last read, for a + /// record of `taken` interrupts; nothing without a record. /// - /// The device writes a used element before the notification that says - /// so, and with `avail.flags` zero and no `VIRTIO_F_EVENT_IDX` it owes one - /// for every element (§2.7.7), so a ring read after a record was taken - /// holds every period that record counts. With no record the ring is not - /// read: a period already in it has a notification still to land, which - /// makes the record readable again, and it is read then. + /// The periods are the used ring's and never the count's: the interrupt + /// controller folds messages and the claim's mask drops them, so a count + /// says only that the device spoke (`toyos_abi::pci`). With no record the + /// ring is not read: a period already in it has a notification still to + /// land, which makes the record readable again, and it is read then. /// - /// **A period read before its notification landed is not clocked by this - /// record**: notifications answer periods in the order they came back, the - /// ones an earlier read counted first, and [`Played::clocked`] counts the - /// periods of this mask the record's notifications reach. - /// - /// **A record of [`Notified::AtLeast`] dropped the notifications past its - /// mask**, so the ones it holds still answer the first periods in order, - /// and what it leaves owed is not known: a period this read found may have - /// had its notification dropped, or have it still to land. - /// It is taken as none, and the next record clocks nothing, since its - /// first notification may be one such period's. - pub fn played(&mut self, notified: Option) -> Result, Refusal> { - let Some(notified) = notified else { return Ok(None) }; + /// **The record's newest time clocks a period only where it can be no + /// other's**: one interrupt taken and one period read. Any other read + /// takes no anchor, and its periods carry into the next that does. + pub fn played(&mut self, taken: Option) -> Result, Refusal> { + let Some(count) = taken else { return Ok(None) }; let mask = self.completed()?; - let periods = mask.count_ones(); - let (count, dropped) = match notified { - Notified::All(count) => (count, false), - Notified::AtLeast(count) => (count, true), - }; - let fresh = count.saturating_sub(self.unnotified).min(periods); - let clocked = if self.doubted { 0 } else { fresh }; - // No more can be owed than the ring holds periods. - self.unnotified = if dropped { - 0 - } else { - (self.unnotified.saturating_sub(count) + periods - fresh).min(PERIODS as u32) - }; - self.doubted = dropped; + let clocked = u32::from(count == 1 && mask.count_ones() == 1); Ok((mask != 0).then_some(Played { mask, clocked })) } diff --git a/system/soundserver/virtio-sound/src/tests.rs b/system/soundserver/virtio-sound/src/tests.rs index 337619c8ca3..69ea0094663 100644 --- a/system/soundserver/virtio-sound/src/tests.rs +++ b/system/soundserver/virtio-sound/src/tests.rs @@ -13,7 +13,7 @@ use toyos_virtio::queue::{Published, UsedRefusal}; use crate::wire::{self, Event, Failed, PcmInfo, Request}; use crate::{ - Notified, Played, + Played, Doorbell, Queues, Refusal, Sound, Stream, CONTROL_PARTS, EVENTS, EVENT_PARTS, PERIODS, PERIOD_BYTES, TX_PARTS, }; @@ -625,7 +625,7 @@ fn no_period_is_stranded_between_its_used_element_and_its_notification() { let mut taken: Option<(u32, u64)> = None; let mut back: Vec<(Played, u64)> = Vec::new(); let read = |sound: &mut Sound<_, _>, record: Option<(u32, u64)>, back: &mut Vec<(Played, u64)>| { - let played = sound.played(record.map(|(count, _)| Notified::All(count))).expect("a whole period"); + let played = sound.played(record.map(|(count, _)| count)).expect("a whole period"); back.extend(played.zip(record.map(|(_, at)| at))); }; for n in 0..8 { @@ -665,30 +665,24 @@ fn no_period_is_stranded_between_its_used_element_and_its_notification() { assert_eq!(orders, 70); } -/// A driver that fell behind is clocked again once it catches up: three -/// periods complete before a read, the claim's interrupt masks at the second -/// notification and drops the third (`toyos_abi::pci`), and the record is -/// [`Notified::AtLeast`]; then the device and the driver alternate, one -/// period, one notification and one read at a time, and the second steady read after it is clocked again, the first being doubted. -/// -/// Today's accounting reds this: it keeps the dropped notification owed, and -/// no record after it ever clocks a period. +/// A record of one interrupt over two periods anchors nothing, and the +/// steady reads after it are each clocked again. #[test] -fn a_record_that_dropped_notifications_does_not_stop_the_clock() { +fn a_record_folded_over_two_periods_does_not_stop_the_clock() { let model = Model::new(); let mut sound = opened(&model); for period in 0..PERIODS { sound.submit(period).expect("a period goes out"); } - model.play(3); - let behind = sound.played(Some(Notified::AtLeast(2))).expect("whole periods").expect("three periods came back"); - assert_eq!((behind.mask.count_ones(), behind.clocked), (3, 2), "the two notifications that landed clock the first two"); - let mut clocked = Vec::new(); + model.play(2); + let folded = sound.played(Some(1)).expect("whole periods").expect("two periods came back"); + assert_eq!(folded.mask.count_ones(), 2); + let mut clocked = vec![folded.clocked]; for _ in 0..3 { model.play(1); - let played = sound.played(Some(Notified::All(1))).expect("a whole period").expect("one period came back"); + let played = sound.played(Some(1)).expect("a whole period").expect("one period came back"); assert_eq!(played.mask.count_ones(), 1); clocked.push(played.clocked); } - assert_eq!(clocked, [0, 1, 1], "how many periods each steady read clocked"); + assert_eq!(clocked, [0, 1, 1, 1], "how many periods the folded read and each steady read clocked"); } diff --git a/tests/guest/rust-tests/src/bin/i219_msi.rs b/tests/guest/rust-tests/src/bin/i219_msi.rs index c68ba4ffae8..7e9a102995a 100644 --- a/tests/guest/rust-tests/src/bin/i219_msi.rs +++ b/tests/guest/rust-tests/src/bin/i219_msi.rs @@ -152,7 +152,7 @@ fn force(regs: &R, claim: &Claim, forced: u32, unmask: u32) -> (u3 /// `STORM` causes forced with `ITR` off and the record unread, each read off /// `ICR` as asserted, then the messages that landed: the claim's interrupt -/// is masked from the first until this read. +/// is masked from the second until this read. fn storm(regs: &R, claim: &Claim) -> (u32, u32) { regs.write32(regs::IMC, u32::MAX); regs.write32(regs::ITR, 0); From 594d7cf55fa8ba874b8ca0693c98f4e1065b6352 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 5 Nov 2026 21:15:05 +0100 Subject: [PATCH 10/12] issues: the sysroot pin test reads a stat-dirty file as uncommitted work Seen in #1193's host CI at ce5d8469d under host load; the test passes alone at the same revision. Unrelated to #1193, recorded with its evidence. Co-Authored-By: Claude Opus 5.5 --- ...s-a-stat-dirty-file-as-uncommitted-work.md | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 issues/the-sysroot-pin-test-reads-a-stat-dirty-file-as-uncommitted-work.md diff --git a/issues/the-sysroot-pin-test-reads-a-stat-dirty-file-as-uncommitted-work.md b/issues/the-sysroot-pin-test-reads-a-stat-dirty-file-as-uncommitted-work.md new file mode 100644 index 00000000000..34a5ff6506a --- /dev/null +++ b/issues/the-sysroot-pin-test-reads-a-stat-dirty-file-as-uncommitted-work.md @@ -0,0 +1,25 @@ +--- +status: open +kind: tooling +opened: 2026-11-05 +--- + +# The sysroot pin test reads a stat-dirty file as uncommitted work + +`sysroot::tests::the_primary_s_fork_checkout_is_moved_to_its_pin` +(`build/sysroot.rs`) redded in a `cargo run -- --ci host` run at ce5d8469d on +a host under other builds' load, and passed alone at the same revision. Its +refusal named `library/std/src/lib.rs` as uncommitted work in the test's own +fake fork, with `git status --porcelain=v2` reporting it `.M` over the same +blob on both sides (`82a3da18…`): the file was dirty by its stat alone, git's +racy-timestamp case, and not by content. The check in the "move the fork +checkout to its pin" step reads that status as work and panics, so the test +reds whenever the test's own checkout lands in the same timestamp tick as the +index write. + +Evidence: #1193's host CI log at ce5d8469d, and the test alone passing there: +https://github.com/ToyOSOrg/evidence/tree/38c6ab9b31d7e7eff1b3ffc9be1180c3130f29c7/pr-1193/r3/logs + +Owner: the build track. Exit: the pin check tells content from stat (for +instance by refreshing the index first), and the test no longer reds on a +stat-only change. From 9b0fe9ca61965697ef7834b885fee37a6aa4c41e Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 5 Nov 2026 21:20:48 +0100 Subject: [PATCH 11/12] review r3 of #1193: the clock anchor's bound stated, and the sysroot record held to what was seen - virtio-sound's anchor is the record's newest time where one interrupt was taken and one period read. A period read ahead of its notification can make the next such read anchor early, by at most that read's wake latency; played, Played::clocked and the interleaving test's doc now say so, the test's stamp claim narrowed to the two periods it drives. - The sysroot record states the step that refused, its line, and that the test passes alone, and leaves the cause open; renamed to match. - DeviceIrqRecord::last_nanos: "when the newest was taken". Co-Authored-By: Claude Opus 5.5 --- ...s-a-stat-dirty-file-as-uncommitted-work.md | 25 ------------------ ...-test-refused-a-move-it-expects-to-make.md | 26 +++++++++++++++++++ sdk/abi/src/pci.rs | 6 ++--- system/soundserver/virtio-sound/src/lib.rs | 13 +++++----- system/soundserver/virtio-sound/src/tests.rs | 7 ++--- 5 files changed, 40 insertions(+), 37 deletions(-) delete mode 100644 issues/the-sysroot-pin-test-reads-a-stat-dirty-file-as-uncommitted-work.md create mode 100644 issues/the-sysroot-pin-test-refused-a-move-it-expects-to-make.md diff --git a/issues/the-sysroot-pin-test-reads-a-stat-dirty-file-as-uncommitted-work.md b/issues/the-sysroot-pin-test-reads-a-stat-dirty-file-as-uncommitted-work.md deleted file mode 100644 index 34a5ff6506a..00000000000 --- a/issues/the-sysroot-pin-test-reads-a-stat-dirty-file-as-uncommitted-work.md +++ /dev/null @@ -1,25 +0,0 @@ ---- -status: open -kind: tooling -opened: 2026-11-05 ---- - -# The sysroot pin test reads a stat-dirty file as uncommitted work - -`sysroot::tests::the_primary_s_fork_checkout_is_moved_to_its_pin` -(`build/sysroot.rs`) redded in a `cargo run -- --ci host` run at ce5d8469d on -a host under other builds' load, and passed alone at the same revision. Its -refusal named `library/std/src/lib.rs` as uncommitted work in the test's own -fake fork, with `git status --porcelain=v2` reporting it `.M` over the same -blob on both sides (`82a3da18…`): the file was dirty by its stat alone, git's -racy-timestamp case, and not by content. The check in the "move the fork -checkout to its pin" step reads that status as work and panics, so the test -reds whenever the test's own checkout lands in the same timestamp tick as the -index write. - -Evidence: #1193's host CI log at ce5d8469d, and the test alone passing there: -https://github.com/ToyOSOrg/evidence/tree/38c6ab9b31d7e7eff1b3ffc9be1180c3130f29c7/pr-1193/r3/logs - -Owner: the build track. Exit: the pin check tells content from stat (for -instance by refreshing the index first), and the test no longer reds on a -stat-only change. diff --git a/issues/the-sysroot-pin-test-refused-a-move-it-expects-to-make.md b/issues/the-sysroot-pin-test-refused-a-move-it-expects-to-make.md new file mode 100644 index 00000000000..59fd0de51a7 --- /dev/null +++ b/issues/the-sysroot-pin-test-refused-a-move-it-expects-to-make.md @@ -0,0 +1,26 @@ +--- +status: open +kind: tooling +opened: 2026-11-05 +--- + +# The sysroot pin test refused a move it expects to make, once + +`sysroot::tests::the_primary_s_fork_checkout_is_moved_to_its_pin` +(`build/sysroot.rs`) redded once in a `cargo run -- --ci host` run at +ce5d8469d, on a host other builds were loading, and passed run alone at the +same revision. + +What the log shows: after three moves of the test's own fork checkout +(0a69 to 3a47, back, and to 3a47 again), the fourth, the test's `pin(&c1)` +before its deliberate edit, refused with "is at 3a47… with uncommitted work, +and this tree pins the fork at 0a69…", naming one entry, +`1 .M N... 100644 100644 100644 82a3da18… 82a3da18… library/std/src/lib.rs`. +The test had written nothing to that file before the refusal. Why `git +status` reported it modified is not established. + +Evidence: #1193's host CI log at ce5d8469d and the test alone passing there: +https://github.com/ToyOSOrg/evidence/tree/38c6ab9b31d7e7eff1b3ffc9be1180c3130f29c7/pr-1193/r3/logs + +Owner: the build track. Exit: the cause found and fixed, or the test shown to +hold under the load it redded in. diff --git a/sdk/abi/src/pci.rs b/sdk/abi/src/pci.rs index f57ad2bd366..f0fa833099c 100644 --- a/sdk/abi/src/pci.rs +++ b/sdk/abi/src/pci.rs @@ -222,9 +222,9 @@ pub struct DeviceIrqRecord { pub _pad: u32, /// When the first interrupt `count` holds was taken. pub first_nanos: u64, - /// When the newest was: never older than the newest interrupt `count` - /// holds, and newer only by one taken as the read is made, which the next - /// read counts. + /// When the newest was taken: never older than the newest interrupt + /// `count` holds, and newer only by one taken as the read is made, which + /// the next read counts. pub last_nanos: u64, } diff --git a/system/soundserver/virtio-sound/src/lib.rs b/system/soundserver/virtio-sound/src/lib.rs index d1b084e8c6c..c182ffc04ae 100644 --- a/system/soundserver/virtio-sound/src/lib.rs +++ b/system/soundserver/virtio-sound/src/lib.rs @@ -240,9 +240,8 @@ impl Queues { pub struct Played { /// The periods the device gave back, one bit each. pub mask: u32, - /// 1 where the record's newest notification is the time of the one - /// period this read found, and 0 where it may not be: these periods are - /// then clocked by the next record that is. + /// 1 where this read anchors the clock at the record's newest time, else + /// 0: these periods are then clocked by the next read that anchors. pub clocked: u32, } @@ -341,9 +340,11 @@ impl Sound { /// ring is not read: a period already in it has a notification still to /// land, which makes the record readable again, and it is read then. /// - /// **The record's newest time clocks a period only where it can be no - /// other's**: one interrupt taken and one period read. Any other read - /// takes no anchor, and its periods carry into the next that does. + /// **The record's newest time anchors the clock where one interrupt was + /// taken and one period read**, and any other read anchors nothing, its + /// periods carrying into the next that does. A period read ahead of its + /// notification can make the next such read anchor early, by at most that + /// read's wake latency. pub fn played(&mut self, taken: Option) -> Result, Refusal> { let Some(count) = taken else { return Ok(None) }; let mask = self.completed()?; diff --git a/system/soundserver/virtio-sound/src/tests.rs b/system/soundserver/virtio-sound/src/tests.rs index 69ea0094663..a5134347a4d 100644 --- a/system/soundserver/virtio-sound/src/tests.rs +++ b/system/soundserver/virtio-sound/src/tests.rs @@ -586,9 +586,10 @@ impl Claim { } /// No period is stranded: every one comes back exactly once, and none while -/// the claim is left unreadable. **And none is stamped earlier than its own -/// notification**: a period a record clocks is one whose notification that -/// record, or an earlier one, carried. +/// the claim is left unreadable. **And within two periods none is stamped +/// earlier than its own notification**: a period a record clocks is one whose +/// notification that record, or an earlier one, carried. A third period can +/// be (`Sound::played`). /// /// Two periods, each a used element and then its notification (§2.7.7), /// against two wakes of a driver that takes the record and then reads the From 972ba7c5229a1f0cad11aa1881c49bb04c67f7d0 Mon Sep 17 00:00:00 2001 From: japabu Date: Fri, 6 Nov 2026 09:19:23 +0100 Subject: [PATCH 12/12] issues: a read into a claim's BAR is not refused on AArch64, a sighting virt_claim_lpi's claim_bar_fault job reds on main's own 0b2dec674 exactly as on #1193's rebased head: the read answers Ok(16), not BadAddress. Main's, not #1193's; recorded with both runs. Co-Authored-By: Claude Opus 5.5 --- ...-a-claims-bar-is-not-refused-on-aarch64.md | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 issues/a-read-into-a-claims-bar-is-not-refused-on-aarch64.md diff --git a/issues/a-read-into-a-claims-bar-is-not-refused-on-aarch64.md b/issues/a-read-into-a-claims-bar-is-not-refused-on-aarch64.md new file mode 100644 index 00000000000..ed489f705aa --- /dev/null +++ b/issues/a-read-into-a-claims-bar-is-not-refused-on-aarch64.md @@ -0,0 +1,27 @@ +--- +status: open +kind: defect +opened: 2026-11-06 +--- + +# A read into a claim's BAR is not refused on AArch64 + +`virt_claim_lpi`'s `claim_bar_fault` job (`tests/guest/rust-tests/src/bin/claim_bar_fault.rs`) +reads a pipe into a slice that points at its claim's mapped BAR registers and +expects `Err(BadAddress)`. On main at 0b2dec674 the read answered `Ok(16)`, +and the job ended at line 100: "a read into the BAR was not refused +BadAddress", left `Ok(16)`, right `Err(BadAddress)`. + +Seen on the guest runner's aarch64 row, both at main's own commit (run +42124293522, branch `storm-main-check`) and at #1193's rebased head (run +42120673220); the same row passed at #1193's heads before the rebase, on +#1188's base (runs 42035960983, 42077063953). Between the two bases main +took, among others, #1150, which routes every user copy through the process's +own mapping. Which change made the read succeed, and whether the kernel or +the test's expectation is now wrong, is not established. + +Evidence: https://github.com/ToyOSOrg/evidence/tree/05c9ce10c698b6618a8ac9c5f38f41fe1e2451a0/pr-1193/r4/logs + +Owner: the memory track (S0-a). Exit: a read whose destination is a claim's +BAR is refused as the kernel's copy contract states, or the contract and the +test say otherwise, and `virt_claim_lpi` passes on main.