[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