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

[Peter Maydell]

On 24 January 2012 08:42, Rusty Russell <address@hidden> wrote:
> On Fri, 13 Jan 2012 20:52:39 +0000, Peter Maydell <address@hidden> wrote:
>> From: Mark Langsdorf <address@hidden>
>>
>> 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 <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 :-)

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

> @@ -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