[PATCH 0/3] x86,time,xen: maintain an accurate persistent clock in more cases

[PATCH 0/3] x86,time,xen: maintain an accurate persistent clock in more cases

[David Vrabel]

The kernel has limited support for updating the persistent clock or
RTC when NTP is synced.  This has the following limitations:

* The persistent clock is not updated on step changes.  This leaves a
  window where it will be incorrect (while NTP resyncs).

* On x86, the virtual platforms have persistent clocks with nanosecond
  precision but this is lost in the x86_platform.set/get_wallclock()
  calls.  Guests may see wallclocks that are out by up to 1 second.

* Xen guests use the Xen wallclock as their persistent clock.  dom0
  maintains this clock so it is persistent for domUs and not dom0
  itself.

These series fixes the above limitations.

Each patch in the series is independent so may be applied to their
respective trees separately.

David

[PATCH 1/3] x86: increase precision of x86_platform.get/set_wallclock()

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

All the virtualized platforms (KVM, lguest and Xen) have persistent
wallclocks that have more than one second of precision.

read_persistent_wallclock() and update_persistent_wallclock() allow
for nanosecond precision but their implementation on x86 with
x86_platform.get/set_wallclock() only allows for one second precision.
This means guests may see a wallclock time that is off by up to 1
second.

Make set_wallclock() and get_wallclock() take a struct timespec
parameter (which allows for nanosecond precision) so KVM and Xen
guests may start with a more accurate wallclock time and a Xen dom0
can maintain a more accurate wallclock for guests.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 arch/x86/include/asm/mc146818rtc.h |    4 ++--
 arch/x86/include/asm/x86_init.h    |    6 ++++--
 arch/x86/kernel/kvmclock.c         |    9 +++------
 arch/x86/kernel/rtc.c              |   17 +++++++----------
 arch/x86/lguest/boot.c             |    4 ++--
 arch/x86/platform/efi/efi.c        |   10 ++++++----
 arch/x86/xen/time.c                |   19 ++++++-------------
 include/linux/efi.h                |    4 ++--
 8 files changed, 32 insertions(+), 41 deletions(-)

diff --git a/arch/x86/include/asm/mc146818rtc.h b/arch/x86/include/asm/mc146818rtc.h
index d354fb7..a55c7ef 100644
--- a/arch/x86/include/asm/mc146818rtc.h
+++ b/arch/x86/include/asm/mc146818rtc.h
@@ -95,8 +95,8 @@ static inline unsigned char current_lock_cmos_reg(void)
 unsigned char rtc_cmos_read(unsigned char addr);
 void rtc_cmos_write(unsigned char val, unsigned char addr);
 
-extern int mach_set_rtc_mmss(unsigned long nowtime);
-extern unsigned long mach_get_cmos_time(void);
+extern int mach_set_rtc_mmss(const struct timespec *now);
+extern void mach_get_cmos_time(struct timespec *now);
 
 #define RTC_IRQ 8
 
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index d8d9922..828a156 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -142,6 +142,8 @@ struct x86_cpuinit_ops {
 	void (*fixup_cpu_id)(struct cpuinfo_x86 *c, int node);
 };
 
+struct timespec;
+
 /**
  * struct x86_platform_ops - platform specific runtime functions
  * @calibrate_tsc:		calibrate TSC
@@ -156,8 +158,8 @@ struct x86_cpuinit_ops {
  */
 struct x86_platform_ops {
 	unsigned long (*calibrate_tsc)(void);
-	unsigned long (*get_wallclock)(void);
-	int (*set_wallclock)(unsigned long nowtime);
+	void (*get_wallclock)(struct timespec *ts);
+	int (*set_wallclock)(const struct timespec *ts);
 	void (*iommu_shutdown)(void);
 	bool (*is_untracked_pat_range)(u64 start, u64 end);
 	void (*nmi_init)(void);
diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index d2c3812..0db81ab 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -48,10 +48,9 @@ static struct pvclock_wall_clock wall_clock;
  * have elapsed since the hypervisor wrote the data. So we try to account for
  * that with system time
  */
-static unsigned long kvm_get_wallclock(void)
+static void kvm_get_wallclock(struct timespec *now)
 {
 	struct pvclock_vcpu_time_info *vcpu_time;
-	struct timespec ts;
 	int low, high;
 	int cpu;
 
@@ -64,14 +63,12 @@ static unsigned long kvm_get_wallclock(void)
 	cpu = smp_processor_id();
 
 	vcpu_time = &hv_clock[cpu].pvti;
-	pvclock_read_wallclock(&wall_clock, vcpu_time, &ts);
+	pvclock_read_wallclock(&wall_clock, vcpu_time, now);
 
 	preempt_enable();
-
-	return ts.tv_sec;
 }
 
-static int kvm_set_wallclock(unsigned long now)
+static int kvm_set_wallclock(const struct timespec *now)
 {
 	return -1;
 }
diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
index 198eb20..0aa2939 100644
--- a/arch/x86/kernel/rtc.c
+++ b/arch/x86/kernel/rtc.c
@@ -38,8 +38,9 @@ EXPORT_SYMBOL(rtc_lock);
  * jump to the next second precisely 500 ms later. Check the Motorola
  * MC146818A or Dallas DS12887 data sheet for details.
  */
-int mach_set_rtc_mmss(unsigned long nowtime)
+int mach_set_rtc_mmss(const struct timespec *now)
 {
+	unsigned long nowtime = now->tv_sec;
 	struct rtc_time tm;
 	int retval = 0;
 
@@ -58,7 +59,7 @@ int mach_set_rtc_mmss(unsigned long nowtime)
 	return retval;
 }
 
-unsigned long mach_get_cmos_time(void)
+void mach_get_cmos_time(struct timespec *now)
 {
 	unsigned int status, year, mon, day, hour, min, sec, century = 0;
 	unsigned long flags;
@@ -107,7 +108,8 @@ unsigned long mach_get_cmos_time(void)
 	} else
 		year += CMOS_YEARS_OFFS;
 
-	return mktime(year, mon, day, hour, min, sec);
+	now->tv_sec = mktime(year, mon, day, hour, min, sec);
+	now->tv_nsec = 0;
 }
 
 /* Routines for accessing the CMOS RAM/RTC. */
@@ -135,18 +137,13 @@ EXPORT_SYMBOL(rtc_cmos_write);
 
 int update_persistent_clock(struct timespec now)
 {
-	return x86_platform.set_wallclock(now.tv_sec);
+	return x86_platform.set_wallclock(&now);
 }
 
 /* not static: needed by APM */
 void read_persistent_clock(struct timespec *ts)
 {
-	unsigned long retval;
-
-	retval = x86_platform.get_wallclock();
-
-	ts->tv_sec = retval;
-	ts->tv_nsec = 0;
+	x86_platform.get_wallclock(ts);
 }
 
 
diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c
index 7114c63..8424d5a 100644
--- a/arch/x86/lguest/boot.c
+++ b/arch/x86/lguest/boot.c
@@ -882,9 +882,9 @@ int lguest_setup_irq(unsigned int irq)
  * It would be far better for everyone if the Guest had its own clock, but
  * until then the Host gives us the time on every interrupt.
  */
-static unsigned long lguest_get_wallclock(void)
+static void lguest_get_wallclock(struct timespec *now)
 {
-	return lguest_data.time.tv_sec;
+	*now = lguest_data.time;
 }
 
 /*
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 55856b2..dd3b825 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -352,8 +352,9 @@ static efi_status_t __init phys_efi_get_time(efi_time_t *tm,
 	return status;
 }
 
-int efi_set_rtc_mmss(unsigned long nowtime)
+int efi_set_rtc_mmss(const struct timespec *now)
 {
+	unsigned long nowtime = now->tv_sec;
 	efi_status_t 	status;
 	efi_time_t 	eft;
 	efi_time_cap_t 	cap;
@@ -388,7 +389,7 @@ int efi_set_rtc_mmss(unsigned long nowtime)
 	return 0;
 }
 
-unsigned long efi_get_time(void)
+void efi_get_time(struct timespec *now)
 {
 	efi_status_t status;
 	efi_time_t eft;
@@ -398,8 +399,9 @@ unsigned long efi_get_time(void)
 	if (status != EFI_SUCCESS)
 		pr_err("Oops: efitime: can't read time!\n");
 
-	return mktime(eft.year, eft.month, eft.day, eft.hour,
-		      eft.minute, eft.second);
+	now->tv_sec = mktime(eft.year, eft.month, eft.day, eft.hour,
+			     eft.minute, eft.second);
+	now->tv_nsec = 0;
 }
 
 /*
diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index 3d88bfd..a1947ac 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -191,32 +191,25 @@ static void xen_read_wallclock(struct timespec *ts)
 	put_cpu_var(xen_vcpu);
 }
 
-static unsigned long xen_get_wallclock(void)
+static void xen_get_wallclock(struct timespec *now)
 {
-	struct timespec ts;
-
-	xen_read_wallclock(&ts);
-	return ts.tv_sec;
+	xen_read_wallclock(now);
 }
 
-static int xen_set_wallclock(unsigned long now)
+static int xen_set_wallclock(const struct timespec *now)
 {
 	struct xen_platform_op op;
-	int rc;
 
 	/* do nothing for domU */
 	if (!xen_initial_domain())
 		return -1;
 
 	op.cmd = XENPF_settime;
-	op.u.settime.secs = now;
-	op.u.settime.nsecs = 0;
+	op.u.settime.secs = now->tv_sec;
+	op.u.settime.nsecs = now->tv_nsec;
 	op.u.settime.system_time = xen_clocksource_read();
 
