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

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 15 July 2013 19:39, Stefano Stabellini
<stefano.stabellini@xxxxxxxxxxxxx> 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@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?

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

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