From ca79e3ab647f6373398a8e6172da52cd4e1811e7 Mon Sep 17 00:00:00 2001 From: japabu Date: Fri, 9 Oct 2026 23:04:42 +0200 Subject: [PATCH 1/6] virtio-sound moves from the kernel into soundserver, which claims the function as pci:1af4:1059 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The kernel's drivers/virtio_sound.rs, its IDT vector 0x23, the virtio-sound device class, its arms of SYS_DEVICE_REG_READ/WRITE and of the read and poll paths, and toyos-abi's virtio_sound layout are deleted, with the kernel transport's code only that driver used (the split used-ring consumer, write_chain, bind_msix). soundserver holds the function as a PCI claim and drives it itself through toyos-virtio: the capability walk, §3.1.1's bring-up, the three queues in one grant from SYS_DEVICE_DMA_ALLOC behind the unit, and the transmit queue's MSI-X as the claim's interrupt record. The sound device's messages and queues are a pure crate beside the mixer, toyos-virtio-sound, host-tested against a model device that answers on the doorbell with VirtIO 1.2 §5.14's tables as the oracle: every request byte for byte, the stream information at its offsets, and each malformed answer — a short response, an undefined status, a short or failed period status, a short event — refused by name. toyos-virtio gains the capability walk (PCI 3.0 §6.7: links masked, a link into the header and a looped list refused, a refused read refused by the claim's own word), NO_VECTOR and a 32-bit device-configuration read (§4.1.3.1), and refuses a device-specific structure off four bytes (§4.1.4.6.1). netstack walks through it; its own walk and the issue against it go. The HDA path is untouched. The completion record soundserver's DLL reads is now stamped when soundserver reads the used ring, not in an ISR: a claim's interrupt record carries a count and no time. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017cSFvbD35xJ2kGANVdm23C --- Cargo.lock | 11 + Cargo.toml | 1 + ...in-flight-acts-after-the-claims-release.md | 10 +- issues/every-driver-is-still-in-the-kernel.md | 24 +- ...s-a-refused-configuration-read-as-zeros.md | 35 - issues/toyos-runs-on-arm64.md | 5 +- kernel/src/arch/aarch64/irqchip.rs | 1 - kernel/src/arch/aarch64/trap.rs | 1 - kernel/src/arch/x86_64/idt/mod.rs | 5 - kernel/src/arch/x86_64/idt/virtio_sound.rs | 13 - kernel/src/device.rs | 9 +- kernel/src/drivers/mod.rs | 1 - kernel/src/drivers/virtio.rs | 150 +--- kernel/src/drivers/virtio_sound.rs | 454 ------------ kernel/src/irq_census.rs | 6 +- kernel/src/main.rs | 3 +- kernel/src/object/device.rs | 6 - kernel/src/object/ops.rs | 24 +- kernel/src/syscall/device.rs | 8 - kernel/src/syscall/io.rs | 19 +- src/sourcegate.rs | 1 - system.toml | 2 +- tests/common/iommu.rs | 43 +- tests/common/irqcensus.rs | 6 +- tests/logstallcase/system.toml | 2 +- tests/metalcase/system.toml | 2 +- tests/netcase/system.toml | 11 +- tests/testcases/system.toml | 2 +- .../src/bin/virtio_sound_counts.rs | 132 ++++ tests/toyos.rs | 80 ++ toyos-abi/src/audio.rs | 6 +- toyos-abi/src/hda.rs | 7 +- toyos-abi/src/inventory.rs | 2 +- toyos-abi/src/lib.rs | 1 - toyos-abi/src/syscall.rs | 5 - toyos-abi/src/virtio_sound.rs | 149 ---- toyos-inspect/src/dev.rs | 4 +- toyos-manifest/src/lib.rs | 2 +- toyos-virtio/src/lib.rs | 9 +- toyos-virtio/src/pci.rs | 109 ++- toyos-virtio/src/stub.rs | 10 +- toyos-virtio/src/tests.rs | 166 ++++- toyos/src/device.rs | 46 -- toyos/src/endow.rs | 5 +- toyos/src/lib.rs | 2 +- userland/netstack/src/device.rs | 24 +- userland/netstack/src/virtio_net.rs | 53 +- userland/soundserver/Cargo.toml | 3 + userland/soundserver/src/backend.rs | 4 - userland/soundserver/src/main.rs | 8 +- userland/soundserver/src/virtio.rs | 682 ++++++------------ userland/soundserver/virtio-sound/Cargo.toml | 24 + userland/soundserver/virtio-sound/src/lib.rs | 436 +++++++++++ .../soundserver/virtio-sound/src/tests.rs | 566 +++++++++++++++ userland/soundserver/virtio-sound/src/wire.rs | 187 +++++ userland/supervisor/src/main.rs | 4 +- 56 files changed, 2076 insertions(+), 1505 deletions(-) delete mode 100644 issues/the-virtio-capability-walk-reads-a-refused-configuration-read-as-zeros.md delete mode 100644 kernel/src/arch/x86_64/idt/virtio_sound.rs delete mode 100644 kernel/src/drivers/virtio_sound.rs create mode 100644 tests/toyos-rust-tests/src/bin/virtio_sound_counts.rs delete mode 100644 toyos-abi/src/virtio_sound.rs create mode 100644 userland/soundserver/virtio-sound/Cargo.toml create mode 100644 userland/soundserver/virtio-sound/src/lib.rs create mode 100644 userland/soundserver/virtio-sound/src/tests.rs create mode 100644 userland/soundserver/virtio-sound/src/wire.rs diff --git a/Cargo.lock b/Cargo.lock index 7910f62e324..806319d2daf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5272,9 +5272,12 @@ dependencies = [ "rubato", "toyos", "toyos-abi", + "toyos-device-memory", "toyos-hda", "toyos-inspect", "toyos-mixer", + "toyos-virtio", + "toyos-virtio-sound", ] [[package]] @@ -6093,6 +6096,14 @@ dependencies = [ "toyos-untrusted", ] +[[package]] +name = "toyos-virtio-sound" +version = "0.1.0" +dependencies = [ + "toyos-device-memory", + "toyos-virtio", +] + [[package]] name = "toyos-wallclock" version = "0.1.0" diff --git a/Cargo.toml b/Cargo.toml index 983504f7503..0ce8b1dbee6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -105,6 +105,7 @@ members = [ "userland/snake", "userland/soundserver", "userland/soundserver/mixer", + "userland/soundserver/virtio-sound", "userland/sprite", "userland/sshserver", "userland/supervisor", diff --git a/issues/a-class-claims-call-in-flight-acts-after-the-claims-release.md b/issues/a-class-claims-call-in-flight-acts-after-the-claims-release.md index 62367bf3cee..00e946a2c00 100644 --- a/issues/a-class-claims-call-in-flight-acts-after-the-claims-release.md +++ b/issues/a-class-claims-call-in-flight-acts-after-the-claims-release.md @@ -6,14 +6,14 @@ opened: 2026-10-08 # A class claim's call in flight acts after the claim's release -A claim on a class this kernel still drives — the framebuffer, HDA audio, -virtio-sound — is a per-class flag (`device::TAKEN`), and a call on one checks +A claim on a class this kernel still drives — the framebuffer, HDA audio — is +a per-class flag (`device::TAKEN`), and a call on one checks the handle and then acts with nothing held across the two: - `SYS_DEVICE_REG_READ` and `SYS_DEVICE_REG_WRITE` resolve the claim to - `RegTarget::Hda` or `RegTarget::VirtioSound` under the process-data lock - (`sys_device_reg`, `kernel/src/syscall/device.rs`) and reach - `drivers::hda::reg_write` or `drivers::virtio_sound::reg_write` after it; + `RegTarget::Hda` under the process-data lock (`sys_device_reg`, + `kernel/src/syscall/device.rs`) and reach `drivers::hda::reg_write` after + it; - `SYS_GPU_PRESENT`, `SYS_GPU_SET_CURSOR`, `SYS_GPU_MOVE_CURSOR` and `SYS_GPU_SET_RESOLUTION` ask `holds_claim` and then call `crate::gpu` (`kernel/src/syscall/dispatch.rs`). diff --git a/issues/every-driver-is-still-in-the-kernel.md b/issues/every-driver-is-still-in-the-kernel.md index dbcd2ca1226..31404554d85 100644 --- a/issues/every-driver-is-still-in-the-kernel.md +++ b/issues/every-driver-is-still-in-the-kernel.md @@ -29,20 +29,16 @@ is not built. What is left of the staged work: -1. **Audio and virtio-gpu, re-scoped.** `drivers/hda.rs` and - `drivers/virtio_sound.rs` bring their device up and gate soundd's register - access; `drivers/virtio_gpu.rs` is the only `Gpu` whose `SYS_GPU_*` calls do - anything, since GOP's are all no-ops. Each leaves when its userland holder - claims the function as `pci`, as netd does, retiring the `hda-audio` and - `virtio-sound` classes, their arms of `SYS_DEVICE_REG_READ`/`WRITE`, and - `SYS_GPU_*`, which is an ABI change. GOP stays: it is memory the loader - hands over, and the panic console paints it. - A virtio holder stands on `toyos-virtio`, whose first client is netstack's - NIC, and the second one owes that crate two things. The walk of the - capability list moves into it from `userland/netstack/src/virtio_net.rs`, - over a configuration read the caller passes in, which closes - `issues/the-virtio-capability-walk-reads-a-refused-configuration-read-as-zeros.md`. - And before a client ends its device on `UsedRefusal::Written` for a chain +1. **HDA and virtio-gpu, re-scoped.** `drivers/hda.rs` brings its device up + and gates soundserver's register access; `drivers/virtio_gpu.rs` is the only + `Gpu` whose `SYS_GPU_*` calls do anything, since GOP's are all no-ops. Each + leaves when its userland holder claims the function as `pci`, as netstack + and soundserver's virtio-sound driver do, retiring the `hda-audio` class, + its arms of `SYS_DEVICE_REG_READ`/`WRITE`, and `SYS_GPU_*`, which is an ABI + change. GOP stays: it is memory the loader hands over, and the panic console + paints it. + A virtio holder stands on `toyos-virtio`, which walks the capability list + too. Before a client ends its device on `UsedRefusal::Written` for a chain the device only reads, whose bound is 0, what QEMU's device reports as `len` on such a queue is measured: the NIC's transmit queue is the only one read so far. diff --git a/issues/the-virtio-capability-walk-reads-a-refused-configuration-read-as-zeros.md b/issues/the-virtio-capability-walk-reads-a-refused-configuration-read-as-zeros.md deleted file mode 100644 index a8cf43c6eaa..00000000000 --- a/issues/the-virtio-capability-walk-reads-a-refused-configuration-read-as-zeros.md +++ /dev/null @@ -1,35 +0,0 @@ ---- -status: open -kind: defect -opened: 2026-10-09 ---- - -# The virtio capability walk reads a refused configuration read as zeros - -`vendor_caps` (`userland/netstack/src/virtio_net.rs`) walks a function's -capability list through its claim, and two things in it believe more than they -were told: - -- A field of a vendor capability is read as - `dev.config_read(next + at, width).unwrap_or(0)`, so a read the kernel - refused becomes a `cfg_type`, `bar`, `offset` or `length` of 0. A capability - made of zeros is then refused or ignored by `toyos_virtio::pci::Layout::of` - under some other name (`MissingCap`, `TooShort`), and the kernel's word for - what happened is gone. -- The capabilities pointer and every `next` link are used as read. The *PCI - Local Bus Specification*, revision 3.0, section 6.7, reserves the bottom two - bits of each and has software mask them before using the value as an offset - (cited from memory: the document was not at hand when this was filed). - An unmasked pointer with either bit set is an offset no capability is at, - and the 32-bit reads at `next + 8` and on are then misaligned, which the - claim refuses and the first item turns into zeros. - -Neither reaches memory: the claim bounds every read to the function's own -configuration space (`config_space_is_bounded`). What they cost is a device -refused for the wrong reason. - -Owned by the second client of `toyos-virtio`, which moves the walk into the -crate (stage 1 of `issues/every-driver-is-still-in-the-kernel.md`). Exit: the -walk is the crate's, over a configuration read its caller passes in; a refused -read is a refusal by name and a link is masked before it is an offset; and a -host test is red against each of the two as they stand today. diff --git a/issues/toyos-runs-on-arm64.md b/issues/toyos-runs-on-arm64.md index 6f98b613235..fa9ea99d90a 100644 --- a/issues/toyos-runs-on-arm64.md +++ b/issues/toyos-runs-on-arm64.md @@ -328,8 +328,9 @@ Each stage names its exit; "measured" means a number from a run. stage's SMMUv3 first. Stubbed on AArch64, each owned by the small-kernel track, which moves the driver out of the kernel: - `arch::msi_message` refuses, so the kernel's xHCI (`virt`'s boot stick), - HDA, virtio-sound, virtio-console and virtio-gpu drivers each - refuse their function by name. + HDA, virtio-console and virtio-gpu drivers each refuse their function by + name, and a process's claim on one (netstack's, soundserver's + virtio-sound) is refused with them. - `drivers::gop` refuses a scanout that is not whole 2 MiB pages of its own, which a `ramfb` scanout carved out of RAM need not be. diff --git a/kernel/src/arch/aarch64/irqchip.rs b/kernel/src/arch/aarch64/irqchip.rs index ebef83ce249..6a9191e6a96 100644 --- a/kernel/src/arch/aarch64/irqchip.rs +++ b/kernel/src/arch/aarch64/irqchip.rs @@ -44,7 +44,6 @@ pub(super) enum Intid { /// What `irq-storm` floods this CPU with. Storm, Hda, - VirtioSound, } pub(super) const SGI_KICK: u32 = Intid::Kick as u32; diff --git a/kernel/src/arch/aarch64/trap.rs b/kernel/src/arch/aarch64/trap.rs index 2ac9947151c..6b9db871c47 100644 --- a/kernel/src/arch/aarch64/trap.rs +++ b/kernel/src/arch/aarch64/trap.rs @@ -490,7 +490,6 @@ pub fn install() { } pub const HDA_VECTOR: u8 = irqchip::Intid::Hda as u8; -pub const VIRTIO_SOUND_VECTOR: u8 = irqchip::Intid::VirtioSound as u8; /// The crash report for a panic, from the frame pointer the panic handler /// stood on: the backtrace, which CPU is on which stack, and what the diff --git a/kernel/src/arch/x86_64/idt/mod.rs b/kernel/src/arch/x86_64/idt/mod.rs index 4194826b6ab..cc9e2fe1b8d 100644 --- a/kernel/src/arch/x86_64/idt/mod.rs +++ b/kernel/src/arch/x86_64/idt/mod.rs @@ -11,7 +11,6 @@ mod timer; mod tlb; pub(crate) mod unclaimed; mod user_dev; -mod virtio_sound; mod xhci; use core::arch::naked_asm; @@ -46,9 +45,6 @@ pub const DMA_FAULT_VECTOR: u8 = Vector::DmaFault as u8; /// The vector the HDA controller's message-signalled interrupt carries. pub const HDA_VECTOR: u8 = Vector::Hda as u8; -/// The vector the virtio-sound device's MSI-X entry carries. -pub const VIRTIO_SOUND_VECTOR: u8 = Vector::VirtioSound as u8; - const PF_PRESENT: u64 = 1 << 0; const PF_WRITE: u64 = 1 << 1; const PF_INSTRUCTION_FETCH: u64 = 1 << 4; @@ -260,7 +256,6 @@ idt_vectors! { ring0 Nmi = 0x02, nmi::nmi_entry, ist 2; ring3 Timer = 0x20, timer::timer_entry; ring3 Xhci = 0x21, xhci::xhci_entry; - ring3 VirtioSound = 0x23, virtio_sound::virtio_sound_entry; ring3 I8042 = 0x24, i8042::i8042_entry; ring3 DmaFault = 0x25, dma_fault::dma_fault_entry; ring3 Hda = 0x26, hda::hda_entry; diff --git a/kernel/src/arch/x86_64/idt/virtio_sound.rs b/kernel/src/arch/x86_64/idt/virtio_sound.rs deleted file mode 100644 index 6e3744d624a..00000000000 --- a/kernel/src/arch/x86_64/idt/virtio_sound.rs +++ /dev/null @@ -1,13 +0,0 @@ -use super::device_irq::device_irq_entry; - -// Heap-free, and takes only interrupts-off locks: may interrupt a CPU holding the controller lock, which disables preemption but not interrupts. -extern "sysv64" fn virtio_sound_handler() { - crate::arch::percpu::irq_took!(Sound); - crate::drivers::virtio_sound::isr_complete(); - crate::arch::apic::eoi(); -} - -device_irq_entry! { - /// Virtio-sound MSI-X entry (see `device_irq_entry` for the asm contract). - pub(super) fn virtio_sound_entry => virtio_sound_handler -} diff --git a/kernel/src/device.rs b/kernel/src/device.rs index c955c974510..ae655b5c95d 100644 --- a/kernel/src/device.rs +++ b/kernel/src/device.rs @@ -113,9 +113,7 @@ impl Drop for Claim { &Claimed::Class(class) => { // Before the flag goes: a poll the next holder registers is not this claim's to answer. match class { - DeviceType::HdaAudio | DeviceType::VirtioSound => { - crate::drivers::AUDIO_WATCH.cancel_polls() - } + DeviceType::HdaAudio => crate::drivers::AUDIO_WATCH.cancel_polls(), DeviceType::Keyboard | DeviceType::Mouse | DeviceType::Framebuffer @@ -243,11 +241,6 @@ pub fn try_claim(class: DeviceType, selector: [u64; 2]) -> Result { - let (info, dma) = crate::drivers::virtio_sound::info().ok_or(ClaimError::Absent)?; - let claim = Claim::acquire(class)?; - Ok(DeviceClaim::new(class, DeviceInfo::VirtioSound(info, shm(dma)), claim)) - } } } diff --git a/kernel/src/drivers/mod.rs b/kernel/src/drivers/mod.rs index a4f8cf9f890..d089972afcf 100644 --- a/kernel/src/drivers/mod.rs +++ b/kernel/src/drivers/mod.rs @@ -13,7 +13,6 @@ pub mod usb_storage; pub mod virtio; pub mod virtio_console; pub mod virtio_gpu; -pub mod virtio_sound; pub mod gop; pub mod hda; pub mod panic_console; diff --git a/kernel/src/drivers/virtio.rs b/kernel/src/drivers/virtio.rs index 9f40bfb0055..131929f8c84 100644 --- a/kernel/src/drivers/virtio.rs +++ b/kernel/src/drivers/virtio.rs @@ -36,20 +36,15 @@ pub const COMMON_DEVICE_FEATURE_SELECT: u64 = 0x00; pub const COMMON_DEVICE_FEATURE: u64 = 0x04; pub const COMMON_DRIVER_FEATURE_SELECT: u64 = 0x08; pub const COMMON_DRIVER_FEATURE: u64 = 0x0C; -const COMMON_MSIX_CONFIG: u64 = 0x10; pub const COMMON_DEVICE_STATUS: u64 = 0x14; const COMMON_QUEUE_SELECT: u64 = 0x16; pub const COMMON_QUEUE_SIZE: u64 = 0x18; -const COMMON_QUEUE_MSIX: u64 = 0x1A; pub const COMMON_QUEUE_ENABLE: u64 = 0x1C; pub const COMMON_QUEUE_NOTIFY_OFF: u64 = 0x1E; pub const COMMON_QUEUE_DESC: u64 = 0x20; pub const COMMON_QUEUE_DRIVER: u64 = 0x28; pub const COMMON_QUEUE_DEVICE: u64 = 0x30; -/// Sentinel a virtio device reads back for a vector it could not allocate (virtio 1.2 §4.1.5.1.2). -const NO_VECTOR: u16 = 0xFFFF; - const VIRTQ_DESC_F_NEXT: u16 = 1; const VIRTQ_DESC_F_WRITE: u16 = 2; @@ -92,7 +87,6 @@ struct VirtioPciConfig { notify_off_multiplier: u32, #[allow(dead_code)] // parsed from spec, used for interrupt-based operation isr: Mmio, - device: Mmio, } /// Why a virtio PCI capability names no config window, or the window it names. @@ -262,13 +256,14 @@ impl VirtioPciConfig { } } - Ok(Self { + let config = Self { common: common.ok_or(MissingCap::Common)?, notify: notify.ok_or(MissingCap::Notify)?, notify_off_multiplier, isr: isr.ok_or(MissingCap::Isr)?, - device: device.ok_or(MissingCap::Device)?, - }) + }; + device.ok_or(MissingCap::Device)?; + Ok(config) } } @@ -295,20 +290,6 @@ impl<'pool> VirtqueueRegions<'pool> { used: buf.subview(used_off, used_size), } } - - /// Compute regions from three separate DMA pages. - pub fn from_separate( - desc: Dma<'pool>, - avail: Dma<'pool>, - used: Dma<'pool>, - queue_size: u16, - ) -> Self { - Self { - desc: desc.subview(0, queue_size as usize * DESC_BYTES), - avail: avail.subview(0, avail_bytes(queue_size)), - used: used.subview(0, used_bytes(queue_size)), - } - } } /// Proof a descriptor slot is available for submission; `id()` is always below the queue's size. @@ -320,47 +301,6 @@ impl DescSlot { } -/// Interrupt-context, lock-free consumer of a virtqueue's used ring; an ISR can drain while another CPU submits under a lock. -/// Lock-free because it reads only device-written memory and its own `last_used_idx`, never shared driver state. -pub struct UsedRingConsumer<'pool> { - used: Dma<'pool>, - size: u16, - last_used_idx: u16, - refused: u32, -} - -impl UsedRingConsumer<'_> { - /// Non-blocking poll: the head descriptor id of a completed chain, or `None` if nothing new. - /// Never logs: the only caller is an ISR and the log backend's lock is one it cannot wait on. - pub fn poll(&mut self) -> Option { - loop { - let used_idx: u16 = self.used.read(USED_IDX_OFF); - if used_idx == self.last_used_idx { - return None; - } - // The device writes the element before it bumps the idx (virtio 1.2 - // §2.7.8), so the element is read after the idx that counts it. - barrier::dma_rmb(); - let slot = self.last_used_idx % self.size; - let id: Untrusted = - Untrusted::new(self.used.read(USED_RING_OFF + slot as usize * USED_ELEM_SIZE)); - self.last_used_idx = self.last_used_idx.wrapping_add(1); - // A refused head is skipped, not returned: `None` here means the ring is empty. - let Ok(head) = id.index(self.size as usize) else { - self.refused = self.refused.saturating_add(1); - continue; - }; - // Exact: `index` proved `head < self.size`, a `u16`. - return Some(head as u16); - } - } - - /// How many used-ring elements this consumer has refused, for the life of the boot. - pub fn refused(&self) -> u32 { - self.refused - } -} - /// A VirtIO split virtqueue. pub struct Virtqueue<'pool> { desc: Dma<'pool>, @@ -369,7 +309,6 @@ pub struct Virtqueue<'pool> { size: u16, last_used_idx: u16, notify_offset: u16, - used_split: bool, /// Bytes each chain's descriptor was given; the one bound a device-reported `len` is compared against. 0 means no chain. chain_bytes: alloc::vec::Vec, } @@ -407,23 +346,10 @@ impl<'pool> Virtqueue<'pool> { size: queue_size, last_used_idx: 0, notify_offset: 0, - used_split: false, chain_bytes: alloc::vec![0u32; queue_size as usize], } } - /// Hand the used ring to a dedicated consumer; afterwards `poll_used`/`has_used` panic here. - pub fn split_used_consumer(&mut self) -> UsedRingConsumer<'pool> { - assert!(!self.used_split, "virtqueue: used ring already split"); - self.used_split = true; - UsedRingConsumer { - used: self.used, - size: self.size, - last_used_idx: self.last_used_idx, - refused: 0, - } - } - /// What the device is programmed with for each ring. pub fn descs_addr(&self) -> u64 { self.desc.device_addr() } pub fn avail_addr(&self) -> u64 { self.avail.device_addr() } @@ -434,30 +360,6 @@ impl<'pool> Virtqueue<'pool> { self.notify_offset as u64 * multiplier as u64 } - /// Write one descriptor chain without publishing it. - /// Addressed by index, not [`DescSlot`]: the in-flight proof a slot carries belongs to the publisher, not here. - pub fn write_chain(&mut self, first_desc: u16, bufs: &[(u64, u32, BufDir)]) { - assert!( - (first_desc as usize + bufs.len()) <= self.size as usize, - "virtqueue: chain at {first_desc} of {} descriptors runs past a queue of {}", - bufs.len(), - self.size - ); - self.chain_bytes[first_desc as usize] = chain_bytes(bufs); - for (i, (addr, len, dir)) in bufs.iter().enumerate() { - let desc_idx = first_desc + i as u16; - let mut flags: u16 = match dir { - BufDir::Readable => 0, - BufDir::Writable => VIRTQ_DESC_F_WRITE, - }; - if i != bufs.len() - 1 { - flags |= VIRTQ_DESC_F_NEXT; - } - let desc = VirtqDesc { addr: *addr, len: *len, flags, next: desc_idx + 1 }; - self.desc.write(desc_idx as usize * core::mem::size_of::(), desc); - } - } - /// Where the used element at ring position `i` sits. fn used_elem_at(&self, i: u16) -> usize { USED_RING_OFF + i as usize * USED_ELEM_SIZE @@ -541,7 +443,6 @@ impl<'pool> Virtqueue<'pool> { /// Check if the device has completed any request. pub fn has_used(&self) -> bool { - assert!(!self.used_split, "virtqueue: used ring split off"); let used_idx: u16 = self.used.read(USED_IDX_OFF); used_idx != self.last_used_idx } @@ -550,13 +451,13 @@ impl<'pool> Virtqueue<'pool> { /// A refused element is skipped, never returned, so one forged element cannot hide the ones behind it. /// Never logs: the caller may hold `serial::BackendGuard`, the lock the log backend itself takes. pub fn poll_used(&mut self) -> Option<(DescSlot, u32)> { - assert!(!self.used_split, "virtqueue: used ring split off"); loop { let used_idx: u16 = self.used.read(USED_IDX_OFF); if used_idx == self.last_used_idx { return None; } - // The element after the idx that counts it, as in `UsedRingConsumer::poll`. + // The device writes the element before it bumps the idx (virtio 1.2 + // §2.7.8), so the element is read after the idx that counts it. barrier::dma_rmb(); let slot = self.last_used_idx % self.size; let id = self.used_ring_id(slot); @@ -571,8 +472,7 @@ impl<'pool> Virtqueue<'pool> { /// What a used-ring element must satisfy: a head inside this queue's table, at a published /// chain, written no further than that chain. Refused rather than clamped: there is nothing - /// here to recover from a forged completion, and userland maps virtio-sound's control and - /// event queues writable, so neither device-written field is trustworthy unchecked. + /// here to recover from a forged completion. fn parse_used(&self, id: Untrusted, len: Untrusted) -> Option<(DescSlot, u32)> { // `chain_bytes` is exactly `size` long: the descriptor table's own bound, not a constant beside it. let head = id.index(self.chain_bytes.len()).ok()?; @@ -809,22 +709,6 @@ pub fn cap_selftest() { log!("virtio: pci cap selftest {passed}/{CASES}"); } -/// Which of a device's two interrupt sources it declined to bind — not a driver or kernel bug. -#[derive(Debug, Clone, Copy)] -pub enum NoVector { - Config, - Queue(u16), -} - -impl core::fmt::Display for NoVector { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - match self { - Self::Config => write!(f, "its configuration-change interrupt"), - Self::Queue(queue) => write!(f, "queue {queue}'s interrupt"), - } - } -} - /// A fully initialized VirtIO device. pub struct VirtioDevice { config: VirtioPciConfig, @@ -916,22 +800,6 @@ impl VirtioDevice { common.write_u16(COMMON_QUEUE_ENABLE, 1); } - /// Point the device's config-change and `queue`'s used-ring interrupt at `pci::MSIX_ENTRY`. - /// Kept separate from `PciDevice::enable_msix`: both calls are needed, neither implies the other. - pub fn bind_msix(&self, queue: u16) -> Result<(), NoVector> { - let common = self.config.common; - common.write_u16(COMMON_MSIX_CONFIG, super::pci::MSIX_ENTRY); - if common.read_u16(COMMON_MSIX_CONFIG) == NO_VECTOR { - return Err(NoVector::Config); - } - common.write_u16(COMMON_QUEUE_SELECT, queue); - common.write_u16(COMMON_QUEUE_MSIX, super::pci::MSIX_ENTRY); - if common.read_u16(COMMON_QUEUE_MSIX) == NO_VECTOR { - return Err(NoVector::Queue(queue)); - } - Ok(()) - } - /// Set DRIVER_OK — device is now live. pub fn activate(&self) { let status = STATUS_ACKNOWLEDGE as u32 @@ -948,8 +816,4 @@ impl VirtioDevice { pub fn notify_off_multiplier(&self) -> u32 { self.config.notify_off_multiplier } - - pub fn device_config(&self) -> Mmio { - self.config.device - } } diff --git a/kernel/src/drivers/virtio_sound.rs b/kernel/src/drivers/virtio_sound.rs deleted file mode 100644 index e39ff6c5b76..00000000000 --- a/kernel/src/drivers/virtio_sound.rs +++ /dev/null @@ -1,454 +0,0 @@ -//! virtio-sound: bring-up, the virtqueues, and the register allow-list. -//! -//! Every DMA address lives in the descriptor tables, built once at bind from -//! kernel-allocated offsets; after bind the driver's whole vocabulary is an -//! avail-ring index and a doorbell write. Stream selection, format and timing -//! are soundserver's, not this driver's. -//! -//! Structure layouts and command codes follow VirtIO 1.2 §5.14. - -use core::cell::UnsafeCell; -use core::sync::atomic::{AtomicBool, AtomicU32, AtomicU64, Ordering}; - -use toyos_abi::audio::AudioCompletionRecord; -use toyos_abi::syscall::{RegWidth, SyscallError}; -use toyos_abi::virtio_sound as abi; - -use super::pci::{PciDevice, MSIX_ENTRY}; -use super::virtio::{BufDir, UsedRingConsumer, VirtioDevice, Virtqueue, VirtqueueRegions, - VIRTIO_F_VERSION_1}; -use super::DmaPool; -use crate::log; -use crate::mm::policy::CachePolicy; -use crate::mm::{Dma, Mmio}; -use crate::object::shm::Region; -use crate::sync::Lock; - -const VIRTIO_VENDOR: u16 = 0x1AF4; -const VIRTIO_SND_DEVICE: u16 = 0x1059; // 0x1040 + device_id 25 - -/// Size of the TX transfer header, which QEMU subtracts from the chain's -/// readable length to get the PCM byte count; the kernel never reads its contents. -const XFER_HEADER_BYTES: u32 = 4; -/// The per-period status the device writes back: status and latency, two `le32`. -const STATUS_BYTES: u32 = 8; -/// One event: a code and its data. -const EVENT_BYTES: u32 = 8; - -/// Kernel-only DMA page: three descriptor tables plus the TX used ring the -/// handler alone consumes. -const OFF_CTRL_DESC: usize = 0x0000; -const OFF_EVENT_DESC: usize = 0x0400; -const OFF_TX_DESC: usize = 0x0800; -const OFF_TX_USED: usize = 0x0C00; -const KERNEL_DMA_BYTES: usize = 0x1000; - -const _: () = { - use super::virtio::DESC_BYTES; - assert!(abi::CONTROL_QUEUE_SIZE as usize * DESC_BYTES <= OFF_EVENT_DESC - OFF_CTRL_DESC); - assert!(abi::EVENT_QUEUE_SIZE as usize * DESC_BYTES <= OFF_TX_DESC - OFF_EVENT_DESC); - assert!(abi::TX_QUEUE_SIZE as usize * DESC_BYTES <= OFF_TX_USED - OFF_TX_DESC); - assert!(abi::used_bytes(abi::TX_QUEUE_SIZE) <= KERNEL_DMA_BYTES - OFF_TX_USED); -}; - -/// Cap on logged refusals, past which a misbehaving driver can't spend unbounded log. -const MAX_NAMED_REFUSALS: usize = 16; - - -/// Written once before the vector arms and read without a lock afterwards — -/// not lock-guarded because the handler may interrupt a CPU holding [`CONTROLLER`]. -struct TxIsr { - consumer: UnsafeCell>>, - /// Stray used-ring entries (head names no chain); a userland bug, so counted - /// rather than logged from the ISR. - stray: AtomicU32, - named_stray: AtomicBool, -} - -// SAFETY: `consumer` is write-once before the vector arms and read only by the -// handler after; every other field is atomic. -unsafe impl Sync for TxIsr {} - -static TX_ISR: TxIsr = TxIsr { - consumer: UnsafeCell::new(None), - stray: AtomicU32::new(0), - named_stray: AtomicBool::new(false), -}; - -/// Drain the TX used ring; a head naming no chain is untrusted input, counted not asserted. -fn drain_tx() -> u32 { - // SAFETY: sole accessor after init — see `TxIsr`. - let consumer = unsafe { &mut *TX_ISR.consumer.get() }; - // A configuration-change interrupt shares this vector and may arrive before init installs the consumer. - let Some(consumer) = consumer.as_mut() else { return 0 }; - let mut mask = 0u32; - let refused_before = consumer.refused(); - while let Some(head) = consumer.poll() { - let idx = head as usize / abi::TX_CHAIN as usize; - if idx >= abi::PERIODS || head % abi::TX_CHAIN != 0 { - TX_ISR.stray.fetch_add(1, Ordering::Relaxed); - continue; - } - mask |= 1 << idx; - } - // Folds in the consumer's own refusals; a head past the queue never reaches the loop above. - let refused = consumer.refused() - refused_before; - if refused != 0 { - TX_ISR.stray.fetch_add(refused, Ordering::Relaxed); - } - mask -} - -/// Rust half of the MSI-X handler, called from the IDT entry. -pub fn isr_complete() { - // Timestamp first — this is the hardware-completion time the DLL feeds on. - let timestamp = crate::clock::nanos_since_boot(); - let mask = drain_tx(); - if mask == 0 { - return; - } - isr_push_completion(mask, timestamp); - super::AUDIO_WATCH.post_in_place(); - crate::preempt::set_need_resched(); -} - - -const RECORD_RING_CAP: u32 = 16; - -/// SPSC: producer is the MSI-X handler (single CPU, IF=0); consumer holds [`CONTROLLER`]. -/// One record per interrupt, never accumulated — a folded mask would misreport lateness to soundserver's DLL. -struct RecordRing { - slots: [UnsafeCell; RECORD_RING_CAP as usize], - head: AtomicU32, - tail: AtomicU32, -} - -const _: () = assert!( - RECORD_RING_CAP as usize >= abi::PERIODS, - "record ring must hold one record per period" -); - -// SAFETY: slot access is arbitrated by the head/tail protocol above. -unsafe impl Sync for RecordRing {} - -static RECORDS: RecordRing = RecordRing { - slots: [const { - UnsafeCell::new(AudioCompletionRecord { mask: 0, _pad: 0, timestamp_nanos: 0 }) - }; RECORD_RING_CAP as usize], - head: AtomicU32::new(0), - tail: AtomicU32::new(0), -}; - -/// Overflow sink for a full ring: mask is OR'd (idempotent) and timestamp is -/// newest-wins, so a driver that stops reading costs itself only timestamp granularity. -struct Spill { - mask: AtomicU32, - timestamp: AtomicU64, -} - -static SPILL: Spill = Spill { mask: AtomicU32::new(0), timestamp: AtomicU64::new(0) }; - -fn isr_push_completion(mask: u32, timestamp_nanos: u64) { - let ring = &RECORDS; - let head = ring.head.load(Ordering::Relaxed); // sole writer of head - let tail = ring.tail.load(Ordering::Acquire); - if head.wrapping_sub(tail) >= RECORD_RING_CAP { - SPILL.timestamp.store(timestamp_nanos, Ordering::Relaxed); - SPILL.mask.fetch_or(mask, Ordering::Release); - return; - } - let slot = (head % RECORD_RING_CAP) as usize; - // SAFETY: slot is outside [tail, head) — not visible to the consumer. - unsafe { - *ring.slots[slot].get() = AudioCompletionRecord { mask, _pad: 0, timestamp_nanos }; - } - // Release publishes the record before the consumer can observe the new head. - ring.head.store(head.wrapping_add(1), Ordering::Release); -} - -/// Pop the oldest pending record; called under [`CONTROLLER`], so the tail store -/// needs no CAS. Spill returns last because it is always newer than anything still queued. -fn pop_completion() -> Option { - let ring = &RECORDS; - let tail = ring.tail.load(Ordering::Relaxed); // sole writer of tail - // Acquire pairs with the producer's Release store of head. - let head = ring.head.load(Ordering::Acquire); - if head == tail { - let mask = SPILL.mask.swap(0, Ordering::AcqRel); - if mask == 0 { - return None; - } - return Some(AudioCompletionRecord { - mask, - _pad: 0, - timestamp_nanos: SPILL.timestamp.load(Ordering::Relaxed), - }); - } - // SAFETY: `tail != head` puts this slot in `[tail, head)`, unwritten by the - // producer until wrap; the Acquire load of `head` above pairs with its Release store. - let rec = unsafe { *ring.slots[(tail % RECORD_RING_CAP) as usize].get() }; - ring.tail.store(tail.wrapping_add(1), Ordering::Release); - Some(rec) -} - -/// True if completion records are pending; lock-free. -pub fn has_pending() -> bool { - RECORDS.head.load(Ordering::Acquire) != RECORDS.tail.load(Ordering::Acquire) - || SPILL.mask.load(Ordering::Acquire) != 0 -} - -/// Copy up to `buf.len() / 16` pending records into `buf`, oldest first, and -/// name a stray completion the first time one has been counted. -pub fn drain_completed(buf: &mut crate::user_ptr::UserBytesMut) -> usize { - let stray = TX_ISR.stray.load(Ordering::Relaxed); - if stray != 0 && !TX_ISR.named_stray.swap(true, Ordering::Relaxed) { - log!( - "virtio-sound: the device completed a chain this driver never built ({stray} so far) \ - — a used-ring head past the queue, or one that heads no chain" - ); - } - let max = buf.len() / AudioCompletionRecord::SIZE; - let _guard = CONTROLLER.lock(); - let mut written = 0; - for _ in 0..max { - let Some(rec) = pop_completion() else { break }; - // Field-wise serialization — never expose struct padding. - let mut record = [0u8; AudioCompletionRecord::SIZE]; - record[0..4].copy_from_slice(&rec.mask.to_le_bytes()); - record[8..16].copy_from_slice(&rec.timestamp_nanos.to_le_bytes()); - buf.write_at(written, &record); - written += AudioCompletionRecord::SIZE; - } - written -} - - -/// Notify region only; virtqueues and DMA pools are leaked at bring-up because -/// `TX_ISR` holds a used-ring consumer into one of them for the life of the boot. -struct Bound { - notify: Mmio, -} - -static CONTROLLER: Lock> = Lock::new(None); -static INFO: Lock> = Lock::new(None); -static REFUSALS: AtomicU32 = AtomicU32::new(0); - -pub fn info() -> Option<(abi::VirtioSoundInfo, Region)> { - INFO.lock().clone() -} - - -/// Allow-list for the three doorbells; a doorbell value is a queue index, not an -/// address, so it can reach nothing the already-selected offset did not. -fn write_permit(info: &abi::VirtioSoundInfo, offset: u64, width: RegWidth) -> bool { - let Ok(offset) = u32::try_from(offset) else { return false }; - width == RegWidth::U16 - && [info.notify_control, info.notify_event, info.notify_tx].contains(&offset) -} - -fn refuse(what: &str, offset: u64, width: RegWidth) -> SyscallError { - if REFUSALS.fetch_add(1, Ordering::Relaxed) < MAX_NAMED_REFUSALS as u32 { - log!("virtio-sound: refused a {width:?} {what} of {offset:#x} — not on the allow-list"); - } - SyscallError::PermissionDenied -} - -/// No register is readable; every read is a refusal — answers arrive via memory, not MMIO. -pub fn reg_read(offset: u64, width: RegWidth) -> Result { - Err(refuse("read", offset, width)) -} - -pub fn reg_write(offset: u64, width: RegWidth, value: u32) -> Result<(), SyscallError> { - let (info, _) = info().ok_or(SyscallError::NotFound)?; - if !write_permit(&info, offset, width) { - return Err(refuse("write", offset, width)); - } - if value > width.max_value() { - return Err(SyscallError::InvalidArgument); - } - let guard = CONTROLLER.lock(); - let controller = guard.as_ref().ok_or(SyscallError::NotFound)?; - controller.notify.write_u16(offset, value as u16); - Ok(()) -} - - -/// Bring up virtio-sound, or leave it unclaimed and log why — audio is optional, -/// so a refusal beats a panic over a peripheral. -pub fn init(devices: &[PciDevice]) { - let Some(pci) = devices.iter().find(|d| d.is_id(VIRTIO_VENDOR, VIRTIO_SND_DEVICE)) else { - return; - }; - log!("virtio-sound: found at PCI {:02x}:{:02x}.{}", pci.bus, pci.dev, pci.func); - - let device = match VirtioDevice::init(pci, VIRTIO_F_VERSION_1) { - Ok(device) => device, - Err(why) => { - log!("virtio-sound: NOT INITIALISED — PCI {:02x}:{:02x}.{} {why}", - pci.bus, pci.dev, pci.func); - return; - } - }; - - let cfg = device.device_config(); - let (jacks, streams, chmaps) = (cfg.read_u32(0), cfg.read_u32(4), cfg.read_u32(8)); - log!("virtio-sound: {jacks} jacks, {streams} streams, {chmaps} chmaps"); - if streams == 0 { - log!("virtio-sound: NOT INITIALISED — the device offers no PCM stream to play into"); - return; - } - - // Placed after the stream check so a streamless device allocates nothing; see [`Bound`]. - let space = crate::iommu::DeviceSpace::create(); - let kernel_mem = DmaPool::alloc_in(KERNEL_DMA_BYTES, space).leak(); - let shared = DmaPool::alloc_in(abi::SHARED_BYTES, space).leak(); - // Before the device is told any address, and after both pools' mappings. - space.attach(pci.bus, pci.dev, pci.func); - // Exclusive: just allocated, not yet told to the device or mapped to userland. - shared.zero(); - - let mut controlq = queue( - kernel_mem.subview(OFF_CTRL_DESC, OFF_EVENT_DESC - OFF_CTRL_DESC), - shared.subview(abi::OFF_CTRL_AVAIL, abi::avail_bytes(abi::CONTROL_QUEUE_SIZE)), - shared.subview(abi::OFF_CTRL_USED, abi::used_bytes(abi::CONTROL_QUEUE_SIZE)), - abi::CONTROL_QUEUE_SIZE, - ); - let mut eventq = queue( - kernel_mem.subview(OFF_EVENT_DESC, OFF_TX_DESC - OFF_EVENT_DESC), - shared.subview(abi::OFF_EVENT_AVAIL, abi::avail_bytes(abi::EVENT_QUEUE_SIZE)), - shared.subview(abi::OFF_EVENT_USED, abi::used_bytes(abi::EVENT_QUEUE_SIZE)), - abi::EVENT_QUEUE_SIZE, - ); - // TX used ring lives in kernel memory only — userland must never fabricate a completion by rewriting it. - let mut txq = queue( - kernel_mem.subview(OFF_TX_DESC, OFF_TX_USED - OFF_TX_DESC), - shared.subview(abi::OFF_TX_AVAIL, abi::avail_bytes(abi::TX_QUEUE_SIZE)), - kernel_mem.subview(OFF_TX_USED, abi::used_bytes(abi::TX_QUEUE_SIZE)), - abi::TX_QUEUE_SIZE, - ); - - build_chains(&mut controlq, &mut eventq, &mut txq, shared.device_addr()); - - // Installed before the vector can fire, so no interrupt observes a half-written Option. - // SAFETY: MSI-X is not enabled yet. - unsafe { *TX_ISR.consumer.get() = Some(txq.split_used_consumer()) }; - - device.setup_queue(abi::CONTROL_QUEUE, &mut controlq); - device.setup_queue(abi::EVENT_QUEUE, &mut eventq); - device.setup_queue(abi::TX_QUEUE, &mut txq); - if !arm_interrupt(pci, &device) { - return; - } - device.enable_queue(abi::CONTROL_QUEUE); - device.enable_queue(abi::EVENT_QUEUE); - device.enable_queue(abi::TX_QUEUE); - device.activate(); - - // DmaPool allocations are whole 2 MiB pages; ABI offsets are relative to that page. - let dma_region = Region { - phys: crate::DirectMap::from_phys(shared.host_phys()), - size: crate::mm::PAGE_2M, - cache: CachePolicy::Normal, - pages: None, - }; - let multiplier = device.notify_off_multiplier(); - let info = abi::VirtioSoundInfo { - dma: toyos_abi::HANDLE_INVALID, - notify_control: controlq.notify_bytes(multiplier) as u32, - notify_event: eventq.notify_bytes(multiplier) as u32, - notify_tx: txq.notify_bytes(multiplier) as u32, - jacks, - streams, - chmaps, - }; - - *CONTROLLER.lock() = Some(Bound { notify: device.notify_mmio() }); - *INFO.lock() = Some((info, dma_region)); - - log!( - "virtio-sound: bound, {} periods of {} bytes, doorbells at {:#x}/{:#x}/{:#x}", - abi::PERIODS, - abi::PERIOD_BYTES, - info.notify_control, - info.notify_event, - info.notify_tx - ); -} - -fn queue<'pool>( - desc: Dma<'pool>, - avail: Dma<'pool>, - used: Dma<'pool>, - size: u16, -) -> Virtqueue<'pool> { - Virtqueue::from_regions(&VirtqueueRegions::from_separate(desc, avail, used, size), size) -} - -/// Builds every chain once; after this no descriptor is ever written again — the -/// driver's whole vocabulary becomes an avail-ring index and a doorbell write. -fn build_chains( - controlq: &mut Virtqueue<'_>, - eventq: &mut Virtqueue<'_>, - txq: &mut Virtqueue<'_>, - base: u64, -) { - let at = |offset: usize| base + offset as u64; - - // One chain serves every command: the device reads the header first and - // takes only what that command defines. - controlq.write_chain( - 0, - &[ - (at(abi::OFF_CTRL_REQ), abi::CTRL_BUF_BYTES as u32, BufDir::Readable), - (at(abi::OFF_CTRL_RESP), abi::CTRL_BUF_BYTES as u32, BufDir::Writable), - ], - ); - - // One descriptor per buffer: buffer index equals descriptor index. - for i in 0..abi::EVENT_BUFS { - eventq.write_chain( - i as u16, - &[(at(abi::OFF_EVENT_BUFS + i * abi::EVENT_BUF_STRIDE), EVENT_BYTES, BufDir::Writable)], - ); - } - - for i in 0..abi::PERIODS { - txq.write_chain( - abi::tx_chain_head(i), - &[ - (at(abi::OFF_TX_XFER + i * abi::XFER_STRIDE), XFER_HEADER_BYTES, BufDir::Readable), - (at(abi::OFF_PCM + i * abi::PERIOD_BYTES), abi::PERIOD_BYTES as u32, BufDir::Readable), - (at(abi::OFF_TX_STATUS + i * abi::STATUS_STRIDE), STATUS_BYTES, BufDir::Writable), - ], - ); - } -} - -/// Arm the TX completion interrupt, or refuse and log why; never panics. The -/// handler is the TX used ring's only consumer, so an unarmed device leaves -/// every period in flight forever. -fn arm_interrupt(pci: &PciDevice, device: &VirtioDevice) -> bool { - let vector = crate::arch::trap::VIRTIO_SOUND_VECTOR; - if pci.enable_msix(vector).is_none() { - 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", - pci.bus, - pci.dev, - pci.func - ); - return false; - } - if let Err(refused) = device.bind_msix(abi::TX_QUEUE) { - log!( - "virtio-sound: NOT INITIALISED at PCI {:02x}:{:02x}.{} — the device refused a vector \ - for {refused}", - pci.bus, - pci.dev, - pci.func - ); - return false; - } - log!("virtio-sound: MSI-X vector {vector:#x} on table entry {MSIX_ENTRY}"); - true -} diff --git a/kernel/src/irq_census.rs b/kernel/src/irq_census.rs index 66dec389b6e..28537c0f95b 100644 --- a/kernel/src/irq_census.rs +++ b/kernel/src/irq_census.rs @@ -26,8 +26,6 @@ pub enum Source { /// claim an interrupt belonged to is the claim's own record, and the census /// is about this machine's interrupt routing. UserDev, - /// Vector 0x23, virtio-sound MSI-X. - Sound, /// Vector 0x24, the i8042's I/O APIC pin — both PS/2 lines. I8042, /// Vector 0x25, the remapping unit's fault event. @@ -47,11 +45,11 @@ pub enum Source { } impl Source { - pub const COUNT: usize = 12; + pub const COUNT: usize = 11; /// Order `tests/toyos.rs`'s `irq_census_conservation` parses back; must match variant order. pub const NAMES: [&'static str; Self::COUNT] = [ - "timer", "kick", "xhci", "userdev", "sound", "i8042", "dmafault", "hda", "tlb", "nmi", + "timer", "kick", "xhci", "userdev", "i8042", "dmafault", "hda", "tlb", "nmi", "spurious", "unclaimed", ]; } diff --git a/kernel/src/main.rs b/kernel/src/main.rs index 64464538a19..4ccb461b872 100644 --- a/kernel/src/main.rs +++ b/kernel/src/main.rs @@ -111,7 +111,7 @@ mod late_panic { use crate::mm::policy::MmioPolicy; use alloc::boxed::Box; use arch::{cpu, percpu}; -use drivers::{acpi, gop, pci, serial, virtio_console, virtio_gpu, virtio_sound, xhci}; +use drivers::{acpi, gop, pci, serial, virtio_console, virtio_gpu, xhci}; use toyos_abi::boot::{KernelArgs, MemoryMapEntry}; use toyos_rootimage::handoff::{held, Descriptor}; @@ -525,7 +525,6 @@ pub(crate) unsafe extern "C" fn kernel_main(loader_args: &mut KernelArgs) -> ! { virtio_console::init(&pci_devices); - virtio_sound::init(&pci_devices); drivers::hda::init(&pci_devices); if let Some((gpu_driver, gpu_info)) = virtio_gpu::init(&pci_devices) { diff --git a/kernel/src/object/device.rs b/kernel/src/object/device.rs index b2423518eaf..d6b1a3279bc 100644 --- a/kernel/src/object/device.rs +++ b/kernel/src/object/device.rs @@ -24,7 +24,6 @@ pub enum DeviceInfo { /// own memory, so this mint installs nothing. PciFunction(toyos_abi::pci::PciFunctionInfo), Hda(toyos_abi::hda::HdaInfo, Arc), - VirtioSound(toyos_abi::virtio_sound::VirtioSoundInfo, Arc), /// Which partition, how long, and both its GUIDs; the view it moves blocks /// through is the claim's own (`device::Claim::partition`). Partition(toyos_abi::part::PartitionInfo), @@ -82,11 +81,6 @@ impl DeviceInfo { info.pcm = install_buffers(table, &[pcm])?[0]; bytes(&info).into() } - Self::VirtioSound(info, dma) => { - let mut info = *info; - info.dma = install_buffers(table, &[dma])?[0]; - bytes(&info).into() - } }) } } diff --git a/kernel/src/object/ops.rs b/kernel/src/object/ops.rs index f04c3222fc4..d5e3ba8577e 100644 --- a/kernel/src/object/ops.rs +++ b/kernel/src/object/ops.rs @@ -287,9 +287,7 @@ pub fn read_watch(object: &KObjectRef) -> Option { device_registry::DeviceType::PciFunction | device_registry::DeviceType::Isa | device_registry::DeviceType::Acpi => Some(WatchRef::Claim(d.clone())), - device_registry::DeviceType::HdaAudio | device_registry::DeviceType::VirtioSound => { - Some(WatchRef::Irq(&crate::drivers::AUDIO_WATCH)) - } + device_registry::DeviceType::HdaAudio => Some(WatchRef::Irq(&crate::drivers::AUDIO_WATCH)), device_registry::DeviceType::Framebuffer => None, // A partition answers its description and has nothing to wait for. device_registry::DeviceType::Partition => None, @@ -362,7 +360,6 @@ fn close_ends_polls(object: &KObjectRef) -> bool { | device_registry::DeviceType::Acpi => false, device_registry::DeviceType::Mouse | device_registry::DeviceType::HdaAudio - | device_registry::DeviceType::VirtioSound | device_registry::DeviceType::Framebuffer | device_registry::DeviceType::Partition => true, }, @@ -506,17 +503,6 @@ pub fn read_device( let n = crate::drivers::hda::drain_completed(buf); if n == 0 { None } else { Some(n as u64) } } - device_registry::DeviceType::VirtioSound => { - if !claim.info_read() { - return Some(claim.describe(table, buf)); - } - if buf.len() < toyos_abi::audio::AudioCompletionRecord::SIZE { - return Some(SyscallError::InvalidArgument.to_u64()); - } - // Completion records, oldest first; empty answers `None` so a blocking read parks. - let n = crate::drivers::virtio_sound::drain_completed(buf); - if n == 0 { None } else { Some(n as u64) } - } } } @@ -692,8 +678,7 @@ pub fn fstat(object: &KObjectRef) -> Stat { device_registry::DeviceType::PciFunction | device_registry::DeviceType::Isa | device_registry::DeviceType::Acpi => FileType::Unknown, - device_registry::DeviceType::HdaAudio - | device_registry::DeviceType::VirtioSound => FileType::Unknown, + device_registry::DeviceType::HdaAudio => FileType::Unknown, device_registry::DeviceType::Partition => FileType::Unknown, }), } @@ -770,7 +755,6 @@ fn partition_fsync(claim: &DeviceClaim) -> u64 { | device_registry::DeviceType::Mouse | device_registry::DeviceType::Framebuffer | device_registry::DeviceType::HdaAudio - | device_registry::DeviceType::VirtioSound | device_registry::DeviceType::PciFunction | device_registry::DeviceType::Isa | device_registry::DeviceType::Acpi => { @@ -849,9 +833,6 @@ pub fn has_data(object: &KObjectRef) -> bool { device_registry::DeviceType::HdaAudio => { !d.info_read() || crate::drivers::hda::has_pending() } - device_registry::DeviceType::VirtioSound => { - !d.info_read() || crate::drivers::virtio_sound::has_pending() - } }, KObjectRef::PipeWrite(_) | KObjectRef::Inbox(_) | KObjectRef::SysCap(_) | KObjectRef::Connector(_) | KObjectRef::Namespace(_) @@ -916,7 +897,6 @@ fn write_device(claim: &DeviceClaim, buf: &UserBytes) -> u64 { | device_registry::DeviceType::Mouse | device_registry::DeviceType::Framebuffer | device_registry::DeviceType::HdaAudio - | device_registry::DeviceType::VirtioSound | device_registry::DeviceType::PciFunction | device_registry::DeviceType::Partition => return SyscallError::PermissionDenied.to_u64(), } diff --git a/kernel/src/syscall/device.rs b/kernel/src/syscall/device.rs index 05d4b7dfe82..64aebc035d2 100644 --- a/kernel/src/syscall/device.rs +++ b/kernel/src/syscall/device.rs @@ -41,7 +41,6 @@ pub(super) fn holds_claim( /// The register-access stub a device claim's class selects. enum RegTarget { Hda, - VirtioSound, /// A claimed PCI function's own config space, **read-only**, through the /// claim itself: what names the function is lent by it for each access. /// @@ -63,7 +62,6 @@ pub(super) fn sys_device_reg(handle: RawHandle, offset: u64, width: u64, value: let target = process::with_process_data(|data| { data.handles.get::(handle, Rights::NONE).map(|claim| match claim.class() { device::DeviceType::HdaAudio => Some(RegTarget::Hda), - device::DeviceType::VirtioSound => Some(RegTarget::VirtioSound), device::DeviceType::PciFunction => Some(RegTarget::PciConfig(claim)), _ => None, }) @@ -81,7 +79,6 @@ pub(super) fn sys_device_reg(handle: RawHandle, offset: u64, width: u64, value: None => { let read = match target { RegTarget::Hda => crate::drivers::hda::reg_read(offset, width), - RegTarget::VirtioSound => crate::drivers::virtio_sound::reg_read(offset, width), // The one place a caller's offset becomes an access: the // witness `pcidev::config_window` answers is the only thing // the register read takes, so an unchecked number cannot @@ -102,9 +99,6 @@ pub(super) fn sys_device_reg(handle: RawHandle, offset: u64, width: u64, value: Ok(value) => { let written = match target { RegTarget::Hda => crate::drivers::hda::reg_write(offset, width, value), - RegTarget::VirtioSound => { - crate::drivers::virtio_sound::reg_write(offset, width, value) - } // Config space has no write path from userland at all. RegTarget::PciConfig(_) => Err(SyscallError::NotSupported), }; @@ -180,7 +174,6 @@ pub(super) fn sys_device_claim(syscap: RawHandle, class: u64, selector: [u64; 2] | device::DeviceType::Mouse | device::DeviceType::Framebuffer | device::DeviceType::HdaAudio - | device::DeviceType::VirtioSound | device::DeviceType::Acpi => 0, device::DeviceType::PciFunction => 1, device::DeviceType::Partition | device::DeviceType::Isa => 2, @@ -470,7 +463,6 @@ pub(super) fn sys_partition_transfer( | device::DeviceType::Mouse | device::DeviceType::Framebuffer | device::DeviceType::HdaAudio - | device::DeviceType::VirtioSound | device::DeviceType::PciFunction | device::DeviceType::Isa | device::DeviceType::Acpi => { diff --git a/kernel/src/syscall/io.rs b/kernel/src/syscall/io.rs index 89a4a03d3cd..ced9afa8f3d 100644 --- a/kernel/src/syscall/io.rs +++ b/kernel/src/syscall/io.rs @@ -31,7 +31,6 @@ enum WriteBlock { /// What `sys_read` parks on when the handle has nothing to give. enum ReadBlock { Pipe(alloc::sync::Arc, pipe::PipeId, WaitClass), - VirtioSound, Hda, /// A claimed keyboard, woken by its own IRQ. Keyboard(Deadline), @@ -90,12 +89,11 @@ pub(super) fn sys_write(h: RawHandle, buf: &UserBytes) -> u64 { } } -/// Only these four device classes block; the rest answer `NotFound` on an +/// Only these device classes block; the rest answer `NotFound` on an /// empty blocking read. fn read_block_device(claim: &crate::object::device::DeviceClaim) -> ReadBlock { match claim.class() { device::DeviceType::Keyboard => ReadBlock::Keyboard(Deadline::never()), - device::DeviceType::VirtioSound if claim.info_read() => ReadBlock::VirtioSound, device::DeviceType::HdaAudio if claim.info_read() => ReadBlock::Hda, _ => ReadBlock::Refused(SyscallError::NotFound.to_u64()), } @@ -168,21 +166,6 @@ pub(super) fn sys_read(h: RawHandle, buf: &mut UserBytesMut) -> u64 { return cancelled(); } } - Err(ReadBlock::VirtioSound) => { - let parkable = crate::scheduler::Parkable::at_entry(); - if watch::wait_until( - &parkable, - &crate::drivers::AUDIO_WATCH, - 0, - WaitClass::Io, - Deadline::never(), - crate::drivers::virtio_sound::has_pending, - ) - .is_err() - { - return cancelled(); - } - } Err(ReadBlock::Hda) => { let parkable = crate::scheduler::Parkable::at_entry(); if watch::wait_until( diff --git a/src/sourcegate.rs b/src/sourcegate.rs index 9df06dc8134..46aa8e7343e 100644 --- a/src/sourcegate.rs +++ b/src/sourcegate.rs @@ -140,7 +140,6 @@ const AUTO_TRAIT_IMPLS: &[(&str, usize)] = &[ ("kernel/src/drivers/hda.rs", 1), ("kernel/src/drivers/panic_console/mod.rs", 3), ("kernel/src/drivers/virtio_console.rs", 1), - ("kernel/src/drivers/virtio_sound.rs", 2), ("kernel/src/arch/x86_64/hw.rs", 1), ("kernel/src/mm/mmio.rs", 2), ("kernel/src/mm/region.rs", 2), diff --git a/system.toml b/system.toml index fdc2ef3c6c2..ba018d9f89c 100644 --- a/system.toml +++ b/system.toml @@ -74,7 +74,7 @@ login = true [programs.soundserver] service = true serves = ["soundserver"] -devices = ["hda-audio", "virtio-sound"] +devices = ["hda-audio", "pci:1af4:1059"] syscap = ["rt"] # netstack holds the NIC's PCI function and drives it: the virtqueues, the register diff --git a/tests/common/iommu.rs b/tests/common/iommu.rs index a82220cb98b..ed0a3fb8b3b 100644 --- a/tests/common/iommu.rs +++ b/tests/common/iommu.rs @@ -1,6 +1,7 @@ //! `iommu_virtio_platform`: whether each virtio function QEMU creates behind //! its emulated VT-d unit, and none created without one, negotiated -//! `VIRTIO_F_ACCESS_PLATFORM`. +//! `VIRTIO_F_ACCESS_PLATFORM`, and that a process's claim on one is refused +//! where no unit is. use std::collections::{BTreeMap, BTreeSet}; use std::path::Path; @@ -11,6 +12,9 @@ use super::serial::Serial; /// The function `tests/netcase`'s netstack claims, as its `devices` row spells it. const NETSTACK_CLAIMS: &str = "1af4:1041"; +/// soundserver's feature line, the kernel's shape under soundserver's name. +const SOUNDSERVER_NEGOTIATED: &str = "soundserver: VirtIO: PCI "; + /// fileserver's word for DATA's directories served from memory. const IN_MEMORY: &str = "are in memory and will not survive a reboot"; @@ -69,13 +73,21 @@ pub fn iommu_virtio_platform(test_config: &Path) -> Result<(), String> { // program's line either. let mut qemu = QemuInstance::boot_with_options(&netcase(), &[], &[], options); let said: Vec = if behind_unit { - vec![CLAIM_BOUNDED.to_string(), NETSTACK_NEGOTIATED.to_string(), bar_moved(), msix_armed()] + vec![ + CLAIM_BOUNDED.to_string(), + NETSTACK_NEGOTIATED.to_string(), + SOUNDSERVER_NEGOTIATED.to_string(), + bar_moved(), + msix_armed(), + ] } else { vec![ DISKSERVER_REFUSED.to_string(), FILESERVER_WITHOUT_DATA.to_string(), not_handed_over(CLAIMED_AT, NOT_REMAPPED), not_handed_over(NVME_AT, NOT_REMAPPED), + not_handed_over(SOUND_AT, NOT_REMAPPED), + super::audio::NULL_SINK.to_string(), ] }; let mut text = qemu.boot_log().to_string(); @@ -115,7 +127,8 @@ pub fn iommu_virtio_platform(test_config: &Path) -> Result<(), String> { created.len() } else { no_unit_is_no_claim(&log)?; - created.len() - 1 + // The NIC's and the sound function's claims, refused. + created.len() - 2 }; let mut negotiated = Vec::new(); @@ -154,17 +167,21 @@ pub fn iommu_virtio_platform(test_config: &Path) -> Result<(), String> { } let sound = class_function(&log, "0401") .ok_or_else(|| format!("{name}: this machine enumerated no audio function"))?; - if !negotiated.iter().any(|(who, _)| *who == sound) { + if sound != SOUND_AT { + return Err(format!("{name}: the audio function is {sound}, where {SOUND_AT} is owed")); + } + if behind_unit != negotiated.iter().any(|(who, _)| *who == sound) { return Err(format!( - "{name}: the audio function {sound} negotiated nothing, so whether it is behind \ - the unit was never asked: {negotiated:?}" + "{name}: the audio function {sound} negotiated features = {}, where a process's \ + claim on it is owed = {behind_unit}: {negotiated:?}", + !behind_unit )); } eprintln!( " [iommu] {name}: {} virtio function(s) behind a unit = {behind_unit}, the audio \ function {sound} among them{}", negotiated.len(), - if behind_unit { "" } else { "; the NIC's claim refused for want of a unit" } + if behind_unit { "" } else { "; the NIC's and the audio function's claims refused for want of a unit" } ); } declining_is_not_free(test_config) @@ -185,6 +202,10 @@ const FILESERVER_WITHOUT_DATA: &str = /// the one its diskserver row claims. const NVME_AT: &str = "00:02.0"; +/// The slot QEMU's `-device` order puts the virtio-sound function on, the one +/// `tests/netcase`'s soundserver row claims. +const SOUND_AT: &str = "00:05.0"; + /// Why a machine with no unit hands no function over, in the kernel's words. const NOT_REMAPPED: &str = "its interrupts would not be remapped on this machine"; @@ -205,10 +226,14 @@ fn not_handed_over(at: &str, why: &str) -> String { /// mint, and netstack exits rather than driving anything — and the machine /// finishes booting, which is the half a refusal that panicked would fail. The /// NVMe controller is refused the same, and DATA with it by name: a disk that -/// is there and cannot be used is never answered with memory. +/// is there and cannot be used is never answered with memory. So is the +/// virtio-sound function, and soundserver takes its null sink. fn no_unit_is_no_claim(log: &Serial) -> Result<(), String> { // netstack's own exit is the third saying, and is not read here. - refused_claim(log, NETSTACK_CLAIMS, NOT_REMAPPED, &[NVME_AT])?; + refused_claim(log, NETSTACK_CLAIMS, NOT_REMAPPED, &[NVME_AT, SOUND_AT])?; + log.must_say(¬_handed_over(SOUND_AT, NOT_REMAPPED))?; + log.must_say("supervisor: soundserver: pci:1af4:1059 is on this machine and could not be handed over")?; + log.must_say(super::audio::NULL_SINK)?; log.must_say(¬_handed_over(NVME_AT, NOT_REMAPPED))?; log.must_say("supervisor: diskserver: pci:1b36:0010 is on this machine and could not be handed over")?; log.must_say(DISKSERVER_REFUSED)?; diff --git a/tests/common/irqcensus.rs b/tests/common/irqcensus.rs index bf45c5a80a7..1d758d7aa4c 100644 --- a/tests/common/irqcensus.rs +++ b/tests/common/irqcensus.rs @@ -15,8 +15,8 @@ use std::collections::BTreeMap; /// and [`Census::parse`] refuses a line whose fields are not exactly these, so /// a source added on one side and not the other is a red rather than a silently /// dropped column. -pub const SOURCES: [&str; 12] = [ - "timer", "kick", "xhci", "userdev", "sound", "i8042", "dmafault", "hda", "tlb", "nmi", "spurious", +pub const SOURCES: [&str; 11] = [ + "timer", "kick", "xhci", "userdev", "i8042", "dmafault", "hda", "tlb", "nmi", "spurious", "unclaimed", ]; @@ -25,7 +25,7 @@ pub const SOURCES: [&str; 12] = [ /// `MSG_ADDR` names physical destination 0 and the one I/O APIC pin this kernel /// routes goes to the BSP, so today every one of these is cpu0's alone. The day /// that stops being true is the day the track's change lands. -pub const DEVICE_SOURCES: [&str; 6] = ["xhci", "userdev", "sound", "i8042", "dmafault", "hda"]; +pub const DEVICE_SOURCES: [&str; 5] = ["xhci", "userdev", "i8042", "dmafault", "hda"]; /// One CPU's counters out of one `irq:` line. #[derive(Clone, Debug, PartialEq, Eq)] diff --git a/tests/logstallcase/system.toml b/tests/logstallcase/system.toml index 133b98cf3d5..6f790e6e673 100644 --- a/tests/logstallcase/system.toml +++ b/tests/logstallcase/system.toml @@ -18,7 +18,7 @@ syscap = ["logread"] [programs.soundserver] service = true serves = ["soundserver"] -devices = ["hda-audio", "virtio-sound"] +devices = ["hda-audio", "pci:1af4:1059"] syscap = ["rt"] [programs.test-runner] diff --git a/tests/metalcase/system.toml b/tests/metalcase/system.toml index 49bbffaaa84..6bbfc4c6056 100644 --- a/tests/metalcase/system.toml +++ b/tests/metalcase/system.toml @@ -31,7 +31,7 @@ devices = ["framebuffer", "keyboard", "mouse"] [programs.soundserver] service = true serves = ["soundserver"] -devices = ["hda-audio", "virtio-sound"] +devices = ["hda-audio", "pci:1af4:1059"] syscap = ["rt"] # netstack holds the NIC's PCI function and drives it: the virtqueues, the register diff --git a/tests/netcase/system.toml b/tests/netcase/system.toml index e3bbd82f857..e956a131d72 100644 --- a/tests/netcase/system.toml +++ b/tests/netcase/system.toml @@ -1,5 +1,6 @@ # The one boot that runs netstack with a virtio NIC in front of it, with diskserver's -# NVMe controller beside it: `iommu_virtio_platform`'s machine. +# NVMe controller and soundserver's virtio-sound beside it: `iommu_virtio_platform`'s +# machine, where every virtio function is a process's claim or a kernel driver's. # # netstack's `main` opens the NIC before anything else and returns on `NotFound`, # so a config with no NIC never reaches a line of the daemon proper. @@ -8,7 +9,7 @@ # announce itself. [boot] -start = ["logkeeper", "diskserver", "fileserver", "netstack", "test-runner"] +start = ["logkeeper", "diskserver", "fileserver", "netstack", "soundserver", "test-runner"] # `every_boot_config_runs_logkeeper` is what refuses a boot config without it: the # kernel keeps the record ring and writes no file, so such a boot's `/log` is @@ -26,6 +27,12 @@ service = true serves = ["netstack"] devices = ["pci:1af4:1041", "pci:8086:10d3"] +[programs.soundserver] +service = true +serves = ["soundserver"] +devices = ["pci:1af4:1059"] +syscap = ["rt"] + # `netstack` because `netstack_socket_churn` connects through it and reads its # counts. [programs.test-runner] diff --git a/tests/testcases/system.toml b/tests/testcases/system.toml index ae1061bf4aa..15dec6b6d09 100644 --- a/tests/testcases/system.toml +++ b/tests/testcases/system.toml @@ -26,7 +26,7 @@ receives = ["power"] [programs.soundserver] service = true serves = ["soundserver"] -devices = ["hda-audio", "virtio-sound"] +devices = ["hda-audio", "pci:1af4:1059"] syscap = ["rt"] # The test estate's authority. diff --git a/tests/toyos-rust-tests/src/bin/virtio_sound_counts.rs b/tests/toyos-rust-tests/src/bin/virtio_sound_counts.rs new file mode 100644 index 00000000000..fc874b34d58 --- /dev/null +++ b/tests/toyos-rust-tests/src/bin/virtio_sound_counts.rs @@ -0,0 +1,132 @@ +//! One stream through soundserver's own virtio-sound driver, judged by what +//! soundserver counted and in what state it left the device — never by how long +//! anything took. +//! +//! soundserver suspends only once every period it submitted has come back from +//! the device, so a stream that ends with soundserver suspended is one whose +//! every submitted period completed; and the periods it submitted cover at +//! least the periods this client filled. + +use std::sync::mpsc; +use std::time::{Duration, Instant}; + +use cpal::traits::{DeviceTrait, HostTrait, StreamTrait}; +use toyos_inspect::{Value, SOUND}; + +/// Periods of tone the client fills: a few laps of the eight-period pipeline. +const PERIODS: u64 = 64; + +/// A hang ceiling on each wait here, none of which is more than a second of +/// audio away. +const WITHIN: Duration = Duration::from_secs(30); + +const FREQ_HZ: f64 = 440.0; +const AMPLITUDE: f64 = 16000.0; + +/// What soundserver's `inspect` answer says of its device and stream. +struct Sound { + device: String, + state: String, + clients: u64, + submitted: u64, + period_frames: u64, + rate: u64, +} + +fn sound() -> Sound { + let answer = inspect::ask(SOUND).unwrap_or_else(|why| panic!("soundserver's inspect answer: {why}")); + let text = |path: &str| match answer.get(path) { + Some(Value::Text(text)) => text.clone(), + other => panic!("soundserver's snapshot has no text at {path}: {other:?}"), + }; + let number = |path: &str| match answer.get(path) { + Some(&Value::U64(n)) => n, + other => panic!("soundserver's snapshot has no number at {path}: {other:?}"), + }; + Sound { + device: text("sound.device"), + state: text("sound.stream.state"), + clients: number("sound.stream.clients"), + submitted: number("sound.periods.submitted"), + period_frames: number("sound.period_frames"), + rate: number("sound.rate_hz"), + } +} + +fn main() { + let before = sound(); + assert_eq!(before.device, "virtio-sound", "soundserver drives no virtio-sound device"); + assert_eq!( + (before.state.as_str(), before.clients), + ("suspended", 0), + "soundserver had a stream before this client's: no count here is this client's alone" + ); + + let filled = play(PERIODS * before.period_frames); + + // soundserver says nothing to a client that left; its answer is where the + // suspend is read, and the device playing its tail out wakes it once a + // period. + let deadline = Instant::now() + WITHIN; + let after = loop { + let now = sound(); + if now.state == "suspended" && now.clients == 0 { + break now; + } + assert!( + Instant::now() < deadline, + "soundserver's stream still reads `{}` with {} client(s) {WITHIN:?} after its only client \ + closed: a submitted period never came back", + now.state, + now.clients + ); + std::thread::sleep(Duration::from_nanos(1_000_000_000 * now.period_frames / now.rate)); + }; + + let submitted = after.submitted - before.submitted; + let covered = filled / before.period_frames; + assert!( + submitted >= covered, + "soundserver submitted {submitted} period(s) for a client that filled {covered}" + ); + println!( + "virtio_sound_counts: {submitted} period(s) submitted for {covered} filled, every one \ + completed before the stream stopped" + ); +} + +/// Play a tone until `frames` have been filled, and close the stream: the +/// frames filled. +fn play(frames: u64) -> u64 { + let host = cpal::default_host(); + let device = host.default_output_device().expect("no audio output device"); + let config = device.default_output_config().expect("no audio config"); + let sample_rate = config.sample_rate() as f64; + let channels = config.channels() as usize; + let (done, filled) = mpsc::channel(); + let mut n: u64 = 0; + let stream = device + .build_output_stream( + config.into(), + move |data: &mut [i16], _: &cpal::OutputCallbackInfo| { + for frame in data.chunks_exact_mut(channels) { + let phase = 2.0 * std::f64::consts::PI * FREQ_HZ * n as f64 / sample_rate; + frame.fill((AMPLITUDE * phase.sin()) as i16); + n += 1; + } + if n >= frames { + // The receiver is gone once one has been taken. + let _ = done.send(n); + } + }, + |err| panic!("the stream reported an error: {err}"), + None, + ) + .expect("failed to build audio stream"); + stream.play().expect("failed to play"); + let filled = filled + .recv_timeout(WITHIN) + .unwrap_or_else(|why| panic!("{frames} frames were not filled within {WITHIN:?}: {why}")); + drop(stream); + filled +} diff --git a/tests/toyos.rs b/tests/toyos.rs index 562ca05ee8a..43c501e985c 100644 --- a/tests/toyos.rs +++ b/tests/toyos.rs @@ -183,6 +183,10 @@ const RUST_SKIP: &[&str] = &[ // It claims QEMU's virtio NIC, which the T14 has none of: `bar_map_again` // runs it. "bar_map_again", + // It plays through soundserver's virtio-sound driver, which the T14 has no + // device for, and its counts are judged beside soundserver's lines: + // `virtio_sound_counts` runs it. + "virtio_sound_counts", ]; /// The shared boot's last members, in this order: each fills a bound of its @@ -342,6 +346,10 @@ const MACHINE_TESTS: &[&str] = &[ // function is its I219, and a kernel that dies on this takes the bench's // machine down with it. "bar_map_again", + // A stream through soundserver's own virtio-sound driver, counted and + // ordered: the device exists only on a QEMU machine — the T14's sound is + // HDA — and its audio goes nowhere, since no guest test plays any. + "virtio_sound_counts", // `console/system.toml`'s image, which runs no job and hands no machine // back: a metal boot of it ends with a hand on the power button. "console_image_boots", @@ -3407,6 +3415,7 @@ fn run_machine_test(name: &str, test_config: &Path) -> Result<(), String> { "acpi_supply_outlives_holder" => acpi_supply_outlives_holder(), "machine_shutdown_short_stop" => power::machine_shutdown_short_stop(test_config), "bar_map_again" => bar_map_again(test_config), + "virtio_sound_counts" => virtio_sound_counts(test_config), "console_image_boots" => console_image_boots(), "nvme_disk_keeps_log_and_home" => nvme_disk_keeps_log_and_home(test_config), other => Err(format!("unknown machine test {other}")), @@ -3561,6 +3570,77 @@ fn bar_map_again(test_config: &Path) -> Result<(), String> { Ok(()) } +/// soundserver brings the virtio-sound function up itself, behind the unit, and +/// plays one stream through it to the end: the job's counts (`inspect`'s), and +/// soundserver's lines in their order — the stream started once after the +/// client connected, and stopped once, before soundserver suspended, with no +/// refusal on the way. +fn virtio_sound_counts(test_config: &Path) -> Result<(), String> { + const JOB: &str = "virtio_sound_counts"; + /// soundserver's bring-up, which it says before any client is accepted. + const BROUGHT_UP: [&str; 3] = [ + "virtio-sound: configured stream 0: 44100Hz 2ch s16le", + "soundserver: ready, 8 buffers, 44100Hz 2ch, 512 bytes/period, 128 frames/period", + "soundserver: suspended", + ]; + /// The stream's lines, in the order they are owed, each once. + const IN_ORDER: [&str; 6] = [ + " connected (id=", + "soundserver: resumed", + "virtio-sound: stream 0 started", + " removed (", + "virtio-sound: stream 0 stopped", + "soundserver: suspended", + ]; + let bin = qemu::build_toyos_bin(qemu::SUITE_ARCH, &compile::repo_root().join("tests/toyos-rust-tests"), JOB); + let mut qemu = + QemuInstance::boot_with_options(test_config, &[], &[(JOB.to_string(), bin)], BootOptions::default()); + let mut console = qemu.boot_log().to_string(); + await_guest(&mut qemu, &mut console, "soundserver's bring-up", |c| BROUGHT_UP.iter().all(|l| c.contains(l))) + .map_err(|e| format!("{e} +{console}"))?; + let boot = serial::Serial::named("the boot", console); + boot.must_be_clean()?; + boot.must_say("soundserver: VirtIO: PCI ")?; + boot.must_say("access_platform=y")?; + boot.must_not_say(audio::NULL_SINK)?; + + let result = qemu.run_test(&format!("test_rs_{JOB}"), Duration::from_secs(120)); + if let Some(why) = &result.error { + return Err(format!("{why} +the job said: +{}", result.stdout)); + } + if result.exit_code != Some(0) { + return Err(format!("the job ended {:?}: +{}", result.exit_code, result.stdout)); + } + let said = &result.stdout; + let Some(counted) = said.lines().find(|l| l.contains("virtio_sound_counts: ")) else { + return Err(format!("the job never said what it counted: +{said}")); + }; + if said.contains("cannot be driven on") { + return Err(format!("soundserver refused its device mid-stream: +{said}")); + } + let mut from = 0; + for line in IN_ORDER { + let times = said.matches(line).count(); + let Some(at) = said[from..].find(line) else { + return Err(format!("`{line}` is not after the line before it in the window: +{said}")); + }; + if times != 1 { + return Err(format!("`{line}` said {times} times in one stream's window: +{said}")); + } + from += at + line.len(); + } + eprintln!(" [sound] {}", counted.trim()); + Ok(()) +} + /// Every device delivery is still cpu0's, and no CPU took a shootdown IPI the /// issuer did not count. /// diff --git a/toyos-abi/src/audio.rs b/toyos-abi/src/audio.rs index 2123cf52cf5..384db8ae4a8 100644 --- a/toyos-abi/src/audio.rs +++ b/toyos-abi/src/audio.rs @@ -4,7 +4,7 @@ use core::sync::atomic::AtomicU32; /// One batch of DMA buffer completions, recorded at interrupt time. /// -/// Reads on a sound device's handle (after the initial info read) return an +/// Reads on the HDA claim's handle (after the initial info read) return an /// array of these: the kernel writes as many pending records as fit in the caller's /// buffer and returns the byte count. `mask` bit N set means period N finished /// playing, and `timestamp_nanos` is `nanos_since_boot` captured in the @@ -12,7 +12,9 @@ use core::sync::atomic::AtomicU32; /// mask is derived there rather than by the driver at wake time. Records are /// returned oldest-first. /// -/// Both stubs produce it, so the two backends differ in nothing a mixer sees. +/// soundserver's virtio-sound driver builds the same record out of its own used +/// ring, stamped when it reads the ring rather than when the message landed: a +/// claim's interrupt record carries a count and no time. #[repr(C)] #[derive(Clone, Copy)] pub struct AudioCompletionRecord { diff --git a/toyos-abi/src/hda.rs b/toyos-abi/src/hda.rs index d71960cc5b5..bfc516ea9dd 100644 --- a/toyos-abi/src/hda.rs +++ b/toyos-abi/src/hda.rs @@ -7,13 +7,8 @@ //! against an allow-list and refused by name. Nothing here names a physical //! address. //! -//! [`RegWidth`](crate::syscall::RegWidth) is those calls' and not this device's, -//! since virtio-sound's stub reaches its notification registers the same way. -//! //! Completions come back as [`AudioCompletionRecord`](crate::audio::AudioCompletionRecord), -//! the same record the virtio-sound stub produces, because the mask is derived -//! from a position read in the interrupt handler and the two backends then -//! differ in nothing a mixer can see. +//! whose mask is derived from a position read in the interrupt handler. //! //! [`syscall::device_reg_read`]: crate::syscall::device_reg_read //! [`syscall::device_reg_write`]: crate::syscall::device_reg_write diff --git a/toyos-abi/src/inventory.rs b/toyos-abi/src/inventory.rs index ec93d304d1b..d819aa2410c 100644 --- a/toyos-abi/src/inventory.rs +++ b/toyos-abi/src/inventory.rs @@ -533,7 +533,7 @@ mod tests { state: PartState::Kernel, }), Record::Claim(Claim { on: Claimed::Pci(at), holder }), - Record::Claim(Claim { on: Claimed::Class(DeviceType::VirtioSound), holder }), + Record::Claim(Claim { on: Claimed::Class(DeviceType::HdaAudio), holder }), Record::Claim(Claim { on: Claimed::Partition { device: 3, unique_guid: [0x11; 16] }, holder, diff --git a/toyos-abi/src/lib.rs b/toyos-abi/src/lib.rs index 469b1c72882..6aa383ad4c9 100644 --- a/toyos-abi/src/lib.rs +++ b/toyos-abi/src/lib.rs @@ -32,7 +32,6 @@ pub mod ring; pub mod syscall; pub mod trace; pub mod usersafe; -pub mod virtio_sound; pub use handle::{RawHandle, Rights, HANDLE_INVALID}; pub use usersafe::UserSafe; diff --git a/toyos-abi/src/syscall.rs b/toyos-abi/src/syscall.rs index 4d7f398da6a..c09675841eb 100644 --- a/toyos-abi/src/syscall.rs +++ b/toyos-abi/src/syscall.rs @@ -1284,11 +1284,6 @@ device_classes! { /// An Intel HDA controller the kernel has brought up but drives no policy /// on. HdaAudio = 5 => "hda-audio", - /// A virtio-sound device, on the same terms: the kernel negotiated its - /// features, built its virtqueues and owns their descriptors, and every - /// decision above that — the stream, the rate, the format, when a period is - /// published — belongs to whoever holds this. - VirtioSound = 6 => "virtio-sound", /// One PCI function, driven by whoever holds the claim: the kernel binds no /// driver to it, keeps config space, puts it in an address space of its own /// before it lets it master the bus, and programs its interrupt vector. diff --git a/toyos-abi/src/virtio_sound.rs b/toyos-abi/src/virtio_sound.rs deleted file mode 100644 index c5dbdcc83a5..00000000000 --- a/toyos-abi/src/virtio_sound.rs +++ /dev/null @@ -1,149 +0,0 @@ -//! What the kernel's virtio-sound stub hands its driver. -//! -//! The line through this device is the one HDA's is: **the kernel writes every -//! address.** A split virtqueue names memory in exactly one place, its -//! descriptor table, and the three tables here live in a page no process maps. -//! What a driver gets is the region those descriptors point into, the two ring -//! indices that select one of them, and one register write to say it has. -//! -//! So the layout below is the whole interface. It is a set of constants rather -//! than fields of [`VirtioSoundInfo`] because both halves have to agree on it -//! for the descriptors to mean anything, and a number the kernel reports is a -//! number a driver could believe instead of the one the descriptors were built -//! from. - -/// The pipeline, in periods and bytes. -pub const PERIODS: usize = 8; -pub const PERIOD_BYTES: usize = 512; - -/// Virtqueue indices, fixed by the virtio sound device specification. -pub const CONTROL_QUEUE: u16 = 0; -pub const EVENT_QUEUE: u16 = 1; -pub const TX_QUEUE: u16 = 2; - -/// Descriptors per queue. Powers of two, as virtio 1.2 §2.7 requires. -/// -/// The TX queue holds one three-descriptor chain per period and nothing else, -/// so its slots are exhausted by [`PERIODS`] and never by a driver's pace. -pub const TX_QUEUE_SIZE: u16 = 32; -pub const CONTROL_QUEUE_SIZE: u16 = 16; -pub const EVENT_QUEUE_SIZE: u16 = 16; - -/// Descriptors in one TX chain: the transfer header, the PCM period, and the -/// status the device writes back. -pub const TX_CHAIN: u16 = 3; - -/// The first descriptor of the chain carrying period `idx`. -/// -/// A function and not a search: the chains are built once, at bind, and the -/// driver publishes one by index. It never writes a descriptor, so there is -/// nothing here for it to get wrong beyond naming the wrong period. -pub const fn tx_chain_head(idx: usize) -> u16 { - idx as u16 * TX_CHAIN -} - -/// How many buffers the event queue keeps posted. -pub const EVENT_BUFS: usize = 8; -/// Stride between them. The event structure is eight bytes; the rest is so a -/// buffer index and a descriptor index are the same number. -pub const EVENT_BUF_STRIDE: usize = 16; - -/// Stride between transfer headers, for the same reason. -pub const XFER_STRIDE: usize = 16; -/// Stride between the per-period status structures the device writes. -pub const STATUS_STRIDE: usize = 8; - -/// How much of a control request or response the descriptors describe. -/// -/// One pair of buffers serves every command, so the length is fixed at bind and -/// has to cover the longest of them. The device reads the header first and takes -/// only what that command defines, so a buffer longer than the message is not a -/// message with garbage after it. -pub const CTRL_BUF_BYTES: usize = 128; - -/// An avail ring's bytes: flags, index, and one descriptor index per slot. -pub const fn avail_bytes(size: u16) -> usize { - 4 + size as usize * 2 -} -/// A used ring's: flags, index, and one `(id, len)` pair per slot. -pub const fn used_bytes(size: u16) -> usize { - 4 + size as usize * 8 -} - -// Byte offsets into the shared region. Every ring here is one the driver -// publishes to or reads from; the TX used ring is absent because the interrupt -// handler is its only consumer, and the descriptor tables are absent because -// they are the addresses. -pub const OFF_PCM: usize = 0x0000; -pub const OFF_TX_AVAIL: usize = 0x1000; -pub const OFF_TX_XFER: usize = 0x1080; -pub const OFF_TX_STATUS: usize = 0x1100; -pub const OFF_CTRL_AVAIL: usize = 0x1200; -pub const OFF_CTRL_USED: usize = 0x1240; -pub const OFF_CTRL_REQ: usize = 0x1300; -pub const OFF_CTRL_RESP: usize = 0x1400; -pub const OFF_EVENT_AVAIL: usize = 0x1500; -pub const OFF_EVENT_USED: usize = 0x1540; -pub const OFF_EVENT_BUFS: usize = 0x1600; -pub const SHARED_BYTES: usize = 0x2000; - -/// Nothing overlaps and everything fits. -/// -/// The device writes four of these regions and the driver writes three, at -/// addresses the kernel computed once — so an overlap is not a bug a test would -/// find, it is one period of PCM landing on a used ring. -const _: () = { - let regions = [ - (OFF_PCM, PERIODS * PERIOD_BYTES), - (OFF_TX_AVAIL, avail_bytes(TX_QUEUE_SIZE)), - (OFF_TX_XFER, PERIODS * XFER_STRIDE), - (OFF_TX_STATUS, PERIODS * STATUS_STRIDE), - (OFF_CTRL_AVAIL, avail_bytes(CONTROL_QUEUE_SIZE)), - (OFF_CTRL_USED, used_bytes(CONTROL_QUEUE_SIZE)), - (OFF_CTRL_REQ, CTRL_BUF_BYTES), - (OFF_CTRL_RESP, CTRL_BUF_BYTES), - (OFF_EVENT_AVAIL, avail_bytes(EVENT_QUEUE_SIZE)), - (OFF_EVENT_USED, used_bytes(EVENT_QUEUE_SIZE)), - (OFF_EVENT_BUFS, EVENT_BUFS * EVENT_BUF_STRIDE), - ]; - let mut i = 0; - while i < regions.len() { - let (start, len) = regions[i]; - assert!(start + len <= SHARED_BYTES); - let mut j = i + 1; - while j < regions.len() { - let (other, other_len) = regions[j]; - assert!(start + len <= other || other + other_len <= start); - j += 1; - } - i += 1; - } - assert!(PERIODS * TX_CHAIN as usize <= TX_QUEUE_SIZE as usize); - assert!(EVENT_BUFS <= EVENT_QUEUE_SIZE as usize); -}; - -crate::user_safe! { - /// The device the kernel brought up, as the driver needs to see it. - /// - /// No physical address and no register window: a shared-memory token, the three - /// offsets inside the notification region that are the driver's whole write - /// surface, and what the device said about itself in its configuration space. - #[derive(Clone, Copy)] - pub struct VirtioSoundInfo { - /// The shared region, mapped writable, laid out by the constants above. - pub dma: crate::RawHandle, - /// Byte offsets into the notification region — the offset space - /// [`device_reg_write`] names for this device, and the only one it has. - /// - /// [`device_reg_write`]: crate::syscall::device_reg_write - pub notify_control: u32, - pub notify_event: u32, - pub notify_tx: u32, - /// The device's configuration space, read once by the kernel, which decided - /// nothing with it. Which stream to open and at what rate is the driver's. - pub jacks: u32, - pub streams: u32, - pub chmaps: u32, - } -} - diff --git a/toyos-inspect/src/dev.rs b/toyos-inspect/src/dev.rs index 3f917e4b085..f16b335221c 100644 --- a/toyos-inspect/src/dev.rs +++ b/toyos-inspect/src/dev.rs @@ -261,7 +261,7 @@ mod tests { Record::Claim(Claim { on: Claimed::Pci(NIC), holder: netstack }), // The same claim through a second handle in the same table. Record::Claim(Claim { on: Claimed::Pci(NIC), holder: netstack }), - Record::Claim(Claim { on: Claimed::Class(DeviceType::VirtioSound), holder: holder(7, "soundserver") }), + Record::Claim(Claim { on: Claimed::Class(DeviceType::HdaAudio), holder: holder(7, "soundserver") }), Record::Claim(Claim { on: Claimed::Partition { device: 1, unique_guid: [0xcd; 16] }, holder: holder(12, "test-runner"), @@ -304,7 +304,7 @@ mod tests { text("dev.disk.0.part1.unique").as_deref(), Some("cdcdcdcd-cdcd-cdcd-cdcd-cdcdcdcdcdcd") ); - assert_eq!(text("dev.class.virtio-sound.holder.7").as_deref(), Some("soundserver")); + assert_eq!(text("dev.class.hda-audio.holder.7").as_deref(), Some("soundserver")); } #[test] diff --git a/toyos-manifest/src/lib.rs b/toyos-manifest/src/lib.rs index 13ffce90806..f956e6afbfe 100644 --- a/toyos-manifest/src/lib.rs +++ b/toyos-manifest/src/lib.rs @@ -554,7 +554,7 @@ mod tests { name: "soundserver".into(), path: "/system/bin/soundserver".into(), serves: vec!["soundserver".into()], - devices: vec!["hda-audio".into(), "virtio-sound".into()], + devices: vec!["hda-audio".into(), "pci:1af4:1059".into()], syscap: vec!["rt".into()], service: true, ..Program::default() diff --git a/toyos-virtio/src/lib.rs b/toyos-virtio/src/lib.rs index ff89774760a..49c91676456 100644 --- a/toyos-virtio/src/lib.rs +++ b/toyos-virtio/src/lib.rs @@ -4,8 +4,11 @@ //! Every `§` in this crate is a section of *Virtual I/O Device (VIRTIO) //! Version 1.2*, OASIS Committee Specification 01. [`pci`] is §4.1 with the //! initialisation §3.1.1 orders and the negotiation §2.2 bounds; [`queue`] is -//! §2.7. A device type — its feature bits, its configuration fields, what its -//! buffers carry — is its driver's and is not here. +//! §2.7. [`pci::vendor_caps`] walks the capability list through a +//! [`pci::ConfigSpace`] the driver's claim answers, under §6.7 of the *PCI +//! Local Bus Specification* 3.0. A device type — its feature bits, its +//! configuration fields, what its buffers carry — is its driver's and is not +//! here. //! //! # The boundary //! @@ -49,8 +52,6 @@ //! §2.7.10.1 permits whatever `used.flags` says — reading it would buy one //! skipped register write for one more device-written word believed. //! - **No packed ring, no indirect descriptors, no legacy interface.** -//! - **No walk of configuration space**: [`pci::Layout::of`] takes the vendor -//! capabilities a driver read through its claim. #![no_std] #![forbid(unsafe_code)] diff --git a/toyos-virtio/src/pci.rs b/toyos-virtio/src/pci.rs index 6c08b11757e..642ec8e0e55 100644 --- a/toyos-virtio/src/pci.rs +++ b/toyos-virtio/src/pci.rs @@ -61,6 +61,10 @@ pub mod status { pub const FAILED: u8 = 128; } +/// §4.1.5.1.2: the vector field value that maps a source to no MSI-X entry, +/// so it raises no interrupt at all. +pub const NO_VECTOR: u16 = 0xFFFF; + /// §6: "This indicates compliance with this specification". pub const VIRTIO_F_VERSION_1: u64 = 1 << 32; /// §6: the device reaches memory through addresses the platform translates, @@ -81,6 +85,96 @@ pub struct VendorCap { pub notify_off_multiplier: u32, } +/// A function's configuration space, as a driver reads it through its claim. +/// +/// Each read is one access of the width its name says, at an offset the walk +/// has already aligned for it, and answers the claim's refusal unchanged. +pub trait ConfigSpace { + type Refused: Copy + core::fmt::Debug + PartialEq + Eq; + fn read8(&self, at: u16) -> Result; + fn read32(&self, at: u16) -> Result; +} + +/// *PCI Local Bus Specification* 3.0 §6.2.3: `Status`, whose bit 4 says the +/// capabilities pointer is valid; §6.7: the pointer, the vendor-specific +/// capability's id, and the first offset past the predefined header, below +/// which no capability is. +const STATUS: u16 = 0x06; +const STATUS_CAPABILITIES: u8 = 1 << 4; +const CAPABILITIES_PTR: u16 = 0x34; +const CAP_ID_VENDOR: u8 = 0x09; +const FIRST_CAP: u8 = 0x40; +/// Every dword-aligned place a capability can be in the 256-byte header: a +/// walk that has taken more links than this has come back round. +const CAP_PLACES: usize = (0x100 - FIRST_CAP as usize) / 4; + +/// Why a capability list was not walked to its end. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum WalkRefusal { + /// The claim refused a read the walk had to make, in its own word. + Read { at: u16, why: E }, + /// A link points into the predefined header (§6.7). + IntoHeader { at: u8 }, + /// The list came back round on itself. + Looped, +} + +impl core::fmt::Display for WalkRefusal { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + match self { + Self::Read { at, why } => { + write!(f, "its configuration space refused a read at {at:#x}: {why:?}") + } + Self::IntoHeader { at } => { + write!(f, "its capability list links to {at:#x}, inside the PCI header") + } + Self::Looped => write!(f, "its capability list never ends"), + } + } +} + +/// The vendor-specific capabilities a function publishes (§4.1.4), in its +/// list's order: what [`Layout::of`] chooses among. +/// +/// Every link is the device's: each is masked of the two bits §6.7 reserves +/// before it is an offset, a link into the header is refused, and so is a +/// list longer than the header has places for. A read the claim refuses ends +/// the walk under that refusal's own word. +pub fn vendor_caps(config: &C) -> Result, WalkRefusal> { + let read8 = |at: u16| config.read8(at).map_err(|why| WalkRefusal::Read { at, why }); + let read32 = |at: u16| config.read32(at).map_err(|why| WalkRefusal::Read { at, why }); + let mut found = Vec::new(); + if read8(STATUS)? & STATUS_CAPABILITIES == 0 { + return Ok(found); + } + let mut next = read8(CAPABILITIES_PTR)? & !3; + let mut taken = 0; + while next != 0 { + if next < FIRST_CAP { + return Err(WalkRefusal::IntoHeader { at: next }); + } + if taken == CAP_PLACES { + return Err(WalkRefusal::Looped); + } + taken += 1; + let at = next as u16; + if read8(at)? == CAP_ID_VENDOR { + let cfg_type = read8(at + 3)?; + found.push(VendorCap { + cfg_type, + bar: read8(at + 4)?, + offset: read32(at + 8)?, + length: read32(at + 12)?, + // §4.1.4.4: present after the notification structure's + // capability and no other's. + notify_off_multiplier: if cfg_type == CFG_NOTIFY { read32(at + 16)? } else { 0 }, + }); + } + next = read8(at + 1)? & !3; + } + Ok(found) +} + /// Which of a device's interrupt sources. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum Source { @@ -278,10 +372,10 @@ impl Offer { /// and nothing more is written to it. pub fn acknowledge(regs: R, layout: &Layout) -> Result { let window = regs.bytes(); - // §4.1.4.3.1 and §4.1.4.4.1: the alignment of each offset. + // §4.1.4.3.1, §4.1.4.4.1 and §4.1.4.6.1: the alignment of each offset. let common = Region::of(&layout.common, "COMMON_CFG", window, common::BYTES, 4)?; let notify = Region::of(&layout.notify, "NOTIFY_CFG", window, 0, 2)?; - let device = Region::of(&layout.device, "DEVICE_CFG", window, 0, 1)?; + let device = Region::of(&layout.device, "DEVICE_CFG", window, 0, 4)?; let mut wires = Wires { regs, common: common.at, @@ -425,6 +519,17 @@ impl Setup { Ok((self, byte)) } + /// The 32-bit field `at` bytes into the device-specific structure, read + /// 32 bits wide as §4.1.3.1 has a 32-bit field read. + pub fn device_read32(mut self, at: usize) -> Result<(Self, u32), Refusal> { + let Region { at: base, bytes } = self.wires.device; + if at.checked_add(4).is_none_or(|end| end > bytes) || !at.is_multiple_of(4) { + return Err(self.wires.refuse(Refusal::PastDeviceConfig { at, bytes })); + } + let word = self.wires.regs.read32(base + at); + Ok((self, word)) + } + /// §3.1.1 step 8: the device is live. pub fn driver_ok(mut self) -> Live { self.wires.set_status(status::DRIVER_OK); diff --git a/toyos-virtio/src/stub.rs b/toyos-virtio/src/stub.rs index a7c0e2f36c7..1a96a506701 100644 --- a/toyos-virtio/src/stub.rs +++ b/toyos-virtio/src/stub.rs @@ -364,9 +364,13 @@ impl State { let value = write.expect("stub: a read of the notification structure"); self.notify(at - NOTIFY_AT as usize, bytes, value); } else if device.contains(&at) { - assert!(write.is_none() && bytes == 1, "stub: the device structure is bytes, read"); - self.trace.push(Event::DeviceConfig { at: at - DEVICE_AT as usize }); - return self.device_config[at - DEVICE_AT as usize] as u32; + assert!(write.is_none(), "stub: the device structure is read and never written"); + let at = at - DEVICE_AT as usize; + assert!(at + bytes <= self.device_config.len(), "stub: a read past the device structure"); + self.trace.push(Event::DeviceConfig { at }); + let mut word = [0u8; 4]; + word[..bytes].copy_from_slice(&self.device_config[at..at + bytes]); + return u32::from_le_bytes(word); } else { panic!("stub: an access at {at:#x}, which is in none of the device's structures"); } diff --git a/toyos-virtio/src/tests.rs b/toyos-virtio/src/tests.rs index 4a426bee9c6..5fc3de419a0 100644 --- a/toyos-virtio/src/tests.rs +++ b/toyos-virtio/src/tests.rs @@ -7,12 +7,12 @@ use std::vec::Vec; use toyos_untrusted::Refused; use crate::pci::{ - status, Layout, Live, Offer, Refusal, Setup, Source, VendorCap, VIRTIO_F_ACCESS_PLATFORM, - VIRTIO_F_VERSION_1, + status, Layout, Live, Offer, Refusal, Setup, Source, VendorCap, WalkRefusal, + VIRTIO_F_ACCESS_PLATFORM, VIRTIO_F_VERSION_1, }; use crate::queue::{desc_bytes, Buffer, Parts, Used, UsedRefusal, Virtqueue}; use crate::stub::{ - Event, Machine, Ring, BAR_BYTES, COMMON_AT, DEVICE_BASE, FEATURE_OFFERED, FEATURE_WITHHELD, + Event, Machine, Ring, BAR_BYTES, COMMON_AT, DEVICE_AT, DEVICE_BASE, FEATURE_OFFERED, FEATURE_WITHHELD, GRANT_BYTES, NOTIFY_AT, NOTIFY_OFF_MULTIPLIER, QUEUES, }; @@ -139,6 +139,9 @@ fn a_structure_its_bar_does_not_hold_is_refused_and_never_reached() { // notification structure's. assert_eq!(refused(1, COMMON_AT + 2, 0x38), Some(Refusal::Misaligned("COMMON_CFG"))); assert_eq!(refused(2, NOTIFY_AT + 1, 0x100), Some(Refusal::Misaligned("NOTIFY_CFG"))); + // §4.1.4.6.1: "The offset for the device-specific configuration MUST be + // 4-byte aligned", which is what a 32-bit field read there rests on. + assert_eq!(refused(4, DEVICE_AT + 2, 6), Some(Refusal::Misaligned("DEVICE_CFG"))); // A structure that ends on the BAR's last byte is inside it. assert_eq!(refused(2, bar - 0x1000, 0x1000), None); } @@ -434,6 +437,163 @@ fn a_field_past_the_device_structure_is_refused_and_not_read() { } } +/// §4.1.3.1: "32-bit wide and aligned accesses for 32-bit … wide fields". A +/// field that is not a whole aligned dword of the structure is refused and not +/// read. +#[test] +fn a_32_bit_field_is_read_32_bits_wide_and_only_inside_the_structure() { + let machine = Machine::new(); + let device = setup(&machine); + machine.forget_trace(); + let (_, word) = device.device_read32(0).unwrap_or_else(|why| panic!("dword 0 is inside: {why}")); + assert_eq!(word, 0x1200_5452); + assert_eq!(machine.trace(), [Event::DeviceConfig { at: 0 }]); + // Past the six bytes, straddling their end, off a dword, and wrapping. + for at in [8, 4, 2, usize::MAX - 1] { + let machine = Machine::new(); + let device = setup(&machine); + machine.forget_trace(); + assert_eq!(device.device_read32(at).err(), Some(Refusal::PastDeviceConfig { at, bytes: 6 })); + assert_eq!(machine.trace(), [Event::Status(11 | status::FAILED)]); + } +} + +// --- PCI 3.0 §6.7: the capability list --- + +/// A function's configuration space: 256 bytes, and the offsets a claim +/// refuses. +struct Config { + bytes: [u8; 256], + refused: Vec, +} + +impl Config { + /// Capabilities `at`, each linked to the next and the last to none, the + /// pointer at the first, and `Status` saying there is a list. + fn listing(at: &[u8]) -> Self { + let mut config = Self { bytes: [0; 256], refused: Vec::new() }; + config.bytes[0x06] = 1 << 4; + config.bytes[0x34] = at.first().copied().unwrap_or(0); + for (nth, &cap) in at.iter().enumerate() { + config.bytes[cap as usize + 1] = at.get(nth + 1).copied().unwrap_or(0); + } + config + } + + /// A vendor capability (§4.1.4) at `at`. + fn vendor(&mut self, at: u8, cfg_type: u8, bar: u8, offset: u32, length: u32, mult: u32) { + let at = at as usize; + self.bytes[at] = 0x09; + self.bytes[at + 3] = cfg_type; + self.bytes[at + 4] = bar; + self.bytes[at + 8..at + 12].copy_from_slice(&offset.to_le_bytes()); + self.bytes[at + 12..at + 16].copy_from_slice(&length.to_le_bytes()); + self.bytes[at + 16..at + 20].copy_from_slice(&mult.to_le_bytes()); + } +} + +/// What a claim answers for a read it refused. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +struct ClaimRefused; + +impl crate::pci::ConfigSpace for Config { + type Refused = ClaimRefused; + + fn read8(&self, at: u16) -> Result { + if self.refused.contains(&at) { + return Err(ClaimRefused); + } + Ok(self.bytes[at as usize]) + } + + fn read32(&self, at: u16) -> Result { + assert_eq!(at % 4, 0, "a 32-bit read at {at:#x}, off its alignment"); + if self.refused.contains(&at) { + return Err(ClaimRefused); + } + let at = at as usize; + Ok(u32::from_le_bytes(self.bytes[at..at + 4].try_into().unwrap())) + } +} + +/// The vendor capabilities in the list's order, any other capability passed +/// over, and the multiplier read where §4.1.4.4 puts one and nowhere else. +#[test] +fn the_walk_answers_every_vendor_capability_in_the_lists_order() { + let mut config = Config::listing(&[0x98, 0x40, 0x70, 0x84]); + config.vendor(0x98, 1, 4, 0x0000, 0x1000, 0); + config.bytes[0x40] = 0x11; // MSI-X: not a vendor's. + config.vendor(0x70, 2, 4, 0x3000, 0x1000, 4); + config.vendor(0x84, 4, 4, 0x2000, 0x1000, 0xDEAD); + let caps = crate::pci::vendor_caps(&config).expect("a list that ends"); + assert_eq!( + caps, + [ + VendorCap { cfg_type: 1, bar: 4, offset: 0, length: 0x1000, notify_off_multiplier: 0 }, + VendorCap { cfg_type: 2, bar: 4, offset: 0x3000, length: 0x1000, notify_off_multiplier: 4 }, + VendorCap { cfg_type: 4, bar: 4, offset: 0x2000, length: 0x1000, notify_off_multiplier: 0 }, + ] + ); +} + +/// §6.2.3: with `Status` bit 4 clear the pointer means nothing, and nothing +/// behind it is read. +#[test] +fn a_function_without_a_capability_list_has_no_vendor_capability() { + let mut config = Config::listing(&[0x40]); + config.vendor(0x40, 1, 4, 0, 0x1000, 0); + config.bytes[0x06] = 0; + config.refused = (0x08..0x100).collect(); + assert_eq!(crate::pci::vendor_caps(&config), Ok(Vec::new())); +} + +/// A read the claim refused is that refusal, at that offset, and never a +/// field of zeros another check refuses under a name of its own. +#[test] +fn a_refused_read_ends_the_walk_by_the_claims_own_word() { + for at in [0x06, 0x34, 0x40, 0x41, 0x43, 0x44, 0x48, 0x4C, 0x50] { + let mut config = Config::listing(&[0x40]); + config.vendor(0x40, 2, 4, 0x3000, 0x1000, 4); + config.refused = Vec::from([at]); + assert_eq!( + crate::pci::vendor_caps(&config), + Err(WalkRefusal::Read { at, why: ClaimRefused }), + "a refusal at {at:#x}" + ); + } +} + +/// §6.7: "the bottom two bits are Reserved and must be set to 00b. Software +/// must mask these bits off before using this register as a pointer", of the +/// pointer and of every link. +#[test] +fn every_link_is_masked_of_its_two_reserved_bits_before_it_is_an_offset() { + let mut config = Config::listing(&[0x40, 0x60]); + config.bytes[0x34] = 0x43; + config.bytes[0x41] = 0x62; + config.vendor(0x40, 1, 4, 0, 0x1000, 0); + config.vendor(0x60, 4, 4, 0x2000, 0x1000, 0); + let caps = crate::pci::vendor_caps(&config).expect("a list that ends"); + assert_eq!(caps.iter().map(|cap| cap.cfg_type).collect::>(), [1, 4]); +} + +/// A link into the header and a list that comes back round are each refused +/// by name, and neither is walked for ever. +#[test] +fn a_link_into_the_header_or_round_the_list_is_refused() { + let mut config = Config::listing(&[0x40]); + config.bytes[0x41] = 0x3C; + assert_eq!(crate::pci::vendor_caps(&config), Err(WalkRefusal::IntoHeader { at: 0x3C })); + + let mut config = Config::listing(&[0x40, 0x50]); + config.bytes[0x51] = 0x40; + assert_eq!(crate::pci::vendor_caps(&config), Err(WalkRefusal::Looped)); + + // The longest list a header holds is no loop: a capability on every dword. + let every: Vec = (0x40..=0xFCu8).step_by(4).collect(); + assert_eq!(crate::pci::vendor_caps(&Config::listing(&every)), Ok(Vec::new())); +} + // --- §2.7: the queue --- /// §4.1.5.1.3 step 4 has the three parts zeroed, and §2.7.10.1 has the driver diff --git a/toyos/src/device.rs b/toyos/src/device.rs index 7c69e4f03be..de9d15b65e5 100644 --- a/toyos/src/device.rs +++ b/toyos/src/device.rs @@ -21,7 +21,6 @@ macro_rules! device_info { device_info!( toyos_abi::FramebufferInfo, toyos_abi::pci::PciFunctionInfo, - toyos_abi::virtio_sound::VirtioSoundInfo, toyos_abi::hda::HdaInfo, toyos_abi::part::PartitionInfo, toyos_abi::acpi::AcpiInfo, @@ -277,51 +276,6 @@ impl AsHandle for PartitionDev { fn as_handle(&self) -> RawHandle { self.0.as_handle() } } -/// A virtio-sound device the kernel brought up and drives no policy on. -/// -/// What the claimant gets is the region the descriptors point into, mapped -/// writable, an interrupt it may wait on, and one register call per queue -/// doorbell. It gets no descriptor table and no physical address, so there is -/// nothing here that can point the device at memory. -pub struct VirtioSoundDev(pub(crate) Device); - -impl VirtioSoundDev { - pub fn info(&self) -> Result { - read_info(&self.0) - } - - /// Drain all pending completion records in one nonblocking read. - /// - /// Returns the number of records written to `records`. The kernel ring - /// holds at most 16 records, so a 16-entry buffer always drains fully. - /// Empty ring surfaces as `Err(WouldBlock)`. - pub fn read_completions( - &self, - records: &mut [toyos_abi::audio::AudioCompletionRecord], - ) -> Result { - const REC_SIZE: usize = toyos_abi::audio::AudioCompletionRecord::SIZE; - let buf = unsafe { - core::slice::from_raw_parts_mut( - records.as_mut_ptr() as *mut u8, - records.len() * REC_SIZE, - ) - }; - let n = syscall::read_nonblock(self.0.0.0, buf)?; - assert_eq!(n % REC_SIZE, 0, "partial audio completion record ({n} bytes)"); - Ok(n / REC_SIZE) - } - - /// Ring one queue's doorbell. `offset` is one of the three the info struct - /// reports and nothing else is on the kernel's allow-list. - pub fn notify(&self, offset: u32, queue: u16) -> Result<(), SyscallError> { - syscall::device_reg_write(self.0.as_handle(), offset, syscall::RegWidth::U16, queue as u32) - } -} - -impl AsHandle for VirtioSoundDev { - fn as_handle(&self) -> RawHandle { self.0.as_handle() } -} - /// An Intel HDA controller the kernel brought up and drives no policy on. /// /// What the claimant gets is a PCM ring it may write, an interrupt it may wait diff --git a/toyos/src/endow.rs b/toyos/src/endow.rs index d4d9b97df79..4927ba31de1 100644 --- a/toyos/src/endow.rs +++ b/toyos/src/endow.rs @@ -77,7 +77,6 @@ from_handle! { crate::AcpiDev => |h| crate::AcpiDev(Device(h)), crate::PartitionDev => |h| crate::PartitionDev(Device(h)), crate::HdaDev => |h| crate::HdaDev(Device(h)), - crate::VirtioSoundDev => |h| crate::VirtioSoundDev(Device(h)), } /// This process's endowment table, parsed once. @@ -224,9 +223,7 @@ pub fn provided(labels: &mut [Option<(&'static str, Connector)>]) -> usize { /// /// `None` is a machine that had no such device when the supervisor asked, or a program /// the manifest gives none — the honest answer, and the one soundserver degrades -/// on. It replaces a two-syscall probe: "did I get an HDA or a virtio-sound?" -/// is now "which claims are in my endowment table?", which is the same question -/// with the answer already in hand. +/// on. pub fn device(class: DeviceType) -> Option { with_prefixed(DEV_PREFIX, class.class_name(), |label| Endowments::get().take::(label)) } diff --git a/toyos/src/lib.rs b/toyos/src/lib.rs index 5b7e3e30012..89d6799526d 100644 --- a/toyos/src/lib.rs +++ b/toyos/src/lib.rs @@ -35,7 +35,7 @@ pub mod volatile; pub use ipc::Connection; pub use device::{ - AcpiDev, DmaRegion, FramebufferDev, HdaDev, Keyboard, Mouse, PartitionDev, PciDev, VirtioSoundDev, + AcpiDev, DmaRegion, FramebufferDev, HdaDev, Keyboard, Mouse, PartitionDev, PciDev, }; pub use toyos_abi::RawHandle; diff --git a/userland/netstack/src/device.rs b/userland/netstack/src/device.rs index 74fe0aa6092..e9cd8a45230 100644 --- a/userland/netstack/src/device.rs +++ b/userland/netstack/src/device.rs @@ -1,5 +1,6 @@ //! What both NIC drivers need from the substrate: `toyos-device-memory`'s -//! boundary over the SDK's `toyos::volatile::Window`, the kernel's word for a +//! boundary over the SDK's `toyos::volatile::Window`, the claim's +//! configuration space as `toyos-virtio` walks it, the kernel's word for a //! call a bring-up cannot go on without, and the latch a diagnostic is printed //! on. //! @@ -14,8 +15,10 @@ use std::cell::Cell; use std::sync::atomic::{fence, Ordering}; use toyos::volatile::Window; -use toyos_abi::syscall::SyscallError; +use toyos::PciDev; +use toyos_abi::syscall::{RegWidth, SyscallError}; use toyos_device_memory::{DmaBuffers, Registers}; +use toyos_virtio::pci::ConfigSpace; /// A mapped BAR, as a driver reaches its device's registers. #[derive(Clone, Copy)] @@ -120,6 +123,23 @@ impl DmaBuffers for Grant { } } +/// A claim's configuration space, as `toyos-virtio`'s capability walk reads +/// it: each read one `config_read` of its width, and its refusal the kernel's +/// word. +pub struct ClaimConfig<'a>(pub &'a PciDev); + +impl ConfigSpace for ClaimConfig<'_> { + type Refused = SyscallError; + + fn read8(&self, at: u16) -> Result { + self.0.config_read(at as u32, RegWidth::U8).map(|byte| byte as u8) + } + + fn read32(&self, at: u16) -> Result { + self.0.config_read(at as u32, RegWidth::U32) + } +} + /// The kernel refused a call a bring-up cannot go on without, and the word is /// the kernel's own. #[derive(Clone, Copy, PartialEq, Eq, Debug)] diff --git a/userland/netstack/src/virtio_net.rs b/userland/netstack/src/virtio_net.rs index 752063ca830..4656807099f 100644 --- a/userland/netstack/src/virtio_net.rs +++ b/userland/netstack/src/virtio_net.rs @@ -28,20 +28,10 @@ use toyos::volatile::Window; use toyos::{DmaRegion, PciDev}; use toyos_abi::syscall::{RegWidth, SyscallError}; use toyos_device_memory::DmaBuffers; -use toyos_virtio::pci::{Layout, Live, Offer, VendorCap}; +use toyos_virtio::pci::{vendor_caps, Layout, Live, Offer, WalkRefusal}; use toyos_virtio::queue::{avail_bytes, desc_bytes, Buffer, Parts, Published, Used, Virtqueue}; -use crate::device::{Bar, Grant, KernelRefused}; - -/// PCI's own vendor-specific capability id; virtio's config structures are all -/// published under it (§4.1.4). -const CAP_ID_VENDOR: u8 = 0x09; - -/// Where the capability list starts, and how far a walk may follow it. The -/// pointer is the *device's*, so a malformed or cyclic chain ends the walk -/// rather than running for ever. -const CAPABILITIES_PTR: u32 = 0x34; -const MAX_CAPABILITIES: usize = 48; +use crate::device::{Bar, ClaimConfig, Grant, KernelRefused}; /// §5.1.3: the device has a MAC address of its own to read. const VIRTIO_NET_F_MAC: u64 = 1 << 5; @@ -99,6 +89,8 @@ pub enum Refusal { /// What the device published or answered, as the transport refused it. Device(toyos_virtio::pci::Refusal), Kernel(KernelRefused), + /// The capability list, as the walk refused it. + Walk(WalkRefusal), /// The claim answered a configuration read it had to refuse. Unbounded(&'static str, u32), } @@ -114,6 +106,7 @@ impl std::fmt::Display for Refusal { match self { Self::Device(why) => write!(f, "{why}"), Self::Kernel(why) => write!(f, "{why}"), + Self::Walk(why) => write!(f, "{why}"), Self::Unbounded(what, at) => write!( f, "the claim answered a {what} at {at:#x}, so it is not a claim on one \ @@ -137,7 +130,8 @@ fn finished(rings: &mut Virtqueue) -> Option { /// The bound the capability walk rests on, asked once before the walk. /// -/// **The walk below indexes configuration space by numbers the *device* wrote** +/// **`toyos_virtio::pci::vendor_caps` indexes configuration space by numbers +/// the *device* wrote** /// — the capability pointer and every `next` link in the chain — and it is safe /// 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 @@ -187,7 +181,7 @@ impl VirtioNet { let info = dev.describe().map_err(KernelRefused::on("the claim's description")).map_err(Refusal::Kernel)?; config_space_is_bounded(&dev)?; - let layout = Layout::of(&vendor_caps(&dev))?; + let layout = Layout::of(&vendor_caps(&ClaimConfig(&dev)).map_err(Refusal::Walk)?)?; // The kernel hands out a BAR at a time, and reports 0 bytes for one it // keeps back. let bar = layout.bar(); @@ -417,37 +411,6 @@ impl TxQueue { } } -/// The vendor capabilities a function published, in its list's order, walked -/// once. -fn vendor_caps(dev: &PciDev) -> Vec { - let mut found = Vec::new(); - let mut seen = 0usize; - let Ok(first) = dev.config_read(CAPABILITIES_PTR, RegWidth::U8) else { - return found; - }; - let mut next = first; - // The pointer is the device's: a chain that does not terminate, or one - // pointing outside the header, ends the walk rather than running off - // the window or for ever. - while next >= 0x40 && next < 0x100 && seen < MAX_CAPABILITIES { - seen += 1; - let Ok(id) = dev.config_read(next, RegWidth::U8) else { break }; - if id as u8 == CAP_ID_VENDOR { - let read = |at: u32, width| dev.config_read(next + at, width).unwrap_or(0); - found.push(VendorCap { - cfg_type: read(3, RegWidth::U8) as u8, - bar: read(4, RegWidth::U8) as u8, - offset: read(8, RegWidth::U32), - length: read(12, RegWidth::U32), - notify_off_multiplier: read(16, RegWidth::U32), - }); - } - let Ok(link) = dev.config_read(next + 1, RegWidth::U8) else { break }; - next = link; - } - found -} - /// The transmit queue's room, over a plain allocation for the grant. What a /// used-ring element must satisfy is `toyos-virtio`'s, and tested there. #[cfg(test)] diff --git a/userland/soundserver/Cargo.toml b/userland/soundserver/Cargo.toml index 13fc21e9b8c..66a4d7b6999 100644 --- a/userland/soundserver/Cargo.toml +++ b/userland/soundserver/Cargo.toml @@ -9,6 +9,9 @@ toyos-abi = { path = "../../toyos-abi" } toyos = { path = "../../toyos" } toyos-hda = { path = "../../toyos-hda" } toyos-mixer = { path = "mixer" } +toyos-virtio-sound = { path = "virtio-sound" } +toyos-virtio = { path = "../../toyos-virtio" } +toyos-device-memory = { path = "../../toyos-device-memory" } toyos-inspect = { path = "../../toyos-inspect" } rubato = "0.16" diff --git a/userland/soundserver/src/backend.rs b/userland/soundserver/src/backend.rs index fe2a4f0ecc5..39daf3a1259 100644 --- a/userland/soundserver/src/backend.rs +++ b/userland/soundserver/src/backend.rs @@ -96,10 +96,6 @@ impl Backend for VirtioBackend { } fn completions(&mut self, out: &mut [AudioCompletionRecord]) -> usize { - // Where the kernel used to service the event queue inside the same - // syscall: the device's own view of an underrun, which this process's - // counters cannot see. - self.virtio.poll_events(); self.virtio.completions(out) } diff --git a/userland/soundserver/src/main.rs b/userland/soundserver/src/main.rs index d73ebebdbc3..e47bca86fb6 100644 --- a/userland/soundserver/src/main.rs +++ b/userland/soundserver/src/main.rs @@ -22,7 +22,7 @@ use toyos::endow; use toyos::port::Acceptor; use toyos::shm::SharedMemory; -use toyos::{HdaDev, VirtioSoundDev}; +use toyos::{HdaDev, PciDev}; use toyos_abi::syscall::{self, DeviceType}; use toyos_mixer::{period_frames, ramp_frames}; @@ -83,7 +83,7 @@ fn main() { // // The order is virtio first, and it is not a preference between two cards: // no machine in this project has both. The T14 has only the second. - if let Some(dev) = endow::device::(DeviceType::VirtioSound) { + if let Some(dev) = endow::pci_function::(virtio::PCI_ID) { match virtio::Virtio::claim(dev) { Ok((virtio, rate, channels)) => return run_virtio(acceptor, virtio, rate, channels), Err(why) => { @@ -109,10 +109,10 @@ fn run_virtio(acceptor: Acceptor, virtio: virtio::Virtio, rate: u32, channels: u acceptor, "virtio-sound", &mut VirtioBackend { virtio }, - toyos_abi::virtio_sound::PERIODS, + toyos_virtio_sound::PERIODS, rate, channels as u16, - toyos_abi::virtio_sound::PERIOD_BYTES, + toyos_virtio_sound::PERIOD_BYTES, ); } diff --git a/userland/soundserver/src/virtio.rs b/userland/soundserver/src/virtio.rs index 21348a03513..7f5d46c56ab 100644 --- a/userland/soundserver/src/virtio.rs +++ b/userland/soundserver/src/virtio.rs @@ -1,523 +1,293 @@ -//! soundserver as the driver of a virtio-sound device. +//! soundserver as the driver of a virtio-sound function it holds as a PCI +//! claim. //! -//! The kernel negotiated the device's features, built its virtqueues and owns -//! their descriptors, and answers one register write per queue doorbell. -//! Everything that is a *decision* is here — which stream, at what rate and -//! format, when a period goes out and when the stream runs — and every one of -//! them is a message this process writes into a buffer of its own and publishes -//! by index. +//! What the kernel keeps is the claim: config space, the vector it programmed +//! into the function's MSI-X table, and the address space the function +//! translates through. **The transport and the queues are `toyos-virtio`'s, +//! and the sound device's messages and queues `toyos-virtio-sound`'s**, where +//! every word the device writes back is bounded and every refusal is +//! host-tested; this file is the instructions under them — the register window +//! and the grant as `toyos-device-memory`'s boundary — and what becomes of a +//! refusal. //! -//! There is no descriptor here and no physical address. The chains were built -//! once, at bind, out of offsets into the region below; what this file writes -//! into an avail ring is which of them to run. +//! **Only the transmit queue raises an interrupt.** The control queue is polled +//! for the one answer it owes, and the event queue is read when a period comes +//! back, so the claim is readable exactly when a period has played. //! -//! Structure layouts and command codes are VirtIO 1.2 §5.14. +//! **A refusal after bring-up ends soundserver** by its own name, as netstack's +//! does: no conforming device writes one. -use core::ptr::{read_volatile, write_volatile}; -use core::sync::atomic::{fence, Ordering}; +use std::sync::atomic::{fence, Ordering}; use toyos::shm::SharedMemory; -use toyos::VirtioSoundDev; +use toyos::volatile::Window; +use toyos::{DmaRegion, PciDev}; use toyos_abi::audio::AudioCompletionRecord; -use toyos_abi::syscall::SyscallError; -use toyos_abi::virtio_sound as abi; +use toyos_abi::syscall::{RegWidth, SyscallError}; +use toyos_device_memory::{DmaBuffers, Registers}; +use toyos_virtio::pci::{vendor_caps, ConfigSpace, Layout, Live, Offer, WalkRefusal, NO_VECTOR, + VIRTIO_F_ACCESS_PLATFORM}; +use toyos_virtio_sound::{Queues, Sound, PERIOD_BYTES, STREAM_ID}; -const R_PCM_INFO: u32 = 0x0100; -const R_PCM_SET_PARAMS: u32 = 0x0101; -const R_PCM_PREPARE: u32 = 0x0102; -const R_PCM_START: u32 = 0x0104; -const R_PCM_STOP: u32 = 0x0105; +/// virtio's vendor id and the sound device's (§4.1.2.1: `0x1040` + 25), as +/// the manifest row spells the claim. +pub const PCI_ID: toyos_abi::syscall::PciId = toyos_abi::syscall::PciId { vendor: 0x1af4, device: 0x1059 }; -const EVT_JACK_CONNECTED: u32 = 0x1000; -const EVT_JACK_DISCONNECTED: u32 = 0x1001; -const EVT_PCM_PERIOD_ELAPSED: u32 = 0x1100; -const EVT_PCM_XRUN: u32 = 0x1101; +/// The one MSI-X table entry the kernel programs. +const MSIX_ENTRY: u16 = 0; -const S_OK: u32 = 0x8000; +/// §5.14.4: `streams`, the second `le32` of the configuration. +const CONFIG_STREAMS: usize = 4; -const FMT_S16: u8 = 5; -const RATE_44100: u8 = 6; -const RATE_48000: u8 = 7; - -/// The one stream this driver opens. A device with several is a decision this -/// file has not been asked to make, and taking the first of them is the blind -/// choice forbidden one layer down — but virtio-sound numbers its streams and reports -/// only how many, so there is nothing here to choose *by*. -const STREAM_ID: u32 = 0; - -/// The rates this driver can encode, best first. 44100 leads because it is what -/// the mixer, the resampler and the gate's recorded counters are sized against; -/// 48000 is the one every other device offers. -const SUPPORTED_RATES: [(u32, u8); 2] = [(44100, RATE_44100), (48000, RATE_48000)]; - -/// How many times a control command's completion is polled before the device is -/// called dead. -/// -/// **A count and not a deadline, and the difference is the whole of it.** A wall -/// clock keeps running while this guest is not: under host load a vCPU can lose -/// tens of milliseconds without executing an instruction, so a duration here -/// would measure the host's scheduler and report a healthy device as a stopped -/// one — at a suspend boundary, in the audio path. A count advances only when -/// this driver actually looked. -/// -/// Policy, not physics: the specification has no number. What it is set against -/// is that a device answering at all answers in one round trip, so anything this -/// side of enormous separates "slow" from "gone". -const CTRL_POLLS: u32 = 100_000; -/// Spins between two looks at the used ring. -const SPINS_PER_POLL: u32 = 256; - -#[repr(C)] -#[derive(Clone, Copy)] -struct Hdr { - code: u32, -} - -#[repr(C)] -#[derive(Clone, Copy)] -struct QueryInfo { - hdr: Hdr, - start_id: u32, - count: u32, - size: u32, -} - -#[repr(C)] -#[derive(Clone, Copy)] -struct PcmInfo { - /// The info header the device leads every entry with: its HDA function node. - hdr: u32, - features: u32, - formats: u64, - rates: u64, - direction: u8, - channels_min: u8, - channels_max: u8, - _padding: [u8; 5], -} - -#[repr(C)] +/// A mapped BAR, as the transport reaches the device's registers. #[derive(Clone, Copy)] -struct PcmSetParams { - hdr: Hdr, - stream_id: u32, - buffer_bytes: u32, - period_bytes: u32, - features: u32, - channels: u8, - format: u8, - rate: u8, - _padding: u8, -} +pub struct Bar(Window); -#[repr(C)] -#[derive(Clone, Copy)] -struct PcmHdr { - hdr: Hdr, - stream_id: u32, +impl Registers for Bar { + fn bytes(&self) -> usize { + self.0.bytes() + } + fn read8(&self, at: usize) -> u8 { + self.0.read(at) + } + fn read16(&self, at: usize) -> u16 { + self.0.read(at) + } + fn read32(&self, at: usize) -> u32 { + self.0.read(at) + } + fn write8(&self, at: usize, value: u8) { + self.0.write(at, value); + } + fn write16(&self, at: usize, value: u16) { + self.0.write(at, value); + } + fn write32(&self, at: usize, value: u32) { + self.0.write(at, value); + } } -#[repr(C)] +/// The DMA grant, as the queues reach it. `device_base` is what the unit +/// translates for this function and for nothing else. #[derive(Clone, Copy)] -struct PcmXfer { - stream_id: u32, +pub struct Grant { + window: Window, + device_base: u64, } -#[repr(C)] -#[derive(Clone, Copy)] -struct Event { - code: u32, - data: u32, +impl DmaBuffers for Grant { + fn bytes(&self) -> usize { + self.window.bytes() + } + fn device_addr(&self, at: usize) -> u64 { + self.device_base + at as u64 + } + fn read16(&self, at: usize) -> u16 { + self.window.read(at) + } + fn read32(&self, at: usize) -> u32 { + self.window.read(at) + } + fn read64(&self, at: usize) -> u64 { + self.window.read(at) + } + fn write16(&self, at: usize, value: u16) { + self.window.write(at, value); + } + fn write32(&self, at: usize, value: u32) { + self.window.write(at, value); + } + fn write64(&self, at: usize, value: u64) { + self.window.write(at, value); + } + fn publish(&self) { + fence(Ordering::Release); + } + fn observe(&self) { + fence(Ordering::Acquire); + } } -/// The control buffers the kernel described have to hold every message this file -/// sends or reads back, and the descriptor lengths are fixed at bind — so this -/// is the one place the two halves could disagree about a size. -const _: () = { - assert!(core::mem::size_of::() <= abi::CTRL_BUF_BYTES); - assert!(core::mem::size_of::() <= abi::CTRL_BUF_BYTES); - assert!(core::mem::size_of::() <= abi::CTRL_BUF_BYTES); - assert!( - core::mem::size_of::() + core::mem::size_of::() <= abi::CTRL_BUF_BYTES - ); -}; +/// The claim's configuration space, as the capability walk reads it. +struct ClaimConfig<'a>(&'a PciDev); -// Avail: flags(u16) idx(u16) ring[size](u16). Used: flags(u16) idx(u16) -// ring[size](id:u32 len:u32). -const AVAIL_IDX: usize = 2; -const AVAIL_RING: usize = 4; -const USED_IDX: usize = 2; -const USED_RING: usize = 4; -const USED_ELEM: usize = 8; +impl ConfigSpace for ClaimConfig<'_> { + type Refused = SyscallError; -/// The driver's half of a virtqueue: an index to publish, and a doorbell to ring -/// after publishing it. -/// -/// No descriptor table, which is the whole point — the chains are the kernel's -/// and this names one by its head. -struct Avail { - ring: *mut u8, - size: u16, - doorbell: u32, - queue: u16, -} - -impl Avail { - /// Make `head`'s chain available to the device and ring the doorbell. - fn publish(&self, dev: &VirtioSoundDev, head: u16) { - unsafe { - let idx_ptr = self.ring.add(AVAIL_IDX) as *mut u16; - let idx = read_volatile(idx_ptr); - write_volatile( - self.ring.add(AVAIL_RING + (idx % self.size) as usize * 2) as *mut u16, - head, - ); - // The chain has to be in the ring before the index covers it. - fence(Ordering::Release); - write_volatile(idx_ptr, idx.wrapping_add(1)); - fence(Ordering::Release); - } - dev.notify(self.doorbell, self.queue).unwrap_or_else(|e| { - panic!("soundserver: virtio-sound refused queue {}'s doorbell: {e}", self.queue) - }); + fn read8(&self, at: u16) -> Result { + self.0.config_read(at as u32, RegWidth::U8).map(|byte| byte as u8) } -} - -/// A used ring this process consumes. -/// -/// The TX queue has no such thing here and that is the design: its consumer is -/// the interrupt handler, which timestamps the completion, and a ring this -/// process could rewrite would be a mask the kernel derived from userland. -struct Used { - ring: *mut u8, - size: u16, - last: u16, -} -impl Used { - fn poll(&mut self) -> Option { - unsafe { - let used_idx = read_volatile(self.ring.add(USED_IDX) as *const u16); - if used_idx == self.last { - return None; - } - // The device wrote the element before bumping its index. - fence(Ordering::Acquire); - let slot = (self.last % self.size) as usize; - let id = read_volatile(self.ring.add(USED_RING + slot * USED_ELEM) as *const u32); - self.last = self.last.wrapping_add(1); - Some(id as u16) - } + fn read32(&self, at: u16) -> Result { + self.0.config_read(at as u32, RegWidth::U32) } } -/// Why this machine's virtio-sound device cannot carry audio. Each is a line -/// soundserver prints before falling back to the null sink — "no sound" without which -/// of these it was is a report nobody can act on. +/// Why this machine's virtio-sound function cannot carry audio. Each is a line +/// soundserver prints before it falls back to the null sink. pub enum Refusal { - /// The kernel's answers stopped making sense, which is a bug here or there - /// and never a property of the machine. - Kernel(SyscallError), - /// The device did not answer a control command inside its deadline. - Silent(&'static str), - /// It answered, and said no. - Rejected(&'static str, u32), - /// It offers nothing this driver implements. Already named on its own line, - /// with the bitmap it was read from. - NoFormat, + /// The kernel refused a call a bring-up cannot go on without. + Kernel(&'static str, SyscallError), + Walk(WalkRefusal), + Transport(toyos_virtio::pci::Refusal), + Sound(toyos_virtio_sound::Refusal), } impl core::fmt::Display for Refusal { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { match self { - Self::Kernel(e) => write!(f, "the kernel refused a call this driver has to make ({e})"), - Self::Silent(what) => write!(f, "the device never answered {what}"), - Self::Rejected(what, status) => write!(f, "the device refused {what} ({status:#x})"), - Self::NoFormat => write!(f, "the device offers no format or rate this driver mixes"), + Self::Kernel(call, why) => write!(f, "the kernel refused {call}: {why:?}"), + Self::Walk(why) => write!(f, "{why}"), + Self::Transport(why) => write!(f, "{why}"), + Self::Sound(why) => write!(f, "{why}"), } } } -pub struct Virtio { - dev: VirtioSoundDev, - /// The region every ring and buffer below points into. Held so the mapping - /// outlives the pointers taken out of it. - _shm: SharedMemory, - base: *mut u8, - control: Avail, - control_done: Used, - events: Avail, - events_done: Used, - tx: Avail, - running: bool, +impl From for Refusal { + fn from(why: toyos_virtio::pci::Refusal) -> Self { + Self::Transport(why) + } } -impl Virtio { - /// Ask what the device's stream can do, and configure it. - /// - /// The claim is the argument: `/system/bin/supervisor` minted it and endowed it, so - /// "does this machine have a virtio-sound?" was already answered before - /// soundserver's first instruction. - pub fn claim(dev: VirtioSoundDev) -> Result<(Self, u32, u8), Refusal> { - let info = dev.info().map_err(Refusal::Kernel)?; - let shm = SharedMemory::adopt(info.dma, 2 * 1024 * 1024) - .map_err(Refusal::Kernel)?; - let base = shm.as_ptr(); +fn kernel(call: &'static str) -> impl Fn(SyscallError) -> Refusal { + move |why| Refusal::Kernel(call, why) +} - let avail = |offset: usize, size: u16, doorbell: u32, queue: u16| Avail { - ring: unsafe { base.add(offset) }, - size, - doorbell, - queue, - }; - let used = |offset: usize, size: u16| Used { - ring: unsafe { base.add(offset) }, - size, - last: 0, - }; - let mut virtio = Virtio { - dev, - _shm: shm, - base, - control: avail( - abi::OFF_CTRL_AVAIL, - abi::CONTROL_QUEUE_SIZE, - info.notify_control, - abi::CONTROL_QUEUE, - ), - control_done: used(abi::OFF_CTRL_USED, abi::CONTROL_QUEUE_SIZE), - events: avail( - abi::OFF_EVENT_AVAIL, - abi::EVENT_QUEUE_SIZE, - info.notify_event, - abi::EVENT_QUEUE, - ), - events_done: used(abi::OFF_EVENT_USED, abi::EVENT_QUEUE_SIZE), - tx: avail(abi::OFF_TX_AVAIL, abi::TX_QUEUE_SIZE, info.notify_tx, abi::TX_QUEUE), - running: false, - }; +pub struct Virtio { + dev: PciDev, + sound: Sound>, + grant: Grant, + /// Held for their mappings' lives: the register window points into the + /// first and the grant into the second. + _bar: SharedMemory, + _region: DmaRegion, +} - for i in 0..abi::EVENT_BUFS { - virtio.events.publish(&virtio.dev, i as u16); - } +impl Virtio { + /// Bring the function up in the order `toyos-virtio`'s types fix, which is + /// §3.1.1's, and stream 0 in §5.14.5's: the rate and channel count chosen. + pub fn claim(dev: PciDev) -> Result<(Self, u32, u8), Refusal> { + let info = dev.describe().map_err(kernel("the claim's description"))?; + let layout = Layout::of(&vendor_caps(&ClaimConfig(&dev)).map_err(Refusal::Walk)?)?; + // The kernel reports 0 bytes for a BAR it keeps back. + let bar = layout.bar(); + let bar_bytes = *info + .bar_bytes + .get(bar as usize) + .filter(|bytes| **bytes > 0) + .ok_or(toyos_virtio::pci::Refusal::MissingCap("a BAR this claim may map"))?; + let mapped = dev.map_bar(bar as u32, bar_bytes).map_err(kernel("the register window"))?; + // SAFETY: the mapping is `bar_bytes` long and lives as long as + // `mapped`, which `Virtio` holds for its own life. + let window = unsafe { Window::new(mapped.as_ptr(), bar_bytes as usize) }; + + let offer = Offer::acknowledge(Bar(window), &layout)?; + let offered = offer.features(); + // §5.14.3: the sound device defines no feature bit. + let setup = offer.accept(0)?; + let features = setup.features(); + // The kernel's virtio drivers' feature line, in the same shape: + // `iommu_virtio_platform` reads it back for every virtio function. + say!( + "soundserver: VirtIO: PCI {:02x}:{:02x}.{} features device={offered:#x} \ + negotiated={features:#x} access_platform={}", + info.bus, + info.dev, + info.func, + if features & VIRTIO_F_ACCESS_PLATFORM != 0 { 'y' } else { 'n' }, + ); - let pcm = virtio.pcm_info()?; + let region = dev + .dma_alloc(toyos_virtio_sound::GRANT_BYTES as u64) + .map_err(kernel("a DMA grant"))?; + // SAFETY: the kernel rounds the request up to whole pages, never down, + // and the grant lives as long as `region`, which `Virtio` holds. + let window = unsafe { Window::new(region.memory.as_ptr(), toyos_virtio_sound::GRANT_BYTES) }; + window.zero(); + let grant = Grant { window, device_base: region.device_addr }; + + let queues = Queues::new(grant); + let (setup, streams) = setup + .config_vector(NO_VECTOR)? + .enable(&queues.control, NO_VECTOR)? + .enable(&queues.events, NO_VECTOR)? + .enable(&queues.tx, MSIX_ENTRY)? + .device_read32(CONFIG_STREAMS)?; + say!("virtio-sound: {streams} stream(s)"); + let (sound, stream) = + Sound::open(queues, grant, setup.driver_ok(), streams).map_err(Refusal::Sound)?; + let pcm = stream.info; say!( "virtio-sound: stream {STREAM_ID}: dir={} ch={}-{} fmts={:#x} rates={:#x}", pcm.direction, pcm.channels_min, pcm.channels_max, pcm.formats, pcm.rates ); - let (rate, channels) = choose_params(&pcm).ok_or(Refusal::NoFormat)?; - virtio.configure(rate, channels)?; - Ok((virtio, rate, channels)) + say!( + "virtio-sound: configured stream {STREAM_ID}: {}Hz {}ch s16le", + stream.rate, stream.channels + ); + let virtio = Self { dev, sound, grant, _bar: mapped, _region: region }; + Ok((virtio, stream.rate, stream.channels)) } - pub fn dev(&self) -> &VirtioSoundDev { + pub fn dev(&self) -> &PciDev { &self.dev } pub fn buffer(&self, idx: usize) -> *mut u8 { - unsafe { self.base.add(abi::OFF_PCM + idx * abi::PERIOD_BYTES) } + self.grant.window.sub(Sound::>::period(idx), PERIOD_BYTES).as_ptr() } - pub fn completions(&self, out: &mut [AudioCompletionRecord]) -> usize { - match self.dev.read_completions(out) { - Ok(n) => n, - Err(SyscallError::WouldBlock) => 0, - Err(e) => panic!("soundserver: read_completions failed: {e:?}"), - } - } - - /// Put period `idx` on the wire. + /// The periods played since the last call, as one record stamped now. /// - /// The chain is already built and its PCM descriptor already names this - /// buffer, so what a period costs is a store of the stream id, a store into - /// the avail ring, and one doorbell — the same one syscall the deleted - /// `SYS_AUDIO_SUBMIT` cost. - pub fn submit(&mut self, idx: usize, bytes: usize) { - assert_eq!( - bytes, - abi::PERIOD_BYTES, - "soundserver: the TX chain's PCM descriptor is a whole period and the kernel built it" - ); - self.start(); - unsafe { - write_volatile( - self.base.add(abi::OFF_TX_XFER + idx * abi::XFER_STRIDE) as *mut PcmXfer, - PcmXfer { stream_id: STREAM_ID }, - ); + /// The claim's record is taken before the ring is read, so a period that + /// lands after the read leaves the claim readable for the next wake. + pub fn completions(&mut self, out: &mut [AudioCompletionRecord]) -> usize { + match self.dev.irq() { + Ok(_) | Err(SyscallError::WouldBlock) => {} + Err(why) => panic!("soundserver: the virtio-sound claim's interrupt record: {why:?}"), + } + let timestamp_nanos = toyos_abi::clock::nanos_since_boot(); + // The device's own view of an underrun, which soundserver's counters + // cannot see. + self.sound + .events(|event| match event.name() { + Some(name) => { + say!("virtio-sound: device event {:#x} ({name}) data={}", event.code, event.data) + } + None => say!("virtio-sound: device event {:#x} data={}", event.code, event.data), + }) + .unwrap_or_else(|why| panic!("soundserver: virtio-sound cannot be driven on — {why}")); + let mask = self + .sound + .completed() + .unwrap_or_else(|why| panic!("soundserver: virtio-sound cannot be driven on — {why}")); + if mask == 0 { + return 0; } - self.tx.publish(&self.dev, abi::tx_chain_head(idx)); + out[0] = AudioCompletionRecord { mask, _pad: 0, timestamp_nanos }; + 1 } - fn start(&mut self) { - if self.running { - return; + /// Put period `idx` on the wire: its PCM descriptor is a whole period. + pub fn submit(&mut self, idx: usize, bytes: usize) { + assert_eq!(bytes, PERIOD_BYTES, "soundserver: a virtio-sound period is {PERIOD_BYTES} bytes"); + let starting = !self.sound.running(); + self.sound + .submit(idx) + .unwrap_or_else(|why| panic!("soundserver: virtio-sound could not start its stream: {why}")); + if starting { + say!("virtio-sound: stream {STREAM_ID} started"); } - self.simple_ctrl(R_PCM_START, "START") - .unwrap_or_else(|e| panic!("soundserver: virtio-sound could not start its stream: {e}")); - self.running = true; - say!("virtio-sound: stream {STREAM_ID} started"); } pub fn stop(&mut self) { - if !self.running { + if !self.sound.running() { return; } - self.simple_ctrl(R_PCM_STOP, "STOP") - .unwrap_or_else(|e| panic!("soundserver: virtio-sound could not stop its stream: {e}")); - self.running = false; + self.sound + .stop() + .unwrap_or_else(|why| panic!("soundserver: virtio-sound could not stop its stream: {why}")); say!("virtio-sound: stream {STREAM_ID} stopped"); } - - /// Report what the device says went wrong, and repost the buffer it said it - /// in. The device's own view of an underrun, which soundserver's counters cannot - /// see: they measure what this process failed to submit. - pub fn poll_events(&mut self) { - while let Some(id) = self.events_done.poll() { - let idx = id as usize; - if idx >= abi::EVENT_BUFS { - say!("soundserver: virtio-sound returned event buffer {idx}, which it never had"); - continue; - } - let event = unsafe { - read_volatile( - self.base.add(abi::OFF_EVENT_BUFS + idx * abi::EVENT_BUF_STRIDE) - as *const Event, - ) - }; - let name = match event.code { - EVT_JACK_CONNECTED => " (jack connected)", - EVT_JACK_DISCONNECTED => " (jack disconnected)", - EVT_PCM_PERIOD_ELAPSED => " (period elapsed)", - EVT_PCM_XRUN => " (PCM XRUN)", - _ => "", - }; - say!( - "virtio-sound: device event {:#x}{name} data={}", - event.code, event.data - ); - self.events.publish(&self.dev, id); - } - } - - fn pcm_info(&mut self) -> Result { - let query = QueryInfo { - hdr: Hdr { code: R_PCM_INFO }, - start_id: STREAM_ID, - count: 1, - size: core::mem::size_of::() as u32, - }; - self.ctrl(&query, "PCM_INFO")?; - Ok(unsafe { - core::ptr::read_unaligned( - self.base.add(abi::OFF_CTRL_RESP + core::mem::size_of::()) as *const PcmInfo, - ) - }) - } - - fn configure(&mut self, rate: u32, channels: u8) -> Result<(), Refusal> { - // `expect`, not a fallback: `choose_params` has already checked this rate - // against the device's own bitmap, so an unencodable one here means the - // two disagree — a driver bug, not a device we cannot drive. - let code = rate_code(rate).expect("soundserver: a rate chosen from the device's own bitmap"); - let params = PcmSetParams { - hdr: Hdr { code: R_PCM_SET_PARAMS }, - stream_id: STREAM_ID, - buffer_bytes: (abi::PERIOD_BYTES * abi::PERIODS) as u32, - period_bytes: abi::PERIOD_BYTES as u32, - features: 0, - channels, - format: FMT_S16, - rate: code, - _padding: 0, - }; - self.ctrl(¶ms, "SET_PARAMS")?; - self.simple_ctrl(R_PCM_PREPARE, "PREPARE")?; - say!("virtio-sound: configured stream {STREAM_ID}: {rate}Hz {channels}ch s16le"); - Ok(()) - } - - fn simple_ctrl(&mut self, code: u32, what: &'static str) -> Result<(), Refusal> { - self.ctrl(&PcmHdr { hdr: Hdr { code }, stream_id: STREAM_ID }, what) - } - - /// One control command: copy the request into the buffer the kernel's chain - /// describes, publish that chain, wait for it back, and read the status. - fn ctrl(&mut self, req: &T, what: &'static str) -> Result<(), Refusal> { - unsafe { - core::ptr::copy_nonoverlapping( - req as *const T as *const u8, - self.base.add(abi::OFF_CTRL_REQ), - core::mem::size_of::(), - ); - } - self.control.publish(&self.dev, 0); - - let mut answered = false; - for _ in 0..CTRL_POLLS { - if self.control_done.poll().is_some() { - answered = true; - break; - } - for _ in 0..SPINS_PER_POLL { - core::hint::spin_loop(); - } - } - if !answered { - return Err(Refusal::Silent(what)); - } - - let status = unsafe { read_volatile(self.base.add(abi::OFF_CTRL_RESP) as *const u32) }; - if status != S_OK { - return Err(Refusal::Rejected(what, status)); - } - Ok(()) - } -} - -fn rate_code(hz: u32) -> Option { - SUPPORTED_RATES.iter().find(|(rate, _)| *rate == hz).map(|(_, code)| *code) -} - -/// Pick a rate and channel count the device actually advertises. -/// -/// `None` means it offers nothing this driver implements — audio is optional, so -/// soundserver presents the null sink rather than dying over a peripheral, but the -/// log has to name the missing capability or the next person is decoding a -/// bitmap by hand on a laptop with no serial. -fn choose_params(info: &PcmInfo) -> Option<(u32, u8)> { - if info.formats & (1 << FMT_S16) == 0 { - say!( - "virtio-sound: no usable format — device offers {:#x}, driver needs S16 (bit {FMT_S16})", - info.formats - ); - return None; - } - - let Some(&(rate, _)) = SUPPORTED_RATES.iter().find(|(_, code)| info.rates & (1 << code) != 0) - else { - say!( - "virtio-sound: no usable rate — device offers {:#x}, driver needs 44100 (bit \ - {RATE_44100}) or 48000 (bit {RATE_48000})", - info.rates - ); - return None; - }; - - // Stereo if the device takes it; soundserver converts either way, so the only - // unusable case is a device whose minimum is more channels than we mix. - if info.channels_min > 2 { - say!( - "virtio-sound: no usable channel count — device needs at least {}, driver mixes at \ - most 2", - info.channels_min - ); - return None; - } - let channels = if info.channels_max >= 2 { 2 } else { info.channels_max }; - if channels == 0 { - say!("virtio-sound: device advertises a maximum of zero channels"); - return None; - } - Some((rate, channels)) } diff --git a/userland/soundserver/virtio-sound/Cargo.toml b/userland/soundserver/virtio-sound/Cargo.toml new file mode 100644 index 00000000000..bb8bed344da --- /dev/null +++ b/userland/soundserver/virtio-sound/Cargo.toml @@ -0,0 +1,24 @@ +# A member of the host workspace (root `Cargo.toml`), like `toyos-mixer` beside +# it: pure, and its tests run on the host against a model of the device that +# answers on the doorbell, with the specification's tables as the oracle. + +[package] +name = "toyos-virtio-sound" +description = "soundserver's virtio-sound driver: the sound device's messages (VirtIO 1.2 §5.14) and its three queues over one grant, pure." +version = "0.1.0" +edition = "2021" +license = "MIT OR Apache-2.0" +publish = false + +[lib] +doctest = false + +[dependencies] +# The transport's `Live`, which a doorbell is rung through, and the split +# virtqueue every word the device writes back is bounded by. +toyos-virtio = { path = "../../../toyos-virtio" } +# `DmaBuffers`, the grant the three queues and every buffer are laid out in. +toyos-device-memory = { path = "../../../toyos-device-memory" } + +[lints.rust] +warnings = "deny" diff --git a/userland/soundserver/virtio-sound/src/lib.rs b/userland/soundserver/virtio-sound/src/lib.rs new file mode 100644 index 00000000000..9f5dd3d8ac9 --- /dev/null +++ b/userland/soundserver/virtio-sound/src/lib.rs @@ -0,0 +1,436 @@ +//! soundserver's virtio-sound driver: one output stream through the device's +//! three queues, every decision about them, and none of the instructions that +//! carry them out. +//! +//! Every `§` here is a section of *Virtual I/O Device (VIRTIO) Version 1.2*, +//! OASIS Committee Specification 01. [`wire`] is §5.14.6's messages; this +//! module is the queues they travel on, laid out in one grant, over +//! `toyos-virtio`'s split virtqueue and the [`Doorbell`] its live transport +//! rings. +//! +//! # The device is not trusted +//! +//! Every word it writes back — a used element, a status, a response, an +//! event — is input from outside the driver. What `toyos-virtio` bounds (a +//! head, a length no more than a chain may be written) it bounds; what is the +//! sound device's is bounded here, and one that fails is a [`Refusal`] by +//! name. §2.7.4 has a driver believe nothing past the `len` a used element +//! gives, so a status, a response or an event shorter than its structure is a +//! refusal and is not read. +//! +//! **A refusal is the end of the device's use**, as `toyos-virtio`'s are: one +//! at bring-up leaves soundserver on its null sink, and one after it ends +//! soundserver, since no conforming device writes it and every period after it +//! is one the mixer would count as played on the device's word. +//! +//! # What is not here +//! +//! - **One stream, stream 0.** The device numbers its streams and reports +//! only how many, so there is nothing here to choose a second one by. +//! - **No feature of §5.14.6.6.2 is selected**, no channel map or jack is +//! asked about, and the receive queue is never configured. + +#![no_std] +#![forbid(unsafe_code)] + +extern crate alloc; +#[cfg(test)] +extern crate std; + +pub mod wire; + +#[cfg(test)] +mod tests; + +use toyos_device_memory::{DmaBuffers, Registers}; +use toyos_virtio::pci::Live; +use toyos_virtio::queue::{Buffer, Parts, Published, Used, UsedRefusal, Virtqueue}; + +use wire::{Event, Failed, Params, PcmInfo, Request}; + +/// The pipeline, in periods and bytes: 128 frames of 16-bit stereo each. +pub const PERIODS: usize = 8; +pub const PERIOD_BYTES: usize = 512; + +/// The one stream this driver opens. +pub const STREAM_ID: u32 = 0; + +/// Descriptors per queue, powers of two (§2.7). The transmit queue holds one +/// three-descriptor chain per period, its transfer header, its PCM and its +/// status; the control queue one request and its response; the event queue +/// one buffer per descriptor. +pub const TX_QUEUE_SIZE: u16 = 32; +pub const CONTROL_QUEUE_SIZE: u16 = 2; +pub const EVENT_QUEUE_SIZE: u16 = 8; +const TX_CHAIN: u16 = 3; +const EVENT_BUFS: u16 = EVENT_QUEUE_SIZE; + +/// The longest response this driver asks for: a header and one stream's +/// information (§5.14.6.2 has the driver provide exactly that). +const RESPONSE_BYTES: u32 = wire::HDR_BYTES + wire::PCM_INFO_BYTES; +const REQUEST_BYTES: u32 = wire::SET_PARAMS_BYTES; + +/// The grant's layout: the periods, then each queue on a page of its own with +/// the buffers its chains name. +const PCM: usize = 0x0000; +const TX_PARTS: Parts = Parts::contiguous(0x1000, TX_QUEUE_SIZE); +const TX_XFER: usize = 0x1800; +const TX_STATUS: usize = 0x1C00; +const CONTROL_PARTS: Parts = Parts::contiguous(0x2000, CONTROL_QUEUE_SIZE); +const REQUEST: usize = 0x2800; +const RESPONSE: usize = 0x2C00; +const EVENT_PARTS: Parts = Parts::contiguous(0x3000, EVENT_QUEUE_SIZE); +const EVENTS: usize = 0x3800; +pub const GRANT_BYTES: usize = 0x4000; + +const _: () = { + assert!(PCM + PERIODS * PERIOD_BYTES <= TX_PARTS.desc); + assert!(TX_PARTS.end(TX_QUEUE_SIZE) <= TX_XFER); + assert!(TX_XFER + PERIODS * wire::XFER_BYTES as usize <= TX_STATUS); + assert!(TX_STATUS + PERIODS * wire::STATUS_BYTES as usize <= CONTROL_PARTS.desc); + assert!(CONTROL_PARTS.end(CONTROL_QUEUE_SIZE) <= REQUEST); + assert!(REQUEST + REQUEST_BYTES as usize <= RESPONSE); + assert!(RESPONSE + RESPONSE_BYTES as usize <= EVENT_PARTS.desc); + assert!(EVENT_PARTS.end(EVENT_QUEUE_SIZE) <= EVENTS); + assert!(EVENTS + EVENT_BUFS as usize * wire::EVENT_BYTES as usize <= GRANT_BYTES); + assert!(PERIODS * TX_CHAIN as usize <= TX_QUEUE_SIZE as usize); + assert!(PERIODS <= u32::BITS as usize); + // §5.14.6.6.3.2: `buffer_bytes % period_bytes == 0`, and every period + // whole 16-bit stereo frames. + assert!(PERIOD_BYTES % 4 == 0); +}; + +/// The rates this driver can encode, best first. 44100 leads because it is +/// what the mixer, the resampler and the recorded counters are sized against; +/// 48000 is the one every other device offers. +const RATES: [(u32, u8); 2] = [(44100, wire::RATE_44100), (48000, wire::RATE_48000)]; + +/// How many times a control command's completion is polled before the device +/// is called silent, and the spins between two looks. +/// +/// **A count and not a deadline**: a wall clock keeps running while this guest +/// is not, so a duration here would measure the host's scheduler and call a +/// healthy device gone. Policy, not physics — the specification has no number, +/// and a device answering at all answers in one round trip. +const CONTROL_POLLS: u32 = 100_000; +const SPINS_PER_POLL: u32 = 256; + +/// Where a chain made available is told to the device: the live transport. +pub trait Doorbell { + fn ring(&self, published: Published); +} + +impl Doorbell for Live { + fn ring(&self, published: Published) { + self.notify(published); + } +} + +/// Why the device is not driven. Each is something it answered, or did not. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Refusal { + /// Its configuration says it has no PCM stream (§5.14.4). + NoStream, + /// A used ring `toyos-virtio` did not believe. + Used(u16, UsedRefusal), + /// It never answered a control request. + Silent(Request), + /// It answered fewer bytes than the response's structure. + ShortAnswer { request: Request, written: u32, wanted: u32 }, + /// It answered a status §5.14.6 does not define. + UnknownStatus(Request, u32), + /// It answered, and said no. + Rejected(Request, Failed), + /// Stream 0 does not carry output (§5.14.6.6.2). + NotOutput { direction: u8 }, + /// It offers no format, rate or channel count this driver writes. + NoFormat { formats: u64 }, + NoRate { rates: u64 }, + NoChannels { min: u8, max: u8 }, + /// A period's status is shorter than its structure. + ShortStatus { period: usize, written: u32 }, + /// A period's status is not `S_OK` (§5.14.6.8), or no status at all. + PeriodFailed { period: usize, status: Result }, + /// An event is shorter than its structure. + ShortEvent { written: u32 }, +} + +impl core::fmt::Display for Refusal { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + match self { + Self::NoStream => write!(f, "its configuration offers no PCM stream"), + Self::Used(queue, why) => write!(f, "its queue {queue}'s used ring: {why}"), + Self::Silent(request) => write!(f, "it never answered {}", request.name()), + Self::ShortAnswer { request, written, wanted } => write!( + f, + "it answered {} with {written} byte(s), where the response is {wanted}", + request.name() + ), + Self::UnknownStatus(request, word) => { + write!(f, "it answered {} with the status {word:#x}, which is none", request.name()) + } + Self::Rejected(request, failed) => { + write!(f, "it refused {} ({})", request.name(), failed.name()) + } + Self::NotOutput { direction } => { + write!(f, "its stream {STREAM_ID} has direction {direction}, not output") + } + Self::NoFormat { formats } => write!( + f, + "it offers the formats {formats:#x}, without S16 (bit {})", + wire::FMT_S16 + ), + Self::NoRate { rates } => write!( + f, + "it offers the rates {rates:#x}, without 44100 (bit {}) or 48000 (bit {})", + wire::RATE_44100, + wire::RATE_48000 + ), + Self::NoChannels { min, max } => { + write!(f, "it takes {min} to {max} channel(s), and this driver writes 1 or 2") + } + Self::ShortStatus { period, written } => write!( + f, + "it gave period {period} back with {written} byte(s) of its {}-byte status", + wire::STATUS_BYTES + ), + Self::PeriodFailed { period, status: Ok(failed) } => { + write!(f, "it failed period {period} ({})", failed.name()) + } + Self::PeriodFailed { period, status: Err(word) } => { + write!(f, "it gave period {period} back with the status {word:#x}, which is none") + } + Self::ShortEvent { written } => write!( + f, + "it wrote an event of {written} byte(s), where one is {}", + wire::EVENT_BYTES + ), + } + } +} + +/// The three queues, laid out in the grant before the transport is given +/// them: [`toyos_virtio::pci::Setup::enable`] takes each, and [`Sound::open`] +/// takes them back once the device is live. +pub struct Queues { + pub control: Virtqueue, + pub events: Virtqueue, + pub tx: Virtqueue, +} + +impl Queues { + /// # Panics + /// If the grant is shorter than [`GRANT_BYTES`]: the caller's own mistake. + pub fn new(mem: M) -> Self { + Self { + control: Virtqueue::new(mem.clone(), wire::CONTROL_QUEUE, CONTROL_QUEUE_SIZE, CONTROL_PARTS), + events: Virtqueue::new(mem.clone(), wire::EVENT_QUEUE, EVENT_QUEUE_SIZE, EVENT_PARTS), + tx: Virtqueue::new(mem, wire::TX_QUEUE, TX_QUEUE_SIZE, TX_PARTS), + } + } +} + +/// What the device said stream 0 is, and what this driver chose of it. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct Stream { + pub info: PcmInfo, + pub rate: u32, + pub channels: u8, +} + +/// The device, live and configured: stream 0 prepared, every event buffer +/// posted. +pub struct Sound { + mem: M, + bell: D, + queues: Queues, + running: bool, +} + +impl Sound { + /// §5.14.5 on a live device whose configuration counts `streams`: post + /// the event buffers, ask what stream 0 is, choose a rate and a channel + /// count it offers, and set and prepare it (§5.14.6.6.1). + pub fn open(queues: Queues, mem: M, bell: D, streams: u32) -> Result<(Self, Stream), Refusal> { + if streams == 0 { + return Err(Refusal::NoStream); + } + let mut sound = Self { mem, bell, queues, running: false }; + for buffer in 0..EVENT_BUFS { + sound.post_event(buffer); + } + let info = sound.pcm_info()?; + let (rate, code, channels) = choose(&info)?; + let params = Params { + buffer_bytes: (PERIODS * PERIOD_BYTES) as u32, + period_bytes: PERIOD_BYTES as u32, + channels, + format: wire::FMT_S16, + rate: code, + }; + sound.control(Request::SetParams, &wire::set_params(STREAM_ID, params), wire::HDR_BYTES)?; + sound.simple(Request::Prepare, wire::R_PCM_PREPARE)?; + Ok((sound, Stream { info, rate, channels })) + } + + /// Where period `idx`'s samples go in the grant. + pub fn period(idx: usize) -> usize { + assert!(idx < PERIODS, "virtio-sound: there is no period {idx}"); + PCM + idx * PERIOD_BYTES + } + + pub fn running(&self) -> bool { + self.running + } + + /// Put period `idx` on the wire, starting the stream first if it is + /// stopped (§5.14.6.6.1). + /// + /// # Panics + /// If period `idx` is still the device's: the caller's own mistake. + pub fn submit(&mut self, idx: usize) -> Result<(), Refusal> { + let pcm = Self::period(idx); + if !self.running { + self.simple(Request::Start, wire::R_PCM_START)?; + self.running = true; + } + let xfer = TX_XFER + idx * wire::XFER_BYTES as usize; + let status = TX_STATUS + idx * wire::STATUS_BYTES as usize; + self.mem.write32(xfer, STREAM_ID); + // What a status the device does not write leaves behind is no `S_OK`. + self.mem.write32(status, 0); + let chain = [ + Buffer::readable(self.mem.device_addr(xfer), wire::XFER_BYTES), + Buffer::readable(self.mem.device_addr(pcm), PERIOD_BYTES as u32), + Buffer::writable(self.mem.device_addr(status), wire::STATUS_BYTES), + ]; + let published = self.queues.tx.publish(idx as u16 * TX_CHAIN, &chain); + self.bell.ring(published); + Ok(()) + } + + /// Stop the stream; nothing when it is stopped. + pub fn stop(&mut self) -> Result<(), Refusal> { + if self.running { + self.simple(Request::Stop, wire::R_PCM_STOP)?; + self.running = false; + } + Ok(()) + } + + /// Every period the device has given back since the last call, as a mask. + pub fn completed(&mut self) -> Result { + let mut mask = 0; + while let Some(Used { head, written }) = used(&mut self.queues.tx)? { + // A head `toyos-virtio` answered is one a chain was published at, + // and this driver publishes one only at a period's. + let period = (head / TX_CHAIN) as usize; + if written < wire::STATUS_BYTES { + return Err(Refusal::ShortStatus { period, written }); + } + let word = self.mem.read32(TX_STATUS + period * wire::STATUS_BYTES as usize); + match wire::status(word) { + Ok(Ok(())) => mask |= 1 << period, + Ok(Err(failed)) => return Err(Refusal::PeriodFailed { period, status: Ok(failed) }), + Err(word) => return Err(Refusal::PeriodFailed { period, status: Err(word) }), + } + } + Ok(mask) + } + + /// Every event the device has written since the last call, oldest first, + /// each buffer posted again once it is read. + pub fn events(&mut self, mut said: impl FnMut(Event)) -> Result<(), Refusal> { + while let Some(Used { head, written }) = used(&mut self.queues.events)? { + if written < wire::EVENT_BYTES { + return Err(Refusal::ShortEvent { written }); + } + let at = EVENTS + head as usize * wire::EVENT_BYTES as usize; + said(Event { code: self.mem.read32(at), data: self.mem.read32(at + 4) }); + self.post_event(head); + } + Ok(()) + } + + fn post_event(&mut self, buffer: u16) { + let at = EVENTS + buffer as usize * wire::EVENT_BYTES as usize; + let chain = [Buffer::writable(self.mem.device_addr(at), wire::EVENT_BYTES)]; + let published = self.queues.events.publish(buffer, &chain); + self.bell.ring(published); + } + + fn pcm_info(&mut self) -> Result { + let request = Request::PcmInfo; + self.control(request, &wire::pcm_info_query(STREAM_ID, 1), RESPONSE_BYTES)?; + let at = RESPONSE + wire::HDR_BYTES as usize; + Ok(PcmInfo::decode(core::array::from_fn(|word| self.mem.read32(at + 4 * word)))) + } + + fn simple(&mut self, request: Request, code: u32) -> Result<(), Refusal> { + self.control(request, &wire::pcm_hdr(code, STREAM_ID), wire::HDR_BYTES) + } + + /// One control round trip: the request's words into the request buffer, a + /// chain of it and a `response`-byte response, published and rung, and + /// the answer's length and status held to §5.14.6. + fn control(&mut self, request: Request, words: &[u32], response: u32) -> Result<(), Refusal> { + for (nth, word) in words.iter().enumerate() { + self.mem.write32(REQUEST + 4 * nth, *word); + } + let chain = [ + Buffer::readable(self.mem.device_addr(REQUEST), 4 * words.len() as u32), + Buffer::writable(self.mem.device_addr(RESPONSE), response), + ]; + let published = self.queues.control.publish(0, &chain); + self.bell.ring(published); + + let mut answer = None; + for _ in 0..CONTROL_POLLS { + answer = used(&mut self.queues.control)?; + if answer.is_some() { + break; + } + for _ in 0..SPINS_PER_POLL { + core::hint::spin_loop(); + } + } + let Used { written, .. } = answer.ok_or(Refusal::Silent(request))?; + if written < response { + return Err(Refusal::ShortAnswer { request, written, wanted: response }); + } + match wire::status(self.mem.read32(RESPONSE)) { + Ok(Ok(())) => Ok(()), + Ok(Err(failed)) => Err(Refusal::Rejected(request, failed)), + Err(word) => Err(Refusal::UnknownStatus(request, word)), + } + } +} + +/// The next chain `queue`'s device has finished with, or the used ring's +/// refusal under the queue's own index. +fn used(queue: &mut Virtqueue) -> Result, Refusal> { + let index = queue.index(); + queue.poll_used().map_err(|why| Refusal::Used(index, why)) +} + +/// A rate and a channel count stream 0 offers that this driver writes, and +/// the rate's code. +fn choose(info: &PcmInfo) -> Result<(u32, u8, u8), Refusal> { + if info.direction != wire::D_OUTPUT { + return Err(Refusal::NotOutput { direction: info.direction }); + } + if info.formats & (1 << wire::FMT_S16) == 0 { + return Err(Refusal::NoFormat { formats: info.formats }); + } + let (rate, code) = *RATES + .iter() + .find(|(_, code)| info.rates & (1 << code) != 0) + .ok_or(Refusal::NoRate { rates: info.rates })?; + // Stereo where the device takes it; the mixer converts either way. + let (min, max) = (info.channels_min, info.channels_max); + let channels = max.min(2); + if channels == 0 || min > channels { + return Err(Refusal::NoChannels { min, max }); + } + Ok((rate, code, channels)) +} diff --git a/userland/soundserver/virtio-sound/src/tests.rs b/userland/soundserver/virtio-sound/src/tests.rs new file mode 100644 index 00000000000..c59774424dd --- /dev/null +++ b/userland/soundserver/virtio-sound/src/tests.rs @@ -0,0 +1,566 @@ +//! The driver against a model of the device that answers on the doorbell, +//! with §5.14's tables as the oracle for every byte it is sent and every +//! answer a device can lie in. + +use alloc::boxed::Box; +use alloc::rc::Rc; +use alloc::vec; +use alloc::vec::Vec; +use core::cell::RefCell; + +use toyos_device_memory::DmaBuffers; +use toyos_virtio::queue::{Published, UsedRefusal}; + +use crate::wire::{self, Event, Failed, PcmInfo, Request}; +use crate::{ + Doorbell, Queues, Refusal, Sound, Stream, CONTROL_PARTS, EVENTS, EVENT_PARTS, PERIODS, PERIOD_BYTES, + TX_PARTS, +}; + +/// Where the device reaches the grant's first byte. +const BASE: u64 = 0x4000_0000; + +/// The grant, as both the driver and the model reach it. +#[derive(Clone)] +struct Mem(Rc>>); + +impl Mem { + fn new() -> Self { + // Anything but zero, so a part the driver forgets to clear shows. + Self(Rc::new(RefCell::new(vec![0xA5; crate::GRANT_BYTES]))) + } + + fn get(&self, at: usize, bytes: usize) -> u64 { + let mem = self.0.borrow(); + let mut word = [0u8; 8]; + word[..bytes].copy_from_slice(&mem[at..at + bytes]); + u64::from_le_bytes(word) + } + + fn set(&self, at: usize, bytes: usize, value: u64) { + self.0.borrow_mut()[at..at + bytes].copy_from_slice(&value.to_le_bytes()[..bytes]); + } + + fn bytes_at(&self, addr: u64, len: u32) -> Vec { + let at = (addr - BASE) as usize; + self.0.borrow()[at..at + len as usize].to_vec() + } +} + +impl DmaBuffers for Mem { + fn bytes(&self) -> usize { + crate::GRANT_BYTES + } + fn device_addr(&self, at: usize) -> u64 { + BASE + at as u64 + } + fn read16(&self, at: usize) -> u16 { + self.get(at, 2) as u16 + } + fn read32(&self, at: usize) -> u32 { + self.get(at, 4) as u32 + } + fn read64(&self, at: usize) -> u64 { + self.get(at, 8) + } + fn write16(&self, at: usize, value: u16) { + self.set(at, 2, value as u64) + } + fn write32(&self, at: usize, value: u32) { + self.set(at, 4, value as u64) + } + fn write64(&self, at: usize, value: u64) { + self.set(at, 8, value) + } + fn publish(&self) {} + fn observe(&self) {} +} + +/// One element of a chain the device took: §2.7.5's descriptor. +#[derive(Clone, Debug, PartialEq, Eq)] +struct Desc { + addr: u64, + len: u32, + writable: bool, +} + +/// What the model answers a control request with. +struct Answer { + status: u32, + payload: Vec, + /// The `len` it reports, where it is not the bytes it wrote. + len: Option, + silent: bool, +} + +fn ok() -> Answer { + Answer { status: wire::S_OK, payload: Vec::new(), len: None, silent: false } +} + +/// QEMU's stream at v11.1.1: S16 at 44100 and 48000 among others, output, +/// one or two channels. +fn qemu_stream() -> PcmInfo { + PcmInfo { + hda_fn_nid: 0, + features: 0, + formats: 1 << wire::FMT_S16 | 1 << 3, + rates: 1 << wire::RATE_44100 | 1 << wire::RATE_48000 | 1 << 1, + direction: wire::D_OUTPUT, + channels_min: 1, + channels_max: 2, + } +} + +/// `info` as the eight words of §5.14.6.6.2's structure. +fn encode(info: PcmInfo) -> Vec { + let mut bytes = [0u8; 32]; + bytes[0..4].copy_from_slice(&info.hda_fn_nid.to_le_bytes()); + bytes[4..8].copy_from_slice(&info.features.to_le_bytes()); + bytes[8..16].copy_from_slice(&info.formats.to_le_bytes()); + bytes[16..24].copy_from_slice(&info.rates.to_le_bytes()); + bytes[24] = info.direction; + bytes[25] = info.channels_min; + bytes[26] = info.channels_max; + words(&bytes) +} + +fn words(bytes: &[u8]) -> Vec { + bytes.chunks(4).map(|w| u32::from_le_bytes(w.try_into().unwrap())).collect() +} + +struct State { + /// Per queue: the available entries taken so far, and the used ones + /// written. + taken: [u16; 3], + used: [u16; 3], + /// Every control request, as the device read it. + requests: Vec>, + /// Every chain the device took, by queue, in order. + chains: [Vec<(u16, Vec)>; 3], + /// Chains the device holds: the transmit queue's until played, the + /// event queue's until an event. + held: [Vec<(u16, Vec)>; 3], + answer: Box Answer>, +} + +#[derive(Clone)] +struct Model { + mem: Mem, + state: Rc>, +} + +impl Model { + fn new() -> Self { + Self::answering(|request| match request[0] { + wire::R_PCM_INFO => Answer { payload: encode(qemu_stream()), ..ok() }, + _ => ok(), + }) + } + + fn answering(answer: impl FnMut(&[u32]) -> Answer + 'static) -> Self { + Self { + mem: Mem::new(), + state: Rc::new(RefCell::new(State { + taken: [0; 3], + used: [0; 3], + requests: Vec::new(), + chains: Default::default(), + held: Default::default(), + answer: Box::new(answer), + })), + } + } + + fn parts(queue: u16) -> (crate::Parts, u16) { + match queue { + wire::CONTROL_QUEUE => (CONTROL_PARTS, crate::CONTROL_QUEUE_SIZE), + wire::EVENT_QUEUE => (EVENT_PARTS, crate::EVENT_QUEUE_SIZE), + wire::TX_QUEUE => (TX_PARTS, crate::TX_QUEUE_SIZE), + other => panic!("model: queue {other} was never configured"), + } + } + + /// The chain at `head`, by following its descriptors (§2.7.5). + fn chain(&self, queue: u16, head: u16) -> Vec { + let (parts, size) = Self::parts(queue); + let mut descs = Vec::new(); + let mut at = head; + loop { + assert!(at < size && descs.len() < size as usize, "model: a chain off the table"); + let d = parts.desc + at as usize * 16; + let flags = self.mem.get(d + 12, 2); + descs.push(Desc { + addr: self.mem.get(d, 8), + len: self.mem.get(d + 8, 4) as u32, + writable: flags & 2 != 0, + }); + if flags & 1 == 0 { + return descs; + } + at = self.mem.get(d + 14, 2) as u16; + } + } + + /// A used element on `queue`: `head`, `len` bytes written (§2.7.8). + fn give_back(&self, queue: u16, head: u16, len: u32) { + let (parts, size) = Self::parts(queue); + let mut state = self.state.borrow_mut(); + let used = &mut state.used[queue as usize]; + let element = parts.used + 4 + (*used % size) as usize * 8; + self.mem.set(element, 4, head as u64); + self.mem.set(element + 4, 4, len as u64); + *used = used.wrapping_add(1); + self.mem.set(parts.used + 2, 2, *used as u64); + } + + /// Play the `count` oldest periods: each status `status`, `len` reported. + fn play_with(&self, count: usize, status: u32, len: u32) { + for _ in 0..count { + let (head, chain) = self.state.borrow_mut().held[wire::TX_QUEUE as usize].remove(0); + self.mem.set((chain[2].addr - BASE) as usize, 4, status as u64); + self.give_back(wire::TX_QUEUE, head, len); + } + } + + fn play(&self, count: usize) { + self.play_with(count, wire::S_OK, wire::STATUS_BYTES); + } + + /// Write `event` into the oldest event buffer, `len` reported. + fn event(&self, event: Event, len: u32) { + let (head, chain) = self.state.borrow_mut().held[wire::EVENT_QUEUE as usize].remove(0); + let at = (chain[0].addr - BASE) as usize; + self.mem.set(at, 4, event.code as u64); + self.mem.set(at + 4, 4, event.data as u64); + self.give_back(wire::EVENT_QUEUE, head, len); + } + + /// The control requests' codes, in the order the device read them. + fn codes(&self) -> Vec { + self.state.borrow().requests.iter().map(|r| words(&r[..4])[0]).collect() + } + + fn chains(&self, queue: u16) -> Vec<(u16, Vec)> { + self.state.borrow().chains[queue as usize].clone() + } +} + +impl Doorbell for Model { + fn ring(&self, published: Published) { + let queue = published.queue(); + let (parts, size) = Self::parts(queue); + loop { + let available = self.mem.get(parts.avail + 2, 2) as u16; + let taken = self.state.borrow().taken[queue as usize]; + if taken == available { + return; + } + let head = self.mem.get(parts.avail + 4 + (taken % size) as usize * 2, 2) as u16; + self.state.borrow_mut().taken[queue as usize] = taken.wrapping_add(1); + let chain = self.chain(queue, head); + self.state.borrow_mut().chains[queue as usize].push((head, chain.clone())); + if queue != wire::CONTROL_QUEUE { + self.state.borrow_mut().held[queue as usize].push((head, chain)); + continue; + } + let request: Vec = chain + .iter() + .filter(|d| !d.writable) + .flat_map(|d| self.mem.bytes_at(d.addr, d.len)) + .collect(); + self.state.borrow_mut().requests.push(request.clone()); + let answer = (self.state.borrow_mut().answer)(&words(&request)); + if answer.silent { + continue; + } + let response = chain.iter().find(|d| d.writable).expect("a response buffer"); + let mut out = vec![answer.status]; + out.extend(&answer.payload); + let wrote = (4 * out.len() as u32).min(response.len); + for (nth, word) in out.iter().enumerate().take(wrote as usize / 4) { + self.mem.set((response.addr - BASE) as usize + 4 * nth, 4, *word as u64); + } + self.give_back(queue, head, answer.len.unwrap_or(wrote)); + } + } +} + +fn open(model: &Model, streams: u32) -> Result<(Sound, Stream), Refusal> { + Sound::open(Queues::new(model.mem.clone()), model.mem.clone(), model.clone(), streams) +} + +fn opened(model: &Model) -> Sound { + open(model, 1).unwrap_or_else(|why| panic!("the model's device opens: {why}")).0 +} + +// --- §5.14.6: the messages, byte for byte --- + +/// §5.14.6.1 and §5.14.6.6.2: `virtio_snd_query_info` is `hdr`, `start_id`, +/// `count` and `size`, each `le32`; §5.14.6.6.3: `virtio_snd_pcm_set_params` +/// is `hdr`, `stream_id`, `buffer_bytes`, `period_bytes`, `features`, then +/// `channels`, `format`, `rate` and a padding byte; §5.14.6.6: a PREPARE is +/// `hdr` and `stream_id`. The bring-up is §5.14.5's order. +#[test] +fn the_bring_up_sends_the_specifications_bytes_in_its_order() { + let model = Model::new(); + let (_, stream) = open(&model, 1).expect("the model's device opens"); + assert_eq!(stream.rate, 44100); + assert_eq!(stream.channels, 2); + assert_eq!(stream.info, qemu_stream()); + + let requests = model.state.borrow().requests.clone(); + assert_eq!( + requests, + [ + vec![0x00, 0x01, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 32, 0, 0, 0], + vec![ + 0x01, 0x01, 0, 0, 0, 0, 0, 0, 0x00, 0x10, 0, 0, 0x00, 0x02, 0, 0, 0, 0, 0, 0, 2, + 5, 6, 0 + ], + vec![0x02, 0x01, 0, 0, 0, 0, 0, 0], + ] + ); + // §5.14.6.2: a response buffer of `sizeof(hdr) + count * size`; a bare + // status for the others. + let responses: Vec = model + .chains(wire::CONTROL_QUEUE) + .iter() + .map(|(_, chain)| { + assert_eq!(chain.len(), 2); + assert!(!chain[0].writable && chain[1].writable); + chain[1].len + }) + .collect(); + assert_eq!(responses, [36, 4, 4]); + // §5.14.5.1: the event queue full of device-writable buffers of at least + // `virtio_snd_event`'s eight bytes, and none device-readable. + let events = model.chains(wire::EVENT_QUEUE); + assert_eq!(events.len(), crate::EVENT_QUEUE_SIZE as usize); + for (head, chain) in &events { + assert_eq!(chain, &[Desc { addr: BASE + (EVENTS + *head as usize * 8) as u64, len: 8, writable: true }]); + } + assert!(model.chains(wire::TX_QUEUE).is_empty(), "a period before the stream is started"); +} + +/// §5.14.6.6.2's `virtio_snd_pcm_info`: `hdr` at 0, `features` at 4, +/// `formats` at 8 and `rates` at 16 as `le64`, then `direction`, +/// `channels_min` and `channels_max` at 24, 25 and 26. +#[test] +fn stream_information_is_decoded_at_the_specifications_offsets() { + let mut bytes = [0u8; 32]; + bytes[0..4].copy_from_slice(&0x11u32.to_le_bytes()); + bytes[4..8].copy_from_slice(&0x2222_2222u32.to_le_bytes()); + bytes[8..16].copy_from_slice(&0x0807_0605_0403_0201u64.to_le_bytes()); + bytes[16..24].copy_from_slice(&0x1817_1615_1413_1211u64.to_le_bytes()); + bytes[24..27].copy_from_slice(&[1, 3, 7]); + bytes[27..32].copy_from_slice(&[0xFF; 5]); + let info = PcmInfo::decode(words(&bytes).try_into().unwrap()); + assert_eq!( + info, + PcmInfo { + hda_fn_nid: 0x11, + features: 0x2222_2222, + formats: 0x0807_0605_0403_0201, + rates: 0x1817_1615_1413_1211, + direction: 1, + channels_min: 3, + channels_max: 7, + } + ); +} + +/// §5.14.6: the four status codes and the four event types. +#[test] +fn every_code_is_the_specifications_number() { + assert_eq!(wire::status(0x8000), Ok(Ok(()))); + assert_eq!(wire::status(0x8001), Ok(Err(Failed::BadMsg))); + assert_eq!(wire::status(0x8002), Ok(Err(Failed::NotSupp))); + assert_eq!(wire::status(0x8003), Ok(Err(Failed::IoErr))); + for none in [0, 0x7FFF, 0x8004, u32::MAX] { + assert_eq!(wire::status(none), Err(none)); + } + let named = |code| Event { code, data: 0 }.name(); + assert_eq!(named(0x1000), Some("jack connected")); + assert_eq!(named(0x1001), Some("jack disconnected")); + assert_eq!(named(0x1100), Some("period elapsed")); + assert_eq!(named(0x1101), Some("PCM XRUN")); + assert_eq!(named(0x1102), None); + // §5.14.6.6.2's enumerations, by the bit each is. + assert_eq!((wire::FMT_S16, wire::RATE_44100, wire::RATE_48000), (5, 6, 7)); +} + +// --- What the driver chooses --- + +/// A rate and a channel count the stream offers: 44100 over 48000, stereo +/// over mono, and each refusal names the bitmap or range it read. +#[test] +fn the_stream_is_set_to_what_it_offers_or_refused_by_what_it_lacks() { + let with = |change: fn(&mut PcmInfo)| { + let mut info = qemu_stream(); + change(&mut info); + let model = Model::answering(move |request| match request[0] { + wire::R_PCM_INFO => Answer { payload: encode(info), ..ok() }, + _ => ok(), + }); + let set = open(&model, 1).map(|(_, stream)| (stream.rate, stream.channels)); + (set, model) + }; + assert_eq!(with(|i| i.rates = 1 << wire::RATE_48000).0, Ok((48000, 2))); + assert_eq!(with(|i| i.channels_max = 1).0, Ok((44100, 1))); + assert_eq!(with(|i| i.channels_max = 8).0, Ok((44100, 2))); + let refused = [ + (with(|i| i.direction = 1), Refusal::NotOutput { direction: 1 }), + (with(|i| i.formats = 1 << 3), Refusal::NoFormat { formats: 1 << 3 }), + (with(|i| i.rates = 1 << 10), Refusal::NoRate { rates: 1 << 10 }), + (with(|i| i.channels_min = 3), Refusal::NoChannels { min: 3, max: 2 }), + (with(|i| i.channels_max = 0), Refusal::NoChannels { min: 1, max: 0 }), + ]; + for ((set, model), refusal) in refused { + assert_eq!(set, Err(refusal)); + // Nothing past the question: a stream it cannot carry is never set. + assert_eq!(model.codes(), [wire::R_PCM_INFO]); + } +} + +// --- A device's answers, believed only as far as §5.14.6 goes --- + +/// The negative control: each malformed answer is refused by its own name, +/// and the bring-up goes no further than the request it answered. +#[test] +fn a_malformed_control_answer_is_refused_by_name() { + let answering = |answer: fn(&[u32]) -> Option| { + let model = Model::answering(move |request| { + answer(request).unwrap_or_else(|| match request[0] { + wire::R_PCM_INFO => Answer { payload: encode(qemu_stream()), ..ok() }, + _ => ok(), + }) + }); + (open(&model, 1).err(), model.codes().len()) + }; + // A status §5.14.6 does not define. + assert_eq!( + answering(|r| (r[0] == wire::R_PCM_PREPARE).then(|| Answer { status: 0x1234, ..ok() })), + (Some(Refusal::UnknownStatus(Request::Prepare, 0x1234)), 3) + ); + // An `S_OK` with no stream information behind it. + assert_eq!( + answering(|r| (r[0] == wire::R_PCM_INFO).then(ok)), + (Some(Refusal::ShortAnswer { request: Request::PcmInfo, written: 4, wanted: 36 }), 1) + ); + // A `len` that covers less than the status word. + assert_eq!( + answering(|r| (r[0] == wire::R_PCM_PREPARE).then(|| Answer { len: Some(3), ..ok() })), + (Some(Refusal::ShortAnswer { request: Request::Prepare, written: 3, wanted: 4 }), 3) + ); + // A `len` past the response buffer is `toyos-virtio`'s to refuse. + assert!(matches!( + answering(|r| (r[0] == wire::R_PCM_SET_PARAMS).then(|| Answer { len: Some(5), ..ok() })), + (Some(Refusal::Used(wire::CONTROL_QUEUE, UsedRefusal::Written { head: 0, .. })), 2) + )); + // A well-formed no. + assert_eq!( + answering(|r| (r[0] == wire::R_PCM_SET_PARAMS).then(|| Answer { status: wire::S_NOT_SUPP, ..ok() })), + (Some(Refusal::Rejected(Request::SetParams, Failed::NotSupp)), 2) + ); +} + +/// A device that never answers is called silent after a bounded count of +/// looks, and a device with no stream is never asked anything. +#[test] +fn a_silent_device_and_a_streamless_one_are_refused() { + let model = Model::answering(|_| Answer { silent: true, ..ok() }); + assert_eq!(open(&model, 1).err(), Some(Refusal::Silent(Request::PcmInfo))); + + let model = Model::new(); + assert_eq!(open(&model, 0).err(), Some(Refusal::NoStream)); + assert!(model.codes().is_empty() && model.chains(wire::EVENT_QUEUE).is_empty()); +} + +// --- §5.14.6.8: the periods --- + +/// The stream is started once before its first period, every period is +/// §5.14.6.8's three parts — the stream id, the PCM, a status for the device +/// — and a completion is the set of periods given back with `S_OK`. +#[test] +fn periods_go_out_after_one_start_and_come_back_as_a_mask() { + let model = Model::new(); + let mut sound = opened(&model); + for idx in 0..PERIODS { + sound.submit(idx).expect("a period goes out"); + } + assert_eq!(model.codes(), [wire::R_PCM_INFO, wire::R_PCM_SET_PARAMS, wire::R_PCM_PREPARE, wire::R_PCM_START]); + let chains = model.chains(wire::TX_QUEUE); + assert_eq!(chains.len(), PERIODS); + for (idx, (head, chain)) in chains.iter().enumerate() { + assert_eq!(*head as usize, idx * 3); + assert_eq!(chain.len(), 3); + assert_eq!((chain[0].len, chain[0].writable), (wire::XFER_BYTES, false)); + assert_eq!(model.mem.bytes_at(chain[0].addr, 4), [0, 0, 0, 0], "stream 0"); + assert_eq!(chain[1], Desc { addr: BASE + (idx * PERIOD_BYTES) as u64, len: 512, writable: false }); + assert_eq!((chain[2].len, chain[2].writable), (wire::STATUS_BYTES, true)); + } + assert_eq!(sound.completed(), Ok(0)); + model.play(3); + assert_eq!(sound.completed(), Ok(0b111)); + model.play(5); + assert_eq!(sound.completed(), Ok(0b1111_1000)); + + // A drained stream stops once, and starts again on its next period. + sound.stop().expect("STOP"); + sound.stop().expect("nothing"); + sound.submit(4).expect("a period goes out"); + assert_eq!( + model.codes()[3..], + [wire::R_PCM_START, wire::R_PCM_STOP, wire::R_PCM_START] + ); +} + +/// A period the device gives back with a short status, a failure, or a +/// status §5.14.6.8 does not define is refused by that name, as is a used +/// element for no period in flight. +#[test] +fn a_period_given_back_wrongly_is_refused_by_name() { + let given_back = |status: u32, len: u32| { + let model = Model::new(); + let mut sound = opened(&model); + sound.submit(0).expect("a period goes out"); + sound.submit(1).expect("a period goes out"); + model.play(1); + model.play_with(1, status, len); + sound.completed() + }; + assert_eq!(given_back(wire::S_OK, 8), Ok(0b11)); + assert_eq!(given_back(wire::S_OK, 4), Err(Refusal::ShortStatus { period: 1, written: 4 })); + assert_eq!( + given_back(wire::S_IO_ERR, 8), + Err(Refusal::PeriodFailed { period: 1, status: Ok(Failed::IoErr) }) + ); + assert_eq!(given_back(7, 8), Err(Refusal::PeriodFailed { period: 1, status: Err(7) })); + + let model = Model::new(); + let mut sound = opened(&model); + sound.submit(0).expect("a period goes out"); + model.give_back(wire::TX_QUEUE, 3, 8); + assert_eq!( + sound.completed(), + Err(Refusal::Used(wire::TX_QUEUE, UsedRefusal::NoChain { head: 3 })) + ); +} + +/// An event is read, handed up and its buffer posted again; one shorter than +/// `virtio_snd_event` is refused and not read. +#[test] +fn an_event_is_handed_up_and_its_buffer_posted_again() { + let model = Model::new(); + let mut sound = opened(&model); + model.event(Event { code: wire::EVT_PCM_XRUN, data: 0 }, 8); + let mut said = Vec::new(); + sound.events(|event| said.push(event)).expect("a whole event"); + assert_eq!(said, [Event { code: wire::EVT_PCM_XRUN, data: 0 }]); + assert_eq!(model.chains(wire::EVENT_QUEUE).len(), crate::EVENT_QUEUE_SIZE as usize + 1); + + model.event(Event { code: wire::EVT_JACK_CONNECTED, data: 1 }, 4); + let mut said = Vec::new(); + assert_eq!(sound.events(|event| said.push(event)), Err(Refusal::ShortEvent { written: 4 })); + assert!(said.is_empty()); +} diff --git a/userland/soundserver/virtio-sound/src/wire.rs b/userland/soundserver/virtio-sound/src/wire.rs new file mode 100644 index 00000000000..82be867fb45 --- /dev/null +++ b/userland/soundserver/virtio-sound/src/wire.rs @@ -0,0 +1,187 @@ +//! The sound device's messages (§5.14.6): their codes, and each structure as +//! the little-endian words it is on the wire. +//! +//! **Every structure here is a whole number of `le32` words**, the `u8` +//! fields of one packed into a word in the order the structure lists them, +//! so a message is written into the grant and read out of it as words and no +//! field is ever a byte access of its own. + +/// §5.14.2: the queues, by index. The fourth, `rxq`, carries input and is +/// never configured. +pub const CONTROL_QUEUE: u16 = 0; +pub const EVENT_QUEUE: u16 = 1; +pub const TX_QUEUE: u16 = 2; + +/// §5.14.6: the request types this driver sends. +pub const R_PCM_INFO: u32 = 0x0100; +pub const R_PCM_SET_PARAMS: u32 = 0x0101; +pub const R_PCM_PREPARE: u32 = 0x0102; +pub const R_PCM_START: u32 = 0x0104; +pub const R_PCM_STOP: u32 = 0x0105; + +/// §5.14.6: the event types a device sends. +pub const EVT_JACK_CONNECTED: u32 = 0x1000; +pub const EVT_JACK_DISCONNECTED: u32 = 0x1001; +pub const EVT_PCM_PERIOD_ELAPSED: u32 = 0x1100; +pub const EVT_PCM_XRUN: u32 = 0x1101; + +/// §5.14.6: the status codes, and the only four a device answers. +pub const S_OK: u32 = 0x8000; +pub const S_BAD_MSG: u32 = 0x8001; +pub const S_NOT_SUPP: u32 = 0x8002; +pub const S_IO_ERR: u32 = 0x8003; + +/// §5.14.6: `VIRTIO_SND_D_OUTPUT`. +pub const D_OUTPUT: u8 = 0; + +/// §5.14.6.6.2: the one sample format this driver writes, and the two rates +/// it can encode, as bit numbers of `formats` and `rates`. +pub const FMT_S16: u8 = 5; +pub const RATE_44100: u8 = 6; +pub const RATE_48000: u8 = 7; + +/// Bytes of each structure. +pub const HDR_BYTES: u32 = 4; +pub const QUERY_INFO_BYTES: u32 = 16; +pub const PCM_HDR_BYTES: u32 = 8; +pub const PCM_INFO_BYTES: u32 = 32; +pub const SET_PARAMS_BYTES: u32 = 24; +pub const XFER_BYTES: u32 = 4; +pub const STATUS_BYTES: u32 = 8; +pub const EVENT_BYTES: u32 = 8; + +/// A control request this driver sends, by what it is called. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Request { + PcmInfo, + SetParams, + Prepare, + Start, + Stop, +} + +impl Request { + pub fn name(self) -> &'static str { + match self { + Self::PcmInfo => "PCM_INFO", + Self::SetParams => "PCM_SET_PARAMS", + Self::Prepare => "PCM_PREPARE", + Self::Start => "PCM_START", + Self::Stop => "PCM_STOP", + } + } +} + +/// A status a device answered that was not `S_OK`, by its name. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Failed { + BadMsg, + NotSupp, + IoErr, +} + +impl Failed { + pub fn name(self) -> &'static str { + match self { + Self::BadMsg => "VIRTIO_SND_S_BAD_MSG", + Self::NotSupp => "VIRTIO_SND_S_NOT_SUPP", + Self::IoErr => "VIRTIO_SND_S_IO_ERR", + } + } +} + +/// A status word: `Ok(Ok(()))` for `S_OK`, `Ok(Err(_))` for one of the three +/// failures, and `Err(word)` for anything §5.14.6 does not define. +pub fn status(word: u32) -> Result, u32> { + match word { + S_OK => Ok(Ok(())), + S_BAD_MSG => Ok(Err(Failed::BadMsg)), + S_NOT_SUPP => Ok(Err(Failed::NotSupp)), + S_IO_ERR => Ok(Err(Failed::IoErr)), + other => Err(other), + } +} + +/// §5.14.6.1: `virtio_snd_query_info` asking for `count` PCM streams from +/// `start_id`, each answered as one [`PCM_INFO_BYTES`] structure. +pub fn pcm_info_query(start_id: u32, count: u32) -> [u32; 4] { + [R_PCM_INFO, start_id, count, PCM_INFO_BYTES] +} + +/// §5.14.6.6: `virtio_snd_pcm_hdr`, which is the whole of a PREPARE, START or +/// STOP. +pub fn pcm_hdr(code: u32, stream_id: u32) -> [u32; 2] { + [code, stream_id] +} + +/// §5.14.6.6.3: `virtio_snd_pcm_set_params`, no feature selected and the +/// padding byte 0 (§5.14.6.6.3.2). +pub fn set_params(stream_id: u32, params: Params) -> [u32; 6] { + let Params { buffer_bytes, period_bytes, channels, format, rate } = params; + [ + R_PCM_SET_PARAMS, + stream_id, + buffer_bytes, + period_bytes, + 0, + u32::from_le_bytes([channels, format, rate, 0]), + ] +} + +/// What a SET_PARAMS selects. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct Params { + pub buffer_bytes: u32, + pub period_bytes: u32, + pub channels: u8, + pub format: u8, + pub rate: u8, +} + +/// §5.14.6.6.2: one stream's `virtio_snd_pcm_info`, every field the device's. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct PcmInfo { + pub hda_fn_nid: u32, + pub features: u32, + pub formats: u64, + pub rates: u64, + pub direction: u8, + pub channels_min: u8, + pub channels_max: u8, +} + +impl PcmInfo { + /// The structure's eight words, as the device wrote them. + pub fn decode(words: [u32; 8]) -> Self { + let [direction, channels_min, channels_max, _] = words[6].to_le_bytes(); + Self { + hda_fn_nid: words[0], + features: words[1], + formats: words[2] as u64 | (words[3] as u64) << 32, + rates: words[4] as u64 | (words[5] as u64) << 32, + direction, + channels_min, + channels_max, + } + } +} + +/// §5.14.6: `virtio_snd_event`. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct Event { + pub code: u32, + pub data: u32, +} + +impl Event { + /// The event's name, or `None` for a type §5.14.6 does not define. + pub fn name(self) -> Option<&'static str> { + match self.code { + EVT_JACK_CONNECTED => Some("jack connected"), + EVT_JACK_DISCONNECTED => Some("jack disconnected"), + EVT_PCM_PERIOD_ELAPSED => Some("period elapsed"), + EVT_PCM_XRUN => Some("PCM XRUN"), + _ => None, + } + } +} diff --git a/userland/supervisor/src/main.rs b/userland/supervisor/src/main.rs index 62fcba2c79c..ea090d2e3a6 100644 --- a/userland/supervisor/src/main.rs +++ b/userland/supervisor/src/main.rs @@ -2032,9 +2032,7 @@ fn start<'a>( let request = DeviceRequest::parse(name) .unwrap_or_else(|| panic!("supervisor: `{name}` is not a device this ABI has")); // A device this machine does not have is not endowed, and the supervisor says - // which: "did I get an HDA or a virtio-sound?" becomes "which claims - // are in my endowment table?", which is the same question with the - // answer already in hand. + // which. // // The label is the manifest's own spelling, which is exactly what the // claimant looks the claim up by: one string, written once. From caf794e0cd02ee88bbd16f9b9790124a9cf12325 Mon Sep 17 00:00:00 2001 From: japabu Date: Sat, 10 Oct 2026 08:06:47 +0200 Subject: [PATCH 2/6] The census fixtures drop the sound source, and the virtio-sound crate answers clippy Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017cSFvbD35xJ2kGANVdm23C --- tests/checks.rs | 4 ++-- userland/soundserver/virtio-sound/src/lib.rs | 2 +- userland/soundserver/virtio-sound/src/tests.rs | 5 ++++- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/checks.rs b/tests/checks.rs index f48dc76f464..9905dc25759 100644 --- a/tests/checks.rs +++ b/tests/checks.rs @@ -488,7 +488,7 @@ mod checks { let xhci = if cpu == 0 { 40 } else { 0 }; format!( "[ {at} cpu{on} kernel] irq: cpu{cpu} timer=100 kick={kick} xhci={xhci} userdev=0 \ - sound=0 i8042=0 dmafault=0 hda=0 tlb={tlb} nmi=0 spurious=0 unclaimed=0\n" + i8042=0 dmafault=0 hda=0 tlb={tlb} nmi=0 spurious=0 unclaimed=0\n" ) }; let issued = |at: &str, on: u32, total: u64| { @@ -991,7 +991,7 @@ mod checks { assert!(judge(&[&readback("jobcase", &done(stopped), jobcase)]).is_err()); // What the seal itself writes of the machine, above the ring's tail. - let census = "| irq: cpu0 timer=9 kick=2 xhci=1729 userdev=0 sound=0 i8042=0 dmafault=0 hda=0 tlb=0 \ + let census = "| irq: cpu0 timer=9 kick=2 xhci=1729 userdev=0 i8042=0 dmafault=0 hda=0 tlb=0 \ nmi=0 spurious=0 unclaimed=0\n\ | tlb: shootdowns=4 wait=12us max=3us dlopen=0 pcid=0 mmio=0 unmap=4 pipe=0 staged=0 \ bench=0\n\ diff --git a/userland/soundserver/virtio-sound/src/lib.rs b/userland/soundserver/virtio-sound/src/lib.rs index 9f5dd3d8ac9..ec165c80037 100644 --- a/userland/soundserver/virtio-sound/src/lib.rs +++ b/userland/soundserver/virtio-sound/src/lib.rs @@ -97,7 +97,7 @@ const _: () = { assert!(PERIODS <= u32::BITS as usize); // §5.14.6.6.3.2: `buffer_bytes % period_bytes == 0`, and every period // whole 16-bit stereo frames. - assert!(PERIOD_BYTES % 4 == 0); + assert!(PERIOD_BYTES.is_multiple_of(4)); }; /// The rates this driver can encode, best first. 44100 leads because it is diff --git a/userland/soundserver/virtio-sound/src/tests.rs b/userland/soundserver/virtio-sound/src/tests.rs index c59774424dd..08a091d8631 100644 --- a/userland/soundserver/virtio-sound/src/tests.rs +++ b/userland/soundserver/virtio-sound/src/tests.rs @@ -128,6 +128,9 @@ fn words(bytes: &[u8]) -> Vec { bytes.chunks(4).map(|w| u32::from_le_bytes(w.try_into().unwrap())).collect() } +/// How the model answers each control request it reads. +type Answering = Box Answer>; + struct State { /// Per queue: the available entries taken so far, and the used ones /// written. @@ -140,7 +143,7 @@ struct State { /// Chains the device holds: the transmit queue's until played, the /// event queue's until an event. held: [Vec<(u16, Vec)>; 3], - answer: Box Answer>, + answer: Answering, } #[derive(Clone)] From 31fb80bb72bc768108546ffccb0b6927173598ed Mon Sep 17 00:00:00 2001 From: japabu Date: Sat, 10 Oct 2026 08:17:00 +0200 Subject: [PATCH 3/6] iommu_virtio_platform runs soundserver on netcase at 00:04.0, and its decline control gets a virtio-gpu the kernel still drives With the kernel's virtio-sound gone, virtio-no-access-platform reached no device on a Headless machine: every non-console virtio function there is a process's claim. HeadlessVirtioGpu adds one behind the unit, which the actuator declines and QEMU refuses FEATURES_OK for. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017cSFvbD35xJ2kGANVdm23C --- tests/common/iommu.rs | 17 +++++++++++------ tests/common/qemu.rs | 14 ++++++++++++++ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/tests/common/iommu.rs b/tests/common/iommu.rs index ed0a3fb8b3b..625f8b50354 100644 --- a/tests/common/iommu.rs +++ b/tests/common/iommu.rs @@ -178,10 +178,13 @@ pub fn iommu_virtio_platform(test_config: &Path) -> Result<(), String> { )); } eprintln!( - " [iommu] {name}: {} virtio function(s) behind a unit = {behind_unit}, the audio \ - function {sound} among them{}", + " [iommu] {name}: {} virtio function(s) negotiated, behind a unit = {behind_unit}; {}", negotiated.len(), - if behind_unit { "" } else { "; the NIC's and the audio function's claims refused for want of a unit" } + if behind_unit { + format!("the audio function {sound} among them") + } else { + format!("the NIC's and the audio function {sound}'s claims refused for want of a unit") + } ); } declining_is_not_free(test_config) @@ -204,7 +207,7 @@ const NVME_AT: &str = "00:02.0"; /// The slot QEMU's `-device` order puts the virtio-sound function on, the one /// `tests/netcase`'s soundserver row claims. -const SOUND_AT: &str = "00:05.0"; +const SOUND_AT: &str = "00:04.0"; /// Why a machine with no unit hands no function over, in the kernel's words. const NOT_REMAPPED: &str = "its interrupts would not be remapped on this machine"; @@ -251,14 +254,16 @@ fn no_unit_is_no_claim(log: &Serial) -> Result<(), String> { /// `virtio_validate_features` returns `-EFAULT` and `virtio_set_status` returns /// before it stores the status (`hw/virtio/virtio.c:2270-2276` and `:2292-2299` /// at v11.1.1), so `FEATURES_OK` never sticks. The actuator withholds the bit -/// from every virtio device but the console, and each of them is refused for it. +/// from every virtio device the kernel drives but the console — on this +/// machine the virtio-gpu it adds for that, since every other function is a +/// process's claim — and each of them is refused for it. fn declining_is_not_free(test_config: &Path) -> Result<(), String> { let qemu = QemuInstance::boot_with_options( test_config, &[], &[], BootOptions { - profile: Profile::Headless, + profile: Profile::HeadlessVirtioGpu, kernel_params: &["virtio-no-access-platform"], ..Default::default() }, diff --git a/tests/common/qemu.rs b/tests/common/qemu.rs index 6df69de4ce2..5f83972699e 100644 --- a/tests/common/qemu.rs +++ b/tests/common/qemu.rs @@ -694,6 +694,10 @@ pub enum Profile { /// disk is the only storage it has, and the only device its firmware can /// boot. HeadlessNoUsb, + /// [`Profile::Headless`] with a virtio-gpu behind the unit: a virtio + /// function the kernel still drives itself beside the console, which is + /// what `virtio-no-access-platform` withholds the bit from. + HeadlessVirtioGpu, /// M1 metal-sim: GOP, NVMe, xHCI with the boot stick on it, i8042 from /// q35, and nothing else -- no virtio device and no USB HID. This is the /// machine shape that gets flashed, so it is the one the input tests run @@ -738,6 +742,7 @@ impl Profile { | Self::HeadlessNoIommu | Self::HeadlessE1000e | Self::HeadlessNoUsb + | Self::HeadlessVirtioGpu | Self::Metal => Arch::X86_64, } } @@ -861,6 +866,8 @@ struct Shape { /// RNDR for firmware or the kernel to draw from; a q35's firmware answers /// the protocol from RDRAND without one. rng: bool, + /// A virtio-gpu, which the kernel drives. + virtio_gpu: bool, } /// Where a machine's image and its DATA are. A size is stated because a @@ -917,6 +924,7 @@ impl Profile { storage: Storage::Stick { nvme_bytes: 0 }, iommu: None, rng: true, + virtio_gpu: false, }, Self::Headless => Shape { vga: "none", @@ -928,6 +936,7 @@ impl Profile { storage: Storage::Disk { data_bytes: NVME_SMALL }, iommu: Some(IOMMU_DEFAULT), rng: false, + virtio_gpu: false, }, Self::Metal => Shape { vga: "std", @@ -943,10 +952,12 @@ impl Profile { storage: Storage::Stick { nvme_bytes: NVME_SMALL }, iommu: Some(IOMMU_DEFAULT), rng: false, + virtio_gpu: false, }, Self::HeadlessNoIommu => Shape { iommu: None, ..Self::Headless.shape() }, Self::HeadlessE1000e => Shape { nic: Nic::E1000e, ..Self::Headless.shape() }, Self::HeadlessNoUsb => Shape { xhci: &[], usb: &[], ..Self::Headless.shape() }, + Self::HeadlessVirtioGpu => Shape { virtio_gpu: true, ..Self::Headless.shape() }, } } @@ -2184,6 +2195,9 @@ fn qemu_command( if shape.rng { qemu.arg("-device").arg("virtio-rng-pci"); } + if shape.virtio_gpu { + qemu.arg("-device").arg(format!("virtio-gpu-pci{platform}")); + } // The NIC before the virtio block, so a profile that has one and not the // other still creates it after the unit and before everything else. From ddb9068a11e67ee199ccceed19699ab932fbb42a Mon Sep 17 00:00:00 2001 From: japabu Date: Sat, 10 Oct 2026 08:59:12 +0200 Subject: [PATCH 4/6] A claim's record carries when its messages landed; virtio drivers share one claim crate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 1 of #822. The kernel stamps each claimed function's interrupt record: DeviceIrqRecord grows `first_nanos`, when the first message a read counts landed, and `last_nanos`, never older than the newest it counts. The count shares one word with the number of reads that took one, so which read a message falls to and whether it is that read's first are one compare-exchange; the first message of read n stamps the slot of n's parity, so a message landing while a reader is between its exchange and its loads stamps the next read and never this one. `kernel/loom/tests/device_irq.rs` gains `a_reads_times_are_its_own_messages`, and `device-irq-lossy` still reds `every_message_is_counted_once`. soundserver's virtio-sound path is clocked by device time again: the wake's lateness splits at the first message's landing, as it split at the oldest record's ISR time when the kernel drove the device, and the DLL takes the newest message's landing as the last period's, which is dll.rs's own contract for a folded batch. The HDA backend hands its record's one timestamp as both, so the HDA path computes what it computed before. The backends answer one completion or none, which both already did. `userland/toyos-pci-claim` is the one home of what netstack and soundserver both copied: Bar and Grant as toyos-device-memory's boundary over the SDK's Window, KernelRefused, and `virtio::negotiate` — the claim's description, the capability walk, the BAR, `Offer::acknowledge` and `accept` — with the feature line each driver says under its own name. netstack's `device.rs` goes; its Latch moves into i219.rs, its one user. netstack's `config_space_is_bounded` goes: `vendor_caps` follows `u8` links masked to dword alignment and cannot read past 0x10F whatever the claim bounds, and the bound itself is `toyos_dma::register`'s, host-tested, and a `Register` witness `pcidev::config_read` takes. `iommu_virtio_platform` stops reading its line. `virtio_sound_counts` was already red with the transmit queue on NO_VECTOR: with no client soundserver's wait has no timeout, so the periods left in flight never come back and it never suspends; the job's doc says so. The format strings the test broke across raw newlines are escapes. The defensive status zeroing in `toyos-virtio-sound`'s submit goes. The decline control's dependence on virtio-gpu is recorded in the every-driver track's stage 1 with its exit, and the lending issue no longer cites virtio-sound's pool as present. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017cSFvbD35xJ2kGANVdm23C --- Cargo.lock | 13 +- Cargo.toml | 1 + issues/every-driver-is-still-in-the-kernel.md | 10 + ...views-still-rest-on-a-hand-layout-claim.md | 2 +- ...-region-may-be-lent-is-a-runtime-option.md | 12 +- kernel/loom/tests/device_irq.rs | 85 ++++++-- kernel/src/isa.rs | 4 +- kernel/src/pcidev/mod.rs | 4 +- kernel/src/pcidev/record.rs | 145 +++++++++---- tests/common/iommu.rs | 12 - tests/toyos-rust-tests/src/bin/isa_lines.rs | 2 +- .../src/bin/virtio_sound_counts.rs | 4 +- tests/toyos.rs | 22 +- toyos-abi/src/pci.rs | 18 +- toyos/src/device.rs | 4 +- userland/netstack/Cargo.toml | 1 + userland/netstack/src/i219.rs | 55 ++--- userland/netstack/src/main.rs | 1 - userland/netstack/src/virtio_net.rs | 132 +---------- userland/soundserver/Cargo.toml | 2 +- userland/soundserver/src/backend.rs | 35 ++- userland/soundserver/src/mix.rs | 100 ++++----- userland/soundserver/src/virtio.rs | 205 +++++------------- userland/soundserver/virtio-sound/src/lib.rs | 2 - userland/toyos-pci-claim/Cargo.toml | 16 ++ .../device.rs => toyos-pci-claim/src/lib.rs} | 101 ++++----- userland/toyos-pci-claim/src/virtio.rs | 117 ++++++++++ 27 files changed, 571 insertions(+), 534 deletions(-) create mode 100644 userland/toyos-pci-claim/Cargo.toml rename userland/{netstack/src/device.rs => toyos-pci-claim/src/lib.rs} (52%) create mode 100644 userland/toyos-pci-claim/src/virtio.rs diff --git a/Cargo.lock b/Cargo.lock index abbad80a19a..1c6770511d3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3370,6 +3370,7 @@ dependencies = [ "toyos-net-tcp", "toyos-net-udp", "toyos-net-wire", + "toyos-pci-claim", "toyos-tco", "toyos-virtio", ] @@ -5272,10 +5273,10 @@ dependencies = [ "rubato", "toyos", "toyos-abi", - "toyos-device-memory", "toyos-hda", "toyos-inspect", "toyos-mixer", + "toyos-pci-claim", "toyos-virtio", "toyos-virtio-sound", ] @@ -5994,6 +5995,16 @@ dependencies = [ "toyos-abi", ] +[[package]] +name = "toyos-pci-claim" +version = "0.1.0" +dependencies = [ + "toyos", + "toyos-abi", + "toyos-device-memory", + "toyos-virtio", +] + [[package]] name = "toyos-phys" version = "0.1.0" diff --git a/Cargo.toml b/Cargo.toml index 0ce8b1dbee6..0d1808ed6a4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -115,6 +115,7 @@ members = [ "userland/test-runner", "userland/toybox", "userland/toyos-font", + "userland/toyos-pci-claim", "userland/toyos-window", "userland/trace", "userland/update", diff --git a/issues/every-driver-is-still-in-the-kernel.md b/issues/every-driver-is-still-in-the-kernel.md index 31404554d85..7045934dab1 100644 --- a/issues/every-driver-is-still-in-the-kernel.md +++ b/issues/every-driver-is-still-in-the-kernel.md @@ -42,6 +42,16 @@ What is left of the staged work: the device only reads, whose bound is 0, what QEMU's device reports as `len` on such a queue is measured: the NIC's transmit queue is the only one read so far. + `iommu_virtio_platform`'s decline control, `declining_is_not_free` + (`tests/common/iommu.rs`), declines through the kernel's + `virtio-no-access-platform` actuator, which reaches only a virtio function + the kernel drives; with the NIC and virtio-sound in processes that is the + virtio-gpu `Profile::HeadlessVirtioGpu` adds for it, and virtio-gpu leaving + leaves the control nothing to decline. Exit for the control, in the diff + that moves virtio-gpu out: the decline is made where the features are + negotiated, `toyos-virtio`'s `Offer::accept`, for a function a process + drives, and the control reds when that function keeps `FEATURES_OK`; + `Profile::HeadlessVirtioGpu` goes in the same diff. 2. Done: **BAR sizing and re-assignment onto 2 MiB boundaries** is `pcidev::place_bar`, with the overlap refusal kept as the assertion that it worked rather than as the mechanism. diff --git a/issues/three-kernel-byte-views-still-rest-on-a-hand-layout-claim.md b/issues/three-kernel-byte-views-still-rest-on-a-hand-layout-claim.md index 0c4f3f609fa..f531b782ebb 100644 --- a/issues/three-kernel-byte-views-still-rest-on-a-hand-layout-claim.md +++ b/issues/three-kernel-byte-views-still-rest-on-a-hand-layout-claim.md @@ -16,7 +16,7 @@ user memory through an `unsafe` `from_raw_parts` of the kernel's own whose |---|---|---| | `DmaGrant` | `grant_bytes`, `kernel/src/syscall/device.rs` | `size_of == 4 + 4 + 8 + 8`, a sum written by hand | | `DmaMapping` | inline in `sys_device_dma_map`, the same file | `size_of == 8 + 8`, a sum written by hand | -| `DeviceIrqRecord` | `record_bytes`, `kernel/src/object/ops.rs` | `SIZE == 4`, a total written by hand; the struct is one `u32` | +| `DeviceIrqRecord` | `record_bytes`, `kernel/src/object/ops.rs` | `SIZE == 4 + 4 + 8 + 8`, a sum written by hand | Read, nothing run: none of the three has padding today. Each assertion compares the struct's size with a number a person wrote, and nothing ties diff --git a/issues/whether-a-region-may-be-lent-is-a-runtime-option.md b/issues/whether-a-region-may-be-lent-is-a-runtime-option.md index 37cd278d586..a0e3bc40a8e 100644 --- a/issues/whether-a-region-may-be-lent-is-a-runtime-option.md +++ b/issues/whether-a-region-may-be-lent-is-a-runtime-option.md @@ -11,10 +11,14 @@ may be lent a region by reading `Region::pages` — `Some` for pages the kernel allocated for the region, `None` for an aperture, firmware's framebuffer, or a pool a kernel driver owns — together with its cache policy. A new region kind that fills the field the wrong way is lendable, or refused, with no compile -error; the only check is `blockd_lends_within_its_bound`'s refusal of -virtio-sound's pool. And the field's name says nothing about lending: the -virtio-gpu scanout and cursor carry `Some` and are lendable today, which is the -same authority their holder already has but was decided by nobody. +error. The one test written for it, `blockd_lends_within_its_bound`, refused +virtio-sound's pool, which left the kernel with its driver; the test is out of +the guest suite and staged as a T14 row in +`issues/the-guest-suite-runs-only-what-no-cheaper-tier-reaches.md`'s stage J, +where the kernel driver's pool it can refuse is HDA's. And the field's name +says nothing about lending: the virtio-gpu scanout and cursor carry `Some` and +are lendable today, which is the same authority their holder already has but +was decided by nobody. **Exit condition.** A region's kind is a type — owned pages, a kernel driver's pool, an aperture — and `dma_map` takes only the owned-pages kind, so a region diff --git a/kernel/loom/tests/device_irq.rs b/kernel/loom/tests/device_irq.rs index e2c42d979bf..b098aebdb6a 100644 --- a/kernel/loom/tests/device_irq.rs +++ b/kernel/loom/tests/device_irq.rs @@ -2,15 +2,13 @@ //! //! The kernel programs one MSI-X vector per claimed function and accumulates //! what arrives into a record its holder reads through a syscall. So there are -//! two parties on two CPUs and one word between them: an ISR that bumps a -//! count, and a reader that takes it. +//! two parties on two CPUs: an ISR that counts a message and stamps when it +//! landed, and a reader that takes the count and its times. //! -//! **The invariant is that every message is counted exactly once.** Nothing -//! here orders anything else — the word is the whole of what it says, so the -//! orderings are `Relaxed` and the property is an interleaving. What makes it -//! hold is that the taking side is a read-modify-write: a reader that loaded a +//! **The invariant is that every message is counted exactly once, and a +//! read's times are its own messages'.** What makes the first hold is that +//! both sides change the count by a read-modify-write: a reader that loaded a //! count and then cleared it drops every message the ISR recorded in between. -//! //! That is the record's whole design, so the negative control is it turned off //! — a cargo feature rather than a comment: //! @@ -19,8 +17,8 @@ //! --test device_irq //! ``` //! -//! makes the `swap` a load and a store and the ISR's `fetch_add` a load, an -//! add and a store, and [`every_message_is_counted_once`] must red. +//! makes every compare-exchange a load and a store, and +//! [`every_message_is_counted_once`] must red. use kernel_loom::device_irq::Interrupt; use loom::sync::Arc; @@ -29,8 +27,8 @@ use loom::sync::Arc; /// /// Two messages against one reader that may run at any point between them: what /// the reader took plus what is left is exactly what arrived. A `store` where -/// the count has a `fetch_add` fails this, and so does a reader that loads and -/// then clears instead of swapping. +/// the count has a compare-exchange fails this, and so does a reader that +/// loads and then clears instead of exchanging. #[test] fn every_message_is_counted_once() { loom::model(|| { @@ -39,14 +37,14 @@ fn every_message_is_counted_once() { let isr = { let irq = irq.clone(); loom::thread::spawn(move || { - irq.took(); - irq.took(); + irq.took(1); + irq.took(2); }) }; - let taken = irq.take().unwrap_or(0); + let taken = irq.take().map_or(0, |record| record.count); isr.join().unwrap(); - let left = irq.take().unwrap_or(0); + let left = irq.take().map_or(0, |record| record.count); assert_eq!( taken + left, @@ -57,6 +55,58 @@ fn every_message_is_counted_once() { }); } +/// A read's first time is the landing of the first message it counts, and its +/// newest time no older than the newest it counts. +/// +/// Three messages, landing at 1, 2 and 3, against a reader that takes once at +/// any point among them and then takes what is left: whatever the split, each +/// read's `first_nanos` is its own first message's, and a message that lands +/// while the reader is between its exchange and its loads stamps the next +/// read and never this one. A single slot for both reads' first times fails +/// this, and so does a stamp made after the exchange that counts it. +#[test] +fn a_reads_times_are_its_own_messages() { + loom::model(|| { + let irq = Arc::new(Interrupt::new()); + + let isr = { + let irq = irq.clone(); + loom::thread::spawn(move || { + for at in 1..=3 { + irq.took(at); + } + }) + }; + + let taken = irq.take(); + isr.join().unwrap(); + let left = irq.take(); + + let counted = taken.map_or(0, |record| record.count); + if let Some(record) = taken { + assert_eq!(record.first_nanos, 1, "the first read counted from message 1 and was told {}", record.first_nanos); + assert!( + record.last_nanos >= u64::from(counted), + "a read that counted {counted} message(s) was told the newest landed at {}", + record.last_nanos + ); + } + match left { + None => assert_eq!(counted, 3, "the first read counted {counted} and nothing was left"), + Some(left) => { + assert_eq!( + left.first_nanos, + u64::from(counted) + 1, + "the second read counted from message {} and was told {}", + counted + 1, + left.first_nanos + ); + assert_eq!(left.last_nanos, 3, "the newest message landed at 3 and the last read was told {}", left.last_nanos); + } + } + }); +} + /// A reader that finds nothing answers nothing, and leaves nothing behind. /// /// Single-threaded and deliberately so: this is the answer on the path a @@ -68,9 +118,10 @@ fn an_idle_record_answers_nothing() { let irq = Interrupt::new(); assert!(irq.take().is_none(), "an untouched record answered a reader"); assert!(!irq.armed(), "an untouched record read ready"); - irq.took(); + irq.took(7); assert!(irq.armed(), "a record with a message in it read empty"); - assert_eq!(irq.take(), Some(1)); + let record = irq.take().expect("a record with a message in it answered nothing"); + assert_eq!((record.count, record.first_nanos, record.last_nanos), (1, 7, 7)); assert!(irq.take().is_none(), "the same message was answered twice"); assert!(!irq.armed(), "a drained record still reads ready"); }); diff --git a/kernel/src/isa.rs b/kernel/src/isa.rs index 146b37dbc37..e270962c1d3 100644 --- a/kernel/src/isa.rs +++ b/kernel/src/isa.rs @@ -245,7 +245,7 @@ pub fn take_record(row: &Row) -> Option { if taken.is_some() && IRQ[row].take_unannounced() { log!("isa: {} took its first interrupt", function(row).expect("a claimed row was filled").name); } - taken.map(|count| DeviceIrqRecord { count }) + taken } pub fn has_irq(row: &Row) -> bool { @@ -256,7 +256,7 @@ pub fn has_irq(row: &Row) -> bool { /// watch. Called from the row's handler, so it takes no lock but the watch's /// own and the I/O APIC's masked one, and allocates nothing. pub fn isr(row: usize) { - IRQ[row].took(); + IRQ[row].took(crate::clock::nanos_since_boot()); for &line in lines(row) { if pio::level(line) { pio::set_masked(line, true); diff --git a/kernel/src/pcidev/mod.rs b/kernel/src/pcidev/mod.rs index 0b5487fa7e2..3a73f50847b 100644 --- a/kernel/src/pcidev/mod.rs +++ b/kernel/src/pcidev/mod.rs @@ -1739,7 +1739,7 @@ pub fn take_record(binding: &Binding) -> Result, Syscall if taken.is_some() && IRQ[slot].take_unannounced() { log!("pcidev: slot {slot} took its first message on vector {:#x}", VECTORS[slot]); } - Ok(taken.map(|count| DeviceIrqRecord { count })) + Ok(taken) } /// Whether a read of the claim answers at once: a message is waiting, or the @@ -1752,7 +1752,7 @@ pub fn has_irq(binding: &Binding) -> bool { /// handler, so it allocates nothing; `record.rs` owns the counting, and /// `kernel-loom` models it against a concurrent reader. pub fn isr(slot: usize) { - IRQ[slot].took(); + IRQ[slot].took(crate::clock::nanos_since_boot()); WATCHES[slot].post_in_place(); } diff --git a/kernel/src/pcidev/record.rs b/kernel/src/pcidev/record.rs index 72ff55f33b7..8ba27c7b569 100644 --- a/kernel/src/pcidev/record.rs +++ b/kernel/src/pcidev/record.rs @@ -6,57 +6,77 @@ //! not an ordering, and no guest test in this suite lands on it. //! //! **Two parties race**: the ISR, on whichever CPU the unit routed the message -//! to, and the holder reading its record through a syscall on any CPU. +//! to, and the holder reading its record through a syscall on any CPU. One +//! handler at a time: a slot has one vector, delivered to one CPU. //! //! **The invariant is that every message is counted exactly once.** The count //! is a read-modify-write on both sides and never a load followed by a store: //! a reader that loaded a count and then cleared it drops every message the ISR //! recorded in between, and a driver that misses one waits for a device that -//! has already spoken. No ordering carries anything across these words — each -//! is the whole of what it says — so the orderings here are `Relaxed` and the -//! model is about the interleaving; the holder reads them after the wake the -//! claim's watch post owes it, which orders them. +//! has already spoken. +//! +//! **And a read's times are its own messages'.** The count shares its word +//! with the number of reads that have taken one, so which read a message falls +//! to and whether it is that read's first are one compare-exchange. The first +//! message of read `n` stamps the slot of `n`'s parity, before the exchange +//! that counts it publishes the stamp (`Release`, taken by the read's +//! `Acquire`), and read `n + 1`'s first message stamps the other slot: one +//! landing while a reader is between its exchange and its load never +//! overwrites the time that reader is about to load. A second reader racing +//! the first can take read `n + 1` and let read `n + 2`'s first message into +//! the slot the first is loading; that costs the racing holder its own time, +//! never a count. #[cfg(not(feature = "loom"))] -use core::sync::atomic::{AtomicBool, AtomicU32, Ordering}; +use core::sync::atomic::{AtomicBool, AtomicU64, Ordering}; #[cfg(feature = "loom")] -use loom::sync::atomic::{AtomicBool, AtomicU32, Ordering}; +use loom::sync::atomic::{AtomicBool, AtomicU64, Ordering}; + +use toyos_abi::pci::DeviceIrqRecord; -/// Every word here is the whole of what it says and orders nothing else, so -/// the only property is the interleaving — which `device-irq-lossy` is the -/// control for and `kernel-loom` is the model of. +/// The words that say only what they hold: the latch, the fault and the +/// newest stamp, whose reader takes it after the exchange that orders it. const ORDER: Ordering = Ordering::Relaxed; -/// The negative control: the read-modify-writes become a load and a store, -/// which is the whole of what this record's design is. Never on in a kernel -/// build. +/// The count is the word's low half and the reads that took one its high half. +const COUNT: u64 = 0xFFFF_FFFF; +const READ: u64 = 1 << 32; + +/// The negative control: each compare-exchange on the word becomes a load and +/// a store, which is the whole of what this record's design is. Never on in a +/// kernel build. #[cfg(feature = "device-irq-lossy")] -macro_rules! take_word { - ($word:expr, $empty:expr) => {{ - let held = $word.load(ORDER); - $word.store($empty, ORDER); - held +macro_rules! exchange { + ($word:expr, $held:expr, $new:expr, $success:expr) => {{ + let seen = $word.load(ORDER); + if seen == $held { + $word.store($new, ORDER); + Ok(seen) + } else { + Err(seen) + } }}; } #[cfg(not(feature = "device-irq-lossy"))] -macro_rules! take_word { - ($word:expr, $empty:expr) => { - $word.swap($empty, ORDER) +macro_rules! exchange { + ($word:expr, $held:expr, $new:expr, $success:expr) => { + $word.compare_exchange_weak($held, $new, $success, ORDER) }; } #[cfg(feature = "device-irq-lossy")] -macro_rules! bump { - ($word:expr) => {{ +macro_rules! take_word { + ($word:expr, $empty:expr) => {{ let held = $word.load(ORDER); - $word.store(held.wrapping_add(1), ORDER); + $word.store($empty, ORDER); + held }}; } #[cfg(not(feature = "device-irq-lossy"))] -macro_rules! bump { - ($word:expr) => { - $word.fetch_add(1, ORDER) +macro_rules! take_word { + ($word:expr, $empty:expr) => { + $word.swap($empty, ORDER) }; } @@ -64,8 +84,12 @@ macro_rules! bump { /// /// Atomics only: the handler allocates nothing. pub struct Interrupt { - /// Messages since the holder's last read. - count: AtomicU32, + /// Messages since the holder's last read, under the reads that took one. + word: AtomicU64, + /// When each read's first message landed, by the read's parity. + first: [AtomicU64; 2], + /// When the newest message landed. + last: AtomicU64, /// The unit refused this function an access. Every call the claim answers /// refuses from here on: its bus mastering is gone, so a driver that kept /// going would be driving nothing. @@ -84,7 +108,9 @@ impl Interrupt { #[cfg(not(feature = "loom"))] pub const fn new() -> Self { Self { - count: AtomicU32::new(0), + word: AtomicU64::new(0), + first: [AtomicU64::new(0), AtomicU64::new(0)], + last: AtomicU64::new(0), faulted: AtomicBool::new(false), unannounced: AtomicBool::new(true), } @@ -95,36 +121,63 @@ impl Interrupt { #[cfg(feature = "loom")] pub fn new() -> Self { Self { - count: AtomicU32::new(0), + word: AtomicU64::new(0), + first: [AtomicU64::new(0), AtomicU64::new(0)], + last: AtomicU64::new(0), faulted: AtomicBool::new(false), unannounced: AtomicBool::new(true), } } - /// Record one message. Called from the vector's ISR, so it takes no lock - /// and allocates nothing. + /// Record one message, which landed at `now`. Called from the vector's + /// ISR, so it takes no lock and allocates nothing. /// - /// `fetch_add` and not a store: the reader may take the count between any - /// two of these, and what it took plus what is left has to be what arrived. - pub fn took(&self) { - bump!(self.count); + /// A compare-exchange and not a store: the reader may take the count + /// between any two of these, and what it took plus what is left has to be + /// what arrived. + pub fn took(&self, now: u64) { + self.last.store(now, ORDER); + let mut word = self.word.load(ORDER); + loop { + if word & COUNT == 0 { + self.first[(word / READ) as usize & 1].store(now, ORDER); + } + match exchange!(self.word, word, word.wrapping_add(1), Ordering::Release) { + Ok(_) => return, + Err(seen) => word = seen, + } + } } - /// The messages since the last read, or `None` for none. + /// The messages since the last read and when they landed, or `None` for + /// none. /// - /// `swap` and not a load followed by a store: a message the ISR records - /// between the two would be cleared without ever having been counted. - pub fn take(&self) -> Option { - match take_word!(self.count, 0) { - 0 => None, - count => Some(count), + /// A compare-exchange and not a load followed by a store: a message the + /// ISR records between the two would be cleared without ever having been + /// counted. + pub fn take(&self) -> Option { + let mut word = self.word.load(ORDER); + loop { + if word & COUNT == 0 { + return None; + } + match exchange!(self.word, word, (word / READ).wrapping_add(1) * READ, Ordering::Acquire) { + Ok(_) => break, + Err(seen) => word = seen, + } } + Some(DeviceIrqRecord { + count: (word & COUNT) as u32, + _pad: 0, + first_nanos: self.first[(word / READ) as usize & 1].load(ORDER), + last_nanos: self.last.load(ORDER), + }) } /// Whether a message is waiting, for a readiness check that consumes /// nothing. pub fn armed(&self) -> bool { - self.count.load(ORDER) != 0 + self.word.load(ORDER) & COUNT != 0 } /// Whether this is the first message this slot has taken. Answers `true` @@ -149,7 +202,7 @@ impl Interrupt { /// Back to the state a fresh slot is in, for a claim being minted or given /// up. The holder is not running at either point. pub fn clear(&self) { - self.count.store(0, ORDER); + self.word.store(0, ORDER); self.faulted.store(false, ORDER); self.unannounced.store(true, ORDER); } diff --git a/tests/common/iommu.rs b/tests/common/iommu.rs index 625f8b50354..07915f7fa4f 100644 --- a/tests/common/iommu.rs +++ b/tests/common/iommu.rs @@ -74,7 +74,6 @@ pub fn iommu_virtio_platform(test_config: &Path) -> Result<(), String> { let mut qemu = QemuInstance::boot_with_options(&netcase(), &[], &[], options); let said: Vec = if behind_unit { vec![ - CLAIM_BOUNDED.to_string(), NETSTACK_NEGOTIATED.to_string(), SOUNDSERVER_NEGOTIATED.to_string(), bar_moved(), @@ -111,13 +110,6 @@ pub fn iommu_virtio_platform(test_config: &Path) -> Result<(), String> { // unit has three negotiators, one of them across the boundary, and the // arm without one has two and a refusal. let expected = if behind_unit { - // And the claim netstack 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. - // netstack 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(CLAIM_BOUNDED)?; // The two things a hand-over spends, on the same function and the // same machine the arm below requires to be unspent. Without this // pair those `must_not_say`s would pass against a kernel that had @@ -190,10 +182,6 @@ pub fn iommu_virtio_platform(test_config: &Path) -> Result<(), String> { declining_is_not_free(test_config) } -/// netstack's, once its claim answers nothing outside its own function. -const CLAIM_BOUNDED: &str = - "netstack: this claim answers 4096 bytes of configuration space and refuses every access \ - outside them"; /// netstack's feature line, the kernel's shape under netstack's name. const NETSTACK_NEGOTIATED: &str = "netstack: VirtIO: PCI "; const DISKSERVER_REFUSED: &str = diff --git a/tests/toyos-rust-tests/src/bin/isa_lines.rs b/tests/toyos-rust-tests/src/bin/isa_lines.rs index f35033a7084..7eac42c5106 100644 --- a/tests/toyos-rust-tests/src/bin/isa_lines.rs +++ b/tests/toyos-rust-tests/src/bin/isa_lines.rs @@ -153,7 +153,7 @@ fn record(claim: &Device) -> Option { match syscall::read_nonblock(claim.as_handle(), &mut record) { Ok(n) => { assert_eq!(n, DeviceIrqRecord::SIZE, "isa: a record of {n} bytes"); - Some(u32::from_ne_bytes(record)) + Some(u32::from_ne_bytes([record[0], record[1], record[2], record[3]])) } Err(SyscallError::WouldBlock) => None, Err(other) => panic!("isa: a bound claim's read answered {other:?}"), diff --git a/tests/toyos-rust-tests/src/bin/virtio_sound_counts.rs b/tests/toyos-rust-tests/src/bin/virtio_sound_counts.rs index fc874b34d58..d0072d7d52e 100644 --- a/tests/toyos-rust-tests/src/bin/virtio_sound_counts.rs +++ b/tests/toyos-rust-tests/src/bin/virtio_sound_counts.rs @@ -5,7 +5,9 @@ //! soundserver suspends only once every period it submitted has come back from //! the device, so a stream that ends with soundserver suspended is one whose //! every submitted period completed; and the periods it submitted cover at -//! least the periods this client filled. +//! least the periods this client filled. With no client its wait has no +//! timeout, so the suspend is reached only through the claim's interrupts: a +//! transmit queue with no vector leaves it `running`. use std::sync::mpsc; use std::time::{Duration, Instant}; diff --git a/tests/toyos.rs b/tests/toyos.rs index 43c501e985c..873dc036e5d 100644 --- a/tests/toyos.rs +++ b/tests/toyos.rs @@ -3597,8 +3597,7 @@ fn virtio_sound_counts(test_config: &Path) -> Result<(), String> { QemuInstance::boot_with_options(test_config, &[], &[(JOB.to_string(), bin)], BootOptions::default()); let mut console = qemu.boot_log().to_string(); await_guest(&mut qemu, &mut console, "soundserver's bring-up", |c| BROUGHT_UP.iter().all(|l| c.contains(l))) - .map_err(|e| format!("{e} -{console}"))?; + .map_err(|e| format!("{e}\n{console}"))?; let boot = serial::Serial::named("the boot", console); boot.must_be_clean()?; boot.must_say("soundserver: VirtIO: PCI ")?; @@ -3607,33 +3606,26 @@ fn virtio_sound_counts(test_config: &Path) -> Result<(), String> { let result = qemu.run_test(&format!("test_rs_{JOB}"), Duration::from_secs(120)); if let Some(why) = &result.error { - return Err(format!("{why} -the job said: -{}", result.stdout)); + return Err(format!("{why}\nthe job said:\n{}", result.stdout)); } if result.exit_code != Some(0) { - return Err(format!("the job ended {:?}: -{}", result.exit_code, result.stdout)); + return Err(format!("the job ended {:?}:\n{}", result.exit_code, result.stdout)); } let said = &result.stdout; let Some(counted) = said.lines().find(|l| l.contains("virtio_sound_counts: ")) else { - return Err(format!("the job never said what it counted: -{said}")); + return Err(format!("the job never said what it counted:\n{said}")); }; if said.contains("cannot be driven on") { - return Err(format!("soundserver refused its device mid-stream: -{said}")); + return Err(format!("soundserver refused its device mid-stream:\n{said}")); } let mut from = 0; for line in IN_ORDER { let times = said.matches(line).count(); let Some(at) = said[from..].find(line) else { - return Err(format!("`{line}` is not after the line before it in the window: -{said}")); + return Err(format!("`{line}` is not after the line before it in the window:\n{said}")); }; if times != 1 { - return Err(format!("`{line}` said {times} times in one stream's window: -{said}")); + return Err(format!("`{line}` said {times} times in one stream's window:\n{said}")); } from += at + line.len(); } diff --git a/toyos-abi/src/pci.rs b/toyos-abi/src/pci.rs index 3323e5a0e5c..2e22bb2965c 100644 --- a/toyos-abi/src/pci.rs +++ b/toyos-abi/src/pci.rs @@ -86,18 +86,28 @@ const _: () = assert!(core::mem::size_of::() == 8 + 8); /// One record and not a queue: the kernel accumulates, so a driver that slept /// through several is told about all of them at once and there is no ring for a /// slow reader to overflow. The count says how many messages arrived, never -/// what any of them meant — and it is the whole record, because what a message -/// meant is in the device's own rings and a driver that wanted a time has -/// its clock page (`crate::clock`). +/// what any of them meant: that is in the device's own rings. +/// +/// **The two times are when messages landed, which a driver reading its rings +/// later cannot recover**: a driver clocked by its device — soundserver's DLL — +/// is clocked by these and not by when it woke. Both are nanoseconds since +/// boot on the clock page's clock (`crate::clock`). #[repr(C)] #[derive(Clone, Copy)] pub struct DeviceIrqRecord { /// Never 0 in a record that was answered; an empty count is `WouldBlock`. pub count: u32, + pub _pad: u32, + /// When the first message `count` holds landed. + pub first_nanos: u64, + /// When the newest landed: never older than the newest message `count` + /// holds, and newer only by one that lands as the read is made, which the + /// next read counts. + pub last_nanos: u64, } impl DeviceIrqRecord { pub const SIZE: usize = core::mem::size_of::(); } -const _: () = assert!(DeviceIrqRecord::SIZE == 4); +const _: () = assert!(DeviceIrqRecord::SIZE == 4 + 4 + 8 + 8); diff --git a/toyos/src/device.rs b/toyos/src/device.rs index de9d15b65e5..c34783084a7 100644 --- a/toyos/src/device.rs +++ b/toyos/src/device.rs @@ -178,9 +178,9 @@ impl PciDev { /// A claim's interrupts since its last read, or `Err(WouldBlock)` for none. fn irq_record(dev: &Device) -> Result { - let mut record = toyos_abi::pci::DeviceIrqRecord { count: 0 }; + let mut record = toyos_abi::pci::DeviceIrqRecord { count: 0, _pad: 0, first_nanos: 0, last_nanos: 0 }; // SAFETY: the slice covers exactly the record being filled, and every - // bit pattern of its one integer field is a valid one. + // bit pattern of each of its integer fields is a valid one. let buf = unsafe { core::slice::from_raw_parts_mut(&mut record as *mut _ as *mut u8, toyos_abi::pci::DeviceIrqRecord::SIZE) }; diff --git a/userland/netstack/Cargo.toml b/userland/netstack/Cargo.toml index 498ad8ba171..0dd3297872f 100644 --- a/userland/netstack/Cargo.toml +++ b/userland/netstack/Cargo.toml @@ -10,6 +10,7 @@ toyos = { path = "../../toyos" } toyos-device-memory = { path = "../../toyos-device-memory" } toyos-i219 = { path = "../../toyos-i219" } toyos-virtio = { path = "../../toyos-virtio" } +toyos-pci-claim = { path = "../toyos-pci-claim" } toyos-tco = { path = "../../toyos-tco" } toyos-mdns = { path = "mdns" } toyos-dns = { path = "../../toyos-dns" } diff --git a/userland/netstack/src/i219.rs b/userland/netstack/src/i219.rs index f1ebc450c62..a71fbc5a289 100644 --- a/userland/netstack/src/i219.rs +++ b/userland/netstack/src/i219.rs @@ -1,6 +1,6 @@ //! The I219, as netstack drives it: the clock and the claim the driver's logic -//! is written against beside `device.rs`'s register window and grant, and -//! nothing else. +//! is written against beside `toyos-pci-claim`'s register window and grant, +//! and nothing else. //! //! What the kernel keeps is the *claim*: config space, the interrupt vector it //! programmed into this function, and the address space the function @@ -15,7 +15,7 @@ //! descriptor, which is what lets the driver's logic live in a crate that //! forbids `unsafe` at all. -use std::cell::RefCell; +use std::cell::{Cell, RefCell}; use std::rc::Rc; use toyos::shm::SharedMemory; @@ -24,8 +24,7 @@ use toyos::{DmaRegion, PciDev}; use toyos_abi::syscall::SyscallError; use toyos_device_memory::Registers; use toyos_i219::{Clock, Interrupts}; - -use crate::device::{Bar, Grant, KernelRefused, Latch}; +use toyos_pci_claim::{Bar, Grant, KernelRefused}; /// The machine's monotonic clock: one syscall, which the kernel serves from an /// anchor plus the timestamp counter — and the thread's own sleep, which is how @@ -113,21 +112,14 @@ fn registers(dev: &PciDev) -> Result<(Bar, SharedMemory), Opening> { .find(|(_, bytes)| **bytes >= toyos_i219::regs::REGISTER_BYTES as u64) .map(|(index, bytes)| (index as u32, *bytes)) .ok_or(Opening::NoWindow)?; - let mapped = dev - .map_bar(bar, bytes) - .map_err(KernelRefused::on("the BAR")) - .map_err(Opening::Kernel)?; + let (window, mapped) = Bar::map(dev, bar, bytes).map_err(Opening::Kernel)?; crate::say!( "netstack: I219: PCI {:02x}:{:02x}.{} registers in BAR {bar} ({bytes:#x} bytes)", info.bus, info.dev, info.func, ); - // SAFETY: `map_bar` answered `bytes` bytes of live mapping, and `mapped` - // goes to the caller with the window, which keeps both for as long as it - // keeps either. - let window = unsafe { Window::new(mapped.as_ptr(), bytes as usize) }; - Ok((Bar::over(window), mapped)) + Ok((window, mapped)) } /// Everything the claim is asked for before the part is reached, in the order @@ -145,17 +137,8 @@ fn granted(dev: &PciDev) -> Result<(Bar, Grant, SharedMemory, DmaRegion), Openin bar.read32(toyos_i219::regs::TCTL) ); toyos_i219::quiesce(&bar); - let region = dev - .dma_alloc(toyos_i219::GRANT_BYTES) - .map_err(KernelRefused::on("a DMA grant")) - .map_err(Opening::Kernel)?; - let device_base = region.device_addr; - // SAFETY: `dma_alloc` answered `GRANT_BYTES` bytes of live mapping, and - // `region` goes to the caller with the grant, which keeps both for as - // long as it keeps either. - let window = - unsafe { Window::new(region.memory.as_ptr(), toyos_i219::GRANT_BYTES as usize) }; - Ok((bar, Grant::over(window, device_base), mapped, region)) + let (grant, region) = Grant::alloc(dev, toyos_i219::GRANT_BYTES).map_err(Opening::Kernel)?; + Ok((bar, grant, mapped, region)) } /// What a bring-up says about itself: a sentence each for what it asked the @@ -356,3 +339,25 @@ impl Nic { } } } + +/// What a diagnostic last said, so it says it again only on a change. +/// +/// **On change, not per element**: a device flooding a ring with descriptors a +/// driver will not act on costs one line, not one per descriptor, which is the +/// difference between a diagnostic and a way to drown the console from the +/// other side of the boundary. +struct Latch(Cell); + +impl Default for Latch { + fn default() -> Self { + Self(Cell::new(T::default())) + } +} + +impl Latch { + /// The previous value if `now` is not it, and `None` if nothing has moved. + fn moved(&self, now: T) -> Option { + let was = self.0.replace(now); + (was != now).then_some(was) + } +} diff --git a/userland/netstack/src/main.rs b/userland/netstack/src/main.rs index 619aa93f2a1..f73c8a457e7 100644 --- a/userland/netstack/src/main.rs +++ b/userland/netstack/src/main.rs @@ -44,7 +44,6 @@ use toyos_net_wire::Instant; mod card; mod client; -mod device; mod i219; mod pipes; mod serve; diff --git a/userland/netstack/src/virtio_net.rs b/userland/netstack/src/virtio_net.rs index 4656807099f..ae6b8b0fc86 100644 --- a/userland/netstack/src/virtio_net.rs +++ b/userland/netstack/src/virtio_net.rs @@ -9,7 +9,7 @@ //! //! **The transport and the rings are `toyos-virtio`'s**, where every word the //! device writes is bounded and every refusal is host-tested, over -//! `device.rs`'s register window and grant. What is this file's is the +//! `toyos-pci-claim`'s register window and grant. What is this file's is the //! network device (§5.1): which buffer goes with which head, the header in //! front of every frame, and what becomes of a refusal. //! @@ -24,20 +24,16 @@ use std::cell::RefCell; use toyos::shm::SharedMemory; -use toyos::volatile::Window; use toyos::{DmaRegion, PciDev}; -use toyos_abi::syscall::{RegWidth, SyscallError}; +use toyos_abi::syscall::SyscallError; use toyos_device_memory::DmaBuffers; -use toyos_virtio::pci::{vendor_caps, Layout, Live, Offer, WalkRefusal}; +use toyos_pci_claim::virtio::{negotiate, Negotiated, Refusal}; +use toyos_pci_claim::{Bar, Grant}; +use toyos_virtio::pci::Live; use toyos_virtio::queue::{avail_bytes, desc_bytes, Buffer, Parts, Published, Used, Virtqueue}; -use crate::device::{Bar, ClaimConfig, Grant, KernelRefused}; - /// §5.1.3: the device has a MAC address of its own to read. const VIRTIO_NET_F_MAC: u64 = 1 << 5; -/// §6, for the feature line: the bit `toyos-virtio` accepts wherever it is -/// offered, and a gate reads back. -const VIRTIO_F_ACCESS_PLATFORM: u64 = toyos_virtio::pci::VIRTIO_F_ACCESS_PLATFORM; /// The one MSI-X table entry the kernel programs. const MSIX_ENTRY: u16 = 0; @@ -81,41 +77,6 @@ const _: () = { assert!(NET_HDR_SIZE < RX_BUF_SIZE && NET_HDR_SIZE < TX_BUF_SIZE); }; -/// Why the device was not brought up. Each keeps its own word: a machine with -/// no such device and one that refused a feature set ask different things of a -/// caller. -#[derive(Clone, Copy, PartialEq, Eq, Debug)] -pub enum Refusal { - /// What the device published or answered, as the transport refused it. - Device(toyos_virtio::pci::Refusal), - Kernel(KernelRefused), - /// The capability list, as the walk refused it. - Walk(WalkRefusal), - /// The claim answered a configuration read it had to refuse. - Unbounded(&'static str, u32), -} - -impl From for Refusal { - fn from(why: toyos_virtio::pci::Refusal) -> Self { - Self::Device(why) - } -} - -impl std::fmt::Display for Refusal { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - Self::Device(why) => write!(f, "{why}"), - Self::Kernel(why) => write!(f, "{why}"), - Self::Walk(why) => write!(f, "{why}"), - Self::Unbounded(what, at) => write!( - f, - "the claim answered a {what} at {at:#x}, so it is not a claim on one \ - function's own configuration space" - ), - } - } -} - /// The next chain the device has finished with on `rings`, or `None`. /// /// **Every way the used ring is not believed ends the device's use**: a head @@ -128,37 +89,6 @@ fn finished(rings: &mut Virtqueue) -> Option { .unwrap_or_else(|why| panic!("netstack: this NIC cannot be driven on — {why}")) } -/// The bound the capability walk rests on, asked once before the walk. -/// -/// **`toyos_virtio::pci::vendor_caps` indexes configuration space by numbers -/// the *device* wrote** -/// — the capability pointer and every `next` link in the chain — and it is safe -/// 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. -fn config_space_is_bounded(dev: &PciDev) -> Result<(), Refusal> { - const CONFIG_BYTES: u32 = 4096; - for (what, at, width) in [ - ("read past its configuration space", CONFIG_BYTES, RegWidth::U8), - ("misaligned read", 1, RegWidth::U16), - ] { - if dev.config_read(at, width).is_ok() { - return Err(Refusal::Unbounded(what, at)); - } - } - // 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!( - "netstack: this claim answers {CONFIG_BYTES} bytes of configuration space and refuses \ - every access outside them" - ); - Ok(()) -} - /// The virtio-net function, brought up and driving. pub struct VirtioNet { dev: PciDev, @@ -178,52 +108,11 @@ impl VirtioNet { /// feature negotiation, both queues and their vectors in the order /// `toyos-virtio`'s types fix, which is virtio 1.2 §3.1.1's. pub fn open(dev: PciDev) -> Result { - let info = dev.describe().map_err(KernelRefused::on("the claim's description")).map_err(Refusal::Kernel)?; - - config_space_is_bounded(&dev)?; - let layout = Layout::of(&vendor_caps(&ClaimConfig(&dev)).map_err(Refusal::Walk)?)?; - // The kernel hands out a BAR at a time, and reports 0 bytes for one it - // keeps back. - let bar = layout.bar(); - let bar_bytes = *info - .bar_bytes - .get(bar as usize) - .filter(|bytes| **bytes > 0) - .ok_or(toyos_virtio::pci::Refusal::MissingCap("a BAR this claim may map"))?; - let mapped = dev - .map_bar(bar as u32, bar_bytes) - .map_err(KernelRefused::on("the register window")) - .map_err(Refusal::Kernel)?; - // SAFETY: the mapping is `bar_bytes` long and lives as long as - // `mapped`, which this struct holds for its own life. - let window = unsafe { Window::new(mapped.as_ptr(), bar_bytes as usize) }; - - let offer = Offer::acknowledge(Bar::over(window), &layout)?; - let offered = offer.features(); - let setup = offer.accept(VIRTIO_NET_F_MAC)?; - let features = setup.features(); - // The line the kernel's virtio drivers print, in the same shape: - // `iommu_virtio_platform` reads it back for every virtio function the - // machine creates. - crate::say!( - "netstack: VirtIO: PCI {:02x}:{:02x}.{} features device={offered:#x} \ - negotiated={features:#x} access_platform={}", - info.bus, - info.dev, - info.func, - if features & VIRTIO_F_ACCESS_PLATFORM != 0 { 'y' } else { 'n' }, - ); + let Negotiated { setup, mapping, features } = negotiate(&dev, VIRTIO_NET_F_MAC)?; + crate::say!("netstack: {features}"); - let region = dev - .dma_alloc(GRANT_BYTES) - .map_err(KernelRefused::on("a DMA grant")) - .map_err(Refusal::Kernel)?; - // SAFETY: the grant covers at least `GRANT_BYTES` — the kernel rounds - // the request up to whole pages, never down — and lives as long as - // `region`, which this struct holds. - let window = unsafe { Window::new(region.memory.as_ptr(), GRANT_BYTES as usize) }; - window.zero(); - let grant = Grant::over(window, region.device_addr); + let (grant, region) = Grant::alloc(&dev, GRANT_BYTES)?; + grant.window().zero(); let rx = Virtqueue::new(grant, RX_QUEUE, RX_QUEUE_SIZE, RX_PARTS); let tx = TxQueue::new(grant); @@ -242,7 +131,7 @@ impl VirtioNet { let nic = Self { dev, device: setup.driver_ok(), - _bar: mapped, + _bar: mapping, _region: region, grant, rx: RefCell::new(rx), @@ -415,6 +304,7 @@ impl TxQueue { /// used-ring element must satisfy is `toyos-virtio`'s, and tested there. #[cfg(test)] mod tests { + use toyos::volatile::Window; use toyos_virtio::queue::{AVAIL_ENTRY_BYTES, DESC_BYTES, RING_ENTRIES, USED_ELEM_BYTES}; use super::*; diff --git a/userland/soundserver/Cargo.toml b/userland/soundserver/Cargo.toml index 66a4d7b6999..4926eac28ca 100644 --- a/userland/soundserver/Cargo.toml +++ b/userland/soundserver/Cargo.toml @@ -11,7 +11,7 @@ toyos-hda = { path = "../../toyos-hda" } toyos-mixer = { path = "mixer" } toyos-virtio-sound = { path = "virtio-sound" } toyos-virtio = { path = "../../toyos-virtio" } -toyos-device-memory = { path = "../../toyos-device-memory" } +toyos-pci-claim = { path = "../toyos-pci-claim" } toyos-inspect = { path = "../../toyos-inspect" } rubato = "0.16" diff --git a/userland/soundserver/src/backend.rs b/userland/soundserver/src/backend.rs index 39daf3a1259..dab37a918e8 100644 --- a/userland/soundserver/src/backend.rs +++ b/userland/soundserver/src/backend.rs @@ -7,7 +7,6 @@ //! refilled* and nothing else in the loop can be written without knowing which. use toyos::AsHandle; -use toyos_abi::audio::AudioCompletionRecord; use toyos_abi::syscall; use toyos_abi::RawHandle; @@ -37,6 +36,17 @@ pub(crate) enum Pipeline { Ring, } +/// Periods the device has given back, and when it said so. +#[derive(Clone, Copy)] +pub(crate) struct Completion { + pub(crate) mask: u32, + /// When the device's first word about these periods landed: where a + /// wake's lateness is split between the device and soundserver. + pub(crate) first_nanos: u64, + /// When its newest landed, which the DLL takes as the last period's. + pub(crate) last_nanos: u64, +} + /// The device half of the mix loop. /// /// Two implementations and no more: a framework before there are three is an @@ -55,8 +65,8 @@ pub(crate) trait Backend { /// mapped once at claim. fn buffer(&self, idx: usize) -> *mut u8; - /// Completion records, oldest first, into `out`. `0` is nothing pending. - fn completions(&mut self, out: &mut [AudioCompletionRecord]) -> usize; + /// The periods given back since the last call, or `None` for none. + fn completion(&mut self) -> Option; /// Period `idx` has played and is soundserver's again. /// @@ -95,8 +105,8 @@ impl Backend for VirtioBackend { self.virtio.buffer(idx) } - fn completions(&mut self, out: &mut [AudioCompletionRecord]) -> usize { - self.virtio.completions(out) + fn completion(&mut self) -> Option { + self.virtio.completion() } fn released(&mut self, _idx: usize) {} @@ -129,13 +139,16 @@ impl Backend for HdaBackend { self.buffers[idx] } - fn completions(&mut self, out: &mut [AudioCompletionRecord]) -> usize { + /// The kernel's record has one time, the newest interrupt's, and it is + /// both of the completion's. + fn completion(&mut self) -> Option { match self.hda.dev().completions() { - Ok(record) => { - out[0] = record; - 1 - } - Err(syscall::SyscallError::WouldBlock) => 0, + Ok(record) => Some(Completion { + mask: record.mask, + first_nanos: record.timestamp_nanos, + last_nanos: record.timestamp_nanos, + }), + Err(syscall::SyscallError::WouldBlock) => None, Err(e) => panic!("soundserver: hda completions failed: {e:?}"), } } diff --git a/userland/soundserver/src/mix.rs b/userland/soundserver/src/mix.rs index ea8000eccdc..4f2b18362cb 100644 --- a/userland/soundserver/src/mix.rs +++ b/userland/soundserver/src/mix.rs @@ -14,7 +14,6 @@ use toyos::endow::Endowments; use toyos::poller::{Poller, READABLE}; use toyos::syscap::SysCap; -use toyos_abi::audio::AudioCompletionRecord; use toyos_abi::syscall; use toyos_abi::RawHandle; use toyos_hda::stream; @@ -238,7 +237,6 @@ pub(crate) fn mix_thread( let mut convert_buf = vec![0.0f32; max_client_frames * 2]; let mut dither_rng = Xorshift32::new(toyos_abi::clock::nanos_since_boot() as u32); let mut dll = Dll::new(period_nanos as f64); - let mut records = [AudioCompletionRecord { mask: 0, _pad: 0, timestamp_nanos: 0 }; 16]; const TOKEN_AUDIO: u64 = u64::MAX - 1; const TOKEN_CMD: u64 = u64::MAX - 2; @@ -315,54 +313,50 @@ pub(crate) fn mix_thread( idle_wakes = 0; } - let n_records = backend.completions(&mut records); - if n_records > 0 { - // Read before the record loop, because it is what a *pickup* is - // measured to: the instant soundserver first held the record, not the - // instant it finished acting on a batch of them. + let completed = backend.completion(); + if let Some(rec) = completed { + // Read before the record is acted on, because it is what a + // *pickup* is measured to: the instant soundserver first held the + // record, not the instant it finished acting on it. let seen_at = toyos_abi::clock::nanos_since_boot(); - let mut wake_completions = 0u32; - for rec in &records[..n_records] { - let n = rec.mask.count_ones(); - assert!(n > 0, "soundserver: completion record with empty mask"); - assert_eq!(free_mask & rec.mask, 0, "soundserver: repeated completion for free buffer"); - // Where the engine is, taken from what it reported rather than - // predicted: a mask a driver reads late is the OR of every - // `completed` since it last looked, so it can name a whole lap - // — which places the engine nowhere — and a cursor soundserver - // stepped itself would have to be right about how many laps - // that was. Re-deriving it per record cannot drift. - if pipeline == Pipeline::Ring { - match stream::decode(rec.mask, num_buffers) { - Some(stream::Completed::Run { first, count }) => { - ring_cursor = (first + count) % num_buffers; - } - // Every period played and the mask says no more than - // that. The cursor stays where it was: the fill order - // from here is a guess either way, and a lap of silence - // has already gone out — it counts as the drain it - // is, and the next record re-anchors. - Some(stream::Completed::Lapped) => {} - None => panic!( - "soundserver: the engine completed {:#x}, which is no walk of a \ - {num_buffers}-period ring", - rec.mask - ), + let n = rec.mask.count_ones(); + assert!(n > 0, "soundserver: completion record with empty mask"); + assert_eq!(free_mask & rec.mask, 0, "soundserver: repeated completion for free buffer"); + // Where the engine is, taken from what it reported rather than + // predicted: a mask a driver reads late is the OR of every + // `completed` since it last looked, so it can name a whole lap + // — which places the engine nowhere — and a cursor soundserver + // stepped itself would have to be right about how many laps + // that was. Re-deriving it per record cannot drift. + if pipeline == Pipeline::Ring { + match stream::decode(rec.mask, num_buffers) { + Some(stream::Completed::Run { first, count }) => { + ring_cursor = (first + count) % num_buffers; } + // Every period played and the mask says no more than + // that. The cursor stays where it was: the fill order + // from here is a guess either way, and a lap of silence + // has already gone out — it counts as the drain it + // is, and the next record re-anchors. + Some(stream::Completed::Lapped) => {} + None => panic!( + "soundserver: the engine completed {:#x}, which is no walk of a \ + {num_buffers}-period ring", + rec.mask + ), } - unplayed = unplayed.saturating_sub(n as usize); - free_mask |= rec.mask; - // Zero-on-complete, before anything can decide to leave - // this buffer unfilled: the engine returns to it in - // `num_buffers` periods whatever soundserver does. - for idx in 0..num_buffers { - if rec.mask & (1 << idx) != 0 { - backend.released(idx); - } + } + unplayed = unplayed.saturating_sub(n as usize); + free_mask |= rec.mask; + // Zero-on-complete, before anything can decide to leave + // this buffer unfilled: the engine returns to it in + // `num_buffers` periods whatever soundserver does. + for idx in 0..num_buffers { + if rec.mask & (1 << idx) != 0 { + backend.released(idx); } - wake_completions += n; - dll.update(rec.timestamp_nanos as f64, n); } + dll.update(rec.last_nanos as f64, n); // Measured against the prediction this wait was *armed* on, not // against whatever the DLL holds when the wait returns. They differ // on a window's first wake, armed while soundserver was still idle and @@ -371,9 +365,10 @@ pub(crate) fn mix_thread( // whenever soundserver armed a timer the distance from that prediction // is the sample, however large. // - // **And it is recorded in two halves**, split at the oldest - // record's ISR timestamp: everything before it is the device - // failing to complete when it was due, everything after it is + // **And it is recorded in two halves**, split at when the + // device's first message about these periods landed: everything + // before it is the device failing to complete when it was due, + // everything after it is // soundserver failing to run once it had. `WorstWake` is where that // distinction is argued; here it costs one subtraction, because // both instants were already in hand. @@ -384,18 +379,18 @@ pub(crate) fn mix_thread( // landed *before* it was due was not late by any amount, and a // soundserver that is nonetheless late here slept through it, so all // of the overshoot is the pickup. - let irq_at = records[0].timestamp_nanos.max(t_est); + let irq_at = rec.first_nanos.max(t_est); stats.wake( seen_at.saturating_sub(t_est), irq_at.saturating_sub(t_est), seen_at.saturating_sub(irq_at), - wake_completions, + n, period_nanos, ); } if !streams.is_empty() { - stats.completions += wake_completions; - stats.max_batch = stats.max_batch.max(wake_completions); + stats.completions += n; + stats.max_batch = stats.max_batch.max(n); } } else if armed_on.is_some() { // Armed on a grid point, woken, and the device had produced @@ -601,6 +596,7 @@ pub(crate) fn mix_thread( } if wake_left_idle(was_streaming, started_at_wake, !streams.is_empty(), cmd_ready) { + let n_records = usize::from(completed.is_some()); idle_wakes = idle_wakes.saturating_add(1); if idle_wakes < IDLE_WAKES_SAID { say!("soundserver: idle wake {idle_wakes} ({n_records} records)"); diff --git a/userland/soundserver/src/virtio.rs b/userland/soundserver/src/virtio.rs index 7f5d46c56ab..bb7d407474f 100644 --- a/userland/soundserver/src/virtio.rs +++ b/userland/soundserver/src/virtio.rs @@ -6,29 +6,30 @@ //! translates through. **The transport and the queues are `toyos-virtio`'s, //! and the sound device's messages and queues `toyos-virtio-sound`'s**, where //! every word the device writes back is bounded and every refusal is -//! host-tested; this file is the instructions under them — the register window -//! and the grant as `toyos-device-memory`'s boundary — and what becomes of a -//! refusal. +//! host-tested; the register window, the grant and the bring-up up to +//! `FEATURES_OK` are `toyos-pci-claim`'s. This file is what is left: the +//! sound device's vectors, the times its periods came back at, and what +//! becomes of a refusal. //! //! **Only the transmit queue raises an interrupt.** The control queue is polled //! for the one answer it owes, and the event queue is read when a period comes -//! back, so the claim is readable exactly when a period has played. +//! back, so the claim is readable exactly when a period has played — and the +//! kernel's record of that message says when it landed, which is the time the +//! DLL is clocked by. //! //! **A refusal after bring-up ends soundserver** by its own name, as netstack's //! does: no conforming device writes one. -use std::sync::atomic::{fence, Ordering}; - use toyos::shm::SharedMemory; -use toyos::volatile::Window; use toyos::{DmaRegion, PciDev}; -use toyos_abi::audio::AudioCompletionRecord; -use toyos_abi::syscall::{RegWidth, SyscallError}; -use toyos_device_memory::{DmaBuffers, Registers}; -use toyos_virtio::pci::{vendor_caps, ConfigSpace, Layout, Live, Offer, WalkRefusal, NO_VECTOR, - VIRTIO_F_ACCESS_PLATFORM}; +use toyos_abi::syscall::SyscallError; +use toyos_pci_claim::virtio::{negotiate, Negotiated}; +use toyos_pci_claim::{Bar, Grant, KernelRefused}; +use toyos_virtio::pci::{Live, NO_VECTOR}; use toyos_virtio_sound::{Queues, Sound, PERIOD_BYTES, STREAM_ID}; +use crate::backend::Completion; + /// virtio's vendor id and the sound device's (§4.1.2.1: `0x1040` + 25), as /// the manifest row spells the claim. pub const PCI_ID: toyos_abi::syscall::PciId = toyos_abi::syscall::PciId { vendor: 0x1af4, device: 0x1059 }; @@ -39,119 +40,38 @@ const MSIX_ENTRY: u16 = 0; /// §5.14.4: `streams`, the second `le32` of the configuration. const CONFIG_STREAMS: usize = 4; -/// A mapped BAR, as the transport reaches the device's registers. -#[derive(Clone, Copy)] -pub struct Bar(Window); - -impl Registers for Bar { - fn bytes(&self) -> usize { - self.0.bytes() - } - fn read8(&self, at: usize) -> u8 { - self.0.read(at) - } - fn read16(&self, at: usize) -> u16 { - self.0.read(at) - } - fn read32(&self, at: usize) -> u32 { - self.0.read(at) - } - fn write8(&self, at: usize, value: u8) { - self.0.write(at, value); - } - fn write16(&self, at: usize, value: u16) { - self.0.write(at, value); - } - fn write32(&self, at: usize, value: u32) { - self.0.write(at, value); - } -} - -/// The DMA grant, as the queues reach it. `device_base` is what the unit -/// translates for this function and for nothing else. -#[derive(Clone, Copy)] -pub struct Grant { - window: Window, - device_base: u64, -} - -impl DmaBuffers for Grant { - fn bytes(&self) -> usize { - self.window.bytes() - } - fn device_addr(&self, at: usize) -> u64 { - self.device_base + at as u64 - } - fn read16(&self, at: usize) -> u16 { - self.window.read(at) - } - fn read32(&self, at: usize) -> u32 { - self.window.read(at) - } - fn read64(&self, at: usize) -> u64 { - self.window.read(at) - } - fn write16(&self, at: usize, value: u16) { - self.window.write(at, value); - } - fn write32(&self, at: usize, value: u32) { - self.window.write(at, value); - } - fn write64(&self, at: usize, value: u64) { - self.window.write(at, value); - } - fn publish(&self) { - fence(Ordering::Release); - } - fn observe(&self) { - fence(Ordering::Acquire); - } -} - -/// The claim's configuration space, as the capability walk reads it. -struct ClaimConfig<'a>(&'a PciDev); - -impl ConfigSpace for ClaimConfig<'_> { - type Refused = SyscallError; - - fn read8(&self, at: u16) -> Result { - self.0.config_read(at as u32, RegWidth::U8).map(|byte| byte as u8) - } - - fn read32(&self, at: u16) -> Result { - self.0.config_read(at as u32, RegWidth::U32) - } -} - /// Why this machine's virtio-sound function cannot carry audio. Each is a line /// soundserver prints before it falls back to the null sink. pub enum Refusal { - /// The kernel refused a call a bring-up cannot go on without. - Kernel(&'static str, SyscallError), - Walk(WalkRefusal), - Transport(toyos_virtio::pci::Refusal), + Claim(toyos_pci_claim::virtio::Refusal), Sound(toyos_virtio_sound::Refusal), } impl core::fmt::Display for Refusal { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { match self { - Self::Kernel(call, why) => write!(f, "the kernel refused {call}: {why:?}"), - Self::Walk(why) => write!(f, "{why}"), - Self::Transport(why) => write!(f, "{why}"), + Self::Claim(why) => write!(f, "{why}"), Self::Sound(why) => write!(f, "{why}"), } } } +impl From for Refusal { + fn from(why: toyos_pci_claim::virtio::Refusal) -> Self { + Self::Claim(why) + } +} + impl From for Refusal { fn from(why: toyos_virtio::pci::Refusal) -> Self { - Self::Transport(why) + Self::Claim(why.into()) } } -fn kernel(call: &'static str) -> impl Fn(SyscallError) -> Refusal { - move |why| Refusal::Kernel(call, why) +impl From for Refusal { + fn from(why: KernelRefused) -> Self { + Self::Claim(why.into()) + } } pub struct Virtio { @@ -168,44 +88,12 @@ impl Virtio { /// Bring the function up in the order `toyos-virtio`'s types fix, which is /// §3.1.1's, and stream 0 in §5.14.5's: the rate and channel count chosen. pub fn claim(dev: PciDev) -> Result<(Self, u32, u8), Refusal> { - let info = dev.describe().map_err(kernel("the claim's description"))?; - let layout = Layout::of(&vendor_caps(&ClaimConfig(&dev)).map_err(Refusal::Walk)?)?; - // The kernel reports 0 bytes for a BAR it keeps back. - let bar = layout.bar(); - let bar_bytes = *info - .bar_bytes - .get(bar as usize) - .filter(|bytes| **bytes > 0) - .ok_or(toyos_virtio::pci::Refusal::MissingCap("a BAR this claim may map"))?; - let mapped = dev.map_bar(bar as u32, bar_bytes).map_err(kernel("the register window"))?; - // SAFETY: the mapping is `bar_bytes` long and lives as long as - // `mapped`, which `Virtio` holds for its own life. - let window = unsafe { Window::new(mapped.as_ptr(), bar_bytes as usize) }; - - let offer = Offer::acknowledge(Bar(window), &layout)?; - let offered = offer.features(); // §5.14.3: the sound device defines no feature bit. - let setup = offer.accept(0)?; - let features = setup.features(); - // The kernel's virtio drivers' feature line, in the same shape: - // `iommu_virtio_platform` reads it back for every virtio function. - say!( - "soundserver: VirtIO: PCI {:02x}:{:02x}.{} features device={offered:#x} \ - negotiated={features:#x} access_platform={}", - info.bus, - info.dev, - info.func, - if features & VIRTIO_F_ACCESS_PLATFORM != 0 { 'y' } else { 'n' }, - ); + let Negotiated { setup, mapping, features } = negotiate(&dev, 0)?; + say!("soundserver: {features}"); - let region = dev - .dma_alloc(toyos_virtio_sound::GRANT_BYTES as u64) - .map_err(kernel("a DMA grant"))?; - // SAFETY: the kernel rounds the request up to whole pages, never down, - // and the grant lives as long as `region`, which `Virtio` holds. - let window = unsafe { Window::new(region.memory.as_ptr(), toyos_virtio_sound::GRANT_BYTES) }; - window.zero(); - let grant = Grant { window, device_base: region.device_addr }; + let (grant, region) = Grant::alloc(&dev, toyos_virtio_sound::GRANT_BYTES as u64)?; + grant.window().zero(); let queues = Queues::new(grant); let (setup, streams) = setup @@ -226,7 +114,7 @@ impl Virtio { "virtio-sound: configured stream {STREAM_ID}: {}Hz {}ch s16le", stream.rate, stream.channels ); - let virtio = Self { dev, sound, grant, _bar: mapped, _region: region }; + let virtio = Self { dev, sound, grant, _bar: mapping, _region: region }; Ok((virtio, stream.rate, stream.channels)) } @@ -235,19 +123,22 @@ impl Virtio { } pub fn buffer(&self, idx: usize) -> *mut u8 { - self.grant.window.sub(Sound::>::period(idx), PERIOD_BYTES).as_ptr() + self.grant.window().sub(Sound::>::period(idx), PERIOD_BYTES).as_ptr() } - /// The periods played since the last call, as one record stamped now. + /// The periods played since the last call, and when the device said so. /// /// The claim's record is taken before the ring is read, so a period that - /// lands after the read leaves the claim readable for the next wake. - pub fn completions(&mut self, out: &mut [AudioCompletionRecord]) -> usize { - match self.dev.irq() { - Ok(_) | Err(SyscallError::WouldBlock) => {} + /// lands after the read leaves the claim readable for the next wake. The + /// device writes the used ring before it sends the message, so a ring read + /// between the two holds a period whose message has not landed: then the + /// read is when it lands. + pub fn completion(&mut self) -> Option { + let record = match self.dev.irq() { + Ok(record) => Some(record), + Err(SyscallError::WouldBlock) => None, Err(why) => panic!("soundserver: the virtio-sound claim's interrupt record: {why:?}"), - } - let timestamp_nanos = toyos_abi::clock::nanos_since_boot(); + }; // The device's own view of an underrun, which soundserver's counters // cannot see. self.sound @@ -263,10 +154,16 @@ impl Virtio { .completed() .unwrap_or_else(|why| panic!("soundserver: virtio-sound cannot be driven on — {why}")); if mask == 0 { - return 0; + return None; } - out[0] = AudioCompletionRecord { mask, _pad: 0, timestamp_nanos }; - 1 + let (first_nanos, last_nanos) = match record { + Some(record) => (record.first_nanos, record.last_nanos), + None => { + let now = toyos_abi::clock::nanos_since_boot(); + (now, now) + } + }; + Some(Completion { mask, first_nanos, last_nanos }) } /// Put period `idx` on the wire: its PCM descriptor is a whole period. diff --git a/userland/soundserver/virtio-sound/src/lib.rs b/userland/soundserver/virtio-sound/src/lib.rs index ec165c80037..ae764b7328c 100644 --- a/userland/soundserver/virtio-sound/src/lib.rs +++ b/userland/soundserver/virtio-sound/src/lib.rs @@ -297,8 +297,6 @@ impl Sound { let xfer = TX_XFER + idx * wire::XFER_BYTES as usize; let status = TX_STATUS + idx * wire::STATUS_BYTES as usize; self.mem.write32(xfer, STREAM_ID); - // What a status the device does not write leaves behind is no `S_OK`. - self.mem.write32(status, 0); let chain = [ Buffer::readable(self.mem.device_addr(xfer), wire::XFER_BYTES), Buffer::readable(self.mem.device_addr(pcm), PERIOD_BYTES as u32), diff --git a/userland/toyos-pci-claim/Cargo.toml b/userland/toyos-pci-claim/Cargo.toml new file mode 100644 index 00000000000..9a60f6c5dea --- /dev/null +++ b/userland/toyos-pci-claim/Cargo.toml @@ -0,0 +1,16 @@ +[package] +name = "toyos-pci-claim" +description = "A PCI claim as a userland driver reaches its function: toyos-device-memory's register window and DMA grant over the SDK's Window, and a virtio function's capability walk, mapping and feature negotiation through toyos-virtio." +version = "0.1.0" +edition = "2021" +license = "MIT OR Apache-2.0" +publish = false + +[dependencies] +toyos = { path = "../../toyos" } +toyos-abi = { path = "../../toyos-abi" } +toyos-device-memory = { path = "../../toyos-device-memory" } +toyos-virtio = { path = "../../toyos-virtio" } + +[lib] +doctest = false diff --git a/userland/netstack/src/device.rs b/userland/toyos-pci-claim/src/lib.rs similarity index 52% rename from userland/netstack/src/device.rs rename to userland/toyos-pci-claim/src/lib.rs index e9cd8a45230..483ee7fbe36 100644 --- a/userland/netstack/src/device.rs +++ b/userland/toyos-pci-claim/src/lib.rs @@ -1,32 +1,41 @@ -//! What both NIC drivers need from the substrate: `toyos-device-memory`'s -//! boundary over the SDK's `toyos::volatile::Window`, the claim's -//! configuration space as `toyos-virtio` walks it, the kernel's word for a -//! call a bring-up cannot go on without, and the latch a diagnostic is printed -//! on. +//! A PCI claim, as a userland driver reaches the function behind it. //! -//! **[`Bar`] and [`Grant`] are the one implementation of the boundary both -//! driver crates are written against**, an instruction deep: every offset -//! that reaches them a driver crate has bounded, and the two barriers are here -//! and nowhere else in this program. Neither owns its mapping: a driver's -//! holder keeps the `SharedMemory` and the `DmaRegion` for as long as it keeps -//! the driver. - -use std::cell::Cell; +//! What the kernel keeps is the claim: config space, the vector it programmed +//! into the function's MSI-X table, and the address space the function +//! translates through. **This crate is the one implementation of +//! `toyos-device-memory`'s boundary over the SDK's [`Window`]** — [`Bar`] and +//! [`Grant`], an instruction deep, every offset that reaches them bounded by +//! the driver crate above — and, for a virtio function, the bring-up every +//! virtio driver makes before its device type begins ([`virtio`]). +//! +//! Neither [`Bar`] nor [`Grant`] owns its mapping: whoever holds the driver +//! keeps the `SharedMemory` and the `DmaRegion` for as long as it keeps the +//! driver, and the constructors here hand both back together. + use std::sync::atomic::{fence, Ordering}; +use toyos::shm::SharedMemory; use toyos::volatile::Window; -use toyos::PciDev; -use toyos_abi::syscall::{RegWidth, SyscallError}; +use toyos::{DmaRegion, PciDev}; +use toyos_abi::syscall::SyscallError; use toyos_device_memory::{DmaBuffers, Registers}; -use toyos_virtio::pci::ConfigSpace; + +pub mod virtio; /// A mapped BAR, as a driver reaches its device's registers. #[derive(Clone, Copy)] pub struct Bar(Window); impl Bar { - pub fn over(window: Window) -> Self { - Self(window) + /// BAR `index` of the claim, `bytes` long, and the mapping the window + /// points into. + pub fn map(dev: &PciDev, index: u32, bytes: u64) -> Result<(Self, SharedMemory), KernelRefused> { + let mapped = dev.map_bar(index, bytes).map_err(KernelRefused::on("the register window"))?; + // SAFETY: `map_bar` answered `bytes` bytes of live mapping, and + // `mapped` goes to the caller with the window, which keeps both for as + // long as it keeps either. + let window = unsafe { Window::new(mapped.as_ptr(), bytes as usize) }; + Ok((Self(window), mapped)) } } @@ -70,12 +79,25 @@ pub struct Grant { } impl Grant { + /// A fresh grant of at least `bytes` for the claim's function, and the + /// region the window points into. + pub fn alloc(dev: &PciDev, bytes: u64) -> Result<(Self, DmaRegion), KernelRefused> { + let region = dev.dma_alloc(bytes).map_err(KernelRefused::on("a DMA grant"))?; + // SAFETY: the kernel rounds the request up to whole pages, never down, + // and `region` goes to the caller with the grant, which keeps both for + // as long as it keeps either. + let window = unsafe { Window::new(region.memory.as_ptr(), bytes as usize) }; + Ok((Self::over(window, region.device_addr), region)) + } + + /// The grant over memory the caller already holds: a host test's plain + /// allocation, with the address a device would be told. pub fn over(window: Window, device_base: u64) -> Self { Self { window, device_base } } - /// The same bytes, for the holder's own view of them: the frames, which no - /// driver crate reaches. + /// The same bytes, for the holder's own view of them: what no driver crate + /// reaches — frames, PCM. pub fn window(&self) -> Window { self.window } @@ -123,23 +145,6 @@ impl DmaBuffers for Grant { } } -/// A claim's configuration space, as `toyos-virtio`'s capability walk reads -/// it: each read one `config_read` of its width, and its refusal the kernel's -/// word. -pub struct ClaimConfig<'a>(pub &'a PciDev); - -impl ConfigSpace for ClaimConfig<'_> { - type Refused = SyscallError; - - fn read8(&self, at: u16) -> Result { - self.0.config_read(at as u32, RegWidth::U8).map(|byte| byte as u8) - } - - fn read32(&self, at: u16) -> Result { - self.0.config_read(at as u32, RegWidth::U32) - } -} - /// The kernel refused a call a bring-up cannot go on without, and the word is /// the kernel's own. #[derive(Clone, Copy, PartialEq, Eq, Debug)] @@ -159,25 +164,3 @@ impl std::fmt::Display for KernelRefused { write!(f, "the kernel refused {}: {:?}", self.call, self.why) } } - -/// What a diagnostic last said, so it says it again only on a change. -/// -/// **On change, not per element**: a device flooding a ring with descriptors a -/// driver will not act on costs one line, not one per descriptor, which is the -/// difference between a diagnostic and a way to drown the console from the -/// other side of the boundary. -pub struct Latch(Cell); - -impl Default for Latch { - fn default() -> Self { - Self(Cell::new(T::default())) - } -} - -impl Latch { - /// The previous value if `now` is not it, and `None` if nothing has moved. - pub fn moved(&self, now: T) -> Option { - let was = self.0.replace(now); - (was != now).then_some(was) - } -} diff --git a/userland/toyos-pci-claim/src/virtio.rs b/userland/toyos-pci-claim/src/virtio.rs new file mode 100644 index 00000000000..2877da56f96 --- /dev/null +++ b/userland/toyos-pci-claim/src/virtio.rs @@ -0,0 +1,117 @@ +//! A virtio function's claim, brought to the features its driver and device +//! agreed: the capability walk, the register window, and §3.1.1 up to +//! `FEATURES_OK`, in the order `toyos-virtio`'s types fix. +//! +//! What is the device type's own — its queues, their vectors, its +//! configuration — begins at the [`Setup`] this answers. + +use toyos::shm::SharedMemory; +use toyos::PciDev; +use toyos_abi::syscall::{RegWidth, SyscallError}; +use toyos_virtio::pci::{vendor_caps, ConfigSpace, Layout, Offer, Setup, WalkRefusal, VIRTIO_F_ACCESS_PLATFORM}; + +use crate::{Bar, KernelRefused}; + +/// Why the function was not brought up. Each keeps its own word: a machine +/// whose kernel refused a call and a device that refused a feature set ask +/// different things of a caller. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Refusal { + Kernel(KernelRefused), + /// The capability list, as the walk refused it. + Walk(WalkRefusal), + /// What the device published or answered, as the transport refused it. + Device(toyos_virtio::pci::Refusal), +} + +impl From for Refusal { + fn from(why: toyos_virtio::pci::Refusal) -> Self { + Self::Device(why) + } +} + +impl From for Refusal { + fn from(why: KernelRefused) -> Self { + Self::Kernel(why) + } +} + +impl std::fmt::Display for Refusal { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Kernel(why) => write!(f, "{why}"), + Self::Walk(why) => write!(f, "{why}"), + Self::Device(why) => write!(f, "{why}"), + } + } +} + +/// The claim's configuration space, as the capability walk reads it: each +/// read one `config_read` of its width, and its refusal the kernel's word. +struct ClaimConfig<'a>(&'a PciDev); + +impl ConfigSpace for ClaimConfig<'_> { + type Refused = SyscallError; + + fn read8(&self, at: u16) -> Result { + self.0.config_read(at as u32, RegWidth::U8).map(|byte| byte as u8) + } + + fn read32(&self, at: u16) -> Result { + self.0.config_read(at as u32, RegWidth::U32) + } +} + +/// A function past `FEATURES_OK`. +pub struct Negotiated { + pub setup: Setup, + /// Held for the register window's life: `setup`'s window points into it. + pub mapping: SharedMemory, + /// The driver's feature line, for it to say under its own name. + pub features: Features, +} + +/// What was offered and agreed, in the shape `iommu_virtio_platform` reads +/// back for every virtio function a machine creates. +#[derive(Clone, Copy)] +pub struct Features { + at: (u8, u8, u8), + offered: u64, + negotiated: u64, +} + +impl std::fmt::Display for Features { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let (bus, dev, func) = self.at; + write!( + f, + "VirtIO: PCI {bus:02x}:{dev:02x}.{func} features device={:#x} negotiated={:#x} \ + access_platform={}", + self.offered, + self.negotiated, + if self.negotiated & VIRTIO_F_ACCESS_PLATFORM != 0 { 'y' } else { 'n' }, + ) + } +} + +/// Walk the claim's capabilities, map the BAR they name, and negotiate +/// `wanted` beside what `toyos-virtio` accepts wherever it is offered. +/// +/// Takes the claim's one description (`PciDev::describe`). +pub fn negotiate(dev: &PciDev, wanted: u64) -> Result { + let info = dev.describe().map_err(KernelRefused::on("the claim's description"))?; + let layout = Layout::of(&vendor_caps(&ClaimConfig(dev)).map_err(Refusal::Walk)?)?; + // The kernel reports 0 bytes for a BAR it keeps back. + let bar = layout.bar(); + let bar_bytes = *info + .bar_bytes + .get(bar as usize) + .filter(|bytes| **bytes > 0) + .ok_or(toyos_virtio::pci::Refusal::MissingCap("a BAR this claim may map"))?; + let (window, mapping) = Bar::map(dev, bar as u32, bar_bytes)?; + let offer = Offer::acknowledge(window, &layout)?; + let offered = offer.features(); + let setup = offer.accept(wanted)?; + let features = Features { at: (info.bus, info.dev, info.func), offered, negotiated: setup.features() }; + Ok(Negotiated { setup, mapping, features }) +} From 081785fe7149b7dd304c40b889d8cb635b421ace Mon Sep 17 00:00:00 2001 From: japabu Date: Sat, 10 Oct 2026 09:39:11 +0200 Subject: [PATCH 5/6] soundserver reads the virtio-sound ring only with a claim record in hand, and Grant's host-test constructor is a dev feature MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 2 on #822 named the `None => now` arm of `Virtio::completion`: with no record taken but a period on the used ring, it stamped the read time as the device's, the pickup-as-device-time round 1 removed, kept for the in-flight window. The arm goes. `Sound::played` takes the record the driver took and reads the ring only when there is one: the device writes a used element before the notification §2.7.7 owes for it, so a period on the ring with no record has a notification still to land, which makes the claim readable and soundserver wakes for it. The driver's order, take and then read, is now a type's: the ring is read through the record. `completed` is private. `no_period_is_stranded_between_its_used_element_and_its_notification` runs two periods (used element, then notification) against two driver wakes (take, then read) in all 70 interleavings, then wakes the driver while the claim reads ready, and asserts each period comes back exactly once. The review's other in-flight case — a record in hand while a newer period's notification has not landed — feeds the DLL a period stamped a notification early. It is recorded with an owner and an exit in issues/soundservers-virtio-clock-can-take-a-period-one-notification-early.md. `Grant::over` was public for netstack's host test alone; it is gone, and `Grant::leaked` behind the `host-grant` feature, which only netstack's dev-dependency turns on, is the test's constructor, with the leak and its `unsafe` inside the crate. toyos-abi's sentence that the virtio-sound driver builds an `AudioCompletionRecord` stamped at read time was false at this head and is deleted. diskserver's NVMe bring-up open-codes `Bar::map`, `Grant::alloc` and `KernelRefused`; filed as issues/diskservers-nvme-bring-up-open-codes-what-toyos-pci-claim-holds.md. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017cSFvbD35xJ2kGANVdm23C --- ...p-open-codes-what-toyos-pci-claim-holds.md | 18 +++++ ...an-take-a-period-one-notification-early.md | 31 ++++++++ toyos-abi/src/audio.rs | 4 - userland/netstack/Cargo.toml | 3 + userland/netstack/src/virtio_net.rs | 7 +- userland/soundserver/src/virtio.rs | 25 ++---- userland/soundserver/virtio-sound/src/lib.rs | 18 ++++- .../soundserver/virtio-sound/src/tests.rs | 79 +++++++++++++++++++ userland/toyos-pci-claim/Cargo.toml | 5 ++ userland/toyos-pci-claim/src/lib.rs | 13 ++- 10 files changed, 169 insertions(+), 34 deletions(-) create mode 100644 issues/diskservers-nvme-bring-up-open-codes-what-toyos-pci-claim-holds.md create mode 100644 issues/soundservers-virtio-clock-can-take-a-period-one-notification-early.md diff --git a/issues/diskservers-nvme-bring-up-open-codes-what-toyos-pci-claim-holds.md b/issues/diskservers-nvme-bring-up-open-codes-what-toyos-pci-claim-holds.md new file mode 100644 index 00000000000..f85a5450d3d --- /dev/null +++ b/issues/diskservers-nvme-bring-up-open-codes-what-toyos-pci-claim-holds.md @@ -0,0 +1,18 @@ +--- +status: open +kind: defect +opened: 2026-10-10 +--- + +# diskserver's NVMe bring-up open-codes what `toyos-pci-claim` holds + +`Controller::open` (`userland/diskserver/src/nvme.rs`) maps BAR 0 with +`PciDev::map_bar` and an `unsafe Window::new` over it, allocates its grant with +`PciDev::dma_alloc` and another `unsafe Window::new`, and names each kernel +refusal as `Refusal::Kernel(call, e)`. That is `Bar::map`, `Grant::alloc` and +`KernelRefused` of `userland/toyos-pci-claim/src/lib.rs`, which netstack's two +drivers and soundserver's virtio-sound driver use: two more `unsafe` sites +whose argument is the one the crate already makes. + +Exit: diskserver reaches its BAR and grant through `toyos-pci-claim`, or the +reason it cannot is at the crate's module header. diff --git a/issues/soundservers-virtio-clock-can-take-a-period-one-notification-early.md b/issues/soundservers-virtio-clock-can-take-a-period-one-notification-early.md new file mode 100644 index 00000000000..a89d1db793f --- /dev/null +++ b/issues/soundservers-virtio-clock-can-take-a-period-one-notification-early.md @@ -0,0 +1,31 @@ +--- +status: open +kind: defect +opened: 2026-10-10 +--- + +# soundserver's virtio-sound clock can take a period one notification early + +`Sound::played` (`userland/soundserver/virtio-sound/src/lib.rs`) reads the +transmit queue's used ring only with the claim's interrupt record in hand, and +`Virtio::completion` (`userland/soundserver/src/virtio.rs`) hands the mix loop +that record's `last_nanos`. A record says when its notifications landed and +nothing about which period each was for. So when the device has written a +newer period's used element and its notification has not landed by the ring +read, that period is counted in the mask and stamped with the older +notification's time, and `dll.update(last_nanos, n)` in +`userland/soundserver/src/mix.rs` takes an update whose newest period is +clocked at least one period early: an error shaped like `n − 1` periods on +that update. Its notification then lands into the next record, which comes +back with no period. + +How often the window opens is unmeasured, and no metal machine runs +virtio-sound. Nothing bounds the error but that rarity. + +Owned by whoever next changes what a claim's record carries +(`kernel/src/pcidev/record.rs`) or how soundserver's DLL is fed. Exit: the +record ties a time to the used elements it answers for — or the driver holds +back from a DLL update every period past those its record's notifications +answer for — and `toyos-virtio-sound`'s interleaving test, +`no_period_is_stranded_between_its_used_element_and_its_notification`, asserts +that no period comes back stamped earlier than its own notification. diff --git a/toyos-abi/src/audio.rs b/toyos-abi/src/audio.rs index 384db8ae4a8..8e8098c9613 100644 --- a/toyos-abi/src/audio.rs +++ b/toyos-abi/src/audio.rs @@ -11,10 +11,6 @@ use core::sync::atomic::AtomicU32; /// interrupt handler — the clock source for soundserver's DLL, and the reason the /// mask is derived there rather than by the driver at wake time. Records are /// returned oldest-first. -/// -/// soundserver's virtio-sound driver builds the same record out of its own used -/// ring, stamped when it reads the ring rather than when the message landed: a -/// claim's interrupt record carries a count and no time. #[repr(C)] #[derive(Clone, Copy)] pub struct AudioCompletionRecord { diff --git a/userland/netstack/Cargo.toml b/userland/netstack/Cargo.toml index 0dd3297872f..db88966465c 100644 --- a/userland/netstack/Cargo.toml +++ b/userland/netstack/Cargo.toml @@ -23,5 +23,8 @@ toyos-net-tcp = { path = "../../toyos-net-shard/tcp" } toyos-net-udp = { path = "../../toyos-net-udp" } toyos-net-wire = { path = "../../toyos-net-wire" } +[dev-dependencies] +toyos-pci-claim = { path = "../toyos-pci-claim", features = ["host-grant"] } + [package.metadata.toyos.host] exempt.owns = "the NIC ToyOS claims for it" diff --git a/userland/netstack/src/virtio_net.rs b/userland/netstack/src/virtio_net.rs index ae6b8b0fc86..b729671f927 100644 --- a/userland/netstack/src/virtio_net.rs +++ b/userland/netstack/src/virtio_net.rs @@ -304,7 +304,6 @@ impl TxQueue { /// used-ring element must satisfy is `toyos-virtio`'s, and tested there. #[cfg(test)] mod tests { - use toyos::volatile::Window; use toyos_virtio::queue::{AVAIL_ENTRY_BYTES, DESC_BYTES, RING_ENTRIES, USED_ELEM_BYTES}; use super::*; @@ -313,11 +312,7 @@ mod tests { /// A transmit queue over a plain allocation the size of the grant. fn queue() -> TxQueue { - let backing = vec![0u64; GRANT_BYTES as usize / 8].leak(); - // SAFETY: `leak` gives the allocation the `'static` lifetime the - // window needs, and nothing but this queue and the test reaches it. - let window = unsafe { Window::new(backing.as_mut_ptr().cast(), GRANT_BYTES as usize) }; - TxQueue::new(Grant::over(window, DEVICE_BASE)) + TxQueue::new(Grant::leaked(GRANT_BYTES as usize, DEVICE_BASE)) } /// The head in available-ring entry `nth` (§2.7.6). diff --git a/userland/soundserver/src/virtio.rs b/userland/soundserver/src/virtio.rs index bb7d407474f..1bd40d6def2 100644 --- a/userland/soundserver/src/virtio.rs +++ b/userland/soundserver/src/virtio.rs @@ -128,11 +128,8 @@ impl Virtio { /// The periods played since the last call, and when the device said so. /// - /// The claim's record is taken before the ring is read, so a period that - /// lands after the read leaves the claim readable for the next wake. The - /// device writes the used ring before it sends the message, so a ring read - /// between the two holds a period whose message has not landed: then the - /// read is when it lands. + /// The claim's record is taken before the ring is read, and the ring is + /// read only with one in hand ([`Sound::played`]). pub fn completion(&mut self) -> Option { let record = match self.dev.irq() { Ok(record) => Some(record), @@ -149,21 +146,11 @@ impl Virtio { None => say!("virtio-sound: device event {:#x} data={}", event.code, event.data), }) .unwrap_or_else(|why| panic!("soundserver: virtio-sound cannot be driven on — {why}")); - let mask = self + let (mask, record) = self .sound - .completed() - .unwrap_or_else(|why| panic!("soundserver: virtio-sound cannot be driven on — {why}")); - if mask == 0 { - return None; - } - let (first_nanos, last_nanos) = match record { - Some(record) => (record.first_nanos, record.last_nanos), - None => { - let now = toyos_abi::clock::nanos_since_boot(); - (now, now) - } - }; - Some(Completion { mask, first_nanos, last_nanos }) + .played(record) + .unwrap_or_else(|why| panic!("soundserver: virtio-sound cannot be driven on — {why}"))?; + Some(Completion { mask, first_nanos: record.first_nanos, last_nanos: record.last_nanos }) } /// Put period `idx` on the wire: its PCM descriptor is a whole period. diff --git a/userland/soundserver/virtio-sound/src/lib.rs b/userland/soundserver/virtio-sound/src/lib.rs index ae764b7328c..e4a5dec9a6e 100644 --- a/userland/soundserver/virtio-sound/src/lib.rs +++ b/userland/soundserver/virtio-sound/src/lib.rs @@ -316,8 +316,24 @@ impl Sound { Ok(()) } + /// The periods the device has given back that `taken` answers for, as a + /// mask beside it; nothing without a record. + /// + /// `taken` is the claim's interrupt record, taken before this call. The + /// device writes a used element before the notification that says so, and + /// with `avail.flags` zero and no `VIRTIO_F_EVENT_IDX` it owes one for + /// every element (§2.7.7), so a ring read after a record was taken holds + /// every period that record counts. With no record the ring is not read: a + /// period already in it has a notification still to land, which makes the + /// record readable again, and it is read then. + pub fn played(&mut self, taken: Option) -> Result, Refusal> { + let Some(record) = taken else { return Ok(None) }; + let mask = self.completed()?; + Ok((mask != 0).then_some((mask, record))) + } + /// Every period the device has given back since the last call, as a mask. - pub fn completed(&mut self) -> Result { + fn completed(&mut self) -> Result { let mut mask = 0; while let Some(Used { head, written }) = used(&mut self.queues.tx)? { // A head `toyos-virtio` answered is one a chain was published at, diff --git a/userland/soundserver/virtio-sound/src/tests.rs b/userland/soundserver/virtio-sound/src/tests.rs index 08a091d8631..6a2987812fb 100644 --- a/userland/soundserver/virtio-sound/src/tests.rs +++ b/userland/soundserver/virtio-sound/src/tests.rs @@ -567,3 +567,82 @@ fn an_event_is_handed_up_and_its_buffer_posted_again() { assert_eq!(sound.events(|event| said.push(event)), Err(Refusal::ShortEvent { written: 4 })); assert!(said.is_empty()); } + +/// The claim's interrupt record, as [`Sound::played`] is handed it: when the +/// oldest and newest notification since the last take landed. +#[derive(Default)] +struct Claim(Option<(u64, u64)>); + +impl Claim { + fn land(&mut self, at: u64) { + let first = self.0.map_or(at, |(first, _)| first); + self.0 = Some((first, at)); + } + + fn take(&mut self) -> Option<(u64, u64)> { + self.0.take() + } +} + +/// No period is stranded: every one comes back exactly once, and none while +/// the claim is left unreadable. +/// +/// Two periods, each a used element and then its notification (§2.7.7), +/// against two wakes of a driver that takes the record and then reads the +/// ring, in every interleaving; then the driver wakes for as long as the +/// claim reads ready, which is all a waiting soundserver does. A ring read +/// with no record that took what it found would leave a period nobody's +/// notification answers for, and the claim would never wake the driver for +/// it again. +#[test] +fn no_period_is_stranded_between_its_used_element_and_its_notification() { + #[derive(Clone, Copy)] + enum Step { + Used, + Notified, + Take, + Read, + } + const DEVICE: [Step; 4] = [Step::Used, Step::Notified, Step::Used, Step::Notified]; + const DRIVER: [Step; 4] = [Step::Take, Step::Read, Step::Take, Step::Read]; + + let mut orders = 0; + // Bit `n` of `pick` set: the `n`th step of the merge is the device's. + for pick in 0u32..1 << 8 { + if pick.count_ones() != 4 { + continue; + } + orders += 1; + let model = Model::new(); + let mut sound = opened(&model); + sound.submit(0).expect("a period goes out"); + sound.submit(1).expect("a period goes out"); + let mut claim = Claim::default(); + let (mut device, mut driver, mut landed) = (DEVICE.iter(), DRIVER.iter(), 0); + let mut taken = None; + let mut back = Vec::new(); + for n in 0..8 { + let step = if pick & 1 << n != 0 { device.next() } else { driver.next() }; + match step.copied().expect("four steps each") { + Step::Used => model.play(1), + Step::Notified => { + landed += 1; + claim.land(landed); + } + Step::Take => taken = claim.take(), + Step::Read => back.extend(sound.played(taken.take()).expect("a whole period")), + } + } + while let Some(record) = claim.take() { + back.extend(sound.played(Some(record)).expect("a whole period")); + } + let masks: Vec = back.iter().map(|(mask, _)| *mask).collect(); + let mut seen = 0; + for mask in &masks { + assert_eq!(seen & mask, 0, "order {pick:#010b}: a period came back twice in {masks:?}"); + seen |= mask; + } + assert_eq!(seen, 0b11, "order {pick:#010b}: the periods that came back are {masks:?}"); + } + assert_eq!(orders, 70); +} diff --git a/userland/toyos-pci-claim/Cargo.toml b/userland/toyos-pci-claim/Cargo.toml index 9a60f6c5dea..fa4dbdf34df 100644 --- a/userland/toyos-pci-claim/Cargo.toml +++ b/userland/toyos-pci-claim/Cargo.toml @@ -12,5 +12,10 @@ toyos-abi = { path = "../../toyos-abi" } toyos-device-memory = { path = "../../toyos-device-memory" } toyos-virtio = { path = "../../toyos-virtio" } +[features] +# `Grant::leaked`, for a driver crate's host tests: a dev-dependency's, never +# enabled by a build that ships. +host-grant = [] + [lib] doctest = false diff --git a/userland/toyos-pci-claim/src/lib.rs b/userland/toyos-pci-claim/src/lib.rs index 483ee7fbe36..eb8eeb69a16 100644 --- a/userland/toyos-pci-claim/src/lib.rs +++ b/userland/toyos-pci-claim/src/lib.rs @@ -87,12 +87,17 @@ impl Grant { // and `region` goes to the caller with the grant, which keeps both for // as long as it keeps either. let window = unsafe { Window::new(region.memory.as_ptr(), bytes as usize) }; - Ok((Self::over(window, region.device_addr), region)) + Ok((Self { window, device_base: region.device_addr }, region)) } - /// The grant over memory the caller already holds: a host test's plain - /// allocation, with the address a device would be told. - pub fn over(window: Window, device_base: u64) -> Self { + /// A grant over a leaked host allocation of `bytes`, zeroed, with the + /// address a device would be told: a driver crate's host test's. + #[cfg(feature = "host-grant")] + pub fn leaked(bytes: usize, device_base: u64) -> Self { + let backing = vec![0u64; bytes.div_ceil(8)].leak(); + // SAFETY: `leak` gives the allocation the `'static` lifetime the window + // needs, and it is at least `bytes` long. + let window = unsafe { Window::new(backing.as_mut_ptr().cast(), bytes) }; Self { window, device_base } } From d49182ce2235d2aec57fe3111a37ae981595570a Mon Sep 17 00:00:00 2001 From: japabu Date: Sat, 10 Oct 2026 10:27:00 +0200 Subject: [PATCH 6/6] toyos-pci-claim's host-grant arm is linted, and the virtio-clock issue is owned by HDA's move to a pci claim src/clippy.rs gains a shape for `-p toyos-pci-claim --all-targets --features host-grant` with the adopted lints and warnings denied, as toyos-xhci has for its own feature: `$GUESTS` excludes the crate from both workspace shapes and NESTED reaches only crates nested under a program, so `Grant::leaked` was linted by nothing. The review's named shape exits 101 as written: clippy then lints the crate's path dependency `toyos`, a guest crate no shape lints, which carries 11 findings. `--no-deps` lints the claim crate alone. That checks the host crates it shares with the workspace shapes (toyos-abi, toyos-device-memory, toyos-virtio, toyos-untrusted) with plain rustc, so the shape takes its own target directory, as toyos-abi's does, rather than flip those units between the two checks every run (22M). A redundant clone planted in `Grant::leaked` reds the shape's command (exit 101) and restores clean. issues/soundservers-virtio-clock-can-take-a-period-one-notification-early.md was owned by "whoever next changes" two sites. It is now owned by stage 1 of issues/every-driver-is-still-in-the-kernel.md: HDA leaving the kernel for a pci claim feeds soundserver's DLL from a claim's record on every machine, and that stage now names this exit. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017cSFvbD35xJ2kGANVdm23C --- issues/every-driver-is-still-in-the-kernel.md | 5 +++- ...an-take-a-period-one-notification-early.md | 6 +++-- src/clippy.rs | 23 +++++++++++++++++-- 3 files changed, 29 insertions(+), 5 deletions(-) diff --git a/issues/every-driver-is-still-in-the-kernel.md b/issues/every-driver-is-still-in-the-kernel.md index 7045934dab1..f302f2c88be 100644 --- a/issues/every-driver-is-still-in-the-kernel.md +++ b/issues/every-driver-is-still-in-the-kernel.md @@ -36,7 +36,10 @@ What is left of the staged work: and soundserver's virtio-sound driver do, retiring the `hda-audio` class, its arms of `SYS_DEVICE_REG_READ`/`WRITE`, and `SYS_GPU_*`, which is an ABI change. GOP stays: it is memory the loader hands over, and the panic console - paints it. + paints it. HDA leaving feeds soundserver's DLL from a claim's record on + every machine, so that diff lands + `issues/soundservers-virtio-clock-can-take-a-period-one-notification-early.md`'s + exit. A virtio holder stands on `toyos-virtio`, which walks the capability list too. Before a client ends its device on `UsedRefusal::Written` for a chain the device only reads, whose bound is 0, what QEMU's device reports as diff --git a/issues/soundservers-virtio-clock-can-take-a-period-one-notification-early.md b/issues/soundservers-virtio-clock-can-take-a-period-one-notification-early.md index a89d1db793f..4179c9f0e0c 100644 --- a/issues/soundservers-virtio-clock-can-take-a-period-one-notification-early.md +++ b/issues/soundservers-virtio-clock-can-take-a-period-one-notification-early.md @@ -22,8 +22,10 @@ back with no period. How often the window opens is unmeasured, and no metal machine runs virtio-sound. Nothing bounds the error but that rarity. -Owned by whoever next changes what a claim's record carries -(`kernel/src/pcidev/record.rs`) or how soundserver's DLL is fed. Exit: the +Owned by stage 1 of `issues/every-driver-is-still-in-the-kernel.md`: the +diff that moves HDA onto a `pci` claim feeds soundserver's DLL from a claim's +record (`kernel/src/pcidev/record.rs`) on every machine, and lands this exit +with it. Exit: the record ties a time to the used elements it answers for — or the driver holds back from a DLL update every period past those its record's notifications answer for — and `toyos-virtio-sound`'s interleaving test, diff --git a/src/clippy.rs b/src/clippy.rs index 7580c0f8147..fcabdb691e1 100644 --- a/src/clippy.rs +++ b/src/clippy.rs @@ -62,8 +62,10 @@ fn control_features() -> Vec { /// `undocumented_unsafe_blocks` is adopted per area as each area's /// justifications land. `toyos-xhci` has a shape of its own because the /// workspace run builds it only with `toyos-xhci-sim`'s `flaws`, never as the -/// kernel does. `kernel-loom` has one because `victim-retires-mid-probe`'s -/// test arm excludes `no-preempt-guard`, which `$CONTROLS` turns on beside it. +/// kernel does. `toyos-pci-claim` has one because `$GUESTS` excludes it and +/// only netstack's host tests build its `host-grant`. `kernel-loom` has one +/// because `victim-retires-mid-probe`'s test arm excludes `no-preempt-guard`, +/// which `$CONTROLS` turns on beside it. /// The kernel's library is linted on the host apart from them: its tests as /// `--ci host` runs them, and again with `$KERNEL_CONTROLS`. const SHAPES: &[Shape] = &[ @@ -151,6 +153,23 @@ const SHAPES: &[Shape] = &[ before: &["-p", "toyos-xhci", "--all-targets"], after: &["$ADOPTED", "-D", "warnings"], }, + // `--no-deps` because its dependency `toyos` is a guest crate no shape + // lints. Its own target directory because `--no-deps` checks without clippy + // the host crates the workspace shapes lint, which `toyos-abi`'s shape + // below says costs every run. + Shape { + before: &[ + "-p", + "toyos-pci-claim", + "--all-targets", + "--features", + "host-grant", + "--no-deps", + "--target-dir", + "target/clippy-pci-claim", + ], + after: &["$ADOPTED", "-D", "warnings"], + }, Shape { before: &[ "-p",