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

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

[Jan Klötzke]

On GICv2 and later, 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.

Apparently, the 11MPCore behaves differently. A level triggered
interrupt that is disabled does not get pending. Thus we have to retain
the get-pending-on-enable logic just for this model. For GICv2 and
later, 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>
---
v2:
  * Keep existing logic for 11MPCore

 hw/intc/arm_gic.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 806832439b..4333d52ee2 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -1264,8 +1264,9 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
                 }
                 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)
+                   is as pending on 11MPCore.  */
+                if (s->revision == REV_11MPCORE
+                        && 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);
-- 
2.39.2

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

[Peter Maydell]

On Wed, 11 Sept 2024 at 12:49, Jan Klötzke <jan.kloetzke@kernkonzept.com> wrote:
>
> On GICv2 and later, 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.
>
> Apparently, the 11MPCore behaves differently. A level triggered
> interrupt that is disabled does not get pending. Thus we have to retain
> the get-pending-on-enable logic just for this model. For GICv2 and
> later, 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>
> ---
> v2:
>   * Keep existing logic for 11MPCore
>
>  hw/intc/arm_gic.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 806832439b..4333d52ee2 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -1264,8 +1264,9 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>                  }
>                  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)
> +                   is as pending on 11MPCore.  */
> +                if (s->revision == REV_11MPCORE
> +                        && 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);

Applied to target-arm.next, thanks. (I added some extra text
to the commit message to give a bit more of the historical
backstory about what's going on with the 11MPCore GIC model.)

-- PMM