Re: [PATCH v11 03/10] xen/arm: inflight irqs during migration

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

Hi Stefano,

On 08/11/2014 04:23 PM, Stefano Stabellini wrote:
> On Fri, 8 Aug 2014, Julien Grall wrote:
>>> +void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
>>> +{
>>> +    unsigned long flags;
>>> +    struct pending_irq *p = irq_to_pending(old, irq);
>>> +
>>> +    /* nothing to do for virtual interrupts */
>>> +    if ( p->desc == NULL )
>>> +        return;
>>> +
>>> +    /* migration already in progress, no need to do anything */
>>> +    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
>>> +        return;
>>> +
>>> +    spin_lock_irqsave(&old->arch.vgic.lock, flags);
>>> +
>>> +    if ( list_empty(&p->inflight) )
>>> +    {
>>> +        spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
>>> +        return;
>>> +    }
>>
>> NIT: I would create a label unlock below and jump to it. It would avoid
>> at least one of the 3 call to spin_unlock.
> 
> I would rather avoid making this kind of code style changes at this
> stage.

I'm fine with that.

>>> +    /* If the IRQ is still lr_pending, re-inject it to the new vcpu */
>>> +    if ( !list_empty(&p->lr_queue) )
>>> +    {
>>> +        list_del_init(&p->lr_queue);
>>> +        list_del_init(&p->inflight);
>>> +        spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
>>> +        vgic_vcpu_inject_irq(new, irq);
>>
>> Shouldn't we also clear the p->status? At least for consistency?
> 
> We should probably clear GIC_IRQ_GUEST_QUEUED, but in practice it
> doesn't make any difference because vgic_vcpu_inject_irq immediately
> sets it again.
> 
> This is the updated patch with the GIC_IRQ_GUEST_QUEUED clear.

Thanks! I think we should take this one. It may avoid issue later if the
code is changing, such as checking GUEST_QUEUED in different place.

For the updated patch:

Acked-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall