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

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

[Daniel Almeida]

In light of bindgen being unable to generate bindings for macros,
manually define the bit and genmask C macros in Rust.

Bit and genmask are frequently used in drivers, and are simple enough to
just be redefined. Their implementation is also unlikely to ever change.

These macros are converted from their kernel equivalent. Versions for
u64, u32, u16 and u8 are provided in order to reduce the number of casts
for callers.

Reviewed-by: Fiona Behrens <me@kloenk.dev>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
Changes in v5:
- Added versions for u16 and u8 in order to reduce the amount of casts
  for callers. This came up after discussing the issue with Alexandre
  Courbot in light of his "register" abstractions.
- Link to v4: https://lore.kernel.org/r/20250318-topic-panthor-rs-genmask-v4-1-35004fca6ac5@collabora.com

Changes in v4:
- Split bits into bits_u32 and bits_u64
- Added r-b's
- Rebased on top of rust-next
- Link to v3: https://lore.kernel.org/r/20250121-topic-panthor-rs-genmask-v3-1-5c3bdf21ce05@collabora.com

Changes in v3:
- Changed from declarative macro to const fn
- Added separate versions for u32 and u64
- Link to v2: https://lore.kernel.org/r/20241024-topic-panthor-rs-genmask-v2-1-85237c1f0cea@collabora.com

Changes in v2:

- Added ticks around `BIT`, and `h >=l` (Thanks, Benno).
- Decided to keep the arguments as `expr`, as I see no issues with that
- Added a proper example, with an assert_eq!() (Thanks, Benno)
- Fixed the condition h <= l, which should be h >= l.
- Checked that the assert for the condition above is described in the
  docs.
