[PATCH 0/3] Xen wallclock on arm and arm64

[PATCH 0/3] Xen wallclock on arm and arm64

[Stefano Stabellini]

Hi all,

this series introduces PV wallclock time support on arm and arm64.


Stefano Stabellini (3):
      xen/arm: introduce xen_read_wallclock
      xen/arm: introduce HYPERVISOR_dom0_op on arm and arm64
      xen/arm: set the system time in Xen via the XENPF_settime hypercall

 arch/arm/Kconfig                     |    1 +
 arch/arm/include/asm/xen/hypercall.h |    2 +
 arch/arm/xen/enlighten.c             |   82 ++++++++++++++++++++++++++++++++++
 arch/arm/xen/hypercall.S             |    1 +
 arch/arm64/xen/hypercall.S           |    1 +
 5 files changed, 87 insertions(+)


Cheers,

Stefano

[PATCH 1/3] xen/arm: introduce xen_read_wallclock

[Stefano Stabellini]

Read the wallclock from the shared info page at boot time.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 arch/arm/Kconfig         |    1 +
 arch/arm/xen/enlighten.c |   31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 60be104..a9de420 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1852,6 +1852,7 @@ config XEN
 	depends on CPU_V7 && !CPU_V6
 	depends on !GENERIC_ATOMIC64
 	depends on MMU
+	depends on HAVE_ARM_ARCH_TIMER
 	select ARCH_DMA_ADDR_T_64BIT
 	select ARM_PSCI
 	select SWIOTLB_XEN
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 15621b1..f07383d 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -28,6 +28,8 @@
 #include <linux/cpufreq.h>
 #include <linux/cpu.h>
 #include <linux/console.h>
+#include <linux/timekeeping.h>
+#include <clocksource/arm_arch_timer.h>
 
 #include <linux/mm.h>
 
@@ -95,6 +97,32 @@ static unsigned long long xen_stolen_accounting(int cpu)
 	return state.time[RUNSTATE_runnable] + state.time[RUNSTATE_offline];
 }
 
+static void xen_read_wallclock(struct timespec *ts)
+{
+	u32 version;
+	u64 delta;
+	struct timespec now;
+	struct shared_info *s = HYPERVISOR_shared_info;
+	struct pvclock_wall_clock *wall_clock = &(s->wc);
+
+	/* get wallclock at system boot */
+	do {
+		version = wall_clock->version;
+		rmb();		/* fetch version before time */
+		now.tv_sec  = wall_clock->sec;
+		now.tv_nsec = wall_clock->nsec;
+		rmb();		/* fetch time before checking version */
+	} while ((wall_clock->version & 1) || (version != wall_clock->version));
+
+	delta = arch_timer_read_counter();	/* time since system boot */
+	delta += now.tv_sec * (u64)NSEC_PER_SEC + now.tv_nsec;
+
+	now.tv_nsec = do_div(delta, NSEC_PER_SEC);
+	now.tv_sec = delta;
+
+	set_normalized_timespec(ts, now.tv_sec, now.tv_nsec);
+}
+
 static void xen_percpu_init(void)
 {
 	struct vcpu_register_vcpu_info info;
@@ -218,6 +246,7 @@ static int __init xen_guest_init(void)
 	struct shared_info *shared_info_page = NULL;
 	struct resource res;
 	phys_addr_t grant_frames;
+	struct timespec ts;
 
 	if (!xen_domain())
 		return 0;
@@ -291,6 +320,8 @@ static int __init xen_guest_init(void)
 
 	pv_time_ops.steal_clock = xen_stolen_accounting;
 	static_key_slow_inc(&paravirt_steal_enabled);
+	xen_read_wallclock(&ts);
+	do_settimeofday(&ts);
 
 	return 0;
 }
-- 
1.7.10.4

[PATCH 1/3] xen/arm: introduce xen_read_wallclock

[Arnd Bergmann]

On Thursday 05 November 2015 17:09:43 Stefano Stabellini wrote:
> Read the wallclock from the shared info page at boot time.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Please use the appropriate timespec64 based functions here,
we are in the process of converting all callers of struct timespec.

