Re: [patch 3/3] PTP: add kvm PTP driver

[Radim Krcmar]

2017-01-13 15:40-0200, Marcelo Tosatti:
> On Fri, Jan 13, 2017 at 04:56:58PM +0100, Radim Krcmar wrote:
> > 2017-01-13 10:01-0200, Marcelo Tosatti:
>> > +		version = pvclock_read_begin(src);
>> > +
>> > +		ret = kvm_hypercall2(KVM_HC_CLOCK_OFFSET,
>> > +				     clock_off_gpa,
>> > +				     KVM_CLOCK_OFFSET_WALLCLOCK);
>> > +		if (ret != 0) {
>> > +			pr_err("clock offset hypercall ret %lu\n", ret);
>> > +			spin_unlock(&kvm_ptp_lock);
>> > +			preempt_enable_notrace();
>> > +			return -EOPNOTSUPP;
>> > +		}
>> > +
>> > +		tspec.tv_sec = clock_off.sec;
>> > +		tspec.tv_nsec = clock_off.nsec;
>> > +
>> > +		delta = rdtsc_ordered() - clock_off.tsc;
>> > +
>> > +		offset = pvclock_scale_delta(delta, src->tsc_to_system_mul,
>> > +					     src->tsc_shift);
>> > +
>> > +	} while (pvclock_read_retry(src, version));
>> > +
>> > +	preempt_enable_notrace();
>> > +
>> > +	tspec.tv_nsec = tspec.tv_nsec + offset;
>> > +
>> > +	spin_unlock(&kvm_ptp_lock);
>> > +
>> > +	if (tspec.tv_nsec >= NSEC_PER_SEC) {
>> > +		u64 secs = tspec.tv_nsec;
>> > +
>> > +		tspec.tv_nsec = do_div(secs, NSEC_PER_SEC);
>> > +		tspec.tv_sec += secs;
>> > +	}
>> > +
>> > +	memcpy(ts, &tspec, sizeof(struct timespec64));
>> 
>> But the whole idea is of improving the time by reading tsc a bit later
>> is just weird ... why is it better to provide
>> 
>>   tsc + x, time + tsc_delta_to_time(x)
>> 
>> than just
>> 
>>  tsc, time
>> 
>> ?
> 
> Because you want to calculate the value of the host realtime clock 
> at the moment of ptp_kvm_gettime.
> 
> We do:
> 
> 	1. kvm_hypercall.
> 	2. get {sec, nsec, guest_tsc}.
> 	3. kvm_hypercall returns.
> 	4. delay = rdtsc() - guest_tsc.
> 
> Where delay is the delta (measured with the TSC) between points 2 and 4.

I see now ... the PTP interface is just not good for our purposes.
We don't return {sec, nsec, guest_tsc}, we just return {sec, nsec} at
some random time in the past.  And to make it a bit more accurate, you
add a best-effort delta before returning, which makes sense.

When we have to depend on pvclock, what are the advantages of not using
the existing pvclock API for wall clock?
(You mentioned some extensions.)

  struct pvclock_wall_clock {
  	u32   version;
  	u32   sec;
  	u32   nsec;
  } __attribute__((__packed__));

It gives the wall clock when pvclock was 0, so you just add current
kvmclock and get the host wall clock.  Without a VM exit.

And how often is ptp_kvm_gettime() usually called?

Thanks.

>> Because we'll always be quering the time at tsc + y, where y >> x, and
>> we'd likely have other problems if shifting the time base by few
>> thousand cycles made a difference.
> 
> Radim, i didnt get your "tsc + x", "time + tsc_delta_to_time(x)"
> formulas above. Can you be more verbose please?

x is the delta, tsc_delta_to_time() is what pvclock_scale_delta() does.

I assumed that we set precise time with TSC, so the delta wouldn't
matter, because PTP would either get {sec, nsec, guest_tsc}, or the
same, but just shifted by delta, hence
{sec  + tsc_delta_to_time(x) / NSEC_PER_SEC,
 nsec + tsc_delta_to_time(x) % NSEC_PER_SEC,
 guest_tsc + x}.