[PATCH v2 00/16] Interrupt management reworking

[PATCH v2 00/16] Interrupt management reworking

[Julien Grall]

Hello,

This is the second version of this series to rework interrupt management.

Compare to the RFC sent a couple of months ago, I took a different approach
for handling multiple action per IRQ. If the driver wants to use this feature,
it has to explicitely request it by passing the flag IRQ_SHARED to the
newly created functions ({request,setup}_irq_flags). This will prevent
developpers to mix the interrupt by mistake.

While I was working on it, I also took time to rework the interrupt management
to:
    - Make distinction between GIC and IRQ management
    - Drop {request,setup}_dt_irq which is ARM specific and use the
    common one
    - Merge route and setup IRQ functions
    - Improve error checking

Having generic IRQ function will avoid ifdery in driver code and will hopefully
help to port new feature such as ACPI.

This series is a dependencies for the ARM SMMU drivers (the V2 will be sent
soon.

A working tree can be found here:
    git://xenbits.xen.org/people/julieng/xen-unstable.git branch interrupts-mgmt-v2

Sincerely yours,

Julien Grall (16):
  xen/arm: timer: replace timer_dt_irq by timer_get_irq
  xen/arm: IRQ: Use default irq callback from common code for
    no_irq_type
  xen/arm: IRQ: Rename irq_cfg into arch_irq_desc
  xen/arm: IRQ: move gic {,un}lock in gic_set_irq_properties
  xen/arm: IRQ: drop irq parameter in __setup_irq
  xen/arm: IRQ: remove __init from setup_dt_irq, request_dt_irq and
    release_irq
  xen/arm: IRQ: Move IRQ management from gic.c to irq.c
  xen/arm: IRQ Introduce irq_get_domain
  xen/arm: IRQ: Add lock contrainst for gic_irq_{startup,shutdown}
  xen/arm: IRQ: Don't need to have a specific function to route IRQ to
    Xen
  xen/arm: IRQ: Protect IRQ to be shared between domains and XEN
  xen/serial: remove serial_dt_irq
  xen/arm: IRQ: Store IRQ type in arch_irq_desc
  xen/arm: IRQ: Replace {request,setup}_dt_irq by {request,setup}_irq
  xen: IRQ: Add dev_id parameter to release_irq
  xen/arm: IRQ: Handle multiple action per IRQ

 xen/arch/arm/domain_build.c        |   32 ++--
 xen/arch/arm/gic.c                 |  245 +++++++--------------------
 xen/arch/arm/irq.c                 |  321 +++++++++++++++++++++++++++++++++---
 xen/arch/arm/setup.c               |    5 +-
 xen/arch/arm/smpboot.c             |    2 -
 xen/arch/arm/time.c                |   41 ++---
 xen/arch/arm/vtimer.c              |    4 +-
 xen/arch/x86/irq.c                 |    2 +-
 xen/common/irq.c                   |    3 +
 xen/drivers/char/exynos4210-uart.c |   22 +--
 xen/drivers/char/ns16550.c         |   28 +---
 xen/drivers/char/omap-uart.c       |   22 +--
 xen/drivers/char/pl011.c           |   23 +--
 xen/drivers/char/serial.c          |    9 -
 xen/include/asm-arm/config.h       |    2 +
 xen/include/asm-arm/gic.h          |   16 +-
 xen/include/asm-arm/irq.h          |   19 ++-
 xen/include/asm-arm/time.h         |    7 +-
 xen/include/xen/irq.h              |   10 +-
 xen/include/xen/serial.h           |    5 -
 20 files changed, 462 insertions(+), 356 deletions(-)

-- 
1.7.10.4

[PATCH v2 01/16] xen/arm: timer: replace timer_dt_irq by timer_get_irq

[Julien Grall]

The function is nearly only used to retrieve the IRQ number.

There is one place where the IRQ type is used (in domain_build.c) but
as the timer IRQ is virtualised for guest we might not have the same property
(e.g active-low level sensitive interrupt).

Replace timer_dt_irq by timer_get_irq which will return the IRQ number.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/domain_build.c |   23 +++++++++++++----------
 xen/arch/arm/time.c         |    4 ++--
 xen/arch/arm/vtimer.c       |    4 ++--
 xen/include/asm-arm/time.h  |    4 ++--
 4 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 502db84..2035390 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -620,7 +620,7 @@ static int make_timer_node(const struct domain *d, void *fdt,
     u32 len;
     const void *compatible;
     int res;
-    const struct dt_irq *irq;
+    unsigned int irq;
     gic_interrupt_t intrs[3];
 
     DPRINT("Create timer node\n");
@@ -647,17 +647,20 @@ static int make_timer_node(const struct domain *d, void *fdt,
     if ( res )
         return res;
 
-    irq = timer_dt_irq(TIMER_PHYS_SECURE_PPI);
-    DPRINT("  Secure interrupt %u\n", irq->irq);
-    set_interrupt_ppi(intrs[0], irq->irq, 0xf, irq->type);
+    /* The timer IRQ is emulated by Xen. It always exposes an active-low
+     * level-sensitive interrupt */
 
-    irq = timer_dt_irq(TIMER_PHYS_NONSECURE_PPI);
-    DPRINT("  Non secure interrupt %u\n", irq->irq);
-    set_interrupt_ppi(intrs[1], irq->irq, 0xf, irq->type);
+    irq = timer_get_irq(TIMER_PHYS_SECURE_PPI);
+    DPRINT("  Secure interrupt %u\n", irq);
+    set_interrupt_ppi(intrs[0], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
 
-    irq = timer_dt_irq(TIMER_VIRT_PPI);
-    DPRINT("  Virt interrupt %u\n", irq->irq);
-    set_interrupt_ppi(intrs[2], irq->irq, 0xf, irq->type);
+    irq = timer_get_irq(TIMER_PHYS_NONSECURE_PPI);
+    DPRINT("  Non secure interrupt %u\n", irq);
+    set_interrupt_ppi(intrs[1], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
+
+    irq = timer_get_irq(TIMER_VIRT_PPI);
+    DPRINT("  Virt interrupt %u\n", irq);
+    set_interrupt_ppi(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
 
     res = fdt_property_interrupts(fdt, intrs, 3);
     if ( res )
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 8dd4bea..7cad888 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -50,11 +50,11 @@ unsigned long __read_mostly cpu_khz;  /* CPU clock frequency in kHz. */
 
 static struct dt_irq timer_irq[MAX_TIMER_PPI];
 
-const struct dt_irq *timer_dt_irq(enum timer_ppi ppi)
+unsigned int timer_get_irq(enum timer_ppi ppi)
 {
     ASSERT(ppi >= TIMER_PHYS_SECURE_PPI && ppi < MAX_TIMER_PPI);
 
-    return &timer_irq[ppi];
+    return timer_irq[ppi].irq;
 }
 
 /*static inline*/ s_time_t ticks_to_ns(uint64_t ticks)
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 3d6a721..5742303 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -64,7 +64,7 @@ int vcpu_vtimer_init(struct vcpu *v)
     t->ctl = 0;
     t->cval = NOW();
     t->irq = d0
-        ? timer_dt_irq(TIMER_PHYS_NONSECURE_PPI)->irq
+        ? timer_get_irq(TIMER_PHYS_NONSECURE_PPI)
         : GUEST_TIMER_PHYS_NS_PPI;
     t->v = v;
 
@@ -72,7 +72,7 @@ int vcpu_vtimer_init(struct vcpu *v)
     init_timer(&t->timer, virt_timer_expired, t, v->processor);
     t->ctl = 0;
     t->irq = d0
-        ? timer_dt_irq(TIMER_VIRT_PPI)->irq
+        ? timer_get_irq(TIMER_VIRT_PPI)
         : GUEST_TIMER_VIRT_PPI;
     t->v = v;
 
diff --git a/xen/include/asm-arm/time.h b/xen/include/asm-arm/time.h
index d10c737..9bbab0b 100644
--- a/xen/include/asm-arm/time.h
+++ b/xen/include/asm-arm/time.h
@@ -22,8 +22,8 @@ enum timer_ppi
     MAX_TIMER_PPI = 4,
 };
 
-/* Get one of the timer IRQ description */
-const struct dt_irq* timer_dt_irq(enum timer_ppi ppi);
+/* Get one of the timer IRQ number */
+unsigned int timer_get_irq(enum timer_ppi ppi);
 
 /* Route timer's IRQ on this CPU */
 extern void __cpuinit route_timer_interrupt(void);
-- 
1.7.10.4

Re: [PATCH v2 01/16] xen/arm: timer: replace timer_dt_irq by timer_get_irq

[Ian Campbell]

On Thu, 2014-04-03 at 21:41 +0100, Julien Grall wrote:
> The function is nearly only used to retrieve the IRQ number.
> 
> There is one place where the IRQ type is used (in domain_build.c) but
> as the timer IRQ is virtualised for guest we might not have the same property
> (e.g active-low level sensitive interrupt).

Is this statement impacted at all by Stefano switching things to use the
LR.HW bit?

> 
> Replace timer_dt_irq by timer_get_irq which will return the IRQ number.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

> @@ -647,17 +647,20 @@ static int make_timer_node(const struct domain *d, void *fdt,
>      if ( res )
>          return res;
>  
> -    irq = timer_dt_irq(TIMER_PHYS_SECURE_PPI);
> -    DPRINT("  Secure interrupt %u\n", irq->irq);
> -    set_interrupt_ppi(intrs[0], irq->irq, 0xf, irq->type);
> +    /* The timer IRQ is emulated by Xen. It always exposes an active-low
> +     * level-sensitive interrupt */
>  
> -    irq = timer_dt_irq(TIMER_PHYS_NONSECURE_PPI);
> -    DPRINT("  Non secure interrupt %u\n", irq->irq);
> -    set_interrupt_ppi(intrs[1], irq->irq, 0xf, irq->type);
> +    irq = timer_get_irq(TIMER_PHYS_SECURE_PPI);
> +    DPRINT("  Secure interrupt %u\n", irq);
> +    set_interrupt_ppi(intrs[0], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
>  
> -    irq = timer_dt_irq(TIMER_VIRT_PPI);
> -    DPRINT("  Virt interrupt %u\n", irq->irq);
> -    set_interrupt_ppi(intrs[2], irq->irq, 0xf, irq->type);
> +    irq = timer_get_irq(TIMER_PHYS_NONSECURE_PPI);
> +    DPRINT("  Non secure interrupt %u\n", irq);
> +    set_interrupt_ppi(intrs[1], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
> +
> +    irq = timer_get_irq(TIMER_VIRT_PPI);
> +    DPRINT("  Virt interrupt %u\n", irq);
> +    set_interrupt_ppi(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
>  
>      res = fdt_property_interrupts(fdt, intrs, 3);
>      if ( res )

Re: [PATCH v2 01/16] xen/arm: timer: replace timer_dt_irq by timer_get_irq

[Julien Grall]

On 04/07/2014 02:10 PM, Ian Campbell wrote:
> On Thu, 2014-04-03 at 21:41 +0100, Julien Grall wrote:
>> The function is nearly only used to retrieve the IRQ number.
>>
>> There is one place where the IRQ type is used (in domain_build.c) but
>> as the timer IRQ is virtualised for guest we might not have the same property
>> (e.g active-low level sensitive interrupt).
> 
> Is this statement impacted at all by Stefano switching things to use the
> LR.HW bit?

The LR.HW is only used for physical interrupt routed to the guest. As
the timer interrupt is virtualised it should not impact anything.

Regards,

-- 
Julien Grall

[PATCH v2 02/16] xen/arm: IRQ: Use default irq callback from common code for no_irq_type

[Julien Grall]

Most of no_irq_type callback are already defined in common/irq.c. We don't
need to recreate our own callbacks.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/irq.c |   14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 3e326b0..5d1ed7f 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -27,23 +27,19 @@
 
 #include <asm/gic.h>
 
-static void enable_none(struct irq_desc *irq) { }
-static unsigned int startup_none(struct irq_desc *irq) { return 0; }
-static void disable_none(struct irq_desc *irq) { }
 static void ack_none(struct irq_desc *irq)
 {
     printk("unexpected IRQ trap at irq %02x\n", irq->irq);
 }
 
-#define shutdown_none   disable_none
-#define end_none        enable_none
+static void end_none(struct irq_desc *irq) { }
 
 hw_irq_controller no_irq_type = {
     .typename = "none",
-    .startup = startup_none,
-    .shutdown = shutdown_none,
-    .enable = enable_none,
-    .disable = disable_none,
+    .startup = irq_startup_none,
+    .shutdown = irq_shutdown_none,
+    .enable = irq_enable_none,
+    .disable = irq_disable_none,
     .ack = ack_none,
     .end = end_none
 };
-- 
1.7.10.4

Re: [PATCH v2 02/16] xen/arm: IRQ: Use default irq callback from common code for no_irq_type

[Ian Campbell]

On Thu, 2014-04-03 at 21:41 +0100, Julien Grall wrote:
> Most of no_irq_type callback are already defined in common/irq.c. We don't
> need to recreate our own callbacks.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

[PATCH v2 03/16] xen/arm: IRQ: Rename irq_cfg into arch_irq_desc

[Julien Grall]

irq_cfg is never used in the code and arch_irq_desc is an alias to irq_cfg.

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

---
    Changes in v2:
        - Patch added
---
 xen/include/asm-arm/irq.h |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index 7c20703..3197aec 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -14,8 +14,7 @@ struct arch_pirq
 {
 };
 
-struct irq_cfg {
-#define arch_irq_desc irq_cfg
+struct arch_irq_desc {
     int eoi_cpu;
 };
 
-- 
1.7.10.4

Re: [PATCH v2 03/16] xen/arm: IRQ: Rename irq_cfg into arch_irq_desc

[Ian Campbell]

On Thu, 2014-04-03 at 21:41 +0100, Julien Grall wrote:
> irq_cfg is never used in the code and arch_irq_desc is an alias to irq_cfg.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

[PATCH v2 04/16] 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>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
    Changes in v2:
        - Remove useless comment about gic.lock
---
 xen/arch/arm/gic.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 0095b97..9bf7e1e 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -221,7 +221,6 @@ static hw_irq_controller gic_guest_irq_type = {
 };
 
 /*
- * - needs to be called with gic.lock held
  * - needs to be called with a valid cpu_mask, ie each cpu in the mask has
  * already called gic_cpu_init
  */
@@ -231,7 +230,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];
@@ -250,6 +253,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 */
@@ -272,9 +276,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;
@@ -779,7 +781,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;
@@ -800,7 +801,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

[PATCH v2 05/16] xen/arm: IRQ: drop irq parameter in __setup_irq

[Julien Grall]

The IRQ number is already provided by desc.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/gic.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 9bf7e1e..19db06a 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -594,8 +594,7 @@ void __init release_irq(unsigned int irq)
         xfree(action);
 }
 
-static int __setup_irq(struct irq_desc *desc, unsigned int irq,
-                       struct irqaction *new)
+static int __setup_irq(struct irq_desc *desc, struct irqaction *new)
 {
     if ( desc->action != NULL )
         return -EBUSY;
@@ -615,7 +614,7 @@ 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);
-    rc = __setup_irq(desc, irq->irq, new);
+    rc = __setup_irq(desc, new);
     spin_unlock_irqrestore(&desc->lock, flags);
 
     if ( !rc )
@@ -790,7 +789,7 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
     gic_set_irq_properties(irq->irq, level, cpumask_of(smp_processor_id()),
                            GIC_PRI_IRQ);
 
-    retval = __setup_irq(desc, irq->irq, action);
+    retval = __setup_irq(desc, action);
     if (retval) {
         xfree(action);
         goto out;
-- 
1.7.10.4

Re: [PATCH v2 05/16] xen/arm: IRQ: drop irq parameter in __setup_irq

[Ian Campbell]

On Thu, 2014-04-03 at 21:41 +0100, Julien Grall wrote:
> The IRQ number is already provided by desc.

and __setup_irq doesn't use it in any case.

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

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

Re: [PATCH v2 05/16] xen/arm: IRQ: drop irq parameter in __setup_irq

[Julien Grall]

On 04/07/2014 02:05 PM, Ian Campbell wrote:
> On Thu, 2014-04-03 at 21:41 +0100, Julien Grall wrote:
>> The IRQ number is already provided by desc.
> 
> and __setup_irq doesn't use it in any case.

I will update the commit message.


>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks,

-- 
Julien Grall

[PATCH v2 06/16] xen/arm: IRQ: remove __init from setup_dt_irq, request_dt_irq and release_irq

[Julien Grall]

These functions will be used in SMMU driver which request interrupt when
a device is assigned to a guest.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/gic.c        |    4 ++--
 xen/arch/arm/irq.c        |    6 +++---
 xen/include/asm-arm/irq.h |    8 ++++----
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 19db06a..36cf93d 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -570,7 +570,7 @@ void gic_route_spis(void)
     }
 }
 
-void __init release_irq(unsigned int irq)
+void release_irq(unsigned int irq)
 {
     struct irq_desc *desc;
     unsigned long flags;
@@ -605,7 +605,7 @@ static int __setup_irq(struct irq_desc *desc, struct irqaction *new)
     return 0;
 }
 
-int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
+int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
 {
     int rc;
     unsigned long flags;
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 5d1ed7f..b3bfebc 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -89,9 +89,9 @@ void __cpuinit init_secondary_IRQ(void)
     BUG_ON(init_local_irq_data() < 0);
 }
 
-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 request_dt_irq(const struct dt_irq *irq,
+                   void (*handler)(int, void *, struct cpu_user_regs *),
+                   const char *devname, void *dev_id)
 {
     struct irqaction *action;
     int retval;
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index 3197aec..9380987 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -39,10 +39,10 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq);
 void init_IRQ(void);
 void init_secondary_IRQ(void);
 
-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);
+int request_dt_irq(const struct dt_irq *irq,
+                   void (*handler)(int, void *, struct cpu_user_regs *),
+                   const char *devname, void *dev_id);
+int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new);
 
 #endif /* _ASM_HW_IRQ_H */
 /*
-- 
1.7.10.4

Re: [PATCH v2 06/16] xen/arm: IRQ: remove __init from setup_dt_irq, request_dt_irq and release_irq

[Ian Campbell]

On Thu, 2014-04-03 at 21:41 +0100, Julien Grall wrote:
> These functions will be used in SMMU driver which request interrupt when
> a device is assigned to a guest.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

[PATCH v2 07/16] xen/arm: IRQ: Move IRQ management from gic.c to irq.c

[Julien Grall]

The file gic.c contains functions and variables which is not related to the GIC:
    - release_irq
    - setup_irq
    - gic_route_irq_to_guest
    - {,local_}irq_desc

Move all theses functions/variables in irq.c and split gic_route_irq_to_guest
in 2 parts:
    - route_dt_irq_to_guest: setup the desc
    - gic_route_irq_to_guest: setup correctly the GIC and the desc
    handler

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/domain_build.c |    2 +-
 xen/arch/arm/gic.c          |  125 ++++++++-----------------------------------
 xen/arch/arm/irq.c          |   97 +++++++++++++++++++++++++++++++++
 xen/include/asm-arm/gic.h   |    7 +--
 xen/include/asm-arm/irq.h   |    3 ++
 5 files changed, 126 insertions(+), 108 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 2035390..e7b1674 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -719,7 +719,7 @@ 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));
+        route_dt_irq_to_guest(d, &irq, dt_node_name(dev));
     }
 
     /* Map the address ranges */
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 36cf93d..82e0316 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -52,8 +52,6 @@ static struct {
     spinlock_t lock;
 } gic;
 
-static irq_desc_t irq_desc[NR_IRQS];
-static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc);
 static DEFINE_PER_CPU(uint64_t, lr_mask);
 
 static unsigned nr_lrs;
@@ -88,12 +86,6 @@ unsigned int gic_number_lines(void)
     return gic.lines;
 }
 
-irq_desc_t *__irq_to_desc(int irq)
-{
-    if (irq < NR_LOCAL_IRQS) return &this_cpu(local_irq_desc)[irq];
-    return &irq_desc[irq-NR_LOCAL_IRQS];
-}
-
 void gic_save_state(struct vcpu *v)
 {
     int i;
@@ -282,6 +274,27 @@ static int gic_route_irq(unsigned int irq, bool_t level,
     return 0;
 }
 
+/* Program the GIC to route an interrupt to a guest
+ *   - desc.lock must be held
+ */
+void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc,
+                            bool_t level, const cpumask_t *cpu_mask,
+                            unsigned int priority)
+{
+    struct pending_irq *p;
+    ASSERT(spin_is_locked(&desc->lock));
+
+    desc->handler = &gic_guest_irq_type;
+    desc->status |= IRQ_GUEST;
+
+    gic_set_irq_properties(desc->irq, level, cpumask_of(smp_processor_id()),
+                           GIC_PRI_IRQ);
+
+    /* TODO: do not assume delivery to vcpu0 */
+    p = irq_to_pending(d->vcpu[0], desc->irq);
+    p->desc = desc;
+}
+
 /* 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)
@@ -570,59 +583,6 @@ void gic_route_spis(void)
     }
 }
 
-void release_irq(unsigned int irq)
-{
-    struct irq_desc *desc;
-    unsigned long flags;
-   struct irqaction *action;
-
-    desc = irq_to_desc(irq);
-
-    desc->handler->shutdown(desc);
-
-    spin_lock_irqsave(&desc->lock,flags);
-    action = desc->action;
-    desc->action  = NULL;
-    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)
-        xfree(action);
-}
-
-static int __setup_irq(struct irq_desc *desc, struct irqaction *new)
-{
-    if ( desc->action != NULL )
-        return -EBUSY;
-
-    desc->action  = new;
-    dsb(sy);
-
-    return 0;
-}
-
-int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
-{
-    int rc;
-    unsigned long flags;
-    struct irq_desc *desc;
-
-    desc = irq_to_desc(irq->irq);
-
-    spin_lock_irqsave(&desc->lock, flags);
-    rc = __setup_irq(desc, new);
-    spin_unlock_irqrestore(&desc->lock, flags);
-
-    if ( !rc )
-        desc->handler->startup(desc);
-
-    return rc;
-}
-
 static inline void gic_set_lr(int lr, struct pending_irq *p,
         unsigned int state)
 {
@@ -761,49 +721,6 @@ void gic_inject(void)
         gic_inject_irq_start();
 }
 
-int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
-                           const char * devname)
-{
-    struct irqaction *action;
-    struct irq_desc *desc = irq_to_desc(irq->irq);
-    unsigned long flags;
-    int retval;
-    bool_t level;
-    struct pending_irq *p;
-
-    action = xmalloc(struct irqaction);
-    if (!action)
-        return -ENOMEM;
-
-    action->dev_id = d;
-    action->name = devname;
-    action->free_on_release = 1;
-
-    spin_lock_irqsave(&desc->lock, flags);
-
-    desc->handler = &gic_guest_irq_type;
-    desc->status |= IRQ_GUEST;
-
-    level = dt_irq_is_level_triggered(irq);
-
-    gic_set_irq_properties(irq->irq, level, cpumask_of(smp_processor_id()),
-                           GIC_PRI_IRQ);
-
-    retval = __setup_irq(desc, action);
-    if (retval) {
-        xfree(action);
-        goto out;
-    }
-
-    /* TODO: do not assume delivery to vcpu0 */
-    p = irq_to_pending(d->vcpu[0], irq->irq);
-    p->desc = desc;
-
-out:
-    spin_unlock_irqrestore(&desc->lock, flags);
-    return retval;
-}
-
 static void do_sgi(struct cpu_user_regs *regs, int othercpu, enum gic_sgi sgi)
 {
     /* Lower the priority */
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index b3bfebc..f3a30bd 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -44,6 +44,15 @@ hw_irq_controller no_irq_type = {
     .end = end_none
 };
 
+static irq_desc_t irq_desc[NR_IRQS];
+static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc);
+
+irq_desc_t *__irq_to_desc(int irq)
+{
+    if (irq < NR_LOCAL_IRQS) return &this_cpu(local_irq_desc)[irq];
+    return &irq_desc[irq-NR_LOCAL_IRQS];
+}
+
 int __init arch_init_one_irq_desc(struct irq_desc *desc)
 {
     return 0;
@@ -188,6 +197,94 @@ out_no_end:
     irq_exit();
 }
 
+void release_irq(unsigned int irq)
+{
+    struct irq_desc *desc;
+    unsigned long flags;
+   struct irqaction *action;
+
+    desc = irq_to_desc(irq);
+
+    desc->handler->shutdown(desc);
+
+    spin_lock_irqsave(&desc->lock,flags);
+    action = desc->action;
+    desc->action  = NULL;
+    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 )
+        xfree(action);
+}
+
+static int __setup_irq(struct irq_desc *desc, struct irqaction *new)
+{
+    if ( desc->action != NULL )
+        return -EBUSY;
+
+    desc->action  = new;
+    dsb(sy);
+
+    return 0;
+}
+
+int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
+{
+    int rc;
+    unsigned long flags;
+    struct irq_desc *desc;
+
+    desc = irq_to_desc(irq->irq);
+
+    spin_lock_irqsave(&desc->lock, flags);
+    rc = __setup_irq(desc, new);
+    spin_unlock_irqrestore(&desc->lock, flags);
+
+    if ( !rc )
+        desc->handler->startup(desc);
+
+    return rc;
+}
+
+int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
+                          const char * devname)
+{
+    struct irqaction *action;
+    struct irq_desc *desc = irq_to_desc(irq->irq);
+    unsigned long flags;
+    int retval;
+    bool_t level;
+
+    action = xmalloc(struct irqaction);
+    if (!action)
+        return -ENOMEM;
+
+    action->dev_id = d;
+    action->name = devname;
+    action->free_on_release = 1;
+
+    spin_lock_irqsave(&desc->lock, flags);
+
+    retval = __setup_irq(desc, action);
+    if ( retval )
+    {
+        xfree(action);
+        goto out;
+    }
+
+    level = dt_irq_is_level_triggered(irq);
+    gic_route_irq_to_guest(d, desc, level, cpumask_of(smp_processor_id()),
+                           GIC_PRI_IRQ);
+
+out:
+    spin_unlock_irqrestore(&desc->lock, flags);
+    return retval;
+}
+
 /*
  * pirq event channels. We don't use these on ARM, instead we use the
  * features of the GIC to inject virtualised normal interrupts.
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 071280b..0e6e325 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -153,6 +153,7 @@
 
 #ifndef __ASSEMBLY__
 #include <xen/device_tree.h>
+#include <xen/irq.h>
 
 #define DT_MATCH_GIC    DT_MATCH_COMPATIBLE("arm,cortex-a15-gic"), \
                         DT_MATCH_COMPATIBLE("arm,cortex-a7-gic")
@@ -170,6 +171,9 @@ extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
 extern void gic_route_dt_irq(const struct dt_irq *irq,
                              const cpumask_t *cpu_mask,
                              unsigned int priority);
+extern void gic_route_irq_to_guest(struct domain *, struct irq_desc *desc,
+                                   bool_t level, const cpumask_t *cpu_mask,
+                                   unsigned int priority);
 extern void gic_route_ppis(void);
 extern void gic_route_spis(void);
 
@@ -181,9 +185,6 @@ extern void __cpuinit init_maintenance_interrupt(void);
 extern void gic_set_guest_irq(struct vcpu *v, unsigned int irq,
         unsigned int state, unsigned int priority);
 extern void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq);
-extern int gic_route_irq_to_guest(struct domain *d,
-                                  const struct dt_irq *irq,
-                                  const char * devname);
 
 /* Accept an interrupt from the GIC and dispatch its handler */
 extern void gic_interrupt(struct cpu_user_regs *regs, int is_fiq);
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index 9380987..b52c26f 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -44,6 +44,9 @@ int request_dt_irq(const struct dt_irq *irq,
                    const char *devname, void *dev_id);
 int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new);
 
