[Qemu-devel] [PATCH v2 0/3] Let RTC follow backward jumps of host clock immediately

[Qemu-devel] [PATCH v2 0/3] Let RTC follow backward jumps of host clock immediately

[Jan Kiszka]

Just noticed that this issue is still unfixed because my series was
somehow forgotten. So I've rebased it over current master, refactored it
to use the generic Notifier infrastructure and renamed it to "clock
reset notifier" to avoid confusion with icount related "warping". Please
review / apply before 0.15-rc0, it fixes a relevant issue.

Original series description:

By default, we base the mc146818 RTC on the host clock (CLOCK_REALTIME).
This works fine if only the frequency of the host clock is tuned (e.g.
by NTP) or if it is set to a future time. However, if the host is tuned
backward, e.g. because NTP obtained the correct time after the guest was
already started or the admin decided to tune the local time, we see an
unpleasant effect in the guest: The RTC will stall for the period the
host clock is set back. We identified that one prominent guest affected
by this is Windows which relies on the periodic RTC interrupt for time
keeping.

This series address the issue by detecting those warps and providing a
callback mechanism to device models. The RTC is enabled to update its
timers and register content immediately. Tested successfully both with
hwclock in a Linux guest and by monitoring the Windows clock while
fiddling with the host time.

Note that if this kind of RTC adjustment is not wanted, the user is
still free to decouple the RTC from the host clock and base it on the
VM clock - just like before.

Jan Kiszka (3):
  notifier: Pass data argument to callback
  qemu-timer: Introduce clock reset notifier
  mc146818rtc: Handle host clock resets

 hw/fw_cfg.c      |    2 +-
 hw/mc146818rtc.c |   20 ++++++++++++++++++++
 input.c          |    2 +-
 migration.c      |   12 ++++++------
 notify.c         |    4 ++--
 notify.h         |    4 ++--
 qemu-timer.c     |   29 ++++++++++++++++++++++++++++-
 qemu-timer.h     |    5 +++++
 ui/sdl.c         |    2 +-
 ui/spice-core.c  |    2 +-
 ui/spice-input.c |    4 ++--
 ui/vnc.c         |    4 ++--
 usb-linux.c      |    2 +-
 vl.c             |    4 ++--
 xen-all.c        |    2 +-
 15 files changed, 75 insertions(+), 23 deletions(-)

[Qemu-devel] [PATCH v2 1/3] notifier: Pass data argument to callback

[Jan Kiszka]

This allows to pass additional information to the notifier callback
which is useful if sender and receiver do not share any other distinct
data structure.

Will be used first for the clock reset notifier.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/fw_cfg.c      |    2 +-
 input.c          |    2 +-
 migration.c      |   12 ++++++------
 notify.c         |    4 ++--
 notify.h         |    4 ++--
 ui/sdl.c         |    2 +-
 ui/spice-core.c  |    2 +-
 ui/spice-input.c |    4 ++--
 ui/vnc.c         |    4 ++--
 usb-linux.c      |    2 +-
 vl.c             |    4 ++--
 xen-all.c        |    2 +-
 12 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index 85c8c3c..34e7526 100644
--- a/hw/fw_cfg.c
+++ b/hw/fw_cfg.c
@@ -316,7 +316,7 @@ int fw_cfg_add_file(FWCfgState *s,  const char *filename, uint8_t *data,
     return 1;
 }
 
-static void fw_cfg_machine_ready(struct Notifier* n)
+static void fw_cfg_machine_ready(struct Notifier *n, void *data)
 {
     uint32_t len;
     FWCfgState *s = container_of(n, FWCfgState, machine_ready);
diff --git a/input.c b/input.c
index 5664d3a..894d57f 100644
--- a/input.c
+++ b/input.c
@@ -59,7 +59,7 @@ static void check_mode_change(void)
 
     if (is_absolute != current_is_absolute ||
         has_absolute != current_has_absolute) {
-        notifier_list_notify(&mouse_mode_notifiers);
+        notifier_list_notify(&mouse_mode_notifiers, NULL);
     }
 
     current_is_absolute = is_absolute;
diff --git a/migration.c b/migration.c
index af3a1f2..2a15b98 100644
--- a/migration.c
+++ b/migration.c
@@ -124,7 +124,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
     }
 
     current_migration = s;
-    notifier_list_notify(&migration_state_notifiers);
+    notifier_list_notify(&migration_state_notifiers, NULL);
     return 0;
 }
 
@@ -276,7 +276,7 @@ void migrate_fd_error(FdMigrationState *s)
 {
     DPRINTF("setting error state\n");
     s->state = MIG_STATE_ERROR;
-    notifier_list_notify(&migration_state_notifiers);
+    notifier_list_notify(&migration_state_notifiers, NULL);
     migrate_fd_cleanup(s);
 }
 
@@ -334,7 +334,7 @@ ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size)
             monitor_resume(s->mon);
         }
         s->state = MIG_STATE_ERROR;
-        notifier_list_notify(&migration_state_notifiers);
+        notifier_list_notify(&migration_state_notifiers, NULL);
     }
 
     return ret;
@@ -395,7 +395,7 @@ void migrate_fd_put_ready(void *opaque)
             state = MIG_STATE_ERROR;
         }
         s->state = state;
-        notifier_list_notify(&migration_state_notifiers);
+        notifier_list_notify(&migration_state_notifiers, NULL);
     }
 }
 
@@ -415,7 +415,7 @@ void migrate_fd_cancel(MigrationState *mig_state)
     DPRINTF("cancelling migration\n");
 
     s->state = MIG_STATE_CANCELLED;
-    notifier_list_notify(&migration_state_notifiers);
+    notifier_list_notify(&migration_state_notifiers, NULL);
     qemu_savevm_state_cancel(s->mon, s->file);
 
     migrate_fd_cleanup(s);
@@ -429,7 +429,7 @@ void migrate_fd_release(MigrationState *mig_state)
    
     if (s->state == MIG_STATE_ACTIVE) {
         s->state = MIG_STATE_CANCELLED;
-        notifier_list_notify(&migration_state_notifiers);
+        notifier_list_notify(&migration_state_notifiers, NULL);
         migrate_fd_cleanup(s);
     }
     qemu_free(s);
diff --git a/notify.c b/notify.c
index bcd3fc5..a6bac1f 100644
--- a/notify.c
+++ b/notify.c
@@ -29,11 +29,11 @@ void notifier_list_remove(NotifierList *list, Notifier *notifier)
     QTAILQ_REMOVE(&list->notifiers, notifier, node);
 }
 
-void notifier_list_notify(NotifierList *list)
+void notifier_list_notify(NotifierList *list, void *data)
 {
     Notifier *notifier, *next;
 
     QTAILQ_FOREACH_SAFE(notifier, &list->notifiers, node, next) {
-        notifier->notify(notifier);
+        notifier->notify(notifier, data);
     }
 }
diff --git a/notify.h b/notify.h
index b40522f..54fc57c 100644
--- a/notify.h
+++ b/notify.h
@@ -20,7 +20,7 @@ typedef struct Notifier Notifier;
 
 struct Notifier
 {
-    void (*notify)(Notifier *notifier);
+    void (*notify)(Notifier *notifier, void *data);
     QTAILQ_ENTRY(Notifier) node;
 };
 
@@ -38,6 +38,6 @@ void notifier_list_add(NotifierList *list, Notifier *notifier);
 
 void notifier_list_remove(NotifierList *list, Notifier *notifier);
 
-void notifier_list_notify(NotifierList *list);
+void notifier_list_notify(NotifierList *list, void *data);
 
 #endif
diff --git a/ui/sdl.c b/ui/sdl.c
index f2bd4a0..6dbc5cb 100644
--- a/ui/sdl.c
+++ b/ui/sdl.c
@@ -481,7 +481,7 @@ static void sdl_grab_end(void)
     sdl_update_caption();
 }
 
