xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH, RFC v2] x86/HVM: assorted RTC emulation adjustments (was Re: Big Bug:Time in VM goes slower...)
@ 2012-08-14  9:51 Jan Beulich
  2012-08-15 14:07 ` tupeng212
  0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2012-08-14  9:51 UTC (permalink / raw)
  To: Yang Z Zhang, Keir Fraser, Tim Deegan; +Cc: tupeng212, xen-devel

Below/attached a second draft of a patch to fix not only this
issue, but a few more with the RTC emulation.

Keir, Tim, Yang, others - the change to xen/arch/x86/hvm/vpt.c really
looks more like a hack than a solution, but I don't see another
way without much more intrusive changes. The point is that we
want the RTC code to decide whether to generate an interrupt
(so that RTC_PF can become set correctly even without RTC_PIE
getting enabled by the guest).

Additionally I wonder whether alarm_timer_update() shouldn't
bail on non-conforming RTC_*_ALARM values (as those would
never match the values they get compared against, whereas
with the current way of handling this they would appear to
match - i.e. set RTC_AF and possibly generate an interrupt -
some other point in time). I realize the behavior here may not
be precisely specified, but the specification saying "the current
time has matched the alarm time" means to me a value by value
comparison, which implies that non-conforming values would
never match (since non-conforming current time values could
get replaced at any time by the hardware due to overflow
detection).

Jan

- don't call rtc_timer_update() on REG_A writes when the value didn't
  change (doing the call always was reported to cause wall clock time
  lagging with the JVM running on Windows)
- don't call rtc_timer_update() on REG_B writes at all
- only call alarm_timer_update() on REG_B writes when relevant bits
  change
- only call check_update_timer() on REG_B writes when SET changes
- instead properly handle AF and PF when the guest is not also setting
  AIE/PIE respectively (for UF this was already the case, only a
  comment was slightly inaccurate)
- raise the RTC IRQ not only when UIE gets set while UF was already
  set, but generalize this to cover AIE and PIE as well
- properly mask off bit 7 when retrieving the hour values in
  alarm_timer_update(), and properly use RTC_HOURS_ALARM's bit 7 when
  converting from 12- to 24-hour value
- also handle the two other possible clock bases
- use RTC_* names in a couple of places where literal numbers were used
  so far

--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -50,11 +50,24 @@ 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_periodic_cb(struct vcpu *v, void *opaque)
+static void rtc_toggle_irq(RTCState *s)
+{
+    struct domain *d = vrtc_domain(s);
+
+    ASSERT(spin_is_locked(&s->lock));
+    s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
+    hvm_isa_irq_deassert(d, RTC_IRQ);
+    hvm_isa_irq_assert(d, RTC_IRQ);
+}
+
+void rtc_periodic_interrupt(void *opaque)
 {
     RTCState *s = opaque;
+
     spin_lock(&s->lock);
-    s->hw.cmos_data[RTC_REG_C] |= 0xc0;
+    s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
+    if ( s->hw.cmos_data[RTC_REG_B] & RTC_PIE )
+        rtc_toggle_irq(s);
     spin_unlock(&s->lock);
 }
 
@@ -68,19 +81,25 @@ static void rtc_timer_update(RTCState *s
     ASSERT(spin_is_locked(&s->lock));
 
     period_code = s->hw.cmos_data[RTC_REG_A] & RTC_RATE_SELECT;
-    if ( (period_code != 0) && (s->hw.cmos_data[RTC_REG_B] & RTC_PIE) )
+    switch ( s->hw.cmos_data[RTC_REG_A] & RTC_DIV_CTL )
     {
-        if ( period_code <= 2 )
+    case RTC_REF_CLCK_32KHZ:
+        if ( (period_code != 0) && (period_code <= 2) )
             period_code += 7;
-
-        period = 1 << (period_code - 1); /* period in 32 Khz cycles */
-        period = DIV_ROUND((period * 1000000000ULL), 32768); /* period in ns */
-        create_periodic_time(v, &s->pt, period, period, RTC_IRQ,
-                             rtc_periodic_cb, s);
-    }
-    else
-    {
+        /* fall through */
+    case RTC_REF_CLCK_1MHZ:
+    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 */
+            create_periodic_time(v, &s->pt, period, period, RTC_IRQ, NULL, s);
+            break;
+        }
+        /* fall through */
+    default:
         destroy_periodic_time(&s->pt);
+        break;
     }
 }
 
@@ -102,7 +121,7 @@ static void check_update_timer(RTCState 
         guest_usec = get_localtime_us(d) % USEC_PER_SEC;
         if (guest_usec >= (USEC_PER_SEC - 244))
         {
-            /* RTC is in update cycle when enabling UIE */
+            /* RTC is in update cycle */
             s->hw.cmos_data[RTC_REG_A] |= RTC_UIP;
             next_update_time = (USEC_PER_SEC - guest_usec) * NS_PER_USEC;
             expire_time = NOW() + next_update_time;
@@ -144,7 +163,6 @@ static void rtc_update_timer(void *opaqu
 static void rtc_update_timer2(void *opaque)
 {
     RTCState *s = opaque;
-    struct domain *d = vrtc_domain(s);
 
     spin_lock(&s->lock);
     if (!(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
@@ -152,11 +170,7 @@ static void rtc_update_timer2(void *opaq
         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))
-        {
-            s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
-            hvm_isa_irq_deassert(d, RTC_IRQ);
-            hvm_isa_irq_assert(d, RTC_IRQ);
-        }
+            rtc_toggle_irq(s);
         check_update_timer(s);
     }
     spin_unlock(&s->lock);
@@ -175,21 +189,18 @@ static void alarm_timer_update(RTCState 
 
     stop_timer(&s->alarm_timer);
 
-    if ((s->hw.cmos_data[RTC_REG_B] & RTC_AIE) &&
-            !(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
+    if ( !(s->hw.cmos_data[RTC_REG_B] & RTC_SET) )
     {
         s->current_tm = gmtime(get_localtime(d));
         rtc_copy_date(s);
 
         alarm_sec = from_bcd(s, s->hw.cmos_data[RTC_SECONDS_ALARM]);
         alarm_min = from_bcd(s, s->hw.cmos_data[RTC_MINUTES_ALARM]);
-        alarm_hour = from_bcd(s, s->hw.cmos_data[RTC_HOURS_ALARM]);
-        alarm_hour = convert_hour(s, alarm_hour);
+        alarm_hour = convert_hour(s, s->hw.cmos_data[RTC_HOURS_ALARM]);
 
         cur_sec = from_bcd(s, s->hw.cmos_data[RTC_SECONDS]);
         cur_min = from_bcd(s, s->hw.cmos_data[RTC_MINUTES]);
-        cur_hour = from_bcd(s, s->hw.cmos_data[RTC_HOURS]);
-        cur_hour = convert_hour(s, cur_hour);
+        cur_hour = convert_hour(s, s->hw.cmos_data[RTC_HOURS]);
 
         next_update_time = USEC_PER_SEC - (get_localtime_us(d) % USEC_PER_SEC);
         next_update_time = next_update_time * NS_PER_USEC + NOW();
@@ -343,7 +354,6 @@ static void alarm_timer_update(RTCState 
 static void rtc_alarm_cb(void *opaque)
 {
     RTCState *s = opaque;
-    struct domain *d = vrtc_domain(s);
 
     spin_lock(&s->lock);
     if (!(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
@@ -351,11 +361,7 @@ static void rtc_alarm_cb(void *opaque)
         s->hw.cmos_data[RTC_REG_C] |= RTC_AF;
         /* alarm interrupt */
         if (s->hw.cmos_data[RTC_REG_B] & RTC_AIE)
-        {
-            s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
-            hvm_isa_irq_deassert(d, RTC_IRQ);
-            hvm_isa_irq_assert(d, RTC_IRQ);
-        }
+            rtc_toggle_irq(s);
         alarm_timer_update(s);
     }
     spin_unlock(&s->lock);
@@ -365,6 +371,7 @@ static int rtc_ioport_write(void *opaque
 {
     RTCState *s = opaque;
     struct domain *d = vrtc_domain(s);
+    uint32_t orig, mask;
 
     spin_lock(&s->lock);
 
@@ -382,6 +389,7 @@ static int rtc_ioport_write(void *opaque
         return 0;
     }
 
+    orig = s->hw.cmos_data[s->hw.cmos_index];
     switch ( s->hw.cmos_index )
     {
     case RTC_SECONDS_ALARM:
@@ -405,9 +413,9 @@ static int rtc_ioport_write(void *opaque
         break;
     case RTC_REG_A:
         /* UIP bit is read only */
-        s->hw.cmos_data[RTC_REG_A] = (data & ~RTC_UIP) |
-            (s->hw.cmos_data[RTC_REG_A] & RTC_UIP);
-        rtc_timer_update(s);
+        s->hw.cmos_data[RTC_REG_A] = (data & ~RTC_UIP) | (orig & RTC_UIP);
+        if ( (data ^ orig) & (RTC_RATE_SELECT | RTC_DIV_CTL) )
+            rtc_timer_update(s);
         break;
     case RTC_REG_B:
         if ( data & RTC_SET )
@@ -415,7 +423,7 @@ static int rtc_ioport_write(void *opaque
             /* set mode: reset UIP mode */
             s->hw.cmos_data[RTC_REG_A] &= ~RTC_UIP;
             /* adjust cmos before stopping */
-            if (!(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
+            if (!(orig & RTC_SET))
             {
                 s->current_tm = gmtime(get_localtime(d));
                 rtc_copy_date(s);
@@ -424,21 +432,26 @@ static int rtc_ioport_write(void *opaque
         else
         {
             /* if disabling set mode, update the time */
-            if ( s->hw.cmos_data[RTC_REG_B] & RTC_SET )
+            if ( orig & RTC_SET )
                 rtc_set_time(s);
         }
-        /* if the interrupt is already set when the interrupt become
-         * enabled, raise an interrupt immediately*/
-        if ((data & RTC_UIE) && !(s->hw.cmos_data[RTC_REG_B] & RTC_UIE))
-            if (s->hw.cmos_data[RTC_REG_C] & RTC_UF)
+        /*
+         * 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) )
             {
-                hvm_isa_irq_deassert(d, RTC_IRQ);
-                hvm_isa_irq_assert(d, RTC_IRQ);
+                rtc_toggle_irq(s);
+                break;
             }
         s->hw.cmos_data[RTC_REG_B] = data;
-        rtc_timer_update(s);
-        check_update_timer(s);
-        alarm_timer_update(s);
+        if ( (data ^ orig) & RTC_SET )
+            check_update_timer(s);
+        if ( (data ^ orig) & (RTC_24H | RTC_DM_BINARY | RTC_SET) )
+            alarm_timer_update(s);
         break;
     case RTC_REG_C:
     case RTC_REG_D:
@@ -453,7 +466,7 @@ static int rtc_ioport_write(void *opaque
 
 static inline int to_bcd(RTCState *s, int a)
 {
-    if ( s->hw.cmos_data[RTC_REG_B] & 0x04 )
+    if ( s->hw.cmos_data[RTC_REG_B] & RTC_DM_BINARY )
         return a;
     else
         return ((a / 10) << 4) | (a % 10);
@@ -461,7 +474,7 @@ static inline int to_bcd(RTCState *s, in
 
 static inline int from_bcd(RTCState *s, int a)
 {
-    if ( s->hw.cmos_data[RTC_REG_B] & 0x04 )
+    if ( s->hw.cmos_data[RTC_REG_B] & RTC_DM_BINARY )
         return a;
     else
         return ((a >> 4) * 10) + (a & 0x0f);
@@ -469,12 +482,14 @@ static inline int from_bcd(RTCState *s, 
 
 /* Hours in 12 hour mode are in 1-12 range, not 0-11.
  * So we need convert it before using it*/
-static inline int convert_hour(RTCState *s, int hour)
+static inline int convert_hour(RTCState *s, int raw)
 {
+    int hour = from_bcd(s, raw & 0x7f);
+
     if (!(s->hw.cmos_data[RTC_REG_B] & RTC_24H))
     {
         hour %= 12;
-        if (s->hw.cmos_data[RTC_HOURS] & 0x80)
+        if (raw & 0x80)
             hour += 12;
     }
     return hour;
@@ -493,8 +508,7 @@ static void rtc_set_time(RTCState *s)
     
     tm->tm_sec = from_bcd(s, s->hw.cmos_data[RTC_SECONDS]);
     tm->tm_min = from_bcd(s, s->hw.cmos_data[RTC_MINUTES]);
-    tm->tm_hour = from_bcd(s, s->hw.cmos_data[RTC_HOURS] & 0x7f);
-    tm->tm_hour = convert_hour(s, tm->tm_hour);
+    tm->tm_hour = convert_hour(s, s->hw.cmos_data[RTC_HOURS]);
     tm->tm_wday = from_bcd(s, s->hw.cmos_data[RTC_DAY_OF_WEEK]);
     tm->tm_mday = from_bcd(s, s->hw.cmos_data[RTC_DAY_OF_MONTH]);
     tm->tm_mon = from_bcd(s, s->hw.cmos_data[RTC_MONTH]) - 1;
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -22,6 +22,7 @@
 #include <asm/hvm/vpt.h>
 #include <asm/event.h>
 #include <asm/apic.h>
+#include <asm/mc146818rtc.h>
 
 #define mode_is(d, name) \
     ((d)->arch.hvm_domain.params[HVM_PARAM_TIMER_MODE] == HVMPTM_##name)
@@ -218,6 +219,7 @@ void pt_update_irq(struct vcpu *v)
     struct periodic_time *pt, *temp, *earliest_pt = NULL;
     uint64_t max_lag = -1ULL;
     int irq, is_lapic;
+    void *pt_priv;
 
     spin_lock(&v->arch.hvm_vcpu.tm_lock);
 
@@ -251,13 +253,14 @@ void pt_update_irq(struct vcpu *v)
     earliest_pt->irq_issued = 1;
     irq = earliest_pt->irq;
     is_lapic = (earliest_pt->source == PTSRC_lapic);
+    pt_priv = earliest_pt->priv;
 
     spin_unlock(&v->arch.hvm_vcpu.tm_lock);
 
     if ( is_lapic )
-    {
         vlapic_set_irq(vcpu_vlapic(v), irq, 0);
-    }
+    else if ( irq == RTC_IRQ )
+        rtc_periodic_interrupt(pt_priv);
     else
     {
         hvm_isa_irq_deassert(v->domain, irq);
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -181,6 +181,7 @@ void rtc_migrate_timers(struct vcpu *v);
 void rtc_deinit(struct domain *d);
 void rtc_reset(struct domain *d);
 void rtc_update_clock(struct domain *d);
+void rtc_periodic_interrupt(void *);
 
 void pmtimer_init(struct vcpu *v);
 void pmtimer_deinit(struct domain *d);

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH, RFC v2] x86/HVM: assorted RTC emulation adjustments (was Re: Big Bug:Time in VM goes slower...)
  2012-08-14  9:51 [PATCH, RFC v2] x86/HVM: assorted RTC emulation adjustments (was Re: Big Bug:Time in VM goes slower...) Jan Beulich
@ 2012-08-15 14:07 ` tupeng212
  2012-08-15 14:12   ` tupeng212
  2012-08-15 15:00   ` Jan Beulich
  0 siblings, 2 replies; 8+ messages in thread
