[PATCH v3 0/3] Move RTC interrupt injection back into the vpt code.

[PATCH v3 0/3] Move RTC interrupt injection back into the vpt code.

[Andrew Cooper]

Hi,

This series implements the most recent idea Tim was proposing about
reworking the RTC PF interrupt injection.

Patch 1 switches handling the !PIE case to calculate the right answer
for REG_C.PF on demand rather than running the timers.
Patch 2 switches back to the old model of having the vpt code control
the timer interrupt injection; this is the fix for the w2k3 hang.
Patch 3 is just a minor cleanup, and not particularly necessary.

v3 has undergone extensive testing in XenRT, confirming that the w2k3
has not reoccurred in 100 tests (normally expect to see 10-30
recurrences), and the clock drift tests are happy with the new code.

Roger:
  Would you kindly test against FreeBSD again please?

George:
  Regarding 4.4, I believe these patches are now of sufficient
  quality to be accepted (subject to any other review).

  The previous statements of risk still applies; These are changes to
  a very complicated area of code, and the worst case scenario is that
  a VM gets none/too few/too many timing interrupts, with possible
  clock drift as a problem.  The XenRT test results help alleviate
  concern regarding the worst case.

~Andrew

[PATCH v3 1/3] x86/hvm/rtc: Don't run the vpt timer when !REG_B.PIE.

[Andrew Cooper]

From: Tim Deegan <tim@xen.org>

If the guest has not asked for interrupts, don't run the vpt timer
to generate them.  This is a prerequisite for a patch to simplify how
the vpt interacts with the RTC, and also gets rid of a timer series in
Xen in a case where it's unlikely to be needed.

Instead, calculate the correct value for REG_C.PF whenever REG_C is
read or PIE is enabled.  This allow a guest to poll for the PF bit
while not asking for actual timer interrupts.  Such a guest would no
longer get the benefit of the vpt's timer modes.

Signed-off-by: Tim Deegan <tim@xen.org>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

---

Changes in v3:

 * Always clobber s->period when changing REG_B.PIE, to cause
   rtc_update_timer() to restart the timer when setting REG_B.PIE without
   changing the REG_A divider control.
---
 xen/arch/x86/hvm/rtc.c        |   53 +++++++++++++++++++++++++++++++----------
 xen/include/asm-x86/hvm/vpt.h |    3 ++-
 2 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index cdedefe..1a731f7 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -94,7 +94,7 @@ bool_t rtc_periodic_interrupt(void *opaque)
     {
         /* VM is ignoring its RTC; no point in running the timer */
         destroy_periodic_time(&s->pt);
-        s->pt_code = 0;
+        s->period = 0;
     }
     if ( !(s->hw.cmos_data[RTC_REG_C] & RTC_IRQF) )
         ret = 0;
@@ -103,6 +103,24 @@ bool_t rtc_periodic_interrupt(void *opaque)
     return ret;
 }
 
+/* Check whether the REG_C.PF bit should have been set by a tick since
+ * the last time we looked. This is used to track ticks when REG_B.PIE
+ * is clear; when PIE is set, PF ticks are handled by the VPT callbacks.  */
+static void check_for_pf_ticks(RTCState *s)
+{
+    s_time_t now;
+
+    if ( s->period == 0 || (s->hw.cmos_data[RTC_REG_B] & RTC_PIE) )
+        return;
+
+    now = NOW();
+    if ( (now - s->start_time) / s->period
+         != (s->check_ticks_since - s->start_time) / s->period )
+        s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
+
+    s->check_ticks_since = now;
+}
+
 /* Enable/configure/disable the periodic timer based on the RTC_PIE and
  * RTC_RATE_SELECT settings */
 static void rtc_timer_update(RTCState *s)
@@ -125,24 +143,29 @@ static void rtc_timer_update(RTCState *s)
     case RTC_REF_CLCK_4MHZ:
         if ( period_code != 0 )
         {
-            if ( period_code != s->pt_code )
+            period = 1 << (period_code - 1); /* period in 32 Khz cycles */
+            period = DIV_ROUND(period * 1000000000ULL, 32768); /* in ns */
+            if ( period != s->period )
             {
-                s->pt_code = period_code;
-                period = 1 << (period_code - 1); /* period in 32 Khz cycles */
-                period = DIV_ROUND(period * 1000000000ULL, 32768); /* in ns */
+                s_time_t now = NOW();
+
+                s->period = period;
                 if ( v->domain->arch.hvm_domain.params[HVM_PARAM_VPT_ALIGN] )
                     delta = 0;
                 else
-                    delta = period - ((NOW() - s->start_time) % period);
-                create_periodic_time(v, &s->pt, delta, period,
-                                     RTC_IRQ, NULL, s);
+                    delta = period - ((now - s->start_time) % period);
+                if ( s->hw.cmos_data[RTC_REG_B] & RTC_PIE )
+                    create_periodic_time(v, &s->pt, delta, period,
+                                         RTC_IRQ, NULL, s);
+                else
+                    s->check_ticks_since = now;
             }
             break;
         }
         /* fall through */
     default:
         destroy_periodic_time(&s->pt);
-        s->pt_code = 0;
+        s->period = 0;
         break;
     }
 }
@@ -484,14 +507,19 @@ static int rtc_ioport_write(void *opaque, uint32_t addr, uint32_t data)
             if ( orig & RTC_SET )
                 rtc_set_time(s);
         }
+        check_for_pf_ticks(s);
         s->hw.cmos_data[RTC_REG_B] = data;
         /*
          * If the interrupt is already set when the interrupt becomes
          * enabled, raise an interrupt immediately.
          */
         rtc_update_irq(s);
-        if ( (data & RTC_PIE) && !(orig & RTC_PIE) )
+        if ( (data ^ orig) & RTC_PIE )
+        {
+            destroy_periodic_time(&s->pt);
+            s->period = 0;
             rtc_timer_update(s);
+        }
         if ( (data ^ orig) & RTC_SET )
             check_update_timer(s);
         if ( (data ^ orig) & (RTC_24H | RTC_DM_BINARY | RTC_SET) )
@@ -645,6 +673,7 @@ static uint32_t rtc_ioport_read(RTCState *s, uint32_t addr)
             ret |= RTC_UIP;
         break;
     case RTC_REG_C:
+        check_for_pf_ticks(s);
         ret = s->hw.cmos_data[s->hw.cmos_index];
         s->hw.cmos_data[RTC_REG_C] = 0x00;
         if ( (ret & RTC_IRQF) && !rtc_mode_is(s, no_ack) )
@@ -652,7 +681,7 @@ static uint32_t rtc_ioport_read(RTCState *s, uint32_t addr)
         rtc_update_irq(s);
         check_update_timer(s);
         alarm_timer_update(s);
-        rtc_timer_update(s);
+        s->pt_dead_ticks = 0;
         break;
     default:
         ret = s->hw.cmos_data[s->hw.cmos_index];
@@ -748,7 +777,7 @@ void rtc_reset(struct domain *d)
     RTCState *s = domain_vrtc(d);
 
     destroy_periodic_time(&s->pt);
-    s->pt_code = 0;
+    s->period = 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 87c3a66..9f48635 100644
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -113,7 +113,8 @@ typedef struct RTCState {
     /* periodic timer */
     struct periodic_time pt;
     s_time_t start_time;
-    int pt_code;
+    s_time_t check_ticks_since;
+    int period;
     uint8_t pt_dead_ticks;
     uint32_t use_timer;
     spinlock_t lock;
-- 
1.7.10.4

[PATCH v3 2/3] x86/hvm/rtc: Inject RTC periodic interupts from the vpt code.

[Andrew Cooper]

From: Tim Deegan <tim@xen.org>

Let the vpt code drive the RTC's timer interrupts directly, as it does
for other periodic time sources, and fix up the register state in a
vpt callback when the interrupt is injected.

This fixes a hang seen on Windows 2003 in no-missed-ticks mode, where
when a tick was pending, the early callback from the VPT code would
always set REG_C.PF on every VMENTER; meanwhile the guest was in its
interrupt handler reading REG_C in a loop and waiting to see it clear.

One drawback is that a guest that attempts to suppress RTC periodic
interrupts by failing to read REG_C will receive up to 10 spurious
interrupts, even in 'strict' mode.  However:
 - since all previous RTC models have had this property (including
   the current one, since 'no-ack' mode is hard-coded on) we're
   pretty sure that all guests can handle this; and
 - we're already playing some other interesting games with this
   interrupt in the vpt code.

One other corner case: a guest that enables the PF timer interrupt,
masks the interupt in the APIC and then polls REG_C looking for PF
will not see PF getting set.  The more likely case of enabling the
timers and masking the interrupt with REG_B.PIE is already handled
correctly.

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

diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index 1a731f7..d641d95 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -78,29 +78,26 @@ static void rtc_update_irq(RTCState *s)
     hvm_isa_irq_assert(vrtc_domain(s), RTC_IRQ);
 }
 
-bool_t rtc_periodic_interrupt(void *opaque)
+/* Called by the VPT code after it's injected a PF interrupt for us.
+ * Fix up the register state to reflect what happened. */
+static void rtc_pf_callback(struct vcpu *v, void *opaque)
 {
     RTCState *s = opaque;
-    bool_t ret;
 
     spin_lock(&s->lock);
-    ret = rtc_mode_is(s, no_ack) || !(s->hw.cmos_data[RTC_REG_C] & RTC_IRQF);
-    if ( rtc_mode_is(s, no_ack) || !(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 )
+
+    if ( !rtc_mode_is(s, no_ack)
+         && (s->hw.cmos_data[RTC_REG_C] & RTC_IRQF)
+         && ++(s->pt_dead_ticks) >= 10 )
     {
         /* VM is ignoring its RTC; no point in running the timer */
         destroy_periodic_time(&s->pt);
         s->period = 0;
     }
-    if ( !(s->hw.cmos_data[RTC_REG_C] & RTC_IRQF) )
-        ret = 0;
-    spin_unlock(&s->lock);
 
-    return ret;
+    s->hw.cmos_data[RTC_REG_C] |= RTC_PF|RTC_IRQF;
+
+    spin_unlock(&s->lock);
 }
 
 /* Check whether the REG_C.PF bit should have been set by a tick since
@@ -156,7 +153,7 @@ static void rtc_timer_update(RTCState *s)
                     delta = period - ((now - s->start_time) % period);
                 if ( s->hw.cmos_data[RTC_REG_B] & RTC_PIE )
                     create_periodic_time(v, &s->pt, delta, period,
-                                         RTC_IRQ, NULL, s);
+                                         RTC_IRQ, rtc_pf_callback, s);
                 else
                     s->check_ticks_since = now;
             }
diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
index 1961bda..f7af688 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -231,12 +231,9 @@ int pt_update_irq(struct vcpu *v)
     struct periodic_time *pt, *temp, *earliest_pt;
     uint64_t max_lag;
     int irq, is_lapic;
-    void *pt_priv;
 
- rescan:
     spin_lock(&v->arch.hvm_vcpu.tm_lock);
 
- rescan_locked:
     earliest_pt = NULL;
     max_lag = -1ULL;
     list_for_each_entry_safe ( pt, temp, head, list )
@@ -270,48 +267,11 @@ int 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 )
-    {
-        if ( !rtc_periodic_interrupt(pt_priv) )
-            irq = -1;
-
-        pt_lock(earliest_pt);
-
-        if ( irq < 0 && earliest_pt->pending_intr_nr )
-        {
-            /*
-             * RTC periodic timer runs without the corresponding interrupt
-             * being enabled - need to mimic enough of pt_intr_post() to keep
-             * things going.
-             */
-            earliest_pt->pending_intr_nr = 0;
-            earliest_pt->irq_issued = 0;
-            set_timer(&earliest_pt->timer, earliest_pt->scheduled);
-        }
-        else if ( irq >= 0 && pt_irq_masked(earliest_pt) )
-        {
-            if ( earliest_pt->on_list )
-            {
-                /* suspend timer emulation */
-                list_del(&earliest_pt->list);
-                earliest_pt->on_list = 0;
-            }
-            irq = -1;
-        }
-
-        /* Avoid dropping the lock if we can. */
-        if ( irq < 0 && v == earliest_pt->vcpu )
-            goto rescan_locked;
-        pt_unlock(earliest_pt);
-        if ( irq < 0 )
-            goto rescan;
-    }
     else
     {
         hvm_isa_irq_deassert(v->domain, irq);
diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
index 9f48635..7d62653 100644
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -184,7 +184,6 @@ 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);
-bool_t rtc_periodic_interrupt(void *);
 
 void pmtimer_init(struct vcpu *v);
 void pmtimer_deinit(struct domain *d);
-- 
1.7.10.4

[PATCH v3 3/3] x86/hvm/rtc: Always deassert the IRQ line when clearing REG_C.IRQF.

[Andrew Cooper]

From: Tim Deegan <tim@xen.org>

Even in no-ack mode, there's no reason to leave the line asserted
after an explicit ack of the interrupt.

Furthermore, rtc_update_irq() is an unconditional noop having just cleared
RTC_REG_C.

Signed-off-by: Tim Deegan <tim@xen.org>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/rtc.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index d641d95..639b4c5 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -673,9 +673,8 @@ static uint32_t rtc_ioport_read(RTCState *s, uint32_t addr)
         check_for_pf_ticks(s);
         ret = s->hw.cmos_data[s->hw.cmos_index];
         s->hw.cmos_data[RTC_REG_C] = 0x00;
-        if ( (ret & RTC_IRQF) && !rtc_mode_is(s, no_ack) )
+        if ( ret & RTC_IRQF )
             hvm_isa_irq_deassert(d, RTC_IRQ);
-        rtc_update_irq(s);
         check_update_timer(s);
         alarm_timer_update(s);
         s->pt_dead_ticks = 0;
-- 
1.7.10.4

Re: [PATCH v3 0/3] Move RTC interrupt injection back into the vpt code.

[Jan Beulich]

>>> On 17.02.14 at 12:29, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> This series implements the most recent idea Tim was proposing about
> reworking the RTC PF interrupt injection.
> 
> Patch 1 switches handling the !PIE case to calculate the right answer
> for REG_C.PF on demand rather than running the timers.
> Patch 2 switches back to the old model of having the vpt code control
> the timer interrupt injection; this is the fix for the w2k3 hang.
> Patch 3 is just a minor cleanup, and not particularly necessary.

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

Re: [PATCH v3 0/3] Move RTC interrupt injection back into the vpt code.

[Roger Pau Monné]

On 17/02/14 12:29, Andrew Cooper wrote:
> Hi,
> 
> This series implements the most recent idea Tim was proposing about
> reworking the RTC PF interrupt injection.
> 
> Patch 1 switches handling the !PIE case to calculate the right answer
> for REG_C.PF on demand rather than running the timers.
> Patch 2 switches back to the old model of having the vpt code control
> the timer interrupt injection; this is the fix for the w2k3 hang.
> Patch 3 is just a minor cleanup, and not particularly necessary.
> 
> v3 has undergone extensive testing in XenRT, confirming that the w2k3
> has not reoccurred in 100 tests (normally expect to see 10-30
> recurrences), and the clock drift tests are happy with the new code.
> 
> Roger:
>   Would you kindly test against FreeBSD again please?

Tested-by: Roger Pau Monné <roger.pau@citrix.com>
On FreeBSD 10.0, 9.2 and 8.4.