From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <Ian.Campbell@citrix.com>
Subject: Re: [PATCH v2 11/15] xen/arm: make GIC context data
	version specific
Date: Thu, 10 Apr 2014 10:14:14 +0100
Message-ID: <1397121254.9862.51.camel@kazak.uk.xensource.com>
References: <1396612593-443-1-git-send-email-vijay.kilari@gmail.com>
	<1396612593-443-12-git-send-email-vijay.kilari@gmail.com>
	<533EBCFD.20403@linaro.org>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <533EBCFD.20403@linaro.org>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Julien Grall <julien.grall@linaro.org>
Cc: vijay.kilari@gmail.com, stefano.stabellini@eu.citrix.com, Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com, xen-devel@lists.xen.org, stefano.stabellini@citrix.com
List-Id: xen-devel@lists.xenproject.org

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@gmail.com wrote:
> > From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> > 
> > 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@caviumnetworks.com>
> > ---
> >  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.