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

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

On Fri, Mar 21, 2014 at 10:53 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2014-03-20 at 13:51 +0000, Julien Grall wrote:
>
>> >
>> > 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

> Is there going to be lots of this stuff which is different for the two?
> I'd rather wait and see how unwieldy gic.h gets first.

For now, the difference is in v3 uint32_t  itargets[8] is replaced
with u64 itargets[32]
may be for v4 it other registers might differ

>>
>> >  /*
>> >   * 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?
>
> The exact same thing was present later on, which is crying out for a
> helper function.
>
> But actually if v->domain->arch.vgic.shared_irqs were correctly typed
> this would be &v->domain->arch.vgic.shared_irqs[rank -i ], which is
> clearly the right way to go about this, casting to char * is too ugly to
> live.
>
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)
>>
>> > +    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...
>
> And if this line were done up near the xzalloc the rest of the changes
> would fall away too, the vir variable isn't all that helpful.
>
OK
> Ian.
>