linux-kernel - Re: [patch 2/3] KVM: x86: add KVM_HC_CLOCK

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170113170739.GF22440@potion>
Date:   Fri, 13 Jan 2017 18:07:40 +0100
From:   Radim Krcmar <rkrcmar@...hat.com>
To:     Marcelo Tosatti <mtosatti@...hat.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Richard Cochran <richardcochran@...il.com>,
        Miroslav Lichvar <mlichvar@...hat.com>
Subject: Re: [patch 2/3] KVM: x86: add KVM_HC_CLOCK_OFFSET hypercall

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@...hat.com>
>> > 
>> > ---
>> > 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.