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

[Marc Zyngier <maz@kernel.org>]

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

Something along those lines:

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

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

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

which should do the right thing.

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

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

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

and let the Makefile combine them.

[...]

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

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

Thanks,

	M.

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