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

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(-)

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

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.

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