[Qemu-devel] [PATCH] nvic: allow to set pending status for not active interrupts

[Qemu-devel] [PATCH] nvic: allow to 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>
---
 hw/intc/arm_gic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index b30cc91..85be6e4 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -147,7 +147,8 @@ static void gic_set_irq_11mpcore(GICState *s, int irq, int level,
 {
     if (level) {
         GIC_SET_LEVEL(irq, cm);
-        if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)) {
+        if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)
+                || (!GIC_TEST_ACTIVE(irq, cm) &&  s->revision == REV_NVIC)) {
             DPRINTF("Set %d pending mask %x\n", irq, target);
             GIC_SET_PENDING(irq, target);
         }
-- 
2.7.4

Re: [Qemu-devel] [PATCH] nvic: allow to set pending status for not active interrupts

[Peter Maydell]

On 7 October 2016 at 10:42,  <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>
> ---
>  hw/intc/arm_gic.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index b30cc91..85be6e4 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -147,7 +147,8 @@ static void gic_set_irq_11mpcore(GICState *s, int irq, int level,
>  {
>      if (level) {
>          GIC_SET_LEVEL(irq, cm);
> -        if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)) {
> +        if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)
> +                || (!GIC_TEST_ACTIVE(irq, cm) &&  s->revision == REV_NVIC)) {
>              DPRINTF("Set %d pending mask %x\n", irq, target);
>              GIC_SET_PENDING(irq, target);
>          }
> --

Thanks for this patch. I agree that the current behaviour isn't
correct.

I think it would be cleaner to define a new gic_set_irq_nvic() which
has the NVIC specific behaviour, rather than sticking an "if this
is really an NVIC" check into this function.

You probably also want to check whether the logic for re-pending
level triggered interrupts in gic_complete_irq() also needs a
similar change:

    if (s->revision == REV_11MPCORE || s->revision == REV_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);
        }
    }

thanks
-- PMM

Re: [Qemu-devel] [PATCH] nvic: allow to set pending status for not active interrupts

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

Hi Peter

> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Thursday, October 13, 2016 6:59 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: [PATCH] nvic: allow to set pending status for not active
> interrupts
> 
> On 7 October 2016 at 10:42,  <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>
> > ---
> >  hw/intc/arm_gic.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index
> > b30cc91..85be6e4 100644
> > --- a/hw/intc/arm_gic.c
> > +++ b/hw/intc/arm_gic.c
> > @@ -147,7 +147,8 @@ static void gic_set_irq_11mpcore(GICState *s, int
> > irq, int level,  {
> >      if (level) {
> >          GIC_SET_LEVEL(irq, cm);
> > -        if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)) {
> > +        if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)
> > +                || (!GIC_TEST_ACTIVE(irq, cm) &&  s->revision ==
> > + REV_NVIC)) {
> >              DPRINTF("Set %d pending mask %x\n", irq, target);
> >              GIC_SET_PENDING(irq, target);
> >          }
> > --
> 
> Thanks for this patch. I agree that the current behaviour isn't correct.
> 
> I think it would be cleaner to define a new gic_set_irq_nvic() which has the
> NVIC specific behaviour, rather than sticking an "if this is really an NVIC"
> check into this function.
>
Sure.

> You probably also want to check whether the logic for re-pending level
> triggered interrupts in gic_complete_irq() also needs a similar change:
> 
>     if (s->revision == REV_11MPCORE || s->revision == REV_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);
>         }
>     }
It seem thet level triggered interrupt could be removed from fi for NVIC.
I do not have guest to model that, but I will at least check if this
do not brake anything.

Thanks,
Marcin 
> 
> thanks
> -- PMM