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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-03-20 at 13:51 +0000, Julien Grall wrote:

> > 
> > Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> > ---
> >  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

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.
> 
> >  /*
> >   * 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.

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

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel