T-017: Physical Memory Manager (PMM) bring-up — bitmap allocator + reservation tracking - #26
Merged
Merged
Conversation
First of four bisectable commits for T-017 (B3 prep — Physical Memory
Manager per ADR-0035). Establishes the new `kernel/src/mm/` subsystem
parent + the `Pmm<N, R>` struct shape + the `Pmm::new` constructor
with three fail-fast pre-mutation validations. No `unsafe`. Tests
ride along as host-only `#[cfg(test)]` block.
What lands:
kernel/src/mm/mod.rs [new]
Memory-management subsystem parent. Hosts the PMM today; will
host the AddressSpace data-structure module per ADR-0028
placeholder (B3 §2) when T-018 lands.
PhysFrameRange — half-open `[start, end)` range; carries raw
PhysAddr values rather than PhysFrame to describe multi-page
reservations in one entry. Includes is_aligned / len_bytes /
frame_count / contains const-fn helpers. Used by Pmm::new for
the BSP-provided extent + reservation list.
kernel/src/mm/pmm.rs [new]
PmmError enum (MisalignedAddress / OutOfRange /
TooManyReservedRanges / DoubleFree); #[non_exhaustive] for
forward-compat with future B5+ MemoryRegionCap variants.
PmmStats struct — diagnostic snapshot for runtime self-checks +
the boot-banner format. Counter fields share `_frames` postfix
by design (disambiguates total/reserved/allocated/free at
destructure sites); `clippy::struct_field_names` allowed with
reason.
Pmm<const N: usize, const R: usize> — bitmap allocator, where:
- N = bitmap byte count (BSP-sized: 4096 for QEMU virt's
32 K frames at 128 MiB / 4 KiB)
- R = reserved-range cache capacity (BSP-sized: bsp-qemu-virt
will pick R = 8 in commit 4 to cover its 3 v1 reservations
plus headroom)
Pmm::new(extent, reserved) -> Result<Self, PmmError>:
Three fail-fast validations before any bitmap mutation:
(i) extent.start + extent.end page-aligned -> MisalignedAddress
(ii) every reserved range fits inside [extent) AND is
page-aligned -> MisalignedAddress / OutOfRange
(iii) reserved.len() <= R -> TooManyReservedRanges
Plus an extent-vs-bitmap-size sanity check (extent.frame_count()
must fit in N * 8 bits — BSP programming error surfaced as
OutOfRange rather than silent buffer overflow).
After all pass: walks reserved list, sets bitmap bits, copies
list into reserved_ranges array (remainder None), computes
counters, sets hint to first_zero_bit.
Bitmap arithmetic (set_bit / read_bit / first_zero_bit) — pure
safe Rust; #[deny(clippy::arithmetic_side_effects)] satisfied via
saturating_add / saturating_sub / wrapping_div throughout.
Pmm::extent + Pmm::stats inherent accessors. The struct's other
fields (bitmap / reserved_ranges / hint) carry an explicit
`#[allow(dead_code, reason = "transient until commit 2 lands
alloc_frame / free_frame / stats")]` annotation; commit 2
removes the allow when those methods are added.
5 host tests (all in kernel/src/mm/pmm.rs::tests under
#[cfg(test)] + #[allow(clippy::{arithmetic_side_effects, unwrap_used,
expect_used, panic})] per the existing kernel-test convention):
- new_marks_reserved_ranges_and_initialises_counters
- new_rejects_too_many_reserved_ranges
- new_rejects_unaligned_extent (start + end + reserved-range
bound variants)
- new_rejects_reserved_range_outside_extent (entirely-above /
partially-exceeding / below variants)
- extent_16f_fixture_sanity (sanity-check the test fixture
itself; pins the fixture's frame_count + is_aligned + contains
invariants for use by future commits' tests)
The 5 tests pin every error path in Pmm::new + the happy path's
counter+hint+reserved_ranges initialisation per ADR-0035
§Simulation §Step 0.
kernel/src/lib.rs
Adds `pub mod mm;` to the top-level module declaration list.
Module slot reserved between `ipc` and `obj` alphabetically.
Verification:
- cargo fmt --check clean
- cargo clippy (host) clean -D warnings
- cargo kernel-clippy clean -D warnings
- cargo test 190/190 host (was 185; +5 PMM
construction tests)
- cargo kernel-build clean
The four-commit chain proceeds:
- commit 2: alloc_frame / free_frame / stats + frame-zeroing
unsafe + 8 more host tests + audit-log entry
- commit 3: impl FrameProvider for Pmm + 1 test
- commit 4: bsp-qemu-virt PMM publication + smoke verification
Refs: ADR-0035, T-017
…AFE-2026-0026
Second of four bisectable commits for T-017. Adds the runtime PMM
alloc / free path with the frame-zeroing unsafe block, plus a new
audit-log entry per ADR-0035 §Dependency-chain step 5's
adjudication-deferred verdict (PMM frame-zeroing is semantically
distant from UNSAFE-2026-0001's PL011 MMIO base blessing).
What lands:
kernel/src/mm/pmm.rs
- Pmm::alloc_frame() -> Option<PhysFrame>:
Forward-from-hint linear bitmap scan for the first 0 bit;
on hit sets the bit, advances hint, decrements free_count,
increments allocated_count, zero-fills the 4 KiB frame
contents via core::ptr::write_bytes, returns Some(frame).
None on bitmap fully set. The wrap-then-scan-prefix step is
retained for forward-compat with SMP (per ADR-0035 §Simulation
§Step 3 forward-compat note); structurally unreachable in v1's
single-core cooperative discipline.
- Pmm::free_frame(frame) -> Result<(), PmmError>:
Three-stage validation in order — extent-bounds (returns
OutOfRange if PA outside [extent.start, extent.end)),
defensive reserved-range scan over Some(_) slots only
(returns DoubleFree if PA falls in any cached reserved
range), bitmap-bit check (returns DoubleFree if already
Free). On all-pass: clears bit, rewinds hint = min(hint, idx),
counters updated. Each error path leaves bitmap + counter
state byte-stable.
- clear_bit private helper (companion to set_bit / read_bit).
- PhysFrame import from tyrne_hal.
- Removed the commit-1 #[allow(dead_code)] on the Pmm struct
(bitmap / reserved_ranges / hint are now read by alloc / free).
8 new host tests (kernel/src/mm/pmm.rs::tests):
- alloc_frame_returns_first_free_and_zeroes_payload — uses
a host-allocated PAGE_SIZE-aligned Vec<u8> as backing storage,
pre-poisons it with 0xA5, allocs, asserts every byte of the
returned frame is 0u8 (pins the FrameProvider zero-init
contract).
- free_frame_clears_bit_and_rewinds_hint — alloc x3, free f1,
asserts hint rewinds to 0 and the next alloc returns f1
again.
- free_frame_rejects_double_free_and_reserved — pins the
DoubleFree path for both Reserved-range frames and never-
allocated frames (bitmap unchanged on rejection).
- alloc_frame_returns_none_when_exhausted — pins §Simulation
§Step 3.
- alloc_frame_recovers_after_free_under_exhaustion — pins
that the rewind discipline reclaims a freed frame even
after exhaustion.
- stats_parity_with_bitmap_bit_count — pins the cached-
counter ↔ bitmap-popcount invariant.
- free_frame_reserved_check_iterates_only_populated_slots —
pins the Critical-row defensive scan's contract that
populated Some(_) slots are iterated and None slots are
skipped (regression guard against accidental Wildcard
interpretation of None).
- free_frame_rejects_pa_outside_extent — pins the §Step 2
extent-bounds fail-fast for PAs below extent.start and at /
above extent.end (counters unchanged).
Total kernel test count rises 100 → 113 (+13 PMM tests so far;
commits 3 + 4 won't add more). Workspace total 190 → 198.
docs/audits/unsafe-log.md
- **UNSAFE-2026-0026** introduced (NEW ENTRY, not Amendment):
PMM frame-zeroing via core::ptr::write_bytes. ADR-0035
§Dependency-chain step 5 explicitly left this call to T-017's
security-review verdict; the verdict (recorded in the entry's
Introduced field) is that UNSAFE-2026-0001's original scope
(PL011 MMIO base address blessing in a typed Console wrapper) is
semantically distant enough from PMM RAM zero-fill that an
Amendment would stretch the umbrella beyond unsafe-policy.md
§3's "same safety argument" requirement. The two operations
differ on what they touch (typed MMIO base vs. RAM zero),
what proves ownership (well-known QEMU virt PL011 base vs.
just-set bitmap bit), and what the failure mode is (silent
firmware confusion vs. bitmap-or-extent corruption). The new
entry follows the Operation / Invariants / Rejected-alternatives
shape per unsafe-policy.md §3.
- Five Invariants explicitly named:
1. Page-alignment (Pmm::new validation (i) propagates).
2. Exclusive ownership at write time (just-set bitmap bit).
3. Identity mapping post-MMU (ADR-0027 §Decision outcome (a)).
4. Bitmap math overflow-free (saturating_*, wrapping_div).
5. write_bytes ordering (single-core; no peer reader).
- Five Rejected alternatives walked: slice-from-raw-parts-mut
(same unsafe, different syntax); lazy zero on page-fault
(no v1 page-fault routing); skip zero (FrameProvider contract
+ B5+ isolation hazard); volatile_set_memory (no peer
observer); new Mmu::zero_frame trait method (relocates audit
point, doesn't remove it).
- Status: Active; v1 site is reached on every boot (PMM
constructed in commit 4) but the zero-fill itself is
unexercised in v1's demo — host-test coverage + Miri Stacked
Borrows clean is the v1 evidence base. ADR-0035's "zero new
audit entries" §Positive consequence flips to "one new entry"
per the adjudication caveat; ADR-0035 itself stays unchanged.
Verification:
- cargo fmt --check clean
- cargo clippy (host) clean -D warnings
- cargo kernel-clippy clean -D warnings
- cargo test 198/198 host (was 190; +8 PMM
alloc/free/stats tests)
- cargo +nightly miri test 13/13 PMM tests clean (Stacked
Borrows + raw-pointer discipline)
- cargo kernel-build clean
Refs: ADR-0035, T-017
Audit: UNSAFE-2026-0026
Third of four bisectable commits for T-017. Layers the
tyrne_hal::FrameProvider trait impl on top of Pmm::alloc_frame so
the PMM can be passed directly to Mmu::map as &mut dyn FrameProvider.
Trivial — the impl forwards to the inherent method. Pure safe Rust;
no audit-log change.
What lands:
kernel/src/mm/pmm.rs
- FrameProvider import added (`tyrne_hal::FrameProvider`).
- `impl<const N: usize, const R: usize> FrameProvider for Pmm<N, R>`
block: single fn alloc_frame(&mut self) -> Option<PhysFrame>
that delegates to Pmm::alloc_frame. Doc-comment names the
Mmu::map contract (None propagates as MmuError::OutOfFrames per
ADR-0009 + ADR-0035 §Decision drivers).
- 1 new host test: alloc_frame_implements_frame_provider —
exercises the trait method via &mut dyn FrameProvider so the
impl integration is pinned (regression guard against accidental
signature drift between Pmm::alloc_frame and
FrameProvider::alloc_frame). Allocates 4 frames + asserts the
5th is None.
Workspace test count rises 198 → 199.
Verification:
- cargo fmt --check clean
- cargo clippy (host) clean -D warnings
- cargo kernel-clippy clean -D warnings
- cargo test 199/199 host (was 198; +1
FrameProvider impl test)
- cargo kernel-build clean
Refs: ADR-0035, T-017
Final of four bisectable commits for T-017. Wires the PMM into the
QEMU virt BSP boot sequence — instantiates `Pmm<4096, 8>` over the
128 MiB physical-RAM extent, marks two reserved regions (QEMU
firmware + kernel image / `.bss` / `.boot_pt` / boot stack), and
prints the `tyrne: pmm initialized (...)` banner immediately after
`tyrne: mmu activated` and before GIC init. v1's cooperative IPC
demo never calls `alloc_frame` so the PMM stays unused at runtime;
init success + counter values are the v1 smoke evidence.
What lands:
bsp-qemu-virt/src/main.rs (constants + types):
- PMM_EXTENT_START = 0x4000_0000 (QEMU virt RAM origin per
ADR-0012).
- PMM_EXTENT_END = 0x4800_0000 (128 MiB above).
- KERNEL_IMAGE_START = 0x4008_0000 (linker.ld -kernel load
address; the 512 KiB below is QEMU-firmware-reserved).
- PMM_BITMAP_BYTES = (extent / PAGE_SIZE / 8) = 4096 bytes for
32 768 frames.
- PMM_RESERVED_RANGES = 8 (v1 uses 2; headroom for future BSP
layouts with DTB / ATF / ACPI / initrd / framebuffer
reservations).
- type BspPmm = Pmm<PMM_BITMAP_BYTES, PMM_RESERVED_RANGES> —
per-BSP-const-generic discipline of ADR-0035; future BSPs
instantiate their own (Pi 4 with 4 GiB → N = 131072, etc.).
bsp-qemu-virt/src/main.rs (linker symbols):
- Added extern "C" { static __stack_top: u8; }. linker.ld
aligns this to 16 bytes (post-`ALIGN(16); . = . + 64K`); the
BSP rounds it up to 4 KiB at PMM-init time via
`addr.saturating_add(PAGE_SIZE - 1) & !(PAGE_SIZE - 1)`. The
few-byte slack falls into the kernel-reserved range so the
round-up cannot collide with a valid runtime allocation.
bsp-qemu-virt/src/main.rs (static PMM cell):
- static PMM: StaticCell<BspPmm> = StaticCell::new();
Published before any post-bootstrap Mmu::map caller could
use it (none in v1; first caller is B3+ AddressSpace
bring-up via T-018). Sized ~4.1 KiB of .bss per ADR-0035
§Consequences §Bounded metadata: 4 KiB bitmap + 8 ×
Option<PhysFrameRange> reserved-range cache (~144 bytes
with alignment) + 5 × usize counters.
bsp-qemu-virt/src/main.rs (kernel_entry wiring):
- Insert PMM init between "tyrne: mmu activated" print and
GIC init. Three steps:
1. Compute rounded-up stack-top from the __stack_top linker
symbol.
2. Build the two reserved-range entries: firmware region
[0x4000_0000, 0x4008_0000) + kernel-everything region
[0x4008_0000, __stack_top_aligned_up).
3. BspPmm::new(extent, &reserved).expect("...") +
StaticCell::write. The .expect() is structurally
unreachable in v1 (the static reservation list is
well-formed at compile time; R=8 is much greater than the
2-range count); the message documents the
kernel-discipline contract per CLAUDE.md non-negotiable
rules.
4. Print "tyrne: pmm initialized (N frames available; M
reserved)" via FmtWriter — the only new UART line in
v1's smoke trace.
The kernel_entry full order is now:
hello → VBAR_EL1 install → boot_ns snapshot → mmu_bootstrap
→ "tyrne: mmu activated" → PMM init → "tyrne: pmm initialized
(N available; M reserved)" → GIC init + DAIF unmask → timer
banner → kernel-object setup → IPC + scheduler → start().
Verification:
- cargo fmt --check clean
- cargo clippy (host) clean -D warnings
- cargo kernel-clippy clean -D warnings
- cargo test 199/199 host (unchanged —
BSP wiring touches no test
code; the 14 PMM tests
landed in commits 1+2+3)
- cargo +nightly miri test 14/14 PMM tests clean
- cargo kernel-build clean
- QEMU smoke trace (verbatim):
tyrne: hello from kernel_main
tyrne: mmu activated
tyrne: pmm initialized (32604 frames available; 164 reserved)
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 = 7430992 ns
Total frames 32768 = free 32604 + reserved 164 ✓ (sanity:
164 frames × 4 KiB = 656 KiB of reservation = 128 frames /
512 KiB firmware + 36 frames / 144 KiB kernel image + .bss
+ .boot_pt 16 KiB + 64 KiB stack + alignment slack).
- QEMU `-d int,unimp,guest_errors`:
443 instances of pre-existing "PL011 data written to
disabled UART" warning (was 379 post-T-016; +64 = bytes
of the new "tyrne: pmm initialized (...)" line). No new
fault classes; no Translation Faults; no Permission
Faults; no Unallocated Instructions.
T-017 implementation chain complete. Remaining work (separate
commit on this branch): docs updates (memory-management.md,
boot.md, roadmap banners, T-017 status flip In Progress → Done).
Refs: ADR-0035, T-017
Audit: UNSAFE-2026-0001
Documentation polish + status flips after the four-commit T-017 implementation chain (2533732 / 6500337 / fdb52d6 / a2ffcb6). No code change; gates remain at 199/199 host + 14/14 PMM-miri + clippy / fmt / kernel-build clean; smoke trace stable. Updates: - docs/analysis/tasks/phase-b/T-017-physical-memory-manager.md Status flipped `In Progress → Done`. Review-history gains three new rows: 2026-05-09 ADR-0035-Accepted + Draft-to-InProgress flip; 2026-05-10 In-Progress-to-Done flip with headline numbers + the UNSAFE-2026-0026 new-entry adjudication trail (per ADR-0035 §Dependency chain step 5's deferred verdict — PL011 MMIO base blessing vs. PMM RAM zero-fill semantic distance forced a fresh entry rather than an Amendment to UNSAFE-2026-0001). - docs/architecture/memory-management.md §"Frame allocation discipline" rewritten from the "PMM is not part of T-016" forward-looking framing to the post-T-017 reality. New content: bitmap allocator design recap (N bitmap bytes + R reserved-range cache capacity as per-BSP const generics per ADR-0035); API contract (Pmm::new + alloc_frame + free_frame + extent + stats); FrameProvider integration (one v1 caller — none in runtime; first runtime exercise B3+); the v1 smoke-trace banner with the 32 604 + 164 = 32 768 frame sanity-check; audit-log surface (UNSAFE-2026-0026's five invariants); forward-portability note (bitmap regime-independent; frame-zeroing needs a phys_to_virt helper post-high-half). - docs/architecture/boot.md Stage 3 description extended with the PMM-init step; the Mermaid sequence diagram refreshed with the new `Pmm::new` + `tyrne: pmm initialized (...)` arrows. No other diagram or invariant changes. - docs/roadmap/current.md Active-milestone line updated: B3 §1 closed 2026-05-10; next is ADR-0028 + T-018. Active-task line: none — T-017 moved to Done. Headline numbers + UNSAFE-2026-0026 adjudication summary inline. - docs/roadmap/phases/phase-b.md B3 §Status block flipped to "B3 §1 closed 2026-05-10". Tasks-under-B3 row for T-017 updated to "Done (2026-05-10; 4 bisectable commits on branch t-017-physical-memory-manager; UNSAFE-2026-0026 introduced as new entry)". - docs/analysis/tasks/phase-b/README.md T-017 row flipped to "Done (2026-05-10)". ADR-0035 itself stays byte-stable post-T-017 per its §Dependency chain step 5's adjudication-deferred contract — the "zero new audit entries" → "one new entry" promotion is recorded in T-017's review-history + this commit body + UNSAFE-2026-0026's own §Introduced field. No retroactive ADR edit needed. Verification: - cargo fmt --check clean - cargo clippy (host) clean -D warnings - cargo kernel-clippy clean -D warnings - cargo test 199/199 host (unchanged — docs only) - cargo kernel-build clean - QEMU smoke full demo through "tyrne: all tasks complete"; the "tyrne: pmm initialized (32604 frames available; 164 reserved)" line in the expected position T-017 arc complete. Branch is mergeable; PR opens next. Refs: ADR-0035, T-017
ⓘ 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. |
Reviewer's GuideIntroduces a bitmap-based Physical Memory Manager (PMM) in the kernel, wires it into the QEMU-virt BSP boot path as the concrete FrameProvider implementation over the 128 MiB RAM extent with reservation tracking, adds targeted tests and a new unsafe audit entry for frame zeroing, and updates documentation/roadmap to reflect T-017 completion and PMM behaviour. Sequence diagram for kernel_entry boot with PMM initializationsequenceDiagram
actor FW as Firmware
participant ST as StartStub
participant KE as kernel_entry
participant MMU as mmu_bootstrap
participant PMM as Pmm
participant GIC as GicInit
participant U as Console
FW->>ST: jump to _start (EL1/EL2)
ST->>ST: setup stack, zero BSS
ST->>KE: call kernel_entry()
KE->>U: write_bytes("tyrne: hello ...")
KE->>KE: install EL1 vector table
KE->>KE: boot_ns = cpu.now_ns()
KE->>MMU: mmu_bootstrap()
MMU-->>KE: MMU activated (identity map)
KE->>U: write_bytes("tyrne: mmu activated\n")
KE->>KE: compute stack_top_aligned_up
KE->>KE: build PhysFrameRange extent
KE->>KE: build reserved[0], reserved[1]
KE->>PMM: Pmm::new(extent, &reserved)
PMM-->>KE: Pmm instance
KE->>PMM: stats()
PMM-->>KE: PmmStats
KE->>U: write_bytes("tyrne: pmm initialized (... )\n")
KE->>GIC: init GIC + unmask DAIF.I
GIC-->>KE: ready
KE->>U: write_bytes("tyrne: timer ready (...)\n")
KE->>KE: setup kernel objects, IPC, scheduler
Updated class diagram for the Physical Memory Manager and related typesclassDiagram
class PmmError {
<<enum>>
MisalignedAddress
OutOfRange
TooManyReservedRanges
DoubleFree
}
class PmmStats {
+usize total_frames
+usize reserved_frames
+usize allocated_frames
+usize free_frames
}
class PhysFrameRange {
+PhysAddr start
+PhysAddr end
+new(start: PhysAddr, end: PhysAddr) PhysFrameRange
+is_aligned() bool
+len_bytes() usize
+frame_count() usize
+contains(pa: PhysAddr) bool
}
class FrameProvider {
<<trait>>
+alloc_frame() PhysFrame
}
class Pmm_N_R {
<<Pmm<N,R>>
-u8[N] bitmap
-PhysFrameRange extent
-PhysFrameRange[R] reserved_ranges
-usize hint
-usize free_count
-usize reserved_count
-usize allocated_count
+new(extent: PhysFrameRange, reserved: PhysFrameRange[]) Result~Pmm_N_R, PmmError~
+extent() PhysFrameRange
+stats() PmmStats
+alloc_frame() PhysFrame
+free_frame(frame: PhysFrame) Result~void, PmmError~
}
class PhysAddr
class PhysFrame
PhysFrameRange --> PhysAddr : uses
Pmm_N_R --> PhysFrameRange : manages
Pmm_N_R --> PmmStats : returns
Pmm_N_R --> PmmError : errors
Pmm_N_R --> PhysFrame : allocates
FrameProvider <|.. Pmm_N_R : implements
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
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 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR implements a bitmap-based Physical Memory Manager (PMM) for the kernel with frame allocation, deallocation, and reserved-range tracking. The PMM is initialized during kernel boot after MMU activation, wired into the BSP, exported from the kernel crate, and documented with comprehensive unsafe audits and task tracking. ChangesPhysical Memory Manager (PMM) Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
Pmm::alloc_frame, ifPhysFrame::from_alignedever returnedNoneyou would already have mutated the bitmap and counters; consider either constructing thePhysFramebefore updating state or at least enforcing the invariant with an explicitdebug_assert!/expectso a violation can’t silently desynchronise internal bookkeeping. - The
PMMStaticCellinkernel_entryis manipulated via repeatedunsafe { (*PMM.0.get())... }blocks; it would be cleaner and less error‑prone to wrap this pattern in a small helper onStaticCell(or a BSP-local wrapper) so future uses don’t need to reach into its internals or duplicate the raw‑pointer logic. - In the test fixture
extent_16f, the doc comment claims "16 frames (16 KiB)" but the range0x4000_0000..0x4000_4000is 16 KiB = 4 frames and the test assertsframe_count() == 4; updating the comment to match the actual size will avoid confusion when reading these tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Pmm::alloc_frame`, if `PhysFrame::from_aligned` ever returned `None` you would already have mutated the bitmap and counters; consider either constructing the `PhysFrame` before updating state or at least enforcing the invariant with an explicit `debug_assert!/expect` so a violation can’t silently desynchronise internal bookkeeping.
- The `PMM` `StaticCell` in `kernel_entry` is manipulated via repeated `unsafe { (*PMM.0.get())... }` blocks; it would be cleaner and less error‑prone to wrap this pattern in a small helper on `StaticCell` (or a BSP-local wrapper) so future uses don’t need to reach into its internals or duplicate the raw‑pointer logic.
- In the test fixture `extent_16f`, the doc comment claims "16 frames (16 KiB)" but the range `0x4000_0000..0x4000_4000` is 16 KiB = 4 frames and the test asserts `frame_count() == 4`; updating the comment to match the actual size will avoid confusion when reading these tests.
## Individual Comments
### Comment 1
<location path="kernel/src/mm/pmm.rs" line_range="201-210" />
<code_context>
+ let mut reserved_count: usize = 0;
</code_context>
<issue_to_address>
**issue (bug_risk):** Overlapping reserved ranges double-count frames and can make stats inconsistent with the bitmap/extents.
This relies on reserved ranges being non-overlapping. If a BSP passes overlapping or off‑by‑one-touching ranges, the bitmap remains correct but `reserved_count` will be inflated and can even exceed `total_frames`, making `stats()` inconsistent (`total_frames` > `extent.frame_count()`, `free_frames` clamped from a negative value). Either validate and reject overlapping ranges in `Pmm::new`, or derive `reserved_count` from the bitmap after applying all ranges (e.g., by counting set bits once).
</issue_to_address>
### Comment 2
<location path="kernel/src/mm/pmm.rs" line_range="246-259" />
<code_context>
+ /// Return a diagnostic snapshot of the PMM's frame-state
+ /// counters.
+ #[must_use]
+ pub fn stats(&self) -> PmmStats {
+ PmmStats {
+ total_frames: self
+ .free_count
+ .saturating_add(self.reserved_count)
+ .saturating_add(self.allocated_count),
+ reserved_frames: self.reserved_count,
+ allocated_frames: self.allocated_count,
+ free_frames: self.free_count,
+ }
+ }
</code_context>
<issue_to_address>
**suggestion:** Deriving `total_frames` from cached counters risks divergence from the managed extent.
Currently `stats().total_frames` is derived from `free_count + reserved_count + allocated_count`, which can diverge from `extent.frame_count()` if any counter drifts from the bitmap/extent. Using the extent as the single source of truth for `total_frames` avoids this class of inconsistency:
```rust
pub fn stats(&self) -> PmmStats {
PmmStats {
total_frames: self.extent.frame_count(),
reserved_frames: self.reserved_count,
allocated_frames: self.allocated_count,
free_frames: self.free_count,
}
}
```
You can still detect counter drift by asserting `free + reserved + allocated == extent.frame_count()` in tests or debug builds, while callers always see a consistent `total_frames`.
```suggestion
/// Return a diagnostic snapshot of the PMM's frame-state
/// counters.
#[must_use]
pub fn stats(&self) -> PmmStats {
PmmStats {
total_frames: self.extent.frame_count(),
reserved_frames: self.reserved_count,
allocated_frames: self.allocated_count,
free_frames: self.free_count,
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+201
to
+210
| let mut reserved_count: usize = 0; | ||
| for (idx, range) in reserved.iter().enumerate() { | ||
| // Mark every covered frame as Reserved. | ||
| // | ||
| // Saturating sub keeps `clippy::arithmetic_side_effects` | ||
| // happy; validation (ii) above guarantees | ||
| // `range.start >= extent.start` so the saturation never | ||
| // truncates in well-formed input. | ||
| let start_idx = range | ||
| .start |
There was a problem hiding this comment.
issue (bug_risk): Overlapping reserved ranges double-count frames and can make stats inconsistent with the bitmap/extents.
This relies on reserved ranges being non-overlapping. If a BSP passes overlapping or off‑by‑one-touching ranges, the bitmap remains correct but reserved_count will be inflated and can even exceed total_frames, making stats() inconsistent (total_frames > extent.frame_count(), free_frames clamped from a negative value). Either validate and reject overlapping ranges in Pmm::new, or derive reserved_count from the bitmap after applying all ranges (e.g., by counting set bits once).
Comment on lines
+246
to
+259
| /// Return a diagnostic snapshot of the PMM's frame-state | ||
| /// counters. | ||
| #[must_use] | ||
| pub fn stats(&self) -> PmmStats { | ||
| PmmStats { | ||
| total_frames: self | ||
| .free_count | ||
| .saturating_add(self.reserved_count) | ||
| .saturating_add(self.allocated_count), | ||
| reserved_frames: self.reserved_count, | ||
| allocated_frames: self.allocated_count, | ||
| free_frames: self.free_count, | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion: Deriving total_frames from cached counters risks divergence from the managed extent.
Currently stats().total_frames is derived from free_count + reserved_count + allocated_count, which can diverge from extent.frame_count() if any counter drifts from the bitmap/extent. Using the extent as the single source of truth for total_frames avoids this class of inconsistency:
pub fn stats(&self) -> PmmStats {
PmmStats {
total_frames: self.extent.frame_count(),
reserved_frames: self.reserved_count,
allocated_frames: self.allocated_count,
free_frames: self.free_count,
}
}You can still detect counter drift by asserting free + reserved + allocated == extent.frame_count() in tests or debug builds, while callers always see a consistent total_frames.
Suggested change
| /// Return a diagnostic snapshot of the PMM's frame-state | |
| /// counters. | |
| #[must_use] | |
| pub fn stats(&self) -> PmmStats { | |
| PmmStats { | |
| total_frames: self | |
| .free_count | |
| .saturating_add(self.reserved_count) | |
| .saturating_add(self.allocated_count), | |
| reserved_frames: self.reserved_count, | |
| allocated_frames: self.allocated_count, | |
| free_frames: self.free_count, | |
| } | |
| } | |
| /// Return a diagnostic snapshot of the PMM's frame-state | |
| /// counters. | |
| #[must_use] | |
| pub fn stats(&self) -> PmmStats { | |
| PmmStats { | |
| total_frames: self.extent.frame_count(), | |
| reserved_frames: self.reserved_count, | |
| allocated_frames: self.allocated_count, | |
| free_frames: self.free_count, | |
| } | |
| } |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the Physical Memory Manager (PMM) as a bitmap-based allocator, fulfilling the requirements of ADR-0035. The implementation includes the Pmm struct in the kernel crate, which handles frame allocation, deallocation, and zero-initialization, and is integrated into the bsp-qemu-virt boot sequence. Review feedback identifies a potential bug where overlapping reserved ranges could lead to incorrect frame counts, concerns regarding stack overflow when initializing large bitmaps, and the architectural debt introduced by assuming identity mapping when zeroing frames.
Comment on lines
+215
to
+219
| for off in 0..frames_in_range { | ||
| let frame_idx = start_idx.saturating_add(off); | ||
| set_bit(&mut bitmap, frame_idx); | ||
| } | ||
| reserved_count = reserved_count.saturating_add(frames_in_range); |
There was a problem hiding this comment.
The reserved_count is calculated by summing the lengths of all provided reserved ranges. If any of these ranges overlap, the same frames will be counted multiple times, leading to an incorrect reserved_count and consequently an incorrect free_count. While the bitmap itself handles overlaps correctly (bits are set idempotently), the cached counters will become inconsistent with the actual state of the bitmap. This will result in incorrect values being reported by stats() and may cause free_count to saturate prematurely. Consider calculating the count based on whether a bit was actually flipped.
for off in 0..frames_in_range {
let frame_idx = start_idx.saturating_add(off);
if !read_bit(&bitmap, frame_idx) {
set_bit(&mut bitmap, frame_idx);
reserved_count = reserved_count.saturating_add(1);
}
}| } | ||
|
|
||
| // All validations passed; construct + populate. | ||
| let mut bitmap = [0u8; N]; |
There was a problem hiding this comment.
The bitmap array is constructed on the stack in Pmm::new. Since Pmm<N, R> is generic over the bitmap size N, large values of N (e.g., for systems with several GiB of RAM) will lead to significant stack usage. For instance, a 4 GiB RAM extent requires a 128 KiB bitmap, which exceeds the 64 KiB boot stack defined in the BSP. This limits the portability of the PMM to larger memory configurations. Consider an initialization pattern that avoids placing the entire Pmm struct on the stack, such as initializing the static storage in-place or using a separate static for the bitmap data.
| // structurally provable. | ||
| let pa_off = idx.saturating_mul(PAGE_SIZE); | ||
| let pa_usize = self.extent.start.0.saturating_add(pa_off); | ||
| let pa_ptr = pa_usize as *mut u8; |
There was a problem hiding this comment.
The alloc_frame implementation directly casts a physical address (pa_usize) to a raw pointer (*mut u8) for zero-filling. This assumes an identity mapping where physical addresses are valid virtual addresses in the kernel's address space. While this holds true for the current v1 identity layout on QEMU virt, it violates the board-agnostic nature of the kernel crate. Future configurations (like a high-half kernel) will break this assumption. It would be more architectural to abstract this through a HAL-provided translation helper or a zeroing primitive, even if it's a simple identity pass-through for now.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/main.rs`:
- Around line 700-707: The snippet around __stack_top (stack_top_addr,
stack_top_aligned_up, PAGE_SIZE) and the pmm_extent construction
(PhysFrameRange::new, PhysAddr, PMM_EXTENT_START, PMM_EXTENT_END) is not
rustfmt-formatted; run cargo fmt (or rustfmt) on bsp-qemu-virt/src/main.rs to
reformat this hunk so the alignment and spacing match project style and the CI
rustfmt check passes.
- Around line 82-86: PMM_BITMAP_BYTES is truncating when the number of managed
frames isn't divisible by 8, underallocating the bitmap and causing Pmm::new to
fail for valid extents; change the const to compute the frame count first as
(PMM_EXTENT_END - PMM_EXTENT_START) / PAGE_SIZE and then round up to bytes using
ceiling division by 8 (e.g., (frame_count + 7) / 8) so any partial final byte is
allocated.
In `@docs/analysis/tasks/phase-b/T-017-physical-memory-manager.md`:
- Line 5: The document T-017-physical-memory-manager contains stale
pre-adjudication text referencing "UNSAFE-2026-0001 amendment / no new audit
entry" while the final outcome recorded is "UNSAFE-2026-0026" and the status is
"Done"; update all earlier sections (including the Status line and any mentions
in the intro/summary) to a single consistent audit result matching the final
outcome (UNSAFE-2026-0026 new audit entry) so there are no contradictory
statements about audit entry or status.
In `@docs/audits/unsafe-log.md`:
- Line 555: Update the contradictory sentence in the UNSAFE-2026-0026 status so
it clearly states that PMM initialization (the unsafe site) is reached on every
v1 boot but the zero-fill operation inside alloc_frame is not exercised in v1;
reword the line to say the PMM is constructed and exposed in v1 (so the site is
reached) while Pmm::alloc_frame (the zero-fill unsafe operation validated later
by the test Pmm::alloc_frame_returns_first_free_and_zeroes_payload and runtime
B3+ work) is not actually called in v1.
In `@docs/roadmap/current.md`:
- Around line 45-46: The "Current-focus" / "next steps" text still references
the older T-016/B2 closure; update all downstream "next task" and "review
trigger" entries to match the new active milestone and next tasks: ADR-0028 and
T-018 (AddressSpace kernel object + capability-gated Mmu::map wrappers +
activation-on-context-switch), remove or replace any mentions of T-016/B2, and
ensure the "Active milestone" / "Active task" header (B3 / T-017) is the
authoritative source so the document consistently points to ADR-0028 + T-018 as
the next items.
In `@kernel/src/mm/pmm.rs`:
- Around line 154-157: Run rustfmt on the file to fix formatting issues causing
CI failure: format kernel/src/mm/pmm.rs (including the public function new with
signature pub fn new(extent: PhysFrameRange, reserved: &[PhysFrameRange]) ->
Result<Self, PmmError>) by running `cargo fmt` (or your editor's Rust
formatting) so the file conforms to rustfmt rules before merging.
- Around line 185-195: The reserved-range loop currently allows
overlapping/duplicate ranges which double-counts reserved_count and causes
stats() to mismatch the bitmap; update the logic in the reservation path (the
loop over reserved ranges and the code that increments reserved_count) to first
validate and reject overlaps (by checking each incoming range against
previously-accepted ranges or by scanning the bitmap for already-set bits)
before accepting them, or alternatively derive reserved_count from the bitmap
after marking to ensure only unique bits are counted; reference the reserved
slice iteration, the reserved_count field, the bitmap marking code, and callers
like stats() / alloc_frame() when making the change so counting and bitmap stay
consistent.
- Around line 317-338: Extend the unsafe-block safety comment around the
core::ptr::write_bytes call to also state why unsafe is required (we are writing
to a physical address pointer pa_ptr for a 4 KiB frame and cannot obtain a safe
&mut reference/slice to that PA), enumerate the invariants already listed
(pa_ptr page-aligned, frame exclusively owned per bitmap after alloc_frame,
identity-mapped PA==VA in mmu_bootstrap, PAGE_SIZE fits), and explicitly state
why safe alternatives were rejected (cannot use safe slice/ptr APIs because no
valid Rust &mut [u8] or typed frame wrapper exists for this physical address at
this callsite, and higher-level zeroing helpers would either allocate or assume
different ownership/alignment guarantees that we cannot provide here); reference
symbols pa_ptr, PAGE_SIZE, and the allocation path (e.g., Pmm::new /
alloc_frame) so reviewers can locate the site.
🪄 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: ca0a2808-0f3e-4191-8147-646a280dc113
📒 Files selected for processing (11)
bsp-qemu-virt/src/main.rsdocs/analysis/tasks/phase-b/README.mddocs/analysis/tasks/phase-b/T-017-physical-memory-manager.mddocs/architecture/boot.mddocs/architecture/memory-management.mddocs/audits/unsafe-log.mddocs/roadmap/current.mddocs/roadmap/phases/phase-b.mdkernel/src/lib.rskernel/src/mm/mod.rskernel/src/mm/pmm.rs
3-bot review round (sourcery + gemini + coderabbit; qodo rate-
limited). 13 inline findings on the T-017 implementation arc.
Triage: 1 build failure (fmt), 4 Major (1 from 3 bots, 1 from 2,
2 standalone), 6 Minor, 2 Medium-to-defer.
Verified each against current code:
Applied:
1. **CI fmt failure** — `cargo fmt --all` reformatted 9 sites in
bsp-qemu-virt/src/main.rs + kernel/src/mm/pmm.rs where I'd
manually broken lines that the rust-toolchain.toml-pinned
nightly-2026-01-15 rustfmt collapses to single lines (e.g.,
`let x = a.method().method()` style). No semantic change;
pure whitespace.
2. **Overlapping reserved ranges in Pmm::new** (Major; flagged
independently by sourcery + gemini + coderabbit). If two
reserved ranges overlap, the per-range bit-set loop sets the
shared frame's bit once but each entry increments
`reserved_count` separately, leaving the cached counter
inconsistent with the bitmap. `stats().free_frames` could
report 0 while `alloc_frame()` still finds a free frame —
classic counter-drift. Added:
- **PmmError::OverlappingReservedRanges** variant
(`#[non_exhaustive]`, additive).
- **O(R²) pairwise overlap check** in `Pmm::new` after the
in-extent / alignment validations, before any bitmap
mutation. Two half-open ranges [a, b) and [c, d) overlap
iff `a < d && c < b`. For R ≤ 8 (v1), ≤ 28 comparisons —
trivially cheap.
- **`new_rejects_overlapping_reserved_ranges`** host test
covers three scenarios: classic-overlap (rejected),
duplicate-range (rejected), touching-but-not-overlapping
half-open ranges `[a, b) + [b, c)` (correctly NOT rejected
— boundary semantics).
3. **stats().total_frames anchored against extent** (sourcery
Medium). Previously derived as `free_count + reserved_count +
allocated_count`; if any sub-counter drifts, the total
silently re-establishes internal consistency rather than
surfacing the drift. Now derived from `self.extent.frame_count()`
directly so counter bugs become stats-vs-bitmap mismatches
the existing `stats_parity_with_bitmap_bit_count` test
catches.
4. **PMM_BITMAP_BYTES round-up** (coderabbit Minor). For QEMU
virt's 32 768 frames the old `/8` and new `.div_ceil(8)`
produce the same 4 096 bytes (no v1 bug). Forward-defensive
for future BSPs with non-multiple-of-8 frame counts where the
floor form would underallocate the last bitmap byte. Used
stable `usize::div_ceil` directly per clippy::manual_div_ceil
suggestion.
5. **alloc_frame SAFETY block — inline rejected-alternatives**
(coderabbit Major). Per CLAUDE.md unsafe-policy.md §1: every
unsafe block must state (a) why unsafe is needed, (b)
invariants upheld, (c) why safer alternatives were rejected.
The previous SAFETY comment had (a) + (b) but punted (c) to
the audit-log entry. Now inlines a four-alternative summary
(slice-from-raw-parts-mut + lazy-zero-on-page-fault +
skip-zero + volatile_set_memory) with one-line rejection
reasons each. The fifth alternative (Mmu::zero_frame trait
method) stays in UNSAFE-2026-0026's full Rejected-alternatives
discussion to keep the inline block bounded; the SAFETY
comment names the audit entry for the full record.
6. **T-017 stale UNSAFE outcome text** (coderabbit Minor).
Lines 20 / 87 / 88 / 124 / 134 still claimed
"UNSAFE-2026-0001 Amendment / no new audit entry" — pre-
adjudication wording from the ADR-0035 §Dependency chain
step 5's deferred-verdict era. Now uniformly "UNSAFE-2026-0026
(new audit-log entry per the adjudication-deferred verdict)"
with the same Rejected-alternatives summary the new entry
carries.
7. **UNSAFE-2026-0026 contradictory verification sentence**
(coderabbit Minor). Status line said both "v1's demo never
calls `alloc_frame`" AND "the site is reached on every boot".
The two refer to different "sites": Pmm::new construction
(reached every boot, prints the banner) vs. the
`core::ptr::write_bytes` zero-fill block (the actual unsafe
site, runtime-unexercised because no v1 caller hits
`alloc_frame`). Reworded to make the construction-vs-zero-
fill distinction explicit per coderabbit's suggestion.
8. **current.md downstream "next steps" sections** (coderabbit
Minor). Lines 78-79 ("Next task to open: T-016 implementation"
+ "Next review trigger: B2 closure trio") were stale —
T-016 closed two PRs ago, B2 closed too. Updated to point
at ADR-0028 + T-018 (next task) + B3 milestone closure trio
(next review trigger) with the correct forward-flag (T-018
will be UNSAFE-2026-0026's first runtime exercise).
Skipped with reason:
A. **`bitmap` array on the function stack in `Pmm::new`**
(gemini Medium). `let mut bitmap = [0u8; N]` creates a stack
frame proportional to `N`. For v1 N = 4096 → trivially fits
in the 64 KiB boot stack; for future Pi-4-class BSPs with
N = 131072 (4 GiB RAM → 128 KiB bitmap), this overflows the
v1 stack. The fix is an in-place `Pmm::init_in(slot: &mut
MaybeUninit<Self>, ...)` API that writes through the
StaticCell directly rather than via a stack-allocated value.
That's a larger API change; v1 doesn't need it and the cost
of bundling it here is non-trivial. Tracked as a known
future-BSP concern in T-017's review-history; opens with the
first BSP whose `N` exceeds the boot stack (likely the
Pi-4 BSP arc).
B. **PA → kernel pointer cast in alloc_frame** (gemini Medium).
The `(extent.start.0 + idx * PAGE_SIZE) as *mut u8` cast
relies on the identity-mapped v1 layout (ADR-0027). The
SAFETY block + UNSAFE-2026-0026's invariant (3) already
document this explicitly. The high-half migration
(ADR-0033 placeholder) will need a `phys_to_virt` helper at
this exact site — the placeholder ADR's §Simulation table
will name it as the first migration step. No code change in
T-017's scope; the identity-only assumption is the v1
design.
Test count rises 15 → 16 (new `new_rejects_overlapping_reserved_ranges`
pinning sourcery+gemini+coderabbit's #1 finding). Workspace
total 199 → 200 host tests.
Verification:
- cargo fmt --check clean (CI ran with
nightly-2026-01-15;
matches local now)
- cargo clippy (host) clean -D warnings
- cargo kernel-clippy clean -D warnings (used
`usize::div_ceil` per
clippy::manual_div_ceil)
- cargo test 200/200 host (was 199; +1
overlap test)
- cargo +nightly miri test 15/15 PMM tests clean
(Stacked Borrows clean
against the unchanged
zero-fill path)
- cargo kernel-build clean
- QEMU smoke stable; new banner reads
"tyrne: pmm initialized
(32603 frames available;
165 reserved)" — the +1
reserved-frame delta vs.
prior commit (32604 + 164)
is the kernel-image-section
growth from this commit's
code additions rounding up
one more frame at
__stack_top_aligned
Refs: ADR-0035, T-017
Audit: UNSAFE-2026-0026
…erred)
External "Approve with nits" review on the T-017 implementation
arc. 4 actionable findings (3 Minor + 1 Nit) + 1 preexisting
out-of-scope drift. Triage:
Applied:
1. **T-017 §DoD stale UNSAFE-2026-0001 Amendment references**
(Minor; Axis 12.1). Four sites still carried pre-adjudication
"UNSAFE-2026-0001 Amendment / no new audit entry" wording
that contradicted §Acceptance criteria + §Approach commit 2
+ Review history + the actual UNSAFE-2026-0026 entry:
- §DoD line 135: "UNSAFE-2026-0001 gains the PMM frame-
zeroing Amendment" → "UNSAFE-2026-0026 introduced as a
new entry (NOT a UNSAFE-2026-0001 Amendment) per ADR-0035
§Dependency-chain step 5's adjudication-deferred verdict".
- §DoD line 136: "No new audit-log entry — confirms ADR-0035's
'zero new audit entries' promise byte-for-byte" → "One new
audit-log entry (UNSAFE-2026-0026) — ADR-0035 §Positive
consequence 'Zero new unsafe audits' flips to 'one new
entry' per the same adjudication caveat".
- §DoD line 141 commit-trailer guidance:
"Audit: UNSAFE-2026-0001 trailer where applicable" →
"Audit: UNSAFE-2026-0026 trailer for commits that introduce
or extend the PMM frame-zeroing site".
- §References line 161: "UNSAFE-2026-0001 gains the PMM
frame-zeroing Amendment" → "UNSAFE-2026-0026 covers the
PMM frame-zeroing site (new entry per the §Dependency-
chain-step-5 adjudication)".
Same-file internal contradiction; a future reader walking
T-017 top-to-bottom can now trace the adjudication outcome
without cross-referencing the audit log.
2. **`Pmm::new` doc-comment "three" → "five" validations**
(Minor; Axis 12.2). The doc-comment listed three checks
(extent-aligned, ranges-in-extent, list-fits-in-R) but the
code has five distinct validation steps. Missing entries
in the doc:
- Bitmap-size check (`total_frames > N * 8` → OutOfRange).
- Per-range alignment check (`!range.is_aligned()` →
MisalignedAddress).
- Per-range non-inverted check (`range.end < range.start`
→ OutOfRange).
- Pairwise-overlap check (→ OverlappingReservedRanges, the
round-1 addition).
Replaced the three-item enumeration with the full five-item
list. Same "three fail-fast validations" drift fixed in
memory-management.md §"API".
3. **`extent_16f()` → `extent_4f()` test fixture rename**
(Nit; Axis 13.1). The fixture was named "16 frames" and
doc-commented "16 frames (16 KiB)" but the actual range
`PhysAddr(0x4000_0000)..PhysAddr(0x4000_4000)` is 16 KiB
= 4 frames (at 4 KiB/frame). The sanity-check test asserts
`frame_count() == 4` (matching the code, contradicting the
name). Rename + corrected doc-comment ("4 frames (16 KiB)";
"Bitmap holds 4 bits → 1 byte (rounded up); N = 2 is the
smallest non-trivial PMM with headroom for the sanity
check"). Sanity test renamed `extent_16f_fixture_sanity` →
`extent_4f_fixture_sanity`. Pure cosmetic; no test logic
changes; fixture is referenced only by its own sanity test
(every other test uses `pmm_over_backing` directly).
4. **`alloc_frame` doc-comment on the unreachable-leak path**
(Minor; Axis 3.1). The previous doc-comment claimed
"we keep the path total via `?` chaining on an Option to
avoid an unwrap" — but there is no `?` chain in the
actual code; `PhysFrame::from_aligned(...)` returns the
Option directly. A future maintainer reading the
doc-comment would expect a rollback path that doesn't
exist.
Replaced with an honest description: (a) why `from_aligned`
is provably-Some by construction (Pmm::new validation (i)
+ idx * PAGE_SIZE alignment preservation), (b) the
unreachable-leak caveat (bitmap mutation happens BEFORE
the call, so if a future change ever weakens the alignment
proof and `from_aligned` returns None, the frame is
permanently leaked), (c) explicit guidance for a future
maintainer who alters Pmm::new's validation set ("either
preserve the alignment proof or move the mutation block
below this call"). Doc-only fix; the structural unreachability
in v1 makes the leak path inert today; the smaller doc fix
is defensible vs. the 4-line refactor (move mutation
post-from_aligned) because the refactor would add a
rollback-on-None branch for a structurally impossible case.
Skipped with reason:
A. **BSP `Audit: UNSAFE-2026-0001` tag drift on PMM StaticCell
writes at main.rs:706, 722, 729** (Nit; Axis 9.1; preexisting,
out-of-scope). The new PMM-related unsafe blocks cite
UNSAFE-2026-0001, but UNSAFE-2026-0001's actual scope is
`Pl011Uart::new(PL011_UART_BASE)` (typed MMIO wrapper
construction); the StaticCell unsafe impl Sync pattern is
UNSAFE-2026-0010. The drift is preexisting (other StaticCell
writes for CONSOLE / CPU already cite UNSAFE-2026-0001
incorrectly); this PR perpetuates but doesn't introduce the
pattern. Worth a separate hygiene PR that retags the
StaticCell-pattern unsafe blocks across main.rs to
UNSAFE-2026-0010 — not a T-017 remediation. Queued.
Verification:
- cargo fmt --check clean
- cargo host-clippy clean -D warnings
- cargo kernel-clippy clean -D warnings
- cargo test 200/200 host (unchanged;
only test name renamed)
- cargo kernel-build clean
Refs: ADR-0035, T-017
Audit: UNSAFE-2026-0026
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…mmError to round-2 reality Round-3 review (parallel reviewer 1) flagged §Acceptance criteria line 41 still says "three fail-fast validations" — the only remaining drift inside T-017 after the round-2 fixup. Walking the file once more surfaced three sibling drifts in the same file, all from the same root cause (pre-adjudication / pre- overlap-check wording that the round-2 fixup didn't sweep because it focused on §DoD + §References): 1. **§Acceptance criteria line 41** (`Pmm::new` constructor description) — "three fail-fast validations" → "five fail-fast validations" with the full (i)..(v) enumeration matching the doc-comment at pmm.rs:136-165: extent-aligned, bitmap-size, R-cap, per-range (alignment + bounds + non- inverted), pairwise-overlap. Each validation now names its error variant and the round-1-PR-#26 provenance for the overlap gate. 2. **§Acceptance criteria line 45** (`PmmError` enum description) — added `OverlappingReservedRanges` variant to the documented variant list with its trigger condition ("two reserved ranges share at least one frame, which would cause bitmap-vs-counter drift"). Also extended `OutOfRange`'s description to cover the bitmap-size-exceeds-capacity condition and the reserved-range-out-of-extent / inverted-range conditions, mirroring how Pmm::new actually uses the variant. 3. **§Approach line 123** (commit 1 description) — "three fail-fast validations (extent-alignment, reserved-in-extent, R-cap)" → "five fail-fast validations (extent-alignment, bitmap-size, R-cap, per-range {alignment + bounds + non- inverted}, pairwise-overlap — the last gate added in PR #26 round-1 review)". Also added `new_rejects_overlapping_reserved_ranges` to the constructor-commit host test list (the test exists at pmm.rs:613 but the §Approach narrative didn't yet name it). 4. **§Background line 22 + §Design notes line 146** — "audit- log Amendment" → "new audit-log entry (UNSAFE-2026-0026, per the §Dependency-chain-step-5 adjudication-deferred verdict)". Same pre-adjudication wording the round-2 fixup purged from §DoD + §References; these two §Background / §Design-notes sites slipped through that sweep. All four mentions matching `grep -n "Amendment"` that remain post-this-commit are legitimate negation-form phrasings ("NOT a UNSAFE-2026-0001 Amendment", "not a UNSAFE-2026-0001 Amendment") in §Acceptance criteria line 87, §DoD line 135, and Review history line 175 — all correctly framing the adjudication outcome rather than asserting an Amendment. Verification: - grep -n "three\|Amendment\|zero new audit" docs/analysis/tasks/phase-b/T-017-physical-memory-manager.md returns only the four legitimate negation-form lines (87, 135, 146 [audit-log entry], 175 [Review history audit trail]); no remaining "three fail-fast validations" hits. - cargo fmt --check clean (doc-only change) - cargo host-clippy clean -D warnings - cargo test 200/200 host (unchanged — no code touched) Refs: ADR-0035, T-017 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 11, 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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements T-017 (B3 §1) per ADR-0035: bitmap-based Physical Memory Manager over QEMU virt's 128 MiB RAM extent, with reservation tracking,
FrameProviderimpl, and BSP boot-time wiring. The PMM is the prerequisite layer below B3's address-space abstraction (ADR-0028 + T-018, next); v1's cooperative IPC demo never callsalloc_frame, but the PMM is live and verified at every boot via the new `tyrne: pmm initialized (32604 frames available; 164 reserved)` banner line.Five commits (four bisectable + one docs polish)
`feat(kernel): T-017 commit 1 — Pmm struct + Pmm::new (kernel/src/mm/)` (2533732)
New `kernel/src/mm/` subsystem parent + `Pmm<const N: usize, const R: usize>` struct shape + `Pmm::new(extent, reserved) -> Result<Self, PmmError>` with three fail-fast pre-mutation validations. 5 host tests. No `unsafe`. 190/190 host.
`feat(kernel): T-017 commit 2 — alloc_frame / free_frame / stats + UNSAFE-2026-0026` (6500337)
Runtime alloc / free / stats path. Frame-zeroing via `core::ptr::write_bytes` (the only `unsafe` in the PMM body — UNSAFE-2026-0026 lands as a new audit-log entry per ADR-0035 §Dependency chain step 5's adjudication-deferred verdict). 8 new host tests. 198/198 host + 13/13 Miri (Stacked Borrows clean).
`feat(kernel): T-017 commit 3 — impl FrameProvider for Pmm` (fdb52d6)
Trivial trait-impl forwarding to `Pmm::alloc_frame`. 1 new host test pinning the `&mut dyn FrameProvider` integration. 199/199 host + 14/14 Miri.
`feat(bsp-qemu-virt): T-017 commit 4 — PMM wiring + boot banner + smoke` (a2ffcb6)
`bsp-qemu-virt/src/main.rs`: PMM `StaticCell` + extern `__stack_top` linker symbol + `kernel_entry` wiring between mmu-activated print and GIC init. The kernel_entry order is now: hello → VBAR_EL1 install → boot_ns snapshot → mmu_bootstrap → "mmu activated" → PMM init → "pmm initialized (...)" → GIC init + DAIF unmask → timer banner → demo. Smoke verified.
`docs: T-017 Done — PMM live; B3 §1 closed; ADR-0028 next` (6ed8a70)
Status flips + cross-references: memory-management.md §"Frame allocation discipline" rewritten for post-T-017 reality; boot.md Stage 3 + sequence diagram extended; roadmap current.md + phase-b.md + tasks/phase-b/README.md flipped to Done; T-017 task review-history gains the closure row with the UNSAFE-2026-0026 adjudication trail.
Headline numbers
§Simulation table verification
All 5 rows of ADR-0035 §Simulation are pinned by host tests:
Plus `alloc_frame_implements_frame_provider` (trait-impl integration) and `extent_16f_fixture_sanity` (test fixture sanity check) bring the total to 14 PMM tests.
Audit-log adjudication
ADR-0035 §Dependency chain step 5 explicitly left the Amendment-vs-new-entry call to T-017's security review. The verdict (recorded in UNSAFE-2026-0026's §Introduced field): new entry, not an Amendment of UNSAFE-2026-0001. Reasoning walked in T-017's review-history closure row:
The two share surface shape ("kernel-static / identity-mapped raw-pointer") but differ on what they touch, what proves ownership, and what the failure mode is. Stretching the UNSAFE-2026-0001 umbrella over both would violate unsafe-policy.md §3's "same safety argument" requirement. The new entry's Rejected-alternatives section walks five alternatives (slice-from-raw-parts, lazy zero on page-fault, skip-zero, volatile_set_memory, new Mmu::zero_frame trait method) and explains why `write_bytes` is the honest choice.
ADR-0035 §Positive consequence "Zero new `unsafe` audits" flips to "one new entry" per the same caveat; ADR-0035 itself stays byte-stable.
Test plan
What's deferred
Per T-017 §Out of scope:
Next task: ADR-0028 (Address-space data structure) + T-018 (`AddressSpace` kernel object + capability-gated `Mmu::map` wrappers + activation-on-context-switch) per phase-b.md §B3 items 2–7.
Refs: ADR-0035, T-017
Audit: UNSAFE-2026-0026
Summary by Sourcery
Introduce a bitmap-based physical memory manager and wire it into the QEMU virt BSP boot sequence as the system-wide frame allocator foundation for future address-space work.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
Release Notes
New Features
Documentation