diff mbox

[v4,5/5] i8259: fix dynamically masking slave IRQs with IMR register

Message ID 20120905043346.GA7965@comcast.net
State New
Headers show

Commit Message

Matthew Ogilvie Sept. 5, 2012, 4:33 a.m. UTC 
On Tue, Sep 04, 2012 at 04:42:35PM +0200, Paolo Bonzini wrote:
> Il 04/09/2012 16:29, Maciej W. Rozycki ha scritto:
> >  So first of all, the *output* of the 8259A is always edge triggered, 
> > regardless of whether it's the master or one of the slaves (only one slave 
> > is used in the PC/AT architecture, but up to eight are supported; the 
> > PC/XT had none).  
> 
> I swear I read all your message :) but this seems to be the crux.  It means
> that something like this ought to fix the bug too.  Matthew, can you post
> your code or test it?

The test program source can be downloaded from
http://home.comcast.net/~mmogilvi/downloads/i8259-imr-test-2012-09-02.tar.bz2

I intend to rewrite it for kvm-unit-tests as you suggested earlier, but
likely not before this weekend.

> 
> diff --git a/hw/i8259.c b/hw/i8259.c
> index 53daf78..3dc1dff 100644
> --- a/hw/i8259.c
> +++ b/hw/i8259.c
> @@ -104,12 +104,11 @@ static void pic_update_irq(PICCommonState *s)
>      int irq;
>  
>      irq = pic_get_irq(s);
> +    qemu_irq_lower(s->int_out[0]);
>      if (irq >= 0) {
>          DPRINTF("pic%d: imr=%x irr=%x padd=%d\n",
>                  s->master ? 0 : 1, s->imr, s->irr, s->priority_add);
>          qemu_irq_raise(s->int_out[0]);
> -    } else {
> -        qemu_irq_lower(s->int_out[0]);
>      }
>  }

This doesn't work; the master still locks onto the original low to high
edge, and doesn't cancel it on the high to low edge.  I haven't had a
chance to look into or test your (or any other) KVM patches yet,
although I'll probably get to it this weekend.

According to later discussion, the main issue is actually the input
IRQ behavior on a high to low transition; hence the following fixes
both the test program and the Microport UNIX problem:

 

Perhaps it would be worth it to swap around the "if"s a little bit
to avoid the (!level) duplication, and clarify that the only difference
is in the low to high transition?

             - Matthew Ogilvie

Comments

Jan Kiszka Sept. 5, 2012, 3:43 p.m. UTC | #1 
On 2012-09-05 06:33, Matthew Ogilvie wrote:
> According to later discussion, the main issue is actually the input
> IRQ behavior on a high to low transition; hence the following fixes
> both the test program and the Microport UNIX problem:
> 
> diff --git a/hw/i8259.c b/hw/i8259.c
> index 6587666..c011787 100644
> --- a/hw/i8259.c
> +++ b/hw/i8259.c
> @@ -143,22 +143,23 @@ static void pic_set_irq(void *opaque, int irq, int level)
>      if (s->elcr & mask) {
>          /* level triggered */
>          if (level) {
>              s->irr |= mask;
>              s->last_irr |= mask;
>          } else {
>              s->irr &= ~mask;
>              s->last_irr &= ~mask;
>          }
>      } else {
>          /* edge triggered */
>          if (level) {
>              if ((s->last_irr & mask) == 0) {
>                  s->irr |= mask;
>              }
>              s->last_irr |= mask;
>          } else {
> +            s->irr &= ~mask;
>              s->last_irr &= ~mask;
>          }
>      }
>      pic_update_irq(s);
>  }
>  
> 
> Perhaps it would be worth it to swap around the "if"s a little bit
> to avoid the (!level) duplication, and clarify that the only difference
> is in the low to high transition?

Yes, refactoring would be welcome. But I would suggest to do this in two
patches, leaving the above change (+ changelog that explains the reason)
in its current clarity.

Hurray, no vmstate subsections needed! :)

Jan
diff mbox

Patch

diff --git a/hw/i8259.c b/hw/i8259.c
index 6587666..c011787 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -143,22 +143,23 @@  static void pic_set_irq(void *opaque, int irq, int level)
     if (s->elcr & mask) {
         /* level triggered */
         if (level) {
             s->irr |= mask;
             s->last_irr |= mask;
         } else {
             s->irr &= ~mask;
             s->last_irr &= ~mask;
         }
     } else {
         /* edge triggered */
         if (level) {
             if ((s->last_irr & mask) == 0) {
                 s->irr |= mask;
             }
             s->last_irr |= mask;
         } else {
+            s->irr &= ~mask;
             s->last_irr &= ~mask;
         }
     }
     pic_update_irq(s);
 }