Development - #4
Merged
Merged
Conversation
- T-003 marked Done; PR merged to main 2026-04-21. - A4 milestone closed; A5 (cooperative scheduler) opened. - T-004 task file created: cooperative yield-based scheduler with context switch, IPC blocking integration, and QEMU smoke test. ADR-0019 (scheduler shape) and ADR-0020 (Cpu trait v2) required before implementation begins. - current.md, phase-a.md, and tasks README updated accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Settles four inter-related design axes for Milestone A5:
- Queue structure: single bounded FIFO (SchedQueue<N>, N = TASK_ARENA_CAPACITY).
- Yield semantics: yield to next ready task (FIFO order); no targeted yield in v1.
- Blocked-task state: per-task TaskState enum { Idle, Ready, Blocked { on: EndpointHandle } }
in kernel::sched; O(N) unblock scan accepted at A5 scale (N ≤ 16).
- IPC bridge: scheduler is the orchestration layer; kernel::ipc remains free of
scheduling concerns and its 64 tests are unaffected.
Open questions: yield_to(target) for reply_recv (deferred to ADR-0018),
initial TaskContext setup (ADR-0020), idle task for double-block deadlock (Phase B).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Formally closes the two open questions that ADR-0017 left to this ADR: - Badge scheme: deferred (Option A). No Phase A/A6 scenario requires kernel-injected badges; user-space emulation via CapHandle keying is available today. Revisit trigger: multiplexed-server scenario or formal security argument requiring kernel-TCB injection. - reply_recv fastpath: deferred (Option D). The operation is semantically undefined without a running scheduler (A5) and unmeasurable without real context switches (A6 timings). Revisit trigger: A5/A6 benchmarks showing the extra context switch dominates server-loop latency. Message::label, Capability, SlotEntry, and all IPC operations are unchanged; both features remain additive for a successor ADR. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d Cpu v2 ADR-0019 (scheduler shape) is now Accepted: single bounded FIFO, yield-to-next-ready, per-task TaskState enum, scheduler as IPC orchestration layer. ADR-0020 (Proposed): introduces a separate ContextSwitch trait (preserving Cpu object-safety), unsafe context_switch / init_context operations, and a TaskContext associated type. Defines the aarch64 register frame (x19–x28, fp, lr, sp = 104 bytes) and the TestHal no-op for host tests. Both ADRs are prerequisites for T-004 (A5). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ADR-0020 (`ContextSwitch` trait and `Cpu` v2) accepted: both ADRs that gate T-004 are now Accepted. T-004 status advances from Draft → Ready; all blocking prerequisites recorded in the task file. Roadmap pointers updated in current.md, phase-a.md, and the task index. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… scheduler
Milestone A5 implementation (T-004):
hal/src/context_switch.rs — new ContextSwitch trait (ADR-0020): TaskContext
associated type, unsafe context_switch / init_context with full Safety docs.
bsp-qemu-virt/src/cpu.rs — QemuVirtCpu implementing Cpu + ContextSwitch;
Aarch64TaskContext (repr(C), 104 bytes: x19–x28 + fp + lr + sp); inline-asm
context_switch_asm saving/restoring callee-saved registers and switching via ret.
kernel/src/sched/mod.rs — Scheduler<C: ContextSwitch + Cpu>:
- SchedQueue<N>: bounded FIFO with wrap-around, O(1) enqueue/dequeue
- TaskState: Idle / Ready / Blocked { on: EndpointHandle }
- add_task, start, yield_now: cooperative context-switch operations
- ipc_send_and_yield, ipc_recv_and_yield: IPC bridge over kernel::ipc
- unblock_receiver_on: O(N) scan to re-enqueue blocked tasks
- 11 host tests: queue FIFO, wrap-around, state transitions, unblock logic
docs/audits/unsafe-log.md — audit entries UNSAFE-2026-0006 through 0009.
All checks clean: cargo fmt, cargo host-clippy, cargo host-test (75 passed).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e passes Three bugs prevented task_a from executing after `start()` transferred control. All three are fixed in this commit; QEMU now shows interleaved output from both tasks across all three iterations. Bug 1 — IrqGuard fat-pointer vtable (hal/src/cpu.rs) `IrqGuard::new(cpu)` where `cpu: &QemuVirtCpu` was coerced to `&dyn Cpu`. The compiler placed the vtable adjacent to a panic string in .rodata; the vtable offset landed on ASCII bytes, so `blr` jumped into garbage. Fix: make `IrqGuard` generic — `IrqGuard<'a, C: Cpu>` takes `cpu: &'a C` directly, eliminating the fat-pointer coercion site. Bug 2 — CPACR_EL1.FPEN not initialised (bsp-qemu-virt/src/boot.s) QEMU virt drops the kernel to EL1. CPACR_EL1.FPEN resets to 0b00, which traps FP/SIMD at EL1 as Undefined Instruction. The compiler emits `movi v0.2d, #0` for zero-initialising the throwaway scheduler context; this trapped immediately. No VBAR was set, so the exception vector at 0x200 fetched zeros → silent hang. Fix: set CPACR_EL1.FPEN = 0b11 (bits[21:20] = 0x300000) in boot.s before zeroing BSS, followed by ISB. Bug 3 — context_switch_asm compiler prologue corrupts saved sp (bsp-qemu-virt/src/cpu.rs) Even with `#[inline(never)]`, the compiler generated a standard function prologue (`stp x29, x30, [sp, #-0x10]!`) before our asm, adjusting sp by -16 before we saved it. On context restore, sp was 16 bytes too low; the caller's (yield_now) epilogue read callee-saved registers from the wrong stack addresses, then `ret`-ed to garbage. Fix: convert `context_switch_asm` to `#[unsafe(naked)] extern "C"` with `naked_asm!`. No prologue/epilogue is generated; sp is saved and restored exactly as the caller left it. QEMU smoke output (debug build): umbrix: hello from kernel_main umbrix: starting cooperative scheduler umbrix: task A — iteration 0 umbrix: task B — iteration 0 umbrix: task A — iteration 1 umbrix: task B — iteration 1 umbrix: task A — iteration 2 umbrix: task B — iteration 2 umbrix: task A done; spinning Also: remove debug raw volatile UART writes from task_a; update T-004 → Done; advance current.md and phase-a.md to A6. Audit entries: UNSAFE-2026-0006 through UNSAFE-2026-0009 (existing). Host tests: 75 passed (umbrix-kernel --lib). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Reviewer's GuideIntroduces the A5 cooperative scheduler milestone: adds a generic scheduler over a new ContextSwitch HAL trait, implements QemuVirtCpu with context-switch assembly on aarch64, wires two kernel tasks and their stacks into the BSP entry for a QEMU smoke test, fixes IrqGuard to avoid fat-pointer issues, and updates roadmap, ADRs, and task docs to reflect completion and unsafe-audit entries. Class diagram for new ContextSwitch, QemuVirtCpu, and SchedulerclassDiagram
direction LR
class Cpu {
<<trait>>
+current_core_id() CoreId
+disable_irqs() IrqState
+restore_irq_state(state IrqState) void
+wait_for_interrupt() void
+instruction_barrier() void
}
class ContextSwitch {
<<trait>>
+context_switch(current TaskContext, next TaskContext) void
+init_context(ctx TaskContext, entry fn() -> !, stack_top *mut u8) void
<<associatedtype>> TaskContext
}
class IrqGuard~C~ {
-cpu &C
-prev IrqState
+new(cpu &C) IrqGuard~C~
-drop() void
}
class QemuVirtCpu {
-_priv ()
+new() QemuVirtCpu
+current_core_id() CoreId
+disable_irqs() IrqState
+restore_irq_state(state IrqState) void
+wait_for_interrupt() void
+instruction_barrier() void
+context_switch(current &mut Aarch64TaskContext, next &Aarch64TaskContext) void
+init_context(ctx &mut Aarch64TaskContext, entry fn() -> !, stack_top *mut u8) void
}
class Aarch64TaskContext {
<<reprC>>
+x19_x28 u64[10]
+fp u64
+lr u64
+sp u64
+new() Aarch64TaskContext
}
class SchedQueue~N~ {
-buf Option~TaskHandle~[N]
-head usize
-len usize
+new() SchedQueue~N~
+enqueue(handle TaskHandle) Result~(), TaskHandle~
+dequeue() Option~TaskHandle~
+len() usize
+is_empty() bool
}
class TaskState {
<<enum>>
Idle
Ready
Blocked
+on EndpointHandle
}
class SchedError {
<<enum>>
NoCurrentTask
QueueFull
Ipc
}
class Scheduler~C~ {
-ready SchedQueue~TASK_ARENA_CAPACITY~
-task_states TaskState[TASK_ARENA_CAPACITY]
-task_handles Option~TaskHandle~[TASK_ARENA_CAPACITY]
-current Option~TaskHandle~
-contexts C::TaskContext[TASK_ARENA_CAPACITY]
+new() Scheduler~C~
+add_task(cpu &C, handle TaskHandle, entry fn() -> !, stack_top *mut u8) Result~(), SchedError~
+start(cpu &C) void
+yield_now(cpu &C) Result~(), SchedError~
+ipc_send_and_yield(cpu &C, ep_arena &mut EndpointArena, queues &mut IpcQueues, caller_table &mut CapabilityTable, ep_cap CapHandle, msg Message, transfer Option~CapHandle~) Result~SendOutcome, SchedError~
+ipc_recv_and_yield(cpu &C, ep_arena &mut EndpointArena, queues &mut IpcQueues, caller_table &mut CapabilityTable, ep_cap CapHandle) Result~RecvOutcome, SchedError~
-resolve_ep_cap(caller_table &CapabilityTable, ep_cap CapHandle) Result~EndpointHandle, SchedError~
-unblock_receiver_on(ep EndpointHandle) void
}
class StaticCell~T~ {
-inner UnsafeCell~MaybeUninit~T~~
+new() StaticCell~T~
}
class TaskStack {
<<reprC_align16>>
-inner UnsafeCell~[u8;4096]~
+new() TaskStack
+top() *mut u8
}
class TaskHandle
class EndpointHandle
class CapabilityTable
class IpcQueues
class Message
class SendOutcome
class RecvOutcome
class CapHandle
%% Trait implementations and relationships
QemuVirtCpu ..|> Cpu
QemuVirtCpu ..|> ContextSwitch
ContextSwitch <|.. Scheduler
Cpu <|.. IrqGuard
Scheduler "1" o-- "1" SchedQueue
Scheduler "1" o-- "TASK_ARENA_CAPACITY" TaskState
Scheduler "1" o-- "TASK_ARENA_CAPACITY" Aarch64TaskContext : contexts
Scheduler ..> TaskHandle
Scheduler ..> EndpointHandle
Scheduler ..> CapabilityTable
Scheduler ..> IpcQueues
Scheduler ..> Message
IrqGuard ..> IrqState
StaticCell "1" o-- "1" QemuVirtCpu
StaticCell "1" o-- "1" Scheduler
StaticCell "1" o-- "1" Console
TaskStack <.. TASK_A_STACK : uses
TaskStack <.. TASK_B_STACK : uses
class Console
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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 7 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 (15)
📝 WalkthroughWalkthroughThis PR implements Phase A5 cooperative scheduler with context switching support. It introduces a Changes
Sequence DiagramssequenceDiagram
participant Boot as Boot Sequence
participant CPU as QemuVirtCpu
participant Sched as Scheduler
participant Task as Task A
Boot->>Boot: Enable FP/SIMD (CPACR_EL1)
Boot->>Boot: Zero BSS
Boot->>Sched: kernel_entry()<br/>Initialize Scheduler
Sched->>CPU: new()
Sched->>Sched: TaskArena.add(Task A)<br/>TaskArena.add(Task B)
Sched->>Sched: add_task(Task A, entry_fn,<br/>stack_top)
CPU->>CPU: init_context()<br/>Set lr=entry, sp=stack_top
Sched->>Sched: add_task(Task B, ...)
Sched->>Sched: start()
Sched->>CPU: context_switch_asm<br/>(current=idle, next=Task A)
CPU->>CPU: Restore Task A registers<br/>Switch SP, ret to Task A.lr
Task->>Task: Execute (print, yield)
sequenceDiagram
participant Task_A as Task A
participant Sched as Scheduler
participant CPU as QemuVirtCpu
participant Task_B as Task B
Task_A->>Sched: yield_now()
Sched->>Sched: Re-enqueue Task A<br/>Dequeue Task B
Sched->>Sched: Update TaskState<br/>(A→Ready, B→Running)
Sched->>CPU: disable_irqs() → save DAIF
Sched->>CPU: context_switch_asm<br/>(current=Task A ctx,<br/>next=Task B ctx)
CPU->>CPU: Save Task A callee-saved<br/>(x19–x28, fp, lr, sp)
CPU->>CPU: Restore Task B callee-saved<br/>Switch SP, ret to Task B.lr
Task_B->>Task_B: Execute (print, yield)
Sched->>CPU: restore_irq_state(saved_DAIF)
Note over Sched,Task_B: Task B now running
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes 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 |
Review Summary by QodoCooperative scheduler with context switching and IPC integration (A5 milestone completion)
WalkthroughsDescription• Implements cooperative scheduler with bounded FIFO queue (SchedQueue<N>) providing O(1) enqueue/dequeue operations • Introduces ContextSwitch HAL trait as separate extension to preserve Cpu object-safety, with associated TaskContext type for register state storage • Implements aarch64 context-switch for QEMU virt BSP with Aarch64TaskContext struct and naked assembly context_switch_asm function • Adds TaskState enum (Idle, Ready, Blocked) for per-task scheduling state tracking and IPC bridge methods (ipc_send_and_yield(), ipc_recv_and_yield()) • Provides yield_now() for cooperative context switching with comprehensive unit tests • Fixes IrqGuard vtable dispatch issue by changing from dynamic dispatch to generic `IrqGuard<'a, C: Cpu>` • Enables FP/SIMD in boot sequence by initializing CPACR_EL1.FPEN to prevent NEON instruction traps • Adds smoke-test tasks demonstrating cooperative yielding and scheduler initialization in kernel entry • Documents design decisions in ADR-0018 (badge scheme deferral), ADR-0019 (scheduler shape), and ADR-0020 (context-switch trait) • Records T-004 task completion with three implementation bugs fixed and unsafe audit log entries • Updates roadmap marking A5 milestone complete and activating A6 milestone Diagramflowchart LR
A["Cpu trait<br/>object-safe"] -->|"separate trait"| B["ContextSwitch trait<br/>with TaskContext"]
C["SchedQueue<N><br/>bounded FIFO"] -->|"manages"| D["Scheduler<br/>ready queue + state"]
D -->|"uses"| B
E["aarch64 assembly<br/>context_switch_asm"] -->|"implements"| B
D -->|"provides"| F["yield_now<br/>cooperative switch"]
D -->|"bridges"| G["IPC send/recv<br/>with yield"]
H["IrqGuard generic<br/>no vtable"] -->|"fixes dispatch"| A
I["CPACR_EL1.FPEN<br/>boot init"] -->|"prevents"| J["NEON traps"]
File Changes1. kernel/src/sched/mod.rs
|
Code Review by Qodo
1. New code added in boot.s
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The safety contract in
QemuVirtCpu::new(“there must be at most one instance per core”) doesn’t match it being a safeconst fn; either make the constructorunsafeor relax/rephrase the documented invariant so callers aren’t relying on an unenforced safety guarantee. - The ad‑hoc
unsafeaccess patterns onStaticCell(e.g.,(*CONSOLE.0.get()).assume_init_ref()) are replicated in several places; consider adding safe-ish helper methods likefn init(&self, T)andfn get(&self) -> &T/&mut T(stillunsafe) to centralise the invariants and avoid directly reaching into the innerUnsafeCellat each call site. - In
Scheduler::ipc_recv_and_yield, the deadlock path panics with a fixed message; it might be useful to log which endpoint and task index caused the condition (or at least the fact that all tasks are blocked) to make debugging such a fatal condition easier when it is hit on real hardware.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The safety contract in `QemuVirtCpu::new` (“there must be at most one instance per core”) doesn’t match it being a safe `const fn`; either make the constructor `unsafe` or relax/rephrase the documented invariant so callers aren’t relying on an unenforced safety guarantee.
- The ad‑hoc `unsafe` access patterns on `StaticCell` (e.g., `(*CONSOLE.0.get()).assume_init_ref()`) are replicated in several places; consider adding safe-ish helper methods like `fn init(&self, T)` and `fn get(&self) -> &T`/`&mut T` (still `unsafe`) to centralise the invariants and avoid directly reaching into the inner `UnsafeCell` at each call site.
- In `Scheduler::ipc_recv_and_yield`, the deadlock path panics with a fixed message; it might be useful to log which endpoint and task index caused the condition (or at least the fact that all tasks are blocked) to make debugging such a fatal condition easier when it is hit on real hardware.
## Individual Comments
### Comment 1
<location path="kernel/src/sched/mod.rs" line_range="424-431" />
<code_context>
+ /// Scan `task_states` for a task blocked on `ep` and re-enqueue it.
+ ///
+ /// O(N) scan — acceptable at `TASK_ARENA_CAPACITY ≤ 16` (ADR-0019).
+ fn unblock_receiver_on(&mut self, ep: EndpointHandle) {
+ for idx in 0..TASK_ARENA_CAPACITY {
+ if let TaskState::Blocked { on } = self.task_states[idx] {
+ if on == ep {
+ if let Some(handle) = self.task_handles[idx] {
+ self.task_states[idx] = TaskState::Ready;
+ let _ = self.ready.enqueue(handle);
+ return;
+ }
+ }
</code_context>
<issue_to_address>
**question:** Clarify single-receiver unblocking semantics or consider unblocking all receivers on an endpoint.
This helper currently wakes only the first task blocked on the endpoint and returns, which effectively enforces single-waiter semantics. If multiple receivers per endpoint are possible, either document that only one receiver is woken per send, or adjust the loop to wake all matching tasks to avoid potential starvation or surprising behavior.
</issue_to_address>
### Comment 2
<location path="bsp-qemu-virt/src/cpu.rs" line_range="72-81" />
<code_context>
+ id
+ }
+
+ fn disable_irqs(&self) -> IrqState {
+ let daif: usize;
+ // SAFETY: `MRS x, DAIF` reads the current interrupt mask; `MSR DAIF, #0xF`
+ // masks all DAIF bits (D, A, I, F). Both are EL1-privileged register
+ // operations. The returned `IrqState` captures the prior value so that
+ // `restore_irq_state` can restore it exactly. Audit: UNSAFE-2026-0007.
+ unsafe {
+ asm!(
+ "mrs {daif}, daif",
+ "msr daifset, #0xf",
+ daif = out(reg) daif,
+ options(nostack, nomem),
</code_context>
<issue_to_address>
**nitpick:** Align comments with the actual DAIF-masking instruction used.
The comment mentions `MSR DAIF, #0xF`, but the code uses `msr daifset, #0xf`. Please update the comment to describe the actual `DAIFSet` instruction and its semantics, so it’s clear which masking mechanism is relied on for this operation and for future audits.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+424
to
+431
| fn unblock_receiver_on(&mut self, ep: EndpointHandle) { | ||
| for idx in 0..TASK_ARENA_CAPACITY { | ||
| if let TaskState::Blocked { on } = self.task_states[idx] { | ||
| if on == ep { | ||
| if let Some(handle) = self.task_handles[idx] { | ||
| self.task_states[idx] = TaskState::Ready; | ||
| let _ = self.ready.enqueue(handle); | ||
| return; |
There was a problem hiding this comment.
question: Clarify single-receiver unblocking semantics or consider unblocking all receivers on an endpoint.
This helper currently wakes only the first task blocked on the endpoint and returns, which effectively enforces single-waiter semantics. If multiple receivers per endpoint are possible, either document that only one receiver is woken per send, or adjust the loop to wake all matching tasks to avoid potential starvation or surprising behavior.
Comment on lines
+72
to
+81
| fn disable_irqs(&self) -> IrqState { | ||
| let daif: usize; | ||
| // SAFETY: `MRS x, DAIF` reads the current interrupt mask; `MSR DAIF, #0xF` | ||
| // masks all DAIF bits (D, A, I, F). Both are EL1-privileged register | ||
| // operations. The returned `IrqState` captures the prior value so that | ||
| // `restore_irq_state` can restore it exactly. Audit: UNSAFE-2026-0007. | ||
| unsafe { | ||
| asm!( | ||
| "mrs {daif}, daif", | ||
| "msr daifset, #0xf", |
There was a problem hiding this comment.
nitpick: Align comments with the actual DAIF-masking instruction used.
The comment mentions MSR DAIF, #0xF, but the code uses msr daifset, #0xf. Please update the comment to describe the actual DAIFSet instruction and its semantics, so it’s clear which masking mechanism is relied on for this operation and for future audits.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a cooperative scheduler and aarch64 context switching for the QEMU virt target. Key changes include the introduction of the ContextSwitch HAL trait, a FIFO-based scheduler in the kernel, and naked assembly for register save/restore. Additionally, the boot sequence was updated to enable FP/SIMD at EL1 to prevent traps during initialization, and IrqGuard was refactored to use generics to avoid vtable dispatch issues. I have no feedback to provide.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+30
to
+36
| /* Enable FP/SIMD at EL1 and EL0 (CPACR_EL1.FPEN = 0b11 = bits[21:20]). | ||
| * 0x300000 = 3 << 20. CPACR_EL1 resets to zero, so only FPEN needs | ||
| * setting; all other fields remain 0 (no ZEN, no TTA traps). | ||
| * ISB ensures the write is visible before the first NEON instruction. */ | ||
| mov x0, #0x300000 | ||
| msr cpacr_el1, x0 | ||
| isb |
There was a problem hiding this comment.
1. New code added in boot.s 📘 Rule violation ⛨ Security
The PR adds new boot logic in bsp-qemu-virt/src/boot.s (AArch64 assembly) rather than Rust. This violates the requirement that kernel/userspace implementation code be written in Rust to preserve memory-safety and consistency guarantees.
Agent Prompt
## Issue description
The PR adds new boot-time functionality in an assembly source file (`boot.s`). This violates the rule requiring kernel/userspace implementation code to be written in Rust.
## Issue Context
The added instructions configure `CPACR_EL1.FPEN` to avoid NEON traps during early boot. This is legitimate functionality, but it should be expressed from Rust source (e.g., via a Rust `#[naked]` entry stub using `core::arch::asm!`/`naked_asm!`, or another Rust-owned early-init path) rather than adding new `.s` implementation code.
## Fix Focus Areas
- bsp-qemu-virt/src/boot.s[30-36]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/roadmap/phases/phase-a.md (1)
129-153:⚠️ Potential issue | 🟡 MinorT-004 status is inconsistent between this roadmap and the task index.
Line 129 marks milestone A5 as
✓ (done 2026-04-21)and Line 153 lists T-004 asDone, butdocs/analysis/tasks/phase-a/README.mdLine 12 still shows T-004 asIn Progress. One of the two is stale — please reconcile before merging so the roadmap, task index, and the T-004 task doc itself agree on A5/T-004 being either Done or still In Progress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/roadmap/phases/phase-a.md` around lines 129 - 153, Milestone A5 and the T-004 task state disagree: update the status so Milestone A5 (heading "Milestone A5 — Cooperative scheduler and context switch") and the T-004 entry in the phase-a task index (the README that lists T-004) and the T-004 task doc all show the same state (either "Done" or "In Progress"); pick the correct final state, change the text labeled "✓ (done 2026-04-21)" or the T-004 line in the phase-a README to match, and ensure the T-004 task document itself reflects that same status so A5, T-004, and the task doc are consistent.kernel/src/lib.rs (1)
52-68:⚠️ Potential issue | 🟡 Minor
kernel::run()is dead code — either remove or rewire the BSP to use it.No call to
kernel::run()exists anywhere in the codebase. The BSP'skernel_entry()(lines 203+) initializes the console, CPU, scheduler, and tasks directly, then transfers control to the scheduler. This function never reaches therun()function marked as the "portable kernel entry."The function's doc comment claims it handles A5 scheduler wiring, but that work happens in
kernel_entry()instead. The added_cpuparameter is unused. Either:
- Delete
run()and clarify the doc onkernel_entry()as the actual entry point, or- Move scheduler/task initialization into
kernel::run()and havekernel_entry()call it, so the "portable entry" claim becomes truthful.As written, the crate exposes a dead public API whose doc is out of sync with reality.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/src/lib.rs` around lines 52 - 68, The public function kernel::run and its docs are dead — either remove it or make it the real portable entry: Option A: delete pub fn run<Con: Console, C: Cpu>(...) entirely, remove its docs and update the kernel_entry() doc comment to state it is the BSP entry point; Option B: refactor so kernel_entry() delegates to kernel::run by moving scheduler and task initialization out of kernel_entry() into run (and then remove the unused _cpu parameter or use it), ensuring run performs console setup, scheduler wiring and never returns; pick one approach and update docs accordingly.
♻️ Duplicate comments (1)
docs/analysis/tasks/phase-a/README.md (1)
11-12:⚠️ Potential issue | 🟡 MinorT-004 status contradicts the phase roadmap.
This index lists T-004 as
In Progress, whiledocs/roadmap/phases/phase-a.md(Lines 129, 153) now marks Milestone A5 and T-004 asDone (2026-04-21). Pick one source of truth and align the other (and the T-004 task markdown) so readers can't land on contradicting status values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/analysis/tasks/phase-a/README.md` around lines 11 - 12, The README table entry for T-004 ("Cooperative scheduler") currently shows "In Progress" which contradicts the phase roadmap entry marking Milestone A5/T-004 as "Done"; update the T-004 status so both sources match (choose the canonical source of truth and align the other), e.g., change the table row for T-004 in docs/analysis/tasks/phase-a/README.md to "Done (2026-04-21)" and ensure the T-004 task markdown (T-004-cooperative-scheduler.md) and the Milestone A5 reference reflect the same final status and date.
🧹 Nitpick comments (8)
hal/src/cpu.rs (1)
86-119: MonomorphizingIrqGuardoverC: Cpulooks correct.Removes the
&dyn Cpucoercion at guard construction, so the scheduler's hot paths (yield_now, IPC handoff) go through static dispatch and no vtable is materialized. Lifetime/Dropsignatures are in order.One small note: the doc rationale on Lines 86-88 describes a fairly specific codegen pathology ("incorrect vtable references when the coercion site is inlined near large
.rodataconstants"). If that's tied to a reproducer or upstream issue, a link in the doc (or in ADR-0020) would make the justification easier to revisit later; otherwise the more generic "avoid dynamic dispatch on critical-section paths" framing might age better.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hal/src/cpu.rs` around lines 86 - 119, The doc comment on IrqGuard currently includes a specific codegen pathology claim; either add a reference link to the reproducer or upstream issue that demonstrates "incorrect vtable references when the coercion site is inlined near large `.rodata` constants" or reword the rationale to a more generic explanation such as "avoid dynamic dispatch on critical-section paths" to keep the justification maintainable; update the doc comment above the IrqGuard struct (the text that references Cpu and vtable coercion) accordingly.bsp-qemu-virt/src/boot.s (1)
30-36: FP/SIMD enable is correct.
CPACR_EL1.FPEN = 0b11at bits [21:20] (3 << 20 == 0x300000) disables EL0/EL1 SIMD/FP traps, and the trailingISBcorrectly flushes the pipeline before any NEON-emitting Rust code runs. Placing this ahead of the BSS loop is conservative (the BSS zeroing itself only uses scalarstr xzr), but it keeps the "no traps by the time we enter Rust" invariant unambiguous.One optional refinement: the comment on line 31 states
CPACR_EL1 resets to zero. Per the Arm ARM (DDI0601/2023-06), the FPEN field is architecturally UNKNOWN on reset and only happens to be zero on QEMU. If this code runs on real hardware in the future, consider rewording to: "We do not rely on the reset value; an explicit write sets FPEN = 0b11 and leaves all other fields zero."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bsp-qemu-virt/src/boot.s` around lines 30 - 36, Update the comment above the FP/SIMD enable sequence (the mov x0,`#0x300000` / msr cpacr_el1, x0 / isb block) to avoid asserting CPACR_EL1 resets to zero; instead state that we do not rely on the reset value and explicitly write CPACR_EL1.FPEN = 0b11 (bits[21:20]) leaving other fields zero, so FPEN is set and EL0/EL1 SIMD/FP traps are disabled before Rust/NEON code runs.bsp-qemu-virt/src/cpu.rs (1)
34-41: Clarify the singleton invariant or make the constructorunsafe.The
new()function is marked safe but its Safety doc requires "at most oneQemuVirtCpuinstance" to prevent inconsistent DAIF state. SinceQemuVirtCpuis a zero-sized type with no interior mutability, duplicate instances are structurally harmless — the actual invariant concerns disable/restore pairing (eachdisable_irqs()call must be paired with the correctrestore_irq_state()), not instance count.Either:
- Make the constructor
unsafeto enforce the singleton invariant at the type level.- Remove the "at most one instance" requirement from the Safety doc and clarify that multiple zero-sized handles are safe as long as disable/restore calls are paired correctly on the same core.
Current construction (single site in
kernel_entry) follows the stated requirement, but the type system does not enforce it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bsp-qemu-virt/src/cpu.rs` around lines 34 - 41, The constructor QemuVirtCpu::new currently is safe but its Safety doc requires a singleton; change this to make the constructor unsafe to reflect the invariant at the type level: update the function signature to pub const unsafe fn new() -> Self, update the doc comment to state that callers must ensure at most one QemuVirtCpu per physical core (or otherwise uphold disable_irqs()/restore_irq_state() pairing on that core), and adjust call sites (e.g., kernel_entry) to call unsafe { QemuVirtCpu::new() }; ensure references to disable_irqs and restore_irq_state and DAIF behavior remain in the Safety text so the invariant is explicit.kernel/src/sched/mod.rs (5)
148-166: Struct bound differs from impl bounds and doc-comment claim.Struct declaration at line 153 bounds only
C: ContextSwitch, while the impls at 168/174 additionally requireCpu, and the doc at line 150 states "Generic overC: ContextSwitch + Cpu". Consider tightening the struct toScheduler<C: ContextSwitch + Cpu>for consistency — otherwise a caller can instantiateScheduler::<SomeCtxOnlyCpu>fields (though notnew()), which creates a surprising API surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/src/sched/mod.rs` around lines 148 - 166, The struct's generic bound is too weak: update the Scheduler declaration to require both traits so it matches the impl blocks and docstring — change the declaration of Scheduler<C: ContextSwitch> to Scheduler<C: ContextSwitch + Cpu> so the type parameter used by fields like contexts: [C::TaskContext; ...] and the impls (the impl blocks referencing Cpu) are consistent with Scheduler::new / impl Scheduler methods and the doc comment.
276-282: Subtle self-yield path silently drains the ready queue.When
currentis the only task in the system, line 273 enqueues it, then thisdequeue()returnsSome(current_handle)which falls through to the_arm and returnsOk(()). The queue is now empty (the single task was consumed bydequeueand not put back), andself.currentremains the same task. Functionally correct — the nextyield_nowwill re-enqueue and repeat — but it means the ready queue is transiently out-of-sync withself.currentin a way that's easy to miss when debugging. A short comment capturing this invariant ("after self-yield fast path, queue is empty and current is unchanged; next yield re-establishes[current]") would help future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/src/sched/mod.rs` around lines 276 - 282, The self-yield fast-path silently dequeues the lone task (via self.ready.dequeue returning Some(current_handle)) and returns early, leaving the ready queue empty while self.current still points to the same task; add a concise comment near this match (referencing self.ready.dequeue, current_handle, and self.current) explaining the invariant: after this fast-path the ready queue is transiently empty and self.current is unchanged and the next yield will re-enqueue the current task, so readers understand the temporary desynchronization.
408-419:resolve_ep_capcollapses all lookup errors intoInvalidCapability.Line 414 maps any
lookuperror toIpcError::InvalidCapability, losing the distinction between a genuinely invalid handle, a stale generation, or other future variants. Since this helper is called on both IPC paths, the resultingSchedError::Ipc(InvalidCapability)may mask real failure modes. IfCapabilityTable::lookupreturns a typed error convertible toIpcError, forwarding it would give more accurate diagnostics.
203-222:add_taskleaves partially-registered state ifenqueuefails.Lines 217–218 write
task_states[idx] = Readyandtask_handles[idx] = Some(handle)before the fallibleenqueuecall at 219–221. On theQueueFullpath the slot is markedReadywith a stored handle but the task is not in the run queue — an orphaned state that the scheduler can't reach. The doc correctly notes this can't happen whenTASK_ARENA_CAPACITYslots back the queue, but the ordering makes the invariant depend on a capacity argument rather than on local atomicity.♻️ Suggested fix: enqueue first, then commit state
unsafe { cpu.init_context(&mut self.contexts[idx], entry, stack_top); } - self.task_states[idx] = TaskState::Ready; - self.task_handles[idx] = Some(handle); self.ready .enqueue(handle) - .map_err(|_| SchedError::QueueFull) + .map_err(|_| SchedError::QueueFull)?; + self.task_states[idx] = TaskState::Ready; + self.task_handles[idx] = Some(handle); + Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/src/sched/mod.rs` around lines 203 - 222, The current add_task sets task_states[idx] and task_handles[idx] before the fallible self.ready.enqueue(handle), which can leave a partially-registered orphan on QueueFull; change the order so you call self.ready.enqueue(handle) first and only on Ok(()) set self.task_states[idx] = TaskState::Ready and self.task_handles[idx] = Some(handle) and then return Ok(()), mapping enqueue errors to SchedError::QueueFull; keep the unsafe cpu.init_context(&mut self.contexts[idx], entry, stack_top) as-is (it can run before enqueue) but ensure no mutations to task_states/task_handles occur until enqueue succeeds.
367-400: Resumption path re-callsipc_recvbut doesn't handle a secondPending.After resuming from the block (line 398),
ipc_recvis called again and its result is returned directly. If — due to a bug elsewhere or a racing unblock —ipc_recvreturnsRecvOutcome::Pendingagain, this function returnsOk(Pending)to the caller, buttask_states[current_idx]was set toReadyby the unblocker at line 429 andself.currentwas restored implicitly by the context switch. Semantically the caller now seesPendingfor a task that isReadyand running — inconsistent with the contract thatPendingmeans "blocked until resumed".Given the design invariant that unblock only fires on
SendOutcome::Delivered(guaranteeing a message is waiting), a secondPendingshould be impossible; but adebug_assert!(!matches!(outcome, RecvOutcome::Pending))on the resumed path would catch violations during testing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/src/sched/mod.rs` around lines 367 - 400, Add a debug-only assertion on the resumed-path call to ipc_recv to ensure it never returns RecvOutcome::Pending: after the context switch resumes and before returning the ipc_recv result, call ipc_recv into a local (e.g., let outcome = ipc_recv(...).map_err(SchedError::Ipc)? or capture the RecvOutcome result) and insert debug_assert!(!matches!(outcome, RecvOutcome::Pending)), then return outcome (or map it) as before; reference symbols: ipc_recv, RecvOutcome::Pending, task_states/current/current_idx (resumed path after context_switch and before return).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bsp-qemu-virt/src/cpu.rs`:
- Around line 120-136: The AAPCS64 callee-saved SIMD registers d8–d15 are not
preserved: update Aarch64TaskContext and the assembly context switch to either
(preferred) extend the saved context with d8–d15 and implement corresponding
save/restore in context_switch_asm, or (alternate) force kernel-wide
soft-float/disable-NEON codegen so LLVM cannot allocate d8–d15; if you choose
the extend approach, add fields for d8–d15 (match their size/alignment), update
the total size comment and any code that assumes 104 bytes, and modify
context_switch_asm to store/restore those SIMD registers in the same layout and
adjust the stack offsets so the Rust struct and assembly layout remain
identical.
In `@bsp-qemu-virt/src/main.rs`:
- Around line 245-251: The calls to sched.add_task(...) are currently discarding
errors via .ok(), which can hide registration failures; change both
sched.add_task(cpu, handle_a, task_a, TASK_A_STACK.top()) and
sched.add_task(cpu, handle_b, task_b, TASK_B_STACK.top()) to handle the Result
instead (e.g., call .expect(...) or match and panic on Err) so setup will panic
with a clear message if task registration fails rather than silently continuing.
- Around line 69-72: The unsafe impl for StaticCell<T> and the unsafe
abstraction TaskStack must get their own dedicated audit entries and expanded
comments: create unique audit IDs (e.g. UNSAFE-2026-StaticCell-0001 and
UNSAFE-2026-TaskStack-0001), replace the current UNSAFE-2026-0001 reference on
StaticCell and the corresponding lines for TaskStack, and add an audit comment
block immediately above each unsafe impl/abstraction that (a) states why unsafe
is required, (b) lists the invariants the code upholds (concurrency, aliasing,
lifetime, initialization/Drop guarantees), and (c) explains why safer
alternatives (Mutex/RefCell/atomic types, stack-allocated arrays, or runtime
checks) were rejected; also record the new audit IDs in the repository's audit
tracking per docs/standards.
- Around line 141-149: The current code creates long-lived &mut Scheduler via
assume_init_mut() (SCHED) and then performs context switches
(start()/yield_now()), which can suspend a task while that mutable borrow is
live and lead to overlapping &mut references; change usages in the blocks that
call SCHED.0.get().assume_init_mut() / CPU.0.get().assume_init_ref() (occurring
around start(), yield_now(), and the other occurrences noted) to instead obtain
a raw *mut Scheduler / *const Cpu pointer from the UnsafeCell and call new
unsafe helper functions on Scheduler (e.g., scheduler_enter_raw(ptr) /
scheduler_yield_raw(ptr, cpu_ptr)) that operate via raw pointers so no &mut
Scheduler is materialized across context switches; ensure all places that
previously used sched.yield_now(cpu) or similar are routed through these
raw-pointer APIs and remove assume_init_mut() lifetimes that span calls that can
switch contexts.
In `@docs/analysis/tasks/phase-a/T-004-cooperative-scheduler.md`:
- Around line 24-27: The doc incorrectly references ADR-0020 as adding
Cpu::save_context / Cpu::restore_context and links a non-existent file; update
the text to state ADR-0020 defines a separate ContextSwitch trait with methods
context_switch and init_context (not Cpu::save_context/restore_context), remove
or correct the contradictory sentence about design on the later line, and fix
the broken link to point to the actual ADR/filename for ADR-0020; search for
symbols "ADR-0020", "ContextSwitch", "context_switch", "init_context", and
"Cpu::save_context"/"Cpu::restore_context" to locate and update the affected
lines.
In `@docs/audits/unsafe-log.md`:
- Around line 64-119: The pull request added several unsafe boundaries in the
BSP bootstrap that are not individually audited: add separate audit entries and
code comments for each unsafe in the bootstrap (rather than reusing
UNSAFE-2026-0001). Specifically, create new audit entries documenting why and
which invariants hold for unsafe impl Sync for StaticCell<T>, unsafe impl Sync
for TaskStack, unsafe fn TaskStack::top, and the static-cell write/assume_init_*
access blocks; then update the corresponding unsafe code comments in the BSP
bootstrap to include (a) why the unsafe is needed, (b) the invariants upheld,
and (c) why safer alternatives were rejected, referencing the same unique
symbols (StaticCell<T>, TaskStack, TaskStack::top, write, assume_init_*), so the
audit trail maps one-to-one to each unsafe boundary per docs/standards.
In `@docs/decisions/0019-scheduler-shape.md`:
- Line 23: Update the ADR text and API sketch to reference the separate
ContextSwitch trait instead of Cpu: replace mentions that say "context-switch
assembly lives in the BSP, exposed through the `Cpu` trait" with wording that it
lives in the BSP exposed through the `ContextSwitch` trait, and update the API
sketch to show `Scheduler<C: ContextSwitch>` (and any examples/signatures)
instead of tying context switching to `Cpu`; ensure all occurrences (including
the API sketch and the paragraph(s) around the current CPU reference) are
changed consistently so implementers are pointed to `ContextSwitch` per
ADR-0020.
In `@docs/decisions/0020-cpu-trait-v2-context-switch.md`:
- Around line 304-306: CPACR_EL1.FPEN is being enabled while context_switch_asm
only saves general-purpose callee-saved registers (x19–x28, x29, x30, sp), which
violates AAPCS64 because NEON/SIMD callee-saved registers d8–d15 are not
preserved; either add a global compile flag to disable SIMD codegen for kernel
tasks (e.g., rustflags/target-feature to force -mno-simd or equivalent) or
modify the context switch assembly (context_switch_asm) to save/restore d8–d15
across task switches and update ADR-0020 to document the change and rationale.
Ensure the chosen fix references CPACR_EL1.FPEN, context_switch_asm and d8–d15
so reviewers can find and verify the change.
In `@kernel/src/sched/mod.rs`:
- Around line 570-595: The test uses plain [u8; 512] buffers whose end pointer
(stack.as_mut_ptr_range().end) may not be 16-byte aligned, violating the safety
contract of ContextSwitch::init_context called by Scheduler::add_task; change
the tests (e.g., in yield_now_switches_context_and_updates_current and the
earlier test creating `stack`) to allocate a stack with explicit 16-byte
alignment (for example via a small wrapper type or helper like AlignedStack<512>
or using core::mem::align_of/#[repr(align(16))]) and pass its end pointer to
add_task; update the SAFETY comments to justify why the alignment is satisfied
(reference FakeCpu, add_task, and ContextSwitch::init_context) rather than
relying on FakeCpu being a no-op.
- Around line 233-254: The scheduler disables IRQs via an IrqGuard in
Scheduler::start but switches to a task stack that never drops that guard,
leaving IRQs disabled on task entry; either document this invariant on
Scheduler::start and Cpu::context_switch (and mention that Aarch64TaskContext
does not save PSTATE.DAIF so IRQ state does not follow tasks, and that yield_now
relies on guards on task stacks) or implement saving/restoring PSTATE.DAIF in
Aarch64TaskContext so Cpu::context_switch preserves IRQ state across switches;
pick one option, update the doc comments on the referenced functions/types
(Scheduler::start, Cpu::context_switch, Aarch64TaskContext, and yield_now) or
modify Aarch64TaskContext to include DAIF and adjust init_context/context_switch
paths accordingly to save/restore it.
---
Outside diff comments:
In `@docs/roadmap/phases/phase-a.md`:
- Around line 129-153: Milestone A5 and the T-004 task state disagree: update
the status so Milestone A5 (heading "Milestone A5 — Cooperative scheduler and
context switch") and the T-004 entry in the phase-a task index (the README that
lists T-004) and the T-004 task doc all show the same state (either "Done" or
"In Progress"); pick the correct final state, change the text labeled "✓ (done
2026-04-21)" or the T-004 line in the phase-a README to match, and ensure the
T-004 task document itself reflects that same status so A5, T-004, and the task
doc are consistent.
In `@kernel/src/lib.rs`:
- Around line 52-68: The public function kernel::run and its docs are dead —
either remove it or make it the real portable entry: Option A: delete pub fn
run<Con: Console, C: Cpu>(...) entirely, remove its docs and update the
kernel_entry() doc comment to state it is the BSP entry point; Option B:
refactor so kernel_entry() delegates to kernel::run by moving scheduler and task
initialization out of kernel_entry() into run (and then remove the unused _cpu
parameter or use it), ensuring run performs console setup, scheduler wiring and
never returns; pick one approach and update docs accordingly.
---
Duplicate comments:
In `@docs/analysis/tasks/phase-a/README.md`:
- Around line 11-12: The README table entry for T-004 ("Cooperative scheduler")
currently shows "In Progress" which contradicts the phase roadmap entry marking
Milestone A5/T-004 as "Done"; update the T-004 status so both sources match
(choose the canonical source of truth and align the other), e.g., change the
table row for T-004 in docs/analysis/tasks/phase-a/README.md to "Done
(2026-04-21)" and ensure the T-004 task markdown
(T-004-cooperative-scheduler.md) and the Milestone A5 reference reflect the same
final status and date.
---
Nitpick comments:
In `@bsp-qemu-virt/src/boot.s`:
- Around line 30-36: Update the comment above the FP/SIMD enable sequence (the
mov x0,`#0x300000` / msr cpacr_el1, x0 / isb block) to avoid asserting CPACR_EL1
resets to zero; instead state that we do not rely on the reset value and
explicitly write CPACR_EL1.FPEN = 0b11 (bits[21:20]) leaving other fields zero,
so FPEN is set and EL0/EL1 SIMD/FP traps are disabled before Rust/NEON code
runs.
In `@bsp-qemu-virt/src/cpu.rs`:
- Around line 34-41: The constructor QemuVirtCpu::new currently is safe but its
Safety doc requires a singleton; change this to make the constructor unsafe to
reflect the invariant at the type level: update the function signature to pub
const unsafe fn new() -> Self, update the doc comment to state that callers must
ensure at most one QemuVirtCpu per physical core (or otherwise uphold
disable_irqs()/restore_irq_state() pairing on that core), and adjust call sites
(e.g., kernel_entry) to call unsafe { QemuVirtCpu::new() }; ensure references to
disable_irqs and restore_irq_state and DAIF behavior remain in the Safety text
so the invariant is explicit.
In `@hal/src/cpu.rs`:
- Around line 86-119: The doc comment on IrqGuard currently includes a specific
codegen pathology claim; either add a reference link to the reproducer or
upstream issue that demonstrates "incorrect vtable references when the coercion
site is inlined near large `.rodata` constants" or reword the rationale to a
more generic explanation such as "avoid dynamic dispatch on critical-section
paths" to keep the justification maintainable; update the doc comment above the
IrqGuard struct (the text that references Cpu and vtable coercion) accordingly.
In `@kernel/src/sched/mod.rs`:
- Around line 148-166: The struct's generic bound is too weak: update the
Scheduler declaration to require both traits so it matches the impl blocks and
docstring — change the declaration of Scheduler<C: ContextSwitch> to
Scheduler<C: ContextSwitch + Cpu> so the type parameter used by fields like
contexts: [C::TaskContext; ...] and the impls (the impl blocks referencing Cpu)
are consistent with Scheduler::new / impl Scheduler methods and the doc comment.
- Around line 276-282: The self-yield fast-path silently dequeues the lone task
(via self.ready.dequeue returning Some(current_handle)) and returns early,
leaving the ready queue empty while self.current still points to the same task;
add a concise comment near this match (referencing self.ready.dequeue,
current_handle, and self.current) explaining the invariant: after this fast-path
the ready queue is transiently empty and self.current is unchanged and the next
yield will re-enqueue the current task, so readers understand the temporary
desynchronization.
- Around line 203-222: The current add_task sets task_states[idx] and
task_handles[idx] before the fallible self.ready.enqueue(handle), which can
leave a partially-registered orphan on QueueFull; change the order so you call
self.ready.enqueue(handle) first and only on Ok(()) set self.task_states[idx] =
TaskState::Ready and self.task_handles[idx] = Some(handle) and then return
Ok(()), mapping enqueue errors to SchedError::QueueFull; keep the unsafe
cpu.init_context(&mut self.contexts[idx], entry, stack_top) as-is (it can run
before enqueue) but ensure no mutations to task_states/task_handles occur until
enqueue succeeds.
- Around line 367-400: Add a debug-only assertion on the resumed-path call to
ipc_recv to ensure it never returns RecvOutcome::Pending: after the context
switch resumes and before returning the ipc_recv result, call ipc_recv into a
local (e.g., let outcome = ipc_recv(...).map_err(SchedError::Ipc)? or capture
the RecvOutcome result) and insert debug_assert!(!matches!(outcome,
RecvOutcome::Pending)), then return outcome (or map it) as before; reference
symbols: ipc_recv, RecvOutcome::Pending, task_states/current/current_idx
(resumed path after context_switch and before return).
🪄 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: a9f441a3-0e87-4266-966f-73b58b421ee3
📒 Files selected for processing (18)
bsp-qemu-virt/src/boot.sbsp-qemu-virt/src/cpu.rsbsp-qemu-virt/src/main.rsdocs/analysis/tasks/phase-a/README.mddocs/analysis/tasks/phase-a/T-003-ipc-primitives.mddocs/analysis/tasks/phase-a/T-004-cooperative-scheduler.mddocs/audits/unsafe-log.mddocs/decisions/0018-badge-scheme-and-reply-recv-deferral.mddocs/decisions/0019-scheduler-shape.mddocs/decisions/0020-cpu-trait-v2-context-switch.mddocs/decisions/README.mddocs/roadmap/current.mddocs/roadmap/phases/phase-a.mdhal/src/context_switch.rshal/src/cpu.rshal/src/lib.rskernel/src/lib.rskernel/src/sched/mod.rs
Comment on lines
+141
to
+149
| // SAFETY: SCHED and CPU are fully initialised before `start()`. | ||
| // Cooperative scheduling ensures no concurrent mutation. | ||
| // Audit: UNSAFE-2026-0001. | ||
| let sched = unsafe { (*SCHED.0.get()).assume_init_mut() }; | ||
| // SAFETY: same invariants as SCHED above. Audit: UNSAFE-2026-0001. | ||
| let cpu = unsafe { (*CPU.0.get()).assume_init_ref() }; | ||
| // yield_now returns Err only if current == None, which cannot | ||
| // happen once the scheduler has started. | ||
| let _ = sched.yield_now(cpu); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect global scheduler mutable borrows and switching call sites.
# Expectation: no `assume_init_mut()`-derived `&mut Scheduler` remains live across `start`/`yield_now`.
rg -n -C4 '\(\*SCHED\.0\.get\(\)\)\.assume_init_mut\(\)|\.start\(cpu\)|\.yield_now\(cpu\)|fn (start|yield_now)\b|unsafe fn (start|yield_now)\b' \
bsp-qemu-virt/src/main.rs kernel/src/sched/mod.rsRepository: cemililik/UmbrixOS
Length of output: 4426
Fix overlapping mutable borrows to scheduler across context switches.
assume_init_mut() creates an exclusive &mut Scheduler reference that persists across context switches. When start() or yield_now() switches away, the borrow remains live on the suspended task's stack. When another task resumes and calls assume_init_mut() on the same UnsafeCell, overlapping &mut Scheduler references exist simultaneously, violating Rust's aliasing model.
This occurs at lines 141–149, 171–175, and 265. Route all scheduler entry and yield operations through an unsafe raw-pointer API that avoids materializing overlapping mutable references across task context switches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bsp-qemu-virt/src/main.rs` around lines 141 - 149, The current code creates
long-lived &mut Scheduler via assume_init_mut() (SCHED) and then performs
context switches (start()/yield_now()), which can suspend a task while that
mutable borrow is live and lead to overlapping &mut references; change usages in
the blocks that call SCHED.0.get().assume_init_mut() /
CPU.0.get().assume_init_ref() (occurring around start(), yield_now(), and the
other occurrences noted) to instead obtain a raw *mut Scheduler / *const Cpu
pointer from the UnsafeCell and call new unsafe helper functions on Scheduler
(e.g., scheduler_enter_raw(ptr) / scheduler_yield_raw(ptr, cpu_ptr)) that
operate via raw pointers so no &mut Scheduler is materialized across context
switches; ensure all places that previously used sched.yield_now(cpu) or similar
are routed through these raw-pointer APIs and remove assume_init_mut() lifetimes
that span calls that can switch contexts.
…-int-log Three silent hangs during A5 debugging revealed gaps in documented boot requirements and unsafe coding rules. This commit closes those gaps so the same bugs cannot repeat unnoticed on future BSP targets. docs/standards/bsp-boot-checklist.md (new) Six-item ordered checklist for every new BSP: EL verification, CPACR_EL1 FP/SIMD enable, VBAR, SP alignment, BSS zeroing, and the naked-asm rule for context-switch functions. Each item explains what goes wrong if skipped and how to diagnose it. Includes a diagnostic cheat sheet mapping symptom → first check, and the QEMU -d int invocation. docs/standards/unsafe-policy.md — new §5a Codifies that any function saving/restoring SP in inline asm must be #[unsafe(naked)] with naked_asm!. Shows a correct and a broken example. Explains why #[inline(never)] is not sufficient (compiler still emits the stp x29,x30,[sp,#-N]! prologue). tools/run-qemu.sh — --int-log flag Passes -d int -D /tmp/qemu_int.log to QEMU when requested. Eliminates the need to remember or look up the QEMU exception-log flags mid-debug session. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
All findings from the post-A5 review verified against current code and
fixed or explicitly documented. Summary by category:
Safety / correctness
- bsp-qemu-virt/src/cpu.rs: extend Aarch64TaskContext with d8_d15 [u64;8]
and update context_switch_asm to save/restore AAPCS64 callee-saved SIMD
registers d8–d15. Total context size: 104 → 168 bytes. Without this fix
a task that causes NEON register allocation in d8–d15 would corrupt state
across a cooperative yield.
- kernel/src/sched/mod.rs: reorder add_task so ready.enqueue() is called
before task_states/task_handles are set; a QueueFull error no longer
leaves partial registration.
- bsp-qemu-virt/src/main.rs: change add_task .ok() to .expect() so setup
failures panic with a message instead of silently continuing.
- bsp-qemu-virt/src/main.rs: restructure task_a/task_b yield calls so
&mut Scheduler is not bound to a named variable that persists as a loop
local across context switches. Add UNSAFE-2026-0012 audit entry documenting
the deliberate aliasing relaxation under the cooperative model.
API / type correctness
- bsp-qemu-virt/src/cpu.rs: make QemuVirtCpu::new() unsafe to match its
# Safety invariant (at most one instance per core).
- kernel/src/sched/mod.rs: add Cpu bound to Scheduler<C> struct declaration
to match the existing impl<C: ContextSwitch + Cpu> blocks.
Documentation / comments
- bsp-qemu-virt/src/cpu.rs: fix disable_irqs SAFETY comment — the
instruction is MSR DAIFSet, not MSR DAIF.
- bsp-qemu-virt/src/cpu.rs: add (c) "why safer alternatives rejected" to
Send/Sync SAFETY comments.
- bsp-qemu-virt/src/boot.s: remove "CPACR_EL1 resets to zero" assertion;
state we write FPEN explicitly without relying on reset value.
- bsp-qemu-virt/src/main.rs: add (c) to StaticCell and TaskStack SAFETY
comments; update audit IDs to UNSAFE-2026-0010 and UNSAFE-2026-0011.
- hal/src/cpu.rs: reword IrqGuard doc comment to avoid an overly specific
codegen claim about .rodata layout.
- kernel/src/sched/mod.rs: add IRQ-state-on-entry doc to Scheduler::start;
add self-yield fast-path invariant comment in yield_now; add single-waiter
semantics doc to unblock_receiver_on.
- docs/analysis/tasks/phase-a/T-004-cooperative-scheduler.md: fix
ADR-0020 API references (save_context/restore_context → context_switch/
init_context; Cpu trait → ContextSwitch trait); update frame size comment.
- docs/decisions/0019-scheduler-shape.md: replace "Cpu trait" with
"ContextSwitch trait" in constraints and references sections.
- docs/analysis/tasks/phase-a/README.md: T-004 status → Done (2026-04-21).
Tests
- kernel/src/sched/mod.rs: replace plain [u8; 512] stacks with
AlignedStack<512> (#[repr(C, align(16))]) in Scheduler tests so the
stack_top pointer satisfies init_context's 16-byte alignment contract.
- kernel/src/sched/mod.rs: add debug_assert in ipc_recv_and_yield that
the second ipc_recv call after resume never returns Pending.
Audit log
- docs/audits/unsafe-log.md: add UNSAFE-2026-0010 (StaticCell<T> Sync),
UNSAFE-2026-0011 (TaskStack Sync), UNSAFE-2026-0012 (&mut Scheduler
aliasing across cooperative yield).
Dead code
- kernel/src/lib.rs: remove the stale pub fn run<Con, C>() stub, which
printed a greeting and spun. kernel_entry() is the real entry point and
the stub was never called.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Covers the full procedure for adding a new BSP crate to the workspace: boot-flow ADR first, crate skeleton, the six-item boot checklist from bsp-boot-checklist.md, cpu.rs with naked context switch and d8-d15, console.rs, linker script, run script with --int-log, and smoke test verification. Acceptance criteria require the naked-fn rule and SIMD register coverage to be met before the task is considered done. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Merged
cemililik
added a commit
that referenced
this pull request
Apr 27, 2026
…le SAFETY) Verified each finding; all five applied as suggested. Inline #1 — boot.md halt-loop syntax in Mermaid diagram + body text - Line 41 (Mermaid sequence diagram): `halt: wfe; b .-` → `halt_unsupported_el: wfe ; b halt_unsupported_el`. The named-label form matches the actual asm in `bsp-qemu-virt/src/boot.s` and removes the malformed `.-` token. - Line 16 (text description): `wfe; b .-` halt loop wording corrected in two places — the EL3 path now reads "named-label `wfe`-loop (`halt_unsupported_el: wfe ; b halt_unsupported_el`)" and the kernel_entry-return defensive halt now reads `wfe ; b 2b` (matching the local-label loop at the bottom of `_start`). Inline #2 — UNSAFE-2026-0017 GAS halt-loop syntax (Amendment) - §Operation said "halt via `wfe; b -1b`". `-1b` is not valid GAS syntax — `1b` is the back-reference to local label `1:`, but a leading `-` is meaningless there. Real asm uses `b halt_unsupported_el`. - §Rejected alternatives → "Halt on EL3 with a panic frame" said "`wfe; b .-` is the visible silence". `b .-` is a similar malformed token (`.` is the current address; `b .-` with no offset is not a valid branch target). - Both occurrences corrected via an Amendment block per unsafe-policy.md §3 (the entry was committed in f289d4d / T-013; in-place edits to a committed entry's body are forbidden). The behaviour the audit describes is unchanged; only the prose's asm rendering was wrong. Outside-diff #4 — bsp-qemu-virt/src/cpu.rs SAFETY blocks for CNTFRQ_EL0 (`new()`) + CNTVCT_EL0 (`now_ns()`) - Both blocks claimed "boot.s performs no EL transition" — true before T-013, stale after. Updated to reflect ADR-0024: `boot.s` drives an EL2 → EL1 transition when the firmware/emulator delivers at EL2, falls through at EL1, halts at EL3. The runtime EL-check via `tyrne_hal::cpu::current_el()` (audited under UNSAFE-2026-0018) is the checked invariant pinning `CurrentEL == 1` at this point. Both SAFETY paragraphs now name UNSAFE-2026-0017 (the boot.s sequence) explicitly, and document (a) why-unsafe-is- required, (b) invariants, (c) rejected alternatives per CLAUDE.md rule 2. Nitpick — UNSAFE-2026-0018 cfg-gating wording (Amendment) - §Invariants relied on → "Cfg-gating" said "user code reading `CurrentEL` would trap or yield `EL0` with no useful information". The "or yield `EL0`" alternative is wrong: per ARM ARM §D11.2 / §C5.2, `MRS x, CurrentEL` at EL0 is undefined — the system register is not accessible at EL0 and the read raises an Undefined Instruction exception (which becomes `SIGILL` on hosted Unix-like targets such as `aarch64-apple-darwin`). There is no fallback EL0 read. Corrected via Amendment block (same §3 reasoning as UNSAFE-2026-0017 above). Nitpick — T-008 task file:120 review-history release-note structure - Single dense paragraph split into structured bullets using `<br/>•` separators inside the table cell: New docs / Updated docs / Scope discipline / Verification / State transition. Same factual content, scannable. Verification: cargo fmt clean; host-clippy / kernel-clippy / kernel- build clean; host-test 143 / 143 green. Refs: ADR-0024 Audit: UNSAFE-2026-0015, UNSAFE-2026-0017, UNSAFE-2026-0018 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Apr 28, 2026
Two Düşük findings from the T-012-arc approval review:
1. `bsp-qemu-virt/src/main.rs::idle_entry` — function-level doc-comment
was stale: still described the body as `spin_loop` + `yield_now`
("not `wfi` + `yield_now` — until a timer IRQ source lands (T-009)")
and projected `wait_for_interrupt` as future work. The body itself
landed in commit b4ed68c using `cpu.wait_for_interrupt()`; only the
doc-comment lagged. The PR-#9 post-fix-sweep DoD rule
(code-review.md item 7 — `git grep -F` after a behaviour change)
would have caught this. Rewritten to describe the now-current
behaviour: WFI + yield, ADR-0022 first-rider Sub-rider closed,
v1's IPC demo never reaching idle in practice (so WFI is
structurally unreachable in the demo path).
2. `docs/decisions/0021-raw-pointer-scheduler-ipc-bridge.md:123` —
Amendment header read "2026-04-27" but the cited commit `28c5ce9`
landed 2026-04-28; the matching UNSAFE-2026-0014 Amendment in
`docs/audits/unsafe-log.md:225` already had the correct
2026-04-28 date. One-character fix bringing the two Amendments
into agreement on when the discipline-extension landed.
Bilgi #3 (TrapFrame `_reserved` never written by trampoline) and
Bilgi #4 (`compiler_fence` on spurious-acknowledge path is defensive)
are noted but not actioned — observation-grade only.
Gates: cargo fmt / host-clippy / kernel-clippy / kernel-build clean.
148/148 host tests unchanged (no code paths touched).
Refs: T-012, ADR-0021 (Amendment), ADR-0022 (Sub-rider closure)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
Apr 28, 2026
Code (5 fixes): 1. `bsp-qemu-virt/src/exceptions.rs` — irq_entry and panic_entry are now `unsafe extern "C" fn` (was `extern "C" fn`). Both have caller-side safety preconditions (frame validity for irq_entry; class validity for panic_entry) already documented in `# Safety` doc-comments; making them `unsafe fn` enforces the contract at the type level for any future Rust caller. The asm trampolines `bl` them unchanged. 2. `bsp-qemu-virt/src/gic.rs` — enable/disable now assert `irq.0 < GIC_MAX_IRQ` (1020) per ARM IHI 0048B §4.3.2 before computing distributor offsets. Without the check, an out-of-range IrqNumber would compute an MMIO offset outside the distributor window even with saturating arithmetic — writing to reserved or wrong-window addresses. The IrqController trait returns no Result, so panic is the right mechanism for kernel-internal contract violations. SAFETY comments updated to cite the range invariant. 3. `hal/src/timer.rs` — ns_to_ticks switched from floor to ceiling division (via `u128::div_ceil`). ADR-0010 §Decision outcome specifies `arm_deadline` semantics as "When `now_ns()` reaches or exceeds `deadline_ns`, the hardware timer IRQ fires"; flooring could arm the comparator at a tick whose time-equivalent is sub-tick *before* deadline_ns, violating the contract. Ceiling guarantees the comparator's tick-equivalent is ≥ deadline_ns. New `# Rounding` doc section explains the choice. Existing tests (round-trip at 62.5 MHz divisor frequency, one-second-yields- frequency at multiple frequencies, zero-ns, panic-on-zero-frequency) all still pass — they exercise frequencies that divide evenly into 1e9 ns/s, where ceiling and floor agree. 4. `hal/src/timer.rs::ns_to_ticks_saturates_at_u64_max` — added two over-boundary cases (`freq = 1_000_000_001` and `freq = u64::MAX`) so the saturation *branch* (not just the boundary) is exercised. The original test hit only the exact boundary (`huge_ns * 1e9 = u64::MAX * 1e9` divides exactly). 5. `bsp-qemu-virt/src/exceptions.rs::TrapFrame` — no code change; the doc-comment's "Padding" claim about `_reserved` was already correct. Reviewer's Bilgi #3 was an observation, not a fix request. Documentation (8 fixes): 6. `.claude/skills/conduct-approval-review/SKILL.md` — Turkish quote in line 30 translated to English ("kabul edilen bulguların tamamı düzeltildi mi?" → "Have all accepted findings been fixed?"); severity labels in the output-format template translated (`Yüksek`/`Orta`/`Düşük` → `High`/`Medium`/`Low`); the line-53 "Yüksek finding" prose translated; the example fenced code block gained a `markdown` language tag. CLAUDE.md §3 mandates English in all committed artefacts. 7. `docs/analysis/reviews/business-reviews/2026-04-27-B0-closure.md` — "Five `unsafe` regions" → "Seven" to match the actual table count (UNSAFE-2026-0012 through 0018 inclusive = 7 entries). 8. `docs/analysis/tasks/phase-b/README.md` — T-012 row status changed from `In Progress (design-first 2026-04-28)` to `In Review (2026-04-28)` to match the task file's header. 9. `docs/architecture/exceptions.md` — IRQ dispatch flow Mermaid diagram updated to reflect shipped behaviour: timer-IRQ branch now shows "msr CNTV_CTL_EL0 → gic.end_of_interrupt → return" (ack-and-ignore for v1) instead of the placeholder sched::on_timer_irq call. Spurious-INTID arm added (returns without EOI per GICv2 architecture spec). Saved-register list corrected: trampoline saves `x0..x18, x30` (not `x0..x29`); the prose now explicitly states that `x19..x29` are AAPCS64 callee-saved and preserved by the Rust handler, not pushed by the trampoline. 10. `docs/decisions/0021-raw-pointer-scheduler-ipc-bridge.md` — the Amendment paragraph that said "When the first scheduler-touching IRQ handler lands, ... UNSAFE-2026-0014 ... gains an Amendment" reworded to reflect that the Amendment **already exists** (commit `28c5ce9`) and v1's body has no live citation yet because `irq_entry` is ack-and-ignore. Future scheduler-touching arcs add a follow-up Amendment recording the activation. 11. `docs/roadmap/current.md` — pre-T-011 baseline coverage numbers reconciled with the canonical B0 closure retro: sched/mod.rs was `83.93%` → `84.16%`; workspace was `96.33%` → `96.37%`. The post- T-011 numbers (93.97% / 96.37%) were already correct. 12. `docs/roadmap/phases/phase-b.md` — "T-012 closed" → "T-012 delivered (pending verification)". The bullet's existing parenthetical already explained that closure waits on QEMU verification; the lead label is now consistent with that. 13. `docs/analysis/tasks/phase-b/T-012-exception-and-irq-infrastructure.md` — Dependencies field: T-009 status `In Review` → `Done` (closed 2026-04-27 via PR #9); ADR-0024 mention sharpened to record its Accept date (2026-04-27). Bilgi findings #3 (`TrapFrame._reserved` never written) and #4 (spurious-acknowledge `compiler_fence` is defensive-not-load-bearing) intentionally not actioned per their "no action today" tags. Gates: cargo fmt / host-clippy / kernel-clippy / kernel-build all clean. host-test 148 / 148 unchanged (24 hal + 90 kernel + 34 test-hal); the new ns_to_ticks_saturates_at_u64_max assertions extend an existing test rather than add a new one. Refs: T-012, ADR-0010 (rounding clarification rider candidate), ADR-0021 (Amendment reword), ADR-0024 (precondition language) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
May 8, 2026
… 2026-05-08 + 2026-05-07 follow-ups Closes the 12 remaining items flagged by the 2026-05-08 multi-axis review's §Follow-up backlog (3 Track-2 Majors, 1 Track-3 Major fix already closed in `59c08e9`, 6 Track-3 / Track-2 Minors, 2 Track-4 Minors, 2 Track-2 Nits) plus the carry-forward 2026-05-07 Track-H NIT-1. **Track 2 Majors (forward-flagged → ADR-0027 / phase-b ledger riders):** - M1 (escape-hatch doc): ADR-0027 §Decision outcome (c) gains a bullet documenting `mem::forget` / `ManuallyDrop` / `let _ = ...` as deliberate-but-rare escape hatches, mirroring the `x86_64::structures::paging::MapperFlush` precedent. - M2 (MMU-instance binding): ADR-0027 §Decision outcome (c) gains a bullet noting `MapperFlush::flush(self, mmu: &impl Mmu)` accepts any `Mmu`; multi-`Mmu` deployments (B3+ per-task `AddressSpace`, Phase C multi-CPU) will need a stronger token type. Out of scope for v1. - M3 (ADR-0034 placeholder): ADR-0027 §Decision outcome adds an "ADR-0034 (kernel-image section permissions) placeholder" block alongside ADR-0033, and `phase-b.md` ADR ledger gains rows for both ADR-0033 and ADR-0034 with named-but-unallocated discipline. **Track 3 Minors (governance / wording polish):** - m1 + m2 (current.md L52): drop "Phase-2" prefix on "§Simulation table" (the table walks Steps 0–4, not a "Phase 2"); "Accept will be" → "Accept landed as" + actual commit SHA `bb0a6ba`. - m3 (commit-style.md PR-numbering rider): new §"PR-number references in committed artefacts" subsection naming the recurrence (PR #18 + PR #20 each had a one-commit PR-number fix-up) and codifying three acceptable disciplines (defer banner authoring; reference branch slug; or use commit SHA). - n1 (framing alignment): "first to apply Simulation forward" wording in current.md (banner + Active decisions row) and phase-b.md §B2 status block aligned to the precise "first non-recovery-primitive state-machine ADR drafted under §Simulation" phrasing — ADR-0032's Propose did land with a table; the prior framing was technically defensible only under a narrow reading of "retro-extracted". **Track 2 Nits (substance riders in ADR-0027):** - #4 (DSB ISH vs DSB NSH rationale): §Simulation gains a rationale paragraph after the table — `ISH` is forward-compatible with the eventual SMP boot, sub-microsecond cost on single-core, matches Linux aarch64 `arch/arm64/mm/proc.S` for the same reason. - #5 (TCR_EL1.AS wording tighten): line 59 reworded — `AS = 0` selects 8-bit ASID *size*, not "the ASID value is 0" (the value is `TTBR0_EL1.ASID = 0` and is what's "globally used in v1"). - #7 (line 17 §-citation precision) + Track-3 n1: "first non-recovery-primitive state-machine" framing now precise. - #8 (memory-management.md L88 page-table descriptor cosmetic): ASCII bit-field diagram redrawn to match L2 block-descriptor reality (OutputAddress[47:21], not [47:12]); explanatory note added for L1-block / L3-page variants. **Track 4 Minors (perf-harness.sh):** - #1 (Ctrl-C cleanup trap): new `cleanup_in_flight` shell function + `trap '...' EXIT INT TERM` that kills any in-flight QEMU + watchdog PIDs tracked in `CURRENT_CMD_PID` / `CURRENT_WATCHDOG_PID` shell globals. `run_with_timeout` updates the globals at every call so the trap addresses whichever pair is currently in flight; clears them at every clean exit so the trap is a no-op outside iterations. - #2 (p99 small-N reporting hygiene): generated baseline report Methodology section gains a "**Note on p99 at small `n`**" paragraph explaining that under nearest-rank `p99 == max` for `n < 100` and callers should not over-read it as a tail-latency signal until `n >= 100`. **Track 4 Nit #3 (read_stats refactor):** **Already closed by PR #21 review-round commit `ef30b5c`** — the 8 `echo | awk` parses became a single `while read` loop. Verified at HEAD (`grep -c "while read -r key val" tools/perf-harness.sh` → 1 hit). **2026-05-07 Track-H NIT-1 (Pending Amendment closure-path indexing):** UNSAFE-2026-0019 / 0020 / 0021 each gain a 2026-05-08 "closure-path indexed" Amendment naming the canonical clearance trigger (B5 Milestone, ADR-0030 entry-point, deadline-arming syscall) explicitly, so a future reader of `unsafe-log.md` alone has the full picture without leaving the file. No semantic change; co-locates information that was previously distributed across `phase-b.md` cross-references. Verification gates re-run on the integration branch: - `cargo fmt --all -- --check` clean - `cargo host-test` 159/159 (25 + 100 + 34) - `cargo host-clippy` clean (-D warnings) - `cargo kernel-clippy` clean - `cargo kernel-build` clean - `tools/perf-harness.sh --iterations=3` runs end-to-end with the new trap + Methodology note + while-read parsing intact This commit + the prior 2026-05-08 review's recommendations close all follow-ups identified by both 2026-05-07 and 2026-05-08 multi-axis reviews. Forward-flagged items (10/12/13 from 2026-05-07) and any review-round bot input on this integration branch remain the only open items. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cemililik
added a commit
that referenced
this pull request
May 9, 2026
Doc-only fixup commit on the B3-prep PR. Reviewer Approve-with-nits verdict; all 10 actionable findings applied. No merge-blocker; cargo gates unchanged from main (185/185 host; clippy / fmt / kernel-build clean). Applied (Minor): 1. **Finding 3.1 — §Simulation row 3 wrap-scan rationale.** The "two-pass scan ensures correctness even under hint-past-last- free" claim describes a state that cannot occur in v1 (the unconditional rewind in row 2 keeps `hint ≤ lowest-free-index`). Reworded row 3 to honestly frame the wrap as forward-compat scaffolding for the future SMP extension where per-core caches may leave the global hint stale. 2. **Finding 4.1 — UNSAFE-2026-0001 umbrella adjudication-deferred.** The reviewer correctly noted that UNSAFE-2026-0001's actual scope (PL011 MMIO base blessing) is semantically distant from PMM frame-zeroing (raw write_bytes over normal-cached RAM). Added an explicit "adjudication-deferred to T-017's security review" soft-flag in §Dependency chain step 5, propagated to §Decision drivers + §Decision outcome + Option A pros + T-017 §Approach commit 2. The Amendment-vs-new-entry call is now honestly framed as a borderline judgment for T-017's review; either outcome is bounded. 3. **Finding 8.1 — repurposed T-017 test #10.** The original `free_frame_rejects_pa_in_reserved_range` overlapped with test #4 (which already pinned `free_frame(reserved)` → `DoubleFree`). Repurposed to `free_frame_reserved_check_iterates_only_populated_slots` to pin the populated-slots-only contract on the defensive scan (genuinely O(populated-entries), not O(R)). Combined coverage is now: test #4 = "reserved-PA correctly rejected"; test #10 = "non-reserved-PA correctly accepted under partially-populated array". 4. **Finding 8.2 — `MAX_RESERVED_RANGES` per-BSP const generic.** The hardcoded `8` was unjustified and risked TooManyReservedRanges on future BSPs whose typical aarch64 boot-stack reservations (DTB + ATF + ACPI + initrd + framebuffer) could land 7–9 ranges. Promoted to a per-BSP const generic `R` consistent with the existing `PMM_BITMAP_BYTES` `N` parameterisation: the kernel crate exposes `Pmm<const N: usize, const R: usize>`; v1's `bsp-qemu-virt` picks `R = 8`; future BSPs pick what they need. The `expect("...")` BSP panic is structurally unreachable when `R` is sized per the BSP's static reservation list. 5. **Finding 10.1 — Option A regime-independence qualifier.** The bare "regime-independent (high-half-portable)" claim in §Pros didn't carry the qualifier from §Decision outcome / §Positive ("metadata addressing"). Added the same qualifier to §Pros and noted that the frame-zeroing step's `*mut u8` from PA is identity-mapping-dependent today — a `phys_to_virt` helper is needed post-high-half (but the same helper would be needed for the rejected Option C, so Option A's regime-independence advantage at the algorithm-and-metadata level still stands). Applied (Nit): 6. **Finding 3.2 — §Simulation row 0 `.bss` zero-fill prose.** The "bitmap zero-initialised by `.bss` zero-fill" precondition was technically true but not load-bearing — `Pmm::new` constructs a fully-initialised value. Reworded row 0 to acknowledge this: "the zero-init is precondition-only, not load-bearing". 7. **Finding 5.1 — Option B u32/u16 indices math.** The 128 KiB metadata claim assumed `u32` indices; `u16` halves to 64 KiB for v1's 32 K frames. Tightened the math with a parenthetical ("64 KiB with `u16` since 32 K fits in 15 bits — but `u32` is forward-portable to BSPs with > 64 K frames such as Pi 4 with 1 M frames at 4 GiB"). Qualitative judgment unchanged (still 16–32× the bitmap). 8. **Finding 8.3 — §Approach commit 1 test set.** Moved `new_rejects_too_many_reserved_ranges` from "implicit later commit" to commit 1 alongside `new_marks_reserved_ranges_and_initialises_counters` (both pin `Pmm::new`, so they belong in the constructor commit). 9. **Finding 9 — UNSAFE-2026-0001 umbrella name consistency.** The ADR's prose drifted between "kernel-static-buffer raw-pointer write to identity-mapped memory" (L35) and "MMIO + kernel-static- buffer raw-pointer" (L58). Picked the former as the canonical phrasing and aligned both surfaces. 10. **Finding 12.1 — T-016 §"Why one bundled task" disambiguation.** The line "instead of T-016 + T-017 + T-018?" was written when those numbers were hypothetical sub-task splits of T-016 itself. Post-T-017 (PMM) + T-018 (AddressSpace, future) numbering, the line risked confusion. Added a parenthetical clarifying it's about a hypothetical 3-way split of T-016, not the actual post-T-016 numbering. Bonus — Finding 12.2 (kernel-internal framing) — slight cosmetic qualifier added: "kernel-internal at runtime" + a parenthetical clarifying that the BSP touches the PMM only at construction time (linker-symbol → Pmm::new → StaticCell publication) and never afterwards. Verification: - cargo fmt --check clean - cargo test 185/185 host (unchanged — docs only) - cargo kernel-build clean Files touched: - docs/decisions/0035-physical-memory-manager.md (+30 / -20) - docs/analysis/tasks/phase-b/T-017-physical-memory-manager.md (+18 / -10) - docs/analysis/tasks/phase-b/T-016-mmu-activation.md (+1 / -1) This commit completes the PR #25 review-round; all reviewer-flagged findings closed. The Accept commit (2de67f1) carrying ADR-0035 → Accepted stays as the load-bearing flip; this fixup commit is the honesty / consistency polish pass on top. Refs: ADR-0035, T-017
This was referenced May 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce a cooperative, single-core scheduler with a HAL-level context-switch trait and QEMU
virtCPU implementation, and wire up a two-task smoke test on the BSP, along with roadmap, ADR, and unsafe-audit documentation updates.New Features:
ContextSwitchHAL trait and implement it for the QEMUvirtCPU with an AArch64 task-context structure and inline context-switch assembly.kernel::schedmodule providing a bounded ready queue, per-task state tracking, and IPC-aware cooperative scheduling APIs.Enhancements:
IrqGuardto be generic over the concreteCputype to avoid fat-pointer vtable issues.Documentation:
reply_recv, and document the cooperative scheduler task (T-004).Tests:
Summary by CodeRabbit
Release Notes
New Features
Documentation