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/arch/include/linux/bits.h ( taken only definitions for BIT_MASK,
BIT_WORD, BITS_PER_BYTE )
* 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.
* update function prototypes from
generic___test_and_*(unsigned long nr nr, volatile unsigned long *addr)
to
generic___test_and_*(unsigned long nr, volatile void *addr) to be
consistent with other related macros/defines.
* convert identations from tabs to spaces.
* inside generic__test_and_* use 'bitops_uint_t' instead of 'unsigned long'
to be generic.
Additionaly, the following bit ops are introduced:
* __ffs
* ffsl
* fls
* flsl
* ffs
* ffz
* find_first_bit_set
* hweight64
* test_bit
Some of the introduced bit operations are included in asm-generic,
as they exhibit similarity across multiple architectures.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V4:
- updated the commit message: dropped the message about what was taken from linux/include/asm-generic/bitops/find.h
as related changes now are located in xen/bitops.h. Also these changes were removed from riscv/bitops.h
- switch tabs to spaces.
- update return type of __ffs function, format __ffs according to Xen code style. Move the function to
respective asm-generic header.
- format ffsl() according to Xen code style, update the type of num: int -> unsigned to be align with
return type of the function. Move the function to respective asm-generic header.
- add new line for the files:
asm-generic/bitops-bits.h
asm-generic/ffz.h
asm-generic/find-first-bit-set.h
asm-generic/fls.h
asm-generic/flsl.h
asm-generic/test-bit.h
- rename asm-generic/find-first-bit-set.h to asm-generic/find-first-set-bit.h to be aligned with the function
name implemented inside.
- introduce generic___test_and*() operation for non-atomic bitops.
- rename current __test_and_*() -> test_and_*() as their implementation are atomic aware.
- define __test_and_*() to generic___test_and_*().
- introduce test_and_change_bit().
- update asm-generic/bitops/bitops-bits.h to give possoibility to change BITOP_*() macros by architecture.
Also, it was introduced bitops_uint_t type to make generic___test_and_*() generic.
- "include asm-generic/bitops/bitops-bits.h" to files which use its definitions.
- add comment why generic ffz is defined as __ffs().
- update the commit message.
- swtich ffsl() to generic_ffsl().
---
Changes in V3:
- update the commit message
- Introduce the following asm-generic bitops headers:
create mode 100644 xen/arch/riscv/include/asm/bitops.h
create mode 100644 xen/include/asm-generic/bitops/bitops-bits.h
create mode 100644 xen/include/asm-generic/bitops/ffs.h
create mode 100644 xen/include/asm-generic/bitops/ffz.h
create mode 100644 xen/include/asm-generic/bitops/find-first-bit-set.h
create mode 100644 xen/include/asm-generic/bitops/fls.h
create mode 100644 xen/include/asm-generic/bitops/flsl.h
create mode 100644 xen/include/asm-generic/bitops/hweight.h
create mode 100644 xen/include/asm-generic/bitops/test-bit.h
- switch some bitops functions to asm-generic's versions.
- re-sync some macros with Linux kernel version mentioned in the commit message.
- Xen code style fixes.
---
Changes in V2:
- Nothing changed. Only rebase.
---
xen/arch/riscv/include/asm/bitops.h | 164 ++++++++++++++++++
xen/arch/riscv/include/asm/config.h | 2 +
xen/include/asm-generic/bitops/__ffs.h | 47 +++++
xen/include/asm-generic/bitops/bitops-bits.h | 21 +++
xen/include/asm-generic/bitops/ffs.h | 9 +
xen/include/asm-generic/bitops/ffsl.h | 16 ++
xen/include/asm-generic/bitops/ffz.h | 18 ++
.../asm-generic/bitops/find-first-set-bit.h | 17 ++
xen/include/asm-generic/bitops/fls.h | 18 ++
xen/include/asm-generic/bitops/flsl.h | 10 ++
.../asm-generic/bitops/generic-non-atomic.h | 89 ++++++++++
xen/include/asm-generic/bitops/hweight.h | 13 ++
xen/include/asm-generic/bitops/test-bit.h | 18 ++
13 files changed, 442 insertions(+)
create mode 100644 xen/arch/riscv/include/asm/bitops.h
create mode 100644 xen/include/asm-generic/bitops/__ffs.h
create mode 100644 xen/include/asm-generic/bitops/bitops-bits.h
create mode 100644 xen/include/asm-generic/bitops/ffs.h
create mode 100644 xen/include/asm-generic/bitops/ffsl.h
create mode 100644 xen/include/asm-generic/bitops/ffz.h
create mode 100644 xen/include/asm-generic/bitops/find-first-set-bit.h
create mode 100644 xen/include/asm-generic/bitops/fls.h
create mode 100644 xen/include/asm-generic/bitops/flsl.h
create mode 100644 xen/include/asm-generic/bitops/generic-non-atomic.h
create mode 100644 xen/include/asm-generic/bitops/hweight.h
create mode 100644 xen/include/asm-generic/bitops/test-bit.h
diff --git a/xen/arch/riscv/include/asm/bitops.h b/xen/arch/riscv/include/asm/bitops.h
new file mode 100644
index 0000000000..1225298d35
--- /dev/null
+++ b/xen/arch/riscv/include/asm/bitops.h
@@ -0,0 +1,164 @@
+/* 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>
+
+#include <asm-generic/bitops/bitops-bits.h>
+
+/* Based on linux/arch/include/linux/bits.h */
+
+#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG))
+#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
+
+#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 */
+
+#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 __test_and_op_bit_ord(op, mod, nr, addr, ord) \
+({ \
+ unsigned long __res, __mask; \
+ __mask = BIT_MASK(nr); \
+ __asm__ __volatile__ ( \
+ __AMO(op) #ord " %0, %2, %1" \
+ : "=r" (__res), "+A" (addr[BIT_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[BIT_WORD(nr)]) \
+ : "r" (mod(BIT_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
+ *
+ * This operation may be reordered on other architectures than x86.
+ */
+static inline int test_and_set_bit(int nr, volatile void *p)
+{
+ volatile uint32_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
+ *
+ * This operation can be reordered on other architectures other than x86.
+ */
+static inline int test_and_clear_bit(int nr, volatile void *p)
+{
+ volatile uint32_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: there are no guarantees that this function will not be reordered
+ * on non x86 architectures, so if you are writing portable code,
+ * make sure not to rely on its reordering guarantees.
+ *
+ * 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 uint32_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
+ *
+ * Note: there are no guarantees that this function will not be reordered
+ * on non x86 architectures, so if you are writing portable code,
+ * make sure not to rely on its reordering guarantees.
+ */
+static inline void clear_bit(int nr, volatile void *p)
+{
+ volatile uint32_t *addr = p;
+
+ __op_bit(and, __NOT, nr, addr);
+}
+
+/**
+ * test_and_change_bit - 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
+
+#include <asm-generic/bitops/generic-non-atomic.h>
+
+#define __test_and_set_bit generic___test_and_set_bit
+#define __test_and_clear_bit generic___test_and_clear_bit
+#define __test_and_change_bit generic___test_and_change_bit
+
+#include <asm-generic/bitops/fls.h>
+#include <asm-generic/bitops/flsl.h>
+#include <asm-generic/bitops/__ffs.h>
+#include <asm-generic/bitops/ffs.h>
+#include <asm-generic/bitops/ffsl.h>
+#include <asm-generic/bitops/ffz.h>
+#include <asm-generic/bitops/find-first-set-bit.h>
+#include <asm-generic/bitops/hweight.h>
+#include <asm-generic/bitops/test-bit.h>
+
+#endif /* _ASM_RISCV_BITOPS_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/riscv/include/asm/config.h b/xen/arch/riscv/include/asm/config.h
index a80cdd4f85..56387ac159 100644
--- a/xen/arch/riscv/include/asm/config.h
+++ b/xen/arch/riscv/include/asm/config.h
@@ -50,6 +50,8 @@
# error "Unsupported RISCV variant"
#endif
+#define BITS_PER_BYTE 8
+
#define BYTES_PER_LONG (1 << LONG_BYTEORDER)
#define BITS_PER_LONG (BYTES_PER_LONG << 3)
#define POINTER_ALIGN BYTES_PER_LONG
diff --git a/xen/include/asm-generic/bitops/__ffs.h b/xen/include/asm-generic/bitops/__ffs.h
new file mode 100644
index 0000000000..fecb4484d9
--- /dev/null
+++ b/xen/include/asm-generic/bitops/__ffs.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_GENERIC_BITOPS___FFS_H_
+#define _ASM_GENERIC_BITOPS___FFS_H_
+
+/**
+ * ffs - find first bit in word.
+ * @word: The word to search
+ *
+ * Returns 0 if no bit exists, otherwise returns 1-indexed bit location.
+ */
+static inline unsigned int __ffs(unsigned long word)
+{
+ unsigned int num = 0;
+
+#if BITS_PER_LONG == 64
+ if ( (word & 0xffffffff) == 0 )
+ {
+ num += 32;
+ word >>= 32;
+ }
+#endif
+ if ( (word & 0xffff) == 0 )
+ {
+ num += 16;
+ word >>= 16;
+ }
+ if ( (word & 0xff) == 0 )
+ {
+ num += 8;
+ word >>= 8;
+ }
+ if ( (word & 0xf) == 0 )
+ {
+ num += 4;
+ word >>= 4;
+ }
+ if ( (word & 0x3) == 0 )
+ {
+ num += 2;
+ word >>= 2;
+ }
+ if ( (word & 0x1) == 0 )
+ num += 1;
+ return num;
+}
+
+#endif /* _ASM_GENERIC_BITOPS___FFS_H_ */
diff --git a/xen/include/asm-generic/bitops/bitops-bits.h b/xen/include/asm-generic/bitops/bitops-bits.h
new file mode 100644
index 0000000000..4ece2affd6
--- /dev/null
+++ b/xen/include/asm-generic/bitops/bitops-bits.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_GENERIC_BITOPS_BITS_H_
+#define _ASM_GENERIC_BITOPS_BITS_H_
+
+#ifndef BITOP_BITS_PER_WORD
+#define BITOP_BITS_PER_WORD 32
+#endif
+
+#ifndef BITOP_MASK
+#define BITOP_MASK(nr) (1U << ((nr) % BITOP_BITS_PER_WORD))
+#endif
+
+#ifndef BITOP_WORD
+#define BITOP_WORD(nr) ((nr) / BITOP_BITS_PER_WORD)
+#endif
+
+#ifndef BITOP_TYPE
+typedef uint32_t bitops_uint_t;
+#endif
+
+#endif /* _ASM_GENERIC_BITOPS_BITS_H_ */
diff --git a/xen/include/asm-generic/bitops/ffs.h b/xen/include/asm-generic/bitops/ffs.h
new file mode 100644
index 0000000000..3f75fded14
--- /dev/null
+++ b/xen/include/asm-generic/bitops/ffs.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_GENERIC_BITOPS_FFS_H_
+#define _ASM_GENERIC_BITOPS_FFS_H_
+
+#include <xen/macros.h>
+
+#define ffs(x) ({ unsigned int t_ = (x); fls(ISOLATE_LSB(t_)); })
+
+#endif /* _ASM_GENERIC_BITOPS_FFS_H_ */
diff --git a/xen/include/asm-generic/bitops/ffsl.h b/xen/include/asm-generic/bitops/ffsl.h
new file mode 100644
index 0000000000..d0996808f5
--- /dev/null
+++ b/xen/include/asm-generic/bitops/ffsl.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_GENERIC_BITOPS_FFSL_H_
+#define _ASM_GENERIC_BITOPS_FFSL_H_
+
+/**
+ * ffsl - find first bit in long.
+ * @word: The word to search
+ *
+ * Returns 0 if no bit exists, otherwise returns 1-indexed bit location.
+ */
+static inline unsigned int ffsl(unsigned long word)
+{
+ return generic_ffsl(word);
+}
+
+#endif /* _ASM_GENERIC_BITOPS_FFSL_H_ */
diff --git a/xen/include/asm-generic/bitops/ffz.h b/xen/include/asm-generic/bitops/ffz.h
new file mode 100644
index 0000000000..5932fe6695
--- /dev/null
+++ b/xen/include/asm-generic/bitops/ffz.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_GENERIC_BITOPS_FFZ_H_
+#define _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.
+ *
+ * ffz() is defined as __ffs() and not as ffs() as it is defined in such
+ * a way in Linux kernel (6.4.0 ) from where this header was taken, so this
+ * header is supposed to be aligned with Linux kernel version.
+ * Also, most architectures are defined in the same way in Xen.
+ */
+#define ffz(x) __ffs(~(x))
+
+#endif /* _ASM_GENERIC_BITOPS_FFZ_H_ */
diff --git a/xen/include/asm-generic/bitops/find-first-set-bit.h b/xen/include/asm-generic/bitops/find-first-set-bit.h
new file mode 100644
index 0000000000..7d28b8a89b
--- /dev/null
+++ b/xen/include/asm-generic/bitops/find-first-set-bit.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_GENERIC_BITOPS_FIND_FIRST_SET_BIT_H_
+#define _ASM_GENERIC_BITOPS_FIND_FIRST_SET_BIT_H_
+
+/**
+ * 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.
+ */
+static inline unsigned int find_first_set_bit(unsigned long word)
+{
+ return ffsl(word) - 1;
+}
+
+#endif /* _ASM_GENERIC_BITOPS_FIND_FIRST_SET_BIT_H_ */
diff --git a/xen/include/asm-generic/bitops/fls.h b/xen/include/asm-generic/bitops/fls.h
new file mode 100644
index 0000000000..369a4c790c
--- /dev/null
+++ b/xen/include/asm-generic/bitops/fls.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_GENERIC_BITOPS_FLS_H_
+#define _ASM_GENERIC_BITOPS_FLS_H_
+
+/**
+ * fls - find last (most-significant) bit set
+ * @x: the word to search
+ *
+ * This is defined the same way as ffs.
+ * Note fls(0) = 0, fls(1) = 1, fls(0x80000000) = 32.
+ */
+
+static inline int fls(unsigned int x)
+{
+ return generic_fls(x);
+}
+
+#endif /* _ASM_GENERIC_BITOPS_FLS_H_ */
diff --git a/xen/include/asm-generic/bitops/flsl.h b/xen/include/asm-generic/bitops/flsl.h
new file mode 100644
index 0000000000..d0a2e9c729
--- /dev/null
+++ b/xen/include/asm-generic/bitops/flsl.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_GENERIC_BITOPS_FLSL_H_
+#define _ASM_GENERIC_BITOPS_FLSL_H_
+
+static inline int flsl(unsigned long x)
+{
+ return generic_flsl(x);
+}
+
+#endif /* _ASM_GENERIC_BITOPS_FLSL_H_ */
diff --git a/xen/include/asm-generic/bitops/generic-non-atomic.h b/xen/include/asm-generic/bitops/generic-non-atomic.h
new file mode 100644
index 0000000000..07efca245e
--- /dev/null
+++ b/xen/include/asm-generic/bitops/generic-non-atomic.h
@@ -0,0 +1,89 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * The file is based on Linux ( 6.4.0 ) header:
+ * include/asm-generic/bitops/generic-non-atomic.h
+ *
+ * Only functions that can be reused in Xen were left; others were removed.
+ *
+ * Also, the following changes were done:
+ * - it was updated the message inside #ifndef ... #endif.
+ * - __always_inline -> always_inline to be align with definition in
+ * xen/compiler.h.
+ * - update function prototypes from
+ * generic___test_and_*(unsigned long nr nr, volatile unsigned long *addr) to
+ * generic___test_and_*(unsigned long nr, volatile void *addr) to be
+ * consistent with other related macros/defines.
+ * - convert identations from tabs to spaces.
+ * - inside generic__test_and_* use 'bitops_uint_t' instead of 'unsigned long'
+ * to be generic.
+ */
+
+#ifndef __ASM_GENERIC_BITOPS_GENERIC_NON_ATOMIC_H
+#define __ASM_GENERIC_BITOPS_GENERIC_NON_ATOMIC_H
+
+#include <xen/compiler.h>
+
+#include <asm-generic/bitops/bitops-bits.h>
+
+#ifndef _LINUX_BITOPS_H
+#error only <xen/bitops.h> can be included directly
+#endif
+
+/*
+ * Generic definitions for bit operations, should not be used in regular code
+ * directly.
+ */
+
+/**
+ * generic___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 always_inline bool
+generic___test_and_set_bit(unsigned long nr, volatile void *addr)
+{
+ bitops_uint_t mask = BIT_MASK(nr);
+ bitops_uint_t *p = ((bitops_uint_t *)addr) + BIT_WORD(nr);
+ bitops_uint_t old = *p;
+
+ *p = old | mask;
+ return (old & mask) != 0;
+}
+
+/**
+ * generic___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 always_inline bool
+generic___test_and_clear_bit(bitops_uint_t nr, volatile void *addr)
+{
+ bitops_uint_t mask = BIT_MASK(nr);
+ bitops_uint_t *p = ((bitops_uint_t *)addr) + BIT_WORD(nr);
+ bitops_uint_t old = *p;
+
+ *p = old & ~mask;
+ return (old & mask) != 0;
+}
+
+/* WARNING: non atomic and it can be reordered! */
+static always_inline bool
+generic___test_and_change_bit(unsigned long nr, volatile void *addr)
+{
+ bitops_uint_t mask = BIT_MASK(nr);
+ bitops_uint_t *p = ((bitops_uint_t *)addr) + BIT_WORD(nr);
+ bitops_uint_t old = *p;
+
+ *p = old ^ mask;
+ return (old & mask) != 0;
+}
+
+#endif /* __ASM_GENERIC_BITOPS_GENERIC_NON_ATOMIC_H */
diff --git a/xen/include/asm-generic/bitops/hweight.h b/xen/include/asm-generic/bitops/hweight.h
new file mode 100644
index 0000000000..0d7577054e
--- /dev/null
+++ b/xen/include/asm-generic/bitops/hweight.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_GENERIC_BITOPS_HWEIGHT_H_
+#define _ASM_GENERIC_BITOPS_HWEIGHT_H_
+
+/*
+ * 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)
+
+#endif /* _ASM_GENERIC_BITOPS_HWEIGHT_H_ */
diff --git a/xen/include/asm-generic/bitops/test-bit.h b/xen/include/asm-generic/bitops/test-bit.h
new file mode 100644
index 0000000000..6fb414d808
--- /dev/null
+++ b/xen/include/asm-generic/bitops/test-bit.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_GENERIC_BITOPS_TESTBIT_H_
+#define _ASM_GENERIC_BITOPS_TESTBIT_H_
+
+#include <asm-generic/bitops/bitops-bits.h>
+
+/**
+ * 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 bitops_uint_t *p = addr;
+ return 1 & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD - 1)));
+}
+
+#endif /* _ASM_GENERIC_BITOPS_TESTBIT_H_ */
--
2.43.0
On 05.02.2024 16:32, Oleksii Kurochko wrote: > --- a/xen/arch/riscv/include/asm/config.h > +++ b/xen/arch/riscv/include/asm/config.h > @@ -50,6 +50,8 @@ > # error "Unsupported RISCV variant" > #endif > > +#define BITS_PER_BYTE 8 > + > #define BYTES_PER_LONG (1 << LONG_BYTEORDER) > #define BITS_PER_LONG (BYTES_PER_LONG << 3) > #define POINTER_ALIGN BYTES_PER_LONG How does this change relate to this patch? I can't see the new symbol being used anywhere. > --- /dev/null > +++ b/xen/include/asm-generic/bitops/__ffs.h > @@ -0,0 +1,47 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _ASM_GENERIC_BITOPS___FFS_H_ > +#define _ASM_GENERIC_BITOPS___FFS_H_ > + > +/** > + * ffs - find first bit in word. __ffs ? Or wait, ... > + * @word: The word to search > + * > + * Returns 0 if no bit exists, otherwise returns 1-indexed bit location. ... this actually describes ffs(), not __ffs(), and the implementation doesn't match the description. The correct description for this function (as Linux also has it) * Undefined if no bit exists, so code should check against 0 first. Which raises a question regarding "Taken from Linux-6.4.0-rc1" in the description. ffs.h pretty clearly also doesn't come from there. I first I thought I might withdraw my earlier request to split all of this up. But with just these two observations I now feel it's even more important that you do, so every piece can be properly attributed to (and then checked for) its origin. Jan
On 05.02.2024 16:32, Oleksii Kurochko wrote: > --- /dev/null > +++ b/xen/arch/riscv/include/asm/bitops.h > @@ -0,0 +1,164 @@ > +/* 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> > + > +#include <asm-generic/bitops/bitops-bits.h> Especially with ... > +/* Based on linux/arch/include/linux/bits.h */ > + > +#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG)) > +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) ... these it's not entirely obvious why bitops-bits.h would be needed here. > +#define __set_bit(n,p) set_bit(n,p) > +#define __clear_bit(n,p) clear_bit(n,p) Nit (as before?): Missing blanks after commas. > +/* Based on linux/arch/include/asm/bitops.h */ > + > +#if ( BITS_PER_LONG == 64 ) Imo the parentheses here make things only harder to read. > +#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 __test_and_op_bit_ord(op, mod, nr, addr, ord) \ The revision log says __test_and_* were renamed. Same anomaly for __test_and_op_bit() then. > +({ \ > + unsigned long __res, __mask; \ Leftover leading underscores? > + __mask = BIT_MASK(nr); \ > + __asm__ __volatile__ ( \ > + __AMO(op) #ord " %0, %2, %1" \ > + : "=r" (__res), "+A" (addr[BIT_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[BIT_WORD(nr)]) \ > + : "r" (mod(BIT_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)) Here the (double) leading underscores are truly worrying: Simple names like this aren't impossible to be assigned meaninb by a compiler. > +/** > + * __test_and_set_bit - Set a bit and return its old value > + * @nr: Bit to set > + * @addr: Address to count from > + * > + * This operation may be reordered on other architectures than x86. > + */ > +static inline int test_and_set_bit(int nr, volatile void *p) > +{ > + volatile uint32_t *addr = p; With BIT_WORD() / BIT_MASK() being long-based, is the use of uint32_t here actually correct? > + 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 > + * > + * This operation can be reordered on other architectures other than x86. Nit: double "other" (and I think it's the 1st one that wants dropping, not - as the earlier comment suggests - the 2nd one). Question is: Are the comments correct? Both resolve to something which is (also) at least a compiler barrier. Same concern also applies further down, to at least set_bit() and clear_bit(). > + */ > +static inline int test_and_clear_bit(int nr, volatile void *p) > +{ > + volatile uint32_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: there are no guarantees that this function will not be reordered > + * on non x86 architectures, so if you are writing portable code, > + * make sure not to rely on its reordering guarantees. > + * > + * 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 uint32_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 > + * > + * Note: there are no guarantees that this function will not be reordered > + * on non x86 architectures, so if you are writing portable code, > + * make sure not to rely on its reordering guarantees. > + */ > +static inline void clear_bit(int nr, volatile void *p) > +{ > + volatile uint32_t *addr = p; > + > + __op_bit(and, __NOT, nr, addr); > +} > + > +/** > + * test_and_change_bit - Change a bit and return its old value How come this one's different? I notice the comments are the same (and hence as confusing) in Linux; are you sure they're applicable there? > + * @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 > + > +#include <asm-generic/bitops/generic-non-atomic.h> > + > +#define __test_and_set_bit generic___test_and_set_bit > +#define __test_and_clear_bit generic___test_and_clear_bit > +#define __test_and_change_bit generic___test_and_change_bit > + > +#include <asm-generic/bitops/fls.h> > +#include <asm-generic/bitops/flsl.h> > +#include <asm-generic/bitops/__ffs.h> > +#include <asm-generic/bitops/ffs.h> > +#include <asm-generic/bitops/ffsl.h> > +#include <asm-generic/bitops/ffz.h> > +#include <asm-generic/bitops/find-first-set-bit.h> > +#include <asm-generic/bitops/hweight.h> > +#include <asm-generic/bitops/test-bit.h> To be honest there's too much stuff being added here to asm-generic/, all in one go. I'll see about commenting on the remaining parts here, but I'd like to ask that you seriously consider splitting. Jan
On Mon, 2024-02-12 at 16:58 +0100, Jan Beulich wrote: > On 05.02.2024 16:32, Oleksii Kurochko wrote: > > --- /dev/null > > +++ b/xen/arch/riscv/include/asm/bitops.h > > @@ -0,0 +1,164 @@ > > +/* 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> > > + > > +#include <asm-generic/bitops/bitops-bits.h> > > Especially with ... > > > +/* Based on linux/arch/include/linux/bits.h */ > > + > > +#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG)) > > +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) > > ... these it's not entirely obvious why bitops-bits.h would be needed > here. They are needed as __test_and_op_bit_ord(), __op_bit_ord() macros are used them, but probably it makes sense to drop BIT_MASK() and BIT_WORD(), and just use BITOPS_MASK() and BITOPS_WORD() from asm- generic/bitops-bits.h or re-define BITOPS_MASK() and BITOPS_WORD() before inclusion of bitops-bits.h in the way as BIT_MASK and BIT_WORD macros are defined now to be aligned with Linux. > > > +#define __set_bit(n,p) set_bit(n,p) > > +#define __clear_bit(n,p) clear_bit(n,p) > > Nit (as before?): Missing blanks after commas. Thanks. I'll add blanks. > > > +/* Based on linux/arch/include/asm/bitops.h */ > > + > > +#if ( BITS_PER_LONG == 64 ) > > Imo the parentheses here make things only harder to read. I can drop them, this part was copied from Linux, so it was decided to leave it as is. > > > +#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 __test_and_op_bit_ord(op, mod, nr, addr, ord) \ > > The revision log says __test_and_* were renamed. Same anomaly for > __test_and_op_bit() then. I'll double check the namings. Thanks. > > > +({ \ > > + unsigned long __res, __mask; \ > > Leftover leading underscores? It is how it was defined in Linux, so I thought that I've to leave it as it, but I am OK to rename this variables in next patch version. > > > + __mask = BIT_MASK(nr); \ > > + __asm__ __volatile__ ( \ > > + __AMO(op) #ord " %0, %2, %1" \ > > + : "=r" (__res), "+A" (addr[BIT_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[BIT_WORD(nr)]) \ > > + : "r" (mod(BIT_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)) > > Here the (double) leading underscores are truly worrying: Simple > names like this aren't impossible to be assigned meaninb by a > compiler. I am not really understand what is the difference for a compiler between NOP and __NOP? Do you mean that the leading double underscores (__) are often used to indicate that these macros are implementation- specific and might be reserved for the compiler or the standard library? > > > +/** > > + * __test_and_set_bit - Set a bit and return its old value > > + * @nr: Bit to set > > + * @addr: Address to count from > > + * > > + * This operation may be reordered on other architectures than > > x86. > > + */ > > +static inline int test_and_set_bit(int nr, volatile void *p) > > +{ > > + volatile uint32_t *addr = p; > > With BIT_WORD() / BIT_MASK() being long-based, is the use of uint32_t > here actually correct? No, it is not correct. It seems to me it would be better to use BITOPS_WORD(), BITOPS_MASK() and bitops_uint_t() and just redefine them before inclusion of bitops-bit.h to be aligned with Linux implementation. > > > + 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 > > + * > > + * This operation can be reordered on other architectures other > > than x86. > > Nit: double "other" (and I think it's the 1st one that wants > dropping, > not - as the earlier comment suggests - the 2nd one). Question is: > Are > the comments correct? Both resolve to something which is (also) at > least a compiler barrier. Same concern also applies further down, to > at least set_bit() and clear_bit(). It looks like comments aren't correct as operation inside is atomic, also it implies compiler memory barrier. So the comments related to 'reordering' should be dropped. I am not sure that I know why in Linux these comments were left. > > > + */ > > +static inline int test_and_clear_bit(int nr, volatile void *p) > > +{ > > + volatile uint32_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: there are no guarantees that this function will not be > > reordered > > + * on non x86 architectures, so if you are writing portable code, > > + * make sure not to rely on its reordering guarantees. > > + * > > + * 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 uint32_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 > > + * > > + * Note: there are no guarantees that this function will not be > > reordered > > + * on non x86 architectures, so if you are writing portable code, > > + * make sure not to rely on its reordering guarantees. > > + */ > > +static inline void clear_bit(int nr, volatile void *p) > > +{ > > + volatile uint32_t *addr = p; > > + > > + __op_bit(and, __NOT, nr, addr); > > +} > > + > > +/** > > + * test_and_change_bit - Change a bit and return its old value > > How come this one's different? I notice the comments are the same > (and > hence as confusing) in Linux; are you sure they're applicable there? No, I am not sure. As I mentioned above, all this functions are atomic and uses compiler memory barrier, so it looks like the comment for clear_bit isn't really correct. In case if all such functions are uses __test_and_op_bit_ord() and __op_bit_ord() which are atomic and with compiler barrier, it seems that we can just drop all such comments or even all the comments. I am not sure that anyone is needed the comment as by default this function is safe to use: * This operation is atomic and cannot be reordered. * It also implies a memory barrier. > > > + * @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 > > + > > +#include <asm-generic/bitops/generic-non-atomic.h> > > + > > +#define __test_and_set_bit generic___test_and_set_bit > > +#define __test_and_clear_bit generic___test_and_clear_bit > > +#define __test_and_change_bit generic___test_and_change_bit > > + > > +#include <asm-generic/bitops/fls.h> > > +#include <asm-generic/bitops/flsl.h> > > +#include <asm-generic/bitops/__ffs.h> > > +#include <asm-generic/bitops/ffs.h> > > +#include <asm-generic/bitops/ffsl.h> > > +#include <asm-generic/bitops/ffz.h> > > +#include <asm-generic/bitops/find-first-set-bit.h> > > +#include <asm-generic/bitops/hweight.h> > > +#include <asm-generic/bitops/test-bit.h> > > To be honest there's too much stuff being added here to asm-generic/, > all in one go. I'll see about commenting on the remaining parts here, > but I'd like to ask that you seriously consider splitting. Would it be better to send it outside of this patch series? I can create a separate patch series with a separate patch for each asm- generic/bitops/*.h ~ Oleksii
On 14.02.2024 12:06, Oleksii wrote: > On Mon, 2024-02-12 at 16:58 +0100, Jan Beulich wrote: >> On 05.02.2024 16:32, Oleksii Kurochko wrote: >>> +({ \ >>> + unsigned long __res, __mask; \ >> >> Leftover leading underscores? > It is how it was defined in Linux, so I thought that I've to leave it > as it, but I am OK to rename this variables in next patch version. My view: If you retain Linux style, retaining such names is also (kind of) okay. If you convert to Xen style, then name changes are to occur as part of that conversion. >>> + __mask = BIT_MASK(nr); \ >>> + __asm__ __volatile__ ( \ >>> + __AMO(op) #ord " %0, %2, %1" \ >>> + : "=r" (__res), "+A" (addr[BIT_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[BIT_WORD(nr)]) \ >>> + : "r" (mod(BIT_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)) >> >> Here the (double) leading underscores are truly worrying: Simple >> names like this aren't impossible to be assigned meaninb by a >> compiler. > I am not really understand what is the difference for a compiler > between NOP and __NOP? Do you mean that the leading double underscores > (__) are often used to indicate that these macros are implementation- > specific and might be reserved for the compiler or the standard > library? It's not "often used". Identifiers starting with two underscores or an underscore and a capital letter are reserved for the implementation (i.e. for the compiler's internal use). When not overly generic we stand a fair chance of getting away. But NOP and NOT are pretty generic. >>> +#include <asm-generic/bitops/fls.h> >>> +#include <asm-generic/bitops/flsl.h> >>> +#include <asm-generic/bitops/__ffs.h> >>> +#include <asm-generic/bitops/ffs.h> >>> +#include <asm-generic/bitops/ffsl.h> >>> +#include <asm-generic/bitops/ffz.h> >>> +#include <asm-generic/bitops/find-first-set-bit.h> >>> +#include <asm-generic/bitops/hweight.h> >>> +#include <asm-generic/bitops/test-bit.h> >> >> To be honest there's too much stuff being added here to asm-generic/, >> all in one go. I'll see about commenting on the remaining parts here, >> but I'd like to ask that you seriously consider splitting. > Would it be better to send it outside of this patch series? I can > create a separate patch series with a separate patch for each asm- > generic/bitops/*.h Not sure. Depends in part on whether then you'd effectively introduce dead code. If the introduction was such that RISC-V used the new ones right away, then yes, that would quite likely be better. Jan
© 2016 - 2024 Red Hat, Inc.