From 8dec3ec85d90fc792d991ead8240de49bb0c209b Mon Sep 17 00:00:00 2001 From: japabu Date: Tue, 8 Sep 2026 17:31:35 +0200 Subject: [PATCH 1/6] A claimed function is armed on MSI where it has no MSI-X MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `pcidev::bring_up` armed exactly one mechanism — `enable_msix(...).ok_or(Refusal::NoMsix)?` — so a function that publishes no MSI-X capability was refused by name and its holder never ran. Every function this project had handed to a process so far was a virtio one and every one of those has MSI-X, so the refusal had only ever been reached by `virtio_net_no_msix`'s deliberate `vectors=0`. The ThinkPad T14's onboard NIC is not one of those. Measured on the machine, not assumed: `/proc/interrupts` names its interrupt `IR-PCI-MSI-0000:00:1f.6 ... enp0s31f6` and `/sys/bus/pci/devices/0000:00:1f.6/msi_irqs/162` reads `mode=msi`, so Linux drives that function on MSI. Flashed and booted (run 28), this kernel wrote `pcidev: PCI 00:1f.6 NOT HANDED OVER — its MSI-X could not be armed`, netd found no endowment and exited, and the bench's one cable stayed dark. `bring_up` now arms MSI-X and falls back to MSI, and `Bound` holds whichever it got. **The driver above the boundary cannot tell which one it is and does not have to**: both deliver the same vector into the same `Interrupt`, and the claim answers the same handle either way. What differs is where the message lives, and therefore what a hand-over back has to write to silence it — `Armed::silence` masks an MSI-X table entry or clears MSI Enable, and `Armed::undo` puts the capability itself back off for a hand-over that armed a vector and was then refused. **MSI is not the weaker mechanism here, and the security argument is the same one.** MSI-X's table is kept out of what the holder maps because a holder that could rewrite it could point the device's message at any address the LAPIC decodes. An MSI function's message is in its own config space, which `pcidev` keeps: `config_read` is read-only and there is no writing counterpart. So MSI needs no BAR withheld — the same rule reaching a different register file. `Refusal::NoMsix` becomes `NoInterrupt` and says both mechanisms, because that is now what it means. `PciDevice::disable_msi` is new and is `disable_msix`'s counterpart: it sets the per-vector mask where the capability implements one and clears MSI Enable, which every function has. `toyos_pci::msi` grows `disabled` and the `MASKED`/`UNMASKED` mask values, host-tested — `disabled` deliberately does not restore the Multiple Message Enable field an arming zeroed, or a function would come back armed for as many vectors as it can raise. The hand-over record now names the mechanism (`vector 0x28 on MSI-X`): it is the first thing a machine that never heard from its device is asked, and it is not something the driver above the boundary can see. The two checks. - **Negative control.** Run 28 on the T14 is this change reverted whole, on the base the granted claim will be measured against: the same `tests/lancase` image on this machine's own I219 with `bring_up` arming MSI-X alone. It refused the claim by name at 1.349 s, netd exited `code=0` at 2.268 s, and nothing answered on the cable for the twenty seconds the boot stayed up. - **Independent oracles, two.** Linux's own reading of the same function, above — a driver nobody here wrote, saying that function is an MSI part. And the capability's register layout, which is the PCI spec's and which `toyos-pci/src/msi.rs`'s tests encode: the offsets of the address, data and mask registers all move with the 64-bit address bit, and writing a vector at the wrong one of them lands in whatever capability comes next in the list. Beside them, this kernel already produces the shape MSI must reproduce — `PCI 00:1f.3: msi address=0xfee00098 data=0x00000000` for the T14's HDA. Green: `cargo test -p toyos-pci` (41), `cargo test --lib` (296), `cargo run -- --clippy` (all five invocations), and the four guest arms that read this module — `virtio_net_no_msix`, `iommu_virtio_platform`, `pci_function_is_exclusive` and `userdev_dma_fault`. The MSI arm itself is exercised on the T14 alone: no device QEMU models that a process may claim publishes MSI without MSI-X, so there is no guest that can take that branch. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01GGQ2H2aCwd1jfiNmjsiUvz --- kernel/src/drivers/pci.rs | 18 ++++++- kernel/src/pcidev/mod.rs | 103 +++++++++++++++++++++++++++++++------- tests/common/faults.rs | 7 ++- tests/common/iommu.rs | 14 ++++-- toyos-pci/src/msi.rs | 43 ++++++++++++++++ 5 files changed, 161 insertions(+), 24 deletions(-) diff --git a/kernel/src/drivers/pci.rs b/kernel/src/drivers/pci.rs index 407c5ab1d8c..0030246e4c2 100644 --- a/kernel/src/drivers/pci.rs +++ b/kernel/src/drivers/pci.rs @@ -326,7 +326,7 @@ impl PciDevice { } cap.write_u16(msi.data(), data as u16); if let Some(mask) = msi.mask() { - cap.write_u32(mask, 0); + cap.write_u32(mask, msi::UNMASKED); } cap.write_u16(msi::MESSAGE_CONTROL, msi::Msi::enabled(control)); self.report_message( @@ -337,6 +337,22 @@ impl PciDevice { true } + /// Put MSI back off: the counterpart of [`Self::disable_msix`], and what a + /// claimed MSI function has in place of masking a table entry. + /// + /// The mask first where the function implements one, because that is the + /// per-vector lever and the one MSI-X's own hand-back uses; the enable bit + /// after, because every function has that one. A function still delivering + /// once its holder is gone writes its message into a slot with no reader. + pub fn disable_msi(&self) { + let Some(cap) = self.capabilities().find(|c| c.id() == msi::CAP_ID) else { return }; + let control = cap.read_u16(msi::MESSAGE_CONTROL); + if let Some(mask) = msi::Msi::decode(control).mask() { + cap.write_u32(mask, msi::MASKED); + } + cap.write_u16(msi::MESSAGE_CONTROL, msi::Msi::disabled(control)); + } + pub fn capabilities(&self) -> CapabilityIter<'_> { let first = self.mmio.read_u8(CAPABILITIES_PTR); CapabilityIter { device: self, walk: caps::CapWalk::new(), next: first } diff --git a/kernel/src/pcidev/mod.rs b/kernel/src/pcidev/mod.rs index ea8d9f58135..429126ae628 100644 --- a/kernel/src/pcidev/mod.rs +++ b/kernel/src/pcidev/mod.rs @@ -3,8 +3,9 @@ //! The line through the device is **who can name an address**. This module //! keeps config space — there is no write path to it from userland — puts the //! function in an address space of its own at the unit *before* it enables bus -//! mastering, programs the interrupt vector into its MSI-X table, and hands out -//! every device address a descriptor may carry. Nothing the holder writes into +//! mastering, programs the interrupt vector into whichever of the function's +//! two message mechanisms it has, and hands out every device address a +//! descriptor may carry. Nothing the holder writes into //! a descriptor can make the device touch memory the kernel did not grant it: //! the domain maps the grants and nothing else, and an address outside them is //! refused at the unit and recorded against that claim. @@ -16,7 +17,9 @@ //! //! **The BAR holding the MSI-X table or PBA is never mapped**: a holder that //! could rewrite the table could point the device's message at any address the -//! LAPIC decodes. +//! LAPIC decodes. An MSI function's message is in config space instead, which +//! is read-only from userland, so it needs no BAR withheld — the same rule +//! reaching a different register file, not a weaker one. //! //! **A function with no address space of its own is not handed over**, because //! every grant would answer with a physical address and a descriptor holding @@ -32,7 +35,7 @@ //! above is the mechanism and the reset is the belt. //! //! **What is read back, and what is not.** `Owned` is -//! `pci_function_is_exclusive`, `NoMsix` is `virtio_net_no_msix`, +//! `pci_function_is_exclusive`, `NoInterrupt` is `virtio_net_no_msix`, //! `Untranslated` is `iommu_virtio_platform`'s no-unit arm, the domain is //! `userdev_dma_fault`, and `SYS_DEVICE_REG_READ`'s bound is netd's own //! `config_space_is_bounded`. `Ambiguous`, `KernelDriven`, `Exhausted`, every @@ -121,12 +124,69 @@ struct Grant { bytes: u64, } +/// How a claimed function was made to speak, and what it takes to silence it. +/// +/// **The driver above the boundary cannot tell which one it got, and does not +/// care**: both deliver [`VECTORS`]`[slot]` into the same [`Interrupt`], and the +/// claim answers the same handle either way. What differs is where the message +/// lives — an MSI-X table entry in a mapping this kernel keeps for itself, or a +/// word of config space, which has no write path from userland at all — and so +/// what a hand-over back has to write to stop it. +enum Armed { + /// This function's one MSI-X table entry, mapped for the kernel alone. + Msix(Mmio), + /// MSI: the message is in this function's own config space and nothing was + /// mapped for it. + Msi, +} + +impl Armed { + fn name(&self) -> &'static str { + match self { + Self::Msix(_) => "MSI-X", + Self::Msi => "MSI", + } + } + + /// Stop this function delivering, for a holder that is gone. + fn silence(&self, pci: &PciDevice) { + match self { + Self::Msix(entry) => entry.write_u32(msix::ENTRY_VECTOR_CONTROL, msix::ENTRY_MASKED), + Self::Msi => pci.disable_msi(), + } + } + + /// Put the capability itself back off, for a hand-over that armed a vector + /// and was then refused: a function left enabled at a vector nobody holds + /// delivers into a slot with no reader. + fn undo(&self, pci: &PciDevice) { + match self { + Self::Msix(_) => pci.disable_msix(), + Self::Msi => pci.disable_msi(), + } + } +} + +/// MSI-X first, then MSI, and neither is somewhere a holder can write. +/// +/// **MSI-X first because it is the one this kernel can mask per vector**, and +/// because a function that publishes it is one whose table BAR is then kept out +/// of what the holder maps. MSI is not a lesser mechanism — the device performs +/// the same write to the same address — and the parts that have only it are not +/// rare: the ThinkPad's onboard I219 is one, which is where this arm came from. +fn arm(pci: &PciDevice, vector: u8) -> Option { + if let Some(entry) = pci.enable_msix(vector) { + return Some(Armed::Msix(entry)); + } + pci.enable_msi(vector).then_some(Armed::Msi) +} + /// What a live slot drives. The ISR never reads this. struct Bound { pci: PciDevice, space: DeviceSpace, - /// This function's one MSI-X table entry, mapped for the kernel alone. - entry: Mmio, + /// How this function was made to speak, and what silences it. + armed: Armed, id: PciId, /// Where each mappable BAR was put, and how much of it the function /// advertises; 0 bytes is a slot with no BAR this claim may map. @@ -329,7 +389,7 @@ fn window(assigned: u64, ceiling: u64) -> (u64, u64) { /// place. #[derive(Clone, Copy, PartialEq, Eq, Debug)] enum Refusal { - NoMsix, + NoInterrupt, Untranslated(IommuError), NoWindow, BarUnsizable(u8), @@ -341,10 +401,10 @@ enum Refusal { impl core::fmt::Display for Refusal { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { match self { - Self::NoMsix => write!( + Self::NoInterrupt => write!( f, - "its MSI-X could not be armed, and a claim with no interrupt is a driver \ - that would never be told anything" + "neither its MSI-X nor its MSI could be armed, and a claim with no interrupt \ + is a driver that would never be told anything" ), Self::Untranslated(why) => write!( f, @@ -422,12 +482,17 @@ pub fn claim(id: PciId) -> Result<(PciFunctionInfo, u8, Claim), ClaimError> { func: pci.func, _pad: [0; 5], }; + // Read off the hand-over rather than restated: which mechanism a + // function was armed on is the first thing a machine that never + // heard from its device is asked, and it is not a thing the driver + // above the boundary can see. + let armed = bound.armed.name(); *BOUND[slot].lock() = Some(bound); IRQ[slot].clear(); crate::iommu::note_user_owned(pci.bus, pci.dev, pci.func, Some(slot)); log!( "pcidev: PCI {:02x}:{:02x}.{} [{:04x}:{:04x}] handed over on slot {slot}, \ - vector {:#x}", + vector {:#x} on {armed}", pci.bus, pci.dev, pci.func, @@ -475,11 +540,11 @@ fn bring_up(pci: PciDevice, id: PciId, slot: usize) -> Result { // Decode must be on for a BAR to answer, and off across each move. pci.enable_memory_space(); - // Then the interrupt, still before a window is cut: a function whose MSI-X - // cannot be armed is one no holder could ever be told anything about, and - // `virtio_net_no_msix` reads that refusal off the console *and* the absence - // of any BAR line after it. - let entry = pci.enable_msix(VECTORS[slot]).ok_or(Refusal::NoMsix)?; + // Then the interrupt, still before a window is cut: a function neither + // mechanism can be armed on is one no holder could ever be told anything + // about, and `virtio_net_no_msix` reads that refusal off the console *and* + // the absence of any BAR line after it. + let armed = arm(&pci, VECTORS[slot]).ok_or(Refusal::NoInterrupt)?; // From here a refusal has to undo: a vector is armed, and the arms below // move the function's BARs. @@ -489,7 +554,7 @@ fn bring_up(pci: PciDevice, id: PciId, slot: usize) -> Result { Ok(Bound { pci, space, - entry, + armed, id, bar_at, bar_bytes, @@ -499,7 +564,7 @@ fn bring_up(pci: PciDevice, id: PciId, slot: usize) -> Result { }) } Err(why) => { - pci.disable_msix(); + armed.undo(&pci); Err(why) } } @@ -732,7 +797,7 @@ pub fn release(slot: usize) { fn tear_down(slot: usize, bound: Bound) { bound.pci.disable_bus_master(); - bound.entry.write_u32(msix::ENTRY_VECTOR_CONTROL, msix::ENTRY_MASKED); + bound.armed.silence(&bound.pci); crate::iommu::note_user_owned(bound.pci.bus, bound.pci.dev, bound.pci.func, None); for grant in bound.grants.iter() { if let Err(why) = bound.space.unmap(grant.at, grant.bytes) { diff --git a/tests/common/faults.rs b/tests/common/faults.rs index c1338bad029..1189e99aebb 100644 --- a/tests/common/faults.rs +++ b/tests/common/faults.rs @@ -285,7 +285,12 @@ pub fn virtio_net_no_msix() -> Result<(), String> { // whose holder would never be told anything, and handing it over anyway // would be handing out a device that looks alive and never speaks. log.must_say("pcidev: PCI 00:03.0 NOT HANDED OVER")?; - log.must_say("its MSI-X could not be armed")?; + // **Both mechanisms, named.** `pcidev` arms MSI-X and falls back to MSI, so + // this refusal is owed only by a function that has neither — and a virtio + // function stripped of its MSI-X table publishes no MSI capability to fall + // back to. A predicate naming one of them alone would be satisfied on a + // machine that armed the other and handed the function over. + log.must_say("neither its MSI-X nor its MSI could be armed")?; log.must_not_say("[1af4:1041] handed over")?; // And the refusal is the *whole* of it: no BAR moved for a function nobody // can be given one. diff --git a/tests/common/iommu.rs b/tests/common/iommu.rs index c6768648b90..66df45e8d8e 100644 --- a/tests/common/iommu.rs +++ b/tests/common/iommu.rs @@ -794,11 +794,12 @@ fn no_unit_is_no_claim(log: &Serial) -> Result<(), String> { log.must_not_say("handed over on slot")?; // **And the refusal spent nothing on the way out.** No BAR was moved, so // this function's BARs are still where firmware put them, and no vector was - // programmed into its MSI-X table — which is what says `bring_up` asks for - // the address space *before* it touches the function. `slot_space` put back - // below `place_bars` reds here. + // programmed into *either* of the two mechanisms `bring_up` may arm — which + // is what says it asks for the address space before it touches the function. + // `slot_space` put back below `place_bars` reds here. log.must_not_say(BAR_MOVED)?; log.must_not_say(MSIX_ARMED)?; + log.must_not_say(MSI_ARMED)?; log.must_say("init: netd: pci:1af4:1041 is on this machine and could not be handed over")?; // netd's own exit is not read here: it speaks after the ready marker this // capture ends at. It is the same endowment-is-empty path @@ -815,6 +816,13 @@ fn no_unit_is_no_claim(log: &Serial) -> Result<(), String> { const BAR_MOVED: &str = "pcidev: PCI 00:03.0 BAR"; const MSIX_ARMED: &str = "PCI 00:03.0: msix address="; +/// The other mechanism a hand-over may arm, whose absence the no-unit arm owes +/// as well. `bring_up` falls back to MSI, so "no MSI-X message was written" +/// stopped being the whole of "no vector was programmed" the moment it did — +/// and the two lines differ by one character, which is why each is spelled once +/// here rather than at its use. +const MSI_ARMED: &str = "PCI 00:03.0: msi address="; + /// The control that makes the two arms above mean something: a guest that /// declines the feature its host offered gets no device, not a bypassing one. /// `virtio_validate_features` returns `-EFAULT` and `virtio_set_status` returns diff --git a/toyos-pci/src/msi.rs b/toyos-pci/src/msi.rs index db05f0ea909..23a2c3dde80 100644 --- a/toyos-pci/src/msi.rs +++ b/toyos-pci/src/msi.rs @@ -79,8 +79,28 @@ impl Msi { pub fn enabled(message_control: u16) -> u16 { (message_control & !MULTI_MESSAGE_ENABLE) | ENABLE } + + /// Message Control with the function delivering nothing, and everything it + /// said about itself left alone. + /// + /// **The counterpart of a hand-over.** A function whose holder is gone has + /// to stop writing its message somewhere, and MSI-X's per-entry mask lives + /// in a table that does not exist here: this bit is what stands in for it, + /// because a capability's per-vector mask is optional and [`Msi::mask`] is + /// `None` on every function that does not implement one. + pub fn disabled(message_control: u16) -> u16 { + message_control & !ENABLE + } } +/// The Mask Bits value that masks the one vector [`Msi::enabled`] leaves a +/// function armed for, and the one that unmasks it. +/// +/// Bit *n* is vector *n* (PCIe §7.7.1.7), and Multiple Message Enable is zero +/// after `enabled`, so the armed vector is always index 0. +pub const MASKED: u32 = 1 << 0; +pub const UNMASKED: u32 = 0; + #[cfg(test)] mod tests { use super::*; @@ -135,4 +155,27 @@ mod tests { let ctrl = ADDRESS_64 | PER_VECTOR_MASK | (5 << 1); assert_eq!(Msi::enabled(ctrl), ctrl | ENABLE); } + + /// **Disabling is not the inverse of enabling, and must not be.** What a + /// hand-over back owes is that the function stops delivering; what it does + /// not owe is the Multiple Message Enable field an arming zeroed, and a + /// `disabled` that restored it would leave a function armed for as many + /// vectors as it can raise the moment anything set the enable bit again. + #[test] + fn disabling_clears_the_enable_bit_and_nothing_else() { + let ctrl = ADDRESS_64 | PER_VECTOR_MASK | (5 << 1); + assert_eq!(Msi::disabled(ctrl | ENABLE), ctrl); + assert_eq!(Msi::disabled(ctrl), ctrl); + // Round trip, on the field that moves: arming zeroes Multiple Message + // Enable and disarming leaves it zeroed. + assert_eq!(Msi::disabled(Msi::enabled(ctrl | MULTI_MESSAGE_ENABLE)), ctrl); + } + + /// The armed vector is index 0, so the mask that silences it is bit 0. + #[test] + fn the_mask_names_the_one_vector_that_was_armed() { + assert_eq!(MASKED, 1); + assert_eq!(UNMASKED, 0); + assert_eq!(Msi::enabled(MULTI_MESSAGE_ENABLE) & MULTI_MESSAGE_ENABLE, 0); + } } From 94341ce13f38b7b455937395f6bead4bb8b882db Mon Sep 17 00:00:00 2001 From: japabu Date: Tue, 8 Sep 2026 18:22:01 +0200 Subject: [PATCH 2/6] MSI is armed where a function publishes no MSI-X, and refused where it does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fallback this branch added chose by what an arming answered, so a function that publishes MSI-X and whose table this kernel could not decode fell through to MSI and was handed over with that table and its PBA inside a BAR `place_bars` did not withhold: `msix_bar` answers `None` on a decode failure, so nothing is kept back, while `enable_msix` answers `None` on the same input. On the base that function was refused and never reached a holder. The choice is now what the function's capability list publishes and never what an arming answered. `toyos_pci::mechanism` is that rule, pure and host-tested: a function publishing MSI-X is armed on MSI-X or refused `MsixUnusable`, MSI is armed only where there is no table in a BAR at all, and neither is `NoInterrupt`. `enable_msix` succeeding implies `Msix::decode` succeeded implies `msix_bar` named the BAR, so the table's BAR is withheld on every path that arms MSI-X. `disable_msi` is the enable bit alone. Its per-vector mask write had no specification behind its order and was the opposite of the one independent implementation this branch cites — Linux's `pci_msi_shutdown` clears MSI Enable and then *unmasks* — and leaving the Mask bit set owes a message on the set-to-clear transition a later arming makes of it with the Pending bit set (PCIe 7.7.1.7). What is left is the one decision `Msi::disabled` makes, which `disabling_clears_the_enable_bit_and_nothing_else` gates on the host; its fixture now carries Multiple Message Enable set, so the partial implementation that cleared that field too is red where it used to pass. `netd`'s `config_space_is_bounded` now attempts a configuration write and refuses a claim that answers one. That refusal — `RegTarget::PciConfig(_) => Err(NotSupported)` — is the whole of why an MSI function's message may stay in configuration space with no BAR withheld for it, and nothing asserted it. Deleted: `Armed`'s three one-caller methods and the `arm` free function, whose bodies are one `match` each at their sites; `msi::MASKED`/`UNMASKED` and the test that restated their declarations; `iommu.rs`'s `MSI_ARMED` `must_not_say`, which no implementation of that module could red — 00:03.0 publishes MSI-X, so that arm never reaches the MSI branch. `issues/kernel/a-claimed-function-must-have-msi-x-and-the-i219-may-not.md` is renamed and cut to the half that still stands: `toyos-i219` refuses a part outside MSI-X mode at `IVAR`, and the T14's `00:1f.6` is measured to be one. The kernel half the slug claimed is refuted by this branch. No citation to either the slug or the path exists anywhere else in the tree (`git grep`). Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01GGQ2H2aCwd1jfiNmjsiUvz --- ...fuses-a-part-outside-msi-x-mode-at-ivar.md | 29 +++++ ...on-must-have-msi-x-and-the-i219-may-not.md | 43 ------- kernel/src/drivers/pci.rs | 16 +-- kernel/src/pcidev/mod.rs | 113 ++++++++---------- tests/common/faults.rs | 8 +- tests/common/iommu.rs | 25 ++-- toyos-pci/src/lib.rs | 42 +++++++ toyos-pci/src/msi.rs | 36 +----- userland/netd/src/virtio_net.rs | 24 ++-- 9 files changed, 158 insertions(+), 178 deletions(-) create mode 100644 issues/hardware/toyos-i219-refuses-a-part-outside-msi-x-mode-at-ivar.md delete mode 100644 issues/kernel/a-claimed-function-must-have-msi-x-and-the-i219-may-not.md diff --git a/issues/hardware/toyos-i219-refuses-a-part-outside-msi-x-mode-at-ivar.md b/issues/hardware/toyos-i219-refuses-a-part-outside-msi-x-mode-at-ivar.md new file mode 100644 index 00000000000..073254a3c55 --- /dev/null +++ b/issues/hardware/toyos-i219-refuses-a-part-outside-msi-x-mode-at-ivar.md @@ -0,0 +1,29 @@ +--- +status: open +kind: defect +opened: 2026-09-08 +--- + +# `toyos-i219` refuses a part outside MSI-X mode at `IVAR`, and the T14's I219 is one + +`toyos-i219`'s `open` writes §10.2.4.9's `IVAR` and reads it back, refusing a +part that does not take the write: + +``` +nic.regs.write(regs::IVAR, ivar::ALL_ON_VECTOR_ZERO); +nic.accepted(regs::IVAR, ivar::ALL_ON_VECTOR_ZERO)?; +``` + +§10.2.4.9 defines that register only "in MSI-X mode" and says nothing about what +a part outside it answers, so the read-back is a guess refused rather than a +guess driven on. + +The T14's `00:1f.6` is outside that mode: Linux's own reading of the same +function is `IR-PCI-MSI-0000:00:1f.6 … enp0s31f6` in `/proc/interrupts` and +`mode=msi` at `/sys/bus/pci/devices/0000:00:1f.6/msi_irqs/162`. So the driver may +refuse the part the moment a claim on it is granted, and what `IVAR` answers on +an MSI part has never been read: on `24625c6b` the hand-over stopped at the BAR +window, before the driver ran. + +Owned by the stage-2 I219 worker: the first `nic.accepted(regs::IVAR, …)` on the +bench either passes or names the register that has to be driven differently. diff --git a/issues/kernel/a-claimed-function-must-have-msi-x-and-the-i219-may-not.md b/issues/kernel/a-claimed-function-must-have-msi-x-and-the-i219-may-not.md deleted file mode 100644 index d23afcf223d..00000000000 --- a/issues/kernel/a-claimed-function-must-have-msi-x-and-the-i219-may-not.md +++ /dev/null @@ -1,43 +0,0 @@ ---- -status: open -kind: finding -opened: 2026-09-07 ---- - -# A claimed function must publish MSI-X, and the I219 may not - -`kernel/src/pcidev/mod.rs`'s `bring_up` arms exactly one interrupt mechanism: - -``` -let entry = pci.enable_msix(VECTORS[slot]).ok_or(Refusal::NoMsix)?; -``` - -A function that publishes no MSI-X capability is refused by name, and `Bound` -holds an `Mmio` pointing at that function's one table entry so `tear_down` can -mask it. Every device this project has handed to a process so far is a virtio -function, and every one of those has MSI-X, so the refusal has never been -reached other than by `virtio_net_no_msix`'s deliberate `vectors=0`. - -**The kernel can already arm the other mechanism and nothing calls it.** -`PciDevice::enable_msi` exists in `kernel/src/drivers/pci.rs`, `toyos-pci`'s -`msi` module decodes the capability, and `iommu::remap_msi` is on that path -too. What is missing is `pcidev` choosing between them and a `Bound` that can -hold either — MSI has no per-entry table, so the masking `tear_down` does has -no counterpart and the capability's optional per-vector mask bit is what -stands in for it. - -Why it matters now: the ThinkPad T14's onboard NIC is an Intel I219 at -`00:1f.6`, and the e1000e family's PCH parts (I217/I218/I219) are documented as -MSI parts — Linux's `e1000e` sets `FLAG_HAS_MSIX` for the 82574 and 82583 and -for nothing else. **This has not been read off the laptop**, and it is the -first thing to check there: if `lspci -vv` on `00:1f.6` shows a `MSI-X` -capability the refusal never fires and nothing here is owed; if it shows only -`MSI`, netd's claim on `pci:8086:15fc` is refused `NoMsix`, netd exits, and -stage 2's metal half cannot run until this is built. - -`toyos-i219` writes §10.2.4.9's `IVAR` and reads it back, and refuses a part -that does not take the write by name — §10.2.4.9 defines the register only "in -MSI-X mode" and says nothing about what a part outside it answers. So an I219 -that is an MSI part is refused twice over: by `pcidev::bring_up` before the -driver runs, and by the driver if the claim is ever granted. Both refusals name -what to read off the laptop. diff --git a/kernel/src/drivers/pci.rs b/kernel/src/drivers/pci.rs index 0030246e4c2..dc9b31e1705 100644 --- a/kernel/src/drivers/pci.rs +++ b/kernel/src/drivers/pci.rs @@ -326,7 +326,7 @@ impl PciDevice { } cap.write_u16(msi.data(), data as u16); if let Some(mask) = msi.mask() { - cap.write_u32(mask, msi::UNMASKED); + cap.write_u32(mask, 0); } cap.write_u16(msi::MESSAGE_CONTROL, msi::Msi::enabled(control)); self.report_message( @@ -337,19 +337,15 @@ impl PciDevice { true } - /// Put MSI back off: the counterpart of [`Self::disable_msix`], and what a - /// claimed MSI function has in place of masking a table entry. + /// Put MSI back off: the counterpart of [`Self::disable_msix`]. /// - /// The mask first where the function implements one, because that is the - /// per-vector lever and the one MSI-X's own hand-back uses; the enable bit - /// after, because every function has that one. A function still delivering - /// once its holder is gone writes its message into a slot with no reader. + /// The enable bit alone, which every function has. The per-vector mask an + /// arming cleared stays clear: a function whose Mask bit this set would owe + /// a message on the set-to-clear transition a later arming makes of it, with + /// its Pending bit set (PCIe §7.7.1.7). pub fn disable_msi(&self) { let Some(cap) = self.capabilities().find(|c| c.id() == msi::CAP_ID) else { return }; let control = cap.read_u16(msi::MESSAGE_CONTROL); - if let Some(mask) = msi::Msi::decode(control).mask() { - cap.write_u32(mask, msi::MASKED); - } cap.write_u16(msi::MESSAGE_CONTROL, msi::Msi::disabled(control)); } diff --git a/kernel/src/pcidev/mod.rs b/kernel/src/pcidev/mod.rs index 429126ae628..d0607789acb 100644 --- a/kernel/src/pcidev/mod.rs +++ b/kernel/src/pcidev/mod.rs @@ -17,9 +17,10 @@ //! //! **The BAR holding the MSI-X table or PBA is never mapped**: a holder that //! could rewrite the table could point the device's message at any address the -//! LAPIC decodes. An MSI function's message is in config space instead, which -//! is read-only from userland, so it needs no BAR withheld — the same rule -//! reaching a different register file, not a weaker one. +//! LAPIC decodes. **So a function that publishes MSI-X is armed on MSI-X or +//! refused**, and MSI is armed only where there is no table in a BAR at all: +//! its message is a word of config space, which has no write path from +//! userland. //! //! **A function with no address space of its own is not handed over**, because //! every grant would answer with a physical address and a descriptor holding @@ -38,7 +39,8 @@ //! `pci_function_is_exclusive`, `NoInterrupt` is `virtio_net_no_msix`, //! `Untranslated` is `iommu_virtio_platform`'s no-unit arm, the domain is //! `userdev_dma_fault`, and `SYS_DEVICE_REG_READ`'s bound is netd's own -//! `config_space_is_bounded`. `Ambiguous`, `KernelDriven`, `Exhausted`, every +//! `config_space_is_bounded`, which reads the absence of a write path too. +//! `MsixUnusable`, `Ambiguous`, `KernelDriven`, `Exhausted`, every //! window refusal, and every bound `SYS_DEVICE_BAR_MAP` and //! `SYS_DEVICE_DMA_ALLOC` check are refused here and read by nothing: a //! registration of them waits on a boot config whose own test binary holds a @@ -58,7 +60,7 @@ use toyos_abi::boot::MemoryMapEntry; use toyos_abi::pci::{DeviceIrqRecord, PciFunctionInfo, BARS}; use toyos_abi::syscall::{PciId, RegWidth, SyscallError}; use toyos_dma::Register; -use toyos_pci::{bar, express, msix}; +use toyos_pci::{bar, express, mechanism, msi, msix, Mechanism}; use crate::device::{Claim, ClaimError}; use crate::drivers::pci::PciDevice; @@ -124,68 +126,20 @@ struct Grant { bytes: u64, } -/// How a claimed function was made to speak, and what it takes to silence it. -/// -/// **The driver above the boundary cannot tell which one it got, and does not -/// care**: both deliver [`VECTORS`]`[slot]` into the same [`Interrupt`], and the -/// claim answers the same handle either way. What differs is where the message -/// lives — an MSI-X table entry in a mapping this kernel keeps for itself, or a -/// word of config space, which has no write path from userland at all — and so -/// what a hand-over back has to write to stop it. +/// How a claimed function was made to speak. Both deliver [`VECTORS`]`[slot]` +/// into the same [`Interrupt`] and the claim answers the same handle either way. enum Armed { /// This function's one MSI-X table entry, mapped for the kernel alone. Msix(Mmio), - /// MSI: the message is in this function's own config space and nothing was - /// mapped for it. + /// The message is a word of this function's own config space, and nothing + /// was mapped for it. Msi, } -impl Armed { - fn name(&self) -> &'static str { - match self { - Self::Msix(_) => "MSI-X", - Self::Msi => "MSI", - } - } - - /// Stop this function delivering, for a holder that is gone. - fn silence(&self, pci: &PciDevice) { - match self { - Self::Msix(entry) => entry.write_u32(msix::ENTRY_VECTOR_CONTROL, msix::ENTRY_MASKED), - Self::Msi => pci.disable_msi(), - } - } - - /// Put the capability itself back off, for a hand-over that armed a vector - /// and was then refused: a function left enabled at a vector nobody holds - /// delivers into a slot with no reader. - fn undo(&self, pci: &PciDevice) { - match self { - Self::Msix(_) => pci.disable_msix(), - Self::Msi => pci.disable_msi(), - } - } -} - -/// MSI-X first, then MSI, and neither is somewhere a holder can write. -/// -/// **MSI-X first because it is the one this kernel can mask per vector**, and -/// because a function that publishes it is one whose table BAR is then kept out -/// of what the holder maps. MSI is not a lesser mechanism — the device performs -/// the same write to the same address — and the parts that have only it are not -/// rare: the ThinkPad's onboard I219 is one, which is where this arm came from. -fn arm(pci: &PciDevice, vector: u8) -> Option { - if let Some(entry) = pci.enable_msix(vector) { - return Some(Armed::Msix(entry)); - } - pci.enable_msi(vector).then_some(Armed::Msi) -} - /// What a live slot drives. The ISR never reads this. struct Bound { pci: PciDevice, space: DeviceSpace, - /// How this function was made to speak, and what silences it. armed: Armed, id: PciId, /// Where each mappable BAR was put, and how much of it the function @@ -390,6 +344,7 @@ fn window(assigned: u64, ceiling: u64) -> (u64, u64) { #[derive(Clone, Copy, PartialEq, Eq, Debug)] enum Refusal { NoInterrupt, + MsixUnusable, Untranslated(IommuError), NoWindow, BarUnsizable(u8), @@ -406,6 +361,11 @@ impl core::fmt::Display for Refusal { "neither its MSI-X nor its MSI could be armed, and a claim with no interrupt \ is a driver that would never be told anything" ), + Self::MsixUnusable => write!( + f, + "it publishes MSI-X and this kernel could not arm it, so its table is in a BAR \ + nothing here can name to withhold, and MSI is not a fallback from that" + ), Self::Untranslated(why) => write!( f, "it would have no address space of its own — {why} — and a process driving \ @@ -482,11 +442,10 @@ pub fn claim(id: PciId) -> Result<(PciFunctionInfo, u8, Claim), ClaimError> { func: pci.func, _pad: [0; 5], }; - // Read off the hand-over rather than restated: which mechanism a - // function was armed on is the first thing a machine that never - // heard from its device is asked, and it is not a thing the driver - // above the boundary can see. - let armed = bound.armed.name(); + let armed = match &bound.armed { + Armed::Msix(_) => "MSI-X", + Armed::Msi => "MSI", + }; *BOUND[slot].lock() = Some(bound); IRQ[slot].clear(); crate::iommu::note_user_owned(pci.bus, pci.dev, pci.func, Some(slot)); @@ -544,7 +503,21 @@ fn bring_up(pci: PciDevice, id: PciId, slot: usize) -> Result { // mechanism can be armed on is one no holder could ever be told anything // about, and `virtio_net_no_msix` reads that refusal off the console *and* // the absence of any BAR line after it. - let armed = arm(&pci, VECTORS[slot]).ok_or(Refusal::NoInterrupt)?; + // + // **The choice is what the function publishes, never what an arming + // answered**: a function whose MSI-X this kernel could not arm is refused + // rather than handed over on MSI, because `place_bars` withholds the table's + // BAR only for a function whose table [`msix_bar`] could name. + let publishes = |id| pci.capabilities().any(|cap| cap.id() == id); + let armed = match mechanism(publishes(msix::CAP_ID), publishes(msi::CAP_ID)) { + Some(Mechanism::Msix) => { + Armed::Msix(pci.enable_msix(VECTORS[slot]).ok_or(Refusal::MsixUnusable)?) + } + Some(Mechanism::Msi) => { + pci.enable_msi(VECTORS[slot]).then_some(Armed::Msi).ok_or(Refusal::NoInterrupt)? + } + None => return Err(Refusal::NoInterrupt), + }; // From here a refusal has to undo: a vector is armed, and the arms below // move the function's BARs. @@ -564,7 +537,12 @@ fn bring_up(pci: PciDevice, id: PciId, slot: usize) -> Result { }) } Err(why) => { - armed.undo(&pci); + // A function left enabled at a vector nobody holds delivers into a + // slot with no reader. + match armed { + Armed::Msix(_) => pci.disable_msix(), + Armed::Msi => pci.disable_msi(), + } Err(why) } } @@ -797,7 +775,12 @@ pub fn release(slot: usize) { fn tear_down(slot: usize, bound: Bound) { bound.pci.disable_bus_master(); - bound.armed.silence(&bound.pci); + // The entry masked where there is a table to mask, the capability off where + // there is not. + match &bound.armed { + Armed::Msix(entry) => entry.write_u32(msix::ENTRY_VECTOR_CONTROL, msix::ENTRY_MASKED), + Armed::Msi => bound.pci.disable_msi(), + } crate::iommu::note_user_owned(bound.pci.bus, bound.pci.dev, bound.pci.func, None); for grant in bound.grants.iter() { if let Err(why) = bound.space.unmap(grant.at, grant.bytes) { diff --git a/tests/common/faults.rs b/tests/common/faults.rs index 1189e99aebb..9e11a6df010 100644 --- a/tests/common/faults.rs +++ b/tests/common/faults.rs @@ -285,11 +285,9 @@ pub fn virtio_net_no_msix() -> Result<(), String> { // whose holder would never be told anything, and handing it over anyway // would be handing out a device that looks alive and never speaks. log.must_say("pcidev: PCI 00:03.0 NOT HANDED OVER")?; - // **Both mechanisms, named.** `pcidev` arms MSI-X and falls back to MSI, so - // this refusal is owed only by a function that has neither — and a virtio - // function stripped of its MSI-X table publishes no MSI capability to fall - // back to. A predicate naming one of them alone would be satisfied on a - // machine that armed the other and handed the function over. + // Both mechanisms named: this refusal is owed only by a function that + // publishes neither, and one naming MSI-X alone would be satisfied by a + // machine that armed MSI instead and handed the function over. log.must_say("neither its MSI-X nor its MSI could be armed")?; log.must_not_say("[1af4:1041] handed over")?; // And the refusal is the *whole* of it: no BAR moved for a function nobody diff --git a/tests/common/iommu.rs b/tests/common/iommu.rs index 66df45e8d8e..0e9d51977f6 100644 --- a/tests/common/iommu.rs +++ b/tests/common/iommu.rs @@ -707,12 +707,13 @@ pub fn iommu_virtio_platform( // And the claim netd was given is bounded to its own function's // configuration space, which is what makes its capability walk — // an index by numbers the *device* wrote — safe to run at all. - // netd asks the kernel for a read past the end, one straddling it - // and one misaligned, and refuses to drive a claim that answers any - // of them. + // netd asks the kernel for a read past the end, one misaligned, and + // a write inside the bound — the last being what the kernel's own + // reason for withholding no BAR for an MSI message rests on — and + // refuses to drive a claim that answers any of them. log.must_say( - "netd: this claim answers 4096 bytes of configuration space and refuses every \ - access outside them", + "netd: this claim answers 4096 bytes of configuration space, refuses every \ + access outside them and every write inside them", )?; // The two things a hand-over spends, on the same function and the // same machine the arm below requires to be unspent. Without this @@ -794,12 +795,11 @@ fn no_unit_is_no_claim(log: &Serial) -> Result<(), String> { log.must_not_say("handed over on slot")?; // **And the refusal spent nothing on the way out.** No BAR was moved, so // this function's BARs are still where firmware put them, and no vector was - // programmed into *either* of the two mechanisms `bring_up` may arm — which - // is what says it asks for the address space before it touches the function. - // `slot_space` put back below `place_bars` reds here. + // programmed into its MSI-X table — which is what says `bring_up` asks for + // the address space *before* it touches the function. `slot_space` put back + // below `place_bars` reds here. log.must_not_say(BAR_MOVED)?; log.must_not_say(MSIX_ARMED)?; - log.must_not_say(MSI_ARMED)?; log.must_say("init: netd: pci:1af4:1041 is on this machine and could not be handed over")?; // netd's own exit is not read here: it speaks after the ready marker this // capture ends at. It is the same endowment-is-empty path @@ -816,13 +816,6 @@ fn no_unit_is_no_claim(log: &Serial) -> Result<(), String> { const BAR_MOVED: &str = "pcidev: PCI 00:03.0 BAR"; const MSIX_ARMED: &str = "PCI 00:03.0: msix address="; -/// The other mechanism a hand-over may arm, whose absence the no-unit arm owes -/// as well. `bring_up` falls back to MSI, so "no MSI-X message was written" -/// stopped being the whole of "no vector was programmed" the moment it did — -/// and the two lines differ by one character, which is why each is spelled once -/// here rather than at its use. -const MSI_ARMED: &str = "PCI 00:03.0: msi address="; - /// The control that makes the two arms above mean something: a guest that /// declines the feature its host offered gets no device, not a bypassing one. /// `virtio_validate_features` returns `-EFAULT` and `virtio_set_status` returns diff --git a/toyos-pci/src/lib.rs b/toyos-pci/src/lib.rs index c8527e3b1e6..feddbda2bfc 100644 --- a/toyos-pci/src/lib.rs +++ b/toyos-pci/src/lib.rs @@ -27,3 +27,45 @@ pub mod caps; pub mod express; pub mod msi; pub mod msix; + +/// Which of a function's two message capabilities a claim may be armed on. +/// +/// **A function that publishes MSI-X is armed on MSI-X or on nothing.** Its +/// table and its PBA live in one of its BARs, and that BAR is withheld from the +/// holder only where this kernel armed the table itself — so a fall back to MSI +/// would hand the BAR over with the table still inside it, and a holder that can +/// write a table entry can point the device's write at any address the LAPIC +/// decodes. MSI is for a function that publishes no table at all. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Mechanism { + Msix, + Msi, +} + +/// `None` for a function that publishes neither capability: nothing to arm. +pub fn mechanism(publishes_msix: bool, publishes_msi: bool) -> Option { + match (publishes_msix, publishes_msi) { + (true, _) => Some(Mechanism::Msix), + (false, true) => Some(Mechanism::Msi), + (false, false) => None, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The whole of the rule: what a function publishes decides this, and + /// whether the arming then succeeded may not enter into it. + #[test] + fn a_function_that_publishes_msix_is_never_offered_msi() { + assert_eq!(mechanism(true, true), Some(Mechanism::Msix)); + assert_eq!(mechanism(true, false), Some(Mechanism::Msix)); + } + + #[test] + fn msi_is_for_a_function_with_no_table_in_a_bar() { + assert_eq!(mechanism(false, true), Some(Mechanism::Msi)); + assert_eq!(mechanism(false, false), None); + } +} diff --git a/toyos-pci/src/msi.rs b/toyos-pci/src/msi.rs index 23a2c3dde80..1a78f8903c5 100644 --- a/toyos-pci/src/msi.rs +++ b/toyos-pci/src/msi.rs @@ -82,25 +82,11 @@ impl Msi { /// Message Control with the function delivering nothing, and everything it /// said about itself left alone. - /// - /// **The counterpart of a hand-over.** A function whose holder is gone has - /// to stop writing its message somewhere, and MSI-X's per-entry mask lives - /// in a table that does not exist here: this bit is what stands in for it, - /// because a capability's per-vector mask is optional and [`Msi::mask`] is - /// `None` on every function that does not implement one. pub fn disabled(message_control: u16) -> u16 { message_control & !ENABLE } } -/// The Mask Bits value that masks the one vector [`Msi::enabled`] leaves a -/// function armed for, and the one that unmasks it. -/// -/// Bit *n* is vector *n* (PCIe §7.7.1.7), and Multiple Message Enable is zero -/// after `enabled`, so the armed vector is always index 0. -pub const MASKED: u32 = 1 << 0; -pub const UNMASKED: u32 = 0; - #[cfg(test)] mod tests { use super::*; @@ -156,26 +142,14 @@ mod tests { assert_eq!(Msi::enabled(ctrl), ctrl | ENABLE); } - /// **Disabling is not the inverse of enabling, and must not be.** What a - /// hand-over back owes is that the function stops delivering; what it does - /// not owe is the Multiple Message Enable field an arming zeroed, and a - /// `disabled` that restored it would leave a function armed for as many - /// vectors as it can raise the moment anything set the enable bit again. + /// Disabling is not the inverse of arming: the Multiple Message Enable an + /// arming zeroed stays zero, or the function comes back armed for every + /// vector it can raise the moment anything sets the enable bit. The fixture + /// carries that field set, so an implementation clearing it here is refused. #[test] fn disabling_clears_the_enable_bit_and_nothing_else() { - let ctrl = ADDRESS_64 | PER_VECTOR_MASK | (5 << 1); + let ctrl = ADDRESS_64 | PER_VECTOR_MASK | MULTI_MESSAGE_ENABLE | (5 << 1); assert_eq!(Msi::disabled(ctrl | ENABLE), ctrl); assert_eq!(Msi::disabled(ctrl), ctrl); - // Round trip, on the field that moves: arming zeroes Multiple Message - // Enable and disarming leaves it zeroed. - assert_eq!(Msi::disabled(Msi::enabled(ctrl | MULTI_MESSAGE_ENABLE)), ctrl); - } - - /// The armed vector is index 0, so the mask that silences it is bit 0. - #[test] - fn the_mask_names_the_one_vector_that_was_armed() { - assert_eq!(MASKED, 1); - assert_eq!(UNMASKED, 0); - assert_eq!(Msi::enabled(MULTI_MESSAGE_ENABLE) & MULTI_MESSAGE_ENABLE, 0); } } diff --git a/userland/netd/src/virtio_net.rs b/userland/netd/src/virtio_net.rs index e08c35def74..dce2c7e66e5 100644 --- a/userland/netd/src/virtio_net.rs +++ b/userland/netd/src/virtio_net.rs @@ -20,7 +20,7 @@ use std::cell::{Cell, RefCell}; use toyos::shm::SharedMemory; -use toyos::{DmaRegion, PciDev}; +use toyos::{AsHandle, DmaRegion, PciDev}; use toyos_abi::syscall::{RegWidth, SyscallError}; use crate::device::{KernelRefused, Latch, Window}; @@ -279,7 +279,7 @@ pub enum Refusal { FeaturesRefused { offered: u64, status: u32 }, NoVector(&'static str), Kernel(KernelRefused), - /// The claim answered a configuration read it had to refuse. + /// The claim answered a configuration access it had to refuse. Unbounded(&'static str, u32), } @@ -311,10 +311,11 @@ impl std::fmt::Display for Refusal { /// only because a claim answers its own function's 4 KiB and nothing else. That /// is the kernel's contract, so this is where the driver that depends on it /// checks it: a read past the end and one not aligned for its own width are -/// both refused, and the first byte is not. An aligned read that straddles the -/// end cannot be written — 4096 is a multiple of every width — and one whose -/// offset wraps cannot be expressed, `PciDev::config_read` taking a `u32`; both -/// are answered where the arithmetic lives, in `toyos-dma`'s host tests. +/// both refused, no write anywhere in the space is answered, and the first byte +/// still reads. An aligned read that straddles the end cannot be written — 4096 +/// is a multiple of every width — and one whose offset wraps cannot be +/// expressed, `PciDev::config_read` taking a `u32`; both are answered where the +/// arithmetic lives, in `toyos-dma`'s host tests. fn config_space_is_bounded(dev: &PciDev) -> Result<(), Refusal> { const CONFIG_BYTES: u32 = 4096; for (what, at, width) in [ @@ -325,11 +326,18 @@ fn config_space_is_bounded(dev: &PciDev) -> Result<(), Refusal> { return Err(Refusal::Unbounded(what, at)); } } + // **And there is no write path at all**, which is the whole of why the + // kernel may leave an MSI function's message address in configuration space + // and withhold no BAR for it. The vendor id, because a write the kernel let + // through would land on a register the device holds read-only anyway. + if toyos_abi::syscall::device_reg_write(dev.as_handle(), 0, RegWidth::U16, 0).is_ok() { + return Err(Refusal::Unbounded("write into its configuration space", 0)); + } // And the bound is a bound rather than a wall: the vendor id is still there. dev.config_read(0, RegWidth::U16).map_err(KernelRefused::on("its vendor id")).map_err(Refusal::Kernel)?; crate::say!( - "netd: this claim answers {CONFIG_BYTES} bytes of configuration space and refuses \ - every access outside them" + "netd: this claim answers {CONFIG_BYTES} bytes of configuration space, refuses \ + every access outside them and every write inside them" ); Ok(()) } From 76fb456d716113620a3437a04323fa4f09303d1d Mon Sep 17 00:00:00 2001 From: japabu Date: Tue, 8 Sep 2026 18:58:53 +0200 Subject: [PATCH 3/6] The choice is eight lines where it is made, and the holes it leaves are tracked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `toyos_pci::mechanism` was a new pure-crate rule with one caller, a four-row truth table its two tests transcribed row for row, and a doc that told the kernel's hand-over story inside a crate that knows nothing of hand-overs. It is deleted whole: `toyos-pci/src/lib.rs` is byte-identical to origin/main again, and `bring_up` asks the capability list directly. The rule that MSI is not a fall-back is a rule about hand-over and is stated once, in the module whose subject is a function driven by a process (`kernel/src/pcidev/mod.rs`). The kernel's own xHCI and HDA drivers arm MSI-X or fall back to MSI, and that is not the same choice: nothing is handed over there, so no BAR carrying an MSI-X table reaches a holder and there is no rule to contradict. Also deleted, because nothing consumed them: the `on {armed}` discriminant on the hand-over line, which restates what `report_message` printed off the device's own registers one line earlier; netd's configuration-space write probe, which ran on the one claim that is armed on MSI-X and never on the one armed on MSI, and which had to reach past `PciDev` — a typed handle with no write method at all — into `toyos_abi::syscall` to make the call; and the second assertion in `disabling_clears_the_enable_bit_and_nothing_else`, which the first already implies. Two weaknesses this branch leaves standing are now recorded rather than carried in a pull request body: issues/kernel/nothing-reaches-the-msi-arm-of-a-claimed-function.md issues/kernel/nothing-asserts-that-a-claim-answers-no-configuration-write.md `grep -rn 'disable_msi\b\|enable_msi\b' kernel/` gives five sites: hda.rs:666 and xhci/wait/boot.rs:104 arm and never disarm, and pcidev/mod.rs:495, :520 and :756 are this branch's, reached by no tier. Each file carries its owner and the exit condition that closes it. Prose deleted at the sites the review named, including two pieces of pre-existing prose in files this branch edits: `pcidev`'s "What is read back, and what is not" register of tests, which no gate held and which went stale every time a test moved, and five of the six lines on `BAR_MOVED`/`MSIX_ARMED`. TWO SENTENCES OF 8dec3ec8 ARE RETRACTED. History is not rewritten here, so they are withdrawn by name instead: "nothing answered on the cable for the twenty seconds the boot stayed up" is false. /Users/jan/.claude/jobs/2280e09e/tmp/t14-run28/lancase.log:12 reads "100.92.92.12 answered a ping 64 s into the window, after 5 s of silence — so something on this cable was up while Ubuntu was not". "Run 28 on the T14 is this change reverted whole, on the base the granted claim will be measured against" is false. Run 28 is tip=220305b4 (t14-run28/lancase.log:1) and run 29 is tip=24625c6b (t14-run29/lancase.log:1), whose parent 938957ae run 28 does not carry, so the pair reverts two changes and is not a negative control. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01GGQ2H2aCwd1jfiNmjsiUvz --- ...fuses-a-part-outside-msi-x-mode-at-ivar.md | 4 +- ...-a-claim-answers-no-configuration-write.md | 26 +++++++++ ...aches-the-msi-arm-of-a-claimed-function.md | 37 ++++++++++++ kernel/src/drivers/pci.rs | 7 +-- kernel/src/pcidev/mod.rs | 56 +++++-------------- tests/common/faults.rs | 3 - tests/common/iommu.rs | 18 ++---- toyos-pci/src/lib.rs | 42 -------------- toyos-pci/src/msi.rs | 5 -- userland/netd/src/virtio_net.rs | 24 +++----- 10 files changed, 97 insertions(+), 125 deletions(-) create mode 100644 issues/kernel/nothing-asserts-that-a-claim-answers-no-configuration-write.md create mode 100644 issues/kernel/nothing-reaches-the-msi-arm-of-a-claimed-function.md diff --git a/issues/hardware/toyos-i219-refuses-a-part-outside-msi-x-mode-at-ivar.md b/issues/hardware/toyos-i219-refuses-a-part-outside-msi-x-mode-at-ivar.md index 073254a3c55..6fa0988d5bd 100644 --- a/issues/hardware/toyos-i219-refuses-a-part-outside-msi-x-mode-at-ivar.md +++ b/issues/hardware/toyos-i219-refuses-a-part-outside-msi-x-mode-at-ivar.md @@ -22,8 +22,8 @@ The T14's `00:1f.6` is outside that mode: Linux's own reading of the same function is `IR-PCI-MSI-0000:00:1f.6 … enp0s31f6` in `/proc/interrupts` and `mode=msi` at `/sys/bus/pci/devices/0000:00:1f.6/msi_irqs/162`. So the driver may refuse the part the moment a claim on it is granted, and what `IVAR` answers on -an MSI part has never been read: on `24625c6b` the hand-over stopped at the BAR -window, before the driver ran. +an MSI part has never been read: no hand-over of `00:1f.6` has reached the +driver yet, so nothing has run the write. Owned by the stage-2 I219 worker: the first `nic.accepted(regs::IVAR, …)` on the bench either passes or names the register that has to be driven differently. diff --git a/issues/kernel/nothing-asserts-that-a-claim-answers-no-configuration-write.md b/issues/kernel/nothing-asserts-that-a-claim-answers-no-configuration-write.md new file mode 100644 index 00000000000..5cf81e2f7b5 --- /dev/null +++ b/issues/kernel/nothing-asserts-that-a-claim-answers-no-configuration-write.md @@ -0,0 +1,26 @@ +--- +status: open +kind: tooling +opened: 2026-09-08 +--- + +# Nothing asserts that a claim answers no configuration write + +`SYS_DEVICE_REG_WRITE` on a `RegTarget::PciConfig` target is refused +`NotSupported` in `kernel/src/arch/syscall/device.rs`, and no test in any tier +reads that refusal. It is what a handed-over MSI function's safety rests on: its +message address and data are words of configuration space rather than a table in +a BAR, so nothing is withheld from the holder and the whole of the boundary is +that the write path does not exist. A one-field mutation there — the arm +answering `Ok` — hands the holder the ability to aim the device's write at any +address the LAPIC decodes, and every arm in every tier stays green. + +The SDK's `PciDev` offers `config_read` and no write, so a driver cannot express +the call without reaching past it into `toyos_abi::syscall` — which is why this +is not answered by a probe in one driver's `open`: netd's virtio-net path is the +only one that would run it, and that function is armed on MSI-X, never on MSI. + +Owned by whoever next adds a boot config with a test binary holding a claimable +function. Exit condition: a guest arm in which the holder calls +`SYS_DEVICE_REG_WRITE` on its own claim and the kernel refuses it, red against a +kernel whose `PciConfig` write arm answers `Ok`. diff --git a/issues/kernel/nothing-reaches-the-msi-arm-of-a-claimed-function.md b/issues/kernel/nothing-reaches-the-msi-arm-of-a-claimed-function.md new file mode 100644 index 00000000000..e99aa38aa11 --- /dev/null +++ b/issues/kernel/nothing-reaches-the-msi-arm-of-a-claimed-function.md @@ -0,0 +1,37 @@ +--- +status: open +kind: tooling +opened: 2026-09-08 +--- + +# Nothing reaches the MSI arm of a claimed function + +`pcidev::bring_up` arms a claimed function on MSI where it publishes no MSI-X, +and every line of that arm is reached by no test in any tier: + +- `PciDevice::enable_msi` from `bring_up`, and `PciDevice::disable_msi` from both + hand-back sites (`bring_up`'s `place_bars` failure and `tear_down`); +- `Refusal::MsixUnusable`, which is owed only by a function that publishes MSI-X + this kernel cannot arm; +- `Armed::Msi`'s teardown, which turns the capability off where there is no + table entry to mask. + +No device QEMU models that a process may claim publishes MSI without MSI-X, and +none publishes an MSI-X capability that cannot be armed, so no guest arm can take +either branch. `virtio_net_no_msix` reaches the neither-mechanism refusal and +nothing beyond it. The two pre-existing MSI armings in this kernel — xHCI's and +HDA's `arm_interrupt` — never disarm, so MSI teardown is exercised nowhere in +the tree at all. + +On the T14, `00:1f.6` has been armed as far as the message +(`PCI 00:1f.6: msi address=0xfee000b8 data=0x00000000`, run 29) and no further: +the hand-over is refused at the BAR window, so no interrupt has ever been +delivered on MSI on any machine, and no hand-back has ever run. + +Owned by the network track's stage-2 I219 worker. Exit condition: the first +`userdev` interrupt counted against a claim on `00:1f.6` on the bench, which +needs the 32-bit BAR window before it, plus netd exiting from that claim, which +runs `tear_down`'s MSI arm. A guest exit is the alternative and costs more: an +actuator that hides a function's MSI-X capability from the claim path, a boot +config whose own test binary holds a claimable function, and the tier row and CI +price of the boot that carries them. diff --git a/kernel/src/drivers/pci.rs b/kernel/src/drivers/pci.rs index dc9b31e1705..4c3ef58ffe6 100644 --- a/kernel/src/drivers/pci.rs +++ b/kernel/src/drivers/pci.rs @@ -339,10 +339,9 @@ impl PciDevice { /// Put MSI back off: the counterpart of [`Self::disable_msix`]. /// - /// The enable bit alone, which every function has. The per-vector mask an - /// arming cleared stays clear: a function whose Mask bit this set would owe - /// a message on the set-to-clear transition a later arming makes of it, with - /// its Pending bit set (PCIe §7.7.1.7). + /// The per-vector mask an arming cleared stays clear: a function whose Mask + /// bit this set would owe a message on the set-to-clear transition a later + /// arming makes of it, with its Pending bit set (PCIe §7.7.1.7). pub fn disable_msi(&self) { let Some(cap) = self.capabilities().find(|c| c.id() == msi::CAP_ID) else { return }; let control = cap.read_u16(msi::MESSAGE_CONTROL); diff --git a/kernel/src/pcidev/mod.rs b/kernel/src/pcidev/mod.rs index d0607789acb..56d9eb2b8e9 100644 --- a/kernel/src/pcidev/mod.rs +++ b/kernel/src/pcidev/mod.rs @@ -3,12 +3,12 @@ //! The line through the device is **who can name an address**. This module //! keeps config space — there is no write path to it from userland — puts the //! function in an address space of its own at the unit *before* it enables bus -//! mastering, programs the interrupt vector into whichever of the function's -//! two message mechanisms it has, and hands out every device address a -//! descriptor may carry. Nothing the holder writes into -//! a descriptor can make the device touch memory the kernel did not grant it: -//! the domain maps the grants and nothing else, and an address outside them is -//! refused at the unit and recorded against that claim. +//! mastering, programs the interrupt vector into whichever of the function's two +//! message mechanisms it has, and hands out every device address a descriptor +//! may carry. Nothing the holder writes into a descriptor can make the device +//! touch memory the kernel did not grant it: the domain maps the grants and +//! nothing else, and an address outside them is refused at the unit and +//! recorded against that claim. //! //! **A window is 2 MiB because that is the only page this kernel maps.** A BAR //! a process may see is re-assigned onto a 2 MiB boundary above everything @@ -35,17 +35,6 @@ //! advertises one (PCIe §6.6.2), which no device in reach does — so the order //! above is the mechanism and the reset is the belt. //! -//! **What is read back, and what is not.** `Owned` is -//! `pci_function_is_exclusive`, `NoInterrupt` is `virtio_net_no_msix`, -//! `Untranslated` is `iommu_virtio_platform`'s no-unit arm, the domain is -//! `userdev_dma_fault`, and `SYS_DEVICE_REG_READ`'s bound is netd's own -//! `config_space_is_bounded`, which reads the absence of a write path too. -//! `MsixUnusable`, `Ambiguous`, `KernelDriven`, `Exhausted`, every -//! window refusal, and every bound `SYS_DEVICE_BAR_MAP` and -//! `SYS_DEVICE_DMA_ALLOC` check are refused here and read by nothing: a -//! registration of them waits on a boot config whose own test binary holds a -//! claimable function, and netd holds this machine's only one. -//! //! Nothing here is specific to what a function *is*. /// No `crate::` reference, so `kernel-loom` compiles it and models the @@ -60,7 +49,7 @@ use toyos_abi::boot::MemoryMapEntry; use toyos_abi::pci::{DeviceIrqRecord, PciFunctionInfo, BARS}; use toyos_abi::syscall::{PciId, RegWidth, SyscallError}; use toyos_dma::Register; -use toyos_pci::{bar, express, mechanism, msi, msix, Mechanism}; +use toyos_pci::{bar, express, msi, msix}; use crate::device::{Claim, ClaimError}; use crate::drivers::pci::PciDevice; @@ -442,16 +431,12 @@ pub fn claim(id: PciId) -> Result<(PciFunctionInfo, u8, Claim), ClaimError> { func: pci.func, _pad: [0; 5], }; - let armed = match &bound.armed { - Armed::Msix(_) => "MSI-X", - Armed::Msi => "MSI", - }; *BOUND[slot].lock() = Some(bound); IRQ[slot].clear(); crate::iommu::note_user_owned(pci.bus, pci.dev, pci.func, Some(slot)); log!( "pcidev: PCI {:02x}:{:02x}.{} [{:04x}:{:04x}] handed over on slot {slot}, \ - vector {:#x} on {armed}", + vector {:#x}", pci.bus, pci.dev, pci.func, @@ -503,20 +488,13 @@ fn bring_up(pci: PciDevice, id: PciId, slot: usize) -> Result { // mechanism can be armed on is one no holder could ever be told anything // about, and `virtio_net_no_msix` reads that refusal off the console *and* // the absence of any BAR line after it. - // - // **The choice is what the function publishes, never what an arming - // answered**: a function whose MSI-X this kernel could not arm is refused - // rather than handed over on MSI, because `place_bars` withholds the table's - // BAR only for a function whose table [`msix_bar`] could name. - let publishes = |id| pci.capabilities().any(|cap| cap.id() == id); - let armed = match mechanism(publishes(msix::CAP_ID), publishes(msi::CAP_ID)) { - Some(Mechanism::Msix) => { - Armed::Msix(pci.enable_msix(VECTORS[slot]).ok_or(Refusal::MsixUnusable)?) - } - Some(Mechanism::Msi) => { - pci.enable_msi(VECTORS[slot]).then_some(Armed::Msi).ok_or(Refusal::NoInterrupt)? - } - None => return Err(Refusal::NoInterrupt), + let publishes = |cap_id| pci.capabilities().any(|cap| cap.id() == cap_id); + let armed = if publishes(msix::CAP_ID) { + Armed::Msix(pci.enable_msix(VECTORS[slot]).ok_or(Refusal::MsixUnusable)?) + } else if publishes(msi::CAP_ID) { + pci.enable_msi(VECTORS[slot]).then_some(Armed::Msi).ok_or(Refusal::NoInterrupt)? + } else { + return Err(Refusal::NoInterrupt); }; // From here a refusal has to undo: a vector is armed, and the arms below @@ -537,8 +515,6 @@ fn bring_up(pci: PciDevice, id: PciId, slot: usize) -> Result { }) } Err(why) => { - // A function left enabled at a vector nobody holds delivers into a - // slot with no reader. match armed { Armed::Msix(_) => pci.disable_msix(), Armed::Msi => pci.disable_msi(), @@ -775,8 +751,6 @@ pub fn release(slot: usize) { fn tear_down(slot: usize, bound: Bound) { bound.pci.disable_bus_master(); - // The entry masked where there is a table to mask, the capability off where - // there is not. match &bound.armed { Armed::Msix(entry) => entry.write_u32(msix::ENTRY_VECTOR_CONTROL, msix::ENTRY_MASKED), Armed::Msi => bound.pci.disable_msi(), diff --git a/tests/common/faults.rs b/tests/common/faults.rs index 9e11a6df010..515fc66bdcd 100644 --- a/tests/common/faults.rs +++ b/tests/common/faults.rs @@ -285,9 +285,6 @@ pub fn virtio_net_no_msix() -> Result<(), String> { // whose holder would never be told anything, and handing it over anyway // would be handing out a device that looks alive and never speaks. log.must_say("pcidev: PCI 00:03.0 NOT HANDED OVER")?; - // Both mechanisms named: this refusal is owed only by a function that - // publishes neither, and one naming MSI-X alone would be satisfied by a - // machine that armed MSI instead and handed the function over. log.must_say("neither its MSI-X nor its MSI could be armed")?; log.must_not_say("[1af4:1041] handed over")?; // And the refusal is the *whole* of it: no BAR moved for a function nobody diff --git a/tests/common/iommu.rs b/tests/common/iommu.rs index 0e9d51977f6..67970638b4a 100644 --- a/tests/common/iommu.rs +++ b/tests/common/iommu.rs @@ -707,13 +707,12 @@ pub fn iommu_virtio_platform( // And the claim netd was given is bounded to its own function's // configuration space, which is what makes its capability walk — // an index by numbers the *device* wrote — safe to run at all. - // netd asks the kernel for a read past the end, one misaligned, and - // a write inside the bound — the last being what the kernel's own - // reason for withholding no BAR for an MSI message rests on — and - // refuses to drive a claim that answers any of them. + // netd asks the kernel for a read past the end, one straddling it + // and one misaligned, and refuses to drive a claim that answers any + // of them. log.must_say( - "netd: this claim answers 4096 bytes of configuration space, refuses every \ - access outside them and every write inside them", + "netd: this claim answers 4096 bytes of configuration space and refuses every \ + access outside them", )?; // The two things a hand-over spends, on the same function and the // same machine the arm below requires to be unspent. Without this @@ -807,12 +806,7 @@ fn no_unit_is_no_claim(log: &Serial) -> Result<(), String> { Ok(()) } -/// The two lines a hand-over spends, on the function `netcase` claims. -/// -/// Named once because both arms of `iommu_virtio_platform` read them, in -/// opposite directions: the arm with a unit requires them and the arm without -/// one requires their absence. An absence nothing ever produces would pass -/// against a kernel that had stopped writing the line at all. +/// The two lines a hand-over spends: one arm requires them, the other their absence. const BAR_MOVED: &str = "pcidev: PCI 00:03.0 BAR"; const MSIX_ARMED: &str = "PCI 00:03.0: msix address="; diff --git a/toyos-pci/src/lib.rs b/toyos-pci/src/lib.rs index feddbda2bfc..c8527e3b1e6 100644 --- a/toyos-pci/src/lib.rs +++ b/toyos-pci/src/lib.rs @@ -27,45 +27,3 @@ pub mod caps; pub mod express; pub mod msi; pub mod msix; - -/// Which of a function's two message capabilities a claim may be armed on. -/// -/// **A function that publishes MSI-X is armed on MSI-X or on nothing.** Its -/// table and its PBA live in one of its BARs, and that BAR is withheld from the -/// holder only where this kernel armed the table itself — so a fall back to MSI -/// would hand the BAR over with the table still inside it, and a holder that can -/// write a table entry can point the device's write at any address the LAPIC -/// decodes. MSI is for a function that publishes no table at all. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum Mechanism { - Msix, - Msi, -} - -/// `None` for a function that publishes neither capability: nothing to arm. -pub fn mechanism(publishes_msix: bool, publishes_msi: bool) -> Option { - match (publishes_msix, publishes_msi) { - (true, _) => Some(Mechanism::Msix), - (false, true) => Some(Mechanism::Msi), - (false, false) => None, - } -} - -#[cfg(test)] -mod tests { - use super::*; - - /// The whole of the rule: what a function publishes decides this, and - /// whether the arming then succeeded may not enter into it. - #[test] - fn a_function_that_publishes_msix_is_never_offered_msi() { - assert_eq!(mechanism(true, true), Some(Mechanism::Msix)); - assert_eq!(mechanism(true, false), Some(Mechanism::Msix)); - } - - #[test] - fn msi_is_for_a_function_with_no_table_in_a_bar() { - assert_eq!(mechanism(false, true), Some(Mechanism::Msi)); - assert_eq!(mechanism(false, false), None); - } -} diff --git a/toyos-pci/src/msi.rs b/toyos-pci/src/msi.rs index 1a78f8903c5..0f9d3b3faaf 100644 --- a/toyos-pci/src/msi.rs +++ b/toyos-pci/src/msi.rs @@ -142,14 +142,9 @@ mod tests { assert_eq!(Msi::enabled(ctrl), ctrl | ENABLE); } - /// Disabling is not the inverse of arming: the Multiple Message Enable an - /// arming zeroed stays zero, or the function comes back armed for every - /// vector it can raise the moment anything sets the enable bit. The fixture - /// carries that field set, so an implementation clearing it here is refused. #[test] fn disabling_clears_the_enable_bit_and_nothing_else() { let ctrl = ADDRESS_64 | PER_VECTOR_MASK | MULTI_MESSAGE_ENABLE | (5 << 1); assert_eq!(Msi::disabled(ctrl | ENABLE), ctrl); - assert_eq!(Msi::disabled(ctrl), ctrl); } } diff --git a/userland/netd/src/virtio_net.rs b/userland/netd/src/virtio_net.rs index dce2c7e66e5..e08c35def74 100644 --- a/userland/netd/src/virtio_net.rs +++ b/userland/netd/src/virtio_net.rs @@ -20,7 +20,7 @@ use std::cell::{Cell, RefCell}; use toyos::shm::SharedMemory; -use toyos::{AsHandle, DmaRegion, PciDev}; +use toyos::{DmaRegion, PciDev}; use toyos_abi::syscall::{RegWidth, SyscallError}; use crate::device::{KernelRefused, Latch, Window}; @@ -279,7 +279,7 @@ pub enum Refusal { FeaturesRefused { offered: u64, status: u32 }, NoVector(&'static str), Kernel(KernelRefused), - /// The claim answered a configuration access it had to refuse. + /// The claim answered a configuration read it had to refuse. Unbounded(&'static str, u32), } @@ -311,11 +311,10 @@ impl std::fmt::Display for Refusal { /// only because a claim answers its own function's 4 KiB and nothing else. That /// is the kernel's contract, so this is where the driver that depends on it /// checks it: a read past the end and one not aligned for its own width are -/// both refused, no write anywhere in the space is answered, and the first byte -/// still reads. An aligned read that straddles the end cannot be written — 4096 -/// is a multiple of every width — and one whose offset wraps cannot be -/// expressed, `PciDev::config_read` taking a `u32`; both are answered where the -/// arithmetic lives, in `toyos-dma`'s host tests. +/// both refused, and the first byte is not. An aligned read that straddles the +/// end cannot be written — 4096 is a multiple of every width — and one whose +/// offset wraps cannot be expressed, `PciDev::config_read` taking a `u32`; both +/// are answered where the arithmetic lives, in `toyos-dma`'s host tests. fn config_space_is_bounded(dev: &PciDev) -> Result<(), Refusal> { const CONFIG_BYTES: u32 = 4096; for (what, at, width) in [ @@ -326,18 +325,11 @@ fn config_space_is_bounded(dev: &PciDev) -> Result<(), Refusal> { return Err(Refusal::Unbounded(what, at)); } } - // **And there is no write path at all**, which is the whole of why the - // kernel may leave an MSI function's message address in configuration space - // and withhold no BAR for it. The vendor id, because a write the kernel let - // through would land on a register the device holds read-only anyway. - if toyos_abi::syscall::device_reg_write(dev.as_handle(), 0, RegWidth::U16, 0).is_ok() { - return Err(Refusal::Unbounded("write into its configuration space", 0)); - } // And the bound is a bound rather than a wall: the vendor id is still there. dev.config_read(0, RegWidth::U16).map_err(KernelRefused::on("its vendor id")).map_err(Refusal::Kernel)?; crate::say!( - "netd: this claim answers {CONFIG_BYTES} bytes of configuration space, refuses \ - every access outside them and every write inside them" + "netd: this claim answers {CONFIG_BYTES} bytes of configuration space and refuses \ + every access outside them" ); Ok(()) } From 6d59a093236884893fef378d39c90d29db3c319a Mon Sep 17 00:00:00 2001 From: japabu Date: Tue, 8 Sep 2026 19:40:20 +0200 Subject: [PATCH 4/6] The arming answers which case it refused, and the caller stops re-deriving it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `enable_msix` collapsed four outcomes into one `None`, so `bring_up` had to ask the capability list again to tell "publishes no table" from "publishes one this kernel could not arm" — and the refusal it raised then asserted a reason that was false on three of the paths it fired on. `Refusal::MsixUnusable` said the table "is in a BAR nothing here can name to withhold", which holds only where `Msix::decode` failed: after a successful decode `msix_bar` answers `Some(bir)` and `place_bars` does withhold that BAR. On the `message()` path it was worse than false — `enable_msi` calls the same `message()` and fails identically, so the honest refusal there is that neither mechanism could be armed, and the console had already printed `not armed — {why}` one line above the contradiction. `enable_msix` now answers `Result` (`drivers/pci.rs:33-44`): `Absent` is a function publishing no MSI-X, `Unusable` a table this kernel could not reach, `Blocked` a message the unit refuses — which is the same message MSI would carry. `bring_up` is one match over it (`pcidev/mod.rs:489-496`) with no capability walk of its own: `Unusable` refuses `MsixUnusable`, `Blocked` refuses `NoInterrupt`, `Absent` tries MSI. The `else if publishes(msi::CAP_ID)` arm is gone with the closure: `enable_msi` already opens by looking the capability up and answering false without one, so the arm decided nothing and no log line on any machine moved with it. `virtio_net_no_msix` is the test that reaches the `Absent` arm and reads that false back. `MsixUnusable` no longer claims what withholding happened; it says the function publishes MSI-X, this kernel could not arm it, and MSI is not a fallback for a function that has a table. `PciDevice::capability(id)` (`drivers/pci.rs:363-366`) is the one spelling of "find this function's capability by id". It replaces seven copies of `capabilities().find(|c| c.id() == …)` — the four in `drivers/pci.rs`, `msix_bar` and `reset` in `pcidev/mod.rs`, and `power_up` in `drivers/hda.rs`. Three callers move from `Option` to `Result` and decide nothing new: `hda::arm_interrupt`, `virtio_sound::arm_interrupt` and `xhci::wait::boot:: arm_interrupt` read `.is_ok()`/`.is_err()` where they read `.is_some()`/ `.is_none()`. Two numbers in 76fb456d's message and body are corrected here, since history is not rewritten. Its message attributed "five sites" to `grep -rn 'disable_msi\b\|enable_msi\b' kernel/`; that command prints seven lines — the two definitions plus five call sites — and the enumeration that followed it was of the call sites only. Its body called `tests/common/iommu.rs` byte-identical to `origin/main`, which `git diff origin/main HEAD --shortstat -- tests/common/iommu.rs` refutes at `1 insertion(+), 6 deletions(-)`; `toyos-pci/src/lib.rs` and `userland/netd/src/virtio_net.rs` are byte-identical and `tests/common/iommu.rs` is not. The body also cited the choice at `mod.rs:487-496`, which was the comment above it and one line short of the block. Prose deleted rather than rewritten: `Armed::Msi`'s doc, which restated the module header's own MSI clause; the unbacked "no device QEMU models…" sentence and the session-local "run 29" locator in `issues/kernel/nothing-reaches-the-msi-arm-of-a-claimed-function.md`; the rebuttal of the dropped `open` probe in `issues/kernel/nothing-asserts-that-a-claim-answers-no-configuration-write.md`; and, pre-existing in files this branch edits, the "The refusal moved with the driver" chronology in `tests/common/faults.rs` and `enable_msix`'s two-meaning `None` clause. That tracker's own claim moved with the code: `enable_msi` from `bring_up` is now reached by `virtio_net_no_msix`, and what nothing reaches is a successful arming and everything past it. Green in this worktree: `cargo test -p toyos-pci` 40 passed exit 0; `cargo test --lib` 295 passed 1 ignored exit 0; `cargo test --workspace --exclude toyos-build` 138 suites 1347 passed exit 0; all five `src/clippy.rs` shapes exit 0, no warning. Every `cargo test --test toyos-build` is still refused at `src/toolchain.rs:1382` (exit 101) while `/Users/jan/Dev/jan/toyos-aperture` holds the sysroot; `--claim-sysroot` was not passed and `main` was not merged in. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_014iqcj4jDKpaiDX8B7CMvmK --- ...-a-claim-answers-no-configuration-write.md | 4 +- ...aches-the-msi-arm-of-a-claimed-function.md | 29 +++++-------- kernel/src/drivers/hda.rs | 4 +- kernel/src/drivers/pci.rs | 42 +++++++++++++------ kernel/src/drivers/virtio_sound.rs | 2 +- kernel/src/drivers/xhci/wait/boot.rs | 2 +- kernel/src/pcidev/mod.rs | 28 ++++++------- tests/common/faults.rs | 6 --- 8 files changed, 59 insertions(+), 58 deletions(-) diff --git a/issues/kernel/nothing-asserts-that-a-claim-answers-no-configuration-write.md b/issues/kernel/nothing-asserts-that-a-claim-answers-no-configuration-write.md index 5cf81e2f7b5..1bcb3c4c862 100644 --- a/issues/kernel/nothing-asserts-that-a-claim-answers-no-configuration-write.md +++ b/issues/kernel/nothing-asserts-that-a-claim-answers-no-configuration-write.md @@ -16,9 +16,7 @@ answering `Ok` — hands the holder the ability to aim the device's write at any address the LAPIC decodes, and every arm in every tier stays green. The SDK's `PciDev` offers `config_read` and no write, so a driver cannot express -the call without reaching past it into `toyos_abi::syscall` — which is why this -is not answered by a probe in one driver's `open`: netd's virtio-net path is the -only one that would run it, and that function is armed on MSI-X, never on MSI. +the call without reaching past it into `toyos_abi::syscall`. Owned by whoever next adds a boot config with a test binary holding a claimable function. Exit condition: a guest arm in which the holder calls diff --git a/issues/kernel/nothing-reaches-the-msi-arm-of-a-claimed-function.md b/issues/kernel/nothing-reaches-the-msi-arm-of-a-claimed-function.md index e99aa38aa11..b04e99becd7 100644 --- a/issues/kernel/nothing-reaches-the-msi-arm-of-a-claimed-function.md +++ b/issues/kernel/nothing-reaches-the-msi-arm-of-a-claimed-function.md @@ -7,26 +7,19 @@ opened: 2026-09-08 # Nothing reaches the MSI arm of a claimed function `pcidev::bring_up` arms a claimed function on MSI where it publishes no MSI-X, -and every line of that arm is reached by no test in any tier: +and no test in any tier arms one. `virtio_net_no_msix` calls +`PciDevice::enable_msi` from `bring_up` and reads false back; nothing reaches a +true, and so nothing reaches: -- `PciDevice::enable_msi` from `bring_up`, and `PciDevice::disable_msi` from both - hand-back sites (`bring_up`'s `place_bars` failure and `tear_down`); -- `Refusal::MsixUnusable`, which is owed only by a function that publishes MSI-X - this kernel cannot arm; -- `Armed::Msi`'s teardown, which turns the capability off where there is no - table entry to mask. +- `PciDevice::disable_msi` from either hand-back site (`bring_up`'s `place_bars` + failure and `tear_down`), or `Armed::Msi`'s teardown, which turns the + capability off where there is no table entry to mask; +- `Refusal::MsixUnusable` and `Unarmed::Blocked`, owed only by a function that + publishes MSI-X this kernel cannot arm and by a unit that refuses the message. -No device QEMU models that a process may claim publishes MSI without MSI-X, and -none publishes an MSI-X capability that cannot be armed, so no guest arm can take -either branch. `virtio_net_no_msix` reaches the neither-mechanism refusal and -nothing beyond it. The two pre-existing MSI armings in this kernel — xHCI's and -HDA's `arm_interrupt` — never disarm, so MSI teardown is exercised nowhere in -the tree at all. - -On the T14, `00:1f.6` has been armed as far as the message -(`PCI 00:1f.6: msi address=0xfee000b8 data=0x00000000`, run 29) and no further: -the hand-over is refused at the BAR window, so no interrupt has ever been -delivered on MSI on any machine, and no hand-back has ever run. +The two pre-existing MSI armings in this kernel — xHCI's and HDA's +`arm_interrupt` — never disarm, so MSI teardown is exercised nowhere in the tree +at all. Owned by the network track's stage-2 I219 worker. Exit condition: the first `userdev` interrupt counted against a claim on `00:1f.6` on the bench, which diff --git a/kernel/src/drivers/hda.rs b/kernel/src/drivers/hda.rs index d56e635b23a..456be4be6f8 100644 --- a/kernel/src/drivers/hda.rs +++ b/kernel/src/drivers/hda.rs @@ -622,7 +622,7 @@ fn probe(pci: &PciDevice) -> Option<(Mmio, u16, u16)> { /// Put the function in D0 if firmware left it lower; D3hot reads all ones, indistinguishable from /// an absent controller. fn power_up(pci: &PciDevice) { - let Some(cap) = pci.capabilities().find(|c| c.id() == CAP_POWER_MANAGEMENT) else { + let Some(cap) = pci.capability(CAP_POWER_MANAGEMENT) else { return; }; let pmcsr = cap.read_u16(PM_CONTROL_STATUS); @@ -663,7 +663,7 @@ fn reset_stream(stream: Mmio) -> bool { /// panic, over a peripheral. fn arm_interrupt(pci: &PciDevice) -> bool { let vector = crate::arch::idt::HDA_VECTOR; - if pci.enable_msix(vector).is_some() || pci.enable_msi(vector) { + if pci.enable_msix(vector).is_ok() || pci.enable_msi(vector) { return true; } log!( diff --git a/kernel/src/drivers/pci.rs b/kernel/src/drivers/pci.rs index 4c3ef58ffe6..c8a14f39f41 100644 --- a/kernel/src/drivers/pci.rs +++ b/kernel/src/drivers/pci.rs @@ -30,6 +30,19 @@ const MSG_ADDR: u32 = 0xFEE0_0000; // The same CPU, named as a destination rather than encoded in an address, for the unit to put in an entry. const MSG_DEST: u32 = 0; +/// Why [`PciDevice::enable_msix`] armed nothing. Named rather than collapsed +/// into one `None` because a caller choosing a mechanism answers each +/// differently. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Unarmed { + /// This function publishes no MSI-X capability. + Absent, + /// It publishes one whose table this kernel could not reach. + Unusable, + /// The unit refuses this function's message, and MSI would carry the same one. + Blocked, +} + pub struct Capability<'a> { device: &'a PciDevice, offset: u64, @@ -224,17 +237,16 @@ impl PciDevice { /// /// Answers the entry's own window, which stays this kernel's: masking is a /// write to it, and a claimant that could reach it could aim the device's - /// message at any address the LAPIC decodes. `None` is MSI-X that could not - /// be armed. - pub fn enable_msix(&self, vector: u8) -> Option { - let cap = self.capabilities().find(|c| c.id() == msix::CAP_ID)?; + /// message at any address the LAPIC decodes. + pub fn enable_msix(&self, vector: u8) -> Result { + let cap = self.capability(msix::CAP_ID).ok_or(Unarmed::Absent)?; let control = cap.read_u16(msix::MESSAGE_CONTROL); let table = match msix::Msix::decode(control, cap.read_u32(msix::TABLE)) { Ok(table) => table, Err(why) => { log!("PCI {:02x}:{:02x}.{}: MSI-X not armed, {}", self.bus, self.dev, self.func, why); - return None; + return Err(Unarmed::Unusable); } }; // Decoded, not assumed memory: a device may name a BAR that is an I/O BAR. @@ -243,7 +255,7 @@ impl PciDevice { Err(why) => { log!("PCI {:02x}:{:02x}.{}: MSI-X not armed, its table names BAR {} and {}", self.bus, self.dev, self.func, table.bir(), why); - return None; + return Err(Unarmed::Unusable); } }; let address = match table.table_address(base) { @@ -251,11 +263,11 @@ impl PciDevice { Err(why) => { log!("PCI {:02x}:{:02x}.{}: MSI-X not armed, {}", self.bus, self.dev, self.func, why); - return None; + return Err(Unarmed::Unusable); } }; - let (message, data) = self.message(vector)?; + let (message, data) = self.message(vector).ok_or(Unarmed::Blocked)?; let entry = address + MSIX_ENTRY as u64 * msix::ENTRY_BYTES; let table = crate::mm::paging::map_mmio(entry, 0x1000, MmioPolicy::Uncacheable); @@ -271,14 +283,14 @@ impl PciDevice { table.read_u32(msix::ENTRY_ADDRESS_LO), table.read_u32(msix::ENTRY_DATA), ); - Some(table) + Ok(table) } /// Put MSI-X back off, for a hand-over that armed a vector and was then /// refused: a function left enabled at a vector nobody holds delivers into /// a slot with no reader. pub fn disable_msix(&self) { - let Some(cap) = self.capabilities().find(|c| c.id() == msix::CAP_ID) else { return }; + let Some(cap) = self.capability(msix::CAP_ID) else { return }; let control = cap.read_u16(msix::MESSAGE_CONTROL); cap.write_u16(msix::MESSAGE_CONTROL, msix::Msix::disabled(control)); } @@ -310,7 +322,7 @@ impl PciDevice { /// Point this function's single MSI message at `vector` and enable it. pub fn enable_msi(&self, vector: u8) -> bool { - let Some(cap) = self.capabilities().find(|c| c.id() == msi::CAP_ID) else { + let Some(cap) = self.capability(msi::CAP_ID) else { return false; }; @@ -343,11 +355,17 @@ impl PciDevice { /// bit this set would owe a message on the set-to-clear transition a later /// arming makes of it, with its Pending bit set (PCIe §7.7.1.7). pub fn disable_msi(&self) { - let Some(cap) = self.capabilities().find(|c| c.id() == msi::CAP_ID) else { return }; + let Some(cap) = self.capability(msi::CAP_ID) else { return }; let control = cap.read_u16(msi::MESSAGE_CONTROL); cap.write_u16(msi::MESSAGE_CONTROL, msi::Msi::disabled(control)); } + /// The capability this function publishes under `id`, or `None` where it + /// publishes none. Every reader that asks a function what it has asks here. + pub fn capability(&self, id: u8) -> Option> { + self.capabilities().find(|cap| cap.id() == id) + } + pub fn capabilities(&self) -> CapabilityIter<'_> { let first = self.mmio.read_u8(CAPABILITIES_PTR); CapabilityIter { device: self, walk: caps::CapWalk::new(), next: first } diff --git a/kernel/src/drivers/virtio_sound.rs b/kernel/src/drivers/virtio_sound.rs index 955ddd4b4a3..8f1cf4096a8 100644 --- a/kernel/src/drivers/virtio_sound.rs +++ b/kernel/src/drivers/virtio_sound.rs @@ -483,7 +483,7 @@ fn build_chains( /// every period in flight forever. fn arm_interrupt(pci: &PciDevice, device: &VirtioDevice) -> bool { let vector = crate::arch::idt::VIRTIO_SOUND_VECTOR; - if pci.enable_msix(vector).is_none() { + if pci.enable_msix(vector).is_err() { log!( "virtio-sound: NOT INITIALISED at PCI {:02x}:{:02x}.{} — its MSI-X could not be \ armed and this driver has no other way to be told a period completed", diff --git a/kernel/src/drivers/xhci/wait/boot.rs b/kernel/src/drivers/xhci/wait/boot.rs index 4bd865f9892..418a44eef44 100644 --- a/kernel/src/drivers/xhci/wait/boot.rs +++ b/kernel/src/drivers/xhci/wait/boot.rs @@ -98,7 +98,7 @@ fn await_connect_settle(controllers: &[XhciController]) { // `None` must stay a refusal, never a degradation: there is no polled mode, and // every event-ring read depends on `irq_ring`, which only the ISR sets. fn arm_interrupt(pci_dev: &PciDevice) -> Option<&'static str> { - if pci_dev.enable_msix(XHCI_VECTOR).is_some() { + if pci_dev.enable_msix(XHCI_VECTOR).is_ok() { return Some("MSI-X"); } pci_dev.enable_msi(XHCI_VECTOR).then_some("MSI") diff --git a/kernel/src/pcidev/mod.rs b/kernel/src/pcidev/mod.rs index 56d9eb2b8e9..3a70f3b228b 100644 --- a/kernel/src/pcidev/mod.rs +++ b/kernel/src/pcidev/mod.rs @@ -49,10 +49,10 @@ use toyos_abi::boot::MemoryMapEntry; use toyos_abi::pci::{DeviceIrqRecord, PciFunctionInfo, BARS}; use toyos_abi::syscall::{PciId, RegWidth, SyscallError}; use toyos_dma::Register; -use toyos_pci::{bar, express, msi, msix}; +use toyos_pci::{bar, express, msix}; use crate::device::{Claim, ClaimError}; -use crate::drivers::pci::PciDevice; +use crate::drivers::pci::{PciDevice, Unarmed}; use crate::inbox::InboxId; use crate::iommu::{DeviceSpace, IommuError}; use crate::mm::paging::{CachePolicy, MmioPolicy}; @@ -120,8 +120,6 @@ struct Grant { enum Armed { /// This function's one MSI-X table entry, mapped for the kernel alone. Msix(Mmio), - /// The message is a word of this function's own config space, and nothing - /// was mapped for it. Msi, } @@ -352,8 +350,8 @@ impl core::fmt::Display for Refusal { ), Self::MsixUnusable => write!( f, - "it publishes MSI-X and this kernel could not arm it, so its table is in a BAR \ - nothing here can name to withhold, and MSI is not a fallback from that" + "it publishes MSI-X and this kernel could not arm it, and MSI is not a fallback \ + for a function that has a table" ), Self::Untranslated(why) => write!( f, @@ -488,13 +486,13 @@ fn bring_up(pci: PciDevice, id: PciId, slot: usize) -> Result { // mechanism can be armed on is one no holder could ever be told anything // about, and `virtio_net_no_msix` reads that refusal off the console *and* // the absence of any BAR line after it. - let publishes = |cap_id| pci.capabilities().any(|cap| cap.id() == cap_id); - let armed = if publishes(msix::CAP_ID) { - Armed::Msix(pci.enable_msix(VECTORS[slot]).ok_or(Refusal::MsixUnusable)?) - } else if publishes(msi::CAP_ID) { - pci.enable_msi(VECTORS[slot]).then_some(Armed::Msi).ok_or(Refusal::NoInterrupt)? - } else { - return Err(Refusal::NoInterrupt); + let armed = match pci.enable_msix(VECTORS[slot]) { + Ok(entry) => Armed::Msix(entry), + Err(Unarmed::Unusable) => return Err(Refusal::MsixUnusable), + Err(Unarmed::Blocked) => return Err(Refusal::NoInterrupt), + Err(Unarmed::Absent) => { + pci.enable_msi(VECTORS[slot]).then_some(Armed::Msi).ok_or(Refusal::NoInterrupt)? + } }; // From here a refusal has to undo: a vector is armed, and the arms below @@ -574,7 +572,7 @@ fn slot_space(slot: usize) -> Result { /// has in reach does, so nothing rests on this: what makes a re-claim safe is /// that bus mastering starts on the first grant and not at hand-over. fn reset(pci: &PciDevice) -> Option { - let cap = pci.capabilities().find(|c| c.id() == express::CAP_ID)?; + let cap = pci.capability(express::CAP_ID)?; if !express::resets(cap.read_u32(express::DEVICE_CAPABILITIES)) { return None; } @@ -606,7 +604,7 @@ fn settle_after_reset(pci: &PciDevice) { /// the two live in one BAR on every device in reach, and a device that split /// them costs the second BAR too rather than publishing one of them. fn msix_bar(pci: &PciDevice) -> Option { - let cap = pci.capabilities().find(|c| c.id() == msix::CAP_ID)?; + let cap = pci.capability(msix::CAP_ID)?; let control = cap.read_u16(msix::MESSAGE_CONTROL); let table = msix::Msix::decode(control, cap.read_u32(msix::TABLE)).ok()?; Some(table.bir()) diff --git a/tests/common/faults.rs b/tests/common/faults.rs index 515fc66bdcd..d72ecf2a29b 100644 --- a/tests/common/faults.rs +++ b/tests/common/faults.rs @@ -278,12 +278,6 @@ pub fn virtio_net_no_msix() -> Result<(), String> { // Refused by name, at a named function, and not by claiming a mode it does // not have: the xHCI driver's `polled mode` line is the defect this whole // family exists to keep out of the tree. - // - // **The refusal moved with the driver.** It used to be the kernel's own - // virtio-net `init` giving up; it is now the *claim* being refused, before - // any driver exists — a function whose interrupt cannot be armed is one - // whose holder would never be told anything, and handing it over anyway - // would be handing out a device that looks alive and never speaks. log.must_say("pcidev: PCI 00:03.0 NOT HANDED OVER")?; log.must_say("neither its MSI-X nor its MSI could be armed")?; log.must_not_say("[1af4:1041] handed over")?; From a2be5f84d809dd9e55af17ba1b126818203c9910 Mon Sep 17 00:00:00 2001 From: japabu Date: Sun, 13 Sep 2026 14:11:37 +0200 Subject: [PATCH 5/6] A capability list that ends early is refused, never armed on what the walk reached MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `bring_up` armed MSI on `Unarmed::Absent`, and `Absent` meant only "the walk yielded no MSI-X capability". The walk is `caps::CapWalk`, which ends at a misaligned pointer, one below the standard header, or one already visited (`toyos-pci/src/caps.rs`). So a function whose MSI capability precedes such a link and whose MSI-X capability follows it read as `Absent`, was armed on MSI, and `msix_bar` — the same blind walk, one line above — named no BAR, so `place_bars` withheld none and the MSI-X table's BAR went to the holder with everything else. A holder that can write that table points the device's message at any address the LAPIC decodes. On origin/main that function was refused `NoMsix` and reached no holder, so the hole is this branch's. An early end is now a distinct answer, and the refusal is by name. - `CapWalk::truncated()` says whether the walk ended at the list's terminator or at a link the spec forbids. The decision stays in the pure crate; the kernel reads it. - `PciDevice::capability(id)` answers `Result`: `Absent` is a walk that reached the terminator and found nothing under the id, `Truncated` a walk that stopped early, so what lies past that link was never read. - `enable_msix` carries it out as `Unarmed::NoTable(NoCapability)`, and `bring_up` refuses `Refusal::CapsTruncated` — "its capability list ends at a link the PCI spec forbids, so whether it holds an MSI-X table in a BAR was never read, and MSI is not armed on a guess". Only `NoTable(Absent)` reaches the MSI arm. The five remaining readers of `capability` — `disable_msix`, `enable_msi`, `disable_msi`, `hda::power_up`, `pcidev::reset` and `msix_bar` — take the same answer and decide nothing new; a truncated walk leaves each of them where a missing capability did. `msix_bar` is the one that matters, and what makes it safe is that `bring_up` refuses the function before `place_bars` ever reads what it answered. The host tests that see a partial implementation are `toyos-pci`'s four walk tests, which now assert the flag as well as the step: an implementation that treats a misaligned pointer, a below-header pointer or a cycle as a clean end is red at `a_pointer_that_is_not_dword_aligned_is_refused`, `a_pointer_below_the_standard_header_is_refused` and `a_pointer_to_a_visited_link_ends_the_walk`, and one that reports the terminator as an early end is red at `the_terminator_ends_the_walk` and `a_forward_chain_is_followed_to_its_end`. The module header at `kernel/src/pcidev/mod.rs` stated the bound as an absolute — "a function that publishes MSI-X is armed on MSI-X or refused" — which held only over what the walk reached. It now states it over the walk. Deleted, because nothing reads them: `#[derive(Debug, Clone, Copy, PartialEq, Eq)]` on `Unarmed`, whose five traits nothing in the tree formats, clones, copies or compares. Prose deleted rather than rewritten, at the sites the review named: `Unarmed`'s "Named rather than collapsed into one `None`", which is the argument `Refusal`'s own doc already makes, by reference to a `None` that no longer exists; `capability`'s first sentence, which restated its signature; `bring_up`'s clause naming which test reads which refusal, which the same commit deleted ten lines of elsewhere; and, pre-existing in files this branch edits, the three-implementations investigation story above `tests/common/faults.rs`'s `virtio_net_no_msix` and the reason under `userland/netd/src/i219.rs`'s BAR search, which this branch falsifies — the T14's `00:1f.6` publishes no MSI-X, so nothing is withheld from that claim. Green in this worktree: `cargo test -p toyos-pci` 40 passed exit 0; `cargo test --lib` 295 passed 1 ignored exit 0; `cargo test --workspace --exclude toyos-build` 138 suites 1347 passed 0 failed exit 0; all five `src/clippy.rs` shapes exit 0 with no warning. Every `cargo test --test toyos-build` is still refused at `src/toolchain.rs:1382` (exit 101) while `/Users/jan/Dev/jan/toyos-aperture` holds the sysroot; `--claim-sysroot` was not passed and `main` was not merged in. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014iqcj4jDKpaiDX8B7CMvmK --- kernel/src/drivers/hda.rs | 2 +- kernel/src/drivers/pci.rs | 40 +++++++++++++++++++++++++-------------- kernel/src/pcidev/mod.rs | 29 ++++++++++++++++++---------- tests/common/faults.rs | 6 ------ toyos-pci/src/caps.rs | 32 ++++++++++++++++++++++++++----- userland/netd/src/i219.rs | 3 --- 6 files changed, 73 insertions(+), 39 deletions(-) diff --git a/kernel/src/drivers/hda.rs b/kernel/src/drivers/hda.rs index 456be4be6f8..1b22527d0d0 100644 --- a/kernel/src/drivers/hda.rs +++ b/kernel/src/drivers/hda.rs @@ -622,7 +622,7 @@ fn probe(pci: &PciDevice) -> Option<(Mmio, u16, u16)> { /// Put the function in D0 if firmware left it lower; D3hot reads all ones, indistinguishable from /// an absent controller. fn power_up(pci: &PciDevice) { - let Some(cap) = pci.capability(CAP_POWER_MANAGEMENT) else { + let Ok(cap) = pci.capability(CAP_POWER_MANAGEMENT) else { return; }; let pmcsr = cap.read_u16(PM_CONTROL_STATUS); diff --git a/kernel/src/drivers/pci.rs b/kernel/src/drivers/pci.rs index c8a14f39f41..58fe18809de 100644 --- a/kernel/src/drivers/pci.rs +++ b/kernel/src/drivers/pci.rs @@ -30,13 +30,20 @@ const MSG_ADDR: u32 = 0xFEE0_0000; // The same CPU, named as a destination rather than encoded in an address, for the unit to put in an entry. const MSG_DEST: u32 = 0; -/// Why [`PciDevice::enable_msix`] armed nothing. Named rather than collapsed -/// into one `None` because a caller choosing a mechanism answers each -/// differently. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum Unarmed { - /// This function publishes no MSI-X capability. +/// Why a walk of a function's capability list answered no capability. +pub enum NoCapability { + /// The walk reached the list's terminator and nothing on it carried the id. Absent, + /// The walk ended at a link the spec forbids, so whether the function + /// publishes the capability past it was never read. + Truncated, +} + +/// Why [`PciDevice::enable_msix`] armed nothing. +pub enum Unarmed { + /// No MSI-X capability came off the walk, which is a table this function + /// does not have only where the walk reached the list's terminator. + NoTable(NoCapability), /// It publishes one whose table this kernel could not reach. Unusable, /// The unit refuses this function's message, and MSI would carry the same one. @@ -239,7 +246,7 @@ impl PciDevice { /// write to it, and a claimant that could reach it could aim the device's /// message at any address the LAPIC decodes. pub fn enable_msix(&self, vector: u8) -> Result { - let cap = self.capability(msix::CAP_ID).ok_or(Unarmed::Absent)?; + let cap = self.capability(msix::CAP_ID).map_err(Unarmed::NoTable)?; let control = cap.read_u16(msix::MESSAGE_CONTROL); let table = match msix::Msix::decode(control, cap.read_u32(msix::TABLE)) { Ok(table) => table, @@ -290,7 +297,7 @@ impl PciDevice { /// refused: a function left enabled at a vector nobody holds delivers into /// a slot with no reader. pub fn disable_msix(&self) { - let Some(cap) = self.capability(msix::CAP_ID) else { return }; + let Ok(cap) = self.capability(msix::CAP_ID) else { return }; let control = cap.read_u16(msix::MESSAGE_CONTROL); cap.write_u16(msix::MESSAGE_CONTROL, msix::Msix::disabled(control)); } @@ -322,7 +329,7 @@ impl PciDevice { /// Point this function's single MSI message at `vector` and enable it. pub fn enable_msi(&self, vector: u8) -> bool { - let Some(cap) = self.capability(msi::CAP_ID) else { + let Ok(cap) = self.capability(msi::CAP_ID) else { return false; }; @@ -355,15 +362,20 @@ impl PciDevice { /// bit this set would owe a message on the set-to-clear transition a later /// arming makes of it, with its Pending bit set (PCIe §7.7.1.7). pub fn disable_msi(&self) { - let Some(cap) = self.capability(msi::CAP_ID) else { return }; + let Ok(cap) = self.capability(msi::CAP_ID) else { return }; let control = cap.read_u16(msi::MESSAGE_CONTROL); cap.write_u16(msi::MESSAGE_CONTROL, msi::Msi::disabled(control)); } - /// The capability this function publishes under `id`, or `None` where it - /// publishes none. Every reader that asks a function what it has asks here. - pub fn capability(&self, id: u8) -> Option> { - self.capabilities().find(|cap| cap.id() == id) + /// Every reader that asks a function what it has asks here, so no reader + /// can mistake a list that ended early for one that named nothing. + pub fn capability(&self, id: u8) -> Result, NoCapability> { + let mut walk = self.capabilities(); + match walk.find(|cap| cap.id() == id) { + Some(cap) => Ok(cap), + None if walk.walk.truncated() => Err(NoCapability::Truncated), + None => Err(NoCapability::Absent), + } } pub fn capabilities(&self) -> CapabilityIter<'_> { diff --git a/kernel/src/pcidev/mod.rs b/kernel/src/pcidev/mod.rs index 3a70f3b228b..55f6ab93d44 100644 --- a/kernel/src/pcidev/mod.rs +++ b/kernel/src/pcidev/mod.rs @@ -17,10 +17,13 @@ //! //! **The BAR holding the MSI-X table or PBA is never mapped**: a holder that //! could rewrite the table could point the device's message at any address the -//! LAPIC decodes. **So a function that publishes MSI-X is armed on MSI-X or -//! refused**, and MSI is armed only where there is no table in a BAR at all: -//! its message is a word of config space, which has no write path from -//! userland. +//! LAPIC decodes. **So a function is armed on MSI only where a walk that +//! reached its capability list's terminator found no MSI-X**: its message is +//! then a word of config space, which has no write path from userland, and it +//! has no table in a BAR for [`msix_bar`] to keep back. A list that ends at a +//! link the spec forbids says nothing about what it publishes past that link, +//! so it is refused by name rather than armed on the mechanism the walk +//! happened to reach. //! //! **A function with no address space of its own is not handed over**, because //! every grant would answer with a physical address and a descriptor holding @@ -52,7 +55,7 @@ use toyos_dma::Register; use toyos_pci::{bar, express, msix}; use crate::device::{Claim, ClaimError}; -use crate::drivers::pci::{PciDevice, Unarmed}; +use crate::drivers::pci::{NoCapability, PciDevice, Unarmed}; use crate::inbox::InboxId; use crate::iommu::{DeviceSpace, IommuError}; use crate::mm::paging::{CachePolicy, MmioPolicy}; @@ -332,6 +335,7 @@ fn window(assigned: u64, ceiling: u64) -> (u64, u64) { enum Refusal { NoInterrupt, MsixUnusable, + CapsTruncated, Untranslated(IommuError), NoWindow, BarUnsizable(u8), @@ -353,6 +357,11 @@ impl core::fmt::Display for Refusal { "it publishes MSI-X and this kernel could not arm it, and MSI is not a fallback \ for a function that has a table" ), + Self::CapsTruncated => write!( + f, + "its capability list ends at a link the PCI spec forbids, so whether it holds \ + an MSI-X table in a BAR was never read, and MSI is not armed on a guess" + ), Self::Untranslated(why) => write!( f, "it would have no address space of its own — {why} — and a process driving \ @@ -484,13 +493,13 @@ fn bring_up(pci: PciDevice, id: PciId, slot: usize) -> Result { // Then the interrupt, still before a window is cut: a function neither // mechanism can be armed on is one no holder could ever be told anything - // about, and `virtio_net_no_msix` reads that refusal off the console *and* - // the absence of any BAR line after it. + // about. let armed = match pci.enable_msix(VECTORS[slot]) { Ok(entry) => Armed::Msix(entry), Err(Unarmed::Unusable) => return Err(Refusal::MsixUnusable), Err(Unarmed::Blocked) => return Err(Refusal::NoInterrupt), - Err(Unarmed::Absent) => { + Err(Unarmed::NoTable(NoCapability::Truncated)) => return Err(Refusal::CapsTruncated), + Err(Unarmed::NoTable(NoCapability::Absent)) => { pci.enable_msi(VECTORS[slot]).then_some(Armed::Msi).ok_or(Refusal::NoInterrupt)? } }; @@ -572,7 +581,7 @@ fn slot_space(slot: usize) -> Result { /// has in reach does, so nothing rests on this: what makes a re-claim safe is /// that bus mastering starts on the first grant and not at hand-over. fn reset(pci: &PciDevice) -> Option { - let cap = pci.capability(express::CAP_ID)?; + let cap = pci.capability(express::CAP_ID).ok()?; if !express::resets(cap.read_u32(express::DEVICE_CAPABILITIES)) { return None; } @@ -604,7 +613,7 @@ fn settle_after_reset(pci: &PciDevice) { /// the two live in one BAR on every device in reach, and a device that split /// them costs the second BAR too rather than publishing one of them. fn msix_bar(pci: &PciDevice) -> Option { - let cap = pci.capability(msix::CAP_ID)?; + let cap = pci.capability(msix::CAP_ID).ok()?; let control = cap.read_u16(msix::MESSAGE_CONTROL); let table = msix::Msix::decode(control, cap.read_u32(msix::TABLE)).ok()?; Some(table.bir()) diff --git a/tests/common/faults.rs b/tests/common/faults.rs index d72ecf2a29b..c4b1e5a3ec0 100644 --- a/tests/common/faults.rs +++ b/tests/common/faults.rs @@ -212,12 +212,6 @@ pub fn idle_stack_guard( /// A NIC that cannot raise an interrupt must cost the machine networking and /// nothing else. /// -/// The MSI-X setup was written out three times and the copies answered this -/// question three different ways: the xHCI driver fell back to MSI, and both -/// virtio drivers called `panic!`. So the one device on the bus with no way to -/// deliver a packet took down a kernel whose disk, console, audio and USB were -/// all working — class M1 again, on the mechanism M1's own fix went through. -/// /// The other two virtio functions keep their vectors, which is what makes the /// verdict mean anything: the console that carries the refusal and the audio /// device beside it are on the same bus, driven by the same code, and neither diff --git a/toyos-pci/src/caps.rs b/toyos-pci/src/caps.rs index 3ab605b5554..1d69d7be03c 100644 --- a/toyos-pci/src/caps.rs +++ b/toyos-pci/src/caps.rs @@ -11,26 +11,39 @@ pub const FIRST_CAP: u8 = 0x40; #[derive(Debug, Default)] pub struct CapWalk { seen: [u64; 4], + truncated: bool, } impl CapWalk { pub const fn new() -> Self { - Self { seen: [0; 4] } + Self { seen: [0; 4], truncated: false } } /// The next capability's offset, or `None` to end the walk: the terminator /// (0), a pointer the spec forbids, or one already visited (a cycle). pub fn step(&mut self, raw: u8) -> Option { - if raw == 0 || raw < FIRST_CAP || raw & 0x3 != 0 { + if raw == 0 { + return None; + } + if raw < FIRST_CAP || raw & 0x3 != 0 { + self.truncated = true; return None; } let (word, bit) = ((raw >> 6) as usize, 1u64 << (raw & 0x3F)); if self.seen[word] & bit != 0 { + self.truncated = true; return None; } self.seen[word] |= bit; Some(raw) } + + /// Whether the walk ended at a link the spec forbids rather than at the + /// terminator: nothing past that link was read, so what the function + /// publishes past it is unknown and never absent. + pub const fn truncated(&self) -> bool { + self.truncated + } } #[cfg(test)] @@ -39,14 +52,18 @@ mod tests { #[test] fn the_terminator_ends_the_walk() { - assert_eq!(CapWalk::new().step(0), None); + let mut w = CapWalk::new(); + assert_eq!(w.step(0), None); + assert!(!w.truncated()); } /// PCI spec §6.7: a capability pointer is dword-aligned. #[test] fn a_pointer_that_is_not_dword_aligned_is_refused() { for raw in [0x41u8, 0x42, 0x43, 0x4F, 0xFD, 0xFE, 0xFF] { - assert_eq!(CapWalk::new().step(raw), None, "{raw:#x}"); + let mut w = CapWalk::new(); + assert_eq!(w.step(raw), None, "{raw:#x}"); + assert!(w.truncated(), "{raw:#x}"); } } @@ -54,7 +71,9 @@ mod tests { #[test] fn a_pointer_below_the_standard_header_is_refused() { for raw in [0x04u8, 0x20, 0x3C] { - assert_eq!(CapWalk::new().step(raw), None, "{raw:#x}"); + let mut w = CapWalk::new(); + assert_eq!(w.step(raw), None, "{raw:#x}"); + assert!(w.truncated(), "{raw:#x}"); } assert_eq!(CapWalk::new().step(FIRST_CAP), Some(FIRST_CAP)); } @@ -66,6 +85,7 @@ mod tests { assert_eq!(w.step(0x50), Some(0x50)); assert_eq!(w.step(0xF8), Some(0xF8)); assert_eq!(w.step(0), None); + assert!(!w.truncated()); } /// A visited set, not an "increasing" test: a list may be laid out out of @@ -75,11 +95,13 @@ mod tests { let mut w = CapWalk::new(); assert_eq!(w.step(0x40), Some(0x40)); assert_eq!(w.step(0x40), None); + assert!(w.truncated()); let mut w = CapWalk::new(); assert_eq!(w.step(0x60), Some(0x60)); assert_eq!(w.step(0x50), Some(0x50)); assert_eq!(w.step(0x60), None); + assert!(w.truncated()); } #[test] diff --git a/userland/netd/src/i219.rs b/userland/netd/src/i219.rs index 5f960b98c4c..93cc901eb72 100644 --- a/userland/netd/src/i219.rs +++ b/userland/netd/src/i219.rs @@ -150,9 +150,6 @@ impl Nic { .describe() .map_err(KernelRefused::on("the claim's description")) .map_err(Opening::Kernel)?; - // The register file is in BAR 0 on every part of this family; the - // lowest BAR the claim will map is taken rather than assumed, because - // the kernel reports 0 bytes for one it keeps — the MSI-X table's. let (bar, bytes) = info .bar_bytes .iter() From cad60ec64e982300364c908ba962c86f200b5606 Mon Sep 17 00:00:00 2001 From: japabu Date: Sun, 13 Sep 2026 14:12:43 +0200 Subject: [PATCH 6/6] The tracker names the refusal this branch just added to what nothing reaches `Refusal::CapsTruncated` and `NoCapability::Truncated` are reached by no test in any tier, for the same reason the MSI arm is: the guest has no function that publishes one. The walk's half of the decision is host-tested in `toyos-pci/src/caps.rs` and a partial implementation of it is red there; the kernel's refusal arm is not, and the file now says so rather than describing a tree one commit older. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014iqcj4jDKpaiDX8B7CMvmK --- .../nothing-reaches-the-msi-arm-of-a-claimed-function.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/issues/kernel/nothing-reaches-the-msi-arm-of-a-claimed-function.md b/issues/kernel/nothing-reaches-the-msi-arm-of-a-claimed-function.md index b04e99becd7..c5a779bff00 100644 --- a/issues/kernel/nothing-reaches-the-msi-arm-of-a-claimed-function.md +++ b/issues/kernel/nothing-reaches-the-msi-arm-of-a-claimed-function.md @@ -15,7 +15,11 @@ true, and so nothing reaches: failure and `tear_down`), or `Armed::Msi`'s teardown, which turns the capability off where there is no table entry to mask; - `Refusal::MsixUnusable` and `Unarmed::Blocked`, owed only by a function that - publishes MSI-X this kernel cannot arm and by a unit that refuses the message. + publishes MSI-X this kernel cannot arm and by a unit that refuses the message; +- `Refusal::CapsTruncated` and `NoCapability::Truncated`, owed by a function + whose capability list ends at a link the spec forbids. The walk's half of that + decision is host-tested in `toyos-pci/src/caps.rs`; the kernel's refusal arm + is reached by nothing. The two pre-existing MSI armings in this kernel — xHCI's and HDA's `arm_interrupt` — never disarm, so MSI teardown is exercised nowhere in the tree