diff --git a/FABRIC-3.md b/FABRIC-3.md index 9f3d94e..682a0d2 100644 --- a/FABRIC-3.md +++ b/FABRIC-3.md @@ -987,38 +987,72 @@ and is no longer true, much in the same way a build flag for console."* — same correction as the headless-console change (§ pending its own FABRIC writeup): a simplification that was right when made, and is stale now that the project needs it to not be true. -### VII.2 — Exact scope of the singleton (full inventory, `include/starkernel/xhci_driver.h`) +### VII.2 — Exact scope of the singleton (full inventory, code-verified, not recalled) -`port_slot_id[]` is the only field in `xhci_dev_t` that is already an array. Every field -downstream of a successful Address Device is a single reused scalar, each with an explicit -"single-device scope" comment written at the time it was added: +`port_slot_id[]` (`xhci_driver.h:58`) is the only field in `xhci_dev_t` that is already an +array. Every field downstream of a successful Address Device is a single reused scalar, each +with an explicit "single-device scope" comment written at the time it was added: **Connect / enumerate state machine:** -- `connect_state`, `pending_connect_port_id`, `pending_connect_slot_id` -- `input_ctx`, `device_ctx`, `ep0_ring` — "only ever addresses one device at a time" +- `connect_state`, `pending_connect_port_id`, `pending_connect_slot_id` (`xhci_driver.h:59,70-79`) +- `input_ctx`, `device_ctx`, `ep0_ring` (`xhci_driver.h:91-95`) — "only ever addresses one + device at a time" **Control-transfer / descriptor state machine:** -- `transfer_purpose`, `pending_transfer_slot_id` -- `device_descriptor[18]`, `config_descriptor[128]` — "reused, not per-slot" +- `transfer_purpose`, `pending_transfer_slot_id` (`xhci_driver.h:113-126`) +- `device_descriptor[18]`, `config_descriptor[128]` (`xhci_driver.h:127-128`) — "reused, not + per-slot" - `bulk_in_ep_addr`/`bulk_out_ep_addr`, `bulk_in_ring`/`bulk_out_ring`, endpoint max-packet - fields + fields (`xhci_driver.h:139-157`) -**BOT (Bulk-Only Transport / SCSI) state machine:** -- `bot_cbw`/`bot_csw`, `bot_data_buf[1024]`, `bot_last_tag` +**BOT (Bulk-Only Transport / SCSI) state machine** (`xhci_driver.h:159-274`): +- `bot_cbw`/`bot_csw`, `bot_data_buf[1024]`, `bot_last_tag`, `bot_next_tag` - `bot_cmd_kind`, `bot_last_status`, `bot_expected_data_len` - `bot_read10_*`/`bot_write10_*` staging fields - `bot_tur_retries`, `bot_tur_chain_target` - `bot_cap_last_lba`/`bot_cap_block_size` -- Full stall-recovery sub-state-machine (G.1/§F.14): "this driver runs exactly one bulk +- Full stall-recovery sub-state-machine (`bot_stall_recoveries`, `stall_dci`, `stall_ep_addr`, + `bot_reset_clear_remaining`, `stall_retry_action`) — "this driver runs exactly one bulk transfer at a time... there is no concurrency to serialize" - `bot_msc_attach_pending`/`bot_msc_attach_slot_id`/`bot_msc_attached`/`bot_msc_detach_pending` - — the flags `repl.c`'s own `static blkio_dev_t usb_blk_dev` (single-device scope, matching - the xHCI driver's own precedent, per its comment at `repl.c:244`) depends on. + — the flags `repl.c`'s own `static blkio_dev_t usb_blk_dev` (single-device scope, + `repl.c:244`) and the two module-level pointers `g_homeblocks_dev` (`repl.c:122`) / + `g_attached_blk_dev` (`repl.c:139`) all depend on. + +**A third, previously-unlisted singleton point, found while tracing `xhci.c` for this +writeup:** `xhci_scan_ports_for_already_connected()` (`src/starkernel/usb/xhci.c:1343-1362`, +called once from `xhci_bringup()`) walks every tracked port looking for one already connected +at boot (needed because a device present on the QEMU command line before controller reset +never generates a Port Status Change *event* — confirmed against the xHCI event model, not +assumed) — and **`break`s after the first hit**, with its own comment stating why: "this +driver's real use case is exactly one thumbdrive already attached at boot, not several." A +second device already plugged in at boot (e.g. the mint workflow's Zuse-drive-plus-target- +drive scenario, both present from `qemu-xhci` at launch) would never be discovered by this +scan at all — not merely mishandled once found, genuinely invisible. This must be fixed +alongside the state-shape changes below, and is now folded into the punch list. + +**A structural finding that narrows the fix, not widens it:** `xhci_handle_port_connected()` +(`xhci.c:1300-1327`) already gates new Enable Slot requests on `connect_state == XHCI_CONN_IDLE` +and *drops* (does not queue) a second simultaneous connect with `"xhci: enable slot already +pending -- dropped"` — the same pattern repeats for disconnect (`xhci.c:1531-1539`, +`"disable slot skipped -- command ring busy"`). This confirms §VII.3's category-2 in-flight +state genuinely never needs true concurrency: the driver's own connect/disconnect handling is +already built to serialize one command at a time and simply discard what it can't yet serve +— the fix for these two sites is to *not drop the second event*, not to make the driver +concurrent (see punch-list item 3). + +**Also confirmed, a positive finding:** `block_subsystem.c`'s own attach layer +(`blk_subsys_attach_device()`, `block_subsystem.c:677`, and its `first_disk_slot()` device-slot +table) is **already multi-device-capable** — Artemis (virtio) and a USB MSC device already +coexist as independent attached devices there today. The bottleneck is confined to the xHCI +driver itself and the three singleton points immediately above it in `repl.c` +(`usb_blk_dev`/`g_homeblocks_dev`/`g_attached_blk_dev`) — `block_subsystem.c` itself needs no +change for this fix. This makes the fix smaller than initially framed in §VII.1/§VII.3. Also confirmed: this driver runs on **all three architectures**, not just amd64 — `qemu-xhci` is instantiated for amd64, aarch64, and riscv64 alike (`Makefile.starkernel` lines 819, 903, 993), and `xhci_find_and_map()`/`xhci_bringup()` are called unconditionally from -`kernel_main.c` (no `ARCH_*` guard). Any fix here is a three-architecture change, same +`kernel_main.c:653-656` (no `ARCH_*` guard). Any fix here is a three-architecture change, same acceptance bar as everything else in this document. ### VII.3 — The distinction that makes this tractable: persistent vs. in-flight state @@ -1044,13 +1078,38 @@ machine. Only category 1 needs to change shape. ### VII.4 — Concurrency bound -Decided 2026-09-05 (Bob): **a small N, matching a real xHCI root hub port count**, not a -hardcoded 2. `XHCI_MAX_TRACKED_PORTS` is already 32 (port/slot correlation layer) — the new -per-slot arrays (category 1 above) get their own bound, `XHCI_MAX_CONCURRENT_MSC` (proposed -default **4**), independent of and smaller than the 32-port tracking table, since concurrent -*mass-storage devices actually open at once* is a much smaller number than *ports the -hardware can electrically report*. Exact value is part of the punch list below (§VII.6 item -2), not fixed yet. +Revised 2026-09-05 (Bob, second pass): the earlier framing on this page — "a small N, +matching a real xHCI root hub port count," floated with a proposed default of 4 — is +rejected. Bob's ruling: *"it might again, been true at one time, but we REALLY need to not +paint ourselves into ANY scale issues by taking an easier way out."* This is the exact +pattern this section itself exists to correct (a deliberate simplification, right when made, +now stale) — hardcoding a second small ceiling right next to the one just being removed would +just relocate the same mistake, not fix it. + +**Corrected direction, and a correction to this page's own first draft of that direction:** +the per-slot registry (category 1 state, §VII.3) must not carry a small fixed-size array at +all — but the fix is not "start reading `HCSPARAMS1.MaxSlots`," because **that register is +already read, today**: `xhci_find_and_map()` (`xhci.c:68-71`) already does +`dev->max_slots = XHCI_HCSPARAMS1_MAX_SLOTS(hcs1)`, and `xhci_bringup()` already uses it +correctly — the DCBAA (`xhci.c:188-198`) is allocated at exactly `(dev->max_slots + 1) * +sizeof(uint64_t)` via `kmalloc_aligned()` (a real, hardware-sized heap allocation, not a fixed +array), and `dev->op->config` (`xhci.c:299`) is programmed with +`XHCI_CONFIG_MAX_SLOTS_EN(dev->max_slots)` to tell the controller the same number back. **The +slot-correlation layer of this driver is already correctly scaled to real hardware and needs +no change.** The bug is narrower and more specific than "the driver doesn't know how big the +hardware is": `dev->max_slots` is known and used correctly for the DCBAA, but the category-1 +fields (§VII.3) never use it at all — they're scalars regardless of what `max_slots` says, +because they were written before per-slot addressing was a hardware value from anywhere +convenient to size against. + +**Corrected punch-list direction:** size the new per-slot registry for category-1 state off +`dev->max_slots` — the same field already driving the DCBAA allocation — via the same +`kmalloc_aligned()`-at-bringup pattern the DCBAA itself already establishes as this driver's +precedent for hardware-sized allocation, rather than introducing a second, separately-derived +bound or a fixed-size array at all. (`XHCI_MAX_TRACKED_PORTS`, 32, `xhci.h:515`, is a +*different* table — port-to-slot correlation, sized as "comfortably covers any real root hub," +by its own comment's admission a chosen convenience constant, not hardware-derived — and +should not be reused or treated as precedent for this one.) ### VII.5 — Bob's live suggestion: consolidate into one file, pass state explicitly @@ -1062,15 +1121,18 @@ isn't, before this becomes a punch-list item: `xhci_ep0_*`) already takes an explicit `xhci_dev_t *dev` parameter — this is already dependency-injection-shaped at the call-site level, not a global-variable design. - **Not true — the actual singleton:** `xhci_poll_events()` itself takes **no** arguments and - reads a module-static `xhci_dev_t*` via `xhci_get_dev()`; `repl.c`'s `sk_repl_idle()` and - `kernel_main.c`'s bring-up call site are the only two places that hold a real handle, and - everything else (the entire event-driven dispatch chain) resolves the device through that - static rather than being handed it. That static-singleton *retrieval* pattern is the one - thing genuinely worth removing as part of this — moving from "one implicit global device" - to "one explicit registry of up to `XHCI_MAX_CONCURRENT_MSC` devices, looked up by slot ID, - passed explicitly" is the natural per-slot-array design from §VII.3 *and* answers Bob's - "calling from the dependency" framing at the same time — no separate mechanism needed for - both goals. + reads a module-static `xhci_dev_t *g_xhci_dev` (`xhci.c:1482`) set once by `xhci_bringup()`; + `xhci_get_dev()` (returning that same static) is used by exactly two callers — + `kernel_main.c`'s bring-up block and `repl.c`'s `sk_repl_idle()` (`repl.c:243`) — and + everything else (the entire event-driven dispatch chain inside `xhci_poll_events()` itself) + already has `dev` in hand as a real parameter, it just came from that one static originally. + This static-singleton *retrieval* pattern is not actually the multi-device blocker (only one + physical xHCI *controller* is ever supported or claimed to be — that premise is unchanged + and correct, `virtio_blk.c` uses the identical single-controller-static precedent) — the + blocker is entirely inside `xhci_dev_t`'s own fields (§VII.2/§VII.3), one controller struct + holding scalar per-*device* state instead of per-slot arrays. Bob's "calling from the + dependency" framing is already satisfied at the controller level; the fix below applies the + same idea one level down, to the device slots living inside that one controller. - **File consolidation (`xhci_driver.h` + `src/starkernel/usb/xhci.c` → one file):** a separate, smaller question from the state-shape fix above, genuinely optional. Current split is ordinary header/implementation separation, already followed throughout `src/starkernel/` @@ -1078,39 +1140,88 @@ isn't, before this becomes a punch-list item: the split and judging "simpler" by whether the *singleton* is gone, not by file count — but this is Bob's call, not a default to override. Tracked as punch-list item 6 (optional). -### VII.6 — Punch list (numbered = proposed execution order; nothing here has been started) +### VII.6 — Punch list (numbered = proposed execution order; nothing here has been started; +each item names the exact functions/fields it touches, traced against real code, not estimated) -1. Introduce `XHCI_MAX_CONCURRENT_MSC` (proposed 4) and convert every category-1 field - (§VII.3) in `xhci_dev_t` from a scalar to a `[XHCI_MAX_CONCURRENT_MSC]` array indexed by a - new small "MSC slot index" (not the raw xHCI hardware slot ID, which can be sparse/large) — - endpoint addresses/max-packet, bulk rings, device/config descriptors, MSC attach flags. -2. Add the index-allocation/lookup pair (`xhci_msc_slot_for(slot_id)` / - `xhci_msc_slot_alloc(slot_id)` or equivalent) that maps an xHCI hardware slot ID to an MSC - slot index, replacing the current implicit "there is only one" assumption everywhere a - category-1 field is touched. -3. Thread an explicit slot selector through the category-2 in-flight fields (§VII.3) so the - existing one-transfer-at-a-time state machine knows *which* device's transfer is in flight, - without itself becoming concurrent. -4. Replace `xhci_get_dev()`'s single-static-pointer retrieval with the explicit per-slot - registry from item 1/2, updating the two real call sites (`kernel_main.c` bring-up, - `repl.c`'s `sk_repl_idle()`) and `xhci_poll_events()`'s own internal dispatch to look up by - slot instead of assuming the one device. -5. Update `repl.c`'s `static blkio_dev_t usb_blk_dev` (currently single-device scope, - `repl.c:244`) to the same `[XHCI_MAX_CONCURRENT_MSC]` shape, and update - `capsule_wirebind.c`/`capsule_zuse_boot.c`'s attach call sites accordingly — this is the - layer the actual identity-login (WIREBIND/Zuse) flow depends on, and the reason this fix - exists at all. +1. **New `xhci_msc_slot_t` per-slot record type, registry array sized off `dev->max_slots`.** + Define one struct bundling every category-1 field currently scalar in `xhci_dev_t` + (`xhci_driver.h:91-95` connect/enumerate: `input_ctx`, `device_ctx`, `ep0_ring`, + `ep0_ring_cycle`, `ep0_ring_enq`; `xhci_driver.h:127-157` control/bulk: `device_descriptor`, + `config_descriptor`, `config_total_length`, `bulk_in_ep_addr`/`bulk_out_ep_addr`, + `bulk_in_max_packet`/`bulk_out_max_packet`, `bulk_in_ring`/`bulk_out_ring` + + cycle/enqueue state; `xhci_driver.h:285-294` MSC attach: `bot_msc_attach_pending`, + `bot_msc_attached`) plus a `uint32_t slot_id` tag (0 = unused slot). Add + `xhci_msc_slot_t *msc_slots;` and `uint32_t msc_slot_count;` to `xhci_dev_t`, replacing the + individual fields listed above (removed from the struct, not left dead alongside the new + ones — this project's own no-dead-code convention). Allocate in `xhci_bringup()` + immediately after `dev->max_slots` is known (`xhci.c:69`, already set by + `xhci_find_and_map()` which always runs first) via `kmalloc_aligned(sizeof(xhci_msc_slot_t) + * (dev->max_slots + 1), ...)`, zeroed — the exact same sizing input and allocation call + `xhci.c:188-198`'s DCBAA already uses, so this item adds no new sizing policy, just a + second allocation using the existing one's already-correct input. +2. **`xhci_msc_slot_for(dev, slot_id)` lookup, replacing every direct field access.** One + function: bounds-check `slot_id <= dev->max_slots`, return `&dev->msc_slots[slot_id]` (or + NULL out of range). Every one of the ~15 category-1 fields' current access sites across + `xhci.c` (`xhci_cmd_address_device()` at `xhci.c:453` onward, `xhci_cmd_configure_endpoint()` + at `xhci.c:554`, the `xhci_bot_*`/`xhci_ep0_*` families at `xhci.c:753-1266`, and the + completion handlers inside `xhci_poll_events()` at `xhci.c:1480-2175`) changes from + `dev->field` to `xhci_msc_slot_for(dev, slot_id)->field` — every one of these call sites + already receives `slot_id` as a parameter today (confirmed: every `xhci_cmd_*`/`xhci_bot_*`/ + `xhci_ep0_*` function signature already takes `uint32_t slot_id`), so this item is a + mechanical field-access rewrite, not a new parameter-threading exercise. +3. **Stop dropping simultaneous connect/disconnect instead of building real concurrency.** + `xhci_handle_port_connected()` (`xhci.c:1300-1327`) currently drops a second connect with + `"enable slot already pending -- dropped"` when `connect_state != XHCI_CONN_IDLE`; the + disconnect path (`xhci.c:1531-1539`) drops the same way with `"disable slot skipped -- + command ring busy"`. Per §VII.2's finding, this in-flight state genuinely stays + single-outstanding (one Command Ring, one command at a time, matching real xHCI command + submission) — the fix is a **pending-request queue of depth `dev->max_slots`** (a small + array of `{port_id, is_connect}` entries, not a state-machine rewrite): a dropped connect/ + disconnect is queued instead of discarded, and drained one entry at a time as + `connect_state` returns to `XHCI_CONN_IDLE` (the same point that already exists at + `xhci.c:1594`, `:1610`, `:1611` — add a "drain one queued request" call there). This + directly fixes the currently-real bug where two thumbdrives connected close together (well + within human/QMP timing) can silently lose the second one today, independent of the + category-1 field-shape fix in items 1-2. +4. **Fix `xhci_scan_ports_for_already_connected()`'s single-hit `break` (`xhci.c:1343-1362`).** + Remove the `break` at `xhci.c:1360` so the boot-time already-connected scan drives *every* + tracked port with `CCS` set through `xhci_handle_port_connected()`, not just the first. + Since item 3's queue now exists, a second (or third) already-connected device found here + queues cleanly instead of needing its own separate handling — this item has no correctness + force of its own once item 3 lands, it is purely "stop deliberately stopping early." +5. **Update `repl.c`'s three singleton points to arrays over `xdev->max_slots`.** + `static blkio_dev_t usb_blk_dev` (`repl.c:244`), `static blkio_dev_t *g_homeblocks_dev` + (`repl.c:122`), and `static blkio_dev_t *g_attached_blk_dev` (`repl.c:139`) all currently + assume one attached USB MSC device. Convert `usb_blk_dev` to an array indexed the same way + as item 1's `msc_slots` (by `slot_id`, bounds-checked against `xdev->max_slots`); + `g_homeblocks_dev`/`g_attached_blk_dev`'s callers (`sk_get_homeblocks_dev()`/ + `sk_get_attached_blk_dev()`, whichever functions wrap `repl.c:127`/`:142` today) need to + become "does a homeblocks/attached device exist for *this* identity/slot" rather than "the + one homeblocks/attached device" — the real consumers are `capsule_zuse_boot_try_attach()` + and `capsule_wirebind_try_attach()` (`repl.c:300`, `:311`), which is exactly the layer the + identity-login (WIREBIND/Zuse) flow depends on and the reason this whole fix exists. Per + §VII.2, `block_subsystem.c`'s own `blk_subsys_attach_device()`/`first_disk_slot()` layer + needs **no** change — it already supports multiple simultaneously-attached devices; only + the `repl.c` glue feeding it is the singleton. 6. *(Optional, Bob's call per §VII.5, not required for correctness)* — consolidate - `xhci_driver.h` + `xhci.c` into one file, if the per-slot-registry change above doesn't - already read as "simple enough" on its own. -7. Three-architecture acceptance (`clean qemu`, amd64 → aarch64 → riscv64, sequential, - foreground) confirming: (a) existing single-device behavior is unchanged when only one - device is ever attached (no regression), (b) two devices attached simultaneously (QMP - `device_add` twice against `xhci0.0`/`xhci0.1` without an intervening `device_del`) both - enumerate, both open as BOT/SCSI devices, and neither corrupts the other's descriptors/ - rings/BOT state. + `xhci_driver.h` + `src/starkernel/usb/xhci.c` into one file, if items 1-5 together don't + already read as "simple enough" on their own. Recommendation stands: keep the split, judge + simplicity by whether the singleton is gone (it will be), not by file count. +7. **Three-architecture acceptance** (`clean qemu`, amd64 → aarch64 → riscv64, sequential, + foreground, per this project's non-negotiable acceptance bar) confirming both directions: + (a) **no regression** — existing single-device behavior (the mandatory `ZUSEDISK_QEMU_ARGS` + attach path every current boot already exercises) is unchanged when only one device is ever + attached; (b) **the actual fix** — two devices attached simultaneously (a second + `-device usb-storage,bus=xhci0.0,drive=...` added to the existing QEMU launch args, or via + QMP `device_add` against a second port on `xhci0.0` without an intervening `device_del`) + both enumerate, both reach `SET_CONFIGURATION`, both open as BOT/SCSI devices via + `blkio_usb_open_msc()`, and neither's `msc_slots[]` entry (item 1) shows corruption from the + other's descriptors/rings/BOT state — verified by log inspection (each device's own + `bot_cap_last_lba`/`bot_cap_block_size` reported correctly and independently), not just + "both attached without crashing." 8. Only after item 7 passes: resume the identity-thumbdrive minting workflow — either via true - simultaneous multi-device attach (Zuse's drive + a target drive both present at once) or the - original QMP hot-swap sequencing (still valid, just no longer the only option). + simultaneous multi-device attach (Zuse's drive + a target drive both present at once, the + scenario item 4's fix specifically enables) or the original QMP hot-swap sequencing (still + valid, just no longer the only option) — Bob's call at that point, not assumed now. **Halt point:** implementation does not begin until Bob says so.