Close Phase 1's four real gaps, and give the RSP its memory (T-21-001) - #35
Merged
Conversation
…bus (T-21-001) DMEM and IMEM lived twice: as `Bus::spmem`, which is what the CPU actually read, and as `Rsp::dmem`/`Rsp::imem`, which nothing touched. That was harmless only while the RSP was a stub -- the moment it executes, the CPU and the RSP would address two different memories that happened to start out equal. The RSP now owns both banks and the Bus reaches them through `Rsp::mem_read`/`mem_write`. The behavioural half is the RCP's **size-blind** write path. Everything on the RCP's internal bus (`0x0400_0000-0x04FF_FFFF`) ignores the access size and the low two address bits, latching the whole 32-bit word the VR4300 placed on SysAD (N64brew *Memory map* SS Physical Memory Map accesses). The VR4300 has already shifted the source register into the addressed byte lane, so a narrow store writes that shifted register -- which is why the effect looks like zero-fill rather than a partial update. n64-systemtest states the rule outright in its own header comment (`src/tests/sp_memory/mod.rs`): SH/SB overwrite the whole 32 bits, SD writes only the upper word and touches four bytes. The CPU cannot narrow the value itself, because whether narrowing is correct is a property of the target, not of the instruction: RDRAM honours the byte enables because the RI forwards the low address bits and the access size to the RDRAM devices. So `Bus::write_sized` carries the width and the *untruncated* register to the bus and lets the implementor decide. The default narrows, as the in-crate test buses need; `rustyn64-core` overrides it. The SP memory window also repeats its 8 KiB up to `0x0404_0000` rather than ending at `0x0400_2000`, which is why the folding is the behaviour rather than a bounds-check standing in for one. The PI and SI external buses share the size-blindness on hardware and are deliberately left out: the PI models its own bus quirks separately, and folding both into one rule without the cart tests to check it against would be a change made blind. Phase 5. n64-systemtest: `spmem: SB`, `SH`, `SD` and `SW (out of bounds)` now pass. Each new test was mutation-checked -- reverting the size-blind path turns exactly the three store tests red and leaves the RDRAM and window tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A stall could swallow the timer interrupt entirely. `timer_edge` asked whether `Count == Compare` at the instant it was polled, and it is polled only from `DC` -- which `advance_at` skips for the whole duration of a stall. `Count` is derived from the master clock and keeps advancing regardless, so a 69-PCycle multiply interlock (UM Table 3-12) steps clean over the single cycle for which the equality holds. `IP7` then never latches at all: not a late interrupt, a lost one, with software waiting on the timer hanging forever. The fix asks whether `Compare` lies in the half-open interval `(last_count, now]`, in wrapping arithmetic so a counter wrap is just another interval. Excluding `last_count` preserves the existing edge semantics -- sitting *on* `Compare` is not a transition into it, which is what keeps the power-on `Count == Compare == 0` from latching `IP7` before an instruction retires. Making the interval the question also makes the poll cadence irrelevant, which is why no poll was added to the stall path: an earlier version latched `IP7` during the stall as well, and that turned out to be unobservable -- no guest instruction can read `Cause` while the pipeline is held -- so it was dropped rather than kept as inert code. `MTC0` to `Count` or `Compare` now re-bases the interval. Both writes move an endpoint underneath the detector: a `Count` write that jumps *over* `Compare` would read as a crossing, and a `Compare` placed behind `Count` as one that already happened. Neither fires on hardware. Found by the Phase 1 checkbox audit (sprint-2 line 138), which was recorded as an unmet acceptance criterion with no deferral note rather than as a known bug. Oracles: golden-log 0-diff holds; n64-systemtest Phase 1 categories still 0. All three new tests were mutation-checked -- removing the re-base turns both write tests red, and the stall test was red before the interval fix. The `Compare` test deliberately leaves an unpolled gap before the write; without it the detector's interval is already empty and the test passes either way. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ssing `VectorKind::XtlbRefill` existed, `vector()` mapped it to offset 0x080, and its unit tests exercised the enum directly -- but no live code path ever produced it. Every TLB refill took the 32-bit vector at 0x000, whatever the addressing width. The two vectors carry the same `ExcCode`, so the entry point is the only thing that tells the handler which page-table walk to run: the 64-bit handler reads `XContext`, the 32-bit one `Context`. Sending a 64-bit miss to 0x000 runs the 32-bit walker over a 64-bit address. n64-systemtest installs a distinct handler at 0x8000_0080, so this is a real entry point, not a theoretical one. `Exception::TlbRefill` now carries the addressing width of the faulting access. It has to be captured there rather than re-derived at dispatch: dispatch has already set `EXL`, which forces `access_mode` to report Kernel, so a late re-derivation would answer for the handler's mode instead of the access's. Two tests, deliberately at different levels. The mapping test is satisfied just as well by a `wide` flag that is never true in practice, which is the decoded-but-no-op hazard `docs/engineering-lessons.md` describes; the pipeline test therefore drives a real miss and differs from the existing 32-bit case in `Status.KX` alone, so the two landing on different vectors is attributable to nothing else. Both go red when the selection arm is removed. Found by the Phase 1 checkbox audit (sprint-2 line 174). Not currently visible to n64-systemtest -- its three handlers behave alike, so the suite does not discriminate on entry point -- which is why the gap survived a green oracle. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Phase 1's cycle-cost criterion asks for "FPU rates with the latency = rate + 1 rule". The rule was written down in `docs/cpu.md` and never implemented: `fp_arith` charged nothing, so every COP1 operation from `MOV.S` to `DIV.D` retired in a single cycle. The rates come from the manual itself rather than from the four rows this repo had already quoted. Charging only those four would have meant inventing the rest, so Table 7-14 was extracted with `mutool draw -F txt` (the project's documented method for this PDF -- `pdftotext` fails on it) and transcribed whole, including the three rows that vary by *source* format: `CVT.S` is 2 from double but 5 from a fixed-point format, and `CVT.D` is 1 from single. A uniform per-instruction table gets exactly those wrong. The manual's "+1 for a dependent consumer" is deliberately not added anywhere. The stall holds every stage, so a consumer spends its own cycle once the stall drains and arrives at rate + 1 without a second rule -- adding it explicitly would double-count. A rate of 1 charges nothing. One cycle is what an ordinary instruction already takes, so `ABS`, `MOV`, `NEG`, `C.cond` and `CVT.D.S` are free; the test asserts that asymmetry directly, because a blanket stall on every COP1 op would satisfy a `MUL.D`-only test. Two test levels, for the reason `docs/engineering-lessons.md` gives: the table test asserts the numbers, and a table nothing calls is inert, so the pipeline test drives real instructions and reads the interlock back. Removing the charge turns the second red and leaves the first green. Not implemented, and recorded rather than glossed: the early exit on trivial operands (UM SS7.5.6, table note 2). Those operands are charged the full rate, so the model runs slower than hardware there and never faster -- a bounded, one-directional error. Accuracy ledger C-29, which also records that both oracles are insensitive to these stalls, so the rates are currently unfalsified rather than verified. Oracles: golden-log 0-diff holds over 50,027 records; n64-systemtest Phase 1 categories still 0, suite-wide still 409. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The last unimplemented Phase 1 exception. There was no NMI anywhere in the tree: no signal, no variant, and no path that bypassed the interrupt masks. NMI is deliberately kept out of `dispatch`, because almost none of that function applies to it. It writes `ErrorEPC` rather than `EPC`, sets `ERL` rather than `EXL`, and writes **no `Cause` at all** -- "the contents of all registers are preserved except for" `ErrorEPC` and four `Status` bits (`ERL`/`SR`/`BEV` set, `TS` cleared). Routing it through the general path would have overwritten `Cause.ExcCode` with a code NMI does not have. It still rides in the `Exception` enum so it can reuse the pipeline-flush machinery, and `dispatch` intercepts it before any of the six epilogue steps run. It ignores `IE`, `EXL` and `ERL` entirely. The one condition it respects is the instruction boundary -- "unlike Cold Reset and Soft Reset, but like other exceptions, NMI is taken only at instruction boundaries" -- which is the same run-cycle gate every interrupt already passes. The test asserts the bypass against a `Status` with `IE` clear and both `EXL` and `ERL` set, and a second test shows an ordinary timer interrupt is refused in that exact state. Without the control, "the NMI was taken" would be equally consistent with the masks simply not working. Nothing calls `signal_nmi` yet, and that is stated rather than glossed: on hardware the source is the console's reset button, which arrives as PRENMI then NMI, and the frontend owns that button (Phase 6). The exception is Phase 1 work and belongs with the rest of the exception model; the wire from the button is a separate, later concern. Bit positions are from UM Fig. 6-6 (the Self-Diagnostic Status Field) rather than from memory: BEV 22, TS 21, SR 20. Oracles: golden-log 0-diff holds; n64-systemtest Phase 1 categories still 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ctually does
Phase 1 was tagged v0.2.0 with 42 acceptance criteria still unticked. That is
not a formatting oversight: an audit of all 42 against the source found four
real gaps hiding behind a green oracle, each now fixed on this branch (the
Count/Compare crossing, the XTLB refill vector, the FPU rates, NMI). The rest
were genuinely done and simply never recorded.
Ticked only with evidence. One box claimed a test "where a multi-cycle stall
separates a branch from its delay slot and Cause.BD / EPC still come out right";
the existing test pinned flag *propagation* only, which is a different claim --
a flag can travel correctly and still be read from the wrong latch at dispatch,
the reverse-cascade hazard this project has hit twice. That test is now written,
and it passed on the first honest run: the implementation was right, my first
draft of the test was not (it flagged the instruction that never faults).
Four boxes stay unticked, and now say who owns them rather than trailing off at
"Sprint 2":
- Stepping the RCP between SysAD phases -> Phase 3, the first phase with a
second engine contending for the bus and so the first that can observe the
difference.
- `M` (memory access time) -> Phase 7, still unmeasured, still no invented
value.
- Cache-miss costs -> Phase 7. This deferral's premise is gone (the caches are
modelled now), but the cost is blocked on `M`, so it moves with C-1 rather
than being quietly ticked.
- The suite-wide criterion that the above be explicitly deferred *with*
reasons -- which is what this commit makes true.
Also corrected three stale claims that survived because nothing fails when a
document is wrong:
- `Interlock::Cp0i`'s doc said it "fires when ...", present tense, while
nothing raises it. Its trigger overlaps the exception flush, so charging it
would invent a cycle; the variant stays because `Interlock` enumerates the
documented taxonomy, but the doc now says it is never raised.
- The ledger's Status section still described Sprint 1. Its replacement keeps
the uncomfortable part: after a whole phase, nothing here has been
*measured* -- every resolution so far was by citation.
- Ledger U-5 and U-8 were resolved long ago and still listed Sprint 2 as
owner. Marked resolved with forward references rather than rewritten, per
this file's append-mostly rule.
Oracles unchanged: golden-log 0-diff, Phase 1 categories 0, 403 workspace tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR moves SPMEM ownership into ChangesRSP memory and bus integration
CPU timing and exception control
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ExternalSignal
participant Pipeline
participant COP0
participant ExceptionDispatch
ExternalSignal->>Pipeline: signal_nmi()
Pipeline->>COP0: sample pending NMI at instruction boundary
Pipeline->>ExceptionDispatch: dispatch Exception::Nmi
ExceptionDispatch->>COP0: update ErrorEPC and Status
ExceptionDispatch-->>Pipeline: reset-vector dispatch
Possibly related PRs
🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 passed)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request completes Phase 1 of the emulator by implementing size-blind write paths for the RCP's internal bus, accurate timer edge crossing detection to prevent missed interrupts during stalls, Non-Maskable Interrupt (NMI) support, and XTLB refill vector selection for 64-bit addressing. It also models FPU pipeline stalls based on UM Table 7-14 and updates the RSP to own DMEM and IMEM directly. Feedback on these changes highlights a potential underflow panic in the size-blind shift calculation on the bus, and an issue where calling stall_for with zero cycles still triggers a one-cycle pipeline stall.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
`Pipeline::access_mode` is private, so the rustdoc gate rejects a link to it from a public item. Plain code span instead, which is what `docs/` already prescribes for links that cannot resolve in a default doc build. Caught by CI rather than locally: the rustdoc gate was run earlier in this branch's work and not again after the XTLB commit added the reference. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adopted from a Gemini Code Assist review comment on #35. Within the CPU the invariant holds -- MIPS requires natural alignment and `AddressError` is raised before a misaligned store reaches the bus, so `width + (addr & 3) <= 4` -- but `write_sized` is a public trait method and this PR's own tests call it directly. The failure mode is worse than the debug panic the comment named: in release the subtraction wraps, `8 * huge` overflows again, and the shift is masked, so it would silently write the wrong byte lane instead of failing loudly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/rustyn64-cpu/src/pipeline.rs (2)
639-650: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winComment describes an effect the branch doesn't produce.
"Latching
IP7here does not accept the interrupt... it only records inCause" — but this branch only callsself.cop0.set_now(count_now)before returning; nothing here callsset_ip/timer_edge. The actual latch happens later, the next timedc_stageruns after the stall drains (which is correct, since the crossing-basedtimer_edgetolerates the gap). Worth rewording so the comment doesn't read as describing code that isn't in this branch — path instructions call this exact pattern out as costly for this repo ("this project has been bitten by that four times").As per path instructions: "A comment that asserts what the code does but disagrees with it. Treat comment and code as INDEPENDENT claims; this project has been bitten by that four times and no test failed any of them."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rustyn64-cpu/src/pipeline.rs` around lines 639 - 650, The stall-branch comment incorrectly claims that it latches IP7 and updates Cause, while this branch only advances the clock through self.cop0.set_now(count_now) before returning. Rewrite the comment to describe only that timer progression continues during the stall and that timer-edge handling occurs when dc_stage resumes, without claiming interrupt latching here.Source: Path instructions
709-735: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftCascade order inverts the documented WB > DC > EX > RF stall priority for the new FPU stall.
stall_for(line 517) overwritesself.stallunconditionally with no priority check, andadvance_atruns stagesWB → DC → EX → RF → IC. That means whichever stage callsstall_forlast in a cycle wins the arbitration in practice — IC > RF > EX > DC > WB — which is the exact inverse of the prioritydocs/cpu.mddocuments for this same rule ("Priority runs WB > DC > EX > RF (UM §4.7.4): a later stage's exception or stall request always outranks an earlier stage's").Before this PR
wb_stagenever calledstall_for, so the collision was unreachable. Now it does (line 734, the newMcistall for multi-cycle FPU ops), so an FPU instruction retiring in WB and an integerMULT/DIVtwo instructions younger reachingEXin the same cycle will have the FPU's stall duration silently replaced by the integer op's — the new test (the_documented_fpu_rates_are_charged_as_stalls) only exercises the FPU stall in isolation and does not catch this.Needs a priority-aware arbitration (e.g. tag
Stallwith the requesting stage and only overwrite when the new request's stage priority is >= the current one's), covering the pre-existing DC/EX/RF/IC collisions too rather than just the new WB one.Also applies to: 517-522, 652-656, 1912-1914, 1980-1980, 2131-2131
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rustyn64-cpu/src/pipeline.rs` around lines 709 - 735, Make stall arbitration priority-aware across stall_for and every existing caller: associate each request with its originating pipeline stage and replace the current stall only when the new request has equal or higher documented priority (WB > DC > EX > RF, with IC handled consistently). Update the WB FPU request in wb_stage and the existing DC/EX/RF/IC callers without changing their stall durations or exception behavior, ensuring lower-priority later calls cannot overwrite higher-priority stalls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/rustyn64-core/src/bus.rs`:
- Around line 650-679: Update write_sized to reject unsupported widths before
the is_rcp_internal branch, returning immediately unless width is 1, 2, 4, or 8.
Preserve the existing handling for supported widths and prevent internal-address
writes and shift calculations for width 0, 3, or values above 8.
In `@docs/rsp.md`:
- Around line 59-64: Add authoritative provenance for the repeated SPMEM window:
create or update a measured entry in docs/accuracy-ledger.md naming the exact
n64-systemtest test and marking the behavior measured, then cite that ledger
entry from docs/rsp.md. Update the implementation documentation near the SPMEM
handling in crates/rustyn64-rsp/src/lib.rs and the SPMEM_WINDOW_END definition
in crates/rustyn64-core/src/bus.rs to reference the same ledger entry; apply
these changes at docs/rsp.md:59-64, crates/rustyn64-rsp/src/lib.rs:98-106, and
crates/rustyn64-core/src/bus.rs:281-285.
In `@to-dos/phase-1-cpu-golden-log/overview.md`:
- Around line 20-21: Align the Phase 1 status statements in the overview so COP1
is consistently represented: remove the completed COP1 claim if Sprint 3 remains
a stub, and update the Phase 1 oracle gate criteria to explicitly include COP1
alongside CPU, COP0, and TLB. Ensure the related statements in the referenced
section use the same completion status.
In `@to-dos/phase-1-cpu-golden-log/sprint-2-cop0-tlb-exceptions.md`:
- Around line 265-267: Uncheck the result-count reading criterion in the harness
checklist until the output channel is identified, the reader is implemented, and
execution evidence is documented. Preserve the note requiring selection among
isviewer.rs, sc64.rs, and FramebufferConsole without assuming which channel is
active.
- Around line 206-209: Update the earlier acceptance text and ledger reference
in this ticket to describe the actual cache model consistently, removing the
stale claim that cache contents are not modelled. Keep the subsequent statement
that the unmeasured M parameter remains the blocker and that the work moves to
Phase 7.
In `@to-dos/ROADMAP.md`:
- Around line 21-24: Resolve the v0.2.0 status inconsistency in the roadmap:
align the release entry with the later statement that no tag has been cut by
marking v0.2.0 as planned and unreleased, unless an existing tag can be
verified, in which case update the later status to reference it.
---
Outside diff comments:
In `@crates/rustyn64-cpu/src/pipeline.rs`:
- Around line 639-650: The stall-branch comment incorrectly claims that it
latches IP7 and updates Cause, while this branch only advances the clock through
self.cop0.set_now(count_now) before returning. Rewrite the comment to describe
only that timer progression continues during the stall and that timer-edge
handling occurs when dc_stage resumes, without claiming interrupt latching here.
- Around line 709-735: Make stall arbitration priority-aware across stall_for
and every existing caller: associate each request with its originating pipeline
stage and replace the current stall only when the new request has equal or
higher documented priority (WB > DC > EX > RF, with IC handled consistently).
Update the WB FPU request in wb_stage and the existing DC/EX/RF/IC callers
without changing their stall durations or exception behavior, ensuring
lower-priority later calls cannot overwrite higher-priority stalls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f052af61-821d-434f-8249-3237609141ff
📒 Files selected for processing (17)
crates/rustyn64-core/src/bus.rscrates/rustyn64-cpu/src/cop0.rscrates/rustyn64-cpu/src/exception.rscrates/rustyn64-cpu/src/fpu.rscrates/rustyn64-cpu/src/lib.rscrates/rustyn64-cpu/src/pipeline.rscrates/rustyn64-rsp/src/lib.rscrates/rustyn64-test-harness/src/rom.rsdocs/accuracy-ledger.mddocs/cpu.mddocs/engineering-lessons.mddocs/rsp.mdto-dos/ROADMAP.mdto-dos/phase-1-cpu-golden-log/overview.mdto-dos/phase-1-cpu-golden-log/sprint-1-integer-core.mdto-dos/phase-1-cpu-golden-log/sprint-2-cop0-tlb-exceptions.mdto-dos/phase-2-rsp-lle/sprint-1-scalar-sp.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/rustyn64-cpu/src/pipeline.rs (1)
532-534: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument
Pipeline::signal_nmi.This new public method lacks rustdoc and will fail the documentation gate. As per coding guidelines, “New public Rust items must have rustdoc documentation.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rustyn64-cpu/src/pipeline.rs` around lines 532 - 534, Add rustdoc documentation directly above the public Pipeline::signal_nmi method, briefly describing that it signals the non-maskable interrupt and marks an NMI as pending. Keep the method’s behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/rustyn64-cpu/src/pipeline.rs`:
- Around line 532-534: Add rustdoc documentation directly above the public
Pipeline::signal_nmi method, briefly describing that it signals the non-maskable
interrupt and marks an NMI as pending. Keep the method’s behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 34b8813d-52ca-4b80-9b5a-5d343f44a51d
📒 Files selected for processing (2)
crates/rustyn64-core/src/bus.rscrates/rustyn64-cpu/src/pipeline.rs
Six comments, all adopted -- four of them documentation inconsistencies this
branch either introduced or left standing, which is the class of error nothing
in CI can catch.
**Unsupported widths now behave the same on both paths.** The non-RCP branch
fell through to `_ => {}`, while the internal-bus arm matched `w => ...` and
would have *stored* for a width of 3, 0 or 9. Nothing reaches it -- `StoreKind`
yields only 1/2/4/8 -- but "unreachable" is a property of the current caller and
this is a public trait method, so the two paths now share one contract.
**The SPMEM mirroring gets real provenance (ledger C-30).** It was resting on a
code comment that named a test, which is not a source. The wiki documents the
two 4 KiB ranges and stops; the mirroring rests entirely on the oracle, in two
independent forms -- n64-systemtest's own source comment and the
`sp_memory: SW (out of bounds)` test that executes the claim. Recorded because
masking an address is the natural implementation of both "it mirrors" and "we
skipped the bounds check", and those are different claims about hardware.
**Phase 1's oracle criterion omitted COP1**, in both `overview.md` and
`ROADMAP.md`. VERSION-PLAN 0.2.0 is authoritative and names all four
categories; AGENTS.md already records that `docs/STATUS.md` once carried the
same three-category conflation. Fixed in both files. The sprint statuses were
stale too: Sprint 2 said "planned" and Sprint 3 "stub" while both are complete,
and Sprint 3 is now marked complete with a note that it never got a plan file --
it ran against the ledger and the two oracles rather than a ticket list.
**The zero-depth cache box is annotated, not rewritten.** It is marked
SUPERSEDED by T-11-003/D-6 the way the ledger marks D-5 itself, because
rewriting a completed sprint's acceptance record erases reasoning that was sound
while it held. The annotation keeps the useful part: the boundary that box
predicted (Phase 5 DMA coherency) came due earlier, at n64-systemtest's cache
groups, which observe staleness with no DMA at all.
**The result-count box was stale in its text, not in its tick.** It still said
the output channel was undetermined. Resolved: both ISViewer and EMUX `xlog`,
EMUX preferred -- the suite picks its console at runtime from what the emulator
advertises, so a single hardcoded choice would have been a guess either way.
**The v1.0.0 milestone still claimed no tag had ever been cut.** Two are.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
to-dos/phase-1-cpu-golden-log/overview.md (1)
39-40: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSynchronise the determinism criterion with
docs/STATUS.md.This checkbox now says the ADR 0004 gap is closed, while its own text says
docs/STATUS.mdstill records it as unexercised. Update the authoritative status record with the test evidence in this change, or leave the criterion deferred.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@to-dos/phase-1-cpu-golden-log/overview.md` around lines 39 - 40, Synchronize the determinism criterion with the authoritative status record: either update docs/STATUS.md to document the ADR 0004 regression-test evidence and mark the gap exercised, or revert the checklist item to its deferred state so it no longer claims closure.Sources: Path instructions, Learnings
crates/rustyn64-core/src/bus.rs (1)
1194-1282: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for unsupported store widths.
The new guard at Line [658] restores the public
write_sizedno-op contract, but the shown tests cover only supported widths. Add cases for widths0,3, and16against an internal address and assert that memory and register state remain unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rustyn64-core/src/bus.rs` around lines 1194 - 1282, Add a regression test alongside the existing Bus store tests that calls write_sized with unsupported widths 0, 3, and 16 at an internal address. Initialize representative memory and register state, perform each store, then assert both memory and register values remain unchanged, preserving the public no-op contract.docs/rsp.md (1)
50-72: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRecord the measured oracle effect.
This behaviour section cites C-30 and C-28 but does not state the measured change in
n64-systemtest’s failing-assertion count, or explicitly say that it was not measured. Add that result to this specification or the referenced ledger entry.As per coding guidelines, emulation-behaviour changes must state the measured failure-count effect or explicitly state that it was not measured.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/rsp.md` around lines 50 - 72, Update the RSP memory behavior documentation around the C-30 and C-28 references, or the referenced accuracy ledger entries, to record the measured change in n64-systemtest failing-assertion count caused by this behavior. If no measurement exists, explicitly state that the failure-count effect was not measured.Source: Coding guidelines
crates/rustyn64-rsp/src/lib.rs (1)
129-138: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winDrop
constfrommem_write
bank[off & 0xFFF] = val;relies on constIndexMut/DerefMut, which is not stable on Rust 1.97. Thisconst fnblocks the pinnedthumbv7em-none-eabihfbuild.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rustyn64-rsp/src/lib.rs` around lines 129 - 138, Remove the const qualifier from Rsp::mem_write while preserving its existing address folding, bank selection, and byte-write behavior. Keep the method signature and implementation otherwise unchanged so it no longer requires unstable const mutable indexing.Sources: Coding guidelines, MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/accuracy-ledger.md`:
- Around line 1242-1244: Revise the upper-bound statement in the accuracy ledger
to avoid attributing the 0x0404_0000/SP-register boundary to the cited test.
Separate the measured evidence from the bus-map/manual source, or add a test
that explicitly probes the boundary before describing it as test-asserted.
In `@to-dos/phase-1-cpu-golden-log/overview.md`:
- Around line 81-86: Update the Sprint 3 completion record in the overview
document to mark the stale golden-trace producer risk as superseded, and
reference the actual oracle and verification run supporting the 0-diff over
50,027 retired records. Ensure the document no longer states that the producer
is missing or the 0-diff gate cannot be met.
---
Outside diff comments:
In `@crates/rustyn64-core/src/bus.rs`:
- Around line 1194-1282: Add a regression test alongside the existing Bus store
tests that calls write_sized with unsupported widths 0, 3, and 16 at an internal
address. Initialize representative memory and register state, perform each
store, then assert both memory and register values remain unchanged, preserving
the public no-op contract.
In `@crates/rustyn64-rsp/src/lib.rs`:
- Around line 129-138: Remove the const qualifier from Rsp::mem_write while
preserving its existing address folding, bank selection, and byte-write
behavior. Keep the method signature and implementation otherwise unchanged so it
no longer requires unstable const mutable indexing.
In `@docs/rsp.md`:
- Around line 50-72: Update the RSP memory behavior documentation around the
C-30 and C-28 references, or the referenced accuracy ledger entries, to record
the measured change in n64-systemtest failing-assertion count caused by this
behavior. If no measurement exists, explicitly state that the failure-count
effect was not measured.
In `@to-dos/phase-1-cpu-golden-log/overview.md`:
- Around line 39-40: Synchronize the determinism criterion with the
authoritative status record: either update docs/STATUS.md to document the ADR
0004 regression-test evidence and mark the gap exercised, or revert the
checklist item to its deferred state so it no longer claims closure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 46db6a4d-d2ec-439d-96db-b85ed04727ca
📒 Files selected for processing (7)
crates/rustyn64-core/src/bus.rscrates/rustyn64-rsp/src/lib.rsdocs/accuracy-ledger.mddocs/rsp.mdto-dos/ROADMAP.mdto-dos/phase-1-cpu-golden-log/overview.mdto-dos/phase-1-cpu-golden-log/sprint-2-cop0-tlb-exceptions.md
Two CodeRabbit findings on prose added earlier in this PR. Ledger C-30 cited `spmem: SW (out of bounds)` for both halves of the claim, but that test proves only that the window repeats with an 8 KiB period. It never probes `0x0404_0000`. The upper bound comes from the address map -- the SP registers start there, so the window ends where the next device begins -- which is a different kind of evidence, and conflating the two is exactly what this ledger exists to prevent. The entry now says the last repetition before that address has never been tested, and names the boundary test that would close it. The Phase 1 overview marked Sprint 3 complete while still listing "the golden trace does not exist yet" as an open risk, and repeating that claim under Dependencies. Both now record the resolution. The risk is struck rather than deleted: its predicted mitigation is what actually happened, which makes it a rare case of a risk register earning its keep. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/accuracy-ledger.md`:
- Around line 1242-1250: Update the preceding “Basis” statement for this
bounded-window entry to distinguish the oracle-supported repetition and period
from the address-map-supported upper bound at 0x0404_0000. Preserve the explicit
separation between sourced, measured, and inferred behavior, and do not
attribute the boundary claim solely to the oracle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a66e1c95-3b7a-4b96-8698-3706a4fee1ef
📒 Files selected for processing (2)
docs/accuracy-ledger.mdto-dos/phase-1-cpu-golden-log/overview.md
The Bounded paragraph was fixed to separate the two sources; the basis heading above it still said "the oracle, and only the oracle", so the entry contradicted itself one paragraph later. In this file that half-fix is worse than the original overstatement, because the heading is what gets skimmed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/accuracy-ledger.md (3)
1222-1252: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun the pinned markdownlint hook locally before merge. This repo pins markdownlint in
.pre-commit-config.yaml, and CI does not run it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/accuracy-ledger.md` around lines 1222 - 1252, Run the repository’s pinned markdownlint hook locally against the updated documentation before merging, using the configuration and tool version specified in .pre-commit-config.yaml; resolve any reported issues in the C-30 entry while preserving its content.Sources: Path instructions, Learnings
1231-1236: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not call same-suite evidence independent
docs/accuracy-ledger.mdC-30 overstates provenance under the honesty rule: the n64-systemtest source comment andspmem: SW (out of bounds)test are different evidence forms, but they are not independent references. Rephrase that line or add a separate primary source if you want to claim independence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/accuracy-ledger.md` around lines 1231 - 1236, Revise the C-30 provenance wording around the n64-systemtest evidence to avoid calling its source comment and `spmem: SW (out of bounds)` test independent references. Describe them as two evidence forms from the same suite, or add a genuinely separate primary source before retaining any independence claim.Sources: Path instructions, Learnings
1248-1250: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCite the exact SP register-map location for the
0x0404_0000boundary.N64brew *RSP Interface*is too broad here; point the ledger at the specific section or table row used for the register boundary claim.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/accuracy-ledger.md` around lines 1248 - 1250, Update the boundary explanation near the `0x0404_0000` claim to cite the exact N64brew RSP Interface section or table row that identifies the SP register-map start. Replace the broad `N64brew *RSP Interface*` reference while preserving the distinction between the map-derived boundary and the untested behavior.Sources: Path instructions, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/accuracy-ledger.md`:
- Around line 1222-1252: Run the repository’s pinned markdownlint hook locally
against the updated documentation before merging, using the configuration and
tool version specified in .pre-commit-config.yaml; resolve any reported issues
in the C-30 entry while preserving its content.
- Around line 1231-1236: Revise the C-30 provenance wording around the
n64-systemtest evidence to avoid calling its source comment and `spmem: SW (out
of bounds)` test independent references. Describe them as two evidence forms
from the same suite, or add a genuinely separate primary source before retaining
any independence claim.
- Around line 1248-1250: Update the boundary explanation near the `0x0404_0000`
claim to cite the exact N64brew RSP Interface section or table row that
identifies the SP register-map start. Replace the broad `N64brew *RSP
Interface*` reference while preserving the distinction between the map-derived
boundary and the untested behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0263da37-46f3-4454-abc0-43ffda598119
📒 Files selected for processing (1)
docs/accuracy-ledger.md
This was referenced Jul 21, 2026
Merged
Merged
doublegate
added a commit
that referenced
this pull request
Jul 22, 2026
chore(release): v0.3.0 "Microcode" — the LLE RSP (Phase 2 close) (#54) Phase 2 close. Bumps the workspace + all crates 0.2.0 -> 0.3.0, renames the CHANGELOG [Unreleased] section to [0.3.0] "Microcode" (2026-07-22) with the full Phase 2 summary (PRs #35-#53), and brings README, docs/STATUS.md, VERSION-PLAN, and AGENTS.md into line with both phases complete. Docs + version only, no behavior change. Review: Antigravity clean (one nitpick, declined); CodeRabbit's two findings adjudicated (softened premature "tagged" claims; the changelog-provenance point declined and CodeRabbit concurred that the provenance lives in the specs). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Two things, in this order: close Phase 1 honestly, then lay the first stone of Phase 2.
Phase 1 was tagged
v0.2.0with 42 acceptance criteria still unticked. An audit of all 42 against the source — not against the docs, which is the point — found that most were genuinely done and never recorded, but four were real gaps sitting behind a green oracle. All four are fixed here.The four Phase 1 gaps
1. The timer interrupt could be lost outright.
timer_edgeasked whetherCount == Compareat the instant it was polled, and it is polled only fromDC— whichadvance_atskips for the entire duration of a stall.Countis derived from the master clock and keeps advancing regardless, so a 69-PCyclemultiply interlock (UM Table 3-12) steps clean over the single cycle the equality holds. Not a late interrupt — a lost one:IP7never latched, and software waiting on the timer would hang forever.Now detected as a crossing of the half-open interval
(last_count, now], in wrapping arithmetic.MTC0toCountorComparere-bases that interval, because both writes move an endpoint underneath the detector and would otherwise manufacture an edge the hardware never produces.2. The XTLB refill vector was never selected.
VectorKind::XtlbRefillexisted and its unit tests exercised the enum directly, but no live path produced it — every refill took the 32-bit vector at0x000. The two vectors share anExcCode, so the entry point is the only thing telling the handler which page-table walk to run (XContextvsContext). n64-systemtest installs a real handler at0x8000_0080.3. The FPU rates were documented and never charged.
fp_arithcharged nothing, soDIV.Dretired as fast asMOV.S. Rather than implement only the four rows this repo had already quoted — which would have meant inventing the rest — UM Table 7-14 was extracted withmutool draw -F txtand transcribed whole, including the three rows that vary by source format (CVT.Sis 2 from double but 5 from fixed point).4. NMI was unimplemented. No signal, no variant, no path bypassing the interrupt masks.
Phase 2 groundwork (T-21-001)
DMEM and IMEM lived twice: as
Bus::spmem, which is what the CPU actually read, and asRsp::dmem/imem, which nothing touched. Harmless only while the RSP was a stub. The RSP now owns both banks.The behavioural half is the RCP's size-blind bus. Every device in
0x0400_0000-0x04FF_FFFFignores the access size and the low two address bits, latching the whole 32-bit word the VR4300 placed on SysAD. n64-systemtest states the rule in its own header comment: "SH/SB are broken: they overwrite the whole 32 bit… SD is broken: it only writes the upper 32 bit".Since RDRAM does honour byte enables (the RI forwards the size to the RDRAM devices), correct narrowing is a property of the target, not the instruction — so
Bus::write_sizedcarries the width and the untruncated register and lets the target decide.Results
spmem: SB,SH,SDandSW (out of bounds)now pass.What is deliberately not done
Interlock::Cp0istays named but never raised; its trigger overlaps the exception flush, so charging it would invent a cycle. Its doc said "fires when…" in the present tense — corrected.Mand the cache-miss costs) instead of trailing off at "Sprint 2".Verification
Each fix was mutation-checked: the guard removed, the test confirmed red, the guard restored via a file copy. Two tests were rewritten after the check showed they passed either way — the
Comparere-base test needed an unpolled gap before the write, or the detector's interval is already empty and the test proves nothing.Gates run locally on the final commit:
cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace,RUSTDOCFLAGS="-D warnings" cargo doc, theno_stdbuild,pre-commit run markdownlint --all-files, plus both#[ignore]d oracles.🤖 Generated with Claude Code