[BUG] cva6_mmu: Sv39 canonical-address check samples the unregistered request, so a non-canonical access page-faults the preceding in-flight access

[ido-accelerato]

Code of Conduct

• I have searched the existing bug issues.
• I am a human engaging in an interpersonal interaction. During this interaction, my words are my own and are not generated. If relevant, I provide links to my sources.

Bug Description

In core/cva6_mmu/cva6_mmu.sv, the data interface is a two-stage pipeline: the cycle-0 request is registered, and the DTLB-hit response is produced in cycle 1 from the _q copies:

cva6_mmu.sv (current master, a3dc2c5e):
  524:  lsu_vaddr_n = lsu_vaddr_i;
  525:  lsu_req_n   = lsu_req_i;
  ...
  592:  if (dtlb_hit_q && lsu_req_q) begin            // response for the previous cycle's access
  ...
  647:        lsu_exception_o.tval = {
  648:          {CVA6Cfg.XLEN - CVA6Cfg.VLEN{lsu_vaddr_q[CVA6Cfg.VLEN-1]}}, lsu_vaddr_q
  649:        };

Every other term evaluated in that response block follows this discipline: daccess_err reads dtlb_pte_q.u (lines 544-546), the store permission checks read dtlb_pte_q.w / dtlb_pte_q.d (line 615). The Sv39 canonical-address check alone is built from the unregistered inputs:

  539:  canonical_addr_check = (lsu_req_i && en_ld_st_translation_i &&
  540:         !((&lsu_vaddr_i[CVA6Cfg.VLEN-1:CVA6Cfg.SV-1]) == 1'b1 || (|lsu_vaddr_i[CVA6Cfg.VLEN-1:CVA6Cfg.SV-1]) == 1'b0));

yet it is consumed inside the registered response - in the STORE_PAGE_FAULT term at line 615 and the LOAD_PAGE_FAULT term at line 643. The condition and the reported tval therefore describe two different accesses: the canonicality of the access currently on the input bus (access N+1) decides whether the previous access (access N) faults, while tval reports access N's address.

This is reachable in the core: lsu_vaddr_i is fed combinationally from the current LSU request - load_unit.sv:199 (assign vaddr_o = lsu_ctrl_i.vaddr) via load_store_unit.sv:652/663 - so it advances to the next access as soon as one is issued. The load unit asserts translation_req_o for a new request while it is in SEND_TAG (load_unit.sv:357), i.e. exactly the cycle in which the MMU's registered response for the previous access is produced, and it consumes the MMU exception in that same state (load_unit.sv:476). So any DTLB-hitting access immediately followed by a translating access with a non-canonical VA corrupts the first access's response: the first (architecturally legal) access takes a spurious LOAD_PAGE_FAULT / STORE_PAGE_FAULT whose tval points at a canonical, correctly-mapped address - one the OS page-fault handler can make no sense of.

Note the second access does not even need a PTW walk to keep this timing: a non-canonical VA whose bits [38:12] match a resident entry still hits the DTLB, because the tag only covers VA[38:12] (cva6_tlb.sv:148). Bits above SV-1 never reach the tag compare.

Steps to reproduce

Reproduction commit a3dc2c5 (current master). Target cv64a6_imafdc_sv39, Verilator 5.008 test harness in tandem with Spike (DV_SIMULATORS=veri-testharness,spike), GCC 13.1.0.

The attached reproducer maps a 1 GiB identity leaf over DRAM (V|R|W|X|A|D), runs data accesses as S-mode via MPRV/MPP=S from M-mode, warms the DTLB, then executes the back-to-back pair:

  la a0, tdat                   # canonical, mapped, DTLB-resident
  li t0, 1
  slli t0, t0, 40
  or a3, a0, t0                 # same VA[38:12], bit 40 set -> non-canonical
  ...
  ld t1, 0(a0)                  # access N   : legal, must NOT fault
  ld t2, 0(a3)                  # access N+1 : non-canonical, must fault

