Development - #6
Conversation
Draft the ADR that closes UNSAFE-2026-0012 by reshaping Scheduler::ipc_send_and_yield / Scheduler::ipc_recv_and_yield so they take raw pointers (*mut EndpointArena, *mut IpcQueues, *mut CapabilityTable) instead of &mut references. With raw pointers as parameters, no &mut alive across cpu.context_switch is visible to the compiler; inside the scheduler each pointer is momentarily dereferenced to a &mut only outside the switch window. The &mut self borrow that persists across the switch is a single intra-struct split-borrow owned by the scheduler itself, sound by construction. Four options considered: - A: raw-pointer parameters (chosen) - B: scheduler owns the arenas (rejected — collapses layering, breaks tests, fights per-CPU locking in Phase C) - C: continuation-passing (rejected — captured &mut is the same hazard; closure erasure needs an allocator or bloats binary) - D: per-task TaskContext extensions (rejected — relocates the hazard rather than eliminating it; violates ADR-0020 contract) Consequences, including the trade-off of losing compile-time non-aliasing guarantees on the function signatures (mitigated by doc-comment contract + `cargo miri test` + narrow audited helpers), are spelled out in the Negative subsection. ADR index updated. UNSAFE-2026-0012 entry status narrative points at ADR-0021 and T-006 (was "future ADR"). T-006 review history records the Proposed state. Refs: ADR-0013, ADR-0021 Audit: UNSAFE-2026-0012 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…liasing fully Self-review caught a flaw in the initial draft. The original Option A kept `&mut self` on the bridge methods and argued the remaining "intra-struct borrow" was sound. That argument was wrong: a `&mut Scheduler` produced by `SCHED.assume_init_mut()` lives for the full duration of the bridge method, which spans cpu.context_switch. When the second task resumes and calls SCHED.assume_init_mut() again, two live &mut Scheduler to the same referent exist — the same aliasing UB as UNSAFE-2026-0012, merely relocated from the arenas to the scheduler itself. Retiring UNSAFE-2026-0012 requires the bridge to never produce a &mut Scheduler that crosses the switch; therefore the bridge cannot accept &mut self. Option A is promoted to "all-pointers, including self": bridge entry points are unsafe fn free functions over *mut Scheduler<C>, with momentary &mut materialisation strictly outside the switch window. Method (dot-call) syntax at the BSP is lost and replaced by a thin BSP-level wrapper that packages the argument set for each task body. Sections updated: - Considered options § Option A — description reshaped - Decision outcome — the chosen option's justification and the Option B "superset-not-alternative" framing - Consequences § Positive — UNSAFE-2026-0012 retires *in full* - Consequences § Negative — new con for loss of method syntax - Pros and cons § Option A / Option B — updated accordingly - New Revision notes section records the correction Status unchanged (Proposed); awaiting maintainer sign-off before T-006's implementation commits begin. T-006 review-history row appended with the correction summary. Refs: ADR-0013, ADR-0021 Audit: UNSAFE-2026-0012 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Maintainer signed off on the revised ADR-0021. The chosen shape — Option A "all-pointers, including self" — promotes the IPC bridge from &mut self methods to unsafe fn free functions over *mut Scheduler<C>, with momentary &mut materialisation strictly outside the cpu.context_switch window. This retires UNSAFE-2026-0012 in full with no residual aliasing window. ADR index updated. T-006 acceptance criterion 1 checked off; review history notes the sign-off and the six projected implementation commits to follow. Refs: ADR-0013, ADR-0021 Audit: UNSAFE-2026-0012 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Promote Scheduler::yield_now / ipc_send_and_yield / ipc_recv_and_yield
from &mut-self methods to `unsafe fn` free functions over
*mut Scheduler<C>, following ADR-0021. Every &mut that the bridge
materialises (to Scheduler, EndpointArena, IpcQueues, CapabilityTable)
is now scoped to a single inner block per phase that ends strictly
before cpu.context_switch and is reacquired strictly after the switch
returns; no such &mut is alive across the switch. UNSAFE-2026-0012 is
fully retired with no residual aliasing window.
kernel/src/sched/mod.rs:
- remove methods yield_now, ipc_send_and_yield, ipc_recv_and_yield
from impl Scheduler<C>
- add free `pub unsafe fn`s with the same names at module scope, each
taking *mut pointers and acquiring momentary &muts in a single
inner unsafe block per phase
- two scheduler tests updated to pass core::ptr::from_mut(&mut sched)
to yield_now; FakeCpu::context_switch is a no-op so the single-
threaded test harness has no aliasing concern
- Shared Safety Contract comment block at the top of the raw-pointer
bridge documents pointer validity, non-aliasing across the switch,
and IrqGuard discipline — each function references this single
contract rather than restating it
bsp-qemu-virt/src/main.rs:
- add `StaticCell::as_mut_ptr()` inherent helper: a plain pointer
cast via UnsafeCell::get().cast(), no &mut materialisation anywhere
- task_a / task_b rewritten to call the free-function bridge with
*mut pointers produced by StaticCell::as_mut_ptr(); no
assume_init_mut on SCHED / EP_ARENA / IPC_QUEUES / TABLE_* at any
call site
- SAFETY comments use standard "// SAFETY:" prefix for
clippy::undocumented_unsafe_blocks compatibility; ADR-0021 cite moved
into the comment body ("SAFETY: per ADR-0021 — …")
UNSAFE-2026-0012 retirement + new audit entries for
StaticCell::as_mut_ptr and the scheduler momentary-borrow pattern land
in a follow-up docs(audits) commit so the mechanical audit-log update
is reviewable on its own.
TaskArena local → StaticCell global migration (K3-11) likewise lands
in a follow-up feat(bsp) commit so this refactor keeps a single
orthogonal concern.
Checks:
- cargo fmt --all -- --check: clean
- cargo host-clippy: clean with -D warnings
- cargo kernel-clippy: clean
- cargo host-test: 109 tests pass (75 kernel + 34 test-hal)
- cargo kernel-build: clean
- QEMU smoke: trace matches A6 exactly
Refs: ADR-0013, ADR-0021
Audit: UNSAFE-2026-0012
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Move `TaskArena` from a `kernel_entry` stack local into `static TASK_ARENA: StaticCell<TaskArena>`, matching the pattern used by `EP_ARENA`, `IPC_QUEUES`, `TABLE_A`, and `TABLE_B`. The v1 demo still does not read the arena after `create_task` returns the two `TaskHandle`s, but keeping it on the stack diverged from ADR-0016's "arenas belong to the kernel" framing and would have forced a second round of BSP static-cell churn the moment a Phase B task-destruction or status-query API needs to look up a `Task` object by its handle. - static TASK_ARENA lives beside the other StaticCells - kernel_entry writes it via `(*TASK_ARENA.0.get()).write(TaskArena::default())` before `start()`, consistent with how the other arenas are initialised - the two `create_task` calls run inside a single unsafe block whose momentary `&mut TaskArena` is scoped tightly to those calls and drops before the scheduler starts — trivially satisfies ADR-0021's non-aliasing-across-switch invariant (no switch has run yet) Closes Kova-3 entry K3-11 listed in the security-review follow-ups absorbed into phase-b.md §B0. Checks: - cargo fmt --all -- --check: clean - cargo host-clippy / kernel-clippy: clean - cargo host-test: 109 tests pass - cargo kernel-build: clean - QEMU smoke: A6 trace unchanged Refs: ADR-0013, ADR-0016, ADR-0021 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Close out the audit-log side of T-006 / ADR-0021. UNSAFE-2026-0012 — `&mut` aliasing on shared kernel state across cooperative yields — moved from Active to Removed (commit f9b72f8). The status block records the resolution: the scheduler's IPC bridge is now a set of `unsafe fn` free functions over *mut Scheduler<C>; every cross-switch &mut has been eliminated; QEMU smoke matches the A6 baseline. UNSAFE-2026-0013 — `StaticCell::as_mut_ptr` BSP helper. The BSP's inherent method that turns a `&StaticCell<T>` into a `*mut T` without materialising a &mut, by way of a plain `UnsafeCell::get().cast::<T>()`. This is the foundation on which UNSAFE-2026-0012's retirement rests. UNSAFE-2026-0014 — Scheduler free-function momentary `&mut` pattern. Documents the narrow inner blocks inside `yield_now`, `ipc_send_and_yield`, and `ipc_recv_and_yield` (plus the `create_task` call site at the BSP's `kernel_entry`) that dereference *mut into &mut strictly outside cpu.context_switch windows. Invariants enumerated; alternatives (NonNull<T>, addr_of_mut!) considered and rejected with reasons. Refs: ADR-0013, ADR-0021 Audit: UNSAFE-2026-0012, UNSAFE-2026-0013, UNSAFE-2026-0014 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Implementation of T-006 (raw-pointer scheduler API refactor) is complete. Four commits landed the change: - 3b8aa34 ADR-0021 Accepted - f9b72f8 sched refactor + BSP adoption (UNSAFE-2026-0012 closed) - 1746bc8 TaskArena → StaticCell global (K3-11) - a1310ae UNSAFE-2026-0012 → Removed; 0013 / 0014 recorded All seven acceptance criteria and all ten definition-of-done checks are met: fmt clean, host-clippy + kernel-clippy clean, 109 host tests green, kernel-build clean, QEMU smoke reproduces the A6 trace unchanged, ADR-0021 Accepted, UNSAFE-2026-0012 audit entry moved to Removed with commit SHA, two new audit entries for the helper patterns. current.md points at T-006 In Review; notes the audit-log status shift and identifies T-007 (idle task + typed SchedError::Deadlock) as the next task to open inside B0. phase-b task index row updated. Task promotes to `Done` on maintainer sign-off. Refs: ADR-0013, ADR-0021 Audit: UNSAFE-2026-0012 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 40 minutes and 57 seconds. ⌛ 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 (3)
📝 WalkthroughWalkthroughThe PR implements ADR-0021 and ADR-0022, refactoring the scheduler's IPC and yield operations from methods to unsafe free functions taking raw pointers, adding an idle task to prevent scheduler deadlock, introducing typed Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application Task
participant Sched as Scheduler<br/>(via unsafe fn)
participant CPU as CPU Context<br/>Switch
participant Idle as Idle Task
participant Queue as IPC Ready Queue
rect rgba(200, 150, 100, 0.5)
Note over App,Queue: Old Flow: Deadlock Path
App->>Sched: ipc_recv_and_yield<br/>(attempts to receive)
Sched->>Queue: enqueue blocked task
Sched->>Queue: check ready queue
Queue-->>Sched: empty!
Sched->>Sched: panic() — deadlock!
end
rect rgba(100, 200, 100, 0.5)
Note over App,Queue: New Flow: With Idle Task
App->>Sched: ipc_recv_and_yield<br/>(*mut Scheduler)
Sched->>Queue: enqueue blocked task
Sched->>Queue: check ready queue
Queue-->>Sched: Idle is enqueued ✓
Sched->>CPU: context_switch<br/>(no &mut refs alive)
CPU->>Idle: resume Idle task
Idle->>Sched: yield_now<br/>(*mut Scheduler)
Sched->>CPU: context_switch<br/>(back to App)
CPU->>App: resume with<br/>Ok(RecvOutcome::Pending)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoImplement raw-pointer scheduler IPC-bridge API (ADR-0021, T-006)
WalkthroughsDescription• Implement raw-pointer scheduler IPC-bridge API per ADR-0021 to eliminate aliasing hazard UNSAFE-2026-0012 • Replace Scheduler methods with unsafe fn free functions taking *mut pointers for all shared state • Add StaticCell::as_mut_ptr() helper to safely produce raw pointers without materialising &mut references • Migrate TaskArena from stack-local to global StaticCell storage, matching kernel-object arena pattern • Update audit log with UNSAFE-2026-0012 retirement and two new audit entries (UNSAFE-2026-0013, UNSAFE-2026-0014) • Update task T-006 status to In Review with all acceptance criteria met Diagramflowchart LR
A["UNSAFE-2026-0012<br/>Aliasing hazard"] -->|"ADR-0021<br/>raw-pointer API"| B["Scheduler free-functions<br/>yield_now, ipc_send_and_yield,<br/>ipc_recv_and_yield"]
B -->|"*mut Scheduler<C><br/>*mut arenas/queues/tables"| C["No &mut across<br/>cpu.context_switch"]
D["StaticCell::as_mut_ptr()<br/>helper"] -->|"produces *mut<br/>without &mut"| E["BSP task bodies<br/>task_a, task_b"]
E -->|"call bridge with<br/>raw pointers"| C
F["TaskArena<br/>stack-local"] -->|"migrate to<br/>StaticCell global"| G["Uniform kernel-object<br/>storage pattern"]
C -->|"retire"| H["UNSAFE-2026-0012<br/>Removed"]
B -->|"introduce"| I["UNSAFE-2026-0013<br/>UNSAFE-2026-0014"]
File Changes1. bsp-qemu-virt/src/main.rs
|
Code Review by Qodo
1. ipc_send_and_yield unsafe lacks alternatives
|
There was a problem hiding this comment.
Code Review
This pull request implements ADR-0021, refactoring the scheduler's IPC bridge from &mut self methods to unsafe fn free functions that utilize raw pointers. This architectural change eliminates the &mut aliasing hazard across cooperative context switches, effectively retiring the UNSAFE-2026-0012 audit entry. Key changes include the introduction of a StaticCell::as_mut_ptr helper in the BSP, the migration of TaskArena to a global static cell, and the implementation of a momentary &mut pattern within the scheduler to ensure memory safety. Documentation, audit logs, and roadmap files have been updated to reflect the completion of task T-006. I have no feedback to provide as there are no review comments to assess.
| // Pre-switch work — momentary &muts, dropped before the switch. | ||
| // SAFETY: caller contract — all four pointers are valid, distinct, and | ||
| // exclusively-owned for the duration of this inner block. Each `&mut` | ||
| // materialised in the tuple below lives only inside this block and is | ||
| // dropped before the `yield_now` call site. Audit: UNSAFE-2026-0014. | ||
| let (outcome, needs_yield) = unsafe { | ||
| let s: &mut Scheduler<C> = &mut *sched; | ||
| let arena_ref: &mut EndpointArena = &mut *ep_arena; | ||
| let queues_ref: &mut IpcQueues = &mut *queues; | ||
| let table_ref: &mut CapabilityTable = &mut *caller_table; | ||
|
|
||
| // Resolve the endpoint handle up-front so it remains valid even after | ||
| // `ipc_send` mutates the endpoint state. | ||
| let ep_handle = Scheduler::<C>::resolve_ep_cap(table_ref, ep_cap)?; | ||
|
|
||
| let outcome = ipc_send(arena_ref, queues_ref, ep_cap, table_ref, msg, transfer)?; | ||
|
|
||
| let needs_yield = if outcome == SendOutcome::Delivered { | ||
| s.unblock_receiver_on(ep_handle); | ||
| true | ||
| } else { | ||
| false | ||
| }; | ||
|
|
||
| (outcome, needs_yield) | ||
| }; // All `&mut`s drop here. | ||
|
|
||
| // Switch window — no `&mut` to any shared state is alive. | ||
| if needs_yield { | ||
| // SAFETY: `sched` still satisfies the caller contract; we have just | ||
| // released our `&mut` so the re-entrant `yield_now` can acquire its | ||
| // own momentary `&mut` without overlapping ours. Audit: UNSAFE-2026-0014. | ||
| unsafe { | ||
| yield_now(sched, cpu)?; | ||
| } | ||
| } |
There was a problem hiding this comment.
1. ipc_send_and_yield unsafe lacks alternatives 📘 Rule violation ⛨ Security
The new raw-pointer scheduler bridge introduces unsafe blocks whose // SAFETY: comments describe invariants but do not document why safer alternatives were rejected. This fails the required per-unsafe justification standard and makes future reviews of the scheduler’s soundness harder.
Agent Prompt
## Issue description
New `unsafe` blocks in the raw-pointer scheduler bridge are missing the “rejected safer alternatives” portion of the required unsafe justification.
## Issue Context
The bridge moved from `&mut self` methods to `unsafe fn` free functions taking raw pointers (ADR-0021). Each pointer-deref `unsafe` block should explicitly document why a safe signature/approach isn’t used (and why other alternatives are not chosen), in addition to invariants.
## Fix Focus Areas
- kernel/src/sched/mod.rs[447-482]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| pub unsafe fn yield_now<C: ContextSwitch + Cpu>( | ||
| sched: *mut Scheduler<C>, | ||
| cpu: &C, | ||
| ) -> Result<(), SchedError> { | ||
| // Pre-switch work — momentary &mut Scheduler, dropped before the switch. | ||
| let (current_idx, next_idx) = { | ||
| // SAFETY: caller contract — `sched` is valid, exclusively-owned for | ||
| // the duration of this inner block, and this `&mut` does not cross | ||
| // the `cpu.context_switch` call below because the block ends first. | ||
| // Audit: UNSAFE-2026-0014. | ||
| let s = unsafe { &mut *sched }; | ||
|
|
There was a problem hiding this comment.
2. Bootstrap &mut aliasing ub 🐞 Bug ⛨ Security
kernel_entry calls Scheduler::start(&mut self, ...), and start performs cpu.context_switch while the &mut Scheduler is still live on the bootstrap stack; when the first task runs, the raw-pointer bridge re-materialises another &mut Scheduler from *mut Scheduler, creating two live mutable references to the same scheduler (UB). This contradicts the PR’s claim that no &mut crosses the context-switch boundary and makes the UNSAFE-2026-0012 “Removed” status premature.
Agent Prompt
## Issue description
`Scheduler::start(&mut self, ...)` calls `cpu.context_switch(...)` while its `&mut Scheduler` receiver is live on the bootstrap stack frame. After the switch, tasks call the new raw-pointer bridge, which momentarily materialises `&mut Scheduler` from `*mut Scheduler`, creating two live `&mut` aliases to the same scheduler.
## Issue Context
This undermines the ADR-0021 objective (“no `&mut` alive across `cpu.context_switch`”) and makes the UNSAFE-2026-0012 retirement claim incorrect.
## Fix Focus Areas
- kernel/src/sched/mod.rs[227-267]
- kernel/src/sched/mod.rs[308-417]
- bsp-qemu-virt/src/main.rs[489-501]
- docs/audits/unsafe-log.md[147-160]
## Suggested implementation direction
1. Introduce a raw-pointer bootstrap entry point (e.g. `pub unsafe fn start(sched: *mut Scheduler<C>, cpu: &C) -> !`) in the ADR-0021 raw-pointer bridge section.
2. Ensure this new start function:
- Does all scheduler-state mutation in a narrow inner block using `&mut *sched`,
- Drops that `&mut` before calling `cpu.context_switch`,
- Calls `cpu.context_switch` using raw-pointer access to `(*sched).contexts` (like `yield_now` does),
- Returns `!` (or loops forever) to match the “bootstrap never resumes” reality.
3. Update `kernel_entry` to call the new raw-pointer start with `SCHED.as_mut_ptr()` (avoid `assume_init_mut()` entirely at the bootstrap switch site).
4. Update the unsafe-log entry: either (a) keep UNSAFE-2026-0012 active until start is refactored, or (b) extend UNSAFE-2026-0014 / add a new audit entry that explicitly covers the bootstrap-start switch invariants.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
- sched::start is now a raw-pointer free function, matching yield_now /
ipc_send_and_yield / ipc_recv_and_yield; no &mut Scheduler ever crosses
cpu.context_switch on any scheduler entry point.
- yield_now / unblock_receiver_on: turn silent `let _ = ready.enqueue(...)`
into a documented invariant panic so a regression (preemption / SMP /
resized queue) fails loudly instead of losing a task.
- Add debug_assert_ne!(current_idx, next_idx) before both context-switch
sites; zero release cost, catches split-borrow regressions early.
- ipc: rename sync_generation → reset_if_stale_generation and add a
debug_assert catching silent Capability drop if a future destroy path
forgets to drain SendPending/RecvComplete { cap: Some(_) }.
- Docstrings: SendOutcome::Enqueued flags that the bridge does not yield
and the caller must; ipc_notify spells out that waiter wake-up is not
wired and any future blocking-wait path must grow an unblock step.
- Scheduler module doc makes the "no other &mut to shared referents while
a task is mid-bridge" rule explicit as a global invariant.
- ADR-0021: follow-up rider documents the start() raw-pointer reshape and
the global-invariant clarification.
- UNSAFE-2026-0012 retirement gains a rider noting the start() exception
existed in f9b72f8 and is closed by this commit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mini-retro filed at T-006 closure. Captures the ADR-0021 draft-to-revision arc (parameters-only → all-pointers including `self`), the post-In-Review second pass that caught three silent-failure gaps (closed in 7eaa10a), and the `start()` exception + its follow-up fix. Adjustments: tighten the ADR pre-accept checklist with an explicit "trace who holds &mut across the switch/lock" step, and formalise a post-In-Review second-read gate in the task DoD. No plan churn; next step remains T-007 (ADR-0022 + idle task). Refs: ADR-0013, ADR-0021 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the two kernel-liveness panics in the scheduler IPC bridge with (a) a BSP-registered idle task using the existing add_task + yield_now surface (no new unsafe, no new scheduler API), and (b) a typed SchedError::Deadlock + IpcError::PendingAfterResume pair. start()'s empty-queue panic is kept — boot-time programming errors stay at panic! where the invariant is violated. Informs T-007. Refs: ADR-0022, ADR-0019, ADR-0021 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
As the Umbrix kernel scheduler, register an idle task and introduce typed SchedError::Deadlock + IpcError::PendingAfterResume in place of the two runtime panics in ipc_recv_and_yield. 9 acceptance criteria; phase B, milestone B0. ADR-0022 promoted from Proposed → Accepted in the same commit so T-007 can begin implementation. Refs: ADR-0022, ADR-0019, ADR-0017 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…(T-007) Replace the two runtime panics in ipc_recv_and_yield with typed returns per ADR-0022: - Empty ready queue after blocking current → Err(SchedError::Deadlock). Pre-block state (task_states[current], current) is restored before return so the caller's scheduler view is unchanged. - Release-build fall-through on the resume-path Pending invariant → Err(SchedError::Ipc(IpcError::PendingAfterResume)). The debug_assert! stays as a loud test-mode complement. start()'s empty-queue panic is kept — boot-time programming error stays at panic! where the invariant is violated (ADR-0022 §Decision outcome). BSP idle registration lands in the next commit; without it, SchedError::Deadlock becomes the everyday path instead of a defensive return. Kernel host tests (75) remain green; QEMU smoke is deferred until the BSP side catches up. Refs: ADR-0022 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds TASK_IDLE_STACK, fn idle_entry() -> !, and a create_task + add_task for idle at the end of the kernel-entry bring-up. Idle is registered last so the FIFO dispatch order still runs Task B first (matching the A6 trace). With idle registered, SchedError::Deadlock is structurally unreachable in the v1 workload — every ipc_recv_and_yield that would have panicked now finds idle in the ready queue. Idle's body is spin_loop + yield_now, not wfi + yield_now. The ADR-0022 Revision notes document the reason: v1 has no IRQ source (timer wiring is T-009), so wfi would suspend the core indefinitely the first time FIFO dispatches idle between two ready tasks — which happens on the first inter-task yield in the demo. When T-009 lands, idle's body becomes wfi + yield_now with no other call-site changes. QEMU smoke reproduces the A6 five-line trace byte-for-byte; 75 kernel host tests green; cargo fmt / host-clippy / kernel-clippy all clean. No new `unsafe` blocks introduced. Refs: ADR-0022 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two new kernel host tests covering the ADR-0022 typed-error contract: - ipc_recv_and_yield_returns_deadlock_when_ready_queue_empty — blocks the sole registered task on an endpoint with no idle registered, asserts Err(SchedError::Deadlock), and verifies the scheduler state was restored (current, task_states, ready queue unchanged). - ipc_recv_and_yield_resume_pending_returns_typed_err — a purpose-built ResetQueuesCpu forces the pathological "resumed without delivery" scenario by zeroing IpcQueues during context_switch; asserts the bridge returns Err(SchedError::Ipc(IpcError::PendingAfterResume)) instead of letting Ok(Pending) propagate to the caller. While writing the second test, the resume-path debug_assert! was found to be in tension with the typed-error contract (the assert fires before the typed return, making the path untestable without cfg hacks). It is dropped — the typed Err IS the loud signal, and an untestable assert rots. ADR-0022 Revision notes gains a second rider documenting this. 77 kernel + 34 test-hal = 111 host tests green; QEMU smoke reproduces the A6 five-line trace byte-for-byte. Refs: ADR-0022 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implementation landed in 029d066 / 25cfaf4 / 8110cc5. 111 host tests green (+2 from T-007); QEMU smoke matches A6 baseline; fmt/clippy clean; no new unsafe. ADR-0022 acquired two in-place riders during implementation — WFI deferred to T-009, and resume-path debug_assert dropped as redundant with typed Err. Refs: ADR-0022 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Independent-agent second-read found four small gaps before T-006 and T-007 can safely be marked Done. All cosmetic or invariant-assert hardening; no behavioural change. T-007: - SchedError gains #[non_exhaustive] — ADR-0022 Consequences/Neutral already claimed this, code did not match. - ipc_recv_and_yield gains debug_assert_eq!(prior_state, Ready) before blocking the current task — a running task's slot must be Ready per ADR-0019; guards against a scheduler-invariant regression silently leaking into the Deadlock state-restore path. - Test pending_after_resume asserts ep generation == 0 explicitly — the test depends on ResetQueuesCpu's fresh IpcQueues matching the endpoint's generation; previously an implicit invariant. - Task file §Approach step 3 had a stale "Keep the debug_assert as test-mode complement" sentence that contradicted the ADR-0022 second rider (which dropped the assert). Text now reflects shipped code. - New §Deferred follow-ups section routes the symmetric-state-restore test for ipc_send_and_yield to T-011 and the idle .expect softening to the future preemption ADR — no loose ends. T-006: - ADR-0021 Revision notes reordered into chronological order; the stale "Status unchanged — Proposed" sentence removed (ADR is Accepted). - Task-file DoD checkbox gained the actual commit SHA (92e5acd) in place of the "(pending — follows this status change)" parenthetical. - Review-history tabulates the 7eaa10a post-review fix pass and this close-out second-read, so the final state is self-documenting. Verification: 77 kernel + 34 test-hal = 111 host tests green; QEMU smoke matches the A6 five-line trace byte-for-byte; cargo fmt / host-clippy / kernel-clippy all clean. Refs: ADR-0021, ADR-0022 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/analysis/tasks/phase-b/T-007-idle-task-typed-deadlock.md`:
- Around line 61-87: Update the Approach/Design-notes to match the landed
implementation: describe idle_entry as using core::hint::spin_loop() followed by
sched::yield_now(...) (not cpu.wait_for_interrupt()), rename the two test
references to ipc_recv_and_yield_returns_deadlock_when_ready_queue_empty and
ipc_recv_and_yield_resume_pending_returns_typed_err to match the actual kernel
tests, and replace the tentative FIFO commentary with a definitive sentence that
the scheduler add_task order is B → A → idle (idle added last so B is dispatched
first), referencing the create_task/add_task sequence observed in bsp-qemu-virt
main and the idle_entry and ipc_recv_and_yield symbols for clarity.
In `@docs/roadmap/phases/phase-b.md`:
- Around line 52-53: Update the T-006 row in docs/roadmap/phases/phase-b.md for
the entry "T-006 — Raw-pointer scheduler API refactor + TaskArena global
migration" so its status matches the task index and T-007 front-matter (change
"In Progress (opened 2026-04-22)" to "In Review (opened 2026-04-22)"), or
remove/annotate the row if you intend to keep T-006 transition out of this PR's
scope to avoid the roadmap/index mismatch.
In `@kernel/src/sched/mod.rs`:
- Around line 152-165: The Deadlock enum variant documentation overstates
rollback: while ipc_recv_and_yield restores the caller's scheduler state, it
does not roll the endpoint state from RecvWaiting back to Idle (ipc_recv's Phase
1 transition). Update the docs for Deadlock (and the corresponding comment on
ipc_recv_and_yield/ipc_recv) to state clearly that scheduler state is restored
but endpoint state remains RecvWaiting and recovery is out of scope for v1, or
alternatively implement a rollback in ipc_recv/ipc_recv_and_yield that sets the
endpoint back to Idle on the Deadlock path; reference the Deadlock variant,
ipc_recv_and_yield, and the Phase 1 ipc_recv transition when making the change.
- Around line 588-594: Update the doc comment describing
SchedError::Ipc/IpcError::PendingAfterResume to remove the claim that a
debug_assert! fires; instead state that the resume-path Pending condition is
indicated solely via the typed IpcError::PendingAfterResume return (the
redundant debug_assert! was removed per the inline note). Edit the text
referencing the resume-Pending behavior so it no longer promises a debug trap in
debug builds and explicitly notes the ADR-0022 typed return is the only signal.
🪄 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: d7bcf44f-000f-4a5e-8ab9-d5139b83b62a
📒 Files selected for processing (14)
bsp-qemu-virt/src/main.rsdocs/analysis/reviews/business-reviews/2026-04-22-T-006-mini-retro.mddocs/analysis/reviews/business-reviews/README.mddocs/analysis/tasks/phase-b/README.mddocs/analysis/tasks/phase-b/T-006-raw-pointer-scheduler-api.mddocs/analysis/tasks/phase-b/T-007-idle-task-typed-deadlock.mddocs/audits/unsafe-log.mddocs/decisions/0021-raw-pointer-scheduler-ipc-bridge.mddocs/decisions/0022-idle-task-and-typed-scheduler-deadlock.mddocs/decisions/README.mddocs/roadmap/current.mddocs/roadmap/phases/phase-b.mdkernel/src/ipc/mod.rskernel/src/sched/mod.rs
Two issues filed: **Issue 1 (real):** The raw-pointer bridge's per-block SAFETY comments cited audit entries and invariants but omitted the unsafe-policy §1 "rejected safer alternatives" third leg — that rationale lived only in the UNSAFE-2026-0014 audit entry, not in the code. Added a named "Rejected safer alternatives" subsection to the Shared safety contract enumerating `&mut self` / Mutex / Option-B / continuation alternatives, and extended every bridge SAFETY block to cite it alongside the block-local invariants. The comment density is now policy-compliant: every block states invariants + rejected alternatives + audit tag. **Issue 2 (duplicate — stale):** The issue reported that `Scheduler::start(&mut self, ...)` aliased across cpu.context_switch and that UNSAFE-2026-0012 retirement was incorrect. That exact bug was closed in 7eaa10a (T-006 post-review fix): start is now a raw-pointer free function, kernel_entry uses SCHED.as_mut_ptr(), and UNSAFE-2026-0012 has a Post-review rider documenting the closure. The line ranges in the issue reference pre-7eaa10a file contents. No code change needed; this commit only notes the verification in-line via the Shared safety contract's expanded prose. Also four inline review comments resolved: 1. T-007 task file §Approach / §Design notes: idle_entry uses core::hint::spin_loop(), not cpu.wait_for_interrupt() (WFI deferred to T-009 per ADR-0022 first rider); test names updated to the actual symbols (ipc_recv_and_yield_returns_deadlock_when_ready_queue_empty and ipc_recv_and_yield_resume_pending_returns_typed_err); tentative FIFO commentary replaced with definitive B → A → idle registration order matching the shipped bsp-qemu-virt/src/main.rs kernel_entry. 2. phase-b.md T-006 row: "In Progress" → "In Review" to match the phase-b task index and T-007 front-matter (the mismatch would have confused a returning reader). 3. SchedError::Deadlock doc gained a §Rollback scope subsection explaining that scheduler state is restored but endpoint state remains RecvWaiting (Phase 1's ipc_recv transition is not rolled back) — recovery semantics are out of scope for v1 since the variant is structurally unreachable. The ipc_recv_and_yield # Errors section and the Phase 2 inline comment reference the rollback-scope note. 4. ipc_recv_and_yield # Errors description of PendingAfterResume no longer promises a debug_assert! fires — that assert was dropped in 8110cc5 and the typed Err is the sole signal per ADR-0022 Revision notes second rider. Verification: 77 kernel + 34 test-hal = 111 host tests green; QEMU smoke matches the A6 five-line trace byte-for-byte; cargo fmt / host-clippy / kernel-clippy / kernel-build all clean. Refs: ADR-0021, ADR-0022 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address inline + nitpick comments on PR #12 from coderabbitai and gemini-code-assist. Each finding verified against current code; 8 applied, 2 skipped with reason. ## Applied (8) 1. **B1 mini-retro UNSAFE-2026-0021 status alignment** ([docs/analysis/reviews/business-reviews/2026-05-06-B1-smoke-regression.md](docs/analysis/reviews/business-reviews/2026-05-06-B1-smoke-regression.md)) — clarify that adjustment item 6 is *partially complete*: 0019/0020 received post-T-014 Amendments while 0021 stayed in its no-verification state; remove the inconsistency between line 97 and lines 124-126. 2. **track-j-hygiene fenced code blocks gain language tags** ([docs/analysis/reviews/code-reviews/2026-05-06-full-tree/track-j-hygiene.md](docs/analysis/reviews/code-reviews/2026-05-06-full-tree/track-j-hygiene.md)) — three blocks now tagged `sh` / `toml` / `sh` (lines 11-16, 44-48, 58-63). Closes markdownlint MD040. 3. **T-014 review-history broken table row + code block fix** ([docs/analysis/tasks/phase-b/T-014-idle-dispatch-fallback.md](docs/analysis/tasks/phase-b/T-014-idle-dispatch-fallback.md)) — the row at line 115 had a fenced code block embedded inside a table cell, breaking markdown rendering. Compressed the row to a one-liner with closing pipe; moved the smoke trace + audit-log Amendments paragraph out of the table into a new `## Verification artefacts` section directly below. Closes markdownlint MD055. 4. **UNSAFE-2026-0014 register_idle Amendment** ([docs/audits/unsafe-log.md](docs/audits/unsafe-log.md)) — append-only Amendment names `register_idle` as the fourth sanctioned site of the momentary-`&mut Scheduler<C>` pattern (alongside `add_task`, `start_prelude`, and the IPC bridge). Records the post-T-014 smoke verification status and the unconditional `assert!` discipline (see #6 below). Mirrors the 2026-04-27 `start_prelude` Amendment and the 2026-04-28 `irq_entry` Amendment shape. 5. **URL slug fix on the 8 lines added in this PR** (kernel/src/sched/mod.rs lines 42, 43, 196, 231, 465, 1479; bsp- qemu-virt/src/main.rs lines 260, 705) — `cemililik/TyrneOS` → `cemililik/Tyrne`. Pre-existing `TyrneOS` URLs in unmodified parts of these files (and the other 60+ across 27 files) stay untouched — they are scoped to a separate post-B1-closure doc-fix sweep per Track J's J-NB1 finding. 6. **register_idle hardened: `debug_assert!` → unconditional `assert!`** ([kernel/src/sched/mod.rs](kernel/src/sched/mod.rs) — `register_idle` body) — the single-idle invariant is load-bearing for ADR-0026's dispatch chain; a release-build silent overwrite would let the previously-registered idle task re-enter normal scheduling on its next `yield_now`. Replace `debug_assert!` with an unconditional `assert!` wrapped in `#[allow(clippy::panic, reason = ...)]`. The function's `# Panics` doc-comment updated to describe the unconditional behaviour (was "panics in debug builds; release silently overwrites" — now "panics unconditionally in both debug and release"). 7. **start_prelude / start `# Panics` rustdoc updated for ADR-0026** ([kernel/src/sched/mod.rs](kernel/src/sched/mod.rs)) — the docs previously said "panics if the ready queue is empty"; the new contract is "panics if ready queue empty AND `s.idle` is None". `start_prelude` and `start` doc-comments now reference both halves of the dispatch chain explicitly. Notes that registering only an idle task (via `register_idle`, no `add_task`) is sufficient to avoid the panic — the post-T-014 "minimum viable boot" shape. 8. **Comprehensive review plan §12 amendment for stale CI claim** ([docs/analysis/reviews/code-reviews/2026-05-06-full-tree-comprehensive-review-plan.md](docs/analysis/reviews/code-reviews/2026-05-06-full-tree-comprehensive-review-plan.md)) — the plan was written assuming "no GitHub Actions workflow at HEAD"; Track H's actual run discovered `.github/workflows/ci.yml` was added 2026-04-23. Append a one-line plan-level amendment recording the correction so future readers find it without cross-referencing the merged artefact (which carries the same correction in its verdict). ## Skipped (2 style nits) - **gemini-code-assist on `start_prelude` line 551** — proposed `s.ready.dequeue().or(s.idle).expect("…")` simplification. Skipped: current `match`-arm shape with explicit `Some(h)` / `Some(idle_h)` / `None` branches is more readable in context (mirrors ADR-0026 §Decision outcome's prose explicitly enumerating the dispatch fallback chain), and the panic message is identical between the two forms. - **gemini-code-assist on `yield_now` line 709** — proposed coalescing the two-level `match` into a single `match next_handle`. Skipped: current arms carry case-comments distinguishing "dequeue returned current itself (case a)" from "fallback resolved to current (case b)", which the simplified form would lose. The branch-by-branch comments are documentation that ADR-0026's queue-state simulation table relies on; coalescing them is a regression on readability. ## Verification `cargo fmt --check` clean; `cargo host-clippy` clean (`-D warnings`); `cargo kernel-clippy` clean; `cargo host-test` 152/152; `cargo +nightly miri test` 152/152 clean; `cargo kernel-build` clean. QEMU smoke reproduces the full demo trace through `tyrne: all tasks complete` (boot-to-end ~7.9 ms; `-d int,unimp,guest_errors` empty). Refs: ADR-0026, T-014, PR #12 Audit: UNSAFE-2026-0014 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e guard, status line
Verified each finding against current code; all valid (minimal fixes):
- T-029 status line self-contradiction ('Both phases implemented' + 'Phase 2 …
in review'): made unambiguous — Phase 1 Done (PR #44); Phase 2 In Review
(PR #45); ACs implemented + passing gates, AC#4/#6 close on Phase 2 merge.
- div-by-zero hardening: added a compile-time that
the three iteration counts (N_CTX_ROUNDTRIPS / N_IPC_CYCLES / N_EL0_ROUNDTRIPS,
each a divisor) are non-zero — catches a future 0 at build time.
- el0_roundtrip_tick: -> so the bench
still terminates if ever overshoots (defensive vs a future preemptive/SMP
syscall_entry); behaviourally identical in today's strictly-serial path.
All changes are in the feature-gated perf_bench.rs (+ the T-029 doc); the
feature-off binary is unchanged. Gates: fmt clean; kernel-clippy ±feature clean
(-D warnings; the const-assert is not flagged); feature-on build OK (compile-time
assert holds).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e guard, status line
Verified each finding against current code; all valid (minimal fixes):
- T-029 status line self-contradiction ("Both phases implemented" + "Phase 2 …
in review"): made unambiguous — Phase 1 Done (PR #44); Phase 2 In Review
(PR #45); ACs implemented + passing gates, AC#4/#6 close on Phase 2 merge.
- div-by-zero hardening: added a compile-time `const _: () = assert!(...)` that
the three iteration counts (N_CTX_ROUNDTRIPS / N_IPC_CYCLES / N_EL0_ROUNDTRIPS,
each a `total / N` divisor) are non-zero — catches a future 0 at build time.
- el0_roundtrip_tick: `n == EL0_WARMUP + N_EL0_ROUNDTRIPS` -> `n >=` so the bench
still terminates if `n` ever overshoots (defensive vs a future preemptive/SMP
syscall_entry); behaviourally identical in today's strictly-serial path.
All changes are in the feature-gated perf_bench.rs (+ the T-029 doc); the
feature-off binary is unchanged. Gates: fmt clean; kernel-clippy ±feature clean
(-D warnings; the const-assert is not flagged); feature-on build OK (compile-time
assert holds).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…etes T-029) (#45) * feat(perf): T-029 Phase 2 — EL0 syscall round-trip micro-bench Completes T-029: the EL0↔EL1↔EL0 syscall round-trip, timed kernel-side, with no CNTVCT_EL0 exposure to EL0 (AC#4). Combined with the Phase-1 ctx+IPC benches so one `perf-bench` measurement build reports all three. Design (maintainer-approved: hand-assembled image + combined build): - A hand-assembled 12-byte raw-flat EL0 image (`mov x8,#0; svc #0; b .-4`, BENCH_EL0_IMAGE) loops a REJECTED syscall (number 0 → BadSyscallNumber → Resume, before any cap is touched — no yield, no side effect, no log). So each svc is the pure fixed round-trip overhead (trap + 272-byte frame save + decode + return), and the task monopolises the CPU (never switches) → back-to-back syscall_entry entries. - perf_bench::run loads it via the existing load_image + task_create_from_image + add_user_task (added FIRST, dispatched first), alongside the Phase-1 driver/partner/idle. - A feature-gated hook in syscall_entry — el0_roundtrip_tick(effect) — times consecutive entries (safe Timer::now_ns), skips a warm-up, accumulates N deltas, prints the mean, then returns SyscallEffect::Terminate(0). The EXISTING (T-028) Terminate arm ends the EL0 bench via task_exit_current and dispatches the driver → the ctx/IPC benches run next. - Un-gated USER_TASK_STACK / USER_TASK_TABLE: now the EL0 bench task's SP_EL1 + an empty cap table (the rejected svc consults no capability). No new `unsafe` operation: timing reuses safe now_ns (UNSAFE-2026-0015); the EL0 load reuses load_image (UNSAFE-2026-0025/0026/0027) + add_user_task/enter_el0 (UNSAFE-2026-0032); the Terminate is applied by the audited arm (UNSAFE-2026-0008). UNSAFE-2026-0014 gains a Phase-2 Amendment. Numbers (QEMU-virt/TCG, relative-only): EL0 syscall round-trip 9 108 ns/syscall (N=50 000) — ≈3.6x a context switch in the same run. Gates: fmt clean; kernel-clippy ±feature clean (-D warnings; run() gets the same too_many_lines allow kernel_main_high uses); host tests pass; build ±feature; feature-off byte-identity (.text/.data/.bss + footprint identical to base, only 32 .rodata panic-loc bytes shift); feature-off smoke = unchanged demo trace; one feature-on run captured all three numbers + `perf-bench complete`; Miri --workspace --exclude bsp 0 UB. Docs: baseline §Micro-measurements Phase 2 section filled in (section complete); T-029 AC#4/#6 flipped, all ACs met; UNSAFE-2026-0014 Phase-2 Amendment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(perf): PR #45 review-round — non-zero N const-assert, >= terminate guard, status line Verified each finding against current code; all valid (minimal fixes): - T-029 status line self-contradiction ("Both phases implemented" + "Phase 2 … in review"): made unambiguous — Phase 1 Done (PR #44); Phase 2 In Review (PR #45); ACs implemented + passing gates, AC#4/#6 close on Phase 2 merge. - div-by-zero hardening: added a compile-time `const _: () = assert!(...)` that the three iteration counts (N_CTX_ROUNDTRIPS / N_IPC_CYCLES / N_EL0_ROUNDTRIPS, each a `total / N` divisor) are non-zero — catches a future 0 at build time. - el0_roundtrip_tick: `n == EL0_WARMUP + N_EL0_ROUNDTRIPS` -> `n >=` so the bench still terminates if `n` ever overshoots (defensive vs a future preemptive/SMP syscall_entry); behaviourally identical in today's strictly-serial path. All changes are in the feature-gated perf_bench.rs (+ the T-029 doc); the feature-off binary is unchanged. Gates: fmt clean; kernel-clippy ±feature clean (-D warnings; the const-assert is not flagged); feature-on build OK (compile-time assert holds). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary by CodeRabbit
Release Notes
Bug Fixes
Documentation
Tests