Re: [Qemu-devel] [PATCH v5 3/8] arm_gic: Fix GIC pending behavior

[Peter Maydell]

On 28 January 2014 20:32, Christoffer Dall <address@hidden> 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 <address@hidden>

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