[Japabu]

Head 5cdf3c9bc on main 34cb4c918. Commits: osbench's after-idle row and Linux's APST read; the driver's APST; the records of what the measurement found; the plan boxed (superseded); the first review's code fixes; its record fixes and the track's close; the follow-up review's test names and comments.

Problem

ToyOS's NVMe driver never set Autonomous Power State Transitions, so a drive stayed in its operational state whenever it was idle. That was the track issues/diskserver-drives-nvme-apst.md, which this PR closes.

Behavior

• Where Identify Controller's APSTA says the controller supports APST, the driver sets Feature 0Ch (NVM Express Base 2.1 §5.1.25.1.6, §8.1.17.2), at bring-up and after every reset. It logs every power state and the table. A controller that refuses the table is reported, at bring-up and after a reset, and served from its operational state.

• Identify Controller is validated once. An NPSS above 31 names more states than the page holds descriptors for: the driver reports it and serves without APST.

• The table is built by system/nvme/apst from the drive's own descriptors, once, with no table of drives. A target is a non-operational state that states both latencies. Each state goes to the highest-numbered target after it within the first tier that holds one, reporting no higher idle power:

  • a round trip (entry plus exit latency) within 15 ms, after 100 ms idle;
  • else one within 100 ms (ROUND_TRIP_BOUND_US), after 2 s.

  States are ranked by number because the specification numbers them so that none consumes more maximum power than the one before (§8.1.17); latencies carry no order. A table with no transition is sent with APSTE clear.

• The tiers are kept, by the orchestrator's decision after the A/B: they give the same table Linux sets on both benches' drives, 0,1,2,3->4 after 100 ms.

• osbench (first commit, no driver change): persist.write_4k_fsync_after_idle times the first durable write after 10 s idle on both systems, and Linux's nvme group reads the controller's APST table through the UAPI admin passthrough.

• The manual's power section gains the decision.

Measured: it meets the landing rule on both benches

Three boots of the driver's APST (047e0a708) against three of the same image without it (349ccb26b), interleaved, on each bench, with Linux's persist between rounds and its nvme read last. Every step ran once. Rules written before staging and amended before any boot. Evidence: https://github.com/ToyOSOrg/evidence/blob/4ee8f9357170c27c4faa80e2d0be38d2f139d387/r10/README.md

rule	AMD	T14
no back-to-back disk row over 5% worse and resolved	pass: nothing moved more than 4%	pass: fs.disk.seek is 1% worse and resolved, write_4k_fsync 5% worse and not resolved; six rows better and resolved
every boot logs the table Linux reads	pass	pass

Both systems set 0,1,2,3->4 after 100 ms on both drives.

The review's fixes leave both benches' tables as measured. Both drives report NPSS 4, list their latencies rising with state number and never reset during the runs. A host test over each drive's logged descriptors pins 0,1,2,3->4 after 100 ms under the new ranking.

Reported, not gated: the first durable write after idle (median of p50s):

bench	with APST	Linux	over Linux	worst sample, ToyOS / Linux
T14	14.31 ms	13.02 ms	+1.29 ms, 10%	18.34 / 13.75 ms
AMD	11.33 ms	10.70 ms	+0.63 ms, 6%	11.78 / 11.83 ms

• The wake, which Linux pays too, is about 7.1 ms on the AMD (11.33 against 4.19 ms without APST). On the T14 the image without APST ran in the drive's slow write mode in every boot, so it measures no wake. Within the fast mode, the APST boot that ran fast throughout read 14.20 ms after idle against 1.10 ms back to back, about 13.1 ms, and Linux about 12.0 ms.
• The remainder over Linux is recorded as its own defect with an exit: issues/the-first-durable-write-after-the-drive-wakes-is-a-millisecond-behind-linuxs.md. The commit's second Flush is its first suspect; that path has not been split again since Consolidate storage around native bcachefs and userland filesystems #1152.
• The AMD is at 6% over Linux's median, one point past the 5% the rules named for "about parity", with every ToyOS boot inside the range of Linux's runs (10.58 to 11.47 ms). The orchestrator ruled it parity for this purpose.

Consistent with the data, not proved: APST ends the T14 drive's slow write mode. The T14's durable writes run about six times slower in some boots, on both systems. Of the boots that began persist slow:

