feat(cpu): Status.RE, privilege-aware addressing, and the COP1 residuals that close Phase 1 - #33
Merged
Merged
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Review limit reached
Next review available in: 2 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR adds privilege- and width-aware segment translation, reverse-endian memory and instruction access, COP1 condition forwarding, floating-point and TLB corrections, cache-state validation, 64-bit operation reservation checks, and sign-extended address handling. ChangesMIPS Phase 1 semantics
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CPU
participant ic_stage
participant translate_via
participant TLB
participant Memory
CPU->>ic_stage: request instruction fetch
ic_stage->>translate_via: classify and translate virtual address
translate_via->>TLB: resolve mapped segment
TLB-->>translate_via: return physical address
translate_via->>Memory: apply User-mode byte-lane swap and fetch
Memory-->>ic_stage: return instruction bytes
Possibly related PRs
🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR advances the VR4300 address-translation and privilege model to better match hardware behavior and n64-systemtest’s Privilege: / RE expectations, by making segment classification depend on privilege + addressing width, enforcing compatibility (sign-extension) rules in 32-bit addressing, and implementing Status.RE reverse-endian behavior in User mode (including instruction fetch).
Changes:
- Implement
Status.REreverse-endian swapping in User mode only, applied post-translation on the physical address (including instruction fetch). - Make the segment map privilege- and width-aware (User/Supervisor/Kernel differences, 64-bit segment widening, XKPHYS), and distinguish address errors from TLB faults at the translation boundary.
- Enforce 32-bit compatibility (sign-extension) addressing and reserve MIPS III 64-bit ops in 32-bit non-kernel modes; update harness/tests/docs accordingly.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/STATUS.md | Updates Phase 1 failure counts to reflect improved n64-systemtest results. |
| docs/cpu.md | Documents privilege/width-dependent segment mapping, sign-extension rule, Status.RE, and reserved 64-bit op behavior. |
| docs/accuracy-ledger.md | Adds residual entry R-1 with correlated capture findings for an outstanding half-mode FP issue. |
| CHANGELOG.md | Records the newly modeled behaviors and their impact on Phase 1 category counts. |
| crates/rustyn64-cpu/src/addr.rs | Introduces privilege+width-aware segment classification, XKPHYS support, compatibility checks, and TranslateError to separate address errors from TLB faults. |
| crates/rustyn64-cpu/src/pipeline.rs | Wires new translation API into pipeline, adds Status.RE handling for data and fetch, and raises address errors pre-TLB for invalid segments; adds 64-bit reserved-encoding check. |
| crates/rustyn64-cpu/src/decode.rs | Adds Op::is_64_bit() classification for gating reserved 64-bit operations. |
| crates/rustyn64-cpu/src/lib.rs | Fixes reset PC to be a sign-extended 64-bit address and updates corresponding tests/comments. |
| crates/rustyn64-test-harness/src/rom.rs | Sign-extends ROM entry point to a 64-bit address to avoid invalid zero-extended kernel addresses. |
| crates/rustyn64-test-harness/tests/first_rom.rs | Updates expected entry/subroutine ranges and direct-load calls to use sign-extended kernel addresses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Owner
Author
|
Added one more commit,
The manual declines to specify this (an odd register with It does not revise C-14, which governs Subject-wise this belongs with #31, not here; it sits on top only because it depends on the stack. Phase 1's categories: 23 → 19. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@CHANGELOG.md`:
- Around line 12-69: Run the pinned markdownlint-cli v0.49.1 hook with
pre-commit across all files and resolve every reported Markdown issue in
CHANGELOG.md (lines 12-69), docs/STATUS.md (line 181), and
docs/accuracy-ledger.md (line 178), preserving the documented content.
In `@crates/rustyn64-cpu/src/decode.rs`:
- Around line 448-477: Update the operation classification match containing
`Self::Ld` and related 64-bit instructions to include `Self::Lwu`, so `LWU` is
treated as a 64-bit operation and raises RI in narrow non-Kernel modes. Extend
the reservation test for this classification with an `LWU` encoding whose result
differs between narrow and wide User mode.
In `@docs/STATUS.md`:
- Line 181: Use docs/STATUS.md as the authoritative Phase 1 source: update the
status row near line 181 to match the surrounding statement about whether the
ROM executed and the current count of 23 failures. In CHANGELOG.md lines 27 and
49, label the 24→23 and 27→24 figures as historical checkpoints or revise them
into one consistent cumulative progression; at line 68, remove the presentation
of 31→27 as the final result and align it with the authoritative current count
of 23.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8c4b7107-64d7-4d54-b666-5ae2f5d6c49b
📒 Files selected for processing (10)
CHANGELOG.mdcrates/rustyn64-cpu/src/addr.rscrates/rustyn64-cpu/src/decode.rscrates/rustyn64-cpu/src/lib.rscrates/rustyn64-cpu/src/pipeline.rscrates/rustyn64-test-harness/src/rom.rscrates/rustyn64-test-harness/tests/first_rom.rsdocs/STATUS.mddocs/accuracy-ledger.mddocs/cpu.md
doublegate
added a commit
that referenced
this pull request
Jul 21, 2026
Two functional, four inaccurate comments, two documentation. LWU joins the 64-bit operations (CodeRabbit, #33). It was missing because the set was built from n64-systemtest's 28 tested instructions, and the ROM does not cover it. The manual states the rule once, as the EPSILON marker in the opcode table: "valid in the 64-bit mode and 32-bit Kernel mode. In the 32-bit User or Supervisor mode, this code generates the reserved instruction exception." Reading that legend rather than the per-instruction notes is what found the gap, and the doc comment now cites it so the next reader checks the same place. The oracle is unchanged at Failed: 0, so this adds a rule without disturbing a measured one. Index_Store_Tag now requires the cache's OWN valid PState -- 2 for the I-cache, 3 for the D-cache -- rather than any non-zero field (Copilot, #32). The old form would conjure a live line from an undefined encoding, and would let one cache's Valid validate the other's. Pinned by a test that walks the rejected values. The four comments were each describing code that had moved on: - The FpCommit::Single comment still said "preserves the upper half" while the code calls write_s_arith, which clears it. This is the fourth time in this project a comment has outlived its code; the rule it states was right and the attribution was not. - The Index_* cache comment claimed segment translation leaves the index bits alone. False for every mapped segment -- translation preserves only the low 12 bits, while the D-cache index reaches bit 12 and the I-cache bit 13. The real situation is the D-6 deviation, and it now says so. - translate_re's Errors section named only TLB faults; it can also raise AddressError. - The KSU == 3 fallback was described as "most restrictive-to-reach" when Kernel is the most permissive. Documentation: D-6 no longer claims physical indexing is strictly more coherent than the hardware -- it is a divergence in both directions, and the entry now names the two observable cases and bounds the tested scope to KSEG0. The STATUS.md gate row no longer reads "Failed: 0" as a label while reporting a non-zero count in the same row. The MOV.S test asserted only the low word, so a formatted half-copy would have passed its whole-register claim (CodeRabbit, #31). It now asserts read_raw. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
doublegate
force-pushed
the
feat/vr4300-caches
branch
from
July 21, 2026 04:30
84ee3eb to
020f1b8
Compare
Reversing endianness on a 64-bit datapath is a permutation of byte lanes within the doubleword, expressed as an XOR of the low address bits: a doubleword does not move, a word moves by 4, a halfword by 6 and a byte by 7. Kernel and Supervisor are unaffected, and so is any access taken while EXL/ERL forces kernel mode -- the same rule the coprocessor-usability check already applies. Instruction fetch is swapped too, being a 4-byte access like any other. That is why n64-systemtest emits its reverse-endian programs with each instruction PAIR exchanged for them to execute in order. The swap is applied to the PHYSICAL address, after translation. It touches only bits 2:0, which every translation maps identically, so it is exactly equivalent to swapping the virtual address first -- and it keeps BadVAddr raw on a fault, which the suite asserts directly in its own test. The LWL/LWR/SWL/SWR family under RE is deliberately NOT included: it addresses individual bytes rather than a fixed-width container, so it needs a different rule, and guessing one would be a fitted constant in another guise. Two assertions still fail there and are recorded as outstanding in docs/cpu.md. n64-systemtest, Phase 1's categories: 31 -> 27. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…g width KSEG0 does not exist in User mode. The map was a function of the address alone, so a user program reached 0x8000_1000 exactly as the kernel does -- and it is the address-space check, not the TLB, that is supposed to stop it. The map is now a function of (address, mode, width). User sees USEG alone; Supervisor sees SUSEG and SSEG; Kernel sees the whole map. Anything else is an address error raised BEFORE the TLB is consulted. That distinction is load-bearing rather than cosmetic: folding it into a TLB refill would send the offending program to the refill handler, where a well-behaved kernel maps the page and grants the access it was never allowed to make. Hence a separate TranslateError variant rather than a synthesised TlbFault. With Status.KX/SX/UX set, each mapped segment widens to 2^40 and the space grows holes -- an address inside a segment's region but past its size faults. Kernel additionally gains XKPHYS: eight 2^32 direct windows chosen by bits 61:59, differing only in cacheability, and by the same rule as a TLB entry's C field, only C == 2 is uncached. Under 32-bit addressing an address must be the sign extension of its low word. 0x0000_0000_8000_1000 is not a shorthand for KSEG0; it is an address error, and n64-systemtest asserts that directly. The old code truncated to u32 and accepted it silently -- and this project's own reset vector and ROM entry point were both stored in the truncated form, so both are corrected here. Every kernel address in the test suite needed the same correction, which is the honest reading: those tests were passing on an address the hardware rejects. One test moved rather than being adjusted: cop0_is_unusable_in_user_mode_without_cu0 ran its program from KSEG0 in User mode, which now raises AdEL at the FETCH -- so it would have "passed" on the wrong exception. It maps page-pair 0 and runs from USEG instead, which is the only place a user program can be. n64-systemtest, Phase 1's categories: 27 -> 24. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rvisor mode DADD, DSLL, LD, SD and the rest of the MIPS III doubleword set raise Reserved Instruction when the current mode's UX/SX bit is clear and the mode is not Kernel. Kernel may use them at any width, so this cannot be a property of Status.KX alone. Both halves of that condition are mutation-checked, because only one of them fails loudly: gating on the width bit alone reserves them for a 32-bit KERNEL, which is the mode every N64 boots into, so that mistake breaks everything. Gating on the mode alone reserves them for a 64-bit USER program, which nothing common does -- that one would sit unnoticed behind the rows that still pass. Deliberately excluded: DMFC0/DMTC0 and DMFC1/DMTC1. Doubleword moves to and from a coprocessor follow that coprocessor's own usability and reserved-encoding rules (ledger C-18) and raise different exceptions; folding them in here would make an already-unusable COP0 report the wrong cause. The check runs after coprocessor usability, not before, so an unusable coprocessor is still reported as such when the encoding is also a 64-bit one. n64-systemtest, Phase 1's categories: 24 -> 23. The whole Privilege group passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… NOT The failing assertion names ADD.S, and ADD.S is not the defect: running the identical instruction in isolation with the ROM's own operands produces the expected result in both FR modes. Recording that rules out the obvious suspect before the next session spends the same effort re-deriving it. The observed value is 16f32 in the high half with the low half cleared, which is the signature of a single-precision write to an ODD register under FR = 0 -- and the only instruction in the block that produces exactly that targets fgr[16]. That is a lead, not a conclusion, and the entry says so. The status line names the next step as a correlated capture at the ROM's own marker rather than further reasoning, because two rounds of reasoning have already produced two wrong answers here -- the same trap C-10 recorded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… the easy fix A correlated capture armed on the ROM's own marker settles which instruction is at fault, and it is not the one the assertion names: ADD.S $0 executes correctly, and it is ADD.S $1, $29, $30 five rows later whose result is wrong. The pipeline depth is exactly that gap, so the assertion index and the guilty instruction are one apart -- which is what sent two earlier attempts to the wrong place. The entry also records what the evidence RULES OUT, which is the more useful half. The expected table is self-contradictory under every one-line index-remap rule: result[1] needs $29 to read as $28, while result[2] needs $31 to read as itself rather than as $30. And a companion failure shows FGR1 keeping its untouched preload, so hardware writes an odd destination where we fold it into the even pair. Sources and destinations need separate rules. Naming that explicitly is the point: the next session should open the manual on the FR description rather than do more arithmetic on the expected values, which is where three rounds have already been spent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A floating-point arithmetic instruction ignores the low bit of fs and does NOT
ignore the low bit of ft. The destination fd is used as-is in both modes.
The manual declines to specify this -- an odd register with FR = 0 is
"undefined" (UM 7.5.3, 16) -- so the oracle is n64-systemtest's measured table
and the ledger records it as C-21, a measurement rather than documentation.
Two rows settle it, and no single mapping satisfies both:
SQRT.S $13, $31 yields sqrt(16), so fs = 31 read FGR30.
ADD.S $2, $28, $31 yields -10 + -16, so ft = 31 read FGR31.
The suite then states it outright in its own assertion messages ("Lowest bit of
fs should be ignored", "Lowest bit of ft should not be ignored"), which is worth
recording because it means this was measurable rather than inferred.
This does NOT revise C-14. That entry governs MTC1/LWC1 and the doubleword
coprocessor moves, which really do reach an odd register's high half; it simply
does not extend to the arithmetic operand ports. Two mappings for two instruction
classes is surprising, so read_s_fs/read_s_ft exist as separate accessors and a
call site cannot silently pick the wrong one.
Getting the destination wrong is separately observable: folding an odd
destination into its even partner leaves the odd FGR holding its preload, which
the suite checks after ADD.D $1.
n64-systemtest, Phase 1's categories: 23 -> 19. Four of the five half-mode tests
pass; the fifth is a pipeline hazard, not an addressing bug, and is recorded as
residual R-2 rather than left to be rediscovered.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… fault Three unrelated rules, each with a reason it stayed wrong. PRId now reads 0x0B22. The Rev field was recorded as undocumented and left zero, which was a true statement about the User's Manual and a false one about the N64brew wiki this project mirrors -- it names 0x10, 0x22 and 0x40 for early, later and iQue parts. Ledger U-3 is superseded by C-22. This is the third time a decayed "undocumented" claim has been cited here as if it described the hardware, which is exactly why engineering-lessons 3.3b exists: nothing fails when such a claim goes stale, so it survives review. Random is a plain 6-bit down-counter whose reload fires on == Wired, not <= Wired, and whose decrement wraps 0 -> 63. The two readings agree for the ordinary Wired <= 31 case and diverge only above it, which software can reach because the field is six bits: the old reading pinned the register at 31 forever. Ledger C-23. Worth noting the suite samples a RANGE to catch this -- a single sample cannot tell a pinned counter from a slow one. BadVAddr reports the address the instruction named, not the container address translated on its behalf. A fault on SWL 0x12345001 reported 0x12345000. n64-systemtest, Phase 1's categories: 19 -> 16. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CVT.S.W, CVT.S.L, CVT.D.W and CVT.D.L were each a Rust `as` cast plus a round-trip inexact check. `as` rounds to nearest-even unconditionally, so the mode was ignored -- while the round-trip check still reported `inexact` correctly. That combination is worth naming: the flags were right and the value was wrong, so flag agreement gave no signal at all. n64-systemtest converts 1234567891 toward zero and expects 0x4E93_2C05 where nearest-even gives 0x4E93_2C06; CVT.D.L of 0x007F_FFFF_FFFF_FFFE toward zero expects 0x435F_FFFF_FFFF_FFFF, not 0x4360_0000_0000_0000. All four now go through softfloat::from_int, which is one line on top of the shared rounding point: an integer is sign x |v| x 2^0, so it is round_pack with a zero exponent and no sticky bit. Routing it through the same place as every other operation is what makes the mode impossible to forget. The four old converters are DELETED rather than left unused. An unused function that quietly gets an operation wrong is the inert-API hazard engineering-lessons 3.2 describes, and addr.rs deleted a stale `translate` for exactly this reason. long_convertible stays -- the VR4300 range restriction is a separate rule and is still consulted. Ledger C-24. n64-systemtest, Phase 1's categories: 16 -> 11. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CVT.W.W, CVT.W.L, CVT.L.W, CVT.L.L and the whole ROUND/TRUNC/CEIL/FLOOR family from .W/.L are not instructions. They were reaching the Cop1Unimplemented fallthrough, which deliberately does not raise, so they retired silently -- n64-systemtest saw no exception where it expects Unimplemented Operation. They now decode into the arithmetic path specifically so they reach the refusal that already handles the subnormal and 2^53 cases. Widening the decode arm rather than making the generic fallthrough raise keeps the trap machinery in one place, and keeps the fallthrough's own contract intact -- there is a test asserting an unimplemented COP1 encoding with CU1 set must NOT raise. The refusal is checked BEFORE the source is read as a float. An integer source format has no float to widen, and every branch below would happily reinterpret the register and return a plausible number for an instruction that does not exist -- which is precisely the shape of the bug being fixed. n64-systemtest, Phase 1's categories: 11 -> 7. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PageMask bits 24:13 are six 2-bit pairs, and an entry does not store twelve independent bits: a pair reads back as 11 exactly when its HIGHER bit was written, and 00 otherwise. 0b10 becomes 0b11; 0b01 is discarded. The natural implementation -- keep the value, mask it to 24:13 -- is wrong in both directions and quietly: it accepts page sizes the hardware has no encoding for, and it reports back a mask that was never stored. n64-systemtest writes seventeen values and checks each read-back, including 0b00000000100 -> 0, where a masking implementation returns its input unchanged. Canonicalisation happens on write, because that is where the information is lost -- the entry has nowhere to keep the discarded bits. Mutation-checked: substituting `mask & 0x01FF_E000` turns the new test red. The test's literals are grouped in PAIRS rather than nibbles, since the pair is the unit the hardware stores and a mask that looks wrong that way is wrong. n64-systemtest, Phase 1's categories: 7 -> 6. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
EntryHi's tag keeps every bit PageMask does not cover. It was stored as VA / pair_size, which clears the tag's LOW bits -- correct for all six legal page sizes, since those are contiguous runs from bit 13, and wrong as soon as a canonicalised mask has a hole. 0b11_11_11_11_00 covers bits 22:15 and leaves 14:13 alone, and no divisor expresses that. The tag is now held in place and masked, so vpn2_of and the read-back agree by construction rather than by two matching derivations. For every contiguous mask the two are identical, which is why this survived until a test wrote a holed one. Mutation-checked: restoring the divide turns the new test red on exactly the holed-mask row and leaves the five contiguous rows green -- which is the useful shape, since it shows the old code was not merely untested but untestable by any legal page size. n64-systemtest, Phase 1's categories: 6 -> 5. Every TLB item now passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Underflow was decided from the packed result -- subnormal or zero raised it, a normal result did not. IEEE 754 permits tininess to be judged either before or after rounding, and the VR4300 judges it BEFORE. The two conventions differ exactly when a directed rounding mode lifts a tiny result back into the normal range. FLT_MIN / (1 + 1ulp) under round-toward-+inf yields FLT_MIN, a perfectly normal number, and hardware still raises underflow because the value it rounded FROM was tiny. n64-systemtest carries that case in both signs, and note what made it hard to spot: the result VALUE was already correct, so only the flag disagreed. The flag is now set once from the pre-rounding shift decision rather than re-derived on each exit path. The mutation check takes down three tests instead of one, which is the point -- the previous shape had two independent assignments that could drift apart. n64-systemtest, Phase 1's categories: 5 -> 3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LWL/LWR/SWL/SWR and their doubleword siblings address individual bytes, so RE moves them by addr ^ 7 rather than by their container's width. One XOR relocates the container and complements the byte index together: LWL 0 becomes container 4 with byte index 3, since 0 ^ 7 == 7, 7 & !3 == 4 and 7 & 3 == 3. This was left deliberately open when RE landed, because the width-based rule plainly did not apply and any replacement would have been fitted rather than derived. It is derived now, from the ROM's own expected tables: SWL at offset 0 writes a single byte -- rt's most significant -- into the doubleword's LAST byte, which no width-based swap produces, and reading the table settled in minutes what computing against it had not. BadVAddr still reports the untransformed address, since the swap is applied to a separate effective address and the fault path already overrides it. n64-systemtest, Phase 1's categories: 3 -> 1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nted The previous entry diagnosed this as a pipeline hazard: FCSR.C commits in WB while the branch resolves in EX, so the branch reads a stale condition. That reading was derived from the pipeline's structure without first checking that the branch existed. It does not. A per-cycle trace shows BC1T decoding to Cop1Unimplemented and retiring as a no-op, so the flag the test reads is never overwritten by anything. pipeline.rs already said so in as many words -- "a real, valid COP1 branch that is genuinely not wired yet", with a note to move that test when BC1 lands -- and I wrote a hazard diagnosis without reading it. Worth recording as more than a correction: R-2's own status line said to trace rather than reason, and the reasoning that produced the wrong entry happened before the trace. The rule only helps if it runs first. The entry now names the concrete work (four Op variants, decode from COP1 rs = 0o10 with bits 17:16 as nd:tf, is_likely for the *L pair, and the condition plumbed into execute, which today cannot see FCSR) and keeps the hazard as a SECOND question -- the same trace does show FCSR.C committing two cycles after an adjacent branch would reach EX, so an interlock is probably still needed once the branch exists. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
They were not implemented at all. The branch decoded to Cop1Unimplemented and retired as a no-op, so a program branching on a compare simply fell through -- and the test that pinned this said so in as many words, with an instruction to move it when BC1 landed. Moved rather than deleted, onto COP1 rs = 0o11, which is genuinely unassigned. COP1 rs = 0o10, bits 17:16 as nd:tf, so the four encodings are true/false crossed with likely/not. Target arithmetic and branch-likely nullification are shared with every other branch rather than duplicated. FCSR.C is passed into `execute` as a parameter rather than reached for: that function is pure and has no view of coprocessor state, and a parameter makes every call site a compile error until it supplies one -- which is what keeps a defaulted or stale condition from being wired in silently. STILL OUTSTANDING, and now the only Phase 1 failure: BC1 reads the condition in EX while C.cond.fmt writes it in WB, so an adjacent pair samples the previous condition. Hardware interlocks; we do not yet. A first interlock attempt is recorded in ledger R-2 together with why it is insufficient -- it fires once and is then satisfied while the commit still has not happened. The remaining fix must come from tracing where the compare actually is on each cycle, not from choosing a stall count that makes the ROM pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ed out A per-cycle trace pins it: with C.EQ.S at cycle 1, BC1T executes EX at cycle 3 while the compare's WB runs at cycle 4. The branch is one cycle short. The obvious fix does not work, and that is the useful part of this entry. stall_for freezes the WHOLE pipeline, so holding the branch delays the compare's WB by exactly the same amount and the branch never catches up -- an interlock on ex_dc/dc_wb was implemented, fired once, and changed nothing. That interlock has been removed rather than left in: a guard with the shape of a fix but no effect is worse than none, because it makes the hazard look handled. A bypass is also unavailable as things stand: fp_arith COMPUTES the condition in WB, not EX, so there is no pending value to forward. Two viable routes are recorded instead of a guess: a draining bubble that holds IC/RF/EX while DC/WB continue, or moving the condition computation to EX and leaving only the commit in WB so it can be bypassed like a GPR. The second touches ADR 0007's single commit point and would need writing up, so the choice is deliberate rather than incidental. Not doing: picking a stall count that makes the ROM pass. That is the fitted timing constant this ledger exists to prevent -- every later timing result built on it would stop being evidence. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This clears Phase 1's cut criterion: n64-systemtest's CPU/COP0/TLB/COP1 categories are at Failed: 0. BC1 resolves in EX while C.cond.fmt commits FCSR.C in WB, so an adjacent pair sampled the previous condition -- and the ROM emits exactly that pair with no separating instruction. Fixed by a forwarding path rather than a stall, and the distinction was measured rather than assumed. stall_for freezes every stage, so holding the branch delays the compare's WB by the same number of cycles and the gap never closes; an interlock on ex_dc/dc_wb was written and traced, fires once, is satisfied while the commit still has not happened, and changes nothing. The load interlock is not a counter-example: it works because its consumer reads through the bypass network, so the one-cycle stall buys DC time and forwarding does the rest. The FP condition had no forwarding path at all, which is what this adds. Re-evaluating the pending compare is sound because it reads two FP registers and writes only FCSR.C -- nothing between it and the branch can change those registers, since a branch has no destination. Flags are discarded: a forwarding path must not raise the compare's trap on the branch's behalf. ex_dc is consulted before dc_wb because it holds the younger instruction. Mutation-checked in both directions. Removing the bypass reddens the test, and so does a bypass that always reports "true" -- the second matters because a test of the taken case alone would pass against it. Ledger C-25; R-2 closed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The state section still said "Phase 1 in progress" with 40 failing assertions and BC1 decoding to a no-op. It now records the criterion as MET (Failed: 0 in CPU/COP0/TLB/COP1), that the ~413 remaining are RSP/RCP and belong to Phase 2, and that v0.2.0 is unblocked but deliberately NOT tagged -- tagging is the VERSION-PLAN phase-close ceremony and belongs on main. Also records a gap worth a ticket rather than a fourth rewrite: there is no committed n64-systemtest runner, so every session so far has rebuilt a throwaway one for the project's primary oracle. Six conventions added, each from a bug this session shipped or nearly shipped: - Agreeing flags are not evidence the value is right, and vice versa -- three bugs had exactly one half correct, and in two of them the correct half is what made the wrong half look fine. - An address in 32-bit mode must be the sign extension of its low word. - The segment map is a function of (address, mode, width), and an out-of-range address raises AdEL BEFORE the TLB is consulted. - Read the oracle's table; do not compute against it. - A stall cannot substitute for a bypass when the stall freezes both sides. - Delete a superseded function rather than leaving it unused. The "undocumented decays" entry gains its third instance (PRId.Rev, documented in the wiki this repo mirrors) and a sharper rule: name the source, because "the UM does not say" is checkable and "undocumented" is not. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two functional, four inaccurate comments, two documentation. LWU joins the 64-bit operations (CodeRabbit, #33). It was missing because the set was built from n64-systemtest's 28 tested instructions, and the ROM does not cover it. The manual states the rule once, as the EPSILON marker in the opcode table: "valid in the 64-bit mode and 32-bit Kernel mode. In the 32-bit User or Supervisor mode, this code generates the reserved instruction exception." Reading that legend rather than the per-instruction notes is what found the gap, and the doc comment now cites it so the next reader checks the same place. The oracle is unchanged at Failed: 0, so this adds a rule without disturbing a measured one. Index_Store_Tag now requires the cache's OWN valid PState -- 2 for the I-cache, 3 for the D-cache -- rather than any non-zero field (Copilot, #32). The old form would conjure a live line from an undefined encoding, and would let one cache's Valid validate the other's. Pinned by a test that walks the rejected values. The four comments were each describing code that had moved on: - The FpCommit::Single comment still said "preserves the upper half" while the code calls write_s_arith, which clears it. This is the fourth time in this project a comment has outlived its code; the rule it states was right and the attribution was not. - The Index_* cache comment claimed segment translation leaves the index bits alone. False for every mapped segment -- translation preserves only the low 12 bits, while the D-cache index reaches bit 12 and the I-cache bit 13. The real situation is the D-6 deviation, and it now says so. - translate_re's Errors section named only TLB faults; it can also raise AddressError. - The KSU == 3 fallback was described as "most restrictive-to-reach" when Kernel is the most permissive. Documentation: D-6 no longer claims physical indexing is strictly more coherent than the hardware -- it is a divergence in both directions, and the entry now names the two observable cases and bounds the tested scope to KSEG0. The STATUS.md gate row no longer reads "Failed: 0" as a label while reporting a non-zero count in the same row. The MOV.S test asserted only the low word, so a formatted half-copy would have passed its whole-register claim (CodeRabbit, #31). It now asserts read_raw. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
doublegate
force-pushed
the
feat/addressing-and-privilege
branch
from
July 21, 2026 04:57
d33fbef to
419962e
Compare
doublegate
changed the title
CodeRabbit's finding named two sites; I fixed one. `docs/accuracy-ledger.md` D-6 and the `cache.rs` module doc were rewritten to say physical indexing is a divergence in both directions, and `docs/cpu.md` was left still framing it as an improvement -- "makes cache aliases impossible rather than merely unlikely", which reads as strictly better and discourages the next reader from looking. It now names the same two observable cases as D-6 (a deliberately constructed alias, and an Index_* operation on a TLB-mapped page, where translation preserves only the low 12 bits while the D-cache index reaches bit 12 and the I-cache bit 13) and states the tested scope as KSEG0. Worth recording as a pattern rather than a typo: a claim asserted in three places gets corrected in the two the author is looking at. The bounded version now reads the same in all three. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Found by auditing for the pattern CodeRabbit's last finding exposed: a claim asserted in several places gets corrected in the ones the author happens to be looking at. `Op::Cache`'s doc comment was the fourth site and still read "executed as an address-translating no-op: this CPU does not model cache contents" -- true under ledger D-5, false since T-11-003 landed the caches in this same stack. It now states what the instruction does, records that the old wording was accurate under D-5, and points at D-6 which supersedes it. Keeping the retraction visible rather than silently swapping the text, because the next reader is more likely to trust a doc that shows its own correction. The audit also confirmed the other three sites (accuracy-ledger D-5/D-6, docs/cpu.md, cache.rs module docs) already agree. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
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/cache.rs (1)
236-242: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUncited invented side effect:
store_tagclearsdirty.The module's own docs (lines 41-43) state the write-back bit has no
TagLofield and is invisible toIndex_Load_Tag/Index_Store_Tag. Clearingdirtyhere on a valid, dirty line silently discards a pending write-back with no citation, no ledger entry, and no test — this is exactly the invented-side-effect pattern the path instructions single out as worse than an invented constant, because it never lands where it can be argued with.Icache::store_tag(lines 382-387) makes no equivalent assumption, so this isn't a shared convention either.Either cite a source for this behaviour (or record it in
docs/accuracy-ledger.mdwith a covering test), or drop the assignment and let the existing dirty state survive the tag overwrite.🐛 Proposed fix
pub const fn store_tag(&mut self, addr: u32, tag_lo: u32) { let i = Self::index(addr); let (tag, valid) = unpack_tag(tag_lo, DCACHE_VALID_STATE); self.lines[i].tag = tag; self.lines[i].valid = valid; - self.lines[i].dirty = false; }🤖 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/cache.rs` around lines 236 - 242, Remove the self.lines[i].dirty = false assignment from Dcache::store_tag so Index_Store_Tag preserves the existing dirty state, matching Icache::store_tag and the documented TagLo behavior. Do not add new side effects or alter the tag and valid updates.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 `@docs/accuracy-ledger.md`:
- Around line 1010-1028: Update the earlier U-3 ledger row to mark it resolved
or superseded by C-22, preserving its original historical wording and adding a
reference to C-22. Do not alter the new C-22 entry or its technical conclusions.
In `@docs/cpu.md`:
- Around line 509-533: Attach authoritative provenance to each newly documented
hardware rule: in docs/cpu.md lines 509-533, cite a manual, wiki, or
accuracy-ledger measurement for privilege-aware segment mapping, XKPHYS
behavior, and sign extension; in AGENTS.md lines 306-315, cite the source for
sign-extension and segment-map conventions; in docs/cpu.md lines 643-674, cite
the source for integer-source conversion refusal and 32-bit-mode doubleword
reservation; and in docs/cpu.md lines 692-696, cite the measured or documented
basis for the byte-granular Status.RE rule.
In `@docs/STATUS.md`:
- Line 181: Reconcile the execution-status claims across docs/STATUS.md lines
181-181, AGENTS.md lines 62-76 and 89-91, and CHANGELOG.md lines 12-18: do not
publish “Failed: 0” or mark the criterion cleared without reproducible oracle
output. Remove or correct the asserted result, update the stale COP1-failure
text consistently, and replace the “no committed runner” statement only if a
canonical runner actually exists.
---
Outside diff comments:
In `@crates/rustyn64-cpu/src/cache.rs`:
- Around line 236-242: Remove the self.lines[i].dirty = false assignment from
Dcache::store_tag so Index_Store_Tag preserves the existing dirty state,
matching Icache::store_tag and the documented TagLo behavior. Do not add new
side effects or alter the tag and valid updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c2f2e097-20e5-40be-a980-c8be6421e21d
📒 Files selected for processing (18)
AGENTS.mdCHANGELOG.mdcrates/rustyn64-cpu/src/addr.rscrates/rustyn64-cpu/src/cache.rscrates/rustyn64-cpu/src/cop0.rscrates/rustyn64-cpu/src/decode.rscrates/rustyn64-cpu/src/exec.rscrates/rustyn64-cpu/src/fpr.rscrates/rustyn64-cpu/src/fpu.rscrates/rustyn64-cpu/src/lib.rscrates/rustyn64-cpu/src/pipeline.rscrates/rustyn64-cpu/src/softfloat.rscrates/rustyn64-cpu/src/tlb.rscrates/rustyn64-test-harness/src/rom.rscrates/rustyn64-test-harness/tests/first_rom.rsdocs/STATUS.mddocs/accuracy-ledger.mddocs/cpu.md
…ducible CodeRabbit flagged docs/STATUS.md for publishing `Failed: 0` while AGENTS.md said in the same breath that no runner existed. It was wrong that the oracle had not run -- it has, repeatedly -- and right about what matters: the result was not reproducible from the repository, so nobody could check it. A measured claim nobody can re-run is a self-assessment wearing a measurement's clothes, and the correct answer is to make it checkable rather than to soften the wording. The runner is now a committed `#[ignore]`d harness test (~2 min in --release, far too slow for the default path). The ROM is already committed under MIT, so it needs no external corpus. It asserts Phase 1's criterion directly -- the CPU/COP0/TLB/COP1 categories, matching on what to EXCLUDE so a new CPU-side category added upstream lands inside the gate rather than being silently ignored. It witnesses execution before trusting a zero. Writing that guard immediately caught my own first version, which asserted on a count of FAILURE lines: 413 failures read as "the suite did not complete" when the suite had in fact run all 917 tests. Counting failures cannot witness a run, because zero of them is exactly what a run that never started also produces -- the vacuous-pass hazard, which bit the guard meant to prevent it. Also from the same review: U-3 is closed against C-22 with its original wording preserved, and provenance is attached to the hardware rules documented this phase -- the epsilon legend for the 64-bit reservation, the specific n64-systemtest cases for sign extension, the integer-source conversion refusal, and the byte-granular Status.RE rule. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 29, 2026
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
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.
This PR closes Phase 1's cut criterion: n64-systemtest's CPU/COP0/TLB/COP1 categories are at
Failed: 0.Third and last of the stack (#31 → #32 → #33), now rebased onto
main. 19 commits; the whole session took those categories from 40 → 0.Addressing and privilege
Status.RE— reverse endian in User mode. A permutation of byte lanes within the doubleword, expressed as an XOR of the low address bits. Instruction fetch is swapped too. The swap is applied to the physical address, which touches only bits 2:0 and so keepsBadVAddrraw on a fault.(address, mode, width), never the address alone.KSEG0does not exist in User mode, and it is the address-space check — not the TLB — that stops a user program reaching it. An out-of-range address raisesAdELbefore the TLB is consulted, carried by a separateTranslateErrorvariant: folding it into a refill would send the program to the refill handler, where a well-behaved kernel maps the page and grants the access it was never allowed to make.0x0000_0000_8000_1000is not shorthand forKSEG0; it is an address error. The old code truncated tou32and accepted it — and the reset vector, the ROM entry point, and every kernel address in the test suite were stored that way, all passing on addresses the hardware rejects.LWU, which n64-systemtest does not exercise.XKPHYS— eight 2^32 direct windows; onlyC == 2is uncached.COP1
FR = 0mapsfsandftby different rules. The manual declines to define odd registers here, so the ROM's table is the oracle. Two rows admit no single mapping:SQRT.S $13, $31reads FGR30,ADD.S $2, $28, $31reads FGR31. Ledger C-21.FCSR.RM. The old converters wereascasts plus a round-tripinexactcheck — so the flag was right and the value ignored the mode. All four were deleted rather than left unused. Ledger C-24.CVT.W.Wand friends are not instructions) with Unimplemented Operation, rather than reinterpreting the register and returning a plausible number.FLT_MIN / (1+1ulp)toward +∞ rounds back to a normal number and still underflows — here the value was right and the flag missing.BC1F/BC1T/BC1FL/BC1TL— they were not implemented at all; the branch retired as a no-op.C.cond.fmtis forwarded toBC1. A stall cannot do this:stall_forfreezes every stage, so holding the branch delays the compare'sWBequally. The load interlock is not a counter-example — it works because its consumer reads through the bypass network. Ledger C-25.TLB
PageMaskpair stores only its higher bit.PageMask, not divided by the page size — division only clears low bits, which is right for all six legal page sizes and wrong once a canonicalised mask has a hole.Randomis a plain 6-bit down-counter reloading on== Wired;PRId.Revis0x22;BadVAddrreports the address the instruction named.Verification
Every commit was green on
cargo fmt --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace,RUSTDOCFLAGS="-D warnings" cargo doc, thethumbv7em-none-eabihfno_std build, andpre-commit run markdownlint. Re-verified after the rebase ontomain, and the rebased tree is byte-identical to the branch those gates ran on.Guards are mutation-checked where the check is meaningful — the
PageMaskcanonicalisation, the TLB masking, the underflow flag, the 64-bit reservation, and theBC1bypass each turn a specific test red when reverted.Not included, deliberately
DMFC0/DMTC0, which the epsilon table also marks: they are additionally governed by coprocessor usability, and in User mode COP0 is unusable so hardware reportsCpU. The ROM exercises neither, so the omission is recorded as a decision rather than resolved on no evidence.v0.2.0is unblocked by this but not tagged — that is the VERSION-PLAN phase-close ceremony and belongs onmainafter this lands.🤖 Generated with Claude Code