+int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
+                          const char *devname);
+
 #endif /* _ASM_HW_IRQ_H */
 /*
  * Local variables:
-- 
1.7.10.4

Re: [PATCH v2 07/16] xen/arm: IRQ: Move IRQ management from gic.c to irq.c

[Ian Campbell]

On Thu, 2014-04-03 at 21:41 +0100, Julien Grall wrote:
> The file gic.c contains functions and variables which is not related to the GIC:
>     - release_irq
>     - setup_irq
>     - gic_route_irq_to_guest
>     - {,local_}irq_desc
> 
> Move all theses functions/variables in irq.c and split gic_route_irq_to_guest
> in 2 parts:

Please can you separate pure code motion from refactoring.

e.g. You could do the split first leaving route_dt_irq_to_guest in gic.c
and then move it along with everything else.

>     - route_dt_irq_to_guest: setup the desc
>     - gic_route_irq_to_guest: setup correctly the GIC and the desc
>     handler
>  
> +void release_irq(unsigned int irq)
> +{
> +    struct irq_desc *desc;
> +    unsigned long flags;
> +   struct irqaction *action;

Indentation.

Re: [PATCH v2 07/16] xen/arm: IRQ: Move IRQ management from gic.c to irq.c

[Julien Grall]

On 04/07/2014 02:07 PM, Ian Campbell wrote:
> On Thu, 2014-04-03 at 21:41 +0100, Julien Grall wrote:
>> The file gic.c contains functions and variables which is not related to the GIC:
>>     - release_irq
>>     - setup_irq
>>     - gic_route_irq_to_guest
>>     - {,local_}irq_desc
>>
>> Move all theses functions/variables in irq.c and split gic_route_irq_to_guest
>> in 2 parts:
> 
> Please can you separate pure code motion from refactoring.
> 
> e.g. You could do the split first leaving route_dt_irq_to_guest in gic.c
> and then move it along with everything else.

Sure, as the refactoring was simple I didn't create a new patch.

>>     - route_dt_irq_to_guest: setup the desc
>>     - gic_route_irq_to_guest: setup correctly the GIC and the desc
>>     handler
>>  
>> +void release_irq(unsigned int irq)
>> +{
>> +    struct irq_desc *desc;
>> +    unsigned long flags;
>> +   struct irqaction *action;
> 
> Indentation.

I've blindly copied for the previous location. I think, I fix the
indention on a previous patch. I will fix it here.

Regards

-- 
Julien Grall

[PATCH v2 08/16] xen/arm: IRQ Introduce irq_get_domain

[Julien Grall]

This function retrieves a domain from an IRQ. It will be used in several
places (such as do_IRQ) to avoid duplicated code when multiple action will be
supported.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/irq.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index f3a30bd..5111b90 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -98,6 +98,15 @@ void __cpuinit init_secondary_IRQ(void)
     BUG_ON(init_local_irq_data() < 0);
 }
 
+static inline struct domain *irq_get_domain(struct irq_desc *desc)
+{
+    ASSERT(spin_is_locked(&desc->lock));
+    ASSERT(desc->status & IRQ_GUEST);
+    ASSERT(desc->action != NULL);
+
+    return desc->action->dev_id;
+}
+
 int request_dt_irq(const struct dt_irq *irq,
                    void (*handler)(int, void *, struct cpu_user_regs *),
                    const char *devname, void *dev_id)
@@ -156,7 +165,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
 
     if ( desc->status & IRQ_GUEST )
     {
-        struct domain *d = action->dev_id;
+        struct domain *d = irq_get_domain(desc);
 
         desc->handler->end(desc);
 
-- 
1.7.10.4

Re: [PATCH v2 08/16] xen/arm: IRQ Introduce irq_get_domain

[Ian Campbell]

On Thu, 2014-04-03 at 21:41 +0100, Julien Grall wrote:
> This function retrieves a domain from an IRQ. It will be used in several
> places (such as do_IRQ) to avoid duplicated code when multiple action will be
> supported.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/irq.c |   11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index f3a30bd..5111b90 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -98,6 +98,15 @@ void __cpuinit init_secondary_IRQ(void)
>      BUG_ON(init_local_irq_data() < 0);
>  }
>  
> +static inline struct domain *irq_get_domain(struct irq_desc *desc)
> +{
> +    ASSERT(spin_is_locked(&desc->lock));
> +    ASSERT(desc->status & IRQ_GUEST);

I don't know if this will be helpful for any of the forthcoming callers
but you could return DOMID_XEN if this isn't the case.

> +    ASSERT(desc->action != NULL);
> +
> +    return desc->action->dev_id;
> +}
> +
>  int request_dt_irq(const struct dt_irq *irq,
>                     void (*handler)(int, void *, struct cpu_user_regs *),
>                     const char *devname, void *dev_id)
> @@ -156,7 +165,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
>  
>      if ( desc->status & IRQ_GUEST )
>      {
> -        struct domain *d = action->dev_id;
> +        struct domain *d = irq_get_domain(desc);
>  
>          desc->handler->end(desc);
>  

Re: [PATCH v2 08/16] xen/arm: IRQ Introduce irq_get_domain

[Julien Grall]

On 04/07/2014 02:15 PM, Ian Campbell wrote:
> On Thu, 2014-04-03 at 21:41 +0100, Julien Grall wrote:
>> This function retrieves a domain from an IRQ. It will be used in several
>> places (such as do_IRQ) to avoid duplicated code when multiple action will be
>> supported.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
>> ---
>>     Changes in v2:
>>         - Patch added
>> ---
>>  xen/arch/arm/irq.c |   11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>> index f3a30bd..5111b90 100644
>> --- a/xen/arch/arm/irq.c
>> +++ b/xen/arch/arm/irq.c
>> @@ -98,6 +98,15 @@ void __cpuinit init_secondary_IRQ(void)
>>      BUG_ON(init_local_irq_data() < 0);
>>  }
>>  
>> +static inline struct domain *irq_get_domain(struct irq_desc *desc)
>> +{
>> +    ASSERT(spin_is_locked(&desc->lock));
>> +    ASSERT(desc->status & IRQ_GUEST);
> 
> I don't know if this will be helpful for any of the forthcoming callers
> but you could return DOMID_XEN if this isn't the case.

This function was created to retrieve easily the domain (mainly when we
will switch to a list for the action).
As it's only used withing the file and it should only be called when
desc->status == IRQ_GUEST, I don't think we need to return DOMID_XEN.

Regards,

-- 
Julien Grall

[PATCH v2 09/16] 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 as 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 not being enabled.

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

---
    Changes in v2:
        - Fix typoes in commit message
        - Move this patch earlier in the series => move shutdown() in
        release_irq and gic_route_irq
---
 xen/arch/arm/gic.c |   39 ++++++++++++++++++++++++---------------
 xen/arch/arm/irq.c |    6 ++++--
 2 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 82e0316..8c53e52 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -123,44 +123,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(sy);
     /* 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;
-    unsigned long flags;
 
-    spin_lock_irqsave(&desc->lock, flags);
+    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_irqrestore(&desc->lock, flags);
 }
 
-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)
@@ -261,11 +270,11 @@ static int gic_route_irq(unsigned int irq, bool_t level,
     if ( desc->action != NULL )
         return -EBUSY;
 
+    spin_lock_irqsave(&desc->lock, flags);
+
     /* Disable interrupt */
     desc->handler->shutdown(desc);
 
