KVM virtual PTP driver (v3)

To be used by KVM PTP driver.

Signed-off-by Marcelo Tosatti <[email protected]>

---
 arch/x86/include/asm/kvmclock.h |    6 ++++++
 arch/x86/kernel/kvmclock.c      |    6 ++++++
 2 files changed, 12 insertions(+)

Index: kvm-ptpdriver/arch/x86/include/asm/kvmclock.h
===================================================================

arch/x86/include/asm/kvmclock.h new file +6 -0

@@ -0,0 +1,6 @@
+#ifndef _ASM_X86_KVM_CLOCK_H
+#define _ASM_X86_KVM_CLOCK_H
+
+struct clocksource *get_kvmclock_cs(void);
+
+#endif /* _ASM_X86_KVM_CLOCK_H */

Index: kvm-ptpdriver/arch/x86/kernel/kvmclock.c
===================================================================

arch/x86/kernel/kvmclock.c +1 -0

@@ -28,6 +28,7 @@
 
 #include <asm/x86_init.h>
 #include <asm/reboot.h>
+#include <asm/kvmclock.h>
 
 static int kvmclock __ro_after_init = 1;
 static int msr_kvm_system_time = MSR_KVM_SYSTEM_TIME;

arch/x86/kernel/kvmclock.c +2 -0

+}
+
 int kvm_register_clock(char *txt)
 {
 	int cpu = smp_processor_id();

 kernel/time/timekeeping.c        |   79 +++++++++++++++++++++++++++++++++++++++

Quoted 1 line · Marcelo Tosatti

 kernel/time/timekeeping.c        |   79 +++++++++++++++++++++++++++++++++++++++

Why not leave this in drivers/ptp/ptp_chardev.c?

drivers/ptp/ptp_chardev.c line 227

Quoted 4 lines

drivers/ptp/ptp_chardev.c +4 -0

+		if (ptp->info->emulate_ptp_sys_offset_mean) {
+			err = emulate_ptp_sys_offset(ptp->info, sysoff, arg);
+			break;
+		}

I think this should be simply "if (!ptp->info->gettime64)" and, likewise, there should be an emulation based getcrosststamp in ptp_clock_gettime.

Paolo

Quoted 6 lines · Paolo Bonzini

Quoted 1 line · Marcelo Tosatti

 kernel/time/timekeeping.c        |   79 +++++++++++++++++++++++++++++++++++++++

Why not leave this in drivers/ptp/ptp_chardev.c?

timekeeper_lock

Quoted 9 lines

drivers/ptp/ptp_chardev.c line 227

Quoted 4 lines

drivers/ptp/ptp_chardev.c +4 -0

+		if (ptp->info->emulate_ptp_sys_offset_mean) {
+			err = emulate_ptp_sys_offset(ptp->info, sysoff, arg);
+			break;
+		}

I think this should be simply "if (!ptp->info->gettime64)" and, likewise, there should be an emulation based getcrosststamp in ptp_clock_gettime.

Quoted 1 line

Paolo

gettime64 is called directly via ptp_clock_gettime.

Quoted 4 lines · Marcelo Tosatti

Quoted 3 lines · Paolo Bonzini

On 20/01/2017 13:20, Marcelo Tosatti wrote:

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

 kernel/time/timekeeping.c        |   79 +++++++++++++++++++++++++++++++++++++++

Why not leave this in drivers/ptp/ptp_chardev.c?

timekeeper_lock

Why does emulate_ptp_sys_offset need it, if the current PTP_SYS_OFFSET code doesn't?

Is the latency acceptable (considering this is a raw spin
lock) or is there a seqlock that we can use instead (such as tk_core.seq like in get_device_system_crosststamp)?

Quoted 6 lines · Paolo Bonzini

Quoted 5 lines · Marcelo Tosatti

Quoted 1 line

 kernel/time/timekeeping.c        |   79 +++++++++++++++++++++++++++++++++++++++

Quoted 3 lines

Quoted 1 line

Why not leave this in drivers/ptp/ptp_chardev.c?

timekeeper_lock

Why does emulate_ptp_sys_offset need it, if the current PTP_SYS_OFFSET code doesn't? Is the latency acceptable (considering this is a raw spin lock) or is there a seqlock that we can use instead (such as tk_core.seq like in get_device_system_crosststamp)?

Quoted 9 lines

Quoted 9 lines

drivers/ptp/ptp_chardev.c line 227

Quoted 4 lines

drivers/ptp/ptp_chardev.c +4 -0

+		if (ptp->info->emulate_ptp_sys_offset_mean) {
+			err = emulate_ptp_sys_offset(ptp->info, sysoff, arg);
+			break;
+		}

I think this should be simply "if (!ptp->info->gettime64)" and, likewise, there should be an emulation based getcrosststamp in ptp_clock_gettime.

Quoted 3 lines

Quoted 1 line

Paolo

gettime64 is called directly via ptp_clock_gettime.

Yes, but ptp_clock_gettime can be taught to use getcrosststamp instead.

2017-01-20 14:36+0100, Paolo Bonzini:

Quoted 7 lines · Marcelo Tosatti

Quoted 6 lines · Paolo Bonzini

Quoted 5 lines · Marcelo Tosatti

Quoted 1 line

 kernel/time/timekeeping.c        |   79 +++++++++++++++++++++++++++++++++++++++

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

Why not leave this in drivers/ptp/ptp_chardev.c?

timekeeper_lock

Why does emulate_ptp_sys_offset need it, if the current PTP_SYS_OFFSET code doesn't? Is the latency acceptable (considering this is a raw spin lock) or is there a seqlock that we can use instead (such as tk_core.seq like in get_device_system_crosststamp)?

The spinlock prevents writers to take the tk_core.seq, which means that time cannot be changed during that.

The simplest alternative would be to use tk_core.seq for all our reads, but that would increse the chance of re-reading, even infinitely.

But we don't need to read everything with the same time base -- if the time is changed (by NTP/user/...) between our reads, then the value will be off, but if writer took tk_core.seq just to accumulate current time, then the time after accumulation stays the same and it would work as if we had the tk_core.seq for the whole time.

Another solution would be to do just one one read and set it to all saples -- the difference between t[i] and t[i+2] would be 0. We are quite sure the just one read is enough, this hack could be even better.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

drivers/ptp/ptp_chardev.c line 227

Quoted 4 lines

drivers/ptp/ptp_chardev.c +4 -0

+		if (ptp->info->emulate_ptp_sys_offset_mean) {
+			err = emulate_ptp_sys_offset(ptp->info, sysoff, arg);
+			break;
+		}

I think this should be simply "if (!ptp->info->gettime64)" and, likewise, there should be an emulation based getcrosststamp in ptp_clock_gettime.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Paolo

gettime64 is called directly via ptp_clock_gettime.

Yes, but ptp_clock_gettime can be taught to use getcrosststamp instead.

I agree,

if (!gettime64)
use getcrosststamp

and KVM PTP device will not implement gettime64().

Quoted 3 lines · Radim Krcmar

Another solution would be to do just one one read and set it to all saples -- the difference between t[i] and t[i+2] would be 0. We are quite sure the just one read is enough, this hack could be even better.

I'd be afraid of messing up chrony's stats...

Quoted 20 lines

2017-01-20 14:36+0100, Paolo Bonzini:

Quoted 7 lines · Marcelo Tosatti

Quoted 6 lines · Paolo Bonzini

Quoted 5 lines · Marcelo Tosatti

Quoted 1 line

 kernel/time/timekeeping.c        |   79 +++++++++++++++++++++++++++++++++++++++

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

Why not leave this in drivers/ptp/ptp_chardev.c?

timekeeper_lock

Why does emulate_ptp_sys_offset need it, if the current PTP_SYS_OFFSET code doesn't? Is the latency acceptable (considering this is a raw spin lock) or is there a seqlock that we can use instead (such as tk_core.seq like in get_device_system_crosststamp)?

The spinlock prevents writers to take the tk_core.seq, which means that time cannot be changed during that.

Quoted 2 lines

The simplest alternative would be to use tk_core.seq for all our reads, but that would increse the chance of re-reading, even infinitely.

How much? If a hypercall takes 1 microsecond, and PTP_MAX_SAMPLES is 25, we should be done in less than 50 microseconds. If update_wall-time is called with 250 Hz frequency (sounds like a lot), that's still 4000 microseconds so the probability of even 3-4 consecutive retries should be very low.

Quoted 5 lines

But we don't need to read everything with the same time base -- if the time is changed (by NTP/user/...) between our reads, then the value will be off, but if writer took tk_core.seq just to accumulate current time, then the time after accumulation stays the same and it would work as if we had the tk_core.seq for the whole time.

You mean only check seqlock separately for each sample, but restart the entire loop upon changes to cs_was_changed_seq or clock_was_set_seq? That would work too.

Emulate PTP_SYS_OFFSET by using an arithmetic mean of the realtime samples from ->getcrosststamp callback.

Quoted 2 lines · Marcelo Tosatti

Emulate PTP_SYS_OFFSET by using an arithmetic mean of the realtime samples from ->getcrosststamp callback.

This change log is not very informative.

Yes, I can see that the new code calculates a mean.

But WHY is this needed?

From the following PTP_SYS_OFFSET_PRECISE data:

And why is this kvm case so special, that it requires its own new flag within the generic PHC subsystem?

This is needed to generate the PTP_SYS_OFFSET data: a table with read from realtime clock, read from device clock, read from realtime clock, ... :

time ->
device clock | |sample2| |sample4| |sample6| ...
-------------------------------------------------------------
realtime clock |sample1| |sample3| |sample5|

Where sampleN is the read of the respective clock.

From the following PTP_SYS_OFFSET_PRECISE data:

device clock |sample1P,deviceclock| |sample2P,deviceclock|
-------------------------------------------------------------
realtime clock |sample1P,realtimeclock| |sample2P,realtimeclock|

Quoted 3 lines

device clock |sample1P,deviceclock| |sample2P,deviceclock|
-------------------------------------------------------------
realtime clock |sample1P,realtimeclock| |sample2P,realtimeclock|

Are |sample1P,deviceclock| and |sample1P,realtimeclock| taken at the same instant in time?

If not, then calling that PTP_SYS_OFFSET_PRECISE is misleading.

Still don't get what you are doing here...

If you just implement getclock64 the PTP_SYS_OFFSET output:

device clock | |sample2| |sample4| |sample6| ...
-------------------------------------------------------------
realtime clock |sample1| |sample3| |sample5|

has a very large distance between samples on the same line (about 1 us), and I think it is too noisy for userspace to make sense of the output.

Marcelo's patch then produces fake realtime clock samples that, however, let chrony derive the cross timestamps that KVM produced in the first place. The outcome is really great accuracy compared to previous versions of the patch, often just +/- 2 or 3 nanoseconds.

Quoted 8 lines · Richard Cochran

Quoted 3 lines

device clock |sample1P,deviceclock| |sample2P,deviceclock|
-------------------------------------------------------------
realtime clock |sample1P,realtimeclock| |sample2P,realtimeclock|

Are |sample1P,deviceclock| and |sample1P,realtimeclock| taken at the same instant in time?

If not, then calling that PTP_SYS_OFFSET_PRECISE is misleading.

Yes, of course. This was added for the e1000e drivers first, but chrony isn't using it yet. In the case of KVM, the same host TSC value is used to produce the host clock and (converted to guest TSC) the guest clock.

If you just implement getclock64 the PTP_SYS_OFFSET output:

device clock | |sample2| |sample4| |sample6| ...
-------------------------------------------------------------
realtime clock |sample1| |sample3| |sample5|

has a very large distance between samples on the same line (about 1 us), and I think it is too noisy for userspace to make sense of the output.

So, on one hand chrony only uses the mean of realtime clock samples, in an attempt to produce precise cross timestamps. On the other hand, even though KVM could produce those natively, chrony does not support PTP_SYS_OFFSET_PRECISE.

Marcelo's patch then produces fake realtime clock samples that, however, let chrony derive the cross timestamps that KVM produced in the first place. The outcome is really great accuracy compared to previous versions of the patch, often just +/- 2 or 3 nanoseconds.

Paolo

I'm resending v5 with native support for ->gettime and ->getcrosststamps.

Quoted 4 lines · Marcelo Tosatti

This is needed to generate the PTP_SYS_OFFSET data: a table with read from realtime clock, read from device clock, read from realtime clock, ... :

Quoted 4 lines

time ->
device clock | |sample2| |sample4| |sample6| ...
-------------------------------------------------------------
realtime clock |sample1| |sample3| |sample5|

Here "realtime clock" is CLOCK_REALTIME on the guest, and "device clock" is CLOCK_REALTIME on the host?

Quoted 8 lines

Where sampleN is the read of the respective clock.

From the following PTP_SYS_OFFSET_PRECISE data:

device clock |sample1P,deviceclock| |sample2P,deviceclock|
-------------------------------------------------------------
realtime clock |sample1P,realtimeclock| |sample2P,realtimeclock|

Are |sample1P,deviceclock| and |sample1P,realtimeclock| taken at the same instant in time?

If not, then calling that PTP_SYS_OFFSET_PRECISE is misleading.

Still don't get what you are doing here...

Can you please describe what problem exists with this scheme?

Quoted 1 line · Marcelo Tosatti

Can you please describe what problem exists with this scheme?

This new kernel code exists just because chrony doesn't implement the PRECISE ioctl. Instead of adding new "fake" modes, just teach chrony about the better method.

drivers/ptp/ptp_kvm.c line 86

Quoted 10 lines · Marcelo Tosatti

drivers/ptp/ptp_kvm.c new file +10 -0

+		ret = kvm_hypercall2(KVM_HC_CLOCK_PAIRING,
+				     clock_off_gpa,
+				     KVM_CLOCK_PAIRING_WALLCLOCK);
+		if (ret != 0) {
+			pr_err("clock offset hypercall ret %lu\n", ret);
+			spin_unlock(&kvm_ptp_lock);
+			preempt_enable_notrace();
+			return -EOPNOTSUPP;
+		}
+

Is it worth making this hypercall, or even all of ptp_kvm_get_time_fn, a pv_ops entry?

But this looks good already, apart from my different preference on emulate_ptp_sys_offset_mean.

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 4 +162ns[ -683ns] +/- 11ns

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.

Signed-off-by Marcelo Tosatti <[email protected]>

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

v2: check for kvmclock (Radim)
initialize global variables before device registration (Radim)
v3: use cross timestamps callback (Paolo, Miroslav, Radim)

Index: kvm-ptpdriver/drivers/ptp/ptp_kvm.c
===================================================================

drivers/ptp/ptp_kvm.c new file +53 -0

@@ -0,0 +1,213 @@
+/*
+ * 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 <asm/kvmclock.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;
+
+static struct kvm_clock_offset clock_off;
+static phys_addr_t clock_off_gpa;
+
+/*
+ * system_counterval.cycles: kvmclock value com TSC do host.
+ * system_counterval.cs: kvmclock clocksource.
+ * device_time: host realtime clock.
+ *
+ */
+static int ptp_kvm_get_time_fn(ktime_t *device_time,
+			       struct system_counterval_t *system_counter,
+			       void *ctx)
+{
+	unsigned long ret;
+	struct timespec64 tspec;

drivers/ptp/ptp_kvm.c new file +23 -0

+
+	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_PAIRING,
+				     clock_off_gpa,
+				     KVM_CLOCK_PAIRING_WALLCLOCK);
+		if (ret != 0) {
+			pr_err("clock offset hypercall ret %lu\n", ret);
+			spin_unlock(&kvm_ptp_lock);
+			preempt_enable_notrace();
+			return -EOPNOTSUPP;
+		}
+

2017-01-20 10:20-0200, Marcelo Tosatti:

Review

Quoted 1 line

(quoted diff) +1 -0

+	spin_lock(&kvm_ptp_lock);

What does the lock prevent?

2017-01-20 10:20-0200, Marcelo Tosatti:

Quoted 90 lines

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 4 +162ns[ -683ns] +/- 11ns

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.

Signed-off-by Marcelo Tosatti <[email protected]>

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

v2: check for kvmclock (Radim)
initialize global variables before device registration (Radim)
v3: use cross timestamps callback (Paolo, Miroslav, Radim)

Index: kvm-ptpdriver/drivers/ptp/ptp_kvm.c
===================================================================

drivers/ptp/ptp_kvm.c new file +57 -0

@@ -0,0 +1,213 @@
+/*
+ * 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 <asm/kvmclock.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;
+
+static struct kvm_clock_offset clock_off;
+static phys_addr_t clock_off_gpa;
+
+/*
+ * system_counterval.cycles: kvmclock value com TSC do host.
+ * system_counterval.cs: kvmclock clocksource.
+ * device_time: host realtime clock.
+ *
+ */
+static int ptp_kvm_get_time_fn(ktime_t *device_time,
+			       struct system_counterval_t *system_counter,
+			       void *ctx)
+{
+	unsigned long ret;
+	struct timespec64 tspec;
+	unsigned version;
+	u8 flags;
+	int cpu;
+	struct pvclock_vcpu_time_info *src;

drivers/ptp/ptp_kvm.c line 63

Quoted 6 lines

drivers/ptp/ptp_kvm.c new file +6 -0

+
+	preempt_disable_notrace();
+	cpu = smp_processor_id();
+	src = &hv_clock[cpu].pvti;
+
+	spin_lock(&kvm_ptp_lock);

What does the lock prevent?

Quoted 28 lines

drivers/ptp/ptp_kvm.c new file +28 -0

+
+	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_PAIRING,
+				     clock_off_gpa,
+				     KVM_CLOCK_PAIRING_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;
+		ret = __pvclock_read_cycles(src, clock_off.tsc);
+		flags = src->flags;
+	} while (pvclock_read_retry(src, version));

drivers/ptp/ptp_kvm.c line 96

Quoted 5 lines

drivers/ptp/ptp_kvm.c new file +5 -0

+
+	preempt_enable_notrace();
+
+	system_counter->cycles = ret;
+	system_counter->cs = get_kvmclock_cs();

Can't we use clocksource_tsc and just pass the tsc without kvmclock in the middle?

drivers/ptp/ptp_kvm.c line 98

Quoted 1 line

drivers/ptp/ptp_kvm.c new file +1 -0

+	tspec.tv_nsec = tspec.tv_nsec;

(This looks extraneous.)

Thanks.

Quoted 4 lines

drivers/ptp/ptp_kvm.c line 96

Quoted 1 line

drivers/ptp/ptp_kvm.c new file +1 -0

+	system_counter->cs = get_kvmclock_cs();

Can't we use clocksource_tsc and just pass the tsc without kvmclock in the middle?

No, it has to be the kvmclock value.

Quoted 28 lines

2017-01-20 10:20-0200, Marcelo Tosatti:

Quoted 27 lines

drivers/ptp/ptp_kvm.c new file +27 -0

+	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_PAIRING,
+				     clock_off_gpa,
+				     KVM_CLOCK_PAIRING_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;
+		ret = __pvclock_read_cycles(src, clock_off.tsc);
+		flags = src->flags;
+	} while (pvclock_read_retry(src, version));

