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

[Peter Maydell]

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.

-- PMM