[PATCH v12 24/24] KVM: x86: Keep master clock for guests using legacy MSR_KVM_SYSTEM_TIME

[David Woodhouse <dwmw2@infradead.org>]

From: David Woodhouse <dwmw@amazon.co.uk>

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, the delta is back under the threshold, and the 2.6.16
heuristic is content. The reference point moving is invisible to any
correct guest — the clock remains the same function of the guest TSC.

A guest hammering the legacy MSR now 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.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 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 8af808fbf6ce..59260f087654 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1272,7 +1272,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 de3b5086f17c..2136c1f68032 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1144,21 +1144,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 039a2b802d84..1599fb0c7275 100644
--- a/tools/testing/selftests/kvm/x86/pvclock_test.c
+++ b/tools/testing/selftests/kvm/x86/pvclock_test.c
@@ -213,6 +213,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)
 {
@@ -273,10 +275,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