diff mbox series

[2/2] hw/intc/arm_gic: Fix writes to GICD_ITARGETSRn

Message ID 20240507130038.86787-2-sebastian.huber@embedded-brains.de (mailing list archive)
State New
Headers show
Series None | expand

Commit Message

Sebastian Huber May 7, 2024, 1 p.m. UTC
According to the GICv2 specification section 4.3.12, "Interrupt Processor
Targets Registers, GICD_ITARGETSRn":

"Any change to a CPU targets field value:
[...]
* Has an effect on any pending interrupts. This means:
  - adding a CPU interface to the target list of a pending interrupt makes that
    interrupt pending on that CPU interface
  - removing a CPU interface from the target list of a pending interrupt
    removes the pending state of that interrupt on that CPU interface."

Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
---
 hw/intc/arm_gic.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Peter Maydell May 20, 2024, 1:33 p.m. UTC | #1
On Tue, 7 May 2024 at 14:00, Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
>
> According to the GICv2 specification section 4.3.12, "Interrupt Processor
> Targets Registers, GICD_ITARGETSRn":
>
> "Any change to a CPU targets field value:
> [...]
> * Has an effect on any pending interrupts. This means:
>   - adding a CPU interface to the target list of a pending interrupt makes that
>     interrupt pending on that CPU interface
>   - removing a CPU interface from the target list of a pending interrupt
>     removes the pending state of that interrupt on that CPU interface."
>
> Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
> ---
>  hw/intc/arm_gic.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 20b3f701e0..79aee56053 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -1397,6 +1397,13 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>                  value = ALL_CPU_MASK;
>              }
>              s->irq_target[irq] = value & ALL_CPU_MASK;
> +            if (irq >= GIC_INTERNAL && s->irq_state[irq].pending) {
> +                /*
> +                 * Changing the target of an interrupt that is currently
> +                 * pending updates the set of CPUs it is pending on.
> +                 */
> +                GIC_DIST_SET_PENDING(irq, value);

Looking back at the 2021 thread this is the change I suggested then,
but I think I was wrong. This will set the pending bit for the new
specified set of targets, but it won't remove it from any CPUs that
previously were targeted and are not in the new target list (because
GIC_DIST_SET_PENDING does a logical OR into the pending field).
So I think what we want is
                   s->irq_state[irq].pending = value & ALL_CPU_MASK;

> +            }
>          }
>      } else if (offset < 0xf00) {
>          /* Interrupt Configuration.  */
> --

thanks
-- PMM
diff mbox series

Patch

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 20b3f701e0..79aee56053 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -1397,6 +1397,13 @@  static void gic_dist_writeb(void *opaque, hwaddr offset,
                 value = ALL_CPU_MASK;
             }
             s->irq_target[irq] = value & ALL_CPU_MASK;
+            if (irq >= GIC_INTERNAL && s->irq_state[irq].pending) {
+                /*
+                 * Changing the target of an interrupt that is currently
+                 * pending updates the set of CPUs it is pending on.
+                 */
+                GIC_DIST_SET_PENDING(irq, value);
+            }
         }
     } else if (offset < 0xf00) {
         /* Interrupt Configuration.  */