1. Patchew
  2. QEMU
  3. View series

[PATCH] hw/intc/aspeed: Drop stale pending interrupts

Jian Zhang posted 1 patch 1 month, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260612060857.1842819-1-zhangjian.3032@bytedance.com
Maintainers: "Cédric Le Goater" <clg@kaod.org>, Peter Maydell <peter.maydell@linaro.org>, Steven Lee <steven_lee@aspeedtech.com>, Troy Lee <leetroy@gmail.com>, Jamin Lin <jamin_lin@aspeedtech.com>, Kane Chen <kane_chen@aspeedtech.com>, Andrew Jeffery <andrew@codeconstruct.com.au>, Joel Stanley <joel@jms.id.au>
hw/intc/aspeed_intc.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
[PATCH] hw/intc/aspeed: Drop stale pending interrupts
Posted by Jian Zhang 1 month, 2 weeks ago 
The Aspeed INTC records an interrupt source in the pending bitmap when
the source is masked or another status bit is still being handled.  When
the guest later clears the status register, the model promotes all saved
pending bits back to status unconditionally.

This is not correct for level-triggered sources.  A source can deassert
while another source connected to the same OR gate keeps the aggregated
INTC line asserted.  Promoting the stale bit later makes the guest demux
a child interrupt whose device status has already been cleared.

This is visible on AST2700 I2C, where the I2C buses are aggregated
through INTCIO GICINT194 before reaching the GIC.  A stale I2C source bit
can be promoted back to the INTCIO status register, causing Linux to run
the corresponding I2C ISR with an empty I2C interrupt status register.
For example, the Linux aspeed-i2c debug ring shows a transfer that first
receives a valid status interrupt, then receives a spurious ISR with both
isr and raw status equal to zero.  The zero-status ISR clears the saved
command error and the transfer completes with ret=0:

  event=start isr=0x00000000 raw=0x00000000 cmd_err=0 msgs_idx=0
  event=isr   isr=0x00010011 raw=0x00010011 cmd_err=0 msgs_idx=0
  event=isr   isr=0x00000000 raw=0x00000000 cmd_err=1 msgs_idx=1
  event=complete ret=0 cmd_err=0 msgs_idx=1

A normal command can then be reported as zero transferred messages, which
is converted to -EIO by Linux i2c_smbus_xfer_emulated().  The race is
more likely when multiple I2C buses are accessed concurrently.

Drop pending bits that no longer correspond to an asserted and enabled
source before they can be promoted back to status.

Signed-off-by: Jian Zhang <zhangjian.3032@bytedance.com>
---
 hw/intc/aspeed_intc.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c
index 5a36fff5204..ab80c239bc1 100644
--- a/hw/intc/aspeed_intc.c
+++ b/hw/intc/aspeed_intc.c
@@ -107,6 +107,27 @@ static const AspeedINTCIRQ *aspeed_intc_get_irq(AspeedINTCClass *aic,
     g_assert_not_reached();
 }
 
+static uint32_t aspeed_intc_orgate_levels(AspeedINTCState *s, int inpin_idx)
+{
+    AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
+    uint32_t levels = 0;
+    int i;
+
+    for (i = 0; i < aic->num_lines && i < 32; i++) {
+        if (s->orgates[inpin_idx].levels[i]) {
+            levels |= BIT(i);
+        }
+    }
+
+    return levels;
+}
+
+static void aspeed_intc_drop_stale_pending(AspeedINTCState *s, int inpin_idx)
+{
+    s->pending[inpin_idx] &= aspeed_intc_orgate_levels(s, inpin_idx) &
+                             s->enable[inpin_idx];
+}
+
 /*
  * Update the state of an interrupt controller pin by setting
  * the specified output pin to the given level.
@@ -231,6 +252,8 @@ static void aspeed_intc_set_irq(void *opaque, int irq, int level)
     trace_aspeed_intc_set_irq(name, inpin_idx, level);
     enable = s->enable[inpin_idx];
 
+    aspeed_intc_drop_stale_pending(s, inpin_idx);
+
     if (!level) {
         return;
     }
@@ -343,6 +366,7 @@ static void aspeed_intc_status_handler(AspeedINTCState *s, hwaddr offset,
     /* All source ISR execution are done */
     if (!s->regs[reg]) {
         trace_aspeed_intc_all_isr_done(name, inpin_idx);
+        aspeed_intc_drop_stale_pending(s, inpin_idx);
         if (s->pending[inpin_idx]) {
             /*
              * handle pending source interrupt
@@ -402,6 +426,7 @@ static void aspeed_intc_status_handler_multi_outpins(AspeedINTCState *s,
         /* All source ISR executions are done from a specific bit */
         if (data & BIT(i)) {
             trace_aspeed_intc_all_isr_done_bit(name, inpin_idx, i);
+            aspeed_intc_drop_stale_pending(s, inpin_idx);
             if (s->pending[inpin_idx] & BIT(i)) {
                 /*
                  * Handle pending source interrupt.
-- 
2.20.1
Re: [PATCH] hw/intc/aspeed: Drop stale pending interrupts
Posted by Cédric Le Goater 3 weeks, 5 days ago 
On 6/12/26 08:08, Jian Zhang wrote:
> The Aspeed INTC records an interrupt source in the pending bitmap when
> the source is masked or another status bit is still being handled.  When
> the guest later clears the status register, the model promotes all saved
> pending bits back to status unconditionally.
> 
> This is not correct for level-triggered sources.  A source can deassert
> while another source connected to the same OR gate keeps the aggregated
> INTC line asserted.  Promoting the stale bit later makes the guest demux
> a child interrupt whose device status has already been cleared.
> 
> This is visible on AST2700 I2C, where the I2C buses are aggregated
> through INTCIO GICINT194 before reaching the GIC.  A stale I2C source bit
> can be promoted back to the INTCIO status register, causing Linux to run
> the corresponding I2C ISR with an empty I2C interrupt status register.
> For example, the Linux aspeed-i2c debug ring shows a transfer that first
> receives a valid status interrupt, then receives a spurious ISR with both
> isr and raw status equal to zero.  The zero-status ISR clears the saved
> command error and the transfer completes with ret=0:
> 
>    event=start isr=0x00000000 raw=0x00000000 cmd_err=0 msgs_idx=0
>    event=isr   isr=0x00010011 raw=0x00010011 cmd_err=0 msgs_idx=0
>    event=isr   isr=0x00000000 raw=0x00000000 cmd_err=1 msgs_idx=1
>    event=complete ret=0 cmd_err=0 msgs_idx=1
> 
> A normal command can then be reported as zero transferred messages, which
> is converted to -EIO by Linux i2c_smbus_xfer_emulated().  The race is
> more likely when multiple I2C buses are accessed concurrently.
> 
> Drop pending bits that no longer correspond to an asserted and enabled
> source before they can be promoted back to status.
> 
> Signed-off-by: Jian Zhang <zhangjian.3032@bytedance.com>


This looks correct to me.

Jamin, could you take a look please ?

Thanks,

C.



> ---
>   hw/intc/aspeed_intc.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c
> index 5a36fff5204..ab80c239bc1 100644
> --- a/hw/intc/aspeed_intc.c
> +++ b/hw/intc/aspeed_intc.c
> @@ -107,6 +107,27 @@ static const AspeedINTCIRQ *aspeed_intc_get_irq(AspeedINTCClass *aic,
>       g_assert_not_reached();
>   }
>   
> +static uint32_t aspeed_intc_orgate_levels(AspeedINTCState *s, int inpin_idx)
> +{
> +    AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
> +    uint32_t levels = 0;
> +    int i;
> +
> +    for (i = 0; i < aic->num_lines && i < 32; i++) {
> +        if (s->orgates[inpin_idx].levels[i]) {
> +            levels |= BIT(i);
> +        }
> +    }
> +
> +    return levels;
> +}
> +
> +static void aspeed_intc_drop_stale_pending(AspeedINTCState *s, int inpin_idx)
> +{
> +    s->pending[inpin_idx] &= aspeed_intc_orgate_levels(s, inpin_idx) &
> +                             s->enable[inpin_idx];
> +}
> +
>   /*
>    * Update the state of an interrupt controller pin by setting
>    * the specified output pin to the given level.
> @@ -231,6 +252,8 @@ static void aspeed_intc_set_irq(void *opaque, int irq, int level)
>       trace_aspeed_intc_set_irq(name, inpin_idx, level);
>       enable = s->enable[inpin_idx];
>   
> +    aspeed_intc_drop_stale_pending(s, inpin_idx);
> +
>       if (!level) {
>           return;
>       }
> @@ -343,6 +366,7 @@ static void aspeed_intc_status_handler(AspeedINTCState *s, hwaddr offset,
>       /* All source ISR execution are done */
>       if (!s->regs[reg]) {
>           trace_aspeed_intc_all_isr_done(name, inpin_idx);
> +        aspeed_intc_drop_stale_pending(s, inpin_idx);
>           if (s->pending[inpin_idx]) {
>               /*
>                * handle pending source interrupt
> @@ -402,6 +426,7 @@ static void aspeed_intc_status_handler_multi_outpins(AspeedINTCState *s,
>           /* All source ISR executions are done from a specific bit */
>           if (data & BIT(i)) {
>               trace_aspeed_intc_all_isr_done_bit(name, inpin_idx, i);
> +            aspeed_intc_drop_stale_pending(s, inpin_idx);
>               if (s->pending[inpin_idx] & BIT(i)) {
>                   /*
>                    * Handle pending source interrupt.
RE: [PATCH] hw/intc/aspeed: Drop stale pending interrupts
Posted by Jamin Lin 3 weeks, 3 days ago 
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Tuesday, June 30, 2026 11:50 AM
> To: Jian Zhang <zhangjian.3032@bytedance.com>; Peter Maydell
> <peter.maydell@linaro.org>; Steven Lee <steven_lee@aspeedtech.com>; Troy
> Lee <leetroy@gmail.com>; Jamin Lin <jamin_lin@aspeedtech.com>; Kane
> Chen <kane_chen@aspeedtech.com>; Andrew Jeffery
> <andrew@codeconstruct.com.au>; Joel Stanley <joel@jms.id.au>;
> qemu-arm@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH] hw/intc/aspeed: Drop stale pending interrupts
> 
> 
> On 6/12/26 08:08, Jian Zhang wrote:
> > The Aspeed INTC records an interrupt source in the pending bitmap when
> > the source is masked or another status bit is still being handled.
> > When the guest later clears the status register, the model promotes
> > all saved pending bits back to status unconditionally.
> >
> > This is not correct for level-triggered sources.  A source can
> > deassert while another source connected to the same OR gate keeps the
> > aggregated INTC line asserted.  Promoting the stale bit later makes
> > the guest demux a child interrupt whose device status has already been
> cleared.
> >
> > This is visible on AST2700 I2C, where the I2C buses are aggregated
> > through INTCIO GICINT194 before reaching the GIC.  A stale I2C source
> > bit can be promoted back to the INTCIO status register, causing Linux
> > to run the corresponding I2C ISR with an empty I2C interrupt status register.
> > For example, the Linux aspeed-i2c debug ring shows a transfer that
> > first receives a valid status interrupt, then receives a spurious ISR
> > with both isr and raw status equal to zero.  The zero-status ISR
> > clears the saved command error and the transfer completes with ret=0:
> >
> >    event=start isr=0x00000000 raw=0x00000000 cmd_err=0 msgs_idx=0
> >    event=isr   isr=0x00010011 raw=0x00010011 cmd_err=0 msgs_idx=0
> >    event=isr   isr=0x00000000 raw=0x00000000 cmd_err=1 msgs_idx=1
> >    event=complete ret=0 cmd_err=0 msgs_idx=1
> >
> > A normal command can then be reported as zero transferred messages,
> > which is converted to -EIO by Linux i2c_smbus_xfer_emulated().  The
> > race is more likely when multiple I2C buses are accessed concurrently.
> >
> > Drop pending bits that no longer correspond to an asserted and enabled
> > source before they can be promoted back to status.
> >
> > Signed-off-by: Jian Zhang <zhangjian.3032@bytedance.com>
> 
> 
> This looks correct to me.
> 
> Jamin, could you take a look please ?
> 
> Thanks,
> 
> C.
> 

Reviewed-by: Jamin Lin <jamin_lin@aspeedtech.com>

Thanks,
Jamin

> 
> 
> > ---
> >   hw/intc/aspeed_intc.c | 25 +++++++++++++++++++++++++
> >   1 file changed, 25 insertions(+)
> >
> > diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c index
> > 5a36fff5204..ab80c239bc1 100644
> > --- a/hw/intc/aspeed_intc.c
> > +++ b/hw/intc/aspeed_intc.c
> > @@ -107,6 +107,27 @@ static const AspeedINTCIRQ
> *aspeed_intc_get_irq(AspeedINTCClass *aic,
> >       g_assert_not_reached();
> >   }
> >
> > +static uint32_t aspeed_intc_orgate_levels(AspeedINTCState *s, int
> > +inpin_idx) {
> > +    AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
> > +    uint32_t levels = 0;
> > +    int i;
> > +
> > +    for (i = 0; i < aic->num_lines && i < 32; i++) {
> > +        if (s->orgates[inpin_idx].levels[i]) {
> > +            levels |= BIT(i);
> > +        }
> > +    }
> > +
> > +    return levels;
> > +}
> > +
> > +static void aspeed_intc_drop_stale_pending(AspeedINTCState *s, int
> > +inpin_idx) {
> > +    s->pending[inpin_idx] &= aspeed_intc_orgate_levels(s, inpin_idx) &
> > +                             s->enable[inpin_idx]; }
> > +
> >   /*
> >    * Update the state of an interrupt controller pin by setting
> >    * the specified output pin to the given level.
> > @@ -231,6 +252,8 @@ static void aspeed_intc_set_irq(void *opaque, int irq,
> int level)
> >       trace_aspeed_intc_set_irq(name, inpin_idx, level);
> >       enable = s->enable[inpin_idx];
> >
> > +    aspeed_intc_drop_stale_pending(s, inpin_idx);
> > +
> >       if (!level) {
> >           return;
> >       }
> > @@ -343,6 +366,7 @@ static void
> aspeed_intc_status_handler(AspeedINTCState *s, hwaddr offset,
> >       /* All source ISR execution are done */
> >       if (!s->regs[reg]) {
> >           trace_aspeed_intc_all_isr_done(name, inpin_idx);
> > +        aspeed_intc_drop_stale_pending(s, inpin_idx);
> >           if (s->pending[inpin_idx]) {
> >               /*
> >                * handle pending source interrupt @@ -402,6 +426,7 @@
> > static void aspeed_intc_status_handler_multi_outpins(AspeedINTCState *s,
> >           /* All source ISR executions are done from a specific bit */
> >           if (data & BIT(i)) {
> >               trace_aspeed_intc_all_isr_done_bit(name, inpin_idx, i);
> > +            aspeed_intc_drop_stale_pending(s, inpin_idx);
> >               if (s->pending[inpin_idx] & BIT(i)) {
> >                   /*
> >                    * Handle pending source interrupt.

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.