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

[Julien Grall <julien.grall@linaro.org>]

Hi Ian,

On 19/01/15 15:55, Ian Campbell wrote:
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 7221bc8..d0229d1 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -548,6 +548,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags)
>>      else
>>          d->arch.evtchn_irq = platform_dom0_evtchn_ppi();
>>  
>> +    if ( !vgic_reserve_virq(d, d->arch.evtchn_irq) )
>> +        BUG();
>> +
>>      /*
>>       * Virtual UART is only used by linux early printk and decompress code.
>>       * Only use it for the hardware domain because the linux kernel may not
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index c2dcb49..3d4f317 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -970,6 +970,12 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
>>          irq = res;
>>  
>>          DPRINT("irq %u = %u\n", i, irq);
>> +        /*
>> +         * Checking the return of vgic_reserve_virq is not
>> +         * necessary. It should not fail except when we try to map
>> +         * twice the IRQ. This can happen if the IRQ is shared
> 
> "to map the IRQ twice."
> 
> Perhaps also "This can legitimately happen if the ..." (to make it clear
> it is expected).

Sounds better. I will fix it in the next version.

>> 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 will introduce GIC_SGI_FIRST, GIC_SGI_END and maybe the same for
PPIs/SPIs.

> 
>> +        set_bit(i, d->arch.vgic.allocated_irqs);
>> +
>>      return 0;
>>  }
>>  
>> @@ -122,6 +131,7 @@ void domain_vgic_free(struct domain *d)
>>  {
>>      xfree(d->arch.vgic.shared_irqs);
>>      xfree(d->arch.vgic.pending_irqs);
>> +    xfree(d->arch.vgic.allocated_irqs);
>>  }
>>  
>>  int vcpu_vgic_init(struct vcpu *v)
>> @@ -452,6 +462,54 @@ 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;
>> +
>> +    reserved = !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
>> +
>> +    return reserved;
> 
> Can just return !test_and... directly. (I don't think you add anything
> between these in the next patch?)

Yes. It's a left-over of the spinlock solution in V1.

> 
>> +}
>> +
>> +int vgic_allocate_virq(struct domain *d, bool_t spi)
>> +{
>> +    int ret;
>> +    int first, end;
>> +    unsigned int virq;
>> +
>> +retry:
>> +    if ( !spi )
>> +    {
>> +        /* We only allocate PPIs. SGIs are all reserved */
>> +        first = 16;
>> +        end = 32;
>> +    }
>> +    else
>> +    {
>> +        first = 32;
>> +        end = vgic_num_irqs(d);
>> +    }
> 
> I think retry: could be at least here not way above, couldn't it?


Yes.

> 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.

> 
>> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
>> index 74d5a4e..5ddea51 100644
>> --- a/xen/include/asm-arm/vgic.h
>> +++ b/xen/include/asm-arm/vgic.h
>> @@ -199,6 +199,19 @@ extern int vgic_to_sgi(struct vcpu *v, register_t sgir,
>>                         enum gic_sgi_mode irqmode, int virq,
>>                         unsigned long vcpu_mask);
>>  extern void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq);
>> +
>> +/* Reserve a specific guest vIRQ */
>> +extern bool_t vgic_reserve_virq(struct domain *d, unsigned int virq);
>> +
>> +/*
>> + * Allocate a guest VIRQ
>> + *  - spi == 0 => allocate a PPI. It will be the same on every vCPU
> 
> The second sentence makes me think I should somehow call this per-vcpu
> and expect the same value to be returned each time, which isn't true
> (nor possible given the api as it stands). I think you can assume
> familiarity with PPI vs SGI in the context.
> 
> Personally I'd prefer vgic_allocate_{ppi,spi} as a wrapper round a
> common helper over potentially opaque bool arguments to functions.
> Writing "0 /* ppi */" or "1 /* spi */" at the callers would be a
> reasonable alternative if you don't want to do that.

Good point, I will introduce wrappers.

>> + *  - spi == 1 => allocate an SGI
>> + */
> 
> SGI != SPI.

Oops.


Regards,


-- 
Julien Grall