---
 rust/kernel/bits.rs | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs  |  1 +
 2 files changed, 94 insertions(+)

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.
+pub const fn bit_u64(n: u32) -> u64 {
+    1u64 << n as u64
+}
+
+/// 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
+}
+
+/// 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 {
+    assert!(h >= l);
+    (!0u64 - (1u64 << l) + 1) & (!0u64 >> (64 - 1 - h))
+}
+
+/// Create a contiguous bitmask starting at bit position `l` and ending at
+/// position `h`, where `h >= l`.
+///
+/// # Examples
+/// ```
+///     use kernel::bits::genmask_u32;
+///     let mask = genmask_u32(9, 0);
+///     assert_eq!(mask, 0x000003ff);
+/// ```
+///
+pub const fn genmask_u32(h: u32, l: u32) -> u32 {
+    assert!(h >= l);
+    (!0u32 - (1u32 << l) + 1) & (!0u32 >> (32 - 1 - h))
+}
+
+/// Create a contiguous bitmask starting at bit position `l` and ending at
+/// position `h`, where `h >= l`.
+///
+/// # Examples
+/// ```
+///     use kernel::bits::genmask_u16;
+///     let mask = genmask_u16(9, 0);
+///     assert_eq!(mask, 0x000003ff);
+/// ```
+///
+pub const fn genmask_u16(h: u32, l: u32) -> u16 {
+    assert!(h >= l);
+    (!0u16 - (1u16 << l) + 1) & (!0u16 >> (16 - 1 - h))
+}
+
+/// Create a contiguous bitmask starting at bit position `l` and ending at
+/// position `h`, where `h >= l`.
+///
+/// # Examples
+/// ```
+///     use kernel::bits::genmask_u8;
+///     let mask = genmask_u8(7, 0);
+///     assert_eq!(mask, 0x000000ff);
+/// ```
+///
+pub const fn genmask_u8(h: u32, l: u32) -> u8 {
+    assert!(h >= l);
+    (!0u8 - (1u8 << l) + 1) & (!0u8 >> (8 - 1 - h))
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index c92497c7c655e8faefd85bb4a1d5b4cc696b8499..a90aaf7fe6755a5a42055b7b4008714fcafe6f6f 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -36,6 +36,7 @@
 pub use ffi;
 
 pub mod alloc;
+pub mod bits;
 #[cfg(CONFIG_BLOCK)]
 pub mod block;
 #[doc(hidden)]

---
base-commit: cf25bc61f8aecad9b0c45fe32697e35ea4b13378
change-id: 20241023-topic-panthor-rs-genmask-fabc573fef43

Best regards,
-- 
Daniel Almeida <daniel.almeida@collabora.com>

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

[Miguel Ojeda]

Hi Daniel,

My usual docs-only review... I hope that helps!

On Wed, Mar 26, 2025 at 3:07 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> +/// Equivalent to the kernel's `BIT` macro.

"To the C `BIT` macro" or "The C side ..." or similar -- these one
would be also the kernel's :)

> +/// Create a contiguous bitmask starting at bit position `l` and ending at
> +/// position `h`, where `h >= l`.

The first paragraph is a "short description" / title -- you may want
to leave the details to a second sentence, i.e. in a second paragraph.
Please check in any case how it looks in the rendered docs -- it may
be fine to have all in the title.

In fact, given you `assert!`, we should probably mention that very
prominently e.g. in a `# Panics` section. Or, better, avoid the panics
to begin with if it makes sense.

> +/// # Examples
> +/// ```

(Multiple instances) Newline between these.

> +///     use kernel::bits::genmask_u64;

(Multiple instances) You can hide this one with `#` -- when the `use`
is just for a single free function, I think it is not very useful to
show in the rendered docs, i.e. it is clear that you are showing that
one since the docs go with it.

> +///     let mask = genmask_u64(39, 21);
> +///     assert_eq!(mask, 0x000000ffffe00000);

(Multiple instances) The example is overindented, as if it was inside
a function.

> +///
> +pub const fn genmask_u32(h: u32, l: u32) -> u32 {

(Multiple instances) Extra `///` line.

Thanks!

Cheers,
Miguel

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

[Daniel Almeida]

Hi Miguel,

> On 27 Mar 2025, at 18:27, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> 
> Hi Daniel,
> 
> My usual docs-only review... I hope that helps!
> 
> On Wed, Mar 26, 2025 at 3:07 PM Daniel Almeida
> <daniel.almeida@collabora.com> wrote:
>> 
>> +/// Equivalent to the kernel's `BIT` macro.
> 
> "To the C `BIT` macro" or "The C side ..." or similar -- these one
> would be also the kernel's :)
> 
>> +/// Create a contiguous bitmask starting at bit position `l` and ending at
>> +/// position `h`, where `h >= l`.
> 
> The first paragraph is a "short description" / title -- you may want
> to leave the details to a second sentence, i.e. in a second paragraph.
> Please check in any case how it looks in the rendered docs -- it may
> be fine to have all in the title.
> 
> In fact, given you `assert!`, we should probably mention that very
> prominently e.g. in a `# Panics` section. Or, better, avoid the panics
> to begin with if it makes sense.

I have been staring at this for a little while.

I wonder what is everyone's opinions on an extra set of:

// e.g.: for u32
const fn const_genmask_u32<const H: u32, const L: u32>() -> u32 {
  crate::build_assert!(H >= L);
  ...
}

..on top of the current genmask functions we already have?

This lets us move the checks to compile time for most cases, because for the
majority of users, h and l are simply integer literals.  

For the rest, we can probably modify the current functions:

fn genmask_u32(h: u32, l: u32) -> Result<u32> {
  if(h < l) {
    return Err(EINVAL);
  }
  ..
}

The implementation can probably be shared by using macros like kernel::io::Io,
for example, and the panics would be gone.

— Daniel

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

[Miguel Ojeda]

On Tue, May 13, 2025 at 8:52 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> I have been staring at this for a little while.
>
> I wonder what is everyone's opinions on an extra set of:
>
> // e.g.: for u32
> const fn const_genmask_u32<const H: u32, const L: u32>() -> u32 {
>   crate::build_assert!(H >= L);
>   ...
> }
>
> ..on top of the current genmask functions we already have?

It seems you want `consteval` from C++ :)

Without having thought about the particular use case, just a quick
note: if you have const generics, then you can use `const { assert!(H
>= L); }` instead.

Nowadays `.unwrap()` on `Option` is `const`, but not `Result`'s. That
would be a way to have a single fallible function that allows users to
decide to unwrap in a const context or use them fallibly for runtime
values. Another is having a custom `const` unwrap for those concrete
`Result`s.

Cheers,
Miguel

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

[Alexandre Courbot]

Hi Daniel,

On Wed Mar 26, 2025 at 11:06 PM JST, Daniel Almeida wrote:
> In light of bindgen being unable to generate bindings for macros,
> manually define the bit and genmask C macros in Rust.
>
> Bit and genmask are frequently used in drivers, and are simple enough to
> just be redefined. Their implementation is also unlikely to ever change.
>
> These macros are converted from their kernel equivalent. Versions for
> u64, u32, u16 and u8 are provided in order to reduce the number of casts
> for callers.

This looks good to me, my only suggestion would be to try and use a
macro to generate the methods from a shared template, to rule out
copy/paste errors? Similarly to what we do in [1].

I guess this means the examples would need to be copied to another place
(module documentation?) for them to run, although I am not quite sure
whether this is needed.

[1] https://lore.kernel.org/rust-for-linux/20250521-nova-frts-v4-4-05dfd4f39479@nvidia.com/T/#u

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

[Alexandre Courbot]

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.

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

[Alexandre Courbot]

On Wed May 14, 2025 at 3:52 AM JST, Daniel Almeida wrote:
> Hi Miguel,
>
>> On 27 Mar 2025, at 18:27, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>> 
>> Hi Daniel,
>> 
>> My usual docs-only review... I hope that helps!
>> 
>> On Wed, Mar 26, 2025 at 3:07 PM Daniel Almeida
>> <daniel.almeida@collabora.com> wrote:
>>> 
>>> +/// Equivalent to the kernel's `BIT` macro.
>> 
>> "To the C `BIT` macro" or "The C side ..." or similar -- these one
>> would be also the kernel's :)
>> 
>>> +/// Create a contiguous bitmask starting at bit position `l` and ending at
>>> +/// position `h`, where `h >= l`.
>> 
>> The first paragraph is a "short description" / title -- you may want
>> to leave the details to a second sentence, i.e. in a second paragraph.
>> Please check in any case how it looks in the rendered docs -- it may
>> be fine to have all in the title.
>> 
>> In fact, given you `assert!`, we should probably mention that very
>> prominently e.g. in a `# Panics` section. Or, better, avoid the panics
>> to begin with if it makes sense.
>
> I have been staring at this for a little while.
>
> I wonder what is everyone's opinions on an extra set of:
>
> // e.g.: for u32
> const fn const_genmask_u32<const H: u32, const L: u32>() -> u32 {
>   crate::build_assert!(H >= L);
>   ...
> }
>
> ..on top of the current genmask functions we already have?

Do we need to make this generic? IIUC `build_assert` should enforce that
the condition can be guaranteed at build time, even without the const
generics (which a quick test seems to confirm).

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

[Miguel Ojeda]

On Thu, May 22, 2025 at 5:17 AM Alexandre Courbot <acourbot@nvidia.com> wrote:
>
>                 // 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
>                 }

I think this was meant to be `build_assert!`. `static_assert!` cannot
be used when the value is potentially not known.

(It would need to be `#[inline]` too).

Cheers,
Miguel

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

[Alexandre Courbot]

On Thu May 22, 2025 at 5:21 PM JST, Miguel Ojeda wrote:
> On Thu, May 22, 2025 at 5:17 AM Alexandre Courbot <acourbot@nvidia.com> wrote:
>>
>>                 // 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
>>                 }
>
> I think this was meant to be `build_assert!`. `static_assert!` cannot
> be used when the value is potentially not known.

Indeed, thanks for correcting!

>
> (It would need to be `#[inline]` too).

Like everything else in this module. :)

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

[Miguel Ojeda]

On Thu, May 22, 2025 at 10:23 AM Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> Like everything else in this module. :)

I meant it in the sense that using `build_assert!` means one cannot
compile down a "concrete implementation" because the value wouldn't be
able to be checked (so it needs to use generics or otherwise be marked
as `#[inline]`).

Cheers,
Miguel