> ---
> +static void xen_read_wallclock(struct timespec *ts)
> +{
> +	u32 version;
> +	u64 delta;
> +	struct timespec now;
> +	struct shared_info *s = HYPERVISOR_shared_info;
> +	struct pvclock_wall_clock *wall_clock = &(s->wc);

pvclock_wall_clock has a 'u32 sec', so that suffers from
a potential overflow. We should try to avoid introducing that
on ARM, and instead have a 64-bit seconds based version,
or alternatively a 64-bit nanoseconds version (with no
extra seconds) that is also sufficient.

>  	struct vcpu_register_vcpu_info info;
> @@ -218,6 +246,7 @@ static int __init xen_guest_init(void)
>  	struct shared_info *shared_info_page = NULL;
>  	struct resource res;
>  	phys_addr_t grant_frames;
> +	struct timespec ts;
>  
>  	if (!xen_domain())
>  		return 0;
> @@ -291,6 +320,8 @@ static int __init xen_guest_init(void)
>  
>  	pv_time_ops.steal_clock = xen_stolen_accounting;
>  	static_key_slow_inc(&paravirt_steal_enabled);
> +	xen_read_wallclock(&ts);
> +	do_settimeofday(&ts);
>  

Once you can get a 64-bit time, call do_settimeofday64()
here.

	Arnd

[PATCH 1/3] xen/arm: introduce xen_read_wallclock

[Stefano Stabellini]

On Thu, 5 Nov 2015, Arnd Bergmann wrote:
> On Thursday 05 November 2015 17:09:43 Stefano Stabellini wrote:
> > Read the wallclock from the shared info page at boot time.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> Please use the appropriate timespec64 based functions here,
> we are in the process of converting all callers of struct timespec.

OK, I can do that


> > ---
> > +static void xen_read_wallclock(struct timespec *ts)
> > +{
> > +	u32 version;
> > +	u64 delta;
> > +	struct timespec now;
> > +	struct shared_info *s = HYPERVISOR_shared_info;
> > +	struct pvclock_wall_clock *wall_clock = &(s->wc);
> 
> pvclock_wall_clock has a 'u32 sec', so that suffers from
> a potential overflow. We should try to avoid introducing that
> on ARM, and instead have a 64-bit seconds based version,
> or alternatively a 64-bit nanoseconds version (with no
> extra seconds) that is also sufficient.

Unfortunately this a preexisting ABI which is common with other existing
architectures (see arch/x86/kernel/pvclock.c:pvclock_read_wallclock). I
cannot just change it.


> >  	struct vcpu_register_vcpu_info info;
> > @@ -218,6 +246,7 @@ static int __init xen_guest_init(void)
> >  	struct shared_info *shared_info_page = NULL;
> >  	struct resource res;
> >  	phys_addr_t grant_frames;
> > +	struct timespec ts;
> >  
> >  	if (!xen_domain())
> >  		return 0;
> > @@ -291,6 +320,8 @@ static int __init xen_guest_init(void)
> >  
> >  	pv_time_ops.steal_clock = xen_stolen_accounting;
> >  	static_key_slow_inc(&paravirt_steal_enabled);
> > +	xen_read_wallclock(&ts);
> > +	do_settimeofday(&ts);
> >  
> 
> Once you can get a 64-bit time, call do_settimeofday64()
> here.

OK

[PATCH 1/3] xen/arm: introduce xen_read_wallclock

[Arnd Bergmann]

On Friday 06 November 2015 15:09:53 Stefano Stabellini wrote:
> > > ---
> > > +static void xen_read_wallclock(struct timespec *ts)
> > > +{
> > > +   u32 version;
> > > +   u64 delta;
> > > +   struct timespec now;
> > > +   struct shared_info *s = HYPERVISOR_shared_info;
> > > +   struct pvclock_wall_clock *wall_clock = &(s->wc);
> > 
> > pvclock_wall_clock has a 'u32 sec', so that suffers from
> > a potential overflow. We should try to avoid introducing that
> > on ARM, and instead have a 64-bit seconds based version,
> > or alternatively a 64-bit nanoseconds version (with no
> > extra seconds) that is also sufficient.
> 
> Unfortunately this a preexisting ABI which is common with other existing
> architectures (see arch/x86/kernel/pvclock.c:pvclock_read_wallclock). I
> cannot just change it.

Maybe we need a "struct pvclock_wall_clock_v2"?

In theory, 'u32 sec' takes us all the way until 2106, which is significantly
longer away than the signed overflow in 2038, but it would still be nice
to not introduce this limitation on ARM in the first place.

I'm not quite sure about how the split between pvclock_wall_clock and
the delta works. Normally I'd expect that pvclock_wall_clock is the wallclock
time as it was at boot, while delta is the number of expired nanoseconds
since boot. However it is unclear why the latter has a longer range
(539 years starting at boot, rather than 126 years starting in 1970).

	Arnd

[PATCH 1/3] xen/arm: introduce xen_read_wallclock

[Stefano Stabellini]

On Fri, 6 Nov 2015, Arnd Bergmann wrote:
> > > > ---
> > > > +static void xen_read_wallclock(struct timespec *ts)
> > > > +{
> > > > +   u32 version;
> > > > +   u64 delta;
> > > > +   struct timespec now;
> > > > +   struct shared_info *s = HYPERVISOR_shared_info;
> > > > +   struct pvclock_wall_clock *wall_clock = &(s->wc);
> > > 
> > > pvclock_wall_clock has a 'u32 sec', so that suffers from
> > > a potential overflow. We should try to avoid introducing that
> > > on ARM, and instead have a 64-bit seconds based version,
> > > or alternatively a 64-bit nanoseconds version (with no
> > > extra seconds) that is also sufficient.
> > 
> > Unfortunately this a preexisting ABI which is common with other existing
> > architectures (see arch/x86/kernel/pvclock.c:pvclock_read_wallclock). I
> > cannot just change it.
> 
> Maybe we need a "struct pvclock_wall_clock_v2"?
> 
> In theory, 'u32 sec' takes us all the way until 2106, which is significantly
> longer away than the signed overflow in 2038, but it would still be nice
> to not introduce this limitation on ARM in the first place.
> 
> I'm not quite sure about how the split between pvclock_wall_clock and
> the delta works. Normally I'd expect that pvclock_wall_clock is the wallclock
> time as it was at boot, while delta is the number of expired nanoseconds
> since boot. However it is unclear why the latter has a longer range
> (539 years starting at boot, rather than 126 years starting in 1970).

Actually we already have a sec overflow field in struct shared_info on
the hypervisor side, called wc_sec_hi. I just need to make use of it in
Linux. See:

http://xenbits.xen.org/gitweb/?p=xen.git;a=blob_plain;f=xen/include/public/xen.h;hb=HEAD

Thanks for raising my attention to the problem,

Stefano

[PATCH 1/3] xen/arm: introduce xen_read_wallclock

[Arnd Bergmann]

On Monday 09 November 2015 13:53:30 Stefano Stabellini wrote:
> On Fri, 6 Nov 2015, Arnd Bergmann wrote:
> > I'm not quite sure about how the split between pvclock_wall_clock and
> > the delta works. Normally I'd expect that pvclock_wall_clock is the wallclock
> > time as it was at boot, while delta is the number of expired nanoseconds
> > since boot. However it is unclear why the latter has a longer range
> > (539 years starting at boot, rather than 126 years starting in 1970).
> 
> Actually we already have a sec overflow field in struct shared_info on
> the hypervisor side, called wc_sec_hi. I just need to make use of it in
> Linux. See:
> 
> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob_plain;f=xen/include/public/xen.h;hb=HEAD
> 
> Thanks for raising my attention to the problem,

Sounds good for Xen on ARM. This same interface is also used on
KVM, right? Does the extension also work there?

	Arnd

[PATCH 1/3] xen/arm: introduce xen_read_wallclock

[Stefano Stabellini]

On Mon, 9 Nov 2015, Arnd Bergmann wrote:
> On Monday 09 November 2015 13:53:30 Stefano Stabellini wrote:
> > On Fri, 6 Nov 2015, Arnd Bergmann wrote:
> > > I'm not quite sure about how the split between pvclock_wall_clock and
> > > the delta works. Normally I'd expect that pvclock_wall_clock is the wallclock
> > > time as it was at boot, while delta is the number of expired nanoseconds
> > > since boot. However it is unclear why the latter has a longer range
> > > (539 years starting at boot, rather than 126 years starting in 1970).
> > 
> > Actually we already have a sec overflow field in struct shared_info on
> > the hypervisor side, called wc_sec_hi. I just need to make use of it in
> > Linux. See:
> > 
> > http://xenbits.xen.org/gitweb/?p=xen.git;a=blob_plain;f=xen/include/public/xen.h;hb=HEAD
> > 
> > Thanks for raising my attention to the problem,
> 
> Sounds good for Xen on ARM. This same interface is also used on
> KVM, right? Does the extension also work there?

It doesn't look like it (see arch/x86/kvm/x86.c:kvm_write_wall_clock).

[PATCH 1/3] xen/arm: introduce xen_read_wallclock

[Arnd Bergmann]

On Monday 09 November 2015 17:14:24 Stefano Stabellini wrote:
> On Mon, 9 Nov 2015, Arnd Bergmann wrote:
> > On Monday 09 November 2015 13:53:30 Stefano Stabellini wrote:
> > > On Fri, 6 Nov 2015, Arnd Bergmann wrote:
> > > > I'm not quite sure about how the split between pvclock_wall_clock and
> > > > the delta works. Normally I'd expect that pvclock_wall_clock is the wallclock
> > > > time as it was at boot, while delta is the number of expired nanoseconds
> > > > since boot. However it is unclear why the latter has a longer range
> > > > (539 years starting at boot, rather than 126 years starting in 1970).
> > > 
> > > Actually we already have a sec overflow field in struct shared_info on
> > > the hypervisor side, called wc_sec_hi. I just need to make use of it in
> > > Linux. See:
> > > 
> > > http://xenbits.xen.org/gitweb/?p=xen.git;a=blob_plain;f=xen/include/public/xen.h;hb=HEAD
> > > 
> > > Thanks for raising my attention to the problem,
> > 
> > Sounds good for Xen on ARM. This same interface is also used on
> > KVM, right? Does the extension also work there?
> 
> It doesn't look like it (see arch/x86/kvm/x86.c:kvm_write_wall_clock).

(adding Christoffer and Marc)

That kvm_set_msr_common() function appears to be very x86 specific though,
and I'm not sure what the ARM equivalent would be.

How does KVM get/set the system time today on ARM and ARM64? Is this
going to be done through a PSCI call based on pvclock_wall_clock in
the future?

	Arnd

[PATCH 1/3] xen/arm: introduce xen_read_wallclock

[Mark Rutland]

> +	delta = arch_timer_read_counter();	/* time since system boot */
> +	delta += now.tv_sec * (u64)NSEC_PER_SEC + now.tv_nsec;

The arch counter value is not a number of nanoseconds (unless CNTFRQ
reads as 1000000), so this doesn't look right; the units don't match.

Thanks,
Mark.

[PATCH 1/3] xen/arm: introduce xen_read_wallclock

[Stefano Stabellini]

On Fri, 6 Nov 2015, Mark Rutland wrote:
> > +	delta = arch_timer_read_counter();	/* time since system boot */
> > +	delta += now.tv_sec * (u64)NSEC_PER_SEC + now.tv_nsec;
> 
> The arch counter value is not a number of nanoseconds (unless CNTFRQ
> reads as 1000000), so this doesn't look right; the units don't match.

That was a bad mistake. Thanks for catching it!

[PATCH 2/3] xen/arm: introduce HYPERVISOR_dom0_op on arm and arm64

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 arch/arm/include/asm/xen/hypercall.h |    2 ++
 arch/arm/xen/enlighten.c             |    1 +
 arch/arm/xen/hypercall.S             |    1 +
 arch/arm64/xen/hypercall.S           |    1 +
 4 files changed, 5 insertions(+)

diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h
index 712b50e..7a8ee15 100644
--- a/arch/arm/include/asm/xen/hypercall.h
+++ b/arch/arm/include/asm/xen/hypercall.h
@@ -35,6 +35,7 @@
 
 #include <xen/interface/xen.h>
 #include <xen/interface/sched.h>
+#include <xen/interface/platform.h>
 
 long privcmd_call(unsigned call, unsigned long a1,
 		unsigned long a2, unsigned long a3,
@@ -49,6 +50,7 @@ int HYPERVISOR_memory_op(unsigned int cmd, void *arg);
 int HYPERVISOR_physdev_op(int cmd, void *arg);
 int HYPERVISOR_vcpu_op(int cmd, int vcpuid, void *extra_args);
 int HYPERVISOR_tmem_op(void *arg);
+int HYPERVISOR_dom0_op(void *arg);
 int HYPERVISOR_multicall(struct multicall_entry *calls, uint32_t nr);
 
 static inline int
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index f07383d..b6aea9c 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -359,5 +359,6 @@ EXPORT_SYMBOL_GPL(HYPERVISOR_memory_op);
 EXPORT_SYMBOL_GPL(HYPERVISOR_physdev_op);
 EXPORT_SYMBOL_GPL(HYPERVISOR_vcpu_op);
 EXPORT_SYMBOL_GPL(HYPERVISOR_tmem_op);
+EXPORT_SYMBOL_GPL(HYPERVISOR_dom0_op);
 EXPORT_SYMBOL_GPL(HYPERVISOR_multicall);
 EXPORT_SYMBOL_GPL(privcmd_call);
diff --git a/arch/arm/xen/hypercall.S b/arch/arm/xen/hypercall.S
index 10fd99c..89db58f 100644
--- a/arch/arm/xen/hypercall.S
+++ b/arch/arm/xen/hypercall.S
@@ -89,6 +89,7 @@ HYPERCALL2(memory_op);
 HYPERCALL2(physdev_op);
 HYPERCALL3(vcpu_op);
 HYPERCALL1(tmem_op);
+HYPERCALL1(dom0_op);
 HYPERCALL2(multicall);
 
 ENTRY(privcmd_call)
diff --git a/arch/arm64/xen/hypercall.S b/arch/arm64/xen/hypercall.S
index 8bbe940..3840b1a 100644
--- a/arch/arm64/xen/hypercall.S
+++ b/arch/arm64/xen/hypercall.S
@@ -80,6 +80,7 @@ HYPERCALL2(memory_op);
 HYPERCALL2(physdev_op);
 HYPERCALL3(vcpu_op);
 HYPERCALL1(tmem_op);
+HYPERCALL1(dom0_op);
 HYPERCALL2(multicall);
 
 ENTRY(privcmd_call)
-- 
1.7.10.4

[Xen-devel] [PATCH 2/3] xen/arm: introduce HYPERVISOR_dom0_op on arm and arm64

[Jan Beulich]

>>> On 05.11.15 at 18:09, <stefano.stabellini@eu.citrix.com> wrote:
> --- a/arch/arm/xen/hypercall.S
> +++ b/arch/arm/xen/hypercall.S
> @@ -89,6 +89,7 @@ HYPERCALL2(memory_op);
>  HYPERCALL2(physdev_op);
>  HYPERCALL3(vcpu_op);
>  HYPERCALL1(tmem_op);
> +HYPERCALL1(dom0_op);

Assuming this somehow tries to mirror x86 naming - time to rename it
there? I don't see why you'd want to introduce a dom0_op when it
has been renamed to platform_op many years ago - see
public/dom0_ops.h.

Jan

[Xen-devel] [PATCH 2/3] xen/arm: introduce HYPERVISOR_dom0_op on arm and arm64

[Stefano Stabellini]

On Thu, 5 Nov 2015, Jan Beulich wrote:
> >>> On 05.11.15 at 18:09, <stefano.stabellini@eu.citrix.com> wrote:
> > --- a/arch/arm/xen/hypercall.S
> > +++ b/arch/arm/xen/hypercall.S
> > @@ -89,6 +89,7 @@ HYPERCALL2(memory_op);
> >  HYPERCALL2(physdev_op);
> >  HYPERCALL3(vcpu_op);
> >  HYPERCALL1(tmem_op);
> > +HYPERCALL1(dom0_op);
> 
> Assuming this somehow tries to mirror x86 naming - time to rename it
> there? I don't see why you'd want to introduce a dom0_op when it
> has been renamed to platform_op many years ago - see
> public/dom0_ops.h.

In Linux it was never renamed. I could do that now as a precursor to
this.

[PATCH 3/3] xen/arm: set the system time in Xen via the XENPF_settime hypercall

[Stefano Stabellini]

If Linux is running as dom0, call XENPF_settime to update the system
time in Xen on pvclock_gtod notifications.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/arm/xen/enlighten.c |   52 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index b6aea9c..0176db0 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -28,6 +28,7 @@
 #include <linux/cpufreq.h>
 #include <linux/cpu.h>
 #include <linux/console.h>
+#include <linux/pvclock_gtod.h>
 #include <linux/timekeeping.h>
 #include <clocksource/arm_arch_timer.h>
 
@@ -123,6 +124,50 @@ static void xen_read_wallclock(struct timespec *ts)
 	set_normalized_timespec(ts, now.tv_sec, now.tv_nsec);
 }
 
+static int xen_pvclock_gtod_notify(struct notifier_block *nb,
+				   unsigned long was_set, void *priv)
+{
+	/* Protected by the calling core code serialization */
+	static struct timespec next_sync;
+
+	struct xen_platform_op op;
+	struct timespec now;
+
+	now = __current_kernel_time();
+
+	/*
+	 * We only take the expensive HV call when the clock was set
+	 * or when the 11 minutes RTC synchronization time elapsed.
+	 */
+	if (!was_set && timespec_compare(&now, &next_sync) < 0)
+		return NOTIFY_OK;
+
+	op.interface_version = XENPF_INTERFACE_VERSION;
+	op.cmd = XENPF_settime;
+	op.u.settime.secs = now.tv_sec;
+	op.u.settime.nsecs = now.tv_nsec;
+	op.u.settime.system_time = arch_timer_read_counter();
+	printk("GTOD: Setting to %ld.%ld@%lld\n",
+	       (long)op.u.settime.secs,
+	       (long)op.u.settime.nsecs,
+	       (long long)op.u.settime.system_time);
+	(void)HYPERVISOR_dom0_op(&op);
+
+	/*
+	 * Move the next drift compensation time 11 minutes
+	 * ahead. That's emulating the sync_cmos_clock() update for
+	 * the hardware RTC.
+	 */
+	next_sync = now;
+	next_sync.tv_sec += 11 * 60;
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block xen_pvclock_gtod_notifier = {
+	.notifier_call = xen_pvclock_gtod_notify,
+};
+
 static void xen_percpu_init(void)
 {
 	struct vcpu_register_vcpu_info info;
@@ -321,7 +366,12 @@ static int __init xen_guest_init(void)
 	pv_time_ops.steal_clock = xen_stolen_accounting;
 	static_key_slow_inc(&paravirt_steal_enabled);
 	xen_read_wallclock(&ts);
-	do_settimeofday(&ts);
+	if (xen_initial_domain())
+		pvclock_gtod_register_notifier(&xen_pvclock_gtod_notifier);
+	else {
+		xen_read_wallclock(&ts);
+		do_settimeofday(&ts);
+	}
 
 	return 0;
 }
-- 
1.7.10.4

[Xen-devel] [PATCH 3/3] xen/arm: set the system time in Xen via the XENPF_settime hypercall

[David Vrabel]

On 05/11/15 17:09, Stefano Stabellini wrote:
> If Linux is running as dom0, call XENPF_settime to update the system
> time in Xen on pvclock_gtod notifications.

Isn't this a cut-and-paste from x86?  Can you make it common?

David

[Xen-devel] [PATCH 3/3] xen/arm: set the system time in Xen via the XENPF_settime hypercall

[Stefano Stabellini]

On Thu, 5 Nov 2015, David Vrabel wrote:
> On 05/11/15 17:09, Stefano Stabellini wrote:
> > If Linux is running as dom0, call XENPF_settime to update the system
> > time in Xen on pvclock_gtod notifications.
> 
> Isn't this a cut-and-paste from x86?  Can you make it common?

Not exactly, for the way system_time is set.

[PATCH 3/3] xen/arm: set the system time in Xen via the XENPF_settime hypercall

[Arnd Bergmann]

On Thursday 05 November 2015 17:09:45 Stefano Stabellini wrote:
> If Linux is running as dom0, call XENPF_settime to update the system
> time in Xen on pvclock_gtod notifications.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  arch/arm/xen/enlighten.c |   52 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index b6aea9c..0176db0 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -28,6 +28,7 @@
>  #include <linux/cpufreq.h>
>  #include <linux/cpu.h>
>  #include <linux/console.h>
> +#include <linux/pvclock_gtod.h>
>  #include <linux/timekeeping.h>
>  #include <clocksource/arm_arch_timer.h>
>  
> @@ -123,6 +124,50 @@ static void xen_read_wallclock(struct timespec *ts)
>  	set_normalized_timespec(ts, now.tv_sec, now.tv_nsec);
>  }
>  
> +static int xen_pvclock_gtod_notify(struct notifier_block *nb,
> +				   unsigned long was_set, void *priv)
> +{
> +	/* Protected by the calling core code serialization */
> +	static struct timespec next_sync;
> +
> +	struct xen_platform_op op;
> +	struct timespec now;

Again, use timespec64

> +	now = __current_kernel_time();

We don't have __current_kernel_time64() yet, but it is trivial
to add, just follow the example of
current_kernel_time()/current_kernel_time64() and convert the
existing __current_kernel_time() function into a static
inline wrapper for the new __current_kernel_time64().

> +	/*
> +	 * We only take the expensive HV call when the clock was set
> +	 * or when the 11 minutes RTC synchronization time elapsed.
> +	 */
> +	if (!was_set && timespec_compare(&now, &next_sync) < 0)
> +		return NOTIFY_OK;
> +
> +	op.interface_version = XENPF_INTERFACE_VERSION;
> +	op.cmd = XENPF_settime;
> +	op.u.settime.secs = now.tv_sec;
> +	op.u.settime.nsecs = now.tv_nsec;
> +	op.u.settime.system_time = arch_timer_read_counter();
> +	printk("GTOD: Setting to %ld.%ld at %lld\n",
> +	       (long)op.u.settime.secs,
> +	       (long)op.u.settime.nsecs,
> +	       (long long)op.u.settime.system_time);
> +	(void)HYPERVISOR_dom0_op(&op);

I guess we will also need a XENPF_settime64 interface, but at
least we can get away with implementing only that one on ARM,
while x86 will have to support both the 32-bit and 64-bit
based variant.

	Arnd

[PATCH 3/3] xen/arm: set the system time in Xen via the XENPF_settime hypercall

[Stefano Stabellini]

On Thu, 5 Nov 2015, Arnd Bergmann wrote:
> On Thursday 05 November 2015 17:09:45 Stefano Stabellini wrote:
> > If Linux is running as dom0, call XENPF_settime to update the system
> > time in Xen on pvclock_gtod notifications.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  arch/arm/xen/enlighten.c |   52 +++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 51 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > index b6aea9c..0176db0 100644
> > --- a/arch/arm/xen/enlighten.c
> > +++ b/arch/arm/xen/enlighten.c
> > @@ -28,6 +28,7 @@
> >  #include <linux/cpufreq.h>
> >  #include <linux/cpu.h>
> >  #include <linux/console.h>
> > +#include <linux/pvclock_gtod.h>
> >  #include <linux/timekeeping.h>
> >  #include <clocksource/arm_arch_timer.h>
> >  
> > @@ -123,6 +124,50 @@ static void xen_read_wallclock(struct timespec *ts)
> >  	set_normalized_timespec(ts, now.tv_sec, now.tv_nsec);
> >  }
> >  
> > +static int xen_pvclock_gtod_notify(struct notifier_block *nb,
> > +				   unsigned long was_set, void *priv)
> > +{
> > +	/* Protected by the calling core code serialization */
> > +	static struct timespec next_sync;
> > +
> > +	struct xen_platform_op op;
> > +	struct timespec now;
> 
> Again, use timespec64

OK


> > +	now = __current_kernel_time();
> 
> We don't have __current_kernel_time64() yet, but it is trivial
> to add, just follow the example of
> current_kernel_time()/current_kernel_time64() and convert the
> existing __current_kernel_time() function into a static
> inline wrapper for the new __current_kernel_time64().

All right. I guess something like:

struct timespec64 __current_kernel_time64(void)
{
	struct timekeeper *tk = &tk_core.timekeeper;

	return tk_xtime(tk);
}


> > +	/*
> > +	 * We only take the expensive HV call when the clock was set
> > +	 * or when the 11 minutes RTC synchronization time elapsed.
> > +	 */
> > +	if (!was_set && timespec_compare(&now, &next_sync) < 0)
> > +		return NOTIFY_OK;
> > +
> > +	op.interface_version = XENPF_INTERFACE_VERSION;
> > +	op.cmd = XENPF_settime;
> > +	op.u.settime.secs = now.tv_sec;
> > +	op.u.settime.nsecs = now.tv_nsec;
> > +	op.u.settime.system_time = arch_timer_read_counter();
> > +	printk("GTOD: Setting to %ld.%ld at %lld\n",
> > +	       (long)op.u.settime.secs,
> > +	       (long)op.u.settime.nsecs,
> > +	       (long long)op.u.settime.system_time);
> > +	(void)HYPERVISOR_dom0_op(&op);
> 
> I guess we will also need a XENPF_settime64 interface, but at
> least we can get away with implementing only that one on ARM,
> while x86 will have to support both the 32-bit and 64-bit
> based variant.

We already have XENPF_settime64, I'll just use it instead.

[PATCH 3/3] xen/arm: set the system time in Xen via the XENPF_settime hypercall

[Arnd Bergmann]

On Monday 09 November 2015 14:10:22 Stefano Stabellini wrote:
> On Thu, 5 Nov 2015, Arnd Bergmann wrote:
> > On Thursday 05 November 2015 17:09:45 Stefano Stabellini wrote:
> > > +	now = __current_kernel_time();
> > 
> > We don't have __current_kernel_time64() yet, but it is trivial
> > to add, just follow the example of
> > current_kernel_time()/current_kernel_time64() and convert the
> > existing __current_kernel_time() function into a static
> > inline wrapper for the new __current_kernel_time64().
> 
> All right. I guess something like:
> 
> struct timespec64 __current_kernel_time64(void)
> {
> 	struct timekeeper *tk = &tk_core.timekeeper;
> 
> 	return tk_xtime(tk);
> }

Yes, exactly.

Just to make sure that this is actually the correct interface
that you want to call:

__current_kernel_time{,64}() is the fastest interface we have
to get an approximation of the current time, while ignoring
all of the locking.

Is is possible that you instead want ktime_get_real_ts64(),
which gives you the time as precise as the kernel knows it,
but uses locking?

> > > +	/*
> > > +	 * We only take the expensive HV call when the clock was set
> > > +	 * or when the 11 minutes RTC synchronization time elapsed.
> > > +	 */
> > > +	if (!was_set && timespec_compare(&now, &next_sync) < 0)
> > > +		return NOTIFY_OK;
> > > +
> > > +	op.interface_version = XENPF_INTERFACE_VERSION;
> > > +	op.cmd = XENPF_settime;
> > > +	op.u.settime.secs = now.tv_sec;
> > > +	op.u.settime.nsecs = now.tv_nsec;
> > > +	op.u.settime.system_time = arch_timer_read_counter();
> > > +	printk("GTOD: Setting to %ld.%ld at %lld\n",
> > > +	       (long)op.u.settime.secs,
> > > +	       (long)op.u.settime.nsecs,
> > > +	       (long long)op.u.settime.system_time);
> > > +	(void)HYPERVISOR_dom0_op(&op);
> > 
> > I guess we will also need a XENPF_settime64 interface, but at
> > least we can get away with implementing only that one on ARM,
> > while x86 will have to support both the 32-bit and 64-bit
> > based variant.
> 
> We already have XENPF_settime64, I'll just use it instead.

Ok, great! Then we just need to find a volunteer who can
do the same thing on x86, with the fallback to XENPF_settime
that they need for older hosts.

	Arnd

[PATCH 3/3] xen/arm: set the system time in Xen via the XENPF_settime hypercall

[Stefano Stabellini]

On Mon, 9 Nov 2015, Arnd Bergmann wrote:
> On Monday 09 November 2015 14:10:22 Stefano Stabellini wrote:
> > On Thu, 5 Nov 2015, Arnd Bergmann wrote:
> > > On Thursday 05 November 2015 17:09:45 Stefano Stabellini wrote:
> > > > +	now = __current_kernel_time();
> > > 
> > > We don't have __current_kernel_time64() yet, but it is trivial
> > > to add, just follow the example of
> > > current_kernel_time()/current_kernel_time64() and convert the
> > > existing __current_kernel_time() function into a static
> > > inline wrapper for the new __current_kernel_time64().
> > 
> > All right. I guess something like:
> > 
> > struct timespec64 __current_kernel_time64(void)
> > {
> > 	struct timekeeper *tk = &tk_core.timekeeper;
> > 
> > 	return tk_xtime(tk);
> > }
> 
> Yes, exactly.
> 
> Just to make sure that this is actually the correct interface
> that you want to call:
> 
> __current_kernel_time{,64}() is the fastest interface we have
> to get an approximation of the current time, while ignoring
> all of the locking.
> 
> Is is possible that you instead want ktime_get_real_ts64(),
> which gives you the time as precise as the kernel knows it,
> but uses locking?

I am not 100% sure. Can I call ktime_get_real_ts64 from a
pvclock_gtod notifier_call function?

[PATCH 3/3] xen/arm: set the system time in Xen via the XENPF_settime hypercall

[Stefano Stabellini]

On Mon, 9 Nov 2015, Stefano Stabellini wrote:
> On Mon, 9 Nov 2015, Arnd Bergmann wrote:
> > On Monday 09 November 2015 14:10:22 Stefano Stabellini wrote:
> > > On Thu, 5 Nov 2015, Arnd Bergmann wrote:
> > > > On Thursday 05 November 2015 17:09:45 Stefano Stabellini wrote:
> > > > > +	now = __current_kernel_time();
> > > > 
> > > > We don't have __current_kernel_time64() yet, but it is trivial
> > > > to add, just follow the example of
> > > > current_kernel_time()/current_kernel_time64() and convert the
> > > > existing __current_kernel_time() function into a static
> > > > inline wrapper for the new __current_kernel_time64().
> > > 
> > > All right. I guess something like:
> > > 
> > > struct timespec64 __current_kernel_time64(void)
> > > {
> > > 	struct timekeeper *tk = &tk_core.timekeeper;
> > > 
> > > 	return tk_xtime(tk);
> > > }
> > 
> > Yes, exactly.
> > 
> > Just to make sure that this is actually the correct interface
> > that you want to call:
> > 
> > __current_kernel_time{,64}() is the fastest interface we have
> > to get an approximation of the current time, while ignoring
> > all of the locking.
> > 
> > Is is possible that you instead want ktime_get_real_ts64(),
> > which gives you the time as precise as the kernel knows it,
> > but uses locking?
> 
> I am not 100% sure. Can I call ktime_get_real_ts64 from a
> pvclock_gtod notifier_call function?

It doesn't look like it: we would be getting the tk_core seqcount twice.

[PATCH 3/3] xen/arm: set the system time in Xen via the XENPF_settime hypercall

[Arnd Bergmann]

On Monday 09 November 2015 17:42:50 Stefano Stabellini wrote:
> On Mon, 9 Nov 2015, Stefano Stabellini wrote:
> > On Mon, 9 Nov 2015, Arnd Bergmann wrote:
> > > On Monday 09 November 2015 14:10:22 Stefano Stabellini wrote:
> > > 
> > > Just to make sure that this is actually the correct interface
> > > that you want to call:
> > > 
> > > __current_kernel_time{,64}() is the fastest interface we have
> > > to get an approximation of the current time, while ignoring
> > > all of the locking.
> > > 
> > > Is is possible that you instead want ktime_get_real_ts64(),
> > > which gives you the time as precise as the kernel knows it,
> > > but uses locking?
> > 
> > I am not 100% sure. Can I call ktime_get_real_ts64 from a
> > pvclock_gtod notifier_call function?
> 
> It doesn't look like it: we would be getting the tk_core seqcount twice.

Ok, I see. I'm still unsure what this is all good for, but at
least this call appears to be safe.

	Arnd