-    spin_lock_irqsave(&desc->lock, flags);
-
     desc->handler = &gic_host_irq_type;
 
     gic_set_irq_properties(irq, level, cpu_mask, priority);
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 5111b90..798353b 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -214,9 +214,10 @@ void release_irq(unsigned int irq)
 
     desc = irq_to_desc(irq);
 
+    spin_lock_irqsave(&desc->lock,flags);
+
     desc->handler->shutdown(desc);
 
-    spin_lock_irqsave(&desc->lock,flags);
     action = desc->action;
     desc->action  = NULL;
     desc->status &= ~IRQ_GUEST;
@@ -251,11 +252,12 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
 
     spin_lock_irqsave(&desc->lock, flags);
     rc = __setup_irq(desc, 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 v2 09/16] xen/arm: IRQ: Add lock contrainst for gic_irq_{startup, shutdown}

[Ian Campbell]

On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote:

In the subject "constraint".

A better title might be "require $FOO lock be held by callers of
gic_irq...blah"

> When multiple action will be supported, gic_irq_{startup,shutdown} will have

s/will be/are/

> to be called in the same critical zone as setup/release.

"critical section" is the more usual term I think. Or "under the same
lock as".

> Otherwise it could have a race condition if at the same time CPU A is calling

"Otherwise there is a race condition..."

> release_dt_irq and CPU B is calling setup_dt_irq.
> 
> This could end up to the IRQ not being enabled.

s/to/with/

> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v2:
>         - Fix typoes in commit message
>         - Move this patch earlier in the series => move shutdown() in
>         release_irq and gic_route_irq
> ---
>  xen/arch/arm/gic.c |   39 ++++++++++++++++++++++++---------------
>  xen/arch/arm/irq.c |    6 ++++--
>  2 files changed, 28 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 82e0316..8c53e52 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -123,44 +123,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)

Is there code motion mixed in with this locking change?

It looks a bit like the relationship between e.g. gic_irq_startup and
gic_irq_enable is being turned inside out? Maybe diff has just chosen an
unhelpful representation of a relatively simple change?

> -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)
> @@ -261,11 +270,11 @@ static int gic_route_irq(unsigned int irq, bool_t level,
>      if ( desc->action != NULL )
>          return -EBUSY;
>  
> +    spin_lock_irqsave(&desc->lock, flags);
> +
>      /* Disable interrupt */
>      desc->handler->shutdown(desc);
>  
> -    spin_lock_irqsave(&desc->lock, flags);

Since desc->handler is a generic construct I think it is worth
mentioning in the commit log that this is consistent with x86.

After this change are arm's locking requirements wrt the
hw_irq_controller callbacks now consistent with x86's?
 
Ian.

Re: [PATCH v2 09/16] xen/arm: IRQ: Add lock contrainst for gic_irq_{startup, shutdown}

[Julien Grall]

On 04/07/2014 02:27 PM, Ian Campbell wrote:
> On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote:
> 
> In the subject "constraint".
> 
> A better title might be "require $FOO lock be held by callers of
> gic_irq...blah"

I will change the title.

>> When multiple action will be supported, gic_irq_{startup,shutdown} will have
> 
> s/will be/are/
> 
>> to be called in the same critical zone as setup/release.
> 
> "critical section" is the more usual term I think. Or "under the same
> lock as".
> 
>> Otherwise it could have a race condition if at the same time CPU A is calling
> 
> "Otherwise there is a race condition..."
> 
>> release_dt_irq and CPU B is calling setup_dt_irq.
>>
>> This could end up to the IRQ not being enabled.
> 
> s/to/with/

I will fix all the typoes.

>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>     Changes in v2:
>>         - Fix typoes in commit message
>>         - Move this patch earlier in the series => move shutdown() in
>>         release_irq and gic_route_irq
>> ---
>>  xen/arch/arm/gic.c |   39 ++++++++++++++++++++++++---------------
>>  xen/arch/arm/irq.c |    6 ++++--
>>  2 files changed, 28 insertions(+), 17 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 82e0316..8c53e52 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -123,44 +123,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)
> 
> Is there code motion mixed in with this locking change?
> 
> It looks a bit like the relationship between e.g. gic_irq_startup and
> gic_irq_enable is being turned inside out? Maybe diff has just chosen an
> unhelpful representation of a relatively simple change?

[..]

> Since desc->handler is a generic construct I think it is worth
> mentioning in the commit log that this is consistent with x86.
>
> After this change are arm's locking requirements wrt the
> hw_irq_controller callbacks now consistent with x86's?


Before this patch gic_irq_startup was calling gic_irq_enable, now it's
flipped.

After thinking, I will rework this patch. It's also possible to take the
desc->lock outside for irq_enable and irq_startup. So we will be
consistent with x86's that AFAIU request desc->lock to be held by the
callers.

Regards,

-- 
Julien Grall

[PATCH v2 10/16] xen/arm: IRQ: Don't need to have a specific function to route IRQ to Xen

[Julien Grall]

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 steps are called on every cpu. For SPIs, they are only called
on the boot CPU.

Dividing the setup in two step complicates the code when a new driver is
added to Xen (for instance a SMMU driver). Xen can safely route the IRQ
when the driver sets up the interrupt handler.

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

---
    Changes in v2:
        - Fix typo in commit message
        - s/SGI/SPI/ in comments
        - Rename gic_route_dt_irq into gic_route_irq_to_xen which is
        taking a desc now
        - Call setup_irq before initializing the GIC IRQ as the first one
        can fail.
---
 xen/arch/arm/gic.c         |   63 +++++++-------------------------------------
 xen/arch/arm/irq.c         |   24 ++++++++++++++++-
 xen/arch/arm/setup.c       |    2 --
 xen/arch/arm/smpboot.c     |    2 --
 xen/arch/arm/time.c        |   11 --------
 xen/include/asm-arm/gic.h  |   10 +++----
 xen/include/asm-arm/time.h |    3 ---
 7 files changed, 36 insertions(+), 79 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 8c53e52..9127ecf 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -257,30 +257,20 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
     spin_unlock(&gic.lock);
 }
 
-/* Program the GIC to route an interrupt */
-static int gic_route_irq(unsigned int irq, bool_t level,
-                         const cpumask_t *cpu_mask, unsigned int priority)
+/* Program the GIC to route an interrupt to the host (eg Xen)
+ * - needs to be called with desc.lock held
+ */
+void gic_route_irq_to_xen(struct irq_desc *desc, 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;
-
-    spin_lock_irqsave(&desc->lock, flags);
-
-    /* Disable interrupt */
-    desc->handler->shutdown(desc);
+    ASSERT(desc->irq < gic.lines);/* Can't route interrupts that don't exist */
+    ASSERT(desc->status & IRQ_DISABLED);
+    ASSERT(spin_is_locked(&desc->lock));
 
     desc->handler = &gic_host_irq_type;
 
-    gic_set_irq_properties(irq, level, cpu_mask, priority);
-
-    spin_unlock_irqrestore(&desc->lock, flags);
-    return 0;
+    gic_set_irq_properties(desc->irq, level, cpu_mask, priority);
 }
 
 /* Program the GIC to route an interrupt to a guest
@@ -304,17 +294,6 @@ void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc,
     p->desc = desc;
 }
 
-/* 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;
@@ -568,30 +547,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()),
-                     GIC_PRI_IRQ);
-    /* 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()),
-                         GIC_PRI_IRQ);
-    }
-}
-
 static inline void gic_set_lr(int lr, struct pending_irq *p,
         unsigned int state)
 {
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 798353b..1262a9c 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -247,15 +247,37 @@ int 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);
 
     spin_lock_irqsave(&desc->lock, flags);
+
+    disabled = (desc->action == NULL);
+
     rc = __setup_irq(desc, new);
+    if ( rc )
+        goto err;
 
-    if ( !rc )
+    /* 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 SPI: we don't care for now which CPU will receive the
+         * interrupt
+         * TODO: Handle case where SPI is setup on different CPU than
+         * the targeted CPU and the priority.
+         */
+        gic_route_irq_to_xen(desc, level, cpumask_of(smp_processor_id()),
+                             GIC_PRI_IRQ);
         desc->handler->startup(desc);
+    }
 
+err:
     spin_unlock_irqrestore(&desc->lock, flags);
 
     return rc;
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 0892a54..7b02282 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -718,8 +718,6 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     init_IRQ();
 
-    gic_route_ppis();
-    gic_route_spis();
     xsm_dt_init();
 
     init_maintenance_interrupt();
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 7f28b68..cf149da 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 7cad888..ce96337 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()), GIC_PRI_IRQ);
-    gic_route_dt_irq(&timer_irq[TIMER_HYP_PPI],
-                     cpumask_of(smp_processor_id()), GIC_PRI_IRQ);
-    gic_route_dt_irq(&timer_irq[TIMER_VIRT_PPI],
-                     cpumask_of(smp_processor_id()), GIC_PRI_IRQ);
-}
-
 /* 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 0e6e325..b750b17 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -167,15 +167,13 @@ 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);
+/* Program the GIC to route an interrupt */
+extern void gic_route_irq_to_xen(struct irq_desc *desc, bool_t level,
+                                 const cpumask_t *cpu_mask,
+                                 unsigned int priority);
 extern void gic_route_irq_to_guest(struct domain *, struct irq_desc *desc,
                                    bool_t level, 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);
diff --git a/xen/include/asm-arm/time.h b/xen/include/asm-arm/time.h
index 9bbab0b..d544b5b 100644
--- a/xen/include/asm-arm/time.h
+++ b/xen/include/asm-arm/time.h
@@ -25,9 +25,6 @@ enum timer_ppi
 /* Get one of the timer IRQ number */
 unsigned int timer_get_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 v2 10/16] xen/arm: IRQ: Don't need to have a specific function to route IRQ to Xen

[Ian Campbell]

On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote:
> When the IRQ is handling by Xen, the setup is done in 2 steps:

"an IRQ is handled" (and perhaps s/, the//)

$subject is an odd way to describe the change too (it's more like the
motivation). Something like "defer routing IRQ to Xen until setup_irq()
call" perhaps?

>     - Route the IRQ to the current CPU and set priorities
>     - Set up the handler
> 
> For PPIs, these steps are called on every cpu. For SPIs, they are only called
> on the boot CPU.
> 
> Dividing the setup in two step complicates the code when a new driver is
> added to Xen (for instance a SMMU driver). Xen can safely route the IRQ
> when the driver sets up the interrupt handler.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v2:
>         - Fix typo in commit message
>         - s/SGI/SPI/ in comments
>         - Rename gic_route_dt_irq into gic_route_irq_to_xen which is
>         taking a desc now
>         - Call setup_irq before initializing the GIC IRQ as the first one
>         can fail.
> ---
>  xen/arch/arm/gic.c         |   63 +++++++-------------------------------------
>  xen/arch/arm/irq.c         |   24 ++++++++++++++++-
>  xen/arch/arm/setup.c       |    2 --
>  xen/arch/arm/smpboot.c     |    2 --
>  xen/arch/arm/time.c        |   11 --------
>  xen/include/asm-arm/gic.h  |   10 +++----
>  xen/include/asm-arm/time.h |    3 ---
>  7 files changed, 36 insertions(+), 79 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 8c53e52..9127ecf 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -257,30 +257,20 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
>      spin_unlock(&gic.lock);
>  }
>  
> -/* Program the GIC to route an interrupt */
> -static int gic_route_irq(unsigned int irq, bool_t level,
> -                         const cpumask_t *cpu_mask, unsigned int priority)
> +/* Program the GIC to route an interrupt to the host (eg Xen)

You mean i.e. not e.g.

> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 798353b..1262a9c 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -247,15 +247,37 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>      int rc;
>      unsigned long flags;
>      struct irq_desc *desc;
> +    bool_t disabled = 0;

No need to init, it's unconditionally assigned below. But if you do want
to keep it then I think boot_t wants to go with false even if that is
the same as 0 in the end.

>      desc = irq_to_desc(irq->irq);
>  
>      spin_lock_irqsave(&desc->lock, flags);
> +
> +    disabled = (desc->action == NULL);
> +
>      rc = __setup_irq(desc, new);
> +    if ( rc )
> +        goto err;
>  
> -    if ( !rc )
> +    /* First time the IRQ is setup */
> +    if ( disabled )

There's no way we can get back into this state. Perhaps with calls to
release_irq?

> +    {
> +        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 SPI: we don't care for now which CPU will receive the
> +         * interrupt

setup_dt_irq expected to be called multiple times for a PPI and the desc
is not shared, so that's how they get setup as well, right?

> +         * TODO: Handle case where SPI is setup on different CPU than
> +         * the targeted CPU and the priority.
> +         */
> +        gic_route_irq_to_xen(desc, level, cpumask_of(smp_processor_id()),
> +                             GIC_PRI_IRQ);
>          desc->handler->startup(desc);
> +    }
>  
> +err:
>      spin_unlock_irqrestore(&desc->lock, flags);
>  
>      return rc;

Ian.

Re: [PATCH v2 10/16] xen/arm: IRQ: Don't need to have a specific function to route IRQ to Xen

[Julien Grall]

On 04/07/2014 02:53 PM, Ian Campbell wrote:
> On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote:
>> When the IRQ is handling by Xen, the setup is done in 2 steps:
> 
> "an IRQ is handled" (and perhaps s/, the//)

Will fix it.

> $subject is an odd way to describe the change too (it's more like the
> motivation). Something like "defer routing IRQ to Xen until setup_irq()
> call" perhaps?

Sounds better. I will change the commit title.

> 
>>     - Route the IRQ to the current CPU and set priorities
>>     - Set up the handler
>>
>> For PPIs, these steps are called on every cpu. For SPIs, they are only called
>> on the boot CPU.
>>
>> Dividing the setup in two step complicates the code when a new driver is
>> added to Xen (for instance a SMMU driver). Xen can safely route the IRQ
>> when the driver sets up the interrupt handler.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>     Changes in v2:
>>         - Fix typo in commit message
>>         - s/SGI/SPI/ in comments
>>         - Rename gic_route_dt_irq into gic_route_irq_to_xen which is
>>         taking a desc now
>>         - Call setup_irq before initializing the GIC IRQ as the first one
>>         can fail.
>> ---
>>  xen/arch/arm/gic.c         |   63 +++++++-------------------------------------
>>  xen/arch/arm/irq.c         |   24 ++++++++++++++++-
>>  xen/arch/arm/setup.c       |    2 --
>>  xen/arch/arm/smpboot.c     |    2 --
>>  xen/arch/arm/time.c        |   11 --------
>>  xen/include/asm-arm/gic.h  |   10 +++----
>>  xen/include/asm-arm/time.h |    3 ---
>>  7 files changed, 36 insertions(+), 79 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 8c53e52..9127ecf 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -257,30 +257,20 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
>>      spin_unlock(&gic.lock);
>>  }
>>  
>> -/* Program the GIC to route an interrupt */
>> -static int gic_route_irq(unsigned int irq, bool_t level,
>> -                         const cpumask_t *cpu_mask, unsigned int priority)
>> +/* Program the GIC to route an interrupt to the host (eg Xen)
> 
> You mean i.e. not e.g.

Will fix it.

>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>> index 798353b..1262a9c 100644
>> --- a/xen/arch/arm/irq.c
>> +++ b/xen/arch/arm/irq.c
>> @@ -247,15 +247,37 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>>      int rc;
>>      unsigned long flags;
>>      struct irq_desc *desc;
>> +    bool_t disabled = 0;
> 
> No need to init, it's unconditionally assigned below. But if you do want
> to keep it then I think boot_t wants to go with false even if that is
> the same as 0 in the end.

Ok.

>>      desc = irq_to_desc(irq->irq);
>>  
>>      spin_lock_irqsave(&desc->lock, flags);
>> +
>> +    disabled = (desc->action == NULL);
>> +
>>      rc = __setup_irq(desc, new);
>> +    if ( rc )
>> +        goto err;
>>  
>> -    if ( !rc )
>> +    /* First time the IRQ is setup */
>> +    if ( disabled )
> 
> There's no way we can get back into this state. Perhaps with calls to
> release_irq?

release_irq will disable the interrupt if all the actions are removed.

