[PATCH v13 26/26] KVM: x86: Keep master clock for guests using legacy MSR_KVM_SYSTEM_TIME

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

From: David Woodhouse <dwmw@xxxxxxxxxxxx>

Commit 54750f2cf042 ("KVM: x86: workaround SuSE's 2.6.16 pvclock vs
masterclock issue") disables the master clock entirely for any guest
whose boot vCPU uses the legacy MSR. SUSE's 2.6.16 kernel treats a
pvclock tsc_timestamp more than 1/5 jiffie behind the TSC as suspect
and re-requests the time by rewriting the MSR; against the master
clock's long-lived reference point the delta never shrinks, so it
re-requests in a loop and fails to boot.

Satisfy the guest's freshness heuristic instead of capitulating to it:
a guest write to the legacy MSR now requests a masterclock update,
sliding the reference point forward before the pvclock is written (the
masterclock request is serviced before KVM_REQ_CLOCK_UPDATE on the
same guest entry). The re-requested pvclock then carries a recent
tsc_timestamp and the heuristic is content. Each re-anchor steps the
pvclock function by the sub-nanosecond (possibly backwards)
mult-rounding divergence accumulated since the previous anchor — the
same step every pre-existing masterclock update already takes — and
legacy-MSR guests now see PVCLOCK_TSC_STABLE_BIT like anyone else.

A guest hammering the legacy MSR forces repeated VM-wide masterclock
updates, but only its own VM's: a guest has always been able to burn
its own cycles. Re-anchoring just the writing vCPU's pvclock along the
masterclock line would avoid even that, but isn't worth the extra
plumbing for a 2006 guest kernel.

This removes the last way a guest could force the master clock off;
the remaining disable, backwards_tsc_observed, is the host's own
doing. The pvclock_test selftest grows a legacy-MSR case proving the
master clock survives and the rewritten pvclock is fresh.

Not tested against a real SLES10 image; the behaviour it demands is
reconstructed from the guest comment quoted in 54750f2cf042. The
freshness that guest observes can be no worse than before: with the
master clock disabled it saw a tsc_timestamp one exit-to-read latency
old, exactly what the rewrite now produces, and the update's own cost
does not age the timestamp, which is written only after the update
completes. A host loaded enough to defeat that would have livelocked
the old scheme equally. Should a real SLES10 guest prove otherwise,
the revert path is to restore the boot_vcpu_runs_old_kvmclock gate in
kvm_use_master_clock().

Signed-off-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
---
 arch/x86/include/asm/kvm_host.h               |  1 -
 arch/x86/kvm/msrs.c                           | 26 ++++----
 arch/x86/kvm/x86.c                            | 15 +----
 .../testing/selftests/kvm/x86/pvclock_test.c  | 63 +++++++++++++++++++
 4 files changed, 77 insertions(+), 28 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 773df58898ea..0db8169612a4 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1275,7 +1275,6 @@ struct kvm_arch {
 #endif
 
        bool backwards_tsc_observed;
-       bool boot_vcpu_runs_old_kvmclock;
        u32 bsp_vcpu_id;
 
        u64 disabled_quirks;
diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
index d07831b5dfb8..94cd4d73689a 100644
--- a/arch/x86/kvm/msrs.c
+++ b/arch/x86/kvm/msrs.c
@@ -1275,19 +1275,19 @@ static void kvm_write_wall_clock(struct kvm *kvm, gpa_t 
wall_clock, int sec_hi_o
 static void kvm_write_system_time(struct kvm_vcpu *vcpu, gpa_t system_time,
                                  bool old_msr, bool host_initiated)
 {
-       struct kvm_arch *ka = &vcpu->kvm->arch;
-
-       if (vcpu->vcpu_id == 0 && !host_initiated) {
-               /*
-                * Store the flag before requesting the masterclock update:
-                * the update may now be performed by another vCPU (which
-                * consumes this request), and it must observe the new value.
-                */
-               if (ka->boot_vcpu_runs_old_kvmclock != old_msr) {
-                       ka->boot_vcpu_runs_old_kvmclock = old_msr;
-                       kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
-               }
-       }
+       /*
+        * Slide the master clock reference point forward, so that the
+        * pvclock written below carries a recent tsc_timestamp. SUSE's
+        * 2.6.16 kernel (the only known user of the legacy MSR with a
+        * reason to care) re-requests the time by rewriting this MSR
+        * whenever tsc_timestamp falls more than 1/5 jiffie behind the
+        * TSC, and would livelock against a fixed reference point; each
+        * rewrite now genuinely freshens it. A write with the enable
+        * bit clear deactivates the pvclock; there is nothing to
+        * freshen.
+        */
+       if (old_msr && !host_initiated && (system_time & 1))
+               kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
 
        vcpu->arch.time = system_time;
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 921d834b9682..191f811a0cdd 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1181,21 +1181,8 @@ static bool kvm_use_master_clock(struct kvm *kvm)
 {
        struct kvm_arch *ka = &kvm->arch;
 
-       /*
-        * The 'old kvmclock' check is a workaround (from 2015) for a
-        * SUSE 2.6.16 kernel that didn't boot if the system_time in
-        * its kvmclock was too far behind the current time. So the
-        * mode of just setting the reference point and allowing time
-        * to proceed linearly from there makes it fail to boot.
-        * Despite that being kind of the *point* of the way the clock
-        * is exposed to the guest. By coincidence, the offending
-        * kernels used the old MSR_KVM_SYSTEM_TIME, which was moved
-        * only because it resided in the wrong number range. So the
-        * workaround is activated for *all* guests using the old MSR.
-        */
        return ka->all_vcpus_matched_freq &&
-               !ka->backwards_tsc_observed &&
-               !ka->boot_vcpu_runs_old_kvmclock;
+               !ka->backwards_tsc_observed;
 }
 
 static void kvm_track_tsc_matching(struct kvm_vcpu *vcpu, bool update_mclock)
diff --git a/tools/testing/selftests/kvm/x86/pvclock_test.c 
b/tools/testing/selftests/kvm/x86/pvclock_test.c
index cb57f5e2bf1e..f3c8d9d8303a 100644
--- a/tools/testing/selftests/kvm/x86/pvclock_test.c
+++ b/tools/testing/selftests/kvm/x86/pvclock_test.c
@@ -189,6 +189,8 @@ static void run_at_frequency(u64 tsc_khz, unsigned int 
sleep_sec)
 
 static void test_tsc_stable_bit(void);
 static void test_clock_guest_with_offsets(void);
+static void test_legacy_msr(void);
+static void run_vcpu_once(struct kvm_vcpu *vcpu);
 
 static void usage(const char *name)
 {
@@ -251,10 +253,71 @@ int main(int argc, char *argv[])
 
        test_tsc_stable_bit();
        test_clock_guest_with_offsets();
+       test_legacy_msr();
 
        return 0;
 }
 
+static void guest_code_legacy_msr(void)
+{
+       wrmsr(MSR_KVM_SYSTEM_TIME, KVMCLOCK_GPA | KVM_MSR_ENABLED);
+       GUEST_SYNC(0);
+       /* Re-request the time, as SUSE's 2.6.16 kernel would. */
+       wrmsr(MSR_KVM_SYSTEM_TIME, KVMCLOCK_GPA | KVM_MSR_ENABLED);
+       GUEST_SYNC(1);
+}
+
+/*
+ * A guest using the legacy MSR_KVM_SYSTEM_TIME must no longer lose the
+ * master clock, and rewriting the MSR must slide the reference point
+ * forward so the pvclock's tsc_timestamp is recent — that freshness is
+ * what SUSE's 2.6.16 kernel demands instead of tolerating a fixed
+ * reference point.
+ */
+static void test_legacy_msr(void)
+{
+       struct pvclock_vcpu_time_info *pvti;
+       struct kvm_clock_data clock;
+       struct kvm_vcpu *vcpu;
+       struct kvm_vm *vm;
+       u64 tsc_now, age_limit;
+
+       pr_info("Testing the legacy MSR_KVM_SYSTEM_TIME\n");
+
+       vm = vm_create_with_one_vcpu(&vcpu, guest_code_legacy_msr);
+       configure_pvclock(vm);
+
+       run_vcpu_once(vcpu);
+       vm_ioctl(vm, KVM_GET_CLOCK, &clock);
+       TEST_ASSERT(clock.flags & KVM_CLOCK_TSC_STABLE,
+                   "Legacy MSR_KVM_SYSTEM_TIME should not disable the master 
clock");
+
+       /* Age the master clock reference point, then re-request. */
+       usleep(300 * 1000);
+       run_vcpu_once(vcpu);
+
+       pvti = addr_gpa2hva(vm, KVMCLOCK_GPA);
+       tsc_now = vcpu_get_msr(vcpu, MSR_IA32_TSC);
+
+       /*
+        * The rewrite slid the reference point, so the pvclock must be
+        * anchored after the 300ms sleep: allow 250ms of slack for the
+        * ioctls around it. SUSE's real threshold was 1/5 jiffie, but
+        * asserting that tightly would flake on a loaded host.
+        */
+       age_limit = (u64)__vcpu_ioctl(vcpu, KVM_GET_TSC_KHZ, NULL) * 250;
+       TEST_ASSERT(tsc_now - pvti->tsc_timestamp < age_limit,
+                   "pvclock tsc_timestamp should be fresh after legacy MSR 
rewrite (age %llu cycles, limit %llu)",
+                   (unsigned long long)(tsc_now - pvti->tsc_timestamp),
+                   (unsigned long long)age_limit);
+
+       vm_ioctl(vm, KVM_GET_CLOCK, &clock);
+       TEST_ASSERT(clock.flags & KVM_CLOCK_TSC_STABLE,
+                   "Master clock should survive a legacy MSR rewrite");
+
+       kvm_vm_free(vm);
+}
+
 static volatile u32 vcpu_counter;
 static void guest_code_stable_bit(void)
 {
-- 
2.43.0