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 <[email protected]>
---
arch/arm64/include/asm/arch_timer.h | 3 ++
arch/arm64/kvm/arch_ptp_kvm.c | 76 ++++++++++++++++++++++++++++
drivers/clocksource/arm_arch_timer.c | 6 ++-
drivers/ptp/Kconfig | 2 +-
include/linux/arm-smccc.h | 14 +++++
virt/kvm/arm/psci.c | 17 +++++++
6 files changed, 115 insertions(+), 3 deletions(-)
create mode 100644 arch/arm64/kvm/arch_ptp_kvm.c
diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 6756178c27db..880576a814b6 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -229,4 +229,7 @@ static inline int arch_timer_arch_init(void)
return 0;
}
+extern struct clocksource clocksource_counter;
+extern u64 arch_counter_read(struct clocksource *cs);
+
#endif
diff --git a/arch/arm64/kvm/arch_ptp_kvm.c b/arch/arm64/kvm/arch_ptp_kvm.c
new file mode 100644
index 000000000000..6b2165ebce62
--- /dev/null
+++ b/arch/arm64/kvm/arch_ptp_kvm.c
@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Virtual PTP 1588 clock for use with KVM guests
+ * Copyright (C) 2019 ARM Ltd.
+ * All Rights Reserved
+ */
+
+#include <asm/hypervisor.h>
+#include <linux/module.h>
+#include <linux/psci.h>
+#include <linux/arm-smccc.h>
+#include <linux/timecounter.h>
+#include <linux/sched/clock.h>
+#include <asm/arch_timer.h>
+
+/*
+ * as trap call cause delay, this function will return the delay in nanosecond
+ */
+static u64 arm_smccc_1_1_invoke_delay(u32 id, struct arm_smccc_res *res)
+{
+ u64 ns, t1, t2;
+
+ t1 = sched_clock();
+ arm_smccc_1_1_invoke(id, res);
+ t2 = sched_clock();
+ t2 -= t1;
+ ns = t2;
+ return ns;
+}
+
+int kvm_arch_ptp_init(void)
+{
+ return 0;
+}
+
+int kvm_arch_ptp_get_clock(struct timespec64 *ts)
+{
+ u64 ns;
+ struct arm_smccc_res hvc_res;
+
+ if (!kvm_arm_hyp_service_available(
+ ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID)) {
+ return -EOPNOTSUPP;
+ }
+ ns = arm_smccc_1_1_invoke_delay(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
+ &hvc_res);
+ ts->tv_sec = hvc_res.a0;
+ ts->tv_nsec = hvc_res.a1;
+ timespec64_add_ns(ts, ns);
+ return 0;
+}
+
+int kvm_arch_ptp_get_clock_fn(long *cycle, struct timespec64 *ts,
+ struct clocksource **cs)
+{
+ u64 ns;
+ struct arm_smccc_res hvc_res;
+
+ if (!kvm_arm_hyp_service_available(
+ ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID)) {
+ return -EOPNOTSUPP;
+ }
+ ns = arm_smccc_1_1_invoke_delay(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
+ &hvc_res);
+ ts->tv_sec = hvc_res.a0;
+ ts->tv_nsec = hvc_res.a1;
+ timespec64_add_ns(ts, ns);
+ *cycle = hvc_res.a2;
+ *cs = &clocksource_counter;
+
+ return 0;
+}
+
+MODULE_AUTHOR("Marcelo Tosatti <[email protected]>");
+MODULE_DESCRIPTION("PTP clock using KVMCLOCK");
+MODULE_LICENSE("GPL");
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 07e57a49d1e8..021e3f69364c 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -175,23 +175,25 @@ static notrace u64 arch_counter_get_cntvct(void)
u64 (*arch_timer_read_counter)(void) = arch_counter_get_cntvct;
EXPORT_SYMBOL_GPL(arch_timer_read_counter);
-static u64 arch_counter_read(struct clocksource *cs)
+u64 arch_counter_read(struct clocksource *cs)
{
return arch_timer_read_counter();
}
+EXPORT_SYMBOL(arch_counter_read);
static u64 arch_counter_read_cc(const struct cyclecounter *cc)
{
return arch_timer_read_counter();
}
-static struct clocksource clocksource_counter = {
+struct clocksource clocksource_counter = {
.name = "arch_sys_counter",
.rating = 400,
.read = arch_counter_read,
.mask = CLOCKSOURCE_MASK(56),
.flags = CLOCK_SOURCE_IS_CONTINUOUS,
};
+EXPORT_SYMBOL(clocksource_counter);
static struct cyclecounter cyclecounter __ro_after_init = {
.read = arch_counter_read_cc,
diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index 9b8fee5178e8..e032fafdafa7 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -110,7 +110,7 @@ config PTP_1588_CLOCK_PCH
config PTP_1588_CLOCK_KVM
tristate "KVM virtual PTP clock"
depends on PTP_1588_CLOCK
- depends on KVM_GUEST && X86
+ depends on KVM_GUEST && X86 || ARM64
default y
help
This driver adds support for using kvm infrastructure as a PTP
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index a6e4d3e3d10a..2a222a1a8594 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -94,6 +94,7 @@
/* KVM "vendor specific" services */
#define ARM_SMCCC_KVM_FUNC_FEATURES 0
+#define ARM_SMCCC_KVM_PTP 1
#define ARM_SMCCC_KVM_FUNC_FEATURES_2 127
#define ARM_SMCCC_KVM_NUM_FUNCS 128
@@ -102,6 +103,16 @@
ARM_SMCCC_SMC_32, \
ARM_SMCCC_OWNER_VENDOR_HYP, \
ARM_SMCCC_KVM_FUNC_FEATURES)
+/*
+ * This ID used for virtual ptp kvm clock and it will pass second value
+ * and nanosecond value of host real time and system counter by vcpu
+ * register to guest.
+ */
+#define ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID \
+ ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+ ARM_SMCCC_SMC_32, \
+ ARM_SMCCC_OWNER_VENDOR_HYP, \
+ ARM_SMCCC_KVM_PTP)
#ifndef __ASSEMBLY__
@@ -373,5 +384,8 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
method; \
})
+#include <linux/psci.h>
+#include <linux/clocksource.h>
+
#endif /*__ASSEMBLY__*/
#endif /*__LINUX_ARM_SMCCC_H*/
diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
index 0debf49bf259..7fffdb25d32c 100644
--- a/virt/kvm/arm/psci.c
+++ b/virt/kvm/arm/psci.c
@@ -392,6 +392,8 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
u32 func_id = smccc_get_function(vcpu);
u32 val[4] = {};
u32 option;
+ struct timespec *ts;
+ u64 cnt;
val[0] = SMCCC_RET_NOT_SUPPORTED;
@@ -431,6 +433,21 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
case ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID:
val[0] = BIT(ARM_SMCCC_KVM_FUNC_FEATURES);
break;
+ /*
+ * This will used for virtual ptp kvm clock. three
+ * values will be passed back.
+ * reg0 stores seconds of host real time;
+ * reg1 stores nanoseconds of host real time;
+ * reg2 stotes system counter cycle value.
+ */
+ case ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID:
+ getnstimeofday(ts);
+ cnt = arch_timer_read_counter();
+ val[0] = ts->tv_sec;
+ val[1] = ts->tv_nsec;
+ val[2] = cnt;
+ val[3] = 0;
+ break;
default:
return kvm_psci_call(vcpu);
}
--
2.17.1
On 29/08/2019 07:39, Jianyong Wu wrote:
> Currently in arm64 virtualization environment, there is no mechanism to
> keep time sync between guest and host. Time in guest will drift compared
> with host after boot up as they may both use third party time sources
> to correct their time respectively. The time deviation will be in order
> of milliseconds but some scenarios ask for higher time precision, like
> in cloud envirenment, we want all the VMs running in the host aquire the
> same level accuracy from host clock.
>
> Use of kvm ptp clock, which choose the host clock source clock as a
> reference clock to sync time clock between guest and host has been adopted
> by x86 which makes the time sync order from milliseconds to nanoseconds.
>
> This patch enable kvm ptp on arm64 and we get the similar clock drift as
> found with x86 with kvm ptp.
>
> Test result comparison between with kvm ptp and without it in arm64 are
> as follows. This test derived from the result of command 'chronyc
> sources'. we should take more cure of the last sample column which shows
> the offset between the local clock and the source at the last measurement.
>
> no kvm ptp in guest:
> MS Name/IP address Stratum Poll Reach LastRx Last sample
> ========================================================================
> ^* dns1.synet.edu.cn 2 6 377 13 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 21 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 29 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 37 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 45 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 53 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 61 +1040us[+1581us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 4 -130us[ +796us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 12 -130us[ +796us] +/- 21ms
> ^* dns1.synet.edu.cn 2 6 377 20 -130us[ +796us] +/- 21ms
>
> in host:
> MS Name/IP address Stratum Poll Reach LastRx Last sample
> ========================================================================
> ^* 120.25.115.20 2 7 377 72 -470us[ -603us] +/- 18ms
> ^* 120.25.115.20 2 7 377 92 -470us[ -603us] +/- 18ms
> ^* 120.25.115.20 2 7 377 112 -470us[ -603us] +/- 18ms
> ^* 120.25.115.20 2 7 377 2 +872ns[-6808ns] +/- 17ms
> ^* 120.25.115.20 2 7 377 22 +872ns[-6808ns] +/- 17ms
> ^* 120.25.115.20 2 7 377 43 +872ns[-6808ns] +/- 17ms
> ^* 120.25.115.20 2 7 377 63 +872ns[-6808ns] +/- 17ms
> ^* 120.25.115.20 2 7 377 83 +872ns[-6808ns] +/- 17ms
> ^* 120.25.115.20 2 7 377 103 +872ns[-6808ns] +/- 17ms
> ^* 120.25.115.20 2 7 377 123 +872ns[-6808ns] +/- 17ms
>
> The dns1.synet.edu.cn is the network reference clock for guest and
> 120.25.115.20 is the network reference clock for host. we can't get the
> clock error between guest and host directly, but a roughly estimated value
> will be in order of hundreds of us to ms.
>
> with kvm ptp in guest:
> chrony has been disabled in host to remove the disturb by network clock.
Is that a realistic use case? Why should the host not use NTP?
>
> MS Name/IP address Stratum Poll Reach LastRx Last sample
> ========================================================================
> * PHC0 0 3 377 8 -7ns[ +1ns] +/- 3ns
> * PHC0 0 3 377 8 +1ns[ +16ns] +/- 3ns
> * PHC0 0 3 377 6 -4ns[ -0ns] +/- 6ns
> * PHC0 0 3 377 6 -8ns[ -12ns] +/- 5ns
> * PHC0 0 3 377 5 +2ns[ +4ns] +/- 4ns
> * PHC0 0 3 377 13 +2ns[ +4ns] +/- 4ns
> * PHC0 0 3 377 12 -4ns[ -6ns] +/- 4ns
> * PHC0 0 3 377 11 -8ns[ -11ns] +/- 6ns
> * PHC0 0 3 377 10 -14ns[ -20ns] +/- 4ns
> * PHC0 0 3 377 8 +4ns[ +5ns] +/- 4ns
>
> The PHC0 is the ptp clock which choose the host clock as its source
> clock. So we can be sure to say that the clock error between host and guest
> is in order of ns.
>
> Signed-off-by: Jianyong Wu <[email protected]>
> ---
> arch/arm64/include/asm/arch_timer.h | 3 ++
> arch/arm64/kvm/arch_ptp_kvm.c | 76 ++++++++++++++++++++++++++++
> drivers/clocksource/arm_arch_timer.c | 6 ++-
> drivers/ptp/Kconfig | 2 +-
> include/linux/arm-smccc.h | 14 +++++
> virt/kvm/arm/psci.c | 17 +++++++
> 6 files changed, 115 insertions(+), 3 deletions(-)
> create mode 100644 arch/arm64/kvm/arch_ptp_kvm.c
Please split this patch into two parts: the hypervisor code in a patch
and the guest code in another patch. Having both of them together is
confusing.
>
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 6756178c27db..880576a814b6 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -229,4 +229,7 @@ static inline int arch_timer_arch_init(void)
> return 0;
> }
>
> +extern struct clocksource clocksource_counter;
> +extern u64 arch_counter_read(struct clocksource *cs);
I'm definitely not keen on exposing the internals of the arch_timer
driver to random subsystems. Furthermore, you seem to expect that the
guest kernel will only use the arch timer as a clocksource, and nothing
really guarantees that (in which case get_device_system_crosststamp will
fail).
It looks to me that we'd be better off exposing a core timekeeping API
that populates a struct system_counterval_t based on the *current*
timekeeper monotonic clocksource. This would simplify the split between
generic and arch-specific code.
Whether or not tglx will be happy with the idea is another problem, but
I'm certainly not taking any change to the arch timer code based on this.
> +
> #endif
> diff --git a/arch/arm64/kvm/arch_ptp_kvm.c b/arch/arm64/kvm/arch_ptp_kvm.c
We don't put non-hypervisor in arch/arm64/kvm. Please move it back to
drivers/ptp (as well as its x86 counterpart), and just link the two
parts there. This should also allow this to be enabled for 32bit guests.
> new file mode 100644
> index 000000000000..6b2165ebce62
> --- /dev/null
> +++ b/arch/arm64/kvm/arch_ptp_kvm.c
> @@ -0,0 +1,76 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Virtual PTP 1588 clock for use with KVM guests
> + * Copyright (C) 2019 ARM Ltd.
> + * All Rights Reserved
> + */
> +
> +#include <asm/hypervisor.h>
> +#include <linux/module.h>
> +#include <linux/psci.h>
> +#include <linux/arm-smccc.h>
> +#include <linux/timecounter.h>
> +#include <linux/sched/clock.h>
> +#include <asm/arch_timer.h>
> +
> +/*
> + * as trap call cause delay, this function will return the delay in nanosecond
> + */
> +static u64 arm_smccc_1_1_invoke_delay(u32 id, struct arm_smccc_res *res)
> +{
> + u64 ns, t1, t2;
> +
> + t1 = sched_clock();
> + arm_smccc_1_1_invoke(id, res);
> + t2 = sched_clock();
> + t2 -= t1;
> + ns = t2;
> + return ns;
I think you can get rid of the ns variable here...
> +}
> +
> +int kvm_arch_ptp_init(void)
> +{
> + return 0;
> +}
> +
> +int kvm_arch_ptp_get_clock(struct timespec64 *ts)
> +{
> + u64 ns;
> + struct arm_smccc_res hvc_res;
> +
> + if (!kvm_arm_hyp_service_available(
> + ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID)) {
> + return -EOPNOTSUPP;
> + }
> + ns = arm_smccc_1_1_invoke_delay(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
> + &hvc_res);
> + ts->tv_sec = hvc_res.a0;
> + ts->tv_nsec = hvc_res.a1;
> + timespec64_add_ns(ts, ns);
> + return 0;
> +}
> +
> +int kvm_arch_ptp_get_clock_fn(long *cycle, struct timespec64 *ts,
> + struct clocksource **cs)
> +{
> + u64 ns;
> + struct arm_smccc_res hvc_res;
> +
> + if (!kvm_arm_hyp_service_available(
> + ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID)) {
> + return -EOPNOTSUPP;
> + }
> + ns = arm_smccc_1_1_invoke_delay(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID,
> + &hvc_res);
> + ts->tv_sec = hvc_res.a0;
> + ts->tv_nsec = hvc_res.a1;
> + timespec64_add_ns(ts, ns);
> + *cycle = hvc_res.a2;
> + *cs = &clocksource_counter;
> +
> + return 0;
> +}
Why do we have two functions doing almost the same thing? Why do you
call kvm_arm_hyp_service_available on each and every time? Isn't it
enough to check in kvm_arch_ptp_init()?
> +
> +MODULE_AUTHOR("Marcelo Tosatti <[email protected]>");
> +MODULE_DESCRIPTION("PTP clock using KVMCLOCK");
> +MODULE_LICENSE("GPL");
This should only exist in the generic code.
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 07e57a49d1e8..021e3f69364c 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -175,23 +175,25 @@ static notrace u64 arch_counter_get_cntvct(void)
> u64 (*arch_timer_read_counter)(void) = arch_counter_get_cntvct;
> EXPORT_SYMBOL_GPL(arch_timer_read_counter);
>
> -static u64 arch_counter_read(struct clocksource *cs)
> +u64 arch_counter_read(struct clocksource *cs)
> {
> return arch_timer_read_counter();
> }
> +EXPORT_SYMBOL(arch_counter_read);
>
> static u64 arch_counter_read_cc(const struct cyclecounter *cc)
> {
> return arch_timer_read_counter();
> }
>
> -static struct clocksource clocksource_counter = {
> +struct clocksource clocksource_counter = {
> .name = "arch_sys_counter",
> .rating = 400,
> .read = arch_counter_read,
> .mask = CLOCKSOURCE_MASK(56),
> .flags = CLOCK_SOURCE_IS_CONTINUOUS,
> };
> +EXPORT_SYMBOL(clocksource_counter);
I've said what I thought about this. Not happening.
>
> static struct cyclecounter cyclecounter __ro_after_init = {
> .read = arch_counter_read_cc,
> diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
> index 9b8fee5178e8..e032fafdafa7 100644
> --- a/drivers/ptp/Kconfig
> +++ b/drivers/ptp/Kconfig
> @@ -110,7 +110,7 @@ config PTP_1588_CLOCK_PCH
> config PTP_1588_CLOCK_KVM
> tristate "KVM virtual PTP clock"
> depends on PTP_1588_CLOCK
> - depends on KVM_GUEST && X86
> + depends on KVM_GUEST && X86 || ARM64
> default y
> help
> This driver adds support for using kvm infrastructure as a PTP
> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> index a6e4d3e3d10a..2a222a1a8594 100644
> --- a/include/linux/arm-smccc.h
> +++ b/include/linux/arm-smccc.h
> @@ -94,6 +94,7 @@
>
> /* KVM "vendor specific" services */
> #define ARM_SMCCC_KVM_FUNC_FEATURES 0
> +#define ARM_SMCCC_KVM_PTP 1
> #define ARM_SMCCC_KVM_FUNC_FEATURES_2 127
> #define ARM_SMCCC_KVM_NUM_FUNCS 128
>
> @@ -102,6 +103,16 @@
> ARM_SMCCC_SMC_32, \
> ARM_SMCCC_OWNER_VENDOR_HYP, \
> ARM_SMCCC_KVM_FUNC_FEATURES)
> +/*
> + * This ID used for virtual ptp kvm clock and it will pass second value
> + * and nanosecond value of host real time and system counter by vcpu
> + * register to guest.
> + */
> +#define ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID \
> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> + ARM_SMCCC_SMC_32, \
> + ARM_SMCCC_OWNER_VENDOR_HYP, \
> + ARM_SMCCC_KVM_PTP)
>
> #ifndef __ASSEMBLY__
>
> @@ -373,5 +384,8 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
> method; \
> })
>
> +#include <linux/psci.h>
> +#include <linux/clocksource.h>
> +
> #endif /*__ASSEMBLY__*/
> #endif /*__LINUX_ARM_SMCCC_H*/
> diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> index 0debf49bf259..7fffdb25d32c 100644
> --- a/virt/kvm/arm/psci.c
> +++ b/virt/kvm/arm/psci.c
> @@ -392,6 +392,8 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
> u32 func_id = smccc_get_function(vcpu);
> u32 val[4] = {};
> u32 option;
> + struct timespec *ts;
> + u64 cnt;
>
> val[0] = SMCCC_RET_NOT_SUPPORTED;
>
> @@ -431,6 +433,21 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
> case ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID:
> val[0] = BIT(ARM_SMCCC_KVM_FUNC_FEATURES);
> break;
> + /*
> + * This will used for virtual ptp kvm clock. three
> + * values will be passed back.
> + * reg0 stores seconds of host real time;
> + * reg1 stores nanoseconds of host real time;
> + * reg2 stotes system counter cycle value.
stores
> + */
> + case ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID:
> + getnstimeofday(ts);
> + cnt = arch_timer_read_counter();
> + val[0] = ts->tv_sec;
> + val[1] = ts->tv_nsec;
> + val[2] = cnt;
Can you explain what the purpose of exposing this counter is? The guest
should have access to the physical counter already.
> + val[3] = 0;
> + break;
This will probably conflict with Steven's stolen time series. Not a big
deal though.
> default:
> return kvm_psci_call(vcpu);
> }
>
Other questions: how does this works with VM migration? Specially when
moving from a hypervisor that supports the feature to one that doesn't?
Thanks,
M.
--
Jazz is not dead, it just smells funny...
Hi Marc,
Very sorry to have missed this comments.
> -----Original Message-----
> From: Marc Zyngier <[email protected]>
> Sent: Thursday, August 29, 2019 6:33 PM
> To: Jianyong Wu (Arm Technology China) <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected]; Mark Rutland
> <[email protected]>; Will Deacon <[email protected]>; Suzuki
> Poulose <[email protected]>
> Cc: [email protected]; Steve Capper <[email protected]>;
> Kaly Xin (Arm Technology China) <[email protected]>; Justin He (Arm
> Technology China) <[email protected]>
> Subject: Re: [RFC PATCH 3/3] Enable ptp_kvm for arm64
>
> On 29/08/2019 07:39, Jianyong Wu wrote:
> > Currently in arm64 virtualization environment, there is no mechanism
> > to keep time sync between guest and host. Time in guest will drift
> > compared with host after boot up as they may both use third party time
> > sources to correct their time respectively. The time deviation will be
> > in order of milliseconds but some scenarios ask for higher time
> > precision, like in cloud envirenment, we want all the VMs running in
> > the host aquire the same level accuracy from host clock.
> >
> > Use of kvm ptp clock, which choose the host clock source clock as a
> > reference clock to sync time clock between guest and host has been
> > adopted by x86 which makes the time sync order from milliseconds to
> nanoseconds.
> >
> > This patch enable kvm ptp on arm64 and we get the similar clock drift
> > as found with x86 with kvm ptp.
> >
> > Test result comparison between with kvm ptp and without it in arm64
> > are as follows. This test derived from the result of command 'chronyc
> > sources'. we should take more cure of the last sample column which
> > shows the offset between the local clock and the source at the last
> measurement.
> >
> > no kvm ptp in guest:
> > MS Name/IP address Stratum Poll Reach LastRx Last sample
> >
> ==========================================================
> ==============
> > ^* dns1.synet.edu.cn 2 6 377 13 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 21 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 29 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 37 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 45 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 53 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 61 +1040us[+1581us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 4 -130us[ +796us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 12 -130us[ +796us] +/- 21ms
> > ^* dns1.synet.edu.cn 2 6 377 20 -130us[ +796us] +/- 21ms
> >
> > in host:
> > MS Name/IP address Stratum Poll Reach LastRx Last sample
> >
> ==========================================================
> ==============
> > ^* 120.25.115.20 2 7 377 72 -470us[ -603us] +/- 18ms
> > ^* 120.25.115.20 2 7 377 92 -470us[ -603us] +/- 18ms
> > ^* 120.25.115.20 2 7 377 112 -470us[ -603us] +/- 18ms
> > ^* 120.25.115.20 2 7 377 2 +872ns[-6808ns] +/- 17ms
> > ^* 120.25.115.20 2 7 377 22 +872ns[-6808ns] +/- 17ms
> > ^* 120.25.115.20 2 7 377 43 +872ns[-6808ns] +/- 17ms
> > ^* 120.25.115.20 2 7 377 63 +872ns[-6808ns] +/- 17ms
> > ^* 120.25.115.20 2 7 377 83 +872ns[-6808ns] +/- 17ms
> > ^* 120.25.115.20 2 7 377 103 +872ns[-6808ns] +/- 17ms
> > ^* 120.25.115.20 2 7 377 123 +872ns[-6808ns] +/- 17ms
> >
> > The dns1.synet.edu.cn is the network reference clock for guest and
> > 120.25.115.20 is the network reference clock for host. we can't get
> > the clock error between guest and host directly, but a roughly
> > estimated value will be in order of hundreds of us to ms.
> >
> > with kvm ptp in guest:
> > chrony has been disabled in host to remove the disturb by network clock.
>
> Is that a realistic use case? Why should the host not use NTP?
>
Not really, NTP will change the the host clock which will contaminate the data of sync between
Host and guest. But in reality, we will keep NTP online.
> >
> > MS Name/IP address Stratum Poll Reach LastRx Last sample
> >
> ==========================================================
> ==============
> > * PHC0 0 3 377 8 -7ns[ +1ns] +/- 3ns
> > * PHC0 0 3 377 8 +1ns[ +16ns] +/- 3ns
> > * PHC0 0 3 377 6 -4ns[ -0ns] +/- 6ns
> > * PHC0 0 3 377 6 -8ns[ -12ns] +/- 5ns
> > * PHC0 0 3 377 5 +2ns[ +4ns] +/- 4ns
> > * PHC0 0 3 377 13 +2ns[ +4ns] +/- 4ns
> > * PHC0 0 3 377 12 -4ns[ -6ns] +/- 4ns
> > * PHC0 0 3 377 11 -8ns[ -11ns] +/- 6ns
> > * PHC0 0 3 377 10 -14ns[ -20ns] +/- 4ns
> > * PHC0 0 3 377 8 +4ns[ +5ns] +/- 4ns
> >
> > The PHC0 is the ptp clock which choose the host clock as its source
> > clock. So we can be sure to say that the clock error between host and
> > guest is in order of ns.
> >
> > Signed-off-by: Jianyong Wu <[email protected]>
> > ---
> > arch/arm64/include/asm/arch_timer.h | 3 ++
> > arch/arm64/kvm/arch_ptp_kvm.c | 76
> ++++++++++++++++++++++++++++
> > drivers/clocksource/arm_arch_timer.c | 6 ++-
> > drivers/ptp/Kconfig | 2 +-
> > include/linux/arm-smccc.h | 14 +++++
> > virt/kvm/arm/psci.c | 17 +++++++
> > 6 files changed, 115 insertions(+), 3 deletions(-) create mode
> > 100644 arch/arm64/kvm/arch_ptp_kvm.c
>
> Please split this patch into two parts: the hypervisor code in a patch and the
> guest code in another patch. Having both of them together is confusing.
>
Ok, really better.
> >
> > diff --git a/arch/arm64/include/asm/arch_timer.h
> > b/arch/arm64/include/asm/arch_timer.h
> > index 6756178c27db..880576a814b6 100644
> > --- a/arch/arm64/include/asm/arch_timer.h
> > +++ b/arch/arm64/include/asm/arch_timer.h
> > @@ -229,4 +229,7 @@ static inline int arch_timer_arch_init(void)
> > return 0;
> > }
> >
> > +extern struct clocksource clocksource_counter; extern u64
> > +arch_counter_read(struct clocksource *cs);
>
> I'm definitely not keen on exposing the internals of the arch_timer driver to
> random subsystems. Furthermore, you seem to expect that the guest kernel
> will only use the arch timer as a clocksource, and nothing really guarantees
> that (in which case get_device_system_crosststamp will fail).
>
The code here is really ugly, I need a better solution to offer a clock source
For the guest.
> It looks to me that we'd be better off exposing a core timekeeping API that
> populates a struct system_counterval_t based on the *current* timekeeper
> monotonic clocksource. This would simplify the split between generic and
> arch-specific code.
>
I think it really necessary.
> Whether or not tglx will be happy with the idea is another problem, but I'm
> certainly not taking any change to the arch timer code based on this.
>
I can have a try, but the detail is not clear for me now.
> > +
> > #endif
> > diff --git a/arch/arm64/kvm/arch_ptp_kvm.c
> > b/arch/arm64/kvm/arch_ptp_kvm.c
>
> We don't put non-hypervisor in arch/arm64/kvm. Please move it back to
> drivers/ptp (as well as its x86 counterpart), and just link the two parts there.
> This should also allow this to be enabled for 32bit guests.
>
Err, sorry, what's mean of "link the two parts there"? should I add another two file update driver/ptp/
Both for arm64 and x86 to contains these arch-specific code or pack them all into ptp_kvm.c?
> > new file mode 100644
> > index 000000000000..6b2165ebce62
> > --- /dev/null
> > +++ b/arch/arm64/kvm/arch_ptp_kvm.c
> > @@ -0,0 +1,76 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Virtual PTP 1588 clock for use with KVM guests
> > + * Copyright (C) 2019 ARM Ltd.
> > + * All Rights Reserved
> > + */
> > +
> > +#include <asm/hypervisor.h>
> > +#include <linux/module.h>
> > +#include <linux/psci.h>
> > +#include <linux/arm-smccc.h>
> > +#include <linux/timecounter.h>
> > +#include <linux/sched/clock.h>
> > +#include <asm/arch_timer.h>
> > +
> > +/*
> > + * as trap call cause delay, this function will return the delay in
> > +nanosecond */ static u64 arm_smccc_1_1_invoke_delay(u32 id, struct
> > +arm_smccc_res *res) {
> > + u64 ns, t1, t2;
> > +
> > + t1 = sched_clock();
> > + arm_smccc_1_1_invoke(id, res);
> > + t2 = sched_clock();
> > + t2 -= t1;
> > + ns = t2;
> > + return ns;
>
> I think you can get rid of the ns variable here...
Yeah, ns is really redundant.
>
> > +}
> > +
> > +int kvm_arch_ptp_init(void)
> > +{
> > + return 0;
> > +}
> > +
> > +int kvm_arch_ptp_get_clock(struct timespec64 *ts) {
> > + u64 ns;
> > + struct arm_smccc_res hvc_res;
> > +
> > + if (!kvm_arm_hyp_service_available(
> > + ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID)) {
> > + return -EOPNOTSUPP;
> > + }
> > + ns =
> arm_smccc_1_1_invoke_delay(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU
> NC_ID,
> > + &hvc_res);
> > + ts->tv_sec = hvc_res.a0;
> > + ts->tv_nsec = hvc_res.a1;
> > + timespec64_add_ns(ts, ns);
> > + return 0;
> > +}
> > +
> > +int kvm_arch_ptp_get_clock_fn(long *cycle, struct timespec64 *ts,
> > + struct clocksource **cs)
> > +{
> > + u64 ns;
> > + struct arm_smccc_res hvc_res;
> > +
> > + if (!kvm_arm_hyp_service_available(
> > + ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID)) {
> > + return -EOPNOTSUPP;
> > + }
> > + ns =
> arm_smccc_1_1_invoke_delay(ARM_SMCCC_VENDOR_HYP_KVM_PTP_FU
> NC_ID,
> > + &hvc_res);
> > + ts->tv_sec = hvc_res.a0;
> > + ts->tv_nsec = hvc_res.a1;
> > + timespec64_add_ns(ts, ns);
> > + *cycle = hvc_res.a2;
> > + *cs = &clocksource_counter;
> > +
> > + return 0;
> > +}
>
> Why do we have two functions doing almost the same thing? Why do you call
> kvm_arm_hyp_service_available on each and every time? Isn't it enough to
> check in kvm_arch_ptp_init()?
>
Yeah, it's better.
> > +
> > +MODULE_AUTHOR("Marcelo Tosatti <[email protected]>");
> > +MODULE_DESCRIPTION("PTP clock using KVMCLOCK");
> > +MODULE_LICENSE("GPL");
>
> This should only exist in the generic code.
Ok. I will remove them.
>
> > diff --git a/drivers/clocksource/arm_arch_timer.c
> > b/drivers/clocksource/arm_arch_timer.c
> > index 07e57a49d1e8..021e3f69364c 100644
> > --- a/drivers/clocksource/arm_arch_timer.c
> > +++ b/drivers/clocksource/arm_arch_timer.c
> > @@ -175,23 +175,25 @@ static notrace u64 arch_counter_get_cntvct(void)
> > u64 (*arch_timer_read_counter)(void) = arch_counter_get_cntvct;
> > EXPORT_SYMBOL_GPL(arch_timer_read_counter);
> >
> > -static u64 arch_counter_read(struct clocksource *cs)
> > +u64 arch_counter_read(struct clocksource *cs)
> > {
> > return arch_timer_read_counter();
> > }
> > +EXPORT_SYMBOL(arch_counter_read);
> >
> > static u64 arch_counter_read_cc(const struct cyclecounter *cc) {
> > return arch_timer_read_counter();
> > }
> >
> > -static struct clocksource clocksource_counter = {
> > +struct clocksource clocksource_counter = {
> > .name = "arch_sys_counter",
> > .rating = 400,
> > .read = arch_counter_read,
> > .mask = CLOCKSOURCE_MASK(56),
> > .flags = CLOCK_SOURCE_IS_CONTINUOUS,
> > };
> > +EXPORT_SYMBOL(clocksource_counter);
>
> I've said what I thought about this. Not happening.
>
Ok.
> >
> > static struct cyclecounter cyclecounter __ro_after_init = {
> > .read = arch_counter_read_cc,
> > diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig index
> > 9b8fee5178e8..e032fafdafa7 100644
> > --- a/drivers/ptp/Kconfig
> > +++ b/drivers/ptp/Kconfig
> > @@ -110,7 +110,7 @@ config PTP_1588_CLOCK_PCH config
> > PTP_1588_CLOCK_KVM
> > tristate "KVM virtual PTP clock"
> > depends on PTP_1588_CLOCK
> > - depends on KVM_GUEST && X86
> > + depends on KVM_GUEST && X86 || ARM64
> > default y
> > help
> > This driver adds support for using kvm infrastructure as a PTP
> > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> > index a6e4d3e3d10a..2a222a1a8594 100644
> > --- a/include/linux/arm-smccc.h
> > +++ b/include/linux/arm-smccc.h
> > @@ -94,6 +94,7 @@
> >
> > /* KVM "vendor specific" services */
> > #define ARM_SMCCC_KVM_FUNC_FEATURES 0
> > +#define ARM_SMCCC_KVM_PTP 1
> > #define ARM_SMCCC_KVM_FUNC_FEATURES_2 127
> > #define ARM_SMCCC_KVM_NUM_FUNCS 128
> >
> > @@ -102,6 +103,16 @@
> > ARM_SMCCC_SMC_32,
> \
> > ARM_SMCCC_OWNER_VENDOR_HYP,
> \
> > ARM_SMCCC_KVM_FUNC_FEATURES)
> > +/*
> > + * This ID used for virtual ptp kvm clock and it will pass second
> > +value
> > + * and nanosecond value of host real time and system counter by vcpu
> > + * register to guest.
> > + */
> > +#define ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID
> \
> > + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,
> \
> > + ARM_SMCCC_SMC_32,
> \
> > + ARM_SMCCC_OWNER_VENDOR_HYP,
> \
> > + ARM_SMCCC_KVM_PTP)
> >
> > #ifndef __ASSEMBLY__
> >
> > @@ -373,5 +384,8 @@ asmlinkage void __arm_smccc_hvc(unsigned long
> a0, unsigned long a1,
> > method;
> \
> > })
> >
> > +#include <linux/psci.h>
> > +#include <linux/clocksource.h>
> > +
> > #endif /*__ASSEMBLY__*/
> > #endif /*__LINUX_ARM_SMCCC_H*/
> > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c index
> > 0debf49bf259..7fffdb25d32c 100644
> > --- a/virt/kvm/arm/psci.c
> > +++ b/virt/kvm/arm/psci.c
> > @@ -392,6 +392,8 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
> > u32 func_id = smccc_get_function(vcpu);
> > u32 val[4] = {};
> > u32 option;
> > + struct timespec *ts;
> > + u64 cnt;
> >
> > val[0] = SMCCC_RET_NOT_SUPPORTED;
> >
> > @@ -431,6 +433,21 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
> > case ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID:
> > val[0] = BIT(ARM_SMCCC_KVM_FUNC_FEATURES);
> > break;
> > + /*
> > + * This will used for virtual ptp kvm clock. three
> > + * values will be passed back.
> > + * reg0 stores seconds of host real time;
> > + * reg1 stores nanoseconds of host real time;
> > + * reg2 stotes system counter cycle value.
>
> stores
Yeah
>
> > + */
> > + case ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID:
> > + getnstimeofday(ts);
> > + cnt = arch_timer_read_counter();
> > + val[0] = ts->tv_sec;
> > + val[1] = ts->tv_nsec;
> > + val[2] = cnt;
>
> Can you explain what the purpose of exposing this counter is? The guest
> should have access to the physical counter already.
One api of ptp_kvm called ptp_kvm_get_time_fn need a clock sources passed from host as system_counter.
>
> > + val[3] = 0;
> > + break;
>
> This will probably conflict with Steven's stolen time series. Not a big deal
> though.
Err, sorry I am not familiar with this theory. Let me check it.
>
> > default:
> > return kvm_psci_call(vcpu);
> > }
> >
>
> Other questions: how does this works with VM migration? Specially when
> moving from a hypervisor that supports the feature to one that doesn't?
>
I think it won't solve the problem generated by VM migration and only for VMs in a single machine.
Ptp_kvm only works for VMs in the same machine.
But using ptp (not ptp_kvm) clock, all the machines in a low latency network environment can keep time sync in high precision,
Then VMs move from one machine to another will obtain a high precision time sync.
Thanks
Jianyong Wu
> Thanks,
>
> M.
> --
> Jazz is not dead, it just smells funny...
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On Fri, 06 Sep 2019 12:58:15 +0100,
"Jianyong Wu (Arm Technology China)" <[email protected]> wrote:
>
> Hi Marc,
>
> Very sorry to have missed this comments.
>
> > -----Original Message-----
> > From: Marc Zyngier <[email protected]>
> > Sent: Thursday, August 29, 2019 6:33 PM
> > To: Jianyong Wu (Arm Technology China) <[email protected]>;
> > [email protected]; [email protected];
> > [email protected]; [email protected]; Mark Rutland
> > <[email protected]>; Will Deacon <[email protected]>; Suzuki
> > Poulose <[email protected]>
> > Cc: [email protected]; Steve Capper <[email protected]>;
> > Kaly Xin (Arm Technology China) <[email protected]>; Justin He (Arm
> > Technology China) <[email protected]>
> > Subject: Re: [RFC PATCH 3/3] Enable ptp_kvm for arm64
> >
> > On 29/08/2019 07:39, Jianyong Wu wrote:
> > > Currently in arm64 virtualization environment, there is no mechanism
> > > to keep time sync between guest and host. Time in guest will drift
> > > compared with host after boot up as they may both use third party time
> > > sources to correct their time respectively. The time deviation will be
> > > in order of milliseconds but some scenarios ask for higher time
> > > precision, like in cloud envirenment, we want all the VMs running in
> > > the host aquire the same level accuracy from host clock.
> > >
> > > Use of kvm ptp clock, which choose the host clock source clock as a
> > > reference clock to sync time clock between guest and host has been
> > > adopted by x86 which makes the time sync order from milliseconds to
> > nanoseconds.
> > >
> > > This patch enable kvm ptp on arm64 and we get the similar clock drift
> > > as found with x86 with kvm ptp.
> > >
> > > Test result comparison between with kvm ptp and without it in arm64
> > > are as follows. This test derived from the result of command 'chronyc
> > > sources'. we should take more cure of the last sample column which
> > > shows the offset between the local clock and the source at the last
> > measurement.
> > >
> > > no kvm ptp in guest:
> > > MS Name/IP address Stratum Poll Reach LastRx Last sample
> > >
> > ==========================================================
> > ==============
> > > ^* dns1.synet.edu.cn 2 6 377 13 +1040us[+1581us] +/- 21ms
> > > ^* dns1.synet.edu.cn 2 6 377 21 +1040us[+1581us] +/- 21ms
> > > ^* dns1.synet.edu.cn 2 6 377 29 +1040us[+1581us] +/- 21ms
> > > ^* dns1.synet.edu.cn 2 6 377 37 +1040us[+1581us] +/- 21ms
> > > ^* dns1.synet.edu.cn 2 6 377 45 +1040us[+1581us] +/- 21ms
> > > ^* dns1.synet.edu.cn 2 6 377 53 +1040us[+1581us] +/- 21ms
> > > ^* dns1.synet.edu.cn 2 6 377 61 +1040us[+1581us] +/- 21ms
> > > ^* dns1.synet.edu.cn 2 6 377 4 -130us[ +796us] +/- 21ms
> > > ^* dns1.synet.edu.cn 2 6 377 12 -130us[ +796us] +/- 21ms
> > > ^* dns1.synet.edu.cn 2 6 377 20 -130us[ +796us] +/- 21ms
> > >
> > > in host:
> > > MS Name/IP address Stratum Poll Reach LastRx Last sample
> > >
> > ==========================================================
> > ==============
> > > ^* 120.25.115.20 2 7 377 72 -470us[ -603us] +/- 18ms
> > > ^* 120.25.115.20 2 7 377 92 -470us[ -603us] +/- 18ms
> > > ^* 120.25.115.20 2 7 377 112 -470us[ -603us] +/- 18ms
> > > ^* 120.25.115.20 2 7 377 2 +872ns[-6808ns] +/- 17ms
> > > ^* 120.25.115.20 2 7 377 22 +872ns[-6808ns] +/- 17ms
> > > ^* 120.25.115.20 2 7 377 43 +872ns[-6808ns] +/- 17ms
> > > ^* 120.25.115.20 2 7 377 63 +872ns[-6808ns] +/- 17ms
> > > ^* 120.25.115.20 2 7 377 83 +872ns[-6808ns] +/- 17ms
> > > ^* 120.25.115.20 2 7 377 103 +872ns[-6808ns] +/- 17ms
> > > ^* 120.25.115.20 2 7 377 123 +872ns[-6808ns] +/- 17ms
> > >
> > > The dns1.synet.edu.cn is the network reference clock for guest and
> > > 120.25.115.20 is the network reference clock for host. we can't get
> > > the clock error between guest and host directly, but a roughly
> > > estimated value will be in order of hundreds of us to ms.
> > >
> > > with kvm ptp in guest:
> > > chrony has been disabled in host to remove the disturb by network clock.
> >
> > Is that a realistic use case? Why should the host not use NTP?
> >
>
> Not really, NTP will change the the host clock which will contaminate the data of sync between
> Host and guest. But in reality, we will keep NTP online.
>
> > >
> > > MS Name/IP address Stratum Poll Reach LastRx Last sample
> > >
> > ==========================================================
> > ==============
> > > * PHC0 0 3 377 8 -7ns[ +1ns] +/- 3ns
> > > * PHC0 0 3 377 8 +1ns[ +16ns] +/- 3ns
> > > * PHC0 0 3 377 6 -4ns[ -0ns] +/- 6ns
> > > * PHC0 0 3 377 6 -8ns[ -12ns] +/- 5ns
> > > * PHC0 0 3 377 5 +2ns[ +4ns] +/- 4ns
> > > * PHC0 0 3 377 13 +2ns[ +4ns] +/- 4ns
> > > * PHC0 0 3 377 12 -4ns[ -6ns] +/- 4ns
> > > * PHC0 0 3 377 11 -8ns[ -11ns] +/- 6ns
> > > * PHC0 0 3 377 10 -14ns[ -20ns] +/- 4ns
> > > * PHC0 0 3 377 8 +4ns[ +5ns] +/- 4ns
> > >
> > > The PHC0 is the ptp clock which choose the host clock as its source
> > > clock. So we can be sure to say that the clock error between host and
> > > guest is in order of ns.
> > >
> > > Signed-off-by: Jianyong Wu <[email protected]>
> > > ---
> > > arch/arm64/include/asm/arch_timer.h | 3 ++
> > > arch/arm64/kvm/arch_ptp_kvm.c | 76
> > ++++++++++++++++++++++++++++
> > > drivers/clocksource/arm_arch_timer.c | 6 ++-
> > > drivers/ptp/Kconfig | 2 +-
> > > include/linux/arm-smccc.h | 14 +++++
> > > virt/kvm/arm/psci.c | 17 +++++++
> > > 6 files changed, 115 insertions(+), 3 deletions(-) create mode
> > > 100644 arch/arm64/kvm/arch_ptp_kvm.c
> >
> > Please split this patch into two parts: the hypervisor code in a patch and the
> > guest code in another patch. Having both of them together is confusing.
> >
> Ok, really better.
>
> > >
> > > diff --git a/arch/arm64/include/asm/arch_timer.h
> > > b/arch/arm64/include/asm/arch_timer.h
> > > index 6756178c27db..880576a814b6 100644
> > > --- a/arch/arm64/include/asm/arch_timer.h
> > > +++ b/arch/arm64/include/asm/arch_timer.h
> > > @@ -229,4 +229,7 @@ static inline int arch_timer_arch_init(void)
> > > return 0;
> > > }
> > >
> > > +extern struct clocksource clocksource_counter; extern u64
> > > +arch_counter_read(struct clocksource *cs);
> >
> > I'm definitely not keen on exposing the internals of the arch_timer driver to
> > random subsystems. Furthermore, you seem to expect that the guest kernel
> > will only use the arch timer as a clocksource, and nothing really guarantees
> > that (in which case get_device_system_crosststamp will fail).
> >
> The code here is really ugly, I need a better solution to offer a clock source
> For the guest.
>
> > It looks to me that we'd be better off exposing a core timekeeping API that
> > populates a struct system_counterval_t based on the *current* timekeeper
> > monotonic clocksource. This would simplify the split between generic and
> > arch-specific code.
> >
> I think it really necessary.
>
> > Whether or not tglx will be happy with the idea is another problem, but I'm
> > certainly not taking any change to the arch timer code based on this.
> >
> I can have a try, but the detail is not clear for me now.
Something along those lines:
From 5f1c061e55c691d64012bc7c1490a1a8c4432c67 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <[email protected]>
Date: Sat, 7 Sep 2019 10:11:49 +0100
Subject: [PATCH] timekeeping: Expose API allowing retrival of current
clocksource and counter value
Signed-off-by: Marc Zyngier <[email protected]>
---
include/linux/timekeeping.h | 5 +++++
kernel/time/timekeeping.c | 12 ++++++++++++
2 files changed, 17 insertions(+)
diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index b27e2ffa96c1..6df26a913711 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -275,6 +275,11 @@ extern int get_device_system_crosststamp(
struct system_time_snapshot *history,
struct system_device_crosststamp *xtstamp);
+/*
+ * Obtain current monotonic clock and its counter value
+ */
+extern void get_current_counterval(struct system_counterval_t *sc);
+
/*
* Simultaneously snapshot realtime and monotonic raw clocks
*/
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index d911c8470149..de689bbd3808 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1098,6 +1098,18 @@ static bool cycle_between(u64 before, u64 test, u64 after)
return false;
}
+/**
+ * get_current_counterval - Snapshot the current clocksource and counter value
+ * @sc: Pointer to a struct containing the current clocksource and its value
+ */
+void get_current_counterval(struct system_counterval_t *sc)
+{
+ struct timekeeper *tk = &tk_core.timekeeper;
+
+ sc->cs = READ_ONCE(tk->tkr_mono.clock);
+ sc->cycles = sc->cs->read(sc->cs);
+}
+
/**
* get_device_system_crosststamp - Synchronously capture system/device timestamp
* @get_time_fn: Callback to get simultaneous device time and
which should do the right thing.
>
> > > +
> > > #endif
> > > diff --git a/arch/arm64/kvm/arch_ptp_kvm.c
> > > b/arch/arm64/kvm/arch_ptp_kvm.c
> >
> > We don't put non-hypervisor in arch/arm64/kvm. Please move it back to
> > drivers/ptp (as well as its x86 counterpart), and just link the two parts there.
> > This should also allow this to be enabled for 32bit guests.
> >
> Err, sorry, what's mean of "link the two parts there"? should I add
> another two file update driver/ptp/ Both for arm64 and x86 to
> contains these arch-specific code or pack them all into ptp_kvm.c?
What I'm suggesting is that you have 3 files:
drivers/ptp/ptp_kvm.c
drivers/ptp/ptp_kvm_x86.c
drivers/ptp/ptp_kvm_arm.c
and let the Makefile combine them.
[...]
> > Other questions: how does this works with VM migration? Specially when
> > moving from a hypervisor that supports the feature to one that doesn't?
> >
> I think it won't solve the problem generated by VM migration and
> only for VMs in a single machine. Ptp_kvm only works for VMs in the
> same machine. But using ptp (not ptp_kvm) clock, all the machines
> in a low latency network environment can keep time sync in high
> precision, Then VMs move from one machine to another will obtain a
> high precision time sync.
That's a problem. Migration must be possible from one host to another,
even if that means temporarily loosing some (or a lot of)
precision. The service must be discoverable from userspace on the host
so that the MVV can decie whether a migration is possible or not.
Thanks,
M.
--
Jazz is not dead, it just smells funny.
Hi Marc,
> -----Original Message-----
> From: Marc Zyngier <[email protected]>
> Sent: Saturday, September 7, 2019 5:16 PM
> To: Jianyong Wu (Arm Technology China) <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Mark Rutland
> <[email protected]>; Will Deacon <[email protected]>; Suzuki
> Poulose <[email protected]>; [email protected]; Steve
> Capper <[email protected]>; Kaly Xin (Arm Technology China)
> <[email protected]>; Justin He (Arm Technology China)
> <[email protected]>
> Subject: Re: [RFC PATCH 3/3] Enable ptp_kvm for arm64
>
> On Fri, 06 Sep 2019 12:58:15 +0100,
> "Jianyong Wu (Arm Technology China)" <[email protected]> wrote:
> >
> > Hi Marc,
> >
> > Very sorry to have missed this comments.
> >
> > > -----Original Message-----
> > > From: Marc Zyngier <[email protected]>
> > > Sent: Thursday, August 29, 2019 6:33 PM
> > > To: Jianyong Wu (Arm Technology China) <[email protected]>;
> > > [email protected]; [email protected];
> > > [email protected]; [email protected]; Mark
> > > Rutland <[email protected]>; Will Deacon
> <[email protected]>;
> > > Suzuki Poulose <[email protected]>
> > > Cc: [email protected]; Steve Capper
> > > <[email protected]>; Kaly Xin (Arm Technology China)
> > > <[email protected]>; Justin He (Arm Technology China)
> > > <[email protected]>
> > > Subject: Re: [RFC PATCH 3/3] Enable ptp_kvm for arm64
> > >
> > > On 29/08/2019 07:39, Jianyong Wu wrote:
> > > > Currently in arm64 virtualization environment, there is no
> > > > mechanism to keep time sync between guest and host. Time in guest
> > > > will drift compared with host after boot up as they may both use
> > > > third party time sources to correct their time respectively. The
> > > > time deviation will be in order of milliseconds but some scenarios
> > > > ask for higher time precision, like in cloud envirenment, we want
> > > > all the VMs running in the host aquire the same level accuracy from
> host clock.
> > > >
> > > > Use of kvm ptp clock, which choose the host clock source clock as
> > > > a reference clock to sync time clock between guest and host has
> > > > been adopted by x86 which makes the time sync order from
> > > > milliseconds to
> > > nanoseconds.
> > > >
> > > > This patch enable kvm ptp on arm64 and we get the similar clock
> > > > drift as found with x86 with kvm ptp.
> > > >
> > > > Test result comparison between with kvm ptp and without it in
> > > > arm64 are as follows. This test derived from the result of command
> > > > 'chronyc sources'. we should take more cure of the last sample
> > > > column which shows the offset between the local clock and the
> > > > source at the last
> > > measurement.
> > > >
> > > > no kvm ptp in guest:
> > > > MS Name/IP address Stratum Poll Reach LastRx Last sample
> > > >
> > >
> ==========================================================
> > > ==============
> > > > ^* dns1.synet.edu.cn 2 6 377 13 +1040us[+1581us] +/- 21ms
> > > > ^* dns1.synet.edu.cn 2 6 377 21 +1040us[+1581us] +/- 21ms
> > > > ^* dns1.synet.edu.cn 2 6 377 29 +1040us[+1581us] +/- 21ms
> > > > ^* dns1.synet.edu.cn 2 6 377 37 +1040us[+1581us] +/- 21ms
> > > > ^* dns1.synet.edu.cn 2 6 377 45 +1040us[+1581us] +/- 21ms
> > > > ^* dns1.synet.edu.cn 2 6 377 53 +1040us[+1581us] +/- 21ms
> > > > ^* dns1.synet.edu.cn 2 6 377 61 +1040us[+1581us] +/- 21ms
> > > > ^* dns1.synet.edu.cn 2 6 377 4 -130us[ +796us] +/- 21ms
> > > > ^* dns1.synet.edu.cn 2 6 377 12 -130us[ +796us] +/- 21ms
> > > > ^* dns1.synet.edu.cn 2 6 377 20 -130us[ +796us] +/- 21ms
> > > >
> > > > in host:
> > > > MS Name/IP address Stratum Poll Reach LastRx Last sample
> > > >
> > >
> ==========================================================
> > > ==============
> > > > ^* 120.25.115.20 2 7 377 72 -470us[ -603us] +/- 18ms
> > > > ^* 120.25.115.20 2 7 377 92 -470us[ -603us] +/- 18ms
> > > > ^* 120.25.115.20 2 7 377 112 -470us[ -603us] +/- 18ms
> > > > ^* 120.25.115.20 2 7 377 2 +872ns[-6808ns] +/- 17ms
> > > > ^* 120.25.115.20 2 7 377 22 +872ns[-6808ns] +/- 17ms
> > > > ^* 120.25.115.20 2 7 377 43 +872ns[-6808ns] +/- 17ms
> > > > ^* 120.25.115.20 2 7 377 63 +872ns[-6808ns] +/- 17ms
> > > > ^* 120.25.115.20 2 7 377 83 +872ns[-6808ns] +/- 17ms
> > > > ^* 120.25.115.20 2 7 377 103 +872ns[-6808ns] +/- 17ms
> > > > ^* 120.25.115.20 2 7 377 123 +872ns[-6808ns] +/- 17ms
> > > >
> > > > The dns1.synet.edu.cn is the network reference clock for guest and
> > > > 120.25.115.20 is the network reference clock for host. we can't
> > > > get the clock error between guest and host directly, but a roughly
> > > > estimated value will be in order of hundreds of us to ms.
> > > >
> > > > with kvm ptp in guest:
> > > > chrony has been disabled in host to remove the disturb by network
> clock.
> > >
> > > Is that a realistic use case? Why should the host not use NTP?
> > >
> >
> > Not really, NTP will change the the host clock which will contaminate
> > the data of sync between Host and guest. But in reality, we will keep NTP
> online.
> >
> > > >
> > > > MS Name/IP address Stratum Poll Reach LastRx Last sample
> > > >
> > >
> ==========================================================
> > > ==============
> > > > * PHC0 0 3 377 8 -7ns[ +1ns] +/- 3ns
> > > > * PHC0 0 3 377 8 +1ns[ +16ns] +/- 3ns
> > > > * PHC0 0 3 377 6 -4ns[ -0ns] +/- 6ns
> > > > * PHC0 0 3 377 6 -8ns[ -12ns] +/- 5ns
> > > > * PHC0 0 3 377 5 +2ns[ +4ns] +/- 4ns
> > > > * PHC0 0 3 377 13 +2ns[ +4ns] +/- 4ns
> > > > * PHC0 0 3 377 12 -4ns[ -6ns] +/- 4ns
> > > > * PHC0 0 3 377 11 -8ns[ -11ns] +/- 6ns
> > > > * PHC0 0 3 377 10 -14ns[ -20ns] +/- 4ns
> > > > * PHC0 0 3 377 8 +4ns[ +5ns] +/- 4ns
> > > >
> > > > The PHC0 is the ptp clock which choose the host clock as its
> > > > source clock. So we can be sure to say that the clock error
> > > > between host and guest is in order of ns.
> > > >
> > > > Signed-off-by: Jianyong Wu <[email protected]>
> > > > ---
> > > > arch/arm64/include/asm/arch_timer.h | 3 ++
> > > > arch/arm64/kvm/arch_ptp_kvm.c | 76
> > > ++++++++++++++++++++++++++++
> > > > drivers/clocksource/arm_arch_timer.c | 6 ++-
> > > > drivers/ptp/Kconfig | 2 +-
> > > > include/linux/arm-smccc.h | 14 +++++
> > > > virt/kvm/arm/psci.c | 17 +++++++
> > > > 6 files changed, 115 insertions(+), 3 deletions(-) create mode
> > > > 100644 arch/arm64/kvm/arch_ptp_kvm.c
> > >
> > > Please split this patch into two parts: the hypervisor code in a
> > > patch and the guest code in another patch. Having both of them together
> is confusing.
> > >
> > Ok, really better.
> >
> > > >
> > > > diff --git a/arch/arm64/include/asm/arch_timer.h
> > > > b/arch/arm64/include/asm/arch_timer.h
> > > > index 6756178c27db..880576a814b6 100644
> > > > --- a/arch/arm64/include/asm/arch_timer.h
> > > > +++ b/arch/arm64/include/asm/arch_timer.h
> > > > @@ -229,4 +229,7 @@ static inline int arch_timer_arch_init(void)
> > > > return 0;
> > > > }
> > > >
> > > > +extern struct clocksource clocksource_counter; extern u64
> > > > +arch_counter_read(struct clocksource *cs);
> > >
> > > I'm definitely not keen on exposing the internals of the arch_timer
> > > driver to random subsystems. Furthermore, you seem to expect that
> > > the guest kernel will only use the arch timer as a clocksource, and
> > > nothing really guarantees that (in which case
> get_device_system_crosststamp will fail).
> > >
> > The code here is really ugly, I need a better solution to offer a
> > clock source For the guest.
> >
> > > It looks to me that we'd be better off exposing a core timekeeping
> > > API that populates a struct system_counterval_t based on the
> > > *current* timekeeper monotonic clocksource. This would simplify the
> > > split between generic and arch-specific code.
> > >
> > I think it really necessary.
> >
> > > Whether or not tglx will be happy with the idea is another problem,
> > > but I'm certainly not taking any change to the arch timer code based on
> this.
> > >
> > I can have a try, but the detail is not clear for me now.
>
> Something along those lines:
>
> From 5f1c061e55c691d64012bc7c1490a1a8c4432c67 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <[email protected]>
> Date: Sat, 7 Sep 2019 10:11:49 +0100
> Subject: [PATCH] timekeeping: Expose API allowing retrival of current
> clocksource and counter value
>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> include/linux/timekeeping.h | 5 +++++
> kernel/time/timekeeping.c | 12 ++++++++++++
> 2 files changed, 17 insertions(+)
>
> diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h index
> b27e2ffa96c1..6df26a913711 100644
> --- a/include/linux/timekeeping.h
> +++ b/include/linux/timekeeping.h
> @@ -275,6 +275,11 @@ extern int get_device_system_crosststamp(
> struct system_time_snapshot *history,
> struct system_device_crosststamp *xtstamp);
>
> +/*
> + * Obtain current monotonic clock and its counter value */ extern void
> +get_current_counterval(struct system_counterval_t *sc);
> +
> /*
> * Simultaneously snapshot realtime and monotonic raw clocks
> */
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c index
> d911c8470149..de689bbd3808 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -1098,6 +1098,18 @@ static bool cycle_between(u64 before, u64 test,
> u64 after)
> return false;
> }
>
> +/**
> + * get_current_counterval - Snapshot the current clocksource and counter
> value
> + * @sc: Pointer to a struct containing the current clocksource and its
> value
> + */
> +void get_current_counterval(struct system_counterval_t *sc) {
> + struct timekeeper *tk = &tk_core.timekeeper;
> +
> + sc->cs = READ_ONCE(tk->tkr_mono.clock);
> + sc->cycles = sc->cs->read(sc->cs);
> +}
> +
> /**
> * get_device_system_crosststamp - Synchronously capture system/device
> timestamp
> * @get_time_fn: Callback to get simultaneous device time and
>
> which should do the right thing.
>
It is a good news for me. These code is indeed what I need!
So what's your plan about this patch? Is there any problem with you if I include these code
into my patch ?
> >
> > > > +
> > > > #endif
> > > > diff --git a/arch/arm64/kvm/arch_ptp_kvm.c
> > > > b/arch/arm64/kvm/arch_ptp_kvm.c
> > >
> > > We don't put non-hypervisor in arch/arm64/kvm. Please move it back
> > > to drivers/ptp (as well as its x86 counterpart), and just link the two parts
> there.
> > > This should also allow this to be enabled for 32bit guests.
> > >
> > Err, sorry, what's mean of "link the two parts there"? should I add
> > another two file update driver/ptp/ Both for arm64 and x86 to contains
> > these arch-specific code or pack them all into ptp_kvm.c?
>
> What I'm suggesting is that you have 3 files:
>
> drivers/ptp/ptp_kvm.c
> drivers/ptp/ptp_kvm_x86.c
> drivers/ptp/ptp_kvm_arm.c
>
> and let the Makefile combine them.
>
> [...]
>
it is what I want to do at the beginning of drafting these patches.
> > > Other questions: how does this works with VM migration? Specially
> > > when moving from a hypervisor that supports the feature to one that
> doesn't?
> > >
> > I think it won't solve the problem generated by VM migration and only
> > for VMs in a single machine. Ptp_kvm only works for VMs in the same
> > machine. But using ptp (not ptp_kvm) clock, all the machines in a low
> > latency network environment can keep time sync in high precision, Then
> > VMs move from one machine to another will obtain a high precision time
> > sync.
>
> That's a problem. Migration must be possible from one host to another, even
> if that means temporarily loosing some (or a lot of) precision. The service
> must be discoverable from userspace on the host so that the MVV can decie
> whether a migration is possible or not.
>
Don't worry, things will be not that bad.
ptp_kvm will not trouble the VM migration. This ptp_kvm is one clocksource of the clock pool for
chrony. Chrony will choose the highest precision clock from the pool. If host does not support
ptp_kvm, the ptp_kvm will not be chosen as the clocksouce of chrony.
We have roughly the same logic of implementation of ptp_kvm with x86, and ptp_kvm works well in x86.
so I think that will be the case for arm64.
Maybe I miss your point, I have no idea of MVV and can't get related info from google.
Also I'm not clear of your last words of how to decide VM migration is possible?
Thanks
Jianyong Wu
> Thanks,
>
> M.
>
> --
> Jazz is not dead, it just smells funny.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On Mon, 09 Sep 2019 11:17:24 +0100,
"Jianyong Wu (Arm Technology China)" <[email protected]> wrote:
Hi Jianyoung,
[...]
> > > > I'm definitely not keen on exposing the internals of the arch_timer
> > > > driver to random subsystems. Furthermore, you seem to expect that
> > > > the guest kernel will only use the arch timer as a clocksource, and
> > > > nothing really guarantees that (in which case
> > get_device_system_crosststamp will fail).
> > > >
> > > The code here is really ugly, I need a better solution to offer a
> > > clock source For the guest.
> > >
> > > > It looks to me that we'd be better off exposing a core timekeeping
> > > > API that populates a struct system_counterval_t based on the
> > > > *current* timekeeper monotonic clocksource. This would simplify the
> > > > split between generic and arch-specific code.
> > > >
> > > I think it really necessary.
> > >
> > > > Whether or not tglx will be happy with the idea is another problem,
> > > > but I'm certainly not taking any change to the arch timer code based on
> > this.
> > > >
> > > I can have a try, but the detail is not clear for me now.
> >
> > Something along those lines:
> >
> > From 5f1c061e55c691d64012bc7c1490a1a8c4432c67 Mon Sep 17 00:00:00 2001
> > From: Marc Zyngier <[email protected]>
> > Date: Sat, 7 Sep 2019 10:11:49 +0100
> > Subject: [PATCH] timekeeping: Expose API allowing retrival of current
> > clocksource and counter value
> >
> > Signed-off-by: Marc Zyngier <[email protected]>
> > ---
> > include/linux/timekeeping.h | 5 +++++
> > kernel/time/timekeeping.c | 12 ++++++++++++
> > 2 files changed, 17 insertions(+)
> >
> > diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h index
> > b27e2ffa96c1..6df26a913711 100644
> > --- a/include/linux/timekeeping.h
> > +++ b/include/linux/timekeeping.h
> > @@ -275,6 +275,11 @@ extern int get_device_system_crosststamp(
> > struct system_time_snapshot *history,
> > struct system_device_crosststamp *xtstamp);
> >
> > +/*
> > + * Obtain current monotonic clock and its counter value */ extern void
> > +get_current_counterval(struct system_counterval_t *sc);
> > +
> > /*
> > * Simultaneously snapshot realtime and monotonic raw clocks
> > */
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c index
> > d911c8470149..de689bbd3808 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -1098,6 +1098,18 @@ static bool cycle_between(u64 before, u64 test,
> > u64 after)
> > return false;
> > }
> >
> > +/**
> > + * get_current_counterval - Snapshot the current clocksource and counter
> > value
> > + * @sc: Pointer to a struct containing the current clocksource and its
> > value
> > + */
> > +void get_current_counterval(struct system_counterval_t *sc) {
> > + struct timekeeper *tk = &tk_core.timekeeper;
> > +
> > + sc->cs = READ_ONCE(tk->tkr_mono.clock);
> > + sc->cycles = sc->cs->read(sc->cs);
> > +}
> > +
> > /**
> > * get_device_system_crosststamp - Synchronously capture system/device
> > timestamp
> > * @get_time_fn: Callback to get simultaneous device time and
> >
> > which should do the right thing.
> >
> It is a good news for me. These code is indeed what I need! So
> what's your plan about this patch? Is there any problem with you if
> I include these code into my patch ?
Just add this patch as part of your series (I'll try to write an
actual commit log for that).
[...]
> > > > Other questions: how does this works with VM migration? Specially
> > > > when moving from a hypervisor that supports the feature to one that
> > doesn't?
> > > >
> > > I think it won't solve the problem generated by VM migration and only
> > > for VMs in a single machine. Ptp_kvm only works for VMs in the same
> > > machine. But using ptp (not ptp_kvm) clock, all the machines in a low
> > > latency network environment can keep time sync in high precision, Then
> > > VMs move from one machine to another will obtain a high precision time
> > > sync.
> >
> > That's a problem. Migration must be possible from one host to another, even
> > if that means temporarily loosing some (or a lot of) precision. The service
> > must be discoverable from userspace on the host so that the MVV can decie
> > whether a migration is possible or not.
> >
> Don't worry, things will be not that bad. ptp_kvm will not trouble
> the VM migration. This ptp_kvm is one clocksource of the clock pool
> for chrony. Chrony will choose the highest precision clock from the
> pool. If host does not support ptp_kvm, the ptp_kvm will not be
> chosen as the clocksouce of chrony. We have roughly the same logic
> of implementation of ptp_kvm with x86, and ptp_kvm works well in
> x86. so I think that will be the case for arm64.
>
> Maybe I miss your point, I have no idea of MVV and can't get related
> info from google. Also I'm not clear of your last words of how to
> decide VM migration is possible?
Sorry. s/MVV/VMM/. Basically userspace, such as QEMU.
Here's an example: The guest runs on a PTP aware host, starts using
the PTP service and uses HVC calls to get its clock. We now migrate
the guest to a non PTP-aware host. The hypercalls are now going to
fail unexpectedly. Is that something that is acceptable? I don't think
it is. Once you've allowed a guest to use a service, this service
should be preserved. I'd be more confident if we gave to userspace the
indication that the hypervisor supports PTP. Userspace can then decide
whether to perform migration or not.
Thanks,
M.
--
Jazz is not dead, it just smells funny.
Hi Marc,
> -----Original Message-----
> From: Marc Zyngier <[email protected]>
> Sent: Monday, September 9, 2019 7:25 PM
> To: Jianyong Wu (Arm Technology China) <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Mark Rutland
> <[email protected]>; Will Deacon <[email protected]>; Suzuki
> Poulose <[email protected]>; [email protected]; Steve
> Capper <[email protected]>; Kaly Xin (Arm Technology China)
> <[email protected]>; Justin He (Arm Technology China)
> <[email protected]>
> Subject: Re: [RFC PATCH 3/3] Enable ptp_kvm for arm64
>
> On Mon, 09 Sep 2019 11:17:24 +0100,
> "Jianyong Wu (Arm Technology China)" <[email protected]> wrote:
>
> Hi Jianyoung,
>
> [...]
>
> > > > > I'm definitely not keen on exposing the internals of the
> > > > > arch_timer driver to random subsystems. Furthermore, you seem to
> > > > > expect that the guest kernel will only use the arch timer as a
> > > > > clocksource, and nothing really guarantees that (in which case
> > > get_device_system_crosststamp will fail).
> > > > >
> > > > The code here is really ugly, I need a better solution to offer a
> > > > clock source For the guest.
> > > >
> > > > > It looks to me that we'd be better off exposing a core
> > > > > timekeeping API that populates a struct system_counterval_t
> > > > > based on the
> > > > > *current* timekeeper monotonic clocksource. This would simplify
> > > > > the split between generic and arch-specific code.
> > > > >
> > > > I think it really necessary.
> > > >
> > > > > Whether or not tglx will be happy with the idea is another
> > > > > problem, but I'm certainly not taking any change to the arch
> > > > > timer code based on
> > > this.
> > > > >
> > > > I can have a try, but the detail is not clear for me now.
> > >
> > > Something along those lines:
> > >
> > > From 5f1c061e55c691d64012bc7c1490a1a8c4432c67 Mon Sep 17 00:00:00
> > > 2001
> > > From: Marc Zyngier <[email protected]>
> > > Date: Sat, 7 Sep 2019 10:11:49 +0100
> > > Subject: [PATCH] timekeeping: Expose API allowing retrival of
> > > current clocksource and counter value
> > >
> > > Signed-off-by: Marc Zyngier <[email protected]>
> > > ---
> > > include/linux/timekeeping.h | 5 +++++
> > > kernel/time/timekeeping.c | 12 ++++++++++++
> > > 2 files changed, 17 insertions(+)
> > >
> > > diff --git a/include/linux/timekeeping.h
> > > b/include/linux/timekeeping.h index
> > > b27e2ffa96c1..6df26a913711 100644
> > > --- a/include/linux/timekeeping.h
> > > +++ b/include/linux/timekeeping.h
> > > @@ -275,6 +275,11 @@ extern int get_device_system_crosststamp(
> > > struct system_time_snapshot *history,
> > > struct system_device_crosststamp *xtstamp);
> > >
> > > +/*
> > > + * Obtain current monotonic clock and its counter value */ extern
> > > +void get_current_counterval(struct system_counterval_t *sc);
> > > +
> > > /*
> > > * Simultaneously snapshot realtime and monotonic raw clocks
> > > */
> > > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > > index
> > > d911c8470149..de689bbd3808 100644
> > > --- a/kernel/time/timekeeping.c
> > > +++ b/kernel/time/timekeeping.c
> > > @@ -1098,6 +1098,18 @@ static bool cycle_between(u64 before, u64
> > > test,
> > > u64 after)
> > > return false;
> > > }
> > >
> > > +/**
> > > + * get_current_counterval - Snapshot the current clocksource and
> > > +counter
> > > value
> > > + * @sc: Pointer to a struct containing the current clocksource and its
> > > value
> > > + */
> > > +void get_current_counterval(struct system_counterval_t *sc) {
> > > + struct timekeeper *tk = &tk_core.timekeeper;
> > > +
> > > + sc->cs = READ_ONCE(tk->tkr_mono.clock);
> > > + sc->cycles = sc->cs->read(sc->cs); }
> > > +
> > > /**
> > > * get_device_system_crosststamp - Synchronously capture
> > > system/device timestamp
> > > * @get_time_fn: Callback to get simultaneous device time and
> > >
> > > which should do the right thing.
> > >
> > It is a good news for me. These code is indeed what I need! So what's
> > your plan about this patch? Is there any problem with you if I
> > include these code into my patch ?
>
> Just add this patch as part of your series (I'll try to write an actual commit log
> for that).
Very kind of you!
>
> [...]
>
> > > > > Other questions: how does this works with VM migration?
> > > > > Specially when moving from a hypervisor that supports the
> > > > > feature to one that
> > > doesn't?
> > > > >
> > > > I think it won't solve the problem generated by VM migration and
> > > > only for VMs in a single machine. Ptp_kvm only works for VMs in
> > > > the same machine. But using ptp (not ptp_kvm) clock, all the
> > > > machines in a low latency network environment can keep time sync
> > > > in high precision, Then VMs move from one machine to another will
> > > > obtain a high precision time sync.
> > >
> > > That's a problem. Migration must be possible from one host to
> > > another, even if that means temporarily loosing some (or a lot of)
> > > precision. The service must be discoverable from userspace on the
> > > host so that the MVV can decie whether a migration is possible or not.
> > >
> > Don't worry, things will be not that bad. ptp_kvm will not trouble
> > the VM migration. This ptp_kvm is one clocksource of the clock pool
> > for chrony. Chrony will choose the highest precision clock from the
> > pool. If host does not support ptp_kvm, the ptp_kvm will not be chosen
> > as the clocksouce of chrony. We have roughly the same logic of
> > implementation of ptp_kvm with x86, and ptp_kvm works well in x86. so
> > I think that will be the case for arm64.
> >
> > Maybe I miss your point, I have no idea of MVV and can't get related
> > info from google. Also I'm not clear of your last words of how to
> > decide VM migration is possible?
>
> Sorry. s/MVV/VMM/. Basically userspace, such as QEMU.
>
> Here's an example: The guest runs on a PTP aware host, starts using the PTP
> service and uses HVC calls to get its clock. We now migrate the guest to a non
> PTP-aware host. The hypercalls are now going to fail unexpectedly. Is that
> something that is acceptable? I don't think it is. Once you've allowed a guest
> to use a service, this service should be preserved. I'd be more confident if we
> gave to userspace the indication that the hypervisor supports PTP. Userspace
> can then decide whether to perform migration or not.
>
It's really a point we should consider. let me check the behavior of chrony in this scenario first.
Thanks
Jianyong Wu
> Thanks,
>
> M.
>
> --
> Jazz is not dead, it just smells funny.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Hi Marc,
I think there are three points for the migration issue of ptp_kvm, where a VM using ptp_kvm migrates to a host without ptp_kvm support.
First: how does it impact the VM having migrated?
I run a VM with ptp_kvm support in guest but not support in host. the ptp0 will return 0 when get time from it which can't pass the check
of chrony, then chrony will choose another clocksource. From this point, VM will only get lost in precision of time sync.
Second: how to check the failure of the ptp kvm service
when there is no ptp kvm service, hypercall will go into default ops, so we can check the return value which can inform us the failure.
Third: how to inform VMM
There is ioctl cmd call "KVM_CHECK_EXTENSION" in kvm, which may do that thing. Accordingly, qemu should be offered the support which will block us.
We can try to add this support in kvm but we are not sure the response from qemu side.
WDYT?
Jianyong Wu
Thanks
> -----Original Message-----
> From: Jianyong Wu (Arm Technology China)
> Sent: Tuesday, September 10, 2019 6:29 PM
> To: Marc Zyngier <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Mark Rutland
> <[email protected]>; Will Deacon <[email protected]>; Suzuki
> Poulose <[email protected]>; [email protected]; Steve
> Capper <[email protected]>; Kaly Xin (Arm Technology China)
> <[email protected]>; Justin He (Arm Technology China)
> <[email protected]>
> Subject: RE: [RFC PATCH 3/3] Enable ptp_kvm for arm64
>
> Hi Marc,
>
> > -----Original Message-----
> > From: Marc Zyngier <[email protected]>
> > Sent: Monday, September 9, 2019 7:25 PM
> > To: Jianyong Wu (Arm Technology China) <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]; Mark
> > Rutland <[email protected]>; Will Deacon <[email protected]>;
> > Suzuki Poulose <[email protected]>; [email protected];
> > Steve Capper <[email protected]>; Kaly Xin (Arm Technology China)
> > <[email protected]>; Justin He (Arm Technology China)
> > <[email protected]>
> > Subject: Re: [RFC PATCH 3/3] Enable ptp_kvm for arm64
> >
> >
> > > > > > Other questions: how does this works with VM migration?
> > > > > > Specially when moving from a hypervisor that supports the
> > > > > > feature to one that
> > > > doesn't?
> > > > > >
> > > > > I think it won't solve the problem generated by VM migration and
> > > > > only for VMs in a single machine. Ptp_kvm only works for VMs in
> > > > > the same machine. But using ptp (not ptp_kvm) clock, all the
> > > > > machines in a low latency network environment can keep time sync
> > > > > in high precision, Then VMs move from one machine to another
> > > > > will obtain a high precision time sync.
> > > >
> > > > That's a problem. Migration must be possible from one host to
> > > > another, even if that means temporarily loosing some (or a lot of)
> > > > precision. The service must be discoverable from userspace on the
> > > > host so that the MVV can decie whether a migration is possible or not.
> > > >
> > > Don't worry, things will be not that bad. ptp_kvm will not trouble
> > > the VM migration. This ptp_kvm is one clocksource of the clock pool
> > > for chrony. Chrony will choose the highest precision clock from the
> > > pool. If host does not support ptp_kvm, the ptp_kvm will not be
> > > chosen as the clocksouce of chrony. We have roughly the same logic
> > > of implementation of ptp_kvm with x86, and ptp_kvm works well in
> > > x86. so I think that will be the case for arm64.
> > >
> > > Maybe I miss your point, I have no idea of MVV and can't get related
> > > info from google. Also I'm not clear of your last words of how to
> > > decide VM migration is possible?
> >
> > Sorry. s/MVV/VMM/. Basically userspace, such as QEMU.
> >
> > Here's an example: The guest runs on a PTP aware host, starts using
> > the PTP service and uses HVC calls to get its clock. We now migrate
> > the guest to a non PTP-aware host. The hypercalls are now going to
> > fail unexpectedly. Is that something that is acceptable? I don't think
> > it is. Once you've allowed a guest to use a service, this service
> > should be preserved. I'd be more confident if we gave to userspace the
> > indication that the hypervisor supports PTP. Userspace can then decide
> whether to perform migration or not.
> >
>
> It's really a point we should consider. let me check the behavior of chrony in
> this scenario first.
>
> Thanks
> Jianyong Wu
>
> > Thanks,
> >
> > M.
> >
> > --
> > Jazz is not dead, it just smells funny.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On Wed, 11 Sep 2019 11:06:18 +0100,
"Jianyong Wu (Arm Technology China)" <[email protected]> wrote:
>
> Hi Marc,
>
> I think there are three points for the migration issue of ptp_kvm,
> where a VM using ptp_kvm migrates to a host without ptp_kvm support.
>
> First: how does it impact the VM having migrated?
> I run a VM with ptp_kvm support in guest but not support in
> host. the ptp0 will return 0 when get time from it which can't pass
> the check of chrony, then chrony will choose another clocksource.
> From this point, VM will only get lost in precision of time sync.
"only" is a bit of an understatement. Once the guest has started
relying on a service, it seems rather harsh to pretend this service
doesn't exist anymore. It could well be that the VM cannot perform its
function if the precision is not good enough.
The analogy is the Spectre-v2 mitigation, which is implemented as a
hypercall. Nothing will break if you migrate to a host that doesn't
support the mitigation, but the guest will now be unsafe. Is that
acceptable? the answer is of course "no".
> Second: how to check the failure of the ptp kvm service
> when there is no ptp kvm service, hypercall will go into default
> ops, so we can check the return value which can inform us the
> failure.
Sure. But that's still an issue. The VM relied on the service, and the
service isn't available anymore.
> Third: how to inform VMM
> There is ioctl cmd call "KVM_CHECK_EXTENSION" in kvm, which may do
> that thing. Accordingly, qemu should be offered the support which
> will block us. We can try to add this support in kvm but we are not
> sure the response from qemu side.
It doesn't matter whether QEMU implements that check or . The
important thing is that we give userspace a way to check this for
this, and having a capability that can be checked against is probably
the right thing to do.
Thanks,
M.
--
Jazz is not dead, it just smells funny.
Hi Marc,
> -----Original Message-----
> From: Marc Zyngier <[email protected]>
> Sent: Wednesday, September 11, 2019 7:31 PM
> To: Jianyong Wu (Arm Technology China) <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Mark Rutland
> <[email protected]>; Will Deacon <[email protected]>; Suzuki
> Poulose <[email protected]>; [email protected]; Steve
> Capper <[email protected]>; Kaly Xin (Arm Technology China)
> <[email protected]>; Justin He (Arm Technology China)
> <[email protected]>
> Subject: Re: [RFC PATCH 3/3] Enable ptp_kvm for arm64
>
> On Wed, 11 Sep 2019 11:06:18 +0100,
> "Jianyong Wu (Arm Technology China)" <[email protected]> wrote:
> >
> > Hi Marc,
> >
> > I think there are three points for the migration issue of ptp_kvm,
> > where a VM using ptp_kvm migrates to a host without ptp_kvm support.
> >
> > First: how does it impact the VM having migrated?
> > I run a VM with ptp_kvm support in guest but not support in host. the
> > ptp0 will return 0 when get time from it which can't pass the check of
> > chrony, then chrony will choose another clocksource.
> > From this point, VM will only get lost in precision of time sync.
>
> "only" is a bit of an understatement. Once the guest has started relying on a
> service, it seems rather harsh to pretend this service doesn't exist anymore.
> It could well be that the VM cannot perform its function if the precision is not
> good enough.
>
> The analogy is the Spectre-v2 mitigation, which is implemented as a hypercall.
> Nothing will break if you migrate to a host that doesn't support the mitigation,
> but the guest will now be unsafe. Is that acceptable? the answer is of course
> "no".
>
> > Second: how to check the failure of the ptp kvm service when there is
> > no ptp kvm service, hypercall will go into default ops, so we can
> > check the return value which can inform us the failure.
>
> Sure. But that's still an issue. The VM relied on the service, and the service
> isn't available anymore.
>
> > Third: how to inform VMM
> > There is ioctl cmd call "KVM_CHECK_EXTENSION" in kvm, which may do
> > that thing. Accordingly, qemu should be offered the support which will
> > block us. We can try to add this support in kvm but we are not sure
> > the response from qemu side.
>
> It doesn't matter whether QEMU implements that check or . The important
> thing is that we give userspace a way to check this for this, and having a
> capability that can be checked against is probably the right thing to do.
Ok, I agree.
Adding a new capability item under "KVM_CHECK_EXTENSION" in kvm for ptp_kvm will do and Using ioctl in userspace can check if the ptp service is available.
I will append this patch in this patch serial.
Thanks
Jianyong Wu
>
> Thanks,
>
> M.
>
> --
> Jazz is not dead, it just smells funny.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.