From 2dda61539f2440e2e75204e2a73be57e1c6c08a1 Mon Sep 17 00:00:00 2001 From: Robert Nowotny Date: Thu, 20 Aug 2026 16:11:17 +0200 Subject: [PATCH 1/3] virt_kvm: restart the guest tsc on a machine reset A machine reset returns the partition's reference time counter to zero, because `reset_all` sets every state element to its at-reset value and `ReferenceTime`'s is zero. It asks the same of each vp's `Tsc`, whose at-reset value is also zero, and on KVM that half silently does not happen. The `Tsc` element is written through `MSR_IA32_TSC`, and `kvm_synchronize_tsc` (arch/x86/kvm/x86.c) reads a host write of exactly zero as "userspace is creating or synchronizing this vcpu" rather than as a value to store: it sets `synchronizing = true` on that branch, comment "Force synchronization when creating a vCPU, or when userspace explicitly writes a zero value", and then substitutes `kvm->arch.cur_tsc_offset` for whatever offset the write implied. So zero, the one value a reset needs to deliver, is the one value that path cannot deliver. Measured on a live guest: TSC 5403506220, wrote 0, read back 5403522705. A control write of 0x4000000000000000 landed, so the write itself was reaching the kernel. The guest is then left holding two clocks that disagree by the previous boot's uptime. A guest hypervisor calibrates its reference clock off the TSC and arms a one-shot synthetic timer at an absolute deadline on the old timeline (38.50 s measured) while the partition counter reads 2.24 s. The deadline is about 36 real seconds away, the clock init polls for it with a bounded budget, retires the timer when it never fires, runs out of candidates and bugchecks, which presents as a hang at the firmware logo. Reset the counter for real, through the vcpu device attribute `KVM_VCPU_TSC_CTRL`/`KVM_VCPU_TSC_OFFSET`. `kvm_arch_tsc_set_attr` hands the caller's value straight to `__kvm_synchronize_tsc` with no heuristic in between, so it lands verbatim. Writing a non-zero value close to the target through the MSR instead would dodge the zero special case, but not the one after it: a non-zero write within a second of the previous one is folded onto the existing offset by the slop branch once `kvm->arch.user_set_tsc` is set, and a reset that follows the previous write inside a second is inside that window. Three things about that attribute drive the shape of this change: * It is an offset, not a counter value. The guest reads `scale(host TSC) + offset`, so restarting the guest near zero means writing roughly minus the scaled host TSC. This derives it from the current pair (`new = old_offset - guest_tsc`) rather than from `rdtsc`, which is exact under TSC scaling without having to know the ratio, since `scale(host) == guest_tsc - old_offset` by the same identity. * Every vp must get the same offset, computed once. The kernel reads unequal offsets as unsynchronized vcpus, so a separately sampled value per vp would open a new TSC generation on each write and drop the partition out of masterclock mode, which is what the reference clock is built on. One read, one value, one pass over the vps keeps them in a single generation. * The residual error is the host time between the read and the writes, so the guest restarts a few microseconds' worth of cycles above zero rather than exactly at it. That is the same order as the skew a cold start has anyway, and the next commit removes both. The per-vp `set_tsc` accessor is deliberately left alone. A restore writes a real non-zero value there and does not hit the zero heuristic, and routing it through the offset attribute would make it a per-vp write of an individually sampled value, which is exactly the generation churn described above. The reset now logs both halves of the partition clock, before and after, at info level. A reset that moves only one of them is the failure mode this fixes, and the pair is the only reading that tells that apart from a healthy reset. The TSC is read back after the write rather than assumed, since a write that reported success and changed nothing is the whole defect. On a kernel without the attribute (it landed in 5.16) `KVM_HAS_DEVICE_ATTR` says so up front and the reset warns and proceeds. Failing the reset outright would be a worse outcome than a counter that does not restart, and the warning keeps a later guest bugcheck from being unexplained. --- vm/kvm/src/lib.rs | 73 ++++++++++++++++++++ vmm_core/virt_kvm/src/arch/x86_64/mod.rs | 87 ++++++++++++++++++++++++ 2 files changed, 160 insertions(+) diff --git a/vm/kvm/src/lib.rs b/vm/kvm/src/lib.rs index 606f2e28d2c..be7528d1db6 100644 --- a/vm/kvm/src/lib.rs +++ b/vm/kvm/src/lib.rs @@ -127,6 +127,11 @@ mod ioctl { ); ioctl_readwrite!(kvm_create_device, KVMIO, 0xe0, kvm_create_device); ioctl_write_ptr!(kvm_set_device_attr, KVMIO, 0xe1, kvm_device_attr); + // KVM_GET_DEVICE_ATTR and KVM_HAS_DEVICE_ATTR are both _IOW: the struct itself is + // input either way, and a get returns its value through the user pointer the struct's + // `addr` field carries, not through the struct. + ioctl_write_ptr!(kvm_get_device_attr, KVMIO, 0xe2, kvm_device_attr); + ioctl_write_ptr!(kvm_has_device_attr, KVMIO, 0xe3, kvm_device_attr); ioctl_readwrite!(kvm_create_guest_memfd, KVMIO, 0xd4, kvm_create_guest_memfd); #[cfg(target_arch = "aarch64")] ioctl_readwrite_bad!( @@ -366,6 +371,10 @@ pub enum Error { CreateDevice(#[source] nix::Error), #[error("SetDeviceAttr")] SetDeviceAttr(#[source] nix::Error), + #[error("GetTscOffset")] + GetTscOffset(#[source] nix::Error), + #[error("SetTscOffset")] + SetTscOffset(#[source] nix::Error), #[error("CheckExtension")] CheckExtension(#[source] nix::Error), #[error("GetClock")] @@ -1726,6 +1735,70 @@ impl<'a> Processor<'a> { } } + /// Returns whether the kernel implements the vcpu TSC-offset attribute. + /// + /// The attribute landed in Linux 5.16, so an older kernel answers `ENXIO` here + /// rather than failing the write later. Callers that only want the TSC to move + /// can degrade quietly on `false`. + #[cfg(target_arch = "x86_64")] + pub fn supports_tsc_offset(&self) -> bool { + // SAFETY: KVM_HAS_DEVICE_ATTR reads only the struct; `addr` is unused for it. + unsafe { + ioctl::kvm_has_device_attr( + self.get().vcpu.as_raw_fd(), + &kvm_device_attr { + group: KVM_VCPU_TSC_CTRL, + attr: KVM_VCPU_TSC_OFFSET as u64, + addr: 0, + flags: 0, + }, + ) + .is_ok() + } + } + + /// Returns the vcpu's current L1 TSC offset. + /// + /// The guest reads `scale(host TSC) + offset`, so this is the whole of what stands + /// between the host counter and the guest's view of it. + #[cfg(target_arch = "x86_64")] + pub fn tsc_offset(&self) -> Result { + let mut offset = 0u64; + // SAFETY: the attribute's payload is a single u64, which is what `offset` is, + // and it outlives the call. + unsafe { + ioctl::kvm_get_device_attr( + self.get().vcpu.as_raw_fd(), + &kvm_device_attr { + group: KVM_VCPU_TSC_CTRL, + attr: KVM_VCPU_TSC_OFFSET as u64, + addr: std::ptr::from_mut(&mut offset) as u64, + flags: 0, + }, + ) + .map_err(Error::GetTscOffset)?; + } + Ok(offset) + } + + /// Sets the vcpu's L1 TSC offset, so that the guest reads + /// `scale(host TSC) + offset`. + /// + /// Unlike a write to `MSR_IA32_TSC`, this lands verbatim: the MSR path infers what + /// userspace "meant" and substitutes the partition's current offset for a write of + /// zero, which makes a write of zero the one value it cannot deliver. Write the same + /// offset to every vcpu, in one pass, so they stay in a single TSC-matching + /// generation. + #[cfg(target_arch = "x86_64")] + pub fn set_tsc_offset(&self, offset: u64) -> Result<()> { + // SAFETY: the attribute's payload is a single u64, which is what `offset` is. + unsafe { + self.set_device_attr(KVM_VCPU_TSC_CTRL, KVM_VCPU_TSC_OFFSET, &offset, 0) + .map_err(Error::SetTscOffset)?; + } + Ok(()) + } + pub fn runner(&self) -> VpRunner<'a> { // Ensure this thread is uniquely running the VP, and store the thread // ID to support cancellation. diff --git a/vmm_core/virt_kvm/src/arch/x86_64/mod.rs b/vmm_core/virt_kvm/src/arch/x86_64/mod.rs index 9d0d91a5b86..4ce17bfe532 100644 --- a/vmm_core/virt_kvm/src/arch/x86_64/mod.rs +++ b/vmm_core/virt_kvm/src/arch/x86_64/mod.rs @@ -715,12 +715,99 @@ impl ResetPartition for KvmPartition { fn reset(&self) -> Result<(), Self::Error> { let mut this = self; + // Sampled before the reset so the line below carries BOTH halves of the + // partition's clock. A reset that moves only one of them is what wedges a guest + // that calibrates one against the other, and the pair is the only reading that + // distinguishes that from a healthy reset. + let reference_time_before = self.inner.now().ref_time; this.reset_all(&self.inner.bsp().vp_info) .map_err(Box::new)?; + let tsc = self.inner.restart_tsc()?; + tracing::info!( + reference_time_before, + reference_time_after = self.inner.now().ref_time, + guest_tsc_before = tsc.map(|t| t.before), + guest_tsc_after = tsc.map(|t| t.after), + "machine reset" + ); Ok(()) } } +/// The guest timestamp counter either side of a machine reset, as read from the bsp. +/// +/// Absent when the kernel cannot express a TSC reset, so that a reader can tell "the +/// counter did not move" from "we never asked it to". +#[derive(Copy, Clone)] +struct GuestTscRestart { + before: u64, + after: u64, +} + +impl KvmPartitionInner { + /// Restarts the guest timestamp counter from (approximately) zero on every vp. + /// + /// `reset_all` asks for this already, by setting each vp's `Tsc` element to its + /// at-reset value of zero, but on KVM that request cannot arrive: the write goes to + /// `MSR_IA32_TSC`, and the kernel treats a host write of exactly zero as "userspace + /// is creating or synchronizing this vcpu" and substitutes the partition's current + /// offset (`kvm_synchronize_tsc`, arch/x86/kvm/x86.c - the branch commented "Force + /// synchronization when creating a vCPU, or when userspace explicitly writes a zero + /// value"). So the one value a reset needs is the one value that path discards, and + /// the counter carries the previous boot's elapsed cycles into the new one. Measured + /// on this host: guest TSC 5403506220, wrote 0, read back 5403522705. + /// + /// The vcpu device attribute has no such heuristic - `kvm_arch_tsc_set_attr` hands + /// the caller's value straight to `__kvm_synchronize_tsc` - so this writes the offset + /// instead. The attribute is an OFFSET, not a counter value: the guest reads + /// `scale(host TSC) + offset`, so restarting the guest near zero means writing + /// roughly minus the (scaled) host TSC. Deriving that from the current pair rather + /// than from `rdtsc` keeps it correct under TSC scaling and needs no knowledge of the + /// ratio, since `scale(host) == guest_tsc - offset` by the same identity. + /// + /// Every vp gets the SAME offset, computed once. Writing a separately-sampled value + /// per vp would leave them fractionally apart, and the kernel reads unequal offsets + /// as unsynchronized vcpus: each write would open a new TSC generation and drop the + /// partition out of masterclock mode, which is what the reference clock is built on. + /// + /// The residual error is the host time between the read and the writes, so the guest + /// restarts a few microseconds' worth of cycles above zero rather than exactly at it. + /// That is the same order as the skew a cold start has anyway, and far below anything + /// a guest can calibrate against. + fn restart_tsc(&self) -> Result, KvmError> { + let bsp = self.kvm.vp(self.bsp().vp_info.apic_id); + if !bsp.supports_tsc_offset() { + // Pre-5.16 kernels have no way to express this, and failing the whole reset + // would be a worse outcome than a counter that does not restart. Say so + // loudly, rather than leaving the guest's later bugcheck unexplained. + tracing::warn!( + "kernel does not support KVM_VCPU_TSC_OFFSET; \ + the guest TSC will not restart across a machine reset" + ); + return Ok(None); + } + + let offset = bsp.tsc_offset()?; + let mut tsc = [0u64; 1]; + bsp.get_msrs(&[x86defs::X86X_MSR_TSC], &mut tsc)?; + let new_offset = offset.wrapping_sub(tsc[0]); + + for vp in &self.vps { + self.kvm.vp(vp.vp_info.apic_id).set_tsc_offset(new_offset)?; + } + + // Read back rather than assume. The write goes through the kernel's TSC + // synchronization, which is entitled to adjust what it stores, and the whole + // defect this fixes was a write that reported success and changed nothing. + let mut after = [0u64; 1]; + bsp.get_msrs(&[x86defs::X86X_MSR_TSC], &mut after)?; + Ok(Some(GuestTscRestart { + before: tsc[0], + after: after[0], + })) + } +} + impl Partition for KvmPartition { fn supports_reset(&self) -> Option<&dyn ResetPartition> { Some(self) From e4d2ad815f45578921b832783b02669e962d0afc Mon Sep 17 00:00:00 2001 From: Robert Nowotny Date: Thu, 20 Aug 2026 16:11:33 +0200 Subject: [PATCH 2/3] virt_kvm: put the guest tsc on the reference clock's origin, a hair ahead of it The partition has two views of time and a guest hypervisor calibrates one against the other, so they have to start together. The previous commit fixed the reset half by targeting zero. The creation half is untouched, and it is the larger of the two. On a cold start the kernel zeroes the reference clock when the vm is created (`kvm_arch_init_vm` sets `kvmclock_offset` to minus the current base time) and fixes the guest counter's origin only when the bsp vcpu is created (`kvm_arch_vcpu_postcreate` -> `kvm_synchronize_tsc(vcpu, NULL)`). openvmm builds the whole partition between those two ioctls - the supported-cpuid query, the leaf build, capability derivation - so the guest starts life carrying that interval as a constant disagreement between the counter and the clock. Measured on this host at 832 to 977 us over seven starts. It lands 1:1 in the horizon a guest hypervisor computes for a synthetic timer deadline (regressing horizon on offset across those runs gives slope 1.0107 over a 10,153-tick span), which shortens a 1.978 ms one-shot to about 1.15 ms and puts half the arms in the past: about 1.88 M past-dated re-arms a second, and a nested guest that never reaches a usable desktop. Both halves are the same operation, so they become one function called from two places: read the clock, read the counter, write the corrected offset to every vp. At creation it runs after the `add_vp` loop, so the loop's own duration is not left in the answer. At reset it runs after `reset_all`, which has just returned the clock to zero, and it now targets the clock rather than zero - so the counter no longer restarts a few microseconds behind the clock the way it did (measured -6.52 us post-reset before this change). The correction moves the counter to the clock, never the other way. The clock is the partition's authority on time: every synthetic timer deadline is expressed in it and `GetReferenceTime` reads it directly, while the counter is a per-vp view of the same instant. The target is not a zero lead The comparison that decides a deadline is in `stimer_start` (arch/x86/kvm/hyperv.c): time_now = get_time_ref_counter(hv_stimer_to_vcpu(stimer)->kvm); ... stimer->exp_time = stimer->count; if (time_now >= stimer->count) { /* ... expire immediately ... */ `stimer->count` is written by the guest and computed from the guest's own counter: `counter_now + horizon`. `time_now` is the partition reference counter, which `get_time_ref_counter` derives from the kvmclock. With the counter behind the clock by D, `counter_now` reads D low, the deadline lands D early, and the branch is taken for every horizon shorter than D. A long horizon is harmless and a short one is the failure - that principle is right, and the sign it maps to is the counter-intuitive part. So exactly-on-the-clock is not the safe target either: the comparison is `>=`, so at a lead of zero a zero-horizon arm still fires immediately. That was measured rather than argued, interleaved over 3 rounds x 2 cells x {cold, warm}. At a 20 us lead: 0.027 past-dated arms a second, no near-class arm at all in 565 s over 12 boots. At zero: 0.104 a second, 46 of 63 forming a near class from -8.2 to -78.9 us, median -19.5 us, none within 3 us of zero. The control is that the main horizon cluster moved 2007.6 -> 1987.6 us, exactly the 20.0 us of lead, so nothing else changed. A deeper class (-277 us to -193 ms) sits at ~0.027/s in both arms; no lead of this size touches it. Firing early to cover a latency that cannot be removed is what KVM already does for the LAPIC timer: `lapic_timer_advance` (arch/x86/kvm/lapic.c), whose comment gives the same argument - KVM "programs the host timer event to fire early ... to account for the delay between taking the VM-Exit ... and the subsequent VM-Enter" - applied in `start_sw_tscdeadline` as `ktime_sub_ns(expire, timer_advance_ns)` under a `ns > timer_advance_ns` guard, sized `LAPIC_TIMER_ADVANCE_NS_INIT` 1000 / `LAPIC_TIMER_ADVANCE_NS_MAX` 5000 and tuned by `adjust_lapic_timer_advance`. What differs here is the stage. That advance is applied when a deadline becomes a host timer, which is past the point where this goes wrong: by then `stimer_start` has already sorted the arm into past or future, and the sorting is the defect. So the same compensation moves one step earlier, onto the origin the deadline is computed from. KVM's accepted remedy for a horizon shorter than the latency - `apic_timer_expired` in `start_sw_tscdeadline`, `xen_timer_callback` in `kvm_xen_start_timer` - is not available for this timer. Both are terminal: a LAPIC or Xen one-shot that fires immediately fires once and the guest loses a tick. The Hyper-V direct-mode auto-enable one-shot RE-ARMS on delivery, so firing immediately hands the guest back a deadline it recomputes from the same trailing counter and re-arms just as short. That loop is the storm. For a timer that re-arms, this function has already picked the other answer: `stimer_start`'s periodic branch re-anchors a past-dated deadline strictly into the future (`div64_u64_rem(time_now - exp_time, count, &remainder); exp_time = time_now + (count - remainder)`) and discards the missed ticks. This does that to the one-shot's inputs, which is where a one-shot allows it. `kvm_xen_start_timer` is worth naming for the other half, since it hits our exact defect and stops short of this: it computes the guest's view of the clock the way the guest does, refusing `get_kvmclock_ns()` for having "a systemic error ... because it scales directly from host TSC to nanoseconds, and doesn't scale first to guest TSC and *then* to nanoseconds as the guest does", and then accepts the residual between its own `ktime_get()` and `rdtsc()` uncompensated. That is a defensible trade for a timer whose immediate-fire is terminal. Stock KVM needs neither, because `compute_tsc_page_parameters` derives the reference TSC page from the kvmclock and `get_time_ref_counter` reads the same struct back, so guest and kernel share one origin by construction. The gap exists only where something else establishes the counter. The lead is a floor plus a runtime term. The floor covers the delay L between the guest reading its counter to compute a deadline and KVM evaluating that deadline against the clock; over L the clock advances and the guest's number does not, so a zero-horizon arm reads as already past whenever the lead is at or under L. L was measured directly, pairing `kvm_exit(MSR_WRITE)` with the `set_count` and the `get_time_ref_counter` in `stimer_start`: ~98k arms per run, 100% pairing, two independent arms agreeing. p50 3.2 us, p90 3.9, p99 12-14, p99.9 18.4-18.7, max 72-123. The floor is 20 us because that is L's p99.9 rounded up - a percentile of a measured distribution, not a multiple of its median. It is also the bottom of the 21-to-38 us achieved band that measured clean at under 0.05 past-dated arms a second, so it sits at the edge of the data rather than under it. The cost is a synthetic timer delivered 20 us late, about 1% of the 1.978 ms one-shot period this guest uses. The floor is static where `timer_advance_ns` is adaptive, and deliberately so. `adjust_lapic_timer_advance` closes a loop on `guest_tsc - tsc_deadline`, which it has on every expiry; there is no such signal here. This runs twice in a partition's life, at creation and at reset, and would have to converge on a property of the host's exit latency that nothing downstream observes. An adaptive lead means first putting the ftrace pairing above into the arm path, which is future work if the static floor proves wrong on some host, not now. The runtime term is the pairing's own worst-case error, zero when the pairing is exact, so the achieved lead is at least the floor even at the worst residual and an exact pairing pays nothing for accuracy it did not need. The pairing comes from the kernel, and is verified `KVM_GET_CLOCK` reports `host_tsc` at the instant it was called and computes the clock it returns from exactly that value (`__get_kvmclock` ends with `__pvclock_read_cycles` on `data->host_tsc`); the guest's view of it is `host_tsc + offset`. The bracket of two counter reads around the ioctl is kept in a different job: as the bound the exact value has to lie inside, which turns "the counter is not scaled" from an assumption into a checked one. On a host that scales the guest counter the translation misses the bracket by orders of magnitude, and the fallback says so in the log rather than degrading quietly. The field is filled only on the masterclock branch, and `use_master_clock` is a cached bool that only `pvclock_update_vm_gtod_copy` writes. At partition build it was computed once, in `kvm_arch_init_vm`, when the vm had no vcpus and the matching-tsc test could not hold; the vcpu creations since made that test true but only raised `KVM_REQ_MASTERCLOCK_UPDATE`, which a vp has to run to service, and none has. Measured there: 4 of 4 reads carried no host counter, every read once the guest was running did. `KVM_SET_CLOCK` is the way out, because `kvm_vm_ioctl_set_clock` calls `pvclock_update_vm_gtod_copy` on the calling thread, so writing the clock back at the value just read recomputes the flag with no vp. That rebases `kvmclock_offset` and rewinds the clock by the read-to-write gap, which is acceptable at that one point and nowhere else: nothing has observed the clock yet, and the counter is then put on whatever it reads afterwards. On a machine reset the caller has just written the clock anyway, so the first read there already carries the counter. And the result is measured rather than assumed. After the write the pair is read again under the new offset, the lead it produced is computed from that reading, and it is classified: within the band both measurements allow, outside it, or under the floor. The last is an error-level line naming the lead, because a host where this goes wrong would otherwise produce a timer storm and no signal at all. Two things the arithmetic has to get right, because both were wrong first and both were caught by that verification rather than by a test: * The clock and the lead are converted to ticks as one ceiling over the whole sum. Converting them separately - the clock truncating, the lead ceiling - leaves the clock's own fractional tick discarded, which the lead's ceiling cannot always make up, so the target lands under `clock + lead` depending on the clock's value. * The achieved lead is differenced in the counter's own units and converted once, and it is judged at the resolution the conversions actually have (two whole nanoseconds plus a counter tick), kept separate from the band tolerance so the pairing error cannot excuse a genuine shortfall. Converting both counters to nanoseconds and subtracting puts the quantization error of both into an answer that is one part in fifty thousand of either. `kvm::Partition::get_clock_ns` becomes `get_clock` and returns a `ClockReading` whose `host_tsc` and `realtime` are options, so the flag bit and the field it guards stay together and no caller can read an unfilled field as a value. Converting the clock to ticks needs the guest's own rate, so this adds `KVM_GET_TSC_KHZ` on the bsp vcpu, asked of the vcpu rather than the vm so the answer is the rate the guest sees where the hardware scales the counter. The multiply is widened to 128 bits, since a long-running partition's clock times a GHz-scale rate leaves 64 bits well before the counter it describes does. Unit tests cover the offset arithmetic (a counter behind the clock, ahead of it, already on it, a conversion too large for 64 bits, a correction carrying the offset below zero), the bracket midpoint and its error bound against either endpoint, a bracket spanning the counter wrap, and the direction stated as the inequality a reader cares about. The conversion tests drive the production conversions rather than a test helper, compare against `clock + lead` as exact rationals, sweep the clock as well as the rate, and carry a negative control that fails if no swept case defeats the earlier piecewise form. The origin gap itself is only observable live. --- vm/kvm/src/lib.rs | 58 +- vmm_core/virt_kvm/src/arch/x86_64/mod.rs | 1439 ++++++++++++++++- vmm_core/virt_kvm/src/arch/x86_64/vm_state.rs | 4 +- 3 files changed, 1432 insertions(+), 69 deletions(-) diff --git a/vm/kvm/src/lib.rs b/vm/kvm/src/lib.rs index be7528d1db6..85a92f33a37 100644 --- a/vm/kvm/src/lib.rs +++ b/vm/kvm/src/lib.rs @@ -28,6 +28,7 @@ mod ioctl { use kvm_bindings::*; #[cfg(target_arch = "x86_64")] use nix::errno::Errno; + use nix::ioctl_none_bad; use nix::ioctl_read; use nix::ioctl_readwrite; use nix::ioctl_readwrite_bad; @@ -98,6 +99,11 @@ mod ioctl { #[cfg(target_arch = "x86_64")] ioctl_write_ptr!(kvm_set_debugregs, KVMIO, 0xa2, kvm_debugregs); ioctl_write_ptr!(kvm_enable_cap, KVMIO, 0xa3, kvm_enable_cap); + // Shares its number with KVM_ENABLE_CAP above; the two differ in the direction and + // size bits of the request code, so both spellings of 0xa3 are distinct requests. + // This one carries no payload at all and returns the rate as its result. + #[cfg(target_arch = "x86_64")] + ioctl_none_bad!(kvm_get_tsc_khz, request_code_none!(KVMIO, 0xa3)); #[cfg(target_arch = "x86_64")] ioctl_read!(kvm_get_xsave, KVMIO, 0xa4, kvm_xsave); #[cfg(target_arch = "x86_64")] @@ -371,6 +377,8 @@ pub enum Error { CreateDevice(#[source] nix::Error), #[error("SetDeviceAttr")] SetDeviceAttr(#[source] nix::Error), + #[error("GetTscKhz")] + GetTscKhz(#[source] nix::Error), #[error("GetTscOffset")] GetTscOffset(#[source] nix::Error), #[error("SetTscOffset")] @@ -1185,14 +1193,24 @@ impl Partition { Ok(()) } - /// Gets the current kvmclock value. - pub fn get_clock_ns(&self) -> Result { + /// Gets the current kvmclock value, with the host counter it was sampled at when the + /// kernel reports one. + pub fn get_clock(&self) -> Result { let mut clock = kvm_clock_data::default(); // SAFETY: Calling IOCTL as documented, with no special requirements. unsafe { ioctl::kvm_get_clock(self.vm.as_raw_fd(), &mut clock).map_err(Error::GetClock)?; } - Ok(clock) + Ok(ClockReading { + clock_ns: clock.clock, + // Both of these are only meaningful when the kernel says so: the fields are + // left at whatever the caller passed in on the branch that does not sample + // them, so a caller reading one unconditionally gets a zero that looks like a + // reading. Decoded here rather than at the call sites so the flag bit and the + // field it guards stay together. + host_tsc: (clock.flags & KVM_CLOCK_HOST_TSC != 0).then_some(clock.host_tsc), + realtime_ns: (clock.flags & KVM_CLOCK_REALTIME != 0).then_some(clock.realtime), + }) } /// Sets the current kvmclock value. @@ -1209,6 +1227,25 @@ impl Partition { } } +/// A reading of the partition's kvmclock. +#[derive(Debug, Copy, Clone)] +pub struct ClockReading { + /// The kvmclock, in nanoseconds. + pub clock_ns: u64, + /// The host timestamp counter at the instant the kernel sampled `clock_ns`, when the + /// kernel reported the pair. + /// + /// Present only on the masterclock branch of the kernel's clock read (`__get_kvmclock` + /// sets `KVM_CLOCK_HOST_TSC` there and computes `clock` from exactly this counter + /// value), which is why it is an option rather than a field: a caller that needs the + /// two as one observation has to know when the kernel is offering that and when it + /// has to measure the pairing itself. + pub host_tsc: Option, + /// Host wall-clock time at the same instant, in nanoseconds since the epoch, when the + /// kernel reported it. + pub realtime_ns: Option, +} + /// An in-kernel emulated device. pub struct Device(File); @@ -1735,6 +1772,21 @@ impl<'a> Processor<'a> { } } + /// Returns the rate the guest's timestamp counter runs at, in kHz. + /// + /// Asked of the vcpu rather than the vm so that the answer is the rate the GUEST + /// sees: where the hardware supports TSC scaling the two differ, and every conversion + /// between the counter and a wall-clock duration has to use the guest's rate to come + /// out right. + #[cfg(target_arch = "x86_64")] + pub fn tsc_khz(&self) -> Result { + // SAFETY: the request carries no payload; the rate comes back as the result. + let khz = unsafe { + ioctl::kvm_get_tsc_khz(self.get().vcpu.as_raw_fd()).map_err(Error::GetTscKhz)? + }; + Ok(khz as u32) + } + /// Returns whether the kernel implements the vcpu TSC-offset attribute. /// /// The attribute landed in Linux 5.16, so an older kernel answers `ENXIO` here diff --git a/vmm_core/virt_kvm/src/arch/x86_64/mod.rs b/vmm_core/virt_kvm/src/arch/x86_64/mod.rs index 4ce17bfe532..bc53daa16e8 100644 --- a/vmm_core/virt_kvm/src/arch/x86_64/mod.rs +++ b/vmm_core/virt_kvm/src/arch/x86_64/mod.rs @@ -486,6 +486,39 @@ impl ProtoPartition for KvmProtoPartition<'_> { self.vm.add_vp(vp_info.apic_id)?; } + // The vps exist, so the guest timestamp counter now has an origin - and it is not + // the one the partition reference clock got when the vm was created, several + // hundred microseconds earlier. Close that before anything can observe it. This + // has to come after the loop rather than after the bsp alone, or the loop's own + // duration (one vcpu creation per vp) stays in the answer. + // + // Done for every partition, not only the ones that enlighten the guest: the two + // origins disagreeing is a property of how the partition is built, and a guest + // that calibrates a counter against a clock is entitled to find them consistent + // whether or not it does so through the hyper-v interfaces. + // + // The per-vp `Tsc` element that guest state initialization writes later is a plain + // zero and cannot undo this: the kernel substitutes the partition's current offset + // for a host write of zero, and that is now the offset written here. + let alignment = align_guest_tsc_to_reference_clock( + &self.vm, + bsp_apic_id, + self.config + .processor_topology + .vps_arch() + .map(|vp_info| vp_info.apic_id), + )?; + tracing::info!( + reference_time = self.vm.get_clock()?.clock_ns / 100, + guest_tsc_before = alignment.map(|a| a.before), + guest_tsc_after = alignment.map(|a| a.after), + tsc_pairing = alignment.map(|a| a.pairing), + tsc_pairing_error_ns = alignment.map(|a| a.pairing_error_ns), + tsc_requested_lead_ns = alignment.map(|a| a.requested_lead_ns), + tsc_achieved_lead_ns = alignment.map(|a| a.achieved_lead_ns), + "partition created" + ); + let mut gsi_routing = GsiRouting::new(); // Claim the IOAPIC routes. @@ -722,90 +755,490 @@ impl ResetPartition for KvmPartition { let reference_time_before = self.inner.now().ref_time; this.reset_all(&self.inner.bsp().vp_info) .map_err(Box::new)?; - let tsc = self.inner.restart_tsc()?; + // `reset_all` has just returned the reference clock to zero; put the counter back + // on that same origin. Both halves of the partition clock restart together, which + // is what recreating the partition would do. + let tsc = align_guest_tsc_to_reference_clock( + &self.inner.kvm, + self.inner.bsp().vp_info.apic_id, + self.inner.vps.iter().map(|vp| vp.vp_info.apic_id), + )?; tracing::info!( reference_time_before, reference_time_after = self.inner.now().ref_time, guest_tsc_before = tsc.map(|t| t.before), guest_tsc_after = tsc.map(|t| t.after), + tsc_pairing = tsc.map(|t| t.pairing), + tsc_pairing_error_ns = tsc.map(|t| t.pairing_error_ns), + tsc_requested_lead_ns = tsc.map(|t| t.requested_lead_ns), + tsc_achieved_lead_ns = tsc.map(|t| t.achieved_lead_ns), "machine reset" ); Ok(()) } } -/// The guest timestamp counter either side of a machine reset, as read from the bsp. +/// The FLOOR under how far AHEAD of the partition reference clock the guest timestamp +/// counter is left, in nanoseconds. +/// +/// Not zero, and the sign is the point rather than a detail. A guest hypervisor computes a +/// synthetic timer deadline from ITS OWN counter and KVM tests that deadline against the +/// reference clock (`stimer_start`, arch/x86/kvm/hyperv.c: `time_now = +/// get_time_ref_counter(...)`, then `if (time_now >= stimer->count)` takes the +/// fire-immediately branch). So the guest's deadline is `counter_now + horizon` while the +/// test is against `clock_now`, and the two sides are only comparable to the extent the +/// counter and the clock share an origin: +/// +/// * counter BEHIND the clock by D - the guest's `counter_now` reads D low, so every +/// deadline lands D early and every arm with a horizon shorter than D reads as already +/// past. That is the storm. Measured on this host with the counter 22.08 us behind: +/// 3589 past-dated arms a second, clustered at a past-lag of 25.4 us, exactly D plus the +/// arm's own delivery latency. +/// * counter AHEAD of the clock by D - every deadline lands D late, so even a +/// zero-horizon arm is D in the future and the immediate branch is not reached at all. +/// Measured with the counter 21 to 38 us ahead: under 0.05 past-dated arms a second. +/// +/// A long horizon is harmless and a short one is the failure, which is the right +/// principle; it is the mapping to the sign that is counter-intuitive. A counter that +/// TRAILS makes deadlines read EARLY, not far away. +/// +/// So exactly-on-the-clock is not the target: at a lead of zero a zero-horizon arm still +/// satisfies `time_now >= count` and fires immediately. That was measured rather than +/// argued from the source: an interleaved A/B of this floor against a lead of zero put 46 +/// of 63 past-dated arms into a near class between -8.2 and -78.9 us (median -19.5 us), +/// none of them within 3 us of zero, and this floor removed that class outright. A zero +/// lead is materially worse, not merely "arms that were genuinely due". +/// +/// What this floor has to cover, and the only thing it has to cover, is the delay L +/// between the guest READING its counter to compute a deadline and KVM evaluating that +/// deadline against the clock. Over L the clock advances and the guest's number does not, +/// so a zero-horizon arm reads as already past whenever the lead is at or under L. +/// +/// L is a DISTRIBUTION, not a value, which is the whole reason this is 20 us and not 4. +/// Measured directly on this host by pairing `kvm_exit(MSR_WRITE)` with the `set_count` and +/// the `get_time_ref_counter` in `stimer_start` (~98k arms per run, 100% paired, two +/// independent arms agreeing): p50 3.2 us, p90 3.9, p99 12-14, p99.9 18.4-18.7, max 72-123. +/// The tail runs 20x the median, so sizing this off a TYPICAL L would leave everything past +/// the p99 uncovered - and an uncovered arm does not cost one late timer, it re-arms into +/// the storm. Hence a percentile, and hence a high one: 20 us is L's p99.9 rounded up. The +/// cost either side of that choice is asymmetric, a rare late delivery above it against an +/// unbounded failure below it. It is also the bottom of the 21-to-38 us achieved band that +/// measured clean (under 0.05 past-dated arms a second), so it sits at the edge of measured +/// data rather than extrapolated underneath it. +/// +/// Firing a timer early to cover a latency that cannot be removed is not a new idea; it is +/// what KVM does for the LAPIC timer (`lapic_timer_advance`, arch/x86/kvm/lapic.c, "programs +/// the host timer event to fire early ... to account for the delay between taking the +/// VM-Exit ... and the subsequent VM-Enter"), applied in `start_sw_tscdeadline` as +/// `ktime_sub_ns(expire, timer_advance_ns)` behind a `ns > timer_advance_ns` guard. What +/// differs is the STAGE, and that is why the compensation sits here rather than nearer the +/// timer: KVM's advance is applied when a deadline BECOMES a host timer, and by then +/// `stimer_start` has already sorted the arm into past or future. The sorting is the thing +/// that goes wrong, so the only stage left to compensate at is the origin the deadline is +/// computed from. +/// +/// Do NOT "simplify" this into an adaptive lead to match `adjust_lapic_timer_advance`. That +/// can close a loop because KVM holds `guest_tsc - tsc_deadline` on every expiry; this code +/// runs twice in a partition's life, at creation and at reset, and observes nothing +/// afterwards. Making it adaptive means first putting the ftrace pairing above into the arm +/// path, which is a different change with its own cost. +/// +/// The cost of the floor is a synthetic timer delivered up to 20 us late, about 1% of the +/// 1.978 ms one-shot period this guest arms. +/// +/// This is a floor, not the lead: the lead asked for is this plus a term for how well the +/// alignment knows its own inputs, so a host whose measurement is poor gets more margin +/// and one whose measurement is exact pays only this. See [`guest_tsc_lead_ns`]. +const GUEST_TSC_LEAD_FLOOR_NS: u64 = 20_000; + +/// The lead to ask for, given how well the counter/clock pairing behind the correction is +/// known. +/// +/// A single constant is wrong in both directions. Too large and every synthetic timer is +/// delivered that late for nothing. Too small and the alignment's OWN error can swallow +/// it: the correction lands the counter at `requested` plus or minus that error, so a +/// request under the error can leave the counter BEHIND the clock - the exact failure the +/// lead exists to prevent, and silently, because the error was measured and then not acted +/// on. +/// +/// So the request is the floor plus the error. At the worst case of the residual the +/// ACHIEVED lead is still at least the floor, and when the pairing is exact - KVM's own +/// counter/clock pair, error zero - nothing is paid for accuracy that was not needed. +fn guest_tsc_lead_ns(pairing_error_ns: u64) -> u64 { + GUEST_TSC_LEAD_FLOOR_NS.saturating_add(pairing_error_ns) +} + +/// How many times the reference clock read is bracketed by counter reads, the narrowest +/// bracket winning. /// -/// Absent when the kernel cannot express a TSC reset, so that a reader can tell "the +/// One bracket is enough for correctness - the midpoint estimate is unbiased either way - +/// but its error is half the bracket width, and a bracket that catches a preemption or a +/// host interrupt is wide. Taking the narrowest of a few costs two register reads each and +/// bounds the residual by the best sample rather than by an arbitrary one. +const REFERENCE_CLOCK_BRACKET_SAMPLES: usize = 3; + +/// What an alignment did, as measured afterwards rather than as intended. +/// +/// Absent when the kernel cannot express the alignment, so that a reader can tell "the /// counter did not move" from "we never asked it to". #[derive(Copy, Clone)] -struct GuestTscRestart { +struct GuestTscAlignment { before: u64, after: u64, + /// Where the counter/clock pairing the correction was computed from came from. + pairing: &'static str, + /// How far that pairing could be out, in nanoseconds. Zero when it was exact. + pairing_error_ns: u64, + /// The lead asked for, and the lead a fresh reading found afterwards. The second is + /// the one that decides whether the guest is safe; they differ by the residual. + requested_lead_ns: u64, + achieved_lead_ns: i64, } -impl KvmPartitionInner { - /// Restarts the guest timestamp counter from (approximately) zero on every vp. - /// - /// `reset_all` asks for this already, by setting each vp's `Tsc` element to its - /// at-reset value of zero, but on KVM that request cannot arrive: the write goes to - /// `MSR_IA32_TSC`, and the kernel treats a host write of exactly zero as "userspace - /// is creating or synchronizing this vcpu" and substitutes the partition's current - /// offset (`kvm_synchronize_tsc`, arch/x86/kvm/x86.c - the branch commented "Force - /// synchronization when creating a vCPU, or when userspace explicitly writes a zero - /// value"). So the one value a reset needs is the one value that path discards, and - /// the counter carries the previous boot's elapsed cycles into the new one. Measured - /// on this host: guest TSC 5403506220, wrote 0, read back 5403522705. - /// - /// The vcpu device attribute has no such heuristic - `kvm_arch_tsc_set_attr` hands - /// the caller's value straight to `__kvm_synchronize_tsc` - so this writes the offset - /// instead. The attribute is an OFFSET, not a counter value: the guest reads - /// `scale(host TSC) + offset`, so restarting the guest near zero means writing - /// roughly minus the (scaled) host TSC. Deriving that from the current pair rather - /// than from `rdtsc` keeps it correct under TSC scaling and needs no knowledge of the - /// ratio, since `scale(host) == guest_tsc - offset` by the same identity. - /// - /// Every vp gets the SAME offset, computed once. Writing a separately-sampled value - /// per vp would leave them fractionally apart, and the kernel reads unequal offsets - /// as unsynchronized vcpus: each write would open a new TSC generation and drop the - /// partition out of masterclock mode, which is what the reference clock is built on. - /// - /// The residual error is the host time between the read and the writes, so the guest - /// restarts a few microseconds' worth of cycles above zero rather than exactly at it. - /// That is the same order as the skew a cold start has anyway, and far below anything - /// a guest can calibrate against. - fn restart_tsc(&self) -> Result, KvmError> { - let bsp = self.kvm.vp(self.bsp().vp_info.apic_id); - if !bsp.supports_tsc_offset() { - // Pre-5.16 kernels have no way to express this, and failing the whole reset - // would be a worse outcome than a counter that does not restart. Say so - // loudly, rather than leaving the guest's later bugcheck unexplained. - tracing::warn!( - "kernel does not support KVM_VCPU_TSC_OFFSET; \ - the guest TSC will not restart across a machine reset" - ); - return Ok(None); +/// Where the guest counter's value at the instant of a reference clock read came from. +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +enum CounterPairing { + /// KVM reported the host counter it sampled the clock at, and translating it into the + /// guest's view lands inside the bracket measured around the same read. Exact: the + /// kernel and the caller are describing one instant, not two. + Exact(u64), + /// KVM did not report a host counter. Only the masterclock branch of `__get_kvmclock` + /// fills the field, so this says the partition was not on the masterclock at the + /// moment of the read. + NotReported, + /// KVM reported a host counter whose translation does NOT land inside the bracket, by + /// this many ticks. Not usable: the guest sees `scale(host tsc) + offset`, so adding + /// the offset alone assumes the scale is the identity, which holds only while the + /// guest counter runs at the host's own rate. A host that scales it fails here by far + /// more than a bracket, which is what makes the bracket a workable check on the + /// assumption rather than a formality. + Disagrees(u64), +} + +impl CounterPairing { + /// A stable name for the log, so a reader can tell which path an alignment took, and + /// on the fallback why. + fn source(&self) -> &'static str { + match self { + CounterPairing::Exact(_) => "kvm_host_tsc", + CounterPairing::NotReported => "bracket_host_tsc_not_reported", + CounterPairing::Disagrees(_) => "bracket_host_tsc_disagreed", } + } +} - let offset = bsp.tsc_offset()?; - let mut tsc = [0u64; 1]; - bsp.get_msrs(&[x86defs::X86X_MSR_TSC], &mut tsc)?; - let new_offset = offset.wrapping_sub(tsc[0]); +/// Translates the host counter KVM paired with a reference clock read into the guest's +/// view of it, and checks that answer against the bracket measured around the same read. +/// +/// `KVM_GET_CLOCK` reports `host_tsc` "at the instant when KVM_GET_CLOCK was called" +/// (`Documentation/virt/kvm/api.rst`, 4.29) and computes the clock it returns from exactly +/// that counter value (`__get_kvmclock` ends with +/// `data->clock = __pvclock_read_cycles(&hv_clock, data->host_tsc)`). That is the pairing +/// this whole function set is trying to establish, handed over for free - so use it, and +/// bracket only where it is genuinely absent. +/// +/// The bracket does not go away, it changes job: from producing the estimate to checking +/// the exact value. The translation `host tsc + offset` is only the guest's view while the +/// counter is not scaled, and the bracket is a bound that the true value must lie inside, +/// so requiring the translation to land within it tests the assumption instead of +/// asserting it. +fn pair_guest_tsc_with_reference_clock( + clock_host_tsc: Option, + tsc_offset: u64, + bracketed_guest_tsc: u64, + bracket_error_ticks: u64, +) -> CounterPairing { + let Some(host_tsc) = clock_host_tsc else { + return CounterPairing::NotReported; + }; + let paired = host_tsc.wrapping_add(tsc_offset); + // Modular distance: the counter is 64 bits, and either value can be the larger one + // when the pair straddles a wrap. + let disagreement = paired + .wrapping_sub(bracketed_guest_tsc) + .min(bracketed_guest_tsc.wrapping_sub(paired)); + if disagreement <= bracket_error_ticks { + CounterPairing::Exact(paired) + } else { + CounterPairing::Disagrees(disagreement) + } +} + +/// The reference clock, and the guest counter's value at the instant it was sampled. +struct ReferenceClockSample { + reference_clock_ns: u64, + /// The counter at the instant the clock was read. + guest_tsc: u64, + /// How far `guest_tsc` can be from the truth, in nanoseconds. Zero when it came from + /// KVM's own pairing rather than from an estimate. + error_ns: u64, + /// Which of those two it was. + pairing: CounterPairing, +} - for vp in &self.vps { - self.kvm.vp(vp.vp_info.apic_id).set_tsc_offset(new_offset)?; +/// Reads the partition reference clock and the guest counter as one paired observation. +/// +/// Preferred form: KVM reports the host counter it sampled the clock at, and the guest's +/// view of that counter is the exact answer. Fallback: the clock read is a `KVM_GET_CLOCK` +/// ioctl costing real host time - about 22 us here - so the counter is read on BOTH sides +/// of it and the clock attributed to the midpoint. The ioctl's own duration then cancels +/// instead of surviving as a bias, and what is left is half the bracket width in an +/// unknown direction. The bracket is measured either way, because it is also what the +/// exact answer is checked against. +fn sample_reference_clock_against_counter( + vm: &kvm::Partition, + bsp: &kvm::Processor<'_>, + tsc_offset: u64, + guest_tsc_khz: u32, +) -> Result { + let mut best: Option = None; + for _ in 0..REFERENCE_CLOCK_BRACKET_SAMPLES { + let mut opening = [0u64; 1]; + bsp.get_msrs(&[x86defs::X86X_MSR_TSC], &mut opening)?; + let clock = vm.get_clock()?; + let mut closing = [0u64; 1]; + bsp.get_msrs(&[x86defs::X86X_MSR_TSC], &mut closing)?; + + let bracketed = counter_at_bracket_midpoint(opening[0], closing[0]); + let error_ticks = bracket_width_ticks(opening[0], closing[0]) / 2; + let pairing = + pair_guest_tsc_with_reference_clock(clock.host_tsc, tsc_offset, bracketed, error_ticks); + let sample = match pairing { + CounterPairing::Exact(guest_tsc) => ReferenceClockSample { + reference_clock_ns: clock.clock_ns, + guest_tsc, + error_ns: 0, + pairing, + }, + CounterPairing::NotReported | CounterPairing::Disagrees(_) => ReferenceClockSample { + reference_clock_ns: clock.clock_ns, + guest_tsc: bracketed, + error_ns: ns_from_guest_tsc_ticks(error_ticks, guest_tsc_khz), + pairing, + }, + }; + // An exact pairing cannot be improved on, so stop: the remaining brackets exist + // only to narrow an estimate that is no longer being made. + if sample.error_ns == 0 { + return Ok(sample); + } + if best + .as_ref() + .is_none_or(|best| sample.error_ns < best.error_ns) + { + best = Some(sample); } + } + // The loop runs a fixed, non-zero number of times, so a sample always exists. Written + // as an expect rather than an unwrap so a later edit to the constant that made it zero + // fails with the reason rather than as a bare panic. + Ok(best.expect("at least one bracket is always sampled")) +} + +/// Asks the kernel to re-evaluate its masterclock, so the clock reads that follow carry +/// the host counter they were sampled at. +/// +/// `KVM_GET_CLOCK` fills `host_tsc` only on the masterclock branch (`__get_kvmclock`, +/// arch/x86/kvm/x86.c, under `ka->use_master_clock`), and `use_master_clock` is a CACHED +/// bool that only `pvclock_update_vm_gtod_copy` writes. On a cold start it was computed +/// once, from `kvm_arch_init_vm`, when the vm had no vcpus at all and the "every vcpu has a +/// matching TSC" test it depends on could not hold - so it is false. The vcpu creations +/// since then made that test true (each one after the first takes the `matched` branch of +/// `__kvm_synchronize_tsc` and raises `nr_vcpus_matched_tsc`) but only ASKED for a +/// recompute, via `KVM_REQ_MASTERCLOCK_UPDATE`, which a vp has to RUN to service. None has +/// run yet at the point the partition is built. Measured: 4 of 4 clock reads there came +/// back with no host counter, and every read once the guest was running had one. +/// +/// `KVM_SET_CLOCK` is the way out, because `kvm_vm_ioctl_set_clock` calls +/// `pvclock_update_vm_gtod_copy` on the calling thread. Writing the clock back at the value +/// just read therefore recomputes the flag without waiting for a vp. +/// +/// It is not quite a no-op - the kernel rebases `kvmclock_offset` onto the value passed, so +/// the clock is rewound by the time between the read and the write, tens of microseconds - +/// and that is acceptable at exactly this point and nowhere else: no vp has run, so nothing +/// has observed the clock, and the counter is then put on whatever the clock reads +/// AFTERWARDS. On a machine reset the caller has just written the clock anyway, so the +/// first read here already carries the counter and this returns without a second write. +/// +/// Best effort throughout: if the pairing is still absent the alignment brackets instead, +/// and its log says which it used. +fn prime_reference_clock_pairing(vm: &kvm::Partition) -> Result<(), KvmError> { + let clock = vm.get_clock()?; + if clock.host_tsc.is_some() { + return Ok(()); + } + vm.set_clock_ns(clock.clock_ns)?; + Ok(()) +} + +/// Puts the guest timestamp counter on the partition reference clock's origin, a +/// deliberate hair ahead of it, on every vp. +/// +/// The partition has two views of time and a guest hypervisor calibrates one against the +/// other, so they have to start together. They do not, on their own, at either of the two +/// points where the partition's clock starts: +/// +/// * On a cold start the kernel zeroes the reference clock when the vm is created +/// (`kvm_arch_init_vm` sets `kvmclock_offset` to minus the current base time) but fixes +/// the guest counter's origin only when the bsp vcpu is created +/// (`kvm_arch_vcpu_postcreate` -> `kvm_synchronize_tsc(vcpu, NULL)`). Everything between +/// those two ioctls - the supported-cpuid query, the leaf build, capability derivation - +/// is a gap the guest then carries for its whole life. Measured on this host at 832 to +/// 977 us across seven starts, landing 1:1 in the horizon a guest hypervisor computes +/// for a synthetic timer deadline, which is enough to make it arm ~1.9 M past-dated +/// timers a second. +/// * On a machine reset `reset_all` sets the reference clock to its at-reset value of zero +/// and asks the same of each vp's `Tsc`. That second half silently does not happen: the +/// write goes to `MSR_IA32_TSC`, and the kernel reads a host write of exactly zero as +/// "userspace is creating or synchronizing this vcpu" rather than as a value to store - +/// it sets `synchronizing` and substitutes `kvm->arch.cur_tsc_offset` +/// (`kvm_synchronize_tsc`, arch/x86/kvm/x86.c, the branch commented "Force +/// synchronization when creating a vCPU, or when userspace explicitly writes a zero +/// value"). So zero, the one value a reset needs to deliver, is the one value that path +/// cannot deliver. Measured on a live guest: TSC 5403506220, wrote 0, read back +/// 5403522705, while a control write of 0x4000000000000000 landed. +/// +/// Both are corrected the same way and in the same direction: the reference clock is the +/// partition's authority on time - every synthetic timer deadline is expressed in it, and +/// `GetReferenceTime` reads it directly - while the counter is a per-vp view of the same +/// instant. So this moves the counter to the clock. Moving the clock to the counter would +/// work arithmetically but is the wrong instrument: `KVM_SET_CLOCK` is a partition-wide +/// write that invalidates the reference page and kicks every vcpu, its effective origin is +/// a timestamp the kernel samples inside the ioctl and never reports, and on a cold start +/// it would have to be issued in the middle of building the partition. +/// +/// The correction goes through the vcpu device attribute `KVM_VCPU_TSC_OFFSET`, not +/// through `MSR_IA32_TSC`. `kvm_arch_tsc_set_attr` hands the caller's value straight to +/// `__kvm_synchronize_tsc`, so it lands verbatim; the MSR path infers what userspace +/// "meant" from the value's distance to where the counter would be anyway, and both of the +/// writes this function makes fall inside the window where that inference discards them. +/// The attribute is an OFFSET, not a counter value - the guest reads +/// `scale(host TSC) + offset` - so a correction of N ticks to the counter is a correction +/// of N ticks to the offset, and deriving it from the pair the guest itself reads keeps it +/// exact under TSC scaling without having to know the ratio. +/// +/// Every vp gets the SAME offset, computed once. The kernel takes unequal offsets as +/// unsynchronized vcpus: `kvm_arch_tsc_set_attr` marks a write `matched` only when it +/// equals the last offset written, so one pass with one value opens a single TSC +/// generation that every vp joins, whereas a separately sampled value per vp would open +/// one generation per write and drop the partition out of masterclock mode - which is what +/// the reference clock is built on. +/// +/// The counter is left deliberately AHEAD of the clock, by [`guest_tsc_lead_ns`]. Landing +/// it exactly on the clock is not the safe target and landing it behind is the failure +/// itself; see [`GUEST_TSC_LEAD_FLOOR_NS`] for which direction is which and why. +/// +/// The lead is then MEASURED rather than assumed. Everything above says what the write +/// should produce; only a fresh reading of the same pair afterwards says what it did, and +/// a host where that comes out wrong is exactly the host that would otherwise storm in +/// silence. So the reading is taken, compared to what was asked for, and reported at a +/// level that matches how bad the answer is. +fn align_guest_tsc_to_reference_clock( + vm: &kvm::Partition, + bsp_apic_id: u32, + apic_ids: impl Iterator, +) -> Result, KvmError> { + let bsp = vm.vp(bsp_apic_id); + if !bsp.supports_tsc_offset() { + // The attribute landed in Linux 5.16. Older kernels have no way to express this, + // and failing outright would be a worse outcome than a counter left on the wrong + // origin. Say so loudly, so a later guest bugcheck is not unexplained. + tracing::warn!( + "kernel does not support KVM_VCPU_TSC_OFFSET; \ + the guest tsc will not be aligned to the partition reference clock" + ); + return Ok(None); + } + + let guest_tsc_khz = bsp.tsc_khz()?; + let offset = bsp.tsc_offset()?; - // Read back rather than assume. The write goes through the kernel's TSC - // synchronization, which is entitled to adjust what it stores, and the whole - // defect this fixes was a write that reported success and changed nothing. - let mut after = [0u64; 1]; - bsp.get_msrs(&[x86defs::X86X_MSR_TSC], &mut after)?; - Ok(Some(GuestTscRestart { - before: tsc[0], - after: after[0], - })) + prime_reference_clock_pairing(vm)?; + + let sample = sample_reference_clock_against_counter(vm, &bsp, offset, guest_tsc_khz)?; + if let CounterPairing::Disagrees(by_ticks) = sample.pairing { + // Worth saying out loud rather than silently degrading: the kernel offered a + // pairing and the guest's view of it is not where the bracket says the counter + // was. On a host that scales the guest counter that is expected and the fallback + // is correct; anything else means one of the two readings is not what it claims. + tracing::warn!( + by_ticks, + "kvm reported a host counter for the reference clock that is not the guest's \ + view of it; falling back to bracketing" + ); } + + let before = sample.guest_tsc; + let requested_lead_ns = guest_tsc_lead_ns(sample.error_ns); + let new_offset = tsc_offset_aligned_to_reference_clock( + offset, + before, + sample.reference_clock_ns, + guest_tsc_khz, + requested_lead_ns, + ); + for apic_id in apic_ids { + vm.vp(apic_id).set_tsc_offset(new_offset)?; + } + + // Measure the result rather than assume it, on both counts. The write goes through the + // kernel's TSC synchronization, which is entitled to adjust what it stores - a write + // that reported success and changed nothing is exactly the defect this replaces - and + // the lead the guest will actually see is the only thing that decides whether its + // synthetic timers arm past-dated. So the pair is read again, under the offset just + // written, and the lead it produced is computed from that reading. + let verify = sample_reference_clock_against_counter(vm, &bsp, new_offset, guest_tsc_khz)?; + let achieved_lead_ns = guest_tsc_lead_from_reference_clock( + verify.guest_tsc, + verify.reference_clock_ns, + guest_tsc_khz, + ); + // Both readings contribute: the first decides where the counter was put, the second + // where it is seen to be, so the band the answer is allowed to land in is as wide as + // the two of them together - plus what the units themselves cannot resolve, without + // which an exact pairing leaves no band at all. + let resolution_ns = lead_measurement_resolution_ns(guest_tsc_khz); + let tolerances = LeadTolerances { + requested_ns: requested_lead_ns, + band_ns: sample + .error_ns + .saturating_add(verify.error_ns) + .saturating_add(resolution_ns), + resolution_ns, + }; + let alignment = GuestTscAlignment { + before, + after: verify.guest_tsc, + pairing: sample.pairing.source(), + pairing_error_ns: sample.error_ns, + requested_lead_ns, + achieved_lead_ns, + }; + match classify_achieved_lead(achieved_lead_ns, &tolerances) { + LeadVerdict::AsIntended => tracing::info!( + achieved_lead_ns, + requested_lead_ns, + tolerance_ns = tolerances.band_ns, + pairing = alignment.pairing, + "guest tsc aligned to the partition reference clock" + ), + LeadVerdict::OutsideBand => tracing::warn!( + achieved_lead_ns, + requested_lead_ns, + tolerance_ns = tolerances.band_ns, + pairing = alignment.pairing, + "guest tsc lead landed outside the band its own measurement error allows" + ), + LeadVerdict::BelowFloor => tracing::error!( + achieved_lead_ns, + requested_lead_ns, + floor_ns = GUEST_TSC_LEAD_FLOOR_NS, + resolution_ns = tolerances.resolution_ns, + pairing = alignment.pairing, + "guest tsc lead is under its floor; the guest may arm past-dated synthetic timers" + ), + } + Ok(Some(alignment)) } impl Partition for KvmPartition { @@ -913,11 +1346,12 @@ impl GetReferenceTime for KvmPartitionInner { // clock for the reference time counter within KVM. // // This also gives us the system time, in some configurations. - let clock = self.kvm.get_clock_ns().unwrap(); + let clock = self.kvm.get_clock().unwrap(); ReferenceTimeResult { - ref_time: clock.clock / 100, - system_time: (clock.flags & kvm::KVM_CLOCK_REALTIME != 0) - .then(|| jiff::Timestamp::from_nanosecond(clock.realtime as i128).unwrap()), + ref_time: clock.clock_ns / 100, + system_time: clock + .realtime_ns + .map(|ns| jiff::Timestamp::from_nanosecond(ns as i128).unwrap()), } } } @@ -2030,3 +2464,880 @@ impl SignalMsi for KvmPartitionInner { self.request_msi(MsiRequest { address, data }); } } + +/// Converts a duration in nanoseconds to guest timestamp counter ticks. +/// +/// Widened for the multiply alone: a long-running partition's reference clock times a +/// GHz-scale rate leaves 64 bits well before the counter it describes does. +fn guest_tsc_ticks_from_ns(ns: u64, guest_tsc_khz: u32) -> u64 { + (ns as u128 * guest_tsc_khz as u128 / 1_000_000) as u64 +} + +/// Converts guest timestamp counter ticks to nanoseconds. +/// +/// The inverse of [`guest_tsc_ticks_from_ns`], widened for the same reason: a counter that +/// has been running a while, times a nanosecond scale, leaves 64 bits long before the +/// counter itself does. +fn ns_from_guest_tsc_ticks(ticks: u64, guest_tsc_khz: u32) -> u64 { + if guest_tsc_khz == 0 { + // The kernel reports the rate of a vcpu it has already created, so this does not + // happen. Answering zero rather than dividing by it keeps a kernel that surprises + // us out of a panic in the middle of building a partition; the caller's own + // verification then reports the lead as absent rather than as correct. + return 0; + } + (ticks as u128 * 1_000_000 / guest_tsc_khz as u128) as u64 +} + +/// The guest counter value that sits at least `lead_ns` ahead of a reference clock +/// reading, in ticks, rounding UP. +/// +/// ONE ceiling over the whole sum, not a truncated clock plus a separately rounded-up +/// lead. Rounding the lead alone is not enough and the difference is the whole defect this +/// replaces: [`guest_tsc_ticks_from_ns`] truncates, so converting the clock on its own +/// throws away its fractional tick, and the counter then lands that far under +/// `reference + lead` however carefully the lead itself was rounded. Whether it does is +/// decided by the fractional part of the clock's own tick equivalent, which is effectively +/// random per boot - measured on this host, a 20 us lead placed by truncate-then-add came +/// back under its floor on about a third of boots, always by 1 to 2 ns, and clean on the +/// rest. +/// +/// Ceiling the sum instead puts the counter at or above `reference + lead` for every +/// clock value, with the overshoot bounded by a single tick. +fn guest_tsc_ticks_at_reference_clock_plus_lead( + reference_clock_ns: u64, + lead_ns: u64, + guest_tsc_khz: u32, +) -> u64 { + ((reference_clock_ns as u128 + lead_ns as u128) * guest_tsc_khz as u128).div_ceil(1_000_000) + as u64 +} + +/// How far UNDER the true lead the verification's own arithmetic can read, in nanoseconds. +/// +/// The verification is a measurement, so it has a resolution, and the floor has to be +/// judged at that resolution or the alarm reports the instrument rather than the counter. +/// Three steps in it discard something, and two discard it downward: +/// +/// * the reference clock is reported in whole nanoseconds, and the reading the correction +/// was computed from and the reading the check takes are two such reports of one clock, +/// so the span between them can be a nanosecond short of the truth; +/// * the final ticks-to-nanoseconds conversion truncates, losing up to one more; +/// * the difference itself is carried in whole counter ticks, worth `ceil(1e6 / khz)` +/// nanoseconds once rendered. +/// +/// Truncating the clock into ticks is the third step and is left out on purpose: it moves +/// the reference BACKWARDS, so it can only inflate the reported lead, and an allowance for +/// it would be an allowance against nothing. +/// +/// Derived from the conversions rather than picked, so a slow counter - whose tick is +/// worth whole nanoseconds - gets the allowance it needs instead of the one that suited +/// this host. +fn lead_measurement_resolution_ns(guest_tsc_khz: u32) -> u64 { + if guest_tsc_khz == 0 { + return 2; + } + 2 + 1_000_000u64.div_ceil(guest_tsc_khz as u64) +} + +/// How far the guest counter leads the partition reference clock, in nanoseconds. +/// +/// SIGNED, because the sign is the safety property rather than a presentational detail: +/// negative is the counter TRAILING the clock, which is the state that makes a guest +/// hypervisor arm past-dated synthetic timers. An unsigned difference would render exactly +/// the dangerous case as a very large safe-looking one. +/// +/// Differenced in TICKS and converted once, not converted twice and differenced. The two +/// operands are whole counters and the answer is tens of microseconds, so converting each +/// to nanoseconds separately puts the full quantization error of both into the answer; +/// against a floor that is one part in fifty thousand of the operands, that is exactly +/// where a spurious shortfall comes from. +fn guest_tsc_lead_from_reference_clock( + guest_tsc: u64, + reference_clock_ns: u64, + guest_tsc_khz: u32, +) -> i64 { + if guest_tsc_khz == 0 { + return 0; + } + // Modular, then read as signed: the counter and the clock's tick equivalent can + // straddle a wrap, and the counter trailing the clock has to come out negative. + let lead_ticks = + guest_tsc.wrapping_sub(guest_tsc_ticks_from_ns(reference_clock_ns, guest_tsc_khz)) as i64; + ((lead_ticks as i128 * 1_000_000) / guest_tsc_khz as i128) + .clamp(i64::MIN as i128, i64::MAX as i128) as i64 +} + +/// How the lead an alignment achieved compares to the one it asked for. +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +enum LeadVerdict { + /// Within the band the alignment's own measurement error allows. + AsIntended, + /// Outside that band, but still clear of the floor. The guest is safe and the + /// correction was simply less accurate than its inputs claimed. + OutsideBand, + /// Under the floor, negative included. The counter is close enough to the clock, or + /// behind it, that a short-horizon arm can read as already past. + BelowFloor, +} + +/// What an achieved lead is judged against. +/// +/// The two allowances answer different questions and must not be collapsed into one +/// number. `band_ns` is how far the correction could have MISSED its request, so it +/// carries the pairing error of both readings. `resolution_ns` is what the verification +/// cannot SEE, and only that. Admitting the pairing error at the floor would excuse a +/// genuine shortfall the size of a bad measurement, which is the one thing the floor +/// exists to catch; leaving the resolution out of it reports rounding as a fault. +struct LeadTolerances { + requested_ns: u64, + band_ns: u64, + resolution_ns: u64, +} + +/// Judges the lead an alignment actually achieved. +/// +/// Two questions, and the second is the one that matters. Whether the lead landed near the +/// request says how good the correction was; whether it clears [`GUEST_TSC_LEAD_FLOOR_NS`] +/// says whether the guest is safe. An overshoot beyond what the measurement allows is +/// worth a look and harms nothing, while a shortfall to the floor is the storm returning, +/// so the caller reports them at different levels instead of collapsing both into one +/// "unexpected" line that a reader has to decode. +fn classify_achieved_lead(achieved_ns: i64, against: &LeadTolerances) -> LeadVerdict { + // Judged at the resolution of the instrument. A reported shortfall smaller than what + // the verification's own conversions discard says nothing about where the counter is, + // and firing on it costs the alarm its meaning: measured on this host, the bare + // comparison fired on about a third of boots at 1 to 2 ns under a 20 us floor. + let resolution_ns = against.resolution_ns.min(i64::MAX as u64) as i64; + if achieved_ns.saturating_add(resolution_ns) < GUEST_TSC_LEAD_FLOOR_NS as i64 { + return LeadVerdict::BelowFloor; + } + let low = against.requested_ns.saturating_sub(against.band_ns); + let high = against.requested_ns.saturating_add(against.band_ns); + if (achieved_ns as u64) < low || achieved_ns as u64 > high { + return LeadVerdict::OutsideBand; + } + LeadVerdict::AsIntended +} + +/// The number of counter ticks a bracket spans, i.e. how long the read it encloses took. +/// +/// Modular, because the counter is 64 bits and an alignment can be asked for either side +/// of a wrap. +fn bracket_width_ticks(before: u64, after: u64) -> u64 { + after.wrapping_sub(before) +} + +/// The counter's value at the middle of a bracket. +/// +/// The reference clock is sampled at an unknown instant inside the bracket, so the +/// midpoint is the estimate whose worst-case error is smallest: half the width, rather +/// than the whole of it that either endpoint alone would carry. That is what takes the +/// cost of the `KVM_GET_CLOCK` ioctl itself out of the answer, instead of leaving it in +/// the residual as a systematic bias in whichever direction the reads were ordered. +fn counter_at_bracket_midpoint(before: u64, after: u64) -> u64 { + before.wrapping_add(bracket_width_ticks(before, after) / 2) +} + +/// Returns the L1 TSC offset that puts the guest timestamp counter at least `lead_ns` +/// AHEAD of the partition reference clock. +/// +/// The guest reads `scale(host TSC) + offset`, so moving the counter by a known number of +/// ticks means moving the offset by the same number: the correction is the difference +/// between where the counter should read - [`guest_tsc_ticks_at_reference_clock_plus_lead`] +/// - and where it does read. +/// +/// The target is computed from the clock and the lead TOGETHER rather than converted +/// piecewise, because the lead is a minimum and only a single rounding of the whole +/// quantity can guarantee one; see that function. +fn tsc_offset_aligned_to_reference_clock( + current_offset: u64, + guest_tsc: u64, + reference_clock_ns: u64, + guest_tsc_khz: u32, + lead_ns: u64, +) -> u64 { + let target = + guest_tsc_ticks_at_reference_clock_plus_lead(reference_clock_ns, lead_ns, guest_tsc_khz); + // Modular throughout: the counter is 64 bits and wraps, and so does what stands in + // front of it, so a correction that carries either end past a boundary is ordinary. + current_offset.wrapping_add(target).wrapping_sub(guest_tsc) +} + +#[cfg(test)] +mod tests { + use super::CounterPairing; + use super::GUEST_TSC_LEAD_FLOOR_NS; + use super::LeadTolerances; + use super::LeadVerdict; + use super::bracket_width_ticks; + use super::classify_achieved_lead; + use super::counter_at_bracket_midpoint; + use super::guest_tsc_lead_from_reference_clock; + use super::guest_tsc_lead_ns; + use super::guest_tsc_ticks_at_reference_clock_plus_lead; + use super::guest_tsc_ticks_from_ns; + use super::lead_measurement_resolution_ns; + use super::ns_from_guest_tsc_ticks; + use super::pair_guest_tsc_with_reference_clock; + use super::tsc_offset_aligned_to_reference_clock; + + /// A plausible guest TSC rate for the hosts this runs on, in kHz. + const KHZ: u32 = 2_700_000; + + /// Rates to sweep anything quantization-sensitive over. A single rate makes one + /// fractional part stand for all of them, which is how a rounding defect hides. + const RATES_KHZ: [u32; 5] = [2_701_631, 2_701_609, 2_700_000, 1_999_999, 3_800_017]; + + /// Converts nanoseconds to guest TSC ticks the way the caller's clock does. + fn ticks(ns: u64) -> u64 { + (ns as u128 * KHZ as u128 / 1_000_000) as u64 + } + + /// The lead the production caller asks for when its pairing is exact, in nanoseconds. + fn lead_ns() -> u64 { + guest_tsc_lead_ns(0) + } + + /// The counter the correction leaves behind, at the instant it was computed from. + /// + /// The guest reads `scale(host tsc) + offset`, so the same host instant that read + /// `guest_tsc` under `offset` reads this under the new one. + fn corrected_counter(offset: u64, guest_tsc: u64, new_offset: u64) -> u64 { + guest_tsc.wrapping_add(new_offset.wrapping_sub(offset)) + } + + /// Asserts a counter really is at or past `reference_clock_ns + lead_ns`, compared as + /// exact rationals. + /// + /// Deliberately not expressed through either conversion. Checking a rounded counter + /// against a rounded expectation lets the two roundings agree with each other and + /// prove nothing, which is precisely how the shortfall this guards reached a live + /// host; cross-multiplying leaves nothing to round. + fn assert_counter_clears(counter: u64, reference_clock_ns: u64, lead_ns: u64, khz: u32) { + let have = counter as u128 * 1_000_000; + let want = (reference_clock_ns as u128 + lead_ns as u128) * khz as u128; + assert!( + have >= want, + "khz={khz} clock_ns={reference_clock_ns} lead_ns={lead_ns}: \ + counter {counter} is short of the clock plus the lead by {} ticks", + (want - have) as f64 / 1_000_000.0, + ); + } + + /// The judgement the production caller makes, with its two allowances kept distinct. + fn tolerances(requested_ns: u64, pairing_error_ns: u64, khz: u32) -> LeadTolerances { + let resolution_ns = lead_measurement_resolution_ns(khz); + LeadTolerances { + requested_ns, + band_ns: pairing_error_ns.saturating_add(resolution_ns), + resolution_ns, + } + } + + #[test] + fn a_counter_that_trails_the_reference_clock_is_advanced_to_it() { + // Cold boot: the kernel zeroes the kvmclock when the vm is created and the guest + // TSC only when the bsp vcpu is created, so the counter starts a creation's worth + // of time behind the clock. + let gap_ns = 900_000; + let offset = 0x1234_5678_9abc_def0; + let new = tsc_offset_aligned_to_reference_clock(offset, 0, gap_ns, KHZ, lead_ns()); + assert_eq!( + new, + offset.wrapping_add(guest_tsc_ticks_at_reference_clock_plus_lead( + gap_ns, + lead_ns(), + KHZ + )) + ); + assert_counter_clears(corrected_counter(offset, 0, new), gap_ns, lead_ns(), KHZ); + } + + #[test] + fn a_counter_that_leads_the_reference_clock_is_retarded_to_it() { + // Machine reset: the reference clock has just been returned to zero and the + // counter still carries the previous boot's cycles. + let elapsed_ns = 5_000; + let offset = 0x1234_5678_9abc_def0; + let guest_tsc = 5_403_506_220; + let new = + tsc_offset_aligned_to_reference_clock(offset, guest_tsc, elapsed_ns, KHZ, lead_ns()); + assert_eq!( + new, + offset.wrapping_sub(guest_tsc).wrapping_add( + guest_tsc_ticks_at_reference_clock_plus_lead(elapsed_ns, lead_ns(), KHZ) + ) + ); + assert_counter_clears( + corrected_counter(offset, guest_tsc, new), + elapsed_ns, + lead_ns(), + KHZ, + ); + } + + #[test] + fn a_counter_already_on_the_reference_clock_is_advanced_by_the_lead_alone() { + // Zero is NOT the target. A deadline the guest computes from a counter sitting + // exactly on the clock still satisfies stimer_start's `time_now >= count` at a + // zero horizon, so the alignment has to leave the counter ahead. + // + // A clock whose tick equivalent is exact, so "advanced by the lead alone" is a + // statement about the lead and not about where the clock's own fraction landed. + let ns = 12_345_670; + assert_eq!( + ns as u128 * KHZ as u128 % 1_000_000, + 0, + "clock must be exact" + ); + let offset = 0x1234_5678_9abc_def0; + let new = tsc_offset_aligned_to_reference_clock(offset, ticks(ns), ns, KHZ, lead_ns()); + assert_eq!( + new, + offset.wrapping_add((lead_ns() as u128 * KHZ as u128).div_ceil(1_000_000) as u64) + ); + assert_counter_clears( + corrected_counter(offset, ticks(ns), new), + ns, + lead_ns(), + KHZ, + ); + } + + #[test] + fn the_corrected_counter_ends_ahead_of_the_clock_by_the_lead() { + // The direction is the whole point of the fix, so assert it as the inequality a + // reader cares about rather than only as an arithmetic identity: behind is the + // storm, ahead is safe. + // + // Swept over the CLOCK as well as the starting gap, and compared as exact + // rationals. The clock's own fractional tick is what the correction used to throw + // away, so a single clock value - or a comparison made through the same truncating + // conversion the correction uses - agrees with itself and misses it. + let offset = 0x0000_0100_0000_0000u64; + for khz in RATES_KHZ { + for clock_ns in [ + 4_000_000u64, + 4_000_001, + 17_807_900, + 123_456_789, + 999_999_999, + ] { + for behind_ns in [0, 1, 900_000] { + let guest_tsc = guest_tsc_ticks_from_ns(clock_ns, khz) + - guest_tsc_ticks_from_ns(behind_ns, khz); + let new = tsc_offset_aligned_to_reference_clock( + offset, + guest_tsc, + clock_ns, + khz, + lead_ns(), + ); + let corrected = corrected_counter(offset, guest_tsc, new); + assert_counter_clears(corrected, clock_ns, lead_ns(), khz); + // And not by more than the single tick the rounding is allowed to add, + // so the guarantee is not bought with unbounded margin. Both sides + // scaled by 1e6, as in the clearance check, to keep it exact. + let overshoot = corrected as u128 * 1_000_000 + - (clock_ns as u128 + lead_ns() as u128) * khz as u128; + assert!( + overshoot < 1_000_000, + "khz={khz} clock_ns={clock_ns} behind_ns={behind_ns}: \ + overshoot of {} ticks exceeds one", + overshoot as f64 / 1_000_000.0, + ); + } + } + } + } + + #[test] + fn a_zero_lead_leaves_the_counter_exactly_on_the_clock() { + // The lead is a parameter, not baked into the correction, so the arithmetic + // without it is still the plain alignment. Asserted at a clock whose tick + // equivalent is exact, where "on the clock" has an unambiguous answer. + let ns = 12_345_670; + assert_eq!( + ns as u128 * KHZ as u128 % 1_000_000, + 0, + "clock must be exact" + ); + let offset = 0x1234_5678_9abc_def0; + assert_eq!( + tsc_offset_aligned_to_reference_clock(offset, ticks(ns), ns, KHZ, 0), + offset + ); + } + + #[test] + fn a_long_uptime_does_not_overflow_the_conversion() { + // A year of reference clock times a GHz-scale rate exceeds 64 bits as a product, + // so the conversion has to be done wider than the values it converts. + let ns = 365 * 24 * 60 * 60 * 1_000_000_000u64; + let offset = 7; + let expected = (ns as u128 * KHZ as u128 / 1_000_000) as u64; + assert_eq!(guest_tsc_ticks_from_ns(ns, KHZ), expected); + assert_eq!( + tsc_offset_aligned_to_reference_clock(offset, 0, ns, KHZ, 0), + offset.wrapping_add(expected) + ); + } + + #[test] + fn an_offset_correction_below_zero_wraps_rather_than_panicking() { + // The offset is modular: the guest counter is 64 bits and so is what stands in + // front of it. A correction that takes the offset below zero is ordinary. + assert_eq!( + tsc_offset_aligned_to_reference_clock(10, 100, 2, KHZ, 0), + 10u64 + .wrapping_add(guest_tsc_ticks_at_reference_clock_plus_lead(2, 0, KHZ)) + .wrapping_sub(100) + ); + } + + #[test] + fn a_bracket_midpoint_is_half_way_between_its_ends() { + assert_eq!(bracket_width_ticks(1_000, 1_100), 100); + assert_eq!(counter_at_bracket_midpoint(1_000, 1_100), 1_050); + // Odd widths round toward the opening read, which is the conservative half: it + // attributes the clock slightly early, leaving the counter slightly further + // ahead rather than slightly behind. + assert_eq!(counter_at_bracket_midpoint(1_000, 1_101), 1_050); + } + + #[test] + fn a_zero_width_bracket_is_its_own_midpoint() { + assert_eq!(bracket_width_ticks(42, 42), 0); + assert_eq!(counter_at_bracket_midpoint(42, 42), 42); + } + + #[test] + fn a_bracket_spanning_the_counter_wrap_stays_correct() { + // The counter is 64 bits and an alignment can be asked for either side of a wrap, + // so the width has to be modular rather than a subtraction that would panic in + // debug and produce an absurd midpoint in release. + let before = u64::MAX - 9; + let after = before.wrapping_add(20); + assert_eq!(bracket_width_ticks(before, after), 20); + assert_eq!( + counter_at_bracket_midpoint(before, after), + before.wrapping_add(10) + ); + } + + #[test] + fn the_midpoint_halves_the_error_either_endpoint_would_carry() { + // The reason for bracketing at all: the clock is sampled at an unknown instant + // inside the bracket, so the worst case over that interval is what matters, and + // the midpoint's is half of what either end alone would carry. + let (before, after) = (10_000u64, 10_600u64); + let mid = counter_at_bracket_midpoint(before, after); + let width = bracket_width_ticks(before, after); + let worst = |estimate: u64| { + let lo = estimate.abs_diff(before); + let hi = estimate.abs_diff(after); + lo.max(hi) + }; + assert_eq!(worst(mid), width / 2); + assert_eq!(worst(before), width); + assert_eq!(worst(after), width); + } + + #[test] + fn the_host_counter_kvm_reports_is_used_when_it_lands_inside_the_bracket() { + // KVM hands over the pairing the bracket exists to estimate, so take it: the + // guest's view of the reported host counter is `host tsc + offset`, exactly. + let offset = 0x0000_0500_0000_0000u64; + let host_tsc = 900_000_000_000u64; + let exact = host_tsc.wrapping_add(offset); + // A bracket whose midpoint is 400 ticks off the truth, half-width 1000: the exact + // value lies inside it, which is what makes it usable. + assert_eq!( + pair_guest_tsc_with_reference_clock(Some(host_tsc), offset, exact - 400, 1_000), + CounterPairing::Exact(exact), + ); + } + + #[test] + fn a_host_counter_outside_the_bracket_is_rejected_rather_than_trusted() { + // `host tsc + offset` is the guest's view only while the counter is unscaled. A + // host that scales it misses the bracket by orders of magnitude, so the bracket is + // a real test of that assumption and not a formality - and the reported distance + // is what tells a reader which of the two readings to doubt. + let offset = 0x0000_0500_0000_0000u64; + let host_tsc = 900_000_000_000u64; + let bracketed = host_tsc.wrapping_add(offset).wrapping_add(5_000); + assert_eq!( + pair_guest_tsc_with_reference_clock(Some(host_tsc), offset, bracketed, 1_000), + CounterPairing::Disagrees(5_000), + ); + } + + #[test] + fn an_absent_host_counter_falls_back_to_the_bracket() { + // The cold-boot case when the partition is not on the masterclock: the field is + // not reported, and "not reported" has to be distinguishable from "reported as + // zero", which is why the input is an option rather than a counter plus a flag. + assert_eq!( + pair_guest_tsc_with_reference_clock(None, 7, 12_345, 1_000), + CounterPairing::NotReported, + ); + } + + #[test] + fn a_pairing_that_straddles_the_counter_wrap_is_still_recognised() { + // Both the counter and the offset are modular, so the exact value and the bracket + // can sit on opposite sides of a wrap. A plain subtraction would call that pair + // 2^64 apart and reject a perfectly good pairing. + let host_tsc = u64::MAX - 100; + let offset = 200u64; + let exact = host_tsc.wrapping_add(offset); + // The exact value has wrapped past zero and the bracket midpoint has not, so the + // two really are on opposite sides. Stepping back by less than that would leave + // both above the boundary and the test would pass without exercising anything. + let bracketed = exact.wrapping_sub(150); + assert!(bracketed > exact, "the pair must straddle the wrap"); + assert_eq!( + pair_guest_tsc_with_reference_clock(Some(host_tsc), offset, bracketed, 1_000), + CounterPairing::Exact(exact), + ); + } + + #[test] + fn the_requested_lead_is_the_floor_plus_the_error_the_alignment_actually_has() { + // The point of the runtime term: a correction known to within e can land the + // counter e short of what it asked for, so asking for the floor alone lets that + // residual eat the whole margin. Asking for floor + e keeps the ACHIEVED lead at + // the floor even at the worst case. + assert_eq!(guest_tsc_lead_ns(0), GUEST_TSC_LEAD_FLOOR_NS); + assert_eq!(guest_tsc_lead_ns(11_000), GUEST_TSC_LEAD_FLOOR_NS + 11_000); + // And a pathological error does not wrap the request round to a tiny one. + assert_eq!(guest_tsc_lead_ns(u64::MAX), u64::MAX); + } + + #[test] + fn the_worst_case_residual_still_leaves_the_counter_at_the_floor() { + // The property the runtime term exists for, stated end to end: ask for the lead + // the alignment's own error justifies, let the correction land at its worst, and + // the counter is still at least the floor ahead of the clock. + let error_ns = 11_000; + let requested = guest_tsc_lead_ns(error_ns); + for residual in [-(error_ns as i64), 0, error_ns as i64] { + let achieved = requested as i64 + residual; + assert!( + achieved >= GUEST_TSC_LEAD_FLOOR_NS as i64, + "residual={residual} left the counter under the floor at {achieved}" + ); + } + } + + #[test] + fn a_counter_behind_the_clock_measures_as_a_negative_lead() { + // The dangerous direction has to be REPRESENTABLE. Measured as an unsigned + // difference it would come back as an enormous safe-looking number, which is how a + // host in the failing state would report itself as healthy. + let clock_ns = 4_000_000; + let behind = guest_tsc_lead_from_reference_clock(ticks(clock_ns - 22_080), clock_ns, KHZ); + assert!(behind < 0, "counter behind the clock must read negative"); + assert_eq!(behind, -22_080); + assert_eq!( + guest_tsc_lead_from_reference_clock(ticks(clock_ns + 20_000), clock_ns, KHZ), + 20_000, + ); + assert_eq!( + guest_tsc_lead_from_reference_clock(ticks(clock_ns), clock_ns, KHZ), + 0, + ); + } + + #[test] + fn the_correction_and_the_verification_agree_on_the_lead() { + // Ties the two halves together: what the offset arithmetic asks for is what a + // fresh reading of the same pair, under the new offset, measures. If these ever + // disagreed the verification would be checking a different quantity from the one + // being set, and its silence would mean nothing. + // + // Driven through the PRODUCTION conversion, not this module's `ticks` helper. An + // earlier form used the helper on both sides, so it agreed with itself and said + // nothing about the quantization that the live run then found. + let offset = 0x0000_0100_0000_0000u64; + for khz in RATES_KHZ { + for clock_ns in [4_000_000u64, 4_000_001, 17_807_900, 999_999_999] { + for error_ns in [0, 11_000] { + let requested = guest_tsc_lead_ns(error_ns); + let guest_tsc = guest_tsc_ticks_from_ns(clock_ns, khz) + - guest_tsc_ticks_from_ns(900_000, khz); + let new = tsc_offset_aligned_to_reference_clock( + offset, guest_tsc, clock_ns, khz, requested, + ); + let measured = guest_tsc_lead_from_reference_clock( + corrected_counter(offset, guest_tsc, new), + clock_ns, + khz, + ); + // Not an equality: the correction rounds the target UP by up to a tick + // and the measurement renders the answer in whole nanoseconds, so the + // two agree to within what those steps can move, not exactly. What + // must hold is that the measurement never reports the correction as + // having undershot its request. + assert!( + measured >= requested as i64, + "khz={khz} clock_ns={clock_ns}: measured {measured} under \ + requested {requested}", + ); + assert_eq!( + classify_achieved_lead(measured, &tolerances(requested, error_ns, khz)), + LeadVerdict::AsIntended, + "khz={khz} clock_ns={clock_ns}: measured {measured}", + ); + } + } + } + } + + #[test] + fn the_target_counter_clears_the_clock_plus_the_lead_at_every_clock_value() { + // The write half of the defect, isolated. Rounding the LEAD up is not enough: the + // clock's own fractional tick is discarded by the same conversion, and the target + // then sits under `clock + lead` by that fraction. Which clock values it happens + // at depends on the rate, so the sweep is over both. + // + // Compared as exact rationals, never through either conversion. + let mut piecewise_was_ever_short = false; + for khz in RATES_KHZ { + for clock_ns in [0u64, 1, 17_807_900, 4_000_000, 4_000_001, 999_999_999] { + for lead in [0u64, 1, GUEST_TSC_LEAD_FLOOR_NS] { + let target = guest_tsc_ticks_at_reference_clock_plus_lead(clock_ns, lead, khz); + assert_counter_clears(target, clock_ns, lead, khz); + + // The form this replaced, measured the same way. Recorded rather than + // asserted per case, because it is wrong only at some clock values - + // which is exactly why a single-value test passed while a third of + // live boots did not. + let piecewise = guest_tsc_ticks_from_ns(clock_ns, khz) + + (lead as u128 * khz as u128).div_ceil(1_000_000) as u64; + if (piecewise as u128) * 1_000_000 + < (clock_ns as u128 + lead as u128) * khz as u128 + { + piecewise_was_ever_short = true; + } + } + } + } + // The sweep's own negative control. If no case in it defeats the truncate-then-add + // form, the cases are not exercising the quantization and everything above passes + // vacuously. + assert!( + piecewise_was_ever_short, + "the sweep contains no clock value the piecewise form gets wrong, so it \ + cannot show that rounding the sum is what fixes it", + ); + } + + #[test] + fn the_measured_lead_does_not_fall_under_the_floor_through_quantization() { + // The defect the live run caught, as a test. The lead was converted to ticks by + // truncation and measured back by converting two whole nanosecond values and + // subtracting, so 20 us came back as 19999 ns on EVERY boot and the verification + // correctly called it a shortfall - a false alarm, which is worse than no alarm + // because it teaches a reader to ignore the real one. A minimum margin is + // quantized upward, and the lead is measured in the counter's own units and + // converted once. + // Swept over the clock, not just the rate: whether the double conversion loses a + // nanosecond depends on the fractional part of the clock's own tick equivalent, so + // a single convenient clock value can make the two forms agree and prove nothing. + // 17.8 ms is where the live partition sat when this was found. + let cases = [2_701_609u32, 2_700_000, 1_999_999, 3_800_017] + .into_iter() + .flat_map(|khz| { + [ + 17_807_900u64, + 4_000_000, + 4_000_001, + 123_456_789, + 999_999_999, + ] + .into_iter() + .map(move |clock_ns| (khz, clock_ns)) + }); + for (khz, clock_ns) in cases { + let offset = 0x0000_0100_0000_0000u64; + let requested = guest_tsc_lead_ns(0); + let guest_tsc = guest_tsc_ticks_from_ns(clock_ns, khz); + let new = + tsc_offset_aligned_to_reference_clock(offset, guest_tsc, clock_ns, khz, requested); + let corrected = corrected_counter(offset, guest_tsc, new); + let measured = guest_tsc_lead_from_reference_clock(corrected, clock_ns, khz); + assert!( + measured >= GUEST_TSC_LEAD_FLOOR_NS as i64, + "khz={khz} clock_ns={clock_ns}: measured {measured} is under the floor", + ); + assert_eq!( + classify_achieved_lead(measured, &tolerances(requested, 0, khz)), + LeadVerdict::AsIntended, + "khz={khz} clock_ns={clock_ns}: measured {measured} vs requested {requested}", + ); + } + } + + #[test] + fn the_floor_holds_when_the_verification_reads_the_clock_after_the_correction() { + // The case the previous round missed, and the one the live host was failing on. + // Every test above reads the SAME clock value the correction was computed from, so + // the reference reading's own nanosecond quantization cancels and the verification + // agrees with the write by construction. In the real sequence it does not: the + // correction is computed from one `KVM_GET_CLOCK`, sixteen `KVM_VCPU_TSC_OFFSET` + // writes follow, and the verification takes a SECOND reading tens of microseconds + // later. + // + // Both readings are the same clock rendered in whole nanoseconds, so the span + // between them is either whole nanosecond either side of the true one, depending + // on where inside a nanosecond the first read fell. `extra_ns = 1` is that second + // case, and it is the one that takes the measured lead under the floor - a + // MEASUREMENT artifact, not a counter that moved, so it must not raise the alarm. + let offset = 0x0000_0100_0000_0000u64; + for khz in RATES_KHZ { + for clock_ns in [17_807_900u64, 4_000_000, 4_000_001, 999_999_999] { + let requested = guest_tsc_lead_ns(0); + let guest_tsc = guest_tsc_ticks_from_ns(clock_ns, khz); + let new = tsc_offset_aligned_to_reference_clock( + offset, guest_tsc, clock_ns, khz, requested, + ); + let corrected = corrected_counter(offset, guest_tsc, new); + // The counter advances by whole ticks; the clock reports the same span + // rendered in whole nanoseconds. 270163 ticks is about 100 us at this + // host's rate, the order of the real write-to-verify gap. + for elapsed_ticks in [0u64, 1, 2, 3, 7, 100, 54_321, 270_163] { + for extra_ns in [0u64, 1] { + let seen_tsc = corrected.wrapping_add(elapsed_ticks); + let seen_clock = + clock_ns + ns_from_guest_tsc_ticks(elapsed_ticks, khz) + extra_ns; + let measured = + guest_tsc_lead_from_reference_clock(seen_tsc, seen_clock, khz); + assert_ne!( + classify_achieved_lead(measured, &tolerances(requested, 0, khz)), + LeadVerdict::BelowFloor, + "khz={khz} clock_ns={clock_ns} elapsed_ticks={elapsed_ticks} \ + extra_ns={extra_ns}: measured {measured} raised the floor \ + alarm, but the counter was never moved", + ); + } + } + } + } + } + + #[test] + fn a_real_shortfall_still_raises_the_floor_alarm() { + // The other half, and the reason the allowance is derived rather than widened: it + // must be small enough that the failure it exists to catch still fires. The storm + // this whole alignment answers ran the counter 22 us BEHIND the clock, and a lead + // one microsecond short of the floor is already a hundred times the resolution. + for khz in RATES_KHZ { + let against = tolerances(GUEST_TSC_LEAD_FLOOR_NS, 0, khz); + assert!( + against.resolution_ns < 1_000, + "khz={khz}: a resolution of {} ns would swallow a real shortfall", + against.resolution_ns, + ); + for measured in [-22_080i64, 0, 1_000, GUEST_TSC_LEAD_FLOOR_NS as i64 - 1_000] { + assert_eq!( + classify_achieved_lead(measured, &against), + LeadVerdict::BelowFloor, + "khz={khz}: {measured} ns must still be reported as under the floor", + ); + } + } + } + + #[test] + fn the_resolution_covers_both_whole_unit_conversions_and_the_tick() { + // Derived, not picked, so it has to be checkable against the conversions it comes + // from: one nanosecond for the reference clock being reported in whole + // nanoseconds, one for the final ticks-to-nanoseconds truncation, and a whole + // counter tick - which is a rounding at GHz rates and real nanoseconds below. + for khz in RATES_KHZ { + let resolution = lead_measurement_resolution_ns(khz); + assert_eq!( + resolution, + 2 + 1_000_000u64.div_ceil(khz as u64), + "khz={khz}" + ); + assert!(resolution >= 3, "khz={khz}"); + } + // A counter slow enough for a tick to be worth real time gets a real allowance, + // which a constant would not give it. + assert_eq!(lead_measurement_resolution_ns(1_000), 2 + 1_000); + // And a rate the kernel should never report does not divide by zero. + assert_eq!(lead_measurement_resolution_ns(0), 2); + } + + #[test] + fn a_lead_within_its_measurement_error_is_as_intended() { + let requested = guest_tsc_lead_ns(11_000); + let against = tolerances(requested, 11_000, KHZ); + assert_eq!( + classify_achieved_lead(requested as i64, &against), + LeadVerdict::AsIntended, + ); + assert_eq!( + classify_achieved_lead(requested as i64 + 11_000, &against), + LeadVerdict::AsIntended, + ); + } + + #[test] + fn a_lead_past_its_measurement_error_is_reported_but_not_alarming() { + // Overshoot costs late timers, nothing worse, so it is a different signal from a + // shortfall - the caller logs it at a lower level, and it must not be collapsed + // into the same verdict. + let requested = guest_tsc_lead_ns(0); + assert_eq!( + classify_achieved_lead(requested as i64 + 5_000, &tolerances(requested, 0, KHZ)), + LeadVerdict::OutsideBand, + ); + } + + #[test] + fn a_lead_under_the_floor_outranks_the_band() { + // The verdict that matters. A counter under the floor can arm past-dated timers + // whatever the request was, so it must not be reported as merely out-of-band even + // when a generous band would cover it. The band is the wide allowance and the + // resolution the narrow one; only the narrow one may reach the floor. + let wide = LeadTolerances { + requested_ns: 30_000, + band_ns: 60_000, + resolution_ns: lead_measurement_resolution_ns(KHZ), + }; + assert_eq!( + classify_achieved_lead(GUEST_TSC_LEAD_FLOOR_NS as i64 - 1_000, &wide), + LeadVerdict::BelowFloor, + ); + assert_eq!( + classify_achieved_lead(-22_080, &wide), + LeadVerdict::BelowFloor, + ); + assert_eq!( + classify_achieved_lead( + GUEST_TSC_LEAD_FLOOR_NS as i64, + &tolerances(GUEST_TSC_LEAD_FLOOR_NS, 0, KHZ) + ), + LeadVerdict::AsIntended, + ); + } + + #[test] + fn nanoseconds_and_ticks_round_trip_at_a_long_uptime() { + // The reverse conversion is on the verification path, where the input is a whole + // counter rather than a short duration, so it is the one that meets the big + // numbers first. + let ns = 365 * 24 * 60 * 60 * 1_000_000_000u64; + assert_eq!( + ns_from_guest_tsc_ticks(guest_tsc_ticks_from_ns(ns, KHZ), KHZ), + ns + ); + // And a rate the kernel should never report does not divide by zero. + assert_eq!(ns_from_guest_tsc_ticks(1_000, 0), 0); + } +} diff --git a/vmm_core/virt_kvm/src/arch/x86_64/vm_state.rs b/vmm_core/virt_kvm/src/arch/x86_64/vm_state.rs index a7bd948a108..81bc184d0ed 100644 --- a/vmm_core/virt_kvm/src/arch/x86_64/vm_state.rs +++ b/vmm_core/virt_kvm/src/arch/x86_64/vm_state.rs @@ -43,9 +43,9 @@ impl AccessVmState for &'_ KvmPartition { // Round up so that restoring this value never moves the kvm clock // backwards, since the guest can observe the clock at nanosecond // granularity. - let clock = self.inner.kvm.get_clock_ns()?; + let clock = self.inner.kvm.get_clock()?; Ok(vm::ReferenceTime { - value: clock.clock.div_ceil(100), + value: clock.clock_ns.div_ceil(100), }) } From 186eed320dee18992b5efd9edf01871cdfac3500 Mon Sep 17 00:00:00 2001 From: Robert Nowotny Date: Thu, 20 Aug 2026 18:41:17 +0200 Subject: [PATCH 3/3] virt_kvm, kvm: harden the guest tsc alignment against bad inputs Three review findings on the alignment, all of them about an input the code trusted rather than checked. kvm::Processor::tsc_khz cast the ioctl's signed result straight to u32, so a negative rate widened into one near 4.29e9 kHz and a zero passed through untouched. Either one scales the entire alignment: the target counter falls out at zero ticks and the offset written to every vp puts the guest counter on an origin unrelated to the reference clock, which is worse than not aligning at all, and the verification afterwards can report the missing lead but cannot undo the write. Reject a non-positive rate at the wrapper, and have the caller degrade the way it already does when the kernel cannot express the attribute - warn, and leave the counter where it was - rather than fail the partition build over a correction that is best effort by design. classify_achieved_lead settled the sign only as a side effect of the floor test. That holds today because a resolution derived from any plausible counter rate cannot reach the floor, but it is a coupling between two constants that nothing states: lower the floor towards a microsecond and a small negative lead clears the floor test, then widens through an unsigned cast into a large positive, so the counter trailing the clock is reported as a harmless overshoot. Decide the sign first, on its own terms. supports_tsc_offset already did the right thing with an error and only its comment was wrong, naming ENXIO as though a kernel older than 5.16 were the sole way the probe can fail. Say what the code does instead: any error means the attribute is unavailable, and the reason is dropped deliberately, because no caller has a response to one that differs from its response to another. --- vm/kvm/src/lib.rs | 22 ++++- vmm_core/virt_kvm/src/arch/x86_64/mod.rs | 105 ++++++++++++++++++++++- 2 files changed, 123 insertions(+), 4 deletions(-) diff --git a/vm/kvm/src/lib.rs b/vm/kvm/src/lib.rs index 85a92f33a37..5a88e3f5e9a 100644 --- a/vm/kvm/src/lib.rs +++ b/vm/kvm/src/lib.rs @@ -379,6 +379,8 @@ pub enum Error { SetDeviceAttr(#[source] nix::Error), #[error("GetTscKhz")] GetTscKhz(#[source] nix::Error), + #[error("kvm reported an implausible guest tsc rate of {0} kHz")] + ImplausibleTscKhz(libc::c_int), #[error("GetTscOffset")] GetTscOffset(#[source] nix::Error), #[error("SetTscOffset")] @@ -1778,20 +1780,34 @@ impl<'a> Processor<'a> { /// sees: where the hardware supports TSC scaling the two differ, and every conversion /// between the counter and a wall-clock duration has to use the guest's rate to come /// out right. + /// + /// A rate that is not strictly positive is an error rather than a cast. The ioctl + /// hands back a signed int, so a negative one widens into a rate near 4.29e9 kHz and + /// a zero divides or multiplies every conversion down to nothing; both give a caller + /// a number it cannot tell from a real one, and a scaling derived from either is + /// worse than no scaling at all. Returning the raw value keeps which of the two it + /// was in the error. #[cfg(target_arch = "x86_64")] pub fn tsc_khz(&self) -> Result { // SAFETY: the request carries no payload; the rate comes back as the result. let khz = unsafe { ioctl::kvm_get_tsc_khz(self.get().vcpu.as_raw_fd()).map_err(Error::GetTscKhz)? }; + if khz <= 0 { + return Err(Error::ImplausibleTscKhz(khz)); + } Ok(khz as u32) } /// Returns whether the kernel implements the vcpu TSC-offset attribute. /// - /// The attribute landed in Linux 5.16, so an older kernel answers `ENXIO` here - /// rather than failing the write later. Callers that only want the TSC to move - /// can degrade quietly on `false`. + /// ANY error is read as "not implemented", not only the `ENXIO` that a kernel older + /// than the attribute's 5.16 debut answers with in practice. Reporting the reason + /// would be reporting it to nobody: the probe exists so that the write is not + /// attempted blind, and no error it can return leaves a caller anything to do except + /// go without the attribute, so a `Result` would hand back a decision that cannot be + /// made differently. Callers that only want the TSC to move can degrade quietly on + /// `false`. #[cfg(target_arch = "x86_64")] pub fn supports_tsc_offset(&self) -> bool { // SAFETY: KVM_HAS_DEVICE_ATTR reads only the struct; `addr` is unused for it. diff --git a/vmm_core/virt_kvm/src/arch/x86_64/mod.rs b/vmm_core/virt_kvm/src/arch/x86_64/mod.rs index bc53daa16e8..06b4271dc83 100644 --- a/vmm_core/virt_kvm/src/arch/x86_64/mod.rs +++ b/vmm_core/virt_kvm/src/arch/x86_64/mod.rs @@ -1150,7 +1150,29 @@ fn align_guest_tsc_to_reference_clock( return Ok(None); } - let guest_tsc_khz = bsp.tsc_khz()?; + // Every conversion below is scaled by this rate, so a rate that cannot be read does + // not merely make the correction inaccurate: the target counter falls out as zero + // ticks and the offset written to every vp puts the guest counter on an origin + // unrelated to anything. That is strictly worse than the no-op of leaving the counter + // alone, which is at least self-consistent, and the verification afterwards would + // report the missing lead without being able to undo the write. So this degrades + // exactly like the unsupported-attribute branch above does: the alignment is best + // effort and must not fail the partition build over it. + // + // Whether a rate is usable at all is the wrapper's judgement, not this function's - + // it rejects anything not strictly positive - so there is one place to look and no + // second opinion to keep in step with it here. + let guest_tsc_khz = match bsp.tsc_khz() { + Ok(khz) => khz, + Err(err) => { + tracing::warn!( + error = &err as &dyn std::error::Error, + "could not read a usable guest tsc rate; \ + the guest tsc will not be aligned to the partition reference clock" + ); + return Ok(None); + } + }; let offset = bsp.tsc_offset()?; prime_reference_clock_pairing(vm)?; @@ -2604,6 +2626,22 @@ struct LeadTolerances { /// so the caller reports them at different levels instead of collapsing both into one /// "unexpected" line that a reader has to decode. fn classify_achieved_lead(achieved_ns: i64, against: &LeadTolerances) -> LeadVerdict { + // The sign is settled first, on its own terms. A negative lead is the counter behind + // the clock, which is the state the whole check exists to catch, and no allowance for + // what the instrument cannot see can make it acceptable. + // + // Stating it here also removes a coupling the rest of the function would otherwise + // depend on without saying so: the band comparison below casts to `u64`, which is + // sound only while every negative value has already been rejected. Today that holds + // by arithmetic rather than by intent - a resolution derived from any plausible + // counter rate cannot reach a 20 us floor, so the floor test catches the negatives + // first - and it stops holding the moment the floor is lowered towards a microsecond. + // A small negative lead would then clear the floor test, widen into a huge positive + // on the cast, and be reported as a harmless overshoot: the dangerous direction + // rendered as the safe one. + if achieved_ns < 0 { + return LeadVerdict::BelowFloor; + } // Judged at the resolution of the instrument. A reported shortfall smaller than what // the verification's own conversions discard says nothing about where the counter is, // and firing on it costs the alarm its meaning: measured on this host, the bare @@ -3327,6 +3365,71 @@ mod tests { ); } + #[test] + fn a_negative_lead_is_under_the_floor_however_coarse_the_instrument() { + // The counter behind the clock is the failure the floor exists to catch, so the + // sign has to decide the verdict on its own rather than have an allowance added + // to it first. A resolution wider than the floor is what separates the two: added + // to a small negative lead it clears the floor arithmetically, and the band + // comparison that follows reads the negative through an unsigned cast, so the + // dangerous direction would be reported as a harmless overshoot. + let coarse = LeadTolerances { + requested_ns: GUEST_TSC_LEAD_FLOOR_NS, + band_ns: GUEST_TSC_LEAD_FLOOR_NS, + resolution_ns: GUEST_TSC_LEAD_FLOOR_NS + 5_000, + }; + for measured in [-1i64, -5_000, -22_080, i64::MIN] { + assert_eq!( + classify_achieved_lead(measured, &coarse), + LeadVerdict::BelowFloor, + "{measured} ns is the counter trailing the clock", + ); + } + // And at a resolution the caller would really derive, where the floor test + // happens to catch the same values, so the two agree rather than one covering up + // for the other. + for khz in RATES_KHZ { + assert_eq!( + classify_achieved_lead(-1, &tolerances(GUEST_TSC_LEAD_FLOOR_NS, 0, khz)), + LeadVerdict::BelowFloor, + "khz={khz}", + ); + } + } + + #[test] + fn a_zero_tsc_rate_would_put_the_counter_on_a_meaningless_origin() { + // Why the caller refuses an unusable rate at the entry point instead of letting + // the conversions absorb it. Each leaf special-cases zero so it cannot divide by + // it, which keeps them all DEFINED - and none of that keeps the answer MEANINGFUL. + // The rate is not reachable through the ioctl wrapper without a live vcpu, so what + // is checkable here is the arithmetic the guard stands in front of. + let clock_ns = 900_000; + let offset = 0x1234_5678_9abc_def0; + let guest_tsc = 0x0fed_cba9_8765_4321; + // The target collapses to zero ticks whatever the clock reads, so the offset that + // gets written to every vp carries no relation to the reference clock at all. + assert_eq!( + guest_tsc_ticks_at_reference_clock_plus_lead(clock_ns, lead_ns(), 0), + 0, + ); + assert_eq!( + tsc_offset_aligned_to_reference_clock(offset, guest_tsc, clock_ns, 0, lead_ns()), + offset.wrapping_sub(guest_tsc), + ); + // The verification does notice - it reads the lead as zero for any counter, and + // zero is under the floor - but only after the write. Noticing is not undoing, + // which is what makes this worse than not aligning at all. + assert_eq!( + guest_tsc_lead_from_reference_clock(guest_tsc, clock_ns, 0), + 0 + ); + assert_eq!( + classify_achieved_lead(0, &tolerances(lead_ns(), 0, KHZ)), + LeadVerdict::BelowFloor, + ); + } + #[test] fn nanoseconds_and_ticks_round_trip_at_a_long_uptime() { // The reverse conversion is on the verification path, where the input is a whole