Re: [PATCH RFC v2 2/3] rust: kernel: add bounded integer types

[Joel Fernandes <joelagnelf@nvidia.com>]

On 10/10/2025 4:49 AM, Alexandre Courbot wrote:
> On Fri Oct 10, 2025 at 6:33 AM JST, Joel Fernandes wrote:
>> 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.
> 
> I'm all for smaller patches when it makes reviewing easier, but in this
> case all it would achieve is making the second patch append code right
> after the next. :) Is there a benefit in doing so?

I think there is a benefit, because reviewers don't have to scroll through huge
emails :). Plus separate commit messages would be added in too, to reason about
new code. There's other possible logical splits too, was just giving example but
I am Ok with it if you still want to keep it singular. :)

>>> +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.
> 
> Agreed, the preferred method should appear first. IIRC Alice also made a
> similar suggestion about v1 during the DRM sync, sorry for not picking
> it up.

Cool, sounds good.

[..]>>> +    /// 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.
> 
> Benno suggested that it would on a different patch:
> 
> https://lore.kernel.org/rust-for-linux/DBL1ZGZCSJF3.29HNS9BSN89C6@kernel.org/
> 
> OTOH as shown in patch 3/3, this doesn't exempt us from handling
> impossible values when using this in a match expression...

Interesting, TIL.

>>> +        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.
> 
> Thanks. I need to do a thorough review of all the contraints as I've
> changed them quite a bit as the implementation matured.

Cool. :)

>> [...]
>>> +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.
> 
> The name is a bit ambiguous indeed. An accurate one would be
> `LargerOrEqualThanU32`, but `AtLeastU32` should also work.
Sure, I prefer AtLeastU32 since it is shorter :)

thanks,

 - Joel