[v7,07/19] xen/riscv: introduce bitops.h

Commit Message

Oleksii Kurochko April 3, 2024, 10:20 a.m. UTC

Taken from Linux-6.4.0-rc1

Xen's bitops.h consists of several Linux's headers:
* linux/arch/include/asm/bitops.h:
  * The following function were removed as they aren't used in Xen:
        * test_and_set_bit_lock
        * clear_bit_unlock
        * __clear_bit_unlock
  * The following functions were renamed in the way how they are
    used by common code:
        * __test_and_set_bit
        * __test_and_clear_bit
  * The declaration and implementation of the following functios
    were updated to make Xen build happy:
        * clear_bit
        * set_bit
        * __test_and_clear_bit
        * __test_and_set_bit
  * linux/include/asm-generic/bitops/generic-non-atomic.h with the
    following changes:
     * Only functions that can be reused in Xen were left;
       others were removed.
     * it was updated the message inside #ifndef ... #endif.
     * __always_inline -> always_inline to be align with definition in
       xen/compiler.h.
     * convert identations from tabs to spaces.
     * inside generic__test_and_* use 'bitops_uint_t' instead of 'unsigned long'
        to be generic.

Signed-off-by: Oleksii Kurochko <[e-mail]>
---
Changes in V7:
 - Update the commit message.
 - Drop "__" for __op_bit and __op_bit_ord as they are atomic.
 - add comment above __set_bit and __clear_bit about why they are defined as atomic.
 - align bitops_uint_t with __AMO().
 - make changes after  generic non-atomic test_*bit() were changed.
 - s/__asm__ __volatile__/asm volatile
---
Changes in V6:
 - rebase clean ups were done: drop unused asm-generic includes
---
 Changes in V5:
   - new patch
---
 xen/arch/riscv/include/asm/bitops.h | 146 ++++++++++++++++++++++++++++
 1 file changed, 146 insertions(+)
 create mode 100644 xen/arch/riscv/include/asm/bitops.h

Comments

Jan Beulich April 4, 2024, 2:47 p.m. UTC | #1

On 03.04.2024 12:20, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/bitops.h
> @@ -0,0 +1,146 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2012 Regents of the University of California */
> +
> +#ifndef _ASM_RISCV_BITOPS_H
> +#define _ASM_RISCV_BITOPS_H
> +
> +#include <asm/system.h>
> +
> +#undef BITOP_BITS_PER_WORD
> +#undef bitop_uint_t
> +
> +#define BITOP_BITS_PER_WORD BITS_PER_LONG
> +#define bitop_uint_t unsigned long
> +
> +#if BITS_PER_LONG == 64
> +#define __AMO(op)   "amo" #op ".d"
> +#elif BITS_PER_LONG == 32
> +#define __AMO(op)   "amo" #op ".w"
> +#else
> +#error "Unexpected BITS_PER_LONG"
> +#endif
> +
> +#define __set_bit(n, p)      set_bit(n, p)
> +#define __clear_bit(n, p)    clear_bit(n, p)

First without comment and then ...

> +/* Based on linux/arch/include/asm/bitops.h */
> +
> +/*
> + * Non-atomic bit manipulation.
> + *
> + * Implemented using atomics to be interrupt safe. Could alternatively
> + * implement with local interrupt masking.
> + */
> +#define __set_bit(n, p)      set_bit(n, p)
> +#define __clear_bit(n, p)    clear_bit(n, p)

... with one?

> +/* Based on linux/arch/include/asm/bitops.h */

Does this really need repeating?

> +#define test_and_op_bit_ord(op, mod, nr, addr, ord)     \
> +({                                                      \
> +    unsigned long res, mask;                            \

bitop_uint_t?

> +    mask = BITOP_MASK(nr);                              \
> +    asm volatile (                                       \

Nit: One too many padding blanks.

> +        __AMO(op) #ord " %0, %2, %1"                    \
> +        : "=r" (res), "+A" (addr[BITOP_WORD(nr)])       \
> +        : "r" (mod(mask))                               \
> +        : "memory");                                    \
> +    ((res & mask) != 0);                                \
> +})
> +
> +#define op_bit_ord(op, mod, nr, addr, ord)      \
> +    asm volatile (                              \
> +        __AMO(op) #ord " zero, %1, %0"          \
> +        : "+A" (addr[BITOP_WORD(nr)])           \
> +        : "r" (mod(BITOP_MASK(nr)))             \
> +        : "memory");
> +
> +#define test_and_op_bit(op, mod, nr, addr)    \
> +    test_and_op_bit_ord(op, mod, nr, addr, .aqrl)
> +#define op_bit(op, mod, nr, addr) \
> +    op_bit_ord(op, mod, nr, addr, )
> +
> +/* Bitmask modifiers */
> +#define NOP(x)    (x)
> +#define NOT(x)    (~(x))
> +
> +/**
> + * test_and_set_bit - Set a bit and return its old value
> + * @nr: Bit to set
> + * @addr: Address to count from
> + */
> +static inline int test_and_set_bit(int nr, volatile void *p)

