[PATCH for-4.5 0/8] Interrupt management reworking

[PATCH for-4.5 0/8] Interrupt management reworking

[Julien Grall]

Hello,

While I was working on the ARM SMMU driver for Xen, I made some changes
to improve interrupt handling.

The main modifications of this patch series:
    - Add multiple handler support for interrupts
    - Merge route and setup IRQ functions
    - Improve error checking on some functions

This patch series is a requirement to support ARM SMMU driver.

Sincelery yours,

*** BLURB HERE ***

Julien Grall (8):
  xen/arm: irq: move gic {,un}lock in gic_set_irq_properties
  xen/arm: setup_dt_irq: don't enable the IRQ if the creation has
    failed
  xen/arm: IRQ: Protect IRQ to be shared between domains and XEN
  xen/arm: irq: Don't need to have a specific function to route IRQ to
    Xen
  xen/arm: IRQ: rename release_irq in release_dt_irq
  xen/arm: IRQ: Add lock contrainst for gic_irq_{startup,shutdown}
  xen/irq: Handle multiple action per IRQ
  xen/serial: remove serial_dt_irq

 xen/arch/arm/domain_build.c        |    8 +-
 xen/arch/arm/gic.c                 |  206 +++++++++++++++++++++++-------------
 xen/arch/arm/irq.c                 |    6 +-
 xen/arch/arm/setup.c               |    3 -
 xen/arch/arm/smpboot.c             |    2 -
 xen/arch/arm/time.c                |   11 --
 xen/drivers/char/exynos4210-uart.c |    8 --
 xen/drivers/char/ns16550.c         |   11 --
 xen/drivers/char/omap-uart.c       |    8 --
 xen/drivers/char/pl011.c           |    8 --
 xen/drivers/char/serial.c          |    9 --
 xen/include/asm-arm/gic.h          |    7 --
 xen/include/asm-arm/irq.h          |    1 +
 xen/include/asm-arm/time.h         |    3 -
 xen/include/xen/irq.h              |    1 +
 xen/include/xen/serial.h           |    5 -
 16 files changed, 146 insertions(+), 151 deletions(-)

-- 
1.7.10.4

[PATCH for-4.5 1/8] xen/arm: irq: move gic {, un}lock in gic_set_irq_properties

[Julien Grall]

The function gic_set_irq_properties is only called in two places:
    - gic_route_irq: the gic.lock is only taken for the call to the
    former function.
    - gic_route_irq_to_guest: the gic.lock is taken for the duration of
    the function. But the lock is only useful when gic_set_irq_properties.

So we can safely move the lock in gic_set_irq_properties and restrict the
critical section for the gic.lock in gic_route_irq_to_guest.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/gic.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index e6257a7..1943f92 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -228,7 +228,11 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
 {
     volatile unsigned char *bytereg;
     uint32_t cfg, edgebit;
-    unsigned int mask = gic_cpu_mask(cpu_mask);
+    unsigned int mask;
+
+    spin_lock(&gic.lock);
+
+    mask = gic_cpu_mask(cpu_mask);
 
     /* Set edge / level */
     cfg = GICD[GICD_ICFGR + irq / 16];
@@ -247,6 +251,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
     bytereg = (unsigned char *) (GICD + GICD_IPRIORITYR);
     bytereg[irq] = priority;
 
+    spin_unlock(&gic.lock);
 }
 
 /* Program the GIC to route an interrupt */
@@ -269,9 +274,7 @@ static int gic_route_irq(unsigned int irq, bool_t level,
 
     desc->handler = &gic_host_irq_type;
 
-    spin_lock(&gic.lock);
     gic_set_irq_properties(irq, level, cpu_mask, priority);
-    spin_unlock(&gic.lock);
 
     spin_unlock_irqrestore(&desc->lock, flags);
     return 0;
@@ -769,7 +772,6 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
     action->free_on_release = 1;
 
     spin_lock_irqsave(&desc->lock, flags);
-    spin_lock(&gic.lock);
 
     desc->handler = &gic_guest_irq_type;
     desc->status |= IRQ_GUEST;
@@ -790,7 +792,6 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
     p->desc = desc;
 
 out:
-    spin_unlock(&gic.lock);
     spin_unlock_irqrestore(&desc->lock, flags);
     return retval;
 }
-- 
1.7.10.4

Re: [PATCH for-4.5 1/8] xen/arm: irq: move gic {, un}lock in gic_set_irq_properties

[Ian Campbell]

On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
> The function gic_set_irq_properties is only called in two places:
>     - gic_route_irq: the gic.lock is only taken for the call to the
>     former function.
>     - gic_route_irq_to_guest: the gic.lock is taken for the duration of
>     the function. But the lock is only useful when gic_set_irq_properties.
> 
> So we can safely move the lock in gic_set_irq_properties and restrict the
> critical section for the gic.lock in gic_route_irq_to_guest.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Although ISTR Stefano saying he had got rid of the lock altogether. I'll
let you to battle that one out ;-)

Re: [PATCH for-4.5 1/8] xen/arm: irq: move gic {, un}lock in gic_set_irq_properties

[Julien Grall]

On 02/19/2014 11:23 AM, Ian Campbell wrote:
> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
>> The function gic_set_irq_properties is only called in two places:
>>     - gic_route_irq: the gic.lock is only taken for the call to the
>>     former function.
>>     - gic_route_irq_to_guest: the gic.lock is taken for the duration of
>>     the function. But the lock is only useful when gic_set_irq_properties.
>>
>> So we can safely move the lock in gic_set_irq_properties and restrict the
>> critical section for the gic.lock in gic_route_irq_to_guest.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks for the ack.

> Although ISTR Stefano saying he had got rid of the lock altogether. I'll
> let you to battle that one out ;-)

I can't find any patch on the mailing which remove the lock in theses
functions. I will keep it for now.

-- 
Julien Grall

[PATCH for-4.5 2/8] xen/arm: setup_dt_irq: don't enable the IRQ if the creation has failed

[Julien Grall]

For now __setup_dt_irq can only fail if the action is already set. If in the
future, the function is updated we don't want to enable the IRQ.

Assuming the function can fail with action = NULL, when Xen will receive the
IRQ it will segfault because do_IRQ doesn't check if action is NULL.

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 1943f92..55e7622 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -608,8 +608,8 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
     rc = __setup_irq(desc, irq->irq, new);
     spin_unlock_irqrestore(&desc->lock, flags);
 
-    desc->handler->startup(desc);
-
+    if ( !rc )
+        desc->handler->startup(desc);
 
     return rc;
 }
-- 
1.7.10.4

Re: [PATCH for-4.5 2/8] xen/arm: setup_dt_irq: don't enable the IRQ if the creation has failed

[Ian Campbell]

On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
> For now __setup_dt_irq can only fail if the action is already set. If in the
> future, the function is updated we don't want to enable the IRQ.
> 
> Assuming the function can fail with action = NULL, when Xen will receive the
> IRQ it will segfault because do_IRQ doesn't check if action is NULL.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH for-4.5 2/8] xen/arm: setup_dt_irq: don't enable the IRQ if the creation has failed

[Ian Campbell]

On Wed, 2014-02-19 at 11:24 +0000, Ian Campbell wrote:
> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
> > For now __setup_dt_irq can only fail if the action is already set. If in the
> > future, the function is updated we don't want to enable the IRQ.
> > 
> > Assuming the function can fail with action = NULL, when Xen will receive the
> > IRQ it will segfault because do_IRQ doesn't check if action is NULL.
> > 
> > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

applied just this one for now. I'm picking up trivial looking stuff from
my queue I'll revisit the rest in my next pass.

[PATCH for-4.5 3/8] xen/arm: IRQ: Protect IRQ to be shared between domains and XEN

[Julien Grall]

The current dt_route_irq_to_guest implementation set IRQ_GUEST no matter if the
IRQ is correctly setup.

As IRQ can be shared between devices, if the devices are not assigned to the
same domain or Xen, this could result to IRQ route to the domain instead of
Xen ...

Also avoid to rely on wrong behaviour when Xen is routing an IRQ to DOM0.

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

---
    Hopefully, none of the supported platforms have UARTs (the only device
    currently used by Xen). It would be nice to have this patch for Xen 4.4 to
    avoid waste of time for developer.

    The downside of this patch is if someone wants to support a such platform
    (eg IRQ shared between device assigned to different domain/XEN), it will
    end up to a error message and a panic.
---
 xen/arch/arm/domain_build.c |    8 ++++++--
 xen/arch/arm/gic.c          |   40 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 47b781b..1fc359a 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -712,8 +712,12 @@ static int map_device(struct domain *d, const struct dt_device_node *dev)
         }
 
         DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq, irq.type);
-        /* Don't check return because the IRQ can be use by multiple device */
-        gic_route_irq_to_guest(d, &irq, dt_node_name(dev));
+        res = gic_route_irq_to_guest(d, &irq, dt_node_name(dev));
+        if ( res )
+        {
+            printk(XENLOG_ERR "Unable to route the IRQ %u to dom0\n", irq.irq);
+            return res;
+        }
     }
 
     /* Map the address ranges */
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 55e7622..d68bde3 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -605,6 +605,21 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
     desc = irq_to_desc(irq->irq);
 
     spin_lock_irqsave(&desc->lock, flags);
+
+    if ( desc->status & IRQ_GUEST )
+    {
+        struct domain *d;
+
+        ASSERT(desc->action != NULL);
+
+        d = desc->action->dev_id;
+
+        spin_unlock_irqrestore(&desc->lock, flags);
+        printk(XENLOG_ERR "ERROR: IRQ %u is already used by the domain %u\n",
+               irq->irq, d->domain_id);
+        return -EADDRINUSE;
+    }
+
     rc = __setup_irq(desc, irq->irq, new);
     spin_unlock_irqrestore(&desc->lock, flags);
 
@@ -759,7 +774,7 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
     struct irqaction *action;
     struct irq_desc *desc = irq_to_desc(irq->irq);
     unsigned long flags;
-    int retval;
+    int retval = 0;
     bool_t level;
     struct pending_irq *p;
 
@@ -773,6 +788,29 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
 
     spin_lock_irqsave(&desc->lock, flags);
 
+    /* If the IRQ is already used by someone
+     *  - If it's the same domain -> Xen doesn't need to update the IRQ desc
+     *  - Otherwise -> For now, don't allow the IRQ to be shared between
+     *  Xen and domains.
+     */
+    if ( desc->action != NULL )
+    {
+        if ( (desc->status & IRQ_GUEST) && d == desc->action->dev_id )
+            goto out;
+
+        if ( desc->status & IRQ_GUEST )
+        {
+            d = desc->action->dev_id;
+            printk(XENLOG_ERR "ERROR: IRQ %u is already used by the domain %u\n",
+                   irq->irq, d->domain_id);
+        }
+        else
+            printk(XENLOG_ERR "ERROR: IRQ %u is already used by Xen\n",
+                   irq->irq);
+        retval = -EADDRINUSE;
+        goto out;
+    }
+
     desc->handler = &gic_guest_irq_type;
     desc->status |= IRQ_GUEST;
 
-- 
1.7.10.4

Re: [PATCH for-4.5 3/8] xen/arm: IRQ: Protect IRQ to be shared between domains and XEN

[Ian Campbell]

On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
> The current dt_route_irq_to_guest implementation set IRQ_GUEST no matter if the
> IRQ is correctly setup.
> 
> As IRQ can be shared between devices, if the devices are not assigned to the
> same domain or Xen, this could result to IRQ route to the domain instead of
> Xen ...
> 
> Also avoid to rely on wrong behaviour when Xen is routing an IRQ to DOM0.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Hopefully, none of the supported platforms have UARTs (the only device

                                                     ^shared?

Other than wondering if EBUSY might be more natural than EADDRINUSE and
some grammar nits (below) I think this patch looks good.

>     currently used by Xen). It would be nice to have this patch for Xen 4.4 to
>     avoid waste of time for developer.
> 
>     The downside of this patch is if someone wants to support a such platform
>     (eg IRQ shared between device assigned to different domain/XEN), it will
>     end up to a error message and a panic.
> ---
>  xen/arch/arm/domain_build.c |    8 ++++++--
>  xen/arch/arm/gic.c          |   40 +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 47b781b..1fc359a 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -712,8 +712,12 @@ static int map_device(struct domain *d, const struct dt_device_node *dev)
>          }
>  
>          DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq, irq.type);
> -        /* Don't check return because the IRQ can be use by multiple device */
> -        gic_route_irq_to_guest(d, &irq, dt_node_name(dev));
> +        res = gic_route_irq_to_guest(d, &irq, dt_node_name(dev));
> +        if ( res )
> +        {
> +            printk(XENLOG_ERR "Unable to route the IRQ %u to dom0\n", irq.irq);

"Unable to route IRQ %u..." and I think you want to use d->domain_id
rather than hardcoding 0.

> +            return res;
> +        }
>      }
>  
>      /* Map the address ranges */
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 55e7622..d68bde3 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -605,6 +605,21 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>      desc = irq_to_desc(irq->irq);
>  
>      spin_lock_irqsave(&desc->lock, flags);
> +
> +    if ( desc->status & IRQ_GUEST )
> +    {
> +        struct domain *d;
> +
> +        ASSERT(desc->action != NULL);
> +
> +        d = desc->action->dev_id;
> +
> +        spin_unlock_irqrestore(&desc->lock, flags);
> +        printk(XENLOG_ERR "ERROR: IRQ %u is already used by the domain %u\n",

"is already in use by domain ..."



> +               irq->irq, d->domain_id);
> +        return -EADDRINUSE;
> +    }
> +
>      rc = __setup_irq(desc, irq->irq, new);
>      spin_unlock_irqrestore(&desc->lock, flags);
>  
> @@ -759,7 +774,7 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
>      struct irqaction *action;
>      struct irq_desc *desc = irq_to_desc(irq->irq);
>      unsigned long flags;
> -    int retval;
> +    int retval = 0;
>      bool_t level;
>      struct pending_irq *p;
>  
> @@ -773,6 +788,29 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
>  
>      spin_lock_irqsave(&desc->lock, flags);
>  
> +    /* If the IRQ is already used by someone
> +     *  - If it's the same domain -> Xen doesn't need to update the IRQ desc
> +     *  - Otherwise -> For now, don't allow the IRQ to be shared between
> +     *  Xen and domains.
> +     */
> +    if ( desc->action != NULL )
> +    {
> +        if ( (desc->status & IRQ_GUEST) && d == desc->action->dev_id )
> +            goto out;
> +
> +        if ( desc->status & IRQ_GUEST )
> +        {
> +            d = desc->action->dev_id;
> +            printk(XENLOG_ERR "ERROR: IRQ %u is already used by the domain %u\n",
> +                   irq->irq, d->domain_id);

s/the //

> +        }
> +        else
> +            printk(XENLOG_ERR "ERROR: IRQ %u is already used by Xen\n",
> +                   irq->irq);
> +        retval = -EADDRINUSE;
> +        goto out;
> +    }
> +
>      desc->handler = &gic_guest_irq_type;
>      desc->status |= IRQ_GUEST;
>  

Re: [PATCH for-4.5 3/8] xen/arm: IRQ: Protect IRQ to be shared between domains and XEN

[Julien Grall]

Hi Ian,

