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

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

[Ulrich Obergfell]

Hi,

This is version 3 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 2:

 -  The vmstate related to 'driftfix' is now in a separate subsection
    that can be migrated conditionally.

 -  The new fields of the HPETTimer structure are no longer initialized
    in hpet_init(). This is now done in hpet_reset() only.

 -  Compensation of lost timer interrupts is now based on a backlog of
    unaccounted HPET clock periods instead of 'irqs_to_inject'. This
    eliminates the need to scale 'irqs_to_inject' when a guest o/s
    modifies the comparator register value.

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 |  114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 sysemu.h  |    3 ++
 vl.c      |    3 ++
 4 files changed, 120 insertions(+), 4 deletions(-)

[Qemu-devel] [PATCH v3 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' contain
entry addresses of functions that are utilized by update_irq() to
detect coalesced interrupts. apic code loads these pointers during
initialization.

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 ++++
 sysemu.h  |    3 +++
 vl.c      |    3 +++
 3 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index a45b57f..eb0f6d9 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -24,6 +24,7 @@
 #include "sysbus.h"
 #include "trace.h"
 #include "kvm.h"
+#include "sysemu.h"
 
 /* APIC Local Vector Table */
 #define APIC_LVT_TIMER   0
@@ -1143,6 +1144,9 @@ static SysBusDeviceInfo apic_info = {
 
 static void apic_register_devices(void)
 {
+    target_get_irq_delivered = apic_get_irq_delivered;
+    target_reset_irq_delivered = apic_reset_irq_delivered;
+
     sysbus_register_withprop(&apic_info);
 }
 
diff --git a/sysemu.h b/sysemu.h
index 07d85cd..75b0139 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -98,6 +98,9 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f);
 void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f);
 int qemu_loadvm_state(QEMUFile *f);
 
+extern int (*target_get_irq_delivered)(void);
+extern void (*target_reset_irq_delivered)(void);
+
 /* SLIRP */
 void do_info_slirp(Monitor *mon);
 
diff --git a/vl.c b/vl.c
index 0c24e07..7bab315 100644
--- a/vl.c
+++ b/vl.c
@@ -233,6 +233,9 @@ const char *prom_envs[MAX_PROM_ENVS];
 const char *nvram = NULL;
 int boot_menu;
 
+int (*target_get_irq_delivered)(void) = 0;
+void (*target_reset_irq_delivered)(void) = 0;
+
 typedef struct FWBootEntry FWBootEntry;
 
 struct FWBootEntry {
-- 
1.6.2.5

[Qemu-devel] [PATCH v3 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 v3 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 |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/hw/hpet.c b/hw/hpet.c
index 7513065..7ab6e62 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -55,6 +55,10 @@ 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.
                              */
+    uint64_t prev_period;
+    uint64_t ticks_not_accounted;
+    uint32_t irq_rate;
+    uint32_t divisor;
 } HPETTimer;
 
 typedef struct HPETState {
@@ -246,6 +250,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 +285,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 v3 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 |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/hw/hpet.c b/hw/hpet.c
index 7ab6e62..35466ae 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -31,6 +31,7 @@
 #include "hpet_emul.h"
 #include "sysbus.h"
 #include "mc146818rtc.h"
+#include "sysemu.h"
 
 //#define HPET_DEBUG
 #ifdef HPET_DEBUG
@@ -175,11 +176,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
@@ -204,8 +206,17 @@ 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_get_irq_delivered
+                        && target_reset_irq_delivered) {
+            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 v3 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 |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/hw/hpet.c b/hw/hpet.c
index 35466ae..92d5f58 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -32,6 +32,7 @@
 #include "sysbus.h"
 #include "mc146818rtc.h"
 #include "sysemu.h"
+#include <assert.h>
 
 //#define HPET_DEBUG
 #ifdef HPET_DEBUG
@@ -42,6 +43,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*/
@@ -326,28 +330,63 @@ static const VMStateDescription vmstate_hpet = {
     }
 };
 
+static bool hpet_timer_has_tick_backlog(HPETTimer *t)
+{
+    uint64_t 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 irq_count = 0;
     uint64_t period = t->period;
     uint64_t cur_tick = hpet_get_ticks(t->state);
 
+    if (s->driftfix && !t->ticks_not_accounted) {
+        t->ticks_not_accounted = t->prev_period = t->period;
+    }
     if (timer_is_periodic(t) && period != 0) {
         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;
+                irq_count++;
             }
         } else {
             while (hpet_time_after64(cur_tick, t->cmp)) {
                 t->cmp += period;
+                t->ticks_not_accounted += period;
+                irq_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 || irq_count > 1) {
+                    t->irq_rate++;
+                    t->irq_rate = MIN(t->irq_rate, MAX_IRQ_RATE);
+                }
+                if (t->divisor == 0) {
+                    assert(irq_count);
+                }
+                if (irq_count) {
+                    t->divisor = t->irq_rate;
+                }
+                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)) {
@@ -358,7 +397,22 @@ 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 (irq_count) {
+                    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)
@@ -697,6 +751,11 @@ static void hpet_reset(DeviceState *d)
         timer->config |=  0x00000004ULL << 32;
         timer->period = 0ULL;
         timer->wrap_flag = 0;
+
+        timer->prev_period = 0;
+        timer->ticks_not_accounted = 0;
+        timer->irq_rate = 1;
+        timer->divisor = 1;
     }
 
     s->hpet_counter = 0ULL;
-- 
1.6.2.5

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

[Blue Swirl]

On Thu, Apr 28, 2011 at 5:24 PM, Ulrich Obergfell <uobergfe@redhat.com> wrote:
> 'target_get_irq_delivered' and 'target_reset_irq_delivered' contain
> entry addresses of functions that are utilized by update_irq() to
> detect coalesced interrupts. apic code loads these pointers during
> initialization.

I'm not so happy with this approach, but probably then the i386
dependencies can be removed from RTC and it can be compiled only once
for all targets.

> 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 ++++
>  sysemu.h  |    3 +++
>  vl.c      |    3 +++
>  3 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/hw/apic.c b/hw/apic.c
> index a45b57f..eb0f6d9 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -24,6 +24,7 @@
>  #include "sysbus.h"
>  #include "trace.h"
>  #include "kvm.h"
> +#include "sysemu.h"
>
>  /* APIC Local Vector Table */
>  #define APIC_LVT_TIMER   0
> @@ -1143,6 +1144,9 @@ static SysBusDeviceInfo apic_info = {
>
>  static void apic_register_devices(void)
>  {
> +    target_get_irq_delivered = apic_get_irq_delivered;
> +    target_reset_irq_delivered = apic_reset_irq_delivered;
> +
>     sysbus_register_withprop(&apic_info);
>  }
>
> diff --git a/sysemu.h b/sysemu.h
> index 07d85cd..75b0139 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -98,6 +98,9 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f);
>  void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f);
>  int qemu_loadvm_state(QEMUFile *f);
>
> +extern int (*target_get_irq_delivered)(void);
> +extern void (*target_reset_irq_delivered)(void);
> +
>  /* SLIRP */
>  void do_info_slirp(Monitor *mon);
>
> diff --git a/vl.c b/vl.c
> index 0c24e07..7bab315 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -233,6 +233,9 @@ const char *prom_envs[MAX_PROM_ENVS];
>  const char *nvram = NULL;
>  int boot_menu;
>
> +int (*target_get_irq_delivered)(void) = 0;
> +void (*target_reset_irq_delivered)(void) = 0;

Instead of initializing with 0 (should be actually NULL in C), please
define stub functions, which are used by default. Then the users don't
need to check for NULL pointers.

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

On 2011-04-28 20:51, Blue Swirl wrote:
> On Thu, Apr 28, 2011 at 5:24 PM, Ulrich Obergfell <uobergfe@redhat.com> wrote:
>> 'target_get_irq_delivered' and 'target_reset_irq_delivered' contain
>> entry addresses of functions that are utilized by update_irq() to
>> detect coalesced interrupts. apic code loads these pointers during
>> initialization.
> 
> I'm not so happy with this approach, but probably then the i386
> dependencies can be removed from RTC and it can be compiled only once
> for all targets.

This whole series is really the minimalistic approach. The callbacks
defined here must remain a temporary "shortcut". Just like proper
abstraction of periodic tick compensation for reuse in other timers has
to be added later on. And the limitation to edge-triggered legacy HPET
INTs has to be removed.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

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

[Ulrich Obergfell]

> On 2011-04-28 20:51, Blue Swirl wrote:
>> On Thu, Apr 28, 2011 at 5:24 PM, Ulrich Obergfell wrote:
>>> 'target_get_irq_delivered' and 'target_reset_irq_delivered' contain
>>> entry addresses of functions that are utilized by update_irq() to
>>> detect coalesced interrupts. apic code loads these pointers during
>>> initialization.
>>
>> I'm not so happy with this approach, but probably then the i386
>> dependencies can be removed from RTC and it can be compiled only once
>> for all targets.
> 
> This whole series is really the minimalistic approach. The callbacks
> defined here must remain a temporary "shortcut". Just like proper
> abstraction of periodic tick compensation for reuse in other timers has
> to be added later on. And the limitation to edge-triggered legacy HPET
> INTs has to be removed.

Since QEMU doesn't have a generic infrastructure to track interrupt
delivery, I decided to reuse something that is currently available.
The only mechanism that I'm aware of is the one that is utilized by
RTC code ('rtc_td_hack'), i.e. apic_get_irq_delivered() etc.

