Re: [PATCH] xen/arm: Missing +1 when then number of interrupt lines for the GIC is computed

[Julien Grall <julien.grall@linaro.org>]

On 04/25/2013 09:10 AM, Ian Campbell wrote:

> On Wed, 2013-04-24 at 20:44 +0100, Julien Grall wrote:
>> In the GIC manual, the number of interrupt lines is computed with the following
>> formula: 32(N + 1) where N is the value retrieved from GICD_TYPER.
> 
> My copy of the manual says "The ITLinesNumber field only indicates the
> maximum number of SPIs that the GIC might support", which excludes SGIs
> and PPIs. On the other hand it also says that 0b0011 == 128 interrupts,
> with ID 0..127, and elsewhere it includes SPI and PPI in the term
> interrupts.
> 
> So it's not really clear, but I think your interpretation is likely
> correct.

Perhaps we need to add a comment to describe the "lines" field to avoid
confusion later.

> The impact of getting this count wrong is that currently we don't
> initialise the final 32 interrupts worth of the GICD_FOOn registers, is
> that right?


Right.

> I was concerned that we special case the first 32 SG/PPI interrupts in
> various other places, but we don't appear to use gic.lines anywhere
> other than that.
> 
> BTW, I also happened across section 3.1.2 "Identifying the supported
> interrupts" which recommends probing GICD_ISENABLERn to determine which
> of the 32*(N+1) interrupts are actually available us, might be one for
> the todo list (would lead to a useful debugging message "request_irq:
> IRQ%d is not available").
> 
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
>> ---
>>  xen/arch/arm/gic.c |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 760c86b..389c217 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -216,7 +216,7 @@ static int gic_route_irq(unsigned int irq, bool_t level,
>>  
>>      ASSERT(!(cpu_mask & ~0xff));  /* Targets bitmap only supports 8 CPUs */
>>      ASSERT(priority <= 0xff);     /* Only 8 bits of priority */
>> -    ASSERT(irq < gic.lines + 32); /* Can't route interrupts that don't exist */
>> +    ASSERT(irq < gic.lines);      /* Can't route interrupts that don't exist */
>>  
>>      spin_lock_irqsave(&desc->lock, flags);
>>      spin_lock(&gic.lock);
>> @@ -264,7 +264,7 @@ static void __init gic_dist_init(void)
>>      GICD[GICD_CTLR] = 0;
>>  
>>      type = GICD[GICD_TYPER];
>> -    gic.lines = 32 * (type & GICD_TYPE_LINES);
>> +    gic.lines = 32 * ((type & GICD_TYPE_LINES) + 1);
>>      gic.cpus = 1 + ((type & GICD_TYPE_CPUS) >> 5);
>>      printk("GIC: %d lines, %d cpu%s%s (IID %8.8x).\n",
>>             gic.lines, gic.cpus, (gic.cpus == 1) ? "" : "s",
> 
>