In patch 4 you switched to bool. Any reason you didn't here, too?

> +{
> +    volatile bitop_uint_t *addr = p;
> +
> +    return test_and_op_bit(or, NOP, nr, addr);
> +}
> +
> +/**
> + * test_and_clear_bit - Clear a bit and return its old value
> + * @nr: Bit to clear
> + * @addr: Address to count from
> + */
> +static inline int test_and_clear_bit(int nr, volatile void *p)
> +{
> +    volatile bitop_uint_t *addr = p;
> +
> +    return test_and_op_bit(and, NOT, nr, addr);
> +}
> +
> +/**
> + * set_bit - Atomically set a bit in memory
> + * @nr: the bit to set
> + * @addr: the address to start counting from
> + *
> + * Note that @nr may be almost arbitrarily large; this function is not
> + * restricted to acting on a single-word quantity.
> + */
> +static inline void set_bit(int nr, volatile void *p)
> +{
> +    volatile bitop_uint_t *addr = p;
> +
> +    op_bit(or, NOP, nr, addr);
> +}
> +
> +/**
> + * clear_bit - Clears a bit in memory
> + * @nr: Bit to clear
> + * @addr: Address to start counting from
> + */
> +static inline void clear_bit(int nr, volatile void *p)
> +{
> +    volatile bitop_uint_t *addr = p;
> +
> +    op_bit(and, NOT, nr, addr);
> +}
> +
> +/**
> + * test_and_change_bit - Toggle (change) a bit and return its old value
> + * @nr: Bit to change
> + * @addr: Address to count from
> + *
> + * This operation is atomic and cannot be reordered.
> + * It also implies a memory barrier.
> + */
> +static inline int test_and_change_bit(int nr, volatile unsigned long *addr)

unsigned long?

> +{
> +	return test_and_op_bit(xor, NOP, nr, addr);
> +}

Perhaps better to move up a little, next to the other test_and-s?

Also - nit: Hard tab used for indentation.

> +#undef test_and_op_bit
> +#undef __op_bit

This no longer has any effect in this shape.

Jan

Oleksii Kurochko April 4, 2024, 3:54 p.m. UTC | #2

On Thu, 2024-04-04 at 16:47 +0200, Jan Beulich wrote:
> On 03.04.2024 12:20, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/bitops.h
> > @@ -0,0 +1,146 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright (C) 2012 Regents of the University of California */
> > +
> > +#ifndef _ASM_RISCV_BITOPS_H
> > +#define _ASM_RISCV_BITOPS_H
> > +
> > +#include <asm/system.h>
> > +
> > +#undef BITOP_BITS_PER_WORD
> > +#undef bitop_uint_t
> > +
> > +#define BITOP_BITS_PER_WORD BITS_PER_LONG
> > +#define bitop_uint_t unsigned long
> > +
> > +#if BITS_PER_LONG == 64
> > +#define __AMO(op)   "amo" #op ".d"
> > +#elif BITS_PER_LONG == 32
> > +#define __AMO(op)   "amo" #op ".w"
> > +#else
> > +#error "Unexpected BITS_PER_LONG"
> > +#endif
> > +
> > +#define __set_bit(n, p)      set_bit(n, p)
> > +#define __clear_bit(n, p)    clear_bit(n, p)
> 
> First without comment and then ...
> 
> > +/* Based on linux/arch/include/asm/bitops.h */
> > +
> > +/*
> > + * Non-atomic bit manipulation.
> > + *
> > + * Implemented using atomics to be interrupt safe. Could
> > alternatively
> > + * implement with local interrupt masking.
> > + */
> > +#define __set_bit(n, p)      set_bit(n, p)
> > +#define __clear_bit(n, p)    clear_bit(n, p)
> 
> ... with one?
Hmm, it seems like rebasing issue with autoconflict resolving. It
should be only definitions with the comment.

