Re: [Xen-devel] [PATCH] xen: arm: bitops take unsigned int (Was: Re: [PATCH] xen/arm64: disable alignment check)

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 9 May 2014, Ian Campbell wrote:
> (Just adding the other ARM guys...)
> 
> On Fri, 2014-05-09 at 14:24 +0100, Ian Campbell wrote:
> > On Tue, 2014-04-29 at 09:58 +0100, Ian Campbell wrote:
> > > On Tue, 2014-04-29 at 08:38 +0100, Vladimir Murzin wrote:
> > > > On Mon, Apr 28, 2014 at 11:43 AM, Ian Campbell 
> > > > <Ian.Campbell@xxxxxxxxxx> wrote:
> > > > > But I also wanted confirmation that the problematic instruction was
> > > > > generated by gcc and not by some handcoded asm somewhere which we 
> > > > > hadn't
> > > > > properly fixed.
> > > 
> > > > I believe it comes form test_bit (xen/include/asm-arm/bitops.h).
> > > 
> > > Ah, then I think this code needs fixing too. Probably switching to
> > > unsigned int * throughout would work, what do you think?
> > 
> > I finally managed to upgrade to a new enough kernel to trigger this.
> > 
> > This Works For Me(tm), along with the Linux patch "xen/events/fifo:
> > correctly align bitops" which is queued for 3.15 Linus (but not sent
> > yet?)

The change looks good to me.


> > 8<-------------------
> > 
> > From aa6afe6520ea22241fb0ce430ef315c49a73867f Mon Sep 17 00:00:00 2001
> > From: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > Date: Thu, 8 May 2014 16:13:55 +0100
> > Subject: [PATCH] xen: arm: bitops take unsigned int
> > 
> > Xen bitmaps can be 4 rather than 8 byte aligned, so use the appropriate 
> > type.
> > Otherwise the compiler can generate unaligned 8 byte accesses and cause 
> > traps.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > ---
> >  xen/include/asm-arm/bitops.h |   37 +++++++++++++++++++------------------
> >  1 file changed, 19 insertions(+), 18 deletions(-)
> > 
> > diff --git a/xen/include/asm-arm/bitops.h b/xen/include/asm-arm/bitops.h
> > index 0a7caee..25f96c8 100644
> > --- a/xen/include/asm-arm/bitops.h
> > +++ b/xen/include/asm-arm/bitops.h
> > @@ -18,13 +18,14 @@
> >  #define __set_bit(n,p)            set_bit(n,p)
> >  #define __clear_bit(n,p)          clear_bit(n,p)
> >  
> > +#define BITS_PER_WORD           32
> >  #define BIT(nr)                 (1UL << (nr))
> > -#define BIT_MASK(nr)            (1UL << ((nr) % BITS_PER_LONG))
> > -#define BIT_WORD(nr)            ((nr) / BITS_PER_LONG)
> > +#define BIT_MASK(nr)            (1UL << ((nr) % BITS_PER_WORD))
> > +#define BIT_WORD(nr)            ((nr) / BITS_PER_WORD)
> >  #define BITS_PER_BYTE           8
> >  
> > -#define ADDR (*(volatile long *) addr)
> > -#define CONST_ADDR (*(const volatile long *) addr)
> > +#define ADDR (*(volatile int *) addr)
> > +#define CONST_ADDR (*(const volatile int *) addr)
> >  
> >  #if defined(CONFIG_ARM_32)
> >  # include <asm/arm32/bitops.h>
> > @@ -45,10 +46,10 @@
> >   */
> >  static inline int __test_and_set_bit(int nr, volatile void *addr)
> >  {
> > -        unsigned long mask = BIT_MASK(nr);
> > -        volatile unsigned long *p =
> > -                ((volatile unsigned long *)addr) + BIT_WORD(nr);
> > -        unsigned long old = *p;
> > +        unsigned int mask = BIT_MASK(nr);
> > +        volatile unsigned int *p =
> > +                ((volatile unsigned int *)addr) + BIT_WORD(nr);
> > +        unsigned int old = *p;
> >  
> >          *p = old | mask;
> >          return (old & mask) != 0;
> > @@ -65,10 +66,10 @@ static inline int __test_and_set_bit(int nr, volatile 
> > void *addr)
> >   */
> >  static inline int __test_and_clear_bit(int nr, volatile void *addr)
> >  {
> > -        unsigned long mask = BIT_MASK(nr);
> > -        volatile unsigned long *p =
> > -                ((volatile unsigned long *)addr) + BIT_WORD(nr);
> > -        unsigned long old = *p;
> > +        unsigned int mask = BIT_MASK(nr);
> > +        volatile unsigned int *p =
> > +                ((volatile unsigned int *)addr) + BIT_WORD(nr);
> > +        unsigned int old = *p;
> >  
> >          *p = old & ~mask;
> >          return (old & mask) != 0;
> > @@ -78,10 +79,10 @@ static inline int __test_and_clear_bit(int nr, volatile 
> > void *addr)
> >  static inline int __test_and_change_bit(int nr,
> >                                              volatile void *addr)
> >  {
> > -        unsigned long mask = BIT_MASK(nr);
> > -        volatile unsigned long *p =
> > -                ((volatile unsigned long *)addr) + BIT_WORD(nr);
> > -        unsigned long old = *p;
> > +        unsigned int mask = BIT_MASK(nr);
> > +        volatile unsigned int *p =
> > +                ((volatile unsigned int *)addr) + BIT_WORD(nr);
> > +        unsigned int old = *p;
> >  
> >          *p = old ^ mask;
> >          return (old & mask) != 0;
> > @@ -94,8 +95,8 @@ static inline int __test_and_change_bit(int nr,
> >   */
> >  static inline int test_bit(int nr, const volatile void *addr)
> >  {
> > -        const volatile unsigned long *p = (const volatile unsigned long 
> > *)addr;
> > -        return 1UL & (p[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));
> > +        const volatile unsigned int *p = (const volatile unsigned int 
> > *)addr;
> > +        return 1UL & (p[BIT_WORD(nr)] >> (nr & (BITS_PER_WORD-1)));
> >  }
> >  
> >  static inline int constant_fls(int x)
> 
> 

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