[PATCH 0/3]: Fixes to IRQ routing

[PATCH 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.

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

Chris Lalancette

[PATCH 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 |  117 ++++++++++++++++++++++++++++----------------------
 arch/x86/kvm/i8254.h |    4 +-
 arch/x86/kvm/irq.c   |    1 -
 3 files changed, 69 insertions(+), 53 deletions(-)

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index 188d827..99c7472 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)
@@ -281,6 +282,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;
+
+	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;
@@ -295,10 +348,9 @@ static void create_pit_timer(struct kvm_kpit_state *ps, u32 val, int is_period)
 	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 +678,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;
@@ -687,52 +746,8 @@ void kvm_free_pit(struct kvm *kvm)
 		hrtimer_cancel(timer);
 		kvm_free_irq_source_id(kvm, kvm->arch.vpit->irq_source_id);
 		mutex_unlock(&kvm->arch.vpit->pit_state.lock);
+		cancel_work_sync(&kvm->arch.vpit->expired);
+		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.5.2

[PATCH 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.5.2

[PATCH 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.5.2

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

[Marcelo Tosatti]

On Thu, Jun 10, 2010 at 04:44:05PM -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 |  117 ++++++++++++++++++++++++++++----------------------
>  arch/x86/kvm/i8254.h |    4 +-
>  arch/x86/kvm/irq.c   |    1 -
>  3 files changed, 69 insertions(+), 53 deletions(-)
> 
> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> index 188d827..99c7472 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)
> @@ -281,6 +282,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;
> +
> +	queue_work(pt->wq, &pt->expired);

So this disables interrupt reinjection. Older RHEL3 guests do not
compensate for lost ticks, and as such are likely to drift without 
it (but RHEL3 is EOL, should one care?).

Are there other guests which rely on PIT reinjection, or is it OK 
to remove it completly?

> +
> +	if (ktimer->t_ops->is_periodic(ktimer)) {
> +		hrtimer_add_expires_ns(&ktimer->timer, ktimer->period);
> +		return HRTIMER_RESTART;
> +	}
> +	else
> +		return HRTIMER_NORESTART;
> +}

Also need to cancel the pending work whenever the current code cancels
the hrtimer (destroy_pit_timer, etc).

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

[Yang, Sheng]

On Saturday 12 June 2010 01:35:23 Marcelo Tosatti wrote:
> On Thu, Jun 10, 2010 at 04:44:05PM -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 |  117
> >  ++++++++++++++++++++++++++++---------------------- arch/x86/kvm/i8254.h
> >  |    4 +-
> >  arch/x86/kvm/irq.c   |    1 -
> >  3 files changed, 69 insertions(+), 53 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> > index 188d827..99c7472 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)
> > 
> > @@ -281,6 +282,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;
> > +
> > +	queue_work(pt->wq, &pt->expired);
> 
> So this disables interrupt reinjection. Older RHEL3 guests do not
> compensate for lost ticks, and as such are likely to drift without
> it (but RHEL3 is EOL, should one care?).
> 
> Are there other guests which rely on PIT reinjection, or is it OK
> to remove it completly?

IIRC, the old kernel *does* compensate ticks, so we need disable reinjection. And 
the latest kernel doesn't do this, so we have to do reinjection.

So we can't disable reinjection anyway.

BTW: The patch has some coding style issues, suggest using scripts/checkpatch.pl 
to check it.

--
regards
Yang, Sheng

> 
> > +
> > +	if (ktimer->t_ops->is_periodic(ktimer)) {
> > +		hrtimer_add_expires_ns(&ktimer->timer, ktimer->period);
> > +		return HRTIMER_RESTART;
> > +	}
> > +	else
> > +		return HRTIMER_NORESTART;
> > +}
> 
> Also need to cancel the pending work whenever the current code cancels
> the hrtimer (destroy_pit_timer, etc).

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

[Zachary Amsden]

On 06/11/2010 06:15 PM, Yang, Sheng wrote:
> On Saturday 12 June 2010 01:35:23 Marcelo Tosatti wrote:
>    
>> On Thu, Jun 10, 2010 at 04:44:05PM -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 |  117
>>>   ++++++++++++++++++++++++++++---------------------- arch/x86/kvm/i8254.h
>>>   |    4 +-
>>>   arch/x86/kvm/irq.c   |    1 -
>>>   3 files changed, 69 insertions(+), 53 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
>>> index 188d827..99c7472 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)
>>>
>>> @@ -281,6 +282,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;
>>> +
>>> +	queue_work(pt->wq,&pt->expired);
>>>        
>> So this disables interrupt reinjection. Older RHEL3 guests do not
>> compensate for lost ticks, and as such are likely to drift without
>> it (but RHEL3 is EOL, should one care?).
>>
>> Are there other guests which rely on PIT reinjection, or is it OK
>> to remove it completly?
>>      
> IIRC, the old kernel *does* compensate ticks, so we need disable reinjection. And
> the latest kernel doesn't do this, so we have to do reinjection.
>    

It looks like 2.4.21 has lost tick compensation for x86_64, but not for 
i386, so the reinjection is still needed.

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

[Gleb Natapov]

On Thu, Jun 10, 2010 at 04:44:04PM -0400, Chris Lalancette wrote:
> 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.
> 
> 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
> 
> Chris Lalancette
> 
Looks good to me.

--
			Gleb.