[PATCH 0/12]: Fix kdump under KVM

[PATCH 0/12]: Fix kdump under KVM

[Chris Lalancette]

Another version of the patch series to make kdump work inside KVM
guests.  The current problem with using kdump is that KVM only
delivers PIT interrupts to the BSP.  While this is technically
allowed by the MPS spec, most motherboards deliver timer interrupts
to *any* LAPIC in virtual wire mode.  Since a crash can occur on
any CPU, timer interrupts must be able to reach any CPU in order
for kdump to work properly.  Therefore, this series allows timer
interrupts to be routed to any virtual CPU that is in the correct mode.

I've broken up the series into much smaller chunks now; some of them
can be applied on their own as bug fixes.  There are a few changes
to this series from the last time I posted.  They are:

1)  I've taken Marcelo's advice to only redirect PIT interrupts in
the IOAPIC to the BSP if the destination is multiple vcpus.
2)  I've changed it so that we use a spin_lock() in the IOAPIC instead
of a mutex lock, based on the discussion with Avi.  This means that
we can get rid of all of the "delayed" interrupt action in the PIT
and just directly "set_irq" in the hrtimer callbacks.

The conversion of the IOAPIC to a spin_lock instead of a mutex_lock
caused some concern with Avi when we last discussed it.  In particular
he was concerned that any notifiers that kvm_set_irq kicked off were
safe from an IRQ context.  From what I can tell, there are two sets
of notifiers: the IRQ mask notifiers and the IRQ ACK notifiers.  There
is one, and only one mask notifier, in the PIT; it merely sets a flag,
so it should be interrupt safe.  For the IRQ ACK notifiers, there is
one notifier for the PIT and one for assigned-dev.c.  The one in the
PIT is definitely interrupt safe, it just sets a flag.  The one
in assigned-dev is more complicated.  It calls out to kvm_set_irq()
(which is IRQ-safe now), and also calls out to enable_irq(), which
should be interrupt-safe as long as bus_lock is not set
(which it never is, from what I can tell).  Therefore, I *think* this
should be safe, although I don't have VT-d hardware available at the
moment to test for sure.

I did basic smoke testing on a few different SMP guests: Fedora 11 i386,
Fedora 12 x86_64, Red Hat Linux 5.2, Windows XP i386, RHEL-4.8, 
Ubuntu 8.10, Windows 2003 i386, and RHEL-5.3.  I also did targetted
testing of the kdump feature in the RHEL-5.3 guest, and kdump works there.

[PATCH 01/12] Fix up some comments around the source tree.

[Chris Lalancette]

Signed-off-by: Chris Lalancette <clalance@redhat.com>
---
 arch/x86/kvm/svm.c  |    2 +-
 virt/kvm/ioapic.c   |    4 ++--
 virt/kvm/kvm_main.c |    2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 3de0b37..68fe7f2 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2628,7 +2628,7 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
 		break;
 	case SVM_EXITINTINFO_TYPE_EXEPT:
 		/* In case of software exception do not reinject an exception
-		   vector, but re-execute and instruction instead */
+		   vector, but re-execute an instruction instead */
 		if (is_nested(svm))
 			break;
 		if (kvm_exception_is_soft(vector))
diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 38a2d20..53f3166 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -164,10 +164,10 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
 	irqe.shorthand = 0;
 
 #ifdef CONFIG_X86
-	/* Always delivery PIT interrupt to vcpu 0 */
+	/* Always deliver PIT interrupt to vcpu 0 */
 	if (irq == 0) {
 		irqe.dest_mode = 0; /* Physical mode. */
-		/* need to read apic_id from apic regiest since
+		/* need to read apic_id from apic register since
 		 * it can be rewritten */
 		irqe.dest_id = ioapic->kvm->bsp_vcpu->vcpu_id;
 	}
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index f92ba13..748900e 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1180,7 +1180,7 @@ static int create_vcpu_fd(struct kvm_vcpu *vcpu)
 }
 
 /*
- * Creates some virtual cpus.  Good luck creating more than one.
+ * Creates a virtual cpu.
  */
 static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
 {
-- 
1.6.5.2

[PATCH 02/12] Make kvm_pic_reset static.

[Chris Lalancette]

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

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index d057c0c..59a8a68 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -233,7 +233,7 @@ int kvm_pic_read_irq(struct kvm *kvm)
 	return intno;
 }
 
-void kvm_pic_reset(struct kvm_kpic_state *s)
+static void kvm_pic_reset(struct kvm_kpic_state *s)
 {
 	int irq;
 	struct kvm *kvm = s->pics_state->irq_request_opaque;
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index be399e2..a5344f1 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -93,8 +93,6 @@ static inline int irqchip_in_kernel(struct kvm *kvm)
 	return ret;
 }
 
-void kvm_pic_reset(struct kvm_kpic_state *s);
-
 void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu);
 void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu);
 void kvm_apic_nmi_wd_deliver(struct kvm_vcpu *vcpu);
-- 
1.6.5.2

[PATCH 03/12] Remove KVM_REQ_PENDING_TIMER.

[Chris Lalancette]

KVM_REQ_PENDING_TIMER is set and cleared in a couple of places,
but it never seems to be actually checked.  Remove it.

Signed-off-by: Chris Lalancette <clalance@redhat.com>
---
 arch/x86/kvm/timer.c     |    5 +----
 arch/x86/kvm/x86.c       |    1 -
 include/linux/kvm_host.h |    1 -
 3 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/timer.c b/arch/x86/kvm/timer.c
index eea4043..72b5144 100644
--- a/arch/x86/kvm/timer.c
+++ b/arch/x86/kvm/timer.c
@@ -14,11 +14,8 @@ static int __kvm_timer_fn(struct kvm_vcpu *vcpu, struct kvm_timer *ktimer)
 	 * not care about potentially loosing timer events in the !reinject
 	 * case anyway.
 	 */
-	if (ktimer->reinject || !atomic_read(&ktimer->pending)) {
+	if (ktimer->reinject || !atomic_read(&ktimer->pending))
 		atomic_inc(&ktimer->pending);
-		/* FIXME: this code should not know anything about vcpus */
-		set_bit(KVM_REQ_PENDING_TIMER, &vcpu->requests);
-	}
 
 	if (waitqueue_active(q))
 		wake_up_interruptible(q);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 153a526..2fb4251 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4034,7 +4034,6 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 		if (r <= 0)
 			break;
 