On 02/19/2014 11:35 AM, Ian Campbell wrote:
> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
>> The current dt_route_irq_to_guest implementation set IRQ_GUEST no matter if the
>> IRQ is correctly setup.
>>
>> As IRQ can be shared between devices, if the devices are not assigned to the
>> same domain or Xen, this could result to IRQ route to the domain instead of
>> Xen ...
>>
>> Also avoid to rely on wrong behaviour when Xen is routing an IRQ to DOM0.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>     Hopefully, none of the supported platforms have UARTs (the only device
> 
>                                                      ^shared?

Hmmm ... I don't remember what I was trying to say here :/.

Anyway, this part was for argue to push it for Xen 4.4. It doesn't make
sense anymore. I will remove it.

> 
> Other than wondering if EBUSY might be more natural than EADDRINUSE and
> some grammar nits (below) I think this patch looks good.

Right, I will use EBUSY for the next version.

> 
>>     currently used by Xen). It would be nice to have this patch for Xen 4.4 to
>>     avoid waste of time for developer.
>>
>>     The downside of this patch is if someone wants to support a such platform
>>     (eg IRQ shared between device assigned to different domain/XEN), it will
>>     end up to a error message and a panic.
>> ---
>>  xen/arch/arm/domain_build.c |    8 ++++++--
>>  xen/arch/arm/gic.c          |   40 +++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 45 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 47b781b..1fc359a 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -712,8 +712,12 @@ static int map_device(struct domain *d, const struct dt_device_node *dev)
>>          }
>>  
>>          DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq, irq.type);
>> -        /* Don't check return because the IRQ can be use by multiple device */
>> -        gic_route_irq_to_guest(d, &irq, dt_node_name(dev));
>> +        res = gic_route_irq_to_guest(d, &irq, dt_node_name(dev));
>> +        if ( res )
>> +        {
>> +            printk(XENLOG_ERR "Unable to route the IRQ %u to dom0\n", irq.irq);
> 
> "Unable to route IRQ %u..." and I think you want to use d->domain_id
> rather than hardcoding 0.

I will fix it. At the same time, the error message when Xen is unable to
map the range also use "dom0". I will send a separate patch for that.

Cheers,

-- 
Julien Grall

[PATCH for-4.5 4/8] xen/arm: irq: Don't need to have a specific function to route IRQ to Xen

[Julien Grall]

Actually, when the IRQ is handling by Xen, the setup is done in 2 steps:
    - Route the IRQ to the current CPU and set priorities
    - Set up the handler

For PPIs, these step are called on every cpus. For SPIs, it's called only
on the boot CPU.

Divided the setup in two step complicated the code when a new driver is
added by Xen (for instance a SMMU driver). Xen can safely route the IRQ
when the driver setup the interrupt handler.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/gic.c         |   67 +++++++++++++++-----------------------------
 xen/arch/arm/setup.c       |    3 --
 xen/arch/arm/smpboot.c     |    2 --
 xen/arch/arm/time.c        |   11 --------
 xen/include/asm-arm/gic.h  |    7 -----
 xen/include/asm-arm/time.h |    3 --
 6 files changed, 23 insertions(+), 70 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index d68bde3..58bcba3 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -254,43 +254,25 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
     spin_unlock(&gic.lock);
 }
 
-/* Program the GIC to route an interrupt */
+/* Program the GIC to route an interrupt to the host (eg Xen)
+ * - needs to be called with desc.lock held
+ */
 static int gic_route_irq(unsigned int irq, bool_t level,
                          const cpumask_t *cpu_mask, unsigned int priority)
 {
     struct irq_desc *desc = irq_to_desc(irq);
-    unsigned long flags;
 
     ASSERT(priority <= 0xff);     /* Only 8 bits of priority */
     ASSERT(irq < gic.lines);      /* Can't route interrupts that don't exist */
-
-    if ( desc->action != NULL )
-        return -EBUSY;
-
-    /* Disable interrupt */
-    desc->handler->shutdown(desc);
-
-    spin_lock_irqsave(&desc->lock, flags);
+    ASSERT(desc->status & IRQ_DISABLED);
 
     desc->handler = &gic_host_irq_type;
 
     gic_set_irq_properties(irq, level, cpu_mask, priority);
 
-    spin_unlock_irqrestore(&desc->lock, flags);
     return 0;
 }
 
-/* Program the GIC to route an interrupt with a dt_irq */
-void gic_route_dt_irq(const struct dt_irq *irq, const cpumask_t *cpu_mask,
-                      unsigned int priority)
-{
-    bool_t level;
-
-    level = dt_irq_is_level_triggered(irq);
-
-    gic_route_irq(irq->irq, level, cpu_mask, priority);
-}
-
 static void __init gic_dist_init(void)
 {
     uint32_t type;
@@ -538,28 +520,6 @@ void gic_disable_cpu(void)
     spin_unlock(&gic.lock);
 }
 
-void gic_route_ppis(void)
-{
-    /* GIC maintenance */
-    gic_route_dt_irq(&gic.maintenance, cpumask_of(smp_processor_id()), 0xa0);
-    /* Route timer interrupt */
-    route_timer_interrupt();
-}
-
-void gic_route_spis(void)
-{
-    int seridx;
-    const struct dt_irq *irq;
-
-    for ( seridx = 0; seridx <= SERHND_IDX; seridx++ )
-    {
-        if ( (irq = serial_dt_irq(seridx)) == NULL )
-            continue;
-
-        gic_route_dt_irq(irq, cpumask_of(smp_processor_id()), 0xa0);
-    }
-}
-
 void __init release_irq(unsigned int irq)
 {
     struct irq_desc *desc;
@@ -601,6 +561,7 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
     int rc;
     unsigned long flags;
     struct irq_desc *desc;
+    bool_t disabled = 0;
 
     desc = irq_to_desc(irq->irq);
 
@@ -620,6 +581,24 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
         return -EADDRINUSE;
     }
 
+    disabled = (desc->action == NULL);
+
+    /* First time the IRQ is setup */
+    if ( disabled )
+    {
+        bool_t level;
+
+        level = dt_irq_is_level_triggered(irq);
+        /* It's fine to use smp_processor_id() because:
+         * For PPI: irq_desc is banked
+         * For SGI: we don't care for now which CPU will receive the
+         * interrupt
+         * TODO: Handle case where SGI is setup on different CPU than
+         * the targeted CPU and the priority.
+         */
+        gic_route_irq(irq->irq, level, cpumask_of(smp_processor_id()), 0xa0);
+    }
+
     rc = __setup_irq(desc, irq->irq, new);
     spin_unlock_irqrestore(&desc->lock, flags);
 
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 5434784..1f6d713 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -711,9 +711,6 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     init_IRQ();
 
-    gic_route_ppis();
-    gic_route_spis();
-
     init_maintenance_interrupt();
     init_timer_interrupt();
 
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index c53c765..807e7d3 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -287,8 +287,6 @@ void __cpuinit start_secondary(unsigned long boot_phys_offset,
 
     init_secondary_IRQ();
 
-    gic_route_ppis();
-
     init_maintenance_interrupt();
     init_timer_interrupt();
 
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 81e3e28..cd16318 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -218,17 +218,6 @@ static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
     vgic_vcpu_inject_irq(current, current->arch.virt_timer.irq, 1);
 }
 
-/* Route timer's IRQ on this CPU */
-void __cpuinit route_timer_interrupt(void)
-{
-    gic_route_dt_irq(&timer_irq[TIMER_PHYS_NONSECURE_PPI],
-                     cpumask_of(smp_processor_id()), 0xa0);
-    gic_route_dt_irq(&timer_irq[TIMER_HYP_PPI],
-                     cpumask_of(smp_processor_id()), 0xa0);
-    gic_route_dt_irq(&timer_irq[TIMER_VIRT_PPI],
-                     cpumask_of(smp_processor_id()), 0xa0);
-}
-
 /* Set up the timer interrupt on this CPU */
 void __cpuinit init_timer_interrupt(void)
 {
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 87f4298..3fd1024 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -144,13 +144,6 @@ extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq,int virtual);
 extern void vgic_clear_pending_irqs(struct vcpu *v);
 extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
 
-/* Program the GIC to route an interrupt with a dt_irq */
-extern void gic_route_dt_irq(const struct dt_irq *irq,
-                             const cpumask_t *cpu_mask,
-                             unsigned int priority);
-extern void gic_route_ppis(void);
-extern void gic_route_spis(void);
-
 extern void gic_inject(void);
 extern void gic_clear_pending_irqs(struct vcpu *v);
 extern int gic_events_need_delivery(void);
diff --git a/xen/include/asm-arm/time.h b/xen/include/asm-arm/time.h
index 9d302d3..eaa96bc 100644
--- a/xen/include/asm-arm/time.h
+++ b/xen/include/asm-arm/time.h
@@ -28,9 +28,6 @@ enum timer_ppi
 /* Get one of the timer IRQ description */
 const struct dt_irq* timer_dt_irq(enum timer_ppi ppi);
 
-/* Route timer's IRQ on this CPU */
-extern void __cpuinit route_timer_interrupt(void);
-
 /* Set up the timer interrupt on this CPU */
 extern void __cpuinit init_timer_interrupt(void);
 
-- 
1.7.10.4

Re: [PATCH for-4.5 4/8] xen/arm: irq: Don't need to have a specific function to route IRQ to Xen

[Ian Campbell]

On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
> Actually, when the IRQ is handling by Xen, the setup is done in 2 steps:

s/Actually, //

I'd also go with a title like "remove need to have specific..." or
"remove function to route...".

>     - Route the IRQ to the current CPU and set priorities
>     - Set up the handler
> 
> For PPIs, these step are called on every cpus. For SPIs, it's called only

                     ^s                    cpu             they are only called

> on the boot CPU.
> 
> Divided the setup in two step complicated the code when a new driver is

Dividing           into two steps complicates

> added by Xen (for instance a SMMU driver). Xen can safely route the IRQ

       to Xen

> when the driver setup the interrupt handler.

                 sets up

Although for this final para I'm not sure why a new driver is needed --
either it is already complicated or not.

> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/gic.c         |   67 +++++++++++++++-----------------------------
>  xen/arch/arm/setup.c       |    3 --
>  xen/arch/arm/smpboot.c     |    2 --
>  xen/arch/arm/time.c        |   11 --------
>  xen/include/asm-arm/gic.h  |    7 -----
>  xen/include/asm-arm/time.h |    3 --
>  6 files changed, 23 insertions(+), 70 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index d68bde3..58bcba3 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -254,43 +254,25 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
>      spin_unlock(&gic.lock);
>  }
>  
> -/* Program the GIC to route an interrupt */
> +/* Program the GIC to route an interrupt to the host (eg Xen)
> + * - needs to be called with desc.lock held

This suggests that the caller must have desc in its hand, but it then
passes irq here so we can look it up again. It may as well pass desc I
think.

>  void __init release_irq(unsigned int irq)
>  {
>      struct irq_desc *desc;
> @@ -601,6 +561,7 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>      int rc;
>      unsigned long flags;
>      struct irq_desc *desc;
> +    bool_t disabled = 0;
>  
>      desc = irq_to_desc(irq->irq);
>  
> @@ -620,6 +581,24 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>          return -EADDRINUSE;
>      }
>  
> +    disabled = (desc->action == NULL);
> +
> +    /* First time the IRQ is setup */
> +    if ( disabled )
> +    {
> +        bool_t level;
> +
> +        level = dt_irq_is_level_triggered(irq);
> +        /* It's fine to use smp_processor_id() because:
> +         * For PPI: irq_desc is banked
> +         * For SGI: we don't care for now which CPU will receive the
> +         * interrupt
> +         * TODO: Handle case where SGI is setup on different CPU than
> +         * the targeted CPU and the priority.

Do you mean s/SGI/SPI/ here? SGIs are inherently per CPU, like PPIs.

> +         */
> +        gic_route_irq(irq->irq, level, cpumask_of(smp_processor_id()), 0xa0);
> +    }
> +
>      rc = __setup_irq(desc, irq->irq, new);
>      spin_unlock_irqrestore(&desc->lock, flags);
>  

Re: [PATCH for-4.5 4/8] xen/arm: irq: Don't need to have a specific function to route IRQ to Xen

[Julien Grall]

Hello Ian,

On 02/19/2014 11:45 AM, Ian Campbell wrote:
> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
>> Actually, when the IRQ is handling by Xen, the setup is done in 2 steps:
> 
> s/Actually, //
> 
> I'd also go with a title like "remove need to have specific..." or
> "remove function to route...".
> 
>>     - Route the IRQ to the current CPU and set priorities
>>     - Set up the handler
>>
>> For PPIs, these step are called on every cpus. For SPIs, it's called only
> 
>                      ^s                    cpu             they are only called
> 
>> on the boot CPU.
>>
>> Divided the setup in two step complicated the code when a new driver is
> 
> Dividing           into two steps complicates
> 
>> added by Xen (for instance a SMMU driver). Xen can safely route the IRQ
> 
>        to Xen
> 
>> when the driver setup the interrupt handler.
> 
>                  sets up

Thanks to look at my grammar nits :).

> Although for this final para I'm not sure why a new driver is needed --
> either it is already complicated or not.

I will remove this para then.

>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/arch/arm/gic.c         |   67 +++++++++++++++-----------------------------
>>  xen/arch/arm/setup.c       |    3 --
>>  xen/arch/arm/smpboot.c     |    2 --
>>  xen/arch/arm/time.c        |   11 --------
>>  xen/include/asm-arm/gic.h  |    7 -----
>>  xen/include/asm-arm/time.h |    3 --
>>  6 files changed, 23 insertions(+), 70 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index d68bde3..58bcba3 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -254,43 +254,25 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
>>      spin_unlock(&gic.lock);
>>  }
>>  
>> -/* Program the GIC to route an interrupt */
>> +/* Program the GIC to route an interrupt to the host (eg Xen)
>> + * - needs to be called with desc.lock held
> 
> This suggests that the caller must have desc in its hand, but it then
> passes irq here so we can look it up again. It may as well pass desc I
> think.

Right, I will update release_irq to take an irq_desc in parameters
instead of the IRQ.

> 
>>  void __init release_irq(unsigned int irq)
>>  {
>>      struct irq_desc *desc;
>> @@ -601,6 +561,7 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>>      int rc;
>>      unsigned long flags;
>>      struct irq_desc *desc;
>> +    bool_t disabled = 0;
>>  
>>      desc = irq_to_desc(irq->irq);
>>  
>> @@ -620,6 +581,24 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>>          return -EADDRINUSE;
>>      }
>>  
>> +    disabled = (desc->action == NULL);
>> +
>> +    /* First time the IRQ is setup */
>> +    if ( disabled )
>> +    {
>> +        bool_t level;
>> +
>> +        level = dt_irq_is_level_triggered(irq);
>> +        /* It's fine to use smp_processor_id() because:
>> +         * For PPI: irq_desc is banked
>> +         * For SGI: we don't care for now which CPU will receive the
>> +         * interrupt
>> +         * TODO: Handle case where SGI is setup on different CPU than
>> +         * the targeted CPU and the priority.
> 
> Do you mean s/SGI/SPI/ here? SGIs are inherently per CPU, like PPIs.

Yes, SPI.

Cheers,

-- 
Julien Grall

[PATCH for-4.5 5/8] xen/arm: IRQ: rename release_irq in release_dt_irq

[Julien Grall]

Rename the function and make the prototype consistent with request_dt_irq.

The new parameter (dev_id) will be used in a later patch to release the right
action when the support for multiple action will be added.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/gic.c        |    4 ++--
 xen/include/asm-arm/irq.h |    1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 58bcba3..2643b46 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -520,13 +520,13 @@ void gic_disable_cpu(void)
     spin_unlock(&gic.lock);
 }
 
-void __init release_irq(unsigned int irq)
+void release_dt_irq(const struct dt_irq *irq, const void *dev_id)
 {
     struct irq_desc *desc;
     unsigned long flags;
    struct irqaction *action;
 
-    desc = irq_to_desc(irq);
+    desc = irq_to_desc(irq->irq);
 
     desc->handler->shutdown(desc);
 
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index 7c20703..bd8aac1 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -44,6 +44,7 @@ int __init request_dt_irq(const struct dt_irq *irq,
                           void (*handler)(int, void *, struct cpu_user_regs *),
                           const char *devname, void *dev_id);
 int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new);
+void release_dt_irq(const struct dt_irq *irq, const void *dev_id);
 
 #endif /* _ASM_HW_IRQ_H */
 /*
-- 
1.7.10.4

Re: [PATCH for-4.5 5/8] xen/arm: IRQ: rename release_irq in release_dt_irq

[Ian Campbell]

On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:

Subject: s/in/to/

> Rename the function and make the prototype consistent with request_dt_irq.
> 
> The new parameter (dev_id) will be used in a later patch to release the right
> action when the support for multiple action will be added.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  xen/arch/arm/gic.c        |    4 ++--
>  xen/include/asm-arm/irq.h |    1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 58bcba3..2643b46 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -520,13 +520,13 @@ void gic_disable_cpu(void)
>      spin_unlock(&gic.lock);
>  }
>  
> -void __init release_irq(unsigned int irq)
> +void release_dt_irq(const struct dt_irq *irq, const void *dev_id)
>  {
>      struct irq_desc *desc;
>      unsigned long flags;
>     struct irqaction *action;
>  
> -    desc = irq_to_desc(irq);
> +    desc = irq_to_desc(irq->irq);
>  
>      desc->handler->shutdown(desc);
>  
> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index 7c20703..bd8aac1 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -44,6 +44,7 @@ int __init request_dt_irq(const struct dt_irq *irq,
>                            void (*handler)(int, void *, struct cpu_user_regs *),
>                            const char *devname, void *dev_id);
>  int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new);

This patch implies that things can now be dynamically registered and
unregistered -- does this therefore need to become non-__init?

> +void release_dt_irq(const struct dt_irq *irq, const void *dev_id);
>  
>  #endif /* _ASM_HW_IRQ_H */
>  /*

Re: [PATCH for-4.5 5/8] xen/arm: IRQ: rename release_irq in release_dt_irq

[Julien Grall]

Hi Ian,

On 02/19/2014 11:47 AM, Ian Campbell wrote:
> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
> 
> Subject: s/in/to/
> 
>> Rename the function and make the prototype consistent with request_dt_irq.
>>
>> The new parameter (dev_id) will be used in a later patch to release the right
>> action when the support for multiple action will be added.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks.

>> ---
>>  xen/arch/arm/gic.c        |    4 ++--
>>  xen/include/asm-arm/irq.h |    1 +
>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 58bcba3..2643b46 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -520,13 +520,13 @@ void gic_disable_cpu(void)
>>      spin_unlock(&gic.lock);
>>  }
>>  
>> -void __init release_irq(unsigned int irq)
>> +void release_dt_irq(const struct dt_irq *irq, const void *dev_id)
>>  {
>>      struct irq_desc *desc;
>>      unsigned long flags;
>>     struct irqaction *action;
>>  
>> -    desc = irq_to_desc(irq);
>> +    desc = irq_to_desc(irq->irq);
>>  
>>      desc->handler->shutdown(desc);
>>  
>> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
>> index 7c20703..bd8aac1 100644
>> --- a/xen/include/asm-arm/irq.h
>> +++ b/xen/include/asm-arm/irq.h
>> @@ -44,6 +44,7 @@ int __init request_dt_irq(const struct dt_irq *irq,
>>                            void (*handler)(int, void *, struct cpu_user_regs *),
>>                            const char *devname, void *dev_id);
>>  int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new);
> 
> This patch implies that things can now be dynamically registered and
> unregistered -- does this therefore need to become non-__init?

Yes, I noticed that after I sent this patch series. I have a patch for
that which I will add on the next version.

Cheers,

-- 
Julien Grall

[PATCH for-4.5 6/8] xen/arm: IRQ: Add lock contrainst for gic_irq_{startup, shutdown}

[Julien Grall]

When multiple action will be supported, gic_irq_{startup,shutdown} will have
to be called in the same critical zone has setup/release.
Otherwise it could have a race condition if at the same time CPU A is calling
release_dt_irq and CPU B is calling setup_dt_irq.

This could end up to the IRQ is not enabled.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/gic.c |   40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 2643b46..ebd2b5f 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -129,43 +129,53 @@ void gic_restore_state(struct vcpu *v)
     gic_restore_pending_irqs(v);
 }
 
-static void gic_irq_enable(struct irq_desc *desc)
+static unsigned int gic_irq_startup(struct irq_desc *desc)
 {
     int irq = desc->irq;
-    unsigned long flags;
 
-    spin_lock_irqsave(&desc->lock, flags);
+    ASSERT(spin_is_locked(&desc->lock));
+    ASSERT(!local_irq_is_enabled());
+
     spin_lock(&gic.lock);
     desc->status &= ~IRQ_DISABLED;
     dsb();
     /* Enable routing */
     GICD[GICD_ISENABLER + irq / 32] = (1u << (irq % 32));
     spin_unlock(&gic.lock);
-    spin_unlock_irqrestore(&desc->lock, flags);
+
+    return 0;
 }
 
-static void gic_irq_disable(struct irq_desc *desc)
+static void gic_irq_shutdown(struct irq_desc *desc)
 {
     int irq = desc->irq;
 
-    spin_lock(&desc->lock);
+    ASSERT(spin_is_locked(&desc->lock));
+    ASSERT(!local_irq_is_enabled());
+
     spin_lock(&gic.lock);
     /* Disable routing */
     GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32));
     desc->status |= IRQ_DISABLED;
     spin_unlock(&gic.lock);
-    spin_unlock(&desc->lock);
 }
 
-static unsigned int gic_irq_startup(struct irq_desc *desc)
+static void gic_irq_enable(struct irq_desc *desc)
 {
-    gic_irq_enable(desc);
-    return 0;
+    unsigned long flags;
+
+    spin_lock_irqsave(&desc->lock, flags);
+    gic_irq_startup(desc);
+    spin_unlock_irqrestore(&desc->lock, flags);
 }
 
-static void gic_irq_shutdown(struct irq_desc *desc)
+static void gic_irq_disable(struct irq_desc *desc)
 {
-    gic_irq_disable(desc);
+    unsigned long flags;
+
+    spin_lock_irqsave(&desc->lock, flags);
+    gic_irq_shutdown(desc);
+    spin_unlock_irqrestore(&desc->lock, flags);
 }
 
 static void gic_irq_ack(struct irq_desc *desc)
@@ -528,9 +538,8 @@ void release_dt_irq(const struct dt_irq *irq, const void *dev_id)
 
     desc = irq_to_desc(irq->irq);
 
-    desc->handler->shutdown(desc);
-
     spin_lock_irqsave(&desc->lock,flags);
+    desc->handler->shutdown(desc);
     action = desc->action;
     desc->action  = NULL;
     desc->status &= ~IRQ_GUEST;
@@ -600,11 +609,12 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
     }
 
     rc = __setup_irq(desc, irq->irq, new);
-    spin_unlock_irqrestore(&desc->lock, flags);
 
     if ( !rc )
         desc->handler->startup(desc);
 
+    spin_unlock_irqrestore(&desc->lock, flags);
+
     return rc;
 }
 
-- 
1.7.10.4

Re: [PATCH for-4.5 6/8] xen/arm: IRQ: Add lock contrainst for gic_irq_{startup, shutdown}

[Ian Campbell]

On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
> When multiple action will be supported, gic_irq_{startup,shutdown} will have
> to be called in the same critical zone has setup/release.

s/has/as/?

> Otherwise it could have a race condition if at the same time CPU A is calling
> release_dt_irq and CPU B is calling setup_dt_irq.
> 
> This could end up to the IRQ is not enabled.

"This could end up with the IRQ not being enabled."

> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/gic.c |   40 +++++++++++++++++++++++++---------------
>  1 file changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 2643b46..ebd2b5f 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -129,43 +129,53 @@ void gic_restore_state(struct vcpu *v)
>      gic_restore_pending_irqs(v);
>  }
>  
> -static void gic_irq_enable(struct irq_desc *desc)
> +static unsigned int gic_irq_startup(struct irq_desc *desc)

unsigned? What are the error codes here going to be?

>  {
>      int irq = desc->irq;
> -    unsigned long flags;
>  
> -    spin_lock_irqsave(&desc->lock, flags);
> +    ASSERT(spin_is_locked(&desc->lock));
> +    ASSERT(!local_irq_is_enabled());
> +
>      spin_lock(&gic.lock);
>      desc->status &= ~IRQ_DISABLED;
>      dsb();
>      /* Enable routing */
>      GICD[GICD_ISENABLER + irq / 32] = (1u << (irq % 32));
>      spin_unlock(&gic.lock);
> -    spin_unlock_irqrestore(&desc->lock, flags);
> +
> +    return 0;
>  }
>  
> -static void gic_irq_disable(struct irq_desc *desc)
> +static void gic_irq_shutdown(struct irq_desc *desc)
>  {
>      int irq = desc->irq;
>  
> -    spin_lock(&desc->lock);
> +    ASSERT(spin_is_locked(&desc->lock));
> +    ASSERT(!local_irq_is_enabled());
> +
>      spin_lock(&gic.lock);
>      /* Disable routing */
>      GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32));
>      desc->status |= IRQ_DISABLED;
>      spin_unlock(&gic.lock);
> -    spin_unlock(&desc->lock);
>  }
>  
> -static unsigned int gic_irq_startup(struct irq_desc *desc)
> +static void gic_irq_enable(struct irq_desc *desc)
>  {
> -    gic_irq_enable(desc);
> -    return 0;
> +    unsigned long flags;
> +
> +    spin_lock_irqsave(&desc->lock, flags);
> +    gic_irq_startup(desc);
> +    spin_unlock_irqrestore(&desc->lock, flags);
>  }
>  
> -static void gic_irq_shutdown(struct irq_desc *desc)
> +static void gic_irq_disable(struct irq_desc *desc)
>  {
> -    gic_irq_disable(desc);
> +    unsigned long flags;
> +
> +    spin_lock_irqsave(&desc->lock, flags);
> +    gic_irq_shutdown(desc);
> +    spin_unlock_irqrestore(&desc->lock, flags);
>  }
>  
>  static void gic_irq_ack(struct irq_desc *desc)
> @@ -528,9 +538,8 @@ void release_dt_irq(const struct dt_irq *irq, const void *dev_id)
>  
>      desc = irq_to_desc(irq->irq);
>  
> -    desc->handler->shutdown(desc);
> -
>      spin_lock_irqsave(&desc->lock,flags);
> +    desc->handler->shutdown(desc);
>      action = desc->action;
>      desc->action  = NULL;
>      desc->status &= ~IRQ_GUEST;
> @@ -600,11 +609,12 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>      }
>  
>      rc = __setup_irq(desc, irq->irq, new);
> -    spin_unlock_irqrestore(&desc->lock, flags);
>  
>      if ( !rc )
>          desc->handler->startup(desc);
>  
> +    spin_unlock_irqrestore(&desc->lock, flags);
> +
>      return rc;
>  }
>  

Re: [PATCH for-4.5 6/8] xen/arm: IRQ: Add lock contrainst for gic_irq_{startup, shutdown}

[Julien Grall]

Hi Ian,

On 02/19/2014 11:51 AM, Ian Campbell wrote:
> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
>> When multiple action will be supported, gic_irq_{startup,shutdown} will have
>> to be called in the same critical zone has setup/release.
> 
> s/has/as/?

Yes.

>> Otherwise it could have a race condition if at the same time CPU A is calling
>> release_dt_irq and CPU B is calling setup_dt_irq.
>>
>> This could end up to the IRQ is not enabled.
> 
> "This could end up with the IRQ not being enabled."
> 
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/arch/arm/gic.c |   40 +++++++++++++++++++++++++---------------
>>  1 file changed, 25 insertions(+), 15 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 2643b46..ebd2b5f 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -129,43 +129,53 @@ void gic_restore_state(struct vcpu *v)
>>      gic_restore_pending_irqs(v);
>>  }
>>  
>> -static void gic_irq_enable(struct irq_desc *desc)
>> +static unsigned int gic_irq_startup(struct irq_desc *desc)
> 
> unsigned? What are the error codes here going to be?

This is the return type requested by hw_interrupt_type.startup.

It seems that the return is never checked (even in x86 code). Maybe we
should change the prototype of hw_interrupt_type.startup.

Cheers,

-- 
Julien Grall

Re: [PATCH for-4.5 6/8] xen/arm: IRQ: Add lock contrainst for gic_irq_{startup, shutdown}

[Ian Campbell]

On Wed, 2014-02-19 at 14:35 +0000, Julien Grall wrote:

> >> -static void gic_irq_enable(struct irq_desc *desc)
> >> +static unsigned int gic_irq_startup(struct irq_desc *desc)
> > 
> > unsigned? What are the error codes here going to be?
> 
> This is the return type requested by hw_interrupt_type.startup.
> 
> It seems that the return is never checked (even in x86 code). Maybe we
> should change the prototype of hw_interrupt_type.startup.

Worth investigating. I wonder if someone thought this might return the
resulting interrupt number (those are normally unsigned int I think) or
if it actually did used to etc.

Ian.

Re: [PATCH for-4.5 6/8] xen/arm: IRQ: Add lock contrainst for gic_irq_{startup, shutdown}

[Julien Grall]

Adding Keir and Jan.

On 02/19/2014 02:38 PM, Ian Campbell wrote:
> On Wed, 2014-02-19 at 14:35 +0000, Julien Grall wrote:
> 
>>>> -static void gic_irq_enable(struct irq_desc *desc)
>>>> +static unsigned int gic_irq_startup(struct irq_desc *desc)
>>>
>>> unsigned? What are the error codes here going to be?
>>
>> This is the return type requested by hw_interrupt_type.startup.
>>
>> It seems that the return is never checked (even in x86 code). Maybe we
>> should change the prototype of hw_interrupt_type.startup.
> 
> Worth investigating. I wonder if someone thought this might return the
> resulting interrupt number (those are normally unsigned int I think) or
> if it actually did used to etc.

I think it was copied from Linux which also have unsigned int. I gave a
quick look to the code and this callback is only used in 2 places which
always return 0.

Surprisingly, the wrapper irq_startup (kernel/irq/manage.c) is returning
an int...

I can create a patch to return void instead of unsigned if everyone is
happy with this solution.

-- 
Julien Grall

Re: [PATCH for-4.5 6/8] xen/arm: IRQ: Add lock contrainst for gic_irq_{startup, shutdown}

[Jan Beulich]

>>> On 19.02.14 at 15:51, Julien Grall <julien.grall@linaro.org> wrote:
> Adding Keir and Jan.
> 
> On 02/19/2014 02:38 PM, Ian Campbell wrote:
>> On Wed, 2014-02-19 at 14:35 +0000, Julien Grall wrote:
>> 
>>>>> -static void gic_irq_enable(struct irq_desc *desc)
>>>>> +static unsigned int gic_irq_startup(struct irq_desc *desc)
>>>>
>>>> unsigned? What are the error codes here going to be?
>>>
>>> This is the return type requested by hw_interrupt_type.startup.
>>>
>>> It seems that the return is never checked (even in x86 code). Maybe we
>>> should change the prototype of hw_interrupt_type.startup.
>> 
>> Worth investigating. I wonder if someone thought this might return the
>> resulting interrupt number (those are normally unsigned int I think) or
>> if it actually did used to etc.
> 
> I think it was copied from Linux which also have unsigned int. I gave a
> quick look to the code and this callback is only used in 2 places which
> always return 0.
> 
> Surprisingly, the wrapper irq_startup (kernel/irq/manage.c) is returning
> an int...
> 
> I can create a patch to return void instead of unsigned if everyone is
> happy with this solution.

I'd be fine with such a change; I'd like to ask though that if you
do this, you at the same time do the resulting possible cleanup:
As an example, xen/arch/x86/msi.c:startup_msi_irq() becomes
unnecessary then. It will in fact be interesting to see how many
distinct startup routines actually remain.

Jan

Re: [PATCH for-4.5 6/8] xen/arm: IRQ: Add lock contrainst for gic_irq_{startup, shutdown}

[Julien Grall]

On 02/19/2014 03:07 PM, Jan Beulich wrote:
>>>> On 19.02.14 at 15:51, Julien Grall <julien.grall@linaro.org> wrote:
>> Adding Keir and Jan.
>>
>> On 02/19/2014 02:38 PM, Ian Campbell wrote:
>>> On Wed, 2014-02-19 at 14:35 +0000, Julien Grall wrote:
>>>
>>>>>> -static void gic_irq_enable(struct irq_desc *desc)
>>>>>> +static unsigned int gic_irq_startup(struct irq_desc *desc)
>>>>>
>>>>> unsigned? What are the error codes here going to be?
>>>>
>>>> This is the return type requested by hw_interrupt_type.startup.
>>>>
>>>> It seems that the return is never checked (even in x86 code). Maybe we
>>>> should change the prototype of hw_interrupt_type.startup.
>>>
>>> Worth investigating. I wonder if someone thought this might return the
>>> resulting interrupt number (those are normally unsigned int I think) or
>>> if it actually did used to etc.
>>
>> I think it was copied from Linux which also have unsigned int. I gave a
>> quick look to the code and this callback is only used in 2 places which
>> always return 0.
>>
>> Surprisingly, the wrapper irq_startup (kernel/irq/manage.c) is returning
>> an int...
>>
>> I can create a patch to return void instead of unsigned if everyone is
>> happy with this solution.
> 
> I'd be fine with such a change; I'd like to ask though that if you
> do this, you at the same time do the resulting possible cleanup:
> As an example, xen/arch/x86/msi.c:startup_msi_irq() becomes
> unnecessary then. It will in fact be interesting to see how many
> distinct startup routines actually remain.

Sure, I will give a look at it.

-- 
Julien Grall

Re: [PATCH for-4.5 6/8] xen/arm: IRQ: Add lock contrainst for gic_irq_{startup, shutdown}

[Julien Grall]

Hi Jan,

On 02/19/2014 03:07 PM, Jan Beulich wrote:
>>>> On 19.02.14 at 15:51, Julien Grall <julien.grall@linaro.org> wrote:
>> Adding Keir and Jan.
>>
>> On 02/19/2014 02:38 PM, Ian Campbell wrote:
>>> On Wed, 2014-02-19 at 14:35 +0000, Julien Grall wrote:
>>>
>>>>>> -static void gic_irq_enable(struct irq_desc *desc)
>>>>>> +static unsigned int gic_irq_startup(struct irq_desc *desc)
>>>>>
>>>>> unsigned? What are the error codes here going to be?
>>>>
>>>> This is the return type requested by hw_interrupt_type.startup.
>>>>
>>>> It seems that the return is never checked (even in x86 code). Maybe we
>>>> should change the prototype of hw_interrupt_type.startup.
>>>
>>> Worth investigating. I wonder if someone thought this might return the
>>> resulting interrupt number (those are normally unsigned int I think) or
>>> if it actually did used to etc.
>>
>> I think it was copied from Linux which also have unsigned int. I gave a
>> quick look to the code and this callback is only used in 2 places which
>> always return 0.
>>
>> Surprisingly, the wrapper irq_startup (kernel/irq/manage.c) is returning
>> an int...
>>
>> I can create a patch to return void instead of unsigned if everyone is
>> happy with this solution.
> 
> I'd be fine with such a change; I'd like to ask though that if you
> do this, you at the same time do the resulting possible cleanup:
> As an example, xen/arch/x86/msi.c:startup_msi_irq() becomes
> unnecessary then. It will in fact be interesting to see how many
> distinct startup routines actually remain.

Before the clean up there was 8 distinct startup routines for x86. No
there is only 2:
  - drivers/passthrough/amd/iommu_init.c: iommu_maskable_msi_startup
  - arch/x86/ioapic.c: startup_edge_ioapic_irq

For a latter one, I'm a bit surprised that the function can return 1,
but the result is never used.

Cheers,

-- 
Julien Grall

Re: [PATCH for-4.5 6/8] xen/arm: IRQ: Add lock contrainst for gic_irq_{startup, shutdown}

[Jan Beulich]

>>> On 20.02.14 at 21:48, Julien Grall <julien.grall@linaro.org> wrote:
> Before the clean up there was 8 distinct startup routines for x86. No
> there is only 2:
>   - drivers/passthrough/amd/iommu_init.c: iommu_maskable_msi_startup
>   - arch/x86/ioapic.c: startup_edge_ioapic_irq
> 
> For a latter one, I'm a bit surprised that the function can return 1,
> but the result is never used.

Which means consumption of the return value was intended, but
never implemented (or lost _very_ long ago). Looking at the Linux
code, the intention apparently would be for the non-zero return
value to propagate into IRQ_PENDING in one very special case we
didn't ever support (auto-probing). Re-sending of an already
pending interrupt is being handled differently there anyway. So if
needed something like the setting of IRQ_PENDING at some point,
I guess we could as well have the startup routine do this itself. I.e.
I think converting the return value to void is still fine, as long as
you leave some commentary in
arch/x86/ioapic.c:startup_edge_ioapic_irq().

Jan

Re: [PATCH for-4.5 6/8] xen/arm: IRQ: Add lock contrainst for gic_irq_{startup, shutdown}

[Julien Grall]

On 02/21/2014 08:55 AM, Jan Beulich wrote:
>>>> On 20.02.14 at 21:48, Julien Grall <julien.grall@linaro.org> wrote:
>> Before the clean up there was 8 distinct startup routines for x86. No
>> there is only 2:
>>   - drivers/passthrough/amd/iommu_init.c: iommu_maskable_msi_startup
>>   - arch/x86/ioapic.c: startup_edge_ioapic_irq
>>
>> For a latter one, I'm a bit surprised that the function can return 1,
>> but the result is never used.
> 
> Which means consumption of the return value was intended, but
> never implemented (or lost _very_ long ago). Looking at the Linux
> code, the intention apparently would be for the non-zero return
> value to propagate into IRQ_PENDING in one very special case we
> didn't ever support (auto-probing). Re-sending of an already
> pending interrupt is being handled differently there anyway. So if
> needed something like the setting of IRQ_PENDING at some point,
> I guess we could as well have the startup routine do this itself. I.e.
> I think converting the return value to void is still fine, as long as
> you leave some commentary in
> arch/x86/ioapic.c:startup_edge_ioapic_irq().

I will send the patch to change startup prototype separately later.

-- 
Julien Grall

[PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Julien Grall]

On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same
interrupt.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
CC: Keir Fraser <keir@xen.org>
---
 xen/arch/arm/gic.c    |   48 ++++++++++++++++++++++++++++++++++++++++--------
 xen/arch/arm/irq.c    |    6 +++++-
 xen/include/xen/irq.h |    1 +
 3 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index ebd2b5f..8ba1de3 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -534,32 +534,64 @@ void release_dt_irq(const struct dt_irq *irq, const void *dev_id)
 {
     struct irq_desc *desc;
     unsigned long flags;
-   struct irqaction *action;
+    struct irqaction *action, **action_ptr;
 
     desc = irq_to_desc(irq->irq);
 
     spin_lock_irqsave(&desc->lock,flags);
     desc->handler->shutdown(desc);
     action = desc->action;
-    desc->action  = NULL;
-    desc->status &= ~IRQ_GUEST;
+
+    action_ptr = &desc->action;
+    for ( ;; )
+    {
+        action = *action_ptr;
+
+        if ( !action )
+        {
+            printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n",
+                   irq->irq);
+            return;
+        }
+
+        if ( action->dev_id == dev_id )
+            break;
+
+        action_ptr = &action->next;
+    }
+
+    /* Found it - remove it from the action list */
+    *action_ptr = action->next;
+
+    /* If this was the list action, shut down the IRQ */
+    if ( !desc->action )
+    {
+        desc->handler->shutdown(desc);
+        desc->status &= ~IRQ_GUEST;
+    }
 
     spin_unlock_irqrestore(&desc->lock,flags);
 
     /* Wait to make sure it's not being used on another CPU */
     do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS );
 
-    if (action && action->free_on_release)
+    if ( action && action->free_on_release )
         xfree(action);
 }
 
 static int __setup_irq(struct irq_desc *desc, unsigned int irq,
                        struct irqaction *new)
 {
-    if ( desc->action != NULL )
-        return -EBUSY;
+    struct irqaction *action = desc->action;
+
+    ASSERT(new != NULL);
+
+    /* Check that dev_id is correctly filled if we have multiple action */
+    if ( action != NULL && ( action->dev_id == NULL || new->dev_id == NULL ) )
+        return -EINVAL;
 
-    desc->action  = new;
+    new->next = desc->action;
+    desc->action = new;
     dsb();
 
     return 0;
@@ -610,7 +642,7 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
 
     rc = __setup_irq(desc, irq->irq, new);
 
-    if ( !rc )
+    if ( !rc && disabled )
         desc->handler->startup(desc);
 
     spin_unlock_irqrestore(&desc->lock, flags);
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 3e326b0..edf0404 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -179,7 +179,11 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
     {
         desc->status &= ~IRQ_PENDING;
         spin_unlock_irq(&desc->lock);
-        action->handler(irq, action->dev_id, regs);
+        do
+        {
+            action->handler(irq, action->dev_id, regs);
+            action = action->next;
+        } while ( action );
         spin_lock_irq(&desc->lock);
     }
 
diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
index f2e6215..54314b8 100644
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -11,6 +11,7 @@
 
 struct irqaction {
     void (*handler)(int, void *, struct cpu_user_regs *);
+    struct irqaction *next;
     const char *name;
     void *dev_id;
     bool_t free_on_release;
-- 
1.7.10.4

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Ian Campbell]

On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same
> interrupt.

Mention here that you are therefore creating a linked list of actions
for each interrupt.

If you use xen/list.h for this then you get a load of helpers and
iterators which would save you open coding them.

Some discussion of the behaviour wrt acking the physical interrupt might
also be interesting, especially in the case where the shared IRQ is
routed to the guest and to the hypervisor or to multiple guests.

> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> CC: Keir Fraser <keir@xen.org>
> ---
>  xen/arch/arm/gic.c    |   48 ++++++++++++++++++++++++++++++++++++++++--------
>  xen/arch/arm/irq.c    |    6 +++++-
>  xen/include/xen/irq.h |    1 +
>  3 files changed, 46 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index ebd2b5f..8ba1de3 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -534,32 +534,64 @@ void release_dt_irq(const struct dt_irq *irq, const void *dev_id)
>  {
>      struct irq_desc *desc;
>      unsigned long flags;
> -   struct irqaction *action;
> +    struct irqaction *action, **action_ptr;
>  
>      desc = irq_to_desc(irq->irq);
>  
>      spin_lock_irqsave(&desc->lock,flags);
>      desc->handler->shutdown(desc);
>      action = desc->action;
> -    desc->action  = NULL;
> -    desc->status &= ~IRQ_GUEST;
> +
> +    action_ptr = &desc->action;
> +    for ( ;; )
> +    {
> +        action = *action_ptr;
> +
> +        if ( !action )
> +        {
> +            printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n",
> +                   irq->irq);
> +            return;
> +        }
> +
> +        if ( action->dev_id == dev_id )
> +            break;
> +
> +        action_ptr = &action->next;
> +    }
> +
> +    /* Found it - remove it from the action list */
> +    *action_ptr = action->next;
> +
> +    /* If this was the list action, shut down the IRQ */
> +    if ( !desc->action )
> +    {
> +        desc->handler->shutdown(desc);
> +        desc->status &= ~IRQ_GUEST;
> +    }
>  
>      spin_unlock_irqrestore(&desc->lock,flags);
>  
>      /* Wait to make sure it's not being used on another CPU */
>      do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS );
>  
> -    if (action && action->free_on_release)
> +    if ( action && action->free_on_release )
>          xfree(action);
>  }
>  
>  static int __setup_irq(struct irq_desc *desc, unsigned int irq,
>                         struct irqaction *new)
>  {
> -    if ( desc->action != NULL )
> -        return -EBUSY;
> +    struct irqaction *action = desc->action;
> +
> +    ASSERT(new != NULL);
> +
> +    /* Check that dev_id is correctly filled if we have multiple action */
> +    if ( action != NULL && ( action->dev_id == NULL || new->dev_id == NULL ) )
> +        return -EINVAL;
>  
> -    desc->action  = new;
> +    new->next = desc->action;
> +    desc->action = new;
>      dsb();
>  
>      return 0;
> @@ -610,7 +642,7 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>  
>      rc = __setup_irq(desc, irq->irq, new);
>  
> -    if ( !rc )
> +    if ( !rc && disabled )
>          desc->handler->startup(desc);
>  
>      spin_unlock_irqrestore(&desc->lock, flags);
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 3e326b0..edf0404 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -179,7 +179,11 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
>      {
>          desc->status &= ~IRQ_PENDING;
>          spin_unlock_irq(&desc->lock);
> -        action->handler(irq, action->dev_id, regs);
> +        do
> +        {
> +            action->handler(irq, action->dev_id, regs);
> +            action = action->next;
> +        } while ( action );
>          spin_lock_irq(&desc->lock);
>      }
>  
> diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
> index f2e6215..54314b8 100644
> --- a/xen/include/xen/irq.h
> +++ b/xen/include/xen/irq.h
> @@ -11,6 +11,7 @@
>  
>  struct irqaction {
>      void (*handler)(int, void *, struct cpu_user_regs *);
> +    struct irqaction *next;
>      const char *name;
>      void *dev_id;
>      bool_t free_on_release;

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Julien Grall]

Hi Ian,

On 02/19/2014 11:55 AM, Ian Campbell wrote:
> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same
>> interrupt.
> 
> Mention here that you are therefore creating a linked list of actions
> for each interrupt.
> 
> If you use xen/list.h for this then you get a load of helpers and
> iterators which would save you open coding them.

I will use it on the next version.

> Some discussion of the behaviour wrt acking the physical interrupt might
> also be interesting, especially in the case where the shared IRQ is
> routed to the guest and to the hypervisor or to multiple guests.

For now, it's forbidden by the patch #3 of this series. Sharing IRQ
between Xen and guest will need a bit of rework, mainly when the
Stefano's patch series to remove the maintenance interrupt will be applied.

Cheers,

-- 
Julien Grall

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Julien Grall]

Hi Ian,

On 02/19/2014 11:55 AM, Ian Campbell wrote:
> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same
>> interrupt.
> 
> Mention here that you are therefore creating a linked list of actions
> for each interrupt.
> 
> If you use xen/list.h for this then you get a load of helpers and
> iterators which would save you open coding them.

After thinking, using xen/list.h won't really remove open code, except
removing "action_ptr" in release_dt_irq.

Calling release_dt_irq to an IRQ with multiple action shouldn't be
called often. Therefore, having both prev and next is a waste of space.

Cheers,

-- 
Julien Grall

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Julien Grall]

(Adding Jan for x86 part).

On 02/20/2014 09:29 PM, Julien Grall wrote:
> Hi Ian,
> 
> On 02/19/2014 11:55 AM, Ian Campbell wrote:
>> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
>>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same
>>> interrupt.
>>
>> Mention here that you are therefore creating a linked list of actions
>> for each interrupt.
>>
>> If you use xen/list.h for this then you get a load of helpers and
>> iterators which would save you open coding them.
> 
> After thinking, using xen/list.h won't really remove open code, except
> removing "action_ptr" in release_dt_irq.
> 
> Calling release_dt_irq to an IRQ with multiple action shouldn't be
> called often. Therefore, having both prev and next is a waste of space.

Jan, as it's common code, do you have any thoughts?

-- 
Julien Grall

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Ian Campbell]

On Mon, 2014-02-24 at 14:08 +0000, Julien Grall wrote:
> (Adding Jan for x86 part).
> 
> On 02/20/2014 09:29 PM, Julien Grall wrote:
> > Hi Ian,
> > 
> > On 02/19/2014 11:55 AM, Ian Campbell wrote:
> >> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
> >>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same
> >>> interrupt.
> >>
> >> Mention here that you are therefore creating a linked list of actions
> >> for each interrupt.
> >>
> >> If you use xen/list.h for this then you get a load of helpers and
> >> iterators which would save you open coding them.
> > 
> > After thinking, using xen/list.h won't really remove open code, except
> > removing "action_ptr" in release_dt_irq.

