Re: [Xen-devel] [PATCH 1/7] xen/arm: fix rank calculation in vgic_vcpu_inject_irq

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-11-13 at 11:57 +0000, Stefano Stabellini wrote:
> On Sat, 27 Oct 2012, Tim Deegan wrote:
> > At 21:47 +0100 on 26 Oct (1351288040), Ian Campbell wrote:
> > > On Fri, 2012-10-26 at 19:42 +0100, Stefano Stabellini wrote:
> > > > I think that the problem is the usage of vgic_irq_rank with registers
> > > > that have 1 bit per interrupt.
> > > 
> > > That's very plausible indeed.
> > > 
> > > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > > > index 3c3983f..92731b6 100644
> > > > --- a/xen/arch/arm/vgic.c
> > > > +++ b/xen/arch/arm/vgic.c
> > > > @@ -42,13 +42,7 @@
> > > >   */
> > > >  static inline int REG_RANK_NR(int b, uint32_t n)
> > > >  {
> > > > -    switch ( b )
> > > > -    {
> > > > -    case 8: return n >> 3;
> > > > -    case 4: return n >> 2;
> > > > -    case 2: return n >> 1;
> > > > -    default: BUG();
> > > > -    }
> > > > +    return n / b;
> > > 
> > > All the infrastructure will fall apart if b isn't a power of two, that's
> > > why I used the switch. Can you just add the appropriate case 1 instead?
> > > Probably the bug should be a call to an undefined function to make this
> > > a compile time rather than runtime failure too.
> > 
> > BUILD_BUG_ON((b & -b) != b); ?
> 
> I can't do that:
> 
> error: expression in static assertion is not constant

I suppose you could make REG_RANK_NR a macro (as it's name implies it
ought to be, oops!).

If not then given that b must be in {1,2,4,8} maybe the switch is
ok/tolerable? If we end up supporting b=16 or 32 then more thought is
probably required around the place anyway.

Ian.


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