-		clear_bit(KVM_REQ_PENDING_TIMER, &vcpu->requests);
 		if (kvm_cpu_has_pending_timer(vcpu))
 			kvm_inject_pending_timer_irqs(vcpu);
 
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index bd5a616..053e49f 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -33,7 +33,6 @@
 #define KVM_REQ_REPORT_TPR_ACCESS  2
 #define KVM_REQ_MMU_RELOAD         3
 #define KVM_REQ_TRIPLE_FAULT       4
-#define KVM_REQ_PENDING_TIMER      5
 #define KVM_REQ_UNHALT             6
 #define KVM_REQ_MMU_SYNC           7
 #define KVM_REQ_KVMCLOCK_UPDATE    8
-- 
1.6.5.2

Re: [PATCH 03/12] Remove KVM_REQ_PENDING_TIMER.

[Jan Kiszka]

Chris Lalancette wrote:
> KVM_REQ_PENDING_TIMER is set and cleared in a couple of places,
> but it never seems to be actually checked.  Remove it.
> 

I would suggest to study the introducing commit
06e05645661211b9eaadaf6344c335d2e80f0ba2. My strong feeling is that this
removal is wrong.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 03/12] Remove KVM_REQ_PENDING_TIMER.

[Marcelo Tosatti]

On Tue, Dec 01, 2009 at 08:04:15PM +0100, Jan Kiszka wrote:
> Chris Lalancette wrote:
> > KVM_REQ_PENDING_TIMER is set and cleared in a couple of places,
> > but it never seems to be actually checked.  Remove it.
> > 
> 
> I would suggest to study the introducing commit
> 06e05645661211b9eaadaf6344c335d2e80f0ba2. My strong feeling is that this
> removal is wrong.

Right.

Re: [PATCH 03/12] Remove KVM_REQ_PENDING_TIMER.

[Chris Lalancette]

On 12/01/2009 08:04 PM, Jan Kiszka wrote:
> Chris Lalancette wrote:
>> KVM_REQ_PENDING_TIMER is set and cleared in a couple of places,
>> but it never seems to be actually checked.  Remove it.
>>
> 
> I would suggest to study the introducing commit
> 06e05645661211b9eaadaf6344c335d2e80f0ba2. My strong feeling is that this
> removal is wrong.

Ah, thanks.  Now I see what you mean.  In newer KVM, if you are in
__vcpu_run() and are in the while (r > 0) loop, after you've
done kvm_inject_pending_timer_irqs(), but before you do the next
__vcpu_run() (and hence disabled local IRQs), you could receive an
hrtimer callback.  In that case I guess you would hit the 
if (vcpu->requests || need_resched() || signal_pending()) test, don't
inject the interrupt, and possibly spend a long time in the guest
before an unrelated event causes the exit and hence causes the timer
injection.

Well, that certainly is a bit subtle :).  I think I might submit a
patch to at least put a comment in vcpu_enter_guest() about this.

Unfortunately, this does sort of put a damper on what's going on later
in the series.  If the i8254 doesn't belong to the BSP (which it really
shouldn't), then I don't know which vcpu to raise the bit on.  Maybe I
can do it in the kvm_irq_delivery_to_apic() function, or
kvm_apic_set_irq().  I'll take a look at that.

Thanks again for the review,
-- 
Chris Lalancette

Re: [PATCH 03/12] Remove KVM_REQ_PENDING_TIMER.

[Avi Kivity]

On 12/02/2009 02:59 PM, Chris Lalancette wrote:
> On 12/01/2009 08:04 PM, Jan Kiszka wrote:
>    
>> Chris Lalancette wrote:
>>      
>>> KVM_REQ_PENDING_TIMER is set and cleared in a couple of places,
>>> but it never seems to be actually checked.  Remove it.
>>>
>>>        
>> I would suggest to study the introducing commit
>> 06e05645661211b9eaadaf6344c335d2e80f0ba2. My strong feeling is that this
>> removal is wrong.
>>      
> Ah, thanks.  Now I see what you mean.  In newer KVM, if you are in
> __vcpu_run() and are in the while (r>  0) loop, after you've
> done kvm_inject_pending_timer_irqs(), but before you do the next
> __vcpu_run() (and hence disabled local IRQs), you could receive an
> hrtimer callback.  In that case I guess you would hit the
> if (vcpu->requests || need_resched() || signal_pending()) test, don't
> inject the interrupt, and possibly spend a long time in the guest
> before an unrelated event causes the exit and hence causes the timer
> injection.
>
> Well, that certainly is a bit subtle :).  I think I might submit a
> patch to at least put a comment in vcpu_enter_guest() about this.
>
>    

It is, and I don't like it much either.

> Unfortunately, this does sort of put a damper on what's going on later
> in the series.  If the i8254 doesn't belong to the BSP (which it really
> shouldn't), then I don't know which vcpu to raise the bit on.  Maybe I
> can do it in the kvm_irq_delivery_to_apic() function, or
> kvm_apic_set_irq().  I'll take a look at that.
>    

We shouldn't have that bit at all.

An alternative is to make the pic/ioapic work in irq context 
(spin_lock_irq() and friends) and queue the interrupt directly from the 
hrtimer instead of the vcpu.  My worry is that we have quite a bit of 
work to do in irq context if the interrupt is broadcast and if we have 
many vcpus; maybe we can use a work_struct in that case.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 03/12] Remove KVM_REQ_PENDING_TIMER.

[Avi Kivity]

On 12/02/2009 04:16 PM, Avi Kivity wrote:
>
>> Unfortunately, this does sort of put a damper on what's going on later
>> in the series.  If the i8254 doesn't belong to the BSP (which it really
>> shouldn't), then I don't know which vcpu to raise the bit on.  Maybe I
>> can do it in the kvm_irq_delivery_to_apic() function, or
>> kvm_apic_set_irq().  I'll take a look at that.
>
> We shouldn't have that bit at all.
>
> An alternative is to make the pic/ioapic work in irq context 
> (spin_lock_irq() and friends) and queue the interrupt directly from 
> the hrtimer instead of the vcpu.  My worry is that we have quite a bit 
> of work to do in irq context if the interrupt is broadcast and if we 
> have many vcpus; maybe we can use a work_struct in that case.
>

er, you do that later on.  In this case nothing blocks the removal of 
the bit - if the timer causes an interrupt to be injected, then the 
lapic/irq code will send an IPI to the vcpu to cause it to resample.  
See __apic_accept_irq() in lapic.c calling kvm_vcpu_kick(), for example.

The only issue is the ordering of the patches, you can only remove 
KVM_REQ_PENDING_TIMER after rearranging interrupt injection to happen 
inline.

-- 
error compiling committee.c: too many arguments to function

[PATCH 04/12] IOAPIC timer interrupt redirect.

[Chris Lalancette]

