thr3ads.net - Xen devel - [PATCH, v2] x86/HVM: assorted RTC emulation adjustments [Aug 2012]

If this information is useful, please help other people find it:
Share via:

Jan Beulich

2012-Aug-22 15:17 UTC

[PATCH, v2] x86/HVM: assorted RTC emulation adjustments

- 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

Note that this only improves the situation described in the thread at
http://lists.xen.org/archives/html/xen-devel/2012-08/msg00664.html,
there are still problems with the emulation when invoked at a high rate
as described there.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Small adjustment to the pt_update_irq() change, avoiding to call
    the RTC code for a HPET event (which also may pass 8 [aka RTC_IRQ]
    as create_periodic_time()''s "irq" argument.

--- 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 && pt_priv )
+        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

Stefano Stabellini

2012-Aug-23 13:44 UTC

head link

Re: [PATCH, v2] x86/HVM: assorted RTC emulation adjustments

On Wed, 22 Aug 2012, Jan Beulich wrote:> - 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
> 
> Note that this only improves the situation described in the thread at
> http://lists.xen.org/archives/html/xen-devel/2012-08/msg00664.html,
> there are still problems with the emulation when invoked at a high rate
> as described there.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Although this patch solves a real problem and should probably go in at
some point, I am a bit worried about drifting too much from the original
RTC emulator (that was taken from QEMU), because it would be nice to be
able to backport features like this one:

http://marc.info/?l=qemu-devel&m=134392375010304

> v2: Small adjustment to the pt_update_irq() change, avoiding to call
>     the RTC code for a HPET event (which also may pass 8 [aka RTC_IRQ]
>     as create_periodic_time()''s "irq" argument.
> 
> --- 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 && pt_priv )
> +        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);
> 
> 
>

Jan Beulich

2012-Aug-24 15:45 UTC

head link

Re: [PATCH, v2] x86/HVM: assorted RTC emulation adjustments

>>> On 23.08.12 at 15:44, Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
wrote:> On Wed, 22 Aug 2012, Jan Beulich wrote:
>> - 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
>> 
>> Note that this only improves the situation described in the thread at
>> http://lists.xen.org/archives/html/xen-devel/2012-08/msg00664.html,
>> there are still problems with the emulation when invoked at a high rate
>> as described there.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Although this patch solves a real problem and should probably go in at
> some point, I am a bit worried about drifting too much from the original
> RTC emulator (that was taken from QEMU),
Then does that emulator have similar problems?
> because it would be nice to be able to backport features like this one:
> 
> http://marc.info/?l=qemu-devel&m=134392375010304 
I agree this would be nice to have (albeit I''m not sure how much
the original problem is actually present in the Xen code, particularly
with the patch here applied, as I think it may implicitly clean up some
of the unneccesary active timers).

Jan

Stefano Stabellini

2012-Aug-24 17:34 UTC

head link

Re: [PATCH, v2] x86/HVM: assorted RTC emulation adjustments

On Fri, 24 Aug 2012, Jan Beulich wrote:> >>> On 23.08.12 at 15:44, Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> wrote:
> > On Wed, 22 Aug 2012, Jan Beulich wrote:
> >> - 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
> >> 
> >> Note that this only improves the situation described in the thread
at
> >>
http://lists.xen.org/archives/html/xen-devel/2012-08/msg00664.html,
> >> there are still problems with the emulation when invoked at a high
rate
> >> as described there.
> >> 
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Although this patch solves a real problem and should probably go in at
> > some point, I am a bit worried about drifting too much from the
original
> > RTC emulator (that was taken from QEMU),
> 
> Then does that emulator have similar problems?
I am not sure, it probably does.
I am afraid the code already drifted too much to make comparisons.

> > because it would be nice to be able to backport features like this
one:
> > 
> > http://marc.info/?l=qemu-devel&m=134392375010304 
> 
> I agree this would be nice to have (albeit I''m not sure how much
> the original problem is actually present in the Xen code, particularly
> with the patch here applied, as I think it may implicitly clean up some
> of the unneccesary active timers).
Maybe, but with your patch applied, are there going to be any timers
running if the guest is not making use of the RTC?

Jan Beulich

2012-Aug-24 19:49 UTC

head link

Re: [PATCH, v2] x86/HVM: assorted RTC emulation adjustments

>>> On 24.08.12 at 19:34, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
>> > because it would be nice to be able to backport features like this
one:
>> > 
>> > http://marc.info/?l=qemu-devel&m=134392375010304 
>> 
>> I agree this would be nice to have (albeit I''m not sure how
much
>> the original problem is actually present in the Xen code, particularly
>> with the patch here applied, as I think it may implicitly clean up some
>> of the unneccesary active timers).
> 
> Maybe, but with your patch applied, are there going to be any timers
> running if the guest is not making use of the RTC?
Can''t tell without in depth analysis, as I didn''t pay too much
attention to this aspect of the code. I just vaguely recall that
in at least one case the changes might have resulted in not
re-arming a timer under some condition where it previously
got re-armed.

Jan

Xen devel - Aug 2012 - [PATCH, v2] x86/HVM: assorted RTC emulation adjustments

[PATCH, v2] x86/HVM: assorted RTC emulation adjustments

Re: [PATCH, v2] x86/HVM: assorted RTC emulation adjustments

Re: [PATCH, v2] x86/HVM: assorted RTC emulation adjustments

Re: [PATCH, v2] x86/HVM: assorted RTC emulation adjustments

Re: [PATCH, v2] x86/HVM: assorted RTC emulation adjustments