On Mon, Oct 14, 2013 at 03:24:43PM +0100, Peter Maydell wrote: > On 26 September 2013 22:03, Christoffer Dall > <christoffer.d...@linaro.org> 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