* [PATCH RFC v2 1/3] x86/hvm/rtc: Don't run the vpt timer when !REG_B.PIE.
2014-02-14 19:51 [PATCH RFC v2 0/3] Move RTC interrupt injection back into the vpt code Andrew Cooper
@ 2014-02-14 19:51 ` Andrew Cooper
2014-02-14 23:03 ` Tim Deegan
2014-02-14 19:52 ` [PATCH RFC v2 2/3] x86/hvm/rtc: Inject RTC periodic interupts from the vpt code Andrew Cooper
2014-02-14 19:52 ` [PATCH RFC v2 3/3] x86/hvm/rtc: Always deassert the IRQ line when clearing REG_C.IRQF Andrew Cooper
2 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2014-02-14 19:51 UTC (permalink / raw)
To: Xen-devel
Cc: keir, george.dunlap, Andrew Cooper, Tim Deegan, JBeulich,
roger.pau
From: Tim Deegan <tim@xen.org>
If the guest has not asked for interrupts, don't run the vpt timer
to generate them. This is a prerequisite for a patch to simplify how
the vpt interacts with the RTC, and also gets rid of a timer series in
Xen in a case where it's unlikely to be needed.
Instead, calculate the correct value for REG_C.PF whenever REG_C is
read or PIE is enabled. This allow a guest to poll for the PF bit
while not asking for actual timer interrupts. Such a guest would no
longer get the benefit of the vpt's timer modes.
Signed-off-by: Tim Deegan <tim@xen.org>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes in v2:
* Reduce scope of `now` in rtc_timer_update()
* Merge PIE logic in REG_B write
* Tightly couple setting s->period with creating/destroying timers, so the
timer change gets properly recreated when the guest sets REG_B.PIE
---
xen/arch/x86/hvm/rtc.c | 58 ++++++++++++++++++++++++++++++++---------
xen/include/asm-x86/hvm/vpt.h | 3 ++-
2 files changed, 48 insertions(+), 13 deletions(-)
diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index cdedefe..1455bc6 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -94,7 +94,7 @@ bool_t rtc_periodic_interrupt(void *opaque)
{
/* VM is ignoring its RTC; no point in running the timer */
destroy_periodic_time(&s->pt);
- s->pt_code = 0;
+ s->period = 0;
}
if ( !(s->hw.cmos_data[RTC_REG_C] & RTC_IRQF) )
ret = 0;
@@ -103,6 +103,24 @@ bool_t rtc_periodic_interrupt(void *opaque)
return ret;
}
+/* Check whether the REG_C.PF bit should have been set by a tick since
+ * the last time we looked. This is used to track ticks when REG_B.PIE
+ * is clear; when PIE is set, PF ticks are handled by the VPT callbacks. */
+static void check_for_pf_ticks(RTCState *s)
+{
+ s_time_t now;
+
+ if ( s->period == 0 || (s->hw.cmos_data[RTC_REG_B] & RTC_PIE) )
+ return;
+
+ now = NOW();
+ if ( (now - s->start_time) / s->period
+ != (s->check_ticks_since - s->start_time) / s->period )
+ s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
+
+ s->check_ticks_since = now;
+}
+
/* Enable/configure/disable the periodic timer based on the RTC_PIE and
* RTC_RATE_SELECT settings */
static void rtc_timer_update(RTCState *s)
@@ -125,24 +143,31 @@ static void rtc_timer_update(RTCState *s)
case RTC_REF_CLCK_4MHZ:
if ( period_code != 0 )
{
- if ( period_code != s->pt_code )
+ period = 1 << (period_code - 1); /* period in 32 Khz cycles */
+ period = DIV_ROUND(period * 1000000000ULL, 32768); /* in ns */
+ if ( period != s->period )
{
- s->pt_code = period_code;
- period = 1 << (period_code - 1); /* period in 32 Khz cycles */
- period = DIV_ROUND(period * 1000000000ULL, 32768); /* in ns */
+ s_time_t now = NOW();
+
if ( v->domain->arch.hvm_domain.params[HVM_PARAM_VPT_ALIGN] )
delta = 0;
else
- delta = period - ((NOW() - s->start_time) % period);
- create_periodic_time(v, &s->pt, delta, period,
- RTC_IRQ, NULL, s);
+ delta = period - ((now - s->start_time) % period);
+ if ( s->hw.cmos_data[RTC_REG_B] & RTC_PIE )
+ {
+ create_periodic_time(v, &s->pt, delta, period,
+ RTC_IRQ, NULL, s);
+ s->period = period;
+ }
+ else
+ s->check_ticks_since = now;
}
break;
}
/* fall through */
default:
destroy_periodic_time(&s->pt);
- s->pt_code = 0;
+ s->period = 0;
break;
}
}
@@ -484,14 +509,22 @@ static int rtc_ioport_write(void *opaque, uint32_t addr, uint32_t data)
if ( orig & RTC_SET )
rtc_set_time(s);
}
+ check_for_pf_ticks(s);
s->hw.cmos_data[RTC_REG_B] = data;
/*
* If the interrupt is already set when the interrupt becomes
* enabled, raise an interrupt immediately.
*/
rtc_update_irq(s);
- if ( (data & RTC_PIE) && !(orig & RTC_PIE) )
+ if ( (data ^ orig) & RTC_PIE )
+ {
+ if ( !(data & RTC_PIE) )
+ {
+ destroy_periodic_time(&s->pt);
+ s->period = 0;
+ }
rtc_timer_update(s);
+ }
if ( (data ^ orig) & RTC_SET )
check_update_timer(s);
if ( (data ^ orig) & (RTC_24H | RTC_DM_BINARY | RTC_SET) )
@@ -645,6 +678,7 @@ static uint32_t rtc_ioport_read(RTCState *s, uint32_t addr)
ret |= RTC_UIP;
break;
case RTC_REG_C:
+ check_for_pf_ticks(s);
ret = s->hw.cmos_data[s->hw.cmos_index];
s->hw.cmos_data[RTC_REG_C] = 0x00;
if ( (ret & RTC_IRQF) && !rtc_mode_is(s, no_ack) )
@@ -652,7 +686,7 @@ static uint32_t rtc_ioport_read(RTCState *s, uint32_t addr)
rtc_update_irq(s);
check_update_timer(s);
alarm_timer_update(s);
- rtc_timer_update(s);
+ s->pt_dead_ticks = 0;
break;
default:
ret = s->hw.cmos_data[s->hw.cmos_index];
@@ -748,7 +782,7 @@ void rtc_reset(struct domain *d)
RTCState *s = domain_vrtc(d);
destroy_periodic_time(&s->pt);
- s->pt_code = 0;
+ s->period = 0;
s->pt.source = PTSRC_isa;
}
diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
index 87c3a66..9f48635 100644
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -113,7 +113,8 @@ typedef struct RTCState {
/* periodic timer */
struct periodic_time pt;
s_time_t start_time;
- int pt_code;
+ s_time_t check_ticks_since;
+ int period;
uint8_t pt_dead_ticks;
uint32_t use_timer;
spinlock_t lock;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH RFC v2 1/3] x86/hvm/rtc: Don't run the vpt timer when !REG_B.PIE.
2014-02-14 19:51 ` [PATCH RFC v2 1/3] x86/hvm/rtc: Don't run the vpt timer when !REG_B.PIE Andrew Cooper
@ 2014-02-14 23:03 ` Tim Deegan
2014-02-15 0:02 ` Andrew Cooper
0 siblings, 1 reply; 6+ messages in thread
From: Tim Deegan @ 2014-02-14 23:03 UTC (permalink / raw)
To: Andrew Cooper; +Cc: george.dunlap, roger.pau, keir, JBeulich, Xen-devel
At 19:51 +0000 on 14 Feb (1392403919), Andrew Cooper wrote:
> From: Tim Deegan <tim@xen.org>
>
> If the guest has not asked for interrupts, don't run the vpt timer
> to generate them. This is a prerequisite for a patch to simplify how
> the vpt interacts with the RTC, and also gets rid of a timer series in
> Xen in a case where it's unlikely to be needed.
>
> Instead, calculate the correct value for REG_C.PF whenever REG_C is
> read or PIE is enabled. This allow a guest to poll for the PF bit
> while not asking for actual timer interrupts. Such a guest would no
> longer get the benefit of the vpt's timer modes.
>
> Signed-off-by: Tim Deegan <tim@xen.org>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> ---
>
> Changes in v2:
> * Reduce scope of `now` in rtc_timer_update()
> * Merge PIE logic in REG_B write
> * Tightly couple setting s->period with creating/destroying timers, so the
> timer change gets properly recreated when the guest sets REG_B.PIE
Thanks for sorting out that bug, but I think in this version the !PIE
case won't work. check_for_pf_ticks() uses s->period to figure out
whether to set PF, so it needs to be set whenever the REG_A selector
is configured, even if the timer's not running.
It looks like always setting s->period == 0 just before the call to
rtc_timer_update in the REG_B write (i.e. not just in the !PIE case)
would DTRT, but is that what you tried earlier?
Er, that is, here:
> rtc_update_irq(s);
> - if ( (data & RTC_PIE) && !(orig & RTC_PIE) )
> + if ( (data ^ orig) & RTC_PIE )
> + {
> + if ( !(data & RTC_PIE) )
> + {
> + destroy_periodic_time(&s->pt);
> + s->period = 0;
> + }
> rtc_timer_update(s);
> + }
> if ( (data ^ orig) & RTC_SET )
do this:
> + if ( (data ^ orig) & RTC_PIE )
> + {
> + destroy_periodic_time(&s->pt);
> + s->period = 0;
> rtc_timer_update(s);
> + }
Tim.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RFC v2 1/3] x86/hvm/rtc: Don't run the vpt timer when !REG_B.PIE.
2014-02-14 23:03 ` Tim Deegan
@ 2014-02-15 0:02 ` Andrew Cooper
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2014-02-15 0:02 UTC (permalink / raw)
To: Tim Deegan; +Cc: george.dunlap, roger.pau, keir, JBeulich, Xen-devel
On 14/02/2014 23:03, Tim Deegan wrote:
> At 19:51 +0000 on 14 Feb (1392403919), Andrew Cooper wrote:
>> From: Tim Deegan <tim@xen.org>
>>
>> If the guest has not asked for interrupts, don't run the vpt timer
>> to generate them. This is a prerequisite for a patch to simplify how
>> the vpt interacts with the RTC, and also gets rid of a timer series in
>> Xen in a case where it's unlikely to be needed.
>>
>> Instead, calculate the correct value for REG_C.PF whenever REG_C is
>> read or PIE is enabled. This allow a guest to poll for the PF bit
>> while not asking for actual timer interrupts. Such a guest would no
>> longer get the benefit of the vpt's timer modes.
>>
>> Signed-off-by: Tim Deegan <tim@xen.org>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> ---
>>
>> Changes in v2:
>> * Reduce scope of `now` in rtc_timer_update()
>> * Merge PIE logic in REG_B write
>> * Tightly couple setting s->period with creating/destroying timers, so the
>> timer change gets properly recreated when the guest sets REG_B.PIE
> Thanks for sorting out that bug, but I think in this version the !PIE
> case won't work. check_for_pf_ticks() uses s->period to figure out
> whether to set PF, so it needs to be set whenever the REG_A selector
> is configured, even if the timer's not running.
>
> It looks like always setting s->period == 0 just before the call to
> rtc_timer_update in the REG_B write (i.e. not just in the !PIE case)
> would DTRT, but is that what you tried earlier?
I tried the "careful clobbering" which resulted in the fragment below,
and altering the position of s->period in rtc_timer_update() to match,
but that does indeed lead to issues of s->period being 0 if !PIE.
I clearly should have been less careful.
>
> Er, that is, here:
>
>> rtc_update_irq(s);
>> - if ( (data & RTC_PIE) && !(orig & RTC_PIE) )
>> + if ( (data ^ orig) & RTC_PIE )
>> + {
>> + if ( !(data & RTC_PIE) )
>> + {
>> + destroy_periodic_time(&s->pt);
>> + s->period = 0;
>> + }
>> rtc_timer_update(s);
>> + }
>> if ( (data ^ orig) & RTC_SET )
> do this:
>
>> + if ( (data ^ orig) & RTC_PIE )
>> + {
>> + destroy_periodic_time(&s->pt);
>> + s->period = 0;
>> rtc_timer_update(s);
>> + }
> Tim.
... and undo the changes in rtc_timer_update().
~Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH RFC v2 2/3] x86/hvm/rtc: Inject RTC periodic interupts from the vpt code.
2014-02-14 19:51 [PATCH RFC v2 0/3] Move RTC interrupt injection back into the vpt code Andrew Cooper
2014-02-14 19:51 ` [PATCH RFC v2 1/3] x86/hvm/rtc: Don't run the vpt timer when !REG_B.PIE Andrew Cooper
@ 2014-02-14 19:52 ` Andrew Cooper
2014-02-14 19:52 ` [PATCH RFC v2 3/3] x86/hvm/rtc: Always deassert the IRQ line when clearing REG_C.IRQF Andrew Cooper
2 siblings, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2014-02-14 19:52 UTC (permalink / raw)
To: Xen-devel; +Cc: george.dunlap, Tim Deegan, keir, JBeulich, roger.pau
From: Tim Deegan <tim@xen.org>
Let the vpt code drive the RTC's timer interrupts directly, as it does
for other periodic time sources, and fix up the register state in a
vpt callback when the interrupt is injected.
This fixes a hang seen on Windows 2003 in no-missed-ticks mode, where
when a tick was pending, the early callback from the VPT code would
always set REG_C.PF on every VMENTER; meanwhile the guest was in its
interrupt handler reading REG_C in a loop and waiting to see it clear.
One drawback is that a guest that attempts to suppress RTC periodic
interrupts by failing to read REG_C will receive up to 10 spurious
interrupts, even in 'strict' mode. However:
- since all previous RTC models have had this property (including
the current one, since 'no-ack' mode is hard-coded on) we're
pretty sure that all guests can handle this; and
- we're already playing some other interesting games with this
interrupt in the vpt code.
One other corner case: a guest that enables the PF timer interrupt,
masks the interupt in the APIC and then polls REG_C looking for PF
will not see PF getting set. The more likely case of enabling the
timers and masking the interrupt with REG_B.PIE is already handled
correctly.
Signed-off-by: Tim Deegan <tim@xen.org>
---
xen/arch/x86/hvm/rtc.c | 25 +++++++++++--------------
xen/arch/x86/hvm/vpt.c | 40 ----------------------------------------
xen/include/asm-x86/hvm/vpt.h | 1 -
3 files changed, 11 insertions(+), 55 deletions(-)
diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index 1455bc6..7a37ebb 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -78,29 +78,26 @@ static void rtc_update_irq(RTCState *s)
hvm_isa_irq_assert(vrtc_domain(s), RTC_IRQ);
}
-bool_t rtc_periodic_interrupt(void *opaque)
+/* Called by the VPT code after it's injected a PF interrupt for us.
+ * Fix up the register state to reflect what happened. */
+static void rtc_pf_callback(struct vcpu *v, void *opaque)
{
RTCState *s = opaque;
- bool_t ret;
spin_lock(&s->lock);
- ret = rtc_mode_is(s, no_ack) || !(s->hw.cmos_data[RTC_REG_C] & RTC_IRQF);
- if ( rtc_mode_is(s, no_ack) || !(s->hw.cmos_data[RTC_REG_C] & RTC_PF) )
- {
- s->hw.cmos_data[RTC_REG_C] |= RTC_PF;
- rtc_update_irq(s);
- }
- else if ( ++(s->pt_dead_ticks) >= 10 )
+
+ if ( !rtc_mode_is(s, no_ack)
+ && (s->hw.cmos_data[RTC_REG_C] & RTC_IRQF)
+ && ++(s->pt_dead_ticks) >= 10 )
{
/* VM is ignoring its RTC; no point in running the timer */
destroy_periodic_time(&s->pt);
s->period = 0;
}
- if ( !(s->hw.cmos_data[RTC_REG_C] & RTC_IRQF) )
- ret = 0;
- spin_unlock(&s->lock);
- return ret;
+ s->hw.cmos_data[RTC_REG_C] |= RTC_PF|RTC_IRQF;
+
+ spin_unlock(&s->lock);
}
/* Check whether the REG_C.PF bit should have been set by a tick since
@@ -156,7 +153,7 @@ static void rtc_timer_update(RTCState *s)
if ( s->hw.cmos_data[RTC_REG_B] & RTC_PIE )
{
create_periodic_time(v, &s->pt, delta, period,
- RTC_IRQ, NULL, s);
+ RTC_IRQ, rtc_pf_callback, s);
s->period = period;
}
else
diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
index 1961bda..f7af688 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -231,12 +231,9 @@ int pt_update_irq(struct vcpu *v)
struct periodic_time *pt, *temp, *earliest_pt;
uint64_t max_lag;
int irq, is_lapic;
- void *pt_priv;
- rescan:
spin_lock(&v->arch.hvm_vcpu.tm_lock);
- rescan_locked:
earliest_pt = NULL;
max_lag = -1ULL;
list_for_each_entry_safe ( pt, temp, head, list )
@@ -270,48 +267,11 @@ int pt_update_irq(struct vcpu *v)
earliest_pt->irq_issued = 1;
irq = earliest_pt->irq;
is_lapic = (earliest_pt->source == PTSRC_lapic);
- pt_priv = earliest_pt->priv;
spin_unlock(&v->arch.hvm_vcpu.tm_lock);
if ( is_lapic )
vlapic_set_irq(vcpu_vlapic(v), irq, 0);
- else if ( irq == RTC_IRQ && pt_priv )
- {
- if ( !rtc_periodic_interrupt(pt_priv) )
- irq = -1;
-
- pt_lock(earliest_pt);
-
- if ( irq < 0 && earliest_pt->pending_intr_nr )
- {
- /*
- * RTC periodic timer runs without the corresponding interrupt
- * being enabled - need to mimic enough of pt_intr_post() to keep
- * things going.
- */
- earliest_pt->pending_intr_nr = 0;
- earliest_pt->irq_issued = 0;
- set_timer(&earliest_pt->timer, earliest_pt->scheduled);
- }
- else if ( irq >= 0 && pt_irq_masked(earliest_pt) )
- {
- if ( earliest_pt->on_list )
- {
- /* suspend timer emulation */
- list_del(&earliest_pt->list);
- earliest_pt->on_list = 0;
- }
- irq = -1;
- }
-
- /* Avoid dropping the lock if we can. */
- if ( irq < 0 && v == earliest_pt->vcpu )
- goto rescan_locked;
- pt_unlock(earliest_pt);
- if ( irq < 0 )
- goto rescan;
- }
else
{
hvm_isa_irq_deassert(v->domain, irq);
diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
index 9f48635..7d62653 100644
--- a/xen/include/asm-x86/hvm/vpt.h
+++ b/xen/include/asm-x86/hvm/vpt.h
@@ -184,7 +184,6 @@ void rtc_migrate_timers(struct vcpu *v);
void rtc_deinit(struct domain *d);
void rtc_reset(struct domain *d);
void rtc_update_clock(struct domain *d);
-bool_t rtc_periodic_interrupt(void *);
void pmtimer_init(struct vcpu *v);
void pmtimer_deinit(struct domain *d);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH RFC v2 3/3] x86/hvm/rtc: Always deassert the IRQ line when clearing REG_C.IRQF.
2014-02-14 19:51 [PATCH RFC v2 0/3] Move RTC interrupt injection back into the vpt code Andrew Cooper
2014-02-14 19:51 ` [PATCH RFC v2 1/3] x86/hvm/rtc: Don't run the vpt timer when !REG_B.PIE Andrew Cooper
2014-02-14 19:52 ` [PATCH RFC v2 2/3] x86/hvm/rtc: Inject RTC periodic interupts from the vpt code Andrew Cooper
@ 2014-02-14 19:52 ` Andrew Cooper
2 siblings, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2014-02-14 19:52 UTC (permalink / raw)
To: Xen-devel
Cc: keir, george.dunlap, Andrew Cooper, Tim Deegan, JBeulich,
roger.pau
From: Tim Deegan <tim@xen.org>
Even in no-ack mode, there's no reason to leave the line asserted
after an explicit ack of the interrupt.
Furthermore, rtc_update_irq() is an unconditional noop having just cleared
RTC_REG_C.
Signed-off-by: Tim Deegan <tim@xen.org>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/hvm/rtc.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/xen/arch/x86/hvm/rtc.c b/xen/arch/x86/hvm/rtc.c
index 7a37ebb..1844f2d 100644
--- a/xen/arch/x86/hvm/rtc.c
+++ b/xen/arch/x86/hvm/rtc.c
@@ -678,9 +678,8 @@ static uint32_t rtc_ioport_read(RTCState *s, uint32_t addr)
check_for_pf_ticks(s);
ret = s->hw.cmos_data[s->hw.cmos_index];
s->hw.cmos_data[RTC_REG_C] = 0x00;
- if ( (ret & RTC_IRQF) && !rtc_mode_is(s, no_ack) )
+ if ( ret & RTC_IRQF )
hvm_isa_irq_deassert(d, RTC_IRQ);
- rtc_update_irq(s);
check_update_timer(s);
alarm_timer_update(s);
s->pt_dead_ticks = 0;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread