Re: [PATCH v6 5/8] riscv/runtime-const: Introduce runtime_const_mask_32()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The BUG_ON bounds check in `__runtime_fixup_mask` incorrectly panics the kernel on a valid 32-bit all-ones mask (0xFFFFFFFF) due to an off-by-one error caused by confusing bit index with bit width.
- [Medium] The commit message and inline comments incorrectly describe the instruction sequence and immediate shift calculations, contradicting the correct code implementation.
--

--- Patch [5]: [PATCH v6 5/8] riscv/runtime-const: Introduce runtime_const_mask_32() ---
commit f8085a85ab8321e958c65177cacf4c654afce3c8
Author: K Prateek Nayak <kprateek.nayak@amd.com>
Subject: riscv/runtime-const: Introduce runtime_const_mask_32()

This commit introduces runtime_const_mask_32() to optimize mask operations in
the futex hash computation hot path on RISC-V. It implements this using left
and right shifts, patching the immediate values at runtime.

Link: https://docs.riscv.org/reference/isa/_attachments/riscv-unprivileged.pdf [1]
Link: https://lore.kernel.org/lkml/20260430094730.31624-6-kprateek.nayak@amd.com/ [2]

> Introduce runtime_const_mask_32 to further optimize the mask operation
> in the futex hash computation hot path. Since all the current use-cases
> are of the form GENMASK(n, 0), with n > 0, following sequence:
> 
>   srli a0, a1, imm
>   slli a0, a0, imm
> 
> is used for RISC-V where imm = (31 - width) to improve instruction
> density and performance.

[Severity: Medium]
Is this description in the commit message accurate?

The commit message describes using an srli + slli sequence and calculates the
shift as imm = 31 - width. However, the actual macro implementation correctly
uses an slli + srli sequence (shifting left then right) to clear the upper
bits, and __runtime_fixup_mask() uses imm = 32 - width.

Could the commit message be updated to match the implementation?

> diff --git a/arch/riscv/include/asm/runtime-const.h b/arch/riscv/include/asm/runtime-const.h
> index 1ce02605d2e43..a472ebbf09718 100644
> --- a/arch/riscv/include/asm/runtime-const.h
> +++ b/arch/riscv/include/asm/runtime-const.h
[ ... ]
> @@ -262,6 +279,33 @@ static inline void __runtime_fixup_shift(void *where, unsigned long val)
>  	mutex_unlock(&text_mutex);
>  }
>  
> +static inline void __runtime_fixup_mask(void *where, unsigned long val)
> +{
> +	unsigned int width = (val) ? __fls(val) + 1 : 0;
> +
> +	/*
> +	 * XXX: Current implementation only supports patching masks of
> +	 * form GENMASK(width, 0) (width >= 0) using a SRLI + SLLI

[Severity: Medium]
Is the inline documentation here slightly out of sync with the code?

Similar to the commit message, this mentions SRLI + SLLI and GENMASK(width, 0),
but the code below uses GENMASK(width - 1, 0) and the macro actually implements 
SLLI + SRLI.

> +	 * sequence instead of LUI + ADDI + AND sequence to improve
[ ... ]
> +	 * and outside the asm block before returning the result. Fixup
> +	 * can simply reuse the existing __runtime_fixup_32() to patch
> +	 * the LUI + ADDI sequence.
> +	 */
> +	BUG_ON(!val || width > 31 || (GENMASK(width - 1, 0) != val));

[Severity: Medium]
Does this BUG_ON() condition reject a valid 32-bit mask?

When the mask is all ones (0xFFFFFFFF), __fls(val) + 1 evaluates to a width
of 32. The width > 31 check would trip the BUG_ON() and panic the kernel.

A shift of 32 - 32 = 0 is mathematically valid for RISC-V, as using slliw
and srliw with a 0 shift correctly truncates and zero-extends the 32-bit
value to 64 bits.

Should this check allow a full 32-bit mask instead?

> +
> +	__runtime_fixup_shift(where, 32 - width);
> +	__runtime_fixup_shift(where + 4, 32 - width);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728052540.4728-1-kprateek.nayak@amd.com?part=5