The patch introduces the following generic functions:
* test_bit
* generic__test_and_set_bit
* generic__test_and_clear_bit
* generic__test_and_change_bit

Also, the patch introduces the following generics which are
used by the functions mentioned above:
* BITOP_BITS_PER_WORD
* BITOP_MASK
* BITOP_WORD
* BITOP_TYPE

These functions and macros can be useful for architectures
that don't have corresponding arch-specific instructions.

Because of that x86 has the following check in the macros test_bit(),
__test_and_set_bit(), __test_and_clear_bit(), __test_and_change_bit():
    if ( bitop_bad_size(addr) ) __bitop_bad_size();
It was necessary to move the check to generic code and define as 0
for other architectures as they do not require this check.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 Changes in V7:
  - move everything to xen/bitops.h to follow the same approach for all generic
    bit ops.
  - put together BITOP_BITS_PER_WORD and bitops_uint_t.
  - make BITOP_MASK more generic.
  - drop #ifdef ... #endif around BITOP_MASK, BITOP_WORD as they are generic
    enough.
  - drop "_" for generic__{test_and_set_bit,...}().
  - drop " != 0" for functions which return bool.
  - add volatile during the cast for generic__{...}().
  - update the commit message.
  - update arch related code to follow the proposed generic approach.
---
 Changes in V6:
  - Nothing changed ( only rebase )
---
 Changes in V5:
   - new patch
---
 xen/arch/arm/arm64/livepatch.c    |   1 -
 xen/arch/arm/include/asm/bitops.h |  67 -------------
 xen/arch/ppc/include/asm/bitops.h |  64 -------------
 xen/arch/ppc/include/asm/page.h   |   2 +-
 xen/arch/ppc/mm-radix.c           |   2 +-
 xen/arch/x86/include/asm/bitops.h |  28 ++----
 xen/include/xen/bitops.h          | 154 ++++++++++++++++++++++++++++++
 7 files changed, 165 insertions(+), 153 deletions(-)

diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index df2cebedde..4bc8ed9be5 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -10,7 +10,6 @@
 #include <xen/mm.h>
 #include <xen/vmap.h>
 
-#include <asm/bitops.h>
 #include <asm/byteorder.h>
 #include <asm/insn.h>
 #include <asm/livepatch.h>
diff --git a/xen/arch/arm/include/asm/bitops.h b/xen/arch/arm/include/asm/bitops.h
index 5104334e48..8e16335e76 100644
--- a/xen/arch/arm/include/asm/bitops.h
+++ b/xen/arch/arm/include/asm/bitops.h
@@ -22,9 +22,6 @@
 #define __set_bit(n,p)            set_bit(n,p)
 #define __clear_bit(n,p)          clear_bit(n,p)
 
-#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
 
 #define ADDR (*(volatile int *) addr)
@@ -76,70 +73,6 @@ bool test_and_change_bit_timeout(int nr, volatile void *p,
 bool clear_mask16_timeout(uint16_t mask, volatile void *p,
                           unsigned int max_try);
 
-/**
- * __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;
-}
-
-/* WARNING: non atomic and it can be reordered! */
-static inline int __test_and_change_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_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 int *p = (const volatile unsigned int *)addr;
-        return 1UL & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD-1)));
-}
-
 /*
  * On ARMv5 and above those functions can be implemented around
  * the clz instruction for much better code efficiency.
diff --git a/xen/arch/ppc/include/asm/bitops.h b/xen/arch/ppc/include/asm/bitops.h
index 989d341a44..a17060c7c2 100644
--- a/xen/arch/ppc/include/asm/bitops.h
+++ b/xen/arch/ppc/include/asm/bitops.h
@@ -15,9 +15,6 @@
 #define __set_bit(n, p)         set_bit(n, p)
 #define __clear_bit(n, p)       clear_bit(n, p)
 
-#define BITOP_BITS_PER_WORD     32
-#define BITOP_MASK(nr)          (1U << ((nr) % BITOP_BITS_PER_WORD))
-#define BITOP_WORD(nr)          ((nr) / BITOP_BITS_PER_WORD)
 #define BITS_PER_BYTE           8
 
 /* PPC bit number conversion */
@@ -69,17 +66,6 @@ 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 int *p = addr;
-    return 1 & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD - 1)));
-}
-
 static inline unsigned int test_and_clear_bits(
     unsigned int mask,
     volatile unsigned int *p)
@@ -133,56 +119,6 @@ static inline int test_and_set_bit(unsigned int nr, volatile void *addr)
         (volatile unsigned int *)addr + BITOP_WORD(nr)) != 0;
 }
 
-/**
- * __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)
-
-/* 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
diff --git a/xen/arch/ppc/include/asm/page.h b/xen/arch/ppc/include/asm/page.h
index 890e285051..482053b023 100644
--- a/xen/arch/ppc/include/asm/page.h
+++ b/xen/arch/ppc/include/asm/page.h
@@ -4,7 +4,7 @@
 
 #include <xen/types.h>
 
-#include <asm/bitops.h>
+#include <xen/bitops.h>
 #include <asm/byteorder.h>
 
 #define PDE_VALID     PPC_BIT(0)
diff --git a/xen/arch/ppc/mm-radix.c b/xen/arch/ppc/mm-radix.c
index daa411a6fa..3cd8c4635a 100644
--- a/xen/arch/ppc/mm-radix.c
+++ b/xen/arch/ppc/mm-radix.c
@@ -1,11 +1,11 @@
 /* SPDX-License-Identifier: GPL-2.0-or-later */
+#include <xen/bitops.h>
 #include <xen/init.h>
 #include <xen/kernel.h>
 #include <xen/mm.h>
 #include <xen/types.h>
 #include <xen/lib.h>
 
-#include <asm/bitops.h>
 #include <asm/byteorder.h>
 #include <asm/early_printk.h>
 #include <asm/page.h>
diff --git a/xen/arch/x86/include/asm/bitops.h b/xen/arch/x86/include/asm/bitops.h
index dd439b69a0..81b43da5db 100644
--- a/xen/arch/x86/include/asm/bitops.h
+++ b/xen/arch/x86/include/asm/bitops.h
@@ -175,7 +175,7 @@ static inline int test_and_set_bit(int nr, volatile void *addr)
 })
 
 /**
- * __test_and_set_bit - Set a bit and return its old value
+ * arch__test_and_set_bit - Set a bit and return its old value
  * @nr: Bit to set
  * @addr: Address to count from
  *
@@ -183,7 +183,7 @@ static inline int test_and_set_bit(int nr, volatile void *addr)
  * 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, void *addr)
+static inline int arch__test_and_set_bit(int nr, volatile void *addr)
 {
     int oldbit;
 
@@ -194,10 +194,7 @@ static inline int __test_and_set_bit(int nr, void *addr)
 
     return oldbit;
 }
-#define __test_and_set_bit(nr, addr) ({                 \
-    if ( bitop_bad_size(addr) ) __bitop_bad_size();     \
-    __test_and_set_bit(nr, addr);                       \
-})
+#define arch__test_and_set_bit arch__test_and_set_bit
 
 /**
  * test_and_clear_bit - Clear a bit and return its old value
@@ -224,7 +221,7 @@ static inline int test_and_clear_bit(int nr, volatile void *addr)
 })
 
 /**
- * __test_and_clear_bit - Clear a bit and return its old value
+ * arch__test_and_clear_bit - Clear a bit and return its old value
  * @nr: Bit to set
  * @addr: Address to count from
  *
@@ -232,7 +229,7 @@ static inline int test_and_clear_bit(int nr, volatile void *addr)
  * 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, void *addr)
+static inline int arch__test_and_clear_bit(int nr, volatile void *addr)
 {
     int oldbit;
 
@@ -243,13 +240,10 @@ static inline int __test_and_clear_bit(int nr, void *addr)
 
     return oldbit;
 }
-#define __test_and_clear_bit(nr, addr) ({               \
-    if ( bitop_bad_size(addr) ) __bitop_bad_size();     \
-    __test_and_clear_bit(nr, addr);                     \
-})
+#define arch__test_and_clear_bit arch__test_and_clear_bit
 
 /* WARNING: non atomic and it can be reordered! */
