Re: [PATCH 3/3] KVM: x86: conditionally update masterclock data in pvclock_update_vm_gtod_copy()

[David Woodhouse <dwmw2@infradead.org>]

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

On Thu, 2026-01-15 at 12:22 -0800, Dongli Zhang wrote:
> The pvclock_update_vm_gtod_copy() function always unconditionally updates
> ka->master_kernel_ns and ka->master_cycle_now whenever a
> KVM_REQ_MASTERCLOCK_UPDATE occurs. Unfortunately, each masterclock update
> increases the risk of kvm-clock drift.
> 
> If pvclock_update_vm_gtod_copy() is not called from
> vcpu_enter_guest()-->kvm_update_masterclock(), we keep the existing
> workflow. The argument 'forced' is introduced to tell where it is from.
> 
> Otherwise, we avoid updating the masterclock if it is already
> active and will remain active. In such cases, updating the masterclock
> data is not beneficial and can instead lead to kvm-clock drift.
> 
> As a result, this patch minimizes the chance of unnecessary masterclock
> data updates to avoid kvm-clock drift.
> 
> Cc: David Woodhouse <dwmw@amazon.co.uk>
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>

Hmm... so the only caller of pvclock_update_vm_gtod_copy() that doesn't
set the 'force' argument is the one in kvm_update_masterclock(), so we
are asserting that kvm_update_masterclock() never needs to *change* the
masterclock origin point, if it was already set?

The gtod notifier callback in pvclock_gtod_update_fn() also ends up
setting KVM_REQ_MASTERCLOCK_UPDATE, and is triggered by an actual host
timekeeping update (which could potentially be from a clocksource
change). It also hypothetically possible that the clocksource changes
from TSC → HPET → TSC, switching back to TSC again before the
masterclock update ever gets to run. Or maybe a suspend/resume?

Are you *sure* that the optimisation is always valid...?

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