Re: [PATCH v2 2/3] xen/arm: vgic: Keep track of vIRQ used by a domain

[Ian Campbell <Ian.Campbell@citrix.com>]

On Mon, 2015-01-19 at 16:14 +0000, Julien Grall wrote:
> >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> >> index b272d86..1a8b3cd 100644
> >> --- a/xen/arch/arm/vgic.c
> >> +++ b/xen/arch/arm/vgic.c
> >> @@ -110,6 +110,15 @@ int domain_vgic_init(struct domain *d)
> >>  
> >>      d->arch.vgic.handler->domain_init(d);
> >>  
> >> +    d->arch.vgic.allocated_irqs =
> >> +        xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_irqs(d)));
> >> +    if ( !d->arch.vgic.allocated_irqs )
> >> +        return -ENOMEM;
> >> +
> >> +    /* vIRQ0-15 (SGIs) are reserved */
> >> +    for ( i = 0; i <= 15; i++ )
> > 
> > ... ; i < NR_GIC_SGI; ...
> 
> I don't really like the idea to use NR_GIC_SGI here. You are assuming
> that SGI is always start from 0.

I think that's pretty much architectural in the GIC.

> I will introduce GIC_SGI_FIRST, GIC_SGI_END and maybe the same for
> PPIs/SPIs.

If GICvN in the future doesn't have SGI_FIRST == 0 then having these
macros in place isn't going to appreciably reduce the amount of stuff
which needs doing, it'll the least of your worries I think..

> > In any case rather than goto can you use a while loop or some other
> > proper looping construct please, something like this ought to work:
> > 
> >      virq = first
> >      while ( (virq = find_next...) < end )
> >      {
> >           if ( test_and_set... )
> >               return virq;
> >           first = virq; /* +1 ? */
> >      }
> > 
> > or perhaps:
> > 
> >      for ( virq = first ; virq = find... ; first = virq )
> >      {
> >           ....
> >      }
> > 
> > I think you might also be able combine virq and first into a single
> > variable? Unless always searching from the beginning is a feature?
> 
> The goal was to avoid race condition with vgic_reserver_virq. In second
> though, we could also avoid to retry ad the raise condition would happen
> in very rare case.

If it is merely rare, rather than impossible, then the code should
handle the possibility not punt it to the caller.

Ian.