[PATCH v2 0/3]: Fixes to IRQ routing

[PATCH v2 0/3]: Fixes to IRQ routing

[Chris Lalancette]

As we've discussed previously, here is a series of patches to
fix some of the IRQ routing issues we have in KVM.  With this series
in place I was able to successfully kdump a RHEL-5 64-bit, and RHEL-6
32- and 64-bit guest on CPU's other than the BSP.  RHEL-5 32-bit kdump still
does not work; it gets stuck on "Checking 'hlt' instruction".  However,
it does that both before and after this series, so there is something
else going on there that I still have to debug.

I also need to change the "kvm_migrate_pit_timer" function to migrate the
timer over to the last CPU that handled the timer interrupt, on the
theory that that particlar CPU is likely to handle the timer interrupt again
in the near future.  However, this is an optimization and shouldn't delay the
inclusion of the rest of the series for correctness.

Changes since RFC:
     - Changed ps->inject_lock from raw_spinlock_t to spinlock_t
     - Fixed up some formatting issues
     - Changed to have one PIT workqueue per-guest
     - Remember to cancel_work_sync when destroying the PIT

Changes since v1:
     - Call cancel_work_sync everywhere we call hrtimer_cancel
     - Bring back the reinjection logic
     - Fix up formatting issues from checkpatch

[PATCH v2 1/3] Introduce a workqueue to deliver PIT timer interrupts.

[Chris Lalancette]

We really want to "kvm_set_irq" during the hrtimer callback,
but that is risky because that is during interrupt context.
Instead, offload the work to a workqueue, which is a bit safer
and should provide most of the same functionality.

Signed-off-by: Chris Lalancette <clalance@redhat.com>
---
 arch/x86/kvm/i8254.c |  125 ++++++++++++++++++++++++++++---------------------
 arch/x86/kvm/i8254.h |    4 +-
 arch/x86/kvm/irq.c   |    1 -
 3 files changed, 74 insertions(+), 56 deletions(-)

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index 188d827..3bed8ac 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -34,6 +34,7 @@
 
 #include <linux/kvm_host.h>
 #include <linux/slab.h>
+#include <linux/workqueue.h>
 
 #include "irq.h"
 #include "i8254.h"
@@ -244,11 +245,11 @@ static void kvm_pit_ack_irq(struct kvm_irq_ack_notifier *kian)
 {
 	struct kvm_kpit_state *ps = container_of(kian, struct kvm_kpit_state,
 						 irq_ack_notifier);
-	raw_spin_lock(&ps->inject_lock);
+	spin_lock(&ps->inject_lock);
 	if (atomic_dec_return(&ps->pit_timer.pending) < 0)
 		atomic_inc(&ps->pit_timer.pending);
 	ps->irq_ack = 1;
-	raw_spin_unlock(&ps->inject_lock);
+	spin_unlock(&ps->inject_lock);
 }
 
 void __kvm_migrate_pit_timer(struct kvm_vcpu *vcpu)
@@ -267,7 +268,8 @@ void __kvm_migrate_pit_timer(struct kvm_vcpu *vcpu)
 static void destroy_pit_timer(struct kvm_timer *pt)
 {
 	pr_debug("execute del timer!\n");
-	hrtimer_cancel(&pt->timer);
+	if (hrtimer_cancel(&pt->timer))
+		cancel_work_sync(&pt->kvm->arch.vpit->expired);
 }
 
 static bool kpit_is_periodic(struct kvm_timer *ktimer)
@@ -281,6 +283,58 @@ static struct kvm_timer_ops kpit_ops = {
 	.is_periodic = kpit_is_periodic,
 };
 
