diff --git a/docs/bounds-track-plan.md b/docs/bounds-track-plan.md index 83da3ec..8ab7143 100644 --- a/docs/bounds-track-plan.md +++ b/docs/bounds-track-plan.md @@ -41,8 +41,8 @@ that cannot safely run in user space.** |---|---|---| | D1 | `device_transfer(device_id, task_id)` — the holder gives a device away | **done** — syscall 54; a move, not a copy | | D2 | Adversarial case: a process handed nothing is refused, on a held device and a free one | **done** — `device-authority-test`; the claim half joins it at D6 | -| D3 | The manager claims the seeded devices at boot, before any driver is spawned | not started | -| D4 | `usb-xhci-bus` receives its controller in the `hello` reply instead of claiming argv[1] | not started | +| D3 | The manager claims the seeded devices at boot, before any driver is spawned | **merged into D4** — see below | +| D4 | The manager claims + delegates on `hello`; `usb-xhci-bus` is the first driver converted | **done** — caught an IOMMU regression I introduced; see below | | D5 | The other four claimants converted: `pci-bus`, `ps2-bus`, `virtio-gpu`, `acpi` | not started | | D6 | `device_claim` refuses a device the caller was not handed; the hole is closed | not started | | D7 | Zero-resource devices stop being kernel objects — inventory moves to the manager | not started | @@ -50,11 +50,36 @@ that cannot safely run in user space.** | D9 | The device table becomes dynamic; **`maximum_devices` deleted**; per-holder quota declared | not started | Ordering is load-bearing. D1–D2 build and prove the mechanism with nothing depending on -it. D3–D5 move each claimant across one at a time, so the suite stays green throughout +it. D4–D5 move each claimant across one at a time, so the suite stays green throughout and a regression names the driver that caused it. D6 is the flag day. D7 must precede D9, because zero-resource children are the case that sidesteps containment and so the reason a shared cap was needed at all. +**D3 merged into D4** (found while implementing, recorded rather than worked around). +The two cannot be separated: the moment the manager claims a device, any driver still +calling `device_claim` on it is refused `AlreadyClaimed`, so D3 on its own turns the +suite red — and D3 applied to *nothing* changes no behaviour and cannot be tested. +They land together, with the manager claiming only for drivers in an explicit +**delegated set** so every unconverted driver keeps claiming exactly as before. +`usb-xhci-bus` is the first member, as it was the first driver to conform to `hello` +(device-manager.md, M18.1). D5 moves the rest in one at a time; the set and the +`device_claim` path both disappear at D6. + +### Observations from the run + +- **D4 introduced an IOMMU regression, caught by converting one driver at a time.** + `confineDevice` runs inside `systemDeviceClaim`, so a device arriving by *transfer* + was never confined for its new owner: the driver's DMA rings went unbound (three + IOMMU+USB cases failed), and two worse consequences were latent — a manager death + would have torn down a domain a live driver was using, and a driver death would have + leaked one. `iommu.reassign` moves the confinement with the device, keeping the + domain and attachment intact so it never translates through nothing. Converting all + five drivers at once would have produced the same three failures with five suspects. +- **`usb-hub` failed once in a full run, then passed six times** (four isolated, two + full). Suspected instance of the known intermittent AP ring-3 fault rather than + anything in D4 — recorded rather than dismissed, because D4 moved the `hello` earlier + and so did shift boot timing. Watch it across the remaining steps. + ### Settled, so the run does not re-litigate them - **The manager claims, it is not granted.** No binary names in the kernel; the rule is diff --git a/system/drivers/usb-xhci-bus/usb-xhci-bus.zig b/system/drivers/usb-xhci-bus/usb-xhci-bus.zig index be7a9b3..8fcb5fb 100644 --- a/system/drivers/usb-xhci-bus/usb-xhci-bus.zig +++ b/system/drivers/usb-xhci-bus/usb-xhci-bus.zig @@ -173,10 +173,22 @@ fn initialise(endpoint: ipc.Handle) bool { if (!channel.bindPatiently("usb-transfer", endpoint)) _ = logging.write("/system/drivers/usb-xhci-bus: /protocol/usb-transfer is another controller's; serving mine unnamed\n"); - device.claim(controller_id) catch |e| { - std.log.warn("unable to claim controller device {d}: {s}", .{ controller_id, @errorName(e) }); + // **The handshake comes first, because it is where the device arrives.** This + // driver used to claim `controller_id` here — first-come-first-served, so the + // manager's matching was advisory and any process could have claimed it by + // passing the same integer. Now the manager holds the controller and transfers + // it in `onHello`, so by the time this call returns the device is ours and + // nothing else could have taken it (docs/os-development/device-authority.md). + // + // `hello` is synchronous, so the transfer has completed before the reply lands — + // there is no window between being told yes and holding the thing. + // + // Keep the handle: the tick's hot-plug dispatch reports through it. + const handle = device_manager.hello(.bus, controller_id) orelse { + std.log.warn("no hello with the device manager; controller {d} not delegated", .{controller_id}); return false; }; + manager_handle = handle; // Fetch our own descriptor back for the controller's resources. const buffer = memory.allocator().alloc(device.DeviceDescriptor, 64) catch { @@ -204,7 +216,7 @@ fn initialise(endpoint: ipc.Handle) bool { std.log.info("controller device {d} has no register BAR", .{controller_id}); return false; }; - std.log.info("claimed controller device {d} (registers at 0x{x}, {d} bytes)", .{ + std.log.info("controller device {d} registers at 0x{x}, {d} bytes", .{ controller_id, register_window.start, register_window.len, @@ -247,12 +259,6 @@ fn initialise(endpoint: ipc.Handle) bool { return false; } - // The handshake (role: bus — we enumerate USB ports and report the devices - // behind them), inside the manager's hello deadline. Keep the handle: the - // tick's hot-plug dispatch reports through it. - const handle = device_manager.hello(.bus, controller_id) orelse return false; - manager_handle = handle; - scanPorts(handle); // Arm the timer: in polling mode it drains the event ring; in MSI mode it is the diff --git a/system/kernel/iommu.zig b/system/kernel/iommu.zig index 8c2c2ca..7166fc4 100644 --- a/system/kernel/iommu.zig +++ b/system/kernel/iommu.zig @@ -200,6 +200,26 @@ pub fn unmapRegionEverywhere(physical: u64, len: u64) void { /// A driver died or released its devices: tear down every domain it held (detach the /// device, free the tables) so their DMA is blocked again and a restarted driver /// re-claims cleanly. Runs BEFORE the broker claims and the DMA frames are released. +/// Re-point a device's existing confinement at a new owner, keeping its domain and +/// its attachment intact. +/// +/// Delegation needs this: the device manager claims a device (which confines it, with +/// the manager as owner) and then transfers it to the driver. Without moving the +/// confinement record too, the domain stays the manager's — so the driver's DMA +/// buffers are never bound into it, its rings are invisible to the device, and every +/// transfer faults. Worse, a manager death would then tear down a domain a live driver +/// is using, and a driver death would leave one behind. +/// +/// The domain is *not* rebuilt: the device stays attached throughout, so there is no +/// window in which it is translating through nothing. +pub fn reassign(device_id: u64, owner: u32) void { + if (!active) return; + if (device_id >= confined.len) return; + const record = &confined[@intCast(device_id)]; + if (!record.active) return; + record.owner = owner; +} + pub fn releaseAllOwnedBy(owner: u32) void { if (!active) return; for (&confined) |*c| { diff --git a/system/kernel/process.zig b/system/kernel/process.zig index c2d8923..5efecc8 100644 --- a/system/kernel/process.zig +++ b/system/kernel/process.zig @@ -461,6 +461,18 @@ fn systemDeviceTransfer(state: *architecture.CpuState) void { devices_broker.transfer(device_id, scheduler.current().id, task_id) catch |e| return failErr(state, devices_broker.transferErrnoOf(e)); + + // The device's IOMMU confinement moves with it. The giver confined it when it + // claimed, so the domain exists and the device stays attached — but the record + // still names the giver as owner, which would leave the receiver's DMA buffers + // unbound (every transfer faulting), a giver's death tearing down a domain the + // receiver is using, and the receiver's death leaving one behind. + if (devices_broker.pciAddressOf(device_id)) |_| { + iommu.reassign(device_id, task_id); + // Bind whatever the receiver has already allocated — the same courtesy the + // claim path does for a driver that dma_alloc'd its rings before claiming. + dmaBindOwnerRegionsInto(task_id, device_id); + } architecture.setSystemCallResult(state, 0); } diff --git a/system/services/device-manager/device-manager.zig b/system/services/device-manager/device-manager.zig index 6b87d14..92ec1e0 100644 --- a/system/services/device-manager/device-manager.zig +++ b/system/services/device-manager/device-manager.zig @@ -237,6 +237,25 @@ fn alreadySupervised(name: []const u8) bool { return false; } +/// Drivers that receive their device from the manager rather than claiming it +/// themselves. Scaffolding for the conversion, not a permanent concept: it exists so +/// each driver can move across one at a time with the suite green throughout, and it +/// disappears at D6 when `device_claim` stops being a way to acquire a device at all +/// (docs/bounds-track-plan.md, Run 2). +/// +/// `usb-xhci-bus` is first because it was the first driver to conform to `hello` +/// (device-manager.md, M18.1), so it is the one whose handshake is best proven. +const delegated_drivers = [_][]const u8{ + "/system/drivers/usb-xhci-bus", +}; + +fn isDelegated(name: []const u8) bool { + for (delegated_drivers) |candidate| { + if (std.mem.eql(u8, name, candidate)) return true; + } + return false; +} + /// Record a driver in the table and spawn its first instance. fn addDriver(name: []const u8, device_id: u64, speaks_protocol: bool) void { for (&drivers) |*driver| { @@ -257,6 +276,22 @@ fn addDriver(name: []const u8, device_id: u64, speaks_protocol: bool) void { /// device id as argv[1] when it has one, the hello deadline armed when it /// speaks the protocol. fn spawnDriver(driver: *Driver) void { + // Take the device before the driver exists, so there is no window in which anyone + // else could claim it — which is the whole of what makes the handover authoritative + // rather than advisory. Re-claiming across a restart is expected to say + // AlreadyClaimed once the manager already holds it, and that is fine: it means the + // device never left our hands while the driver was dead. + if (isDelegated(driver.name()) and driver.device_id != device_manager_protocol.no_device) { + device.claim(driver.device_id) catch |e| switch (e) { + error.AlreadyClaimed => {}, // ours already, from a previous spawn of this driver + else => { + std.log.warn("cannot hold device {d} for {s}: {s}", .{ driver.device_id, driver.name(), @errorName(e) }); + driver.state = .failed; + return; + }, + }; + } + var id_text: [20]u8 = undefined; var arguments: [1][]const u8 = undefined; var argument_count: usize = 0; @@ -424,6 +459,23 @@ fn onHello(_: void, invocation: Invocation(device_manager_protocol.Hello), _: An return -envelope.EPERM; }; driver.state = .running; + + // **Delegation.** The manager holds this driver's device and hands it over here — + // what replaces first-come-first-served `device_claim` with policy + // (docs/os-development/device-authority.md). `invocation.sender` is the driver's + // task id stamped by the kernel, so the manager cannot be lied to about who is + // asking, and the transfer is a move: the manager stops holding it. + // + // Gated on the delegated set so an unconverted driver still claims for itself and + // its path is untouched; the set and `device_claim` both go at D6. + if (isDelegated(driver.name()) and driver.device_id != device_manager_protocol.no_device) { + device.transfer(driver.device_id, invocation.sender) catch |e| { + std.log.warn("could not delegate device {d} to {s}: {s}", .{ driver.device_id, driver.name(), @errorName(e) }); + return -envelope.EPERM; + }; + std.log.info("delegated device {d} to {s}", .{ driver.device_id, driver.name() }); + } + std.log.info("hello from {s} (device {d})", .{ driver.name(), invocation.target }); // Resilience drill (V6): once, kill the virtio-gpu driver a moment after it hellos, so // the normal restart policy respawns it — the compositor must survive and re-attach. diff --git a/test/qemu_test.py b/test/qemu_test.py index b8b14af..e4b3f13 100644 --- a/test/qemu_test.py +++ b/test/qemu_test.py @@ -635,7 +635,13 @@ CASES = [ "smp": 4, "timeout": 150, # usb-kbd/usb-mouse ride the default boot xHCI bus (see qemu_args). - "expect": r"(?=[\s\S]*usb-xhci-bus: controller running \((\d+) slots, tracking \1,)" + # The controller must arrive by DELEGATION, not by claiming: the manager holds it + # and transfers it in the hello reply, so the match is authoritative rather than + # advisory (docs/os-development/device-authority.md). The delegation line must + # precede the hello, because the transfer completes before the reply lands. + "expect": r"(?=[\s\S]*device-manager: delegated device (\d+) to /system/drivers/usb-xhci-bus" + r"[\s\S]*usb-xhci-bus: controller device \1 registers at)" + r"(?=[\s\S]*usb-xhci-bus: controller running \((\d+) slots, tracking \2,)" r"(?=[\s\S]*usb-hid-keyboard: ok)(?=[\s\S]*usb-hid-mouse: ok)", "fail": r"DANOS-TEST-RESULT: FAIL"}, # Keyboard echo: inject a known phrase via QMP send-key; the usb-hid-keyboard