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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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

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@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;
> +    }

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

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