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 05:43:54PM +0000, Peter Maydell wrote:
> On 28 November 2013 16:17, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 19 November 2013 06:18, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> > 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.
> 
> The other possibility is that it's a correct implementation
> of 11MPCore GIC semantics -- the documentation of the
> 11MPCore definitely says that level triggered interrupts
> go from Pending to Active and can't be Active+Pending
> unless software messes with the GIC state. So either
> the docs are actively wrong for 11MPCore or it behaves
> differently from GICv1 and v2 here (my guess would be
> the latter, in which case we need to support both flavours).
> 
A correct implementation?  I don't see how, unless the pending/level
fields are used in some obscure different way for the 11MPCore than for
GICv2.0.  For the 11MPCore, shouldn't it be:

if (level) {
	GIC_SET_LEVEL(irq, cm);
	if (GIC_TEST_TRIGGER || !GIC_TEST_ACTIVE(irq, cm)) {
		GIC_SET_PENDING(irq, target);
	}
} else {
	GIC_CLEAR_LEVEL(irq, cm)
}

and then the acknowledge could should check if the level is high and we
are acking an active interrupt, make it pending instead of inactive?


That being said, we are absolutely sure that support the 11MPCore is
still needed?

I would probably go the route of creating some structs with function
pointers in them for things like the ack or raise etc. which have
different semantics for the two versions and have separate functions to
reduce the branching in each funciton.  What do you think?

-Christoffer