[PATCH 0/3] PIC fixes

[PATCH 0/3] PIC fixes

[Gleb Natapov]

Ack notifiers called at wrong time in PIC. kvm_vcpu_kick() can be called
under spinlock now, so code can be simplified.

Gleb Natapov (3):
  Call kvm_vcpu_kick() inside pic spinlock
  Call ack notifiers from PIC when guest OS acks an IRQ.
  Replace pic_lock()/pic_unlock() with direct call to spinlock
    functions.

 arch/x86/kvm/i8259.c |   64 +++++++++++++------------------------------------
 arch/x86/kvm/irq.h   |    1 -
 2 files changed, 17 insertions(+), 48 deletions(-)

[PATCH 1/3] Call kvm_vcpu_kick() inside pic spinlock

[Gleb Natapov]

d5ecfdd25 moved it out because back than it was impossible to
call it inside spinlock. This restriction no longer exists.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/i8259.c |   10 +---------
 arch/x86/kvm/irq.h   |    1 -
 2 files changed, 1 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index daf4606..d27320c 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -43,11 +43,9 @@ static void pic_unlock(struct kvm_pic *s)
 {
 	struct kvm *kvm = s->kvm;
 	unsigned acks = s->pending_acks;
-	bool wakeup = s->wakeup_needed;
 	struct kvm_vcpu *vcpu;
 
 	s->pending_acks = 0;
-	s->wakeup_needed = false;
 
 	spin_unlock(&s->lock);
 
@@ -56,12 +54,6 @@ static void pic_unlock(struct kvm_pic *s)
 				     __ffs(acks));
 		acks &= acks - 1;
 	}
-
-	if (wakeup) {
-		vcpu = s->kvm->bsp_vcpu;
-		if (vcpu)
-			kvm_vcpu_kick(vcpu);
-	}
 }
 
 static void pic_clear_isr(struct kvm_kpic_state *s, int irq)
@@ -527,7 +519,7 @@ static void pic_irq_request(void *opaque, int level)
 	s->output = level;
 	if (vcpu && level && (s->pics[0].isr_ack & (1 << irq))) {
 		s->pics[0].isr_ack &= ~(1 << irq);
-		s->wakeup_needed = true;
+		kvm_vcpu_kick(vcpu);
 	}
 }
 
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index 9f59318..7d6058a 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -63,7 +63,6 @@ struct kvm_kpic_state {
 
 struct kvm_pic {
 	spinlock_t lock;
-	bool wakeup_needed;
 	unsigned pending_acks;
 	struct kvm *kvm;
 	struct kvm_kpic_state pics[2]; /* 0 is master pic, 1 is slave pic */
-- 
1.6.3.3

[PATCH 2/3] Call ack notifiers from PIC when guest OS acks an IRQ.

[Gleb Natapov]

Currently they are called when irq vector is been delivered.  Calling ack
notifiers at this point is wrong.  Device assignment ack notifier enables
host interrupts, but guest not yet had a chance to clear interrupt
condition in a device.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/i8259.c |   18 ++++--------------
 1 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index d27320c..3aacd33 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -41,25 +41,16 @@ static void pic_lock(struct kvm_pic *s)
 static void pic_unlock(struct kvm_pic *s)
 	__releases(&s->lock)
 {
-	struct kvm *kvm = s->kvm;
-	unsigned acks = s->pending_acks;
-	struct kvm_vcpu *vcpu;
-
-	s->pending_acks = 0;
-
 	spin_unlock(&s->lock);
-
-	while (acks) {
-		kvm_notify_acked_irq(kvm, SELECT_PIC(__ffs(acks)),
-				     __ffs(acks));
-		acks &= acks - 1;
-	}
 }
 
 static void pic_clear_isr(struct kvm_kpic_state *s, int irq)
 {
 	s->isr &= ~(1 << irq);
 	s->isr_ack |= (1 << irq);
+	if (s != &s->pics_state->pics[0])
+		irq += 8;
+	kvm_notify_acked_irq(s->pics_state->kvm, SELECT_PIC(irq), irq);
 }
 
 void kvm_pic_clear_isr_ack(struct kvm *kvm)
@@ -240,7 +231,6 @@ int kvm_pic_read_irq(struct kvm *kvm)
 	}
 	pic_update_irq(s);
 	pic_unlock(s);
-	kvm_notify_acked_irq(kvm, SELECT_PIC(irq), irq);
 
 	return intno;
 }
@@ -260,7 +250,7 @@ void kvm_pic_reset(struct kvm_kpic_state *s)
 		if (vcpu0 && kvm_apic_accept_pic_intr(vcpu0))
 			if (s->irr & (1 << irq) || s->isr & (1 << irq)) {
 				n = irq + irqbase;
-				s->pics_state->pending_acks |= 1 << n;
+				kvm_notify_acked_irq(kvm, SELECT_PIC(n), n);
 			}
 	}
 	s->last_irr = 0;
-- 
1.6.3.3

