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/