Re: [PATCH v4 2/5] xen/arm: vgic-v2: Don't ignore a write in ITARGETSR if one field is 0

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-10-23 at 10:37 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 23/10/15 10:30, Ian Campbell wrote:
> > On Thu, 2015-10-22 at 17:51 +0100, Julien Grall wrote:
> > > On 22/10/15 17:07, Ian Campbell wrote:
> > > > > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> > > > > index 665afeb..6b7eab3 100644
> > > > > --- a/xen/arch/arm/vgic-v2.c
> > > > > +++ b/xen/arch/arm/vgic-v2.c
> > > > > @@ -50,6 +50,94 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t
> > > > > cbase,
> > > > > paddr_t vbase)
> > > > >      vgic_v2_hw.vbase = vbase;
> > > > >  }
> > > > >  
> > > > > +#define NR_TARGET_PER_REG 4U
> > > > > +#define NR_BIT_PER_TARGET 8U
> > > > 
> > > > NR_TARGETS_ and NR_BITS_...
> > > > 
> > > > "REG" there is a bit generic, given this only works for registers
> > > > with
> > > > 8
> > > > -bit fields, _ITARGETSR instead?
> > > 
> > > Well this is within the vgic-v2.c and only one register contains
> > > target.
> > > So I found pointless to add ITARGETSR to the name.
> > 
> > It's the use of the generic "REG" when there is only one relevant
> > register
> > (which could be named) which I found confusing, since the current name
> > implies it has wider relevance than it actually does.
> 
> Ok. I guess you'd like to rename NR_BITS_PER_TARGET? If so, do you have
> any suggestion?

NR_BITS_PER_TARGET is OK here, since 8 is always correct for that in the
gic v2 irrespective of whether it is used with any register.

NR_TARGET_PER_REG was the thing I thought we were discussing above. I think
that one should be NR_TARGETS_PER_ITARGETSR.

Alternatively
    const unsigned long nr_targets_per_reg = 32/NR_BITS_PER_TARGET;
in the context of the vgic_store_itargetsr function (and others which use
it) would be ok too. Or maybe 32=>(sizeof(...)*8). And maybe in that
context an even terser name would be ok too.

Ian.