> 
>> +    {
>> +        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 SPI: we don't care for now which CPU will receive the
>> +         * interrupt
> 
> setup_dt_irq expected to be called multiple times for a PPI and the desc
> is not shared, so that's how they get setup as well, right?

Yes, this setup_dt_irq should be called on the right processor when the
IRQ is a PPI.

Regards,

-- 
Julien Grall

[PATCH v2 11/16] 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 route the IRQ 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>

---
    Changes in v2:
        - Use EBUSY instead of EADDRINUSE for error code
        - Don't hardcode the domain in error message
        - Fix typo in error message
        - Use irq_get_domain
---
 xen/arch/arm/domain_build.c |    9 +++++++--
 xen/arch/arm/irq.c          |   34 +++++++++++++++++++++++++++++++++-
 2 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index e7b1674..f85e5a9 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -718,8 +718,13 @@ 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 */
-        route_dt_irq_to_guest(d, &irq, dt_node_name(dev));
+        res = route_dt_irq_to_guest(d, &irq, dt_node_name(dev));
+        if ( res )
+        {
+            printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n",
+                   irq.irq, d->domain_id);
+            return res;
+        }
     }
 
     /* Map the address ranges */
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 1262a9c..2bf6618 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -253,6 +253,16 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
 
     spin_lock_irqsave(&desc->lock, flags);
 
+    if ( desc->status & IRQ_GUEST )
+    {
+        struct domain *d = irq_get_domain(desc);
+
+        spin_unlock_irqrestore(&desc->lock, flags);
+        printk(XENLOG_ERR "ERROR: IRQ %u is already in use by the domain %u\n",
+               irq->irq, d->domain_id);
+        return -EBUSY;
+    }
+
     disabled = (desc->action == NULL);
 
     rc = __setup_irq(desc, new);
@@ -289,7 +299,7 @@ int route_dt_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;
 
     action = xmalloc(struct irqaction);
@@ -302,6 +312,28 @@ int route_dt_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 )
+    {
+        struct domain *ad = irq_get_domain(desc);
+
+        if ( (desc->status & IRQ_GUEST) && d == ad )
+            goto out;
+
+        if ( desc->status & IRQ_GUEST )
+            printk(XENLOG_ERR "ERROR: IRQ %u is already used by domain %u\n",
+                   irq->irq, ad->domain_id);
+        else
+            printk(XENLOG_ERR "ERROR: IRQ %u is already used by Xen\n",
+                   irq->irq);
+        retval = -EBUSY;
+        goto out;
+    }
+
     retval = __setup_irq(desc, action);
     if ( retval )
     {
-- 
1.7.10.4

Re: [PATCH v2 11/16] xen/arm: IRQ: Protect IRQ to be shared between domains and XEN

[Ian Campbell]

On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote:

$subject "do not allow IRQs to be shared between..."

> The current dt_route_irq_to_guest implementation set IRQ_GUEST no matter if the

s/set/sets/; s/no matter if/even if/

> IRQ is correctly setup.

"is already correctly setup" (I think that's what you meant)

> As IRQ can be shared between devices, if the devices are not assigned to the

s/As/An/ ?

> same domain or Xen, this could result to route the IRQ to the domain instead of

"then this could result in routing the IRQ..."

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

"Also avoid relying on the wrong behaviour ..." perhaps? If that not
then I'm not sure what this is referring to. Apart from that doubt the
code looks ok to me.

> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v2:
>         - Use EBUSY instead of EADDRINUSE for error code
>         - Don't hardcode the domain in error message
>         - Fix typo in error message
>         - Use irq_get_domain
> ---
>  xen/arch/arm/domain_build.c |    9 +++++++--
>  xen/arch/arm/irq.c          |   34 +++++++++++++++++++++++++++++++++-
>  2 files changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index e7b1674..f85e5a9 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -718,8 +718,13 @@ 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 */
> -        route_dt_irq_to_guest(d, &irq, dt_node_name(dev));
> +        res = route_dt_irq_to_guest(d, &irq, dt_node_name(dev));
> +        if ( res )
> +        {
> +            printk(XENLOG_ERR "Unable to route IRQ %u to domain %u\n",
> +                   irq.irq, d->domain_id);
> +            return res;
> +        }
>      }
>  
>      /* Map the address ranges */
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 1262a9c..2bf6618 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -253,6 +253,16 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>  
>      spin_lock_irqsave(&desc->lock, flags);
>  
> +    if ( desc->status & IRQ_GUEST )
> +    {
> +        struct domain *d = irq_get_domain(desc);
> +
> +        spin_unlock_irqrestore(&desc->lock, flags);
> +        printk(XENLOG_ERR "ERROR: IRQ %u is already in use by the domain %u\n",
> +               irq->irq, d->domain_id);
> +        return -EBUSY;
> +    }
> +
>      disabled = (desc->action == NULL);
>  
>      rc = __setup_irq(desc, new);
> @@ -289,7 +299,7 @@ int route_dt_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;
>  
>      action = xmalloc(struct irqaction);
> @@ -302,6 +312,28 @@ int route_dt_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 )
> +    {
> +        struct domain *ad = irq_get_domain(desc);
> +
> +        if ( (desc->status & IRQ_GUEST) && d == ad )
> +            goto out;
> +
> +        if ( desc->status & IRQ_GUEST )
> +            printk(XENLOG_ERR "ERROR: IRQ %u is already used by domain %u\n",
> +                   irq->irq, ad->domain_id);
> +        else
> +            printk(XENLOG_ERR "ERROR: IRQ %u is already used by Xen\n",
> +                   irq->irq);
> +        retval = -EBUSY;
> +        goto out;
> +    }
> +
>      retval = __setup_irq(desc, action);
>      if ( retval )
>      {

Re: [PATCH v2 11/16] xen/arm: IRQ: Protect IRQ to be shared between domains and XEN

[Julien Grall]

On 04/07/2014 03:46 PM, Ian Campbell wrote:
> On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote:
> 
> $subject "do not allow IRQs to be shared between..."

I will change the commit title.

>> The current dt_route_irq_to_guest implementation set IRQ_GUEST no matter if the
> 
> s/set/sets/; s/no matter if/even if/
> 
>> IRQ is correctly setup.
> 
> "is already correctly setup" (I think that's what you meant)
> 
>> As IRQ can be shared between devices, if the devices are not assigned to the
> 
> s/As/An/ ?

I meant "As an ...". But "An" sounds better with the change below.

>> same domain or Xen, this could result to route the IRQ to the domain instead of
> 
> "then this could result in routing the IRQ..."



>> Xen ...
>>
>> Also avoid to rely on wrong behaviour when Xen is routing an IRQ to DOM0.
> 
> "Also avoid relying on the wrong behaviour ..." perhaps? If that not
> then I'm not sure what this is referring to. Apart from that doubt the
> code looks ok to me.

Currently Xen doesn't check the return of routing the IRQ into DOM0. So,
if it fails we silently ignore the error.

Regards,

-- 
Julien Grall

Re: [PATCH v2 11/16] xen/arm: IRQ: Protect IRQ to be shared between domains and XEN

[Ian Campbell]

On Mon, 2014-04-07 at 15:53 +0100, Julien Grall wrote:
> >> Xen ...
> >>
> >> Also avoid to rely on wrong behaviour when Xen is routing an IRQ to DOM0.
> > 
> > "Also avoid relying on the wrong behaviour ..." perhaps? If that not
> > then I'm not sure what this is referring to. Apart from that doubt the
> > code looks ok to me.
> 
> Currently Xen doesn't check the return of routing the IRQ into DOM0. So,
> if it fails we silently ignore the error.

OK, that's what I thought you were talking about then. You might want
append to the existing sentence: "Therefore check the return code from
XXX in YYYY." to make it clear what it means.

Ian.

Re: [PATCH v2 11/16] xen/arm: IRQ: Protect IRQ to be shared between domains and XEN

[Julien Grall]

On 04/07/2014 04:12 PM, Ian Campbell wrote:
> On Mon, 2014-04-07 at 15:53 +0100, Julien Grall wrote:
>>>> Xen ...
>>>>
>>>> Also avoid to rely on wrong behaviour when Xen is routing an IRQ to DOM0.
>>>
>>> "Also avoid relying on the wrong behaviour ..." perhaps? If that not
>>> then I'm not sure what this is referring to. Apart from that doubt the
>>> code looks ok to me.
>>
>> Currently Xen doesn't check the return of routing the IRQ into DOM0. So,
>> if it fails we silently ignore the error.
> 
> OK, that's what I thought you were talking about then. You might want
> append to the existing sentence: "Therefore check the return code from
> XXX in YYYY." to make it clear what it means.

I will update the commit message.

Regards,

-- 
Julien Grall

[PATCH v2 12/16] xen/serial: remove serial_dt_irq

[Julien Grall]

This function was only used for ARM 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>

---
    Changes in v2:
        - Specify in the commit this patch is ARM specific
---
 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 d49e1fe..370539c 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 44e13b7..cc86921 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -717,14 +717,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)
 {
@@ -744,9 +736,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 49ae1a4..b8da509 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 90bf0c6..459e686 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 v2 12/16] xen/serial: remove serial_dt_irq

[Ian Campbell]

On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote:
> This function was only used for ARM 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>

[PATCH v2 13/16] xen/arm: IRQ: Store IRQ type in arch_irq_desc

[Julien Grall]

For now, ARM uses different IRQ functions to setup an interrupt handler. This
is a bit annoying for common driver because we have to add idefery when
an IRQ is setup (see ns16550_init_postirq for an example).

To avoid to completely fork the IRQ management code, we can introduce a field
to store the IRQ type (e.g level/edge ...).

This patch also adds platform_get_irq which will retrieve the IRQ from the
device tree and setup correctly the IRQ type.

In order to use this solution, we have to move init_IRQ earlier for the boot
CPU. It's fine because the code only depends on percpu.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/gic.c        |   21 +++++++-----
 xen/arch/arm/irq.c        |   80 ++++++++++++++++++++++++++++++++++++++++-----
 xen/arch/arm/setup.c      |    3 +-
 xen/include/asm-arm/gic.h |    5 ++-
 xen/include/asm-arm/irq.h |    3 ++
 5 files changed, 91 insertions(+), 21 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 9127ecf..ec2994e 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -223,15 +223,20 @@ static hw_irq_controller gic_guest_irq_type = {
 
 /*
  * - needs to be called with a valid cpu_mask, ie each cpu in the mask has
+ * - desc.lock must be held
  * already called gic_cpu_init
  */
-static void gic_set_irq_properties(unsigned int irq, bool_t level,
+static void gic_set_irq_properties(struct irq_desc *desc,
                                    const cpumask_t *cpu_mask,
                                    unsigned int priority)
 {
     volatile unsigned char *bytereg;
     uint32_t cfg, edgebit;
     unsigned int mask;
+    unsigned int irq = desc->irq;
+    unsigned int type = desc->arch.type;
+
+    ASSERT(spin_is_locked(&desc->lock));
 
     spin_lock(&gic.lock);
 
@@ -240,7 +245,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
     /* Set edge / level */
     cfg = GICD[GICD_ICFGR + irq / 16];
     edgebit = 2u << (2 * (irq % 16));
-    if ( level )
+    if ( (type & DT_IRQ_TYPE_LEVEL_MASK) || (type == DT_IRQ_TYPE_NONE) )
         cfg &= ~edgebit;
     else
         cfg |= edgebit;
@@ -260,8 +265,8 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
 /* Program the GIC to route an interrupt to the host (eg Xen)
  * - needs to be called with desc.lock held
  */
-void gic_route_irq_to_xen(struct irq_desc *desc, bool_t level,
-                          const cpumask_t *cpu_mask, unsigned int priority)
+void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
+                          unsigned int priority)
 {
     ASSERT(priority <= 0xff);     /* Only 8 bits of priority */
     ASSERT(desc->irq < gic.lines);/* Can't route interrupts that don't exist */
@@ -270,15 +275,14 @@ void gic_route_irq_to_xen(struct irq_desc *desc, bool_t level,
 
     desc->handler = &gic_host_irq_type;
 
-    gic_set_irq_properties(desc->irq, level, cpu_mask, priority);
+    gic_set_irq_properties(desc, cpu_mask, priority);
 }
 
 /* Program the GIC to route an interrupt to a guest
  *   - desc.lock must be held
  */
 void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc,
-                            bool_t level, const cpumask_t *cpu_mask,
-                            unsigned int priority)
+                            const cpumask_t *cpu_mask, unsigned int priority)
 {
     struct pending_irq *p;
     ASSERT(spin_is_locked(&desc->lock));
@@ -286,8 +290,7 @@ void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc,
     desc->handler = &gic_guest_irq_type;
     desc->status |= IRQ_GUEST;
 
-    gic_set_irq_properties(desc->irq, level, cpumask_of(smp_processor_id()),
-                           GIC_PRI_IRQ);
+    gic_set_irq_properties(desc, cpumask_of(smp_processor_id()), GIC_PRI_IRQ);
 
     /* TODO: do not assume delivery to vcpu0 */
     p = irq_to_pending(d->vcpu[0], desc->irq);
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 2bf6618..a56ccf2 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -55,6 +55,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;
     return 0;
 }
 
@@ -82,6 +83,12 @@ static int __cpuinit init_local_irq_data(void)
         init_one_irq_desc(desc);
         desc->irq = irq;
         desc->action  = NULL;
+
+        /* PPIs are include in local_irqs, we have to copy the IRQ type from
+         * CPU0 otherwise we may miss the type if the IRQ type has been
+         * set early.
+         */
+        desc->arch.type = per_cpu(local_irq_desc, 0)[irq].arch.type;
     }
 
     return 0;
@@ -272,9 +279,6 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
     /* 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 SPI: we don't care for now which CPU will receive the
@@ -282,7 +286,8 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
          * TODO: Handle case where SPI is setup on different CPU than
          * the targeted CPU and the priority.
          */
-        gic_route_irq_to_xen(desc, level, cpumask_of(smp_processor_id()),
+        desc->arch.type = irq->type;
+        gic_route_irq_to_xen(desc, cpumask_of(smp_processor_id()),
                              GIC_PRI_IRQ);
         desc->handler->startup(desc);
     }
@@ -300,7 +305,6 @@ int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
     struct irq_desc *desc = irq_to_desc(irq->irq);
     unsigned long flags;
     int retval = 0;
-    bool_t level;
 
     action = xmalloc(struct irqaction);
     if (!action)
@@ -341,10 +345,9 @@ int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
         goto out;
     }
 
-    level = dt_irq_is_level_triggered(irq);
-    gic_route_irq_to_guest(d, desc, level, cpumask_of(smp_processor_id()),
+    desc->arch.type = irq->type;
+    gic_route_irq_to_guest(d, desc, cpumask_of(smp_processor_id()),
                            GIC_PRI_IRQ);
-
 out:
     spin_unlock_irqrestore(&desc->lock, flags);
     return retval;
@@ -379,6 +382,67 @@ void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask)
     BUG();
 }
 
+static inline int irq_set_type(struct irq_desc *desc, unsigned int type)
+{
+    unsigned int flags;
+    int ret = -EBUSY;
+
+    if ( type == DT_IRQ_TYPE_NONE )
+        return 0;
+
+    spin_lock_irqsave(&desc->lock, flags);
+
+    if ( desc->arch.type != DT_IRQ_TYPE_NONE && desc->arch.type != type )
+        goto err;
+
+    desc->arch.type = type;
+
+    ret = 0;
+
+err:
+    spin_unlock_irqrestore(&desc->lock, flags);
+    return ret;
+}
+
+unsigned int platform_get_irq(const struct dt_device_node *device,
+                              int index)
+{
+    struct dt_irq dt_irq;
+    struct irq_desc *desc;
+    unsigned int type, irq;
+    int res;
+
+    res = dt_device_get_irq(device, index, &dt_irq);
+    if ( res )
+        return 0;
+
+    irq = dt_irq.irq;
+    type = dt_irq.type;
+
+    /* Setup the IRQ type */
+
+    if ( irq < NR_LOCAL_IRQS )
+    {
+        unsigned int cpu;
+        /* For PPIs, we need to set IRQ type on every online CPUs */
+        for_each_cpu( cpu, &cpu_online_map )
+        {
+            desc = &per_cpu(local_irq_desc, cpu)[irq];
+            res = irq_set_type(desc, type);
+            if ( res )
+                return 0;
+        }
+    }
+    else
+    {
+        res = irq_set_type(irq_to_desc(irq), type);
+        if ( res )
+            return 0;
+    }
+
+    return irq;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 7b02282..b755964 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -687,6 +687,8 @@ void __init start_xen(unsigned long boot_phys_offset,
     dt_unflatten_host_device_tree();
     dt_irq_xlate = gic_irq_xlate;
 
+    init_IRQ();
+
     dt_uart_init();
     console_init_preirq();
 
@@ -716,7 +718,6 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     tasklet_subsys_init();
 
-    init_IRQ();
 
     xsm_dt_init();
 
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index b750b17..80f8dd2 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -168,11 +168,10 @@ 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 */
-extern void gic_route_irq_to_xen(struct irq_desc *desc, bool_t level,
-                                 const cpumask_t *cpu_mask,
+extern void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
                                  unsigned int priority);
 extern void gic_route_irq_to_guest(struct domain *, struct irq_desc *desc,
-                                   bool_t level, const cpumask_t *cpu_mask,
+                                   const cpumask_t *cpu_mask,
                                    unsigned int priority);
 
 extern void gic_inject(void);
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index b52c26f..107c13a 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -16,6 +16,7 @@ struct arch_pirq
 
 struct arch_irq_desc {
     int eoi_cpu;
+    unsigned int type;
 };
 
 #define NR_LOCAL_IRQS	32
@@ -46,6 +47,8 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new);
 
 int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
                           const char *devname);
