[PATCH] xen/arm: gic: GICv2 & GICv3 only supports 1020 physical interrupts

[PATCH] xen/arm: gic: GICv2 & GICv3 only supports 1020 physical interrupts

[Julien Grall]

GICD_TYPER.ITLinesNumber can encode up to 1024 interrupts. Although,
IRQ 1020-1023 are reserved for special purpose.

This helps to check when an IRQ is valid or not.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/gic-v2.c | 16 ++++++++++------
 xen/arch/arm/gic-v3.c | 16 ++++++++++------
 2 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 20cdbc9..826a457 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -256,6 +256,7 @@ static void __init gicv2_dist_init(void)
     uint32_t type;
     uint32_t cpumask;
     uint32_t gic_cpus;
+    unsigned int nr_lines;
     int i;
 
     cpumask = readl_gicd(GICD_ITARGETSR) & 0xff;
@@ -266,31 +267,34 @@ static void __init gicv2_dist_init(void)
     writel_gicd(0, GICD_CTLR);
 
     type = readl_gicd(GICD_TYPER);
-    gicv2_info.nr_lines = 32 * ((type & GICD_TYPE_LINES) + 1);
+    nr_lines = 32 * ((type & GICD_TYPE_LINES) + 1);
     gic_cpus = 1 + ((type & GICD_TYPE_CPUS) >> 5);
     printk("GICv2: %d lines, %d cpu%s%s (IID %8.8x).\n",
-           gicv2_info.nr_lines, gic_cpus, (gic_cpus == 1) ? "" : "s",
+           nr_lines, gic_cpus, (gic_cpus == 1) ? "" : "s",
            (type & GICD_TYPE_SEC) ? ", secure" : "",
            readl_gicd(GICD_IIDR));
 
     /* Default all global IRQs to level, active low */
-    for ( i = 32; i < gicv2_info.nr_lines; i += 16 )
+    for ( i = 32; i < nr_lines; i += 16 )
         writel_gicd(0x0, GICD_ICFGR + (i / 16) * 4);
 
     /* Route all global IRQs to this CPU */
-    for ( i = 32; i < gicv2_info.nr_lines; i += 4 )
+    for ( i = 32; i < nr_lines; i += 4 )
         writel_gicd(cpumask, GICD_ITARGETSR + (i / 4) * 4);
 
     /* Default priority for global interrupts */
-    for ( i = 32; i < gicv2_info.nr_lines; i += 4 )
+    for ( i = 32; i < nr_lines; i += 4 )
         writel_gicd(GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 |
                     GIC_PRI_IRQ << 8 | GIC_PRI_IRQ,
                     GICD_IPRIORITYR + (i / 4) * 4);
 
     /* Disable all global interrupts */
-    for ( i = 32; i < gicv2_info.nr_lines; i += 32 )
+    for ( i = 32; i < nr_lines; i += 32 )
         writel_gicd(~0x0, GICD_ICENABLER + (i / 32) * 4);
 
+    /* Only 1020 interrupts are supported */
+    gicv2_info.nr_lines = min((unsigned)1020, nr_lines);
+
     /* Turn on the distributor */
     writel_gicd(GICD_CTL_ENABLE, GICD_CTLR);
 }
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index ab80670..99a3b9a 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -528,23 +528,24 @@ static void __init gicv3_dist_init(void)
     uint32_t type;
     uint32_t priority;
     uint64_t affinity;
+    unsigned int nr_lines;
     int i;
 
     /* Disable the distributor */
     writel_relaxed(0, GICD + GICD_CTLR);
 
     type = readl_relaxed(GICD + GICD_TYPER);
-    gicv3_info.nr_lines = 32 * ((type & GICD_TYPE_LINES) + 1);
+    nr_lines = 32 * ((type & GICD_TYPE_LINES) + 1);
 
     printk("GICv3: %d lines, (IID %8.8x).\n",
-           gicv3_info.nr_lines, readl_relaxed(GICD + GICD_IIDR));
+           nr_lines, readl_relaxed(GICD + GICD_IIDR));
 
     /* Default all global IRQs to level, active low */
-    for ( i = NR_GIC_LOCAL_IRQS; i < gicv3_info.nr_lines; i += 16 )
+    for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i += 16 )
         writel_relaxed(0, GICD + GICD_ICFGR + (i / 16) * 4);
 
     /* Default priority for global interrupts */
-    for ( i = NR_GIC_LOCAL_IRQS; i < gicv3_info.nr_lines; i += 4 )
+    for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i += 4 )
     {
         priority = (GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 |
                     GIC_PRI_IRQ << 8 | GIC_PRI_IRQ);
@@ -552,7 +553,7 @@ static void __init gicv3_dist_init(void)
     }
 
     /* Disable all global interrupts */
-    for ( i = NR_GIC_LOCAL_IRQS; i < gicv3_info.nr_lines; i += 32 )
+    for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i += 32 )
         writel_relaxed(0xffffffff, GICD + GICD_ICENABLER + (i / 32) * 4);
 
     gicv3_dist_wait_for_rwp();
@@ -566,8 +567,11 @@ static void __init gicv3_dist_init(void)
     /* Make sure we don't broadcast the interrupt */
     affinity &= ~GICD_IROUTER_SPI_MODE_ANY;
 
-    for ( i = NR_GIC_LOCAL_IRQS; i < gicv3_info.nr_lines; i++ )
+    for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i++ )
         writeq_relaxed(affinity, GICD + GICD_IROUTER + i * 8);
+
+    /* Only 1020 interrupts are supported */
+    gicv3_info.nr_lines = min((unsigned)1020, nr_lines);
 }
 
 static int gicv3_enable_redist(void)
-- 
2.1.4

Re: [PATCH] xen/arm: gic: GICv2 & GICv3 only supports 1020 physical interrupts

[Ian Campbell]

On Tue, 2015-03-03 at 16:35 +0000, Julien Grall wrote:
> +    gicv3_info.nr_lines = min((unsigned)1020, nr_lines);

"1020U" is the correct way to write (unsigned)1020 I think (in both
places).

Otherwise looks ok, although I had to look twice to figure out that the
register initialisation was the same afterwards.

Where does this value get used? Can you spell it out in the commit log
please.

In particular are you sure that there are no usages which assume this is
a multiple of 32?

Ian.

Re: [PATCH] xen/arm: gic: GICv2 & GICv3 only supports 1020 physical interrupts

[Julien Grall]

Hi Ian,

On 05/03/2015 19:00, Ian Campbell wrote:
> On Tue, 2015-03-03 at 16:35 +0000, Julien Grall wrote:
>> +    gicv3_info.nr_lines = min((unsigned)1020, nr_lines);
>
> "1020U" is the correct way to write (unsigned)1020 I think (in both
> places).

I gave a look on several usage of min in arch/arm and (unsigned) was used.

Anyway, I guess 1020U is fine. I will use it.

>
> Otherwise looks ok, although I had to look twice to figure out that the
> register initialisation was the same afterwards.
>
> Where does this value get used? Can you spell it out in the commit log
> please.

This value is used to check if we can route the IRQ to Xen/a Guest.

For instance, using IRQ 1023-1024 in the LRs is unpredictable. So we 
have to catch it earlier. Even though, the hardware domain should not 
have the permission to control it.

> In particular are you sure that there are no usages which assume this is
> a multiple of 32?

The only place where we require a multiple of 32 is the vGIC emulation. 
Although, I have a patch with use DIV_ROUND_UP but I forgot to send with 
this patch.

I will resend both together.

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: gic: GICv2 & GICv3 only supports 1020 physical interrupts

[Ian Campbell]

On Mon, 2015-03-09 at 14:00 +0200, Julien Grall wrote:
> Hi Ian,
> 
> On 05/03/2015 19:00, Ian Campbell wrote:
> > On Tue, 2015-03-03 at 16:35 +0000, Julien Grall wrote:
> >> +    gicv3_info.nr_lines = min((unsigned)1020, nr_lines);
> >
> > "1020U" is the correct way to write (unsigned)1020 I think (in both
> > places).
> 
> I gave a look on several usage of min in arch/arm and (unsigned) was used.

I don't see any with a literal number, which is the main point.
(unsigned)PAGE_SIZE is fine, because you can't write PAGE_SIZEU very
easily. (Leaving aside whether PAGE_SIZE should be unsigned in its
definition.
Ian.

Re: [PATCH] xen/arm: gic: GICv2 & GICv3 only supports 1020 physical interrupts

[Julien Grall]

Hi Ian,

On 09/03/15 16:06, Ian Campbell wrote:
> On Mon, 2015-03-09 at 14:00 +0200, Julien Grall wrote:
>> Hi Ian,
>>
>> On 05/03/2015 19:00, Ian Campbell wrote:
>>> On Tue, 2015-03-03 at 16:35 +0000, Julien Grall wrote:
>>>> +    gicv3_info.nr_lines = min((unsigned)1020, nr_lines);
>>>
>>> "1020U" is the correct way to write (unsigned)1020 I think (in both
>>> places).
>>
>> I gave a look on several usage of min in arch/arm and (unsigned) was used.
> 
> I don't see any with a literal number, which is the main point.
> (unsigned)PAGE_SIZE is fine, because you can't write PAGE_SIZEU very
> easily. (Leaving aside whether PAGE_SIZE should be unsigned in its
> definition.

Hmmm ... right. I will update the patch.

Regards,

-- 
Julien Grall