Date: Wed, 27 Sep 2017 13:53:50 +0200 (CEST)
From: Thomas Gleixner <tglx@linutronix.de>
To: Paolo Bonzini <pbonzini@redhat.com>
cc: Denis Plotnikov <dplotnikov@virtuozzo.com>, rkrcmar@redhat.com,
        kvm@vger.kernel.org, john.stultz@linaro.org, mingo@redhat.com,
        hpa@zytor.com, linux-kernel@vger.kernel.org, x86@kernel.org,
        rkagan@virtuozzo.com, den@virtuozzo.com
Subject: Re: [PATCH v5 1/6] timekeeper: introduce extended clocksource reading
 callback
In-Reply-To: <7f73ec16-7262-329c-d2c3-d5481142c370@redhat.com>
Message-ID: <alpine.DEB.2.20.1709271339020.2027@nanos>
References: <1504106628-172372-1-git-send-email-dplotnikov@virtuozzo.com> <1504106628-172372-2-git-send-email-dplotnikov@virtuozzo.com> <alpine.DEB.2.20.1709252322120.2418@nanos> <7b886c9f-70a7-951b-ff6f-f77f263754e7@redhat.com> <alpine.DEB.2.20.1709270946200.1945@nanos>
 <7f73ec16-7262-329c-d2c3-d5481142c370@redhat.com>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2624
Lines: 79

On Wed, 27 Sep 2017, Paolo Bonzini wrote:
> The definitions do fit KVM, and indeed there is ptp-kvm that does
> something very similar to what you describe in the other mail.  We have:
> 
> 	#1 is host time
> 	#2 is host TSC
> 	#3 is guest TSC
> 
> We know that #2 and #3 have a fixed ratio and offset.  The point is
> whether #1 and #2 can be captured atomically.
> 
> For PTP-KVM, the host tells the guest; if capturing the two is
> impossible, it fails the hypercall and ioctl(PTP_SYS_OFFSET_PRECISE)
> fails too.
> 
> Right now, the hypercall fails if the host clocksource is not the TSC
> clocksource, which is safe.
> 
> These patches are about ascertaining whether #1 and #2 can be captured
> atomically in a more generic way.  In the read_with_stamp case:
> 
> - if it returns true, it gives an atomic reading of #1 and #2
> 
> - if it returns false, it gives a reading of #1 only.
> 
> 
> I think the hook should be specific to x86.  For example it could be an
> array of function pointers, indexed by vclock_mode, with the same
> semantics as read_with_stamp.

I don't think you need that.

The get_time_fn() which is handed in to get_device_system_crossstamp() can
convey that information:

                /*
                 * Try to synchronously capture device time and a system
                 * counter value calling back into the device driver
                 */
                ret = get_time_fn(&xtstamp->device, &system_counterval, ctx);
                if (ret)
                        return ret;

So in your case get_time_fn() would be kvmclock or hyperv clock specific
and the actual hypercall implementation can return a failure code if the
requirements are not met:

   1) host clock source is TSC
   2) capturing of host time and TSC is atomic

The host side of the hypercall can use ktime_get_snapshot() with the boot
time extension and the extra bits I posted yesterday and fail if the
snapshot is not valid for that purpose, i.e. host clocksource is not valid
for guest correlation.

The hypercall side on the guest can then do:

	if (hypercall(hypercalldata))
		return -ENODEV;
	system_counterval->cs = tsc_clocksource;
       	system_counterval->data = hypercalldata;
	return 0;

Then the following check in get_device_system_crossstamp() will do the
last sanity check:

		/*
		 * Verify that the clocksource associated with the captured
		 * system counter value is the same as the currently installed
		 * timekeeper clocksource
		 */
		if (tk->tkr_mono.clock != system_counterval.cs)
			return -ENODEV;

which catches the case where the guest side clock source is !TSC.

Thanks,

	tglx