From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751017AbdAMRHt (ORCPT ); Fri, 13 Jan 2017 12:07:49 -0500 Received: from mx1.redhat.com ([209.132.183.28]:31765 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750881AbdAMRHq (ORCPT ); Fri, 13 Jan 2017 12:07:46 -0500 Date: Fri, 13 Jan 2017 18:07:40 +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 2/3] KVM: x86: add KVM_HC_CLOCK_OFFSET hypercall Message-ID: <20170113170739.GF22440@potion> References: <20170113120131.086634482@redhat.com> <20170113120319.692007194@redhat.com> <20170113153157.GB25835@potion> <20170113154321.GB4796@amt.cnet> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170113154321.GB4796@amt.cnet> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Fri, 13 Jan 2017 17:07:43 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2017-01-13 13:43-0200, Marcelo Tosatti: > On Fri, Jan 13, 2017 at 04:31:58PM +0100, Radim Krcmar wrote: >> 2017-01-13 10:01-0200, Marcelo Tosatti: >> > Add a hypercall to retrieve the host realtime clock >> > and the TSC value used to calculate that clock read. >> > >> > Used to implement clock synchronization between >> > host and guest. >> > >> > Signed-off-by: Marcelo Tosatti >> > >> > --- >> > Index: kvm-ptpdriver/Documentation/virtual/kvm/hypercalls.txt >> > @@ -81,3 +81,33 @@ >> > +6. KVM_HC_CLOCK_OFFSET >> > +------------------------ >> > +Architecture: x86 >> > +Status: active >> > +Purpose: Hypercall used to synchronize host and guest clocks. >> > +Usage: >> > + >> > +a0: guest physical address where host copies >> > +"struct kvm_clock_offset" structure. >> > + >> > +a1: clock_type, ATM only KVM_CLOCK_OFFSET_WALLCLOCK (0) >> > +is supported (hosts CLOCK_REALTIME clock). >> > + >> > + struct kvm_clock_offset { >> > + __s64 sec; >> > + __s64 nsec; >> >> Why is nsec: >> 1) signed -- it is a remainder after division by NSEC_PER_SEC > > Because "struct timespec" is signed because it can be used for > time deltas (you won't actually get signed values for > kvm_get_walltime_and_clockread). > > Just wanted to match "struct timespec". > >> 2) bigger than 32 bit -- NSEC_PER_SEC < 2^32 >> ? > > Again matching struct timespec. It is "long" in struct timespec, which could also be "s32" ... I'd rather waste those 8 bytes inside padding -- its purpose is clear there. :) >> > + __u64 tsc; >> > + __u32 flags; >> > + __u32 pad; >> > + }; >> > + >> > + Where: >> > + * sec: seconds from clock_type clock. >> > + * nsec: nanoseconds from clock_type clock. >> >> The important part of an offset is the starting point -- I assume it is >> the the usual one, but documentation better be explicit. > > Don't get what you mean? (the values have same meaning as hosts > clock_gettime(CLOCK_REALTIME), supposedly that is clear). Ah, I didn't understand that clock_type was refering to CLOCK_REALTIME. I'd drop offset from the hypercall name. IIUC, all various clock types would be compared to TSC, so we could name the hypercall somewhat like KVM_HC_CLOCK_AT_TSC -- we want to know what was the time on the selected clock when TSC was at value __u64 tsc. The only offset is between sec+nsec and the beginning of time (and tsc and 0), but we don't care about that offset by itself -- we care about relation of sec+nsec to tsc, which isn't a simple offset as they flow differently. If we wanted to be more generic, then KVM_HC_CLOCK_PAIRING/SNAPSHOT/... and argument 1 would contain clock_types, here REALTIME and TSC.