> 
> > +/* Based on linux/arch/include/asm/bitops.h */
> 
> Does this really need repeating?
> 
> > +#define test_and_op_bit_ord(op, mod, nr, addr, ord)     \
> > +({                                                      \
> > +    unsigned long res, mask;                            \
> 
> bitop_uint_t?
> 
> > +    mask = BITOP_MASK(nr);                              \
> > +    asm volatile (                                       \
> 
> Nit: One too many padding blanks.
> 
> > +        __AMO(op) #ord " %0, %2, %1"                    \
> > +        : "=r" (res), "+A" (addr[BITOP_WORD(nr)])       \
> > +        : "r" (mod(mask))                               \
> > +        : "memory");                                    \
> > +    ((res & mask) != 0);                                \
> > +})
> > +
> > +#define op_bit_ord(op, mod, nr, addr, ord)      \
> > +    asm volatile (                              \
> > +        __AMO(op) #ord " zero, %1, %0"          \
> > +        : "+A" (addr[BITOP_WORD(nr)])           \
> > +        : "r" (mod(BITOP_MASK(nr)))             \
> > +        : "memory");
> > +
> > +#define test_and_op_bit(op, mod, nr, addr)    \
> > +    test_and_op_bit_ord(op, mod, nr, addr, .aqrl)
> > +#define op_bit(op, mod, nr, addr) \
> > +    op_bit_ord(op, mod, nr, addr, )
> > +
> > +/* Bitmask modifiers */
> > +#define NOP(x)    (x)
> > +#define NOT(x)    (~(x))
> > +
> > +/**
> > + * test_and_set_bit - Set a bit and return its old value
> > + * @nr: Bit to set
> > + * @addr: Address to count from
> > + */
> > +static inline int test_and_set_bit(int nr, volatile void *p)
> 
> In patch 4 you switched to bool. Any reason you didn't here, too?
Sure, it should return bool here too and probably some other functions
below.

> 
> > +{
> > +    volatile bitop_uint_t *addr = p;
> > +
> > +    return test_and_op_bit(or, NOP, nr, addr);
> > +}
> > +
> > +/**
> > + * test_and_clear_bit - Clear a bit and return its old value
> > + * @nr: Bit to clear
> > + * @addr: Address to count from
> > + */
> > +static inline int test_and_clear_bit(int nr, volatile void *p)
> > +{
> > +    volatile bitop_uint_t *addr = p;
> > +
> > +    return test_and_op_bit(and, NOT, nr, addr);
> > +}
> > +
> > +/**
> > + * set_bit - Atomically set a bit in memory
> > + * @nr: the bit to set
> > + * @addr: the address to start counting from
> > + *
> > + * Note that @nr may be almost arbitrarily large; this function is
> > not
> > + * restricted to acting on a single-word quantity.
> > + */
> > +static inline void set_bit(int nr, volatile void *p)
> > +{
> > +    volatile bitop_uint_t *addr = p;
> > +
> > +    op_bit(or, NOP, nr, addr);
> > +}
> > +
> > +/**
> > + * clear_bit - Clears a bit in memory
> > + * @nr: Bit to clear
> > + * @addr: Address to start counting from
> > + */
> > +static inline void clear_bit(int nr, volatile void *p)
> > +{
> > +    volatile bitop_uint_t *addr = p;
> > +
> > +    op_bit(and, NOT, nr, addr);
> > +}
> > +
> > +/**
> > + * test_and_change_bit - Toggle (change) a bit and return its old
> > value
> > + * @nr: Bit to change
> > + * @addr: Address to count from
> > + *
> > + * This operation is atomic and cannot be reordered.
> > + * It also implies a memory barrier.
> > + */
> > +static inline int test_and_change_bit(int nr, volatile unsigned
> > long *addr)
> 
> unsigned long?
It should be volatile void *. Something that was copied from Linux
kernel and I missed to change.

~ Oleksii

> 
> > +{
> > +	return test_and_op_bit(xor, NOP, nr, addr);
> > +}
> 
> Perhaps better to move up a little, next to the other test_and-s?
> 
> Also - nit: Hard tab used for indentation.
> 
> > +#undef test_and_op_bit
> > +#undef __op_bit
> 
> This no longer has any effect in this shape.
> 
> Jan

Patch

