Message-Id: <DDZ4S7O5ML1K.H8QOKW85N3L3@nvidia.com>
Date: Mon, 03 Nov 2025 23:31:43 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Alice Ryhl" <aliceryhl@...gle.com>, "Yury Norov" <yury.norov@...il.com>
Cc: "Alexandre Courbot" <acourbot@...dia.com>, "Danilo Krummrich"
 <dakr@...nel.org>, "Miguel Ojeda" <ojeda@...nel.org>, "Joel Fernandes"
 <joelagnelf@...dia.com>, "Jesung Yang" <y.j3ms.n@...il.com>, "Boqun Feng"
 <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <lossin@...nel.org>, "Andreas Hindborg" <a.hindborg@...nel.org>, "Trevor
 Gross" <tmgross@...ch.edu>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH 1/2] rust: add BitInt integer wrapping type

Hi Alice,

On Mon Nov 3, 2025 at 7:47 PM JST, Alice Ryhl wrote:
> On Sun, Nov 02, 2025 at 09:17:35PM -0500, Yury Norov wrote:
>> On Fri, Oct 31, 2025 at 10:39:57PM +0900, Alexandre Courbot wrote:
>> > Add the `BitInt` type, which is an integer on which the number of bits
>> > allowed to be used is restricted, capping its maximal value below that
>> > of primitive type is wraps.
>> > 
>> > This is useful to e.g. enforce guarantees when working with bit fields.
>> > 
>> > Alongside this type, provide many `From` and `TryFrom` implementations
>> > are to reduce friction when using with regular integer types. Proxy
>> > implementations of common integer traits are also provided.
>> > 
>> > Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
>> > ---
>> >  rust/kernel/lib.rs        |   1 +
>> >  rust/kernel/num.rs        |  75 ++++
>> >  rust/kernel/num/bitint.rs | 896 ++++++++++++++++++++++++++++++++++++++++++++++
>> >  3 files changed, 972 insertions(+)
>>  
>> ...
>> 
>> > +/// Evaluates to `true` if `$value` can be represented using at most `$num_bits` on `$type`.
>> > +///
>> > +/// Can be used in const context.
>> > +macro_rules! fits_within {
>> > +    ($value:expr, $type:ty, $num_bits:expr) => {{
>> > +        // Any attempt to create a `BitInt` with more bits used for representation than its backing
>> > +        // type (i.e. create an invalid `BitInt`) must be aborted at build time.
>> > +        build_assert!(
>> > +            <$type>::BITS >= $num_bits,
>> > +            "Number of bits requested for representation is larger than backing type."
>> > +        );
>> > +
>> > +        let shift: u32 = <$type>::BITS - $num_bits;
>> > +        let v = $value;
>> > +
>> > +        // The value fits within `NUM_BITS` if shifting it left by the number of unused bits, then
>> > +        // right by the same number, doesn't change the value.
>> > +        //
>> > +        // This method has the benefit of working with both unsigned and signed integers.
>> > +        (v << shift) >> shift == v
>> 
>> In C it doesn't work:
>> 
>>         int c = 0x7fffffff;
>>         printf("%x\t%x\n", c, (c << 1) >> 1); // 7fffffff	ffffffff
>> 
>> Neither in rust:   
>> 
>>         let c: i32 = 0x7fffffff;
>>         println!("{} {}", c, (c << 1) >> 1);  // 2147483647 -1
>> 
>> Or I misunderstand something?
>
> I think we should probably just define a BitInt::MIN and BitInt::MAX
> constant and have this code check MIN <= v && v <= MAX.

Thankfully (and unless I am mistaken) it looks like this is actually
working as intended - please see my reply to Yury if you want to
confirm.

>
>> > +    }};
>> > +}
>> > +
>> > +/// Trait for primitive integer types that can be used to back a [`BitInt`].
>> > +///
>> > +/// This is mostly used to lock all the operations we need for [`BitInt`] in a single trait.
>> > +pub trait Boundable
>> > +where
>> > +    Self: Integer
>> > +        + Sized
>> > +        + Copy
>> > +        + core::ops::Shl<u32, Output = Self>
>> > +        + core::ops::Shr<u32, Output = Self>
>> > +        + core::cmp::PartialEq,
>> > +    Self: TryInto<u8> + TryInto<u16> + TryInto<u32> + TryInto<u64>,
>> > +    Self: TryInto<i8> + TryInto<i16> + TryInto<i32> + TryInto<i64>,
>> > +{
>> > +    /// Returns `true` if `value` can be represented with at most `NUM_BITS` on `T`.
>> > +    fn fits_within(value: Self, num_bits: u32) -> bool {
>> > +        fits_within!(value, Self, num_bits)
>> > +    }
>> > +}
>> > +
>> > +/// Inplement `Boundable` for all integers types.
>> > +impl<T> Boundable for T
>> > +where
>> > +    T: Integer
>> > +        + Sized
>> > +        + Copy
>> > +        + core::ops::Shl<u32, Output = Self>
>> > +        + core::ops::Shr<u32, Output = Self>
>> > +        + core::cmp::PartialEq,
>> > +    Self: TryInto<u8> + TryInto<u16> + TryInto<u32> + TryInto<u64>,
>> > +    Self: TryInto<i8> + TryInto<i16> + TryInto<i32> + TryInto<i64>,
>> > +{
>> > +}
>> > +
>> > +/// Integer type for which only the bits `0..NUM_BITS` are valid.
>> > +///
>> > +/// # Invariants
>> > +///
>> > +/// Stored values are represented with at most `NUM_BITS` bits.
>> > +///
>> > +/// # Examples
>> > +///
>> > +/// ```
>> > +/// use kernel::num::BitInt;
>> > +///
>> > +/// // An unsigned 8-bit integer, of which only the 4 LSBs can ever be set.
>> > +/// // The value `15` is statically validated to fit within the specified number of bits.
>> > +/// let v = BitInt::<u8, 4>::new::<15>();
>> 
>> What for do you make user to declare the storage explicitly? From
>> end-user perspective, declaring 4-bit variable must imply the most
>> suitable storage... C version of the same doesn't allow user to select
>> the storage as well:
>> 
>>         _BitInt(4) x = 8;
>> 
>> I can't think out any useful usecase for this... I believe that the
>> optimal storage must be chosen by implementation. And it may even be
>> different for different arches.
>
> It's more complex to not specify the backing storage explicitly, but I
> also think it would be nice to be able to avoid it.

