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

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

[Tim Deegan]

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

1/4 is just a bit of tidying, untangling the IRQ masking logic.
2/4 is the same patch I posted before, to keep the periods aligned.
3/4 avoids calling create_periodic_time() when it's already running.
4/4 holds off disabling the timer for a few ticks, while still
    disabling it if the guest is obviously ignoring the IRQ.

Cheers,

Tim.

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

[Tim Deegan]

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

diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index c1e09d8..368d3c8 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -50,14 +50,26 @@ static void rtc_set_time(RTCState *s);
 static inline int from_bcd(RTCState *s, int a);
 static inline int convert_hour(RTCState *s, int hour);
 
-static void rtc_toggle_irq(RTCState *s)
+static void rtc_update_irq(RTCState *s)
 {
     struct domain *d = vrtc_domain(s);
+    uint8_t irqf;
 
     ASSERT(spin_is_locked(&s->lock));
-    s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
-    hvm_isa_irq_deassert(d, RTC_IRQ);
-    hvm_isa_irq_assert(d, RTC_IRQ);
+
+    /* IRQ is raised if any of the sources is raised & enabled */
+    irqf = (s->hw.cmos_data[RTC_REG_B]
+            & s->hw.cmos_data[RTC_REG_C]
+            & (RTC_PF|RTC_AF|RTC_UF))
+        ? RTC_IRQF : 0;
+
+    s->hw.cmos_data[RTC_REG_C] &= ~RTC_IRQF;
+    s->hw.cmos_data[RTC_REG_C] |= irqf;
+
+    if ( irqf )
+        hvm_isa_irq_assert(d, RTC_IRQ);
+    else
+        hvm_isa_irq_deassert(d, RTC_IRQ);
 }
 
 void rtc_periodic_interrupt(void *opaque)
@@ -70,8 +82,7 @@ void rtc_periodic_interrupt(void *opaque)
     else
     {
         s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
-        if ( s->hw.cmos_data[RTC_REG_B] & RTC_PIE )
-            rtc_toggle_irq(s);
+        rtc_update_irq(s);
     }
     spin_unlock(&s->lock);
 }
@@ -174,8 +185,7 @@ static void rtc_update_timer2(void *opaque)
     {
         s->hw.cmos_data[RTC_REG_C] |= RTC_UF;
         s->hw.cmos_data[RTC_REG_A] &= ~RTC_UIP;
-        if ((s->hw.cmos_data[RTC_REG_B] & RTC_UIE))
-            rtc_toggle_irq(s);
+        rtc_update_irq(s);
         check_update_timer(s);
     }
     spin_unlock(&s->lock);
@@ -365,9 +375,7 @@ static void rtc_alarm_cb(void *opaque)
     if (!(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
     {
         s->hw.cmos_data[RTC_REG_C] |= RTC_AF;
-        /* alarm interrupt */
-        if (s->hw.cmos_data[RTC_REG_B] & RTC_AIE)
-            rtc_toggle_irq(s);
+        rtc_update_irq(s);
         alarm_timer_update(s);
     }
     spin_unlock(&s->lock);
@@ -377,7 +385,7 @@ static int rtc_ioport_write(void *opaque, uint32_t addr, uint32_t data)
 {
     RTCState *s = opaque;
     struct domain *d = vrtc_domain(s);
-    uint32_t orig, mask;
+    uint32_t orig;
 
     spin_lock(&s->lock);
 
@@ -451,15 +459,8 @@ static int rtc_ioport_write(void *opaque, uint32_t addr, uint32_t data)
         /*
          * If the interrupt is already set when the interrupt becomes
          * enabled, raise an interrupt immediately.
-         * NB: RTC_{A,P,U}IE == RTC_{A,P,U}F respectively.
          */
-        for ( mask = RTC_UIE; mask <= RTC_PIE; mask <<= 1 )
-            if ( (data & mask) && !(orig & mask) &&
-                 (s->hw.cmos_data[RTC_REG_C] & mask) )
-            {
-                rtc_toggle_irq(s);
-                break;
-            }
+        rtc_update_irq(s);
         s->hw.cmos_data[RTC_REG_B] = data;
         if ( (data ^ orig) & RTC_SET )
             check_update_timer(s);
@@ -615,8 +616,8 @@ static uint32_t rtc_ioport_read(RTCState *s, uint32_t addr)
         break;
     case RTC_REG_C:
         ret = s->hw.cmos_data[s->hw.cmos_index];
-        hvm_isa_irq_deassert(vrtc_domain(s), RTC_IRQ);
         s->hw.cmos_data[RTC_REG_C] = 0x00;
+        rtc_update_irq(s);
         check_update_timer(s);
         alarm_timer_update(s);
         rtc_timer_update(s);
-- 
1.7.10.4

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

[Tim Deegan]

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

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

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

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

[Tim Deegan]

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

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

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

[Tim Deegan]

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

diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index ecc9916..4c0e7d4 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -77,16 +77,17 @@ void rtc_periodic_interrupt(void *opaque)
     RTCState *s = opaque;
 
     spin_lock(&s->lock);
-    if ( s->hw.cmos_data[RTC_REG_C] & RTC_PF )
-    {
-        destroy_periodic_time(&s->pt);
-        s->pt_active = 0;
-    }
-    else
+    if ( !(s->hw.cmos_data[RTC_REG_C] & RTC_PF) )
     {
         s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
         rtc_update_irq(s);
     }
+    else if ( ++(s->pt_dead_ticks) >= 10 )
+    {
+        /* VM is ignoring its RTC; no point in running the timer */
+        destroy_periodic_time(&s->pt);
+        s->pt_active = 0;
+    }
     spin_unlock(&s->lock);
 }
 
@@ -99,6 +100,8 @@ static void rtc_timer_update(RTCState *s)
 
     ASSERT(spin_is_locked(&s->lock));
 
+    s->pt_dead_ticks = 0;
+
     period_code = s->hw.cmos_data[RTC_REG_A] & RTC_RATE_SELECT;
     switch ( s->hw.cmos_data[RTC_REG_A] & RTC_DIV_CTL )
     {
diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
index 76a83a1..4d274d3 100644
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -115,6 +115,7 @@ typedef struct RTCState {
     s_time_t start_time;
     int period;
     bool_t pt_active;
+    uint8_t pt_dead_ticks;
     uint32_t use_timer;
     spinlock_t lock;
 } RTCState;
-- 
1.7.10.4

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

[Keir Fraser]

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

> When the RTC periodic timer gets restarted, align it to the VM's boot
> time, not to whatever time it is now.  Otherwise every read of REG_C
> will restart the current period
> 
> Signed-off-by: Tim Deegan <tim@xen.org>

Looks rather sensible.

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

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

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

[Andrew Cooper]

On 28/03/2013 13:22, Tim Deegan wrote:
> Signed-off-by: Tim Deegan <tim@xen.org>
> ---
>  xen/arch/x86/hvm/rtc.c |   43 ++++++++++++++++++++++---------------------
>  1 file changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
> index c1e09d8..368d3c8 100644
> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -50,14 +50,26 @@ static void rtc_set_time(RTCState *s);
>  static inline int from_bcd(RTCState *s, int a);
>  static inline int convert_hour(RTCState *s, int hour);
>  
> -static void rtc_toggle_irq(RTCState *s)
> +static void rtc_update_irq(RTCState *s)
>  {
>      struct domain *d = vrtc_domain(s);
> +    uint8_t irqf;
>  
>      ASSERT(spin_is_locked(&s->lock));
> -    s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
> -    hvm_isa_irq_deassert(d, RTC_IRQ);
> -    hvm_isa_irq_assert(d, RTC_IRQ);
> +
> +    /* IRQ is raised if any of the sources is raised & enabled */

s/sources is/sources are/

~Andrew

> +    irqf = (s->hw.cmos_data[RTC_REG_B]
> +            & s->hw.cmos_data[RTC_REG_C]
> +            & (RTC_PF|RTC_AF|RTC_UF))
> +        ? RTC_IRQF : 0;
> +
> +    s->hw.cmos_data[RTC_REG_C] &= ~RTC_IRQF;
> +    s->hw.cmos_data[RTC_REG_C] |= irqf;
> +
> +    if ( irqf )
> +        hvm_isa_irq_assert(d, RTC_IRQ);
> +    else
> +        hvm_isa_irq_deassert(d, RTC_IRQ);
>  }
>  
>  void rtc_periodic_interrupt(void *opaque)
> @@ -70,8 +82,7 @@ void rtc_periodic_interrupt(void *opaque)
>      else
>      {
>          s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
> -        if ( s->hw.cmos_data[RTC_REG_B] & RTC_PIE )
> -            rtc_toggle_irq(s);
> +        rtc_update_irq(s);
>      }
>      spin_unlock(&s->lock);
>  }
> @@ -174,8 +185,7 @@ static void rtc_update_timer2(void *opaque)
>      {
>          s->hw.cmos_data[RTC_REG_C] |= RTC_UF;
>          s->hw.cmos_data[RTC_REG_A] &= ~RTC_UIP;
> -        if ((s->hw.cmos_data[RTC_REG_B] & RTC_UIE))
> -            rtc_toggle_irq(s);
> +        rtc_update_irq(s);
>          check_update_timer(s);
>      }
>      spin_unlock(&s->lock);
> @@ -365,9 +375,7 @@ static void rtc_alarm_cb(void *opaque)
>      if (!(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
>      {
>          s->hw.cmos_data[RTC_REG_C] |= RTC_AF;
> -        /* alarm interrupt */
> -        if (s->hw.cmos_data[RTC_REG_B] & RTC_AIE)
> -            rtc_toggle_irq(s);
> +        rtc_update_irq(s);
>          alarm_timer_update(s);
>      }
>      spin_unlock(&s->lock);
> @@ -377,7 +385,7 @@ static int rtc_ioport_write(void *opaque, uint32_t addr, uint32_t data)
>  {
>      RTCState *s = opaque;
>      struct domain *d = vrtc_domain(s);
> -    uint32_t orig, mask;
> +    uint32_t orig;
>  
>      spin_lock(&s->lock);
>  
> @@ -451,15 +459,8 @@ static int rtc_ioport_write(void *opaque, uint32_t addr, uint32_t data)
>          /*
>           * If the interrupt is already set when the interrupt becomes
>           * enabled, raise an interrupt immediately.
> -         * NB: RTC_{A,P,U}IE == RTC_{A,P,U}F respectively.
>           */
> -        for ( mask = RTC_UIE; mask <= RTC_PIE; mask <<= 1 )
> -            if ( (data & mask) && !(orig & mask) &&
> -                 (s->hw.cmos_data[RTC_REG_C] & mask) )
> -            {
> -                rtc_toggle_irq(s);
> -                break;
> -            }
> +        rtc_update_irq(s);
>          s->hw.cmos_data[RTC_REG_B] = data;
>          if ( (data ^ orig) & RTC_SET )
>              check_update_timer(s);
> @@ -615,8 +616,8 @@ static uint32_t rtc_ioport_read(RTCState *s, uint32_t addr)
>          break;
>      case RTC_REG_C:
>          ret = s->hw.cmos_data[s->hw.cmos_index];
> -        hvm_isa_irq_deassert(vrtc_domain(s), RTC_IRQ);
>          s->hw.cmos_data[RTC_REG_C] = 0x00;
> +        rtc_update_irq(s);
>          check_update_timer(s);
>          alarm_timer_update(s);
>          rtc_timer_update(s);

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

[Jan Beulich]

>>> On 28.03.13 at 14:22, Tim Deegan <tim@xen.org> wrote:
> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -78,7 +78,10 @@ void rtc_periodic_interrupt(void *opaque)
>  
>      spin_lock(&s->lock);
>      if ( s->hw.cmos_data[RTC_REG_C] & RTC_PF )
> +    {
>          destroy_periodic_time(&s->pt);
> +        s->pt_active = 0;
> +    }
>      else
>      {
>          s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
> @@ -109,13 +112,20 @@ static void rtc_timer_update(RTCState *s)
>          {
>              period = 1 << (period_code - 1); /* period in 32 Khz cycles */
>              period = DIV_ROUND(period * 1000000000ULL, 32768); /* in ns */
> -            delta = period - ((NOW() - s->start_time) % period);
> -            create_periodic_time(v, &s->pt, delta, period, RTC_IRQ, NULL, s);
> +            if ( !s->pt_active || (period != s->period) )
> +            {
> +                s->period = period;
> +                s->pt_active = 1;

I think that by storing period_code rather than period, you could
easily fold the two new fields "period" and "pt_active" into one
(because period_code is zero if and only if the timer is inactive).

Beyond that the patch looks mostly like what I had in mind during
our recent discussion.

Jan

> +                delta = period - ((NOW() - s->start_time) % period);
> +                create_periodic_time(v, &s->pt, delta, period,
> +                                     RTC_IRQ, NULL, s);
> +            }
>              break;
>          }
>          /* fall through */
>      default:
>          destroy_periodic_time(&s->pt);
> +        s->pt_active = 0;
>          break;
>      }
>  }
> @@ -717,6 +727,7 @@ void rtc_reset(struct domain *d)
>      RTCState *s = domain_vrtc(d);
>  
>      destroy_periodic_time(&s->pt);
> +    s->pt_active = 0;
>      s->pt.source = PTSRC_isa;
>  }
>  
> diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
> index 2e9c7d2..76a83a1 100644
> --- a/xen/include/asm-x86/hvm/vpt.h
> +++ b/xen/include/asm-x86/hvm/vpt.h
> @@ -104,16 +104,17 @@ typedef struct RTCState {
>      struct hvm_hw_rtc hw;
>      /* RTC's idea of the current time */
>      struct tm current_tm;
> -    /* periodic timer */
> -    s_time_t start_time;
> -    /* second update */
> -    struct periodic_time pt;
>      /* update-ended timer */
>      struct timer update_timer;
>      struct timer update_timer2;
> +    uint64_t next_update_time;
>      /* alarm timer */
>      struct timer alarm_timer;
> -    uint64_t next_update_time;
> +    /* periodic timer */
> +    struct periodic_time pt;
> +    s_time_t start_time;
> +    int period;
> +    bool_t pt_active;
>      uint32_t use_timer;
>      spinlock_t lock;
>  } RTCState;
> -- 
> 1.7.10.4

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

[Tim Deegan]

At 13:33 +0000 on 28 Mar (1364477638), Andrew Cooper wrote:
> On 28/03/2013 13:22, Tim Deegan wrote:
> > Signed-off-by: Tim Deegan <tim@xen.org>
> > ---
> >  xen/arch/x86/hvm/rtc.c |   43 ++++++++++++++++++++++---------------------
> >  1 file changed, 22 insertions(+), 21 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
> > index c1e09d8..368d3c8 100644
> > --- a/xen/arch/x86/hvm/rtc.c
> > +++ b/xen/arch/x86/hvm/rtc.c
> > @@ -50,14 +50,26 @@ static void rtc_set_time(RTCState *s);
> >  static inline int from_bcd(RTCState *s, int a);
> >  static inline int convert_hour(RTCState *s, int hour);
> >  
> > -static void rtc_toggle_irq(RTCState *s)
> > +static void rtc_update_irq(RTCState *s)
> >  {
> >      struct domain *d = vrtc_domain(s);
> > +    uint8_t irqf;
> >  
> >      ASSERT(spin_is_locked(&s->lock));
> > -    s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
> > -    hvm_isa_irq_deassert(d, RTC_IRQ);
> > -    hvm_isa_irq_assert(d, RTC_IRQ);
> > +
> > +    /* IRQ is raised if any of the sources is raised & enabled */
> 
> s/sources is/sources are/

No, thanks. :P

Tim.

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

[Jan Beulich]

>>> On 28.03.13 at 14:22, Tim Deegan <tim@xen.org> wrote:
> Signed-off-by: Tim Deegan <tim@xen.org>
> ---
>  xen/arch/x86/hvm/rtc.c |   43 ++++++++++++++++++++++---------------------
>  1 file changed, 22 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
> index c1e09d8..368d3c8 100644
> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -50,14 +50,26 @@ static void rtc_set_time(RTCState *s);
>  static inline int from_bcd(RTCState *s, int a);
>  static inline int convert_hour(RTCState *s, int hour);
>  
> -static void rtc_toggle_irq(RTCState *s)
> +static void rtc_update_irq(RTCState *s)
>  {
>      struct domain *d = vrtc_domain(s);
> +    uint8_t irqf;
>  
>      ASSERT(spin_is_locked(&s->lock));
> -    s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
> -    hvm_isa_irq_deassert(d, RTC_IRQ);
> -    hvm_isa_irq_assert(d, RTC_IRQ);
> +
> +    /* IRQ is raised if any of the sources is raised & enabled */
> +    irqf = (s->hw.cmos_data[RTC_REG_B]
> +            & s->hw.cmos_data[RTC_REG_C]
> +            & (RTC_PF|RTC_AF|RTC_UF))
> +        ? RTC_IRQF : 0;
> +
> +    s->hw.cmos_data[RTC_REG_C] &= ~RTC_IRQF;
> +    s->hw.cmos_data[RTC_REG_C] |= irqf;
> +
> +    if ( irqf )
> +        hvm_isa_irq_assert(d, RTC_IRQ);
> +    else
> +        hvm_isa_irq_deassert(d, RTC_IRQ);

One fundamental question here is - do you or does anyone else
know why originally the code did this odd looking assert-deassert
sequence. I'm a little afraid that this change may cause observably
different behavior. In particular, our ACPI MADT doesn't (and
shouldn't) declare IRQ8 as level triggered, yet the way you change
the code would seem to make the interrupt behave as if it was.

Jan

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

[Jan Beulich]

>>> On 28.03.13 at 14:22, Tim Deegan <tim@xen.org> wrote:

Interesting. And you say this is the most important one. Gets us
somewhere in the middle of 4.2 and recent code, which I guess is
quite fine. Thanks for working out all this!

Jan

> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -77,16 +77,17 @@ void rtc_periodic_interrupt(void *opaque)
>      RTCState *s = opaque;
>  
>      spin_lock(&s->lock);
> -    if ( s->hw.cmos_data[RTC_REG_C] & RTC_PF )
> -    {
> -        destroy_periodic_time(&s->pt);
> -        s->pt_active = 0;
> -    }
> -    else
> +    if ( !(s->hw.cmos_data[RTC_REG_C] & RTC_PF) )
>      {
>          s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
>          rtc_update_irq(s);
>      }
> +    else if ( ++(s->pt_dead_ticks) >= 10 )
> +    {
> +        /* VM is ignoring its RTC; no point in running the timer */
> +        destroy_periodic_time(&s->pt);
> +        s->pt_active = 0;
> +    }
>      spin_unlock(&s->lock);
>  }
>  
> @@ -99,6 +100,8 @@ static void rtc_timer_update(RTCState *s)
>  
>      ASSERT(spin_is_locked(&s->lock));
>  
> +    s->pt_dead_ticks = 0;
> +
>      period_code = s->hw.cmos_data[RTC_REG_A] & RTC_RATE_SELECT;
>      switch ( s->hw.cmos_data[RTC_REG_A] & RTC_DIV_CTL )
>      {
> diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
> index 76a83a1..4d274d3 100644
> --- a/xen/include/asm-x86/hvm/vpt.h
> +++ b/xen/include/asm-x86/hvm/vpt.h
> @@ -115,6 +115,7 @@ typedef struct RTCState {
>      s_time_t start_time;
>      int period;
>      bool_t pt_active;
> +    uint8_t pt_dead_ticks;
>      uint32_t use_timer;
>      spinlock_t lock;
>  } RTCState;
> -- 
> 1.7.10.4

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

[Tim Deegan]

At 13:41 +0000 on 28 Mar (1364478118), Jan Beulich wrote:
> >>> On 28.03.13 at 14:22, Tim Deegan <tim@xen.org> wrote:
> > +            if ( !s->pt_active || (period != s->period) )
> > +            {
> > +                s->period = period;
> > +                s->pt_active = 1;
> 
> I think that by storing period_code rather than period, you could
> easily fold the two new fields "period" and "pt_active" into one
> (because period_code is zero if and only if the timer is inactive).

Good idea.  Here's v2:

commit bf6591a94ea3f77e40cad5662ab320e37de080c1
Author: Tim Deegan <tim@xen.org>
Date:   Thu Mar 28 12:19:32 2013 +0000

    x86/hvm: Avoid needlessly resetting the periodic timer.
    
    Signed-off-by: Tim Deegan <tim@xen.org>

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

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

[Tim Deegan]

At 13:49 +0000 on 28 Mar (1364478581), Jan Beulich wrote:
> >>> On 28.03.13 at 14:22, Tim Deegan <tim@xen.org> wrote:
> > Signed-off-by: Tim Deegan <tim@xen.org>
> > ---
> >  xen/arch/x86/hvm/rtc.c |   43 ++++++++++++++++++++++---------------------
> >  1 file changed, 22 insertions(+), 21 deletions(-)
> > 
> > diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
> > index c1e09d8..368d3c8 100644
> > --- a/xen/arch/x86/hvm/rtc.c
> > +++ b/xen/arch/x86/hvm/rtc.c
> > @@ -50,14 +50,26 @@ static void rtc_set_time(RTCState *s);
> >  static inline int from_bcd(RTCState *s, int a);
> >  static inline int convert_hour(RTCState *s, int hour);
> >  
> > -static void rtc_toggle_irq(RTCState *s)
> > +static void rtc_update_irq(RTCState *s)
> >  {
> >      struct domain *d = vrtc_domain(s);
> > +    uint8_t irqf;
> >  
> >      ASSERT(spin_is_locked(&s->lock));
> > -    s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
> > -    hvm_isa_irq_deassert(d, RTC_IRQ);
> > -    hvm_isa_irq_assert(d, RTC_IRQ);
> > +
> > +    /* IRQ is raised if any of the sources is raised & enabled */
> > +    irqf = (s->hw.cmos_data[RTC_REG_B]
> > +            & s->hw.cmos_data[RTC_REG_C]
> > +            & (RTC_PF|RTC_AF|RTC_UF))
> > +        ? RTC_IRQF : 0;
> > +
> > +    s->hw.cmos_data[RTC_REG_C] &= ~RTC_IRQF;
> > +    s->hw.cmos_data[RTC_REG_C] |= irqf;
> > +
> > +    if ( irqf )
> > +        hvm_isa_irq_assert(d, RTC_IRQ);
> > +    else
> > +        hvm_isa_irq_deassert(d, RTC_IRQ);
> 
> One fundamental question here is - do you or does anyone else
> know why originally the code did this odd looking assert-deassert
> sequence. I'm a little afraid that this change may cause observably
> different behavior. In particular, our ACPI MADT doesn't (and
> shouldn't) declare IRQ8 as level triggered, yet the way you change
> the code would seem to make the interrupt behave as if it was.

Hmm.  I was following the spec for the MC146818A RTC chip, which is
pretty clearly level-triggered.  Looking at the piix4 spec:

  IRQ 8#. IRQ8# is always an active low edge triggered interrupt and can
  not be modified by software.

I'm not sure why the original code always strobes the line.

Tim.

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

[Jan Beulich]

>>> On 28.03.13 at 15:08, Tim Deegan <tim@xen.org> wrote:
> At 13:49 +0000 on 28 Mar (1364478581), Jan Beulich wrote:
>> >>> On 28.03.13 at 14:22, Tim Deegan <tim@xen.org> wrote:
>> > Signed-off-by: Tim Deegan <tim@xen.org>
>> > ---
>> >  xen/arch/x86/hvm/rtc.c |   43 ++++++++++++++++++++++---------------------
>> >  1 file changed, 22 insertions(+), 21 deletions(-)
>> > 
>> > diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
>> > index c1e09d8..368d3c8 100644
>> > --- a/xen/arch/x86/hvm/rtc.c
>> > +++ b/xen/arch/x86/hvm/rtc.c
>> > @@ -50,14 +50,26 @@ static void rtc_set_time(RTCState *s);
>> >  static inline int from_bcd(RTCState *s, int a);
>> >  static inline int convert_hour(RTCState *s, int hour);
>> >  
>> > -static void rtc_toggle_irq(RTCState *s)
>> > +static void rtc_update_irq(RTCState *s)
>> >  {
>> >      struct domain *d = vrtc_domain(s);
>> > +    uint8_t irqf;
>> >  
>> >      ASSERT(spin_is_locked(&s->lock));
>> > -    s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
>> > -    hvm_isa_irq_deassert(d, RTC_IRQ);
>> > -    hvm_isa_irq_assert(d, RTC_IRQ);
>> > +
>> > +    /* IRQ is raised if any of the sources is raised & enabled */
>> > +    irqf = (s->hw.cmos_data[RTC_REG_B]
>> > +            & s->hw.cmos_data[RTC_REG_C]
>> > +            & (RTC_PF|RTC_AF|RTC_UF))
>> > +        ? RTC_IRQF : 0;
>> > +
>> > +    s->hw.cmos_data[RTC_REG_C] &= ~RTC_IRQF;
>> > +    s->hw.cmos_data[RTC_REG_C] |= irqf;
>> > +
>> > +    if ( irqf )
>> > +        hvm_isa_irq_assert(d, RTC_IRQ);
>> > +    else
>> > +        hvm_isa_irq_deassert(d, RTC_IRQ);
>> 
>> One fundamental question here is - do you or does anyone else
>> know why originally the code did this odd looking assert-deassert
>> sequence. I'm a little afraid that this change may cause observably
>> different behavior. In particular, our ACPI MADT doesn't (and
>> shouldn't) declare IRQ8 as level triggered, yet the way you change
>> the code would seem to make the interrupt behave as if it was.
> 
> Hmm.  I was following the spec for the MC146818A RTC chip, which is
> pretty clearly level-triggered.  Looking at the piix4 spec:
> 
>   IRQ 8#. IRQ8# is always an active low edge triggered interrupt and can
>   not be modified by software.

So first you say level, then you quote edge? Now I'm even more
confused.

Active low is indeed the usual thing for IRQ8, yet our MADT table
doesn't appear to say so. Perhaps the PnP stuff does?

Jan

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

[Tim Deegan]

At 14:22 +0000 on 28 Mar (1364480528), Jan Beulich wrote:
> >>> On 28.03.13 at 15:08, Tim Deegan <tim@xen.org> wrote:
> > At 13:49 +0000 on 28 Mar (1364478581), Jan Beulich wrote:
> >> >>> On 28.03.13 at 14:22, Tim Deegan <tim@xen.org> wrote:
> >> > Signed-off-by: Tim Deegan <tim@xen.org>
> >> > ---
> >> >  xen/arch/x86/hvm/rtc.c |   43 ++++++++++++++++++++++---------------------
> >> >  1 file changed, 22 insertions(+), 21 deletions(-)
> >> > 
> >> > diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
> >> > index c1e09d8..368d3c8 100644
> >> > --- a/xen/arch/x86/hvm/rtc.c
> >> > +++ b/xen/arch/x86/hvm/rtc.c
> >> > @@ -50,14 +50,26 @@ static void rtc_set_time(RTCState *s);
> >> >  static inline int from_bcd(RTCState *s, int a);
> >> >  static inline int convert_hour(RTCState *s, int hour);
> >> >  
> >> > -static void rtc_toggle_irq(RTCState *s)
> >> > +static void rtc_update_irq(RTCState *s)
> >> >  {
> >> >      struct domain *d = vrtc_domain(s);
> >> > +    uint8_t irqf;
> >> >  
> >> >      ASSERT(spin_is_locked(&s->lock));
> >> > -    s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
> >> > -    hvm_isa_irq_deassert(d, RTC_IRQ);
> >> > -    hvm_isa_irq_assert(d, RTC_IRQ);
> >> > +
> >> > +    /* IRQ is raised if any of the sources is raised & enabled */
> >> > +    irqf = (s->hw.cmos_data[RTC_REG_B]
> >> > +            & s->hw.cmos_data[RTC_REG_C]
> >> > +            & (RTC_PF|RTC_AF|RTC_UF))
> >> > +        ? RTC_IRQF : 0;
> >> > +
> >> > +    s->hw.cmos_data[RTC_REG_C] &= ~RTC_IRQF;
> >> > +    s->hw.cmos_data[RTC_REG_C] |= irqf;
> >> > +
> >> > +    if ( irqf )
> >> > +        hvm_isa_irq_assert(d, RTC_IRQ);
> >> > +    else
> >> > +        hvm_isa_irq_deassert(d, RTC_IRQ);
> >> 
> >> One fundamental question here is - do you or does anyone else
> >> know why originally the code did this odd looking assert-deassert
> >> sequence. I'm a little afraid that this change may cause observably
> >> different behavior. In particular, our ACPI MADT doesn't (and
> >> shouldn't) declare IRQ8 as level triggered, yet the way you change
> >> the code would seem to make the interrupt behave as if it was.
> > 
> > Hmm.  I was following the spec for the MC146818A RTC chip, which is
> > pretty clearly level-triggered.  Looking at the piix4 spec:
> > 
> >   IRQ 8#. IRQ8# is always an active low edge triggered interrupt and can
> >   not be modified by software.
> 
> So first you say level, then you quote edge? Now I'm even more
> confused.

Me too.  The original RTC seems to be level, but the piix4 (which ought
to be compatible with it) says edge.  The piix4 spec is pretty unhelpful
about whether it will send a second interrupt if IRQF is already set:

  Interrupt Request Flag (IRQF). Interrupt Request Flag=PF * PIE + AF *
  AIE + UF * UFE. This also causes the CH_IRQ_B signal to be asserted.

No mention of CH_IRQ_B anywhere else in the document.

The existing code is a weird mix: it deasserts the IRQ when REG_C is
read, and also strobes it whenever any of PF, AF or UF is set (and the
corresponding enable bit) - even if that bit is already set.

Tim.

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

[Tim Deegan]

At 13:53 +0000 on 28 Mar (1364478787), Jan Beulich wrote:
> >>> On 28.03.13 at 14:22, Tim Deegan <tim@xen.org> wrote:
> 
> Interesting. And you say this is the most important one.

Yep.  My guess is that by disabling and reenabling the timer we were
missing out on the missed_ticks processing, but I didn't dig much
deeper into it.

Tim.

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

[Jan Beulich]

>>> On 28.03.13 at 15:38, Tim Deegan <tim@xen.org> wrote:
> At 14:22 +0000 on 28 Mar (1364480528), Jan Beulich wrote:
>> >>> On 28.03.13 at 15:08, Tim Deegan <tim@xen.org> wrote:
>> > At 13:49 +0000 on 28 Mar (1364478581), Jan Beulich wrote:
>> >> >>> On 28.03.13 at 14:22, Tim Deegan <tim@xen.org> wrote:
>> >> > -    hvm_isa_irq_deassert(d, RTC_IRQ);
>> >> > -    hvm_isa_irq_assert(d, RTC_IRQ);
>> >> > +
>> >> > +    /* IRQ is raised if any of the sources is raised & enabled */
>> >> > +    irqf = (s->hw.cmos_data[RTC_REG_B]
>> >> > +            & s->hw.cmos_data[RTC_REG_C]
>> >> > +            & (RTC_PF|RTC_AF|RTC_UF))
>> >> > +        ? RTC_IRQF : 0;
>> >> > +
>> >> > +    s->hw.cmos_data[RTC_REG_C] &= ~RTC_IRQF;
>> >> > +    s->hw.cmos_data[RTC_REG_C] |= irqf;
>> >> > +
>> >> > +    if ( irqf )
>> >> > +        hvm_isa_irq_assert(d, RTC_IRQ);
>> >> > +    else
>> >> > +        hvm_isa_irq_deassert(d, RTC_IRQ);
>> >> 
>> >> One fundamental question here is - do you or does anyone else
>> >> know why originally the code did this odd looking assert-deassert
>> >> sequence. I'm a little afraid that this change may cause observably
>> >> different behavior. In particular, our ACPI MADT doesn't (and
>> >> shouldn't) declare IRQ8 as level triggered, yet the way you change
>> >> the code would seem to make the interrupt behave as if it was.
>> > 
>> > Hmm.  I was following the spec for the MC146818A RTC chip, which is
>> > pretty clearly level-triggered.  Looking at the piix4 spec:
>> > 
>> >   IRQ 8#. IRQ8# is always an active low edge triggered interrupt and can
>> >   not be modified by software.
>> 
>> So first you say level, then you quote edge? Now I'm even more
>> confused.
> 
> Me too.  The original RTC seems to be level, but the piix4 (which ought
> to be compatible with it) says edge.  The piix4 spec is pretty unhelpful
> about whether it will send a second interrupt if IRQF is already set:
> 
>   Interrupt Request Flag (IRQF). Interrupt Request Flag=PF * PIE + AF *
>   AIE + UF * UFE. This also causes the CH_IRQ_B signal to be asserted.
> 
> No mention of CH_IRQ_B anywhere else in the document.
> 
> The existing code is a weird mix: it deasserts the IRQ when REG_C is
> read, and also strobes it whenever any of PF, AF or UF is set (and the
> corresponding enable bit) - even if that bit is already set.

And it further doesn't help that we don't even have
vioapic_irq_negative_edge() as counterpart to
vioapic_irq_positive_edge(), i.e. we're not even capable of truly
delivering an active low IRQ. Which sadly makes me feel even more
nervous about your change here. Plus if IRQ8 is active low in our
emulation, then the if and else bodies would need to be switched
(which then wouldn't work because of deassert_irq() being too
simplistic).

Jan

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

[Tim Deegan]

At 15:19 +0000 on 28 Mar (1364483946), Jan Beulich wrote:
> >>> On 28.03.13 at 15:38, Tim Deegan <tim@xen.org> wrote:
> > At 14:22 +0000 on 28 Mar (1364480528), Jan Beulich wrote:
> >> >>> On 28.03.13 at 15:08, Tim Deegan <tim@xen.org> wrote:
> >> > At 13:49 +0000 on 28 Mar (1364478581), Jan Beulich wrote:
> >> >> >>> On 28.03.13 at 14:22, Tim Deegan <tim@xen.org> wrote:
> >> >> > -    hvm_isa_irq_deassert(d, RTC_IRQ);
> >> >> > -    hvm_isa_irq_assert(d, RTC_IRQ);
> >> >> > +
> >> >> > +    /* IRQ is raised if any of the sources is raised & enabled */
> >> >> > +    irqf = (s->hw.cmos_data[RTC_REG_B]
> >> >> > +            & s->hw.cmos_data[RTC_REG_C]
> >> >> > +            & (RTC_PF|RTC_AF|RTC_UF))
> >> >> > +        ? RTC_IRQF : 0;
> >> >> > +
> >> >> > +    s->hw.cmos_data[RTC_REG_C] &= ~RTC_IRQF;
> >> >> > +    s->hw.cmos_data[RTC_REG_C] |= irqf;
> >> >> > +
> >> >> > +    if ( irqf )
> >> >> > +        hvm_isa_irq_assert(d, RTC_IRQ);
> >> >> > +    else
> >> >> > +        hvm_isa_irq_deassert(d, RTC_IRQ);
> >> >> 
> >> >> One fundamental question here is - do you or does anyone else
> >> >> know why originally the code did this odd looking assert-deassert
> >> >> sequence. I'm a little afraid that this change may cause observably
> >> >> different behavior. In particular, our ACPI MADT doesn't (and
> >> >> shouldn't) declare IRQ8 as level triggered, yet the way you change
> >> >> the code would seem to make the interrupt behave as if it was.
> >> > 
> >> > Hmm.  I was following the spec for the MC146818A RTC chip, which is
> >> > pretty clearly level-triggered.  Looking at the piix4 spec:
> >> > 
> >> >   IRQ 8#. IRQ8# is always an active low edge triggered interrupt and can
> >> >   not be modified by software.
> >> 
> >> So first you say level, then you quote edge? Now I'm even more
> >> confused.
> > 
> > Me too.  The original RTC seems to be level, but the piix4 (which ought
> > to be compatible with it) says edge.  The piix4 spec is pretty unhelpful
> > about whether it will send a second interrupt if IRQF is already set:
> > 
> >   Interrupt Request Flag (IRQF). Interrupt Request Flag=PF * PIE + AF *
> >   AIE + UF * UFE. This also causes the CH_IRQ_B signal to be asserted.
> > 
> > No mention of CH_IRQ_B anywhere else in the document.
> > 
> > The existing code is a weird mix: it deasserts the IRQ when REG_C is
> > read, and also strobes it whenever any of PF, AF or UF is set (and the
> > corresponding enable bit) - even if that bit is already set.
> 
> And it further doesn't help that we don't even have
> vioapic_irq_negative_edge() as counterpart to
> vioapic_irq_positive_edge(), i.e. we're not even capable of truly
> delivering an active low IRQ.

I get the impression that the xen IRQ model doesn't actually include a
concept of 'active high' vs 'active low', just 'asserted' or 'not
asserted'.  Keir?

> Which sadly makes me feel even more
> nervous about your change here. Plus if IRQ8 is active low in our
> emulation, then the if and else bodies would need to be switched
> (which then wouldn't work because of deassert_irq() being too
> simplistic).

OK, so should I just respin without patch 1/4 and pretend I never saw
any of this mess? :)

Tim.

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

[Jan Beulich]

>>> On 28.03.13 at 16:30, Tim Deegan <tim@xen.org> wrote:
> At 15:19 +0000 on 28 Mar (1364483946), Jan Beulich wrote:
>> >>> On 28.03.13 at 15:38, Tim Deegan <tim@xen.org> wrote:
>> > At 14:22 +0000 on 28 Mar (1364480528), Jan Beulich wrote:
>> >> >>> On 28.03.13 at 15:08, Tim Deegan <tim@xen.org> wrote:
>> >> > At 13:49 +0000 on 28 Mar (1364478581), Jan Beulich wrote:
>> >> >> >>> On 28.03.13 at 14:22, Tim Deegan <tim@xen.org> wrote:
>> >> >> > -    hvm_isa_irq_deassert(d, RTC_IRQ);
>> >> >> > -    hvm_isa_irq_assert(d, RTC_IRQ);
>> >> >> > +
>> >> >> > +    /* IRQ is raised if any of the sources is raised & enabled */
>> >> >> > +    irqf = (s->hw.cmos_data[RTC_REG_B]
>> >> >> > +            & s->hw.cmos_data[RTC_REG_C]
>> >> >> > +            & (RTC_PF|RTC_AF|RTC_UF))
>> >> >> > +        ? RTC_IRQF : 0;
>> >> >> > +
>> >> >> > +    s->hw.cmos_data[RTC_REG_C] &= ~RTC_IRQF;
>> >> >> > +    s->hw.cmos_data[RTC_REG_C] |= irqf;
>> >> >> > +
>> >> >> > +    if ( irqf )
>> >> >> > +        hvm_isa_irq_assert(d, RTC_IRQ);
>> >> >> > +    else
>> >> >> > +        hvm_isa_irq_deassert(d, RTC_IRQ);
>> >> >> 
>> >> >> One fundamental question here is - do you or does anyone else
>> >> >> know why originally the code did this odd looking assert-deassert
>> >> >> sequence. I'm a little afraid that this change may cause observably
>> >> >> different behavior. In particular, our ACPI MADT doesn't (and
>> >> >> shouldn't) declare IRQ8 as level triggered, yet the way you change
>> >> >> the code would seem to make the interrupt behave as if it was.
>> >> > 
>> >> > Hmm.  I was following the spec for the MC146818A RTC chip, which is
>> >> > pretty clearly level-triggered.  Looking at the piix4 spec:
>> >> > 
>> >> >   IRQ 8#. IRQ8# is always an active low edge triggered interrupt and can
>> >> >   not be modified by software.
>> >> 
>> >> So first you say level, then you quote edge? Now I'm even more
>> >> confused.
>> > 
>> > Me too.  The original RTC seems to be level, but the piix4 (which ought
>> > to be compatible with it) says edge.  The piix4 spec is pretty unhelpful
>> > about whether it will send a second interrupt if IRQF is already set:
>> > 
>> >   Interrupt Request Flag (IRQF). Interrupt Request Flag=PF * PIE + AF *
>> >   AIE + UF * UFE. This also causes the CH_IRQ_B signal to be asserted.
>> > 
>> > No mention of CH_IRQ_B anywhere else in the document.
>> > 
>> > The existing code is a weird mix: it deasserts the IRQ when REG_C is
>> > read, and also strobes it whenever any of PF, AF or UF is set (and the
>> > corresponding enable bit) - even if that bit is already set.
>> 
>> And it further doesn't help that we don't even have
>> vioapic_irq_negative_edge() as counterpart to
>> vioapic_irq_positive_edge(), i.e. we're not even capable of truly
>> delivering an active low IRQ.
> 
> I get the impression that the xen IRQ model doesn't actually include a
> concept of 'active high' vs 'active low', just 'asserted' or 'not
> asserted'.  Keir?
> 
>> Which sadly makes me feel even more
>> nervous about your change here. Plus if IRQ8 is active low in our
>> emulation, then the if and else bodies would need to be switched
>> (which then wouldn't work because of deassert_irq() being too
>> simplistic).
> 
> OK, so should I just respin without patch 1/4 and pretend I never saw
> any of this mess? :)

I'd favor keeping the patch, but with the IRQ raising itself left to
be a strobe:

    if ( irqf )
    {
        hvm_isa_irq_assert(d, RTC_IRQ);
        hvm_isa_irq_deassert(d, RTC_IRQ);
    }

Perhaps say a word about this being bogus in the description.

Jan

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

[Keir Fraser]

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

>> And it further doesn't help that we don't even have
>> vioapic_irq_negative_edge() as counterpart to
>> vioapic_irq_positive_edge(), i.e. we're not even capable of truly
>> delivering an active low IRQ.
> 
> I get the impression that the xen IRQ model doesn't actually include a
> concept of 'active high' vs 'active low', just 'asserted' or 'not
> asserted'.  Keir?

Yes, this is correct. Possibly the naming of v[ioa]pic_irq_positive_edge is
unfortunate! Really it is indicating an asserting edge, regardless of
whether that edge is driving to a high or low voltage.

 -- Keir

>> Which sadly makes me feel even more
>> nervous about your change here. Plus if IRQ8 is active low in our
>> emulation, then the if and else bodies would need to be switched
>> (which then wouldn't work because of deassert_irq() being too
>> simplistic).
> 
> OK, so should I just respin without patch 1/4 and pretend I never saw
> any of this mess? :)
> 
> Tim.