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

[Sean Christopherson <seanjc@google.com>]

+David (I realized I'm having a conversation between me, myself, and Sashiko)

On Thu, Aug 06, 2026, Sean Christopherson wrote:
> 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.

Same question for "KVM: x86: Use ktime_get_snapshot_id() for master clock".  I
don't see any obvious dependencies in here:

  KVM: x86: Re-synchronize TSC after KVM_SET_TSC_KHZ
  KVM: x86: Remove runtime Xen TSC frequency CPUID update
  KVM: x86: Avoid redundant masterclock updates from multiple vCPUs
  KVM: x86/xen: Prevent runstate times from becoming negative
  KVM: x86: Avoid gratuitous global clock updates
  KVM: x86: Factor out kvm_use_master_clock()
  KVM: x86: Allow KVM master clock mode when TSCs are offset from each other
  KVM: x86: Replace nr_vcpus_matched_tsc count with all_vcpus_matched_tsc bool
  KVM: x86: Kill last_tsc_{nsec,write,offset} fields
  KVM: x86: Improve synchronization in kvm_synchronize_tsc()

And if I pull those in earlier, and grab these two (which AFICT also don't have
dependencies either):

  KVM: x86: Remove runtime Xen TSC frequency CPUID update
  KVM: x86/xen: Prevent runstate times from becoming negative

Then what's left fits nicely into two or three buckets:

  1: Masterclock overhaul
  2. New uAPI and tests

> > 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)