-	rc = HYPERVISOR_dom0_op(&op);
-	WARN(rc != 0, "XENPF_settime failed: now=%ld\n", now);
-
-	return rc;
+	return HYPERVISOR_dom0_op(&op);
 }
 
 static struct clocksource xen_clocksource __read_mostly = {
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 2bc0ad7..0068bba 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -594,8 +594,8 @@ extern u64 efi_mem_attribute (unsigned long phys_addr, unsigned long size);
 extern int __init efi_uart_console_only (void);
 extern void efi_initialize_iomem_resources(struct resource *code_resource,
 		struct resource *data_resource, struct resource *bss_resource);
-extern unsigned long efi_get_time(void);
-extern int efi_set_rtc_mmss(unsigned long nowtime);
+extern void efi_get_time(struct timespec *now);
+extern int efi_set_rtc_mmss(const struct timespec *now);
 extern void efi_reserve_boot_services(void);
 extern struct efi_memory_map memmap;
 
-- 
1.7.2.5

[PATCH 1/3] x86: increase precision of x86_platform.get/set_wallclock()

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

All the virtualized platforms (KVM, lguest and Xen) have persistent
wallclocks that have more than one second of precision.

read_persistent_wallclock() and update_persistent_wallclock() allow
for nanosecond precision but their implementation on x86 with
x86_platform.get/set_wallclock() only allows for one second precision.
This means guests may see a wallclock time that is off by up to 1
second.

Make set_wallclock() and get_wallclock() take a struct timespec
parameter (which allows for nanosecond precision) so KVM and Xen
guests may start with a more accurate wallclock time and a Xen dom0
can maintain a more accurate wallclock for guests.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 arch/x86/include/asm/mc146818rtc.h |    4 ++--
 arch/x86/include/asm/x86_init.h    |    6 ++++--
 arch/x86/kernel/kvmclock.c         |    9 +++------
 arch/x86/kernel/rtc.c              |   17 +++++++----------
 arch/x86/lguest/boot.c             |    4 ++--
 arch/x86/platform/efi/efi.c        |   10 ++++++----
 arch/x86/xen/time.c                |   19 ++++++-------------
 include/linux/efi.h                |    4 ++--
 8 files changed, 32 insertions(+), 41 deletions(-)

diff --git a/arch/x86/include/asm/mc146818rtc.h b/arch/x86/include/asm/mc146818rtc.h
index d354fb7..a55c7ef 100644
--- a/arch/x86/include/asm/mc146818rtc.h
+++ b/arch/x86/include/asm/mc146818rtc.h
@@ -95,8 +95,8 @@ static inline unsigned char current_lock_cmos_reg(void)
 unsigned char rtc_cmos_read(unsigned char addr);
 void rtc_cmos_write(unsigned char val, unsigned char addr);
 
-extern int mach_set_rtc_mmss(unsigned long nowtime);
-extern unsigned long mach_get_cmos_time(void);
+extern int mach_set_rtc_mmss(const struct timespec *now);
+extern void mach_get_cmos_time(struct timespec *now);
 
 #define RTC_IRQ 8
 
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index d8d9922..828a156 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -142,6 +142,8 @@ struct x86_cpuinit_ops {
 	void (*fixup_cpu_id)(struct cpuinfo_x86 *c, int node);
 };
 
+struct timespec;
+
 /**
  * struct x86_platform_ops - platform specific runtime functions
  * @calibrate_tsc:		calibrate TSC
@@ -156,8 +158,8 @@ struct x86_cpuinit_ops {
  */
 struct x86_platform_ops {
 	unsigned long (*calibrate_tsc)(void);
-	unsigned long (*get_wallclock)(void);
-	int (*set_wallclock)(unsigned long nowtime);
+	void (*get_wallclock)(struct timespec *ts);
+	int (*set_wallclock)(const struct timespec *ts);
 	void (*iommu_shutdown)(void);
 	bool (*is_untracked_pat_range)(u64 start, u64 end);
 	void (*nmi_init)(void);
diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index d2c3812..0db81ab 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -48,10 +48,9 @@ static struct pvclock_wall_clock wall_clock;
  * have elapsed since the hypervisor wrote the data. So we try to account for
  * that with system time
  */
-static unsigned long kvm_get_wallclock(void)
+static void kvm_get_wallclock(struct timespec *now)
 {
 	struct pvclock_vcpu_time_info *vcpu_time;
-	struct timespec ts;
 	int low, high;
 	int cpu;
 
@@ -64,14 +63,12 @@ static unsigned long kvm_get_wallclock(void)
 	cpu = smp_processor_id();
 
 	vcpu_time = &hv_clock[cpu].pvti;
-	pvclock_read_wallclock(&wall_clock, vcpu_time, &ts);
+	pvclock_read_wallclock(&wall_clock, vcpu_time, now);
 
 	preempt_enable();
-
-	return ts.tv_sec;
 }
 
-static int kvm_set_wallclock(unsigned long now)
+static int kvm_set_wallclock(const struct timespec *now)
 {
 	return -1;
 }
diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
index 198eb20..0aa2939 100644
--- a/arch/x86/kernel/rtc.c
+++ b/arch/x86/kernel/rtc.c
@@ -38,8 +38,9 @@ EXPORT_SYMBOL(rtc_lock);
  * jump to the next second precisely 500 ms later. Check the Motorola
  * MC146818A or Dallas DS12887 data sheet for details.
  */
-int mach_set_rtc_mmss(unsigned long nowtime)
+int mach_set_rtc_mmss(const struct timespec *now)
 {
+	unsigned long nowtime = now->tv_sec;
 	struct rtc_time tm;
 	int retval = 0;
 
@@ -58,7 +59,7 @@ int mach_set_rtc_mmss(unsigned long nowtime)
 	return retval;
 }
 
-unsigned long mach_get_cmos_time(void)
+void mach_get_cmos_time(struct timespec *now)
 {
 	unsigned int status, year, mon, day, hour, min, sec, century = 0;
 	unsigned long flags;
@@ -107,7 +108,8 @@ unsigned long mach_get_cmos_time(void)
 	} else
 		year += CMOS_YEARS_OFFS;
 
-	return mktime(year, mon, day, hour, min, sec);
+	now->tv_sec = mktime(year, mon, day, hour, min, sec);
+	now->tv_nsec = 0;
 }
 
 /* Routines for accessing the CMOS RAM/RTC. */
@@ -135,18 +137,13 @@ EXPORT_SYMBOL(rtc_cmos_write);
 
 int update_persistent_clock(struct timespec now)
 {
-	return x86_platform.set_wallclock(now.tv_sec);
+	return x86_platform.set_wallclock(&now);
 }
 
 /* not static: needed by APM */
 void read_persistent_clock(struct timespec *ts)
 {
-	unsigned long retval;
-
-	retval = x86_platform.get_wallclock();
-
-	ts->tv_sec = retval;
-	ts->tv_nsec = 0;
+	x86_platform.get_wallclock(ts);
 }
 
 
diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c
index 7114c63..8424d5a 100644
--- a/arch/x86/lguest/boot.c
+++ b/arch/x86/lguest/boot.c
@@ -882,9 +882,9 @@ int lguest_setup_irq(unsigned int irq)
  * It would be far better for everyone if the Guest had its own clock, but
  * until then the Host gives us the time on every interrupt.
  */
-static unsigned long lguest_get_wallclock(void)
+static void lguest_get_wallclock(struct timespec *now)
 {
-	return lguest_data.time.tv_sec;
+	*now = lguest_data.time;
 }
 
 /*
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 55856b2..dd3b825 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -352,8 +352,9 @@ static efi_status_t __init phys_efi_get_time(efi_time_t *tm,
 	return status;
 }
 
-int efi_set_rtc_mmss(unsigned long nowtime)
+int efi_set_rtc_mmss(const struct timespec *now)
 {
+	unsigned long nowtime = now->tv_sec;
 	efi_status_t 	status;
 	efi_time_t 	eft;
 	efi_time_cap_t 	cap;
@@ -388,7 +389,7 @@ int efi_set_rtc_mmss(unsigned long nowtime)
 	return 0;
 }
 
-unsigned long efi_get_time(void)
+void efi_get_time(struct timespec *now)
 {
 	efi_status_t status;
 	efi_time_t eft;
@@ -398,8 +399,9 @@ unsigned long efi_get_time(void)
 	if (status != EFI_SUCCESS)
 		pr_err("Oops: efitime: can't read time!\n");
 
-	return mktime(eft.year, eft.month, eft.day, eft.hour,
-		      eft.minute, eft.second);
+	now->tv_sec = mktime(eft.year, eft.month, eft.day, eft.hour,
+			     eft.minute, eft.second);
+	now->tv_nsec = 0;
 }
 
 /*
diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index 3d88bfd..a1947ac 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -191,32 +191,25 @@ static void xen_read_wallclock(struct timespec *ts)
 	put_cpu_var(xen_vcpu);
 }
 
-static unsigned long xen_get_wallclock(void)
+static void xen_get_wallclock(struct timespec *now)
 {
-	struct timespec ts;
-
-	xen_read_wallclock(&ts);
-	return ts.tv_sec;
+	xen_read_wallclock(now);
 }
 
-static int xen_set_wallclock(unsigned long now)
+static int xen_set_wallclock(const struct timespec *now)
 {
 	struct xen_platform_op op;
-	int rc;
 
 	/* do nothing for domU */
 	if (!xen_initial_domain())
 		return -1;
 
 	op.cmd = XENPF_settime;
-	op.u.settime.secs = now;
-	op.u.settime.nsecs = 0;
+	op.u.settime.secs = now->tv_sec;
+	op.u.settime.nsecs = now->tv_nsec;
 	op.u.settime.system_time = xen_clocksource_read();
 
-	rc = HYPERVISOR_dom0_op(&op);
-	WARN(rc != 0, "XENPF_settime failed: now=%ld\n", now);
-
-	return rc;
+	return HYPERVISOR_dom0_op(&op);
 }
 
 static struct clocksource xen_clocksource __read_mostly = {
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 2bc0ad7..0068bba 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -594,8 +594,8 @@ extern u64 efi_mem_attribute (unsigned long phys_addr, unsigned long size);
 extern int __init efi_uart_console_only (void);
 extern void efi_initialize_iomem_resources(struct resource *code_resource,
 		struct resource *data_resource, struct resource *bss_resource);
-extern unsigned long efi_get_time(void);
-extern int efi_set_rtc_mmss(unsigned long nowtime);
+extern void efi_get_time(struct timespec *now);
+extern int efi_set_rtc_mmss(const struct timespec *now);
 extern void efi_reserve_boot_services(void);
 extern struct efi_memory_map memmap;
 
-- 
1.7.2.5

Re: [PATCH 1/3] x86: increase precision of x86_platform.get/set_wallclock()

[John Stultz]

On 05/13/2013 10:56 AM, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> All the virtualized platforms (KVM, lguest and Xen) have persistent
> wallclocks that have more than one second of precision.
>
> read_persistent_wallclock() and update_persistent_wallclock() allow
> for nanosecond precision but their implementation on x86 with
> x86_platform.get/set_wallclock() only allows for one second precision.
> This means guests may see a wallclock time that is off by up to 1
> second.
>
> Make set_wallclock() and get_wallclock() take a struct timespec
> parameter (which allows for nanosecond precision) so KVM and Xen
> guests may start with a more accurate wallclock time and a Xen dom0
> can maintain a more accurate wallclock for guests.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

This one looks pretty good.
Lets circle around on the other two and if after that they still don't 
make sense, I'll just queue this one.

Sorry if I'm being thick headed.

thanks
-john

Re: [PATCH 1/3] x86: increase precision of x86_platform.get/set_wallclock()

[John Stultz]

On 05/13/2013 10:56 AM, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> All the virtualized platforms (KVM, lguest and Xen) have persistent
> wallclocks that have more than one second of precision.
>
> read_persistent_wallclock() and update_persistent_wallclock() allow
> for nanosecond precision but their implementation on x86 with
> x86_platform.get/set_wallclock() only allows for one second precision.
> This means guests may see a wallclock time that is off by up to 1
> second.
>
> Make set_wallclock() and get_wallclock() take a struct timespec
> parameter (which allows for nanosecond precision) so KVM and Xen
> guests may start with a more accurate wallclock time and a Xen dom0
> can maintain a more accurate wallclock for guests.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

This one looks pretty good.
Lets circle around on the other two and if after that they still don't 
make sense, I'll just queue this one.

Sorry if I'm being thick headed.

thanks
-john

Re: [PATCH 1/3] x86: increase precision of x86_platform.get/set_wallclock()

[John Stultz]

On 05/13/2013 10:56 AM, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> All the virtualized platforms (KVM, lguest and Xen) have persistent
> wallclocks that have more than one second of precision.
>
> read_persistent_wallclock() and update_persistent_wallclock() allow
> for nanosecond precision but their implementation on x86 with
> x86_platform.get/set_wallclock() only allows for one second precision.
> This means guests may see a wallclock time that is off by up to 1
> second.
>
> Make set_wallclock() and get_wallclock() take a struct timespec
> parameter (which allows for nanosecond precision) so KVM and Xen
> guests may start with a more accurate wallclock time and a Xen dom0
> can maintain a more accurate wallclock for guests.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

I've gone ahead and queued this one for 3.11

thanks
-john

Re: [PATCH 1/3] x86: increase precision of x86_platform.get/set_wallclock()

[John Stultz]

On 05/13/2013 10:56 AM, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> All the virtualized platforms (KVM, lguest and Xen) have persistent
> wallclocks that have more than one second of precision.
>
> read_persistent_wallclock() and update_persistent_wallclock() allow
> for nanosecond precision but their implementation on x86 with
> x86_platform.get/set_wallclock() only allows for one second precision.
> This means guests may see a wallclock time that is off by up to 1
> second.
>
> Make set_wallclock() and get_wallclock() take a struct timespec
> parameter (which allows for nanosecond precision) so KVM and Xen
> guests may start with a more accurate wallclock time and a Xen dom0
> can maintain a more accurate wallclock for guests.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

I've gone ahead and queued this one for 3.11

thanks
-john

Re: [PATCH 1/3] x86: increase precision of x86_platform.get/set_wallclock()

[John Stultz]

On 05/13/2013 10:56 AM, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> All the virtualized platforms (KVM, lguest and Xen) have persistent
> wallclocks that have more than one second of precision.
>
> read_persistent_wallclock() and update_persistent_wallclock() allow
> for nanosecond precision but their implementation on x86 with
> x86_platform.get/set_wallclock() only allows for one second precision.
> This means guests may see a wallclock time that is off by up to 1
> second.
>
> Make set_wallclock() and get_wallclock() take a struct timespec
> parameter (which allows for nanosecond precision) so KVM and Xen
> guests may start with a more accurate wallclock time and a Xen dom0
> can maintain a more accurate wallclock for guests.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>   arch/x86/include/asm/mc146818rtc.h |    4 ++--
>   arch/x86/include/asm/x86_init.h    |    6 ++++--
>   arch/x86/kernel/kvmclock.c         |    9 +++------
>   arch/x86/kernel/rtc.c              |   17 +++++++----------
>   arch/x86/lguest/boot.c             |    4 ++--
>   arch/x86/platform/efi/efi.c        |   10 ++++++----
>   arch/x86/xen/time.c                |   19 ++++++-------------
>   include/linux/efi.h                |    4 ++--

Just FYI, David: You missed the vrtc code with this conversion.

I finally got around to pushing my current queue to my public tree and 
the kbuild robot noticed the build failure (required INTEL_MID config to 
trigger).

The fix I've queued is here:
https://git.linaro.org/gitweb?p=people/jstultz/linux.git;a=commitdiff;h=52d8e9cc6718ae9e6c44b7723028e4717ba91c8b;hp=1dd5234774b33a17743ae62e8b46b5cd0059e1c7

Let me know if you have any objection or comments on this.

thanks
-john

Re: [PATCH 1/3] x86: increase precision of x86_platform.get/set_wallclock()

[John Stultz]

On 05/13/2013 10:56 AM, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> All the virtualized platforms (KVM, lguest and Xen) have persistent
> wallclocks that have more than one second of precision.
>
> read_persistent_wallclock() and update_persistent_wallclock() allow
> for nanosecond precision but their implementation on x86 with
> x86_platform.get/set_wallclock() only allows for one second precision.
> This means guests may see a wallclock time that is off by up to 1
> second.
>
> Make set_wallclock() and get_wallclock() take a struct timespec
> parameter (which allows for nanosecond precision) so KVM and Xen
> guests may start with a more accurate wallclock time and a Xen dom0
> can maintain a more accurate wallclock for guests.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>   arch/x86/include/asm/mc146818rtc.h |    4 ++--
>   arch/x86/include/asm/x86_init.h    |    6 ++++--
>   arch/x86/kernel/kvmclock.c         |    9 +++------
>   arch/x86/kernel/rtc.c              |   17 +++++++----------
>   arch/x86/lguest/boot.c             |    4 ++--
>   arch/x86/platform/efi/efi.c        |   10 ++++++----
>   arch/x86/xen/time.c                |   19 ++++++-------------
>   include/linux/efi.h                |    4 ++--

Just FYI, David: You missed the vrtc code with this conversion.

I finally got around to pushing my current queue to my public tree and 
the kbuild robot noticed the build failure (required INTEL_MID config to 
trigger).

The fix I've queued is here:
https://git.linaro.org/gitweb?p=people/jstultz/linux.git;a=commitdiff;h=52d8e9cc6718ae9e6c44b7723028e4717ba91c8b;hp=1dd5234774b33a17743ae62e8b46b5cd0059e1c7

Let me know if you have any objection or comments on this.

thanks
-john

Re: [PATCH 1/3] x86: increase precision of x86_platform.get/set_wallclock()

[David Vrabel]

On 29/05/13 01:18, John Stultz wrote:
> On 05/13/2013 10:56 AM, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> All the virtualized platforms (KVM, lguest and Xen) have persistent
>> wallclocks that have more than one second of precision.
>>
>> read_persistent_wallclock() and update_persistent_wallclock() allow
>> for nanosecond precision but their implementation on x86 with
>> x86_platform.get/set_wallclock() only allows for one second precision.
>> This means guests may see a wallclock time that is off by up to 1
>> second.
>>
>> Make set_wallclock() and get_wallclock() take a struct timespec
>> parameter (which allows for nanosecond precision) so KVM and Xen
>> guests may start with a more accurate wallclock time and a Xen dom0
>> can maintain a more accurate wallclock for guests.
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> ---
>>   arch/x86/include/asm/mc146818rtc.h |    4 ++--
>>   arch/x86/include/asm/x86_init.h    |    6 ++++--
>>   arch/x86/kernel/kvmclock.c         |    9 +++------
>>   arch/x86/kernel/rtc.c              |   17 +++++++----------
>>   arch/x86/lguest/boot.c             |    4 ++--
>>   arch/x86/platform/efi/efi.c        |   10 ++++++----
>>   arch/x86/xen/time.c                |   19 ++++++-------------
>>   include/linux/efi.h                |    4 ++--
> 
> Just FYI, David: You missed the vrtc code with this conversion.
> 
> I finally got around to pushing my current queue to my public tree and
> the kbuild robot noticed the build failure (required INTEL_MID config to
> trigger).
> 
> The fix I've queued is here:
> https://git.linaro.org/gitweb?p=people/jstultz/linux.git;a=commitdiff;h=52d8e9cc6718ae9e6c44b7723028e4717ba91c8b;hp=1dd5234774b33a17743ae62e8b46b5cd0059e1c7

Oops. Sorry about that.  That fix looks fine.

David

Re: [PATCH 1/3] x86: increase precision of x86_platform.get/set_wallclock()

[David Vrabel]

On 29/05/13 01:18, John Stultz wrote:
> On 05/13/2013 10:56 AM, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> All the virtualized platforms (KVM, lguest and Xen) have persistent
>> wallclocks that have more than one second of precision.
>>
>> read_persistent_wallclock() and update_persistent_wallclock() allow
>> for nanosecond precision but their implementation on x86 with
>> x86_platform.get/set_wallclock() only allows for one second precision.
>> This means guests may see a wallclock time that is off by up to 1
>> second.
>>
>> Make set_wallclock() and get_wallclock() take a struct timespec
>> parameter (which allows for nanosecond precision) so KVM and Xen
>> guests may start with a more accurate wallclock time and a Xen dom0
>> can maintain a more accurate wallclock for guests.
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> ---
>>   arch/x86/include/asm/mc146818rtc.h |    4 ++--
>>   arch/x86/include/asm/x86_init.h    |    6 ++++--
>>   arch/x86/kernel/kvmclock.c         |    9 +++------
>>   arch/x86/kernel/rtc.c              |   17 +++++++----------
>>   arch/x86/lguest/boot.c             |    4 ++--
>>   arch/x86/platform/efi/efi.c        |   10 ++++++----
>>   arch/x86/xen/time.c                |   19 ++++++-------------
>>   include/linux/efi.h                |    4 ++--
> 
> Just FYI, David: You missed the vrtc code with this conversion.
> 
> I finally got around to pushing my current queue to my public tree and
> the kbuild robot noticed the build failure (required INTEL_MID config to
> trigger).
> 
> The fix I've queued is here:
> https://git.linaro.org/gitweb?p=people/jstultz/linux.git;a=commitdiff;h=52d8e9cc6718ae9e6c44b7723028e4717ba91c8b;hp=1dd5234774b33a17743ae62e8b46b5cd0059e1c7

Oops. Sorry about that.  That fix looks fine.

David

[PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

The persistent clock or the RTC is only synchronized with system time
every 11 minutes if NTP is running.  This gives a window where the
persistent clock may be incorrect after a step change in the time
(such as on first boot).

This particularly affects Xen guests as until an update to the control
domain's persistent clock, new guests will start with the incorrect
system time.

When there is a step change in the system time, call
update_persistent_clock or rtc_set_ntp_time() to synchronize the
persistent clock or RTC to the new system time.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 kernel/time/timekeeping.c |   46 +++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 98cd470..56f2349 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -240,12 +240,54 @@ int pvclock_gtod_unregister_notifier(struct notifier_block *nb)
 }
 EXPORT_SYMBOL_GPL(pvclock_gtod_unregister_notifier);
 
+#if defined(CONFIG_GENERIC_CMOS_UPDATE) || defined(CONFIG_RTC_SYSTOHC)
+
+static void __sync_persistent_clock(struct work_struct *work)
+{
+	struct timespec now;
+	int ret = 0;
+
+	getnstimeofday(&now);
+
+#ifdef CONFIG_GENERIC_CMOS_UPDATE
+	ret = update_persistent_clock(now);
+#endif
+#ifdef CONFIG_RTC_SYSTOHC
+	if (ret == -ENODEV)
+		rtc_set_ntp_time(now);
+#endif
+}
+
+static DECLARE_DELAYED_WORK(sync_persistent_clock_work, __sync_persistent_clock);
+
+static void sync_persistent_clock(struct timekeeper *tk)
+{
+	u64 nsecs;
+	u32 remainder;
+
+	/* Many RTCs require updates 500 ms before the next second. */
+	nsecs = timekeeping_get_ns(tk);
+	div_u64_rem(nsecs, NSEC_PER_SEC, &remainder);
+	if (remainder > NSEC_PER_SEC / 2)
+		nsecs = remainder - NSEC_PER_SEC / 2;
+	else
+		nsecs = remainder + NSEC_PER_SEC / 2;
+
+	if (system_wq)
+		schedule_delayed_work(&sync_persistent_clock_work, nsecs_to_jiffies(nsecs));
+}
+
+#endif
+
 /* must hold timekeeper_lock */
-static void timekeeping_update(struct timekeeper *tk, bool clearntp, bool mirror)
+static void timekeeping_update(struct timekeeper *tk, bool step, bool mirror)
 {
-	if (clearntp) {
+	if (step) {
 		tk->ntp_error = 0;
 		ntp_clear();
+#if defined(CONFIG_GENERIC_CMOS_UPDATE) || defined(CONFIG_RTC_SYSTOHC)
+		sync_persistent_clock(tk);
+#endif
 	}
 	update_vsyscall(tk);
 	update_pvclock_gtod(tk);
-- 
1.7.2.5

[PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

The persistent clock or the RTC is only synchronized with system time
every 11 minutes if NTP is running.  This gives a window where the
persistent clock may be incorrect after a step change in the time
(such as on first boot).

This particularly affects Xen guests as until an update to the control
domain's persistent clock, new guests will start with the incorrect
system time.

When there is a step change in the system time, call
update_persistent_clock or rtc_set_ntp_time() to synchronize the
persistent clock or RTC to the new system time.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 kernel/time/timekeeping.c |   46 +++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 98cd470..56f2349 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -240,12 +240,54 @@ int pvclock_gtod_unregister_notifier(struct notifier_block *nb)
 }
 EXPORT_SYMBOL_GPL(pvclock_gtod_unregister_notifier);
 
+#if defined(CONFIG_GENERIC_CMOS_UPDATE) || defined(CONFIG_RTC_SYSTOHC)
+
+static void __sync_persistent_clock(struct work_struct *work)
+{
+	struct timespec now;
+	int ret = 0;
+
+	getnstimeofday(&now);
+
+#ifdef CONFIG_GENERIC_CMOS_UPDATE
+	ret = update_persistent_clock(now);
+#endif
+#ifdef CONFIG_RTC_SYSTOHC
+	if (ret == -ENODEV)
+		rtc_set_ntp_time(now);
+#endif
+}
+
+static DECLARE_DELAYED_WORK(sync_persistent_clock_work, __sync_persistent_clock);
+
+static void sync_persistent_clock(struct timekeeper *tk)
+{
+	u64 nsecs;
+	u32 remainder;
+
+	/* Many RTCs require updates 500 ms before the next second. */
+	nsecs = timekeeping_get_ns(tk);
+	div_u64_rem(nsecs, NSEC_PER_SEC, &remainder);
+	if (remainder > NSEC_PER_SEC / 2)
+		nsecs = remainder - NSEC_PER_SEC / 2;
+	else
+		nsecs = remainder + NSEC_PER_SEC / 2;
+
+	if (system_wq)
+		schedule_delayed_work(&sync_persistent_clock_work, nsecs_to_jiffies(nsecs));
+}
+
+#endif
+
 /* must hold timekeeper_lock */
-static void timekeeping_update(struct timekeeper *tk, bool clearntp, bool mirror)
+static void timekeeping_update(struct timekeeper *tk, bool step, bool mirror)
 {
-	if (clearntp) {
+	if (step) {
 		tk->ntp_error = 0;
 		ntp_clear();
+#if defined(CONFIG_GENERIC_CMOS_UPDATE) || defined(CONFIG_RTC_SYSTOHC)
+		sync_persistent_clock(tk);
+#endif
 	}
 	update_vsyscall(tk);
 	update_pvclock_gtod(tk);
-- 
1.7.2.5

Re: [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[John Stultz]

On 05/13/2013 10:56 AM, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> The persistent clock or the RTC is only synchronized with system time
> every 11 minutes if NTP is running.  This gives a window where the
> persistent clock may be incorrect after a step change in the time
> (such as on first boot).
>
> This particularly affects Xen guests as until an update to the control
> domain's persistent clock, new guests will start with the incorrect
> system time.
>
> When there is a step change in the system time, call
> update_persistent_clock or rtc_set_ntp_time() to synchronize the
> persistent clock or RTC to the new system time.

I'm sorry, this isn't quite making sense to me. Could you further 
describe the exact problematic behavior you're seeing here, and why its 
a problem?

You seem to be saying we should always set the RTC any time settimeofday 
is called (regardless of the NTP sync state), which doesn't seem right 
to me. Also I worry that this would cause the RTC to be set when the RTC 
hctosys() code (or hwclock) sets the time to the RTC clock, which is a 
bit circular.

I suspect once the problem is better understood, there will be a better 
solution then trying to always set the RTC on any settimeofday() call.

thanks
-john



>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>   kernel/time/timekeeping.c |   46 +++++++++++++++++++++++++++++++++++++++++++-
>   1 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 98cd470..56f2349 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -240,12 +240,54 @@ int pvclock_gtod_unregister_notifier(struct notifier_block *nb)
>   }
>   EXPORT_SYMBOL_GPL(pvclock_gtod_unregister_notifier);
>   
> +#if defined(CONFIG_GENERIC_CMOS_UPDATE) || defined(CONFIG_RTC_SYSTOHC)
> +
> +static void __sync_persistent_clock(struct work_struct *work)
> +{
> +	struct timespec now;
> +	int ret = 0;
> +
> +	getnstimeofday(&now);
> +
> +#ifdef CONFIG_GENERIC_CMOS_UPDATE
> +	ret = update_persistent_clock(now);
> +#endif
> +#ifdef CONFIG_RTC_SYSTOHC
> +	if (ret == -ENODEV)
> +		rtc_set_ntp_time(now);
> +#endif
> +}
> +
> +static DECLARE_DELAYED_WORK(sync_persistent_clock_work, __sync_persistent_clock);
> +
> +static void sync_persistent_clock(struct timekeeper *tk)
> +{
> +	u64 nsecs;
> +	u32 remainder;
> +
> +	/* Many RTCs require updates 500 ms before the next second. */
> +	nsecs = timekeeping_get_ns(tk);
> +	div_u64_rem(nsecs, NSEC_PER_SEC, &remainder);
> +	if (remainder > NSEC_PER_SEC / 2)
> +		nsecs = remainder - NSEC_PER_SEC / 2;
> +	else
> +		nsecs = remainder + NSEC_PER_SEC / 2;
> +
> +	if (system_wq)
> +		schedule_delayed_work(&sync_persistent_clock_work, nsecs_to_jiffies(nsecs));
> +}
> +
> +#endif
> +
>   /* must hold timekeeper_lock */
> -static void timekeeping_update(struct timekeeper *tk, bool clearntp, bool mirror)
> +static void timekeeping_update(struct timekeeper *tk, bool step, bool mirror)
>   {
> -	if (clearntp) {
> +	if (step) {
>   		tk->ntp_error = 0;
>   		ntp_clear();
> +#if defined(CONFIG_GENERIC_CMOS_UPDATE) || defined(CONFIG_RTC_SYSTOHC)
> +		sync_persistent_clock(tk);
> +#endif
>   	}
>   	update_vsyscall(tk);
>   	update_pvclock_gtod(tk);

Re: [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[John Stultz]

On 05/13/2013 10:56 AM, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> The persistent clock or the RTC is only synchronized with system time
> every 11 minutes if NTP is running.  This gives a window where the
> persistent clock may be incorrect after a step change in the time
> (such as on first boot).
>
> This particularly affects Xen guests as until an update to the control
> domain's persistent clock, new guests will start with the incorrect
> system time.
>
> When there is a step change in the system time, call
> update_persistent_clock or rtc_set_ntp_time() to synchronize the
> persistent clock or RTC to the new system time.

I'm sorry, this isn't quite making sense to me. Could you further 
describe the exact problematic behavior you're seeing here, and why its 
a problem?

You seem to be saying we should always set the RTC any time settimeofday 
is called (regardless of the NTP sync state), which doesn't seem right 
to me. Also I worry that this would cause the RTC to be set when the RTC 
hctosys() code (or hwclock) sets the time to the RTC clock, which is a 
bit circular.

I suspect once the problem is better understood, there will be a better 
solution then trying to always set the RTC on any settimeofday() call.

thanks
-john



>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>   kernel/time/timekeeping.c |   46 +++++++++++++++++++++++++++++++++++++++++++-
>   1 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 98cd470..56f2349 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -240,12 +240,54 @@ int pvclock_gtod_unregister_notifier(struct notifier_block *nb)
>   }
>   EXPORT_SYMBOL_GPL(pvclock_gtod_unregister_notifier);
>   
> +#if defined(CONFIG_GENERIC_CMOS_UPDATE) || defined(CONFIG_RTC_SYSTOHC)
> +
> +static void __sync_persistent_clock(struct work_struct *work)
> +{
> +	struct timespec now;
> +	int ret = 0;
> +
> +	getnstimeofday(&now);
> +
> +#ifdef CONFIG_GENERIC_CMOS_UPDATE
> +	ret = update_persistent_clock(now);
> +#endif
> +#ifdef CONFIG_RTC_SYSTOHC
> +	if (ret == -ENODEV)
> +		rtc_set_ntp_time(now);
> +#endif
> +}
> +
> +static DECLARE_DELAYED_WORK(sync_persistent_clock_work, __sync_persistent_clock);
> +
> +static void sync_persistent_clock(struct timekeeper *tk)
> +{
> +	u64 nsecs;
> +	u32 remainder;
> +
> +	/* Many RTCs require updates 500 ms before the next second. */
> +	nsecs = timekeeping_get_ns(tk);
> +	div_u64_rem(nsecs, NSEC_PER_SEC, &remainder);
> +	if (remainder > NSEC_PER_SEC / 2)
> +		nsecs = remainder - NSEC_PER_SEC / 2;
> +	else
> +		nsecs = remainder + NSEC_PER_SEC / 2;
> +
> +	if (system_wq)
> +		schedule_delayed_work(&sync_persistent_clock_work, nsecs_to_jiffies(nsecs));
> +}
> +
> +#endif
> +
>   /* must hold timekeeper_lock */
> -static void timekeeping_update(struct timekeeper *tk, bool clearntp, bool mirror)
> +static void timekeeping_update(struct timekeeper *tk, bool step, bool mirror)
>   {
> -	if (clearntp) {
> +	if (step) {
>   		tk->ntp_error = 0;
>   		ntp_clear();
> +#if defined(CONFIG_GENERIC_CMOS_UPDATE) || defined(CONFIG_RTC_SYSTOHC)
> +		sync_persistent_clock(tk);
> +#endif
>   	}
>   	update_vsyscall(tk);
>   	update_pvclock_gtod(tk);

Re: [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[David Vrabel]

On 14/05/13 01:40, John Stultz wrote:
> On 05/13/2013 10:56 AM, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> The persistent clock or the RTC is only synchronized with system time
>> every 11 minutes if NTP is running.  This gives a window where the
>> persistent clock may be incorrect after a step change in the time
>> (such as on first boot).
>>
>> This particularly affects Xen guests as until an update to the control
>> domain's persistent clock, new guests will start with the incorrect
>> system time.
>>
>> When there is a step change in the system time, call
>> update_persistent_clock or rtc_set_ntp_time() to synchronize the
>> persistent clock or RTC to the new system time.
> 
> I'm sorry, this isn't quite making sense to me. Could you further 
> describe the exact problematic behavior you're seeing here, and why its 
> a problem?

The Xen wallclock is used as the persistent clock for Xen guests.  This
is initialized (by Xen) with the CMOS RTC at the start of day.  If the
RTC is incorrect then guests will see an incorrect wallclock time until
dom0 has corrected it.

Currently dom0 only updates the Xen wallclock with the 11 min periodic
work when NTP is synced.  This leaves a window where newly started
guests will see an incorrect wallclock time.  This can cause guests to
fail to start correctly if the wallclock is now behind what it was when
the guest last started. (e.g., fsck of its disk fails as its last mount
time appears to be far into the future).

Similarly (but less problematic), if a bare metal system is rebooted
before the RTC is updated it will still have the incorrect time.

> You seem to be saying we should always set the RTC any time settimeofday 
> is called (regardless of the NTP sync state), which doesn't seem right 
> to me. Also I worry that this would cause the RTC to be set when the RTC 
> hctosys() code (or hwclock) sets the time to the RTC clock, which is a 
> bit circular.

I'm not too concerned about the behaviour of manual syncs of the RTC
because: a) if the kernel does this automatically then the use of manual
syncs is no longer necessary; and b) the RTC will still end up with the
correct time.

> I suspect once the problem is better understood, there will be a better 
> solution then trying to always set the RTC on any settimeofday() call.

David

Re: [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[David Vrabel]

On 14/05/13 01:40, John Stultz wrote:
> On 05/13/2013 10:56 AM, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> The persistent clock or the RTC is only synchronized with system time
>> every 11 minutes if NTP is running.  This gives a window where the
>> persistent clock may be incorrect after a step change in the time
>> (such as on first boot).
>>
>> This particularly affects Xen guests as until an update to the control
>> domain's persistent clock, new guests will start with the incorrect
>> system time.
>>
>> When there is a step change in the system time, call
>> update_persistent_clock or rtc_set_ntp_time() to synchronize the
>> persistent clock or RTC to the new system time.
> 
> I'm sorry, this isn't quite making sense to me. Could you further 
> describe the exact problematic behavior you're seeing here, and why its 
> a problem?

The Xen wallclock is used as the persistent clock for Xen guests.  This
is initialized (by Xen) with the CMOS RTC at the start of day.  If the
RTC is incorrect then guests will see an incorrect wallclock time until
dom0 has corrected it.

Currently dom0 only updates the Xen wallclock with the 11 min periodic
work when NTP is synced.  This leaves a window where newly started
guests will see an incorrect wallclock time.  This can cause guests to
fail to start correctly if the wallclock is now behind what it was when
the guest last started. (e.g., fsck of its disk fails as its last mount
time appears to be far into the future).

Similarly (but less problematic), if a bare metal system is rebooted
before the RTC is updated it will still have the incorrect time.

> You seem to be saying we should always set the RTC any time settimeofday 
> is called (regardless of the NTP sync state), which doesn't seem right 
> to me. Also I worry that this would cause the RTC to be set when the RTC 
> hctosys() code (or hwclock) sets the time to the RTC clock, which is a 
> bit circular.

I'm not too concerned about the behaviour of manual syncs of the RTC
because: a) if the kernel does this automatically then the use of manual
syncs is no longer necessary; and b) the RTC will still end up with the
correct time.

> I suspect once the problem is better understood, there will be a better 
> solution then trying to always set the RTC on any settimeofday() call.

David

Re: [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[John Stultz]

On 05/14/2013 02:47 AM, David Vrabel wrote:
> On 14/05/13 01:40, John Stultz wrote:
>> On 05/13/2013 10:56 AM, David Vrabel wrote:
>>> From: David Vrabel <david.vrabel@citrix.com>
>>>
>>> The persistent clock or the RTC is only synchronized with system time
>>> every 11 minutes if NTP is running.  This gives a window where the
>>> persistent clock may be incorrect after a step change in the time
>>> (such as on first boot).
>>>
>>> This particularly affects Xen guests as until an update to the control
>>> domain's persistent clock, new guests will start with the incorrect
>>> system time.
>>>
>>> When there is a step change in the system time, call
>>> update_persistent_clock or rtc_set_ntp_time() to synchronize the
>>> persistent clock or RTC to the new system time.
>> I'm sorry, this isn't quite making sense to me. Could you further
>> describe the exact problematic behavior you're seeing here, and why its
>> a problem?
> The Xen wallclock is used as the persistent clock for Xen guests.  This
> is initialized (by Xen) with the CMOS RTC at the start of day.

Start of the day? I assume you mean on dom0 bootup? Or is it done 
pre-dom0 bootup by Xen itself?

>    If the
> RTC is incorrect then guests will see an incorrect wallclock time until
> dom0 has corrected it.


Sorry, just a bit more clarifying context here: So there is a 1:1 
relationship between xen_wall_clock and the RTC for all domN guests? And 
even if dom0 has set its system time properly, domN guests will 
initialize (in effect) from the hardware RTC and not from dom0's system 
time?


So, let me see if I'm getting this right:

* Hardware has misconfigured RTC, set to the wrong date/time
* Xen boots up dom0 (or Xen itself) initializes the xen_wall_clock to 
the RTC
* dom0 finishes booting, and uses NTP to correct the system time
* dom1 starts up, uses xen_wall_clock to initialize its system time, but 
the RTC is still wrong, so it boots with the wrong time.
* After 11 minutes of sync w/ NTP, dom0 sets the RTC fixing the time.
* dom2 starts up, uses xen_wall_clock to initialize its system time 
correctly.

At this point, dom0 and dom2 have the correct time and dom1 is 
incorrect, right?

And with your other patches, after the next boot up (assuming dom0 is 
synced with NTP for > 11 minutes) everything will be fine (and if NTP 
doesn't set dom0's RTC, or the hwclock isn't otherwise corrected we'll 
see the same behavior).

Is this correct?


> Currently dom0 only updates the Xen wallclock with the 11 min periodic
> work when NTP is synced.  This leaves a window where newly started
> guests will see an incorrect wallclock time.  This can cause guests to
> fail to start correctly if the wallclock is now behind what it was when
> the guest last started. (e.g., fsck of its disk fails as its last mount
> time appears to be far into the future).
>
> Similarly (but less problematic), if a bare metal system is rebooted
> before the RTC is updated it will still have the incorrect time.

So this has been the existing behavior for quite some time. If the RTC 
is misconfigured, it has to be corrected either explicitly by the admin 
via hwclock or by the kernel but only if we're well synced with NTP.



>> You seem to be saying we should always set the RTC any time settimeofday
>> is called (regardless of the NTP sync state), which doesn't seem right
>> to me. Also I worry that this would cause the RTC to be set when the RTC
>> hctosys() code (or hwclock) sets the time to the RTC clock, which is a
>> bit circular.
> I'm not too concerned about the behaviour of manual syncs of the RTC
> because: a) if the kernel does this automatically then the use of manual
> syncs is no longer necessary;

Well, we can't break existing interface behavior. Even if its 
unnecessary at that point.

> and b) the RTC will still end up with the
> correct time.

But this isn't in fact the case. Imagine an networkless embedded system 
that's system clock drifts. Its setup to use a cron job to set the time 
a few times a day to correct this (its a bit naive, I know, but this is 
how these embedded systems often are configured).

The problem is, that every time it uses hwclock --hctosys, we read the 
RTC, and write it to the system time, which will then write that value 
back to the RTC. Since there will be some delay between the RTC read and 
the RTC write, we will inject some slight error into the RTC, such that 
it may be a second behind where it ought to be.

We do this regularly enough, and now the RTC clock is drifting behind.

And sure, this is somewhat of a contrived an example, but we can't break 
folks using these approaches.


So I think the other patches in this series are fine, and should help 
limit the effect of the problematic case of a  mis-configured RTC. But 
this one I don't think we can do reasonably.

If you really feel this tight-binding of the system time and the RTC is 
necessary (if NTP synced or not), you might continue working the 
approach with following modifications:

1) Limit the RTC syncing to settimeofday() and inject_time_offset(). 
Where it is now, we'd call it on every resume from suspend, which would 
cause the same problematic circular RTC drift on systems that do 
frequent suspend/resumes.

2) Modify the logic so we only set the RTC on settimeofday() if the 
current RTC value is more then N seconds off. This would limit the 
circular drift, allowing time being set by the RTC to not result in 
modifying the RTC.


With those two changes you might have a better chance, but I'm still 
hesitant. There may be use cases where the RTC and the system time are 
intentionally kept out of sync.

thanks
-john

Re: [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[John Stultz]

On 05/14/2013 02:47 AM, David Vrabel wrote:
> On 14/05/13 01:40, John Stultz wrote:
>> On 05/13/2013 10:56 AM, David Vrabel wrote:
>>> From: David Vrabel <david.vrabel@citrix.com>
>>>
>>> The persistent clock or the RTC is only synchronized with system time
>>> every 11 minutes if NTP is running.  This gives a window where the
>>> persistent clock may be incorrect after a step change in the time
>>> (such as on first boot).
>>>
>>> This particularly affects Xen guests as until an update to the control
>>> domain's persistent clock, new guests will start with the incorrect
>>> system time.
>>>
>>> When there is a step change in the system time, call
>>> update_persistent_clock or rtc_set_ntp_time() to synchronize the
>>> persistent clock or RTC to the new system time.
>> I'm sorry, this isn't quite making sense to me. Could you further
>> describe the exact problematic behavior you're seeing here, and why its
>> a problem?
> The Xen wallclock is used as the persistent clock for Xen guests.  This
> is initialized (by Xen) with the CMOS RTC at the start of day.

Start of the day? I assume you mean on dom0 bootup? Or is it done 
pre-dom0 bootup by Xen itself?

>    If the
> RTC is incorrect then guests will see an incorrect wallclock time until
> dom0 has corrected it.


Sorry, just a bit more clarifying context here: So there is a 1:1 
relationship between xen_wall_clock and the RTC for all domN guests? And 
even if dom0 has set its system time properly, domN guests will 
initialize (in effect) from the hardware RTC and not from dom0's system 
time?


So, let me see if I'm getting this right:

* Hardware has misconfigured RTC, set to the wrong date/time
* Xen boots up dom0 (or Xen itself) initializes the xen_wall_clock to 
the RTC
* dom0 finishes booting, and uses NTP to correct the system time
* dom1 starts up, uses xen_wall_clock to initialize its system time, but 
the RTC is still wrong, so it boots with the wrong time.
* After 11 minutes of sync w/ NTP, dom0 sets the RTC fixing the time.
* dom2 starts up, uses xen_wall_clock to initialize its system time 
correctly.

At this point, dom0 and dom2 have the correct time and dom1 is 
incorrect, right?

And with your other patches, after the next boot up (assuming dom0 is 
synced with NTP for > 11 minutes) everything will be fine (and if NTP 
doesn't set dom0's RTC, or the hwclock isn't otherwise corrected we'll 
see the same behavior).

Is this correct?


> Currently dom0 only updates the Xen wallclock with the 11 min periodic
> work when NTP is synced.  This leaves a window where newly started
> guests will see an incorrect wallclock time.  This can cause guests to
> fail to start correctly if the wallclock is now behind what it was when
> the guest last started. (e.g., fsck of its disk fails as its last mount
> time appears to be far into the future).
>
> Similarly (but less problematic), if a bare metal system is rebooted
> before the RTC is updated it will still have the incorrect time.

So this has been the existing behavior for quite some time. If the RTC 
is misconfigured, it has to be corrected either explicitly by the admin 
via hwclock or by the kernel but only if we're well synced with NTP.



>> You seem to be saying we should always set the RTC any time settimeofday
>> is called (regardless of the NTP sync state), which doesn't seem right
>> to me. Also I worry that this would cause the RTC to be set when the RTC
>> hctosys() code (or hwclock) sets the time to the RTC clock, which is a
>> bit circular.
> I'm not too concerned about the behaviour of manual syncs of the RTC
> because: a) if the kernel does this automatically then the use of manual
> syncs is no longer necessary;

Well, we can't break existing interface behavior. Even if its 
unnecessary at that point.

> and b) the RTC will still end up with the
> correct time.

But this isn't in fact the case. Imagine an networkless embedded system 
that's system clock drifts. Its setup to use a cron job to set the time 
a few times a day to correct this (its a bit naive, I know, but this is 
how these embedded systems often are configured).

The problem is, that every time it uses hwclock --hctosys, we read the 
RTC, and write it to the system time, which will then write that value 
back to the RTC. Since there will be some delay between the RTC read and 
the RTC write, we will inject some slight error into the RTC, such that 
it may be a second behind where it ought to be.

We do this regularly enough, and now the RTC clock is drifting behind.

And sure, this is somewhat of a contrived an example, but we can't break 
folks using these approaches.


So I think the other patches in this series are fine, and should help 
limit the effect of the problematic case of a  mis-configured RTC. But 
this one I don't think we can do reasonably.

If you really feel this tight-binding of the system time and the RTC is 
necessary (if NTP synced or not), you might continue working the 
approach with following modifications:

1) Limit the RTC syncing to settimeofday() and inject_time_offset(). 
Where it is now, we'd call it on every resume from suspend, which would 
cause the same problematic circular RTC drift on systems that do 
frequent suspend/resumes.

2) Modify the logic so we only set the RTC on settimeofday() if the 
current RTC value is more then N seconds off. This would limit the 
circular drift, allowing time being set by the RTC to not result in 
modifying the RTC.


With those two changes you might have a better chance, but I'm still 
hesitant. There may be use cases where the RTC and the system time are 
intentionally kept out of sync.

thanks
-john

Re: [Xen-devel] [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[Jan Beulich]

>>> On 14.05.13 at 19:15, John Stultz <john.stultz@linaro.org> wrote:
> On 05/14/2013 02:47 AM, David Vrabel wrote:
>> On 14/05/13 01:40, John Stultz wrote:
>>> I'm sorry, this isn't quite making sense to me. Could you further
>>> describe the exact problematic behavior you're seeing here, and why its
>>> a problem?
>> The Xen wallclock is used as the persistent clock for Xen guests.  This
>> is initialized (by Xen) with the CMOS RTC at the start of day.
> 
> Start of the day? I assume you mean on dom0 bootup? Or is it done 
> pre-dom0 bootup by Xen itself?

It is, indeed - Xen reads the CMOS clock (or consults EFI) once
when it starts up, but leaves those alone as soon as it launched
Dom0.

>>    If the
>> RTC is incorrect then guests will see an incorrect wallclock time until
>> dom0 has corrected it.
> 
> 
> Sorry, just a bit more clarifying context here: So there is a 1:1 
> relationship between xen_wall_clock and the RTC for all domN guests? And 
> even if dom0 has set its system time properly, domN guests will 
> initialize (in effect) from the hardware RTC and not from dom0's system 
> time?

No, (PV) DomU-s will get their time from the software clock that
Xen maintains, which Dom0 is helping keep in good shape when
NTP-synced.

So the hypervisor really doesn't care about the actual RTC getting
updated in hardware, all it needs is for Dom0 to notify it of wall
clock corrections.

Jan

Re: [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[John Stultz]

On 05/15/2013 01:16 AM, Jan Beulich wrote:
>>>> On 14.05.13 at 19:15, John Stultz <john.stultz@linaro.org> wrote:
>> On 05/14/2013 02:47 AM, David Vrabel wrote:
>>> On 14/05/13 01:40, John Stultz wrote:
>>>> I'm sorry, this isn't quite making sense to me. Could you further
>>>> describe the exact problematic behavior you're seeing here, and why its
>>>> a problem?
>>> The Xen wallclock is used as the persistent clock for Xen guests.  This
>>> is initialized (by Xen) with the CMOS RTC at the start of day.
>> Start of the day? I assume you mean on dom0 bootup? Or is it done
>> pre-dom0 bootup by Xen itself?
> It is, indeed - Xen reads the CMOS clock (or consults EFI) once
> when it starts up, but leaves those alone as soon as it launched
> Dom0.
>
>>>     If the
>>> RTC is incorrect then guests will see an incorrect wallclock time until
>>> dom0 has corrected it.
>>
>> Sorry, just a bit more clarifying context here: So there is a 1:1
>> relationship between xen_wall_clock and the RTC for all domN guests? And
>> even if dom0 has set its system time properly, domN guests will
>> initialize (in effect) from the hardware RTC and not from dom0's system
>> time?
> No, (PV) DomU-s will get their time from the software clock that
> Xen maintains, which Dom0 is helping keep in good shape when
> NTP-synced.

Ok, so really, as soon as the Dom0 time is set by NTP, all guests will 
see the right time? That makes more sense, and means the window for 
these sorts of issues is reasonably quite small.

David: So I'm less inclined to merge this individual change, but if you 
still feel strongly about it, let me know and we can circle around on it 
after you've addressed the specific issues I pointed out earlier.

thanks
-john

Re: [Xen-devel] [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[John Stultz]

On 05/15/2013 01:16 AM, Jan Beulich wrote:
>>>> On 14.05.13 at 19:15, John Stultz <john.stultz@linaro.org> wrote:
>> On 05/14/2013 02:47 AM, David Vrabel wrote:
>>> On 14/05/13 01:40, John Stultz wrote:
>>>> I'm sorry, this isn't quite making sense to me. Could you further
>>>> describe the exact problematic behavior you're seeing here, and why its
>>>> a problem?
>>> The Xen wallclock is used as the persistent clock for Xen guests.  This
>>> is initialized (by Xen) with the CMOS RTC at the start of day.
>> Start of the day? I assume you mean on dom0 bootup? Or is it done
>> pre-dom0 bootup by Xen itself?
> It is, indeed - Xen reads the CMOS clock (or consults EFI) once
> when it starts up, but leaves those alone as soon as it launched
> Dom0.
>
>>>     If the
>>> RTC is incorrect then guests will see an incorrect wallclock time until
>>> dom0 has corrected it.
>>
>> Sorry, just a bit more clarifying context here: So there is a 1:1
>> relationship between xen_wall_clock and the RTC for all domN guests? And
>> even if dom0 has set its system time properly, domN guests will
>> initialize (in effect) from the hardware RTC and not from dom0's system
>> time?
> No, (PV) DomU-s will get their time from the software clock that
> Xen maintains, which Dom0 is helping keep in good shape when
> NTP-synced.

Ok, so really, as soon as the Dom0 time is set by NTP, all guests will 
see the right time? That makes more sense, and means the window for 
these sorts of issues is reasonably quite small.

David: So I'm less inclined to merge this individual change, but if you 
still feel strongly about it, let me know and we can circle around on it 
after you've addressed the specific issues I pointed out earlier.

thanks
-john

Re: [Xen-devel] [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[David Vrabel]

On 15/05/13 19:10, John Stultz wrote:
> 
> Ok, so really, as soon as the Dom0 time is set by NTP, all guests will 
> see the right time? That makes more sense, and means the window for 
> these sorts of issues is reasonably quite small.

It's a small window but it's occurring in our automated test system.

> David: So I'm less inclined to merge this individual change, but if you 
> still feel strongly about it, let me know and we can circle around on it 
> after you've addressed the specific issues I pointed out earlier.

This patch was the actual bug fix but I've reworked it to use the
pvclock_gtod notifier chain as this seemed to be what KVM hosts were
using to maintain a clock for guests.  Please review the new series, thanks.

David

Re: [Xen-devel] [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[Konrad Rzeszutek Wilk]

On Tue, May 28, 2013 at 07:26:14PM +0100, David Vrabel wrote:
> On 15/05/13 19:10, John Stultz wrote:
> > 
> > Ok, so really, as soon as the Dom0 time is set by NTP, all guests will 
> > see the right time? That makes more sense, and means the window for 
> > these sorts of issues is reasonably quite small.
> 
> It's a small window but it's occurring in our automated test system.
> 
> > David: So I'm less inclined to merge this individual change, but if you 
> > still feel strongly about it, let me know and we can circle around on it 
> > after you've addressed the specific issues I pointed out earlier.
> 
> This patch was the actual bug fix but I've reworked it to use the
> pvclock_gtod notifier chain as this seemed to be what KVM hosts were
> using to maintain a clock for guests.  Please review the new series, thanks.

Looks good.

John if you are OK I am thinking to push this to Linus shortly as it is
fixing a bug.

> 
> David

Re: [Xen-devel] [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[John Stultz]

On 05/28/2013 11:31 AM, Konrad Rzeszutek Wilk wrote:
> On Tue, May 28, 2013 at 07:26:14PM +0100, David Vrabel wrote:
>> On 15/05/13 19:10, John Stultz wrote:
>>> Ok, so really, as soon as the Dom0 time is set by NTP, all guests will
>>> see the right time? That makes more sense, and means the window for
>>> these sorts of issues is reasonably quite small.
>> It's a small window but it's occurring in our automated test system.
>>
>>> David: So I'm less inclined to merge this individual change, but if you
>>> still feel strongly about it, let me know and we can circle around on it
>>> after you've addressed the specific issues I pointed out earlier.
>> This patch was the actual bug fix but I've reworked it to use the
>> pvclock_gtod notifier chain as this seemed to be what KVM hosts were
>> using to maintain a clock for guests.  Please review the new series, thanks.
> Looks good.
>
> John if you are OK I am thinking to push this to Linus shortly as it is
> fixing a bug.

I'm really not sure I'd call this a bug.  That seems like an 
over-reaction to a misconfigured system.

Or if there is a bug, I'm not sure its been clearly explained.

thanks
-john

Re: [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[Konrad Rzeszutek Wilk]

On Tue, May 28, 2013 at 12:09:14PM -0700, John Stultz wrote:
> On 05/28/2013 11:31 AM, Konrad Rzeszutek Wilk wrote:
> >On Tue, May 28, 2013 at 07:26:14PM +0100, David Vrabel wrote:
> >>On 15/05/13 19:10, John Stultz wrote:
> >>>Ok, so really, as soon as the Dom0 time is set by NTP, all guests will
> >>>see the right time? That makes more sense, and means the window for
> >>>these sorts of issues is reasonably quite small.
> >>It's a small window but it's occurring in our automated test system.
> >>
> >>>David: So I'm less inclined to merge this individual change, but if you
> >>>still feel strongly about it, let me know and we can circle around on it
> >>>after you've addressed the specific issues I pointed out earlier.
> >>This patch was the actual bug fix but I've reworked it to use the
> >>pvclock_gtod notifier chain as this seemed to be what KVM hosts were
> >>using to maintain a clock for guests.  Please review the new series, thanks.
> >Looks good.
> >
> >John if you are OK I am thinking to push this to Linus shortly as it is
> >fixing a bug.
> 
> I'm really not sure I'd call this a bug.  That seems like an
> over-reaction to a misconfigured system.
> 
> Or if there is a bug, I'm not sure its been clearly explained.

The #1 patch - b/c you try to set the RTC time and it actually never takes. Meaning
on the next time the machine is booted the time is again off.
> 
> thanks
> -john
> 
> 
> 

Re: [Xen-devel] [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[Konrad Rzeszutek Wilk]

On Tue, May 28, 2013 at 12:09:14PM -0700, John Stultz wrote:
> On 05/28/2013 11:31 AM, Konrad Rzeszutek Wilk wrote:
> >On Tue, May 28, 2013 at 07:26:14PM +0100, David Vrabel wrote:
> >>On 15/05/13 19:10, John Stultz wrote:
> >>>Ok, so really, as soon as the Dom0 time is set by NTP, all guests will
> >>>see the right time? That makes more sense, and means the window for
> >>>these sorts of issues is reasonably quite small.
> >>It's a small window but it's occurring in our automated test system.
> >>
> >>>David: So I'm less inclined to merge this individual change, but if you
> >>>still feel strongly about it, let me know and we can circle around on it
> >>>after you've addressed the specific issues I pointed out earlier.
> >>This patch was the actual bug fix but I've reworked it to use the
> >>pvclock_gtod notifier chain as this seemed to be what KVM hosts were
> >>using to maintain a clock for guests.  Please review the new series, thanks.
> >Looks good.
> >
> >John if you are OK I am thinking to push this to Linus shortly as it is
> >fixing a bug.
> 
> I'm really not sure I'd call this a bug.  That seems like an
> over-reaction to a misconfigured system.
> 
> Or if there is a bug, I'm not sure its been clearly explained.

The #1 patch - b/c you try to set the RTC time and it actually never takes. Meaning
on the next time the machine is booted the time is again off.
> 
> thanks
> -john
> 
> 
> 

Re: [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[John Stultz]

On 05/28/2013 12:48 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, May 28, 2013 at 12:09:14PM -0700, John Stultz wrote:
>> On 05/28/2013 11:31 AM, Konrad Rzeszutek Wilk wrote:
>>> On Tue, May 28, 2013 at 07:26:14PM +0100, David Vrabel wrote:
>>>> On 15/05/13 19:10, John Stultz wrote:
>>>>> Ok, so really, as soon as the Dom0 time is set by NTP, all guests will
>>>>> see the right time? That makes more sense, and means the window for
>>>>> these sorts of issues is reasonably quite small.
>>>> It's a small window but it's occurring in our automated test system.
>>>>
>>>>> David: So I'm less inclined to merge this individual change, but if you
>>>>> still feel strongly about it, let me know and we can circle around on it
>>>>> after you've addressed the specific issues I pointed out earlier.
>>>> This patch was the actual bug fix but I've reworked it to use the
>>>> pvclock_gtod notifier chain as this seemed to be what KVM hosts were
>>>> using to maintain a clock for guests.  Please review the new series, thanks.
>>> Looks good.
>>>
>>> John if you are OK I am thinking to push this to Linus shortly as it is
>>> fixing a bug.
>> I'm really not sure I'd call this a bug.  That seems like an
>> over-reaction to a misconfigured system.
>>
>> Or if there is a bug, I'm not sure its been clearly explained.
> The #1 patch - b/c you try to set the RTC time and it actually never takes. Meaning
> on the next time the machine is booted the time is again off.

Ok. Yea, that one I'm fine with and have queued for 3.11.

If you want to push it as a bug fix for 3.10, I'll leave it to you to 
push to Linus.

thanks
-john

Re: [Xen-devel] [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[John Stultz]

On 05/28/2013 12:48 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, May 28, 2013 at 12:09:14PM -0700, John Stultz wrote:
>> On 05/28/2013 11:31 AM, Konrad Rzeszutek Wilk wrote:
>>> On Tue, May 28, 2013 at 07:26:14PM +0100, David Vrabel wrote:
>>>> On 15/05/13 19:10, John Stultz wrote:
>>>>> Ok, so really, as soon as the Dom0 time is set by NTP, all guests will
>>>>> see the right time? That makes more sense, and means the window for
>>>>> these sorts of issues is reasonably quite small.
>>>> It's a small window but it's occurring in our automated test system.
>>>>
>>>>> David: So I'm less inclined to merge this individual change, but if you
>>>>> still feel strongly about it, let me know and we can circle around on it
>>>>> after you've addressed the specific issues I pointed out earlier.
>>>> This patch was the actual bug fix but I've reworked it to use the
>>>> pvclock_gtod notifier chain as this seemed to be what KVM hosts were
>>>> using to maintain a clock for guests.  Please review the new series, thanks.
>>> Looks good.
>>>
>>> John if you are OK I am thinking to push this to Linus shortly as it is
>>> fixing a bug.
>> I'm really not sure I'd call this a bug.  That seems like an
>> over-reaction to a misconfigured system.
>>
>> Or if there is a bug, I'm not sure its been clearly explained.
> The #1 patch - b/c you try to set the RTC time and it actually never takes. Meaning
> on the next time the machine is booted the time is again off.

Ok. Yea, that one I'm fine with and have queued for 3.11.

If you want to push it as a bug fix for 3.10, I'll leave it to you to 
push to Linus.

thanks
-john

Re: [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[John Stultz]

On 05/28/2013 01:03 PM, John Stultz wrote:
> On 05/28/2013 12:48 PM, Konrad Rzeszutek Wilk wrote:
>> On Tue, May 28, 2013 at 12:09:14PM -0700, John Stultz wrote:
>>> On 05/28/2013 11:31 AM, Konrad Rzeszutek Wilk wrote:
>>>> On Tue, May 28, 2013 at 07:26:14PM +0100, David Vrabel wrote:
>>>>> On 15/05/13 19:10, John Stultz wrote:
>>>>>> Ok, so really, as soon as the Dom0 time is set by NTP, all guests 
>>>>>> will
>>>>>> see the right time? That makes more sense, and means the window for
>>>>>> these sorts of issues is reasonably quite small.
>>>>> It's a small window but it's occurring in our automated test system.
>>>>>
>>>>>> David: So I'm less inclined to merge this individual change, but 
>>>>>> if you
>>>>>> still feel strongly about it, let me know and we can circle 
>>>>>> around on it
>>>>>> after you've addressed the specific issues I pointed out earlier.
>>>>> This patch was the actual bug fix but I've reworked it to use the
>>>>> pvclock_gtod notifier chain as this seemed to be what KVM hosts were
>>>>> using to maintain a clock for guests.  Please review the new 
>>>>> series, thanks.
>>>> Looks good.
>>>>
>>>> John if you are OK I am thinking to push this to Linus shortly as 
>>>> it is
>>>> fixing a bug.
>>> I'm really not sure I'd call this a bug.  That seems like an
>>> over-reaction to a misconfigured system.
>>>
>>> Or if there is a bug, I'm not sure its been clearly explained.
>> The #1 patch - b/c you try to set the RTC time and it actually never 
>> takes. Meaning
>> on the next time the machine is booted the time is again off.
>
> Ok. Yea, that one I'm fine with and have queued for 3.11.
>
> If you want to push it as a bug fix for 3.10, I'll leave it to you to 
> push to Linus.

Probably obvious, but just in case its not clear:

Though if that one patch goes to linus for 3.10, it needs to be reworked 
to not depend on the mach_set_rtc_mmss() interface change it currently 
depends on. The interface change is not bugfix material.

thanks
-john

Re: [Xen-devel] [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[John Stultz]

On 05/28/2013 01:03 PM, John Stultz wrote:
> On 05/28/2013 12:48 PM, Konrad Rzeszutek Wilk wrote:
>> On Tue, May 28, 2013 at 12:09:14PM -0700, John Stultz wrote:
>>> On 05/28/2013 11:31 AM, Konrad Rzeszutek Wilk wrote:
>>>> On Tue, May 28, 2013 at 07:26:14PM +0100, David Vrabel wrote:
>>>>> On 15/05/13 19:10, John Stultz wrote:
>>>>>> Ok, so really, as soon as the Dom0 time is set by NTP, all guests 
>>>>>> will
>>>>>> see the right time? That makes more sense, and means the window for
>>>>>> these sorts of issues is reasonably quite small.
>>>>> It's a small window but it's occurring in our automated test system.
>>>>>
>>>>>> David: So I'm less inclined to merge this individual change, but 
>>>>>> if you
>>>>>> still feel strongly about it, let me know and we can circle 
>>>>>> around on it
>>>>>> after you've addressed the specific issues I pointed out earlier.
>>>>> This patch was the actual bug fix but I've reworked it to use the
>>>>> pvclock_gtod notifier chain as this seemed to be what KVM hosts were
>>>>> using to maintain a clock for guests.  Please review the new 
>>>>> series, thanks.
>>>> Looks good.
>>>>
>>>> John if you are OK I am thinking to push this to Linus shortly as 
>>>> it is
>>>> fixing a bug.
>>> I'm really not sure I'd call this a bug.  That seems like an
>>> over-reaction to a misconfigured system.
>>>
>>> Or if there is a bug, I'm not sure its been clearly explained.
>> The #1 patch - b/c you try to set the RTC time and it actually never 
>> takes. Meaning
>> on the next time the machine is booted the time is again off.
>
> Ok. Yea, that one I'm fine with and have queued for 3.11.
>
> If you want to push it as a bug fix for 3.10, I'll leave it to you to 
> push to Linus.

Probably obvious, but just in case its not clear:

Though if that one patch goes to linus for 3.10, it needs to be reworked 
to not depend on the mach_set_rtc_mmss() interface change it currently 
depends on. The interface change is not bugfix material.

thanks
-john

Re: [Xen-devel] [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[Konrad Rzeszutek Wilk]

On Tue, May 28, 2013 at 01:11:33PM -0700, John Stultz wrote:
> On 05/28/2013 01:03 PM, John Stultz wrote:
> >On 05/28/2013 12:48 PM, Konrad Rzeszutek Wilk wrote:
> >>On Tue, May 28, 2013 at 12:09:14PM -0700, John Stultz wrote:
> >>>On 05/28/2013 11:31 AM, Konrad Rzeszutek Wilk wrote:
> >>>>On Tue, May 28, 2013 at 07:26:14PM +0100, David Vrabel wrote:
> >>>>>On 15/05/13 19:10, John Stultz wrote:
> >>>>>>Ok, so really, as soon as the Dom0 time is set by NTP,
> >>>>>>all guests will
> >>>>>>see the right time? That makes more sense, and means the window for
> >>>>>>these sorts of issues is reasonably quite small.
> >>>>>It's a small window but it's occurring in our automated test system.
> >>>>>
> >>>>>>David: So I'm less inclined to merge this individual
> >>>>>>change, but if you
> >>>>>>still feel strongly about it, let me know and we can
> >>>>>>circle around on it
> >>>>>>after you've addressed the specific issues I pointed out earlier.
> >>>>>This patch was the actual bug fix but I've reworked it to use the
> >>>>>pvclock_gtod notifier chain as this seemed to be what KVM hosts were
> >>>>>using to maintain a clock for guests.  Please review the
> >>>>>new series, thanks.
> >>>>Looks good.
> >>>>
> >>>>John if you are OK I am thinking to push this to Linus
> >>>>shortly as it is
> >>>>fixing a bug.
> >>>I'm really not sure I'd call this a bug.  That seems like an
> >>>over-reaction to a misconfigured system.
> >>>
> >>>Or if there is a bug, I'm not sure its been clearly explained.
> >>The #1 patch - b/c you try to set the RTC time and it actually
> >>never takes. Meaning
> >>on the next time the machine is booted the time is again off.
> >
> >Ok. Yea, that one I'm fine with and have queued for 3.11.
> >
> >If you want to push it as a bug fix for 3.10, I'll leave it to you
> >to push to Linus.
> 
> Probably obvious, but just in case its not clear:
> 
> Though if that one patch goes to linus for 3.10, it needs to be
> reworked to not depend on the mach_set_rtc_mmss() interface change
> it currently depends on. The interface change is not bugfix
> material.

You are referring to commit 3195ef59cb42cda3aeeb24a7fd2ba1b900c4a3cc
Author: Prarit Bhargava <prarit@redhat.com>
Date:   Thu Feb 14 12:02:54 2013 -0500

    x86: Do full rtc synchronization with ntp

which is not CC-ed to stable@vger.kernel.org, so the #1 patch would
not be able to be back-ported to any kernel right (so v3.9, v3.8..)?

But it could go to v3.10 - as that change is there. But you are saying
it can't go to v3.10 b/c the interface change is not a bugfix
material.  Is that b/c said git commit might cause regressions and
you wouldn't want to do two reverts?

That makes sense -  which point I  you are right that it makes
sense to stick said patch (#1) in your 3.11 queue and skip the v3.10
train for it.

Re: [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[Konrad Rzeszutek Wilk]

On Tue, May 28, 2013 at 01:11:33PM -0700, John Stultz wrote:
> On 05/28/2013 01:03 PM, John Stultz wrote:
> >On 05/28/2013 12:48 PM, Konrad Rzeszutek Wilk wrote:
> >>On Tue, May 28, 2013 at 12:09:14PM -0700, John Stultz wrote:
> >>>On 05/28/2013 11:31 AM, Konrad Rzeszutek Wilk wrote:
> >>>>On Tue, May 28, 2013 at 07:26:14PM +0100, David Vrabel wrote:
> >>>>>On 15/05/13 19:10, John Stultz wrote:
> >>>>>>Ok, so really, as soon as the Dom0 time is set by NTP,
> >>>>>>all guests will
> >>>>>>see the right time? That makes more sense, and means the window for
> >>>>>>these sorts of issues is reasonably quite small.
> >>>>>It's a small window but it's occurring in our automated test system.
> >>>>>
> >>>>>>David: So I'm less inclined to merge this individual
> >>>>>>change, but if you
> >>>>>>still feel strongly about it, let me know and we can
> >>>>>>circle around on it
> >>>>>>after you've addressed the specific issues I pointed out earlier.
> >>>>>This patch was the actual bug fix but I've reworked it to use the
> >>>>>pvclock_gtod notifier chain as this seemed to be what KVM hosts were
> >>>>>using to maintain a clock for guests.  Please review the
> >>>>>new series, thanks.
> >>>>Looks good.
> >>>>
> >>>>John if you are OK I am thinking to push this to Linus
> >>>>shortly as it is
> >>>>fixing a bug.
> >>>I'm really not sure I'd call this a bug.  That seems like an
> >>>over-reaction to a misconfigured system.
> >>>
> >>>Or if there is a bug, I'm not sure its been clearly explained.
> >>The #1 patch - b/c you try to set the RTC time and it actually
> >>never takes. Meaning
> >>on the next time the machine is booted the time is again off.
> >
> >Ok. Yea, that one I'm fine with and have queued for 3.11.
> >
> >If you want to push it as a bug fix for 3.10, I'll leave it to you
> >to push to Linus.
> 
> Probably obvious, but just in case its not clear:
> 
> Though if that one patch goes to linus for 3.10, it needs to be
> reworked to not depend on the mach_set_rtc_mmss() interface change
> it currently depends on. The interface change is not bugfix
> material.

You are referring to commit 3195ef59cb42cda3aeeb24a7fd2ba1b900c4a3cc
Author: Prarit Bhargava <prarit@redhat.com>
Date:   Thu Feb 14 12:02:54 2013 -0500

    x86: Do full rtc synchronization with ntp

which is not CC-ed to stable@vger.kernel.org, so the #1 patch would
not be able to be back-ported to any kernel right (so v3.9, v3.8..)?

But it could go to v3.10 - as that change is there. But you are saying
it can't go to v3.10 b/c the interface change is not a bugfix
material.  Is that b/c said git commit might cause regressions and
you wouldn't want to do two reverts?

That makes sense -  which point I  you are right that it makes
sense to stick said patch (#1) in your 3.11 queue and skip the v3.10
train for it.

Re: [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[John Stultz]

On 05/28/2013 01:25 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, May 28, 2013 at 01:11:33PM -0700, John Stultz wrote:
>> On 05/28/2013 01:03 PM, John Stultz wrote:
>>> On 05/28/2013 12:48 PM, Konrad Rzeszutek Wilk wrote:
>>>> On Tue, May 28, 2013 at 12:09:14PM -0700, John Stultz wrote:
>>>>> On 05/28/2013 11:31 AM, Konrad Rzeszutek Wilk wrote:
>>>>>> On Tue, May 28, 2013 at 07:26:14PM +0100, David Vrabel wrote:
>>>>>>> On 15/05/13 19:10, John Stultz wrote:
>>>>>>>> Ok, so really, as soon as the Dom0 time is set by NTP,
>>>>>>>> all guests will
>>>>>>>> see the right time? That makes more sense, and means the window for
>>>>>>>> these sorts of issues is reasonably quite small.
>>>>>>> It's a small window but it's occurring in our automated test system.
>>>>>>>
>>>>>>>> David: So I'm less inclined to merge this individual
>>>>>>>> change, but if you
>>>>>>>> still feel strongly about it, let me know and we can
>>>>>>>> circle around on it
>>>>>>>> after you've addressed the specific issues I pointed out earlier.
>>>>>>> This patch was the actual bug fix but I've reworked it to use the
>>>>>>> pvclock_gtod notifier chain as this seemed to be what KVM hosts were
>>>>>>> using to maintain a clock for guests.  Please review the
>>>>>>> new series, thanks.
>>>>>> Looks good.
>>>>>>
>>>>>> John if you are OK I am thinking to push this to Linus
>>>>>> shortly as it is
>>>>>> fixing a bug.
>>>>> I'm really not sure I'd call this a bug.  That seems like an
>>>>> over-reaction to a misconfigured system.
>>>>>
>>>>> Or if there is a bug, I'm not sure its been clearly explained.
>>>> The #1 patch - b/c you try to set the RTC time and it actually
>>>> never takes. Meaning
>>>> on the next time the machine is booted the time is again off.
>>> Ok. Yea, that one I'm fine with and have queued for 3.11.
>>>
>>> If you want to push it as a bug fix for 3.10, I'll leave it to you
>>> to push to Linus.
>> Probably obvious, but just in case its not clear:
>>
>> Though if that one patch goes to linus for 3.10, it needs to be
>> reworked to not depend on the mach_set_rtc_mmss() interface change
>> it currently depends on. The interface change is not bugfix
>> material.
> You are referring to commit 3195ef59cb42cda3aeeb24a7fd2ba1b900c4a3cc
> Author: Prarit Bhargava <prarit@redhat.com>
> Date:   Thu Feb 14 12:02:54 2013 -0500
>
>      x86: Do full rtc synchronization with ntp

No. I mean David's patch:
     x86: Increase precision of x86_platform.get/set_wallclock()

That one should be held back for 3.11.

But the calling of mach_set_rtc_mmss() from the Xen set_wallclock hook 
would still be reasonable to go in 3.10 (limited impact to only Xen, 
fixing a bug), but it would have to limit the call to the second 
granular mach_set_rtc_mmss() in 3.10.

thanks
-john

Re: [Xen-devel] [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[John Stultz]

On 05/28/2013 01:25 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, May 28, 2013 at 01:11:33PM -0700, John Stultz wrote:
>> On 05/28/2013 01:03 PM, John Stultz wrote:
>>> On 05/28/2013 12:48 PM, Konrad Rzeszutek Wilk wrote:
>>>> On Tue, May 28, 2013 at 12:09:14PM -0700, John Stultz wrote:
>>>>> On 05/28/2013 11:31 AM, Konrad Rzeszutek Wilk wrote:
>>>>>> On Tue, May 28, 2013 at 07:26:14PM +0100, David Vrabel wrote:
>>>>>>> On 15/05/13 19:10, John Stultz wrote:
>>>>>>>> Ok, so really, as soon as the Dom0 time is set by NTP,
>>>>>>>> all guests will
>>>>>>>> see the right time? That makes more sense, and means the window for
>>>>>>>> these sorts of issues is reasonably quite small.
>>>>>>> It's a small window but it's occurring in our automated test system.
>>>>>>>
>>>>>>>> David: So I'm less inclined to merge this individual
>>>>>>>> change, but if you
>>>>>>>> still feel strongly about it, let me know and we can
>>>>>>>> circle around on it
>>>>>>>> after you've addressed the specific issues I pointed out earlier.
>>>>>>> This patch was the actual bug fix but I've reworked it to use the
>>>>>>> pvclock_gtod notifier chain as this seemed to be what KVM hosts were
>>>>>>> using to maintain a clock for guests.  Please review the
>>>>>>> new series, thanks.
>>>>>> Looks good.
>>>>>>
>>>>>> John if you are OK I am thinking to push this to Linus
>>>>>> shortly as it is
>>>>>> fixing a bug.
>>>>> I'm really not sure I'd call this a bug.  That seems like an
>>>>> over-reaction to a misconfigured system.
>>>>>
>>>>> Or if there is a bug, I'm not sure its been clearly explained.
>>>> The #1 patch - b/c you try to set the RTC time and it actually
>>>> never takes. Meaning
>>>> on the next time the machine is booted the time is again off.
>>> Ok. Yea, that one I'm fine with and have queued for 3.11.
>>>
>>> If you want to push it as a bug fix for 3.10, I'll leave it to you
>>> to push to Linus.
>> Probably obvious, but just in case its not clear:
>>
>> Though if that one patch goes to linus for 3.10, it needs to be
>> reworked to not depend on the mach_set_rtc_mmss() interface change
>> it currently depends on. The interface change is not bugfix
>> material.
> You are referring to commit 3195ef59cb42cda3aeeb24a7fd2ba1b900c4a3cc
> Author: Prarit Bhargava <prarit@redhat.com>
> Date:   Thu Feb 14 12:02:54 2013 -0500
>
>      x86: Do full rtc synchronization with ntp

No. I mean David's patch:
     x86: Increase precision of x86_platform.get/set_wallclock()

That one should be held back for 3.11.

But the calling of mach_set_rtc_mmss() from the Xen set_wallclock hook 
would still be reasonable to go in 3.10 (limited impact to only Xen, 
fixing a bug), but it would have to limit the call to the second 
granular mach_set_rtc_mmss() in 3.10.

thanks
-john

Re: [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[John Stultz]

On 05/28/2013 11:31 AM, Konrad Rzeszutek Wilk wrote:
> On Tue, May 28, 2013 at 07:26:14PM +0100, David Vrabel wrote:
>> On 15/05/13 19:10, John Stultz wrote:
>>> Ok, so really, as soon as the Dom0 time is set by NTP, all guests will
>>> see the right time? That makes more sense, and means the window for
>>> these sorts of issues is reasonably quite small.
>> It's a small window but it's occurring in our automated test system.
>>
>>> David: So I'm less inclined to merge this individual change, but if you
>>> still feel strongly about it, let me know and we can circle around on it
>>> after you've addressed the specific issues I pointed out earlier.
>> This patch was the actual bug fix but I've reworked it to use the
>> pvclock_gtod notifier chain as this seemed to be what KVM hosts were
>> using to maintain a clock for guests.  Please review the new series, thanks.
> Looks good.
>
> John if you are OK I am thinking to push this to Linus shortly as it is
> fixing a bug.

I'm really not sure I'd call this a bug.  That seems like an 
over-reaction to a misconfigured system.

Or if there is a bug, I'm not sure its been clearly explained.

thanks
-john

Re: [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[Konrad Rzeszutek Wilk]

On Tue, May 28, 2013 at 07:26:14PM +0100, David Vrabel wrote:
> On 15/05/13 19:10, John Stultz wrote:
> > 
> > Ok, so really, as soon as the Dom0 time is set by NTP, all guests will 
> > see the right time? That makes more sense, and means the window for 
> > these sorts of issues is reasonably quite small.
> 
> It's a small window but it's occurring in our automated test system.
> 
> > David: So I'm less inclined to merge this individual change, but if you 
> > still feel strongly about it, let me know and we can circle around on it 
> > after you've addressed the specific issues I pointed out earlier.
> 
> This patch was the actual bug fix but I've reworked it to use the
> pvclock_gtod notifier chain as this seemed to be what KVM hosts were
> using to maintain a clock for guests.  Please review the new series, thanks.

Looks good.

John if you are OK I am thinking to push this to Linus shortly as it is
fixing a bug.

> 
> David

Re: [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[John Stultz]

On 05/28/2013 11:26 AM, David Vrabel wrote:
> On 15/05/13 19:10, John Stultz wrote:
>> Ok, so really, as soon as the Dom0 time is set by NTP, all guests will
>> see the right time? That makes more sense, and means the window for
>> these sorts of issues is reasonably quite small.
> It's a small window but it's occurring in our automated test system.

What is it with about your automated test system that causes a 
misconfigured RTC & system such that xen guests are started before Dom0 
has finished being initialized? Or am I still misunderstanding the 
situation?

1) Dom0 boots w/ misconfigured RTC
2) Dom0 starts ntp, correcting the date
3) After 11 minutes of sync the RTC is synced to NTP time.

 From earlier discussions, the only problem would be with guests 
starting between 1 and 2 (which seems a bit early for guests to be 
starting). And after 3, with your patch 1/2 (that I've queued) the issue 
should not recur again, as the RTC is corrected.


>
>> David: So I'm less inclined to merge this individual change, but if you
>> still feel strongly about it, let me know and we can circle around on it
>> after you've addressed the specific issues I pointed out earlier.
> This patch was the actual bug fix but I've reworked it to use the
> pvclock_gtod notifier chain as this seemed to be what KVM hosts were
> using to maintain a clock for guests.  Please review the new series, thanks.

I'm still not convinced that we should be changing the behavior we've 
had for quite a long time.

thanks
-john

Re: [Xen-devel] [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[John Stultz]

On 05/28/2013 11:26 AM, David Vrabel wrote:
> On 15/05/13 19:10, John Stultz wrote:
>> Ok, so really, as soon as the Dom0 time is set by NTP, all guests will
>> see the right time? That makes more sense, and means the window for
>> these sorts of issues is reasonably quite small.
> It's a small window but it's occurring in our automated test system.

What is it with about your automated test system that causes a 
misconfigured RTC & system such that xen guests are started before Dom0 
has finished being initialized? Or am I still misunderstanding the 
situation?

1) Dom0 boots w/ misconfigured RTC
2) Dom0 starts ntp, correcting the date
3) After 11 minutes of sync the RTC is synced to NTP time.

 From earlier discussions, the only problem would be with guests 
starting between 1 and 2 (which seems a bit early for guests to be 
starting). And after 3, with your patch 1/2 (that I've queued) the issue 
should not recur again, as the RTC is corrected.


>
>> David: So I'm less inclined to merge this individual change, but if you
>> still feel strongly about it, let me know and we can circle around on it
>> after you've addressed the specific issues I pointed out earlier.
> This patch was the actual bug fix but I've reworked it to use the
> pvclock_gtod notifier chain as this seemed to be what KVM hosts were
> using to maintain a clock for guests.  Please review the new series, thanks.

I'm still not convinced that we should be changing the behavior we've 
had for quite a long time.

thanks
-john

Re: [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[David Vrabel]

On 15/05/13 19:10, John Stultz wrote:
> 
> Ok, so really, as soon as the Dom0 time is set by NTP, all guests will 
> see the right time? That makes more sense, and means the window for 
> these sorts of issues is reasonably quite small.

It's a small window but it's occurring in our automated test system.

> David: So I'm less inclined to merge this individual change, but if you 
> still feel strongly about it, let me know and we can circle around on it 
> after you've addressed the specific issues I pointed out earlier.

This patch was the actual bug fix but I've reworked it to use the
pvclock_gtod notifier chain as this seemed to be what KVM hosts were
using to maintain a clock for guests.  Please review the new series, thanks.

David

Re: [PATCH 2/3] timekeeping: sync persistent clock and RTC on system time step changes

[Jan Beulich]

>>> On 14.05.13 at 19:15, John Stultz <john.stultz@linaro.org> wrote:
> On 05/14/2013 02:47 AM, David Vrabel wrote:
>> On 14/05/13 01:40, John Stultz wrote:
>>> I'm sorry, this isn't quite making sense to me. Could you further
>>> describe the exact problematic behavior you're seeing here, and why its
>>> a problem?
>> The Xen wallclock is used as the persistent clock for Xen guests.  This
>> is initialized (by Xen) with the CMOS RTC at the start of day.
> 
> Start of the day? I assume you mean on dom0 bootup? Or is it done 
> pre-dom0 bootup by Xen itself?

It is, indeed - Xen reads the CMOS clock (or consults EFI) once
when it starts up, but leaves those alone as soon as it launched
Dom0.

>>    If the
>> RTC is incorrect then guests will see an incorrect wallclock time until
>> dom0 has corrected it.
> 
> 
> Sorry, just a bit more clarifying context here: So there is a 1:1 
> relationship between xen_wall_clock and the RTC for all domN guests? And 
> even if dom0 has set its system time properly, domN guests will 
> initialize (in effect) from the hardware RTC and not from dom0's system 
> time?

No, (PV) DomU-s will get their time from the software clock that
Xen maintains, which Dom0 is helping keep in good shape when
NTP-synced.

So the hypervisor really doesn't care about the actual RTC getting
updated in hardware, all it needs is for Dom0 to notify it of wall
clock corrections.

Jan

[PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

If NTP is used in dom0 and it is synchronized to its clock source,
then the kernel will periodically synchronize the Xen wallclock with
the system time.  Updates to the Xen wallclock do not persist across
reboots, so also synchronize the CMOS RTC (as on bare metal).

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 arch/x86/xen/time.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index a1947ac..4656165 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -14,6 +14,7 @@
 #include <linux/kernel_stat.h>
 #include <linux/math64.h>
 #include <linux/gfp.h>
+#include <linux/mc146818rtc.h>
 
 #include <asm/pvclock.h>
 #include <asm/xen/hypervisor.h>
@@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec *now)
 static int xen_set_wallclock(const struct timespec *now)
 {
 	struct xen_platform_op op;
+	int ret;
 
 	/* do nothing for domU */
 	if (!xen_initial_domain())
 		return -1;
 
+	/* Set the Xen wallclock. */
 	op.cmd = XENPF_settime;
 	op.u.settime.secs = now->tv_sec;
 	op.u.settime.nsecs = now->tv_nsec;
 	op.u.settime.system_time = xen_clocksource_read();
 
-	return HYPERVISOR_dom0_op(&op);
+	ret = HYPERVISOR_dom0_op(&op);
+	if (ret)
+		return ret;
+
+	/* Set the hardware RTC. */
+	return mach_set_rtc_mmss(now);
+
 }
 
 static struct clocksource xen_clocksource __read_mostly = {
-- 
1.7.2.5

[PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[David Vrabel]

From: David Vrabel <david.vrabel@citrix.com>

If NTP is used in dom0 and it is synchronized to its clock source,
then the kernel will periodically synchronize the Xen wallclock with
the system time.  Updates to the Xen wallclock do not persist across
reboots, so also synchronize the CMOS RTC (as on bare metal).

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 arch/x86/xen/time.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index a1947ac..4656165 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -14,6 +14,7 @@
 #include <linux/kernel_stat.h>
 #include <linux/math64.h>
 #include <linux/gfp.h>
+#include <linux/mc146818rtc.h>
 
 #include <asm/pvclock.h>
 #include <asm/xen/hypervisor.h>
@@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec *now)
 static int xen_set_wallclock(const struct timespec *now)
 {
 	struct xen_platform_op op;
+	int ret;
 
 	/* do nothing for domU */
 	if (!xen_initial_domain())
 		return -1;
 
+	/* Set the Xen wallclock. */
 	op.cmd = XENPF_settime;
 	op.u.settime.secs = now->tv_sec;
 	op.u.settime.nsecs = now->tv_nsec;
 	op.u.settime.system_time = xen_clocksource_read();
 
-	return HYPERVISOR_dom0_op(&op);
+	ret = HYPERVISOR_dom0_op(&op);
+	if (ret)
+		return ret;
+
+	/* Set the hardware RTC. */
+	return mach_set_rtc_mmss(now);
+
 }
 
 static struct clocksource xen_clocksource __read_mostly = {
-- 
1.7.2.5

Re: [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[John Stultz]

On 05/13/2013 10:56 AM, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> If NTP is used in dom0 and it is synchronized to its clock source,
> then the kernel will periodically synchronize the Xen wallclock with
> the system time.  Updates to the Xen wallclock do not persist across
> reboots, so also synchronize the CMOS RTC (as on bare metal).

Sorry again, not getting this one either.

So normally in this case we're using the Xen wallclock as the underlying 
source for the persistent_clock here, my understanding is we use this 
instead of the standard cmos, because we get benefits of using the 
hypervisor's sense of time instead of the bare hardware, and allows for 
virtualization of the persistent clock.

But the problem is that even if Dom0 tries to set the xen persistent 
clock, it doesn't actually update anything in the underlying hardware?  
So here you instead try to sync the underlying hardware cmos from the 
same Xen dom0 environment?

Honestly, it seems a little strange to me. If you're running as dom0, 
why does HYPERVISOR_dom0_op() not cause the hypervisor to set the cmos 
its virtualizing? This seems to mess with the proper virtualization 
layering.

I'd appreciate some more details and maybe some feedback from other xen 
folks here to make sure this really makes sense.

thanks
-john


>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>   arch/x86/xen/time.c |   11 ++++++++++-
>   1 files changed, 10 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
> index a1947ac..4656165 100644
> --- a/arch/x86/xen/time.c
> +++ b/arch/x86/xen/time.c
> @@ -14,6 +14,7 @@
>   #include <linux/kernel_stat.h>
>   #include <linux/math64.h>
>   #include <linux/gfp.h>
> +#include <linux/mc146818rtc.h>
>   
>   #include <asm/pvclock.h>
>   #include <asm/xen/hypervisor.h>
> @@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec *now)
>   static int xen_set_wallclock(const struct timespec *now)
>   {
>   	struct xen_platform_op op;
> +	int ret;
>   
>   	/* do nothing for domU */
>   	if (!xen_initial_domain())
>   		return -1;
>   
> +	/* Set the Xen wallclock. */
>   	op.cmd = XENPF_settime;
>   	op.u.settime.secs = now->tv_sec;
>   	op.u.settime.nsecs = now->tv_nsec;
>   	op.u.settime.system_time = xen_clocksource_read();
>   
> -	return HYPERVISOR_dom0_op(&op);
> +	ret = HYPERVISOR_dom0_op(&op);
> +	if (ret)
> +		return ret;
> +
> +	/* Set the hardware RTC. */
> +	return mach_set_rtc_mmss(now);
> +
>   }
>   
>   static struct clocksource xen_clocksource __read_mostly = {

Re: [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[John Stultz]

On 05/13/2013 10:56 AM, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> If NTP is used in dom0 and it is synchronized to its clock source,
> then the kernel will periodically synchronize the Xen wallclock with
> the system time.  Updates to the Xen wallclock do not persist across
> reboots, so also synchronize the CMOS RTC (as on bare metal).

Sorry again, not getting this one either.

So normally in this case we're using the Xen wallclock as the underlying 
source for the persistent_clock here, my understanding is we use this 
instead of the standard cmos, because we get benefits of using the 
hypervisor's sense of time instead of the bare hardware, and allows for 
virtualization of the persistent clock.

But the problem is that even if Dom0 tries to set the xen persistent 
clock, it doesn't actually update anything in the underlying hardware?  
So here you instead try to sync the underlying hardware cmos from the 
same Xen dom0 environment?

Honestly, it seems a little strange to me. If you're running as dom0, 
why does HYPERVISOR_dom0_op() not cause the hypervisor to set the cmos 
its virtualizing? This seems to mess with the proper virtualization 
layering.

I'd appreciate some more details and maybe some feedback from other xen 
folks here to make sure this really makes sense.

thanks
-john


>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>   arch/x86/xen/time.c |   11 ++++++++++-
>   1 files changed, 10 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
> index a1947ac..4656165 100644
> --- a/arch/x86/xen/time.c
> +++ b/arch/x86/xen/time.c
> @@ -14,6 +14,7 @@
>   #include <linux/kernel_stat.h>
>   #include <linux/math64.h>
>   #include <linux/gfp.h>
> +#include <linux/mc146818rtc.h>
>   
>   #include <asm/pvclock.h>
>   #include <asm/xen/hypervisor.h>
> @@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec *now)
>   static int xen_set_wallclock(const struct timespec *now)
>   {
>   	struct xen_platform_op op;
> +	int ret;
>   
>   	/* do nothing for domU */
>   	if (!xen_initial_domain())
>   		return -1;
>   
> +	/* Set the Xen wallclock. */
>   	op.cmd = XENPF_settime;
>   	op.u.settime.secs = now->tv_sec;
>   	op.u.settime.nsecs = now->tv_nsec;
>   	op.u.settime.system_time = xen_clocksource_read();
>   
> -	return HYPERVISOR_dom0_op(&op);
> +	ret = HYPERVISOR_dom0_op(&op);
> +	if (ret)
> +		return ret;
> +
> +	/* Set the hardware RTC. */
> +	return mach_set_rtc_mmss(now);
> +
>   }
>   
>   static struct clocksource xen_clocksource __read_mostly = {

Re: [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[Jan Beulich]

>>> On 14.05.13 at 02:52, John Stultz <john.stultz@linaro.org> wrote:
> Honestly, it seems a little strange to me. If you're running as dom0, 
> why does HYPERVISOR_dom0_op() not cause the hypervisor to set the cmos 
> its virtualizing? This seems to mess with the proper virtualization 
> layering.

Thy hypervisor tries to control as little system and peripheral devices
as possible, and the CMOS (including the clock) is among those not
controlled by it, but by Dom0.

Jan

Re: [Xen-devel] [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[Jan Beulich]

>>> On 14.05.13 at 02:52, John Stultz <john.stultz@linaro.org> wrote:
> Honestly, it seems a little strange to me. If you're running as dom0, 
> why does HYPERVISOR_dom0_op() not cause the hypervisor to set the cmos 
> its virtualizing? This seems to mess with the proper virtualization 
> layering.

Thy hypervisor tries to control as little system and peripheral devices
as possible, and the CMOS (including the clock) is among those not
controlled by it, but by Dom0.

Jan

Re: [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[John Stultz]

On 05/14/2013 12:57 AM, Jan Beulich wrote:
>>>> On 14.05.13 at 02:52, John Stultz <john.stultz@linaro.org> wrote:
>> Honestly, it seems a little strange to me. If you're running as dom0,
>> why does HYPERVISOR_dom0_op() not cause the hypervisor to set the cmos
>> its virtualizing? This seems to mess with the proper virtualization
>> layering.
> Thy hypervisor tries to control as little system and peripheral devices
> as possible, and the CMOS (including the clock) is among those not
> controlled by it, but by Dom0.

Huh. So what does calling HYPERVISOR_dom0_op do then?

Either way, I don't have the context to insightfully review this, so 
I'll want this to get some Acked-bys from additional Xen folks before I 
queue it.

thanks
-john

Re: [Xen-devel] [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[John Stultz]

On 05/14/2013 12:57 AM, Jan Beulich wrote:
>>>> On 14.05.13 at 02:52, John Stultz <john.stultz@linaro.org> wrote:
>> Honestly, it seems a little strange to me. If you're running as dom0,
>> why does HYPERVISOR_dom0_op() not cause the hypervisor to set the cmos
>> its virtualizing? This seems to mess with the proper virtualization
>> layering.
> Thy hypervisor tries to control as little system and peripheral devices
> as possible, and the CMOS (including the clock) is among those not
> controlled by it, but by Dom0.

Huh. So what does calling HYPERVISOR_dom0_op do then?

Either way, I don't have the context to insightfully review this, so 
I'll want this to get some Acked-bys from additional Xen folks before I 
queue it.

thanks
-john

Re: [Xen-devel] [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[Jan Beulich]

>>> On 14.05.13 at 17:59, John Stultz <john.stultz@linaro.org> wrote:
> On 05/14/2013 12:57 AM, Jan Beulich wrote:
>>>>> On 14.05.13 at 02:52, John Stultz <john.stultz@linaro.org> wrote:
>>> Honestly, it seems a little strange to me. If you're running as dom0,
>>> why does HYPERVISOR_dom0_op() not cause the hypervisor to set the cmos
>>> its virtualizing? This seems to mess with the proper virtualization
>>> layering.
>> Thy hypervisor tries to control as little system and peripheral devices
>> as possible, and the CMOS (including the clock) is among those not
>> controlled by it, but by Dom0.
> 
> Huh. So what does calling HYPERVISOR_dom0_op do then?

Here it merely tells the hypervisor that the wall clock changed (so
it can propagate this on to DomU-s).

Jan

Re: [Xen-devel] [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[John Stultz]

On 05/14/2013 09:14 AM, Jan Beulich wrote:
>>>> On 14.05.13 at 17:59, John Stultz <john.stultz@linaro.org> wrote:
>> On 05/14/2013 12:57 AM, Jan Beulich wrote:
>>>>>> On 14.05.13 at 02:52, John Stultz <john.stultz@linaro.org> wrote:
>>>> Honestly, it seems a little strange to me. If you're running as dom0,
>>>> why does HYPERVISOR_dom0_op() not cause the hypervisor to set the cmos
>>>> its virtualizing? This seems to mess with the proper virtualization
>>>> layering.
>>> Thy hypervisor tries to control as little system and peripheral devices
>>> as possible, and the CMOS (including the clock) is among those not
>>> controlled by it, but by Dom0.
>> Huh. So what does calling HYPERVISOR_dom0_op do then?
> Here it merely tells the hypervisor that the wall clock changed (so
> it can propagate this on to DomU-s).

Ok, I appreciate the explanation. Still waiting for acks on this one.

thanks
-john

Re: [Xen-devel] [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[Konrad Rzeszutek Wilk]

John Stultz <john.stultz@linaro.org> wrote:

>On 05/14/2013 09:14 AM, Jan Beulich wrote:
>>>>> On 14.05.13 at 17:59, John Stultz <john.stultz@linaro.org> wrote:
>>> On 05/14/2013 12:57 AM, Jan Beulich wrote:
>>>>>>> On 14.05.13 at 02:52, John Stultz <john.stultz@linaro.org>
>wrote:
>>>>> Honestly, it seems a little strange to me. If you're running as
>dom0,
>>>>> why does HYPERVISOR_dom0_op() not cause the hypervisor to set the
>cmos
>>>>> its virtualizing? This seems to mess with the proper
>virtualization
>>>>> layering.
>>>> Thy hypervisor tries to control as little system and peripheral
>devices
>>>> as possible, and the CMOS (including the clock) is among those not
>>>> controlled by it, but by Dom0.
>>> Huh. So what does calling HYPERVISOR_dom0_op do then?
>> Here it merely tells the hypervisor that the wall clock changed (so
>> it can propagate this on to DomU-s).
>
>Ok, I appreciate the explanation. Still waiting for acks on this one.
>
>thanks
>-john

Acked-by: Konrad Rzeszutek Wilk

Or Reviewed-by. Or John if you would like I can carry this in my branch for Linus.

Thought it might make sense to add the comment from Jan about the CMOS and the follow up explanation.
-- 
Sent from my Android phone. Please excuse my brevity.

Re: [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[John Stultz]

On 05/14/2013 09:24 AM, Konrad Rzeszutek Wilk wrote:
> John Stultz <john.stultz@linaro.org> wrote:
>
>> On 05/14/2013 09:14 AM, Jan Beulich wrote:
>>>>>> On 14.05.13 at 17:59, John Stultz <john.stultz@linaro.org> wrote:
>>>> On 05/14/2013 12:57 AM, Jan Beulich wrote:
>>>>>>>> On 14.05.13 at 02:52, John Stultz <john.stultz@linaro.org>
>> wrote:
>>>>>> Honestly, it seems a little strange to me. If you're running as
>> dom0,
>>>>>> why does HYPERVISOR_dom0_op() not cause the hypervisor to set the
>> cmos
>>>>>> its virtualizing? This seems to mess with the proper
>> virtualization
>>>>>> layering.
>>>>> Thy hypervisor tries to control as little system and peripheral
>> devices
>>>>> as possible, and the CMOS (including the clock) is among those not
>>>>> controlled by it, but by Dom0.
>>>> Huh. So what does calling HYPERVISOR_dom0_op do then?
>>> Here it merely tells the hypervisor that the wall clock changed (so
>>> it can propagate this on to DomU-s).
>> Ok, I appreciate the explanation. Still waiting for acks on this one.
>>
>> thanks
>> -john
> Acked-by: Konrad Rzeszutek Wilk
>
> Or Reviewed-by. Or John if you would like I can carry this in my branch for Linus.
>
> Thought it might make sense to add the comment from Jan about the CMOS and the follow up explanation.
I'm fine queuing it, but yea, the commit will need some more context.

thanks
-john

Re: [Xen-devel] [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[John Stultz]

On 05/14/2013 09:24 AM, Konrad Rzeszutek Wilk wrote:
> John Stultz <john.stultz@linaro.org> wrote:
>
>> On 05/14/2013 09:14 AM, Jan Beulich wrote:
>>>>>> On 14.05.13 at 17:59, John Stultz <john.stultz@linaro.org> wrote:
>>>> On 05/14/2013 12:57 AM, Jan Beulich wrote:
>>>>>>>> On 14.05.13 at 02:52, John Stultz <john.stultz@linaro.org>
>> wrote:
>>>>>> Honestly, it seems a little strange to me. If you're running as
>> dom0,
>>>>>> why does HYPERVISOR_dom0_op() not cause the hypervisor to set the
>> cmos
>>>>>> its virtualizing? This seems to mess with the proper
>> virtualization
>>>>>> layering.
>>>>> Thy hypervisor tries to control as little system and peripheral
>> devices
>>>>> as possible, and the CMOS (including the clock) is among those not
>>>>> controlled by it, but by Dom0.
>>>> Huh. So what does calling HYPERVISOR_dom0_op do then?
>>> Here it merely tells the hypervisor that the wall clock changed (so
>>> it can propagate this on to DomU-s).
>> Ok, I appreciate the explanation. Still waiting for acks on this one.
>>
>> thanks
>> -john
> Acked-by: Konrad Rzeszutek Wilk
>
> Or Reviewed-by. Or John if you would like I can carry this in my branch for Linus.
>
> Thought it might make sense to add the comment from Jan about the CMOS and the follow up explanation.
I'm fine queuing it, but yea, the commit will need some more context.

thanks
-john

Re: [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[Konrad Rzeszutek Wilk]

John Stultz <john.stultz@linaro.org> wrote:

>On 05/14/2013 09:14 AM, Jan Beulich wrote:
>>>>> On 14.05.13 at 17:59, John Stultz <john.stultz@linaro.org> wrote:
>>> On 05/14/2013 12:57 AM, Jan Beulich wrote:
>>>>>>> On 14.05.13 at 02:52, John Stultz <john.stultz@linaro.org>
>wrote:
>>>>> Honestly, it seems a little strange to me. If you're running as
>dom0,
>>>>> why does HYPERVISOR_dom0_op() not cause the hypervisor to set the
>cmos
>>>>> its virtualizing? This seems to mess with the proper
>virtualization
>>>>> layering.
>>>> Thy hypervisor tries to control as little system and peripheral
>devices
>>>> as possible, and the CMOS (including the clock) is among those not
>>>> controlled by it, but by Dom0.
>>> Huh. So what does calling HYPERVISOR_dom0_op do then?
>> Here it merely tells the hypervisor that the wall clock changed (so
>> it can propagate this on to DomU-s).
>
>Ok, I appreciate the explanation. Still waiting for acks on this one.
>
>thanks
>-john

Acked-by: Konrad Rzeszutek Wilk

Or Reviewed-by. Or John if you would like I can carry this in my branch for Linus.

Thought it might make sense to add the comment from Jan about the CMOS and the follow up explanation.
-- 
Sent from my Android phone. Please excuse my brevity.

Re: [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[John Stultz]

On 05/14/2013 09:14 AM, Jan Beulich wrote:
>>>> On 14.05.13 at 17:59, John Stultz <john.stultz@linaro.org> wrote:
>> On 05/14/2013 12:57 AM, Jan Beulich wrote:
>>>>>> On 14.05.13 at 02:52, John Stultz <john.stultz@linaro.org> wrote:
>>>> Honestly, it seems a little strange to me. If you're running as dom0,
>>>> why does HYPERVISOR_dom0_op() not cause the hypervisor to set the cmos
>>>> its virtualizing? This seems to mess with the proper virtualization
>>>> layering.
>>> Thy hypervisor tries to control as little system and peripheral devices
>>> as possible, and the CMOS (including the clock) is among those not
>>> controlled by it, but by Dom0.
>> Huh. So what does calling HYPERVISOR_dom0_op do then?
> Here it merely tells the hypervisor that the wall clock changed (so
> it can propagate this on to DomU-s).

Ok, I appreciate the explanation. Still waiting for acks on this one.

thanks
-john

Re: [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[Jan Beulich]

>>> On 14.05.13 at 17:59, John Stultz <john.stultz@linaro.org> wrote:
> On 05/14/2013 12:57 AM, Jan Beulich wrote:
>>>>> On 14.05.13 at 02:52, John Stultz <john.stultz@linaro.org> wrote:
>>> Honestly, it seems a little strange to me. If you're running as dom0,
>>> why does HYPERVISOR_dom0_op() not cause the hypervisor to set the cmos
>>> its virtualizing? This seems to mess with the proper virtualization
>>> layering.
>> Thy hypervisor tries to control as little system and peripheral devices
>> as possible, and the CMOS (including the clock) is among those not
>> controlled by it, but by Dom0.
> 
> Huh. So what does calling HYPERVISOR_dom0_op do then?

Here it merely tells the hypervisor that the wall clock changed (so
it can propagate this on to DomU-s).

Jan

Re: [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[David Vrabel]

On 14/05/13 01:52, John Stultz wrote:
> On 05/13/2013 10:56 AM, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> If NTP is used in dom0 and it is synchronized to its clock source,
>> then the kernel will periodically synchronize the Xen wallclock with
>> the system time.  Updates to the Xen wallclock do not persist across
>> reboots, so also synchronize the CMOS RTC (as on bare metal).
> 
> Sorry again, not getting this one either.
> 
> So normally in this case we're using the Xen wallclock as the underlying 
> source for the persistent_clock here, my understanding is we use this 
> instead of the standard cmos, because we get benefits of using the 
> hypervisor's sense of time instead of the bare hardware, and allows for 
> virtualization of the persistent clock.
> 
> But the problem is that even if Dom0 tries to set the xen persistent 
> clock, it doesn't actually update anything in the underlying hardware?  
> So here you instead try to sync the underlying hardware cmos from the 
> same Xen dom0 environment?

Yes.

> Honestly, it seems a little strange to me. If you're running as dom0, 
> why does HYPERVISOR_dom0_op() not cause the hypervisor to set the cmos 
> its virtualizing? This seems to mess with the proper virtualization 
> layering.

As Jan says the hypervisor only drives a minimal set of hardware,
everything else is made accessible to dom0 for it to control.

I think this makes sense as it allows us to reuse the existing RTC
drivers etc. in the Linux kernel, instead of having to reimplement them
in the hypervisor.

David

Re: [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[David Vrabel]

On 14/05/13 01:52, John Stultz wrote:
> On 05/13/2013 10:56 AM, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> If NTP is used in dom0 and it is synchronized to its clock source,
>> then the kernel will periodically synchronize the Xen wallclock with
>> the system time.  Updates to the Xen wallclock do not persist across
>> reboots, so also synchronize the CMOS RTC (as on bare metal).
> 
> Sorry again, not getting this one either.
> 
> So normally in this case we're using the Xen wallclock as the underlying 
> source for the persistent_clock here, my understanding is we use this 
> instead of the standard cmos, because we get benefits of using the 
> hypervisor's sense of time instead of the bare hardware, and allows for 
> virtualization of the persistent clock.
> 
> But the problem is that even if Dom0 tries to set the xen persistent 
> clock, it doesn't actually update anything in the underlying hardware?  
> So here you instead try to sync the underlying hardware cmos from the 
> same Xen dom0 environment?

Yes.

> Honestly, it seems a little strange to me. If you're running as dom0, 
> why does HYPERVISOR_dom0_op() not cause the hypervisor to set the cmos 
> its virtualizing? This seems to mess with the proper virtualization 
> layering.

As Jan says the hypervisor only drives a minimal set of hardware,
everything else is made accessible to dom0 for it to control.

I think this makes sense as it allows us to reuse the existing RTC
drivers etc. in the Linux kernel, instead of having to reimplement them
in the hypervisor.

David

Re: [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[John Stultz]

On 05/13/2013 10:56 AM, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> If NTP is used in dom0 and it is synchronized to its clock source,
> then the kernel will periodically synchronize the Xen wallclock with
> the system time.  Updates to the Xen wallclock do not persist across
> reboots, so also synchronize the CMOS RTC (as on bare metal).
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>   arch/x86/xen/time.c |   11 ++++++++++-
>   1 files changed, 10 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
> index a1947ac..4656165 100644
> --- a/arch/x86/xen/time.c
> +++ b/arch/x86/xen/time.c
> @@ -14,6 +14,7 @@
>   #include <linux/kernel_stat.h>
>   #include <linux/math64.h>
>   #include <linux/gfp.h>
> +#include <linux/mc146818rtc.h>
>   
>   #include <asm/pvclock.h>
>   #include <asm/xen/hypervisor.h>
> @@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec *now)
>   static int xen_set_wallclock(const struct timespec *now)
>   {
>   	struct xen_platform_op op;
> +	int ret;
>   
>   	/* do nothing for domU */
>   	if (!xen_initial_domain())
>   		return -1;
>   
> +	/* Set the Xen wallclock. */
>   	op.cmd = XENPF_settime;
>   	op.u.settime.secs = now->tv_sec;
>   	op.u.settime.nsecs = now->tv_nsec;
>   	op.u.settime.system_time = xen_clocksource_read();
>   
> -	return HYPERVISOR_dom0_op(&op);
> +	ret = HYPERVISOR_dom0_op(&op);
> +	if (ret)
> +		return ret;
> +
> +	/* Set the hardware RTC. */
> +	return mach_set_rtc_mmss(now);

Sorry, just noticed one more thing while applying this. Do all Xen 
systems run on hardware that has the conventional CMOS clock?

What happens if the bare-metal needs to use efi_set_rtc() or 
vrtc_set_mmss() ?

thanks
-john

Re: [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[John Stultz]

On 05/13/2013 10:56 AM, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> If NTP is used in dom0 and it is synchronized to its clock source,
> then the kernel will periodically synchronize the Xen wallclock with
> the system time.  Updates to the Xen wallclock do not persist across
> reboots, so also synchronize the CMOS RTC (as on bare metal).
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>   arch/x86/xen/time.c |   11 ++++++++++-
>   1 files changed, 10 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
> index a1947ac..4656165 100644
> --- a/arch/x86/xen/time.c
> +++ b/arch/x86/xen/time.c
> @@ -14,6 +14,7 @@
>   #include <linux/kernel_stat.h>
>   #include <linux/math64.h>
>   #include <linux/gfp.h>
> +#include <linux/mc146818rtc.h>
>   
>   #include <asm/pvclock.h>
>   #include <asm/xen/hypervisor.h>
> @@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec *now)
>   static int xen_set_wallclock(const struct timespec *now)
>   {
>   	struct xen_platform_op op;
> +	int ret;
>   
>   	/* do nothing for domU */
>   	if (!xen_initial_domain())
>   		return -1;
>   
> +	/* Set the Xen wallclock. */
>   	op.cmd = XENPF_settime;
>   	op.u.settime.secs = now->tv_sec;
>   	op.u.settime.nsecs = now->tv_nsec;
>   	op.u.settime.system_time = xen_clocksource_read();
>   
> -	return HYPERVISOR_dom0_op(&op);
> +	ret = HYPERVISOR_dom0_op(&op);
> +	if (ret)
> +		return ret;
> +
> +	/* Set the hardware RTC. */
> +	return mach_set_rtc_mmss(now);

Sorry, just noticed one more thing while applying this. Do all Xen 
systems run on hardware that has the conventional CMOS clock?

What happens if the bare-metal needs to use efi_set_rtc() or 
vrtc_set_mmss() ?

thanks
-john

Re: [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[David Vrabel]

On 14/05/13 18:24, John Stultz wrote:
> On 05/13/2013 10:56 AM, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> If NTP is used in dom0 and it is synchronized to its clock source,
>> then the kernel will periodically synchronize the Xen wallclock with
>> the system time.  Updates to the Xen wallclock do not persist across
>> reboots, so also synchronize the CMOS RTC (as on bare metal).
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> ---
>>   arch/x86/xen/time.c |   11 ++++++++++-
>>   1 files changed, 10 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
>> index a1947ac..4656165 100644
>> --- a/arch/x86/xen/time.c
>> +++ b/arch/x86/xen/time.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/kernel_stat.h>
>>   #include <linux/math64.h>
>>   #include <linux/gfp.h>
>> +#include <linux/mc146818rtc.h>
>>   
>>   #include <asm/pvclock.h>
>>   #include <asm/xen/hypervisor.h>
>> @@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec *now)
>>   static int xen_set_wallclock(const struct timespec *now)
>>   {
>>   	struct xen_platform_op op;
>> +	int ret;
>>   
>>   	/* do nothing for domU */
>>   	if (!xen_initial_domain())
>>   		return -1;
>>   
>> +	/* Set the Xen wallclock. */
>>   	op.cmd = XENPF_settime;
>>   	op.u.settime.secs = now->tv_sec;
>>   	op.u.settime.nsecs = now->tv_nsec;
>>   	op.u.settime.system_time = xen_clocksource_read();
>>   
>> -	return HYPERVISOR_dom0_op(&op);
>> +	ret = HYPERVISOR_dom0_op(&op);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Set the hardware RTC. */
>> +	return mach_set_rtc_mmss(now);
> 
> Sorry, just noticed one more thing while applying this. Do all Xen 
> systems run on hardware that has the conventional CMOS clock?
> 
> What happens if the bare-metal needs to use efi_set_rtc() or 
> vrtc_set_mmss() ?

Ug. I hadn't considered that.  I'll have think some more on this.

David

Re: [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[David Vrabel]

On 14/05/13 18:24, John Stultz wrote:
> On 05/13/2013 10:56 AM, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> If NTP is used in dom0 and it is synchronized to its clock source,
>> then the kernel will periodically synchronize the Xen wallclock with
>> the system time.  Updates to the Xen wallclock do not persist across
>> reboots, so also synchronize the CMOS RTC (as on bare metal).
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> ---
>>   arch/x86/xen/time.c |   11 ++++++++++-
>>   1 files changed, 10 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
>> index a1947ac..4656165 100644
>> --- a/arch/x86/xen/time.c
>> +++ b/arch/x86/xen/time.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/kernel_stat.h>
>>   #include <linux/math64.h>
>>   #include <linux/gfp.h>
>> +#include <linux/mc146818rtc.h>
>>   
>>   #include <asm/pvclock.h>
>>   #include <asm/xen/hypervisor.h>
>> @@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec *now)
>>   static int xen_set_wallclock(const struct timespec *now)
>>   {
>>   	struct xen_platform_op op;
>> +	int ret;
>>   
>>   	/* do nothing for domU */
>>   	if (!xen_initial_domain())
>>   		return -1;
>>   
>> +	/* Set the Xen wallclock. */
>>   	op.cmd = XENPF_settime;
>>   	op.u.settime.secs = now->tv_sec;
>>   	op.u.settime.nsecs = now->tv_nsec;
>>   	op.u.settime.system_time = xen_clocksource_read();
>>   
>> -	return HYPERVISOR_dom0_op(&op);
>> +	ret = HYPERVISOR_dom0_op(&op);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Set the hardware RTC. */
>> +	return mach_set_rtc_mmss(now);
> 
> Sorry, just noticed one more thing while applying this. Do all Xen 
> systems run on hardware that has the conventional CMOS clock?
> 
> What happens if the bare-metal needs to use efi_set_rtc() or 
> vrtc_set_mmss() ?

Ug. I hadn't considered that.  I'll have think some more on this.

David

Re: [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[John Stultz]

On 05/14/2013 11:00 AM, David Vrabel wrote:
> On 14/05/13 18:24, John Stultz wrote:
>> On 05/13/2013 10:56 AM, David Vrabel wrote:
>>> From: David Vrabel <david.vrabel@citrix.com>
>>>
>>> If NTP is used in dom0 and it is synchronized to its clock source,
>>> then the kernel will periodically synchronize the Xen wallclock with
>>> the system time.  Updates to the Xen wallclock do not persist across
>>> reboots, so also synchronize the CMOS RTC (as on bare metal).
>>>
>>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>>> ---
>>>    arch/x86/xen/time.c |   11 ++++++++++-
>>>    1 files changed, 10 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
>>> index a1947ac..4656165 100644
>>> --- a/arch/x86/xen/time.c
>>> +++ b/arch/x86/xen/time.c
>>> @@ -14,6 +14,7 @@
>>>    #include <linux/kernel_stat.h>
>>>    #include <linux/math64.h>
>>>    #include <linux/gfp.h>
>>> +#include <linux/mc146818rtc.h>
>>>    
>>>    #include <asm/pvclock.h>
>>>    #include <asm/xen/hypervisor.h>
>>> @@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec *now)
>>>    static int xen_set_wallclock(const struct timespec *now)
>>>    {
>>>    	struct xen_platform_op op;
>>> +	int ret;
>>>    
>>>    	/* do nothing for domU */
>>>    	if (!xen_initial_domain())
>>>    		return -1;
>>>    
>>> +	/* Set the Xen wallclock. */
>>>    	op.cmd = XENPF_settime;
>>>    	op.u.settime.secs = now->tv_sec;
>>>    	op.u.settime.nsecs = now->tv_nsec;
>>>    	op.u.settime.system_time = xen_clocksource_read();
>>>    
>>> -	return HYPERVISOR_dom0_op(&op);
>>> +	ret = HYPERVISOR_dom0_op(&op);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* Set the hardware RTC. */
>>> +	return mach_set_rtc_mmss(now);
>> Sorry, just noticed one more thing while applying this. Do all Xen
>> systems run on hardware that has the conventional CMOS clock?
>>
>> What happens if the bare-metal needs to use efi_set_rtc() or
>> vrtc_set_mmss() ?
> Ug. I hadn't considered that.  I'll have think some more on this.

Ok. I'll look forward to a future revision then and not queue this one.


Just FYI, here's what I rewrote the commit message to:

     x86/xen: Sync the CMOS RTC as well as the Xen wallclock

     Adjustments to Xen's persistent_clock via update_persistent_clock()
     don't actually persist, as the xen_set_walltime() just notifies
     other domN guests that it has been updated, and does not modify
     the underlying CMOS clock.

     Thus, this patch modifies xen_set_wallclock() so it will set
     the underlying CMOS clock when called from dom0, ensuring the
     persistent_clock will be correct on the next hardware boot.


I feel its a bit more clear, but feel free to tweak it as you see fit.

thanks
-john

Re: [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[John Stultz]

On 05/14/2013 11:00 AM, David Vrabel wrote:
> On 14/05/13 18:24, John Stultz wrote:
>> On 05/13/2013 10:56 AM, David Vrabel wrote:
>>> From: David Vrabel <david.vrabel@citrix.com>
>>>
>>> If NTP is used in dom0 and it is synchronized to its clock source,
>>> then the kernel will periodically synchronize the Xen wallclock with
>>> the system time.  Updates to the Xen wallclock do not persist across
>>> reboots, so also synchronize the CMOS RTC (as on bare metal).
>>>
>>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>>> ---
>>>    arch/x86/xen/time.c |   11 ++++++++++-
>>>    1 files changed, 10 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
>>> index a1947ac..4656165 100644
>>> --- a/arch/x86/xen/time.c
>>> +++ b/arch/x86/xen/time.c
>>> @@ -14,6 +14,7 @@
>>>    #include <linux/kernel_stat.h>
>>>    #include <linux/math64.h>
>>>    #include <linux/gfp.h>
>>> +#include <linux/mc146818rtc.h>
>>>    
>>>    #include <asm/pvclock.h>
>>>    #include <asm/xen/hypervisor.h>
>>> @@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec *now)
>>>    static int xen_set_wallclock(const struct timespec *now)
>>>    {
>>>    	struct xen_platform_op op;
>>> +	int ret;
>>>    
>>>    	/* do nothing for domU */
>>>    	if (!xen_initial_domain())
>>>    		return -1;
>>>    
>>> +	/* Set the Xen wallclock. */
>>>    	op.cmd = XENPF_settime;
>>>    	op.u.settime.secs = now->tv_sec;
>>>    	op.u.settime.nsecs = now->tv_nsec;
>>>    	op.u.settime.system_time = xen_clocksource_read();
>>>    
>>> -	return HYPERVISOR_dom0_op(&op);
>>> +	ret = HYPERVISOR_dom0_op(&op);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* Set the hardware RTC. */
>>> +	return mach_set_rtc_mmss(now);
>> Sorry, just noticed one more thing while applying this. Do all Xen
>> systems run on hardware that has the conventional CMOS clock?
>>
>> What happens if the bare-metal needs to use efi_set_rtc() or
>> vrtc_set_mmss() ?
> Ug. I hadn't considered that.  I'll have think some more on this.

Ok. I'll look forward to a future revision then and not queue this one.


Just FYI, here's what I rewrote the commit message to:

     x86/xen: Sync the CMOS RTC as well as the Xen wallclock

     Adjustments to Xen's persistent_clock via update_persistent_clock()
     don't actually persist, as the xen_set_walltime() just notifies
     other domN guests that it has been updated, and does not modify
     the underlying CMOS clock.

     Thus, this patch modifies xen_set_wallclock() so it will set
     the underlying CMOS clock when called from dom0, ensuring the
     persistent_clock will be correct on the next hardware boot.


I feel its a bit more clear, but feel free to tweak it as you see fit.

thanks
-john

Re: [Xen-devel] [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[Jan Beulich]

>>> On 14.05.13 at 19:24, John Stultz <john.stultz@linaro.org> wrote:
> On 05/13/2013 10:56 AM, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> If NTP is used in dom0 and it is synchronized to its clock source,
>> then the kernel will periodically synchronize the Xen wallclock with
>> the system time.  Updates to the Xen wallclock do not persist across
>> reboots, so also synchronize the CMOS RTC (as on bare metal).
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> ---
>>   arch/x86/xen/time.c |   11 ++++++++++-
>>   1 files changed, 10 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
>> index a1947ac..4656165 100644
>> --- a/arch/x86/xen/time.c
>> +++ b/arch/x86/xen/time.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/kernel_stat.h>
>>   #include <linux/math64.h>
>>   #include <linux/gfp.h>
>> +#include <linux/mc146818rtc.h>
>>   
>>   #include <asm/pvclock.h>
>>   #include <asm/xen/hypervisor.h>
>> @@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec *now)
>>   static int xen_set_wallclock(const struct timespec *now)
>>   {
>>   	struct xen_platform_op op;
>> +	int ret;
>>   
>>   	/* do nothing for domU */
>>   	if (!xen_initial_domain())
>>   		return -1;
>>   
>> +	/* Set the Xen wallclock. */
>>   	op.cmd = XENPF_settime;
>>   	op.u.settime.secs = now->tv_sec;
>>   	op.u.settime.nsecs = now->tv_nsec;
>>   	op.u.settime.system_time = xen_clocksource_read();
>>   
>> -	return HYPERVISOR_dom0_op(&op);
>> +	ret = HYPERVISOR_dom0_op(&op);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Set the hardware RTC. */
>> +	return mach_set_rtc_mmss(now);
> 
> Sorry, just noticed one more thing while applying this. Do all Xen 
> systems run on hardware that has the conventional CMOS clock?
> 
> What happens if the bare-metal needs to use efi_set_rtc() or 
> vrtc_set_mmss() ?

There's no EFI support in the upstream kernel yet when running
on Xen (the code that's in the kernel can't be used, as Dom0
can't directly invoke EFI runtime service functions).

And I don't think Xen is in any way prepared to run on
Moorestown, which afaict is where vrtc_set_mmss() comes into
the picture.

Jan

Re: [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[John Stultz]

On 05/15/2013 01:19 AM, Jan Beulich wrote:
>>>> On 14.05.13 at 19:24, John Stultz <john.stultz@linaro.org> wrote:
>> On 05/13/2013 10:56 AM, David Vrabel wrote:
>>> From: David Vrabel <david.vrabel@citrix.com>
>>>
>>> If NTP is used in dom0 and it is synchronized to its clock source,
>>> then the kernel will periodically synchronize the Xen wallclock with
>>> the system time.  Updates to the Xen wallclock do not persist across
>>> reboots, so also synchronize the CMOS RTC (as on bare metal).
>>>
>>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>>> ---
>>>    arch/x86/xen/time.c |   11 ++++++++++-
>>>    1 files changed, 10 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
>>> index a1947ac..4656165 100644
>>> --- a/arch/x86/xen/time.c
>>> +++ b/arch/x86/xen/time.c
>>> @@ -14,6 +14,7 @@
>>>    #include <linux/kernel_stat.h>
>>>    #include <linux/math64.h>
>>>    #include <linux/gfp.h>
>>> +#include <linux/mc146818rtc.h>
>>>    
>>>    #include <asm/pvclock.h>
>>>    #include <asm/xen/hypervisor.h>
>>> @@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec *now)
>>>    static int xen_set_wallclock(const struct timespec *now)
>>>    {
>>>    	struct xen_platform_op op;
>>> +	int ret;
>>>    
>>>    	/* do nothing for domU */
>>>    	if (!xen_initial_domain())
>>>    		return -1;
>>>    
>>> +	/* Set the Xen wallclock. */
>>>    	op.cmd = XENPF_settime;
>>>    	op.u.settime.secs = now->tv_sec;
>>>    	op.u.settime.nsecs = now->tv_nsec;
>>>    	op.u.settime.system_time = xen_clocksource_read();
>>>    
>>> -	return HYPERVISOR_dom0_op(&op);
>>> +	ret = HYPERVISOR_dom0_op(&op);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* Set the hardware RTC. */
>>> +	return mach_set_rtc_mmss(now);
>> Sorry, just noticed one more thing while applying this. Do all Xen
>> systems run on hardware that has the conventional CMOS clock?
>>
>> What happens if the bare-metal needs to use efi_set_rtc() or
>> vrtc_set_mmss() ?
> There's no EFI support in the upstream kernel yet when running
> on Xen (the code that's in the kernel can't be used, as Dom0
> can't directly invoke EFI runtime service functions).
>
> And I don't think Xen is in any way prepared to run on
> Moorestown, which afaict is where vrtc_set_mmss() comes into
> the picture.

Ok.

David: Would you resubmit this patch, adding this additional context in 
a comment?

thanks
-john

Re: [Xen-devel] [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[John Stultz]

On 05/15/2013 01:19 AM, Jan Beulich wrote:
>>>> On 14.05.13 at 19:24, John Stultz <john.stultz@linaro.org> wrote:
>> On 05/13/2013 10:56 AM, David Vrabel wrote:
>>> From: David Vrabel <david.vrabel@citrix.com>
>>>
>>> If NTP is used in dom0 and it is synchronized to its clock source,
>>> then the kernel will periodically synchronize the Xen wallclock with
>>> the system time.  Updates to the Xen wallclock do not persist across
>>> reboots, so also synchronize the CMOS RTC (as on bare metal).
>>>
>>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>>> ---
>>>    arch/x86/xen/time.c |   11 ++++++++++-
>>>    1 files changed, 10 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
>>> index a1947ac..4656165 100644
>>> --- a/arch/x86/xen/time.c
>>> +++ b/arch/x86/xen/time.c
>>> @@ -14,6 +14,7 @@
>>>    #include <linux/kernel_stat.h>
>>>    #include <linux/math64.h>
>>>    #include <linux/gfp.h>
>>> +#include <linux/mc146818rtc.h>
>>>    
>>>    #include <asm/pvclock.h>
>>>    #include <asm/xen/hypervisor.h>
>>> @@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec *now)
>>>    static int xen_set_wallclock(const struct timespec *now)
>>>    {
>>>    	struct xen_platform_op op;
>>> +	int ret;
>>>    
>>>    	/* do nothing for domU */
>>>    	if (!xen_initial_domain())
>>>    		return -1;
>>>    
>>> +	/* Set the Xen wallclock. */
>>>    	op.cmd = XENPF_settime;
>>>    	op.u.settime.secs = now->tv_sec;
>>>    	op.u.settime.nsecs = now->tv_nsec;
>>>    	op.u.settime.system_time = xen_clocksource_read();
>>>    
>>> -	return HYPERVISOR_dom0_op(&op);
>>> +	ret = HYPERVISOR_dom0_op(&op);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* Set the hardware RTC. */
>>> +	return mach_set_rtc_mmss(now);
>> Sorry, just noticed one more thing while applying this. Do all Xen
>> systems run on hardware that has the conventional CMOS clock?
>>
>> What happens if the bare-metal needs to use efi_set_rtc() or
>> vrtc_set_mmss() ?
> There's no EFI support in the upstream kernel yet when running
> on Xen (the code that's in the kernel can't be used, as Dom0
> can't directly invoke EFI runtime service functions).
>
> And I don't think Xen is in any way prepared to run on
> Moorestown, which afaict is where vrtc_set_mmss() comes into
> the picture.

Ok.

David: Would you resubmit this patch, adding this additional context in 
a comment?

thanks
-john

Re: [PATCH 3/3] x86/xen: sync the CMOS RTC as well as the Xen wallclock

[Jan Beulich]

>>> On 14.05.13 at 19:24, John Stultz <john.stultz@linaro.org> wrote:
> On 05/13/2013 10:56 AM, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> If NTP is used in dom0 and it is synchronized to its clock source,
>> then the kernel will periodically synchronize the Xen wallclock with
>> the system time.  Updates to the Xen wallclock do not persist across
>> reboots, so also synchronize the CMOS RTC (as on bare metal).
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> ---
>>   arch/x86/xen/time.c |   11 ++++++++++-
>>   1 files changed, 10 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
>> index a1947ac..4656165 100644
>> --- a/arch/x86/xen/time.c
>> +++ b/arch/x86/xen/time.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/kernel_stat.h>
>>   #include <linux/math64.h>
>>   #include <linux/gfp.h>
>> +#include <linux/mc146818rtc.h>
>>   
>>   #include <asm/pvclock.h>
>>   #include <asm/xen/hypervisor.h>
>> @@ -199,17 +200,25 @@ static void xen_get_wallclock(struct timespec *now)
>>   static int xen_set_wallclock(const struct timespec *now)
>>   {
>>   	struct xen_platform_op op;
>> +	int ret;
>>   
>>   	/* do nothing for domU */
>>   	if (!xen_initial_domain())
>>   		return -1;
>>   
>> +	/* Set the Xen wallclock. */
>>   	op.cmd = XENPF_settime;
>>   	op.u.settime.secs = now->tv_sec;
>>   	op.u.settime.nsecs = now->tv_nsec;
>>   	op.u.settime.system_time = xen_clocksource_read();
>>   
>> -	return HYPERVISOR_dom0_op(&op);
>> +	ret = HYPERVISOR_dom0_op(&op);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Set the hardware RTC. */
>> +	return mach_set_rtc_mmss(now);
> 
> Sorry, just noticed one more thing while applying this. Do all Xen 
> systems run on hardware that has the conventional CMOS clock?
> 
> What happens if the bare-metal needs to use efi_set_rtc() or 
> vrtc_set_mmss() ?

There's no EFI support in the upstream kernel yet when running
on Xen (the code that's in the kernel can't be used, as Dom0
can't directly invoke EFI runtime service functions).

And I don't think Xen is in any way prepared to run on
Moorestown, which afaict is where vrtc_set_mmss() comes into
the picture.

Jan