[PATCH v13 24/26] KVM: x86/xen: Drop open-coded fallback clock read in kvm_xen_start_timer()

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

From: David Woodhouse <dwmw@xxxxxxxxxxxx>

When no coherent (CLOCK_MONOTONIC, TSC) pair was available,
kvm_xen_start_timer() computed one from separate ktime_get() and
rdtsc() calls rather than using get_kvmclock_ns(), with a comment
explaining that the latter had a systemic error from scaling host TSC
directly to nanoseconds instead of scaling to the guest TSC first.

Commit abdef42276d8 ("KVM: x86: Fix KVM clock precision in
get_kvmclock() with TSC scaling") removed that systemic error: in
master clock mode, get_kvmclock_ns() now scales the host TSC delta to
the guest TSC and converts it with the guest's own mul/shift,
computing precisely what the guest would.

The fallback is also nearly unreachable. It sits behind the
use_master_clock check, so on 32-bit (where the master clock is never
active) it is dead code, and on 64-bit kvm_get_monotonic_and_clockread()
can only fail there if the host clocksource changed after the last
master clock update, in the window before the resulting masterclock
update is processed. In exactly that window get_kvmclock_ns() takes
its raw-clock fallback path rather than the guest TSC computation,
which is continuous with the last valid snapshot — though up to
~0.1ppm of the snapshot's age (multiplier quantization) away from the
guest's own pvclock arithmetic — and tolerable for the timers that
can be started before the pending masterclock update lands (the
clocksource change is noticed via a two-stage deferral, so it is not
strictly one).

Drop it and fall back to the existing get_kvmclock_ns() path.

Signed-off-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
---
 arch/x86/kvm/xen.c | 50 +++++++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 6aa00f746cba..2a157cba1c7c 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -188,7 +188,7 @@ static void kvm_xen_start_timer(struct kvm_vcpu *vcpu, u64 
guest_abs,
        struct kvm_vcpu_xen *xen = &vcpu->arch.xen;
        int64_t kernel_now, delta;
        uint64_t guest_now;
-       int r = -EOPNOTSUPP;
+       bool fallback = true;
 
        /*
         * The guest provides the requested timeout in absolute nanoseconds
@@ -210,6 +210,7 @@ static void kvm_xen_start_timer(struct kvm_vcpu *vcpu, u64 
guest_abs,
        do {
                struct pvclock_vcpu_time_info hv_clock;
                uint64_t host_tsc, guest_tsc;
+               int r = -EOPNOTSUPP;
 
                if (!cpu_feature_enabled(X86_FEATURE_CONSTANT_TSC) ||
                    !vcpu->kvm->arch.use_master_clock)
@@ -230,43 +231,42 @@ static void kvm_xen_start_timer(struct kvm_vcpu *vcpu, 
u64 guest_abs,
                if (r)
                        break;
 
+               /*
+                * With use_master_clock set, the clocksource was TSC-based
+                * when the master clock was last updated. The clockread can
+                * only fail here if the clocksource has changed since, in
+                * which case a masterclock update is already pending and
+                * get_kvmclock_ns() returns the raw-clock fallback value —
+                * continuous with the last valid snapshot, though it may
+                * sit up to ~0.1ppm of the snapshot's age away from the
+                * guest's own pvclock arithmetic. Only timers armed
+                * before the pending update is consumed see it.
+                */
                if (!IS_ENABLED(CONFIG_64BIT) ||
-                   !kvm_get_monotonic_and_clockread(&kernel_now, &host_tsc)) {
-                       /*
-                        * Don't fall back to get_kvmclock_ns() because it's
-                        * broken; it has a systemic error in its results
-                        * 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.
-                        *
-                        * There is a small error introduced here because time
-                        * continues to elapse between the ktime_get() and the
-                        * subsequent rdtsc(). But not the systemic drift due
-                        * to get_kvmclock_ns().
-                        */
-                       kernel_now = ktime_get(); /* This is CLOCK_MONOTONIC */
-                       host_tsc = rdtsc();
-               }
+                   !kvm_get_monotonic_and_clockread(&kernel_now, &host_tsc))
+                       break;
 
                /* Calculate the guest kvmclock as the guest would do it. */
                guest_tsc = kvm_read_l1_tsc(vcpu, host_tsc);
                guest_now = __pvclock_read_cycles(&hv_clock, guest_tsc);
+               fallback = false;
        } while (0);
 
-       if (r) {
+       if (fallback) {
                /*
-                * Without CONSTANT_TSC, get_kvmclock_ns() is the only option.
+                * Without CONSTANT_TSC and the master clock,
+                * get_kvmclock_ns() is the only option.
                 *
                 * Also if the guest PV clock hasn't been set up yet, as is
                 * likely to be the case during migration when the vCPU has
                 * not been run yet. It would be possible to calculate the
                 * scaling factors properly in that case but there's not much
-                * point in doing so. The get_kvmclock_ns() drift accumulates
-                * over time, so it's OK to use it at startup. Besides, on
-                * migration there's going to be a little bit of skew in the
-                * precise moment at which timers fire anyway. Often they'll
-                * be in the "past" by the time the VM is running again after
-                * migration.
+                * point in doing so. Without the master clock, the
+                * get_kvmclock_ns() drift accumulates over time, so it's OK
+                * to use it at startup. Besides, on migration there's going
+                * to be a little bit of skew in the precise moment at which
+                * timers fire anyway. Often they'll be in the "past" by the
+                * time the VM is running again after migration.
                 */
                guest_now = get_kvmclock_ns(vcpu->kvm);
                kernel_now = ktime_get();
-- 
2.43.0