Xen wallclock on arm and arm64

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

Signed-off-by Stefano Stabellini <[email protected]>

arch/arm/xen/enlighten.c +1 -0

 	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;

Quoted 3 lines · 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 <[email protected]>

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

Quoted 6 lines

---

arch/arm/xen/enlighten.c +5 -0

+static void xen_read_wallclock(struct timespec *ts)
+{
+	u32 version;
+	u64 delta;
+	struct timespec now;

arch/arm/xen/enlighten.c static unsigned long long xen_stolen_accounting(int cpu) · line 106

Quoted 2 lines

arch/arm/xen/enlighten.c +2 -0

+	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.

Quoted 11 lines

arch/arm/xen/enlighten.c +1 -0

 	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)
 

arch/arm/xen/enlighten.c static int __init xen_guest_init(void) · line 325

Quoted 5 lines

arch/arm/xen/enlighten.c +2 -0

 	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.

Quoted 6 lines

Quoted 6 lines

---

arch/arm/xen/enlighten.c +5 -0

+static void xen_read_wallclock(struct timespec *ts)
+{
+   u32 version;
+   u64 delta;
+   struct timespec now;

Quoted 8 lines

arch/arm/xen/enlighten.c static unsigned long long xen_stolen_accounting(int cpu) · line 106

Quoted 2 lines

arch/arm/xen/enlighten.c +2 -0

+   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.

Quoted 15 lines

Quoted 6 lines

Quoted 6 lines

---

arch/arm/xen/enlighten.c +5 -0

+static void xen_read_wallclock(struct timespec *ts)
+{
+   u32 version;
+   u64 delta;
+   struct timespec now;

Quoted 8 lines

arch/arm/xen/enlighten.c static unsigned long long xen_stolen_accounting(int cpu) · line 106

Quoted 2 lines

arch/arm/xen/enlighten.c +2 -0

+   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.

Quoted 3 lines

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).

Quoted 5 lines · Arnd Bergmann

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,

Quoted 11 lines · On Monday 09 November 2015 13:53:30 Stefano Stabellini wrote:

Quoted 5 lines · Arnd Bergmann

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:

Quoted 3 lines

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?

Quoted 12 lines · Arnd Bergmann

Quoted 11 lines · On Monday 09 November 2015 13:53:30 Stefano Stabellini wrote:

Quoted 5 lines · Arnd Bergmann

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:

Quoted 6 lines

Quoted 3 lines

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).

arch/arm/xen/enlighten.c static unsigned long long xen_stolen_accounting(int cpu) · line 118

Quoted 2 lines

arch/arm/xen/enlighten.c +2 -0

+	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.

arch/arm/xen/hypercall.S HYPERCALL2(memory_op); · line 92

Quoted 8 lines

Quoted 1 line

Quoted 1 line · On 05.11.15 at 18:09, <[email protected]> wrote:

arch/arm/xen/hypercall.S +1 -0

@@ -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.

arch/arm/xen/hypercall.S HYPERCALL2(memory_op); · line 92

Quoted 8 lines

Quoted 1 line

Quoted 1 line · On 05.11.15 at 18:09, <[email protected]> wrote:

arch/arm/xen/hypercall.S +1 -0

@@ -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.

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

Quoted 2 lines · Stefano Stabellini

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?

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 <[email protected]>

Signed-off-by Ian Campbell <[email protected]>

---
 arch/arm/xen/enlighten.c |   52 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

arch/arm/xen/enlighten.c +1 -0

index b6aea9c..0176db0 100644
@@ -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);

arch/arm/xen/enlighten.c +7 -0

+	/*
+	 * 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;
+

Quoted 25 lines · 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 <[email protected]>

Signed-off-by Ian Campbell <[email protected]>

---
 arch/arm/xen/enlighten.c |   52 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

arch/arm/xen/enlighten.c +1 -0

index b6aea9c..0176db0 100644
@@ -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);
 }
 

arch/arm/xen/enlighten.c static void xen_read_wallclock(struct timespec *ts) · line 134

Quoted 8 lines

arch/arm/xen/enlighten.c +8 -0

+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

arch/arm/xen/enlighten.c static void xen_read_wallclock(struct timespec *ts) · line 136

Quoted 1 line

arch/arm/xen/enlighten.c +1 -0

+	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().

Quoted 14 lines

arch/arm/xen/enlighten.c +14 -0

+	/*
+	 * 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,

arch/arm/xen/enlighten.c static void xen_read_wallclock(struct timespec *ts) · line 154

Quoted 3 lines

arch/arm/xen/enlighten.c +3 -0

+	       (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.

Quoted 8 lines · Arnd Bergmann

arch/arm/xen/enlighten.c static void xen_read_wallclock(struct timespec *ts) · line 136

Quoted 1 line · On Thursday 05 November 2015 17:09:45 Stefano Stabellini wrote:

arch/arm/xen/enlighten.c +1 -0

+	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);
}

Quoted 14 lines

Quoted 14 lines

arch/arm/xen/enlighten.c +14 -0

+	/*
+	 * 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,

Quoted 8 lines

arch/arm/xen/enlighten.c static void xen_read_wallclock(struct timespec *ts) · line 154

Quoted 3 lines

arch/arm/xen/enlighten.c +3 -0

+	       (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.

Quoted 10 lines · On Monday 09 November 2015 14:10:22 Stefano Stabellini wrote:

Quoted 8 lines · Arnd Bergmann

arch/arm/xen/enlighten.c static void xen_read_wallclock(struct timespec *ts) · line 136

Quoted 1 line · On Thursday 05 November 2015 17:09:45 Stefano Stabellini wrote:

arch/arm/xen/enlighten.c +1 -0

+	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().

Quoted 8 lines

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?

Quoted 29 lines · Arnd Bergmann

Quoted 10 lines · On Monday 09 November 2015 14:10:22 Stefano Stabellini wrote:

Quoted 8 lines · Arnd Bergmann

arch/arm/xen/enlighten.c static void xen_read_wallclock(struct timespec *ts) · line 136

Quoted 1 line · On Thursday 05 November 2015 17:09:45 Stefano Stabellini wrote:

arch/arm/xen/enlighten.c +1 -0

+	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().

Quoted 8 lines

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.

Quoted 3 lines

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?

Quoted 10 lines · Stefano Stabellini

Quoted 9 lines · Arnd Bergmann

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.

Quoted 6 lines

Quoted 3 lines

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.