+unsigned int platform_get_irq(const struct dt_device_node *device,
+                              int index);
 
 #endif /* _ASM_HW_IRQ_H */
 /*
-- 
1.7.10.4

Re: [PATCH v2 13/16] xen/arm: IRQ: Store IRQ type in arch_irq_desc

[Ian Campbell]

On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote:
> For now, ARM uses different IRQ functions to setup an interrupt handler. This
> is a bit annoying for common driver because we have to add idefery when
> an IRQ is setup (see ns16550_init_postirq for an example).
> 
> To avoid to completely fork the IRQ management code, we can introduce a field
> to store the IRQ type (e.g level/edge ...).
> 
> This patch also adds platform_get_irq which will retrieve the IRQ from the
> device tree and setup correctly the IRQ type.
> 
> In order to use this solution, we have to move init_IRQ earlier for the boot
> CPU. It's fine because the code only depends on percpu.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/gic.c        |   21 +++++++-----
>  xen/arch/arm/irq.c        |   80 ++++++++++++++++++++++++++++++++++++++++-----
>  xen/arch/arm/setup.c      |    3 +-
>  xen/include/asm-arm/gic.h |    5 ++-
>  xen/include/asm-arm/irq.h |    3 ++
>  5 files changed, 91 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 9127ecf..ec2994e 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -223,15 +223,20 @@ static hw_irq_controller gic_guest_irq_type = {
>  
>  /*
>   * - needs to be called with a valid cpu_mask, ie each cpu in the mask has
> + * - desc.lock must be held
>   * already called gic_cpu_init

I think you've injected that line in the middle of a sentence ;-)

>   */
> -static void gic_set_irq_properties(unsigned int irq, bool_t level,
> +static void gic_set_irq_properties(struct irq_desc *desc,
>                                     const cpumask_t *cpu_mask,
>                                     unsigned int priority)
>  {
>      volatile unsigned char *bytereg;
>      uint32_t cfg, edgebit;
>      unsigned int mask;
> +    unsigned int irq = desc->irq;
> +    unsigned int type = desc->arch.type;
> +
> +    ASSERT(spin_is_locked(&desc->lock));
>  
>      spin_lock(&gic.lock);
>  
> @@ -240,7 +245,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
>      /* Set edge / level */
>      cfg = GICD[GICD_ICFGR + irq / 16];
>      edgebit = 2u << (2 * (irq % 16));
> -    if ( level )
> +    if ( (type & DT_IRQ_TYPE_LEVEL_MASK) || (type == DT_IRQ_TYPE_NONE) )

Is getting DT_IRQ_TYPE_NONE here not an error?

Oh, I see this is the innards of dt_irq_is_level_triggered. Could that
be refactored e.g. into dt_irq_type_is_level_triggered(const something
type)?

>          cfg &= ~edgebit;
>      else
>          cfg |= edgebit;

> @@ -82,6 +83,12 @@ static int __cpuinit init_local_irq_data(void)
>          init_one_irq_desc(desc);
>          desc->irq = irq;
>          desc->action  = NULL;
> +
> +        /* PPIs are include in local_irqs, we have to copy the IRQ type from
> +         * CPU0 otherwise we may miss the type if the IRQ type has been
> +         * set early.
> +         */
> +        desc->arch.type = per_cpu(local_irq_desc, 0)[irq].arch.type;

I thought we had a boot_cpu(foo) accessor, but I guess not (or at least
I can't find it right now).

That might be nicer to add than adding a hardcoded 0 (I suppose it isn't
the only one though).

> @@ -379,6 +382,67 @@ void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask)
>      BUG();
>  }
>  
> +static inline int irq_set_type(struct irq_desc *desc, unsigned int type)
> +{
> +    unsigned int flags;
> +    int ret = -EBUSY;
> +
> +    if ( type == DT_IRQ_TYPE_NONE )
> +        return 0;
> +
> +    spin_lock_irqsave(&desc->lock, flags);
> +
> +    if ( desc->arch.type != DT_IRQ_TYPE_NONE && desc->arch.type != type )
> +        goto err;
> +
> +    desc->arch.type = type;

There was an open coded assignment in the guest path which unfortunately
I already trimmed. Shouldn't that have all these checks too?

> +
> +    ret = 0;
> +
> +err:
> +    spin_unlock_irqrestore(&desc->lock, flags);
> +    return ret;
> +}
> +
> +unsigned int platform_get_irq(const struct dt_device_node *device,
> +                              int index)
> +{
> +    struct dt_irq dt_irq;
> +    struct irq_desc *desc;
> +    unsigned int type, irq;
> +    int res;
> +
> +    res = dt_device_get_irq(device, index, &dt_irq);
> +    if ( res )
> +        return 0;

Not an error? Do we take precautions against IRQ0 being actually used
somewhere?

We should have an explicit #define for an invalid IRQ number.

> +    irq = dt_irq.irq;
> +    type = dt_irq.type;
> +
> +    /* Setup the IRQ type */
> +
> +    if ( irq < NR_LOCAL_IRQS )
> +    {
> +        unsigned int cpu;
> +        /* For PPIs, we need to set IRQ type on every online CPUs */
> +        for_each_cpu( cpu, &cpu_online_map )
> +        {
> +            desc = &per_cpu(local_irq_desc, cpu)[irq];
> +            res = irq_set_type(desc, type);
> +            if ( res )
> +                return 0;

Error?

Also no need to undo any partial work?

I haven't seen the caller yet, but for PPIs do we not get called for
each CPU as it binds to the PPI anyway?

> +        }
> +    }
> +    else
> +    {
> +        res = irq_set_type(irq_to_desc(irq), type);
> +        if ( res )
> +            return 0;
> +    }
> +
> +    return irq;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C

> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index b52c26f..107c13a 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -16,6 +16,7 @@ struct arch_pirq
>  
>  struct arch_irq_desc {
>      int eoi_cpu;
> +    unsigned int type;

I was wondering through the above if this ought to be a "bool_t level"
or not. Thoughts?

>  };
>  
>  #define NR_LOCAL_IRQS	32
> @@ -46,6 +47,8 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new);
>  
>  int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
>                            const char *devname);
> +unsigned int platform_get_irq(const struct dt_device_node *device,
> +                              int index);
>  
>  #endif /* _ASM_HW_IRQ_H */
>  /*

Re: [PATCH v2 13/16] xen/arm: IRQ: Store IRQ type in arch_irq_desc

[Julien Grall]

On 04/07/2014 04:03 PM, Ian Campbell wrote:
>>  /*
>>   * - needs to be called with a valid cpu_mask, ie each cpu in the mask has
>> + * - desc.lock must be held
>>   * already called gic_cpu_init
> 
> I think you've injected that line in the middle of a sentence ;-)

Oops right.

>>   */
>> -static void gic_set_irq_properties(unsigned int irq, bool_t level,
>> +static void gic_set_irq_properties(struct irq_desc *desc,
>>                                     const cpumask_t *cpu_mask,
>>                                     unsigned int priority)
>>  {
>>      volatile unsigned char *bytereg;
>>      uint32_t cfg, edgebit;
>>      unsigned int mask;
>> +    unsigned int irq = desc->irq;
>> +    unsigned int type = desc->arch.type;
>> +
>> +    ASSERT(spin_is_locked(&desc->lock));
>>  
>>      spin_lock(&gic.lock);
>>  
>> @@ -240,7 +245,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
>>      /* Set edge / level */
>>      cfg = GICD[GICD_ICFGR + irq / 16];
>>      edgebit = 2u << (2 * (irq % 16));
>> -    if ( level )
>> +    if ( (type & DT_IRQ_TYPE_LEVEL_MASK) || (type == DT_IRQ_TYPE_NONE) )
> 
> Is getting DT_IRQ_TYPE_NONE here not an error?

No, there is some DT like the exynos one which is using 0 (i.e
DT_IRQ_TYPE_NONE) for the IRQ type.

I guess we have to skip setting level/edge property in this case.

> Oh, I see this is the innards of dt_irq_is_level_triggered. Could that
> be refactored e.g. into dt_irq_type_is_level_triggered(const something
> type)?

I was wondering something like that instead:

if ( (type & DT_IRQ_TYPE_LEVEL_MASK) )
...
else if ( (type & DT_IRQ_TYPE_EDGE_BOTH) )
...

So we skip the DT_IRQ_TYPE_NONE.

> 
>>          cfg &= ~edgebit;
>>      else
>>          cfg |= edgebit;
> 
>> @@ -82,6 +83,12 @@ static int __cpuinit init_local_irq_data(void)
>>          init_one_irq_desc(desc);
>>          desc->irq = irq;
>>          desc->action  = NULL;
>> +
>> +        /* PPIs are include in local_irqs, we have to copy the IRQ type from
>> +         * CPU0 otherwise we may miss the type if the IRQ type has been
>> +         * set early.
>> +         */
>> +        desc->arch.type = per_cpu(local_irq_desc, 0)[irq].arch.type;
> 
> I thought we had a boot_cpu(foo) accessor, but I guess not (or at least
> I can't find it right now).
> 
> That might be nicer to add than adding a hardcoded 0 (I suppose it isn't
> the only one though).

It's also used in mm.c when page tables is setup.

I will introduce boot_cpu macro.

>> @@ -379,6 +382,67 @@ void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask)
>>      BUG();
>>  }
>>  
>> +static inline int irq_set_type(struct irq_desc *desc, unsigned int type)
>> +{
>> +    unsigned int flags;
>> +    int ret = -EBUSY;
>> +
>> +    if ( type == DT_IRQ_TYPE_NONE )
>> +        return 0;
>> +
>> +    spin_lock_irqsave(&desc->lock, flags);
>> +
>> +    if ( desc->arch.type != DT_IRQ_TYPE_NONE && desc->arch.type != type )
>> +        goto err;
>> +
>> +    desc->arch.type = type;
> 
> There was an open coded assignment in the guest path which unfortunately
> I already trimmed. Shouldn't that have all these checks too?

No, because with patch #11 the desc->arch.type is only set once by IRQ.

>> +
>> +    ret = 0;
>> +
>> +err:
>> +    spin_unlock_irqrestore(&desc->lock, flags);
>> +    return ret;
>> +}
>> +
>> +unsigned int platform_get_irq(const struct dt_device_node *device,
>> +                              int index)
>> +{
>> +    struct dt_irq dt_irq;
>> +    struct irq_desc *desc;
>> +    unsigned int type, irq;
>> +    int res;
>> +
>> +    res = dt_device_get_irq(device, index, &dt_irq);
>> +    if ( res )
>> +        return 0;
> 
> Not an error? Do we take precautions against IRQ0 being actually used
> somewhere?

Yes in gic_interrupt. do_IRQ is by-passed because IRQ 0 is a SGI.

> We should have an explicit #define for an invalid IRQ number.

I don't think it's useful because the device tree can't provide an IRQ
smaller than 16.

>> +    irq = dt_irq.irq;
>> +    type = dt_irq.type;
>> +
>> +    /* Setup the IRQ type */
>> +
>> +    if ( irq < NR_LOCAL_IRQS )
>> +    {
>> +        unsigned int cpu;
>> +        /* For PPIs, we need to set IRQ type on every online CPUs */
>> +        for_each_cpu( cpu, &cpu_online_map )
>> +        {
>> +            desc = &per_cpu(local_irq_desc, cpu)[irq];
>> +            res = irq_set_type(desc, type);
>> +            if ( res )
>> +                return 0;
> 
> Error?
> 
> Also no need to undo any partial work?

desc->arch.type should be sync on every CPU. It would be crazy to have a
different IRQ type on every CPU.

> I haven't seen the caller yet, but for PPIs do we not get called for
> each CPU as it binds to the PPI anyway?

No, this function is only called once, when the DT is parsed at startup.

Basically, this function will replace dt_device_get_irq call.

>> +        }
>> +    }
>> +    else
>> +    {
>> +        res = irq_set_type(irq_to_desc(irq), type);
>> +        if ( res )
>> +            return 0;
>> +    }
>> +
>> +    return irq;
>> +}
>> +
>>  /*
>>   * Local variables:
>>   * mode: C
> 
>> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
>> index b52c26f..107c13a 100644
>> --- a/xen/include/asm-arm/irq.h
>> +++ b/xen/include/asm-arm/irq.h
>> @@ -16,6 +16,7 @@ struct arch_pirq
>>  
>>  struct arch_irq_desc {
>>      int eoi_cpu;
>> +    unsigned int type;
> 
> I was wondering through the above if this ought to be a "bool_t level"
> or not. Thoughts?

We have 5 possibles types:
	- default : we don't have to set the edge bit
        - level high/low
        - edge rising/falling

Even if GICv2 is only handling 2 types, it can change for the next
version of GIC.

Regards,

-- 
Julien Grall

Re: [PATCH v2 13/16] xen/arm: IRQ: Store IRQ type in arch_irq_desc

[Ian Campbell]

On Mon, 2014-04-07 at 17:06 +0100, Julien Grall wrote:
> >> @@ -240,7 +245,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
> >>      /* Set edge / level */
> >>      cfg = GICD[GICD_ICFGR + irq / 16];
> >>      edgebit = 2u << (2 * (irq % 16));
> >> -    if ( level )
> >> +    if ( (type & DT_IRQ_TYPE_LEVEL_MASK) || (type == DT_IRQ_TYPE_NONE) )
> > 
> > Is getting DT_IRQ_TYPE_NONE here not an error?
> 
> No, there is some DT like the exynos one which is using 0 (i.e
> DT_IRQ_TYPE_NONE) for the IRQ type.

The underlying physical interrupt must be one or the other though,
surely?

So either there is some implicit (or perhaps documented?) assumption
that NONE==something or the DT is buggy.

> 
> I guess we have to skip setting level/edge property in this case.
> 
> > Oh, I see this is the innards of dt_irq_is_level_triggered. Could that
> > be refactored e.g. into dt_irq_type_is_level_triggered(const something
> > type)?
> 
> I was wondering something like that instead:
> 
> if ( (type & DT_IRQ_TYPE_LEVEL_MASK) )
> ...
> else if ( (type & DT_IRQ_TYPE_EDGE_BOTH) )
> ...
> 
> So we skip the DT_IRQ_TYPE_NONE.

Well, it seems the existing code treats NONE as == level, I don't know
if that is deliberate or not.

> >> @@ -379,6 +382,67 @@ void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask)
> >>      BUG();
> >>  }
> >>  
> >> +static inline int irq_set_type(struct irq_desc *desc, unsigned int type)
> >> +{
> >> +    unsigned int flags;
> >> +    int ret = -EBUSY;
> >> +
> >> +    if ( type == DT_IRQ_TYPE_NONE )
> >> +        return 0;
> >> +
> >> +    spin_lock_irqsave(&desc->lock, flags);
> >> +
> >> +    if ( desc->arch.type != DT_IRQ_TYPE_NONE && desc->arch.type != type )
> >> +        goto err;
> >> +
> >> +    desc->arch.type = type;
> > 
> > There was an open coded assignment in the guest path which unfortunately
> > I already trimmed. Shouldn't that have all these checks too?
> 
> No, because with patch #11 the desc->arch.type is only set once by IRQ.

I don't follow. What is all this stuff above for if that is the case?

Was I misremembering the other instance of desc->arch.type = type?

> 
> >> +
> >> +    ret = 0;
> >> +
> >> +err:
> >> +    spin_unlock_irqrestore(&desc->lock, flags);
> >> +    return ret;
> >> +}
> >> +
> >> +unsigned int platform_get_irq(const struct dt_device_node *device,
> >> +                              int index)
> >> +{
> >> +    struct dt_irq dt_irq;
> >> +    struct irq_desc *desc;
> >> +    unsigned int type, irq;
> >> +    int res;
> >> +
> >> +    res = dt_device_get_irq(device, index, &dt_irq);
> >> +    if ( res )
> >> +        return 0;
> > 
> > Not an error? Do we take precautions against IRQ0 being actually used
> > somewhere?
> 
> Yes in gic_interrupt. do_IRQ is by-passed because IRQ 0 is a SGI.

Ah yes.

> > We should have an explicit #define for an invalid IRQ number.
> 
> I don't think it's useful because the device tree can't provide an IRQ
> smaller than 16.

It would also potentially serve to make the code more self-documenting.
"return INVALID_IRQ" and "if (irq == INVALID_IRQ)" are a bit more
obvious than "return 0" and "if (irq == 0)" (I suppose "if (!irq)" is ok
and more normal though)

> 
> >> +    irq = dt_irq.irq;
> >> +    type = dt_irq.type;
> >> +
> >> +    /* Setup the IRQ type */
> >> +
> >> +    if ( irq < NR_LOCAL_IRQS )
> >> +    {
> >> +        unsigned int cpu;
> >> +        /* For PPIs, we need to set IRQ type on every online CPUs */
> >> +        for_each_cpu( cpu, &cpu_online_map )
> >> +        {
> >> +            desc = &per_cpu(local_irq_desc, cpu)[irq];
> >> +            res = irq_set_type(desc, type);
> >> +            if ( res )
> >> +                return 0;
> > 
> > Error?
> > 
> > Also no need to undo any partial work?
> 
> desc->arch.type should be sync on every CPU. It would be crazy to have a
> different IRQ type on every CPU.

Well, the code as it stands appears to make a partial attempt at
handling just that. If that weren't the case irq_set_type wouldn't be
able to fail for cpu > 0.

> >> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> >> index b52c26f..107c13a 100644
> >> --- a/xen/include/asm-arm/irq.h
> >> +++ b/xen/include/asm-arm/irq.h
> >> @@ -16,6 +16,7 @@ struct arch_pirq
> >>  
> >>  struct arch_irq_desc {
> >>      int eoi_cpu;
> >> +    unsigned int type;
> > 
> > I was wondering through the above if this ought to be a "bool_t level"
> > or not. Thoughts?
> 
> We have 5 possibles types:
> 	- default : we don't have to set the edge bit
>         - level high/low
>         - edge rising/falling

OK.

> Even if GICv2 is only handling 2 types, it can change for the next
> version of GIC.

Well, there aren't all that many more ways you can frob a physical line.
I suppose MSIs will account for one more though.

Ian.

Re: [PATCH v2 13/16] xen/arm: IRQ: Store IRQ type in arch_irq_desc

[Julien Grall]

On 04/07/2014 05:26 PM, Ian Campbell wrote:
> On Mon, 2014-04-07 at 17:06 +0100, Julien Grall wrote:
>>>> @@ -240,7 +245,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
>>>>      /* Set edge / level */
>>>>      cfg = GICD[GICD_ICFGR + irq / 16];
>>>>      edgebit = 2u << (2 * (irq % 16));
>>>> -    if ( level )
>>>> +    if ( (type & DT_IRQ_TYPE_LEVEL_MASK) || (type == DT_IRQ_TYPE_NONE) )
>>>
>>> Is getting DT_IRQ_TYPE_NONE here not an error?
>>
>> No, there is some DT like the exynos one which is using 0 (i.e
>> DT_IRQ_TYPE_NONE) for the IRQ type.
> 
> The underlying physical interrupt must be one or the other though,
> surely?
> 
> So either there is some implicit (or perhaps documented?) assumption
> that NONE==something or the DT is buggy.

By default Linux is setting every interrupt to be level triggered,
active low. I've just noticed we do the same thing in gic_dist_init.

>>
>> I guess we have to skip setting level/edge property in this case.
>>
>>> Oh, I see this is the innards of dt_irq_is_level_triggered. Could that
>>> be refactored e.g. into dt_irq_type_is_level_triggered(const something
>>> type)?
>>
>> I was wondering something like that instead:
>>
>> if ( (type & DT_IRQ_TYPE_LEVEL_MASK) )
>> ...
>> else if ( (type & DT_IRQ_TYPE_EDGE_BOTH) )
>> ...
>>
>> So we skip the DT_IRQ_TYPE_NONE.
> 
> Well, it seems the existing code treats NONE as == level, I don't know
> if that is deliberate or not.

I wrote this code, until now I had forgotten why I was using NONE :).

>>>> @@ -379,6 +382,67 @@ void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask)
>>>>      BUG();
>>>>  }
>>>>  
>>>> +static inline int irq_set_type(struct irq_desc *desc, unsigned int type)
>>>> +{
>>>> +    unsigned int flags;
>>>> +    int ret = -EBUSY;
>>>> +
>>>> +    if ( type == DT_IRQ_TYPE_NONE )
>>>> +        return 0;
>>>> +
>>>> +    spin_lock_irqsave(&desc->lock, flags);
>>>> +
>>>> +    if ( desc->arch.type != DT_IRQ_TYPE_NONE && desc->arch.type != type )
>>>> +        goto err;
>>>> +
>>>> +    desc->arch.type = type;
>>>
>>> There was an open coded assignment in the guest path which unfortunately
>>> I already trimmed. Shouldn't that have all these checks too?
>>
>> No, because with patch #11 the desc->arch.type is only set once by IRQ.
> 
> I don't follow. What is all this stuff above for if that is the case?

> Was I misremembering the other instance of desc->arch.type = type?

Sorry, I was talking about desc->arch.type = type in route_dt_irq_to_guest.

>>
>>>> +
>>>> +    ret = 0;
>>>> +
>>>> +err:
>>>> +    spin_unlock_irqrestore(&desc->lock, flags);
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +unsigned int platform_get_irq(const struct dt_device_node *device,
>>>> +                              int index)
>>>> +{
>>>> +    struct dt_irq dt_irq;
>>>> +    struct irq_desc *desc;
>>>> +    unsigned int type, irq;
>>>> +    int res;
>>>> +
>>>> +    res = dt_device_get_irq(device, index, &dt_irq);
>>>> +    if ( res )
>>>> +        return 0;
>>>
>>> Not an error? Do we take precautions against IRQ0 being actually used
>>> somewhere?
>>
>> Yes in gic_interrupt. do_IRQ is by-passed because IRQ 0 is a SGI.
> 
> Ah yes.
> 
>>> We should have an explicit #define for an invalid IRQ number.
>>
>> I don't think it's useful because the device tree can't provide an IRQ
>> smaller than 16.
> 
> It would also potentially serve to make the code more self-documenting.
> "return INVALID_IRQ" and "if (irq == INVALID_IRQ)" are a bit more
> obvious than "return 0" and "if (irq == 0)" (I suppose "if (!irq)" is ok
> and more normal though)

I would prefer to use either both either nothing. It's confusing to
return INVALID_IRQ and assuming after it's always 0...

If you prefer I can add a common above the function to say 0 is used
when an error is occured.

>>
>>>> +    irq = dt_irq.irq;
>>>> +    type = dt_irq.type;
>>>> +
>>>> +    /* Setup the IRQ type */
>>>> +
>>>> +    if ( irq < NR_LOCAL_IRQS )
>>>> +    {
>>>> +        unsigned int cpu;
>>>> +        /* For PPIs, we need to set IRQ type on every online CPUs */
>>>> +        for_each_cpu( cpu, &cpu_online_map )
>>>> +        {
>>>> +            desc = &per_cpu(local_irq_desc, cpu)[irq];
>>>> +            res = irq_set_type(desc, type);
>>>> +            if ( res )
>>>> +                return 0;
>>>
>>> Error?
>>>
>>> Also no need to undo any partial work?
>>
>> desc->arch.type should be sync on every CPU. It would be crazy to have a
>> different IRQ type on every CPU.
> 
> Well, the code as it stands appears to make a partial attempt at
> handling just that. If that weren't the case irq_set_type wouldn't be
> able to fail for cpu > 0.

I just use the irq_set_type handler for more convenience. If you want I
can add an ASSERT(cpu > 0 && !res);

Regards,

-- 
Julien Grall

Re: [PATCH v2 13/16] xen/arm: IRQ: Store IRQ type in arch_irq_desc

[Ian Campbell]

On Tue, 2014-04-08 at 12:46 +0100, Julien Grall wrote:
> On 04/07/2014 05:26 PM, Ian Campbell wrote:
> > On Mon, 2014-04-07 at 17:06 +0100, Julien Grall wrote:
> >>>> @@ -240,7 +245,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
> >>>>      /* Set edge / level */
> >>>>      cfg = GICD[GICD_ICFGR + irq / 16];
> >>>>      edgebit = 2u << (2 * (irq % 16));
> >>>> -    if ( level )
> >>>> +    if ( (type & DT_IRQ_TYPE_LEVEL_MASK) || (type == DT_IRQ_TYPE_NONE) )
> >>>
> >>> Is getting DT_IRQ_TYPE_NONE here not an error?
> >>
> >> No, there is some DT like the exynos one which is using 0 (i.e
> >> DT_IRQ_TYPE_NONE) for the IRQ type.
> > 
> > The underlying physical interrupt must be one or the other though,
> > surely?
> > 
> > So either there is some implicit (or perhaps documented?) assumption
> > that NONE==something or the DT is buggy.
> 
> By default Linux is setting every interrupt to be level triggered,
> active low.

Do we know if this is because ePAPR or some other standard say this is
the default or just a random choice by Linux?

>  I've just noticed we do the same thing in gic_dist_init.

Whatever the reason for them doing it it probably make sense to do the
same, otherwise we are just making pain for ourselves.

I'm not convinced that the exynos DT doesn't also need to be fixed
though.


> >>
> >>>> +
> >>>> +    ret = 0;
> >>>> +
> >>>> +err:
> >>>> +    spin_unlock_irqrestore(&desc->lock, flags);
> >>>> +    return ret;
> >>>> +}
> >>>> +
> >>>> +unsigned int platform_get_irq(const struct dt_device_node *device,
> >>>> +                              int index)
> >>>> +{
> >>>> +    struct dt_irq dt_irq;
> >>>> +    struct irq_desc *desc;
> >>>> +    unsigned int type, irq;
> >>>> +    int res;
> >>>> +
> >>>> +    res = dt_device_get_irq(device, index, &dt_irq);
> >>>> +    if ( res )
> >>>> +        return 0;
> >>>
> >>> Not an error? Do we take precautions against IRQ0 being actually used
> >>> somewhere?
> >>
> >> Yes in gic_interrupt. do_IRQ is by-passed because IRQ 0 is a SGI.
> > 
> > Ah yes.
> > 
> >>> We should have an explicit #define for an invalid IRQ number.
> >>
> >> I don't think it's useful because the device tree can't provide an IRQ
> >> smaller than 16.
> > 
> > It would also potentially serve to make the code more self-documenting.
> > "return INVALID_IRQ" and "if (irq == INVALID_IRQ)" are a bit more
> > obvious than "return 0" and "if (irq == 0)" (I suppose "if (!irq)" is ok
> > and more normal though)
> 
> I would prefer to use either both either nothing. It's confusing to
> return INVALID_IRQ and assuming after it's always 0...

Sure, if INVALID_IRQ is used then it should be used, and we should
probably make it ~0 to avoid people mistreating it.

> If you prefer I can add a common above the function to say 0 is used
> when an error is occured.

Well, this is a more global thing than this one function really.

> >>>> +    irq = dt_irq.irq;
> >>>> +    type = dt_irq.type;
> >>>> +
> >>>> +    /* Setup the IRQ type */
> >>>> +
> >>>> +    if ( irq < NR_LOCAL_IRQS )
> >>>> +    {
> >>>> +        unsigned int cpu;
> >>>> +        /* For PPIs, we need to set IRQ type on every online CPUs */
> >>>> +        for_each_cpu( cpu, &cpu_online_map )
> >>>> +        {
> >>>> +            desc = &per_cpu(local_irq_desc, cpu)[irq];
> >>>> +            res = irq_set_type(desc, type);
> >>>> +            if ( res )
> >>>> +                return 0;
> >>>
> >>> Error?
> >>>
> >>> Also no need to undo any partial work?
> >>
> >> desc->arch.type should be sync on every CPU. It would be crazy to have a
> >> different IRQ type on every CPU.
> > 
> > Well, the code as it stands appears to make a partial attempt at
> > handling just that. If that weren't the case irq_set_type wouldn't be
> > able to fail for cpu > 0.
> 
> I just use the irq_set_type handler for more convenience. If you want I
> can add an ASSERT(cpu > 0 && !res);

If you like.

Re: [PATCH v2 13/16] xen/arm: IRQ: Store IRQ type in arch_irq_desc

[Julien Grall]

Hi Ian,

I have sent a V3 few minutes ago. I will take into account of your
remarks in V4.

On 04/08/2014 04:30 PM, Ian Campbell wrote:
> On Tue, 2014-04-08 at 12:46 +0100, Julien Grall wrote:
>> On 04/07/2014 05:26 PM, Ian Campbell wrote:
>>> On Mon, 2014-04-07 at 17:06 +0100, Julien Grall wrote:
>>>>>> @@ -240,7 +245,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
>>>>>>      /* Set edge / level */
>>>>>>      cfg = GICD[GICD_ICFGR + irq / 16];
>>>>>>      edgebit = 2u << (2 * (irq % 16));
>>>>>> -    if ( level )
>>>>>> +    if ( (type & DT_IRQ_TYPE_LEVEL_MASK) || (type == DT_IRQ_TYPE_NONE) )
>>>>>
>>>>> Is getting DT_IRQ_TYPE_NONE here not an error?
>>>>
>>>> No, there is some DT like the exynos one which is using 0 (i.e
>>>> DT_IRQ_TYPE_NONE) for the IRQ type.
>>>
>>> The underlying physical interrupt must be one or the other though,
>>> surely?
>>>
>>> So either there is some implicit (or perhaps documented?) assumption
>>> that NONE==something or the DT is buggy.
>>
>> By default Linux is setting every interrupt to be level triggered,
>> active low.
> 
> Do we know if this is because ePAPR or some other standard say this is
> the default or just a random choice by Linux?

ePAPR only describes interrupts with 2 cells. I don't find anything
about the default value.

I don't know how Linux chose this value. I guess it's because most of
interrupt level-sensitive on ARM.

>>  I've just noticed we do the same thing in gic_dist_init.
> 
> Whatever the reason for them doing it it probably make sense to do the
> same, otherwise we are just making pain for ourselves.
> 
> I'm not convinced that the exynos DT doesn't also need to be fixed
> though.

In any case we have to be able to boot any valid device tree for Linux
no matter are the values.

It seems that Linux is ignoring NONE and doesn't update the ICFGR.

>> If you prefer I can add a common above the function to say 0 is used
>> when an error is occured.
> 
> Well, this is a more global thing than this one function really.

I can use the same solution as serial_irq callbacks, i.e returning -1 in
case of an error.

Regards,

-- 
Julien Grall

Re: [PATCH v2 13/16] xen/arm: IRQ: Store IRQ type in arch_irq_desc

[Ian Campbell]

On Tue, 2014-04-08 at 16:50 +0100, Julien Grall wrote:

> >>  I've just noticed we do the same thing in gic_dist_init.
> > 
> > Whatever the reason for them doing it it probably make sense to do the
> > same, otherwise we are just making pain for ourselves.
> > 
> > I'm not convinced that the exynos DT doesn't also need to be fixed
> > though.
> 
> In any case we have to be able to boot any valid device tree for Linux
> no matter are the values.

Right. Those two statements are not contradictory though.

> It seems that Linux is ignoring NONE and doesn't update the ICFGR.

That makes sense actually: i.e. if DT says none then you have assume
that someone (e.g. the bootloader) has done the right thing already and
not touch it.

> >> If you prefer I can add a common above the function to say 0 is used
> >> when an error is occured.
> > 
> > Well, this is a more global thing than this one function really.
> 
> I can use the same solution as serial_irq callbacks, i.e returning -1 in
> case of an error.

OK.

[PATCH v2 14/16] xen/arm: IRQ: Replace {request, setup}_dt_irq by {request, setup}_irq

[Julien Grall]

Now that irq_desc stores the type of the IRQ (e.g level/edge,...), we don't
need anymore to use specific IRQ function for ARM.

Also replace every call to dt_device_get_irq by platform_get_irq which is
a wrapper to this function and setup the IRQ type correctly.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Keir Fraser <keir@xen.org>

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/gic.c                 |   12 ++++++------
 xen/arch/arm/irq.c                 |   23 +++++++++++------------
 xen/arch/arm/time.c                |   28 ++++++++++++++--------------
 xen/drivers/char/exynos4210-uart.c |   14 +++++++-------
 xen/drivers/char/ns16550.c         |   17 +++--------------
 xen/drivers/char/omap-uart.c       |   14 +++++++-------
 xen/drivers/char/pl011.c           |   17 +++++++++--------
 xen/include/asm-arm/irq.h          |    5 -----
 8 files changed, 57 insertions(+), 73 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index ec2994e..97b0153 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -47,7 +47,7 @@ static struct {
     paddr_t hbase;       /* Address of virtual interface registers */
     paddr_t vbase;       /* Address of virtual cpu interface registers */
     unsigned int lines;  /* Number of interrupts (SPIs + PPIs + SGIs) */
-    struct dt_irq maintenance; /* IRQ maintenance */
+    unsigned int maintenance_irq; /* IRQ maintenance */
     unsigned int cpus;
     spinlock_t lock;
 } gic;
@@ -439,8 +439,8 @@ void __init gic_init(void)
     if ( res || !gic.vbase || (gic.vbase & ~PAGE_MASK) )
         panic("GIC: Cannot find a valid address for the virtual CPU");
 
-    res = dt_device_get_irq(node, 0, &gic.maintenance);
-    if ( res )
+    gic.maintenance_irq = platform_get_irq(node, 0);
+    if ( !gic.maintenance_irq )
         panic("GIC: Cannot find the maintenance IRQ");
 
     /* Set the GIC as the primary interrupt controller */
@@ -455,7 +455,7 @@ void __init gic_init(void)
               "        gic_vcpu_addr=%"PRIpaddr"\n"
               "        gic_maintenance_irq=%u\n",
               gic.dbase, gic.cbase, gic.hbase, gic.vbase,
-              gic.maintenance.irq);
+              gic.maintenance_irq);
 
     if ( (gic.dbase & ~PAGE_MASK) || (gic.cbase & ~PAGE_MASK) ||
          (gic.hbase & ~PAGE_MASK) || (gic.vbase & ~PAGE_MASK) )
@@ -892,8 +892,8 @@ void gic_dump_info(struct vcpu *v)
 
 void __cpuinit init_maintenance_interrupt(void)
 {
-    request_dt_irq(&gic.maintenance, maintenance_interrupt,
-                   "irq-maintenance", NULL);
+    request_irq(gic.maintenance_irq, maintenance_interrupt,
+                "irq-maintenance", NULL);
 }
 
 /*
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index a56ccf2..49717f8 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -114,9 +114,9 @@ static inline struct domain *irq_get_domain(struct irq_desc *desc)
     return desc->action->dev_id;
 }
 
-int request_dt_irq(const struct dt_irq *irq,
-                   void (*handler)(int, void *, struct cpu_user_regs *),
-                   const char *devname, void *dev_id)
+int request_irq(unsigned int irq,
+                void (*handler)(int, void *, struct cpu_user_regs *),
+                const char *devname, void *dev_id)
 {
     struct irqaction *action;
     int retval;
@@ -127,13 +127,13 @@ int request_dt_irq(const struct dt_irq *irq,
      * which interrupt is which (messes up the interrupt freeing
      * logic etc).
      */
-    if (irq->irq >= nr_irqs)
+    if ( irq >= nr_irqs )
         return -EINVAL;
-    if (!handler)
+    if ( !handler )
         return -EINVAL;
 
     action = xmalloc(struct irqaction);
-    if (!action)
+    if ( !action )
         return -ENOMEM;
 
     action->handler = handler;
@@ -141,8 +141,8 @@ int request_dt_irq(const struct dt_irq *irq,
     action->dev_id = dev_id;
     action->free_on_release = 1;
 
-    retval = setup_dt_irq(irq, action);
-    if (retval)
+    retval = setup_irq(irq, action);
+    if ( retval )
         xfree(action);
 
     return retval;
@@ -249,14 +249,14 @@ static int __setup_irq(struct irq_desc *desc, struct irqaction *new)
     return 0;
 }
 
-int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
+int setup_irq(unsigned int irq, struct irqaction *new)
 {
     int rc;
     unsigned long flags;
     struct irq_desc *desc;
     bool_t disabled = 0;
 
-    desc = irq_to_desc(irq->irq);
+    desc = irq_to_desc(irq);
 
     spin_lock_irqsave(&desc->lock, flags);
 
@@ -266,7 +266,7 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
 
         spin_unlock_irqrestore(&desc->lock, flags);
         printk(XENLOG_ERR "ERROR: IRQ %u is already in use by the domain %u\n",
-               irq->irq, d->domain_id);
+               irq, d->domain_id);
         return -EBUSY;
     }
 
@@ -286,7 +286,6 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
          * TODO: Handle case where SPI is setup on different CPU than
          * the targeted CPU and the priority.
          */
-        desc->arch.type = irq->type;
         gic_route_irq_to_xen(desc, cpumask_of(smp_processor_id()),
                              GIC_PRI_IRQ);
         desc->handler->startup(desc);
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index ce96337..c4adfe4 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -48,13 +48,13 @@ uint64_t __read_mostly boot_count;
  * register-mapped time source in the SoC. */
 unsigned long __read_mostly cpu_khz;  /* CPU clock frequency in kHz. */
 
-static struct dt_irq timer_irq[MAX_TIMER_PPI];
+static int timer_irq[MAX_TIMER_PPI];
 
 unsigned int timer_get_irq(enum timer_ppi ppi)
 {
     ASSERT(ppi >= TIMER_PHYS_SECURE_PPI && ppi < MAX_TIMER_PPI);
 
-    return timer_irq[ppi].irq;
+    return timer_irq[ppi];
 }
 
 /*static inline*/ s_time_t ticks_to_ns(uint64_t ticks)
@@ -120,15 +120,15 @@ int __init init_xen_time(void)
     /* Retrieve all IRQs for the timer */
     for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
     {
-        res = dt_device_get_irq(dev, i, &timer_irq[i]);
-        if ( res )
+        timer_irq[i] = platform_get_irq(dev, i);
+        if ( !timer_irq[i] )
             panic("Timer: Unable to retrieve IRQ %u from the device tree", i);
     }
 
     printk("Generic Timer IRQ: phys=%u hyp=%u virt=%u\n",
-           timer_irq[TIMER_PHYS_NONSECURE_PPI].irq,
-           timer_irq[TIMER_HYP_PPI].irq,
-           timer_irq[TIMER_VIRT_PPI].irq);
+           timer_irq[TIMER_PHYS_NONSECURE_PPI],
+           timer_irq[TIMER_HYP_PPI],
+           timer_irq[TIMER_VIRT_PPI]);
 
     res = platform_init_time();
     if ( res )
@@ -192,7 +192,7 @@ int reprogram_timer(s_time_t timeout)
 /* Handle the firing timer */
 static void timer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
 {
-    if ( irq == (timer_irq[TIMER_HYP_PPI].irq) &&
+    if ( irq == (timer_irq[TIMER_HYP_PPI]) &&
          READ_SYSREG32(CNTHP_CTL_EL2) & CNTx_CTL_PENDING )
     {
         /* Signal the generic timer code to do its work */
@@ -201,7 +201,7 @@ static void timer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
         WRITE_SYSREG32(0, CNTHP_CTL_EL2);
     }
 
-    if ( irq == (timer_irq[TIMER_PHYS_NONSECURE_PPI].irq) &&
+    if ( irq == (timer_irq[TIMER_PHYS_NONSECURE_PPI]) &&
          READ_SYSREG32(CNTP_CTL_EL0) & CNTx_CTL_PENDING )
     {
         /* Signal the generic timer code to do its work */
@@ -235,12 +235,12 @@ void __cpuinit init_timer_interrupt(void)
     WRITE_SYSREG32(0, CNTHP_CTL_EL2);   /* Hypervisor's timer disabled */
     isb();
 
-    request_dt_irq(&timer_irq[TIMER_HYP_PPI], timer_interrupt,
-                   "hyptimer", NULL);
-    request_dt_irq(&timer_irq[TIMER_VIRT_PPI], vtimer_interrupt,
+    request_irq(timer_irq[TIMER_HYP_PPI], timer_interrupt,
+                "hyptimer", NULL);
+    request_irq(timer_irq[TIMER_VIRT_PPI], vtimer_interrupt,
                    "virtimer", NULL);
-    request_dt_irq(&timer_irq[TIMER_PHYS_NONSECURE_PPI], timer_interrupt,
-                   "phytimer", NULL);
+    request_irq(timer_irq[TIMER_PHYS_NONSECURE_PPI], timer_interrupt,
+                "phytimer", NULL);
 }
 
 /* Wait a set number of microseconds */
diff --git a/xen/drivers/char/exynos4210-uart.c b/xen/drivers/char/exynos4210-uart.c
index 370539c..0f77300 100644
--- a/xen/drivers/char/exynos4210-uart.c
+++ b/xen/drivers/char/exynos4210-uart.c
@@ -30,7 +30,7 @@
 
 static struct exynos4210_uart {
     unsigned int baud, clock_hz, data_bits, parity, stop_bits;
-    struct dt_irq irq;
+    unsigned int irq;
     void *regs;
     struct irqaction irqaction;
     struct vuart_info vuart;
@@ -197,9 +197,9 @@ static void __init exynos4210_uart_init_postirq(struct serial_port *port)
     uart->irqaction.name    = "exynos4210_uart";
     uart->irqaction.dev_id  = port;
 
-    if ( (rc = setup_dt_irq(&uart->irq, &uart->irqaction)) != 0 )
+    if ( (rc = setup_irq(uart->irq, &uart->irqaction)) != 0 )
         dprintk(XENLOG_ERR, "Failed to allocated exynos4210_uart IRQ %d\n",
-                uart->irq.irq);
+                uart->irq);
 
     /* Unmask interrupts */
     exynos4210_write(uart, UINTM, ~UINTM_ALLI);
@@ -272,7 +272,7 @@ static int __init exynos4210_uart_irq(struct serial_port *port)
 {
     struct exynos4210_uart *uart = port->uart;
 
-    return uart->irq.irq;
+    return uart->irq;
 }
 
 static const struct vuart_info *exynos4210_vuart_info(struct serial_port *port)
@@ -323,11 +323,11 @@ static int __init exynos4210_uart_init(struct dt_device_node *dev,
         return res;
     }
 
-    res = dt_device_get_irq(dev, 0, &uart->irq);
-    if ( res )
+    uart->irq = platform_get_irq(dev, 0);
+    if ( !uart->irq )
     {
         printk("exynos4210: Unable to retrieve the IRQ\n");
-        return res;
+        return -EINVAL;
     }
 
     uart->regs = ioremap_nocache(addr, size);
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index cc86921..78e3aab 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -76,9 +76,6 @@ static struct ns16550 {
     u8 bar_idx;
     bool_t enable_ro; /* Make MMIO devices read only to Dom0 */
 #endif
-#ifdef HAS_DEVICE_TREE
-    struct dt_irq dt_irq;
-#endif
 } ns16550_com[2] = { { 0 } };
 
 struct ns16550_config_mmio {
@@ -589,13 +586,8 @@ static void __init ns16550_init_postirq(struct serial_port *port)
         uart->irqaction.handler = ns16550_interrupt;
         uart->irqaction.name    = "ns16550";
         uart->irqaction.dev_id  = port;
-#ifdef HAS_DEVICE_TREE
-        if ( (rc = setup_dt_irq(&uart->dt_irq, &uart->irqaction)) != 0 )
-            printk("ERROR: Failed to allocate ns16550 DT IRQ.\n");
-#else
         if ( (rc = setup_irq(uart->irq, &uart->irqaction)) != 0 )
             printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart->irq);
-#endif
     }
 
     ns16550_setup_postirq(uart);
@@ -1147,12 +1139,9 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
     if ( uart->reg_width != 1 && uart->reg_width != 4 )
         return -EINVAL;
 
-    res = dt_device_get_irq(dev, 0, &uart->dt_irq);
-    if ( res )
-        return res;
-
-    /* The common bit of the driver mostly deals with irq not dt_irq. */
-    uart->irq = uart->dt_irq.irq;
+    uart->irq = platform_get_irq(dev, 0);
+    if ( !uart->irq )
+        return -EINVAL;
 
     uart->dw_usr_bsy = dt_device_is_compatible(dev, "snps,dw-apb-uart");
 
diff --git a/xen/drivers/char/omap-uart.c b/xen/drivers/char/omap-uart.c
index b8da509..c2d513b 100644
--- a/xen/drivers/char/omap-uart.c
+++ b/xen/drivers/char/omap-uart.c
@@ -30,7 +30,7 @@
 
 static struct omap_uart {
     u32 baud, clock_hz, data_bits, parity, stop_bits, fifo_size;
-    struct dt_irq irq;
+    unsigned int irq;
     char __iomem *regs;
     struct irqaction irqaction;
     struct vuart_info vuart;
@@ -205,10 +205,10 @@ static void __init omap_uart_init_postirq(struct serial_port *port)
     uart->irqaction.name = "omap_uart";
     uart->irqaction.dev_id = port;
 
-    if ( setup_dt_irq(&uart->irq, &uart->irqaction) != 0 )
+    if ( setup_irq(uart->irq, &uart->irqaction) != 0 )
     {
         dprintk(XENLOG_ERR, "Failed to allocated omap_uart IRQ %d\n",
-                uart->irq.irq);
+                uart->irq);
         return;
     }
 
@@ -259,7 +259,7 @@ static int __init omap_uart_irq(struct serial_port *port)
 {
     struct omap_uart *uart = port->uart;
 
-    return ((uart->irq.irq > 0) ? uart->irq.irq : -1);
+    return ((uart->irq > 0) ? uart->irq : -1);
 }
 
 static const struct vuart_info *omap_vuart_info(struct serial_port *port)
@@ -317,11 +317,11 @@ static int __init omap_uart_init(struct dt_device_node *dev,
         return res;
     }
 
-    res = dt_device_get_irq(dev, 0, &uart->irq);
-    if ( res )
+    uart->irq = platform_get_irq(dev, 0);
+    if ( !uart->irq )
     {
         printk("omap-uart: Unable to retrieve the IRQ\n");
-        return res;
+        return -EINVAL;
     }
 
     uart->regs = ioremap_nocache(addr, size);
diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
index 459e686..acf2bbc 100644
--- a/xen/drivers/char/pl011.c
+++ b/xen/drivers/char/pl011.c
@@ -32,7 +32,7 @@
 
 static struct pl011 {
     unsigned int baud, clock_hz, data_bits, parity, stop_bits;
-    struct dt_irq irq;
+    unsigned int irq;
     void __iomem *regs;
     /* UART with IRQ line: interrupt-driven I/O. */
     struct irqaction irqaction;
@@ -132,13 +132,13 @@ static void __init pl011_init_postirq(struct serial_port *port)
     struct pl011 *uart = port->uart;
     int rc;
 
-    if ( uart->irq.irq > 0 )
+    if ( uart->irq > 0 )
     {
         uart->irqaction.handler = pl011_interrupt;
         uart->irqaction.name    = "pl011";
         uart->irqaction.dev_id  = port;
-        if ( (rc = setup_dt_irq(&uart->irq, &uart->irqaction)) != 0 )
-            printk("ERROR: Failed to allocate pl011 IRQ %d\n", uart->irq.irq);
+        if ( (rc = setup_irq(uart->irq, &uart->irqaction)) != 0 )
+            printk("ERROR: Failed to allocate pl011 IRQ %d\n", uart->irq);
     }
 
     /* Clear pending error interrupts */
@@ -186,7 +186,8 @@ static int pl011_getc(struct serial_port *port, char *pc)
 static int __init pl011_irq(struct serial_port *port)
 {
     struct pl011 *uart = port->uart;
-    return ((uart->irq.irq > 0) ? uart->irq.irq : -1);
+
+    return ((uart->irq > 0) ? uart->irq : -1);
 }
 
 static const struct vuart_info *pl011_vuart(struct serial_port *port)
@@ -239,11 +240,11 @@ static int __init pl011_uart_init(struct dt_device_node *dev,
         return res;
     }
 
-    res = dt_device_get_irq(dev, 0, &uart->irq);
-    if ( res )
+    uart->irq = platform_get_irq(dev, 0);
+    if ( !uart->irq )
     {
         printk("pl011: Unable to retrieve the IRQ\n");
-        return res;
+        return -EINVAL;
     }
 
     uart->regs = ioremap_nocache(addr, size);
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index 107c13a..ad2cc18 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -40,11 +40,6 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq);
 void init_IRQ(void);
 void init_secondary_IRQ(void);
 
-int request_dt_irq(const struct dt_irq *irq,
-                   void (*handler)(int, void *, struct cpu_user_regs *),
-                   const char *devname, void *dev_id);
-int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new);
-
 int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
                           const char *devname);
 unsigned int platform_get_irq(const struct dt_device_node *device,
-- 
1.7.10.4

Re: [PATCH v2 14/16] xen/arm: IRQ: Replace {request, setup}_dt_irq by {request, setup}_irq

[Ian Campbell]

On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote:
> Now that irq_desc stores the type of the IRQ (e.g level/edge,...), we don't
> need anymore to use specific IRQ function for ARM.

"need to use specific...for ARM any more"

> Also replace every call to dt_device_get_irq by platform_get_irq which is
> a wrapper to this function and setup the IRQ type correctly.

Why platform_*? It's still dt specific isn't it?

> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: Keir Fraser <keir@xen.org>

Ian.

Re: [PATCH v2 14/16] xen/arm: IRQ: Replace {request, setup}_dt_irq by {request, setup}_irq

[Julien Grall]

On 04/07/2014 04:06 PM, Ian Campbell wrote:
> On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote:
>> Now that irq_desc stores the type of the IRQ (e.g level/edge,...), we don't
>> need anymore to use specific IRQ function for ARM.
> 
> "need to use specific...for ARM any more"
> 
>> Also replace every call to dt_device_get_irq by platform_get_irq which is
>> a wrapper to this function and setup the IRQ type correctly.
> 
> Why platform_*? It's still dt specific isn't it?

I didn't want to extend dt_device_get_irq because setting arch.type
doesn't belong to the DT code.

IHMO, soon this will take another structure in parameter to get the IRQ
either from ACPI or DT.

-- 
Julien Grall

[PATCH v2 15/16] xen: IRQ: Add dev_id parameter to release_irq

[Julien Grall]

The new parameter (dev_id) will be used in on ARM to release the right
action when support for mulltiple action will be added.

Even if this function is declared in common code, no one is using it. So it's
safe to modify the prototype also for x86.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Jan Beulich <jbeulich@suse.com>

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/irq.c    |    2 +-
 xen/arch/x86/irq.c    |    2 +-
 xen/include/xen/irq.h |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 49717f8..ef67279 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -213,7 +213,7 @@ out_no_end:
     irq_exit();
 }
 
-void release_irq(unsigned int irq)
+void release_irq(unsigned int irq, const void *dev_id)
 {
     struct irq_desc *desc;
     unsigned long flags;
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 88444be..f4cafd6 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -983,7 +983,7 @@ int __init request_irq(unsigned int irq,
     return retval;
 }
 
-void __init release_irq(unsigned int irq)
+void release_irq(unsigned int irq, const void *dev_id)
 {
     struct irq_desc *desc;
     unsigned long flags;
diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
index f2e6215..1f8bdb3 100644
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -90,7 +90,7 @@ int arch_init_one_irq_desc(struct irq_desc *);
 #define irq_desc_initialized(desc) ((desc)->handler != NULL)
 
 extern int setup_irq(unsigned int irq, struct irqaction *);
-extern void release_irq(unsigned int irq);
+extern void release_irq(unsigned int irq, const void *dev_id);
 extern int request_irq(unsigned int irq,
                void (*handler)(int, void *, struct cpu_user_regs *),
                const char * devname, void *dev_id);
-- 
1.7.10.4

Re: [PATCH v2 15/16] xen: IRQ: Add dev_id parameter to release_irq

[Jan Beulich]

>>> On 03.04.14 at 22:42, <julien.grall@linaro.org> wrote:
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -983,7 +983,7 @@ int __init request_irq(unsigned int irq,
>      return retval;
>  }
>  
> -void __init release_irq(unsigned int irq)
> +void release_irq(unsigned int irq, const void *dev_id)

Why? There shouldn't be any post-init use for it (and in fact the
function is - as you validly say - entirely unused right now), so
there's no need for it to stay in memory.

Jan

Re: [PATCH v2 15/16] xen: IRQ: Add dev_id parameter to release_irq

[Julien Grall]

On 04/04/2014 08:47 AM, Jan Beulich wrote:
>>>> On 03.04.14 at 22:42, <julien.grall@linaro.org> wrote:
>> --- a/xen/arch/x86/irq.c
>> +++ b/xen/arch/x86/irq.c
>> @@ -983,7 +983,7 @@ int __init request_irq(unsigned int irq,
>>      return retval;
>>  }
>>  
>> -void __init release_irq(unsigned int irq)
>> +void release_irq(unsigned int irq, const void *dev_id)
> 
> Why? There shouldn't be any post-init use for it (and in fact the
> function is - as you validly say - entirely unused right now), so
> there's no need for it to stay in memory.

Oops, my mistake. The x86 version should keep the __init but not ARM. I
will re-add the __init in the next version.

Regards,

-- 
Julien Grall

Re: [PATCH v2 15/16] xen: IRQ: Add dev_id parameter to release_irq

[Ian Campbell]

On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote:
> The new parameter (dev_id) will be used in on ARM to release the right
> action when support for mulltiple action will be added.

"multiple".

"...actions is added".

> Even if this function is declared in common code, no one is using it. So it's
> safe to modify the prototype also for x86.

Ian.

[PATCH v2 16/16] xen/arm: IRQ: Handle multiple action per IRQ

[Julien Grall]

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

To be able to use multiple action, the driver has to explicitly call
{setup,request}_irq_flags (newly created) with IRQ_SHARED as 3rd parameter.

The behavior stays the same on x86, e.g only one action is handled.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>

---
    Changes in v2:
        - Explain design choice
        - Introduce CONFIG_IRQ_HAS_MULTIPLE_ACTION
        - Use list instead of custom pointer
        - release_irq should not shutdown the IRQ at the beginning
        - Add IRQ_SHARED flags
        - Introduce request_irq_flags and setup_irq_flags
        - Fix compilation on x86. Some code are creating the irqaction
        via = { ... } so the "next" field should be moved
        toward the end
---
 xen/arch/arm/irq.c           |  106 +++++++++++++++++++++++++++++++-----------
 xen/common/irq.c             |    3 ++
 xen/include/asm-arm/config.h |    2 +
 xen/include/asm-arm/irq.h    |    7 +++
 xen/include/xen/irq.h        |    8 ++++
 5 files changed, 99 insertions(+), 27 deletions(-)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index ef67279..556c2e1 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -68,7 +68,6 @@ static int __init init_irq_data(void)
         struct irq_desc *desc = irq_to_desc(irq);
         init_one_irq_desc(desc);
         desc->irq = irq;
-        desc->action  = NULL;
     }
 
     return 0;
@@ -82,7 +81,6 @@ static int __cpuinit init_local_irq_data(void)
         struct irq_desc *desc = irq_to_desc(irq);
         init_one_irq_desc(desc);
         desc->irq = irq;
-        desc->action  = NULL;
 
         /* PPIs are include in local_irqs, we have to copy the IRQ type from
          * CPU0 otherwise we may miss the type if the IRQ type has been
@@ -107,16 +105,21 @@ void __cpuinit init_secondary_IRQ(void)
 
 static inline struct domain *irq_get_domain(struct irq_desc *desc)
 {
+    struct irqaction *action;
+
     ASSERT(spin_is_locked(&desc->lock));
     ASSERT(desc->status & IRQ_GUEST);
-    ASSERT(desc->action != NULL);
+    ASSERT(!list_empty(&desc->action));
+
+    action = list_entry(desc->action.next, struct irqaction, next);
 
-    return desc->action->dev_id;
+    return action->dev_id;
 }
 
-int request_irq(unsigned int irq,
-                void (*handler)(int, void *, struct cpu_user_regs *),
-                const char *devname, void *dev_id)
+int request_irq_flags(unsigned int irq,
+                      void (*handler)(int, void *, struct cpu_user_regs *),
+                      unsigned int irqflags,
+                      const char *devname, void *dev_id)
 {
     struct irqaction *action;
     int retval;
@@ -141,18 +144,24 @@ int request_irq(unsigned int irq,
     action->dev_id = dev_id;
     action->free_on_release = 1;
 
-    retval = setup_irq(irq, action);
+    retval = setup_irq_flags(irq, action, irqflags);
     if ( retval )
         xfree(action);
 
     return retval;
 }
 
+int request_irq(unsigned int irq,
+                void (*handler)(int, void *, struct cpu_user_regs *),
+                const char *devname, void *dev_id)
+{
+    return request_irq_flags(irq, handler, 0, devname, dev_id);
+}
+
 /* Dispatch an interrupt */
 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); */
 
@@ -163,7 +172,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);
@@ -195,12 +204,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);
-        action->handler(irq, action->dev_id, regs);
+        list_for_each_entry_safe(action, next, &desc->action, next)
+            action->handler(irq, action->dev_id, regs);
         spin_lock_irq(&desc->lock);
     }
 
