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:
> On 08/06/2014 05:53 PM, Stefano Stabellini wrote:
> > 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's implicitly done by (p->desc == desc). p->desc is only set for SPIs
> assigned to a guest. If desc is NULL, then it will fault a bit later.
> If someone doesn't use this API to route an IRQ then it's his fault.
> 
> Hence, this as been checked in route_irq_guest. I don't think we should
> bother to check again.
> 
> >>>
> >>>> @@ -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)
> 
> Already done in route_irq_to_guest. I don't think we have to add yet
> another assert here.
> 
> > 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)
> 
> This code won't cope with non-SGIs (here PPIs). As PPIs have an irq_desc
> per CPU we will have to loop on every VCPU to unmap it.
> 
> But I doubt we will have PPIs in future, there is more issues to handle
> (such as the number of VCPUs doesn't match the number of physical CPUs).

In that case, why not call the two functions release_guest_spi and
gic_remove_spi_from_guest?

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