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

Radim Krcmar
2017-01-16T15:40:29Z

2017-01-13 15:51-0200, 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@redhat.com>
>> >> >
>> >> > ---
>> >> > 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?

Because it doesn't count nanoseconds. In update_vsyscall(), the *_snsec
are shifted by tk->tkr_mono.shift bits to the left and that precision
goes to sub-nanoseconds.

64 bit value makes sense then -- would be nice if we could pass that
extra precision to the guest.

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

True.

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

I see, thanks. They are not nanoseconds as the sub-nanosecond shift is
there as well:

u64 nsecps = (u64)NSEC_PER_SEC << tk->tkr_mono.shift;
while (tk->tkr_mono.xtime_nsec >= nsecps) {
tk->tkr_mono.xtime_nsec -= nsecps;
tk->xtime_sec++;
}