[PATCH] hw/intc/arm_gic: fix spurious level triggered interrupts

[PATCH] hw/intc/arm_gic: fix spurious level triggered interrupts

[Jan Klötzke]

Level triggered interrupts are pending when either the interrupt line
is asserted or the interrupt was made pending by a GICD_ISPENDRn write.
Making a level triggered interrupt pending by software persists until
either the interrupt is acknowledged or cleared by writing
GICD_ICPENDRn. As long as the interrupt line is asserted, the interrupt
is pending in any case.

This logic is transparently implemented in gic_test_pending(). The
function combines the "pending" irq_state flag (used for edge triggered
interrupts and software requests) and the line status (tracked in the
"level" field). Now, writing GICD_ISENABLERn incorrectly set the
pending flag if the line of a level triggered interrupt was asserted.
This keeps the interrupt pending even if the line is de-asserted after
some time.

Fix this by simply removing the code. The pending status is fully
handled by gic_test_pending() and does not need any special treatment
when enabling the level interrupt.

Signed-off-by: Jan Klötzke <jan.kloetzke@kernkonzept.com>
---
 hw/intc/arm_gic.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 806832439b..10fc9bfd14 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -1248,9 +1248,6 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
 
         for (i = 0; i < 8; i++) {
             if (value & (1 << i)) {
-                int mask =
-                    (irq < GIC_INTERNAL) ? (1 << cpu)
-                                         : GIC_DIST_TARGET(irq + i);
                 int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
 
                 if (s->security_extn && !attrs.secure &&
@@ -1263,13 +1260,6 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
                     trace_gic_enable_irq(irq + i);
                 }
                 GIC_DIST_SET_ENABLED(irq + i, cm);
-                /* If a raised level triggered IRQ enabled then mark
-                   is as pending.  */
-                if (GIC_DIST_TEST_LEVEL(irq + i, mask)
-                        && !GIC_DIST_TEST_EDGE_TRIGGER(irq + i)) {
-                    DPRINTF("Set %d pending mask %x\n", irq + i, mask);
-                    GIC_DIST_SET_PENDING(irq + i, mask);
-                }
             }
         }
     } else if (offset < 0x200) {
-- 
2.39.2

Re: [PATCH] hw/intc/arm_gic: fix spurious level triggered interrupts

[Peter Maydell]

On Mon, 2 Sept 2024 at 13:32, Jan Klötzke <jan.kloetzke@kernkonzept.com> wrote:
>
> Level triggered interrupts are pending when either the interrupt line
> is asserted or the interrupt was made pending by a GICD_ISPENDRn write.
> Making a level triggered interrupt pending by software persists until
> either the interrupt is acknowledged or cleared by writing
> GICD_ICPENDRn. As long as the interrupt line is asserted, the interrupt
> is pending in any case.
>
> This logic is transparently implemented in gic_test_pending(). The
> function combines the "pending" irq_state flag (used for edge triggered
> interrupts and software requests) and the line status (tracked in the
> "level" field). Now, writing GICD_ISENABLERn incorrectly set the
> pending flag if the line of a level triggered interrupt was asserted.
> This keeps the interrupt pending even if the line is de-asserted after
> some time.
>
> Fix this by simply removing the code. The pending status is fully
> handled by gic_test_pending() and does not need any special treatment
> when enabling the level interrupt.
>
> Signed-off-by: Jan Klötzke <jan.kloetzke@kernkonzept.com>

Thanks for this patch. I agree that this is wrong for the
GICv2 -- I think this is a bit we missed in commit 8d999995e45c
back in 2013 where we fixed most other places that were not
correctly making this distinction of "pending because of
ISPENDR write" and "pending because level triggered and
line is held high".

However I think for consistency with that commit, we should
retain the current behaviour here for the s->revision == REV_11MPCORE
case. (This is basically saying "we don't really know exactly
how the 11MPCore GIC behaved and we don't much care to try to
find out, so leave it alone", which is the stance we were
already taking in 2013...) In particular, notice that
gic_test_pending() only does the "pending if level triggered
and held high" logic for the not-REV_11MPCORE case.

thanks
-- PMM

Re: [PATCH] hw/intc/arm_gic: fix spurious level triggered interrupts

[Jan Klötzke]

On Fri, 2024-09-06 at 13:50 +0100, Peter Maydell wrote:
> On Mon, 2 Sept 2024 at 13:32, Jan Klötzke <jan.kloetzke@kernkonzept.com> wrote:
> > 
> > Level triggered interrupts are pending when either the interrupt line
> > is asserted or the interrupt was made pending by a GICD_ISPENDRn write.
> > Making a level triggered interrupt pending by software persists until
> > either the interrupt is acknowledged or cleared by writing
> > GICD_ICPENDRn. As long as the interrupt line is asserted, the interrupt
> > is pending in any case.
> > 
> > This logic is transparently implemented in gic_test_pending(). The
> > function combines the "pending" irq_state flag (used for edge triggered
> > interrupts and software requests) and the line status (tracked in the
> > "level" field). Now, writing GICD_ISENABLERn incorrectly set the
> > pending flag if the line of a level triggered interrupt was asserted.
> > This keeps the interrupt pending even if the line is de-asserted after
> > some time.
> > 
> > Fix this by simply removing the code. The pending status is fully
> > handled by gic_test_pending() and does not need any special treatment
> > when enabling the level interrupt.
> > 
> > Signed-off-by: Jan Klötzke <jan.kloetzke@kernkonzept.com>
> 
> Thanks for this patch. I agree that this is wrong for the
> GICv2 -- I think this is a bit we missed in commit 8d999995e45c
> back in 2013 where we fixed most other places that were not
> correctly making this distinction of "pending because of
> ISPENDR write" and "pending because level triggered and
> line is held high".
> 
> However I think for consistency with that commit, we should
> retain the current behaviour here for the s->revision == REV_11MPCORE
> case. (This is basically saying "we don't really know exactly
> how the 11MPCore GIC behaved and we don't much care to try to
> find out, so leave it alone", which is the stance we were
> already taking in 2013...) In particular, notice that
> gic_test_pending() only does the "pending if level triggered
> and held high" logic for the not-REV_11MPCORE case.

Right. Thanks for catching this. I'll send a V2 shortly.

Actually, I tried to make sense out of the ARM11 MPCore TRM but gave
up. At least, I could not come with a consistent idea how the hardware
is supposed to behave. Keeping the old behavior really looks like the
most sensible option here.


Thanks,
Jan

-- 
Jan Klötzke, jan.kloetzke@kernkonzept.com, +49 351 41883238

Kernkonzept GmbH, Dresden, Germany, HRB 31129, CEO Dr.-Ing. Michael
Hohmuth

Re: [PATCH] hw/intc/arm_gic: fix spurious level triggered interrupts

[Peter Maydell]

On Wed, 11 Sept 2024 at 11:54, Jan Klötzke <jan.kloetzke@kernkonzept.com> wrote:
>
> On Fri, 2024-09-06 at 13:50 +0100, Peter Maydell wrote:
> > On Mon, 2 Sept 2024 at 13:32, Jan Klötzke <jan.kloetzke@kernkonzept.com> wrote:
> > >
> > > Level triggered interrupts are pending when either the interrupt line
> > > is asserted or the interrupt was made pending by a GICD_ISPENDRn write.
> > > Making a level triggered interrupt pending by software persists until
> > > either the interrupt is acknowledged or cleared by writing
> > > GICD_ICPENDRn. As long as the interrupt line is asserted, the interrupt
> > > is pending in any case.
> > >
> > > This logic is transparently implemented in gic_test_pending(). The
> > > function combines the "pending" irq_state flag (used for edge triggered
> > > interrupts and software requests) and the line status (tracked in the
> > > "level" field). Now, writing GICD_ISENABLERn incorrectly set the
> > > pending flag if the line of a level triggered interrupt was asserted.
> > > This keeps the interrupt pending even if the line is de-asserted after
> > > some time.
> > >
> > > Fix this by simply removing the code. The pending status is fully
> > > handled by gic_test_pending() and does not need any special treatment
> > > when enabling the level interrupt.
> > >
> > > Signed-off-by: Jan Klötzke <jan.kloetzke@kernkonzept.com>
> >
> > Thanks for this patch. I agree that this is wrong for the
> > GICv2 -- I think this is a bit we missed in commit 8d999995e45c
> > back in 2013 where we fixed most other places that were not
> > correctly making this distinction of "pending because of
> > ISPENDR write" and "pending because level triggered and
> > line is held high".
> >
> > However I think for consistency with that commit, we should
> > retain the current behaviour here for the s->revision == REV_11MPCORE
> > case. (This is basically saying "we don't really know exactly
> > how the 11MPCore GIC behaved and we don't much care to try to
> > find out, so leave it alone", which is the stance we were
> > already taking in 2013...) In particular, notice that
> > gic_test_pending() only does the "pending if level triggered
> > and held high" logic for the not-REV_11MPCORE case.
>
> Right. Thanks for catching this. I'll send a V2 shortly.
>
> Actually, I tried to make sense out of the ARM11 MPCore TRM but gave
> up. At least, I could not come with a consistent idea how the hardware
> is supposed to behave. Keeping the old behavior really looks like the
> most sensible option here.

Yeah. To be honest, I wouldn't be surprised if actually the
11mpcore and the GICv2 behave the same and the behaviour
we have under REV_11MPCORE is just "what the person who
implemented the QEMU model guessed at based on the rather
nonspecific 11mpcore TRM documentation". But at this late date
there's really no benefit in trying to find out :-)

thanks
-- PMM