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

> > This is really a part of the for() iteration expression, but oddly
> > place
> > here.
> > If you turn the "((1 << NR_BIT_PER_TARGET) - 1)" thing into a #define
> > or
> > constant, then you may find that extracting the relevant byte from the
> > unshifted itargetsr using (itargetsr >> (i*NR_BITS_PER_TARGET) and then
> > applying the mask is clean enough to use here instead.
> 
> I placed this shift here because I didn't want to use ... >> (i *
> NR_BIT_..) which require a multiplication rather than a shift in the
> resulting code.

FWIW given a constant NR_BITS which is a power of two I think i*NR_BITS
would end up a shift with any reasonable compiler.

> > > +         * guest).
> > > +         */
> > > +        if ( !new_target || (new_target > d->max_vcpus) )
> > > +        {
> > > +            printk(XENLOG_G_DEBUG
> > 
> > gdprintk?
> 
> I would prefer to keep this printk in non-debug to help us catching any
> OS potentially using this trick.
> 
> Based on that I would even use XENLOG_G_WARNING because this is not
> really compliant to the spec and we are meant to fix it.

Assuming I remember correctly that XENLOG_G_WARNING is ratelimited in
default configurations, then OK.

> 
> Regards,
>