-static void sdl_mouse_mode_change(Notifier *notify)
+static void sdl_mouse_mode_change(Notifier *notify, void *data)
 {
     if (kbd_mouse_is_absolute()) {
         if (!absolute_enabled) {
diff --git a/ui/spice-core.c b/ui/spice-core.c
index dd9905b..e134f04 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -416,7 +416,7 @@ void do_info_spice(Monitor *mon, QObject **ret_data)
     *ret_data = QOBJECT(server);
 }
 
-static void migration_state_notifier(Notifier *notifier)
+static void migration_state_notifier(Notifier *notifier, void *data)
 {
     int state = get_migration_state();
 
diff --git a/ui/spice-input.c b/ui/spice-input.c
index 37c8578..75abf5f 100644
--- a/ui/spice-input.c
+++ b/ui/spice-input.c
@@ -178,7 +178,7 @@ static const SpiceTabletInterface tablet_interface = {
     .buttons            = tablet_buttons,
 };
 
-static void mouse_mode_notifier(Notifier *notifier)
+static void mouse_mode_notifier(Notifier *notifier, void *data)
 {
     QemuSpicePointer *pointer = container_of(notifier, QemuSpicePointer, mouse_mode);
     bool is_absolute  = kbd_mouse_is_absolute();
@@ -213,5 +213,5 @@ void qemu_spice_input_init(void)
     pointer->absolute = false;
     pointer->mouse_mode.notify = mouse_mode_notifier;
     qemu_add_mouse_mode_change_notifier(&pointer->mouse_mode);
-    mouse_mode_notifier(&pointer->mouse_mode);
+    mouse_mode_notifier(&pointer->mouse_mode, NULL);
 }
diff --git a/ui/vnc.c b/ui/vnc.c
index 14f2930..f385bdd 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1346,7 +1346,7 @@ static void client_cut_text(VncState *vs, size_t len, uint8_t *text)
 {
 }
 
-static void check_pointer_type_change(Notifier *notifier)
+static void check_pointer_type_change(Notifier *notifier, void *data)
 {
     VncState *vs = container_of(notifier, VncState, mouse_mode_notifier);
     int absolute = kbd_mouse_is_absolute();
@@ -1769,7 +1769,7 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
         }
     }
     vnc_desktop_resize(vs);
-    check_pointer_type_change(&vs->mouse_mode_notifier);
+    check_pointer_type_change(&vs->mouse_mode_notifier, NULL);
 }
 
 static void set_pixel_conversion(VncState *vs)
diff --git a/usb-linux.c b/usb-linux.c
index 5d2ec5c..0ca442d 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -1189,7 +1189,7 @@ static int usb_host_close(USBHostDevice *dev)
     return 0;
 }
 
-static void usb_host_exit_notifier(struct Notifier* n)
+static void usb_host_exit_notifier(struct Notifier *n, void *data)
 {
     USBHostDevice *s = container_of(n, USBHostDevice, exit);
 
diff --git a/vl.c b/vl.c
index dbdec71..6014ba1 100644
--- a/vl.c
+++ b/vl.c
@@ -1987,7 +1987,7 @@ void qemu_remove_exit_notifier(Notifier *notify)
 
 static void qemu_run_exit_notifiers(void)
 {
-    notifier_list_notify(&exit_notifiers);
+    notifier_list_notify(&exit_notifiers, NULL);
 }
 
 void qemu_add_machine_init_done_notifier(Notifier *notify)
@@ -1997,7 +1997,7 @@ void qemu_add_machine_init_done_notifier(Notifier *notify)
 
 static void qemu_run_machine_init_done_notifiers(void)
 {
-    notifier_list_notify(&machine_init_done_notifiers);
+    notifier_list_notify(&machine_init_done_notifiers, NULL);
 }
 
 static const QEMUOption *lookup_opt(int argc, char **argv,
diff --git a/xen-all.c b/xen-all.c
index 0eac202..0ef52de 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -503,7 +503,7 @@ static void xen_vm_change_state_handler(void *opaque, int running, int reason)
     }
 }
 