The M-mode trap handler accepts exactly one LOAD_PAGE_FAULT with mtval == a3; a load page fault whose mtval is any other address makes the test exit with code 13. The STORES_ONLY variant does the same with sd (spurious-fault exit code 15), the MIRROR_ONLY variant reverses the pair ordering (see caveats).

To reproduce, apply the attached reproducer patch and run:

git apply cva6_canonical_stage_reproducer.patch
cd verif/sim
python3 cva6.py --testlist=../tests/testlist_canonical_stage.yaml --test rv64si-p-canonical-stage        --iss_yaml cva6.yaml --target cv64a6_imafdc_sv39 --iss=veri-testharness,spike
python3 cva6.py --testlist=../tests/testlist_canonical_stage.yaml --test rv64si-p-canonical-stage-st     --iss_yaml cva6.yaml --target cv64a6_imafdc_sv39 --iss=veri-testharness,spike
python3 cva6.py --testlist=../tests/testlist_canonical_stage.yaml --test rv64si-p-canonical-stage-mirror --iss_yaml cva6.yaml --target cv64a6_imafdc_sv39 --iss=veri-testharness,spike

Pre-fix (current master): on the RTL the first, legal load takes the page fault, and mtval is the canonical, correctly-mapped address (rvfi trace, spike-dasm'd):

LOAD_PAGE_FAULT exception @ 0x000000008000017c (0x00053303)        <- ld t1, 0(a0)
core   0: 0x0000000080000214 (0x34202f73) csrrs   t5, mcause, zero
3 0x0000000080000214 (0x34202f73) x30 0x000000000000000d           <- mcause = LOAD_PAGE_FAULT
core   0: 0x0000000080000218 (0x34302ff3) csrrs   t6, mtval, zero
3 0x0000000080000218 (0x34302ff3) x31 0x0000000080003000           <- mtval = the CANONICAL address

canonical_stage_repro.o *** FAILED *** (tohost = 13) after 738 cycles

The store variant fails the same way on the canonical sd:

STORE_PAGE_FAULT exception @ 0x0000000080000190 (0x01c53023)       <- sd t3, 0(a0)
...
canonical_stage_repro.o *** FAILED *** (tohost = 15) after 762 cycles

Spike passes the same binaries - only the non-canonical access faults, with tval = the non-canonical address:

core   0: 0x000000008000017c (0x00053303) ld      t1, 0(a0)
core   0: 3 0x000000008000017c (0x00053303) x6  0x5aa55aa5c33cc33c mem 0x0000000080003000
core   0: 0x0000000080000180 (0x0006b383) ld      t2, 0(a3)
core   0: exception trap_load_page_fault, epc 0x0000000080000180
core   0:           tval 0x0000010080003000

and the tandem trace comparison flags the divergence.

Then apply the attached fix patch and re-run the three commands above, plus the existing upstream non-canonical test:

git apply cva6_canonical_stage_fix.patch
python3 cva6.py --testlist=../tests/testlist_riscv-tests-cv64a6_imafdc_sv39-p.yaml --test rv64si-p-noncanonical --iss_yaml cva6.yaml --target cv64a6_imafdc_sv39 --iss=veri-testharness,spike

Post-fix: all three reproducer variants pass with a full tandem trace match against Spike (80 / 56 / 52 lines), and rv64si-p-noncanonical (issue #371) still passes (75 lines matched) - the legitimate non-canonical faults, including the fetch-side check, are unaffected.

Suggested fix direction

Compute the check from the registered request, like every other term consumed in the response block (this is the attached cva6_canonical_stage_fix.patch):

-    canonical_addr_check = (lsu_req_i && en_ld_st_translation_i &&
-           !((&lsu_vaddr_i[CVA6Cfg.VLEN-1:CVA6Cfg.SV-1]) == 1'b1 || (|lsu_vaddr_i[CVA6Cfg.VLEN-1:CVA6Cfg.SV-1]) == 1'b0));
+    canonical_addr_check = (lsu_req_q && en_ld_st_translation_i &&
+           !((&lsu_vaddr_q[CVA6Cfg.VLEN-1:CVA6Cfg.SV-1]) == 1'b1 || (|lsu_vaddr_q[CVA6Cfg.VLEN-1:CVA6Cfg.SV-1]) == 1'b0));

(en_ld_st_translation_i is left unregistered for consistency with daccess_err, which combines it with _q state the same way.)

Reproducer target

cv64a6_imafdc_sv39 (full core, Verilator test harness in tandem with Spike)

Expected behavior

Expected result with the pre-fix RTL:

rv64si-p-canonical-stage     FAILED (tohost = 13)  — spurious LOAD_PAGE_FAULT on the canonical ld
rv64si-p-canonical-stage-st  FAILED (tohost = 15)  — spurious STORE_PAGE_FAULT on the canonical sd
rv64si-p-canonical-stage-mirror  PASSES            — see caveats

Expected result with the fix:

All three variants PASS with full tandem trace match against Spike;
rv64si-p-noncanonical still PASSES (75 lines matched)

Observed behavior

Caveats:

• Active only for Sv39 (rv64 configs). For Sv32, VLEN = SV = 32, the range [CVA6Cfg.VLEN-1:CVA6Cfg.SV-1] collapses to the single bit [31], and (&x)==1 || (|x)==0 on one bit is a tautology, so canonical_addr_check is constant 0 there.
• The mirror direction - a non-canonical access escaping its own fault because the following access is canonical - is what the same expression permits on paper, but it does not occur: the load unit holds the faulting request (and thus the non-canonical address on lsu_vaddr_i) through its own response cycle, so the check still fires. The MIRROR_ONLY variant of the reproducer covers exactly this ordering (non-canonical load immediately chased by a canonical load) and passes on unmodified master with a full trace match against Spike (52 lines). Only the confirmed direction is reported.
• The Hypervisor/G-stage exception terms were not separately exercised; the reproducer runs with RVH present but virtualization disabled, and the affected canonical_addr_check term only appears in the non-virtualized STORE_PAGE_FAULT / LOAD_PAGE_FAULT branches.
• Provenance: canonical_addr_check was introduced by PR bug fix: canonical check on virtual address for data accesses #2667 ("bug fix: canonical check on virtual address for data accesses", merged 2024-12-16) and has not been modified since; the defect is present in releases v5.2.0 and v5.3.0 (harmless for Sv32-only configurations per the first caveat). The check itself is correct; this issue is only about the pipeline stage it samples.

cva6_canonical_stage_fix.patch
cva6_canonical_stage_reproducer.patch

[Maanvi212006]

Thanks for the detailed root-cause and reproducer @ido-accelerato — this is a clear one-cycle staging mismatch (canonical_addr_check reads lsu_vaddr_i/lsu_req_i while every other term in the same response block reads the _q registered copies).

I'd like to pick this up. My approach would be to apply the fix as outlined — switch canonical_addr_check to sample lsu_vaddr_q/lsu_req_q instead of the unregistered inputs — then verify with the three reproducer variants (canonical-stage, canonical-stage-st, canonical-stage-mirror) plus the existing rv64si-p-noncanonical regression to confirm no regressions on the legitimate non-canonical fault path.

Could I be assigned to this issue?

[ido-accelerato]

Thanks @Maanvi212006 , glad it helps. Please go ahead. Happy to review the PR.

[Maanvi212006]

Hii @ido-accelerato I have linked the PR. Can you review it?
Thanks!

[Maanvi212006]

Hi @cainria is there a slack channel wherein I can connect with more people regarding my PRs? I would love to connect
Thanks!

[cainria]

Hi! We are on the OpenHW Mattermost https://mattermost.openhwgroup.org/all-users/channels/twg--cores--cva6