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 Ian,

On 13/01/15 15:51, Ian Campbell wrote:
> On Fri, 2014-12-12 at 14:43 +0000, Julien Grall wrote:
>> While it's easy to know which hardware IRQ is assigned to a domain, there
>> is no way to know which IRQ is emulated by Xen for a specific domain.
> 
> It seems you are tracking all valid interrupts, including hardware ones,
> not just those for emulated devices? Perhaps rather than emulated you
> meant "allocated to the guest" or "routed" or something?
> 
>> Introduce a bitmap to keep track of every vIRQ used by a domain. This
>> will be used later to find free vIRQ for interrupt device assignment and
>> emulated interrupt.
> 
> Actually, don't you implement the alloc/free of vIRQs here too?

Yes.

> Is there a usecase for tracking SPIs in this way, or would tracking PPIs
> only be sufficient?

We need to track everything for interrupt assignment to a guest/dom0. So
if the guest ask for a free vIRQ we can give it directly.

>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/arch/arm/domain_build.c          |  6 +++
>>  xen/arch/arm/platforms/xgene-storm.c |  4 ++
>>  xen/arch/arm/vgic.c                  | 76 ++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/vtimer.c                | 15 +++++++
>>  xen/include/asm-arm/domain.h         |  1 +
>>  xen/include/asm-arm/vgic.h           | 13 ++++++
>>  6 files changed, 115 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index de180d8..c238c8f 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -968,6 +968,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
> 
> Return and handle EBUSY to distinguish other errors?

vgic_reserve_virq can fail for 2 reasons:
	- The IRQ is too high to handle by the vGIC => Unlikely as DOM0 use the
same IRQ number as the hardware.
	- The vIRQ is already reserved.

The former will be catch just after with route_irq_to_guest. So I don't
think it's worth to change the return from a bool to an int and return
-EBUSY.

> ("try to map the IRQ twice")
> 
>> +         */
>> +        vgic_reserve_virq(d, irq);
>>          res = route_irq_to_guest(d, irq, dt_node_name(dev));
>>          if ( res )
>>          {
>> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
>> index 0b3492d..416d42c 100644
>> --- a/xen/arch/arm/platforms/xgene-storm.c
>> +++ b/xen/arch/arm/platforms/xgene-storm.c
>> @@ -71,6 +71,10 @@ static int map_one_spi(struct domain *d, const char *what,
>>  
>>      printk("Additional IRQ %u (%s)\n", irq, what);
>>  
>> +    if ( !vgic_reserve_virq(d, irq) )
>> +        printk("Failed to reserve the vIRQ %u on dom%d\n",
> 
> Drop "the".

Ok.

>> +               irq, d->domain_id);
>> +
>>      ret = route_irq_to_guest(d, irq, what);
>>      if ( ret )
>>          printk("Failed to route %s to dom%d\n", what, d->domain_id);
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 75cb7ff..dbfc259 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -87,6 +87,8 @@ int domain_vgic_init(struct domain *d)
>>          return -ENODEV;
>>      }
>>  
>> +    spin_lock_init(&d->arch.vgic.lock);
>> +
>>      d->arch.vgic.shared_irqs =
>>          xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_RANKS(d));
>>      if ( d->arch.vgic.shared_irqs == NULL )
>> @@ -107,6 +109,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)));
> 
> (this was why I asked if tracking SPIs was needed...)

To complete my answer above:
	- dom0: vgic_num_irqs() = number of hardware IRQS
	- guest: vgic_num_irqs() = 32.

So we don't waste memory.

> 
>> +    if ( !d->arch.vgic.allocated_irqs )
>> +        return -ENOMEM;
>> +
>> +    /* vIRQ0-15 (SGIs) are reserved */
>> +    for (i = 0; i <= 15; i++)
>> +        set_bit(i, d->arch.vgic.allocated_irqs);
>> +
>>      return 0;
>>  }
>>  
>> @@ -119,6 +130,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)
>> @@ -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;
> 
> EINVAL?

vgic_reserve_irq returns a boolean:
	0 => not reserved
	1 => reserved

I don't see why we should return an int in this case, as the caller
should know how to use it.

>> +    spin_lock(&d->arch.vgic.lock);
>> +    reserved = !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
>> +    spin_unlock(&d->arch.vgic.lock);
>> +
>> +    return reserved;
>> +}
>> +
>> +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);
> 
> I think you could use find_next_zero_bit here to start the search at bit
> 16 and stop at bit 31. Having done so, it might be nicer to if (spi) to
> select min and max IRQs and have the bit manipulation all be common.

I will give a look for the next version.

> 
>> +void vgic_free_virq(struct domain *d, unsigned int virq)
> 
> It only frees spis, but the alloc version can do SPI or PPI. Is that on
> purpose?

I forgot to update vgic_free_virq when I made the support for PPIs.

>> +{
>> +    unsigned int spi;
>> +
>> +    if ( is_hardware_domain(d) )
>> +        return;
>> +
>> +    if ( virq < 32 && virq >= vgic_num_irqs(d) )
>> +        return;
>> +
>> +    spi = virq - 32;
>> +
>> +    /* Taking the vGIC domain lock is not necessary. We don't care if
>> +     * the bit is cleared a bit later. What only matters is bit to 1.
> 
> I don't grok the last sentence here.
> 
>> +     *
>> +     * With this solution vgic_allocate may fail to find an vIRQ if the
>> +     * allocated_irqs is fully. But we don't care.
> 
> are some words missing after fully?

This will be dropped in the next version. So forget this part :).

>> +     */
>> +    clear_bit(spi, d->arch.vgic.allocated_irqs);
>> +}
>> +
>>  /*
>>   * Local variables:
>>   * mode: C
>> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
>> index 2e95ceb..de660bb 100644
>> + */
>> +extern int vgic_allocate_virq(struct domain *d, bool_t spi);
>> +
>> +extern void vgic_free_virq(struct domain *d, unsigned int irq);
>> +
>>  #endif /* __ASM_ARM_VGIC_H__ */
>>  
>>  /*
> 
> 

>> --- a/xen/arch/arm/vtimer.c
>> +++ b/xen/arch/arm/vtimer.c
>> @@ -49,6 +49,21 @@ int domain_vtimer_init(struct domain *d)
>>  {
>>      d->arch.phys_timer_base.offset = NOW();
>>      d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0);
>> +
>> +    /* At this stage vgic_reserve_virq can't fail */
>> +    if ( is_hardware_domain(d) )
>> +    {
>> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI)));
>> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI)));
>> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI)));
>> +    }
>> +    else
>> +    {
>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI));
>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI));
>> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI));
> 
> Although BUG_ON is not conditional on $debug I think we still should
> avoid side effects in the condition.

I know, but this should never fail as it called during on domain
construction. If so we may have some other issue later if we decide to
assign PPI to a guest.

I would prefer to keep the BUG_ON here.

> 
>> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
>> index 74d5a4e..9e167fa 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
>> + *  - spi == 0 => allocate an SGI
> 
> s/== 0/== 1/ and s/SGI/SPI/ in the last line.

I will fix it.

Regards,

-- 
Julien Grall