[PATCH v3 00/18] xen/arm: Interrupt management reworking

[PATCH v3 00/18] xen/arm: Interrupt management reworking

[Julien Grall]

This is the third version of this series to rework interrupt management for
ARM.

Major changes in v3:
    - Rename IRQ_SHARED into IRQF_SHARED
    - Extend {request,setup}_irq function to take an irqflags
    - Rework lock for hw_irq_controller callbacks
    - Rework some commit messages

For all changes see in each patch.

This series is a dependency for the ARM SMMU driver and depends patch #10
depends on Stefano's interrupt series.

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

Sincerely yours,

Julien Grall (18):
  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: Rework gic_route_irq_to_guest function
  xen/arm: IRQ: Move IRQ management from gic.c to irq.c
  xen/arm: IRQ Introduce irq_get_domain
  xen/arm: IRQ: Require desc.lock held by callers of hw_irq_controller
    callbacks
  xen/arm: IRQ: Defer routing IRQ to Xen until setup_irq() call
  xen/arm: IRQ: Do not allow 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: extend {request,setup}_irq to take an irqflags in
    parameter
  xen/arm: IRQ: Handle multiple action per IRQ

 xen/arch/arm/domain_build.c              |   32 ++--
 xen/arch/arm/gic.c                       |  229 +++++-----------------
 xen/arch/arm/irq.c                       |  308 +++++++++++++++++++++++++++---
 xen/arch/arm/setup.c                     |    5 +-
 xen/arch/arm/smpboot.c                   |    2 -
 xen/arch/arm/time.c                      |   41 ++--
 xen/arch/arm/vgic.c                      |   11 +-
 xen/arch/arm/vtimer.c                    |    4 +-
 xen/arch/x86/hpet.c                      |    2 +-
 xen/arch/x86/i8259.c                     |    2 +-
 xen/arch/x86/irq.c                       |   11 +-
 xen/arch/x86/time.c                      |    2 +-
 xen/common/irq.c                         |    3 +
 xen/drivers/char/exynos4210-uart.c       |   22 +--
 xen/drivers/char/ns16550.c               |   30 +--
 xen/drivers/char/omap-uart.c             |   22 +--
 xen/drivers/char/pl011.c                 |   23 +--
 xen/drivers/char/serial.c                |    9 -
 xen/drivers/passthrough/amd/iommu_init.c |    2 +-
 xen/drivers/passthrough/vtd/iommu.c      |    2 +-
 xen/include/asm-arm/config.h             |    2 +
 xen/include/asm-arm/gic.h                |   16 +-
 xen/include/asm-arm/irq.h                |   12 +-
 xen/include/asm-arm/time.h               |    7 +-
 xen/include/xen/irq.h                    |   15 +-
 xen/include/xen/serial.h                 |    5 -
 26 files changed, 455 insertions(+), 364 deletions(-)

-- 
1.7.10.4

[PATCH v3 01/18] 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 8a55016..2db6148 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 5603702..4a944dcf 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

[PATCH v3 02/18] 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>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
    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 5daa269..b287bce 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

[PATCH v3 03/18] 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>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

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

[PATCH v3 04/18] 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 a0bbde5..bc928ec 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -225,7 +225,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
  */
@@ -235,7 +234,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];
@@ -254,6 +257,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 */
@@ -276,9 +280,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;
@@ -892,7 +894,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;
@@ -913,7 +914,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 v3 05/18] xen/arm: IRQ: drop irq parameter in __setup_irq

[Julien Grall]

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>

---
    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 bc928ec..9cc5fd6 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -598,8 +598,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;
@@ -619,7 +618,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 )
@@ -903,7 +902,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

[PATCH v3 06/18] 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>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
    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 9cc5fd6..d5122fc 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -574,7 +574,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;
@@ -609,7 +609,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 b287bce..5b66a5c 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

[PATCH v3 07/18] xen/arm: IRQ: Rework gic_route_irq_to_guest function

[Julien Grall]

The function gic_route_irq_to_guest contains code which is not related to the
GIC. Split the function 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 v3:
        - Patch added (split from "Move IRQ management from gic.c to irq.c")
---
 xen/arch/arm/domain_build.c          |    2 +-
 xen/arch/arm/gic.c                   |   43 ++++++++++++++++++++++------------
 xen/arch/arm/platforms/xgene-storm.c |    2 +-
 xen/include/asm-arm/gic.h            |    3 ---
 xen/include/asm-arm/irq.h            |    3 +++
 5 files changed, 33 insertions(+), 20 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 d5122fc..d30b979 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -286,6 +286,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
+ */
+static 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)
@@ -874,15 +895,14 @@ void gic_inject(void)
         GICH[GICH_HCR] &= ~GICH_HCR_UIE;
 }
 
-int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
-                           const char * devname)
+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;
-    struct pending_irq *p;
 
     action = xmalloc(struct irqaction);
     if (!action)
@@ -894,23 +914,16 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
 
     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) {
+    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;
+    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);
diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index af3b71c..70aab73 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -66,7 +66,7 @@ static int map_one_spi(struct domain *d, const char *what,
 
     printk("Additional IRQ %u (%s)\n", irq.irq, what);
 
-    ret = gic_route_irq_to_guest(d, &irq, what);
+    ret = route_dt_irq_to_guest(d, &irq, what);
     if ( ret )
         printk("Failed to route %s to dom%d\n", what, d->domain_id);
 
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 5d8f7f1..1b94fd7 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -184,9 +184,6 @@ extern void __cpuinit init_maintenance_interrupt(void);
 extern void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
         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

[PATCH v3 08/18] 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

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

---
    Changes in v3:
        - Split the patch in 2: refactoring + code motion
    Changes in v2:
        - Patch added
---
 xen/arch/arm/gic.c                   |  102 +---------------------------------
 xen/arch/arm/irq.c                   |   97 ++++++++++++++++++++++++++++++++
 xen/arch/arm/platforms/xgene-storm.c |    2 +-
 xen/include/asm-arm/gic.h            |    4 ++
 4 files changed, 105 insertions(+), 100 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index d30b979..5f16fe6 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 uint8_t nr_lrs;
@@ -91,12 +89,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;
@@ -289,9 +281,9 @@ static int gic_route_irq(unsigned int irq, bool_t level,
 /* Program the GIC to route an interrupt to a guest
  *   - desc.lock must be held
  */
-static void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc,
-                                   bool_t level, const cpumask_t *cpu_mask,
-                                   unsigned int priority)
+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));
@@ -595,59 +587,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)
 {
@@ -895,41 +834,6 @@ void gic_inject(void)
         GICH[GICH_HCR] &= ~GICH_HCR_UIE;
 }
 
-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;
-}
-
 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 5b66a5c..bb4e049 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/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index 70aab73..af3b71c 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -66,7 +66,7 @@ static int map_one_spi(struct domain *d, const char *what,
 
     printk("Additional IRQ %u (%s)\n", irq.irq, what);
 
-    ret = route_dt_irq_to_guest(d, &irq, what);
+    ret = gic_route_irq_to_guest(d, &irq, what);
     if ( ret )
         printk("Failed to route %s to dom%d\n", what, d->domain_id);
 
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 1b94fd7..18a55c5 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -156,6 +156,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")
@@ -173,6 +174,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);
 
