[PATCH 0/6] x86/HVM: RTC/VPT adjustments

[PATCH 0/6] x86/HVM: RTC/VPT adjustments

[Jan Beulich]

1: fix processing of RTC REG_B writes
2: slightly adjust RTC reset
3: adjust IRQ (de-)assertion
4: properly handle RTC periodic timer even when !RTC_PIE
5: fix legacy PIC check in pt_update_irq()
6: RTC code must be in line with WAET flags passed by hvmloader

This fixes the Win2003 boot failure George reported. Roger, since
the first patch is slightly different from what you tested earlier,
could you re-test that patch alone and the full series against
FreeBSD?

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

[PATCH 1/6] x86/HVM: fix processing of RTC REG_B writes

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]

We must store the new values before calling rtc_update_irq(), and we
need to call rtc_timer_update() when PIE transitions from 0 to 1 (as we
may have previously turned off the periodic timer due to the guest not
reading REG_C, and hence may have to re-enable it in order to start
IRQs getting delivered to the guest).

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

--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -468,12 +468,14 @@ static int rtc_ioport_write(void *opaque
             if ( orig & RTC_SET )
                 rtc_set_time(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);
-        s->hw.cmos_data[RTC_REG_B] = data;
+        if ( (data & RTC_PIE) && !(orig & RTC_PIE) )
+            rtc_timer_update(s);
         if ( (data ^ orig) & RTC_SET )
             check_update_timer(s);
         if ( (data ^ orig) & (RTC_24H | RTC_DM_BINARY | RTC_SET) )




[-- Attachment #2: x86-HVM-RTC-REG_B-write.patch --]
[-- Type: text/plain, Size: 1138 bytes --]

x86/HVM: fix processing of RTC REG_B writes

We must store the new values before calling rtc_update_irq(), and we
need to call rtc_timer_update() when PIE transitions from 0 to 1 (as we
may have previously turned off the periodic timer due to the guest not
reading REG_C, and hence may have to re-enable it in order to start
IRQs getting delivered to the guest).

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

--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -468,12 +468,14 @@ static int rtc_ioport_write(void *opaque
             if ( orig & RTC_SET )
                 rtc_set_time(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);
-        s->hw.cmos_data[RTC_REG_B] = data;
+        if ( (data & RTC_PIE) && !(orig & RTC_PIE) )
+            rtc_timer_update(s);
         if ( (data ^ orig) & RTC_SET )
             check_update_timer(s);
         if ( (data ^ orig) & (RTC_24H | RTC_DM_BINARY | RTC_SET) )

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 2/6] x86/HVM: slightly adjust RTC reset

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 595 bytes --]

We should clear the interrupt enable flags here, deassert the IRQ, and
clear REG_C.

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

--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -732,6 +732,11 @@ void rtc_reset(struct domain *d)
     destroy_periodic_time(&s->pt);
     s->pt_code = 0;
     s->pt.source = PTSRC_isa;
+
+    s->hw.cmos_data[RTC_REG_B] &= ~(RTC_PIE | RTC_AIE | RTC_UIE);
+    if ( s->hw.cmos_data[RTC_REG_C] & RTC_IRQF )
+        hvm_isa_irq_deassert(d, RTC_IRQ);
+    s->hw.cmos_data[RTC_REG_C] = 0;
 }
 
 void rtc_init(struct domain *d)




[-- Attachment #2: x86-HVM-RTC-reset.patch --]
[-- Type: text/plain, Size: 627 bytes --]

x86/HVM: slightly adjust RTC reset

We should clear the interrupt enable flags here, deassert the IRQ, and
clear REG_C.

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

--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -732,6 +732,11 @@ void rtc_reset(struct domain *d)
     destroy_periodic_time(&s->pt);
     s->pt_code = 0;
     s->pt.source = PTSRC_isa;
+
+    s->hw.cmos_data[RTC_REG_B] &= ~(RTC_PIE | RTC_AIE | RTC_UIE);
+    if ( s->hw.cmos_data[RTC_REG_C] & RTC_IRQF )
+        hvm_isa_irq_deassert(d, RTC_IRQ);
+    s->hw.cmos_data[RTC_REG_C] = 0;
 }
 
 void rtc_init(struct domain *d)

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 3/6] x86/HVM: adjust IRQ (de-)assertion

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1624 bytes --]

De-assertion should only happen when RTC_IRQF gets cleared, i.e. upon
REG_C reads. Assertion should be done only when the flag transitions
from 0 to 1.

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

