Message-Id: <DAMAPVAI3V8X.N8SAQD6KOO1Q@nvidia.com>
Date: Sat, 14 Jun 2025 22:38:11 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Daniel Almeida" <daniel.almeida@...labora.com>, "Miguel Ojeda"
 <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>, "Boqun Feng"
 <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <benno.lossin@...ton.me>, "Andreas Hindborg" <a.hindborg@...nel.org>,
 "Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
 "Danilo Krummrich" <dakr@...nel.org>
Cc: <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

Hi Daniel,

On Tue Jun 10, 2025 at 11:14 PM 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.

I think this is the right approach, I wish we could make the functions
generic but that doesn't appear to be currently possible (and it
wouldn't make their invocation shorter anyway).

I agree with Miguel's suggestion to use paste to shorten the macro
syntax, it would make it much easier to understand them (and most of the
naming could be harmonized in the macro itself instead of relying on the
invocations to use the same names).

>
> Signed-off-by: Daniel Almeida <daniel.almeida@...labora.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 | 168 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs  |   1 +
>  2 files changed, 169 insertions(+)
>
> diff --git a/rust/kernel/bits.rs b/rust/kernel/bits.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..98065c8f7c94cfc3b076e041de190e942e1b4a9f
> --- /dev/null
> +++ b/rust/kernel/bits.rs
> @@ -0,0 +1,168 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Bit manipulation macros.
> +//!
> +//! C header: [`include/linux/bits.h`](srctree/include/linux/bits.h)
> +
> +use crate::build_assert;
> +use core::ops::Range;
> +
> +macro_rules! impl_bit_fn {
> +    (
> +        $checked_name:ident, $unbounded_name:ident, $const_name:ident, $ty:ty
> +    ) => {
> +        /// 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_name(n: u32) -> Option<$ty> {
> +            (1 as $ty) .checked_shl(n)
> +        }
> +
> +        /// 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 `0` otherwise.
> +        ///
> +        /// This is a convenience, as [`Option::unwrap_or`] cannot be used in
> +        /// const-context.
> +        #[inline]
> +        pub fn $unbounded_name(n: u32) -> $ty {
> +            match $checked_name(n) {
> +                Some(v) => v,
> +                None => 0,
> +            }

This could more succintly be `$checked_name(n).unwrap_or(0)` (same
remark for `$genmask_unbounded` below).

> +        }
> +
> +        /// 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 $const_name(n: u32) -> $ty {
> +            build_assert!(n < <$ty>::BITS);
> +            1 as $ty << n
> +        }
> +    };
> +}
> +
> +impl_bit_fn!(checked_bit_u64, unbounded_bit_u64, bit_u64, u64);
> +impl_bit_fn!(checked_bit_u32, unbounded_bit_u32, bit_u32, u32);
> +impl_bit_fn!(checked_bit_u16, unbounded_bit_u16, bit_u16, u16);
> +impl_bit_fn!(checked_bit_u8, unbounded_bit_u8, bit_u8, u8);
> +
> +macro_rules! impl_genmask_fn {
> +    (
> +        $ty:ty, $checked_bit:ident, $bit:ident, $genmask:ident, $genmask_checked:ident, $genmask_unbounded:ident,
> +        $(#[$genmask_ex:meta])*
> +    ) => {
> +        /// 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(range: Range<u32>) -> Option<$ty> {
> +            if range.start >= range.end || range.end > <$ty>::BITS {
> +                return None;
> +            }

>From this check I assumed that you interpret `range` as non-inclusive,
since `range.end == 32` is valid on u32...

> +            let high = $checked_bit(range.end)?;

... however IIUC `checked_bit` will return `None` here in such a case.
Should the argument be `range.end - 1`?

Your examples do seem to interpret the range as inclusive though, so
probably the check should be `|| range.end >= <$ty>::BITS`. But that
triggers the question, is it ok to use `Range` that way, when its
documentation specifically states that it is bounded exclusively above?
We could use `RangeInclusive` to match the semantics, which would
require us to write the ranges as `0..=7`. At least it is clear that the
upper bound is inclusive.

... or we make the methods generic against `RangeBounds` and allow both
`Range` and `RangeInclusive` to be used. But I'm concerned that callers
might use `0..1` thinking it is inclusive while it is not.

Thoughts?

> +            let low = $checked_bit(range.start)?;
> +            Some((high | (high - 1)) & !(low - 1))
> +        }
> +
> +        /// Creates a compile-time contiguous bitmask for the given range by
> +        /// validating the range at runtime.
> +        ///
> +        /// Returns `0` if the range is invalid, i.e.: if the start is greater
> +        /// than or equal to the end.
> +        #[inline]
> +        pub fn $genmask_unbounded(range: Range<u32>) -> $ty {
> +            match $genmask_checked(range) {
> +                Some(v) => v,
> +                None => 0,
> +            }

This could more succintly be `$genmask_checked(range).unwrap_or(0)`.

> +        }
> +
> +        /// 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(range: Range<u32>) -> $ty {
> +            build_assert!(range.start < range.end);
> +            build_assert!(range.end <= <$ty>::BITS);

I guess this check also needs to be fixed.

Powered by blists - more mailing lists