FABRIC-3.md §VII: rewrite punch list to real code-level detail
Build / build-amd64-iso (push) Waiting to run
Build / build-aarch64-iso (push) Waiting to run
Build / build-riscv64-img (push) Waiting to run

Previous pass was too abstract for this series' own bar. Re-traced
xhci.c/repl.c function-by-function: found max_slots is already read and
correctly sizes the DCBAA (so the fix reuses that value, not "start
reading a register"); found xhci_scan_ports_for_already_connected()
deliberately breaks after the first hit, missing a second already-
connected device at boot entirely; found block_subsystem.c's attach
layer is already multi-device-capable, narrowing the real singleton to
xhci_dev_t's own fields plus three repl.c pointers. Punch list items now
name exact functions/fields/line numbers and what's proposed vs. already
true. Also drops the earlier small-fixed-N concurrency bound per
direct correction -- size off dev->max_slots, not a guessed ceiling.

Still plan-only. No driver code touched.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018EjXFo7mPXjUMjfJeuUUz4
This commit is contained in:
Robert Allan James
2026-09-05 21:00:45 -04:00
co-authored by Claude Sonnet 5
parent c3db963164
commit cc6fcb6a0b
+173 -62
View File
@@ -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.