[PATCH v8 00/17] KVM: x86: Cleaning up the KVM clock mess, part 1

[PATCH v8 00/17] KVM: x86: Cleaning up the KVM clock mess, part 1

[Sean Christopherson]

This is the first half (give or take) of David's series to clean up some of
the mess that is timekeeping in KVM.  The "cut point" is rather arbitrary:
I stopped when I lost confidence in my understanding of the patches.  I'm
planning on diving back into review for the remaining patches asap, but want
to get start landing changes since many of these are fixes.

v8:
 - Obviously cut partway through the full series.
 - Update commit references for 633d7652f80f ("KVM: x86/xen: Do not corrupt
   KVM clock in kvm_xen_shared_info_init()").
 - Chunk the "Restructure ..." patches into individual logical changes
   ("Restructure kvm_guest_time_update() for TSC upscaling" in particular was
   quite gnarly).
 - Ensure the TSC+freq pair during guest time updates happens on a single
   CPU. [Sashiko]
 - Keep __get_kvmclock() as a masterclock-specific helper to avoid deep
   indentation and gotos.
 - Do waaaaay too much spelunking to try piece together the history of some
   of the many warts.
 - Ensure preemption is disabled in get_kvmclock() when getting the TSC freq.

v7: https://lore.kernel.org/all/20260728144954.355376-1-dwmw2@infradead.org

David Woodhouse (12):
  KVM: x86: Improve accuracy of KVM clock when TSC scaling is in force
  KVM: x86: Explicitly disable TSC scaling without CONSTANT_TSC
  KVM: x86: Activate master clock immediately on vCPU creation
  KVM: x86: Avoid NTP frequency skew for KVM clock on 32-bit host
  KVM: x86: Wrap all of __get_kvmclock_master_clock() with
    CONFIG_X86_64=y
  KVM: x86: Fall back to non-master-clock if clockread fails in
    get_kvmclock()
  KVM: x86: Fix KVM clock precision in get_kvmclock() with TSC scaling
  KVM: x86: Use get_kvmclock() in kvm_get_wall_clock_epoch()
  KVM: x86: Fix compute_guest_tsc() to handle negative time deltas
  KVM: x86: Upscale TSC to "now", not master clock when updating PV
    clocks
  KVM: x86: Simplify and comment kvm_get_time_scale()
  KVM: x86: Remove implicit rdtsc() from kvm_compute_l1_tsc_offset()

Sean Christopherson (5):
  KVM: x86: Update "last guest TSC" snapshot prior to enabling
    IRQs/preemption
  KVM: x86: Drop unnecessary CPU pinning when computing/getting kvmclock
  KVM: x86: Move "no master clock" fallback from __get_kvmclock() to
    get_kvmclock()
  KVM: x86: Disable preemption, not IRQs, when getting TSC+freq pair
  KVM: x86: Make master clock logic in guest PV clock updates 64-bit
    only

 arch/x86/include/asm/kvm_host.h |   6 +-
 arch/x86/kvm/cpuid.c            |   3 +-
 arch/x86/kvm/msrs.c             |   3 +-
 arch/x86/kvm/svm/svm.c          |   3 +-
 arch/x86/kvm/vmx/vmx.c          |  10 +
 arch/x86/kvm/x86.c              | 332 ++++++++++++++++++--------------
 arch/x86/kvm/x86.h              |   3 +-
 7 files changed, 212 insertions(+), 148 deletions(-)


base-commit: 51ba04112e93ac6e04c627eddcea5caf9cbc0134
-- 
2.55.0.571.g244d577d93-goog

[PATCH v8 01/17] KVM: x86: Update "last guest TSC" snapshot prior to enabling IRQs/preemption

[Sean Christopherson]

When refreshing the last observed guest TSC during a guest time update,
write the snapshot before enabling IRQs, i.e. before enabling preemption.
If the task is migrated between updating the local tsc_timestamp, e.g. to
account for catch-up mode, and setting last_guest_tsc, kvm_arch_vcpu_load()
would set the vCPU's TSC offset using the old last_guest_tsc.

In practice, the bug is largely benign as it's not even strictly necessary
for KVM to refresh last_guest_tsc when updating guest time, as KVM's goal
is purely to prevent the guest from observing time jump backwards, i.e.
super duper strictly speaking, KVM only *needs* to update last_guest_tsc in
the VM-Exit path.

In fact, the update kvm_guest_time_update() in wasn't even added to play
nice with kvm_arch_vcpu_load(), it was added by commit 28e4639adf0c ("KVM:
x86: Fix kvmclock bug") to fix code that no longer exists.  As of commit
28e4639adf0c, kvm_guest_time_update() also consumed last_guest_tsc, to try
and prevent guest time from jumping backwards.  That code was eventually
removed by commit f25e656d31ad ("KVM: x86: fix tsc catchup issue with tsc
scaling"), but the last_guest_tsc update hung around.

Keep the update even though it's technically ok to drop the update, e.g. so
that the tsc_catchup updates aren't lost, and so that the guest won't see a
PV clock timestamp that appears to be in the future.

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

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d94b59140c45..d3b47e38698c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1820,6 +1820,12 @@ int kvm_guest_time_update(struct kvm_vcpu *v)
 		}
 	}
 
+	/*
+	 * Refresh L1's last "observed" TSC to match the PV clock's timestamp,
+	 * e.g. so that the guest can't see a TSC that's behind the reference.
+	 */
+	vcpu->last_guest_tsc = tsc_timestamp;
+
 	local_irq_restore(flags);
 
 	/* With all the info we got, fill in the values */
@@ -1841,7 +1847,6 @@ int kvm_guest_time_update(struct kvm_vcpu *v)
 	hv_clock.tsc_to_system_mul = vcpu->pvclock_tsc_mul;
 	hv_clock.tsc_timestamp = tsc_timestamp;
 	hv_clock.system_time = kernel_ns + v->kvm->arch.kvmclock_offset;
-	vcpu->last_guest_tsc = tsc_timestamp;
 
 	/* If the host uses TSC clocksource, then it is stable */
 	hv_clock.flags = 0;
-- 
2.55.0.571.g244d577d93-goog

[PATCH v8 02/17] KVM: x86: Improve accuracy of KVM clock when TSC scaling is in force

[Sean Christopherson]

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

The kvm_guest_time_update() function scales the host TSC frequency to
the guest's using kvm_scale_tsc() and the v->arch.l1_tsc_scaling_ratio
scaling ratio previously calculated for that vCPU. Then calculates the
scaling factors for the KVM clock itself based on that guest TSC
frequency.

However, it uses kHz as the unit when scaling, and then multiplies by
1000 only at the end.

With a host TSC frequency of 3000MHz and a guest set to 2500MHz, the
result of kvm_scale_tsc() will actually come out at 2,499,999kHz. So
the KVM clock advertised to the guest is based on a frequency of
2,499,999,000 Hz.

By using Hz as the unit from the beginning, the KVM clock would be based
on a more accurate frequency of 2,499,999,999 Hz in this example.

Use u64 for the hw_tsc_hz field since an unsigned int would overflow for
TSC frequencies above 4GHz. Use div_u64() for the Xen CPUID leaf to
play nice with 32-bit kernels.

Fixes: 78db6a503796 ("KVM: x86: rewrite handling of scaled TSC for kvmclock")
Reviewed-by: Paul Durrant <paul@xen.org>
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Tested-by: Dongli Zhang <dongli.zhang@oracle.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/cpuid.c            |  3 ++-
 arch/x86/kvm/x86.c              | 18 ++++++++++--------
 3 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 283847619ff8..6ddc988576d0 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -864,7 +864,7 @@ struct kvm_vcpu_arch {
 	gpa_t time;
 	s8  pvclock_tsc_shift;
 	u32 pvclock_tsc_mul;
-	unsigned int hw_tsc_khz;
+	u64 hw_tsc_hz;
 	struct gfn_to_pfn_cache pv_time;
 	/* set guest stopped flag in pvclock flags field */
 	bool pvclock_set_guest_stopped_request;
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 9e9cf6538a96..7049f5a5597b 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -16,6 +16,7 @@
 #include <linux/vmalloc.h>
 #include <linux/uaccess.h>
 #include <linux/sched/stat.h>
+#include <linux/units.h>
 
 #include <asm/processor.h>
 #include <asm/user.h>
@@ -2146,7 +2147,7 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
 				*ecx = vcpu->arch.pvclock_tsc_mul;
 				*edx = vcpu->arch.pvclock_tsc_shift;
 			} else if (index == 2) {
-				*eax = vcpu->arch.hw_tsc_khz;
+				*eax = div_u64(vcpu->arch.hw_tsc_hz, HZ_PER_KHZ);
 			}
 		}
 	} else {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d3b47e38698c..ce6d155b2ee6 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -61,6 +61,7 @@
 #include <linux/mem_encrypt.h>
 #include <linux/suspend.h>
 #include <linux/smp.h>
+#include <linux/units.h>
 
 #include <trace/events/ipi.h>
 #include <trace/events/kvm.h>
@@ -1763,7 +1764,8 @@ 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, tgt_tsc_khz;
+	unsigned long flags;
+	u64 tgt_tsc_hz;
 	unsigned seq;
 	struct kvm_vcpu_arch *vcpu = &v->arch;
 	struct kvm_arch *ka = &v->kvm->arch;
@@ -1789,8 +1791,8 @@ int kvm_guest_time_update(struct kvm_vcpu *v)
 
 	/* Keep irq disabled to prevent changes to the clock */
 	local_irq_save(flags);
-	tgt_tsc_khz = get_cpu_tsc_khz();
-	if (unlikely(tgt_tsc_khz == 0)) {
+	tgt_tsc_hz = (u64)get_cpu_tsc_khz() * HZ_PER_KHZ;
+	if (unlikely(tgt_tsc_hz == 0)) {
 		local_irq_restore(flags);
 		kvm_make_request(KVM_REQ_CLOCK_UPDATE, v);
 		return 1;
@@ -1831,16 +1833,16 @@ int kvm_guest_time_update(struct kvm_vcpu *v)
 	/* With all the info we got, fill in the values */
 
 	if (kvm_caps.has_tsc_control) {
-		tgt_tsc_khz = kvm_scale_tsc(tgt_tsc_khz,
+		tgt_tsc_hz = kvm_scale_tsc(tgt_tsc_hz,
 					    v->arch.l1_tsc_scaling_ratio);
-		tgt_tsc_khz = tgt_tsc_khz ? : 1;
+		tgt_tsc_hz = tgt_tsc_hz ? : 1;
 	}
 
-	if (unlikely(vcpu->hw_tsc_khz != tgt_tsc_khz)) {
-		kvm_get_time_scale(NSEC_PER_SEC, tgt_tsc_khz * 1000LL,
+	if (unlikely(vcpu->hw_tsc_hz != tgt_tsc_hz)) {
+		kvm_get_time_scale(NSEC_PER_SEC, tgt_tsc_hz,
 				   &vcpu->pvclock_tsc_shift,
 				   &vcpu->pvclock_tsc_mul);
-		vcpu->hw_tsc_khz = tgt_tsc_khz;
+		vcpu->hw_tsc_hz = tgt_tsc_hz;
 	}
 
 	hv_clock.tsc_shift = vcpu->pvclock_tsc_shift;
-- 
2.55.0.571.g244d577d93-goog

[PATCH v8 03/17] KVM: x86: Explicitly disable TSC scaling without CONSTANT_TSC

[Sean Christopherson]

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

KVM does make an attempt to cope with non-constant TSC, and has
notifiers to handle host TSC frequency changes. However, it *only*
adjusts the KVM clock, and doesn't adjust TSC frequency scaling when
the host changes.

This is presumably because non-constant TSCs were fixed in hardware
long before TSC scaling was implemented, so there should never be real
CPUs which have TSC scaling but *not* CONSTANT_TSC.

Such a combination could potentially happen in some odd L1 nesting
environment, but it isn't worth trying to support it. Just make the
dependency explicit.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Paul Durrant <paul@xen.org>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/svm.c |  3 ++-
 arch/x86/kvm/vmx/vmx.c | 10 ++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 9d607b98bd06..3057ca7051db 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -5602,7 +5602,8 @@ static __init int svm_hardware_setup(void)
 				     XFEATURE_MASK_BNDCSR);
 
 	if (tsc_scaling) {
-		if (!boot_cpu_has(X86_FEATURE_TSCRATEMSR)) {
+		if (!boot_cpu_has(X86_FEATURE_TSCRATEMSR) ||
+		    !boot_cpu_has(X86_FEATURE_CONSTANT_TSC)) {
 			tsc_scaling = false;
 		} else {
 			pr_info("TSC scaling supported\n");
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index e3bfe6aca1a0..9abd2ed3aeae 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2841,6 +2841,16 @@ static int setup_vmcs_config(struct vmcs_config *vmcs_conf,
 	if (!cpu_has_sgx())
 		_cpu_based_2nd_exec_control &= ~SECONDARY_EXEC_ENCLS_EXITING;
 
+	/*
+	 * KVM doesn't re-derive the TSC scaling ratio when the host TSC
+	 * frequency changes, so TSC scaling is only usable with a constant
+	 * TSC.  Clear the control here rather than in vmx_hardware_setup() so
+	 * that the per-CPU configs recomputed by vmx_check_processor_compat()
+	 * stay consistent with the golden vmcs_config.
+	 */
+	if (!boot_cpu_has(X86_FEATURE_CONSTANT_TSC))
+		_cpu_based_2nd_exec_control &= ~SECONDARY_EXEC_TSC_SCALING;
+
 	if (_cpu_based_exec_control & CPU_BASED_ACTIVATE_TERTIARY_CONTROLS)
 		_cpu_based_3rd_exec_control =
 			adjust_vmx_controls64(KVM_OPTIONAL_VMX_TERTIARY_VM_EXEC_CONTROL,
-- 
2.55.0.571.g244d577d93-goog

[PATCH v8 04/17] KVM: x86: Activate master clock immediately on vCPU creation

[Sean Christopherson]

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

Previously, the master clock was only activated when the first vCPU
processed KVM_REQ_MASTERCLOCK_UPDATE during KVM_RUN. This meant that
KVM_GET_CLOCK could not return the host_tsc field until after the
first KVM_RUN, making it impossible for userspace to follow the
documented TSC migration procedure without a dummy vCPU run.

Fix this by calling kvm_update_masterclock() directly from
kvm_arch_vcpu_postcreate(), after kvm_synchronize_tsc() has already
set all_vcpus_matched_freq. This ensures the master clock is active
immediately, and KVM_GET_CLOCK returns a valid {host_tsc, realtime}
pair as soon as a vCPU exists.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ce6d155b2ee6..3ebcf2f27f7d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9447,6 +9447,8 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
 		return;
 	vcpu_load(vcpu);
 	kvm_synchronize_tsc(vcpu, NULL);
+	if (!vcpu->kvm->arch.use_master_clock)
+		kvm_update_masterclock(vcpu->kvm);
 	vcpu_put(vcpu);
 
 	/* poll control enabled by default */
-- 
2.55.0.571.g244d577d93-goog

[PATCH v8 05/17] KVM: x86: Avoid NTP frequency skew for KVM clock on 32-bit host

[Sean Christopherson]

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

Commit 53fafdbb8b21 ("KVM: x86: switch KVMCLOCK base to monotonic raw
clock") did so only for 64-bit hosts, by capturing the boot offset from
within the existing clocksource notifier update_pvclock_gtod().

That notifier was added in commit 16e8d74d2da9 ("KVM: x86: notifier for
clocksource changes") but only on x86_64, because its original purpose
was just to disable the "master clock" mode which is only supported on
x86_64.

Now that the notifier is used for more than disabling master clock mode,
enable it for the 32-bit build too so that get_kvmclock_base_ns() can be
unaffected by NTP sync on 32-bit too.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Paul Durrant <paul@xen.org>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 3ebcf2f27f7d..f60f4826db1a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -874,7 +874,6 @@ static inline bool kvm_vcpu_exit_request(struct kvm_vcpu *vcpu)
 	       kvm_request_pending(vcpu) || xfer_to_guest_mode_work_pending();
 }
 
-#ifdef CONFIG_X86_64
 struct pvclock_clock {
 	int vclock_mode;
 	u64 cycle_last;
@@ -932,13 +931,6 @@ static s64 get_kvmclock_base_ns(void)
 	/* Count up from boot time, but with the frequency of the raw clock.  */
 	return ktime_to_ns(ktime_add(ktime_get_raw(), pvclock_gtod_data.offs_boot));
 }
-#else
-static s64 get_kvmclock_base_ns(void)
-{
-	/* Master clock not used, so we can just use CLOCK_BOOTTIME.  */
-	return ktime_get_boottime_ns();
-}
-#endif
 
 static uint32_t div_frac(uint32_t dividend, uint32_t divisor)
 {
@@ -6873,6 +6865,7 @@ static void pvclock_irq_work_fn(struct irq_work *w)
 }
 
 static DEFINE_IRQ_WORK(pvclock_irq_work, pvclock_irq_work_fn);
+#endif
 
 /*
  * Notification about pvclock gtod data update.
@@ -6880,26 +6873,26 @@ static DEFINE_IRQ_WORK(pvclock_irq_work, pvclock_irq_work_fn);
 static int pvclock_gtod_notify(struct notifier_block *nb, unsigned long unused,
 			       void *priv)
 {
-	struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
 	struct timekeeper *tk = priv;
 
 	update_pvclock_gtod(tk);
 
+#ifdef CONFIG_X86_64
 	/*
 	 * Disable master clock if host does not trust, or does not use,
 	 * TSC based clocksource. Delegate queue_work() to irq_work as
 	 * this is invoked with tk_core.seq write held.
 	 */
-	if (!gtod_is_based_on_tsc(gtod->clock.vclock_mode) &&
+	if (!gtod_is_based_on_tsc(pvclock_gtod_data.clock.vclock_mode) &&
 	    atomic_read(&kvm_guest_has_master_clock) != 0)
 		irq_work_queue(&pvclock_irq_work);
+#endif
 	return 0;
 }
 
 static struct notifier_block pvclock_gtod_notifier = {
 	.notifier_call = pvclock_gtod_notify,
 };
-#endif
 
 void kvm_setup_xss_caps(void)
 {
@@ -7118,9 +7111,9 @@ int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops)
 
 	if (pi_inject_timer == -1)
 		pi_inject_timer = housekeeping_enabled(HK_TYPE_TIMER);
-#ifdef CONFIG_X86_64
 	pvclock_gtod_register_notifier(&pvclock_gtod_notifier);
 
+#ifdef CONFIG_X86_64
 	if (hypervisor_is_type(X86_HYPER_MS_HYPERV))
 		set_hv_tscchange_cb(kvm_hyperv_tsc_notifier);
 #endif
@@ -7177,8 +7170,8 @@ void kvm_x86_vendor_exit(void)
 					    CPUFREQ_TRANSITION_NOTIFIER);
 		cpuhp_remove_state_nocalls(CPUHP_AP_X86_KVM_CLK_ONLINE);
 	}
-#ifdef CONFIG_X86_64
 	pvclock_gtod_unregister_notifier(&pvclock_gtod_notifier);
+#ifdef CONFIG_X86_64
 	irq_work_sync(&pvclock_irq_work);
 	cancel_work_sync(&pvclock_gtod_work);
 #endif
-- 
2.55.0.571.g244d577d93-goog

[PATCH v8 06/17] KVM: x86: Drop unnecessary CPU pinning when computing/getting kvmclock

[Sean Christopherson]

When computing the current kvmclock value, don't pin the task to the
current CPU for the entire duration of the master clock path, as the
CPU pinning was never about ensuring rdtsc() and cpu_tsc_khz would
agree.  As pointed out by David, ka->use_master_clock can only be true
when the host clocksource is TSC based, which in turn requires a stable,
constant and synchronised TSC across all CPUs.

The CPU pinning was added in commit e2c2206a1899 ("KVM: x86: Fix potential
preemption when get the current kvmclock timestamp") purely in response to
a CONFIG_DEBUG_PREEMPT=y bug due.  Despite what the comment would suggest,
including rdtsc() in the {get,put}_cpu() section was opportunistic.  In
fact, Paolo even said exactly that when suggesting that KVM guarantee the
rdtsc() would execute on the same CPU[*].

 : Also, rdtsc() should really be on the same CPU as __this_cpu_read.  We
 : know it's not really really necessary because the master clock is
 : active, but since we need a get_cpu/put_cpu pair, better be clean.

Nothing has changed in the last ~9 years, i.e. the rdtsc() still *should*
be on the same CPU, but super strictly speaking, all will be fine if the
task is migrated between grabbing the frequency and doing rdtsc().
Dropping the CPU pinning will allow dropping the rdtsc() entirely without
having to resort to a large "rewrite get_kvmclock()" patch.

Opportunistically add a comment to explain why KVM needs to snapshot the
frequency, because that _is_ a hard requirement to avoid reintroducing the
bug fixed by commit e70b57a6ce4e ("KVM: X86: Fix softlockup when get the
current kvmclock")

Link: https://lore.kernel.org/all/ae8de642-8f14-a70a-1fab-57e2c4093cd5@redhat.com [*]
Suggested-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f60f4826db1a..0c0bf4c14bc5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1655,13 +1655,18 @@ static void __get_kvmclock(struct kvm *kvm, struct kvm_clock_data *data)
 {
 	struct kvm_arch *ka = &kvm->arch;
 	struct pvclock_vcpu_time_info hv_clock;
+	u64 tsc_hz;
 
-	/* both __this_cpu_read() and rdtsc() should be on the same cpu */
+	/*
+	 * Snapshot and validate the TSC frequency as kvmclock_cpu_down_prep()
+	 * zeros the per-CPU value when a CPU is going offline.
+	 */
 	get_cpu();
+	tsc_hz = (u64)get_cpu_tsc_khz() * HZ_PER_KHZ;
+	put_cpu();
 
 	data->flags = 0;
-	if (ka->use_master_clock &&
-	    (static_cpu_has(X86_FEATURE_CONSTANT_TSC) || __this_cpu_read(cpu_tsc_khz))) {
+	if (ka->use_master_clock && tsc_hz) {
 #ifdef CONFIG_X86_64
 		struct timespec64 ts;
 
@@ -1675,15 +1680,13 @@ static void __get_kvmclock(struct kvm *kvm, struct kvm_clock_data *data)
 		data->flags |= KVM_CLOCK_TSC_STABLE;
 		hv_clock.tsc_timestamp = ka->master_cycle_now;
 		hv_clock.system_time = ka->master_kernel_ns + ka->kvmclock_offset;
-		kvm_get_time_scale(NSEC_PER_SEC, get_cpu_tsc_khz() * 1000LL,
+		kvm_get_time_scale(NSEC_PER_SEC,  tsc_hz,
 				   &hv_clock.tsc_shift,
 				   &hv_clock.tsc_to_system_mul);
 		data->clock = __pvclock_read_cycles(&hv_clock, data->host_tsc);
 	} else {
 		data->clock = get_kvmclock_base_ns() + ka->kvmclock_offset;
 	}
-
-	put_cpu();
 }
 
 static void get_kvmclock(struct kvm *kvm, struct kvm_clock_data *data)
-- 
2.55.0.571.g244d577d93-goog

[PATCH v8 07/17] KVM: x86: Move "no master clock" fallback from __get_kvmclock() to get_kvmclock()

[Sean Christopherson]

Move the fallback logic for getting the current kvmclock when not in master
clock mode out of __get_kvmclock() and into its sole caller, get_kvmclock().
This will allow use early-return logic in the master clock code, without
having to resort to a do-while() loop and/or gotos.

No functional change intended.

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

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0c0bf4c14bc5..8666bd0337b8 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1651,12 +1651,16 @@ static unsigned long get_cpu_tsc_khz(void)
 }
 
 /* Called within read_seqcount_begin/retry for kvm->pvclock_sc.  */
-static void __get_kvmclock(struct kvm *kvm, struct kvm_clock_data *data)
+static bool __get_kvmclock_master_clock(struct kvm *kvm,
+					struct kvm_clock_data *data)
 {
 	struct kvm_arch *ka = &kvm->arch;
 	struct pvclock_vcpu_time_info hv_clock;
 	u64 tsc_hz;
 
+	if (!ka->use_master_clock)
+		return false;
+
 	/*
 	 * Snapshot and validate the TSC frequency as kvmclock_cpu_down_prep()
 	 * zeros the per-CPU value when a CPU is going offline.
@@ -1665,8 +1669,10 @@ static void __get_kvmclock(struct kvm *kvm, struct kvm_clock_data *data)
 	tsc_hz = (u64)get_cpu_tsc_khz() * HZ_PER_KHZ;
 	put_cpu();
 
-	data->flags = 0;
-	if (ka->use_master_clock && tsc_hz) {
+	if (!tsc_hz)
+		return false;
+
+	{
 #ifdef CONFIG_X86_64
 		struct timespec64 ts;
 
@@ -1684,8 +1690,6 @@ static void __get_kvmclock(struct kvm *kvm, struct kvm_clock_data *data)
 				   &hv_clock.tsc_shift,
 				   &hv_clock.tsc_to_system_mul);
 		data->clock = __pvclock_read_cycles(&hv_clock, data->host_tsc);
-	} else {
-		data->clock = get_kvmclock_base_ns() + ka->kvmclock_offset;
 	}
 }
 
@@ -1695,8 +1699,11 @@ static void get_kvmclock(struct kvm *kvm, struct kvm_clock_data *data)
 	unsigned seq;
 
 	do {
+		data->flags = 0;
+
 		seq = read_seqcount_begin(&ka->pvclock_sc);
-		__get_kvmclock(kvm, data);
+		if (!__get_kvmclock_master_clock(kvm, data))
+			data->clock = get_kvmclock_base_ns() + ka->kvmclock_offset;
 	} while (read_seqcount_retry(&ka->pvclock_sc, seq));
 }
 
-- 
2.55.0.571.g244d577d93-goog

[PATCH v8 08/17] KVM: x86: Wrap all of __get_kvmclock_master_clock() with CONFIG_X86_64=y

[Sean Christopherson]

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

Wrap the entire use_master_clock block in #ifdef CONFIG_X86_64, since
use_master_clock is never true on 32-bit (host_tsc_clocksource is only
set under CONFIG_X86_64), and declare hv_clock inside the block so it is
not left as an unused variable on 32-bit.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
[sean: keep only the CONFIG_X86_64 ifdef, update changelog accordingly]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8666bd0337b8..cd2a140e8e63 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1654,8 +1654,10 @@ static unsigned long get_cpu_tsc_khz(void)
 static bool __get_kvmclock_master_clock(struct kvm *kvm,
 					struct kvm_clock_data *data)
 {
+#ifdef CONFIG_X86_64
 	struct kvm_arch *ka = &kvm->arch;
 	struct pvclock_vcpu_time_info hv_clock;
+	struct timespec64 ts;
 	u64 tsc_hz;
 
 	if (!ka->use_master_clock)
@@ -1672,25 +1674,24 @@ static bool __get_kvmclock_master_clock(struct kvm *kvm,
 	if (!tsc_hz)
 		return false;
 
-	{
-#ifdef CONFIG_X86_64
-		struct timespec64 ts;
-
-		if (kvm_get_walltime_and_clockread(&ts, &data->host_tsc)) {
-			data->realtime = ts.tv_nsec + NSEC_PER_SEC * ts.tv_sec;
-			data->flags |= KVM_CLOCK_REALTIME | KVM_CLOCK_HOST_TSC;
-		} else
-#endif
+	if (kvm_get_walltime_and_clockread(&ts, &data->host_tsc)) {
+		data->realtime = ts.tv_nsec + NSEC_PER_SEC * ts.tv_sec;
+		data->flags |= KVM_CLOCK_REALTIME | KVM_CLOCK_HOST_TSC;
+	} else {
 		data->host_tsc = rdtsc();
-
-		data->flags |= KVM_CLOCK_TSC_STABLE;
-		hv_clock.tsc_timestamp = ka->master_cycle_now;
-		hv_clock.system_time = ka->master_kernel_ns + ka->kvmclock_offset;
-		kvm_get_time_scale(NSEC_PER_SEC,  tsc_hz,
-				   &hv_clock.tsc_shift,
-				   &hv_clock.tsc_to_system_mul);
-		data->clock = __pvclock_read_cycles(&hv_clock, data->host_tsc);
 	}
+
+	data->flags |= KVM_CLOCK_TSC_STABLE;
+	hv_clock.tsc_timestamp = ka->master_cycle_now;
+	hv_clock.system_time = ka->master_kernel_ns + ka->kvmclock_offset;
+	kvm_get_time_scale(NSEC_PER_SEC,  tsc_hz,
+				&hv_clock.tsc_shift,
+				&hv_clock.tsc_to_system_mul);
+	data->clock = __pvclock_read_cycles(&hv_clock, data->host_tsc);
+	return true;
+#else
+	return false;
+#endif
 }
 
 static void get_kvmclock(struct kvm *kvm, struct kvm_clock_data *data)
-- 
2.55.0.571.g244d577d93-goog

[PATCH v8 09/17] KVM: x86: Fall back to non-master-clock if clockread fails in get_kvmclock()

[Sean Christopherson]

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

When computing kvmclock and it's currently in master-clock mode, fall back
to the non-master-clock path if the clock read fails, e.g. if the kernel's
clocksource transitioning away from TSC but ka->use_master_clock hasn't
been updated yet.

The rdtsc() fallback was added (well, kept) in commit c68dc1b577ea ("KVM:
x86: Report host tsc and realtime values in KVM_GET_CLOCK") purely to avoid
uninitialized variables and compilation problems on 32-bit kernels (already
addressed).  In hindsight, keeping the rdtsc() was a hack and a mistake.

Link: https://lore.kernel.org/all/CAOQ_QsgVqS_PuJo8F10Gg5Xw+tKt+5gDx+kJf1j3CiPO4MAOqg@mail.gmail.com
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
[sean: isolate from refactoring changes, write changelog]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index cd2a140e8e63..7768a8909cf8 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1674,14 +1674,13 @@ static bool __get_kvmclock_master_clock(struct kvm *kvm,
 	if (!tsc_hz)
 		return false;
 
-	if (kvm_get_walltime_and_clockread(&ts, &data->host_tsc)) {
-		data->realtime = ts.tv_nsec + NSEC_PER_SEC * ts.tv_sec;
-		data->flags |= KVM_CLOCK_REALTIME | KVM_CLOCK_HOST_TSC;
-	} else {
-		data->host_tsc = rdtsc();
-	}
+	if (!kvm_get_walltime_and_clockread(&ts, &data->host_tsc))
+		return false;
+
+	data->realtime = ts.tv_nsec + NSEC_PER_SEC * ts.tv_sec;
+	data->flags |= KVM_CLOCK_REALTIME | KVM_CLOCK_HOST_TSC |
+		       KVM_CLOCK_TSC_STABLE;
 
-	data->flags |= KVM_CLOCK_TSC_STABLE;
 	hv_clock.tsc_timestamp = ka->master_cycle_now;
 	hv_clock.system_time = ka->master_kernel_ns + ka->kvmclock_offset;
 	kvm_get_time_scale(NSEC_PER_SEC,  tsc_hz,
-- 
2.55.0.571.g244d577d93-goog

[PATCH v8 10/17] KVM: x86: Fix KVM clock precision in get_kvmclock() with TSC scaling

[Sean Christopherson]

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

When in master clock mode, the KVM clock is defined in terms of the
guest TSC. But get_kvmclock() was computing it from the host TSC
without applying TSC scaling, leading to a systemic drift from the
values the guest computes from its own TSC.

Store the VM's TSC scaling ratio in kvm_arch and precompute the
guest-TSC-based mul/shift in pvclock_update_vm_gtod_copy(). Use these
in get_kvmclock() to scale the host TSC delta to guest TSC before
converting to nanoseconds.

This avoids "definition C" of the KVM clock described in commit
633d7652f80f ("KVM: x86/xen: Do not corrupt KVM clock in
kvm_xen_shared_info_init()").

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/kvm_host.h |  4 +++
 arch/x86/kvm/x86.c              | 61 ++++++++++++++++++++++++---------
 2 files changed, 48 insertions(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 6ddc988576d0..2878a5181cd9 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1236,6 +1236,7 @@ struct kvm_arch {
 	u64 last_tsc_write;
 	u32 last_tsc_khz;
 	u64 last_tsc_offset;
+	u64 last_tsc_scaling_ratio;
 	u64 cur_tsc_nsec;
 	u64 cur_tsc_write;
 	u64 cur_tsc_offset;
@@ -1251,6 +1252,9 @@ struct kvm_arch {
 	u64 master_kernel_ns;
 	u64 master_cycle_now;
 	struct ratelimit_state kvmclock_update_rs;
+	u64 master_tsc_scaling_ratio;
+	s8  master_tsc_shift;
+	u32 master_tsc_mul;
 
 #ifdef CONFIG_KVM_HYPERV
 	struct kvm_hv hyperv;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7768a8909cf8..f13100a7a63e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1247,6 +1247,7 @@ static void __kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 offset, u64 tsc,
 	kvm->arch.last_tsc_write = tsc;
 	kvm->arch.last_tsc_khz = vcpu->arch.virtual_tsc_khz;
 	kvm->arch.last_tsc_offset = offset;
+	kvm->arch.last_tsc_scaling_ratio = vcpu->arch.l1_tsc_scaling_ratio;
 
 	vcpu->arch.last_guest_tsc = tsc;
 
@@ -1559,6 +1560,8 @@ static bool kvm_get_walltime_and_clockread(struct timespec64 *ts,
  *
  */
 
+static unsigned long get_cpu_tsc_khz(void);
+
 static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
 {
 #ifdef CONFIG_X86_64
@@ -1582,9 +1585,30 @@ static void pvclock_update_vm_gtod_copy(struct kvm *kvm)
 				&& !ka->backwards_tsc_observed
 				&& !ka->boot_vcpu_runs_old_kvmclock;
 
-	if (ka->use_master_clock)
+	if (ka->use_master_clock) {
+		u64 tsc_hz;
+
 		atomic_set(&kvm_guest_has_master_clock, 1);
 
+		/*
+		 * Copy the scaling ratio and precompute the mul/shift for
+		 * converting guest TSC to nanoseconds. These are used by
+		 * get_kvmclock() to compute kvmclock from the host TSC
+		 * without needing a vCPU reference.
+		 */
+		ka->master_tsc_scaling_ratio = ka->last_tsc_scaling_ratio;
+		tsc_hz = (u64)get_cpu_tsc_khz() * HZ_PER_KHZ;
+		if (tsc_hz && kvm_caps.has_tsc_control)
+			tsc_hz = kvm_scale_tsc(tsc_hz,
+					       ka->master_tsc_scaling_ratio);
+		if (tsc_hz)
+			kvm_get_time_scale(NSEC_PER_SEC, tsc_hz,
+					   &ka->master_tsc_shift,
+					   &ka->master_tsc_mul);
+		else
+			ka->use_master_clock = false;
+	}
+
 	vclock_mode = pvclock_gtod_data.clock.vclock_mode;
 	trace_kvm_update_master_clock(ka->use_master_clock, vclock_mode,
 					vcpus_matched);
@@ -1658,22 +1682,10 @@ static bool __get_kvmclock_master_clock(struct kvm *kvm,
 	struct kvm_arch *ka = &kvm->arch;
 	struct pvclock_vcpu_time_info hv_clock;
 	struct timespec64 ts;
-	u64 tsc_hz;
 
 	if (!ka->use_master_clock)
 		return false;
 
-	/*
-	 * Snapshot and validate the TSC frequency as kvmclock_cpu_down_prep()
-	 * zeros the per-CPU value when a CPU is going offline.
-	 */
-	get_cpu();
-	tsc_hz = (u64)get_cpu_tsc_khz() * HZ_PER_KHZ;
-	put_cpu();
-
-	if (!tsc_hz)
-		return false;
-
 	if (!kvm_get_walltime_and_clockread(&ts, &data->host_tsc))
 		return false;
 
@@ -1683,10 +1695,25 @@ static bool __get_kvmclock_master_clock(struct kvm *kvm,
 
 	hv_clock.tsc_timestamp = ka->master_cycle_now;
 	hv_clock.system_time = ka->master_kernel_ns + ka->kvmclock_offset;
-	kvm_get_time_scale(NSEC_PER_SEC,  tsc_hz,
-				&hv_clock.tsc_shift,
-				&hv_clock.tsc_to_system_mul);
-	data->clock = __pvclock_read_cycles(&hv_clock, data->host_tsc);
+
+	/*
+	 * Use the precomputed guest-TSC-based mul/shift so that the kvmclock
+	 * value matches what the guest computes from its own TSC.
+	 */
+	hv_clock.tsc_shift = ka->master_tsc_shift;
+	hv_clock.tsc_to_system_mul = ka->master_tsc_mul;
+
+	if (kvm_caps.has_tsc_control) {
+		u64 tsc_delta = data->host_tsc - ka->master_cycle_now;
+
+		tsc_delta = kvm_scale_tsc(tsc_delta, ka->master_tsc_scaling_ratio);
+		data->clock = hv_clock.system_time +
+			      pvclock_scale_delta(tsc_delta,
+						  hv_clock.tsc_to_system_mul,
+						  hv_clock.tsc_shift);
+	} else {
+		data->clock = __pvclock_read_cycles(&hv_clock, data->host_tsc);
+	}
 	return true;
 #else
 	return false;
-- 
2.55.0.571.g244d577d93-goog

[PATCH v8 11/17] KVM: x86: Use get_kvmclock() in kvm_get_wall_clock_epoch()

[Sean Christopherson]

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

Now that get_kvmclock() correctly handles TSC scaling and captures both
wallclock and kvmclock from the same TSC reading,
kvm_get_wall_clock_epoch() can simply call it instead of duplicating
the pvclock computation.

This eliminates the last instance of the "definition C" kvmclock
calculation — as described in commit 633d7652f80f ("KVM: x86/xen: Do not
corrupt KVM clock in kvm_xen_shared_info_init()") — which computed
nanoseconds directly from the host TSC without accounting for guest TSC
scaling.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 59 +++++++---------------------------------------
 1 file changed, 9 insertions(+), 50 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f13100a7a63e..ec39ca82633b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1939,63 +1939,22 @@ int kvm_guest_time_update(struct kvm_vcpu *v)
  * wallclock and kvmclock times, and subtracting one from the other.
  *
  * Fall back to using their values at slightly different moments by
- * calling ktime_get_real_ns() and get_kvmclock_ns() separately.
+ * calling ktime_get_real_ns() and get_kvmclock() separately.
  */
 uint64_t kvm_get_wall_clock_epoch(struct kvm *kvm)
 {
-#ifdef CONFIG_X86_64
-	struct pvclock_vcpu_time_info hv_clock;
-	struct kvm_arch *ka = &kvm->arch;
-	unsigned long seq, local_tsc_khz;
-	struct timespec64 ts;
-	uint64_t host_tsc;
+	struct kvm_clock_data data;
 
-	do {
-		seq = read_seqcount_begin(&ka->pvclock_sc);
-
-		local_tsc_khz = 0;
-		if (!ka->use_master_clock)
-			break;
-
-		/*
-		 * The TSC read and the call to get_cpu_tsc_khz() must happen
-		 * on the same CPU.
-		 */
-		get_cpu();
-
-		local_tsc_khz = get_cpu_tsc_khz();
-
-		if (local_tsc_khz &&
-		    !kvm_get_walltime_and_clockread(&ts, &host_tsc))
-			local_tsc_khz = 0; /* Fall back to old method */
-
-		put_cpu();
-
-		/*
-		 * These values must be snapshotted within the seqcount loop.
-		 * After that, it's just mathematics which can happen on any
-		 * CPU at any time.
-		 */
-		hv_clock.tsc_timestamp = ka->master_cycle_now;
-		hv_clock.system_time = ka->master_kernel_ns + ka->kvmclock_offset;
-
-	} while (read_seqcount_retry(&ka->pvclock_sc, seq));
+	get_kvmclock(kvm, &data);
 
 	/*
-	 * If the conditions were right, and obtaining the wallclock+TSC was
-	 * successful, calculate the KVM clock at the corresponding time and
-	 * subtract one from the other to get the guest's epoch in nanoseconds
-	 * since 1970-01-01.
+	 * If get_kvmclock() captured both wallclock and kvmclock from the
+	 * same TSC reading, use them for a precise epoch calculation.
 	 */
-	if (local_tsc_khz) {
-		kvm_get_time_scale(NSEC_PER_SEC, local_tsc_khz * NSEC_PER_USEC,
-				   &hv_clock.tsc_shift,
-				   &hv_clock.tsc_to_system_mul);
-		return ts.tv_nsec + NSEC_PER_SEC * ts.tv_sec -
-			__pvclock_read_cycles(&hv_clock, host_tsc);
-	}
-#endif
-	return ktime_get_real_ns() - get_kvmclock_ns(kvm);
+	if (data.flags & KVM_CLOCK_REALTIME)
+		return data.realtime - data.clock;
+
+	return ktime_get_real_ns() - data.clock;
 }
 
 /*
-- 
2.55.0.571.g244d577d93-goog

[PATCH v8 12/17] KVM: x86: Fix compute_guest_tsc() to handle negative time deltas

[Sean Christopherson]

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

The compute_guest_tsc() function computes the guest TSC at a given
kernel_ns timestamp. When the master clock reference point
(master_kernel_ns) is earlier than vcpu->arch.this_tsc_nsec, the delta
is negative. Since pvclock_scale_delta() takes a u64, the negative
value wraps to a huge positive number, producing a wildly wrong result.

Handle negative deltas explicitly by scaling the absolute value of the
delta and applying it to this_tsc_write with the appropriate sign.

This is believed to be unreachable in practice; no path has been
identified which invokes compute_guest_tsc() with a timestamp from
before the vCPU's TSC generation was established. Fix it for
robustness, in the spirit of defence in depth.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ec39ca82633b..5667cd17672b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1054,11 +1054,15 @@ static int kvm_set_tsc_khz(struct kvm_vcpu *vcpu, u32 user_tsc_khz)
 
 static u64 compute_guest_tsc(struct kvm_vcpu *vcpu, s64 kernel_ns)
 {
-	u64 tsc = pvclock_scale_delta(kernel_ns-vcpu->arch.this_tsc_nsec,
-				      vcpu->arch.virtual_tsc_mult,
-				      vcpu->arch.virtual_tsc_shift);
-	tsc += vcpu->arch.this_tsc_write;
-	return tsc;
+	s64 delta_ns = kernel_ns - vcpu->arch.this_tsc_nsec;
+	u64 tsc;
+
+	/* Handle negative deltas gracefully (master clock ref may be earlier) */
+	tsc = pvclock_scale_delta(abs(delta_ns),
+				  vcpu->arch.virtual_tsc_mult,
+				  vcpu->arch.virtual_tsc_shift);
+
+	return vcpu->arch.this_tsc_write + (delta_ns >= 0 ? tsc : -tsc);
 }
 
 #ifdef CONFIG_X86_64
-- 
2.55.0.571.g244d577d93-goog

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

[Sean Christopherson]

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

[PATCH v8 14/17] KVM: x86: Make master clock logic in guest PV clock updates 64-bit only

[Sean Christopherson]

Wrap kvm_guest_time_update()'s entire use_master_clock block in #ifdef
CONFIG_X86_64 and providing a hardcoded-to-false variable for the 32-bit
path, as use_master_clock is never true on 32-bit (host_tsc_clocksource is
only set under CONFIG_X86_64)

Keep the local "ka" variable outside of the 64-bit-only code as it's also
used for Xen code, but tag it as potentially unused since it's not used on
32-bit kernels without Xen emulation.

For all intents and purposes, no functional change intended.

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

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 63702be799cc..070a12535788 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1796,22 +1796,21 @@ static void kvm_setup_guest_pvclock(struct pvclock_vcpu_time_info *ref_hv_clock,
 
 int kvm_guest_time_update(struct kvm_vcpu *v)
 {
+	struct kvm_arch *ka __maybe_unused = &v->kvm->arch;
 	struct pvclock_vcpu_time_info hv_clock = {};
 	u64 tgt_tsc_hz;
-	unsigned seq;
 	struct kvm_vcpu_arch *vcpu = &v->arch;
-	struct kvm_arch *ka = &v->kvm->arch;
 	s64 kernel_ns;
 	u64 tsc_timestamp, host_tsc;
+
+	/*
+	 * If the host uses TSC clock, then passthrough TSC as stable
+	 * to the guest.
+	 */
+#ifdef CONFIG_X86_64
 	bool use_master_clock;
+	unsigned int seq;
 
-	kernel_ns = 0;
-	host_tsc = 0;
-
-	/*
-	 * If the host uses TSC clock, then passthrough TSC as stable
-	 * to the guest.
-	 */
 	do {
 		seq = read_seqcount_begin(&ka->pvclock_sc);
 		use_master_clock = ka->use_master_clock;
@@ -1820,7 +1819,9 @@ int kvm_guest_time_update(struct kvm_vcpu *v)
 			kernel_ns = ka->master_kernel_ns;
 		}
 	} while (read_seqcount_retry(&ka->pvclock_sc, seq));
-
+#else
+	const bool use_master_clock = false;
+#endif
 	/*
 	 * 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.
-- 
2.55.0.571.g244d577d93-goog

[PATCH v8 15/17] KVM: x86: Upscale TSC to "now", not master clock when updating PV clocks

[Sean Christopherson]

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

When doing TSC upscaling in software, e.g. when the guest TSC frequency is
configured to run faster than the host without hardware support, always
advance guest TSC to "now", not to the master clock's reference values,
which represents some time in the past.

In master clock mode, there are two points of time which need to be taken
into account. First there is the master clock reference point, stored in
kvm->arch.master_kernel_ns (and associated host TSC ->master_cycle_now).
Secondly, there is the time *now*, at the point kvm_update_guest_time()
is being called.

With software TSC upscaling, the guest TSC is getting further and further
ahead of the host TSC as time elapses.  So at time "now", the guest TSC
should be further ahead of the host, than it was at master_kernel_ns.
Not taking that into account means KVM was advancing the guest TSC only by
the amount appropriate for the snapshot taken in the past, *not* to the
current time (which is what RDTSC is supposed to return).

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Co-developed-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 070a12535788..cd438d06e4cc 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1796,12 +1796,11 @@ static void kvm_setup_guest_pvclock(struct pvclock_vcpu_time_info *ref_hv_clock,
 
 int kvm_guest_time_update(struct kvm_vcpu *v)
 {
+	u64 tgt_tsc_hz, tsc_timestamp, host_tsc, master_tsc, master_ns;
 	struct kvm_arch *ka __maybe_unused = &v->kvm->arch;
 	struct pvclock_vcpu_time_info hv_clock = {};
-	u64 tgt_tsc_hz;
 	struct kvm_vcpu_arch *vcpu = &v->arch;
 	s64 kernel_ns;
-	u64 tsc_timestamp, host_tsc;
 
 	/*
 	 * If the host uses TSC clock, then passthrough TSC as stable
@@ -1814,10 +1813,16 @@ int kvm_guest_time_update(struct kvm_vcpu *v)
 	do {
 		seq = read_seqcount_begin(&ka->pvclock_sc);
 		use_master_clock = ka->use_master_clock;
-		if (use_master_clock) {
-			host_tsc = ka->master_cycle_now;
-			kernel_ns = ka->master_kernel_ns;
+		if (!use_master_clock)
+			continue;
+
+		if (!kvm_get_time_and_clockread(&kernel_ns, &host_tsc)) {
+			use_master_clock = false;
+			continue;
 		}
+
+		master_tsc = ka->master_cycle_now;
+		master_ns = ka->master_kernel_ns;
 	} while (read_seqcount_retry(&ka->pvclock_sc, seq));
 #else
 	const bool use_master_clock = false;
@@ -1883,8 +1888,18 @@ int kvm_guest_time_update(struct kvm_vcpu *v)
 
 	hv_clock.tsc_shift = vcpu->pvclock_tsc_shift;
 	hv_clock.tsc_to_system_mul = vcpu->pvclock_tsc_mul;
-	hv_clock.tsc_timestamp = tsc_timestamp;
-	hv_clock.system_time = kernel_ns + v->kvm->arch.kvmclock_offset;
+	/*
+	 * If the master clock is NOT in use, the reference time placed in the
+	 * hv_clock is "now".  If master clock is in use, the reference time is
+	 * the master clock's snapshot from some time in the past, not "now".
+	 */
+	if (use_master_clock) {
+		hv_clock.tsc_timestamp = kvm_read_l1_tsc(v, master_tsc);
+		hv_clock.system_time = master_ns + v->kvm->arch.kvmclock_offset;
+	} else {
+		hv_clock.tsc_timestamp = tsc_timestamp;
+		hv_clock.system_time = kernel_ns + v->kvm->arch.kvmclock_offset;
+	}
 
 	/* If the host uses TSC clocksource, then it is stable */
 	hv_clock.flags = 0;
-- 
2.55.0.571.g244d577d93-goog

[PATCH v8 16/17] KVM: x86: Simplify and comment kvm_get_time_scale()

[Sean Christopherson]

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

The kvm_get_time_scale() function was entirely opaque. Add comments
explaining what it does: compute a fixed-point multiplier and shift for
converting TSC ticks to nanoseconds via pvclock_scale_delta().

Rename the local variables from the cryptic tps64/tps32/scaled64 to
base_hz_u64/base32/scaled_hz_u64 to make the code self-documenting.
The "tps32" name stood for "Ticks Per Second" but was misleading since
it held the shifted base frequency, not a tick count.

No functional change.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Paul Durrant <paul@xen.org>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 55 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index cd438d06e4cc..02c389523f52 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -938,32 +938,57 @@ static uint32_t div_frac(uint32_t dividend, uint32_t divisor)
 	return dividend;
 }
 
-static void kvm_get_time_scale(uint64_t scaled_hz, uint64_t base_hz,
+static void kvm_get_time_scale(u64 scaled_hz, u64 base_hz,
 			       s8 *pshift, u32 *pmultiplier)
 {
-	uint64_t scaled64;
-	int32_t  shift = 0;
-	uint64_t tps64;
-	uint32_t tps32;
+	u64 scaled_hz_u64 = scaled_hz;
+	s32 shift = 0;
+	u64 base_hz_u64;
+	u32 base32;
 
-	tps64 = base_hz;
-	scaled64 = scaled_hz;
-	while (tps64 > scaled64*2 || tps64 & 0xffffffff00000000ULL) {
-		tps64 >>= 1;
+	/*
+	 * This function calculates a fixed-point multiplier and shift such
+	 * that:
+	 *   time_ns = (tsc_cycles << shift) * multiplier >> 32
+	 *
+	 * Where tsc_cycles tick at base_hz, and time_ns should count at
+	 * scaled_hz (typically NSEC_PER_SEC for a TSC→nanoseconds conversion).
+	 *
+	 * The multiplier is: (scaled_hz << 32) / base_hz, adjusted by shift
+	 * to keep everything in range.
+	 */
+
+	base_hz_u64 = base_hz;
+
+	/*
+	 * Start by shifting base_hz right until it fits in 32 bits, and
+	 * is lower than double the target rate. This introduces a negative
+	 * shift value which would result in pvclock_scale_delta() shifting
+	 * the actual tick count right before performing the multiplication.
+	 */
+	while (base_hz_u64 > scaled_hz_u64 * 2 || base_hz_u64 >> 32) {
+		base_hz_u64 >>= 1;
 		shift--;
 	}
 
-	tps32 = (uint32_t)tps64;
-	while (tps32 <= scaled64 || scaled64 & 0xffffffff00000000ULL) {
-		if (scaled64 & 0xffffffff00000000ULL || tps32 & 0x80000000)
-			scaled64 >>= 1;
+	/* Now the shifted base_hz fits in 32 bits. */
+	base32 = (u32)base_hz_u64;
+
+	/*
+	 * Next, shift scaled_hz right until it fits in 32 bits, and ensure
+	 * that the shifted base_hz is strictly larger (so that the result of the
+	 * final division also fits in 32 bits).
+	 */
+	while (base32 <= scaled_hz_u64 || scaled_hz_u64 >> 32) {
+		if (scaled_hz_u64 >> 32 || base32 & BIT(31))
+			scaled_hz_u64 >>= 1;
 		else
-			tps32 <<= 1;
+			base32 <<= 1;
 		shift++;
 	}
 
 	*pshift = shift;
-	*pmultiplier = div_frac(scaled64, tps32);
+	*pmultiplier = div_frac(scaled_hz_u64, base32);
 }
 
 #ifdef CONFIG_X86_64
-- 
2.55.0.571.g244d577d93-goog

[PATCH v8 17/17] KVM: x86: Remove implicit rdtsc() from kvm_compute_l1_tsc_offset()

[Sean Christopherson]

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

Let the callers pass the host TSC value in as an explicit parameter.
This leaves some fairly obviously stupid code, which is using this
function to compare the guest TSC at some *other* time, with the
newly-minted TSC value from rdtsc(). Unless it's being used to measure
*elapsed* time, that isn't very sensible.

In this case, "obviously stupid" is an improvement over being
non-obviously so.

No functional change intended.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Paul Durrant <paul@xen.org>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/msrs.c |  3 ++-
 arch/x86/kvm/x86.c  | 11 ++++++-----
 arch/x86/kvm/x86.h  |  3 ++-
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
index 66fa7140d65d..22ceb39d41f6 100644
--- a/arch/x86/kvm/msrs.c
+++ b/arch/x86/kvm/msrs.c
@@ -1631,7 +1631,8 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		if (msr_info->host_initiated) {
 			kvm_synchronize_tsc(vcpu, &data);
 		} else if (!vcpu->arch.guest_tsc_protected) {
-			u64 adj = kvm_compute_l1_tsc_offset(vcpu, data) - vcpu->arch.l1_tsc_offset;
+			u64 adj = kvm_compute_l1_tsc_offset(vcpu, rdtsc(), data) -
+				  vcpu->arch.l1_tsc_offset;
 			adjust_tsc_offset_guest(vcpu, adj);
 			vcpu->arch.ia32_tsc_adjust_msr += adj;
 		}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 02c389523f52..cbe4fd3457b4 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1153,11 +1153,12 @@ u64 kvm_scale_tsc(u64 tsc, u64 ratio)
 	return _tsc;
 }
 
-u64 kvm_compute_l1_tsc_offset(struct kvm_vcpu *vcpu, u64 target_tsc)
+u64 kvm_compute_l1_tsc_offset(struct kvm_vcpu *vcpu, u64 host_tsc,
+			      u64 target_tsc)
 {
 	u64 tsc;
 
-	tsc = kvm_scale_tsc(rdtsc(), vcpu->arch.l1_tsc_scaling_ratio);
+	tsc = kvm_scale_tsc(host_tsc, vcpu->arch.l1_tsc_scaling_ratio);
 
 	return target_tsc - tsc;
 }
@@ -1319,7 +1320,7 @@ void kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 *user_value)
 	bool synchronizing = false;
 
 	raw_spin_lock_irqsave(&kvm->arch.tsc_write_lock, flags);
-	offset = kvm_compute_l1_tsc_offset(vcpu, data);
+	offset = kvm_compute_l1_tsc_offset(vcpu, rdtsc(), data);
 	ns = get_kvmclock_base_ns();
 	elapsed = ns - kvm->arch.last_tsc_nsec;
 
@@ -1368,7 +1369,7 @@ void kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 *user_value)
 		} else {
 			u64 delta = nsec_to_cycles(vcpu, elapsed);
 			data += delta;
-			offset = kvm_compute_l1_tsc_offset(vcpu, data);
+			offset = kvm_compute_l1_tsc_offset(vcpu, rdtsc(), data);
 		}
 		matched = true;
 	}
@@ -2625,7 +2626,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 			mark_tsc_unstable("KVM discovered backwards TSC");
 
 		if (kvm_check_tsc_unstable()) {
-			u64 offset = kvm_compute_l1_tsc_offset(vcpu,
+			u64 offset = kvm_compute_l1_tsc_offset(vcpu, rdtsc(),
 						vcpu->arch.last_guest_tsc);
 			kvm_vcpu_write_tsc_offset(vcpu, offset);
 			if (!vcpu->arch.guest_tsc_protected)
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 0f5919b092e4..1d9a66048b01 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -335,7 +335,8 @@ u64 kvm_scale_tsc(u64 tsc, u64 ratio);
 u64 kvm_read_l1_tsc(struct kvm_vcpu *vcpu, u64 host_tsc);
 u64 kvm_calc_nested_tsc_offset(u64 l1_offset, u64 l2_offset, u64 l2_multiplier);
 u64 kvm_calc_nested_tsc_multiplier(u64 l1_multiplier, u64 l2_multiplier);
-u64 kvm_compute_l1_tsc_offset(struct kvm_vcpu *vcpu, u64 target_tsc);
+u64 kvm_compute_l1_tsc_offset(struct kvm_vcpu *vcpu, u64 host_tsc,
+			      u64 target_tsc);
 void kvm_vcpu_write_tsc_offset(struct kvm_vcpu *vcpu, u64 l1_offset);
 
 static inline void adjust_tsc_offset_guest(struct kvm_vcpu *vcpu,
-- 
2.55.0.571.g244d577d93-goog

Re: [PATCH v8 07/17] KVM: x86: Move "no master clock" fallback from __get_kvmclock() to get_kvmclock()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The function `__get_kvmclock_master_clock()` is missing a `return true;` statement on its success path, causing it to return an undefined boolean value.
--

--- Patch [7]: [PATCH v8 07/17] KVM: x86: Move "no master clock" fallback from __get_kvmclock() to get_kvmclock() ---
commit cb25993776f1881f5ff66de261984bd8992ceece
Author: Sean Christopherson <seanjc@google.com>

KVM: x86: Move "no master clock" fallback from __get_kvmclock() to get_kvmclock()

This commit refactors the kvmclock retrieval logic by extracting the master
clock fallback out of the inner helper and changing it to return a boolean.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b520928579129..7366d4602f89f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -1667,8 +1673,6 @@ static void __get_kvmclock(struct kvm *kvm, struct kvm_clock_data *data)
>  				   &hv_clock.tsc_shift,
>  				   &hv_clock.tsc_to_system_mul);
>  		data->clock = __pvclock_read_cycles(&hv_clock, data->host_tsc);
> -	} else {
> -		data->clock = get_kvmclock_base_ns() + ka->kvmclock_offset;
>  	}
>  }

[Severity: High]
Does this function need to return true on the success path?

Since __get_kvmclock_master_clock() was changed to return a bool, it returns
false on the early exit paths, but appears to fall off the end of the
function without explicitly returning a value on success.

Since the return value here is undefined, could it evaluate to false in the
caller? If so, get_kvmclock() would incorrectly trigger the fallback logic:

get_kvmclock()
{
        if (!__get_kvmclock_master_clock(kvm, data))
                data->clock = get_kvmclock_base_ns() + ka->kvmclock_offset;
}

This could overwrite the accurately calculated data->clock with the base
offset calculation, while leaving data->flags populated with master clock
flags.

>  
> @@ -1678,8 +1682,11 @@ static void get_kvmclock(struct kvm *kvm, struct kvm_clock_data *data)
>  	unsigned seq;
>  
>  	do {
> +		data->flags = 0;
> +
>  		seq = read_seqcount_begin(&ka->pvclock_sc);
> -		__get_kvmclock(kvm, data);
> +		if (!__get_kvmclock_master_clock(kvm, data))
> +			data->clock = get_kvmclock_base_ns() + ka->kvmclock_offset;
>  	} while (read_seqcount_retry(&ka->pvclock_sc, seq));
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804233923.3504629-1-seanjc@google.com?part=7

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Replacing `local_irq_save()` with `preempt_disable()` allows hardware interrupts to break the temporal atomicity of clock reads, leading to guest time corruption.
--

--- Patch [13]: [PATCH v8 13/17] KVM: x86: Disable preemption, not IRQs, when getting TSC+freq pair ---
commit a5cd5356feb83302b6464039e18ebbb2f4d759f9
Author: Sean Christopherson <seanjc@google.com>

KVM: x86: Disable preemption, not IRQs, when getting TSC+freq pair

This commit replaces local_irq_save() with preempt_disable() in
kvm_guest_time_update() when reading the TSC and frequency pair. The
intention is to avoid overkill IRQ disabling, as preemption disabling is
sufficient to prevent task migration across CPUs during get_cpu_tsc_khz().

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fa00799ba13da..e0e8c6883b831 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
[ ... ]
> @@ -1805,11 +1804,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;
>  	}

[Severity: High]
Could replacing local_irq_save() with preempt_disable() allow hardware
interrupts to break the temporal atomicity of the fallback clock reads?

If KVM falls back to the non-master clock path just below this code,
host_tsc and kernel_ns are read sequentially:

        if (!use_master_clock) {
                host_tsc = rdtsc();
                kernel_ns = get_kvmclock_base_ns();
        }

If an interrupt fires between these two reads, it would cause kernel_ns
to be sampled later than host_tsc.

Could this inflated kernel_ns then falsely trigger a catch-up adjustment
later in kvm_guest_time_update():

        if (vcpu->tsc_catchup) {
                u64 tsc = compute_guest_tsc(v, kernel_ns);
                if (tsc > tsc_timestamp) {
                        adjust_tsc_offset_guest(v, tsc - tsc_timestamp);
                        tsc_timestamp = tsc;
                }
        }

and permanently advance the guest's TSC offset or cause temporary time freezes?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804233923.3504629-1-seanjc@google.com?part=13

Re: [PATCH v8 05/17] KVM: x86: Avoid NTP frequency skew for KVM clock on 32-bit host

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Unprotected 64-bit read of `pvclock_gtod_data.offs_boot` introduces a strict C11 data race and will trigger KCSAN warnings on 32-bit systems, though functional tearing is prevented by the freezer subsystem.
--

--- Patch [5]: [PATCH v8 05/17] KVM: x86: Avoid NTP frequency skew for KVM clock on 32-bit host ---
commit f1da2e2df5faddd4d0dcc9a823e72cc243ff1f13
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: x86: Avoid NTP frequency skew for KVM clock on 32-bit host
    
This commit removes the master clock check restriction for 32-bit hosts,
enabling the clocksource notifier for 32-bit builds so get_kvmclock_base_ns()
is unaffected by NTP sync.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 3107878a6fe5d..8f469fd9863f9 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -915,13 +914,6 @@ static s64 get_kvmclock_base_ns(void)
>  	/* Count up from boot time, but with the frequency of the raw clock.  */
>  	return ktime_to_ns(ktime_add(ktime_get_raw(), pvclock_gtod_data.offs_boot));

[Severity: Low]
Will reading the 64-bit pvclock_gtod_data.offs_boot without seqcount
protection or data_race() annotations trigger KCSAN data race warnings on
32-bit systems?

By removing the ktime_get_boottime_ns() fallback, this read now executes on
32-bit architectures where it compiles to two non-atomic 32-bit accesses.
If a KVM vCPU thread calls get_kvmclock_base_ns() while a timer interrupt
runs timekeeping_update(), it overwrites offs_boot.

Even though the value only actually changes during suspend when the freezer
subsystem guarantees vCPU threads are frozen (preventing functional tearing),
overwriting the identical value concurrently with an unprotected read still
introduces a formal C11 data race.

> -#else
> -static s64 get_kvmclock_base_ns(void)
> -{
> -	/* Master clock not used, so we can just use CLOCK_BOOTTIME.  */
> -	return ktime_get_boottime_ns();
> -}
> -#endif
>  
>  static uint32_t div_frac(uint32_t dividend, uint32_t divisor)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804233923.3504629-1-seanjc@google.com?part=5

Re: [PATCH v8 04/17] KVM: x86: Activate master clock immediately on vCPU creation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Checking the boolean `!vcpu->kvm->arch.use_master_clock` instead of consuming the pending KVM request causes O(N^2) IPI storms on unstable TSC hosts and stale clock data when vCPUs have mismatched TSCs.
--

--- Patch [4]: [PATCH v8 04/17] KVM: x86: Activate master clock immediately on vCPU creation ---
commit 5f130615aa4f4dff0ab2f9d96232c00ec3b489b7
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: x86: Activate master clock immediately on vCPU creation

This commit updates kvm_arch_vcpu_postcreate() to activate the master
clock immediately upon vCPU creation instead of waiting for the first
KVM_RUN. This allows userspace to retrieve a valid host_tsc and realtime
pair via KVM_GET_CLOCK without needing a dummy vCPU run.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9390,6 +9390,8 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>  		return;
>  	vcpu_load(vcpu);
>  	kvm_synchronize_tsc(vcpu, NULL);
> +	if (!vcpu->kvm->arch.use_master_clock)
> +		kvm_update_masterclock(vcpu->kvm);

[Severity: High]
Instead of checking the boolean use_master_clock, should this check and
consume the pending KVM_REQ_MASTERCLOCK_UPDATE request via kvm_check_request?

If the host has an unstable TSC, use_master_clock remains false. Won't this
unconditionally call kvm_update_masterclock() for every vCPU creation,
resulting in O(N^2) iterations and an IPI storm to running vCPUs during
hotplug?

Additionally, if a newly created vCPU has a mismatched TSC frequency,
kvm_synchronize_tsc() correctly queues KVM_REQ_MASTERCLOCK_UPDATE to
disable the master clock. Because use_master_clock is currently true,
this boolean check bypasses the update.

Could this bypass leave the master clock inappropriately enabled and the
request pending, causing KVM_GET_CLOCK to return stale data to userspace
before the first KVM_RUN?

>  	vcpu_put(vcpu);
>  
>  	/* poll control enabled by default */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804233923.3504629-1-seanjc@google.com?part=4

Re: [PATCH v8 04/17] KVM: x86: Activate master clock immediately on vCPU creation

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1720 bytes --]

On Wed, 2026-08-05 at 00:06 +0000, sashiko-bot@kernel.org wrote:
> [Severity: High]
> Instead of checking the boolean use_master_clock, should this check and
> consume the pending KVM_REQ_MASTERCLOCK_UPDATE request via kvm_check_request?

Oh, all right then :)

Sean suggested exactly that in a previous round¹ and I said I thought
I'd tried it and the request wasn't set. Having paged back in some of
that context... the thing which didn't work was a different variant —
deferring with a bare kvm_make_request()², which fails because
KVM_GET_CLOCK doesn't process vCPU requests, so calling it before the
first KVM_RUN still didn't work. That isn't what's being suggested
here.

With the code as it stands in this series, the first vCPU's
kvm_synchronize_tsc() reliably queues KVM_REQ_MASTERCLOCK_UPDATE via
the use_master_clock toggle in kvm_track_tsc_matching(). So

	if (kvm_check_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu))
		kvm_update_masterclock(vcpu->kvm);

should work, and is better than checking the boolean: it avoids the
repeated masterclock update that Sashiko is whining about, while still
honouring the case where a mismatched new vCPU should *disable* the
master clock.

By the end of my full series, this call gets removed entirely in favour
of setting up the whole clock epoch from kvm_arch_init_vm()³, which is
why I didn't care much before. But that's now on the far side of the
part 1 / part 2 split, so it's worth fixing here.

¹ https://lore.kernel.org/all/amPW1eAPpUuxLoj_@google.com/
² https://lore.kernel.org/all/58c5a521b94e8d90e101602cfe160d1fb128a5dd.camel@infradead.org/
³ https://lore.kernel.org/all/20260728144954.355376-29-dwmw2@infradead.org/

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 6179 bytes --]

Re: [PATCH v8 04/17] KVM: x86: Activate master clock immediately on vCPU creation

[Sean Christopherson]

On Wed, Aug 05, 2026, David Woodhouse wrote:
> On Wed, 2026-08-05 at 00:06 +0000, sashiko-bot@kernel.org wrote:
> > [Severity: High]
> > Instead of checking the boolean use_master_clock, should this check and
> > consume the pending KVM_REQ_MASTERCLOCK_UPDATE request via kvm_check_request?
> 
> Oh, all right then :)

LOL.

> Sean suggested exactly that in a previous round¹ and I said I thought
> I'd tried it and the request wasn't set. Having paged back in some of
> that context... the thing which didn't work was a different variant —
> deferring with a bare kvm_make_request()², which fails because
> KVM_GET_CLOCK doesn't process vCPU requests, so calling it before the
> first KVM_RUN still didn't work. That isn't what's being suggested
> here.
> 
> With the code as it stands in this series, the first vCPU's
> kvm_synchronize_tsc() reliably queues KVM_REQ_MASTERCLOCK_UPDATE via
> the use_master_clock toggle in kvm_track_tsc_matching(). So
> 
> 	if (kvm_check_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu))
> 		kvm_update_masterclock(vcpu->kvm);
> 
> should work, and is better than checking the boolean: it avoids the
> repeated masterclock update that Sashiko is whining about, while still
> honouring the case where a mismatched new vCPU should *disable* the
> master clock.

Roger that, I'll update for v9.

> By the end of my full series, this call gets removed entirely in favour
> of setting up the whole clock epoch from kvm_arch_init_vm()³, which is
> why I didn't care much before. But that's now on the far side of the
> part 1 / part 2 split, so it's worth fixing here.
> 
> ¹ https://lore.kernel.org/all/amPW1eAPpUuxLoj_@google.com/
> ² https://lore.kernel.org/all/58c5a521b94e8d90e101602cfe160d1fb128a5dd.camel@infradead.org/
> ³ https://lore.kernel.org/all/20260728144954.355376-29-dwmw2@infradead.org/

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

[Sean Christopherson]

+David

On Tue, Aug 04, 2026, sashiko-bot@kernel.org wrote:
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index fa00799ba13da..e0e8c6883b831 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> [ ... ]
> > @@ -1805,11 +1804,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;
> >  	}
> 
> [Severity: High]
> Could replacing local_irq_save() with preempt_disable() allow hardware
> interrupts to break the temporal atomicity of the fallback clock reads?
> 
> If KVM falls back to the non-master clock path just below this code,
> host_tsc and kernel_ns are read sequentially:
> 
>         if (!use_master_clock) {
>                 host_tsc = rdtsc();
>                 kernel_ns = get_kvmclock_base_ns();
>         }
> 
> If an interrupt fires between these two reads, it would cause kernel_ns
> to be sampled later than host_tsc.
> 
> Could this inflated kernel_ns then falsely trigger a catch-up adjustment
> later in kvm_guest_time_update():
> 
>         if (vcpu->tsc_catchup) {
>                 u64 tsc = compute_guest_tsc(v, kernel_ns);
>                 if (tsc > tsc_timestamp) {
>                         adjust_tsc_offset_guest(v, tsc - tsc_timestamp);
>                         tsc_timestamp = tsc;
>                 }
>         }
> 
> and permanently advance the guest's TSC offset or cause temporary time freezes?

Hmm, yes?  Though it's worth pointing out that NMIs can introduce the same issue,
and that's not really a solvable problem.

And if we keep the "disable IRQs" logic, then I think we should also disable IRQs
when doing kvm_get_time_and_clockread() for the same reasons, because that too
grabs a TSC+nanoseconds pair.  And that would mean disabling IRQs in get_kvmclock(),
so that kvm_vm_ioctl_get_clock() in particular provides an atomic-as-possible pair.

David, any thoughts?  I'm leaning towards keeping IRQs disabled to minimize the
chances of introducing a regression, even though I highly doubt disabling IRQs
to provide an atomic-ish pair was ever done deliberately.  My main concern with
disabling IRQs is that it will further muddy the waters with respect to what is
actually necessary, versus weird things KVM does for historical reasons.  Though
that can largely be solved with a verbose changelog.

Re: [PATCH v8 07/17] KVM: x86: Move "no master clock" fallback from __get_kvmclock() to get_kvmclock()

[Sean Christopherson]

On Tue, Aug 04, 2026, sashiko-bot@kernel.org wrote:
> commit cb25993776f1881f5ff66de261984bd8992ceece
> Author: Sean Christopherson <seanjc@google.com>
> 
> KVM: x86: Move "no master clock" fallback from __get_kvmclock() to get_kvmclock()
> 
> This commit refactors the kvmclock retrieval logic by extracting the master
> clock fallback out of the inner helper and changing it to return a boolean.
> 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index b520928579129..7366d4602f89f 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> 
> [ ... ]
> 
> > @@ -1667,8 +1673,6 @@ static void __get_kvmclock(struct kvm *kvm, struct kvm_clock_data *data)
> >  				   &hv_clock.tsc_shift,
> >  				   &hv_clock.tsc_to_system_mul);
> >  		data->clock = __pvclock_read_cycles(&hv_clock, data->host_tsc);
> > -	} else {
> > -		data->clock = get_kvmclock_base_ns() + ka->kvmclock_offset;
> >  	}
> >  }
> 
> [Severity: High]
> Does this function need to return true on the success path?

Doh, yes.  I botched the splitting and incorrectly added the "return true" in
"KVM: x86: Wrap all of __get_kvmclock_master_clock() with CONFIG_X86_64=y".  I'll
fix, and properly compile test all patches (I did that before splitting the patch,
but obviously not after).

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1146 bytes --]

On Wed, 2026-08-05 at 08:16 -0700, Sean Christopherson wrote:
> 
> David, any thoughts?  I'm leaning towards keeping IRQs disabled to minimize the
> chances of introducing a regression, even though I highly doubt disabling IRQs
> to provide an atomic-ish pair was ever done deliberately.  My main concern with
> disabling IRQs is that it will further muddy the waters with respect to what is
> actually necessary, versus weird things KVM does for historical reasons.  Though
> that can largely be solved with a verbose changelog.

I'm not sure I'd bother. There are plenty of other places we use an
"atomic-ish pair", although I've tried to kill most of those by the
time we get to the end of my series. And we don't disable interrupts
around them all; why should this one do so just because it accidentally
inherited it for other reasons?

I'd rather just keep on with eliminating those cases for all the
masterclock code paths, and reducing the cases where we *don't* use
masterclock (I've already fixed it for the case where TSCs are offset
but running at the same frequency; the old_kvmclock thing is on my
radar too).

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 6179 bytes --]

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

[Sean Christopherson]

On Wed, Aug 05, 2026, David Woodhouse wrote:
> On Wed, 2026-08-05 at 08:16 -0700, Sean Christopherson wrote:
> > 
> > David, any thoughts?  I'm leaning towards keeping IRQs disabled to minimize the
> > chances of introducing a regression, even though I highly doubt disabling IRQs
> > to provide an atomic-ish pair was ever done deliberately.  My main concern with
> > disabling IRQs is that it will further muddy the waters with respect to what is
> > actually necessary, versus weird things KVM does for historical reasons.  Though
> > that can largely be solved with a verbose changelog.
> 
> I'm not sure I'd bother. There are plenty of other places we use an
> "atomic-ish pair", although I've tried to kill most of those by the
> time we get to the end of my series. And we don't disable interrupts
> around them all; why should this one do so just because it accidentally
> inherited it for other reasons?

Ya, after trying to write a changelog and reconcile the new "rule" with the
existing code, I agree.  For kvmclock, the badness is that the guest's view of
time would be off by a smidge until the next kvm_guest_time_update(), but that's
a complete non-issue when considering that a host IRQ at any time immediately
introduces significantly lag into the guest's read of "now".

TSC catchup due to an unstable TSC is a similar story.  The "bad" offset will be
corrected on the next kvm_arch_vcpu_load().

So it's really just the "always catchup" mode for software-based TSC scaling that
would have a persistent flaw, because as Sashiko pointed out, KVM would adjust
the offset by "too much".  But that's a fundamental flaw in the catchup logic:
KVM should compute an guest TSC as an absolute value by using the current time
and a reference time, not by accumulating delta.

Re: [PATCH v8 05/17] KVM: x86: Avoid NTP frequency skew for KVM clock on 32-bit host

[Sean Christopherson]

On Wed, Aug 05, 2026, sashiko-bot@kernel.org wrote:
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 3107878a6fe5d..8f469fd9863f9 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> 
> [ ... ]
> 
> > @@ -915,13 +914,6 @@ static s64 get_kvmclock_base_ns(void)
> >  	/* Count up from boot time, but with the frequency of the raw clock.  */
> >  	return ktime_to_ns(ktime_add(ktime_get_raw(), pvclock_gtod_data.offs_boot));
> 
> [Severity: Low]
> Will reading the 64-bit pvclock_gtod_data.offs_boot without seqcount
> protection or data_race() annotations trigger KCSAN data race warnings on
> 32-bit systems?
> 
> By removing the ktime_get_boottime_ns() fallback, this read now executes on
> 32-bit architectures where it compiles to two non-atomic 32-bit accesses.
> If a KVM vCPU thread calls get_kvmclock_base_ns() while a timer interrupt
> runs timekeeping_update(), it overwrites offs_boot.
> 
> Even though the value only actually changes during suspend when the freezer
> subsystem guarantees vCPU threads are frozen (preventing functional tearing),
> overwriting the identical value concurrently with an unprotected read still
> introduces a formal C11 data race.

Huh.  And strictly speaking, 64-bit could tear the store/load.  Stealing heavily
from ktime_mono_to_any(), this as a prep patch plus fixup (not yet tested)?

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b6e1dfd6db6a..57679d871581 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -921,7 +921,8 @@ static void update_pvclock_gtod(struct timekeeper *tk)
 
        vdata->wall_time_sec            = tk->xtime_sec;
 
-       vdata->offs_boot                = tk->offs_boot;
+       /* Pairs with the READ_ONCE() in get_kvmclock_base_ns(). */
+       WRITE_ONCE(vdata->offs_boot, tk->offs_boot);
 
        write_seqcount_end(&vdata->seq);
 }
