fix(qemu-arm64): implement generic timer-backed tick - #102
Conversation
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#102 "fix(qemu-arm64): implement generic timer-backed tick"
head: 92d85ac author: ata-sesli ci: none reported
Verdict: Real bug, right mechanism, and the arithmetic is correct including the 32-bit wraparound — I checked it rather than trusting the constants. Four things to fix: a divide with no zero guard, inline asm with no architecture guard, no CI compiles the file at all, and the conversion is architecture logic sitting in a board directory that §7.1 specifically says not to copy per board.
Credit where it is due on claim discipline: "Full CTest suite run" is left unchecked with the reason given, and the reason is true — I built origin/master (22d8f8b, which is this PR's base) from a git archive snapshot and it fails with 12 errors at include/eos_image.h:135/:142. The claims that are ticked hold: gcc -Wall -Wextra -Werror -I boards/qemu_arm64 tests/unit/test_qemu_arm64_timer.c builds clean and prints 4/4 tests passed.
I verified the wraparound is genuinely correct, not accidentally so: (uint32_t)(counter / frequency) * 1000U truncates the seconds before the multiply, and ((S mod 2^32) * 1000) mod 2^32 == (S * 1000) mod 2^32, so the result is the true millisecond count mod 2^32. The 4294967296123 / 1000 case checks out by hand at 123, and remainder * 1000U cannot overflow because remainder < frequency <= UINT32_MAX.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | boards/qemu_arm64/qemu_arm64_timer.h:50–51 |
No zero guard on the divisor. CNTFRQ_EL0 is not architecturally guaranteed to hold a value — it is programmed by the highest exception level's firmware and reads whatever was written, which is 0 if nothing did. eBoot is early boot code, so this is a real input, not a theoretical one. Verified on the host: qemu_arm64_counter_to_ms(123456789, 0) dies with SIGFPE, exit 136. On AArch64 UDIV by zero does not trap and yields 0, so the target instead returns a constant 0 tick — exactly the bug this PR fixes — with no signal that the timer was unreadable. .ai/security.md: "A verification step that cannot run must fail, not pass"; a target without hardware support "degrades to a documented, weaker posture — it does not silently pretend to have the strong one." |
Guard the divide. eos_board_ops_t.get_tick_ms returns uint32_t with no error channel, so the honest form is a guard plus a loud degradation: if (frequency == 0) return 0; in the helper, and in qemu_get_tick_ms fall back to QEMU virt's known 62,500,000 Hz and record the fallback via the boot log rather than returning a stuck tick silently. Add a frequency == 0 test case. |
| 2 | Medium | boards/qemu_arm64/board_qemu_arm64.c:59–62 |
Unguarded inline asm. Verified: gcc -c -I include -I boards/qemu_arm64 boards/qemu_arm64/board_qemu_arm64.c on this x86-64 host now fails — Error: no such instruction: 'mrs %rax,cntfrq_el0', 'isb', 'mrs %rax,cntvct_el0'. The established pattern in this same tree guards it: boards/rpi4/board_rpi4.c:153 and :160 wrap msr daifset/daifclr in #if defined(__aarch64__). |
#if defined(__aarch64__) around the two mrs reads, with an #else that returns 0 (or the documented fallback from finding 1) so the file still compiles for a non-AArch64 target. |
| 3 | Medium | — (CI) | Nothing compiles this file. CMakeLists.txt:205–206 builds board_qemu_arm64.c only under EBLDR_BOARD=qemu_arm64; pull-request CI configures stm32f4 (.github/workflows/ci.yml:107) and none (build.yml:27), and release.yml covers rpi4, riscv64_virt, esp32, esp32c3 and x86_64_efi — never qemu_arm64. So the only part of this PR that touches the device is compiled by no job, and the new unit test covers the pure helper, not qemu_get_tick_ms. On top of that, this PR has zero checks reported (gh pr checks 102 → "no checks reported on the 'fix/qemu-arm64-tick' branch", statusCheckRollup length 0) — likely the first-time-contributor workflow-approval gate. |
A maintainer needs to approve the workflow run. Separately, add a cross-compile leg that configures EBLDR_BOARD=qemu_arm64 with an aarch64 toolchain, or this board's code stays in the same "never compiled" state that #84 is currently fixing for core/fdt_loader.c. |
| 4 | Medium | boards/qemu_arm64/qemu_arm64_timer.h (new file) |
Architecture logic placed in a board directory. Master design §7.1: "Architecture and SoC logic must not be copied independently into every board." Generic-Timer counter→ms conversion is AArch64 architecture logic; nothing in it is qemu-virt-specific. And the copy is already predictable: boards/rpi4/board_rpi4.c:143–147 is static uint32_t rpi4_get_tick_ms(void) { /* ARM generic timer: CNTPCT_EL0 / CNTFRQ_EL0 */ return tick_ms; } — the same never-advancing tick with a comment naming the same intended implementation — and imx8m/am64x return a static tick_ms too. .ai/architect.md: hal/ is hardware abstraction with no board specifics, boards/ holds board and SoC specifics, "a board file is the leaf, never the trunk". |
Cheapest correct sequence, and it is cheap because the file is new: put the helper in a shared AArch64 location (hal/, or an arch header under include/) in this PR rather than under boards/qemu_arm64/, and drop the target_include_directories(... ../boards/qemu_arm64) from tests/CMakeLists.txt — a test reaching into a board directory is the coupling that makes the next move expensive. Then a follow-up converts rpi4, imx8m and am64x, which is where the fix has actual users (rpi4 is built by release.yml:80). |
| 5 | Low | tests/unit/test_qemu_arm64_timer.c:123–126 |
The two large expectations (1271310319U, 123U) are hand-computed constants with no derivation, so if the arithmetic were wrong the constant would have been computed from the same wrong arithmetic. They happen to be right — I checked — but the test as written pins the implementation, not the specification. Missing cases: frequency == 0 (finding 1), and the frequency QEMU virt actually reports (62,500,000); the suite only uses 1,000 and 1,000,000. |
One comment per case showing the expected value's derivation, plus a 62.5 MHz case and a zero-frequency case. |
| 6 | Low | tests/CMakeLists.txt:123–130 |
Adding test_qemu_arm64_timer to the hand-maintained Valgrind foreach list collides with #95 ("test: derive the suite totals and the Valgrind list instead of restating them"), which replaces that list wholesale. It also buys nothing: the test allocates nothing, so --leak-check=full on it costs a Valgrind run per CI cycle to check a pure function. |
Leave it out of the Valgrind list, and rebase behind #95 if #95 lands first. |
Architecture conformance
Deviates on §7.1, per finding 4: architecture-level Generic Timer logic has been placed under boards/<board>/ where the design says architecture and SoC logic must not be copied into each board, and three other AArch64 boards in the tree carry the same stub waiting for the same helper. Everything else conforms — Tier 1 Foundation (§21), no dependency pointing up a tier (§5.1): board_qemu_arm64.c includes only board_qemu_arm64.h, eos_board_registry.h, the new local header and <string.h>, and the board library links eboot_hal only. §22 is also relevant: qemu_arm64 is an Experimental/simulation target with no hardware CI, and this PR does not claim otherwise.
No architecture proposal is warranted — the design text is correct and this change is on the wrong side of it. §7.1 already says what should happen.
Proposed changes
Smallest sequence that keeps everything building:
- In this PR:
if (frequency == 0) return 0;in the helper;#if defined(__aarch64__)around the twomrsreads with a non-AArch64#else; moveqemu_arm64_timer.hout ofboards/qemu_arm64/into a shared AArch64 header and update the include inboard_qemu_arm64.candtests/CMakeLists.txt; add the zero-frequency and 62.5 MHz test cases; drop the Valgrind list entry. - Get the workflow run approved so this branch reports checks.
- Follow-up PR: an
EBLDR_BOARD=qemu_arm64cross-compile leg inci.yml. - Follow-up PR: convert
rpi4,imx8mandam64xto the shared helper —rpi4ships fromrelease.ymlwith a tick that never advances.
Not checked
- The asm itself: NOT RUN and NOT COMPILED for its target. No
aarch64toolchain on this host, so I did not cross-compileboard_qemu_arm64.c, and I did not boot it under QEMU. The claim thatCNTFRQ_EL0/CNTVCT_EL0are readable at EL1 on QEMU virt, and thatUDIVby zero returns 0 rather than trapping on AArch64, are inferred from the architecture reference, not measured here. The SIGFPE result in finding 1 is measured, on x86-64. isbplacement: readingCNTVCT_EL0after anISBis the self-synchronising sequence the ARM ARM prescribes and it looks right, but I have not verified it produces a monotonic tick on real or emulated hardware. There is no test that callsqemu_get_tick_ms()at all.ctest: NOT RUN. The base (22d8f8b) does not compile, so the suite cannot run on this branch — which is what the PR body says. I built and ran only the new test in isolation.- Cross builds and static analysis: NOT RUN, and there is no CI to fall back on (finding 3).
- I did not check whether
imx8m/am64x'stick_msis incremented by an interrupt handler somewhere; I only observed that the accessor returns a static variable.rpi4's is a barereturn tick_ms;under a comment naming an unimplemented mechanism, which is what finding 4 leans on. - The local clone does not have
92d85acf; everything above is from agh api tarballsnapshot at that sha.
Automated architecture review of 92d85acf4f18 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.
Summary
Replace the QEMU ARM64 board's constant-zero tick with a monotonic
millisecond tick backed by the AArch64 Generic Timer.
Returning zero prevented tick-based delays and timeouts from progressing
when running with the QEMU ARM64 board operations.
Type of Change
Changes
CNTFRQ_EL0andCNTVCT_EL0to obtain the timer frequency andcurrent virtual counter.
32-bit tick wraparound.
and 32-bit wraparound cases.
Testing
-Wall -Wextra -Werrorgit diff --check HEAD^ HEADpassedThe full suite was not run because the current upstream
masterhasunrelated pre-existing build failures.
Pre-Submission Checklist
masterRelated Issues
None.