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>
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) )
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
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)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
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);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Jan Beulich
2013-Apr-29 13:53 UTC
[PATCH 4/6] 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);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Jan Beulich
2013-Apr-29 13:54 UTC
[PATCH 5/6] 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);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Jan Beulich
2013-Apr-29 13:56 UTC
[PATCH 6/6] 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);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
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
>>> 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
Roger Pau Monné
2013-May-02 08:07 UTC
Re: [PATCH 1/6] x86/HVM: fix processing of RTC REG_B writes
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) ) > > >
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).
>>> 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
Tim Deegan
2013-May-02 09:21 UTC
Re: [PATCH 1/6] x86/HVM: fix processing of RTC REG_B writes
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.
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
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)
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);
Jan Beulich
2013-May-02 09:40 UTC
Re: [PATCH 1/6] x86/HVM: fix processing of RTC REG_B writes
>>> 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
Tim Deegan
2013-May-02 09:41 UTC
Re: [PATCH 4/6] x86/HVM: properly handle RTC periodic timer even when !RTC_PIE
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);
Tim Deegan
2013-May-02 09:46 UTC
Re: [PATCH 5/6] x86/HVM: fix legacy PIC check in pt_update_irq()
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); > > >
Tim Deegan
2013-May-02 09:48 UTC
Re: [PATCH 1/6] x86/HVM: fix processing of RTC REG_B writes
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>
>>> 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
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.
>>> 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
Jan Beulich
2013-May-02 10:02 UTC
Re: [PATCH 4/6] x86/HVM: properly handle RTC periodic timer even when !RTC_PIE
>>> 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
>>> 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
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.