linux-kernel - Re: [PATCH] riscv: Always inline bitops

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <Z0dtohAlZqad841q@yury-ThinkPad>
Date: Wed, 27 Nov 2024 11:06:10 -0800
From: Yury Norov <yury.norov@...il.com>
To: Nathan Chancellor <nathan@...nel.org>
Cc: Palmer Dabbelt <palmer@...belt.com>,
	Rasmus Villemoes <linux@...musvillemoes.dk>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Albert Ou <aou@...s.berkeley.edu>, Conor Dooley <conor@...nel.org>,
	linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
	llvm@...ts.linux.dev
Subject: Re: [PATCH] riscv: Always inline bitops

On Sat, Nov 23, 2024 at 07:30:19PM -0700, Nathan Chancellor wrote:
> When building allmodconfig + ThinLTO with certain versions of clang,
> arch_set_bit() may not be inlined, resulting in a modpost warning:
> 
>   WARNING: modpost: vmlinux: section mismatch in reference: arch_set_bit+0x58 (section: .text.arch_set_bit) -> numa_nodes_parsed (section: .init.data)
> 
> acpi_numa_rintc_affinity_init() calls arch_set_bit() via __node_set()
> with numa_nodes_parsed, which is marked as __initdata. If arch_set_bit()
> is not inlined, modpost will flag that it is being called with data that
> will be freed after init.
> 
> As acpi_numa_rintc_affinity_init() is marked as __init, there is not
> actually a functional issue here. However, the bitop functions should be
> marked as __always_inline, so that they work consistently for init and
> non-init code, which the comment in include/linux/nodemask.h alludes to.
> This matches s390 and x86's implementations.
> 
> Signed-off-by: Nathan Chancellor <nathan@...nel.org>

Applied, thanks!

So we have generic, x86 and riscv bitops inlined, and powerpc and s390
non-inlined. We need to align them all, I think.

Thanks,
Yury

> ---
>  arch/riscv/include/asm/bitops.h | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/bitops.h b/arch/riscv/include/asm/bitops.h
> index fae152ea0508d2e1ea490fffd645eab99cf387bf..c6bd3d8354a96b4e7bbef0e98a201da412301b57 100644
> --- a/arch/riscv/include/asm/bitops.h
> +++ b/arch/riscv/include/asm/bitops.h
> @@ -228,7 +228,7 @@ static __always_inline int variable_fls(unsigned int x)
>   *
>   * This operation may be reordered on other architectures than x86.
>   */
> -static inline int arch_test_and_set_bit(int nr, volatile unsigned long *addr)
> +static __always_inline int arch_test_and_set_bit(int nr, volatile unsigned long *addr)
>  {
>  	return __test_and_op_bit(or, __NOP, nr, addr);
>  }
> @@ -240,7 +240,7 @@ static inline int arch_test_and_set_bit(int nr, volatile unsigned long *addr)
>   *
>   * This operation can be reordered on other architectures other than x86.
>   */
> -static inline int arch_test_and_clear_bit(int nr, volatile unsigned long *addr)
> +static __always_inline int arch_test_and_clear_bit(int nr, volatile unsigned long *addr)
>  {
>  	return __test_and_op_bit(and, __NOT, nr, addr);
>  }
> @@ -253,7 +253,7 @@ static inline int arch_test_and_clear_bit(int nr, volatile unsigned long *addr)
>   * This operation is atomic and cannot be reordered.
>   * It also implies a memory barrier.
>   */
> -static inline int arch_test_and_change_bit(int nr, volatile unsigned long *addr)
> +static __always_inline int arch_test_and_change_bit(int nr, volatile unsigned long *addr)
>  {
>  	return __test_and_op_bit(xor, __NOP, nr, addr);
>  }
> @@ -270,7 +270,7 @@ static inline int arch_test_and_change_bit(int nr, volatile unsigned long *addr)
>   * Note that @nr may be almost arbitrarily large; this function is not
>   * restricted to acting on a single-word quantity.
>   */
> -static inline void arch_set_bit(int nr, volatile unsigned long *addr)
> +static __always_inline void arch_set_bit(int nr, volatile unsigned long *addr)
>  {
>  	__op_bit(or, __NOP, nr, addr);
>  }
> @@ -284,7 +284,7 @@ static inline void arch_set_bit(int nr, volatile unsigned long *addr)
>   * on non x86 architectures, so if you are writing portable code,
>   * make sure not to rely on its reordering guarantees.
>   */
> -static inline void arch_clear_bit(int nr, volatile unsigned long *addr)
> +static __always_inline void arch_clear_bit(int nr, volatile unsigned long *addr)
>  {
>  	__op_bit(and, __NOT, nr, addr);
>  }
> @@ -298,7 +298,7 @@ static inline void arch_clear_bit(int nr, volatile unsigned long *addr)
>   * Note that @nr may be almost arbitrarily large; this function is not
>   * restricted to acting on a single-word quantity.
>   */
> -static inline void arch_change_bit(int nr, volatile unsigned long *addr)
> +static __always_inline void arch_change_bit(int nr, volatile unsigned long *addr)
>  {
>  	__op_bit(xor, __NOP, nr, addr);
>  }
> @@ -311,7 +311,7 @@ static inline void arch_change_bit(int nr, volatile unsigned long *addr)
>   * This operation is atomic and provides acquire barrier semantics.
>   * It can be used to implement bit locks.
>   */
> -static inline int arch_test_and_set_bit_lock(
> +static __always_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);
> @@ -324,7 +324,7 @@ static inline int arch_test_and_set_bit_lock(
>   *
>   * This operation is atomic and provides release barrier semantics.
>   */
> -static inline void arch_clear_bit_unlock(
> +static __always_inline void arch_clear_bit_unlock(
>  	unsigned long nr, volatile unsigned long *addr)
>  {
>  	__op_bit_ord(and, __NOT, nr, addr, .rl);
> @@ -345,13 +345,13 @@ static inline void arch_clear_bit_unlock(
>   * non-atomic property here: it's a lot more instructions and we still have to
>   * provide release semantics anyway.
>   */
> -static inline void arch___clear_bit_unlock(
> +static __always_inline void arch___clear_bit_unlock(
>  	unsigned long nr, volatile unsigned long *addr)
>  {
>  	arch_clear_bit_unlock(nr, addr);
>  }
>  
> -static inline bool arch_xor_unlock_is_negative_byte(unsigned long mask,
> +static __always_inline bool arch_xor_unlock_is_negative_byte(unsigned long mask,
>  		volatile unsigned long *addr)
>  {
>  	unsigned long res;
> 
> ---
> base-commit: 0eb512779d642b21ced83778287a0f7a3ca8f2a1
> change-id: 20241123-riscv-always-inline-bitops-0021c4dae36b
> 
> Best regards,
> -- 
> Nathan Chancellor <nathan@...nel.org>