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

["Eric Trudeau" <etrudeau@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@xxxxxxxxxx]
> Sent: Monday, July 15, 2013 7:14 PM
> To: Eric Trudeau
> Cc: xen-devel; Stefano.Stabellini@xxxxxxxxxx; 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> >> > 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@xxxxxxxxxxxx>
> > 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@xxxxxxxxxxxx>
> > ---
> >  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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel