[RFC PATCH 0/3] arm64: enable virtual kvm ptp for arm64

[RFC PATCH 0/3] arm64: enable virtual kvm ptp for arm64

[Jianyong Wu]

kvm ptp targets to provide high precision time sync between guest
and host in virtualization environment. This patch enable kvm ptp
for arm64.

This patch set base on [1][2][3]

[1]https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/
commit/?h=kvm/hvc&id=125ea89e4a21e2fc5235410f966a996a1a7148bf
[2]https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/
commit/?h=kvm/hvc&id=464f5a1741e5959c3e4d2be1966ae0093b4dce06
[3]https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/
commit/?h=kvm/hvc&id=6597490e005d0eeca8ed8c1c1d7b4318ee014681

Jianyong Wu (3):
  Export psci_ops.conduit symbol as modules will use it.
  reorganize ptp_kvm modules to make it arch-independent.
  Enable ptp_kvm for arm64

 arch/arm64/include/asm/arch_timer.h  |  3 +
 arch/arm64/kvm/arch_ptp_kvm.c        | 76 +++++++++++++++++++++++
 arch/x86/kvm/arch_ptp_kvm.c          | 92 ++++++++++++++++++++++++++++
 drivers/clocksource/arm_arch_timer.c |  6 +-
 drivers/firmware/psci/psci.c         |  6 ++
 drivers/ptp/Kconfig                  |  2 +-
 drivers/ptp/Makefile                 |  1 +
 drivers/ptp/{ptp_kvm.c => kvm_ptp.c} | 77 ++++++-----------------
 include/asm-generic/ptp_kvm.h        | 12 ++++
 include/linux/arm-smccc.h            | 16 ++++-
 include/linux/psci.h                 |  1 +
 virt/kvm/arm/psci.c                  | 17 +++++
 12 files changed, 246 insertions(+), 63 deletions(-)
 create mode 100644 arch/arm64/kvm/arch_ptp_kvm.c
 create mode 100644 arch/x86/kvm/arch_ptp_kvm.c
 rename drivers/ptp/{ptp_kvm.c => kvm_ptp.c} (63%)
 create mode 100644 include/asm-generic/ptp_kvm.h

-- 
2.17.1

[RFC PATCH 1/3] Export psci_ops.conduit symbol as modules will use it.

[Jianyong Wu]

If arm_smccc_1_1_invoke used in modules, psci_ops.conduit should
be export.

Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
---
 drivers/firmware/psci/psci.c | 6 ++++++
 include/linux/arm-smccc.h    | 2 +-
 include/linux/psci.h         | 1 +
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index f82ccd39a913..35c4eaab1451 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -212,6 +212,12 @@ static unsigned long psci_migrate_info_up_cpu(void)
 			      0, 0, 0);
 }
 
+enum psci_conduit psci_get_conduit(void)
+{
+	return psci_ops.conduit;
+}
+EXPORT_SYMBOL(psci_get_conduit);
+
 static void set_conduit(enum psci_conduit conduit)
 {
 	switch (conduit) {
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 552cbd49abe8..a6e4d3e3d10a 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -357,7 +357,7 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
  * The return value also provides the conduit that was used.
  */
 #define arm_smccc_1_1_invoke(...) ({					\
-		int method = psci_ops.conduit;				\
+		int method = psci_get_conduit();			\
 		switch (method) {					\
 		case PSCI_CONDUIT_HVC:					\
 			arm_smccc_1_1_hvc(__VA_ARGS__);			\
diff --git a/include/linux/psci.h b/include/linux/psci.h
index a8a15613c157..e5cedc986049 100644
--- a/include/linux/psci.h
+++ b/include/linux/psci.h
@@ -42,6 +42,7 @@ struct psci_operations {
 	enum smccc_version smccc_version;
 };
 
+extern enum psci_conduit psci_get_conduit(void);
 extern struct psci_operations psci_ops;
 
 #if defined(CONFIG_ARM_PSCI_FW)
-- 
2.17.1

[RFC PATCH 2/3] reorganize ptp_kvm modules to make it arch-independent.

[Jianyong Wu]

Currently, ptp_kvm modules implementation is only for x86 which includs
large part of arch-specific code.  This patch move all of those code
into related arch directory.

Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
---
 arch/x86/kvm/arch_ptp_kvm.c          | 92 ++++++++++++++++++++++++++++
 drivers/ptp/Makefile                 |  1 +
 drivers/ptp/{ptp_kvm.c => kvm_ptp.c} | 77 ++++++-----------------
 include/asm-generic/ptp_kvm.h        | 12 ++++
 4 files changed, 123 insertions(+), 59 deletions(-)
 create mode 100644 arch/x86/kvm/arch_ptp_kvm.c
 rename drivers/ptp/{ptp_kvm.c => kvm_ptp.c} (63%)
 create mode 100644 include/asm-generic/ptp_kvm.h

diff --git a/arch/x86/kvm/arch_ptp_kvm.c b/arch/x86/kvm/arch_ptp_kvm.c
new file mode 100644
index 000000000000..56ea84a86da2
--- /dev/null
+++ b/arch/x86/kvm/arch_ptp_kvm.c
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ *  Virtual PTP 1588 clock for use with KVM guests
+ *
+ *  Copyright (C) 2019 ARM Ltd.
+ *  All Rights Reserved
+ */
+
+#include <asm/pvclock.h>
+#include <asm/kvmclock.h>
+#include <linux/module.h>
+#include <uapi/asm/kvm_para.h>
+#include <uapi/linux/kvm_para.h>
+#include <linux/ptp_clock_kernel.h>
+
+phys_addr_t clock_pair_gpa;
+struct kvm_clock_pairing clock_pair;
+struct pvclock_vsyscall_time_info *hv_clock;
+
+int kvm_arch_ptp_init(void)
+{
+	int ret;
+
+	if (!kvm_para_available())
+		return -ENODEV;
+
+	clock_pair_gpa = slow_virt_to_phys(&clock_pair);
+	hv_clock = pvclock_get_pvti_cpu0_va();
+	if (!hv_clock)
+		return -ENODEV;
+
+	ret = kvm_hypercall2(KVM_HC_CLOCK_PAIRING, clock_pair_gpa,
+			     KVM_CLOCK_PAIRING_WALLCLOCK);
+	if (ret == -KVM_ENOSYS || ret == -KVM_EOPNOTSUPP)
+		return -ENODEV;
+
+	return 0;
+}
+
+int kvm_arch_ptp_get_clock(struct timespec64 *ts)
+{
+	long ret;
+
+	ret = kvm_hypercall2(KVM_HC_CLOCK_PAIRING,
+			     clock_pair_gpa,
+			     KVM_CLOCK_PAIRING_WALLCLOCK);
+	if (ret != 0)
+		return -EOPNOTSUPP;
+
+	ts->tv_sec = clock_pair.sec;
+	ts->tv_nsec = clock_pair.nsec;
+
+	return 0;
+}
+
+int kvm_arch_ptp_get_clock_fn(long *cycle, struct timespec64 *tspec,
+			      struct clocksource **cs)
+{
+	unsigned long ret;
+	unsigned int version;
+	int cpu;
+	struct pvclock_vcpu_time_info *src;
+
+	cpu = smp_processor_id();
+	src = &hv_clock[cpu].pvti;
+
+	do {
+		/*
+		 * We are using a TSC value read in the hosts
+		 * kvm_hc_clock_pairing handling.
+		 * So any changes to tsc_to_system_mul
+		 * and tsc_shift or any other pvclock
+		 * data invalidate that measurement.
+		 */
+		version = pvclock_read_begin(src);
+
+		ret = kvm_hypercall2(KVM_HC_CLOCK_PAIRING,
+				     clock_pair_gpa,
+				     KVM_CLOCK_PAIRING_WALLCLOCK);
+		tspec->tv_sec = clock_pair.sec;
+		tspec->tv_nsec = clock_pair.nsec;
+		*cycle = __pvclock_read_cycles(src, clock_pair.tsc);
+	} while (pvclock_read_retry(src, version));
+
+	*cs = &kvm_clock;
+
+	return 0;
+}
+
+MODULE_AUTHOR("Marcelo Tosatti <mtosatti@redhat.com>");
+MODULE_DESCRIPTION("PTP clock using KVMCLOCK");
+MODULE_LICENSE("GPL");
diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile
index 677d1d178a3e..5a8c6462fc0f 100644
--- a/drivers/ptp/Makefile
+++ b/drivers/ptp/Makefile
@@ -4,6 +4,7 @@
 #
 
 ptp-y					:= ptp_clock.o ptp_chardev.o ptp_sysfs.o
+ptp_kvm-y				:= ../../arch/$(ARCH)/kvm/arch_ptp_kvm.o kvm_ptp.o
 obj-$(CONFIG_PTP_1588_CLOCK)		+= ptp.o
 obj-$(CONFIG_PTP_1588_CLOCK_DTE)	+= ptp_dte.o
 obj-$(CONFIG_PTP_1588_CLOCK_IXP46X)	+= ptp_ixp46x.o
diff --git a/drivers/ptp/ptp_kvm.c b/drivers/ptp/kvm_ptp.c
similarity index 63%
rename from drivers/ptp/ptp_kvm.c
rename to drivers/ptp/kvm_ptp.c
index fc7d0b77e118..9d07cf872be7 100644
--- a/drivers/ptp/ptp_kvm.c
+++ b/drivers/ptp/kvm_ptp.c
@@ -8,12 +8,12 @@
 #include <linux/err.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/slab.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 <asm-generic/ptp_kvm.h>
 
 #include <linux/ptp_clock_kernel.h>
 
@@ -24,56 +24,29 @@ struct kvm_ptp_clock {
 
 DEFINE_SPINLOCK(kvm_ptp_lock);
 
-static struct pvclock_vsyscall_time_info *hv_clock;
-
-static struct kvm_clock_pairing clock_pair;
-static phys_addr_t clock_pair_gpa;
-
 static int ptp_kvm_get_time_fn(ktime_t *device_time,
 			       struct system_counterval_t *system_counter,
 			       void *ctx)
 {
-	unsigned long ret;
+	unsigned long ret, cycle;
 	struct timespec64 tspec;
-	unsigned version;
-	int cpu;
-	struct pvclock_vcpu_time_info *src;
+	struct clocksource *cs;
 
 	spin_lock(&kvm_ptp_lock);
 
 	preempt_disable_notrace();
-	cpu = smp_processor_id();
-	src = &hv_clock[cpu].pvti;
-
-	do {
-		/*
-		 * We are using a TSC value read in the hosts
-		 * kvm_hc_clock_pairing handling.
-		 * So any changes to tsc_to_system_mul
-		 * and tsc_shift or any other pvclock
-		 * data invalidate that measurement.
-		 */
-		version = pvclock_read_begin(src);
-
-		ret = kvm_hypercall2(KVM_HC_CLOCK_PAIRING,
-				     clock_pair_gpa,
-				     KVM_CLOCK_PAIRING_WALLCLOCK);
-		if (ret != 0) {
-			pr_err_ratelimited("clock pairing hypercall ret %lu\n", ret);
-			spin_unlock(&kvm_ptp_lock);
-			preempt_enable_notrace();
-			return -EOPNOTSUPP;
-		}
-
-		tspec.tv_sec = clock_pair.sec;
-		tspec.tv_nsec = clock_pair.nsec;
-		ret = __pvclock_read_cycles(src, clock_pair.tsc);
-	} while (pvclock_read_retry(src, version));
+	ret = kvm_arch_ptp_get_clock_fn(&cycle, &tspec, &cs);
+	if (ret != 0) {
+		pr_err_ratelimited("clock pairing hypercall ret %lu\n", ret);
+		spin_unlock(&kvm_ptp_lock);
+		preempt_enable_notrace();
+		return -EOPNOTSUPP;
+	}
 
 	preempt_enable_notrace();
 
-	system_counter->cycles = ret;
-	system_counter->cs = &kvm_clock;
+	system_counter->cycles = cycle;
+	system_counter->cs = cs;
 
 	*device_time = timespec64_to_ktime(tspec);
 
@@ -116,17 +89,13 @@ static int ptp_kvm_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
 
 	spin_lock(&kvm_ptp_lock);
 
-	ret = kvm_hypercall2(KVM_HC_CLOCK_PAIRING,
-			     clock_pair_gpa,
-			     KVM_CLOCK_PAIRING_WALLCLOCK);
+	ret = kvm_arch_ptp_get_clock(&tspec);
 	if (ret != 0) {
 		pr_err_ratelimited("clock offset hypercall ret %lu\n", ret);
 		spin_unlock(&kvm_ptp_lock);
 		return -EOPNOTSUPP;
 	}
 
-	tspec.tv_sec = clock_pair.sec;
-	tspec.tv_nsec = clock_pair.nsec;
 	spin_unlock(&kvm_ptp_lock);
 
 	memcpy(ts, &tspec, sizeof(struct timespec64));
@@ -166,21 +135,11 @@ static void __exit ptp_kvm_exit(void)
 
 static int __init ptp_kvm_init(void)
 {
-	long ret;
-
-	if (!kvm_para_available())
-		return -ENODEV;
-
-	clock_pair_gpa = slow_virt_to_phys(&clock_pair);
-	hv_clock = pvclock_get_pvti_cpu0_va();
-
-	if (!hv_clock)
-		return -ENODEV;
+	int ret;
 
-	ret = kvm_hypercall2(KVM_HC_CLOCK_PAIRING, clock_pair_gpa,
-			KVM_CLOCK_PAIRING_WALLCLOCK);
-	if (ret == -KVM_ENOSYS || ret == -KVM_EOPNOTSUPP)
-		return -ENODEV;
+	ret = kvm_arch_ptp_init();
+	if (IS_ERR(ret))
+		return ret;
 
 	kvm_ptp_clock.caps = ptp_kvm_caps;
 
diff --git a/include/asm-generic/ptp_kvm.h b/include/asm-generic/ptp_kvm.h
new file mode 100644
index 000000000000..128a9d7af161
--- /dev/null
+++ b/include/asm-generic/ptp_kvm.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ *  Virtual PTP 1588 clock for use with KVM guests
+ *
+ *  Copyright (C) 2019 ARM Ltd.
+ *  All Rights Reserved
+ */
+
+static int kvm_arch_ptp_init(void);
+static int kvm_arch_ptp_get_clock(struct timespec64 *ts);
+static int kvm_arch_ptp_get_clock_fn(long *cycle,
+		struct timespec64 *tspec, void *cs);
-- 
2.17.1

Re: [RFC PATCH 2/3] reorganize ptp_kvm modules to make it arch-independent.

[Marc Zyngier]

On 29/08/2019 07:39, Jianyong Wu wrote:
> Currently, ptp_kvm modules implementation is only for x86 which includs
> large part of arch-specific code.  This patch move all of those code
> into related arch directory.
> 
> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> ---
>  arch/x86/kvm/arch_ptp_kvm.c          | 92 ++++++++++++++++++++++++++++
>  drivers/ptp/Makefile                 |  1 +
>  drivers/ptp/{ptp_kvm.c => kvm_ptp.c} | 77 ++++++-----------------
>  include/asm-generic/ptp_kvm.h        | 12 ++++
>  4 files changed, 123 insertions(+), 59 deletions(-)
>  create mode 100644 arch/x86/kvm/arch_ptp_kvm.c
>  rename drivers/ptp/{ptp_kvm.c => kvm_ptp.c} (63%)
>  create mode 100644 include/asm-generic/ptp_kvm.h
> 
> diff --git a/arch/x86/kvm/arch_ptp_kvm.c b/arch/x86/kvm/arch_ptp_kvm.c
> new file mode 100644
> index 000000000000..56ea84a86da2
> --- /dev/null
> +++ b/arch/x86/kvm/arch_ptp_kvm.c
> @@ -0,0 +1,92 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + *  Virtual PTP 1588 clock for use with KVM guests
> + *
> + *  Copyright (C) 2019 ARM Ltd.
> + *  All Rights Reserved

No. This isn't ARM's code, not by a million mile. You've simply
refactored existing code. Please keep the correct attribution (i.e. that
of the original code).

> + */
> +
> +#include <asm/pvclock.h>
> +#include <asm/kvmclock.h>
> +#include <linux/module.h>
> +#include <uapi/asm/kvm_para.h>
> +#include <uapi/linux/kvm_para.h>
> +#include <linux/ptp_clock_kernel.h>
> +
> +phys_addr_t clock_pair_gpa;
> +struct kvm_clock_pairing clock_pair;
> +struct pvclock_vsyscall_time_info *hv_clock;
> +
> +int kvm_arch_ptp_init(void)
> +{
> +	int ret;
> +
> +	if (!kvm_para_available())
> +		return -ENODEV;
> +
> +	clock_pair_gpa = slow_virt_to_phys(&clock_pair);
> +	hv_clock = pvclock_get_pvti_cpu0_va();
> +	if (!hv_clock)
> +		return -ENODEV;
> +
> +	ret = kvm_hypercall2(KVM_HC_CLOCK_PAIRING, clock_pair_gpa,
> +			     KVM_CLOCK_PAIRING_WALLCLOCK);
> +	if (ret == -KVM_ENOSYS || ret == -KVM_EOPNOTSUPP)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +int kvm_arch_ptp_get_clock(struct timespec64 *ts)
> +{
> +	long ret;
> +
> +	ret = kvm_hypercall2(KVM_HC_CLOCK_PAIRING,
> +			     clock_pair_gpa,
> +			     KVM_CLOCK_PAIRING_WALLCLOCK);
> +	if (ret != 0)
> +		return -EOPNOTSUPP;
> +
> +	ts->tv_sec = clock_pair.sec;
> +	ts->tv_nsec = clock_pair.nsec;
> +
> +	return 0;
> +}
> +
> +int kvm_arch_ptp_get_clock_fn(long *cycle, struct timespec64 *tspec,
> +			      struct clocksource **cs)
> +{
> +	unsigned long ret;
> +	unsigned int version;
> +	int cpu;
> +	struct pvclock_vcpu_time_info *src;
> +
> +	cpu = smp_processor_id();
> +	src = &hv_clock[cpu].pvti;
> +
> +	do {
> +		/*
> +		 * We are using a TSC value read in the hosts
> +		 * kvm_hc_clock_pairing handling.
> +		 * So any changes to tsc_to_system_mul
> +		 * and tsc_shift or any other pvclock
> +		 * data invalidate that measurement.
> +		 */
> +		version = pvclock_read_begin(src);
> +
> +		ret = kvm_hypercall2(KVM_HC_CLOCK_PAIRING,
> +				     clock_pair_gpa,
> +				     KVM_CLOCK_PAIRING_WALLCLOCK);
> +		tspec->tv_sec = clock_pair.sec;
> +		tspec->tv_nsec = clock_pair.nsec;
> +		*cycle = __pvclock_read_cycles(src, clock_pair.tsc);
> +	} while (pvclock_read_retry(src, version));
> +
> +	*cs = &kvm_clock;
> +
> +	return 0;
> +}
> +
> +MODULE_AUTHOR("Marcelo Tosatti <mtosatti@redhat.com>");
> +MODULE_DESCRIPTION("PTP clock using KVMCLOCK");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile
> index 677d1d178a3e..5a8c6462fc0f 100644
> --- a/drivers/ptp/Makefile
> +++ b/drivers/ptp/Makefile
> @@ -4,6 +4,7 @@
>  #
>  
>  ptp-y					:= ptp_clock.o ptp_chardev.o ptp_sysfs.o
> +ptp_kvm-y				:= ../../arch/$(ARCH)/kvm/arch_ptp_kvm.o kvm_ptp.o
>  obj-$(CONFIG_PTP_1588_CLOCK)		+= ptp.o
>  obj-$(CONFIG_PTP_1588_CLOCK_DTE)	+= ptp_dte.o
>  obj-$(CONFIG_PTP_1588_CLOCK_IXP46X)	+= ptp_ixp46x.o
> diff --git a/drivers/ptp/ptp_kvm.c b/drivers/ptp/kvm_ptp.c
> similarity index 63%
> rename from drivers/ptp/ptp_kvm.c
> rename to drivers/ptp/kvm_ptp.c
> index fc7d0b77e118..9d07cf872be7 100644
> --- a/drivers/ptp/ptp_kvm.c
> +++ b/drivers/ptp/kvm_ptp.c
> @@ -8,12 +8,12 @@
>  #include <linux/err.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> +#include <linux/slab.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 <asm-generic/ptp_kvm.h>
>  
>  #include <linux/ptp_clock_kernel.h>
>  
> @@ -24,56 +24,29 @@ struct kvm_ptp_clock {
>  
>  DEFINE_SPINLOCK(kvm_ptp_lock);
>  
> -static struct pvclock_vsyscall_time_info *hv_clock;
> -
> -static struct kvm_clock_pairing clock_pair;
> -static phys_addr_t clock_pair_gpa;
> -
>  static int ptp_kvm_get_time_fn(ktime_t *device_time,
>  			       struct system_counterval_t *system_counter,
>  			       void *ctx)
>  {
> -	unsigned long ret;
> +	unsigned long ret, cycle;
>  	struct timespec64 tspec;
> -	unsigned version;
> -	int cpu;
> -	struct pvclock_vcpu_time_info *src;
> +	struct clocksource *cs;
>  
>  	spin_lock(&kvm_ptp_lock);
>  
>  	preempt_disable_notrace();
> -	cpu = smp_processor_id();
> -	src = &hv_clock[cpu].pvti;
> -
> -	do {
> -		/*
> -		 * We are using a TSC value read in the hosts
> -		 * kvm_hc_clock_pairing handling.
> -		 * So any changes to tsc_to_system_mul
> -		 * and tsc_shift or any other pvclock
> -		 * data invalidate that measurement.
> -		 */
> -		version = pvclock_read_begin(src);
> -
> -		ret = kvm_hypercall2(KVM_HC_CLOCK_PAIRING,
> -				     clock_pair_gpa,
> -				     KVM_CLOCK_PAIRING_WALLCLOCK);
> -		if (ret != 0) {
> -			pr_err_ratelimited("clock pairing hypercall ret %lu\n", ret);
> -			spin_unlock(&kvm_ptp_lock);
> -			preempt_enable_notrace();
> -			return -EOPNOTSUPP;
> -		}
> -
> -		tspec.tv_sec = clock_pair.sec;
> -		tspec.tv_nsec = clock_pair.nsec;
> -		ret = __pvclock_read_cycles(src, clock_pair.tsc);
> -	} while (pvclock_read_retry(src, version));
> +	ret = kvm_arch_ptp_get_clock_fn(&cycle, &tspec, &cs);
> +	if (ret != 0) {
> +		pr_err_ratelimited("clock pairing hypercall ret %lu\n", ret);
> +		spin_unlock(&kvm_ptp_lock);
> +		preempt_enable_notrace();
> +		return -EOPNOTSUPP;
> +	}
>  
>  	preempt_enable_notrace();
>  
> -	system_counter->cycles = ret;
> -	system_counter->cs = &kvm_clock;
> +	system_counter->cycles = cycle;
> +	system_counter->cs = cs;
>  
>  	*device_time = timespec64_to_ktime(tspec);
>  
> @@ -116,17 +89,13 @@ static int ptp_kvm_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
>  
>  	spin_lock(&kvm_ptp_lock);
>  
> -	ret = kvm_hypercall2(KVM_HC_CLOCK_PAIRING,
> -			     clock_pair_gpa,
> -			     KVM_CLOCK_PAIRING_WALLCLOCK);
> +	ret = kvm_arch_ptp_get_clock(&tspec);
>  	if (ret != 0) {
>  		pr_err_ratelimited("clock offset hypercall ret %lu\n", ret);
>  		spin_unlock(&kvm_ptp_lock);
>  		return -EOPNOTSUPP;
>  	}
>  
> -	tspec.tv_sec = clock_pair.sec;
> -	tspec.tv_nsec = clock_pair.nsec;
>  	spin_unlock(&kvm_ptp_lock);
>  
>  	memcpy(ts, &tspec, sizeof(struct timespec64));
> @@ -166,21 +135,11 @@ static void __exit ptp_kvm_exit(void)
>  
>  static int __init ptp_kvm_init(void)
>  {
> -	long ret;
> -
> -	if (!kvm_para_available())
> -		return -ENODEV;
> -
> -	clock_pair_gpa = slow_virt_to_phys(&clock_pair);
> -	hv_clock = pvclock_get_pvti_cpu0_va();
> -
> -	if (!hv_clock)
> -		return -ENODEV;
> +	int ret;
>  
> -	ret = kvm_hypercall2(KVM_HC_CLOCK_PAIRING, clock_pair_gpa,
> -			KVM_CLOCK_PAIRING_WALLCLOCK);
> -	if (ret == -KVM_ENOSYS || ret == -KVM_EOPNOTSUPP)
> -		return -ENODEV;
> +	ret = kvm_arch_ptp_init();
> +	if (IS_ERR(ret))
> +		return ret;
>  
>  	kvm_ptp_clock.caps = ptp_kvm_caps;
>  
> diff --git a/include/asm-generic/ptp_kvm.h b/include/asm-generic/ptp_kvm.h
> new file mode 100644
> index 000000000000..128a9d7af161
> --- /dev/null
> +++ b/include/asm-generic/ptp_kvm.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + *  Virtual PTP 1588 clock for use with KVM guests
> + *
> + *  Copyright (C) 2019 ARM Ltd.
> + *  All Rights Reserved

Same here.

> + */
> +
> +static int kvm_arch_ptp_init(void);
> +static int kvm_arch_ptp_get_clock(struct timespec64 *ts);
> +static int kvm_arch_ptp_get_clock_fn(long *cycle,
> +		struct timespec64 *tspec, void *cs);
> 

	M.
-- 
Jazz is not dead, it just smells funny...

RE: [RFC PATCH 2/3] reorganize ptp_kvm modules to make it arch-independent.

[Jianyong Wu (Arm Technology China)]

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Thursday, August 29, 2019 5:09 PM
> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>;
> netdev@vger.kernel.org; pbonzini@redhat.com;
> sean.j.christopherson@intel.com; richardcochran@gmail.com; Mark Rutland
> <Mark.Rutland@arm.com>; Will Deacon <Will.Deacon@arm.com>; Suzuki
> Poulose <Suzuki.Poulose@arm.com>
> Cc: linux-kernel@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>;
> Kaly Xin (Arm Technology China) <Kaly.Xin@arm.com>; Justin He (Arm
> Technology China) <Justin.He@arm.com>
> Subject: Re: [RFC PATCH 2/3] reorganize ptp_kvm modules to make it arch-
> independent.
>
.........

> > diff --git a/arch/x86/kvm/arch_ptp_kvm.c
> b/arch/x86/kvm/arch_ptp_kvm.c
> > new file mode 100644 index 000000000000..56ea84a86da2
> > --- /dev/null
> > +++ b/arch/x86/kvm/arch_ptp_kvm.c
> > @@ -0,0 +1,92 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + *  Virtual PTP 1588 clock for use with KVM guests
> > + *
> > + *  Copyright (C) 2019 ARM Ltd.
> > + *  All Rights Reserved
>
> No. This isn't ARM's code, not by a million mile. You've simply refactored
> existing code. Please keep the correct attribution (i.e. that of the original
> code).
>
Ok, I will fix it.

.......
> >
> > diff --git a/include/asm-generic/ptp_kvm.h
> > b/include/asm-generic/ptp_kvm.h new file mode 100644 index
> > 000000000000..128a9d7af161
> > --- /dev/null
> > +++ b/include/asm-generic/ptp_kvm.h
> > @@ -0,0 +1,12 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + *  Virtual PTP 1588 clock for use with KVM guests
> > + *
> > + *  Copyright (C) 2019 ARM Ltd.
> > + *  All Rights Reserved
>
> Same here.
>
Ok.

Thanks
Jianyong Wu

> > + */
> > +
> > +static int kvm_arch_ptp_init(void);
> > +static int kvm_arch_ptp_get_clock(struct timespec64 *ts); static int
> > +kvm_arch_ptp_get_clock_fn(long *cycle,
> > +           struct timespec64 *tspec, void *cs);
> >
>
>       M.
> --
> Jazz is not dead, it just smells funny...
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[RFC PATCH 3/3] Enable ptp_kvm for arm64

[Jianyong Wu]

Currently in arm64 virtualization environment, there is no mechanism to
keep time sync between guest and host. Time in guest will drift compared
with host after boot up as they may both use third party time sources
to correct their time respectively. The time deviation will be in order
of milliseconds but some scenarios ask for higher time precision, like
in cloud envirenment, we want all the VMs running in the host aquire the
same level accuracy from host clock.

Use of kvm ptp clock, which choose the host clock source clock as a
reference clock to sync time clock between guest and host has been adopted
by x86 which makes the time sync order from milliseconds to nanoseconds.

This patch enable kvm ptp on arm64 and we get the similar clock drift as
found with x86 with kvm ptp.

Test result comparison between with kvm ptp and without it in arm64 are
as follows. This test derived from the result of command 'chronyc
sources'. we should take more cure of the last sample column which shows
the offset between the local clock and the source at the last measurement.

no kvm ptp in guest:
MS Name/IP address   Stratum Poll Reach LastRx Last sample
========================================================================
^* dns1.synet.edu.cn      2   6   377    13  +1040us[+1581us] +/-   21ms
^* dns1.synet.edu.cn      2   6   377    21  +1040us[+1581us] +/-   21ms
^* dns1.synet.edu.cn      2   6   377    29  +1040us[+1581us] +/-   21ms
^* dns1.synet.edu.cn      2   6   377    37  +1040us[+1581us] +/-   21ms
^* dns1.synet.edu.cn      2   6   377    45  +1040us[+1581us] +/-   21ms
^* dns1.synet.edu.cn      2   6   377    53  +1040us[+1581us] +/-   21ms
^* dns1.synet.edu.cn      2   6   377    61  +1040us[+1581us] +/-   21ms
^* dns1.synet.edu.cn      2   6   377     4   -130us[ +796us] +/-   21ms
^* dns1.synet.edu.cn      2   6   377    12   -130us[ +796us] +/-   21ms
^* dns1.synet.edu.cn      2   6   377    20   -130us[ +796us] +/-   21ms

in host:
MS Name/IP address   Stratum Poll Reach LastRx Last sample
========================================================================
^* 120.25.115.20          2   7   377    72   -470us[ -603us] +/-   18ms
^* 120.25.115.20          2   7   377    92   -470us[ -603us] +/-   18ms
^* 120.25.115.20          2   7   377   112   -470us[ -603us] +/-   18ms
^* 120.25.115.20          2   7   377     2   +872ns[-6808ns] +/-   17ms
^* 120.25.115.20          2   7   377    22   +872ns[-6808ns] +/-   17ms
^* 120.25.115.20          2   7   377    43   +872ns[-6808ns] +/-   17ms
^* 120.25.115.20          2   7   377    63   +872ns[-6808ns] +/-   17ms
^* 120.25.115.20          2   7   377    83   +872ns[-6808ns] +/-   17ms
^* 120.25.115.20          2   7   377   103   +872ns[-6808ns] +/-   17ms
^* 120.25.115.20          2   7   377   123   +872ns[-6808ns] +/-   17ms

The dns1.synet.edu.cn is the network reference clock for guest and
120.25.115.20 is the network reference clock for host. we can't get the
clock error between guest and host directly, but a roughly estimated value
will be in order of hundreds of us to ms.

with kvm ptp in guest:
chrony has been disabled in host to remove the disturb by network clock.

MS Name/IP address         Stratum Poll Reach LastRx Last sample
========================================================================
* PHC0                    0   3   377     8     -7ns[   +1ns] +/-    3ns
* PHC0                    0   3   377     8     +1ns[  +16ns] +/-    3ns
* PHC0                    0   3   377     6     -4ns[   -0ns] +/-    6ns
* PHC0                    0   3   377     6     -8ns[  -12ns] +/-    5ns
* PHC0                    0   3   377     5     +2ns[   +4ns] +/-    4ns
* PHC0                    0   3   377    13     +2ns[   +4ns] +/-    4ns
* PHC0                    0   3   377    12     -4ns[   -6ns] +/-    4ns
* PHC0                    0   3   377    11     -8ns[  -11ns] +/-    6ns
* PHC0                    0   3   377    10    -14ns[  -20ns] +/-    4ns
* PHC0                    0   3   377     8     +4ns[   +5ns] +/-    4ns

The PHC0 is the ptp clock which choose the host clock as its source
clock. So we can be sure to say that the clock error between host and guest
is in order of ns.

Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
---
 arch/arm64/include/asm/arch_timer.h  |  3 ++
 arch/arm64/kvm/arch_ptp_kvm.c        | 76 ++++++++++++++++++++++++++++
 drivers/clocksource/arm_arch_timer.c |  6 ++-
 drivers/ptp/Kconfig                  |  2 +-
 include/linux/arm-smccc.h            | 14 +++++
 virt/kvm/arm/psci.c                  | 17 +++++++
 6 files changed, 115 insertions(+), 3 deletions(-)
 create mode 100644 arch/arm64/kvm/arch_ptp_kvm.c

diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 6756178c27db..880576a814b6 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -229,4 +229,7 @@ static inline int arch_timer_arch_init(void)
 	return 0;
 }
 
+extern struct clocksource clocksource_counter;
+extern u64 arch_counter_read(struct clocksource *cs);
+
 #endif
diff --git a/arch/arm64/kvm/arch_ptp_kvm.c b/arch/arm64/kvm/arch_ptp_kvm.c
new file mode 100644
index 000000000000..6b2165ebce62
--- /dev/null
+++ b/arch/arm64/kvm/arch_ptp_kvm.c
@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ *  Virtual PTP 1588 clock for use with KVM guests
+ *  Copyright (C) 2019 ARM Ltd.
+ *  All Rights Reserved
+ */
+
+#include <asm/hypervisor.h>
+#include <linux/module.h>
+#include <linux/psci.h>
+#include <linux/arm-smccc.h>
+#include <linux/timecounter.h>
+#include <linux/sched/clock.h>
+#include <asm/arch_timer.h>
+
+/*
+ * as trap call cause delay, this function will return the delay in nanosecond
+ */
+static u64 arm_smccc_1_1_invoke_delay(u32 id, struct arm_smccc_res *res)
+{
+	u64 ns, t1, t2;
+
+	t1 = sched_clock();
+	arm_smccc_1_1_invoke(id, res);
+	t2 = sched_clock();
+	t2 -= t1;
+	ns = t2;
+	return ns;
+}
+
+int kvm_arch_ptp_init(void)
+{
+	return 0;
+}
+
+int kvm_arch_ptp_get_clock(struct timespec64 *ts)
+{
+	u64 ns;
+	struct arm_smccc_res hvc_res;
+
+	if (!kvm_arm_hyp_service_available(
+			ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID)) {
+		return -EOPNOTSUPP;
+	}
+	ns = arm_smccc_1_1_invoke_delay(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
+					&hvc_res);
+	ts->tv_sec = hvc_res.a0;
+	ts->tv_nsec = hvc_res.a1;
+	timespec64_add_ns(ts, ns);
+	return 0;
+}
+
+int kvm_arch_ptp_get_clock_fn(long *cycle, struct timespec64 *ts,
+			      struct clocksource **cs)
+{
+	u64 ns;
+	struct arm_smccc_res hvc_res;
+
+	if (!kvm_arm_hyp_service_available(
+			ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID)) {
+		return -EOPNOTSUPP;
+	}
+	ns = arm_smccc_1_1_invoke_delay(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
+					&hvc_res);
+	ts->tv_sec = hvc_res.a0;
+	ts->tv_nsec = hvc_res.a1;
+	timespec64_add_ns(ts, ns);
+	*cycle = hvc_res.a2;
+	*cs = &clocksource_counter;
+
+	return 0;
+}
+
+MODULE_AUTHOR("Marcelo Tosatti <mtosatti@redhat.com>");
+MODULE_DESCRIPTION("PTP clock using KVMCLOCK");
+MODULE_LICENSE("GPL");
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 07e57a49d1e8..021e3f69364c 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -175,23 +175,25 @@ static notrace u64 arch_counter_get_cntvct(void)
 u64 (*arch_timer_read_counter)(void) = arch_counter_get_cntvct;
 EXPORT_SYMBOL_GPL(arch_timer_read_counter);
 
-static u64 arch_counter_read(struct clocksource *cs)
+u64 arch_counter_read(struct clocksource *cs)
 {
 	return arch_timer_read_counter();
 }
+EXPORT_SYMBOL(arch_counter_read);
 
 static u64 arch_counter_read_cc(const struct cyclecounter *cc)
 {
 	return arch_timer_read_counter();
 }
 
-static struct clocksource clocksource_counter = {
+struct clocksource clocksource_counter = {
 	.name	= "arch_sys_counter",
 	.rating	= 400,
 	.read	= arch_counter_read,
 	.mask	= CLOCKSOURCE_MASK(56),
 	.flags	= CLOCK_SOURCE_IS_CONTINUOUS,
 };
+EXPORT_SYMBOL(clocksource_counter);
 
 static struct cyclecounter cyclecounter __ro_after_init = {
 	.read	= arch_counter_read_cc,
diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index 9b8fee5178e8..e032fafdafa7 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -110,7 +110,7 @@ config PTP_1588_CLOCK_PCH
 config PTP_1588_CLOCK_KVM
 	tristate "KVM virtual PTP clock"
 	depends on PTP_1588_CLOCK
-	depends on KVM_GUEST && X86
+	depends on KVM_GUEST && X86 || ARM64
 	default y
 	help
 	  This driver adds support for using kvm infrastructure as a PTP
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index a6e4d3e3d10a..2a222a1a8594 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -94,6 +94,7 @@
 
 /* KVM "vendor specific" services */
 #define ARM_SMCCC_KVM_FUNC_FEATURES		0
+#define ARM_SMCCC_KVM_PTP			1
 #define ARM_SMCCC_KVM_FUNC_FEATURES_2		127
 #define ARM_SMCCC_KVM_NUM_FUNCS			128
 
@@ -102,6 +103,16 @@
 			   ARM_SMCCC_SMC_32,				\
 			   ARM_SMCCC_OWNER_VENDOR_HYP,			\
 			   ARM_SMCCC_KVM_FUNC_FEATURES)
+/*
+ * This ID used for virtual ptp kvm clock and it will pass second value
+ * and nanosecond value of host real time and system counter by vcpu
+ * register to guest.
+ */
+#define ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID				\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
+			   ARM_SMCCC_SMC_32,				\
+			   ARM_SMCCC_OWNER_VENDOR_HYP,			\
+			   ARM_SMCCC_KVM_PTP)
 
 #ifndef __ASSEMBLY__
 
@@ -373,5 +384,8 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
 		method;							\
 	})
 
+#include <linux/psci.h>
+#include <linux/clocksource.h>
+
 #endif /*__ASSEMBLY__*/
 #endif /*__LINUX_ARM_SMCCC_H*/
diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
index 0debf49bf259..7fffdb25d32c 100644
--- a/virt/kvm/arm/psci.c
+++ b/virt/kvm/arm/psci.c
@@ -392,6 +392,8 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 	u32 func_id = smccc_get_function(vcpu);
 	u32 val[4] = {};
 	u32 option;
+	struct timespec *ts;
+	u64 cnt;
 
 	val[0] = SMCCC_RET_NOT_SUPPORTED;
 
@@ -431,6 +433,21 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 	case ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID:
 		val[0] = BIT(ARM_SMCCC_KVM_FUNC_FEATURES);
 		break;
+	/*
+	 * This will used for virtual ptp kvm clock. three
+	 * values will be passed back.
+	 * reg0 stores seconds of host real time;
+	 * reg1 stores nanoseconds of host real time;
+	 * reg2 stotes system counter cycle value.
+	 */
+	case ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID:
+		getnstimeofday(ts);
+		cnt = arch_timer_read_counter();
+		val[0] = ts->tv_sec;
+		val[1] = ts->tv_nsec;
+		val[2] = cnt;
+		val[3] = 0;
+		break;
 	default:
 		return kvm_psci_call(vcpu);
 	}
-- 
2.17.1

Re: [RFC PATCH 3/3] Enable ptp_kvm for arm64

[Marc Zyngier]

On 29/08/2019 07:39, Jianyong Wu wrote:
> Currently in arm64 virtualization environment, there is no mechanism to
> keep time sync between guest and host. Time in guest will drift compared
> with host after boot up as they may both use third party time sources
> to correct their time respectively. The time deviation will be in order
> of milliseconds but some scenarios ask for higher time precision, like
> in cloud envirenment, we want all the VMs running in the host aquire the
> same level accuracy from host clock.
> 
> Use of kvm ptp clock, which choose the host clock source clock as a
> reference clock to sync time clock between guest and host has been adopted
> by x86 which makes the time sync order from milliseconds to nanoseconds.
> 
> This patch enable kvm ptp on arm64 and we get the similar clock drift as
> found with x86 with kvm ptp.
> 
> Test result comparison between with kvm ptp and without it in arm64 are
> as follows. This test derived from the result of command 'chronyc
> sources'. we should take more cure of the last sample column which shows
> the offset between the local clock and the source at the last measurement.
> 
> no kvm ptp in guest:
> MS Name/IP address   Stratum Poll Reach LastRx Last sample
> ========================================================================
> ^* dns1.synet.edu.cn      2   6   377    13  +1040us[+1581us] +/-   21ms
> ^* dns1.synet.edu.cn      2   6   377    21  +1040us[+1581us] +/-   21ms
> ^* dns1.synet.edu.cn      2   6   377    29  +1040us[+1581us] +/-   21ms
> ^* dns1.synet.edu.cn      2   6   377    37  +1040us[+1581us] +/-   21ms
> ^* dns1.synet.edu.cn      2   6   377    45  +1040us[+1581us] +/-   21ms
> ^* dns1.synet.edu.cn      2   6   377    53  +1040us[+1581us] +/-   21ms
> ^* dns1.synet.edu.cn      2   6   377    61  +1040us[+1581us] +/-   21ms
> ^* dns1.synet.edu.cn      2   6   377     4   -130us[ +796us] +/-   21ms
> ^* dns1.synet.edu.cn      2   6   377    12   -130us[ +796us] +/-   21ms
> ^* dns1.synet.edu.cn      2   6   377    20   -130us[ +796us] +/-   21ms
> 
> in host:
> MS Name/IP address   Stratum Poll Reach LastRx Last sample
> ========================================================================
> ^* 120.25.115.20          2   7   377    72   -470us[ -603us] +/-   18ms
> ^* 120.25.115.20          2   7   377    92   -470us[ -603us] +/-   18ms
> ^* 120.25.115.20          2   7   377   112   -470us[ -603us] +/-   18ms
> ^* 120.25.115.20          2   7   377     2   +872ns[-6808ns] +/-   17ms
> ^* 120.25.115.20          2   7   377    22   +872ns[-6808ns] +/-   17ms
> ^* 120.25.115.20          2   7   377    43   +872ns[-6808ns] +/-   17ms
> ^* 120.25.115.20          2   7   377    63   +872ns[-6808ns] +/-   17ms
> ^* 120.25.115.20          2   7   377    83   +872ns[-6808ns] +/-   17ms
> ^* 120.25.115.20          2   7   377   103   +872ns[-6808ns] +/-   17ms
> ^* 120.25.115.20          2   7   377   123   +872ns[-6808ns] +/-   17ms
> 
> The dns1.synet.edu.cn is the network reference clock for guest and
> 120.25.115.20 is the network reference clock for host. we can't get the
> clock error between guest and host directly, but a roughly estimated value
> will be in order of hundreds of us to ms.
> 
> with kvm ptp in guest:
> chrony has been disabled in host to remove the disturb by network clock.

Is that a realistic use case? Why should the host not use NTP?

> 
> MS Name/IP address         Stratum Poll Reach LastRx Last sample
> ========================================================================
> * PHC0                    0   3   377     8     -7ns[   +1ns] +/-    3ns
> * PHC0                    0   3   377     8     +1ns[  +16ns] +/-    3ns
> * PHC0                    0   3   377     6     -4ns[   -0ns] +/-    6ns
> * PHC0                    0   3   377     6     -8ns[  -12ns] +/-    5ns
> * PHC0                    0   3   377     5     +2ns[   +4ns] +/-    4ns
> * PHC0                    0   3   377    13     +2ns[   +4ns] +/-    4ns
> * PHC0                    0   3   377    12     -4ns[   -6ns] +/-    4ns
> * PHC0                    0   3   377    11     -8ns[  -11ns] +/-    6ns
> * PHC0                    0   3   377    10    -14ns[  -20ns] +/-    4ns
> * PHC0                    0   3   377     8     +4ns[   +5ns] +/-    4ns
> 
> The PHC0 is the ptp clock which choose the host clock as its source
> clock. So we can be sure to say that the clock error between host and guest
> is in order of ns.
> 
> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> ---
>  arch/arm64/include/asm/arch_timer.h  |  3 ++
>  arch/arm64/kvm/arch_ptp_kvm.c        | 76 ++++++++++++++++++++++++++++
>  drivers/clocksource/arm_arch_timer.c |  6 ++-
>  drivers/ptp/Kconfig                  |  2 +-
>  include/linux/arm-smccc.h            | 14 +++++
>  virt/kvm/arm/psci.c                  | 17 +++++++
>  6 files changed, 115 insertions(+), 3 deletions(-)
>  create mode 100644 arch/arm64/kvm/arch_ptp_kvm.c

Please split this patch into two parts: the hypervisor code in a patch
and the guest code in another patch. Having both of them together is
confusing.

> 
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 6756178c27db..880576a814b6 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -229,4 +229,7 @@ static inline int arch_timer_arch_init(void)
>  	return 0;
>  }
>  
> +extern struct clocksource clocksource_counter;
> +extern u64 arch_counter_read(struct clocksource *cs);

I'm definitely not keen on exposing the internals of the arch_timer
driver to random subsystems. Furthermore, you seem to expect that the
guest kernel will only use the arch timer as a clocksource, and nothing
really guarantees that (in which case get_device_system_crosststamp will
fail).

It looks to me that we'd be better off exposing a core timekeeping API
that populates a struct system_counterval_t based on the *current*
timekeeper monotonic clocksource. This would simplify the split between
generic and arch-specific code.

Whether or not tglx will be happy with the idea is another problem, but
I'm certainly not taking any change to the arch timer code based on this.

> +
>  #endif
> diff --git a/arch/arm64/kvm/arch_ptp_kvm.c b/arch/arm64/kvm/arch_ptp_kvm.c

We don't put non-hypervisor in arch/arm64/kvm. Please move it back to
drivers/ptp (as well as its x86 counterpart), and just link the two
parts there. This should also allow this to be enabled for 32bit guests.

> new file mode 100644
> index 000000000000..6b2165ebce62
> --- /dev/null
> +++ b/arch/arm64/kvm/arch_ptp_kvm.c
> @@ -0,0 +1,76 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + *  Virtual PTP 1588 clock for use with KVM guests
> + *  Copyright (C) 2019 ARM Ltd.
> + *  All Rights Reserved
> + */
> +
> +#include <asm/hypervisor.h>
> +#include <linux/module.h>
> +#include <linux/psci.h>
> +#include <linux/arm-smccc.h>
> +#include <linux/timecounter.h>
> +#include <linux/sched/clock.h>
> +#include <asm/arch_timer.h>
> +
> +/*
> + * as trap call cause delay, this function will return the delay in nanosecond
> + */
> +static u64 arm_smccc_1_1_invoke_delay(u32 id, struct arm_smccc_res *res)
> +{
> +	u64 ns, t1, t2;
> +
> +	t1 = sched_clock();
> +	arm_smccc_1_1_invoke(id, res);
> +	t2 = sched_clock();
> +	t2 -= t1;
> +	ns = t2;
> +	return ns;

I think you can get rid of the ns variable here...

> +}
> +
> +int kvm_arch_ptp_init(void)
> +{
> +	return 0;
> +}
> +
> +int kvm_arch_ptp_get_clock(struct timespec64 *ts)
> +{
> +	u64 ns;
> +	struct arm_smccc_res hvc_res;
> +
> +	if (!kvm_arm_hyp_service_available(
> +			ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID)) {
> +		return -EOPNOTSUPP;
> +	}
> +	ns = arm_smccc_1_1_invoke_delay(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
> +					&hvc_res);
> +	ts->tv_sec = hvc_res.a0;
> +	ts->tv_nsec = hvc_res.a1;
> +	timespec64_add_ns(ts, ns);
> +	return 0;
> +}
> +
> +int kvm_arch_ptp_get_clock_fn(long *cycle, struct timespec64 *ts,
> +			      struct clocksource **cs)
> +{
> +	u64 ns;
> +	struct arm_smccc_res hvc_res;
> +
> +	if (!kvm_arm_hyp_service_available(
> +			ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID)) {
> +		return -EOPNOTSUPP;
> +	}
> +	ns = arm_smccc_1_1_invoke_delay(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
> +					&hvc_res);
> +	ts->tv_sec = hvc_res.a0;
> +	ts->tv_nsec = hvc_res.a1;
> +	timespec64_add_ns(ts, ns);
> +	*cycle = hvc_res.a2;
> +	*cs = &clocksource_counter;
> +
> +	return 0;
> +}

Why do we have two functions doing almost the same thing? Why do you
call kvm_arm_hyp_service_available on each and every time? Isn't it
enough to check in kvm_arch_ptp_init()?

> +
> +MODULE_AUTHOR("Marcelo Tosatti <mtosatti@redhat.com>");
> +MODULE_DESCRIPTION("PTP clock using KVMCLOCK");
> +MODULE_LICENSE("GPL");

This should only exist in the generic code.

> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 07e57a49d1e8..021e3f69364c 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -175,23 +175,25 @@ static notrace u64 arch_counter_get_cntvct(void)
>  u64 (*arch_timer_read_counter)(void) = arch_counter_get_cntvct;
>  EXPORT_SYMBOL_GPL(arch_timer_read_counter);
>  
> -static u64 arch_counter_read(struct clocksource *cs)
> +u64 arch_counter_read(struct clocksource *cs)
>  {
>  	return arch_timer_read_counter();
>  }
> +EXPORT_SYMBOL(arch_counter_read);
>  
>  static u64 arch_counter_read_cc(const struct cyclecounter *cc)
>  {
>  	return arch_timer_read_counter();
>  }
>  
> -static struct clocksource clocksource_counter = {
> +struct clocksource clocksource_counter = {
>  	.name	= "arch_sys_counter",
>  	.rating	= 400,
>  	.read	= arch_counter_read,
>  	.mask	= CLOCKSOURCE_MASK(56),
>  	.flags	= CLOCK_SOURCE_IS_CONTINUOUS,
>  };
> +EXPORT_SYMBOL(clocksource_counter);

I've said what I thought about this. Not happening.

>  
>  static struct cyclecounter cyclecounter __ro_after_init = {
>  	.read	= arch_counter_read_cc,
> diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
> index 9b8fee5178e8..e032fafdafa7 100644
> --- a/drivers/ptp/Kconfig
> +++ b/drivers/ptp/Kconfig
> @@ -110,7 +110,7 @@ config PTP_1588_CLOCK_PCH
>  config PTP_1588_CLOCK_KVM
>  	tristate "KVM virtual PTP clock"
>  	depends on PTP_1588_CLOCK
> -	depends on KVM_GUEST && X86
> +	depends on KVM_GUEST && X86 || ARM64
>  	default y
>  	help
>  	  This driver adds support for using kvm infrastructure as a PTP
> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> index a6e4d3e3d10a..2a222a1a8594 100644
> --- a/include/linux/arm-smccc.h
> +++ b/include/linux/arm-smccc.h
> @@ -94,6 +94,7 @@
>  
>  /* KVM "vendor specific" services */
>  #define ARM_SMCCC_KVM_FUNC_FEATURES		0
> +#define ARM_SMCCC_KVM_PTP			1
>  #define ARM_SMCCC_KVM_FUNC_FEATURES_2		127
>  #define ARM_SMCCC_KVM_NUM_FUNCS			128
>  
> @@ -102,6 +103,16 @@
>  			   ARM_SMCCC_SMC_32,				\
>  			   ARM_SMCCC_OWNER_VENDOR_HYP,			\
>  			   ARM_SMCCC_KVM_FUNC_FEATURES)
> +/*
> + * This ID used for virtual ptp kvm clock and it will pass second value
> + * and nanosecond value of host real time and system counter by vcpu
> + * register to guest.
> + */
> +#define ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID				\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
> +			   ARM_SMCCC_SMC_32,				\
> +			   ARM_SMCCC_OWNER_VENDOR_HYP,			\
> +			   ARM_SMCCC_KVM_PTP)
>  
>  #ifndef __ASSEMBLY__
>  
> @@ -373,5 +384,8 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
>  		method;							\
>  	})
>  
> +#include <linux/psci.h>
> +#include <linux/clocksource.h>
> +
>  #endif /*__ASSEMBLY__*/
>  #endif /*__LINUX_ARM_SMCCC_H*/
> diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> index 0debf49bf259..7fffdb25d32c 100644
> --- a/virt/kvm/arm/psci.c
> +++ b/virt/kvm/arm/psci.c
> @@ -392,6 +392,8 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
>  	u32 func_id = smccc_get_function(vcpu);
>  	u32 val[4] = {};
>  	u32 option;
> +	struct timespec *ts;
> +	u64 cnt;
>  
>  	val[0] = SMCCC_RET_NOT_SUPPORTED;
>  
> @@ -431,6 +433,21 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
>  	case ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID:
>  		val[0] = BIT(ARM_SMCCC_KVM_FUNC_FEATURES);
>  		break;
> +	/*
> +	 * This will used for virtual ptp kvm clock. three
> +	 * values will be passed back.
> +	 * reg0 stores seconds of host real time;
> +	 * reg1 stores nanoseconds of host real time;
> +	 * reg2 stotes system counter cycle value.

stores

> +	 */
> +	case ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID:
> +		getnstimeofday(ts);
> +		cnt = arch_timer_read_counter();
> +		val[0] = ts->tv_sec;
> +		val[1] = ts->tv_nsec;
> +		val[2] = cnt;

Can you explain what the purpose of exposing this counter is? The guest
should have access to the physical counter already.

> +		val[3] = 0;
> +		break;

This will probably conflict with Steven's stolen time series. Not a big
deal though.

>  	default:
>  		return kvm_psci_call(vcpu);
>  	}
> 

Other questions: how does this works with VM migration? Specially when
moving from a hypervisor that supports the feature to one that doesn't?

Thanks,

	M.
-- 
Jazz is not dead, it just smells funny...

RE: [RFC PATCH 3/3] Enable ptp_kvm for arm64

[Jianyong Wu (Arm Technology China)]

Hi Marc,

Very sorry to have missed this comments.

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Thursday, August 29, 2019 6:33 PM
> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>;
> netdev@vger.kernel.org; pbonzini@redhat.com;
> sean.j.christopherson@intel.com; richardcochran@gmail.com; Mark Rutland
> <Mark.Rutland@arm.com>; Will Deacon <Will.Deacon@arm.com>; Suzuki
> Poulose <Suzuki.Poulose@arm.com>
> Cc: linux-kernel@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>;
> Kaly Xin (Arm Technology China) <Kaly.Xin@arm.com>; Justin He (Arm
> Technology China) <Justin.He@arm.com>
> Subject: Re: [RFC PATCH 3/3] Enable ptp_kvm for arm64
>
> On 29/08/2019 07:39, Jianyong Wu wrote:
> > Currently in arm64 virtualization environment, there is no mechanism
> > to keep time sync between guest and host. Time in guest will drift
> > compared with host after boot up as they may both use third party time
> > sources to correct their time respectively. The time deviation will be
> > in order of milliseconds but some scenarios ask for higher time
> > precision, like in cloud envirenment, we want all the VMs running in
> > the host aquire the same level accuracy from host clock.
> >
> > Use of kvm ptp clock, which choose the host clock source clock as a
> > reference clock to sync time clock between guest and host has been
> > adopted by x86 which makes the time sync order from milliseconds to
> nanoseconds.
> >
> > This patch enable kvm ptp on arm64 and we get the similar clock drift
> > as found with x86 with kvm ptp.
> >
> > Test result comparison between with kvm ptp and without it in arm64
> > are as follows. This test derived from the result of command 'chronyc
> > sources'. we should take more cure of the last sample column which
> > shows the offset between the local clock and the source at the last
> measurement.
> >
> > no kvm ptp in guest:
> > MS Name/IP address   Stratum Poll Reach LastRx Last sample
> >
> ==========================================================
> ==============
> > ^* dns1.synet.edu.cn      2   6   377    13  +1040us[+1581us] +/-   21ms
> > ^* dns1.synet.edu.cn      2   6   377    21  +1040us[+1581us] +/-   21ms
> > ^* dns1.synet.edu.cn      2   6   377    29  +1040us[+1581us] +/-   21ms
> > ^* dns1.synet.edu.cn      2   6   377    37  +1040us[+1581us] +/-   21ms
> > ^* dns1.synet.edu.cn      2   6   377    45  +1040us[+1581us] +/-   21ms
> > ^* dns1.synet.edu.cn      2   6   377    53  +1040us[+1581us] +/-   21ms
> > ^* dns1.synet.edu.cn      2   6   377    61  +1040us[+1581us] +/-   21ms
> > ^* dns1.synet.edu.cn      2   6   377     4   -130us[ +796us] +/-   21ms
> > ^* dns1.synet.edu.cn      2   6   377    12   -130us[ +796us] +/-   21ms
> > ^* dns1.synet.edu.cn      2   6   377    20   -130us[ +796us] +/-   21ms
> >
> > in host:
> > MS Name/IP address   Stratum Poll Reach LastRx Last sample
> >
> ==========================================================
> ==============
> > ^* 120.25.115.20          2   7   377    72   -470us[ -603us] +/-   18ms
> > ^* 120.25.115.20          2   7   377    92   -470us[ -603us] +/-   18ms
> > ^* 120.25.115.20          2   7   377   112   -470us[ -603us] +/-   18ms
> > ^* 120.25.115.20          2   7   377     2   +872ns[-6808ns] +/-   17ms
> > ^* 120.25.115.20          2   7   377    22   +872ns[-6808ns] +/-   17ms
> > ^* 120.25.115.20          2   7   377    43   +872ns[-6808ns] +/-   17ms
> > ^* 120.25.115.20          2   7   377    63   +872ns[-6808ns] +/-   17ms
> > ^* 120.25.115.20          2   7   377    83   +872ns[-6808ns] +/-   17ms
> > ^* 120.25.115.20          2   7   377   103   +872ns[-6808ns] +/-   17ms
> > ^* 120.25.115.20          2   7   377   123   +872ns[-6808ns] +/-   17ms
> >
> > The dns1.synet.edu.cn is the network reference clock for guest and
> > 120.25.115.20 is the network reference clock for host. we can't get
> > the clock error between guest and host directly, but a roughly
> > estimated value will be in order of hundreds of us to ms.
> >
> > with kvm ptp in guest:
> > chrony has been disabled in host to remove the disturb by network clock.
>
> Is that a realistic use case? Why should the host not use NTP?
>

Not really, NTP will change the the host clock which will contaminate the data of sync between
Host and guest. But in reality, we will keep NTP online.

> >
> > MS Name/IP address         Stratum Poll Reach LastRx Last sample
> >
> ==========================================================
> ==============
> > * PHC0                    0   3   377     8     -7ns[   +1ns] +/-    3ns
> > * PHC0                    0   3   377     8     +1ns[  +16ns] +/-    3ns
> > * PHC0                    0   3   377     6     -4ns[   -0ns] +/-    6ns
> > * PHC0                    0   3   377     6     -8ns[  -12ns] +/-    5ns
> > * PHC0                    0   3   377     5     +2ns[   +4ns] +/-    4ns
> > * PHC0                    0   3   377    13     +2ns[   +4ns] +/-    4ns
> > * PHC0                    0   3   377    12     -4ns[   -6ns] +/-    4ns
> > * PHC0                    0   3   377    11     -8ns[  -11ns] +/-    6ns
> > * PHC0                    0   3   377    10    -14ns[  -20ns] +/-    4ns
> > * PHC0                    0   3   377     8     +4ns[   +5ns] +/-    4ns
> >
> > The PHC0 is the ptp clock which choose the host clock as its source
> > clock. So we can be sure to say that the clock error between host and
> > guest is in order of ns.
> >
> > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> > ---
> >  arch/arm64/include/asm/arch_timer.h  |  3 ++
> >  arch/arm64/kvm/arch_ptp_kvm.c        | 76
> ++++++++++++++++++++++++++++
> >  drivers/clocksource/arm_arch_timer.c |  6 ++-
> >  drivers/ptp/Kconfig                  |  2 +-
> >  include/linux/arm-smccc.h            | 14 +++++
> >  virt/kvm/arm/psci.c                  | 17 +++++++
> >  6 files changed, 115 insertions(+), 3 deletions(-)  create mode
> > 100644 arch/arm64/kvm/arch_ptp_kvm.c
>
> Please split this patch into two parts: the hypervisor code in a patch and the
> guest code in another patch. Having both of them together is confusing.
>
Ok,  really better.

> >
> > diff --git a/arch/arm64/include/asm/arch_timer.h
> > b/arch/arm64/include/asm/arch_timer.h
> > index 6756178c27db..880576a814b6 100644
> > --- a/arch/arm64/include/asm/arch_timer.h
> > +++ b/arch/arm64/include/asm/arch_timer.h
> > @@ -229,4 +229,7 @@ static inline int arch_timer_arch_init(void)
> >     return 0;
> >  }
> >
> > +extern struct clocksource clocksource_counter; extern u64
> > +arch_counter_read(struct clocksource *cs);
>
> I'm definitely not keen on exposing the internals of the arch_timer driver to
> random subsystems. Furthermore, you seem to expect that the guest kernel
> will only use the arch timer as a clocksource, and nothing really guarantees
> that (in which case get_device_system_crosststamp will fail).
>
The code here is really ugly, I need a better solution to offer a clock source
For the guest.

> It looks to me that we'd be better off exposing a core timekeeping API that
> populates a struct system_counterval_t based on the *current* timekeeper
> monotonic clocksource. This would simplify the split between generic and
> arch-specific code.
>
I think it really necessary.

> Whether or not tglx will be happy with the idea is another problem, but I'm
> certainly not taking any change to the arch timer code based on this.
>
I can have a try, but the detail is not clear for me now.

> > +
> >  #endif
> > diff --git a/arch/arm64/kvm/arch_ptp_kvm.c
> > b/arch/arm64/kvm/arch_ptp_kvm.c
>
> We don't put non-hypervisor in arch/arm64/kvm. Please move it back to
> drivers/ptp (as well as its x86 counterpart), and just link the two parts there.
> This should also allow this to be enabled for 32bit guests.
>
Err, sorry, what's mean of "link the two parts there"? should I add another two file update driver/ptp/
Both for arm64 and x86 to contains these arch-specific code or pack them all into ptp_kvm.c?

> > new file mode 100644
> > index 000000000000..6b2165ebce62
> > --- /dev/null
> > +++ b/arch/arm64/kvm/arch_ptp_kvm.c
> > @@ -0,0 +1,76 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + *  Virtual PTP 1588 clock for use with KVM guests
> > + *  Copyright (C) 2019 ARM Ltd.
> > + *  All Rights Reserved
> > + */
> > +
> > +#include <asm/hypervisor.h>
> > +#include <linux/module.h>
> > +#include <linux/psci.h>
> > +#include <linux/arm-smccc.h>
> > +#include <linux/timecounter.h>
> > +#include <linux/sched/clock.h>
> > +#include <asm/arch_timer.h>
> > +
> > +/*
> > + * as trap call cause delay, this function will return the delay in
> > +nanosecond  */ static u64 arm_smccc_1_1_invoke_delay(u32 id, struct
> > +arm_smccc_res *res) {
> > +   u64 ns, t1, t2;
> > +
> > +   t1 = sched_clock();
> > +   arm_smccc_1_1_invoke(id, res);
> > +   t2 = sched_clock();
> > +   t2 -= t1;
> > +   ns = t2;
> > +   return ns;
>
> I think you can get rid of the ns variable here...

Yeah, ns is really redundant.

>
> > +}
> > +
> > +int kvm_arch_ptp_init(void)
> > +{
> > +   return 0;
> > +}
> > +
> > +int kvm_arch_ptp_get_clock(struct timespec64 *ts) {
> > +   u64 ns;
> > +   struct arm_smccc_res hvc_res;
> > +
> > +   if (!kvm_arm_hyp_service_available(
> > +                   ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID)) {
> > +           return -EOPNOTSUPP;
> > +   }
> > +   ns =
> arm_smccc_1_1_invoke_delay(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU
> NC_ID,
> > +                                   &hvc_res);
> > +   ts->tv_sec = hvc_res.a0;
> > +   ts->tv_nsec = hvc_res.a1;
> > +   timespec64_add_ns(ts, ns);
> > +   return 0;
> > +}
> > +
> > +int kvm_arch_ptp_get_clock_fn(long *cycle, struct timespec64 *ts,
> > +                         struct clocksource **cs)
> > +{
> > +   u64 ns;
> > +   struct arm_smccc_res hvc_res;
> > +
> > +   if (!kvm_arm_hyp_service_available(
> > +                   ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID)) {
> > +           return -EOPNOTSUPP;
> > +   }
> > +   ns =
> arm_smccc_1_1_invoke_delay(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU
> NC_ID,
> > +                                   &hvc_res);
> > +   ts->tv_sec = hvc_res.a0;
> > +   ts->tv_nsec = hvc_res.a1;
> > +   timespec64_add_ns(ts, ns);
> > +   *cycle = hvc_res.a2;
> > +   *cs = &clocksource_counter;
> > +
> > +   return 0;
> > +}
>
> Why do we have two functions doing almost the same thing? Why do you call
> kvm_arm_hyp_service_available on each and every time? Isn't it enough to
> check in kvm_arch_ptp_init()?
>

Yeah, it's better.

> > +
> > +MODULE_AUTHOR("Marcelo Tosatti <mtosatti@redhat.com>");
> > +MODULE_DESCRIPTION("PTP clock using KVMCLOCK");
> > +MODULE_LICENSE("GPL");
>
> This should only exist in the generic code.

Ok. I will remove them.

>
> > diff --git a/drivers/clocksource/arm_arch_timer.c
> > b/drivers/clocksource/arm_arch_timer.c
> > index 07e57a49d1e8..021e3f69364c 100644
> > --- a/drivers/clocksource/arm_arch_timer.c
> > +++ b/drivers/clocksource/arm_arch_timer.c
> > @@ -175,23 +175,25 @@ static notrace u64 arch_counter_get_cntvct(void)
> >  u64 (*arch_timer_read_counter)(void) = arch_counter_get_cntvct;
> > EXPORT_SYMBOL_GPL(arch_timer_read_counter);
> >
> > -static u64 arch_counter_read(struct clocksource *cs)
> > +u64 arch_counter_read(struct clocksource *cs)
> >  {
> >     return arch_timer_read_counter();
> >  }
> > +EXPORT_SYMBOL(arch_counter_read);
> >
> >  static u64 arch_counter_read_cc(const struct cyclecounter *cc)  {
> >     return arch_timer_read_counter();
> >  }
> >
> > -static struct clocksource clocksource_counter = {
> > +struct clocksource clocksource_counter = {
> >     .name   = "arch_sys_counter",
> >     .rating = 400,
> >     .read   = arch_counter_read,
> >     .mask   = CLOCKSOURCE_MASK(56),
> >     .flags  = CLOCK_SOURCE_IS_CONTINUOUS,
> >  };
> > +EXPORT_SYMBOL(clocksource_counter);
>
> I've said what I thought about this. Not happening.
>
Ok.

> >
> >  static struct cyclecounter cyclecounter __ro_after_init = {
> >     .read   = arch_counter_read_cc,
> > diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig index
> > 9b8fee5178e8..e032fafdafa7 100644
> > --- a/drivers/ptp/Kconfig
> > +++ b/drivers/ptp/Kconfig
> > @@ -110,7 +110,7 @@ config PTP_1588_CLOCK_PCH  config
> > PTP_1588_CLOCK_KVM
> >     tristate "KVM virtual PTP clock"
> >     depends on PTP_1588_CLOCK
> > -   depends on KVM_GUEST && X86
> > +   depends on KVM_GUEST && X86 || ARM64
> >     default y
> >     help
> >       This driver adds support for using kvm infrastructure as a PTP
> > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> > index a6e4d3e3d10a..2a222a1a8594 100644
> > --- a/include/linux/arm-smccc.h
> > +++ b/include/linux/arm-smccc.h
> > @@ -94,6 +94,7 @@
> >
> >  /* KVM "vendor specific" services */
> >  #define ARM_SMCCC_KVM_FUNC_FEATURES                0
> > +#define ARM_SMCCC_KVM_PTP                  1
> >  #define ARM_SMCCC_KVM_FUNC_FEATURES_2              127
> >  #define ARM_SMCCC_KVM_NUM_FUNCS                    128
> >
> > @@ -102,6 +103,16 @@
> >                        ARM_SMCCC_SMC_32,
>       \
> >                        ARM_SMCCC_OWNER_VENDOR_HYP,
>               \
> >                        ARM_SMCCC_KVM_FUNC_FEATURES)
> > +/*
> > + * This ID used for virtual ptp kvm clock and it will pass second
> > +value
> > + * and nanosecond value of host real time and system counter by vcpu
> > + * register to guest.
> > + */
> > +#define ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID
>               \
> > +   ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,
>               \
> > +                      ARM_SMCCC_SMC_32,
>       \
> > +                      ARM_SMCCC_OWNER_VENDOR_HYP,
>               \
> > +                      ARM_SMCCC_KVM_PTP)
> >
> >  #ifndef __ASSEMBLY__
> >
> > @@ -373,5 +384,8 @@ asmlinkage void __arm_smccc_hvc(unsigned long
> a0, unsigned long a1,
> >             method;
>       \
> >     })
> >
> > +#include <linux/psci.h>
> > +#include <linux/clocksource.h>
> > +
> >  #endif /*__ASSEMBLY__*/
> >  #endif /*__LINUX_ARM_SMCCC_H*/
> > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c index
> > 0debf49bf259..7fffdb25d32c 100644
> > --- a/virt/kvm/arm/psci.c
> > +++ b/virt/kvm/arm/psci.c
> > @@ -392,6 +392,8 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
> >     u32 func_id = smccc_get_function(vcpu);
> >     u32 val[4] = {};
> >     u32 option;
> > +   struct timespec *ts;
> > +   u64 cnt;
> >
> >     val[0] = SMCCC_RET_NOT_SUPPORTED;
> >
> > @@ -431,6 +433,21 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
> >     case ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID:
> >             val[0] = BIT(ARM_SMCCC_KVM_FUNC_FEATURES);
> >             break;
> > +   /*
> > +    * This will used for virtual ptp kvm clock. three
> > +    * values will be passed back.
> > +    * reg0 stores seconds of host real time;
> > +    * reg1 stores nanoseconds of host real time;
> > +    * reg2 stotes system counter cycle value.
>
> stores

Yeah

>
> > +    */
> > +   case ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID:
> > +           getnstimeofday(ts);
> > +           cnt = arch_timer_read_counter();
> > +           val[0] = ts->tv_sec;
> > +           val[1] = ts->tv_nsec;
> > +           val[2] = cnt;
>
> Can you explain what the purpose of exposing this counter is? The guest
> should have access to the physical counter already.

One api of ptp_kvm called ptp_kvm_get_time_fn need a clock sources passed from host as system_counter.
>
> > +           val[3] = 0;
> > +           break;
>
> This will probably conflict with Steven's stolen time series. Not a big deal
> though.
Err, sorry I am not familiar with this theory. Let me check it.

>
> >     default:
> >             return kvm_psci_call(vcpu);
> >     }
> >
>
> Other questions: how does this works with VM migration? Specially when
> moving from a hypervisor that supports the feature to one that doesn't?
>
I think it won't solve the problem generated by VM migration and only for VMs in a single machine.
Ptp_kvm only works for VMs in the same machine.
But using ptp (not ptp_kvm) clock, all the machines in a low latency network environment can keep time sync in high precision,
Then VMs move from one machine to another will obtain a high precision time sync.

Thanks
Jianyong Wu

> Thanks,
>
>       M.
> --
> Jazz is not dead, it just smells funny...
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [RFC PATCH 3/3] Enable ptp_kvm for arm64

[Marc Zyngier]

On Fri, 06 Sep 2019 12:58:15 +0100,
"Jianyong Wu (Arm Technology China)" <Jianyong.Wu@arm.com> wrote:
> 
> Hi Marc,
> 
> Very sorry to have missed this comments.
> 
> > -----Original Message-----
> > From: Marc Zyngier <maz@kernel.org>
> > Sent: Thursday, August 29, 2019 6:33 PM
> > To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>;
> > netdev@vger.kernel.org; pbonzini@redhat.com;
> > sean.j.christopherson@intel.com; richardcochran@gmail.com; Mark Rutland
> > <Mark.Rutland@arm.com>; Will Deacon <Will.Deacon@arm.com>; Suzuki
> > Poulose <Suzuki.Poulose@arm.com>
> > Cc: linux-kernel@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>;
> > Kaly Xin (Arm Technology China) <Kaly.Xin@arm.com>; Justin He (Arm
> > Technology China) <Justin.He@arm.com>
> > Subject: Re: [RFC PATCH 3/3] Enable ptp_kvm for arm64
> >
> > On 29/08/2019 07:39, Jianyong Wu wrote:
> > > Currently in arm64 virtualization environment, there is no mechanism
> > > to keep time sync between guest and host. Time in guest will drift
> > > compared with host after boot up as they may both use third party time
> > > sources to correct their time respectively. The time deviation will be
> > > in order of milliseconds but some scenarios ask for higher time
> > > precision, like in cloud envirenment, we want all the VMs running in
> > > the host aquire the same level accuracy from host clock.
> > >
> > > Use of kvm ptp clock, which choose the host clock source clock as a
> > > reference clock to sync time clock between guest and host has been
> > > adopted by x86 which makes the time sync order from milliseconds to
> > nanoseconds.
> > >
> > > This patch enable kvm ptp on arm64 and we get the similar clock drift
> > > as found with x86 with kvm ptp.
> > >
> > > Test result comparison between with kvm ptp and without it in arm64
> > > are as follows. This test derived from the result of command 'chronyc
> > > sources'. we should take more cure of the last sample column which
> > > shows the offset between the local clock and the source at the last
> > measurement.
> > >
> > > no kvm ptp in guest:
> > > MS Name/IP address   Stratum Poll Reach LastRx Last sample
> > >
> > ==========================================================
> > ==============
> > > ^* dns1.synet.edu.cn      2   6   377    13  +1040us[+1581us] +/-   21ms
> > > ^* dns1.synet.edu.cn      2   6   377    21  +1040us[+1581us] +/-   21ms
> > > ^* dns1.synet.edu.cn      2   6   377    29  +1040us[+1581us] +/-   21ms
> > > ^* dns1.synet.edu.cn      2   6   377    37  +1040us[+1581us] +/-   21ms
> > > ^* dns1.synet.edu.cn      2   6   377    45  +1040us[+1581us] +/-   21ms
> > > ^* dns1.synet.edu.cn      2   6   377    53  +1040us[+1581us] +/-   21ms
> > > ^* dns1.synet.edu.cn      2   6   377    61  +1040us[+1581us] +/-   21ms
> > > ^* dns1.synet.edu.cn      2   6   377     4   -130us[ +796us] +/-   21ms
> > > ^* dns1.synet.edu.cn      2   6   377    12   -130us[ +796us] +/-   21ms
> > > ^* dns1.synet.edu.cn      2   6   377    20   -130us[ +796us] +/-   21ms
> > >
> > > in host:
> > > MS Name/IP address   Stratum Poll Reach LastRx Last sample
> > >
> > ==========================================================
> > ==============
> > > ^* 120.25.115.20          2   7   377    72   -470us[ -603us] +/-   18ms
> > > ^* 120.25.115.20          2   7   377    92   -470us[ -603us] +/-   18ms
> > > ^* 120.25.115.20          2   7   377   112   -470us[ -603us] +/-   18ms
> > > ^* 120.25.115.20          2   7   377     2   +872ns[-6808ns] +/-   17ms
> > > ^* 120.25.115.20          2   7   377    22   +872ns[-6808ns] +/-   17ms
> > > ^* 120.25.115.20          2   7   377    43   +872ns[-6808ns] +/-   17ms
> > > ^* 120.25.115.20          2   7   377    63   +872ns[-6808ns] +/-   17ms
> > > ^* 120.25.115.20          2   7   377    83   +872ns[-6808ns] +/-   17ms
> > > ^* 120.25.115.20          2   7   377   103   +872ns[-6808ns] +/-   17ms
> > > ^* 120.25.115.20          2   7   377   123   +872ns[-6808ns] +/-   17ms
> > >
> > > The dns1.synet.edu.cn is the network reference clock for guest and
> > > 120.25.115.20 is the network reference clock for host. we can't get
> > > the clock error between guest and host directly, but a roughly
> > > estimated value will be in order of hundreds of us to ms.
> > >
> > > with kvm ptp in guest:
> > > chrony has been disabled in host to remove the disturb by network clock.
> >
> > Is that a realistic use case? Why should the host not use NTP?
> >
> 
> Not really, NTP will change the the host clock which will contaminate the data of sync between
> Host and guest. But in reality, we will keep NTP online.
> 
> > >
> > > MS Name/IP address         Stratum Poll Reach LastRx Last sample
> > >
> > ==========================================================
> > ==============
> > > * PHC0                    0   3   377     8     -7ns[   +1ns] +/-    3ns
> > > * PHC0                    0   3   377     8     +1ns[  +16ns] +/-    3ns
> > > * PHC0                    0   3   377     6     -4ns[   -0ns] +/-    6ns
> > > * PHC0                    0   3   377     6     -8ns[  -12ns] +/-    5ns
> > > * PHC0                    0   3   377     5     +2ns[   +4ns] +/-    4ns
> > > * PHC0                    0   3   377    13     +2ns[   +4ns] +/-    4ns
> > > * PHC0                    0   3   377    12     -4ns[   -6ns] +/-    4ns
> > > * PHC0                    0   3   377    11     -8ns[  -11ns] +/-    6ns
> > > * PHC0                    0   3   377    10    -14ns[  -20ns] +/-    4ns
> > > * PHC0                    0   3   377     8     +4ns[   +5ns] +/-    4ns
> > >
> > > The PHC0 is the ptp clock which choose the host clock as its source
> > > clock. So we can be sure to say that the clock error between host and
> > > guest is in order of ns.
> > >
> > > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> > > ---
> > >  arch/arm64/include/asm/arch_timer.h  |  3 ++
> > >  arch/arm64/kvm/arch_ptp_kvm.c        | 76
> > ++++++++++++++++++++++++++++
> > >  drivers/clocksource/arm_arch_timer.c |  6 ++-
> > >  drivers/ptp/Kconfig                  |  2 +-
> > >  include/linux/arm-smccc.h            | 14 +++++
> > >  virt/kvm/arm/psci.c                  | 17 +++++++
> > >  6 files changed, 115 insertions(+), 3 deletions(-)  create mode
> > > 100644 arch/arm64/kvm/arch_ptp_kvm.c
> >
> > Please split this patch into two parts: the hypervisor code in a patch and the
> > guest code in another patch. Having both of them together is confusing.
> >
> Ok,  really better.
> 
> > >
> > > diff --git a/arch/arm64/include/asm/arch_timer.h
> > > b/arch/arm64/include/asm/arch_timer.h
> > > index 6756178c27db..880576a814b6 100644
> > > --- a/arch/arm64/include/asm/arch_timer.h
> > > +++ b/arch/arm64/include/asm/arch_timer.h
> > > @@ -229,4 +229,7 @@ static inline int arch_timer_arch_init(void)
> > >     return 0;
> > >  }
> > >
> > > +extern struct clocksource clocksource_counter; extern u64
> > > +arch_counter_read(struct clocksource *cs);
> >
> > I'm definitely not keen on exposing the internals of the arch_timer driver to
> > random subsystems. Furthermore, you seem to expect that the guest kernel
> > will only use the arch timer as a clocksource, and nothing really guarantees
> > that (in which case get_device_system_crosststamp will fail).
> >
> The code here is really ugly, I need a better solution to offer a clock source
> For the guest.
> 
> > It looks to me that we'd be better off exposing a core timekeeping API that
> > populates a struct system_counterval_t based on the *current* timekeeper
> > monotonic clocksource. This would simplify the split between generic and
> > arch-specific code.
> >
> I think it really necessary.
> 
> > Whether or not tglx will be happy with the idea is another problem, but I'm
> > certainly not taking any change to the arch timer code based on this.
> >
> I can have a try, but the detail is not clear for me now.

