On 28 January 2014 20:32, Christoffer Dall <christoffer.dall@xxxxxxxxxx> wrote: > The existing implementation of the pending behavior in gic_set_irq, > gic_acknowledge_irq, gic_complete_irq, and the distributor pending > set/clear registers does not follow the semantics of the GICv2.0 specs, > but may implement the 11MPCore support. Therefore, maintain the > existing semantics for 11MPCore and v7M NVIC and change the behavior to > be in accordance with the GICv2.0 specs for "generic implementations" > (s->revision == 1 || s->revision == 2). > > Generic implementations distinguish between setting a level-triggered > interrupt pending through writes to the GICD_ISPENDR and when hardware > raises the interrupt line. Writing to the GICD_ICPENDR will not cause > the interrupt to become non-pending if the line is still active, and > conversely, if the line is deactivated but the interrupt is marked as > pending through a write to GICD_ISPENDR, the interrupt remains pending. > Handle this situation in the GIC_TEST_PENDING (which now becomes a > static inline named gic_test_pending) and let the 'pending' field > correspond only to the latched state of the D-flip flop in the GICv2.0 > specs Figure 4-10. > > The following changes are added: > > gic_test_pending: > Make this a static inline and split out the 11MPCore from the generic > behavior. For the generic behavior, consider interrupts pending if: > ((s->irq_state[irq].pending & (cm) != 0) || > (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_LEVEL(irq, cm)) > > gic_set_irq: > Split out the 11MPCore from the generic behavior. For the generic > behavior, always GIC_SET_LEVEL(irq, 1) on positive level, but only > GIC_SET_PENDING for edge-triggered interrupts and vice versa on a > negative level. > > gic_complete_irq: > Only resample the line for line-triggered interrupts on an 11MPCore. > Generic implementations will sample the line directly in > gic_test_pending(). > > Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx> This looks broadly right; a couple of comments below. > --- > Changes [v4 -> v5]: > - Factor out GIC_NR_SGIS and gic_dist_writeb bugfixes > - Change meaning of pending field for level-triggered interrupts for > GIC v1/v2, to only capture manually written state to pending > registers. Add or-clause to gic_test_pending to sample the line > state, as per Peter's suggestions: > https://lists.cs.columbia.edu/pipermail/kvmarm/2014-January/008798.html > > Changes [v3 -> v4]: > - Maintain 11MPCore semantics > - Combine all pending interrupts fixing patches into this patch. See > the detailed description above. > > Changes [v1 -> v2]: > - Fix bisection issue, by not using gic_clear_pending yet. > > hw/intc/arm_gic.c | 74 ++++++++++++++++++++++++++++++++++++-------------- > hw/intc/gic_internal.h | 16 ++++++++++- > 2 files changed, 69 insertions(+), 21 deletions(-) > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > index 1c4a114..5e2cf14 100644 > --- a/hw/intc/arm_gic.c > +++ b/hw/intc/arm_gic.c > @@ -66,7 +66,7 @@ void gic_update(GICState *s) > best_prio = 0x100; > best_irq = 1023; > for (irq = 0; irq < s->num_irq; irq++) { > - if (GIC_TEST_ENABLED(irq, cm) && GIC_TEST_PENDING(irq, cm)) { > + if (GIC_TEST_ENABLED(irq, cm) && gic_test_pending(s, irq, cm)) { > if (GIC_GET_PRIORITY(irq, cpu) < best_prio) { > best_prio = GIC_GET_PRIORITY(irq, cpu); > best_irq = irq; > @@ -89,14 +89,46 @@ void gic_set_pending_private(GICState *s, int cpu, int irq) > { > int cm = 1 << cpu; > > - if (GIC_TEST_PENDING(irq, cm)) > + if (gic_test_pending(s, irq, cm)) { > return; > + } > > DPRINTF("Set %d pending cpu %d\n", irq, cpu); > GIC_SET_PENDING(irq, cm); > gic_update(s); > } > > +static void gic_set_irq_11mpcore(GICState *s, int irq, int level, > + int cm, int target) > +{ > + if (level) { > + GIC_SET_LEVEL(irq, cm); > + if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)) { > + DPRINTF("Set %d pending mask %x\n", irq, target); > + GIC_SET_PENDING(irq, target); > + } > + } else { > + GIC_CLEAR_LEVEL(irq, cm); > + } > +} > + > +static void gic_set_irq_generic(GICState *s, int irq, int level, > + int cm, int target) > +{ > + if (level) { > + GIC_SET_LEVEL(irq, cm); > + DPRINTF("Set %d pending mask %x\n", irq, target); > + if (GIC_TEST_EDGE_TRIGGER(irq)) { > + GIC_SET_PENDING(irq, target); > + } > + } else { > + if (GIC_TEST_EDGE_TRIGGER(irq)) { > + GIC_CLEAR_PENDING(irq, target); > + } This doesn't look right. A falling edge for an edge triggered interrupt should just CLEAR_LEVEL and leave the state of the pending latch alone. > + GIC_CLEAR_LEVEL(irq, cm); > + } > +} > + > /* Process a change in an external IRQ input. */ > static void gic_set_irq(void *opaque, int irq, int level) > { > @@ -126,15 +158,12 @@ static void gic_set_irq(void *opaque, int irq, int level) > return; > } > > - if (level) { > - GIC_SET_LEVEL(irq, cm); > - if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)) { > - DPRINTF("Set %d pending mask %x\n", irq, target); > - GIC_SET_PENDING(irq, target); > - } > + if (s->revision == REV_11MPCORE) { Or NVIC. > + gic_set_irq_11mpcore(s, irq, level, cm, target); > } else { > - GIC_CLEAR_LEVEL(irq, cm); > + gic_set_irq_generic(s, irq, level, cm, target); > } > + > gic_update(s); > } > > @@ -160,9 +189,10 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu) > return 1023; > } > s->last_active[new_irq][cpu] = s->running_irq[cpu]; > - /* Clear pending flags for both level and edge triggered interrupts. > - Level triggered IRQs will be reasserted once they become inactive. */ > - GIC_CLEAR_PENDING(new_irq, GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm); > + > + cm = GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm; > + GIC_CLEAR_PENDING(new_irq, cm); > + This assignment to cm ends up changing behaviour of following code for 11mpcore/NVIC, I think. It looks to me like you can just drop this hunk. > gic_set_running_irq(s, cpu, new_irq); > DPRINTF("ACK %d\n", new_irq); > return new_irq; > @@ -195,14 +225,18 @@ void gic_complete_irq(GICState *s, int cpu, int irq) > } > if (s->running_irq[cpu] == 1023) > return; /* No active IRQ. */ > - /* Mark level triggered interrupts as pending if they are still > - raised. */ > - if (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_ENABLED(irq, cm) > - && GIC_TEST_LEVEL(irq, cm) && (GIC_TARGET(irq) & cm) != 0) { > - DPRINTF("Set %d pending mask %x\n", irq, cm); > - GIC_SET_PENDING(irq, cm); > - update = 1; > + > + if (s->revision == REV_11MPCORE) { Or NVIC. > + /* Mark level triggered interrupts as pending if they are still > + raised. */ > + if (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_ENABLED(irq, cm) > + && GIC_TEST_LEVEL(irq, cm) && (GIC_TARGET(irq) & cm) != 0) { > + DPRINTF("Set %d pending mask %x\n", irq, cm); > + GIC_SET_PENDING(irq, cm); > + update = 1; > + } > } > + > if (irq != s->running_irq[cpu]) { > /* Complete an IRQ that is not currently running. */ > int tmp = s->running_irq[cpu]; > @@ -273,7 +307,7 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset) > res = 0; > mask = (irq < GIC_INTERNAL) ? cm : ALL_CPU_MASK; > for (i = 0; i < 8; i++) { > - if (GIC_TEST_PENDING(irq + i, mask)) { > + if (gic_test_pending(s, irq + i, mask)) { > res |= (1 << i); > } > } > diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h > index 8c02d58..92a6f7a 100644 > --- a/hw/intc/gic_internal.h > +++ b/hw/intc/gic_internal.h > @@ -34,7 +34,6 @@ > #define GIC_TEST_ENABLED(irq, cm) ((s->irq_state[irq].enabled & (cm)) != 0) > #define GIC_SET_PENDING(irq, cm) s->irq_state[irq].pending |= (cm) > #define GIC_CLEAR_PENDING(irq, cm) s->irq_state[irq].pending &= ~(cm) > -#define GIC_TEST_PENDING(irq, cm) ((s->irq_state[irq].pending & (cm)) != 0) > #define GIC_SET_ACTIVE(irq, cm) s->irq_state[irq].active |= (cm) > #define GIC_CLEAR_ACTIVE(irq, cm) s->irq_state[irq].active &= ~(cm) > #define GIC_TEST_ACTIVE(irq, cm) ((s->irq_state[irq].active & (cm)) != 0) > @@ -63,4 +62,19 @@ void gic_update(GICState *s); > void gic_init_irqs_and_distributor(GICState *s, int num_irq); > void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val); > > +static inline bool gic_test_pending(GICState *s, int irq, int cm) > +{ > + if (s->revision == REV_NVIC || s->revision == REV_11MPCORE) { > + return s->irq_state[irq].pending & cm; > + } else { > + /* Edge-triggered interrupts are marked pending on a rising edge, but > + * level-triggered interrupts are either considered pending when the > + * level is active or if software has explicitly written to > + * GICD_ISPENDR to set the state pending. > + */ > + return (s->irq_state[irq].pending & cm) || > + (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_LEVEL(irq, cm)); > + } > +} > + > #endif /* !QEMU_ARM_GIC_INTERNAL_H */ thanks -- PMM _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