@@ -217,39 +228,75 @@ void release_irq(unsigned int irq, const void *dev_id)
 {
     struct irq_desc *desc;
     unsigned long flags;
-   struct irqaction *action;
+    struct irqaction *action;
+    bool_t found = 0;
 
     desc = irq_to_desc(irq);
 
     spin_lock_irqsave(&desc->lock,flags);
 
-    desc->handler->shutdown(desc);
+    list_for_each_entry(action, &desc->action, next)
+    {
+        if ( action->dev_id == dev_id )
+        {
+            found  = 1;
+            break;
+         }
+    }
 
-    action = desc->action;
-    desc->action  = NULL;
-    desc->status &= ~IRQ_GUEST;
+    if ( !found )
+    {
+        printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n", irq);
+        return;
+    }
+
+    /* Found it - remove it from the action list */
+    list_del_init(&action->next);
+
+    /* If this was the last action, shut down the IRQ */
+    if ( list_empty(&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->free_on_release )
         xfree(action);
 }
 
-static int __setup_irq(struct irq_desc *desc, struct irqaction *new)
+static int __setup_irq(struct irq_desc *desc, struct irqaction *new,
+                       unsigned int irqflags)
 {
-    if ( desc->action != NULL )
-        return -EBUSY;
+    bool_t shared = !!(irqflags & IRQ_SHARED);
+
+    ASSERT(new != NULL);
 
-    desc->action  = new;
+    /* Sanity checks:
+     *  - if the IRQ is marked as shared
+     *  - dev_id is not NULL when IRQ_SHARED is set
+     */
+    if ( !list_empty(&desc->action) && (!(desc->status & IRQ_SHARED) || !shared) )
+        return -EINVAL;
+    if ( shared && new->dev_id == NULL )
+        return -EINVAL;
+
+    if ( shared )
+        desc->status |= IRQ_SHARED;
+
+    INIT_LIST_HEAD(&new->next);
+    list_add_tail(&new->next, &desc->action);
     dsb(sy);
 
     return 0;
 }
 
-int setup_irq(unsigned int irq, struct irqaction *new)
+int setup_irq_flags(unsigned int irq, struct irqaction *new,
+                    unsigned int irqflags)
 {
     int rc;
     unsigned long flags;
@@ -270,9 +317,9 @@ int setup_irq(unsigned int irq, struct irqaction *new)
         return -EBUSY;
     }
 
-    disabled = (desc->action == NULL);
+    disabled = list_empty(&desc->action);
 
-    rc = __setup_irq(desc, new);
+    rc = __setup_irq(desc, new, irqflags);
     if ( rc )
         goto err;
 
@@ -297,6 +344,11 @@ err:
     return rc;
 }
 
