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

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

On Thu, 2015-01-15 at 20:23 +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 vIRQ is allocated by Xen for a specific domain.
> 
> 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.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v2:
>         - Remove lock
>         - Update the prototype of vgic_free_irq to actually show we are
>         taking in parameter a vIRQ and and IRQ
>         - vgic_free_virq was completely buggy. Rewrite it
>         - The usage of find_next_zero_bit was buggy. I don't know how it
>         was working before.
>         - Make find_next_zero_bit common for SPIs and PPIs
>         - Use if (...) BUG() rather than BUG_ON()
>         - Fix comments and some printk message
>         - Update the commit message
>         - Add missing vgic_reserve_irq for the event channel PPI
> ---
>  xen/arch/arm/domain.c                |  3 ++
>  xen/arch/arm/domain_build.c          |  6 ++++
>  xen/arch/arm/platforms/xgene-storm.c |  4 +++
>  xen/arch/arm/vgic.c                  | 58 ++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/vtimer.c                | 25 ++++++++++++++++
>  xen/include/asm-arm/domain.h         |  1 +
>  xen/include/asm-arm/vgic.h           | 13 ++++++++
>  7 files changed, 110 insertions(+)
> 
> 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).

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

> +        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?)

> +}
> +
> +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?

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?

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

> + *  - spi == 1 => allocate an SGI
> + */

SGI != SPI.

> +extern int vgic_allocate_virq(struct domain *d, bool_t spi);
> +
> +extern void vgic_free_virq(struct domain *d, unsigned int virq);
> +
>  #endif /* __ASM_ARM_VGIC_H__ */
>  
>  /*