| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <86h85osbzz.wl-maz@kernel.org>
Date: Sat, 07 Sep 2019 10:15:44 +0100
From: Marc Zyngier <maz@...nel.org>
To: "Jianyong Wu (Arm Technology China)" <Jianyong.Wu@....com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"pbonzini@...hat.com" <pbonzini@...hat.com>,
"sean.j.christopherson@...el.com" <sean.j.christopherson@...el.com>,
"richardcochran@...il.com" <richardcochran@...il.com>,
Mark Rutland <Mark.Rutland@....com>,
Will Deacon <Will.Deacon@....com>,
Suzuki Poulose <Suzuki.Poulose@....com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Steve Capper <Steve.Capper@....com>,
"Kaly Xin (Arm Technology China)" <Kaly.Xin@....com>,
"Justin He (Arm Technology China)" <Justin.He@....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@....com> wrote:
>
> Hi Marc,
>
> Very sorry to have missed this comments.
>
> > -----Original Message-----
> > From: Marc Zyngier <maz@...nel.org>
> > Sent: Thursday, August 29, 2019 6:33 PM
> > To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@....com>;
> > netdev@...r.kernel.org; pbonzini@...hat.com;
> > sean.j.christopherson@...el.com; richardcochran@...il.com; Mark Rutland
> > <Mark.Rutland@....com>; Will Deacon <Will.Deacon@....com>; Suzuki
> > Poulose <Suzuki.Poulose@....com>
> > Cc: linux-kernel@...r.kernel.org; Steve Capper <Steve.Capper@....com>;
> > Kaly Xin (Arm Technology China) <Kaly.Xin@....com>; Justin He (Arm
> > Technology China) <Justin.He@....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@....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@...nel.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@...nel.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.
Powered by blists - more mailing lists