| Message ID | 1380229386-24166-2-git-send-email-christoffer.dall@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 26 September 2013 22:03, Christoffer Dall <christoffer.dall@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. -- PMM
On Mon, Oct 14, 2013 at 03:24:43PM +0100, Peter Maydell wrote: > On 26 September 2013 22:03, Christoffer Dall > <christoffer.dall@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
diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index d431b7a..c7a24d5 100644 --- a/hw/intc/arm_gic.c +++ b/hw/intc/arm_gic.c @@ -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);
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. Changelog [v2]: - Fix bisection issue, by not using gic_clear_pending yet. Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> --- hw/intc/arm_gic.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)