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

[Julien Grall <julien.grall@linaro.org>]

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@linaro.org>
>>>>
>>>> ---
>>>>     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).

-- 
Julien Grall