Re: [PATCH, RFC v2] x86/HVM: assorted RTC emulation adjustments (was Re: Big Bug:Time in VM goes slower...)

[tupeng212 <tupeng212@gmail.com>]

[-- 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