[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 4/8] xen/ppc: Implement bitops.h


  • To: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 29 Aug 2023 15:59:01 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=m9PtzEkBtpU7v0tqSiGeA8L6JHWl6y7ZYZ9DwbwhSQY=; b=jsm+PfrtATWKhtP9mH4GmO+CI+EiPKjbJ1bWa+biuvRRcJ0QQCiDJRiffDtUbMPG53fZZre89xXPz4y2883vXlq/xR35OuXnUqXa+jMY8KpZdPalGwnsvVHgHYOQpxaOvHAL26lqZFUROHjKbOlBcgjlqtkzCG5SGaQMoOJQGo8TzW/Mwr1XcmvLfzQab0BLydlw4Wo3OXjxtmQf5wOxNxPV6YEt1GE4i9nrPDfCMJOT3Snts9CIHwVe8vCu5tBoDP9sdNr0FjRQKnpka3MnQhILj0D/4qcahdmRCV+UrujRdgTuAvHUPM2PmhxWLYfuMMg2PPrN32uxaXfHVAbIXw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=eS/xSq/N+PlKrEKGqBuKTpvfQISgSd9AHj6wvO1eoGXDK302U3gdd6p6mK3APJQwx0+JzQ3SGQtpMLhjvqjw2AllWRObzRCqjYfgsyetp3Yf3IBTuSBSsQbBomnwfw2ZBGXn8Y15DcbIyzl4SGqmaFU6HLLTNPagLkQQ+aD453fE5y5cN10jZMXpiyiHWajzsXyTTIvKGb4xMplu+8g+NwtXD0WRbVKnox8wtsHY6OdAtEHRYLh4VCAmxnLH/Vb3+P52HrbakL1/eUyqynrp4zPkUpPiMHujl1YXaqOG6O/wzesd0CWN4JI1THaGDKvDWOyF+TlnTMtkzXRuLMaBNg==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Timothy Pearson <tpearson@xxxxxxxxxxxxxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Tue, 29 Aug 2023 13:59:18 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 23.08.2023 22:07, Shawn Anastasio wrote:
> Implement bitops.h, based on Linux's implementation as of commit
> 5321d1b1afb9a17302c6cec79f0cbf823eb0d3fc. Though it is based off of
> Linux's implementation, this code diverges significantly in a number of
> ways:
>   - Bitmap entries changed to 32-bit words to match X86 and Arm on Xen
>   - PPC32-specific code paths dropped
>   - Formatting completely re-done to more closely line up with Xen.
>     Including 4 space indentation.

With this goal, ...

> --- a/xen/arch/ppc/include/asm/bitops.h
> +++ b/xen/arch/ppc/include/asm/bitops.h
> @@ -1,9 +1,335 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Adapted from Linux's arch/powerpc/include/asm/bitops.h.
> + *
> + * Merged version by David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>.
> + * Based on ppc64 versions by: Dave Engebretsen, Todd Inglett, Don
> + * Reed, Pat McCarthy, Peter Bergner, Anton Blanchard.  They
> + * originally took it from the ppc32 code.
> + */
>  #ifndef _ASM_PPC_BITOPS_H
>  #define _ASM_PPC_BITOPS_H
> 
> +#include <asm/memory.h>
> +
> +#define __set_bit(n,p)            set_bit(n,p)
> +#define __clear_bit(n,p)          clear_bit(n,p)

... you want to add blanks after the commas as well. (You might also
simply omit parameters altogether.)

> +#define BITOP_BITS_PER_WORD     32
> +#define BITOP_MASK(nr)          (1UL << ((nr) % BITOP_BITS_PER_WORD))
> +#define BITOP_WORD(nr)          ((nr) / BITOP_BITS_PER_WORD)
> +#define BITS_PER_BYTE           8
> +
>  /* PPC bit number conversion */
> -#define PPC_BITLSHIFT(be)    (BITS_PER_LONG - 1 - (be))
> -#define PPC_BIT(bit)         (1UL << PPC_BITLSHIFT(bit))
> -#define PPC_BITMASK(bs, be)  ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
> +#define PPC_BITLSHIFT(be)    (BITS_PER_LONG - 1 - (be))
> +#define PPC_BIT(bit)         (1UL << PPC_BITLSHIFT(bit))
> +#define PPC_BITMASK(bs, be)  ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
> +
> +/* Macro for generating the ***_bits() functions */
> +#define DEFINE_BITOP(fn, op, prefix)                                         
>   \
> +static inline void fn(unsigned long mask,                                    
>   \
> +        volatile unsigned int *_p)                                           
>   \

Nit: Style. Either

static inline void fn(unsigned long mask,                                      \
                      volatile unsigned int *_p)                               \

or

static inline void fn(unsigned long mask,                                      \
    volatile unsigned int *_p)                                                 \

. Also there's again an underscore-prefixed identifier here.

