Re: add back pending timer irqs for kernel APIC timer

Dong, Eddie
2007-08-18T10:22:21Z

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

Avi:
Any comments to this patch? Seems not in yet.
The sequence is apic-timer-TMCCT.patch, apic-timer-last5.patch and apic-timer-pending4.patch.
thx,eddie

>-----Original Message-----
>From: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>[mailto:kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org] On Behalf Of
>Dong, Eddie
>Sent: 2007年8月14日 11:25
>To: Avi Kivity
>Cc: kvm-devel
>Subject: Re: [kvm-devel] add back pending timer irqs for
>kernel APIC timer
>
>Here are rebased and fixed patches, please check.
>thanks, eddie
>
>

[-- Attachment #2: apic-timer-tmcct.patch --]
[-- Type: application/octet-stream, Size: 2281 bytes --]

commit 9b1a9ad955d9ac44ee6e60b4a565edf044d3dcc7
Author: root <root@vt32-pae.(none)>
Date: Fri Aug 10 22:38:59 2007 +0800

APIC_TMCCT is a dynamically running count which always
need to be recalculated at the time it is read. Setting
intermediate value is meaningless.

Signed-off-by: Yaozu (Eddie) Dong <Eddie.Dong@intel.com>

diff --git a/drivers/kvm/lapic.c b/drivers/kvm/lapic.c
index d59b69f..fc53e88 100644
--- a/drivers/kvm/lapic.c
+++ b/drivers/kvm/lapic.c
@@ -445,7 +445,7 @@ static u32 apic_get_tmcct(struct kvm_lapic *apic)
{
u32 counter_passed;
ktime_t passed, now = apic->timer.dev.base->get_time();
- u32 tmcct = apic_get_reg(apic, APIC_TMCCT);
+ u32 tmcct = apic_get_reg(apic, APIC_TMICT);

ASSERT(apic != NULL);

@@ -474,9 +474,6 @@ static u32 apic_get_tmcct(struct kvm_lapic *apic)
} while (tmcct <= 0);
}

- apic->timer.last_update = now;
- apic_set_reg(apic, APIC_TMCCT, tmcct);
-
return tmcct;
}

@@ -633,7 +630,6 @@ static void apic_mmio_write(struct kvm_io_device *this,
u32 offset;

apic_set_reg(apic, APIC_TMICT, val);
- apic_set_reg(apic, APIC_TMCCT, val);
apic->timer.last_update = now;
offset =
APIC_BUS_CYCLE_NS * apic->timer.divide_count * val;
@@ -815,7 +811,6 @@ static void lapic_reset(struct kvm_vcpu *vcpu)
apic_set_reg(apic, APIC_ICR2, 0);
apic_set_reg(apic, APIC_TDCR, 0);
apic_set_reg(apic, APIC_TMICT, 0);
- apic_set_reg(apic, APIC_TMCCT, 0);
for (i = 0; i < 8; i++) {
apic_set_reg(apic, APIC_IRR + 0x10 * i, 0);
apic_set_reg(apic, APIC_ISR + 0x10 * i, 0);
@@ -886,13 +881,10 @@ static int __apic_timer_fn(struct kvm_lapic *apic)
u32 offset;
u32 tmict = apic_get_reg(apic, APIC_TMICT);

- apic_set_reg(apic, APIC_TMCCT, tmict);
offset = APIC_BUS_CYCLE_NS * apic->timer.divide_count * tmict;

result = 1;
apic->timer.dev.expires = ktime_add_ns(now, offset);
- } else {
- apic_set_reg(apic, APIC_TMCCT, 0);
}

return result;
@@ -996,7 +988,6 @@ void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu)
MSR_IA32_APICBASE_BASE;
apic_set_reg(apic, APIC_LVR, APIC_VERSION);
val = apic_get_reg(apic, APIC_TMICT);
- apic_set_reg(apic, APIC_TMCCT, val);
apic_update_ppr(apic);

/* TODO: following code can be in a common API */

[-- Attachment #3: apic-timer-last5.patch --]
[-- Type: application/octet-stream, Size: 4220 bytes --]

In order to calculate instant APIC counter register value,
KVM refers host time stamp of previous APIC timer firing point
(last_update) to get the time passed and thus calculate
current APIC time counter register. Using NOW of
the host hrtime firing time will have accumulated difference
since the timer callback is executed with uncertainity delay,
and it may introduce many drift as the time goes on.

This patch fix this by adjust last_update by expected period.

Signed-off-by: Yaozu (Eddie) Dong <Eddie.Dong@intel.com>
diff --git a/drivers/kvm/irq.c b/drivers/kvm/irq.c
diff --git a/drivers/kvm/irq.h b/drivers/kvm/irq.h
index 44e1fa4..ed6d20a 100644
--- a/drivers/kvm/irq.h
+++ b/drivers/kvm/irq.h
@@ -111,6 +111,7 @@ struct kvm_lapic {
struct kvm_io_device dev;
struct {
unsigned long pending;
+ s64 period; /* unit: ns */
u32 divide_count;
ktime_t last_update;
struct hrtimer dev;
diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
diff --git a/drivers/kvm/lapic.c b/drivers/kvm/lapic.c
index 1430667..09aadcb 100644
--- a/drivers/kvm/lapic.c
+++ b/drivers/kvm/lapic.c
@@ -350,18 +350,6 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
return result;
}

-static inline int apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
- int vector, int level, int trig_mode)
-{
- int result = 0;
-
- spin_lock_bh(&apic->lock);
- result = __apic_accept_irq(apic, delivery_mode,
- vector, level, trig_mode);
- spin_unlock_bh(&apic->lock);
- return result;
-}
-
struct kvm_lapic *kvm_apic_round_robin(struct kvm *kvm, u8 vector,
unsigned long bitmap)
{
@@ -625,27 +613,28 @@ static void apic_mmio_write(struct kvm_io_device *this,
case APIC_TMICT:
{
ktime_t now = apic->timer.dev.base->get_time();
- u32 offset;

apic_set_reg(apic, APIC_TMICT, val);
apic->timer.last_update = now;
- offset =
+ apic->timer.period =
APIC_BUS_CYCLE_NS * apic->timer.divide_count * val;

/* Make sure the lock ordering is coherent */
spin_unlock_bh(&apic->lock);
hrtimer_cancel(&apic->timer.dev);
hrtimer_start(&apic->timer.dev,
- ktime_add_ns(now, offset),
+ ktime_add_ns(now, apic->timer.period),
HRTIMER_MODE_ABS);

apic_debug("%s: bus cycle is %" PRId64 "ns, now 0x%016"
PRIx64 ", "
- "timer initial count 0x%x, offset 0x%x, "
+ "timer initial count 0x%x, period %lldns, "
"expire @ 0x%016" PRIx64 ".\n", __FUNCTION__,
APIC_BUS_CYCLE_NS, ktime_to_ns(now),
- apic_get_reg(apic, APIC_TMICT), offset,
- ktime_to_ns(ktime_add_ns(now, offset)));
+ apic_get_reg(apic, APIC_TMICT),
+ apic->timer.period,
+ ktime_to_ns(ktime_add_ns(now,
+ apic->timer.period)));
}
return;

@@ -760,7 +749,7 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)

/* with FSB delivery interrupt, we can restart APIC functionality */
apic_debug("apic base msr is 0x%016" PRIx64 ", and base address is "
- "0x%lx.\n", apic->apic_base, apic->base_address);
+ "0x%lx.\n", apic->vcpu->apic_base, apic->base_address);

