docs: B2 closure-trio — milestone Closed 2026-05-09#24
Merged
Conversation
Phase B / Milestone B2 ("MMU activation, kernel-half mapping") flips
implementation-complete → Closed via the canonical three-review
closure trio. Pattern follows the B1 precedent (2026-05-07).
Lands:
- docs/analysis/reviews/business-reviews/2026-05-09-B2-closure.md
What landed (T-016 Done; ADR-0027 Accepted; ADR-0033/ADR-0034
placeholders reserved; PR #18 / #22 / #23 merged); audit-log
surface (UNSAFE-2026-0022/0023/0024/0025 + Amendments); test
counts (185/185 host + miri); smoke trace verbatim. What
changed in the plan: B2 closed cleanly on first attempt
(no smoke-regression rollback unlike B1) — the §Simulation
discipline did its job. What we learned: ADR-0027's table
walked the worst-case interaction; T-016's review-rounds
surfaced compiler-side concerns but zero descriptor /
encoding bugs at the architecture level. Adjustments queue
(PMM bring-up next; PL011 init follow-up; MmuError::BlockMapped
on first block-split caller; codify "simulation row → host
test" mapping discipline). Next: B3, name TBD via first ADR.
- docs/analysis/reviews/security-reviews/2026-05-09-B2-closure.md
Eight-axis adversarial pass. Verdict Approve. Key items:
capability surface unchanged (no new userspace-reachable
operation); MMU adds no new trust boundary in v1 (identity
layout = transparent) but mediates every memory access
architecturally; four new audit entries land cleanly under
unsafe-policy.md §3 discipline; MapperFlush #[must_use]
promotes "forgot to flush" from reviewer-attention to compile
error; threat-model unchanged (v1 attack surface empty;
ADR-0033/0034 placeholders deferred to B5+). Carry-forward:
-1 (ipc_recv_and_yield Deadlock asymmetry closed by T-015),
+1 (UNSAFE-2026-0025 per-call Mmu::map smoke verification
pending B3+ caller).
- docs/analysis/reviews/performance-optimization-reviews/2026-05-09-B2-closure.md
Release ELF: .text 22,384 (+364 from MMU surface) /
.rodata 2,944 (+16) / .bss 40,208 (+17,952; dominantly the
16 KiB .boot_pt reservation + 4 KiB alignment slack —
explicit per ADR-0027 §"Bounded bootstrap frame budget").
Source LOC ~11,290 (+1,010). Test count 185/185 host +
miri clean. Release-build perf-harness band (the first
release-codegen baseline-of-record): p10/p50/p90 = 4.262 /
4.642 / 6.456 ms across 20 iterations; min 4.164, max 7.903,
mean 5.110, stddev 1.016 ms. Single-run smoke 4.891 ms within
band. Debug-build Δ pre/post-T-016: +1.5 ms p10/p50 (QEMU TCG
translation-cache overhead at MMU bring-up; real-hardware
activation sub-100 µs per ADR-0027 §Consequences). No new
hot path (Mmu::map / unmap dead-code-allow in v1). Verdict
Merge — re-baseline complete; no proposal this cycle.
- docs/analysis/reports/perf-baseline-2026-05-09-B2-closure.md
20-iteration release-build harness output emitted by
tools/perf-harness.sh; the canonical reference for B3+
regression checks against B2's closing release-build perf.
Roadmap updates:
- docs/roadmap/current.md
New 2026-05-09 banner under the prior 2026-05-08 banner
(chronological; both preserved per the append-only
discipline). Active-milestone line flips B2 → B3 (next; name
TBD via first B3 ADR). Active-task line: none (T-016 Done +
closed); next opens with first B3 ADR's Dependency chain
section per ADR-0025 §Rule 1.
- docs/roadmap/phases/phase-b.md
B2 §Status block flips "implementation-complete" → "Closed
2026-05-09" with cross-references to the three closure-trio
artefacts. Original status text preserved as historical
record (per the same append-only discipline B0 / B1 followed).
Verification:
- cargo fmt --check clean
- cargo clippy (host) clean -D warnings
- cargo kernel-clippy clean -D warnings
- cargo test 185/185 host (unchanged; this is a
docs-only commit — no code touched)
- cargo +nightly miri test 185/185 clean
- cargo kernel-build clean
Refs: ADR-0027, T-016
ⓘ 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[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
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 (5)
📝 WalkthroughWalkthroughThis PR records the completion of the B2 (MMU activation) milestone with five documentation artifacts: a performance baseline report capturing boot-to-end timing metrics, a comprehensive security review covering eight audit axes and four new unsafe entries, a detailed performance analysis with kernel image size and timing deltas, a business retrospective summarizing what landed and key learnings, and roadmap updates closing B2 and transitioning to the B3 milestone. ChangesB2 Milestone Closure Documentation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 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 |
![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 documents the closure of the B2 milestone (MMU activation) by adding comprehensive business, security, and performance reviews, and updating the project roadmap for the B3 milestone. Review feedback recommends simplifying the test count breakdown in the performance report for clarity and ensuring the description of the kernel entry boot sequence is consistent across all closure documents.
|
|
||
| T-016's source diff: ~590 LOC HAL (`vmsav8.rs` encoders + `MapperFlush` newtype + trait return-type changes + 12 new encoder tests + 6 new MapperFlush tests + 3 contract-parity tests). ~600 LOC BSP (`mmu.rs` walker + `mmu_bootstrap.rs` activation routine + `linker.ld` extension + `main.rs` wiring). ~140 LOC test-hal (FakeMmu return-type updates + 6 + 3 new tests). ~580 LOC docs (ADR-0027 + memory-management.md + audit-log entries + closure-trio docs). Net ~1,910 LOC of *added work*; net source growth ~1,010 LOC reflects the doc / test ratio (docs make up ~50 % of the touched surface but live outside the LOC count's `*.rs` / `*.s` filter). | ||
|
|
||
| **Test count:** 185 / 185 (was 152 at B1 closure; pre-T-016 baseline of 159 included +7 from T-015's `cancel_recv` tests + PR-#18 coverage gap closure; post-T-016 adds +18 from T-016's 12 vmsav8 + 6 MapperFlush; +5 mid-arc from PR #22's harness host-test inheritance — actually mostly from `tyrne-hal` tests +12 = 30→42; +3 round-2 contract-parity tests in test-hal; the precise breakdown is in the [business retrospective §Test counts](../business-reviews/2026-05-09-B2-closure.md#test-counts-at-b2-closure)). All pass under `cargo +nightly miri test` 185 / 185 clean (~25 s wall-clock). |
There was a problem hiding this comment.
The explanation for the test count increase from 152 to 185 is difficult to follow. For clarity, consider replacing the detailed breakdown with a simpler summary by crate, similar to how it's presented in the business retrospective. For example:
"The +33 test increase breaks down as: +7 in tyrne-kernel, +17 in tyrne-hal, and +9 in tyrne-test-hal."
| @@ -0,0 +1,115 @@ | |||
| # Security review 2026-05-09 — B2 closure consolidated pass (post-T-016) | |||
|
|
|||
| - **Change:** all committed code on `main` from `8dc433e` (PR #17 merge — T-015's `ipc_cancel_recv` recovery primitive, the last Phase-B-only commit covered by [`2026-05-07-B1-closure.md`](2026-05-07-B1-closure.md) §Post-T-015 amendment) through `b0035ce` (PR #23 merge — T-016 with all three review-rounds applied). The arc covers ADR-0027 (kernel virtual memory layout — `Accepted` 2026-05-08); the `MapperFlush` typed flush-token discipline at the `Mmu` HAL trait surface; the pure VMSAv8 descriptor encoders in `tyrne_hal::mmu::vmsav8`; the `bsp-qemu-virt::QemuVirtMmu` impl; the `mmu_bootstrap` boot-time activation routine; the `.boot_pt` linker reservation; the `kernel_entry` re-ordering (vector install → `boot_ns` snapshot → `mmu_bootstrap` → `tyrne: mmu activated` → GIC init → DAIF unmask → timer banner); the four new audit-log entries (UNSAFE-2026-0022 / 0023 / 0024 / 0025) with their bootstrap-Amendments and 2026-05-09 smoke-verification Amendments; and the doc-only / hygiene PRs #18 + #22's path-drift / banner / link sweeps. | |||
There was a problem hiding this comment.
The description of the kernel_entry re-ordering in the "Change" section appears incomplete. It omits the "print" for the "mmu activated" message and the final "demo" step. For consistency with other documents in this pull request, please consider updating it to match the more complete description from the business retrospective: (vector install → boot_ns snapshot → mmu_bootstrap → tyrne: mmu activated print → GIC init → DAIF unmask → timer banner → demo).
Audit pass on the 2026-05-09 B2 closure-trio docs surfaced three open / inaccurate items. All closed in this commit; no findings remain unaddressed. Closed: 1. **Track J §J-OBS1 — lift `#![deny(clippy::todo)]` to workspace lints** (B1 closure-trio §Adjustments item, still open as of 2026-05-09 audit). The `clippy::todo` macro lint catches `todo!()` macro calls — never appropriate in checked-in code, regardless of crate. Original placement was kernel-local alongside `clippy::panic` / `unwrap_used` / `expect_used` (which ARE crate-specific because `tyrne-test-hal` legitimately panics in its host-test fakes). `clippy::todo` doesn't share that constraint and was kernel-local by inertia rather than design. Lifted to `[workspace.lints.clippy] todo = "deny"` in `Cargo.toml`; trimmed from `kernel/src/lib.rs` with a header comment recording the closure trail. The lint now applies to every workspace crate. Verified by `cargo clippy --all-targets -- -D warnings` clean. 2. **Test count miscount in B2 business retro** — claimed "+12 vmsav8 + 5 unrelated existing growth" for the hal crate delta. Empirical re-verification: `git checkout 6494ed2 --` (the pre-T-016 main tip) and `cargo test -p tyrne-hal` returns 25 (all in `timer.rs`); current HEAD returns 42; Δ = +17, ALL in `vmsav8.rs`. There is no "unrelated growth" — the prior `mmu.rs` had zero tests (`from_aligned`, `addr`, etc. are pub fns, not test fns), and the new `mmu/vmsav8.rs` module added 17 tests at T-016 implementation time. Corrected the breakdown in both the business retro §"Test counts at B2 closure" and the perf baseline §"Source LOC" → §"Test count" sections, with the empirical verification cited inline so future readers see the closure-trail on this miscount. 3. **B1 closure §Adjustments closure-status audit** — the B2 business retro listed new adjustments but did not audit the prior B1 list. Re-read the [B1 §Adjustments](../../../docs/analysis/reviews/business-reviews/2026-05-07-B1-closure.md#adjustments) list (7 items) and added a "B1 closure §Adjustments — closure status" subsection to the B2 business retro. Of 7 items: 6 closed, 1 trigger-deferred (Track K — Live execution to comprehensive-review template, gates on next maintainer- initiated full-tree review). Item 7 (clippy::todo lift) is closed by item 1 of this commit. Verification: - cargo fmt --check clean - cargo clippy (host) clean -D warnings (incl. new workspace `todo = "deny"`) - cargo kernel-clippy clean -D warnings - cargo test 185/185 host (unchanged — only lint surface + doc text edits) - cargo kernel-build clean This commit completes the 2026-05-09 B2 closure-trio cleanup; no further open items from the prior B1 → B2 arc. Refs: ADR-0027, T-016
PR #24 review-round (gemini-code-assist; sourcery / coderabbit / qodo all rate-limited so no substantive feedback). Two medium- priority findings, both stylistic / consistency. Applied: 1. **Perf baseline §"Test count" — simpler per-crate breakdown.** The post-cleanup text still embedded the +33 delta inside a parenthetical arithmetic chain that read densely. Gemini suggested the business retro's per-crate sentence form. Adopted that shape: "+7 in tyrne-kernel, +17 in tyrne-hal, +9 in tyrne-test-hal". The audit-correction note (the closure-trio re-read finding that the prior +12+5 split was a miscount) stays; the headline count is now scannable. 2. **Security review §"Change" — kernel_entry sequence consistency.** The boot-sequence list omitted "print" after `tyrne: mmu activated` and the trailing `→ demo` step that the business retro carries. Aligned both: now reads "vector install → boot_ns snapshot → mmu_bootstrap → tyrne: mmu activated print → GIC init → DAIF unmask → timer banner → demo" — byte-stable with the [business retro §"Smoke trace"](../business-reviews/2026-05-09-B2-closure.md) and the [boot.md §Stage 3 sequence diagram](../../../architecture/boot.md). Verification: - cargo fmt --check clean - cargo test 185/185 host (unchanged — doc-only) This is the final closure-trio cleanup. PR #24 ready to merge. Refs: ADR-0027, T-016
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
Phase B / Milestone B2 ("MMU activation, kernel-half mapping") flips implementation-complete → Closed via the canonical three-review closure trio. Pattern follows the 2026-05-07 B1 precedent.
Three closure artefacts
Plus the canonical perf-harness output:
docs/analysis/reports/perf-baseline-2026-05-09-B2-closure.md.Headline numbers
.text.rodata.bss.boot_ptreservation)What's next
Mmu::map/unmapare live butFrameProviderhas no real backing in v1.Test plan
cargo fmt --all -- --check— cleancargo host-clippy(`-D warnings`) — cleancargo kernel-clippy(`-D warnings`) — cleancargo host-test— 185/185 passcargo +nightly miri test— 185/185 cleancargo kernel-build— cleantools/perf-harness.sh --release --iterations=20 --report=2026-05-09-B2-closure— 20/20 valid; band recordedRefs: ADR-0027, T-016
Summary by CodeRabbit