+static void pit_do_work(struct work_struct *work)
+{
+	struct kvm_pit *pit = container_of(work, struct kvm_pit, expired);
+	struct kvm *kvm = pit->kvm;
+	struct kvm_vcpu *vcpu;
+	int i;
+	struct kvm_kpit_state *ps = &pit->pit_state;
+	int inject = 0;
+
+	/* Try to inject pending interrupts when
+	 * last one has been acked.
+	 */
+	spin_lock(&ps->inject_lock);
+	if (ps->irq_ack) {
+		ps->irq_ack = 0;
+		inject = 1;
+	}
+	spin_unlock(&ps->inject_lock);
+	if (inject) {
+		kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1);
+		kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 0);
+
+		/*
+		 * Provides NMI watchdog support via Virtual Wire mode.
+		 * The route is: PIT -> PIC -> LVT0 in NMI mode.
+		 *
+		 * Note: Our Virtual Wire implementation is simplified, only
+		 * propagating PIT interrupts to all VCPUs when they have set
+		 * LVT0 to NMI delivery. Other PIC interrupts are just sent to
+		 * VCPU0, and only if its LVT0 is in EXTINT mode.
+		 */
+		if (kvm->arch.vapics_in_nmi_mode > 0)
+			kvm_for_each_vcpu(i, vcpu, kvm)
+				kvm_apic_nmi_wd_deliver(vcpu);
+	}
+}
+
+static enum hrtimer_restart pit_timer_fn(struct hrtimer *data)
+{
+	struct kvm_timer *ktimer = container_of(data, struct kvm_timer, timer);
+	struct kvm_pit *pt = ktimer->kvm->arch.vpit;
+
+	if (ktimer->reinject)
+		queue_work(pt->wq, &pt->expired);
+
+	if (ktimer->t_ops->is_periodic(ktimer)) {
+		hrtimer_add_expires_ns(&ktimer->timer, ktimer->period);
+		return HRTIMER_RESTART;
+	} else
+		return HRTIMER_NORESTART;
+}
+
 static void create_pit_timer(struct kvm_kpit_state *ps, u32 val, int is_period)
 {
 	struct kvm_timer *pt = &ps->pit_timer;
@@ -291,14 +345,14 @@ static void create_pit_timer(struct kvm_kpit_state *ps, u32 val, int is_period)
 	pr_debug("create pit timer, interval is %llu nsec\n", interval);
 
 	/* TODO The new value only affected after the retriggered */
-	hrtimer_cancel(&pt->timer);
+	if (hrtimer_cancel(&pt->timer))
+		cancel_work_sync(&pt->kvm->arch.vpit->expired);
 	pt->period = interval;
 	ps->is_periodic = is_period;
 
-	pt->timer.function = kvm_timer_fn;
+	pt->timer.function = pit_timer_fn;
 	pt->t_ops = &kpit_ops;
 	pt->kvm = ps->pit->kvm;
-	pt->vcpu = pt->kvm->bsp_vcpu;
 
 	atomic_set(&pt->pending, 0);
 	ps->irq_ack = 1;
@@ -626,7 +680,14 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
 
 	mutex_init(&pit->pit_state.lock);
 	mutex_lock(&pit->pit_state.lock);
-	raw_spin_lock_init(&pit->pit_state.inject_lock);
+	spin_lock_init(&pit->pit_state.inject_lock);
+
+	pit->wq = create_singlethread_workqueue("kvm-pit-wq");
+	if (!pit->wq) {
+		kfree(pit);
+		return NULL;
+	}
+	INIT_WORK(&pit->expired, pit_do_work);
 
 	kvm->arch.vpit = pit;
 	pit->kvm = kvm;
@@ -684,55 +745,11 @@ void kvm_free_pit(struct kvm *kvm)
 				&kvm->arch.vpit->pit_state.irq_ack_notifier);
 		mutex_lock(&kvm->arch.vpit->pit_state.lock);
 		timer = &kvm->arch.vpit->pit_state.pit_timer.timer;
-		hrtimer_cancel(timer);
+		if (hrtimer_cancel(timer))
+			cancel_work_sync(&kvm->arch.vpit->expired);
 		kvm_free_irq_source_id(kvm, kvm->arch.vpit->irq_source_id);
 		mutex_unlock(&kvm->arch.vpit->pit_state.lock);
+		destroy_workqueue(kvm->arch.vpit->wq);
 		kfree(kvm->arch.vpit);
 	}
 }
-
-static void __inject_pit_timer_intr(struct kvm *kvm)
-{
-	struct kvm_vcpu *vcpu;
-	int i;
-
-	kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1);
-	kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 0);
-
-	/*
-	 * Provides NMI watchdog support via Virtual Wire mode.
-	 * The route is: PIT -> PIC -> LVT0 in NMI mode.
-	 *
-	 * Note: Our Virtual Wire implementation is simplified, only
-	 * propagating PIT interrupts to all VCPUs when they have set
-	 * LVT0 to NMI delivery. Other PIC interrupts are just sent to
-	 * VCPU0, and only if its LVT0 is in EXTINT mode.
-	 */
-	if (kvm->arch.vapics_in_nmi_mode > 0)
-		kvm_for_each_vcpu(i, vcpu, kvm)
-			kvm_apic_nmi_wd_deliver(vcpu);
-}
-
-void kvm_inject_pit_timer_irqs(struct kvm_vcpu *vcpu)
-{
-	struct kvm_pit *pit = vcpu->kvm->arch.vpit;
-	struct kvm *kvm = vcpu->kvm;
-	struct kvm_kpit_state *ps;
-
-	if (pit) {
-		int inject = 0;
-		ps = &pit->pit_state;
-
-		/* Try to inject pending interrupts when
-		 * last one has been acked.
-		 */
-		raw_spin_lock(&ps->inject_lock);
-		if (atomic_read(&ps->pit_timer.pending) && ps->irq_ack) {
-			ps->irq_ack = 0;
-			inject = 1;
-		}
-		raw_spin_unlock(&ps->inject_lock);
-		if (inject)
-			__inject_pit_timer_intr(kvm);
-	}
-}
diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h
index 900d6b0..46d08ca 100644
--- a/arch/x86/kvm/i8254.h
+++ b/arch/x86/kvm/i8254.h
@@ -27,7 +27,7 @@ struct kvm_kpit_state {
 	u32    speaker_data_on;
 	struct mutex lock;
 	struct kvm_pit *pit;
-	raw_spinlock_t inject_lock;
+	spinlock_t inject_lock;
 	unsigned long irq_ack;
 	struct kvm_irq_ack_notifier irq_ack_notifier;
 };
@@ -40,6 +40,8 @@ struct kvm_pit {
 	struct kvm_kpit_state pit_state;
 	int irq_source_id;
 	struct kvm_irq_mask_notifier mask_notifier;
+	struct workqueue_struct *wq;
+	struct work_struct expired;
 };
 
 #define KVM_PIT_BASE_ADDRESS	    0x40
diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
index 0f4e488..2095a04 100644
--- a/arch/x86/kvm/irq.c
+++ b/arch/x86/kvm/irq.c
@@ -90,7 +90,6 @@ EXPORT_SYMBOL_GPL(kvm_cpu_get_interrupt);
 void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu)
 {
 	kvm_inject_apic_timer_irqs(vcpu);
-	kvm_inject_pit_timer_irqs(vcpu);
 	/* TODO: PIT, RTC etc. */
 }
 EXPORT_SYMBOL_GPL(kvm_inject_pending_timer_irqs);
-- 
1.6.6.1

[PATCH v2 2/3] Allow any LAPIC to accept PIC interrupts.

[Chris Lalancette]

If the guest wants to accept timer interrupts on a CPU other
than the BSP, we need to remove this gate.

Signed-off-by: Chris Lalancette <clalance@redhat.com>
---
 arch/x86/kvm/lapic.c |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index d8258a0..ee0f76c 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1107,13 +1107,11 @@ int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)
 	u32 lvt0 = apic_get_reg(vcpu->arch.apic, APIC_LVT0);
 	int r = 0;
 
-	if (kvm_vcpu_is_bsp(vcpu)) {
-		if (!apic_hw_enabled(vcpu->arch.apic))
-			r = 1;
-		if ((lvt0 & APIC_LVT_MASKED) == 0 &&
-		    GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT)
-			r = 1;
-	}
+	if (!apic_hw_enabled(vcpu->arch.apic))
+		r = 1;
+	if ((lvt0 & APIC_LVT_MASKED) == 0 &&
+	    GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT)
+		r = 1;
 	return r;
 }
 
-- 
1.6.6.1

[PATCH v2 3/3] In DM_LOWEST, only deliver interrupts to vcpus with enabled LAPIC's

[Chris Lalancette]

Otherwise we might try to deliver a timer interrupt to a cpu that
can't possibly handle it.

Signed-off-by: Chris Lalancette <clalance@redhat.com>
---
 virt/kvm/irq_comm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index 52f412f..06cf61e 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -100,7 +100,7 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 			if (r < 0)
 				r = 0;
 			r += kvm_apic_set_irq(vcpu, irq);
-		} else {
+		} else if (kvm_lapic_enabled(vcpu)) {
 			if (!lowest)
 				lowest = vcpu;
 			else if (kvm_apic_compare_prio(vcpu, lowest) < 0)
-- 
1.6.6.1

Re: [PATCH v2 1/3] Introduce a workqueue to deliver PIT timer interrupts.

[Marcelo Tosatti]

On Mon, Jun 14, 2010 at 01:11:20PM -0400, Chris Lalancette wrote:
> We really want to "kvm_set_irq" during the hrtimer callback,
> but that is risky because that is during interrupt context.
> Instead, offload the work to a workqueue, which is a bit safer
> and should provide most of the same functionality.
> 
> Signed-off-by: Chris Lalancette <clalance@redhat.com>
> ---
>  arch/x86/kvm/i8254.c |  125 ++++++++++++++++++++++++++++---------------------
>  arch/x86/kvm/i8254.h |    4 +-
>  arch/x86/kvm/irq.c   |    1 -
>  3 files changed, 74 insertions(+), 56 deletions(-)
> 
> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> index 188d827..3bed8ac 100644
> --- a/arch/x86/kvm/i8254.c
> +++ b/arch/x86/kvm/i8254.c
> @@ -34,6 +34,7 @@
>  
>  #include <linux/kvm_host.h>
>  #include <linux/slab.h>
> +#include <linux/workqueue.h>
>  
>  #include "irq.h"
>  #include "i8254.h"
> @@ -244,11 +245,11 @@ static void kvm_pit_ack_irq(struct kvm_irq_ack_notifier *kian)
>  {
>  	struct kvm_kpit_state *ps = container_of(kian, struct kvm_kpit_state,
>  						 irq_ack_notifier);
> -	raw_spin_lock(&ps->inject_lock);
> +	spin_lock(&ps->inject_lock);
>  	if (atomic_dec_return(&ps->pit_timer.pending) < 0)
>  		atomic_inc(&ps->pit_timer.pending);
>  	ps->irq_ack = 1;
> -	raw_spin_unlock(&ps->inject_lock);
> +	spin_unlock(&ps->inject_lock);
>  }
>  
>  void __kvm_migrate_pit_timer(struct kvm_vcpu *vcpu)
> @@ -267,7 +268,8 @@ void __kvm_migrate_pit_timer(struct kvm_vcpu *vcpu)
>  static void destroy_pit_timer(struct kvm_timer *pt)
>  {
>  	pr_debug("execute del timer!\n");
> -	hrtimer_cancel(&pt->timer);
> +	if (hrtimer_cancel(&pt->timer))
> +		cancel_work_sync(&pt->kvm->arch.vpit->expired);
>  }
>  
>  static bool kpit_is_periodic(struct kvm_timer *ktimer)
> @@ -281,6 +283,58 @@ static struct kvm_timer_ops kpit_ops = {
>  	.is_periodic = kpit_is_periodic,
>  };
>  
> +static void pit_do_work(struct work_struct *work)
> +{
> +	struct kvm_pit *pit = container_of(work, struct kvm_pit, expired);
> +	struct kvm *kvm = pit->kvm;
> +	struct kvm_vcpu *vcpu;
> +	int i;
> +	struct kvm_kpit_state *ps = &pit->pit_state;
> +	int inject = 0;
> +
> +	/* Try to inject pending interrupts when
> +	 * last one has been acked.
> +	 */
> +	spin_lock(&ps->inject_lock);
> +	if (ps->irq_ack) {
> +		ps->irq_ack = 0;
> +		inject = 1;
> +	}
> +	spin_unlock(&ps->inject_lock);
> +	if (inject) {
> +		kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1);
> +		kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 0);
> +
> +		/*
> +		 * Provides NMI watchdog support via Virtual Wire mode.
> +		 * The route is: PIT -> PIC -> LVT0 in NMI mode.
> +		 *
> +		 * Note: Our Virtual Wire implementation is simplified, only
> +		 * propagating PIT interrupts to all VCPUs when they have set
> +		 * LVT0 to NMI delivery. Other PIC interrupts are just sent to
> +		 * VCPU0, and only if its LVT0 is in EXTINT mode.
> +		 */
> +		if (kvm->arch.vapics_in_nmi_mode > 0)
> +			kvm_for_each_vcpu(i, vcpu, kvm)
> +				kvm_apic_nmi_wd_deliver(vcpu);
> +	}
> +}
> +
> +static enum hrtimer_restart pit_timer_fn(struct hrtimer *data)
> +{
> +	struct kvm_timer *ktimer = container_of(data, struct kvm_timer, timer);
> +	struct kvm_pit *pt = ktimer->kvm->arch.vpit;
> +
> +	if (ktimer->reinject)
> +		queue_work(pt->wq, &pt->expired);

The "problem" is queue_work only queues the work if it was not already
queued. So multiple queue_work() calls can collapse into one executed
job.

You need to maintain a counter here in pit_timer_fn, and reinject at 
some point (perhaps on ACK) if there are multiple interrupts pending.

> +
> +	if (ktimer->t_ops->is_periodic(ktimer)) {
> +		hrtimer_add_expires_ns(&ktimer->timer, ktimer->period);
> +		return HRTIMER_RESTART;
> +	} else
> +		return HRTIMER_NORESTART;
> +}
> +
>  static void create_pit_timer(struct kvm_kpit_state *ps, u32 val, int is_period)
>  {
>  	struct kvm_timer *pt = &ps->pit_timer;
> @@ -291,14 +345,14 @@ static void create_pit_timer(struct kvm_kpit_state *ps, u32 val, int is_period)
>  	pr_debug("create pit timer, interval is %llu nsec\n", interval);
>  
>  	/* TODO The new value only affected after the retriggered */
> -	hrtimer_cancel(&pt->timer);
> +	if (hrtimer_cancel(&pt->timer))
> +		cancel_work_sync(&pt->kvm->arch.vpit->expired);

There can be a queued work instance even if the hrtimer is not active,
so cancel_work_sync should be unconditional.

Re: [PATCH v2 1/3] Introduce a workqueue to deliver PIT timer interrupts.

[Gleb Natapov]

On Mon, Jun 14, 2010 at 01:11:20PM -0400, Chris Lalancette wrote:
> We really want to "kvm_set_irq" during the hrtimer callback,
> but that is risky because that is during interrupt context.
> Instead, offload the work to a workqueue, which is a bit safer
> and should provide most of the same functionality.
> 
> Signed-off-by: Chris Lalancette <clalance@redhat.com>
> ---
>  arch/x86/kvm/i8254.c |  125 ++++++++++++++++++++++++++++---------------------
>  arch/x86/kvm/i8254.h |    4 +-
>  arch/x86/kvm/irq.c   |    1 -
>  3 files changed, 74 insertions(+), 56 deletions(-)
> 
> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> index 188d827..3bed8ac 100644
> --- a/arch/x86/kvm/i8254.c
> +++ b/arch/x86/kvm/i8254.c
> @@ -34,6 +34,7 @@
>  
>  #include <linux/kvm_host.h>
>  #include <linux/slab.h>
> +#include <linux/workqueue.h>
>  
>  #include "irq.h"
>  #include "i8254.h"
> @@ -244,11 +245,11 @@ static void kvm_pit_ack_irq(struct kvm_irq_ack_notifier *kian)
>  {
>  	struct kvm_kpit_state *ps = container_of(kian, struct kvm_kpit_state,
>  						 irq_ack_notifier);
> -	raw_spin_lock(&ps->inject_lock);
> +	spin_lock(&ps->inject_lock);
>  	if (atomic_dec_return(&ps->pit_timer.pending) < 0)
>  		atomic_inc(&ps->pit_timer.pending);
>  	ps->irq_ack = 1;
> -	raw_spin_unlock(&ps->inject_lock);
> +	spin_unlock(&ps->inject_lock);
>  }
>  
>  void __kvm_migrate_pit_timer(struct kvm_vcpu *vcpu)
> @@ -267,7 +268,8 @@ void __kvm_migrate_pit_timer(struct kvm_vcpu *vcpu)
>  static void destroy_pit_timer(struct kvm_timer *pt)
>  {
>  	pr_debug("execute del timer!\n");
> -	hrtimer_cancel(&pt->timer);
> +	if (hrtimer_cancel(&pt->timer))
> +		cancel_work_sync(&pt->kvm->arch.vpit->expired);
>  }
>  
>  static bool kpit_is_periodic(struct kvm_timer *ktimer)
> @@ -281,6 +283,58 @@ static struct kvm_timer_ops kpit_ops = {
>  	.is_periodic = kpit_is_periodic,
>  };
>  
> +static void pit_do_work(struct work_struct *work)
> +{
> +	struct kvm_pit *pit = container_of(work, struct kvm_pit, expired);
> +	struct kvm *kvm = pit->kvm;
> +	struct kvm_vcpu *vcpu;
> +	int i;
> +	struct kvm_kpit_state *ps = &pit->pit_state;
> +	int inject = 0;
> +
> +	/* Try to inject pending interrupts when
> +	 * last one has been acked.
> +	 */
> +	spin_lock(&ps->inject_lock);
> +	if (ps->irq_ack) {
> +		ps->irq_ack = 0;
> +		inject = 1;
> +	}
> +	spin_unlock(&ps->inject_lock);
> +	if (inject) {
> +		kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1);
> +		kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 0);
> +
> +		/*
> +		 * Provides NMI watchdog support via Virtual Wire mode.
> +		 * The route is: PIT -> PIC -> LVT0 in NMI mode.
> +		 *
> +		 * Note: Our Virtual Wire implementation is simplified, only
> +		 * propagating PIT interrupts to all VCPUs when they have set
> +		 * LVT0 to NMI delivery. Other PIC interrupts are just sent to
> +		 * VCPU0, and only if its LVT0 is in EXTINT mode.
> +		 */
> +		if (kvm->arch.vapics_in_nmi_mode > 0)
> +			kvm_for_each_vcpu(i, vcpu, kvm)
> +				kvm_apic_nmi_wd_deliver(vcpu);
> +	}
> +}
> +
> +static enum hrtimer_restart pit_timer_fn(struct hrtimer *data)
> +{
> +	struct kvm_timer *ktimer = container_of(data, struct kvm_timer, timer);
> +	struct kvm_pit *pt = ktimer->kvm->arch.vpit;
> +
> +	if (ktimer->reinject)
> +		queue_work(pt->wq, &pt->expired);
If ktimer->reinject is set to false by userspace pit irq will never be
delivered or do I missing something here?

May be we should consider using return value from kvm_set_irq() for
coalescing detection to simplify things. I once had patch for that.

> +
> +	if (ktimer->t_ops->is_periodic(ktimer)) {
> +		hrtimer_add_expires_ns(&ktimer->timer, ktimer->period);
> +		return HRTIMER_RESTART;
> +	} else
> +		return HRTIMER_NORESTART;
> +}
> +
>  static void create_pit_timer(struct kvm_kpit_state *ps, u32 val, int is_period)
>  {
>  	struct kvm_timer *pt = &ps->pit_timer;
> @@ -291,14 +345,14 @@ static void create_pit_timer(struct kvm_kpit_state *ps, u32 val, int is_period)
>  	pr_debug("create pit timer, interval is %llu nsec\n", interval);
>  
>  	/* TODO The new value only affected after the retriggered */
> -	hrtimer_cancel(&pt->timer);
> +	if (hrtimer_cancel(&pt->timer))
> +		cancel_work_sync(&pt->kvm->arch.vpit->expired);
>  	pt->period = interval;
>  	ps->is_periodic = is_period;
>  
> -	pt->timer.function = kvm_timer_fn;
> +	pt->timer.function = pit_timer_fn;
>  	pt->t_ops = &kpit_ops;
>  	pt->kvm = ps->pit->kvm;
> -	pt->vcpu = pt->kvm->bsp_vcpu;
>  
>  	atomic_set(&pt->pending, 0);
>  	ps->irq_ack = 1;
> @@ -626,7 +680,14 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
>  
>  	mutex_init(&pit->pit_state.lock);
>  	mutex_lock(&pit->pit_state.lock);
> -	raw_spin_lock_init(&pit->pit_state.inject_lock);
> +	spin_lock_init(&pit->pit_state.inject_lock);
> +
> +	pit->wq = create_singlethread_workqueue("kvm-pit-wq");
> +	if (!pit->wq) {
> +		kfree(pit);
> +		return NULL;
> +	}
> +	INIT_WORK(&pit->expired, pit_do_work);
>  
>  	kvm->arch.vpit = pit;
>  	pit->kvm = kvm;
> @@ -684,55 +745,11 @@ void kvm_free_pit(struct kvm *kvm)
>  				&kvm->arch.vpit->pit_state.irq_ack_notifier);
>  		mutex_lock(&kvm->arch.vpit->pit_state.lock);
>  		timer = &kvm->arch.vpit->pit_state.pit_timer.timer;
> -		hrtimer_cancel(timer);
> +		if (hrtimer_cancel(timer))
> +			cancel_work_sync(&kvm->arch.vpit->expired);
>  		kvm_free_irq_source_id(kvm, kvm->arch.vpit->irq_source_id);
>  		mutex_unlock(&kvm->arch.vpit->pit_state.lock);
> +		destroy_workqueue(kvm->arch.vpit->wq);
>  		kfree(kvm->arch.vpit);
>  	}
>  }
> -
> -static void __inject_pit_timer_intr(struct kvm *kvm)
> -{
> -	struct kvm_vcpu *vcpu;
> -	int i;
> -
> -	kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1);
> -	kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 0);
> -
> -	/*
> -	 * Provides NMI watchdog support via Virtual Wire mode.
> -	 * The route is: PIT -> PIC -> LVT0 in NMI mode.
> -	 *
> -	 * Note: Our Virtual Wire implementation is simplified, only
> -	 * propagating PIT interrupts to all VCPUs when they have set
> -	 * LVT0 to NMI delivery. Other PIC interrupts are just sent to
> -	 * VCPU0, and only if its LVT0 is in EXTINT mode.
> -	 */
> -	if (kvm->arch.vapics_in_nmi_mode > 0)
> -		kvm_for_each_vcpu(i, vcpu, kvm)
> -			kvm_apic_nmi_wd_deliver(vcpu);
> -}
> -
> -void kvm_inject_pit_timer_irqs(struct kvm_vcpu *vcpu)
> -{
> -	struct kvm_pit *pit = vcpu->kvm->arch.vpit;
> -	struct kvm *kvm = vcpu->kvm;
> -	struct kvm_kpit_state *ps;
> -
> -	if (pit) {
> -		int inject = 0;
> -		ps = &pit->pit_state;
> -
> -		/* Try to inject pending interrupts when
> -		 * last one has been acked.
> -		 */
> -		raw_spin_lock(&ps->inject_lock);
> -		if (atomic_read(&ps->pit_timer.pending) && ps->irq_ack) {
> -			ps->irq_ack = 0;
> -			inject = 1;
> -		}
> -		raw_spin_unlock(&ps->inject_lock);
> -		if (inject)
> -			__inject_pit_timer_intr(kvm);
> -	}
> -}
> diff --git a/arch/x86/kvm/i8254.h b/arch/x86/kvm/i8254.h
> index 900d6b0..46d08ca 100644
> --- a/arch/x86/kvm/i8254.h
> +++ b/arch/x86/kvm/i8254.h
> @@ -27,7 +27,7 @@ struct kvm_kpit_state {
>  	u32    speaker_data_on;
>  	struct mutex lock;
>  	struct kvm_pit *pit;
> -	raw_spinlock_t inject_lock;
> +	spinlock_t inject_lock;
>  	unsigned long irq_ack;
>  	struct kvm_irq_ack_notifier irq_ack_notifier;
>  };
> @@ -40,6 +40,8 @@ struct kvm_pit {
>  	struct kvm_kpit_state pit_state;
>  	int irq_source_id;
>  	struct kvm_irq_mask_notifier mask_notifier;
> +	struct workqueue_struct *wq;
> +	struct work_struct expired;
>  };
>  
>  #define KVM_PIT_BASE_ADDRESS	    0x40
> diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
> index 0f4e488..2095a04 100644
> --- a/arch/x86/kvm/irq.c
> +++ b/arch/x86/kvm/irq.c
> @@ -90,7 +90,6 @@ EXPORT_SYMBOL_GPL(kvm_cpu_get_interrupt);
>  void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu)
>  {
>  	kvm_inject_apic_timer_irqs(vcpu);
> -	kvm_inject_pit_timer_irqs(vcpu);
>  	/* TODO: PIT, RTC etc. */
>  }
>  EXPORT_SYMBOL_GPL(kvm_inject_pending_timer_irqs);
> -- 
> 1.6.6.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

