T-016: activate MMU with identity-mapped kernel + MapperFlush flush-token discipline

[cemililik]

Summary

Implements T-016 (B2 milestone) per ADR-0027: turns the MMU on with an identity-mapped layout, lights up TTBR0_EL1 with kernel + GIC + UART regions, configures MAIR_EL1 for device-nGnRnE / normal-cached, and lands the #[must_use] MapperFlush flush-token discipline at the Mmu trait surface.

After this lands, every load and instruction-fetch goes through the live translation regime, MMIO accesses inherit the device-nGnRnE attributes, and forgetting to flush after a mapping mutation becomes a unused_must_use lint failure (denied workspace-wide) instead of a reviewer-attention concern.

Three commits (bisectable)

1. feat(hal): MapperFlush flush-token + VMSAv8 descriptor encoders (8c3c166)
   • HAL trait: Mmu::map → Result<MapperFlush, MmuError>; Mmu::unmap → Result<(MapperFlush, PhysFrame), MmuError>
   • New tyrne_hal::mmu::vmsav8 module: pure const fn descriptor encoders + MAIR_EL1_VALUE / TCR_EL1_VALUE / SCTLR_EL1_MMU_ENABLE_MASK
   • tyrne-test-hal::FakeMmu updated to mint tokens; existing round-trip tests discharge them
   • Module restructure: hal/src/mmu.rs → hal/src/mmu/mod.rs (rename) + hal/src/mmu/vmsav8.rs (new)
2. feat(bsp-qemu-virt): activate MMU with identity layout + MapperFlush (b573d83)
   • bsp-qemu-virt/src/mmu.rs (new): QemuVirtMmu impl with L0 → L1 → L2 → L3 walker, activate, invalidate_tlb_*
   • bsp-qemu-virt/src/mmu_bootstrap.rs (new): unsafe fn mmu_bootstrap() — once-per-boot routine running ADR-0027 §Simulation Steps 1–3
   • bsp-qemu-virt/linker.ld: .boot_pt reservation (4 × 4 KiB inside .bss)
   • bsp-qemu-virt/src/main.rs: kernel_entry wired (vector install → boot_ns → mmu_bootstrap → tyrne: mmu activated → GIC + DAIF unmask → timer banner)
   • Audit log: UNSAFE-2026-0022 / 0023 / 0024 / 0025 + bootstrap-Amendments on 0023 / 0024
3. docs(roadmap): T-016 Done; B2 implementation-complete; post-MMU baseline (cdb2717)
   • current.md + phase-b.md updated; T-016 task status flips Draft → In Progress → Done
   • hal.md §Mmu tense hedge flipped to past tense; boot.md Stage 3 + sequence diagram refreshed
   • New perf-baseline-2026-05-08-post-t-016-mmu-activated.md (20-iter harness band)
   • Smoke-verification Amendments dated 2026-05-09 on UNSAFE-2026-0022 / 0023 / 0024

ADR-0027 §Simulation table (the load-bearing design checklist)

Step	Pre	Action	Post
0	SCTLR_EL1.M = 0, MMU off, caches off, all bootstrap frames zero	(entry)	unchanged
1	bootstrap frames zero	populate L0[0], L1[0], L1[1], L2_low[64..73] (9 device blocks), L2_high[0..64] (64 RAM blocks)	all 4 frames populated; PC still at PA
2	frames populated, MMU off	MSR MAIR_EL1 + TCR_EL1 + TTBR0_EL1 + TTBR1_EL1=xzr + ISB	system regs configured; MMU still off
3	regs configured	TLBI VMALLE1 + DSB ISH + IC IALLU + DSB ISH + ISB + RMW SCTLR_EL1.{M,I,C}=1 + ISB	MMU ON; PC continues at identity-mapped VA
4	MMU on	return to kernel_entry	print tyrne: mmu activated; demo runs

The critical correctness moment is Step 3's final ISB: it drains the pipeline so the next instruction-fetch goes through the freshly-installed regime. Any error in Step 1 (typoed descriptor, missing AF, wrong attribute index) produces a Translation Fault on that next fetch.

Smoke trace (post-T-016)

