[Qemu-devel] [PATCH v2] target-arm: GIC: bug fixes for arm_gic.c

[Daniel Sangorrin]

Sorry, it seems that I did not understand the flow in the function
"hw/arm_gic.c:gic_dist_writeb()" and made some mistaken assumptions in
my previous patch. Please do not apply the previous patch, and apply
this one instead if you consider that it is correct.

target-arm: fix bug in irq value on arm_gic.c

Fix a small bug that was using an incorrect IRQ
value in the function gic_dist_writeb.

Signed-off-by: Daniel Sangorrin <address@hidden>
---
 hw/arm_gic.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm_gic.c b/hw/arm_gic.c
index f9e423f..64d4e23 100644
--- a/hw/arm_gic.c
+++ b/hw/arm_gic.c
@@ -374,7 +374,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
           value = 0xff;
         for (i = 0; i < 8; i++) {
             if (value & (1 << i)) {
-                int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : GIC_TARGET(irq);
+                int mask = (irq < GIC_INTERNAL) ? (1 << cpu) :
GIC_TARGET(irq + i);
                 int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;

                 if (!GIC_TEST_ENABLED(irq + i, cm)) {
@@ -417,7 +417,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,

         for (i = 0; i < 8; i++) {
             if (value & (1 << i)) {
-                GIC_SET_PENDING(irq + i, GIC_TARGET(irq));
+                GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i));
             }
         }
     } else if (offset < 0x300) {
-- 
1.7.9.5


On Fri, Dec 7, 2012 at 2:14 PM, Daniel Sangorrin
<address@hidden> wrote:
> Hi, I found some bugs in the way the IRQ number is calculated
> at certain places in arm_gic.c. Perhaps there are a few more
> errors that I didn't notice. These bugs were not noticeable
> when running Linux as a guest, but I found them when running
> my own porting of a multicore real-time OS (TOPPERS/FMP).
>
> NOTE: the main problem I had was that the target CPUs where not
> set correctly. Instead of GIC_SET_PENDING(irq + i, GIC_TARGET(irq));
> it should read as GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i));
>
> I have tested the patch both with FMP and with a recent Linux, but
> please review it since I'm not an expert on QEMU and this is my
> first patch.
>
> target-arm: bug fixes for arm_gic.c
>
> The IRQ number was not calculated correctly in the
> function gic_dist_writeb for accesses to set/clear
> pending and set/clear enable.
>
> Signed-off-by: Daniel Sangorrin <address@hidden>
> ---
>  hw/arm_gic.c |   90 
> ++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 50 insertions(+), 40 deletions(-)
>
> diff --git a/hw/arm_gic.c b/hw/arm_gic.c
> index f9e423f..f3f233c 100644
> --- a/hw/arm_gic.c
> +++ b/hw/arm_gic.c
> @@ -368,71 +368,81 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>      } else if (offset < 0x180) {
>          /* Interrupt Set Enable.  */
>          irq = (offset - 0x100) * 8 + GIC_BASE_IRQ;
> +        for (i = 0; i < 8; i++) {
> +                if (value & (1 << i)) {
> +                        irq = irq + i;
> +                        break;
> +                }
> +        }
>          if (irq >= s->num_irq)
>              goto bad_reg;
>          if (irq < 16)
> -          value = 0xff;
> -        for (i = 0; i < 8; i++) {
> -            if (value & (1 << i)) {
> -                int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : 
> GIC_TARGET(irq);
> -                int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
> +            value = 0xff;
> +        int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : GIC_TARGET(irq);
> +        int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
>
> -                if (!GIC_TEST_ENABLED(irq + i, cm)) {
> -                    DPRINTF("Enabled IRQ %d\n", irq + i);
> -                }
> -                GIC_SET_ENABLED(irq + i, cm);
> -                /* If a raised level triggered IRQ enabled then mark
> -                   is as pending.  */
> -                if (GIC_TEST_LEVEL(irq + i, mask)
> -                        && !GIC_TEST_TRIGGER(irq + i)) {
> -                    DPRINTF("Set %d pending mask %x\n", irq + i, mask);
> -                    GIC_SET_PENDING(irq + i, mask);
> -                }
> -            }
> +        if (!GIC_TEST_ENABLED(irq, cm)) {
> +            DPRINTF("Enabled IRQ %d\n", irq);
> +        }
> +        GIC_SET_ENABLED(irq, cm);
> +        /* If a raised level triggered IRQ enabled then mark is as pending */
> +        if (GIC_TEST_LEVEL(irq, mask) && !GIC_TEST_TRIGGER(irq)) {
> +            DPRINTF("Set %d pending mask %x\n", irq, mask);
> +            GIC_SET_PENDING(irq, mask);
>          }
>      } else if (offset < 0x200) {
>          /* Interrupt Clear Enable.  */
>          irq = (offset - 0x180) * 8 + GIC_BASE_IRQ;
> -        if (irq >= s->num_irq)
> -            goto bad_reg;
> -        if (irq < 16)
> -          value = 0;
>          for (i = 0; i < 8; i++) {
>              if (value & (1 << i)) {
> -                int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
> -
> -                if (GIC_TEST_ENABLED(irq + i, cm)) {
> -                    DPRINTF("Disabled IRQ %d\n", irq + i);
> -                }
> -                GIC_CLEAR_ENABLED(irq + i, cm);
> +                irq = irq + i;
> +                break;
>              }
>          }
> -    } else if (offset < 0x280) {
> -        /* Interrupt Set Pending.  */
> -        irq = (offset - 0x200) * 8 + GIC_BASE_IRQ;
>          if (irq >= s->num_irq)
>              goto bad_reg;
>          if (irq < 16)
> -          irq = 0;
> +            value = 0;
> +
> +        int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
>
> +        if (GIC_TEST_ENABLED(irq, cm)) {
> +            DPRINTF("Disabled IRQ %d\n", irq);
> +        }
> +        GIC_CLEAR_ENABLED(irq, cm);
> +    } else if (offset < 0x280) {
> +        /* Interrupt Set Pending.  */
> +        irq = (offset - 0x200) * 8 + GIC_BASE_IRQ;
>          for (i = 0; i < 8; i++) {
>              if (value & (1 << i)) {
> -                GIC_SET_PENDING(irq + i, GIC_TARGET(irq));
> +                irq = irq + i;
> +                break;
>              }
>          }
> +
> +        if (irq >= s->num_irq) {
> +            goto bad_reg;
> +        }
> +        if (irq < 16) {
> +            irq = 0;
> +        }
> +
> +        GIC_SET_PENDING(irq, GIC_TARGET(irq));
>      } else if (offset < 0x300) {
>          /* Interrupt Clear Pending.  */
>          irq = (offset - 0x280) * 8 + GIC_BASE_IRQ;
> -        if (irq >= s->num_irq)
> -            goto bad_reg;
>          for (i = 0; i < 8; i++) {
> -            /* ??? This currently clears the pending bit for all CPUs, even
> -               for per-CPU interrupts.  It's unclear whether this is the
> -               corect behavior.  */
> -            if (value & (1 << i)) {
> -                GIC_CLEAR_PENDING(irq + i, ALL_CPU_MASK);
> -            }
> +                if (value & (1 << i)) {
> +                        irq = irq + i;
> +                        break;
> +                }
>          }
> +
> +        if (irq >= s->num_irq) {
> +            goto bad_reg;
> +        }
> +
> +        GIC_CLEAR_PENDING(irq, ALL_CPU_MASK);
>      } else if (offset < 0x400) {
>          /* Interrupt Active.  */
>          goto bad_reg;
> --
> 1.7.9.5