Re: [patch 1/3] KVM: x86: provide realtime host clock via vsyscall notifiers

[Marcelo Tosatti]

On Fri, Jan 13, 2017 at 05:28:09PM +0100, Radim Krcmar wrote:
> 2017-01-13 13:34-0200, Marcelo Tosatti:
> > On Fri, Jan 13, 2017 at 04:18:04PM +0100, Radim Krcmar wrote:
> >> 2017-01-13 10:01-0200, Marcelo Tosatti:
> >> > Expose the realtime host clock and save the TSC value
> >> > used for the clock calculation.
> >> > 
> >> > Signed-off-by: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
> >> > 
> >> > ---
> >> >  arch/x86/kvm/x86.c |   38 ++++++++++++++++++++++++++++++++++++++
> >> >  1 file changed, 38 insertions(+)
> >> > 
> >> > Index: kvm-ptpdriver/arch/x86/kvm/x86.c
> >> > ===================================================================
> >> > --- kvm-ptpdriver.orig/arch/x86/kvm/x86.c	2017-01-13 08:59:03.015895353 -0200
> >> > +++ kvm-ptpdriver/arch/x86/kvm/x86.c	2017-01-13 09:04:46.581415259 -0200
> >> > @@ -1139,6 +1139,8 @@
> >> >  
> >> >  	u64		boot_ns;
> >> >  	u64		nsec_base;
> >> > +	u64		wall_time_sec;
> >> > +	u64		wall_time_snsec;
> >> 
> >> The leading "s" in "snsec" looks like a copy-paste residue.
> > 
> > Just copying the userspace vsyscall interface.
> 
> Oh, so the "s" means "sub-" for sub-nanosecond precision.

It only counts nanoseconds, how can it be sub nanosecond precise?

> >> >  };
> >> >  
> >> >  static struct pvclock_gtod_data pvclock_gtod_data;
> >> > @@ -1162,6 +1164,9 @@
> >> >  	vdata->boot_ns			= boot_ns;
> >> >  	vdata->nsec_base		= tk->tkr_mono.xtime_nsec;
> >> >  
> >> > +	vdata->wall_time_sec            = tk->xtime_sec;
> >> > +	vdata->wall_time_snsec          = tk->tkr_mono.xtime_nsec;
> >> 
> >> Using tk->tkr_mono offsets for real time seems wrong -- what happens if
> >> the real time is half a second shifted from monotonic time?
> > 
> > Both the userspace vsyscall interface and getnstimeofday
> > use it for realtime clock.
> > 
> > Monotonic clock adds the offset:
> > 
> >         vdata->monotonic_time_snsec     = tk->tkr_mono.xtime_nsec
> >                                         +
> > ((u64)tk->wall_to_monotonic.tv_nsec
> >                                                 << tk->tkr_mono.shift);
> 
> I see, thanks.  Makes me wonder why our monotonic time is correct then,
> but that is problably thanks to boot_ns.

The actual starting point of the system_timestamp part of kvmclock
does not matter, all it matters is that it counts in nanoseconds.

> >> If it's ok, then vdata->nsec_base == vdata->wall_time_snsec, so we don't
> >> need it.
> > 
> > Just copying the userspace vsyscall interface.
> > 
> > Do you actually want to change the "s" and unify wall_time_snsec with
> > nsec_base?
> 
> The "s" isn't important, even though I don't think we do anything that
> would justify it, but make use just 8 bytes for both.

Unified.

> Renaming nsec_base is ok, but I'm not sure what tk->tkr_mono.xtime_nsec
> is anymore.

Is the nsec part of tk->xtime_sec. See accumulate_nsecs_to_secs
(which is called from the timer interrupt handler).

/**
 * struct timekeeper - Structure holding internal timekeeping values.
 * @tkr_mono:           The readout base structure for CLOCK_MONOTONIC
 * @tkr_raw:            The readout base structure for
 * CLOCK_MONOTONIC_RAW
 * @xtime_sec:          Current CLOCK_REALTIME time in seconds
 * @ktime_sec:          Current CLOCK_MONOTONIC time in seconds
 * @wall_to_monotonic:  CLOCK_REALTIME to CLOCK_MONOTONIC offset
 * @offs_real:          Offset clock monotonic -> clock realtime
 * @offs_boot:          Offset clock monotonic -> clock boottime
 * @offs_tai:           Offset clock monotonic -> clock tai
 * @tai_offset:         The current UTC to TAI offset in seconds