-static inline int __test_and_change_bit(int nr, void *addr)
+static inline int arch__test_and_change_bit(int nr, volatile void *addr)
 {
     int oldbit;
 
@@ -260,10 +254,7 @@ static inline int __test_and_change_bit(int nr, void *addr)
 
     return oldbit;
 }
-#define __test_and_change_bit(nr, addr) ({              \
-    if ( bitop_bad_size(addr) ) __bitop_bad_size();     \
-    __test_and_change_bit(nr, addr);                    \
-})
+#define arch__test_and_change_bit arch__test_and_change_bit
 
 /**
  * test_and_change_bit - Change a bit and return its new value
@@ -307,8 +298,7 @@ static inline int variable_test_bit(int nr, const volatile void *addr)
     return oldbit;
 }
 
-#define test_bit(nr, addr) ({                           \
-    if ( bitop_bad_size(addr) ) __bitop_bad_size();     \
+#define arch_test_bit(nr, addr) ({                      \
     __builtin_constant_p(nr) ?                          \
         constant_test_bit(nr, addr) :                   \
         variable_test_bit(nr, addr);                    \
diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
index f14ad0d33a..685c7540cc 100644
--- a/xen/include/xen/bitops.h
+++ b/xen/include/xen/bitops.h
@@ -65,10 +65,164 @@ static inline int generic_flsl(unsigned long x)
  * scope
  */
 
+#define BITOP_BITS_PER_WORD 32
+/* typedef uint32_t bitop_uint_t; */
+#define bitop_uint_t uint32_t
+
+#define BITOP_MASK(nr)  ((bitop_uint_t)1 << ((nr) % BITOP_BITS_PER_WORD))
+
+#define BITOP_WORD(nr)  ((nr) / BITOP_BITS_PER_WORD)
+
 /* --------------------- Please tidy above here --------------------- */
 
 #include <asm/bitops.h>
 
+#ifndef bitop_bad_size
+extern void __bitop_bad_size(void);
+#define bitop_bad_size(addr) 0
+#endif
+
+/**
+ * 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 __pure bool
+generic__test_and_set_bit(unsigned long nr, volatile void *addr)
+{
+    bitop_uint_t mask = BITOP_MASK(nr);
+    volatile bitop_uint_t *p = ((volatile bitop_uint_t *)addr) + BITOP_WORD(nr);
+    bitop_uint_t old = *p;
+
+    *p = old | mask;
+    return (old & mask);
+}
+
+/**
+ * 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 __pure bool
+generic__test_and_clear_bit(bitop_uint_t nr, volatile void *addr)
+{
+    bitop_uint_t mask = BITOP_MASK(nr);
+    volatile bitop_uint_t *p = ((volatile bitop_uint_t *)addr) + BITOP_WORD(nr);
+    bitop_uint_t old = *p;
+
+    *p = old & ~mask;
+    return (old & mask);
+}
+
+/* WARNING: non atomic and it can be reordered! */
+static always_inline __pure bool
+generic__test_and_change_bit(unsigned long nr, volatile void *addr)
+{
+    bitop_uint_t mask = BITOP_MASK(nr);
+    volatile bitop_uint_t *p = ((volatile bitop_uint_t *)addr) + BITOP_WORD(nr);
+    bitop_uint_t old = *p;
+
+    *p = old ^ mask;
+    return (old & mask);
+}
+/**
+ * generic_test_bit - Determine whether a bit is set
+ * @nr: bit number to test
+ * @addr: Address to start counting from
+ */
+static always_inline __pure int generic_test_bit(int nr, const volatile void *addr)
+{
+    const volatile bitop_uint_t *p = addr;
+    return 1 & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD - 1)));
+}
+
+static always_inline __pure bool
+__test_and_set_bit(unsigned long nr, volatile void *addr)
+{
+#ifndef arch__test_and_set_bit
+#define arch__test_and_set_bit generic__test_and_set_bit
+#endif
+
+    return arch__test_and_set_bit(nr, addr);
+}
+#define __test_and_set_bit(nr, addr) ({             \
+    if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
+    __test_and_set_bit(nr, addr);                   \
+})
+
+static always_inline __pure bool
+__test_and_clear_bit(bitop_uint_t nr, volatile void *addr)
+{
+#ifndef arch__test_and_clear_bit
+#define arch__test_and_clear_bit generic__test_and_clear_bit
+#endif
+
+    return arch__test_and_clear_bit(nr, addr);
+}
+#define __test_and_clear_bit(nr, addr) ({           \
+    if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
+    __test_and_clear_bit(nr, addr);                 \
+})
+
+static always_inline __pure bool
+__test_and_change_bit(unsigned long nr, volatile void *addr)
+{
+#ifndef arch__test_and_change_bit
+#define arch__test_and_change_bit generic__test_and_change_bit
+#endif
+
+    return arch__test_and_change_bit(nr, addr);
+}
+#define __test_and_change_bit(nr, addr) ({              \
+    if ( bitop_bad_size(addr) ) __bitop_bad_size();     \
+    __test_and_change_bit(nr, addr);                    \
+})
+
+static always_inline __pure int test_bit(int nr, const volatile void *addr)
+{
+#ifndef arch_test_bit
+#define arch_test_bit generic_test_bit
+#endif
+
+    return arch_test_bit(nr, addr);
+}
+#define test_bit(nr, addr) ({                           \
+    if ( bitop_bad_size(addr) ) __bitop_bad_size();     \
+    test_bit(nr, addr);                                 \
+})
+
+static always_inline __pure int fls(unsigned int x)
+{
+    if (__builtin_constant_p(x))
+        return generic_fls(x);
+
+#ifndef arch_fls
+#define arch_fls generic_fls
+#endif
+
+    return arch_fls(x);
+}
+
+static always_inline __pure int flsl(unsigned long x)
+{
+    if (__builtin_constant_p(x))
+        return generic_flsl(x);
+
+#ifndef arch_flsl
+#define arch_flsl generic_flsl
+#endif
+
+    return arch_flsl(x);
+}
+
 /*
  * Find First Set bit.  Bits are labelled from 1.
  */
-- 
2.43.0

On 03.04.2024 12:19, Oleksii Kurochko wrote:
> The patch introduces the following generic functions:
> * test_bit
> * generic__test_and_set_bit
> * generic__test_and_clear_bit
> * generic__test_and_change_bit
> 
> Also, the patch introduces the following generics which are
> used by the functions mentioned above:
> * BITOP_BITS_PER_WORD
> * BITOP_MASK
> * BITOP_WORD
> * BITOP_TYPE
> 
> These functions and macros can be useful for architectures
> that don't have corresponding arch-specific instructions.
> 
> Because of that x86 has the following check in the macros test_bit(),
> __test_and_set_bit(), __test_and_clear_bit(), __test_and_change_bit():
>     if ( bitop_bad_size(addr) ) __bitop_bad_size();
> It was necessary to move the check to generic code and define as 0
> for other architectures as they do not require this check.

Hmm, yes, the checks need to be in the outermost wrapper macros. While
you're abstracting other stuff to arch_*(), wouldn't it make sense to
also abstract this to e.g. arch_check_bitop_size(), with the expansion
simply being (effectively) empty in the generic fallback case?

> --- a/xen/include/xen/bitops.h
> +++ b/xen/include/xen/bitops.h
> @@ -65,10 +65,164 @@ static inline int generic_flsl(unsigned long x)
>   * scope
>   */
>  
> +#define BITOP_BITS_PER_WORD 32
> +/* typedef uint32_t bitop_uint_t; */
> +#define bitop_uint_t uint32_t

So no arch overrides permitted anymore at all?

> +#define BITOP_MASK(nr)  ((bitop_uint_t)1 << ((nr) % BITOP_BITS_PER_WORD))
> +
> +#define BITOP_WORD(nr)  ((nr) / BITOP_BITS_PER_WORD)
> +
>  /* --------------------- Please tidy above here --------------------- */
>  
>  #include <asm/bitops.h>
>  
> +#ifndef bitop_bad_size
> +extern void __bitop_bad_size(void);

If not switching to arch_check_bitop_size() or alike as suggested above,
why exactly does this need duplicating here and in x86? Can't the decl
simply move ahead of the #include right above? (Sure, this will then
require that nothing needing any of the functions you move here would
still include asm/bitops.h; it would need to be xen/bitops.h everywhere.)

> +#define bitop_bad_size(addr) 0
> +#endif
> +
> +/**
> + * 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 __pure bool
> +generic__test_and_set_bit(unsigned long nr, volatile void *addr)

Does __pure actually fit with the use of volatile? The former says multiple
accesses may be folded; the latter says they must not be.

> +{
> +    bitop_uint_t mask = BITOP_MASK(nr);
> +    volatile bitop_uint_t *p = ((volatile bitop_uint_t *)addr) + BITOP_WORD(nr);

Nit: Slightly shorter line possible:

    volatile bitop_uint_t *p = (volatile bitop_uint_t *)addr + BITOP_WORD(nr);

> +    bitop_uint_t old = *p;
> +
> +    *p = old | mask;
> +    return (old & mask);
> +}
> +
> +/**
> + * 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 __pure bool
> +generic__test_and_clear_bit(bitop_uint_t nr, volatile void *addr)
> +{
> +    bitop_uint_t mask = BITOP_MASK(nr);
> +    volatile bitop_uint_t *p = ((volatile bitop_uint_t *)addr) + BITOP_WORD(nr);
> +    bitop_uint_t old = *p;
> +
> +    *p = old & ~mask;
> +    return (old & mask);
> +}
> +
> +/* WARNING: non atomic and it can be reordered! */
> +static always_inline __pure bool
> +generic__test_and_change_bit(unsigned long nr, volatile void *addr)
> +{
> +    bitop_uint_t mask = BITOP_MASK(nr);
> +    volatile bitop_uint_t *p = ((volatile bitop_uint_t *)addr) + BITOP_WORD(nr);
> +    bitop_uint_t old = *p;
> +
> +    *p = old ^ mask;
> +    return (old & mask);
> +}
> +/**
> + * generic_test_bit - Determine whether a bit is set
> + * @nr: bit number to test
> + * @addr: Address to start counting from
> + */
> +static always_inline __pure int generic_test_bit(int nr, const volatile void *addr)

Further up you use bool; why int here?

> +{
> +    const volatile bitop_uint_t *p = addr;
> +    return 1 & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD - 1)));

And reason not to use BITOP_MASK() here as well (once having switched to
bool return type)?

> +}
> +
> +static always_inline __pure bool
> +__test_and_set_bit(unsigned long nr, volatile void *addr)
> +{
> +#ifndef arch__test_and_set_bit
> +#define arch__test_and_set_bit generic__test_and_set_bit
> +#endif
> +
> +    return arch__test_and_set_bit(nr, addr);
> +}
> +#define __test_and_set_bit(nr, addr) ({             \
> +    if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
> +    __test_and_set_bit(nr, addr);                   \
> +})
> +
> +static always_inline __pure bool
> +__test_and_clear_bit(bitop_uint_t nr, volatile void *addr)
> +{
> +#ifndef arch__test_and_clear_bit
> +#define arch__test_and_clear_bit generic__test_and_clear_bit
> +#endif
> +
> +    return arch__test_and_clear_bit(nr, addr);
> +}
> +#define __test_and_clear_bit(nr, addr) ({           \
> +    if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
> +    __test_and_clear_bit(nr, addr);                 \
> +})
> +
> +static always_inline __pure bool
> +__test_and_change_bit(unsigned long nr, volatile void *addr)
> +{
> +#ifndef arch__test_and_change_bit
> +#define arch__test_and_change_bit generic__test_and_change_bit
> +#endif
> +
> +    return arch__test_and_change_bit(nr, addr);
> +}
> +#define __test_and_change_bit(nr, addr) ({              \
> +    if ( bitop_bad_size(addr) ) __bitop_bad_size();     \
> +    __test_and_change_bit(nr, addr);                    \
> +})
> +
> +static always_inline __pure int test_bit(int nr, const volatile void *addr)

Further up you use bool; why int here?

> +{
> +#ifndef arch_test_bit
> +#define arch_test_bit generic_test_bit
> +#endif
> +
> +    return arch_test_bit(nr, addr);
> +}
> +#define test_bit(nr, addr) ({                           \
> +    if ( bitop_bad_size(addr) ) __bitop_bad_size();     \
> +    test_bit(nr, addr);                                 \
> +})

From here onwards, ...

> +static always_inline __pure int fls(unsigned int x)
> +{
> +    if (__builtin_constant_p(x))
> +        return generic_fls(x);
> +
> +#ifndef arch_fls
> +#define arch_fls generic_fls
> +#endif
> +
> +    return arch_fls(x);
> +}
> +
> +static always_inline __pure int flsl(unsigned long x)
> +{
> +    if (__builtin_constant_p(x))
> +        return generic_flsl(x);
> +
> +#ifndef arch_flsl
> +#define arch_flsl generic_flsl
> +#endif
> +
> +    return arch_flsl(x);
> +}

... does all of this really belong here? Neither title nor description have
any hint towards this.

>  /*
>   * Find First Set bit.  Bits are labelled from 1.
>   */

This context suggests there's a dependency on an uncommitted patch. Nothing
above says so. I guess you have a remark in the cover letter, yet imo that's
only partly helpful.

Jan

