With these four patches, the time drift that was happening on Windows XP guests goes away, at least in my testing. 3/4 is probably the most important (with the other three, I still see slow drift), but the others all seem like a good idea to me. Changes since v1: - moved the irq logic patch to the end of the queue so the others can be applied independently - in the irq logic, keep the strobing behaviour as before - store period_code instead of period and pt_active Cheers, Tim.
Tim Deegan
2013-Mar-28 16:09 UTC
[PATCH 1/4] x86/hvm: Run the RTC periodic timer on a consistent time series.
When the RTC periodic timer gets restarted, align it to the VM''s boot
time, not to whatever time it is now. Otherwise every read of REG_C
will restart the current period
Signed-off-by: Tim Deegan <tim@xen.org>
Acked-by: Keir Fraser <keir@xen.org>
---
xen/arch/x86/hvm/rtc.c | 6 ++++--
xen/include/asm-x86/hvm/vpt.h | 3 ++-
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index c1e09d8..e9aa81a 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -80,7 +80,7 @@ void rtc_periodic_interrupt(void *opaque)
* RTC_RATE_SELECT settings */
static void rtc_timer_update(RTCState *s)
{
- int period_code, period;
+ int period_code, period, delta;
struct vcpu *v = vrtc_vcpu(s);
ASSERT(spin_is_locked(&s->lock));
@@ -98,7 +98,8 @@ static void rtc_timer_update(RTCState *s)
{
period = 1 << (period_code - 1); /* period in 32 Khz cycles
*/
period = DIV_ROUND(period * 1000000000ULL, 32768); /* in ns */
- create_periodic_time(v, &s->pt, period, period, RTC_IRQ,
NULL, s);
+ delta = period - ((NOW() - s->start_time) % period);
+ create_periodic_time(v, &s->pt, delta, period, RTC_IRQ,
NULL, s);
break;
}
/* fall through */
@@ -740,6 +741,7 @@ void rtc_init(struct domain *d)
s->hw.cmos_data[RTC_REG_D] = RTC_VRT;
s->current_tm = gmtime(get_localtime(d));
+ s->start_time = NOW();
rtc_copy_date(s);
diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
index c75297b..2e9c7d2 100644
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -104,8 +104,9 @@ typedef struct RTCState {
struct hvm_hw_rtc hw;
/* RTC''s idea of the current time */
struct tm current_tm;
+ /* periodic timer */
+ s_time_t start_time;
/* second update */
- int64_t next_second_time;
struct periodic_time pt;
/* update-ended timer */
struct timer update_timer;
--
1.7.10.4
Tim Deegan
2013-Mar-28 16:09 UTC
[PATCH 2/4] x86/hvm: Avoid needlessly resetting the periodic timer.
Signed-off-by: Tim Deegan <tim@xen.org>
---
xen/arch/x86/hvm/rtc.c | 18 ++++++++++++++----
xen/include/asm-x86/hvm/vpt.h | 10 +++++-----
2 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index e9aa81a..8d9195f 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -66,7 +66,10 @@ void rtc_periodic_interrupt(void *opaque)
spin_lock(&s->lock);
if ( s->hw.cmos_data[RTC_REG_C] & RTC_PF )
+ {
destroy_periodic_time(&s->pt);
+ s->pt_code = 0;
+ }
else
{
s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
@@ -96,15 +99,21 @@ static void rtc_timer_update(RTCState *s)
case RTC_REF_CLCK_4MHZ:
if ( period_code != 0 )
{
- period = 1 << (period_code - 1); /* period in 32 Khz cycles
*/
- period = DIV_ROUND(period * 1000000000ULL, 32768); /* in ns */
- delta = period - ((NOW() - s->start_time) % period);
- create_periodic_time(v, &s->pt, delta, period, RTC_IRQ,
NULL, s);
+ if ( period_code != s->pt_code )
+ {
+ s->pt_code = period_code;
+ period = 1 << (period_code - 1); /* period in 32 Khz
cycles */
+ period = DIV_ROUND(period * 1000000000ULL, 32768); /* in ns */
+ delta = period - ((NOW() - s->start_time) % period);
+ create_periodic_time(v, &s->pt, delta, period,
+ RTC_IRQ, NULL, s);
+ }
break;
}
/* fall through */
default:
destroy_periodic_time(&s->pt);
+ s->pt_code = 0;
break;
}
}
@@ -716,6 +725,7 @@ void rtc_reset(struct domain *d)
RTCState *s = domain_vrtc(d);
destroy_periodic_time(&s->pt);
+ s->pt_code = 0;
s->pt.source = PTSRC_isa;
}
diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
index 2e9c7d2..ea9df42 100644
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -104,16 +104,16 @@ typedef struct RTCState {
struct hvm_hw_rtc hw;
/* RTC''s idea of the current time */
struct tm current_tm;
- /* periodic timer */
- s_time_t start_time;
- /* second update */
- struct periodic_time pt;
/* update-ended timer */
struct timer update_timer;
struct timer update_timer2;
+ uint64_t next_update_time;
/* alarm timer */
struct timer alarm_timer;
- uint64_t next_update_time;
+ /* periodic timer */
+ struct periodic_time pt;
+ s_time_t start_time;
+ int pt_code;
uint32_t use_timer;
spinlock_t lock;
} RTCState;
--
1.7.10.4
Tim Deegan
2013-Mar-28 16:09 UTC
[PATCH 3/4] x86/hvm: Let the guest miss a few ticks before resetting the timer.
Signed-off-by: Tim Deegan <tim@xen.org>
---
xen/arch/x86/hvm/rtc.c | 15 +++++++++------
xen/include/asm-x86/hvm/vpt.h | 1 +
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index 8d9195f..88cacd1 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -65,17 +65,18 @@ void rtc_periodic_interrupt(void *opaque)
RTCState *s = opaque;
spin_lock(&s->lock);
- if ( s->hw.cmos_data[RTC_REG_C] & RTC_PF )
- {
- destroy_periodic_time(&s->pt);
- s->pt_code = 0;
- }
- else
+ if ( !(s->hw.cmos_data[RTC_REG_C] & RTC_PF) )
{
s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
if ( s->hw.cmos_data[RTC_REG_B] & RTC_PIE )
rtc_toggle_irq(s);
}
+ else if ( ++(s->pt_dead_ticks) >= 10 )
+ {
+ /* VM is ignoring its RTC; no point in running the timer */
+ destroy_periodic_time(&s->pt);
+ s->pt_code = 0;
+ }
spin_unlock(&s->lock);
}
@@ -88,6 +89,8 @@ static void rtc_timer_update(RTCState *s)
ASSERT(spin_is_locked(&s->lock));
+ s->pt_dead_ticks = 0;
+
period_code = s->hw.cmos_data[RTC_REG_A] & RTC_RATE_SELECT;
switch ( s->hw.cmos_data[RTC_REG_A] & RTC_DIV_CTL )
{
diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
index ea9df42..765fbb7 100644
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -114,6 +114,7 @@ typedef struct RTCState {
struct periodic_time pt;
s_time_t start_time;
int pt_code;
+ uint8_t pt_dead_ticks;
uint32_t use_timer;
spinlock_t lock;
} RTCState;
--
1.7.10.4
Tim Deegan
2013-Mar-28 16:09 UTC
[PATCH 4/4] x86/hvm: Centralize and simplify the RTC IRQ logic.
This keeps the behaviour of strobing the IRQ line every time any RTC
interrupt source is raised. I rather suspect (based on the behaviour
of the MC146818A RTC) that we ought to be suppressing all subsequent
interrupts whenever RTC_IRQF is set, but this way avoids making
guest-visible changes.
Signed-off-by: Tim Deegan <tim@xen.org>
---
xen/arch/x86/hvm/rtc.c | 40 ++++++++++++++++++++--------------------
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index 88cacd1..26ab12c 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -50,14 +50,25 @@ static void rtc_set_time(RTCState *s);
static inline int from_bcd(RTCState *s, int a);
static inline int convert_hour(RTCState *s, int hour);
-static void rtc_toggle_irq(RTCState *s)
+static void rtc_update_irq(RTCState *s)
{
struct domain *d = vrtc_domain(s);
+ uint8_t irqf;
ASSERT(spin_is_locked(&s->lock));
- s->hw.cmos_data[RTC_REG_C] |= RTC_IRQF;
+
+ /* 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);
- hvm_isa_irq_assert(d, RTC_IRQ);
+ if ( irqf )
+ hvm_isa_irq_assert(d, RTC_IRQ);
}
void rtc_periodic_interrupt(void *opaque)
@@ -68,8 +79,7 @@ void rtc_periodic_interrupt(void *opaque)
if ( !(s->hw.cmos_data[RTC_REG_C] & RTC_PF) )
{
s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
- if ( s->hw.cmos_data[RTC_REG_B] & RTC_PIE )
- rtc_toggle_irq(s);
+ rtc_update_irq(s);
}
else if ( ++(s->pt_dead_ticks) >= 10 )
{
@@ -187,8 +197,7 @@ static void rtc_update_timer2(void *opaque)
{
s->hw.cmos_data[RTC_REG_C] |= RTC_UF;
s->hw.cmos_data[RTC_REG_A] &= ~RTC_UIP;
- if ((s->hw.cmos_data[RTC_REG_B] & RTC_UIE))
- rtc_toggle_irq(s);
+ rtc_update_irq(s);
check_update_timer(s);
}
spin_unlock(&s->lock);
@@ -378,9 +387,7 @@ static void rtc_alarm_cb(void *opaque)
if (!(s->hw.cmos_data[RTC_REG_B] & RTC_SET))
{
s->hw.cmos_data[RTC_REG_C] |= RTC_AF;
- /* alarm interrupt */
- if (s->hw.cmos_data[RTC_REG_B] & RTC_AIE)
- rtc_toggle_irq(s);
+ rtc_update_irq(s);
alarm_timer_update(s);
}
spin_unlock(&s->lock);
@@ -390,7 +397,7 @@ static int rtc_ioport_write(void *opaque, uint32_t addr,
uint32_t data)
{
RTCState *s = opaque;
struct domain *d = vrtc_domain(s);
- uint32_t orig, mask;
+ uint32_t orig;
spin_lock(&s->lock);
@@ -464,15 +471,8 @@ static int rtc_ioport_write(void *opaque, uint32_t addr,
uint32_t data)
/*
* If the interrupt is already set when the interrupt becomes
* enabled, raise an interrupt immediately.
- * NB: RTC_{A,P,U}IE == RTC_{A,P,U}F respectively.
*/
- for ( mask = RTC_UIE; mask <= RTC_PIE; mask <<= 1 )
- if ( (data & mask) && !(orig & mask) &&
- (s->hw.cmos_data[RTC_REG_C] & mask) )
- {
- rtc_toggle_irq(s);
- break;
- }
+ rtc_update_irq(s);
s->hw.cmos_data[RTC_REG_B] = data;
if ( (data ^ orig) & RTC_SET )
check_update_timer(s);
@@ -628,8 +628,8 @@ static uint32_t rtc_ioport_read(RTCState *s, uint32_t addr)
break;
case RTC_REG_C:
ret = s->hw.cmos_data[s->hw.cmos_index];
- hvm_isa_irq_deassert(vrtc_domain(s), RTC_IRQ);
s->hw.cmos_data[RTC_REG_C] = 0x00;
+ rtc_update_irq(s);
check_update_timer(s);
alarm_timer_update(s);
rtc_timer_update(s);
--
1.7.10.4
Jan Beulich
2013-Mar-28 16:18 UTC
Re: [PATCH 0/4 v2] x86/hvm: RTC periodic timer adjustments
>>> On 28.03.13 at 17:09, Tim Deegan <tim@xen.org> wrote: > With these four patches, the time drift that was happening on Windows XP > guests goes away, at least in my testing. 3/4 is probably the most > important (with the other three, I still see slow drift), but the others > all seem like a good idea to me. > > Changes since v1: > - moved the irq logic patch to the end of the queue so the others > can be applied independently > - in the irq logic, keep the strobing behaviour as before > - store period_code instead of period and pt_activeAcked-by: Jan Beulich <jbeulich@suse.com> Of course it would be nice if Suravee could test this in his setup, so we''d have another data point on the positive effects this has. Jan
Keir Fraser
2013-Mar-28 19:57 UTC
Re: [PATCH 0/4 v2] x86/hvm: RTC periodic timer adjustments
On 28/03/2013 16:09, "Tim Deegan" <tim@xen.org> wrote:> With these four patches, the time drift that was happening on Windows XP > guests goes away, at least in my testing. 3/4 is probably the most > important (with the other three, I still see slow drift), but the others > all seem like a good idea to me. > > Changes since v1: > - moved the irq logic patch to the end of the queue so the others > can be applied independently > - in the irq logic, keep the strobing behaviour as before > - store period_code instead of period and pt_activeAcked-by: Keir Fraser <keir@xen.org>> Cheers, > > Tim. > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Suravee Suthikulanit
2013-Mar-28 21:36 UTC
Re: [PATCH 0/4 v2] x86/hvm: RTC periodic timer adjustments
On 3/28/2013 11:18 AM, Jan Beulich wrote:>>>> On 28.03.13 at 17:09, Tim Deegan <tim@xen.org> wrote: >> With these four patches, the time drift that was happening on Windows XP >> guests goes away, at least in my testing. 3/4 is probably the most >> important (with the other three, I still see slow drift), but the others >> all seem like a good idea to me. >> >> Changes since v1: >> - moved the irq logic patch to the end of the queue so the others >> can be applied independently >> - in the irq logic, keep the strobing behaviour as before >> - store period_code instead of period and pt_active > Acked-by: Jan Beulich <jbeulich@suse.com> > > Of course it would be nice if Suravee could test this in his setup, > so we''d have another data point on the positive effects this has. > > JanI have tested this and it seems to stop drifting on my same test box that observed the issue before. Suravee