diff --git a/xen/arch/riscv/include/asm/bitops.h b/xen/arch/riscv/include/asm/bitops.h
new file mode 100644
index 0000000000..6f0212e5ac
--- /dev/null
+++ b/xen/arch/riscv/include/asm/bitops.h
@@ -0,0 +1,146 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2012 Regents of the University of California */
+
+#ifndef _ASM_RISCV_BITOPS_H
+#define _ASM_RISCV_BITOPS_H
+
+#include <asm/system.h>
+
+#undef BITOP_BITS_PER_WORD
+#undef bitop_uint_t
+
+#define BITOP_BITS_PER_WORD BITS_PER_LONG
+#define bitop_uint_t unsigned long
+
+#if BITS_PER_LONG == 64
+#define __AMO(op)   "amo" #op ".d"
+#elif BITS_PER_LONG == 32
+#define __AMO(op)   "amo" #op ".w"
+#else
+#error "Unexpected BITS_PER_LONG"
+#endif
+
+#define __set_bit(n, p)      set_bit(n, p)
+#define __clear_bit(n, p)    clear_bit(n, p)
+
+/* Based on linux/arch/include/asm/bitops.h */
+
+/*
+ * Non-atomic bit manipulation.
+ *
+ * Implemented using atomics to be interrupt safe. Could alternatively
+ * implement with local interrupt masking.
+ */
+#define __set_bit(n, p)      set_bit(n, p)
+#define __clear_bit(n, p)    clear_bit(n, p)
+
+/* Based on linux/arch/include/asm/bitops.h */
+
+#define test_and_op_bit_ord(op, mod, nr, addr, ord)     \
+({                                                      \
+    unsigned long res, mask;                            \
+    mask = BITOP_MASK(nr);                              \
+    asm volatile (                                       \
+        __AMO(op) #ord " %0, %2, %1"                    \
+        : "=r" (res), "+A" (addr[BITOP_WORD(nr)])       \
+        : "r" (mod(mask))                               \
+        : "memory");                                    \
+    ((res & mask) != 0);                                \
+})
+
+#define op_bit_ord(op, mod, nr, addr, ord)      \
+    asm volatile (                              \
+        __AMO(op) #ord " zero, %1, %0"          \
+        : "+A" (addr[BITOP_WORD(nr)])           \
+        : "r" (mod(BITOP_MASK(nr)))             \
+        : "memory");
+
+#define test_and_op_bit(op, mod, nr, addr)    \
+    test_and_op_bit_ord(op, mod, nr, addr, .aqrl)
+#define op_bit(op, mod, nr, addr) \
+    op_bit_ord(op, mod, nr, addr, )
+
+/* Bitmask modifiers */
+#define NOP(x)    (x)
+#define NOT(x)    (~(x))
+
+/**
+ * test_and_set_bit - Set a bit and return its old value
+ * @nr: Bit to set
+ * @addr: Address to count from
+ */
+static inline int test_and_set_bit(int nr, volatile void *p)
+{
+    volatile bitop_uint_t *addr = p;
+
+    return test_and_op_bit(or, NOP, nr, addr);
+}
+
+/**
+ * test_and_clear_bit - Clear a bit and return its old value
+ * @nr: Bit to clear
+ * @addr: Address to count from
+ */
+static inline int test_and_clear_bit(int nr, volatile void *p)
+{
+    volatile bitop_uint_t *addr = p;
+
+    return test_and_op_bit(and, NOT, nr, addr);
+}
+
+/**
+ * set_bit - Atomically set a bit in memory
+ * @nr: the bit to set
+ * @addr: the address to start counting from
+ *
+ * Note that @nr may be almost arbitrarily large; this function is not
+ * restricted to acting on a single-word quantity.
+ */
+static inline void set_bit(int nr, volatile void *p)
+{
+    volatile bitop_uint_t *addr = p;
+
+    op_bit(or, NOP, nr, addr);
+}
+
+/**
+ * clear_bit - Clears a bit in memory
+ * @nr: Bit to clear
+ * @addr: Address to start counting from
+ */
+static inline void clear_bit(int nr, volatile void *p)
+{
+    volatile bitop_uint_t *addr = p;
+
+    op_bit(and, NOT, nr, addr);
+}
+
+/**
+ * test_and_change_bit - Toggle (change) a bit and return its old value
+ * @nr: Bit to change
+ * @addr: Address to count from
+ *
+ * This operation is atomic and cannot be reordered.
+ * It also implies a memory barrier.
+ */
+static inline int test_and_change_bit(int nr, volatile unsigned long *addr)
+{
+	return test_and_op_bit(xor, NOP, nr, addr);
+}
+
+#undef test_and_op_bit
+#undef __op_bit
+#undef NOP
+#undef NOT
+#undef __AMO
+
+#endif /* _ASM_RISCV_BITOPS_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */