Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1558340 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [patch 3/3] PTP: add kvm PTP driver Marcelo Tosatti <mtosatti@redhat.com> - 2017-01-13 13:10 +0100
    Re: [patch 3/3] PTP: add kvm PTP driver Radim Krcmar <rkrcmar@redhat.com> - 2017-01-13 17:00 +0100
      Re: [patch 3/3] PTP: add kvm PTP driver Marcelo Tosatti <mtosatti@redhat.com> - 2017-01-13 19:10 +0100
        Re: [patch 3/3] PTP: add kvm PTP driver Radim Krcmar <rkrcmar@redhat.com> - 2017-01-16 17:30 +0100
          Re: [patch 3/3] PTP: add kvm PTP driver Radim Krcmar <rkrcmar@redhat.com> - 2017-01-16 18:00 +0100
            Re: [patch 3/3] PTP: add kvm PTP driver Marcelo Tosatti <mtosatti@redhat.com> - 2017-01-16 18:10 +0100
              Re: [patch 3/3] PTP: add kvm PTP driver Radim Krcmar <rkrcmar@redhat.com> - 2017-01-16 18:30 +0100
                Re: [patch 3/3] PTP: add kvm PTP driver Marcelo Tosatti <mtosatti@redhat.com> - 2017-01-16 18:40 +0100
                  Re: [patch 3/3] PTP: add kvm PTP driver Radim Krcmar <rkrcmar@redhat.com> - 2017-01-16 19:10 +0100
          Re: [patch 3/3] PTP: add kvm PTP driver Marcelo Tosatti <mtosatti@redhat.com> - 2017-01-16 18:10 +0100
            Re: [patch 3/3] PTP: add kvm PTP driver Radim Krcmar <rkrcmar@redhat.com> - 2017-01-16 18:50 +0100
      Re: [patch 3/3] PTP: add kvm PTP driver Richard Cochran <richardcochran@gmail.com> - 2017-01-14 16:30 +0100
        Re: [patch 3/3] PTP: add kvm PTP driver Radim Krcmar <rkrcmar@redhat.com> - 2017-01-16 16:50 +0100

#1558340 — [patch 3/3] PTP: add kvm PTP driver

Add a driver with gettime method returning hosts realtime clock.
This allows Chrony to synchronize host and guest clocks with 
high precision (see results below).

chronyc> sources
MS Name/IP address         Stratum Poll Reach LastRx Last sample
===============================================================================
#* PHC0                          0   3   377     6     +4ns[   +4ns] +/-    3ns

To configure Chronyd to use PHC refclock, add the 
following line to its configuration file:

refclock PHC /dev/ptpX poll 3 dpoll -2 offset 0

Where /dev/ptpX is the kvmclock PTP clock.


---
 drivers/ptp/Kconfig   |   12 +++
 drivers/ptp/Makefile  |    1 
 drivers/ptp/ptp_kvm.c |  180 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 193 insertions(+)

Index: kvm-ptpdriver/drivers/ptp/Kconfig
===================================================================
--- kvm-ptpdriver.orig/drivers/ptp/Kconfig	2017-01-13 09:17:31.724568567 -0200
+++ kvm-ptpdriver/drivers/ptp/Kconfig	2017-01-13 09:55:33.344208894 -0200
@@ -90,4 +90,16 @@
 	  To compile this driver as a module, choose M here: the module
 	  will be called ptp_pch.
 
+config PTP_1588_CLOCK_KVM
+	tristate "KVM virtual PTP clock"
+	depends on PTP_1588_CLOCK
+	depends on KVM_GUEST
+	default y
+	help
+	  This driver adds support for using kvm infrastructure as a PTP
+	  clock. This clock is only useful if you are using KVM guests.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called ptp_kvm.
+
 endmenu
Index: kvm-ptpdriver/drivers/ptp/ptp_kvm.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ kvm-ptpdriver/drivers/ptp/ptp_kvm.c	2017-01-13 09:57:55.013440645 -0200
@@ -0,0 +1,180 @@
+/*
+ * Virtual PTP 1588 clock for use with KVM guests
+ *
+ * Copyright (C) 2017 Red Hat Inc.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ */
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <uapi/linux/kvm_para.h>
+#include <asm/kvm_para.h>
+#include <asm/pvclock.h>
+#include <uapi/asm/kvm_para.h>
+
+#include <linux/ptp_clock_kernel.h>
+
+struct kvm_ptp_clock {
+	struct ptp_clock *ptp_clock;
+	struct ptp_clock_info caps;
+};
+
+DEFINE_SPINLOCK(kvm_ptp_lock);
+
+static struct pvclock_vsyscall_time_info *hv_clock;
+
+/*
+ * PTP clock operations
+ */
+
+static int ptp_kvm_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
+{
+	return -EOPNOTSUPP;
+}
+
+static int ptp_kvm_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+	return -EOPNOTSUPP;
+}
+
+static struct kvm_clock_offset clock_off;
+static phys_addr_t clock_off_gpa;
+
+static int ptp_kvm_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
+{
+	unsigned long ret;
+	struct timespec64 tspec;
+	u64 delta;
+	cycle_t offset;
+	unsigned version;
+	int cpu;
+	struct pvclock_vcpu_time_info *src;
+
+	preempt_disable_notrace();
+	cpu = smp_processor_id();
+	src = &hv_clock[cpu].pvti;
+
+	spin_lock(&kvm_ptp_lock);
+
+	do {
+		/*
+		 * We are measuring the delay between
+		 * kvm_hypercall and rdtsc using TSC,
+		 * and converting that delta to
+		 * tsc_to_system_mul and tsc_shift
+		 * So any changes to tsc_to_system_mul
+		 * and tsc_shift in this region
+		 * invalidate the measurement.
+		 */
+		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));
+
+	return 0;
+}
+
+static int ptp_kvm_settime(struct ptp_clock_info *ptp,
+			   const struct timespec64 *ts)
+{
+	return -EOPNOTSUPP;
+}
+
+static int ptp_kvm_enable(struct ptp_clock_info *ptp,
+			  struct ptp_clock_request *rq, int on)
+{
+	return -EOPNOTSUPP;
+}
+
+static struct ptp_clock_info ptp_kvm_caps = {
+	.owner		= THIS_MODULE,
+	.name		= "KVM virtual PTP",
+	.max_adj	= 0,
+	.n_ext_ts	= 0,
+	.n_pins		= 0,
+	.pps		= 0,
+	.adjfreq	= ptp_kvm_adjfreq,
+	.adjtime	= ptp_kvm_adjtime,
+	.gettime64	= ptp_kvm_gettime,
+	.settime64	= ptp_kvm_settime,
+	.enable		= ptp_kvm_enable,
+};
+
+/* module operations */
+
+static struct kvm_ptp_clock kvm_ptp_clock;
+
+static void __exit ptp_kvm_exit(void)
+{
+	ptp_clock_unregister(kvm_ptp_clock.ptp_clock);
+}
+
+static int __init ptp_kvm_init(void)
+{
+	if (!kvm_para_available())
+		return -ENODEV;
+
+	kvm_ptp_clock.caps = ptp_kvm_caps;
+
+	kvm_ptp_clock.ptp_clock = ptp_clock_register(&kvm_ptp_clock.caps, NULL);
+
+	if (IS_ERR(kvm_ptp_clock.ptp_clock))
+		return PTR_ERR(kvm_ptp_clock.ptp_clock);
+
+	clock_off_gpa = slow_virt_to_phys(&clock_off);
+
+	hv_clock = pvclock_pvti_cpu0_va();
+
+	return 0;
+}
+
+module_init(ptp_kvm_init);
+module_exit(ptp_kvm_exit);
+
+MODULE_AUTHOR("Marcelo Tosatti <mtosatti@redhat.com>");
+MODULE_DESCRIPTION("PTP clock using KVMCLOCK");
+MODULE_LICENSE("GPL");
Index: kvm-ptpdriver/drivers/ptp/Makefile
===================================================================
--- kvm-ptpdriver.orig/drivers/ptp/Makefile	2017-01-13 09:17:31.724568567 -0200
+++ kvm-ptpdriver/drivers/ptp/Makefile	2017-01-13 09:17:58.997609570 -0200
@@ -6,3 +6,4 @@
 obj-$(CONFIG_PTP_1588_CLOCK)		+= ptp.o
 obj-$(CONFIG_PTP_1588_CLOCK_IXP46X)	+= ptp_ixp46x.o
 obj-$(CONFIG_PTP_1588_CLOCK_PCH)	+= ptp_pch.o
+obj-$(CONFIG_PTP_1588_CLOCK_KVM)	+= ptp_kvm.o

[toc] | [next] | [standalone]

#1558527

2017-01-13 10:01-0200, Marcelo Tosatti:
> Add a driver with gettime method returning hosts realtime clock.
> This allows Chrony to synchronize host and guest clocks with 
> high precision (see results below).
> 
> chronyc> sources
> MS Name/IP address         Stratum Poll Reach LastRx Last sample
> ===============================================================================
> #* PHC0                          0   3   377     6     +4ns[   +4ns] +/-    3ns
> 
> To configure Chronyd to use PHC refclock, add the 
> following line to its configuration file:
> 
> refclock PHC /dev/ptpX poll 3 dpoll -2 offset 0
> 
> Where /dev/ptpX is the kvmclock PTP clock.
> 
> 
> ---
>  drivers/ptp/Kconfig   |   12 +++
>  drivers/ptp/Makefile  |    1 
>  drivers/ptp/ptp_kvm.c |  180 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 193 insertions(+)
> 
> Index: kvm-ptpdriver/drivers/ptp/Kconfig
> ===================================================================
> --- kvm-ptpdriver.orig/drivers/ptp/Kconfig	2017-01-13 09:17:31.724568567 -0200
> +++ kvm-ptpdriver/drivers/ptp/Kconfig	2017-01-13 09:55:33.344208894 -0200
> @@ -90,4 +90,16 @@
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called ptp_pch.
>  
> +config PTP_1588_CLOCK_KVM
> +	tristate "KVM virtual PTP clock"
> +	depends on PTP_1588_CLOCK
> +	depends on KVM_GUEST
> +	default y
> +	help
> +	  This driver adds support for using kvm infrastructure as a PTP
> +	  clock. This clock is only useful if you are using KVM guests.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called ptp_kvm.
> +
>  endmenu
> Index: kvm-ptpdriver/drivers/ptp/ptp_kvm.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ kvm-ptpdriver/drivers/ptp/ptp_kvm.c	2017-01-13 09:57:55.013440645 -0200
> @@ -0,0 +1,180 @@
> +/*
> + * Virtual PTP 1588 clock for use with KVM guests
> + *
> + * Copyright (C) 2017 Red Hat Inc.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + */
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <uapi/linux/kvm_para.h>
> +#include <asm/kvm_para.h>
> +#include <asm/pvclock.h>
> +#include <uapi/asm/kvm_para.h>
> +
> +#include <linux/ptp_clock_kernel.h>
> +
> +struct kvm_ptp_clock {
> +	struct ptp_clock *ptp_clock;
> +	struct ptp_clock_info caps;
> +};
> +
> +DEFINE_SPINLOCK(kvm_ptp_lock);
> +
> +static struct pvclock_vsyscall_time_info *hv_clock;
> +
> +/*
> + * PTP clock operations
> + */
> +
> +static int ptp_kvm_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int ptp_kvm_adjtime(struct ptp_clock_info *ptp, s64 delta)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static struct kvm_clock_offset clock_off;
> +static phys_addr_t clock_off_gpa;
> +
> +static int ptp_kvm_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
> +{
> +	unsigned long ret;
> +	struct timespec64 tspec;
> +	u64 delta;
> +	cycle_t offset;
> +	unsigned version;
> +	int cpu;
> +	struct pvclock_vcpu_time_info *src;
> +
> +	preempt_disable_notrace();
> +	cpu = smp_processor_id();
> +	src = &hv_clock[cpu].pvti;
> +
> +	spin_lock(&kvm_ptp_lock);
> +
> +	do {
> +		/*
> +		 * We are measuring the delay between
> +		 * kvm_hypercall and rdtsc using TSC,
> +		 * and converting that delta to
> +		 * tsc_to_system_mul and tsc_shift
> +		 * So any changes to tsc_to_system_mul
> +		 * and tsc_shift in this region
> +		 * invalidate the measurement.
> +		 */

This assumes that the host uses kvmclock, but the guest can be using
just TSC and even have kvmclock disabled.  This code should at least
check that kvmclock enabled.

(If we made kvmclock mandatory, then we could fix pvclock_wall_clock
 interface, because it is already defined to provide real time based on
 kvmclock ...)

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

> +
> +	return 0;
> +}
> +
> +static int ptp_kvm_settime(struct ptp_clock_info *ptp,
> +			   const struct timespec64 *ts)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int ptp_kvm_enable(struct ptp_clock_info *ptp,
> +			  struct ptp_clock_request *rq, int on)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static struct ptp_clock_info ptp_kvm_caps = {
> +	.owner		= THIS_MODULE,
> +	.name		= "KVM virtual PTP",
> +	.max_adj	= 0,
> +	.n_ext_ts	= 0,
> +	.n_pins		= 0,
> +	.pps		= 0,
> +	.adjfreq	= ptp_kvm_adjfreq,
> +	.adjtime	= ptp_kvm_adjtime,
> +	.gettime64	= ptp_kvm_gettime,
> +	.settime64	= ptp_kvm_settime,
> +	.enable		= ptp_kvm_enable,
> +};
> +
> +/* module operations */
> +
> +static struct kvm_ptp_clock kvm_ptp_clock;
> +
> +static void __exit ptp_kvm_exit(void)
> +{
> +	ptp_clock_unregister(kvm_ptp_clock.ptp_clock);
> +}
> +
> +static int __init ptp_kvm_init(void)
> +{
> +	if (!kvm_para_available())
> +		return -ENODEV;
> +
> +	kvm_ptp_clock.caps = ptp_kvm_caps;
> +
> +	kvm_ptp_clock.ptp_clock = ptp_clock_register(&kvm_ptp_clock.caps, NULL);

It is a shame that the infrastructure uses polling when the guest could
be notified on every host real time change, but this should be good
enough.

> +
> +	if (IS_ERR(kvm_ptp_clock.ptp_clock))
> +		return PTR_ERR(kvm_ptp_clock.ptp_clock);
> +
> +	clock_off_gpa = slow_virt_to_phys(&clock_off);
> +
> +	hv_clock = pvclock_pvti_cpu0_va();

Would safer to assign required globals before the registration -- races
could get ugly.

Thanks.

> +
> +	return 0;
> +}
> +
> +module_init(ptp_kvm_init);
> +module_exit(ptp_kvm_exit);
> +
> +MODULE_AUTHOR("Marcelo Tosatti <mtosatti@redhat.com>");
> +MODULE_DESCRIPTION("PTP clock using KVMCLOCK");
> +MODULE_LICENSE("GPL");
> Index: kvm-ptpdriver/drivers/ptp/Makefile
> ===================================================================
> --- kvm-ptpdriver.orig/drivers/ptp/Makefile	2017-01-13 09:17:31.724568567 -0200
> +++ kvm-ptpdriver/drivers/ptp/Makefile	2017-01-13 09:17:58.997609570 -0200
> @@ -6,3 +6,4 @@
>  obj-$(CONFIG_PTP_1588_CLOCK)		+= ptp.o
>  obj-$(CONFIG_PTP_1588_CLOCK_IXP46X)	+= ptp_ixp46x.o
>  obj-$(CONFIG_PTP_1588_CLOCK_PCH)	+= ptp_pch.o
> +obj-$(CONFIG_PTP_1588_CLOCK_KVM)	+= ptp_kvm.o
> 
> 

[toc] | [prev] | [next] | [standalone]

#1558649

On Fri, Jan 13, 2017 at 04:56:58PM +0100, Radim Krcmar wrote:
> 2017-01-13 10:01-0200, Marcelo Tosatti:
> > Add a driver with gettime method returning hosts realtime clock.
> > This allows Chrony to synchronize host and guest clocks with 
> > high precision (see results below).
> > 
> > chronyc> sources
> > MS Name/IP address         Stratum Poll Reach LastRx Last sample
> > ===============================================================================
> > #* PHC0                          0   3   377     6     +4ns[   +4ns] +/-    3ns
> > 
> > To configure Chronyd to use PHC refclock, add the 
> > following line to its configuration file:
> > 
> > refclock PHC /dev/ptpX poll 3 dpoll -2 offset 0
> > 
> > Where /dev/ptpX is the kvmclock PTP clock.
> > 
> > 
> > ---
> >  drivers/ptp/Kconfig   |   12 +++
> >  drivers/ptp/Makefile  |    1 
> >  drivers/ptp/ptp_kvm.c |  180 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 193 insertions(+)
> > 
> > Index: kvm-ptpdriver/drivers/ptp/Kconfig
> > ===================================================================
> > --- kvm-ptpdriver.orig/drivers/ptp/Kconfig	2017-01-13 09:17:31.724568567 -0200
> > +++ kvm-ptpdriver/drivers/ptp/Kconfig	2017-01-13 09:55:33.344208894 -0200
> > @@ -90,4 +90,16 @@
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called ptp_pch.
> >  
> > +config PTP_1588_CLOCK_KVM
> > +	tristate "KVM virtual PTP clock"
> > +	depends on PTP_1588_CLOCK
> > +	depends on KVM_GUEST
> > +	default y
> > +	help
> > +	  This driver adds support for using kvm infrastructure as a PTP
> > +	  clock. This clock is only useful if you are using KVM guests.
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called ptp_kvm.
> > +
> >  endmenu
> > Index: kvm-ptpdriver/drivers/ptp/ptp_kvm.c
> > ===================================================================
> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ kvm-ptpdriver/drivers/ptp/ptp_kvm.c	2017-01-13 09:57:55.013440645 -0200
> > @@ -0,0 +1,180 @@
> > +/*
> > + * Virtual PTP 1588 clock for use with KVM guests
> > + *
> > + * Copyright (C) 2017 Red Hat Inc.
> > + *
> > + *  This program is free software; you can redistribute it and/or modify
> > + *  it under the terms of the GNU General Public License as published by
> > + *  the Free Software Foundation; either version 2 of the License, or
> > + *  (at your option) any later version.
> > + *
> > + *  This program is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + *
> > + */
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <uapi/linux/kvm_para.h>
> > +#include <asm/kvm_para.h>
> > +#include <asm/pvclock.h>
> > +#include <uapi/asm/kvm_para.h>
> > +
> > +#include <linux/ptp_clock_kernel.h>
> > +
> > +struct kvm_ptp_clock {
> > +	struct ptp_clock *ptp_clock;
> > +	struct ptp_clock_info caps;
> > +};
> > +
> > +DEFINE_SPINLOCK(kvm_ptp_lock);
> > +
> > +static struct pvclock_vsyscall_time_info *hv_clock;
> > +
> > +/*
> > + * PTP clock operations
> > + */
> > +
> > +static int ptp_kvm_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static int ptp_kvm_adjtime(struct ptp_clock_info *ptp, s64 delta)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static struct kvm_clock_offset clock_off;
> > +static phys_addr_t clock_off_gpa;
> > +
> > +static int ptp_kvm_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
> > +{
> > +	unsigned long ret;
> > +	struct timespec64 tspec;
> > +	u64 delta;
> > +	cycle_t offset;
> > +	unsigned version;
> > +	int cpu;
> > +	struct pvclock_vcpu_time_info *src;
> > +
> > +	preempt_disable_notrace();
> > +	cpu = smp_processor_id();
> > +	src = &hv_clock[cpu].pvti;
> > +
> > +	spin_lock(&kvm_ptp_lock);
> > +
> > +	do {
> > +		/*
> > +		 * We are measuring the delay between
> > +		 * kvm_hypercall and rdtsc using TSC,
> > +		 * and converting that delta to
> > +		 * tsc_to_system_mul and tsc_shift
> > +		 * So any changes to tsc_to_system_mul
> > +		 * and tsc_shift in this region
> > +		 * invalidate the measurement.
> > +		 */
> 
> This assumes that the host uses kvmclock, but the guest can be using
> just TSC and even have kvmclock disabled.  This code should at least
> check that kvmclock enabled.

Fixed.

> (If we made kvmclock mandatory, then we could fix pvclock_wall_clock
>  interface, because it is already defined to provide real time based on
>  kvmclock ...)
> 
> > +		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.

> 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?

> > +
> > +	return 0;
> > +}
> > +
> > +static int ptp_kvm_settime(struct ptp_clock_info *ptp,
> > +			   const struct timespec64 *ts)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static int ptp_kvm_enable(struct ptp_clock_info *ptp,
> > +			  struct ptp_clock_request *rq, int on)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static struct ptp_clock_info ptp_kvm_caps = {
> > +	.owner		= THIS_MODULE,
> > +	.name		= "KVM virtual PTP",
> > +	.max_adj	= 0,
> > +	.n_ext_ts	= 0,
> > +	.n_pins		= 0,
> > +	.pps		= 0,
> > +	.adjfreq	= ptp_kvm_adjfreq,
> > +	.adjtime	= ptp_kvm_adjtime,
> > +	.gettime64	= ptp_kvm_gettime,
> > +	.settime64	= ptp_kvm_settime,
> > +	.enable		= ptp_kvm_enable,
> > +};
> > +
> > +/* module operations */
> > +
> > +static struct kvm_ptp_clock kvm_ptp_clock;
> > +
> > +static void __exit ptp_kvm_exit(void)
> > +{
> > +	ptp_clock_unregister(kvm_ptp_clock.ptp_clock);
> > +}
> > +
> > +static int __init ptp_kvm_init(void)
> > +{
> > +	if (!kvm_para_available())
> > +		return -ENODEV;
> > +
> > +	kvm_ptp_clock.caps = ptp_kvm_caps;
> > +
> > +	kvm_ptp_clock.ptp_clock = ptp_clock_register(&kvm_ptp_clock.caps, NULL);
> 
> It is a shame that the infrastructure uses polling when the guest could
> be notified on every host real time change, but this should be good
> enough.

Well, the whole NTP scheme is a feedback loop, so it wants to query
the value of the clock at the moments the feedback loop logic decides it.

> > +	if (IS_ERR(kvm_ptp_clock.ptp_clock))
> > +		return PTR_ERR(kvm_ptp_clock.ptp_clock);
> > +
> > +	clock_off_gpa = slow_virt_to_phys(&clock_off);
> > +
> > +	hv_clock = pvclock_pvti_cpu0_va();
> 
> Would safer to assign required globals before the registration -- races
> could get ugly.

Fixed, thanks.

[toc] | [prev] | [next] | [standalone]

#1559875

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

[toc] | [prev] | [next] | [standalone]

#1559898

2017-01-16 17:26+0100, 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.

There is getcrosststamp() callback in PTP, which seems to be exactly
what we want when pairing with TSC, so the pvclock delay fixup can be
dropped when using it.

[toc] | [prev] | [next] | [standalone]

#1559914

On Mon, Jan 16, 2017 at 05:54:11PM +0100, Radim Krcmar wrote:
> 2017-01-16 17:26+0100, 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.
> 
> There is getcrosststamp() callback in PTP, which seems to be exactly
> what we want when pairing with TSC, so the pvclock delay fixup can be
> dropped when using it.

What pvclock delay fixup you refer to? The "rdtsc() - clock_offset.tsc"
part? You can't drop it, because if you do then your "host realtime
clock read" will be behind by "rdtsc() - clock_offset.tsc" TSC cycles.
We want the highest precision as possible.

[toc] | [prev] | [next] | [standalone]

#1559930

2017-01-16 15:08-0200, Marcelo Tosatti:
> On Mon, Jan 16, 2017 at 05:54:11PM +0100, Radim Krcmar wrote:
>> 2017-01-16 17:26+0100, 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.
>> 
>> There is getcrosststamp() callback in PTP, which seems to be exactly
>> what we want when pairing with TSC, so the pvclock delay fixup can be
>> dropped when using it.
> 
> What pvclock delay fixup you refer to? The "rdtsc() - clock_offset.tsc"
> part?

Yes.

>       You can't drop it, because if you do then your "host realtime
> clock read" will be behind by "rdtsc() - clock_offset.tsc" TSC cycles.

The TSC read will be some cycles old when the hypercall ends, but that
doesn't matter, because we will pass {sec, nsec, guest_tsc} to PTP and
PTP should plug them into kernel's realtime clock roughly like this:

  sec/nsec + (rdtsc() - guest_tsc) * tsc_freq

Adding delay to guest_tsc and sec/nsec cannot improve precision.
(And will likely degrade it as kvmclock's frequency is incorrect.)

> We want the highest precision as possible.

I agree, which is why we don't want to lose precision in the delay
guesswork because of gettime64().

[toc] | [prev] | [next] | [standalone]

#1559944

On Mon, Jan 16, 2017 at 06:27:58PM +0100, Radim Krcmar wrote:
> 2017-01-16 15:08-0200, Marcelo Tosatti:
> > On Mon, Jan 16, 2017 at 05:54:11PM +0100, Radim Krcmar wrote:
> >> 2017-01-16 17:26+0100, 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.
> >> 
> >> There is getcrosststamp() callback in PTP, which seems to be exactly
> >> what we want when pairing with TSC, so the pvclock delay fixup can be
> >> dropped when using it.
> > 
> > What pvclock delay fixup you refer to? The "rdtsc() - clock_offset.tsc"
> > part?
> 
> Yes.
> 
> >       You can't drop it, because if you do then your "host realtime
> > clock read" will be behind by "rdtsc() - clock_offset.tsc" TSC cycles.
> 
> The TSC read will be some cycles old when the hypercall ends, but that
> doesn't matter, because we will pass {sec, nsec, guest_tsc} to PTP and
> PTP should plug them into kernel's realtime clock roughly like this:
> 
>   sec/nsec + (rdtsc() - guest_tsc) * tsc_freq
> 
> Adding delay to guest_tsc and sec/nsec cannot improve precision.
> (And will likely degrade it as kvmclock's frequency is incorrect.)
> 
> > We want the highest precision as possible.
> 
> I agree, which is why we don't want to lose precision in the delay
> guesswork because of gettime64().

Sorry the clock difference is 10ns now. So the guest clock is off by _10 ns_ 
of the host clock.

You are suggesting to use getcrosststamp instead, to drop the (rdtsc() -
guest_tsc) part ?

Please be more verbose.

[toc] | [prev] | [next] | [standalone]

#1559959

2017-01-16 15:39-0200, Marcelo Tosatti:
> On Mon, Jan 16, 2017 at 06:27:58PM +0100, Radim Krcmar wrote:
>> 2017-01-16 15:08-0200, Marcelo Tosatti:
>> > On Mon, Jan 16, 2017 at 05:54:11PM +0100, Radim Krcmar wrote:
>> >> 2017-01-16 17:26+0100, 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.
>> >> 
>> >> There is getcrosststamp() callback in PTP, which seems to be exactly
>> >> what we want when pairing with TSC, so the pvclock delay fixup can be
>> >> dropped when using it.
>> > 
>> > What pvclock delay fixup you refer to? The "rdtsc() - clock_offset.tsc"
>> > part?
>> 
>> Yes.
>> 
>> >       You can't drop it, because if you do then your "host realtime
>> > clock read" will be behind by "rdtsc() - clock_offset.tsc" TSC cycles.
>> 
>> The TSC read will be some cycles old when the hypercall ends, but that
>> doesn't matter, because we will pass {sec, nsec, guest_tsc} to PTP and
>> PTP should plug them into kernel's realtime clock roughly like this:
>> 
>>   sec/nsec + (rdtsc() - guest_tsc) * tsc_freq
>> 
>> Adding delay to guest_tsc and sec/nsec cannot improve precision.
>> (And will likely degrade it as kvmclock's frequency is incorrect.)
>> 
>> > We want the highest precision as possible.
>> 
>> I agree, which is why we don't want to lose precision in the delay
>> guesswork because of gettime64().
> 
> Sorry the clock difference is 10ns now. So the guest clock is off by _10 ns_ 
> of the host clock.

That is pretty good.

> You are suggesting to use getcrosststamp instead, to drop the (rdtsc() -
> guest_tsc) part ?

Yes, it results in simpler code, doesn't create dependency on the
dreaded kvmclock, and is the best we can currently do wrt. precision.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1559922

On Mon, Jan 16, 2017 at 05:26:53PM +0100, Radim Krcmar wrote:
> 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.

Not random time in the past. We return {sec, nsec} from the host
realtime at the moment the user ran the hypercall. 

Since PTP is very accurate, that "a bit more" counts, yes.

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

Well, no. For one, the TSC part of kvmclock: 

	kvmclock-read = system_timestamp + convert-to-1GHz(rdtsc() - tsc_timestamp)
				           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Drifts relative to UTC. This part can be large.
The guests NTP is responsible for fixing
that drift of the guests realtime clock (talking about current setup, 
without KVM PTP driver).

Now, we want very high precision (less than 1us) for this
driver. Very small TSC drifts on a large delta defeat the purpose.

> Without a VM exit.

Huge performance is not an issue. Accuracy (how different from the host
realtime clock our "approximation" of the host realtime clock) is.

> And how often is ptp_kvm_gettime() usually called?

The PTP_SYS_OFFSET ioctl calls the following code in a loop:

struct ptp_sys_offset {
        unsigned int n_samples; /* Desired number of measurements. */
        unsigned int rsv[3];    /* Reserved for future use. */
        /*
         * Array of interleaved system/phc time stamps. The kernel
         * will provide 2*n_samples + 1 time stamps, with the last
         * one as a system time stamp.
         */
        struct ptp_clock_time ts[2 * PTP_MAX_SAMPLES + 1];
};

#define PTP_MAX_SAMPLES 25 /* Maximum allowed offset measurement
samples. */

        case PTP_SYS_OFFSET:
                sysoff = memdup_user((void __user *)arg,
sizeof(*sysoff));
                if (IS_ERR(sysoff)) {
                        err = PTR_ERR(sysoff);
                        sysoff = NULL;
                        break;
                }
                if (sysoff->n_samples > PTP_MAX_SAMPLES) {
                        err = -EINVAL;
                        break;
                }
                pct = &sysoff->ts[0];
                for (i = 0; i < sysoff->n_samples; i++) {
                        getnstimeofday64(&ts);
                        pct->sec = ts.tv_sec;
                        pct->nsec = ts.tv_nsec;
                        pct++;
                        ptp->info->gettime64(ptp->info, &ts);
                        pct->sec = ts.tv_sec;
                        pct->nsec = ts.tv_nsec;
                        pct++;
                }
                getnstimeofday64(&ts);
                pct->sec = ts.tv_sec;
                pct->nsec = ts.tv_nsec;

How often that ioctl is called depends on the parameters of the Chrony
PHC code. Initially (to determine the clock difference Chrony should call it 
more frequently, later on it should call it less frequency).

Perhaps once every second initially (the ioctl). I'll confirm with the
exact value for my setup and reply to this email.


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

Ah, OK. I understand you now understood the meaning of "tsc"
part of the {sec, nsec, guest_tsc} triple.

[toc] | [prev] | [next] | [standalone]

#1559946

2017-01-16 15:04-0200, Marcelo Tosatti:
> On Mon, Jan 16, 2017 at 05:26:53PM +0100, Radim Krcmar wrote:
>> 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.
> 
> Not random time in the past. We return {sec, nsec} from the host
> realtime at the moment the user ran the hypercall. 

That is what we want, but {sec, nsec} is not anchored to any running
time, so it is unavoidably late when we return it.

> Since PTP is very accurate, that "a bit more" counts, yes.
> 
>> 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.  
> 
> Well, no. For one, the TSC part of kvmclock: 
> 
> 	kvmclock-read = system_timestamp + convert-to-1GHz(rdtsc() - tsc_timestamp)
> 				           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Drifts relative to UTC. This part can be large.
> The guests NTP is responsible for fixing
> that drift of the guests realtime clock (talking about current setup, 
> without KVM PTP driver).
> 
> Now, we want very high precision (less than 1us) for this
> driver. Very small TSC drifts on a large delta defeat the purpose.

True.

>> Without a VM exit.
> 
> Huge performance is not an issue. Accuracy (how different from the host
> realtime clock our "approximation" of the host realtime clock) is.

Ah, ok.

>> And how often is ptp_kvm_gettime() usually called?
> 
> The PTP_SYS_OFFSET ioctl calls the following code in a loop:
> 
> struct ptp_sys_offset {
>         unsigned int n_samples; /* Desired number of measurements. */
>         unsigned int rsv[3];    /* Reserved for future use. */
>         /*
>          * Array of interleaved system/phc time stamps. The kernel
>          * will provide 2*n_samples + 1 time stamps, with the last
>          * one as a system time stamp.
>          */
>         struct ptp_clock_time ts[2 * PTP_MAX_SAMPLES + 1];
> };
> 
> #define PTP_MAX_SAMPLES 25 /* Maximum allowed offset measurement
> samples. */
> 
>         case PTP_SYS_OFFSET:
>                 sysoff = memdup_user((void __user *)arg,
> sizeof(*sysoff));
>                 if (IS_ERR(sysoff)) {
>                         err = PTR_ERR(sysoff);
>                         sysoff = NULL;
>                         break;
>                 }
>                 if (sysoff->n_samples > PTP_MAX_SAMPLES) {
>                         err = -EINVAL;
>                         break;
>                 }
>                 pct = &sysoff->ts[0];
>                 for (i = 0; i < sysoff->n_samples; i++) {
>                         getnstimeofday64(&ts);
>                         pct->sec = ts.tv_sec;
>                         pct->nsec = ts.tv_nsec;
>                         pct++;
>                         ptp->info->gettime64(ptp->info, &ts);
>                         pct->sec = ts.tv_sec;
>                         pct->nsec = ts.tv_nsec;
>                         pct++;

Hm, this loop alternates between system time and ptp time and I'd guess
that userspace then manipulates deltas of these two readings and applies
the result to system time.
I'm not even sure that pvclock delta improves the situation anymore --
it adds an offset to all gettime64() reads, but that can be computed
from multiple datapoints.  (And the delta also adds uncertainty to
results.)

>                 }
>                 getnstimeofday64(&ts);
>                 pct->sec = ts.tv_sec;
>                 pct->nsec = ts.tv_nsec;
> 
> How often that ioctl is called depends on the parameters of the Chrony
> PHC code. Initially (to determine the clock difference Chrony should call it 
> more frequently, later on it should call it less frequency).
> 
> Perhaps once every second initially (the ioctl). I'll confirm with the
> exact value for my setup and reply to this email.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1559015

On Fri, Jan 13, 2017 at 04:56:58PM +0100, Radim Krcmar wrote:
> > +static int __init ptp_kvm_init(void)
> > +{
> > +	if (!kvm_para_available())
> > +		return -ENODEV;
> > +
> > +	kvm_ptp_clock.caps = ptp_kvm_caps;
> > +
> > +	kvm_ptp_clock.ptp_clock = ptp_clock_register(&kvm_ptp_clock.caps, NULL);
> 
> It is a shame that the infrastructure uses polling when the guest could
> be notified on every host real time change, but this should be good
> enough.

This comment makes no sense at all.  What do you mean by "host real
time change"?

Thanks,
Richard

[toc] | [prev] | [next] | [standalone]

#1559846

2017-01-14 16:26+0100, Richard Cochran:
> On Fri, Jan 13, 2017 at 04:56:58PM +0100, Radim Krcmar wrote:
>> > +static int __init ptp_kvm_init(void)
>> > +{
>> > +	if (!kvm_para_available())
>> > +		return -ENODEV;
>> > +
>> > +	kvm_ptp_clock.caps = ptp_kvm_caps;
>> > +
>> > +	kvm_ptp_clock.ptp_clock = ptp_clock_register(&kvm_ptp_clock.caps, NULL);
>> 
>> It is a shame that the infrastructure uses polling when the guest could
>> be notified on every host real time change, but this should be good
>> enough.
> 
> This comment makes no sense at all.  What do you mean by "host real
> time change"?

Real time in the sense of wall clock, as perceived by the host, and the
change of that time.

Unlike other PTP drivers, host (source) and guest (destination) share
the same hardware clock, so they cannot shift or drift unless one of
them changes its "TSC to real time" conversion (the host is most likely
using NTP/PTP to keep its own real time).

I meant that the host could notify the guest when a change happens,
which would be more efficient.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web