tyrne: hello from kernel_main
tyrne: mmu activated                ← NEW
tyrne: timer ready (62500000 Hz, resolution 16 ns)
tyrne: starting cooperative scheduler
tyrne: task B — waiting for IPC
tyrne: task A -- sending IPC
tyrne: task B — received IPC (label=0xaaaa); replying
tyrne: task A — received reply (label=0xbbbb); done
tyrne: all tasks complete
tyrne: boot-to-end elapsed = X ns

Every line other than the new tyrne: mmu activated insertion is byte-stable from the post-T-015 baseline.

Test plan

• cargo fmt --all -- --check — clean
• cargo host-clippy (-D warnings) — clean
• cargo kernel-clippy (-D warnings) — clean
• cargo host-test — 182/182 pass (was 159 pre-T-016; +23 = 12 vmsav8 encoder tests + 6 MapperFlush semantics tests + 5 existing FakeMmu round-trip tests updated for the token return type)
• cargo kernel-build — clean
• QEMU smoke — full demo through tyrne: all tasks complete; tyrne: mmu activated line printed in the expected position
• -d int,unimp,guest_errors — only the pre-existing PL011 "data written to disabled UART" warning (358 instances on main, 379 on T-016; the +21 delta is exactly one byte per character of the new UART line). No new fault classes.
• tools/perf-harness.sh --report=... (20 iter, debug, QEMU TCG) — p10 / p50 / p90 = 5.397 / 6.153 / 6.652 ms. Pre-T-016 baseline was 3.884 / 4.642 / 5.584 ms. The Δ ≈ +1.5 ms is QEMU TCG translation-cache overhead at MMU bring-up; on real hardware the activation cost is sub-100 µs per ADR-0027 §Consequences.
• B2 closure-trio (business + security + performance) — landed as a follow-on per the B1 closure precedent (NOT required for T-016 Done flip).

What's deferred

