[PATCH v3 1/2] xen/arm: Divide GIC initialization in 2 parts

[PATCH v3 1/2] xen/arm: Divide GIC initialization in 2 parts

[Julien Grall]

Currently the function to translate IRQ from the device tree is set
unconditionally  to be able to be able to retrieve serial/timer IRQ before the
GIC has been initialized.

It assumes that the xlate function won't never changed. We may also need to
have the primary interrupt controller very early.

Rework the gic initialization in 2 parts:
    - gic_preinit: Get the interrupt controller device tree node and set
up GIC and xlate callbacks
    - gic_init: Initialize the interrupt controller and the boot CPU
    interrupts.

The former function will be called just after the IRQ subsystem as been
initialized.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v3:
        - Patch added.
---
 xen/arch/arm/gic-v2.c     |   27 +++++++++++++++++----------
 xen/arch/arm/gic.c        |   16 ++++++++++++++--
 xen/arch/arm/setup.c      |    3 ++-
 xen/include/asm-arm/gic.h |    8 ++++++++
 4 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index cc60af8..0762329 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -74,6 +74,7 @@ static struct {
     void __iomem * map_hbase; /* IO Address of virtual interface registers */
     paddr_t vbase;            /* Address of virtual cpu interface registers */
     spinlock_t lock;
+    struct dt_device_node *node;
 } gicv2;
 
 static struct gic_info gicv2_info;
@@ -560,8 +561,11 @@ static hw_irq_controller gicv2_guest_irq_type = {
     .set_affinity = gicv2_irq_set_affinity,
 };
 
