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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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

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