Re: [patch 2/3] KVM: x86: add KVM_HC_CLOCK_OFFSET hypercall

[Marcelo Tosatti]

On Fri, Jan 13, 2017 at 06:07:40PM +0100, Radim Krcmar wrote:
> 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 <mtosatti@xxxxxxxxxx>
> >> > 
> >> > ---
> >> > 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" ...

Which fits in a s64.

> I'd rather waste those 8 bytes inside padding -- its purpose is clear
> there. :)

The purpose of the s64 is to match timespec.

> >> > +			__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.

Well its the offset from TSC, so thats why "offset" in the name.

And as you say, the interface could be extended to support 
other clocks (using the available data in pad, i'll make it larger BTW).

Which would render the "TSC" from the hypercall name invalid.

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

Pairing is fine, changed.