[PATCH] hw/intc/arm_gic: Fix set/clear pending of PPI/SPI

[PATCH] hw/intc/arm_gic: Fix set/clear pending of PPI/SPI

[Sebastian Huber]

According to the GICv3 specification register GICD_ISPENDR0 is Banked for each
connected PE with GICR_TYPER.Processor_Number < 8.  For Qemu this is the case
since GIC_NCPU == 8.

For SPI, make the interrupt pending on all CPUs and not just the processor
targets of the interrupt.

This behaviour is at least present on the i.MX7D which uses an Cortex-A7MPCore.

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

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index a994b1f024..8e377bac59 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -1294,12 +1294,14 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
 
         for (i = 0; i < 8; i++) {
             if (value & (1 << i)) {
+                int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
+
                 if (s->security_extn && !attrs.secure &&
                     !GIC_DIST_TEST_GROUP(irq + i, 1 << cpu)) {
                     continue; /* Ignore Non-secure access of Group0 IRQ */
                 }
 
-                GIC_DIST_SET_PENDING(irq + i, GIC_DIST_TARGET(irq + i));
+                GIC_DIST_SET_PENDING(irq + i, cm);
             }
         }
     } else if (offset < 0x300) {
@@ -1317,11 +1319,10 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
                 continue; /* Ignore Non-secure access of Group0 IRQ */
             }
 
-            /* ??? This currently clears the pending bit for all CPUs, even
-               for per-CPU interrupts.  It's unclear whether this is the
-               corect behavior.  */
             if (value & (1 << i)) {
-                GIC_DIST_CLEAR_PENDING(irq + i, ALL_CPU_MASK);
+                int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
+
+                GIC_DIST_CLEAR_PENDING(irq + i, cm);
             }
         }
     } else if (offset < 0x380) {
-- 
2.26.2

Re: [PATCH] hw/intc/arm_gic: Fix set/clear pending of PPI/SPI

[Sebastian Huber]

On 09/07/2021 11:49, Sebastian Huber wrote:
> According to the GICv3 specification register GICD_ISPENDR0 is Banked for each
> connected PE with GICR_TYPER.Processor_Number < 8.  For Qemu this is the case
> since GIC_NCPU == 8.
> 
> For SPI, make the interrupt pending on all CPUs and not just the processor
> targets of the interrupt.
> 
> This behaviour is at least present on the i.MX7D which uses an Cortex-A7MPCore.

Could someone please have a look at this patch?

-- 
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/

Re: [PATCH] hw/intc/arm_gic: Fix set/clear pending of PPI/SPI

[Philippe Mathieu-Daudé]

Cc'ing qemu-arm@

On 7/9/21 11:49 AM, Sebastian Huber wrote:
> According to the GICv3 specification register GICD_ISPENDR0 is Banked for each
> connected PE with GICR_TYPER.Processor_Number < 8.  For Qemu this is the case
> since GIC_NCPU == 8.
> 
> For SPI, make the interrupt pending on all CPUs and not just the processor
> targets of the interrupt.
> 
> This behaviour is at least present on the i.MX7D which uses an Cortex-A7MPCore.
> 
> Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
> ---
>  hw/intc/arm_gic.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index a994b1f024..8e377bac59 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -1294,12 +1294,14 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>  
>          for (i = 0; i < 8; i++) {
>              if (value & (1 << i)) {
> +                int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
> +
>                  if (s->security_extn && !attrs.secure &&
>                      !GIC_DIST_TEST_GROUP(irq + i, 1 << cpu)) {
>                      continue; /* Ignore Non-secure access of Group0 IRQ */
>                  }
>  
> -                GIC_DIST_SET_PENDING(irq + i, GIC_DIST_TARGET(irq + i));
> +                GIC_DIST_SET_PENDING(irq + i, cm);
>              }
>          }
>      } else if (offset < 0x300) {
> @@ -1317,11 +1319,10 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>                  continue; /* Ignore Non-secure access of Group0 IRQ */
>              }
>  
> -            /* ??? This currently clears the pending bit for all CPUs, even
> -               for per-CPU interrupts.  It's unclear whether this is the
> -               corect behavior.  */
>              if (value & (1 << i)) {
> -                GIC_DIST_CLEAR_PENDING(irq + i, ALL_CPU_MASK);
> +                int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
> +
> +                GIC_DIST_CLEAR_PENDING(irq + i, cm);
>              }
>          }
>      } else if (offset < 0x380) {
> 

Re: [PATCH] hw/intc/arm_gic: Fix set/clear pending of PPI/SPI

[Luc Michel]

Hi Sebastian,

On 11:49 Fri 09 Jul     , Sebastian Huber wrote:
> According to the GICv3 specification register GICD_ISPENDR0 is Banked for each
You're referring to GICv3 but actually modifying GICv2 model. Having a
look at GICv2 reference manual, your affirmation still hold though.

> connected PE with GICR_TYPER.Processor_Number < 8.  For Qemu this is the case
> since GIC_NCPU == 8.
This is GICv3 specific. For GICv2 the architectural limit is 8 CPUs.

> 
> For SPI, make the interrupt pending on all CPUs and not just the processor
> targets of the interrupt.
So you're not referring to GICD_ISPENDR0 anymore right? SPIs starts at
IRQ number 32.  GICD_ISPENDR0 is for IRQs 0 to 31, which are SGIs and
PPIs (This is why this reg is banked, meaning that a CPU can only
trigger a PPI of its own). Maybe make it clear in your commit message
that you are now talking about GICD_ISPENDRn with n > 0

Moreover your statement regarding SPIs seems weird to me. Setting an
SPI pending (in GICD_ISPENDRn with n > 0) should really be like having
it being triggered from the IRQ line. It makes it pending in the
distributor. The distributor then forward it as normal. Why the
GICD_ITARGETSRn configuration should be ignored in this case? At least I
can't find any reference to such a behaviour in the reference manual.

> 
> This behaviour is at least present on the i.MX7D which uses an Cortex-A7MPCore.
Which has a GICv2, not a v3 right?

> 
> Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
> ---
>  hw/intc/arm_gic.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index a994b1f024..8e377bac59 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -1294,12 +1294,14 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>  
>          for (i = 0; i < 8; i++) {
>              if (value & (1 << i)) {
> +                int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
> +
Indeed, I think the current implementation for PPIs is wrong
(GIC_DIST_TARGET(irq + i) is probably 0 in this case, so a set pending
request for a PPI will get incorrectly ignored). So I agree with the irq <
GIC_INTERNAL case. But for SPIs my concerns still hold (see my comment
in the commit message).

>                  if (s->security_extn && !attrs.secure &&
>                      !GIC_DIST_TEST_GROUP(irq + i, 1 << cpu)) {
>                      continue; /* Ignore Non-secure access of Group0 IRQ */
>                  }
>  
> -                GIC_DIST_SET_PENDING(irq + i, GIC_DIST_TARGET(irq + i));
> +                GIC_DIST_SET_PENDING(irq + i, cm);
>              }
>          }
>      } else if (offset < 0x300) {
> @@ -1317,11 +1319,10 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>                  continue; /* Ignore Non-secure access of Group0 IRQ */
>              }
>  
> -            /* ??? This currently clears the pending bit for all CPUs, even
> -               for per-CPU interrupts.  It's unclear whether this is the
> -               corect behavior.  */
>              if (value & (1 << i)) {
> -                GIC_DIST_CLEAR_PENDING(irq + i, ALL_CPU_MASK);
> +                int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
> +
> +                GIC_DIST_CLEAR_PENDING(irq + i, cm);
I agree with this change too, but you are modifying the GICD_ICPENDRn
register behaviour without mentioning it in the commit message.

Thanks,

-- 
Luc

Re: [PATCH] hw/intc/arm_gic: Fix set/clear pending of PPI/SPI

[Sebastian Huber]

Hello Luc,

thanks for having a look at the patch.

On 25/07/2021 10:08, Luc Michel wrote:
> Hi Sebastian,
> 
> On 11:49 Fri 09 Jul     , Sebastian Huber wrote:
>> According to the GICv3 specification register GICD_ISPENDR0 is Banked for each
> You're referring to GICv3 but actually modifying GICv2 model. Having a
> look at GICv2 reference manual, your affirmation still hold though.
> 
>> connected PE with GICR_TYPER.Processor_Number < 8.  For Qemu this is the case
>> since GIC_NCPU == 8.
> This is GICv3 specific. For GICv2 the architectural limit is 8 CPUs.

The wording in the GICv2 manual is:

"In a multiprocessor implementation, GICD_ISPENDR0 is banked for each 
connected processor. This register holds the Set-pending bits for 
interrupts 0-31."

> 
>>
>> For SPI, make the interrupt pending on all CPUs and not just the processor
>> targets of the interrupt.
> So you're not referring to GICD_ISPENDR0 anymore right? SPIs starts at
> IRQ number 32.  GICD_ISPENDR0 is for IRQs 0 to 31, which are SGIs and
> PPIs (This is why this reg is banked, meaning that a CPU can only
> trigger a PPI of its own). Maybe make it clear in your commit message
> that you are now talking about GICD_ISPENDRn with n > 0
> 
> Moreover your statement regarding SPIs seems weird to me. Setting an
> SPI pending (in GICD_ISPENDRn with n > 0) should really be like having
> it being triggered from the IRQ line. It makes it pending in the
> distributor. The distributor then forward it as normal. Why the
> GICD_ITARGETSRn configuration should be ignored in this case? At least I
> can't find any reference to such a behaviour in the reference manual.

Ok, I will remove this part from the patch in v2. I probably didn't 
fully understand how the Qemu GICv2 emulation works. What I wanted to 
address is this behaviour (see GICv2 manual) when someone changes the 
GICD_ITARGETSR<n> (n > 1):

"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.

Note

There is a small but finite time required for any change to take effect."

The set/clear active bit uses ALL_CPU_MASK for example.

> 
>>
>> This behaviour is at least present on the i.MX7D which uses an Cortex-A7MPCore.
> Which has a GICv2, not a v3 right?

Yes, the Cortex-A7MPCore uses a GICv2:

https://developer.arm.com/documentation/ddi0464/f/Generic-Interrupt-Controller/About-the-GIC?lang=en

> 
>>
>> Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
>> ---
>>   hw/intc/arm_gic.c | 11 ++++++-----
>>   1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>> index a994b1f024..8e377bac59 100644
>> --- a/hw/intc/arm_gic.c
>> +++ b/hw/intc/arm_gic.c
>> @@ -1294,12 +1294,14 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>>   
>>           for (i = 0; i < 8; i++) {
>>               if (value & (1 << i)) {
>> +                int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
>> +
> Indeed, I think the current implementation for PPIs is wrong
> (GIC_DIST_TARGET(irq + i) is probably 0 in this case, so a set pending
> request for a PPI will get incorrectly ignored). So I agree with the irq <
> GIC_INTERNAL case. But for SPIs my concerns still hold (see my comment
> in the commit message).
> 
>>                   if (s->security_extn && !attrs.secure &&
>>                       !GIC_DIST_TEST_GROUP(irq + i, 1 << cpu)) {
>>                       continue; /* Ignore Non-secure access of Group0 IRQ */
>>                   }
>>   
>> -                GIC_DIST_SET_PENDING(irq + i, GIC_DIST_TARGET(irq + i));
>> +                GIC_DIST_SET_PENDING(irq + i, cm);
>>               }
>>           }
>>       } else if (offset < 0x300) {
>> @@ -1317,11 +1319,10 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>>                   continue; /* Ignore Non-secure access of Group0 IRQ */
>>               }
>>   
>> -            /* ??? This currently clears the pending bit for all CPUs, even
>> -               for per-CPU interrupts.  It's unclear whether this is the
>> -               corect behavior.  */
>>               if (value & (1 << i)) {
>> -                GIC_DIST_CLEAR_PENDING(irq + i, ALL_CPU_MASK);
>> +                int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
>> +
>> +                GIC_DIST_CLEAR_PENDING(irq + i, cm);
> I agree with this change too, but you are modifying the GICD_ICPENDRn
> register behaviour without mentioning it in the commit message.
> 
> Thanks,
> 

-- 
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/

Re: [PATCH] hw/intc/arm_gic: Fix set/clear pending of PPI/SPI

[Peter Maydell]

On Mon, 26 Jul 2021 at 09:06, Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
> Ok, I will remove this part from the patch in v2. I probably didn't
> fully understand how the Qemu GICv2 emulation works. What I wanted to
> address is this behaviour (see GICv2 manual) when someone changes the
> GICD_ITARGETSR<n> (n > 1):
>
> "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.
>
> Note
>
> There is a small but finite time required for any change to take effect."

We do get this wrong, but none of your changes to the file actually
change this behaviour :-)

