public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Joel Fernandes <joelagnelf@nvidia.com>
To: Alexandre Courbot <acourbot@nvidia.com>
Cc: "Danilo Krummrich" <dakr@kernel.org>,
	"Yury Norov" <yury.norov@gmail.com>,
	"Jesung Yang" <y.j3ms.n@gmail.com>,
	"Miguel Ojeda" <ojeda@kernel.org>,
	"Alex Gaynor" <alex.gaynor@gmail.com>,
	"Boqun Feng" <boqun.feng@gmail.com>,
	"Gary Guo" <gary@garyguo.net>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	"Benno Lossin" <lossin@kernel.org>,
	"Andreas Hindborg" <a.hindborg@kernel.org>,
	"Alice Ryhl" <aliceryhl@google.com>,
	"Trevor Gross" <tmgross@umich.edu>,
	nouveau@lists.freedesktop.org, linux-kernel@vger.kernel.org,
	rust-for-linux@vger.kernel.org
Subject: Re: [PATCH RFC v2 2/3] rust: kernel: add bounded integer types
Date: Thu, 9 Oct 2025 17:33:53 -0400	[thread overview]
Message-ID: <20251009213353.GA2326866@joelbox2> (raw)
In-Reply-To: <20251009-bounded_ints-v2-2-ff3d7fee3ffd@nvidia.com>

Hi Alex,

Great effort, thanks. I replied with few comments below. Since the patch is
large, it would be great if could be possibly split. Maybe the From
primitives deserve a separate patch.