You can list_for_each*() in do_IRQ too and in release_dt_irq you get to
use a well debugged list delete implementation instead of reimplementing
your own.

> > Calling release_dt_irq to an IRQ with multiple action shouldn't be
> > called often. Therefore, having both prev and next is a waste of space.

If this is a concern we could take in the Linux single linked list
macros alongside the existing doubly linked ines we took from them.

Ian.

> Jan, as it's common code, do you have any thoughts?
> 

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Jan Beulich]

>>> On 24.02.14 at 15:08, Julien Grall <julien.grall@citrix.com> wrote:
> (Adding Jan for x86 part).
> 
> On 02/20/2014 09:29 PM, Julien Grall wrote:
>> Hi Ian,
>> 
>> On 02/19/2014 11:55 AM, Ian Campbell wrote:
>>> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
>>>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same
>>>> interrupt.
>>>
>>> Mention here that you are therefore creating a linked list of actions
>>> for each interrupt.
>>>
>>> If you use xen/list.h for this then you get a load of helpers and
>>> iterators which would save you open coding them.
>> 
>> After thinking, using xen/list.h won't really remove open code, except
>> removing "action_ptr" in release_dt_irq.
>> 
>> Calling release_dt_irq to an IRQ with multiple action shouldn't be
>> called often. Therefore, having both prev and next is a waste of space.
> 
> Jan, as it's common code, do you have any thoughts?

In fact I'm not convinced this action chaining is correct in the first
place, as mentioned by Ian too (considering the potential sharing
between hypervisor and guest). Furthermore, if this is really just
about IOMMU handlers, why can't the SMMU code register a single
action and disambiguate by the dev_id argument passed to the
handler?

Jan

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Julien Grall]

On 02/24/2014 02:32 PM, Jan Beulich wrote:
>>>> On 24.02.14 at 15:08, Julien Grall <julien.grall@citrix.com> wrote:
>> (Adding Jan for x86 part).
>>
>> On 02/20/2014 09:29 PM, Julien Grall wrote:
>>> Hi Ian,
>>>
>>> On 02/19/2014 11:55 AM, Ian Campbell wrote:
>>>> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
>>>>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same
>>>>> interrupt.
>>>>
>>>> Mention here that you are therefore creating a linked list of actions
>>>> for each interrupt.
>>>>
>>>> If you use xen/list.h for this then you get a load of helpers and
>>>> iterators which would save you open coding them.
>>>
>>> After thinking, using xen/list.h won't really remove open code, except
>>> removing "action_ptr" in release_dt_irq.
>>>
>>> Calling release_dt_irq to an IRQ with multiple action shouldn't be
>>> called often. Therefore, having both prev and next is a waste of space.
>>
>> Jan, as it's common code, do you have any thoughts?
> 
> In fact I'm not convinced this action chaining is correct in the first
> place, as mentioned by Ian too (considering the potential sharing
> between hypervisor and guest). Furthermore, if this is really just
> about IOMMU handlers, why can't the SMMU code register a single
> action and disambiguate by the dev_id argument passed to the
> handler?

The patch #3 of this serie protects the IRQ to be shared with the domain.

I should have remove "eg ARM SMMU" in the description. ARM SMMU is not
the only the case, we don't know in advance if the IRQ will be shared
(except browsing the DT and checking if this IRQ was used by another
devices...). We may have the same thing with other devices.

The logic is painful to handle internally in ARM SMMU driver while we
can handle it generically. No need to duplicate the code when a new
driver will have the same problem.

-- 
Julien Grall

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Julien Grall]

Hello Jan,

On 02/24/2014 02:48 PM, Julien Grall wrote:
> On 02/24/2014 02:32 PM, Jan Beulich wrote:
>>>>> On 24.02.14 at 15:08, Julien Grall <julien.grall@citrix.com> wrote:
>>> (Adding Jan for x86 part).
>>>
>>> On 02/20/2014 09:29 PM, Julien Grall wrote:
>>>> Hi Ian,
>>>>
>>>> On 02/19/2014 11:55 AM, Ian Campbell wrote:
>>>>> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
>>>>>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same
>>>>>> interrupt.
>>>>>
>>>>> Mention here that you are therefore creating a linked list of actions
>>>>> for each interrupt.
>>>>>
>>>>> If you use xen/list.h for this then you get a load of helpers and
>>>>> iterators which would save you open coding them.
>>>>
>>>> After thinking, using xen/list.h won't really remove open code, except
>>>> removing "action_ptr" in release_dt_irq.
>>>>
>>>> Calling release_dt_irq to an IRQ with multiple action shouldn't be
>>>> called often. Therefore, having both prev and next is a waste of space.
>>>
>>> Jan, as it's common code, do you have any thoughts?
>>
>> In fact I'm not convinced this action chaining is correct in the first
>> place, as mentioned by Ian too (considering the potential sharing
>> between hypervisor and guest). Furthermore, if this is really just
>> about IOMMU handlers, why can't the SMMU code register a single
>> action and disambiguate by the dev_id argument passed to the
>> handler?
> 
> The patch #3 of this serie protects the IRQ to be shared with the domain.
> 
> I should have remove "eg ARM SMMU" in the description. ARM SMMU is not
> the only the case, we don't know in advance if the IRQ will be shared
> (except browsing the DT and checking if this IRQ was used by another
> devices...). We may have the same thing with other devices.
> 
> The logic is painful to handle internally in ARM SMMU driver while we
> can handle it generically. No need to duplicate the code when a new
> driver will have the same problem.
> 

I haven't heard any answer from you. Shall I take as a "go"?

Regards,

-- 
Julien Grall

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Jan Beulich]

>>> On 11.03.14 at 16:16, Julien Grall <julien.grall@linaro.org> wrote:
> Hello Jan,
> 
> On 02/24/2014 02:48 PM, Julien Grall wrote:
>> On 02/24/2014 02:32 PM, Jan Beulich wrote:
>>>>>> On 24.02.14 at 15:08, Julien Grall <julien.grall@citrix.com> wrote:
>>>> (Adding Jan for x86 part).
>>>>
>>>> On 02/20/2014 09:29 PM, Julien Grall wrote:
>>>>> Hi Ian,
>>>>>
>>>>> On 02/19/2014 11:55 AM, Ian Campbell wrote:
>>>>>> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
>>>>>>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same
>>>>>>> interrupt.
>>>>>>
>>>>>> Mention here that you are therefore creating a linked list of actions
>>>>>> for each interrupt.
>>>>>>
>>>>>> If you use xen/list.h for this then you get a load of helpers and
>>>>>> iterators which would save you open coding them.
>>>>>
>>>>> After thinking, using xen/list.h won't really remove open code, except
>>>>> removing "action_ptr" in release_dt_irq.
>>>>>
>>>>> Calling release_dt_irq to an IRQ with multiple action shouldn't be
>>>>> called often. Therefore, having both prev and next is a waste of space.
>>>>
>>>> Jan, as it's common code, do you have any thoughts?
>>>
>>> In fact I'm not convinced this action chaining is correct in the first
>>> place, as mentioned by Ian too (considering the potential sharing
>>> between hypervisor and guest). Furthermore, if this is really just
>>> about IOMMU handlers, why can't the SMMU code register a single
>>> action and disambiguate by the dev_id argument passed to the
>>> handler?
>> 
>> The patch #3 of this serie protects the IRQ to be shared with the domain.
>> 
>> I should have remove "eg ARM SMMU" in the description. ARM SMMU is not
>> the only the case, we don't know in advance if the IRQ will be shared
>> (except browsing the DT and checking if this IRQ was used by another
>> devices...). We may have the same thing with other devices.
>> 
>> The logic is painful to handle internally in ARM SMMU driver while we
>> can handle it generically. No need to duplicate the code when a new
>> driver will have the same problem.
> 
> I haven't heard any answer from you. Shall I take as a "go"?

I'm sorry, this got lost between other stuff. Honestly I'm still not
convinced generic multi-action IRQ support is indeed useful. But
I wouldn't veto it either if others are convinced of this approach.

An option possibly to explore might be to have a per-arch trigger
enabling this, and keep it off for x86.

Jan

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Stefano Stabellini]

On Tue, 11 Mar 2014, Jan Beulich wrote:
> >>> On 11.03.14 at 16:16, Julien Grall <julien.grall@linaro.org> wrote:
> > Hello Jan,
> > 
> > On 02/24/2014 02:48 PM, Julien Grall wrote:
> >> On 02/24/2014 02:32 PM, Jan Beulich wrote:
> >>>>>> On 24.02.14 at 15:08, Julien Grall <julien.grall@citrix.com> wrote:
> >>>> (Adding Jan for x86 part).
> >>>>
> >>>> On 02/20/2014 09:29 PM, Julien Grall wrote:
> >>>>> Hi Ian,
> >>>>>
> >>>>> On 02/19/2014 11:55 AM, Ian Campbell wrote:
> >>>>>> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
> >>>>>>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same
> >>>>>>> interrupt.
> >>>>>>
> >>>>>> Mention here that you are therefore creating a linked list of actions
> >>>>>> for each interrupt.
> >>>>>>
> >>>>>> If you use xen/list.h for this then you get a load of helpers and
> >>>>>> iterators which would save you open coding them.
> >>>>>
> >>>>> After thinking, using xen/list.h won't really remove open code, except
> >>>>> removing "action_ptr" in release_dt_irq.
> >>>>>
> >>>>> Calling release_dt_irq to an IRQ with multiple action shouldn't be
> >>>>> called often. Therefore, having both prev and next is a waste of space.
> >>>>
> >>>> Jan, as it's common code, do you have any thoughts?
> >>>
> >>> In fact I'm not convinced this action chaining is correct in the first
> >>> place, as mentioned by Ian too (considering the potential sharing
> >>> between hypervisor and guest). Furthermore, if this is really just
> >>> about IOMMU handlers, why can't the SMMU code register a single
> >>> action and disambiguate by the dev_id argument passed to the
> >>> handler?
> >> 
> >> The patch #3 of this serie protects the IRQ to be shared with the domain.
> >> 
> >> I should have remove "eg ARM SMMU" in the description. ARM SMMU is not
> >> the only the case, we don't know in advance if the IRQ will be shared
> >> (except browsing the DT and checking if this IRQ was used by another
> >> devices...). We may have the same thing with other devices.
>
> >> The logic is painful to handle internally in ARM SMMU driver while we
> >> can handle it generically. No need to duplicate the code when a new
> >> driver will have the same problem.
> > 
> > I haven't heard any answer from you. Shall I take as a "go"?
> 
> I'm sorry, this got lost between other stuff. Honestly I'm still not
> convinced generic multi-action IRQ support is indeed useful. 

I agree.
In general if an IRQ is shared among multiple devices, it is likely to
go to Dom0 and have a single action from Xen point of view.
An IRQ shared between Xen and a guest is a very bad idea.

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Julien Grall]

Hi Stefano,

On 17/03/14 19:06, Stefano Stabellini wrote:
> On Tue, 11 Mar 2014, Jan Beulich wrote:
>>>>> On 11.03.14 at 16:16, Julien Grall <julien.grall@linaro.org> wrote:
>>> Hello Jan,
>>>
>>> On 02/24/2014 02:48 PM, Julien Grall wrote:
>>>> On 02/24/2014 02:32 PM, Jan Beulich wrote:
>>>>>>>> On 24.02.14 at 15:08, Julien Grall <julien.grall@citrix.com> wrote:
>>>>>> (Adding Jan for x86 part).
>>>>>>
>>>>>> On 02/20/2014 09:29 PM, Julien Grall wrote:
>>>>>>> Hi Ian,
>>>>>>>
>>>>>>> On 02/19/2014 11:55 AM, Ian Campbell wrote:
>>>>>>>> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
>>>>>>>>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same
>>>>>>>>> interrupt.
>>>>>>>>
>>>>>>>> Mention here that you are therefore creating a linked list of actions
>>>>>>>> for each interrupt.
>>>>>>>>
>>>>>>>> If you use xen/list.h for this then you get a load of helpers and
>>>>>>>> iterators which would save you open coding them.
>>>>>>>
>>>>>>> After thinking, using xen/list.h won't really remove open code, except
>>>>>>> removing "action_ptr" in release_dt_irq.
>>>>>>>
>>>>>>> Calling release_dt_irq to an IRQ with multiple action shouldn't be
>>>>>>> called often. Therefore, having both prev and next is a waste of space.
>>>>>>
>>>>>> Jan, as it's common code, do you have any thoughts?
>>>>>
>>>>> In fact I'm not convinced this action chaining is correct in the first
>>>>> place, as mentioned by Ian too (considering the potential sharing
>>>>> between hypervisor and guest). Furthermore, if this is really just
>>>>> about IOMMU handlers, why can't the SMMU code register a single
>>>>> action and disambiguate by the dev_id argument passed to the
>>>>> handler?
>>>>
>>>> The patch #3 of this serie protects the IRQ to be shared with the domain.
>>>>
>>>> I should have remove "eg ARM SMMU" in the description. ARM SMMU is not
>>>> the only the case, we don't know in advance if the IRQ will be shared
>>>> (except browsing the DT and checking if this IRQ was used by another
>>>> devices...). We may have the same thing with other devices.
>>
>>>> The logic is painful to handle internally in ARM SMMU driver while we
>>>> can handle it generically. No need to duplicate the code when a new
>>>> driver will have the same problem.
>>>
>>> I haven't heard any answer from you. Shall I take as a "go"?
>>
>> I'm sorry, this got lost between other stuff. Honestly I'm still not
>> convinced generic multi-action IRQ support is indeed useful.
>
> I agree.
> In general if an IRQ is shared among multiple devices, it is likely to
> go to Dom0 and have a single action from Xen point of view.
> An IRQ shared between Xen and a guest is a very bad idea.

I guess you agree with Jan, rigth? If so, I think you misunderstood the 
goal of this patch. This patch does *NOT* add support for IRQ sharing 
between domains and Xen (patch #3 is preventing that).

Some devices are describing a same interrupt twice in the device tree 
and the action is not the same to accomplish.

For instance for the SMMU on midway, the device tree bindings is:

               smmu_sata: smmu@9,20180000 {
                       compatible = "arm,mmu-400";
                       reg = <0x9 0x20180000 0x10000>;
                       mmu-masters = <&sata 0 1 2 3 4 5 6 7 8 9>;
                       #global-interrupts = <1>;
                       interrupts = <0 114 4 0 114 4>;
                       calxeda,smmu-secure-config-access;
                       arm,smmu-isolate-devices;
              };

As you can see the same interrupts is used twice: the first one is used 
for the global interrupt and the second one as context interrupt. For 
the latter, each time a new context bank is created (e.g a device is 
passthrough to IOMMU), we need to register another handler.

Now we have 2 solutions to implement it:
	1) Implement it directly in the SMMU drivers => We are assuming we 
wouldn't this situation on another IOMMU drivers
         2) Implement it generically

The former one is complicated to implement, because it's not fixed if 
the IRQ will be described twice or not. We can take advantage of the 
generic code for this purpose.

Regards,

-- 
Julien Grall

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Ian Campbell]

On Mon, 2014-03-17 at 21:05 +0000, Julien Grall wrote:
> For instance for the SMMU on midway, the device tree bindings is:
> 
>                smmu_sata: smmu@9,20180000 {
>                        compatible = "arm,mmu-400";
>                        reg = <0x9 0x20180000 0x10000>;
>                        mmu-masters = <&sata 0 1 2 3 4 5 6 7 8 9>;
>                        #global-interrupts = <1>;
>                        interrupts = <0 114 4 0 114 4>;
>                        calxeda,smmu-secure-config-access;
>                        arm,smmu-isolate-devices;
>               };
> 
> As you can see the same interrupts is used twice:

Is that actually valid in device tree? Or is this a quirk of the midway
DT?

Ian.

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Julien Grall]

Hi Ian,

