From b07e7db72b8b08dc9dc353f5bc75cbe2e9374121 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 8 Oct 2026 11:09:23 +0200 Subject: [PATCH 1/3] A device call borrows what names its device from its claim, under the lock the claim's release takes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every syscall on a PCI claim read a `pcidev` slot number out of the claim, let the claim go, and acted on whatever `BOUND[slot]` held by then: `with_bound` asked whether a function was bound in the slot, not whose. The last handle's close releases the slot, a function that was reset leaves it free, and `slot::reserve` gives the first free slot to the next claim. So a call a sibling thread's close overtook, with a second claim arriving before its `with_bound`, mapped a BAR of, granted memory to, or read the configuration of another process's function. Read from the code by an audit and two reviews; nothing in the tree can hold a call between the two steps, so it was never run. `pcidev::claim` now answers a `Binding` — the slot and its requester, private, neither `Copy` nor `Clone`, its drop the release — and the claim holds it in its `Held`. Every `pcidev` call takes `&Binding`, and the only way to one is `DeviceClaim::pci`, which lends it inside `Held::with`: under the lock `Held::release` takes the binding out with. A call therefore runs wholly before the release, which waits for it, or finds nothing and answers `Gone`. No slot number leaves `pcidev` except to the two interrupt handlers, which get theirs from hardware. `with_bound`'s `NotFound` for an unbound slot is gone with the state that reached it. The poll paths had the same shape with no call in between: a readiness check read the slot's record and a registration landed on the slot's watch, both through an `Arc` cloned out of the table. A released claim now reads as ready, so a poll on it is fired by its registrar and never left on a watch the next holder posts. The ISA row is the same construction, for the same reason at a smaller reach: `isa::Row` is what `isa`'s calls take and its drop is the row's release, so an acknowledgement in flight can no longer unmask a level line the release masked, and a poll cannot land on the next claim's watch. That the ports stay with the process that bound them until it ends is true of every row — nothing in `isa` tells one from another — and the header says so. `dma_unmap` answers the region it took back instead of dropping it, so its last `Arc` still goes with no lock held now that the caller holds its claim's. Found beside it and filed, not fixed: a claim on a class this kernel drives (framebuffer, HDA, virtio-sound) checks its handle and acts with nothing held, and the two handlers that name a slot write its record a statement after reading the number. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RvnWQFcMuGqTHYhvSnTe8A --- ...in-flight-acts-after-the-claims-release.md | 39 ++++++ ...t-it-read-before-the-slot-changed-hands.md | 35 ++++++ ...unction-by-a-slot-the-next-claim-reuses.md | 38 ------ kernel/src/arch/aarch64/acpi_mode.rs | 4 +- kernel/src/arch/x86_64/acpi_mode.rs | 20 ++- kernel/src/device.rs | 63 ++++++---- kernel/src/inventory.rs | 7 +- kernel/src/isa.rs | 82 ++++++++----- kernel/src/object/device.rs | 59 ++++----- kernel/src/object/ops.rs | 79 +++++++----- kernel/src/pcidev/mod.rs | 114 ++++++++++++------ kernel/src/syscall/device.rs | 94 +++++++++------ 12 files changed, 387 insertions(+), 247 deletions(-) create mode 100644 issues/a-class-claims-call-in-flight-acts-after-the-claims-release.md create mode 100644 issues/a-handler-tells-a-slot-it-read-before-the-slot-changed-hands.md delete mode 100644 issues/a-pci-call-in-flight-names-its-function-by-a-slot-the-next-claim-reuses.md diff --git a/issues/a-class-claims-call-in-flight-acts-after-the-claims-release.md b/issues/a-class-claims-call-in-flight-acts-after-the-claims-release.md new file mode 100644 index 00000000000..766bdc07021 --- /dev/null +++ b/issues/a-class-claims-call-in-flight-acts-after-the-claims-release.md @@ -0,0 +1,39 @@ +--- +status: open +kind: defect +opened: 2026-10-08 +--- + +# A class claim's call in flight acts after the claim's release + +A claim on a class this kernel still drives — the framebuffer, HDA audio, +virtio-sound — is a per-class flag (`device::TAKEN`), and a call on one checks +the handle and then acts with nothing held across the two: + +- `SYS_DEVICE_REG_READ` and `SYS_DEVICE_REG_WRITE` resolve the claim to + `RegTarget::Hda` or `RegTarget::VirtioSound` under the process-data lock + (`sys_device_reg`, `kernel/src/syscall/device.rs`) and reach + `drivers::hda::reg_write` or `drivers::virtio_sound::reg_write` after it; +- `SYS_GPU_PRESENT`, `SYS_GPU_SET_CURSOR`, `SYS_GPU_MOVE_CURSOR` and + `SYS_GPU_SET_RESOLUTION` ask `holds_claim` and then call `crate::gpu` + (`kernel/src/syscall/dispatch.rs`). + +The flag clears when the claim's last handle goes (`Claim`'s drop, +`kernel/src/device.rs`) and the next claim sets it again. So a call whose +claim a sibling thread closed after the check, and whose class another process +claimed before the act, writes a register of the audio controller, or presents +on the display, that the second process now holds. + +A claim on a PCI function, an ISA function or the ACPI fixed hardware does not +have this: its calls borrow what names the device from the claim under the +lock its release takes it with (`DeviceClaim::pci`, `DeviceClaim::isa`). The +same borrow closes these — the act made inside the claim's `Held`. + +**Read from the code, not run.** A guest cannot order a close and a second +claim between the two steps of one syscall. + +**Exit condition**: every call on a class claim acts under the lock its +claim's release takes, or is refused, and no syscall reaches a kernel-driven +device on a `holds_claim` answer alone. + +**Owner**: whoever holds `issues/every-driver-is-still-in-the-kernel.md`. diff --git a/issues/a-handler-tells-a-slot-it-read-before-the-slot-changed-hands.md b/issues/a-handler-tells-a-slot-it-read-before-the-slot-changed-hands.md new file mode 100644 index 00000000000..b809074d1d6 --- /dev/null +++ b/issues/a-handler-tells-a-slot-it-read-before-the-slot-changed-hands.md @@ -0,0 +1,35 @@ +--- +status: open +kind: finding +opened: 2026-10-08 +--- + +# A handler tells a slot it read before the slot changed hands + +Two interrupt handlers name a `pcidev` slot by a number and write that slot's +record a moment later, with nothing excluding a release in between: + +- the remapping fault handler reads the faulting stream's slot + (`owner_of`, `kernel/src/arch/x86_64/vtd/fault.rs`) and then calls + `pcidev::note_fault(slot)`; +- a claimed function's vector handler is its slot (`idt/user_dev.rs`) and + calls `pcidev::isr(slot)`. + +`pcidev::tear_down` clears the stream's slot and takes the remapping entry +away before the slot is free, so a handler that *starts* after a release names +nothing. One already past its read on another CPU still holds the number: if +the slot is released and bound to a second function before its write, that +function's claim is told a fault — every call on it refuses `Io` from then +on — or counts one interrupt its device did not raise. + +The interval is two adjacent statements of a handler running with interrupts +off, and a second function's bring-up in between is config-space writes and, +after a reset, a wait of at least the reset's settle time; no authority moves +either way. Nothing in the tree can hold a handler there, so this is read and +not run. + +**Exit condition**: the slot's record says which binding it is for and a +handler's write is refused by a record that has moved on, or this is folded +into `kernel/src/pcidev/mod.rs`'s header as the bound it is. + +**Owner**: whoever holds `issues/every-driver-is-still-in-the-kernel.md`. diff --git a/issues/a-pci-call-in-flight-names-its-function-by-a-slot-the-next-claim-reuses.md b/issues/a-pci-call-in-flight-names-its-function-by-a-slot-the-next-claim-reuses.md deleted file mode 100644 index 5e8445994aa..00000000000 --- a/issues/a-pci-call-in-flight-names-its-function-by-a-slot-the-next-claim-reuses.md +++ /dev/null @@ -1,38 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-10-08 ---- - -# A PCI call in flight names its function by a slot the next claim reuses - -Every syscall on a PCI claim — the BAR, the DMA grants and mappings, the -config read — resolves its handle to a `pcidev` slot number -(`pci_slot` and the register target, `kernel/src/syscall/device.rs`) and lets -go of the claim before it acts. What -it then acts on is whatever `BOUND[slot]` holds when `with_bound` -(`kernel/src/pcidev/mod.rs`) takes that slot's lock, which checks that the -slot is bound and not faulted, and not whose binding it is. The two steps are -under different locks with nothing held across them. - -A slot is not a name for one claim: the last handle's close releases it, a -function that was reset leaves it `Free`, and `slot::reserve` -(`kernel/pci/src/slot.rs`) gives the first free slot to the next claim, -whichever function and process that is. A call that resolved its slot before -its own claim was released therefore runs against the next holder's function, -and answers its caller with that function's object. A claim is authority over -one function for one holder; here it reaches another's. - -**Read from the code, not run.** A call made after the close is refused at -the handle, so only a call already past `pci_slot` is exposed, and a thread -in a syscall holding no lock can be preempted there. A guest cannot order a -release and a second claim inside that interval; the test owes the kernel a -hold at that point, which is an actuator this tree does not have. - -**Exit condition**: a call on a claim acts on that claim's binding or is -refused — its identity is held from the handle to the act, or the binding is -checked against it — and a test that holds a call between the two steps, -across the claim's release and the slot's next claim, sees it refused with -the second holder's function untouched. - -**Owner**: whoever holds `issues/every-driver-is-still-in-the-kernel.md`. diff --git a/kernel/src/arch/aarch64/acpi_mode.rs b/kernel/src/arch/aarch64/acpi_mode.rs index 46eef7140af..4a14b397b10 100644 --- a/kernel/src/arch/aarch64/acpi_mode.rs +++ b/kernel/src/arch/aarch64/acpi_mode.rs @@ -5,11 +5,11 @@ use toyos_abi::acpi::AcpiInfo; use crate::device::ClaimError; -pub fn claim() -> Result<(usize, AcpiInfo), ClaimError> { +pub fn claim() -> Result<(crate::isa::Row, AcpiInfo), ClaimError> { Err(ClaimError::Absent) } /// Never called: nothing is released where nothing can be claimed. -pub fn release(_row: usize) { +pub fn release() { unreachable!("AArch64 has no ACPI row") } diff --git a/kernel/src/arch/x86_64/acpi_mode.rs b/kernel/src/arch/x86_64/acpi_mode.rs index fb9ac1bec61..8783375e98f 100644 --- a/kernel/src/arch/x86_64/acpi_mode.rs +++ b/kernel/src/arch/x86_64/acpi_mode.rs @@ -161,16 +161,12 @@ fn embedded_controller(rsdp_addr: u64, gpe0: Block) -> Result { /// The row, claimed, with the machine in ACPI mode; or the refusal that /// leaves it in legacy mode, said by name. -pub fn claim() -> Result<(usize, AcpiInfo), ClaimError> { +pub fn claim() -> Result<(isa::Row, AcpiInfo), ClaimError> { let hardware = hardware().ok_or(ClaimError::Absent)?; + // Released by the refusal's return, which drops it. let row = isa::claim_row(ROW)?; - match enter(hardware) { - Ok(()) => Ok((row, info(hardware))), - Err(refused) => { - isa::release(row); - Err(refused) - } - } + enter(hardware)?; + Ok((row, info(hardware))) } fn info(hardware: &Hardware) -> AcpiInfo { @@ -247,13 +243,13 @@ fn enter(hardware: &Hardware) -> Result<(), ClaimError> { } /// The claim's last handle went: the machine back in legacy mode where the -/// mint took it out of it, and then the row released, so no claimant finds -/// `SCI_EN` set by a holder whose disable is still to come. -pub fn release(row: usize) { +/// mint took it out of it. Before the row's own release, which its caller's +/// drop of the row is, so no claimant finds `SCI_EN` set by a holder whose +/// disable is still to come. +pub fn release() { if ENABLED.load(Ordering::Relaxed) { leave(hardware().expect("a claimed row has its hardware")); } - isa::release(row); } /// `ACPI_DISABLE`, written where the mint wrote `ACPI_ENABLE`, and `SCI_EN` diff --git a/kernel/src/device.rs b/kernel/src/device.rs index 8da06cb1816..c3295b400d5 100644 --- a/kernel/src/device.rs +++ b/kernel/src/device.rs @@ -44,12 +44,16 @@ pub struct Claim { /// so what it gives back is its `pcidev` slot; a partition's exclusivity is its /// view's own hold on the blocks (`block::Partition::of`); an ISA function's and /// the ACPI fixed hardware's is its `isa` row's. +/// +/// The slot and the row are held as what their modules' calls take, and the +/// drop of each is its release: a call borrows one from the claim's `Held`, so +/// none is made on a slot or a row its claim has let go. enum Claimed { Class(DeviceType), - PciFunction(usize), + PciFunction(crate::pcidev::Binding), Partition(crate::block::Partition), - Isa(usize), - Acpi(usize), + Isa(crate::isa::Row), + Acpi(crate::isa::Row), } impl Claim { @@ -62,12 +66,27 @@ impl Claim { Ok(Self { what: Claimed::Class(class) }) } - /// The guard for a `pcidev` slot the caller has already reserved. - /// - /// It exists from the moment the slot is taken, so a bring-up that refuses - /// half way through frees the slot on the way out rather than stranding it. - pub(crate) fn pci(slot: usize) -> Self { - Self { what: Claimed::PciFunction(slot) } + /// The claim on a function `pcidev` has bound. + pub(crate) fn pci(binding: crate::pcidev::Binding) -> Self { + Self { what: Claimed::PciFunction(binding) } + } + + /// What `pcidev`'s calls on this claim are made with, or `None` for a + /// claim on anything but a PCI function. + pub(crate) fn binding(&self) -> Option<&crate::pcidev::Binding> { + match &self.what { + Claimed::PciFunction(binding) => Some(binding), + Claimed::Class(_) | Claimed::Partition(_) | Claimed::Isa(_) | Claimed::Acpi(_) => None, + } + } + + /// What `isa`'s calls on this claim are made with, or `None` for a claim + /// on anything but an ISA function or the ACPI fixed hardware. + pub(crate) fn row(&self) -> Option<&crate::isa::Row> { + match &self.what { + Claimed::Isa(row) | Claimed::Acpi(row) => Some(row), + Claimed::Class(_) | Claimed::PciFunction(_) | Claimed::Partition(_) => None, + } } /// The view a partition claim transfers through, or `None` for any other. @@ -81,8 +100,8 @@ impl Claim { impl Drop for Claim { fn drop(&mut self) { - match self.what { - Claimed::Class(class) => { + match &self.what { + &Claimed::Class(class) => { // Before the flag goes: a poll the next holder registers is not this claim's to answer. match class { DeviceType::HdaAudio | DeviceType::VirtioSound => { @@ -98,14 +117,16 @@ impl Drop for Claim { } *taken(class).lock() = false; } - // Bus mastering off, then the domain, then the pages: `release` - // owns that order, and this is where a dying process reaches it. - Claimed::PciFunction(slot) => crate::pcidev::release(slot), + // Bus mastering off, then the domain, then the pages: the + // binding's drop owns that order, and this is where a dying + // process reaches it. + Claimed::PciFunction(_) => {} // The view drops with this, and its hold with the last clone of it. Claimed::Partition(_) => {} - Claimed::Isa(row) => crate::isa::release(row), - // Back to the mode the firmware handed over, too. - Claimed::Acpi(row) => crate::arch::acpi_mode::release(row), + // The row's drop is its release. + Claimed::Isa(_) => {} + // Back to the mode the firmware handed over, and then the row's drop. + Claimed::Acpi(_) => crate::arch::acpi_mode::release(), } } } @@ -182,8 +203,8 @@ pub fn try_claim(class: DeviceType, selector: [u64; 2]) -> Result { let found = crate::gpt::claimable(toyos_abi::part::PartGuid::from_wire(selector))?; @@ -200,13 +221,13 @@ pub fn try_claim(class: DeviceType, selector: [u64; 2]) -> Result { // The row's own guard, taken inside, and the switch to ACPI mode. let (row, info) = crate::arch::acpi_mode::claim()?; let claim = Claim { what: Claimed::Acpi(row) }; - Ok(DeviceClaim::new(class, DeviceInfo::Acpi(info, row), claim)) + Ok(DeviceClaim::new(class, DeviceInfo::Acpi(info), claim)) } DeviceType::HdaAudio => { let (info, pcm) = crate::drivers::hda::info().ok_or(ClaimError::Absent)?; diff --git a/kernel/src/inventory.rs b/kernel/src/inventory.rs index 40fbc451daa..8a907e325b4 100644 --- a/kernel/src/inventory.rs +++ b/kernel/src/inventory.rs @@ -68,10 +68,9 @@ fn claims() -> Vec { let data = data.lock(); for (_, entry) in data.handles.iter() { let KObjectRef::Device(claim) = entry.object() else { continue }; - let on = if let Some(slot) = claim.pci_slot() { - // A slot nobody holds any more is a claim whose release is - // under way; it names nothing. - let Some(at) = crate::pcidev::held_at(slot) else { continue }; + let on = if claim.class() == toyos_abi::syscall::DeviceType::PciFunction { + // A function the last handle has let go names nothing. + let Some(at) = claim.pci(crate::pcidev::held_at) else { continue }; Claimed::Pci(at) } else if claim.class() == toyos_abi::syscall::DeviceType::Partition { // A partition the last handle has let go names nothing. diff --git a/kernel/src/isa.rs b/kernel/src/isa.rs index b190377c306..918e522e5de 100644 --- a/kernel/src/isa.rs +++ b/kernel/src/isa.rs @@ -15,7 +15,8 @@ //! threads opens them and every other switch closes them //! (`arch::pio::switch_to`). A handle moved on after that answers nothing but //! refusals, and the row is claimable again only once both the claim and the -//! process are gone. A binding reaches the process's other threads at their +//! process are gone. **That holds for every row**: nothing here tells one from +//! another. A binding reaches the process's other threads at their //! next switch, and its end needs no switch at all: teardown runs once every //! thread has left, and no thread that has left returns to Ring 3. //! @@ -26,6 +27,11 @@ //! its handler too, and stays masked until the holder has served what raised //! it and acknowledged the claim ([`ack`]); it is masked from the claim until //! the holder's first acknowledgement. +//! +//! **A call on a claim is made with the claim's [`Row`] and never with a row +//! number**, lent under the lock the claim's release takes it with +//! (`object::Held`): the call runs wholly before the release or finds no row, +//! so none acknowledges, reads or waits on a row its claim has given up. use alloc::boxed::Box; use alloc::vec::Vec; @@ -105,8 +111,32 @@ fn lines(row: usize) -> &'static [pio::Line] { if at.is_null() { &[] } else { unsafe { &*at } } } +/// One claim's hold on a row, from its mint to the release its drop is. +/// +/// Neither `Copy` nor `Clone` and made only by [`claim_row`], so a reference +/// to it is a row whose claim has not been released. +pub struct Row(usize); + +/// The claim's last handle went: its lines masked, its record emptied and its +/// polls answered. The ports stay with the process that bound them until that +/// process ends. +impl Drop for Row { + fn drop(&mut self) { + let row = self.0; + for &line in lines(row) { + pio::set_masked(line, true); + } + IRQ[row].clear(); + // The claim is gone, so a poll on it is answered rather than left for the + // next holder's interrupts; the row stays minted until that is made, so no + // next claim's poll is among the ones answered. + WATCHES[row].cancel_polls(); + *MINTED[row].lock() = false; + } +} + /// The row an `isa:` selector names whole. -pub fn claim(set: IsaId) -> Result { +pub fn claim(set: IsaId) -> Result { let row = (0..MAX_ROWS) .find(|&row| { function(row).is_some_and(|f| { @@ -122,7 +152,7 @@ pub fn claim(set: IsaId) -> Result { /// Mint the claim on `row`: its ports checked against every port this kernel /// declared, its edge lines routed and unmasked, its level lines routed and /// left masked for the holder's first acknowledgement. -pub fn claim_row(row: usize) -> Result { +pub fn claim_row(row: usize) -> Result { let function = function(row).ok_or(ClaimError::Absent)?; if let Some((run, holder)) = function.runs.iter().find_map(|&run| pio::holder(run).map(|h| (run, h))) { log!("isa: {}'s ports {:#x}+{} are {holder}'s", function.name, run.first(), run.count()); @@ -155,28 +185,13 @@ pub fn claim_row(row: usize) -> Result { } } *minted = true; - Ok(row) -} - -/// The claim's last handle went: its lines masked, its record emptied and its -/// polls answered. The ports stay with the process that bound them until that -/// process ends. -pub fn release(row: usize) { - for &line in lines(row) { - pio::set_masked(line, true); - } - IRQ[row].clear(); - // The claim is gone, so a poll on it is answered rather than left for the - // next holder's interrupts; the row stays minted until that is made, so no - // next claim's poll is among the ones answered. - WATCHES[row].cancel_polls(); - *MINTED[row].lock() = false; + Ok(Row(row)) } /// The holder served what raised the row's level lines: unmask them. A row /// with no level line has nothing to acknowledge. -pub fn ack(row: usize) -> Result<(), ()> { - let mut level = lines(row).iter().copied().filter(|&line| pio::level(line)).peekable(); +pub fn ack(row: &Row) -> Result<(), ()> { + let mut level = lines(row.0).iter().copied().filter(|&line| pio::level(line)).peekable(); if level.peek().is_none() { return Err(()); } @@ -189,7 +204,8 @@ pub fn ack(row: usize) -> Result<(), ()> { /// Open the row's ports to `pid` for the rest of its life. Called once per /// claim, by the claim's first read, and never twice for a row: [`claim_row`] /// mints none while a process holds its ports. -pub fn bind(row: usize, pid: Pid) { +pub fn bind(row: &Row, pid: Pid) { + let row = row.0; let name = function(row).expect("a claimed row was filled").name; match BOUND[row].compare_exchange(NOBODY, pid.raw(), Ordering::AcqRel, Ordering::Acquire) { Ok(_) => log!("isa: {name}'s ports are pid {pid}'s"), @@ -200,11 +216,17 @@ pub fn bind(row: usize, pid: Pid) { pio::switch_to(Some(pid)); } -/// Whether `pid` holds the row's ports. +/// Whether `pid` holds the row's ports: the context switch's question, of +/// every row. pub fn bound_to(row: usize, pid: Pid) -> bool { BOUND[row].load(Ordering::Acquire) == pid.raw() } +/// Whether `pid` holds the ports of the row this claim is on. +pub fn held_by(row: &Row, pid: Pid) -> bool { + bound_to(row.0, pid) +} + /// Called from the process teardown, once every thread has left. pub fn process_ends(pid: Pid) { for (row, bound) in BOUND.iter().enumerate() { @@ -215,7 +237,8 @@ pub fn process_ends(pid: Pid) { } /// The interrupts since the last read, or `None` for none. -pub fn take_record(row: usize) -> Option { +pub fn take_record(row: &Row) -> Option { + let row = row.0; let taken = IRQ[row].take(); if taken.is_some() && IRQ[row].take_unannounced() { log!("isa: {} took its first interrupt", function(row).expect("a claimed row was filled").name); @@ -223,8 +246,8 @@ pub fn take_record(row: usize) -> Option { taken.map(|count| DeviceIrqRecord { count }) } -pub fn has_irq(row: usize) -> bool { - IRQ[row].armed() +pub fn has_irq(row: &Row) -> bool { + IRQ[row.0].armed() } /// Records one interrupt, masks the row's level lines and posts the claim's @@ -240,8 +263,11 @@ pub fn isr(row: usize) { WATCHES[row].post_in_place(); } -pub fn watch(row: usize) -> &'static IrqWatch { - &WATCHES[row] +/// It outlives the borrow it was asked with: a poll registered on it after the +/// claim's release is fired by its registrar's own recheck, which finds the +/// claim released (`ops::has_data`). +pub fn watch(row: &Row) -> &'static IrqWatch { + &WATCHES[row.0] } /// The function a filled row raises `wire` with too, if one does: a second diff --git a/kernel/src/object/device.rs b/kernel/src/object/device.rs index 54145354633..56da1273269 100644 --- a/kernel/src/object/device.rs +++ b/kernel/src/object/device.rs @@ -20,20 +20,18 @@ pub enum DeviceInfo { // Keyboard and mouse answer with events, not a description. Events, Framebuffer(FramebufferInfo, FramebufferBuffers), - /// The function, and the `pcidev` slot every call on this claim reaches it - /// through. No buffer handle beside it: a PCI claimant asks for its own - /// memory, so this mint installs nothing. - PciFunction(toyos_abi::pci::PciFunctionInfo, u8), + /// The function. No buffer handle beside it: a PCI claimant asks for its + /// own memory, so this mint installs nothing. + PciFunction(toyos_abi::pci::PciFunctionInfo), Hda(toyos_abi::hda::HdaInfo, Arc), VirtioSound(toyos_abi::virtio_sound::VirtioSoundInfo, Arc), /// Which partition, how long, and both its GUIDs; the view it moves blocks /// through is the claim's own (`device::Claim::partition`). Partition(toyos_abi::part::PartitionInfo), - /// The ports and lines granted, as the selector named them, and the `isa` - /// row they are. - Isa(toyos_abi::syscall::IsaId, usize), - /// The fixed hardware's blocks, and the `isa` row they are. - Acpi(toyos_abi::acpi::AcpiInfo, usize), + /// The ports and lines granted, as the selector named them. + Isa(toyos_abi::syscall::IsaId), + /// The fixed hardware's blocks. + Acpi(toyos_abi::acpi::AcpiInfo), } /// The two scanout buffers and the cursor plane. @@ -75,10 +73,10 @@ impl DeviceInfo { } // Nothing to install: every address in it is a size, and the memory // is what `SYS_DEVICE_DMA_ALLOC` answers later. - Self::PciFunction(info, _) => bytes(info).into(), + Self::PciFunction(info) => bytes(info).into(), Self::Partition(info) => bytes(info).into(), - Self::Isa(set, _) => set.wire().iter().flat_map(|word| word.to_ne_bytes()).collect(), - Self::Acpi(info, _) => bytes(info).into(), + Self::Isa(set) => set.wire().iter().flat_map(|word| word.to_ne_bytes()).collect(), + Self::Acpi(info) => bytes(info).into(), Self::Hda(info, pcm) => { let mut info = *info; info.pcm = install_buffers(table, &[pcm])?[0]; @@ -97,12 +95,6 @@ impl DeviceInfo { pub struct DeviceClaim { pub(super) core: ObjectCore, class: DeviceType, - /// The `pcidev` slot for a claim on one, read without the `described` lock: - /// a poll's readiness check and a `close` are both places that must not - /// take it. - pci_slot: Option, - /// The `isa` row for a claim on one, read without that lock for the same reasons. - isa_row: Option, // No Rights::DUP: at most one handle exists, so info_read needs no per-handle state. info_read: AtomicBool, described: crate::sync::Lock, @@ -118,19 +110,9 @@ struct Described { impl DeviceClaim { pub fn new(class: DeviceType, info: DeviceInfo, claim: Claim) -> Arc { - let pci_slot = match &info { - DeviceInfo::PciFunction(_, slot) => Some(*slot), - _ => None, - }; - let isa_row = match &info { - DeviceInfo::Isa(_, row) | DeviceInfo::Acpi(_, row) => Some(*row), - _ => None, - }; Arc::new(Self { core: Self::new_core(), class, - pci_slot, - isa_row, info_read: AtomicBool::new(false), described: crate::sync::Lock::new(Described { info, bytes: None }), reference: Held::new(claim), @@ -141,18 +123,21 @@ impl DeviceClaim { self.class } - /// Which `pcidev` slot this claim drives, for a claim on a PCI function. + /// `f` over the function this claim drives, with what `pcidev`'s calls + /// take: `None` for a claim on anything else, and once the last handle has + /// let the function go. /// - /// Every call the substrate answers goes through this: the handle is the - /// authority and the slot is what it names. - pub fn pci_slot(&self) -> Option { - self.pci_slot.map(usize::from) + /// Every call the substrate answers goes through this, under the lock the + /// release takes the binding with: `f` has returned before the function is + /// let go, or never runs. So `f` does not wait. + pub fn pci(&self, f: impl FnOnce(&crate::pcidev::Binding) -> R) -> Option { + self.reference.with(|claim| claim.binding().map(f)).flatten() } - /// Which `isa` row this claim holds, for a claim on an ISA function or the - /// ACPI fixed hardware. - pub fn isa_row(&self) -> Option { - self.isa_row + /// [`pci`](Self::pci), for the `isa` row of a claim on an ISA function or + /// the ACPI fixed hardware. + pub fn isa(&self, f: impl FnOnce(&crate::isa::Row) -> R) -> Option { + self.reference.with(|claim| claim.row().map(f)).flatten() } /// The view a partition claim transfers through: `None` for a claim on diff --git a/kernel/src/object/ops.rs b/kernel/src/object/ops.rs index 3ed962a6869..e795b49a99a 100644 --- a/kernel/src/object/ops.rs +++ b/kernel/src/object/ops.rs @@ -278,10 +278,10 @@ pub fn read_watch(object: &KObjectRef) -> Option { device_registry::DeviceType::Keyboard => Some(WatchRef::Static(&keyboard::WATCH)), device_registry::DeviceType::Mouse => Some(WatchRef::Static(&mouse::WATCH)), device_registry::DeviceType::PciFunction => { - d.pci_slot().map(|slot| WatchRef::Irq(crate::pcidev::watch(slot))) + d.pci(crate::pcidev::watch).map(WatchRef::Irq) } device_registry::DeviceType::Isa | device_registry::DeviceType::Acpi => { - d.isa_row().map(|row| WatchRef::Irq(crate::isa::watch(row))) + d.isa(crate::isa::watch).map(WatchRef::Irq) } device_registry::DeviceType::HdaAudio | device_registry::DeviceType::VirtioSound => { Some(WatchRef::Irq(&crate::drivers::AUDIO_WATCH)) @@ -427,10 +427,10 @@ pub fn read_device( if buf.len() < toyos_abi::pci::DeviceIrqRecord::SIZE { return Some(SyscallError::InvalidArgument.to_u64()); } - let slot = claim.pci_slot().expect("a PCI claim knows its slot"); - let record = match crate::pcidev::take_record(slot) { - Ok(record) => record?, - Err(refused) => return Some(refused.to_u64()), + let record = match claim.pci(crate::pcidev::take_record) { + Some(Ok(record)) => record?, + Some(Err(refused)) => return Some(refused.to_u64()), + None => return Some(SyscallError::Gone.to_u64()), }; buf.write_at(0, record_bytes(&record)); Some(toyos_abi::pci::DeviceIrqRecord::SIZE as u64) @@ -438,19 +438,29 @@ pub fn read_device( // The PCI shape, but the description is what binds the ports to the // reader, and nothing after it answers any other process. device_registry::DeviceType::Isa | device_registry::DeviceType::Acpi => { - let row = claim.isa_row().expect("an ISA or ACPI claim knows its row"); let pid = crate::process::current_process(); if !claim.info_read() { - crate::isa::bind(row, pid); + if claim.isa(|row| crate::isa::bind(row, pid)).is_none() { + return Some(SyscallError::Gone.to_u64()); + } return Some(claim.describe(table, buf)); } - if !crate::isa::bound_to(row, pid) { - return Some(SyscallError::PermissionDenied.to_u64()); - } - if buf.len() < toyos_abi::pci::DeviceIrqRecord::SIZE { - return Some(SyscallError::InvalidArgument.to_u64()); - } - let record = crate::isa::take_record(row)?; + let room = buf.len() >= toyos_abi::pci::DeviceIrqRecord::SIZE; + // The holder's check and the take are one borrow of the row. + let taken = claim.isa(|row| { + if !crate::isa::held_by(row, pid) { + Err(SyscallError::PermissionDenied) + } else if !room { + Err(SyscallError::InvalidArgument) + } else { + Ok(crate::isa::take_record(row)) + } + }); + let record = match taken { + Some(Ok(record)) => record?, + Some(Err(refused)) => return Some(refused.to_u64()), + None => return Some(SyscallError::Gone.to_u64()), + }; buf.write_at(0, record_bytes(&record)); Some(toyos_abi::pci::DeviceIrqRecord::SIZE as u64) } @@ -794,11 +804,14 @@ pub fn has_data(object: &KObjectRef) -> bool { KObjectRef::Device(d) => match d.class() { device_registry::DeviceType::Keyboard => keyboard::has_data(), device_registry::DeviceType::Mouse => mouse::has_data(), + // A claim its last handle has let go is ready: nothing will post + // for it again, so a poll on it is answered and never left + // registered on a watch its slot's or row's next holder posts. device_registry::DeviceType::PciFunction => { - !d.info_read() || d.pci_slot().is_some_and(crate::pcidev::has_irq) + !d.info_read() || d.pci(crate::pcidev::has_irq).unwrap_or(true) } device_registry::DeviceType::Isa | device_registry::DeviceType::Acpi => { - !d.info_read() || d.isa_row().is_some_and(crate::isa::has_irq) + !d.info_read() || d.isa(crate::isa::has_irq).unwrap_or(true) } device_registry::DeviceType::Framebuffer => true, device_registry::DeviceType::Partition => true, @@ -876,20 +889,28 @@ fn write_device(claim: &DeviceClaim, buf: &UserBytes) -> u64 { | device_registry::DeviceType::PciFunction | device_registry::DeviceType::Partition => return SyscallError::PermissionDenied.to_u64(), } - let row = claim.isa_row().expect("an ISA or ACPI claim knows its row"); - if !claim.info_read() || !crate::isa::bound_to(row, crate::process::current_process()) { + if !claim.info_read() { return SyscallError::PermissionDenied.to_u64(); } let mut word = [0u8; 4]; - if buf.len() != word.len() { - return SyscallError::InvalidArgument.to_u64(); - } - buf.read_at(0, &mut word); - if u32::from_ne_bytes(word) != toyos_abi::acpi::ACK { - return SyscallError::InvalidArgument.to_u64(); - } - match crate::isa::ack(row) { - Ok(()) => word.len() as u64, - Err(()) => SyscallError::InvalidArgument.to_u64(), + let acknowledges = buf.len() == word.len() && { + buf.read_at(0, &mut word); + u32::from_ne_bytes(word) == toyos_abi::acpi::ACK + }; + let pid = crate::process::current_process(); + // The holder's check and the unmask are one borrow of the row. + let acked = claim.isa(|row| { + if !crate::isa::held_by(row, pid) { + Err(SyscallError::PermissionDenied) + } else if !acknowledges { + Err(SyscallError::InvalidArgument) + } else { + crate::isa::ack(row).map_err(|()| SyscallError::InvalidArgument) + } + }); + match acked { + Some(Ok(())) => word.len() as u64, + Some(Err(refused)) => refused.to_u64(), + None => SyscallError::Gone.to_u64(), } } diff --git a/kernel/src/pcidev/mod.rs b/kernel/src/pcidev/mod.rs index 1590f7a8533..fd9468860df 100644 --- a/kernel/src/pcidev/mod.rs +++ b/kernel/src/pcidev/mod.rs @@ -94,6 +94,14 @@ //! grants are still mapped; the next claim of the slot waits the reset out and //! writes back what the reset cleared and [`bring_up`] does not write itself //! ([`Kept`]) before it reads a register of the function. +//! +//! **A call names its function by its claim's [`Binding`] and never by a slot +//! number.** A slot is the next claim's once its holder's release is over, so +//! a number carried from a claim to a call would be another holder's function +//! by the time it was used. The binding is lent by the claim under the lock +//! its release takes it with (`object::Held`): a call runs wholly before the +//! release or finds no binding. Only the two handlers name a slot, by the +//! vector and the requester the hardware gave them. /// No `crate::` reference, so `kernel-loom` compiles it and models the /// interleaving no guest test lands on. @@ -368,6 +376,23 @@ static MACHINE: Lock = Lock::new(Machine { /// takes it once the teardown is over. static SLOTS: Lock<[Slot; MAX_FUNCTIONS]> = Lock::new([Slot::Free; MAX_FUNCTIONS]); +/// One claim's hold on a slot, from its reservation to the release its drop +/// is: what every call on the claim is made with. +/// +/// Neither `Copy` nor `Clone` and made only by [`claim`], so there is one per +/// reservation, and a reference to it is a slot no other claim holds. +pub struct Binding { + slot: usize, + /// The requester the slot is held for. + who: u16, +} + +impl Drop for Binding { + fn drop(&mut self) { + release(self.slot); + } +} + /// Take a slot for `who`, or say why not. /// /// The scan and the take are one critical section: two claims arriving together @@ -408,10 +433,9 @@ pub fn segment() -> u16 { MACHINE.lock().segment } -/// The function a claim's slot holds, or `None` for a slot nobody holds. -pub fn held_at(slot: usize) -> Option { - let Some(Slot::Held(who)) = SLOTS.lock().get(slot).copied() else { return None }; - Some(addr_of(MACHINE.lock().segment, who)) +/// The function a claim holds. +pub fn held_at(binding: &Binding) -> toyos_abi::inventory::PciAddr { + addr_of(MACHINE.lock().segment, binding.who) } pub(crate) fn requester(pci: &PciDevice) -> u16 { @@ -672,7 +696,7 @@ impl core::fmt::Display for Refusal { /// Claim `id`, bring the function up to the point a driver takes over, and /// answer what that driver needs to know. -pub fn claim(id: PciId) -> Result<(PciFunctionInfo, u8, Claim), ClaimError> { +pub fn claim(id: PciId) -> Result<(PciFunctionInfo, Claim), ClaimError> { let (pci, driven) = { let machine = MACHINE.lock(); let mut found: Option = None; @@ -708,9 +732,10 @@ pub fn claim(id: PciId) -> Result<(PciFunctionInfo, u8, Claim), ClaimError> { // `Owned` before `Exhausted`: a second claim on a function somebody holds // is a different fact from a machine with no slot left. - let slot = reserve(requester(&pci))?; - // Dropped by every refusal below, and `release` is what gives the slot back. - let claim = Claim::pci(slot); + let who = requester(&pci); + let slot = reserve(who)?; + // Dropped by every refusal below, which is what gives the slot back. + let binding = Binding { slot, who }; match bring_up(pci, id, slot) { Ok(mut bound) => { @@ -735,7 +760,7 @@ pub fn claim(id: PciId) -> Result<(PciFunctionInfo, u8, Claim), ClaimError> { id.device, VECTORS[slot], ); - Ok((info, slot as u8, claim)) + Ok((info, Claim::pci(binding))) } Err(why) => { log!( @@ -1395,7 +1420,7 @@ fn alone_in_its_page(claimed: &PciDevice, index: u8, at: u64, span: u64) { /// The order is what makes a dying driver safe. A page freed while the function /// could still reach it is a device writing into memory the allocator has /// already handed to somebody else. -pub fn release(slot: usize) { +fn release(slot: usize) { // Two statements, because edition 2021 keeps an `if let`'s scrutinee // temporaries alive to the end of its block: `BOUND[slot]`'s guard would be // held across the unmaps, the reset and the watch's answer. @@ -1484,22 +1509,22 @@ fn tear_down(slot: usize, mut bound: Bound) { ); } -/// What every call a claim answers checks first. +/// What every call a claim answers checks first, over what that claim bound. fn with_bound( - slot: usize, + binding: &Binding, f: impl FnOnce(&mut Bound) -> Result, ) -> Result { - if IRQ[slot].faulted() { + if IRQ[binding.slot].faulted() { return Err(SyscallError::Io); } - let mut guard = BOUND[slot].lock(); - let bound = guard.as_mut().ok_or(SyscallError::NotFound)?; - f(bound) + let mut guard = BOUND[binding.slot].lock(); + // Bound before `claim` hands the binding out, and unbound only by its drop. + f(guard.as_mut().expect("pcidev: a claim's binding has its function bound")) } /// One memory BAR as an object to map; the same object every time. -pub fn bar_object(slot: usize, index: u64) -> Result, SyscallError> { - with_bound(slot, |bound| { +pub fn bar_object(binding: &Binding, index: u64) -> Result, SyscallError> { + with_bound(binding, |bound| { let index = usize::try_from(index).map_err(|_| SyscallError::InvalidArgument)?; if index >= BARS || bound.bar_bytes[index] == 0 { return Err(SyscallError::InvalidArgument); @@ -1522,10 +1547,11 @@ pub fn bar_object(slot: usize, index: u64) -> Result, Sysca /// Memory this function may reach, and nothing else may. pub fn dma_alloc( - slot: usize, + binding: &Binding, bytes: u64, ) -> Result<(Arc, u64, u64), SyscallError> { - with_bound(slot, |bound| { + let slot = binding.slot; + with_bound(binding, |bound| { if bytes == 0 || bytes > MAX_GRANT_BYTES { return Err(SyscallError::InvalidArgument); } @@ -1574,8 +1600,9 @@ pub fn dma_alloc( /// Found by its memory and not by position: a [`dma_map`] on another thread of /// the same holder can push between the grant and this, and the address the /// caller was *told* is not always the address the grant is at. -pub fn dma_undo(slot: usize, memory: &Arc) { - let _ = with_bound(slot, |bound| { +pub fn dma_undo(binding: &Binding, memory: &Arc) { + let slot = binding.slot; + let _ = with_bound(binding, |bound| { let Some(index) = bound.grants.iter().position(|grant| Arc::ptr_eq(&grant.memory, memory)) else { return Ok(()); @@ -1605,8 +1632,9 @@ pub fn dma_undo(slot: usize, memory: &Arc) { /// Counted against [`MAX_GRANT_TOTAL`] with the claim's own grants, so a holder /// mapping every region its clients send is bounded by the same number; and a /// window with no run of the region's length left free is the same refusal. -pub fn dma_map(slot: usize, region: &Arc) -> Result<(u64, u64), SyscallError> { - with_bound(slot, |bound| { +pub fn dma_map(binding: &Binding, region: &Arc) -> Result<(u64, u64), SyscallError> { + let slot = binding.slot; + with_bound(binding, |bound| { let (phys, span) = region.ram().ok_or(SyscallError::InvalidArgument)?; // One region, one address: a second mapping of it would be two grants // naming one set of pages, and taking either back would leave the @@ -1637,8 +1665,13 @@ pub fn dma_map(slot: usize, region: &Arc) -> Result<(u64, u64), /// there is refused at the unit and recorded against this claim — and only /// after that is the region's `Arc` let go, so its pages can never be freed /// under a translation that still names them. -pub fn dma_unmap(slot: usize, at: u64) -> Result<(), SyscallError> { - let grant = with_bound(slot, |bound| { +/// +/// **Answers the region, for the caller to let go with nothing held**: the +/// last `Arc` of one can run its zero-handle teardown, which takes other +/// objects' locks, and the caller holds its claim's. +pub fn dma_unmap(binding: &Binding, at: u64) -> Result, SyscallError> { + let slot = binding.slot; + with_bound(binding, |bound| { let index = bound .grants .iter() @@ -1648,12 +1681,8 @@ pub fn dma_unmap(slot: usize, at: u64) -> Result<(), SyscallError> { if let Err(why) = bound.space.unmap(grant.at, grant.bytes) { panic!("pcidev: slot {slot} could not take {:#x} back: {why}", grant.at); } - Ok(grant) - })?; - // Outside the slot's lock: the last `Arc` of a region can run its - // zero-handle teardown, which takes other objects' locks. - drop(grant); - Ok(()) + Ok(grant.memory) + }) } /// The window a claim's configuration reads are checked against, for the one @@ -1672,8 +1701,8 @@ pub fn config_window(offset: u64, width: RegWidth) -> Result Result { - with_bound(slot, |bound| { +pub fn config_read(binding: &Binding, at: Register, width: RegWidth) -> Result { + with_bound(binding, |bound| { Ok(match width { RegWidth::U8 => bound.pci.read_config_u8(at.offset()) as u32, RegWidth::U16 => bound.pci.read_config_u16(at.offset()) as u32, @@ -1685,7 +1714,8 @@ pub fn config_read(slot: usize, at: Register, width: RegWidth) -> Result Result, SyscallError> { +pub fn take_record(binding: &Binding) -> Result, SyscallError> { + let slot = binding.slot; if IRQ[slot].faulted() { return Err(SyscallError::Io); } @@ -1698,8 +1728,8 @@ pub fn take_record(slot: usize) -> Result, SyscallError> /// Whether a read of the claim answers at once: a message is waiting, or the /// refusal is. -pub fn has_irq(slot: usize) -> bool { - IRQ[slot].armed() || IRQ[slot].faulted() +pub fn has_irq(binding: &Binding) -> bool { + IRQ[binding.slot].armed() || IRQ[binding.slot].faulted() } /// Records one message and posts the claim's watch. Called from the vector's @@ -1721,7 +1751,11 @@ pub fn note_fault(slot: usize) { crate::preempt::set_need_resched(); } -/// The watch of the function a claim holds at `slot`. -pub fn watch(slot: usize) -> &'static IrqWatch { - &WATCHES[slot] +/// The watch of the function a claim holds. +/// +/// It outlives the borrow it was asked with, so a poll can be registered on it +/// after the claim's release: its registrar's own recheck then finds the claim +/// released and fires it (`ops::has_data`). +pub fn watch(binding: &Binding) -> &'static IrqWatch { + &WATCHES[binding.slot] } diff --git a/kernel/src/syscall/device.rs b/kernel/src/syscall/device.rs index da73a9691cd..6b0265fd6b9 100644 --- a/kernel/src/syscall/device.rs +++ b/kernel/src/syscall/device.rs @@ -3,6 +3,9 @@ //! [`holds_claim`] checks a claim's class against what the syscall requires; //! device register access and protocol stay in the device's own code. +use alloc::sync::Arc; + +use crate::object::device::DeviceClaim; use crate::object::{ops, KObjectRef}; use crate::user_ptr::SyscallContext; use crate::UserAddr; @@ -39,7 +42,8 @@ pub(super) fn holds_claim( enum RegTarget { Hda, VirtioSound, - /// A claimed PCI function's own config space, **read-only**. + /// A claimed PCI function's own config space, **read-only**, through the + /// claim itself: what names the function is lent by it for each access. /// /// There is no writing counterpart and that is the point: a driver cannot /// find its own registers without its capability chain — virtio's four @@ -48,7 +52,7 @@ enum RegTarget { /// mastering, the BARs and the MSI-X control word are all writes, and /// refusing them is what stops a holder aiming its device at memory or its /// interrupt at a vector. - PciConfig(usize), + PciConfig(Arc), } /// Reads or writes one register of the claimed device; the device owns its allow-list. @@ -57,14 +61,12 @@ pub(super) fn sys_device_reg(handle: RawHandle, offset: u64, width: u64, value: return SyscallError::InvalidArgument.to_u64(); }; let target = process::with_process_data(|data| { - data.handles - .get::(handle, Rights::NONE) - .map(|claim| match claim.class() { - device::DeviceType::HdaAudio => Some(RegTarget::Hda), - device::DeviceType::VirtioSound => Some(RegTarget::VirtioSound), - device::DeviceType::PciFunction => claim.pci_slot().map(RegTarget::PciConfig), - _ => None, - }) + data.handles.get::(handle, Rights::NONE).map(|claim| match claim.class() { + device::DeviceType::HdaAudio => Some(RegTarget::Hda), + device::DeviceType::VirtioSound => Some(RegTarget::VirtioSound), + device::DeviceType::PciFunction => Some(RegTarget::PciConfig(claim)), + _ => None, + }) }); // `refuse` must run with nothing held; the guard above has already been dropped. let target = match target { @@ -84,8 +86,10 @@ pub(super) fn sys_device_reg(handle: RawHandle, offset: u64, width: u64, value: // witness `pcidev::config_window` answers is the only thing // the register read takes, so an unchecked number cannot // reach the register file. - RegTarget::PciConfig(slot) => match crate::pcidev::config_window(offset, width) { - Ok(at) => crate::pcidev::config_read(slot, at, width), + RegTarget::PciConfig(claim) => match crate::pcidev::config_window(offset, width) { + Ok(at) => on_function(&claim, |function| { + crate::pcidev::config_read(function, at, width) + }), Err(_) => Err(SyscallError::InvalidArgument), }, }; @@ -158,34 +162,48 @@ pub(super) fn sys_device_claim(syscap: RawHandle, class: u64, selector: [u64; 2] }) } -/// The `pcidev` slot a claim handle names, with the right every substrate call +/// The PCI function claim a handle names, with the right every substrate call /// demands. /// /// [`Rights::WRITE`] for all three: each one gives the holder control of the /// device — a register window, memory the device can reach, its interrupt — and /// a claim carries no `DUP`, so no narrower handle to one can exist. -fn pci_slot(handle: RawHandle) -> Result { - let slot = process::with_process_data(|data| { - data.handles - .get::(handle, Rights::WRITE) - .map(|claim| claim.pci_slot()) +/// +/// **The claim, and nothing read out of it**: a call reaches its function only +/// through [`on_function`], so what it acts on is this claim's or nothing. +fn pci_claim(handle: RawHandle) -> Result, crate::object::Refusal> { + let claim = process::with_process_data(|data| { + data.handles.get::(handle, Rights::WRITE) })?; // A claim of another class is the wrong kind of thing here, not a missing // device: it names a device this call has no meaning for. - slot.ok_or(crate::object::HandleError::WrongType { - held: device::DeviceType::PciFunction.class_name(), - wanted: "a PCI function claim", + match claim.class() { + device::DeviceType::PciFunction => Ok(claim), + other => Err(crate::object::HandleError::WrongType { + held: other.class_name(), + wanted: "a PCI function claim", + } + .into()), } - .into()) +} + +/// One `pcidev` call on the function `claim` drives, or [`SyscallError::Gone`] +/// for a claim whose last handle went under the call: a sibling thread closed +/// it, and whatever holds its slot now is not this caller's. +fn on_function( + claim: &DeviceClaim, + call: impl FnOnce(&crate::pcidev::Binding) -> Result, +) -> Result { + claim.pci(call).unwrap_or(Err(SyscallError::Gone)) } /// One memory BAR of a claimed function, as an object to map. pub(super) fn sys_device_bar_map(handle: RawHandle, index: u64) -> u64 { - let slot = match pci_slot(handle) { - Ok(slot) => slot, + let claim = match pci_claim(handle) { + Ok(claim) => claim, Err(e) => return e.refuse(), }; - let object = match crate::pcidev::bar_object(slot, index) { + let object = match on_function(&claim, |function| crate::pcidev::bar_object(function, index)) { Ok(object) => object, Err(e) => return e.to_u64(), }; @@ -201,8 +219,8 @@ pub(super) fn sys_device_dma_alloc( bytes: u64, out: UserAddr, ) -> u64 { - let slot = match pci_slot(handle) { - Ok(slot) => slot, + let claim = match pci_claim(handle) { + Ok(claim) => claim, Err(e) => return e.refuse(), }; // The output window is taken before the allocation, the same order @@ -212,7 +230,8 @@ pub(super) fn sys_device_dma_alloc( let Some(mut window) = ctx.user_bytes_mut(out, len) else { return SyscallError::BadAddress.to_u64(); }; - let (memory, device_addr, granted) = match crate::pcidev::dma_alloc(slot, bytes) { + let granted = on_function(&claim, |function| crate::pcidev::dma_alloc(function, bytes)); + let (memory, device_addr, granted) = match granted { Ok(grant) => grant, Err(e) => return e.to_u64(), }; @@ -224,8 +243,9 @@ pub(super) fn sys_device_dma_alloc( // The grant goes back rather than staying mapped with nothing naming // it: a caller left holding neither the handle nor the quota would be // refused every later grant with no way out but dying. + // A claim released since took the grant back itself. Err(e) => { - crate::pcidev::dma_undo(slot, &memory); + claim.pci(|function| crate::pcidev::dma_undo(function, &memory)); return e.to_u64(); } }; @@ -254,8 +274,8 @@ pub(super) fn sys_device_dma_map( region: RawHandle, out: UserAddr, ) -> u64 { - let slot = match pci_slot(handle) { - Ok(slot) => slot, + let claim = match pci_claim(handle) { + Ok(claim) => claim, Err(e) => return e.refuse(), }; let memory = match process::with_process_data(|data| { @@ -268,7 +288,8 @@ pub(super) fn sys_device_dma_map( let Some(mut window) = ctx.user_bytes_mut(out, len) else { return SyscallError::BadAddress.to_u64(); }; - let (device_addr, bytes) = match crate::pcidev::dma_map(slot, &memory) { + let mapped = on_function(&claim, |function| crate::pcidev::dma_map(function, &memory)); + let (device_addr, bytes) = match mapped { Ok(mapped) => mapped, Err(e) => return e.to_u64(), }; @@ -288,12 +309,13 @@ pub(super) fn sys_device_dma_map( /// Take back what [`sys_device_dma_map`] put at `device_addr`. pub(super) fn sys_device_dma_unmap(handle: RawHandle, device_addr: u64) -> u64 { - let slot = match pci_slot(handle) { - Ok(slot) => slot, + let claim = match pci_claim(handle) { + Ok(claim) => claim, Err(e) => return e.refuse(), }; - match crate::pcidev::dma_unmap(slot, device_addr) { - Ok(()) => 0, + // The region is let go here, with the claim's lock given back. + match on_function(&claim, |function| crate::pcidev::dma_unmap(function, device_addr)) { + Ok(_region) => 0, Err(e) => e.to_u64(), } } From 6cc267dfb85e03e3992f92e35c8b48b250e916b7 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 8 Oct 2026 11:52:57 +0200 Subject: [PATCH 2/3] A poll on a claim is registered with what the claim lends pcidev::watch and isa::watch answered a &'static IrqWatch from inside the claim's borrow, and two callers acted on it after the lock was given back: ops::close cancelled polls on it, and inbox::arm registered on it. A release and the slot's or row's next claim between the borrow and the act answered the next holder's polls as gone, or left a poll of the old process on the watch the next holder's interrupt posts: one bit of another process's device activity in the old process's ring. That is issues/a-poll-registered-racing-its-own-close-outlives-the-release.md, by the road the first commit of this branch left open; the only thing between it and the next holder was has_data answering ready for a released claim. The reviewer's mutation of that answer (unwrap_or(true) to unwrap_or(false), both lines) was run at b07e7db72 before this change: it builds, and acpi_power_button, iommu_virtio_platform and machine_shutdown stay green, exit 0 each. Nothing in the tree saw it. Now a poll reaches a slot's or a row's watch only through pcidev::add_poll(&Binding) or isa::add_poll(&Row), and a cancel only through their cancel_polls, each made inside the borrow (DeviceClaim::add_poll, DeviceClaim::cancel_polls; ops::WatchRef::Claim carries the claim, not a watch). A registration therefore lands before the release, whose own cancel answers it before the slot or row can be claimed again, or finds the claim released and is ended by its registrant. The two watch functions are deleted, and has_data answers a released claim as having nothing to read: the poll's end no longer rests on it. The issue is deleted with its line in pcidev's and isa's headers. Its exit asked for a guest test of an interval that no longer exists. What remains of it is the same registration on a class claim's static watch (the audio classes and the mouse), recorded in issues/a-class-claims-call-in-flight-acts-after-the-claims-release.md, whose exit now names the property and admits the classes' retirement, and whose owner, like the handler finding's, is said to be nobody. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RvnWQFcMuGqTHYhvSnTe8A --- ...in-flight-acts-after-the-claims-release.md | 26 +++++++++++++---- ...t-it-read-before-the-slot-changed-hands.md | 2 +- ...cing-its-own-close-outlives-the-release.md | 28 ------------------- kernel/src/device.rs | 18 ++++++++++++ kernel/src/isa.rs | 17 +++++++---- kernel/src/object/device.rs | 19 +++++++++++++ kernel/src/object/ops.rs | 25 +++++++++-------- kernel/src/pcidev/mod.rs | 28 +++++++++++++------ 8 files changed, 101 insertions(+), 62 deletions(-) delete mode 100644 issues/a-poll-registered-racing-its-own-close-outlives-the-release.md diff --git a/issues/a-class-claims-call-in-flight-acts-after-the-claims-release.md b/issues/a-class-claims-call-in-flight-acts-after-the-claims-release.md index 766bdc07021..62367bf3cee 100644 --- a/issues/a-class-claims-call-in-flight-acts-after-the-claims-release.md +++ b/issues/a-class-claims-call-in-flight-acts-after-the-claims-release.md @@ -26,14 +26,28 @@ on the display, that the second process now holds. A claim on a PCI function, an ISA function or the ACPI fixed hardware does not have this: its calls borrow what names the device from the claim under the -lock its release takes it with (`DeviceClaim::pci`, `DeviceClaim::isa`). The -same borrow closes these — the act made inside the claim's `Held`. +lock its release takes it with (`DeviceClaim::pci`, `DeviceClaim::isa`). That +borrow is no fix to copy here as it stands: `SYS_GPU_SET_RESOLUTION` allocates, +and the claim's lock is a spinlock. + +**A poll has the same shape on these classes.** The watch a poll on an audio +claim or the mouse's registers on is the class's one `static` +(`drivers::AUDIO_WATCH`, `mouse::WATCH`, by `ops::read_watch`), and +`inbox::arm` registers on it with nothing held. A sibling thread's close +between the resolve and the registration lets the poll land after the close +answered the watch's polls (`ops::close`, and for audio `Claim`'s drop in +`kernel/src/device.rs` too), and the class's next holder's first interrupt +fires it into the old process's ring: one bit of another process's device +activity. A claim on a PCI +function, an ISA function or the ACPI fixed hardware registers with what the +claim lends (`DeviceClaim::add_poll`) and does not have it. **Read from the code, not run.** A guest cannot order a close and a second claim between the two steps of one syscall. -**Exit condition**: every call on a class claim acts under the lock its -claim's release takes, or is refused, and no syscall reaches a kernel-driven -device on a `holds_claim` answer alone. +**Exit condition**: no call on a released class claim reaches the device, and +no poll registered on one stays on its watch, once the class's next claim is +minted; or the class is gone from the kernel, as +`issues/every-driver-is-still-in-the-kernel.md` retires each of them. -**Owner**: whoever holds `issues/every-driver-is-still-in-the-kernel.md`. +**Owner**: none. That track is open and nobody holds it. diff --git a/issues/a-handler-tells-a-slot-it-read-before-the-slot-changed-hands.md b/issues/a-handler-tells-a-slot-it-read-before-the-slot-changed-hands.md index b809074d1d6..75b4c9a9e90 100644 --- a/issues/a-handler-tells-a-slot-it-read-before-the-slot-changed-hands.md +++ b/issues/a-handler-tells-a-slot-it-read-before-the-slot-changed-hands.md @@ -32,4 +32,4 @@ not run. handler's write is refused by a record that has moved on, or this is folded into `kernel/src/pcidev/mod.rs`'s header as the bound it is. -**Owner**: whoever holds `issues/every-driver-is-still-in-the-kernel.md`. +**Owner**: none. No track covers a slot's record. diff --git a/issues/a-poll-registered-racing-its-own-close-outlives-the-release.md b/issues/a-poll-registered-racing-its-own-close-outlives-the-release.md deleted file mode 100644 index 7cbc6e084ea..00000000000 --- a/issues/a-poll-registered-racing-its-own-close-outlives-the-release.md +++ /dev/null @@ -1,28 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-09-26 ---- - -# A poll registered racing its own process's close outlives the release - -`kernel/src/inbox/mod.rs`'s `process_watch` resolves the handle, clones the -object out, and only then `add_poll`s onto the object's watch with no lock -held. A PCI function's watch is a per-slot `static` -(`kernel/src/pcidev/mod.rs`'s `WATCHES`), and the release's `tear_down` answers -every poll on it with `cancel_polls`. A sibling thread of the same process that -closes the claim between the resolve and the `add_poll` lets the poll land on -the slot's watch *after* that cancel: the slot's next holder's first interrupt -then fires it into the old process's ring — one bit of another process's device -activity, across the isolation boundary. - -It predates the watch: main's `inbox::add_watcher` pushed onto -`pcidev`'s per-slot watcher list in the same order. - -## Exit condition - -A poll cannot reach a watch whose object has ended — the registration rechecks -the object's life after its `add_poll` and answers itself as gone — with a -guest test that closes a claim from one thread while another submits -`OP_WATCH` on it, and a second process claiming the slot afterwards sees no -completion arrive in the first one's ring. diff --git a/kernel/src/device.rs b/kernel/src/device.rs index c3295b400d5..8e464e20618 100644 --- a/kernel/src/device.rs +++ b/kernel/src/device.rs @@ -89,6 +89,24 @@ impl Claim { } } + /// Register a poll on the watch of the function or row this claim holds. + pub(crate) fn add_poll(&self, entry: crate::inbox::PollEntry) { + match &self.what { + Claimed::PciFunction(binding) => crate::pcidev::add_poll(binding, entry), + Claimed::Isa(row) | Claimed::Acpi(row) => crate::isa::add_poll(row, entry), + Claimed::Class(_) | Claimed::Partition(_) => unreachable!("a poll registered through a claim on no function or row"), + } + } + + /// Answer every poll on the function or row this claim holds as gone. + pub(crate) fn cancel_polls(&self) { + match &self.what { + Claimed::PciFunction(binding) => crate::pcidev::cancel_polls(binding), + Claimed::Isa(row) | Claimed::Acpi(row) => crate::isa::cancel_polls(row), + Claimed::Class(_) | Claimed::Partition(_) => unreachable!("a poll cancelled through a claim on no function or row"), + } + } + /// The view a partition claim transfers through, or `None` for any other. pub(crate) fn partition(&self) -> Option<&crate::block::Partition> { match &self.what { diff --git a/kernel/src/isa.rs b/kernel/src/isa.rs index 918e522e5de..ea152103aad 100644 --- a/kernel/src/isa.rs +++ b/kernel/src/isa.rs @@ -31,7 +31,8 @@ //! **A call on a claim is made with the claim's [`Row`] and never with a row //! number**, lent under the lock the claim's release takes it with //! (`object::Held`): the call runs wholly before the release or finds no row, -//! so none acknowledges, reads or waits on a row its claim has given up. +//! so none acknowledges, reads or registers a poll on a row its claim has +//! given up, and the release answers every poll registered before it. use alloc::boxed::Box; use alloc::vec::Vec; @@ -43,6 +44,7 @@ use toyos_userbound::Ports; use crate::arch::pio; use crate::device::ClaimError; +use crate::inbox::PollEntry; use crate::pcidev::record::Interrupt; use crate::process::Pid; use crate::sync::Lock; @@ -263,11 +265,14 @@ pub fn isr(row: usize) { WATCHES[row].post_in_place(); } -/// It outlives the borrow it was asked with: a poll registered on it after the -/// claim's release is fired by its registrar's own recheck, which finds the -/// claim released (`ops::has_data`). -pub fn watch(row: &Row) -> &'static IrqWatch { - &WATCHES[row.0] +/// Register a poll on the watch of the row a claim holds. +pub(crate) fn add_poll(row: &Row, entry: PollEntry) { + WATCHES[row.0].add_poll(entry); +} + +/// Answer every poll on the row a claim holds as gone. +pub fn cancel_polls(row: &Row) { + WATCHES[row.0].cancel_polls(); } /// The function a filled row raises `wire` with too, if one does: a second diff --git a/kernel/src/object/device.rs b/kernel/src/object/device.rs index 56da1273269..4080539d050 100644 --- a/kernel/src/object/device.rs +++ b/kernel/src/object/device.rs @@ -140,6 +140,25 @@ impl DeviceClaim { self.reference.with(|claim| claim.row().map(f)).flatten() } + /// Register `entry` on the watch of the function or row this claim holds, + /// with what the claim lends, so before its release: the release answers + /// every poll registered before it, and one that finds the claim released + /// is ended here, by its registrant. No poll is left for the interrupts of + /// whoever holds the slot or the row next. + pub(crate) fn add_poll(&self, entry: crate::inbox::PollEntry) { + let mut entry = Some(entry); + self.reference.with(|claim| claim.add_poll(entry.take().expect("lent once"))); + if let Some(entry) = entry { + kernel::sched::watch::Ring::fire(&entry, kernel::sched::watch::Fire::Gone); + } + } + + /// Answer every poll on this claim's function or row as gone; its release + /// has, or will, where the last handle has let it go. + pub(crate) fn cancel_polls(&self) { + self.reference.with(Claim::cancel_polls); + } + /// The view a partition claim transfers through: `None` for a claim on /// anything else, and once the last handle has let the partition go. /// diff --git a/kernel/src/object/ops.rs b/kernel/src/object/ops.rs index e795b49a99a..7c68e4d487d 100644 --- a/kernel/src/object/ops.rs +++ b/kernel/src/object/ops.rs @@ -243,8 +243,11 @@ pub fn pipe_write(object: &KObjectRef) -> Option<(PipeId, WaitClass)> { pub enum WatchRef { Static(&'static Watch), Shared(Arc), - /// A device's, which its interrupt handler posts. + /// A device class's, which its interrupt handler posts. Irq(&'static IrqWatch), + /// The watch of the function or row a claim holds: named by the claim, + /// and reached only with what the claim lends. + Claim(Arc), } impl WatchRef { @@ -253,6 +256,7 @@ impl WatchRef { Self::Static(watch) => watch.add_poll(entry), Self::Shared(watch) => watch.add_poll(entry), Self::Irq(watch) => watch.add_poll(entry), + Self::Claim(claim) => claim.add_poll(entry), } } @@ -261,6 +265,7 @@ impl WatchRef { Self::Static(watch) => watch.cancel_polls(), Self::Shared(watch) => watch.cancel_polls(), Self::Irq(watch) => watch.cancel_polls(), + Self::Claim(claim) => claim.cancel_polls(), } } } @@ -277,12 +282,9 @@ pub fn read_watch(object: &KObjectRef) -> Option { KObjectRef::Device(d) => match d.class() { device_registry::DeviceType::Keyboard => Some(WatchRef::Static(&keyboard::WATCH)), device_registry::DeviceType::Mouse => Some(WatchRef::Static(&mouse::WATCH)), - device_registry::DeviceType::PciFunction => { - d.pci(crate::pcidev::watch).map(WatchRef::Irq) - } - device_registry::DeviceType::Isa | device_registry::DeviceType::Acpi => { - d.isa(crate::isa::watch).map(WatchRef::Irq) - } + device_registry::DeviceType::PciFunction + | device_registry::DeviceType::Isa + | device_registry::DeviceType::Acpi => Some(WatchRef::Claim(d.clone())), device_registry::DeviceType::HdaAudio | device_registry::DeviceType::VirtioSound => { Some(WatchRef::Irq(&crate::drivers::AUDIO_WATCH)) } @@ -804,14 +806,13 @@ pub fn has_data(object: &KObjectRef) -> bool { KObjectRef::Device(d) => match d.class() { device_registry::DeviceType::Keyboard => keyboard::has_data(), device_registry::DeviceType::Mouse => mouse::has_data(), - // A claim its last handle has let go is ready: nothing will post - // for it again, so a poll on it is answered and never left - // registered on a watch its slot's or row's next holder posts. + // A claim its last handle has let go has nothing to read: a poll + // on it is ended where it registers (`DeviceClaim::add_poll`). device_registry::DeviceType::PciFunction => { - !d.info_read() || d.pci(crate::pcidev::has_irq).unwrap_or(true) + !d.info_read() || d.pci(crate::pcidev::has_irq).unwrap_or(false) } device_registry::DeviceType::Isa | device_registry::DeviceType::Acpi => { - !d.info_read() || d.isa(crate::isa::has_irq).unwrap_or(true) + !d.info_read() || d.isa(crate::isa::has_irq).unwrap_or(false) } device_registry::DeviceType::Framebuffer => true, device_registry::DeviceType::Partition => true, diff --git a/kernel/src/pcidev/mod.rs b/kernel/src/pcidev/mod.rs index 03ce31ef7fd..cd57f712573 100644 --- a/kernel/src/pcidev/mod.rs +++ b/kernel/src/pcidev/mod.rs @@ -100,8 +100,16 @@ //! a number carried from a claim to a call would be another holder's function //! by the time it was used. The binding is lent by the claim under the lock //! its release takes it with (`object::Held`): a call runs wholly before the -//! release or finds no binding. Only the two handlers name a slot, by the -//! vector and the requester the hardware gave them. +//! release or finds no binding. Outside a claim's own mint and release, only +//! the two handlers name a slot, by the vector and the requester the hardware +//! gave them. +//! +//! **A poll is on a slot's watch only while its claim holds the slot.** It is +//! registered with the binding ([`add_poll`]), so before the release, whose +//! own answer to every poll on the watch comes before the slot can be +//! reserved again; a registration that finds the claim released is ended by +//! its registrant (`DeviceClaim::add_poll`). So a slot's next holder's +//! interrupt fires no poll of the last one's, in whatever ring. /// No `crate::` reference, so `kernel-loom` compiles it and models the /// interleaving no guest test lands on. @@ -128,6 +136,7 @@ use crate::mm::policy::{CachePolicy, MmioPolicy}; use crate::mm::{align_2m, DirectMap, Mmio, PAGE_2M}; use crate::object::shm::{Region, SharedMemObject}; use crate::sync::Lock; +use crate::inbox::PollEntry; use crate::watch::IrqWatch; /// How many functions this machine can hand out at once. @@ -1763,11 +1772,12 @@ pub fn note_fault(slot: usize) { crate::preempt::set_need_resched(); } -/// The watch of the function a claim holds. -/// -/// It outlives the borrow it was asked with, so a poll can be registered on it -/// after the claim's release: its registrar's own recheck then finds the claim -/// released and fires it (`ops::has_data`). -pub fn watch(binding: &Binding) -> &'static IrqWatch { - &WATCHES[binding.slot] +/// Register a poll on the watch of the function a claim holds. +pub(crate) fn add_poll(binding: &Binding, entry: PollEntry) { + WATCHES[binding.slot].add_poll(entry); +} + +/// Answer every poll on the function a claim holds as gone. +pub fn cancel_polls(binding: &Binding) { + WATCHES[binding.slot].cancel_polls(); } From 5d1469615e330bdd6df2df7c01883832e9245610 Mon Sep 17 00:00:00 2001 From: japabu Date: Thu, 8 Oct 2026 12:44:31 +0200 Subject: [PATCH 3/3] Answer review round 2: a refusal lets its claim go first, and a claim's close ends no poll sys_acpi on a claim of another class, and sys_device_dma_map on a bad region handle, called a refusal that does not return with the claim's Arc on the stack: the DeviceClaim and its census count were stranded with the process. Each lets the claim go first, as pci_claim and sys_partition_transfer do. ops::close answered a claim's polls under the claim's lock and then queued the release that answers them itself (pcidev's tear_down, isa::Row's drop). A claim has one handle, so its close is its release: close_ends_polls is false for a PCI function, an ISA row and the ACPI row, and DeviceClaim::cancel_polls, Claim::cancel_polls, pcidev::cancel_polls and isa::cancel_polls are deleted. The answer now comes with the deferred release, as everything else a claim's close gives back does. Two issues filed. The whole-suite red of acpi_mediated_access is main's: main's nightly at b6bcb9691, without this branch, stalls the same way under TCG (run 37758546165), and the line the test waits for is logged by the power-off after the console's last drain. lan_talk is unread on the T14 while the bench's wired path is down. The reset issue's sentence about pcidev::release's one caller is made true of this branch: it is Binding's drop. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RvnWQFcMuGqTHYhvSnTe8A --- ...leaves-every-claimed-pci-function-armed.md | 6 +- ...the-t14-since-the-claims-binding-landed.md | 28 +++++++++ ...-is-logged-after-the-last-console-drain.md | 57 +++++++++++++++++++ kernel/src/device.rs | 9 --- kernel/src/isa.rs | 5 -- kernel/src/object/device.rs | 6 -- kernel/src/object/ops.rs | 13 +++-- kernel/src/pcidev/mod.rs | 5 -- kernel/src/syscall/device.rs | 12 +++- 9 files changed, 105 insertions(+), 36 deletions(-) create mode 100644 issues/lan-talk-is-unread-on-the-t14-since-the-claims-binding-landed.md create mode 100644 issues/the-power-offs-global-lock-line-is-logged-after-the-last-console-drain.md diff --git a/issues/a-reset-stops-xhci-and-leaves-every-claimed-pci-function-armed.md b/issues/a-reset-stops-xhci-and-leaves-every-claimed-pci-function-armed.md index df7628e42e6..49d0d2dc9bd 100644 --- a/issues/a-reset-stops-xhci-and-leaves-every-claimed-pci-function-armed.md +++ b/issues/a-reset-stops-xhci-and-leaves-every-claimed-pci-function-armed.md @@ -33,9 +33,9 @@ caches, waits for the log to be durable, seals `DONE` and calls `pcidev::release` (`kernel/src/pcidev/mod.rs:782`) calls `tear_down` (`:796`), which disables bus mastering (`:797`), masks the function's MSI-X entry (`:798`), empties its IOMMU domain (`:799`) and only then resets the function -(`:809`). The one caller of `pcidev::release` in the kernel -(`rg -n 'pcidev::release' kernel/src`, one hit) is `kernel/src/device.rs:75`, -inside `Claim`'s `Drop` — a dying process's handle table. +(`:809`). `release` is private to `pcidev` and its one caller is +`pcidev::Binding`'s `Drop`; a binding is held by a `device::Claim` and dropped +with it — a dying process's handle table. **So on every one of the five paths, a function whose holder is alive is left as its holder last wrote it when the register is written.** No process is diff --git a/issues/lan-talk-is-unread-on-the-t14-since-the-claims-binding-landed.md b/issues/lan-talk-is-unread-on-the-t14-since-the-claims-binding-landed.md new file mode 100644 index 00000000000..f87fcca74ed --- /dev/null +++ b/issues/lan-talk-is-unread-on-the-t14-since-the-claims-binding-landed.md @@ -0,0 +1,28 @@ +--- +status: open +kind: tooling +opened: 2026-10-08 +--- + +# `lan_talk` has not been read on the T14 since a device call began to borrow its claim's binding + +#763 makes every `pcidev` call act on a `&Binding` its claim lends, and a poll +register through the claim. The I219 under traffic is the one holder that +drives those calls on real hardware at rate: its interrupt records +(`take_record`, `has_irq`), its polls and its DMA grants. The `lan_talk` metal +row is what reads it, and it was staged from #763's head and not read: the +bench's wired path between the T14 and the development machine is down. + +The row cannot tell #763 from `main` while the path is down: staged from +`main` at `6f87cdb9c` and booted the same day, it is red the same way, `the +stream never opened` (`src/metaltalk.rs`), which is the host failing to reach +the machine and not the machine's answer. + +What stands in for it meanwhile: under QEMU, `iommu_virtio_platform` and the +suite's network tests drive the same `pcidev` functions through virtio-net's +claim; on the T14, `claim_reuses_its_remapping_entry`, `isa_` and `acpi_`. None +is the I219. + +Owner: the orchestrator, which holds the T14 and the bench. + +**Exit**: `lan_talk` green from the T14 on a head that contains #763. diff --git a/issues/the-power-offs-global-lock-line-is-logged-after-the-last-console-drain.md b/issues/the-power-offs-global-lock-line-is-logged-after-the-last-console-drain.md new file mode 100644 index 00000000000..e23d06a9a25 --- /dev/null +++ b/issues/the-power-offs-global-lock-line-is-logged-after-the-last-console-drain.md @@ -0,0 +1,57 @@ +--- +status: open +kind: defect +opened: 2026-10-08 +--- + +# The power-off logs the Global Lock's give-back after the console's last drain, and `acpi_mediated_access` waits for that line + +`acpi_mediated_access` (`tests/toyos.rs`) ends its wait on the kernel's line +`acpi: the Global Lock given back for a holder that left it taken (the machine +is stopping)`. Twice it never arrived, and the harness ended the guest after +`GUEST_QUIET` (15 s) of silence: + +``` +FAIL acpi_mediated_access: STALLED: waiting for the probe's power-off to give the lock back — it went quiet +``` + +| where | head | instrument | load | result | +|---|---|---|---|---| +| `main`'s nightly, run 37758546165, `tcg / suite` | `b6bcb9691` | TCG on a hosted 4-core x86-64 runner, 31 tests one wide | the suite alone | STALL after 18 s | +| the development machine, a whole-suite run of #763 | `252d19247` | TCG, x86-64 guest on an AArch64 host, 14 cores, 12 wide | load average 45 to 67: five agents compiling beside the suite | STALL after 35 s | +| `main`'s merge queue, six runs, #749 to #746 (37753990562, 37755528529, 37757410477, 37758981663, 37759626406, 37760845496), `guest / suite` | `6f87cdb9c` to `1084ddc9a` | KVM on hosted 4-core runners, 31 tests one wide | the suite alone | PASS, 3 s each | +| the development machine, `acpi_` by name | `252d19247` | TCG, 2 tests | run alone | PASS, 4 s | + +The nightly's head has #749, which added the test, and nothing of #763: the +red is `main`'s. KVM on a hosted runner is not TCG on a loaded host, so the +six green runs say only that the test passes where the guest is fast. + +## What was read + +`power::shutdown` (`kernel/src/power.rs`) calls `serial::flush_final()` under +the comment "Last chance: nothing drains the log ring after this point", and +then `arch::power::off`. `off` (`kernel/src/arch/x86_64/power.rs`) calls +`acpi_mode::settle`, whose `give_back` logs the line the test waits for, then +`acpi_mode::quiet`, then writes `SLP_TYP` and `SLP_EN`. So the line is +committed after the stop's last drain. Past boot the console's one writer is +`klogd` (`kernel/src/log/console.rs`, `Drain::Thread`), woken at the commit: the +line reaches the host only if `klogd`, on the other CPU, puts it on the wire +before this CPU has made one port read, `quiet`'s writes and the two writes +that end the machine. Nothing waits for it. + +That is a reading, and no run confirms it: neither red kept what the guest +said after boot. The test passes `await_guest`'s error up without its capture, +and the `uart-*.log` the nightly kept (artifact `serial-suite`, `uart-6.log`, +the `i8042-withheld` boot) ends where every boot's does, at the hand-over to +the virtio console. It fits both reds: the nightly's 18 s is the 3 s the test +takes under KVM and the 15 s of `GUEST_QUIET`. A probe whose arm failed would +have ended the wait by `===TEST_END`, and a kernel panic by its own line. + +The branch that saw it locally moves nothing here: `settle`, `give_back`, +`off`, `shutdown` and the console are `main`'s bytes in #763. + +Owner: the `acpi` claim's author (#749). + +**Exit**: `acpi_mediated_access` green in the nightly's `tcg / suite`, with the +give-back's line on the wire before `SLP_EN` by construction and not by +`klogd` winning; and a stall of this test carrying what the guest said. diff --git a/kernel/src/device.rs b/kernel/src/device.rs index 8e464e20618..c955c974510 100644 --- a/kernel/src/device.rs +++ b/kernel/src/device.rs @@ -98,15 +98,6 @@ impl Claim { } } - /// Answer every poll on the function or row this claim holds as gone. - pub(crate) fn cancel_polls(&self) { - match &self.what { - Claimed::PciFunction(binding) => crate::pcidev::cancel_polls(binding), - Claimed::Isa(row) | Claimed::Acpi(row) => crate::isa::cancel_polls(row), - Claimed::Class(_) | Claimed::Partition(_) => unreachable!("a poll cancelled through a claim on no function or row"), - } - } - /// The view a partition claim transfers through, or `None` for any other. pub(crate) fn partition(&self) -> Option<&crate::block::Partition> { match &self.what { diff --git a/kernel/src/isa.rs b/kernel/src/isa.rs index ea152103aad..146b37dbc37 100644 --- a/kernel/src/isa.rs +++ b/kernel/src/isa.rs @@ -270,11 +270,6 @@ pub(crate) fn add_poll(row: &Row, entry: PollEntry) { WATCHES[row.0].add_poll(entry); } -/// Answer every poll on the row a claim holds as gone. -pub fn cancel_polls(row: &Row) { - WATCHES[row.0].cancel_polls(); -} - /// The function a filled row raises `wire` with too, if one does: a second /// row on one line would take the other's interrupts. pub fn line_holder(wire: pio::Wire) -> Option<&'static str> { diff --git a/kernel/src/object/device.rs b/kernel/src/object/device.rs index 4080539d050..b2423518eaf 100644 --- a/kernel/src/object/device.rs +++ b/kernel/src/object/device.rs @@ -153,12 +153,6 @@ impl DeviceClaim { } } - /// Answer every poll on this claim's function or row as gone; its release - /// has, or will, where the last handle has let it go. - pub(crate) fn cancel_polls(&self) { - self.reference.with(Claim::cancel_polls); - } - /// The view a partition claim transfers through: `None` for a claim on /// anything else, and once the last handle has let the partition go. /// diff --git a/kernel/src/object/ops.rs b/kernel/src/object/ops.rs index 7c68e4d487d..5ebb983baf6 100644 --- a/kernel/src/object/ops.rs +++ b/kernel/src/object/ops.rs @@ -265,7 +265,8 @@ impl WatchRef { Self::Static(watch) => watch.cancel_polls(), Self::Shared(watch) => watch.cancel_polls(), Self::Irq(watch) => watch.cancel_polls(), - Self::Claim(claim) => claim.cancel_polls(), + // Its release answers them, and its close ends none (`close_ends_polls`). + Self::Claim(_) => unreachable!("a claim's polls are cancelled by its release alone"), } } } @@ -319,18 +320,20 @@ pub fn write_watch(object: &KObjectRef) -> Option { /// and the keyboard, which the machine ends on its own and which other handles /// share: a console closing is not every console's keyboard going away. `false` /// for a process, which only its own end ends: closing one handle to it ends no -/// other's watch. +/// other's watch. `false` for a claim on a function or a row: its one handle's +/// close is its release, which answers every poll on the watch before the slot +/// or the row can be held again (`pcidev`'s and `isa::Row`'s release). fn close_ends_polls(object: &KObjectRef) -> bool { match object { KObjectRef::SysCap(_) => false, KObjectRef::Console(_) => false, KObjectRef::Process(_) => false, KObjectRef::Device(d) => match d.class() { - device_registry::DeviceType::Keyboard => false, - device_registry::DeviceType::Mouse + device_registry::DeviceType::Keyboard | device_registry::DeviceType::PciFunction | device_registry::DeviceType::Isa - | device_registry::DeviceType::Acpi + | device_registry::DeviceType::Acpi => false, + device_registry::DeviceType::Mouse | device_registry::DeviceType::HdaAudio | device_registry::DeviceType::VirtioSound | device_registry::DeviceType::Framebuffer diff --git a/kernel/src/pcidev/mod.rs b/kernel/src/pcidev/mod.rs index 732b127cfe9..0b5487fa7e2 100644 --- a/kernel/src/pcidev/mod.rs +++ b/kernel/src/pcidev/mod.rs @@ -1771,8 +1771,3 @@ pub fn note_fault(slot: usize) { pub(crate) fn add_poll(binding: &Binding, entry: PollEntry) { WATCHES[binding.slot].add_poll(entry); } - -/// Answer every poll on the function a claim holds as gone. -pub fn cancel_polls(binding: &Binding) { - WATCHES[binding.slot].cancel_polls(); -} diff --git a/kernel/src/syscall/device.rs b/kernel/src/syscall/device.rs index 69b0b83bbb0..f6084b35e97 100644 --- a/kernel/src/syscall/device.rs +++ b/kernel/src/syscall/device.rs @@ -126,8 +126,10 @@ pub(super) fn sys_acpi(ctx: &SyscallContext, handle: RawHandle, op: u64, at: u64 let claim = match process::with_process_data(|data| data.handles.get::(handle, Rights::WRITE)) { Ok(claim) if claim.class() == device::DeviceType::Acpi => claim, Ok(claim) => { - return crate::object::HandleError::WrongType { held: claim.class().class_name(), wanted: "an acpi claim" } - .refuse() + let held = claim.class().class_name(); + // `refuse` does not return: the claim is let go first. + drop(claim); + return crate::object::HandleError::WrongType { held, wanted: "an acpi claim" }.refuse(); } Err(e) => return e.refuse(), }; @@ -326,7 +328,11 @@ pub(super) fn sys_device_dma_map( data.handles.get::(region, Rights::MAP) }) { Ok(memory) => memory, - Err(e) => return e.refuse(), + Err(e) => { + // `refuse` does not return: the claim is let go first. + drop(claim); + return e.refuse(); + } }; let len = core::mem::size_of::() as u64; let Some(mut window) = ctx.user_bytes_mut(out, len) else {