Only redirect IRQ 0 (i.e. timer interrupt) to the BSP if
the APIC destination is multiple vcpus.

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

diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 53f3166..fd52949 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -144,6 +144,15 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
 	}
 }
 
+static int ioapic_dest_multiple_cpus(struct kvm_lapic_irq *irqe)
+{
+	/* physical mode is always directed to 1 cpu */
+	if (irqe->dest_mode == 0 || irqe->dest_id == 0)
+		return 0;
+
+	return hweight_long(irqe->dest_id) != 1;
+}
+
 static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
 {
 	union kvm_ioapic_redirect_entry *entry = &ioapic->redirtbl[irq];
@@ -164,8 +173,10 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
 	irqe.shorthand = 0;
 
 #ifdef CONFIG_X86
-	/* Always deliver PIT interrupt to vcpu 0 */
-	if (irq == 0) {
+	/* if this is the PIT interrupt and the dest_id is multiple
+	 * vcpus, re-write to go to vcpu 0
+	 */
+	if (irq == 0 && ioapic_dest_multiple_cpus(&irqe)) {
 		irqe.dest_mode = 0; /* Physical mode. */
 		/* need to read apic_id from apic register since
 		 * it can be rewritten */
-- 
1.6.5.2

Re: [PATCH 04/12] IOAPIC timer interrupt redirect.

[Marcelo Tosatti]

On Tue, Dec 01, 2009 at 03:36:33PM +0100, Chris Lalancette wrote:
> Only redirect IRQ 0 (i.e. timer interrupt) to the BSP if
> the APIC destination is multiple vcpus.
> 
> Signed-off-by: Chris Lalancette <clalance@redhat.com>
> ---
>  virt/kvm/ioapic.c |   15 +++++++++++++--
>  1 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 53f3166..fd52949 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -144,6 +144,15 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
>  	}
>  }
>  
> +static int ioapic_dest_multiple_cpus(struct kvm_lapic_irq *irqe)
> +{
> +	/* physical mode is always directed to 1 cpu */
> +	if (irqe->dest_mode == 0 || irqe->dest_id == 0)
> +		return 0;
> +
> +	return hweight_long(irqe->dest_id) != 1;
> +}

A single ID can have more than one bit set :(

Better limit the override to delivery mode lowest priority (001).

[PATCH 05/12] Make the IOAPIC lock a spinlock.

[Chris Lalancette]

In order to be able to call kvm_set_irq from an interrupt
context, the IOAPIC lock can't be a (possibly sleeping) mutex.
Convert it to a spinlock.

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

diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index fd52949..3d68826 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -192,8 +192,9 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
 	u32 mask = 1 << irq;
 	union kvm_ioapic_redirect_entry entry;
 	int ret = 1;
+	unsigned long flags;
 
-	mutex_lock(&ioapic->lock);
+	spin_lock_irqsave(&ioapic->lock, flags);
 	if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
 		entry = ioapic->redirtbl[irq];
 		level ^= entry.fields.polarity;
@@ -210,7 +211,7 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
 		}
 		trace_kvm_ioapic_set_irq(entry.bits, irq, ret == 0);
 	}
-	mutex_unlock(&ioapic->lock);
+	spin_unlock_irqrestore(&ioapic->lock, flags);
 
 	return ret;
 }
@@ -234,9 +235,9 @@ static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
 		 * is dropped it will be put into irr and will be delivered
 		 * after ack notifier returns.
 		 */
-		mutex_unlock(&ioapic->lock);
+		spin_unlock(&ioapic->lock);
 		kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, i);
-		mutex_lock(&ioapic->lock);
+		spin_lock(&ioapic->lock);
 
 		if (trigger_mode != IOAPIC_LEVEL_TRIG)
 			continue;
@@ -251,10 +252,11 @@ static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
 void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode)
 {
 	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
+	unsigned long flags;
 
-	mutex_lock(&ioapic->lock);
+	spin_lock_irqsave(&ioapic->lock, flags);
 	__kvm_ioapic_update_eoi(ioapic, vector, trigger_mode);
-	mutex_unlock(&ioapic->lock);
+	spin_unlock_irqrestore(&ioapic->lock, flags);
 }
 
 static inline struct kvm_ioapic *to_ioapic(struct kvm_io_device *dev)
@@ -273,6 +275,8 @@ static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
 {
 	struct kvm_ioapic *ioapic = to_ioapic(this);
 	u32 result;
+	unsigned long flags;
+
 	if (!ioapic_in_range(ioapic, addr))
 		return -EOPNOTSUPP;
 
@@ -280,7 +284,7 @@ static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
 	ASSERT(!(addr & 0xf));	/* check alignment */
 
 	addr &= 0xff;
-	mutex_lock(&ioapic->lock);
+	spin_lock_irqsave(&ioapic->lock, flags);
 	switch (addr) {
 	case IOAPIC_REG_SELECT:
 		result = ioapic->ioregsel;
@@ -294,7 +298,7 @@ static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
 		result = 0;
 		break;
 	}
-	mutex_unlock(&ioapic->lock);
+	spin_unlock_irqrestore(&ioapic->lock, flags);
 
 	switch (len) {
 	case 8:
@@ -316,6 +320,8 @@ static int ioapic_mmio_write(struct kvm_io_device *this, gpa_t addr, int len,
 {
 	struct kvm_ioapic *ioapic = to_ioapic(this);
 	u32 data;
+	unsigned long flags;
+
 	if (!ioapic_in_range(ioapic, addr))
 		return -EOPNOTSUPP;
 
@@ -331,7 +337,7 @@ static int ioapic_mmio_write(struct kvm_io_device *this, gpa_t addr, int len,
 	}
 
 	addr &= 0xff;
-	mutex_lock(&ioapic->lock);
+	spin_lock_irqsave(&ioapic->lock, flags);
 	switch (addr) {
 	case IOAPIC_REG_SELECT:
 		ioapic->ioregsel = data;
@@ -349,7 +355,7 @@ static int ioapic_mmio_write(struct kvm_io_device *this, gpa_t addr, int len,
 	default:
 		break;
 	}
-	mutex_unlock(&ioapic->lock);
+	spin_unlock_irqrestore(&ioapic->lock, flags);
 	return 0;
 }
 
@@ -378,7 +384,7 @@ int kvm_ioapic_init(struct kvm *kvm)
 	ioapic = kzalloc(sizeof(struct kvm_ioapic), GFP_KERNEL);
 	if (!ioapic)
 		return -ENOMEM;
-	mutex_init(&ioapic->lock);
+	spin_lock_init(&ioapic->lock);
 	kvm->arch.vioapic = ioapic;
 	kvm_ioapic_reset(ioapic);
 	kvm_iodevice_init(&ioapic->dev, &ioapic_mmio_ops);
@@ -393,23 +399,27 @@ int kvm_ioapic_init(struct kvm *kvm)
 int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
 {
 	struct kvm_ioapic *ioapic = ioapic_irqchip(kvm);
+	unsigned long flags;
+
 	if (!ioapic)
 		return -EINVAL;
 
-	mutex_lock(&ioapic->lock);
+	spin_lock_irqsave(&ioapic->lock, flags);
 	memcpy(state, ioapic, sizeof(struct kvm_ioapic_state));
-	mutex_unlock(&ioapic->lock);
+	spin_unlock_irqrestore(&ioapic->lock, flags);
 	return 0;
 }
 
 int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
 {
 	struct kvm_ioapic *ioapic = ioapic_irqchip(kvm);
+	unsigned long flags;
+
 	if (!ioapic)
 		return -EINVAL;
 
-	mutex_lock(&ioapic->lock);
+	spin_lock_irqsave(&ioapic->lock, flags);
 	memcpy(ioapic, state, sizeof(struct kvm_ioapic_state));
-	mutex_unlock(&ioapic->lock);
+	spin_unlock_irqrestore(&ioapic->lock, flags);
 	return 0;
 }
diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
index 419c43b..5b4f756 100644
--- a/virt/kvm/ioapic.h
+++ b/virt/kvm/ioapic.h
@@ -45,7 +45,7 @@ struct kvm_ioapic {
 	struct kvm_io_device dev;
 	struct kvm *kvm;
 	void (*ack_notifier)(void *opaque, int irq);
-	struct mutex lock;
+	spinlock_t lock;
 };
 
 #ifdef DEBUG
-- 
1.6.5.2

Re: [PATCH 05/12] Make the IOAPIC lock a spinlock.

[Avi Kivity]

On 12/01/2009 04:36 PM, Chris Lalancette wrote:
> In order to be able to call kvm_set_irq from an interrupt
> context, the IOAPIC lock can't be a (possibly sleeping) mutex.
> Convert it to a spinlock.
>    

The ioapic can call various notifiers (ack_notifiers, mask_notifiers).  
Have you validated that all notifier callbacks are safe wrt the lock 
(that is, they don't sleep or send IPIs)?

-- 
error compiling committee.c: too many arguments to function

[PATCH 06/12] Make the PIC use interrupt safe locking.

[Chris Lalancette]

Since we want to be able to call kvm_pic_set_irq() in interrupt
context, convert the uses of spin_lock() to spin_lock_irqsave()
as appropriate in i8259.c

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

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 59a8a68..8285f0d 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -52,10 +52,12 @@ static void pic_clear_isr(struct kvm_kpic_state *s, int irq)
 void kvm_pic_clear_isr_ack(struct kvm *kvm)
 {
 	struct kvm_pic *s = pic_irqchip(kvm);
-	spin_lock(&s->lock);
+	unsigned long flags;
+
+	spin_lock_irqsave(&s->lock, flags);
 	s->pics[0].isr_ack = 0xff;
 	s->pics[1].isr_ack = 0xff;
-	spin_unlock(&s->lock);
+	spin_unlock_irqrestore(&s->lock, flags);
 }
 
 /*
@@ -156,24 +158,27 @@ static void pic_update_irq(struct kvm_pic *s)
 
 void kvm_pic_update_irq(struct kvm_pic *s)
 {
-	spin_lock(&s->lock);
+	unsigned long flags;
+
+	spin_lock_irqsave(&s->lock, flags);
 	pic_update_irq(s);
-	spin_unlock(&s->lock);
+	spin_unlock_irqrestore(&s->lock, flags);
 }
 
 int kvm_pic_set_irq(void *opaque, int irq, int level)
 {
 	struct kvm_pic *s = opaque;
 	int ret = -1;
+	unsigned long flags;
 
-	spin_lock(&s->lock);
+	spin_lock_irqsave(&s->lock, flags);
 	if (irq >= 0 && irq < PIC_NUM_PINS) {
 		ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
 		pic_update_irq(s);
 		trace_kvm_pic_set_irq(irq >> 3, irq & 7, s->pics[irq >> 3].elcr,
 				      s->pics[irq >> 3].imr, ret == 0);
 	}
-	spin_unlock(&s->lock);
+	spin_unlock_irqrestore(&s->lock, flags);
 
 	return ret;
 }
@@ -202,8 +207,9 @@ int kvm_pic_read_irq(struct kvm *kvm)
 {
 	int irq, irq2, intno;
 	struct kvm_pic *s = pic_irqchip(kvm);
+	unsigned long flags;
 
-	spin_lock(&s->lock);
+	spin_lock_irqsave(&s->lock, flags);
 	irq = pic_get_irq(&s->pics[0]);
 	if (irq >= 0) {
 		pic_intack(&s->pics[0], irq);
@@ -228,7 +234,7 @@ int kvm_pic_read_irq(struct kvm *kvm)
 		intno = s->pics[0].irq_base + irq;
 	}
 	pic_update_irq(s);
-	spin_unlock(&s->lock);
+	spin_unlock_irqrestore(&s->lock, flags);
 
 	return intno;
 }
@@ -434,6 +440,8 @@ static int picdev_write(struct kvm_io_device *this,
 {
 	struct kvm_pic *s = to_pic(this);
 	unsigned char data = *(unsigned char *)val;
+	unsigned long flags;
+
 	if (!picdev_in_range(addr))
 		return -EOPNOTSUPP;
 
@@ -442,7 +450,7 @@ static int picdev_write(struct kvm_io_device *this,
 			printk(KERN_ERR "PIC: non byte write\n");
 		return 0;
 	}
-	spin_lock(&s->lock);
+	spin_lock_irqsave(&s->lock, flags);
 	switch (addr) {
 	case 0x20:
 	case 0x21:
@@ -455,7 +463,7 @@ static int picdev_write(struct kvm_io_device *this,
 		elcr_ioport_write(&s->pics[addr & 1], addr, data);
 		break;
 	}
-	spin_unlock(&s->lock);
+	spin_unlock_irqrestore(&s->lock, flags);
 	return 0;
 }
 
@@ -464,6 +472,8 @@ static int picdev_read(struct kvm_io_device *this,
 {
 	struct kvm_pic *s = to_pic(this);
 	unsigned char data = 0;
+	unsigned long flags;
+
 	if (!picdev_in_range(addr))
 		return -EOPNOTSUPP;
 
@@ -472,7 +482,7 @@ static int picdev_read(struct kvm_io_device *this,
 			printk(KERN_ERR "PIC: non byte read\n");
 		return 0;
 	}
-	spin_lock(&s->lock);
+	spin_lock_irqsave(&s->lock, flags);
 	switch (addr) {
 	case 0x20:
 	case 0x21:
@@ -486,7 +496,7 @@ static int picdev_read(struct kvm_io_device *this,
 		break;
 	}
 	*(unsigned char *)val = data;
-	spin_unlock(&s->lock);
+	spin_unlock_irqrestore(&s->lock, flags);
 	return 0;
 }
 
-- 
1.6.5.2

[PATCH 07/12] Rename kvm_apic_accept_pic_intr

[Chris Lalancette]

Call it kvm_apic_in_virtual_wire_mode, which is more
correct.  Also change it to not only operate properly
on the boot CPU, but on any CPU.

Signed-off-by: Chris Lalancette <clalance@redhat.com>
---
 arch/x86/kvm/i8259.c |    2 +-
 arch/x86/kvm/irq.c   |    4 ++--
 arch/x86/kvm/lapic.c |   17 ++++++++---------
 arch/x86/kvm/lapic.h |    2 +-
 4 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 8285f0d..80d4705 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -263,7 +263,7 @@ static void kvm_pic_reset(struct kvm_kpic_state *s)
 	s->init4 = 0;
 
 	for (irq = 0; irq < PIC_NUM_PINS/2; irq++) {
-		if (vcpu0 && kvm_apic_accept_pic_intr(vcpu0))
+		if (vcpu0 && kvm_apic_in_virtual_wire_mode(vcpu0))
 			if (irr & (1 << irq) || isr & (1 << irq)) {
 				pic_clear_isr(s, irq);
 			}
diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
index 96dfbb6..05721fc 100644
--- a/arch/x86/kvm/irq.c
+++ b/arch/x86/kvm/irq.c
@@ -53,7 +53,7 @@ int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
 		return v->arch.interrupt.pending;
 
 	if (kvm_apic_has_interrupt(v) == -1) {	/* LAPIC */
-		if (kvm_apic_accept_pic_intr(v)) {
+		if (kvm_apic_in_virtual_wire_mode(v)) {
 			s = pic_irqchip(v->kvm);	/* PIC */
 			return s->output;
 		} else
@@ -76,7 +76,7 @@ int kvm_cpu_get_interrupt(struct kvm_vcpu *v)
 
 	vector = kvm_get_apic_interrupt(v);	/* APIC */
 	if (vector == -1) {
-		if (kvm_apic_accept_pic_intr(v)) {
+		if (kvm_apic_in_virtual_wire_mode(v)) {
 			s = pic_irqchip(v->kvm);
 			s->output = 0;		/* PIC */
 			vector = kvm_pic_read_irq(v->kvm);
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index cd60c0b..44acf7c 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -435,7 +435,7 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 		/*
 		 * Should only be called by kvm_apic_local_deliver() with LVT0,
 		 * before NMI watchdog was enabled. Already handled by
-		 * kvm_apic_accept_pic_intr().
+		 * kvm_apic_in_virtual_wire_mode().
 		 */
 		break;
 
@@ -1099,18 +1099,17 @@ int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu)
 	return highest_irr;
 }
 
-int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)
+int kvm_apic_in_virtual_wire_mode(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;
 }
 
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 40010b0..ce4cd2d 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -22,7 +22,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu);
 void kvm_free_lapic(struct kvm_vcpu *vcpu);
 
 int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu);
-int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu);
+int kvm_apic_in_virtual_wire_mode(struct kvm_vcpu *vcpu);
 int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
 void kvm_lapic_reset(struct kvm_vcpu *vcpu);
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu);
-- 
1.6.5.2

Re: [PATCH 07/12] Rename kvm_apic_accept_pic_intr

[Avi Kivity]

On 12/01/2009 04:36 PM, Chris Lalancette wrote:
> Call it kvm_apic_in_virtual_wire_mode, which is more
> correct.  Also change it to not only operate properly
> on the boot CPU, but on any CPU.
>    

I'm a little worried about the latter change.  It effectively modifies 
the platform from having the PIC wired to just one cpu to having it 
wired to all CPUs.  Let's leave that for now (but keep the rename).

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 07/12] Rename kvm_apic_accept_pic_intr

[Gleb Natapov]

On Tue, Dec 01, 2009 at 03:36:36PM +0100, Chris Lalancette wrote:
> Call it kvm_apic_in_virtual_wire_mode, which is more
> correct.  Also change it to not only operate properly
> on the boot CPU, but on any CPU.
> 
Currently it is assumed that if kvm_cpu_has_interrupt() returns true
kvm_cpu_get_interrupt() will return valid interrupt. After this change
it may not be the case any more. Suppose there are more then one vcpus
in a virtual wire mode. Pic receives interrupt and multiple cpus try
to inject it simultaneously. They all check that interrupt is pending
by calling kvm_cpu_has_interrupt() and then try to get the vector by
calling kvm_cpu_get_interrupt(). Only one vcpu will get correct value,
others will get spurious interrupt.

--
			Gleb.

[PATCH 08/12] Remove timer.c

[Chris Lalancette]

The code in timer.c isn't really similar enough between
the i8254 and the lapic to share.  Split these into
separate functions, and remove timer.c

Signed-off-by: Chris Lalancette <clalance@redhat.com>
---
 arch/x86/kvm/Makefile    |    3 +-
 arch/x86/kvm/i8254.c     |   27 ++++++++++++++++++++++++-
 arch/x86/kvm/kvm_timer.h |    5 ++-
 arch/x86/kvm/lapic.c     |   33 +++++++++++++++++++++++++++++++-
 arch/x86/kvm/timer.c     |   47 ----------------------------------------------
 5 files changed, 61 insertions(+), 54 deletions(-)
 delete mode 100644 arch/x86/kvm/timer.c

diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
index 31a7035..8d9adf6 100644
--- a/arch/x86/kvm/Makefile
+++ b/arch/x86/kvm/Makefile
@@ -10,8 +10,7 @@ kvm-y			+= $(addprefix ../../../virt/kvm/, kvm_main.o ioapic.o \
 				assigned-dev.o)
 kvm-$(CONFIG_IOMMU_API)	+= $(addprefix ../../../virt/kvm/, iommu.o)
 
-kvm-y			+= x86.o mmu.o emulate.o i8259.o irq.o lapic.o \
-			   i8254.o timer.o
+kvm-y			+= x86.o mmu.o emulate.o i8259.o irq.o lapic.o i8254.o
 kvm-intel-y		+= vmx.o
 kvm-amd-y		+= svm.o
 
diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index fab7440..dc6eff4 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -277,6 +277,30 @@ static struct kvm_timer_ops kpit_ops = {
 	.is_periodic = kpit_is_periodic,
 };
 
+static enum hrtimer_restart pit_timer_fn(struct hrtimer *data)
+{
+	struct kvm_timer *ktimer = container_of(data, struct kvm_timer, timer);
+	struct kvm *kvm = ktimer->kvm;
+
+	/*
+	 * There is a race window between reading and incrementing, but we do
+	 * not care about potentially losing timer events in the !reinject
+	 * case anyway.
+	 */
+	if (ktimer->reinject || !atomic_read(&ktimer->pending))
+		atomic_inc(&ktimer->pending);
+
+	if (waitqueue_active(&kvm->bsp_vcpu->wq))
+		wake_up_interruptible(&kvm->bsp_vcpu->wq);
+
+	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,10 +315,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;
diff --git a/arch/x86/kvm/kvm_timer.h b/arch/x86/kvm/kvm_timer.h
index 55c7524..fbd4acb 100644
--- a/arch/x86/kvm/kvm_timer.h
+++ b/arch/x86/kvm/kvm_timer.h
@@ -1,3 +1,5 @@
+#ifndef __KVM_TIMER_H
+#define __KVM_TIMER_H
 
 struct kvm_timer {
 	struct hrtimer timer;
@@ -13,6 +15,5 @@ struct kvm_timer_ops {
         bool (*is_periodic)(struct kvm_timer *);
 };
 
-
-enum hrtimer_restart kvm_timer_fn(struct hrtimer *data);
+#endif
 
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 44acf7c..0005409 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1040,6 +1040,37 @@ static const struct kvm_io_device_ops apic_mmio_ops = {
 	.write    = apic_mmio_write,
 };
 
+static enum hrtimer_restart lapic_timer_fn(struct hrtimer *data)
+{
+	struct kvm_timer *ktimer = container_of(data, struct kvm_timer, timer);
+	struct kvm_vcpu *vcpu;
+	struct kvm_lapic *lapic;
+
+	vcpu = ktimer->vcpu;
+	if (!vcpu)
+		return HRTIMER_NORESTART;
+
+	lapic = vcpu->arch.apic;
+
+	/*
+	 * There is a race window between reading and incrementing, but we do
+	 * not care about potentially losing timer events in the !reinject
+	 * case anyway.
+	 */
+	if (ktimer->reinject || !atomic_read(&ktimer->pending))
+		atomic_inc(&ktimer->pending);
+
+	if (waitqueue_active(&vcpu->wq))
+		wake_up_interruptible(&vcpu->wq);
+
+	if (ktimer->t_ops->is_periodic(ktimer)) {
+		hrtimer_add_expires_ns(&ktimer->timer, ktimer->period);
+		return HRTIMER_RESTART;
+	}
+	else
+		return HRTIMER_NORESTART;
+}
+
 int kvm_create_lapic(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic;
@@ -1065,7 +1096,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
 
 	hrtimer_init(&apic->lapic_timer.timer, CLOCK_MONOTONIC,
 		     HRTIMER_MODE_ABS);
-	apic->lapic_timer.timer.function = kvm_timer_fn;
+	apic->lapic_timer.timer.function = lapic_timer_fn;
 	apic->lapic_timer.t_ops = &lapic_timer_ops;
 	apic->lapic_timer.kvm = vcpu->kvm;
 	apic->lapic_timer.vcpu = vcpu;
diff --git a/arch/x86/kvm/timer.c b/arch/x86/kvm/timer.c
deleted file mode 100644
index 72b5144..0000000
--- a/arch/x86/kvm/timer.c
+++ /dev/null
@@ -1,47 +0,0 @@
-#include <linux/kvm_host.h>
-#include <linux/kvm.h>
-#include <linux/hrtimer.h>
-#include <asm/atomic.h>
-#include "kvm_timer.h"
-
-static int __kvm_timer_fn(struct kvm_vcpu *vcpu, struct kvm_timer *ktimer)
-{
-	int restart_timer = 0;
-	wait_queue_head_t *q = &vcpu->wq;
-
-	/*
-	 * There is a race window between reading and incrementing, but we do
-	 * not care about potentially loosing timer events in the !reinject
-	 * case anyway.
-	 */
-	if (ktimer->reinject || !atomic_read(&ktimer->pending))
-		atomic_inc(&ktimer->pending);
-
-	if (waitqueue_active(q))
-		wake_up_interruptible(q);
-
-	if (ktimer->t_ops->is_periodic(ktimer)) {
-		hrtimer_add_expires_ns(&ktimer->timer, ktimer->period);
-		restart_timer = 1;
-	}
-
-	return restart_timer;
-}
-
-enum hrtimer_restart kvm_timer_fn(struct hrtimer *data)
-{
-	int restart_timer;
-	struct kvm_vcpu *vcpu;
-	struct kvm_timer *ktimer = container_of(data, struct kvm_timer, timer);
-
-	vcpu = ktimer->vcpu;
-	if (!vcpu)
-		return HRTIMER_NORESTART;
-
-	restart_timer = __kvm_timer_fn(vcpu, ktimer);
-	if (restart_timer)
-		return HRTIMER_RESTART;
-	else
-		return HRTIMER_NORESTART;
-}
-
-- 
1.6.5.2

Re: [PATCH 08/12] Remove timer.c

[Marcelo Tosatti]

On Tue, Dec 01, 2009 at 03:36:37PM +0100, Chris Lalancette wrote:
> The code in timer.c isn't really similar enough between
> the i8254 and the lapic to share.  Split these into
> separate functions, and remove timer.c
> 
> Signed-off-by: Chris Lalancette <clalance@redhat.com>

There should be one function. Abstract the wake up waitqueue (which
requires a vcpu) for lapic and kvm_set_irq/whatever for PIT into
per-timer callback please.

[PATCH 09/12] Fix missing spin_lock in PIT timer.

[Chris Lalancette]

Changes to the irq_ack variable in the pit_state must be
protected by the inject_lock spinlock; otherwise, we can
erroneously inject a timer interrupt into a guest.

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

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index dc6eff4..ece7e12 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -606,10 +606,13 @@ void kvm_pit_reset(struct kvm_pit *pit)
 static void pit_mask_notifer(struct kvm_irq_mask_notifier *kimn, bool mask)
 {
 	struct kvm_pit *pit = container_of(kimn, struct kvm_pit, mask_notifier);
+	unsigned long flags;
 
 	if (!mask) {
+		spin_lock_irqsave(&pit->pit_state.inject_lock, flags);
 		atomic_set(&pit->pit_state.pit_timer.pending, 0);
 		pit->pit_state.irq_ack = 1;
+		spin_unlock_irqrestore(&pit->pit_state.inject_lock, flags);
 	}
 }
 
-- 
1.6.5.2

[PATCH 10/12] Inject i8254 interrupts directly from hrtimer callback.

[Chris Lalancette]

Signed-off-by: Chris Lalancette <clalance@redhat.com>
---
 arch/x86/kvm/i8254.c |  101 +++++++++++---------------------------------------
 arch/x86/kvm/i8254.h |    1 -
 arch/x86/kvm/irq.c   |   10 +----
 arch/x86/kvm/irq.h   |    2 -
 4 files changed, 23 insertions(+), 91 deletions(-)

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index ece7e12..feef9d5 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -227,37 +227,15 @@ static void pit_latch_status(struct kvm *kvm, int channel)
 	}
 }
 
-int pit_has_pending_timer(struct kvm_vcpu *vcpu)
-{
-	struct kvm_pit *pit = vcpu->kvm->arch.vpit;
-
-	if (pit && kvm_vcpu_is_bsp(vcpu) && pit->pit_state.irq_ack)
-		return atomic_read(&pit->pit_state.pit_timer.pending);
-	return 0;
-}
-
 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);
-	spin_lock(&ps->inject_lock);
-	if (atomic_dec_return(&ps->pit_timer.pending) < 0)
-		atomic_inc(&ps->pit_timer.pending);
-	ps->irq_ack = 1;
-	spin_unlock(&ps->inject_lock);
-}
-
-void __kvm_migrate_pit_timer(struct kvm_vcpu *vcpu)
-{
-	struct kvm_pit *pit = vcpu->kvm->arch.vpit;
-	struct hrtimer *timer;
-
-	if (!kvm_vcpu_is_bsp(vcpu) || !pit)
-		return;
+	unsigned long flags;
 
-	timer = &pit->pit_state.pit_timer.timer;
-	if (hrtimer_cancel(timer))
-		hrtimer_start_expires(timer, HRTIMER_MODE_ABS);
+	spin_lock_irqsave(&ps->inject_lock, flags);
+	ps->irq_ack = 1;
+	spin_unlock_irqrestore(&ps->inject_lock, flags);
 }
 
 static void destroy_pit_timer(struct kvm_timer *pt)