+static int __init gicv2_init(void);
+
 const static struct gic_hw_operations gicv2_ops = {
     .info                = &gicv2_info,
+    .init                = gicv2_init,
     .secondary_init      = gicv2_secondary_cpu_init,
     .save_state          = gicv2_save_state,
     .restore_state       = gicv2_restore_state,
@@ -585,11 +589,20 @@ const static struct gic_hw_operations gicv2_ops = {
 };
 
 /* Set up the GIC */
-static int __init gicv2_init(struct dt_device_node *node, const void *data)
+static int __init gicv2_preinit(struct dt_device_node *node, const void *data)
 {
-    int res;
+    gicv2_info.hw_version = GIC_V2;
+    gicv2_info.node = node;
+    register_gic_ops(&gicv2_ops);
+    dt_irq_xlate = gic_irq_xlate;
 
-    dt_device_set_used_by(node, DOMID_XEN);
+    return 0;
+}
+
+static int __init gicv2_init(void)
+{
+    int res;
+    const struct dt_device_node *node = gicv2_info.node;
 
     res = dt_device_get_address(node, 0, &gicv2.dbase, NULL);
     if ( res || !gicv2.dbase || (gicv2.dbase & ~PAGE_MASK) )
@@ -612,9 +625,6 @@ static int __init gicv2_init(struct dt_device_node *node, const void *data)
         panic("GICv2: Cannot find the maintenance IRQ");
     gicv2_info.maintenance_irq = res;
 
-    /* Set the GIC as the primary interrupt controller */
-    dt_interrupt_controller = node;
-
     /* TODO: Add check on distributor, cpu size */
 
     printk("GICv2 initialization:\n"
@@ -656,9 +666,6 @@ static int __init gicv2_init(struct dt_device_node *node, const void *data)
 
     spin_unlock(&gicv2.lock);
 
-    gicv2_info.hw_version = GIC_V2;
-    register_gic_ops(&gicv2_ops);
-
     return 0;
 }
 
@@ -671,7 +678,7 @@ static const char * const gicv2_dt_compat[] __initconst =
 
 DT_DEVICE_START(gicv2, "GICv2:", DEVICE_GIC)
         .compatible = gicv2_dt_compat,
-        .init = gicv2_init,
+        .init = gicv2_preinit,
 DT_DEVICE_END
 
 /*
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 83b004c..91552fb 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -161,8 +161,10 @@ int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
     return 0;
 }
 
-/* Set up the GIC */
-void __init gic_init(void)
+/* Find the interrupt controller and set up the callback to translate
+ * device tree IRQ.
+ */
+void __init gic_preinit(void)
 {
     int rc;
     struct dt_device_node *node;
@@ -187,6 +189,16 @@ void __init gic_init(void)
     if ( !num_gics )
         panic("Unable to find compatible GIC in the device tree");
 
+    /* Set the GIC as the primary interrupt controller */
+    dt_interrupt_controller = node;
+    dt_device_set_used_by(node, DOMID_XEN);
+}
+
+/* Set up the GIC */
+void __init gic_init(void)
+{
+    if ( gic_hw_ops->init() )
+        panic("Failed to initialize the GIC drivers");
     /* Clear LR mask for cpu0 */
     clear_cpu_lr_mask();
 }
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 78dc7f5..36104fc 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -692,10 +692,11 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     vm_init();
     dt_unflatten_host_device_tree();
-    dt_irq_xlate = gic_irq_xlate;
 
     init_IRQ();
 
+    gic_preinit();
+
     dt_uart_init();
     console_init_preirq();
 
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index ed610cb..fec57b5 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -209,6 +209,10 @@ extern void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq);
 
 /* Accept an interrupt from the GIC and dispatch its handler */
 extern void gic_interrupt(struct cpu_user_regs *regs, int is_fiq);
+/* Find the interrupt controller and set up the callback to translate
+ * device tree IRQ.
+ */
+extern void gic_preinit(void);
 /* Bring up the interrupt controller, and report # cpus attached */
 extern void gic_init(void);
 /* Bring up a secondary CPU's per-CPU GIC interface */
@@ -261,11 +265,15 @@ struct gic_info {
     uint8_t nr_lrs;
     /* Maintenance irq number */
     unsigned int maintenance_irq;
+    /* Pointer to the device tree node representing the interrupt controller */
+    const struct dt_device_node *node;
 };
 
 struct gic_hw_operations {
     /* Hold GIC HW information */
     const struct gic_info *info;
+    /* Initialize the GIC and the boot CPU */
+    int (*init)(void);
     /* Save GIC registers */
     void (*save_state)(struct vcpu *);
     /* Restore GIC registers */
-- 
1.7.10.4

[PATCH v3 2/2] xen/dt: Allow only IRQ translation that are mapped to main GIC

[Julien Grall]

Xen is only able to handle one GIC controller. Some platform may contain
other interrupt controller.

Make sure to only translate IRQ mapped into the GIC handled by Xen.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v3:
        - Add an ASSERT to check that dt_interrupt_controller is not
        NULL.

    Changes in v2:
        - Fix compilation...
---
 xen/common/device_tree.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 310635e..cc45bd1 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -1442,9 +1442,12 @@ int dt_irq_translate(const struct dt_raw_irq *raw,
                      struct dt_irq *out_irq)
 {
     ASSERT(dt_irq_xlate != NULL);
+    ASSERT(dt_interrupt_controller != NULL);
 
-    /* TODO: Retrieve the right irq_xlate. This is only work for the gic */
+    if ( raw->controller != dt_interrupt_controller )
+        return -EINVAL;
 
+    /* TODO: Retrieve the right irq_xlate. This is only work for the gic */
     return dt_irq_xlate(raw->specifier, raw->size,
                         &out_irq->irq, &out_irq->type);
 }
-- 
1.7.10.4

Re: [PATCH v3 2/2] xen/dt: Allow only IRQ translation that are mapped to main GIC

[Ian Campbell]

On Wed, 2014-07-09 at 14:23 +0100, Julien Grall wrote:
> Xen is only able to handle one GIC controller. Some platform may contain
> other interrupt controller.
> 
> Make sure to only translate IRQ mapped into the GIC handled by Xen.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v3:
>         - Add an ASSERT to check that dt_interrupt_controller is not
>         NULL.
> 
>     Changes in v2:
>         - Fix compilation...
> ---
>  xen/common/device_tree.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 310635e..cc45bd1 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -1442,9 +1442,12 @@ int dt_irq_translate(const struct dt_raw_irq *raw,
>                       struct dt_irq *out_irq)
>  {
>      ASSERT(dt_irq_xlate != NULL);
> +    ASSERT(dt_interrupt_controller != NULL);
>  
> -    /* TODO: Retrieve the right irq_xlate. This is only work for the gic */
> +    if ( raw->controller != dt_interrupt_controller )
> +        return -EINVAL;
>  
> +    /* TODO: Retrieve the right irq_xlate. This is only work for the gic */

"This only works for ...". 

Do you mean it to say "primary gic"? In which case I think it was in the
correct location before (i.e. before the check which enforced that).

And you can probably say "primary interrupt controller" since this code
doesn't care if it is a gic or not so long as it has set the callback
correct.

Ian.

>      return dt_irq_xlate(raw->specifier, raw->size,
>                          &out_irq->irq, &out_irq->type);
>  }

Re: [PATCH v3 2/2] xen/dt: Allow only IRQ translation that are mapped to main GIC

[Julien Grall]

On 16/07/14 14:14, Ian Campbell wrote:
> On Wed, 2014-07-09 at 14:23 +0100, Julien Grall wrote:
>> Xen is only able to handle one GIC controller. Some platform may contain
>> other interrupt controller.
>>
>> Make sure to only translate IRQ mapped into the GIC handled by Xen.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>      Changes in v3:
>>          - Add an ASSERT to check that dt_interrupt_controller is not
>>          NULL.
>>
>>      Changes in v2:
>>          - Fix compilation...
>> ---
>>   xen/common/device_tree.c |    5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index 310635e..cc45bd1 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -1442,9 +1442,12 @@ int dt_irq_translate(const struct dt_raw_irq *raw,
>>                        struct dt_irq *out_irq)
>>   {
>>       ASSERT(dt_irq_xlate != NULL);
>> +    ASSERT(dt_interrupt_controller != NULL);
>>
>> -    /* TODO: Retrieve the right irq_xlate. This is only work for the gic */
>> +    if ( raw->controller != dt_interrupt_controller )
>> +        return -EINVAL;
>>
>> +    /* TODO: Retrieve the right irq_xlate. This is only work for the gic */
>
> "This only works for ...".
>
> Do you mean it to say "primary gic"? In which case I think it was in the
> correct location before (i.e. before the check which enforced that).

Which location are you talking about? The one in map_device?

If so, it doesn't protect anything, but only avoid to assign an IRQ to 
DOM0 which is not routed to the primary interrupt controller.

Checking the controller in the location will prevent platform_get_irq to 
translate IRQ not handled by the gic controller. Futhermore, it will 
avoid the new hypercall to list interrupt for a specific device node 
returning an invalid IRQ number.

Regards,

-- 
Julien Grall

Re: [PATCH v3 2/2] xen/dt: Allow only IRQ translation that are mapped to main GIC

[Ian Campbell]

On Wed, 2014-07-16 at 15:28 +0100, Julien Grall wrote:
> >> -    /* TODO: Retrieve the right irq_xlate. This is only work for the gic */
> >> +    if ( raw->controller != dt_interrupt_controller )
> >> +        return -EINVAL;
> >>
> >> +    /* TODO: Retrieve the right irq_xlate. This is only work for the gic */
> >
> > "This only works for ...".
> >
> > Do you mean it to say "primary gic"? In which case I think it was in the
> > correct location before (i.e. before the check which enforced that).
> 
> Which location are you talking about? The one in map_device?

I meant the place from where it is removed by this patch. I was trying
to say that the correct form would be (modulo the long line):

        /* TODO: Retrieve the right irq_xlate. This only works for the primary gic */
        if ( raw->controller != dt_interrupt_controller )
            return -EINVAL;

        return dt_...xlat(...)

i.e. the comment belongs with the check.

Ian.

Re: [PATCH v3 2/2] xen/dt: Allow only IRQ translation that are mapped to main GIC

[Julien Grall]

On 16/07/14 15:34, Ian Campbell wrote:
> On Wed, 2014-07-16 at 15:28 +0100, Julien Grall wrote:
>>>> -    /* TODO: Retrieve the right irq_xlate. This is only work for the gic */
>>>> +    if ( raw->controller != dt_interrupt_controller )
>>>> +        return -EINVAL;
>>>>
>>>> +    /* TODO: Retrieve the right irq_xlate. This is only work for the gic */
>>>
>>> "This only works for ...".
>>>
>>> Do you mean it to say "primary gic"? In which case I think it was in the
>>> correct location before (i.e. before the check which enforced that).
>>
>> Which location are you talking about? The one in map_device?
>
> I meant the place from where it is removed by this patch. I was trying
> to say that the correct form would be (modulo the long line):
>
>          /* TODO: Retrieve the right irq_xlate. This only works for the primary gic */
>          if ( raw->controller != dt_interrupt_controller )
>              return -EINVAL;
>
>          return dt_...xlat(...)
>
> i.e. the comment belongs with the check.

Oh ok. I will it before the check.

Regards,

-- 
Julien Grall

Re: [PATCH v3 1/2] xen/arm: Divide GIC initialization in 2 parts

[Ian Campbell]

On Wed, 2014-07-09 at 14:23 +0100, Julien Grall wrote:
> Currently the function to translate IRQ from the device tree is set
> unconditionally  to be able to be able to retrieve serial/timer IRQ before the
> GIC has been initialized.
> 
> It assumes that the xlate function won't never changed. We may also need to

"won't ever change".

> @@ -560,8 +561,11 @@ static hw_irq_controller gicv2_guest_irq_type = {
>      .set_affinity = gicv2_irq_set_affinity,
>  };
>  
> +static int __init gicv2_init(void);

I think you can reorder things to
	gic_init() { }
	ops
	gic_preinit()
to avoid the forward declaration.

> @@ -261,11 +265,15 @@ struct gic_info {
>      uint8_t nr_lrs;
>      /* Maintenance irq number */
>      unsigned int maintenance_irq;
> +    /* Pointer to the device tree node representing the interrupt controller */
> +    const struct dt_device_node *node;

You added this to both struct gic_info and gicv2_info?

Ian.

Re: [PATCH v3 1/2] xen/arm: Divide GIC initialization in 2 parts

[Julien Grall]

Hi Ian,

On 16/07/14 14:10, Ian Campbell wrote:
> On Wed, 2014-07-09 at 14:23 +0100, Julien Grall wrote:
>> Currently the function to translate IRQ from the device tree is set
>> unconditionally  to be able to be able to retrieve serial/timer IRQ before the
>> GIC has been initialized.
>>
>> It assumes that the xlate function won't never changed. We may also need to
>
> "won't ever change".
>
>> @@ -560,8 +561,11 @@ static hw_irq_controller gicv2_guest_irq_type = {
>>       .set_affinity = gicv2_irq_set_affinity,
>>   };
>>
>> +static int __init gicv2_init(void);
>
> I think you can reorder things to
> 	gic_init() { }
> 	ops
> 	gic_preinit()
> to avoid the forward declaration.

I wanted to move the less possible code. I will reorder it in the next 
series.

>> @@ -261,11 +265,15 @@ struct gic_info {
>>       uint8_t nr_lrs;
>>       /* Maintenance irq number */
>>       unsigned int maintenance_irq;
>> +    /* Pointer to the device tree node representing the interrupt controller */
>> +    const struct dt_device_node *node;
>
> You added this to both struct gic_info and gicv2_info?

I guess you wanted to say gicv2 instead of gicv2_info?

The one in gicv2 is an error, I will drop it.

Regards,

-- 
Julien Grall