docs: B3 milestone closure trio (business + security + performance)#29
Conversation
PR #28 merged T-018 (`AddressSpace` kernel object + cap-gated `Mmu::map`/ `unmap` wrappers + activation-on-context-switch) and T-017 (PMM) into `main`. B3 milestone reaches implementation-complete. - **New top dated update block (2026-05-14)** records the merge, total review-round count (5), the 226 host-test workspace-wide total, two parallel side-effects (`MmuError::BlockMapped` variant + `CapabilityTable::depth_of` `pub(crate)` preflight helper + `.claude/skills/` → `.agents/skills/` migration), and points forward to the closure trio. - **"Last completed tasks"** bullet rewritten to lead with T-018 (Done 2026-05-11, live 2026-05-14 via PR #28), back-fill T-017 (Done 2026-05-10, was missing entirely), then T-016 / T-015 / earlier history preserved. - **"Next task to open"** flipped from "T-018" (now Done) to "none — B3 closure trio next; B4 starts after the trio". - **"Next review trigger"** updated to name the closure-trio artefacts and their target paths under `docs/analysis/reviews/<type>-reviews/2026-05-14-B3-closure.md`. - **Footer (L90)** stale `.claude/skills/...` skill links updated to `.agents/skills/...` to match the migration. Historical dated blockquotes (2026-05-07 / -08 / -09 updates) retain their original `.claude/skills/` references — those are point-in-time records describing repo state at the time of the update and are not rewritten retroactively (same discipline as the historical review snapshots). No code changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
B3 milestone ("Address space abstraction") closed via T-018's PR #28
merge into `main` on 2026-05-14. This commit lands the business
retrospective per `conduct-review`'s `business` master plan + the
release-build perf-harness baseline at HEAD `6334881` (closure-trio
working state; functionally identical to `47b0a86` on main).
**Business retrospective**
(`docs/analysis/reviews/business-reviews/2026-05-14-B3-closure.md`)
covers the 2026-05-09 → 2026-05-14 period spanning 4 merged PRs
(ADR-0035 design, T-017 PMM, ADR-0028 design, T-018 AddressSpace
+ cap-gated wrappers + activation hook). Five-section shape:
- *What landed:* 2 tasks Done (T-017, T-018), 2 ADRs Accepted
(0035, 0028), 4 PRs (#25–#28), 1 new audit entry (UNSAFE-2026-0026
PMM frame-zeroing), UNSAFE-2026-0014 5th Amendment scope-extends
to activation hook + BSP closure, 226 host tests workspace-wide
(+41 vs B2 closure), verbatim QEMU smoke trace, 526 `-d guest_errors`
events (100 % pre-existing PL011-disabled-UART noise, zero
non-PL011 events), perf-harness band (release, 20 iter: p10
10.311 / p50 11.884 / p90 13.823 ms — Δ vs B2 ≈ +6/+7/+7 ms
capturing PMM scan + AS arena publish cost amplified under QEMU
TCG; real-hardware boot unaffected).
- *What changed in the plan:* B3 closed cleanly on first attempt
(second consecutive milestone with no re-open arc — B1's smoke-
regression arc remains the lone counter-example); B3 named
implicitly via ADR-0028's title; two cross-cutting kernel surface
additions (`MmuError::BlockMapped`, `CapabilityTable::depth_of`)
landed during T-018's review-round arc; `.claude/skills/` →
`.agents/skills/` migration completed in parallel.
- *What we learned:* (i) PR #28's five-round review arc is unusual
but legitimate when each round catches a different axis — pattern
recorded for future PR-budgeting; (ii) ADR-0028 row 0 (bootstrap-
AS wrap special case) is the third data point confirming the
"non-obvious row 0 catches load-bearing setup before downstream
rows" §Simulation pattern; (iii) the leak-path-closure discipline
matured — every fallible preflight runs before PMM commitment;
(iv) bot-review signal calibration unchanged from B1+B2 (~95 %
factual, ~50 % architectural apply rate); (v) trust CodeRabbit's
finding direction more than its prescription (the `.agents/skills/`
migration's path-detail was wrong even though the drift was real).
- *B2 closure adjustments status:* 2 of 6 closed (B3 PMM bring-up
via T-017, `MmuError::BlockMapped` via `8b9f52e`), 4 trigger-
deferred (PL011 init, §Simulation→test codification, ADR-0033 +
ADR-0034 placeholders).
- *Adjustments:* 7 forward items, headlined by B4 (initial userspace
image format) as the next milestone and the re-triggered
§Simulation→test codification (trigger renamed to "B4's first
task" to break the recurring deferral).
- *Next:* Active phase B; Active milestone B4; Active task none;
Next review trigger B4 closure trio.
**Perf-baseline report**
(`docs/analysis/reports/perf-baseline-2026-05-14-B3-closure.md`)
generated by `tools/perf-harness.sh --iterations=20 --timeout=5
--release --report=B3-closure` at HEAD `6334881`. Baseline-of-record
for B4+ regression checks against B3's closing release-build
performance.
**README index** updated with two missing rows: B2 closure
(2026-05-09; the row was inadvertently omitted when the B2 trio
landed via PR #24) and the new B3 row.
No code changes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
B3 milestone security pass per `perform-security-review` skill's eight axes. Standalone artefact after the five-round PR #28 review arc; fresh agent re-walks adversarially without continuing the prior round's findings. **Scope:** post-`b0035ce` (B2 closure baseline) → `47b0a86` (PR #28 merge). Covers T-017 PMM, T-018 `AddressSpace` kernel object + cap-gated wrappers + activation-on-context-switch, ADR-0035 + ADR-0028, the `CapKind::AddressSpace` + `CapObject::AddressSpace` cap-variant additions, the `CapabilityTable::depth_of` `pub(crate)` preflight helper, the `MmuError::BlockMapped` additive variant, UNSAFE-2026-0026 (new), UNSAFE-2026-0014's 5th Amendment (activation-hook scope extension), UNSAFE-2026-0025's body-correction Amendment, and the `.claude/skills/` → `.agents/skills/` migration. **Verdict:** Approve. All eight axes pass. Key adversarial findings: - *Capability correctness:* `cap_create_address_space` walks an eight- step preflight chain (kind, DERIVE, no-widening, depth, arena capacity, table capacity, then PMM alloc, arena commit, `cap_derive` insert) closing every leak path. Uses `cap_derive` (not `insert_root`) so revocation cascades — pinned by `cap_create_uses_cap_derive_so_revoke_parent_invalidates_child`. Depth preflight closes the `DerivationTooDeep`-after-PMM-alloc leak surface — pinned by `cap_create_rejects_too_deep_parent_without_consuming_pmm`. - *Per-op AS cap rights gap flagged non-blocking:* `cap_map` / `cap_unmap` check kind only, not `CapRights` bits. v1 unreachable (no userspace); deferred to B5+ ADR pairing `CapRights::{MAP,UNMAP, ACTIVATE}` with `CapKind::MemoryRegion`. Documented inline at `resolve_address_space_cap` + flagged in business retro §Adjustments. - *Memory safety:* UNSAFE-2026-0026 lands cleanly under `unsafe-policy. md §3` (three explicit invariants + Rejected-alternatives); the zero-fill discipline establishes the `FrameProvider` contract for downstream callers. UNSAFE-2026-0014 5th Amendment is scope-additive. Three load-bearing memory-safety items closed pre-merge by the five- round PR #28 review arc: (i) PMM leak via depth-preflight gap; (ii) `ipc_recv_and_yield` endpoint-state leak via Phase-1-mutates- before-Phase-2-checks; (iii) `ipc_recv_and_yield` self-dispatch UB when idle == current. All three pinned by regression tests. - *Kernel-mode discipline:* No new panic on reachable path; all allocation paths return typed errors; activation hook fires inside `IrqGuard` scope so DAIF discipline preserved. - *Cryptography:* N/A. - *Secrets / logging:* Two new banners (`pmm initialized`, `address- space-arena ready`); no secret leakage. Bootstrap AS root PA logged is a linker symbol address (public information). - *Dependencies:* Zero-extern still. - *Threat-model:* Infrastructure for future security wins (per-task isolation, kernel-image section permissions); no v1 threat-model shift. The `cap_revoke` cascade-property and depth-preflight defense are defense-in-depth wins. **Forward-flagged carry-forward items** (non-blocking): 1. UNSAFE-2026-0025 / 0026 retain `Pending QEMU smoke verification` for per-call runtime exercise (gates on first B5+ per-task AS task). 2. UNSAFE-2026-0019/0020/0021 still pending IRQ-take/dispatch smoke (gates on first deadline-arming caller). 3. `cap_map` / `cap_unmap` per-op rights gap (B5+ ADR trigger). 4. BSP host-test infrastructure for block-descriptor `unmap` regression (no BSP host-test crate exists). 5. `AddressSpaceArena` SMP-readiness of `get_mut` (v1 single-core OK). 6. `activate_address_space_handle` stale-handle release-mode silent no-op (trade-off accepted; structurally unreachable in v1). **README index** updated with B2 (2026-05-09) row that was inadvertently omitted when the B2 trio landed via PR #24, plus the new B3 row. The README's stale `.claude/skills/perform-security-review` link migrated to `.agents/skills/`. No code changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Security-Review: @cemililik (+ Claude Sonnet 4.6 agent)
…posal Re-baseline artefact per `conduct-review`'s `performance-optimization` master plan. No hypothesis-driven cycle this round; the goal is to record the post-T-018 baseline for B4+ regression checks. **Scope:** post-`b0035ce` (B2 closure baseline) → `47b0a86` (PR #28 merge). Same arc as the business + security reviews. **Headline metrics** (release build, QEMU 10.2.2 on Apple Silicon / Rosetta): - ELF section sizes: `.text +1,624` / `.rodata +592` / `.bss +1,872` vs B2 closure. Total kernel image ~69.6 KiB (was ~64.2 KiB). Δ decomposes cleanly: PMM Pmm::new + alloc_frame body, AddressSpace struct + arena + cap-wrappers, activation-hook closure threading, CapabilityTable::depth_of preflight helper. No new linker reservation (both `Pmm<32768, 4>` and `AddressSpaceArena<QemuVirtMmu, 8>` live in regular `.bss`). - Source LOC: 15,717 (+4,427 vs B2 — ~3,000 LOC impl + tests, ~1,400 LOC docs). - Test count: 226 / 226 pass (+41 vs B2 closure: +14 PMM + +22 AddressSpace + +5 review-round regression tests). - Boot-to-end harness band (20 iter): p10/p50/p90 = 10.311 / 11.884 / 13.823 ms. **Δ vs B2 closure: +6 to +7 ms per percentile** — the band more than doubled. **Hotspot decomposition** of the boot-to-end increase (§Hotspot in the artefact): 1. `Pmm::new` bitmap initialisation pass over 32,768 frames: ~1-2 ms under QEMU TCG (sub-microsecond on real Cortex-A72). 2. `AddressSpaceArena` bootstrap-slot publish + bootstrap-AS cap mint: ~1-2 ms under QEMU TCG (sub-microsecond on real hw). 3. Sustained TCG translation-cache churn from the +5.4 KiB image growth: ~0.2-0.5 ms per first-time-translated function, paid on every iteration (harness destroys the TCG cache between runs). All three are **one-time-at-boot costs amplified by QEMU TCG emulation**, not the result of inefficient implementations. Real- hardware projection: sub-5 ms boot-to-end on a Cortex-A72 at 1.4 GHz (dominated by PL011 UART output bandwidth, not kernel work). **Verdict:** Baseline only — no proposal under measurement. Cite the band when comparing later changes against this snapshot. **Regression check:** `cargo host-test` 226/226 pass; `cargo fmt`, host-clippy, kernel-clippy, kernel-build all clean. QEMU smoke clean — full demo through `tyrne: all tasks complete`; `-d int,unimp,guest_errors` 526 events, 100% pre-existing PL011 noise, zero non-PL011 events. Security review cross-reference: Approve verdict (see 2026-05-14-B3-closure.md security artefact). `unsafe` diff: +1 entry (UNSAFE-2026-0026); UNSAFE-2026-0014 5th Amendment scope-additive; UNSAFE-2026-0025 body-correction Amendment for `MmuError::BlockMapped` variant split. **Forward-flagged items:** 1. Real-hardware perf measurement (trigger: first real-hardware BSP, e.g. Raspberry Pi 4 per README.md). 2. `Pmm::new` bitmap init scales linearly with extent — re-measure when first BSP with >128 MiB RAM lands. 3. `AddressSpaceArena<QemuVirtMmu, 8>` slot count revisit (trigger: B4 userspace bring-up). 4. Activation-hook `differ-path` cost (currently short-circuits because all v1 tasks share bootstrap AS). **README index** updated with B2 (2026-05-09) row that was inadvertently omitted when the B2 trio landed via PR #24, plus the new B3 row. No code changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
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 (4)
📝 WalkthroughWalkthroughPR ChangesB3 Closure Metrics and Reviews
Possibly Related PRs
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/roadmap/current.md`:
- Line 7: Update the lead phrase that currently reads "after three review
rounds" to accurately reflect the full sequence of review rounds and follow-ons
described later in the sentence—e.g., change to "after five review rounds" or
"after five review rounds (including follow-on rounds)"—so the opening clause
matches the enumerated round-1, round-2, round-3, round-4 follow-on and round-5
follow-on items in the same sentence (refer to the phrase "after three review
rounds" and the subsequent "round-4 follow-on" / "round-5 follow-on" mentions to
locate the text to edit).
- Line 55: Update the bootstrap AS root address in the smoke-trace text: find
the string "tyrne: address-space-arena ready (1 / 8 slots used; bootstrap AS
root = 0x40092000)" and replace the literal 0x40092000 with the B3 closure
canonical 0x4008d000 so the docs' reported bootstrap AS root matches the PR
artifacts; this change is in the same paragraph describing AddressSpace<M>,
AddressSpaceArena and the BSP wiring (references: AddressSpace<M>,
AddressSpaceArena, QemuVirtAddressSpace::from_existing_root).
🪄 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: ab9c64d8-822e-4970-90aa-763419863eb5
📒 Files selected for processing (8)
docs/analysis/reports/perf-baseline-2026-05-14-B3-closure.mddocs/analysis/reviews/business-reviews/2026-05-14-B3-closure.mddocs/analysis/reviews/business-reviews/README.mddocs/analysis/reviews/performance-optimization-reviews/2026-05-14-B3-closure.mddocs/analysis/reviews/performance-optimization-reviews/README.mddocs/analysis/reviews/security-reviews/2026-05-14-B3-closure.mddocs/analysis/reviews/security-reviews/README.mddocs/roadmap/current.md
There was a problem hiding this comment.
Code Review
This pull request formalizes the closure of Milestone B3 ("Address space abstraction") by introducing comprehensive business, security, and performance reviews, as well as a new performance baseline report. The changes include updates to the project roadmap and review indices, documenting the completion of the Physical Memory Manager (T-017) and the AddressSpace kernel object (T-018). Feedback from the review highlights several data inconsistencies across the new documents, specifically regarding the breakdown of new test counts and the reported physical address for the bootstrap address space root.
|
|
||
| - `tyrne_kernel::mm::pmm::tests` (14 new): PMM bring-up + alloc / free / reservation / stats / exhaustion-recovery, plus 4 `Pmm::new`-rejects-invalid-input cases (unaligned extent, overlapping reserved ranges, reserved range outside extent, too many reserved ranges). | ||
| - `tyrne_kernel::mm::address_space::tests` (17 new from T-018 commit 1–3, 2 from review-round arc — totalling 19; the table above tracks the 17 originally + the 2 add-ons under the kernel-host count): `wrap_bootstrap_returns_address_space_with_root`, arena round-trips, generation-tag staleness check, full-arena rejection, `cap_create_address_space` happy path + 4 rejection cases (wrong-parent-kind, missing DERIVE, widened rights, OOM), `cap_map` + `cap_unmap` happy paths + wrong-kind + MmuError passthrough cases, `resolve_address_space_cap` kind check, plus two regression tests added during review-round arc: `cap_create_uses_cap_derive_so_revoke_parent_invalidates_child` (revocation cascade) and `cap_create_rejects_too_deep_parent_without_consuming_pmm` (depth preflight closes PMM leak). | ||
| - `tyrne_kernel::sched::tests` (5 new): 3 activation-hook tests (`yield_now_activates_when_tasks_differ_in_address_space`, `yield_now_skips_activation_when_tasks_share_address_space`, `address_space_activation_target_pure_function`) + 1 `Task::address_space_handle` round-trip test + 2 review-round regression tests: `ipc_recv_and_yield_with_no_current_task_leaves_endpoint_idle` (closes the Phase-1-mutates-before-Phase-2-checks endpoint-state leak) and `ipc_recv_and_yield_with_idle_as_current_returns_deadlock` (closes the release-mode split-borrow UB when idle == current). |
There was a problem hiding this comment.
| **Test count:** 226 / 226 (was 185 at B2 closure). The +41 net delta: | ||
| - **+14 in `tyrne-kernel`** (T-017's PMM tests: bring-up + alloc / free / reservation / stats / exhaustion-recovery + 4 `Pmm::new`-rejection tests). | ||
| - **+22 in `tyrne-kernel`** (T-018's AddressSpace tests: 19 cap-wrapper tests + 3 scheduler activation-hook tests + the 2 review-round regression tests counted once each). | ||
| - **+5 in `tyrne-kernel`** (T-018's review-round arc additions: `cap_create_uses_cap_derive_so_revoke_parent_invalidates_child`, `cap_create_rejects_too_deep_parent_without_consuming_pmm`, `ipc_recv_and_yield_with_no_current_task_leaves_endpoint_idle`, `ipc_recv_and_yield_with_idle_as_current_returns_deadlock`, and `Task::address_space_handle_round_trips`). |
There was a problem hiding this comment.
The breakdown of the 41 new tests in this report is inconsistent with the breakdown provided in the business retrospective report (docs/analysis/reviews/business-reviews/2026-05-14-B3-closure.md). For example, this report categorizes the tests into groups of +14, +22, and +5, while the business report uses a different categorization and sub-totals. For accuracy and clarity, the test metrics should be consistent across all milestone closure documents.
|
|
||
| --- | ||
|
|
||
| > **2026-05-14 update — PR #28 merged; T-018 live on `main`; B3 implementation closed; closure trio next.** [PR #28](https://github.com/cemililik/Tyrne/pull/28) merged into `main` as commit `47b0a86` after three review rounds (round-1: 2 valid; round-2: 1 valid + 1 skipped-with-reason; round-3: 6 valid + 1 skipped; round-4 follow-on: CodeRabbit findings on commits `0d4e62c..8b9f52e` — 6 valid + 1 nit skipped; round-5 follow-on: 2 valid). The merge brings T-018 (`AddressSpace` kernel object + cap-gated `Mmu::map`/`unmap` wrappers + activation-on-context-switch) live on `main`. Headline numbers: **226 host tests pass** workspace-wide (was 200 at PR #27 merge; +26: 17 `AddressSpace` tests + 3 scheduler activation-hook tests + 1 `Task::address_space_handle` round-trip test + 5 from the review-round fix-ups — cap-derive revocation, depth preflight, no-current-task, idle-self deadlock, and the existing test-discriminant cleanup). Two parallel side-effects landed in the same arc: (i) commit `8b9f52e` introduced `MmuError::BlockMapped` (additive variant; `Mmu::unmap` returns it instead of `AlreadyMapped` on a block-descriptor walk) + the `CapabilityTable::depth_of` `pub(crate)` preflight helper closing the PMM-leak path in `cap_create_address_space`; (ii) commit `77d3e7e` finished the `.claude/skills/` → `.agents/skills/` migration (deleted the 16-entry duplicate, updated CLAUDE.md / AGENTS.md / 12 live cross-references; the dated review snapshots under `docs/analysis/reviews/code-reviews/2026-05-0{6,7}-*` deliberately retain their `.claude/skills/...` links as point-in-time records). **B3 milestone status:** implementation closed; **next:** B3 closure trio (business + consolidated security + performance baseline), modelled on the [2026-05-09 B2 closure trio](../analysis/reviews/business-reviews/2026-05-09-B2-closure.md). |
There was a problem hiding this comment.
The breakdown of new tests, specifically the "5 from the review-round fix-ups", is inconsistent with other reports in this PR. The business retrospective mentions 6 regression tests, and the performance report categorizes them differently. Aligning the test count breakdowns across all documentation would improve clarity and prevent confusion.
| - **Working branch:** development branches off `main` per PR pattern; T-016 lives on `t-016-mmu-activation`. No rebase pending. | ||
| - **Last completed milestone:** **B1 — Drop to EL1 + exception infrastructure, closed 2026-05-07** via PR #15 merge (`e9fa019`) and the closure trio that landed in PR #16 (`95b15aa`). Required tasks all Done: T-013 (2026-04-27) + T-012 (2026-04-28) + T-014 (2026-05-07). Headline numbers at B1 closure: 152 host tests (149 → 152 via T-014's +3); kernel image `.text` 21,792 bytes (-116 vs 2026-04-28); QEMU smoke produces full demo trace + boot-to-end timing line; miri 152/152 clean. ADR-0026 Accepted (supersedes ADR-0022's *idle-task-location* axis only; *typed-error* axis stands). UNSAFE-2026-0019 / 0020 gained *partial-verification* + *post-T-014 smoke* Amendments; UNSAFE-2026-0021 gained *no-verification* Amendment (timer-write site unreachable in v1 demo); all three retain `Pending QEMU smoke verification` for the IRQ-dispatch path. Comprehensive code review (2026-05-06) + α/β/γ doc/code-polish PRs closed 7 Track-E blockers + Track-J/A/B/F/G/I non-blockers. **Post-T-015 amendment (2026-05-07, PR #17):** kernel image `.text` is now **22,020 bytes (+228 vs PR-#16 baseline; +112 vs 2026-04-28)** — the 228-byte delta is the new non-generic `ipc_cancel_recv` body (216 bytes) + the cold cancel-call arm in `ipc_recv_and_yield<QemuVirtCpu>` (12 bytes); `.rodata` and `.bss` byte-identical to the closure-trio re-baseline. See [`2026-05-07-B1-closure.md` §"Post-T-015 amendment"](../analysis/reviews/performance-optimization-reviews/2026-05-07-B1-closure.md) for the per-symbol decomposition. Previous milestone closure: **B0 — Phase A exit hygiene, closed 2026-04-27** via PR #9 merge (`9a66e8b`). | ||
| - **Last completed tasks:** **T-016 — Done 2026-05-08** (branch `t-016-mmu-activation`). T-016 implementation: HAL `MapperFlush` typed flush-token + `Mmu::map`/`unmap` return-type change (`Result<MapperFlush, MmuError>` / `Result<(MapperFlush, PhysFrame), MmuError>`); pure VMSAv8 descriptor encoders in [`tyrne_hal::mmu::vmsav8`](../../hal/src/mmu/vmsav8.rs) (host-tested); `bsp-qemu-virt::QemuVirtMmu` impl ([`bsp-qemu-virt/src/mmu.rs`](../../bsp-qemu-virt/src/mmu.rs)) covering `create_address_space` / `activate` / `invalidate_tlb_*` / `map` / `unmap`; bootstrap routine in [`bsp-qemu-virt/src/mmu_bootstrap.rs`](../../bsp-qemu-virt/src/mmu_bootstrap.rs) walking ADR-0027 §Simulation steps 1-3; `.boot_pt` linker reservation in [`bsp-qemu-virt/linker.ld`](../../bsp-qemu-virt/linker.ld); UNSAFE-2026-0022 / 0023 / 0024 / 0025 introduced + Amendments to 0023 / 0024 for the bootstrap-site scope extension. Smoke trace byte-stable except for the inserted `tyrne: mmu activated` line; `-d int,unimp,guest_errors` empty across the boot-to-end window. **Earlier:** T-015 — Done 2026-05-07 (PR #17, branch `t-015-endpoint-rollback-cancel-recv`). T-015 implementation: `ipc_cancel_recv(ep_arena, queues, ep_cap, &caller_table) -> Result<(), IpcError>` recovery primitive in [`kernel/src/ipc/mod.rs`](../../kernel/src/ipc/mod.rs); Phase 2 Deadlock branch in [`kernel/src/sched/mod.rs::ipc_recv_and_yield`](../../kernel/src/sched/mod.rs) calls it before returning `Err(SchedError::Deadlock)` so the scheduler-state and endpoint-state rollbacks are symmetric per ADR-0032. Six new tests (5 IPC unit tests covering the cancel state machine + 1 scheduler regression test for the Deadlock-path endpoint rollback); existing T-007 Deadlock test gained an endpoint-state assertion. UNSAFE-2026-0014 fourth Amendment names the new Deadlock-branch momentary `&mut EndpointArena` + `&mut IpcQueues` site (covered under the existing umbrella). ADR-0017 §Revision notes rider records the additive recovery primitive (the user-observable IPC surface remains `send` / `recv` / `notify`). [`docs/architecture/ipc.md`](../architecture/ipc.md) §State machine gained the `RecvWaiting → Idle` cancel arc. Smoke trace byte-for-byte unchanged: full demo through `tyrne: all tasks complete`, boot-to-end ~4.1 ms. **Earlier:** T-014 (2026-05-07 via PR #15), T-012 (2026-04-28 via PR #10), T-013 (2026-04-27 via PR #9). | ||
| - **Last completed tasks:** **T-018 — Done 2026-05-11, live on `main` 2026-05-14 via PR #28** (branch `t-018-address-space-kernel-object`, merge commit `47b0a86`). T-018 implementation: [`AddressSpace<M>`](../../kernel/src/mm/address_space.rs) kernel-object struct + per-type [`AddressSpaceArena`](../../kernel/src/mm/address_space.rs) (ADR-0016 pattern); `CapKind::AddressSpace` + `CapObject::AddressSpace(AddressSpaceHandle)` variants in [`kernel/src/cap/mod.rs`](../../kernel/src/cap/mod.rs); capability-gated wrappers `cap_create_address_space` / `cap_map` / `cap_unmap` with step-by-step preflights (DERIVE rights → no-widening → depth preflight → arena/cap-table capacity → PMM alloc → arena commit → `cap_derive` cap-table insert); `Task` struct extension with `address_space_handle`; activation-on-context-switch hook threaded through `yield_now` / `start` / `ipc_recv_and_yield` / `ipc_send_and_yield` (closure-as-parameter, fires only when outgoing and incoming task ASes differ — short-circuits in v1's bootstrap-shared topology); BSP wiring in [`bsp-qemu-virt/src/main.rs`](../../bsp-qemu-virt/src/main.rs) wraps the already-live bootstrap root via the new `QemuVirtAddressSpace::from_existing_root` `pub unsafe fn` companion. Cross-cutting additions during the review-round arc: `MmuError::BlockMapped` variant (commit `8b9f52e`) so unmap into a bootstrap block descriptor surfaces a distinct typed error from `AlreadyMapped`; `CapabilityTable::depth_of` `pub(crate)` preflight helper closing the PMM-leak path; UNSAFE-2026-0014 fifth Amendment scope-extends the umbrella to the activation hook + BSP-side activation closure (zero new audit entries — additive scope on the existing `&mut Scheduler<C>` momentary-borrow umbrella). Smoke trace gains one new line `tyrne: address-space-arena ready (1 / 8 slots used; bootstrap AS root = 0x40092000)` immediately after `tyrne: pmm initialized (...)` and before `tyrne: timer ready (...)`. Full demo runs to `tyrne: all tasks complete`; `-d int,unimp,guest_errors` reports only the pre-existing PL011-disabled-UART noise (unchanged baseline). **Earlier:** T-017 — Done 2026-05-10 (PR #27, branch `t-017-physical-memory-manager`) — Physical Memory Manager (`Pmm<N, R>` bitmap allocator + `FrameProvider` trait + UNSAFE-2026-0026 zero-fill audit). **Earlier:** T-016 — Done 2026-05-08 (branch `t-016-mmu-activation`) — MMU activation, VMSAv8 descriptor encoders, `MapperFlush` flush-token, UNSAFE-2026-0022 / 0023 / 0024 / 0025 introduced. **Earlier:** T-015 — Done 2026-05-07 (PR #17, branch `t-015-endpoint-rollback-cancel-recv`) — `ipc_cancel_recv` recovery primitive + symmetric scheduler+endpoint rollback in `ipc_recv_and_yield`'s Phase 2 Deadlock branch (ADR-0032). **Earlier:** T-014 (2026-05-07 via PR #15), T-012 (2026-04-28 via PR #10), T-013 (2026-04-27 via PR #9). |
There was a problem hiding this comment.
There is an inconsistency in the reported bootstrap address space root physical address. This file states it is 0x40092000, while the business, security, and performance review documents for the B3 closure all report it as 0x4008d000. Please ensure this value is consistent across all related documents.
…t codification recurring-deferral A maintainer-driven re-read pass on the B3 closure trio surfaced two items to action in-branch before PR #29 merges; the remaining 14 findings across the three artefacts were verified as correctly deferred (B4 / B5+ / SMP / real-hardware triggers all valid). **F3 — Codify "ADR §Simulation row → host-test or audit-log line" mapping discipline.** The recurring deferral across B2 → B3 closures was the load-bearing signal the B3 retro §"What we learned" diagnosed. The rule was practised at four ADR data points (ADR-0026 / 0032 / 0027 / 0028) but the `write-adr` skill text never caught up. The re-read observed that codification is a pure documentation change with four confirming data points already in tree; acting on it pre-emptively breaks the deferral pattern the retro itself flagged as load-bearing. Action: `.agents/skills/write-adr/SKILL.md` extended in three places: - §Procedure step 5 gains a new sub-bullet "Decision outcome → Simulation row-to-verification mapping" codifying the discipline + citing the four data points + naming the rationale (prose-only ADR review cannot catch off-by-one transition errors; pairing every row with executable / audited verification forces the implementer to discharge each simulated state-change with a concrete artefact). - §Acceptance criteria gains a new line requiring the row-to- verification mapping to be recorded in the implementing task's review-history row. - §Anti-patterns gains a new bullet "§Simulation table without row-to-verification mapping" so the rule is enforced going forward. **Fact-fix — B3 retro §Adjustments F1 referenced "ADR-0036".** Verified against `phase-b.md §B4 Approach item 1` and the ADR index reservation row: the actual planned slot is ADR-0029 (Initial userspace image format), not ADR-0036. Inline fix applied to the F1 bullet; no other location referenced the incorrect number. **B3 retro updates** to reflect the in-branch closures: - B2 closure status table row for the §Simulation codification: "Open (still trigger-deferred)" → "Closed in-branch (see §Closure-trio re-read below)". - Net summary: "2 of 6 closed" → "3 of 6 closed; 3 trigger-deferred"; rewrote the trailing diagnosis sentence to reflect that the recurring-deferral signal was the trigger to act pre-emptively. - §Adjustments F3 checkbox flipped to [x] + closure-note appended linking to the new section. - New §"Closure-trio re-read — items closed in-branch" section inserted between §Adjustments and §Next, recording the two closures + an explicit "other 14 findings correctly deferred" paragraph that names every forward-flag from the three artefacts and the trigger each is waiting on. No code changes; documentation-only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bot review-round on PR #29 (CodeRabbit + gemini-code-assist; sourcery rate-limited). All six findings are valid drift in the closure-trio docs + `current.md` and were verified against empirical per-module test counts via `cargo test -p tyrne-kernel --lib -- <module>:: --list` at HEAD `3ec94b0` and at B2 closure `b0035ce`. **Empirical truth — per-module test deltas (B2 closure → HEAD):** | Module | B2 | HEAD | Δ | |---|---|---|---| | `mm::pmm::tests` | 0 | 15 | +15 | | `mm::address_space::tests` | 0 | 20 | +20 | | `sched::tests` | 22 | 27 | +5 | | `obj::task::tests` | 3 | 4 | +1 | | **Total kernel-host** | **25** | **66** | **+41** | Categorised by origin: 15 PMM (T-017) + 22 T-018-commits-1-3 (18 AS + 3 sched + 1 task) + 4 review-round (2 AS + 2 sched) = 41. **Six findings applied:** 1. **CodeRabbit @ current.md:7 — "three review rounds" → "five".** The lead phrase contradicted the enumeration of round-1..round-3 + round-4-follow-on + round-5-follow-on in the same sentence. 2. **CodeRabbit @ current.md:55 + gemini @ current.md:55 — bootstrap AS root `0x40092000` → `0x4008d000`.** Cross-doc drift: the closure-trio artefacts all cite `0x4008d000` (live smoke trace at HEAD `6334881`); current.md L50 + L55 had the older value from commit `daee78f` (pre-PR-#28-merge; the address shifted because review-round commits altered `.text`/`.bss` layout, moving `__boot_pt_l0`). Both L50 (Active task bullet, daee78f-inherited) and L55 (Last completed tasks bullet, my closure-trio commit) now match the live value. 3. **gemini @ business-reviews/2026-05-14-B3-closure.md:65 — `sched::tests` (5 new) text vs 6-item list mismatch.** The list incorrectly attributed the `Task::address_space_handle_round_trips` test to `sched::tests`; it actually lives in `obj::task::tests`. Rewrote the per-test breakdown paragraph to list each module empirically (PMM +15, AS +20, sched +5, task +1) with sum = 41 and origin-category cross-check; added the missing `obj::task::tests (+1, 3 → 4)` bullet. 4. **gemini @ perf-reviews/2026-05-14-B3-closure.md:73 — per-crate categorisation inconsistent with business retro.** Perf used "+14 PMM + 22 AS + 5 review-round = 41"; business retro used different sub-totals; both differed from empirical reality (+15 PMM, +20 AS-module, +4 review-round). Rewrote perf's Test count section to match the business retro's per-module empirical layout + added an origin-category sum + cross-link to the business retro per-test breakdown. 5. **gemini @ current.md:7 — "5 from the review-round fix-ups" inconsistent with other reports.** The "5th" entry (cited as "the existing test-discriminant cleanup") was a *phantom*: the only existing-test modification in the review-round arc was the T-007 Deadlock test gaining an endpoint-state assertion (a *modified* test, not a *new* one). Rewrote L7's test breakdown as "18 AS + 3 sched + 1 task + 4 review-round = 26", which matches the PR #27 → PR #28 delta exactly. 6. **gemini @ current.md:55 — same bootstrap AS root drift as CodeRabbit #2 (consolidated above).** **Business retro audit-note paragraph updated** to record the correction trail: the earlier draft mis-counted PMM as +14 (actual +15) and the review-round arc as +5 or +6 (actual +4); the closure-trio re-read pass on 2026-05-14 corrected both. No drift between the headline (41) and the breakdown after the correction. Security review unchanged — it doesn't cite per-module test counts, only the workspace-wide 226 total which is correct. No code changes; docs-only sweep. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round-1 review findings — all 6 applied (commit
|
| Module | B2 | HEAD | Δ |
|---|---|---|---|
mm::pmm::tests |
0 | 15 | +15 |
mm::address_space::tests |
0 | 20 | +20 |
sched::tests |
22 | 27 | +5 |
obj::task::tests |
3 | 4 | +1 |
| Total kernel-host | 25 | 66 | +41 |
Categorised by origin: 15 PMM (T-017) + 22 T-018-commits-1-3 (18 AS + 3 sched + 1 task) + 4 review-round (2 AS + 2 sched) = 41.
Per-finding closure
| # | Reviewer | Finding | Action |
|---|---|---|---|
| 1 | @coderabbitai | current.md:7 "three review rounds" vs enumeration of 5 | Lead phrase fixed: "three" → "five" |
| 2 | @coderabbitai | current.md:55 bootstrap AS root 0x40092000 vs canonical 0x4008d000 |
Both L50 (daee78f-inherited) and L55 (closure-trio commit) updated to live value 0x4008d000. Drift cause: review-round commits altered .text/.bss layout, shifting __boot_pt_l0 linker symbol between commit daee78f and HEAD |
| 3 | @gemini-code-assist | business retro:65 sched::tests (5 new) vs 6-item list |
The list incorrectly attributed Task::address_space_handle_round_trips to sched::tests (lives in obj::task::tests). Rewrote per-test breakdown to list each module empirically + added missing obj::task::tests (+1) bullet |
| 4 | @gemini-code-assist | perf review:73 categorisation inconsistent with business retro | Rewrote perf Test-count section to match business retro's per-module empirical layout + origin-category sum + cross-link |
| 5 | @gemini-code-assist | current.md:7 "5 review-round fix-ups" inconsistent | The "5th" entry was a phantom: only modification (T-007 Deadlock test gaining an endpoint-state assertion) was a modified test, not new. Corrected to 4 review-round tests; total 26 = 18 AS + 3 sched + 1 task + 4 review-round |
| 6 | @gemini-code-assist | current.md:55 bootstrap AS root drift | Same as #2; consolidated |
Audit-note trail in business retro
The per-test breakdown's audit note now records: "An earlier draft of this row mis-counted PMM as +14 (actual +15) and the review-round arc as +5 or +6 (actual +4; the phantom '5th' entry was a modified T-007 Deadlock test that gained an endpoint-state assertion in T-015, not a new test added in the T-018 review-round arc); the closure-trio re-read pass on 2026-05-14 corrected both."
Security review unchanged — it doesn't cite per-module test counts, only the workspace-wide 226 / 226 total which remains correct.
No code changes; docs-only sweep across 3 files (current.md, business retro, perf review). All gates clean (cargo host-test 226/226, cargo fmt, host-clippy, kernel-clippy, kernel-build).
🤖 Generated with Claude Code
|
The empirical per-module verification table is a clean cross-check: 15 + (18 + 3 + 1) + (2 + 2) = 41 ✓, matching the workspace delta. The audit-note trail in the business retro correctly distinguishes new tests from modified tests, which is important for accurate milestone accounting. No further concerns from my side. I'll resolve my open comments now. ✅ Actions performedComments resolved. Approval is disabled; enable |
…5 valid, 2 skipped) Bot review-round on PR #30 (gemini-code-assist + sourcery-ai; coderabbit rate-limited; qodo paused). 7 findings total — 5 valid + 2 invalid (gemini PR-scope confusion). **Applied (5):** 1. **G-1b — current.md L50 outdated signature.** Active-task bullet still cited `task_create_from_image(...) -> Result<TaskHandle, TaskLoaderError>` from the original ffd712c draft; the pre-Accept #1 review (fd91567) shifted to `load_image(...) -> Result<LoadedImage, LoadError>` but current.md was never updated. Fix: L50 rewritten with the current canonical signature + the "LoadedImage opaque descriptor, not TaskHandle" framing + the typed `LoadError` enum variants + the cap_drop-not-cap_revoke rollback note + UNSAFE-2026-0027 audit-entry forward-flag. 2. **G-2 — ADR-0029 §Negative consequences `.data` write-fault risk now explicit.** The original "no per-section permissions" bullet stated the constraint but didn't make the executable- without-write consequence concrete. Fix: rewrote to state plainly that v1 image region maps `USER | EXECUTE` (no `WRITE`), so a real binary's `.data` section would permission-fault on the first write — v1 userspace is effectively restricted to code + read-only data. Hand-coded placeholder (`mov w0, #42; ret`) and B6's planned minimal "hello" have no `.data`, so the constraint is non-blocking for the v1 demo trajectory. 3. **G-3 — T-019 §Simulation step 5 pseudo-code inaccuracies.** Three issues: (a) `frame_pa` used as pointer but `pmm.alloc_frame ()` returns `PhysFrame` struct — should be `frame.as_usize() as *mut u8`; (b) `copy_len` undefined — should be `core::cmp::min (PAGE_SIZE, image.len() - i*PAGE_SIZE)` for tail truncation on the partial last page; (c) tail-zeroing relies on UNSAFE-2026- 0026's PMM zero-init contract for bytes copy_len..PAGE_SIZE. Fix: pseudo-code rewritten with the correct conversions + new `task_loader::tests::tail_zeroing_on_partial_last_page` row-7 verification test added. 4. **S-1 — cap_drop vs cap_revoke explanation consolidation.** Sourcery flagged that the cap_drop/cap_revoke + leak baseline reasoning was repeated in 3 places (§Context leak-path-closure paragraph, §Acceptance criteria MapFailed bullet, §Rollback contract intro). Fix: §Rollback contract intro stays canonical (most authoritative location); §Context paragraph trimmed to a forward-reference; §Acceptance criteria MapFailed bullet shortened to a one-line summary + pointer to the canonical section. Drift risk on the cap_drop reasoning now sits in one place to maintain. **Skipped (2):** 5. **G-1a — "B3 closure files missing from this PR."** Skipped — gemini's claim is wrong. The B3 closure files (`2026-05-14-B3-closure.md` in business / security / perf review folders) landed in PR #29 (merged 2026-05-14 as commit `b425dc1`) and exist on `main`. PR #30's diff is additive to main; linking from PR #30's files to existing main files is fine and the links resolve correctly. 6. **G-4 — "Link to B3 closure adjustments broken."** Same skip reason as G-1a. Link `../../analysis/reviews/business-reviews/ 2026-05-14-B3-closure.md#adjustments` from `docs/roadmap/phases/phase-b.md` resolves to the file on `main`. **Sourcery-2 (B5/B6 forward-refs depth) also skipped:** judgement call rather than correctness issue. The forward-flags for MemoryRegionCap, destroy path, syscall ABI, ADR-0033 high-half, and ADR-0034 section permissions serve a scope-clarity purpose (they explicitly enumerate what B4 does NOT do), which is valuable for the implementer reading T-019 cold. Aggressive trimming would lose information that scopes the work explicitly. The §Out of scope section already concentrates most of these refs; minor consolidation would help marginally but isn't load-bearing. No code changes; docs-only sweep across 3 files. Refs: ADR-0029 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Closes B3 milestone ("Address space abstraction") via the standard
business + security + performance review trio per ADR-0013
and the established B1 (2026-05-07) / B2 (2026-05-09) closure precedents.
A subsequent closure-trio re-read pass (commit
3ec94b0) examinedthe 16 findings across the three artefacts and closed two in-branch
before merge — the rest were verified as correctly deferred.
Scope: post-
b0035ce(B2 closure baseline) →47b0a86(PR #28merge). Covers T-017 PMM, T-018
AddressSpacekernel object + cap-gated wrappers + activation-on-context-switch, ADR-0035 + ADR-0028,
the
CapKind::AddressSpace+CapObject::AddressSpacecap-variantadditions,
CapabilityTable::depth_ofpub(crate)preflight helper,MmuError::BlockMappedadditive variant, UNSAFE-2026-0026 (new),UNSAFE-2026-0014 5th Amendment, UNSAFE-2026-0025 body-correction
Amendment, and the
.claude/skills/→.agents/skills/migration.Branch commits (5 total; all docs-only — no code changes):
6334881— refreshcurrent.mdpost-PR-feat: T-018 — AddressSpace kernel object + cap-gated Mmu::map/unmap wrappers + activation-on-context-switch (B3 §§2-7 closure) #28 (new 2026-05-14 updateblock; Last completed tasks rewritten to lead with T-018 + back-fill
T-017; Next task to open flipped to "none — B4 next"; Next review
trigger armed; footer skill links migrated to
.agents/skills/).c808a96— business retrospective + perf-baseline report.7519d94— consolidated security review (8 axes Approve).b2229f7— performance baseline (re-baseline, no proposal).3ec94b0— closure-trio re-read pass. Closes F3 (§Simulationrow → host-test/audit-log codification — was deferred twice through
B2 → B3 closures; the recurring deferral was itself the signal the
retro diagnosed as load-bearing) by extending the
write-adrskillin three places (Procedure step 5 sub-bullet + Acceptance criteria
line + Anti-pattern bullet). Also fixes a small ADR-number drift in
§Adjustments F1 ("ADR-0036" → ADR-0029, verified against
phase-b.md §B4and the ADR index reservation row).Verdicts
docs/analysis/reviews/business-reviews/2026-05-14-B3-closure.mdMmuError::BlockMappedvariant + §Simulation row-to-verification codification in-branch); 3 trigger-deferred (PL011 init BSP task + 2 future-phase placeholders ADR-0033/0034).docs/analysis/reviews/security-reviews/2026-05-14-B3-closure.mdipc_recv_and_yieldendpoint-state leak;ipc_recv_and_yieldself-dispatch UB when idle == current). One non-blocking forward flag (cap_map/cap_unmapper-op rights gap — deferred to B5+ ADR pairingCapRights::{MAP,UNMAP,ACTIVATE}withCapKind::MemoryRegion).docs/analysis/reviews/performance-optimization-reviews/2026-05-14-B3-closure.mdHeadline numbers
.text +1,624 (+7.3 %)/.rodata +592 (+20.1 %)/.bss +1,872 (+4.7 %). Total kernel image ~69.6 KiB.tyrne: all tasks complete; new bannertyrne: address-space-arena ready (1 / 8 slots used; bootstrap AS root = 0x4008d000).-d int,unimp,guest_errors: 526 events, 100 % pre-existing PL011 disabled-UART noise, zero non-PL011 events.unsafeaudit log: 26 entries (was 25 at B2 closure; 0012 still Removed). UNSAFE-2026-0026 new (PMM frame-zeroing); UNSAFE-2026-0014 gained 5th Amendment (scope-additive); UNSAFE-2026-0025 gained body-correction Amendment.Closure-trio re-read — findings audit
Of the 16 findings in the trio:
write-adrskill); ADR-0036 → ADR-0029 fact-fixcap_map/cap_unmaprights gap; UNSAFE-2026-0019/0020/0021 smoke; UNSAFE-2026-0025/0026 smoke; ADR-0033 high-half placeholder; ADR-0034 section permissions placeholder;activate_address_space_handlestale-handle silent no-opget_mutNext (from Pathfinder)
phase-b.md §B4).Test plan
cargo fmt --all --checkclean.cargo host-test— 226 / 226 pass.cargo host-clippy -D warningsclean.cargo kernel-clippy -D warningsclean.cargo kernel-buildclean.tyrne: all tasks complete; zero non-PL011-d guest_errorsevents.Refs: ADR-0013, ADR-0028, ADR-0035
🤖 Generated with Claude Code