Re: [PATCH v7 20/28] xen/arm: ITS: Add virtual ITS availability check helper

[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>
> 
> Introduce vgic_is_lpi_supported() helper function
> to know virtual ITS availability for a domain
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
> v7: - its_enabled field is added to vgic structure
> ---
>  xen/arch/arm/vgic-v2.c       |    2 ++
>  xen/arch/arm/vgic-v3.c       |    3 +++
>  xen/arch/arm/vgic.c          |    5 +++++
>  xen/include/asm-arm/domain.h |    1 +
>  xen/include/asm-arm/vgic.h   |    1 +
>  5 files changed, 12 insertions(+)
> 
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 600d27e..de0cd4b 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -594,6 +594,8 @@ static int vgic_v2_domain_init(struct domain *d)
>      register_mmio_handler(d, &vgic_v2_distr_mmio_handler, d->arch.vgic.dbase,
>                            PAGE_SIZE);
>  
> +    d->arch.vgic.its_enabled = 0;
> +

Not necessary and very confusing.

>      return 0;
>  }
>  
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 52d4277..d99dedb 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1250,6 +1250,9 @@ static int vgic_v3_domain_init(struct domain *d)
>  
>      d->arch.vgic.ctlr = VGICD_CTLR_DEFAULT;
>  
> +    if ( is_hardware_domain(d) && vgic_v3_hw.lpi_support )
> +        d->arch.vgic.its_enabled = 1;

We don't want platform support ITS to use the vITS until everything is
supported in Xen. Otherwise you would break bisection.

This vITS inialization in the vGICv3 driver should be done in one of the
latest patch.

> +
>      return 0;
>  }
>  
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index a5ab99d9..3555833 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -62,6 +62,11 @@ struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq)
>      return vgic_get_rank(v, rank);
>  }
>  
> +bool_t vgic_is_lpi_supported(struct domain *d)
> +{
> +    return d->arch.vgic.its_enabled;
> +}

Please move this helper as a static inline to allow the compiler
inlining the code.

> +
>  static void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq)
>  {
>      INIT_LIST_HEAD(&p->inflight);
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 011f85b..0f6f9d8 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -93,6 +93,7 @@ struct arch_domain
>          int ctlr;
>          int nr_spis; /* Number of SPIs */
>          int nr_lpis; /* Number of LPIs */
> +        bool_t its_enabled;

The field is GICv3 specific and should go within the HAS_GICV3 section
below.

Although, I don't think having this boolean is necessary. You can know
whether we support LPIs or not by checking either the nr_lpis > 0 or
vits != NULL.

The former would be the best if you plan to keep an helper.

>          unsigned long *allocated_irqs; /* bitmap of IRQs allocated */
>          struct vgic_irq_rank *shared_irqs;
>          /*
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 2bf061f..edeec9c 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -309,6 +309,7 @@ enum gic_sgi_mode;
>  
>  #define vgic_num_irq_lines(d)   ((d)->arch.vgic.nr_spis + 32)
>  
> +extern bool_t vgic_is_lpi_supported(struct domain *d);
>  extern int domain_vgic_init(struct domain *d, unsigned int nr_spis);
>  extern void domain_vgic_free(struct domain *d);
>  extern int vcpu_vgic_init(struct vcpu *v);
> 

Regards,

-- 
Julien Grall