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

[Sean Christopherson <seanjc@google.com>]

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)