| Message ID | 20210709094948.60344-1-sebastian.huber@embedded-brains.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/intc/arm_gic: Fix set/clear pending of PPI/SPI | expand |
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?
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) { >
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,
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, >
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
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) {
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(-)