What we currently do for writes to GICD_ITARGETS<r> is:

        /* Interrupt CPU Target. RAZ/WI on uniprocessor GICs, with the
         * annoying exception of the 11MPCore's GIC.
         */
        if (s->num_cpu != 1 || s->revision == REV_11MPCORE) {
            irq = (offset - 0x800);
            if (irq >= s->num_irq) {
                goto bad_reg;
            }
            if (irq < 29 && s->revision == REV_11MPCORE) {
                value = 0;
            } else if (irq < GIC_INTERNAL) {
                value = ALL_CPU_MASK;
            }
            s->irq_target[irq] = value & ALL_CPU_MASK;
        }

which is to say that we update irq_target[] but we don't change the
status of any pending interrupt. We should add code here that
checks whether irq is pending on any core and if so marks it as
instead pending on the targets we just set up, something like

 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);
 }

thanks
-- PMM

[PATCH 1/2] hw/intc/arm_gic: Fix set pending of PPIs

[Sebastian Huber]

According to the GICv2 specification section 4.3.7, "Interrupt Set-Pending
Registers, GICD_ISPENDRn":

"In a multiprocessor implementation, GICD_ISPENDR0 is banked for each connected
processor. This register holds the Set-pending bits for interrupts 0-31."

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

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 4da5326ed6..20b3f701e0 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -1296,12 +1296,14 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
 
         for (i = 0; i < 8; i++) {
             if (value & (1 << i)) {
+                int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
+
                 if (s->security_extn && !attrs.secure &&
                     !GIC_DIST_TEST_GROUP(irq + i, 1 << cpu)) {
                     continue; /* Ignore Non-secure access of Group0 IRQ */
                 }
 
-                GIC_DIST_SET_PENDING(irq + i, GIC_DIST_TARGET(irq + i));
+                GIC_DIST_SET_PENDING(irq + i, cm);
             }
         }
     } else if (offset < 0x300) {
-- 
2.35.3

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

[Sebastian Huber]

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);
+            }
         }
     } else if (offset < 0xf00) {
         /* Interrupt Configuration.  */
-- 
2.35.3

[PATCH 1/2] hw/intc/arm_gic: Fix set pending of PPIs

[Sebastian Huber]

According to the GICv2 specification section 4.3.7, "Interrupt Set-Pending
Registers, GICD_ISPENDRn":

"In a multiprocessor implementation, GICD_ISPENDR0 is banked for each connected
processor. This register holds the Set-pending bits for interrupts 0-31."

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

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 4da5326ed6..20b3f701e0 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -1296,12 +1296,14 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
 
         for (i = 0; i < 8; i++) {
             if (value & (1 << i)) {
+                int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
+
                 if (s->security_extn && !attrs.secure &&
                     !GIC_DIST_TEST_GROUP(irq + i, 1 << cpu)) {
                     continue; /* Ignore Non-secure access of Group0 IRQ */
                 }
 
-                GIC_DIST_SET_PENDING(irq + i, GIC_DIST_TARGET(irq + i));
+                GIC_DIST_SET_PENDING(irq + i, cm);
             }
         }
     } else if (offset < 0x300) {
-- 
2.35.3

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

[Sebastian Huber]

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);
+            }
         }
     } else if (offset < 0xf00) {
         /* Interrupt Configuration.  */
-- 
2.35.3

Re: [PATCH 1/2] hw/intc/arm_gic: Fix set pending of PPIs

[Peter Maydell]

On Tue, 7 May 2024 at 14:00, Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
>
> According to the GICv2 specification section 4.3.7, "Interrupt Set-Pending
> Registers, GICD_ISPENDRn":
>
> "In a multiprocessor implementation, GICD_ISPENDR0 is banked for each connected
> processor. This register holds the Set-pending bits for interrupts 0-31."

The commit message says it's only changing the handling of
setting the pending bit for a PPI, but...

> Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
> ---
>  hw/intc/arm_gic.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 4da5326ed6..20b3f701e0 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -1296,12 +1296,14 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>
>          for (i = 0; i < 8; i++) {
>              if (value & (1 << i)) {
> +                int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
> +
>                  if (s->security_extn && !attrs.secure &&
>                      !GIC_DIST_TEST_GROUP(irq + i, 1 << cpu)) {
>                      continue; /* Ignore Non-secure access of Group0 IRQ */
>                  }
>
> -                GIC_DIST_SET_PENDING(irq + i, GIC_DIST_TARGET(irq + i));
> +                GIC_DIST_SET_PENDING(irq + i, cm);

... the patch changes also the handling of set-pending for
SPIs (which previously were marked pending on the target CPU
and are now marked pending on all CPUs).

Looking back at the thread from your 2021 patch this was also
noted in that version as being wrong:
https://lore.kernel.org/qemu-devel/20210725080817.ivlkutnow7sojoyd@sekoia-pc.home.lmichel.fr/

PS: for multi-patch patches please can you send also a cover
letter? Our automated tooling gets confused if there isn't one.
It looks also like you sent these respins of these patches as
followups to the thread of the original patch you sent back in
2021. Can you send new versions of patches as their own threads,
please (and with a "PATCH v2" (v3, etc) tag if they're respins?

thanks
-- PMM

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

[Peter Maydell]

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