spin_unlock_bh(&apic->lock);
}
@@ -859,7 +848,6 @@ EXPORT_SYMBOL_GPL(kvm_lapic_get_regs);
static int __apic_timer_fn(struct kvm_lapic *apic)
{
u32 vector;
- ktime_t now;
int result = 0;

if (unlikely(!apic_enabled(apic) ||
@@ -870,8 +858,7 @@ static int __apic_timer_fn(struct kvm_lapic *apic)
}

vector = apic_lvt_vector(apic, APIC_LVTT);
- now = apic->timer.dev.base->get_time();
- apic->timer.last_update = now;
+ apic->timer.last_update = apic->timer.dev.expires;
apic->timer.pending++;
__apic_accept_irq(apic, APIC_DM_FIXED, vector, 1, 0);

@@ -882,7 +869,9 @@ static int __apic_timer_fn(struct kvm_lapic *apic)
offset = APIC_BUS_CYCLE_NS * apic->timer.divide_count * tmict;

result = 1;
- apic->timer.dev.expires = ktime_add_ns(now, offset);
+ apic->timer.dev.expires = ktime_add_ns(
+ apic->timer.dev.expires,
+ apic->timer.period);
}

return result;
diff --git a/drivers/kvm/svm.c b/drivers/kvm/svm.c
diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c

[-- Attachment #4: apic-timer-pending4.patch --]
[-- Type: application/octet-stream, Size: 9108 bytes --]

commit f1c8d1076b48625469b69abd6767bf429f35120d
Author: root <root@vt32-pae.(none)>
Date: Tue Aug 14 11:22:47 2007 +0800

APIC timer IRQ is set every time when a certain period
expires at host time, but the guest may be descheduled
at that time and thus the irq be overwritten by later fire.
This patch keep track of firing irq numbers and decrease
only when the IRQ is injected to guest or buffered in
APIC.

Signed-off-by: Yaozu (Eddie) Dong <Eddie.Dong@intel.com>

diff --git a/drivers/kvm/irq.c b/drivers/kvm/irq.c
index e09cd65..eb72b56 100644
--- a/drivers/kvm/irq.c
+++ b/drivers/kvm/irq.c
@@ -78,3 +78,17 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
smp_call_function_single(ipi_pcpu, vcpu_kick_intr, vcpu, 0, 0);
}

+void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu)
+{
+ kvm_inject_apic_timer_irqs(vcpu);
+ /* TODO: PIT, RTC etc. */
+}
+EXPORT_SYMBOL_GPL(kvm_inject_pending_timer_irqs);
+
+void kvm_timer_intr_post(struct kvm_vcpu *vcpu, int vec)
+{
+ kvm_apic_timer_intr_post(vcpu, vec);
+ /* TODO: PIT, RTC etc. */
+}
+EXPORT_SYMBOL_GPL(kvm_timer_intr_post);
+
diff --git a/drivers/kvm/irq.h b/drivers/kvm/irq.h
index ed6d20a..419004e 100644
--- a/drivers/kvm/irq.h
+++ b/drivers/kvm/irq.h
@@ -110,7 +110,7 @@ struct kvm_lapic {
unsigned long base_address;
struct kvm_io_device dev;
struct {
- unsigned long pending;
+ atomic_t pending;
s64 period; /* unit: ns */
u32 divide_count;
ktime_t last_update;
@@ -153,5 +153,9 @@ int kvm_apic_set_irq(struct kvm_lapic *apic, u8 vec, u8 trig);
void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu);
int kvm_ioapic_init(struct kvm *kvm);
void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
+void kvm_apic_timer_intr_post(struct kvm_vcpu *vcpu, int vec);
+void kvm_timer_intr_post(struct kvm_vcpu *vcpu, int vec);
+void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu);
+void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu);