[PATCH 3/3] Replace pic_lock()/pic_unlock() with direct call to spinlock functions.

[Gleb Natapov]

They are not doing anything else now.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/i8259.c |   36 ++++++++++++------------------------
 1 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 3aacd33..01f1516 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -32,18 +32,6 @@
 #include <linux/kvm_host.h>
 #include "trace.h"
 
-static void pic_lock(struct kvm_pic *s)
-	__acquires(&s->lock)
-{
-	spin_lock(&s->lock);
-}
-
-static void pic_unlock(struct kvm_pic *s)
-	__releases(&s->lock)
-{
-	spin_unlock(&s->lock);
-}
-
 static void pic_clear_isr(struct kvm_kpic_state *s, int irq)
 {
 	s->isr &= ~(1 << irq);
@@ -56,10 +44,10 @@ 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);
-	pic_lock(s);
+	spin_lock(&s->lock);
 	s->pics[0].isr_ack = 0xff;
 	s->pics[1].isr_ack = 0xff;
-	pic_unlock(s);
+	spin_unlock(&s->lock);
 }
 
 /*
@@ -160,9 +148,9 @@ static void pic_update_irq(struct kvm_pic *s)
 
 void kvm_pic_update_irq(struct kvm_pic *s)
 {
-	pic_lock(s);
+	spin_lock(&s->lock);
 	pic_update_irq(s);
-	pic_unlock(s);
+	spin_unlock(&s->lock);
 }
 
 int kvm_pic_set_irq(void *opaque, int irq, int level)
@@ -170,14 +158,14 @@ int kvm_pic_set_irq(void *opaque, int irq, int level)
 	struct kvm_pic *s = opaque;
 	int ret = -1;
 
-	pic_lock(s);
+	spin_lock(&s->lock);
 	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);
 	}
-	pic_unlock(s);
+	spin_unlock(&s->lock);
 
 	return ret;
 }
@@ -205,7 +193,7 @@ int kvm_pic_read_irq(struct kvm *kvm)
 	int irq, irq2, intno;
 	struct kvm_pic *s = pic_irqchip(kvm);
 
-	pic_lock(s);
+	spin_lock(&s->lock);
 	irq = pic_get_irq(&s->pics[0]);
 	if (irq >= 0) {
 		pic_intack(&s->pics[0], irq);
@@ -230,7 +218,7 @@ int kvm_pic_read_irq(struct kvm *kvm)
 		intno = s->pics[0].irq_base + irq;
 	}
 	pic_update_irq(s);
-	pic_unlock(s);
+	spin_unlock(&s->lock);
 
 	return intno;
 }
@@ -448,7 +436,7 @@ static int picdev_write(struct kvm_io_device *this,
 			printk(KERN_ERR "PIC: non byte write\n");
 		return 0;
 	}
-	pic_lock(s);
+	spin_lock(&s->lock);
 	switch (addr) {
 	case 0x20:
 	case 0x21:
@@ -461,7 +449,7 @@ static int picdev_write(struct kvm_io_device *this,
 		elcr_ioport_write(&s->pics[addr & 1], addr, data);
 		break;
 	}
-	pic_unlock(s);
+	spin_unlock(&s->lock);
 	return 0;
 }
 
@@ -478,7 +466,7 @@ static int picdev_read(struct kvm_io_device *this,
 			printk(KERN_ERR "PIC: non byte read\n");
 		return 0;
 	}
-	pic_lock(s);
+	spin_lock(&s->lock);
 	switch (addr) {
 	case 0x20:
 	case 0x21:
@@ -492,7 +480,7 @@ static int picdev_read(struct kvm_io_device *this,
 		break;
 	}
 	*(unsigned char *)val = data;
-	pic_unlock(s);
+	spin_unlock(&s->lock);
 	return 0;
 }
 
-- 
1.6.3.3

Re: [PATCH 0/3] PIC fixes

[Marcelo Tosatti]

On Tue, Aug 04, 2009 at 03:30:26PM +0300, Gleb Natapov wrote:
> Ack notifiers called at wrong time in PIC. kvm_vcpu_kick() can be called
> under spinlock now, so code can be simplified.
> 
> Gleb Natapov (3):
>   Call kvm_vcpu_kick() inside pic spinlock
>   Call ack notifiers from PIC when guest OS acks an IRQ.
>   Replace pic_lock()/pic_unlock() with direct call to spinlock
>     functions.
> 
>  arch/x86/kvm/i8259.c |   64 +++++++++++++------------------------------------
>  arch/x86/kvm/irq.h   |    1 -
>  2 files changed, 17 insertions(+), 48 deletions(-)

Looks good to me.

Re: [PATCH 0/3] PIC fixes

[Avi Kivity]

On 08/04/2009 03:30 PM, Gleb Natapov wrote:
> Ack notifiers called at wrong time in PIC. kvm_vcpu_kick() can be called
> under spinlock now, so code can be simplified.
>
>    

Applied, thanks.

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