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