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

[PATCH RFC v2 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.

v2 now appears to work correctly, given my dev testing.  I am setting XenRT up
over the weekend to give it some thorough testing.

~Andrew

[PATCH RFC v2 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 v2:
 * Reduce scope of `now` in rtc_timer_update()
 * Merge PIE logic in REG_B write
 * Tightly couple setting s->period with creating/destroying timers, so the
   timer change gets properly recreated when the guest sets REG_B.PIE
---
 xen/arch/x86/hvm/rtc.c        |   58 ++++++++++++++++++++++++++++++++---------
 xen/include/asm-x86/hvm/vpt.h |    3 ++-
 2 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index cdedefe..1455bc6 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,31 @@ 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();
+
                 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);
+                    s->period = period;
+                }
+                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 +509,22 @@ 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 )
+        {
+            if ( !(data & 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 +678,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 +686,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 +782,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 RFC v2 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 1455bc6..7a37ebb 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)
                 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);
                     s->period = period;
                 }
                 else
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 RFC v2 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 7a37ebb..1844f2d 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -678,9 +678,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 RFC v2 1/3] x86/hvm/rtc: Don't run the vpt timer when !REG_B.PIE.

[Tim Deegan]

At 19:51 +0000 on 14 Feb (1392403919), Andrew Cooper wrote:
> 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 v2:
>  * Reduce scope of `now` in rtc_timer_update()
>  * Merge PIE logic in REG_B write
>  * Tightly couple setting s->period with creating/destroying timers, so the
>    timer change gets properly recreated when the guest sets REG_B.PIE

Thanks for sorting out that bug, but I think in this version the !PIE
case won't work.  check_for_pf_ticks() uses s->period to figure out
whether to set PF, so it needs to be set whenever the REG_A selector
is configured, even if the timer's not running.

It looks like always setting s->period == 0 just before the call to
rtc_timer_update in the REG_B write (i.e. not just in the !PIE case)
would DTRT, but is that what you tried earlier?

Er, that is, here:

>          rtc_update_irq(s);
> -        if ( (data & RTC_PIE) && !(orig & RTC_PIE) )
> +        if ( (data ^ orig) & RTC_PIE )
> +        {
> +            if ( !(data & RTC_PIE) )
> +            {
> +                destroy_periodic_time(&s->pt);
> +                s->period = 0;
> +            }
>              rtc_timer_update(s);
> +        }
>          if ( (data ^ orig) & RTC_SET )

do this:

> +        if ( (data ^ orig) & RTC_PIE )
> +        {
> +            destroy_periodic_time(&s->pt);
> +            s->period = 0;
>              rtc_timer_update(s);
> +        }

Tim.

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

[Andrew Cooper]

On 14/02/2014 23:03, Tim Deegan wrote:
> At 19:51 +0000 on 14 Feb (1392403919), Andrew Cooper wrote:
>> 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 v2:
>>  * Reduce scope of `now` in rtc_timer_update()
>>  * Merge PIE logic in REG_B write
>>  * Tightly couple setting s->period with creating/destroying timers, so the
>>    timer change gets properly recreated when the guest sets REG_B.PIE
> Thanks for sorting out that bug, but I think in this version the !PIE
> case won't work.  check_for_pf_ticks() uses s->period to figure out
> whether to set PF, so it needs to be set whenever the REG_A selector
> is configured, even if the timer's not running.
>
> It looks like always setting s->period == 0 just before the call to
> rtc_timer_update in the REG_B write (i.e. not just in the !PIE case)
> would DTRT, but is that what you tried earlier?

I tried the "careful clobbering" which resulted in the fragment below,
and altering the position of s->period in rtc_timer_update() to match,
but that does indeed lead to issues of s->period being 0 if !PIE.

I clearly should have been less careful.

>
> Er, that is, here:
>
>>          rtc_update_irq(s);
>> -        if ( (data & RTC_PIE) && !(orig & RTC_PIE) )
>> +        if ( (data ^ orig) & RTC_PIE )
>> +        {
>> +            if ( !(data & RTC_PIE) )
>> +            {
>> +                destroy_periodic_time(&s->pt);
>> +                s->period = 0;
>> +            }
>>              rtc_timer_update(s);
>> +        }
>>          if ( (data ^ orig) & RTC_SET )
> do this:
>
>> +        if ( (data ^ orig) & RTC_PIE )
>> +        {
>> +            destroy_periodic_time(&s->pt);
>> +            s->period = 0;
>>              rtc_timer_update(s);
>> +        }
> Tim.

... and undo the changes in rtc_timer_update().

~Andrew