Hi Oleksii,
On 17/05/2024 14:54, Oleksii Kurochko wrote:
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>
It is a bit unclear how this change is related to the patch. Can you
explain in the commit message?
#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
OOI, any reason BITS_PER_BYTE has not been moved as well? I don't expect
the value to change across arch.
[...]
diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
index f14ad0d33a..6eeeff0117 100644
--- a/xen/include/xen/bitops.h
+++ b/xen/include/xen/bitops.h
@@ -65,10 +65,141 @@ static inline int generic_flsl(unsigned long x)
* scope
*/
+#define BITOP_BITS_PER_WORD 32
+typedef uint32_t bitop_uint_t;
+
+#define BITOP_MASK(nr) ((bitop_uint_t)1 << ((nr) % BITOP_BITS_PER_WORD))
+
+#define BITOP_WORD(nr) ((nr) / BITOP_BITS_PER_WORD)
+
+extern void __bitop_bad_size(void);
+
+#define bitop_bad_size(addr) (sizeof(*(addr)) < sizeof(bitop_uint_t))
+
/* --------------------- Please tidy above here --------------------- */
#include <asm/bitops.h>
+/**
+ * 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.
+ */
Sorry for only mentioning this on v10. I think this comment should be
duplicated (or moved to) on top of test_bit() because this is what
everyone will use. This will avoid the developper to follow the function
calls and only notice the x86 version which says "This function is
atomic and may not be reordered." and would be wrong for all the other arch.
+static always_inline bool
+generic__test_and_set_bit(int 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.
+ */
Same applies here and ...
+static always_inline bool
+generic__test_and_clear_bit(int 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! */
... here.
+static always_inline bool
+generic__test_and_change_bit(int 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 bool generic_test_bit(int nr, const volatile void *addr)
+{
+ bitop_uint_t mask = BITOP_MASK(nr);
+ const volatile bitop_uint_t *p =
+ (const volatile bitop_uint_t *)addr + BITOP_WORD(nr);
+
+ return (*p & mask);
+}
+
+static always_inline bool
+__test_and_set_bit(int 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);
+}
NIT: It is a bit too late to change this one. But I have to admit, I
don't understand the purpose of the static inline when you could have
simply call...
+#define __test_and_set_bit(nr, addr) ({ \
+ if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
+ __test_and_set_bit(nr, addr); \
... __arch__test_and_set_bit here.
The only two reasons I am not providing an ack is the:
* Explanation for the removal of asm/bitops.h in livepatch.c
* The placement of the comments
There are not too important for me.
Cheers,
--
Julien Grall
