Patchwork [RFC,v2,1/6] hw: arm_gic: Fix gic_set_irq handling

login
register
mail settings
Submitter Christoffer Dall
Date Sept. 26, 2013, 9:03 p.m.
Message ID <1380229386-24166-2-git-send-email-christoffer.dall@linaro.org>
Download mbox | patch
Permalink /patch/278274/
State New
Headers show

Comments

Christoffer Dall - Sept. 26, 2013, 9:03 p.m.
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(-)
Peter Maydell - Oct. 14, 2013, 2:24 p.m.
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
Christoffer Dall - Oct. 23, 2013, 3:23 p.m.
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

Patch

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);