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

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

[Julien Grall]

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

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

[Ian Campbell]

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

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

[Julien Grall]

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

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

[Ian Campbell]

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.