From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of
 vIRQ used by a domain
Date: Mon, 15 Dec 2014 16:07:15 +0000
Message-ID: <548F0733.2020108@linaro.org>
References: <1418395392-30460-1-git-send-email-julien.grall@linaro.org>
	<1418395392-30460-3-git-send-email-julien.grall@linaro.org>
	<alpine.DEB.2.02.1412151527310.30971@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>
Received: from mail6.bemta5.messagelabs.com ([195.245.231.135])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <julien.grall@linaro.org>) id 1Y0YB7-0002n0-Sj
	for xen-devel@lists.xenproject.org; Mon, 15 Dec 2014 16:07:21 +0000
Received: by mail-wi0-f177.google.com with SMTP id l15so9574355wiw.4
	for <xen-devel@lists.xenproject.org>;
	Mon, 15 Dec 2014 08:07:20 -0800 (PST)
In-Reply-To: <alpine.DEB.2.02.1412151527310.30971@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: ian.campbell@citrix.com, tim@xen.org, stefano.stabellini@citrix.com, xen-devel@lists.xenproject.org, parth.dixit@linaro.org, christoffer.dall@linaro.org
List-Id: xen-devel@lists.xenproject.org

Hi Stefano,

On 15/12/14 15:32, Stefano Stabellini wrote:
> On Fri, 12 Dec 2014, Julien Grall wrote:
>> +    spin_lock_init(&d->arch.vgic.lock);
> 
> you should probably explain in the commit message the reason why you are
> making changes to the vgic lock

Actually the domain vgic lock was never used. Only the per-vcpu vgic
lock was in used.

So I don't make any change to the vgic lock. If I don't use it, I will
add a patch to drop it.

>> @@ -441,6 +453,70 @@ int vgic_emulate(struct cpu_user_regs *regs, union hsr hsr)
>>      return v->domain->arch.vgic.handler->emulate_sysreg(regs, hsr);
>>  }
>>  
>> +bool_t vgic_reserve_virq(struct domain *d, unsigned int virq)
>> +{
>> +    bool_t reserved;
>> +
>> +    if ( virq >= vgic_num_irqs(d) )
>> +        return 0;
>> +
>> +    spin_lock(&d->arch.vgic.lock);
>> +    reserved = !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
>> +    spin_unlock(&d->arch.vgic.lock);
> 
> test_and_set_bit is atomic, why do you need to take the lock?

To avoid race condition with vgic_allocate_virq.

Anyway, I will dropped it with your suggestion to implement
vgic_allocate_virq without lock.

[..]

>> +int vgic_allocate_virq(struct domain *d, bool_t spi)
>> +{
>> +    int ret = -1;
>> +    unsigned int virq;
>> +
>> +    spin_lock(&d->arch.vgic.lock);
>> +    if ( !spi )
>> +    {
>> +        virq = find_first_zero_bit(d->arch.vgic.allocated_irqs, 32);
>> +        if ( virq >= 32 )
>> +            goto unlock;
>> +    }
>> +    else
>> +    {
>> +        virq = find_next_zero_bit(d->arch.vgic.allocated_irqs,
>> +                                  32, vgic_num_irqs(d));
>> +        if ( virq >= vgic_num_irqs(d) )
>> +            goto unlock;
>> +    }
>> +
>> +    set_bit(virq, d->arch.vgic.allocated_irqs);
>> +    ret = virq;
>> +
>> +unlock:
>> +    spin_unlock(&d->arch.vgic.lock);
> 
> you might be able to write this function without taking the lock too, by
> using test_and_set_bit and retries:
> 
> retry:
>     virq = find_first_zero_bit;
>     if (test_and_set_bit(virq))
>         goto retry;

I will give a look to it. I will also to limit the number of retry
(maybe to the number of vIRQ) for safety.

Regards,

-- 
Julien Grall