On Thu, 2024-04-04 at 15:22 +0200, Jan Beulich wrote:
> On 03.04.2024 12:19, Oleksii Kurochko wrote:
> > The patch introduces the following generic functions:
> > * test_bit
> > * generic__test_and_set_bit
> > * generic__test_and_clear_bit
> > * generic__test_and_change_bit
> > 
> > Also, the patch introduces the following generics which are
> > used by the functions mentioned above:
> > * BITOP_BITS_PER_WORD
> > * BITOP_MASK
> > * BITOP_WORD
> > * BITOP_TYPE
> > 
> > These functions and macros can be useful for architectures
> > that don't have corresponding arch-specific instructions.
> > 
> > Because of that x86 has the following check in the macros
> > test_bit(),
> > __test_and_set_bit(), __test_and_clear_bit(),
> > __test_and_change_bit():
> >     if ( bitop_bad_size(addr) ) __bitop_bad_size();
> > It was necessary to move the check to generic code and define as 0
> > for other architectures as they do not require this check.
> 
> Hmm, yes, the checks need to be in the outermost wrapper macros.
> While
> you're abstracting other stuff to arch_*(), wouldn't it make sense to
> also abstract this to e.g. arch_check_bitop_size(), with the
> expansion
> simply being (effectively) empty in the generic fallback case?
Probably it would be better to be consistent and introduce
arch_check_bitop_size().

> 
> > --- a/xen/include/xen/bitops.h
> > +++ b/xen/include/xen/bitops.h
> > @@ -65,10 +65,164 @@ static inline int generic_flsl(unsigned long
> > x)
> >   * scope
> >   */
> >  
> > +#define BITOP_BITS_PER_WORD 32
> > +/* typedef uint32_t bitop_uint_t; */
> > +#define bitop_uint_t uint32_t
> 
> So no arch overrides permitted anymore at all?
Not really, I agree that it is ugly, but I expected that arch will use
undef to override.

> 
> > +#define BITOP_MASK(nr)  ((bitop_uint_t)1 << ((nr) %
> > BITOP_BITS_PER_WORD))
> > +
> > +#define BITOP_WORD(nr)  ((nr) / BITOP_BITS_PER_WORD)
> > +
> >  /* --------------------- Please tidy above here ------------------
> > --- */
> >  
> >  #include <asm/bitops.h>
> >  
> > +#ifndef bitop_bad_size
> > +extern void __bitop_bad_size(void);
> 
> If not switching to arch_check_bitop_size() or alike as suggested
> above,
> why exactly does this need duplicating here and in x86? Can't the
> decl
> simply move ahead of the #include right above? (Sure, this will then
> require that nothing needing any of the functions you move here would
> still include asm/bitops.h; it would need to be xen/bitops.h
> everywhere.)
It could be done in way you suggest, I just wanted to keep changes
minimal ( without going and switching asm/bitops.h -> xen/bitops.h ),
but we can consider such option.

> 
> > +#define bitop_bad_size(addr) 0
> > +#endif
> > +
> > +/**
> > + * 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 __pure bool
> > +generic__test_and_set_bit(unsigned long nr, volatile void *addr)
> 
> Does __pure actually fit with the use of volatile? The former says
> multiple
> accesses may be folded; the latter says they must not be.
Overlooked that, __pure should be dropped.

> 
> > +{
> > +    bitop_uint_t mask = BITOP_MASK(nr);
> > +    volatile bitop_uint_t *p = ((volatile bitop_uint_t *)addr) +
> > BITOP_WORD(nr);
> 
> Nit: Slightly shorter line possible:
> 
>     volatile bitop_uint_t *p = (volatile bitop_uint_t *)addr +
> BITOP_WORD(nr);
> 
> > +    bitop_uint_t old = *p;
> > +
> > +    *p = old | mask;
> > +    return (old & mask);
> > +}
> > +
> > +/**
> > + * 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 __pure bool
> > +generic__test_and_clear_bit(bitop_uint_t nr, volatile void *addr)
> > +{
> > +    bitop_uint_t mask = BITOP_MASK(nr);
> > +    volatile bitop_uint_t *p = ((volatile bitop_uint_t *)addr) +
> > BITOP_WORD(nr);
> > +    bitop_uint_t old = *p;
> > +
> > +    *p = old & ~mask;
> > +    return (old & mask);
> > +}
> > +
> > +/* WARNING: non atomic and it can be reordered! */
> > +static always_inline __pure bool
> > +generic__test_and_change_bit(unsigned long nr, volatile void
> > *addr)
> > +{
> > +    bitop_uint_t mask = BITOP_MASK(nr);
> > +    volatile bitop_uint_t *p = ((volatile bitop_uint_t *)addr) +
> > BITOP_WORD(nr);
> > +    bitop_uint_t old = *p;
> > +
> > +    *p = old ^ mask;
> > +    return (old & mask);
> > +}
> > +/**
> > + * generic_test_bit - Determine whether a bit is set
> > + * @nr: bit number to test
> > + * @addr: Address to start counting from
> > + */
> > +static always_inline __pure int generic_test_bit(int nr, const
> > volatile void *addr)
> 
> Further up you use bool; why int here?
No specific reason, I have to update everything to bool.

> 
> > +{
> > +    const volatile bitop_uint_t *p = addr;
> > +    return 1 & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD -
> > 1)));
> 
> And reason not to use BITOP_MASK() here as well (once having switched
> to
> bool return type)?
It seems we can use BITOP_MASK() this implementation was copied from
arch specific code.

> 
> > +}
> > +
> > +static always_inline __pure bool
> > +__test_and_set_bit(unsigned long nr, volatile void *addr)
> > +{
> > +#ifndef arch__test_and_set_bit
> > +#define arch__test_and_set_bit generic__test_and_set_bit
> > +#endif
> > +
> > +    return arch__test_and_set_bit(nr, addr);
> > +}
> > +#define __test_and_set_bit(nr, addr) ({             \
> > +    if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
> > +    __test_and_set_bit(nr, addr);                   \
> > +})
> > +
> > +static always_inline __pure bool
> > +__test_and_clear_bit(bitop_uint_t nr, volatile void *addr)
> > +{
> > +#ifndef arch__test_and_clear_bit
> > +#define arch__test_and_clear_bit generic__test_and_clear_bit
> > +#endif
> > +
> > +    return arch__test_and_clear_bit(nr, addr);
> > +}
> > +#define __test_and_clear_bit(nr, addr) ({           \
> > +    if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
> > +    __test_and_clear_bit(nr, addr);                 \
> > +})
> > +
> > +static always_inline __pure bool
> > +__test_and_change_bit(unsigned long nr, volatile void *addr)
> > +{
> > +#ifndef arch__test_and_change_bit
> > +#define arch__test_and_change_bit generic__test_and_change_bit
> > +#endif
> > +
> > +    return arch__test_and_change_bit(nr, addr);
> > +}
> > +#define __test_and_change_bit(nr, addr) ({              \
> > +    if ( bitop_bad_size(addr) ) __bitop_bad_size();     \
> > +    __test_and_change_bit(nr, addr);                    \
> > +})
> > +
> > +static always_inline __pure int test_bit(int nr, const volatile
> > void *addr)
> 
> Further up you use bool; why int here?
> 
> > +{
> > +#ifndef arch_test_bit
> > +#define arch_test_bit generic_test_bit
> > +#endif
> > +
> > +    return arch_test_bit(nr, addr);
> > +}
> > +#define test_bit(nr, addr) ({                           \
> > +    if ( bitop_bad_size(addr) ) __bitop_bad_size();     \
> > +    test_bit(nr, addr);                                 \
> > +})
> 
> From here onwards, ...
> 
> > +static always_inline __pure int fls(unsigned int x)
> > +{
> > +    if (__builtin_constant_p(x))
> > +        return generic_fls(x);
> > +
> > +#ifndef arch_fls
> > +#define arch_fls generic_fls
> > +#endif
> > +
> > +    return arch_fls(x);
> > +}
> > +
> > +static always_inline __pure int flsl(unsigned long x)
> > +{
> > +    if (__builtin_constant_p(x))
> > +        return generic_flsl(x);
> > +
> > +#ifndef arch_flsl
> > +#define arch_flsl generic_flsl
> > +#endif
> > +
> > +    return arch_flsl(x);
> > +}
> 
> ... does all of this really belong here? Neither title nor
> description have
> any hint towards this.
No, it should be a part of the 	[PATCH v7 05/19] xen/bitops: implement
fls{l}() in common logic. An issue during rebase. I'll update that.

