Re: [RFC PATCH v1 03/10] xen/arm: move vgic data to vgic driver

[Julien Grall <julien.grall@linaro.org>]

Hello Vijay,

Thank you for your patch.

On 03/19/2014 02:17 PM, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> vgic_irq_rank structure contains gic specific
> data elements. Move this out of domain.h and
> allocate memory dynamically in vgic driver

I think we should have a vcpu_vgic_v2 structure which contains
everything related to GICv2 in it, e.g:
	- VGIC rank
        - CTLR
        - cbase, dbase,...

And a domain_vgic_v2 for everything common with all VCPUs.

> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  xen/arch/arm/vgic.c          |   35 ++++++++++++++++++++++++++++-------
>  xen/include/asm-arm/domain.h |   13 ++-----------
>  2 files changed, 30 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index d2a13fb..694a15c 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -35,6 +35,15 @@
>  /* Number of ranks of interrupt registers for a domain */
>  #define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_lines+31)/32)
>  
> +/* Represents state corresponding to a block of 32 interrupts */
> +struct vgic_irq_rank {
> +    spinlock_t lock; /* Covers access to all other members of this struct */
> +    uint32_t ienable, iactive, ipend, pendsgi;
> +    uint32_t icfg[2];
> +    uint32_t ipriority[8];
> +    uint32_t itargets[8];
> +};
> +

I would move the definition in a vgic_v2.h

>  /*
>   * Rank containing GICD_<FOO><n> for GICD_<FOO> with
>   * <b>-bits-per-interrupt
> @@ -66,9 +75,10 @@ static struct vgic_irq_rank *vgic_irq_rank(struct vcpu *v, int b, int n)
>      int rank = REG_RANK_NR(b, n);
>  
>      if ( rank == 0 )
> -        return &v->arch.vgic.private_irqs;
> +        return (struct vgic_irq_rank *)v->arch.vgic.private_irqs;
>      else if ( rank <= DOMAIN_NR_RANKS(v->domain) )
> -        return &v->domain->arch.vgic.shared_irqs[rank - 1];
> +       return (struct vgic_irq_rank *)((unsigned char *)(v->domain->arch.vgic.shared_irqs)
> +                           + (sizeof(struct vgic_irq_rank) *(rank - 1)));

It's so ugly, can you use temporary variable?

>      else
>          return NULL;
>  }
> @@ -82,9 +92,11 @@ void domain_vgic_free(struct domain *d)
>  int vcpu_vgic_init(struct vcpu *v)
>  {
>      int i;
> -    memset(&v->arch.vgic.private_irqs, 0, sizeof(v->arch.vgic.private_irqs));
> +    struct vgic_irq_rank *vir;
>  
> -    spin_lock_init(&v->arch.vgic.private_irqs.lock);
> +    vir =  xzalloc(struct vgic_irq_rank);
> +    memset(vir, 0, sizeof(struct vgic_irq_rank));

sizeof(*vir)

> +    spin_lock_init(&vir->lock);
>  
>      memset(&v->arch.vgic.pending_irqs, 0, sizeof(v->arch.vgic.pending_irqs));
>      for (i = 0; i < 32; i++)
> @@ -95,11 +107,14 @@ int vcpu_vgic_init(struct vcpu *v)
>  
>      /* For SGI and PPI the target is always this CPU */
>      for ( i = 0 ; i < 8 ; i++ )
> -        v->arch.vgic.private_irqs.itargets[i] =
> +        vir->itargets[i] =
>                (1<<(v->vcpu_id+0))
>              | (1<<(v->vcpu_id+8))
>              | (1<<(v->vcpu_id+16))
>              | (1<<(v->vcpu_id+24));
> +
> +    v->arch.vgic.private_irqs = (struct vgic_irq_irank *)vir;
> +

The cast is not useful here...

>      INIT_LIST_HEAD(&v->arch.vgic.inflight_irqs);
>      INIT_LIST_HEAD(&v->arch.vgic.lr_pending);
>      spin_lock_init(&v->arch.vgic.lock);
> @@ -636,6 +651,7 @@ static struct mmio_handler vgic_distr_mmio_handler = {
>  int domain_vgic_init(struct domain *d)
>  {
>      int i;
> +    struct vgic_irq_rank *r;
>  
>      d->arch.vgic.ctlr = 0;
>  
> @@ -648,7 +664,8 @@ int domain_vgic_init(struct domain *d)
>          d->arch.vgic.nr_lines = 0; /* We don't need SPIs for the guest */
>  
>      d->arch.vgic.shared_irqs =
> -        xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_RANKS(d));
> +        (struct vgic_irq_rank *)xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_RANKS(d));
> +

Same here.

>      d->arch.vgic.pending_irqs =
>          xzalloc_array(struct pending_irq, d->arch.vgic.nr_lines);
>      for (i=0; i<d->arch.vgic.nr_lines; i++)
> @@ -657,7 +674,11 @@ int domain_vgic_init(struct domain *d)
>          INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].lr_queue);
>      }
>      for (i=0; i<DOMAIN_NR_RANKS(d); i++)
> -        spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
> +    {
> +        r = (struct vgic_irq_rank *)((unsigned char *)(d->arch.vgic.shared_irqs)
> +                                      + sizeof(struct vgic_irq_rank) * i);

Please use temporary variable instead of ugly cast to unsigned char.

Regards,

-- 
Julien Grall