T-019: Task loader (load_image + BSP smoke + UNSAFE-2026-0027)

[cemililik]

Summary

• task_loader::load_image<M, N, R> lands in kernel/src/obj/task_loader.rs as the §Simulation row 1–7 state machine (preflight chain → cap_create_address_space → image-page loop with copy_nonoverlapping byte-copy + cap_map(USER|EXECUTE) → stack-page loop with cap_map(USER|WRITE) → LoadedImage return). Rollback helper covers every fallible step with reverse-order cap_unmap + pmm.free_frame + leaf-frame direct free + cap_drop on the AS cap.
• UNSAFE-2026-0027 opens standalone for the loader's copy_nonoverlapping byte-copy (semantically distant from UNSAFE-2026-0026's PMM zero-fill per justify-unsafe audit-tag scoping). UNSAFE-2026-0025 + UNSAFE-2026-0026 each gain a 2026-05-14 smoke-verification Amendment lifting their Pending QEMU smoke verification status — T-019 is the first runtime exerciser of both.
• BSP wiring in bsp-qemu-virt/src/main.rs adds an 8-byte mov w0, #42; ret placeholder image, calls load_image after the address-space-arena banner, and prints tyrne: image loaded (entry = 0x800000; sp = 0x802000; image bytes 8; stack bytes 4096; AS cap = idx 1) on every boot. Image is loaded but not executed — runnability gates on B5/B6 per phase-b §B4 §Revision-notes.
• New arch doc docs/architecture/task-loader.md synthesises the loader sequence, rollback contract, v1 baseline leaks (root L0 + intermediate L1/L2/L3 + AS arena slot — full reclaim arrives with future MemoryRegionCap + per-AS destroy ADR), mapping flags, and audit-log surface. Cross-linked from memory-management.md §"Address-space objects" and boot.md sequence diagram.
• 3 bisectable commits: 911f2ad (types) → 5711756 (function + audit) → ae31bc8 (BSP + docs). Each commit independently compiles + passes tests.

Test plan

