Re: [Qemu-devel] [PATCH v4 5/5] i8259: fix dynamically masking slave IRQs with IMR register

[Matthew Ogilvie <mmogilvi_qemu@miniinfo.net>]

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:

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?

             - Matthew Ogilvie