| Message ID | 841d59c3950970f4937da200cf8f04aa39132e14.1703255175.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable build of full Xen for RISC-V | expand |
On 22.12.2023 16:12, Oleksii Kurochko wrote: > --- /dev/null > +++ b/xen/arch/riscv/include/asm/bitops.h > @@ -0,0 +1,267 @@ > +/* 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); \ > +}) Despite the taking from Linux I think the overall result wants to be consistent formatting-wise: You switched to blank indentation (which is fine), but you left tabs as padding for the line continuation characters. > +#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) \ (At least) here you even use a mix. > + __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); > +} > + > +#undef __test_and_op_bit > +#undef __op_bit > +#undef __NOP > +#undef __NOT > +#undef __AMO > + > +#define test_and_set_bit __test_and_set_bit > +#define test_and_clear_bit __test_and_clear_bit I realize test-and-change have no present users, despite being made available by Arm and x86, but I think they would better be provided right away, rather than someone introducing a use then needing to fiddle with RISC-V (and apparently also PPC) code. I'm also puzzled by this aliasing: Aren't there cheaper non-atomic insn forms that could be used for the double-underscore-prefixed variants? > +/* Based on linux/include/asm-generic/bitops/find.h */ > + > +#ifndef find_next_bit What is this to guard against? > +/** > + * find_next_bit - find the next set bit in a memory region > + * @addr: The address to base the search on > + * @offset: The bitnumber to start searching at > + * @size: The bitmap size in bits > + */ > +extern unsigned long find_next_bit(const unsigned long *addr, unsigned long > + size, unsigned long offset); > +#endif > + > +#ifndef find_next_zero_bit > +/** > + * find_next_zero_bit - find the next cleared bit in a memory region > + * @addr: The address to base the search on > + * @offset: The bitnumber to start searching at > + * @size: The bitmap size in bits > + */ > +extern unsigned long find_next_zero_bit(const unsigned long *addr, unsigned > + long size, unsigned long offset); > +#endif > + > +/** > + * find_first_bit - find the first set bit in a memory region > + * @addr: The address to start the search at > + * @size: The maximum size to search > + * > + * Returns the bit number of the first set bit. > + */ > +extern unsigned long find_first_bit(const unsigned long *addr, > + unsigned long size); > + > +/** > + * find_first_zero_bit - find the first cleared bit in a memory region > + * @addr: The address to start the search at > + * @size: The maximum size to search > + * > + * Returns the bit number of the first cleared bit. > + */ > +extern unsigned long find_first_zero_bit(const unsigned long *addr, > + unsigned long size); > + > +/** > + * 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 long __ffs(unsigned long word) > +{ > + int num = 0; I understand it's this way in Linux, so there may be reasons to keep it like that. Generally though we'd prefer unsigned here, and the type of a variable used for the return value of a function would also better be in sync with the function's return type (which I don't think needs to be "unsigned long" either; "unsigned int" would apparently suffice). > +#if BITS_PER_LONG == 64 > + if ((word & 0xffffffff) == 0) { > + num += 32; > + word >>= 32; > + } You're ending up with neither Xen nor Linux style this way. May I suggest to settle on either? > +#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; > +} > + > +/** > + * 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) > +{ > + int num = 1; > + > + if (!word) > + return 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; > +} What's RISC-V-specific about the above? IOW why ... > +#include <asm-generic/bitops/fls.h> > +#include <asm-generic/bitops/flsl.h> > +#include <asm-generic/bitops/ffs.h> > +#include <asm-generic/bitops/ffz.h> ... can't those two also come from respective generic headers? > --- /dev/null > +++ b/xen/include/asm-generic/bitops/bitops-bits.h > @@ -0,0 +1,10 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _ASM_GENERIC_BITOPS_BITS_H_ > +#define _ASM_GENERIC_BITOPS_BITS_H_ > + > +#define BITOP_BITS_PER_WORD 32 > +#define BITOP_MASK(nr) (1UL << ((nr) % BITOP_BITS_PER_WORD)) Why 1UL and not just 1U, when bits per word is 32? > +#define BITOP_WORD(nr) ((nr) / BITOP_BITS_PER_WORD) > +#define BITS_PER_BYTE 8 > + > +#endif /* _ASM_GENERIC_BITOPS_BITS_H_ */ > \ No newline at end of file Nit: I did comment on such before (and there's at least one more further down). > --- /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/ffz.h b/xen/include/asm-generic/bitops/ffz.h > new file mode 100644 > index 0000000000..92c35455d5 > --- /dev/null > +++ b/xen/include/asm-generic/bitops/ffz.h > @@ -0,0 +1,13 @@ > +/* 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. > + */ > +#define ffz(x) __ffs(~(x)) For a generic header I'd like to see consistency: ffz() may expand to ffs(), and __ffz() may expand to __ffs(). A mix like you have it above wants at least explaining in the description. (I don't understand anyway why both ffs() and __ffs() would be needed. We don't have any __ffs() on x86 afaics.) > --- /dev/null > +++ b/xen/include/asm-generic/bitops/find-first-bit-set.h This file name, if it really needs to be this long, wants to match ... > @@ -0,0 +1,17 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _ASM_GENERIC_BITOPS_FIND_FIRST_BIT_SET_H_ > +#define _ASM_GENERIC_BITOPS_FIND_FIRST_BIT_SET_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) ... the function implemented herein. > --- /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); > +} Seing this, why would e.g. ffsl() not use generic_ffsl() then? > --- /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_ */ Do we really need this? An arch not having suitable insns (RISC-V has, iirc) can easily have this one #define (or the ip to four ones when also covering the other widths) in its asm header. > --- /dev/null > +++ b/xen/include/asm-generic/bitops/test-bit.h > @@ -0,0 +1,16 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _ASM_GENERIC_BITOPS_TESTBIT_H_ > +#define _ASM_GENERIC_BITOPS_TESTBIT_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 unsigned int *p = addr; With BITOP_BITS_PER_WORD I think you really mean uint32_t here. Also you want to make sure asm-generic/bitops/bitops-bits.h is really in use here, or else an arch overriding / not using that header may end up screwed. Jan > + return 1 & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD - 1))); > +} > + > +#endif /* _ASM_GENERIC_BITOPS_TESTBIT_H_ */ > \ No newline at end of file
On Mon, 2024-01-15 at 17:44 +0100, Jan Beulich wrote: > On 22.12.2023 16:12, Oleksii Kurochko wrote: > > --- /dev/null > > +++ b/xen/arch/riscv/include/asm/bitops.h > > @@ -0,0 +1,267 @@ > > +/* 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); \ > > +}) > > Despite the taking from Linux I think the overall result wants to be > consistent formatting-wise: You switched to blank indentation (which > is fine), but you left tabs as padding for the line continuation > characters. I think it is because of my IDE. I will be consistent in next patch version. Thanks. > > > +#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) \ > > (At least) here you even use a mix. > > > + __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); > > +} > > + > > +#undef __test_and_op_bit > > +#undef __op_bit > > +#undef __NOP > > +#undef __NOT > > +#undef __AMO > > + > > +#define test_and_set_bit __test_and_set_bit > > +#define test_and_clear_bit __test_and_clear_bit > > I realize test-and-change have no present users, despite being made > available by Arm and x86, but I think they would better be provided > right away, rather than someone introducing a use then needing to > fiddle with RISC-V (and apparently also PPC) code. Sure, it makes sense. I'll add test-and-change too. > > I'm also puzzled by this aliasing: Aren't there cheaper non-atomic > insn forms that could be used for the double-underscore-prefixed > variants? It will be cheaper, but I assume that this API should be safe in the case of SMP where different CPUs can access the same variable or similar cases with simultaneous access to the variable. > > > +/* Based on linux/include/asm-generic/bitops/find.h */ > > + > > +#ifndef find_next_bit > > What is this to guard against? This was copied from Linux, but in case of Xen it can be dropped. > > > +/** > > + * find_next_bit - find the next set bit in a memory region > > + * @addr: The address to base the search on > > + * @offset: The bitnumber to start searching at > > + * @size: The bitmap size in bits > > + */ > > +extern unsigned long find_next_bit(const unsigned long *addr, > > unsigned long > > + size, unsigned long offset); > > +#endif > > + > > +#ifndef find_next_zero_bit > > +/** > > + * find_next_zero_bit - find the next cleared bit in a memory > > region > > + * @addr: The address to base the search on > > + * @offset: The bitnumber to start searching at > > + * @size: The bitmap size in bits > > + */ > > +extern unsigned long find_next_zero_bit(const unsigned long *addr, > > unsigned > > + long size, unsigned long offset); > > +#endif > > + > > +/** > > + * find_first_bit - find the first set bit in a memory region > > + * @addr: The address to start the search at > > + * @size: The maximum size to search > > + * > > + * Returns the bit number of the first set bit. > > + */ > > +extern unsigned long find_first_bit(const unsigned long *addr, > > + unsigned long size); > > + > > +/** > > + * find_first_zero_bit - find the first cleared bit in a memory > > region > > + * @addr: The address to start the search at > > + * @size: The maximum size to search > > + * > > + * Returns the bit number of the first cleared bit. > > + */ > > +extern unsigned long find_first_zero_bit(const unsigned long > > *addr, > > + unsigned long size); > > + > > +/** > > + * 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 long __ffs(unsigned long word) > > +{ > > + int num = 0; > > I understand it's this way in Linux, so there may be reasons to keep > it > like that. Generally though we'd prefer unsigned here, and the type > of > a variable used for the return value of a function would also better > be > in sync with the function's return type (which I don't think needs to > be "unsigned long" either; "unsigned int" would apparently suffice). > > > +#if BITS_PER_LONG == 64 > > + if ((word & 0xffffffff) == 0) { > > + num += 32; > > + word >>= 32; > > + } > > You're ending up with neither Xen nor Linux style this way. May I > suggest to settle on either? Will it fine to rework header from Linux to Xen style? Does it make sense? I think this file can be reworked to Xen style as I don't expect that it will be changed since it will be merged. > > > +#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; > > +} > > + > > +/** > > + * 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) > > +{ > > + int num = 1; > > + > > + if (!word) > > + return 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; > > +} > > What's RISC-V-specific about the above? IOW why ... > > > +#include <asm-generic/bitops/fls.h> > > +#include <asm-generic/bitops/flsl.h> > > +#include <asm-generic/bitops/ffs.h> > > +#include <asm-generic/bitops/ffz.h> > > ... can't those two also come from respective generic headers? Sure it can. Originally, I don't introduce it as generic headers because RISC-V is only one who isn using C generic version. I found that there is already present such generic function in xen/bitops.h. I think it makes sense to re-use them. > > > --- /dev/null > > +++ b/xen/include/asm-generic/bitops/bitops-bits.h > > @@ -0,0 +1,10 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +#ifndef _ASM_GENERIC_BITOPS_BITS_H_ > > +#define _ASM_GENERIC_BITOPS_BITS_H_ > > + > > +#define BITOP_BITS_PER_WORD 32 > > +#define BITOP_MASK(nr) (1UL << ((nr) % > > BITOP_BITS_PER_WORD)) > > Why 1UL and not just 1U, when bits per word is 32? There is no specific reason, should 1U. ( I originally used BITOPS_BITS_PER_LONG ) and with introduction of asm-generic bitops decided to follow what other archs provide. Regarding to the second part of the question, I don't understand it fully. Considering BITOP_BIT_PER_WORD definition for other archs ( ARM and PPC ) it is expected that word is 32 bits. > > > +#define BITOP_WORD(nr) ((nr) / BITOP_BITS_PER_WORD) > > +#define BITS_PER_BYTE 8 > > + > > +#endif /* _ASM_GENERIC_BITOPS_BITS_H_ */ > > \ No newline at end of file > > Nit: I did comment on such before (and there's at least one more > further down). Thanks. I'll add a newline. > > > --- /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/ffz.h > > b/xen/include/asm-generic/bitops/ffz.h > > new file mode 100644 > > index 0000000000..92c35455d5 > > --- /dev/null > > +++ b/xen/include/asm-generic/bitops/ffz.h > > @@ -0,0 +1,13 @@ > > +/* 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. > > + */ > > +#define ffz(x) __ffs(~(x)) > > For a generic header I'd like to see consistency: ffz() may expand to > ffs(), and __ffz() may expand to __ffs(). A mix like you have it > above > wants at least explaining in the description. (I don't understand > anyway why both ffs() and __ffs() would be needed. We don't have any > __ffs() on x86 afaics.) Then it looks to me that ffs() should be used instead. ffz() was defined with __ffs() because Arm and PPC are defined so. > > > --- /dev/null > > +++ b/xen/include/asm-generic/bitops/find-first-bit-set.h > > This file name, if it really needs to be this long, wants to match > ... > > > @@ -0,0 +1,17 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +#ifndef _ASM_GENERIC_BITOPS_FIND_FIRST_BIT_SET_H_ > > +#define _ASM_GENERIC_BITOPS_FIND_FIRST_BIT_SET_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) > > ... the function implemented herein. Thanks. I'll update the file name. > > > --- /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); > > +} > > Seing this, why would e.g. ffsl() not use generic_ffsl() then? > > > --- /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_ */ > > Do we really need this? An arch not having suitable insns (RISC-V > has, > iirc) can easily have this one #define (or the ip to four ones when > also covering the other widths) in its asm header. > > > --- /dev/null > > +++ b/xen/include/asm-generic/bitops/test-bit.h > > @@ -0,0 +1,16 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +#ifndef _ASM_GENERIC_BITOPS_TESTBIT_H_ > > +#define _ASM_GENERIC_BITOPS_TESTBIT_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 unsigned int *p = addr; > > With BITOP_BITS_PER_WORD I think you really mean uint32_t here. Isn't it the same: 'unsigned int' and 'uint32_t'? > Also you want to make sure asm-generic/bitops/bitops-bits.h is > really in use here, or else an arch overriding / not using that > header may end up screwed. I am not really understand what do you mean. Could you please explain a little bit more. ~ Oleksii > > Jan > > > + return 1 & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD - > > 1))); > > +} > > + > > +#endif /* _ASM_GENERIC_BITOPS_TESTBIT_H_ */ > > \ No newline at end of file >
On 16.01.2024 14:06, Oleksii wrote: > On Mon, 2024-01-15 at 17:44 +0100, Jan Beulich wrote: >> On 22.12.2023 16:12, Oleksii Kurochko wrote: >>> +#define test_and_set_bit __test_and_set_bit >>> +#define test_and_clear_bit __test_and_clear_bit >> >> I realize test-and-change have no present users, despite being made >> available by Arm and x86, but I think they would better be provided >> right away, rather than someone introducing a use then needing to >> fiddle with RISC-V (and apparently also PPC) code. > Sure, it makes sense. I'll add test-and-change too. > >> I'm also puzzled by this aliasing: Aren't there cheaper non-atomic >> insn forms that could be used for the double-underscore-prefixed >> variants? > It will be cheaper, but I assume that this API should be safe in the > case of SMP where different CPUs can access the same variable or > similar cases with simultaneous access to the variable. Of course, that's what test_and_...() are for. __test_and_...() are for cases where there's no concurrency, when hence the cheaper forms can be used. Thus my asking about the aliasing done above. >>> +#if BITS_PER_LONG == 64 >>> + if ((word & 0xffffffff) == 0) { >>> + num += 32; >>> + word >>= 32; >>> + } >> >> You're ending up with neither Xen nor Linux style this way. May I >> suggest to settle on either? > > Will it fine to rework header from Linux to Xen style? Does it make > sense? > I think this file can be reworked to Xen style as I don't expect that > it will be changed since it will be merged. You may keep Linux style or fully switch to Xen style - which one is largely up to you. All I'm asking is to avoid introducing further mixed-style source files. >>> --- /dev/null >>> +++ b/xen/include/asm-generic/bitops/bitops-bits.h >>> @@ -0,0 +1,10 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> +#ifndef _ASM_GENERIC_BITOPS_BITS_H_ >>> +#define _ASM_GENERIC_BITOPS_BITS_H_ >>> + >>> +#define BITOP_BITS_PER_WORD 32 >>> +#define BITOP_MASK(nr) (1UL << ((nr) % >>> BITOP_BITS_PER_WORD)) >> >> Why 1UL and not just 1U, when bits per word is 32? > There is no specific reason, should 1U. ( I originally used > BITOPS_BITS_PER_LONG ) and with introduction of asm-generic bitops > decided to follow what other archs provide. > > Regarding to the second part of the question, I don't understand it > fully. Considering BITOP_BIT_PER_WORD definition for other archs ( ARM > and PPC ) it is expected that word is 32 bits. The 2nd part was explaining why I'm asking. It wasn't another question. >>> --- /dev/null >>> +++ b/xen/include/asm-generic/bitops/test-bit.h >>> @@ -0,0 +1,16 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> +#ifndef _ASM_GENERIC_BITOPS_TESTBIT_H_ >>> +#define _ASM_GENERIC_BITOPS_TESTBIT_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 unsigned int *p = addr; >> >> With BITOP_BITS_PER_WORD I think you really mean uint32_t here. > Isn't it the same: 'unsigned int' and 'uint32_t'? No, or else there wouldn't have been a need to introduce uint<N>_t (and others) in C99. It just so happens that right now all architectures Xen can be built for have sizeof(int) == 4 and CHAR_BITS == 8. In an arch- specific header I would see this as less of an issue, but in a generic header we'd better avoid encoding wrong assumptions. The one assumption we generally make is that sizeof(int) >= 4 and CHAR_BITS >= 8 (albeit I bet really in various places we assume CHAR_BITS == 8). >> Also you want to make sure asm-generic/bitops/bitops-bits.h is >> really in use here, or else an arch overriding / not using that >> header may end up screwed. > I am not really understand what do you mean. Could you please explain a > little bit more. Whichever type you use here, it needs to be in sync with BITOP_BITS_PER_WORD. Hence you want to include the _local_ bitops-bits.h here, such that in case of an inconsistent override by an arch the compiler would complain about the two differring #define-s. (IOW an arch overriding BITOP_BITS_PER_WORD cannot re-use this header as-is.) The same may, btw, be true for others of the new headers you add - the same #include would therefore be needed there as well. Jan
On Tue, 2024-01-16 at 14:24 +0100, Jan Beulich wrote: > On 16.01.2024 14:06, Oleksii wrote: > > On Mon, 2024-01-15 at 17:44 +0100, Jan Beulich wrote: > > > On 22.12.2023 16:12, Oleksii Kurochko wrote: > > > > +#define test_and_set_bit __test_and_set_bit > > > > +#define test_and_clear_bit __test_and_clear_bit > > > > > > I realize test-and-change have no present users, despite being > > > made > > > available by Arm and x86, but I think they would better be > > > provided > > > right away, rather than someone introducing a use then needing to > > > fiddle with RISC-V (and apparently also PPC) code. > > Sure, it makes sense. I'll add test-and-change too. > > > > > I'm also puzzled by this aliasing: Aren't there cheaper non- > > > atomic > > > insn forms that could be used for the double-underscore-prefixed > > > variants? > > It will be cheaper, but I assume that this API should be safe in > > the > > case of SMP where different CPUs can access the same variable or > > similar cases with simultaneous access to the variable. > > Of course, that's what test_and_...() are for. __test_and_...() are > for cases where there's no concurrency, when hence the cheaper forms > can be used. Thus my asking about the aliasing done above. Then it makes sense to update __test_and...() to use non-atomic insn. I'll do that in the next patch version. Thanks for explanation. > > > > > +#if BITS_PER_LONG == 64 > > > > + if ((word & 0xffffffff) == 0) { > > > > + num += 32; > > > > + word >>= 32; > > > > + } > > > > > > You're ending up with neither Xen nor Linux style this way. May I > > > suggest to settle on either? > > > > Will it fine to rework header from Linux to Xen style? Does it make > > sense? > > I think this file can be reworked to Xen style as I don't expect > > that > > it will be changed since it will be merged. > > You may keep Linux style or fully switch to Xen style - which one is > largely up to you. All I'm asking is to avoid introducing further > mixed-style source files. I'll be consistent in code style. > > > > > --- /dev/null > > > > +++ b/xen/include/asm-generic/bitops/bitops-bits.h > > > > @@ -0,0 +1,10 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > > +#ifndef _ASM_GENERIC_BITOPS_BITS_H_ > > > > +#define _ASM_GENERIC_BITOPS_BITS_H_ > > > > + > > > > +#define BITOP_BITS_PER_WORD 32 > > > > +#define BITOP_MASK(nr) (1UL << ((nr) % > > > > BITOP_BITS_PER_WORD)) > > > > > > Why 1UL and not just 1U, when bits per word is 32? > > There is no specific reason, should 1U. ( I originally used > > BITOPS_BITS_PER_LONG ) and with introduction of asm-generic bitops > > decided to follow what other archs provide. > > > > Regarding to the second part of the question, I don't understand it > > fully. Considering BITOP_BIT_PER_WORD definition for other archs ( > > ARM > > and PPC ) it is expected that word is 32 bits. > > The 2nd part was explaining why I'm asking. It wasn't another > question. > > > > > --- /dev/null > > > > +++ b/xen/include/asm-generic/bitops/test-bit.h > > > > @@ -0,0 +1,16 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > > +#ifndef _ASM_GENERIC_BITOPS_TESTBIT_H_ > > > > +#define _ASM_GENERIC_BITOPS_TESTBIT_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 unsigned int *p = addr; > > > > > > With BITOP_BITS_PER_WORD I think you really mean uint32_t here. > > Isn't it the same: 'unsigned int' and 'uint32_t'? > > No, or else there wouldn't have been a need to introduce uint<N>_t > (and > others) in C99. It just so happens that right now all architectures > Xen > can be built for have sizeof(int) == 4 and CHAR_BITS == 8. In an > arch- > specific header I would see this as less of an issue, but in a > generic > header we'd better avoid encoding wrong assumptions. The one > assumption > we generally make is that sizeof(int) >= 4 and CHAR_BITS >= 8 (albeit > I > bet really in various places we assume CHAR_BITS == 8). In this case we have to switch to uint<N>_t. Thanks for the explanation. I'll update this part of code in the next patch version. > > > > Also you want to make sure asm-generic/bitops/bitops-bits.h is > > > really in use here, or else an arch overriding / not using that > > > header may end up screwed. > > I am not really understand what do you mean. Could you please > > explain a > > little bit more. > > Whichever type you use here, it needs to be in sync with > BITOP_BITS_PER_WORD. Hence you want to include the _local_ bitops- > bits.h > here, such that in case of an inconsistent override by an arch the > compiler would complain about the two differring #define-s. (IOW an > arch overriding BITOP_BITS_PER_WORD cannot re-use this header as-is.) > > The same may, btw, be true for others of the new headers you add - > the > same #include would therefore be needed there as well. Now it clear to me. It seems like BITOP_BITS_PER_WORD, BITOP_MASK, BITOP_WORD, and BITS_PER_BYTE are defined in {arm, ppc, riscv}/include/asm/bitops.h. I expected that any architecture planning to use asm- generic/bitops/bitops-bits.h would include it at the beginning of <arch>/include/asm/bitops.h, similar to what is done for RISC-V: #ifndef _ASM_RISCV_BITOPS_H #define _ASM_RISCV_BITOPS_H #include <asm/system.h> #include <asm-generic/bitops/bitops-bits.h> ... But in this case, to allow architecture overrides macros, it is necessary to update asm-generic/bitops/bitops-bits.h: #ifndef BITOP_BITS_PER_WORD #define BITOP_BITS_PER_WORD 32 #endif ... Therefore, if an architecture needs to override something, it will add #define ... before #include <asm-generic/bitops/bitops-bits.h>. Does it make sense? ~ Oleksii
On 17.01.2024 12:13, Oleksii wrote: > On Tue, 2024-01-16 at 14:24 +0100, Jan Beulich wrote: >> On 16.01.2024 14:06, Oleksii wrote: >>> On Mon, 2024-01-15 at 17:44 +0100, Jan Beulich wrote: >>>> On 22.12.2023 16:12, Oleksii Kurochko wrote: >>>>> --- /dev/null >>>>> +++ b/xen/include/asm-generic/bitops/test-bit.h >>>>> @@ -0,0 +1,16 @@ >>>>> +/* SPDX-License-Identifier: GPL-2.0 */ >>>>> +#ifndef _ASM_GENERIC_BITOPS_TESTBIT_H_ >>>>> +#define _ASM_GENERIC_BITOPS_TESTBIT_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 unsigned int *p = addr; >>>> >>>> With BITOP_BITS_PER_WORD I think you really mean uint32_t here. >>> Isn't it the same: 'unsigned int' and 'uint32_t'? >> >> No, or else there wouldn't have been a need to introduce uint<N>_t >> (and >> others) in C99. It just so happens that right now all architectures >> Xen >> can be built for have sizeof(int) == 4 and CHAR_BITS == 8. In an >> arch- >> specific header I would see this as less of an issue, but in a >> generic >> header we'd better avoid encoding wrong assumptions. The one >> assumption >> we generally make is that sizeof(int) >= 4 and CHAR_BITS >= 8 (albeit >> I >> bet really in various places we assume CHAR_BITS == 8). > In this case we have to switch to uint<N>_t. > Thanks for the explanation. I'll update this part of code in the next > patch version. > >> >>>> Also you want to make sure asm-generic/bitops/bitops-bits.h is >>>> really in use here, or else an arch overriding / not using that >>>> header may end up screwed. >>> I am not really understand what do you mean. Could you please >>> explain a >>> little bit more. >> >> Whichever type you use here, it needs to be in sync with >> BITOP_BITS_PER_WORD. Hence you want to include the _local_ bitops- >> bits.h >> here, such that in case of an inconsistent override by an arch the >> compiler would complain about the two differring #define-s. (IOW an >> arch overriding BITOP_BITS_PER_WORD cannot re-use this header as-is.) >> >> The same may, btw, be true for others of the new headers you add - >> the >> same #include would therefore be needed there as well. > Now it clear to me. > > > It seems like BITOP_BITS_PER_WORD, BITOP_MASK, BITOP_WORD, and > BITS_PER_BYTE are defined in {arm, ppc, riscv}/include/asm/bitops.h. > I expected that any architecture planning to use asm- > generic/bitops/bitops-bits.h would include it at the beginning of > <arch>/include/asm/bitops.h, similar to what is done for RISC-V: > #ifndef _ASM_RISCV_BITOPS_H > #define _ASM_RISCV_BITOPS_H > > #include <asm/system.h> > > #include <asm-generic/bitops/bitops-bits.h> > ... > > But in this case, to allow architecture overrides macros, it is > necessary to update asm-generic/bitops/bitops-bits.h: > #ifndef BITOP_BITS_PER_WORD > #define BITOP_BITS_PER_WORD 32 > #endif > ... > Therefore, if an architecture needs to override something, it will add > #define ... before #include <asm-generic/bitops/bitops-bits.h>. > > Does it make sense? Sure. But then the arch also needs to provide a corresponding typedef (and bitops-bits.h the fallback one), for use wherever you use any of those #define-s. Jan
> > > > > > > > Also you want to make sure asm-generic/bitops/bitops-bits.h > > > > > is > > > > > really in use here, or else an arch overriding / not using > > > > > that > > > > > header may end up screwed. > > > > I am not really understand what do you mean. Could you please > > > > explain a > > > > little bit more. > > > > > > Whichever type you use here, it needs to be in sync with > > > BITOP_BITS_PER_WORD. Hence you want to include the _local_ > > > bitops- > > > bits.h > > > here, such that in case of an inconsistent override by an arch > > > the > > > compiler would complain about the two differring #define-s. (IOW > > > an > > > arch overriding BITOP_BITS_PER_WORD cannot re-use this header as- > > > is.) > > > > > > The same may, btw, be true for others of the new headers you add > > > - > > > the > > > same #include would therefore be needed there as well. > > Now it clear to me. > > > > > > It seems like BITOP_BITS_PER_WORD, BITOP_MASK, BITOP_WORD, and > > BITS_PER_BYTE are defined in {arm, ppc, > > riscv}/include/asm/bitops.h. > > I expected that any architecture planning to use asm- > > generic/bitops/bitops-bits.h would include it at the beginning of > > <arch>/include/asm/bitops.h, similar to what is done for RISC-V: > > #ifndef _ASM_RISCV_BITOPS_H > > #define _ASM_RISCV_BITOPS_H > > > > #include <asm/system.h> > > > > #include <asm-generic/bitops/bitops-bits.h> > > ... > > > > But in this case, to allow architecture overrides macros, it is > > necessary to update asm-generic/bitops/bitops-bits.h: > > #ifndef BITOP_BITS_PER_WORD > > #define BITOP_BITS_PER_WORD 32 > > #endif > > ... > > Therefore, if an architecture needs to override something, it will > > add > > #define ... before #include <asm-generic/bitops/bitops-bits.h>. > > > > Does it make sense? > > Sure. But then the arch also needs to provide a corresponding typedef > (and bitops-bits.h the fallback one), for use wherever you use any of > those #define-s. Which one typedef is needed to provide? <asm-generic/bitops/bitops-bits.h> contains only macros. ~ Oleksii
On 17.01.2024 12:37, Oleksii wrote: >>>> >>>>>> Also you want to make sure asm-generic/bitops/bitops-bits.h >>>>>> is >>>>>> really in use here, or else an arch overriding / not using >>>>>> that >>>>>> header may end up screwed. >>>>> I am not really understand what do you mean. Could you please >>>>> explain a >>>>> little bit more. >>>> >>>> Whichever type you use here, it needs to be in sync with >>>> BITOP_BITS_PER_WORD. Hence you want to include the _local_ >>>> bitops- >>>> bits.h >>>> here, such that in case of an inconsistent override by an arch >>>> the >>>> compiler would complain about the two differring #define-s. (IOW >>>> an >>>> arch overriding BITOP_BITS_PER_WORD cannot re-use this header as- >>>> is.) >>>> >>>> The same may, btw, be true for others of the new headers you add >>>> - >>>> the >>>> same #include would therefore be needed there as well. >>> Now it clear to me. >>> >>> >>> It seems like BITOP_BITS_PER_WORD, BITOP_MASK, BITOP_WORD, and >>> BITS_PER_BYTE are defined in {arm, ppc, >>> riscv}/include/asm/bitops.h. >>> I expected that any architecture planning to use asm- >>> generic/bitops/bitops-bits.h would include it at the beginning of >>> <arch>/include/asm/bitops.h, similar to what is done for RISC-V: >>> #ifndef _ASM_RISCV_BITOPS_H >>> #define _ASM_RISCV_BITOPS_H >>> >>> #include <asm/system.h> >>> >>> #include <asm-generic/bitops/bitops-bits.h> >>> ... >>> >>> But in this case, to allow architecture overrides macros, it is >>> necessary to update asm-generic/bitops/bitops-bits.h: >>> #ifndef BITOP_BITS_PER_WORD >>> #define BITOP_BITS_PER_WORD 32 >>> #endif >>> ... >>> Therefore, if an architecture needs to override something, it will >>> add >>> #define ... before #include <asm-generic/bitops/bitops-bits.h>. >>> >>> Does it make sense? >> >> Sure. But then the arch also needs to provide a corresponding typedef >> (and bitops-bits.h the fallback one), for use wherever you use any of >> those #define-s. > Which one typedef is needed to provide? > <asm-generic/bitops/bitops-bits.h> contains only macros. A new one, to replace where right now you use "unsigned int" and I initially said you need to use "uint32_t" instead. With what you said earlier, uint32_t won't work there (anymore). Jan
On Wed, 2024-01-17 at 14:42 +0100, Jan Beulich wrote: > On 17.01.2024 12:37, Oleksii wrote: > > > > > > > > > > > > Also you want to make sure asm-generic/bitops/bitops- > > > > > > > bits.h > > > > > > > is > > > > > > > really in use here, or else an arch overriding / not > > > > > > > using > > > > > > > that > > > > > > > header may end up screwed. > > > > > > I am not really understand what do you mean. Could you > > > > > > please > > > > > > explain a > > > > > > little bit more. > > > > > > > > > > Whichever type you use here, it needs to be in sync with > > > > > BITOP_BITS_PER_WORD. Hence you want to include the _local_ > > > > > bitops- > > > > > bits.h > > > > > here, such that in case of an inconsistent override by an > > > > > arch > > > > > the > > > > > compiler would complain about the two differring #define-s. > > > > > (IOW > > > > > an > > > > > arch overriding BITOP_BITS_PER_WORD cannot re-use this header > > > > > as- > > > > > is.) > > > > > > > > > > The same may, btw, be true for others of the new headers you > > > > > add > > > > > - > > > > > the > > > > > same #include would therefore be needed there as well. > > > > Now it clear to me. > > > > > > > > > > > > It seems like BITOP_BITS_PER_WORD, BITOP_MASK, BITOP_WORD, and > > > > BITS_PER_BYTE are defined in {arm, ppc, > > > > riscv}/include/asm/bitops.h. > > > > I expected that any architecture planning to use asm- > > > > generic/bitops/bitops-bits.h would include it at the beginning > > > > of > > > > <arch>/include/asm/bitops.h, similar to what is done for RISC- > > > > V: > > > > #ifndef _ASM_RISCV_BITOPS_H > > > > #define _ASM_RISCV_BITOPS_H > > > > > > > > #include <asm/system.h> > > > > > > > > #include <asm-generic/bitops/bitops-bits.h> > > > > ... > > > > > > > > But in this case, to allow architecture overrides macros, it is > > > > necessary to update asm-generic/bitops/bitops-bits.h: > > > > #ifndef BITOP_BITS_PER_WORD > > > > #define BITOP_BITS_PER_WORD 32 > > > > #endif > > > > ... > > > > Therefore, if an architecture needs to override something, it > > > > will > > > > add > > > > #define ... before #include <asm-generic/bitops/bitops-bits.h>. > > > > > > > > Does it make sense? > > > > > > Sure. But then the arch also needs to provide a corresponding > > > typedef > > > (and bitops-bits.h the fallback one), for use wherever you use > > > any of > > > those #define-s. > > Which one typedef is needed to provide? > > <asm-generic/bitops/bitops-bits.h> contains only macros. > > A new one, to replace where right now you use "unsigned int" and I > initially said you need to use "uint32_t" instead. With what you said > earlier, uint32_t won't work there (anymore). Wouldn't it be enough just to "#include <xen/types.h>" in headers where "uint32_t" is used? ~ Olkesii
On 18.01.2024 10:43, Oleksii wrote: > On Wed, 2024-01-17 at 14:42 +0100, Jan Beulich wrote: >> On 17.01.2024 12:37, Oleksii wrote: >>>>>> >>>>>>>> Also you want to make sure asm-generic/bitops/bitops- >>>>>>>> bits.h >>>>>>>> is >>>>>>>> really in use here, or else an arch overriding / not >>>>>>>> using >>>>>>>> that >>>>>>>> header may end up screwed. >>>>>>> I am not really understand what do you mean. Could you >>>>>>> please >>>>>>> explain a >>>>>>> little bit more. >>>>>> >>>>>> Whichever type you use here, it needs to be in sync with >>>>>> BITOP_BITS_PER_WORD. Hence you want to include the _local_ >>>>>> bitops- >>>>>> bits.h >>>>>> here, such that in case of an inconsistent override by an >>>>>> arch >>>>>> the >>>>>> compiler would complain about the two differring #define-s. >>>>>> (IOW >>>>>> an >>>>>> arch overriding BITOP_BITS_PER_WORD cannot re-use this header >>>>>> as- >>>>>> is.) >>>>>> >>>>>> The same may, btw, be true for others of the new headers you >>>>>> add >>>>>> - >>>>>> the >>>>>> same #include would therefore be needed there as well. >>>>> Now it clear to me. >>>>> >>>>> >>>>> It seems like BITOP_BITS_PER_WORD, BITOP_MASK, BITOP_WORD, and >>>>> BITS_PER_BYTE are defined in {arm, ppc, >>>>> riscv}/include/asm/bitops.h. >>>>> I expected that any architecture planning to use asm- >>>>> generic/bitops/bitops-bits.h would include it at the beginning >>>>> of >>>>> <arch>/include/asm/bitops.h, similar to what is done for RISC- >>>>> V: >>>>> #ifndef _ASM_RISCV_BITOPS_H >>>>> #define _ASM_RISCV_BITOPS_H >>>>> >>>>> #include <asm/system.h> >>>>> >>>>> #include <asm-generic/bitops/bitops-bits.h> >>>>> ... >>>>> >>>>> But in this case, to allow architecture overrides macros, it is >>>>> necessary to update asm-generic/bitops/bitops-bits.h: >>>>> #ifndef BITOP_BITS_PER_WORD >>>>> #define BITOP_BITS_PER_WORD 32 >>>>> #endif >>>>> ... >>>>> Therefore, if an architecture needs to override something, it >>>>> will >>>>> add >>>>> #define ... before #include <asm-generic/bitops/bitops-bits.h>. >>>>> >>>>> Does it make sense? >>>> >>>> Sure. But then the arch also needs to provide a corresponding >>>> typedef >>>> (and bitops-bits.h the fallback one), for use wherever you use >>>> any of >>>> those #define-s. >>> Which one typedef is needed to provide? >>> <asm-generic/bitops/bitops-bits.h> contains only macros. >> >> A new one, to replace where right now you use "unsigned int" and I >> initially said you need to use "uint32_t" instead. With what you said >> earlier, uint32_t won't work there (anymore). > Wouldn't it be enough just to "#include <xen/types.h>" in headers where > "uint32_t" is used? No, my point wasn't to make uint32_t available. We need a _separate_ typedef which matches the #define-s. Otherwise, if an arch defines BITOP_BITS_PER_WORD to, say, 64, this generic code would do the wrong thing. Jan
On 22.12.2023 16:12, Oleksii Kurochko wrote: > --- /dev/null > +++ b/xen/include/asm-generic/bitops/bitops-bits.h > @@ -0,0 +1,10 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _ASM_GENERIC_BITOPS_BITS_H_ > +#define _ASM_GENERIC_BITOPS_BITS_H_ > + > +#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 Btw, I can't spot a use of BITS_PER_BYTE. Why do you add it? And if it really needed adding, it surely wouldn't belong here. Jan
On Thu, 2024-01-18 at 12:03 +0100, Jan Beulich wrote: > On 22.12.2023 16:12, Oleksii Kurochko wrote: > > --- /dev/null > > +++ b/xen/include/asm-generic/bitops/bitops-bits.h > > @@ -0,0 +1,10 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +#ifndef _ASM_GENERIC_BITOPS_BITS_H_ > > +#define _ASM_GENERIC_BITOPS_BITS_H_ > > + > > +#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 > > Btw, I can't spot a use of BITS_PER_BYTE. Why do you add it? And if > it really needed adding, it surely wouldn't belong here. It is used in common/bitmap.c and ns16550.c, and inside some arch code, but it is not used by RISC-V right now. Would it be better to define it in config.h? ~ Oleksii
On 19.01.2024 10:09, Oleksii wrote: > On Thu, 2024-01-18 at 12:03 +0100, Jan Beulich wrote: >> On 22.12.2023 16:12, Oleksii Kurochko wrote: >>> --- /dev/null >>> +++ b/xen/include/asm-generic/bitops/bitops-bits.h >>> @@ -0,0 +1,10 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> +#ifndef _ASM_GENERIC_BITOPS_BITS_H_ >>> +#define _ASM_GENERIC_BITOPS_BITS_H_ >>> + >>> +#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 >> >> Btw, I can't spot a use of BITS_PER_BYTE. Why do you add it? And if >> it really needed adding, it surely wouldn't belong here. > It is used in common/bitmap.c and ns16550.c, and inside some arch code, > but it is not used by RISC-V right now. > > Would it be better to define it in config.h? Yes, perhaps. Imo this shouldn't have a "generic" fallback; every arch should explicitly state this (along with e.g. BITS_PER_LONG). Jan
On Thu, 2024-01-18 at 12:01 +0100, Jan Beulich wrote: > On 18.01.2024 10:43, Oleksii wrote: > > On Wed, 2024-01-17 at 14:42 +0100, Jan Beulich wrote: > > > On 17.01.2024 12:37, Oleksii wrote: > > > > > > > > > > > > > > > > Also you want to make sure asm-generic/bitops/bitops- > > > > > > > > > bits.h > > > > > > > > > is > > > > > > > > > really in use here, or else an arch overriding / not > > > > > > > > > using > > > > > > > > > that > > > > > > > > > header may end up screwed. > > > > > > > > I am not really understand what do you mean. Could you > > > > > > > > please > > > > > > > > explain a > > > > > > > > little bit more. > > > > > > > > > > > > > > Whichever type you use here, it needs to be in sync with > > > > > > > BITOP_BITS_PER_WORD. Hence you want to include the > > > > > > > _local_ > > > > > > > bitops- > > > > > > > bits.h > > > > > > > here, such that in case of an inconsistent override by an > > > > > > > arch > > > > > > > the > > > > > > > compiler would complain about the two differring #define- > > > > > > > s. > > > > > > > (IOW > > > > > > > an > > > > > > > arch overriding BITOP_BITS_PER_WORD cannot re-use this > > > > > > > header > > > > > > > as- > > > > > > > is.) > > > > > > > > > > > > > > The same may, btw, be true for others of the new headers > > > > > > > you > > > > > > > add > > > > > > > - > > > > > > > the > > > > > > > same #include would therefore be needed there as well. > > > > > > Now it clear to me. > > > > > > > > > > > > > > > > > > It seems like BITOP_BITS_PER_WORD, BITOP_MASK, BITOP_WORD, > > > > > > and > > > > > > BITS_PER_BYTE are defined in {arm, ppc, > > > > > > riscv}/include/asm/bitops.h. > > > > > > I expected that any architecture planning to use asm- > > > > > > generic/bitops/bitops-bits.h would include it at the > > > > > > beginning > > > > > > of > > > > > > <arch>/include/asm/bitops.h, similar to what is done for > > > > > > RISC- > > > > > > V: > > > > > > #ifndef _ASM_RISCV_BITOPS_H > > > > > > #define _ASM_RISCV_BITOPS_H > > > > > > > > > > > > #include <asm/system.h> > > > > > > > > > > > > #include <asm-generic/bitops/bitops-bits.h> > > > > > > ... > > > > > > > > > > > > But in this case, to allow architecture overrides macros, > > > > > > it is > > > > > > necessary to update asm-generic/bitops/bitops-bits.h: > > > > > > #ifndef BITOP_BITS_PER_WORD > > > > > > #define BITOP_BITS_PER_WORD 32 > > > > > > #endif > > > > > > ... > > > > > > Therefore, if an architecture needs to override something, > > > > > > it > > > > > > will > > > > > > add > > > > > > #define ... before #include <asm-generic/bitops/bitops- > > > > > > bits.h>. > > > > > > > > > > > > Does it make sense? > > > > > > > > > > Sure. But then the arch also needs to provide a corresponding > > > > > typedef > > > > > (and bitops-bits.h the fallback one), for use wherever you > > > > > use > > > > > any of > > > > > those #define-s. > > > > Which one typedef is needed to provide? > > > > <asm-generic/bitops/bitops-bits.h> contains only macros. > > > > > > A new one, to replace where right now you use "unsigned int" and > > > I > > > initially said you need to use "uint32_t" instead. With what you > > > said > > > earlier, uint32_t won't work there (anymore). > > Wouldn't it be enough just to "#include <xen/types.h>" in headers > > where > > "uint32_t" is used? > > No, my point wasn't to make uint32_t available. We need a _separate_ > typedef which matches the #define-s. Otherwise, if an arch defines > BITOP_BITS_PER_WORD to, say, 64, this generic code would do the wrong > thing. Oh, yeah this is true. We have to introduce in bitops-bits.h: typedef uint_32t bitops_type; And then use it in function such as test_bit: static inline int test_bit(int nr, const volatile void *addr) { const volatile bitops_type *p = addr; return 1 & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD - 1))); } Thanks for clarification. ~ Oleksii
On 19.01.2024 10:16, Oleksii wrote: > On Thu, 2024-01-18 at 12:01 +0100, Jan Beulich wrote: >> On 18.01.2024 10:43, Oleksii wrote: >>> On Wed, 2024-01-17 at 14:42 +0100, Jan Beulich wrote: >>>> On 17.01.2024 12:37, Oleksii wrote: >>>>>>>> >>>>>>>>>> Also you want to make sure asm-generic/bitops/bitops- >>>>>>>>>> bits.h >>>>>>>>>> is >>>>>>>>>> really in use here, or else an arch overriding / not >>>>>>>>>> using >>>>>>>>>> that >>>>>>>>>> header may end up screwed. >>>>>>>>> I am not really understand what do you mean. Could you >>>>>>>>> please >>>>>>>>> explain a >>>>>>>>> little bit more. >>>>>>>> >>>>>>>> Whichever type you use here, it needs to be in sync with >>>>>>>> BITOP_BITS_PER_WORD. Hence you want to include the >>>>>>>> _local_ >>>>>>>> bitops- >>>>>>>> bits.h >>>>>>>> here, such that in case of an inconsistent override by an >>>>>>>> arch >>>>>>>> the >>>>>>>> compiler would complain about the two differring #define- >>>>>>>> s. >>>>>>>> (IOW >>>>>>>> an >>>>>>>> arch overriding BITOP_BITS_PER_WORD cannot re-use this >>>>>>>> header >>>>>>>> as- >>>>>>>> is.) >>>>>>>> >>>>>>>> The same may, btw, be true for others of the new headers >>>>>>>> you >>>>>>>> add >>>>>>>> - >>>>>>>> the >>>>>>>> same #include would therefore be needed there as well. >>>>>>> Now it clear to me. >>>>>>> >>>>>>> >>>>>>> It seems like BITOP_BITS_PER_WORD, BITOP_MASK, BITOP_WORD, >>>>>>> and >>>>>>> BITS_PER_BYTE are defined in {arm, ppc, >>>>>>> riscv}/include/asm/bitops.h. >>>>>>> I expected that any architecture planning to use asm- >>>>>>> generic/bitops/bitops-bits.h would include it at the >>>>>>> beginning >>>>>>> of >>>>>>> <arch>/include/asm/bitops.h, similar to what is done for >>>>>>> RISC- >>>>>>> V: >>>>>>> #ifndef _ASM_RISCV_BITOPS_H >>>>>>> #define _ASM_RISCV_BITOPS_H >>>>>>> >>>>>>> #include <asm/system.h> >>>>>>> >>>>>>> #include <asm-generic/bitops/bitops-bits.h> >>>>>>> ... >>>>>>> >>>>>>> But in this case, to allow architecture overrides macros, >>>>>>> it is >>>>>>> necessary to update asm-generic/bitops/bitops-bits.h: >>>>>>> #ifndef BITOP_BITS_PER_WORD >>>>>>> #define BITOP_BITS_PER_WORD 32 >>>>>>> #endif >>>>>>> ... >>>>>>> Therefore, if an architecture needs to override something, >>>>>>> it >>>>>>> will >>>>>>> add >>>>>>> #define ... before #include <asm-generic/bitops/bitops- >>>>>>> bits.h>. >>>>>>> >>>>>>> Does it make sense? >>>>>> >>>>>> Sure. But then the arch also needs to provide a corresponding >>>>>> typedef >>>>>> (and bitops-bits.h the fallback one), for use wherever you >>>>>> use >>>>>> any of >>>>>> those #define-s. >>>>> Which one typedef is needed to provide? >>>>> <asm-generic/bitops/bitops-bits.h> contains only macros. >>>> >>>> A new one, to replace where right now you use "unsigned int" and >>>> I >>>> initially said you need to use "uint32_t" instead. With what you >>>> said >>>> earlier, uint32_t won't work there (anymore). >>> Wouldn't it be enough just to "#include <xen/types.h>" in headers >>> where >>> "uint32_t" is used? >> >> No, my point wasn't to make uint32_t available. We need a _separate_ >> typedef which matches the #define-s. Otherwise, if an arch defines >> BITOP_BITS_PER_WORD to, say, 64, this generic code would do the wrong >> thing. > Oh, yeah this is true. > > We have to introduce in bitops-bits.h: > typedef uint_32t bitops_type; Perhaps e.g. typedef uint32_t bitops_uint_t; though. Jan > And then use it in function such as test_bit: > static inline int test_bit(int nr, const volatile void *addr) > { > const volatile bitops_type *p = addr; > return 1 & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD - > 1))); > } > > Thanks for clarification. > > ~ Oleksii
On Fri, 2024-01-19 at 10:14 +0100, Jan Beulich wrote: > On 19.01.2024 10:09, Oleksii wrote: > > On Thu, 2024-01-18 at 12:03 +0100, Jan Beulich wrote: > > > On 22.12.2023 16:12, Oleksii Kurochko wrote: > > > > --- /dev/null > > > > +++ b/xen/include/asm-generic/bitops/bitops-bits.h > > > > @@ -0,0 +1,10 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > > +#ifndef _ASM_GENERIC_BITOPS_BITS_H_ > > > > +#define _ASM_GENERIC_BITOPS_BITS_H_ > > > > + > > > > +#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 > > > > > > Btw, I can't spot a use of BITS_PER_BYTE. Why do you add it? And > > > if > > > it really needed adding, it surely wouldn't belong here. > > It is used in common/bitmap.c and ns16550.c, and inside some arch > > code, > > but it is not used by RISC-V right now. > > > > Would it be better to define it in config.h? > > Yes, perhaps. Imo this shouldn't have a "generic" fallback; every > arch > should explicitly state this (along with e.g. BITS_PER_LONG). Got it. Thanks. ~ Oleksii
diff --git a/xen/arch/riscv/include/asm/bitops.h b/xen/arch/riscv/include/asm/bitops.h new file mode 100644 index 0000000000..d210f529c8 --- /dev/null +++ b/xen/arch/riscv/include/asm/bitops.h @@ -0,0 +1,267 @@ +/* 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); +} + +#undef __test_and_op_bit +#undef __op_bit +#undef __NOP +#undef __NOT +#undef __AMO + +#define test_and_set_bit __test_and_set_bit +#define test_and_clear_bit __test_and_clear_bit + +/* Based on linux/include/asm-generic/bitops/find.h */ + +#ifndef find_next_bit +/** + * find_next_bit - find the next set bit in a memory region + * @addr: The address to base the search on + * @offset: The bitnumber to start searching at + * @size: The bitmap size in bits + */ +extern unsigned long find_next_bit(const unsigned long *addr, unsigned long + size, unsigned long offset); +#endif + +#ifndef find_next_zero_bit +/** + * find_next_zero_bit - find the next cleared bit in a memory region + * @addr: The address to base the search on + * @offset: The bitnumber to start searching at + * @size: The bitmap size in bits + */ +extern unsigned long find_next_zero_bit(const unsigned long *addr, unsigned + long size, unsigned long offset); +#endif + +/** + * find_first_bit - find the first set bit in a memory region + * @addr: The address to start the search at + * @size: The maximum size to search + * + * Returns the bit number of the first set bit. + */ +extern unsigned long find_first_bit(const unsigned long *addr, + unsigned long size); + +/** + * find_first_zero_bit - find the first cleared bit in a memory region + * @addr: The address to start the search at + * @size: The maximum size to search + * + * Returns the bit number of the first cleared bit. + */ +extern unsigned long find_first_zero_bit(const unsigned long *addr, + unsigned long size); + +/** + * 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 long __ffs(unsigned long word) +{ + 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; +} + +/** + * 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) +{ + int num = 1; + + if (!word) + return 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; +} + +#include <asm-generic/bitops/fls.h> +#include <asm-generic/bitops/flsl.h> +#include <asm-generic/bitops/ffs.h> +#include <asm-generic/bitops/ffz.h> +#include <asm-generic/bitops/find-first-bit-set.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/include/asm-generic/bitops/bitops-bits.h b/xen/include/asm-generic/bitops/bitops-bits.h new file mode 100644 index 0000000000..8a57e47c63 --- /dev/null +++ b/xen/include/asm-generic/bitops/bitops-bits.h @@ -0,0 +1,10 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _ASM_GENERIC_BITOPS_BITS_H_ +#define _ASM_GENERIC_BITOPS_BITS_H_ + +#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 + +#endif /* _ASM_GENERIC_BITOPS_BITS_H_ */ \ No newline at end of file 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/ffz.h b/xen/include/asm-generic/bitops/ffz.h new file mode 100644 index 0000000000..92c35455d5 --- /dev/null +++ b/xen/include/asm-generic/bitops/ffz.h @@ -0,0 +1,13 @@ +/* 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. + */ +#define ffz(x) __ffs(~(x)) + +#endif /* _ASM_GENERIC_BITOPS_FFZ_H_ */ \ No newline at end of file diff --git a/xen/include/asm-generic/bitops/find-first-bit-set.h b/xen/include/asm-generic/bitops/find-first-bit-set.h new file mode 100644 index 0000000000..8ae9751b11 --- /dev/null +++ b/xen/include/asm-generic/bitops/find-first-bit-set.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _ASM_GENERIC_BITOPS_FIND_FIRST_BIT_SET_H_ +#define _ASM_GENERIC_BITOPS_FIND_FIRST_BIT_SET_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_BIT_SET_H_ */ \ No newline at end of file diff --git a/xen/include/asm-generic/bitops/fls.h b/xen/include/asm-generic/bitops/fls.h new file mode 100644 index 0000000000..f232925080 --- /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_ */ \ No newline at end of file diff --git a/xen/include/asm-generic/bitops/flsl.h b/xen/include/asm-generic/bitops/flsl.h new file mode 100644 index 0000000000..127221056e --- /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_ */ \ No newline at end of file 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..9fa36652e3 --- /dev/null +++ b/xen/include/asm-generic/bitops/test-bit.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _ASM_GENERIC_BITOPS_TESTBIT_H_ +#define _ASM_GENERIC_BITOPS_TESTBIT_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 unsigned int *p = addr; + return 1 & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD - 1))); +} + +#endif /* _ASM_GENERIC_BITOPS_TESTBIT_H_ */ \ No newline at end of file
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_change_bit * test_and_set_bit_lock * clear_bit_unlock * __clear_bit_unlock * The following functions were renamed in the way how they are used by common code: * __test_and_set_bit * __test_and_clear_bit * The declaration and implementation of the following functios were updated to make Xen build happy: * clear_bit * set_bit * __test_and_clear_bit * __test_and_set_bit * linux/include/asm-generic/bitops/find.h ( only few function declaration were taken, as implementation will be provided by Xen ). * linux/arch/include/linux/bits.h ( taken only definitions for BIT_MASK, BIT_WORD, BITS_PER_BYTE ) 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 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 | 267 ++++++++++++++++++ xen/include/asm-generic/bitops/bitops-bits.h | 10 + xen/include/asm-generic/bitops/ffs.h | 9 + xen/include/asm-generic/bitops/ffz.h | 13 + .../asm-generic/bitops/find-first-bit-set.h | 17 ++ xen/include/asm-generic/bitops/fls.h | 18 ++ xen/include/asm-generic/bitops/flsl.h | 10 + xen/include/asm-generic/bitops/hweight.h | 13 + xen/include/asm-generic/bitops/test-bit.h | 16 ++ 9 files changed, 373 insertions(+) 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