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

["Jan Klötzke" <jan.kloetzke@kernkonzept.com>]

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