> +{                                                                            
>   \
> +    unsigned long old;                                                       
>   \
> +    unsigned int *p = (unsigned int *)_p;                                    
>   \
> +    asm volatile (                                                           
>   \
> +    prefix                                                                   
>   \
> +"1: lwarx %0,0,%3,0\n"                                                       
>   \
> +    #op "%I2 %0,%0,%2\n"                                                     
>   \
> +    "stwcx. %0,0,%3\n"                                                       
>   \
> +    "bne- 1b\n"                                                              
>   \
> +    : "=&r" (old), "+m" (*p)                                                 
>   \
> +    : "rK" (mask), "r" (p)                                                   
>   \
> +    : "cc", "memory");                                                       
>   \

The asm() body wants indenting by another four blanks (more instances below).

> +}
> +
> +DEFINE_BITOP(set_bits, or, "")
> +DEFINE_BITOP(change_bits, xor, "")
> +
> +#define DEFINE_CLROP(fn, prefix)                                             
>   \
> +static inline void fn(unsigned long mask, volatile unsigned int *_p)         
>   \
> +{                                                                            
>   \
> +    unsigned long old;                                                       
>   \
> +    unsigned int *p = (unsigned int *)_p;                                    
>   \
> +    asm volatile (                                                           
>   \
> +    prefix                                                                   
>   \
> +"1: lwarx %0,0,%3,0\n"                                                       
>   \
> +    "andc %0,%0,%2\n"                                                        
>   \
> +    "stwcx. %0,0,%3\n"                                                       
>   \
> +    "bne- 1b\n"                                                              
>   \
> +    : "=&r" (old), "+m" (*p)                                                 
>   \
> +    : "r" (mask), "r" (p)                                                    
>   \
> +    : "cc", "memory");                                                       
>   \
> +}
> +
> +DEFINE_CLROP(clear_bits, "")
> +
> +static inline void set_bit(int nr, volatile void *addr)
> +{
> +    set_bits(BITOP_MASK(nr), (volatile unsigned int *)addr + BITOP_WORD(nr));
> +}
> +static inline void clear_bit(int nr, volatile void *addr)
> +{
> +    clear_bits(BITOP_MASK(nr), (volatile unsigned int *)addr + 
> BITOP_WORD(nr));
> +}
> +
> +/**
> + * test_bit - Determine whether a bit is set
> + * @nr: bit number to test
> + * @addr: Address to start counting from
> + */
> +static inline int test_bit(int nr, const volatile void *addr)
> +{
> +        const volatile unsigned long *p = (const volatile unsigned long 
> *)addr;
> +        return 1UL & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD-1)));

Nit: Too deep indentation. Plus blanks around - please. I also don't see
the need for the UL suffix, when the function returns int only (and really
means to return bool, I assume, but int is in line with x86 and Arm, I
expect).

> +}
> +
> +static inline unsigned long test_and_clear_bits(unsigned long mask, volatile 
> void *_p)
> +{
> +    unsigned long old, t;
> +    unsigned int *p = (unsigned int *)_p;
> +
> +    asm volatile (
> +        PPC_ATOMIC_ENTRY_BARRIER
> +        "1: lwarx %0,0,%3,0\n"
> +        "andc %1,%0,%2\n"
> +        "stwcx. %1,0,%3\n"
> +        "bne- 1b\n"
> +        PPC_ATOMIC_EXIT_BARRIER
> +        : "=&r" (old), "=&r" (t)
> +        : "r" (mask), "r" (p)
> +        : "cc", "memory");
> +
> +    return (old & mask);
> +}
> +
> +static inline int test_and_clear_bit(unsigned int nr,
> +                                       volatile void *addr)

Nit: Too deep indentation again.

> +{
> +    return test_and_clear_bits(BITOP_MASK(nr), addr + BITOP_WORD(nr)) != 0;
> +}
> +
> +#define DEFINE_TESTOP(fn, op, eh)                                            
>   \
> +static inline unsigned long fn(                                              
>   \
> +        unsigned long mask,                                                  
>   \
> +        volatile unsigned int *_p)                                           
>   \

And yet once more (and there are more below).

