From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751107AbdAPPk3 (ORCPT ); Mon, 16 Jan 2017 10:40:29 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49086 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750924AbdAPPk1 (ORCPT ); Mon, 16 Jan 2017 10:40:27 -0500 Date: Mon, 16 Jan 2017 16:40:24 +0100 From: Radim Krcmar To: Marcelo Tosatti Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, Paolo Bonzini , Richard Cochran , Miroslav Lichvar Subject: Re: [patch 1/3] KVM: x86: provide realtime host clock via vsyscall notifiers Message-ID: <20170116154020.GC25835@potion> References: <20170113120131.086634482@redhat.com> <20170113120317.948215303@redhat.com> <20170113151804.GA25835@potion> <20170113153406.GA4796@amt.cnet> <20170113162809.GE22440@potion> <20170113175124.GB9310@amt.cnet> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170113175124.GB9310@amt.cnet> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Mon, 16 Jan 2017 15:40:28 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> >> > >> >> > --- >> >> > 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++; }