| Message ID | 20240802040506.712-1-dqfext@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | riscv: support KASAN instrumentation of bitops | expand |
Hi Qingfang, On 02/08/2024 06:05, Qingfang Deng wrote: > From: Qingfang Deng <qingfang.deng@siflower.com.cn> > > The arch-specific bitops are not being picked up by the KASAN test > suite. > > Instrumentation is done via the bitops/instrumented-{atomic,lock}.h > headers. They require that arch-specific versions of bitop functions > are renamed to arch_*. Do this renaming. > > As most comments are identical to the ones in the instrumented headers, > remove them. > > Signed-off-by: Qingfang Deng <qingfang.deng@siflower.com.cn> > --- > arch/riscv/include/asm/bitops.h | 100 +++++--------------------------- > 1 file changed, 15 insertions(+), 85 deletions(-) > > diff --git a/arch/riscv/include/asm/bitops.h b/arch/riscv/include/asm/bitops.h > index 71af9ecfcfcb..44ff3114c112 100644 > --- a/arch/riscv/include/asm/bitops.h > +++ b/arch/riscv/include/asm/bitops.h > @@ -221,134 +221,62 @@ static __always_inline int variable_fls(unsigned int x) > #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 unsigned long *addr) > +static inline int arch_test_and_set_bit(int nr, volatile unsigned long *addr) > { > 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 unsigned long *addr) > +static inline int arch_test_and_clear_bit(int nr, volatile unsigned long *addr) > { > return __test_and_op_bit(and, __NOT, nr, addr); > } > > -/** > - * test_and_change_bit - Change a bit and return its old value > - * @nr: Bit to change > - * @addr: Address to count from > - * > - * This operation is atomic and cannot be reordered. > - * It also implies a memory barrier. > - */ > -static inline int test_and_change_bit(int nr, volatile unsigned long *addr) > +static inline int arch_test_and_change_bit(int nr, volatile unsigned long *addr) > { > return __test_and_op_bit(xor, __NOP, 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 unsigned long *addr) > +static inline void arch_set_bit(int nr, volatile unsigned long *addr) > { > __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 unsigned long *addr) > +static inline void arch_clear_bit(int nr, volatile unsigned long *addr) > { > __op_bit(and, __NOT, nr, addr); > } > > -/** > - * change_bit - Toggle a bit in memory > - * @nr: Bit to change > - * @addr: Address to start counting from > - * > - * change_bit() may be reordered on other architectures than x86. > - * Note that @nr may be almost arbitrarily large; this function is not > - * restricted to acting on a single-word quantity. > - */ > -static inline void change_bit(int nr, volatile unsigned long *addr) > +static inline void arch_change_bit(int nr, volatile unsigned long *addr) > { > __op_bit(xor, __NOP, nr, addr); > } > > -/** > - * test_and_set_bit_lock - Set a bit and return its old value, for lock > - * @nr: Bit to set > - * @addr: Address to count from > - * > - * This operation is atomic and provides acquire barrier semantics. > - * It can be used to implement bit locks. > - */ > -static inline int test_and_set_bit_lock( > +static inline int arch_test_and_set_bit_lock( > unsigned long nr, volatile unsigned long *addr) > { > return __test_and_op_bit_ord(or, __NOP, nr, addr, .aq); > } > > -/** > - * clear_bit_unlock - Clear a bit in memory, for unlock > - * @nr: the bit to set > - * @addr: the address to start counting from > - * > - * This operation is atomic and provides release barrier semantics. > - */ > -static inline void clear_bit_unlock( > +static inline void arch_clear_bit_unlock( > unsigned long nr, volatile unsigned long *addr) > { > __op_bit_ord(and, __NOT, nr, addr, .rl); > } > > /** > - * __clear_bit_unlock - Clear a bit in memory, for unlock > - * @nr: the bit to set > - * @addr: the address to start counting from > + * arch___clear_bit_unlock - Clear a bit in memory, for unlock > * > - * This operation is like clear_bit_unlock, however it is not atomic. > - * It does provide release barrier semantics so it can be used to unlock > - * a bit lock, however it would only be used if no other CPU can modify > - * any bits in the memory until the lock is released (a good example is > - * if the bit lock itself protects access to the other bits in the word). > + * This should not be used directly, use the instrumented __clear_bit_unlock > + * instead. See asm-generic/bitops/instrumented-lock.h > * > * On RISC-V systems there seems to be no benefit to taking advantage of the > * non-atomic property here: it's a lot more instructions and we still have to > * provide release semantics anyway. > */ > -static inline void __clear_bit_unlock( > +static inline void arch___clear_bit_unlock( > unsigned long nr, volatile unsigned long *addr) > { > - clear_bit_unlock(nr, addr); > + arch_clear_bit_unlock(nr, addr); > } > > static inline bool xor_unlock_is_negative_byte(unsigned long mask, > @@ -369,6 +297,8 @@ static inline bool xor_unlock_is_negative_byte(unsigned long mask, > #undef __NOT > #undef __AMO > > +#include <asm-generic/bitops/instrumented-atomic.h> > +#include <asm-generic/bitops/instrumented-lock.h> > #include <asm-generic/bitops/non-atomic.h> > #include <asm-generic/bitops/le.h> > #include <asm-generic/bitops/ext2-atomic.h> Samuel has just posted a similar patch here https://lore.kernel.org/linux-riscv/20240801033725.28816-1-samuel.holland@sifive.com/T/#t Thanks, Alex
On Fri, Aug 02, 2024 at 07:45:38AM +0200, Alexandre Ghiti wrote: > Hi Qingfang, > > On 02/08/2024 06:05, Qingfang Deng wrote: > > From: Qingfang Deng <qingfang.deng@siflower.com.cn> > > > > The arch-specific bitops are not being picked up by the KASAN test > > suite. > > > > Instrumentation is done via the bitops/instrumented-{atomic,lock}.h > > headers. They require that arch-specific versions of bitop functions > > are renamed to arch_*. Do this renaming. > > > > As most comments are identical to the ones in the instrumented headers, > > remove them. > > > > Signed-off-by: Qingfang Deng <qingfang.deng@siflower.com.cn> > Samuel has just posted a similar patch here https://lore.kernel.org/linux-riscv/20240801033725.28816-1-samuel.holland@sifive.com/T/#t Additionally Samuel's patch has the advantage of compiling :) This one errors with: include/asm-generic/bitops/instrumented-lock.h:80:16: error: implicit declaration of function 'arch_xor_unlock_is_negative_byte'; did you mean 'xor_unlock_is_negative_byte'? [-Werror=implicit-function-declaration]
diff --git a/arch/riscv/include/asm/bitops.h b/arch/riscv/include/asm/bitops.h index 71af9ecfcfcb..44ff3114c112 100644 --- a/arch/riscv/include/asm/bitops.h +++ b/arch/riscv/include/asm/bitops.h @@ -221,134 +221,62 @@ static __always_inline int variable_fls(unsigned int x) #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 unsigned long *addr) +static inline int arch_test_and_set_bit(int nr, volatile unsigned long *addr) { 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 unsigned long *addr) +static inline int arch_test_and_clear_bit(int nr, volatile unsigned long *addr) { return __test_and_op_bit(and, __NOT, nr, addr); } -/** - * test_and_change_bit - Change a bit and return its old value - * @nr: Bit to change - * @addr: Address to count from - * - * This operation is atomic and cannot be reordered. - * It also implies a memory barrier. - */ -static inline int test_and_change_bit(int nr, volatile unsigned long *addr) +static inline int arch_test_and_change_bit(int nr, volatile unsigned long *addr) { return __test_and_op_bit(xor, __NOP, 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 unsigned long *addr) +static inline void arch_set_bit(int nr, volatile unsigned long *addr) { __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 unsigned long *addr) +static inline void arch_clear_bit(int nr, volatile unsigned long *addr) { __op_bit(and, __NOT, nr, addr); } -/** - * change_bit - Toggle a bit in memory - * @nr: Bit to change - * @addr: Address to start counting from - * - * change_bit() may be reordered on other architectures than x86. - * Note that @nr may be almost arbitrarily large; this function is not - * restricted to acting on a single-word quantity. - */ -static inline void change_bit(int nr, volatile unsigned long *addr) +static inline void arch_change_bit(int nr, volatile unsigned long *addr) { __op_bit(xor, __NOP, nr, addr); } -/** - * test_and_set_bit_lock - Set a bit and return its old value, for lock - * @nr: Bit to set - * @addr: Address to count from - * - * This operation is atomic and provides acquire barrier semantics. - * It can be used to implement bit locks. - */ -static inline int test_and_set_bit_lock( +static inline int arch_test_and_set_bit_lock( unsigned long nr, volatile unsigned long *addr) { return __test_and_op_bit_ord(or, __NOP, nr, addr, .aq); } -/** - * clear_bit_unlock - Clear a bit in memory, for unlock - * @nr: the bit to set - * @addr: the address to start counting from - * - * This operation is atomic and provides release barrier semantics. - */ -static inline void clear_bit_unlock( +static inline void arch_clear_bit_unlock( unsigned long nr, volatile unsigned long *addr) { __op_bit_ord(and, __NOT, nr, addr, .rl); } /** - * __clear_bit_unlock - Clear a bit in memory, for unlock - * @nr: the bit to set - * @addr: the address to start counting from + * arch___clear_bit_unlock - Clear a bit in memory, for unlock * - * This operation is like clear_bit_unlock, however it is not atomic. - * It does provide release barrier semantics so it can be used to unlock - * a bit lock, however it would only be used if no other CPU can modify - * any bits in the memory until the lock is released (a good example is - * if the bit lock itself protects access to the other bits in the word). + * This should not be used directly, use the instrumented __clear_bit_unlock + * instead. See asm-generic/bitops/instrumented-lock.h * * On RISC-V systems there seems to be no benefit to taking advantage of the * non-atomic property here: it's a lot more instructions and we still have to * provide release semantics anyway. */ -static inline void __clear_bit_unlock( +static inline void arch___clear_bit_unlock( unsigned long nr, volatile unsigned long *addr) { - clear_bit_unlock(nr, addr); + arch_clear_bit_unlock(nr, addr); } static inline bool xor_unlock_is_negative_byte(unsigned long mask, @@ -369,6 +297,8 @@ static inline bool xor_unlock_is_negative_byte(unsigned long mask, #undef __NOT #undef __AMO +#include <asm-generic/bitops/instrumented-atomic.h> +#include <asm-generic/bitops/instrumented-lock.h> #include <asm-generic/bitops/non-atomic.h> #include <asm-generic/bitops/le.h> #include <asm-generic/bitops/ext2-atomic.h>