[RFC PATCH v9 0/8] Enable ptp_kvm for arm64

[RFC PATCH v9 0/8] Enable ptp_kvm for arm64

[Jianyong Wu]

kvm ptp targets to provide high precision time sync between guest
and host in virtualization environment. Here, we enable kvm ptp
for arm64.
This patch set base on [1][2][3]

change log:

from v8 to v9:
	(1) move ptp_kvm.h to driver/ptp/
	(2) replace license declaration of ptp_kvm.h the same with other
header files in the same directory.

from v7 to v8:
        (1) separate adding clocksource id for arm_arch_counter as a
single patch.
        (2) update commit message for patch 4/8.
        (3) refine patch 7/8 and patch 8/8 to make them more independent.

from v6 to v7:
        (1) include the omitted clocksource_id.h in last version.
        (2) reorder the header file in patch.
        (3) refine some words in commit message to make it more impersonal.

from v5 to v6:
        (1) apply Mark's patch[4] to get SMCCC conduit.
        (2) add mechanism to recognize current clocksource by add
clocksouce_id value into struct clocksource instead of method in patch-v5.
        (3) rename kvm_arch_ptp_get_clock_fn into
kvm_arch_ptp_get_crosststamp.

from v4 to v5:
        (1) remove hvc delay compensasion as it should leave to userspace.
        (2) check current clocksource in hvc call service.
        (3) expose current clocksource by adding it to
system_time_snapshot.
        (4) add helper to check if clocksource is arm_arch_counter.
        (5) rename kvm_ptp.c to ptp_kvm_common.c

from v3 to v4:
        (1) fix clocksource of ptp_kvm to arch_sys_counter.
        (2) move kvm_arch_ptp_get_clock_fn into arm_arch_timer.c
        (3) subtract cntvoff before return cycles from host.
        (4) use ktime_get_snapshot instead of getnstimeofday and
get_current_counterval to return time and counter value.
        (5) split ktime and counter into two 32-bit block respectively
to avoid Y2038-safe issue.
        (6) set time compensation to device time as half of the delay of
hvc call.
        (7) add ARM_ARCH_TIMER as dependency of ptp_kvm for
arm64.

from v2 to v3:
        (1) fix some issues in commit log.
        (2) add some receivers in send list.

from v1 to v2:
        (1) move arch-specific code from arch/ to driver/ptp/
        (2) offer mechanism to inform userspace if ptp_kvm service is
available.
        (3) separate ptp_kvm code for arm64 into hypervisor part and
guest part.
        (4) add API to expose monotonic clock and counter value.
        (5) refine code: remove no necessary part and reconsitution.

