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

Daniel Almeida posted 1 patch 3 months, 2 weeks ago
There is a newer version of this series
rust/kernel/bits.rs | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs  |   1 +
2 files changed, 134 insertions(+)
[PATCH v7] rust: kernel: add support for bits/genmask macros
Posted by Daniel Almeida 3 months, 2 weeks ago
In light of bindgen being unable to generate bindings for macros, and
owing to the widespread use of these macros in drivers, manually define
the bit and genmask C macros in Rust.

The *_checked version of the functions provide runtime checking while
the const version performs compile-time assertions on the arguments via
the build_assert!() macro.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
Changes in v7:
- Rebase on top of latest rust-next
- Use RangeInclusive
- Fix formatting
- Fix checks of start/end to account for RangeInclusive
- Use the paste macro to simplify the implementation
- Get rid of the _unbounded versions (they were not const fns anyway, so
  users can just write it themselves)
- Change the examples to account for RangeInclusive
- Link to v6: https://lore.kernel.org/r/20250610-topic-panthor-rs-genmask-v6-1-50fa1a981bc1@collabora.com

Changes in v6:
Thanks, Alex {
  - Added _checked and _unbounded versions of the functions
  - Implemented the functions through a macro
  - Changed the genmask logic to prevent over/underflow (hopefully)
  - Genmask now takes a range instead of "h" and "l" arguments
  - Made all functions #[inline]
  - +cc Alex directly
  - Removed all panics
}
- Link to v5: https://lore.kernel.org/r/20250326-topic-panthor-rs-genmask-v5-1-bfa6140214da@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 | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs  |   1 +
 2 files changed, 134 insertions(+)

