Re: [PATCH v3 10/16] xen/arm: move vgic rank data to gic header file

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

Hello Vijaya,

Thank you for the patch.

On 04/15/2014 12:17 PM, vijay.kilari@gmail.com wrote:
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 4830b5d..f73247d 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -30,6 +30,7 @@
>  #include <asm/mmio.h>
>  #include <asm/gic_v2_defs.h>
>  #include <asm/gic.h>
> +#include <asm/vgic.h>
>  
>  #define REG(n) (n/4)
>  
> @@ -69,7 +70,7 @@ 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 v->arch.vgic.private_irqs;
>      else if ( rank <= DOMAIN_NR_RANKS(v->domain) )
>          return &v->domain->arch.vgic.shared_irqs[rank - 1];
>      else
> @@ -117,9 +118,14 @@ 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));
>  
> -    spin_lock_init(&v->arch.vgic.private_irqs.lock);
> +    v->arch.vgic.private_irqs = xzalloc(struct vgic_irq_rank);
> +    if ( v->arch.vgic.private_irqs == NULL )
> +      return -ENOMEM;
> +
> +    memset(v->arch.vgic.private_irqs, 0, sizeof(v->arch.vgic.private_irqs));
> +

xzalloc will zeroed the structure for you. You don't need to zeroed
again. BTW the sizeof was wrong here :).

> +    spin_lock_init(&v->arch.vgic.private_irqs->lock);
>  
>      memset(&v->arch.vgic.pending_irqs, 0, sizeof(v->arch.vgic.pending_irqs));
>      for (i = 0; i < 32; i++)
> @@ -130,7 +136,7 @@ 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] =
> +        v->arch.vgic.private_irqs->itargets[i] =
>                (1<<(v->vcpu_id+0))
>              | (1<<(v->vcpu_id+8))
>              | (1<<(v->vcpu_id+16))
> @@ -142,6 +148,12 @@ int vcpu_vgic_init(struct vcpu *v)
>      return 0;
>  }
>  
> +int vcpu_vgic_free(struct vcpu *v)
> +{
> +    xfree(v->arch.vgic.private_irqs);

Can you add a newline here for clarity?

> +    return 0;
> +}
> +
>  #define vgic_lock(v)   spin_lock_irq(&(v)->domain->arch.vgic.lock)
>  #define vgic_unlock(v) spin_unlock_irq(&(v)->domain->arch.vgic.lock)
>  

[..]

> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> new file mode 100644
> index 0000000..104a87d
> --- /dev/null
> +++ b/xen/include/asm-arm/vgic.h
> @@ -0,0 +1,40 @@
> +/*
> + * ARM Virtual Generic Interrupt Controller support
> + *
> + * Ian Campbell <ian.campbell@citrix.com>
> + * Copyright (c) 2011 Citrix Systems.
> + *

Hmmm... where does the copyright come from?

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef __ASM_ARM_VGIC_H__
> +#define __ASM_ARM_VGIC_H__
> +
> +/* 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];
> +};
> +
> +extern int vcpu_vgic_free(struct vcpu *v);
> +#endif

#endif /* __ASM_ARM_VGIC_H__ */

Regards,

-- 
Julien Grall