Re: [PATCH v6 17/31] xen/arm: ITS: Store LPIs allocated and IRQ ID bits per domain

[Julien Grall <julien.grall@citrix.com>]

Hi Vijay,

On 31/08/15 12:06, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Store number of lpis and number of id bits
> in vgic structure
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  xen/arch/arm/irq.c           |    9 +++++++++
>  xen/arch/arm/vgic-v3-its.c   |    2 ++
>  xen/arch/arm/vgic.c          |   12 ++++++++++++
>  xen/include/asm-arm/domain.h |    3 +++
>  xen/include/asm-arm/irq.h    |    3 +++
>  5 files changed, 29 insertions(+)
> 
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 24c4f24..93e9411 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -31,6 +31,15 @@
>  static unsigned int local_irqs_type[NR_LOCAL_IRQS];
>  static DEFINE_SPINLOCK(local_irqs_type_lock);
>  
> +/* Number of LPI supported in XEN */
> +/*

Number of LPI supported by Xen.

Also, it's not necessary to have a separate comment for this. It could
be included in the next one. I.e:

/*
 * Number of LPI supported by Xen
 *
 * LPI ....

> + * LPI number start from 8192. Minimum number of bits
> + * required to represent 8192 is 13 bits. So to Support LPIs minimum 
> + * 14 bits are required which can represent maximum LPI 16384.
> + * 16384 - 8192 = 8192. Minimum number of LPIs supported is 8192
> + */

The explanation is hard to understand. I would rewrite like:

"The LPI identifier starts from 8192. Given that the hardware is
providing the number of identifier bits, supporting LPIs requires at
least 14 bits. This will represent 16384 interrupt ID of which 8192
LPIs. So the minimum of LPIs supported when the hardware supports LPIs
is 8192 ".

> +unsigned int nr_lpis = 8192;
> +

I still don't think that it makes sense to introduce the number of LPIs
variable in a patch for vITS. As you describe it, it should be part of
an ITS/GICv3 patch.

Although, I think you should use the field nr_irq_ids in the gic
structure (see patch #10) to avoid problem you currently have with this
solution.

For instance, gic_is_lpi is relying on the number of ID bits exposed by
the hardware but you allocate the IRQ desc for LPIs based on nr_lpis.

It's fine to restrict the value in the GIC compare to hardware.

Furthermore this value should be 0 on platform where LPIs is not
supported in order to make confusion and introduce possible problem with
the code later. I could add that, AFAICT, this new variable (nr_lpis) is
not that often within the code.

Lastly, on a previous version (don't remember which one) we said that
the user should be able to change the value on the command line. What's
your plan for that?

>  /* Describe an IRQ assigned to a guest */
>  struct irq_guest
>  {
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index fabbad0..cef6139 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -547,6 +547,8 @@ int vits_domain_init(struct domain *d)
>  
>      ASSERT(is_hardware_domain(d));
>  
> +    d->arch.vgic.nr_lpis = nr_lpis;
> +

With  my suggestion, this would turn into gic_nr_irq_ids() - 8192;

>      d->arch.vgic.vits = xzalloc(struct vgic_its);
>      if ( !d->arch.vgic.vits )
>          return -ENOMEM;
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index e28c30d..6b6bbce 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -72,8 +72,10 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
>  {
>      int i;
>      int ret;
> +    unsigned int irq_lines;

This will be used as the number of IRQ identifiers which is different as
the number of lines. So please rename into irq_ids.

>  
>      d->arch.vgic.ctlr = 0;
> +    d->arch.vgic.nr_lpis = 0;
>  
>      /* Limit the number of virtual SPIs supported to (1020 - 32) = 988  */
>      if ( nr_spis > (1020 - NR_LOCAL_IRQS) )
> @@ -130,6 +132,16 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
>      for ( i = 0; i < NR_GIC_SGI; i++ )
>          set_bit(i, d->arch.vgic.allocated_irqs);
>  
> +    irq_lines = d->arch.vgic.nr_spis + 32;
> +    /*
> +     * If LPIs are supported, then just overwrite nr_spis
> +     * in computing id_bits.
> +     */
> +    if ( d->arch.vgic.nr_lpis != 0 )
> +       irq_lines = d->arch.vgic.nr_lpis + FIRST_GIC_LPI;
> +

This would be more logic to do:

if ( !d->arch.vgic.nr_lpis )
	irq_ids = d->arch.vgic.nr_spis + 32;
else
	irq_ids = d->arch.vgic.nr_lpis + FIRST_GIC_LPI;

You could also use "min()" if you want to avoid the if/else.

> +    d->arch.vgic.id_bits = get_count_order(irq_lines);
> +
>      return 0;
>  }
>  
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 986a4d6..269e4bb 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -93,6 +93,9 @@ struct arch_domain
>          spinlock_t lock;
>          int ctlr;
>          int nr_spis; /* Number of SPIs */
> +        int nr_lpis; /* Number of LPIs */
> +        /* Number of bits required to represent IRQs(SPIs+LPIs) */
> +        int id_bits;

I'm not entirely sure this is worth to expose it given it's only used in
a single place (setup of the property table) which is called rarely.

>          unsigned long *allocated_irqs; /* bitmap of IRQs allocated */
>          struct vgic_irq_rank *shared_irqs;
>          /*
> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index bddd1ea..ff37234 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -33,6 +33,9 @@ struct msi_desc {
>  #define nr_static_irqs NR_LINE_IRQS
>  #define arch_hwdom_irqs(domid) NR_LINE_IRQS
>  
> +/* Number of LPI supported */
> +extern unsigned int nr_lpis;
> +
>  struct irq_desc;
>  struct irqaction;
>  
> 

Regards,

-- 
Julien Grall