diff mbox

KVM: PIC: call ack notifiers for irqs that are dropped form irr

Message ID 20120717115910.GG6345@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Gleb Natapov July 17, 2012, 11:59 a.m. UTC 
After commit 242ec97c358256 PIT interrupts are no longer delivered after
PIC reset. It happens because PIT injects interrupt only if previous one
was acked, but since on PIC reset it is dropped from irr it will never
be delivered and hence acknowledged. Fix that by calling ack notifier on
PIC reset.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
--
			Gleb.
--
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

Comments

Marcelo Tosatti July 20, 2012, 11:58 a.m. UTC | #1 
On Tue, Jul 17, 2012 at 02:59:11PM +0300, Gleb Natapov wrote:
> After commit 242ec97c358256 PIT interrupts are no longer delivered after
> PIC reset. It happens because PIT injects interrupt only if previous one
> was acked, but since on PIC reset it is dropped from irr it will never
> be delivered and hence acknowledged. Fix that by calling ack notifier on
> PIC reset.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> index 81cf4fa..f09e790 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -305,6 +305,7 @@ static void pic_ioport_write(void *opaque, u32 addr, u32 val)
>  	addr &= 1;
>  	if (addr == 0) {
>  		if (val & 0x10) {
> +			u8 edge_irr = s->irr & ~s->elcr;
>  			s->init4 = val & 1;
>  			s->last_irr = 0;
>  			s->irr &= s->elcr;
> @@ -322,6 +323,9 @@ static void pic_ioport_write(void *opaque, u32 addr, u32 val)
>  			if (val & 0x08)
>  				pr_pic_unimpl(
>  					"level sensitive irq not supported");
> +			for (irq = 0; irq < PIC_NUM_PINS/2; irq++)
> +				if (edge_irr & (1 << irq))
> +					pic_clear_isr(s, irq);
>  		} else if (val & 0x08) {
>  			if (val & 0x04)
>  				s->poll = 1;
> --
> 			Gleb.

Can modify kvm_pic_reset (currently unused BTW) to conform to
9ed049c3b6230b6898 ? It checks for APIC handling interrupts
before acking.

--
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 Natapov July 20, 2012, 1:20 p.m. UTC | #2 
On Fri, Jul 20, 2012 at 08:58:56AM -0300, Marcelo Tosatti wrote:
> On Tue, Jul 17, 2012 at 02:59:11PM +0300, Gleb Natapov wrote:
> > After commit 242ec97c358256 PIT interrupts are no longer delivered after
> > PIC reset. It happens because PIT injects interrupt only if previous one
> > was acked, but since on PIC reset it is dropped from irr it will never
> > be delivered and hence acknowledged. Fix that by calling ack notifier on
> > PIC reset.
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> > index 81cf4fa..f09e790 100644
> > --- a/arch/x86/kvm/i8259.c
> > +++ b/arch/x86/kvm/i8259.c
> > @@ -305,6 +305,7 @@ static void pic_ioport_write(void *opaque, u32 addr, u32 val)
> >  	addr &= 1;
> >  	if (addr == 0) {
> >  		if (val & 0x10) {
> > +			u8 edge_irr = s->irr & ~s->elcr;
> >  			s->init4 = val & 1;
> >  			s->last_irr = 0;
> >  			s->irr &= s->elcr;
> > @@ -322,6 +323,9 @@ static void pic_ioport_write(void *opaque, u32 addr, u32 val)
> >  			if (val & 0x08)
> >  				pr_pic_unimpl(
> >  					"level sensitive irq not supported");
> > +			for (irq = 0; irq < PIC_NUM_PINS/2; irq++)
> > +				if (edge_irr & (1 << irq))
> > +					pic_clear_isr(s, irq);
> >  		} else if (val & 0x08) {
> >  			if (val & 0x04)
> >  				s->poll = 1;
> > --
> > 			Gleb.
> 
> Can modify kvm_pic_reset (currently unused BTW) to conform to
> 9ed049c3b6230b6898 ? It checks for APIC handling interrupts
> before acking.
Since it is not used we can make it do anything. But preferably in
speared patch otherwise bug fix will be obfuscated by code move.

--
			Gleb.
--
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 Natapov July 24, 2012, 12:35 p.m. UTC | #3 
Ping. Is some additional work expected from me before this is applied?

On Tue, Jul 17, 2012 at 02:59:11PM +0300, Gleb Natapov wrote:
> After commit 242ec97c358256 PIT interrupts are no longer delivered after
> PIC reset. It happens because PIT injects interrupt only if previous one
> was acked, but since on PIC reset it is dropped from irr it will never
> be delivered and hence acknowledged. Fix that by calling ack notifier on
> PIC reset.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> index 81cf4fa..f09e790 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -305,6 +305,7 @@ static void pic_ioport_write(void *opaque, u32 addr, u32 val)
>  	addr &= 1;
>  	if (addr == 0) {
>  		if (val & 0x10) {
> +			u8 edge_irr = s->irr & ~s->elcr;
>  			s->init4 = val & 1;
>  			s->last_irr = 0;
>  			s->irr &= s->elcr;
> @@ -322,6 +323,9 @@ static void pic_ioport_write(void *opaque, u32 addr, u32 val)
>  			if (val & 0x08)
>  				pr_pic_unimpl(
>  					"level sensitive irq not supported");
> +			for (irq = 0; irq < PIC_NUM_PINS/2; irq++)
> +				if (edge_irr & (1 << irq))
> +					pic_clear_isr(s, irq);
>  		} else if (val & 0x08) {
>  			if (val & 0x04)
>  				s->poll = 1;
> --
> 			Gleb.
> --
> 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.
--
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
Marcelo Tosatti July 24, 2012, 12:40 p.m. UTC | #4 
On Tue, Jul 24, 2012 at 03:35:14PM +0300, Gleb Natapov wrote:
> Ping. Is some additional work expected from me before this is applied?

Still not clear to me why its safe to skip kvm_apic_accept_pic_intr check
before calling ack notifier. 

> On Tue, Jul 17, 2012 at 02:59:11PM +0300, Gleb Natapov wrote:
> > After commit 242ec97c358256 PIT interrupts are no longer delivered after
> > PIC reset. It happens because PIT injects interrupt only if previous one
> > was acked, but since on PIC reset it is dropped from irr it will never
> > be delivered and hence acknowledged. Fix that by calling ack notifier on
> > PIC reset.
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> > index 81cf4fa..f09e790 100644
> > --- a/arch/x86/kvm/i8259.c
> > +++ b/arch/x86/kvm/i8259.c
> > @@ -305,6 +305,7 @@ static void pic_ioport_write(void *opaque, u32 addr, u32 val)
> >  	addr &= 1;
> >  	if (addr == 0) {
> >  		if (val & 0x10) {
> > +			u8 edge_irr = s->irr & ~s->elcr;
> >  			s->init4 = val & 1;
> >  			s->last_irr = 0;
> >  			s->irr &= s->elcr;
> > @@ -322,6 +323,9 @@ static void pic_ioport_write(void *opaque, u32 addr, u32 val)
> >  			if (val & 0x08)
> >  				pr_pic_unimpl(
> >  					"level sensitive irq not supported");
> > +			for (irq = 0; irq < PIC_NUM_PINS/2; irq++)
> > +				if (edge_irr & (1 << irq))
> > +					pic_clear_isr(s, irq);
> >  		} else if (val & 0x08) {
> >  			if (val & 0x04)
> >  				s->poll = 1;
> > --
> > 			Gleb.
> > --
--
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 Natapov July 24, 2012, 1:15 p.m. UTC | #5 
On Tue, Jul 24, 2012 at 09:40:26AM -0300, Marcelo Tosatti wrote:
> 
> On Tue, Jul 24, 2012 at 03:35:14PM +0300, Gleb Natapov wrote:
> > Ping. Is some additional work expected from me before this is applied?
> 
> Still not clear to me why its safe to skip kvm_apic_accept_pic_intr check
> before calling ack notifier. 
> 
I do not think it is actually.

> > On Tue, Jul 17, 2012 at 02:59:11PM +0300, Gleb Natapov wrote:
> > > After commit 242ec97c358256 PIT interrupts are no longer delivered after
> > > PIC reset. It happens because PIT injects interrupt only if previous one
> > > was acked, but since on PIC reset it is dropped from irr it will never
> > > be delivered and hence acknowledged. Fix that by calling ack notifier on
> > > PIC reset.
> > > 
> > > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> > > index 81cf4fa..f09e790 100644
> > > --- a/arch/x86/kvm/i8259.c
> > > +++ b/arch/x86/kvm/i8259.c
> > > @@ -305,6 +305,7 @@ static void pic_ioport_write(void *opaque, u32 addr, u32 val)
> > >  	addr &= 1;
> > >  	if (addr == 0) {
> > >  		if (val & 0x10) {
> > > +			u8 edge_irr = s->irr & ~s->elcr;
> > >  			s->init4 = val & 1;
> > >  			s->last_irr = 0;
> > >  			s->irr &= s->elcr;
> > > @@ -322,6 +323,9 @@ static void pic_ioport_write(void *opaque, u32 addr, u32 val)
> > >  			if (val & 0x08)
> > >  				pr_pic_unimpl(
> > >  					"level sensitive irq not supported");
> > > +			for (irq = 0; irq < PIC_NUM_PINS/2; irq++)
> > > +				if (edge_irr & (1 << irq))
> > > +					pic_clear_isr(s, irq);
> > >  		} else if (val & 0x08) {
> > >  			if (val & 0x04)
> > >  				s->poll = 1;
> > > --
> > > 			Gleb.
> > > --

--
			Gleb.
--
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
diff mbox

Patch

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 81cf4fa..f09e790 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -305,6 +305,7 @@  static void pic_ioport_write(void *opaque, u32 addr, u32 val)
 	addr &= 1;
 	if (addr == 0) {
 		if (val & 0x10) {
+			u8 edge_irr = s->irr & ~s->elcr;
 			s->init4 = val & 1;
 			s->last_irr = 0;
 			s->irr &= s->elcr;
@@ -322,6 +323,9 @@  static void pic_ioport_write(void *opaque, u32 addr, u32 val)
 			if (val & 0x08)
 				pr_pic_unimpl(
 					"level sensitive irq not supported");
+			for (irq = 0; irq < PIC_NUM_PINS/2; irq++)
+				if (edge_irr & (1 << irq))
+					pic_clear_isr(s, irq);
 		} else if (val & 0x08) {
 			if (val & 0x04)
 				s->poll = 1;