Xen devel - Apr 2013 - [PATCH] xen/arm: Missing +1 when then number of interrupt lines for the GIC is computed

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.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 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",
-- 
1.7.10.4

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.

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?

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",

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",
> 
>

On Thu, 2013-04-25 at 10:52 +0100, Julien Grall wrote:
> 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.
Yes, please.
> > 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.
If you are respinning please could you mention this in the changelog,
it''s the headline feature of the patch really ;-)

Ian.