[PATCH 0/4 v2] x86/hvm: RTC periodic timer adjustments

[PATCH 0/4 v2] x86/hvm: RTC periodic timer adjustments

[Tim Deegan]

With these four patches, the time drift that was happening on Windows XP
guests goes away, at least in my testing.  3/4 is probably the most
important (with the other three, I still see slow drift), but the others
all seem like a good idea to me.

Changes since v1:
 - moved the irq logic patch to the end of the queue so the others
   can be applied independently
 - in the irq logic, keep the strobing behaviour as before 
 - store period_code instead of period and pt_active

Cheers,

Tim.

[PATCH 1/4] x86/hvm: Run the RTC periodic timer on a consistent time series.

[Tim Deegan]

When the RTC periodic timer gets restarted, align it to the VM's boot
time, not to whatever time it is now.  Otherwise every read of REG_C
will restart the current period

Signed-off-by: Tim Deegan <tim@xen.org>
Acked-by: Keir Fraser <keir@xen.org>
---
 xen/arch/x86/hvm/rtc.c        |    6 ++++--
 xen/include/asm-x86/hvm/vpt.h |    3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index c1e09d8..e9aa81a 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -80,7 +80,7 @@ void rtc_periodic_interrupt(void *opaque)
  * RTC_RATE_SELECT settings */
 static void rtc_timer_update(RTCState *s)
 {
-    int period_code, period;
+    int period_code, period, delta;
     struct vcpu *v = vrtc_vcpu(s);
 
     ASSERT(spin_is_locked(&s->lock));
@@ -98,7 +98,8 @@ static void rtc_timer_update(RTCState *s)
         {
             period = 1 << (period_code - 1); /* period in 32 Khz cycles */
             period = DIV_ROUND(period * 1000000000ULL, 32768); /* in ns */
-            create_periodic_time(v, &s->pt, period, period, RTC_IRQ, NULL, s);
+            delta = period - ((NOW() - s->start_time) % period);
+            create_periodic_time(v, &s->pt, delta, period, RTC_IRQ, NULL, s);
             break;
         }
         /* fall through */
@@ -740,6 +741,7 @@ void rtc_init(struct domain *d)
     s->hw.cmos_data[RTC_REG_D] = RTC_VRT;
 
     s->current_tm = gmtime(get_localtime(d));
+    s->start_time = NOW();
 
     rtc_copy_date(s);
 
diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
index c75297b..2e9c7d2 100644
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -104,8 +104,9 @@ typedef struct RTCState {
     struct hvm_hw_rtc hw;
     /* RTC's idea of the current time */
     struct tm current_tm;
+    /* periodic timer */
+    s_time_t start_time;
     /* second update */
-    int64_t next_second_time;
     struct periodic_time pt;
     /* update-ended timer */
     struct timer update_timer;
-- 
1.7.10.4

[PATCH 2/4] x86/hvm: Avoid needlessly resetting the periodic timer.

[Tim Deegan]

Signed-off-by: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/hvm/rtc.c        |   18 ++++++++++++++----
 xen/include/asm-x86/hvm/vpt.h |   10 +++++-----
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index e9aa81a..8d9195f 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -66,7 +66,10 @@ void rtc_periodic_interrupt(void *opaque)
 
     spin_lock(&s->lock);
     if ( s->hw.cmos_data[RTC_REG_C] & RTC_PF )
+    {
         destroy_periodic_time(&s->pt);
+        s->pt_code = 0;
+    }
     else
     {
         s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
@@ -96,15 +99,21 @@ static void rtc_timer_update(RTCState *s)
     case RTC_REF_CLCK_4MHZ:
         if ( period_code != 0 )
         {
-            period = 1 << (period_code - 1); /* period in 32 Khz cycles */
-            period = DIV_ROUND(period * 1000000000ULL, 32768); /* in ns */
-            delta = period - ((NOW() - s->start_time) % period);
-            create_periodic_time(v, &s->pt, delta, period, RTC_IRQ, NULL, s);
+            if ( period_code != s->pt_code )
+            {
+                s->pt_code = period_code;
+                period = 1 << (period_code - 1); /* period in 32 Khz cycles */
+                period = DIV_ROUND(period * 1000000000ULL, 32768); /* in ns */
+                delta = period - ((NOW() - s->start_time) % period);
+                create_periodic_time(v, &s->pt, delta, period,
+                                     RTC_IRQ, NULL, s);
+            }
             break;
         }
         /* fall through */
     default:
         destroy_periodic_time(&s->pt);
+        s->pt_code = 0;
         break;
     }
 }
@@ -716,6 +725,7 @@ void rtc_reset(struct domain *d)
     RTCState *s = domain_vrtc(d);
 
     destroy_periodic_time(&s->pt);
+    s->pt_code = 0;
     s->pt.source = PTSRC_isa;
 }
 
diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
index 2e9c7d2..ea9df42 100644
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -104,16 +104,16 @@ typedef struct RTCState {
     struct hvm_hw_rtc hw;
     /* RTC's idea of the current time */
     struct tm current_tm;
-    /* periodic timer */
-    s_time_t start_time;
-    /* second update */
-    struct periodic_time pt;
     /* update-ended timer */
     struct timer update_timer;
     struct timer update_timer2;
+    uint64_t next_update_time;
     /* alarm timer */
     struct timer alarm_timer;
-    uint64_t next_update_time;
+    /* periodic timer */
+    struct periodic_time pt;
+    s_time_t start_time;
+    int pt_code;
     uint32_t use_timer;
     spinlock_t lock;
 } RTCState;
-- 
1.7.10.4

[PATCH 3/4] x86/hvm: Let the guest miss a few ticks before resetting the timer.

[Tim Deegan]

Signed-off-by: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/hvm/rtc.c        |   15 +++++++++------
 xen/include/asm-x86/hvm/vpt.h |    1 +
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index 8d9195f..88cacd1 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -65,17 +65,18 @@ void rtc_periodic_interrupt(void *opaque)
     RTCState *s = opaque;
 
     spin_lock(&s->lock);
-    if ( s->hw.cmos_data[RTC_REG_C] & RTC_PF )
-    {
-        destroy_periodic_time(&s->pt);
-        s->pt_code = 0;
-    }
-    else
+    if ( !(s->hw.cmos_data[RTC_REG_C] & RTC_PF) )
     {
         s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
         if ( s->hw.cmos_data[RTC_REG_B] & RTC_PIE )
             rtc_toggle_irq(s);
     }
+    else if ( ++(s->pt_dead_ticks) >= 10 )
+    {
+        /* VM is ignoring its RTC; no point in running the timer */
+        destroy_periodic_time(&s->pt);
+        s->pt_code = 0;
+    }
     spin_unlock(&s->lock);
 }
 
@@ -88,6 +89,8 @@ static void rtc_timer_update(RTCState *s)
 
     ASSERT(spin_is_locked(&s->lock));
 
