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

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

Peter,

> -----Original Message-----
> From: Peter Maydell [mailto:address@hidden
> Sent: Monday, October 24, 2016 5:26 PM
> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> <address@hidden>
> Cc: QEMU Developers <address@hidden>; qemu-arm <qemu-
> address@hidden>; address@hidden
> Subject: Re: [v2] nvic: set pending status for not active interrupts
> 
> On 18 October 2016 at 07:14,  <address@hidden> wrote:
> > From: Marcin Krzeminski <address@hidden>
> >
> > 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 <address@hidden>
> 
> Thanks for rolling a v2. Unfortunately I think we don't have the logic quite
> right yet...
> 
Yeap,  I separated it but not update to work only with NVIC.
> > ---
> > 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.
> 
Yes, it works exactly as you're saying (checked on HW), if level is still
high after exit interrupt handler, interrupt is re-pend.
> > +    } 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)
> 
Same as above. I'll send v3.

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