| Message ID | 20240902123038.1135412-1-jan.kloetzke@kernkonzept.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/intc/arm_gic: fix spurious level triggered interrupts | expand |
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
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
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
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) {
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(-)