--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -52,23 +52,19 @@ static inline int convert_hour(RTCState 
 
 static void rtc_update_irq(RTCState *s)
 {
-    struct domain *d = vrtc_domain(s);
-    uint8_t irqf;
-
     ASSERT(spin_is_locked(&s->lock));
 
+    if ( s->hw.cmos_data[RTC_REG_C] & RTC_IRQF )
+        return;
+
     /* IRQ is raised if any source is both 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;
-
-    hvm_isa_irq_deassert(d, RTC_IRQ);
-    if ( irqf )
-        hvm_isa_irq_assert(d, RTC_IRQ);
+    if ( !(s->hw.cmos_data[RTC_REG_B] &
+           s->hw.cmos_data[RTC_REG_C] &
+           (RTC_PF | RTC_AF | RTC_UF)) )
+        return;
+
+    s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
+    hvm_isa_irq_assert(vrtc_domain(s), RTC_IRQ);
 }
 
 void rtc_periodic_interrupt(void *opaque)
@@ -631,6 +627,8 @@ static uint32_t rtc_ioport_read(RTCState
     case RTC_REG_C:
         ret = s->hw.cmos_data[s->hw.cmos_index];
         s->hw.cmos_data[RTC_REG_C] = 0x00;
+        if ( ret & RTC_IRQF )
+            hvm_isa_irq_deassert(d, RTC_IRQ);
         rtc_update_irq(s);
         check_update_timer(s);
         alarm_timer_update(s);




[-- Attachment #2: x86-HVM-RTC-IRQ-assert.patch --]
[-- Type: text/plain, Size: 1656 bytes --]

x86/HVM: adjust IRQ (de-)assertion

De-assertion should only happen when RTC_IRQF gets cleared, i.e. upon
REG_C reads. Assertion should be done only when the flag transitions
from 0 to 1.

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

--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -52,23 +52,19 @@ static inline int convert_hour(RTCState 
 
 static void rtc_update_irq(RTCState *s)
 {
-    struct domain *d = vrtc_domain(s);
-    uint8_t irqf;
-
     ASSERT(spin_is_locked(&s->lock));
 
+    if ( s->hw.cmos_data[RTC_REG_C] & RTC_IRQF )
+        return;
+
     /* IRQ is raised if any source is both 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;
-
-    hvm_isa_irq_deassert(d, RTC_IRQ);
-    if ( irqf )
-        hvm_isa_irq_assert(d, RTC_IRQ);
+    if ( !(s->hw.cmos_data[RTC_REG_B] &
+           s->hw.cmos_data[RTC_REG_C] &
+           (RTC_PF | RTC_AF | RTC_UF)) )
+        return;
+
+    s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
+    hvm_isa_irq_assert(vrtc_domain(s), RTC_IRQ);
 }
 
 void rtc_periodic_interrupt(void *opaque)
@@ -631,6 +627,8 @@ static uint32_t rtc_ioport_read(RTCState
     case RTC_REG_C:
         ret = s->hw.cmos_data[s->hw.cmos_index];
         s->hw.cmos_data[RTC_REG_C] = 0x00;
+        if ( ret & RTC_IRQF )
+            hvm_isa_irq_deassert(d, RTC_IRQ);
         rtc_update_irq(s);
         check_update_timer(s);
         alarm_timer_update(s);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 4/6] x86/HVM: properly handle RTC periodic timer even when !RTC_PIE

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3905 bytes --]

Since in that case the processing it pr_intr_post() won't occur, we
need to do some additional work in pt_update_irq(). Additionally we
must not pay attention to the respective IRQ being masked.

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

--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -67,11 +67,13 @@ static void rtc_update_irq(RTCState *s)
     hvm_isa_irq_assert(vrtc_domain(s), RTC_IRQ);
 }
 
-void rtc_periodic_interrupt(void *opaque)
+bool_t rtc_periodic_interrupt(void *opaque)
 {
     RTCState *s = opaque;
+    bool_t ret;
 
     spin_lock(&s->lock);
+    ret = !(s->hw.cmos_data[RTC_REG_C] & RTC_IRQF);
     if ( !(s->hw.cmos_data[RTC_REG_C] & RTC_PF) )
     {
         s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
@@ -83,7 +85,11 @@ void rtc_periodic_interrupt(void *opaque
         destroy_periodic_time(&s->pt);
         s->pt_code = 0;
     }
+    if ( !(s->hw.cmos_data[RTC_REG_C] & RTC_IRQF) )
+        ret = 0;
     spin_unlock(&s->lock);
+
+    return ret;
 }
 
 /* Enable/configure/disable the periodic timer based on the RTC_PIE and
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -216,18 +216,23 @@ static void pt_timer_fn(void *data)
 int pt_update_irq(struct vcpu *v)
 {
     struct list_head *head = &v->arch.hvm_vcpu.tm_list;
-    struct periodic_time *pt, *temp, *earliest_pt = NULL;
-    uint64_t max_lag = -1ULL;
+    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 )
     {
         if ( pt->pending_intr_nr )
         {
-            if ( pt_irq_masked(pt) )
+            /* RTC code takes care of disabling the timer itself. */
+            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) )
             {
                 /* suspend timer emulation */
                 list_del(&pt->list);
@@ -260,7 +265,41 @@ int pt_update_irq(struct vcpu *v)
     if ( is_lapic )
         vlapic_set_irq(vcpu_vlapic(v), irq, 0);
     else if ( irq == RTC_IRQ && pt_priv )
-        rtc_periodic_interrupt(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);
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -183,7 +183,7 @@ void rtc_migrate_timers(struct vcpu *v);
 void rtc_deinit(struct domain *d);
 void rtc_reset(struct domain *d);
 void rtc_update_clock(struct domain *d);
-void rtc_periodic_interrupt(void *);
+bool_t rtc_periodic_interrupt(void *);
 
 void pmtimer_init(struct vcpu *v);
 void pmtimer_deinit(struct domain *d);




[-- Attachment #2: x86-HVM-RTC-no-interrupt.patch --]
[-- Type: text/plain, Size: 3965 bytes --]

x86/HVM: properly handle RTC periodic timer even when !RTC_PIE

Since in that case the processing it pr_intr_post() won't occur, we
need to do some additional work in pt_update_irq(). Additionally we
must not pay attention to the respective IRQ being masked.

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

--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -67,11 +67,13 @@ static void rtc_update_irq(RTCState *s)
     hvm_isa_irq_assert(vrtc_domain(s), RTC_IRQ);
 }
 
-void rtc_periodic_interrupt(void *opaque)
+bool_t rtc_periodic_interrupt(void *opaque)
 {
     RTCState *s = opaque;
+    bool_t ret;
 
     spin_lock(&s->lock);
+    ret = !(s->hw.cmos_data[RTC_REG_C] & RTC_IRQF);
     if ( !(s->hw.cmos_data[RTC_REG_C] & RTC_PF) )
     {
         s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
@@ -83,7 +85,11 @@ void rtc_periodic_interrupt(void *opaque
         destroy_periodic_time(&s->pt);
         s->pt_code = 0;
     }
+    if ( !(s->hw.cmos_data[RTC_REG_C] & RTC_IRQF) )
+        ret = 0;
     spin_unlock(&s->lock);
+
+    return ret;
 }
 
 /* Enable/configure/disable the periodic timer based on the RTC_PIE and
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -216,18 +216,23 @@ static void pt_timer_fn(void *data)
 int pt_update_irq(struct vcpu *v)
 {
     struct list_head *head = &v->arch.hvm_vcpu.tm_list;
-    struct periodic_time *pt, *temp, *earliest_pt = NULL;
-    uint64_t max_lag = -1ULL;
+    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 )
     {
         if ( pt->pending_intr_nr )
         {
-            if ( pt_irq_masked(pt) )
+            /* RTC code takes care of disabling the timer itself. */
+            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) )
             {
                 /* suspend timer emulation */
                 list_del(&pt->list);
@@ -260,7 +265,41 @@ int pt_update_irq(struct vcpu *v)
     if ( is_lapic )
         vlapic_set_irq(vcpu_vlapic(v), irq, 0);
     else if ( irq == RTC_IRQ && pt_priv )
-        rtc_periodic_interrupt(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);
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -183,7 +183,7 @@ void rtc_migrate_timers(struct vcpu *v);
 void rtc_deinit(struct domain *d);
 void rtc_reset(struct domain *d);
 void rtc_update_clock(struct domain *d);
-void rtc_periodic_interrupt(void *);
+bool_t rtc_periodic_interrupt(void *);
 
 void pmtimer_init(struct vcpu *v);
 void pmtimer_deinit(struct domain *d);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 5/6] x86/HVM: fix legacy PIC check in pt_update_irq()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