> +{                                                                            
>   \
> +    unsigned long old, t;                                                    
>   \
> +    unsigned int *p = (unsigned int *)_p;                                    
>   \
> +    __asm__ __volatile__ (                                                   
>   \
> +    PPC_ATOMIC_ENTRY_BARRIER                                                 
>   \
> +"1:" "lwarx %0,0,%3,%4\n"                                                    
>   \
> +    #op "%I2 %1,%0,%2\n"                                                     
>   \
> +    "stwcx. %1,0,%3\n"                                                       
>   \
> +    "bne- 1b\n"                                                              
>   \
> +    PPC_ATOMIC_EXIT_BARRIER                                                  
>   \
> +    : "=&r" (old), "=&r" (t)                                                 
>   \
> +    : "rK" (mask), "r" (p), "n" (eh)                                         
>   \
> +    : "cc", "memory");                                                       
>   \
> +    return (old & mask);                                                     
>   \
> +}
> +
> +DEFINE_TESTOP(test_and_set_bits, or, 0)
> +
> +static inline int test_and_set_bit(unsigned long nr, volatile void *addr)
> +{
> +    return test_and_set_bits(BITOP_MASK(nr), (volatile unsigned int *)addr + 
> BITOP_WORD(nr)) != 0;

Too long line.

> +}
> +
> +/**
> + * __test_and_set_bit - Set a bit and return its old value
> + * @nr: Bit to set
> + * @addr: Address to count from
> + *
> + * This operation is non-atomic and can be reordered.
> + * If two examples of this operation race, one can appear to succeed
> + * but actually fail.  You must protect multiple accesses with a lock.
> + */
> +static inline int __test_and_set_bit(int nr, volatile void *addr)
> +{
> +        unsigned int mask = BITOP_MASK(nr);
> +        volatile unsigned int *p =
> +                ((volatile unsigned int *)addr) + BITOP_WORD(nr);
> +        unsigned int old = *p;
> +
> +        *p = old | mask;
> +        return (old & mask) != 0;
> +}
> +
> +/**
> + * __test_and_clear_bit - Clear a bit and return its old value
> + * @nr: Bit to clear
> + * @addr: Address to count from
> + *
> + * This operation is non-atomic and can be reordered.
> + * If two examples of this operation race, one can appear to succeed
> + * but actually fail.  You must protect multiple accesses with a lock.
> + */
> +static inline int __test_and_clear_bit(int nr, volatile void *addr)
> +{
> +        unsigned int mask = BITOP_MASK(nr);
> +        volatile unsigned int *p =
> +                ((volatile unsigned int *)addr) + BITOP_WORD(nr);
> +        unsigned int old = *p;
> +
> +        *p = old & ~mask;
> +        return (old & mask) != 0;
> +}
> +
> +#define flsl(x) generic_flsl(x)
> +#define fls(x) generic_fls(x)
> +#define ffs(x) ({ unsigned int __t = (x); fls(__t & -__t); })
> +#define ffsl(x) ({ unsigned long __t = (x); flsl(__t & -__t); })

Hmm, here you even have two underscores as prefixes.

> +/* Based on linux/include/asm-generic/bitops/ffz.h */
> +/*
> + * ffz - find first zero in word.
> + * @word: The word to search
> + *
> + * Undefined if no zero exists, so code should check against ~0UL first.
> + */
> +#define ffz(x)  __ffs(~(x))
> +
> +/**
> + * hweightN - returns the hamming weight of a N-bit word
> + * @x: the word to weigh
> + *
> + * The Hamming Weight of a number is the total number of bits set in it.
> + */
> +#define hweight64(x) generic_hweight64(x)
> +#define hweight32(x) generic_hweight32(x)
> +#define hweight16(x) generic_hweight16(x)
> +#define hweight8(x) generic_hweight8(x)

Not using popcnt{b,w,d}, e.g. via a compiler builtin?

> +/* Based on linux/include/asm-generic/bitops/builtin-__ffs.h */
> +/**
> + * __ffs - find first bit in word.
> + * @word: The word to search
> + *
> + * Undefined if no bit exists, so code should check against 0 first.
> + */
> +static /*__*/always_inline unsigned long __ffs(unsigned long word)

What's this odd comment about here?

> +{
> +        return __builtin_ctzl(word);
> +}
> +
> +/**
> + * find_first_set_bit - find the first set bit in @word
> + * @word: the word to search
> + *
> + * Returns the bit-number of the first set bit (first bit being 0).
> + * The input must *not* be zero.
> + */
> +#define find_first_set_bit(x) ({ ffsl(x) - 1; })

Simply

#define find_first_set_bit(x) (ffsl(x) - 1)

without use of any extensions?

> +/*
> + * Find the first set bit in a memory region.
> + */
> +static inline unsigned long find_first_bit(const unsigned long *addr,
> +                                           unsigned long size)
> +{
> +    const unsigned long *p = addr;
> +    unsigned long result = 0;
> +    unsigned long tmp;
> +
> +    while (size & ~(BITS_PER_LONG-1)) {
> +        if ((tmp = *(p++)))
> +            goto found;
> +        result += BITS_PER_LONG;
> +        size -= BITS_PER_LONG;
> +    }
> +    if (!size)
> +        return result;

Just using 4-blank indentation isn't enough to make this Xen style.
(More such elsewhere.)

> +    tmp = (*p) & (~0UL >> (BITS_PER_LONG - size));
> +    if (tmp == 0UL)        /* Are any bits set? */
> +        return result + size;    /* Nope. */
> +found:

Labels indented by at least one blank please. (More elsewhere.)

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.