• cargo fmt --check clean
• cargo host-test — 247/247 passing (was 231 at T-019 commit 1; +16 from this PR's new tests)
• cargo host-clippy -- -D warnings clean
• cargo kernel-clippy -- -D warnings clean
• cargo kernel-build clean
• QEMU smoke (debug build) — full demo runs to tyrne: all tasks complete; new tyrne: image loaded (...) line lands in expected position
• qemu-system-aarch64 -d int,unimp,guest_errors — reports only the pre-existing 629 PL011-disabled-UART warnings (no new fault classes, no Translation/Permission faults)
• T-019 host-test coverage pins every §Simulation row (rows 1–7) + every rollback-contract path (mid-image-loop failure, mid-stack-loop failure, misaligned VA, zero-pages cap_drop-only) + the 7-variant LoadError taxonomy (within-crate exhaustiveness check)

Linked artefacts

• T-019 — Task loader (Status: In Review)
• ADR-0029 — Initial userspace image format (Accepted 2026-05-14, merged via docs(adr,task): ADR-0029 — Initial userspace image format (B4 — raw flat) + T-019 Draft #30)
• docs/architecture/task-loader.md — new chapter
• docs/audits/unsafe-log.md — UNSAFE-2026-0025 + 0026 Amendments + UNSAFE-2026-0027 introduction

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Adds a task‑loader that loads an embedded userspace placeholder into a freshly created address space for boot-time smoke verification (load-only; not executed).
  • Publishes a loader API and explicit error taxonomy for image/stack validation and mapping outcomes.

• Documentation

  • Updates boot, memory-management, task‑loader, audits, ADR, and roadmap docs to describe behavior, validation/rollback contracts, and audit amendments (T-019 now In Review).

• Tests

  • Adds host tests and smoke checks, including deterministic allocation-failure injection and overlap/budget edge-case coverage.

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[sourcery-ai]

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

Please try again later or upgrade to continue using Sourcery

[coderabbitai]

Warning

Rate limit exceeded

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

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

⌛ How to resolve this issue?

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

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

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

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

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9bcd3d27-6ccd-4b79-90cb-a94ac9c3ac06

📥 Commits

Reviewing files that changed from the base of the PR and between 74694d4 and eb14c51.

📒 Files selected for processing (8)

• bsp-qemu-virt/src/main.rs
• docs/analysis/tasks/phase-b/T-019-task-loader.md
• docs/architecture/task-loader.md
• docs/decisions/0029-initial-userspace-image-format.md
• hal/src/mmu/mod.rs
• kernel/src/mm/mod.rs
• kernel/src/mm/pmm.rs
• kernel/src/obj/task_loader.rs

📝 Walkthrough

Walkthrough

Loads an embedded v1 userspace blob into a new address space via task_loader::load_image: precise preflights (alignment, VA bounds, exact frame budgeting, PA-overlap), allocation/mapping with rollback on mid-loop failures, PMM helpers and test injection, BSP smoke invocation, and updated docs/audits/roadmap.

Changes

T-019 Task Loader Implementation

Layer / File(s)	Summary
Memory management helpers kernel/src/mm/mod.rs, kernel/src/mm/pmm.rs	Adds phys_frame_kernel_ptr for kernel-frame pointers, Pmm::could_yield_pa_overlapping for defensive PA-overlap queries, and test-only alloc-failure injection to drive rollbacks.
Task loader API & types kernel/src/obj/task_loader.rs	Defines intermediate_frame_count, USERSPACE_VA_LIMIT, LoadedImage descriptor, and LoadError enum with alignment/VA-range/overlap/budget/map failure variants.
Core load_image & rollback kernel/src/obj/task_loader.rs	Implements staged preflights (misaligned base, VA bounds, exact intermediate-frame budget, PA-overlap), address-space creation, image/stack allocation, unsafe copy and cap_map calls, and a rollback helper that unmaps/frees on mid-loop failures.
Comprehensive test suite kernel/src/obj/task_loader.rs (#[cfg(test)])	Host-side fakes and injection tests covering preflight rejections, intermediate-frame accounting, PA-overlap regressions, mapping flags, tail-zeroing, and rollback semantics.
Module exports and BSP boot smoke kernel/src/obj/mod.rs, bsp-qemu-virt/src/main.rs	Re-exports task_loader types; BSP boot-phase smoke embeds a placeholder image, calls load_image, and prints returned metadata (entry VA, stack-top VA, image/stack byte counts, AS cap index).
Spec, audits, and ADRs docs/analysis/tasks/phase-b/T-019-task-loader.md, docs/audits/unsafe-log.md, docs/decisions/0029-*.md	Moves T-019 to In Review, updates LoadError with Misaligned/InvalidBase/ImageOverlapsAllocatableMemory, rewrites simulation/rollback rows, adds UNSAFE-2026-0027 for byte-copy invariants, and syncs placeholder blob bytes.
Architecture docs & roadmap docs/architecture/task-loader.md, docs/architecture/boot.md, docs/architecture/memory-management.md, docs/roadmap/current.md	Adds task-loader chapter, documents boot-time smoke step (NOT executed), ties first runtime Pmm/Mmu exercise to T-019, and updates roadmap status and next-step planning.

Sequence Diagram(s)

sequenceDiagram
  participant BSP as BSP (kernel_entry)
  participant load_image as task_loader::load_image
  participant Pmm as Pmm
  participant CapTable as CapabilityTable
  participant Mmu as Mmu
  BSP->>load_image: call load_image(image, image_base_va, stack_pages, ...)
  load_image->>load_image: preflight (align, VA bounds, frame budget)
  load_image->>Pmm: could_yield_pa_overlapping(image PA range)
  load_image->>CapTable: cap_create_address_space(parent_as_cap)
  load_image->>Pmm: alloc_frame() for image pages
  load_image->>load_image: copy_nonoverlapping -> phys_frame_kernel_ptr(dst)
  load_image->>Mmu: cap_map(image page, USER|EXECUTE)
  load_image->>Pmm: alloc_frame() for stack pages
  load_image->>Mmu: cap_map(stack page, USER|WRITE)
  alt mapping/allocation failure
    load_image->>Mmu: unmap committed pages (rollback)
    load_image->>Pmm: free returned frames
    load_image->>CapTable: drop AS cap
  else success
    load_image->>BSP: return LoadedImage (as_cap, entry_va, stack_top_va, byte counts)
  end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• cemililik/Tyrne#28: BSP address-space scaffolding (BOOTSTRAP_AS_CAP/BOOTSTRAP_AS_TABLE) that T-019 builds upon.
• cemililik/Tyrne#23: Prior boot ordering/MMU setup changes that affect where the loader smoke runs.
• cemililik/Tyrne#26: PMM bring-up and alloc semantics changes used by the task-loader (alloc/query integration).

Poem

🐰 I hopped a blob into a brand-new space,

with checks and rollbacks in careful place.
Pages aligned, the PMM gave room,
a tiny userspace bud began to bloom,
B4's trail marked with a rabbit's trace.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately summarizes the main change: implementing T-019 task loader with load_image function, BSP QEMU smoke integration, and UNSAFE-2026-0027 audit logging.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch t-019-task-loader

---

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

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[gemini-code-assist]

Code Review

This pull request implements the task loader (T-019), enabling the loading of raw-flat userspace images into new address spaces with allocated stacks. It includes the task_loader module, extensive documentation, and BSP wiring for smoke testing. Feedback highlights opportunities to improve portability by abstracting physical-to-virtual address conversion, enhance robustness with VA range overflow checks, and avoid potential fragility by explicitly defining required capability rights in the smoke test.

[gemini-code-assist]

The cast from usize (physical address) to *mut u8 relies on the identity mapping invariant of the v1 kernel. While this is documented in the audit log, it makes the code less portable to future high-half kernel designs. Consider introducing a phys_to_virt helper or using a HAL-provided method to materialise the pointer, which would centralise the identity-mapping assumption.

[gemini-code-assist]

The stack_top_va calculation uses saturating_add. If the stack region overflows the 64-bit address space (or the 48-bit userspace range), stack_top_va will saturate to usize::MAX. This value is not page-aligned and does not accurately represent 'one-past-the-highest' mapped address in an overflow scenario. A preflight check for the total VA range [image_base_va, stack_top_va) against the architected userspace VA bounds would be more defensive.

[gemini-code-assist]

The smoke test passes CapRights::empty() for the new address space. If cap_map or cap_unmap (called during rollback) enforce specific rights (e.g., CapRights::MAP), the loader will fail. While the smoke test currently passes, this suggests either a lack of rights enforcement in the capability wrappers or a potential fragility if enforcement is added later. Passing a minimal set of required rights (e.g., CapRights::MAP | CapRights::UNMAP) would be more robust.

[cemililik]

Review-round 2 (gemini-code-assist line-level) addressed in commit 164522d. All 3 findings handled:

F1 (P2) — task_loader.rs:479 destination cast portability. ✅ Fixed. New public kernel helper crate::mm::phys_frame_kernel_ptr(PhysFrame) -> *mut u8 in kernel/src/mm/mod.rs centralises v1's identity-mapping invariant per ADR-0027 §Decision outcome (a); the load_image byte-copy site now routes through it. Future ADR-0033 high-half migration updates one helper body instead of every kernel-side caller. The PMM zero-fill site (UNSAFE-2026-0026) is intentionally NOT migrated in the same commit — its existing inline doc-comment already names the future-migration plan; both adopters update at the ADR-0033 commit per the audit-entry append-only discipline. UNSAFE-2026-0027 gained a 2026-05-15 Amendment recording the helper introduction.

F2 (P2) — task_loader.rs:569 stack_top_va saturating overflow. ✅ Fixed. §Simulation row 3 now bundles a VA-range preflight: image_base_va.saturating_add((image_pages + stack_pages) * PAGE_SIZE) <= USERSPACE_VA_LIMIT (= 1 << 48 per ADR-0027 TCR_EL1.T0SZ = 16). New variant LoadError::InvalidImageBaseVa { base, end } (with end == VirtAddr(usize::MAX) as the documented sentinel for the saturated path). Three new host tests pin the contract: rejects_image_base_va_past_userspace_va_limit, rejects_image_base_va_saturating_overflow, accepts_image_base_va_exactly_at_userspace_va_limit_minus_span (half-open end == USERSPACE_VA_LIMIT boundary is permitted).

F3 (P2) — main.rs:997 CapRights::empty() for BSP smoke. ❌ Rejected with documentation. v1's cap-rights model is kind-only, not per-operation (per resolve_address_space_cap's doc-comment in kernel/src/mm/address_space.rs); the CapRights enum (kernel/src/cap/rights.rs) accordingly exposes only DUPLICATE / DERIVE / REVOKE / TRANSFER / SEND / RECV / NOTIFY — there is no MAP / UNMAP bit defined to pass. The suggested CapRights::MAP | CapRights::UNMAP does not compile against v1. The BSP smoke call site gained a multi-line comment recording the v1 contract + the additive-update path that lands with the future per-operation rights ADR (B5+); UNSAFE-2026-0027's Amendment also documents the rejection rationale.

Tests at HEAD 164522d: 253/253 (+3 from the F2 VA-range tests; baseline 250 → 253). All gates clean: cargo fmt --check, cargo host-test, cargo host-clippy -D warnings, cargo kernel-clippy -D warnings, cargo kernel-build. QEMU smoke byte-stable — full demo through tyrne: all tasks complete; -d int,unimp,guest_errors reports only the pre-existing 629 PL011-disabled-UART warnings.

[cemililik]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[cemililik]

Review-round 3 addressed in commit 5b1f153. All 5 findings handled (4 × P2 substantive + 1 × P3 doc).

F1 (P2) — intermediate-frame budget undercount. ✅ Fixed. Removed the hard-coded pub const INTERMEDIATE_FRAME_BUDGET: usize = 6 (which under-counted for image spans crossing > 1 L2 slot); introduced pub fn intermediate_frame_count(image_base_va, image_pages, stack_pages) -> usize that walks VMSAv8 4-level index decomposition (shifts 21/30/39) for an exact distinct-table count. The reviewer's 8 MiB image example now yields exactly 7 intermediates (1 L1 + 1 L2 + 5 L3) as expected. Five helper unit tests pin the contract: _minimal_single_l2_slot (= 3), _8mib_image_one_stack_page_crosses_five_l2 (regression for reviewer's example; = 7), _l1_boundary_crossing (= 5), _zero_span_defensive, _saturated_total_pages.

F2 (P2) — VA-range overflow shadowed by frame-budget. ✅ Fixed. Reordered §Simulation row 3 to run VA-range check first, then frame-budget. New regression test va_range_preflight_runs_before_frame_budget pins the order: image = &[0xAA], stack_size_pages = usize::MAX now returns InvalidImageBaseVa { end: VirtAddr(usize::MAX) } (the documented saturated sentinel) instead of being masked by FrameBudgetExceeded.

F3 (P2) — current.md stale. ✅ Fixed. Banner refreshed: 6-commit arc ending at 5b1f153, 259/259 tests, full 9-variant LoadError taxonomy listed (including InvalidImageBaseVa + ImageOverlapsAllocatableMemory), PR #31 In Review.

F4 (P2) — memory-management.md misattribution. ✅ Fixed. Corrected the §"Address-space objects" / §"FrameProvider integration" attribution: T-018 installs the AS-object surface but uses wrap_bootstrap on the live .boot_pt L0 (per ADR-0028 §Simulation row 0); T-019 is the first runtime exerciser of Pmm::alloc_frame + cap_create_address_space + post-bootstrap Mmu::map, and accordingly of UNSAFE-2026-0025 / 0026 / 0027.

F5 (P3) — Minor counter/prose drift. ✅ Fixed. docs/architecture/task-loader.md test count 24 → 33; docs/architecture/boot.md stage-3 prose now mentions the task_loader::load_image step (Mermaid already had it).

UNSAFE-2026-0027 gained a 2026-05-15 Amendment recording the F1+F2 contract fixes (the audited unsafe operation is unchanged — both fixes harden the safe-API boundary in front of it). T-019 user-story §Simulation row 3 rewritten to reflect the new ordering + exact-count budget; new review-history row records this round.

Tests at HEAD 5b1f153: 259/259 (was 253, +6: F1's 5 helper unit tests + F2's reorder regression test). All gates clean: cargo fmt --check, cargo host-test, cargo host-clippy -D warnings, cargo kernel-clippy -D warnings, cargo kernel-build. QEMU smoke byte-stable — full demo through tyrne: all tasks complete; -d int,unimp,guest_errors reports only the pre-existing 629 PL011-disabled-UART warnings.

[cemililik]

Review-round 4 addressed in commit 95efd62. All 3 findings handled (1 × P2 + 2 × P3).

P2 — Preventable root-frame leak on misaligned image_base_va. ✅ Fixed. Row 1's argument preflight now checks image_base_va.0.is_multiple_of(PAGE_SIZE) and rejects with new variant LoadError::MisalignedImageBaseVa(VirtAddr) before any cap_create_address_space invocation. The renamed test rejects_misaligned_image_base_va_with_pmm_byte_stable asserts pmm.stats().free_frames == pmm_before (no root-frame leak). The rejects_image_base_va_saturating_overflow test was retargeted from VirtAddr(usize::MAX - PAGE_SIZE) (unaligned, now intercepted by row 1) to VirtAddr(usize::MAX & !(PAGE_SIZE - 1)) (page-aligned) to preserve the row-3 saturating-overflow coverage. LoadError taxonomy: 9 → 10 variants; exhaustiveness regression test would compile-fail if the variant is silently removed.

P3 #1 — Non-deterministic accepts_image_disjoint_from_pmm_extent test. ✅ Fixed. Switched to a .rodata-resident static DISJOINT_RODATA_IMAGE: [u8; 8]; disjointness from the heap-backed PMM extent is structurally guaranteed. The silent-skip if {...} return replaced with a hard assert! that fail-loudly on toolchain regression. Also mirrors the production BSP wiring shape.

P3 #2 — Stale LoadError::FrameBudgetExceeded doc-comment. ✅ Fixed. Refreshed from "intermediate_budget = 6 is the safe upper bound..." (stale since round-3 F1) to a reference to the exact intermediate_frame_count(...) helper.

UNSAFE-2026-0027 gained a 2026-05-15 Amendment recording all three fixes (the audited unsafe operation is unchanged; P2 closes an internal-API-misuse PMM leak path; P3s harden test/doc accuracy in front of the byte-copy site).

Tests at HEAD 95efd62: 259/259 (no net new tests — round 4 reframes the misaligned-VA test rather than adding new ones; the distinctness assertion gained 2 sub-cases inside an existing test; the disjoint test's assert! replaces the prior return skip path). All gates clean: cargo fmt --check, cargo host-test, cargo host-clippy -D warnings, cargo kernel-clippy -D warnings, cargo kernel-build. QEMU smoke byte-stable — full demo through tyrne: all tasks complete; -d int,unimp,guest_errors reports only the pre-existing 629 PL011-disabled-UART warnings.

[cemililik]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (1)

kernel/src/mm/mod.rs (1)

96-140: 💤 Low value

Forward-compat doc snippet uses .expect(...) which is denied in this crate.

The forward-compat note at line 122 sketches the future high-half body as KERNEL_PHYS_BASE.checked_add(frame.as_usize()).expect(...). Per the existing crate convention clippy::expect_used is denied, so a future maintainer copying this verbatim would hit a lint failure. Consider rephrasing the example to use unwrap_or(...) with a sentinel or a debug_assert!-guarded path so the doc-suggested form is lint-clean when ADR-0033 lands.

Based on learnings: "In the Tyrne kernel crate (Rust files under kernel/src/), avoid using .expect() (clippy lint clippy::expect_used is denied)."

🤖 Prompt for AI Agents

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

In `@kernel/src/mm/mod.rs` around lines 96 - 140, The forward-compat doc for
phys_frame_kernel_ptr suggests using
KERNEL_PHYS_BASE.checked_add(frame.as_usize()).expect(...), which violates the
crate's denied clippy::expect_used; update the doc example to show a lint-clean
alternative (e.g. use checked_add(...).unwrap_or(fallback) or
checked_add(...).unwrap_or_else(|| { debug_assert!(false, "..."); fallback }) or
perform a debug_assert! before an unwrap) so future maintainers copying the
example won't introduce .expect(); mention KERNEL_PHYS_BASE and ADR-0033 in the
revised snippet.

🤖 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 977-983: Update the existing safety comment for the new unsafe
block that manipulates PMM, MMU, AS_ARENA, BOOTSTRAP_AS_TABLE and
BOOTSTRAP_AS_CAP: explicitly state (a) why unsafe is required (e.g., temporarily
creating &mut aliases to just-initialized static storage and performing
low-level initialization steps that the compiler cannot prove safe), (b) the
exact invariants being upheld (single-core/cooperative context, initialization
order guarantees, momentary scope-limited &mut borrows that do not cross
scheduler start, and references to audit IDs UNSAFE-2026-0010 and
UNSAFE-2026-0014), and (c) why a safer alternative was rejected (e.g., boxing or
runtime-checked wrappers would add unacceptable boot-time dynamic overhead or
cannot express the required initialization ordering/aliasing constraints). Keep
it brief and tied to the named symbols (PMM, MMU, AS_ARENA, BOOTSTRAP_AS_TABLE,
BOOTSTRAP_AS_CAP) so reviewers can verify the invariants.

In `@docs/analysis/tasks/phase-b/T-019-task-loader.md`:
- Around line 91-92: The Markdown table cells containing the flag expressions
`USER|EXECUTE` and `USER|WRITE` are being parsed as column separators; update
those occurrences (near the row that references
`task_loader::tests::maps_image_pages_with_user_execute_flags` and
`task_loader::tests::maps_stack_with_user_write_flags`) to escape the pipe
characters (e.g., change `USER|EXECUTE` -> `USER\|EXECUTE` and `USER|WRITE` ->
`USER\|WRITE`) or wrap them in inline code backticks to prevent them being
treated as table delimiters.

In `@docs/architecture/task-loader.md`:
- Line 153: Replace the outdated test name
rolls_back_on_misaligned_image_base_va with the renamed test identifier
rejects_misaligned_image_base_va_with_pmm_byte_stable in the rollback contract
list so the document consistently references
rejects_misaligned_image_base_va_with_pmm_byte_stable alongside the other
rollback tests (e.g., rolls_back_on_cap_map_failure_mid_image_loop,
rolls_back_on_pmm_exhausted_mid_image_loop,
rolls_back_on_cap_map_failure_mid_stack_loop,
rollback_helper_zero_pages_only_drops_cap).

In `@docs/roadmap/current.md`:
- Line 7: Update the banner's HEAD placeholder in the docs/roadmap/current.md
entry that begins "**2026-05-15 update — T-019 implementation In Review on PR
`#31`; branch `t-019-task-loader`**" by replacing the literal HEAD `<TBD>` token
with the actual final commit SHA for the HEAD commit (the commit referenced as
"HEAD `<TBD>`" in the 7-commit list), ensuring the commit SHA is the one created
in the final review-round commit before merging.

In `@kernel/src/obj/mod.rs`:
- Around line 35-40: The module-level rustdoc for the obj module is now out of
date because task_loader (pub mod task_loader and the exported types LoadError
and LoadedImage) was made public; update the doc comment at the top of mod.rs to
mention the new task_loader surface, remove or revise the claim that "all v1
code here is safe Rust" if it no longer holds, and briefly describe the four
public kinds (Endpoint, Notification, Task, and the task_loader API) so the
top-level docs for tyrne_kernel::obj accurately reflect the exported symbols
Endpoint/EndpointArena/EndpointHandle,
Notification/NotificationArena/NotificationHandle, Task/TaskArena/TaskHandle,
and task_loader::{LoadError, LoadedImage}.

---

Nitpick comments:
In `@kernel/src/mm/mod.rs`:
- Around line 96-140: The forward-compat doc for phys_frame_kernel_ptr suggests
using KERNEL_PHYS_BASE.checked_add(frame.as_usize()).expect(...), which violates
the crate's denied clippy::expect_used; update the doc example to show a
lint-clean alternative (e.g. use checked_add(...).unwrap_or(fallback) or
checked_add(...).unwrap_or_else(|| { debug_assert!(false, "..."); fallback }) or
perform a debug_assert! before an unwrap) so future maintainers copying the
example won't introduce .expect(); mention KERNEL_PHYS_BASE and ADR-0033 in the
revised snippet.

🪄 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: 72c3d158-413b-4dfa-953a-759cbcc32c3c

📥 Commits

Reviewing files that changed from the base of the PR and between e09755d and 95efd62.

📒 Files selected for processing (12)

• bsp-qemu-virt/src/main.rs
• docs/analysis/tasks/phase-b/T-019-task-loader.md
• docs/architecture/boot.md
• docs/architecture/memory-management.md
• docs/architecture/task-loader.md
• docs/audits/unsafe-log.md
• docs/decisions/0029-initial-userspace-image-format.md
• docs/roadmap/current.md
• kernel/src/mm/mod.rs
• kernel/src/mm/pmm.rs
• kernel/src/obj/mod.rs
• kernel/src/obj/task_loader.rs

[cemililik]

Post-review-round-4 doc/style polish addressed in commit `74694d4`. All 6 nits verified valid against current code; all fixed in a single follow-up commit with no production-code behaviour change.

#	Location	Fix
1	`bsp-qemu-virt/src/main.rs` SAFETY comment	✅ Expanded to satisfy CLAUDE.md non-neg #2 (a)/(b)/(c): why unsafe is required (StaticCell `assume_init_*` materialisation), exact invariants (initialisation order + single-core cooperative + scope-limited `&mut` + ADR-0021 no-`&mut`-across-switch + UNSAFE-2026-0010 / 0014 audit IDs), why safer alternatives rejected (no heap → no `Box`; `OnceCell` can't express boot-flow ordering)
2	T-019 §Simulation table rows 6+7	✅ Escaped pipes: `USER\|EXECUTE` and `USER \| WRITE`
3	`docs/architecture/task-loader.md` §Rollback contract	✅ Renamed `rolls_back_on_misaligned_image_base_va` → `rejects_misaligned_image_base_va_with_pmm_byte_stable` + parenthetical noting it's the "no-rollback-needed" leg
4	`docs/roadmap/current.md` banner	✅ Resolved `HEAD ` → `95efd62` (the round-4 follow-up commit); subsequent on-branch commits framed as post-review polish rather than substantive arc
5	`kernel/src/obj/mod.rs` module rustdoc	✅ Removed stale "all v1 kernel-object code is safe Rust" claim (no longer true: `task_loader` has one audited `unsafe` for `copy_nonoverlapping` per UNSAFE-2026-0027); added §"Public surface (v1)" listing all 4 exported families
6	`kernel/src/mm/mod.rs` `phys_frame_kernel_ptr` forward-compat doc	✅ Replaced `.expect(...)` example (which would lint-fail under the kernel's `#![deny(clippy::expect_used)]`) with two lint-clean patterns: (A) `checked_add(...).unwrap_or_else(

Tests at HEAD `74694d4`: 259/259 (no test changes). All gates clean: `cargo fmt --check`, `cargo host-test`, `cargo host-clippy -D warnings`, `cargo kernel-clippy -D warnings`, `cargo kernel-build`. QEMU smoke byte-stable.

[cemililik]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

Actionable comments posted: 3

Caution

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

⚠️ Outside diff range comments (1)

kernel/src/mm/pmm.rs (1)

879-885: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Expand these test SAFETY comments to match the repo rule.

These comments cover the memory-validity part, but they still omit why unsafe is required here and why a safer alternative was rejected. The production unsafe blocks in this PR already follow that pattern, so the test blocks should be brought up to the same bar.

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

Also applies to: 896-899

🤖 Prompt for AI Agents

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

In `@kernel/src/mm/pmm.rs` around lines 879 - 885, Expand the existing SAFETY
comment above the unsafe block that calls core::ptr::write_bytes(ptr, 0xA5u8, 16
* 4096) to follow the repo rule: state explicitly (a) why unsafe is required
here (calling raw pointer write via core::ptr::write_bytes requires unsafe), (b)
the invariants being upheld (ptr is PAGE_SIZE-aligned and points to the head of
a 16-frame host-allocated Vec<u8> kept alive by _buf for the test duration, the
write length exactly matches the Vec payload so no out-of-bounds or aliasing
occurs, and no other aliasing mutable references exist), and (c) why a safer
alternative was rejected (e.g., using safe slice APIs would require reborrowing
or moving ownership of _buf which would change test semantics or cannot
guarantee the same alignment/size for the specific low-level test). Apply the
same expanded comment pattern to the other test unsafe block that writes to ptr
at the later location (lines referenced in the review).

🧹 Nitpick comments (1)

kernel/src/mm/mod.rs (1)

165-168: ⚡ Quick win

Prefer pub(crate) unless an out-of-crate caller already needs this.

The doc positions this as a kernel-internal migration shim, so exporting it as pub widens the API around the current identity-mapping assumption and makes ADR-0033 harder to change cleanly later. If the only callers are inside kernel, tightening the visibility here keeps the surface area smaller.

🤖 Prompt for AI Agents

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

In `@kernel/src/mm/mod.rs` around lines 165 - 168, The function
phys_frame_kernel_ptr is currently exported pub but should be restricted to
pub(crate) to keep the kernel-internal shim from widening the public API; change
the declaration from pub fn phys_frame_kernel_ptr(...) to pub(crate) fn
phys_frame_kernel_ptr(...) in mod.rs, then search for callers of
phys_frame_kernel_ptr to ensure none are outside the kernel crate (update or
re-export only if truly needed) and run cargo check to verify visibility changes
compile cleanly.

🤖 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/analysis/tasks/phase-b/T-019-task-loader.md`:
- Line 134: The draft contains unresolved `<TBD>` placeholders in the changelog:
replace the `<TBD>` commit placeholder in the three-commit arc (after commits
`911f2ad` and `5711756` on branch `t-019-task-loader`) with the actual final
commit hash that adds BSP wiring (the commit that introduces `USERSPACE_IMAGE`,
`USERSPACE_IMAGE_BASE_VA`, `USERSPACE_STACK_PAGES` and the `load_image`
invocation), and replace the Smoke trace HEAD `<TBD>` with the exact smoke line
emitted by the BSP wiring commit (the `tyrne: image loaded (entry = 0x...; sp =
0x...; image bytes ...; stack bytes ...; AS cap = idx ...)` string). Ensure the
UNSAFE references (`UNSAFE-2026-0025`, `UNSAFE-2026-0026`, `UNSAFE-2026-0027`)
and the Amendment/smoke-verification notes point to the final commit hash and
update the docs/architecture/task-loader.md cross-links if they reference the
placeholder; run the full gate suite (`cargo fmt --check`, `cargo host-test`,
`cargo host-clippy -D warnings`, `cargo kernel-clippy -D warnings`, `cargo
kernel-build`) and include the resulting green verification line in place of
`<TBD>`.

In `@docs/decisions/0029-initial-userspace-image-format.md`:
- Line 44: This change mutates the accepted ADR text in
docs/decisions/0029-initial-userspace-image-format.md (specifically the "Build
pipeline (B4 / T-019 — placeholder blob)" paragraph) which violates the repo's
ADR append-only policy; instead revert this inline edit and add an append-only
amendment: create a new dated revision note or a superseding ADR that references
0029 and describes the intended correction (or additional context) about the
T-019 placeholder blob and build pipeline, so the original ADR remains unchanged
while the new document records the update.

In `@kernel/src/obj/task_loader.rs`:
- Around line 897-899: The unsafe call to FakeMmu::create_address_space (seen as
mmu.create_address_space(frame(0x4000_0000))) needs a three-part justification
comment: state why unsafe is required (calling an FFI/host-only operation or
invoking an API that can break Rust safety), enumerate the invariants that make
this particular call safe (e.g., frame is page-aligned, mmu is a test-only
FakeMmu with no UB, lifetime/aliasing guarantees), and explain why a safe
alternative was not used (test-only helper, no safe wrapper exists or would be
impractical). Add the same style of comment to the other test-only unsafe call
sites mentioned (the other create_address_space and similar unsafe blocks around
task loader initialization) so every unsafe block contains the required
(a)/(b)/(c) explanation.

---

Outside diff comments:
In `@kernel/src/mm/pmm.rs`:
- Around line 879-885: Expand the existing SAFETY comment above the unsafe block
that calls core::ptr::write_bytes(ptr, 0xA5u8, 16 * 4096) to follow the repo
rule: state explicitly (a) why unsafe is required here (calling raw pointer
write via core::ptr::write_bytes requires unsafe), (b) the invariants being
upheld (ptr is PAGE_SIZE-aligned and points to the head of a 16-frame
host-allocated Vec<u8> kept alive by _buf for the test duration, the write
length exactly matches the Vec payload so no out-of-bounds or aliasing occurs,
and no other aliasing mutable references exist), and (c) why a safer alternative
was rejected (e.g., using safe slice APIs would require reborrowing or moving
ownership of _buf which would change test semantics or cannot guarantee the same
alignment/size for the specific low-level test). Apply the same expanded comment
pattern to the other test unsafe block that writes to ptr at the later location
(lines referenced in the review).

---

Nitpick comments:
In `@kernel/src/mm/mod.rs`:
- Around line 165-168: The function phys_frame_kernel_ptr is currently exported
pub but should be restricted to pub(crate) to keep the kernel-internal shim from
widening the public API; change the declaration from pub fn
phys_frame_kernel_ptr(...) to pub(crate) fn phys_frame_kernel_ptr(...) in
mod.rs, then search for callers of phys_frame_kernel_ptr to ensure none are
outside the kernel crate (update or re-export only if truly needed) and run
cargo check to verify visibility changes compile cleanly.

🪄 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: f75a0c2e-60cc-49c3-82e8-ef68b9d0cf64

📥 Commits

Reviewing files that changed from the base of the PR and between e09755d and 74694d4.

📒 Files selected for processing (12)

• bsp-qemu-virt/src/main.rs
• docs/analysis/tasks/phase-b/T-019-task-loader.md
• docs/architecture/boot.md
• docs/architecture/memory-management.md
• docs/architecture/task-loader.md
• docs/audits/unsafe-log.md
• docs/decisions/0029-initial-userspace-image-format.md
• docs/roadmap/current.md
• kernel/src/mm/mod.rs
• kernel/src/mm/pmm.rs
• kernel/src/obj/mod.rs
• kernel/src/obj/task_loader.rs

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don’t rewrite accepted ADR body content; record this as an append-only update.

These edits directly mutate the accepted ADR narrative. Per repo ADR governance, post-acceptance corrections should be captured as a dated revision/amendment note (or superseding ADR), not by rewriting original decision text.

As per coding guidelines, "ADRs are append-only; to override an old decision, write a new ADR that supersedes it."

Also applies to: 78-78

🤖 Prompt for AI Agents

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

In `@docs/decisions/0029-initial-userspace-image-format.md` at line 44, This
change mutates the accepted ADR text in
docs/decisions/0029-initial-userspace-image-format.md (specifically the "Build
pipeline (B4 / T-019 — placeholder blob)" paragraph) which violates the repo's
ADR append-only policy; instead revert this inline edit and add an append-only
amendment: create a new dated revision note or a superseding ADR that references
0029 and describes the intended correction (or additional context) about the
T-019 placeholder blob and build pipeline, so the original ADR remains unchanged
while the new document records the update.

[cemililik]

Review-round 5 addressed in commit `5078944`. All 5 findings verified valid; minimal-change fixes applied — no LoadError variants added, no production-behaviour changes, one new regression test.

P1 — `Mmu::map` trait failure-semantics contract was implicit. ✅ Fixed. `hal/src/mmu/mod.rs`'s `Mmu::map` doc-comment gained a new # Failure semantics (load-bearing contract for `unsafe`-free callers) section with three explicit clauses any BSP impl must satisfy: (1) on Err, no mapping installed at `va`; (2) on Err, `pa` is not consumed (caller retains ownership); (3) intermediate frames pulled from `frames` mid-walk may be retained by `as_`. The loader's rollback path now explicitly cites clause (2) as its safety argument. Current `QemuVirtMmu` satisfies the tightened contract by construction (leaf write is the last fallible step — see UNSAFE-2026-0025); the contract just makes the invariant explicit so future BSP impls cannot drift.

P2 — `Pmm::could_yield_pa_overlapping` doesn't consult the bitmap. ✅ Fixed. Doc-comment gained a new # Conservatism (over-approximation) section explicitly stating the bitmap is deliberately not consulted: a currently-`Allocated` frame is still reported as a yield candidate because the staged region becomes a yield candidate the moment its owner calls `free_frame`, so the soundness argument stays independent of allocation timing. New regression test `could_yield_pa_overlapping_treats_allocated_frame_as_yieldable` pins the conservative behaviour + negates against the reserved-range exclusion case.

P3a — `0x0080_0000` "mirrors kernel offset" math drift. ✅ Fixed. The previous rationale claimed userspace VA `0x0080_0000` (8 MiB) "mirrors the 0.5 MiB-into-segment offset" of the kernel image at PA `0x4008_0000`. The math doesn't check out (8 MiB ≠ 0.5 MiB). Both `bsp-qemu-virt/src/main.rs:316-322` and `docs/architecture/task-loader.md:107` rewritten with the honest rationale (page-aligned, null-page-clear, structurally aligned at 8 MiB, far below `USERSPACE_VA_LIMIT`; not algebraically derived from kernel PA layout). The arch-doc revision explicitly retires the prior "mirror" claim so it doesn't re-emerge.

P3b — Dead ADR-0030 link. ✅ Fixed. `kernel/src/obj/task_loader.rs:59` module rustdoc linked to `docs/decisions/0030-syscall-abi.md` which doesn't exist (verified via `ls`). All other repo references (T-016, T-015, T-019 user-story, fundamentals doc, current.md) treat ADR-0030 as a plain-text forward-reference. Dropped the markdown link; kept the textual reference with a "(placeholder; not yet authored)" qualifier consistent with the existing ADR-0033 placeholder convention.

P3c — Stale test count `34 → 33`. ✅ Fixed. `docs/architecture/task-loader.md:144` now matches `grep -c "#\[test\]" kernel/src/obj/task_loader.rs` = 33.

Tests at HEAD `5078944`: 260/260 (+1: the new `could_yield_pa_overlapping_treats_allocated_frame_as_yieldable` PMM regression). All gates clean: `cargo fmt --check`, `cargo host-test`, `cargo host-clippy -D warnings`, `cargo kernel-clippy -D warnings`, `cargo kernel-build`. QEMU smoke byte-stable — full demo through `tyrne: all tasks complete`.

[cemililik]

Review-round 6 addressed in commit `eb14c51`. All 5 findings verified valid against current code; minimal-change fixes applied — no production-code behaviour change, 1 visibility narrowing, 4 doc fixes.

#	Finding	Status / fix
1	T-019 user-story line 134 carried two unresolved `` placeholders (BSP-wiring commit hash + smoke-trace HEAD)	✅ Resolved both to `ae31bc8` (the round-3 BSP-wiring commit). The accompanying numbers/gates wording stays as the historical record at commit `ae31bc8`. `grep -r '' docs/ kernel/ bsp-qemu-virt/ hal/` confirms no remaining placeholders
2	ADR-0029 §"Build pipeline" + §"Host-side loader tests" — in-place edit of an Accepted ADR (196d3fb rewrote placeholder bytes from `[0x40, 0x00, 0x80, 0xd2, ...]` to `[0x40, 0x05, 0x80, 0x52, ...]` after Accept) violates CLAUDE.md non-neg #5 + ADR README §Format append-only policy	✅ Reverted both byte literals to Accept-state form. Added new §Revision notes section before §References with a 2026-05-16 entry decoding both literals (Accept-state → `MOVZ x0, #2; RET`; corrected → `MOVZ w0, #42; RET`), citing `bsp-qemu-virt/src/main.rs::USERSPACE_IMAGE` as canonical source, noting the discrepancy is non-load-bearing (placeholder not executed in B4). Kernel-source bytes unchanged
3	`kernel/src/obj/task_loader.rs` test `unsafe` `FakeMmu::create_address_space` sites missing (a)/(b)/(c) discipline	✅ Expanded `fixture` helper's SAFETY (canonical site) with full (a) why-unsafe / (b) invariants / (c) why-safer-rejected. Cross-referenced 3 peer sites by name (`missing_derive_surfaces_...`, `FailingMapMmu::create_address_space` body, `fixture_with_failing_mmu`). `FailingMapMmu` body separately documents the `unsafe_op_in_unsafe_fn` lint requirement
4	`kernel/src/mm/pmm.rs` 2 test `unsafe` blocks (write_bytes pre-poison + per-byte zero-fill assert) — one-liners only covering invariants (b)	✅ Both expanded to full (a)/(b)/(c) — naming `aligned_backing`'s host-allocation contract for invariants and rejecting `from_raw_parts`-based slice forms (would re-introduce same audit obligation)
5	`kernel/src/mm/mod.rs::phys_frame_kernel_ptr` — `pub` widens public API unnecessarily	✅ Narrowed `pub` → `pub(crate)`. `grep -rn phys_frame_kernel_ptr kernel/ bsp-qemu-virt/ hal/ test-hal/` confirms only one in-crate caller (`task_loader::load_image`); no external caller exists

Tests at HEAD `eb14c51`: 260/260 (no test changes — all fixes are doc/visibility). All gates clean: `cargo fmt --check`, `cargo host-test`, `cargo host-clippy -D warnings`, `cargo kernel-clippy -D warnings`, `cargo kernel-build`. QEMU smoke byte-stable — full demo through `tyrne: all tasks complete`.