[PATCH v5 0/5] hpet 'driftfix': alleviate time drift with HPET periodic timers

[PATCH v5 0/5] hpet 'driftfix': alleviate time drift with HPET periodic timers

[Ulrich Obergfell]

Hi,

This is version 5 of a series of patches that I originally posted in:

    http://lists.gnu.org/archive/html/qemu-devel/2011-03/msg01989.html
    http://lists.gnu.org/archive/html/qemu-devel/2011-03/msg01992.html
    http://lists.gnu.org/archive/html/qemu-devel/2011-03/msg01991.html
    http://lists.gnu.org/archive/html/qemu-devel/2011-03/msg01990.html

    http://article.gmane.org/gmane.comp.emulators.kvm.devel/69325
    http://article.gmane.org/gmane.comp.emulators.kvm.devel/69326
    http://article.gmane.org/gmane.comp.emulators.kvm.devel/69327
    http://article.gmane.org/gmane.comp.emulators.kvm.devel/69328


Changes since version 4:

Added comments to patch part 3 and part 5. No changes in the actual code.


Please review and please comment.

Regards,

Uli


Ulrich Obergfell (5):
  hpet 'driftfix': add hooks required to detect coalesced interrupts
    (x86 apic only)
  hpet 'driftfix': add driftfix property to HPETState and DeviceInfo
  hpet 'driftfix': add fields to HPETTimer and VMStateDescription
  hpet 'driftfix': add code in update_irq() to detect coalesced
    interrupts (x86 apic only)
  hpet 'driftfix': add code in hpet_timer() to compensate delayed
    callbacks and coalesced interrupts

 hw/apic.c |    4 ++
 hw/hpet.c |  178 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 hw/pc.h   |   13 +++++
 vl.c      |   13 +++++
 4 files changed, 204 insertions(+), 4 deletions(-)

[Qemu-devel] [PATCH v5 0/5] hpet 'driftfix': alleviate time drift with HPET periodic timers

[Ulrich Obergfell]

Hi,

This is version 5 of a series of patches that I originally posted in:

    http://lists.gnu.org/archive/html/qemu-devel/2011-03/msg01989.html
    http://lists.gnu.org/archive/html/qemu-devel/2011-03/msg01992.html
    http://lists.gnu.org/archive/html/qemu-devel/2011-03/msg01991.html
    http://lists.gnu.org/archive/html/qemu-devel/2011-03/msg01990.html

    http://article.gmane.org/gmane.comp.emulators.kvm.devel/69325
    http://article.gmane.org/gmane.comp.emulators.kvm.devel/69326
    http://article.gmane.org/gmane.comp.emulators.kvm.devel/69327
    http://article.gmane.org/gmane.comp.emulators.kvm.devel/69328


Changes since version 4:

Added comments to patch part 3 and part 5. No changes in the actual code.


Please review and please comment.

Regards,

Uli


Ulrich Obergfell (5):
  hpet 'driftfix': add hooks required to detect coalesced interrupts
    (x86 apic only)
  hpet 'driftfix': add driftfix property to HPETState and DeviceInfo
  hpet 'driftfix': add fields to HPETTimer and VMStateDescription
  hpet 'driftfix': add code in update_irq() to detect coalesced
    interrupts (x86 apic only)
  hpet 'driftfix': add code in hpet_timer() to compensate delayed
    callbacks and coalesced interrupts

 hw/apic.c |    4 ++
 hw/hpet.c |  178 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 hw/pc.h   |   13 +++++
 vl.c      |   13 +++++
 4 files changed, 204 insertions(+), 4 deletions(-)

[PATCH v5 1/5] hpet 'driftfix': add hooks required to detect coalesced interrupts (x86 apic only)

[Ulrich Obergfell]

'target_get_irq_delivered' and 'target_reset_irq_delivered' point
to functions that are called by update_irq() to detect coalesced
interrupts. Initially they point to stub functions which pretend
successful interrupt injection. apic code calls two registration
functions to replace the stubs with apic_get_irq_delivered() and
apic_reset_irq_delivered().

This change can be replaced if a generic feedback infrastructure to
track coalesced IRQs for periodic, clock providing devices becomes
available.

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
---
 hw/apic.c |    4 ++++
 hw/pc.h   |   13 +++++++++++++
 vl.c      |   13 +++++++++++++
 3 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index a45b57f..94b1d15 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -17,6 +17,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>
  */
 #include "hw.h"
+#include "pc.h"
 #include "apic.h"
 #include "ioapic.h"
 #include "qemu-timer.h"
@@ -1143,6 +1144,9 @@ static SysBusDeviceInfo apic_info = {
 
 static void apic_register_devices(void)
 {
+    register_target_get_irq_delivered(apic_get_irq_delivered);
+    register_target_reset_irq_delivered(apic_reset_irq_delivered);
+
     sysbus_register_withprop(&apic_info);
 }
 
diff --git a/hw/pc.h b/hw/pc.h
index bc8fcec..7511f28 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -7,6 +7,19 @@
 #include "fdc.h"
 #include "net.h"
 
+extern int (*target_get_irq_delivered)(void);
+extern void (*target_reset_irq_delivered)(void);
+
+static inline void register_target_get_irq_delivered(int (*func)(void))
+{
+    target_get_irq_delivered = func;
+}
+
+static inline void register_target_reset_irq_delivered(void (*func)(void))
+{
+    target_reset_irq_delivered = func;
+}
+
 /* PC-style peripherals (also used by other machines).  */
 
 /* serial.c */
diff --git a/vl.c b/vl.c
index 73e147f..456e320 100644
--- a/vl.c
+++ b/vl.c
@@ -232,6 +232,19 @@ const char *prom_envs[MAX_PROM_ENVS];
 const char *nvram = NULL;
 int boot_menu;
 
+static int target_get_irq_delivered_stub(void)
+{
+    return 1;
+}
+
+static void target_reset_irq_delivered_stub(void)
+{
+    return;
+}
+
+int (*target_get_irq_delivered)(void) = target_get_irq_delivered_stub;
+void (*target_reset_irq_delivered)(void) = target_reset_irq_delivered_stub;
+
 typedef struct FWBootEntry FWBootEntry;
 
 struct FWBootEntry {
-- 
1.6.2.5

[Qemu-devel] [PATCH v5 1/5] hpet 'driftfix': add hooks required to detect coalesced interrupts (x86 apic only)

[Ulrich Obergfell]

'target_get_irq_delivered' and 'target_reset_irq_delivered' point
to functions that are called by update_irq() to detect coalesced
interrupts. Initially they point to stub functions which pretend
successful interrupt injection. apic code calls two registration
functions to replace the stubs with apic_get_irq_delivered() and
apic_reset_irq_delivered().

This change can be replaced if a generic feedback infrastructure to
track coalesced IRQs for periodic, clock providing devices becomes
available.

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
---
 hw/apic.c |    4 ++++
 hw/pc.h   |   13 +++++++++++++
 vl.c      |   13 +++++++++++++
 3 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index a45b57f..94b1d15 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -17,6 +17,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>
  */
 #include "hw.h"
+#include "pc.h"
 #include "apic.h"
 #include "ioapic.h"
 #include "qemu-timer.h"
@@ -1143,6 +1144,9 @@ static SysBusDeviceInfo apic_info = {
 
 static void apic_register_devices(void)
 {
+    register_target_get_irq_delivered(apic_get_irq_delivered);
+    register_target_reset_irq_delivered(apic_reset_irq_delivered);
+
     sysbus_register_withprop(&apic_info);
 }
 
diff --git a/hw/pc.h b/hw/pc.h
index bc8fcec..7511f28 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -7,6 +7,19 @@
 #include "fdc.h"
 #include "net.h"
 
+extern int (*target_get_irq_delivered)(void);
+extern void (*target_reset_irq_delivered)(void);
+
+static inline void register_target_get_irq_delivered(int (*func)(void))
+{
+    target_get_irq_delivered = func;
+}
+
+static inline void register_target_reset_irq_delivered(void (*func)(void))
+{
+    target_reset_irq_delivered = func;
+}
+
 /* PC-style peripherals (also used by other machines).  */
 
 /* serial.c */
diff --git a/vl.c b/vl.c
index 73e147f..456e320 100644
--- a/vl.c
+++ b/vl.c
@@ -232,6 +232,19 @@ const char *prom_envs[MAX_PROM_ENVS];
 const char *nvram = NULL;
 int boot_menu;
 
+static int target_get_irq_delivered_stub(void)
+{
+    return 1;
+}
+
+static void target_reset_irq_delivered_stub(void)
+{
+    return;
+}
+
+int (*target_get_irq_delivered)(void) = target_get_irq_delivered_stub;
+void (*target_reset_irq_delivered)(void) = target_reset_irq_delivered_stub;
+
 typedef struct FWBootEntry FWBootEntry;
 
 struct FWBootEntry {
-- 
1.6.2.5

[PATCH v5 2/5] hpet 'driftfix': add driftfix property to HPETState and DeviceInfo

[Ulrich Obergfell]

driftfix is a 'bit type' property. Compensation of delayed callbacks
and coalesced interrupts can be enabled with the command line option

    -global hpet.driftfix=on

driftfix is 'off' (disabled) by default.

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
---
 hw/hpet.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/hpet.c b/hw/hpet.c
index 6ce07bc..7513065 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -72,6 +72,8 @@ typedef struct HPETState {
     uint64_t isr;               /* interrupt status reg */
     uint64_t hpet_counter;      /* main counter */
     uint8_t  hpet_id;           /* instance id */
+
+    uint32_t driftfix;
 } HPETState;
 
 static uint32_t hpet_in_legacy_mode(HPETState *s)
@@ -738,6 +740,7 @@ static SysBusDeviceInfo hpet_device_info = {
     .qdev.props = (Property[]) {
         DEFINE_PROP_UINT8("timers", HPETState, num_timers, HPET_MIN_TIMERS),
         DEFINE_PROP_BIT("msi", HPETState, flags, HPET_MSI_SUPPORT, false),
+        DEFINE_PROP_BIT("driftfix", HPETState, driftfix, 0, false),
         DEFINE_PROP_END_OF_LIST(),
     },
 };
-- 
1.6.2.5

[Qemu-devel] [PATCH v5 2/5] hpet 'driftfix': add driftfix property to HPETState and DeviceInfo

[Ulrich Obergfell]

driftfix is a 'bit type' property. Compensation of delayed callbacks
and coalesced interrupts can be enabled with the command line option

    -global hpet.driftfix=on

driftfix is 'off' (disabled) by default.

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
---
 hw/hpet.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/hpet.c b/hw/hpet.c
index 6ce07bc..7513065 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -72,6 +72,8 @@ typedef struct HPETState {
     uint64_t isr;               /* interrupt status reg */
     uint64_t hpet_counter;      /* main counter */
     uint8_t  hpet_id;           /* instance id */
+
+    uint32_t driftfix;
 } HPETState;
 
 static uint32_t hpet_in_legacy_mode(HPETState *s)
@@ -738,6 +740,7 @@ static SysBusDeviceInfo hpet_device_info = {
     .qdev.props = (Property[]) {
         DEFINE_PROP_UINT8("timers", HPETState, num_timers, HPET_MIN_TIMERS),
         DEFINE_PROP_BIT("msi", HPETState, flags, HPET_MSI_SUPPORT, false),
+        DEFINE_PROP_BIT("driftfix", HPETState, driftfix, 0, false),
         DEFINE_PROP_END_OF_LIST(),
     },
 };
-- 
1.6.2.5

[PATCH v5 3/5] hpet 'driftfix': add fields to HPETTimer and VMStateDescription

[Ulrich Obergfell]

The new fields in HPETTimer are covered by a separate VMStateDescription
which is a subsection of 'vmstate_hpet_timer'. They are only migrated if

    -global hpet.driftfix=on

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
---
 hw/hpet.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/hw/hpet.c b/hw/hpet.c
index 7513065..dba9370 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -55,6 +55,19 @@ typedef struct HPETTimer {  /* timers */
     uint8_t wrap_flag;      /* timer pop will indicate wrap for one-shot 32-bit
                              * mode. Next pop will be actual timer expiration.
                              */
+    /* driftfix state */
+    uint64_t prev_period;            /* needed when the guest o/s changes the
+                                      * comparator value */
+    uint64_t ticks_not_accounted;    /* ticks for which no interrupts have been
+                                      * delivered to the guest o/s yet */
+    uint32_t irq_rate;               /* rate at which interrupts are delivered
+                                      * to the guest o/s during one period
+                                      * interval; if rate is greater than 1,
+                                      * additional interrupts are delivered
+                                      * to compensate missed interrupts */
+    uint32_t divisor;                /* needed to determine when the next
+                                      * timer callback should occur while
+                                      * rate is greater than 1 */
 } HPETTimer;
 
 typedef struct HPETState {
@@ -246,6 +259,27 @@ static int hpet_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static bool hpet_timer_driftfix_vmstate_needed(void *opaque)
+{
+    HPETTimer *t = opaque;
+
+    return (t->state->driftfix != 0);
+}
+
+static const VMStateDescription vmstate_hpet_timer_driftfix = {
+    .name = "hpet_timer_driftfix",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_UINT64(prev_period, HPETTimer),
+        VMSTATE_UINT64(ticks_not_accounted, HPETTimer),
+        VMSTATE_UINT32(irq_rate, HPETTimer),
+        VMSTATE_UINT32(divisor, HPETTimer),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_hpet_timer = {
     .name = "hpet_timer",
     .version_id = 1,
@@ -260,6 +294,14 @@ static const VMStateDescription vmstate_hpet_timer = {
         VMSTATE_UINT8(wrap_flag, HPETTimer),
         VMSTATE_TIMER(qemu_timer, HPETTimer),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection []) {
+        {
+            .vmsd = &vmstate_hpet_timer_driftfix,
+            .needed = hpet_timer_driftfix_vmstate_needed,
+        }, {
+            /* empty */
+        }
     }
 };
 
-- 
1.6.2.5

[Qemu-devel] [PATCH v5 3/5] hpet 'driftfix': add fields to HPETTimer and VMStateDescription

[Ulrich Obergfell]

The new fields in HPETTimer are covered by a separate VMStateDescription
which is a subsection of 'vmstate_hpet_timer'. They are only migrated if

    -global hpet.driftfix=on

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
---
 hw/hpet.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/hw/hpet.c b/hw/hpet.c
index 7513065..dba9370 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -55,6 +55,19 @@ typedef struct HPETTimer {  /* timers */
     uint8_t wrap_flag;      /* timer pop will indicate wrap for one-shot 32-bit
                              * mode. Next pop will be actual timer expiration.
                              */
+    /* driftfix state */
+    uint64_t prev_period;            /* needed when the guest o/s changes the
+                                      * comparator value */
+    uint64_t ticks_not_accounted;    /* ticks for which no interrupts have been
+                                      * delivered to the guest o/s yet */
+    uint32_t irq_rate;               /* rate at which interrupts are delivered
+                                      * to the guest o/s during one period
+                                      * interval; if rate is greater than 1,
+                                      * additional interrupts are delivered
+                                      * to compensate missed interrupts */
+    uint32_t divisor;                /* needed to determine when the next
+                                      * timer callback should occur while
+                                      * rate is greater than 1 */
 } HPETTimer;
 
 typedef struct HPETState {
@@ -246,6 +259,27 @@ static int hpet_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static bool hpet_timer_driftfix_vmstate_needed(void *opaque)
+{
+    HPETTimer *t = opaque;
+
+    return (t->state->driftfix != 0);
+}
+
+static const VMStateDescription vmstate_hpet_timer_driftfix = {
+    .name = "hpet_timer_driftfix",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_UINT64(prev_period, HPETTimer),
+        VMSTATE_UINT64(ticks_not_accounted, HPETTimer),
+        VMSTATE_UINT32(irq_rate, HPETTimer),
+        VMSTATE_UINT32(divisor, HPETTimer),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_hpet_timer = {
     .name = "hpet_timer",
     .version_id = 1,
@@ -260,6 +294,14 @@ static const VMStateDescription vmstate_hpet_timer = {
         VMSTATE_UINT8(wrap_flag, HPETTimer),
         VMSTATE_TIMER(qemu_timer, HPETTimer),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection []) {
+        {
+            .vmsd = &vmstate_hpet_timer_driftfix,
+            .needed = hpet_timer_driftfix_vmstate_needed,
+        }, {
+            /* empty */
+        }
     }
 };
 
-- 
1.6.2.5

[PATCH v5 4/5] hpet 'driftfix': add code in update_irq() to detect coalesced interrupts (x86 apic only)

[Ulrich Obergfell]

update_irq() uses a similar method as in 'rtc_td_hack' to detect
coalesced interrupts. The function entry addresses are retrieved
from 'target_get_irq_delivered' and 'target_reset_irq_delivered'.

This change can be replaced if a generic feedback infrastructure to
track coalesced IRQs for periodic, clock providing devices becomes
available.

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
---
 hw/hpet.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/hpet.c b/hw/hpet.c
index dba9370..0428290 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -184,11 +184,12 @@ static inline uint64_t hpet_calculate_diff(HPETTimer *t, uint64_t current)
     }
 }
 
-static void update_irq(struct HPETTimer *timer, int set)
+static int update_irq(struct HPETTimer *timer, int set)
 {
     uint64_t mask;
     HPETState *s;
     int route;
+    int irq_delivered = 1;
 
     if (timer->tn <= 1 && hpet_in_legacy_mode(timer->state)) {
         /* if LegacyReplacementRoute bit is set, HPET specification requires
@@ -213,8 +214,16 @@ static void update_irq(struct HPETTimer *timer, int set)
         qemu_irq_raise(s->irqs[route]);
     } else {
         s->isr &= ~mask;
-        qemu_irq_pulse(s->irqs[route]);
+        if (s->driftfix) {
+            target_reset_irq_delivered();
+            qemu_irq_raise(s->irqs[route]);
+            irq_delivered = target_get_irq_delivered();
+            qemu_irq_lower(s->irqs[route]);
+        } else {
+            qemu_irq_pulse(s->irqs[route]);
+        }
     }
+    return irq_delivered;
 }
 
 static void hpet_pre_save(void *opaque)
-- 
1.6.2.5

[Qemu-devel] [PATCH v5 4/5] hpet 'driftfix': add code in update_irq() to detect coalesced interrupts (x86 apic only)

[Ulrich Obergfell]

update_irq() uses a similar method as in 'rtc_td_hack' to detect
coalesced interrupts. The function entry addresses are retrieved
from 'target_get_irq_delivered' and 'target_reset_irq_delivered'.

This change can be replaced if a generic feedback infrastructure to
track coalesced IRQs for periodic, clock providing devices becomes
available.

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
---
 hw/hpet.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/hpet.c b/hw/hpet.c
index dba9370..0428290 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -184,11 +184,12 @@ static inline uint64_t hpet_calculate_diff(HPETTimer *t, uint64_t current)
     }
 }
 
-static void update_irq(struct HPETTimer *timer, int set)
+static int update_irq(struct HPETTimer *timer, int set)
 {
     uint64_t mask;
     HPETState *s;
     int route;
+    int irq_delivered = 1;
 
     if (timer->tn <= 1 && hpet_in_legacy_mode(timer->state)) {
         /* if LegacyReplacementRoute bit is set, HPET specification requires
@@ -213,8 +214,16 @@ static void update_irq(struct HPETTimer *timer, int set)
         qemu_irq_raise(s->irqs[route]);
     } else {
         s->isr &= ~mask;
-        qemu_irq_pulse(s->irqs[route]);
+        if (s->driftfix) {
+            target_reset_irq_delivered();
+            qemu_irq_raise(s->irqs[route]);
+            irq_delivered = target_get_irq_delivered();
+            qemu_irq_lower(s->irqs[route]);
+        } else {
+            qemu_irq_pulse(s->irqs[route]);
+        }
     }
+    return irq_delivered;
 }
 
 static void hpet_pre_save(void *opaque)
-- 
1.6.2.5

[PATCH v5 5/5] hpet 'driftfix': add code in hpet_timer() to compensate delayed callbacks and coalesced interrupts

[Ulrich Obergfell]

Loss of periodic timer interrupts caused by delayed callbacks and by
interrupt coalescing is compensated by gradually injecting additional
interrupts during subsequent timer intervals, starting at a rate of
one additional interrupt per interval. The injection of additional
interrupts is based on a backlog of unaccounted HPET clock periods
(new HPETTimer field 'ticks_not_accounted'). The backlog increases
due to delayed callbacks and coalesced interrupts, and it decreases
if an interrupt was injected successfully. If the backlog increases
while compensation is still in progress, the rate at which additional
interrupts are injected is increased too. A limit is imposed on the
backlog and on the rate.

Injecting additional timer interrupts to compensate lost interrupts
can alleviate long term time drift. However, on a short time scale,
this method can have the side effect of making virtual machine time
intermittently pass slower and faster than real time (depending on
the guest's time keeping algorithm). Compensation is disabled by
default and can be enabled for guests where this behaviour may be
acceptable.

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
---
 hw/hpet.c |  120 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 118 insertions(+), 2 deletions(-)

diff --git a/hw/hpet.c b/hw/hpet.c
index 0428290..bc2a21a 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -31,6 +31,7 @@
 #include "hpet_emul.h"
 #include "sysbus.h"
 #include "mc146818rtc.h"
+#include <assert.h>
 
 //#define HPET_DEBUG
 #ifdef HPET_DEBUG
@@ -41,6 +42,9 @@
 
 #define HPET_MSI_SUPPORT        0
 
+#define MAX_TICKS_NOT_ACCOUNTED     (uint64_t)500000000 /* 5 sec */
+#define MAX_IRQ_RATE                (uint32_t)10
+
 struct HPETState;
 typedef struct HPETTimer {  /* timers */
     uint8_t tn;             /*timer number*/
@@ -334,13 +338,68 @@ static const VMStateDescription vmstate_hpet = {
 };
 
 /*
+ * This function resets the driftfix state in the following situations.
+ *
+ * - When the guest o/s changes the 'CFG_ENABLE' bit (overall enable)
+ *   in the General Configuration Register from 0 to 1.
+ *
+ * - When the guest o/s changes the 'TN_ENABLE' bit (timer N interrupt enable)
+ *   in the Timer N Configuration and Capabilities Register from 0 to 1.
+ */
+static void hpet_timer_driftfix_reset(HPETTimer *t)
+{
+    if (t->state->driftfix && timer_is_periodic(t)) {
+        t->ticks_not_accounted = t->prev_period = t->period;
+        t->irq_rate = 1;
+        t->divisor = 1;
+    }
+}
+
+/*
+ * This function determines whether there is a backlog of ticks for which
+ * no interrupts have been delivered to the guest o/s yet. If the backlog
+ * is equal to or greater than the current period length, then additional
+ * interrupts will be delivered to the guest o/s inside of the subsequent
+ * period interval to compensate missed interrupts.
+ *
+ * 'ticks_not_accounted' increases by 'N * period' when the comparator is
+ * being advanced, and it decreases by 'prev_period' when an interrupt is
+ * delivered to the guest o/s. Normally 'prev_period' is equal to 'period'
+ * and 'N' is 1. 'prev_period' is different from 'period' if a guest o/s
+ * has changed the comparator value during the previous period interval.
+ * 'N' is greater than 1 if the callback was delayed by 'N - 1' periods,
+ * and 'N' is zero while additional interrupts are delivered inside of an
+ * interval.
+ *
+ * This function is called after the comparator has been advanced but before
+ * the interrupt is delivered to the guest o/s. Hence, 'ticks_not_accounted'
+ * is equal to 'prev_period' plus 'period' if there is no backlog.
+ */
+static bool hpet_timer_has_tick_backlog(HPETTimer *t)
+{
+    uint64_t backlog = 0;
+
+    if (t->ticks_not_accounted >= t->period + t->prev_period) {
+        backlog = t->ticks_not_accounted - (t->period + t->prev_period);
+    }
+    return (backlog >= t->period);
+}
+
+/*
  * timer expiration callback
  */
 static void hpet_timer(void *opaque)
 {
     HPETTimer *t = opaque;
+    HPETState *s = t->state;
     uint64_t diff;
-
+    int irq_delivered = 0;
+    uint32_t period_count = 0;   /* elapsed periods since last callback
+                                  *   1: normal case
+                                  * > 1: missed 'period_count - 1' interrupts
+                                  *      due to delayed callback
+                                  *   0: callback inside of an interval
+                                  *      to deliver additional interrupts */
     uint64_t period = t->period;
     uint64_t cur_tick = hpet_get_ticks(t->state);
 
@@ -348,13 +407,48 @@ static void hpet_timer(void *opaque)
         if (t->config & HPET_TN_32BIT) {
             while (hpet_time_after(cur_tick, t->cmp)) {
                 t->cmp = (uint32_t)(t->cmp + t->period);
+                t->ticks_not_accounted += t->period;
+                period_count++;
             }
         } else {
             while (hpet_time_after64(cur_tick, t->cmp)) {
                 t->cmp += period;
+                t->ticks_not_accounted += period;
+                period_count++;
             }
         }
         diff = hpet_calculate_diff(t, cur_tick);
+        if (s->driftfix) {
+            if (t->ticks_not_accounted > MAX_TICKS_NOT_ACCOUNTED) {
+                t->ticks_not_accounted = t->period + t->prev_period;
+            }
+            if (hpet_timer_has_tick_backlog(t)) {
+                if (t->irq_rate == 1 || period_count > 1) {
+                    /* Increase the interrupt delivery rate if compensation
+                     * was not already in progress or if this callback was
+                     * delayed. */
+                    t->irq_rate++;
+                    t->irq_rate = MIN(t->irq_rate, MAX_IRQ_RATE);
+                }
+                if (t->divisor == 0) {
+                    assert(period_count);
+                }
+                if (period_count) {
+                    /* A non-zero 'period_count' marks the start of a new
+                     * period interval. Initialize the divisor now. */
+                    t->divisor = t->irq_rate;
+                }
+                /* 'diff' is the amount of ticks between the current callback
+                 * and the start of the next period interval. 'diff' becomes
+                 * smaller at each additional callback inside of an interval
+                 * while compensation is in progress. Decrement the divisor
+                 * here, so that the entire interval is divided into parts
+                 * of similar size. */
+                diff /= t->divisor--;
+            } else {
+                t->irq_rate = 1;
+            }
+        }
         qemu_mod_timer(t->qemu_timer,
                        qemu_get_clock_ns(vm_clock) + (int64_t)ticks_to_ns(diff));
     } else if (t->config & HPET_TN_32BIT && !timer_is_periodic(t)) {
@@ -365,7 +459,27 @@ static void hpet_timer(void *opaque)
             t->wrap_flag = 0;
         }
     }
-    update_irq(t, 1);
+    if (s->driftfix && timer_is_periodic(t) && period != 0) {
+        if (t->ticks_not_accounted >= t->period + t->prev_period) {
+            irq_delivered = update_irq(t, 1);
+            if (irq_delivered) {
+                t->ticks_not_accounted -= t->prev_period;
+                t->prev_period = t->period;
+            } else {
+                if (period_count) {
+                    /* If the interrupt was not delivered to the guest o/s,
+                     * increase the delivery rate only if this happened at
+                     * the start of a period interval (a failure to deliver
+                     * additional interrupts inside of an interval does not
+                     * cause an increase). */
+                    t->irq_rate++;
+                    t->irq_rate = MIN(t->irq_rate, MAX_IRQ_RATE);
+                }
+            }
+        }
+    } else {
+        update_irq(t, 1);
+    }
 }
 
 static void hpet_set_timer(HPETTimer *t)
@@ -534,6 +648,7 @@ static void hpet_ram_writel(void *opaque, target_phys_addr_t addr,
                 timer->period = (uint32_t)timer->period;
             }
             if (activating_bit(old_val, new_val, HPET_TN_ENABLE)) {
+                hpet_timer_driftfix_reset(timer);
                 hpet_set_timer(timer);
             } else if (deactivating_bit(old_val, new_val, HPET_TN_ENABLE)) {
                 hpet_del_timer(timer);
@@ -608,6 +723,7 @@ static void hpet_ram_writel(void *opaque, target_phys_addr_t addr,
                     ticks_to_ns(s->hpet_counter) - qemu_get_clock_ns(vm_clock);
                 for (i = 0; i < s->num_timers; i++) {
                     if ((&s->timer[i])->cmp != ~0ULL) {
+                        hpet_timer_driftfix_reset(&s->timer[i]);
                         hpet_set_timer(&s->timer[i]);
                     }
                 }
-- 
1.6.2.5

[Qemu-devel] [PATCH v5 5/5] hpet 'driftfix': add code in hpet_timer() to compensate delayed callbacks and coalesced interrupts

[Ulrich Obergfell]

Loss of periodic timer interrupts caused by delayed callbacks and by
interrupt coalescing is compensated by gradually injecting additional
interrupts during subsequent timer intervals, starting at a rate of
one additional interrupt per interval. The injection of additional
interrupts is based on a backlog of unaccounted HPET clock periods
(new HPETTimer field 'ticks_not_accounted'). The backlog increases
due to delayed callbacks and coalesced interrupts, and it decreases
if an interrupt was injected successfully. If the backlog increases
while compensation is still in progress, the rate at which additional
interrupts are injected is increased too. A limit is imposed on the
backlog and on the rate.

Injecting additional timer interrupts to compensate lost interrupts
can alleviate long term time drift. However, on a short time scale,
this method can have the side effect of making virtual machine time
intermittently pass slower and faster than real time (depending on
the guest's time keeping algorithm). Compensation is disabled by
default and can be enabled for guests where this behaviour may be
acceptable.

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
---
 hw/hpet.c |  120 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 118 insertions(+), 2 deletions(-)

diff --git a/hw/hpet.c b/hw/hpet.c
index 0428290..bc2a21a 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -31,6 +31,7 @@
 #include "hpet_emul.h"
 #include "sysbus.h"
 #include "mc146818rtc.h"
+#include <assert.h>
 
 //#define HPET_DEBUG
 #ifdef HPET_DEBUG
@@ -41,6 +42,9 @@
 
 #define HPET_MSI_SUPPORT        0
 
+#define MAX_TICKS_NOT_ACCOUNTED     (uint64_t)500000000 /* 5 sec */
+#define MAX_IRQ_RATE                (uint32_t)10
+
 struct HPETState;
 typedef struct HPETTimer {  /* timers */
     uint8_t tn;             /*timer number*/
@@ -334,13 +338,68 @@ static const VMStateDescription vmstate_hpet = {
 };
 
 /*
+ * This function resets the driftfix state in the following situations.
+ *
+ * - When the guest o/s changes the 'CFG_ENABLE' bit (overall enable)
+ *   in the General Configuration Register from 0 to 1.
+ *
+ * - When the guest o/s changes the 'TN_ENABLE' bit (timer N interrupt enable)
+ *   in the Timer N Configuration and Capabilities Register from 0 to 1.
+ */
+static void hpet_timer_driftfix_reset(HPETTimer *t)
+{
+    if (t->state->driftfix && timer_is_periodic(t)) {
+        t->ticks_not_accounted = t->prev_period = t->period;
+        t->irq_rate = 1;
+        t->divisor = 1;
+    }
+}
+
+/*
+ * This function determines whether there is a backlog of ticks for which
+ * no interrupts have been delivered to the guest o/s yet. If the backlog
+ * is equal to or greater than the current period length, then additional
+ * interrupts will be delivered to the guest o/s inside of the subsequent
+ * period interval to compensate missed interrupts.
+ *
+ * 'ticks_not_accounted' increases by 'N * period' when the comparator is
+ * being advanced, and it decreases by 'prev_period' when an interrupt is
+ * delivered to the guest o/s. Normally 'prev_period' is equal to 'period'
+ * and 'N' is 1. 'prev_period' is different from 'period' if a guest o/s
+ * has changed the comparator value during the previous period interval.
+ * 'N' is greater than 1 if the callback was delayed by 'N - 1' periods,
+ * and 'N' is zero while additional interrupts are delivered inside of an
+ * interval.
+ *
+ * This function is called after the comparator has been advanced but before
+ * the interrupt is delivered to the guest o/s. Hence, 'ticks_not_accounted'
+ * is equal to 'prev_period' plus 'period' if there is no backlog.
+ */
+static bool hpet_timer_has_tick_backlog(HPETTimer *t)
+{
+    uint64_t backlog = 0;
+
+    if (t->ticks_not_accounted >= t->period + t->prev_period) {
+        backlog = t->ticks_not_accounted - (t->period + t->prev_period);
+    }
+    return (backlog >= t->period);
+}
+
+/*
  * timer expiration callback
  */
 static void hpet_timer(void *opaque)
 {
     HPETTimer *t = opaque;
+    HPETState *s = t->state;
     uint64_t diff;
-
+    int irq_delivered = 0;
+    uint32_t period_count = 0;   /* elapsed periods since last callback
+                                  *   1: normal case
+                                  * > 1: missed 'period_count - 1' interrupts
+                                  *      due to delayed callback
+                                  *   0: callback inside of an interval
+                                  *      to deliver additional interrupts */
     uint64_t period = t->period;
     uint64_t cur_tick = hpet_get_ticks(t->state);
 
@@ -348,13 +407,48 @@ static void hpet_timer(void *opaque)
         if (t->config & HPET_TN_32BIT) {
             while (hpet_time_after(cur_tick, t->cmp)) {
                 t->cmp = (uint32_t)(t->cmp + t->period);
+                t->ticks_not_accounted += t->period;
+                period_count++;
             }
         } else {
             while (hpet_time_after64(cur_tick, t->cmp)) {
                 t->cmp += period;
+                t->ticks_not_accounted += period;
+                period_count++;
             }
         }
         diff = hpet_calculate_diff(t, cur_tick);
+        if (s->driftfix) {
+            if (t->ticks_not_accounted > MAX_TICKS_NOT_ACCOUNTED) {
+                t->ticks_not_accounted = t->period + t->prev_period;
+            }
+            if (hpet_timer_has_tick_backlog(t)) {
+                if (t->irq_rate == 1 || period_count > 1) {
+                    /* Increase the interrupt delivery rate if compensation
+                     * was not already in progress or if this callback was
+                     * delayed. */
+                    t->irq_rate++;
+                    t->irq_rate = MIN(t->irq_rate, MAX_IRQ_RATE);
+                }
+                if (t->divisor == 0) {
+                    assert(period_count);
+                }
+                if (period_count) {
+                    /* A non-zero 'period_count' marks the start of a new
+                     * period interval. Initialize the divisor now. */
+                    t->divisor = t->irq_rate;
+                }
+                /* 'diff' is the amount of ticks between the current callback
+                 * and the start of the next period interval. 'diff' becomes
+                 * smaller at each additional callback inside of an interval
+                 * while compensation is in progress. Decrement the divisor
+                 * here, so that the entire interval is divided into parts
+                 * of similar size. */
+                diff /= t->divisor--;
+            } else {
+                t->irq_rate = 1;
+            }
+        }
         qemu_mod_timer(t->qemu_timer,
                        qemu_get_clock_ns(vm_clock) + (int64_t)ticks_to_ns(diff));
     } else if (t->config & HPET_TN_32BIT && !timer_is_periodic(t)) {
@@ -365,7 +459,27 @@ static void hpet_timer(void *opaque)
             t->wrap_flag = 0;
         }
     }
-    update_irq(t, 1);
+    if (s->driftfix && timer_is_periodic(t) && period != 0) {
+        if (t->ticks_not_accounted >= t->period + t->prev_period) {
+            irq_delivered = update_irq(t, 1);
+            if (irq_delivered) {
+                t->ticks_not_accounted -= t->prev_period;
+                t->prev_period = t->period;
+            } else {
+                if (period_count) {
+                    /* If the interrupt was not delivered to the guest o/s,
+                     * increase the delivery rate only if this happened at
+                     * the start of a period interval (a failure to deliver
+                     * additional interrupts inside of an interval does not
+                     * cause an increase). */
+                    t->irq_rate++;
+                    t->irq_rate = MIN(t->irq_rate, MAX_IRQ_RATE);
+                }
+            }
+        }
+    } else {
+        update_irq(t, 1);
+    }
 }
 
 static void hpet_set_timer(HPETTimer *t)
@@ -534,6 +648,7 @@ static void hpet_ram_writel(void *opaque, target_phys_addr_t addr,
                 timer->period = (uint32_t)timer->period;
             }
             if (activating_bit(old_val, new_val, HPET_TN_ENABLE)) {
+                hpet_timer_driftfix_reset(timer);
                 hpet_set_timer(timer);
             } else if (deactivating_bit(old_val, new_val, HPET_TN_ENABLE)) {
                 hpet_del_timer(timer);
@@ -608,6 +723,7 @@ static void hpet_ram_writel(void *opaque, target_phys_addr_t addr,
                     ticks_to_ns(s->hpet_counter) - qemu_get_clock_ns(vm_clock);
                 for (i = 0; i < s->num_timers; i++) {
                     if ((&s->timer[i])->cmp != ~0ULL) {
+                        hpet_timer_driftfix_reset(&s->timer[i]);
                         hpet_set_timer(&s->timer[i]);
                     }
                 }
-- 
1.6.2.5