Re: [PATCH v7 14/28] xen/arm: ITS: Initialize physical ITS and export lpi support

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

Hi Vijay,

On 18/09/15 14:09, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Initialize physical ITS if HW supports LPIs.
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
> v7: - Export lpi support information to vgic-v3 driver from gic-v3.
>     - Drop gic_lpi_supported() helper function
>     - Add boot param to enable or disable physical ITS
> v6: - Updated lpi_supported gic_info member for GICv2 and GICv3
>     - Introduced helper gic_lpi_supported() and exported
> v5: - Made check of its dt node availability before
>       setting lpi_supported flag
> ---
>  xen/arch/arm/gic-v3.c             |   38 ++++++++++++++++++++++++++++++++++---
>  xen/arch/arm/vgic-v3.c            |    5 ++++-
>  xen/include/asm-arm/gic_v3_defs.h |    4 +++-
>  xen/include/asm-arm/vgic.h        |    2 +-
>  4 files changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index c4c77a7..ac8a0ea 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -55,6 +55,18 @@ static struct {
>  } gicv3;
>  
>  static struct gic_info gicv3_info;
> +/* Enable/Disable ITS support */
> +static bool_t its_enable  = 1;
> +/* Availability of ITS support after successful ITS initialization */
> +static bool_t its_enabled = 0;
> +
> +static void __init parse_its_param(char *s)
> +{
> +    if ( !parse_bool(s) )
> +        its_enable = 0;
> +}
> +
> +custom_param("its", parse_its_param);

Why do you need a command line option to enable/disable the physical ITS?

>  
>  /* per-cpu re-distributor base */
>  DEFINE_PER_CPU(struct rdist, rdist);
> @@ -590,7 +602,7 @@ static void __init gicv3_dist_init(void)
>       * Here we override HW supported number of LPIs and
>       * limit to to LPIs specified in nr_lpis.
>       */
> -    if ( gicv3_dist_supports_lpis() )
> +    if ( its_enabled && gicv3_dist_supports_lpis() )
>          gicv3_info.nr_irq_ids = nr_lpis + FIRST_GIC_LPI;
>      else
>      {
> @@ -714,6 +726,10 @@ static int __cpuinit gicv3_cpu_init(void)
>      if ( gicv3_enable_redist() )
>          return -ENODEV;
>  
> +    /* Give LPIs a spin */
> +    if ( its_enabled && gicv3_dist_supports_lpis() )

If the ITS is not enabled, the list of ITS nodes will be empty and
therefore its_cpu_init will return directly.

So its_enabled is not necessary.

> +        its_cpu_init();
> +
>      /* Set priority on PPI and SGI interrupts */
>      priority = (GIC_PRI_IPI << 24 | GIC_PRI_IPI << 16 | GIC_PRI_IPI << 8 |
>                  GIC_PRI_IPI);
> @@ -1303,14 +1319,30 @@ static int __init gicv3_init(void)
>                 i, r->base, r->base + r->size);
>      }
>  
> -    vgic_v3_setup_hw(dbase, gicv3.rdist_count, gicv3.rdist_regions,
> -                     gicv3.rdist_stride);
> +    reg = readl_relaxed(GICD + GICD_TYPER);
> +
> +    gicv3.rdist_data.id_bits = ((reg >> GICD_TYPE_ID_BITS_SHIFT) &
> +                                GICD_TYPE_ID_BITS_MASK) + 1;
> +
>      gicv3_init_v2(node, dbase);
>  
>      spin_lock_init(&gicv3.lock);
>  
>      spin_lock(&gicv3.lock);
>  
> +    if ( its_enable && gicv3_dist_supports_lpis() )
> +    {
> +        /*
> +         * LPI support is enabled only if HW supports it and
> +         * ITS dt node is available
> +         */
> +        if ( !its_init(&gicv3.rdist_data) )
> +            its_enabled = 1;
> +    }
> +
> +    vgic_v3_setup_hw(dbase, gicv3.rdist_count, gicv3.rdist_regions,
> +                     gicv3.rdist_stride, its_enabled);
> +

Why do you need to execute all this new code with the gicv3.lock taken?
AFAICT there is no use of GICv3 registers inside the ITS initialization.

>      gicv3_dist_init();
>      res = gicv3_cpu_init();
>      gicv3_hyp_init();
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 12c5d87..52d4277 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -51,6 +51,8 @@
>  
>  static struct {
>      bool_t enabled;
> +    /* Check if its supported */
> +    bool_t lpi_support;

While ITS means LPIs is supported, the invert is not true.
Can you please rename this variable to its_enabled.

>      /* Distributor interface address */
>      paddr_t dbase;
>      /* Re-distributor regions */
> @@ -62,9 +64,10 @@ static struct {
>  void vgic_v3_setup_hw(paddr_t dbase,
>                        unsigned int nr_rdist_regions,
>                        const struct rdist_region *regions,
> -                      uint32_t rdist_stride)
> +                      uint32_t rdist_stride, bool_t lpi_support)

Ditto.

>  {
>      vgic_v3_hw.enabled = 1;
> +    vgic_v3_hw.lpi_support = lpi_support;

Ditto.

>      vgic_v3_hw.dbase = dbase;
>      vgic_v3_hw.nr_rdist_regions = nr_rdist_regions;
>      vgic_v3_hw.regions = regions;
> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
> index 1bc88f6..f819589 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -46,7 +46,9 @@
>  #define GICC_SRE_EL2_ENEL1           (1UL << 3)
>  
>  /* Additional bits in GICD_TYPER defined by GICv3 */
> -#define GICD_TYPE_ID_BITS_SHIFT 19
> +#define GICD_TYPE_ID_BITS_SHIFT      (19)
> +#define GICD_TYPE_ID_BITS_MASK       (0x1f)
> +#define GICD_TYPE_LPIS               (0x1UL << 17)


Please correct the typo in the name rather than keeping it everywhere. I.e

s/TYPE/TYPER/

Also, you've introduced GICD_TYPER_LPIS_SUPPORTED in patch #5 which does
exactly the same things as GICD_TYPE_LPIS. Please use it rather
introducing a new one.

>  
>  #define GICD_TYPER_LPIS_SUPPORTED    (1U << 17)
>  #define GICD_CTLR_RWP                (1UL << 31)
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index cf5a790..2bf061f 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -362,7 +362,7 @@ struct rdist_region;
>  void vgic_v3_setup_hw(paddr_t dbase,
>                        unsigned int nr_rdist_regions,
>                        const struct rdist_region *regions,
> -                      uint32_t rdist_stride);
> +                      uint32_t rdist_stride, bool_t lpi_support);

Ditto.

>  #endif
>  
>  #endif /* __ASM_ARM_VGIC_H__ */
> 

Regards,


-- 
Julien Grall