From: tupeng212 @ 2012-08-15 14:07 UTC (permalink / raw)
  To: Jan Beulich, Yang Z Zhang, Keir Fraser, Tim Deegan; +Cc: xen-devel


[-- Attachment #1.1: Type: text/plain, Size: 15306 bytes --]

Hi, Jan:
I am sorry I really don't have much time to try a test of your patch, and it is not convenient
for me to have a try. For the version I have been using is xen4.0.x, and your patch is based on 
the latest version xen4.2.x.(I have never complied the unstable one), so I merged your patch to my 
xen4.0.x, still couldn't find the two functions below:
 static void rtc_update_timer2(void *opaque) 
 static void rtc_alarm_cb(void *opaque) 
so I didn't merge the two functions which contains a rtc_toggle_irq() .

The results for me were these:
1 In my real application environment, it worked very well in the former 5mins, much better than before,
 but at last it lagged again. I don't know whether it belongs to the two missed functions. I lack the 
 ability to figure them out.

2 When I tested my test program which I provided days before, it worked very well, maybe the program doesn't 
emulate the real environment due to the same setting rate, so I modified this program as which in the attachment.
if you are more convenient, you can help me to have a look of it.
And I have a opinion, because our product is based on Version Xen4.0.x, if you have enough time, can you write 
another patch based http://xenbits.xen.org/hg/xen-4.0-testing.hg/ for me, thank you very much!

3 I also have a thought that can we have some detecting methods to find the lagging time earlier to adjust time
back to normal value in the code?

best regards,




tupeng212

Second draft of a patch posted; no test results so far for first draft.
Jan

From: Jan Beulich
Date: 2012-08-14 17:51
To: Yang Z Zhang; Keir Fraser; Tim Deegan
CC: tupeng212; xen-devel
Subject: [Xen-devel] [PATCH, RFC v2] x86/HVM: assorted RTC emulation adjustments (was Re: Big Bug:Time in VM goes slower...)
Below/attached a second draft of a patch to fix not only this
issue, but a few more with the RTC emulation.

Keir, Tim, Yang, others - the change to xen/arch/x86/hvm/vpt.c really
looks more like a hack than a solution, but I don't see another
way without much more intrusive changes. The point is that we
want the RTC code to decide whether to generate an interrupt
(so that RTC_PF can become set correctly even without RTC_PIE
getting enabled by the guest).

Additionally I wonder whether alarm_timer_update() shouldn't
bail on non-conforming RTC_*_ALARM values (as those would
never match the values they get compared against, whereas
with the current way of handling this they would appear to
match - i.e. set RTC_AF and possibly generate an interrupt -
some other point in time). I realize the behavior here may not
be precisely specified, but the specification saying "the current
time has matched the alarm time" means to me a value by value
comparison, which implies that non-conforming values would
never match (since non-conforming current time values could
get replaced at any time by the hardware due to overflow
detection).

Jan

- don't call rtc_timer_update() on REG_A writes when the value didn't
  change (doing the call always was reported to cause wall clock time
  lagging with the JVM running on Windows)
- don't call rtc_timer_update() on REG_B writes at all
- only call alarm_timer_update() on REG_B writes when relevant bits
  change
- only call check_update_timer() on REG_B writes when SET changes
- instead properly handle AF and PF when the guest is not also setting
  AIE/PIE respectively (for UF this was already the case, only a
  comment was slightly inaccurate)
- raise the RTC IRQ not only when UIE gets set while UF was already
  set, but generalize this to cover AIE and PIE as well
- properly mask off bit 7 when retrieving the hour values in
  alarm_timer_update(), and properly use RTC_HOURS_ALARM's bit 7 when
  converting from 12- to 24-hour value
- also handle the two other possible clock bases
- use RTC_* names in a couple of places where literal numbers were used
  so far

--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -50,11 +50,24 @@ 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_periodic_cb(struct vcpu *v, void *opaque)
+static void rtc_toggle_irq(RTCState *s)
+{
+    struct domain *d = vrtc_domain(s);
+
+    ASSERT(spin_is_locked(&s->lock));
+    s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
+    hvm_isa_irq_deassert(d, RTC_IRQ);
+    hvm_isa_irq_assert(d, RTC_IRQ);
+}
+
+void rtc_periodic_interrupt(void *opaque)
 {
     RTCState *s = opaque;
+
     spin_lock(&s->lock);
-    s->hw.cmos_data[RTC_REG_C] |= 0xc0;
+    s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
+    if ( s->hw.cmos_data[RTC_REG_B] & RTC_PIE )
+        rtc_toggle_irq(s);
     spin_unlock(&s->lock);
 }

@@ -68,19 +81,25 @@ static void rtc_timer_update(RTCState *s
     ASSERT(spin_is_locked(&s->lock));

     period_code = s->hw.cmos_data[RTC_REG_A] & RTC_RATE_SELECT;
-    if ( (period_code != 0) && (s->hw.cmos_data[RTC_REG_B] & RTC_PIE) )
+    switch ( s->hw.cmos_data[RTC_REG_A] & RTC_DIV_CTL )
     {
-        if ( period_code <= 2 )
+    case RTC_REF_CLCK_32KHZ:
+        if ( (period_code != 0) && (period_code <= 2) )
             period_code += 7;
-
-        period = 1 << (period_code - 1); /* period in 32 Khz cycles */
-        period = DIV_ROUND((period * 1000000000ULL), 32768); /* period in ns */
-        create_periodic_time(v, &s->pt, period, period, RTC_IRQ,
-                             rtc_periodic_cb, s);
-    }
-    else
-    {
+        /* fall through */
+    case RTC_REF_CLCK_1MHZ:
+    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 */
+            create_periodic_time(v, &s->pt, period, period, RTC_IRQ, NULL, s);
+            break;
+        }
+        /* fall through */
+    default:
         destroy_periodic_time(&s->pt);
+        break;
     }
 }