-- 
1.7.10.4

[PATCH v3 09/18] 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>
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 bb4e049..9320ac7 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

[PATCH v3 10/18] xen/arm: IRQ: Require desc.lock held by callers of hw_irq_controller callbacks

[Julien Grall]

When multiple action are supported, gic_irq_{startup,shutdown} will have
to be called in the same critical section as setup/release.
Otherwise there is 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 with the IRQ not being enabled.

At the same time, modify gic_irq_{enable,disable} to require desc.lock held.

With this both changes, ARM's locking requirements is the same as x86's.

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

---
    Changes in v3:
        - Update commit message
        - Require desc.lock also for gic_irq_{enable,disable}
            - irqflags is unsigned long not unsigned int

    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  |   21 +++++++++++----------
 xen/arch/arm/irq.c  |    6 ++++--
 xen/arch/arm/vgic.c |   11 ++++++++++-
 3 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 5f16fe6..cbef41f 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -132,14 +132,14 @@ static void gic_irq_enable(struct irq_desc *desc)
     int irq = desc->irq;
     unsigned long flags;
 
-    spin_lock_irqsave(&desc->lock, flags);
-    spin_lock(&gic.lock);
+    ASSERT(spin_is_locked(&desc->lock));
+
+    spin_lock_irqsave(&gic.lock, flags);
     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);
+    spin_unlock_irqrestore(&gic.lock, flags);
 }
 
 static void gic_irq_disable(struct irq_desc *desc)
@@ -147,18 +147,19 @@ static void gic_irq_disable(struct irq_desc *desc)
     int irq = desc->irq;
     unsigned long flags;
 
-    spin_lock_irqsave(&desc->lock, flags);
-    spin_lock(&gic.lock);
+    ASSERT(spin_is_locked(&desc->lock));
+
+    spin_lock_irqsave(&gic.lock, flags);
     /* Disable routing */
     GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32));
     desc->status |= IRQ_DISABLED;
-    spin_unlock(&gic.lock);
-    spin_unlock_irqrestore(&desc->lock, flags);
+    spin_unlock_irqrestore(&gic.lock, flags);
 }
 
 static unsigned int gic_irq_startup(struct irq_desc *desc)
 {
     gic_irq_enable(desc);
+
     return 0;
 }
 
@@ -265,11 +266,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 9320ac7..2d7a9e5 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;
 }
 
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 8616534..b5e919e 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -374,6 +374,7 @@ static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
     const unsigned long mask = r;
     struct pending_irq *p;
     unsigned int irq;
+    unsigned long flags;
     int i = 0;
 
     while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
@@ -382,7 +383,11 @@ static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
         clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
         gic_remove_from_queues(v, irq);
         if ( p->desc != NULL )
+        {
+            spin_lock_irqsave(&p->desc->lock, flags);
             p->desc->handler->disable(p->desc);
+            spin_unlock_irqrestore(&p->desc->lock, flags);
+        }
         i++;
     }
 }
@@ -392,6 +397,7 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
     const unsigned long mask = r;
     struct pending_irq *p;
     unsigned int irq;
+    unsigned long flags;
     int i = 0;
 
     while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
@@ -403,14 +409,17 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
              list_empty(&p->inflight) )
             vgic_vcpu_inject_irq(v, irq);
         else {
-            unsigned long flags;
             spin_lock_irqsave(&v->arch.vgic.lock, flags);
             if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
                 gic_raise_guest_irq(v, irq, p->priority);
             spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
         }
         if ( p->desc != NULL )
+        {
+            spin_lock_irqsave(&p->desc->lock, flags);
             p->desc->handler->enable(p->desc);
+            spin_unlock_irqrestore(&p->desc->lock, flags);
+        }
         i++;
     }
 }
-- 
1.7.10.4

[PATCH v3 11/18] xen/arm: IRQ: Defer routing IRQ to Xen until setup_irq() call

[Julien Grall]

When an IRQ is handling by Xen, 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 v3:
        - Don't need to initialiase disabled
        - Typoes and update commit message

    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 cbef41f..0e97fd0 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -253,30 +253,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 (i.e. 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
@@ -300,17 +290,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;
@@ -564,30 +543,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 2d7a9e5..aac39b0 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;
 
     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 2db6148..1d4317d 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);
 }
 
-/* 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 18a55c5..83000ca 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -170,15 +170,13 @@ extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq);
 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

[PATCH v3 12/18] xen/arm: IRQ: Do not allow IRQ to be shared between domains and XEN

[Julien Grall]

The current dt_route_irq_to_guest implementation sets IRQ_GUEST even if the
IRQ is correctly setup.

An IRQ can be shared between devices, if the devices are not assigned to the
same domain or Xen, then this could result in routing the IRQ to the domain
instead of Xen ...

Also avoid to relying on wrong the behaviour when Xen is routing an IRQ to
DOM0. Therefore check the return code from route_dt_irq_to_guest in
map_device.

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

---
    Changes in v3:
        - Fix typoes and update commit message

    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 aac39b0..ae92919 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

[PATCH v3 13/18] 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>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
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

[PATCH v3 14/18] 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 v3:
        - irqflags is unsigned long not unsigned int
        - fix comment
        - don't need to set IRQ type when NONE is used
        (already set at startup).

    Changes in v2:
        - Patch added
---
 xen/arch/arm/gic.c        |   23 +++++++------
 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, 92 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 0e97fd0..e789369 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -220,14 +220,19 @@ static hw_irq_controller gic_guest_irq_type = {
 /*
  * - needs to be called with a valid cpu_mask, ie each cpu in the mask has
  * already called gic_cpu_init
+ * - desc.lock must be held
  */
-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);
 
@@ -236,9 +241,9 @@ 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 )
         cfg &= ~edgebit;
-    else
+    else if ( type & DT_IRQ_TYPE_EDGE_BOTH )
         cfg |= edgebit;
     GICD[GICD_ICFGR + irq / 16] = cfg;
 
@@ -256,8 +261,8 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
 /* Program the GIC to route an interrupt to the host (i.e. 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 */
@@ -266,15 +271,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));
@@ -282,8 +286,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 ae92919..54c91e1 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 long 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 83000ca..3a524ae 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -171,11 +171,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

[PATCH v3 15/18] 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 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 v3:
        - Fix typoes in commit message

    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 e789369..44c86e3 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;
@@ -435,8 +435,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 */
@@ -451,7 +451,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) )
@@ -934,8 +934,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 54c91e1..5db474f 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;
 
-    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 1d4317d..0e32978 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

[PATCH v3 16/18] 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 multiple action 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.

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

---
    Changes in v3:
        - Fix typoes in commit message
        - Don't remove __init on release_irq for x86

    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 5db474f..598f2b4 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..eb8dcb2 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 __init 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

[PATCH v3 17/18] xen/arm: IRQ: extend {request, setup}_irq to take an irqflags in parameter

[Julien Grall]

The irqflags will be used later on ARM to know if we can shared the IRQ or not.

On x86, the irqflags should always be 0.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Xiantao Zhang <xiantao.zhang@intel.com>

---
    Changes in v3:
        - Patch added
---
 xen/arch/arm/gic.c                       |    2 +-
 xen/arch/arm/irq.c                       |    6 +++---
 xen/arch/arm/time.c                      |    6 +++---
 xen/arch/x86/hpet.c                      |    2 +-
 xen/arch/x86/i8259.c                     |    2 +-
 xen/arch/x86/irq.c                       |    9 ++++++---
 xen/arch/x86/time.c                      |    2 +-
 xen/drivers/char/exynos4210-uart.c       |    2 +-
 xen/drivers/char/ns16550.c               |    2 +-
 xen/drivers/char/omap-uart.c             |    2 +-
 xen/drivers/char/pl011.c                 |    2 +-
 xen/drivers/passthrough/amd/iommu_init.c |    2 +-
 xen/drivers/passthrough/vtd/iommu.c      |    2 +-
 xen/include/xen/irq.h                    |    5 +++--
 14 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 44c86e3..b709c2b 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -934,7 +934,7 @@ void gic_dump_info(struct vcpu *v)
 
 void __cpuinit init_maintenance_interrupt(void)
 {
-    request_irq(gic.maintenance_irq, maintenance_interrupt,
+    request_irq(gic.maintenance_irq, 0, maintenance_interrupt,
                 "irq-maintenance", NULL);
 }
 
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 598f2b4..18217e8 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -114,7 +114,7 @@ static inline struct domain *irq_get_domain(struct irq_desc *desc)
     return desc->action->dev_id;
 }
 
-int request_irq(unsigned int irq,
+int request_irq(unsigned int irq, unsigned int irqflags,
                 void (*handler)(int, void *, struct cpu_user_regs *),
                 const char *devname, void *dev_id)
 {
@@ -141,7 +141,7 @@ int request_irq(unsigned int irq,
     action->dev_id = dev_id;
     action->free_on_release = 1;
 
-    retval = setup_irq(irq, action);
+    retval = setup_irq(irq, irqflags, action);
     if ( retval )
         xfree(action);
 
@@ -249,7 +249,7 @@ static int __setup_irq(struct irq_desc *desc, struct irqaction *new)
     return 0;
 }
 
-int setup_irq(unsigned int irq, struct irqaction *new)
+int setup_irq(unsigned int irq, unsigned int irqflags, struct irqaction *new)
 {
     int rc;
     unsigned long flags;
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 0e32978..6bbe980 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -235,11 +235,11 @@ void __cpuinit init_timer_interrupt(void)
     WRITE_SYSREG32(0, CNTHP_CTL_EL2);   /* Hypervisor's timer disabled */
     isb();
 
-    request_irq(timer_irq[TIMER_HYP_PPI], timer_interrupt,
+    request_irq(timer_irq[TIMER_HYP_PPI], 0, timer_interrupt,
                 "hyptimer", NULL);
-    request_irq(timer_irq[TIMER_VIRT_PPI], vtimer_interrupt,
+    request_irq(timer_irq[TIMER_VIRT_PPI], 0, vtimer_interrupt,
                    "virtimer", NULL);
-    request_irq(timer_irq[TIMER_PHYS_NONSECURE_PPI], timer_interrupt,
+    request_irq(timer_irq[TIMER_PHYS_NONSECURE_PPI], 0, timer_interrupt,
                 "phytimer", NULL);
 }
 
diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 3a4f7e8..0b13f52 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -355,7 +355,7 @@ static int __init hpet_setup_msi_irq(struct hpet_event_channel *ch)
     hpet_write32(cfg, HPET_Tn_CFG(ch->idx));
 
     desc->handler = &hpet_msi_type;
-    ret = request_irq(ch->msi.irq, hpet_interrupt_handler, "HPET", ch);
+    ret = request_irq(ch->msi.irq, 0, hpet_interrupt_handler, "HPET", ch);
     if ( ret >= 0 )
         ret = __hpet_setup_msi_irq(desc);
     if ( ret < 0 )
diff --git a/xen/arch/x86/i8259.c b/xen/arch/x86/i8259.c
index 6fdcce8..a0270c9 100644
--- a/xen/arch/x86/i8259.c
+++ b/xen/arch/x86/i8259.c
@@ -435,6 +435,6 @@ void __init init_IRQ(void)
     outb_p(LATCH & 0xff, PIT_CH0); /* LSB */
     outb(LATCH >> 8, PIT_CH0);     /* MSB */
 
-    setup_irq(2, &cascade);
+    setup_irq(2, 0, &cascade);
 }
 
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index eb8dcb2..66c1bca 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -949,7 +949,7 @@ static int __init irq_ratelimit_init(void)
 }
 __initcall(irq_ratelimit_init);
 
-int __init request_irq(unsigned int irq,
+int __init request_irq(unsigned int irq, unsigned int irqflags,
         void (*handler)(int, void *, struct cpu_user_regs *),
         const char * devname, void *dev_id)
 {
@@ -976,7 +976,7 @@ int __init request_irq(unsigned int irq,
     action->dev_id = dev_id;
     action->free_on_release = 1;
 
-    retval = setup_irq(irq, action);
+    retval = setup_irq(irq, 0, action);
     if (retval)
         xfree(action);
 
@@ -1005,11 +1005,14 @@ void __init release_irq(unsigned int irq, const void *dev_id)
         xfree(action);
 }
 
-int __init setup_irq(unsigned int irq, struct irqaction *new)
+int __init setup_irq(unsigned int irq, unsigned int irqflags,
+                     struct irqaction *new)
 {
     struct irq_desc *desc;
     unsigned long flags;
 
+    ASSERT(irqflags == 0);
+
     desc = irq_to_desc(irq);
  
     spin_lock_irqsave(&desc->lock,flags);
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index f904af2..d2a3c11 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1464,7 +1464,7 @@ void __init early_time_init(void)
     printk("Detected %lu.%03lu MHz processor.\n", 
            cpu_khz / 1000, cpu_khz % 1000);
 
-    setup_irq(0, &irq0);
+    setup_irq(0, 0, &irq0);
 }
 
 /* keep pit enabled for pit_broadcast working while cpuidle enabled */
diff --git a/xen/drivers/char/exynos4210-uart.c b/xen/drivers/char/exynos4210-uart.c
index 0f77300..4328572 100644
--- a/xen/drivers/char/exynos4210-uart.c
+++ b/xen/drivers/char/exynos4210-uart.c
@@ -197,7 +197,7 @@ static void __init exynos4210_uart_init_postirq(struct serial_port *port)
     uart->irqaction.name    = "exynos4210_uart";
     uart->irqaction.dev_id  = port;
 
-    if ( (rc = setup_irq(uart->irq, &uart->irqaction)) != 0 )
+    if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )
         dprintk(XENLOG_ERR, "Failed to allocated exynos4210_uart IRQ %d\n",
                 uart->irq);
 
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 78e3aab..aa706b1 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -586,7 +586,7 @@ static void __init ns16550_init_postirq(struct serial_port *port)
         uart->irqaction.handler = ns16550_interrupt;
         uart->irqaction.name    = "ns16550";
         uart->irqaction.dev_id  = port;
-        if ( (rc = setup_irq(uart->irq, &uart->irqaction)) != 0 )
+        if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )
             printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart->irq);
     }
 
diff --git a/xen/drivers/char/omap-uart.c b/xen/drivers/char/omap-uart.c
index c2d513b..e4662d2 100644
--- a/xen/drivers/char/omap-uart.c
+++ b/xen/drivers/char/omap-uart.c
@@ -205,7 +205,7 @@ static void __init omap_uart_init_postirq(struct serial_port *port)
     uart->irqaction.name = "omap_uart";
     uart->irqaction.dev_id = port;
 
-    if ( setup_irq(uart->irq, &uart->irqaction) != 0 )
+    if ( setup_irq(uart->irq, 0, &uart->irqaction) != 0 )
     {
         dprintk(XENLOG_ERR, "Failed to allocated omap_uart IRQ %d\n",
                 uart->irq);
diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
index acf2bbc..19d98a1 100644
--- a/xen/drivers/char/pl011.c
+++ b/xen/drivers/char/pl011.c
@@ -137,7 +137,7 @@ static void __init pl011_init_postirq(struct serial_port *port)
         uart->irqaction.handler = pl011_interrupt;
         uart->irqaction.name    = "pl011";
         uart->irqaction.dev_id  = port;
-        if ( (rc = setup_irq(uart->irq, &uart->irqaction)) != 0 )
+        if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )
             printk("ERROR: Failed to allocate pl011 IRQ %d\n", uart->irq);
     }
 
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index 4686813..6ae44d9 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -815,7 +815,7 @@ static bool_t __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
         handler = &iommu_msi_type;
     ret = __setup_msi_irq(irq_to_desc(irq), &iommu->msi, handler);
     if ( !ret )
-        ret = request_irq(irq, iommu_interrupt_handler, "amd_iommu", iommu);
+        ret = request_irq(irq, 0, iommu_interrupt_handler, "amd_iommu", iommu);
     if ( ret )
     {
         destroy_irq(irq);
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index beb3723..6c71fdf 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1082,7 +1082,7 @@ static int __init iommu_set_interrupt(struct acpi_drhd_unit *drhd)
 
     desc = irq_to_desc(irq);
     desc->handler = &dma_msi_type;
-    ret = request_irq(irq, iommu_page_fault, "dmar", iommu);
+    ret = request_irq(irq, 0, iommu_page_fault, "dmar", iommu);
     if ( ret )
     {
         desc->handler = &no_irq_type;
diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
index 1f8bdb3..f9a18d8 100644
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -89,9 +89,10 @@ 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 int setup_irq(unsigned int irq, unsigned int irqflags,
+                     struct irqaction *);
 extern void release_irq(unsigned int irq, const void *dev_id);
-extern int request_irq(unsigned int irq,
+extern int request_irq(unsigned int irq, unsigned int irqflags,
                void (*handler)(int, void *, struct cpu_user_regs *),
                const char * devname, void *dev_id);
 
-- 
1.7.10.4

[PATCH v3 18/18] 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 with IRQF_SHARED as 2nd 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 v3:
        - Drop {setup,request}_irq_flags, the current function has been
        extended on an earlier patch.
        - Rename IRQ_SHARED into IRQF_SHARED

    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           |   83 +++++++++++++++++++++++++++++++-----------
 xen/common/irq.c             |    3 ++
 xen/include/asm-arm/config.h |    2 +
 xen/include/xen/irq.h        |    8 ++++
 4 files changed, 74 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 18217e8..31edfc8 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,11 +105,15 @@ 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, unsigned int irqflags,
@@ -152,7 +154,6 @@ int request_irq(unsigned int irq, unsigned int irqflags,
 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 +164,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 +196,14 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
 
     desc->status |= IRQ_INPROGRESS;
 
-    action = desc->action;
     while ( desc->status & IRQ_PENDING )
     {
+        struct irqaction *action, *next;
+
         desc->status &= ~IRQ_PENDING;
         spin_unlock_irq(&desc->lock);
-        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,33 +220,69 @@ 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;
+         }
+    }
+
+    if ( !found )
+    {
+        printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n", irq);
+        return;
+    }
 
-    action = desc->action;
-    desc->action  = NULL;
-    desc->status &= ~IRQ_GUEST;
+    /* 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, unsigned int irqflags,
+                       struct irqaction *new)
 {
-    if ( desc->action != NULL )
-        return -EBUSY;
+    bool_t shared = !!(irqflags & IRQF_SHARED);
+
+    ASSERT(new != NULL);
+
+    /* Sanity checks:
+     *  - if the IRQ is marked as shared
+     *  - dev_id is not NULL when IRQF_SHARED is set
+     */
+    if ( !list_empty(&desc->action) &&
+         (!(desc->status & IRQF_SHARED) || !shared) )
+        return -EINVAL;
+    if ( shared && new->dev_id == NULL )
+        return -EINVAL;
+
+    if ( shared )
+        desc->status |= IRQF_SHARED;
 
-    desc->action  = new;
+    INIT_LIST_HEAD(&new->next);
+    list_add_tail(&new->next, &desc->action);
     dsb(sy);
 
     return 0;
@@ -270,9 +309,9 @@ int setup_irq(unsigned int irq, unsigned int irqflags, struct irqaction *new)
         return -EBUSY;
     }
 
-    disabled = (desc->action == NULL);
+    disabled = list_empty(&desc->action);
 
-    rc = __setup_irq(desc, new);
+    rc = __setup_irq(desc, irqflags, new);
     if ( rc )
         goto err;
 
@@ -320,7 +359,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 +376,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, 0, action);
     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/xen/irq.h b/xen/include/xen/irq.h
index f9a18d8..c42b022 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 IRQF_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 v3 16/18] xen: IRQ: Add dev_id parameter to release_irq

[Jan Beulich]

>>> On 08.04.14 at 16:44, <julien.grall@linaro.org> wrote:
> The new parameter (dev_id) will be used in on ARM to release the right
> action when support for multiple action 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.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

x86 part:
Acked-by: Jan Beulich <jbeulich@suse.com>

> 
> ---
>     Changes in v3:
>         - Fix typoes in commit message
>         - Don't remove __init on release_irq for x86
> 
>     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 5db474f..598f2b4 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..eb8dcb2 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 __init 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 v3 17/18] xen/arm: IRQ: extend {request, setup}_irq to take an irqflags in parameter

[Jan Beulich]

