Xen devel - Jul 2013 - Re: [XenARM] XEN tools for ARM with Virtualization Extensions - Patch #2

Second patch submitted with changes based on comments on first patch.
> > From 551f174c9653def01890fadd2dd769ab8a9acde7 Mon Sep 17 00:00:00
> 2001
> > From: Eric Trudeau <etrudeau@broadcom.com>
> > Date: Thu, 11 Jul 2013 20:03:51 -0400
> > Subject: [PATCH] Add support for Guest physdev irqs
> >
> > ---
> >  xen/arch/arm/domain.c  | 16 ++++++++++++++++
> >  xen/arch/arm/gic.c     | 15 ++++++++++-----
> >  xen/arch/arm/physdev.c | 48
> ++++++++++++++++++++++++++++++++++++++++++++++--
> >  xen/arch/arm/vgic.c    |  5 +----
> >  4 files changed, 73 insertions(+), 11 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index 4c434a1..52d3429 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -31,6 +31,8 @@
> >  #include <asm/gic.h>
> >  #include "vtimer.h"
> >  #include "vpl011.h"
> > +#include <xen/iocap.h>
> > +#include <xen/irq.h>
> >
> >  DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
> >
> > @@ -513,8 +515,22 @@ fail:
> >      return rc;
> >  }
> >
> > +static int release_domain_irqs(struct domain *d)
> > +{
> > +    int i;
> > +    for (i = 0; i <= d->nr_pirqs; i++) {
> > +        if (irq_access_permitted(d, i)) {
> > +            release_irq(i);
> > +        }
> > +    }
> > +    return 0;
> > +}
> 
> As you may know, release_irq will spin until the flags IRQ_INPROGRESS
> is unset. This is done my the maintenance handler.
> 
> Now, let say the domain has crashed and the IRQ is not yet EOIed, then you
> will spin forever.
> 
I put a timeout in the IRQ_INPROGRESS check in release_irq() of 100 attempts
with a 1 msec delay per loop iteration.
> > +
> >  void arch_domain_destroy(struct domain *d)
> >  {
> > +    if (d->irq_caps != NULL)
> You don''t need this check.
> During the domain create, Xen ensures that irq_caps is not NULL.
> 
> > +        release_domain_irqs(d);
> >      p2m_teardown(d);
> >      domain_vgic_free(d);
> >      domain_uart0_free(d);
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index cafb681..1f576d1 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -510,7 +510,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;
> > @@ -522,6 +522,7 @@ void __init release_irq(unsigned int irq)
> >      action = desc->action;
> >      desc->action  = NULL;
> >      desc->status |= IRQ_DISABLED;
> > +    desc->status &= ~IRQ_GUEST;
> >
> >      spin_lock(&gic.lock);
> >      desc->handler->shutdown(desc);
> > @@ -707,6 +708,12 @@ int gic_route_irq_to_guest(struct domain *d,
const
> struct dt_irq *irq,
> >      spin_lock_irqsave(&desc->lock, flags);
> >      spin_lock(&gic.lock);
> >
> > +    if ( desc->action != NULL )
> > +    {
> > +        retval = -EBUSY;
> > +        goto out;
> > +    }
> > +
> >      desc->handler = &gic_guest_irq_type;
> >      desc->status |= IRQ_GUEST;
> >
> > @@ -715,12 +722,10 @@ int gic_route_irq_to_guest(struct domain *d,
const
> struct dt_irq *irq,
> >      gic_set_irq_properties(irq->irq, level, 1u <<
smp_processor_id(), 0xa0);
> >
> >      retval = __setup_irq(desc, irq->irq, action);
> > -    if (retval) {
> > -        xfree(action);
> > -        goto out;
> > -    }
> >
> >  out:
> > +    if (retval)
> > +        xfree(action);
> >      spin_unlock(&gic.lock);
> >      spin_unlock_irqrestore(&desc->lock, flags);
> >      return retval;
> > diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c
> > index 61b4a18..8a5f770 100644
> > --- a/xen/arch/arm/physdev.c
> > +++ b/xen/arch/arm/physdev.c
> > @@ -9,12 +9,56 @@
> >  #include <xen/lib.h>
> >  #include <xen/errno.h>
> >  #include <asm/hypercall.h>
> > +#include <public/physdev.h>
> > +#include <xen/guest_access.h>
> > +#include <xen/irq.h>
> > +#include <xen/sched.h>
> > +#include <asm/gic.h>
> >
> >
> >  int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >  {
> > -    printk("%s %d cmd=%d: not implemented yet\n", __func__,
__LINE__,
> cmd);
> > -    return -ENOSYS;
> > +    int ret;
> > +
> > +    switch ( cmd )
> > +    {
> > +    case PHYSDEVOP_map_pirq: {
> > +        physdev_map_pirq_t map;
> > +        struct dt_irq irq;
> > +        struct domain *d;
> > +
> > +        ret = -EFAULT;
> > +        if ( copy_from_guest(&map, arg, 1) != 0 )
> > +            break;
> > +
> > +        d = rcu_lock_domain_by_any_id(map.domid);
> > +        if ( d == NULL ) {
> > +            ret = -ESRCH;
> > +            break;
> > +        }
> 
> Missing sanity check on the map.pirq value.
> 
I added a check for (map.pirq < gic_number_lines()).  This is the sanity
check
that is done in gic_route_irq().  Should this be less than or equal or are the
device tree SPI irq numbers 0-based before they are offset by 32?
> > +        irq.irq = map.pirq;
> > +        irq.type = DT_IRQ_TYPE_LEVEL_MASK;
> > +
> > +        ret = gic_route_irq_to_guest(d, &irq,
"GuestIRQ");
> 
> Do you plan to handle non 1:1 IRQ mapping?
> How does work your the IRQ mapping if the IRQ is already mapped to dom0?
> 
See comment below about sticking with 1:1 irq mapping.
> > +        if (!ret)
> > +            printk("%s gic_route_irq_to_guest added IRQ %d to
dom%d\n",
> > +                   __FUNCTION__, irq.irq, d->domain_id);
> > +
> > +        rcu_unlock_domain(d);
> > +
> > +        if (!ret && __copy_to_guest(arg, &map, 1) )
> > +            ret = -EFAULT;
> > +        break;
> > +    }
> > +
> > +    default:
> > +        printk("%s %d cmd=%d: not implemented yet\n",
__func__, __LINE__,
> cmd);
> > +        ret = -ENOSYS;
> > +        break;
> > +    }
> > +
> > +    return ret;
> >  }
> >
> >  /*
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index 2e4b11f..0ebcdac 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -82,10 +82,7 @@ int domain_vgic_init(struct domain *d)
> >      /* Currently nr_lines in vgic and gic doesn''t have the
same meanings
> >       * Here nr_lines = number of SPIs
> >       */
> > -    if ( d->domain_id == 0 )
> > -        d->arch.vgic.nr_lines = gic_number_lines() - 32;
> > -    else
> > -        d->arch.vgic.nr_lines = 0; /* We don''t need SPIs
for the guest */
> > +    d->arch.vgic.nr_lines = d->nr_pirqs - 32;
> 
> If you want to stick on the 1:1 mapping, the best solution
> is to set "nr_lines to gic_number_lines() - 32" for all the
domains.
> 
I will stick with 1:1 mapping for guest IRQs and use gic_number_lines() - 32.
> --
> Julien Grall
Patch #2 begins here:
--------------------------------------------

From 924441c3eddb1765cb4fb0e94f6b62177195837d Mon Sep 17 00:00:00 2001
From: Eric Trudeau <etrudeau@broadcom.com>
Date: Fri, 12 Jul 2013 14:54:24 -0400
Subject: [PATCH] xen/arm: Add support for 1:1 IRQ mapping into guest domains
 on ARM

ARM guests will now have the ability to access 1:1 mapped IRQs.

The irqs list in the domain configuration file is used.  A SPI irq
number must include the offset of 32.  For example, if the device
tree irq number is 76 for a SPI, then the irqs field in the dom.cfg
file would be:
   irqs = [ 108 ]

Only level-triggered IRQs are supported at this time.

When an IRQ is released on destruction of the guest, any in-progress
handlers are given at least a 100 msec to complete.

Signed-off-by: Eric Trudeau <etrudeau@broadcom.com>
---
 xen/arch/arm/domain.c  | 15 ++++++++++++++
 xen/arch/arm/gic.c     | 29 ++++++++++++++++++--------
 xen/arch/arm/physdev.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++--
 xen/arch/arm/vgic.c    |  5 +----
 4 files changed, 91 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 4c434a1..f15ff06 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -31,6 +31,8 @@
 #include <asm/gic.h>
 #include "vtimer.h"
 #include "vpl011.h"
+#include <xen/iocap.h>
+#include <xen/irq.h>

 DEFINE_PER_CPU(struct vcpu *, curr_vcpu);

@@ -513,8 +515,21 @@ fail:
     return rc;
 }

+static int release_domain_irqs(struct domain *d)
+{
+    int i;
+    for (i = 0; i <= d->nr_pirqs; i++) {
+        if (irq_access_permitted(d, i)) {
+            release_irq(i);
+        }
+    }
+    return 0;
+}
+
+
 void arch_domain_destroy(struct domain *d)
 {
+    release_domain_irqs(d);
     p2m_teardown(d);
     domain_vgic_free(d);
     domain_uart0_free(d);
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index ccce565..ed15ec3 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -30,6 +30,7 @@
 #include <xen/device_tree.h>
 #include <asm/p2m.h>
 #include <asm/domain.h>
+#include <xen/delay.h>

 #include <asm/gic.h>

@@ -510,11 +511,12 @@ 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;
-   struct irqaction *action;
+    struct irqaction *action;
+    int    inprogresschecks;

     desc = irq_to_desc(irq);

@@ -530,8 +532,15 @@ void __init release_irq(unsigned int irq)

     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 );
+    /* Wait a little while to make sure it''s not being used on another
CPU
+     * But not indefinitely, because a guest may have crashed */
+    for (inprogresschecks = 0; (inprogresschecks < 100); inprogresschecks++)
{
+        smp_mb();
+        if ( desc->status & IRQ_INPROGRESS )
+            mdelay(1);
+        else
+            break;
+    }

     if (action && action->free_on_release)
         xfree(action);
@@ -708,6 +717,12 @@ int gic_route_irq_to_guest(struct domain *d, const struct
dt_irq *irq,
     spin_lock_irqsave(&desc->lock, flags);
     spin_lock(&gic.lock);

+    if ( desc->action != NULL )
+    {
+        retval = -EBUSY;
+        goto out;
+    }
+
     desc->handler = &gic_guest_irq_type;
     desc->status |= IRQ_GUEST;

@@ -716,12 +731,10 @@ int gic_route_irq_to_guest(struct domain *d, const struct
dt_irq *irq,
     gic_set_irq_properties(irq->irq, level, 1u << smp_processor_id(),
0xa0);

     retval = __setup_irq(desc, irq->irq, action);
-    if (retval) {
-        xfree(action);
-        goto out;
-    }

 out:
+    if (retval)
+        xfree(action);
     spin_unlock(&gic.lock);
     spin_unlock_irqrestore(&desc->lock, flags);
     return retval;
diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c
index 61b4a18..8d76d11 100644
--- a/xen/arch/arm/physdev.c
+++ b/xen/arch/arm/physdev.c
@@ -9,12 +9,64 @@
 #include <xen/lib.h>
 #include <xen/errno.h>
 #include <asm/hypercall.h>
+#include <public/physdev.h>
+#include <xen/guest_access.h>
+#include <xen/irq.h>
+#include <xen/sched.h>
+#include <asm/gic.h>
+#include <xsm/xsm.h>


 int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
-    printk("%s %d cmd=%d: not implemented yet\n", __func__, __LINE__,
cmd);
-    return -ENOSYS;
+    int ret;
+
+    switch ( cmd )
+    {
+    case PHYSDEVOP_map_pirq: {
+        physdev_map_pirq_t map;
+        struct dt_irq irq;
+        struct domain *d;
+
+        ret = -EFAULT;
+        if ( copy_from_guest(&map, arg, 1) != 0 )
+            break;
+
+        d = rcu_lock_domain_by_any_id(map.domid);
+        if ( d == NULL ) {
+            ret = -ESRCH;
+            break;
+        }
+
+        ret = xsm_map_domain_pirq(XSM_TARGET, d);
+
+        if (!ret && (map.pirq >= gic_number_lines()))
+            ret = -EINVAL;
+
+        if (!ret) {
+            irq.irq = map.pirq;
+            irq.type = DT_IRQ_TYPE_LEVEL_MASK;
+
+            ret = gic_route_irq_to_guest(d, &irq, "GuestIRQ");
+            if (!ret)
+                dprintk(XENLOG_G_INFO, "dom%d: gic_route_irq_to_guest
mapped in IRQ %d\n",
+                        d->domain_id, irq.irq);
+        }
+
+        rcu_unlock_domain(d);
+
+        if (!ret && __copy_to_guest(arg, &map, 1) )
+            ret = -EFAULT;
+        break;
+    }
+
+    default:
+        printk("%s %d cmd=%d: not implemented yet\n", __func__,
__LINE__, cmd);
+        ret = -ENOSYS;
+        break;
+    }
+
+    return ret;
 }

 /*
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 2e4b11f..9c95f67 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -82,10 +82,7 @@ int domain_vgic_init(struct domain *d)
     /* Currently nr_lines in vgic and gic doesn''t have the same
meanings
      * Here nr_lines = number of SPIs
      */
-    if ( d->domain_id == 0 )
-        d->arch.vgic.nr_lines = gic_number_lines() - 32;
-    else
-        d->arch.vgic.nr_lines = 0; /* We don''t need SPIs for the
guest */
+    d->arch.vgic.nr_lines = gic_number_lines() - 32;

     d->arch.vgic.shared_irqs          xzalloc_array(struct vgic_irq_rank,
DOMAIN_NR_RANKS(d));
--
1.8.1.2

On Fri, 12 Jul 2013, Eric Trudeau wrote:
> Patch #2 begins here:
> --------------------------------------------
> 
> From 924441c3eddb1765cb4fb0e94f6b62177195837d Mon Sep 17 00:00:00 2001
> From: Eric Trudeau <etrudeau@broadcom.com>
> Date: Fri, 12 Jul 2013 14:54:24 -0400
> Subject: [PATCH] xen/arm: Add support for 1:1 IRQ mapping into guest
domains
>  on ARM
> 
> ARM guests will now have the ability to access 1:1 mapped IRQs.
> 
> The irqs list in the domain configuration file is used.  A SPI irq
> number must include the offset of 32.  For example, if the device
> tree irq number is 76 for a SPI, then the irqs field in the dom.cfg
> file would be:
>    irqs = [ 108 ]
> 
> Only level-triggered IRQs are supported at this time.
> 
> When an IRQ is released on destruction of the guest, any in-progress
> handlers are given at least a 100 msec to complete.
Overall it''s pretty good patch, but I don''t like the busy
loop. I admit
that it''s an improvement over what we have today but release_irq
wasn''t
actually called by anybody until now.

> Signed-off-by: Eric Trudeau <etrudeau@broadcom.com>
> ---
>  xen/arch/arm/domain.c  | 15 ++++++++++++++
>  xen/arch/arm/gic.c     | 29 ++++++++++++++++++--------
>  xen/arch/arm/physdev.c | 56
++++++++++++++++++++++++++++++++++++++++++++++++--
>  xen/arch/arm/vgic.c    |  5 +----
>  4 files changed, 91 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 4c434a1..f15ff06 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -31,6 +31,8 @@
>  #include <asm/gic.h>
>  #include "vtimer.h"
>  #include "vpl011.h"
> +#include <xen/iocap.h>
> +#include <xen/irq.h>
> 
>  DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
> 
> @@ -513,8 +515,21 @@ fail:
>      return rc;
>  }
> 
> +static int release_domain_irqs(struct domain *d)
> +{
> +    int i;
> +    for (i = 0; i <= d->nr_pirqs; i++) {
> +        if (irq_access_permitted(d, i)) {
> +            release_irq(i);
> +        }
> +    }
> +    return 0;
> +}
> +
> +
>  void arch_domain_destroy(struct domain *d)
>  {
> +    release_domain_irqs(d);
>      p2m_teardown(d);
>      domain_vgic_free(d);
>      domain_uart0_free(d);
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index ccce565..ed15ec3 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -30,6 +30,7 @@
>  #include <xen/device_tree.h>
>  #include <asm/p2m.h>
>  #include <asm/domain.h>
> +#include <xen/delay.h>
> 
>  #include <asm/gic.h>
> 
> @@ -510,11 +511,12 @@ 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;
> -   struct irqaction *action;
> +    struct irqaction *action;
> +    int    inprogresschecks;
> 
>      desc = irq_to_desc(irq);
> 
> @@ -530,8 +532,15 @@ void __init release_irq(unsigned int irq)
> 
>      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 );
> +    /* Wait a little while to make sure it''s not being used on
another CPU
> +     * But not indefinitely, because a guest may have crashed */
> +    for (inprogresschecks = 0; (inprogresschecks < 100);
inprogresschecks++) {
> +        smp_mb();
> +        if ( desc->status & IRQ_INPROGRESS )
> +            mdelay(1);
> +        else
> +            break;
> +    }
Can we use a timer based watchdog instead to avoid the busy loop?
Looping for 100ms means wasting a considerable amount of time.

>      if (action && action->free_on_release)
>          xfree(action);
> @@ -708,6 +717,12 @@ int gic_route_irq_to_guest(struct domain *d, const
struct dt_irq *irq,
>      spin_lock_irqsave(&desc->lock, flags);
>      spin_lock(&gic.lock);
> 
> +    if ( desc->action != NULL )
> +    {
> +        retval = -EBUSY;
> +        goto out;
> +    }
>
>      desc->handler = &gic_guest_irq_type;
>      desc->status |= IRQ_GUEST;
> 
> @@ -716,12 +731,10 @@ int gic_route_irq_to_guest(struct domain *d, const
struct dt_irq *irq,
>      gic_set_irq_properties(irq->irq, level, 1u <<
smp_processor_id(), 0xa0);
> 
>      retval = __setup_irq(desc, irq->irq, action);
> -    if (retval) {
> -        xfree(action);
> -        goto out;
> -    }
> 
>  out:
> +    if (retval)
> +        xfree(action);
>      spin_unlock(&gic.lock);
>      spin_unlock_irqrestore(&desc->lock, flags);
>      return retval;
> diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c
> index 61b4a18..8d76d11 100644
> --- a/xen/arch/arm/physdev.c
> +++ b/xen/arch/arm/physdev.c
> @@ -9,12 +9,64 @@
>  #include <xen/lib.h>
>  #include <xen/errno.h>
>  #include <asm/hypercall.h>
> +#include <public/physdev.h>
> +#include <xen/guest_access.h>
> +#include <xen/irq.h>
> +#include <xen/sched.h>
> +#include <asm/gic.h>
> +#include <xsm/xsm.h>
> 
> 
>  int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
> -    printk("%s %d cmd=%d: not implemented yet\n", __func__,
__LINE__, cmd);
> -    return -ENOSYS;
> +    int ret;
> +
> +    switch ( cmd )
> +    {
> +    case PHYSDEVOP_map_pirq: {
> +        physdev_map_pirq_t map;
> +        struct dt_irq irq;
> +        struct domain *d;
> +
> +        ret = -EFAULT;
> +        if ( copy_from_guest(&map, arg, 1) != 0 )
> +            break;
> +
> +        d = rcu_lock_domain_by_any_id(map.domid);
> +        if ( d == NULL ) {
> +            ret = -ESRCH;
> +            break;
> +        }
> +
> +        ret = xsm_map_domain_pirq(XSM_TARGET, d);
> +
> +        if (!ret && (map.pirq >= gic_number_lines()))
> +            ret = -EINVAL;
> +
> +        if (!ret) {
> +            irq.irq = map.pirq;
> +            irq.type = DT_IRQ_TYPE_LEVEL_MASK;
You should add a comment here to explain why you are hardcoding level,
even if it''s just a temporary limitation.

> +            ret = gic_route_irq_to_guest(d, &irq,
"GuestIRQ");
> +            if (!ret)
Code style (even though I admit that we are not following it to the
letter ourselves). See CODING_STYLE.

> +                dprintk(XENLOG_G_INFO, "dom%d: gic_route_irq_to_guest
mapped in IRQ %d\n",
> +                        d->domain_id, irq.irq);
> +        }
> +
> +        rcu_unlock_domain(d);
> +
> +        if (!ret && __copy_to_guest(arg, &map, 1) )
Shouldn''t you be copying back the irq number written by
gic_route_irq_to_guest in map.irq?

> +            ret = -EFAULT;
> +        break;
> +    }
> +
> +    default:
> +        printk("%s %d cmd=%d: not implemented yet\n", __func__,
__LINE__, cmd);
> +        ret = -ENOSYS;
> +        break;
> +    }
> +
> +    return ret;
>  }
> 
>  /*
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 2e4b11f..9c95f67 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -82,10 +82,7 @@ int domain_vgic_init(struct domain *d)
>      /* Currently nr_lines in vgic and gic doesn''t have the same
meanings
>       * Here nr_lines = number of SPIs
>       */
> -    if ( d->domain_id == 0 )
> -        d->arch.vgic.nr_lines = gic_number_lines() - 32;
> -    else
> -        d->arch.vgic.nr_lines = 0; /* We don''t need SPIs for
the guest */
> +    d->arch.vgic.nr_lines = gic_number_lines() - 32;
> 
>      d->arch.vgic.shared_irqs >          xzalloc_array(struct
vgic_irq_rank, DOMAIN_NR_RANKS(d));

On 15 July 2013 19:39, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 12 Jul 2013, Eric Trudeau wrote:
>> Patch #2 begins here:
>> --------------------------------------------
>>
>> From 924441c3eddb1765cb4fb0e94f6b62177195837d Mon Sep 17 00:00:00 2001
>> From: Eric Trudeau <etrudeau@broadcom.com>
>> Date: Fri, 12 Jul 2013 14:54:24 -0400
>> Subject: [PATCH] xen/arm: Add support for 1:1 IRQ mapping into guest
domains
>>  on ARM
>>
>> ARM guests will now have the ability to access 1:1 mapped IRQs.
>>
>> The irqs list in the domain configuration file is used.  A SPI irq
>> number must include the offset of 32.  For example, if the device
>> tree irq number is 76 for a SPI, then the irqs field in the dom.cfg
>> file would be:
>>    irqs = [ 108 ]
>>
>> Only level-triggered IRQs are supported at this time.
>>
>> When an IRQ is released on destruction of the guest, any in-progress
>> handlers are given at least a 100 msec to complete.
>
> Overall it''s pretty good patch, but I don''t like the busy
loop. I admit
> that it''s an improvement over what we have today but release_irq
wasn''t
> actually called by anybody until now.
>
>
>> Signed-off-by: Eric Trudeau <etrudeau@broadcom.com>
>> ---
>>  xen/arch/arm/domain.c  | 15 ++++++++++++++
>>  xen/arch/arm/gic.c     | 29 ++++++++++++++++++--------
>>  xen/arch/arm/physdev.c | 56
++++++++++++++++++++++++++++++++++++++++++++++++--
>>  xen/arch/arm/vgic.c    |  5 +----
>>  4 files changed, 91 insertions(+), 14 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 4c434a1..f15ff06 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -31,6 +31,8 @@
>>  #include <asm/gic.h>
>>  #include "vtimer.h"
>>  #include "vpl011.h"
>> +#include <xen/iocap.h>
>> +#include <xen/irq.h>
>>
>>  DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
>>
>> @@ -513,8 +515,21 @@ fail:
>>      return rc;
>>  }
>>
>> +static int release_domain_irqs(struct domain *d)
>> +{
>> +    int i;
>> +    for (i = 0; i <= d->nr_pirqs; i++) {
>> +        if (irq_access_permitted(d, i)) {
>> +            release_irq(i);
>> +        }
>> +    }
>> +    return 0;
>> +}
>> +
>> +
>>  void arch_domain_destroy(struct domain *d)
>>  {
>> +    release_domain_irqs(d);
>>      p2m_teardown(d);
>>      domain_vgic_free(d);
>>      domain_uart0_free(d);
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index ccce565..ed15ec3 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -30,6 +30,7 @@
>>  #include <xen/device_tree.h>
>>  #include <asm/p2m.h>
>>  #include <asm/domain.h>
>> +#include <xen/delay.h>
>>
>>  #include <asm/gic.h>
>>
>> @@ -510,11 +511,12 @@ 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;
>> -   struct irqaction *action;
>> +    struct irqaction *action;
>> +    int    inprogresschecks;
>>
>>      desc = irq_to_desc(irq);
>>
>> @@ -530,8 +532,15 @@ void __init release_irq(unsigned int irq)
>>
>>      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 );
>> +    /* Wait a little while to make sure it''s not being used
on another CPU
>> +     * But not indefinitely, because a guest may have crashed */
>> +    for (inprogresschecks = 0; (inprogresschecks < 100);
inprogresschecks++) {
>> +        smp_mb();
>> +        if ( desc->status & IRQ_INPROGRESS )
>> +            mdelay(1);
>> +        else
>> +            break;
>> +    }
>
> Can we use a timer based watchdog instead to avoid the busy loop?
> Looping for 100ms means wasting a considerable amount of time.
>
>
>>      if (action && action->free_on_release)
>>          xfree(action);
>> @@ -708,6 +717,12 @@ int gic_route_irq_to_guest(struct domain *d, const
struct dt_irq *irq,
>>      spin_lock_irqsave(&desc->lock, flags);
>>      spin_lock(&gic.lock);
>>
>> +    if ( desc->action != NULL )
>> +    {
>> +        retval = -EBUSY;
>> +        goto out;
>> +    }
>>
>>      desc->handler = &gic_guest_irq_type;
>>      desc->status |= IRQ_GUEST;
>>
>> @@ -716,12 +731,10 @@ int gic_route_irq_to_guest(struct domain *d,
const struct dt_irq *irq,
>>      gic_set_irq_properties(irq->irq, level, 1u <<
smp_processor_id(), 0xa0);
>>
>>      retval = __setup_irq(desc, irq->irq, action);
>> -    if (retval) {
>> -        xfree(action);
>> -        goto out;
>> -    }
>>
>>  out:
>> +    if (retval)
>> +        xfree(action);
>>      spin_unlock(&gic.lock);
>>      spin_unlock_irqrestore(&desc->lock, flags);
>>      return retval;
>> diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c
>> index 61b4a18..8d76d11 100644
>> --- a/xen/arch/arm/physdev.c
>> +++ b/xen/arch/arm/physdev.c
>> @@ -9,12 +9,64 @@
>>  #include <xen/lib.h>
>>  #include <xen/errno.h>
>>  #include <asm/hypercall.h>
>> +#include <public/physdev.h>
>> +#include <xen/guest_access.h>
>> +#include <xen/irq.h>
>> +#include <xen/sched.h>
>> +#include <asm/gic.h>
>> +#include <xsm/xsm.h>
>>
>>
>>  int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>  {
>> -    printk("%s %d cmd=%d: not implemented yet\n", __func__,
__LINE__, cmd);
>> -    return -ENOSYS;
>> +    int ret;
>> +
>> +    switch ( cmd )
>> +    {
>> +    case PHYSDEVOP_map_pirq: {
>> +        physdev_map_pirq_t map;
>> +        struct dt_irq irq;
>> +        struct domain *d;
>> +
>> +        ret = -EFAULT;
>> +        if ( copy_from_guest(&map, arg, 1) != 0 )
>> +            break;
>> +
>> +        d = rcu_lock_domain_by_any_id(map.domid);
>> +        if ( d == NULL ) {
>> +            ret = -ESRCH;
>> +            break;
>> +        }
>> +
>> +        ret = xsm_map_domain_pirq(XSM_TARGET, d);
>> +
>> +        if (!ret && (map.pirq >= gic_number_lines()))
>> +            ret = -EINVAL;
>> +
>> +        if (!ret) {
>> +            irq.irq = map.pirq;
>> +            irq.type = DT_IRQ_TYPE_LEVEL_MASK;
>
> You should add a comment here to explain why you are hardcoding level,
> even if it''s just a temporary limitation.
>
>
>> +            ret = gic_route_irq_to_guest(d, &irq,
"GuestIRQ");
>> +            if (!ret)
>
> Code style (even though I admit that we are not following it to the
> letter ourselves). See CODING_STYLE.
>
>
>> +                dprintk(XENLOG_G_INFO, "dom%d:
gic_route_irq_to_guest mapped in IRQ %d\n",
>> +                        d->domain_id, irq.irq);
>> +        }
>> +
>> +        rcu_unlock_domain(d);
>> +
>> +        if (!ret && __copy_to_guest(arg, &map, 1) )
>
> Shouldn''t you be copying back the irq number written by
> gic_route_irq_to_guest in map.irq?
That''s fine because pirq is used to return the IRQ number into the
guest.
Here, as we have 1:1 mapping, the value is same. But in the future, a physical
IRQ will be mapped to a different number into the guest

--
Julien Grall

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@linaro.org]
> Sent: Monday, July 15, 2013 6:08 PM
> To: Stefano Stabellini
> Cc: Eric Trudeau; xen-devel; Stefano.Stabellini@citrix.com; Ian Campbell
> Subject: Re: [Xen-devel] [XenARM] XEN tools for ARM with Virtualization
> Extensions - Patch #2
> 
> On 15 July 2013 19:39, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > On Fri, 12 Jul 2013, Eric Trudeau wrote:
> >> Patch #2 begins here:
> >> --------------------------------------------
> >>
> >> From 924441c3eddb1765cb4fb0e94f6b62177195837d Mon Sep 17 00:00:00
> 2001
> >> From: Eric Trudeau <etrudeau@broadcom.com>
> >> Date: Fri, 12 Jul 2013 14:54:24 -0400
> >> Subject: [PATCH] xen/arm: Add support for 1:1 IRQ mapping into
guest
> domains
> >>  on ARM
> >>
> >> ARM guests will now have the ability to access 1:1 mapped IRQs.
> >>
> >> The irqs list in the domain configuration file is used.  A SPI irq
> >> number must include the offset of 32.  For example, if the device
> >> tree irq number is 76 for a SPI, then the irqs field in the
dom.cfg
> >> file would be:
> >>    irqs = [ 108 ]
> >>
> >> Only level-triggered IRQs are supported at this time.
> >>
> >> When an IRQ is released on destruction of the guest, any
in-progress
> >> handlers are given at least a 100 msec to complete.
> >
> > Overall it''s pretty good patch, but I don''t like the
busy loop. I admit
> > that it''s an improvement over what we have today but
release_irq wasn''t
> > actually called by anybody until now.
> >
> >
> >> Signed-off-by: Eric Trudeau <etrudeau@broadcom.com>
> >> ---
> >>  xen/arch/arm/domain.c  | 15 ++++++++++++++
> >>  xen/arch/arm/gic.c     | 29 ++++++++++++++++++--------
> >>  xen/arch/arm/physdev.c | 56
> ++++++++++++++++++++++++++++++++++++++++++++++++--
> >>  xen/arch/arm/vgic.c    |  5 +----
> >>  4 files changed, 91 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> >> index 4c434a1..f15ff06 100644
> >> --- a/xen/arch/arm/domain.c
> >> +++ b/xen/arch/arm/domain.c
> >> @@ -31,6 +31,8 @@
> >>  #include <asm/gic.h>
> >>  #include "vtimer.h"
> >>  #include "vpl011.h"
> >> +#include <xen/iocap.h>
> >> +#include <xen/irq.h>
> >>
> >>  DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
> >>
> >> @@ -513,8 +515,21 @@ fail:
> >>      return rc;
> >>  }
> >>
> >> +static int release_domain_irqs(struct domain *d)
> >> +{
> >> +    int i;
> >> +    for (i = 0; i <= d->nr_pirqs; i++) {
> >> +        if (irq_access_permitted(d, i)) {
> >> +            release_irq(i);
> >> +        }
> >> +    }
> >> +    return 0;
> >> +}
> >> +
> >> +
> >>  void arch_domain_destroy(struct domain *d)
> >>  {
> >> +    release_domain_irqs(d);
> >>      p2m_teardown(d);
> >>      domain_vgic_free(d);
> >>      domain_uart0_free(d);
> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> >> index ccce565..ed15ec3 100644
> >> --- a/xen/arch/arm/gic.c
> >> +++ b/xen/arch/arm/gic.c
> >> @@ -30,6 +30,7 @@
> >>  #include <xen/device_tree.h>
> >>  #include <asm/p2m.h>
> >>  #include <asm/domain.h>
> >> +#include <xen/delay.h>
> >>
> >>  #include <asm/gic.h>
> >>
> >> @@ -510,11 +511,12 @@ 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;
> >> -   struct irqaction *action;
> >> +    struct irqaction *action;
> >> +    int    inprogresschecks;
> >>
> >>      desc = irq_to_desc(irq);
> >>
> >> @@ -530,8 +532,15 @@ void __init release_irq(unsigned int irq)
> >>
> >>      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
);
> >> +    /* Wait a little while to make sure it''s not being
used on another CPU
> >> +     * But not indefinitely, because a guest may have crashed */
> >> +    for (inprogresschecks = 0; (inprogresschecks < 100);
inprogresschecks++) {
> >> +        smp_mb();
> >> +        if ( desc->status & IRQ_INPROGRESS )
> >> +            mdelay(1);
> >> +        else
> >> +            break;
> >> +    }
> >
> > Can we use a timer based watchdog instead to avoid the busy loop?
> > Looping for 100ms means wasting a considerable amount of time.
> >
Can you kindly point me to an example in the Hypervisor code of a use case
as an example of how I would implement the timer-based watchdog?
> >
> >>      if (action && action->free_on_release)
> >>          xfree(action);
> >> @@ -708,6 +717,12 @@ int gic_route_irq_to_guest(struct domain *d,
const
> struct dt_irq *irq,
> >>      spin_lock_irqsave(&desc->lock, flags);
> >>      spin_lock(&gic.lock);
> >>
> >> +    if ( desc->action != NULL )
> >> +    {
> >> +        retval = -EBUSY;
> >> +        goto out;
> >> +    }
> >>
> >>      desc->handler = &gic_guest_irq_type;
> >>      desc->status |= IRQ_GUEST;
> >>
> >> @@ -716,12 +731,10 @@ int gic_route_irq_to_guest(struct domain *d,
> const struct dt_irq *irq,
> >>      gic_set_irq_properties(irq->irq, level, 1u <<
smp_processor_id(), 0xa0);
> >>
> >>      retval = __setup_irq(desc, irq->irq, action);
> >> -    if (retval) {
> >> -        xfree(action);
> >> -        goto out;
> >> -    }
> >>
> >>  out:
> >> +    if (retval)
> >> +        xfree(action);
> >>      spin_unlock(&gic.lock);
> >>      spin_unlock_irqrestore(&desc->lock, flags);
> >>      return retval;
> >> diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c
> >> index 61b4a18..8d76d11 100644
> >> --- a/xen/arch/arm/physdev.c
> >> +++ b/xen/arch/arm/physdev.c
> >> @@ -9,12 +9,64 @@
> >>  #include <xen/lib.h>
> >>  #include <xen/errno.h>
> >>  #include <asm/hypercall.h>
> >> +#include <public/physdev.h>
> >> +#include <xen/guest_access.h>
> >> +#include <xen/irq.h>
> >> +#include <xen/sched.h>
> >> +#include <asm/gic.h>
> >> +#include <xsm/xsm.h>
> >>
> >>
> >>  int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >>  {
> >> -    printk("%s %d cmd=%d: not implemented yet\n",
__func__, __LINE__,
> cmd);
> >> -    return -ENOSYS;
> >> +    int ret;
> >> +
> >> +    switch ( cmd )
> >> +    {
> >> +    case PHYSDEVOP_map_pirq: {
> >> +        physdev_map_pirq_t map;
> >> +        struct dt_irq irq;
> >> +        struct domain *d;
> >> +
> >> +        ret = -EFAULT;
> >> +        if ( copy_from_guest(&map, arg, 1) != 0 )
> >> +            break;
> >> +
> >> +        d = rcu_lock_domain_by_any_id(map.domid);
> >> +        if ( d == NULL ) {
> >> +            ret = -ESRCH;
> >> +            break;
> >> +        }
> >> +
> >> +        ret = xsm_map_domain_pirq(XSM_TARGET, d);
> >> +
> >> +        if (!ret && (map.pirq >= gic_number_lines()))
> >> +            ret = -EINVAL;
> >> +
> >> +        if (!ret) {
> >> +            irq.irq = map.pirq;
> >> +            irq.type = DT_IRQ_TYPE_LEVEL_MASK;
> >
> > You should add a comment here to explain why you are hardcoding level,
> > even if it''s just a temporary limitation.
> >
> >
> >> +            ret = gic_route_irq_to_guest(d, &irq,
"GuestIRQ");
> >> +            if (!ret)
> >
> > Code style (even though I admit that we are not following it to the
> > letter ourselves). See CODING_STYLE.
> >
> >
> >> +                dprintk(XENLOG_G_INFO, "dom%d:
gic_route_irq_to_guest
> mapped in IRQ %d\n",
> >> +                        d->domain_id, irq.irq);
> >> +        }
> >> +
> >> +        rcu_unlock_domain(d);
> >> +
> >> +        if (!ret && __copy_to_guest(arg, &map, 1) )
> >
> > Shouldn''t you be copying back the irq number written by
> > gic_route_irq_to_guest in map.irq?
> 
> That''s fine because pirq is used to return the IRQ number into the
guest.
> Here, as we have 1:1 mapping, the value is same. But in the future, a
physical
> IRQ will be mapped to a different number into the guest
> 
Correct. gic_route_irq_to_guest() only supports 1:1 mapping.  Later, if non-1:1
mapping is supported the new implementation would use the map.pirq to return
the guest IRQ used.  Presumably, we would have support at that time for
updating the device tree for Linux guests to obtain this "assigned"
IRQ.

On 12 July 2013 20:27, Eric Trudeau <etrudeau@broadcom.com>
wrote:
> Second patch submitted with changes based on comments on first patch.
>
>> > From 551f174c9653def01890fadd2dd769ab8a9acde7 Mon Sep 17 00:00:00
>> 2001
>> > From: Eric Trudeau <etrudeau@broadcom.com>
>> > Date: Thu, 11 Jul 2013 20:03:51 -0400
>> > Subject: [PATCH] Add support for Guest physdev irqs
>> >
>> > ---
>> >  xen/arch/arm/domain.c  | 16 ++++++++++++++++
>> >  xen/arch/arm/gic.c     | 15 ++++++++++-----
>> >  xen/arch/arm/physdev.c | 48
>> ++++++++++++++++++++++++++++++++++++++++++++++--
>> >  xen/arch/arm/vgic.c    |  5 +----
>> >  4 files changed, 73 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> > index 4c434a1..52d3429 100644
>> > --- a/xen/arch/arm/domain.c
>> > +++ b/xen/arch/arm/domain.c
>> > @@ -31,6 +31,8 @@
>> >  #include <asm/gic.h>
>> >  #include "vtimer.h"
>> >  #include "vpl011.h"
>> > +#include <xen/iocap.h>
>> > +#include <xen/irq.h>
>> >
>> >  DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
>> >
>> > @@ -513,8 +515,22 @@ fail:
>> >      return rc;
>> >  }
>> >
>> > +static int release_domain_irqs(struct domain *d)
>> > +{
>> > +    int i;
>> > +    for (i = 0; i <= d->nr_pirqs; i++) {
>> > +        if (irq_access_permitted(d, i)) {
>> > +            release_irq(i);
>> > +        }
>> > +    }
>> > +    return 0;
>> > +}
>>
>> As you may know, release_irq will spin until the flags IRQ_INPROGRESS
>> is unset. This is done my the maintenance handler.
>>
>> Now, let say the domain has crashed and the IRQ is not yet EOIed, then
you
>> will spin forever.
>>
>
> I put a timeout in the IRQ_INPROGRESS check in release_irq() of 100
attempts
> with a 1 msec delay per loop iteration.
If we plan to only use release_irq when a domain is destroyed, this
check is useless,
so it should be removed.

An IRQ stays with the flag IRQ_INPROGRESS until Xen will eoi it.
If the domain has crashed or received an hard shutdown (ie xl
destroy), the IRQ will
remain "inflight" and can never come up again.

You need to check if the IRQ is still inflight and, if yes, eoi it.
>> > +
>> >  void arch_domain_destroy(struct domain *d)
>> >  {
>> > +    if (d->irq_caps != NULL)
>> You don''t need this check.
>> During the domain create, Xen ensures that irq_caps is not NULL.
>>
>> > +        release_domain_irqs(d);
>> >      p2m_teardown(d);
>> >      domain_vgic_free(d);
>> >      domain_uart0_free(d);
>> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> > index cafb681..1f576d1 100644
>> > --- a/xen/arch/arm/gic.c
>> > +++ b/xen/arch/arm/gic.c
>> > @@ -510,7 +510,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;
>> > @@ -522,6 +522,7 @@ void __init release_irq(unsigned int irq)
>> >      action = desc->action;
>> >      desc->action  = NULL;
>> >      desc->status |= IRQ_DISABLED;
>> > +    desc->status &= ~IRQ_GUEST;
>> >
>> >      spin_lock(&gic.lock);
>> >      desc->handler->shutdown(desc);
>> > @@ -707,6 +708,12 @@ int gic_route_irq_to_guest(struct domain *d,
const
>> struct dt_irq *irq,
>> >      spin_lock_irqsave(&desc->lock, flags);
>> >      spin_lock(&gic.lock);
>> >
>> > +    if ( desc->action != NULL )
>> > +    {
>> > +        retval = -EBUSY;
>> > +        goto out;
>> > +    }
>> > +
>> >      desc->handler = &gic_guest_irq_type;
>> >      desc->status |= IRQ_GUEST;
>> >
>> > @@ -715,12 +722,10 @@ int gic_route_irq_to_guest(struct domain *d,
const
>> struct dt_irq *irq,
>> >      gic_set_irq_properties(irq->irq, level, 1u <<
smp_processor_id(), 0xa0);
>> >
>> >      retval = __setup_irq(desc, irq->irq, action);
>> > -    if (retval) {
>> > -        xfree(action);
>> > -        goto out;
>> > -    }
>> >
>> >  out:
>> > +    if (retval)
>> > +        xfree(action);
>> >      spin_unlock(&gic.lock);
>> >      spin_unlock_irqrestore(&desc->lock, flags);
>> >      return retval;
>> > diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c
>> > index 61b4a18..8a5f770 100644
>> > --- a/xen/arch/arm/physdev.c
>> > +++ b/xen/arch/arm/physdev.c
>> > @@ -9,12 +9,56 @@
>> >  #include <xen/lib.h>
>> >  #include <xen/errno.h>
>> >  #include <asm/hypercall.h>
>> > +#include <public/physdev.h>
>> > +#include <xen/guest_access.h>
>> > +#include <xen/irq.h>
>> > +#include <xen/sched.h>
>> > +#include <asm/gic.h>
>> >
>> >
>> >  int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>> >  {
>> > -    printk("%s %d cmd=%d: not implemented yet\n",
__func__, __LINE__,
>> cmd);
>> > -    return -ENOSYS;
>> > +    int ret;
>> > +
>> > +    switch ( cmd )
>> > +    {
>> > +    case PHYSDEVOP_map_pirq: {
>> > +        physdev_map_pirq_t map;
>> > +        struct dt_irq irq;
>> > +        struct domain *d;
>> > +
>> > +        ret = -EFAULT;
>> > +        if ( copy_from_guest(&map, arg, 1) != 0 )
>> > +            break;
>> > +
>> > +        d = rcu_lock_domain_by_any_id(map.domid);
>> > +        if ( d == NULL ) {
>> > +            ret = -ESRCH;
>> > +            break;
>> > +        }
>>
>> Missing sanity check on the map.pirq value.
>>
>
> I added a check for (map.pirq < gic_number_lines()).  This is the sanity
check
> that is done in gic_route_irq().  Should this be less than or equal or are
the
> device tree SPI irq numbers 0-based before they are offset by 32?
Your sanity check looks good to me. gic_number_lines returns the exact
number of IRQ (SPI + SGI + PPI).
>> > +        irq.irq = map.pirq;
>> > +        irq.type = DT_IRQ_TYPE_LEVEL_MASK;
>> > +
>> > +        ret = gic_route_irq_to_guest(d, &irq,
"GuestIRQ");
>>
>> Do you plan to handle non 1:1 IRQ mapping?
>> How does work your the IRQ mapping if the IRQ is already mapped to
dom0?
>>
>
> See comment below about sticking with 1:1 irq mapping.
I think you didn''t answer to the second question. How do you tell to
Xen : "Don''t map
this IRQ in dom0"? Do you manually remove the node from dom0 DTS or do you
have an automatic way?
>> > +        if (!ret)
>> > +            printk("%s gic_route_irq_to_guest added IRQ %d
to dom%d\n",
>> > +                   __FUNCTION__, irq.irq, d->domain_id);
>> > +
>> > +        rcu_unlock_domain(d);
>> > +
>> > +        if (!ret && __copy_to_guest(arg, &map, 1) )
>> > +            ret = -EFAULT;
>> > +        break;
>> > +    }
>> > +
>> > +    default:
>> > +        printk("%s %d cmd=%d: not implemented yet\n",
__func__, __LINE__,
>> cmd);
>> > +        ret = -ENOSYS;
>> > +        break;
>> > +    }
>> > +
>> > +    return ret;
>> >  }
>> >
>> >  /*
>> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> > index 2e4b11f..0ebcdac 100644
>> > --- a/xen/arch/arm/vgic.c
>> > +++ b/xen/arch/arm/vgic.c
>> > @@ -82,10 +82,7 @@ int domain_vgic_init(struct domain *d)
>> >      /* Currently nr_lines in vgic and gic doesn''t have
the same meanings
>> >       * Here nr_lines = number of SPIs
>> >       */
>> > -    if ( d->domain_id == 0 )
>> > -        d->arch.vgic.nr_lines = gic_number_lines() - 32;
>> > -    else
>> > -        d->arch.vgic.nr_lines = 0; /* We don''t need
SPIs for the guest */
>> > +    d->arch.vgic.nr_lines = d->nr_pirqs - 32;
>>
>> If you want to stick on the 1:1 mapping, the best solution
>> is to set "nr_lines to gic_number_lines() - 32" for all the
domains.
>>
>
> I will stick with 1:1 mapping for guest IRQs and use gic_number_lines() -
32.
>
>> --
>> Julien Grall
>
> Patch #2 begins here:
> --------------------------------------------
>
> From 924441c3eddb1765cb4fb0e94f6b62177195837d Mon Sep 17 00:00:00 2001
> From: Eric Trudeau <etrudeau@broadcom.com>
> Date: Fri, 12 Jul 2013 14:54:24 -0400
> Subject: [PATCH] xen/arm: Add support for 1:1 IRQ mapping into guest
domains
>  on ARM
>
> ARM guests will now have the ability to access 1:1 mapped IRQs.
>
> The irqs list in the domain configuration file is used.  A SPI irq
> number must include the offset of 32.  For example, if the device
> tree irq number is 76 for a SPI, then the irqs field in the dom.cfg
> file would be:
>    irqs = [ 108 ]
>
> Only level-triggered IRQs are supported at this time.
>
> When an IRQ is released on destruction of the guest, any in-progress
> handlers are given at least a 100 msec to complete.
>
> Signed-off-by: Eric Trudeau <etrudeau@broadcom.com>
> ---
>  xen/arch/arm/domain.c  | 15 ++++++++++++++
>  xen/arch/arm/gic.c     | 29 ++++++++++++++++++--------
>  xen/arch/arm/physdev.c | 56
++++++++++++++++++++++++++++++++++++++++++++++++--
>  xen/arch/arm/vgic.c    |  5 +----
>  4 files changed, 91 insertions(+), 14 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 4c434a1..f15ff06 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -31,6 +31,8 @@
>  #include <asm/gic.h>
>  #include "vtimer.h"
>  #include "vpl011.h"
> +#include <xen/iocap.h>
> +#include <xen/irq.h>
>
>  DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
>
> @@ -513,8 +515,21 @@ fail:
>      return rc;
>  }
>
> +static int release_domain_irqs(struct domain *d)
> +{
> +    int i;
> +    for (i = 0; i <= d->nr_pirqs; i++) {
> +        if (irq_access_permitted(d, i)) {
> +            release_irq(i);
> +        }
> +    }
> +    return 0;
> +}
> +
> +
>  void arch_domain_destroy(struct domain *d)
>  {
> +    release_domain_irqs(d);
>      p2m_teardown(d);
>      domain_vgic_free(d);
>      domain_uart0_free(d);
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index ccce565..ed15ec3 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -30,6 +30,7 @@
>  #include <xen/device_tree.h>
>  #include <asm/p2m.h>
>  #include <asm/domain.h>
> +#include <xen/delay.h>
>
>  #include <asm/gic.h>
>
> @@ -510,11 +511,12 @@ 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;
> -   struct irqaction *action;
> +    struct irqaction *action;
> +    int    inprogresschecks;
>
>      desc = irq_to_desc(irq);
>
> @@ -530,8 +532,15 @@ void __init release_irq(unsigned int irq)
>
>      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 );
> +    /* Wait a little while to make sure it''s not being used on
another CPU
> +     * But not indefinitely, because a guest may have crashed */
> +    for (inprogresschecks = 0; (inprogresschecks < 100);
inprogresschecks++) {
> +        smp_mb();
> +        if ( desc->status & IRQ_INPROGRESS )
> +            mdelay(1);
> +        else
> +            break;
> +    }
>
>      if (action && action->free_on_release)
>          xfree(action);
> @@ -708,6 +717,12 @@ int gic_route_irq_to_guest(struct domain *d, const
struct dt_irq *irq,
>      spin_lock_irqsave(&desc->lock, flags);
>      spin_lock(&gic.lock);
>
> +    if ( desc->action != NULL )
> +    {
> +        retval = -EBUSY;
> +        goto out;
> +    }
> +
>      desc->handler = &gic_guest_irq_type;
>      desc->status |= IRQ_GUEST;
>
> @@ -716,12 +731,10 @@ int gic_route_irq_to_guest(struct domain *d, const
struct dt_irq *irq,
>      gic_set_irq_properties(irq->irq, level, 1u <<
smp_processor_id(), 0xa0);
>
>      retval = __setup_irq(desc, irq->irq, action);
> -    if (retval) {
> -        xfree(action);
> -        goto out;
> -    }
>
>  out:
> +    if (retval)
> +        xfree(action);
>      spin_unlock(&gic.lock);
>      spin_unlock_irqrestore(&desc->lock, flags);
>      return retval;
> diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c
> index 61b4a18..8d76d11 100644
> --- a/xen/arch/arm/physdev.c
> +++ b/xen/arch/arm/physdev.c
> @@ -9,12 +9,64 @@
>  #include <xen/lib.h>
>  #include <xen/errno.h>
>  #include <asm/hypercall.h>
> +#include <public/physdev.h>
> +#include <xen/guest_access.h>
> +#include <xen/irq.h>
> +#include <xen/sched.h>
> +#include <asm/gic.h>
> +#include <xsm/xsm.h>
>
>
>  int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
> -    printk("%s %d cmd=%d: not implemented yet\n", __func__,
__LINE__, cmd);
> -    return -ENOSYS;
> +    int ret;
> +
> +    switch ( cmd )
> +    {
> +    case PHYSDEVOP_map_pirq: {
> +        physdev_map_pirq_t map;
> +        struct dt_irq irq;
> +        struct domain *d;
> +
> +        ret = -EFAULT;
> +        if ( copy_from_guest(&map, arg, 1) != 0 )
> +            break;
> +
> +        d = rcu_lock_domain_by_any_id(map.domid);
> +        if ( d == NULL ) {
> +            ret = -ESRCH;
> +            break;
> +        }
> +
> +        ret = xsm_map_domain_pirq(XSM_TARGET, d);
> +
> +        if (!ret && (map.pirq >= gic_number_lines()))
> +            ret = -EINVAL;
> +
> +        if (!ret) {
> +            irq.irq = map.pirq;
> +            irq.type = DT_IRQ_TYPE_LEVEL_MASK;
> +
> +            ret = gic_route_irq_to_guest(d, &irq,
"GuestIRQ");
> +            if (!ret)
> +                dprintk(XENLOG_G_INFO, "dom%d: gic_route_irq_to_guest
mapped in IRQ %d\n",
> +                        d->domain_id, irq.irq);
> +        }
> +
> +        rcu_unlock_domain(d);
> +
> +        if (!ret && __copy_to_guest(arg, &map, 1) )
> +            ret = -EFAULT;
> +        break;
> +    }
> +
> +    default:
> +        printk("%s %d cmd=%d: not implemented yet\n", __func__,
__LINE__, cmd);
> +        ret = -ENOSYS;
> +        break;
> +    }
> +
> +    return ret;
>  }
>
>  /*
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 2e4b11f..9c95f67 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -82,10 +82,7 @@ int domain_vgic_init(struct domain *d)
>      /* Currently nr_lines in vgic and gic doesn''t have the same
meanings
>       * Here nr_lines = number of SPIs
>       */
> -    if ( d->domain_id == 0 )
> -        d->arch.vgic.nr_lines = gic_number_lines() - 32;
> -    else
> -        d->arch.vgic.nr_lines = 0; /* We don''t need SPIs for
the guest */
> +    d->arch.vgic.nr_lines = gic_number_lines() - 32;
>
>      d->arch.vgic.shared_irqs >          xzalloc_array(struct
vgic_irq_rank, DOMAIN_NR_RANKS(d));
> --
> 1.8.1.2


--
Julien Grall

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@linaro.org]
> Sent: Monday, July 15, 2013 7:14 PM
> To: Eric Trudeau
> Cc: xen-devel; Stefano.Stabellini@citrix.com; Ian Campbell
> Subject: Re: [Xen-devel] [XenARM] XEN tools for ARM with Virtualization
> Extensions - Patch #2
> 
> On 12 July 2013 20:27, Eric Trudeau <etrudeau@broadcom.com> wrote:
> > Second patch submitted with changes based on comments on first patch.
> >
> >> > From 551f174c9653def01890fadd2dd769ab8a9acde7 Mon Sep 17
00:00:00
> >> 2001
> >> > From: Eric Trudeau <etrudeau@broadcom.com>
> >> > Date: Thu, 11 Jul 2013 20:03:51 -0400
> >> > Subject: [PATCH] Add support for Guest physdev irqs
> >> >
> >> > ---
> >> >  xen/arch/arm/domain.c  | 16 ++++++++++++++++
> >> >  xen/arch/arm/gic.c     | 15 ++++++++++-----
> >> >  xen/arch/arm/physdev.c | 48
> >> ++++++++++++++++++++++++++++++++++++++++++++++--
> >> >  xen/arch/arm/vgic.c    |  5 +----
> >> >  4 files changed, 73 insertions(+), 11 deletions(-)
> >> >
> >> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> >> > index 4c434a1..52d3429 100644
> >> > --- a/xen/arch/arm/domain.c
> >> > +++ b/xen/arch/arm/domain.c
> >> > @@ -31,6 +31,8 @@
> >> >  #include <asm/gic.h>
> >> >  #include "vtimer.h"
> >> >  #include "vpl011.h"
> >> > +#include <xen/iocap.h>
> >> > +#include <xen/irq.h>
> >> >
> >> >  DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
> >> >
> >> > @@ -513,8 +515,22 @@ fail:
> >> >      return rc;
> >> >  }
> >> >
> >> > +static int release_domain_irqs(struct domain *d)
> >> > +{
> >> > +    int i;
> >> > +    for (i = 0; i <= d->nr_pirqs; i++) {
> >> > +        if (irq_access_permitted(d, i)) {
> >> > +            release_irq(i);
> >> > +        }
> >> > +    }
> >> > +    return 0;
> >> > +}
> >>
> >> As you may know, release_irq will spin until the flags
IRQ_INPROGRESS
> >> is unset. This is done my the maintenance handler.
> >>
> >> Now, let say the domain has crashed and the IRQ is not yet EOIed,
then you
> >> will spin forever.
> >>
> >
> > I put a timeout in the IRQ_INPROGRESS check in release_irq() of 100
attempts
> > with a 1 msec delay per loop iteration.
> 
> If we plan to only use release_irq when a domain is destroyed, this
> check is useless,
> so it should be removed.
> 
> An IRQ stays with the flag IRQ_INPROGRESS until Xen will eoi it.
> If the domain has crashed or received an hard shutdown (ie xl
> destroy), the IRQ will
> remain "inflight" and can never come up again.
> 
> You need to check if the IRQ is still inflight and, if yes, eoi it.
>
I will have to research this implementation as I am not familiar with the flow
and functions in the IRQ handler code.  If you have any suggestions, please
let me know.  Is it simply a matter of checking IRQ_INFLIGHT bit in the
desc->status word and then calling a function to EOI the interrupt?
 
> >> > +
> >> >  void arch_domain_destroy(struct domain *d)
> >> >  {
> >> > +    if (d->irq_caps != NULL)
> >> You don''t need this check.
> >> During the domain create, Xen ensures that irq_caps is not NULL.
> >>
> >> > +        release_domain_irqs(d);
> >> >      p2m_teardown(d);
> >> >      domain_vgic_free(d);
> >> >      domain_uart0_free(d);
> >> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> >> > index cafb681..1f576d1 100644
> >> > --- a/xen/arch/arm/gic.c
> >> > +++ b/xen/arch/arm/gic.c
> >> > @@ -510,7 +510,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;
> >> > @@ -522,6 +522,7 @@ void __init release_irq(unsigned int irq)
> >> >      action = desc->action;
> >> >      desc->action  = NULL;
> >> >      desc->status |= IRQ_DISABLED;
> >> > +    desc->status &= ~IRQ_GUEST;
> >> >
> >> >      spin_lock(&gic.lock);
> >> >      desc->handler->shutdown(desc);
> >> > @@ -707,6 +708,12 @@ int gic_route_irq_to_guest(struct domain
*d,
> const
> >> struct dt_irq *irq,
> >> >      spin_lock_irqsave(&desc->lock, flags);
> >> >      spin_lock(&gic.lock);
> >> >
> >> > +    if ( desc->action != NULL )
> >> > +    {
> >> > +        retval = -EBUSY;
> >> > +        goto out;
> >> > +    }
> >> > +
> >> >      desc->handler = &gic_guest_irq_type;
> >> >      desc->status |= IRQ_GUEST;
> >> >
> >> > @@ -715,12 +722,10 @@ int gic_route_irq_to_guest(struct
domain *d,
> const
> >> struct dt_irq *irq,
> >> >      gic_set_irq_properties(irq->irq, level, 1u <<
smp_processor_id(), 0xa0);
> >> >
> >> >      retval = __setup_irq(desc, irq->irq, action);
> >> > -    if (retval) {
> >> > -        xfree(action);
> >> > -        goto out;
> >> > -    }
> >> >
> >> >  out:
> >> > +    if (retval)
> >> > +        xfree(action);
> >> >      spin_unlock(&gic.lock);
> >> >      spin_unlock_irqrestore(&desc->lock, flags);
> >> >      return retval;
> >> > diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c
> >> > index 61b4a18..8a5f770 100644
> >> > --- a/xen/arch/arm/physdev.c
> >> > +++ b/xen/arch/arm/physdev.c
> >> > @@ -9,12 +9,56 @@
> >> >  #include <xen/lib.h>
> >> >  #include <xen/errno.h>
> >> >  #include <asm/hypercall.h>
> >> > +#include <public/physdev.h>
> >> > +#include <xen/guest_access.h>
> >> > +#include <xen/irq.h>
> >> > +#include <xen/sched.h>
> >> > +#include <asm/gic.h>
> >> >
> >> >
> >> >  int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >> >  {
> >> > -    printk("%s %d cmd=%d: not implemented yet\n",
__func__, __LINE__,
> >> cmd);
> >> > -    return -ENOSYS;
> >> > +    int ret;
> >> > +
> >> > +    switch ( cmd )
> >> > +    {
> >> > +    case PHYSDEVOP_map_pirq: {
> >> > +        physdev_map_pirq_t map;
> >> > +        struct dt_irq irq;
> >> > +        struct domain *d;
> >> > +
> >> > +        ret = -EFAULT;
> >> > +        if ( copy_from_guest(&map, arg, 1) != 0 )
> >> > +            break;
> >> > +
> >> > +        d = rcu_lock_domain_by_any_id(map.domid);
> >> > +        if ( d == NULL ) {
> >> > +            ret = -ESRCH;
> >> > +            break;
> >> > +        }
> >>
> >> Missing sanity check on the map.pirq value.
> >>
> >
> > I added a check for (map.pirq < gic_number_lines()).  This is the
sanity check
> > that is done in gic_route_irq().  Should this be less than or equal or
are the
> > device tree SPI irq numbers 0-based before they are offset by 32?
> 
> Your sanity check looks good to me. gic_number_lines returns the exact
> number of IRQ (SPI + SGI + PPI).
> 
> >> > +        irq.irq = map.pirq;
> >> > +        irq.type = DT_IRQ_TYPE_LEVEL_MASK;
> >> > +
> >> > +        ret = gic_route_irq_to_guest(d, &irq,
"GuestIRQ");
> >>
> >> Do you plan to handle non 1:1 IRQ mapping?
> >> How does work your the IRQ mapping if the IRQ is already mapped to
dom0?
> >>
> >
> > See comment below about sticking with 1:1 irq mapping.
> 
> I think you didn''t answer to the second question. How do you tell
to
> Xen : "Don''t map
> this IRQ in dom0"? Do you manually remove the node from dom0 DTS or do
you
> have an automatic way?
The gic_route_irq_to_guest call fails if Dom0 (or any other domain) has the IRQ
routed to
it because of the check for (desc->action != NULL).
I removed the device tree node from the Dom0 device tree.  The use case we have
is
for some devices to be handled (not virtualized) by Dom0 or privileged guest
domains exclusively.
> 
> >> > +        if (!ret)
> >> > +            printk("%s gic_route_irq_to_guest added IRQ
%d to dom%d\n",
> >> > +                   __FUNCTION__, irq.irq, d->domain_id);
> >> > +
> >> > +        rcu_unlock_domain(d);
> >> > +
> >> > +        if (!ret && __copy_to_guest(arg, &map,
1) )
> >> > +            ret = -EFAULT;
> >> > +        break;
> >> > +    }
> >> > +
> >> > +    default:
> >> > +        printk("%s %d cmd=%d: not implemented
yet\n", __func__, __LINE__,
> >> cmd);
> >> > +        ret = -ENOSYS;
> >> > +        break;
> >> > +    }
> >> > +
> >> > +    return ret;
> >> >  }
> >> >
> >> >  /*
> >> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> >> > index 2e4b11f..0ebcdac 100644
> >> > --- a/xen/arch/arm/vgic.c
> >> > +++ b/xen/arch/arm/vgic.c
> >> > @@ -82,10 +82,7 @@ int domain_vgic_init(struct domain *d)
> >> >      /* Currently nr_lines in vgic and gic doesn''t
have the same meanings
> >> >       * Here nr_lines = number of SPIs
> >> >       */
> >> > -    if ( d->domain_id == 0 )
> >> > -        d->arch.vgic.nr_lines = gic_number_lines() - 32;
> >> > -    else
> >> > -        d->arch.vgic.nr_lines = 0; /* We don''t
need SPIs for the guest */
> >> > +    d->arch.vgic.nr_lines = d->nr_pirqs - 32;
> >>
> >> If you want to stick on the 1:1 mapping, the best solution
> >> is to set "nr_lines to gic_number_lines() - 32" for all
the domains.
> >>
> >
> > I will stick with 1:1 mapping for guest IRQs and use
gic_number_lines() - 32.
> >
> >> --
> >> Julien Grall
> >
> > Patch #2 begins here:
> > --------------------------------------------
> >
> > From 924441c3eddb1765cb4fb0e94f6b62177195837d Mon Sep 17 00:00:00
> 2001
> > From: Eric Trudeau <etrudeau@broadcom.com>
> > Date: Fri, 12 Jul 2013 14:54:24 -0400
> > Subject: [PATCH] xen/arm: Add support for 1:1 IRQ mapping into guest
> domains
> >  on ARM
> >
> > ARM guests will now have the ability to access 1:1 mapped IRQs.
> >
> > The irqs list in the domain configuration file is used.  A SPI irq
> > number must include the offset of 32.  For example, if the device
> > tree irq number is 76 for a SPI, then the irqs field in the dom.cfg
> > file would be:
> >    irqs = [ 108 ]
> >
> > Only level-triggered IRQs are supported at this time.
> >
> > When an IRQ is released on destruction of the guest, any in-progress
> > handlers are given at least a 100 msec to complete.
> >
> > Signed-off-by: Eric Trudeau <etrudeau@broadcom.com>
> > ---
> >  xen/arch/arm/domain.c  | 15 ++++++++++++++
> >  xen/arch/arm/gic.c     | 29 ++++++++++++++++++--------
> >  xen/arch/arm/physdev.c | 56
> ++++++++++++++++++++++++++++++++++++++++++++++++--
> >  xen/arch/arm/vgic.c    |  5 +----
> >  4 files changed, 91 insertions(+), 14 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index 4c434a1..f15ff06 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -31,6 +31,8 @@
> >  #include <asm/gic.h>
> >  #include "vtimer.h"
> >  #include "vpl011.h"
> > +#include <xen/iocap.h>
> > +#include <xen/irq.h>
> >
> >  DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
> >
> > @@ -513,8 +515,21 @@ fail:
> >      return rc;
> >  }
> >
> > +static int release_domain_irqs(struct domain *d)
> > +{
> > +    int i;
> > +    for (i = 0; i <= d->nr_pirqs; i++) {
> > +        if (irq_access_permitted(d, i)) {
> > +            release_irq(i);
> > +        }
> > +    }
> > +    return 0;
> > +}
> > +
> > +
> >  void arch_domain_destroy(struct domain *d)
> >  {
> > +    release_domain_irqs(d);
> >      p2m_teardown(d);
> >      domain_vgic_free(d);
> >      domain_uart0_free(d);
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index ccce565..ed15ec3 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -30,6 +30,7 @@
> >  #include <xen/device_tree.h>
> >  #include <asm/p2m.h>
> >  #include <asm/domain.h>
> > +#include <xen/delay.h>
> >
> >  #include <asm/gic.h>
> >
> > @@ -510,11 +511,12 @@ 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;
> > -   struct irqaction *action;
> > +    struct irqaction *action;
> > +    int    inprogresschecks;
> >
> >      desc = irq_to_desc(irq);
> >
> > @@ -530,8 +532,15 @@ void __init release_irq(unsigned int irq)
> >
> >      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 );
> > +    /* Wait a little while to make sure it''s not being used
on another CPU
> > +     * But not indefinitely, because a guest may have crashed */
> > +    for (inprogresschecks = 0; (inprogresschecks < 100);
inprogresschecks++) {
> > +        smp_mb();
> > +        if ( desc->status & IRQ_INPROGRESS )
> > +            mdelay(1);
> > +        else
> > +            break;
> > +    }
> >
> >      if (action && action->free_on_release)
> >          xfree(action);
> > @@ -708,6 +717,12 @@ int gic_route_irq_to_guest(struct domain *d,
const
> struct dt_irq *irq,
> >      spin_lock_irqsave(&desc->lock, flags);
> >      spin_lock(&gic.lock);
> >
> > +    if ( desc->action != NULL )
> > +    {
> > +        retval = -EBUSY;
> > +        goto out;
> > +    }
> > +
> >      desc->handler = &gic_guest_irq_type;
> >      desc->status |= IRQ_GUEST;
> >
> > @@ -716,12 +731,10 @@ int gic_route_irq_to_guest(struct domain *d,
const
> struct dt_irq *irq,
> >      gic_set_irq_properties(irq->irq, level, 1u <<
smp_processor_id(), 0xa0);
> >
> >      retval = __setup_irq(desc, irq->irq, action);
> > -    if (retval) {
> > -        xfree(action);
> > -        goto out;
> > -    }
> >
> >  out:
> > +    if (retval)
> > +        xfree(action);
> >      spin_unlock(&gic.lock);
> >      spin_unlock_irqrestore(&desc->lock, flags);
> >      return retval;
> > diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c
> > index 61b4a18..8d76d11 100644
> > --- a/xen/arch/arm/physdev.c
> > +++ b/xen/arch/arm/physdev.c
> > @@ -9,12 +9,64 @@
> >  #include <xen/lib.h>
> >  #include <xen/errno.h>
> >  #include <asm/hypercall.h>
> > +#include <public/physdev.h>
> > +#include <xen/guest_access.h>
> > +#include <xen/irq.h>
> > +#include <xen/sched.h>
> > +#include <asm/gic.h>
> > +#include <xsm/xsm.h>
> >
> >
> >  int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >  {
> > -    printk("%s %d cmd=%d: not implemented yet\n", __func__,
__LINE__,
> cmd);
> > -    return -ENOSYS;
> > +    int ret;
> > +
> > +    switch ( cmd )
> > +    {
> > +    case PHYSDEVOP_map_pirq: {
> > +        physdev_map_pirq_t map;
> > +        struct dt_irq irq;
> > +        struct domain *d;
> > +
> > +        ret = -EFAULT;
> > +        if ( copy_from_guest(&map, arg, 1) != 0 )
> > +            break;
> > +
> > +        d = rcu_lock_domain_by_any_id(map.domid);
> > +        if ( d == NULL ) {
> > +            ret = -ESRCH;
> > +            break;
> > +        }
> > +
> > +        ret = xsm_map_domain_pirq(XSM_TARGET, d);
> > +
> > +        if (!ret && (map.pirq >= gic_number_lines()))
> > +            ret = -EINVAL;
> > +
> > +        if (!ret) {
> > +            irq.irq = map.pirq;
> > +            irq.type = DT_IRQ_TYPE_LEVEL_MASK;
> > +
> > +            ret = gic_route_irq_to_guest(d, &irq,
"GuestIRQ");
> > +            if (!ret)
> > +                dprintk(XENLOG_G_INFO, "dom%d:
gic_route_irq_to_guest mapped
> in IRQ %d\n",
> > +                        d->domain_id, irq.irq);
> > +        }
> > +
> > +        rcu_unlock_domain(d);
> > +
> > +        if (!ret && __copy_to_guest(arg, &map, 1) )
> > +            ret = -EFAULT;
> > +        break;
> > +    }
> > +
> > +    default:
> > +        printk("%s %d cmd=%d: not implemented yet\n",
__func__, __LINE__,
> cmd);
> > +        ret = -ENOSYS;
> > +        break;
> > +    }
> > +
> > +    return ret;
> >  }
> >
> >  /*
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index 2e4b11f..9c95f67 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -82,10 +82,7 @@ int domain_vgic_init(struct domain *d)
> >      /* Currently nr_lines in vgic and gic doesn''t have the
same meanings
> >       * Here nr_lines = number of SPIs
> >       */
> > -    if ( d->domain_id == 0 )
> > -        d->arch.vgic.nr_lines = gic_number_lines() - 32;
> > -    else
> > -        d->arch.vgic.nr_lines = 0; /* We don''t need SPIs
for the guest */
> > +    d->arch.vgic.nr_lines = gic_number_lines() - 32;
> >
> >      d->arch.vgic.shared_irqs > >         
xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_RANKS(d));
> > --
> > 1.8.1.2
> 
> 
> 
> --
> Julien Grall

On Mon, 15 Jul 2013, Eric Trudeau wrote:
> > -----Original Message-----
> > From: Julien Grall [mailto:julien.grall@linaro.org]
> > Sent: Monday, July 15, 2013 6:08 PM
> > To: Stefano Stabellini
> > Cc: Eric Trudeau; xen-devel; Stefano.Stabellini@citrix.com; Ian
Campbell
> > Subject: Re: [Xen-devel] [XenARM] XEN tools for ARM with
Virtualization
> > Extensions - Patch #2
> > 
> > On 15 July 2013 19:39, Stefano Stabellini
> > <stefano.stabellini@eu.citrix.com> wrote:
> > > On Fri, 12 Jul 2013, Eric Trudeau wrote:
> > >> Patch #2 begins here:
> > >> --------------------------------------------
> > >>
> > >> From 924441c3eddb1765cb4fb0e94f6b62177195837d Mon Sep 17
00:00:00
> > 2001
> > >> From: Eric Trudeau <etrudeau@broadcom.com>
> > >> Date: Fri, 12 Jul 2013 14:54:24 -0400
> > >> Subject: [PATCH] xen/arm: Add support for 1:1 IRQ mapping
into guest
> > domains
> > >>  on ARM
> > >>
> > >> ARM guests will now have the ability to access 1:1 mapped
IRQs.
> > >>
> > >> The irqs list in the domain configuration file is used.  A
SPI irq
> > >> number must include the offset of 32.  For example, if the
device
> > >> tree irq number is 76 for a SPI, then the irqs field in the
dom.cfg
> > >> file would be:
> > >>    irqs = [ 108 ]
> > >>
> > >> Only level-triggered IRQs are supported at this time.
> > >>
> > >> When an IRQ is released on destruction of the guest, any
in-progress
> > >> handlers are given at least a 100 msec to complete.
> > >
> > > Overall it''s pretty good patch, but I don''t
like the busy loop. I admit
> > > that it''s an improvement over what we have today but
release_irq wasn''t
> > > actually called by anybody until now.
> > >
> > >
> > >> Signed-off-by: Eric Trudeau <etrudeau@broadcom.com>
> > >> ---
> > >>  xen/arch/arm/domain.c  | 15 ++++++++++++++
> > >>  xen/arch/arm/gic.c     | 29 ++++++++++++++++++--------
> > >>  xen/arch/arm/physdev.c | 56
> > ++++++++++++++++++++++++++++++++++++++++++++++++--
> > >>  xen/arch/arm/vgic.c    |  5 +----
> > >>  4 files changed, 91 insertions(+), 14 deletions(-)
> > >>
> > >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > >> index 4c434a1..f15ff06 100644
> > >> --- a/xen/arch/arm/domain.c
> > >> +++ b/xen/arch/arm/domain.c
> > >> @@ -31,6 +31,8 @@
> > >>  #include <asm/gic.h>
> > >>  #include "vtimer.h"
> > >>  #include "vpl011.h"
> > >> +#include <xen/iocap.h>
> > >> +#include <xen/irq.h>
> > >>
> > >>  DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
> > >>
> > >> @@ -513,8 +515,21 @@ fail:
> > >>      return rc;
> > >>  }
> > >>
> > >> +static int release_domain_irqs(struct domain *d)
> > >> +{
> > >> +    int i;
> > >> +    for (i = 0; i <= d->nr_pirqs; i++) {
> > >> +        if (irq_access_permitted(d, i)) {
> > >> +            release_irq(i);
> > >> +        }
> > >> +    }
> > >> +    return 0;
> > >> +}
> > >> +
> > >> +
> > >>  void arch_domain_destroy(struct domain *d)
> > >>  {
> > >> +    release_domain_irqs(d);
> > >>      p2m_teardown(d);
> > >>      domain_vgic_free(d);
> > >>      domain_uart0_free(d);
> > >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > >> index ccce565..ed15ec3 100644
> > >> --- a/xen/arch/arm/gic.c
> > >> +++ b/xen/arch/arm/gic.c
> > >> @@ -30,6 +30,7 @@
> > >>  #include <xen/device_tree.h>
> > >>  #include <asm/p2m.h>
> > >>  #include <asm/domain.h>
> > >> +#include <xen/delay.h>
> > >>
> > >>  #include <asm/gic.h>
> > >>
> > >> @@ -510,11 +511,12 @@ 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;
> > >> -   struct irqaction *action;
> > >> +    struct irqaction *action;
> > >> +    int    inprogresschecks;
> > >>
> > >>      desc = irq_to_desc(irq);
> > >>
> > >> @@ -530,8 +532,15 @@ void __init release_irq(unsigned int
irq)
> > >>
> > >>      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 );
> > >> +    /* Wait a little while to make sure it''s not
being used on another CPU
> > >> +     * But not indefinitely, because a guest may have
crashed */
> > >> +    for (inprogresschecks = 0; (inprogresschecks < 100);
inprogresschecks++) {
> > >> +        smp_mb();
> > >> +        if ( desc->status & IRQ_INPROGRESS )
> > >> +            mdelay(1);
> > >> +        else
> > >> +            break;
> > >> +    }
> > >
> > > Can we use a timer based watchdog instead to avoid the busy loop?
> > > Looping for 100ms means wasting a considerable amount of time.
> > >
> 
> Can you kindly point me to an example in the Hypervisor code of a use case
> as an example of how I would implement the timer-based watchdog?
I think that Julien''s suggestion to EOI the irq from here it''s
better.

> > >
> > >>      if (action && action->free_on_release)
> > >>          xfree(action);
> > >> @@ -708,6 +717,12 @@ int gic_route_irq_to_guest(struct domain
*d, const
> > struct dt_irq *irq,
> > >>      spin_lock_irqsave(&desc->lock, flags);
> > >>      spin_lock(&gic.lock);
> > >>
> > >> +    if ( desc->action != NULL )
> > >> +    {
> > >> +        retval = -EBUSY;
> > >> +        goto out;
> > >> +    }
> > >>
> > >>      desc->handler = &gic_guest_irq_type;
> > >>      desc->status |= IRQ_GUEST;
> > >>
> > >> @@ -716,12 +731,10 @@ int gic_route_irq_to_guest(struct
domain *d,
> > const struct dt_irq *irq,
> > >>      gic_set_irq_properties(irq->irq, level, 1u <<
smp_processor_id(), 0xa0);
> > >>
> > >>      retval = __setup_irq(desc, irq->irq, action);
> > >> -    if (retval) {
> > >> -        xfree(action);
> > >> -        goto out;
> > >> -    }
> > >>
> > >>  out:
> > >> +    if (retval)
> > >> +        xfree(action);
> > >>      spin_unlock(&gic.lock);
> > >>      spin_unlock_irqrestore(&desc->lock, flags);
> > >>      return retval;
> > >> diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c
> > >> index 61b4a18..8d76d11 100644
> > >> --- a/xen/arch/arm/physdev.c
> > >> +++ b/xen/arch/arm/physdev.c
> > >> @@ -9,12 +9,64 @@
> > >>  #include <xen/lib.h>
> > >>  #include <xen/errno.h>
> > >>  #include <asm/hypercall.h>
> > >> +#include <public/physdev.h>
> > >> +#include <xen/guest_access.h>
> > >> +#include <xen/irq.h>
> > >> +#include <xen/sched.h>
> > >> +#include <asm/gic.h>
> > >> +#include <xsm/xsm.h>
> > >>
> > >>
> > >>  int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> > >>  {
> > >> -    printk("%s %d cmd=%d: not implemented yet\n",
__func__, __LINE__,
> > cmd);
> > >> -    return -ENOSYS;
> > >> +    int ret;
> > >> +
> > >> +    switch ( cmd )
> > >> +    {
> > >> +    case PHYSDEVOP_map_pirq: {
> > >> +        physdev_map_pirq_t map;
> > >> +        struct dt_irq irq;
> > >> +        struct domain *d;
> > >> +
> > >> +        ret = -EFAULT;
> > >> +        if ( copy_from_guest(&map, arg, 1) != 0 )
> > >> +            break;
> > >> +
> > >> +        d = rcu_lock_domain_by_any_id(map.domid);
> > >> +        if ( d == NULL ) {
> > >> +            ret = -ESRCH;
> > >> +            break;
> > >> +        }
> > >> +
> > >> +        ret = xsm_map_domain_pirq(XSM_TARGET, d);
> > >> +
> > >> +        if (!ret && (map.pirq >=
gic_number_lines()))
> > >> +            ret = -EINVAL;
> > >> +
> > >> +        if (!ret) {
> > >> +            irq.irq = map.pirq;
> > >> +            irq.type = DT_IRQ_TYPE_LEVEL_MASK;
> > >
> > > You should add a comment here to explain why you are hardcoding
level,
> > > even if it''s just a temporary limitation.
> > >
> > >
> > >> +            ret = gic_route_irq_to_guest(d, &irq,
"GuestIRQ");
> > >> +            if (!ret)
> > >
> > > Code style (even though I admit that we are not following it to
the
> > > letter ourselves). See CODING_STYLE.
> > >
> > >
> > >> +                dprintk(XENLOG_G_INFO, "dom%d:
gic_route_irq_to_guest
> > mapped in IRQ %d\n",
> > >> +                        d->domain_id, irq.irq);
> > >> +        }
> > >> +
> > >> +        rcu_unlock_domain(d);
> > >> +
> > >> +        if (!ret && __copy_to_guest(arg, &map,
1) )
> > >
> > > Shouldn''t you be copying back the irq number written by
> > > gic_route_irq_to_guest in map.irq?
> > 
> > That''s fine because pirq is used to return the IRQ number
into the guest.
> > Here, as we have 1:1 mapping, the value is same. But in the future, a
physical
> > IRQ will be mapped to a different number into the guest
> > 
> 
> Correct. gic_route_irq_to_guest() only supports 1:1 mapping.  Later, if
non-1:1
> mapping is supported the new implementation would use the map.pirq to
return
> the guest IRQ used.  Presumably, we would have support at that time for
> updating the device tree for Linux guests to obtain this
"assigned" IRQ.
Thinking more about this, a better interface and more similar to the x86
version of this hypercall would be the following:

map.index is used by the caller to ask for the machine irq to map

map.pirq is used by the caller to ask for the guest irq number to use
(right now map.pirq is going to be equal to map.index or -1, see below)

If map.pirq is < 0, then Xen is free to choose the guest irq number it
prefers. Either way, Xen should update the map.pirq field.


Of course all this would need to be documented :-)

On Tue, 16 Jul 2013, Eric Trudeau wrote:
> > -----Original Message-----
> > From: Julien Grall [mailto:julien.grall@linaro.org]
> > Sent: Monday, July 15, 2013 7:14 PM
> > To: Eric Trudeau
> > Cc: xen-devel; Stefano.Stabellini@citrix.com; Ian Campbell
> > Subject: Re: [Xen-devel] [XenARM] XEN tools for ARM with
Virtualization
> > Extensions - Patch #2
> > 
> > On 12 July 2013 20:27, Eric Trudeau <etrudeau@broadcom.com>
wrote:
> > > Second patch submitted with changes based on comments on first
patch.
> > >
> > >> > From 551f174c9653def01890fadd2dd769ab8a9acde7 Mon Sep 17
00:00:00
> > >> 2001
> > >> > From: Eric Trudeau <etrudeau@broadcom.com>
> > >> > Date: Thu, 11 Jul 2013 20:03:51 -0400
> > >> > Subject: [PATCH] Add support for Guest physdev irqs
> > >> >
> > >> > ---
> > >> >  xen/arch/arm/domain.c  | 16 ++++++++++++++++
> > >> >  xen/arch/arm/gic.c     | 15 ++++++++++-----
> > >> >  xen/arch/arm/physdev.c | 48
> > >> ++++++++++++++++++++++++++++++++++++++++++++++--
> > >> >  xen/arch/arm/vgic.c    |  5 +----
> > >> >  4 files changed, 73 insertions(+), 11 deletions(-)
> > >> >
> > >> > diff --git a/xen/arch/arm/domain.c
b/xen/arch/arm/domain.c
> > >> > index 4c434a1..52d3429 100644
> > >> > --- a/xen/arch/arm/domain.c
> > >> > +++ b/xen/arch/arm/domain.c
> > >> > @@ -31,6 +31,8 @@
> > >> >  #include <asm/gic.h>
> > >> >  #include "vtimer.h"
> > >> >  #include "vpl011.h"
> > >> > +#include <xen/iocap.h>
> > >> > +#include <xen/irq.h>
> > >> >
> > >> >  DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
> > >> >
> > >> > @@ -513,8 +515,22 @@ fail:
> > >> >      return rc;
> > >> >  }
> > >> >
> > >> > +static int release_domain_irqs(struct domain *d)
> > >> > +{
> > >> > +    int i;
> > >> > +    for (i = 0; i <= d->nr_pirqs; i++) {
> > >> > +        if (irq_access_permitted(d, i)) {
> > >> > +            release_irq(i);
> > >> > +        }
> > >> > +    }
> > >> > +    return 0;
> > >> > +}
> > >>
> > >> As you may know, release_irq will spin until the flags
IRQ_INPROGRESS
> > >> is unset. This is done my the maintenance handler.
> > >>
> > >> Now, let say the domain has crashed and the IRQ is not yet
EOIed, then you
> > >> will spin forever.
> > >>
> > >
> > > I put a timeout in the IRQ_INPROGRESS check in release_irq() of
100 attempts
> > > with a 1 msec delay per loop iteration.
> > 
> > If we plan to only use release_irq when a domain is destroyed, this
> > check is useless,
> > so it should be removed.
> > 
> > An IRQ stays with the flag IRQ_INPROGRESS until Xen will eoi it.
> > If the domain has crashed or received an hard shutdown (ie xl
> > destroy), the IRQ will
> > remain "inflight" and can never come up again.
> > 
> > You need to check if the IRQ is still inflight and, if yes, eoi it.
> >
I think that Julien is right: we need to call something similar to
xen/arch/arm/gic.c:maintenance_interrupt. I would refactor the code
there into a separate function that ends up being called by
maintenance_interrupt and release_irq.

> I will have to research this implementation as I am not familiar with the
flow
> and functions in the IRQ handler code.  If you have any suggestions, please
> let me know.  Is it simply a matter of checking IRQ_INFLIGHT bit in the
> desc->status word and then calling a function to EOI the interrupt?
First we need to make sure that the domain is paused and about to be
destroyed otherwise we risk breaking the gic/vgic interface.  Then we
need to check whether the irq is currently inflight, looking at the
inflight queue, see for example
xen/arch/arm/vgic.c:vgic_vcpu_inject_irq.  If the irq is inflight we
need to check whether it''s actually in one of the LR registers or just
in the lr_queue. See xen/arch/arm/gic.c:gic_set_guest_irq to see how the
lr_queue works. If the irq is queued there, just remove it from the
lr_queue, EOI it and remove it from the inflight queue. If the irq is
not present in lr_queue, it means it''s in one of the LR registers. In
that case we can call something similar to maintenance_interrupt to
remove it from the LR, EOI the interrupt and remove it from the inflight
queue.

On Tue, 2013-07-16 at 12:24 +0100, Stefano Stabellini
wrote:
> On Tue, 16 Jul 2013, Eric Trudeau wrote:
> > > -----Original Message-----
> > > From: Julien Grall [mailto:julien.grall@linaro.org]
> > > Sent: Monday, July 15, 2013 7:14 PM
> > > To: Eric Trudeau
> > > Cc: xen-devel; Stefano.Stabellini@citrix.com; Ian Campbell
> > > Subject: Re: [Xen-devel] [XenARM] XEN tools for ARM with
Virtualization
> > > Extensions - Patch #2
> > > 
> > > On 12 July 2013 20:27, Eric Trudeau <etrudeau@broadcom.com>
wrote:
> > > > Second patch submitted with changes based on comments on
first patch.
> > > >
> > > >> > From 551f174c9653def01890fadd2dd769ab8a9acde7 Mon
Sep 17 00:00:00
> > > >> 2001
> > > >> > From: Eric Trudeau <etrudeau@broadcom.com>
> > > >> > Date: Thu, 11 Jul 2013 20:03:51 -0400
> > > >> > Subject: [PATCH] Add support for Guest physdev irqs
> > > >> >
> > > >> > ---
> > > >> >  xen/arch/arm/domain.c  | 16 ++++++++++++++++
> > > >> >  xen/arch/arm/gic.c     | 15 ++++++++++-----
> > > >> >  xen/arch/arm/physdev.c | 48
> > > >> ++++++++++++++++++++++++++++++++++++++++++++++--
> > > >> >  xen/arch/arm/vgic.c    |  5 +----
> > > >> >  4 files changed, 73 insertions(+), 11 deletions(-)
> > > >> >
> > > >> > diff --git a/xen/arch/arm/domain.c
b/xen/arch/arm/domain.c
> > > >> > index 4c434a1..52d3429 100644
> > > >> > --- a/xen/arch/arm/domain.c
> > > >> > +++ b/xen/arch/arm/domain.c
> > > >> > @@ -31,6 +31,8 @@
> > > >> >  #include <asm/gic.h>
> > > >> >  #include "vtimer.h"
> > > >> >  #include "vpl011.h"
> > > >> > +#include <xen/iocap.h>
> > > >> > +#include <xen/irq.h>
> > > >> >
> > > >> >  DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
> > > >> >
> > > >> > @@ -513,8 +515,22 @@ fail:
> > > >> >      return rc;
> > > >> >  }
> > > >> >
> > > >> > +static int release_domain_irqs(struct domain *d)
> > > >> > +{
> > > >> > +    int i;
> > > >> > +    for (i = 0; i <= d->nr_pirqs; i++) {
> > > >> > +        if (irq_access_permitted(d, i)) {
> > > >> > +            release_irq(i);
> > > >> > +        }
> > > >> > +    }
> > > >> > +    return 0;
> > > >> > +}
> > > >>
> > > >> As you may know, release_irq will spin until the flags
IRQ_INPROGRESS
> > > >> is unset. This is done my the maintenance handler.
> > > >>
> > > >> Now, let say the domain has crashed and the IRQ is not
yet EOIed, then you
> > > >> will spin forever.
> > > >>
> > > >
> > > > I put a timeout in the IRQ_INPROGRESS check in release_irq()
of 100 attempts
> > > > with a 1 msec delay per loop iteration.
> > > 
> > > If we plan to only use release_irq when a domain is destroyed,
this
> > > check is useless,
> > > so it should be removed.
> > > 
> > > An IRQ stays with the flag IRQ_INPROGRESS until Xen will eoi it.
> > > If the domain has crashed or received an hard shutdown (ie xl
> > > destroy), the IRQ will
> > > remain "inflight" and can never come up again.
> > > 
> > > You need to check if the IRQ is still inflight and, if yes, eoi
it.
> > >
> 
> I think that Julien is right: we need to call something similar to
> xen/arch/arm/gic.c:maintenance_interrupt. I would refactor the code
> there into a separate function that ends up being called by
> maintenance_interrupt and release_irq.
> 
> 
> > I will have to research this implementation as I am not familiar with
the flow
> > and functions in the IRQ handler code.  If you have any suggestions,
please
> > let me know.  Is it simply a matter of checking IRQ_INFLIGHT bit in
the
> > desc->status word and then calling a function to EOI the interrupt?
> 
> First we need to make sure that the domain is paused and about to be
> destroyed otherwise we risk breaking the gic/vgic interface.  Then we
> need to check whether the irq is currently inflight, looking at the
> inflight queue, see for example
> xen/arch/arm/vgic.c:vgic_vcpu_inject_irq.  If the irq is inflight we
> need to check whether it''s actually in one of the LR registers or
just
> in the lr_queue. See xen/arch/arm/gic.c:gic_set_guest_irq to see how the
> lr_queue works. If the irq is queued there, just remove it from the
> lr_queue, EOI it and remove it from the inflight queue. If the irq is
> not present in lr_queue, it means it''s in one of the LR registers.
In
> that case we can call something similar to maintenance_interrupt to
> remove it from the LR, EOI the interrupt and remove it from the inflight
> queue.
This all sounds plausible to me. I''m not sure we need to worry overly
much about the impact on the guest of pulling an interrupt out from
under it -- that''s not ever going to be a clever thing to do and it
seems like it should be up to the host and guest admin to arrange that
the guest has quiesced the device. If the guest admin won''t co-operate,
well then they get to suffer the consequences. I guess that doesn''t
mean
we shouldn''t try and at least be a bit graceful about it if
it''s easy
enough to do.

The thing to worry about is that the IRQ remains usable at the host
level and can be assigned to someone else etc.

One thing to bare in mind is that when you EOI the interrupt, unless the
guest has dealt with it you might immediately get another, which you may
not currently be set up to handle.

You likely want to make sure it is at least masked at the physical level
or maybe you want to try and ensure you do things in the right order
such that it is routed to the host before you do the EOI. Safer to mask
I think.

Ian.

On 07/19/2013 09:45 AM, Ian Campbell wrote:
> On Tue, 2013-07-16 at 12:24 +0100, Stefano Stabellini wrote:
>> On Tue, 16 Jul 2013, Eric Trudeau wrote:
>>>> -----Original Message-----
>>>> From: Julien Grall [mailto:julien.grall@linaro.org]
>>>> Sent: Monday, July 15, 2013 7:14 PM
>>>> To: Eric Trudeau
>>>> Cc: xen-devel; Stefano.Stabellini@citrix.com; Ian Campbell
>>>> Subject: Re: [Xen-devel] [XenARM] XEN tools for ARM with
Virtualization
>>>> Extensions - Patch #2
>>>>
>>>> On 12 July 2013 20:27, Eric Trudeau
<etrudeau@broadcom.com> wrote:
>>>>> Second patch submitted with changes based on comments on
first patch.
>>>>>
>>>>>>> From 551f174c9653def01890fadd2dd769ab8a9acde7 Mon
Sep 17 00:00:00
>>>>>> 2001
>>>>>>> From: Eric Trudeau <etrudeau@broadcom.com>
>>>>>>> Date: Thu, 11 Jul 2013 20:03:51 -0400
>>>>>>> Subject: [PATCH] Add support for Guest physdev irqs
>>>>>>>
>>>>>>> ---
>>>>>>>  xen/arch/arm/domain.c  | 16 ++++++++++++++++
>>>>>>>  xen/arch/arm/gic.c     | 15 ++++++++++-----
>>>>>>>  xen/arch/arm/physdev.c | 48
>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++--
>>>>>>>  xen/arch/arm/vgic.c    |  5 +----
>>>>>>>  4 files changed, 73 insertions(+), 11 deletions(-)
>>>>>>>
>>>>>>> diff --git a/xen/arch/arm/domain.c
b/xen/arch/arm/domain.c
>>>>>>> index 4c434a1..52d3429 100644
>>>>>>> --- a/xen/arch/arm/domain.c
>>>>>>> +++ b/xen/arch/arm/domain.c
>>>>>>> @@ -31,6 +31,8 @@
>>>>>>>  #include <asm/gic.h>
>>>>>>>  #include "vtimer.h"
>>>>>>>  #include "vpl011.h"
>>>>>>> +#include <xen/iocap.h>
>>>>>>> +#include <xen/irq.h>
>>>>>>>
>>>>>>>  DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
>>>>>>>
>>>>>>> @@ -513,8 +515,22 @@ fail:
>>>>>>>      return rc;
>>>>>>>  }
>>>>>>>
>>>>>>> +static int release_domain_irqs(struct domain *d)
>>>>>>> +{
>>>>>>> +    int i;
>>>>>>> +    for (i = 0; i <= d->nr_pirqs; i++) {
>>>>>>> +        if (irq_access_permitted(d, i)) {
>>>>>>> +            release_irq(i);
>>>>>>> +        }
>>>>>>> +    }
>>>>>>> +    return 0;
>>>>>>> +}
>>>>>>
>>>>>> As you may know, release_irq will spin until the flags
IRQ_INPROGRESS
>>>>>> is unset. This is done my the maintenance handler.
>>>>>>
>>>>>> Now, let say the domain has crashed and the IRQ is not
yet EOIed, then you
>>>>>> will spin forever.
>>>>>>
>>>>>
>>>>> I put a timeout in the IRQ_INPROGRESS check in
release_irq() of 100 attempts
>>>>> with a 1 msec delay per loop iteration.
>>>>
>>>> If we plan to only use release_irq when a domain is destroyed,
this
>>>> check is useless,
>>>> so it should be removed.
>>>>
>>>> An IRQ stays with the flag IRQ_INPROGRESS until Xen will eoi
it.
>>>> If the domain has crashed or received an hard shutdown (ie xl
>>>> destroy), the IRQ will
>>>> remain "inflight" and can never come up again.
>>>>
>>>> You need to check if the IRQ is still inflight and, if yes, eoi
it.
>>>>
>>
>> I think that Julien is right: we need to call something similar to
>> xen/arch/arm/gic.c:maintenance_interrupt. I would refactor the code
>> there into a separate function that ends up being called by
>> maintenance_interrupt and release_irq.
>>
>>
>>> I will have to research this implementation as I am not familiar
with the flow
>>> and functions in the IRQ handler code.  If you have any
suggestions, please
>>> let me know.  Is it simply a matter of checking IRQ_INFLIGHT bit in
the
>>> desc->status word and then calling a function to EOI the
interrupt?
>>
>> First we need to make sure that the domain is paused and about to be
>> destroyed otherwise we risk breaking the gic/vgic interface.  Then we
>> need to check whether the irq is currently inflight, looking at the
>> inflight queue, see for example
>> xen/arch/arm/vgic.c:vgic_vcpu_inject_irq.  If the irq is inflight we
>> need to check whether it''s actually in one of the LR registers
or just
>> in the lr_queue. See xen/arch/arm/gic.c:gic_set_guest_irq to see how
the
>> lr_queue works. If the irq is queued there, just remove it from the
>> lr_queue, EOI it and remove it from the inflight queue. If the irq is
>> not present in lr_queue, it means it''s in one of the LR
registers. In
>> that case we can call something similar to maintenance_interrupt to
>> remove it from the LR, EOI the interrupt and remove it from the
inflight
>> queue.
> 
> This all sounds plausible to me. I''m not sure we need to worry
overly
> much about the impact on the guest of pulling an interrupt out from
> under it -- that''s not ever going to be a clever thing to do and
it
> seems like it should be up to the host and guest admin to arrange that
> the guest has quiesced the device. If the guest admin won''t
co-operate,
> well then they get to suffer the consequences. I guess that
doesn''t mean
> we shouldn''t try and at least be a bit graceful about it if
it''s easy
> enough to do.
> 
> The thing to worry about is that the IRQ remains usable at the host
> level and can be assigned to someone else etc.
Is it enough to check IRQ_INPROGRESS flags and if it''s enabled Xen will
need to EOI the interrupt?

So the code of release_irq could be:
	1) disable/mask the IRQ
        2) if IRQ_INPROGRESS => EOI it on the right CPU
> One thing to bare in mind is that when you EOI the interrupt, unless the
> guest has dealt with it you might immediately get another, which you may
> not currently be set up to handle.
> 
> You likely want to make sure it is at least masked at the physical level
> or maybe you want to try and ensure you do things in the right order
> such that it is routed to the host before you do the EOI. Safer to mask
> I think.
> 
> Ian.
>