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

[Radim Krcmar]

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.

>> >  };
>> >  
>> >  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.

>> 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.
Renaming nsec_base is ok, but I'm not sure what tk->tkr_mono.xtime_nsec
is anymore.