Re: [PATCH v5] rust: kernel: add support for bits/genmask macros

["Alexandre Courbot" <acourbot@nvidia.com>]

On Wed Mar 26, 2025 at 11:06 PM JST, Daniel Almeida wrote:
<snip>
> diff --git a/rust/kernel/bits.rs b/rust/kernel/bits.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..ddae8a5be4698bb7df66ee2c42ac6c2bc07eae7e
> --- /dev/null
> +++ b/rust/kernel/bits.rs
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Bit manipulation macros.
> +//!
> +//! C header: [`include/linux/bits.h`](srctree/include/linux/bits.h)
> +
> +/// Produces a literal where bit `n` is set.
> +///
> +/// Equivalent to the kernel's `BIT` macro.

The compiler is probably smart enough to figure that out, but shall we
make all these functions `#[inline(always)]`?

Also, how about adding a few examples for `bit_*` in their respective
doc comment, similarly to `genmask_*`?

> +pub const fn bit_u64(n: u32) -> u64 {
> +    1u64 << n as u64

The `n as u64` cast seems unneeded (and in other functions as well).

> +}
> +
> +/// Produces a literal where bit `n` is set.
> +///
> +/// Equivalent to the kernel's `BIT` macro.
> +pub const fn bit_u32(n: u32) -> u32 {
> +    1u32 << n
> +}
> +
> +/// Produces a literal where bit `n` is set.
> +///
> +/// Equivalent to the kernel's `BIT` macro.
> +pub const fn bit_u16(n: u32) -> u16 {
> +    1u16 << n as u16
> +}
> +
> +/// Produces a literal where bit `n` is set.
> +///
> +/// Equivalent to the kernel's `BIT` macro.
> +pub const fn bit_u8(n: u32) -> u8 {
> +    1u8 << n as u8
> +}

Doing `bit_u8(foo)` if `foo >=8` (and the compiler cannot determine this
at build-time) will overflow and possibly panic. This should be
documented at the very least, but the best would be to avoid that
entirely.

Maybe we could have several variants:

	  // Returns `None` if `n` is out of bounds.
	  pub fn checked_bit_u32(n: u32) -> Option<u32> {
			  1u32.checked_shl(n)
		}

		// Returns `0` if `n` is out of bounds.
		pub fn unbounded_bit_u32(n: u32) -> u32 {
			  // Cannot use `unwrap_or` as it is not const.
			  match checked_bit_u32(n) {
					  Some(v) => v,
					  None => 0,
				}
		}

		// Compile-time error if `n` is out of bounds.
		pub const fn bit_u32(n: u32) -> u32 {
        // Only accept values known at compile-time.
			  static_assert!(n < u32::BITS);
			  1u32 << n
		}

All versions are guaranteed to never panic, and can come in handy depending on
context. The preferred one being `bit_u32` with a constant value, but if the
bit index is not known until runtime then users can use one of the other
variants depending on whether they want to validate the input.

I know that's a lot more functions, but the standard library does that
with e.g. `checked_add`, `overflowing_add`, etc. So it is definitely an
accepted pattern.

> +
> +/// Create a contiguous bitmask starting at bit position `l` and ending at
> +/// position `h`, where `h >= l`.
> +///
> +/// # Examples
> +/// ```
> +///     use kernel::bits::genmask_u64;
> +///     let mask = genmask_u64(39, 21);
> +///     assert_eq!(mask, 0x000000ffffe00000);
> +/// ```
> +///
> +pub const fn genmask_u64(h: u32, l: u32) -> u64 {

Would it make sense to take a range as argument here? This would invert
`h` and `l`, but carries the intent better imho, e.g.

    let mask = genmask_u64(8..15);

Makes it pretty clear that bits 8 to 15 will constitute the mask.

> +    assert!(h >= l);

Do we want to use asserts here? This adds a path for the kernel to panic in a
very common function, and it looks like we are trying to avoid such panics when
they are preventable:

https://lore.kernel.org/rust-for-linux/aBJPwKeJy1ixtwg2@pollux/

If `h > l` then this function returns 0 - I wonder if we cannot just accept
that this is a valid input.

One thing to consider also is how to behave when `h` or `l` is larger than the
number of bits in the type. The current version overflows, so maybe we need to
introduce several variants here as well.

> +    (!0u64 - (1u64 << l) + 1) & (!0u64 >> (64 - 1 - h))

Nit: using `u64::MAX` might be more idiomatic than `!0u64`.

Instead of doing `(1u64 << l)`, let's leverage one of the `bit_u64`
methods since they are available.

`(64 - 1 - h)` can also be `(u64::BITS - 1 - h)`. Here as well, beware
of underflows.