The changes that would be introduced by part 1/5 and part 4/5 of
this patch series could be replaced if a generic infrastructure
to track interrupt delivery becomes available.

Regards,

Uli

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

[Jan Kiszka]

On 2011-04-29 11:45, Ulrich Obergfell wrote:
> 
>> On 2011-04-28 20:51, Blue Swirl wrote:
>>> On Thu, Apr 28, 2011 at 5:24 PM, Ulrich Obergfell wrote:
>>>> 'target_get_irq_delivered' and 'target_reset_irq_delivered' contain
>>>> entry addresses of functions that are utilized by update_irq() to
>>>> detect coalesced interrupts. apic code loads these pointers during
>>>> initialization.
>>>
>>> I'm not so happy with this approach, but probably then the i386
>>> dependencies can be removed from RTC and it can be compiled only once
>>> for all targets.
>>
>> This whole series is really the minimalistic approach. The callbacks
>> defined here must remain a temporary "shortcut". Just like proper
>> abstraction of periodic tick compensation for reuse in other timers has
>> to be added later on. And the limitation to edge-triggered legacy HPET
>> INTs has to be removed.
> 
> Since QEMU doesn't have a generic infrastructure to track interrupt
> delivery, I decided to reuse something that is currently available.
> The only mechanism that I'm aware of is the one that is utilized by
> RTC code ('rtc_td_hack'), i.e. apic_get_irq_delivered() etc.
> 
> The changes that would be introduced by part 1/5 and part 4/5 of
> this patch series could be replaced if a generic infrastructure
> to track interrupt delivery becomes available.

Right, and I hope you have the resources to continue working on this
topic in the foreseeable future.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

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

[Blue Swirl]

On Fri, Apr 29, 2011 at 12:45 PM, Ulrich Obergfell <uobergfe@redhat.com> wrote:
>
>> On 2011-04-28 20:51, Blue Swirl wrote:
>>> On Thu, Apr 28, 2011 at 5:24 PM, Ulrich Obergfell wrote:
>>>> 'target_get_irq_delivered' and 'target_reset_irq_delivered' contain
>>>> entry addresses of functions that are utilized by update_irq() to
>>>> detect coalesced interrupts. apic code loads these pointers during
>>>> initialization.
>>>
>>> I'm not so happy with this approach, but probably then the i386
>>> dependencies can be removed from RTC and it can be compiled only once
>>> for all targets.
>>
>> This whole series is really the minimalistic approach. The callbacks
>> defined here must remain a temporary "shortcut". Just like proper
>> abstraction of periodic tick compensation for reuse in other timers has
>> to be added later on. And the limitation to edge-triggered legacy HPET
>> INTs has to be removed.
>
> Since QEMU doesn't have a generic infrastructure to track interrupt
> delivery, I decided to reuse something that is currently available.
> The only mechanism that I'm aware of is the one that is utilized by
> RTC code ('rtc_td_hack'), i.e. apic_get_irq_delivered() etc.

I think your approach is slightly better (while not the best possible
one), so it should be used also for RTC.

Some more comments:
- instead of global variables, there could be a setter function (maybe inline)
- pc.h would be a better place instead of sysemu.h

> The changes that would be introduced by part 1/5 and part 4/5 of
> this patch series could be replaced if a generic infrastructure
> to track interrupt delivery becomes available.

OK.

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

[Marcelo Tosatti]

On Thu, Apr 28, 2011 at 04:25:00PM +0200, Ulrich Obergfell wrote:
> 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 |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 61 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/hpet.c b/hw/hpet.c
> index 35466ae..92d5f58 100644
> --- a/hw/hpet.c
> +++ b/hw/hpet.c
> @@ -32,6 +32,7 @@
>  #include "sysbus.h"
>  #include "mc146818rtc.h"
>  #include "sysemu.h"
> +#include <assert.h>
>  
>  //#define HPET_DEBUG
>  #ifdef HPET_DEBUG
> @@ -42,6 +43,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*/
> @@ -326,28 +330,63 @@ static const VMStateDescription vmstate_hpet = {
>      }
>  };
>  
> +static bool hpet_timer_has_tick_backlog(HPETTimer *t)
> +{
> +    uint64_t 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 irq_count = 0;
>      uint64_t period = t->period;
>      uint64_t cur_tick = hpet_get_ticks(t->state);
>  
> +    if (s->driftfix && !t->ticks_not_accounted) {
> +        t->ticks_not_accounted = t->prev_period = t->period;
> +    }
>      if (timer_is_periodic(t) && period != 0) {
>          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;
> +                irq_count++;
>              }
>          } else {
>              while (hpet_time_after64(cur_tick, t->cmp)) {
>                  t->cmp += period;
> +                t->ticks_not_accounted += period;
> +                irq_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 || irq_count > 1) {
> +                    t->irq_rate++;
> +                    t->irq_rate = MIN(t->irq_rate, MAX_IRQ_RATE);
> +                }
> +                if (t->divisor == 0) {
> +                    assert(irq_count);
> +                }
> +                if (irq_count) {
> +                    t->divisor = t->irq_rate;
> +                }
> +                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)) {
> @@ -358,7 +397,22 @@ 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 (irq_count) {
> +                    t->irq_rate++;
> +                    t->irq_rate = MIN(t->irq_rate, MAX_IRQ_RATE);
> +                }
> +            }
> +        }
> +    } else {
> +        update_irq(t, 1);
> +    }
>  }

Hi Ulrich,

Whats prev_period for, since in practice the period will not change
between interrupts (OS programs comparator once, or perhaps twice during
bootup) ?

Other than that, shouldnt reset accounting variables to init state on
write to GLOBAL_ENABLE_CFG / writes to main counter?

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

[Glauber Costa]

On Tue, 2011-05-03 at 16:03 -0300, Marcelo Tosatti wrote:
> On Thu, Apr 28, 2011 at 04:25:00PM +0200, Ulrich Obergfell wrote:
> > 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 |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 61 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/hpet.c b/hw/hpet.c
> > index 35466ae..92d5f58 100644
> > --- a/hw/hpet.c
> > +++ b/hw/hpet.c
> > @@ -32,6 +32,7 @@
> >  #include "sysbus.h"
> >  #include "mc146818rtc.h"
> >  #include "sysemu.h"
> > +#include <assert.h>
> >  
> >  //#define HPET_DEBUG
> >  #ifdef HPET_DEBUG
> > @@ -42,6 +43,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*/
> > @@ -326,28 +330,63 @@ static const VMStateDescription vmstate_hpet = {
> >      }
> >  };
> >  
> > +static bool hpet_timer_has_tick_backlog(HPETTimer *t)
> > +{
> > +    uint64_t 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 irq_count = 0;
> >      uint64_t period = t->period;
> >      uint64_t cur_tick = hpet_get_ticks(t->state);
> >  
> > +    if (s->driftfix && !t->ticks_not_accounted) {
> > +        t->ticks_not_accounted = t->prev_period = t->period;
> > +    }
> >      if (timer_is_periodic(t) && period != 0) {
> >          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;
> > +                irq_count++;
> >              }
> >          } else {
> >              while (hpet_time_after64(cur_tick, t->cmp)) {
> >                  t->cmp += period;
> > +                t->ticks_not_accounted += period;
> > +                irq_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 || irq_count > 1) {
> > +                    t->irq_rate++;
> > +                    t->irq_rate = MIN(t->irq_rate, MAX_IRQ_RATE);
> > +                }
> > +                if (t->divisor == 0) {
> > +                    assert(irq_count);
> > +                }
> > +                if (irq_count) {
> > +                    t->divisor = t->irq_rate;
> > +                }
> > +                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)) {
> > @@ -358,7 +397,22 @@ 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 (irq_count) {
> > +                    t->irq_rate++;
> > +                    t->irq_rate = MIN(t->irq_rate, MAX_IRQ_RATE);
> > +                }
> > +            }
> > +        }
> > +    } else {
> > +        update_irq(t, 1);
> > +    }
> >  }
> 
> Hi Ulrich,
> 
> Whats prev_period for, since in practice the period will not change
> between interrupts (OS programs comparator once, or perhaps twice during
> bootup) ?

Actually, some systems, like Windows 2000-and-something change this
period quite heavily under multimedia workloads.

> Other than that, shouldnt reset accounting variables to init state on
> write to GLOBAL_ENABLE_CFG / writes to main counter?
> 

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

[Marcelo Tosatti]

