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

[Marcelo Tosatti <mtosatti@redhat.com>]

On Wed, Jan 25, 2017 at 01:59:34PM +0100, Paolo Bonzini wrote:
> 
> 
> On 25/01/2017 13:55, Marcelo Tosatti wrote:
> > On Wed, Jan 25, 2017 at 01:30:49PM +0100, Paolo Bonzini wrote:
> >>
> >>
> >> On 24/01/2017 18:09, Marcelo Tosatti wrote:
> >>> 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@redhat.com>
> >>>
> >>> ---
> >>>  Documentation/virtual/kvm/hypercalls.txt |   33 ++++++++++++++++++++++++
> >>>  arch/x86/include/uapi/asm/kvm_para.h     |    9 ++++++
> >>>  arch/x86/kvm/x86.c                       |   41 +++++++++++++++++++++++++++++++
> >>>  include/uapi/linux/kvm_para.h            |    3 ++
> >>>  4 files changed, 86 insertions(+)
> >>>
> >>> v2: improve documentation (Radim)
> >>>     change hypercall name to KVM_HC_CLOCK_PAIRING (Radim)
> >>>     increase padding size
> >>>
> >>> Index: kvm-ptpdriver/arch/x86/include/uapi/asm/kvm_para.h
> >>> ===================================================================
> >>> --- kvm-ptpdriver.orig/arch/x86/include/uapi/asm/kvm_para.h	2017-01-13 16:43:11.947240575 -0200
> >>> +++ kvm-ptpdriver/arch/x86/include/uapi/asm/kvm_para.h	2017-01-13 16:43:25.038258187 -0200
> >>> @@ -50,6 +50,15 @@
> >>>  	__u32 pad[11];
> >>>  };
> >>>  
> >>> +#define KVM_CLOCK_PAIRING_WALLCLOCK 0
> >>> +struct kvm_clock_offset {
> >>
> >> The struct still has the old name.
> >>
> >>> +	__s64 sec;
> >>> +	__s64 nsec;
> >>> +	__u64 tsc;
> >>> +	__u32 flags;
> >>> +	__u32 pad[9];
> >>> +};
> > 
> > That was on purpose: "to pair" is the operation. 
> > 
> > "struct kvm_clock_pairing" makes no sense to me 
> > (because pairing is an "action").
> > 
> > (if you'd prefer that name, let me know).
> 
> A "pairing" is a union of two things, in this case a sec/nsec value and
> a TSC value.  (I had to look it up in the dictionary though :)).
> 
> >>>  #define KVM_STEAL_ALIGNMENT_BITS 5
> >>>  #define KVM_STEAL_VALID_BITS ((-1ULL << (KVM_STEAL_ALIGNMENT_BITS + 1)))
> >>>  #define KVM_STEAL_RESERVED_MASK (((1 << KVM_STEAL_ALIGNMENT_BITS) - 1 ) << 1)
> >>> Index: kvm-ptpdriver/Documentation/virtual/kvm/hypercalls.txt
> >>> ===================================================================
> >>> --- kvm-ptpdriver.orig/Documentation/virtual/kvm/hypercalls.txt	2017-01-13 16:43:11.947240575 -0200
> >>> +++ kvm-ptpdriver/Documentation/virtual/kvm/hypercalls.txt	2017-01-13 16:43:25.038258187 -0200
> >>> @@ -81,3 +81,36 @@
> >>>  same guest can wakeup the sleeping vcpu by issuing KVM_HC_KICK_CPU hypercall,
> >>>  specifying APIC ID (a1) of the vcpu to be woken up. An additional argument (a0)
> >>>  is used in the hypercall for future use.
> >>> +
> >>> +
> >>> +6. KVM_HC_CLOCK_PAIRING
> >>> +------------------------
> >>> +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_PAIRING_WALLCLOCK (0)
> >>> +is supported (hosts CLOCK_REALTIME clock).
> >>> +
> >>> +		struct kvm_clock_offset {
> >>
> >> Name of the struct not adjusted.
> >>> +			__s64 sec;
> >>> +			__s64 nsec;
> >>> +			__u64 tsc;
> >>> +			__u32 flags;
> >>> +			__u32 pad[9];
> >>
> >> I'd just leave one element of padding and put the struct on the stack, 
> >> since we have no reason to think we'll need a lot of padding.
> > 
> > Well you don't know: we could implement it in case of HPET clocksource
> > hosts.
> 
> But then the guest wouldn't have a TSC value to use, would it?
> 
> Also, how would it use more padding?

I don't know, maybe pass the TSC and host HPET read in the structure? 
(after exposing the HPET to the guest).

Or maybe you want to sync CLOCK_MONOTONIC and that requires other data.

Not sure.

Just some space if new features are necessary.