Don't trust CPUID leaf 0x15 for the TSC frequency inside a hypervisor guest - #31295
Don't trust CPUID leaf 0x15 for the TSC frequency inside a hypervisor guest#31295robobun wants to merge 1 commit into
Conversation
WalkthroughImplements CPUID-driven x64 TSC frequency detection (hypervisor-first, CPUID 0x15 fallback, OS-clock fallback), exposes a public resolver and calibration snapshot in Rust, adds test-only JS bindings, and includes tests that validate resolver plausibility and end-to-end calibration. ChangesTSC Frequency Detection with Hypervisor Support
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this changes low-level TSC calibration logic with hypervisor-specific CPUID handling (leaf 0x4000_0010 trust, plausibility bounds) — worth a human look given it will eventually drive all timer deadlines.
Extended reasoning...
Overview
This PR refactors src/perf/hw_timer.rs to stop trusting CPUID leaf 0x15 for the TSC frequency when running inside a hypervisor guest, instead reading the hypervisor timing leaf 0x4000_0010 (with a [100 MHz, 10 GHz] plausibility check) or falling back to the OS clock. The decision is factored into a pure resolve_x64_tsc_frequency(X64TscCpuidInfo) function, and a new calibration_snapshot() plus hwTimerInternals JS bindings (wired via dispatch_js2native.rs, with bun_runtime gaining a bun_perf dep) expose it to a new test file. Six files changed.
Security risks
None. No untrusted input handling, auth, crypto, or network surface — just CPUID reads and arithmetic on values the CPU/VMM hands back.
Level of scrutiny
Medium-high. Although the PR notes hw_timer isn't yet wired into the live timer path in the Rust port, this code is intended to drive every now_ns()-derived deadline once it lands. The correctness of the hypervisor heuristic (only trusting 0x4000_0010, ignoring leaf 0x15 entirely under the HV bit, the chosen plausibility bounds) is a design decision with platform-wide implications across KVM/VMware/Hyper-V/Xen guests, and benefits from review by someone with domain context on how those VMMs populate these leaves.
Other factors
- Bare-metal behavior is preserved (leaf 0x15 path unchanged), and the fallback is conservative (OS clock), so the risk profile is "may be slower in some VMs" rather than "may mis-calibrate worse."
- Test coverage is good: synthetic CPUID cases plus an end-to-end measured-vs-calibrated check with a 20% tolerance over 150 ms.
- CI (Build #57383) is currently reporting failures on the latest commit.
- The new
bun_perfdependency onbun_runtimeand the codegen-mangled binding names indispatch_js2native.rsfollow existing patterns in that file.
0d075d1 to
39e01cf
Compare
There was a problem hiding this comment.
Thanks for dropping the unsafe wrapper — the Windows path looks correct now. I didn't find any further issues, but the hypervisor-vs-leaf-0x15 decision logic and the multi-platform #[cfg] branches in hw_timer.rs are substantive enough that a human should sign off on the design.
Extended reasoning...
Overview
This PR reworks TSC frequency detection in src/perf/hw_timer.rs (~190 net new lines): it gates on invariant TSC, detects the hypervisor bit (CPUID.1:ECX[31]), reads the VMware/KVM timing leaf 0x4000_0010 when present, and refuses to trust leaf 0x15 inside a guest. The decision is factored into a pure resolve_x64_tsc_frequency(X64TscCpuidInfo) plus a calibration_snapshot() helper, both exposed to JS via two new pub(crate) bindings in src/runtime/dispatch_js2native.rs and hwTimerInternals in internal-for-testing.ts. bun_runtime gains a new workspace dep on bun_perf, and a 6-case test file exercises synthetic CPUID combinations plus an end-to-end calibration check on the host machine.
Prior feedback
My only earlier finding (the unused_unsafe lint on the Windows QPC/QPF block, which would have failed the x86_64-pc-windows-msvc build under -D warnings) was fixed in 37e5b1f — the current os_monotonic_ns() calls bun_sys::windows::QueryPerformanceCounter/Frequency directly with an explanatory comment. The bug-hunting pass on this revision found nothing new.
Security risks
None identified. The new code reads CPUID leaves, rdtsc/CNTVCT_EL0, clock_gettime, and QPC — all read-only, side-effect-free, and not influenced by attacker-controlled input. The JS bindings are gated behind bun:internal-for-testing and only marshal numbers/booleans into a struct.
Level of scrutiny
This is low-level, performance-adjacent timing infrastructure with per-arch and per-OS #[cfg] branches (aarch64, x86_64 × macOS/FreeBSD/Linux/Windows), inline asm, and FFI. While hw_timer is not yet wired into the live timer tick path in this tree (per the PR description), it is intended to be, and the core change is a policy decision — never trust leaf 0x15 under a hypervisor, prefer leaf 0x4000_0010 with a [100 MHz, 10 GHz] plausibility window, otherwise fall back to the OS clock. That's a reasonable design and well-argued in the description, but it's the kind of judgment call (along with the new bun_runtime → bun_perf dep edge and the PORT NOTE: diverges from the Zig spec annotation) that a maintainer should ratify rather than a bot.
Other factors
Test coverage is good: the synthetic-CPUID tests pin down each branch of resolve_x64_tsc_frequency, and the end-to-end test compares the chosen frequency against a measured rate over 150 ms with a 20% tolerance (vs. the 2.8× failure mode). The dispatch bindings follow the existing pub(crate) convention from #31254 and the bun_get_use_system_ca pattern in the same file.
There was a problem hiding this comment.
Thanks for addressing the cross-target lint issues — all three prior comments are resolved and I found nothing new. Deferring to a human for the substantive bits: the hypervisor clock-source trust model and the platform-gated FFI/asm paths are the kind of timing-infrastructure decision worth a second pair of eyes.
Extended reasoning...
Overview
This PR reworks TSC frequency detection in src/perf/hw_timer.rs so that CPUID leaf 0x15 is no longer trusted inside a hypervisor guest (where it can describe the host crystal rather than the guest's scaled rdtsc rate, causing a ~2.8× timer skew on some GCP/KVM hosts). It adds a pure resolve_x64_tsc_frequency() decision function plus a calibration_snapshot() helper, wires both through new bun:internal-for-testing bindings in dispatch_js2native.rs (adding a bun_runtime → bun_perf dependency), and ships a 6-case test file. Roughly 250 lines of new Rust across many #[cfg(...)] branches (aarch64/x86_64 × Linux/Windows/macOS/FreeBSD) including inline asm, cpuid, sysctlbyname, clock_gettime, and QPC.
Security risks
None apparent. The new surface reads CPUID/clock registers and exposes results only via bun:internal-for-testing, which is CI/debug-gated. No user-controlled input reaches the FFI paths.
Level of scrutiny
Moderate-to-high. The module is not yet wired into the live timer tick path in this tree (per the PR description), so immediate blast radius is limited to the test bindings. But this is timing infrastructure that will drive setTimeout deadlines once wired, and the core change is a policy decision — which CPUID sources to trust under a hypervisor, what plausibility bounds to apply, and when to fall back to the OS clock. That, plus the density of platform-gated unsafe FFI (which already needed two follow-up commits for cross-target lints that local checks missed), makes this worth a human reviewer rather than a bot approval.
Other factors
- All three of my prior inline comments (Windows
unused_unsafe, macOS/FreeBSDborrow_as_ptr, stalenow_ns()doc reference) were addressed in 37e5b1f and c991b41; the current diff reflects those fixes. - The bug-hunting system found no new issues on the latest revision.
- Good test coverage: the decision function is exercised with synthetic CPUID values for the exact failure shape plus edge cases, and an end-to-end check compares the chosen frequency against the measured counter rate on the CI host.
|
CI status for the latest commit (c991b41, Buildkite build #57730): every build lane and every test lane that ran is green — including the Windows build-rust lanes and all Linux/Alpine/FreeBSD/macOS-x64 test suites. The only two red entries, So the remaining red is runner availability, not a code failure. From my side the PR is complete: fix, tests, and review feedback (Windows |
4e3cf8e to
05d1d99
Compare
|
Update on the latest CI run (build #60251, commit 05d1d99): the red test lanes all trace to one file — Verified locally that this is not from this PR: the identical 4 tests fail the same way on plain So the bunx failures are main/environment-borne (those tests hit GitHub/the registry). Once that's fixed on main this PR can rebase or merge as-is at maintainer discretion — everything specific to this diff is green. |
05d1d99 to
4766495
Compare
… guest Inside a VM the TSC the guest reads may be scaled or emulated, so leaf 0x15 (which describes the host part's crystal) need not match the rate rdtsc actually ticks at; on some GCP/KVM hosts it is off by ~2.8x, which skews every deadline derived from a clock calibrated with it. Under a hypervisor, only trust the timing leaf 0x4000_0010 the hypervisor itself publishes (guest TSC kHz, TSC-scaling aware); otherwise fall back to the OS clock. Bare metal keeps the leaf 0x15 path. hw_timer gets the frequency-resolution layer back in its corrected form (read_frequency + a pure resolve_x64_tsc_frequency + calibration_snapshot); the unwired now_ns/now_ms/calibrate machinery stays out. The decision logic and a counter/OS-clock snapshot are exposed through bun:internal-for-testing (hwTimerInternals) so tests can cover the hypervisor/bare-metal combinations and verify the chosen frequency tracks the OS monotonic clock on the machine running the test.
4766495 to
cc55cd9
Compare
|
Closing: main has removed the code this PR was fixing.
Rebasing would mean re-creating a deleted module purely to host test bindings for logic with no production caller, so there is nothing left to land here. If a calibrated-TSC fast path is ever reintroduced, the hypervisor handling from this PR (never trust leaf 0x15 under CPUID.1:ECX[31]; use the hypervisor timing leaf 0x4000_0010 or fall back to the OS clock) is the piece it needs, and this branch remains as a reference. |
1 participant
What does this PR do?
Fixes the TSC mis-calibration in
hw_timerthat skews timer deadlines inside hypervisor guests.Report (against the shipped Zig build, which
src/perf/hw_timer.zig/hw_timer.rsimplement): on some GCP Cloud Run (KVM) hosts,setTimeout(500)fires at ~1400 ms — exactly 2.80× the requested delay, locked in at process start — whileDate.now/performance.now/hrtimestay correct on the same instance.Cause:
read_frequency()trusts CPUID leaf 0x15 whenever invariant TSC is advertised. Inside a VM that leaf describes the host part's crystal ratio leaked through the VMM; with TSC scaling the guest'srdtscticks at a different rate, sonow_ns()is calibrated with the wrong frequency and every deadline derived from it is off by a constant factor (2.8× on the affected hosts). Every other clock usesclock_gettime, which the guest kernel calibrates correctly — hence the observed divergence.Fix (
src/perf/hw_timer.rs, the Rust port of the module; the non-compiled.zigreference is left untouched per porting conventions):0x4000_0010(VMware interface, also implemented by KVM), which publishes the guest TSC rate in kHz and accounts for TSC scaling — with a plausibility range check. Otherwise return 0 sonow_ns()stays on the OS clock (vDSO/QPC), i.e. the pre-hw_timerbehavior.resolve_x64_tsc_frequency(X64TscCpuidInfo)so it can be exercised with synthetic CPUID values, andcalibration_snapshot()exposes the chosen frequency plus a counter/OS-clock sample pair. Both are reachable from JS via a newhwTimerInternalsentry inbun:internal-for-testing(wired throughdispatch_js2native.rs;bun_runtimenow depends onbun_perf).Note: in the Rust port,
hw_timeris not yet wired intoTimespec::now/ the timer tick path (timers currently readclock_gettimedirectly), so this fixes the module before that wiring lands rather than a live regression in this tree.Verification
test/js/bun/perf/hw-timer-tsc-frequency.test.ts:the exact failure shape from the report: hypervisor bit set + populated leaf 0x15 + no timing leaf → must resolve to 0 (OS fallback), not the leaf 0x15 value;
hypervisor timing leaf wins over leaf 0x15 when both are present; implausible timing-leaf values (0, 1 kHz, ~4.3 THz) are rejected;
bare-metal leaf 0x15 math unchanged, partially-populated leaf 0x15 still falls back;
end-to-end on the machine running the test: if a non-zero frequency is chosen, it must match the counter rate measured against the OS monotonic clock over 150 ms (on the affected GCP hosts the old code calibrates to ~2.8× the measured rate).
bun bd test test/js/bun/perf/hw-timer-tsc-frequency.test.tswith the fix: 6 pass.Same command with
src/reverted to the merge base: all 6 tests fail (thehwTimerInternalsbinding and the hypervisor-aware decision it exposes don't exist there).cargo check/cargo clippy --no-depsclean forbun_perfandbun_runtime.This container is itself a KVM guest (Xeon 8375C) where leaf 0x15 is zeroed and leaf 0x4000_0010 reports 2899987 kHz: the end-to-end test exercises the new hypervisor-leaf path for real and the measured rate agrees within measurement noise.
Rebase notes (after #31783)
Rebased onto current main after #31783 ("Resolve the audited TODO(port)/TODO(b2)/PORT NOTE backlog"), which removed PORT NOTE comments,
ported fromtrailers, and the.zigsection headers this PR's hunks sat next to. Resolution: kept the same code, reworded the three PORT NOTE comments as plain why-comments (the leaf-0x15 divergence rationale and the rbx/cpuid intrinsic note), dropped the historical porting narration inos_monotonic_ns()and the file trailer, and dropped the section-header line indispatch_js2native.rsto match the file's new style. No functional changes; all six tests pass on the rebased tree.Rebase notes (after #31746)
Rebased again after #31746 ("Remove stale TODO(port) comments and delete dead code"), which reduced
hw_timer.rsto justread_counter()— deleting the#[allow(dead_code)]cpuid/sysctlbynamehelpers and theircore::ffiimport. This PR re-adds those helpers (now with live callers throughread_frequency()), restores the cfg-gated import, and merges cleanly around the newlinearFifotest bindings that landed nearby indispatch_js2native.rs/internal-for-testing.ts. History squashed to a single commit; all six tests pass on the rebased tree.Rebase notes (after #31254)
mainmerged #31254, which removed the unwirednow_ns()/now_ms()/calibrate()machinery fromhw_timer.rs(keeping onlyread_counter()and the cpuid/sysctl helpers) and switched locally-defined dispatch functions topub(crate). This PR was rebased onto that shape rather than mechanically restoring the old file:read_frequency()(hypervisor-aware), the pureresolve_x64_tsc_frequency(),calibration_snapshot(), andos_monotonic_ns()as its anchor — all of which have live callers through thehwTimerInternalstesting bindings. Thenow_ns/calibratecode that Restrict crate-internal items to pub(crate) and remove dead code it exposes #31254 deleted as dead stays out until the tick-path wiring lands.#[allow(dead_code)]markers Restrict crate-internal items to pub(crate) and remove dead code it exposes #31254 added toCpuidResult/cpuid/sysctlbynameare dropped becauseread_frequency()uses them again.pub(crate)convention introduced there.