@@ -929,7 +930,26 @@ static void update_pvclock_gtod(struct timekeeper *tk)
 static s64 get_kvmclock_base_ns(void)
 {
        /* Count up from boot time, but with the frequency of the raw clock.  */
-       return ktime_to_ns(ktime_add(ktime_get_raw(), pvclock_gtod_data.offs_boot));
+       struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
+       ktime_t raw = ktime_get_raw();
+       ktime_t now;
+
+       /*
+        * Synchronization with clock updates isn't required on 64-bit as only
+        * one field is being consume
+        * */
+#ifdef CONFIG_X86_64
+       now = ktime_add(raw, READ_ONCE(gtod->offs_boot));
+#else
+       unsigned int seq;
+
+       do {
+               seq = read_seqcount_begin(&gtod->seq);
+               now = ktime_add(raw, *offset);
+       } while (read_seqcount_retry(gtod->seq, seq));
+#endif
+
+       return ktime_to_ns(now);
 }
 
 static uint32_t div_frac(uint32_t dividend, uint32_t divisor)

Re: [PATCH v8 05/17] KVM: x86: Avoid NTP frequency skew for KVM clock on 32-bit host

[Sean Christopherson]

On Wed, Aug 05, 2026, Sean Christopherson wrote:
> On Wed, Aug 05, 2026, sashiko-bot@kernel.org wrote:
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 3107878a6fe5d..8f469fd9863f9 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > 
> > [ ... ]
> > 
> > > @@ -915,13 +914,6 @@ static s64 get_kvmclock_base_ns(void)
> > >  	/* Count up from boot time, but with the frequency of the raw clock.  */
> > >  	return ktime_to_ns(ktime_add(ktime_get_raw(), pvclock_gtod_data.offs_boot));
> > 
> > [Severity: Low]
> > Will reading the 64-bit pvclock_gtod_data.offs_boot without seqcount
> > protection or data_race() annotations trigger KCSAN data race warnings on
> > 32-bit systems?
> > 
> > By removing the ktime_get_boottime_ns() fallback, this read now executes on
> > 32-bit architectures where it compiles to two non-atomic 32-bit accesses.
> > If a KVM vCPU thread calls get_kvmclock_base_ns() while a timer interrupt
> > runs timekeeping_update(), it overwrites offs_boot.
> > 
> > Even though the value only actually changes during suspend when the freezer
> > subsystem guarantees vCPU threads are frozen (preventing functional tearing),
> > overwriting the identical value concurrently with an unprotected read still
> > introduces a formal C11 data race.
> 
> Huh.  And strictly speaking, 64-bit could tear the store/load.  Stealing heavily
> from ktime_mono_to_any(), this as a prep patch plus fixup (not yet tested)?