+int setup_irq(unsigned int irq, struct irqaction *new)
+{
+    return setup_irq_flags(irq, new, 0);
+}
+
 int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
                           const char * devname)
 {
@@ -320,7 +372,7 @@ int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
      *  - Otherwise -> For now, don't allow the IRQ to be shared between
      *  Xen and domains.
      */
-    if ( desc->action != NULL )
+    if ( !list_empty(&desc->action) )
     {
         struct domain *ad = irq_get_domain(desc);
 
@@ -337,7 +389,7 @@ int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
         goto out;
     }
 
-    retval = __setup_irq(desc, action);
+    retval = __setup_irq(desc, action, 0);
     if ( retval )
     {
         xfree(action);
diff --git a/xen/common/irq.c b/xen/common/irq.c
index 3e55dfa..688e48a 100644
--- a/xen/common/irq.c
+++ b/xen/common/irq.c
@@ -17,6 +17,9 @@ int init_one_irq_desc(struct irq_desc *desc)
     spin_lock_init(&desc->lock);
     cpumask_setall(desc->affinity);
     INIT_LIST_HEAD(&desc->rl_link);
+#ifdef CONFIG_IRQ_HAS_MULTIPLE_ACTION
+    INIT_LIST_HEAD(&desc->action);
+#endif
 
     err = arch_init_one_irq_desc(desc);
     if ( err )
diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index 5b7b1a8..079e8b9 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -37,6 +37,8 @@
 
 #define CONFIG_VIDEO 1
 
+#define CONFIG_IRQ_HAS_MULTIPLE_ACTION 1
+
 #define OPT_CONSOLE_STR "dtuart"
 
 #ifdef MAX_PHYS_CPUS
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index ad2cc18..80dd452 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -45,6 +45,13 @@ int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
 unsigned int platform_get_irq(const struct dt_device_node *device,
                               int index);
 
+int request_irq_flags(unsigned int irq,
+                      void (*handler)(int, void *, struct cpu_user_regs *),
+                      unsigned int irqflags,
+                      const char * devname, void *dev_id);
+int setup_irq_flags(unsigned int irq, struct irqaction *new,
+                    unsigned int irqflags);
+
 #endif /* _ASM_HW_IRQ_H */
 /*
  * Local variables:
diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
index 1f8bdb3..49a805b 100644
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -14,6 +14,9 @@ struct irqaction {
     const char *name;
     void *dev_id;
     bool_t free_on_release;
+#ifdef CONFIG_IRQ_HAS_MULTIPLE_ACTION
+    struct list_head next;
+#endif
 };
 
 /*
@@ -27,6 +30,7 @@ struct irqaction {
 #define IRQ_MOVE_PENDING  (1u<<5) /* IRQ is migrating to another CPUs */
 #define IRQ_PER_CPU       (1u<<6) /* IRQ is per CPU */
 #define IRQ_GUEST_EOI_PENDING (1u<<7) /* IRQ was disabled, pending a guest EOI */
+#define IRQ_SHARED        (1<<8)  /* IRQ is shared */
 
 /* Special IRQ numbers. */
 #define AUTO_ASSIGN_IRQ         (-1)
@@ -68,7 +72,11 @@ typedef struct irq_desc {
     unsigned int status;        /* IRQ status */
     hw_irq_controller *handler;
     struct msi_desc   *msi_desc;
+#ifdef CONFIG_IRQ_HAS_MULTIPLE_ACTION
+    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;
-- 
1.7.10.4

Re: [PATCH v2 16/16] xen/arm: IRQ: Handle multiple action per IRQ

[Jan Beulich]

>>> On 03.04.14 at 22:42, <julien.grall@linaro.org> wrote:
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -45,6 +45,13 @@ int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
>  unsigned int platform_get_irq(const struct dt_device_node *device,
>                                int index);
>  
> +int request_irq_flags(unsigned int irq,
> +                      void (*handler)(int, void *, struct cpu_user_regs *),
> +                      unsigned int irqflags,
> +                      const char * devname, void *dev_id);
> +int setup_irq_flags(unsigned int irq, struct irqaction *new,
> +                    unsigned int irqflags);
> +

I think it is a bad choice to have separate ..._flags() versions of these,
the normal functions should take flags at least when
CONFIG_IRQ_HAS_MULTIPLE_ACTION. Perhaps, with the expectation
of wanting to add further flags in the future, it would be reasonable to
have the functions always have the flags parameters, and assert the
value passed is zero on x86.

> @@ -27,6 +30,7 @@ struct irqaction {
>  #define IRQ_MOVE_PENDING  (1u<<5) /* IRQ is migrating to another CPUs */
>  #define IRQ_PER_CPU       (1u<<6) /* IRQ is per CPU */
>  #define IRQ_GUEST_EOI_PENDING (1u<<7) /* IRQ was disabled, pending a guest EOI */
> +#define IRQ_SHARED        (1<<8)  /* IRQ is shared */

This is now given two meanings (input to above functions and status),
which in the long run can become problematic. Please follow Linux by
using two distinct flag sets.

Jan

Re: [PATCH v2 16/16] xen/arm: IRQ: Handle multiple action per IRQ

[Julien Grall]

On 04/04/2014 08:59 AM, Jan Beulich wrote:
>>>> On 03.04.14 at 22:42, <julien.grall@linaro.org> wrote:
>> --- a/xen/include/asm-arm/irq.h
>> +++ b/xen/include/asm-arm/irq.h
>> @@ -45,6 +45,13 @@ int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq,
>>  unsigned int platform_get_irq(const struct dt_device_node *device,
>>                                int index);
>>  
>> +int request_irq_flags(unsigned int irq,
>> +                      void (*handler)(int, void *, struct cpu_user_regs *),
>> +                      unsigned int irqflags,
>> +                      const char * devname, void *dev_id);
>> +int setup_irq_flags(unsigned int irq, struct irqaction *new,
>> +                    unsigned int irqflags);
>> +
> 
> I think it is a bad choice to have separate ..._flags() versions of these,
> the normal functions should take flags at least when
> CONFIG_IRQ_HAS_MULTIPLE_ACTION. Perhaps, with the expectation
> of wanting to add further flags in the future, it would be reasonable to
> have the functions always have the flags parameters, and assert the
> value passed is zero on x86.

I chose this solution because I didn't know if extending the prototype
{request,setup}_irq would be acceptable.

With this patch series I've tried to remove ARM specific functions (e.g
{request,setup}_dt_irq) for IRQ management. So I would prefer to extend
the both function prototypes unconditionally to avoid ifdery in serial
drivers.

> 
>> @@ -27,6 +30,7 @@ struct irqaction {
>>  #define IRQ_MOVE_PENDING  (1u<<5) /* IRQ is migrating to another CPUs */
>>  #define IRQ_PER_CPU       (1u<<6) /* IRQ is per CPU */
>>  #define IRQ_GUEST_EOI_PENDING (1u<<7) /* IRQ was disabled, pending a guest EOI */
>> +#define IRQ_SHARED        (1<<8)  /* IRQ is shared */
> 
> This is now given two meanings (input to above functions and status),
> which in the long run can become problematic. Please follow Linux by
> using two distinct flag sets.

Ok, I will rename it Ito IRQF_SHARED and introduce a new field "flags"
in irq_desc.

Regards,

-- 
Julien Grall

Re: [PATCH v2 16/16] xen/arm: IRQ: Handle multiple action per IRQ

[Jan Beulich]

>>> On 04.04.14 at 10:52, <julien.grall@linaro.org> wrote:
> On 04/04/2014 08:59 AM, Jan Beulich wrote:
>>>>> On 03.04.14 at 22:42, <julien.grall@linaro.org> wrote:
>>> @@ -27,6 +30,7 @@ struct irqaction {
>>>  #define IRQ_MOVE_PENDING  (1u<<5) /* IRQ is migrating to another CPUs */
>>>  #define IRQ_PER_CPU       (1u<<6) /* IRQ is per CPU */
>>>  #define IRQ_GUEST_EOI_PENDING (1u<<7) /* IRQ was disabled, pending a guest EOI */
>>> +#define IRQ_SHARED        (1<<8)  /* IRQ is shared */
>> 
>> This is now given two meanings (input to above functions and status),
>> which in the long run can become problematic. Please follow Linux by
>> using two distinct flag sets.
> 
> Ok, I will rename it Ito IRQF_SHARED and introduce a new field "flags"
> in irq_desc.

No, sorry, that's not what I meant. I want the new definition solely
for the purpose of passing as function argument. Folding all the flags
internally into the status field is quite okay for the time being.

Jan