diff mbox

[Android-virt,02/12] arm: make the number of GIC interrupts configurable

Message ID 87fwf5ebjw.fsf@rustcorp.com.au
State Superseded
Headers show

Commit Message

Rusty Russell Jan. 24, 2012, 8:42 a.m. UTC
On Fri, 13 Jan 2012 20:52:39 +0000, Peter Maydell <peter.maydell@linaro.org> wrote:
> From: Mark Langsdorf <mark.langsdorf@calxeda.com>
> 
> Increase the maximum number of GIC interrupts for a9mp and a11mp to 1020,
> and create a configurable property for each defaulting to 96 and 64
> (respectively) so that device modelers can set the value appropriately
> for their SoC. Other ARM processors also set their maximum number of
> used IRQs appropriately.
> 
> Set the maximum theoretical number of GIC interrupts to 1020 and
> update the save/restore code to only use the appropriate number for
> each SoC.

Reading through this, I see a lot of "- 32".  Trivial patch follows,
which applies to your rebasing branch:

Subject: ARM: clean up GIC constants.
From: Rusty Russell <rusty@rustcorp.com.au>

Interrupts numbers 0-31 are private to the processor interface, 32-1019 are
general interrups.  Add GIC_INTERNAL and substitute everywhere.

Also, add a check that the total number of interrupts is divisible by
32 (required for reporting interupt numbers, see gic_dist_readb(), and
is greater than 32.  And remove a single stray tab.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 hw/arm_gic.c |   48 ++++++++++++++++++++++++++----------------------
 1 files changed, 26 insertions(+), 22 deletions(-)

Comments

Peter Maydell Jan. 25, 2012, 3:09 p.m. UTC | #1
On 24 January 2012 08:42, Rusty Russell <rusty@rustcorp.com.au> wrote:
> On Fri, 13 Jan 2012 20:52:39 +0000, Peter Maydell <peter.maydell@linaro.org> wrote:
>> From: Mark Langsdorf <mark.langsdorf@calxeda.com>
>>
>> Increase the maximum number of GIC interrupts for a9mp and a11mp to 1020,
>> and create a configurable property for each defaulting to 96 and 64
>> (respectively) so that device modelers can set the value appropriately
>> for their SoC. Other ARM processors also set their maximum number of
>> used IRQs appropriately.
>>
>> Set the maximum theoretical number of GIC interrupts to 1020 and
>> update the save/restore code to only use the appropriate number for
>> each SoC.
>
> Reading through this, I see a lot of "- 32".  Trivial patch follows,
> which applies to your rebasing branch:

(If you send patches as fresh new emails then they just apply
with git am without needing manual cleanup, appear with sensible
subjects in patchwork/patches.linaro, etc.)

> Subject: ARM: clean up GIC constants.
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> Interrupts numbers 0-31 are private to the processor interface, 32-1019 are
> general interrups.  Add GIC_INTERNAL and substitute everywhere.
>
> Also, add a check that the total number of interrupts is divisible by
> 32 (required for reporting interupt numbers, see gic_dist_readb(), and
> is greater than 32.  And remove a single stray tab.

I agree with Avi that the presence of "Also" in a git
commit message is generally a sign you should have submitted
more than one patch :-)

> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  hw/arm_gic.c |   48 ++++++++++++++++++++++++++----------------------
>  1 files changed, 26 insertions(+), 22 deletions(-)
>
> diff --git a/hw/arm_gic.c b/hw/arm_gic.c
> index cf582a5..a29eacb 100644
> --- a/hw/arm_gic.c
> +++ b/hw/arm_gic.c
> @@ -13,6 +13,8 @@
>
>  /* Maximum number of possible interrupts, determined by the GIC architecture */
>  #define GIC_MAXIRQ 1020
> +/* First 32 are private to each CPU (SGIs and PPIs). */
> +#define GIC_INTERNAL 32
>  //#define DEBUG_GIC
>
>  #ifdef DEBUG_GIC
> @@ -74,7 +76,7 @@ typedef struct gic_irq_state
>  #define GIC_CLEAR_TRIGGER(irq) s->irq_state[irq].trigger = 0
>  #define GIC_TEST_TRIGGER(irq) s->irq_state[irq].trigger
>  #define GIC_GET_PRIORITY(irq, cpu) \
> -  (((irq) < 32) ? s->priority1[irq][cpu] : s->priority2[(irq) - 32])
> +  (((irq) < GIC_INTERNAL) ? s->priority1[irq][cpu] : s->priority2[(irq) - GIC_INTERNAL])

This line is now >80 characters and needs folding.
(scripts/checkpatch.pl will catch this kind of nit.)

> @@ -812,11 +814,13 @@ static void gic_init(gic_state *s, int num_irq)
>     s->num_cpu = num_cpu;
>  #endif
>     s->num_irq = num_irq + GIC_BASE_IRQ;
> -    if (s->num_irq > GIC_MAXIRQ) {
> -        hw_error("requested %u interrupt lines exceeds GIC maximum %d\n",
> -                 num_irq, GIC_MAXIRQ);
> +    if (s->num_irq > GIC_MAXIRQ
> +        || s->num_irq < GIC_INTERNAL
> +        || (s->num_irq % 32) != 0) {

So I guess our implementation isn't likely to work properly for a
non-multiple-of-32 number of IRQs, but this isn't an architectural GIC
restriction. (In fact the GIC architecture spec allows the supported
interrupt IDs to not even be in a contiguous range, which we certainly
don't support...)
I also think it's architecturally permitted that not all the internal
(SPI/PPI) interrupts are implemented, ie that s->num_irq < 32 (you
have to read the GIC architecture manually quite closely to deduce
this, though).

Anyway, if we would otherwise die horribly later on we should
catch these cases, but it would be good to have at least a comment
saying that these are implementation limitations rather than
architectural ones.

Beefing up the parameter check should be a separate patch.

Otherwise OK.

-- PMM
Rusty Russell Jan. 27, 2012, 12:33 a.m. UTC | #2
On Wed, 25 Jan 2012 15:09:41 +0000, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 24 January 2012 08:42, Rusty Russell <rusty@rustcorp.com.au> wrote:
> > Reading through this, I see a lot of "- 32".  Trivial patch follows,
> > which applies to your rebasing branch:
> 
> (If you send patches as fresh new emails then they just apply
> with git am without needing manual cleanup, appear with sensible
> subjects in patchwork/patches.linaro, etc.)

Indeed, but it's so conversaionally gauche :(  I thought git am did the
Right Thing, but it doesn't, and --scissors doesn't help either (it gets
the wrong Subject line).  Oh well, I'll do it that way in future.

> > Subject: ARM: clean up GIC constants.
> > From: Rusty Russell <rusty@rustcorp.com.au>
> >
> > Interrupts numbers 0-31 are private to the processor interface, 32-1019 are
> > general interrups.  Add GIC_INTERNAL and substitute everywhere.
> >
> > Also, add a check that the total number of interrupts is divisible by
> > 32 (required for reporting interupt numbers, see gic_dist_readb(), and
> > is greater than 32.  And remove a single stray tab.
> 
> I agree with Avi that the presence of "Also" in a git
> commit message is generally a sign you should have submitted
> more than one patch :-)

Indeed, guilty as charged :)

> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > ---
> >  hw/arm_gic.c |   48 ++++++++++++++++++++++++++----------------------
> >  1 files changed, 26 insertions(+), 22 deletions(-)
> >
> > diff --git a/hw/arm_gic.c b/hw/arm_gic.c
> > index cf582a5..a29eacb 100644
> > --- a/hw/arm_gic.c
> > +++ b/hw/arm_gic.c
> > @@ -13,6 +13,8 @@
> >
> >  /* Maximum number of possible interrupts, determined by the GIC architecture */
> >  #define GIC_MAXIRQ 1020
> > +/* First 32 are private to each CPU (SGIs and PPIs). */
> > +#define GIC_INTERNAL 32
> >  //#define DEBUG_GIC
> >
> >  #ifdef DEBUG_GIC
> > @@ -74,7 +76,7 @@ typedef struct gic_irq_state
> >  #define GIC_CLEAR_TRIGGER(irq) s->irq_state[irq].trigger = 0
> >  #define GIC_TEST_TRIGGER(irq) s->irq_state[irq].trigger
> >  #define GIC_GET_PRIORITY(irq, cpu) \
> > -  (((irq) < 32) ? s->priority1[irq][cpu] : s->priority2[(irq) - 32])
> > +  (((irq) < GIC_INTERNAL) ? s->priority1[irq][cpu] : s->priority2[(irq) - GIC_INTERNAL])
> 
> This line is now >80 characters and needs folding.
> (scripts/checkpatch.pl will catch this kind of nit.)

Will do.

> > @@ -812,11 +814,13 @@ static void gic_init(gic_state *s, int num_irq)
> >     s->num_cpu = num_cpu;
> >  #endif
> >     s->num_irq = num_irq + GIC_BASE_IRQ;
> > -    if (s->num_irq > GIC_MAXIRQ) {
> > -        hw_error("requested %u interrupt lines exceeds GIC maximum %d\n",
> > -                 num_irq, GIC_MAXIRQ);
> > +    if (s->num_irq > GIC_MAXIRQ
> > +        || s->num_irq < GIC_INTERNAL
> > +        || (s->num_irq % 32) != 0) {
> 
> So I guess our implementation isn't likely to work properly for a
> non-multiple-of-32 number of IRQs, but this isn't an architectural GIC
> restriction. (In fact the GIC architecture spec allows the supported
> interrupt IDs to not even be in a contiguous range, which we certainly
> don't support...)

Yes, I intuited it from here:

static uint32_t gic_dist_readb(void *opaque, target_phys_addr_t offset)
{
...
        if (offset == 4)
            return ((s->num_irq / 32) - 1) | ((NUM_CPU(s) - 1) << 5);

If want want to support non-32-divisible # irqs, we need at least:

        ((s->num_irq + 31) / 32 - 1)

Seemed easier to have an initialization-time assert than check
everywhere else for overflows.

> I also think it's architecturally permitted that not all the internal
> (SPI/PPI) interrupts are implemented, ie that s->num_irq < 32 (you
> have to read the GIC architecture manually quite closely to deduce
> this, though).

This made me read that part of the manual... interesting.

> Anyway, if we would otherwise die horribly later on we should
> catch these cases, but it would be good to have at least a comment
> saying that these are implementation limitations rather than
> architectural ones.

Good point.  If we add an "supported" bit to each irq, we could do weird
things, but presumably ->num_irq would still correspond to
ITLinesNumber.

I don't want to put too much of an essay in there.  How's this:

        /* ITLinesNumber is represented as (N - 32) / 1.  See
          gic_dist_readb. */
        if (s->num_irq < 32 || (s->num_irq % 32)) {
                hw_error("%u interrupt lines unsupported: not divisible by 32\n",
                         num_irq);
                

> Beefing up the parameter check should be a separate patch.

Thanks, coming soon.

I should be getting access to git.linaro.org RSN, so I can post there
for easier merging.

Thanks,
Rusty.
Peter Maydell Jan. 27, 2012, 9:01 a.m. UTC | #3
On 27 January 2012 00:33, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Peter Maydell wrote:
>> Anyway, if we would otherwise die horribly later on we should
>> catch these cases, but it would be good to have at least a comment
>> saying that these are implementation limitations rather than
>> architectural ones.
>
> Good point.  If we add an "supported" bit to each irq, we could do weird
> things, but presumably ->num_irq would still correspond to
> ITLinesNumber.
>
> I don't want to put too much of an essay in there.  How's this:
>
>        /* ITLinesNumber is represented as (N - 32) / 1.  See
>          gic_dist_readb. */
>        if (s->num_irq < 32 || (s->num_irq % 32)) {
>                hw_error("%u interrupt lines unsupported: not divisible by 32\n",
>                         num_irq);

I think that's a notch too terse for my taste. How about:

        /* ITLinesNumber is represented as (N - 32) / 1 (see
         * gic_dist_readb) so this is an implementation imposed
         * restriction, not an architectural one:
         */

thanks
-- PMM
diff mbox

Patch

diff --git a/hw/arm_gic.c b/hw/arm_gic.c
index cf582a5..a29eacb 100644
--- a/hw/arm_gic.c
+++ b/hw/arm_gic.c
@@ -13,6 +13,8 @@ 
 
 /* Maximum number of possible interrupts, determined by the GIC architecture */
 #define GIC_MAXIRQ 1020
+/* First 32 are private to each CPU (SGIs and PPIs). */
+#define GIC_INTERNAL 32
 //#define DEBUG_GIC
 
 #ifdef DEBUG_GIC
@@ -74,7 +76,7 @@  typedef struct gic_irq_state
 #define GIC_CLEAR_TRIGGER(irq) s->irq_state[irq].trigger = 0
 #define GIC_TEST_TRIGGER(irq) s->irq_state[irq].trigger
 #define GIC_GET_PRIORITY(irq, cpu) \
-  (((irq) < 32) ? s->priority1[irq][cpu] : s->priority2[(irq) - 32])
+  (((irq) < GIC_INTERNAL) ? s->priority1[irq][cpu] : s->priority2[(irq) - GIC_INTERNAL])
 #ifdef NVIC
 #define GIC_TARGET(irq) 1
 #else
@@ -92,8 +94,8 @@  typedef struct gic_state
 #ifndef NVIC
     int irq_target[GIC_MAXIRQ];
 #endif
-    int priority1[32][NCPU];
-    int priority2[GIC_MAXIRQ - 32];
+    int priority1[GIC_INTERNAL][NCPU];
+    int priority2[GIC_MAXIRQ - GIC_INTERNAL];
     int last_active[GIC_MAXIRQ][NCPU];
 
     int priority_mask[NCPU];
@@ -131,7 +133,7 @@  static void gic_update(gic_state *s)
         cm = 1 << cpu;
         s->current_pending[cpu] = 1023;
         if (!s->enabled || !s->cpu_enabled[cpu]) {
-	    qemu_irq_lower(s->parent_irq[cpu]);
+            qemu_irq_lower(s->parent_irq[cpu]);
             return;
         }
         best_prio = 0x100;
@@ -174,7 +176,7 @@  static void gic_set_irq(void *opaque, int irq, int level)
 {
     gic_state *s = (gic_state *)opaque;
     /* The first external input line is internal interrupt 32.  */
-    irq += 32;
+    irq += GIC_INTERNAL;
     if (level == GIC_TEST_LEVEL(irq, ALL_CPU_MASK))
         return;
 
@@ -316,7 +318,7 @@  static uint32_t gic_dist_readb(void *opaque, target_phys_addr_t offset)
         if (irq >= s->num_irq)
             goto bad_reg;
         res = 0;
-        mask = (irq < 32) ?  cm : ALL_CPU_MASK;
+        mask = (irq < GIC_INTERNAL) ?  cm : ALL_CPU_MASK;
         for (i = 0; i < 8; i++) {
             if (GIC_TEST_PENDING(irq + i, mask)) {
                 res |= (1 << i);
@@ -328,7 +330,7 @@  static uint32_t gic_dist_readb(void *opaque, target_phys_addr_t offset)
         if (irq >= s->num_irq)
             goto bad_reg;
         res = 0;
-        mask = (irq < 32) ?  cm : ALL_CPU_MASK;
+        mask = (irq < GIC_INTERNAL) ?  cm : ALL_CPU_MASK;
         for (i = 0; i < 8; i++) {
             if (GIC_TEST_ACTIVE(irq + i, mask)) {
                 res |= (1 << i);
@@ -435,8 +437,8 @@  static void gic_dist_writeb(void *opaque, target_phys_addr_t offset,
           value = 0xff;
         for (i = 0; i < 8; i++) {
             if (value & (1 << i)) {
-                int mask = (irq < 32) ? (1 << cpu) : GIC_TARGET(irq);
-                int cm = (irq < 32) ? (1 << cpu) : ALL_CPU_MASK;
+                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);
@@ -460,7 +462,7 @@  static void gic_dist_writeb(void *opaque, target_phys_addr_t offset,
           value = 0;
         for (i = 0; i < 8; i++) {
             if (value & (1 << i)) {
-                int cm = (irq < 32) ? (1 << cpu) : ALL_CPU_MASK;
+                int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
 
                 if (GIC_TEST_ENABLED(irq + i, cm)) {
                     DPRINTF("Disabled IRQ %d\n", irq + i);
@@ -502,10 +504,10 @@  static void gic_dist_writeb(void *opaque, target_phys_addr_t offset,
         irq = (offset - 0x400) + GIC_BASE_IRQ;
         if (irq >= s->num_irq)
             goto bad_reg;
-        if (irq < 32) {
+        if (irq < GIC_INTERNAL) {
             s->priority1[irq][cpu] = value;
         } else {
-            s->priority2[irq - 32] = value;
+            s->priority2[irq - GIC_INTERNAL] = value;
         }
 #ifndef NVIC
     } else if (offset < 0xc00) {
@@ -515,7 +517,7 @@  static void gic_dist_writeb(void *opaque, target_phys_addr_t offset,
             goto bad_reg;
         if (irq < 29)
             value = 0;
-        else if (irq < 32)
+        else if (irq < GIC_INTERNAL)
             value = ALL_CPU_MASK;
         s->irq_target[irq] = value & ALL_CPU_MASK;
     } else if (offset < 0xf00) {
@@ -523,7 +525,7 @@  static void gic_dist_writeb(void *opaque, target_phys_addr_t offset,
         irq = (offset - 0xc00) * 4 + GIC_BASE_IRQ;
         if (irq >= s->num_irq)
             goto bad_reg;
-        if (irq < 32)
+        if (irq < GIC_INTERNAL)
             value |= 0xaa;
         for (i = 0; i < 4; i++) {
             if (value & (1 << (i * 2))) {
@@ -736,7 +738,7 @@  static void gic_save(QEMUFile *f, void *opaque)
     qemu_put_be32(f, s->enabled);
     for (i = 0; i < NUM_CPU(s); i++) {
         qemu_put_be32(f, s->cpu_enabled[i]);
-        for (j = 0; j < 32; j++)
+        for (j = 0; j < GIC_INTERNAL; j++)
             qemu_put_be32(f, s->priority1[j][i]);
         for (j = 0; j < s->num_irq; j++)
             qemu_put_be32(f, s->last_active[j][i]);
@@ -745,7 +747,7 @@  static void gic_save(QEMUFile *f, void *opaque)
         qemu_put_be32(f, s->running_priority[i]);
         qemu_put_be32(f, s->current_pending[i]);
     }
-    for (i = 0; i < s->num_irq - 32; i++) {
+    for (i = 0; i < s->num_irq - GIC_INTERNAL; i++) {
         qemu_put_be32(f, s->priority2[i]);
     }
     for (i = 0; i < s->num_irq; i++) {
@@ -773,7 +775,7 @@  static int gic_load(QEMUFile *f, void *opaque, int version_id)
     s->enabled = qemu_get_be32(f);
     for (i = 0; i < NUM_CPU(s); i++) {
         s->cpu_enabled[i] = qemu_get_be32(f);
-        for (j = 0; j < 32; j++)
+        for (j = 0; j < GIC_INTERNAL; j++)
             s->priority1[j][i] = qemu_get_be32(f);
         for (j = 0; j < s->num_irq; j++)
             s->last_active[j][i] = qemu_get_be32(f);
@@ -782,7 +784,7 @@  static int gic_load(QEMUFile *f, void *opaque, int version_id)
         s->running_priority[i] = qemu_get_be32(f);
         s->current_pending[i] = qemu_get_be32(f);
     }
-    for (i = 0; i < s->num_irq - 32; i++) {
+    for (i = 0; i < s->num_irq - GIC_INTERNAL; i++) {
         s->priority2[i] = qemu_get_be32(f);
     }
     for (i = 0; i < s->num_irq; i++) {
@@ -812,11 +814,13 @@  static void gic_init(gic_state *s, int num_irq)
     s->num_cpu = num_cpu;
 #endif
     s->num_irq = num_irq + GIC_BASE_IRQ;
-    if (s->num_irq > GIC_MAXIRQ) {
-        hw_error("requested %u interrupt lines exceeds GIC maximum %d\n",
-                 num_irq, GIC_MAXIRQ);
+    if (s->num_irq > GIC_MAXIRQ
+        || s->num_irq < GIC_INTERNAL
+        || (s->num_irq % 32) != 0) {
+        hw_error("requested %u interrupt lines outside GIC range %d-%d\n",
+                 num_irq, GIC_INTERNAL, GIC_MAXIRQ);
     }
-    qdev_init_gpio_in(&s->busdev.qdev, gic_set_irq, s->num_irq - 32);
+    qdev_init_gpio_in(&s->busdev.qdev, gic_set_irq, s->num_irq - GIC_INTERNAL);
     for (i = 0; i < NUM_CPU(s); i++) {
         sysbus_init_irq(&s->busdev, &s->parent_irq[i]);
     }