Re: [Qemu-devel] [RFC PATCH v2 1/6] hw: arm_gic: Fix gic_set_irq handling

[Christoffer Dall]

On Mon, Oct 14, 2013 at 03:24:43PM +0100, Peter Maydell wrote:
> On 26 September 2013 22:03, Christoffer Dall
> <address@hidden> wrote:
> > For some reason only edge-triggered or enabled level-triggered
> > interrupts would set the pending state of a raised IRQ.  This is not in
> > compliance with the specs, which indicate that the pending state is
> > separate from the enabled state, which only controls if a pending
> > interrupt is actually forwarded to the CPU interface.
> >
> > Therefore, simply always set the pending state on a rising edge, but
> > only clear the pending state of falling edge if the interrupt is level
> > triggered.
> 
> > @@ -128,11 +128,12 @@ static void gic_set_irq(void *opaque, int irq, int 
> > level)
> >
> >      if (level) {
> >          GIC_SET_LEVEL(irq, cm);
> > -        if (GIC_TEST_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)) {
> > -            DPRINTF("Set %d pending mask %x\n", irq, target);
> > -            GIC_SET_PENDING(irq, target);
> > -        }
> > +        DPRINTF("Set %d pending mask %x\n", irq, target);
> > +        GIC_SET_PENDING(irq, target);
> >      } else {
> > +        if (!GIC_TEST_TRIGGER(irq)) {
> > +            GIC_CLEAR_PENDING(irq, target);
> > +        }
> >          GIC_CLEAR_LEVEL(irq, cm);
> >      }
> >      gic_update(s);
> 
> The old logic is definitely wrong here, but this still isn't
> quite right. See the GIC v2 spec, section 4.3.8 GICD_ICPENDRn
> and specifically the circuit diagram in Figure 4.10.
> For a level triggered interrupt we mustn't clear the pending
> state on a 1->0 transition of the input if it was latched
> by the guest writing to GICD_ISPENDR.
> 
> In other words, the internal state of the GIC (ie the state
> of the latch) and the value in the ICPENDR/ISPENDR register
> on read aren't the same thing, and the bug in our current
> GIC model is that we're trying to use the .pending field
> for both purposes at the same time.
> 
So I think that's a comment that belongs more in the category of all the
things that are broken with the GICv2 emulation and should be separate
fixes.  I don't believe Linux uses this and the in-kernel GIC emulation
also doesn't keep track of this state, but the following patch should
address the issue.  Do you want me to fold such two patches into one?

-Christoffer