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

[Radim Krcmar]

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