Phase 2 Sprint 1: the RSP scalar unit executes (T-21-002/004/005) - #36
Merged
Conversation
Eight registers at 0x0404_0000 plus SP_PC at 0x0408_0000 -- which is in its own window, not the ninth slot of the block. The same physical registers are exposed to the RSP as COP0 c0-c7, so `sp.rs` holds one copy and both views reach it. `SP_STATUS` reads as a flag word and writes as set/clear command pairs. That asymmetry is the design rather than an encoding quirk: it lets either processor change one flag with a single store, with no read-modify-write to race the other. The rule that falls out is the one that catches naive implementations -- writing a flag's set and clear bits **together leaves it unchanged**, which a "clear then set" implementation silently turns into a set. n64-systemtest checks it for every reachable flag, including all eight signal bits, whose commands sit at 9 + 2n / 10 + 2n while the flags themselves are at 7 + n. The interrupt commands are not a `SP_STATUS` flag at all: they raise and acknowledge the MI's SP line. The register file reports the change and the Bus applies it, kept separate from the DMA return because one write can do both. Semantics taken from n64-systemtest's own header comment where the wiki is thinner -- the semaphore is released by a write *whatever value is written*, and a read returns the current value then takes it, so the sequence is write, 0, then 1 for ever. The DMA engine moves into the RSP crate and returns a description the Bus executes, matching the PI: the RSP does not own RDRAM, and a chip reaching back into its owner is the cycle the architecture exists to prevent. Two bugs fixed in the move: the length field now rounds **up** to a multiple of 8 (hardware transfers 8 bytes for any value 0..=7, and rounding down turned the suite's `length = 11` case into 8 bytes and dropped the tail), and the address registers are double-buffered, so reads report the ongoing or last-completed transfer rather than a pending write. **This truncates the n64-systemtest run, and that is the point of committing it with the SU still missing.** The old `SP_STATUS` was a stub that always read HALTED, so the suite's `wait_until_rsp_is_halted()` returned instantly every time. Now that clearing HALT actually clears it -- and nothing sets it back, because the RSP still executes nothing -- the suite spins at the first test that starts the RSP: 635 of 917 tests start, against 917 before. The committed gate catches this and fails rather than reporting the resulting lower failure count as progress, which is exactly what it was written for. T-21-004 and T-21-005 (the scalar unit and BREAK) are what restore completion. No PR until they land and the suite runs to 917 again. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…21-005)
The RSP runs code. The SU implements the documented MIPS subset, `BREAK` halts
and latches BROKE, `MFC0`/`MTC0` reach the SP registers, and the MI register
block exists so the CPU can see the SP interrupt line at all.
n64-systemtest: **409 -> 250 failures across a full 917-test run.** The `spmem`
and `SP` categories are now at **zero**; the remaining RSP failures are the
vector unit (Sprint 2). Phase 1 categories stay at 0 and the golden log holds
its 0-diff.
Two rules separate this from a reused CPU core, and both are pinned:
- The PC is 12 bits and **wraps**. Targets lose every high bit and running
past 0xFFC continues at 0x000 -- the suite's `RSP Wrap around` puts nops at
0xFF8 and a BREAK at 0x000 and expects to stop at 0x4.
- Misaligned data accesses are **correct, not faults**. `LW` at 0x001 returns
the bytes at 0x1..=0x4, and each byte address wraps inside DMEM
independently, so a word read at 0xFFE takes two bytes from the end and two
from the start. The identical access on the VR4300 is an AddressError.
There is no exception mechanism at all on this core, which has a consequence
worth stating rather than leaving implicit: `ADD` and `ADDI` cannot trap on
overflow, so they are the same instruction as `ADDU`/`ADDIU`.
**Two bugs found by the oracle, both in the CPU-side plumbing rather than the
RSP:**
`SP_SEMAPHORE` was being taken four times per word read. It has a side effect on
read -- it acquires the mutex -- and `read_u32` composed a word out of four byte
reads, so the first byte saw 0 and the rest saw 1 and the assembled word came
back as 1 where hardware returns 0. On hardware the RCP returns the whole
aligned word for one access regardless of size, so one access is the right
model. Registers with read side effects now take exactly one.
The MI register block did not exist. `MI_INTERRUPT` was unreadable, so a guest
could raise the SP line and never observe it -- which is why `SP Set/Clear
Interrupt` failed even though the line was being set correctly. MODE/VERSION/
INTERRUPT/MASK are implemented with their documented 4-bit mirroring. `MI_MODE`'s
RDRAM **repeat mode** is deliberately not modelled: it is an RI-domain transfer
feature, and the five `MI Repeat` failures are its, not the RSP's.
**And one performance trap, caught by its own comment being wrong.** `rsp_tick`
moved the whole chip out of the Bus with `core::mem::take` under a comment
reading "No allocation" -- but `take` needs `Default`, and constructing an `Rsp`
allocates DMEM and IMEM, so every RCP step allocated and freed 8 KiB. `Rsp::tick`
now returns what it wants done instead of borrowing its owner, so there is
nothing to move. The suite run dropped from 134s to 87s.
That change supersedes the `RspBus` trait, which is deleted rather than left
unused: the DMA path returns a description and the interrupt is reported, so
nothing implemented it any more.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe RSP now owns SP register state and scalar execution returns DMA and interrupt actions. The core bus consumes those actions, models SP and MI register accesses, performs banked DMA transfers, and updates interrupt state. Documentation and tests cover the revised semantics and accuracy results. ChangesRSP execution and bus integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Rsp
participant SpRegs
participant Bus
participant RDRAM
Rsp->>SpRegs: execute scalar step and update SP state
SpRegs-->>Rsp: return DMA or interrupt action
Rsp->>Bus: return StepResult
Bus->>RDRAM: perform DMA transfer
Bus->>Bus: update MI SP interrupt state
Possibly related PRs
🚥 Pre-merge checks | ✅ 7 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (7 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 implements the RSP scalar unit and the SP interface registers, replacing the previous stubs and eliminating costly allocations during RSP ticks. The SP interface registers (including status, semaphore, and double-buffered DMA) and the MIPS-like scalar unit are fully modeled and tested. The review feedback suggests improving the interrupt signaling mechanism by replacing the simple boolean raise_interrupt with an Option<bool> to support both raising and clearing the SP interrupt line. Additionally, it is recommended to mask the SP DMA skip parameter with !7 to enforce the hardware's 8-byte alignment contract.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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-rsp/src/sp.rs`:
- Around line 128-140: Update SpRegs::new() to initialize semaphore as taken so
the first un-written semaphore read returns 1. Add a test covering a read
immediately after construction, while preserving the existing behavior that a
semaphore write clears it for the next read.
In `@crates/rustyn64-rsp/src/su.rs`:
- Around line 330-344: Update cop0_write to propagate both interrupt raise and
clear results from SpRegs::interrupt_change through StepResult, including
Some(false) for CLR_INTR, so Bus::rsp_tick can lower mi_intr.sp consistently
with Bus::sp_register_write. Remove the raise-only handling while preserving
existing SP register writes and invalid-index behavior.
- Around line 41-47: Update Bus::rsp_tick() to propagate the result of
SpRegs::interrupt_change(), assigning the MI SP interrupt line from its returned
boolean for both Some(true) and Some(false). Preserve the existing
sp_register_write() behavior so CLR_INTR acknowledgements clear mi_intr.sp as
required by the SP_STATUS contract.
In `@docs/STATUS.md`:
- Line 13: Update both affected rows in docs/STATUS.md to report Phase 1
category failures and suite-wide failures as separate counts, using wording that
states 0 Phase 1 failures and 250 suite-wide failures across 917 tests started.
Replace each command with the canonical runner including --ignored --nocapture,
while preserving docs/STATUS.md as the source of truth for these counts.
- Line 187: Update the stale status statements in docs/STATUS.md, especially the
claims around lines 95-96 and 192-194, to reflect that n64-systemtest now
executes and reports 917 started tests with the documented failure count. Keep
the existing line 187 gate result and oracle counts consistent, removing
obsolete claims that COP0/COP1/exceptions are required before counts can be
reported or that the emulator cannot execute the ROM.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e07620aa-a162-4a49-ad63-510283f62bbb
📒 Files selected for processing (6)
crates/rustyn64-core/src/bus.rscrates/rustyn64-rsp/src/lib.rscrates/rustyn64-rsp/src/sp.rscrates/rustyn64-rsp/src/su.rsdocs/STATUS.mddocs/rsp.md
Adopted from Gemini Code Assist review comments on #36. `StepResult.raise_interrupt` was a `bool`, so "clear the line" and "this step said nothing about the line" were the same value. An RSP acknowledging its own interrupt with `MTC0 SP_STATUS` + `CLR_INTR` had that dropped: `cop0_write` assigned false, the Bus tested `if out.raise_interrupt`, and nothing happened -- `IP2` would stay asserted for ever. The information was computed correctly by `SpRegs::interrupt_change` and then thrown away by a lossy conversion one line later, which is the more interesting shape of the bug. Invisible to every test that only raises: `SP Set/Clear Interrupt` passes because the CPU clears through its own memory-mapped path, which never went through `StepResult` at all. `BREAK` now reports `None` rather than false when `INTBREAK` is clear, so a previously-raised interrupt survives a later break. Separately, the DMA `SKIP` field is 8-byte aligned -- its low three bits "are always 0" -- and applying that surfaced a second gap the comment did not mention: `SKIP` was never stored, so a read-back of `SP_DMA_RDLEN` reported 0 regardless of what was programmed, against a wiki that says it survives the transfer unchanged. Both halves now tested; the survival half is unfalsified rather than verified, since every DMA test in the suite uses skip 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/rustyn64-core/src/bus.rs (1)
310-318: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winImplement MI_MODE init and EBUS command pairs.
Bits 7/8 and 9/10 are documented clear/set commands for the init and EBUS state, but this path drops them; reads can therefore never report bits 7/8. Keep the explicitly deferred RDRAM mode separate, and add set/clear readback tests. (ultra64.ca)
As per path instructions, “Do not invent values/side-effects not supported by docs/tests”; model this documented register contract and cover it with a regression test.
🤖 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 310 - 318, Update the MI_MODE write handling in the bus access match arm to implement the documented init and EBUS clear/set command pairs, preserving their state for subsequent reads while keeping deferred RDRAM-transfer modes separate. Retain ClearDP behavior, and add regression coverage that verifies both set and clear commands update the corresponding readback bits without introducing undocumented side effects.Source: Path instructions
♻️ Duplicate comments (1)
crates/rustyn64-rsp/src/sp.rs (1)
127-137: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInitialise
SP_SEMAPHOREas taken.Reset currently makes the first unwritten read return
0; the documented register contract requires1. The existing test writes first, masking the defect.
crates/rustyn64-rsp/src/sp.rs#L127-L137: initialisesemaphoreas taken.crates/rustyn64-rsp/src/sp.rs#L428-L443: add a no-write read assertion before the release-on-write cases.As per path instructions, “A comment that asserts what the code does but disagrees with it” must be treated as an independent defect.
🤖 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/sp.rs` around lines 127 - 137, Initialize SpRegs::new so semaphore starts as taken, matching the documented reset contract and the existing comment. In crates/rustyn64-rsp/src/sp.rs lines 127-137, update the semaphore initialization; in lines 428-443, add a no-write read assertion before the release-on-write cases to verify the initial value is 1.Source: Path instructions
🤖 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-rsp/src/su.rs`:
- Around line 532-547: Update break_raises_the_interrupt_only_when_enabled to
retain the StepResult from executing BREAK with the default, disabled INTBREAK
state and assert its interrupt_change is None. Keep the existing halted
assertion and enabled INTBREAK case, ensuring both disabled and enabled paths
validate the interrupt behavior.
---
Outside diff comments:
In `@crates/rustyn64-core/src/bus.rs`:
- Around line 310-318: Update the MI_MODE write handling in the bus access match
arm to implement the documented init and EBUS clear/set command pairs,
preserving their state for subsequent reads while keeping deferred
RDRAM-transfer modes separate. Retain ClearDP behavior, and add regression
coverage that verifies both set and clear commands update the corresponding
readback bits without introducing undocumented side effects.
---
Duplicate comments:
In `@crates/rustyn64-rsp/src/sp.rs`:
- Around line 127-137: Initialize SpRegs::new so semaphore starts as taken,
matching the documented reset contract and the existing comment. In
crates/rustyn64-rsp/src/sp.rs lines 127-137, update the semaphore
initialization; in lines 428-443, add a no-write read assertion before the
release-on-write cases to verify the initial value is 1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cb9a9eca-0480-418e-9f58-e5a377136bfe
📒 Files selected for processing (3)
crates/rustyn64-core/src/bus.rscrates/rustyn64-rsp/src/sp.rscrates/rustyn64-rsp/src/su.rs
Adopted from a CodeRabbit review comment on #36. The disabled path ran through the `run` helper, which throws the `StepResult` away, so the only thing checked was that the core halted -- the half of the test named in its own title was not asserted at all. Both configurations now execute the same single BREAK through the same helper and differ only in the flag, so the differing `interrupt_change` is attributable to nothing else. The disabled case asserts `None` rather than "not raised": `Some(false)` would acknowledge an interrupt this instruction never raised, clearing a previously-raised line. Mutation-checked -- reporting `Some(status & INTBREAK != 0)` unconditionally, the natural-looking simplification, turns it red. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 21, 2026
Merged
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
Phase 2 Sprint 1: the RSP executes code. T-21-002 (SP interface registers), T-21-004 (the scalar ISA) and T-21-005 (
BREAK, halt, and the MI path).spmemSPThe run is a full 917 tests — see the note on truncation below, which is the most interesting thing in this PR.
The scalar unit
The documented MIPS subset, with the absences that matter: no multiply/divide unit at all (
MULT,DIV,MFHI, … andHI/LOthemselves), no 64-bit anything, noLWL/LWR, no traps, no likely branches. There is no exception mechanism, which has a consequence worth stating rather than leaving implicit:ADDandADDIcannot trap on overflow, so they are the same instruction asADDU/ADDIU.Two rules separate this from a CPU core reused wholesale, and both are pinned against the suite's own cases:
0xFFCcontinues at0x000.RSP Wrap aroundputsnops at0xFF8and aBREAKat0x000and expects to stop at0x4.LWat0x001returns the bytes at0x1..=0x4, and each byte address wraps inside DMEM independently, so a word read at0xFFEtakes two bytes from the end and two from the start. The identical access on the VR4300 is anAddressError— the easiest place to get the RSP wrong.The SP interface
SP_STATUSreads as a flag word and writes as set/clear command pairs. That asymmetry is the design, not an encoding quirk: it lets either processor change one flag with a single store, with no read-modify-write to race the other. The rule that falls out is the one a "clear then set" implementation silently gets wrong — writing a flag's set and clear bits together leaves it unchanged.The DMA engine moved into the RSP crate and returns a description the Bus executes, matching the PI, since the RSP does not own RDRAM. That move surfaced two bugs: the length field rounded down (hardware transfers 8 bytes for any value 0–7, so the suite's
length = 11case lost its tail), and the address registers were not double-buffered.Three finds, none of them in the RSP itself
1.
SP_SEMAPHOREwas taken four times per word read. It acquires the mutex on read, andread_u32composed a word from four byte reads — the first saw 0, the rest saw 1, and the word came back as 1 where hardware returns 0. On hardware the RCP returns the whole aligned word for one access regardless of size, so one access is the correct model.2. The MI register block did not exist.
MI_INTERRUPTwas unreadable, so a guest could raise the SP line and never observe it — which is whySP Set/Clear Interruptfailed even though the line was being set correctly all along. MODE/VERSION/INTERRUPT/MASK are implemented with their documented 4-bit mirroring.3. A hot-path allocation, caught because its own comment was wrong.
rsp_tickmoved the chip out of the Bus withcore::mem::takeunder a comment reading "No allocation" — buttakeneedsDefault, and constructing anRspallocates DMEM and IMEM. Every RCP step allocated and freed 8 KiB.Rsp::ticknow returns what it wants done instead of borrowing its owner, so there is nothing to move. That supersedes theRspBustrait, deleted rather than left unused.The truncation, which is the point
Committing T-21-002 alone dropped the failure count from 409 to 96 — and that was not progress. The old
SP_STATUSwas a stub that always read HALTED, so the suite'swait_until_rsp_is_halted()returned instantly every time. Once clearing HALT actually cleared it, and nothing set it back because the RSP still executed nothing, the suite span forever at the first test that starts the RSP.The committed runner caught it: 635 of 917 tests started. Had it only counted failures, this would have looked like a 313-test improvement and been reported as one. That intermediate commit is kept, with the diagnosis in its message, because the failure mode is more useful recorded than reconstructed.
Deliberately not done
MI_MODE's RDRAM repeat-write mode. The fiveMI Repeatfailures are its; that is RI-domain transfer behaviour, not the RSP's.MI_MASKset-and-clear-together. UnlikeSP_STATUS, the wiki does not state what that does, so the code applies clear before set rather than inventing a rule, and says so.Verification
Gates on the final commit:
cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace(421 tests),RUSTDOCFLAGS="-D warnings" cargo doc, theno_stdbuild,pre-commit run markdownlint --all-files, plus both#[ignore]d oracles.🤖 Generated with Claude Code