Re: [Qemu-devel] [v2] nvic: set pending status for not active interrupts

[Peter Maydell <peter.maydell@linaro.org>]

On 18 October 2016 at 07:14,  <marcin.krzeminski@nokia.com> wrote:
> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>
> According to ARM DUI 0552A 4.2.10. NVIC set pending status
> also for disabled interrupts. This patch adds possibility
> to emulate this in Qemu.
>
> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>

Thanks for rolling a v2. Unfortunately I think we don't
have the logic quite right yet...

> ---
> Changes for v2:
>     - add a dedicated unction for nvic
>     - update complete_irq
>  hw/intc/arm_gic.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index b30cc91..72e4c01 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -156,6 +156,21 @@ static void gic_set_irq_11mpcore(GICState *s, int irq, int level,
>      }
>  }
>
> +static void gic_set_irq_nvic(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)
> +                || !GIC_TEST_ACTIVE(irq, cm)) {
> +            DPRINTF("Set %d pending mask %x\n", irq, target);
> +            GIC_SET_PENDING(irq, target);
> +        }

Why is GIC_TEST_ENABLED() in this condition, when the idea is
that we don't care about the enabled status for whether we can
pend the interrupt?

I think this should actually just always do
   GIC_SET_PENDING(irq, target)
whenever level is high

because:
 (1) pending status can be set for disabled interrupts
 (2) edge-triggered IRQs definitely always re-pend on rising edge
 (3) I think level-triggered IRQs do too (it's a bit
     less clear in the docs, but DUI0552A 4.2.9 says we pend on
     a rising edge and doesn't say that applies only to edge
     triggered IRQs)

I don't have any real hardware to hand to test against, though.

> +    } else {
> +        GIC_CLEAR_LEVEL(irq, cm);
> +    }
> +}
> +
>  static void gic_set_irq_generic(GICState *s, int irq, int level,
>                                  int cm, int target)
>  {
> @@ -201,8 +216,10 @@ static void gic_set_irq(void *opaque, int irq, int level)
>          return;
>      }
>
> -    if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
> +    if (s->revision == REV_11MPCORE) {
>          gic_set_irq_11mpcore(s, irq, level, cm, target);
> +    } else if (s->revision == REV_NVIC) {
> +        gic_set_irq_nvic(s, irq, level, cm, target);
>      } else {
>          gic_set_irq_generic(s, irq, level, cm, target);
>      }
> @@ -568,7 +585,7 @@ void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs)
>          return; /* No active IRQ.  */
>      }
>
> -    if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
> +    if (s->revision == REV_11MPCORE) {
>          /* Mark level triggered interrupts as pending if they are still
>             raised.  */
>          if (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_ENABLED(irq, cm)
> @@ -576,6 +593,12 @@ void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs)
>              DPRINTF("Set %d pending mask %x\n", irq, cm);
>              GIC_SET_PENDING(irq, cm);
>          }
> +    } else if (s->revision == REV_NVIC) {
> +        if (GIC_TEST_ENABLED(irq, cm) && GIC_TEST_LEVEL(irq, cm)
> +            && (GIC_TARGET(irq) & cm) != 0) {
> +            DPRINTF("Set nvic %d pending mask %x\n", irq, cm);
> +            GIC_SET_PENDING(irq, cm);
> +        }

Similarly, I think this should just be
    if (GIC_TEST_LEVEL(irq, cm) {
        GIC_SET_PENDING(irq, cm);
    }

because:
 (1) for the NVIC there is only one CPU and GIC_TARGET(irq) always
 indicates that IRQs target it
 (2) we don't care whether the interrupt is enabled when deciding
 whether it should be pended
 (3) we don't care whether the interrupt is level-triggered or
 edge-triggered for deciding whether to re-pend it
 (see statement R_XVWM in the v8M ARM ARM DDI0553A.c)

>      }
>
>      group = gic_has_groups(s) && GIC_TEST_GROUP(irq, cm);
> --
> 2.7.4

thanks
-- PMM