Xen devel - May 2013 - [PATCH] xen/arm: Don't hardcode event channel IRQ

On some board the PPI 31 is already used by another device. Xen finds a free
slot by starting from the end of PPIs and decreasing.

For the moment, guest will use the same event channel IRQ number as dom0.

Move map_devices_from_device_tree before generating dom0 DTB, because the
latter function will use the IRQ found by the first function.

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

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 9ca44ea..ebd94d3 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -481,6 +481,10 @@ int arch_domain_create(struct domain *d, unsigned int
domcr_flags)
     d->arch.vpidr = boot_cpu_data.midr.bits;
     d->arch.vmpidr = boot_cpu_data.mpidr.bits;
 
+    /* TODO: retrieve the evtchn IRQ from the guest DTS */
+    if ( d->domain_id )
+        d->arch.evtchn_irq = dom0->arch.evtchn_irq;
+
     clear_page(d->shared_info);
     share_xen_page_with_guest(
         virt_to_page(d->shared_info), d, XENSHARE_writable);
@@ -636,6 +640,8 @@ void arch_dump_domain_info(struct domain *d)
 {
     struct vcpu *v;
 
+    printk("Event channel IRQ: %u\n", d->arch.evtchn_irq);
+
     for_each_vcpu ( d, v )
     {
         gic_dump_info(v);
@@ -672,7 +678,7 @@ void vcpu_mark_events_pending(struct vcpu *v)
     if ( already_pending )
         return;
 
-    vgic_vcpu_inject_irq(v, VGIC_IRQ_EVTCHN_CALLBACK, 1);
+    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq, 1);
 }
 
 /*
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b92c64b..f857e40 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -258,7 +258,9 @@ static int fdt_next_dom0_node(const void *fdt, int node,
     return node;
 }
 
-static void make_hypervisor_node(void *fdt, int addrcells, int sizecells)
+static void make_hypervisor_node(struct domain *d,
+                                 void *fdt, int addrcells,
+                                 int sizecells)
 {
     const char compat[]         
"xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
@@ -291,9 +293,12 @@ static void make_hypervisor_node(void *fdt, int addrcells,
int sizecells)
      * interrupts is evtchn upcall  <1 15 0xf08>
      * See linux Documentation/devicetree/bindings/arm/gic.txt
      */
+    BUG_ON(!irq_is_ppi(d->arch.evtchn_irq));
     intr[0] = cpu_to_fdt32(1); /* is a PPI */
-    intr[1] = cpu_to_fdt32(VGIC_IRQ_EVTCHN_CALLBACK - 16); /* PPIs start at 16
*/
-    intr[2] = cpu_to_fdt32(0xf08); /* Active-low level-sensitive */
+    intr[1] = cpu_to_fdt32(d->arch.evtchn_irq - FIRST_PPI_IRQ);
+    /* PPI attached for each CPU and active-low level-sensitive */
+    intr[2] = cpu_to_fdt32(((0xf) << DT_IRQ_TYPE_CPU_MASK_SHIFT) |
+                           DT_IRQ_TYPE_LEVEL_LOW);
 
     fdt_property(fdt, "interrupts", intr, sizeof(intr[0]) * 3);
 
@@ -348,14 +353,15 @@ static int write_nodes(struct domain *d, struct
kernel_info *kinfo,
     while ( last_depth-- >= 1 )
         fdt_end_node(kinfo->fdt);
 
-    make_hypervisor_node(kinfo->fdt, address_cells[0], size_cells[0]);
+    make_hypervisor_node(d, kinfo->fdt, address_cells[0], size_cells[0]);
 
     fdt_end_node(kinfo->fdt);
     return 0;
 }
 
 /* Map the device in the domain */
-static int map_device(struct domain *d, const struct dt_device_node *dev)
+static int map_device(struct domain *d, const struct dt_device_node *dev,
+                      bool_t ppis[])
 {
     unsigned int nirq;
     unsigned int naddr;
@@ -403,6 +409,9 @@ 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));
+
+        if (irq_is_ppi(irq.irq))
+            ppis[irq.irq - FIRST_PPI_IRQ] = 1;
     }
 
     /* Map the address ranges */
@@ -434,7 +443,8 @@ static int map_device(struct domain *d, const struct
dt_device_node *dev)
     return 0;
 }
 
-static int handle_node(struct domain *d, const struct dt_device_node *np)
+static int handle_node(struct domain *d, const struct dt_device_node *np,
+                       bool_t ppis[])
 {
     const struct dt_device_node *child;
     int res;
@@ -449,7 +459,7 @@ static int handle_node(struct domain *d, const struct
dt_device_node *np)
 
     if ( dt_device_used_by(np) != DOMID_XEN )
     {
-        res = map_device(d, np);
+        res = map_device(d, np, ppis);
 
         if ( res )
             return res;
@@ -457,7 +467,7 @@ static int handle_node(struct domain *d, const struct
dt_device_node *np)
 
     for ( child = np->child; child != NULL; child = child->sibling )
     {
-        res = handle_node(d, child);
+        res = handle_node(d, child, ppis);
         if ( res )
             return res;
     }
@@ -467,9 +477,41 @@ static int handle_node(struct domain *d, const struct
dt_device_node *np)
 
 static int map_devices_from_device_tree(struct domain *d)
 {
+    bool_t ppis[NR_PPI_IRQS] = {0};
+    int res;
+    unsigned int i;
+
     ASSERT(dt_host && (dt_host->sibling == NULL));
 
-    return handle_node(d, dt_host);
+    res = handle_node(d, dt_host, ppis);
+
+    /*
+     * Fill by hand timer IRQS ppis as we don''t map it in Dom0
+     * We assume the host IRQs and the dom0 IRQs are the same for the timer.
+     */
+    for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
+    {
+        const struct dt_irq *irq = timer_dt_irq(i);
+
+        if ( irq_is_ppi(irq->irq) )
+            ppis[irq->irq - FIRST_PPI_IRQ] = 1;
+    }
+
+    /* Start from the last PPIs and decrease to find a free PPI */
+    for ( i = NR_PPI_IRQS; i != 0; i-- )
+    {
+        if ( !ppis[i - 1] )
+            break;
+    }
+
+    if ( i == 0 )
+        panic("Can''t find a free PPI for the event channel
IRQ\n");
+
+    d->arch.evtchn_irq = (i - 1) + FIRST_PPI_IRQ;
+
+    printk(XENLOG_INFO "DOM0 Event channel IRQ: %u\n",
d->arch.evtchn_irq);
+
+    return res;
 }
 
 static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
@@ -546,16 +588,16 @@ int construct_dom0(struct domain *d)
 
     d->max_pages = ~0U;
 
-    rc = prepare_dtb(d, &kinfo);
+    map_devices_from_device_tree(d);
+    rc = platform_specific_mapping(d);
     if ( rc < 0 )
         return rc;
 
-    rc = kernel_prepare(&kinfo);
+    rc = prepare_dtb(d, &kinfo);
     if ( rc < 0 )
         return rc;
 
-    map_devices_from_device_tree(d);
-    rc = platform_specific_mapping(d);
+    rc = kernel_prepare(&kinfo);
     if ( rc < 0 )
         return rc;
 
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 30bf8d1..aa2710c 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -682,7 +682,7 @@ int gic_events_need_delivery(void)
 void gic_inject(void)
 {
     if ( vcpu_info(current, evtchn_upcall_pending) )
-        vgic_vcpu_inject_irq(current, VGIC_IRQ_EVTCHN_CALLBACK, 1);
+        vgic_vcpu_inject_irq(current, current->domain->arch.evtchn_irq,
1);
 
     gic_restore_pending_irqs(current);
     if (!gic_events_need_delivery())
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index cb251cc..6fc3ddd 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -76,6 +76,8 @@ struct arch_domain
         uint64_t offset;
     } virt_timer_base;
 
+    unsigned int evtchn_irq; /* Event Channel IRQ */
+
     struct {
         /*
          * Covers access to other members of this struct _except_ for
diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
index ed05901..f8e8297 100644
--- a/xen/include/asm-arm/event.h
+++ b/xen/include/asm-arm/event.h
@@ -9,7 +9,9 @@ void vcpu_mark_events_pending(struct vcpu *v);
 
 static inline int local_events_need_delivery_nomask(void)
 {
-    struct pending_irq *p = irq_to_pending(current, VGIC_IRQ_EVTCHN_CALLBACK);
+    struct pending_irq *p;
+
+    p = irq_to_pending(current, current->domain->arch.evtchn_irq);
 
     /* XXX: if the first interrupt has already been delivered, we should
      * check whether any other interrupts with priority higher than the
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 513c1fc..fc18c9e 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -129,9 +129,6 @@
 #define GICH_LR_CPUID_SHIFT     9
 #define GICH_VTR_NRLRGS         0x3f
 
-/* XXX: write this into the DT */
-#define VGIC_IRQ_EVTCHN_CALLBACK 31
-
 #ifndef __ASSEMBLY__
 #include <xen/device_tree.h>
 
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index 80ff68d..eeddd82 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -46,6 +46,14 @@ int __init request_dt_irq(const struct dt_irq *irq,
                           void *dev_id);
 int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new);
 
+#define FIRST_PPI_IRQ   16
+#define NR_PPI_IRQS     16
+
+static inline bool_t irq_is_ppi(unsigned int irq)
+{
+    return ((irq >= FIRST_PPI_IRQ) && (irq < (FIRST_PPI_IRQ +
NR_PPI_IRQS)));
+}
+
 #endif /* _ASM_HW_IRQ_H */
 /*
  * Local variables:
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 5a2a5c6..f51f451 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -114,6 +114,9 @@ struct dt_device_node {
     (DT_IRQ_TYPE_LEVEL_LOW | DT_IRQ_TYPE_LEVEL_HIGH)
 #define DT_IRQ_TYPE_SENSE_MASK     0x0000000f
 
+/* Cpu mask shift to the interrupt specifier (GIC specific) */
+#define DT_IRQ_TYPE_CPU_MASK_SHIFT      8
+
 /**
  * dt_irq - describe an IRQ in the device tree
  * @irq: IRQ number
-- 
1.7.10.4

On Fri, 2013-05-17 at 11:49 +0100, Julien Grall wrote:
> @@ -467,9 +477,41 @@ static int handle_node(struct domain *d, const struct
dt_device_node *np)
>  
>  static int map_devices_from_device_tree(struct domain *d)
>  {
> +    bool_t ppis[NR_PPI_IRQS] = {0};
> +    int res;
> +    unsigned int i;
> +
>      ASSERT(dt_host && (dt_host->sibling == NULL));
>  
> -    return handle_node(d, dt_host);
> +    res = handle_node(d, dt_host, ppis);
> +
> +    /*
> +     * Fill by hand timer IRQS ppis as we don''t map it in Dom0
> +     * We assume the host IRQs and the dom0 IRQs are the same for the
timer.
This seems a bit fragile, what happens if new interrupts are added in
the future?

Can the gic.c code not track the PPIs which are either assigned to Xen
or a guest?

Or perhaps better vgic.c should know if which if any IRQs are routed for
a given guest. We may need to add a function to allow the vtimer code to
reserve the virtual interrupts which it is going to use. Seems odd that
we don''t have an existing equivalent to gic_route_irq_to_guest in
vtimer.c...
> +     */
> +    for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
> +    {
> +        const struct dt_irq *irq = timer_dt_irq(i);
> +
> +        if ( irq_is_ppi(irq->irq) )
> +            ppis[irq->irq - FIRST_PPI_IRQ] = 1;
> +    }
> +
> +    /* Start from the last PPIs and decrease to find a free PPI */
> +    for ( i = NR_PPI_IRQS; i != 0; i-- )
> +    {
> +        if ( !ppis[i - 1] )
> +            break;
> +    }
> +
> +    if ( i == 0 )
> +        panic("Can''t find a free PPI for the event channel
IRQ\n");
> +
> +    d->arch.evtchn_irq = (i - 1) + FIRST_PPI_IRQ;
> +
> +    printk(XENLOG_INFO "DOM0 Event channel IRQ: %u\n",
d->arch.evtchn_irq);
> +
> +    return res;
>  }
>  
>  static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
> @@ -546,16 +588,16 @@ int construct_dom0(struct domain *d)
>  
>      d->max_pages = ~0U;
>  
> -    rc = prepare_dtb(d, &kinfo);
> +    map_devices_from_device_tree(d);
May as well make map_devices_from_device_tree void then, it seems ot
always succeeds or panics.

Ian.

On 05/20/2013 04:32 PM, Ian Campbell wrote:
> On Fri, 2013-05-17 at 11:49 +0100, Julien Grall wrote:
>> @@ -467,9 +477,41 @@ static int handle_node(struct domain *d, const
struct dt_device_node *np)
>>  
>>  static int map_devices_from_device_tree(struct domain *d)
>>  {
>> +    bool_t ppis[NR_PPI_IRQS] = {0};
>> +    int res;
>> +    unsigned int i;
>> +
>>      ASSERT(dt_host && (dt_host->sibling == NULL));
>>  
>> -    return handle_node(d, dt_host);
>> +    res = handle_node(d, dt_host, ppis);
>> +
>> +    /*
>> +     * Fill by hand timer IRQS ppis as we don''t map it in
Dom0
>> +     * We assume the host IRQs and the dom0 IRQs are the same for the
timer.
> 
> This seems a bit fragile, what happens if new interrupts are added in
> the future?

Right, but it''s only for the timer as timer is "shared"
between Xen and
dom0 (ie: we fake the timer IRQs).
> Can the gic.c code not track the PPIs which are either assigned to Xen
> or a guest? 
> Or perhaps better vgic.c should know if which if any IRQs are routed for
> a given guest. We may need to add a function to allow the vtimer code to
> reserve the virtual interrupts which it is going to use. Seems odd that
> we don''t have an existing equivalent to gic_route_irq_to_guest in
> vtimer.c...

vtimer will manually inject the virtual IRQ as IRQs are forwarded to the
running vCPU. gic_route_irq_to_guest will always route to a specific domain.

I though to add a new value for dt_used_by which parse the device node
IRQ/range but not map them in the domain. Because we don''t need to know
if the IRQ is a PPI after the end of construct_dom0.
>> +     */
>> +    for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
>> +    {
>> +        const struct dt_irq *irq = timer_dt_irq(i);
>> +
>> +        if ( irq_is_ppi(irq->irq) )
>> +            ppis[irq->irq - FIRST_PPI_IRQ] = 1;
>> +    }
>> +
>> +    /* Start from the last PPIs and decrease to find a free PPI */
>> +    for ( i = NR_PPI_IRQS; i != 0; i-- )
>> +    {
>> +        if ( !ppis[i - 1] )
>> +            break;
>> +    }
>> +
>> +    if ( i == 0 )
>> +        panic("Can''t find a free PPI for the event
channel IRQ\n");
>> +
>> +    d->arch.evtchn_irq = (i - 1) + FIRST_PPI_IRQ;
>> +
>> +    printk(XENLOG_INFO "DOM0 Event channel IRQ: %u\n",
d->arch.evtchn_irq);
>> +
>> +    return res;
>>  }
>>  
>>  static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>> @@ -546,16 +588,16 @@ int construct_dom0(struct domain *d)
>>  
>>      d->max_pages = ~0U;
>>  
>> -    rc = prepare_dtb(d, &kinfo);
>> +    map_devices_from_device_tree(d);
> 
> May as well make map_devices_from_device_tree void then, it seems ot
> always succeeds or panics.

Will be fixed on the next version of the patch.

-- 
Julien

On Mon, 2013-05-20 at 16:58 +0100, Julien Grall wrote:
> On 05/20/2013 04:32 PM, Ian Campbell wrote:
> 
> > On Fri, 2013-05-17 at 11:49 +0100, Julien Grall wrote:
> >> @@ -467,9 +477,41 @@ static int handle_node(struct domain *d,
const struct dt_device_node *np)
> >>  
> >>  static int map_devices_from_device_tree(struct domain *d)
> >>  {
> >> +    bool_t ppis[NR_PPI_IRQS] = {0};
> >> +    int res;
> >> +    unsigned int i;
> >> +
> >>      ASSERT(dt_host && (dt_host->sibling == NULL));
> >>  
> >> -    return handle_node(d, dt_host);
> >> +    res = handle_node(d, dt_host, ppis);
> >> +
> >> +    /*
> >> +     * Fill by hand timer IRQS ppis as we don''t map it
in Dom0
> >> +     * We assume the host IRQs and the dom0 IRQs are the same for
the timer.
> > 
> > This seems a bit fragile, what happens if new interrupts are added in
> > the future?
> 
> 
> Right, but it''s only for the timer as timer is "shared"
between Xen and
> dom0 (ie: we fake the timer IRQs).
Right now it is...
> > Can the gic.c code not track the PPIs which are either assigned to Xen
> > or a guest? 
> > Or perhaps better vgic.c should know if which if any IRQs are routed
for
> > a given guest. We may need to add a function to allow the vtimer code
to
> > reserve the virtual interrupts which it is going to use. Seems odd
that
> > we don''t have an existing equivalent to
gic_route_irq_to_guest in
> > vtimer.c...
> 
> 
> vtimer will manually inject the virtual IRQ as IRQs are forwarded to the
> running vCPU. gic_route_irq_to_guest will always route to a specific
domain.
Right. My point was that there should be an equivalent to
gic_route_irq_to_guest which basically says "this IRQ is valid for this
domain (or vcpu) and I will be manually injecting this IRQ" without
routing a real IRQ to it i.e. some new vgic function which is called
from vcpu_vtimer_init().

Ian.

On Mon, 2013-05-20 at 16:58 +0100, Julien Grall wrote:
> Will be fixed on the next version of the patch.
Did you resend this? If so then I''m afraid I''ve missed/lost
it.

Ian.

On 05/30/2013 09:12 AM, Ian Campbell wrote:
> On Mon, 2013-05-20 at 16:58 +0100, Julien Grall wrote:
>> Will be fixed on the next version of the patch.
> 
> Did you resend this? If so then I''m afraid I''ve
missed/lost it.
Unfortunately not. I will try to resend it next week.

-- 
Julien