Depending on the IRQ we need to
- not look at the PIC at all is this is the LAPIC timer (in that case
  we're dealing with a vector number rather than an IRQ one),
- not look at the PIC for any non-legacy interrupt,
- look at the correct PIC for the IRQ (which will always be PIC 2 for
  the RTC, and possibly also for HPET).

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

--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -311,8 +311,9 @@ int pt_update_irq(struct vcpu *v)
      * IRR is returned and used to set eoi_exit_bitmap for virtual
      * interrupt delivery case. Otherwise return -1 to do nothing.  
      */ 
-    if ( vlapic_accept_pic_intr(v) &&
-         (&v->domain->arch.hvm_domain)->vpic[0].int_output )
+    if ( !is_lapic &&
+         platform_legacy_irq(irq) && vlapic_accept_pic_intr(v) &&
+         (&v->domain->arch.hvm_domain)->vpic[irq >> 3].int_output )
         return -1;
     else 
         return pt_irq_vector(earliest_pt, hvm_intsrc_lapic);




[-- Attachment #2: x86-HVM-vpt-irq-vector.patch --]
[-- Type: text/plain, Size: 1063 bytes --]

x86/HVM: fix legacy PIC check in pt_update_irq()

Depending on the IRQ we need to
- not look at the PIC at all is this is the LAPIC timer (in that case
  we're dealing with a vector number rather than an IRQ one),
- not look at the PIC for any non-legacy interrupt,
- look at the correct PIC for the IRQ (which will always be PIC 2 for
  the RTC, and possibly also for HPET).

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

--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -311,8 +311,9 @@ int pt_update_irq(struct vcpu *v)
      * IRR is returned and used to set eoi_exit_bitmap for virtual
      * interrupt delivery case. Otherwise return -1 to do nothing.  
      */ 
-    if ( vlapic_accept_pic_intr(v) &&
-         (&v->domain->arch.hvm_domain)->vpic[0].int_output )
+    if ( !is_lapic &&
+         platform_legacy_irq(irq) && vlapic_accept_pic_intr(v) &&
+         (&v->domain->arch.hvm_domain)->vpic[irq >> 3].int_output )
         return -1;
     else 
         return pt_irq_vector(earliest_pt, hvm_intsrc_lapic);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 6/6] x86/HVM: RTC code must be in line with WAET flags passed by hvmloader

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3330 bytes --]

With hvmloader telling the guest that it may skip REG_C reads during
the processing of RTC interrupts, the emulation code must not depend
upon these reads to occur. Introduce two modes of operation for the
emulation code, and short of a HVM parameter (too late to be
introduced for 4.3) hard code the mode determination to always assume
that Windows-conforming one for the time being.

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

--- a/tools/firmware/hvmloader/acpi/static_tables.c
+++ b/tools/firmware/hvmloader/acpi/static_tables.c
@@ -136,11 +136,15 @@ struct acpi_20_rsdp Rsdp = {
     .length    = sizeof(struct acpi_20_rsdp)
 };
 
-#define ACPI_WAET_RTC_GOOD      0x00000001
-#define ACPI_WAET_PM_TIMER_GOOD 0x00000002
+#define ACPI_WAET_RTC_NO_ACK        (1<<0) /* RTC requires no int acknowledge */
+#define ACPI_WAET_TIMER_ONE_READ    (1<<1) /* PM timer requires only one read */
 
-#define ACPI_WAET_FLAGS (ACPI_WAET_RTC_GOOD | \
-                         ACPI_WAET_PM_TIMER_GOOD)
+/*
+ * The state of the RTC flag getting passed to the guest must be in
+ * sync with the mode selection in the hypervisor RTC emulation code.
+ */
+#define ACPI_WAET_FLAGS (ACPI_WAET_RTC_NO_ACK | \
+                         ACPI_WAET_TIMER_ONE_READ)
 
 struct acpi_20_waet Waet = {
     .header = {
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -45,6 +45,15 @@
 #define epoch_year     1900
 #define get_year(x)    (x + epoch_year)
 
+enum rtc_mode {
+   rtc_mode_no_ack,
+   rtc_mode_strict
+};
+
+/* This must be in sync with how hvmloader sets the ACPI WAET flags. */
+#define mode_is(d, m) ((void)(d), rtc_mode_##m == rtc_mode_no_ack)
+#define rtc_mode_is(s, m) mode_is(vrtc_domain(s), m)
+
 static void rtc_copy_date(RTCState *s);
 static void rtc_set_time(RTCState *s);
 static inline int from_bcd(RTCState *s, int a);
@@ -54,7 +63,7 @@ static void rtc_update_irq(RTCState *s)
 {
     ASSERT(spin_is_locked(&s->lock));
 
-    if ( s->hw.cmos_data[RTC_REG_C] & RTC_IRQF )
+    if ( rtc_mode_is(s, strict) && (s->hw.cmos_data[RTC_REG_C] & RTC_IRQF) )
         return;
 
     /* IRQ is raised if any source is both raised & enabled */
@@ -64,6 +73,8 @@ static void rtc_update_irq(RTCState *s)
         return;
 
     s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
+    if ( rtc_mode_is(s, no_ack) )
+        hvm_isa_irq_deassert(vrtc_domain(s), RTC_IRQ);
     hvm_isa_irq_assert(vrtc_domain(s), RTC_IRQ);
 }
 
@@ -73,8 +84,8 @@ bool_t rtc_periodic_interrupt(void *opaq
     bool_t ret;
 
     spin_lock(&s->lock);
-    ret = !(s->hw.cmos_data[RTC_REG_C] & RTC_IRQF);
-    if ( !(s->hw.cmos_data[RTC_REG_C] & RTC_PF) )
+    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);
@@ -633,7 +644,7 @@ static uint32_t rtc_ioport_read(RTCState
     case RTC_REG_C:
         ret = s->hw.cmos_data[s->hw.cmos_index];
         s->hw.cmos_data[RTC_REG_C] = 0x00;
-        if ( ret & RTC_IRQF )
+        if ( (ret & RTC_IRQF) && !rtc_mode_is(s, no_ack) )
             hvm_isa_irq_deassert(d, RTC_IRQ);
         rtc_update_irq(s);
         check_update_timer(s);




[-- Attachment #2: x86-HVM-RTC-no-ack.patch --]
[-- Type: text/plain, Size: 3397 bytes --]

x86/HVM: RTC code must be in line with WAET flags passed by hvmloader

With hvmloader telling the guest that it may skip REG_C reads during
the processing of RTC interrupts, the emulation code must not depend
upon these reads to occur. Introduce two modes of operation for the
emulation code, and short of a HVM parameter (too late to be
introduced for 4.3) hard code the mode determination to always assume
that Windows-conforming one for the time being.

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

--- a/tools/firmware/hvmloader/acpi/static_tables.c
+++ b/tools/firmware/hvmloader/acpi/static_tables.c
@@ -136,11 +136,15 @@ struct acpi_20_rsdp Rsdp = {
     .length    = sizeof(struct acpi_20_rsdp)
 };
 
-#define ACPI_WAET_RTC_GOOD      0x00000001
-#define ACPI_WAET_PM_TIMER_GOOD 0x00000002
+#define ACPI_WAET_RTC_NO_ACK        (1<<0) /* RTC requires no int acknowledge */
+#define ACPI_WAET_TIMER_ONE_READ    (1<<1) /* PM timer requires only one read */
 
-#define ACPI_WAET_FLAGS (ACPI_WAET_RTC_GOOD | \
-                         ACPI_WAET_PM_TIMER_GOOD)
+/*
+ * The state of the RTC flag getting passed to the guest must be in
+ * sync with the mode selection in the hypervisor RTC emulation code.
+ */
+#define ACPI_WAET_FLAGS (ACPI_WAET_RTC_NO_ACK | \
+                         ACPI_WAET_TIMER_ONE_READ)
 
 struct acpi_20_waet Waet = {
     .header = {
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -45,6 +45,15 @@
 #define epoch_year     1900
 #define get_year(x)    (x + epoch_year)
 
+enum rtc_mode {
+   rtc_mode_no_ack,
+   rtc_mode_strict
+};
+
+/* This must be in sync with how hvmloader sets the ACPI WAET flags. */
+#define mode_is(d, m) ((void)(d), rtc_mode_##m == rtc_mode_no_ack)
+#define rtc_mode_is(s, m) mode_is(vrtc_domain(s), m)
+
 static void rtc_copy_date(RTCState *s);
 static void rtc_set_time(RTCState *s);
 static inline int from_bcd(RTCState *s, int a);
@@ -54,7 +63,7 @@ static void rtc_update_irq(RTCState *s)
 {
     ASSERT(spin_is_locked(&s->lock));
 
-    if ( s->hw.cmos_data[RTC_REG_C] & RTC_IRQF )
+    if ( rtc_mode_is(s, strict) && (s->hw.cmos_data[RTC_REG_C] & RTC_IRQF) )
         return;
 
     /* IRQ is raised if any source is both raised & enabled */
@@ -64,6 +73,8 @@ static void rtc_update_irq(RTCState *s)
         return;
 
     s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
+    if ( rtc_mode_is(s, no_ack) )
+        hvm_isa_irq_deassert(vrtc_domain(s), RTC_IRQ);
     hvm_isa_irq_assert(vrtc_domain(s), RTC_IRQ);
 }
 
@@ -73,8 +84,8 @@ bool_t rtc_periodic_interrupt(void *opaq
     bool_t ret;
 
     spin_lock(&s->lock);
-    ret = !(s->hw.cmos_data[RTC_REG_C] & RTC_IRQF);
-    if ( !(s->hw.cmos_data[RTC_REG_C] & RTC_PF) )
+    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);
@@ -633,7 +644,7 @@ static uint32_t rtc_ioport_read(RTCState
     case RTC_REG_C:
         ret = s->hw.cmos_data[s->hw.cmos_index];
         s->hw.cmos_data[RTC_REG_C] = 0x00;
-        if ( ret & RTC_IRQF )
+        if ( (ret & RTC_IRQF) && !rtc_mode_is(s, no_ack) )
             hvm_isa_irq_deassert(d, RTC_IRQ);
         rtc_update_irq(s);
         check_update_timer(s);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 0/6] x86/HVM: RTC/VPT adjustments

[George Dunlap]

On 29/04/13 14:42, Jan Beulich wrote:
> 1: fix processing of RTC REG_B writes
> 2: slightly adjust RTC reset
> 3: adjust IRQ (de-)assertion
> 4: properly handle RTC periodic timer even when !RTC_PIE
> 5: fix legacy PIC check in pt_update_irq()
> 6: RTC code must be in line with WAET flags passed by hvmloader
>
> This fixes the Win2003 boot failure George reported. Roger, since
> the first patch is slightly different from what you tested earlier,
> could you re-test that patch alone and the full series against
> FreeBSD?
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

This series seems to fix the w2k3 issue -- but it looks like a series of 
"fixes and updates".  I thought we had decided to revert all the RTC 
changes?

  -George

Re: [PATCH 0/6] x86/HVM: RTC/VPT adjustments

[Jan Beulich]

>>> On 01.05.13 at 18:15, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 29/04/13 14:42, Jan Beulich wrote:
>> 1: fix processing of RTC REG_B writes
>> 2: slightly adjust RTC reset
>> 3: adjust IRQ (de-)assertion
>> 4: properly handle RTC periodic timer even when !RTC_PIE
>> 5: fix legacy PIC check in pt_update_irq()
>> 6: RTC code must be in line with WAET flags passed by hvmloader
>>
>> This fixes the Win2003 boot failure George reported. Roger, since
>> the first patch is slightly different from what you tested earlier,
>> could you re-test that patch alone and the full series against
>> FreeBSD?
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> This series seems to fix the w2k3 issue -- but it looks like a series of 
> "fixes and updates".  I thought we had decided to revert all the RTC 
> changes?

I always said I'd prefer a partial revert over a full one, if at all
possible. Of course I'm not intending to enforce this in any way,
but I'm also not intending to myself revert good fixes (and drop
further ones, as presented in this series) without need. So my
proposed solution is this series of patches (which is a partial
revert in terms of functionality, but not any kind of revert in terms
of source code) - others can certainly propose other solutions.
This is even more so now that we know that the reason for the
observed regression weren't the RTC changes themselves, but
the expectation of non-spec-conforming emulation behavior by
the guest.

Jan

Re: [PATCH 1/6] x86/HVM: fix processing of RTC REG_B writes

[Roger Pau Monné]

On 29/04/13 15:51, Jan Beulich wrote:
> We must store the new values before calling rtc_update_irq(), and we
> need to call rtc_timer_update() when PIE transitions from 0 to 1 (as we
> may have previously turned off the periodic timer due to the guest not
> reading REG_C, and hence may have to re-enable it in order to start
> IRQs getting delivered to the guest).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Tested-by: Roger Pau Monné <roger.pau@citrix.com>

On FreeBSD

> 
> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -468,12 +468,14 @@ static int rtc_ioport_write(void *opaque
>              if ( orig & RTC_SET )
>                  rtc_set_time(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);
> -        s->hw.cmos_data[RTC_REG_B] = data;
> +        if ( (data & RTC_PIE) && !(orig & RTC_PIE) )
> +            rtc_timer_update(s);
>          if ( (data ^ orig) & RTC_SET )
>              check_update_timer(s);
>          if ( (data ^ orig) & (RTC_24H | RTC_DM_BINARY | RTC_SET) )
> 
> 
> 

Re: [PATCH 0/6] x86/HVM: RTC/VPT adjustments

[Roger Pau Monné]

On 29/04/13 15:42, Jan Beulich wrote:
> 1: fix processing of RTC REG_B writes
> 2: slightly adjust RTC reset
> 3: adjust IRQ (de-)assertion
> 4: properly handle RTC periodic timer even when !RTC_PIE
> 5: fix legacy PIC check in pt_update_irq()
> 6: RTC code must be in line with WAET flags passed by hvmloader
> 
> This fixes the Win2003 boot failure George reported. Roger, since
> the first patch is slightly different from what you tested earlier,
> could you re-test that patch alone and the full series against
> FreeBSD?

For the full series:

Tested-by: Roger Pau Monné <roger.pau@citrix.com>

On FreeBSD. I've already tested patch 1 alone and it is also OK (see my
specific Tested-by for that patch).

Re: [PATCH 0/6] x86/HVM: RTC/VPT adjustments

[Jan Beulich]

>>> On 02.05.13 at 10:15, Roger Pau Monné<roger.pau@citrix.com> wrote:
> On 29/04/13 15:42, Jan Beulich wrote:
>> 1: fix processing of RTC REG_B writes
>> 2: slightly adjust RTC reset
>> 3: adjust IRQ (de-)assertion
>> 4: properly handle RTC periodic timer even when !RTC_PIE
>> 5: fix legacy PIC check in pt_update_irq()
>> 6: RTC code must be in line with WAET flags passed by hvmloader
>> 
>> This fixes the Win2003 boot failure George reported. Roger, since
>> the first patch is slightly different from what you tested earlier,
>> could you re-test that patch alone and the full series against
>> FreeBSD?
> 
> For the full series:
> 
> Tested-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> On FreeBSD. I've already tested patch 1 alone and it is also OK (see my
> specific Tested-by for that patch).

Thanks a lot!

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/6] x86/HVM: fix processing of RTC REG_B writes

[Tim Deegan]

At 14:51 +0100 on 29 Apr (1367247101), Jan Beulich wrote:
> We must store the new values before calling rtc_update_irq(), and we
> need to call rtc_timer_update() when PIE transitions from 0 to 1 (as we
> may have previously turned off the periodic timer due to the guest not
> reading REG_C, and hence may have to re-enable it in order to start
> IRQs getting delivered to the guest).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -468,12 +468,14 @@ static int rtc_ioport_write(void *opaque
>              if ( orig & RTC_SET )
>                  rtc_set_time(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);
> -        s->hw.cmos_data[RTC_REG_B] = data;
> +        if ( (data & RTC_PIE) && !(orig & RTC_PIE) )
> +            rtc_timer_update(s);

Shouldn't this be 'if ( (data ^ orig) & RTC_PIE )'? You'll want to
disable the timer if the interrupt's been turned off.

Tim.

Re: [PATCH 0/6] x86/HVM: RTC/VPT adjustments

[George Dunlap]

On 02/05/13 07:58, Jan Beulich wrote:
>>>> On 01.05.13 at 18:15, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>> On 29/04/13 14:42, Jan Beulich wrote:
>>> 1: fix processing of RTC REG_B writes
>>> 2: slightly adjust RTC reset
>>> 3: adjust IRQ (de-)assertion
>>> 4: properly handle RTC periodic timer even when !RTC_PIE
>>> 5: fix legacy PIC check in pt_update_irq()
>>> 6: RTC code must be in line with WAET flags passed by hvmloader
>>>
>>> This fixes the Win2003 boot failure George reported. Roger, since
>>> the first patch is slightly different from what you tested earlier,
>>> could you re-test that patch alone and the full series against
>>> FreeBSD?
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> This series seems to fix the w2k3 issue -- but it looks like a series of
>> "fixes and updates".  I thought we had decided to revert all the RTC
>> changes?
> I always said I'd prefer a partial revert over a full one, if at all
> possible. Of course I'm not intending to enforce this in any way,
> but I'm also not intending to myself revert good fixes (and drop
> further ones, as presented in this series) without need. So my
> proposed solution is this series of patches (which is a partial
> revert in terms of functionality, but not any kind of revert in terms
> of source code) - others can certainly propose other solutions.
> This is even more so now that we know that the reason for the
> observed regression weren't the RTC changes themselves, but
> the expectation of non-spec-conforming emulation behavior by
> the guest.

OK -- well I'll leave it to you and Tim to judge; just let me remind you 
of our primary goals at this point (in order of importance):

1. A bug-free 4.3 release
2. An awesome 4.3 release
3. An on-time 4.3 release

And that for #1, in particular we're worried about bugs that that may 
not be detected until after the release.

If you think this series optimizes those goals from a risk / benefits 
perspective, then I'm OK with it.

  -George

Re: [PATCH 2/6] x86/HVM: slightly adjust RTC reset

[Tim Deegan]

At 14:52 +0100 on 29 Apr (1367247135), Jan Beulich wrote:
> We should clear the interrupt enable flags here, deassert the IRQ, and
> clear REG_C.

I'm not sure at all that we should be tinkering with the IE flags here.
AFAICT this code is called on S3 suspend -- Does a real S3 do that (and
not reset any of the other RTC registers?

Deasserting the IRQ seems fair enough, though probably as part of the
ther IRQ-frobbing changes in another patch.

Tim.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -732,6 +732,11 @@ void rtc_reset(struct domain *d)
>      destroy_periodic_time(&s->pt);
>      s->pt_code = 0;
>      s->pt.source = PTSRC_isa;
> +
> +    s->hw.cmos_data[RTC_REG_B] &= ~(RTC_PIE | RTC_AIE | RTC_UIE);
> +    if ( s->hw.cmos_data[RTC_REG_C] & RTC_IRQF )
> +        hvm_isa_irq_deassert(d, RTC_IRQ);
> +    s->hw.cmos_data[RTC_REG_C] = 0;
>  }
>  
>  void rtc_init(struct domain *d)
> 
> 
> 

> x86/HVM: slightly adjust RTC reset
> 
> We should clear the interrupt enable flags here, deassert the IRQ, and
> clear REG_C.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -732,6 +732,11 @@ void rtc_reset(struct domain *d)
>      destroy_periodic_time(&s->pt);
>      s->pt_code = 0;
>      s->pt.source = PTSRC_isa;
> +
> +    s->hw.cmos_data[RTC_REG_B] &= ~(RTC_PIE | RTC_AIE | RTC_UIE);
> +    if ( s->hw.cmos_data[RTC_REG_C] & RTC_IRQF )
> +        hvm_isa_irq_deassert(d, RTC_IRQ);
> +    s->hw.cmos_data[RTC_REG_C] = 0;
>  }
>  
>  void rtc_init(struct domain *d)

