| Message ID | 1384841896-19566-2-git-send-email-christoffer.dall@linaro.org |
|---|---|
| State | New |
| Headers | show |
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. thanks -- PMM
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). thanks -- PMM
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
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
On 19 December 2013 05:49, Christoffer Dall <christoffer.dall@linaro.org> wrote: > On Thu, Nov 28, 2013 at 05:43:54PM +0000, Peter Maydell wrote: >> 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. That is my point -- the 11MPCore docs say that pending works differently. > That being said, we are absolutely sure that support the 11MPCore is > still needed? I can't just rip it all out of QEMU, really, though it does count as one of our less used CPUs I suspect. > 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? My initial thought would be either to have if statements at the relevant points (which is how we've handled 11mpcore differences so far), or to bite the bullet and reflect the difference in the QOM class structure so we can use QOM methods [ie function pointers in the class struct]. thanks -- PMM
On Thu, Dec 19, 2013 at 7:03 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 19 December 2013 05:49, Christoffer Dall <christoffer.dall@linaro.org> wrote: >> On Thu, Nov 28, 2013 at 05:43:54PM +0000, Peter Maydell wrote: >>> 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. > > That is my point -- the 11MPCore docs say that pending works > differently. > >> That being said, we are absolutely sure that support the 11MPCore is >> still needed? > > I can't just rip it all out of QEMU, really, though it does count > as one of our less used CPUs I suspect. > >> 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? > > My initial thought would be either to have if statements at the > relevant points (which is how we've handled 11mpcore > differences so far), or to bite the bullet and reflect the > difference in the QOM class structure so we can use > QOM methods [ie function pointers in the class struct]. > Even in the "if" approach its probably best to put the "is-11mpcore" or "version" property in the class structure. So I think you want the QOM class structure both ways. Regards, Peter > thanks > -- PMM >
On 19 December 2013 13:49, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Thu, Dec 19, 2013 at 7:03 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> My initial thought would be either to have if statements at the >> relevant points (which is how we've handled 11mpcore >> differences so far), or to bite the bullet and reflect the >> difference in the QOM class structure so we can use >> QOM methods [ie function pointers in the class struct]. >> > > Even in the "if" approach its probably best to put the "is-11mpcore" > or "version" property in the class structure. So I think you want the > QOM class structure both ways. We have precedent elsewhere for having a "revision" property in the object struct rather than having subclasses per class, don't we? (arm_gic already has such a property, it's the 'revision' field.) Properties can't go in the class struct because by definition they can be set per-instance by the creator of the object. thanks -- PMM
On Thu, Dec 19, 2013 at 11:59 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 19 December 2013 13:49, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> On Thu, Dec 19, 2013 at 7:03 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> My initial thought would be either to have if statements at the >>> relevant points (which is how we've handled 11mpcore >>> differences so far), or to bite the bullet and reflect the >>> difference in the QOM class structure so we can use >>> QOM methods [ie function pointers in the class struct]. >>> >> >> Even in the "if" approach its probably best to put the "is-11mpcore" >> or "version" property in the class structure. So I think you want the >> QOM class structure both ways. > > We have precedent elsewhere for having a "revision" property > in the object struct rather than having subclasses per class, don't > we? (arm_gic already has such a property, it's the 'revision' field.) So given its already there, can you just cheat and get the revs right and if on them? Back to longer term though, I'm thinking of the sysbus EHCI as the modern precedent. The small diffs between the incompatible implementations are determined at class init time via which concrete subclass you instantiated. > > Properties can't go in the class struct because by definition > they can be set per-instance by the creator of the object. > You have multiple inherited classes. TYPE_ARM_GIC defines the class property (maybe im looking for a better word than property here), and TYPE_11MPCORE_GIC and friends set it in their class_init. Same as for QOM abstract functions, just instead it's an abstract constant. Regards, Peter > thanks > -- PMM >
On 19 December 2013 14:26, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Thu, Dec 19, 2013 at 11:59 PM, Peter Maydell > <peter.maydell@linaro.org> wrote: >> On 19 December 2013 13:49, Peter Crosthwaite >> <peter.crosthwaite@xilinx.com> wrote: >>> On Thu, Dec 19, 2013 at 7:03 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>>> My initial thought would be either to have if statements at the >>>> relevant points (which is how we've handled 11mpcore >>>> differences so far), or to bite the bullet and reflect the >>>> difference in the QOM class structure so we can use >>>> QOM methods [ie function pointers in the class struct]. >>>> >>> >>> Even in the "if" approach its probably best to put the "is-11mpcore" >>> or "version" property in the class structure. So I think you want the >>> QOM class structure both ways. >> >> We have precedent elsewhere for having a "revision" property >> in the object struct rather than having subclasses per class, don't >> we? (arm_gic already has such a property, it's the 'revision' field.) > > So given its already there, can you just cheat and get the revs right > and if on them? That was the proposal, yes :-) > Back to longer term though, I'm thinking of the sysbus EHCI as the > modern precedent. The small diffs between the incompatible > implementations are determined at class init time via which concrete > subclass you instantiated. Yeah, we could certainly do that. I was just hoping to avoid doing a rework that created a pile of subclasses just for another 11mpcore weirdness :-) -- PMM
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(-)