[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
[4]https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/
commit/?h=for-next/smccc-conduit-cleanup&id=6b7fe77c334ae59fed9500140e08f4f896b36871

Jianyong Wu (6):
  psci: let arm_smccc_1_1_invoke available by modules
  ptp: Reorganize ptp_kvm modules to make it arch-independent.
  clocksource: Add clocksource id for arm arch counter
  psci: Add hvc call service for ptp_kvm.
  ptp: arm64: Enable ptp_kvm for arm64
  kvm: arm64: Add capability check extension for ptp_kvm

Mark Rutland (1):
  arm/arm64: smccc/psci: add arm_smccc_1_1_get_conduit()

Thomas Gleixner (1):
  time: Add mechanism to recognize clocksource in time_get_snapshot

 drivers/clocksource/arm_arch_timer.c        | 24 ++++++
 drivers/firmware/psci/psci.c                | 16 ++++
 drivers/ptp/Kconfig                         |  2 +-
 drivers/ptp/Makefile                        |  1 +
 drivers/ptp/ptp_kvm.h                       | 11 +++
 drivers/ptp/ptp_kvm_arm64.c                 | 53 +++++++++++++
 drivers/ptp/{ptp_kvm.c => ptp_kvm_common.c} | 77 +++++-------------
 drivers/ptp/ptp_kvm_x86.c                   | 87 +++++++++++++++++++++
 include/linux/arm-smccc.h                   | 30 ++++++-
 include/linux/clocksource.h                 |  6 ++
 include/linux/clocksource_ids.h             | 13 +++
 include/linux/timekeeping.h                 | 12 +--
 include/uapi/linux/kvm.h                    |  1 +
 kernel/time/clocksource.c                   |  3 +
 kernel/time/timekeeping.c                   |  1 +
 virt/kvm/arm/arm.c                          |  1 +
 virt/kvm/arm/psci.c                         | 22 ++++++
 17 files changed, 294 insertions(+), 66 deletions(-)
 create mode 100644 drivers/ptp/ptp_kvm.h
 create mode 100644 drivers/ptp/ptp_kvm_arm64.c
 rename drivers/ptp/{ptp_kvm.c => ptp_kvm_common.c} (63%)
 create mode 100644 drivers/ptp/ptp_kvm_x86.c
 create mode 100644 include/linux/clocksource_ids.h

-- 
2.17.1

[RFC PATCH v9 1/8] arm/arm64: smccc/psci: add arm_smccc_1_1_get_conduit()

[Jianyong Wu]

From: Mark Rutland <mark.rutland@arm.com>

SMCCC callers are currently amassing a collection of enums for the SMCCC
conduit, and are having to dig into the PSCI driver's internals in order
to figure out what to do.

Let's clean this up, with common SMCCC_CONDUIT_* definitions, and an
arm_smccc_1_1_get_conduit() helper that abstracts the PSCI driver's
internal state.

We can kill off the PSCI_CONDUIT_* definitions once we've migrated users
over to the new interface.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 drivers/firmware/psci/psci.c | 15 +++++++++++++++
 include/linux/arm-smccc.h    | 16 ++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index f82ccd39a913..5f31f1bea1af 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -57,6 +57,21 @@ struct psci_operations psci_ops = {
 	.smccc_version = SMCCC_VERSION_1_0,
 };
 
+enum arm_smccc_conduit arm_smccc_1_1_get_conduit(void)
+{
+	if (psci_ops.smccc_version < SMCCC_VERSION_1_1)
+		return SMCCC_CONDUIT_NONE;
+
+	switch (psci_ops.conduit) {
+	case PSCI_CONDUIT_SMC:
+		return SMCCC_CONDUIT_SMC;
+	case PSCI_CONDUIT_HVC:
+		return SMCCC_CONDUIT_HVC;
+	default:
+		return SMCCC_CONDUIT_NONE;
+	}
+}
+
 typedef unsigned long (psci_fn)(unsigned long, unsigned long,
 				unsigned long, unsigned long);
 static psci_fn *invoke_psci_fn;
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 552cbd49abe8..701fa1da6b4a 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -107,6 +107,22 @@
 
 #include <linux/linkage.h>
 #include <linux/types.h>
+
+enum arm_smccc_conduit {
+	SMCCC_CONDUIT_NONE,
+	SMCCC_CONDUIT_SMC,
+	SMCCC_CONDUIT_HVC,
+};
+
+/**
+ * arm_smccc_1_1_get_conduit()
+ *
+ * Returns the conduit to be used for SMCCCv1.1 or later.
+ *
+ * When SMCCCv1.1 is not present, returns SMCCC_CONDUIT_NONE.
+ */
+enum arm_smccc_conduit arm_smccc_1_1_get_conduit(void);
+
 /**
  * struct arm_smccc_res - Result from SMC/HVC call
  * @a0-a3 result values from registers 0 to 3
-- 
2.17.1

[RFC PATCH v9 2/8] psci: let arm_smccc_1_1_invoke available by modules

[Jianyong Wu]

Export arm_smccc_1_1_get_conduit and apply it to get conduit
in arm_smccc_1_1_invoke, then modules can use this arm_smccc_1_1_invoke.

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

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index 5f31f1bea1af..cf509171422c 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -71,6 +71,7 @@ enum arm_smccc_conduit arm_smccc_1_1_get_conduit(void)
 		return SMCCC_CONDUIT_NONE;
 	}
 }
+EXPORT_SYMBOL(arm_smccc_1_1_get_conduit);
 
 typedef unsigned long (psci_fn)(unsigned long, unsigned long,
 				unsigned long, unsigned long);
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 701fa1da6b4a..6f82c87308ed 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -373,7 +373,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 = arm_smccc_1_1_get_conduit();		\
 		switch (method) {					\
 		case PSCI_CONDUIT_HVC:					\
 			arm_smccc_1_1_hvc(__VA_ARGS__);			\
-- 
2.17.1

[RFC PATCH v9 3/8] ptp: 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 new arch related file in the same directory.

Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
---
 drivers/ptp/Makefile                        |  1 +
 drivers/ptp/ptp_kvm.h                       | 11 +++
 drivers/ptp/{ptp_kvm.c => ptp_kvm_common.c} | 78 +++++-------------
 drivers/ptp/ptp_kvm_x86.c                   | 87 +++++++++++++++++++++
 4 files changed, 118 insertions(+), 59 deletions(-)
 create mode 100644 drivers/ptp/ptp_kvm.h
 rename drivers/ptp/{ptp_kvm.c => ptp_kvm_common.c} (63%)
 create mode 100644 drivers/ptp/ptp_kvm_x86.c

diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile
index 677d1d178a3e..3b7554f56ad9 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				:= ptp_kvm_$(ARCH).o ptp_kvm_common.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.h b/drivers/ptp/ptp_kvm.h
new file mode 100644
index 000000000000..4bf1802bbeb8
--- /dev/null
+++ b/drivers/ptp/ptp_kvm.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Virtual PTP 1588 clock for use with KVM guests
+ *
+ * Copyright (C) 2017 Red Hat Inc.
+ */
+
+int kvm_arch_ptp_init(void);
+int kvm_arch_ptp_get_clock(struct timespec64 *ts);
+int kvm_arch_ptp_get_crosststamp(unsigned long *cycle,
+		struct timespec64 *tspec, void *cs);
diff --git a/drivers/ptp/ptp_kvm.c b/drivers/ptp/ptp_kvm_common.c
similarity index 63%
rename from drivers/ptp/ptp_kvm.c
rename to drivers/ptp/ptp_kvm_common.c
index fc7d0b77e118..60442f70d3fc 100644
--- a/drivers/ptp/ptp_kvm.c
+++ b/drivers/ptp/ptp_kvm_common.c
@@ -8,15 +8,16 @@
 #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 <linux/ptp_clock_kernel.h>
 
+#include "ptp_kvm.h"
+
 struct kvm_ptp_clock {
 	struct ptp_clock *ptp_clock;
 	struct ptp_clock_info caps;
@@ -24,56 +25,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_crosststamp(&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 +90,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 +136,11 @@ static void __exit ptp_kvm_exit(void)
 
 static int __init ptp_kvm_init(void)
 {
-	long ret;
-
-	if (!kvm_para_available())
-		return -ENODEV;
+	int ret;
 
-	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;
+	ret = kvm_arch_ptp_init();
+	if (ret)
+		return -EOPNOTSUPP;
 
 	kvm_ptp_clock.caps = ptp_kvm_caps;
 
diff --git a/drivers/ptp/ptp_kvm_x86.c b/drivers/ptp/ptp_kvm_x86.c
new file mode 100644
index 000000000000..6c891d7299c6
--- /dev/null
+++ b/drivers/ptp/ptp_kvm_x86.c
@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Virtual PTP 1588 clock for use with KVM guests
+ *
+ * Copyright (C) 2017 Red Hat Inc.
+ */
+
+#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_crosststamp(unsigned 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;
+}
-- 
2.17.1

[RFC PATCH v9 4/8] time: Add mechanism to recognize clocksource in time_get_snapshot

[Jianyong Wu]

From: Thomas Gleixner <tglx@linutronix.de>

System time snapshots are not conveying information about the current
clocksource which was used, but callers like the PTP KVM guest
implementation have the requirement to evaluate the clocksource type to
select the appropriate mechanism.

Introduce a clocksource id field in struct clocksource which is by default
set to CSID_GENERIC (0). Clocksource implementations can set that field to
a value which allows to identify the clocksource.

Store the clocksource id of the current clocksource in the
system_time_snapshot so callers can evaluate which clocksource was used to
take the snapshot and act accordingly.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
---
 include/linux/clocksource.h     |  6 ++++++
 include/linux/clocksource_ids.h | 12 ++++++++++++
 include/linux/timekeeping.h     | 12 +++++++-----
 kernel/time/clocksource.c       |  3 +++
 kernel/time/timekeeping.c       |  1 +
 5 files changed, 29 insertions(+), 5 deletions(-)
 create mode 100644 include/linux/clocksource_ids.h

diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index b21db536fd52..96e85b6f9ca0 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -17,6 +17,7 @@
 #include <linux/timer.h>
 #include <linux/init.h>
 #include <linux/of.h>
+#include <linux/clocksource_ids.h>
 #include <asm/div64.h>
 #include <asm/io.h>
 
@@ -49,6 +50,10 @@ struct module;
  *			400-499: Perfect
  *				The ideal clocksource. A must-use where
  *				available.
+ * @id:			Defaults to CSID_GENERIC. The id value is captured
+ *			in certain snapshot functions to allow callers to
+ *			validate the clocksource from which the snapshot was
+ *			taken.
  * @read:		returns a cycle value, passes clocksource as argument
  * @enable:		optional function to enable the clocksource
  * @disable:		optional function to disable the clocksource
@@ -91,6 +96,7 @@ struct clocksource {
 	const char *name;
 	struct list_head list;
 	int rating;
+	enum clocksource_ids id;
 	int (*enable)(struct clocksource *cs);
 	void (*disable)(struct clocksource *cs);
 	unsigned long flags;
diff --git a/include/linux/clocksource_ids.h b/include/linux/clocksource_ids.h
new file mode 100644
index 000000000000..5595ab843c3d
--- /dev/null
+++ b/include/linux/clocksource_ids.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_CLOCKSOURCE_IDS_H
+#define _LINUX_CLOCKSOURCE_IDS_H
+
+/* Enum to give clocksources a unique identifier */
+enum clocksource_ids {
+	CSID_GENERIC		= 0,
+	CSID_MAX,
+};
+
+#endif
+
diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index a8ab0f143ac4..ecce56269a7e 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -2,6 +2,7 @@
 #ifndef _LINUX_TIMEKEEPING_H
 #define _LINUX_TIMEKEEPING_H
 
+#include <linux/clocksource_ids.h>
 #include <linux/errno.h>
 
 /* Included from linux/ktime.h */
@@ -204,11 +205,12 @@ extern void timekeeping_inject_sleeptime64(const struct timespec64 *delta);
  * @cs_was_changed_seq:	The sequence number of clocksource change events
  */
 struct system_time_snapshot {
-	u64		cycles;
-	ktime_t		real;
-	ktime_t		raw;
-	unsigned int	clock_was_set_seq;
-	u8		cs_was_changed_seq;
+	u64			cycles;
+	ktime_t			real;
+	ktime_t			raw;
+	enum clocksource_ids	cs_id;
+	unsigned int		clock_was_set_seq;
+	u8			cs_was_changed_seq;
 };
 
 /*
diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index 3bcc19ceb073..26a3add61771 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -921,6 +921,9 @@ int __clocksource_register_scale(struct clocksource *cs, u32 scale, u32 freq)
 
 	clocksource_arch_init(cs);
 
+	if (WARN_ON_ONCE((unsigned int)cs->id >= CSID_MAX))
+		cs->id = CSID_GENERIC;
+
 	/* Initialize mult/shift and max_idle_ns */
 	__clocksource_update_freq_scale(cs, scale, freq);
 
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 44b726bab4bd..88078cff7fe2 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -974,6 +974,7 @@ void ktime_get_snapshot(struct system_time_snapshot *systime_snapshot)
 	do {
 		seq = read_seqcount_begin(&tk_core.seq);
 		now = tk_clock_read(&tk->tkr_mono);
+		systime_snapshot->cs_id = tk->tkr_mono.clock->id;
 		systime_snapshot->cs_was_changed_seq = tk->cs_was_changed_seq;
 		systime_snapshot->clock_was_set_seq = tk->clock_was_set_seq;
 		base_real = ktime_add(tk->tkr_mono.base,
-- 
2.17.1

[RFC PATCH v9 5/8] clocksource: Add clocksource id for arm arch counter

[Jianyong Wu]

Add clocksource id for arm arch counter to let it be identified easily and
elegantly in ptp_kvm implementation for arm.

Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
---
 drivers/clocksource/arm_arch_timer.c | 2 ++
 include/linux/clocksource_ids.h      | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 07e57a49d1e8..277846decd33 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -16,6 +16,7 @@
 #include <linux/cpu_pm.h>
 #include <linux/clockchips.h>
 #include <linux/clocksource.h>
+#include <linux/clocksource_ids.h>
 #include <linux/interrupt.h>
 #include <linux/of_irq.h>
 #include <linux/of_address.h>
@@ -187,6 +188,7 @@ static u64 arch_counter_read_cc(const struct cyclecounter *cc)
 
 static struct clocksource clocksource_counter = {
 	.name	= "arch_sys_counter",
+	.id	= CSID_ARM_ARCH_COUNTER,
 	.rating	= 400,
 	.read	= arch_counter_read,
 	.mask	= CLOCKSOURCE_MASK(56),
diff --git a/include/linux/clocksource_ids.h b/include/linux/clocksource_ids.h
index 5595ab843c3d..93bec8426c44 100644
--- a/include/linux/clocksource_ids.h
+++ b/include/linux/clocksource_ids.h
@@ -5,6 +5,7 @@
 /* Enum to give clocksources a unique identifier */
 enum clocksource_ids {
 	CSID_GENERIC		= 0,
+	CSID_ARM_ARCH_COUNTER,
 	CSID_MAX,
 };
 
-- 
2.17.1

[RFC PATCH v9 6/8] psci: Add hvc call service for ptp_kvm.

[Jianyong Wu]

ptp_kvm modules will call hvc to get this service.
The service offers real time and counter cycle of host for guest.

Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
---
 include/linux/arm-smccc.h | 12 ++++++++++++
 virt/kvm/arm/psci.c       | 22 ++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 6f82c87308ed..aafb6bac167d 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
 
@@ -103,6 +104,17 @@
 			   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__
 
 #include <linux/linkage.h>
diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
index 0debf49bf259..682d892d6717 100644
--- a/virt/kvm/arm/psci.c
+++ b/virt/kvm/arm/psci.c
@@ -9,6 +9,7 @@
 #include <linux/kvm_host.h>
 #include <linux/uaccess.h>
 #include <linux/wait.h>
+#include <linux/clocksource_ids.h>
 
 #include <asm/cputype.h>
 #include <asm/kvm_emulate.h>
@@ -389,6 +390,8 @@ static int kvm_psci_call(struct kvm_vcpu *vcpu)
 
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 {
+	struct system_time_snapshot systime_snapshot;
+	u64 cycles;
 	u32 func_id = smccc_get_function(vcpu);
 	u32 val[4] = {};
 	u32 option;
@@ -431,6 +434,25 @@ 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 high 32-bit host ktime;
+	 * reg1 stores low 32-bit host ktime;
+	 * reg2 stores high 32-bit difference of host cycles and cntvoff;
+	 * reg3 stores low 32-bit difference of host cycles and cntvoff.
+	 */
+	case ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID:
+		ktime_get_snapshot(&systime_snapshot);
+		if (systime_snapshot.cs_id != CSID_ARM_ARCH_COUNTER)
+			return kvm_psci_call(vcpu);
+		val[0] = systime_snapshot.real >> 32;
+		val[1] = systime_snapshot.real << 32 >> 32;
+		cycles = systime_snapshot.cycles -
+			 vcpu_vtimer(vcpu)->cntvoff;
+		val[2] = cycles >> 32;
+		val[3] = cycles << 32 >> 32;
+		break;
 	default:
 		return kvm_psci_call(vcpu);
 	}
-- 
2.17.1

[RFC PATCH v9 7/8] ptp: arm64: 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>
---
 drivers/clocksource/arm_arch_timer.c | 22 ++++++++++++
 drivers/ptp/Kconfig                  |  2 +-
 drivers/ptp/ptp_kvm_arm64.c          | 53 ++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+), 1 deletion(-)
 create mode 100644 drivers/ptp/ptp_kvm_arm64.c

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 277846decd33..72260b66f02e 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -1636,3 +1636,25 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
 }
 TIMER_ACPI_DECLARE(arch_timer, ACPI_SIG_GTDT, arch_timer_acpi_init);
 #endif
+
+#if IS_ENABLED(CONFIG_PTP_1588_CLOCK_KVM)
+#include <linux/arm-smccc.h>
+int kvm_arch_ptp_get_crosststamp(unsigned long *cycle, struct timespec64 *ts,
+			      struct clocksource **cs)
+{
+	struct arm_smccc_res hvc_res;
+	ktime_t ktime_overall;
+
+	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID, &hvc_res);
+	if ((long)(hvc_res.a0) < 0)
+		return -EOPNOTSUPP;
+
+	ktime_overall = hvc_res.a0 << 32 | hvc_res.a1;
+	*ts = ktime_to_timespec64(ktime_overall);
+	*cycle = hvc_res.a2 << 32 | hvc_res.a3;
+	*cs = &clocksource_counter;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(kvm_arch_ptp_get_crosststamp);
+#endif
diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index 9b8fee5178e8..3c31ff8eb05f 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 && ARM_ARCH_TIMER
 	default y
 	help
 	  This driver adds support for using kvm infrastructure as a PTP
diff --git a/drivers/ptp/ptp_kvm_arm64.c b/drivers/ptp/ptp_kvm_arm64.c
new file mode 100644
index 000000000000..f3f957117865
--- /dev/null
+++ b/drivers/ptp/ptp_kvm_arm64.c
@@ -0,0 +1,53 @@
+// 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 <linux/kernel.h>
+#include <linux/err.h>
+#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>
+
+int kvm_arch_ptp_init(void)
+{
+	struct arm_smccc_res hvc_res;
+
+	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
+			     &hvc_res);
+	if ((long)(hvc_res.a0) < 0)
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+
+int kvm_arch_ptp_get_clock_generic(struct timespec64 *ts,
+				   struct arm_smccc_res *hvc_res)
+{
+	ktime_t ktime_overall;
+
+	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
+				  hvc_res);
+	if ((long)(hvc_res->a0) < 0)
+		return -EOPNOTSUPP;
+
+	ktime_overall = hvc_res->a0 << 32 | hvc_res->a1;
+	*ts = ktime_to_timespec64(ktime_overall);
+
+	return 0;
+}
+
+int kvm_arch_ptp_get_clock(struct timespec64 *ts)
+{
+	struct arm_smccc_res hvc_res;
+
+	kvm_arch_ptp_get_clock_generic(ts, &hvc_res);
+
+	return 0;
+}
-- 
2.17.1

[RFC PATCH v9 8/8] kvm: arm64: Add capability check extension for ptp_kvm

[Jianyong Wu]

Let userspace check if there is kvm ptp service in host.
before VMs migrate to a another host, VMM may check if this
cap is available to determine the migration behaviour.

Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
Suggested-by: Marc Zyngier <maz@kernel.org>
---
 include/uapi/linux/kvm.h | 1 +
 virt/kvm/arm/arm.c       | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 2fe12b40d503..a0bff6002bd9 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -993,6 +993,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_ARM_SVE 170
 #define KVM_CAP_ARM_PTRAUTH_ADDRESS 171
 #define KVM_CAP_ARM_PTRAUTH_GENERIC 172
+#define KVM_CAP_ARM_KVM_PTP 173
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index bd5c55916d0d..80999985160b 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -201,6 +201,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_MP_STATE:
 	case KVM_CAP_IMMEDIATE_EXIT:
 	case KVM_CAP_VCPU_EVENTS:
+	case KVM_CAP_ARM_KVM_PTP:
 		r = 1;
 		break;
 	case KVM_CAP_ARM_SET_DEVICE_ADDR:
-- 
2.17.1

RE: [RFC PATCH v9 0/8] Enable ptp_kvm for arm64

[Jianyong Wu]

Ping ...
Any comments to this patch set?

> -----Original Message-----
> From: Jianyong Wu <jianyong.wu@arm.com>
> Sent: Tuesday, December 10, 2019 11:40 AM
> To: netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
> tglx@linutronix.de; pbonzini@redhat.com; sean.j.christopherson@intel.com;
> maz@kernel.org; richardcochran@gmail.com; Mark Rutland
> <Mark.Rutland@arm.com>; will@kernel.org; Suzuki Poulose
> <Suzuki.Poulose@arm.com>; Steven Price <Steven.Price@arm.com>
> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> kvmarm@lists.cs.columbia.edu; kvm@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>; Jianyong Wu (Arm Technology China)
> <Jianyong.Wu@arm.com>; nd <nd@arm.com>
> Subject: [RFC PATCH v9 0/8] Enable ptp_kvm for arm64
> 
> kvm ptp targets to provide high precision time sync between guest and host
> in virtualization environment. Here, we enable kvm ptp for arm64.
> This patch set base on [1][2][3]
> 
> change log:
> 
> from v8 to v9:
> 	(1) move ptp_kvm.h to driver/ptp/
> 	(2) replace license declaration of ptp_kvm.h the same with other
> header files in the same directory.
> 
> from v7 to v8:
>         (1) separate adding clocksource id for arm_arch_counter as a single patch.
>         (2) update commit message for patch 4/8.
>         (3) refine patch 7/8 and patch 8/8 to make them more independent.
> 
> from v6 to v7:
>         (1) include the omitted clocksource_id.h in last version.
>         (2) reorder the header file in patch.
>         (3) refine some words in commit message to make it more impersonal.
> 
> from v5 to v6:
>         (1) apply Mark's patch[4] to get SMCCC conduit.
>         (2) add mechanism to recognize current clocksource by add
> clocksouce_id value into struct clocksource instead of method in patch-v5.
>         (3) rename kvm_arch_ptp_get_clock_fn into
> kvm_arch_ptp_get_crosststamp.
> 
> from v4 to v5:
>         (1) remove hvc delay compensasion as it should leave to userspace.
>         (2) check current clocksource in hvc call service.
>         (3) expose current clocksource by adding it to system_time_snapshot.
>         (4) add helper to check if clocksource is arm_arch_counter.
>         (5) rename kvm_ptp.c to ptp_kvm_common.c
> 
> from v3 to v4:
>         (1) fix clocksource of ptp_kvm to arch_sys_counter.
>         (2) move kvm_arch_ptp_get_clock_fn into arm_arch_timer.c
>         (3) subtract cntvoff before return cycles from host.
>         (4) use ktime_get_snapshot instead of getnstimeofday and
> get_current_counterval to return time and counter value.
>         (5) split ktime and counter into two 32-bit block respectively to avoid
> Y2038-safe issue.
>         (6) set time compensation to device time as half of the delay of hvc call.
>         (7) add ARM_ARCH_TIMER as dependency of ptp_kvm for arm64.
> 
> from v2 to v3:
>         (1) fix some issues in commit log.
>         (2) add some receivers in send list.
> 
> from v1 to v2:
>         (1) move arch-specific code from arch/ to driver/ptp/
>         (2) offer mechanism to inform userspace if ptp_kvm service is available.
>         (3) separate ptp_kvm code for arm64 into hypervisor part and guest part.
>         (4) add API to expose monotonic clock and counter value.
>         (5) refine code: remove no necessary part and reconsitution.
> 
> [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
> [4]https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/
> commit/?h=for-next/smccc-conduit-
> cleanup&id=6b7fe77c334ae59fed9500140e08f4f896b36871
> 
> Jianyong Wu (6):
>   psci: let arm_smccc_1_1_invoke available by modules
>   ptp: Reorganize ptp_kvm modules to make it arch-independent.
>   clocksource: Add clocksource id for arm arch counter
>   psci: Add hvc call service for ptp_kvm.
>   ptp: arm64: Enable ptp_kvm for arm64
>   kvm: arm64: Add capability check extension for ptp_kvm
> 
> Mark Rutland (1):
>   arm/arm64: smccc/psci: add arm_smccc_1_1_get_conduit()
> 
> Thomas Gleixner (1):
>   time: Add mechanism to recognize clocksource in time_get_snapshot
> 
>  drivers/clocksource/arm_arch_timer.c        | 24 ++++++
>  drivers/firmware/psci/psci.c                | 16 ++++
>  drivers/ptp/Kconfig                         |  2 +-
>  drivers/ptp/Makefile                        |  1 +
>  drivers/ptp/ptp_kvm.h                       | 11 +++
>  drivers/ptp/ptp_kvm_arm64.c                 | 53 +++++++++++++
>  drivers/ptp/{ptp_kvm.c => ptp_kvm_common.c} | 77 +++++-------------
>  drivers/ptp/ptp_kvm_x86.c                   | 87 +++++++++++++++++++++
>  include/linux/arm-smccc.h                   | 30 ++++++-
>  include/linux/clocksource.h                 |  6 ++
>  include/linux/clocksource_ids.h             | 13 +++
>  include/linux/timekeeping.h                 | 12 +--
>  include/uapi/linux/kvm.h                    |  1 +
>  kernel/time/clocksource.c                   |  3 +
>  kernel/time/timekeeping.c                   |  1 +
>  virt/kvm/arm/arm.c                          |  1 +
>  virt/kvm/arm/psci.c                         | 22 ++++++
>  17 files changed, 294 insertions(+), 66 deletions(-)  create mode 100644
> drivers/ptp/ptp_kvm.h  create mode 100644 drivers/ptp/ptp_kvm_arm64.c
> rename drivers/ptp/{ptp_kvm.c => ptp_kvm_common.c} (63%)  create
> mode 100644 drivers/ptp/ptp_kvm_x86.c  create mode 100644
> include/linux/clocksource_ids.h
> 
> --
> 2.17.1

Re: [RFC PATCH v9 0/8] Enable ptp_kvm for arm64

[Paolo Bonzini]

On 06/01/20 10:38, Jianyong Wu wrote:
> Ping ...
> Any comments to this patch set?

Marc, Will, can you ack it?  Since the sticky point was the detection of
the clocksource and it was solved by Thomas's patch, I don't have any
more problems including it.

Thanks,

Paolo

>> -----Original Message-----
>> From: Jianyong Wu <jianyong.wu@arm.com>
>> Sent: Tuesday, December 10, 2019 11:40 AM
>> To: netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
>> tglx@linutronix.de; pbonzini@redhat.com; sean.j.christopherson@intel.com;
>> maz@kernel.org; richardcochran@gmail.com; Mark Rutland
>> <Mark.Rutland@arm.com>; will@kernel.org; Suzuki Poulose
>> <Suzuki.Poulose@arm.com>; Steven Price <Steven.Price@arm.com>
>> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> kvmarm@lists.cs.columbia.edu; kvm@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>; Jianyong Wu (Arm Technology China)
>> <Jianyong.Wu@arm.com>; nd <nd@arm.com>
>> Subject: [RFC PATCH v9 0/8] Enable ptp_kvm for arm64
>>
>> kvm ptp targets to provide high precision time sync between guest and host
>> in virtualization environment. Here, we enable kvm ptp for arm64.
>> This patch set base on [1][2][3]
>>
>> change log:
>>
>> from v8 to v9:
>> 	(1) move ptp_kvm.h to driver/ptp/
>> 	(2) replace license declaration of ptp_kvm.h the same with other
>> header files in the same directory.
>>
>> from v7 to v8:
>>         (1) separate adding clocksource id for arm_arch_counter as a single patch.
>>         (2) update commit message for patch 4/8.
>>         (3) refine patch 7/8 and patch 8/8 to make them more independent.
>>
>> from v6 to v7:
>>         (1) include the omitted clocksource_id.h in last version.
>>         (2) reorder the header file in patch.
>>         (3) refine some words in commit message to make it more impersonal.
>>
>> from v5 to v6:
>>         (1) apply Mark's patch[4] to get SMCCC conduit.
>>         (2) add mechanism to recognize current clocksource by add
>> clocksouce_id value into struct clocksource instead of method in patch-v5.
>>         (3) rename kvm_arch_ptp_get_clock_fn into
>> kvm_arch_ptp_get_crosststamp.
>>
>> from v4 to v5:
>>         (1) remove hvc delay compensasion as it should leave to userspace.
>>         (2) check current clocksource in hvc call service.
>>         (3) expose current clocksource by adding it to system_time_snapshot.
>>         (4) add helper to check if clocksource is arm_arch_counter.
>>         (5) rename kvm_ptp.c to ptp_kvm_common.c
>>
>> from v3 to v4:
>>         (1) fix clocksource of ptp_kvm to arch_sys_counter.
>>         (2) move kvm_arch_ptp_get_clock_fn into arm_arch_timer.c
>>         (3) subtract cntvoff before return cycles from host.
>>         (4) use ktime_get_snapshot instead of getnstimeofday and
>> get_current_counterval to return time and counter value.
>>         (5) split ktime and counter into two 32-bit block respectively to avoid
>> Y2038-safe issue.
>>         (6) set time compensation to device time as half of the delay of hvc call.
>>         (7) add ARM_ARCH_TIMER as dependency of ptp_kvm for arm64.
>>
>> from v2 to v3:
>>         (1) fix some issues in commit log.
>>         (2) add some receivers in send list.
>>
>> from v1 to v2:
>>         (1) move arch-specific code from arch/ to driver/ptp/
>>         (2) offer mechanism to inform userspace if ptp_kvm service is available.
>>         (3) separate ptp_kvm code for arm64 into hypervisor part and guest part.
>>         (4) add API to expose monotonic clock and counter value.
>>         (5) refine code: remove no necessary part and reconsitution.
>>
>> [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
>> [4]https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/
>> commit/?h=for-next/smccc-conduit-
>> cleanup&id=6b7fe77c334ae59fed9500140e08f4f896b36871
>>
>> Jianyong Wu (6):
>>   psci: let arm_smccc_1_1_invoke available by modules
>>   ptp: Reorganize ptp_kvm modules to make it arch-independent.
>>   clocksource: Add clocksource id for arm arch counter
>>   psci: Add hvc call service for ptp_kvm.
>>   ptp: arm64: Enable ptp_kvm for arm64
>>   kvm: arm64: Add capability check extension for ptp_kvm
>>
>> Mark Rutland (1):
>>   arm/arm64: smccc/psci: add arm_smccc_1_1_get_conduit()
>>
>> Thomas Gleixner (1):
>>   time: Add mechanism to recognize clocksource in time_get_snapshot
>>
>>  drivers/clocksource/arm_arch_timer.c        | 24 ++++++
>>  drivers/firmware/psci/psci.c                | 16 ++++
>>  drivers/ptp/Kconfig                         |  2 +-
>>  drivers/ptp/Makefile                        |  1 +
>>  drivers/ptp/ptp_kvm.h                       | 11 +++
>>  drivers/ptp/ptp_kvm_arm64.c                 | 53 +++++++++++++
>>  drivers/ptp/{ptp_kvm.c => ptp_kvm_common.c} | 77 +++++-------------
>>  drivers/ptp/ptp_kvm_x86.c                   | 87 +++++++++++++++++++++
>>  include/linux/arm-smccc.h                   | 30 ++++++-
>>  include/linux/clocksource.h                 |  6 ++
>>  include/linux/clocksource_ids.h             | 13 +++
>>  include/linux/timekeeping.h                 | 12 +--
>>  include/uapi/linux/kvm.h                    |  1 +
>>  kernel/time/clocksource.c                   |  3 +
>>  kernel/time/timekeeping.c                   |  1 +
>>  virt/kvm/arm/arm.c                          |  1 +
>>  virt/kvm/arm/psci.c                         | 22 ++++++
>>  17 files changed, 294 insertions(+), 66 deletions(-)  create mode 100644
>> drivers/ptp/ptp_kvm.h  create mode 100644 drivers/ptp/ptp_kvm_arm64.c
>> rename drivers/ptp/{ptp_kvm.c => ptp_kvm_common.c} (63%)  create
>> mode 100644 drivers/ptp/ptp_kvm_x86.c  create mode 100644
>> include/linux/clocksource_ids.h
>>
>> --
>> 2.17.1
> 

Re: [RFC PATCH v9 6/8] psci: Add hvc call service for ptp_kvm.

[Marc Zyngier]

On 2019-12-10 03:40, Jianyong Wu wrote:
> ptp_kvm modules will call hvc to get this service.
> The service offers real time and counter cycle of host for guest.
> 
> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> ---
>  include/linux/arm-smccc.h | 12 ++++++++++++
>  virt/kvm/arm/psci.c       | 22 ++++++++++++++++++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> index 6f82c87308ed..aafb6bac167d 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
> 
> @@ -103,6 +104,17 @@
>  			   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)
> +

All of this depends on patches that have never need posted to any ML, 
and
just linger in Will's tree. You need to pick them up and post them as 
part
of this series so that they can at least be reviewed.

>  #ifndef __ASSEMBLY__
> 
>  #include <linux/linkage.h>
> diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> index 0debf49bf259..682d892d6717 100644
> --- a/virt/kvm/arm/psci.c
> +++ b/virt/kvm/arm/psci.c
> @@ -9,6 +9,7 @@
>  #include <linux/kvm_host.h>
>  #include <linux/uaccess.h>
>  #include <linux/wait.h>
> +#include <linux/clocksource_ids.h>
> 
>  #include <asm/cputype.h>
>  #include <asm/kvm_emulate.h>
> @@ -389,6 +390,8 @@ static int kvm_psci_call(struct kvm_vcpu *vcpu)
> 
>  int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
>  {
> +	struct system_time_snapshot systime_snapshot;
> +	u64 cycles;
>  	u32 func_id = smccc_get_function(vcpu);
>  	u32 val[4] = {};
>  	u32 option;
> @@ -431,6 +434,25 @@ 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 high 32-bit host ktime;
> +	 * reg1 stores low 32-bit host ktime;
> +	 * reg2 stores high 32-bit difference of host cycles and cntvoff;
> +	 * reg3 stores low 32-bit difference of host cycles and cntvoff.

That's either two or four values, and not three as you claim above.

Also, I fail to understand the meaning of the host cycle vs cntvoff
comparison. This is something that guest can perform on its own
(it has access to both physical and virtual timers, and can compute
cntvoff without intervention of the hypervisor).

Finally, how does it work with nested virt, where cntvoff is for the
the vEL2 guest?

> +	 */
> +	case ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID:
> +		ktime_get_snapshot(&systime_snapshot);
> +		if (systime_snapshot.cs_id != CSID_ARM_ARCH_COUNTER)
> +			return kvm_psci_call(vcpu);

What does this mean? Calling PSCI because you've failed to identify
the clock source? What result do you expect from this? Hint: this
isn't a PSCI call.

Cosmetic comments below:

> +		val[0] = systime_snapshot.real >> 32;

val[0] = upper_32_bits(systime_snapshot.real);

> +		val[1] = systime_snapshot.real << 32 >> 32;

val[1] = lower_32_bits(systime_snapshot.real);

> +		cycles = systime_snapshot.cycles -
> +			 vcpu_vtimer(vcpu)->cntvoff;

On a single line please.

> +		val[2] = cycles >> 32;
> +		val[3] = cycles << 32 >> 32;

Same as above.

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

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64

[Marc Zyngier]

On 2019-12-10 03:40, 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.
> 
> 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>
> ---
>  drivers/clocksource/arm_arch_timer.c | 22 ++++++++++++
>  drivers/ptp/Kconfig                  |  2 +-
>  drivers/ptp/ptp_kvm_arm64.c          | 53 ++++++++++++++++++++++++++++
>  3 files changed, 76 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/ptp/ptp_kvm_arm64.c
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c
> b/drivers/clocksource/arm_arch_timer.c
> index 277846decd33..72260b66f02e 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -1636,3 +1636,25 @@ static int __init arch_timer_acpi_init(struct
> acpi_table_header *table)
>  }
>  TIMER_ACPI_DECLARE(arch_timer, ACPI_SIG_GTDT, arch_timer_acpi_init);
>  #endif
> +
> +#if IS_ENABLED(CONFIG_PTP_1588_CLOCK_KVM)
> +#include <linux/arm-smccc.h>
> +int kvm_arch_ptp_get_crosststamp(unsigned long *cycle, struct 
> timespec64 *ts,
> +			      struct clocksource **cs)
> +{
> +	struct arm_smccc_res hvc_res;
> +	ktime_t ktime_overall;
> +
> +	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID, &hvc_res);
> +	if ((long)(hvc_res.a0) < 0)
> +		return -EOPNOTSUPP;
> +
> +	ktime_overall = hvc_res.a0 << 32 | hvc_res.a1;
> +	*ts = ktime_to_timespec64(ktime_overall);
> +	*cycle = hvc_res.a2 << 32 | hvc_res.a3;

So why isn't that just a read of the virtual counter, given that what 
you do
in the hypervisor seems to be "cntpct - cntvoff"?

What am I missing here?

> +	*cs = &clocksource_counter;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(kvm_arch_ptp_get_crosststamp);
> +#endif
> diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
> index 9b8fee5178e8..3c31ff8eb05f 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 && ARM_ARCH_TIMER
>  	default y
>  	help
>  	  This driver adds support for using kvm infrastructure as a PTP
> diff --git a/drivers/ptp/ptp_kvm_arm64.c b/drivers/ptp/ptp_kvm_arm64.c
> new file mode 100644
> index 000000000000..f3f957117865
> --- /dev/null
> +++ b/drivers/ptp/ptp_kvm_arm64.c
> @@ -0,0 +1,53 @@
> +// 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 <linux/kernel.h>
> +#include <linux/err.h>
> +#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>
> +
> +int kvm_arch_ptp_init(void)
> +{
> +	struct arm_smccc_res hvc_res;
> +
> +	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
> +			     &hvc_res);
> +	if ((long)(hvc_res.a0) < 0)
> +		return -EOPNOTSUPP;
> +
> +	return 0;
> +}
> +
> +int kvm_arch_ptp_get_clock_generic(struct timespec64 *ts,
> +				   struct arm_smccc_res *hvc_res)
> +{
> +	ktime_t ktime_overall;
> +
> +	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
> +				  hvc_res);
> +	if ((long)(hvc_res->a0) < 0)
> +		return -EOPNOTSUPP;
> +
> +	ktime_overall = hvc_res->a0 << 32 | hvc_res->a1;
> +	*ts = ktime_to_timespec64(ktime_overall);
> +
> +	return 0;
> +}
> +
> +int kvm_arch_ptp_get_clock(struct timespec64 *ts)
> +{
> +	struct arm_smccc_res hvc_res;
> +
> +	kvm_arch_ptp_get_clock_generic(ts, &hvc_res);
> +
> +	return 0;
> +}

I also wonder why this is all arm64 specific, while everything
should also work just fine on 32bit.

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC PATCH v9 0/8] Enable ptp_kvm for arm64

[Marc Zyngier]

On 2020-01-07 08:15, Paolo Bonzini wrote:
> On 06/01/20 10:38, Jianyong Wu wrote:
>> Ping ...
>> Any comments to this patch set?
> 
> Marc, Will, can you ack it?  Since the sticky point was the detection 
> of
> the clocksource and it was solved by Thomas's patch, I don't have any
> more problems including it.

Boo. I had forgotten about this series. :-(

Going back to it, there is a few ugly points in the arm-specific code
(I'm OK with the generic changes though).

Another thing is that the whole series depends on three patches that 
have
never been posted to any list, hence never reviewed.

Jianyong: Please repost this series *with* the dependencies so that they
can be reviewed, once you've addressed my comments on two of the 
patches.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

RE: [RFC PATCH v9 6/8] psci: Add hvc call service for ptp_kvm.

[Jianyong Wu]

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Tuesday, January 7, 2020 5:16 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>
> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
> tglx@linutronix.de; pbonzini@redhat.com; sean.j.christopherson@intel.com;
> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>;
> will@kernel.org; Suzuki Poulose <Suzuki.Poulose@arm.com>; Steven Price
> <Steven.Price@arm.com>; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu;
> kvm@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>; Kaly Xin
> <Kaly.Xin@arm.com>; Justin He <Justin.He@arm.com>; nd <nd@arm.com>
> Subject: Re: [RFC PATCH v9 6/8] psci: Add hvc call service for ptp_kvm.
> 
> On 2019-12-10 03:40, Jianyong Wu wrote:
> > ptp_kvm modules will call hvc to get this service.
> > The service offers real time and counter cycle of host for guest.
> >
> > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> > ---
> >  include/linux/arm-smccc.h | 12 ++++++++++++
> >  virt/kvm/arm/psci.c       | 22 ++++++++++++++++++++++
> >  2 files changed, 34 insertions(+)
> >
> > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> > index 6f82c87308ed..aafb6bac167d 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
> >
> > @@ -103,6 +104,17 @@
> >  			   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)
> > +
> 
> All of this depends on patches that have never need posted to any ML, and
> just linger in Will's tree. You need to pick them up and post them as part of
> this series so that they can at least be reviewed.
> 
Ok, I will add them next version.

> >  #ifndef __ASSEMBLY__
> >
> >  #include <linux/linkage.h>
> > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c index
> > 0debf49bf259..682d892d6717 100644
> > --- a/virt/kvm/arm/psci.c
> > +++ b/virt/kvm/arm/psci.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/kvm_host.h>
> >  #include <linux/uaccess.h>
> >  #include <linux/wait.h>
> > +#include <linux/clocksource_ids.h>
> >
> >  #include <asm/cputype.h>
> >  #include <asm/kvm_emulate.h>
> > @@ -389,6 +390,8 @@ static int kvm_psci_call(struct kvm_vcpu *vcpu)
> >
> >  int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)  {
> > +	struct system_time_snapshot systime_snapshot;
> > +	u64 cycles;
> >  	u32 func_id = smccc_get_function(vcpu);
> >  	u32 val[4] = {};
> >  	u32 option;
> > @@ -431,6 +434,25 @@ 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 high 32-bit host ktime;
> > +	 * reg1 stores low 32-bit host ktime;
> > +	 * reg2 stores high 32-bit difference of host cycles and cntvoff;
> > +	 * reg3 stores low 32-bit difference of host cycles and cntvoff.
> 
> That's either two or four values, and not three as you claim above.
> 
Sorry, I'm not sure what do you mean "three", the registers here is 4 from reg0 to reg3.

> Also, I fail to understand the meaning of the host cycle vs cntvoff comparison.
> This is something that guest can perform on its own (it has access to both
> physical and virtual timers, and can compute cntvoff without intervention of
> the hypervisor).
> 
To keep consistency and precision, clock time and counter cycle must captured at the same time. 
It will perform at ktime_get_snapshot.

> Finally, how does it work with nested virt, where cntvoff is for the the vEL2
> guest?
> 
For now, I have not considered ptp_kvm in nested virtualization. Also I'm not sure about if nested virtualization is
ready on arm64 , as I need test ptp_kvm on it. If so, I can consider it.

> > +	 */
> > +	case ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID:
> > +		ktime_get_snapshot(&systime_snapshot);
> > +		if (systime_snapshot.cs_id != CSID_ARM_ARCH_COUNTER)
> > +			return kvm_psci_call(vcpu);
> 
> What does this mean? Calling PSCI because you've failed to identify the clock
> source? What result do you expect from this? Hint: this isn't a PSCI call.
> 
Sorry, what I want to do here is that return to guest with the error info.
Maybe I should set val[0] to -1 and break to let the guest know that error comes, as
the guest will check if val[0] is positive to determine the next step.

> Cosmetic comments below:
> 
> > +		val[0] = systime_snapshot.real >> 32;
> 
> val[0] = upper_32_bits(systime_snapshot.real);
Ok
> 
> > +		val[1] = systime_snapshot.real << 32 >> 32;
> 
> val[1] = lower_32_bits(systime_snapshot.real);
> 
Yeah

> > +		cycles = systime_snapshot.cycles -
> > +			 vcpu_vtimer(vcpu)->cntvoff;
> 
> On a single line please.
>
ok
 
> > +		val[2] = cycles >> 32;
> > +		val[3] = cycles << 32 >> 32;
> 
> Same as above.
> 
yeah
> > +		break;
> >  	default:
> >  		return kvm_psci_call(vcpu);
> >  	}
> 

Thanks
Jianyong

> Thanks,
> 
>          M.
> --
> Jazz is not dead. It just smells funny...

RE: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64

[Jianyong Wu]

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Tuesday, January 7, 2020 5:29 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>
> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
> tglx@linutronix.de; pbonzini@redhat.com; sean.j.christopherson@intel.com;
> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>;
> will@kernel.org; Suzuki Poulose <Suzuki.Poulose@arm.com>; Steven Price
> <Steven.Price@arm.com>; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu;
> kvm@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>; Kaly Xin
> <Kaly.Xin@arm.com>; Justin He <Justin.He@arm.com>; nd <nd@arm.com>
> Subject: Re: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64
> 
> On 2019-12-10 03:40, 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.
> >
> > 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>
> > ---
> >  drivers/clocksource/arm_arch_timer.c | 22 ++++++++++++
> >  drivers/ptp/Kconfig                  |  2 +-
> >  drivers/ptp/ptp_kvm_arm64.c          | 53
> ++++++++++++++++++++++++++++
> >  3 files changed, 76 insertions(+), 1 deletion(-)  create mode 100644
> > drivers/ptp/ptp_kvm_arm64.c
> >
> > diff --git a/drivers/clocksource/arm_arch_timer.c
> > b/drivers/clocksource/arm_arch_timer.c
> > index 277846decd33..72260b66f02e 100644
> > --- a/drivers/clocksource/arm_arch_timer.c
> > +++ b/drivers/clocksource/arm_arch_timer.c
> > @@ -1636,3 +1636,25 @@ static int __init arch_timer_acpi_init(struct
> > acpi_table_header *table)  }  TIMER_ACPI_DECLARE(arch_timer,
> > ACPI_SIG_GTDT, arch_timer_acpi_init);  #endif
> > +
> > +#if IS_ENABLED(CONFIG_PTP_1588_CLOCK_KVM)
> > +#include <linux/arm-smccc.h>
> > +int kvm_arch_ptp_get_crosststamp(unsigned long *cycle, struct
> > timespec64 *ts,
> > +			      struct clocksource **cs)
> > +{
> > +	struct arm_smccc_res hvc_res;
> > +	ktime_t ktime_overall;
> > +
> > +
> 	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU
> NC_ID, &hvc_res);
> > +	if ((long)(hvc_res.a0) < 0)
> > +		return -EOPNOTSUPP;
> > +
> > +	ktime_overall = hvc_res.a0 << 32 | hvc_res.a1;
> > +	*ts = ktime_to_timespec64(ktime_overall);
> > +	*cycle = hvc_res.a2 << 32 | hvc_res.a3;
> 
> So why isn't that just a read of the virtual counter, given that what you do in
> the hypervisor seems to be "cntpct - cntvoff"?
> 
> What am I missing here?
> 
We need get clock time and counter cycle at the same time, so we can't just read virtual counter
at guest and must get it from host.

> > +	*cs = &clocksource_counter;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(kvm_arch_ptp_get_crosststamp);
> > +#endif
> > diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig index
> > 9b8fee5178e8..3c31ff8eb05f 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 && ARM_ARCH_TIMER
> >  	default y
> >  	help
> >  	  This driver adds support for using kvm infrastructure as a PTP
> > diff --git a/drivers/ptp/ptp_kvm_arm64.c b/drivers/ptp/ptp_kvm_arm64.c
> > new file mode 100644 index 000000000000..f3f957117865
> > --- /dev/null
> > +++ b/drivers/ptp/ptp_kvm_arm64.c
> > @@ -0,0 +1,53 @@
> > +// 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 <linux/kernel.h>
> > +#include <linux/err.h>
> > +#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>
> > +
> > +int kvm_arch_ptp_init(void)
> > +{
> > +	struct arm_smccc_res hvc_res;
> > +
> > +
> 	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU
> NC_ID,
> > +			     &hvc_res);
> > +	if ((long)(hvc_res.a0) < 0)
> > +		return -EOPNOTSUPP;
> > +
> > +	return 0;
> > +}
> > +
> > +int kvm_arch_ptp_get_clock_generic(struct timespec64 *ts,
> > +				   struct arm_smccc_res *hvc_res) {
> > +	ktime_t ktime_overall;
> > +
> > +
> 	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU
> NC_ID,
> > +				  hvc_res);
> > +	if ((long)(hvc_res->a0) < 0)
> > +		return -EOPNOTSUPP;
> > +
> > +	ktime_overall = hvc_res->a0 << 32 | hvc_res->a1;
> > +	*ts = ktime_to_timespec64(ktime_overall);
> > +
> > +	return 0;
> > +}
> > +
> > +int kvm_arch_ptp_get_clock(struct timespec64 *ts) {
> > +	struct arm_smccc_res hvc_res;
> > +
> > +	kvm_arch_ptp_get_clock_generic(ts, &hvc_res);
> > +
> > +	return 0;
> > +}
> 
> I also wonder why this is all arm64 specific, while everything should also work
> just fine on 32bit.
>
ptp_kvm is a feature for cloud computing to keep time consistency from container to container and to host on server,
So we focus it on arm64. Also I have never tested it on arm32 machine ( we lack of arm32 machine)
Do you think it's necessary to enable ptp_kvm on arm32? If so, I can do that.
 
Thanks
Jianyong

>          M.
> --
> Jazz is not dead. It just smells funny...

Re: [RFC PATCH v9 6/8] psci: Add hvc call service for ptp_kvm.

[Marc Zyngier]

On 2020-01-09 05:45, Jianyong Wu wrote:
> Hi Marc,
> 
>> -----Original Message-----
>> From: Marc Zyngier <maz@kernel.org>
>> Sent: Tuesday, January 7, 2020 5:16 PM
>> To: Jianyong Wu <Jianyong.Wu@arm.com>
>> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
>> tglx@linutronix.de; pbonzini@redhat.com; 
>> sean.j.christopherson@intel.com;
>> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>;
>> will@kernel.org; Suzuki Poulose <Suzuki.Poulose@arm.com>; Steven Price
>> <Steven.Price@arm.com>; linux-kernel@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu;
>> kvm@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>; Kaly Xin
>> <Kaly.Xin@arm.com>; Justin He <Justin.He@arm.com>; nd <nd@arm.com>
>> Subject: Re: [RFC PATCH v9 6/8] psci: Add hvc call service for 
>> ptp_kvm.
>> 
>> On 2019-12-10 03:40, Jianyong Wu wrote:
>> > ptp_kvm modules will call hvc to get this service.
>> > The service offers real time and counter cycle of host for guest.
>> >
>> > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
>> > ---
>> >  include/linux/arm-smccc.h | 12 ++++++++++++
>> >  virt/kvm/arm/psci.c       | 22 ++++++++++++++++++++++
>> >  2 files changed, 34 insertions(+)
>> >
>> > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
>> > index 6f82c87308ed..aafb6bac167d 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
>> >
>> > @@ -103,6 +104,17 @@
>> >  			   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)
>> > +
>> 
>> All of this depends on patches that have never need posted to any ML, 
>> and
>> just linger in Will's tree. You need to pick them up and post them as 
>> part of
>> this series so that they can at least be reviewed.
>> 
> Ok, I will add them next version.
> 
>> >  #ifndef __ASSEMBLY__
>> >
>> >  #include <linux/linkage.h>
>> > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c index
>> > 0debf49bf259..682d892d6717 100644
>> > --- a/virt/kvm/arm/psci.c
>> > +++ b/virt/kvm/arm/psci.c
>> > @@ -9,6 +9,7 @@
>> >  #include <linux/kvm_host.h>
>> >  #include <linux/uaccess.h>
>> >  #include <linux/wait.h>
>> > +#include <linux/clocksource_ids.h>
>> >
>> >  #include <asm/cputype.h>
>> >  #include <asm/kvm_emulate.h>
>> > @@ -389,6 +390,8 @@ static int kvm_psci_call(struct kvm_vcpu *vcpu)
>> >
>> >  int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)  {
>> > +	struct system_time_snapshot systime_snapshot;
>> > +	u64 cycles;
>> >  	u32 func_id = smccc_get_function(vcpu);
>> >  	u32 val[4] = {};
>> >  	u32 option;
>> > @@ -431,6 +434,25 @@ 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 high 32-bit host ktime;
>> > +	 * reg1 stores low 32-bit host ktime;
>> > +	 * reg2 stores high 32-bit difference of host cycles and cntvoff;
>> > +	 * reg3 stores low 32-bit difference of host cycles and cntvoff.
>> 
>> That's either two or four values, and not three as you claim above.
>> 
> Sorry, I'm not sure what do you mean "three", the registers here is 4
> from reg0 to reg3.

Please read the comment you have written above...

>> Also, I fail to understand the meaning of the host cycle vs cntvoff 
>> comparison.
>> This is something that guest can perform on its own (it has access to 
>> both
>> physical and virtual timers, and can compute cntvoff without 
>> intervention of
>> the hypervisor).
>> 
> To keep consistency and precision, clock time and counter cycle must
> captured at the same time. It will perform at ktime_get_snapshot.

Fair enough. It would vertainly help if you documented it. It would also
help if you explained why it is so much worse to read the counter in the
guest before *and* after the call, and assume that the clock time read
happened right in the middle?

That aside, what you are returning is something that *looks* like
the virtual counter. What if the guest is using the physical counter,
which is likely to be the case with nested virt? Do you expect the guest
to always use the virtual counter? This isn't going to fly.

>> Finally, how does it work with nested virt, where cntvoff is for the 
>> the vEL2
>> guest?
>> 
> For now, I have not considered ptp_kvm in nested virtualization. Also
> I'm not sure about if nested virtualization is
> ready on arm64 , as I need test ptp_kvm on it. If so, I can consider 
> it.

It is not about testing. It is about taking the architecture into 
account.
And ready or not doesn't come into play here. What you're defining here 
is
an ABI, and it better be totally future proof.

But if you want to test, help yourself to [1] and have fun!
> 
>> > +	 */
>> > +	case ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID:
>> > +		ktime_get_snapshot(&systime_snapshot);
>> > +		if (systime_snapshot.cs_id != CSID_ARM_ARCH_COUNTER)
>> > +			return kvm_psci_call(vcpu);
>> 
>> What does this mean? Calling PSCI because you've failed to identify 
>> the clock
>> source? What result do you expect from this? Hint: this isn't a PSCI 
>> call.
>> 
> Sorry, what I want to do here is that return to guest with the error 
> info.
> Maybe I should set val[0] to -1 and break to let the guest know that
> error comes, as
> the guest will check if val[0] is positive to determine the next step.

What you should do is handle it like a normal SMCCC failure.

         M.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/nv-5.5-rc4-WIP
-- 
Jazz is not dead. It just smells funny...

Re: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64

[Marc Zyngier]

On 2020-01-09 05:59, Jianyong Wu wrote:
> Hi Marc,
> 
>> -----Original Message-----
>> From: Marc Zyngier <maz@kernel.org>
>> Sent: Tuesday, January 7, 2020 5:29 PM
>> To: Jianyong Wu <Jianyong.Wu@arm.com>
>> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
>> tglx@linutronix.de; pbonzini@redhat.com; 
>> sean.j.christopherson@intel.com;
>> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>;
>> will@kernel.org; Suzuki Poulose <Suzuki.Poulose@arm.com>; Steven Price
>> <Steven.Price@arm.com>; linux-kernel@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu;
>> kvm@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>; Kaly Xin
>> <Kaly.Xin@arm.com>; Justin He <Justin.He@arm.com>; nd <nd@arm.com>
>> Subject: Re: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64
>> 
>> On 2019-12-10 03:40, 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.
>> >
>> > 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>
>> > ---
>> >  drivers/clocksource/arm_arch_timer.c | 22 ++++++++++++
>> >  drivers/ptp/Kconfig                  |  2 +-
>> >  drivers/ptp/ptp_kvm_arm64.c          | 53
>> ++++++++++++++++++++++++++++
>> >  3 files changed, 76 insertions(+), 1 deletion(-)  create mode 100644
>> > drivers/ptp/ptp_kvm_arm64.c
>> >
>> > diff --git a/drivers/clocksource/arm_arch_timer.c
>> > b/drivers/clocksource/arm_arch_timer.c
>> > index 277846decd33..72260b66f02e 100644
>> > --- a/drivers/clocksource/arm_arch_timer.c
>> > +++ b/drivers/clocksource/arm_arch_timer.c
>> > @@ -1636,3 +1636,25 @@ static int __init arch_timer_acpi_init(struct
>> > acpi_table_header *table)  }  TIMER_ACPI_DECLARE(arch_timer,
>> > ACPI_SIG_GTDT, arch_timer_acpi_init);  #endif
>> > +
>> > +#if IS_ENABLED(CONFIG_PTP_1588_CLOCK_KVM)
>> > +#include <linux/arm-smccc.h>
>> > +int kvm_arch_ptp_get_crosststamp(unsigned long *cycle, struct
>> > timespec64 *ts,
>> > +			      struct clocksource **cs)
>> > +{
>> > +	struct arm_smccc_res hvc_res;
>> > +	ktime_t ktime_overall;
>> > +
>> > +
>> 	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU
>> NC_ID, &hvc_res);
>> > +	if ((long)(hvc_res.a0) < 0)
>> > +		return -EOPNOTSUPP;
>> > +
>> > +	ktime_overall = hvc_res.a0 << 32 | hvc_res.a1;
>> > +	*ts = ktime_to_timespec64(ktime_overall);
>> > +	*cycle = hvc_res.a2 << 32 | hvc_res.a3;
>> 
>> So why isn't that just a read of the virtual counter, given that what 
>> you do in
>> the hypervisor seems to be "cntpct - cntvoff"?
>> 
>> What am I missing here?
>> 
> We need get clock time and counter cycle at the same time, so we can't
> just read virtual counter
> at guest and must get it from host.

