Re: [Qemu-devel] [Android-virt] [PATCH 02/12] arm: make the number of GIC interrupts configurable

[Rusty Russell]

On Wed, 25 Jan 2012 15:09:41 +0000, Peter Maydell <address@hidden> wrote:
> On 24 January 2012 08:42, Rusty Russell <address@hidden> 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 <address@hidden>
> >
> > 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 <address@hidden>
> > ---
> >  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.