Development#8
Merged
Merged
Conversation
Opens the last unopened B0 task with implementable scope. Phase A exit performance review (2026-04-21-A6-baseline.md) was gated on a working monotonic time source via the ARM Generic Timer; T-009 implements the QemuVirtCpu Timer trait against CNTPCT_EL0 + CNTFRQ_EL0, deliberately scoped to measurement only. Out of scope: arm_deadline / cancel_deadline (need GIC + IVT wiring, their own task) and idle's WFI activation (depends on a wake source T-009 does not provide). ADR-0022 first rider therefore stays open until a follow-up IRQ-wiring task lands. Status: In Progress. Implementation lands in subsequent commits. Refs: ADR-0010, ADR-0022 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…0 (T-009) Lands the measurement half of the Timer trait per ADR-0010. Reads the ARM Generic Timer's frequency once at construction (CNTFRQ_EL0) and the system counter on every now_ns() call (CNTPCT_EL0). Resolution is cached so the hot path is one MRS + one multiply. Implementation choices, settled in T-009 §Approach: - QemuVirtCpu gains two u64 fields (frequency_hz, resolution_ns) populated by new(). The const-fn marker is dropped because reading a system register cannot run in const context; verified that kernel_entry is the only construction site. - new() asserts CNTFRQ_EL0 > 0 to fail loudly if firmware did not initialise the generic timer (would cause divide-by-zero in the resolution computation otherwise). - now_ns() uses count.wrapping_mul(resolution_ns). Overflow margin is ~584 years at the lowest realistic frequency (1 MHz) and 18+ millennia at QEMU virt's 62.5 MHz. ADR-0010 permits sub-resolution precision loss, so the integer multiply is correct. - arm_deadline / cancel_deadline are unimplemented!() with explicit messages naming the missing IRQ-wiring task. Silent no-ops would break the trait contract; the loud panic makes the gap unambiguous. Today there is no caller, so the panics are unreachable. Audit: UNSAFE-2026-0015 written for the two new MRS reads. Same shape as UNSAFE-2026-0007 (read-only system registers, defined behaviour at EL1, no state mutation). Append-only — no edits to existing entries. Verification: - 77 kernel + 34 test-hal = 111 host tests green. - cargo +nightly miri test --workspace --exclude tyrne-bsp-qemu-virt remains clean. - cargo fmt / host-clippy / kernel-clippy / kernel-build all clean. - BSP smoke instrumentation (next commit) proves the Timer impl works end-to-end on QEMU virt. Refs: ADR-0010 Audit: UNSAFE-2026-0015 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wires the live Timer impl into the demo so the QEMU smoke output now includes a numerical performance signal: tyrne: hello from kernel_main tyrne: timer ready (62500000 Hz, resolution 16 ns) (new) tyrne: starting cooperative scheduler tyrne: task B — waiting for IPC tyrne: task A -- sending IPC tyrne: task B — received IPC (label=0xaaaa); replying tyrne: task A — received reply (label=0xbbbb); done tyrne: all tasks complete tyrne: boot-to-end elapsed = 10240992 ns (new) Two additions: 1. After CPU + console init, kernel_entry prints a one-line timer diagnostic banner showing CNTFRQ and computed resolution. This makes "the timer is reachable" an observable boot signal, not only an internal field on QemuVirtCpu. 2. A new `static BOOT_NS: StaticCell<u64>` is written by kernel_entry immediately after the banner, then read by task_a's "all tasks complete" path to print boot-to-end elapsed nanoseconds. Uses the existing StaticCell pattern (UNSAFE-2026-0010) — no new audit surface introduced. The numerical value is QEMU-virtual time, not realistic; it exists to prove the time source works end-to-end. The first hypothesis-driven performance review (a future cycle) will produce the real numbers it cares about by adding more granular timestamps inside the IPC bridge. The original A6 five-line trace is preserved byte-for-byte; the two new lines are bracketed around it. Existing tests / miri / clippy unchanged. Refs: ADR-0010 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lands the documentation half of T-009 alongside the code that landed in beb0963 + 55f2d10: - ADR-0022 first rider gains a *Sub-rider* that explicitly distinguishes the **time source** (T-009 — read CNTPCT_EL0) from **IRQ delivery** (a future, not-yet-opened wiring task — program CNTV_CTL_EL0 + configure GIC + install IVT). An earlier draft conflated the two under "T-009". WFI activation in idle is gated on the IRQ task, not on T-009. The chosen option is unchanged; the precondition wording is now correctly attributed. - Glossary +3 entries: CNTPCT_EL0, CNTFRQ_EL0, "Generic Timer (ARM)". These now appear in audit-log entries / SAFETY comments / inline source comments and a future contributor benefits from one authoritative definition. - README status line refreshed: "Pre-alpha — architecture phase. No bootable kernel yet." was outdated since A6 closed and is now a one-paragraph summary of what works (Phase A complete, Phase B underway, working monotonic time source, capability-gated IPC) with a pointer to current.md for the live state. - README identity line "the name comes from Latin umbra" replaced with Tyrne's clean-slate-no-motif framing per the project memory's rename-rider (project_tyrne_identity.md). - T-009 task file: status → In Review, all acceptance + DoD boxes checked, review-history row appended documenting the implementation arc (beb0963 + 55f2d10) plus this doc sweep. - phase-b.md + phase-b/README.md task index: T-009 row status flipped. - current.md: Active task → none (T-009 moved off); ADR-0010 added to Active decisions; T-009 listed alongside T-006/T-007 in In Review. Verification: 111 host tests green, miri 111/111 clean, fmt / host-clippy / kernel-clippy / kernel-build all clean, QEMU smoke reproduces the eight-line trace (A6 five lines + timer banner + elapsed measurement). Refs: ADR-0010, ADR-0022 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
arithmetic + audit policy Independent two-agent second-read review of T-009 (commits 0b0d993..1df3641) produced three Yüksek findings, all addressed here. The original measurement half remains correct; the fixes move the implementation off behaviours that would have silently broken on the first non-QEMU-virt BSP. 1. Register family — switched from CNTPCT_EL0 to CNTVCT_EL0. ADR-0010's References list and ADR-0022's first-rider sub-rider both name the virtual family (CNTVCT_EL0, CNTV_CVAL_EL0, CNTV_TVAL_EL0, CNTV_CTL_EL0). The original implementation read the physical CNTPCT_EL0, which on QEMU virt with CNTVOFF_EL2 = 0 coincides with CNTVCT but would silently mismatch the deferred deadline-arming side once any boot path leaves a non-zero offset. UNSAFE-2026-0015 gains an Amendment recording the swap; SAFETY comments and the module doc-comment now name CNTVCT explicitly. 2. Arithmetic — count * resolution_ns is not nanoseconds on non-divisor frequencies, and wrapping_mul breaks ADR-0010 monotonicity. The original count.wrapping_mul(resolution_ns) had two problems: (a) at non-divisor frequencies (e.g. 19.2 MHz, 52.0833... ns true period; the truncated 52 ns gives 0.16 % drift ≈ 138 s/day); (b) wrap-on-overflow silently breaks ADR-0010's monotonic-time guarantee at the wrap edge. Conversion extracted to tyrne_hal::timer::ticks_to_ns: pub const fn ticks_to_ns(count: u64, frequency_hz: u64) -> u64 { let intermediate = (count as u128) * (NANOS_PER_SECOND as u128); let ns = intermediate / (frequency_hz as u128); if ns > u64::MAX as u128 { u64::MAX } else { ns as u64 } } 128-bit intermediate is overflow-free for any tick count up to u64::MAX at any frequency. Saturating cast back to u64 preserves monotonicity at the rare ~584-year extreme. Pure functions live in the HAL crate so host unit tests can exercise them without inline asm. resolution_ns_for_freq also moved to the HAL — uses (1e9 + freq/2) / freq for round-to-nearest (ADR-0010's "rounds to the nearest multiple" wording, not floor). 13 new host tests in hal/src/timer.rs cover: zero count; QEMU virt divisor case; Pi-3-class non-divisor case; 1 GHz exact case; u64 saturation at the boundary and at u64::MAX × 1 Hz; const-fn evaluation in const context; round-to-nearest for non-divisor frequencies (52 ns for 19.2 MHz, 19 ns for 54 MHz BCM2711-class). The previously claimed "~18 millennia at QEMU virt" overflow margin was arithmetically wrong; corrected inline (it is ~584 years at any frequency, since count * resolution_ns ≈ elapsed_ns ≤ u64::MAX = ~584 years). 3. Audit-policy compliance — UNSAFE-2026-0006 silent body rewrite repaired. T-009 (commit beb0963) added two u64 fields to QemuVirtCpu and rewrote the in-source SAFETY comment for Send/Sync to match. The audit-log entry UNSAFE-2026-0006's body still claimed "zero-size type with no fields" — silently divergent. Per unsafe-policy §3, scope expansions need an Amendment block, not a body rewrite. Appended Amendment dated 2026-04-23 recording the post-T-009 invariants; original body intact. Other corrections folded in: - SAFETY comments on the two MRS reads now cite ADR-0012 explicitly (Tyrne enters at EL1 in non-VHE mode; CNTHCTL_EL2.EL1{V,P}CTEN gating applies only in VHE which we do not enter) instead of asserting "available at EL1 unconditionally". - Glossary's Pi 4 frequency claim softened: 19.2 MHz is the Pi 3 generic-timer rate; BCM2711 mainline runs at 54 MHz. BSPs read CNTFRQ_EL0 regardless, so this is purely a doc-accuracy fix. - Glossary +1 entry: CNTVCT_EL0. Existing CNTPCT_EL0 entry now notes Tyrne does not read it directly. Generic-Timer entry mentions both views explicitly. Verification after fixes: - 77 kernel + 13 hal + 34 test-hal = 124 host tests green. - cargo +nightly miri test --workspace --exclude tyrne-bsp-qemu-virt remains clean across all 124 tests. - cargo fmt / host-clippy / kernel-clippy / kernel-build all clean. - QEMU smoke produces the same eight-line trace, with the boot-to-end elapsed value computed via the new u128 arithmetic path. Out of scope (per second-read review's own Düşük section): - arm_deadline panic message naming "T-009" / "future B0 follow-up task" — defer to the IRQ-wiring task's scope-in commit when that task is opened. - Past commit subjects over 72 chars (beb0963) and bundled doc-sweep commit (1df3641): note for future commit-style discipline; not rewriting history. Refs: ADR-0010, ADR-0012, ADR-0022, unsafe-policy §3 Audit: UNSAFE-2026-0006 (Amendment), UNSAFE-2026-0015 (Amendment) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both Amendment blocks landed in 39fb66c with "commit TBD" placeholders because the SHA is unknown until commit time (chicken-and-egg, same as the d25a185 / 330e998 follow-up that established the pattern). This follow-up replaces both placeholders with `39fb66c`. Per unsafe-policy §3, this edit is permitted because the SHA slot is part of each Amendment block's required header — equivalent to filling in a dated form field — and the Amendment's content (invariants / register-family change / EL precondition / saturating arithmetic) is not touched. Refs: unsafe-policy §3 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…b66c Self-audit of T-009's second-read fix-up commit (39fb66c) found that Review 1's Yüksek #1 was half-implemented. The recommendation read: > Either (a) explicitly detect/handle EL2 ..., or (b) assert early > (and document) that boot environment guarantees EL1 before these > reads (and add a runtime check that panics with a clear error > if not). Commit 39fb66c took option (b) but landed only the documentation half (ADR-0012 cite, EL precondition wording, removal of "unconditional" claim) — the parenthetical "and add a runtime check" was silently skipped. This commit closes the runtime-check half. Implementation: - QemuVirtCpu::new gains an MRS CurrentEL read at the head of the function, before any generic-timer access. The 2-bit Exception-Level field is shifted out and asserted == 1; a mismatch panics with the observed EL ("must run at EL1 per ADR-0012; observed EL{n} instead"). - The cost is one MRS + one compare per CPU construction. Negligible. The assertion runs once per kernel boot. - One new audit entry: UNSAFE-2026-0016 (boot-time CurrentEL self-check), shaped like UNSAFE-2026-0007 (read-only system register at EL≥1, no state mutation, options(nostack, nomem)). The doc-comment on QemuVirtCpu::new was reorganised so the # Panics section names both invariant violations (EL≠1 and CNTFRQ_EL0==0) with their respective audit tags. The CNTFRQ MRS's SAFETY comment now notes the EL-precondition reasoning is "not just documentation but a checked invariant" — closing the gap between paper claim and runtime behaviour. Lesson — recorded in the T-009 task file's review-history row as the adjustment to carry forward — review recommendations with AND/OR structure should be treated as full asks: documentation + runtime check is the (b) option's complete shape, not "either of the two". Verification: 124 host tests green, miri 124/124 clean, all clippy/ fmt/build/YAML gates clean. QEMU smoke shows the new EL=1 assertion passes (we are at EL1, as ADR-0012 specifies) and the eight-line boot trace continues unchanged. Refs: ADR-0012, ADR-0010, T-009 second-read Review 1 Yüksek #1 Audit: UNSAFE-2026-0016 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer's GuideImplements the Timer HAL for QEMU virt by wiring the ARM Generic Timer into QemuVirtCpu, adds shared tick→nanosecond helpers in the HAL crate, instruments the BSP demo to print boot-time and elapsed timing, and updates unsafe audits and roadmap/docs to reflect the new time-source and task status. Sequence diagram for kernel_entry and task_a using Timer::now_nssequenceDiagram
actor Host
participant Boot as kernel_entry
participant CPUCell as CPU_StaticCell
participant BootNsCell as BOOT_NS_StaticCell
participant CpuObj as QemuVirtCpu
participant TaskA as task_a
participant Console
Host->>Boot: start kernel_entry()
Boot->>CPUCell: init QemuVirtCpu::new()
CPUCell->>CpuObj: construct (reads CurrentEL, CNTFRQ_EL0)
Boot->>Console: write "hello from kernel_main"
Boot->>CpuObj: frequency_hz()
Boot->>CpuObj: resolution_ns()
Boot->>Console: write timer banner
Boot->>CpuObj: now_ns()
CpuObj->>CpuObj: read CNTVCT_EL0
CpuObj->>CpuObj: ticks_to_ns(count, frequency_hz)
Boot->>BootNsCell: store boot_ns
Boot->>TaskA: schedule task_a()
TaskA->>Console: write task progress lines
TaskA->>CPUCell: load QemuVirtCpu via StaticCell (unsafe)
TaskA->>BootNsCell: load boot_ns (unsafe)
TaskA->>CpuObj: now_ns()
CpuObj->>CpuObj: read CNTVCT_EL0
CpuObj->>CpuObj: ticks_to_ns(count, frequency_hz)
TaskA->>TaskA: elapsed_ns = now_ns.saturating_sub(boot_ns)
TaskA->>Console: write "boot-to-end elapsed = elapsed_ns ns"
TaskA->>TaskA: spin_loop() forever
Class diagram for QemuVirtCpu Timer implementation and HAL timer helpersclassDiagram
direction LR
class Timer {
<<trait>>
+now_ns() u64
+arm_deadline(deadline_ns u64) void
+cancel_deadline() void
+resolution_ns() u64
}
class Cpu {
<<trait>>
}
class ContextSwitch {
<<trait>>
}
class QemuVirtCpu {
+frequency_hz u64
+resolution_ns u64
+new() QemuVirtCpu
+frequency_hz() u64
+now_ns() u64
+arm_deadline(deadline_ns u64) void
+cancel_deadline() void
+resolution_ns() u64
}
class TimerModule {
<<module tyrne_hal::timer>>
+NANOS_PER_SECOND u64
+ticks_to_ns(count u64, frequency_hz u64) u64
+resolution_ns_for_freq(frequency_hz u64) u64
}
class StaticCell_T_QemuVirtCpu_ {
<<type>>
}
class StaticCell_T_u64_ {
<<type>>
}
Timer <|.. QemuVirtCpu
Cpu <|.. QemuVirtCpu
ContextSwitch <|.. QemuVirtCpu
TimerModule ..> Timer : helpers_for
QemuVirtCpu ..> TimerModule : uses
StaticCell_T_QemuVirtCpu_ "1" o-- "1" QemuVirtCpu : CPU
StaticCell_T_u64_ "1" o-- "1" QemuVirtCpu : BOOT_NS_timestamp_user
Flow diagram for ticks_to_ns and resolution_ns_for_freq helpersflowchart TD
subgraph ticks_to_ns_flow
A[count, frequency_hz inputs] --> B[Compute intermediate = count as u128 * NANOS_PER_SECOND as u128]
B --> C[Compute ns = intermediate / frequency_hz as u128]
C --> D{ns > u64::MAX as u128?}
D -- Yes --> E[Return u64::MAX]
D -- No --> F[Return ns as u64]
end
subgraph resolution_ns_for_freq_flow
G[frequency_hz input] --> H[Compute numerator = NANOS_PER_SECOND + frequency_hz / 2]
H --> I[Compute resolution = numerator / frequency_hz]
I --> J[Return resolution]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughAdds ARM Generic Timer support to the QEMU Virt BSP (Timer trait implementation and HAL timer helpers), records boot-time EL1 validation and boot-to-end timing instrumentation, and codifies ADR governance updates requiring a dependency-chain subsection and a mandatory careful re-read before accepting ADRs. Includes task specs (T-009, T-012, T-013), ADRs (0024, 0025), audits, and docs updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Kernel
participant BSP as QemuVirtCpu
participant HAL
participant ARM as GenericTimerRegs
rect rgba(200,200,255,0.5)
Kernel->>BSP: call QemuVirtCpu::new()
BSP->>ARM: MRS CurrentEL
ARM-->>BSP: CurrentEL
BSP->>ARM: MRS CNTFRQ_EL0
ARM-->>BSP: frequency_hz
BSP->>BSP: cache frequency_hz, resolution_ns
end
rect rgba(200,255,200,0.5)
Kernel->>BSP: kernel_entry reads BOOT_NS := bsp.now_ns()
BSP->>ARM: MRS CNTVCT_EL0
ARM-->>BSP: ticks
BSP->>HAL: ticks_to_ns(ticks, frequency_hz)
HAL-->>BSP: ns
BSP-->>Kernel: BOOT_NS value
end
rect rgba(255,200,200,0.5)
Kernel->>BSP: later task reads bsp.now_ns()
BSP->>ARM: MRS CNTVCT_EL0
ARM-->>BSP: ticks
BSP->>HAL: ticks_to_ns(...)
HAL-->>BSP: ns
BSP-->>Kernel: now_ns (for elapsed calc)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 docstrings
🧪 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 |
![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 1 issue, and left some high level feedback:
- The helper docs for
ticks_to_ns/resolution_ns_for_freqsay they panic onfrequency_hz == 0, but the implementations just perform a division that will UB/panic implicitly; consider adding an explicitassert!(frequency_hz != 0)(or similar) in these functions so the behavior matches the contract and fails loudly in all call sites, not just BSPs that pre-validate the frequency. - The UNSAFE-2026-0015 entry still describes
Timer::now_nsas usingCNTPCT_EL0in the main bullet list while the implementation now readsCNTVCT_EL0and this is only captured in the amendment; it would be clearer for future readers if the primary description was updated to reflect the current register choice instead of relying on the amendment to correct it.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The helper docs for `ticks_to_ns`/`resolution_ns_for_freq` say they panic on `frequency_hz == 0`, but the implementations just perform a division that will UB/panic implicitly; consider adding an explicit `assert!(frequency_hz != 0)` (or similar) in these functions so the behavior matches the contract and fails loudly in all call sites, not just BSPs that pre-validate the frequency.
- The UNSAFE-2026-0015 entry still describes `Timer::now_ns` as using `CNTPCT_EL0` in the main bullet list while the implementation now reads `CNTVCT_EL0` and this is only captured in the amendment; it would be clearer for future readers if the primary description was updated to reflect the current register choice instead of relying on the amendment to correct it.
## Individual Comments
### Comment 1
<location path="hal/src/timer.rs" line_range="80-89" />
<code_context>
/// snapshot and restoring it would silently override the first instance's
/// state. In v1 (single-core), construct exactly once in `kernel_entry`.
+ ///
+ /// # Panics
+ ///
+ /// Panics in two boot-time-invariant cases. Both indicate a
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Documented `frequency_hz == 0` behaviour relies on implicit division-by-zero panics, which can be brittle in `const fn` and at odds with the explicit panic docs.
The `ticks_to_ns` docs promise a deliberate panic when `frequency_hz == 0`, but the implementation currently relies on `u128` division by zero. That works today at runtime, but in const contexts it becomes a compile-time error and doesn’t reflect an intentional, documented panic.
Given this is a `const fn`, consider either:
- Adding an explicit guard (e.g. `assert!(frequency_hz != 0);` or `debug_assert!` plus a `// SAFETY:` comment), or
- Relaxing the docs to treat zero as out-of-contract / UB and requiring callers to validate the frequency.
The same mismatch between docs and implementation exists in `resolution_ns_for_freq` and should be addressed consistently.
</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
+80
to
+89
| /// # Panics | ||
| /// | ||
| /// Panics if `frequency_hz == 0`. BSP impls should validate the frequency | ||
| /// at construction (e.g. by reading `CNTFRQ_EL0` and asserting non-zero) | ||
| /// so the panic is unreachable in production. | ||
| #[allow( | ||
| clippy::cast_possible_truncation, | ||
| reason = "saturating cast handled explicitly by the if/else guard above" | ||
| )] | ||
| #[must_use] |
There was a problem hiding this comment.
suggestion (bug_risk): Documented frequency_hz == 0 behaviour relies on implicit division-by-zero panics, which can be brittle in const fn and at odds with the explicit panic docs.
The ticks_to_ns docs promise a deliberate panic when frequency_hz == 0, but the implementation currently relies on u128 division by zero. That works today at runtime, but in const contexts it becomes a compile-time error and doesn’t reflect an intentional, documented panic.
Given this is a const fn, consider either:
- Adding an explicit guard (e.g.
assert!(frequency_hz != 0);ordebug_assert!plus a// SAFETY:comment), or - Relaxing the docs to treat zero as out-of-contract / UB and requiring callers to validate the frequency.
The same mismatch between docs and implementation exists in resolution_ns_for_freq and should be addressed consistently.
![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 the Timer trait for the QEMU virt aarch64 target, enabling monotonic time measurement using the ARM Generic Timer's virtual counter. The changes include adding frequency and resolution caching to the CPU structure, implementing the measurement half of the Timer trait, and providing host-testable arithmetic helpers in the HAL. Documentation, including the project roadmap, glossary, and unsafe code audits, has been updated to reflect these additions. The review feedback identifies several instances of stale documentation and comments that still reference the physical counter or outdated conversion logic, and suggests a robustness improvement for the resolution calculation at high frequencies.
Comment on lines
+50
to
+51
| /// - `resolution_ns` — derived as `1_000_000_000 / frequency_hz`. Cached so | ||
| /// `now_ns` is a single multiply rather than a multiply + divide. |
There was a problem hiding this comment.
The comment stating that resolution_ns is cached to avoid a division in now_ns is stale. The current implementation of now_ns (line 426) calls ticks_to_ns, which performs a 128-bit division on every call. The cached value is now only used for the resolution_ns() trait method.
Suggested change
| /// - `resolution_ns` — derived as `1_000_000_000 / frequency_hz`. Cached so | |
| /// `now_ns` is a single multiply rather than a multiply + divide. | |
| /// - `resolution_ns` — derived as `1_000_000_000 / frequency_hz`. Cached for | |
| /// the [`Timer::resolution_ns`] trait method. |
Comment on lines
+55
to
+56
| /// Pre-computed `1_000_000_000 / frequency_hz`. Cached so [`Timer::now_ns`] | ||
| /// avoids a 64-bit divide on every call. |
There was a problem hiding this comment.
This comment is also stale. As noted in the struct layout documentation, now_ns currently performs a division via ticks_to_ns rather than using the cached resolution for a multiply-only path.
Suggested change
| /// Pre-computed `1_000_000_000 / frequency_hz`. Cached so [`Timer::now_ns`] | |
| /// avoids a 64-bit divide on every call. | |
| /// Pre-computed `1_000_000_000 / frequency_hz`. Cached for the | |
| /// [`Timer::resolution_ns`] trait method. |
| @@ -0,0 +1,110 @@ | |||
| # T-009 — Timer init + `CNTPCT_EL0` measurement | |||
There was a problem hiding this comment.
The task title and filename refer to CNTPCT_EL0 (the physical counter), but the implementation in cpu.rs was switched to CNTVCT_EL0 (the virtual counter) to ensure alignment with the virtual deadline-arming registers. The documentation should be updated throughout (including the user story and acceptance criteria) to reflect this change consistently.
Suggested change
| # T-009 — Timer init + `CNTPCT_EL0` measurement | |
| # T-009 — Timer init + `CNTVCT_EL0` measurement |
| ## Acceptance criteria | ||
|
|
||
| - [x] **`QemuVirtCpu` implements `tyrne_hal::Timer`** with four methods. Commit `beb0963`. | ||
| - [x] `now_ns(&self) -> u64` — reads `CNTPCT_EL0`, multiplies by the cached resolution, returns nanoseconds since boot. Monotonic by hardware contract. |
There was a problem hiding this comment.
This acceptance criterion still references CNTPCT_EL0 and the rejected "multiply by cached resolution" approach. It should be updated to match the final implementation using CNTVCT_EL0 and ticks_to_ns.
Suggested change
| - [x] `now_ns(&self) -> u64` — reads `CNTPCT_EL0`, multiplies by the cached resolution, returns nanoseconds since boot. Monotonic by hardware contract. | |
| - [x] `now_ns(&self) -> u64` — reads `CNTVCT_EL0`, converts ticks to nanoseconds, and returns elapsed time since boot. Monotonic by hardware contract. |
Comment on lines
+113
to
+118
| pub const fn resolution_ns_for_freq(frequency_hz: u64) -> u64 { | ||
| // (1e9 + freq/2) / freq is the standard nearest-integer division | ||
| // pattern for positive integers. Overflow analysis: u64::MAX / 2 | ||
| // ≈ 9.2e18, so adding 1e9 stays well within u64 for any frequency | ||
| // a real timer would report. | ||
| (NANOS_PER_SECOND + frequency_hz / 2) / frequency_hz |
There was a problem hiding this comment.
The resolution_ns_for_freq function returns 0 for frequencies greater than 2 GHz (where the period is less than 0.5 ns). A resolution of 0 ns is problematic for rounding logic and could lead to division-by-zero errors in callers. Since the unit is nanoseconds, the resolution should be at least 1 ns for any running timer.
pub const fn resolution_ns_for_freq(frequency_hz: u64) -> u64 {
// (1e9 + freq/2) / freq is the standard nearest-integer division
// pattern for positive integers. Overflow analysis: u64::MAX / 2
// ≈ 9.2e18, so adding 1e9 stays well within u64 for any frequency
// a real timer would report.
let res = (NANOS_PER_SECOND + frequency_hz / 2) / frequency_hz;
if res == 0 {
1
} else {
res
}
}zero-freq panic, audit-log readability Three findings from the in-progress review of T-009's second-read fix-up commit (db3a4c7), addressed here. 1. CI fast-lane was failing: cargo install --locked cargo-llvm-cov errors with "binary already exists in destination" when the cache restore step places the binary first. The "(cached)" comment I wrote claimed cargo install was idempotent — it isn't on older cargo. Guarded the install with a presence check so the cache hit is a true no-op rather than a 1-2 min rebuild OR a 101 exit. 2. Helper docs (ticks_to_ns, resolution_ns_for_freq) promised a named panic when frequency_hz == 0, but the implementations relied on implicit u128 division-by-zero. That works at runtime but produces a less informative message and becomes a compile- time error if a const caller passes zero. Added explicit assert!(frequency_hz != 0, "...") at the head of both helpers with a named message; added two #[should_panic] tests to lock the behaviour. The contract now matches the implementation in every call path, not just BSPs that pre-validate. 3. UNSAFE-2026-0015 audit entry's main body still describes CNTPCT_EL0 reads and the "EL1 unconditional" claim; the 2026-04-23 Amendment supersedes both, but a casual reader scanning the entry top-to-bottom would absorb the wrong description before reaching the Amendment. Per unsafe-policy §3 the original body stays intact (append-only); added a "Note for casual readers" line to the Status field — Status is policy-permitted to evolve — that points readers at the Amendment for the current state. Verification: - 13 + 2 = 15 hal tests, all green (the two new should_panic tests cover the explicit zero-frequency contract). - 77 kernel + 15 hal + 34 test-hal = 126 host tests green. - cargo +nightly miri test --workspace --exclude tyrne-bsp-qemu-virt remains clean. - cargo fmt / host-clippy / kernel-clippy / kernel-build clean. - YAML lint clean for .github/workflows/ci.yml. Refs: ADR-0010, unsafe-policy §3, T-009 review-2 outstanding items Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
hygiene (ADR-0013 amendment + template + skill) The Phase A → B0 implementation arc produced four ADRs that needed post-Accept riders within their first week of life (ADR-0021 mid- proposal revision + post-Accept rider for start(); ADR-0022 first rider for WFI hang + second rider for debug_assert + sub-rider for T-009 vs IRQ-wiring). Every one of those riders, in retrospect, was a gap a single calendar day of cool-down would have surfaced before Accept. Today's pattern-analysis question — "is this a planning problem or a normal situation?" — produced a concrete answer: the *outcome* is healthy (vertical slicing through a feature is good practice), but the *path* exposed three implicit rules that ADR-0013 had never made explicit. Codifying them now, before the next ADR (likely on MMU enablement in B2 or syscall ABI in B5) reproduces the failure. Three rules added to ADR-0013 §"Integration with ADRs": 1. **ADR cool-down: no same-day Accept.** Proposed → Accepted is a transition that takes at least one calendar day. Same-day Accept is forbidden. Rationale: A → B0 produced four same-day Accepts and four post-Accept riders; the correlation is not coincidence. 2. **Forward-reference contract: every "future task" claim is grounded.** ADR text — including riders — that says "task X will do Y" must point at a real T-NNN file, even if Draft. "Future, not-yet-opened task" wording is forbidden. Rationale: ungrounded forward-references drift into purgatory; the ADR-0022 first rider's mis-scoped T-009 claim is the canonical instance the codification prevents. 3. **Riders are not failures; their frequency is a signal.** Riders are valid records of learning. The eliminate-all-riders posture is overcorrection. The metric to watch is rider rate per ADR over time; a sudden climb means audit the ADR-writing process, not the riders. Riders themselves are append-only, same logic as the unsafe-log per `unsafe-policy.md §3`. Concrete implementation: - ADR-0013: §"Integration with ADRs" gains three subsections. - docs/decisions/template.md: Decision outcome gains a "Dependency chain" subsection — every chosen-option ADR now lists every task / piece of infrastructure that must already exist for the decision to be fully in effect, in implementation order, with every step pointing at an existing T-NNN or one opened in the same commit. - write-adr skill: procedure step 5 expanded with the dependency- chain requirement; step 10 added for cool-down; acceptance criteria gain "Status lands at Proposed, not Accepted" and "Decision outcome includes Dependency chain"; anti-patterns gain "Same-day Proposed → Accepted" and "Forward-reference handwaving." These changes do not retroactively edit prior ADRs (ADR-0013 is itself updated in place because that's the meta-ADR and changes to the planning process are exactly what its body covers). Rider discipline applies forward; existing same-day Accepts on ADR-0021 and ADR-0022 stand as historical record. Refs: ADR-0013, T-009 second-read review, T-007 mini-retro Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Grounds the previously-handwaved forward-reference in ADR-0022's first-rider sub-rider per the new ADR-0013 §"Forward-reference contract" (codified in commit 56fd9eb earlier today). Until now, that sub-rider said "future, not-yet-opened IRQ-wiring task" — the exact language ADR-0013's new rule forbids. T-012 scope (Draft — broad bundle, may split into T-012a/b during implementation): 1. GIC v2 driver in bsp-qemu-virt (impl IrqController per ADR-0011). 2. VBAR_EL1 install + minimum 16-entry vector table. 3. Generic-timer IRQ wiring → real arm_deadline / cancel_deadline (closes the unimplemented!() panics from T-009). 4. idle_entry switches from spin_loop to wait_for_interrupt (closes ADR-0022 first rider's Sub-rider in full). 5. Audit-log surface for new MMIO + asm: UNSAFE-2026-0017+. Phase plan adjustment: - Milestone B1 retitled "Drop to EL1 in boot, install exception infrastructure". Original B1 scope (EL2→EL1 transition) stays; IRQ delivery joins it because both are bootstrap-time exception- layer infrastructure that share the same boot.s + VBAR_EL1 surface. A scope-creep note allows T-012 to split into T-012a/T-012b if implementation reveals it should. - B1 sub-breakdown gains item 5 referencing T-012. - B1 acceptance criteria gains the closure condition for ADR-0022 first-rider Sub-rider. ADR-0022 first-rider Sub-rider updated to point at T-012 by file path (no longer "future, not-yet-opened"). The pre-T-012 Sub-rider text is intact except for the forward-reference resolution — same shape as the unsafe-log Amendment pattern, just for ADR riders. Status remains `Draft`. Will move to `In Progress` only after: - T-006 / T-007 / T-009 reach `Done` - ADR-0024 (B1 sub-item 1, EL drop policy) is `Accepted` Refs: ADR-0013 (§Forward-reference contract), ADR-0022, T-009 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
resolution floor Five findings from the in-progress review of T-009. Three are stale- documentation issues that the second-read fix-up commits (39fb66c + db3a4c7 + 01010f4) left behind; two are real correctness gaps in the hal helpers. 1. cpu.rs struct doc comment was stale. The QemuVirtCpu struct's #[doc] said "resolution_ns is cached so now_ns is a single multiply rather than a multiply + divide." That described the original count*resolution_ns implementation, which the T-009 second-read review replaced with a 128-bit ticks_to_ns path. The cache is now used only by the resolution_ns() trait method; now_ns()'s hot path consults frequency_hz, not resolution_ns. Updated both the struct-level doc and the resolution_ns field's inline comment to match shipped behaviour. Reasoning paragraph preserves the historical context (why the multiply form was rejected in second-read review). 2. T-009 task file referred to CNTPCT_EL0 in title, filename, user story, and acceptance criteria. The implementation switched from CNTPCT_EL0 (physical) to CNTVCT_EL0 (virtual) in commit 39fb66c per the register-family alignment the T-009 second-read review caught. The task file was updated for the acceptance-criteria text but the title, filename, and user-story prose still named CNTPCT. Renamed the file via `git mv` (history-preserving) from T-009-timer-init-cntpct.md to T-009-timer-init-cntvct.md; updated title; rewrote user story to explain the virtual-counter choice; updated acceptance criterion text. All references to the old filename in phase-b.md / phase-b/ README.md / current.md / T-012 also updated. Display titles in those index rows similarly fixed (the rows said "CNTPCT_EL0 measurement"; now "CNTVCT_EL0 measurement"). 3. phase-b.md §B0 sub-breakdown item 5 + acceptance-criteria line said "CNTPCT_EL0". Phase-plan items aren't append-only (unlike audit-log entries), but they should reflect current state. Updated to CNTVCT_EL0 with a parenthetical noting "original phase-plan wording said CNTPCT; T-009 second-read review surfaced the family mismatch and switched." Preserves the historical record without leaving misleading text. 4. resolution_ns_for_freq returned 0 for frequencies > 2 GHz. The round-to-nearest formula `(1e9 + freq/2) / freq` truncates to zero when freq exceeds 2 * NANOS_PER_SECOND. A resolution of 0 ns is meaningless and risks divide-by-zero in any caller that uses the value as a divisor. Added a floor: clamp the result to ≥ 1 ns, which is the smallest representable resolution at nanosecond unit (sub-nanosecond precision is silently lost per the trait contract's "finer precision is silently lost" wording). Three new tests: - 3 GHz, 10 GHz, u64::MAX Hz all clamp to 1 ns. - Boundary case (exactly 2 GHz) returns 1 ns naturally with no clamp needed (sanity check that the clamp doesn't fire when not required). - (Existing tests for QEMU virt 62.5 MHz, 19.2 MHz, 54 MHz, 1 GHz unchanged — round-to-nearest behaviour at sub-2-GHz frequencies is identical.) No production caller currently triggers the path (QEMU virt is 62.5 MHz, Pi 4 generic timer is sub-100 MHz), but the future- proofing is cheap and the contract is now consistent. Verification: - 77 kernel + 17 hal + 34 test-hal = 128 host tests green. (Previously 126; +2 from the resolution-floor tests.) - cargo +nightly miri test --workspace --exclude tyrne-bsp-qemu-virt remains clean across all 128 tests. - cargo fmt / host-clippy / kernel-clippy / kernel-build clean. - QEMU smoke unchanged (eight-line trace). Refs: ADR-0010, T-009 review-3 outstanding items Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mini-retro on T-009's three-round implementation arc and the meta-arc it surfaced — the codification of three new rules in ADR-0013 (cool- down, forward-reference contract, riders-are-not-failures), the new template Dependency-chain section, the write-adr skill update, and T-012's opening to ground a previously-handwaved forward-reference. Key learnings: - Rules work better than re-discovered lessons. T-006 retro had named the dependency-analysis weakness as "trace the call graph", but the lesson stayed at prose level and was rediscovered four times across A → B0. Only on the fifth occurrence did we sit down and codify it as ADR-0013 rules. The codified rules are not yet validated; ADR-0024 (next per the user's C→A→B sequence) is the validation event. - Reviews leave their own staleness behind. Each fix commit corrected one type of staleness while leaving another type for the next round — a "rider chain within the rider response itself". Build this into DoD rather than be surprised. - User-driven thoroughness beats agent-driven thoroughness when the agent has been at it for a while. The maintainer's question "are all accepted findings actually fixed?" caught the silently-skipped R1.1 runtime EL check. Self-reports drift; verification doesn't. - File renames have a long tail. Stale-link CI check would catch the rename-ripple problem mechanically; routed to follow-up. Adjustments queued: - Validate new ADR-0013 rules with ADR-0024 (the next ADR; cool-down + dependency-chain section first real test). - Open T-008 (architecture docs, B0) — the only B0 task still without a file; required by the new forward-reference contract. - Add stale-link CI check (small script, high catch rate). - Codify "post-fix sweep" in DoD. Refs: ADR-0013, T-006 mini-retro, T-009 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
First ADR drafted under the new ADR-0013 rules (commit 56fd9eb). Status lands at `Proposed`; per the new ADR cool-down rule, Accept happens no earlier than 2026-04-28 — separate commit. This ADR is the validation event for whether the codified rules are real progress or paper. The decision: aarch64 has four Exception Levels; the kernel runs at exactly one (EL1) from the moment kernel_entry begins. firmware / emulator delivery EL varies (QEMU virt default = EL1; QEMU `-machine virtualization=on` = EL2; real hardware varies). Today's boot.s does no transition; it just trusts the delivery EL. T-009's UNSAFE-2026-0016 catches the violation loudly but cannot recover. Three options weighed: (A) always drop to EL1 in boot.s — chosen. (B) adapt to whichever EL we got — rejected; multiplies HAL-impl maintenance across every later phase. (C) hard-fail on non-EL1 — rejected; loses compatibility with virtualization-aware boot stacks. Option A's cost is ~10 lines of boot asm, runs once. Benefit: one target EL across the entire kernel; works under both QEMU configs and real-hardware stacks; UNSAFE-2026-0016's runtime check is now the post-condition of the transition (load-bearing rather than defensive). VHE explicitly stays off (HCR_EL2.{E2H, TGE} = {0, 0}), matching T-009's UNSAFE-2026-0015 Amendment that describes Tyrne's EL1 as "non-VHE". The ADR's *Decision outcome → Dependency chain* subsection (the new ADR-0013 requirement, first time used in production) lists every step needed to fully realise the decision, each grounded in a real T-NNN file: 1. CurrentEL read primitive — UNSAFE-2026-0016 / T-009 (already exists) 2. boot.s EL2→EL1 transition — T-013 (Draft, opened in this commit) 3. Rust current_el accessor — T-013 4. Tests at both QEMU configs — T-013 T-013 is opened in the same commit per the new "Forward-reference contract" rule — no "future, not-yet-opened" wording allowed. T-013's Dependencies field marks ADR-0024 as the gating decision; its status will move to In Progress only after the cool-down expires and ADR-0024 is Accepted. Files: - docs/decisions/0024-el-drop-policy.md (new, Status: Proposed) - docs/analysis/tasks/phase-b/T-013-el-drop-to-el1.md (new, Status: Draft) - docs/decisions/README.md (ADR index gains 0024 row) - docs/analysis/tasks/phase-b/README.md (task index gains T-013 row) - docs/roadmap/phases/phase-b.md §B1 sub-breakdown updated to reference ADR-0024 + T-013 explicitly (removes the original prose-form description of items 1–4) - docs/roadmap/current.md gains ADR-0024 in Active decisions This commit is the validation milestone for ADR-0013 §"ADR cool- down": NO Accept today. The Accept commit lands tomorrow at earliest, with whatever review-feedback edits arise from sleeping on the draft. Refs: ADR-0024, ADR-0013, ADR-0022 first-rider sub-rider, T-013 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
158-170: Guard works; considertaiki-e/install-actionfor a cleaner cache story.The presence check correctly avoids the "binary already exists" failure on cache hits, and
command -vis appropriate since~/.cargo/binis onPATHfor GitHub-hosted runners. Two minor observations:
- With
restore-keysfalling back acrossCargo.lockhashes (sameNIGHTLY_PIN), the guard can keep an oldercargo-llvm-covindefinitely until aNIGHTLY_PINbump invalidates the cache. That's usually fine for an informational job, but worth knowing if you ever want to track upstreamcargo-llvm-covfixes.cargo install --locked cargo-llvm-covfloats to whatever version is current at install time. Pinning a version (e.g.,cargo install --locked cargo-llvm-cov@<x.y.z>) would make the cached artifact deterministic and make pin bumps explicit, mirroring the policy you already apply toNIGHTLY_PIN.A drop-in alternative that handles both points (and is what most rust CI setups have converged on) is
taiki-e/install-action, which downloads prebuilt binaries and manages its own cache:♻️ Optional simplification using
taiki-e/install-action- - name: Install cargo-llvm-cov (cached) - run: | - # The cache restore step above places ~/.cargo/bin/cargo-llvm-cov - # if a prior run installed it. `cargo install` (older cargo) - # errors with "binary already exists" when the file is present, - # so guard the install with a presence check. On a cache miss - # this still installs cleanly; on a cache hit it's a no-op - # rather than a 1-2 min rebuild. - if command -v cargo-llvm-cov >/dev/null; then - echo "cargo-llvm-cov already installed: $(cargo-llvm-cov --version 2>/dev/null || echo 'version unknown')" - else - cargo install --locked cargo-llvm-cov - fi + - name: Install cargo-llvm-cov + uses: taiki-e/install-action@v2 + with: + tool: cargo-llvm-covHappy to leave the current shape as-is if you prefer to minimise third-party action surface area in CI — the guard itself is correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 158 - 170, The CI step installing cargo-llvm-cov can lead to non-deterministic cached binaries and floating upstream installs; update the step that checks for cargo-llvm-cov (currently using command -v cargo-llvm-cov and cargo install --locked cargo-llvm-cov) to pin a specific version (e.g., use cargo install --locked cargo-llvm-cov@<x.y.z>) or replace the block with the taiki-e/install-action to download prebuilt, cacheable binaries and tie caching to NIGHTLY_PIN/restore-keys so the cache invalidates predictably when you bump NIGHTLY_PIN; ensure the presence-check logic still short-circuits when the binary exists on PATH to avoid "binary already exists" failures.hal/src/timer.rs (1)
56-294: LGTM — solid host-testable arithmetic with the right invariants.The 128-bit intermediate plus saturating cast in
ticks_to_ns, the round-to-nearest formula with floor-of-1 ns clamp inresolution_ns_for_freq, and the explicitassert!onfrequency_hz != 0(with named messages) cover the corner cases that historically bite Generic-Timer code: u64 overflow at the ~584-year horizon, sub-nanosecond resolution at multi-GHz, and divide-by-zero on uninitialised firmware. The unit-test set also locks the rounding policy (33,333,333 Hz / 19.2 MHz / 54 MHz cases) so a future "floor instead of round" regression cannot silently slip past CI.One minor optional refactor only — no action required.
♻️ Optional refactor: lock monotonicity as a property test
The contract
Timer::now_nsexposes is "never goes backwards". The current tests verify discrete points; consider a small property-style sweep so any future change to the saturation branch or the cast still satisfies the contract by construction:+ #[test] + fn ticks_to_ns_is_monotonic_non_decreasing() { + // Non-strict monotonicity is the contract Timer::now_ns inherits. + for &freq in &[1u64, 19_200_000, 62_500_000, 1_000_000_000, 3_000_000_000] { + let mut prev = 0u64; + for c in (0u64..1_000).chain([u64::MAX - 1, u64::MAX]) { + let ns = ticks_to_ns(c, freq); + assert!(ns >= prev, "non-monotonic at count={c} freq={freq}: {prev} -> {ns}"); + prev = ns; + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hal/src/timer.rs` around lines 56 - 294, Optional refactor suggestion: add a small property-style unit test to lock monotonicity (never decreases) and correct saturation behaviour; implement a test in the tests module that sweeps increasing tick counts and asserts ticks_to_ns(count_next, freq) >= ticks_to_ns(count_prev, freq) for representative frequencies (including 1 Hz and a high freq) and also verifies that very large counts saturate to u64::MAX (use the existing ticks_to_ns and NANOS_PER_SECOND symbols); keep it lightweight and deterministic so CI exercises the saturation branch and prevents future regressions to monotonicity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/analysis/tasks/phase-b/T-012-exception-and-irq-infrastructure.md`:
- Line 36: Update the acceptance-criteria text to use the exact method names
from the IrqController trait: replace the mentions of enable_irq / disable_irq
with enable / disable and confirm the other method names (acknowledge,
end_of_interrupt) match the trait; refer to the bsp-qemu-virt gic module (e.g.
bsp-qemu-virt/src/gic.rs) and the IrqController trait declaration
(IrqController) to ensure the doc uses the trait's precise identifiers.
In `@docs/decisions/0013-roadmap-and-planning.md`:
- Around line 147-173: The added governance rules (the "ADR cool-down",
"Forward-reference contract", and "Riders are not failures" sections) must not
be edited into ADR-0013's accepted body; instead create a new ADR (e.g.,
ADR-00XX) that records these three rules, declares it supersedes/extends
ADR-0013, and moves the full text there, then update ADR-0013 only to append a
short pointer/note referencing the new ADR-00XX (preserving ADR-0013's original
body intact). Locate the new content by the section headings "ADR cool-down",
"Forward-reference contract", and "Riders..." and ensure the new ADR explicitly
uses a supersedes link to ADR-0013 and follows the project's ADR naming/status
conventions.
In `@docs/roadmap/phases/phase-b.md`:
- Line 75: Update the milestone scope sentence to correct the milestone status
and GIC terminology: mark T-009 consistently (use its actual current state
rather than saying “closed” if it’s still “In Review”) and replace the incorrect
“GIC v2 redistributor” phrasing with GICv3 terminology or simply “redistributor
(GICv3)” while keeping the concrete items: distributor/redistributor/interface
configuration on QEMU virt, an EL1 exception vector table install at VBAR_EL1, a
thin handler-dispatch loop, and the generic-timer IRQ wiring that allows
Timer::arm_deadline and Timer::cancel_deadline to fire interrupts so idle can
move from spin_loop to wfi.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 158-170: The CI step installing cargo-llvm-cov can lead to
non-deterministic cached binaries and floating upstream installs; update the
step that checks for cargo-llvm-cov (currently using command -v cargo-llvm-cov
and cargo install --locked cargo-llvm-cov) to pin a specific version (e.g., use
cargo install --locked cargo-llvm-cov@<x.y.z>) or replace the block with the
taiki-e/install-action to download prebuilt, cacheable binaries and tie caching
to NIGHTLY_PIN/restore-keys so the cache invalidates predictably when you bump
NIGHTLY_PIN; ensure the presence-check logic still short-circuits when the
binary exists on PATH to avoid "binary already exists" failures.
In `@hal/src/timer.rs`:
- Around line 56-294: Optional refactor suggestion: add a small property-style
unit test to lock monotonicity (never decreases) and correct saturation
behaviour; implement a test in the tests module that sweeps increasing tick
counts and asserts ticks_to_ns(count_next, freq) >= ticks_to_ns(count_prev,
freq) for representative frequencies (including 1 Hz and a high freq) and also
verifies that very large counts saturate to u64::MAX (use the existing
ticks_to_ns and NANOS_PER_SECOND symbols); keep it lightweight and deterministic
so CI exercises the saturation branch and prevents future regressions to
monotonicity.
🪄 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: ea011db1-368d-4a86-9528-adb48e146547
📒 Files selected for processing (22)
.claude/skills/write-adr/SKILL.md.github/workflows/ci.ymlREADME.mdbsp-qemu-virt/src/cpu.rsbsp-qemu-virt/src/main.rsdocs/analysis/reviews/business-reviews/2026-04-27-T-009-mini-retro.mddocs/analysis/reviews/business-reviews/README.mddocs/analysis/tasks/phase-b/README.mddocs/analysis/tasks/phase-b/T-009-timer-init-cntvct.mddocs/analysis/tasks/phase-b/T-012-exception-and-irq-infrastructure.mddocs/analysis/tasks/phase-b/T-013-el-drop-to-el1.mddocs/audits/unsafe-log.mddocs/decisions/0013-roadmap-and-planning.mddocs/decisions/0022-idle-task-and-typed-scheduler-deadlock.mddocs/decisions/0024-el-drop-policy.mddocs/decisions/README.mddocs/decisions/template.mddocs/glossary.mddocs/roadmap/current.mddocs/roadmap/phases/phase-b.mdhal/src/lib.rshal/src/timer.rs
Three findings + two nitpicks from the in-progress review of the 2026-04-27 commit cluster (cool-down rules + ADR-0024 + T-012/T-013). All accepted; all addressed. 1. T-012 used wrong IrqController method names. The trait declares enable / disable / acknowledge / end_of_interrupt (no `_irq` suffix); T-012's GIC-driver acceptance criterion said "enable_irq / disable_irq / ...". Updated to the trait's actual identifiers and added the spurious-INTID-1023→None mapping noted in the trait contract. Verified against hal/src/irq_controller.rs. 2. ADR-0013 in-place edit was an append-only-policy violation — ironic, since the rules being added were *about* the append-only policy. Reverted ADR-0013's body to its pre-56fd9eb state. Created ADR-0025 (Proposed) with the three rules (ADR cool-down, forward-reference contract, riders-are-not-failures) as a first-class ADR that extends ADR-0013. ADR-0013 gains a single "Revision notes" rider pointing at ADR-0025 — the rider pattern ADR-0025 itself codifies. ADR-0025 also follows its own rules: lands as `Proposed` today, Accept ≥ 2026-04-29 per cool-down. Cross-references in eight other files (T-012, T-013, current.md, phase-b.md, template.md, ADR-0024, ADR-0022, T-009 mini-retro) that pointed at "ADR-0013 §X" for the moved rules are rewritten to "ADR-0025 §Rule N (...)". The skill update (write-adr) and template.md "Dependency chain" subsection from commit 56fd9eb stay where they are — they are not ADR bodies and editing them was not a policy violation; only their citations move. 3. phase-b.md §B1 milestone scope sentence had two errors: - "T-009 closed the time-source half" — T-009 is `In Review`, not closed. Softened to "landed in `In Review`". - "GIC v2 distributor / redistributor / interface" — GICv2 has no redistributor; that is GICv3 terminology. Corrected to "GICv2 distributor + CPU interface" with a parenthetical noting QEMU virt's default GIC version. Same fix applied to template.md's example dependency-chain. 4. (Nitpick) cargo-llvm-cov install flow rewritten. Replaced the `if command -v cargo-llvm-cov; then ... else cargo install --locked` block with `taiki-e/install-action@v2` pinned to `cargo-llvm-cov@0.6.16`. Two wins: (a) version is explicitly pinned in the workflow, no upstream surprise; (b) prebuilt binary download instead of compile-from-source — fast even on cache-miss. ci.md "Nightly pinning" section updated with the tool-pin bump procedure. 5. (Nitpick) Property-style monotonicity tests added in hal/src/timer.rs. Two new tests: (a) `ticks_to_ns_is_monotonic_across_frequencies` sweeps increasing tick counts across [1 Hz, 19.2 MHz, 62.5 MHz, 1 GHz] and asserts non-decreasing output (locks the ADR-0010 monotonicity contract; would fail under wrapping_mul, passes under the chosen saturating cast); (b) `ticks_to_ns_plateaus_at_u64_max_after_saturation` confirms the saturated value never decreases past the boundary (no wrap-to-zero). Lightweight: ≤ 200 samples per frequency, all on the host. Verification: - 77 kernel + 19 hal (was 17 → +2 monotonicity) + 34 test-hal = 130 host tests green. - cargo +nightly miri test --workspace --exclude tyrne-bsp-qemu-virt remains clean. - cargo fmt / host-clippy / kernel-clippy / kernel-build clean. - YAML lint clean. - Residual "ADR-0013 §X" cross-references: zero. Refs: ADR-0025, ADR-0013, ADR-0024, ADR-0011 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Done; ADR fixes + phase-b ADR-id collision repair Acts on the 2026-04-27 approval-review report. Six items in scope; two APPROVE → Done, one NEEDS-MORE-INFO → row added → Done, two NEEDS-CHANGES (ADRs in cool-down — text fixes applied, status not flipped), and four cross-document concerns: one ADR-id collision repaired, one stale "T-009 not yet opened" cleanup, two acknowledged without action. Verdict-driven actions: T-006 — APPROVE → Done. Status flipped; six review-history rows already documented the work + post-review fixes + close-out pass. T-007 — NEEDS-MORE-INFO → APPROVE → Done. Added two review-history rows: (a) the second-read pass that produced the §"Deferred follow-ups" section (the textual evidence the reviewer flagged), and (b) the 2026-04-27 close-out approval-review verdict. Status flipped. T-009 — APPROVE → Done. Three review-fix rounds + close-out review already documented; status flipped. Cool-down items (no status flip — that's tomorrow's work): ADR-0024 — fixed Option A's "(out of scope for v1; future task)" wording to "(see §Open questions — EL3 boot behaviour)" per ADR-0025 §Rule 2. The §Open questions entry already exists. ADR-0025 — fixed Date 2026-04-28 → 2026-04-27 to match commit 9e5925a's actual date. Cool-down therefore expires 2026-04-28 (same as ADR-0024); both Accepts can land tomorrow in separate commits. The body's three Date references updated in lockstep (lines 17, 47, 74). README index row updated. ADR-0013's pointer rider also updated to 2026-04-27. Cross-document concerns: ADR-id collision in phase-b.md repaired. The ADR planning table at phase-b.md:241 named ADR-0025 / ADR-0026 / ADR-0027 / ADR-0028 / ADR-0029 as B2..B5 placeholders, but real-world ADR-0025 is now governance amendments and T-012 reserves ADR-0026 conditionally for exception-vector work. Renumbered B2..B5 placeholders by 2: now ADR-0027 / 0028 / 0029 / 0030 / 0031, with new table rows for ADR-0025 (governance) and ADR-0026 (T-012 reservation, conditional) explicitly slotted in. All §B2..§B5 sub-breakdown items and acceptance criteria updated to match. Note: a sed cascade landed B2's "- ADR-XX Accepted" line at ADR-0029; corrected to ADR-0027 by hand. T-011 stale "T-009 not yet opened" wording (lines 50, 52) updated to point at T-009 In Review (now Done) and T-012 Draft. T-006/T-007/T-009 moved off the "In Review" list in current.md; "Last completed task" replaced with the three-task batch note. Acknowledged without action: - ADR-0022 has 3+ riders within its first week — at the ADR-0025 §Rule 3 "rider-frequency signal" threshold. Not a blocker on the current items; the rule was written *because* of this pattern, so the signal already produced the fix (the codification itself). Revisit if a fourth rider lands in the next two weeks. - No orphan audit tags. Status-trio sync clean across the index trio. Verification: - 77 kernel + 19 hal + 34 test-hal = 130 host tests green (no code changes; gates re-run from clean state). - cargo fmt / host-clippy / kernel-clippy / kernel-build clean. - YAML lint clean. - B0 milestone is now four tasks done (T-006/T-007/T-009 promoted here; T-011 ready to start; T-008 still to open). ADR-0024 and ADR-0025 stay `Proposed` per ADR-0025 §Rule 1 cool-down. Accept commits land tomorrow (≥ 2026-04-28) as separate commits. Refs: ADR-0025, ADR-0024, T-006, T-007, T-009 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…24 + ADR-0025 same-day The cool-down rule (24-hour gate between Proposed and Accepted) is withdrawn before Accept on maintainer judgement: the substance the rule enforced (careful re-reading before Accept) is preserved as the write-adr skill's careful-re-read step plus independent agent reviews, without imposing a calendar-day delay disproportionate to the project's single-author + AI-agent cadence. ADR-0025 ships with two surviving rules — Forward-reference contract (§Rule 1) and Riders-are-not-failures (§Rule 2) — both mechanical and self-checkable. ADR-0024 (EL drop to EL1) becomes the first ADR to use ADR-0025's *Dependency chain* section in production; both Accept on 2026-04-27 after careful re-read. Ripple: §Rule 2/3 cross-references renumbered to §Rule 1/2 across T-012, T-013, ADR-0022 sub-rider, ADR-0024, template, phase-b.md. Cool-down date wording (≥ 2026-04-28) removed from T-013, phase-b §B1 sub-breakdown, current.md, ADR-0024. write-adr skill step 10 swapped from "Cool-down before Accept" to "Careful re-read before Accept"; matching acceptance criterion and anti-pattern updated. ADR-0013 pointer rider gains a follow-up bullet documenting the two-rule outcome (original three-rule line stays intact per the new append-only rule). T-009 mini-retro gains a follow-up note for the same reason. Refs: ADR-0024, ADR-0025 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…aft) Append-only second follow-up to the T-009 mini-retro covering six items the original retro got wrong or punted on: test count correction (128 → 130 — resolution-floor cases not counted at retro time), "user-driven thoroughness" lesson now answered (independent approval-review agent shipped 2026-04-27, sample-size 1 → defer skill canonisation to sample-size 2), Adjustments status snapshot (1/4 done; stale-link CI re-routed to R-series; post-fix sweep DoD deferred to B0 retro), Cluster-1 CNTPCT clarification, cost-of-arc question deferred to B0 closure retro, security-review delta gap acknowledged with consolidated B0-closure security-review pass deferred. B0 closure agenda Draft deliberately not opened — trigger is T-011 reaching In Review. T-008 opened as Draft. Scope: docs/architecture/scheduler.md (new) + docs/architecture/ipc.md (new) + small updates to hal.md (Timer subsection post-T-009) and overview.md (Phase A complete cross-link). boot.md is explicitly out of T-008's scope — T-013's DoD already owns the EL drop documentation update there. capabilities.md, memory.md, and a standalone timer.md are deferred to later phases (B6 / B2 / out-of-need respectively). Phase-b/README and phase-b.md sub-breakdown updated; current.md notes T-008 opened + every B0 task now has a file (Forward-reference contract satisfied for B0). Refs: ADR-0017, ADR-0019, ADR-0020, ADR-0021, ADR-0022, ADR-0010, ADR-0025 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
.github/workflows/ci.yml (1)
147-158: Inline comment contradicts the docs and references a non-existent env var.The comment says "Bump
CARGO_LLVM_COV_VERSIONbelow in lockstep withNIGHTLY_PIN", but (a) there is noCARGO_LLVM_COV_VERSIONenv var — the version is hard-coded inline astool: cargo-llvm-cov@0.6.16(Line 158); and (b)docs/guides/ci.mdLine 69 explicitly states the two pins can be bumped independently. Suggest aligning the comment with the docs and the actual workflow.📝 Proposed wording fix
- # Bump CARGO_LLVM_COV_VERSION below in lockstep with NIGHTLY_PIN - # — see docs/guides/ci.md "Nightly pinning" for the procedure. + # Bump the cargo-llvm-cov version below independently of, or + # together with, NIGHTLY_PIN — see docs/guides/ci.md + # "Nightly pinning" for the procedure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 147 - 158, The inline comment wrongly references a non-existent CARGO_LLVM_COV_VERSION env var and contradicts docs/guides/ci.md about independent pins; update the comment around the taiki-e/install-action block (the uses: taiki-e/install-action@v2 and tool: cargo-llvm-cov@0.6.16 lines) to remove the CARGO_LLVM_COV_VERSION reference and state that the cargo-llvm-cov version is pinned inline and may be bumped independently of NIGHTLY_PIN per docs/guides/ci.md.docs/decisions/template.md (2)
42-42: Example references T-009 asIn Review, but the PR promotes T-009 to Done.In the example block, Step 1 says "T-009 (In Review)". Within this PR,
docs/analysis/tasks/phase-b/T-009-timer-init-cntvct.mdLine 5 sets T-009's status toDone. Since this is just an illustrative example in the template, it isn't load-bearing — but consider using a hypothetical/T-NNNplaceholder so the example doesn't go stale every time a referenced task's status changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/decisions/template.md` at line 42, The example in docs/decisions/template.md currently references a real task "CNTVCT_EL0 read path — T-009 (In Review)" which can become stale; update that example to use a hypothetical placeholder (e.g., "CNTVCT_EL0 read path — T-NNN (Status)") or a generic status token instead of the real task T-009 (referenced by docs/analysis/tasks/phase-b/T-009-timer-init-cntvct.md) so the template remains evergreen; replace the literal "T-009 (In Review)" with "T-NNN (Status)" or similar placeholder wherever the example appears.
40-49: Add a language tag to the fenced code block (markdownlint MD040).The example block uses a bare fence; markdownlint reports MD040 (
fenced-code-language). Usetext(ormarkdown) since the contents are illustrative prose.📝 Proposed fix
Example: -``` +```text For this decision to be fully in effect: 1. CNTVCT_EL0 read path — T-009 (In Review) 2. Generic-timer compare register programming — T-012 (Draft, IRQ-wiring task) 3. GICv2 distributor + CPU interface configuration — T-012 4. EL1 exception vector table install — T-012 T-009 closes only step 1. Steps 2-4 are scoped under T-012, opened in the same commit as this ADR.</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/decisions/template.mdaround lines 40 - 49, The fenced code block
containing the checklist prose needs a language tag to satisfy markdownlint
MD040; update the triple-backtick fence that wraps the list (the code block
beginning with "For this decision to be fully in effect:") to use ```text (orhal/src/timer.rs (2)
136-153: Round-to-nearest is approximate at the half-way point due to integer division offrequency_hz / 2.
(NANOS_PER_SECOND + frequency_hz / 2) / frequency_hztruncatesfrequency_hz / 2for odd frequencies, which rounds down by 0.5/frequency_hz. In practice this is a sub-nanosecond correction and never affects any frequency Tyrne targets — but if you want strict round-half-up you can compute(2 * NANOS_PER_SECOND + frequency_hz) / (2 * frequency_hz). Note this would overflowu64forfrequency_hz > u64::MAX/2(~9.2e18 Hz), so the existing form is safer for the high-frequency clamp case. Recommendation: keep the current form (it's correct for every realistic counter and the clamp covers the truly extreme case) but consider documenting the half-way behaviour explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hal/src/timer.rs` around lines 136 - 153, The current round-to-nearest expression in resolution_ns_for_freq contains a half-way bias due to integer truncation of frequency_hz / 2 for odd frequencies; leave the computation as-is (it is safer for extreme frequencies and clamped to ≥1) but add a short comment inside the resolution_ns_for_freq function documenting that the current form rounds-half-to-even-ish because of truncation, that a strict round-half-up alternative is (2 * NANOS_PER_SECOND + frequency_hz) / (2 * frequency_hz) but that alternative can overflow for frequency_hz > u64::MAX/2, and that the clamp handles pathological high-frequency cases.
89-106: Minor: clippyreasonwording is inverted relative to the function body.The attribute on Line 91 says the saturating cast is "handled explicitly by the if/else guard above" — but the guard at Lines 101–105 is below the attribute. Reading the reason in isolation (e.g. in a clippy diagnostic) is mildly confusing.
📝 Proposed wording tweak
#[allow( clippy::cast_possible_truncation, - reason = "saturating cast handled explicitly by the if/else guard above" + reason = "saturating cast handled explicitly by the if/else guard at the end of the function" )]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hal/src/timer.rs` around lines 89 - 106, The clippy reason text on the attribute for ticks_to_ns incorrectly references an "if/else guard above" even though the saturating clamp is below; update the reason wording to accurately reference the guard (e.g., "saturating cast handled explicitly by the if/else guard below" or "handled by the saturating clamp at the end of this function") so the attribute matches the function body and any standalone diagnostics; locate the attribute attached to pub const fn ticks_to_ns and change only the reason string..claude/skills/write-adr/SKILL.md (1)
39-39: Skill updates correctly enforce ADR-0025 §Rule 1 and the cool-down withdrawal substitute.The new dependency-chain step (Line 39), careful re-read step (Line 58), separated Propose/Accept commits (Line 69), and forward-reference anti-pattern (Line 79) cleanly mirror the rules in
docs/decisions/0025-adr-governance-amendments.md. One minor observation: Line 30 (unchanged) still permits an initialAcceptedstatus "if the decision is being recorded after the fact"; the new acceptance criterion at Line 69 says "never in the same commit as the initial draft". The retroactive-recovery exception called out in anti-pattern 5 (Line 77) reconciles these, but a single-sentence pointer between the two would help future readers spot the exception.Also applies to: 58-58, 65-65, 69-69, 78-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/write-adr/SKILL.md at line 39, Summary: Add a one-sentence cross-reference that clarifies the retroactive acceptance exception so readers see how the older allowance ("initial `Accepted` status 'if the decision is being recorded after the fact'") interacts with the new rule ("never in the same commit as the initial draft") and anti-pattern 5 ("retroactive-recovery exception"). Edit SKILL.md to insert a short pointer after the sentence that permits retroactive Accepted status (the phrase "if the decision is being recorded after the fact") linking it to the new acceptance criterion (the phrase "never in the same commit as the initial draft") and to anti-pattern 5, stating that the retroactive-recovery exception is the explicit reconciliation between them; keep wording concise and add no procedural changes, only the clarifying cross-reference that cites ADR-0025.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/analysis/reviews/business-reviews/2026-04-27-T-009-mini-retro.md`:
- Line 45: Replace the mixed-language typo token "ACı" in the document with the
normalized English acronym "AC"; locate the occurrence of "ACı" in the MADR
template / skill description and update it to "AC" (and run a quick grep for any
other "ACı" occurrences to ensure no other non-English characters remain).
- Line 74: Replace the Turkish quotation "kabul edilen bulguların tamamı
düzeltildi mi?" in the document with its English equivalent to comply with the
repo language policy; specifically change that exact phrase to "Have all
accepted findings actually been fixed?" so the sentence remains the same
semantics but is fully in English (update the quoted text wherever it appears in
the file).
In `@docs/analysis/tasks/phase-b/T-009-timer-init-cntvct.md`:
- Line 110: Update the post-commit audit row in T-009-timer-init-cntvct.md that
currently states "124 host tests" to reflect the final PR test count of 130 (the
PR added two monotonicity tests in hal/src/timer.rs); alternatively add a short
follow-up review-history row documenting the follow-up commit that added the two
monotonicity tests and increased the host-test total from 124 to 130 so the doc
matches the hal/src/timer.rs test counts (19 HAL tests: 77 + 19 + 34 = 130).
In `@docs/decisions/0025-adr-governance-amendments.md`:
- Line 11: The sentence currently states “four ADRs (ADR-0021, ADR-0022)” which
mismatches the listed ADRs; change the phrasing to reflect that there were two
ADRs (ADR-0021, ADR-0022) that produced four riders—e.g., replace with “four
riders across two ADRs (ADR-0021, ADR-0022)” or “two ADRs (ADR-0021, ADR-0022)
which produced four riders”; ensure the surrounding mention of ADR-0013’s
framing still reads correctly with the new wording.
In `@docs/roadmap/phases/phase-b.md`:
- Line 242: Update the ADR-0026 note to reference the correct task: change the
dangling "T-011" mention to "T-012" so the clause reads that ADR-0026 "may go
unused if T-012 absorbs the exception-vector design"; locate the ADR-0026 row
(text containing "Exception-vector-table / handler-dispatch shape (T-012,
conditional)") and replace the incorrect task symbol T-011 with T-012 to keep
the roadmap consistent.
---
Nitpick comments:
In @.claude/skills/write-adr/SKILL.md:
- Line 39: Summary: Add a one-sentence cross-reference that clarifies the
retroactive acceptance exception so readers see how the older allowance
("initial `Accepted` status 'if the decision is being recorded after the fact'")
interacts with the new rule ("never in the same commit as the initial draft")
and anti-pattern 5 ("retroactive-recovery exception"). Edit SKILL.md to insert a
short pointer after the sentence that permits retroactive Accepted status (the
phrase "if the decision is being recorded after the fact") linking it to the new
acceptance criterion (the phrase "never in the same commit as the initial
draft") and to anti-pattern 5, stating that the retroactive-recovery exception
is the explicit reconciliation between them; keep wording concise and add no
procedural changes, only the clarifying cross-reference that cites ADR-0025.
In @.github/workflows/ci.yml:
- Around line 147-158: The inline comment wrongly references a non-existent
CARGO_LLVM_COV_VERSION env var and contradicts docs/guides/ci.md about
independent pins; update the comment around the taiki-e/install-action block
(the uses: taiki-e/install-action@v2 and tool: cargo-llvm-cov@0.6.16 lines) to
remove the CARGO_LLVM_COV_VERSION reference and state that the cargo-llvm-cov
version is pinned inline and may be bumped independently of NIGHTLY_PIN per
docs/guides/ci.md.
In `@docs/decisions/template.md`:
- Line 42: The example in docs/decisions/template.md currently references a real
task "CNTVCT_EL0 read path — T-009 (In Review)" which can become stale; update
that example to use a hypothetical placeholder (e.g., "CNTVCT_EL0 read path —
T-NNN (Status)") or a generic status token instead of the real task T-009
(referenced by docs/analysis/tasks/phase-b/T-009-timer-init-cntvct.md) so the
template remains evergreen; replace the literal "T-009 (In Review)" with "T-NNN
(Status)" or similar placeholder wherever the example appears.
- Around line 40-49: The fenced code block containing the checklist prose needs
a language tag to satisfy markdownlint MD040; update the triple-backtick fence
that wraps the list (the code block beginning with "For this decision to be
fully in effect:") to use ```text (or ```markdown) instead of a bare fence so
the block is marked as text.
In `@hal/src/timer.rs`:
- Around line 136-153: The current round-to-nearest expression in
resolution_ns_for_freq contains a half-way bias due to integer truncation of
frequency_hz / 2 for odd frequencies; leave the computation as-is (it is safer
for extreme frequencies and clamped to ≥1) but add a short comment inside the
resolution_ns_for_freq function documenting that the current form
rounds-half-to-even-ish because of truncation, that a strict round-half-up
alternative is (2 * NANOS_PER_SECOND + frequency_hz) / (2 * frequency_hz) but
that alternative can overflow for frequency_hz > u64::MAX/2, and that the clamp
handles pathological high-frequency cases.
- Around line 89-106: The clippy reason text on the attribute for ticks_to_ns
incorrectly references an "if/else guard above" even though the saturating clamp
is below; update the reason wording to accurately reference the guard (e.g.,
"saturating cast handled explicitly by the if/else guard below" or "handled by
the saturating clamp at the end of this function") so the attribute matches the
function body and any standalone diagnostics; locate the attribute attached to
pub const fn ticks_to_ns and change only the reason string.
🪄 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: 19a53839-560d-4826-8afc-4b93cf3e7701
📒 Files selected for processing (21)
.claude/skills/write-adr/SKILL.md.github/workflows/ci.ymldocs/analysis/reviews/business-reviews/2026-04-27-T-009-mini-retro.mddocs/analysis/tasks/phase-b/README.mddocs/analysis/tasks/phase-b/T-006-raw-pointer-scheduler-api.mddocs/analysis/tasks/phase-b/T-007-idle-task-typed-deadlock.mddocs/analysis/tasks/phase-b/T-008-architecture-docs.mddocs/analysis/tasks/phase-b/T-009-timer-init-cntvct.mddocs/analysis/tasks/phase-b/T-011-missing-tests-bundle.mddocs/analysis/tasks/phase-b/T-012-exception-and-irq-infrastructure.mddocs/analysis/tasks/phase-b/T-013-el-drop-to-el1.mddocs/decisions/0013-roadmap-and-planning.mddocs/decisions/0022-idle-task-and-typed-scheduler-deadlock.mddocs/decisions/0024-el-drop-policy.mddocs/decisions/0025-adr-governance-amendments.mddocs/decisions/README.mddocs/decisions/template.mddocs/guides/ci.mddocs/roadmap/current.mddocs/roadmap/phases/phase-b.mdhal/src/timer.rs
✅ Files skipped from review due to trivial changes (7)
- docs/analysis/tasks/phase-b/T-006-raw-pointer-scheduler-api.md
- docs/analysis/tasks/phase-b/T-008-architecture-docs.md
- docs/decisions/README.md
- docs/analysis/tasks/phase-b/T-013-el-drop-to-el1.md
- docs/analysis/tasks/phase-b/T-011-missing-tests-bundle.md
- docs/analysis/tasks/phase-b/README.md
- docs/analysis/tasks/phase-b/T-012-exception-and-irq-infrastructure.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/decisions/0013-roadmap-and-planning.md
| - **ADR-0010** (Timer trait, accepted 2026-04-20) — BSP-side first real implementation lands. | ||
| - **ADR-0022 first rider** — gains a *Sub-rider* clarifying the T-009 (time source) vs T-012 (IRQ delivery) two-task path. Sub-rider grounded at T-012's task file in commit `ca962d6`. | ||
| - **ADR-0013** (roadmap & planning, originally accepted 2026-04-20) — amended in-place to codify three new rules: ADR cool-down (no same-day Accept), Forward-reference contract (every "future task" claim grounded at a real T-NNN), Riders-are-not-failures (frequency is the signal, not presence). | ||
| - **Template + write-adr skill** — `Dependency chain` subsection added to MADR template; skill procedure gains a cool-down step and a dependency-chain ACı. |
There was a problem hiding this comment.
Fix mixed-language typo in acronym token.
Line 45 uses ACı; this looks like an accidental non-English character and should be normalized (likely AC).
Proposed doc fix
-- **Template + write-adr skill** — `Dependency chain` subsection added to MADR template; skill procedure gains a cool-down step and a dependency-chain ACı.
+- **Template + write-adr skill** — `Dependency chain` subsection added to MADR template; skill procedure gains a cool-down step and a dependency-chain AC.As per coding guidelines: **/*: Source code, comments, doc-comments, documentation, commit messages, PR descriptions, and issue text must be in English.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - **Template + write-adr skill** — `Dependency chain` subsection added to MADR template; skill procedure gains a cool-down step and a dependency-chain ACı. | |
| - **Template + write-adr skill** — `Dependency chain` subsection added to MADR template; skill procedure gains a cool-down step and a dependency-chain AC. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/analysis/reviews/business-reviews/2026-04-27-T-009-mini-retro.md` at
line 45, Replace the mixed-language typo token "ACı" in the document with the
normalized English acronym "AC"; locate the occurrence of "ACı" in the MADR
template / skill description and update it to "AC" (and run a quick grep for any
other "ACı" occurrences to ensure no other non-English characters remain).
|
|
||
| **Reviews left their own stale documentation behind.** The five findings in the third-round review (commit `328cab5`) were almost entirely *generated by* the second-round fix commits (`39fb66c`, `db3a4c7`, `01010f4`). Each fix commit corrected one type of staleness while leaving another type behind: the second-round renamed `cntpct → cntvct` in code but not in task-file title; the runtime-EL-check commit added the assertion but not the doc-comment audit-tag note; the explicit-panic commit added asserts but missed the resolution-floor edge case. The pattern is **mini-rider chains within the rider response itself** — one level of recursion below the ADR/rider chain we already knew about. The third-round was the maintainer's own catch ("are all accepted findings actually fixed?"), not an agent-driven find. **The implication:** even with a strong review process, expect the *fixes* to need a small follow-up sweep one round after they land. Build that into DoD rather than be surprised by it. | ||
|
|
||
| **User-driven thoroughness beats agent-driven thoroughness when the agent has been at it for a while.** The maintainer's question "kabul edilen bulguların tamamı düzeltildi mi?" (are all accepted findings actually fixed?) caught the silently-skipped runtime EL check — a real correctness gap. The agent had self-reported "all accepted findings fixed"; the maintainer's verification surfaced that this was wrong. This is the same lesson as the T-006 retro's "post-In-Review second-read" gate, applied one level deeper: the *fix-up commits* themselves need a verification gate. The user is providing this manually today; whether to automate it (e.g. an "all-findings-actually-applied" agent pass) is open. |
There was a problem hiding this comment.
Replace non-English quote with English to satisfy repo language policy.
Line 74 includes Turkish text (kabul edilen bulguların tamamı düzeltildi mi?). Please translate it to English in-place for consistency and policy compliance.
Proposed doc fix
-**User-driven thoroughness beats agent-driven thoroughness when the agent has been at it for a while.** The maintainer's question "kabul edilen bulguların tamamı düzeltildi mi?" (are all accepted findings actually fixed?) caught the silently-skipped runtime EL check — a real correctness gap.
+**User-driven thoroughness beats agent-driven thoroughness when the agent has been at it for a while.** The maintainer's question "were all accepted findings actually fixed?" caught the silently-skipped runtime EL check — a real correctness gap.As per coding guidelines: **/*: Source code, comments, doc-comments, documentation, commit messages, PR descriptions, and issue text must be in English.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| **User-driven thoroughness beats agent-driven thoroughness when the agent has been at it for a while.** The maintainer's question "kabul edilen bulguların tamamı düzeltildi mi?" (are all accepted findings actually fixed?) caught the silently-skipped runtime EL check — a real correctness gap. The agent had self-reported "all accepted findings fixed"; the maintainer's verification surfaced that this was wrong. This is the same lesson as the T-006 retro's "post-In-Review second-read" gate, applied one level deeper: the *fix-up commits* themselves need a verification gate. The user is providing this manually today; whether to automate it (e.g. an "all-findings-actually-applied" agent pass) is open. | |
| **User-driven thoroughness beats agent-driven thoroughness when the agent has been at it for a while.** The maintainer's question "were all accepted findings actually fixed?" caught the silently-skipped runtime EL check — a real correctness gap. The agent had self-reported "all accepted findings fixed"; the maintainer's verification surfaced that this was wrong. This is the same lesson as the T-006 retro's "post-In-Review second-read" gate, applied one level deeper: the *fix-up commits* themselves need a verification gate. The user is providing this manually today; whether to automate it (e.g. an "all-findings-actually-applied" agent pass) is open. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/analysis/reviews/business-reviews/2026-04-27-T-009-mini-retro.md` at
line 74, Replace the Turkish quotation "kabul edilen bulguların tamamı
düzeltildi mi?" in the document with its English equivalent to comply with the
repo language policy; specifically change that exact phrase to "Have all
accepted findings actually been fixed?" so the sentence remains the same
semantics but is fully in English (update the quoted text wherever it appears in
the file).
| | 2026-04-23 | @cemililik (+ Claude Opus 4.7 agent) | opened with status `In Progress`. Scope deliberately narrow: measurement only; deadline arming + WFI activation belong to a follow-up IRQ-wiring task. ADR-0022's first rider stays open. `current.md` will be updated to point at T-009 in the same commit. | | ||
| | 2026-04-23 | @cemililik (+ Claude Opus 4.7 agent) | Implementation complete. Three commits landed: `beb0963` (`QemuVirtCpu` Timer impl + UNSAFE-2026-0015 audit entry; `frequency_hz` and `resolution_ns` cached at `new()`; `arm_deadline` / `cancel_deadline` `unimplemented!()` with explicit deferral messages), `55f2d10` (BSP boot-to-end instrumentation: `BOOT_NS` snapshot + `tyrne: timer ready` banner + `tyrne: boot-to-end elapsed` final line). Verification: 111 host tests green, miri 111/111 clean, fmt/host-clippy/kernel-clippy clean, kernel-build clean, QEMU smoke shows new lines bracketing the unchanged A6 trace. Documentation sweep: ADR-0022 sub-rider clarifying T-009 (time source) vs. future IRQ-wiring task (IRQ delivery); glossary +3 entries (`CNTPCT_EL0`, `CNTFRQ_EL0`, `Generic Timer`); README status + identity lines refreshed; phase-b.md + task index + current.md status flipped. Status → `In Review`. | | ||
| | 2026-04-23 | @cemililik (+ two independent review agents) | Second-read review surfaced three Yüksek findings, all addressed in this commit arc. (1) **Register family**: ADR-0010's *References* and ADR-0022's first-rider sub-rider both name the **virtual** family (`CNTVCT_EL0`, `CNTV_*`); the original implementation read `CNTPCT_EL0` and would have silently mismatched the deferred deadline-arming side once `CNTVOFF_EL2 ≠ 0`. Switched the read to `CNTVCT_EL0`; UNSAFE-2026-0015 gains an Amendment recording the change. (2) **Arithmetic correctness**: `count * resolution_ns` (with `wrapping_mul`) was *not* nanoseconds on non-divisor frequencies — at 19.2 MHz, drift ≈ 0.16 % ≈ 138 s/day; *and* `wrapping_mul` would silently break ADR-0010 monotonicity at the wrap edge. Conversion extracted to `tyrne_hal::timer::ticks_to_ns`, which uses 128-bit intermediate arithmetic and a saturating cast back to `u64`. Pure helpers (`ticks_to_ns`, `resolution_ns_for_freq`, `NANOS_PER_SECOND`) live in the HAL crate so host unit tests can exercise them without inline asm — 13 new tests in `hal/src/timer.rs` cover the QEMU-virt-divisor, Pi-3-class non-divisor, 1 GHz, saturation, and round-to-nearest cases. (3) **Audit-policy compliance**: UNSAFE-2026-0006's body still claimed "zero-size type with no fields"; T-009 had silently rewritten the in-source SAFETY comment without amending the audit entry. Appended an Amendment recording the post-T-009 struct shape. Also corrected: SAFETY comments now cite ADR-0012's "QEMU virt delivers at EL1, non-VHE" precondition rather than asserting "unconditionally available at EL1"; the overflow-margin comment that said "18 millennia at QEMU virt" was arithmetically wrong (the answer is ~584 years independent of frequency since `count * resolution_ns ≈ elapsed_ns ≤ u64::MAX`); glossary's Pi 4 frequency claim softened (BCM2711 mainline rate is 54 MHz, not the 19.2 MHz Pi 3 figure originally written) — BSPs read firmware regardless. Verification after fixes: 77 kernel + 13 hal + 34 test-hal = **124 host tests green**, miri 124/124 clean, all clippy/fmt/build gates clean, QEMU smoke unchanged shape. | | ||
| | 2026-04-27 | @cemililik (+ Claude Opus 4.7 agent) | Self-audit of the 2026-04-23 second-read fix-up commit (`39fb66c`) found that Review 1's Yüksek #1 was only **half** addressed: the documentation half (ADR-0012 cite, EL precondition wording, removal of "unconditional" claim) landed correctly, but the *runtime check* the recommendation also asked for ("(b) ... and add a runtime check that panics with a clear error if not") was silently skipped. Closed by adding a `MRS CurrentEL` boot-time self-check at the head of `QemuVirtCpu::new`, asserting EL == 1 with a named panic message ("must run at EL1 per ADR-0012; observed EL{n} instead"). One new audit entry — **UNSAFE-2026-0016** — covers the new MRS in the same shape as UNSAFE-2026-0007 (read-only system register at EL≥1, no state mutation). Acceptance criteria and DoD remain met: 124 host tests green, miri clean, all clippy/fmt/build gates clean, QEMU smoke shows the assertion path passes (we are at EL1) and the boot trace continues unchanged. T-009's full Review 1 + Review 2 finding-set is now closed; the lesson — second-read review-recommendations have AND/OR structure, partial accept = unfinished — is worth carrying into the next task's DoD checklist. | |
There was a problem hiding this comment.
Minor: host-test count appears stale relative to the final PR state.
Line 110 records "124 host tests" after the EL1 self-check commit. The PR's commit summary mentions a follow-up commit adding "two new monotonicity tests in hal/src/timer.rs", and the T-007 close-out review (Line 121 of T-007-idle-task-typed-deadlock.md) reports 130 host tests. Counting #[test] functions in hal/src/timer.rs confirms 19 HAL tests (77 + 19 + 34 = 130). Consider updating this row to reflect the final 130 count, or adding a follow-up review-history row that absorbs the two new monotonicity tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/analysis/tasks/phase-b/T-009-timer-init-cntvct.md` at line 110, Update
the post-commit audit row in T-009-timer-init-cntvct.md that currently states
"124 host tests" to reflect the final PR test count of 130 (the PR added two
monotonicity tests in hal/src/timer.rs); alternatively add a short follow-up
review-history row documenting the follow-up commit that added the two
monotonicity tests and increased the host-test total from 124 to 130 so the doc
matches the hal/src/timer.rs test counts (19 HAL tests: 77 + 19 + 34 = 130).
|
|
||
| [ADR-0013](0013-roadmap-and-planning.md) settled the project's roadmap-and-planning process on 2026-04-20. Its §"Integration with ADRs" said: *"The roadmap does not replace ADRs. It sequences them. A task may require an ADR as an acceptance criterion; an ADR may result in new tasks being added."* That single paragraph was the entire normative content for how ADRs interact with the planning process. | ||
|
|
||
| The Phase A → B0 implementation arc — six days of work spanning T-006 / T-007 / T-009 — produced four ADRs (ADR-0021, ADR-0022) that needed post-Accept riders within their first week, plus one (ADR-0021) that needed a mid-proposal revision before Accept. Each rider's content traced back to one of two implicit rules that ADR-0013's framing had not made explicit: |
There was a problem hiding this comment.
Fix entity/count mismatch in historical statement.
Line 11 says “four ADRs (ADR-0021, ADR-0022)”, but only two ADRs are listed. Please reword to match the actual count (e.g., four riders across two ADRs).
Proposed doc fix
-The Phase A → B0 implementation arc — six days of work spanning T-006 / T-007 / T-009 — produced four ADRs (ADR-0021, ADR-0022) that needed post-Accept riders within their first week, plus one (ADR-0021) that needed a mid-proposal revision before Accept. Each rider's content traced back to one of two implicit rules that ADR-0013's framing had not made explicit:
+The Phase A → B0 implementation arc — six days of work spanning T-006 / T-007 / T-009 — produced four rider events across two ADRs (ADR-0021, ADR-0022): post-Accept riders within their first week, plus one mid-proposal revision before Accept on ADR-0021. Each rider's content traced back to one of two implicit rules that ADR-0013's framing had not made explicit:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| The Phase A → B0 implementation arc — six days of work spanning T-006 / T-007 / T-009 — produced four ADRs (ADR-0021, ADR-0022) that needed post-Accept riders within their first week, plus one (ADR-0021) that needed a mid-proposal revision before Accept. Each rider's content traced back to one of two implicit rules that ADR-0013's framing had not made explicit: | |
| The Phase A → B0 implementation arc — six days of work spanning T-006 / T-007 / T-009 — produced four rider events across two ADRs (ADR-0021, ADR-0022): post-Accept riders within their first week, plus one mid-proposal revision before Accept on ADR-0021. Each rider's content traced back to one of two implicit rules that ADR-0013's framing had not made explicit: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/decisions/0025-adr-governance-amendments.md` at line 11, The sentence
currently states “four ADRs (ADR-0021, ADR-0022)” which mismatches the listed
ADRs; change the phrasing to reflect that there were two ADRs (ADR-0021,
ADR-0022) that produced four riders—e.g., replace with “four riders across two
ADRs (ADR-0021, ADR-0022)” or “two ADRs (ADR-0021, ADR-0022) which produced four
riders”; ensure the surrounding mention of ADR-0013’s framing still reads
correctly with the new wording.
| | ADR-0029 | Initial syscall set | B5 | was ADR-0026 | | ||
| | ADR-0024 | EL drop policy | B1 (Accepted 2026-04-27) | was ADR-0021 in the pre-review plan | | ||
| | ADR-0025 | ADR governance amendments (forward-reference, riders) | meta-process (Accepted 2026-04-27) | new — captures the rules T-006/T-009 retros surfaced; not B-phase content. Cool-down rule withdrawn pre-Accept; see ADR-0025 §Revision notes | | ||
| | ADR-0026 | Exception-vector-table / handler-dispatch shape (T-012, conditional) | B1 | reserved by T-012 if non-obvious choices arise; may go unused if T-011 absorbs the exception-vector design | |
There was a problem hiding this comment.
Correct task reference in ADR-0026 note (T-011 → T-012).
Line 242 references T-011 for exception-vector design absorption, but this file consistently scopes exception infrastructure under T-012. This should point to T-012 to avoid roadmap confusion.
Proposed doc fix
-| ADR-0026 | Exception-vector-table / handler-dispatch shape (T-012, conditional) | B1 | reserved by T-012 if non-obvious choices arise; may go unused if T-011 absorbs the exception-vector design |
+| ADR-0026 | Exception-vector-table / handler-dispatch shape (T-012, conditional) | B1 | reserved by T-012 if non-obvious choices arise; may go unused if T-012 absorbs the exception-vector design |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | ADR-0026 | Exception-vector-table / handler-dispatch shape (T-012, conditional) | B1 | reserved by T-012 if non-obvious choices arise; may go unused if T-011 absorbs the exception-vector design | | |
| | ADR-0026 | Exception-vector-table / handler-dispatch shape (T-012, conditional) | B1 | reserved by T-012 if non-obvious choices arise; may go unused if T-012 absorbs the exception-vector design | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/roadmap/phases/phase-b.md` at line 242, Update the ADR-0026 note to
reference the correct task: change the dangling "T-011" mention to "T-012" so
the clause reads that ADR-0026 "may go unused if T-012 absorbs the
exception-vector design"; locate the ADR-0026 row (text containing
"Exception-vector-table / handler-dispatch shape (T-012, conditional)") and
replace the incorrect task symbol T-011 with T-012 to keep the roadmap
consistent.
7 tasks
5 tasks
This was referenced May 6, 2026
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>
9 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
Implement a monotonic ARM Generic Timer–based time source for the QEMU virt BSP and wire it into the kernel demo, while adding shared timing utilities, audit entries, and roadmap/docs updates.
New Features:
Timertrait implementation toQemuVirtCpuusing the ARM Generic Timer's virtual counter and cached frequency.Enhancements:
QemuVirtCputo cache generic-timer frequency and resolution, and expose the frequency for diagnostics.QemuVirtCpu's Send/Sync guarantees and timer-related system-register access.CurrentELself-check to enforce the expected EL1 execution environment for timer initialisation.Documentation:
QemuVirtCpushape.Tests:
Chores:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation