| Message ID | 1a0977e3cf5a2de9f760ca5ec89a0d096894a9e3.1713347222.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable build of full Xen for RISC-V | expand |
On 17.04.2024 12:04, Oleksii Kurochko wrote: > --- 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> This wants to move up into the xen/*.h group then, like you have done ... > --- 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> .. e.g. here. > --- a/xen/include/xen/bitops.h > +++ b/xen/include/xen/bitops.h > @@ -65,10 +65,137 @@ static inline int generic_flsl(unsigned long x) > * scope > */ > > +#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 arch_check_bitop_size > +#define arch_check_bitop_size(addr) Can this really do nothing? Passing the address of an object smaller than bitop_uint_t will read past the object in the generic__*_bit() functions. > +#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 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); The revision log suggests excess parentheses were dropped from such cast expressions. > --- a/xen/include/xen/types.h > +++ b/xen/include/xen/types.h > @@ -64,6 +64,11 @@ typedef __u64 __be64; > > typedef unsigned int __attribute__((__mode__(__pointer__))) uintptr_t; > > +#ifndef BITOP_TYPE > + #define BITOP_BITS_PER_WORD 32 > + typedef uint32_t bitop_uint_t; Personally I find this indentation odd / misleading. For pre-processor directives the # preferrably remains first on a line (as was iirc demanded by earlier C standards), followed by one or more blanks if so desired. File-scope declarations imo should never be indented. Jan
On Thu, 2024-04-25 at 17:35 +0200, Jan Beulich wrote: > > /* --------------------- Please tidy above here ----------------- > > ---- */ > > > > #include <asm/bitops.h> > > > > +#ifndef arch_check_bitop_size > > +#define arch_check_bitop_size(addr) > > Can this really do nothing? Passing the address of an object smaller > than > bitop_uint_t will read past the object in the generic__*_bit() > functions. Agree, in generic case it would be better to add: #define arch_check_bitop_size(addr) (sizeof(*(addr)) < sizeof(bitop_uint_t)) Originally, it was defined as empty becuase majority of supported architectures by Xen don't do this check and I decided to use this definition as generic. Thanks. ~ Oleksii
On 26.04.2024 10:14, Oleksii wrote: > On Thu, 2024-04-25 at 17:35 +0200, Jan Beulich wrote: >>> /* --------------------- Please tidy above here ----------------- >>> ---- */ >>> >>> #include <asm/bitops.h> >>> >>> +#ifndef arch_check_bitop_size >>> +#define arch_check_bitop_size(addr) >> >> Can this really do nothing? Passing the address of an object smaller >> than >> bitop_uint_t will read past the object in the generic__*_bit() >> functions. > Agree, in generic case it would be better to add: > #define arch_check_bitop_size(addr) (sizeof(*(addr)) < > sizeof(bitop_uint_t)) At which point x86 won't need any special casing anymore, I think. Jan
On Thu, 2024-04-25 at 17:35 +0200, Jan Beulich wrote: > > #include <asm/bitops.h> > > > > +#ifndef arch_check_bitop_size > > +#define arch_check_bitop_size(addr) > > Can this really do nothing? Passing the address of an object smaller > than > bitop_uint_t will read past the object in the generic__*_bit() > functions. It seems RISC-V isn' happy with the following generic definition: extern void __bitop_bad_size(void); /* --------------------- Please tidy above here -------------------- - */ #include <asm/bitops.h> #ifndef arch_check_bitop_size #define bitop_bad_size(addr) sizeof(*(addr)) < sizeof(bitop_uint_t) #define arch_check_bitop_size(addr) \ if ( bitop_bad_size(addr) ) __bitop_bad_size(); #endif /* arch_check_bitop_size */ The following errors occurs. bitop_uint_t for RISC-V is defined as unsigned long for now: ./common/symbols-dummy.o -o ./.xen-syms.0 riscv64-linux-gnu-ld: prelink.o: in function `atomic_sub': /build/xen/./arch/riscv/include/asm/atomic.h:152: undefined reference to `__bitop_bad_size' riscv64-linux-gnu-ld: prelink.o: in function `evtchn_check_pollers': /build/xen/common/event_channel.c:1531: undefined reference to `__bitop_bad_size' riscv64-linux-gnu-ld: /build/xen/common/event_channel.c:1521: undefined reference to `__bitop_bad_size' riscv64-linux-gnu-ld: prelink.o: in function `evtchn_init': /build/xen/common/event_channel.c:1541: undefined reference to `__bitop_bad_size' riscv64-linux-gnu-ld: prelink.o: in function `_read_lock': /build/xen/./include/xen/rwlock.h:94: undefined reference to `__bitop_bad_size' riscv64-linux-gnu-ld: prelink.o:/build/xen/./arch/riscv/include/asm/atomic.h:195: more undefined references to `__bitop_bad_size' follow riscv64-linux-gnu-ld: ./.xen-syms.0: hidden symbol `__bitop_bad_size' isn't defined riscv64-linux-gnu-ld: final link failed: bad value make[2]: *** [arch/riscv/Makefile:15: xen-syms] Error 1 ~ Oleksii
On 03.05.2024 19:15, Oleksii wrote: > On Thu, 2024-04-25 at 17:35 +0200, Jan Beulich wrote: >>> #include <asm/bitops.h> >>> >>> +#ifndef arch_check_bitop_size >>> +#define arch_check_bitop_size(addr) >> >> Can this really do nothing? Passing the address of an object smaller >> than >> bitop_uint_t will read past the object in the generic__*_bit() >> functions. > It seems RISC-V isn' happy with the following generic definition: > extern void __bitop_bad_size(void); > > /* --------------------- Please tidy above here -------------------- > - */ > > #include <asm/bitops.h> > > #ifndef arch_check_bitop_size > > #define bitop_bad_size(addr) sizeof(*(addr)) < sizeof(bitop_uint_t) > > #define arch_check_bitop_size(addr) \ > if ( bitop_bad_size(addr) ) __bitop_bad_size(); > > #endif /* arch_check_bitop_size */ I'm afraid you've re-discovered something that was also found during the original Arm porting effort. As nice and logical as it would (seem to) be to have bitop_uint_t match machine word size, there are places ... > The following errors occurs. bitop_uint_t for RISC-V is defined as > unsigned long for now: ... where we assume such operations can be done on 32-bit quantities. Jan > ./common/symbols-dummy.o -o ./.xen-syms.0 > riscv64-linux-gnu-ld: prelink.o: in function `atomic_sub': > /build/xen/./arch/riscv/include/asm/atomic.h:152: undefined reference > to `__bitop_bad_size' > riscv64-linux-gnu-ld: prelink.o: in function `evtchn_check_pollers': > /build/xen/common/event_channel.c:1531: undefined reference to > `__bitop_bad_size' > riscv64-linux-gnu-ld: /build/xen/common/event_channel.c:1521: undefined > reference to `__bitop_bad_size' > riscv64-linux-gnu-ld: prelink.o: in function `evtchn_init': > /build/xen/common/event_channel.c:1541: undefined reference to > `__bitop_bad_size' > riscv64-linux-gnu-ld: prelink.o: in function `_read_lock': > /build/xen/./include/xen/rwlock.h:94: undefined reference to > `__bitop_bad_size' > riscv64-linux-gnu-ld: > prelink.o:/build/xen/./arch/riscv/include/asm/atomic.h:195: more > undefined references to `__bitop_bad_size' follow > riscv64-linux-gnu-ld: ./.xen-syms.0: hidden symbol `__bitop_bad_size' > isn't defined > riscv64-linux-gnu-ld: final link failed: bad value > make[2]: *** [arch/riscv/Makefile:15: xen-syms] Error 1 > > ~ Oleksii
On Mon, 2024-05-06 at 08:33 +0200, Jan Beulich wrote: > On 03.05.2024 19:15, Oleksii wrote: > > On Thu, 2024-04-25 at 17:35 +0200, Jan Beulich wrote: > > > > #include <asm/bitops.h> > > > > > > > > +#ifndef arch_check_bitop_size > > > > +#define arch_check_bitop_size(addr) > > > > > > Can this really do nothing? Passing the address of an object > > > smaller > > > than > > > bitop_uint_t will read past the object in the generic__*_bit() > > > functions. > > It seems RISC-V isn' happy with the following generic definition: > > extern void __bitop_bad_size(void); > > > > /* --------------------- Please tidy above here ---------------- > > ---- > > - */ > > > > #include <asm/bitops.h> > > > > #ifndef arch_check_bitop_size > > > > #define bitop_bad_size(addr) sizeof(*(addr)) < > > sizeof(bitop_uint_t) > > > > #define arch_check_bitop_size(addr) \ > > if ( bitop_bad_size(addr) ) __bitop_bad_size(); > > > > #endif /* arch_check_bitop_size */ > > I'm afraid you've re-discovered something that was also found during > the > original Arm porting effort. As nice and logical as it would (seem > to) be > to have bitop_uint_t match machine word size, there are places ... > > > The following errors occurs. bitop_uint_t for RISC-V is defined as > > unsigned long for now: > > ... where we assume such operations can be done on 32-bit quantities. Based on RISC-V spec machine word is 32-bit, so then I can just drop re-definition of bitop_uint_t in riscv/asm/types.h and use the definition of bitop_uint_t in xen/types.h. Also it will be needed to update __AMO() macros in <riscv>/asm/bitops.h in the following way: #if BITOP_BITS_PER_WORD == 64 #define __AMO(op) "amo" #op ".d" #elif BITOP_BITS_PER_WORD == 32 #define __AMO(op) "amo" #op ".w" #else #error "Unexpected BITS_PER_LONG" #endif Note: BITS_PER_LONG was changed to BITOP_BITS_PER_WORD ! Only one question remains for me. Given that there are some operations whichcan be performed on 32-bit quantities, it seems to me that bitop_uint_t can only be uint32_t. Am I correct? If yes, do we need to have ability to redefine bitop_uint_t and BITOP_BITS_PER_WORD in xen/types.h: #ifndef BITOP_TYPE #define BITOP_BITS_PER_WORD 32 typedef uint32_t bitop_uint_t; #endif ~ Oleksii > > Jan > > > ./common/symbols-dummy.o -o ./.xen-syms.0 > > riscv64-linux-gnu-ld: prelink.o: in function `atomic_sub': > > /build/xen/./arch/riscv/include/asm/atomic.h:152: undefined > > reference > > to `__bitop_bad_size' > > riscv64-linux-gnu-ld: prelink.o: in function > > `evtchn_check_pollers': > > /build/xen/common/event_channel.c:1531: undefined reference to > > `__bitop_bad_size' > > riscv64-linux-gnu-ld: /build/xen/common/event_channel.c:1521: > > undefined > > reference to `__bitop_bad_size' > > riscv64-linux-gnu-ld: prelink.o: in function `evtchn_init': > > /build/xen/common/event_channel.c:1541: undefined reference to > > `__bitop_bad_size' > > riscv64-linux-gnu-ld: prelink.o: in function `_read_lock': > > /build/xen/./include/xen/rwlock.h:94: undefined reference to > > `__bitop_bad_size' > > riscv64-linux-gnu-ld: > > prelink.o:/build/xen/./arch/riscv/include/asm/atomic.h:195: more > > undefined references to `__bitop_bad_size' follow > > riscv64-linux-gnu-ld: ./.xen-syms.0: hidden symbol > > `__bitop_bad_size' > > isn't defined > > riscv64-linux-gnu-ld: final link failed: bad value > > make[2]: *** [arch/riscv/Makefile:15: xen-syms] Error 1 > > > > ~ Oleksii >
On 06.05.2024 10:16, Oleksii wrote: > On Mon, 2024-05-06 at 08:33 +0200, Jan Beulich wrote: >> On 03.05.2024 19:15, Oleksii wrote: >>> On Thu, 2024-04-25 at 17:35 +0200, Jan Beulich wrote: >>>>> #include <asm/bitops.h> >>>>> >>>>> +#ifndef arch_check_bitop_size >>>>> +#define arch_check_bitop_size(addr) >>>> >>>> Can this really do nothing? Passing the address of an object >>>> smaller >>>> than >>>> bitop_uint_t will read past the object in the generic__*_bit() >>>> functions. >>> It seems RISC-V isn' happy with the following generic definition: >>> extern void __bitop_bad_size(void); >>> >>> /* --------------------- Please tidy above here ---------------- >>> ---- >>> - */ >>> >>> #include <asm/bitops.h> >>> >>> #ifndef arch_check_bitop_size >>> >>> #define bitop_bad_size(addr) sizeof(*(addr)) < >>> sizeof(bitop_uint_t) >>> >>> #define arch_check_bitop_size(addr) \ >>> if ( bitop_bad_size(addr) ) __bitop_bad_size(); >>> >>> #endif /* arch_check_bitop_size */ >> >> I'm afraid you've re-discovered something that was also found during >> the >> original Arm porting effort. As nice and logical as it would (seem >> to) be >> to have bitop_uint_t match machine word size, there are places ... >> >>> The following errors occurs. bitop_uint_t for RISC-V is defined as >>> unsigned long for now: >> >> ... where we assume such operations can be done on 32-bit quantities. > Based on RISC-V spec machine word is 32-bit, so then I can just drop > re-definition of bitop_uint_t in riscv/asm/types.h and use the > definition of bitop_uint_t in xen/types.h. > Also it will be needed to update __AMO() macros in <riscv>/asm/bitops.h > in the following way: > #if BITOP_BITS_PER_WORD == 64 > #define __AMO(op) "amo" #op ".d" > #elif BITOP_BITS_PER_WORD == 32 > #define __AMO(op) "amo" #op ".w" > #else > #error "Unexpected BITS_PER_LONG" > #endif > Note: BITS_PER_LONG was changed to BITOP_BITS_PER_WORD ! > > Only one question remains for me. Given that there are some operations whichcan be performed on 32-bit quantities, it seems to me that bitop_uint_t > can only be uint32_t. > Am I correct? If yes, do we need to have ability to redefine > bitop_uint_t and BITOP_BITS_PER_WORD in xen/types.h: > #ifndef BITOP_TYPE > #define BITOP_BITS_PER_WORD 32 > > typedef uint32_t bitop_uint_t; > #endif Probably not, right now. Hence me having said "As nice and logical as it would (seem to) be" in the earlier reply. Jan
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..e2b6473c8c 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,44 +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) 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..2b2d9219ef 100644 --- a/xen/arch/x86/include/asm/bitops.h +++ b/xen/arch/x86/include/asm/bitops.h @@ -21,6 +21,8 @@ extern void __bitop_bad_size(void); #define bitop_bad_size(addr) (sizeof(*(addr)) < 4) +#define arch_check_bitop_size(addr) \ + if ( bitop_bad_size(addr) ) __bitop_bad_size(); /** * set_bit - Atomically set a bit in memory @@ -175,7 +177,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 +185,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 +196,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 +223,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 +231,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 +242,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 +256,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 +300,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..a41ec44064 100644 --- a/xen/include/xen/bitops.h +++ b/xen/include/xen/bitops.h @@ -65,10 +65,137 @@ static inline int generic_flsl(unsigned long x) * scope */ +#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 arch_check_bitop_size +#define arch_check_bitop_size(addr) +#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 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 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 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 bool generic_test_bit(int nr, const volatile void *addr) +{ + bitop_uint_t mask = BITOP_MASK(nr); + volatile bitop_uint_t *p = (volatile bitop_uint_t *)addr + BITOP_WORD(nr); + + return (*p & mask); +} + +static always_inline 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) ({ \ + arch_check_bitop_size(addr); \ + __test_and_set_bit(nr, addr); \ +}) + +static always_inline 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) ({ \ + arch_check_bitop_size(addr); \ + __test_and_clear_bit(nr, addr); \ +}) + +static always_inline 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) ({ \ + arch_check_bitop_size(addr); \ + __test_and_change_bit(nr, addr); \ +}) + +static always_inline bool 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) ({ \ + arch_check_bitop_size(addr); \ + test_bit(nr, addr); \ +}) + /* * Find First Set bit. Bits are labelled from 1. */ diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h index 449947b353..7a1f5021bd 100644 --- a/xen/include/xen/types.h +++ b/xen/include/xen/types.h @@ -64,6 +64,11 @@ typedef __u64 __be64; typedef unsigned int __attribute__((__mode__(__pointer__))) uintptr_t; +#ifndef BITOP_TYPE + #define BITOP_BITS_PER_WORD 32 + typedef uint32_t bitop_uint_t; +#endif + #define test_and_set_bool(b) xchg(&(b), true) #define test_and_clear_bool(b) xchg(&(b), false)
The following generic functions were introduced: * 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 make bitop bad size check generic too, so arch_check_bitop_size() was introduced and defined as empty for other architectures except x86. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- The context ("* Find First Set bit. Bits are labelled from 1." in xen/bitops.h ) suggests there's a dependency on an uncommitted patch. It happens becuase the current patch series is based on Andrew's patch series ( https://lore.kernel.org/xen-devel/20240313172716.2325427-1-andrew.cooper3@citrix.com/T/#t ), but if everything is okay with the current one patch it can be merged without Andrew's patch series being merged. --- Changes in V8: - drop __pure for function which uses volatile. - drop unnessary () in generic__test_and_change_bit() for addr casting. - update prototype of generic_test_bit() and test_bit(): now it returns bool instead of int. - update generic_test_bit() to use BITOP_MASK(). - Deal with fls{l} changes: it should be in the patch with introduced generic fls{l}. - add a footer with explanation of dependency on an uncommitted patch after Signed-off. - abstract bitop_size(). - move BITOP_TYPE define to <xen/types.h>. --- 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 | 52 ------------ xen/arch/ppc/include/asm/page.h | 2 +- xen/arch/ppc/mm-radix.c | 2 +- xen/arch/x86/include/asm/bitops.h | 30 +++---- xen/include/xen/bitops.h | 127 ++++++++++++++++++++++++++++++ xen/include/xen/types.h | 5 ++ 8 files changed, 145 insertions(+), 141 deletions(-)