diff --git a/rust/kernel/bits.rs b/rust/kernel/bits.rs
new file mode 100644
index 0000000000000000000000000000000000000000..8db122b5db9565b76c14fc33e33058f9dac7bd75
--- /dev/null
+++ b/rust/kernel/bits.rs
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Bit manipulation macros.
+//!
+//! C header: [`include/linux/bits.h`](srctree/include/linux/bits.h)
+
+use crate::prelude::*;
+use core::ops::RangeInclusive;
+use macros::paste;
+
+macro_rules! impl_bit_fn {
+    (
+        $ty:ty
+    ) => {
+        paste! {
+            /// Computes `1 << n` if `n` is in bounds, i.e.: if `n` is smaller than
+            /// the maximum number of bits supported by the type.
+            ///
+            /// Returns [`None`] otherwise.
+            #[inline]
+            pub fn [<checked_bit_ $ty>](n: u32) -> Option<$ty> {
+                (1 as $ty).checked_shl(n)
+            }
+
+            /// Computes `1 << n` by performing a compile-time assertion that `n` is
+            /// in bounds.
+            ///
+            /// This version is the default and should be used if `n` is known at
+            /// compile time.
+            #[inline]
+            pub const fn [<bit_ $ty>](n: u32) -> $ty {
+                build_assert!(n < <$ty>::BITS);
+                (1 as $ty) << n
+            }
+        }
+    };
+}
+
+impl_bit_fn!(u64);
+impl_bit_fn!(u32);
+impl_bit_fn!(u16);
+impl_bit_fn!(u8);
+
+macro_rules! impl_genmask_fn {
+    (
+        $ty:ty,
+        $(#[$genmask_ex:meta])*
+    ) => {
+        paste! {
+            /// Creates a compile-time contiguous bitmask for the given range by
+            /// validating the range at runtime.
+            ///
+            /// Returns [`None`] if the range is invalid, i.e.: if the start is
+            /// greater than or equal to the end.
+            #[inline]
+            pub fn [<genmask_checked_ $ty>](range: RangeInclusive<u32>) -> Option<$ty> {
+                let start = *range.start();
+                let end = *range.end();
+
+                if start >= end || end >= <$ty>::BITS {
+                    return None;
+                }
+
+                let high = [<checked_bit_ $ty>](end)?;
+                let low = [<checked_bit_ $ty>](start)?;
+                Some((high | (high - 1)) & !(low - 1))
+            }
+
+            /// Creates a compile-time contiguous bitmask for the given range by
+            /// performing a compile-time assertion that the range is valid.
+            ///
+            /// This version is the default and should be used if the range is known
+            /// at compile time.
+            $(#[$genmask_ex])*
+            #[inline]
+            pub const fn [<genmask_ $ty>](range: RangeInclusive<u32>) -> $ty {
+                let start = *range.start();
+                let end = *range.end();
+
+                build_assert!(start < end);
+                build_assert!(end < <$ty>::BITS);
+
+                let high = [<bit_ $ty>](end);
+                let low = [<bit_ $ty>](start);
+                (high | (high - 1)) & !(low - 1)
+            }
+        }
+    };
+}
+
+impl_genmask_fn!(
+    u64,
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::bits::genmask_u64;
+    /// let mask = genmask_u64(21..=39);
+    /// assert_eq!(mask, 0x000000ffffe00000);
+    /// ```
+);
+
+impl_genmask_fn!(
+    u32,
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::bits::genmask_u32;
+    /// let mask = genmask_u32(0..=9);
+    /// assert_eq!(mask, 0x000003ff);
+    /// ```
+);
+
+impl_genmask_fn!(
+    u16,
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::bits::genmask_u16;
+    /// let mask = genmask_u16(0..=9);
+    /// assert_eq!(mask, 0x000003ff);
+    /// ```
+);
+
+impl_genmask_fn!(
+    u8,
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::bits::genmask_u8;
+    /// let mask = genmask_u8(0..=7);
+    /// assert_eq!(mask, 0x000000ff);
+    /// ```
+);
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 6b4774b2b1c37f4da1866e993be6230bc6715841..1bb294de8cb000120a0d04f61d6f7262fbc9f600 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -54,6 +54,7 @@
 pub mod alloc;
 #[cfg(CONFIG_AUXILIARY_BUS)]
 pub mod auxiliary;
+pub mod bits;
 #[cfg(CONFIG_BLOCK)]
 pub mod block;
 #[doc(hidden)]

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

Best regards,
-- 
Daniel Almeida <daniel.almeida@collabora.com>
Re: [PATCH v7] rust: kernel: add support for bits/genmask macros
Posted by Alexandre Courbot 3 months, 1 week ago
Hi Daniel,

On Tue Jun 24, 2025 at 5:17 AM JST, Daniel Almeida wrote:
> In light of bindgen being unable to generate bindings for macros, and
> owing to the widespread use of these macros in drivers, manually define
> the bit and genmask C macros in Rust.
>
> The *_checked version of the functions provide runtime checking while
> the const version performs compile-time assertions on the arguments via
> the build_assert!() macro.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>

Overall this looks great ; I especially like how succint this has
become. I think I found a couple of last remaining issues, please check
below.

<snip>
> diff --git a/rust/kernel/bits.rs b/rust/kernel/bits.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..8db122b5db9565b76c14fc33e33058f9dac7bd75
> --- /dev/null
> +++ b/rust/kernel/bits.rs
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Bit manipulation macros.
> +//!
> +//! C header: [`include/linux/bits.h`](srctree/include/linux/bits.h)
> +
> +use crate::prelude::*;
> +use core::ops::RangeInclusive;
> +use macros::paste;
> +
> +macro_rules! impl_bit_fn {
> +    (
> +        $ty:ty
> +    ) => {
> +        paste! {
> +            /// Computes `1 << n` if `n` is in bounds, i.e.: if `n` is smaller than
> +            /// the maximum number of bits supported by the type.
> +            ///
> +            /// Returns [`None`] otherwise.
> +            #[inline]
> +            pub fn [<checked_bit_ $ty>](n: u32) -> Option<$ty> {
> +                (1 as $ty).checked_shl(n)
> +            }
> +
> +            /// Computes `1 << n` by performing a compile-time assertion that `n` is
> +            /// in bounds.
> +            ///
> +            /// This version is the default and should be used if `n` is known at
> +            /// compile time.
> +            #[inline]
> +            pub const fn [<bit_ $ty>](n: u32) -> $ty {
> +                build_assert!(n < <$ty>::BITS);
> +                (1 as $ty) << n
> +            }
> +        }
> +    };
> +}
> +
> +impl_bit_fn!(u64);
> +impl_bit_fn!(u32);
> +impl_bit_fn!(u16);
> +impl_bit_fn!(u8);
> +
> +macro_rules! impl_genmask_fn {
> +    (
> +        $ty:ty,
> +        $(#[$genmask_ex:meta])*
> +    ) => {
> +        paste! {
> +            /// Creates a compile-time contiguous bitmask for the given range by

This is the checked version, so IIUC the bitmask is not created at
compile-time?

> +            /// validating the range at runtime.
> +            ///
> +            /// Returns [`None`] if the range is invalid, i.e.: if the start is
> +            /// greater than or equal to the end.

"... or is out of the representable range for $ty."

> +            #[inline]

Can we give examples for this function as well?

> +            pub fn [<genmask_checked_ $ty>](range: RangeInclusive<u32>) -> Option<$ty> {
> +                let start = *range.start();
> +                let end = *range.end();
> +
> +                if start >= end || end >= <$ty>::BITS {

The range is inclusive, so something like `0..=0` is valid (and returns
a mask of `0x1`). I think we want `start > end`?

Also, since you are using `checked_bit_*` below, I think the `end >=
<$ty>::BITS` check is redundant.

> +                    return None;
> +                }
> +
> +                let high = [<checked_bit_ $ty>](end)?;
> +                let low = [<checked_bit_ $ty>](start)?;
> +                Some((high | (high - 1)) & !(low - 1))
> +            }
> +
> +            /// Creates a compile-time contiguous bitmask for the given range by
> +            /// performing a compile-time assertion that the range is valid.
> +            ///
> +            /// This version is the default and should be used if the range is known
> +            /// at compile time.
> +            $(#[$genmask_ex])*
> +            #[inline]
> +            pub const fn [<genmask_ $ty>](range: RangeInclusive<u32>) -> $ty {
> +                let start = *range.start();
> +                let end = *range.end();
> +
> +                build_assert!(start < end);

`start <= end`

> +                build_assert!(end < <$ty>::BITS);

Here also this check looks redundant, since `bit_*` will fail if the
shift doesn't fit into $ty.

> +
> +                let high = [<bit_ $ty>](end);
> +                let low = [<bit_ $ty>](start);
> +                (high | (high - 1)) & !(low - 1)
> +            }
> +        }
> +    };
> +}
> +
> +impl_genmask_fn!(
> +    u64,
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::bits::genmask_u64;
> +    /// let mask = genmask_u64(21..=39);
> +    /// assert_eq!(mask, 0x000000ffffe00000);

I think we should also have 2 examples that show the behavior at the
limits (i.e. `0..=0` and `0..=63` here ; possibly others if you can see
interesting cases.). They are useful to understand how the function
actually works, and would also have caught the errors I pointed out
above.
Re: [PATCH v7] rust: kernel: add support for bits/genmask macros
Posted by Alexandre Courbot 3 months, 1 week ago
On Wed Jul 2, 2025 at 10:25 PM JST, Alexandre Courbot wrote:
>> +impl_genmask_fn!(
>> +    u64,
>> +    /// # Examples
>> +    ///
>> +    /// ```
>> +    /// # use kernel::bits::genmask_u64;
>> +    /// let mask = genmask_u64(21..=39);
>> +    /// assert_eq!(mask, 0x000000ffffe00000);
>
> I think we should also have 2 examples that show the behavior at the
> limits (i.e. `0..=0` and `0..=63` here ; possibly others if you can see
> interesting cases.). They are useful to understand how the function
> actually works, and would also have caught the errors I pointed out
> above.

Also one last nit: users will count the bits on these examples, so to
make that task easier, please write long hexadecimal values as
`0x0000_00ff_ffe0_0000`. :)
Re: [PATCH v7] rust: kernel: add support for bits/genmask macros
Posted by Alice Ryhl 3 months, 1 week ago
On Mon, Jun 23, 2025 at 10:18 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> In light of bindgen being unable to generate bindings for macros, and
> owing to the widespread use of these macros in drivers, manually define
> the bit and genmask C macros in Rust.
>
> The *_checked version of the functions provide runtime checking while
> the const version performs compile-time assertions on the arguments via
> the build_assert!() macro.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>

Is it intentional that the macros are not available for usize?

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Re: [PATCH v7] rust: kernel: add support for bits/genmask macros
Posted by Daniel Almeida 3 months, 1 week ago
Hi Alice,

> On 2 Jul 2025, at 07:27, Alice Ryhl <aliceryhl@google.com> wrote:
> 
> On Mon, Jun 23, 2025 at 10:18 PM Daniel Almeida
> <daniel.almeida@collabora.com> wrote:
>> 
>> In light of bindgen being unable to generate bindings for macros, and
>> owing to the widespread use of these macros in drivers, manually define
>> the bit and genmask C macros in Rust.
>> 
>> The *_checked version of the functions provide runtime checking while
>> the const version performs compile-time assertions on the arguments via
>> the build_assert!() macro.
>> 
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> 
> Is it intentional that the macros are not available for usize?
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>


Not intentional, but do we need it?

I really have no idea whether it’d be useful really.

— Daniel