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

["Jianyong Wu (Arm Technology China)" <Jianyong.Wu@arm.com>]

Hi Marc,

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

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

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

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

Thanks
Jianyong Wu

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