the boot had	ran every durable row after the idle row fast
APST and 80 s of idle	2 of 2 here, and 4 of 4 in the earlier A/B
80 s of idle, no APST	0 of 3
neither	1 of 11, by itself (seven boots of the earlier A/B and the four main boots of the 2026-11-11 scoreboard, #1217)

• Three boots an arm cannot prove it. The reading is that the drive leaves the mode when it enters a non-operational state.
• It is why six T14 rows are better with APST and resolved: append_4k_fsync 0.98 against 6.75 ms, create_fsync_unlink 1.16 against 7.44 ms, write_4k_fsync_beside_1m 1.28 against 7.70 ms, seq_write_fsync 259 against 191 MB/s.
• APST does not keep a boot out of the mode: two of the three boots with it began slow, before any idle. Linux shows the same shape.
• Recorded in issues/toyos-durable-writes-on-the-t14s-nvme-run-six-times-slower-in-some-boots.md; the finding that asked the question (osbench's disk groups as a metal row, and the forced journal write recorded as measured and declined #1215) is folded into it and removed.

The track's close. Its exit is met: host tests of the table; its mutation, a table admitting a state past the bound, fails the bound test (r12); the T14's block figures with no row regressed and the table Linux sets (r10); the idle minute, measured in r1 to r4, whose not falling is issues/the-t14s-idle-package-stays-in-pc2.md's; the specification's rules as the oracle. The drive's own rail is outside the package energy osbench reads, so the drive's saving rests on its descriptors (PS4 at most 5 mW) and on the table matching Linux's; the pc2 record says so.

Revisions. The measured images are 349ccb26b and 047e0a708, on main ad5db5a65. Their code is 669a096f6 and 332cf33c5 here, unchanged by the rebase onto 34cb4c918 (the non-comment lines of each commit's diff are identical). 902ca4e30 boxed the plan; 56318e297 replaces it and fixes what the review found, and changes no table either bench gets.

Checks

Targeted at what the PR touches, under the owner's testing policy; the merge queue runs the full host checks and the image. No metal run is repeated: the fixes change no table either bench gets, nor any path either took. The guest row is below.

Logs: r12 https://github.com/ToyOSOrg/evidence/blob/e267c4c96d03ffbe9593fe2384fc3929f39a7fc6/r12/README.md (at a84c64a88) and r13 https://github.com/ToyOSOrg/evidence/blob/ef7ae674c238167d0a4298ab3790c7e98b624de2/r13/README.md (at 5cdf3c9bc).

check	at a84c64a88
cargo test -p nvme-apst	exit 0, 12 passed; again at 5cdf3c9bc, exit 0, 12 passed
cargo test --manifest-path system/nvme/Cargo.toml (the driver, on the host)	exit 0, 5 passed
cargo clippy -p nvme-apst --all-targets, with the adopted lints and warnings denied	exit 0; again at 5cdf3c9bc, exit 0
the same clippy on the driver	exit 0
mutation: the last tier admits any round trip (a table admitting a state past the bound)	exit 101: only the bound test fails
mutation: within a tier, the longest round trip wins, as before the review	exit 101: the inverted-latency test and the NPSS-31 test fail; both bench tests still pass
mutation: no refusal of an NPSS past 31	exit 101: only the NPSS test fails

Each mutation was applied, tested and reverted in the same run, and the source's hash was checked afterwards. 5cdf3c9bc changes only test names, comments and the manual, so the driver's rows and the mutations at a84c64a88 still apply. No gate lints the driver: the merge gate's clippy leaves out every userland program (issues/userland-programs-are-never-linted.md).

Earlier, and why each still applies:

• At 5418cb1a5 (r11): cargo clippy -p osbench on both Linux musl targets exit 0, and cargo test --test toyos-checks exit 0 (44 passed). This round changes neither osbench nor the harness.
• Guest row nvme_disk_boots, PASS at 047e0a708 (run 42983382300): with the driver's new bring-up step, QEMU's controller still boots to ready. The bring-up path has changed since: Identify builds the plan, a table with no transition is sent with APSTE clear, and open and reset return the controller's answer. So the row is dispatched again at a84c64a88, run 43058169740. It passed: PASS nvme_disk_boots, test result: ok. 1 passed, 1 total. 5cdf3c9bc changes no code it builds.
• cargo run -- --build-only was last run at 047e0a708, exit 0. This head's image is built by the merge queue.

Evidence

• r1-r7 (the earlier idle-time multiples, the T14 diagnosis, Linux's table, the split of the after-idle write): branch pr-1131 of the evidence repository, r6 at https://github.com/ToyOSOrg/evidence/blob/c2f9be162f22bb54e90a1d14b5091cf6ab0ad785/r6/README.md, r7 at https://github.com/ToyOSOrg/evidence/blob/020864e5c9d0d3c1d7bac2383d84f26d2651f17c/r7/README.md
• r8 and the A/B on A journal entry is one flush and a forced write: FUA through the block path #1202's base: https://github.com/ToyOSOrg/evidence/blob/7efbe1295aa54370b213719b4d4f460e7a103554/ab/README.md
• r9 (the staged request and its rules) and r10 (the measurement judged): https://github.com/ToyOSOrg/evidence/blob/4ee8f9357170c27c4faa80e2d0be38d2f139d387/r10/README.md
• r11 (the rebased head's targeted host checks): https://github.com/ToyOSOrg/evidence/blob/37ec860eb72a2b24e48926b584fb92b99469bb6b/r11/README.md
• r12 (the review round's checks and mutations): https://github.com/ToyOSOrg/evidence/blob/e267c4c96d03ffbe9593fe2384fc3929f39a7fc6/r12/README.md
• r13 (the follow-up's checks): https://github.com/ToyOSOrg/evidence/blob/ef7ae674c238167d0a4298ab3790c7e98b624de2/r13/README.md

Risks

• That a tier compares the round trip (entry plus exit) is this PR's reading; the specification bounds no latency.
• Idle power is not re-measured. r1-r4 found APST did not lower the T14's idle package under ToyOS (issues/the-t14s-idle-package-stays-in-pc2.md); that record owns it.
• Three boots an arm. The back-to-back verdicts are firm on the AMD, where the arms agree to a few percent. On the T14 the drive's mode moves rows by a factor of six, and what the verdict there rests on is that nothing got worse.
• The paths no bench takes (an NPSS past 31, a refusal, a table with no transition, a reset) are established by host tests of the table and by reading the driver, not by a run.

🤖 Generated with Claude Code

[Japabu]

Review of #1131 at 902ca4e30

SEND BACK. Three code defects and two record or comment corrections. None of the fixes needs a new metal run. Both benches' drives report NPSS 4, list their latencies in rising state order and never reached the reset path. Their tables and the measured behaviour are therefore unchanged by the fixes, and host tests cover each one.

Architecture

The ownership is right. The controller's power policy lives in the one process that holds the claim and reads the descriptors. Neither the kernel nor the block service is touched, and nothing joins toyos-abi.

nvme-apst is a pure, forbid(unsafe_code) crate nested in the driver. It has a real input boundary: it parses Identify Controller data. As a nested crate it is a host member (build/hostws.rs guest_built), so the gate tests and lints it, unlike the driver.

APST is the specification's hardware-autonomous mechanism, simpler than host-timed Set Features (Power Management). The manual's new Power bullet states the design without naming code and fits the storage page's one driver per controller. "No table of drives" is the track's decision.

Size:

• about 150 lines of table code and 89 in the driver;
• 108 lines of osbench measurement code: the Linux APST read and the after-idle row;
• 144 lines of unit tests;
• records +118/−56.

Nothing is disproportionate.

Required

1. An out-of-spec NPSS panics the driver. system/nvme/apst/src/lib.rs:88-99, :111-113; system/nvme/src/nvme.rs:418.

NPSS is device data, and the specification allows at most 32 states (Figure 312, byte 263: "may support up to 31 additional power states"). states checks only that the buffer can hold 32 descriptors, then slices NPSS + 1 of them. Run against this head's crate:

• NPSS 255 panics at lib.rs:93: "range end index 10240 out of range for slice of length 4096". Any NPSS of 64 or more does this. It happens in identify(), before APSTA is read, so a controller without APST, which the driver served before this PR, now crashes it at bring-up.
• NPSS 40 parses 41 states, then plan panics at lib.rs:127: "index out of bounds: the len is 32 but the index is 32".

The driver's own boundary already refuses malformed Identify fields (sector_shift, page_size, MDTS).

Fix: refuse NPSS > 31 in states. Then either report it and serve without APST, or refuse the controller. Add a unit test at NPSS 32. Taking &[u8; 4096] would make the remaining length check, and the expect, unnecessary.

2. "Deepest" is ranked by round-trip latency, which the specification does not order states by. system/nvme/apst/src/lib.rs:111-133, header :13-19.

§8.1.17 orders states by index: "each subsequent power state consumes less than or equal to the maximum power consumed in the previous state". Latency carries no such order.

When a drive's latencies fall with index, which the specification permits, the plan picks a dominated target and moves away from the lowest-power state. A probe against this head used three states:

• PS0: operational;
• PS1: non-operational, MP 0.04 W, 5000/5000 µs;
• PS2: non-operational, MP 0.01 W, 1000/1000 µs.

plan returns 0->1, 1: none, 2->1, each after 100 ms. The drive idles in the state with more power and a slower exit, and never reaches PS2. If it were in PS2, it would leave for PS1.

Using the specification's own order also removes the floor mechanism:

• a state's candidates are the admitted states with a higher index;
• within the first tier that holds one, take the highest index.

Both benches' drives list their latencies in rising state order, so their tables stay 0,1,2,3->4 after 100 ms and r10 stands:

• T14: PS3 round trip 2 ms, PS4 6 ms;
• AMD: PS3 1.41 ms, PS4 10 ms (r10 rows/*-r1-P.txt).

Pin that with a unit test over the AMD descriptors beside the BC501 test, and add a test for the inverted set.

3. A refusal after a reset is never reported. system/nvme/src/nvme.rs:749; system/nvme/src/main.rs:272-288.

reset stores set_power's result in ctrl.power. Only say_power reads that field, once, at bring-up. A controller that accepted the table at bring-up and refuses it after a reset therefore loses APST silently. That contradicts the PR's "a controller that refuses the table is reported", and Fail clearly's rule against hiding a failure.

Fix: report the outcome on the reset path too, for example in reset's line, or by calling say_power when the result is not Autonomous.

4. The T14 wake figure subtracts across the drive's slow mode. issues/the-first-durable-write-after-the-drive-wakes-is-a-millisecond-behind-linuxs.md:24-26.

The record says "Without APST the same row reads 5.95 ms (T14) … the wake itself costs about 8 … ms". Every M' T14 boot ran slow throughout: 5.74–6.56 ms back to back, and 5.78–6.23 ms after idle. So P' − M' compares a fast-mode wake with a slow-mode write.

Measured within one mode:

• r1-P', fast throughout, reads 14.20 ms after idle against 1.10 ms back to back: a wake of about 13.1 ms;
• Linux L1 gives 13.02 − 0.98 ≈ 12.0 ms.

The AMD's 7.1 ms has no mode confound and stands. Correct the T14 figure or drop it. The record's remainder, +1.29 ms against Linux, does not depend on it. The PR text's "the wake costs 8.4 ms (T14)" has the same error, and correcting it is the orchestrator's.

5. Two comments are false as written.

• system/nvme/src/nvme.rs:24-28 cites §5.1.25.1.6 and §8.1.17.2 under a module header that says "NVMe Base Specification 2.0 throughout" (:1). Those are 2.1's numbers. 2.1 puts the admin commands under "5.1 Common Admin Commands", while this file's own 2.0 citation for Identify is §5.17.2.1. Write "Base 2.1" in that paragraph.
• system/nvme/apst/src/lib.rs:8-10 says the first I/O after idle "waits at most the sum of the target's entry and exit latencies". §8.1.17 makes one transition cost the old state's exit plus the new state's entry. A first I/O that arrives during entry therefore waits the operational exit, the target's entry and exit, and the operational entry, and a chain adds the intermediate state's exit. On both benches the operational terms are microseconds, but "at most" is not what the specification gives. Say the bound applies to the target's stated round trip, which is what admitted checks, or drop "at most".

Suggestions

1. Rename EXIT_BOUND_US. system/nvme/apst/src/lib.rs:34. It bounds entry plus exit, as its doc and the driver's log ("round trips within") say, but the name reads as an exit-latency bound, which is a different policy. Consider ROUND_TRIP_BOUND_US.

2. Hold the states once. system/nvme/src/nvme.rs:100-112, :270-273, :452-471. The states are kept in both Controller::states and Power::Autonomous. The plan is a pure function of them, yet it is recomputed on every reset. The boxed variant exists only so the log can print once. A simpler shape:

   • identify builds the plan, or its 256 encoded bytes, once;
   • set_power sends it and returns whether the controller took it;
   • each caller reports the outcome, which also settles required 3.

   This drops the (bool, Vec<_>) tuple, the placeholder Power::Unsupported before set_power runs, and the box. Skipping Set Features when no state is admitted would also stop the log saying "on … no state admitted".

3. Match the heading to its hedge. issues/toyos-durable-writes-on-the-t14s-nvme-run-six-times-slower-in-some-boots.md:65. The bold "What ends the mode: idle under APST, and not idle alone" states as settled what its own bullets call "consistent with … does not prove it". "Not idle alone" rests on 0 of 3 boots. State it as the reading.

4. Name the driver in the track. issues/diskserver-drives-nvme-apst.md:7-18 still says blockd builds the table. No blockd exists, and the new Built paragraph names the NVMe driver. If the record stays open, name the driver throughout.

Evidence

Numbers. I checked the PR's figures against r10's raw rows and verdict.md.

• Every after-idle median, range, percentage and worst sample in the PR reproduces.
• AMD: ToyOS 11.15, 11.33 and 11.34 ms against Linux 10.58, 10.70 and 11.47 ms. That is +0.63 ms (6%), and every ToyOS boot lies inside Linux's range.
• T14: 14.31 against 13.02 ms (+10%); worst sample 18.34 against 13.75 ms.
• Both systems set 0,1,2,3->4 after 100 ms on both drives: ToyOS's driver logs it, and osbench reads it back on Linux.
• The slow-mode counts hold: 2 of 2 plus 4 of 4 with APST and the idle row, 0 of 3 with the idle row alone, and 1 of 11 with neither. The first A/B's M and F boots have no after-idle row in ab/rows, so counting them as "neither" is right.
• The L1 verdicts hold under the 5% rule. One slip in the PR text: the T14 claim "no row worse and resolved" overlooks fs.disk.seek, which is +1% and resolved. It is under 5%, so the verdict does not change.

Revisions. Comparing diffs with comment lines excluded, 669a096f6 and 332cf33c5 match 349ccb26b and 047e0a708 line for line, and 902ca4e30 only adds the box. Main's commits between the measured base and this base touch neither the NVMe path nor the bcachefs path, and the head merges cleanly onto current main. Guest run 42983382300 passed nvme_disk_boots at 047e0a708.

Specification conformance, checked against the NVMe Base 2.1 text:

• PowerState::parse reads Figure 313's offsets for MP, MXPS, NOPS, ENLAT, EXLAT, IDLP and IPS, and treats 0h as "not reported", as the figure defines.
• NPSS at byte 263 and APSTA bit 0 at byte 265 match Figure 312.
• The command matches Figure 392 (APSTE is CDW11 bit 0) and Figure 393 (ITPT in bits 31:08, ITPS in bits 07:03, unused entries zero).
• Every target is non-operational, and none reports higher idle power than the state it leaves, as Figure 393 asks.
• osbench's nvme_passthru_cmd (72 bytes) and _IOWR('N', 0x41) match the UAPI.

Bounds. BENCHCASE, OSBENCH_DISK and --bound-ms each grow by exactly the row's 8 × 10 s of deliberate idle.

• The comment's arithmetic still leaves the 136 s stall: 244 + 50 + 95 + 80 = 469, and 605 − 469 = 136.
• 605 s is the arm's 545 s plus the 60 s every list has.
• OSBENCH_DISK reuses BENCHCASE's bound by its own definition.
• No guest row runs persist.

Both widenings are justified.

Idle power. The drive's own power rail is outside the package energy osbench reads. The drive-power benefit therefore rests on the descriptors (PS4 at most 5 mW) and on the table matching Linux's, not on a ToyOS measurement. r1–r4's package result came from an earlier table that also reached PS4, and the record says what it measured. Whether that closes the track's idle-minute exit is the orchestrator's call.

PR-text corrections (the orchestrator's):

• the T14 wake figure (required 4);
• "APST ends the T14 drive's slow write mode" is stated as a headline, while the body says it is not proved;
• "the remainder over Linux is the commit path's" is stated as fact. The record calls the second Flush only the first suspect, and that path has not been split again since Consolidate storage around native bcachefs and userland filesystems #1152.

Re-verification for the fixes: cargo test -p nvme-apst with the new cases, plus the driver's host test and clippy. No metal or guest rerun is needed, for the reasons given at the top.

SEND BACK

[Japabu]

Response to the review of 902ca4e30

Fixed at 56318e297 (code) and a84c64a88 (records). Checks and mutations: https://github.com/ToyOSOrg/evidence/blob/e267c4c96d03ffbe9593fe2384fc3929f39a7fc6/r12/README.md

Required

1. NPSS past 31. states takes &[u8; IDENTIFY_BYTES] and refuses NPSS > 31 as TooManyStates(npss), so the length check and the expect are gone. The driver reports it and serves without APST (Power::TooManyStates). Test an_npss_past_31_is_refused_and_31_reads_all_32_states: NPSS 31 reads 32 states, and the 32nd is a target whose number fills ITPS's five bits. NPSS 32 and 255 are refused. With the refusal removed, that test fails (mutation npss-unchecked).

2. Ranking by state number. A state's targets are the non-operational states after it that state both latencies and no higher idle power. Within the first tier that holds one, the target is the highest-numbered. floor is gone. The admission bound duplicated the last tier's bound, so only the tiers bound the round trip now. Tests:

   • the_target_is_the_highest_numbered_whatever_the_latencies is your probe: it gives 0->2, 1->2, 2: none.
   • the_amd_benchs_drive_gets_the_table_linux_sets and the_t14s_drive_gets_the_table_linux_sets pin 0,1,2,3->4 after 100 ms over each drive's logged descriptors, with max power now exact through MXPS.

   With ranking by latency restored, the probe test fails, and so does the NPSS-31 test (mutation latency-rank).

3. Refusal after a reset. set_power answers the refusal status. open and reset return that status, and main reports it at bring-up and after a reset (say_refused).

4. The T14 wake. The record and the PR text now give about 13.1 ms within the fast mode (14.20 against 1.10 ms, r1-P'), against Linux's 12.0 ms, and say why the no-APST arm measures no wake on the T14. The AMD's 7.1 ms stays.

5. Comments. The driver's paragraph says "Base 2.1". The crate says the bound is on the target's stated round trip, and that a transition also costs the exit latency of the state it leaves; "at most" is gone. I gave the manual's line the same correction: it said the first request "waits within a bound".

Suggestions

1. Taken: ROUND_TRIP_BOUND_US.
2. Taken in substance. Identify builds Power once: Unsupported, TooManyStates(npss), or Autonomous { states, plan }. The plan is a Vec with one entry per state, so nothing is boxed and the tuple field is gone. set_power sends the plan and answers. Declined: skipping Set Features when nothing is admitted. Such a table is instead sent with APSTE clear and logged as "off". Sending it fixes the drive's behaviour whatever its default APST state is, which I have not read in the specification. power's initial Unsupported stays: it is the same pattern as lba_bytes, vwc and max_blocks, the other fields identify fills.
3. Taken: the heading now reads "The reading, not proved: …".
4. Moot: the track is closed in this PR, by the orchestrator's decision.

The PR text now has:

• the T14 wake;
• "Consistent with the data, not proved: APST ends the T14 drive's slow write mode";
• the second Flush as the remainder's first suspect;
• the T14 L1 row noting fs.disk.seek +1% resolved.

The track diskserver-drives-nvme-apst is deleted.

• The mutation its exit names, a table admitting a state past the bound, was run as a control: with the last tier's bound raised to u64::MAX, a_state_whose_round_trip_exceeds_the_bound_is_never_a_target fails (mutation bound).
• Its durable rule, that the specification bounds no latency and the bound is policy, is now stated at the bound.
• The pc2 record says the drive's rail is outside the package energy, so the drive's saving rests on its descriptors and on the table matching Linux's.
• The speed record no longer lists the track.

No metal or guest rerun is needed for the tables: in all 12 measured boots, the six with APST logged 0,1,2,3->4 after 100 ms on both drives, and none reset.

The guest row nvme_disk_boots is dispatched at a84c64a88 (run 43058169740) for the changed bring-up; not awaited.

🤖 Generated with Claude Code

[Japabu]

Follow-up review of #1131 at a84c64a88

SEND BACK. Two small blockers remain, both in test names or comments that this round introduced or made stale. All five earlier findings are resolved, and the code written to resolve them is correct. Fixing the blockers needs only cargo test -p nvme-apst. No metal run or guest row is needed.

Scope: the earlier findings, plus the diff 902ca4e30..a84c64a88 (56318e297 code, a84c64a88 records). I checked the specification text against the NVM Express Base 2.1 PDF.

Architecture

Nothing changed in ownership. The policy stays in the driver's pure crate.

Power is now built once by identify as Unsupported, TooManyStates or Autonomous { states, plan }. set_power sends it and returns the refusal status. This removes the copy of the states kept beside it, the box and the recomputation on reset.

Size:

• the crate's production code goes from 148 to 152 lines;
• its tests go from 144 to 198 lines, with three new tests and one helper;
• main.rs changes by +48/−29 and nvme.rs by +45/−42;
• the records shrink by the deleted 34-line track.

The five findings

1. NPSS past 31: resolved. states now takes &[u8; IDENTIFY_BYTES]. It returns TooManyStates when count > STATES, so the slice ends at most at 2048 + 32 × 32 = 3072, and the const assert holds that bound. The driver logs the refusal and serves without APST. The test covers NPSS 31, 32 and 255. NPSS 31 also exercises ITPS 31 in the encoding. The npss-unchecked mutation fails that test.
2. Ranking: resolved. Targets are the states after n (skip(n + 1)) that are non-operational, report no higher idle power and state both latencies. rev().find over that ascending list takes the highest number within the first tier that holds one.
   • This matches §8.1.17: "each subsequent power state consumes less than or equal to the maximum power consumed in the previous state".
   • It matches Figure 393: ITPS "shall be a non-operational state" when ITPT is non-zero, and "should not specify a power state with higher reported idle power than the current power state".
   • The probe test gives [2, 2, None], and the latency-rank mutation fails it.
   • The removed admission bound admits nothing extra. A target is chosen only through a tier with trip <= within_us, and the largest within_us is ROUND_TRIP_BOUND_US. The bound mutation, which sets the last tier to u64::MAX, fails a_state_whose_round_trip_exceeds_the_bound_is_never_a_target.
3. Refusal after reset: resolved. open and reset both return set_power's Option<u16>.
   • At bring-up, main passes it to say_power, which logs the states and then calls say_refused.
   • reset calls say_refused after its own line.
   • Unsupported and TooManyStates send nothing, so for them the status is always None.
4. T14 wake: resolved. The after-wake record now reads the wake within the fast mode, 14.20 − 1.10 ≈ 13.1 ms (r1-P′), against Linux's 13.02 − 0.98 ≈ 12.0 ms. It explains why the no-APST arm measures no wake on the T14. The AMD's 7.1 ms stands. These match the earlier review's figures.
5. Comments: resolved. The driver's paragraph now says "Base 2.1". The crate now says the bound applies to the target's own round trip, and that a transition also costs the exit latency of the state it leaves. "At most" is gone.

Empty table

Sending APSTE = 0 with a zeroed table is well defined:

• Figure 393 says an ITPT of 0h "disables the autonomous power state transition feature for this power state", and that ITPS should then be 0h.
• §5.1.25.1.6 requires unused entries to be "cleared to all zeroes".
• Figure 392 says APSTE "is cleared to '0' by default". The author's response says he had not read the default; this is it. The command therefore changes nothing on a controller at its default. It matters only where a saved value had APSTE set, and there it makes the "off" line true.

A controller with APSTA = 0 never receives the command, because identify maps Ok((false, _)) to Unsupported and set_power returns before sending. I accept the choice.

Records

• The manual line now matches the code: it says the controller enters only a state whose stated entry and exit latencies together are within a bound. It names no code.
• Closing the track.
  • At a84c64a88, git grep finds no diskserver-drives-nvme-apst. Main (67713cbc1) cites it only in the pc2 record and the speed record, and this PR edits both. The head merges cleanly onto main.
  • The durable rule ("the specification bounds no latency; this is policy") now sits at ROUND_TRIP_BOUND_US, as issues/README.md asks.
  • The pc2 record gains the bullet saying the drive's power rail lies outside the package energy osbench reads.
  • The after-wake record carries the table that both systems set and the wake.
  • The exit's mutation was run as a control in r12.
  • The two bench tests' descriptors match the driver's r10 logs (rows/amd-r1-P.txt, rows/t14-r1-P.txt) field for field.

Required

1. Two committed tests name Linux as their oracle. system/nvme/apst/src/lib.rs:317-321 and :334-338.

the_t14s_drive_gets_the_table_linux_sets and the_amd_benchs_drive_gets_the_table_linux_sets both say in their doc comments: "The table Linux sets on it too."

.claude/agents/reviewer.md:43 allows source and tests to name Linux "only as a host or as part of an externally defined interface", and :37 keeps readings captured under Linux "in evidence, not committed test oracles". The expected values come from the specification and the tiers applied to each drive's descriptors. Linux's matching table is a measurement from r10, and the after-wake record and the PR already state it. The earlier names (the_deepest_state_within_a_tier_…) did not name Linux. The deleted track gave "the specification's rules" as the oracle.

Fix: name each test for what it asserts, for example the_t14s_drive_goes_to_ps4_after_100_ms_from_every_state, and delete the Linux sentence from both doc comments.

2. A stale comment points at the deleted track. system/nvme/apst/src/lib.rs:264, "/// The mutation the track names: a state past the bound is never a target."

a84c64a88 deletes the track, so "the track" now points at nothing. The rest of the line repeats the test's name. Delete the comment.

Suggestion

Avoid "states" as both verb and noun in the manual line. docs/src/architecture/scheduling.md:41-42: "builds from what the drive states of its own states" uses "states" once as a verb and once as a noun. "Builds from the power states the drive describes" would read more cleanly.

Evidence

r12 ran at a84c64a88 with a clean tree (porcelain: []). It records:

• nvme-apst: 12 passed;
• the driver: 5 passed;
• both clippy runs with the adopted lints: exit 0;
• each of the three mutations applied, failing exactly the tests the PR names (exit 101), reverted, and the source hash checked afterwards.

56318e297 and a84c64a88 have the same code, because the second touches only issues/ and docs/.

I accept the claim that the benches' tables are unchanged. The logged descriptors list rising latencies, so both rankings give 0,1,2,3->4 after 100 ms, and the r10 logs record no reset.

Guest row nvme_disk_boots (run 43058169740, at a84c64a88) is still queued. QEMU's controller takes the Unsupported path, so the row checks that the reorganized bring-up still reaches ready. The two fixes above do not affect it.

SEND BACK

[Japabu]

Follow-up fixes at 5cdf3c9bc (test names, comments and the manual; no code):

• The bench tests are renamed to the_t14s_drive_sends_every_state_to_ps4_after_100_ms and the_amd_benchs_drive_sends_every_state_to_ps4_after_100_ms. "The table Linux sets on it too." is deleted.
• The bound test's note about the track is deleted.
• Taken: the manual now says the table is built "from the power states the drive describes".

cargo test -p nvme-apst: exit 0, 12 passed. Its clippy with the adopted lints and -D warnings: exit 0. Evidence: https://github.com/ToyOSOrg/evidence/blob/ef7ae674c238167d0a4298ab3790c7e98b624de2/r13/README.md

Guest run 43058169740 (nvme_disk_boots at a84c64a88) was still queued on the guest runner at this writing.

🤖 Generated with Claude Code

[Japabu]

Orchestrator: verified 5cdf3c9 against the follow-up review's two required fixes. git diff a84c64a88 5cdf3c9bc touches only system/nvme/apst/src/lib.rs, renaming two test functions, deleting the stale track comment and trimming two doc sentences, and docs/src/architecture/scheduling.md, the suggested rewording. No asserted value or non-test code changed; evidence r13 shows 12 passed and clippy clean at the head. Both required findings are resolved. Landing waits only for guest run 43058169740 (nvme_disk_boots at a84c64a, whose driver code is the head's).

[Japabu]

Orchestrator: guest run 43058169740 (nvme_disk_boots at a84c64a, whose driver code is the head's) passed: test result: ok. 1 passed. The follow-up review's two required fixes were verified at 5cdf3c9. Description updated. Queueing.