Re: [PATCH v2 1/3] Introduce a workqueue to deliver PIT timer interrupts.

[Marcelo Tosatti]

On Tue, Jun 15, 2010 at 08:11:45AM -0400, Chris Lalancette wrote:
> On 06/14/10 - 07:19:49PM, Marcelo Tosatti wrote:
> > On Mon, Jun 14, 2010 at 01:11:20PM -0400, Chris Lalancette wrote:
> > > We really want to "kvm_set_irq" during the hrtimer callback,
> > > but that is risky because that is during interrupt context.
> > > Instead, offload the work to a workqueue, which is a bit safer
> > > and should provide most of the same functionality.
> > > 
> > > Signed-off-by: Chris Lalancette <clalance@redhat.com>
> > > ---
> > >  arch/x86/kvm/i8254.c |  125 ++++++++++++++++++++++++++++---------------------
> > >  arch/x86/kvm/i8254.h |    4 +-
> > >  arch/x86/kvm/irq.c   |    1 -
> > >  3 files changed, 74 insertions(+), 56 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> > > index 188d827..3bed8ac 100644
> > > --- a/arch/x86/kvm/i8254.c
> > > +++ b/arch/x86/kvm/i8254.c
> > > @@ -34,6 +34,7 @@
> > >  
> > >  #include <linux/kvm_host.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/workqueue.h>
> > >  
> > >  #include "irq.h"
> > >  #include "i8254.h"
> > > @@ -244,11 +245,11 @@ static void kvm_pit_ack_irq(struct kvm_irq_ack_notifier *kian)
> > >  {
> > >  	struct kvm_kpit_state *ps = container_of(kian, struct kvm_kpit_state,
> > >  						 irq_ack_notifier);
> > > -	raw_spin_lock(&ps->inject_lock);
> > > +	spin_lock(&ps->inject_lock);
> > >  	if (atomic_dec_return(&ps->pit_timer.pending) < 0)
> > >  		atomic_inc(&ps->pit_timer.pending);
> > >  	ps->irq_ack = 1;
> > > -	raw_spin_unlock(&ps->inject_lock);
> > > +	spin_unlock(&ps->inject_lock);
> > >  }
> > >  
> > >  void __kvm_migrate_pit_timer(struct kvm_vcpu *vcpu)
> > > @@ -267,7 +268,8 @@ void __kvm_migrate_pit_timer(struct kvm_vcpu *vcpu)
> > >  static void destroy_pit_timer(struct kvm_timer *pt)
> > >  {
> > >  	pr_debug("execute del timer!\n");
> > > -	hrtimer_cancel(&pt->timer);
> > > +	if (hrtimer_cancel(&pt->timer))
> > > +		cancel_work_sync(&pt->kvm->arch.vpit->expired);
> > >  }
> > >  
> > >  static bool kpit_is_periodic(struct kvm_timer *ktimer)
> > > @@ -281,6 +283,58 @@ static struct kvm_timer_ops kpit_ops = {
> > >  	.is_periodic = kpit_is_periodic,
> > >  };
> > >  
> > > +static void pit_do_work(struct work_struct *work)
> > > +{
> > > +	struct kvm_pit *pit = container_of(work, struct kvm_pit, expired);
> > > +	struct kvm *kvm = pit->kvm;
> > > +	struct kvm_vcpu *vcpu;
> > > +	int i;
> > > +	struct kvm_kpit_state *ps = &pit->pit_state;
> > > +	int inject = 0;
> > > +
> > > +	/* Try to inject pending interrupts when
> > > +	 * last one has been acked.
> > > +	 */
> > > +	spin_lock(&ps->inject_lock);
> > > +	if (ps->irq_ack) {
> > > +		ps->irq_ack = 0;
> > > +		inject = 1;
> > > +	}
> > > +	spin_unlock(&ps->inject_lock);
> > > +	if (inject) {
> > > +		kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1);
> > > +		kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 0);
> > > +
> > > +		/*
> > > +		 * Provides NMI watchdog support via Virtual Wire mode.
> > > +		 * The route is: PIT -> PIC -> LVT0 in NMI mode.
> > > +		 *
> > > +		 * Note: Our Virtual Wire implementation is simplified, only
> > > +		 * propagating PIT interrupts to all VCPUs when they have set
> > > +		 * LVT0 to NMI delivery. Other PIC interrupts are just sent to
> > > +		 * VCPU0, and only if its LVT0 is in EXTINT mode.
> > > +		 */
> > > +		if (kvm->arch.vapics_in_nmi_mode > 0)
> > > +			kvm_for_each_vcpu(i, vcpu, kvm)
> > > +				kvm_apic_nmi_wd_deliver(vcpu);
> > > +	}
> > > +}
> > > +
> > > +static enum hrtimer_restart pit_timer_fn(struct hrtimer *data)
> > > +{
> > > +	struct kvm_timer *ktimer = container_of(data, struct kvm_timer, timer);
> > > +	struct kvm_pit *pt = ktimer->kvm->arch.vpit;
> > > +
> > > +	if (ktimer->reinject)
> > > +		queue_work(pt->wq, &pt->expired);
> > 
> > The "problem" is queue_work only queues the work if it was not already
> > queued. So multiple queue_work() calls can collapse into one executed
> > job.
> > 
> > You need to maintain a counter here in pit_timer_fn, and reinject at 
> > some point (perhaps on ACK) if there are multiple interrupts pending.
> 
> Ah, OK, so that's what the "pending" variable was all about.  I didn't quite
> understand that before.  I'll make this change.
> 
> Is there any way in particular I can test this change?  Just fire up a RHEL-3
> guest and see if time drifts?  Something more targetted?

Firing up a RHEL-3 guest should be enough (one without lost interrupt
compensation).

> > 
> > > +
> > > +	if (ktimer->t_ops->is_periodic(ktimer)) {
> > > +		hrtimer_add_expires_ns(&ktimer->timer, ktimer->period);
> > > +		return HRTIMER_RESTART;
> > > +	} else
> > > +		return HRTIMER_NORESTART;
> > > +}
> > > +
> > >  static void create_pit_timer(struct kvm_kpit_state *ps, u32 val, int is_period)
> > >  {
> > >  	struct kvm_timer *pt = &ps->pit_timer;
> > > @@ -291,14 +345,14 @@ static void create_pit_timer(struct kvm_kpit_state *ps, u32 val, int is_period)
> > >  	pr_debug("create pit timer, interval is %llu nsec\n", interval);
> > >  
> > >  	/* TODO The new value only affected after the retriggered */
> > > -	hrtimer_cancel(&pt->timer);
> > > +	if (hrtimer_cancel(&pt->timer))
> > > +		cancel_work_sync(&pt->kvm->arch.vpit->expired);
> > 
> > There can be a queued work instance even if the hrtimer is not active,
> > so cancel_work_sync should be unconditional.
> 
> Yeah, that's what I had initially.  However, I ran into a problem with this;
> if you call "cancel_work_sync" when there is *no* work queued up, then you get
> a stack trace like:
> 
> BUG: unable to handle kernel paging request at 0000000000002368
> IP: [<ffffffffa077e2e5>] pit_load_count+0x95/0x190 [kvm]
> <snip>
> Call Trace:
>  [<ffffffffa077e712>] kvm_pit_reset+0x62/0xa0 [kvm]
>  [<ffffffffa077eb42>] kvm_create_pit+0x162/0x290 [kvm]
>  [<ffffffffa0765273>] kvm_arch_vm_ioctl+0x5d3/0xd80 [kvm]
> 
> Since I have to use "pending" for the interrupt reinjection logic, I can
> work around both of these issues by keying off of that for whether to do
> a cancel_work_sync.  Unless you have a better idea for how to work around
> this issue?

It should be fine to call cancel_work_sync on a workqueue with no work
pending. It seems the error is somewhere else.