@@ -281,17 +259,31 @@ static enum hrtimer_restart pit_timer_fn(struct hrtimer *data)
 {
 	struct kvm_timer *ktimer = container_of(data, struct kvm_timer, timer);
 	struct kvm *kvm = ktimer->kvm;
+	struct kvm_kpit_state *ps = &kvm->arch.vpit->pit_state;
+	struct kvm_vcpu *vcpu;
+	int i;
+	int inject = 0;
 
 	/*
 	 * There is a race window between reading and incrementing, but we do
 	 * not care about potentially losing timer events in the !reinject
 	 * case anyway.
 	 */
-	if (ktimer->reinject || !atomic_read(&ktimer->pending))
-		atomic_inc(&ktimer->pending);
+	spin_lock(&ps->inject_lock);
+	if (ktimer->reinject && ps->irq_ack) {
+		ps->irq_ack = 0;
+		inject = 1;
+	}
+	spin_unlock(&ps->inject_lock);
 
-	if (waitqueue_active(&kvm->bsp_vcpu->wq))
-		wake_up_interruptible(&kvm->bsp_vcpu->wq);
+	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);
+
+		if (kvm->arch.vapics_in_nmi_mode > 0)
+			kvm_for_each_vcpu(i, vcpu, kvm)
+				kvm_apic_nmi_wd_deliver(vcpu);
+	}
 
 	if (ktimer->t_ops->is_periodic(ktimer)) {
 		hrtimer_add_expires_ns(&ktimer->timer, ktimer->period);
@@ -319,7 +311,6 @@ static void create_pit_timer(struct kvm_kpit_state *ps, u32 val, int is_period)
 	pt->t_ops = &kpit_ops;
 	pt->kvm = ps->pit->kvm;
 
-	atomic_set(&pt->pending, 0);
 	ps->irq_ack = 1;
 
 	hrtimer_start(&pt->timer, ktime_add_ns(ktime_get(), interval),
@@ -599,7 +590,6 @@ void kvm_pit_reset(struct kvm_pit *pit)
 	}
 	mutex_unlock(&pit->pit_state.lock);
 
-	atomic_set(&pit->pit_state.pit_timer.pending, 0);
 	pit->pit_state.irq_ack = 1;
 }
 
