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 Wed, 6 Aug 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 08/06/2014 04:49 PM, Stefano Stabellini wrote:
> > 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.
> 
> Why do I need to add vgic_get_target_vcpu? This function is only able to
> handle SPIs which is shared between VCPU.

OK, in that case ASSERT(virq >= 32 && virq < nr_lines). I am fine either way.
Also see below.


> It might be interesting to introduce spi_to_pending function.
> 
> > I am sorry that doing that is going to add a dependency on my gic series.
> 
> I'm already depends on your 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
> 
> Will drop the line.
> 
> > 
> >> @@ -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.
> 
> route_irq_to_guest already check if we are able to route an IRQ or not.
> For non-SPIs the function will bailout.
> 
> So, here, it's impossible to have p->desc set to another value than NULL
> for non-SPIs.
> 
> Or Xen is buggy will likely fail in another place.

If you do:

p = irq_to_pending(d->vcpu[0], virq);

you are actually introducing more code that cannot cope with non-SGIs.
So you should either:

1) esplicitely check for it (add an ASSERT)

2) replace it with code that can cope with non-SGIs, such us
irq_to_pending(vgic_get_target_vcpu(d->vcpu[0], virq), virq)

I would prefer 2), but 1) is also an option

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