On Thu, Oct 09, 2025 at 09:37:09PM +0900, Alexandre Courbot wrote:
> Add the BoundedInt type, which restricts the number of bits allowed to
> be used in a given integer value. This is useful to carry guarantees
> when setting bitfields.
> 
> Alongside this type, many `From` and `TryFrom` implementations are
> provided to reduce friction when using with regular integer types. Proxy
> implementations of common integer traits are also provided.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  rust/kernel/lib.rs |   1 +
>  rust/kernel/num.rs | 499 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 500 insertions(+)
> 
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index fcffc3988a90..21c1f452ee6a 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -101,6 +101,7 @@
>  pub mod mm;
>  #[cfg(CONFIG_NET)]
>  pub mod net;
> +pub mod num;
>  pub mod of;
>  #[cfg(CONFIG_PM_OPP)]
>  pub mod opp;
> diff --git a/rust/kernel/num.rs b/rust/kernel/num.rs
> new file mode 100644
> index 000000000000..b2aad95ce51c
> --- /dev/null
> +++ b/rust/kernel/num.rs
> @@ -0,0 +1,499 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Numerical types for the kernel.
> +
> +use kernel::prelude::*;
> +
> +/// Integer type for which only the bits `0..NUM_BITS` are valid.
> +///
> +/// # Invariants
> +///
> +/// Stored values are represented with at most `NUM_BITS` bits.
> +#[repr(transparent)]
> +#[derive(Clone, Copy, Debug, Default, Hash)]
> +pub struct BoundedInt<T, const NUM_BITS: u32>(T);
> +
> +/// Returns `true` if `$value` can be represented with at most `$NUM_BITS` on `$type`.
> +macro_rules! is_in_bounds {
> +    ($value:expr, $type:ty, $num_bits:expr) => {{
> +        let v = $value;
> +        v & <$type as Boundable<NUM_BITS>>::MASK == v
> +    }};
> +}
> +
> +/// Trait for primitive integer types that can be used with `BoundedInt`.
> +pub trait Boundable<const NUM_BITS: u32>
> +where
> +    Self: Sized + Copy + core::ops::BitAnd<Output = Self> + core::cmp::PartialEq,
> +    Self: TryInto<u8> + TryInto<u16> + TryInto<u32> + TryInto<u64>,
> +{
> +    /// Mask of the valid bits for this type.
> +    const MASK: Self;
> +
> +    /// Returns `true` if `value` can be represented with at most `NUM_BITS`.
> +    ///
> +    /// TODO: post-RFC: replace this with a left-shift followed by right-shift operation. This will
> +    /// allow us to handle signed values as well.
> +    fn is_in_bounds(value: Self) -> bool {
> +        is_in_bounds!(value, Self, NUM_BITS)
> +    }
> +}
> +
> +impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u8 {
> +    const MASK: u8 = crate::bits::genmask_u8(0..=(NUM_BITS - 1));
> +}
> +
> +impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u16 {
> +    const MASK: u16 = crate::bits::genmask_u16(0..=(NUM_BITS - 1));
> +}
> +
> +impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u32 {
> +    const MASK: u32 = crate::bits::genmask_u32(0..=(NUM_BITS - 1));
> +}
> +
> +impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u64 {
> +    const MASK: u64 = crate::bits::genmask_u64(0..=(NUM_BITS - 1));
> +}
> +
> +impl<T, const NUM_BITS: u32> BoundedInt<T, NUM_BITS>
> +where
> +    T: Boundable<NUM_BITS>,
> +{
> +    /// Checks that `value` is valid for this type at compile-time and build a new value.
> +    ///
> +    /// This relies on [`build_assert!`] to perform validation at compile-time. If `value` cannot
> +    /// be inferred to be in bounds at compile-time, use the fallible [`Self::try_new`] instead.
> +    ///
> +    /// When possible, use one of the `new_const` methods instead of this method as it statically
> +    /// validates `value` instead of relying on the compiler's optimizations.

This sounds like, users might use the less-optimal API first with the same
build_assert issues we had with the IO accessors, since new() sounds very obvious.
How about the following naming?

new::<VALUE>()        // Primary constructor for constants using const generics.
try_new(value)        // Keep as-is for fallible runtime
new_from_expr(value)  // For compile-time validated runtime values

If new::<VALUE>() does not work for the user, the compiler will fail.

Or, new_from_expr() could be from_value(), Ok with either naming or a better name.

> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::num::BoundedInt;
> +    ///
> +    /// # fn some_number() -> u32 { 0xffffffff }
> +    ///
> +    /// assert_eq!(BoundedInt::<u8, 1>::new(1).get(), 1);
> +    /// assert_eq!(BoundedInt::<u16, 8>::new(0xff).get(), 0xff);
> +    ///
> +    /// // Triggers a build error as `0x1ff` doesn't fit into 8 bits.
> +    /// // assert_eq!(BoundedInt::<u32, 8>::new(0x1ff).get(), 0x1ff);
> +    ///
> +    /// let v: u32 = some_number();
> +    /// // Triggers a build error as `v` cannot be asserted to fit within 4 bits...
> +    /// // let _ = BoundedInt::<u32, 4>::new(v);
> +    /// // ... but this works as the compiler can assert the range from the mask.
> +    /// let _ = BoundedInt::<u32, 4>::new(v & 0xf);
> +    /// ```
> +    pub fn new(value: T) -> Self {
> +        crate::build_assert!(
> +            T::is_in_bounds(value),
> +            "Provided parameter is larger than maximal supported value"
> +        );
> +
> +        Self(value)
> +    }
> +
> +    /// Attempts to convert `value` into a value bounded by `NUM_BITS`.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::num::BoundedInt;
> +    ///
> +    /// assert_eq!(BoundedInt::<u8, 1>::try_new(1).map(|v| v.get()), Ok(1));
> +    /// assert_eq!(BoundedInt::<u16, 8>::try_new(0xff).map(|v| v.get()), Ok(0xff));
> +    ///
> +    /// // `0x1ff` doesn't fit into 8 bits.
> +    /// assert_eq!(BoundedInt::<u32, 8>::try_new(0x1ff), Err(EOVERFLOW));
> +    /// ```
> +    pub fn try_new(value: T) -> Result<Self> {
> +        if !T::is_in_bounds(value) {
> +            Err(EOVERFLOW)
> +        } else {
> +            Ok(Self(value))
> +        }
> +    }
> +
> +    /// Returns the contained value as a primitive type.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::num::BoundedInt;
> +    ///
> +    /// let v = BoundedInt::<u32, 4>::new_const::<7>();
> +    /// assert_eq!(v.get(), 7u32);
> +    /// ```
> +    pub fn get(self) -> T {
> +        if !T::is_in_bounds(self.0) {
> +            // SAFETY: Per the invariants, `self.0` cannot have bits set outside of `MASK`, so
> +            // this block will
> +            // never be reached.
> +            unsafe { core::hint::unreachable_unchecked() }
> +        }

Does this if block help the compiler generate better code? I wonder if code
gen could be checked to confirm the rationale.

> +        self.0
> +    }
> +
> +    /// Increase the number of bits usable for `self`.
> +    ///
> +    /// This operation cannot fail.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::num::BoundedInt;
> +    ///
> +    /// let v = BoundedInt::<u32, 4>::new_const::<7>();
> +    /// let larger_v = v.enlarge::<12>();
> +    /// // The contained values are equal even though `larger_v` has a bigger capacity.
> +    /// assert_eq!(larger_v, v);
> +    /// ```
> +    pub const fn enlarge<const NEW_NUM_BITS: u32>(self) -> BoundedInt<T, NEW_NUM_BITS>
> +    where
> +        T: Boundable<NEW_NUM_BITS>,
> +        T: Copy,

Boundable already implies copy so T: Copy is redundant.

> +    {
> +        build_assert!(NEW_NUM_BITS >= NUM_BITS);
> +
> +        // INVARIANT: the value did fit within `NUM_BITS`, so it will all the more fit within
> +        // `NEW_NUM_BITS` which is larger.
> +        BoundedInt(self.0)
> +    }
> +
> +    /// Shrink the number of bits usable for `self`.
> +    ///
> +    /// Returns `EOVERFLOW` if the value of `self` cannot be represented within `NEW_NUM_BITS`.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::num::BoundedInt;
> +    ///
> +    /// let v = BoundedInt::<u32, 12>::new_const::<7>();
> +    /// let smaller_v = v.shrink::<4>()?;
> +    /// // The contained values are equal even though `smaller_v` has a smaller capacity.
> +    /// assert_eq!(smaller_v, v);
> +    ///
> +    /// # Ok::<(), Error>(())
> +    /// ```
> +    pub fn shrink<const NEW_NUM_BITS: u32>(self) -> Result<BoundedInt<T, NEW_NUM_BITS>>
> +    where
> +        T: Boundable<NEW_NUM_BITS>,
> +        T: Copy,

Here too.

[...]
> +impl_const_new!(u8 u16 u32 u64);
> +
> +/// Declares a new `$trait` and implements it for all bounded types represented using `$num_bits`.
> +///
> +/// This is used to declare properties as traits that we can use for later implementations.
> +macro_rules! impl_size_rule {
> +    ($trait:ident, $($num_bits:literal)*) => {
> +        trait $trait {}
> +
> +        $(
> +        impl<T> $trait for BoundedInt<T, $num_bits> where T: Boundable<$num_bits> {}
> +        )*
> +    };
> +}
> +
> +// Bounds that are larger than a `u64`.
> +impl_size_rule!(LargerThanU64, 64);
> +
> +// Bounds that are larger than a `u32`.
> +impl_size_rule!(LargerThanU32,
> +    32 33 34 35 36 37 38 39

If num_bits == 32 (number of bits), how could BoundedInt<T, 32> a
LargerThanU32? It should be AtleastU32 or something.

Or the above list should start from 33. Only a >= 33-bit wide integer can be
LargerThanU32.

Thanks.


  reply	other threads:[~2025-10-09 21:33 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-10-09 12:37 [RFC PATCH v2 0/3] rust: bounded integer types and use in register macro Alexandre Courbot
2025-10-09 12:37 ` [PATCH RFC v2 1/3] gpu: nova-core: register: use field type for Into implementation Alexandre Courbot
2025-10-09 15:17   ` Joel Fernandes
2025-10-09 15:41     ` Joel Fernandes
2025-10-10  8:24       ` Alexandre Courbot
2025-10-10  8:26         ` Alexandre Courbot
2025-10-10 14:59           ` Joel Fernandes
2025-10-09 20:10   ` Edwin Peer
2025-10-16 14:51   ` Joel Fernandes
2025-10-09 12:37 ` [PATCH RFC v2 2/3] rust: kernel: add bounded integer types Alexandre Courbot
2025-10-09 21:33   ` Joel Fernandes [this message]
2025-10-10  8:49     ` Alexandre Courbot
2025-10-10 15:23       ` Joel Fernandes
2025-10-11 10:33   ` Alice Ryhl
2025-10-09 12:37 ` [PATCH RFC v2 3/3] gpu: nova-core: use BoundedInt Alexandre Courbot
2025-10-09 16:40   ` Yury Norov
2025-10-09 17:18     ` Danilo Krummrich
2025-10-09 18:28       ` Yury Norov
2025-10-09 18:37         ` Joel Fernandes
2025-10-09 19:19         ` Miguel Ojeda
2025-10-09 19:34         ` Danilo Krummrich
2025-10-09 18:29     ` Joel Fernandes
2025-10-10  9:19     ` Alexandre Courbot
2025-10-10 17:20       ` Yury Norov
2025-10-10 17:58         ` Danilo Krummrich
2025-10-13 13:58         ` Joel Fernandes

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20251009213353.GA2326866@joelbox2 \
    --to=joelagnelf@nvidia.com \
    --cc=a.hindborg@kernel.org \
    --cc=acourbot@nvidia.com \
    --cc=alex.gaynor@gmail.com \
    --cc=aliceryhl@google.com \
    --cc=bjorn3_gh@protonmail.com \
    --cc=boqun.feng@gmail.com \
    --cc=dakr@kernel.org \
    --cc=gary@garyguo.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lossin@kernel.org \
    --cc=nouveau@lists.freedesktop.org \
    --cc=ojeda@kernel.org \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=tmgross@umich.edu \
    --cc=y.j3ms.n@gmail.com \
    --cc=yury.norov@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox