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

[Vijay Kilari <vijay.kilari@gmail.com>]

On Thu, Mar 20, 2014 at 7:21 PM, Julien Grall <julien.grall@linaro.org> wrote:
> 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.
>
OK. I will check the feasibility and rework accordingly

>>
>> 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
>
OK

>>  /*
>>   * 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?
>
OK

>>      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)
>
OK

>> +    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...
OK

>
>>      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.
OK

>
>>      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.
>
OK

> Regards,
>
> --
> Julien Grall