Re: [PATCH v5 14/22] xen/arm: ITS: Allocate irq descriptors for LPIs

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

Hi Vijay,

On 27/07/15 12:11, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Allocate irq descriptors for LPIs dynamically and
> also update irq_to_pending helper for LPIs

You are mixing irq_desc and pending_irq aspect in the code which made
quite difficult to review.

It would have been helpful to have a patch for allocate irq_desc and
another for pending_irq.

> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index e16fa03..0d17885 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -1038,6 +1038,7 @@ int __init its_init(struct rdist_prop *rdists)
>      struct its_node *its;
>      struct its_node_info *info;
>      struct dt_device_node *np = NULL;
> +    struct irq_desc *desc;
>      uint32_t i, nr_its = 0;
>  
>      static const struct dt_device_match its_device_ids[] __initconst =
> @@ -1079,6 +1080,20 @@ int __init its_init(struct rdist_prop *rdists)
>      its_lpi_init(rdists->id_bits);
>      its_alloc_lpi_tables();
>  
> +    /* Allocate LPI irq descriptors */
> +    irq_desc_lpi = xzalloc_array(struct irq_desc, num_of_lpis);
> +    if ( !irq_desc_lpi )
> +        return -ENOSPC;
> +
> +    for ( i = 0; i < num_of_lpis; i++ )
> +    {
> +       desc = &irq_desc_lpi[i];
> +       init_one_irq_desc(desc);
> +       desc->irq = FIRST_GIC_LPI + i;
> +       desc->arch.type = DT_IRQ_TYPE_EDGE_BOTH;
> +       desc->action = NULL;
> +    }
> +

This code doesn't below to the gic-v3 ITS. This should go either to
irq.c or a new file irq-lpi.c.

But IHMO, there is no harm to build it on ARM, so irq.c would be the
best place. It would avoid a lot of #ifdef HAS_GICV3 in the code and
help when we will support new version of the GIC hardware.

>      return 0;
>  }
>  
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 3b4dea3..98d45bc 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1280,6 +1280,12 @@ static int __init gicv3_init(void)
>                       gicv3.rdist_stride);
>      gicv3_init_v2(node, dbase);
>  
> +    reg = readl_relaxed(GICD + GICD_TYPER);
> +
> +    gicv3.rdist_data.id_bits = ((reg >> GICD_TYPE_ID_BITS_SHIFT) &
> +                                GICD_TYPE_ID_BITS_MASK) + 1;
> +    gicv3_info.nr_id_bits = gicv3.rdist_data.id_bits;
> +

This is not related to IRQ allocation and you make usage of nr_id_bits
in the previous patch.

>      spin_lock_init(&gicv3.lock);
>  
>      spin_lock(&gicv3.lock);
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 85cacb0..63feb43 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -31,6 +31,7 @@
>  static unsigned int local_irqs_type[NR_LOCAL_IRQS];
>  static DEFINE_SPINLOCK(local_irqs_type_lock);
>  
> +irq_desc_t *irq_desc_lpi;

If you would have handle the irq_desc_lpi allocation within this file
you could have remove the static. Which make the exposed "API" more generic.

>  /* Number of LPI supported in XEN */
>  unsigned int num_of_lpis = 8192;
>  
> @@ -64,7 +65,16 @@ static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc);
>  irq_desc_t *__irq_to_desc(int irq)
>  {
>      if (irq < NR_LOCAL_IRQS) return &this_cpu(local_irq_desc)[irq];
> -    return &irq_desc[irq-NR_LOCAL_IRQS];
> +    else if ( irq >= NR_LOCAL_IRQS && irq < NR_IRQS)

First, the irq >= NR_LOCAL_IRQS is not necessary at all your are in the
else of (irq < NR_LOCAL_IRQS).

Secondly, it's weird to use NR_IRQS here. After all, an LPIs is an
IRQ... That's a call for renaming the define.

> +        return &irq_desc[irq-NR_LOCAL_IRQS];
> +#ifdef HAS_GICV3 
> +    else if ( gic_is_lpi(irq) )
> +    {
> +        ASSERT(irq_desc_lpi != NULL);
> +        return &irq_desc_lpi[irq - FIRST_GIC_LPI];
> +    }
> +#endif

I don't think the HAS_GICV3 is necessary (see my point above). If you
really want to keep it, please ensure that you don't have extra cost on
ARM32 by moving the SPI case at the end. I.e

if (irq < NR_LOCAL_IRQS)
  SGIs/PPIs
#ifdef HAS_GICV3
else if ( gic_is_lpi(irq) )
  LPIs
#endif
else
   /* SPIs */

We expect the IRQ to be valid, so it's perfectly fine to optimize given
that the code will heavily be used.

> +    return NULL;
>  }
>  
>  int __init arch_init_one_irq_desc(struct irq_desc *desc)
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 4afb62b..b8f32ed 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -69,6 +69,12 @@ void vits_setup_hw(struct gic_its_info *its_info)
>      vits_hw.info = its_info;
>  }
>  
> +bool_t is_domain_lpi(struct domain *d, unsigned int lpi)
> +{
> +    return ((lpi >= FIRST_GIC_LPI) &&
> +            (lpi < (d->arch.vgic.nr_lpis + FIRST_GIC_LPI)));
> +}
> +

This should go in vgic.c and not vgic-v3-its.c.

>  static inline uint32_t vits_get_max_collections(struct domain *d)
>  {
>      /* Collection ID is only 16 bit */
> @@ -1049,6 +1055,21 @@ int vits_domain_init(struct domain *d)
>  
>      vits = d->arch.vgic.vits;
>  
> +    d->arch.vgic.pending_lpis = xzalloc_array(struct pending_irq,
> +                                              d->arch.vgic.nr_lpis);
> +    if ( d->arch.vgic.pending_lpis == NULL )
> +    {
> +        xfree(d->arch.vgic.vits);
> +        return -ENOMEM;
> +    }
> +
> +    for ( i = 0; i < d->arch.vgic.nr_lpis; i++ )
> +    {
> +        INIT_LIST_HEAD(&d->arch.vgic.pending_lpis[i].inflight);
> +        INIT_LIST_HEAD(&d->arch.vgic.pending_lpis[i].lr_queue);
> +        d->arch.vgic.pending_lpis[i].irq = FIRST_GIC_LPI + i;

Please use vgic_init_pending_irq rather than hardcoding the initialization.

> +    }
> +
>      spin_lock_init(&vits->lock);
>      spin_lock_init(&vits->prop_lock);
>  
> @@ -1056,6 +1077,7 @@ int vits_domain_init(struct domain *d)
>      if ( !vits->collections )
>      {
>          xfree(d->arch.vgic.vits);
> +        xfree(d->arch.vgic.pending_lpis);
>          return -ENOMEM;
>      }
>  
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index a6835a8..ab5e81b 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -30,6 +30,7 @@
>  
>  #include <asm/mmio.h>
>  #include <asm/gic.h>
> +#include <asm/gic-its.h>

I though I said it on the previous version... I'm against of any
inclusion of GIC ITS headers or any ITS call in the vgic.c.

This should be abstract and we should never see #ifdef HAS_GICV3 in the
common vgic code.

>  #include <asm/vgic.h>
>  
>  static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v, int rank)
> @@ -375,13 +376,19 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int
>  
>  struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq)
>  {
> -    struct pending_irq *n;
> +    struct pending_irq *n = NULL;
>      /* Pending irqs allocation strategy: the first vgic.nr_spis irqs
> -     * are used for SPIs; the rests are used for per cpu irqs */
> +     * are used for SPIs; the rests are used for per cpu irqs.
> +     * For LPIs pending_irq structures are allocated separately */
>      if ( irq < 32 )
>          n = &v->arch.vgic.pending_irqs[irq];
> -    else
> +    else if ( irq < vgic_num_irqs(v->domain) )

Another call for renaming, the LPI is also an IRQ... Please change the
name when it's necessary rather than keeping the current one. They may
not be valid after with your LPIs series.


>          n = &v->domain->arch.vgic.pending_irqs[irq - 32];
> +#ifdef HAS_GICV3
> +    else if ( is_domain_lpi(v->domain, irq) )
> +        n = &v->domain->arch.vgic.pending_lpis[irq - FIRST_GIC_LPI];
> +#endif

My remarks in __irq_to_desc are also valid here.

> +    ASSERT(n != NULL);
>      return n;
>  }
>  
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index c4cf65e..3f26d17 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -108,6 +108,7 @@ struct arch_domain
>  #ifdef HAS_GICV3
>          /* Virtual ITS */
>          struct vgic_its *vits;
> +        struct pending_irq *pending_lpis;

I would prefer to see it with pending_irqs above.

>          int gicr_ctlr;
>          /* GIC V3 addressing */
>          /* List of contiguous occupied by the redistributors */
> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index 30316fd..7bb645e 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -21,11 +21,14 @@
>  #include <asm/gic_v3_defs.h>
>  #include <xen/rbtree.h>
>  
> +extern irq_desc_t *irq_desc_lpi;

This is not gic-its specific.

>  /* Number of LPI supported */
>  extern unsigned int num_of_lpis;
>  
>  #define MASK_4K                         0xfffffffff000UL
>  #define MAPC_ITT_IPA_SHIFT              8
> +/* Number of LPIs allocated per domain */
> +#define NR_LPIS                         8192

Ditto.

>  /*
>   * ITS registers, offsets from ITS_base
>   */
> @@ -146,6 +149,8 @@ struct vgic_its
>     unsigned long cmd_qsize;
>     /* ITS mmio physical base */
>     paddr_t gits_base;
> +   /* ITS mmio physical size */
> +   unsigned long gits_size;
>     /* GICR ctrl register */
>     uint32_t ctrl;
>     /* LPI propbase */
> @@ -348,6 +353,7 @@ struct gic_its_info {
>      struct its_node_info *its_hw;
>  };
>  
> +bool_t is_domain_lpi(struct domain *d, unsigned int lpi);

Ditto.

>  int its_init(struct rdist_prop *rdists);
>  int its_cpu_init(void);
>  int vits_get_vitt_entry(struct domain *d, uint32_t devid,
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index f80f291..2f5d5e3 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -303,6 +303,8 @@ struct gic_info {
>      unsigned int maintenance_irq;
>      /* Pointer to the device tree node representing the interrupt controller */
>      const struct dt_device_node *node;
> +    /* Number of IRQ ID bits supported */
> +    uint32_t nr_id_bits;

Care to explain why you need to export it? IHMO, this is not related to
your patch.

Regards,

-- 
Julien Grall