[PATCH v8 13/17] KVM: x86: Disable preemption, not IRQs, when getting TSC+freq pair

[Sean Christopherson <seanjc@google.com>]

Disable "just" preemption, not IRQs, when reading the TSC+frequency pair to
update guest time, as disabling IRQs to protect against task migration is
overkill (though it's *extremely* hard to see that it's overkill).

Disabling IRQs was added by commit 18068523d3a0 ("KVM: paravirtualized
clocksource: host part") before there was any coordination with timekeeping
(presumably disabling IRQs prevented the kernel from completing a software-
induced frequency change).

After the coordination and locking was added, commit c09664bb4418 ("KVM:
x86: fix deadlock in clock-in-progress request handling") moved the locking
and coordination out of IRQ protection, and thus made disabling IRQs
pointless, except for protecting get_cpu_tsc_khz().

And while cpu_tsc_khz is written only from IRQ context, and the *extremely*
confusing double IPIs sent by __kvmclock_cpufreq_notifier() to update the
per-CPU frequency make it seem like they would require readers to disable
IRQs, it is safe to read and consume cpu_tsc_khz (via get_cpu_tsc_khz())
with IRQs enabled.  The per-CPU variable is specifically written only in
IRQ context to ensure hotplugging a CPU wouldn't write cpu_tsc_khz with a
stale value (because apparently disabling IRQs would be too simple?!?).

As for the double IPIs in the frequency notifier, both IPIs are red
herrings.  The actual sequence that ensures KVM updates guest time with the
new frequency is that the first write is completed *before* the notifier
sets KVM_REQ_CLOCK_UPDATE for all vCPUs that last ran on the target pCPU.
The first write is done via IPI to adhere to the above rules, and the
second IPI is sent purely to kick any vCPU that happens to be running on
the target CPU out of the guest.  I.e. the second IPI writes cpu_tsc_khz
out of pure KVM laziness: it saves having to define another IPI callback.
In fact prior to commit 8cfdc0008542 ("KVM: x86: Make cpu_tsc_khz updates
use local CPU"), KVM did indeed use an empty callback to ack the IPI.  As
for why it was deemed cleaner to abuse tsc_khz_changed()...

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5667cd17672b..63702be799cc 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1797,7 +1797,6 @@ static void kvm_setup_guest_pvclock(struct pvclock_vcpu_time_info *ref_hv_clock,
 int kvm_guest_time_update(struct kvm_vcpu *v)
 {
 	struct pvclock_vcpu_time_info hv_clock = {};
-	unsigned long flags;
 	u64 tgt_tsc_hz;
 	unsigned seq;
 	struct kvm_vcpu_arch *vcpu = &v->arch;
@@ -1822,11 +1821,14 @@ int kvm_guest_time_update(struct kvm_vcpu *v)
 		}
 	} while (read_seqcount_retry(&ka->pvclock_sc, seq));
 
-	/* Keep irq disabled to prevent changes to the clock */
-	local_irq_save(flags);
+	/*
+	 * Ensure reading the TSC+frequency pair is done on the same CPU.  When
+	 * NOT using the master clock, the TSC frequency may vary between CPUs.
+	 */
+	preempt_disable();
 	tgt_tsc_hz = (u64)get_cpu_tsc_khz() * HZ_PER_KHZ;
 	if (unlikely(tgt_tsc_hz == 0)) {
-		local_irq_restore(flags);
+		preempt_enable();
 		kvm_make_request(KVM_REQ_CLOCK_UPDATE, v);
 		return 1;
 	}
@@ -1861,7 +1863,7 @@ int kvm_guest_time_update(struct kvm_vcpu *v)
 	 */
 	vcpu->last_guest_tsc = tsc_timestamp;
 
-	local_irq_restore(flags);
+	preempt_enable();
 
 	/* With all the info we got, fill in the values */
 
-- 
2.55.0.571.g244d577d93-goog