See my comment in my reply to patch #6: *Must* seems like a very strong 
word,
and you don't explain *why* that's better than just computing the total 
hypercall
cost. Hint: given the frequency of the counter (in the few MHz range) vs 
the
frequency of a CPU (in the multiple GHz range, and with an IPC close 
enough to 1),
I doubt that you'll see the counter making much progress across a 
hypercall.

> 
>> > +	*cs = &clocksource_counter;
>> > +
>> > +	return 0;
>> > +}
>> > +EXPORT_SYMBOL_GPL(kvm_arch_ptp_get_crosststamp);
>> > +#endif
>> > diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig index
>> > 9b8fee5178e8..3c31ff8eb05f 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 && ARM_ARCH_TIMER
>> >  	default y
>> >  	help
>> >  	  This driver adds support for using kvm infrastructure as a PTP
>> > diff --git a/drivers/ptp/ptp_kvm_arm64.c b/drivers/ptp/ptp_kvm_arm64.c
>> > new file mode 100644 index 000000000000..f3f957117865
>> > --- /dev/null
>> > +++ b/drivers/ptp/ptp_kvm_arm64.c
>> > @@ -0,0 +1,53 @@
>> > +// 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 <linux/kernel.h>
>> > +#include <linux/err.h>
>> > +#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>
>> > +
>> > +int kvm_arch_ptp_init(void)
>> > +{
>> > +	struct arm_smccc_res hvc_res;
>> > +
>> > +
>> 	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU
>> NC_ID,
>> > +			     &hvc_res);
>> > +	if ((long)(hvc_res.a0) < 0)
>> > +		return -EOPNOTSUPP;
>> > +
>> > +	return 0;
>> > +}
>> > +
>> > +int kvm_arch_ptp_get_clock_generic(struct timespec64 *ts,
>> > +				   struct arm_smccc_res *hvc_res) {
>> > +	ktime_t ktime_overall;
>> > +
>> > +
>> 	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU
>> NC_ID,
>> > +				  hvc_res);
>> > +	if ((long)(hvc_res->a0) < 0)
>> > +		return -EOPNOTSUPP;
>> > +
>> > +	ktime_overall = hvc_res->a0 << 32 | hvc_res->a1;
>> > +	*ts = ktime_to_timespec64(ktime_overall);
>> > +
>> > +	return 0;
>> > +}
>> > +
>> > +int kvm_arch_ptp_get_clock(struct timespec64 *ts) {
>> > +	struct arm_smccc_res hvc_res;
>> > +
>> > +	kvm_arch_ptp_get_clock_generic(ts, &hvc_res);
>> > +
>> > +	return 0;
>> > +}
>> 
>> I also wonder why this is all arm64 specific, while everything should 
>> also work
>> just fine on 32bit.
>> 
> ptp_kvm is a feature for cloud computing to keep time consistency from
> container to container and to host on server,

