Re: [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain

[Julien Grall <julien.grall@linaro.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