Skip to content

fix(core): seed the stack pointer IPL3 inherits, and decode the RI block (R-18) - #173

Merged
doublegate merged 2 commits into
mainfrom
test/f3dex-microcode-witness
Jul 29, 2026
Merged

fix(core): seed the stack pointer IPL3 inherits, and decode the RI block (R-18)#173
doublegate merged 2 commits into
mainfrom
test/f3dex-microcode-witness

Conversation

@doublegate

Copy link
Copy Markdown
Owner

The finding

Retail games never booted. Not "booted but produced no video" — never ran a
single instruction of game code. R-18's recorded theory (missing VI vblank
interrupt / RI registers / F3DEX) was wrong about the cause.

hle_boot skips IPL1/IPL2 and seeds the state IPL3 expects — Status, Config,
s3s7, the PI DOM1 timings, the CIC seed — but it never seeded sp.

IPL1 sets it before handing off (N64brew IPL2 §IPL1 listing, 0xBFC000D0):

[0xBFC000D0][0x37BD1FF0][ORI sp, sp, 0x1FF0]  # sp = 0xA4001FF0 (this prepares sp for use in IPL2)

IPL2 leaves it alone, so IPL3 runs on it — the top of RSP IMEM is IPL3's stack
while IPL3 itself executes from DMEM.

With sp = 0, IPL3's opening prologue:

0xa4000060  ADDIU sp, sp, -24
0xa4000064  SW    s3, 0(sp)     <- stores to 0xFFFF_FFE8

0xFFFF_FFE8 is KSEG3 — TLB-mapped, and no entries exist. The store raised a
TLB-refill exception, vectored to 0x8000_0000 in empty RDRAM, and executed a
NOP sled to the end of memory.

Why it was invisible

The capstone asserted retired >= 1_000_000. A machine sledding through zeroed
RDRAM retires ~180 million instructions, so "a commercial ROM boots and
executes real code" passed for years.

The tell was that retired came back identical across four different games
41666648 for all of them. That is the "same value regardless of input"
signature in docs/engineering-lessons.md, and it is what prompted tracing the
instruction stream instead of inspecting state.

Changes

  • Seed sp = 0xA400_1FF0 (sign-extended) in hle_boot, cited to the IPL1
    listing. real_pif_boot needs nothing — real IPL1 sets it.
  • Decode the RI register block (0x0470_0000..0x0470_0020) as storage.
    IPL3's first act is LW t1, 0xC(t0) on RI_SELECT and a branch on whether
    RDRAM is already up; an undecoded block returned 0 unconditionally.
  • Strengthen the local capstone to assert what was silently false: RDRAM is
    populated (≥256 KiB non-zero) and the PC is inside cached RDRAM. Retired count
    alone could not distinguish a booted game from a sled.

Measured result (120 frames each)

Title PC RDRAM populated IMEM (microcode) RDP cmds Lit pixels
Super Mario 64 0x80246ddc 928 KiB 3163 B 0 27
Star Fox 64 0x80004d98 1.07 MiB 3307 B 122 0
Star Wars: Rogue Squadron 0x80001f84 787 KiB 55 B 0 68,527
World Driver Championship 0x80074f6c 1.18 MiB 3442 B 45 0

Every title now loads its game code, programs the VI (320×237), and uploads its
graphics microcode into IMEM.

n64-systemtest is unchanged — Phase 1 categories Failed: 0, 90 suite-wide.
It loads through the ELF path, not IPL3, so no movement was expected and none
occurred.

What is deliberately left open

  • R-18's remaining half: no title ever starts the RSP. Verified at 24-tick
    sampling granularity — rsp_unhalted_samples = 0 for all four. So those 122/45
    RDP commands come from the CPU driving the DPC directly, not from
    microcode. This is also what still blocks T-71-003, and I am not claiming it.
  • R-22: the RI block is storage only. The wiki marks several read behaviours
    TOVERIFY and n64-systemtest has no RI group, so modelling them would be
    invention, not emulation.
  • R-23 (new): CIC-6105 titles (Banjo-Tooie, Ocarina, Majora's Mask) do not
    HLE-boot. Their IPL3 self-descrambles with LW t0, -0xFF0(t1) / LW t2, 0x44(t3)
    / XOR / SW over registers only the real IPL2 leaves set. They boot correctly
    through real_pif_boot. The capstone detects 6105 from the cartridge header and
    reports it by name rather than passing quietly.

Verification

Mutation-checked: removing the sp seed turns
hle_boot_seeds_the_stack_pointer_ipl3_inherits red. That test asserts the value
and that sp - 24 stays inside SP DMEM/IMEM, since the whole failure was the
prologue landing outside any mapped segment.

Gates run separately with exit status checked, no pipes: cargo fmt --all --check, cargo clippy --workspace --all-targets -- -D warnings, cargo test --workspace, RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps,
pre-commit run markdownlint --all-files, plus the #[ignore]d n64-systemtest
and both local commercial capstones.

🤖 Generated with Claude Code

…ock (R-18)

Retail games never booted. hle_boot skips IPL1/IPL2 and seeds the state IPL3
expects, but it never seeded sp. IPL1 sets it before handing off - N64brew IPL2
section, IPL1 listing at 0xBFC000D0: `ORI sp, sp, 0x1FF0  # sp = 0xA4001FF0
(this prepares sp for use in IPL2)` - and IPL2 leaves it alone, so IPL3 runs on
it, using the top of RSP IMEM as its stack while executing from DMEM.

With sp = 0, IPL3's opening prologue - ADDIU sp, sp, -24 then SW s3, 0(sp) -
stored to 0xFFFF_FFE8. That is KSEG3: TLB-mapped, and no entries exist, so the
store raised a TLB-refill exception, vectored to 0x8000_0000 in empty RDRAM, and
executed a NOP sled to the end of memory. Every retail title did this.

It hid behind the capstone's own metric. A machine sledding through zeroed RDRAM
still retires ~180 million instructions, so "boots and executes real code"
passed. The tell was that retired was IDENTICAL across four different games -
the same-value-whatever-the-input signature - and it was found by tracing the
instruction stream rather than inspecting state, which is what the engineering
lessons prescribe for exactly this.

Also decode the RI register block (0x0470_0000..0x0470_0020) as storage. IPL3's
very first act is to read RI_SELECT and branch on whether RDRAM is already up;
an undecoded block returned 0 unconditionally. Storage is the honest model here:
n64-systemtest has no RI group, and the wiki marks several read behaviours
TOVERIFY, so inventing them would be fabrication. Ledgered as R-22.

Results, measured over 120 frames each: Super Mario 64 reaches pc=0x80246ddc
with 928 KiB of RDRAM populated; Star Fox 64 submits 122 RDP commands; World
Driver Championship 45; Star Wars Rogue Squadron scans out 68,527 lit pixels.
Every title uploads its graphics microcode into IMEM. n64-systemtest is
unchanged at 90 suite-wide with Phase 1 categories still Failed: 0 - it loads
through the ELF path, not IPL3.

The local capstone now asserts what was silently false: RDRAM is populated, and
the PC is inside cached RDRAM rather than sledding past it. Retired count alone
could never have caught this.

Two things stay open and are ledgered rather than papered over. R-18's remaining
half: no title starts the RSP, so those RDP commands come from the CPU driving
the DPC directly, not from microcode - which is also what still blocks T-71-003.
And R-23: CIC-6105 titles need real_pif_boot, because their IPL3 self-descrambles
with an XOR loop over t1/t3 that only the real IPL2 leaves set. The capstone
detects 6105 from the cartridge header and reports it by name instead of passing
quietly; the real-PIF capstone boots those titles and asserts on them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes
    • Improved retail game booting so more titles reliably transition into their own code.
    • Corrected IPL3 startup to avoid invalid execution paths that could prevent video initialisation.
    • Added proper RI register handling during startup to ensure boot-time branching works as expected.
  • Tests
    • Strengthened commercial boot validation with checks for final program counter and non-zero RDRAM contents.
    • Skips unsupported CIC-6105 ROMs during this boot path test.
  • Documentation
    • Updated the accuracy ledger entry for R-18 with the resolved root cause.

Walkthrough

Retail HLE boot now seeds IPL3’s stack pointer and models RI register access. Commercial boot tests validate RDRAM population and final PC placement, skip CIC-6105 ROMs, and return structured results. Changelog and accuracy-ledger entries document the corrected behaviour and remaining gaps.

Changes

Retail HLE boot

Layer / File(s) Summary
IPL3 boot state and RI access
crates/rustyn64-core/src/boot.rs, crates/rustyn64-core/src/bus.rs
hle_boot seeds register 29 for IPL3, and CPU bus reads and writes now decode the RI register block, including RI_SELECT, with focused regression coverage.
Commercial boot evidence
crates/rustyn64-test-harness/tests/commercial_boot.rs
Commercial boot results now include retired instructions, lit pixels, non-zero RDRAM bytes, and the final PC; tests enforce RDRAM and KSEG0 execution checks and skip CIC-6105 ROMs.
Changelog and accuracy records
CHANGELOG.md, docs/accuracy-ledger.md
The R-18 records describe the corrected boot path, observed retail execution, and remaining RSP and CIC-related gaps.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant hle_boot
  participant CPU
  participant RI_register_block
  participant IPL3
  participant RDRAM
  hle_boot->>CPU: seed stack pointer
  IPL3->>CPU: read RI_SELECT
  CPU->>RI_register_block: access RI register
  RI_register_block-->>CPU: return stored value
  IPL3->>RDRAM: execute retail boot code
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 8 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Title check ⚠️ Warning The title matches the change, but it exceeds the 72-character limit in the commit-title rule. Shorten it to 72 characters or fewer while keeping type(scope): imperative subject.
Docs-As-Spec Sync ⚠️ Warning docs/DOCUMENTATION_INDEX.md says core specs stay in sync; cart.md owns boot, but this PR changed boot/bus behaviour without updating that spec. Update docs/cart.md for the HLE-boot stack-pointer seed and RI storage semantics, or explain in the PR body why no spec change was needed.
✅ Passed checks (8 passed)
Check name Status Explanation
Description check ✅ Passed The description is clearly about the same boot-path fix and follow-on RI/CIC changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Oracle Number Is Stated ✅ Passed R-18 explicitly states “n64-systemtest impact: none” and says the suite-wide count is 90, satisfying the oracle-number rule.
Changelog Entry For User-Visible Changes ✅ Passed PASS: CHANGELOG.md has an [Unreleased] entry for the retail-boot fix, matching the user-visible change rule.
Measured, Never Tuned ✅ Passed PASS: the sourced literals are cited/ledgered (C-32, R-18, R-22); MIN_RDRAM_NONZERO is just a test cutoff, not a hardware constant.
Unsafe Stays Out Of The Chip Crates ✅ Passed No actual unsafe syntax appears in the changed files or outside the frontend, and all chip/core crates still have #![forbid(unsafe_code)].

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/rustyn64-test-harness/tests/commercial_boot.rs (1)

112-134: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Fail when no HLE-eligible ROM was exercised.

Line 119 sets any before CIC filtering, and .find(...) selects only one ROM per directory. If every selected ROM is CIC-6105, this test returns success without calling boot_and_run or evaluating any boot assertion. Track an HLE-tested count, scan past skipped ROMs, and fail when staged input produced no eligible HLE execution.

As per path instructions, the accuracy oracle must not report success when its corpus was skipped.

🤖 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-test-harness/tests/commercial_boot.rs` around lines 112 -
134, Update the commercial boot test around the ROM selection and CIC-6105 skip
so it scans candidates until finding an HLE-eligible ROM instead of stopping at
the first match. Track a count of ROMs actually exercised through boot_and_run
and assert or fail at the end when staged input produced zero HLE executions; do
not treat the existing any flag as sufficient.

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.

Outside diff comments:
In `@crates/rustyn64-test-harness/tests/commercial_boot.rs`:
- Around line 112-134: Update the commercial boot test around the ROM selection
and CIC-6105 skip so it scans candidates until finding an HLE-eligible ROM
instead of stopping at the first match. Track a count of ROMs actually exercised
through boot_and_run and assert or fail at the end when staged input produced
zero HLE executions; do not treat the existing any flag as sufficient.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 1cf61c98-9ab1-42c2-a2d8-d64548d6cbd4

📥 Commits

Reviewing files that changed from the base of the PR and between dea573e and 24a7d97.

📒 Files selected for processing (5)
  • CHANGELOG.md
  • crates/rustyn64-core/src/boot.rs
  • crates/rustyn64-core/src/bus.rs
  • crates/rustyn64-test-harness/tests/commercial_boot.rs
  • docs/accuracy-ledger.md

Review follow-up on the R-18 fix.

Rejecting the sub-word-write concern, but with a test rather than an assertion.
The worry was that is_ri_register was added to read_u8/read_u32/write_u32 but
not to write_u8, so narrow stores to RI would fall through or be dropped. The
premise does not hold: narrow CPU stores reach the bus through write_sized,
which shifts the register into its byte lane and funnels to
write_u32(addr & !3, word) - the RCP's documented size-blind path, where the
whole word latches and the access size is ignored. None of MI/DP/VI/AI/SI appear
in write_u8 either, so adding RI there would be inconsistent AND would bypass
that semantics. a_narrow_store_to_ri_latches_the_whole_word now pins it: a byte
store of 0x12 into lane 3 leaves 0x0000_0012, and a halfword store of 0xABCD
into the upper lane leaves 0xABCD_0000.

Read only the 0x1000-byte boot header in is_cic_6105 instead of the whole image.
The CIC is resolved from the header plus IPL3, both inside it, so this drops a
duplicate 8-64 MiB read per ROM while leaving the classification identical -
still parsed via Cart::load, so it cannot drift from what actually boots.

Name the stack-pointer register index GPR_SP rather than writing a bare 29,
matching how the COP0 seeds read as reg::STATUS / reg::CONFIG.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@doublegate

Copy link
Copy Markdown
Owner Author

Adjudicating the Antigravity review.

1. Sub-word writes to RI (write_u8) — REJECTED, with a test rather than an assertion.

The premise does not hold here. Narrow CPU stores do not reach RCP registers through write_u8 at all — they go through write_sized, which shifts the register into its byte lane and funnels to write_u32(addr & !3, word). That is the RCP's documented size-blind path: the whole 32-bit word latches and both the access size and the low two address bits are ignored (N64brew Memory map §Physical Memory Map accesses; n64-systemtest states it outright in src/tests/sp_memory/mod.rs).

Two corroborating facts: none of MI/DP/VI/AI/SI appear in write_u8 either, so adding RI there would be inconsistent with every peer block; and doing so would actively bypass the size-blind semantics, turning a documented hardware quirk into a partial byte update.

Rather than leave that as a code-reading claim, it is now pinned — a_narrow_store_to_ri_latches_the_whole_word:

  • byte store of 0x12 into lane 3 over 0xFFFF_FFFF0x0000_0012 (not 0xFFFF_FF12, not unchanged)
  • halfword store of 0xABCD into the upper lane → 0xABCD_0000

Thanks for raising it — "silently dropped" was exactly the right thing to be suspicious of, and it deserved a test either way.

2. Duplicate disk I/O in is_cic_6105 — ADOPTED.

Fixed, though by a smaller change than suggested: it now reads only the 0x1000-byte boot header rather than the whole 8–64 MiB image. The CIC is resolved from the header plus IPL3, both of which live inside that window, so this removes the large duplicate read without restructuring boot_and_run's signature — and the classification still goes through Cart::load, so it cannot drift from what actually boots.

3. Nitpick: magic register index 29 — ADOPTED.

Now GPR_SP, so the seed reads as intent the way the neighbouring reg::STATUS / reg::CONFIG writes do.

@github-actions

Copy link
Copy Markdown

Antigravity review (Gemini via Ultra)

Fixes commercial ROM boot failures under HLE by seeding the MIPS stack pointer ($sp / $29) to 0xA4001FF0 in hle_boot, decoding the RI register block (0x0470_0000), and strengthening commercial boot harness assertions to check RDRAM population and PC location.

Blocking issues

None found.

Suggestions

  • crates/rustyn64-core/src/boot.rs (lines 70, 237-247): GPR_SP was introduced to eliminate magic register indices, but hle_boot_seeds_the_stack_pointer_ipl3_inherits hardcodes 29 in sys.cpu.regs.read(29). Use GPR_SP in the test.
  • crates/rustyn64-core/src/boot.rs (lines 128-132): hle_boot uses GPR_SP for $sp but continues to use bare literals (19, etc.) for $s3$s7. Define constants for the remaining boot registers to keep register access style consistent.
  • crates/rustyn64-test-harness/tests/commercial_boot.rs (lines 67–81, 136): is_cic_6105 opens and reads the ROM header from disk immediately before boot_and_run opens and reads the full ROM file. Check the CIC from the already-in-memory ROM slice or pass the header buffer to avoid redundant file I/O for every test title.

Nitpicks

  • docs/accuracy-ledger.md (line 413): Inconsistent digit formatting (68 527 vs 68,527 in CHANGELOG.md).

Automated first-pass review by agy on a self-hosted runner -- not a human review.

@doublegate
doublegate merged commit cf240f6 into main Jul 29, 2026
10 of 11 checks passed
@doublegate
doublegate deleted the test/f3dex-microcode-witness branch July 29, 2026 04:32

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
crates/rustyn64-core/src/bus.rs (2)

585-585: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Document the new public Bus::RI_BASE constant.

pub const RI_BASE needs rustdoc, including the hardware address source. The workspace rule requires documentation for every new public Rust item, and the documentation gate treats missing_docs warnings as errors.

As per coding guidelines, every new public Rust item requires rustdoc because the documentation gate treats warnings as errors.

🤖 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` at line 585, Add rustdoc immediately above
the public Bus::RI_BASE constant, describing its purpose and specifying that
0x0470_0000 is the hardware address source. Ensure the documentation satisfies
the workspace missing_docs requirement without changing the constant’s value or
visibility.

Source: Coding guidelines


1451-1453: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Remove RI from the remaining-decode TODO.

The new branch now decodes RI, but the TODO at Line 1489 still claims that RI is undecoded. At minimum, remove RI from that list so the comment matches the implementation.

As per path instructions, comments that disagree with the implementation are correctness hazards. Based on learnings, comments and documentation do not enforce behaviour and must be checked against the code.

🤖 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 1451 - 1453, Update the
remaining-decode TODO near the RI read handling to remove RI from its list of
undecoded registers, while preserving the newly added is_ri_register decoding
branch and all other TODO entries.

Sources: Path instructions, Learnings

crates/rustyn64-test-harness/tests/commercial_boot.rs (2)

129-173: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Bound the final PC to actual cached RDRAM.

r.pc & 0xE000_0000 == 0x8000_0000 accepts every address in the 512 MiB KSEG0 range, not just the RDRAM-backed prefix. A sled executing from an unmapped KSEG0 address could therefore satisfy this capstone.

Carry the actual sys.bus.rdram.len() through BootResult and assert that the KSEG0 physical offset is below that length.

As per path instructions, the accuracy harness must not report success when the execution evidence does not prove that the game ran from mapped RDRAM.

🤖 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-test-harness/tests/commercial_boot.rs` around lines 129 -
173, Bound the final-PC validation in the boot harness to the actually mapped
RDRAM region. Extend BootResult to carry sys.bus.rdram.len(), then update the
assertion around r.pc so it verifies the KSEG0 physical offset is below that
length, rather than accepting the entire KSEG0 range; preserve the existing
cached-RDRAM requirement and failure diagnostics.

Source: Path instructions


103-105: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Pin the RDRAM cutoff to oracle evidence. MIN_RDRAM_NONZERO = 256 * 1024 is still an unproven acceptance rule; ledger R-18 explains the empty-RDRAM false-positive, but it does not justify this boundary. Record the measurement/provenance in docs/accuracy-ledger.md or derive the cutoff from pinned oracle expectations.

🤖 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-test-harness/tests/commercial_boot.rs` around lines 103 -
105, Document the provenance and measured oracle evidence supporting
MIN_RDRAM_NONZERO in docs/accuracy-ledger.md, or replace the hard-coded cutoff
with a value derived from pinned oracle expectations. Ensure the acceptance rule
in the commercial boot test is explicitly justified and remains aligned with the
documented oracle behavior.

Sources: Coding guidelines, 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.

Outside diff comments:
In `@crates/rustyn64-core/src/bus.rs`:
- Line 585: Add rustdoc immediately above the public Bus::RI_BASE constant,
describing its purpose and specifying that 0x0470_0000 is the hardware address
source. Ensure the documentation satisfies the workspace missing_docs
requirement without changing the constant’s value or visibility.
- Around line 1451-1453: Update the remaining-decode TODO near the RI read
handling to remove RI from its list of undecoded registers, while preserving the
newly added is_ri_register decoding branch and all other TODO entries.

In `@crates/rustyn64-test-harness/tests/commercial_boot.rs`:
- Around line 129-173: Bound the final-PC validation in the boot harness to the
actually mapped RDRAM region. Extend BootResult to carry sys.bus.rdram.len(),
then update the assertion around r.pc so it verifies the KSEG0 physical offset
is below that length, rather than accepting the entire KSEG0 range; preserve the
existing cached-RDRAM requirement and failure diagnostics.
- Around line 103-105: Document the provenance and measured oracle evidence
supporting MIN_RDRAM_NONZERO in docs/accuracy-ledger.md, or replace the
hard-coded cutoff with a value derived from pinned oracle expectations. Ensure
the acceptance rule in the commercial boot test is explicitly justified and
remains aligned with the documented oracle behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c3f68e92-82b4-4e1b-8782-b235fda60142

📥 Commits

Reviewing files that changed from the base of the PR and between 24a7d97 and 6eaa1f4.

📒 Files selected for processing (3)
  • crates/rustyn64-core/src/boot.rs
  • crates/rustyn64-core/src/bus.rs
  • crates/rustyn64-test-harness/tests/commercial_boot.rs

doublegate added a commit that referenced this pull request Jul 29, 2026
Adjudicating five CodeRabbit "outside diff range" findings on #173 that I missed
before merging it - those live in the review body, not in the inline-comment
endpoint I was fetching.

The important one: the commercial-boot capstone set its `any` flag before the
CIC-6105 filter and took only the FIRST ROM per save-type folder, so a folder
whose first ROM was 6105 contributed nothing while still counting as staged. In
practice three of the five folders selected 6105 titles, so the test reported
success on two folders' worth of evidence while looking like five. Now it scans
past skipped ROMs to the first HLE-eligible title, counts folders actually put
through boot_and_run separately from folders merely present, and FAILS when
staged input exercised nothing. Coverage went from 2 of 5 folders to 5 of 5, and
Excitebike 64 turns out to render 10,042 lit pixels.

MIN_RDRAM_NONZERO was an invented 256 KiB. It is now derived from the documented
IPL3 copy size (IPL3_COPY_BYTES / 4), and the measured corpus range that
justifies it - 539 KiB to 1.23 MiB for booted titles, exactly 0 for a machine
that faults out of IPL3 - is recorded in ledger R-18 rather than left implicit.

The final-PC assertion used a hard-coded 0x8080_0000. KSEG0 spans 512 MiB and
only the installed RDRAM is backed, so it now bounds by the real rdram.len(),
carried through BootResult. (The finding described the check as
`pc & 0xE000_0000`, which it was not, but the underlying point stands.)

Two stale comments. Bus::RI_BASE had rustdoc but no hardware provenance; it now
cites N64brew RDRAM Interface and enumerates the eight registers. And the
T-CORE-01 TODO listed SP/DP/VI/AI/SI/RI/MI plus the PIF as undecoded - all seven
of those are decoded, some for several phases. It now names what is actually
outstanding: the RDRAM device registers at 0x03F0_0000. A comment that lists
finished work as pending is the same hazard as one that contradicts the code.

Also switches the vestigial Rsp::pc/halted fields from private back to pub with
#[deprecated], per the review on this PR: privatising them is a breaking API
change and module 70 reserves those for an announced release. Deprecation is
non-breaking, and with -D warnings any read is still a hard error. That in turn
exposed that the constructs_halted test asserted on the dead `halted` field,
which is unconditionally true - it now asserts through Rsp::halted().

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
doublegate added a commit that referenced this pull request Jul 29, 2026
Adjudicating a further outside-diff finding: a bare `return` is recorded as
success, so the witness could report a pass having verified nothing.

The local-only absence path is now checked ONCE, before discovery: if the corpus
root does not exist, skip and say so. Past that point every failure is an error.
A save-type folder that is not staged is reported and skipped; a folder that
exists but cannot be READ now panics, instead of being indistinguishable from
"no ROMs here". And reaching the end with nothing staged - the root exists, the
test was explicitly invoked with --ignored - is now an assertion failure naming
the path, not a quiet success.

This is the third variant of the same failure in this session: the #173 `any`
flag, the ReadDir flatten, and now the empty-corpus return. An oracle that runs
nothing looks exactly like one that passes, and it takes a deliberate assertion
each time to tell them apart.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
doublegate added a commit that referenced this pull request Jul 29, 2026
* feat(harness): witness a retail title's own microcode on the LLE RSP (T-71-003)

Retail microcode executes. Castlevania Legacy of Darkness visits 805 distinct
RSP PCs and submits 33 RDP commands; 007 The World Is Not Enough 459; Beetle
Adventure Racing 356; Star Fox 64 331; Super Mario 64 236; Mega Man 64 229 with
50 RDP commands; World Driver Championship 148. That is ADR 0002's payoff on
real commercial microcode, and there is no microcode-specific code path anywhere
in the tree - F3DEX and its vendor variants run for the same reason libdragon's
rdpq does.

The new capstone asserts the whole chain rather than any one link: microcode
lands in IMEM, the RSP leaves halt, and its PC visits many distinct IMEM
addresses. Only the last is load-bearing. Bytes in IMEM prove the CPU worked, not
the RSP; leaving halt proves nothing if the core then stalls; a PC that visits
hundreds of addresses cannot be produced by a halted, stalled or spinning core.
Mutation-checked by stubbing Rsp::tick, which turns it red.

This also corrects a claim I made in #173. That PR said "no title starts the RSP,
so those RDP commands come from the CPU driving the DPC directly". It was wrong.
It was measured off Rsp::halted and Rsp::pc - two pub struct fields that are
NEVER written anywhere. The authoritative state is SP_STATUS, via sp.halted() and
sp.pc(), which is what su_step itself gates on. Sampling the dead fields reports
"halted forever at PC 0" for a running RSP, and it produced two confident wrong
conclusions in a single session: first "the RSP never starts", then "its PC never
advances". That is the inert-API hazard docs/engineering-lessons.md 3.2
describes, and it is worse than an unused function because it answers.

The fields are now private, with Rsp::halted() and Rsp::pc() accessors delegating
to SP_STATUS. They stay in the struct rather than being deleted because serde
serialises them positionally, so removing them would change the save-state layout
- an announced-in-advance format break under ADR 0005, not something to do as a
side effect. Private is enough: nothing outside the crate can read the stale
value any more.

The ledger and CHANGELOG carry the correction explicitly rather than being
quietly edited, since the wrong claim is already in a merged PR body.

Genuinely still open, and now stated precisely: several titles (Blast Corps,
Bomberman 64, Donkey Kong 64, Jet Force Gemini) never load microcode into IMEM at
all, and Rogue Squadron never starts the RSP - those boots stall earlier than the
RSP seam.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(harness): the HLE capstone could pass having asserted nothing

Adjudicating five CodeRabbit "outside diff range" findings on #173 that I missed
before merging it - those live in the review body, not in the inline-comment
endpoint I was fetching.

The important one: the commercial-boot capstone set its `any` flag before the
CIC-6105 filter and took only the FIRST ROM per save-type folder, so a folder
whose first ROM was 6105 contributed nothing while still counting as staged. In
practice three of the five folders selected 6105 titles, so the test reported
success on two folders' worth of evidence while looking like five. Now it scans
past skipped ROMs to the first HLE-eligible title, counts folders actually put
through boot_and_run separately from folders merely present, and FAILS when
staged input exercised nothing. Coverage went from 2 of 5 folders to 5 of 5, and
Excitebike 64 turns out to render 10,042 lit pixels.

MIN_RDRAM_NONZERO was an invented 256 KiB. It is now derived from the documented
IPL3 copy size (IPL3_COPY_BYTES / 4), and the measured corpus range that
justifies it - 539 KiB to 1.23 MiB for booted titles, exactly 0 for a machine
that faults out of IPL3 - is recorded in ledger R-18 rather than left implicit.

The final-PC assertion used a hard-coded 0x8080_0000. KSEG0 spans 512 MiB and
only the installed RDRAM is backed, so it now bounds by the real rdram.len(),
carried through BootResult. (The finding described the check as
`pc & 0xE000_0000`, which it was not, but the underlying point stands.)

Two stale comments. Bus::RI_BASE had rustdoc but no hardware provenance; it now
cites N64brew RDRAM Interface and enumerates the eight registers. And the
T-CORE-01 TODO listed SP/DP/VI/AI/SI/RI/MI plus the PIF as undecoded - all seven
of those are decoded, some for several phases. It now names what is actually
outstanding: the RDRAM device registers at 0x03F0_0000. A comment that lists
finished work as pending is the same hazard as one that contradicts the code.

Also switches the vestigial Rsp::pc/halted fields from private back to pub with
#[deprecated], per the review on this PR: privatising them is a breaking API
change and module 70 reserves those for an announced release. Deprecation is
non-breaking, and with -D warnings any read is still a hard error. That in turn
exposed that the constructs_halted test asserted on the dead `halted` field,
which is unconditionally true - it now asserts through Rsp::halted().

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs(rsp): record the witness parameters' provenance and the vestigial fields

Adjudicating CodeRabbit's three failed pre-merge checks.

Measured, Never Tuned. The witness introduced SAMPLE_TICKS, MIN_DISTINCT_PCS and
a frame count with no recorded provenance, which by this project's own rule makes
the result unfalsifiable. All three are now justified by measurement rather than
assertion, in the code and in ledger R-18:

- MIN_DISTINCT_PCS = 32 separates two measured populations over an order of
  magnitude apart. Titles whose RSP never runs measure 0 distinct PCs; titles
  whose microcode runs measure 148 to 815. Any value inside that gap gives the
  same verdict.
- SAMPLE_TICKS = 24 is a cadence, not a hardware value: 8 RCP steps, and sampling
  can only under-count, so every figure is a lower bound. Re-run at 3 - the
  finest possible, 8x finer - the verdict is unchanged: 805 to 815, 459 to 463,
  356 to 356, 229 to 258, same four witnesses.
- FRAMES = 90 doubled to 180 gives an identical witness set with unchanged
  counts. I had written "doubling adds no new witnesses" before measuring it;
  now measured, and it holds.

Docs-As-Spec. docs/rsp.md still showed pub pc and pub halted as ordinary state.
It now shows them deprecated, documents Rsp::pc()/halted() and the authoritative
sp: SpRegs, and explains why this matters rather than just noting it: sampling
the fields reports "halted forever at PC 0" for a running RSP, which produced two
wrong conclusions in one session. An unused function is inert; an unused field
answers.

The title-length check is addressed by retitling the PR.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(harness): a NOP sled through IMEM could have satisfied the microcode witness

Five more review findings, across both the inline and outside-diff channels.

The substantive one is a real hole in my own claim. distinct_pcs counted visited
IMEM addresses regardless of what was AT them, so an unhalted RSP walking
zero-filled IMEM - executing NOPs - marches through hundreds of addresses and
looks identical to real microcode by that measure. That is the same NOP-sled trap
that hid R-18 on the CPU side, reproduced in the test written to close it. The
witness now fetches the instruction word at each sampled PC and counts only PCs
holding a NON-ZERO word, and the threshold applies to that count. The claim gets
strictly stronger rather than weaker: Castlevania 805 visited / 774 executing,
007 459/435, Beetle Adventure Racing 356/328, Mega Man 64 229/223.

The accessor test could not fail. Both representations start halted = true and
pc = 0, so constructs_halted passed whether halted()/pc() read SP_STATUS or the
vestigial fields. It now mutates rsp.sp after construction and requires the
accessors to follow, which fields that are never written cannot do.
Mutation-checked by pointing halted() back at the dead field: red, with the
intended message.

ReadDir entry failures were flattened away, so a discovery failure could shrink
or empty the corpus while the run still looked complete. They now panic with the
folder and the error.

Two doc corrections in docs/rsp.md. The interface block still showed
tick<B: RspBus>(&mut self, bus: &mut B) when the signature is
tick(&mut self) -> su::StepResult. And my own new prose was imprecise about
provenance: Rsp::pc() reads SP_PC via SpRegs::pc(); only Rsp::halted() reads
SP_STATUS.HALT. It also said the fields were private when they are pub and
deprecated - privatising them would itself be the breaking change this PR
avoided.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(harness): an empty corpus made the microcode witness pass silently

Adjudicating a further outside-diff finding: a bare `return` is recorded as
success, so the witness could report a pass having verified nothing.

The local-only absence path is now checked ONCE, before discovery: if the corpus
root does not exist, skip and say so. Past that point every failure is an error.
A save-type folder that is not staged is reported and skipped; a folder that
exists but cannot be READ now panics, instead of being indistinguishable from
"no ROMs here". And reaching the end with nothing staged - the root exists, the
test was explicitly invoked with --ignored - is now an assertion failure naming
the path, not a quiet success.

This is the third variant of the same failure in this session: the #173 `any`
flag, the ReadDir flatten, and now the empty-corpus return. An oracle that runs
nothing looks exactly like one that passes, and it takes a deliberate assertion
each time to tell them apart.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant