Re: [Xen-devel] [PATCH v2 11/15] xen/arm: make GIC context data version specific

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2014-04-04 at 15:09 +0100, Julien Grall wrote:
> Hello Vijaya,
> 
> Thank you for the patch.
> 
> On 04/04/2014 12:56 PM, vijay.kilari@xxxxxxxxx wrote:
> > From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> > 
> > GIC context data is dependent on hardware version
> > make the contents of gic context data structure
> > as version specific and access accordingly
> > 
> > Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> > ---
> >  xen/arch/arm/gic-v2.c     |   15 ++++++++-------
> >  xen/include/asm-arm/gic.h |   11 ++++++++---
> >  2 files changed, 16 insertions(+), 10 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index 06ed12b..4bcb392 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -96,6 +96,7 @@ static int gic_state_init(struct vcpu *v)
> >       v->arch.gic_state = xzalloc(struct gic_state_data);
> >       if ( !v->arch.gic_state )
> >          return -ENOMEM;
> > +     v->arch.gic_state->version = 2;
> 
> I would prefer an enum rather than a fixed number.

How is this field actually used? I'd have thought the v2 and v3 specific
callbacks would just know which member of the union was "theirs" and
nothing outside of that should be poking at it anyway.

>+            printk("   VCPU_LR[%d]=%x\n", i, v->arch.gic_state->v2.gic_lr[i]);

Too many gic prefixes here, and the _state is redundant.
        v->arch.gic.v2.lr[i]
would be find IMHO.

Ian.



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