@@ -610,7 +600,6 @@ static void pit_mask_notifer(struct kvm_irq_mask_notifier *kimn, bool mask)
 
 	if (!mask) {
 		spin_lock_irqsave(&pit->pit_state.inject_lock, flags);
-		atomic_set(&pit->pit_state.pit_timer.pending, 0);
 		pit->pit_state.irq_ack = 1;
 		spin_unlock_irqrestore(&pit->pit_state.inject_lock, flags);
 	}
@@ -708,49 +697,3 @@ void kvm_free_pit(struct kvm *kvm)
 		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.
-		 */
-		spin_lock(&ps->inject_lock);
-		if (atomic_read(&ps->pit_timer.pending) && ps->irq_ack) {
-			ps->irq_ack = 0;
-			inject = 1;
-		}
-		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 d4c1c7f..02a1e3a 100644
--- a/arch/x86/kvm/i8254.h
+++ b/arch/x86/kvm/i8254.h
@@ -49,7 +49,6 @@ struct kvm_pit {
 #define KVM_MAX_PIT_INTR_INTERVAL   HZ / 100
 #define KVM_PIT_CHANNEL_MASK	    0x3
 
-void kvm_inject_pit_timer_irqs(struct kvm_vcpu *vcpu);
 void kvm_pit_load_count(struct kvm *kvm, int channel, u32 val, int hpet_legacy_start);
 struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags);
 void kvm_free_pit(struct kvm *kvm);
diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
index 05721fc..b5db0de 100644
--- a/arch/x86/kvm/irq.c
+++ b/arch/x86/kvm/irq.c
@@ -32,12 +32,7 @@
  */
 int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu)
 {
-	int ret;
-
-	ret = pit_has_pending_timer(vcpu);
-	ret |= apic_has_pending_timer(vcpu);
-
-	return ret;
+	return apic_has_pending_timer(vcpu);
 }
 EXPORT_SYMBOL(kvm_cpu_has_pending_timer);
 
@@ -89,13 +84,10 @@ 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);
 
 void __kvm_migrate_timers(struct kvm_vcpu *vcpu)
 {
 	__kvm_migrate_apic_timer(vcpu);
-	__kvm_migrate_pit_timer(vcpu);
 }
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index a5344f1..3bba4f3 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -97,10 +97,8 @@ void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu);
 void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu);
 void kvm_apic_nmi_wd_deliver(struct kvm_vcpu *vcpu);
 void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu);
-void __kvm_migrate_pit_timer(struct kvm_vcpu *vcpu);
 void __kvm_migrate_timers(struct kvm_vcpu *vcpu);
 
-int pit_has_pending_timer(struct kvm_vcpu *vcpu);
 int apic_has_pending_timer(struct kvm_vcpu *vcpu);
 
 #endif
-- 
1.6.5.2

[PATCH 11/12] Allow the PIC to signal CPUs other than BSP.

[Chris Lalancette]

While generally the PIT is used to signal only the BSP,
it's doesn't actually have to do that architecturally.
Allow the PIC to signal any VCPU that is in
Virtual Wire mode.

Signed-off-by: Chris Lalancette <clalance@redhat.com>
---
 arch/x86/kvm/i8259.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 80d4705..ff20415 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -242,8 +242,6 @@ int kvm_pic_read_irq(struct kvm *kvm)
 static void kvm_pic_reset(struct kvm_kpic_state *s)
 {
 	int irq;
-	struct kvm *kvm = s->pics_state->irq_request_opaque;
-	struct kvm_vcpu *vcpu0 = kvm->bsp_vcpu;
 	u8 irr = s->irr, isr = s->imr;
 
 	s->last_irr = 0;
@@ -263,10 +261,8 @@ static void kvm_pic_reset(struct kvm_kpic_state *s)
 	s->init4 = 0;
 
 	for (irq = 0; irq < PIC_NUM_PINS/2; irq++) {
-		if (vcpu0 && kvm_apic_in_virtual_wire_mode(vcpu0))
-			if (irr & (1 << irq) || isr & (1 << irq)) {
-				pic_clear_isr(s, irq);
-			}
+		if (irr & (1 << irq) || isr & (1 << irq))
+			pic_clear_isr(s, irq);
 	}
 }
 
@@ -506,14 +502,19 @@ static int picdev_read(struct kvm_io_device *this,
 static void pic_irq_request(void *opaque, int level)
 {
 	struct kvm *kvm = opaque;
-	struct kvm_vcpu *vcpu = kvm->bsp_vcpu;
 	struct kvm_pic *s = pic_irqchip(kvm);
 	int irq = pic_get_irq(&s->pics[0]);
+	struct kvm_vcpu *vcpu;
+	int i;
 
 	s->output = level;
-	if (vcpu && level && (s->pics[0].isr_ack & (1 << irq))) {
+	if (level && (s->pics[0].isr_ack & (1 << irq))) {
 		s->pics[0].isr_ack &= ~(1 << irq);
-		kvm_vcpu_kick(vcpu);
+		kvm_for_each_vcpu(i, vcpu, kvm) {
+			if (kvm_apic_in_virtual_wire_mode(vcpu) &&
+			    waitqueue_active(&vcpu->wq))
+				wake_up_interruptible(&vcpu->wq);
+		}
 	}
 }
 
-- 
1.6.5.2

[PATCH 12/12] Kick appropriate CPUs when signalling interrupts.

[Chris Lalancette]

Make sure that we kick the appropriate vcpu when delivering
an interrupt.  This makes sure that we wake any idle cpus
to cause a "vcpu_run" and an interrupt injection to occur.

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

diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index 9b07734..96df854 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -98,6 +98,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);
+			kvm_vcpu_kick(vcpu);
 		} else {
 			if (!lowest)
 				lowest = vcpu;
@@ -106,8 +107,10 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 		}
 	}
 
-	if (lowest)
+	if (lowest) {
 		r = kvm_apic_set_irq(lowest, irq);
+		kvm_vcpu_kick(lowest);
+	}
 
 	return r;
 }
-- 
1.6.5.2

Re: [PATCH 12/12] Kick appropriate CPUs when signalling interrupts.

[Gleb Natapov]

On Tue, Dec 01, 2009 at 03:36:41PM +0100, Chris Lalancette wrote:
> Make sure that we kick the appropriate vcpu when delivering
> an interrupt.  This makes sure that we wake any idle cpus
> to cause a "vcpu_run" and an interrupt injection to occur.
> 
> Signed-off-by: Chris Lalancette <clalance@redhat.com>
> ---
>  virt/kvm/irq_comm.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> index 9b07734..96df854 100644
> --- a/virt/kvm/irq_comm.c
> +++ b/virt/kvm/irq_comm.c
> @@ -98,6 +98,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);
> +			kvm_vcpu_kick(vcpu);
>  		} else {
>  			if (!lowest)
>  				lowest = vcpu;
> @@ -106,8 +107,10 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>  		}
>  	}
>  
> -	if (lowest)
> +	if (lowest) {
>  		r = kvm_apic_set_irq(lowest, irq);
> +		kvm_vcpu_kick(lowest);
> +	}
>  
>  	return r;
>  }
kvm_vcpu_kick() is done inside kvm_apic_set_irq(), so why is this
needed?

--
			Gleb.

Re: [PATCH 12/12] Kick appropriate CPUs when signalling interrupts.

[Chris Lalancette]

On 12/02/2009 04:44 PM, Gleb Natapov wrote:
> On Tue, Dec 01, 2009 at 03:36:41PM +0100, Chris Lalancette wrote:
>> Make sure that we kick the appropriate vcpu when delivering
>> an interrupt.  This makes sure that we wake any idle cpus
>> to cause a "vcpu_run" and an interrupt injection to occur.
>>
>> Signed-off-by: Chris Lalancette <clalance@redhat.com>
>> ---
>>  virt/kvm/irq_comm.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
>> index 9b07734..96df854 100644
>> --- a/virt/kvm/irq_comm.c
>> +++ b/virt/kvm/irq_comm.c
>> @@ -98,6 +98,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);
>> +			kvm_vcpu_kick(vcpu);
>>  		} else {
>>  			if (!lowest)
>>  				lowest = vcpu;
>> @@ -106,8 +107,10 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>>  		}
>>  	}
>>  
>> -	if (lowest)
>> +	if (lowest) {
>>  		r = kvm_apic_set_irq(lowest, irq);
>> +		kvm_vcpu_kick(lowest);
>> +	}
>>  
>>  	return r;
>>  }
> kvm_vcpu_kick() is done inside kvm_apic_set_irq(), so why is this
> needed?

Yeah, you are absolutely right.  I mis-read it before.  I'll remove this bit.

-- 
Chris Lalancette