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

+David

On Tue, Aug 04, 2026, [email protected] 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.