From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH v11 03/10] xen/arm: inflight irqs during
	migration
Date: Wed, 13 Aug 2014 16:35:25 +0100
Message-ID: <53EB85BD.30909@linaro.org>
References: <alpine.DEB.2.02.1408081806180.2293@kaball.uk.xensource.com>
	<1407518033-10694-3-git-send-email-stefano.stabellini@eu.citrix.com>
	<53E508C8.6080708@linaro.org>
	<alpine.DEB.2.02.1408111613500.2293@kaball.uk.xensource.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: <alpine.DEB.2.02.1408111613500.2293@kaball.uk.xensource.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>
Cc: julien.grall@citrix.com, xen-devel@lists.xensource.com, Ian.Campbell@citrix.com
List-Id: xen-devel@lists.xenproject.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