Re: [PATCH v5 18/22] xen/arm: ITS: Add domain specific ITS initialization

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

Hi Vijay,

This patch doesn't only do initialization but also destruction of vGIC info.

Although, a part of the domain initialization is done in part in #8 and 
#8. It's very confusing to see the initialization split in 3 different 
patch.

I would prefer to see the initialization/destruction in a single patch. 
It would help to catch whether you forgot to see you forgot some bits.

On 27/07/2015 04:12, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>
> Add Domain and vcpu specific ITS initialization
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>   xen/arch/arm/vgic-v3-its.c    |   10 ++++++++++
>   xen/arch/arm/vgic-v3.c        |   10 ++++++++++
>   xen/arch/arm/vgic.c           |    3 +++
>   xen/include/asm-arm/gic-its.h |    2 ++
>   xen/include/asm-arm/vgic.h    |    3 +++
>   5 files changed, 28 insertions(+)
>
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 5323192..e182cee 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -44,6 +44,7 @@
>   #define GITS_PIDR4_VAL               (0x04)
>   #define GITS_BASER_INIT_VAL          ((1UL << GITS_BASER_TYPE_SHIFT) | \
>                                        (0x7UL << GITS_BASER_ENTRY_SIZE_SHIFT))
> +//#define DEBUG_ITS

Why here?


>   #ifdef DEBUG_ITS
>   # define DPRINTK(fmt, args...) dprintk(XENLOG_DEBUG, fmt, ##args)
>   #else
> @@ -1122,6 +1123,15 @@ int vits_domain_init(struct domain *d)
>       return 0;
>   }
>
> +void vits_domain_free(struct domain *d)
> +{
> +   free_xenheap_pages(d->arch.vgic.vits->prop_page,
> +                       get_order_from_bytes(d->arch.vgic.vits->prop_size));
> +   xfree(d->arch.vgic.pending_lpis);
> +   xfree(d->arch.vgic.vits->collections);
> +   xfree(d->arch.vgic.vits);

For instance, you add the initialization of these fields in patch #8 and 
#10 but free everything here. I have to go in the others patch to get if 
you forgot something or not.

> +}
> +
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 9e6e3ff..a09ba36 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1299,12 +1299,22 @@ static int vgic_v3_domain_init(struct domain *d)
>
>       d->arch.vgic.ctlr = VGICD_CTLR_DEFAULT;
>
> +    if ( is_hardware_domain(d) && gic_lpi_supported() )

I think gic_lpi_supported is badly name. vITS should only be supported 
when ITS is present, not when LPIs is present.

And I know that you unset LPIs when ITS is not present. But this confuse 
the reader to do this.

> +        vits_domain_init(d);
> +
>       return 0;
>   }
>
> +void vgic_v3_domain_free(struct domain *d)
> +{
> +    if ( is_hardware_domain(d) && gic_lpi_supported() )
> +        vits_domain_free(d);
> +}
> +
>   static const struct vgic_ops v3_ops = {
>       .vcpu_init   = vgic_v3_vcpu_init,
>       .domain_init = vgic_v3_domain_init,
> +    .domain_free = vgic_v3_domain_free,
>       .get_irq_priority = vgic_v3_get_irq_priority,
>       .get_target_vcpu  = vgic_v3_get_target_vcpu,
>       .emulate_sysreg  = vgic_v3_emulate_sysreg,
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 57c0f52..e2bfdb6 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -166,6 +166,9 @@ 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);
> +
> +    if ( !d->arch.vgic.handler->domain_free )

The test is wrong. If domain_free is NULL you will end up to dereference 
the pointer and crash.

> +        d->arch.vgic.handler->domain_free(d);
>   }
>
>   int vcpu_vgic_init(struct vcpu *v)
> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index 870c9a8..da689a4 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -367,6 +367,8 @@ int its_cpu_init(void);
>   void its_set_lpi_properties(struct irq_desc *desc,
>                               const cpumask_t *cpu_mask,
>                               unsigned int priority);
> +int vits_domain_init(struct domain *d);
> +void vits_domain_free(struct domain *d);
>   int vits_get_vitt_entry(struct domain *d, uint32_t devid,
>                           uint32_t event, struct vitt *entry);
>   int vits_get_vdevice_entry(struct domain *d, uint32_t devid,
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index b11faa0..853df04 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -114,6 +114,8 @@ struct vgic_ops {
>       int (*vcpu_init)(struct vcpu *v);
>       /* Domain specific initialization of vGIC */
>       int (*domain_init)(struct domain *d);
> +    /* Free domain specific resources */
> +    void (*domain_free)(struct domain *d);
>       /* Get priority for a given irq stored in vgic structure */
>       int (*get_irq_priority)(struct vcpu *v, unsigned int irq);
>       /* Get the target vcpu for a given virq. The rank lock is already taken
> @@ -191,6 +193,7 @@ enum gic_sgi_mode;
>   #define vgic_num_irqs(d)        ((d)->arch.vgic.nr_spis + 32)
>
>   extern int domain_vgic_init(struct domain *d, unsigned int nr_spis);
> +extern void vgic_its_init(void);

Where does it come from?

>   extern void domain_vgic_free(struct domain *d);
>   extern int vcpu_vgic_init(struct vcpu *v);
>   extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq);
>

Regards,

-- 
Julien Grall