@@ -102,7 +121,7 @@ static void check_update_timer(RTCState 
         guest_usec = get_localtime_us(d) % USEC_PER_SEC;
         if (guest_usec >= (USEC_PER_SEC - 244))
         {
-            /* RTC is in update cycle when enabling UIE */
+            /* RTC is in update cycle */
             s->hw.cmos_data[RTC_REG_A] |= RTC_UIP;
             next_update_time = (USEC_PER_SEC - guest_usec) * NS_PER_USEC;
             expire_time = NOW() + next_update_time;
@@ -144,7 +163,6 @@ static void rtc_update_timer(void *opaqu
 static void rtc_update_timer2(void *opaque)
 {
     RTCState *s = opaque;
-    struct domain *d = vrtc_domain(s);

     spin_lock(&s->lock);
     if (!(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
@@ -152,11 +170,7 @@ static void rtc_update_timer2(void *opaq
         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))
-        {
-            s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
-            hvm_isa_irq_deassert(d, RTC_IRQ);
-            hvm_isa_irq_assert(d, RTC_IRQ);
-        }
+            rtc_toggle_irq(s);
         check_update_timer(s);
     }
     spin_unlock(&s->lock);
@@ -175,21 +189,18 @@ static void alarm_timer_update(RTCState 

     stop_timer(&s->alarm_timer);

-    if ((s->hw.cmos_data[RTC_REG_B] & RTC_AIE) &&
-            !(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
+    if ( !(s->hw.cmos_data[RTC_REG_B] & RTC_SET) )
     {
         s->current_tm = gmtime(get_localtime(d));
         rtc_copy_date(s);

         alarm_sec = from_bcd(s, s->hw.cmos_data[RTC_SECONDS_ALARM]);
         alarm_min = from_bcd(s, s->hw.cmos_data[RTC_MINUTES_ALARM]);
-        alarm_hour = from_bcd(s, s->hw.cmos_data[RTC_HOURS_ALARM]);
-        alarm_hour = convert_hour(s, alarm_hour);
+        alarm_hour = convert_hour(s, s->hw.cmos_data[RTC_HOURS_ALARM]);

         cur_sec = from_bcd(s, s->hw.cmos_data[RTC_SECONDS]);
         cur_min = from_bcd(s, s->hw.cmos_data[RTC_MINUTES]);
-        cur_hour = from_bcd(s, s->hw.cmos_data[RTC_HOURS]);
-        cur_hour = convert_hour(s, cur_hour);
+        cur_hour = convert_hour(s, s->hw.cmos_data[RTC_HOURS]);

         next_update_time = USEC_PER_SEC - (get_localtime_us(d) % USEC_PER_SEC);
         next_update_time = next_update_time * NS_PER_USEC + NOW();
@@ -343,7 +354,6 @@ static void alarm_timer_update(RTCState 
 static void rtc_alarm_cb(void *opaque)
 {
     RTCState *s = opaque;
-    struct domain *d = vrtc_domain(s);

     spin_lock(&s->lock);
     if (!(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
@@ -351,11 +361,7 @@ static void rtc_alarm_cb(void *opaque)
         s->hw.cmos_data[RTC_REG_C] |= RTC_AF;
         /* alarm interrupt */
         if (s->hw.cmos_data[RTC_REG_B] & RTC_AIE)
-        {
-            s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
-            hvm_isa_irq_deassert(d, RTC_IRQ);
-            hvm_isa_irq_assert(d, RTC_IRQ);
-        }
+            rtc_toggle_irq(s);
         alarm_timer_update(s);
     }
     spin_unlock(&s->lock);
@@ -365,6 +371,7 @@ static int rtc_ioport_write(void *opaque
 {
     RTCState *s = opaque;
     struct domain *d = vrtc_domain(s);
+    uint32_t orig, mask;

     spin_lock(&s->lock);

@@ -382,6 +389,7 @@ static int rtc_ioport_write(void *opaque
         return 0;
     }

+    orig = s->hw.cmos_data[s->hw.cmos_index];
     switch ( s->hw.cmos_index )
     {
     case RTC_SECONDS_ALARM:
@@ -405,9 +413,9 @@ static int rtc_ioport_write(void *opaque
         break;
     case RTC_REG_A:
         /* UIP bit is read only */
-        s->hw.cmos_data[RTC_REG_A] = (data & ~RTC_UIP) |
-            (s->hw.cmos_data[RTC_REG_A] & RTC_UIP);
-        rtc_timer_update(s);
+        s->hw.cmos_data[RTC_REG_A] = (data & ~RTC_UIP) | (orig & RTC_UIP);
+        if ( (data ^ orig) & (RTC_RATE_SELECT | RTC_DIV_CTL) )
+            rtc_timer_update(s);
         break;
     case RTC_REG_B:
         if ( data & RTC_SET )
@@ -415,7 +423,7 @@ static int rtc_ioport_write(void *opaque
             /* set mode: reset UIP mode */
             s->hw.cmos_data[RTC_REG_A] &= ~RTC_UIP;
             /* adjust cmos before stopping */
-            if (!(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
+            if (!(orig & RTC_SET))
             {
                 s->current_tm = gmtime(get_localtime(d));
                 rtc_copy_date(s);
@@ -424,21 +432,26 @@ static int rtc_ioport_write(void *opaque
         else
         {
             /* if disabling set mode, update the time */
-            if ( s->hw.cmos_data[RTC_REG_B] & RTC_SET )
+            if ( orig & RTC_SET )
                 rtc_set_time(s);
         }
-        /* if the interrupt is already set when the interrupt become
-         * enabled, raise an interrupt immediately*/
-        if ((data & RTC_UIE) && !(s->hw.cmos_data[RTC_REG_B] & RTC_UIE))
-            if (s->hw.cmos_data[RTC_REG_C] & RTC_UF)
+        /*
+         * 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) )
             {
-                hvm_isa_irq_deassert(d, RTC_IRQ);
-                hvm_isa_irq_assert(d, RTC_IRQ);
+                rtc_toggle_irq(s);
+                break;
             }
         s->hw.cmos_data[RTC_REG_B] = data;
-        rtc_timer_update(s);
-        check_update_timer(s);
-        alarm_timer_update(s);
+        if ( (data ^ orig) & RTC_SET )
+            check_update_timer(s);
+        if ( (data ^ orig) & (RTC_24H | RTC_DM_BINARY | RTC_SET) )
+            alarm_timer_update(s);
         break;
     case RTC_REG_C:
     case RTC_REG_D:
@@ -453,7 +466,7 @@ static int rtc_ioport_write(void *opaque

 static inline int to_bcd(RTCState *s, int a)
 {
-    if ( s->hw.cmos_data[RTC_REG_B] & 0x04 )
+    if ( s->hw.cmos_data[RTC_REG_B] & RTC_DM_BINARY )
         return a;
     else
         return ((a / 10) << 4) | (a % 10);
@@ -461,7 +474,7 @@ static inline int to_bcd(RTCState *s, in

 static inline int from_bcd(RTCState *s, int a)
 {
-    if ( s->hw.cmos_data[RTC_REG_B] & 0x04 )
+    if ( s->hw.cmos_data[RTC_REG_B] & RTC_DM_BINARY )
         return a;
     else
         return ((a >> 4) * 10) + (a & 0x0f);
@@ -469,12 +482,14 @@ static inline int from_bcd(RTCState *s, 

 /* Hours in 12 hour mode are in 1-12 range, not 0-11.
  * So we need convert it before using it*/
-static inline int convert_hour(RTCState *s, int hour)
+static inline int convert_hour(RTCState *s, int raw)
 {
+    int hour = from_bcd(s, raw & 0x7f);
+
     if (!(s->hw.cmos_data[RTC_REG_B] & RTC_24H))
     {
         hour %= 12;
-        if (s->hw.cmos_data[RTC_HOURS] & 0x80)
+        if (raw & 0x80)
             hour += 12;
     }
     return hour;
@@ -493,8 +508,7 @@ static void rtc_set_time(RTCState *s)
     
     tm->tm_sec = from_bcd(s, s->hw.cmos_data[RTC_SECONDS]);
     tm->tm_min = from_bcd(s, s->hw.cmos_data[RTC_MINUTES]);
-    tm->tm_hour = from_bcd(s, s->hw.cmos_data[RTC_HOURS] & 0x7f);
-    tm->tm_hour = convert_hour(s, tm->tm_hour);
+    tm->tm_hour = convert_hour(s, s->hw.cmos_data[RTC_HOURS]);
     tm->tm_wday = from_bcd(s, s->hw.cmos_data[RTC_DAY_OF_WEEK]);
     tm->tm_mday = from_bcd(s, s->hw.cmos_data[RTC_DAY_OF_MONTH]);
     tm->tm_mon = from_bcd(s, s->hw.cmos_data[RTC_MONTH]) - 1;
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -22,6 +22,7 @@
 #include <asm/hvm/vpt.h>
 #include <asm/event.h>
 #include <asm/apic.h>
+#include <asm/mc146818rtc.h>

 #define mode_is(d, name) \
     ((d)->arch.hvm_domain.params[HVM_PARAM_TIMER_MODE] == HVMPTM_##name)
@@ -218,6 +219,7 @@ void pt_update_irq(struct vcpu *v)
     struct periodic_time *pt, *temp, *earliest_pt = NULL;
     uint64_t max_lag = -1ULL;
     int irq, is_lapic;
+    void *pt_priv;

     spin_lock(&v->arch.hvm_vcpu.tm_lock);

@@ -251,13 +253,14 @@ void pt_update_irq(struct vcpu *v)
     earliest_pt->irq_issued = 1;
     irq = earliest_pt->irq;
     is_lapic = (earliest_pt->source == PTSRC_lapic);
+    pt_priv = earliest_pt->priv;

     spin_unlock(&v->arch.hvm_vcpu.tm_lock);

     if ( is_lapic )
-    {
         vlapic_set_irq(vcpu_vlapic(v), irq, 0);
-    }
+    else if ( irq == RTC_IRQ )
+        rtc_periodic_interrupt(pt_priv);
     else
     {
         hvm_isa_irq_deassert(v->domain, irq);
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -181,6 +181,7 @@ void rtc_migrate_timers(struct vcpu *v);
 void rtc_deinit(struct domain *d);
 void rtc_reset(struct domain *d);
 void rtc_update_clock(struct domain *d);
+void rtc_periodic_interrupt(void *);

 void pmtimer_init(struct vcpu *v);
 void pmtimer_deinit(struct domain *d);


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[-- Attachment #1.2: Type: text/html, Size: 36552 bytes --]

[-- Attachment #2: test.exe.a --]
[-- Type: application/octet-stream, Size: 155692 bytes --]

[-- Attachment #3: test.cpp --]
[-- Type: application/octet-stream, Size: 1200 bytes --]

#include <stdio.h>
#include <windows.h>
typedef int (__stdcall *NTSETTIMER)(IN ULONG RequestedResolution, IN BOOLEAN Set, OUT PULONG ActualResolution );
typedef int (__stdcall *NTQUERYTIMER)(OUT PULONG   MinimumResolution, OUT PULONG MaximumResolution, OUT PULONG CurrentResolution );

int main()
{
	DWORD min_res = 0, max_res = 0, cur_res = 0, ret = 0;
	HMODULE  hdll = NULL;
	hdll = GetModuleHandle("ntdll.dll");
	NTSETTIMER AddrNtSetTimer = 0;
	NTQUERYTIMER AddrNtQueyTimer = 0;
	
	AddrNtSetTimer = (NTSETTIMER) GetProcAddress(hdll, "NtSetTimerResolution");
	AddrNtQueyTimer = (NTQUERYTIMER)GetProcAddress(hdll, "NtQueryTimerResolution");

	while (1)
	{
		ret = AddrNtQueyTimer(&min_res, &max_res, &cur_res);
		printf("min_res = %d, max_res = %d, cur_res = %d\n",min_res, max_res, cur_res);
		Sleep(5);
		ret = AddrNtSetTimer(10000, 1, &cur_res);
		Sleep(5);
		ret = AddrNtSetTimer(10000, 0, &cur_res);
		Sleep(5);
		ret = AddrNtSetTimer(50000, 1, &cur_res);
		Sleep(5);
		ret = AddrNtSetTimer(50000, 0, &cur_res);
		Sleep(5);
		ret = AddrNtSetTimer(100000, 1, &cur_res);
		Sleep(5);
		ret = AddrNtSetTimer(100000, 0, &cur_res);
		Sleep(5);
	}

	return 0;
}


[-- Attachment #4: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH, RFC v2] x86/HVM: assorted RTC emulation adjustments (was Re: Big Bug:Time in VM goes slower...)
  2012-08-15 14:07 ` tupeng212
@ 2012-08-15 14:12   ` tupeng212
  2012-08-16  8:22     ` Jan Beulich
  2012-08-15 15:00   ` Jan Beulich
  1 sibling, 1 reply; 8+ messages in thread
From: tupeng212 @ 2012-08-15 14:12 UTC (permalink / raw)
  To: Jan Beulich, Yang Z Zhang, Keir Fraser, Tim Deegan; +Cc: xen-devel


[-- Attachment #1.1: Type: text/plain, Size: 16600 bytes --]

Hi, Jan:
I am sorry I really don't have much time to try a test of your patch, and it is not convenient
for me to have a try. For the version I have been using is xen4.0.x, and your patch is based on 
the latest version xen4.2.x.(I have never complied the unstable one), so I merged your patch to my 
xen4.0.x, still couldn't find the two functions below:
 static void rtc_update_timer2(void *opaque) 
 static void rtc_alarm_cb(void *opaque) 
so I didn't merge the two functions which contains a rtc_toggle_irq() .

The results for me were these:
1 In my real application environment, it worked very well in the former 5mins, much better than before,
 but at last it lagged again. I don't know whether it belongs to the two missed functions. I lack the 
 ability to figure them out.

2 When I tested my test program which I provided days before, it worked very well, maybe the program doesn't 
emulate the real environment due to the same setting rate, so I modified this program as which in the attachment.
if you are more convenient, you can help me to have a look of it.
--------------------------------------------------------------------
#include <stdio.h>
#include <windows.h>
typedef int (__stdcall *NTSETTIMER)(IN ULONG RequestedResolution, IN BOOLEAN Set, OUT PULONG ActualResolution );
typedef int (__stdcall *NTQUERYTIMER)(OUT PULONG   MinimumResolution, OUT PULONG MaximumResolution, OUT PULONG CurrentResolution );

int main()
{
DWORD min_res = 0, max_res = 0, cur_res = 0, ret = 0;
HMODULE  hdll = NULL;
hdll = GetModuleHandle("ntdll.dll");
NTSETTIMER AddrNtSetTimer = 0;
NTQUERYTIMER AddrNtQueyTimer = 0;
AddrNtSetTimer = (NTSETTIMER) GetProcAddress(hdll, "NtSetTimerResolution");
AddrNtQueyTimer = (NTQUERYTIMER)GetProcAddress(hdll, "NtQueryTimerResolution");

while (1)
{
ret = AddrNtQueyTimer(&min_res, &max_res, &cur_res);
printf("min_res = %d, max_res = %d, cur_res = %d\n",min_res, max_res, cur_res);
Sleep(5);
ret = AddrNtSetTimer(10000, 1, &cur_res);
Sleep(5);
ret = AddrNtSetTimer(10000, 0, &cur_res);
Sleep(5);
ret = AddrNtSetTimer(50000, 1, &cur_res);
Sleep(5);
ret = AddrNtSetTimer(50000, 0, &cur_res);
Sleep(5);
ret = AddrNtSetTimer(100000, 1, &cur_res);
Sleep(5);
ret = AddrNtSetTimer(100000, 0, &cur_res);
Sleep(5);
}

return 0;
}
--------------------------------------------------------------------
And I have a opinion, because our product is based on Version Xen4.0.x, if you have enough time, can you write 
another patch based http://xenbits.xen.org/hg/xen-4.0-testing.hg/ for me, thank you very much!

3 I also have a thought that can we have some detecting methods to find the lagging time earlier to adjust time
back to normal value in the code?

best regards,




tupeng212

Second draft of a patch posted; no test results so far for first draft.
Jan

From: Jan Beulich
Date: 2012-08-14 17:51
To: Yang Z Zhang; Keir Fraser; Tim Deegan
CC: tupeng212; xen-devel
Subject: [Xen-devel] [PATCH, RFC v2] x86/HVM: assorted RTC emulation adjustments (was Re: Big Bug:Time in VM goes slower...)
Below/attached a second draft of a patch to fix not only this
issue, but a few more with the RTC emulation.

Keir, Tim, Yang, others - the change to xen/arch/x86/hvm/vpt.c really
looks more like a hack than a solution, but I don't see another
way without much more intrusive changes. The point is that we
want the RTC code to decide whether to generate an interrupt
(so that RTC_PF can become set correctly even without RTC_PIE
getting enabled by the guest).

Additionally I wonder whether alarm_timer_update() shouldn't
bail on non-conforming RTC_*_ALARM values (as those would
never match the values they get compared against, whereas
with the current way of handling this they would appear to
match - i.e. set RTC_AF and possibly generate an interrupt -
some other point in time). I realize the behavior here may not
be precisely specified, but the specification saying "the current
time has matched the alarm time" means to me a value by value
comparison, which implies that non-conforming values would
never match (since non-conforming current time values could
get replaced at any time by the hardware due to overflow
detection).

Jan

- don't call rtc_timer_update() on REG_A writes when the value didn't
  change (doing the call always was reported to cause wall clock time
  lagging with the JVM running on Windows)
- don't call rtc_timer_update() on REG_B writes at all
- only call alarm_timer_update() on REG_B writes when relevant bits
  change
- only call check_update_timer() on REG_B writes when SET changes
- instead properly handle AF and PF when the guest is not also setting
  AIE/PIE respectively (for UF this was already the case, only a
  comment was slightly inaccurate)
- raise the RTC IRQ not only when UIE gets set while UF was already
  set, but generalize this to cover AIE and PIE as well
- properly mask off bit 7 when retrieving the hour values in
  alarm_timer_update(), and properly use RTC_HOURS_ALARM's bit 7 when
  converting from 12- to 24-hour value
- also handle the two other possible clock bases
- use RTC_* names in a couple of places where literal numbers were used
  so far

--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -50,11 +50,24 @@ 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_periodic_cb(struct vcpu *v, void *opaque)
+static void rtc_toggle_irq(RTCState *s)
+{
+    struct domain *d = vrtc_domain(s);
+
+    ASSERT(spin_is_locked(&s->lock));
+    s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
+    hvm_isa_irq_deassert(d, RTC_IRQ);
+    hvm_isa_irq_assert(d, RTC_IRQ);
+}
+
+void rtc_periodic_interrupt(void *opaque)
 {
     RTCState *s = opaque;
+
     spin_lock(&s->lock);
-    s->hw.cmos_data[RTC_REG_C] |= 0xc0;
+    s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
+    if ( s->hw.cmos_data[RTC_REG_B] & RTC_PIE )
+        rtc_toggle_irq(s);
     spin_unlock(&s->lock);
 }

@@ -68,19 +81,25 @@ static void rtc_timer_update(RTCState *s
     ASSERT(spin_is_locked(&s->lock));

     period_code = s->hw.cmos_data[RTC_REG_A] & RTC_RATE_SELECT;
-    if ( (period_code != 0) && (s->hw.cmos_data[RTC_REG_B] & RTC_PIE) )
+    switch ( s->hw.cmos_data[RTC_REG_A] & RTC_DIV_CTL )
     {
-        if ( period_code <= 2 )
+    case RTC_REF_CLCK_32KHZ:
+        if ( (period_code != 0) && (period_code <= 2) )
             period_code += 7;
-
-        period = 1 << (period_code - 1); /* period in 32 Khz cycles */
-        period = DIV_ROUND((period * 1000000000ULL), 32768); /* period in ns */
-        create_periodic_time(v, &s->pt, period, period, RTC_IRQ,
-                             rtc_periodic_cb, s);
-    }
-    else
-    {
+        /* fall through */
+    case RTC_REF_CLCK_1MHZ:
+    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 */
+            create_periodic_time(v, &s->pt, period, period, RTC_IRQ, NULL, s);
+            break;
+        }
+        /* fall through */
+    default:
         destroy_periodic_time(&s->pt);
+        break;
     }
 }

@@ -102,7 +121,7 @@ static void check_update_timer(RTCState 
         guest_usec = get_localtime_us(d) % USEC_PER_SEC;
         if (guest_usec >= (USEC_PER_SEC - 244))
         {
-            /* RTC is in update cycle when enabling UIE */
+            /* RTC is in update cycle */
             s->hw.cmos_data[RTC_REG_A] |= RTC_UIP;
             next_update_time = (USEC_PER_SEC - guest_usec) * NS_PER_USEC;
             expire_time = NOW() + next_update_time;
@@ -144,7 +163,6 @@ static void rtc_update_timer(void *opaqu
 static void rtc_update_timer2(void *opaque)
 {
     RTCState *s = opaque;
-    struct domain *d = vrtc_domain(s);

     spin_lock(&s->lock);
     if (!(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
@@ -152,11 +170,7 @@ static void rtc_update_timer2(void *opaq
         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))
-        {
-            s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
-            hvm_isa_irq_deassert(d, RTC_IRQ);
-            hvm_isa_irq_assert(d, RTC_IRQ);
-        }
+            rtc_toggle_irq(s);
         check_update_timer(s);
     }
     spin_unlock(&s->lock);
@@ -175,21 +189,18 @@ static void alarm_timer_update(RTCState 

     stop_timer(&s->alarm_timer);

-    if ((s->hw.cmos_data[RTC_REG_B] & RTC_AIE) &&
-            !(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
+    if ( !(s->hw.cmos_data[RTC_REG_B] & RTC_SET) )
     {
         s->current_tm = gmtime(get_localtime(d));
         rtc_copy_date(s);

         alarm_sec = from_bcd(s, s->hw.cmos_data[RTC_SECONDS_ALARM]);
         alarm_min = from_bcd(s, s->hw.cmos_data[RTC_MINUTES_ALARM]);
-        alarm_hour = from_bcd(s, s->hw.cmos_data[RTC_HOURS_ALARM]);
-        alarm_hour = convert_hour(s, alarm_hour);
+        alarm_hour = convert_hour(s, s->hw.cmos_data[RTC_HOURS_ALARM]);

         cur_sec = from_bcd(s, s->hw.cmos_data[RTC_SECONDS]);
         cur_min = from_bcd(s, s->hw.cmos_data[RTC_MINUTES]);
-        cur_hour = from_bcd(s, s->hw.cmos_data[RTC_HOURS]);
-        cur_hour = convert_hour(s, cur_hour);
+        cur_hour = convert_hour(s, s->hw.cmos_data[RTC_HOURS]);

         next_update_time = USEC_PER_SEC - (get_localtime_us(d) % USEC_PER_SEC);
         next_update_time = next_update_time * NS_PER_USEC + NOW();
@@ -343,7 +354,6 @@ static void alarm_timer_update(RTCState 
 static void rtc_alarm_cb(void *opaque)
 {
     RTCState *s = opaque;
-    struct domain *d = vrtc_domain(s);

     spin_lock(&s->lock);
     if (!(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
@@ -351,11 +361,7 @@ static void rtc_alarm_cb(void *opaque)
         s->hw.cmos_data[RTC_REG_C] |= RTC_AF;
         /* alarm interrupt */
         if (s->hw.cmos_data[RTC_REG_B] & RTC_AIE)
-        {
-            s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
-            hvm_isa_irq_deassert(d, RTC_IRQ);
-            hvm_isa_irq_assert(d, RTC_IRQ);
-        }
+            rtc_toggle_irq(s);
         alarm_timer_update(s);
     }
     spin_unlock(&s->lock);
@@ -365,6 +371,7 @@ static int rtc_ioport_write(void *opaque
 {
     RTCState *s = opaque;
     struct domain *d = vrtc_domain(s);
+    uint32_t orig, mask;

     spin_lock(&s->lock);

@@ -382,6 +389,7 @@ static int rtc_ioport_write(void *opaque
         return 0;
     }

+    orig = s->hw.cmos_data[s->hw.cmos_index];
     switch ( s->hw.cmos_index )
     {
     case RTC_SECONDS_ALARM:
@@ -405,9 +413,9 @@ static int rtc_ioport_write(void *opaque
         break;
     case RTC_REG_A:
         /* UIP bit is read only */
-        s->hw.cmos_data[RTC_REG_A] = (data & ~RTC_UIP) |
-            (s->hw.cmos_data[RTC_REG_A] & RTC_UIP);
-        rtc_timer_update(s);
+        s->hw.cmos_data[RTC_REG_A] = (data & ~RTC_UIP) | (orig & RTC_UIP);
+        if ( (data ^ orig) & (RTC_RATE_SELECT | RTC_DIV_CTL) )
+            rtc_timer_update(s);
         break;
     case RTC_REG_B:
         if ( data & RTC_SET )
@@ -415,7 +423,7 @@ static int rtc_ioport_write(void *opaque
             /* set mode: reset UIP mode */
             s->hw.cmos_data[RTC_REG_A] &= ~RTC_UIP;
             /* adjust cmos before stopping */
-            if (!(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
+            if (!(orig & RTC_SET))
             {
                 s->current_tm = gmtime(get_localtime(d));
                 rtc_copy_date(s);
@@ -424,21 +432,26 @@ static int rtc_ioport_write(void *opaque
         else
         {
             /* if disabling set mode, update the time */
-            if ( s->hw.cmos_data[RTC_REG_B] & RTC_SET )
+            if ( orig & RTC_SET )
                 rtc_set_time(s);
         }
-        /* if the interrupt is already set when the interrupt become
-         * enabled, raise an interrupt immediately*/
-        if ((data & RTC_UIE) && !(s->hw.cmos_data[RTC_REG_B] & RTC_UIE))
-            if (s->hw.cmos_data[RTC_REG_C] & RTC_UF)
+        /*
+         * 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) )
             {
-                hvm_isa_irq_deassert(d, RTC_IRQ);
-                hvm_isa_irq_assert(d, RTC_IRQ);
+                rtc_toggle_irq(s);
+                break;
             }
         s->hw.cmos_data[RTC_REG_B] = data;
-        rtc_timer_update(s);
-        check_update_timer(s);
-        alarm_timer_update(s);
+        if ( (data ^ orig) & RTC_SET )
+            check_update_timer(s);
+        if ( (data ^ orig) & (RTC_24H | RTC_DM_BINARY | RTC_SET) )
+            alarm_timer_update(s);
         break;
     case RTC_REG_C:
     case RTC_REG_D:
@@ -453,7 +466,7 @@ static int rtc_ioport_write(void *opaque

 static inline int to_bcd(RTCState *s, int a)
 {
-    if ( s->hw.cmos_data[RTC_REG_B] & 0x04 )
+    if ( s->hw.cmos_data[RTC_REG_B] & RTC_DM_BINARY )
         return a;
     else
         return ((a / 10) << 4) | (a % 10);
@@ -461,7 +474,7 @@ static inline int to_bcd(RTCState *s, in

 static inline int from_bcd(RTCState *s, int a)
 {
-    if ( s->hw.cmos_data[RTC_REG_B] & 0x04 )
+    if ( s->hw.cmos_data[RTC_REG_B] & RTC_DM_BINARY )
         return a;
     else
         return ((a >> 4) * 10) + (a & 0x0f);
@@ -469,12 +482,14 @@ static inline int from_bcd(RTCState *s, 

 /* Hours in 12 hour mode are in 1-12 range, not 0-11.
  * So we need convert it before using it*/
-static inline int convert_hour(RTCState *s, int hour)
+static inline int convert_hour(RTCState *s, int raw)
 {
+    int hour = from_bcd(s, raw & 0x7f);
+
     if (!(s->hw.cmos_data[RTC_REG_B] & RTC_24H))
     {
         hour %= 12;
-        if (s->hw.cmos_data[RTC_HOURS] & 0x80)
+        if (raw & 0x80)
             hour += 12;
     }
     return hour;
@@ -493,8 +508,7 @@ static void rtc_set_time(RTCState *s)
     
     tm->tm_sec = from_bcd(s, s->hw.cmos_data[RTC_SECONDS]);
     tm->tm_min = from_bcd(s, s->hw.cmos_data[RTC_MINUTES]);
-    tm->tm_hour = from_bcd(s, s->hw.cmos_data[RTC_HOURS] & 0x7f);
-    tm->tm_hour = convert_hour(s, tm->tm_hour);
+    tm->tm_hour = convert_hour(s, s->hw.cmos_data[RTC_HOURS]);
     tm->tm_wday = from_bcd(s, s->hw.cmos_data[RTC_DAY_OF_WEEK]);
     tm->tm_mday = from_bcd(s, s->hw.cmos_data[RTC_DAY_OF_MONTH]);
     tm->tm_mon = from_bcd(s, s->hw.cmos_data[RTC_MONTH]) - 1;
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -22,6 +22,7 @@
 #include <asm/hvm/vpt.h>
 #include <asm/event.h>
 #include <asm/apic.h>
+#include <asm/mc146818rtc.h>

 #define mode_is(d, name) \
     ((d)->arch.hvm_domain.params[HVM_PARAM_TIMER_MODE] == HVMPTM_##name)
@@ -218,6 +219,7 @@ void pt_update_irq(struct vcpu *v)
     struct periodic_time *pt, *temp, *earliest_pt = NULL;
     uint64_t max_lag = -1ULL;
     int irq, is_lapic;
+    void *pt_priv;

     spin_lock(&v->arch.hvm_vcpu.tm_lock);

@@ -251,13 +253,14 @@ void pt_update_irq(struct vcpu *v)
     earliest_pt->irq_issued = 1;
     irq = earliest_pt->irq;
     is_lapic = (earliest_pt->source == PTSRC_lapic);
+    pt_priv = earliest_pt->priv;

     spin_unlock(&v->arch.hvm_vcpu.tm_lock);

     if ( is_lapic )
-    {
         vlapic_set_irq(vcpu_vlapic(v), irq, 0);
-    }
+    else if ( irq == RTC_IRQ )
+        rtc_periodic_interrupt(pt_priv);
     else
     {
         hvm_isa_irq_deassert(v->domain, irq);
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -181,6 +181,7 @@ void rtc_migrate_timers(struct vcpu *v);
 void rtc_deinit(struct domain *d);
 void rtc_reset(struct domain *d);
 void rtc_update_clock(struct domain *d);
+void rtc_periodic_interrupt(void *);

 void pmtimer_init(struct vcpu *v);
 void pmtimer_deinit(struct domain *d);


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[-- Attachment #1.2: Type: text/html, Size: 39147 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH, RFC v2] x86/HVM: assorted RTC emulation adjustments (was Re: Big Bug:Time in VM goes slower...)
  2012-08-15 14:07 ` tupeng212
  2012-08-15 14:12   ` tupeng212
@ 2012-08-15 15:00   ` Jan Beulich
  1 sibling, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2012-08-15 15:00 UTC (permalink / raw)
  To: tupeng212; +Cc: Yang Z Zhang, Tim Deegan, Keir Fraser, xen-devel

>>> On 15.08.12 at 16:07, tupeng212 <tupeng212@gmail.com> wrote:
> Hi, Jan:
> I am sorry I really don't have much time to try a test of your patch, and it 
> is not convenient
> for me to have a try. For the version I have been using is xen4.0.x, and 
> your patch is based on 
> the latest version xen4.2.x.(I have never complied the unstable one), so I 
> merged your patch to my 
> xen4.0.x, still couldn't find the two functions below:
>  static void rtc_update_timer2(void *opaque) 
>  static void rtc_alarm_cb(void *opaque) 
> so I didn't merge the two functions which contains a rtc_toggle_irq() .

Which looks quite right for the older tree.

> The results for me were these:
> 1 In my real application environment, it worked very well in the former 
> 5mins, much better than before,
>  but at last it lagged again. I don't know whether it belongs to the two 
> missed functions. I lack the 
>  ability to figure them out.

Unlikely.

> 2 When I tested my test program which I provided days before, it worked very 
> well, maybe the program doesn't 
> emulate the real environment due to the same setting rate, so I modified 
> this program as which in the attachment.

Okay, so we're at least moving in the right direction.

> if you are more convenient, you can help me to have a look of it.
> And I have a opinion, because our product is based on Version Xen4.0.x, if 
> you have enough time, can you write 
> another patch based http://xenbits.xen.org/hg/xen-4.0-testing.hg/ for me, 
> thank you very much!

You seem to have done the right thing, so I don't see an
immediate need.

> 3 I also have a thought that can we have some detecting methods to find the 
> lagging time earlier to adjust time
> back to normal value in the code?

For that we'd first need to understand what the reason for the
remaining mis-behavior is.

Jan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH, RFC v2] x86/HVM: assorted RTC emulation adjustments (was Re: Big Bug:Time in VM goes slower...)
  2012-08-15 14:12   ` tupeng212
@ 2012-08-16  8:22     ` Jan Beulich
  2012-08-16 13:43       ` tupeng212
  0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2012-08-16  8:22 UTC (permalink / raw)
  To: tupeng212; +Cc: Yang Z Zhang, Tim Deegan, Keir Fraser, xen-devel

>>> On 15.08.12 at 16:12, tupeng212 <tupeng212@gmail.com> wrote:
> The results for me were these:
> 1 In my real application environment, it worked very well in the former 
> 5mins, much better than before,
>  but at last it lagged again. I don't know whether it belongs to the two 
> missed functions. I lack the 
>  ability to figure them out.

Did you check whether possibly the guest kernel started flipping
between to rate values? If so, that is something that we could
deal with too (yet it might be a bit involved, so I'd like to avoid
going that route if it would lead nowhere).

Jan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH, RFC v2] x86/HVM: assorted RTC emulation adjustments (was Re: Big Bug:Time in VM goes slower...)
  2012-08-16  8:22     ` Jan Beulich
@ 2012-08-16 13:43       ` tupeng212
  2012-08-16 14:15         ` Jan Beulich
  0 siblings, 1 reply; 8+ messages in thread
From: tupeng212 @ 2012-08-16 13:43 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Yang Z Zhang, Tim Deegan, Keir Fraser, xen-devel


[-- Attachment #1.1: Type: text/plain, Size: 1305 bytes --]

Dear Jan:
I didn't check the flipping in JVM, but from the printing period in xen, this setting happened(unit: ns):
976562, 976562, 976562, 976562, 15625000, 976562, 976562, 976562, 976562, 15625000 .....
and someone told me windows use these two rate/period only.

besides, I checked my former simple tester this morning after it ran for a whole night, it lagged much.
so there exists difference between virtualization and reality.



tupeng212

From: Jan Beulich
Date: 2012-08-16 16:22
To: tupeng212
CC: Yang Z Zhang; xen-devel; Keir Fraser; Tim Deegan
Subject: Re: [Xen-devel] [PATCH, RFC v2] x86/HVM: assorted RTC emulation adjustments (was Re: Big Bug:Time in VM goes slower...)
>>> On 15.08.12 at 16:12, tupeng212 <tupeng212@gmail.com> wrote:
> The results for me were these:
> 1 In my real application environment, it worked very well in the former 
> 5mins, much better than before,
>  but at last it lagged again. I don't know whether it belongs to the two 
> missed functions. I lack the 
>  ability to figure them out.

Did you check whether possibly the guest kernel started flipping
between to rate values? If so, that is something that we could
deal with too (yet it might be a bit involved, so I'd like to avoid
going that route if it would lead nowhere).

Jan

[-- Attachment #1.2: Type: text/html, Size: 3559 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH, RFC v2] x86/HVM: assorted RTC emulation adjustments (was Re: Big Bug:Time in VM goes slower...)
  2012-08-16 13:43       ` tupeng212
@ 2012-08-16 14:15         ` Jan Beulich
  2012-08-16 14:27           ` tupeng212
  0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2012-08-16 14:15 UTC (permalink / raw)
  To: tupeng212; +Cc: Yang Z Zhang, Tim Deegan, Keir Fraser, xen-devel

>>> On 16.08.12 at 15:43, tupeng212 <tupeng212@gmail.com> wrote:
> Dear Jan:
> I didn't check the flipping in JVM, but from the printing period in xen, 
> this setting happened(unit: ns):
> 976562, 976562, 976562, 976562, 15625000, 976562, 976562, 976562, 976562, 
> 15625000 .....
> and someone told me windows use these two rate/period only.

Okay, some back and forth is there in any case, but without
knowing the time distance between the individual instances it's
hard to tell whether what I'm thinking of might help.

> besides, I checked my former simple tester this morning after it ran for a 
> whole night, it lagged much.

Could you btw quantify the lagging?

Your tester sets only a single, constant rate repeatedly, right?
If so, then the thought of adjustment likely won't help.

Jan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH, RFC v2] x86/HVM: assorted RTC emulation adjustments (was Re: Big Bug:Time in VM goes slower...)
  2012-08-16 14:15         ` Jan Beulich
@ 2012-08-16 14:27           ` tupeng212
  0 siblings, 0 replies; 8+ messages in thread
From: tupeng212 @ 2012-08-16 14:27 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Yang Z Zhang, Tim Deegan, Keir Fraser, xen-devel


[-- Attachment #1.1: Type: text/plain, Size: 691 bytes --]

Okay, some back and forth is there in any case, but without
knowing the time distance between the individual instances it's
hard to tell whether what I'm thinking of might help.

> besides, I checked my former simple tester this morning after it ran for a 
> whole night, it lagged much.

Could you btw quantify the lagging? 
// I didn't pay attention to it, but about half an hour's lagging certainly exist. 
when you will go home, you can also have a try to see result tomorrow morning.

Your tester sets only a single, constant rate repeatedly, right? //yes, the simplest test just setting a single 10000 down repeatly.
If so, then the thought of adjustment likely won't help.

[-- Attachment #1.2: Type: text/html, Size: 1842 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2012-08-16 14:27 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-08-14  9:51 [PATCH, RFC v2] x86/HVM: assorted RTC emulation adjustments (was Re: Big Bug:Time in VM goes slower...) Jan Beulich
2012-08-15 14:07 ` tupeng212
2012-08-15 14:12   ` tupeng212
2012-08-16  8:22     ` Jan Beulich
2012-08-16 13:43       ` tupeng212
2012-08-16 14:15         ` Jan Beulich
2012-08-16 14:27           ` tupeng212
2012-08-15 15:00   ` Jan Beulich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).