Re: [Qemu-devel] [RFC PATCH v3 01/10] hw: arm_gic: Fix gic_set_irq handling

[Christoffer Dall <christoffer.dall@linaro.org>]

On Thu, Nov 28, 2013 at 04:17:43PM +0000, Peter Maydell wrote:
> On 19 November 2013 06:18, 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.
> >
> > 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(-)
> >
> > 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);
> 
> So I think this is a correct change in the sense that
> it's fixing the behaviour of this function. However
> we seem to get our pending behaviour for level triggered
> interrupts wrong in several places:
>  * here
>  * gic_acknowledge_irq (which you fix in a later patch)
>  * gic_complete_irq, which currently says "enabled
>    level triggered still-raised interrupts should be
>    remarked as pending"
> 
> This feels to me like a cluster of errors which have come
> from somebody's misreading of the spec and which probably
> combine to produce a coherent not-too-far-from-correct
> result, and which we should therefore fix all at once rather
> than only partially.
> 
Fair enough, I'll try and combine these.

-Christoffer