Re: [PATCH v6 2/5] xen/arm: inflight irqs during migration

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

Hi Stefano,

On 23/06/14 17:37, Stefano Stabellini wrote:
> @@ -786,9 +837,14 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
>   
>       spin_lock_irqsave(&v->arch.vgic.lock, flags);
>   
> +    set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
> +    /* update QUEUED before MIGRATING */
> +    smp_wmb();
> +    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &n->status) )
> +        goto out;

Why do you kick the current VCPU here? It looks like useless because the migration will take care of it.

> +
>       if ( !list_empty(&n->inflight) )
>       {
> -        set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
>           gic_raise_inflight_irq(v, irq);
>           goto out;
>       }
> @@ -796,6 +852,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
>       /* vcpu offline */
>       if ( test_bit(_VPF_down, &v->pause_flags) )
>       {
> +        clear_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
>           spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>           return;

Rather than setting & clearing the GUEST_QUEUED bit. Wouldn't it be better to move the if (test_bit(_VPF_down,...)) before the spin_lock?

If the VCPU is down here, it will likely be down before. Hence, the lock doesn't protect the pause_flags. This would also make the code clearer.

Regards,

-- 
Julien Grall