> 
> >  /*
> >   * Find First Set bit.  Bits are labelled from 1.
> >   */
> 
> This context suggests there's a dependency on an uncommitted patch.
> Nothing
> above says so. I guess you have a remark in the cover letter, yet imo
> that's
> only partly helpful.
Is it really a hard dependency?
The current patch series really depends on ffs{l}() and that was
mentioned in the cover letter ( I'll reword the cover letter to explain
why exactly this dependency is needed ), but this patch isn't really
depends on Andrew's patch series, where ffs{l}() are introduced.

~ Oleksii

On 04.04.2024 17:45, Oleksii wrote:
> On Thu, 2024-04-04 at 15:22 +0200, Jan Beulich wrote:
>> On 03.04.2024 12:19, Oleksii Kurochko wrote:
>>> --- a/xen/include/xen/bitops.h
>>> +++ b/xen/include/xen/bitops.h
>>> @@ -65,10 +65,164 @@ static inline int generic_flsl(unsigned long
>>> x)
>>>   * scope
>>>   */
>>>  
>>> +#define BITOP_BITS_PER_WORD 32
>>> +/* typedef uint32_t bitop_uint_t; */
>>> +#define bitop_uint_t uint32_t
>>
>> So no arch overrides permitted anymore at all?
> Not really, I agree that it is ugly, but I expected that arch will use
> undef to override.

Which would be fine in principle, just that Misra wants us to avoid #undef-s
(iirc).

>>>  /*
>>>   * Find First Set bit.  Bits are labelled from 1.
>>>   */
>>
>> This context suggests there's a dependency on an uncommitted patch.
>> Nothing
>> above says so. I guess you have a remark in the cover letter, yet imo
>> that's
>> only partly helpful.
> Is it really a hard dependency?
> The current patch series really depends on ffs{l}() and that was
> mentioned in the cover letter ( I'll reword the cover letter to explain
> why exactly this dependency is needed ), but this patch isn't really
> depends on Andrew's patch series, where ffs{l}() are introduced.

If anyone acked this patch, and if it otherwise looked independent, it would
be a candidate for committing. Just that it won't apply for a non-obvious
reason.

Jan

On Thu, 2024-04-04 at 18:12 +0200, Jan Beulich wrote:
> On 04.04.2024 17:45, Oleksii wrote:
> > On Thu, 2024-04-04 at 15:22 +0200, Jan Beulich wrote:
> > > On 03.04.2024 12:19, Oleksii Kurochko wrote:
> > > > --- a/xen/include/xen/bitops.h
> > > > +++ b/xen/include/xen/bitops.h
> > > > @@ -65,10 +65,164 @@ static inline int generic_flsl(unsigned
> > > > long
> > > > x)
> > > >   * scope
> > > >   */
> > > >  
> > > > +#define BITOP_BITS_PER_WORD 32
> > > > +/* typedef uint32_t bitop_uint_t; */
> > > > +#define bitop_uint_t uint32_t
> > > 
> > > So no arch overrides permitted anymore at all?
> > Not really, I agree that it is ugly, but I expected that arch will
> > use
> > undef to override.
> 
> Which would be fine in principle, just that Misra wants us to avoid
> #undef-s
> (iirc).
Could you please give me a recommendation how to do that better?

The reason why I put this defintions before inclusion of asm/bitops.h
as RISC-V specific code uses these definitions inside it, so they
should be defined before asm/bitops.h; other option is to define these
definitions inside asm/bitops.h for each architecture.

> 
> > > >  /*
> > > >   * Find First Set bit.  Bits are labelled from 1.
> > > >   */
> > > 
> > > This context suggests there's a dependency on an uncommitted
> > > patch.
> > > Nothing
> > > above says so. I guess you have a remark in the cover letter, yet
> > > imo
> > > that's
> > > only partly helpful.
> > Is it really a hard dependency?
> > The current patch series really depends on ffs{l}() and that was
> > mentioned in the cover letter ( I'll reword the cover letter to
> > explain
> > why exactly this dependency is needed ), but this patch isn't
> > really
> > depends on Andrew's patch series, where ffs{l}() are introduced.
> 
> If anyone acked this patch, and if it otherwise looked independent,
> it would
> be a candidate for committing. Just that it won't apply for a non-
> obvious
> reason.
I didn't think about the it won't apply. In this I have to definitely
mention this moment in cover letter. Thanks.

~ Oleksii

On 04.04.2024 18:24, Oleksii wrote:
> On Thu, 2024-04-04 at 18:12 +0200, Jan Beulich wrote:
>> On 04.04.2024 17:45, Oleksii wrote:
>>> On Thu, 2024-04-04 at 15:22 +0200, Jan Beulich wrote:
>>>> On 03.04.2024 12:19, Oleksii Kurochko wrote:
>>>>> --- a/xen/include/xen/bitops.h
>>>>> +++ b/xen/include/xen/bitops.h
>>>>> @@ -65,10 +65,164 @@ static inline int generic_flsl(unsigned
>>>>> long
>>>>> x)
>>>>>   * scope
>>>>>   */
>>>>>  
>>>>> +#define BITOP_BITS_PER_WORD 32
>>>>> +/* typedef uint32_t bitop_uint_t; */
>>>>> +#define bitop_uint_t uint32_t
>>>>
>>>> So no arch overrides permitted anymore at all?
>>> Not really, I agree that it is ugly, but I expected that arch will
>>> use
>>> undef to override.
>>
>> Which would be fine in principle, just that Misra wants us to avoid
>> #undef-s
>> (iirc).
> Could you please give me a recommendation how to do that better?
> 
> The reason why I put this defintions before inclusion of asm/bitops.h
> as RISC-V specific code uses these definitions inside it, so they
> should be defined before asm/bitops.h; other option is to define these
> definitions inside asm/bitops.h for each architecture.

Earlier on you had it that other way already (in a different header,
but the principle is the same): Move the generic definitions immediately
past inclusion of asm/bitops.h and frame them with #ifndef.

Jan

On Fri, 2024-04-05 at 08:11 +0200, Jan Beulich wrote:
> On 04.04.2024 18:24, Oleksii wrote:
> > On Thu, 2024-04-04 at 18:12 +0200, Jan Beulich wrote:
> > > On 04.04.2024 17:45, Oleksii wrote:
> > > > On Thu, 2024-04-04 at 15:22 +0200, Jan Beulich wrote:
> > > > > On 03.04.2024 12:19, Oleksii Kurochko wrote:
> > > > > > --- a/xen/include/xen/bitops.h
> > > > > > +++ b/xen/include/xen/bitops.h
> > > > > > @@ -65,10 +65,164 @@ static inline int
> > > > > > generic_flsl(unsigned
> > > > > > long
> > > > > > x)
> > > > > >   * scope
> > > > > >   */
> > > > > >  
> > > > > > +#define BITOP_BITS_PER_WORD 32
> > > > > > +/* typedef uint32_t bitop_uint_t; */
> > > > > > +#define bitop_uint_t uint32_t
> > > > > 
> > > > > So no arch overrides permitted anymore at all?
> > > > Not really, I agree that it is ugly, but I expected that arch
> > > > will
> > > > use
> > > > undef to override.
> > > 
> > > Which would be fine in principle, just that Misra wants us to
> > > avoid
> > > #undef-s
> > > (iirc).
> > Could you please give me a recommendation how to do that better?
> > 
> > The reason why I put this defintions before inclusion of
> > asm/bitops.h
> > as RISC-V specific code uses these definitions inside it, so they
> > should be defined before asm/bitops.h; other option is to define
> > these
> > definitions inside asm/bitops.h for each architecture.
> 
> Earlier on you had it that other way already (in a different header,
> but the principle is the same): Move the generic definitions
> immediately
> past inclusion of asm/bitops.h and frame them with #ifndef.
It can be done in this way:
xen/bitops.h:
   ...
   #include <asm/bitops.h>
   
   #ifndef BITOP_TYPE
   #define BITOP_BITS_PER_WORD 32
   /* typedef uint32_t bitop_uint_t; */
   #define bitop_uint_t uint32_t
   #endif
   ...
   
But then RISC-V will fail as it is using bitop_uint_t inside
asm/bitops.h.
So, at least, for RISC-V it will be needed to add asm/bitops.h:
   #define BITOP_BITS_PER_WORD 32
   /* typedef uint32_t bitop_uint_t; */
   #define bitop_uint_t uint32_t
   

It seems to me that this breaks the idea of having these macro
definitions generic, as RISC-V will redefine BITOP_BITS_PER_WORD and
bitop_uint_t with the same values as the generic ones.
   
~ Oleksii

> 
> Jan

On 05.04.2024 09:56, Oleksii wrote:
> On Fri, 2024-04-05 at 08:11 +0200, Jan Beulich wrote:
>> On 04.04.2024 18:24, Oleksii wrote:
>>> On Thu, 2024-04-04 at 18:12 +0200, Jan Beulich wrote:
>>>> On 04.04.2024 17:45, Oleksii wrote:
>>>>> On Thu, 2024-04-04 at 15:22 +0200, Jan Beulich wrote:
>>>>>> On 03.04.2024 12:19, Oleksii Kurochko wrote:
>>>>>>> --- a/xen/include/xen/bitops.h
>>>>>>> +++ b/xen/include/xen/bitops.h
>>>>>>> @@ -65,10 +65,164 @@ static inline int
>>>>>>> generic_flsl(unsigned
>>>>>>> long
>>>>>>> x)
>>>>>>>   * scope
>>>>>>>   */
>>>>>>>  
>>>>>>> +#define BITOP_BITS_PER_WORD 32
>>>>>>> +/* typedef uint32_t bitop_uint_t; */
>>>>>>> +#define bitop_uint_t uint32_t
>>>>>>
>>>>>> So no arch overrides permitted anymore at all?
>>>>> Not really, I agree that it is ugly, but I expected that arch
>>>>> will
>>>>> use
>>>>> undef to override.
>>>>
>>>> Which would be fine in principle, just that Misra wants us to
>>>> avoid
>>>> #undef-s
>>>> (iirc).
>>> Could you please give me a recommendation how to do that better?
>>>
>>> The reason why I put this defintions before inclusion of
>>> asm/bitops.h
>>> as RISC-V specific code uses these definitions inside it, so they
>>> should be defined before asm/bitops.h; other option is to define
>>> these
>>> definitions inside asm/bitops.h for each architecture.
>>
>> Earlier on you had it that other way already (in a different header,
>> but the principle is the same): Move the generic definitions
>> immediately
>> past inclusion of asm/bitops.h and frame them with #ifndef.
> It can be done in this way:
> xen/bitops.h:
>    ...
>    #include <asm/bitops.h>
>    
>    #ifndef BITOP_TYPE
>    #define BITOP_BITS_PER_WORD 32
>    /* typedef uint32_t bitop_uint_t; */
>    #define bitop_uint_t uint32_t
>    #endif
>    ...
>    
> But then RISC-V will fail as it is using bitop_uint_t inside
> asm/bitops.h.
> So, at least, for RISC-V it will be needed to add asm/bitops.h:
>    #define BITOP_BITS_PER_WORD 32
>    /* typedef uint32_t bitop_uint_t; */
>    #define bitop_uint_t uint32_t
>    
> 
> It seems to me that this breaks the idea of having these macro
> definitions generic, as RISC-V will redefine BITOP_BITS_PER_WORD and
> bitop_uint_t with the same values as the generic ones.

I don't follow. Right now patch 7 has

#undef BITOP_BITS_PER_WORD
#undef bitop_uint_t

#define BITOP_BITS_PER_WORD BITS_PER_LONG
#define bitop_uint_t unsigned long

You'd drop the #undef-s and keep the #define-s. You want to override them
both, after all.

A problem would arise for _another_ arch wanting to use these (default)
types in its asm/bitops.h. Which then could still be solved by having a
types-only header. Recall the discussion on the last summit of us meaning
to switch to such a model anyway (perhaps it being xen/types/bitops.h and
asm/types/bitops.h then), in a broader fashion? IOW for now you could use
the simple approach as long as no other arch needs the types in its
asm/bitops.h. Later we would introduce the types-only headers, thus
catering for possible future uses.

Jan

On Fri, 2024-04-05 at 10:05 +0200, Jan Beulich wrote:
> On 05.04.2024 09:56, Oleksii wrote:
> > On Fri, 2024-04-05 at 08:11 +0200, Jan Beulich wrote:
> > > On 04.04.2024 18:24, Oleksii wrote:
> > > > On Thu, 2024-04-04 at 18:12 +0200, Jan Beulich wrote:
> > > > > On 04.04.2024 17:45, Oleksii wrote:
> > > > > > On Thu, 2024-04-04 at 15:22 +0200, Jan Beulich wrote:
> > > > > > > On 03.04.2024 12:19, Oleksii Kurochko wrote:
> > > > > > > > --- a/xen/include/xen/bitops.h
> > > > > > > > +++ b/xen/include/xen/bitops.h
> > > > > > > > @@ -65,10 +65,164 @@ static inline int
> > > > > > > > generic_flsl(unsigned
> > > > > > > > long
> > > > > > > > x)
> > > > > > > >   * scope
> > > > > > > >   */
> > > > > > > >  
> > > > > > > > +#define BITOP_BITS_PER_WORD 32
> > > > > > > > +/* typedef uint32_t bitop_uint_t; */
> > > > > > > > +#define bitop_uint_t uint32_t
> > > > > > > 
> > > > > > > So no arch overrides permitted anymore at all?
> > > > > > Not really, I agree that it is ugly, but I expected that
> > > > > > arch
> > > > > > will
> > > > > > use
> > > > > > undef to override.
> > > > > 
> > > > > Which would be fine in principle, just that Misra wants us to
> > > > > avoid
> > > > > #undef-s
> > > > > (iirc).
> > > > Could you please give me a recommendation how to do that
> > > > better?
> > > > 
> > > > The reason why I put this defintions before inclusion of
> > > > asm/bitops.h
> > > > as RISC-V specific code uses these definitions inside it, so
> > > > they
> > > > should be defined before asm/bitops.h; other option is to
> > > > define
> > > > these
> > > > definitions inside asm/bitops.h for each architecture.
> > > 
> > > Earlier on you had it that other way already (in a different
> > > header,
> > > but the principle is the same): Move the generic definitions
> > > immediately
> > > past inclusion of asm/bitops.h and frame them with #ifndef.
> > It can be done in this way:
> > xen/bitops.h:
> >    ...
> >    #include <asm/bitops.h>
> >    
> >    #ifndef BITOP_TYPE
> >    #define BITOP_BITS_PER_WORD 32
> >    /* typedef uint32_t bitop_uint_t; */
> >    #define bitop_uint_t uint32_t
> >    #endif
> >    ...
> >    
> > But then RISC-V will fail as it is using bitop_uint_t inside
> > asm/bitops.h.
> > So, at least, for RISC-V it will be needed to add asm/bitops.h:
> >    #define BITOP_BITS_PER_WORD 32
> >    /* typedef uint32_t bitop_uint_t; */
> >    #define bitop_uint_t uint32_t
> >    
> > 
> > It seems to me that this breaks the idea of having these macro
> > definitions generic, as RISC-V will redefine BITOP_BITS_PER_WORD
> > and
> > bitop_uint_t with the same values as the generic ones.
> 
> I don't follow. Right now patch 7 has
> 
> #undef BITOP_BITS_PER_WORD
> #undef bitop_uint_t
> 
> #define BITOP_BITS_PER_WORD BITS_PER_LONG
> #define bitop_uint_t unsigned long
> 
> You'd drop the #undef-s and keep the #define-s. You want to override
> them
> both, after all.
> 
> A problem would arise for _another_ arch wanting to use these
> (default)
> types in its asm/bitops.h. Which then could still be solved by having
> a
> types-only header.
This problem arise now for Arm and PPC which use BITOP_BITS_PER_WORD
inside it. Then it is needed to define BITOP_BITS_PER_WORD=32 in
asm/bitops.h for Arm and PPC. If it is okay, then I will happy to
follow this approach.

>  Recall the discussion on the last summit of us meaning
> to switch to such a model anyway (perhaps it being xen/types/bitops.h
> and
> asm/types/bitops.h then), in a broader fashion? IOW for now you could
> use
> the simple approach as long as no other arch needs the types in its
> asm/bitops.h. Later we would introduce the types-only headers, thus
> catering for possible future uses.
Do we really need asm/types/bitops.h? Can't we just do the following in
asm/bitops.h:
  #ifndef BITOP_TYPE
  #include <xen/types/bitops.h>
  #endif

~ Oleksii

On Fri, 2024-04-05 at 13:53 +0200, Oleksii wrote:
> On Fri, 2024-04-05 at 10:05 +0200, Jan Beulich wrote:
> > On 05.04.2024 09:56, Oleksii wrote:
> > > On Fri, 2024-04-05 at 08:11 +0200, Jan Beulich wrote:
> > > > On 04.04.2024 18:24, Oleksii wrote:
> > > > > On Thu, 2024-04-04 at 18:12 +0200, Jan Beulich wrote:
> > > > > > On 04.04.2024 17:45, Oleksii wrote:
> > > > > > > On Thu, 2024-04-04 at 15:22 +0200, Jan Beulich wrote:
> > > > > > > > On 03.04.2024 12:19, Oleksii Kurochko wrote:
> > > > > > > > > --- a/xen/include/xen/bitops.h
> > > > > > > > > +++ b/xen/include/xen/bitops.h
> > > > > > > > > @@ -65,10 +65,164 @@ static inline int
> > > > > > > > > generic_flsl(unsigned
> > > > > > > > > long
> > > > > > > > > x)
> > > > > > > > >   * scope
> > > > > > > > >   */
> > > > > > > > >  
> > > > > > > > > +#define BITOP_BITS_PER_WORD 32
> > > > > > > > > +/* typedef uint32_t bitop_uint_t; */
> > > > > > > > > +#define bitop_uint_t uint32_t
> > > > > > > > 
> > > > > > > > So no arch overrides permitted anymore at all?
> > > > > > > Not really, I agree that it is ugly, but I expected that
> > > > > > > arch
> > > > > > > will
> > > > > > > use
> > > > > > > undef to override.
> > > > > > 
> > > > > > Which would be fine in principle, just that Misra wants us
> > > > > > to
> > > > > > avoid
> > > > > > #undef-s
> > > > > > (iirc).
> > > > > Could you please give me a recommendation how to do that
> > > > > better?
> > > > > 
> > > > > The reason why I put this defintions before inclusion of
> > > > > asm/bitops.h
> > > > > as RISC-V specific code uses these definitions inside it, so
> > > > > they
> > > > > should be defined before asm/bitops.h; other option is to
> > > > > define
> > > > > these
> > > > > definitions inside asm/bitops.h for each architecture.
> > > > 
> > > > Earlier on you had it that other way already (in a different
> > > > header,
> > > > but the principle is the same): Move the generic definitions
> > > > immediately
> > > > past inclusion of asm/bitops.h and frame them with #ifndef.
> > > It can be done in this way:
> > > xen/bitops.h:
> > >    ...
> > >    #include <asm/bitops.h>
> > >    
> > >    #ifndef BITOP_TYPE
> > >    #define BITOP_BITS_PER_WORD 32
> > >    /* typedef uint32_t bitop_uint_t; */
> > >    #define bitop_uint_t uint32_t
> > >    #endif
> > >    ...
> > >    
> > > But then RISC-V will fail as it is using bitop_uint_t inside
> > > asm/bitops.h.
> > > So, at least, for RISC-V it will be needed to add asm/bitops.h:
> > >    #define BITOP_BITS_PER_WORD 32
> > >    /* typedef uint32_t bitop_uint_t; */
> > >    #define bitop_uint_t uint32_t
> > >    
> > > 
> > > It seems to me that this breaks the idea of having these macro
> > > definitions generic, as RISC-V will redefine BITOP_BITS_PER_WORD
> > > and
> > > bitop_uint_t with the same values as the generic ones.
> > 
> > I don't follow. Right now patch 7 has
> > 
> > #undef BITOP_BITS_PER_WORD
> > #undef bitop_uint_t
> > 
> > #define BITOP_BITS_PER_WORD BITS_PER_LONG
> > #define bitop_uint_t unsigned long
> > 
> > You'd drop the #undef-s and keep the #define-s. You want to
> > override
> > them
> > both, after all.
> > 
> > A problem would arise for _another_ arch wanting to use these
> > (default)
> > types in its asm/bitops.h. Which then could still be solved by
> > having
> > a
> > types-only header.
> This problem arise now for Arm and PPC which use BITOP_BITS_PER_WORD
> inside it. Then it is needed to define BITOP_BITS_PER_WORD=32 in
> asm/bitops.h for Arm and PPC. If it is okay, then I will happy to
> follow this approach.
> 
> >  Recall the discussion on the last summit of us meaning
> > to switch to such a model anyway (perhaps it being
> > xen/types/bitops.h
> > and
> > asm/types/bitops.h then), in a broader fashion? IOW for now you
> > could
> > use
> > the simple approach as long as no other arch needs the types in its
> > asm/bitops.h. Later we would introduce the types-only headers, thus
> > catering for possible future uses.
> Do we really need asm/types/bitops.h? Can't we just do the following
> in
> asm/bitops.h:
>   #ifndef BITOP_TYPE
>   #include <xen/types/bitops.h>
>   #endif
Or as an options just update <xen/types.h> with after inclusion of
<asm/types.h>:
   #ifndef BITOP_TYPE
      #define BITOP_BITS_PER_WORD 32
      /* typedef uint32_t bitop_uint_t; */
      #define bitop_uint_t uint32_t
   #endif
   