+    s->pt_dead_ticks = 0;
+
     period_code = s->hw.cmos_data[RTC_REG_A] & RTC_RATE_SELECT;
     switch ( s->hw.cmos_data[RTC_REG_A] & RTC_DIV_CTL )
     {
diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
index ea9df42..765fbb7 100644
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -114,6 +114,7 @@ typedef struct RTCState {
     struct periodic_time pt;
     s_time_t start_time;
     int pt_code;
+    uint8_t pt_dead_ticks;
     uint32_t use_timer;
     spinlock_t lock;
 } RTCState;
-- 
1.7.10.4

[PATCH 4/4] x86/hvm: Centralize and simplify the RTC IRQ logic.

[Tim Deegan]

This keeps the behaviour of strobing the IRQ line every time any RTC
interrupt source is raised.  I rather suspect (based on the behaviour
of the MC146818A RTC) that we ought to be suppressing all subsequent
interrupts whenever RTC_IRQF is set, but this way avoids making
guest-visible changes.

Signed-off-by: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/hvm/rtc.c |   40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index 88cacd1..26ab12c 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -50,14 +50,25 @@ static void rtc_set_time(RTCState *s);
 static inline int from_bcd(RTCState *s, int a);
 static inline int convert_hour(RTCState *s, int hour);
 
-static void rtc_toggle_irq(RTCState *s)
+static void rtc_update_irq(RTCState *s)
 {
     struct domain *d = vrtc_domain(s);
+    uint8_t irqf;
 
     ASSERT(spin_is_locked(&s->lock));
-    s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
+
+    /* IRQ is raised if any source is both raised & enabled */
+    irqf = (s->hw.cmos_data[RTC_REG_B]
+            & s->hw.cmos_data[RTC_REG_C]
+            & (RTC_PF|RTC_AF|RTC_UF))
+        ? RTC_IRQF : 0;
+
+    s->hw.cmos_data[RTC_REG_C] &= ~RTC_IRQF;
+    s->hw.cmos_data[RTC_REG_C] |= irqf;
+
     hvm_isa_irq_deassert(d, RTC_IRQ);
-    hvm_isa_irq_assert(d, RTC_IRQ);
+    if ( irqf )
+        hvm_isa_irq_assert(d, RTC_IRQ);
 }
 
 void rtc_periodic_interrupt(void *opaque)
@@ -68,8 +79,7 @@ void rtc_periodic_interrupt(void *opaque)
     if ( !(s->hw.cmos_data[RTC_REG_C] & RTC_PF) )
     {
         s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
-        if ( s->hw.cmos_data[RTC_REG_B] & RTC_PIE )
-            rtc_toggle_irq(s);
+        rtc_update_irq(s);
     }
     else if ( ++(s->pt_dead_ticks) >= 10 )
     {
@@ -187,8 +197,7 @@ static void rtc_update_timer2(void *opaque)
     {
         s->hw.cmos_data[RTC_REG_C] |= RTC_UF;
         s->hw.cmos_data[RTC_REG_A] &= ~RTC_UIP;
-        if ((s->hw.cmos_data[RTC_REG_B] & RTC_UIE))
-            rtc_toggle_irq(s);
+        rtc_update_irq(s);
         check_update_timer(s);
     }
     spin_unlock(&s->lock);
@@ -378,9 +387,7 @@ static void rtc_alarm_cb(void *opaque)
     if (!(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
     {
         s->hw.cmos_data[RTC_REG_C] |= RTC_AF;
-        /* alarm interrupt */
-        if (s->hw.cmos_data[RTC_REG_B] & RTC_AIE)
-            rtc_toggle_irq(s);
+        rtc_update_irq(s);
         alarm_timer_update(s);
     }
     spin_unlock(&s->lock);
@@ -390,7 +397,7 @@ static int rtc_ioport_write(void *opaque, uint32_t addr, uint32_t data)
 {
     RTCState *s = opaque;
     struct domain *d = vrtc_domain(s);
-    uint32_t orig, mask;
+    uint32_t orig;
 
     spin_lock(&s->lock);
 
@@ -464,15 +471,8 @@ static int rtc_ioport_write(void *opaque, uint32_t addr, uint32_t data)
         /*
          * If the interrupt is already set when the interrupt becomes
          * enabled, raise an interrupt immediately.
-         * NB: RTC_{A,P,U}IE == RTC_{A,P,U}F respectively.
          */
-        for ( mask = RTC_UIE; mask <= RTC_PIE; mask <<= 1 )
-            if ( (data & mask) && !(orig & mask) &&
-                 (s->hw.cmos_data[RTC_REG_C] & mask) )
-            {
-                rtc_toggle_irq(s);
-                break;
-            }
+        rtc_update_irq(s);
         s->hw.cmos_data[RTC_REG_B] = data;
         if ( (data ^ orig) & RTC_SET )
             check_update_timer(s);
@@ -628,8 +628,8 @@ static uint32_t rtc_ioport_read(RTCState *s, uint32_t addr)
         break;
     case RTC_REG_C:
         ret = s->hw.cmos_data[s->hw.cmos_index];
-        hvm_isa_irq_deassert(vrtc_domain(s), RTC_IRQ);
         s->hw.cmos_data[RTC_REG_C] = 0x00;
+        rtc_update_irq(s);
         check_update_timer(s);
         alarm_timer_update(s);
         rtc_timer_update(s);
-- 
1.7.10.4

Re: [PATCH 0/4 v2] x86/hvm: RTC periodic timer adjustments

[Jan Beulich]

>>> On 28.03.13 at 17:09, Tim Deegan <tim@xen.org> wrote:
> With these four patches, the time drift that was happening on Windows XP
> guests goes away, at least in my testing.  3/4 is probably the most
> important (with the other three, I still see slow drift), but the others
> all seem like a good idea to me.
> 
> Changes since v1:
>  - moved the irq logic patch to the end of the queue so the others
>    can be applied independently
>  - in the irq logic, keep the strobing behaviour as before 
>  - store period_code instead of period and pt_active

Acked-by: Jan Beulich <jbeulich@suse.com>

Of course it would be nice if Suravee could test this in his setup,
so we'd have another data point on the positive effects this has.

Jan

Re: [PATCH 0/4 v2] x86/hvm: RTC periodic timer adjustments

[Keir Fraser]

On 28/03/2013 16:09, "Tim Deegan" <tim@xen.org> wrote:

> With these four patches, the time drift that was happening on Windows XP
> guests goes away, at least in my testing.  3/4 is probably the most
> important (with the other three, I still see slow drift), but the others
> all seem like a good idea to me.
> 
> Changes since v1:
>  - moved the irq logic patch to the end of the queue so the others
>    can be applied independently
>  - in the irq logic, keep the strobing behaviour as before
>  - store period_code instead of period and pt_active

Acked-by: Keir Fraser <keir@xen.org>

> Cheers,
> 
> Tim.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 0/4 v2] x86/hvm: RTC periodic timer adjustments

[Suravee Suthikulanit]

On 3/28/2013 11:18 AM, Jan Beulich wrote:
>>>> On 28.03.13 at 17:09, Tim Deegan <tim@xen.org> wrote:
>> With these four patches, the time drift that was happening on Windows XP
>> guests goes away, at least in my testing.  3/4 is probably the most
>> important (with the other three, I still see slow drift), but the others
>> all seem like a good idea to me.
>>
>> Changes since v1:
>>   - moved the irq logic patch to the end of the queue so the others
>>     can be applied independently
>>   - in the irq logic, keep the strobing behaviour as before
>>   - store period_code instead of period and pt_active
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
> Of course it would be nice if Suravee could test this in his setup,
> so we'd have another data point on the positive effects this has.
>
> Jan
I have tested this and it seems to stop drifting on my same test box 
that observed the issue before.

Suravee