On Tue, May 03, 2011 at 07:08:25PM -0300, Glauber Costa wrote:
> On Tue, 2011-05-03 at 16:03 -0300, Marcelo Tosatti wrote:
> > On Thu, Apr 28, 2011 at 04:25:00PM +0200, Ulrich Obergfell wrote:
> > > 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 |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  1 files changed, 61 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/hpet.c b/hw/hpet.c
> > > index 35466ae..92d5f58 100644
> > > --- a/hw/hpet.c
> > > +++ b/hw/hpet.c
> > > @@ -32,6 +32,7 @@
> > >  #include "sysbus.h"
> > >  #include "mc146818rtc.h"
> > >  #include "sysemu.h"
> > > +#include <assert.h>
> > >  
> > >  //#define HPET_DEBUG
> > >  #ifdef HPET_DEBUG
> > > @@ -42,6 +43,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*/
> > > @@ -326,28 +330,63 @@ static const VMStateDescription vmstate_hpet = {
> > >      }
> > >  };
> > >  
> > > +static bool hpet_timer_has_tick_backlog(HPETTimer *t)
> > > +{
> > > +    uint64_t 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 irq_count = 0;
> > >      uint64_t period = t->period;
> > >      uint64_t cur_tick = hpet_get_ticks(t->state);
> > >  
> > > +    if (s->driftfix && !t->ticks_not_accounted) {
> > > +        t->ticks_not_accounted = t->prev_period = t->period;
> > > +    }
> > >      if (timer_is_periodic(t) && period != 0) {
> > >          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;
> > > +                irq_count++;
> > >              }
> > >          } else {
> > >              while (hpet_time_after64(cur_tick, t->cmp)) {
> > >                  t->cmp += period;
> > > +                t->ticks_not_accounted += period;
> > > +                irq_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 || irq_count > 1) {
> > > +                    t->irq_rate++;
> > > +                    t->irq_rate = MIN(t->irq_rate, MAX_IRQ_RATE);
> > > +                }
> > > +                if (t->divisor == 0) {
> > > +                    assert(irq_count);
> > > +                }
> > > +                if (irq_count) {
> > > +                    t->divisor = t->irq_rate;
> > > +                }
> > > +                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)) {
> > > @@ -358,7 +397,22 @@ 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 (irq_count) {
> > > +                    t->irq_rate++;
> > > +                    t->irq_rate = MIN(t->irq_rate, MAX_IRQ_RATE);
> > > +                }
> > > +            }
> > > +        }
> > > +    } else {
> > > +        update_irq(t, 1);
> > > +    }
> > >  }
> > 
> > Hi Ulrich,
> > 
> > Whats prev_period for, since in practice the period will not change
> > between interrupts (OS programs comparator once, or perhaps twice during
> > bootup) ?
> 
> Actually, some systems, like Windows 2000-and-something change this
> period quite heavily under multimedia workloads.

I see. Still, using the period at the previous timer handler instance in
the decision whether to inject an interrupt to the guest makes no sense
to me.

> > Other than that, shouldnt reset accounting variables to init state on
> > write to GLOBAL_ENABLE_CFG / writes to main counter?

What i meant to ask here is whether the reinjection state (including
ticks_not_accounted) should be reset whenever a different period is
programmed.

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

[Ulrich Obergfell]

Hi Marcelo,
 
> Whats prev_period for, since in practice the period will not change
> between interrupts (OS programs comparator once, or perhaps twice
> during bootup) ?

'prev_period' is needed if a guest o/s changes the comparator period
'on the fly' (without stopping and restarting the timer).


             guest o/s changes period
               |
  ti(n-1)      |        ti(n)                          ti(n+1)
    |          v          |                              |
    +---------------------+------------------------------+

     <--- prev_period ---> <---------- period ---------->


The idea is that each timer interrupt represents a certain quantum
of time (the comparator period). If a guest o/s changes the period
between timer interrupt 'n-1' and timer interrupt 'n', I think the
new value should not take effect before timer interrupt 'n'. Timer
interrupt 'n' still represents the old/previous quantum, and timer
interrupt 'n+1' represents the new quantum.

Hence, the patch decrements 'ticks_not_accounted' by 'prev_period'
and sets 'prev_period' to 'period' when an interrupt was delivered
to the guest o/s.

+            irq_delivered = update_irq(t, 1);
+            if (irq_delivered) {
+                t->ticks_not_accounted -= t->prev_period;
+                t->prev_period = t->period;
+            } else {

Most of the time 'prev_period' is equal to 'period'. It should only
be different in the scenario shown above.


Regards,

Uli

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

[Marcelo Tosatti]

On Wed, May 04, 2011 at 04:06:59AM -0400, Ulrich Obergfell wrote:
> 
> Hi Marcelo,
>  
> > Whats prev_period for, since in practice the period will not change
> > between interrupts (OS programs comparator once, or perhaps twice
> > during bootup) ?
> 
> 'prev_period' is needed if a guest o/s changes the comparator period
> 'on the fly' (without stopping and restarting the timer).
> 
> 
>              guest o/s changes period
>                |
>   ti(n-1)      |        ti(n)                          ti(n+1)
>     |          v          |                              |
>     +---------------------+------------------------------+
> 
>      <--- prev_period ---> <---------- period ---------->
> 
> 
> The idea is that each timer interrupt represents a certain quantum
> of time (the comparator period). If a guest o/s changes the period
> between timer interrupt 'n-1' and timer interrupt 'n', I think the
> new value should not take effect before timer interrupt 'n'. Timer
> interrupt 'n' still represents the old/previous quantum, and timer
> interrupt 'n+1' represents the new quantum.
> 
> Hence, the patch decrements 'ticks_not_accounted' by 'prev_period'
> and sets 'prev_period' to 'period' when an interrupt was delivered
> to the guest o/s.
> 
> +            irq_delivered = update_irq(t, 1);
> +            if (irq_delivered) {
> +                t->ticks_not_accounted -= t->prev_period;
> +                t->prev_period = t->period;
> +            } else {
> 
> Most of the time 'prev_period' is equal to 'period'. It should only
> be different in the scenario shown above.

OK, makes sense. You should probably reset ticks_not_accounted to zero
on HPET initialization (for example, to avoid miscalibration when
kexec'ing a new kernel).

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

[Glauber Costa]

On Wed, 2011-05-04 at 06:09 -0300, Marcelo Tosatti wrote:
> On Wed, May 04, 2011 at 04:06:59AM -0400, Ulrich Obergfell wrote:
> > 
> > Hi Marcelo,
> >  
> > > Whats prev_period for, since in practice the period will not change
> > > between interrupts (OS programs comparator once, or perhaps twice
> > > during bootup) ?
> > 
> > 'prev_period' is needed if a guest o/s changes the comparator period
> > 'on the fly' (without stopping and restarting the timer).
> > 
> > 
> >              guest o/s changes period
> >                |
> >   ti(n-1)      |        ti(n)                          ti(n+1)
> >     |          v          |                              |
> >     +---------------------+------------------------------+
> > 
> >      <--- prev_period ---> <---------- period ---------->
> > 
> > 
> > The idea is that each timer interrupt represents a certain quantum
> > of time (the comparator period). If a guest o/s changes the period
> > between timer interrupt 'n-1' and timer interrupt 'n', I think the
> > new value should not take effect before timer interrupt 'n'. Timer
> > interrupt 'n' still represents the old/previous quantum, and timer
> > interrupt 'n+1' represents the new quantum.
> > 
> > Hence, the patch decrements 'ticks_not_accounted' by 'prev_period'
> > and sets 'prev_period' to 'period' when an interrupt was delivered
> > to the guest o/s.
> > 
> > +            irq_delivered = update_irq(t, 1);
> > +            if (irq_delivered) {
> > +                t->ticks_not_accounted -= t->prev_period;
> > +                t->prev_period = t->period;
> > +            } else {
> > 
> > Most of the time 'prev_period' is equal to 'period'. It should only
> > be different in the scenario shown above.
> 
> OK, makes sense. You should probably reset ticks_not_accounted to zero
> on HPET initialization (for example, to avoid miscalibration when
> kexec'ing a new kernel).

Everybody resetting the machine in anyway is expected to force devices
to be reinitialized, right ?
I may be wrong, but I was under the impression that kexec would do this
as well. In this case, the reset function should be enough.

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

[Gleb Natapov]

On Wed, May 04, 2011 at 10:36:12AM -0300, Glauber Costa wrote:
> On Wed, 2011-05-04 at 06:09 -0300, Marcelo Tosatti wrote:
> > On Wed, May 04, 2011 at 04:06:59AM -0400, Ulrich Obergfell wrote:
> > > 
> > > Hi Marcelo,
> > >  
> > > > Whats prev_period for, since in practice the period will not change
> > > > between interrupts (OS programs comparator once, or perhaps twice
> > > > during bootup) ?
> > > 
> > > 'prev_period' is needed if a guest o/s changes the comparator period
> > > 'on the fly' (without stopping and restarting the timer).
> > > 
> > > 
> > >              guest o/s changes period
> > >                |
> > >   ti(n-1)      |        ti(n)                          ti(n+1)
> > >     |          v          |                              |
> > >     +---------------------+------------------------------+
> > > 
> > >      <--- prev_period ---> <---------- period ---------->
> > > 
> > > 
> > > The idea is that each timer interrupt represents a certain quantum
> > > of time (the comparator period). If a guest o/s changes the period
> > > between timer interrupt 'n-1' and timer interrupt 'n', I think the
> > > new value should not take effect before timer interrupt 'n'. Timer
> > > interrupt 'n' still represents the old/previous quantum, and timer
> > > interrupt 'n+1' represents the new quantum.
> > > 
> > > Hence, the patch decrements 'ticks_not_accounted' by 'prev_period'
> > > and sets 'prev_period' to 'period' when an interrupt was delivered
> > > to the guest o/s.
> > > 
> > > +            irq_delivered = update_irq(t, 1);
> > > +            if (irq_delivered) {
> > > +                t->ticks_not_accounted -= t->prev_period;
> > > +                t->prev_period = t->period;
> > > +            } else {
> > > 
> > > Most of the time 'prev_period' is equal to 'period'. It should only
> > > be different in the scenario shown above.
> > 
> > OK, makes sense. You should probably reset ticks_not_accounted to zero
> > on HPET initialization (for example, to avoid miscalibration when
> > kexec'ing a new kernel).
> 
> Everybody resetting the machine in anyway is expected to force devices
> to be reinitialized, right ?
> I may be wrong, but I was under the impression that kexec would do this
> as well. In this case, the reset function should be enough.
> 
kexec does not reset a machine. That's the whole point of kexec in
fact.

--
			Gleb.

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

[Glauber Costa]

On Wed, 2011-05-04 at 16:46 +0300, Gleb Natapov wrote:
> On Wed, May 04, 2011 at 10:36:12AM -0300, Glauber Costa wrote:
> > On Wed, 2011-05-04 at 06:09 -0300, Marcelo Tosatti wrote:
> > > On Wed, May 04, 2011 at 04:06:59AM -0400, Ulrich Obergfell wrote:
> > > > 
> > > > Hi Marcelo,
> > > >  
> > > > > Whats prev_period for, since in practice the period will not change
> > > > > between interrupts (OS programs comparator once, or perhaps twice
> > > > > during bootup) ?
> > > > 
> > > > 'prev_period' is needed if a guest o/s changes the comparator period
> > > > 'on the fly' (without stopping and restarting the timer).
> > > > 
> > > > 
> > > >              guest o/s changes period
> > > >                |
> > > >   ti(n-1)      |        ti(n)                          ti(n+1)
> > > >     |          v          |                              |
> > > >     +---------------------+------------------------------+
> > > > 
> > > >      <--- prev_period ---> <---------- period ---------->
> > > > 
> > > > 
> > > > The idea is that each timer interrupt represents a certain quantum
> > > > of time (the comparator period). If a guest o/s changes the period
> > > > between timer interrupt 'n-1' and timer interrupt 'n', I think the
> > > > new value should not take effect before timer interrupt 'n'. Timer
> > > > interrupt 'n' still represents the old/previous quantum, and timer
> > > > interrupt 'n+1' represents the new quantum.
> > > > 
> > > > Hence, the patch decrements 'ticks_not_accounted' by 'prev_period'
> > > > and sets 'prev_period' to 'period' when an interrupt was delivered
> > > > to the guest o/s.
> > > > 
> > > > +            irq_delivered = update_irq(t, 1);
> > > > +            if (irq_delivered) {
> > > > +                t->ticks_not_accounted -= t->prev_period;
> > > > +                t->prev_period = t->period;
> > > > +            } else {
> > > > 
> > > > Most of the time 'prev_period' is equal to 'period'. It should only
> > > > be different in the scenario shown above.
> > > 
> > > OK, makes sense. You should probably reset ticks_not_accounted to zero
> > > on HPET initialization (for example, to avoid miscalibration when
> > > kexec'ing a new kernel).
> > 
> > Everybody resetting the machine in anyway is expected to force devices
> > to be reinitialized, right ?
> > I may be wrong, but I was under the impression that kexec would do this
> > as well. In this case, the reset function should be enough.
> > 
> kexec does not reset a machine. That's the whole point of kexec in
> fact.
Sure thing, but doesn't it force the initialization routine of the devices themselves, without
going through the bios ?

> --
> 			Gleb.

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

[Gleb Natapov]

On Wed, May 04, 2011 at 10:47:40AM -0300, Glauber Costa wrote:
> On Wed, 2011-05-04 at 16:46 +0300, Gleb Natapov wrote:
> > On Wed, May 04, 2011 at 10:36:12AM -0300, Glauber Costa wrote:
> > > On Wed, 2011-05-04 at 06:09 -0300, Marcelo Tosatti wrote:
> > > > On Wed, May 04, 2011 at 04:06:59AM -0400, Ulrich Obergfell wrote:
> > > > > 
> > > > > Hi Marcelo,
> > > > >  
> > > > > > Whats prev_period for, since in practice the period will not change
> > > > > > between interrupts (OS programs comparator once, or perhaps twice
> > > > > > during bootup) ?
> > > > > 
> > > > > 'prev_period' is needed if a guest o/s changes the comparator period
> > > > > 'on the fly' (without stopping and restarting the timer).
> > > > > 
> > > > > 
> > > > >              guest o/s changes period
> > > > >                |
> > > > >   ti(n-1)      |        ti(n)                          ti(n+1)
> > > > >     |          v          |                              |
> > > > >     +---------------------+------------------------------+
> > > > > 
> > > > >      <--- prev_period ---> <---------- period ---------->
> > > > > 
> > > > > 
> > > > > The idea is that each timer interrupt represents a certain quantum
> > > > > of time (the comparator period). If a guest o/s changes the period
> > > > > between timer interrupt 'n-1' and timer interrupt 'n', I think the
> > > > > new value should not take effect before timer interrupt 'n'. Timer
> > > > > interrupt 'n' still represents the old/previous quantum, and timer
> > > > > interrupt 'n+1' represents the new quantum.
> > > > > 
> > > > > Hence, the patch decrements 'ticks_not_accounted' by 'prev_period'
> > > > > and sets 'prev_period' to 'period' when an interrupt was delivered
> > > > > to the guest o/s.
> > > > > 
> > > > > +            irq_delivered = update_irq(t, 1);
> > > > > +            if (irq_delivered) {
> > > > > +                t->ticks_not_accounted -= t->prev_period;
> > > > > +                t->prev_period = t->period;
> > > > > +            } else {
> > > > > 
> > > > > Most of the time 'prev_period' is equal to 'period'. It should only
> > > > > be different in the scenario shown above.
> > > > 
> > > > OK, makes sense. You should probably reset ticks_not_accounted to zero
> > > > on HPET initialization (for example, to avoid miscalibration when
> > > > kexec'ing a new kernel).
> > > 
> > > Everybody resetting the machine in anyway is expected to force devices
> > > to be reinitialized, right ?
> > > I may be wrong, but I was under the impression that kexec would do this
> > > as well. In this case, the reset function should be enough.
> > > 
> > kexec does not reset a machine. That's the whole point of kexec in
> > fact.
> Sure thing, but doesn't it force the initialization routine of the devices themselves, without
> going through the bios ?
> 
It just starts new kernel. New kernel's init runs as usual and
re-initialize everything. No it doesn't go through the BIOS. What for?
Actually I happily use kexec on IBM blade server where BIOS run takes no
less then 5 minutes and kexec reboots instantly.

--
			Gleb.

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

[Ulrich Obergfell]

Hi Marcelo,
 
> Other than that, shouldnt reset accounting variables to init state on
> write to GLOBAL_ENABLE_CFG / writes to main counter?

I'd suggest to initialize/reset the driftfix-related fields in the
'HPETTimer' structure (including the backlog of unaccounted ticks)
in the following situations.


- When the guest o/s sets the 'CFG_ENABLE' bit (overall enable) in
  the General Configuration Register.

  This is the place in hpet_ram_writel():

        case HPET_CFG:
            ...
            if (activating_bit(old_val, new_val, HPET_CFG_ENABLE)) {
                ...
                for (i = 0; i < s->num_timers; i++) {
                    if ((&s->timer[i])->cmp != ~0ULL) {
                        // initialize / reset fields here
                        hpet_set_timer(&s->timer[i]);


- When the guest o/s sets the 'TN_ENABLE' bit (timer N interrupt
  enable) in the Timer N Configuration and Capabilities Register.

  This is the place in hpet_ram_writel():

        case HPET_TN_CFG:
            ...
            if (activating_bit(old_val, new_val, HPET_TN_ENABLE)) {
                // initialize / reset fields here
                hpet_set_timer(timer);


This should cover cases such as ...

- first time initialization of HPET & timers during guest o/s boot.
  (includes kexec and reboot)

- if a guest o/s stops and restarts the timer.
  (for example, to change the comparator register value or
   to switch a timer between periodic mode and non-periodic mode)


Regards,

Uli

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

[Marcelo Tosatti]

On Thu, May 05, 2011 at 04:07:19AM -0400, Ulrich Obergfell wrote:
> 
> Hi Marcelo,
>  
> > Other than that, shouldnt reset accounting variables to init state on
> > write to GLOBAL_ENABLE_CFG / writes to main counter?
> 
> I'd suggest to initialize/reset the driftfix-related fields in the
> 'HPETTimer' structure (including the backlog of unaccounted ticks)
> in the following situations.
> 
> 
> - When the guest o/s sets the 'CFG_ENABLE' bit (overall enable) in
>   the General Configuration Register.
> 
>   This is the place in hpet_ram_writel():
> 
>         case HPET_CFG:
>             ...
>             if (activating_bit(old_val, new_val, HPET_CFG_ENABLE)) {
>                 ...
>                 for (i = 0; i < s->num_timers; i++) {
>                     if ((&s->timer[i])->cmp != ~0ULL) {
>                         // initialize / reset fields here
>                         hpet_set_timer(&s->timer[i]);
> 
> 
> - When the guest o/s sets the 'TN_ENABLE' bit (timer N interrupt
>   enable) in the Timer N Configuration and Capabilities Register.
> 
>   This is the place in hpet_ram_writel():
> 
>         case HPET_TN_CFG:
>             ...
>             if (activating_bit(old_val, new_val, HPET_TN_ENABLE)) {
>                 // initialize / reset fields here
>                 hpet_set_timer(timer);
> 
> 
> This should cover cases such as ...
> 
> - first time initialization of HPET & timers during guest o/s boot.
>   (includes kexec and reboot)
> 
> - if a guest o/s stops and restarts the timer.
>   (for example, to change the comparator register value or
>    to switch a timer between periodic mode and non-periodic mode)
> 
> 
> Regards,
> 
> Uli

Uli, looks good.