Development - #5
Merged
Merged
Conversation
Task A sends a capability-gated message to Task B through an endpoint; B replies; both tasks exit cleanly — proving the Phase A exit bar end-to-end. 7 acceptance criteria; guide, baseline perf review, and business review required before Done. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Task B registers as receiver on a capability-gated endpoint; Task A sends a message (label=0xAAAA); B receives and replies (label=0xBBBB); A receives the reply. Full IPC round-trip through ipc_send_and_yield / ipc_recv_and_yield and two cooperative context switches confirmed on QEMU virt aarch64. Docs: two-task-demo.md guide; A6 baseline perf review (80 KiB image, 17.5 KiB .bss, 27-instruction context switch); A2–A6 Phase A business retrospective. UNSAFE-2026-0012 extended to cover IPC statics aliasing. Phase A complete. 109 host tests passing; QEMU smoke confirmed. Refs: ADR-0017, ADR-0019, ADR-0020 Audit: UNSAFE-2026-0012 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Reviewer's GuideImplements the A6 two-task IPC demo on QEMU by replacing the previous scheduler smoke-test tasks with a capability-gated send/reply flow, wiring in global IPC state in the BSP, and documenting the milestone closure, unsafe aliasing rationale, and Phase A completion across roadmap, reviews, and guides. Class diagram for BSP IPC infrastructure and tasks in A6classDiagram
class StaticCell_T {
+UnsafeCell_MaybeUninit_T_0
+new() StaticCell_T
+get() *mut MaybeUninit_T
}
class Scheduler_QemuVirtCpu {
+new() Scheduler_QemuVirtCpu
+add_task(cpu QemuVirtCpu, handle TaskHandle, entry fn, stack_top usize) Result_void
+start(cpu QemuVirtCpu) -> !
+yield_now(cpu QemuVirtCpu) Result_void
+ipc_send_and_yield(cpu QemuVirtCpu, ep_arena EndpointArena, queues IpcQueues, table CapabilityTable, cap CapHandle, msg Message, badge Option_u64) Result_void
+ipc_recv_and_yield(cpu QemuVirtCpu, ep_arena EndpointArena, queues IpcQueues, table CapabilityTable, cap CapHandle) Result_RecvOutcome
}
class QemuVirtCpu {
+new() QemuVirtCpu
+context_switch(prev *mut Aarch64TaskContext, next *const Aarch64TaskContext)
}
class EndpointArena {
+default() EndpointArena
+allocate(ep Endpoint) Result_EndpointHandle
}
class Endpoint {
+new(id u32) Endpoint
}
class IpcQueues {
+new() IpcQueues
}
class CapabilityTable {
+new() CapabilityTable
+insert_root(cap Capability) Result_CapHandle
}
class Capability {
+new(rights CapRights, object CapObject) Capability
}
class CapObject {
}
class CapRights {
<<enumeration>>
SEND
RECV
DUPLICATE
}
class CapHandle {
+index usize
}
class Message {
+label u64
+params u64[3]
}
class RecvOutcome {
<<enumeration>>
Received
WouldBlock
InvalidCap
}
class TaskArena {
+default() TaskArena
+create_task(task Task) Result_TaskHandle
}
class Task {
+new(id u32) Task
}
class TaskHandle {
+index usize
}
class Pl011Uart {
+new(base usize) Pl011Uart
+write_bytes(buf *const u8)
}
class KernelEntry {
+kernel_entry() -> !
}
class TaskAEntry {
+task_a() -> !
}
class TaskBEntry {
+task_b() -> !
}
StaticCell_T <|-- StaticCell_Scheduler_QemuVirtCpu
StaticCell_T <|-- StaticCell_QemuVirtCpu
StaticCell_T <|-- StaticCell_Pl011Uart
StaticCell_T <|-- StaticCell_EndpointArena
StaticCell_T <|-- StaticCell_IpcQueues
StaticCell_T <|-- StaticCell_CapabilityTable_A
StaticCell_T <|-- StaticCell_CapabilityTable_B
StaticCell_T <|-- StaticCell_CapHandle_A
StaticCell_T <|-- StaticCell_CapHandle_B
class StaticCell_Scheduler_QemuVirtCpu {
}
class StaticCell_QemuVirtCpu {
}
class StaticCell_Pl011Uart {
}
class StaticCell_EndpointArena {
}
class StaticCell_IpcQueues {
}
class StaticCell_CapabilityTable_A {
}
class StaticCell_CapabilityTable_B {
}
class StaticCell_CapHandle_A {
}
class StaticCell_CapHandle_B {
}
KernelEntry --> Pl011Uart : initialises
KernelEntry --> QemuVirtCpu : initialises
KernelEntry --> TaskArena : allocates_tasks
KernelEntry --> EndpointArena : allocates_endpoint
KernelEntry --> CapabilityTable : creates_TableA_TableB
KernelEntry --> IpcQueues : initialises
KernelEntry --> Scheduler_QemuVirtCpu : configures
Scheduler_QemuVirtCpu --> QemuVirtCpu : uses_for_context_switch
Scheduler_QemuVirtCpu --> EndpointArena : uses_for_ipc
Scheduler_QemuVirtCpu --> IpcQueues : tracks_ipc_state
Scheduler_QemuVirtCpu --> CapabilityTable : validates_caps
Capability --> CapRights
Capability --> CapObject
CapabilityTable --> Capability : stores
CapabilityTable --> CapHandle : returns
EndpointArena --> Endpoint : owns
TaskArena --> Task : owns
TaskArena --> TaskHandle : returns
TaskAEntry --> Scheduler_QemuVirtCpu : calls_ipc_send_and_yield
TaskAEntry --> Scheduler_QemuVirtCpu : calls_ipc_recv_and_yield
TaskBEntry --> Scheduler_QemuVirtCpu : calls_ipc_recv_and_yield
TaskBEntry --> Scheduler_QemuVirtCpu : calls_ipc_send_and_yield
TaskAEntry --> Pl011Uart : logs
TaskBEntry --> Pl011Uart : logs
KernelEntry --> TaskAEntry : registers_entry
KernelEntry --> TaskBEntry : registers_entry
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 17 minutes and 14 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 (7)
📝 WalkthroughWalkthroughReplaces the prior two-task cooperative scheduler smoke test with an A6 two-task IPC end-to-end demo: kernel entry allocates endpoints and per-task capability tables, registers Task B then Task A, and tasks perform a capability-controlled send/receive round-trip via scheduler IPC bridge calls. Supporting docs, reviews, and unsafe-audit scope were added/updated. Changes
Sequence Diagram(s)sequenceDiagram
participant TaskA as Task A
participant Sched as Scheduler
participant TaskB as Task B
participant IPC as IPC Subsystem
rect rgba(100, 150, 255, 0.5)
Note over TaskA,IPC: Capability-controlled IPC round-trip
end
TaskA->>IPC: ipc_send_and_yield(message)
IPC->>Sched: enqueue/send state, suspend Task A
Sched->>TaskB: resume Task B
TaskB->>IPC: ipc_recv_and_yield()
IPC->>TaskB: deliver message label, suspend Task B
Sched->>TaskA: resume Task A
TaskA->>IPC: ipc_recv_and_yield() (await reply)
Sched->>TaskB: resume Task B
TaskB->>IPC: ipc_send_and_yield(reply)
IPC->>TaskB: enqueue reply, suspend Task B
Sched->>TaskA: resume Task A
TaskA->>TaskA: receive reply, print completion
rect rgba(200, 200, 200, 0.5)
Note over Sched: Tasks idle → spin/wfe
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 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 |
Review Summary by QodoA6 two-task IPC demo — Phase A exit bar met
WalkthroughsDescription• Implements end-to-end two-task IPC demo proving Phase A exit bar • Task B receives capability-gated message from Task A, replies; both exit cleanly • Adds IPC infrastructure: endpoint arena, queues, capability tables for both tasks • Replaces yield-loop smoke test with full IPC round-trip using ipc_send_and_yield / ipc_recv_and_yield • Includes comprehensive documentation: guide, baseline performance review, Phase A retrospective Diagramflowchart LR
A["Task A<br/>initiator"] -->|ipc_send_and_yield| EP["Endpoint<br/>capability-gated"]
EP -->|ipc_recv_and_yield| B["Task B<br/>responder"]
B -->|ipc_send_and_yield<br/>reply| EP
EP -->|ipc_recv_and_yield| A
A -->|clean exit| DONE["Phase A<br/>complete"]
File Changes1. bsp-qemu-virt/src/cpu.rs
|
Code Review by Qodo
1.
|
![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 left some high level feedback:
- In
kernel_entryyou grant both TABLE_A and TABLE_B capabilities withSEND | RECV | DUPLICATEon the same endpoint even though the demo only needsSEND | RECV, which both weakens the minimal-rights story and diverges from the T-005 acceptance text (separate send/recv caps); consider tightening the rights and/or updating the task doc to match the actual capability layout. - The guide claims that
core::hint::spin_loop()"compiles towfe", which is not guaranteed across LLVM versions or optimisation levels; either adjust the wording to describe it as an implementation-defined hint or introduce an explicitwfehelper if you want to rely on that specific instruction.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `kernel_entry` you grant both TABLE_A and TABLE_B capabilities with `SEND | RECV | DUPLICATE` on the same endpoint even though the demo only needs `SEND | RECV`, which both weakens the minimal-rights story and diverges from the T-005 acceptance text (separate send/recv caps); consider tightening the rights and/or updating the task doc to match the actual capability layout.
- The guide claims that `core::hint::spin_loop()` "compiles to `wfe`", which is not guaranteed across LLVM versions or optimisation levels; either adjust the wording to describe it as an implementation-defined hint or introduce an explicit `wfe` helper if you want to rely on that specific instruction.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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 completes Milestone A6 by implementing a capability-gated IPC round-trip demo between two tasks on QEMU virt aarch64. It introduces global IPC infrastructure, including endpoint arenas and capability tables, and updates the boot sequence to initialize these components. The PR also adds a performance baseline and a Phase A retrospective. Review feedback highlights opportunities to improve error-handling consistency for scheduler calls and to align the storage of the task arena with the established global pattern for kernel objects.
Comment on lines
+240
to
+242
| let _ = (*SCHED.0.get()) | ||
| .assume_init_mut() | ||
| .yield_now((*CPU.0.get()).assume_init_ref()); |
There was a problem hiding this comment.
The result of yield_now is ignored here using let _ =, whereas all other scheduler and IPC calls in this file use .expect() to handle potential errors. For consistency and to ensure that any unexpected scheduler state (e.g., current being None) is caught during the demo, this call should also be checked.
(*SCHED.0.get())
.assume_init_mut()
.yield_now((*CPU.0.get()).assume_init_ref())
.expect("task B: yield_now failed");
Comment on lines
369
to
+371
| let mut arena = TaskArena::default(); | ||
| // Infallible: arena capacity is 16 and we allocate 2 tasks. | ||
| let handle_a = create_task(&mut arena, Task::new(0)).ok().unwrap(); | ||
| let handle_b = create_task(&mut arena, Task::new(1)).ok().unwrap(); | ||
| let handle_a = create_task(&mut arena, Task::new(0)).expect("create_task A failed"); | ||
| let handle_b = create_task(&mut arena, Task::new(1)).expect("create_task B failed"); |
There was a problem hiding this comment.
The TaskArena is currently allocated as a local variable on the stack of kernel_entry, which differs from the pattern used for EndpointArena and CapabilityTables (which are stored in global StaticCells). While this works for the current demo because kernel_entry never returns and the scheduler only tracks handles and contexts, it prevents other parts of the kernel from looking up Task objects by their handles. Adhering to the global storage pattern established in ADR-0016 for all kernel object types would improve architectural consistency and support future features like task destruction or status queries.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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/main.rs`:
- Around line 212-243: The comment and explicit yield are stale because
ipc_send_and_yield already yields on Enqueued; remove the redundant yield_now
call and its comment in the block that calls ipc_send_and_yield (the unsafe
block invoking SCHED.assume_init_mut().ipc_send_and_yield(...)) and update the
surrounding comment to reflect that ipc_send_and_yield yields to Task A;
alternatively, if you intend the send not to auto-yield, change the scheduler
bridge behavior in kernel/src/sched/mod.rs instead of keeping this extra
yield—do not leave both behaviors in place (references: ipc_send_and_yield,
yield_now, ipc_recv_and_yield, SCHED, CPU, EP_ARENA, IPC_QUEUES, TABLE_B,
EP_CAP_B).
- Around line 379-382: The ep_rights variable grants unnecessary
CapRights::DUPLICATE for the demo endpoint; remove CapRights::DUPLICATE so
ep_rights is only CapRights::SEND | CapRights::RECV because the code never
duplicates or transfers the endpoint (ipc_* calls pass None). Update the binding
that defines ep_rights to drop CapRights::DUPLICATE and ensure any comments
reflect the reduced least-privilege set.
- Around line 175-197: The unsafe block around the ipc_recv_and_yield call
documents aliasing/invariants but misses the required "safer-alternative
rationale"; update the unsafe comment above the block that contains SCHED,
EP_ARENA, IPC_QUEUES, TABLE_B, EP_CAP_B, CPU and the ipc_recv_and_yield call to
include a short justification explaining why safer alternatives were rejected
(e.g., "raw-pointer scheduler APIs deferred to Phase B refactor; current bridge
APIs require &mut"), while keeping the existing aliasing invariant and audit ID
(UNSAFE-2026-0012); apply the same addition to the other unsafe blocks you
flagged (the ones wrapping the same symbols/calls at the later sites).
In `@docs/analysis/reviews/business-reviews/2026-04-21-A6-completion.md`:
- Line 115: The relative link "[UNSAFE-2026-0012](../../audits/unsafe-log.md)"
in docs/analysis/reviews/business-reviews/ is incorrect; update that markdown
link target to "../../../audits/unsafe-log.md" so it points to the actual audit
file under docs/audits/ (i.e., replace ../../audits/unsafe-log.md with
../../../audits/unsafe-log.md).
- Around line 49-57: The code block showing the QEMU trace fence is missing a
language tag and triggers MD040; update the fence by adding the "text" language
tag (i.e., change the opening ``` to ```text) around the trace shown (the lines
beginning with "umbrix: hello from kernel_main" through "umbrix: all tasks
complete") so the block is treated as plain text.
In `@docs/analysis/tasks/phase-a/T-005-two-task-ipc-demo.md`:
- Around line 30-46: Update the acceptance criteria to match the actual
implemented demo: change the deterministic QEMU trace lines to reflect the
current log (including Task B’s waiting line and the exact labels emitted by
kernel_main and tasks), mark checklist items that are implemented as [x], revise
the capability discipline bullet to describe how caps are actually split (Send
for A, Recv index passed to B) and that no raw object access is used, change the
IPC bullet to require use of ipc_send_and_yield / ipc_recv_and_yield (or note
the current implementation if different), and update the clean-exit bullet to
reflect the real shutdown mechanism used by kernel_entry (whether wfe or
spin-loop detection) so the criteria mirror the committed guide/code; apply the
same reconciliation changes to the corresponding checklist blocks at lines noted
(61-65 and 85-87) so all acceptance items match the implementation and completed
items are checked off.
- Around line 33-40: The fenced code block showing the expected trace should
include a language tag to satisfy MD040; update the block in
T-005-two-task-ipc-demo.md so the opening fence is ```text (not just ```), e.g.
change the trace fence containing "umbrix: hello from kernel_main" ... "umbrix:
all tasks complete" to use ```text so the console output is properly tagged.
In `@docs/guides/two-task-demo.md`:
- Around line 57-59: The demo trace incorrectly shows Task B explicitly calling
yield_now before Task A runs; update the narrative so that when B's
ipc_send_and_yield results in the endpoint transitioning to SendPending with
outcome Enqueued, the scheduler bridge yields immediately (so Task A resumes
without B having to call yield_now). Modify the steps involving
ipc_recv_and_yield, ipc_recv, ipc_send_and_yield, and yield_now to state that
Enqueued triggers an automatic yield via the scheduler bridge, matching the
behavior implemented around the Enqueued handling in the scheduler bridge and
the fix referenced in sched/mod.rs and bsp-qemu-virt's main.
- Around line 16-19: The fenced code blocks in docs/guides/two-task-demo.md are
missing language tags (triggering MD040); update each command block (e.g., the
blocks containing "cargo kernel-build", "tools/run-qemu.sh", "tools/run-qemu.sh
--release", and "tools/run-qemu.sh --int-log") to use ```shell and change the
expected console output block showing the umbrix lines (e.g., "umbrix: hello
from kernel_main" through "umbrix: all tasks complete") to use ```text so the
markdown linter passes and the intent of each block is clear.
- Line 82: Clarify the sentence about capability isolation to state that while
tasks may receive EP_ARENA via the scheduler IPC bridge, they never dereference
endpoint objects directly; endpoint resolution must still go through each task's
CapabilityTable and the EndpointArena is only referenced by the scheduler.
Update the line mentioning "No capability escapes its owner's table. The tasks
never access the `EndpointArena` directly." to explicitly mention `EP_ARENA`,
`EndpointArena`, the scheduler IPC bridge, and `CapabilityTable` so readers
understand the scheduler may pass the arena handle but tasks must resolve
endpoints only via their `CapabilityTable`.
- Line 47: The doc incorrectly states that core::hint::spin_loop() "compiles to
`wfe`"; update the sentence to remove the inaccurate instruction reference and
use the suggested clearer wording: replace the claim about `wfe` with "Task A
enters a spin loop; QEMU continues running but produces no further output."
Ensure the symbol core::hint::spin_loop() and the mention of Task A remain for
context but do not assert any specific generated instruction (e.g., remove
references to `wfe` or `ISB SY`).
🪄 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: c2efc9fe-f422-4506-bb7f-396cfc7d9cbe
📒 Files selected for processing (12)
bsp-qemu-virt/src/cpu.rsbsp-qemu-virt/src/main.rsdocs/analysis/reviews/business-reviews/2026-04-21-A6-completion.mddocs/analysis/reviews/performance-optimization-reviews/2026-04-21-A6-baseline.mddocs/analysis/tasks/phase-a/README.mddocs/analysis/tasks/phase-a/T-004-cooperative-scheduler.mddocs/analysis/tasks/phase-a/T-005-two-task-ipc-demo.mddocs/audits/unsafe-log.mddocs/guides/two-task-demo.mddocs/roadmap/current.mddocs/roadmap/phases/phase-a.mdkernel/src/sched/mod.rs
Comment on lines
+212
to
243
| // Send reply. Since Task A is in the ready queue (not yet blocked on recv), | ||
| // this transitions the endpoint to SendPending and returns Enqueued — no | ||
| // auto-yield. An explicit yield_now follows so Task A can collect the reply. | ||
| let reply = Message { | ||
| label: 0xBBBB, | ||
| params: [0; 3], | ||
| }; | ||
| // SAFETY: same aliasing invariants as the ipc_recv_and_yield call above. | ||
| // Audit: UNSAFE-2026-0012. | ||
| unsafe { | ||
| (*SCHED.0.get()) | ||
| .assume_init_mut() | ||
| .ipc_send_and_yield( | ||
| (*CPU.0.get()).assume_init_ref(), | ||
| (*EP_ARENA.0.get()).assume_init_mut(), | ||
| (*IPC_QUEUES.0.get()).assume_init_mut(), | ||
| (*TABLE_B.0.get()).assume_init_mut(), | ||
| *(*EP_CAP_B.0.get()).assume_init_ref(), | ||
| reply, | ||
| None, | ||
| ) | ||
| .expect("task B: ipc_send reply failed"); | ||
|
|
||
| // Yield explicitly so Task A can receive the reply that was just queued | ||
| // as SendPending. Without this yield, A's ipc_recv_and_yield would never | ||
| // run (cooperative scheduling; B never blocks again after the send). | ||
| // yield_now returns Err only when current == None, which cannot happen | ||
| // once the scheduler has started. | ||
| unsafe { | ||
| let _ = (*SCHED.0.get()) | ||
| .assume_init_mut() | ||
| .yield_now((*CPU.0.get()).assume_init_ref()); | ||
| } | ||
| let _ = (*SCHED.0.get()) | ||
| .assume_init_mut() | ||
| .yield_now((*CPU.0.get()).assume_init_ref()); | ||
| } |
There was a problem hiding this comment.
Align the reply-yield flow with ipc_send_and_yield semantics.
ipc_send_and_yield already yields on Enqueued per kernel/src/sched/mod.rs, so the “no auto-yield” comment and explicit yield_now path are stale. With Task A spinning after Line 318, Task B may never resume past the reply send, so the “done; spinning” path is not reached.
Either update the scheduler bridge to match this intended behavior, or remove the extra yield/comment and adjust the demo/docs to reflect that the reply send itself yields to Task A.
Also applies to: 318-322
🤖 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 212 - 243, The comment and explicit
yield are stale because ipc_send_and_yield already yields on Enqueued; remove
the redundant yield_now call and its comment in the block that calls
ipc_send_and_yield (the unsafe block invoking
SCHED.assume_init_mut().ipc_send_and_yield(...)) and update the surrounding
comment to reflect that ipc_send_and_yield yields to Task A; alternatively, if
you intend the send not to auto-yield, change the scheduler bridge behavior in
kernel/src/sched/mod.rs instead of keeping this extra yield—do not leave both
behaviors in place (references: ipc_send_and_yield, yield_now,
ipc_recv_and_yield, SCHED, CPU, EP_ARENA, IPC_QUEUES, TABLE_B, EP_CAP_B).
Full-project review of all code committed from project inception through Phase A exit (Phase 1–4c bootstrap + A1–A6 kernel core). Code review verdict Approve with four non-blocking follow-ups; security review verdict Changes requested with three Phase-B blockers (UNSAFE-2026-0012 aliasing, cross-table revocation gap, Scheduler deadlock panic) — none of which gate the A6 exit bar. Four review-type README indexes updated. Refs: ADR-0013 Code-Review: docs/analysis/reviews/code-reviews/2026-04-21-umbrix-to-phase-a.md Security-Review: @cemililik (+ Claude agent) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Three narrow doc patches that land the non-code follow-ups from the 2026-04-21 security review (Kova 1): - security-model.md: qualify "Revocation is transitive" invariant with the v1 single-table scope; add open questions for cross-table CDT and early IRQ masking in BSP reset vectors. - two-task-demo.md: note that symmetric SEND|RECV grant on one endpoint is demo convenience, not a server topology; cite the ADR-0018 badge deferral. - unsafe-log.md: record that UNSAFE-2026-0008's save set intentionally omits TPIDR_EL0/TPIDRRO_EL0 in v1; extend the audit trail with the 2026-04-21 security-review sign-off. No code changes; the three Phase-B blockers (UNSAFE-2026-0012 aliasing, cross-table revocation gap, Scheduler deadlock panic) still require their own ADR + task and are tracked as Kova 2 work. Refs: ADR-0013 Security-Review: docs/analysis/reviews/security-reviews/2026-04-21-umbrix-to-phase-a.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Address inline review findings across code and docs. No behavioural change; 109 host tests still pass, kernel image still builds clean. Code: - bsp-qemu-virt/src/main.rs: tighten ep_rights to CapRights::SEND | CapRights::RECV (drop DUPLICATE; the demo never calls cap_copy / cap_derive and never transfers the endpoint cap, so DUPLICATE is dead weight and the minimal-rights story is clearer without it). - bsp-qemu-virt/src/main.rs: extend all four UNSAFE-2026-0012 aliasing SAFETY blocks with the "Rejected alternatives" rationale required by unsafe-policy.md §1 (previously carried in the audit entry only). - bsp-qemu-virt/src/main.rs: convert `let _ = yield_now(...)` after Task B's reply into `.expect(...)` for consistency with the surrounding IPC call sites. - kernel/src/sched/mod.rs: backtick `init_context` in a sched-test doc comment (pre-existing doc_markdown lint surfaced by today's host-clippy run). Docs: - T-005: acceptance criteria refreshed to match the actually-shipped demo — trace lines, per-task CapabilityTable with SEND|RECV root cap, clean-exit via core::hint::spin_loop (not wfe), all checkboxes marked done. - 2026-04-21-A6-completion: fix stale ../../audits link to ../../../ audits; add `text` language tag to the QEMU trace fence (MD040). - two-task-demo.md: add `shell` / `text` language tags on fenced blocks (MD040); remove unsubstantiated "compiles to wfe" claim on core::hint::spin_loop (LLVM-implementation-defined); clarify that tasks hand &mut EP_ARENA to the scheduler but never dereference endpoint storage themselves — all authority checks go through each task's own CapabilityTable. Unchanged (verified against current code, findings were stale): - The IPC round-trip still requires Task B's explicit yield_now after the reply — ipc_send_and_yield only auto-yields on Delivered, not Enqueued (see sched/mod.rs:349 and the in-code comment). Refs: ADR-0013 Code-Review: docs/analysis/reviews/code-reviews/2026-04-21-umbrix-to-phase-a.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/architecture/security-model.md (1)
289-289: Consider restructuring this invariant for readability.This 400+ word single bullet point correctly describes the cross-table revocation limitation and accurately reflects the implementation (confirmed by
insert_rootcreating new roots andcap_revoketraversing only within-table). However, the density makes it difficult to extract the key facts:
- The core invariant (revocation is transitive within a table)
- The v1 limitation (IPC transfer breaks the parent-child link)
- The consequence (sender cannot revoke IPC-transferred capabilities)
- The workarounds (transfer a child you keep locally, or accept independent root)
- The citation (security review)
📝 Proposed restructure for clarity
-- **Revocation is transitive over a derivation subtree *within a single capability table*.** Revoking a parent invalidates all derived children atomically. In v1, IPC transfer installs the received capability as a new root in the receiver's table (the `ipc_recv` path calls `insert_root`), discarding the cross-table parent-child link — so `cap_revoke` in the sender does not reach copies that have been moved over IPC. Cross-table revocation (a cross-task capability derivation tree) is an open question tracked below; until it is answered, a sender that needs to retain revoke authority over an IPC-transferred right must either (a) refuse to transfer the derived cap and instead transfer a distinct derived child it keeps locally, or (b) accept that the receiver now holds an independent root. Recorded by the [security review of Phase A exit](../analysis/reviews/security-reviews/2026-04-21-umbrix-to-phase-a.md). +- **Revocation is transitive over a derivation subtree *within a single capability table*.** Revoking a parent invalidates all derived children atomically. + - **v1 limitation:** IPC transfer installs the received capability as a new root in the receiver's table (via `insert_root`), discarding the cross-table parent-child link. Consequently, `cap_revoke` in the sender does not reach capabilities that have been transferred over IPC. + - **Workaround:** A sender that needs to retain revoke authority over an IPC-transferred right must either (a) refuse to transfer the derived capability and instead transfer a distinct derived child while keeping the original, or (b) accept that the receiver now holds an independent root. + - Cross-table revocation (a cross-task capability derivation tree) is an open question tracked below. Recorded by the [security review of Phase A exit](../analysis/reviews/security-reviews/2026-04-21-umbrix-to-phase-a.md).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/architecture/security-model.md` at line 289, The long dense bullet should be split into a concise structured invariant and supporting points: state the core invariant first ("Revocation is transitive over a derivation subtree within a single capability table"), then explicitly note the v1 limitation that IPC transfer installs received caps as new roots (ipc_recv calls insert_root), explain the consequence that cap_revoke traverses only within-table so sender revocation doesn't affect IPC-transferred copies, list the two workarounds (refuse transfer and transfer a separate derived child you keep locally, or accept the independent root), and finally include the citation to the security review; reorganize the paragraph into these discrete lines or sub-bullets to improve readability while keeping the same technical facts and the referenced symbols (insert_root, ipc_recv, cap_revoke).
🤖 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/main.rs`:
- Around line 244-263: Task B's final console.write_bytes("umbrix: task B --
done; spinning") is effectively unreachable because after
SCHED.assume_init_mut().yield_now(...) hands control back to Task A, A's
ipc_recv_and_yield() does not yield again and enters its spin loop, so B never
runs its epilogue; fix by either removing the unreachable console write in Task
B (the CONSOLE write_bytes call) or make Task A perform one more cooperative
yield (call SCHED.assume_init_mut().yield_now(...) / yield_now via the same API
used elsewhere) just after ipc_recv_and_yield() returns and before A enters its
spin loop so B can run its final epilogue — update the code paths around
ipc_recv_and_yield, the follow-up yield, and the CONSOLE write_bytes in Task B
accordingly.
---
Nitpick comments:
In `@docs/architecture/security-model.md`:
- Line 289: The long dense bullet should be split into a concise structured
invariant and supporting points: state the core invariant first ("Revocation is
transitive over a derivation subtree within a single capability table"), then
explicitly note the v1 limitation that IPC transfer installs received caps as
new roots (ipc_recv calls insert_root), explain the consequence that cap_revoke
traverses only within-table so sender revocation doesn't affect IPC-transferred
copies, list the two workarounds (refuse transfer and transfer a separate
derived child you keep locally, or accept the independent root), and finally
include the citation to the security review; reorganize the paragraph into these
discrete lines or sub-bullets to improve readability while keeping the same
technical facts and the referenced symbols (insert_root, ipc_recv, cap_revoke).
🪄 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: 7577c204-76a4-4931-8e19-eae1613ef133
📒 Files selected for processing (13)
bsp-qemu-virt/src/main.rsdocs/analysis/reviews/business-reviews/2026-04-21-A6-completion.mddocs/analysis/reviews/business-reviews/README.mddocs/analysis/reviews/code-reviews/2026-04-21-umbrix-to-phase-a.mddocs/analysis/reviews/code-reviews/README.mddocs/analysis/reviews/performance-optimization-reviews/README.mddocs/analysis/reviews/security-reviews/2026-04-21-umbrix-to-phase-a.mddocs/analysis/reviews/security-reviews/README.mddocs/analysis/tasks/phase-a/T-005-two-task-ipc-demo.mddocs/architecture/security-model.mddocs/audits/unsafe-log.mddocs/guides/two-task-demo.mdkernel/src/sched/mod.rs
✅ Files skipped from review due to trivial changes (7)
- docs/analysis/reviews/business-reviews/README.md
- kernel/src/sched/mod.rs
- docs/analysis/reviews/performance-optimization-reviews/README.md
- docs/analysis/reviews/code-reviews/README.md
- docs/analysis/tasks/phase-a/T-005-two-task-ipc-demo.md
- docs/guides/two-task-demo.md
- docs/analysis/reviews/security-reviews/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/analysis/reviews/business-reviews/2026-04-21-A6-completion.md
- docs/audits/unsafe-log.md
Integrate every follow-up from the 2026-04-21 Phase-A code and security reviews into phase-b.md. No review item remains in an ad-hoc ledger; each is mapped to a specific B0..B6 milestone, and every decision that must close during Phase B is 🚩-flagged in its milestone and collected under the plan's "Open questions" section. New milestone **B0 — Phase A exit hygiene** prepends the original EL-drop pipeline and absorbs: - Security-review blockers #1 / #2 / #3 — UNSAFE-2026-0012 raw-pointer refactor, cross-table revocation policy, scheduler deadlock panic → idle task + typed error — as ADR-0021 / ADR-0022 / ADR-0023. - Code-review follow-ups: architecture docs for kernel-objects / IPC / scheduler; missing tests (ReceiverTableFull, slot-reuse with pending transfer cap, returns-Err replacements for should-panic tests); const {assert!(N>0)} type-level invariants on SchedQueue and CapabilityTable; debug_assert → typed-error hardening. - Non-blocking tracking items, placed in their natural milestone: generation wrap (B2 flag), fault containment scope (B5 flag → Phase E for full supervisor), Capability::Debug redaction (B5 before console_write syscall), write_bytes TX timeout (B6 flag), QEMU-smoke CI gate (B6 flag), cargo-vet init (B6 flag), TaskArena local → global StaticCell migration (bundled with T-006), boot.s early DAIFSet (B1 BSP checklist), TPIDR_EL0 save-set watch (TLS trigger, no decision yet). ADR numbers shifted: the pre-review 0021..0026 (EL-drop, MMU, AS, loader, syscall ABI, initial syscalls) become 0024..0029 so the B0 blockers can claim 0021..0023. Final numbers are assigned when the ADR is actually written per ADR-0013. current.md updated: active phase B, active milestone B0, first task to open is T-006 (raw-pointer scheduler API refactor). Also: docs/analysis/temp/ added to .gitignore as a local-only transient workbook directory (follow-up ledgers, drafts, scratch notes that do not belong in git history). Refs: ADR-0013 Code-Review: docs/analysis/reviews/code-reviews/2026-04-21-umbrix-to-phase-a.md Security-Review: docs/analysis/reviews/security-reviews/2026-04-21-umbrix-to-phase-a.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…note
Two narrow inline-review fixes.
bsp-qemu-virt/src/main.rs: the `console.write_bytes("umbrix: task B --
done; spinning")` call at the tail of task_b is structurally unreachable
in the v1 single-round demo. After Task B's final `yield_now`, Task A's
`ipc_recv_and_yield` picks up the reply from SendPending without
blocking and runs straight to its own spin loop — control never returns
to Task B. The expected QEMU trace (recorded in the A6 business review)
confirms this: the "done; spinning" line has never appeared in output.
Remove the unreachable write; keep `loop { spin_loop() }` because the
`fn() -> !` return type still requires a divergent tail. Expand the
task_b doc comment so future readers do not rediscover the reachability
story.
docs/architecture/security-model.md: the long dense "Revocation is
transitive … within a single capability table" bullet added by commit
`de66d68` packs the core invariant, the v1 scope limit, its
consequence, two workarounds, the cross-table open-question pointer,
and a citation into one paragraph. Reorganise into discrete
sub-bullets so the invariant leads and the supporting details follow.
Technical facts unchanged; referenced symbols (insert_root, ipc_recv,
cap_revoke) preserved.
No behavioural change; 109 host tests still green; QEMU build still
clean.
Refs: ADR-0013
Security-Review: docs/analysis/reviews/security-reviews/2026-04-21-umbrix-to-phase-a.md
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Expose the scheduler's IPC bridge (ipc_send_and_yield / ipc_recv_and_yield) in a form that never holds a live &mut to EP_ARENA / IPC_QUEUES / TABLE_* across cpu.context_switch — closing UNSAFE-2026-0012 (#1 Phase-B blocker from the 2026-04-21 security review) before any later milestone adds more shared state. TaskArena migration to a StaticCell is bundled in the same task to avoid a second round of BSP static-cell churn. 7 acceptance criteria; ADR-0021 writing is the first step inside the task. Status In Progress; current.md points at T-006; phase-b/README and phase-b.md updated. Refs: ADR-0013 Audit: UNSAFE-2026-0012 Security-Review: docs/analysis/reviews/security-reviews/2026-04-21-umbrix-to-phase-a.md Co-Authored-By: Claude Opus 4.7 <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 15, 2026
Verified all 5 review findings against current code state; all valid. Minimal-change fixes: 4 doc-only, 1 visibility narrowing. **(1) T-019 user-story §Review-history line 134 — unresolved `<TBD>` placeholders.** The 2026-05-14 implementation-arc row carried two literal `<TBD>` tokens (the BSP-wiring commit hash and the smoke- trace HEAD reference) that subsequent review-round commits never back-filled. Resolved both to `ae31bc8` (the round-3 BSP-wiring commit). The accompanying narrative (test count, gate names, smoke line content) was the state at commit `ae31bc8` and stays as the historical record per the user-story file's append-only convention. **(2) ADR-0029 §"Build pipeline (B4 / T-019)" + §"Host-side loader tests" — in-place edit of an Accepted ADR violated append-only policy.** Commit `196d3fb` (T-019 commit 4) silently rewrote the placeholder-byte literal from `[0x40, 0x00, 0x80, 0xd2, ...]` (the Accept-state form) to `[0x40, 0x05, 0x80, 0x52, ...]` (the documented-intent `mov w0, #42; ret` form). Per CLAUDE.md non-neg #5 + the ADR README §Format, ADRs are append-only — corrections land in §Revision notes, not via body rewrites. Fix: - Reverted both byte literals (lines 44 + 78) to their Accept- state form. - Added a new §Revision notes section before §References with a 2026-05-16 entry recording the byte-encoding correction, decoding both literals explicitly (Accept-state bytes → `MOVZ x0, #2; RET`; corrected bytes → `MOVZ w0, #42; RET`), citing the canonical kernel-source bytes in `bsp-qemu-virt/src/main.rs::USERSPACE_IMAGE`, and noting the discrepancy was non-load-bearing because the placeholder is not executed in B4. The kernel-source bytes are unchanged (BSP `USERSPACE_IMAGE` is the production reality and stays at the corrected form). **(3) `kernel/src/obj/task_loader.rs` test-only `unsafe` `FakeMmu::create_address_space` call sites — brief SAFETY comments missing (a)/(b)/(c) discipline.** Four sites use the pattern: - `fixture` helper (the canonical one) at line 908 - `missing_derive_surfaces_via_address_space_creation_failed` test body - `FailingMapMmu::create_address_space` `unsafe fn` body - `fixture_with_failing_mmu` helper Fix: expanded the `fixture` helper's SAFETY comment with the full (a) why-unsafe-required / (b) invariants-upheld / (c) why-safer-alternative-rejected discipline. Cross-referenced the three peer sites by name with brief "same argument as fixture" notes, including the `unsafe fn` body which separately documents the `unsafe_op_in_unsafe_fn` lint requirement. **(4) `kernel/src/mm/pmm.rs` test `unsafe` blocks for `core::ptr::write_bytes` and `*returned_ptr.add(off)` — brief SAFETY comments missing (a)/(b)/(c).** Both blocks (the pre-poison fill + the per-byte zero-fill assertion) were one-liners covering only (b). Expanded both to the full (a)/(b)/(c) discipline, naming `aligned_backing`'s host-allocation contract and the per-byte loop's in-bounds reasoning. **(5) `kernel/src/mm/mod.rs::phys_frame_kernel_ptr` — visibility widened public API unnecessarily.** Helper is only called from inside the kernel crate (`task_loader::load_image`'s byte-copy site; verified via `grep -rn phys_frame_kernel_ptr`). Narrowed `pub` → `pub(crate)` to keep the kernel-internal shim out of the crate's public API surface; no external (bsp / hal / test-hal) caller exists. Tests at HEAD: **260/260** (no test changes; all fixes are doc/visibility). All gates clean: cargo fmt --check, cargo host-test, cargo host-clippy -D warnings, cargo kernel-clippy -D warnings, cargo kernel-build. QEMU smoke byte-stable — full demo through `tyrne: all tasks complete`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8 tasks
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
Demonstrate an end-to-end two-task IPC round-trip on the QEMU virt BSP and mark Phase A as complete, updating docs and audits accordingly.
New Features:
Enhancements:
Documentation:
Summary by CodeRabbit
New Features
Documentation
Style