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

From: Marc Zyngier
Date: Thu Jan 09 2020 - 04:24:25 EST

Next message: Anson Huang: "RE: [PATCH 1/5] ARM: dts: imx6qdl-sabresd: Remove incorrect power supply assignment"
Previous message: David Hildenbrand: "Re: [PATCH v3] drivers/base/memory.c: cache blocks in radix tree to accelerate lookup"
In reply to: Jianyong Wu: "RE: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64"
Next in thread: Marc Zyngier: "Re: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 2020-01-09 05:59, Jianyong Wu wrote:

Hi Marc,

-----Original Message-----
From: Marc Zyngier <maz@xxxxxxxxxx>
Sent: Tuesday, January 7, 2020 5:29 PM
To: Jianyong Wu <Jianyong.Wu@xxxxxxx>
Cc: netdev@xxxxxxxxxxxxxxx; yangbo.lu@xxxxxxx; john.stultz@xxxxxxxxxx;
tglx@xxxxxxxxxxxxx; pbonzini@xxxxxxxxxx; sean.j.christopherson@xxxxxxxxx;
richardcochran@xxxxxxxxx; Mark Rutland <Mark.Rutland@xxxxxxx>;
will@xxxxxxxxxx; Suzuki Poulose <Suzuki.Poulose@xxxxxxx>; Steven Price
<Steven.Price@xxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; linux-arm-
kernel@xxxxxxxxxxxxxxxxxxx; kvmarm@xxxxxxxxxxxxxxxxxxxxx;
kvm@xxxxxxxxxxxxxxx; Steve Capper <Steve.Capper@xxxxxxx>; Kaly Xin
<Kaly.Xin@xxxxxxx>; Justin He <Justin.He@xxxxxxx>; nd <nd@xxxxxxx>
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@xxxxxxx>
> ---
> 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...

Next message: Anson Huang: "RE: [PATCH 1/5] ARM: dts: imx6qdl-sabresd: Remove incorrect power supply assignment"
Previous message: David Hildenbrand: "Re: [PATCH v3] drivers/base/memory.c: cache blocks in radix tree to accelerate lookup"
In reply to: Jianyong Wu: "RE: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64"
Next in thread: Marc Zyngier: "Re: [RFC PATCH v9 7/8] ptp: arm64: Enable ptp_kvm for arm64"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]