Quoted 8 lines

drivers/ptp/ptp_kvm.c line 96

Quoted 5 lines

drivers/ptp/ptp_kvm.c new file +5 -0

+
+	preempt_enable_notrace();
+
+	system_counter->cycles = ret;
+	system_counter->cs = get_kvmclock_cs();

Can't we use clocksource_tsc and just pass the tsc without kvmclock in the middle?

No, it has to be the kvmclock value.

2017-01-20 13:00-0200, Marcelo Tosatti:

Quoted 38 lines · Radim Krcmar

Quoted 36 lines

2017-01-20 10:20-0200, Marcelo Tosatti:

Quoted 22 lines

drivers/ptp/ptp_kvm.c new file +22 -0

+	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_PAIRING,
+				     clock_off_gpa,
+				     KVM_CLOCK_PAIRING_WALLCLOCK);
+		if (ret != 0) {
+			pr_err("clock offset hypercall ret %lu\n", ret);
+			spin_unlock(&kvm_ptp_lock);
+			preempt_enable_notrace();
+			return -EOPNOTSUPP;
+		}
+

drivers/ptp/ptp_kvm.c line 96

Quoted 10 lines

drivers/ptp/ptp_kvm.c new file +10 -0

+		tspec.tv_sec = clock_off.sec;
+		tspec.tv_nsec = clock_off.nsec;
+		ret = __pvclock_read_cycles(src, clock_off.tsc);
+		flags = src->flags;
+	} while (pvclock_read_retry(src, version));
+
+	preempt_enable_notrace();
+
+	system_counter->cycles = ret;
+	system_counter->cs = get_kvmclock_cs();

Can't we use clocksource_tsc and just pass the tsc without kvmclock in the middle?

Quoted 1 line

No, it has to be the kvmclock value.

What happens if the guest switches from kvmclock to tsc?

From get_device_system_crosststamp function:

2017-01-20 13:00-0200, Marcelo Tosatti:

Quoted 38 lines · Radim Krcmar

Quoted 36 lines

2017-01-20 10:20-0200, Marcelo Tosatti:

Quoted 22 lines

drivers/ptp/ptp_kvm.c new file +22 -0

+	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_PAIRING,
+				     clock_off_gpa,
+				     KVM_CLOCK_PAIRING_WALLCLOCK);
+		if (ret != 0) {
+			pr_err("clock offset hypercall ret %lu\n", ret);
+			spin_unlock(&kvm_ptp_lock);
+			preempt_enable_notrace();
+			return -EOPNOTSUPP;
+		}
+

drivers/ptp/ptp_kvm.c line 96

Quoted 10 lines

drivers/ptp/ptp_kvm.c new file +10 -0

+		tspec.tv_sec = clock_off.sec;
+		tspec.tv_nsec = clock_off.nsec;
+		ret = __pvclock_read_cycles(src, clock_off.tsc);
+		flags = src->flags;
+	} while (pvclock_read_retry(src, version));
+
+	preempt_enable_notrace();
+
+	system_counter->cycles = ret;
+	system_counter->cs = get_kvmclock_cs();

