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

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

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.

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.


Regards,

-- 
Julien Grall