On 03/18/2014 09:33 AM, Ian Campbell wrote:
> On Mon, 2014-03-17 at 21:05 +0000, Julien Grall wrote:
>> For instance for the SMMU on midway, the device tree bindings is:
>>
>>                smmu_sata: smmu@9,20180000 {
>>                        compatible = "arm,mmu-400";
>>                        reg = <0x9 0x20180000 0x10000>;
>>                        mmu-masters = <&sata 0 1 2 3 4 5 6 7 8 9>;
>>                        #global-interrupts = <1>;
>>                        interrupts = <0 114 4 0 114 4>;
>>                        calxeda,smmu-secure-config-access;
>>                        arm,smmu-isolate-devices;
>>               };
>>
>> As you can see the same interrupts is used twice:
> 
> Is that actually valid in device tree? Or is this a quirk of the midway
> DT?

Yes it's valid. The interrupts property for the SMMU is described as:

"Interrupt list, with the first #global-irqs entries corresponding to
the global interrupts and any following entries corresponding to context
interrupts, specified in order of their indexing by the SMMU.

For SMMUv2 implementations, there must be exactly one interrupt per
context bank. In the case of a single, combined interrupt, it must be
listed multiple times."

On midway there is only one IRQ with is used for both context interrupt
and global interrupt. As it's the only platform on Linux with SMMU
support in the device tree, we don't know if every platform will have
the same behavior.

Regards,

-- 
Julien Grall

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Stefano Stabellini]

On Tue, 18 Mar 2014, Julien Grall wrote:
> Hi Ian,
> 
> On 03/18/2014 09:33 AM, Ian Campbell wrote:
> > On Mon, 2014-03-17 at 21:05 +0000, Julien Grall wrote:
> >> For instance for the SMMU on midway, the device tree bindings is:
> >>
> >>                smmu_sata: smmu@9,20180000 {
> >>                        compatible = "arm,mmu-400";
> >>                        reg = <0x9 0x20180000 0x10000>;
> >>                        mmu-masters = <&sata 0 1 2 3 4 5 6 7 8 9>;
> >>                        #global-interrupts = <1>;
> >>                        interrupts = <0 114 4 0 114 4>;
> >>                        calxeda,smmu-secure-config-access;
> >>                        arm,smmu-isolate-devices;
> >>               };
> >>
> >> As you can see the same interrupts is used twice:
> > 
> > Is that actually valid in device tree? Or is this a quirk of the midway
> > DT?
> 
> Yes it's valid. The interrupts property for the SMMU is described as:
> 
> "Interrupt list, with the first #global-irqs entries corresponding to
> the global interrupts and any following entries corresponding to context
> interrupts, specified in order of their indexing by the SMMU.
> 
> For SMMUv2 implementations, there must be exactly one interrupt per
> context bank. In the case of a single, combined interrupt, it must be
> listed multiple times."
> 
> On midway there is only one IRQ with is used for both context interrupt
> and global interrupt. As it's the only platform on Linux with SMMU
> support in the device tree, we don't know if every platform will have
> the same behavior.

I understand that the SMMU might reuse the same IRQ for multiple
purposes. I would still handle the scenario entirely within the SMMU
driver. Can't we register a single handler for each of the IRQ listed
under the SMMU node and then figure out what was the notification for in
the handler? 

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Julien Grall]

Hi Stefano,

On 03/18/2014 02:06 PM, Stefano Stabellini wrote:
>> On 03/18/2014 09:33 AM, Ian Campbell wrote:
>>> On Mon, 2014-03-17 at 21:05 +0000, Julien Grall wrote:
>>>> For instance for the SMMU on midway, the device tree bindings is:
>>>>
>>>>                smmu_sata: smmu@9,20180000 {
>>>>                        compatible = "arm,mmu-400";
>>>>                        reg = <0x9 0x20180000 0x10000>;
>>>>                        mmu-masters = <&sata 0 1 2 3 4 5 6 7 8 9>;
>>>>                        #global-interrupts = <1>;
>>>>                        interrupts = <0 114 4 0 114 4>;
>>>>                        calxeda,smmu-secure-config-access;
>>>>                        arm,smmu-isolate-devices;
>>>>               };
>>>>
>>>> As you can see the same interrupts is used twice:
>>>
>>> Is that actually valid in device tree? Or is this a quirk of the midway
>>> DT?
>>
>> Yes it's valid. The interrupts property for the SMMU is described as:
>>
>> "Interrupt list, with the first #global-irqs entries corresponding to
>> the global interrupts and any following entries corresponding to context
>> interrupts, specified in order of their indexing by the SMMU.
>>
>> For SMMUv2 implementations, there must be exactly one interrupt per
>> context bank. In the case of a single, combined interrupt, it must be
>> listed multiple times."
>>
>> On midway there is only one IRQ with is used for both context interrupt
>> and global interrupt. As it's the only platform on Linux with SMMU
>> support in the device tree, we don't know if every platform will have
>> the same behavior.
> 
> I understand that the SMMU might reuse the same IRQ for multiple
> purposes. I would still handle the scenario entirely within the SMMU
> driver. Can't we register a single handler for each of the IRQ listed
> under the SMMU node and then figure out what was the notification for in
> the handler? 
> 

We will have to check in the SMMU drivers, if the IRQ was already
registered or not (because we don't know in advance if the IRQ is
re-used). If not, Xen will register it with a new handler.

The code to register the IRQ handler will looks like:

int num_irqs = dt_number_of_irq(smmu->node);
dt_irqs irqs[*];
dt_irq *irq;

for ( i = 0; i < num_irqs; i++ )
{
   irq = dt_device_get_irq(smmu->node, i);
   foreach ( a in irqs )
       if ( irq == a )
         continue;
   request_dt_irq();
   irqs <= irq;
}

I understand the point that people can badly use multiple action in the
future, but is it a good reason to make the code more difficult to
understand?

Regards,

-- 
Julien Grall

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Stefano Stabellini]

On Tue, 18 Mar 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 03/18/2014 02:06 PM, Stefano Stabellini wrote:
> >> On 03/18/2014 09:33 AM, Ian Campbell wrote:
> >>> On Mon, 2014-03-17 at 21:05 +0000, Julien Grall wrote:
> >>>> For instance for the SMMU on midway, the device tree bindings is:
> >>>>
> >>>>                smmu_sata: smmu@9,20180000 {
> >>>>                        compatible = "arm,mmu-400";
> >>>>                        reg = <0x9 0x20180000 0x10000>;
> >>>>                        mmu-masters = <&sata 0 1 2 3 4 5 6 7 8 9>;
> >>>>                        #global-interrupts = <1>;
> >>>>                        interrupts = <0 114 4 0 114 4>;
> >>>>                        calxeda,smmu-secure-config-access;
> >>>>                        arm,smmu-isolate-devices;
> >>>>               };
> >>>>
> >>>> As you can see the same interrupts is used twice:
> >>>
> >>> Is that actually valid in device tree? Or is this a quirk of the midway
> >>> DT?
> >>
> >> Yes it's valid. The interrupts property for the SMMU is described as:
> >>
> >> "Interrupt list, with the first #global-irqs entries corresponding to
> >> the global interrupts and any following entries corresponding to context
> >> interrupts, specified in order of their indexing by the SMMU.
> >>
> >> For SMMUv2 implementations, there must be exactly one interrupt per
> >> context bank. In the case of a single, combined interrupt, it must be
> >> listed multiple times."
> >>
> >> On midway there is only one IRQ with is used for both context interrupt
> >> and global interrupt. As it's the only platform on Linux with SMMU
> >> support in the device tree, we don't know if every platform will have
> >> the same behavior.
> > 
> > I understand that the SMMU might reuse the same IRQ for multiple
> > purposes. I would still handle the scenario entirely within the SMMU
> > driver. Can't we register a single handler for each of the IRQ listed
> > under the SMMU node and then figure out what was the notification for in
> > the handler? 
> > 
> 
> We will have to check in the SMMU drivers, if the IRQ was already
> registered or not (because we don't know in advance if the IRQ is
> re-used). If not, Xen will register it with a new handler.
> 
> The code to register the IRQ handler will looks like:
> 
> int num_irqs = dt_number_of_irq(smmu->node);
> dt_irqs irqs[*];
> dt_irq *irq;
> 
> for ( i = 0; i < num_irqs; i++ )
> {
>    irq = dt_device_get_irq(smmu->node, i);
>    foreach ( a in irqs )
>        if ( irq == a )
>          continue;
>    request_dt_irq();
>    irqs <= irq;
> }
> 
> I understand the point that people can badly use multiple action in the
> future, but is it a good reason to make the code more difficult to
> understand?

Can't we simply register all the irq handlers at boot time, regardless
of whether they are currently used?

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Julien Grall]

Hi Stefano,

On 03/18/2014 03:01 PM, Stefano Stabellini wrote:

>> I understand the point that people can badly use multiple action in the
>> future, but is it a good reason to make the code more difficult to
>> understand?
> 
> Can't we simply register all the irq handlers at boot time, regardless
> of whether they are currently used?
> 

This code is doing this actually. You need to know if the IRQ is
represented multiple times in the "interrupts" list or not.

Regards,

-- 
Julien Grall

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Stefano Stabellini]

On Tue, 18 Mar 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 03/18/2014 03:01 PM, Stefano Stabellini wrote:
> 
> >> I understand the point that people can badly use multiple action in the
> >> future, but is it a good reason to make the code more difficult to
> >> understand?
> > 
> > Can't we simply register all the irq handlers at boot time, regardless
> > of whether they are currently used?
> > 
> 
> This code is doing this actually. You need to know if the IRQ is
> represented multiple times in the "interrupts" list or not.

Oh, that's right.

You could rely on request_dt_irq() being able to handle failures
gracefully to make the code easier to read.
Overall I don't think the code is too bad.

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Julien Grall]

On 03/18/2014 03:39 PM, Stefano Stabellini wrote:
> On Tue, 18 Mar 2014, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 03/18/2014 03:01 PM, Stefano Stabellini wrote:
>>
>>>> I understand the point that people can badly use multiple action in the
>>>> future, but is it a good reason to make the code more difficult to
>>>> understand?
>>>
>>> Can't we simply register all the irq handlers at boot time, regardless
>>> of whether they are currently used?
>>>
>>
>> This code is doing this actually. You need to know if the IRQ is
>> represented multiple times in the "interrupts" list or not.
> 
> Oh, that's right.
> 
> You could rely on request_dt_irq() being able to handle failures
> gracefully to make the code easier to read.
> Overall I don't think the code is too bad.

request_dt_irq doesn't sound the right place to handle it. How will you
differentiate an IRQ already registered by another driver and an IRQ
registered by the driver itself?

IHMO, this hack is worst and could lead issue with the developper
because of misconception of the error code.

Regards,

-- 
Julien Grall

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Ian Campbell]

On Tue, 2014-03-18 at 14:54 +0000, Julien Grall wrote:
> Hi Stefano,
> 
> On 03/18/2014 02:06 PM, Stefano Stabellini wrote:
> >> On 03/18/2014 09:33 AM, Ian Campbell wrote:
> >>> On Mon, 2014-03-17 at 21:05 +0000, Julien Grall wrote:
> >>>> For instance for the SMMU on midway, the device tree bindings is:
> >>>>
> >>>>                smmu_sata: smmu@9,20180000 {
> >>>>                        compatible = "arm,mmu-400";
> >>>>                        reg = <0x9 0x20180000 0x10000>;
> >>>>                        mmu-masters = <&sata 0 1 2 3 4 5 6 7 8 9>;
> >>>>                        #global-interrupts = <1>;
> >>>>                        interrupts = <0 114 4 0 114 4>;
> >>>>                        calxeda,smmu-secure-config-access;
> >>>>                        arm,smmu-isolate-devices;
> >>>>               };
> >>>>
> >>>> As you can see the same interrupts is used twice:
> >>>
> >>> Is that actually valid in device tree? Or is this a quirk of the midway
> >>> DT?
> >>
> >> Yes it's valid. The interrupts property for the SMMU is described as:
> >>
> >> "Interrupt list, with the first #global-irqs entries corresponding to
> >> the global interrupts and any following entries corresponding to context
> >> interrupts, specified in order of their indexing by the SMMU.
> >>
> >> For SMMUv2 implementations, there must be exactly one interrupt per
> >> context bank. In the case of a single, combined interrupt, it must be
> >> listed multiple times."
> >>
> >> On midway there is only one IRQ with is used for both context interrupt
> >> and global interrupt. As it's the only platform on Linux with SMMU
> >> support in the device tree, we don't know if every platform will have
> >> the same behavior.
> > 
> > I understand that the SMMU might reuse the same IRQ for multiple
> > purposes. I would still handle the scenario entirely within the SMMU
> > driver. Can't we register a single handler for each of the IRQ listed
> > under the SMMU node and then figure out what was the notification for in
> > the handler? 
> > 
> 
> We will have to check in the SMMU drivers, if the IRQ was already
> registered or not (because we don't know in advance if the IRQ is
> re-used). If not, Xen will register it with a new handler.
> 
> The code to register the IRQ handler will looks like:
> 
> int num_irqs = dt_number_of_irq(smmu->node);

assert(num_irqs == 2) here and the rest gets a lot simpler. The maximum
is a property of the hardware I think, so the driver is allowed to make
such assumptions.

Ian.

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Julien Grall]

On 03/18/2014 03:02 PM, Ian Campbell wrote:
>> int num_irqs = dt_number_of_irq(smmu->node);
> 
> assert(num_irqs == 2) here and the rest gets a lot simpler. The maximum
> is a property of the hardware I think, so the driver is allowed to make
> such assumptions.

I know it would be easier ... but you can't assume that num_irqs == 2 :).

The number is not determined.

-- 
Julien Grall

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Ian Campbell]

On Tue, 2014-03-18 at 15:08 +0000, Julien Grall wrote:
> On 03/18/2014 03:02 PM, Ian Campbell wrote:
> >> int num_irqs = dt_number_of_irq(smmu->node);
> > 
> > assert(num_irqs == 2) here and the rest gets a lot simpler. The maximum
> > is a property of the hardware I think, so the driver is allowed to make
> > such assumptions.
> 
> I know it would be easier ... but you can't assume that num_irqs == 2 :).
> 
> The number is not determined.

Are you saying that the SMMU-400 has an arbitrary number of interrupts?

Ian.

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Julien Grall]

On 03/18/2014 03:10 PM, Ian Campbell wrote:
> On Tue, 2014-03-18 at 15:08 +0000, Julien Grall wrote:
>> On 03/18/2014 03:02 PM, Ian Campbell wrote:
>>>> int num_irqs = dt_number_of_irq(smmu->node);
>>>
>>> assert(num_irqs == 2) here and the rest gets a lot simpler. The maximum
>>> is a property of the hardware I think, so the driver is allowed to make
>>> such assumptions.
>>
>> I know it would be easier ... but you can't assume that num_irqs == 2 :).
>>
>> The number is not determined.
> 
> Are you saying that the SMMU-400 has an arbitrary number of interrupts?

Yes.

-- 
Julien Grall

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Ian Campbell]

On Tue, 2014-03-18 at 15:12 +0000, Julien Grall wrote:
> On 03/18/2014 03:10 PM, Ian Campbell wrote:
> > On Tue, 2014-03-18 at 15:08 +0000, Julien Grall wrote:
> >> On 03/18/2014 03:02 PM, Ian Campbell wrote:
> >>>> int num_irqs = dt_number_of_irq(smmu->node);
> >>>
> >>> assert(num_irqs == 2) here and the rest gets a lot simpler. The maximum
> >>> is a property of the hardware I think, so the driver is allowed to make
> >>> such assumptions.
> >>
> >> I know it would be easier ... but you can't assume that num_irqs == 2 :).
> >>
> >> The number is not determined.
> > 
> > Are you saying that the SMMU-400 has an arbitrary number of interrupts?
> 
> Yes.

And the relevant doc is in the bindings:

- #global-interrupts : The number of global interrupts exposed by the
                       device.

- interrupts    : Interrupt list, with the first #global-irqs entries
                  corresponding to the global interrupts and any
                  following entries corresponding to context interrupts,
                  specified in order of their indexing by the SMMU.

                  For SMMUv2 implementations, there must be exactly one
                  interrupt per context bank. In the case of a single,
                  combined interrupt, it must be listed multiple times.

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Julien Grall]

Hi all,

On 03/18/2014 03:26 PM, Ian Campbell wrote:
> On Tue, 2014-03-18 at 15:12 +0000, Julien Grall wrote:
>> On 03/18/2014 03:10 PM, Ian Campbell wrote:
>>> On Tue, 2014-03-18 at 15:08 +0000, Julien Grall wrote:
>>>> On 03/18/2014 03:02 PM, Ian Campbell wrote:
>>>>>> int num_irqs = dt_number_of_irq(smmu->node);
>>>>>
>>>>> assert(num_irqs == 2) here and the rest gets a lot simpler. The maximum
>>>>> is a property of the hardware I think, so the driver is allowed to make
>>>>> such assumptions.
>>>>
>>>> I know it would be easier ... but you can't assume that num_irqs == 2 :).
>>>>
>>>> The number is not determined.
>>>
>>> Are you saying that the SMMU-400 has an arbitrary number of interrupts?
>>
>> Yes.
> 
> And the relevant doc is in the bindings:
> 
> - #global-interrupts : The number of global interrupts exposed by the
>                        device.
> 
> - interrupts    : Interrupt list, with the first #global-irqs entries
>                   corresponding to the global interrupts and any
>                   following entries corresponding to context interrupts,
>                   specified in order of their indexing by the SMMU.
> 
>                   For SMMUv2 implementations, there must be exactly one
>                   interrupt per context bank. In the case of a single,
>                   combined interrupt, it must be listed multiple times.

I'm about to resend a new version of the interrupts and IOMMU support.

As I understand the main concern is to let the developer a "powerful"
way to handle multiple action on a same IRQ in the future.

Adding a such feature directly in the SMMU driver will be more complex
(see a preview in
http://www.gossamer-threads.com/lists/xen/devel/321318#321318) and IHMO
complicated the code just for protecting against developer. If in the
future next IOMMU drivers (or other kind of drivers in Xen) will come up
with IRQ shared, the code will be duplicated.

What is the final decision for the interrupt handling? Stefano would
prefer to let the SMMU drivers handle a such case. Ian, do you have any
opinion?

Regards,

-- 
Julien Grall

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Ian Campbell]

On Wed, 2014-03-19 at 17:18 +0000, Julien Grall wrote:
> What is the final decision for the interrupt handling? Stefano would
> prefer to let the SMMU drivers handle a such case. Ian, do you have any
> opinion?

It does sound like the end result is that shared interrupts need to be
handled somehow.

One potential issue is that a driver needs to be somewhat aware that it
can be using a shared interrupt (and e.g. not log spurious looking
interrupts). Linux solves this by having an explicit IRQF_SHARED which
the driver must pass iff it is prepared to cope with this situation and
the core should detect mismatches, which isn't a bad solution.

I think it's pretty clear from the discussion that routing an IRQ to the
guest must be equivalent to a driver which does not pass IRQF_SHARED.

There's also the question of error handling if/when a spurious interrupt
does occur (i.e. none of the multiple handlers dealt with it). Interrupt
handlers should perhaps return a status to say whether or not they did
anything with the interrupt, which the core code can then deal with
(logging, quenching etc).

Ian.

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Julien Grall]

On 02/19/2014 11:55 AM, Ian Campbell wrote:
> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same
>> interrupt.
> 
> Mention here that you are therefore creating a linked list of actions
> for each interrupt.
> 
> If you use xen/list.h for this then you get a load of helpers and
> iterators which would save you open coding them.

I've tried to use xen/list.h. The amount of code it's basically the same
and we I have to write open code to get the first element of the list
and complexify release_dt_irq which is not nice.

I will stick to the version with "next" for the next version of this series.

Regards,

-- 
Julien Grall

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Ian Campbell]

On Mon, 2014-03-31 at 16:45 +0100, Julien Grall wrote:
> On 02/19/2014 11:55 AM, Ian Campbell wrote:
> > On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
> >> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same
> >> interrupt.
> > 
> > Mention here that you are therefore creating a linked list of actions
> > for each interrupt.
> > 
> > If you use xen/list.h for this then you get a load of helpers and
> > iterators which would save you open coding them.
> 
> I've tried to use xen/list.h. The amount of code it's basically the same
> and we I have to write open code to get the first element of the list

Why? Can you post your WIP patch please for comparison.

> and complexify release_dt_irq which is not nice.
> 
> I will stick to the version with "next" for the next version of this series.
> 
> Regards,
> 

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Julien Grall]

On 03/31/2014 04:53 PM, Ian Campbell wrote:
> On Mon, 2014-03-31 at 16:45 +0100, Julien Grall wrote:
>> On 02/19/2014 11:55 AM, Ian Campbell wrote:
>>> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
>>>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same
>>>> interrupt.
>>>
>>> Mention here that you are therefore creating a linked list of actions
>>> for each interrupt.
>>>
>>> If you use xen/list.h for this then you get a load of helpers and
>>> iterators which would save you open coding them.
>>
>> I've tried to use xen/list.h. The amount of code it's basically the same
>> and we I have to write open code to get the first element of the list
> 
> Why? Can you post your WIP patch please for comparison.

Because:
	- there is no helper to get the first element (__setup_irq)
	- I need to use 2 variables to search for an element in a list as there is
	no way to know after the end of the loop if we found or not an element.

Below an incremental patch which change next field to a list (doesn't compile
and not finished):

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 7ea4da8..f4f5b71 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -545,36 +545,33 @@ void release_irq(unsigned int irq, const void *dev_id)
 {
     struct irq_desc *desc;
     unsigned long flags;
-    struct irqaction *action, **action_ptr;
+    struct irqaction *action, *next;
 
     desc = irq_to_desc(irq);
 
     spin_lock_irqsave(&desc->lock,flags);
-    desc->handler->shutdown(desc);
-    action = desc->action;
 
-    action_ptr = &desc->action;
-    for ( ;; )
+    action = NULL;
+    list_for_each_entry(next, &desc->action, next)
     {
-        action = *action_ptr;
-
-        if ( !action )
+        if ( next->dev_id == dev_id )
         {
-            printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n", irq);
-            return;
-        }
-
-        if ( action->dev_id == dev_id )
+            action = next;
             break;
+        }
+    }
 
-        action_ptr = &action->next;
+    if ( !action )
+    {
+        printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n", irq);
+        return;
     }
 
     /* Found it - remove it from the action list */
-    *action_ptr = action->next;
+    list_del_init(&action->next);
 
     /* If this was the list action, shut down the IRQ */