And then just include <xen/types.h> to <<xen/bitops.h>.

~ Oleksii
> 
> ~ Oleksii

On 05.04.2024 13:56, Oleksii wrote:
> On Fri, 2024-04-05 at 13:53 +0200, Oleksii wrote:
>> On Fri, 2024-04-05 at 10:05 +0200, Jan Beulich wrote:
>>> On 05.04.2024 09:56, Oleksii wrote:
>>>> On Fri, 2024-04-05 at 08:11 +0200, Jan Beulich wrote:
>>>>> On 04.04.2024 18:24, Oleksii wrote:
>>>>>> On Thu, 2024-04-04 at 18:12 +0200, Jan Beulich wrote:
>>>>>>> On 04.04.2024 17:45, Oleksii wrote:
>>>>>>>> On Thu, 2024-04-04 at 15:22 +0200, Jan Beulich wrote:
>>>>>>>>> On 03.04.2024 12:19, Oleksii Kurochko wrote:
>>>>>>>>>> --- a/xen/include/xen/bitops.h
>>>>>>>>>> +++ b/xen/include/xen/bitops.h
>>>>>>>>>> @@ -65,10 +65,164 @@ static inline int
>>>>>>>>>> generic_flsl(unsigned
>>>>>>>>>> long
>>>>>>>>>> x)
>>>>>>>>>>   * scope
>>>>>>>>>>   */
>>>>>>>>>>  
>>>>>>>>>> +#define BITOP_BITS_PER_WORD 32
>>>>>>>>>> +/* typedef uint32_t bitop_uint_t; */
>>>>>>>>>> +#define bitop_uint_t uint32_t
>>>>>>>>>
>>>>>>>>> So no arch overrides permitted anymore at all?
>>>>>>>> Not really, I agree that it is ugly, but I expected that
>>>>>>>> arch
>>>>>>>> will
>>>>>>>> use
>>>>>>>> undef to override.
>>>>>>>
>>>>>>> Which would be fine in principle, just that Misra wants us
>>>>>>> to
>>>>>>> avoid
>>>>>>> #undef-s
>>>>>>> (iirc).
>>>>>> Could you please give me a recommendation how to do that
>>>>>> better?
>>>>>>
>>>>>> The reason why I put this defintions before inclusion of
>>>>>> asm/bitops.h
>>>>>> as RISC-V specific code uses these definitions inside it, so
>>>>>> they
>>>>>> should be defined before asm/bitops.h; other option is to
>>>>>> define
>>>>>> these
>>>>>> definitions inside asm/bitops.h for each architecture.
>>>>>
>>>>> Earlier on you had it that other way already (in a different
>>>>> header,
>>>>> but the principle is the same): Move the generic definitions
>>>>> immediately
>>>>> past inclusion of asm/bitops.h and frame them with #ifndef.
>>>> It can be done in this way:
>>>> xen/bitops.h:
>>>>    ...
>>>>    #include <asm/bitops.h>
>>>>    
>>>>    #ifndef BITOP_TYPE
>>>>    #define BITOP_BITS_PER_WORD 32
>>>>    /* typedef uint32_t bitop_uint_t; */
>>>>    #define bitop_uint_t uint32_t
>>>>    #endif
>>>>    ...
>>>>    
>>>> But then RISC-V will fail as it is using bitop_uint_t inside
>>>> asm/bitops.h.
>>>> So, at least, for RISC-V it will be needed to add asm/bitops.h:
>>>>    #define BITOP_BITS_PER_WORD 32
>>>>    /* typedef uint32_t bitop_uint_t; */
>>>>    #define bitop_uint_t uint32_t
>>>>    
>>>>
>>>> It seems to me that this breaks the idea of having these macro
>>>> definitions generic, as RISC-V will redefine BITOP_BITS_PER_WORD
>>>> and
>>>> bitop_uint_t with the same values as the generic ones.
>>>
>>> I don't follow. Right now patch 7 has
>>>
>>> #undef BITOP_BITS_PER_WORD
>>> #undef bitop_uint_t
>>>
>>> #define BITOP_BITS_PER_WORD BITS_PER_LONG
>>> #define bitop_uint_t unsigned long
>>>
>>> You'd drop the #undef-s and keep the #define-s. You want to
>>> override
>>> them
>>> both, after all.
>>>
>>> A problem would arise for _another_ arch wanting to use these
>>> (default)
>>> types in its asm/bitops.h. Which then could still be solved by
>>> having
>>> a
>>> types-only header.
>> This problem arise now for Arm and PPC which use BITOP_BITS_PER_WORD
>> inside it. Then it is needed to define BITOP_BITS_PER_WORD=32 in
>> asm/bitops.h for Arm and PPC. If it is okay, then I will happy to
>> follow this approach.
>>
>>>  Recall the discussion on the last summit of us meaning
>>> to switch to such a model anyway (perhaps it being
>>> xen/types/bitops.h
>>> and
>>> asm/types/bitops.h then), in a broader fashion? IOW for now you
>>> could
>>> use
>>> the simple approach as long as no other arch needs the types in its
>>> asm/bitops.h. Later we would introduce the types-only headers, thus
>>> catering for possible future uses.
>> Do we really need asm/types/bitops.h? Can't we just do the following
>> in
>> asm/bitops.h:
>>   #ifndef BITOP_TYPE
>>   #include <xen/types/bitops.h>
>>   #endif

This might do, yes.

> Or as an options just update <xen/types.h> with after inclusion of
> <asm/types.h>:
>    #ifndef BITOP_TYPE
>       #define BITOP_BITS_PER_WORD 32
>       /* typedef uint32_t bitop_uint_t; */
>       #define bitop_uint_t uint32_t
>    #endif
>    
> And then just include <xen/types.h> to <<xen/bitops.h>.

That's a (transient) option as well, I guess.

Jan