Something along those lines:

From 5f1c061e55c691d64012bc7c1490a1a8c4432c67 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@kernel.org>
Date: Sat, 7 Sep 2019 10:11:49 +0100
Subject: [PATCH] timekeeping: Expose API allowing retrival of current
 clocksource and counter value

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 include/linux/timekeeping.h |  5 +++++
 kernel/time/timekeeping.c   | 12 ++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index b27e2ffa96c1..6df26a913711 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -275,6 +275,11 @@ extern int get_device_system_crosststamp(
 			struct system_time_snapshot *history,
 			struct system_device_crosststamp *xtstamp);
 
+/*
+ * Obtain current monotonic clock and its counter value
+ */
+extern void get_current_counterval(struct system_counterval_t *sc);
+
 /*
  * Simultaneously snapshot realtime and monotonic raw clocks
  */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index d911c8470149..de689bbd3808 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1098,6 +1098,18 @@ static bool cycle_between(u64 before, u64 test, u64 after)
 	return false;
 }
 
+/**
+ * get_current_counterval - Snapshot the current clocksource and counter value
+ * @sc:	Pointer to a struct containing the current clocksource and its value
+ */
+void get_current_counterval(struct system_counterval_t *sc)
+{
+	struct timekeeper *tk = &tk_core.timekeeper;
+
+	sc->cs = READ_ONCE(tk->tkr_mono.clock);
+	sc->cycles = sc->cs->read(sc->cs);
+}
+
 /**
  * get_device_system_crosststamp - Synchronously capture system/device timestamp
  * @get_time_fn:	Callback to get simultaneous device time and

which should do the right thing.

>
> > > +
> > >  #endif
> > > diff --git a/arch/arm64/kvm/arch_ptp_kvm.c
> > > b/arch/arm64/kvm/arch_ptp_kvm.c
> >
> > We don't put non-hypervisor in arch/arm64/kvm. Please move it back to
> > drivers/ptp (as well as its x86 counterpart), and just link the two parts there.
> > This should also allow this to be enabled for 32bit guests.
> >
> Err, sorry, what's mean of "link the two parts there"? should I add
> another two file update driver/ptp/ Both for arm64 and x86 to
> contains these arch-specific code or pack them all into ptp_kvm.c?

What I'm suggesting is that you have 3 files:

  drivers/ptp/ptp_kvm.c
  drivers/ptp/ptp_kvm_x86.c
  drivers/ptp/ptp_kvm_arm.c

and let the Makefile combine them.

[...]

> > Other questions: how does this works with VM migration? Specially when
> > moving from a hypervisor that supports the feature to one that doesn't?
> >
> I think it won't solve the problem generated by VM migration and
> only for VMs in a single machine.  Ptp_kvm only works for VMs in the
> same machine.  But using ptp (not ptp_kvm) clock, all the machines
> in a low latency network environment can keep time sync in high
> precision, Then VMs move from one machine to another will obtain a
> high precision time sync.

That's a problem. Migration must be possible from one host to another,
even if that means temporarily loosing some (or a lot of)
precision. The service must be discoverable from userspace on the host
so that the MVV can decie whether a migration is possible or not.

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.

RE: [RFC PATCH 3/3] Enable ptp_kvm for arm64

[Jianyong Wu (Arm Technology China)]

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Saturday, September 7, 2019 5:16 PM
> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>
> Cc: netdev@vger.kernel.org; pbonzini@redhat.com;
> sean.j.christopherson@intel.com; richardcochran@gmail.com; Mark Rutland
> <Mark.Rutland@arm.com>; Will Deacon <Will.Deacon@arm.com>; Suzuki
> Poulose <Suzuki.Poulose@arm.com>; linux-kernel@vger.kernel.org; Steve
> Capper <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China)
> <Kaly.Xin@arm.com>; Justin He (Arm Technology China)
> <Justin.He@arm.com>
> Subject: Re: [RFC PATCH 3/3] Enable ptp_kvm for arm64
>
> On Fri, 06 Sep 2019 12:58:15 +0100,
> "Jianyong Wu (Arm Technology China)" <Jianyong.Wu@arm.com> wrote:
> >
> > Hi Marc,
> >
> > Very sorry to have missed this comments.
> >
> > > -----Original Message-----
> > > From: Marc Zyngier <maz@kernel.org>
> > > Sent: Thursday, August 29, 2019 6:33 PM
> > > To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>;
> > > netdev@vger.kernel.org; pbonzini@redhat.com;
> > > sean.j.christopherson@intel.com; richardcochran@gmail.com; Mark
> > > Rutland <Mark.Rutland@arm.com>; Will Deacon
> <Will.Deacon@arm.com>;
> > > Suzuki Poulose <Suzuki.Poulose@arm.com>
> > > Cc: linux-kernel@vger.kernel.org; Steve Capper
> > > <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China)
> > > <Kaly.Xin@arm.com>; Justin He (Arm Technology China)
> > > <Justin.He@arm.com>
> > > Subject: Re: [RFC PATCH 3/3] Enable ptp_kvm for arm64
> > >
> > > On 29/08/2019 07:39, Jianyong Wu wrote:
> > > > Currently in arm64 virtualization environment, there is no
> > > > mechanism to keep time sync between guest and host. Time in guest
> > > > will drift compared with host after boot up as they may both use
> > > > third party time sources to correct their time respectively. The
> > > > time deviation will be in order of milliseconds but some scenarios
> > > > ask for higher time precision, like in cloud envirenment, we want
> > > > all the VMs running in the host aquire the same level accuracy from
> host clock.
> > > >
> > > > Use of kvm ptp clock, which choose the host clock source clock as
> > > > a reference clock to sync time clock between guest and host has
> > > > been adopted by x86 which makes the time sync order from
> > > > milliseconds to
> > > nanoseconds.
> > > >
> > > > This patch enable kvm ptp on arm64 and we get the similar clock
> > > > drift as found with x86 with kvm ptp.
> > > >
> > > > Test result comparison between with kvm ptp and without it in
> > > > arm64 are as follows. This test derived from the result of command
> > > > 'chronyc sources'. we should take more cure of the last sample
> > > > column which shows the offset between the local clock and the
> > > > source at the last
> > > measurement.
> > > >
> > > > no kvm ptp in guest:
> > > > MS Name/IP address   Stratum Poll Reach LastRx Last sample
> > > >
> > >
> ==========================================================
> > > ==============
> > > > ^* dns1.synet.edu.cn      2   6   377    13  +1040us[+1581us] +/-   21ms
> > > > ^* dns1.synet.edu.cn      2   6   377    21  +1040us[+1581us] +/-   21ms
> > > > ^* dns1.synet.edu.cn      2   6   377    29  +1040us[+1581us] +/-   21ms
> > > > ^* dns1.synet.edu.cn      2   6   377    37  +1040us[+1581us] +/-   21ms
> > > > ^* dns1.synet.edu.cn      2   6   377    45  +1040us[+1581us] +/-   21ms
> > > > ^* dns1.synet.edu.cn      2   6   377    53  +1040us[+1581us] +/-   21ms
> > > > ^* dns1.synet.edu.cn      2   6   377    61  +1040us[+1581us] +/-   21ms
> > > > ^* dns1.synet.edu.cn      2   6   377     4   -130us[ +796us] +/-   21ms
> > > > ^* dns1.synet.edu.cn      2   6   377    12   -130us[ +796us] +/-   21ms
> > > > ^* dns1.synet.edu.cn      2   6   377    20   -130us[ +796us] +/-   21ms
> > > >
> > > > in host:
> > > > MS Name/IP address   Stratum Poll Reach LastRx Last sample
> > > >
> > >
> ==========================================================
> > > ==============
> > > > ^* 120.25.115.20          2   7   377    72   -470us[ -603us] +/-   18ms
> > > > ^* 120.25.115.20          2   7   377    92   -470us[ -603us] +/-   18ms
> > > > ^* 120.25.115.20          2   7   377   112   -470us[ -603us] +/-   18ms
> > > > ^* 120.25.115.20          2   7   377     2   +872ns[-6808ns] +/-   17ms
> > > > ^* 120.25.115.20          2   7   377    22   +872ns[-6808ns] +/-   17ms
> > > > ^* 120.25.115.20          2   7   377    43   +872ns[-6808ns] +/-   17ms
> > > > ^* 120.25.115.20          2   7   377    63   +872ns[-6808ns] +/-   17ms
> > > > ^* 120.25.115.20          2   7   377    83   +872ns[-6808ns] +/-   17ms
> > > > ^* 120.25.115.20          2   7   377   103   +872ns[-6808ns] +/-   17ms
> > > > ^* 120.25.115.20          2   7   377   123   +872ns[-6808ns] +/-   17ms
> > > >
> > > > The dns1.synet.edu.cn is the network reference clock for guest and
> > > > 120.25.115.20 is the network reference clock for host. we can't
> > > > get the clock error between guest and host directly, but a roughly
> > > > estimated value will be in order of hundreds of us to ms.
> > > >
> > > > with kvm ptp in guest:
> > > > chrony has been disabled in host to remove the disturb by network
> clock.
> > >
> > > Is that a realistic use case? Why should the host not use NTP?
> > >
> >
> > Not really, NTP will change the the host clock which will contaminate
> > the data of sync between Host and guest. But in reality, we will keep NTP
> online.
> >
> > > >
> > > > MS Name/IP address         Stratum Poll Reach LastRx Last sample
> > > >
> > >
> ==========================================================
> > > ==============
> > > > * PHC0                    0   3   377     8     -7ns[   +1ns] +/-    3ns
> > > > * PHC0                    0   3   377     8     +1ns[  +16ns] +/-    3ns
> > > > * PHC0                    0   3   377     6     -4ns[   -0ns] +/-    6ns
> > > > * PHC0                    0   3   377     6     -8ns[  -12ns] +/-    5ns
> > > > * PHC0                    0   3   377     5     +2ns[   +4ns] +/-    4ns
> > > > * PHC0                    0   3   377    13     +2ns[   +4ns] +/-    4ns
> > > > * PHC0                    0   3   377    12     -4ns[   -6ns] +/-    4ns
> > > > * PHC0                    0   3   377    11     -8ns[  -11ns] +/-    6ns
> > > > * PHC0                    0   3   377    10    -14ns[  -20ns] +/-    4ns
> > > > * PHC0                    0   3   377     8     +4ns[   +5ns] +/-    4ns
> > > >
> > > > The PHC0 is the ptp clock which choose the host clock as its
> > > > source clock. So we can be sure to say that the clock error
> > > > between host and guest is in order of ns.
> > > >
> > > > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> > > > ---
> > > >  arch/arm64/include/asm/arch_timer.h  |  3 ++
> > > >  arch/arm64/kvm/arch_ptp_kvm.c        | 76
> > > ++++++++++++++++++++++++++++
> > > >  drivers/clocksource/arm_arch_timer.c |  6 ++-
> > > >  drivers/ptp/Kconfig                  |  2 +-
> > > >  include/linux/arm-smccc.h            | 14 +++++
> > > >  virt/kvm/arm/psci.c                  | 17 +++++++
> > > >  6 files changed, 115 insertions(+), 3 deletions(-)  create mode
> > > > 100644 arch/arm64/kvm/arch_ptp_kvm.c
> > >
> > > Please split this patch into two parts: the hypervisor code in a
> > > patch and the guest code in another patch. Having both of them together
> is confusing.
> > >
> > Ok,  really better.
> >
> > > >
> > > > diff --git a/arch/arm64/include/asm/arch_timer.h
> > > > b/arch/arm64/include/asm/arch_timer.h
> > > > index 6756178c27db..880576a814b6 100644
> > > > --- a/arch/arm64/include/asm/arch_timer.h
> > > > +++ b/arch/arm64/include/asm/arch_timer.h
> > > > @@ -229,4 +229,7 @@ static inline int arch_timer_arch_init(void)
> > > >     return 0;
> > > >  }
> > > >
> > > > +extern struct clocksource clocksource_counter; extern u64
> > > > +arch_counter_read(struct clocksource *cs);
> > >
> > > I'm definitely not keen on exposing the internals of the arch_timer
> > > driver to random subsystems. Furthermore, you seem to expect that
> > > the guest kernel will only use the arch timer as a clocksource, and
> > > nothing really guarantees that (in which case
> get_device_system_crosststamp will fail).
> > >
> > The code here is really ugly, I need a better solution to offer a
> > clock source For the guest.
> >
> > > It looks to me that we'd be better off exposing a core timekeeping
> > > API that populates a struct system_counterval_t based on the
> > > *current* timekeeper monotonic clocksource. This would simplify the
> > > split between generic and arch-specific code.
> > >
> > I think it really necessary.
> >
> > > Whether or not tglx will be happy with the idea is another problem,
> > > but I'm certainly not taking any change to the arch timer code based on
> this.
> > >
> > I can have a try, but the detail is not clear for me now.
>
> Something along those lines:
>
> From 5f1c061e55c691d64012bc7c1490a1a8c4432c67 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz@kernel.org>
> Date: Sat, 7 Sep 2019 10:11:49 +0100
> Subject: [PATCH] timekeeping: Expose API allowing retrival of current
> clocksource and counter value
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  include/linux/timekeeping.h |  5 +++++
>  kernel/time/timekeeping.c   | 12 ++++++++++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h index
> b27e2ffa96c1..6df26a913711 100644
> --- a/include/linux/timekeeping.h
> +++ b/include/linux/timekeeping.h
> @@ -275,6 +275,11 @@ extern int get_device_system_crosststamp(
>                       struct system_time_snapshot *history,
>                       struct system_device_crosststamp *xtstamp);
>
> +/*
> + * Obtain current monotonic clock and its counter value  */ extern void
> +get_current_counterval(struct system_counterval_t *sc);
> +
>  /*
>   * Simultaneously snapshot realtime and monotonic raw clocks
>   */
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c index
> d911c8470149..de689bbd3808 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -1098,6 +1098,18 @@ static bool cycle_between(u64 before, u64 test,
> u64 after)
>       return false;
>  }
>
> +/**
> + * get_current_counterval - Snapshot the current clocksource and counter
> value
> + * @sc:      Pointer to a struct containing the current clocksource and its
> value
> + */
> +void get_current_counterval(struct system_counterval_t *sc) {
> +     struct timekeeper *tk = &tk_core.timekeeper;
> +
> +     sc->cs = READ_ONCE(tk->tkr_mono.clock);
> +     sc->cycles = sc->cs->read(sc->cs);
> +}
> +
>  /**
>   * get_device_system_crosststamp - Synchronously capture system/device
> timestamp
>   * @get_time_fn:     Callback to get simultaneous device time and
>
> which should do the right thing.
>
It is a good news for me. These code is indeed what I need!
So what's your plan about this patch?  Is there any problem with you if I include these code
into my patch ?

> >
> > > > +
> > > >  #endif
> > > > diff --git a/arch/arm64/kvm/arch_ptp_kvm.c
> > > > b/arch/arm64/kvm/arch_ptp_kvm.c
> > >
> > > We don't put non-hypervisor in arch/arm64/kvm. Please move it back
> > > to drivers/ptp (as well as its x86 counterpart), and just link the two parts
> there.
> > > This should also allow this to be enabled for 32bit guests.
> > >
> > Err, sorry, what's mean of "link the two parts there"? should I add
> > another two file update driver/ptp/ Both for arm64 and x86 to contains
> > these arch-specific code or pack them all into ptp_kvm.c?
>
> What I'm suggesting is that you have 3 files:
>
>   drivers/ptp/ptp_kvm.c
>   drivers/ptp/ptp_kvm_x86.c
>   drivers/ptp/ptp_kvm_arm.c
>
> and let the Makefile combine them.
>
> [...]
>
it is what I want to do at the beginning of drafting these patches.

> > > Other questions: how does this works with VM migration? Specially
> > > when moving from a hypervisor that supports the feature to one that
> doesn't?
> > >
> > I think it won't solve the problem generated by VM migration and only
> > for VMs in a single machine.  Ptp_kvm only works for VMs in the same
> > machine.  But using ptp (not ptp_kvm) clock, all the machines in a low
> > latency network environment can keep time sync in high precision, Then
> > VMs move from one machine to another will obtain a high precision time
> > sync.
>
> That's a problem. Migration must be possible from one host to another, even
> if that means temporarily loosing some (or a lot of) precision. The service
> must be discoverable from userspace on the host so that the MVV can decie
> whether a migration is possible or not.
>
Don't worry, things will be not that bad.
ptp_kvm will not trouble the VM migration. This ptp_kvm is one clocksource of the clock pool for
chrony. Chrony will choose the highest precision clock from the pool. If host does not support
ptp_kvm, the ptp_kvm will not be chosen as the clocksouce of chrony.
We have roughly the same logic of implementation of ptp_kvm with x86, and ptp_kvm works well in x86.
so I think that will be the case for arm64.

Maybe I miss your point, I have no idea of MVV and can't get related info from google.
Also I'm not clear of your last words of how to decide VM migration is possible?

Thanks
Jianyong Wu

> Thanks,
>
>       M.
>
> --
> Jazz is not dead, it just smells funny.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [RFC PATCH 3/3] Enable ptp_kvm for arm64

[Marc Zyngier]

On Mon, 09 Sep 2019 11:17:24 +0100,
"Jianyong Wu (Arm Technology China)" <Jianyong.Wu@arm.com> wrote:

Hi Jianyoung,

[...]

> > > > I'm definitely not keen on exposing the internals of the arch_timer
> > > > driver to random subsystems. Furthermore, you seem to expect that
> > > > the guest kernel will only use the arch timer as a clocksource, and
> > > > nothing really guarantees that (in which case
> > get_device_system_crosststamp will fail).
> > > >
> > > The code here is really ugly, I need a better solution to offer a
> > > clock source For the guest.
> > >
> > > > It looks to me that we'd be better off exposing a core timekeeping
> > > > API that populates a struct system_counterval_t based on the
> > > > *current* timekeeper monotonic clocksource. This would simplify the
> > > > split between generic and arch-specific code.
> > > >
> > > I think it really necessary.
> > >
> > > > Whether or not tglx will be happy with the idea is another problem,
> > > > but I'm certainly not taking any change to the arch timer code based on
> > this.
> > > >
> > > I can have a try, but the detail is not clear for me now.
> >
> > Something along those lines:
> >
> > From 5f1c061e55c691d64012bc7c1490a1a8c4432c67 Mon Sep 17 00:00:00 2001
> > From: Marc Zyngier <maz@kernel.org>
> > Date: Sat, 7 Sep 2019 10:11:49 +0100
> > Subject: [PATCH] timekeeping: Expose API allowing retrival of current
> > clocksource and counter value
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  include/linux/timekeeping.h |  5 +++++
> >  kernel/time/timekeeping.c   | 12 ++++++++++++
> >  2 files changed, 17 insertions(+)
> >
> > diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h index
> > b27e2ffa96c1..6df26a913711 100644
> > --- a/include/linux/timekeeping.h
> > +++ b/include/linux/timekeeping.h
> > @@ -275,6 +275,11 @@ extern int get_device_system_crosststamp(
> >                       struct system_time_snapshot *history,
> >                       struct system_device_crosststamp *xtstamp);
> >
> > +/*
> > + * Obtain current monotonic clock and its counter value  */ extern void
> > +get_current_counterval(struct system_counterval_t *sc);
> > +
> >  /*
> >   * Simultaneously snapshot realtime and monotonic raw clocks
> >   */
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c index
> > d911c8470149..de689bbd3808 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -1098,6 +1098,18 @@ static bool cycle_between(u64 before, u64 test,
> > u64 after)
> >       return false;
> >  }
> >
> > +/**
> > + * get_current_counterval - Snapshot the current clocksource and counter
> > value
> > + * @sc:      Pointer to a struct containing the current clocksource and its
> > value
> > + */
> > +void get_current_counterval(struct system_counterval_t *sc) {
> > +     struct timekeeper *tk = &tk_core.timekeeper;
> > +
> > +     sc->cs = READ_ONCE(tk->tkr_mono.clock);
> > +     sc->cycles = sc->cs->read(sc->cs);
> > +}
> > +
> >  /**
> >   * get_device_system_crosststamp - Synchronously capture system/device
> > timestamp
> >   * @get_time_fn:     Callback to get simultaneous device time and
> >
> > which should do the right thing.
> >
> It is a good news for me. These code is indeed what I need!  So
> what's your plan about this patch?  Is there any problem with you if
> I include these code into my patch ?

Just add this patch as part of your series (I'll try to write an
actual commit log for that).

[...]

> > > > Other questions: how does this works with VM migration? Specially
> > > > when moving from a hypervisor that supports the feature to one that
> > doesn't?
> > > >
> > > I think it won't solve the problem generated by VM migration and only
> > > for VMs in a single machine.  Ptp_kvm only works for VMs in the same
> > > machine.  But using ptp (not ptp_kvm) clock, all the machines in a low
> > > latency network environment can keep time sync in high precision, Then
> > > VMs move from one machine to another will obtain a high precision time
> > > sync.
> >
> > That's a problem. Migration must be possible from one host to another, even
> > if that means temporarily loosing some (or a lot of) precision. The service
> > must be discoverable from userspace on the host so that the MVV can decie
> > whether a migration is possible or not.
> >
> Don't worry, things will be not that bad.  ptp_kvm will not trouble
> the VM migration. This ptp_kvm is one clocksource of the clock pool
> for chrony. Chrony will choose the highest precision clock from the
> pool. If host does not support ptp_kvm, the ptp_kvm will not be
> chosen as the clocksouce of chrony.  We have roughly the same logic
> of implementation of ptp_kvm with x86, and ptp_kvm works well in
> x86.  so I think that will be the case for arm64.
> 
> Maybe I miss your point, I have no idea of MVV and can't get related
> info from google.  Also I'm not clear of your last words of how to
> decide VM migration is possible?

Sorry. s/MVV/VMM/. Basically userspace, such as QEMU.

Here's an example: The guest runs on a PTP aware host, starts using
the PTP service and uses HVC calls to get its clock. We now migrate
the guest to a non PTP-aware host. The hypercalls are now going to
fail unexpectedly. Is that something that is acceptable? I don't think
it is. Once you've allowed a guest to use a service, this service
should be preserved. I'd be more confident if we gave to userspace the
indication that the hypervisor supports PTP. Userspace can then decide
whether to perform migration or not.

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.

RE: [RFC PATCH 3/3] Enable ptp_kvm for arm64

[Jianyong Wu (Arm Technology China)]

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Monday, September 9, 2019 7:25 PM
> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>
> Cc: netdev@vger.kernel.org; pbonzini@redhat.com;
> sean.j.christopherson@intel.com; richardcochran@gmail.com; Mark Rutland
> <Mark.Rutland@arm.com>; Will Deacon <Will.Deacon@arm.com>; Suzuki
> Poulose <Suzuki.Poulose@arm.com>; linux-kernel@vger.kernel.org; Steve
> Capper <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China)
> <Kaly.Xin@arm.com>; Justin He (Arm Technology China)
> <Justin.He@arm.com>
> Subject: Re: [RFC PATCH 3/3] Enable ptp_kvm for arm64
>
> On Mon, 09 Sep 2019 11:17:24 +0100,
> "Jianyong Wu (Arm Technology China)" <Jianyong.Wu@arm.com> wrote:
>
> Hi Jianyoung,
>
> [...]
>
> > > > > I'm definitely not keen on exposing the internals of the
> > > > > arch_timer driver to random subsystems. Furthermore, you seem to
> > > > > expect that the guest kernel will only use the arch timer as a
> > > > > clocksource, and nothing really guarantees that (in which case
> > > get_device_system_crosststamp will fail).
> > > > >
> > > > The code here is really ugly, I need a better solution to offer a
> > > > clock source For the guest.
> > > >
> > > > > It looks to me that we'd be better off exposing a core
> > > > > timekeeping API that populates a struct system_counterval_t
> > > > > based on the
> > > > > *current* timekeeper monotonic clocksource. This would simplify
> > > > > the split between generic and arch-specific code.
> > > > >
> > > > I think it really necessary.
> > > >
> > > > > Whether or not tglx will be happy with the idea is another
> > > > > problem, but I'm certainly not taking any change to the arch
> > > > > timer code based on
> > > this.
> > > > >
> > > > I can have a try, but the detail is not clear for me now.
> > >
> > > Something along those lines:
> > >
> > > From 5f1c061e55c691d64012bc7c1490a1a8c4432c67 Mon Sep 17 00:00:00
> > > 2001
> > > From: Marc Zyngier <maz@kernel.org>
> > > Date: Sat, 7 Sep 2019 10:11:49 +0100
> > > Subject: [PATCH] timekeeping: Expose API allowing retrival of
> > > current clocksource and counter value
> > >
> > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > ---
> > >  include/linux/timekeeping.h |  5 +++++
> > >  kernel/time/timekeeping.c   | 12 ++++++++++++
> > >  2 files changed, 17 insertions(+)
> > >
> > > diff --git a/include/linux/timekeeping.h
> > > b/include/linux/timekeeping.h index
> > > b27e2ffa96c1..6df26a913711 100644
> > > --- a/include/linux/timekeeping.h
> > > +++ b/include/linux/timekeeping.h
> > > @@ -275,6 +275,11 @@ extern int get_device_system_crosststamp(
> > >                       struct system_time_snapshot *history,
> > >                       struct system_device_crosststamp *xtstamp);
> > >
> > > +/*
> > > + * Obtain current monotonic clock and its counter value  */ extern
> > > +void get_current_counterval(struct system_counterval_t *sc);
> > > +
> > >  /*
> > >   * Simultaneously snapshot realtime and monotonic raw clocks
> > >   */
> > > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > > index
> > > d911c8470149..de689bbd3808 100644
> > > --- a/kernel/time/timekeeping.c
> > > +++ b/kernel/time/timekeeping.c
> > > @@ -1098,6 +1098,18 @@ static bool cycle_between(u64 before, u64
> > > test,
> > > u64 after)
> > >       return false;
> > >  }
> > >
> > > +/**
> > > + * get_current_counterval - Snapshot the current clocksource and
> > > +counter
> > > value
> > > + * @sc:      Pointer to a struct containing the current clocksource and its
> > > value
> > > + */
> > > +void get_current_counterval(struct system_counterval_t *sc) {
> > > +     struct timekeeper *tk = &tk_core.timekeeper;
> > > +
> > > +     sc->cs = READ_ONCE(tk->tkr_mono.clock);
> > > +     sc->cycles = sc->cs->read(sc->cs); }
> > > +
> > >  /**
> > >   * get_device_system_crosststamp - Synchronously capture
> > > system/device timestamp
> > >   * @get_time_fn:     Callback to get simultaneous device time and
> > >
> > > which should do the right thing.
> > >
> > It is a good news for me. These code is indeed what I need!  So what's
> > your plan about this patch?  Is there any problem with you if I
> > include these code into my patch ?
>
> Just add this patch as part of your series (I'll try to write an actual commit log
> for that).

Very kind of you!
>
> [...]
>
> > > > > Other questions: how does this works with VM migration?
> > > > > Specially when moving from a hypervisor that supports the
> > > > > feature to one that
> > > doesn't?
> > > > >
> > > > I think it won't solve the problem generated by VM migration and
> > > > only for VMs in a single machine.  Ptp_kvm only works for VMs in
> > > > the same machine.  But using ptp (not ptp_kvm) clock, all the
> > > > machines in a low latency network environment can keep time sync
> > > > in high precision, Then VMs move from one machine to another will
> > > > obtain a high precision time sync.
> > >
> > > That's a problem. Migration must be possible from one host to
> > > another, even if that means temporarily loosing some (or a lot of)
> > > precision. The service must be discoverable from userspace on the
> > > host so that the MVV can decie whether a migration is possible or not.
> > >
> > Don't worry, things will be not that bad.  ptp_kvm will not trouble
> > the VM migration. This ptp_kvm is one clocksource of the clock pool
> > for chrony. Chrony will choose the highest precision clock from the
> > pool. If host does not support ptp_kvm, the ptp_kvm will not be chosen
> > as the clocksouce of chrony.  We have roughly the same logic of
> > implementation of ptp_kvm with x86, and ptp_kvm works well in x86.  so
> > I think that will be the case for arm64.
> >
> > Maybe I miss your point, I have no idea of MVV and can't get related
> > info from google.  Also I'm not clear of your last words of how to
> > decide VM migration is possible?
>
> Sorry. s/MVV/VMM/. Basically userspace, such as QEMU.
>
> Here's an example: The guest runs on a PTP aware host, starts using the PTP
> service and uses HVC calls to get its clock. We now migrate the guest to a non
> PTP-aware host. The hypercalls are now going to fail unexpectedly. Is that
> something that is acceptable? I don't think it is. Once you've allowed a guest
> to use a service, this service should be preserved. I'd be more confident if we
> gave to userspace the indication that the hypervisor supports PTP. Userspace
> can then decide whether to perform migration or not.
>

It's really a point we should consider. let me check the behavior of chrony in this scenario first.

Thanks
Jianyong Wu

> Thanks,
>
>       M.
>
> --
> Jazz is not dead, it just smells funny.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

RE: [RFC PATCH 3/3] Enable ptp_kvm for arm64

[Jianyong Wu (Arm Technology China)]

Hi Marc,

I think there are three points for the migration issue of ptp_kvm, where a VM using ptp_kvm migrates to a host without ptp_kvm support.

First: how does it impact the VM having migrated?
I run a VM with ptp_kvm support in guest but not support in host. the ptp0 will return 0 when get time from it which can't pass the check
of chrony, then chrony will choose another clocksource.  From this point, VM will only get lost in precision of time sync.

Second: how to check the failure of the ptp kvm service
when there is no ptp kvm service, hypercall will go into default ops, so we can check the return value which can inform us the failure.

Third: how to inform VMM
There is ioctl cmd call "KVM_CHECK_EXTENSION" in kvm, which may do that thing. Accordingly, qemu should be offered the support which will block us.
We can try to add this support in kvm but we are not sure the response from qemu side.

WDYT?

Jianyong Wu
Thanks

> -----Original Message-----
> From: Jianyong Wu (Arm Technology China)
> Sent: Tuesday, September 10, 2019 6:29 PM
> To: Marc Zyngier <maz@kernel.org>
> Cc: netdev@vger.kernel.org; pbonzini@redhat.com;
> sean.j.christopherson@intel.com; richardcochran@gmail.com; Mark Rutland
> <Mark.Rutland@arm.com>; Will Deacon <Will.Deacon@arm.com>; Suzuki
> Poulose <Suzuki.Poulose@arm.com>; linux-kernel@vger.kernel.org; Steve
> Capper <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China)
> <Kaly.Xin@arm.com>; Justin He (Arm Technology China)
> <Justin.He@arm.com>
> Subject: RE: [RFC PATCH 3/3] Enable ptp_kvm for arm64
>
> Hi Marc,
>
> > -----Original Message-----
> > From: Marc Zyngier <maz@kernel.org>
> > Sent: Monday, September 9, 2019 7:25 PM
> > To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>
> > Cc: netdev@vger.kernel.org; pbonzini@redhat.com;
> > sean.j.christopherson@intel.com; richardcochran@gmail.com; Mark
> > Rutland <Mark.Rutland@arm.com>; Will Deacon <Will.Deacon@arm.com>;
> > Suzuki Poulose <Suzuki.Poulose@arm.com>; linux-kernel@vger.kernel.org;
> > Steve Capper <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China)
> > <Kaly.Xin@arm.com>; Justin He (Arm Technology China)
> > <Justin.He@arm.com>
> > Subject: Re: [RFC PATCH 3/3] Enable ptp_kvm for arm64
> >
> >
> > > > > > Other questions: how does this works with VM migration?
> > > > > > Specially when moving from a hypervisor that supports the
> > > > > > feature to one that
> > > > doesn't?
> > > > > >
> > > > > I think it won't solve the problem generated by VM migration and
> > > > > only for VMs in a single machine.  Ptp_kvm only works for VMs in
> > > > > the same machine.  But using ptp (not ptp_kvm) clock, all the
> > > > > machines in a low latency network environment can keep time sync
> > > > > in high precision, Then VMs move from one machine to another
> > > > > will obtain a high precision time sync.
> > > >
> > > > That's a problem. Migration must be possible from one host to
> > > > another, even if that means temporarily loosing some (or a lot of)
> > > > precision. The service must be discoverable from userspace on the
> > > > host so that the MVV can decie whether a migration is possible or not.
> > > >
> > > Don't worry, things will be not that bad.  ptp_kvm will not trouble
> > > the VM migration. This ptp_kvm is one clocksource of the clock pool
> > > for chrony. Chrony will choose the highest precision clock from the
> > > pool. If host does not support ptp_kvm, the ptp_kvm will not be
> > > chosen as the clocksouce of chrony.  We have roughly the same logic
> > > of implementation of ptp_kvm with x86, and ptp_kvm works well in
> > > x86.  so I think that will be the case for arm64.
> > >
> > > Maybe I miss your point, I have no idea of MVV and can't get related
> > > info from google.  Also I'm not clear of your last words of how to
> > > decide VM migration is possible?
> >
> > Sorry. s/MVV/VMM/. Basically userspace, such as QEMU.
> >
> > Here's an example: The guest runs on a PTP aware host, starts using
> > the PTP service and uses HVC calls to get its clock. We now migrate
> > the guest to a non PTP-aware host. The hypercalls are now going to
> > fail unexpectedly. Is that something that is acceptable? I don't think
> > it is. Once you've allowed a guest to use a service, this service
> > should be preserved. I'd be more confident if we gave to userspace the
> > indication that the hypervisor supports PTP. Userspace can then decide
> whether to perform migration or not.
> >
>
> It's really a point we should consider. let me check the behavior of chrony in
> this scenario first.
>
> Thanks
> Jianyong Wu
>
> > Thanks,
> >
> >     M.
> >
> > --
> > Jazz is not dead, it just smells funny.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [RFC PATCH 3/3] Enable ptp_kvm for arm64

[Marc Zyngier]

On Wed, 11 Sep 2019 11:06:18 +0100,
"Jianyong Wu (Arm Technology China)" <Jianyong.Wu@arm.com> wrote:
> 
> Hi Marc,
> 
> I think there are three points for the migration issue of ptp_kvm,
> where a VM using ptp_kvm migrates to a host without ptp_kvm support.
> 
> First: how does it impact the VM having migrated?
> I run a VM with ptp_kvm support in guest but not support in
> host. the ptp0 will return 0 when get time from it which can't pass
> the check of chrony, then chrony will choose another clocksource.
> From this point, VM will only get lost in precision of time sync.

"only" is a bit of an understatement. Once the guest has started
relying on a service, it seems rather harsh to pretend this service
doesn't exist anymore. It could well be that the VM cannot perform its
function if the precision is not good enough.

The analogy is the Spectre-v2 mitigation, which is implemented as a
hypercall. Nothing will break if you migrate to a host that doesn't
support the mitigation, but the guest will now be unsafe. Is that
acceptable? the answer is of course "no".

> Second: how to check the failure of the ptp kvm service
> when there is no ptp kvm service, hypercall will go into default
> ops, so we can check the return value which can inform us the
> failure.

Sure. But that's still an issue. The VM relied on the service, and the
service isn't available anymore.

> Third: how to inform VMM
> There is ioctl cmd call "KVM_CHECK_EXTENSION" in kvm, which may do
> that thing. Accordingly, qemu should be offered the support which
> will block us.  We can try to add this support in kvm but we are not
> sure the response from qemu side.

It doesn't matter whether QEMU implements that check or . The
important thing is that we give userspace a way to check this for
this, and having a capability that can be checked against is probably
the right thing to do.

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.

RE: [RFC PATCH 3/3] Enable ptp_kvm for arm64

[Jianyong Wu (Arm Technology China)]

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Wednesday, September 11, 2019 7:31 PM
> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>
> Cc: netdev@vger.kernel.org; pbonzini@redhat.com;
> sean.j.christopherson@intel.com; richardcochran@gmail.com; Mark Rutland
> <Mark.Rutland@arm.com>; Will Deacon <Will.Deacon@arm.com>; Suzuki
> Poulose <Suzuki.Poulose@arm.com>; linux-kernel@vger.kernel.org; Steve
> Capper <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China)
> <Kaly.Xin@arm.com>; Justin He (Arm Technology China)
> <Justin.He@arm.com>
> Subject: Re: [RFC PATCH 3/3] Enable ptp_kvm for arm64
>
> On Wed, 11 Sep 2019 11:06:18 +0100,
> "Jianyong Wu (Arm Technology China)" <Jianyong.Wu@arm.com> wrote:
> >
> > Hi Marc,
> >
> > I think there are three points for the migration issue of ptp_kvm,
> > where a VM using ptp_kvm migrates to a host without ptp_kvm support.
> >
> > First: how does it impact the VM having migrated?
> > I run a VM with ptp_kvm support in guest but not support in host. the
> > ptp0 will return 0 when get time from it which can't pass the check of
> > chrony, then chrony will choose another clocksource.
> > From this point, VM will only get lost in precision of time sync.
>
> "only" is a bit of an understatement. Once the guest has started relying on a
> service, it seems rather harsh to pretend this service doesn't exist anymore.
> It could well be that the VM cannot perform its function if the precision is not
> good enough.
>
> The analogy is the Spectre-v2 mitigation, which is implemented as a hypercall.
> Nothing will break if you migrate to a host that doesn't support the mitigation,
> but the guest will now be unsafe. Is that acceptable? the answer is of course
> "no".
>
> > Second: how to check the failure of the ptp kvm service when there is
> > no ptp kvm service, hypercall will go into default ops, so we can
> > check the return value which can inform us the failure.
>
> Sure. But that's still an issue. The VM relied on the service, and the service
> isn't available anymore.
>
> > Third: how to inform VMM
> > There is ioctl cmd call "KVM_CHECK_EXTENSION" in kvm, which may do
> > that thing. Accordingly, qemu should be offered the support which will
> > block us.  We can try to add this support in kvm but we are not sure
> > the response from qemu side.
>
> It doesn't matter whether QEMU implements that check or . The important
> thing is that we give userspace a way to check this for this, and having a
> capability that can be checked against is probably the right thing to do.

Ok, I agree.
Adding a new capability item under "KVM_CHECK_EXTENSION" in kvm for ptp_kvm will do and Using ioctl in userspace can check if the ptp service is available.
I will append this patch in this patch serial.

Thanks
Jianyong Wu

>
> Thanks,
>
>       M.
>
> --
> Jazz is not dead, it just smells funny.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.