-    if ( !desc->action )
+    if ( list_empty(&desc->action) )
     {
         desc->handler->shutdown(desc);
         desc->status &= ~IRQ_GUEST;
@@ -592,12 +589,10 @@ void release_irq(unsigned int irq, const void *dev_id)
 static int __setup_irq(struct irq_desc *desc, struct irqaction *new,
                        unsigned int irqflags)
 {
-    struct irqaction *action = desc->action;
-
     ASSERT(new != NULL);
 
     /* Sanity check if the IRQ already have an action attached */
-    if ( action != NULL )
+    if ( !list_empty(&desc->action) )
     {
         /* Check that IRQ is marked as shared */
         if ( !(desc->status & IRQ_SHARED) || !(irqflags & IRQ_SHARED) )
@@ -610,8 +605,8 @@ static int __setup_irq(struct irq_desc *desc, struct irqaction *new,
     if ( irqflags & IRQ_SHARED )
         desc->status |= IRQ_SHARED;
 
-    new->next = desc->action;
-    desc->action = new;
+    INIT_LIST_HEAD(&new->next);
+    list_add_tail(&new->next, &desc->action);
     dsb(sy);
 
     return 0;
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index d6f3500..8a9ae3d 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -56,6 +56,7 @@ irq_desc_t *__irq_to_desc(int irq)
 int __init arch_init_one_irq_desc(struct irq_desc *desc)
 {
     desc->arch.type = DT_IRQ_TYPE_NONE;
+    INIT_LIST_HEAD(&desc->action);
     return 0;
 }
 
@@ -151,7 +152,6 @@ int request_irq(unsigned int irq,
 void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
 {
     struct irq_desc *desc = irq_to_desc(irq);
-    struct irqaction *action = desc->action;
 
     /* TODO: perfc_incr(irqs); */
 
@@ -162,7 +162,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
     spin_lock(&desc->lock);
     desc->handler->ack(desc);
 
-    if ( action == NULL )
+    if ( list_empty(&desc->action) )
     {
         printk("Unknown %s %#3.3x\n",
                is_fiq ? "FIQ" : "IRQ", irq);
@@ -171,6 +171,8 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
 
     if ( desc->status & IRQ_GUEST )
     {
+        struct irqaction *action = list_entry(&desc->action, struct irqaction,
+                                              next);
         struct domain *d = action->dev_id;
 
         desc->handler->end(desc);
@@ -194,16 +196,14 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
 
     desc->status |= IRQ_INPROGRESS;
 
-    action = desc->action;
     while ( desc->status & IRQ_PENDING )
     {
+        struct irqaction *action, *next;
+
         desc->status &= ~IRQ_PENDING;
         spin_unlock_irq(&desc->lock);
-        do
-        {
+        list_for_each_entry_safe(action, next, &desc->action, next)
             action->handler(irq, action->dev_id, regs);
-            action = action->next;
-        } while ( action );
         spin_lock_irq(&desc->lock);
     }
 
diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
index 20548aa..a926554 100644
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -17,7 +17,9 @@ struct irqaction {
     const char *name;
     void *dev_id;
     bool_t free_on_release;
-    struct irqaction *next;
+#ifdef CONFIG_ARM
+    struct list_head next;
+#endif
 };
 
 /*
@@ -73,7 +75,11 @@ typedef struct irq_desc {
     unsigned int status;        /* IRQ status */
     hw_irq_controller *handler;
     struct msi_desc   *msi_desc;
+#ifdef CONFIG_ARM
+    struct list_head action;    /* IRQ action list */
+#else
     struct irqaction *action;   /* IRQ action list */
+#endif
     int irq;
     spinlock_t lock;
     struct arch_irq_desc arch;

-- 
Julien Grall

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Ian Campbell]

On Mon, 2014-03-31 at 17:02 +0100, Julien Grall wrote:
> On 03/31/2014 04:53 PM, Ian Campbell wrote:
> > On Mon, 2014-03-31 at 16:45 +0100, Julien Grall wrote:
> >> On 02/19/2014 11:55 AM, Ian Campbell wrote:
> >>> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
> >>>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same
> >>>> interrupt.
> >>>
> >>> Mention here that you are therefore creating a linked list of actions
> >>> for each interrupt.
> >>>
> >>> If you use xen/list.h for this then you get a load of helpers and
> >>> iterators which would save you open coding them.
> >>
> >> I've tried to use xen/list.h. The amount of code it's basically the same
> >> and we I have to write open code to get the first element of the list
> > 
> > Why? Can you post your WIP patch please for comparison.
> 
> Because:
> 	- there is no helper to get the first element (__setup_irq)

Wrong function? I don't see any problem in __setup_irq.

> 	- I need to use 2 variables to search for an element in a list as there is
> 	no way to know after the end of the loop if we found or not an element.

You've written that a bit weirdly IMHO.

list_for_each(...)
   if (not the one we want)
	continue
   free the one we wanted
   break;

don't worry about warning on a non-existent IRQ, or set a simple
boolean.

Or refactor into a find_irq_by_devid helper and use that here.

> Below an incremental patch which change next field to a list (doesn't compile
> and not finished):

It really doesn't look that bad to me.

[...]
> -    struct irqaction *next;
> +#ifdef CONFIG_ARM
> +    struct list_head next;
> +#endif
[...]
> +#ifdef CONFIG_ARM
> +    struct list_head action;    /* IRQ action list */
> +#else
>      struct irqaction *action;   /* IRQ action list */
> +#endif

Now these might be a legitimate problem with this approach. At the very
least this should be CONFIG_IRQ_HAS_IRQ_ACTION_LIST or some more
suitable name.

Ian.

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Julien Grall]

On 04/01/2014 01:29 PM, Ian Campbell wrote:
> On Mon, 2014-03-31 at 17:02 +0100, Julien Grall wrote:
>> On 03/31/2014 04:53 PM, Ian Campbell wrote:
>>> On Mon, 2014-03-31 at 16:45 +0100, Julien Grall wrote:
>>>> On 02/19/2014 11:55 AM, Ian Campbell wrote:
>>>>> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
>>>>>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same
>>>>>> interrupt.
>>>>>
>>>>> Mention here that you are therefore creating a linked list of actions
>>>>> for each interrupt.
>>>>>
>>>>> If you use xen/list.h for this then you get a load of helpers and
>>>>> iterators which would save you open coding them.
>>>>
>>>> I've tried to use xen/list.h. The amount of code it's basically the same
>>>> and we I have to write open code to get the first element of the list
>>>
>>> Why? Can you post your WIP patch please for comparison.
>>
>> Because:
>> 	- there is no helper to get the first element (__setup_irq)
> 
> Wrong function? I don't see any problem in __setup_irq.

__setup_irq has to check if every action has a dev_id. After thinking, I
don't need to get the first action as now we have IRQ_SHARED flags set.

> 
>> 	- I need to use 2 variables to search for an element in a list as there is
>> 	no way to know after the end of the loop if we found or not an element.
> 
> You've written that a bit weirdly IMHO.
> 
> list_for_each(...)
>    if (not the one we want)
> 	continue
>    free the one we wanted
>    break;
> 
> don't worry about warning on a non-existent IRQ, or set a simple
> boolean.

We have to worry about non-existent action otherwise Xen may segfault...

> It really doesn't look that bad to me.

Ok. I will continue on this way then.

> [...]
>> -    struct irqaction *next;
>> +#ifdef CONFIG_ARM
>> +    struct list_head next;
>> +#endif
> [...]
>> +#ifdef CONFIG_ARM
>> +    struct list_head action;    /* IRQ action list */
>> +#else
>>      struct irqaction *action;   /* IRQ action list */
>> +#endif
> 
> Now these might be a legitimate problem with this approach. At the very
> least this should be CONFIG_IRQ_HAS_IRQ_ACTION_LIST or some more
> suitable name.

Ok. I will use it.

Regards,

-- 
Julien Grall

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Ian Campbell]

On Tue, 2014-04-01 at 14:13 +0100, Julien Grall wrote:
> >> 	- I need to use 2 variables to search for an element in a list as there is
> >> 	no way to know after the end of the loop if we found or not an element.
> > 
> > You've written that a bit weirdly IMHO.
> > 
> > list_for_each(...)
> >    if (not the one we want)
> > 	continue
> >    free the one we wanted
> >    break;
> > 
> > don't worry about warning on a non-existent IRQ, or set a simple
> > boolean.
> 
> We have to worry about non-existent action otherwise Xen may segfault...

Why? If it doesn't exist we don't do anything.

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Julien Grall]

On 04/01/2014 02:23 PM, Ian Campbell wrote:
> On Tue, 2014-04-01 at 14:13 +0100, Julien Grall wrote:
>>>> 	- I need to use 2 variables to search for an element in a list as there is
>>>> 	no way to know after the end of the loop if we found or not an element.
>>>
>>> You've written that a bit weirdly IMHO.
>>>
>>> list_for_each(...)
>>>    if (not the one we want)
>>> 	continue
>>>    free the one we wanted
>>>    break;
>>>
>>> don't worry about warning on a non-existent IRQ, or set a simple
>>> boolean.
>>
>> We have to worry about non-existent action otherwise Xen may segfault...
> 
> Why? If it doesn't exist we don't do anything.
> 
> 

We can't free in the loop because the action may be used on another CPU
at the same it. (see "active" loop at the end). So I still need two
variables (one for the loop and one for the real action).

-- 
Julien Grall

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Ian Campbell]

On Tue, 2014-04-01 at 14:52 +0100, Julien Grall wrote:
> On 04/01/2014 02:23 PM, Ian Campbell wrote:
> > On Tue, 2014-04-01 at 14:13 +0100, Julien Grall wrote:
> >>>> 	- I need to use 2 variables to search for an element in a list as there is
> >>>> 	no way to know after the end of the loop if we found or not an element.
> >>>
> >>> You've written that a bit weirdly IMHO.
> >>>
> >>> list_for_each(...)
> >>>    if (not the one we want)
> >>> 	continue
> >>>    free the one we wanted
> >>>    break;
> >>>
> >>> don't worry about warning on a non-existent IRQ, or set a simple
> >>> boolean.
> >>
> >> We have to worry about non-existent action otherwise Xen may segfault...
> > 
> > Why? If it doesn't exist we don't do anything.
> > 
> > 
> 
> We can't free in the loop because the action may be used on another CPU
> at the same it. (see "active" loop at the end). So I still need two
> variables (one for the loop and one for the real action).

Perhaps the "goto found" pattern might help here then?

Ian.

Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

[Julien Grall]

On 04/01/2014 03:31 PM, Ian Campbell wrote:
> On Tue, 2014-04-01 at 14:52 +0100, Julien Grall wrote:
>> On 04/01/2014 02:23 PM, Ian Campbell wrote:
>>> On Tue, 2014-04-01 at 14:13 +0100, Julien Grall wrote:
>>>>>> 	- I need to use 2 variables to search for an element in a list as there is
>>>>>> 	no way to know after the end of the loop if we found or not an element.
>>>>>
>>>>> You've written that a bit weirdly IMHO.
>>>>>
>>>>> list_for_each(...)
>>>>>    if (not the one we want)
>>>>> 	continue
>>>>>    free the one we wanted
>>>>>    break;
>>>>>
>>>>> don't worry about warning on a non-existent IRQ, or set a simple
>>>>> boolean.
>>>>
>>>> We have to worry about non-existent action otherwise Xen may segfault...
>>>
>>> Why? If it doesn't exist we don't do anything.
>>>
>>>
>>
>> We can't free in the loop because the action may be used on another CPU
>> at the same it. (see "active" loop at the end). So I still need two
>> variables (one for the loop and one for the real action).
> 
> Perhaps the "goto found" pattern might help here then?

I'm not a big fan of "goto". But in this case the code might be cleaner.

-- 
Julien Grall

[PATCH for-4.5 8/8] xen/serial: remove serial_dt_irq

[Julien Grall]

This function was only used for IRQ routing which has been removed in an
earlier patch.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
CC: Keir Fraser <keir@xen.org>
---
 xen/drivers/char/exynos4210-uart.c |    8 --------
 xen/drivers/char/ns16550.c         |   11 -----------
 xen/drivers/char/omap-uart.c       |    8 --------
 xen/drivers/char/pl011.c           |    8 --------
 xen/drivers/char/serial.c          |    9 ---------
 xen/include/xen/serial.h           |    5 -----
 6 files changed, 49 deletions(-)

diff --git a/xen/drivers/char/exynos4210-uart.c b/xen/drivers/char/exynos4210-uart.c
index 74ac33d..9179138 100644
--- a/xen/drivers/char/exynos4210-uart.c
+++ b/xen/drivers/char/exynos4210-uart.c
@@ -275,13 +275,6 @@ static int __init exynos4210_uart_irq(struct serial_port *port)
     return uart->irq.irq;
 }
 
-static const struct dt_irq __init *exynos4210_uart_dt_irq(struct serial_port *port)
-{
-    struct exynos4210_uart *uart = port->uart;
-
-    return &uart->irq;
-}
-
 static const struct vuart_info *exynos4210_vuart_info(struct serial_port *port)
 {
     struct exynos4210_uart *uart = port->uart;
@@ -299,7 +292,6 @@ static struct uart_driver __read_mostly exynos4210_uart_driver = {
     .putc         = exynos4210_uart_putc,
     .getc         = exynos4210_uart_getc,
     .irq          = exynos4210_uart_irq,
-    .dt_irq_get   = exynos4210_uart_dt_irq,
     .vuart_info   = exynos4210_vuart_info,
 };
 
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index e7cb0ba..ca16d48 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -446,14 +446,6 @@ static int __init ns16550_irq(struct serial_port *port)
     return ((uart->irq > 0) ? uart->irq : -1);
 }
 
-#ifdef HAS_DEVICE_TREE
-static const struct dt_irq __init *ns16550_dt_irq(struct serial_port *port)
-{
-    struct ns16550 *uart = port->uart;
-    return &uart->dt_irq;
-}
-#endif
-
 #ifdef CONFIG_ARM
 static const struct vuart_info *ns16550_vuart_info(struct serial_port *port)
 {
@@ -473,9 +465,6 @@ static struct uart_driver __read_mostly ns16550_driver = {
     .putc         = ns16550_putc,
     .getc         = ns16550_getc,
     .irq          = ns16550_irq,
-#ifdef HAS_DEVICE_TREE
-    .dt_irq_get   = ns16550_dt_irq,
-#endif
 #ifdef CONFIG_ARM
     .vuart_info   = ns16550_vuart_info,
 #endif
diff --git a/xen/drivers/char/omap-uart.c b/xen/drivers/char/omap-uart.c
index 7f21f1f..6d882a3 100644
--- a/xen/drivers/char/omap-uart.c
+++ b/xen/drivers/char/omap-uart.c
@@ -262,13 +262,6 @@ static int __init omap_uart_irq(struct serial_port *port)
     return ((uart->irq.irq > 0) ? uart->irq.irq : -1);
 }
 
-static const struct dt_irq __init *omap_uart_dt_irq(struct serial_port *port)
-{
-    struct omap_uart *uart = port->uart;
-
-    return &uart->irq;
-}
-
 static const struct vuart_info *omap_vuart_info(struct serial_port *port)
 {
     struct omap_uart *uart = port->uart;
@@ -286,7 +279,6 @@ static struct uart_driver __read_mostly omap_uart_driver = {
     .putc = omap_uart_putc,
     .getc = omap_uart_getc,
     .irq = omap_uart_irq,
-    .dt_irq_get = omap_uart_dt_irq,
     .vuart_info = omap_vuart_info,
 };
 
diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
index 9c2870a..ac9c4f8 100644
--- a/xen/drivers/char/pl011.c
+++ b/xen/drivers/char/pl011.c
@@ -189,13 +189,6 @@ static int __init pl011_irq(struct serial_port *port)
     return ((uart->irq.irq > 0) ? uart->irq.irq : -1);
 }
 
-static const struct dt_irq __init *pl011_dt_irq(struct serial_port *port)
-{
-    struct pl011 *uart = port->uart;
-
-    return &uart->irq;
-}
-
 static const struct vuart_info *pl011_vuart(struct serial_port *port)
 {
     struct pl011 *uart = port->uart;
@@ -213,7 +206,6 @@ static struct uart_driver __read_mostly pl011_driver = {
     .putc         = pl011_putc,
     .getc         = pl011_getc,
     .irq          = pl011_irq,
-    .dt_irq_get   = pl011_dt_irq,
     .vuart_info   = pl011_vuart,
 };
 
diff --git a/xen/drivers/char/serial.c b/xen/drivers/char/serial.c
index 9b006f2..44026b1 100644
--- a/xen/drivers/char/serial.c
+++ b/xen/drivers/char/serial.c
@@ -500,15 +500,6 @@ int __init serial_irq(int idx)
     return -1;
 }
 
-const struct dt_irq __init *serial_dt_irq(int idx)
-{
-    if ( (idx >= 0) && (idx < ARRAY_SIZE(com)) &&
-         com[idx].driver && com[idx].driver->dt_irq_get )
-        return com[idx].driver->dt_irq_get(&com[idx]);
-
-    return NULL;
-}
-
 const struct vuart_info *serial_vuart_info(int idx)
 {
     if ( (idx >= 0) && (idx < ARRAY_SIZE(com)) &&
diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
index f38c9b7..9f4451b 100644
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -81,8 +81,6 @@ struct uart_driver {
     int  (*getc)(struct serial_port *, char *);
     /* Get IRQ number for this port's serial line: returns -1 if none. */
     int  (*irq)(struct serial_port *);
-    /* Get IRQ device node for this port's serial line: returns NULL if none. */
-    const struct dt_irq *(*dt_irq_get)(struct serial_port *);
     /* Get serial information */
     const struct vuart_info *(*vuart_info)(struct serial_port *);
 };
@@ -135,9 +133,6 @@ void serial_end_log_everything(int handle);
 /* Return irq number for specified serial port (identified by index). */
 int serial_irq(int idx);
 
-/* Return irq device node for specified serial port (identified by index). */
-const struct dt_irq *serial_dt_irq(int idx);
-
 /* Retrieve basic UART information to emulate it (base address, size...) */
 const struct vuart_info* serial_vuart_info(int idx);
 
-- 
1.7.10.4

Re: [PATCH for-4.5 8/8] xen/serial: remove serial_dt_irq

[Ian Campbell]

On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
> This function was only used for IRQ routing which has been removed in an
> earlier patch.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> CC: Keir Fraser <keir@xen.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  xen/drivers/char/exynos4210-uart.c |    8 --------
>  xen/drivers/char/ns16550.c         |   11 -----------
>  xen/drivers/char/omap-uart.c       |    8 --------
>  xen/drivers/char/pl011.c           |    8 --------
>  xen/drivers/char/serial.c          |    9 ---------
>  xen/include/xen/serial.h           |    5 -----
>  6 files changed, 49 deletions(-)
> 
> diff --git a/xen/drivers/char/exynos4210-uart.c b/xen/drivers/char/exynos4210-uart.c
> index 74ac33d..9179138 100644
> --- a/xen/drivers/char/exynos4210-uart.c
> +++ b/xen/drivers/char/exynos4210-uart.c
> @@ -275,13 +275,6 @@ static int __init exynos4210_uart_irq(struct serial_port *port)
>      return uart->irq.irq;
>  }
>  
> -static const struct dt_irq __init *exynos4210_uart_dt_irq(struct serial_port *port)
> -{
> -    struct exynos4210_uart *uart = port->uart;
> -
> -    return &uart->irq;
> -}
> -
>  static const struct vuart_info *exynos4210_vuart_info(struct serial_port *port)
>  {
>      struct exynos4210_uart *uart = port->uart;
> @@ -299,7 +292,6 @@ static struct uart_driver __read_mostly exynos4210_uart_driver = {
>      .putc         = exynos4210_uart_putc,
>      .getc         = exynos4210_uart_getc,
>      .irq          = exynos4210_uart_irq,
> -    .dt_irq_get   = exynos4210_uart_dt_irq,
>      .vuart_info   = exynos4210_vuart_info,
>  };
>  
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index e7cb0ba..ca16d48 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -446,14 +446,6 @@ static int __init ns16550_irq(struct serial_port *port)
>      return ((uart->irq > 0) ? uart->irq : -1);
>  }
>  
> -#ifdef HAS_DEVICE_TREE
> -static const struct dt_irq __init *ns16550_dt_irq(struct serial_port *port)
> -{
> -    struct ns16550 *uart = port->uart;
> -    return &uart->dt_irq;
> -}
> -#endif
> -
>  #ifdef CONFIG_ARM
>  static const struct vuart_info *ns16550_vuart_info(struct serial_port *port)
>  {
> @@ -473,9 +465,6 @@ static struct uart_driver __read_mostly ns16550_driver = {
>      .putc         = ns16550_putc,
>      .getc         = ns16550_getc,
>      .irq          = ns16550_irq,
> -#ifdef HAS_DEVICE_TREE
> -    .dt_irq_get   = ns16550_dt_irq,
> -#endif
>  #ifdef CONFIG_ARM
>      .vuart_info   = ns16550_vuart_info,
>  #endif
> diff --git a/xen/drivers/char/omap-uart.c b/xen/drivers/char/omap-uart.c
> index 7f21f1f..6d882a3 100644
> --- a/xen/drivers/char/omap-uart.c
> +++ b/xen/drivers/char/omap-uart.c
> @@ -262,13 +262,6 @@ static int __init omap_uart_irq(struct serial_port *port)
>      return ((uart->irq.irq > 0) ? uart->irq.irq : -1);
>  }
>  
> -static const struct dt_irq __init *omap_uart_dt_irq(struct serial_port *port)
> -{
> -    struct omap_uart *uart = port->uart;
> -
> -    return &uart->irq;
> -}
> -
>  static const struct vuart_info *omap_vuart_info(struct serial_port *port)
>  {
>      struct omap_uart *uart = port->uart;
> @@ -286,7 +279,6 @@ static struct uart_driver __read_mostly omap_uart_driver = {
>      .putc = omap_uart_putc,
>      .getc = omap_uart_getc,
>      .irq = omap_uart_irq,
> -    .dt_irq_get = omap_uart_dt_irq,
>      .vuart_info = omap_vuart_info,
>  };
>  
> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
> index 9c2870a..ac9c4f8 100644
> --- a/xen/drivers/char/pl011.c
> +++ b/xen/drivers/char/pl011.c
> @@ -189,13 +189,6 @@ static int __init pl011_irq(struct serial_port *port)
>      return ((uart->irq.irq > 0) ? uart->irq.irq : -1);
>  }
>  
> -static const struct dt_irq __init *pl011_dt_irq(struct serial_port *port)
> -{
> -    struct pl011 *uart = port->uart;
> -
> -    return &uart->irq;
> -}
> -
>  static const struct vuart_info *pl011_vuart(struct serial_port *port)
>  {
>      struct pl011 *uart = port->uart;
> @@ -213,7 +206,6 @@ static struct uart_driver __read_mostly pl011_driver = {
>      .putc         = pl011_putc,
>      .getc         = pl011_getc,
>      .irq          = pl011_irq,
> -    .dt_irq_get   = pl011_dt_irq,
>      .vuart_info   = pl011_vuart,
>  };
>  
> diff --git a/xen/drivers/char/serial.c b/xen/drivers/char/serial.c
> index 9b006f2..44026b1 100644
> --- a/xen/drivers/char/serial.c
> +++ b/xen/drivers/char/serial.c
> @@ -500,15 +500,6 @@ int __init serial_irq(int idx)
>      return -1;
>  }
>  
> -const struct dt_irq __init *serial_dt_irq(int idx)
> -{
> -    if ( (idx >= 0) && (idx < ARRAY_SIZE(com)) &&
> -         com[idx].driver && com[idx].driver->dt_irq_get )
> -        return com[idx].driver->dt_irq_get(&com[idx]);
> -
> -    return NULL;
> -}
> -
>  const struct vuart_info *serial_vuart_info(int idx)
>  {
>      if ( (idx >= 0) && (idx < ARRAY_SIZE(com)) &&
> diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
> index f38c9b7..9f4451b 100644
> --- a/xen/include/xen/serial.h
> +++ b/xen/include/xen/serial.h
> @@ -81,8 +81,6 @@ struct uart_driver {
>      int  (*getc)(struct serial_port *, char *);
>      /* Get IRQ number for this port's serial line: returns -1 if none. */
>      int  (*irq)(struct serial_port *);
> -    /* Get IRQ device node for this port's serial line: returns NULL if none. */
> -    const struct dt_irq *(*dt_irq_get)(struct serial_port *);
>      /* Get serial information */
>      const struct vuart_info *(*vuart_info)(struct serial_port *);
>  };
> @@ -135,9 +133,6 @@ void serial_end_log_everything(int handle);
>  /* Return irq number for specified serial port (identified by index). */
>  int serial_irq(int idx);
>  
> -/* Return irq device node for specified serial port (identified by index). */
> -const struct dt_irq *serial_dt_irq(int idx);
> -
>  /* Retrieve basic UART information to emulate it (base address, size...) */
>  const struct vuart_info* serial_vuart_info(int idx);
>