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