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

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

[marcin.krzeminski]

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>
---
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);
+        }
+    } 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);
+        }
     }
 
     group = gic_has_groups(s) && GIC_TEST_GROUP(irq, cm);
-- 
2.7.4

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

[Peter Maydell]

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

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

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

Peter,

> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Monday, October 24, 2016 5:26 PM
> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> <marcin.krzeminski@nokia.com>
> Cc: QEMU Developers <qemu-devel@nongnu.org>; qemu-arm <qemu-
> arm@nongnu.org>; rfsw-patches@mlist.nokia.com
> Subject: Re: [v2] nvic: set pending status for not active interrupts
> 
> 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...
> 
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

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

[Peter Maydell]

On 31 October 2016 at 09:11, Krzeminski, Marcin (Nokia - PL/Wroclaw)
<marcin.krzeminski@nokia.com> wrote:
> Peter,
>
>> -----Original Message-----
>> From: Peter Maydell [mailto:peter.maydell@linaro.org]
>> > +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.

"after exiting interrupt handler" is the wrong condition to check.
You need to:
 * cause the interrupt line to be set
 * enter the interrupt handler as a result (int becomes active)
 * cause the interrupt line to be lowered (while in the handler)
 * cause the interrupt line to be set again (still in the handler)
 * check the interrupt pending state (still in the handler)

If you only check the pending state after exiting the handler,
then you can't tell whether it was pended on the rising edge
while active, or for the "re-pend if high when we leave the
interrupt handler". We know the hardware does the latter, it's
the former we're unsure about.

thanks
-- PMM

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

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

> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Monday, October 31, 2016 11:15 AM
> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> <marcin.krzeminski@nokia.com>
> Cc: QEMU Developers <qemu-devel@nongnu.org>; qemu-arm <qemu-
> arm@nongnu.org>; rfsw-patches@mlist.nokia.com
> Subject: Re: [v2] nvic: set pending status for not active interrupts
> 
> On 31 October 2016 at 09:11, Krzeminski, Marcin (Nokia - PL/Wroclaw)
> <marcin.krzeminski@nokia.com> wrote:
> > Peter,
> >
> >> -----Original Message-----
> >> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> >> > +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.
> 
> "after exiting interrupt handler" is the wrong condition to check.
> You need to:
>  * cause the interrupt line to be set
>  * enter the interrupt handler as a result (int becomes active)
>  * cause the interrupt line to be lowered (while in the handler)
>  * cause the interrupt line to be set again (still in the handler)
>  * check the interrupt pending state (still in the handler)
>
I performed this test, and when we are in interrupt handler (constantly
pooling pending bit) interrupt is re-pend just after line is set again.
V3 behaves in the same way.

Thanks,
Marcin

> If you only check the pending state after exiting the handler, then you can't
> tell whether it was pended on the rising edge while active, or for the "re-
> pend if high when we leave the interrupt handler". We know the hardware
> does the latter, it's the former we're unsure about.
> 
> thanks
> -- PMM

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

[Peter Maydell]

On 31 October 2016 at 11:56, Krzeminski, Marcin (Nokia - PL/Wroclaw)
<marcin.krzeminski@nokia.com> wrote:
>> "after exiting interrupt handler" is the wrong condition to check.
>> You need to:
>>  * cause the interrupt line to be set
>>  * enter the interrupt handler as a result (int becomes active)
>>  * cause the interrupt line to be lowered (while in the handler)
>>  * cause the interrupt line to be set again (still in the handler)
>>  * check the interrupt pending state (still in the handler)
>>
> I performed this test, and when we are in interrupt handler (constantly
> pooling pending bit) interrupt is re-pend just after line is set again.
> V3 behaves in the same way.

Great -- thanks for checking.

-- PMM