On Thu, 3 Sept 2026 at 15:22, Alex Bennée <[email protected]> wrote: > > In ARM GICv2 (and GICv1), reads of the Highest Priority Pending > Interrupt Register (GICC_HPPIR and GICV_HPPIR) must include the > requesting CPU ID in bits [12:10] when the highest priority pending > interrupt is an SGI (interrupt IDs 0-15).
This is true. The SGI field also is in the GICC_IAR (interrupt acknowledge register) and needs to be accepted for writes to GICC_EOIR (end of interrupt). Currently we don't do any of that (we don't fill in the SGI field for reads from GICC_IARR, and we just ignore the SGI field for writes to GICC_EOIR rather than using it. The important thing here is that "an SGI of ID X from source CPU Y" is different (has its own pending state, etc) from "an SGI of ID X from source CPU Z"). Perhaps we should in retrospect have made the interrupt ID values for SGIs include the CPU source ID part, rather than dropping the source ID and then trying to bolt it back on afterwards, but trying to change that at this point is probably a bit invasive. > Introduce gic_get_sgi_source() to look up the source CPU ID for both > physical and virtual interfaces, and include it in bits [12:10] of the > HPPIR read value for SGIs. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/4402 > AI-used-for: initial prototype changes > Signed-off-by: Alex Bennée <[email protected]> > > --- > NOTE: > - the initial patch was more expansive and included re-factoring > gic_clear_pending_sgi to use the new helper. I dumped that change > so I could reason through the fix on its own. > --- > hw/intc/arm_gic.c | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > index 4d4b79e6f34..181d17ead7c 100644 > --- a/hw/intc/arm_gic.c > +++ b/hw/intc/arm_gic.c > @@ -568,6 +568,17 @@ static void gic_drop_prio(GICState *s, int cpu, int > group) > s->running_priority[cpu] = gic_get_prio_from_apr_bits(s, cpu); > } > > +static inline int gic_get_sgi_source(GICState *s, int irq, int cpu) > +{ > + if (gic_is_vcpu(cpu)) { > + uint32_t *lr_entry = gic_get_lr_entry(s, irq, cpu); > + return GICH_LR_HW(*lr_entry) ? 0 : GICH_LR_CPUID(*lr_entry); > + } else { > + assert(s->sgi_pending[irq][cpu] != 0); > + return ctz32(s->sgi_pending[irq][cpu]) & 0x7; Why mask with 7 ? Unless something's gone wrong there shouldn't be any unexpected bits set. > + } > +} > + > static inline uint32_t gic_clear_pending_sgi(GICState *s, int irq, int cpu) > { > int src; > @@ -1648,8 +1659,16 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, > int offset, > *data = gic_get_running_priority(s, cpu, attrs); > break; > case 0x18: /* Highest Pending Interrupt */ > - *data = gic_get_current_pending_irq(s, cpu, attrs); > + { > + uint16_t p = gic_get_current_pending_irq(s, cpu, attrs); > + > + if (p < GIC_NR_SGIS && s->revision != REV_11MPCORE) { Why "not if 11mpcore" ? The 11mpcore TRM says it has the SGI field for HPPIR and IAR: https://support.arm.com/documentation/ddi0360/f/mpcore-distributed-interrupt-controller/cpu-interrupt-interface-registers/highest-pending-interrupt-register--0x18?lang=en > + *data = p | (gic_get_sgi_source(s, p, cpu) << 10); > + } else { > + *data = p; > + } > break; > + } > case 0x1c: /* Aliased Binary Point */ > /* GIC v2, no security: ABPR > * GIC v1, no security: not implemented (RAZ/WI) > -- > 2.47.3 -- PMM