LOL, hilarious.  I was cherry-picking the rest of the series on top to run the
tests, and discovered that "Compute kvmclock base without pvclock_gtod_data"
does exactly that: uses ktime_mono_to_any() directly.  So at least I went in the
right direction?

David, is there any reason that patch needs to be 25/36?  AFAICT, it slots in
very nicely before this patch.  Then we don't need to do the below, because
ktime_mono_to_any() already takes care of 32-bit.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b6e1dfd6db6a..57679d871581 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -921,7 +921,8 @@ static void update_pvclock_gtod(struct timekeeper *tk)
>  
>         vdata->wall_time_sec            = tk->xtime_sec;
>  
> -       vdata->offs_boot                = tk->offs_boot;
> +       /* Pairs with the READ_ONCE() in get_kvmclock_base_ns(). */
> +       WRITE_ONCE(vdata->offs_boot, tk->offs_boot);
>  
>         write_seqcount_end(&vdata->seq);
>  }
> @@ -929,7 +930,26 @@ static void update_pvclock_gtod(struct timekeeper *tk)
>  static s64 get_kvmclock_base_ns(void)
>  {
>         /* Count up from boot time, but with the frequency of the raw clock.  */
> -       return ktime_to_ns(ktime_add(ktime_get_raw(), pvclock_gtod_data.offs_boot));
> +       struct pvclock_gtod_data *gtod = &pvclock_gtod_data;
> +       ktime_t raw = ktime_get_raw();
> +       ktime_t now;
> +
> +       /*
> +        * Synchronization with clock updates isn't required on 64-bit as only
> +        * one field is being consume
> +        * */
> +#ifdef CONFIG_X86_64
> +       now = ktime_add(raw, READ_ONCE(gtod->offs_boot));
> +#else
> +       unsigned int seq;
> +
> +       do {
> +               seq = read_seqcount_begin(&gtod->seq);
> +               now = ktime_add(raw, *offset);
> +       } while (read_seqcount_retry(gtod->seq, seq));
> +#endif
> +
> +       return ktime_to_ns(now);
>  }
>  
>  static uint32_t div_frac(uint32_t dividend, uint32_t divisor)