Re: [PATCH 3/6] x86/HVM: adjust IRQ (de-)assertion

[Tim Deegan]

At 14:53 +0100 on 29 Apr (1367247201), Jan Beulich wrote:
> De-assertion should only happen when RTC_IRQF gets cleared, i.e. upon
> REG_C reads. Assertion should be done only when the flag transitions
> from 0 to 1.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Well, this seems correct to me as far as the original (level-triggered)
RTC chip goes, but when we discussed changing this before, you suggested
that we should keep the old (somewhat bizarre) behaviour.

Unless this is fixing an observed bug, and unless you've tested it
widely, I don't think this is for 4.3.

Tim.

> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -52,23 +52,19 @@ static inline int convert_hour(RTCState 
>  
>  static void rtc_update_irq(RTCState *s)
>  {
> -    struct domain *d = vrtc_domain(s);
> -    uint8_t irqf;
> -
>      ASSERT(spin_is_locked(&s->lock));
>  
> +    if ( s->hw.cmos_data[RTC_REG_C] & RTC_IRQF )
> +        return;
> +
>      /* IRQ is raised if any source is both 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;
> -
> -    hvm_isa_irq_deassert(d, RTC_IRQ);
> -    if ( irqf )
> -        hvm_isa_irq_assert(d, RTC_IRQ);
> +    if ( !(s->hw.cmos_data[RTC_REG_B] &
> +           s->hw.cmos_data[RTC_REG_C] &
> +           (RTC_PF | RTC_AF | RTC_UF)) )
> +        return;
> +
> +    s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
> +    hvm_isa_irq_assert(vrtc_domain(s), RTC_IRQ);
>  }
>  
>  void rtc_periodic_interrupt(void *opaque)
> @@ -631,6 +627,8 @@ static uint32_t rtc_ioport_read(RTCState
>      case RTC_REG_C:
>          ret = s->hw.cmos_data[s->hw.cmos_index];
>          s->hw.cmos_data[RTC_REG_C] = 0x00;
> +        if ( ret & RTC_IRQF )
> +            hvm_isa_irq_deassert(d, RTC_IRQ);
>          rtc_update_irq(s);
>          check_update_timer(s);
>          alarm_timer_update(s);
> 
> 
> 

> x86/HVM: adjust IRQ (de-)assertion
> 
> De-assertion should only happen when RTC_IRQF gets cleared, i.e. upon
> REG_C reads. Assertion should be done only when the flag transitions
> from 0 to 1.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -52,23 +52,19 @@ static inline int convert_hour(RTCState 
>  
>  static void rtc_update_irq(RTCState *s)
>  {
> -    struct domain *d = vrtc_domain(s);
> -    uint8_t irqf;
> -
>      ASSERT(spin_is_locked(&s->lock));
>  
> +    if ( s->hw.cmos_data[RTC_REG_C] & RTC_IRQF )
> +        return;
> +
>      /* IRQ is raised if any source is both 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;
> -
> -    hvm_isa_irq_deassert(d, RTC_IRQ);
> -    if ( irqf )
> -        hvm_isa_irq_assert(d, RTC_IRQ);
> +    if ( !(s->hw.cmos_data[RTC_REG_B] &
> +           s->hw.cmos_data[RTC_REG_C] &
> +           (RTC_PF | RTC_AF | RTC_UF)) )
> +        return;
> +
> +    s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
> +    hvm_isa_irq_assert(vrtc_domain(s), RTC_IRQ);
>  }
>  
>  void rtc_periodic_interrupt(void *opaque)
> @@ -631,6 +627,8 @@ static uint32_t rtc_ioport_read(RTCState
>      case RTC_REG_C:
>          ret = s->hw.cmos_data[s->hw.cmos_index];
>          s->hw.cmos_data[RTC_REG_C] = 0x00;
> +        if ( ret & RTC_IRQF )
> +            hvm_isa_irq_deassert(d, RTC_IRQ);
>          rtc_update_irq(s);
>          check_update_timer(s);
>          alarm_timer_update(s);

Re: [PATCH 1/6] x86/HVM: fix processing of RTC REG_B writes

[Jan Beulich]

>>> On 02.05.13 at 11:21, Tim Deegan <tim@xen.org> wrote:
> At 14:51 +0100 on 29 Apr (1367247101), Jan Beulich wrote:
>> We must store the new values before calling rtc_update_irq(), and we
>> need to call rtc_timer_update() when PIE transitions from 0 to 1 (as we
>> may have previously turned off the periodic timer due to the guest not
>> reading REG_C, and hence may have to re-enable it in order to start
>> IRQs getting delivered to the guest).
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> --- a/xen/arch/x86/hvm/rtc.c
>> +++ b/xen/arch/x86/hvm/rtc.c
>> @@ -468,12 +468,14 @@ static int rtc_ioport_write(void *opaque
>>              if ( orig & RTC_SET )
>>                  rtc_set_time(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);
>> -        s->hw.cmos_data[RTC_REG_B] = data;
>> +        if ( (data & RTC_PIE) && !(orig & RTC_PIE) )
>> +            rtc_timer_update(s);
> 
> Shouldn't this be 'if ( (data ^ orig) & RTC_PIE )'? You'll want to
> disable the timer if the interrupt's been turned off.

No, in the spirit of the other involved code we'll want to keep it
running until reaching dead_ticks.

Jan

Re: [PATCH 4/6] x86/HVM: properly handle RTC periodic timer even when !RTC_PIE

[Tim Deegan]

At 14:53 +0100 on 29 Apr (1367247238), Jan Beulich wrote:
> Since in that case the processing it pr_intr_post() won't occur, we
> need to do some additional work in pt_update_irq(). Additionally we
> must not pay attention to the respective IRQ being masked.

I don't think this is the right fix for 4.3.  We should just revert to
the old system (where the vpt code raises the IRQ) rather than bodge up
the new one -- especially since the new _behaviour_ is disabled anyway.

After 4.3 branches (which is RSN, right Goerge?) we can sort out a
proper interface for all of that, and this might well be it.

Tim.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -67,11 +67,13 @@ static void rtc_update_irq(RTCState *s)
>      hvm_isa_irq_assert(vrtc_domain(s), RTC_IRQ);
>  }
>  
> -void rtc_periodic_interrupt(void *opaque)
> +bool_t rtc_periodic_interrupt(void *opaque)
>  {
>      RTCState *s = opaque;
> +    bool_t ret;
>  
>      spin_lock(&s->lock);
> +    ret = !(s->hw.cmos_data[RTC_REG_C] & RTC_IRQF);
>      if ( !(s->hw.cmos_data[RTC_REG_C] & RTC_PF) )
>      {
>          s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
> @@ -83,7 +85,11 @@ void rtc_periodic_interrupt(void *opaque
>          destroy_periodic_time(&s->pt);
>          s->pt_code = 0;
>      }
> +    if ( !(s->hw.cmos_data[RTC_REG_C] & RTC_IRQF) )
> +        ret = 0;
>      spin_unlock(&s->lock);
> +
> +    return ret;
>  }
>  
>  /* Enable/configure/disable the periodic timer based on the RTC_PIE and
> --- a/xen/arch/x86/hvm/vpt.c
> +++ b/xen/arch/x86/hvm/vpt.c
> @@ -216,18 +216,23 @@ static void pt_timer_fn(void *data)
>  int pt_update_irq(struct vcpu *v)
>  {
>      struct list_head *head = &v->arch.hvm_vcpu.tm_list;
> -    struct periodic_time *pt, *temp, *earliest_pt = NULL;
> -    uint64_t max_lag = -1ULL;
> +    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 )
>      {
>          if ( pt->pending_intr_nr )
>          {
> -            if ( pt_irq_masked(pt) )
> +            /* RTC code takes care of disabling the timer itself. */
> +            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) )
>              {
>                  /* suspend timer emulation */
>                  list_del(&pt->list);
> @@ -260,7 +265,41 @@ int pt_update_irq(struct vcpu *v)
>      if ( is_lapic )
>          vlapic_set_irq(vcpu_vlapic(v), irq, 0);
>      else if ( irq == RTC_IRQ && pt_priv )
> -        rtc_periodic_interrupt(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);
> --- a/xen/include/asm-x86/hvm/vpt.h
> +++ b/xen/include/asm-x86/hvm/vpt.h
> @@ -183,7 +183,7 @@ void rtc_migrate_timers(struct vcpu *v);
>  void rtc_deinit(struct domain *d);
>  void rtc_reset(struct domain *d);
>  void rtc_update_clock(struct domain *d);
> -void rtc_periodic_interrupt(void *);
> +bool_t rtc_periodic_interrupt(void *);
>  
>  void pmtimer_init(struct vcpu *v);
>  void pmtimer_deinit(struct domain *d);
> 
> 
> 

> x86/HVM: properly handle RTC periodic timer even when !RTC_PIE
> 
> Since in that case the processing it pr_intr_post() won't occur, we
> need to do some additional work in pt_update_irq(). Additionally we
> must not pay attention to the respective IRQ being masked.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/hvm/rtc.c
> +++ b/xen/arch/x86/hvm/rtc.c
> @@ -67,11 +67,13 @@ static void rtc_update_irq(RTCState *s)
>      hvm_isa_irq_assert(vrtc_domain(s), RTC_IRQ);
>  }
>  
> -void rtc_periodic_interrupt(void *opaque)
> +bool_t rtc_periodic_interrupt(void *opaque)
>  {
>      RTCState *s = opaque;
> +    bool_t ret;
>  
>      spin_lock(&s->lock);
> +    ret = !(s->hw.cmos_data[RTC_REG_C] & RTC_IRQF);
>      if ( !(s->hw.cmos_data[RTC_REG_C] & RTC_PF) )
>      {
>          s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
> @@ -83,7 +85,11 @@ void rtc_periodic_interrupt(void *opaque
>          destroy_periodic_time(&s->pt);
>          s->pt_code = 0;
>      }
> +    if ( !(s->hw.cmos_data[RTC_REG_C] & RTC_IRQF) )
> +        ret = 0;
>      spin_unlock(&s->lock);
> +
> +    return ret;
>  }
>  
>  /* Enable/configure/disable the periodic timer based on the RTC_PIE and
> --- a/xen/arch/x86/hvm/vpt.c
> +++ b/xen/arch/x86/hvm/vpt.c
> @@ -216,18 +216,23 @@ static void pt_timer_fn(void *data)
>  int pt_update_irq(struct vcpu *v)
>  {
>      struct list_head *head = &v->arch.hvm_vcpu.tm_list;
> -    struct periodic_time *pt, *temp, *earliest_pt = NULL;
> -    uint64_t max_lag = -1ULL;
> +    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 )
>      {
>          if ( pt->pending_intr_nr )
>          {
> -            if ( pt_irq_masked(pt) )
> +            /* RTC code takes care of disabling the timer itself. */
> +            if ( (pt->irq != RTC_IRQ || !pt->priv) && pt_irq_masked(pt) )
>              {
>                  /* suspend timer emulation */
>                  list_del(&pt->list);
> @@ -260,7 +265,41 @@ int pt_update_irq(struct vcpu *v)
>      if ( is_lapic )
>          vlapic_set_irq(vcpu_vlapic(v), irq, 0);
>      else if ( irq == RTC_IRQ && pt_priv )
> -        rtc_periodic_interrupt(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);
> --- a/xen/include/asm-x86/hvm/vpt.h
> +++ b/xen/include/asm-x86/hvm/vpt.h
> @@ -183,7 +183,7 @@ void rtc_migrate_timers(struct vcpu *v);
>  void rtc_deinit(struct domain *d);
>  void rtc_reset(struct domain *d);
>  void rtc_update_clock(struct domain *d);
> -void rtc_periodic_interrupt(void *);
> +bool_t rtc_periodic_interrupt(void *);
>  
>  void pmtimer_init(struct vcpu *v);
>  void pmtimer_deinit(struct domain *d);

Re: [PATCH 5/6] x86/HVM: fix legacy PIC check in pt_update_irq()

[Tim Deegan]

At 14:54 +0100 on 29 Apr (1367247296), Jan Beulich wrote:
> Depending on the IRQ we need to
> - not look at the PIC at all is this is the LAPIC timer (in that case
>   we're dealing with a vector number rather than an IRQ one),
> - not look at the PIC for any non-legacy interrupt,
> - look at the correct PIC for the IRQ (which will always be PIC 2 for
>   the RTC, and possibly also for HPET).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Tim Deegan <tim@xen.org>

> --- a/xen/arch/x86/hvm/vpt.c
> +++ b/xen/arch/x86/hvm/vpt.c
> @@ -311,8 +311,9 @@ int pt_update_irq(struct vcpu *v)
>       * IRR is returned and used to set eoi_exit_bitmap for virtual
>       * interrupt delivery case. Otherwise return -1 to do nothing.  
>       */ 
> -    if ( vlapic_accept_pic_intr(v) &&
> -         (&v->domain->arch.hvm_domain)->vpic[0].int_output )
> +    if ( !is_lapic &&
> +         platform_legacy_irq(irq) && vlapic_accept_pic_intr(v) &&
> +         (&v->domain->arch.hvm_domain)->vpic[irq >> 3].int_output )
>          return -1;
>      else 
>          return pt_irq_vector(earliest_pt, hvm_intsrc_lapic);
> 
> 
> 

Re: [PATCH 1/6] x86/HVM: fix processing of RTC REG_B writes

[Tim Deegan]

At 10:40 +0100 on 02 May (1367491221), Jan Beulich wrote:
> >>> On 02.05.13 at 11:21, Tim Deegan <tim@xen.org> wrote:
> > At 14:51 +0100 on 29 Apr (1367247101), Jan Beulich wrote:
> >> We must store the new values before calling rtc_update_irq(), and we
> >> need to call rtc_timer_update() when PIE transitions from 0 to 1 (as we
> >> may have previously turned off the periodic timer due to the guest not
> >> reading REG_C, and hence may have to re-enable it in order to start
> >> IRQs getting delivered to the guest).
> >> 
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> 
> >> --- a/xen/arch/x86/hvm/rtc.c
> >> +++ b/xen/arch/x86/hvm/rtc.c
> >> @@ -468,12 +468,14 @@ static int rtc_ioport_write(void *opaque
> >>              if ( orig & RTC_SET )
> >>                  rtc_set_time(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);
> >> -        s->hw.cmos_data[RTC_REG_B] = data;
> >> +        if ( (data & RTC_PIE) && !(orig & RTC_PIE) )
> >> +            rtc_timer_update(s);
> > 
> > Shouldn't this be 'if ( (data ^ orig) & RTC_PIE )'? You'll want to
> > disable the timer if the interrupt's been turned off.
> 
> No, in the spirit of the other involved code we'll want to keep it
> running until reaching dead_ticks.

To get the benefit of VPT processing if the interrupt's only briefly
disabled?  Fair enough.  If you add a comment to that effect, then

Reviewed-by: Tim Deegan <tim@xen.org>

Re: [PATCH 2/6] x86/HVM: slightly adjust RTC reset

[Jan Beulich]

>>> On 02.05.13 at 11:27, Tim Deegan <tim@xen.org> wrote:
> At 14:52 +0100 on 29 Apr (1367247135), Jan Beulich wrote:
>> We should clear the interrupt enable flags here, deassert the IRQ, and
>> clear REG_C.
> 
> I'm not sure at all that we should be tinkering with the IE flags here.
> AFAICT this code is called on S3 suspend -- Does a real S3 do that (and
> not reset any of the other RTC registers?

A real S3 might or might not do this, but I have to admit that I
didn't notice that this is being called from other than rtc_init().
The change was meant to serve purely documentation purposes
based on the name of the function (which isn't in line with being
used in the S3 suspend path if real suspend wouldn't do an RTC
reset).

I wonder, however, whether clearing pt_code here is
appropriate then. And resetting pt.source wouldn't seem to
belong here either if we don't mean to really "reset" the RTC -
it's just that it never gets changed to anything else, so its
mis-placement is benign.

> Deasserting the IRQ seems fair enough, though probably as part of the
> ther IRQ-frobbing changes in another patch.

I'm tending towards dropping the patch altogether in the light
of the above.

Jan

Re: [PATCH 0/6] x86/HVM: RTC/VPT adjustments

[Tim Deegan]

At 10:23 +0100 on 02 May (1367490201), George Dunlap wrote:
> On 02/05/13 07:58, Jan Beulich wrote:
> >>>>On 01.05.13 at 18:15, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> >>On 29/04/13 14:42, Jan Beulich wrote:
> >>>1: fix processing of RTC REG_B writes
> >>>2: slightly adjust RTC reset
> >>>3: adjust IRQ (de-)assertion
> >>>4: properly handle RTC periodic timer even when !RTC_PIE
> >>>5: fix legacy PIC check in pt_update_irq()
> >>>6: RTC code must be in line with WAET flags passed by hvmloader
> >>>
> >>>This fixes the Win2003 boot failure George reported. Roger, since
> >>>the first patch is slightly different from what you tested earlier,
> >>>could you re-test that patch alone and the full series against
> >>>FreeBSD?
> >>>
> >>>Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>This series seems to fix the w2k3 issue -- but it looks like a series of
> >>"fixes and updates".  I thought we had decided to revert all the RTC
> >>changes?
> >I always said I'd prefer a partial revert over a full one, if at all
> >possible. Of course I'm not intending to enforce this in any way,
> >but I'm also not intending to myself revert good fixes (and drop
> >further ones, as presented in this series) without need. So my
> >proposed solution is this series of patches (which is a partial
> >revert in terms of functionality, but not any kind of revert in terms
> >of source code) - others can certainly propose other solutions.
> >This is even more so now that we know that the reason for the
> >observed regression weren't the RTC changes themselves, but
> >the expectation of non-spec-conforming emulation behavior by
> >the guest.
> 
> OK -- well I'll leave it to you and Tim to judge; just let me remind you 
> of our primary goals at this point (in order of importance):
> 
> 1. A bug-free 4.3 release
> 2. An awesome 4.3 release
> 3. An on-time 4.3 release
> 
> And that for #1, in particular we're worried about bugs that that may 
> not be detected until after the release.

On those grounds, as I've said, I think we should back out the vpt/rtc
interaction changes.  They look likely enough to be correct, but we
though that before the last two bugs surfaced, so without some serious
testing I don't think they're ready.

Tim.

Re: [PATCH 3/6] x86/HVM: adjust IRQ (de-)assertion

[Jan Beulich]

>>> On 02.05.13 at 11:34, Tim Deegan <tim@xen.org> wrote:
> At 14:53 +0100 on 29 Apr (1367247201), Jan Beulich wrote:
>> De-assertion should only happen when RTC_IRQF gets cleared, i.e. upon
>> REG_C reads. Assertion should be done only when the flag transitions
>> from 0 to 1.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Well, this seems correct to me as far as the original (level-triggered)
> RTC chip goes, but when we discussed changing this before, you suggested
> that we should keep the old (somewhat bizarre) behaviour.
> 
> Unless this is fixing an observed bug, and unless you've tested it
> widely, I don't think this is for 4.3.

This is setting the ground for (a) being fully in line with the spec
_and_ (b) reverting behavior to the 4.2 one in the final patch.
Doing that revert with the code as it is before this series is much
less clean.

Jan

Re: [PATCH 4/6] x86/HVM: properly handle RTC periodic timer even when !RTC_PIE

[Jan Beulich]

>>> On 02.05.13 at 11:41, Tim Deegan <tim@xen.org> wrote:
> At 14:53 +0100 on 29 Apr (1367247238), Jan Beulich wrote:
>> Since in that case the processing it pr_intr_post() won't occur, we
>> need to do some additional work in pt_update_irq(). Additionally we
>> must not pay attention to the respective IRQ being masked.
> 
> I don't think this is the right fix for 4.3.  We should just revert to
> the old system (where the vpt code raises the IRQ) rather than bodge up
> the new one -- especially since the new _behaviour_ is disabled anyway.

As said in a reply to George already - I always said I'd prefer to
do as little of a revert as possible for 4.3, and in the light of all
the brokenness not really originating from the changes to the
RTC emulation, I'm thinking even more so now.

While I won't ack or support a full revert, I of course also won't
stand in the way of this being done if you, George, and perhaps
Keir all agree.

Jan

Re: [PATCH 0/6] x86/HVM: RTC/VPT adjustments

[Jan Beulich]

>>> On 02.05.13 at 11:23, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 02/05/13 07:58, Jan Beulich wrote:
>>>>> On 01.05.13 at 18:15, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>>> On 29/04/13 14:42, Jan Beulich wrote:
>>>> 1: fix processing of RTC REG_B writes
>>>> 2: slightly adjust RTC reset
>>>> 3: adjust IRQ (de-)assertion
>>>> 4: properly handle RTC periodic timer even when !RTC_PIE
>>>> 5: fix legacy PIC check in pt_update_irq()
>>>> 6: RTC code must be in line with WAET flags passed by hvmloader
>>>>
>>>> This fixes the Win2003 boot failure George reported. Roger, since
>>>> the first patch is slightly different from what you tested earlier,
>>>> could you re-test that patch alone and the full series against
>>>> FreeBSD?
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> This series seems to fix the w2k3 issue -- but it looks like a series of
>>> "fixes and updates".  I thought we had decided to revert all the RTC
>>> changes?
>> I always said I'd prefer a partial revert over a full one, if at all
>> possible. Of course I'm not intending to enforce this in any way,
>> but I'm also not intending to myself revert good fixes (and drop
>> further ones, as presented in this series) without need. So my
>> proposed solution is this series of patches (which is a partial
>> revert in terms of functionality, but not any kind of revert in terms
>> of source code) - others can certainly propose other solutions.
>> This is even more so now that we know that the reason for the
>> observed regression weren't the RTC changes themselves, but
>> the expectation of non-spec-conforming emulation behavior by
>> the guest.
> 
> OK -- well I'll leave it to you and Tim to judge; just let me remind you 
> of our primary goals at this point (in order of importance):
> 
> 1. A bug-free 4.3 release
> 2. An awesome 4.3 release
> 3. An on-time 4.3 release
> 
> And that for #1, in particular we're worried about bugs that that may 
> not be detected until after the release.
> 
> If you think this series optimizes those goals from a risk / benefits 
> perspective, then I'm OK with it.

I continue to think so, but Tim quite obviously has a different
opinion.

Jan

Re: [PATCH 2/6] x86/HVM: slightly adjust RTC reset

[Tim Deegan]

At 10:51 +0100 on 02 May (1367491874), Jan Beulich wrote:
> >>> On 02.05.13 at 11:27, Tim Deegan <tim@xen.org> wrote:
> > At 14:52 +0100 on 29 Apr (1367247135), Jan Beulich wrote:
> >> We should clear the interrupt enable flags here, deassert the IRQ, and
> >> clear REG_C.
> > 
> > I'm not sure at all that we should be tinkering with the IE flags here.
> > AFAICT this code is called on S3 suspend -- Does a real S3 do that (and
> > not reset any of the other RTC registers?
> 
> A real S3 might or might not do this, but I have to admit that I
> didn't notice that this is being called from other than rtc_init().

OK.  rtc_init() resets all four control registers just below the call to
rtc_reset(), so we can probably just drop this.

> The change was meant to serve purely documentation purposes
> based on the name of the function (which isn't in line with being
> used in the S3 suspend path if real suspend wouldn't do an RTC
> reset).

OK, it seems like this isn't really a 'reset' so much as a way to
disable the timer.  There doesn't seem to be an equivalent call after
resume to re-enable it though. I don't understand the S3 framework well
enough to exlain that. :)

> I wonder, however, whether clearing pt_code here is
> appropriate then.

I think we need to clear pt.code since we disable the timer, and
otherwise we might never re-enable it.

> And resetting pt.source wouldn't seem to
> belong here either if we don't mean to really "reset" the RTC -
> it's just that it never gets changed to anything else, so its
> mis-placement is benign.

The pt.source change was moved from rtc_init() to rtc_reset() here:

commit 9194f26eba9e7ce3c27863dabddafe46fcfdba58
Author: Keir Fraser <keir.fraser@citrix.com>
Date:   Wed Jul 2 17:25:05 2008 +0100

    x86 hvm: Fix RTC handling.
     1. Clean up initialisation/destruction.
     2. Better handle per-domain time-offset changes.
    Signed-off-by: Keir Fraser <keir.fraser@citrix.com>

Cc'ing Keir.

Tim.