Re: [Xen-devel] [PATCH v2 09/21] xen/arm: Release IRQ routed to a domain when it's destroying

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 31 Jul 2014, Julien Grall wrote:
> Xen has to release IRQ routed to a domain in order to reuse later. Currently
> only SPIs can be routed to the guest so we only need to browse SPIs for a
> specific domain.
> 
> Futhermore, a guest can crash and let the IRQ in an incorrect state (i.e has
> not being EOIed). Xen will have to reset the IRQ in order to be able to reuse
> the IRQ later.
> 
> Introduce 2 new functions for release an IRQ routed to a domain:
>     - release_guest_irq: upper level to retrieve the IRQ, call the GIC
>     code and release the action
>     - gic_remove_guest_irq: Check if we can remove the IRQ, and reset
>     it if necessary
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> 
> ---
>     Changes in v2:
>         - Drop the desc->handler = &no_irq_type in release_irq as it's
>         buggy the IRQ is routed to Xen
>         - Add release_guest_irq and gic_remove_guest_irq
> ---
>  xen/arch/arm/gic.c        |   36 ++++++++++++++++++++++++++++++++++
>  xen/arch/arm/irq.c        |   48 
> +++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/vgic.c       |   16 +++++++++++++++
>  xen/include/asm-arm/gic.h |    4 ++++
>  xen/include/asm-arm/irq.h |    2 ++
>  5 files changed, 106 insertions(+)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 8ef8764..22f331a 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -144,6 +144,42 @@ void gic_route_irq_to_guest(struct domain *d, unsigned 
> int virq,
>      p->desc = desc;
>  }
>  
> +/* This function only works with SPIs for now */
> +int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
> +                              struct irq_desc *desc)
> +{
> +    struct pending_irq *p = irq_to_pending(d->vcpu[0], virq);

Use vgic_get_target_vcpu to get the target vcpu of virq. You can pass
d->vcpu[0] as first argument to vgic_get_target_vcpu.
I am sorry that doing that is going to add a dependency on my gic series.


> +    ASSERT(spin_is_locked(&desc->lock));
> +    ASSERT(test_bit(_IRQ_GUEST, &desc->status));
> +    ASSERT(p->desc == desc);
> +
> +    /* If the IRQ is removed when the domain is dying, we only need to
> +     * EOI the IRQ if it has not been done by the guest
> +     */
> +    if ( d->is_dying )
> +    {
> +        desc->handler->shutdown(desc);
> +        if ( test_bit(_IRQ_INPROGRESS, &desc->status) )
> +            gic_hw_ops->deactivate_irq(desc);
> +        clear_bit(_IRQ_INPROGRESS, &desc->status);
> +        goto end;
> +    }
> +
> +    /* TODO: Handle eviction from LRs. For now, deny remove if the IRQ
> +     * is inflight and not disabled.
> +     */
> +    if ( test_bit(_IRQ_INPROGRESS, &desc->status) ||
> +         test_bit(_IRQ_DISABLED, &desc->status) )
> +        return -EBUSY;
> +
> +end:
> +    desc->handler = &no_irq_type;
> +    p->desc = NULL;
> +
> +    return 0;
> +}
> +
>  int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
>                    unsigned int *out_hwirq,
>                    unsigned int *out_type)
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 7eeb8dd..ba33571 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -292,6 +292,7 @@ void release_irq(unsigned int irq, const void *dev_id)
>      if ( !desc->action )
>      {
>          desc->handler->shutdown(desc);
> +
>          clear_bit(_IRQ_GUEST, &desc->status);
>      }


spurious change


> @@ -479,6 +480,53 @@ out:
>      return retval;
>  }
>  
> +int release_guest_irq(struct domain *d, unsigned int virq)
> +{
> +    struct irq_desc *desc;
> +    struct irq_guest *info;
> +    unsigned long flags;
> +    struct pending_irq *p;
> +    int ret;
> +
> +    if ( virq >= vgic_num_irqs(d) )
> +        return -EINVAL;
> +
> +    p = irq_to_pending(d->vcpu[0], virq);
> +    if ( !p->desc )
> +        return -EINVAL;

Same here: call vgic_get_target_vcpu.
Also if this function is supposed to work only with SPIs, you should add
a comment or explicitly check for it.


> +    desc = p->desc;
> +
> +    spin_lock_irqsave(&desc->lock, flags);
> +
> +    ret = -EINVAL;
> +    if ( !test_bit(_IRQ_GUEST, &desc->status) )
> +        goto unlock;
> +
> +    ret = -EINVAL;
> +
> +    info = irq_get_guest_info(desc);
> +    if ( d != info->d )
> +        goto unlock;
> +
> +    ret = gic_remove_irq_from_guest(d, virq, desc);
> +
> +    spin_unlock_irqrestore(&desc->lock, flags);
> +
> +    if ( !ret )
> +    {
> +        release_irq(desc->irq, info);
> +        xfree(info);
> +    }
> +
> +    return ret;
> +
> +unlock:
> +    spin_unlock_irqrestore(&desc->lock, flags);
> +
> +    return ret;
> +}
> +
>  /*
>   * pirq event channels. We don't use these on ARM, instead we use the
>   * features of the GIC to inject virtualised normal interrupts.
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index a037ecc..2a5fc18 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -118,6 +118,22 @@ void register_vgic_ops(struct domain *d, const struct 
> vgic_ops *ops)
>  
>  void domain_vgic_free(struct domain *d)
>  {
> +    int i;
> +    int ret;
> +
> +    for ( i = 0; i < (d->arch.vgic.nr_spis); i++ )
> +    {
> +        struct pending_irq *p = &d->arch.vgic.pending_irqs[i];
> +
> +        if ( p->desc )
> +        {
> +            ret = release_guest_irq(d, p->irq);
> +            if ( ret )
> +                dprintk(XENLOG_G_WARNING, "d%u: Failed to release virq %u 
> ret = %d\n",
> +                        d->domain_id, p->irq, ret);
> +        }
> +    }
> +
>      xfree(d->arch.vgic.shared_irqs);
>      xfree(d->arch.vgic.pending_irqs);
>  }
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 89816cd..46d7fd6 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -201,6 +201,10 @@ extern void gic_route_irq_to_guest(struct domain *, 
> unsigned int virq,
>                                     const cpumask_t *cpu_mask,
>                                     unsigned int priority);
>  
> +/* Remove an IRQ passthrough to a guest */
> +int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
> +                              struct irq_desc *desc);
> +
>  extern void gic_inject(void);
>  extern void gic_clear_pending_irqs(struct vcpu *v);
>  extern int gic_events_need_delivery(void);
> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index a7174f3..d25e1a1 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -44,6 +44,8 @@ void init_secondary_IRQ(void);
>  
>  int route_irq_to_guest(struct domain *d, unsigned int virq,
>                         unsigned int irq, const char *devname);
> +int release_guest_irq(struct domain *d, unsigned int irq);
> +
>  void arch_move_irqs(struct vcpu *v);
>  
>  /* Set IRQ type for an SPI */
> -- 
> 1.7.10.4
> 

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