Can't we use clocksource_tsc and just pass the tsc without kvmclock in the middle?

Quoted 1 line

No, it has to be the kvmclock value.

What happens if the guest switches from kvmclock to tsc?

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

PHC0 0 3 377 7 -141ns[ +234ns] ± 7ns

#* PHC0                          0   3   377     5   -197ns[ -403ns] ±    3ns
#* PHC0                          0   3   377    11   +157ns[ +264ns] ±    3ns
#* PHC0                          0   3   377     9    +92ns[ +304ns] ±    2ns
#* PHC0                          0   3   377     7   -102ns[ -164ns] ±    3ns
#* PHC0                          0   3   377     5   -157ns[ -269ns] ±    2ns
#* PHC0                          0   3   377    10    -63ns[  -98ns] ±    2ns
#* PHC0                          0   3   377     8   +351ns[ +399ns] ±    2ns
#* PHC0                          0   3   377     6   +385ns[ +399ns] ±    4ns
#* PHC0                          0   3   377     4   -133ns[ -110ns] ±    6ns
#* PHC0                          0   3   377    10    -59ns[  -90ns] ±    3ns
#* PHC0                          0   3   377     8    -90ns[ +381ns] ±    9ns
#* PHC0                          0   3   377     6     +6ns[ +216ns] ±    6ns
#* PHC0                          0   3   377     4   +166ns[ -666ns] ±   11ns
#* PHC0                          0   3   377    10    -18ns[ +323ns] ±   10ns
#* PHC0                          0   3   377     8    -12ns[ +121ns] ±    5ns
#* PHC0                          0   3   377     5     +4ns[ +218ns] ±    7ns
#* PHC0                          0   3   377     4   +162ns[ -683ns] ±   11ns
#* PHC0                          0   3   377    10    -82ns[ +310ns] ±   12ns
#* PHC0                          0   3   377     7     +5ns[ -320ns] ±    9ns
#* PHC0                          0   3   377     5    -13ns[ +165ns] ±    7ns
#* PHC0                          0   3   377     3     +6ns[ +105ns] ±    2ns
#* PHC0                          0   3   377     9    -19ns[  -67ns] ±    3ns
#* PHC0                          0   3   377     8    +89ns[ +181ns] ±    3ns

#* PHC0                          0   3   377     6    +93ns[ +168ns] ±    2ns
#* PHC0                          0   3   377     4   +100ns[ +154ns] ±    2ns
#* PHC0                          0   3   377    10   -249ns[ +180ns] ±    7ns
#* PHC0                          0   3   377     8   +158ns[ -582ns] ±   22ns
#* PHC0                          0   3   377     6    +10ns[ +213ns] ±    9ns
#* PHC0                          0   3   377     3    -35ns[ +258ns] ±    5ns
#* PHC0                          0   3   377    10    +25ns[ -620ns] ±    8ns
#* PHC0                          0   3   377     7    +13ns[ +153ns] ±    2ns