Cloud computing? Never heard of that. Will probably never catch on.

> So we focus it on arm64. Also I have never tested it on arm32 machine
> ( we lack of arm32 machine)

I'm sure your employer can provide you with such a box. I can probably
even tell you which cupboard they are stored in... ;-)

> Do you think it's necessary to enable ptp_kvm on arm32? If so, I can do 
> that.

I can't see why we wouldn't, given that it should be a zero effort task
(none of the code here is arch specific).

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64

[Marc Zyngier]

On 2020-01-09 09:24, Marc Zyngier wrote:
> On 2020-01-09 05:59, Jianyong Wu wrote:

[...]

>> So we focus it on arm64. Also I have never tested it on arm32 machine
>> ( we lack of arm32 machine)
> 
> I'm sure your employer can provide you with such a box. I can probably
> even tell you which cupboard they are stored in... ;-)
> 
>> Do you think it's necessary to enable ptp_kvm on arm32? If so, I can 
>> do that.
> 
> I can't see why we wouldn't, given that it should be a zero effort task
> (none of the code here is arch specific).

To be clear, what I'm after is support for 32bit *guests*. I don't 
expect any
issue with a 32bit host (it's all common code), but you should be able 
to test
32bit guests pretty easily (most ARMv8.0 CPUs support 32bit EL1).

         M.
-- 
Jazz is not dead. It just smells funny...

RE: [RFC PATCH v9 6/8] psci: Add hvc call service for ptp_kvm.

[Jianyong Wu]

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Thursday, January 9, 2020 5:16 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>
> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
> tglx@linutronix.de; pbonzini@redhat.com; sean.j.christopherson@intel.com;
> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>;
> will@kernel.org; Suzuki Poulose <Suzuki.Poulose@arm.com>; Steven Price
> <Steven.Price@arm.com>; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu;
> kvm@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>; Kaly Xin
> <Kaly.Xin@arm.com>; Justin He <Justin.He@arm.com>; nd <nd@arm.com>
> Subject: Re: [RFC PATCH v9 6/8] psci: Add hvc call service for ptp_kvm.
> 
> On 2020-01-09 05:45, Jianyong Wu wrote:
> > Hi Marc,
> >
> >> -----Original Message-----
> >> From: Marc Zyngier <maz@kernel.org>
> >> Sent: Tuesday, January 7, 2020 5:16 PM
> >> To: Jianyong Wu <Jianyong.Wu@arm.com>
> >> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com;
> >> john.stultz@linaro.org; tglx@linutronix.de; pbonzini@redhat.com;
> >> sean.j.christopherson@intel.com; richardcochran@gmail.com; Mark
> >> Rutland <Mark.Rutland@arm.com>; will@kernel.org; Suzuki Poulose
> >> <Suzuki.Poulose@arm.com>; Steven Price <Steven.Price@arm.com>;
> >> linux-kernel@vger.kernel.org; linux-arm- kernel@lists.infradead.org;
> >> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; Steve Capper
> >> <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Justin He
> >> <Justin.He@arm.com>; nd <nd@arm.com>
> >> Subject: Re: [RFC PATCH v9 6/8] psci: Add hvc call service for
> >> ptp_kvm.
> >>
> >> On 2019-12-10 03:40, Jianyong Wu wrote:
> >> > ptp_kvm modules will call hvc to get this service.
> >> > The service offers real time and counter cycle of host for guest.
> >> >
> >> > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> >> > ---
> >> >  include/linux/arm-smccc.h | 12 ++++++++++++
> >> >  virt/kvm/arm/psci.c       | 22 ++++++++++++++++++++++
> >> >  2 files changed, 34 insertions(+)
> >> >
> >> > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> >> > index 6f82c87308ed..aafb6bac167d 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
> >> >
> >> > @@ -103,6 +104,17 @@
> >> >  			   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)
> >> > +
> >>
> >> All of this depends on patches that have never need posted to any ML,
> >> and just linger in Will's tree. You need to pick them up and post
> >> them as part of this series so that they can at least be reviewed.
> >>
> > Ok, I will add them next version.
> >
> >> >  #ifndef __ASSEMBLY__
> >> >
> >> >  #include <linux/linkage.h>
> >> > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c index
> >> > 0debf49bf259..682d892d6717 100644
> >> > --- a/virt/kvm/arm/psci.c
> >> > +++ b/virt/kvm/arm/psci.c
> >> > @@ -9,6 +9,7 @@
> >> >  #include <linux/kvm_host.h>
> >> >  #include <linux/uaccess.h>
> >> >  #include <linux/wait.h>
> >> > +#include <linux/clocksource_ids.h>
> >> >
> >> >  #include <asm/cputype.h>
> >> >  #include <asm/kvm_emulate.h>
> >> > @@ -389,6 +390,8 @@ static int kvm_psci_call(struct kvm_vcpu *vcpu)
> >> >
> >> >  int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)  {
> >> > +	struct system_time_snapshot systime_snapshot;
> >> > +	u64 cycles;
> >> >  	u32 func_id = smccc_get_function(vcpu);
> >> >  	u32 val[4] = {};
> >> >  	u32 option;
> >> > @@ -431,6 +434,25 @@ 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 high 32-bit host ktime;
> >> > +	 * reg1 stores low 32-bit host ktime;
> >> > +	 * reg2 stores high 32-bit difference of host cycles and cntvoff;
> >> > +	 * reg3 stores low 32-bit difference of host cycles and cntvoff.
> >>
> >> That's either two or four values, and not three as you claim above.
> >>
> > Sorry, I'm not sure what do you mean "three", the registers here is 4
> > from reg0 to reg3.
> 
> Please read the comment you have written above...

oh, I see it.

> 
> >> Also, I fail to understand the meaning of the host cycle vs cntvoff
> >> comparison.
> >> This is something that guest can perform on its own (it has access to
> >> both physical and virtual timers, and can compute cntvoff without
> >> intervention of the hypervisor).
> >>
> > To keep consistency and precision, clock time and counter cycle must
> > captured at the same time. It will perform at ktime_get_snapshot.
> 
> Fair enough. It would vertainly help if you documented it. It would also help if
> you explained why it is so much worse to read the counter in the guest
> before *and* after the call, and assume that the clock time read happened
> right in the middle?
> 
ok, I will give explain in comments.

> That aside, what you are returning is something that *looks* like the virtual
> counter. What if the guest is using the physical counter, which is likely to be
> the case with nested virt? Do you expect the guest to always use the virtual
> counter? This isn't going to fly.

To be honest, I have little knowledge of nested virtualization for arm and I'm confused with that
guest'guest will use physical counter.

IMO, ptp_kvm will call hvc to trap to its hypervisor adjacent to it. So guest'guest will trap to hypervisor in guest and will
get guest's counter cycle then calculate guest'guest's counter cycle by something like offset to sync time with it. So only if the
guest's hypervisor can calculate the guest'guest's counter value, can ptp_kvm works.

the implementation of calculating the return value of counter cycle vary with the way deriving counter cycle from hypervisor to guest.

If considering nested virt here, we need the basic knowledge of how guest'guest's counter cycle is calculated from its hypervisor and how to determine 
we are in guest's hypervisor or guest'guest's hypervisor.
If it is the case, can you give me some knowledge, something like a document, about that?

> 
> >> Finally, how does it work with nested virt, where cntvoff is for the
> >> the vEL2 guest?
> >>
> > For now, I have not considered ptp_kvm in nested virtualization. Also
> > I'm not sure about if nested virtualization is ready on arm64 , as I
> > need test ptp_kvm on it. If so, I can consider it.
> 
> It is not about testing. It is about taking the architecture into account.
> And ready or not doesn't come into play here. What you're defining here is
> an ABI, and it better be totally future proof.
> 
Yeah, should included it in design.

> But if you want to test, help yourself to [1] and have fun!
> >
Thanks

> >> > +	 */
> >> > +	case ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID:
> >> > +		ktime_get_snapshot(&systime_snapshot);
> >> > +		if (systime_snapshot.cs_id != CSID_ARM_ARCH_COUNTER)
> >> > +			return kvm_psci_call(vcpu);
> >>
> >> What does this mean? Calling PSCI because you've failed to identify
> >> the clock source? What result do you expect from this? Hint: this
> >> isn't a PSCI call.
> >>
> > Sorry, what I want to do here is that return to guest with the error
> > info.
> > Maybe I should set val[0] to -1 and break to let the guest know that
> > error comes, as the guest will check if val[0] is positive to
> > determine the next step.
> 
> What you should do is handle it like a normal SMCCC failure.
> 
Yeah, I will fix it.

Thanks
Jianyong 

>          M.
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-
> platforms.git/log/?h=kvm-arm64/nv-5.5-rc4-WIP
> --
> Jazz is not dead. It just smells funny...

RE: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64

[Jianyong Wu]

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Thursday, January 9, 2020 5:24 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>
> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
> tglx@linutronix.de; pbonzini@redhat.com; sean.j.christopherson@intel.com;
> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>;
> will@kernel.org; Suzuki Poulose <Suzuki.Poulose@arm.com>; Steven Price
> <Steven.Price@arm.com>; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu;
> kvm@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>; Kaly Xin
> <Kaly.Xin@arm.com>; Justin He <Justin.He@arm.com>; nd <nd@arm.com>
> Subject: Re: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64
> 
> On 2020-01-09 05:59, Jianyong Wu wrote:
> > Hi Marc,
> >
> >> -----Original Message-----
> >> From: Marc Zyngier <maz@kernel.org>
> >> Sent: Tuesday, January 7, 2020 5:29 PM
> >> To: Jianyong Wu <Jianyong.Wu@arm.com>
> >> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com;
> >> john.stultz@linaro.org; tglx@linutronix.de; pbonzini@redhat.com;
> >> sean.j.christopherson@intel.com; richardcochran@gmail.com; Mark
> >> Rutland <Mark.Rutland@arm.com>; will@kernel.org; Suzuki Poulose
> >> <Suzuki.Poulose@arm.com>; Steven Price <Steven.Price@arm.com>;
> >> linux-kernel@vger.kernel.org; linux-arm- kernel@lists.infradead.org;
> >> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; Steve Capper
> >> <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Justin He
> >> <Justin.He@arm.com>; nd <nd@arm.com>
> >> Subject: Re: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64
> >>
> >> On 2019-12-10 03:40, 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.
> >> >
> >> > 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>
> >> > ---
> >> >  drivers/clocksource/arm_arch_timer.c | 22 ++++++++++++
> >> >  drivers/ptp/Kconfig                  |  2 +-
> >> >  drivers/ptp/ptp_kvm_arm64.c          | 53
> >> ++++++++++++++++++++++++++++
> >> >  3 files changed, 76 insertions(+), 1 deletion(-)  create mode
> >> > 100644 drivers/ptp/ptp_kvm_arm64.c
> >> >
> >> > diff --git a/drivers/clocksource/arm_arch_timer.c
> >> > b/drivers/clocksource/arm_arch_timer.c
> >> > index 277846decd33..72260b66f02e 100644
> >> > --- a/drivers/clocksource/arm_arch_timer.c
> >> > +++ b/drivers/clocksource/arm_arch_timer.c
> >> > @@ -1636,3 +1636,25 @@ static int __init
> >> > arch_timer_acpi_init(struct acpi_table_header *table)  }
> >> > TIMER_ACPI_DECLARE(arch_timer, ACPI_SIG_GTDT,
> >> > arch_timer_acpi_init);  #endif
> >> > +
> >> > +#if IS_ENABLED(CONFIG_PTP_1588_CLOCK_KVM)
> >> > +#include <linux/arm-smccc.h>
> >> > +int kvm_arch_ptp_get_crosststamp(unsigned long *cycle, struct
> >> > timespec64 *ts,
> >> > +			      struct clocksource **cs)
> >> > +{
> >> > +	struct arm_smccc_res hvc_res;
> >> > +	ktime_t ktime_overall;
> >> > +
> >> > +
> >> 	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU
> >> NC_ID, &hvc_res);
> >> > +	if ((long)(hvc_res.a0) < 0)
> >> > +		return -EOPNOTSUPP;
> >> > +
> >> > +	ktime_overall = hvc_res.a0 << 32 | hvc_res.a1;
> >> > +	*ts = ktime_to_timespec64(ktime_overall);
> >> > +	*cycle = hvc_res.a2 << 32 | hvc_res.a3;
> >>
> >> So why isn't that just a read of the virtual counter, given that what
> >> you do in the hypervisor seems to be "cntpct - cntvoff"?
> >>
> >> What am I missing here?
> >>
> > We need get clock time and counter cycle at the same time, so we can't
> > just read virtual counter at guest and must get it from host.
> 
> See my comment in my reply to patch #6: *Must* seems like a very strong
> word, and you don't explain *why* that's better than just computing the
> total hypercall cost. Hint: given the frequency of the counter (in the few MHz
> range) vs the frequency of a CPU (in the multiple GHz range, and with an IPC
> close enough to 1), I doubt that you'll see the counter making much progress
> across a hypercall.
> 
Sorry, I will avoid to use those strong words.

It's really the case that the hypercall won't across cycle in general. But sometimes, kernel preempt 
may happen in the middle of the hypercall which we can't assume how long before schedule back. so it's better capture them
together at the same time.

> >
> >> > +	*cs = &clocksource_counter;
> >> > +
> >> > +	return 0;
> >> > +}
> >> > +EXPORT_SYMBOL_GPL(kvm_arch_ptp_get_crosststamp);
> >> > +#endif
> >> > diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig index
> >> > 9b8fee5178e8..3c31ff8eb05f 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 && ARM_ARCH_TIMER
> >> >  	default y
> >> >  	help
> >> >  	  This driver adds support for using kvm infrastructure as a PTP
> >> > diff --git a/drivers/ptp/ptp_kvm_arm64.c
> >> > b/drivers/ptp/ptp_kvm_arm64.c new file mode 100644 index
> >> > 000000000000..f3f957117865
> >> > --- /dev/null
> >> > +++ b/drivers/ptp/ptp_kvm_arm64.c
> >> > @@ -0,0 +1,53 @@
> >> > +// 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 <linux/kernel.h>
> >> > +#include <linux/err.h>
> >> > +#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>
> >> > +
> >> > +int kvm_arch_ptp_init(void)
> >> > +{
> >> > +	struct arm_smccc_res hvc_res;
> >> > +
> >> > +
> >> 	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU
> >> NC_ID,
> >> > +			     &hvc_res);
> >> > +	if ((long)(hvc_res.a0) < 0)
> >> > +		return -EOPNOTSUPP;
> >> > +
> >> > +	return 0;
> >> > +}
> >> > +
> >> > +int kvm_arch_ptp_get_clock_generic(struct timespec64 *ts,
> >> > +				   struct arm_smccc_res *hvc_res) {
> >> > +	ktime_t ktime_overall;
> >> > +
> >> > +
> >> 	arm_smccc_1_1_invoke(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU
> >> NC_ID,
> >> > +				  hvc_res);
> >> > +	if ((long)(hvc_res->a0) < 0)
> >> > +		return -EOPNOTSUPP;
> >> > +
> >> > +	ktime_overall = hvc_res->a0 << 32 | hvc_res->a1;
> >> > +	*ts = ktime_to_timespec64(ktime_overall);
> >> > +
> >> > +	return 0;
> >> > +}
> >> > +
> >> > +int kvm_arch_ptp_get_clock(struct timespec64 *ts) {
> >> > +	struct arm_smccc_res hvc_res;
> >> > +
> >> > +	kvm_arch_ptp_get_clock_generic(ts, &hvc_res);
> >> > +
> >> > +	return 0;
> >> > +}
> >>
> >> I also wonder why this is all arm64 specific, while everything should
> >> also work just fine on 32bit.
> >>
> > ptp_kvm is a feature for cloud computing to keep time consistency from
> > container to container and to host on server,
> 
> Cloud computing? Never heard of that. Will probably never catch on.
> 
Eh...

> > So we focus it on arm64. Also I have never tested it on arm32 machine
> > ( we lack of arm32 machine)
> 
> I'm sure your employer can provide you with such a box. I can probably even
> tell you which cupboard they are stored in... ;-)
> 
I'm working on building environment for arm32 from now.

> > Do you think it's necessary to enable ptp_kvm on arm32? If so, I can
> > do that.
> 
> I can't see why we wouldn't, given that it should be a zero effort task (none
> of the code here is arch specific).

Ok, I will test it on arm32.
By the way, does nested virtualization diff between arm64 and arm32?

Thanks
Jianyong
> 
> Thanks,
> 
>          M.
> --
> Jazz is not dead. It just smells funny...

RE: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64

[Jianyong Wu]

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Thursday, January 9, 2020 5:46 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>
> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
> tglx@linutronix.de; pbonzini@redhat.com; sean.j.christopherson@intel.com;
> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>;
> will@kernel.org; Suzuki Poulose <Suzuki.Poulose@arm.com>; Steven Price
> <Steven.Price@arm.com>; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu;
> kvm@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>; Kaly Xin
> <Kaly.Xin@arm.com>; Justin He <Justin.He@arm.com>; nd <nd@arm.com>;
> kvm-owner@vger.kernel.org
> Subject: Re: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64
> 
> On 2020-01-09 09:24, Marc Zyngier wrote:
> > On 2020-01-09 05:59, Jianyong Wu wrote:
> 
> [...]
> 
> >> So we focus it on arm64. Also I have never tested it on arm32 machine
> >> ( we lack of arm32 machine)
> >
> > I'm sure your employer can provide you with such a box. I can probably
> > even tell you which cupboard they are stored in... ;-)
> >
> >> Do you think it's necessary to enable ptp_kvm on arm32? If so, I can
> >> do that.
> >
> > I can't see why we wouldn't, given that it should be a zero effort
> > task (none of the code here is arch specific).
> 
> To be clear, what I'm after is support for 32bit *guests*. I don't expect any
> issue with a 32bit host (it's all common code), but you should be able to test
> 32bit guests pretty easily (most ARMv8.0 CPUs support 32bit EL1).
> 
Get it

Thanks
Jianyong 

>          M.
> --
> Jazz is not dead. It just smells funny...

Re: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64

[Marc Zyngier]

Hi Jianyong,

On 2020-01-10 10:15, Jianyong Wu wrote:
> Hi Marc,

[...]

>> >> > +	ktime_overall = hvc_res.a0 << 32 | hvc_res.a1;
>> >> > +	*ts = ktime_to_timespec64(ktime_overall);
>> >> > +	*cycle = hvc_res.a2 << 32 | hvc_res.a3;
>> >>
>> >> So why isn't that just a read of the virtual counter, given that what
>> >> you do in the hypervisor seems to be "cntpct - cntvoff"?
>> >>
>> >> What am I missing here?
>> >>
>> > We need get clock time and counter cycle at the same time, so we can't
>> > just read virtual counter at guest and must get it from host.
>> 
>> See my comment in my reply to patch #6: *Must* seems like a very 
>> strong
>> word, and you don't explain *why* that's better than just computing 
>> the
>> total hypercall cost. Hint: given the frequency of the counter (in the 
>> few MHz
>> range) vs the frequency of a CPU (in the multiple GHz range, and with 
>> an IPC
>> close enough to 1), I doubt that you'll see the counter making much 
>> progress
>> across a hypercall.
>> 
> Sorry, I will avoid to use those strong words.
> 
> It's really the case that the hypercall won't across cycle in general.
> But sometimes, kernel preempt
> may happen in the middle of the hypercall which we can't assume how
> long before schedule back. so it's better capture them
> together at the same time.

Fair enough. Please document the rational, as I guess others will ask
the same questions.

Then the problem to solve is that of the reference counter, as you so 
far
assume the virtual counter. I guess you need to be able to let the guest
select the reference counter when calling the PTP service.

[...]

> By the way, does nested virtualization diff between arm64 and arm32?

There is no nested virt for 32bit (it is explicitly forbidden by the
architecture).

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC PATCH v9 6/8] psci: Add hvc call service for ptp_kvm.

[Marc Zyngier]

On 2020-01-10 09:51, Jianyong Wu wrote:
> Hi Marc,
> 
>> -----Original Message-----
>> From: Marc Zyngier <maz@kernel.org>
>> Sent: Thursday, January 9, 2020 5:16 PM
>> To: Jianyong Wu <Jianyong.Wu@arm.com>
>> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
>> tglx@linutronix.de; pbonzini@redhat.com; 
>> sean.j.christopherson@intel.com;
>> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>;
>> will@kernel.org; Suzuki Poulose <Suzuki.Poulose@arm.com>; Steven Price
>> <Steven.Price@arm.com>; linux-kernel@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu;
>> kvm@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>; Kaly Xin
>> <Kaly.Xin@arm.com>; Justin He <Justin.He@arm.com>; nd <nd@arm.com>
>> Subject: Re: [RFC PATCH v9 6/8] psci: Add hvc call service for 
>> ptp_kvm.
>> 
>> On 2020-01-09 05:45, Jianyong Wu wrote:
>> > Hi Marc,
>> >
>> >> -----Original Message-----
>> >> From: Marc Zyngier <maz@kernel.org>
>> >> Sent: Tuesday, January 7, 2020 5:16 PM
>> >> To: Jianyong Wu <Jianyong.Wu@arm.com>
>> >> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com;
>> >> john.stultz@linaro.org; tglx@linutronix.de; pbonzini@redhat.com;
>> >> sean.j.christopherson@intel.com; richardcochran@gmail.com; Mark
>> >> Rutland <Mark.Rutland@arm.com>; will@kernel.org; Suzuki Poulose
>> >> <Suzuki.Poulose@arm.com>; Steven Price <Steven.Price@arm.com>;
>> >> linux-kernel@vger.kernel.org; linux-arm- kernel@lists.infradead.org;
>> >> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; Steve Capper
>> >> <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Justin He
>> >> <Justin.He@arm.com>; nd <nd@arm.com>
>> >> Subject: Re: [RFC PATCH v9 6/8] psci: Add hvc call service for
>> >> ptp_kvm.
>> >>
>> >> On 2019-12-10 03:40, Jianyong Wu wrote:
>> >> > ptp_kvm modules will call hvc to get this service.
>> >> > The service offers real time and counter cycle of host for guest.
>> >> >
>> >> > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
>> >> > ---
>> >> >  include/linux/arm-smccc.h | 12 ++++++++++++
>> >> >  virt/kvm/arm/psci.c       | 22 ++++++++++++++++++++++
>> >> >  2 files changed, 34 insertions(+)
>> >> >
>> >> > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
>> >> > index 6f82c87308ed..aafb6bac167d 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
>> >> >
>> >> > @@ -103,6 +104,17 @@
>> >> >  			   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)
>> >> > +
>> >>
>> >> All of this depends on patches that have never need posted to any ML,
>> >> and just linger in Will's tree. You need to pick them up and post
>> >> them as part of this series so that they can at least be reviewed.
>> >>
>> > Ok, I will add them next version.
>> >
>> >> >  #ifndef __ASSEMBLY__
>> >> >
>> >> >  #include <linux/linkage.h>
>> >> > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c index
>> >> > 0debf49bf259..682d892d6717 100644
>> >> > --- a/virt/kvm/arm/psci.c
>> >> > +++ b/virt/kvm/arm/psci.c
>> >> > @@ -9,6 +9,7 @@
>> >> >  #include <linux/kvm_host.h>
>> >> >  #include <linux/uaccess.h>
>> >> >  #include <linux/wait.h>
>> >> > +#include <linux/clocksource_ids.h>
>> >> >
>> >> >  #include <asm/cputype.h>
>> >> >  #include <asm/kvm_emulate.h>
>> >> > @@ -389,6 +390,8 @@ static int kvm_psci_call(struct kvm_vcpu *vcpu)
>> >> >
>> >> >  int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)  {
>> >> > +	struct system_time_snapshot systime_snapshot;
>> >> > +	u64 cycles;
>> >> >  	u32 func_id = smccc_get_function(vcpu);
>> >> >  	u32 val[4] = {};
>> >> >  	u32 option;
>> >> > @@ -431,6 +434,25 @@ 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 high 32-bit host ktime;
>> >> > +	 * reg1 stores low 32-bit host ktime;
>> >> > +	 * reg2 stores high 32-bit difference of host cycles and cntvoff;
>> >> > +	 * reg3 stores low 32-bit difference of host cycles and cntvoff.
>> >>
>> >> That's either two or four values, and not three as you claim above.
>> >>
>> > Sorry, I'm not sure what do you mean "three", the registers here is 4
>> > from reg0 to reg3.
>> 
>> Please read the comment you have written above...
> 
> oh, I see it.
> 
>> 
>> >> Also, I fail to understand the meaning of the host cycle vs cntvoff
>> >> comparison.
>> >> This is something that guest can perform on its own (it has access to
>> >> both physical and virtual timers, and can compute cntvoff without
>> >> intervention of the hypervisor).
>> >>
>> > To keep consistency and precision, clock time and counter cycle must
>> > captured at the same time. It will perform at ktime_get_snapshot.
>> 
>> Fair enough. It would vertainly help if you documented it. It would 
>> also help if
>> you explained why it is so much worse to read the counter in the guest
>> before *and* after the call, and assume that the clock time read 
>> happened
>> right in the middle?
>> 
> ok, I will give explain in comments.
> 
>> That aside, what you are returning is something that *looks* like the 
>> virtual
>> counter. What if the guest is using the physical counter, which is 
>> likely to be
>> the case with nested virt? Do you expect the guest to always use the 
>> virtual
>> counter? This isn't going to fly.
> 
> To be honest, I have little knowledge of nested virtualization for arm
> and I'm confused with that guest'guest will use physical counter.

Not the guest's guest (L2), but L1. Just look at what counter the
KVM host uses: that's the physical counter. Now imagine you run that
host as a guest, no other change.

> IMO, ptp_kvm will call hvc to trap to its hypervisor adjacent to it.
> So guest'guest will trap to hypervisor in guest and will
> get guest's counter cycle then calculate guest'guest's counter cycle
> by something like offset to sync time with it. So only if the
> guest's hypervisor can calculate the guest'guest's counter value, can
> ptp_kvm works.

Sure, but that's not the problem we're trying to solve. The issue is 
that
of the reference counter value you're including in the hypercall 
response.
It needs to be a value that makes sense to the guest, and so far you're
assuming virtual.

NV breaks that assumtion, because the guest hypervisor is using the 
physical
counter. Also, let's not forget that the hypercall isn't Linux specific.
I can write my own non-Linux guest and still use this hypercall. Nothing
in there says that I can't use the physical counter if I want to.

So somehow, you need to convey the the hypervisor the notion of *which*
counter the guest uses.

Does it make sense? Or am I missing something?

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

RE: [RFC PATCH v9 6/8] psci: Add hvc call service for ptp_kvm.

[Jianyong Wu]

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Friday, January 10, 2020 6:56 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>
> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
> tglx@linutronix.de; pbonzini@redhat.com; sean.j.christopherson@intel.com;
> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>;
> will@kernel.org; Suzuki Poulose <Suzuki.Poulose@arm.com>; Steven Price
> <Steven.Price@arm.com>; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu;
> kvm@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>; Kaly Xin
> <Kaly.Xin@arm.com>; Justin He <Justin.He@arm.com>; nd <nd@arm.com>
> Subject: Re: [RFC PATCH v9 6/8] psci: Add hvc call service for ptp_kvm.
> 
> On 2020-01-10 09:51, Jianyong Wu wrote:
> > Hi Marc,
> >
> >> -----Original Message-----
> >> From: Marc Zyngier <maz@kernel.org>
> >> Sent: Thursday, January 9, 2020 5:16 PM
> >> To: Jianyong Wu <Jianyong.Wu@arm.com>
> >> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com;
> >> john.stultz@linaro.org; tglx@linutronix.de; pbonzini@redhat.com;
> >> sean.j.christopherson@intel.com; richardcochran@gmail.com; Mark
> >> Rutland <Mark.Rutland@arm.com>; will@kernel.org; Suzuki Poulose
> >> <Suzuki.Poulose@arm.com>; Steven Price <Steven.Price@arm.com>;
> >> linux-kernel@vger.kernel.org; linux-arm- kernel@lists.infradead.org;
> >> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; Steve Capper
> >> <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Justin He
> >> <Justin.He@arm.com>; nd <nd@arm.com>
> >> Subject: Re: [RFC PATCH v9 6/8] psci: Add hvc call service for
> >> ptp_kvm.
> >>
> >> On 2020-01-09 05:45, Jianyong Wu wrote:
> >> > Hi Marc,
> >> >
> >> >> -----Original Message-----
> >> >> From: Marc Zyngier <maz@kernel.org>
> >> >> Sent: Tuesday, January 7, 2020 5:16 PM
> >> >> To: Jianyong Wu <Jianyong.Wu@arm.com>
> >> >> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com;
> >> >> john.stultz@linaro.org; tglx@linutronix.de; pbonzini@redhat.com;
> >> >> sean.j.christopherson@intel.com; richardcochran@gmail.com; Mark
> >> >> Rutland <Mark.Rutland@arm.com>; will@kernel.org; Suzuki Poulose
> >> >> <Suzuki.Poulose@arm.com>; Steven Price <Steven.Price@arm.com>;
> >> >> linux-kernel@vger.kernel.org; linux-arm-
> >> >> kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu;
> >> >> kvm@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>; Kaly
> Xin
> >> >> <Kaly.Xin@arm.com>; Justin He <Justin.He@arm.com>; nd
> <nd@arm.com>
> >> >> Subject: Re: [RFC PATCH v9 6/8] psci: Add hvc call service for
> >> >> ptp_kvm.
> >> >>
> >> >> On 2019-12-10 03:40, Jianyong Wu wrote:
> >> >> > ptp_kvm modules will call hvc to get this service.
> >> >> > The service offers real time and counter cycle of host for guest.
> >> >> >
> >> >> > Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> >> >> > ---
> >> >> >  include/linux/arm-smccc.h | 12 ++++++++++++
> >> >> >  virt/kvm/arm/psci.c       | 22 ++++++++++++++++++++++
> >> >> >  2 files changed, 34 insertions(+)
> >> >> >
> >> >> > diff --git a/include/linux/arm-smccc.h
> >> >> > b/include/linux/arm-smccc.h index 6f82c87308ed..aafb6bac167d
> >> >> > 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
> >> >> >
> >> >> > @@ -103,6 +104,17 @@
> >> >> >  			   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)
> >> >> > +
> >> >>
> >> >> All of this depends on patches that have never need posted to any
> >> >> ML, and just linger in Will's tree. You need to pick them up and
> >> >> post them as part of this series so that they can at least be reviewed.
> >> >>
> >> > Ok, I will add them next version.
> >> >
> >> >> >  #ifndef __ASSEMBLY__
> >> >> >
> >> >> >  #include <linux/linkage.h>
> >> >> > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c index
> >> >> > 0debf49bf259..682d892d6717 100644
> >> >> > --- a/virt/kvm/arm/psci.c
> >> >> > +++ b/virt/kvm/arm/psci.c
> >> >> > @@ -9,6 +9,7 @@
> >> >> >  #include <linux/kvm_host.h>
> >> >> >  #include <linux/uaccess.h>
> >> >> >  #include <linux/wait.h>
> >> >> > +#include <linux/clocksource_ids.h>
> >> >> >
> >> >> >  #include <asm/cputype.h>
> >> >> >  #include <asm/kvm_emulate.h>
> >> >> > @@ -389,6 +390,8 @@ static int kvm_psci_call(struct kvm_vcpu
> >> >> > *vcpu)
> >> >> >
> >> >> >  int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)  {
> >> >> > +	struct system_time_snapshot systime_snapshot;
> >> >> > +	u64 cycles;
> >> >> >  	u32 func_id = smccc_get_function(vcpu);
> >> >> >  	u32 val[4] = {};
> >> >> >  	u32 option;
> >> >> > @@ -431,6 +434,25 @@ 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 high 32-bit host ktime;
> >> >> > +	 * reg1 stores low 32-bit host ktime;
> >> >> > +	 * reg2 stores high 32-bit difference of host cycles and cntvoff;
> >> >> > +	 * reg3 stores low 32-bit difference of host cycles and cntvoff.
> >> >>
> >> >> That's either two or four values, and not three as you claim above.
> >> >>
> >> > Sorry, I'm not sure what do you mean "three", the registers here is
> >> > 4 from reg0 to reg3.
> >>
> >> Please read the comment you have written above...
> >
> > oh, I see it.
> >
> >>
> >> >> Also, I fail to understand the meaning of the host cycle vs
> >> >> cntvoff comparison.
> >> >> This is something that guest can perform on its own (it has access
> >> >> to both physical and virtual timers, and can compute cntvoff
> >> >> without intervention of the hypervisor).
> >> >>
> >> > To keep consistency and precision, clock time and counter cycle
> >> > must captured at the same time. It will perform at ktime_get_snapshot.
> >>
> >> Fair enough. It would vertainly help if you documented it. It would
> >> also help if you explained why it is so much worse to read the
> >> counter in the guest before *and* after the call, and assume that the
> >> clock time read happened right in the middle?
> >>
> > ok, I will give explain in comments.
> >
> >> That aside, what you are returning is something that *looks* like the
> >> virtual counter. What if the guest is using the physical counter,
> >> which is likely to be the case with nested virt? Do you expect the
> >> guest to always use the virtual counter? This isn't going to fly.
> >
> > To be honest, I have little knowledge of nested virtualization for arm
> > and I'm confused with that guest'guest will use physical counter.
> 
> Not the guest's guest (L2), but L1. Just look at what counter the KVM host
> uses: that's the physical counter. Now imagine you run that host as a guest,
> no other change.
> 
Ok,

> > IMO, ptp_kvm will call hvc to trap to its hypervisor adjacent to it.
> > So guest'guest will trap to hypervisor in guest and will get guest's
> > counter cycle then calculate guest'guest's counter cycle by something
> > like offset to sync time with it. So only if the guest's hypervisor
> > can calculate the guest'guest's counter value, can ptp_kvm works.
> 
> Sure, but that's not the problem we're trying to solve. The issue is that of the
> reference counter value you're including in the hypercall response.
> It needs to be a value that makes sense to the guest, and so far you're
> assuming virtual.
>
Get it.
 
> NV breaks that assumtion, because the guest hypervisor is using the physical
> counter. Also, let's not forget that the hypercall isn't Linux specific.
> I can write my own non-Linux guest and still use this hypercall. Nothing in
> there says that I can't use the physical counter if I want to.
> 
> So somehow, you need to convey the the hypervisor the notion of *which*
> counter the guest uses.
> 
> Does it make sense? Or am I missing something?
>
I know what you say. Let me try to solve this problem.
	Step 0, summary out all the conditions we should process, which will sever as branch condition.(now only normal virt and nested virt, I think) 
	Step 1, figure out the set of reference counter value used by guest in all condition.
	Step 2, determine which reference counter value will be used by guest in a certain condition in hypercall.
In step 1, can we give the set only 2 elements that one is physical counter the other is virtual counter?
For step 2, I have no idea for that now. can you give me some hint about it?

Thanks
Jianyong 
 
> Thanks,
> 
>          M.
> --
> Jazz is not dead. It just smells funny...

RE: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64

[Jianyong Wu]

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Friday, January 10, 2020 6:35 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>
> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
> tglx@linutronix.de; pbonzini@redhat.com; sean.j.christopherson@intel.com;
> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>;
> will@kernel.org; Suzuki Poulose <Suzuki.Poulose@arm.com>; Steven Price
> <Steven.Price@arm.com>; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu;
> kvm@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>; Kaly Xin
> <Kaly.Xin@arm.com>; Justin He <Justin.He@arm.com>; nd <nd@arm.com>
> Subject: Re: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64
> 
> Hi Jianyong,
> 
> On 2020-01-10 10:15, Jianyong Wu wrote:
> > Hi Marc,
> 
> [...]
> 
> >> >> > +	ktime_overall = hvc_res.a0 << 32 | hvc_res.a1;
> >> >> > +	*ts = ktime_to_timespec64(ktime_overall);
> >> >> > +	*cycle = hvc_res.a2 << 32 | hvc_res.a3;
> >> >>
> >> >> So why isn't that just a read of the virtual counter, given that
> >> >> what you do in the hypervisor seems to be "cntpct - cntvoff"?
> >> >>
> >> >> What am I missing here?
> >> >>
> >> > We need get clock time and counter cycle at the same time, so we
> >> > can't just read virtual counter at guest and must get it from host.
> >>
> >> See my comment in my reply to patch #6: *Must* seems like a very
> >> strong word, and you don't explain *why* that's better than just
> >> computing the total hypercall cost. Hint: given the frequency of the
> >> counter (in the few MHz
> >> range) vs the frequency of a CPU (in the multiple GHz range, and with
> >> an IPC close enough to 1), I doubt that you'll see the counter making
> >> much progress across a hypercall.
> >>
> > Sorry, I will avoid to use those strong words.
> >
> > It's really the case that the hypercall won't across cycle in general.
> > But sometimes, kernel preempt
> > may happen in the middle of the hypercall which we can't assume how
> > long before schedule back. so it's better capture them together at the
> > same time.
> 
> Fair enough. Please document the rational, as I guess others will ask the
> same questions.
> 
Ok

> Then the problem to solve is that of the reference counter, as you so far
> assume the virtual counter. I guess you need to be able to let the guest
> select the reference counter when calling the PTP service.
> 
I could not come up with an idea about the point where the guest give this info of counter value.
Where we give that interface to ptp service, as it's not a user space application.

> [...]
> 
> > By the way, does nested virtualization diff between arm64 and arm32?
> 
> There is no nested virt for 32bit (it is explicitly forbidden by the architecture).
>  
Ok, thanks

Thanks
Jianyong 

>          M.
> --
> Jazz is not dead. It just smells funny...

Re: [RFC PATCH v9 6/8] psci: Add hvc call service for ptp_kvm.

[Marc Zyngier]

Hi Jianyong,

On 2020-01-13 10:30, Jianyong Wu wrote:
> Hi Marc,
> 
>> -----Original Message-----
>> From: Marc Zyngier <maz@kernel.org>
>> Sent: Friday, January 10, 2020 6:56 PM
>> NV breaks that assumtion, because the guest hypervisor is using the 
>> physical
>> counter. Also, let's not forget that the hypercall isn't Linux 
>> specific.
>> I can write my own non-Linux guest and still use this hypercall. 
>> Nothing in
>> there says that I can't use the physical counter if I want to.
>> 
>> So somehow, you need to convey the the hypervisor the notion of 
>> *which*
>> counter the guest uses.
>> 
>> Does it make sense? Or am I missing something?
>> 
> I know what you say. Let me try to solve this problem.
> 	Step 0, summary out all the conditions we should process, which will
> sever as branch condition.(now only normal virt and nested virt, I
> think)

No. You shouldn't think of the various use cases, but of which time
references a guest can use. You don't need nested virt to use the 
physical
counter, for example.

> 	Step 1, figure out the set of reference counter value used by guest
> in all condition.

That should be for the guest to tell you when it calls into the PV 
service.

> 	Step 2, determine which reference counter value will be used by guest
> in a certain condition in hypercall.
> In step 1, can we give the set only 2 elements that one is physical
> counter the other is virtual counter?

I don't think returning the two values is useful. Just return what the
guest asks for.

> For step 2, I have no idea for that now. can you give me some hint 
> about it?

Just expand your SMC call to take a parameter indicating the reference
counter, and return the sampled (or computed) value corresponding to
that counter.

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64

[Marc Zyngier]

On 2020-01-13 10:37, Jianyong Wu wrote:
> Hi Marc,
> 
>> -----Original Message-----
>> From: Marc Zyngier <maz@kernel.org>
>> Sent: Friday, January 10, 2020 6:35 PM
>> To: Jianyong Wu <Jianyong.Wu@arm.com>
>> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
>> tglx@linutronix.de; pbonzini@redhat.com; 
>> sean.j.christopherson@intel.com;
>> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>;
>> will@kernel.org; Suzuki Poulose <Suzuki.Poulose@arm.com>; Steven Price
>> <Steven.Price@arm.com>; linux-kernel@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu;
>> kvm@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>; Kaly Xin
>> <Kaly.Xin@arm.com>; Justin He <Justin.He@arm.com>; nd <nd@arm.com>
>> Subject: Re: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64
>> 
>> Hi Jianyong,
>> 
>> On 2020-01-10 10:15, Jianyong Wu wrote:
>> > Hi Marc,
>> 
>> [...]
>> 
>> >> >> > +	ktime_overall = hvc_res.a0 << 32 | hvc_res.a1;
>> >> >> > +	*ts = ktime_to_timespec64(ktime_overall);
>> >> >> > +	*cycle = hvc_res.a2 << 32 | hvc_res.a3;
>> >> >>
>> >> >> So why isn't that just a read of the virtual counter, given that
>> >> >> what you do in the hypervisor seems to be "cntpct - cntvoff"?
>> >> >>
>> >> >> What am I missing here?
>> >> >>
>> >> > We need get clock time and counter cycle at the same time, so we
>> >> > can't just read virtual counter at guest and must get it from host.
>> >>
>> >> See my comment in my reply to patch #6: *Must* seems like a very
>> >> strong word, and you don't explain *why* that's better than just
>> >> computing the total hypercall cost. Hint: given the frequency of the
>> >> counter (in the few MHz
>> >> range) vs the frequency of a CPU (in the multiple GHz range, and with
>> >> an IPC close enough to 1), I doubt that you'll see the counter making
>> >> much progress across a hypercall.
>> >>
>> > Sorry, I will avoid to use those strong words.
>> >
>> > It's really the case that the hypercall won't across cycle in general.
>> > But sometimes, kernel preempt
>> > may happen in the middle of the hypercall which we can't assume how
>> > long before schedule back. so it's better capture them together at the
>> > same time.
>> 
>> Fair enough. Please document the rational, as I guess others will ask 
>> the
>> same questions.
>> 
> Ok
> 
>> Then the problem to solve is that of the reference counter, as you so 
>> far
>> assume the virtual counter. I guess you need to be able to let the 
>> guest
>> select the reference counter when calling the PTP service.
>> 
> I could not come up with an idea about the point where the guest give
> this info of counter value.
> Where we give that interface to ptp service, as it's not a user space
> application.

Again: why don't you let the guest ask for the counter it wants as part
of the SMC call? What is preventing this?

         M.
-- 
Jazz is not dead. It just smells funny...

RE: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64

[Jianyong Wu]

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Monday, January 13, 2020 7:21 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>
> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
> tglx@linutronix.de; pbonzini@redhat.com; sean.j.christopherson@intel.com;
> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>;
> will@kernel.org; Suzuki Poulose <Suzuki.Poulose@arm.com>; Steven Price
> <Steven.Price@arm.com>; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu;
> kvm@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>; Kaly Xin
> <Kaly.Xin@arm.com>; Justin He <Justin.He@arm.com>; nd <nd@arm.com>
> Subject: Re: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64
> 
> On 2020-01-13 10:37, Jianyong Wu wrote:
> > Hi Marc,
> >
> >> -----Original Message-----
> >> From: Marc Zyngier <maz@kernel.org>
> >> Sent: Friday, January 10, 2020 6:35 PM
> >> To: Jianyong Wu <Jianyong.Wu@arm.com>
> >> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com;
> >> john.stultz@linaro.org; tglx@linutronix.de; pbonzini@redhat.com;
> >> sean.j.christopherson@intel.com; richardcochran@gmail.com; Mark
> >> Rutland <Mark.Rutland@arm.com>; will@kernel.org; Suzuki Poulose
> >> <Suzuki.Poulose@arm.com>; Steven Price <Steven.Price@arm.com>;
> >> linux-kernel@vger.kernel.org; linux-arm- kernel@lists.infradead.org;
> >> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; Steve Capper
> >> <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Justin He
> >> <Justin.He@arm.com>; nd <nd@arm.com>
> >> Subject: Re: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64
> >>
> >> Hi Jianyong,
> >>
> >> On 2020-01-10 10:15, Jianyong Wu wrote:
> >> > Hi Marc,
> >>
> >> [...]
> >>
> >> >> >> > +	ktime_overall = hvc_res.a0 << 32 | hvc_res.a1;
> >> >> >> > +	*ts = ktime_to_timespec64(ktime_overall);
> >> >> >> > +	*cycle = hvc_res.a2 << 32 | hvc_res.a3;
> >> >> >>
> >> >> >> So why isn't that just a read of the virtual counter, given
> >> >> >> that what you do in the hypervisor seems to be "cntpct - cntvoff"?
> >> >> >>
> >> >> >> What am I missing here?
> >> >> >>
> >> >> > We need get clock time and counter cycle at the same time, so we
> >> >> > can't just read virtual counter at guest and must get it from host.
> >> >>
> >> >> See my comment in my reply to patch #6: *Must* seems like a very
> >> >> strong word, and you don't explain *why* that's better than just
> >> >> computing the total hypercall cost. Hint: given the frequency of
> >> >> the counter (in the few MHz
> >> >> range) vs the frequency of a CPU (in the multiple GHz range, and
> >> >> with an IPC close enough to 1), I doubt that you'll see the
> >> >> counter making much progress across a hypercall.
> >> >>
> >> > Sorry, I will avoid to use those strong words.
> >> >
> >> > It's really the case that the hypercall won't across cycle in general.
> >> > But sometimes, kernel preempt
> >> > may happen in the middle of the hypercall which we can't assume how
> >> > long before schedule back. so it's better capture them together at
> >> > the same time.
> >>
> >> Fair enough. Please document the rational, as I guess others will ask
> >> the same questions.
> >>
> > Ok
> >
> >> Then the problem to solve is that of the reference counter, as you so
> >> far assume the virtual counter. I guess you need to be able to let
> >> the guest select the reference counter when calling the PTP service.
> >>
> > I could not come up with an idea about the point where the guest give
> > this info of counter value.
> > Where we give that interface to ptp service, as it's not a user space
> > application.
> 
> Again: why don't you let the guest ask for the counter it wants as part of the
> SMC call? What is preventing this?
> 
Ok, let me try it

Thanks

>          M.
> --
> Jazz is not dead. It just smells funny...

RE: [RFC PATCH v9 6/8] psci: Add hvc call service for ptp_kvm.

[Jianyong Wu]

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Monday, January 13, 2020 7:16 PM
> To: Jianyong Wu <Jianyong.Wu@arm.com>
> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
> tglx@linutronix.de; pbonzini@redhat.com; sean.j.christopherson@intel.com;
> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>;
> will@kernel.org; Suzuki Poulose <Suzuki.Poulose@arm.com>; Steven Price
> <Steven.Price@arm.com>; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu;
> kvm@vger.kernel.org; Steve Capper <Steve.Capper@arm.com>; Kaly Xin
> <Kaly.Xin@arm.com>; Justin He <Justin.He@arm.com>; nd <nd@arm.com>
> Subject: Re: [RFC PATCH v9 6/8] psci: Add hvc call service for ptp_kvm.
> 
> Hi Jianyong,
> 
> On 2020-01-13 10:30, Jianyong Wu wrote:
> > Hi Marc,
> >
> >> -----Original Message-----
> >> From: Marc Zyngier <maz@kernel.org>
> >> Sent: Friday, January 10, 2020 6:56 PM NV breaks that assumtion,
> >> because the guest hypervisor is using the physical counter. Also,
> >> let's not forget that the hypercall isn't Linux specific.
> >> I can write my own non-Linux guest and still use this hypercall.
> >> Nothing in
> >> there says that I can't use the physical counter if I want to.
> >>
> >> So somehow, you need to convey the the hypervisor the notion of
> >> *which*
> >> counter the guest uses.
> >>
> >> Does it make sense? Or am I missing something?
> >>
> > I know what you say. Let me try to solve this problem.
> > 	Step 0, summary out all the conditions we should process, which will
> > sever as branch condition.(now only normal virt and nested virt, I
> > think)
> 
> No. You shouldn't think of the various use cases, but of which time
> references a guest can use. You don't need nested virt to use the physical
> counter, for example.
Ok,

> 
> > 	Step 1, figure out the set of reference counter value used by guest
> > in all condition.
> 
> That should be for the guest to tell you when it calls into the PV service.
> 
Yeah

> > 	Step 2, determine which reference counter value will be used by
> guest
> > in a certain condition in hypercall.
> > In step 1, can we give the set only 2 elements that one is physical
> > counter the other is virtual counter?
> 
> I don't think returning the two values is useful. Just return what the guest
> asks for.
> 
> > For step 2, I have no idea for that now. can you give me some hint
> > about it?
> 
> Just expand your SMC call to take a parameter indicating the reference
> counter, and return the sampled (or computed) value corresponding to that
> counter.
Get it, I'll try it.

Thanks
Jianyong 

> 
>          M.
> --
> Jazz is not dead. It just smells funny...