>>> On 08.04.14 at 16:44, <julien.grall@linaro.org> wrote:
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -949,7 +949,7 @@ static int __init irq_ratelimit_init(void)
>  }
>  __initcall(irq_ratelimit_init);
>  
> -int __init request_irq(unsigned int irq,
> +int __init request_irq(unsigned int irq, unsigned int irqflags,
>          void (*handler)(int, void *, struct cpu_user_regs *),
>          const char * devname, void *dev_id)
>  {
> @@ -976,7 +976,7 @@ int __init request_irq(unsigned int irq,
>      action->dev_id = dev_id;
>      action->free_on_release = 1;
>  
> -    retval = setup_irq(irq, action);
> +    retval = setup_irq(irq, 0, action);

You should be passing irqflags here.

With that, for x86 and IOMMU:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v3 18/18] xen/arm: IRQ: Handle multiple action per IRQ

[Jan Beulich]

>>> On 08.04.14 at 16:44, <julien.grall@linaro.org> wrote:
> --- 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

Considering that this is a doubly linked list, "next" as the name is sort
of odd.

Jan

Re: [PATCH v3 01/18] xen/arm: timer: replace timer_dt_irq by timer_get_irq

[Ian Campbell]

On Tue, 2014-04-08 at 15:43 +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).
> 
> Replace timer_dt_irq by timer_get_irq which will return the IRQ number.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

Re: [PATCH v3 07/18] xen/arm: IRQ: Rework gic_route_irq_to_guest function

[Ian Campbell]

On Tue, 2014-04-08 at 15:43 +0100, Julien Grall wrote:
> The function gic_route_irq_to_guest contains code which is not related to the
> GIC. Split the function 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>

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

Re: [PATCH v3 10/18] xen/arm: IRQ: Require desc.lock held by callers of hw_irq_controller callbacks

[Ian Campbell]

On Tue, 2014-04-08 at 15:43 +0100, Julien Grall wrote:
> When multiple action are supported, gic_irq_{startup,shutdown} will have
> to be called in the same critical section as setup/release.
> Otherwise there is 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 with the IRQ not being enabled.
> 
> At the same time, modify gic_irq_{enable,disable} to require desc.lock held.

"require that ... is held" or "require ... be held".

> 
> With this both changes, ARM's locking requirements is the same as x86's.

"With both of these changes..."

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

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

Re: [PATCH v3 11/18] xen/arm: IRQ: Defer routing IRQ to Xen until setup_irq() call

[Ian Campbell]

On Tue, 2014-04-08 at 15:44 +0100, Julien Grall wrote:
> When an IRQ is handling by Xen, 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>

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

Re: [PATCH v3 10/18] xen/arm: IRQ: Require desc.lock held by callers of hw_irq_controller callbacks

[Julien Grall]

On 04/16/2014 04:36 PM, Ian Campbell wrote:
> On Tue, 2014-04-08 at 15:43 +0100, Julien Grall wrote:
>> When multiple action are supported, gic_irq_{startup,shutdown} will have
>> to be called in the same critical section as setup/release.
>> Otherwise there is 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 with the IRQ not being enabled.
>>
>> At the same time, modify gic_irq_{enable,disable} to require desc.lock held.
> 
> "require that ... is held" or "require ... be held".

I guess, I also have to update the commit title, rigth?

I plan to change into:

"Require desc.lock be held by callers of hw_irq_controller callbacks"

>>
>> With this both changes, ARM's locking requirements is the same as x86's.
> 
> "With both of these changes..."
> 
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks,

-- 
Julien Grall

Re: [PATCH v3 12/18] xen/arm: IRQ: Do not allow IRQ to be shared between domains and XEN

[Ian Campbell]

On Tue, 2014-04-08 at 15:44 +0100, Julien Grall wrote:
> The current dt_route_irq_to_guest implementation sets IRQ_GUEST even if the
> IRQ is correctly setup.
> 
> An IRQ can be shared between devices, if the devices are not assigned to the
> same domain or Xen, then this could result in routing the IRQ to the domain
> instead of Xen ...
> 
> Also avoid to relying on wrong the behaviour when Xen is routing an IRQ to
> DOM0. Therefore check the return code from route_dt_irq_to_guest in
> map_device.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

Re: [PATCH v3 14/18] xen/arm: IRQ: Store IRQ type in arch_irq_desc

[Ian Campbell]

On Tue, 2014-04-08 at 15:44 +0100, Julien Grall wrote:
[...]
> @@ -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);
>      }
[...]
> @@ -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);


When I asked why these two assignments weren't using irq_set_type you
said you were going to add an assert.

Re: [PATCH v3 15/18] xen/arm: IRQ: Replace {request, setup}_dt_irq by {request, setup}_irq

[Ian Campbell]

On Tue, 2014-04-08 at 15:44 +0100, Julien Grall wrote:
> Now that irq_desc stores the type of the IRQ (e.g level/edge,...), we don't
> need 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>

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

Re: [PATCH v3 17/18] xen/arm: IRQ: extend {request, setup}_irq to take an irqflags in parameter

[Julien Grall]

Hi Jan,

On 04/08/2014 03:58 PM, Jan Beulich wrote:
>>>> On 08.04.14 at 16:44, <julien.grall@linaro.org> wrote:
>> --- a/xen/arch/x86/irq.c
>> +++ b/xen/arch/x86/irq.c
>> @@ -949,7 +949,7 @@ static int __init irq_ratelimit_init(void)
>>  }
>>  __initcall(irq_ratelimit_init);
>>  
>> -int __init request_irq(unsigned int irq,
>> +int __init request_irq(unsigned int irq, unsigned int irqflags,
>>          void (*handler)(int, void *, struct cpu_user_regs *),
>>          const char * devname, void *dev_id)
>>  {
>> @@ -976,7 +976,7 @@ int __init request_irq(unsigned int irq,
>>      action->dev_id = dev_id;
>>      action->free_on_release = 1;
>>  
>> -    retval = setup_irq(irq, action);
>> +    retval = setup_irq(irq, 0, action);
> 
> You should be passing irqflags here.

Oops right. I will fix it in the next version of this series.

> With that, for x86 and IOMMU:
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks,

-- 
Julien Grall

Re: [PATCH v3 16/18] xen: IRQ: Add dev_id parameter to release_irq

[Ian Campbell]

On Tue, 2014-04-08 at 15:44 +0100, Julien Grall wrote:
> The new parameter (dev_id) will be used in on ARM to release the right
> action when support for multiple action 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.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: Jan Beulich <jbeulich@suse.com>

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

Re: [PATCH v3 17/18] xen/arm: IRQ: extend {request, setup}_irq to take an irqflags in parameter

[Ian Campbell]

On Tue, 2014-04-08 at 15:58 +0100, Jan Beulich wrote:
> >>> On 08.04.14 at 16:44, <julien.grall@linaro.org> wrote:
> > --- a/xen/arch/x86/irq.c
> > +++ b/xen/arch/x86/irq.c
> > @@ -949,7 +949,7 @@ static int __init irq_ratelimit_init(void)
> >  }
> >  __initcall(irq_ratelimit_init);
> >  
> > -int __init request_irq(unsigned int irq,
> > +int __init request_irq(unsigned int irq, unsigned int irqflags,
> >          void (*handler)(int, void *, struct cpu_user_regs *),
> >          const char * devname, void *dev_id)
> >  {
> > @@ -976,7 +976,7 @@ int __init request_irq(unsigned int irq,
> >      action->dev_id = dev_id;
> >      action->free_on_release = 1;
> >  
> > -    retval = setup_irq(irq, action);
> > +    retval = setup_irq(irq, 0, action);
> 
> You should be passing irqflags here.
> 
> With that, for x86 and IOMMU:
> Acked-by: Jan Beulich <jbeulich@suse.com>

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

Re: [PATCH v3 14/18] xen/arm: IRQ: Store IRQ type in arch_irq_desc

[Julien Grall]

On 04/16/2014 04:45 PM, Ian Campbell wrote:
> On Tue, 2014-04-08 at 15:44 +0100, Julien Grall wrote:
> [...]
>> @@ -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);
>>      }
> [...]
>> @@ -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);
> 
> 
> When I asked why these two assignments weren't using irq_set_type you
> said you were going to add an assert.

The arch.type in setup_dt_irq will be removed in next patch. It's only
here for bisection.

For the second one, I was planning to add an ASSERT in irq_set_type not
here. But, I forgot to take into account your comment from V3 on this
patch :/.

Here it's fine because the function will bail out if the IRQ desc is
already setup it (see patch #12).

Regards,

-- 
Julien Grall

Re: [PATCH v3 18/18] xen/arm: IRQ: Handle multiple action per IRQ

[Ian Campbell]

On Tue, 2014-04-08 at 15:44 +0100, Julien Grall wrote:
>          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);

You aren't removing entries from within the loop so I don't think you
need the _safe variant.

>          spin_lock_irq(&desc->lock);
>      }
>  
> @@ -217,33 +220,69 @@ 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;

Extra space before =.

I actually think a goto found would actually be clearer here than the
flag.

	for each
		if (got it)
			goto found

	printk(not found)
	return

  found:
	/* Found it. */

> +    /* Sanity checks:
> +     *  - if the IRQ is marked as shared
> +     *  - dev_id is not NULL when IRQF_SHARED is set
> +     */
> +    if ( !list_empty(&desc->action) &&
> +         (!(desc->status & IRQF_SHARED) || !shared) )
> +        return -EINVAL;

Did you mean EBUSY?

> @@ -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 */

This was never actually a list I think, and the comment is certainly
wrong now.

Ian.

Re: [PATCH v3 18/18] xen/arm: IRQ: Handle multiple action per IRQ

[Julien Grall]

Hi Jan,

On 04/08/2014 03:59 PM, Jan Beulich wrote:
>>>> On 08.04.14 at 16:44, <julien.grall@linaro.org> wrote:
>> --- 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
> 
> Considering that this is a doubly linked list, "next" as the name is sort
> of odd.

I will rename into list.

Regards,

-- 
Julien Grall

Re: [PATCH v3 18/18] xen/arm: IRQ: Handle multiple action per IRQ

[Julien Grall]

On 04/16/2014 04:54 PM, Ian Campbell wrote:
> On Tue, 2014-04-08 at 15:44 +0100, Julien Grall wrote:
>>          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);
> 
> You aren't removing entries from within the loop so I don't think you
> need the _safe variant.

As we release the desc->lock here, it might be possible to have the list
changed under the CPU feet by release_irq.

With the double-linked list, how do we make sure that it won't happen?

>>          spin_lock_irq(&desc->lock);
>>      }
>>  
>> @@ -217,33 +220,69 @@ 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;
> 
> Extra space before =.
> 
> I actually think a goto found would actually be clearer here than the
> flag.

I'm not a big fan of goto.

Anyway, I will use it here if you think it's clearer.

> 	for each
> 		if (got it)
> 			goto found
> 
> 	printk(not found)
> 	return
> 
>   found:
> 	/* Found it. */
>> +    /* Sanity checks:
>> +     *  - if the IRQ is marked as shared
>> +     *  - dev_id is not NULL when IRQF_SHARED is set
>> +     */
>> +    if ( !list_empty(&desc->action) &&
>> +         (!(desc->status & IRQF_SHARED) || !shared) )
>> +        return -EINVAL;
> 
> Did you mean EBUSY?

Right, EBUSY would be better here.

>> @@ -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 */
> 
> This was never actually a list I think, and the comment is certainly
> wrong now.

I guess it was copied from Linux :). I will change the comment into
"IRQ action"

Regards,

-- 
Julien Grall

Re: [PATCH v3 18/18] xen/arm: IRQ: Handle multiple action per IRQ

[Ian Campbell]

On Wed, 2014-04-16 at 17:06 +0100, Julien Grall wrote:
> On 04/16/2014 04:54 PM, Ian Campbell wrote:
> > On Tue, 2014-04-08 at 15:44 +0100, Julien Grall wrote:
> >>          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);
> > 
> > You aren't removing entries from within the loop so I don't think you
> > need the _safe variant.
> 
> As we release the desc->lock here, it might be possible to have the list
> changed under the CPU feet by release_irq.
> 
> With the double-linked list, how do we make sure that it won't happen?

Normally by using a lock. I don't know if even list_for_each_entry_safe
is safe against concurrent changes to the list from other threads, I
think it only refers to safe to changing the list within the loop in
this thread.
 
> > I actually think a goto found would actually be clearer here than the
> > flag.
> 
> I'm not a big fan of goto.
> 
> Anyway, I will use it here if you think it's clearer.

It has it's places, I think this is one of them.

Ian.

Re: [PATCH v3 18/18] xen/arm: IRQ: Handle multiple action per IRQ

[Jan Beulich]

>>> On 16.04.14 at 18:17, <Ian.Campbell@citrix.com> wrote:
> On Wed, 2014-04-16 at 17:06 +0100, Julien Grall wrote:
>> On 04/16/2014 04:54 PM, Ian Campbell wrote:
>> > On Tue, 2014-04-08 at 15:44 +0100, Julien Grall wrote:
>> >>          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);
>> > 
>> > You aren't removing entries from within the loop so I don't think you
>> > need the _safe variant.
>> 
>> As we release the desc->lock here, it might be possible to have the list
>> changed under the CPU feet by release_irq.
>> 
>> With the double-linked list, how do we make sure that it won't happen?
> 
> Normally by using a lock. I don't know if even list_for_each_entry_safe
> is safe against concurrent changes to the list from other threads, I
> think it only refers to safe to changing the list within the loop in
> this thread.

Indeed.

Jan

Re: [PATCH v3 18/18] xen/arm: IRQ: Handle multiple action per IRQ

[Julien Grall]

On 04/16/2014 05:17 PM, Ian Campbell wrote:
> On Wed, 2014-04-16 at 17:06 +0100, Julien Grall wrote:
>> On 04/16/2014 04:54 PM, Ian Campbell wrote:
>>> On Tue, 2014-04-08 at 15:44 +0100, Julien Grall wrote:
>>>>          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);
>>>
>>> You aren't removing entries from within the loop so I don't think you
>>> need the _safe variant.
>>
>> As we release the desc->lock here, it might be possible to have the list
>> changed under the CPU feet by release_irq.
>>
>> With the double-linked list, how do we make sure that it won't happen?
> 
> Normally by using a lock. I don't know if even list_for_each_entry_safe
> is safe against concurrent changes to the list from other threads, I
> think it only refers to safe to changing the list within the loop in
> this thread.
>  

Hmmmm... I'm wondering if we can keep desc->lock held while calling the
action handler and enable IRQ.

For SPI, we are fine as the same SPI can't be fired twice.

For PPI, it's bank so it's fine.

Did I miss something?

-- 
Julien Grall

Re: [PATCH v3 18/18] xen/arm: IRQ: Handle multiple action per IRQ

[Jan Beulich]

>>> On 16.04.14 at 18:23, <julien.grall@linaro.org> wrote:
> On 04/16/2014 05:17 PM, Ian Campbell wrote:
>> On Wed, 2014-04-16 at 17:06 +0100, Julien Grall wrote:
>>> On 04/16/2014 04:54 PM, Ian Campbell wrote:
>>>> On Tue, 2014-04-08 at 15:44 +0100, Julien Grall wrote:
>>>>>          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);
>>>>
>>>> You aren't removing entries from within the loop so I don't think you
>>>> need the _safe variant.
>>>
>>> As we release the desc->lock here, it might be possible to have the list
>>> changed under the CPU feet by release_irq.
>>>
>>> With the double-linked list, how do we make sure that it won't happen?
>> 
>> Normally by using a lock. I don't know if even list_for_each_entry_safe
>> is safe against concurrent changes to the list from other threads, I
>> think it only refers to safe to changing the list within the loop in
>> this thread.
>>  
> 
> Hmmmm... I'm wondering if we can keep desc->lock held while calling the
> action handler and enable IRQ.

That would set you up for problems with the handler wanting to
manipulate its IRQ (which might imply locking desc). I'd suggest
looking at how Linux deals with this (synchronize_irq() in particular).

Jan

Re: [PATCH v3 13/18] xen/serial: remove serial_dt_irq

[Julien Grall]

Hi,

As it's part of the "REST" category. Do I need a second ack here?

Regards,

On 04/08/2014 03:44 PM, 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>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 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);
>  
> 


-- 
Julien Grall

Re: [PATCH v3 00/18] xen/arm: Interrupt management reworking

[Julien Grall]

Hi Ian,

I went through the serie. Most of the patches are acked (see [a] below)

[a]  xen/arm: timer: replace timer_dt_irq by timer_get_irq
[a]  xen/arm: IRQ: Use default irq callback from common code for no_irq_type
[a]  xen/arm: IRQ: Rename irq_cfg into arch_irq_desc
[a]  xen/arm: IRQ: move gic {,un}lock in gic_set_irq_properties
[a]  xen/arm: IRQ: drop irq parameter in __setup_irq
[a]  xen/arm: IRQ: remove __init from setup_dt_irq, request_dt_irq and
release_irq
[a]  xen/arm: IRQ: Rework gic_route_irq_to_guest function
     xen/arm: IRQ: Move IRQ management from gic.c to irq.c
[a]  xen/arm: IRQ Introduce irq_get_domain
[a]  xen/arm: IRQ: Require desc.lock held by callers of
hw_irq_controller callbacks
[a]  xen/arm: IRQ: Defer routing IRQ to Xen until setup_irq() call
[a]  xen/arm: IRQ: Do not allow IRQ to be shared between domains and XEN
 ?   xen/serial: remove serial_dt_irq
xen/arm: IRQ: Store IRQ type in arch_irq_desc
[a]  xen/arm: IRQ: Replace {request,setup}_dt_irq by {request,setup}_irq
[a]  xen: IRQ: Add dev_id parameter to release_irq
[a]  xen/arm: IRQ: extend {request,setup}_irq to take an irqflags in
parameter
xen/arm: IRQ: Handle multiple action per IRQ

I may need another ack for patch #13 as some part of the code modifies
the serial common code.

If patch #8 ("Move IRQ...") is acked by someone, can you apply until
patch #12?

Thanks!

-- 
Julien Grall

Re: [PATCH v3 13/18] xen/serial: remove serial_dt_irq

[Ian Campbell]

On Thu, 2014-04-17 at 11:22 +0100, Julien Grall wrote:

I don't think so, if I thought there was any chance of this affecting
x86 I might want to see something from an x86 person, but I don't think
that's likely in this case.

> As it's part of the "REST" category. Do I need a second ack here?
> 
> Regards,
> 
> On 04/08/2014 03:44 PM, 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>
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 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);
> >  
> > 
> 
> 

Re: [PATCH v3 18/18] xen/arm: IRQ: Handle multiple action per IRQ

[Julien Grall]

On 04/17/2014 08:07 AM, Jan Beulich wrote:
>>>> On 16.04.14 at 18:23, <julien.grall@linaro.org> wrote:
>> On 04/16/2014 05:17 PM, Ian Campbell wrote:
>>> On Wed, 2014-04-16 at 17:06 +0100, Julien Grall wrote:
>>>> On 04/16/2014 04:54 PM, Ian Campbell wrote:
>>>>> On Tue, 2014-04-08 at 15:44 +0100, Julien Grall wrote:
>>>>>>          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);
>>>>>
>>>>> You aren't removing entries from within the loop so I don't think you
>>>>> need the _safe variant.
>>>>
>>>> As we release the desc->lock here, it might be possible to have the list
>>>> changed under the CPU feet by release_irq.
>>>>
>>>> With the double-linked list, how do we make sure that it won't happen?
>>>
>>> Normally by using a lock. I don't know if even list_for_each_entry_safe
>>> is safe against concurrent changes to the list from other threads, I
>>> think it only refers to safe to changing the list within the loop in
>>> this thread.
>>>  
>>
>> Hmmmm... I'm wondering if we can keep desc->lock held while calling the
>> action handler and enable IRQ.
> 
> That would set you up for problems with the handler wanting to
> manipulate its IRQ (which might imply locking desc). I'd suggest
> looking at how Linux deals with this (synchronize_irq() in particular).

The release_irq code is borrowed from Linux. We already synchronize at
the end.

We have to delete the element in the critical section to avoid race with
adding a new element.

synchronizing in the critical section is not possible because do_IRQ
will have to take the lock to clear the IRQ_INPROGRESS flag.

In Linux case, they are safe because they are using a single linked list.

-- 
Julien Grall

Re: [PATCH v3 08/18] xen/arm: IRQ: Move IRQ management from gic.c to irq.c

[Ian Campbell]

On Tue, 2014-04-08 at 15:43 +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
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

Re: [PATCH v3 00/18] xen/arm: Interrupt management reworking

[Ian Campbell]

On Thu, 2014-04-17 at 11:26 +0100, Julien Grall wrote:
> Hi Ian,
> 
> I went through the serie. Most of the patches are acked (see [a] below)
> 
> [a]  xen/arm: timer: replace timer_dt_irq by timer_get_irq
> [a]  xen/arm: IRQ: Use default irq callback from common code for no_irq_type
> [a]  xen/arm: IRQ: Rename irq_cfg into arch_irq_desc
> [a]  xen/arm: IRQ: move gic {,un}lock in gic_set_irq_properties
> [a]  xen/arm: IRQ: drop irq parameter in __setup_irq
> [a]  xen/arm: IRQ: remove __init from setup_dt_irq, request_dt_irq and
> release_irq
> [a]  xen/arm: IRQ: Rework gic_route_irq_to_guest function
>      xen/arm: IRQ: Move IRQ management from gic.c to irq.c

I had an Ack for this in my sent mail, but it never hit the list AFAICT.
Strange. I've resent.

> [a]  xen/arm: IRQ Introduce irq_get_domain
> [a]  xen/arm: IRQ: Require desc.lock held by callers of
> hw_irq_controller callbacks
> [a]  xen/arm: IRQ: Defer routing IRQ to Xen until setup_irq() call
> [a]  xen/arm: IRQ: Do not allow IRQ to be shared between domains and XEN
>  ?   xen/serial: remove serial_dt_irq
> xen/arm: IRQ: Store IRQ type in arch_irq_desc
> [a]  xen/arm: IRQ: Replace {request,setup}_dt_irq by {request,setup}_irq
> [a]  xen: IRQ: Add dev_id parameter to release_irq
> [a]  xen/arm: IRQ: extend {request,setup}_irq to take an irqflags in
> parameter
> xen/arm: IRQ: Handle multiple action per IRQ
> 
> I may need another ack for patch #13 as some part of the code modifies
> the serial common code.
> 
> If patch #8 ("Move IRQ...") is acked by someone, can you apply until
> patch #12?

Yes, I'll do so when I next go through my queue.

Ian.

Re: [PATCH v3 18/18] xen/arm: IRQ: Handle multiple action per IRQ

[Jan Beulich]

>>> On 17.04.14 at 12:36, <julien.grall@linaro.org> wrote:
> In Linux case, they are safe because they are using a single linked list.

And is there a strong reason for us to use a doubly linked one?

Jan

Re: [PATCH v3 18/18] xen/arm: IRQ: Handle multiple action per IRQ

[Julien Grall]

On 04/17/2014 12:15 PM, Jan Beulich wrote:
>>>> On 17.04.14 at 12:36, <julien.grall@linaro.org> wrote:
>> In Linux case, they are safe because they are using a single linked list.
> 
> And is there a strong reason for us to use a doubly linked one?

My first version was single-linked list... Ian asked me to use generic
list on V1 to avoid open code.

I was looking to port llist.h from Linux but it seems a bit overkill for
action list.

I'm happy to move back to my first solution with a "next" field (see
https://patches.linaro.org/23687/).

Regards,
-- 
Julien Grall

Re: [PATCH v3 18/18] xen/arm: IRQ: Handle multiple action per IRQ

[Ian Campbell]

On Thu, 2014-04-17 at 12:22 +0100, Julien Grall wrote:
> On 04/17/2014 12:15 PM, Jan Beulich wrote:
> >>>> On 17.04.14 at 12:36, <julien.grall@linaro.org> wrote:
> >> In Linux case, they are safe because they are using a single linked list.
> > 
> > And is there a strong reason for us to use a doubly linked one?
> 
> My first version was single-linked list... Ian asked me to use generic
> list on V1 to avoid open code.

I also suggested that you could import a single linked list
implementation if you thought it would be better.

> I was looking to port llist.h from Linux but it seems a bit overkill for
> action list.

I don't see why. I'm sure there are other potential uses of singly
linked lists which people just used a double list because it was simpler
at the time.

Ian.

Re: [PATCH v3 18/18] xen/arm: IRQ: Handle multiple action per IRQ

[Julien Grall]

On 04/17/2014 12:36 PM, Ian Campbell wrote:
> On Thu, 2014-04-17 at 12:22 +0100, Julien Grall wrote:
>> On 04/17/2014 12:15 PM, Jan Beulich wrote:
>>>>>> On 17.04.14 at 12:36, <julien.grall@linaro.org> wrote:
>>>> In Linux case, they are safe because they are using a single linked list.
>>>
>>> And is there a strong reason for us to use a doubly linked one?
>>
>> My first version was single-linked list... Ian asked me to use generic
>> list on V1 to avoid open code.
> 
> I also suggested that you could import a single linked list
> implementation if you thought it would be better.
> 
>> I was looking to port llist.h from Linux but it seems a bit overkill for
>> action list.
> 
> I don't see why. I'm sure there are other potential uses of singly
> linked lists which people just used a double list because it was simpler
> at the time.


They are using xchg and AFAIU it's not possible to delete an element
anywhere in the list. See:

http://lxr.free-electrons.com/source/include/linux/llist.h

-- 
Julien Grall

Re: [PATCH v3 18/18] xen/arm: IRQ: Handle multiple action per IRQ

[Ian Campbell]

On Thu, 2014-04-17 at 13:18 +0100, Julien Grall wrote:
> On 04/17/2014 12:36 PM, Ian Campbell wrote:
> > On Thu, 2014-04-17 at 12:22 +0100, Julien Grall wrote:
> >> On 04/17/2014 12:15 PM, Jan Beulich wrote:
> >>>>>> On 17.04.14 at 12:36, <julien.grall@linaro.org> wrote:
> >>>> In Linux case, they are safe because they are using a single linked list.
> >>>
> >>> And is there a strong reason for us to use a doubly linked one?
> >>
> >> My first version was single-linked list... Ian asked me to use generic
> >> list on V1 to avoid open code.
> > 
> > I also suggested that you could import a single linked list
> > implementation if you thought it would be better.
> > 
> >> I was looking to port llist.h from Linux but it seems a bit overkill for
> >> action list.
> > 
> > I don't see why. I'm sure there are other potential uses of singly
> > linked lists which people just used a double list because it was simpler
> > at the time.
> 
> 
> They are using xchg and AFAIU it's not possible to delete an element
> anywhere in the list. See:
> 
> http://lxr.free-electrons.com/source/include/linux/llist.h

Hrm, I thought Linux had a standard singly link list available too, oh
well.

Another option would be to delete things using the rcu mechanisms
perhaps.

Ian.

Re: [PATCH v3 18/18] xen/arm: IRQ: Handle multiple action per IRQ

[Julien Grall]

On 04/17/2014 01:27 PM, Ian Campbell wrote:
> On Thu, 2014-04-17 at 13:18 +0100, Julien Grall wrote:
>> On 04/17/2014 12:36 PM, Ian Campbell wrote:
>>> On Thu, 2014-04-17 at 12:22 +0100, Julien Grall wrote:
>>>> On 04/17/2014 12:15 PM, Jan Beulich wrote:
>>>>>>>> On 17.04.14 at 12:36, <julien.grall@linaro.org> wrote:
>>>>>> In Linux case, they are safe because they are using a single linked list.
>>>>>
>>>>> And is there a strong reason for us to use a doubly linked one?
>>>>
>>>> My first version was single-linked list... Ian asked me to use generic
>>>> list on V1 to avoid open code.
>>>
>>> I also suggested that you could import a single linked list
>>> implementation if you thought it would be better.
>>>
>>>> I was looking to port llist.h from Linux but it seems a bit overkill for
>>>> action list.
>>>
>>> I don't see why. I'm sure there are other potential uses of singly
>>> linked lists which people just used a double list because it was simpler
>>> at the time.
>>
>>
>> They are using xchg and AFAIU it's not possible to delete an element
>> anywhere in the list. See:
>>
>> http://lxr.free-electrons.com/source/include/linux/llist.h
> 
> Hrm, I thought Linux had a standard singly link list available too, oh
> well.
> 
> Another option would be to delete things using the rcu mechanisms
> perhaps.

Ok. Unless if you strongly disagree, I will stick into the "next" field.

Using rcu... doesn't seems the right things for an IRQ management code.


-- 
Julien Grall

Re: [PATCH v3 12/18] xen/arm: IRQ: Do not allow IRQ to be shared between domains and XEN

[Julien Grall]

Hi Ian,

On 08/04/14 15:44, Julien Grall wrote:
    > +    /* 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;

This code is buggy, I forgot to free the action in this case. I will fix 
it in the next version.

Ian: As said in the cover letter, I will only resend a V4 with all 
patches from this one (i.e #12 and onwards) as the other are already acked.

Let me know if I need to resend #1-#11.

Regards,

-- 
Julien Grall