-static void xen_exit_notifier(Notifier *n)
+static void xen_exit_notifier(Notifier *n, void *data)
 {
     XenIOState *state = container_of(n, XenIOState, exit);
 
-- 
1.7.1

[Qemu-devel] [PATCH v2 2/3] qemu-timer: Introduce clock reset notifier

[Jan Kiszka]

QEMU_CLOCK_HOST is based on the system time which may jump backward in
case the admin or NTP adjusts it. RTC emulations and other device models
can suffer in this case as timers will stall for the period the clock
was tuned back.

This adds a detection mechanism that checks on every host clock readout
if the new time is before the last result. If that is the case a
notifier list is informed. Device models interested in this event can
register a notifier with the clock.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 qemu-timer.c |   29 ++++++++++++++++++++++++++++-
 qemu-timer.h |    5 +++++
 2 files changed, 33 insertions(+), 1 deletions(-)

diff --git a/qemu-timer.c b/qemu-timer.c
index 72066c7..df323ae 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -150,6 +150,9 @@ struct QEMUClock {
     int enabled;
 
     QEMUTimer *warp_timer;
+
+    NotifierList reset_notifiers;
+    int64_t last;
 };
 
 struct QEMUTimer {
@@ -375,9 +378,15 @@ static QEMUTimer *active_timers[QEMU_NUM_CLOCKS];
 static QEMUClock *qemu_new_clock(int type)
 {
     QEMUClock *clock;
+
     clock = qemu_mallocz(sizeof(QEMUClock));
     clock->type = type;
     clock->enabled = 1;
+    notifier_list_init(&clock->reset_notifiers);
+    /* required to detect & report backward jumps */
+    if (type == QEMU_CLOCK_HOST) {
+        clock->last = get_clock_realtime();
+    }
     return clock;
 }
 
@@ -592,6 +601,8 @@ static void qemu_run_timers(QEMUClock *clock)
 
 int64_t qemu_get_clock_ns(QEMUClock *clock)
 {
+    int64_t now, last;
+
     switch(clock->type) {
     case QEMU_CLOCK_REALTIME:
         return get_clock();
@@ -603,10 +614,26 @@ int64_t qemu_get_clock_ns(QEMUClock *clock)
             return cpu_get_clock();
         }
     case QEMU_CLOCK_HOST:
-        return get_clock_realtime();
+        now = get_clock_realtime();
+        last = clock->last;
+        clock->last = now;
+        if (now < last) {
+            notifier_list_notify(&clock->reset_notifiers, &now);
+        }
+        return now;
     }
 }
 
+void qemu_register_clock_reset_notifier(QEMUClock *clock, Notifier *notifier)
+{
+    notifier_list_add(&clock->reset_notifiers, notifier);
+}
+
+void qemu_unregister_clock_reset_notifier(QEMUClock *clock, Notifier *notifier)
+{
+    notifier_list_remove(&clock->reset_notifiers, notifier);
+}
+
 void init_clocks(void)
 {
     rt_clock = qemu_new_clock(QEMU_CLOCK_REALTIME);
diff --git a/qemu-timer.h b/qemu-timer.h
index 06cbe20..0a43469 100644
--- a/qemu-timer.h
+++ b/qemu-timer.h
@@ -2,6 +2,7 @@
 #define QEMU_TIMER_H
 
 #include "qemu-common.h"
+#include "notify.h"
 #include <time.h>
 #include <sys/time.h>
 
@@ -40,6 +41,10 @@ int64_t qemu_get_clock_ns(QEMUClock *clock);
 void qemu_clock_enable(QEMUClock *clock, int enabled);
 void qemu_clock_warp(QEMUClock *clock);
 
+void qemu_register_clock_reset_notifier(QEMUClock *clock, Notifier *notifier);
+void qemu_unregister_clock_reset_notifier(QEMUClock *clock,
+                                          Notifier *notifier);
+
 QEMUTimer *qemu_new_timer(QEMUClock *clock, int scale,
                           QEMUTimerCB *cb, void *opaque);
 void qemu_free_timer(QEMUTimer *ts);
-- 
1.7.1

[Qemu-devel] [PATCH v2 3/3] mc146818rtc: Handle host clock resets

[Jan Kiszka]

Make use of the new clock reset notifier to update the RTC whenever
rtc_clock is the host clock and that happens to jump backward. This
avoids that the RTC stalls for the period the host clock was set back.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/mc146818rtc.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 1c9a706..feb3b25 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -99,6 +99,7 @@ typedef struct RTCState {
     QEMUTimer *coalesced_timer;
     QEMUTimer *second_timer;
     QEMUTimer *second_timer2;
+    Notifier clock_reset_notifier;
 } RTCState;
 
 static void rtc_set_time(RTCState *s);
@@ -572,6 +573,22 @@ static const VMStateDescription vmstate_rtc = {
     }
 };
 
+static void rtc_notify_clock_reset(Notifier *notifier, void *data)
+{
+    RTCState *s = container_of(notifier, RTCState, clock_reset_notifier);
+    int64_t now = *(int64_t *)data;
+
+    rtc_set_date_from_host(&s->dev);
+    s->next_second_time = now + (get_ticks_per_sec() * 99) / 100;
+    qemu_mod_timer(s->second_timer2, s->next_second_time);
+    rtc_timer_update(s, now);
+#ifdef TARGET_I386
+    if (rtc_td_hack) {
+        rtc_coalesced_timer_update(s);
+    }
+#endif
+}
+
 static void rtc_reset(void *opaque)
 {
     RTCState *s = opaque;
@@ -608,6 +625,9 @@ static int rtc_initfn(ISADevice *dev)
     s->second_timer = qemu_new_timer_ns(rtc_clock, rtc_update_second, s);
     s->second_timer2 = qemu_new_timer_ns(rtc_clock, rtc_update_second2, s);
 
+    s->clock_reset_notifier.notify = rtc_notify_clock_reset;
+    qemu_register_clock_reset_notifier(rtc_clock, &s->clock_reset_notifier);
+
     s->next_second_time =
         qemu_get_clock_ns(rtc_clock) + (get_ticks_per_sec() * 99) / 100;
     qemu_mod_timer(s->second_timer2, s->next_second_time);
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2 0/3] Let RTC follow backward jumps of host clock immediately

[Anthony Liguori]

On 06/20/2011 07:06 AM, Jan Kiszka wrote:
> Just noticed that this issue is still unfixed because my series was
> somehow forgotten. So I've rebased it over current master, refactored it
> to use the generic Notifier infrastructure and renamed it to "clock
> reset notifier" to avoid confusion with icount related "warping". Please
> review / apply before 0.15-rc0, it fixes a relevant issue.
>
> Original series description:
>
> By default, we base the mc146818 RTC on the host clock (CLOCK_REALTIME).
> This works fine if only the frequency of the host clock is tuned (e.g.
> by NTP) or if it is set to a future time. However, if the host is tuned
> backward, e.g. because NTP obtained the correct time after the guest was
> already started or the admin decided to tune the local time, we see an
> unpleasant effect in the guest: The RTC will stall for the period the
> host clock is set back. We identified that one prominent guest affected
> by this is Windows which relies on the periodic RTC interrupt for time
> keeping.
>
> This series address the issue by detecting those warps and providing a
> callback mechanism to device models. The RTC is enabled to update its
> timers and register content immediately. Tested successfully both with
> hwclock in a Linux guest and by monitoring the Windows clock while
> fiddling with the host time.
>
> Note that if this kind of RTC adjustment is not wanted, the user is
> still free to decouple the RTC from the host clock and base it on the
> VM clock - just like before.
>
> Jan Kiszka (3):
>    notifier: Pass data argument to callback
>    qemu-timer: Introduce clock reset notifier
>    mc146818rtc: Handle host clock resets

Applied.  Thanks.

Regards,

Anthony Liguori

>
>   hw/fw_cfg.c      |    2 +-
>   hw/mc146818rtc.c |   20 ++++++++++++++++++++
>   input.c          |    2 +-
>   migration.c      |   12 ++++++------
>   notify.c         |    4 ++--
>   notify.h         |    4 ++--
>   qemu-timer.c     |   29 ++++++++++++++++++++++++++++-
>   qemu-timer.h     |    5 +++++
>   ui/sdl.c         |    2 +-
>   ui/spice-core.c  |    2 +-
>   ui/spice-input.c |    4 ++--
>   ui/vnc.c         |    4 ++--
>   usb-linux.c      |    2 +-
>   vl.c             |    4 ++--
>   xen-all.c        |    2 +-
>   15 files changed, 75 insertions(+), 23 deletions(-)
>
>
>