Everything in T-016 §Out of scope: high-half kernel migration (ADR-0033 placeholder), physical-frame allocator (PMM — Mmu::map's FrameProvider is wired but no kernel caller yet), block-split-on-collision, per-section kernel-image permissions (.text RX vs .rodata R vs .bss/.data RW — ADR-0034 placeholder), multi-core TLB shootdown, page-fault routing into the capability system, ASID assignment per task, MappingFlags::USER runtime exercise, Mmu::lookup.

Refs: ADR-0009, ADR-0027, T-016
Audit: UNSAFE-2026-0022 / 0023 / 0024 / 0025

Summary by CodeRabbit

• New Features

  • MMU now activates during kernel boot; boot ordering updated (vector install, timestamp baseline before MMU, interrupt controller init/unmask).
  • New MMU implementation and a mapping API that returns a flush-token to control TLB invalidation.

• Documentation

  • Boot, HAL/MMU, roadmap, audit, and performance baseline docs updated to reflect MMU activation and design decisions.

• Tests

  • Host-side MMU tests updated to exercise the flush-token discipline and alignment/flag rules.

[qodo-code-review]

ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one.

[sourcery-ai]

Sorry @cemililik, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[coderabbitai]

Warning

Rate limit exceeded

@cemililik has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 51 minutes and 39 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b65580c3-eaa7-4903-915e-c4aa6f083f97

📥 Commits

Reviewing files that changed from the base of the PR and between d5d88d4 and b66d743.

📒 Files selected for processing (1)

• bsp-qemu-virt/src/mmu_bootstrap.rs

📝 Walkthrough

Walkthrough

This PR implements MMU activation for QEMU virt: adds a MapperFlush flush-token discipline to the HAL, host-testable VMSAv8 descriptor encoders, a QemuVirtMmu page-table implementation, a three-stage mmu_bootstrap with linker-reserved bootstrap frames, kernel_entry wiring (timestamp pre-MMU), and updated tests/docs/audits.

Changes

MMU Activation (T-016)

Layer / File(s)	Summary
HAL Contracts & Token hal/src/lib.rs, hal/src/mmu/mod.rs	Expose mmu and add MapperFlush with constructor/accessor and flush/ignore; Mmu::map → Result<MapperFlush, MmuError>; Mmu::unmap → Result<(MapperFlush, PhysFrame), MmuError>.
VMSAv8 Descriptor Encoders hal/src/mmu/vmsav8.rs	Pure const fn encoders and constants (MAIR/TCR/SCTLR/AP/SH), flags_to_descriptor_bits, block_descriptor, page_descriptor, table_descriptor, and unit tests.
QemuVirtMmu Implementation bsp-qemu-virt/src/mmu.rs	QemuVirtAddressSpace wrapper and QemuVirtMmu; activate programs TTBR0_EL1 with ISB/DSB/TLBI barriers; map/unmap perform L0→L3 walks, install/clear L3 leaf descriptors, validate flags/alignment, and return MapperFlush (plus PhysFrame on unmap); TLB invalidate helpers added.
Bootstrap & Linker Reservation bsp-qemu-virt/linker.ld, bsp-qemu-virt/src/mmu_bootstrap.rs	Linker reserves four 4KiB bootstrap page-table frames in .bss (L0/L1/L2_low/L2_high). mmu_bootstrap() populates descriptor chain with identity layout (2MiB L2 blocks for MMIO and RAM/kernel), programs MAIR/TCR/TTBR0 (clears TTBR1), invalidates TLB/I-cache, and enables the MMU via SCTLR_EL1 RMW.
Kernel Entry Wiring bsp-qemu-virt/src/main.rs	Add mmu and mmu_bootstrap modules; install EL1 vector table, snapshot cpu.now_ns() into BOOT_NS before MMU activation, call mmu_bootstrap(), print activation trace, initialize GIC and unmask IRQs after MMU, and reorder timer-banner emission.
Test Implementation & Validation test-hal/src/mmu.rs	FakeMmu updated to return MapperFlush tokens; tests added/updated to assert token VA, flush() per-address invalidation, ignore() no-op, alignment and invalid-flag rejections, and bulk mapping behavior with .ignore().
Documentation & Audit docs/architecture/boot.md, docs/architecture/hal.md, docs/audits/unsafe-log.md, docs/analysis/*, docs/roadmap/*	Boot-flow and boot-time diagram updated, HAL Mmu docs updated to reflect MapperFlush discipline, four UNSAFE audit entries (UNSAFE-2026-0022..0025) appended, perf baseline added, and T-016/T-016 milestone status updated.

Sequence Diagram(s)

sequenceDiagram
  participant KE as kernel_entry
  participant TS as cpu.now_ns()
  participant MMU_B as mmu_bootstrap()
  participant SR as Sys Registers
  participant TLB as TLB/IC
  participant GIC as GIC v2
  participant CON as Console

  KE->>KE: Install VBAR_EL1
  KE->>TS: Capture boot timestamp
  TS-->>KE: BOOT_NS = now
  
  KE->>MMU_B: Activate MMU
  
  MMU_B->>SR: Program MAIR_EL1, TCR_EL1, TTBR0_EL1
  SR-->>MMU_B: Regs configured
  
  MMU_B->>TLB: TLBI VMALLE1, IC IALLU, barriers
  TLB-->>MMU_B: Invalidated
  
  MMU_B->>SR: Set SCTLR_EL1 MMU bit
  SR-->>MMU_B: MMU enabled
  
  MMU_B-->>KE: Return (MMU active)
  
  KE->>CON: Print "mmu activated"
  CON-->>KE: Message output
  
  KE->>GIC: Init GIC v2 (via MMU-mapped MMIO)
  GIC-->>KE: Initialized
  
  KE->>SR: DAIFClr I-bit
  SR-->>KE: IRQs unmasked

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• cemililik/Tyrne#22: Implementation of ADR-0027/T-016 design and related MMU activation work.
• cemililik/Tyrne#15: Kernel entry refactor touching the same BSP boot sequence.
• cemililik/Tyrne#8: Prior changes around BOOT_NS/timer snapshot point in BSP boot wiring.

Poem

🐰 Hop, tiny rabbit, through page-table rows,

Four frames in .boot_pt where the cold zero grows.
TTBR wakes, barriers chant their rhymes,
TLBs hush, and the kernel counts its times.
MMU blooms — the bunny dances in mapped clover.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly summarizes the main change: MMU activation with identity-mapped kernel and the new MapperFlush flush-token discipline, which are the core features of this T-016 implementation.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch t-016-mmu-activation

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gemini-code-assist]

Code Review

This pull request implements MMU activation for the bsp-qemu-virt target, introducing a boot-time bootstrap routine that establishes an identity-mapped layout for kernel RAM and device MMIO. It adds the MapperFlush discipline to the Mmu trait to enforce compile-time TLB invalidation checks and provides pure VMSAv8 descriptor encoders in the HAL. Documentation, audit logs, and performance baselines have been updated to reflect these changes. Feedback was provided regarding the error handling in the page-table walker, specifically that returning AlreadyMapped when encountering a block descriptor during an unmap operation is misleading and should be clarified.

[gemini-code-assist]

Returning MmuError::AlreadyMapped when encountering a block descriptor during a page-table walk is appropriate for the map path, but it is misleading for the unmap path. If unmap is called on a VA that is part of a block mapping, the VA is indeed mapped, but the error suggests a collision during insertion. Since v1 does not support block splitting or unmapping, a more descriptive error or a dedicated check for the unmap case would improve clarity.

[coderabbitai]

Actionable comments posted: 5

Caution

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

⚠️ Outside diff range comments (2)

test-hal/src/mmu.rs (1)

146-169: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Keep FakeMmu aligned with the real Mmu contract.

map / unmap currently accept unaligned VAs, while the trait requires MmuError::MisalignedAddress. That can let host tests pass against the fake and fail on the BSP implementation.

Suggested fix

-use tyrne_hal::{FrameProvider, MapperFlush, MappingFlags, Mmu, MmuError, PhysFrame, VirtAddr};
+use tyrne_hal::{
+    FrameProvider, MapperFlush, MappingFlags, Mmu, MmuError, PhysFrame, VirtAddr, PAGE_SIZE,
+};
@@
     ) -> Result<MapperFlush, MmuError> {
+        if !va.0.is_multiple_of(PAGE_SIZE) {
+            return Err(MmuError::MisalignedAddress);
+        }
         if as_.mappings.contains_key(&va) {
             return Err(MmuError::AlreadyMapped);
         }
@@
     fn unmap(
         &self,
         as_: &mut FakeAddressSpace,
         va: VirtAddr,
     ) -> Result<(MapperFlush, PhysFrame), MmuError> {
+        if !va.0.is_multiple_of(PAGE_SIZE) {
+            return Err(MmuError::MisalignedAddress);
+        }
         as_.mappings
             .remove(&va)
             .map(|(pa, _)| (MapperFlush::new(va), pa))

🤖 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 `@test-hal/src/mmu.rs` around lines 146 - 169, FakeMmu's map and unmap (methods
on FakeMmu operating on FakeAddressSpace with signatures map(&self, as_: &mut
FakeAddressSpace, va: VirtAddr, ...) and unmap(&self, as_: &mut
FakeAddressSpace, va: VirtAddr, ...)) currently accept unaligned virtual
addresses; update both methods to validate VA alignment before doing anything
and return Err(MmuError::MisalignedAddress) when va.is_aligned() (or equivalent
alignment check used by the real Mmu contract) fails, only proceeding to check
existing mappings/insert/remove and return MapperFlush::new(aligned_va) when the
VA is properly aligned. Ensure you use the same alignment utility or constant
used by the real Mmu so FakeMmu enforces the same contract.

docs/analysis/tasks/phase-b/T-016-mmu-activation.md (1)

16-16: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update the stale hal/src/mmu.rs source links.

This PR moved the module to hal/src/mmu/mod.rs, but these task links still point at the old path and will 404 for readers following the implementation notes.

Also applies to: 28-30, 38-38, 149-149

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/analysis/tasks/phase-b/T-016-mmu-activation.md` at line 16, Update the
stale source links in docs/analysis/tasks/phase-b/T-016-mmu-activation.md that
point to the moved module: replace occurrences of the old target hal/src/mmu.rs
(e.g. the [`Mmu`](../../../../hal/src/mmu.rs) link and the other links at lines
~28-30, 38, 149) with the new module path hal/src/mmu/mod.rs so the markdown
links resolve to the renamed module; ensure all instances in this file are
updated consistently (search for "hal/src/mmu.rs" and change to
"hal/src/mmu/mod.rs").

🤖 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 `@bsp-qemu-virt/src/mmu_bootstrap.rs`:
- Around line 173-185: The asm block in the unsafe code uses the option "nomem"
which allows memory operations to be reordered around it, breaking the required
ordering with previous volatile writes that set up page tables. To fix this,
either remove the "nomem" option entirely to make the compiler treat this asm as
a memory barrier, or add explicit memory constraints or clobbers in the asm
macro to ensure the compiler enforces ordering of memory accesses before and
after this block. Locate this fix in the unsafe asm block configuring system
registers (the one setting mair_el1, tcr_el1, ttbr0_el1, and ttbr1_el1).

In `@bsp-qemu-virt/src/mmu.rs`:
- Around line 124-147: The SAFETY comment above the unsafe asm block that writes
ttbr0_el1 (the asm! that uses "msr ttbr0_el1, {0}", "tlbi vmalle1", "dsb ish",
"isb", in(reg) ttbr0, options(nostack, nomem)) must be expanded to state
explicitly: (a) why unsafe is required — direct system register/TLB manipulation
cannot be expressed in safe Rust and requires inline assembly; (b) the
invariants — that ttbr0 is a valid root translation table prepared by
create_address_space (zero-initialised, exclusively-owned PhysFrame with VMSAv8
layout), that callers ensured no concurrent CPU uses this mapping, and that the
sequence TLBI/DSB/ISB fully synchronises TLBs; and (c) why a safer alternative
was rejected — high-level abstractions or CPU instructions wrappers are not
available/insufficient to perform the exact ordered MSR/TLBI/DSB/ISB sequence
with required memory/barrier semantics and register operands, so inline asm! is
the only way to guarantee the correct instruction ordering and operands. Apply
the same pattern of expansion to the other unsafe asm blocks referenced (lines
~166-185, 197-213, 223-248, 305-356, 419-425), referencing their specific asm
sequences and the same three points (need for unsafe, invariants, and rejection
of safer alternatives).
- Around line 150-187: The map implementation currently lets
flags_to_descriptor_bits silently drop EXECUTE when DEVICE is set; update map
(or a helper called before calling flags_to_descriptor_bits) to validate the
requested MappingFlags and return Err(MmuError::InvalidFlags) for any
combinations that cannot be represented (e.g., DEVICE with EXECUTE/USER_EXECUTE
if flags_to_descriptor_bits forces PXN/UXN=1), so reject unrepresentable flag
combos up-front instead of encoding them silently; locate the check in map
(caller of walk_and_install_leaf and flags_to_descriptor_bits) and add the guard
that compares requested flags vs. the set of representable flags and returns
InvalidFlags when there is any mismatch.
- Around line 216-248: The TLBI sequences in invalidate_tlb_address and
invalidate_tlb_all are missing the required store barrier; insert a "dsb ishst"
(DSB ISHST) immediately before the TLBI instruction in both asm! blocks so
descriptor writes are globally observable before invalidation (keep the existing
"dsb ish"/"isb" sequence after TLBI and preserve options(nostack, nomem) and the
unsafe/audit comments).

In `@hal/src/mmu/mod.rs`:
- Around line 233-239: The docstring for MapperFlush::new claims the constructor
is only visible to tyrne_hal implementers but the function is currently pub; fix
this by tightening the visibility to match the docs: change the signature of
MapperFlush::new to pub(crate) const fn new(va: VirtAddr) -> Self so only
in-crate code (and in-tree test impls) can construct flush tokens; update any
call sites if necessary or, alternatively, if you intentionally want downstream
access, change the docstring to state it's public API instead of restricting it
to tyrne_hal implementers.

---

Outside diff comments:
In `@docs/analysis/tasks/phase-b/T-016-mmu-activation.md`:
- Line 16: Update the stale source links in
docs/analysis/tasks/phase-b/T-016-mmu-activation.md that point to the moved
module: replace occurrences of the old target hal/src/mmu.rs (e.g. the
[`Mmu`](../../../../hal/src/mmu.rs) link and the other links at lines ~28-30,
38, 149) with the new module path hal/src/mmu/mod.rs so the markdown links
resolve to the renamed module; ensure all instances in this file are updated
consistently (search for "hal/src/mmu.rs" and change to "hal/src/mmu/mod.rs").

In `@test-hal/src/mmu.rs`:
- Around line 146-169: FakeMmu's map and unmap (methods on FakeMmu operating on
FakeAddressSpace with signatures map(&self, as_: &mut FakeAddressSpace, va:
VirtAddr, ...) and unmap(&self, as_: &mut FakeAddressSpace, va: VirtAddr, ...))
currently accept unaligned virtual addresses; update both methods to validate VA
alignment before doing anything and return Err(MmuError::MisalignedAddress) when
va.is_aligned() (or equivalent alignment check used by the real Mmu contract)
fails, only proceeding to check existing mappings/insert/remove and return
MapperFlush::new(aligned_va) when the VA is properly aligned. Ensure you use the
same alignment utility or constant used by the real Mmu so FakeMmu enforces the
same contract.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9ce8031a-ab6a-46bb-aafc-863b612a592a

📥 Commits

Reviewing files that changed from the base of the PR and between 6494ed2 and d9dc53d.

📒 Files selected for processing (15)

• bsp-qemu-virt/linker.ld
• bsp-qemu-virt/src/main.rs
• bsp-qemu-virt/src/mmu.rs
• bsp-qemu-virt/src/mmu_bootstrap.rs
• docs/analysis/reports/perf-baseline-2026-05-08-post-t-016-mmu-activated.md
• docs/analysis/tasks/phase-b/T-016-mmu-activation.md
• docs/architecture/boot.md
• docs/architecture/hal.md
• docs/audits/unsafe-log.md
• docs/roadmap/current.md
• docs/roadmap/phases/phase-b.md
• hal/src/lib.rs
• hal/src/mmu/mod.rs
• hal/src/mmu/vmsav8.rs
• test-hal/src/mmu.rs

[cemililik]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

🧹 Nitpick comments (2)

test-hal/src/mmu.rs (1)

156-162: 💤 Low value

Consider extracting VA alignment validation to a helper.

The alignment check logic is duplicated between map (lines 156-162) and unmap (lines 184-188). While this is not a critical issue, extracting it to a private helper would improve maintainability:

♻️ Optional refactor to eliminate duplication

Add a helper method to the impl block:

+    fn validate_va_alignment(va: VirtAddr) -> Result<(), MmuError> {
+        if !va.0.is_multiple_of(PAGE_SIZE) {
+            return Err(MmuError::MisalignedAddress);
+        }
+        Ok(())
+    }
+
     fn map(
         &self,
         as_: &mut FakeAddressSpace,
         va: VirtAddr,
         pa: PhysFrame,
         flags: MappingFlags,
         _frames: &mut dyn FrameProvider,
     ) -> Result<MapperFlush, MmuError> {
-        // Mirror the real `Mmu` contract: VA must be `PAGE_SIZE`-aligned.
-        // Without this check, kernel-side code that passes unaligned VAs
-        // would silently succeed under host tests and fail on real hardware.
-        // (PR `#23` review-round 2026-05-09 Finding 7.)
-        if !va.0.is_multiple_of(PAGE_SIZE) {
-            return Err(MmuError::MisalignedAddress);
-        }
+        Self::validate_va_alignment(va)?;

And similarly in unmap:

     fn unmap(
         &self,
         as_: &mut FakeAddressSpace,
         va: VirtAddr,
     ) -> Result<(MapperFlush, PhysFrame), MmuError> {
-        // Mirror the real `Mmu` contract: VA must be `PAGE_SIZE`-aligned.
-        // (PR `#23` review-round 2026-05-09 Finding 7.)
-        if !va.0.is_multiple_of(PAGE_SIZE) {
-            return Err(MmuError::MisalignedAddress);
-        }
+        Self::validate_va_alignment(va)?;

The Finding 7 comment could move to the helper's doc-comment.

Also applies to: 184-188

🤖 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 `@test-hal/src/mmu.rs` around lines 156 - 162, The VA alignment check in map
and unmap is duplicated; extract it into a private helper (e.g., fn
validate_va_alignment(va: VirtualAddress) -> Result<(), MmuError>) inside the
same impl for Mmu so both map and unmap call this helper instead of duplicating
the if !va.0.is_multiple_of(PAGE_SIZE) check, returning
Err(MmuError::MisalignedAddress) on failure; move the "Finding 7" explanatory
comment into the helper's doc-comment for context and update calls in map and
unmap to use validate_va_alignment(va)?.

bsp-qemu-virt/src/mmu_bootstrap.rs (1)

108-112: 💤 Low value

Consider making "why safer alternatives rejected" explicit in unsafe justifications.

The coding guideline requires each unsafe block to document:
(a) why it is needed
(b) what invariants it upholds
(c) why safer alternatives were rejected

All three unsafe blocks (Step 1, Step 2, Step 3) clearly document (a) and (b), but (c) is implicit. While it's evident that direct manipulation of hardware page tables and system registers requires unsafe, explicitly stating this would improve guideline compliance.

For example, Step 1's comment could add:
"(c) Safer alternatives rejected: direct manipulation of hardware page-table descriptors requires raw pointer writes; no safe abstraction exists at this bootstrap stage."

As per coding guidelines: "Every unsafe block must have a comment explaining (a) why it is needed, (b) what invariants it upholds, (c) why safer alternatives were rejected."

Also applies to: 166-185, 201-213

🤖 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 `@bsp-qemu-virt/src/mmu_bootstrap.rs` around lines 108 - 112, Add explicit "(c)
Safer alternatives rejected" text to each SAFETY comment for the unsafe blocks
that build the page-table chain: include a sentence such as "Safer alternatives
rejected: direct manipulation of hardware page-table descriptors and system
registers is required at this bootstrap stage and no safe abstraction is
available," so the SAFETY comment above the L0→L1→L2 write (the block commenting
"write the L0 → L1 → L2 chain") and the two other unsafe blocks referenced (the
Step 1, Step 2, Step 3 blocks around the regions you noted) explicitly document
why a safe alternative was rejected; ensure the new text is added to the
existing SAFETY comment for each unsafe block without changing the technical
invariants already described.

🤖 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.

Nitpick comments:
In `@bsp-qemu-virt/src/mmu_bootstrap.rs`:
- Around line 108-112: Add explicit "(c) Safer alternatives rejected" text to
each SAFETY comment for the unsafe blocks that build the page-table chain:
include a sentence such as "Safer alternatives rejected: direct manipulation of
hardware page-table descriptors and system registers is required at this
bootstrap stage and no safe abstraction is available," so the SAFETY comment
above the L0→L1→L2 write (the block commenting "write the L0 → L1 → L2 chain")
and the two other unsafe blocks referenced (the Step 1, Step 2, Step 3 blocks
around the regions you noted) explicitly document why a safe alternative was
rejected; ensure the new text is added to the existing SAFETY comment for each
unsafe block without changing the technical invariants already described.

In `@test-hal/src/mmu.rs`:
- Around line 156-162: The VA alignment check in map and unmap is duplicated;
extract it into a private helper (e.g., fn validate_va_alignment(va:
VirtualAddress) -> Result<(), MmuError>) inside the same impl for Mmu so both
map and unmap call this helper instead of duplicating the if
!va.0.is_multiple_of(PAGE_SIZE) check, returning
Err(MmuError::MisalignedAddress) on failure; move the "Finding 7" explanatory
comment into the helper's doc-comment for context and update calls in map and
unmap to use validate_va_alignment(va)?.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8e71bca2-4454-4cfe-a0f7-b69aeb443c77

📥 Commits

Reviewing files that changed from the base of the PR and between d9dc53d and d5d88d4.

📒 Files selected for processing (5)

• bsp-qemu-virt/src/mmu.rs
• bsp-qemu-virt/src/mmu_bootstrap.rs
• docs/analysis/tasks/phase-b/T-016-mmu-activation.md
• hal/src/mmu/mod.rs
• test-hal/src/mmu.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• hal/src/mmu/mod.rs
• bsp-qemu-virt/src/mmu.rs