#endif
diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index c59e761..bb3fd24 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -283,6 +283,8 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_init);
void kvm_vcpu_uninit(struct kvm_vcpu *vcpu)
{
kvm_mmu_destroy(vcpu);
+ if (vcpu->apic)
+ hrtimer_cancel(&vcpu->apic->timer.dev);
kvm_free_apic(vcpu->apic);
free_page((unsigned long)vcpu->pio_data);
free_page((unsigned long)vcpu->run);
diff --git a/drivers/kvm/lapic.c b/drivers/kvm/lapic.c
index 09aadcb..a6fec50 100644
--- a/drivers/kvm/lapic.c
+++ b/drivers/kvm/lapic.c
@@ -30,6 +30,7 @@
#include <asm/page.h>
#include <asm/current.h>
#include <asm/apicdef.h>
+#include <asm/atomic.h>
#include "irq.h"

#define PRId64 "d"
@@ -298,6 +299,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
int vector, int level, int trig_mode)
{
int result = 0;
+ int orig_irr;

switch (delivery_mode) {
case APIC_DM_FIXED:
@@ -306,7 +308,8 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
if (unlikely(!apic_enabled(apic)))
break;

- if (apic_test_and_set_irr(vector, apic) && trig_mode) {
+ orig_irr = apic_test_and_set_irr(vector, apic);
+ if (orig_irr && trig_mode) {
apic_debug("level trig mode repeatedly for vector %d",
vector);
break;
@@ -320,7 +323,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,

kvm_vcpu_kick(apic->vcpu);

- result = 1;
+ result = (orig_irr == 0);
break;

case APIC_DM_REMRD:
@@ -581,6 +584,7 @@ static void apic_mmio_write(struct kvm_io_device *this,
apic_set_reg(apic, APIC_LVTT + 0x10 * i,
lvt_val | APIC_LVT_MASKED);
}
+ atomic_set(&apic->timer.pending, 0);

}
break;
@@ -622,6 +626,7 @@ static void apic_mmio_write(struct kvm_io_device *this,
/* Make sure the lock ordering is coherent */
spin_unlock_bh(&apic->lock);
hrtimer_cancel(&apic->timer.dev);
+ atomic_set(&apic->timer.pending, 0);
hrtimer_start(&apic->timer.dev,
ktime_add_ns(now, apic->timer.period),
HRTIMER_MODE_ABS);
@@ -804,7 +809,7 @@ static void lapic_reset(struct kvm_vcpu *vcpu)
apic_set_reg(apic, APIC_TMR + 0x10 * i, 0);
}
apic->timer.divide_count = 0;
- apic->timer.pending = 0;
+ atomic_set(&apic->timer.pending, 0);
if (vcpu->vcpu_id == 0)
vcpu->apic_base |= MSR_IA32_APICBASE_BSP;
apic_update_ppr(apic);
@@ -845,38 +850,33 @@ EXPORT_SYMBOL_GPL(kvm_lapic_get_regs);
* timer interface
*----------------------------------------------------------------------
*/
+
+/* TODO: make sure __apic_timer_fn runs in current pCPU */
static int __apic_timer_fn(struct kvm_lapic *apic)
{
- u32 vector;
int result = 0;
+ wait_queue_head_t *q = &apic->vcpu->wq;

- if (unlikely(!apic_enabled(apic) ||
- !apic_lvt_enabled(apic, APIC_LVTT))) {
- apic_debug("%s: time interrupt although apic is down\n",
- __FUNCTION__);
- return 0;
- }
-
- vector = apic_lvt_vector(apic, APIC_LVTT);
- apic->timer.last_update = apic->timer.dev.expires;
- apic->timer.pending++;
- __apic_accept_irq(apic, APIC_DM_FIXED, vector, 1, 0);
-
+ atomic_inc(&apic->timer.pending);
+ if (waitqueue_active(q))
+ wake_up_interruptible(q);
if (apic_lvtt_period(apic)) {
- u32 offset;
- u32 tmict = apic_get_reg(apic, APIC_TMICT);
-
- offset = APIC_BUS_CYCLE_NS * apic->timer.divide_count * tmict;
-
result = 1;
apic->timer.dev.expires = ktime_add_ns(
apic->timer.dev.expires,
apic->timer.period);
}
-
return result;
}

+static int __inject_apic_timer_irq(struct kvm_lapic *apic)
+{
+ int vector;
+
+ vector = apic_lvt_vector(apic, APIC_LVTT);
+ return __apic_accept_irq(apic, APIC_DM_FIXED, vector, 1, 0);
+}
+
static enum hrtimer_restart apic_timer_fn(struct hrtimer *data)
{
struct kvm_lapic *apic;
@@ -951,6 +951,26 @@ int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu)
return highest_irr;
}

+void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu)
+{
+ struct kvm_lapic *apic = vcpu->apic;
+
+ if (apic && atomic_read(&apic->timer.pending) > 0) {
+ if (__inject_apic_timer_irq(apic))
+ atomic_dec(&apic->timer.pending);
+ }
+}
+
+void kvm_apic_timer_intr_post(struct kvm_vcpu *vcpu, int vec)
+{
+ struct kvm_lapic *apic = vcpu->apic;
+
+ if (apic && apic_lvt_vector(apic, APIC_LVTT) == vec)
+ apic->timer.last_update = ktime_add_ns(
+ apic->timer.last_update,
+ apic->timer.period);
+}
+
int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
{
int vector = kvm_apic_has_interrupt(vcpu);
@@ -980,7 +1000,7 @@ void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu)
/* TODO: following code can be in a common API */
spin_lock_bh(&apic->lock);
hrtimer_cancel(&apic->timer.dev);
- apic->timer.pending = 0;
+ atomic_set(&apic->timer.pending, 0);
val = apic_get_reg(apic, APIC_TDCR);
tmp = ((val & 0x3) | ((val & 0x8) >> 1)) + 1;
apic->timer.divide_count = 0x1 << (tmp & 0x7);
diff --git a/drivers/kvm/svm.c b/drivers/kvm/svm.c
index 139499b..b080feb 100644
--- a/drivers/kvm/svm.c
+++ b/drivers/kvm/svm.c
@@ -1331,7 +1331,9 @@ static void svm_intr_assist(struct vcpu_svm *svm)
{
struct vmcb *vmcb = svm->vmcb;
int intr_vector = -1;
+ struct kvm_vcpu *vcpu = &svm->vcpu;

+ kvm_inject_pending_timer_irqs(vcpu);
if ((vmcb->control.exit_int_info & SVM_EVTINJ_VALID) &&
((vmcb->control.exit_int_info & SVM_EVTINJ_TYPE_MASK) == 0)) {
intr_vector = vmcb->control.exit_int_info &
@@ -1344,7 +1346,7 @@ static void svm_intr_assist(struct vcpu_svm *svm)
if (vmcb->control.int_ctl & V_IRQ_MASK)
return;

- if (!kvm_cpu_has_interrupt(&svm->vcpu))
+ if (!kvm_cpu_has_interrupt(vcpu))
return;

if (!(vmcb->save.rflags & X86_EFLAGS_IF) ||
@@ -1356,8 +1358,9 @@ static void svm_intr_assist(struct vcpu_svm *svm)
return;
}
/* Okay, we can deliver the interrupt: grab it and update PIC state. */
- intr_vector = kvm_cpu_get_interrupt(&svm->vcpu);
+ intr_vector = kvm_cpu_get_interrupt(vcpu);
svm_inject_irq(svm, intr_vector);
+ kvm_timer_intr_post(vcpu, intr_vector);
}

static void kvm_reput_irq(struct vcpu_svm *svm)
diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
index aad02c5..9fa7356 100644
--- a/drivers/kvm/vmx.c
+++ b/drivers/kvm/vmx.c
@@ -2091,7 +2091,9 @@ static void vmx_intr_assist(struct kvm_vcpu *vcpu)
{
u32 idtv_info_field, intr_info_field;
int has_ext_irq, interrupt_window_open;
+ int vector;

+ kvm_inject_pending_timer_irqs(vcpu);
has_ext_irq = kvm_cpu_has_interrupt(vcpu);
intr_info_field = vmcs_read32(VM_ENTRY_INTR_INFO_FIELD);
idtv_info_field = vmcs_read32(IDT_VECTORING_INFO_FIELD);
@@ -2121,9 +2123,11 @@ static void vmx_intr_assist(struct kvm_vcpu *vcpu)
interrupt_window_open =
((vmcs_readl(GUEST_RFLAGS) & X86_EFLAGS_IF) &&
(vmcs_read32(GUEST_INTERRUPTIBILITY_INFO) & 3) == 0);
- if (interrupt_window_open)
- vmx_inject_irq(vcpu, kvm_cpu_get_interrupt(vcpu));
- else
+ if (interrupt_window_open) {
+ vector = kvm_cpu_get_interrupt(vcpu);
+ vmx_inject_irq(vcpu, vector);
+ kvm_timer_intr_post(vcpu, vector);
+ } else
enable_irq_window(vcpu);
}

[-- Attachment #5: Type: text/plain, Size: 315 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

[-- Attachment #6: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org