From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH v6 2/5] xen/arm: inflight irqs during
	migration
Date: Mon, 23 Jun 2014 21:14:41 +0100
Message-ID: <53A88AB1.2080007@linaro.org>
References: <alpine.DEB.2.02.1406231616481.19982@kaball.uk.xensource.com>
	<1403541463-23734-2-git-send-email-stefano.stabellini@eu.citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1403541463-23734-2-git-send-email-stefano.stabellini@eu.citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Stefano Stabellini <stefano.stabellini@eu.citrix.com>, xen-devel@lists.xensource.com
Cc: julien.grall@citrix.com, Ian.Campbell@citrix.com
List-Id: xen-devel@lists.xenproject.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