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

On 7 August 2026 17:01:38 BST, Sean Christopherson <[email protected]> wrote:
>+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, [email protected] 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)

(Home and on the phone after an insane week; full attention span might be limited until Monday).

The only reason the ktime snapshot stuff is last in the series is because I did that work on the kernel's core timekeeping two years after the rest of the series, and it was still sitting at the tip branch at the time so I didn't want to make it a hard dependency for the kvmclock series. Now the timekeeping stuff is merged we can fold it all into the kvmclock series however we like.