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

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

[Tim Deegan]

Hi,

This series implements the most recent idea I 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.

N.B. In its current state it DOES NOT WORK.  I got distracted by
other things today and didn't get a chance to finish working on it,
but I wanted to send it out for feedback on the general approach.
If it seems broadly acceptable then either I can pick it up again next
week or maybe Andrew can look at fixing it.

Cheers,

Tim.

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

[Tim Deegan]

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>
---
 xen/arch/x86/hvm/rtc.c        | 52 ++++++++++++++++++++++++++++++++++---------
 xen/include/asm-x86/hvm/vpt.h |  3 ++-
 2 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index cdedefe..cfc1af9 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,11 +103,30 @@ 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)
 {
     int period_code, period, delta;
+    s_time_t now;
     struct vcpu *v = vrtc_vcpu(s);
 
     ASSERT(spin_is_locked(&s->lock));
@@ -125,24 +144,28 @@ static void rtc_timer_update(RTCState *s)
     case RTC_REF_CLCK_4MHZ:
         if ( period_code != 0 )
         {
-            if ( period_code != s->pt_code )
+            now = NOW();
+            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->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,6 +507,7 @@ 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
@@ -492,6 +516,11 @@ static int rtc_ioport_write(void *opaque, uint32_t addr, uint32_t data)
         rtc_update_irq(s);
         if ( (data & RTC_PIE) && !(orig & RTC_PIE) )
             rtc_timer_update(s);
+        else if ( !(data & RTC_PIE) && (orig & RTC_PIE) )
+        {
+            destroy_periodic_time(&s->pt);
+            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 +674,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 +682,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 +778,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.8.5.2

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

[Tim Deegan]

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 cfc1af9..18a4fe8 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.8.5.2

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

[Tim Deegan]

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

Signed-off-by: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/hvm/rtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index 18a4fe8..b592547 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -674,7 +674,7 @@ 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);
-- 
1.8.5.2

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

[Andrew Cooper]

On 13/02/14 17:32, Tim Deegan wrote:
> Hi,
>
> This series implements the most recent idea I 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.
>
> N.B. In its current state it DOES NOT WORK.  I got distracted by
> other things today and didn't get a chance to finish working on it,
> but I wanted to send it out for feedback on the general approach.
> If it seems broadly acceptable then either I can pick it up again next
> week or maybe Andrew can look at fixing it.
>
> Cheers,
>
> Tim.
>

I should have time to look at the series tomorrow.

~Andrew

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

[George Dunlap]

On Thu, Feb 13, 2014 at 5:39 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 13/02/14 17:32, Tim Deegan wrote:
>> Hi,
>>
>> This series implements the most recent idea I 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.
>>
>> N.B. In its current state it DOES NOT WORK.  I got distracted by
>> other things today and didn't get a chance to finish working on it,
>> but I wanted to send it out for feedback on the general approach.
>> If it seems broadly acceptable then either I can pick it up again next
>> week or maybe Andrew can look at fixing it.
>>
>> Cheers,
>>
>> Tim.
>>
>
> I should have time to look at the series tomorrow.

The next question to ask is this:

This is the last big disruptive bug / bugfix on my list.  We're
planning on cutting an RC Monday probably, with a test day Tuesday.
This bug was originally marked as "Not for 4.4".

So our options are:
* Delay the release, waiting for this new series to be ready
* Take the patch Andy posted last week for now, and backport Tim's fix
when it's ready
* Release without this bug being fixed

As a reminder (for those who haven't been following the thread), the
effect of this bug is that w2k3 guests sometimes hang during boot.
I'm not sure exactly how often this is, but from talking to Andy it
seems to be fairly low -- one percent maybe?  The code is very subtle
and any change may risk causing similar hangs in other situations; in
particular we would want to be able to test it pretty well.

At the moment I'm leaning towards not delaying the release for it.
That could either mean checking the patch we have to hand today (so it
can make it into the RC Monday hopefully), or just going without it.

Any thoughts?

 -George

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

[Jan Beulich]

>>> On 13.02.14 at 18:32, Tim Deegan <tim@xen.org> wrote:
> 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>

Looks okay to me. Two minor comments below.

> @@ -125,24 +144,28 @@ static void rtc_timer_update(RTCState *s)
>      case RTC_REF_CLCK_4MHZ:
>          if ( period_code != 0 )
>          {
> -            if ( period_code != s->pt_code )
> +            now = NOW();

This is needed only inside the next if, so perhaps move it there (and
I'd prefer the variable declaration to be moved there too).

> +            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->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;
>          }
> @@ -492,6 +516,11 @@ static int rtc_ioport_write(void *opaque, uint32_t addr, uint32_t data)
>          rtc_update_irq(s);
>          if ( (data & RTC_PIE) && !(orig & RTC_PIE) )
>              rtc_timer_update(s);
> +        else if ( !(data & RTC_PIE) && (orig & RTC_PIE) )
> +        {
> +            destroy_periodic_time(&s->pt);
> +            rtc_timer_update(s);
> +        }

I think these two paths should be folded, along the lines of

        if ( (data ^ orig) & RTC_PIE )
        {
            if ( !(data & RTC_PIE) )
                destroy_periodic_time(&s->pt);
            rtc_timer_update(s);
        }

Jan

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

[Jan Beulich]

>>> On 13.02.14 at 18:32, Tim Deegan <tim@xen.org> wrote:
> 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>

Looks plausible too, and the revert it is effectively doing is
obviously acceptable now with patch 1 in place. Curious to know
what's still broken with it...

Jan

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

[Andrew Cooper]

On 14/02/14 11:52, George Dunlap wrote:
> On Thu, Feb 13, 2014 at 5:39 PM, Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>> On 13/02/14 17:32, Tim Deegan wrote:
>>> Hi,
>>>
>>> This series implements the most recent idea I 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.
>>>
>>> N.B. In its current state it DOES NOT WORK.  I got distracted by
>>> other things today and didn't get a chance to finish working on it,
>>> but I wanted to send it out for feedback on the general approach.
>>> If it seems broadly acceptable then either I can pick it up again next
>>> week or maybe Andrew can look at fixing it.
>>>
>>> Cheers,
>>>
>>> Tim.
>>>
>> I should have time to look at the series tomorrow.
> The next question to ask is this:
>
> This is the last big disruptive bug / bugfix on my list.  We're
> planning on cutting an RC Monday probably, with a test day Tuesday.
> This bug was originally marked as "Not for 4.4".

It was originally for 4.4 when I proposed the very first fix, then not
for 4.4 when it was clear my first fix didn't work and I had to start
from scratch.  The efforts for fixing now is simply because of getting
some traction on the problem.

>
> So our options are:
> * Delay the release, waiting for this new series to be ready
> * Take the patch Andy posted last week for now, and backport Tim's fix
> when it's ready
> * Release without this bug being fixed
>
> As a reminder (for those who haven't been following the thread), the
> effect of this bug is that w2k3 guests sometimes hang during boot.
> I'm not sure exactly how often this is, but from talking to Andy it
> seems to be fairly low -- one percent maybe?  The code is very subtle
> and any change may risk causing similar hangs in other situations; in
> particular we would want to be able to test it pretty well.
>
> At the moment I'm leaning towards not delaying the release for it.
> That could either mean checking the patch we have to hand today (so it
> can make it into the RC Monday hopefully), or just going without it.
>
> Any thoughts?
>
>  -George

The bug has been present in Xen since the 4.3 dev cycle, meaning that
All Xen-4.3.x are susceptible.

I do not believe holding the release for a fix is sensible, especially
as the fix is still not considered ready yet

Also, I do not advise taking last weeks patch as an interim fix.  As
repeatedly proved, this is very tricky to get right, and it is less risk
to leave it as-is.


I will attempt to get the series working, and if it is well tested and
still within an acceptable timeframe for 4.4 then it should go in.  If
not, releasing with this bug is not the end of the world.

~Andrew

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

[Jan Beulich]

>>> On 13.02.14 at 18:32, Tim Deegan <tim@xen.org> wrote:
> Even in no-ack mode, there's no reason to leave the line asserted
> after an explicit ack of the interrupt.
> 
> Signed-off-by: Tim Deegan <tim@xen.org>
> ---
>  xen/arch/x86/hvm/rtc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
> index 18a4fe8..b592547 100644
> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -674,7 +674,7 @@ 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);

Wait - does one of the earlier patches remove the other de-assert?
Looking... No, they don't. Doing it in exactly one of the two places
should be sufficient, shouldn't it? The more that the other de-assert
is in rtc_update_irq(), which is being called right afterwards. 

But then again - that call seems pointless (I think I mentioned this in
response to Andrew's first attempt): Since REG_C is now clear, the
first conditional return path in that function will never be taken, and
the second one always will be.

So I think together with removing that call, and considering the
intended level-ness of the IRQ, I think I agree with you after all,
provided two de-asserts with no assert in between are not a
problem.

Jan

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

[Jan Beulich]

>>> On 14.02.14 at 12:52, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
> On Thu, Feb 13, 2014 at 5:39 PM, Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>> On 13/02/14 17:32, Tim Deegan wrote:
>>> Hi,
>>>
>>> This series implements the most recent idea I 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.
>>>
>>> N.B. In its current state it DOES NOT WORK.  I got distracted by
>>> other things today and didn't get a chance to finish working on it,
>>> but I wanted to send it out for feedback on the general approach.
>>> If it seems broadly acceptable then either I can pick it up again next
>>> week or maybe Andrew can look at fixing it.
>>>
>>> Cheers,
>>>
>>> Tim.
>>>
>>
>> I should have time to look at the series tomorrow.
> 
> The next question to ask is this:
> 
> This is the last big disruptive bug / bugfix on my list.  We're
> planning on cutting an RC Monday probably, with a test day Tuesday.
> This bug was originally marked as "Not for 4.4".
> 
> So our options are:
> * Delay the release, waiting for this new series to be ready
> * Take the patch Andy posted last week for now, and backport Tim's fix
> when it's ready
> * Release without this bug being fixed
> 
> As a reminder (for those who haven't been following the thread), the
> effect of this bug is that w2k3 guests sometimes hang during boot.
> I'm not sure exactly how often this is, but from talking to Andy it
> seems to be fairly low -- one percent maybe?  The code is very subtle
> and any change may risk causing similar hangs in other situations; in
> particular we would want to be able to test it pretty well.
> 
> At the moment I'm leaning towards not delaying the release for it.
> That could either mean checking the patch we have to hand today (so it
> can make it into the RC Monday hopefully), or just going without it.

I thought we already agreed that Andrew's patch as-is is not an
option. And with the same code being in 4.3, leaving this issue
un-fixed if no sufficiently tested patch becomes ready in time seems
like the better option. Once we have a fix, and it got some extended
exposure in -unstable, we could then still backport it to both 4.4.x
and 4.3.x.

Jan

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

[Tim Deegan]

At 13:32 +0000 on 14 Feb (1392381121), Andrew Cooper wrote:
> On 14/02/14 11:52, George Dunlap wrote:
> > At the moment I'm leaning towards not delaying the release for it.
> > That could either mean checking the patch we have to hand today (so it
> > can make it into the RC Monday hopefully), or just going without it.
> 
> The bug has been present in Xen since the 4.3 dev cycle, meaning that
> All Xen-4.3.x are susceptible.
> 
> I do not believe holding the release for a fix is sensible, especially
> as the fix is still not considered ready yet

Agreed.  Since this isn't a 4.4 regression, we should just let the
release go ahead, and apply whatever the final fix is to the 4.4.x branch.

Tim.

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

[Tim Deegan]

At 13:27 +0000 on 14 Feb (1392380855), Jan Beulich wrote:
> >>> On 13.02.14 at 18:32, Tim Deegan <tim@xen.org> wrote:
> > 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>
> 
> Looks okay to me. Two minor comments below.
> 
> > @@ -125,24 +144,28 @@ static void rtc_timer_update(RTCState *s)
> >      case RTC_REF_CLCK_4MHZ:
> >          if ( period_code != 0 )
> >          {
> > -            if ( period_code != s->pt_code )
> > +            now = NOW();
> 
> This is needed only inside the next if, so perhaps move it there (and
> I'd prefer the variable declaration to be moved there too).

Yep.  That's an oversight, after an earlier version that set the 
check_ticks_since in more places.

> > @@ -492,6 +516,11 @@ static int rtc_ioport_write(void *opaque, uint32_t addr, uint32_t data)
> >          rtc_update_irq(s);
> >          if ( (data & RTC_PIE) && !(orig & RTC_PIE) )
> >              rtc_timer_update(s);
> > +        else if ( !(data & RTC_PIE) && (orig & RTC_PIE) )
> > +        {
> > +            destroy_periodic_time(&s->pt);
> > +            rtc_timer_update(s);
> > +        }
> 
> I think these two paths should be folded, along the lines of
> 
>         if ( (data ^ orig) & RTC_PIE )
>         {
>             if ( !(data & RTC_PIE) )
>                 destroy_periodic_time(&s->pt);
>             rtc_timer_update(s);
>         }

Sure. 

Tim.

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

[Tim Deegan]

At 13:46 +0000 on 14 Feb (1392381999), Jan Beulich wrote:
> >>> On 13.02.14 at 18:32, Tim Deegan <tim@xen.org> wrote:
> > Even in no-ack mode, there's no reason to leave the line asserted
> > after an explicit ack of the interrupt.
> > 
> > Signed-off-by: Tim Deegan <tim@xen.org>
> > ---
> >  xen/arch/x86/hvm/rtc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
> > index 18a4fe8..b592547 100644
> > --- a/xen/arch/x86/hvm/rtc.c
> > +++ b/xen/arch/x86/hvm/rtc.c
> > @@ -674,7 +674,7 @@ 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);
> 
> Wait - does one of the earlier patches remove the other de-assert?
> Looking... No, they don't. Doing it in exactly one of the two places
> should be sufficient, shouldn't it?

Yes; it just seemed odd to leave it asserted in some cases when the
hardware wouldn't.  Given that we know the line is never shared and
that it should always be edge-sensitive in the IOAPIC, it doesn't
matter very much. 

> The more that the other de-assert
> is in rtc_update_irq(), which is being called right afterwards. 
> 
> But then again - that call seems pointless (I think I mentioned this in
> response to Andrew's first attempt): Since REG_C is now clear, the
> first conditional return path in that function will never be taken, and
> the second one always will be.
>
> So I think together with removing that call, and considering the
> intended level-ness of the IRQ, I think I agree with you after all,

Right; this patch could just drop that call too.  I think the original
idea was that having all the irq frobbing in one place was a good
idea.  So maybe the better choice is to make sure that
rtc_update_irq() DTRT and just drop the deassert from here altogether.

> provided two de-asserts with no assert in between are not a
> problem.

They shouldn't be -- the _isa_irq_[de]assert operations are idempotent.

Tim.