Indeed, it's not impossible to automatically assign a primitive type
from the number of requested bits, but it's a bit of a PITA without
generic constant expressions. And I don't think we can avoid explicitly
mentioning the signedness anyway, so why not give the full type.

Finally, this would also make arithmetic operations (or anything using
the `BitInt`'s value) more complex as one would need to cast to the type
they actually need to perform anything useful.

<snip>
>> > +/// // Non-constant expressions can also be validated at build-time thanks to compiler
>> > +/// // optimizations. This should be used as a last resort though.
>> > +/// let v = BitInt::<u8, 4>::from_expr(15);
>> 
>> Not sure I understand that. Can you confirm my understanding?
>> 
>> 1. For compile-time initialization I use BitInt::<i8, 4>::new::<8>();
>> 2. For compile- or runtime initialization: BitInt::<i8, 4>::from_expr(val);
>> 3. For runtime-only initialization: BitInt::<i8, 4>::try_new(val);
>> 
>> In this scheme #3 looks excessive...
>
> I'm not sure we should have `from_expr` at all. What it does is emit an
> if condition checking whether the value is in-bounds containing a call
> to a function that does not exist, so that if the optimizer prove the
> range check at compile-time, then there is a linker error.
>
> What is the use-case for from_expr?

It is pretty fundamental to using this successfully with bitfields. For
instance, the getter method for a given field (which returns a `BitInt`)
works roughly as follows:

    // Extract the field using its mask and shift.
    let field = ((self.0 & MASK) >> SHIFT);

    // Infallibly create the `BitInt` from `field` - the compiler can
    // infer from the mask and shift above that the overflow check
    // will never fail.
    BitInt::<$storage, { $hi + 1 - $lo }>::from_expr(field)

Without this, we should need to resort to unsafe code and probably an
`unwrap_unchecked`.

Another example from the Nova code:

    const FLUSH_SYSMEM_ADDR_SHIFT_HI: u32 = 40;

    pub(super) fn write_sysmem_flush_page_ga100(bar: &Bar0, addr: u64) {
        regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR_HI::default()
            .set_adr_63_40(BitInt::from_expr(addr >> FLUSH_SYSMEM_ADDR_SHIFT_HI).cast())
            .write(bar);
    }

The `adr_63_40` field is 24-bits long, and from the 40-bit shift
operation `from_expr` can infer that the result will fit and do without
a runtime check. I think that's pretty cool! :)

Also, this does not look very different from what e.g.
`Io::io_addr_assert` does.

Powered by blists - more mailing lists