[PATCH v2 0/4] rust: add `Alignment` type

[PATCH v2 0/4] rust: add `Alignment` type

[Alexandre Courbot]

After some discussions on the list [1] and the Rust libs team [2], this
patchset undergoes two major changes:

- The `PowerOfTwo` type is replaced by `Alignment`, which is heavily
  inspired by the nightly type of the same name in the standard library
  [3].
- The `last_set_bit` function is dropped, with the recommendation to use
  the standard library's `checked_ilog2` which does essentially the same
  thing.

The upstream `Alignment` is more constrained than the `PowerOfTwo` of
the last revision: it uses `usize` internally instead of a generic
value, and does not provide `align_down` or `align_up` methods.

These two shortcomings come together very nicely to gift us with a nice
headache: we need to align values potentially larger than `usize`, thus
need to make `align_down` and `align_up` generic. The generic parameter
needs to be constrained on the operations used to perform the alignment
(e.g. `BitAnd`, `Not`, etc) and there is one essential operation for
which no trait exists in the standard library: `checked_add`. Thus the
first patch of this series introduces a trait for it in the `num` module
and implements it for all integer types. I suspect we will need
something alongside these lines for other purposes anyway, and probably
other traits too.

This generic nature also restricts these methods to being non-const,
unfortunately. I have tried to implement them as macros instead, but
quickly hit a wall due to the inability to convert `Alignment`'s `usize`
into the type of the value to align.

So here it is, not perfect but the need for a standard way to align is
starting to become more pressing.

[1] https://lore.kernel.org/rust-for-linux/DBTGVEJQOUDM.OTGZ6PXLB9JV@nvidia.com/T/#m09e068ecadf5b41099d4c6c55e13fbb3a98c5839
[2] https://github.com/rust-lang/libs-team/issues/631
[3] https://doc.rust-lang.org/std/ptr/struct.Alignment.html

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
Changes in v2:
- Fix indentation of paste! in impl_last_set_bit.
- Link to v1: https://lore.kernel.org/r/20250620-num-v1-0-7ec3d3fb06c9@nvidia.com

Changes since split from the nova-core series:
- Rename `fls` to `last_set_bit`,
- Generate per-type doctests,
- Add invariants section to `PowerOfTwo`.
- Do not use reference to `self` in `PowerOfTwo` methods since it
  implements `Copy`,
  - Use #[derive] where possible instead of implementing traits
    manually,
    - Remove `Deref` and `Borrow` implementations.

---
Alexandre Courbot (4):
      rust: add `CheckedAdd` trait
      rust: add `Alignment` type
      gpu: nova-core: use Alignment for alignment-related operations
      gpu: nova-core: use `checked_ilog2` to emulate `fls`

 Documentation/gpu/nova/core/todo.rst      |  15 ---
 drivers/gpu/nova-core/falcon/hal/ga102.rs |   4 +-
 drivers/gpu/nova-core/fb.rs               |   6 +-
 drivers/gpu/nova-core/firmware/fwsec.rs   |  11 +-
 drivers/gpu/nova-core/vbios.rs            |   4 +-
 rust/kernel/lib.rs                        |   2 +
 rust/kernel/num.rs                        |  28 ++++
 rust/kernel/ptr.rs                        | 213 ++++++++++++++++++++++++++++++
 8 files changed, 255 insertions(+), 28 deletions(-)
---
base-commit: 14ae91a81ec8fa0bc23170d4aa16dd2a20d54105
change-id: 20250620-num-9420281c02c7

Best regards,
-- 
Alexandre Courbot <acourbot@nvidia.com>

[PATCH v2 1/4] rust: add `CheckedAdd` trait

[Alexandre Courbot]

Rust provides traits for standard arithmetic and logic operations, but
in the context of the kernel we often need to consider overflows. The
checked Rust arithmetic methods are unfortunately not behind a trait,
which makes them unavailable to generic code.

As a start, add the `CheckedAdd` trait providing the `checked_add`
operation and implement it for all integer types. Its name and location
are inspired by the user-space `num` crate.

This trait is to be first used by the `Alignment` type.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 rust/kernel/lib.rs |  1 +
 rust/kernel/num.rs | 28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 6b4774b2b1c37f4da1866e993be6230bc6715841..2955f65da1278dd4cba1e4272ff178b8211a892c 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -89,6 +89,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 0000000000000000000000000000000000000000..c81bb046078b70c321dd52aa9c2b5518be49d249
--- /dev/null
+++ b/rust/kernel/num.rs
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Numerical and binary utilities for primitive types.
+
+use core::ops::Add;
+
+/// Trait for performing a checked addition that returns `None` if the operation would overflow.
+///
+/// This trait exists in order to represent scalar types already having a `checked_add` method in
+/// generic code.
+pub trait CheckedAdd: Sized + Add<Self, Output = Self> {
+    /// Computes `self + rhs`, returning `None` if an overflow would occur.
+    fn checked_add(self, rhs: Self) -> Option<Self>;
+}
+
+macro_rules! impl_checked_add {
+    ($($t:ty),*) => {
+        $(
+        impl CheckedAdd for $t {
+            fn checked_add(self, rhs: Self) -> Option<Self> {
+                self.checked_add(rhs)
+            }
+        }
+        )*
+    };
+}
+
+impl_checked_add!(u8, u16, u32, u64, usize, i8, i16, i32, i64, isize);

-- 
2.50.1

Re: [PATCH v2 1/4] rust: add `CheckedAdd` trait

[Daniel Almeida]

Hi Alex,

> On 4 Aug 2025, at 08:45, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 
> Rust provides traits for standard arithmetic and logic operations, but
> in the context of the kernel we often need to consider overflows. The
> checked Rust arithmetic methods are unfortunately not behind a trait,
> which makes them unavailable to generic code.
> 
> As a start, add the `CheckedAdd` trait providing the `checked_add`
> operation and implement it for all integer types. Its name and location
> are inspired by the user-space `num` crate.
> 
> This trait is to be first used by the `Alignment` type.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> rust/kernel/lib.rs |  1 +
> rust/kernel/num.rs | 28 ++++++++++++++++++++++++++++
> 2 files changed, 29 insertions(+)
> 
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 6b4774b2b1c37f4da1866e993be6230bc6715841..2955f65da1278dd4cba1e4272ff178b8211a892c 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -89,6 +89,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 0000000000000000000000000000000000000000..c81bb046078b70c321dd52aa9c2b5518be49d249
> --- /dev/null
> +++ b/rust/kernel/num.rs
> @@ -0,0 +1,28 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Numerical and binary utilities for primitive types.
> +
> +use core::ops::Add;
> +
> +/// Trait for performing a checked addition that returns `None` if the operation would overflow.

nit: this can be [`None`] instead, which will let users click on it in the docs.

This is of course pretty frivolous.

> +///
> +/// This trait exists in order to represent scalar types already having a `checked_add` method in
> +/// generic code.

Maybe “scalar types that already have a `checked_add` method?

But overall I feel like the whole sentence is a bit hard to parse, JFYI.

> +pub trait CheckedAdd: Sized + Add<Self, Output = Self> {
> +    /// Computes `self + rhs`, returning `None` if an overflow would occur.
> +    fn checked_add(self, rhs: Self) -> Option<Self>;
> +}
> +
> +macro_rules! impl_checked_add {
> +    ($($t:ty),*) => {
> +        $(
> +        impl CheckedAdd for $t {
> +            fn checked_add(self, rhs: Self) -> Option<Self> {
> +                self.checked_add(rhs)
> +            }
> +        }
> +        )*
> +    };
> +}
> +
> +impl_checked_add!(u8, u16, u32, u64, usize, i8, i16, i32, i64, isize);
> 
> -- 
> 2.50.1
> 
> 


Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH v2 1/4] rust: add `CheckedAdd` trait

[Alexandre Courbot]

On Mon Aug 4, 2025 at 11:37 PM JST, Daniel Almeida wrote:
> Hi Alex,
>
>> On 4 Aug 2025, at 08:45, Alexandre Courbot <acourbot@nvidia.com> wrote:
>> 
>> Rust provides traits for standard arithmetic and logic operations, but
>> in the context of the kernel we often need to consider overflows. The
>> checked Rust arithmetic methods are unfortunately not behind a trait,
>> which makes them unavailable to generic code.
>> 
>> As a start, add the `CheckedAdd` trait providing the `checked_add`
>> operation and implement it for all integer types. Its name and location
>> are inspired by the user-space `num` crate.
>> 
>> This trait is to be first used by the `Alignment` type.
>> 
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>> rust/kernel/lib.rs |  1 +
>> rust/kernel/num.rs | 28 ++++++++++++++++++++++++++++
>> 2 files changed, 29 insertions(+)
>> 
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index 6b4774b2b1c37f4da1866e993be6230bc6715841..2955f65da1278dd4cba1e4272ff178b8211a892c 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -89,6 +89,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 0000000000000000000000000000000000000000..c81bb046078b70c321dd52aa9c2b5518be49d249
>> --- /dev/null
>> +++ b/rust/kernel/num.rs
>> @@ -0,0 +1,28 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Numerical and binary utilities for primitive types.
>> +
>> +use core::ops::Add;
>> +
>> +/// Trait for performing a checked addition that returns `None` if the operation would overflow.
>
> nit: this can be [`None`] instead, which will let users click on it in the docs.
>
> This is of course pretty frivolous.

... but correct. Thanks.

>
>> +///
>> +/// This trait exists in order to represent scalar types already having a `checked_add` method in
>> +/// generic code.
>
> Maybe “scalar types that already have a `checked_add` method?
>
> But overall I feel like the whole sentence is a bit hard to parse, JFYI.

Let me rephrase this as "This trait exists to model scalar types with a
`checked_add` method in generic code." (provided this trait survives the
next revision).

[PATCH v2 2/4] rust: add `Alignment` type

[Alexandre Courbot]

Alignment operations are very common in the kernel. Since they are
always performed using a power of two value, enforcing this invariant
through a dedicated type leads to less bugs and can lead to improved
generated code.

Introduce the `Alignment` type, inspired by the nightly Rust feature of
the same name. It provides the same interface as its upstream namesake,
while extending it with `align_up` and `align_down` operations that are
usable on any integer type.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 rust/kernel/lib.rs |   1 +
 rust/kernel/ptr.rs | 213 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 214 insertions(+)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 2955f65da1278dd4cba1e4272ff178b8211a892c..0e66b55cde66ee1b274862cd78ad465a572dc5d9 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -100,6 +100,7 @@
 pub mod platform;
 pub mod prelude;
 pub mod print;
+pub mod ptr;
 pub mod rbtree;
 pub mod revocable;
 pub mod security;
diff --git a/rust/kernel/ptr.rs b/rust/kernel/ptr.rs
new file mode 100644
index 0000000000000000000000000000000000000000..6d941db58944619ea5b05676af06981a3ceaaca8
--- /dev/null
+++ b/rust/kernel/ptr.rs
@@ -0,0 +1,213 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Types and functions to work with pointers and addresses.
+
+use core::fmt::Debug;
+use core::num::NonZero;
+use core::ops::{BitAnd, Not};
+
+use crate::build_assert;
+use crate::num::CheckedAdd;
+
+/// Type representing an alignment, which is always a power of two.
+///
+/// It be used to validate that a given value is a valid alignment, and to perform masking and
+/// align down/up operations. The alignment operations are done using the [`align_up!`] and
+/// [`align_down!`] macros.
+///
+/// Heavily inspired by the [`Alignment`] nightly feature from the Rust standard library, and
+/// hopefully to be eventually replaced by it.
+///
+/// [`Alignment`]: https://github.com/rust-lang/rust/issues/102070
+///
+/// # Invariants
+///
+/// An alignment is always a power of two.
+#[repr(transparent)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct Alignment(NonZero<usize>);
+
+impl Alignment {
+    /// Validates that `align` is a power of two at build-time, and returns an [`Alignment`] of the
+    /// same value.
+    ///
+    /// A build error is triggered if `align` cannot be asserted to be a power of two.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::ptr::Alignment;
+    ///
+    /// let v = Alignment::new(16);
+    /// assert_eq!(v.as_usize(), 16);
+    /// ```
+    #[inline(always)]
+    pub const fn new(align: usize) -> Self {
+        build_assert!(align.is_power_of_two());
+
+        // INVARIANT: `align` is a power of two.
+        // SAFETY: `align` is a power of two, and thus non-zero.
+        Self(unsafe { NonZero::new_unchecked(align) })
+    }
+
+    /// Validates that `align` is a power of two at runtime, and returns an
+    /// [`Alignment`] of the same value.
+    ///
+    /// [`None`] is returned if `align` was not a power of two.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::ptr::Alignment;
+    ///
+    /// assert_eq!(Alignment::new_checked(16), Some(Alignment::new(16)));
+    /// assert_eq!(Alignment::new_checked(15), None);
+    /// assert_eq!(Alignment::new_checked(1), Some(Alignment::new(1)));
+    /// assert_eq!(Alignment::new_checked(0), None);
+    /// ```
+    #[inline(always)]
+    pub const fn new_checked(align: usize) -> Option<Self> {
+        if align.is_power_of_two() {
+            // INVARIANT: `align` is a power of two.
+            // SAFETY: `align` is a power of two, and thus non-zero.
+            Some(Self(unsafe { NonZero::new_unchecked(align) }))
+        } else {
+            None
+        }
+    }
+
+    /// Returns the alignment of `T`.
+    #[inline(always)]
+    pub const fn of<T>() -> Self {
+        // INVARIANT: `align_of` always returns a power of 2.
+        Self(unsafe { NonZero::new_unchecked(align_of::<T>()) })
+    }
+
+    /// Returns the base-2 logarithm of the alignment.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::ptr::Alignment;
+    ///
+    /// assert_eq!(Alignment::of::<u8>().log2(), 0);
+    /// assert_eq!(Alignment::new(16).log2(), 4);
+    /// ```
+    #[inline(always)]
+    pub const fn log2(self) -> u32 {
+        self.0.ilog2()
+    }
+
+    /// Returns this alignment as a [`NonZero`].
+    ///
+    /// It is guaranteed to be a power of two.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::ptr::Alignment;
+    ///
+    /// assert_eq!(Alignment::new(16).as_nonzero().get(), 16);
+    /// ```
+    #[inline(always)]
+    pub const fn as_nonzero(self) -> NonZero<usize> {
+        if !self.0.is_power_of_two() {
+            // SAFETY: per the invariants, `self.0` is always a power of two so this block will
+            // never be reached.
+            unsafe { core::hint::unreachable_unchecked() }
+        }
+        self.0
+    }
+
+    /// Returns this alignment as a `usize`.
+    ///
+    /// It is guaranteed to be a power of two.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::ptr::Alignment;
+    ///
+    /// assert_eq!(Alignment::new(16).as_usize(), 16);
+    /// ```
+    #[inline(always)]
+    pub const fn as_usize(self) -> usize {
+        self.as_nonzero().get()
+    }
+
+    /// Returns the mask corresponding to `self.as_usize() - 1`.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::ptr::Alignment;
+    ///
+    /// assert_eq!(Alignment::new(0x10).mask(), 0xf);
+    /// ```
+    #[inline(always)]
+    pub const fn mask(self) -> usize {
+        // INVARIANT: `self.as_usize()` is guaranteed to be a power of two (i.e. non-zero), thus
+        // `1` can safely be substracted from it.
+        self.as_usize() - 1
+    }
+
+    /// Aligns `value` down to this alignment.
+    ///
+    /// If the alignment contained in `self` is too large for `T`, then `0` is returned, which
+    /// is correct as it is also the result that would have been returned if it did.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::ptr::Alignment;
+    ///
+    /// assert_eq!(Alignment::new(0x10).align_down(0x2f), 0x20);
+    /// assert_eq!(Alignment::new(0x10).align_down(0x30), 0x30);
+    /// assert_eq!(Alignment::new(0x1000).align_down(0xf0u8), 0x0);
+    /// ```
+    #[inline(always)]
+    pub fn align_down<T>(self, value: T) -> T
+    where
+        T: TryFrom<usize> + BitAnd<Output = T> + Not<Output = T> + Default,
+    {
+        T::try_from(self.mask())
+            .map(|mask| value & !mask)
+            .unwrap_or(T::default())
+    }
+
+    /// Aligns `value` up to this alignment, returning `None` if aligning pushes the result above
+    /// the limits of `value`'s type.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::ptr::Alignment;
+    ///
+    /// assert_eq!(Alignment::new(0x10).align_up(0x4f), Some(0x50));
+    /// assert_eq!(Alignment::new(0x10).align_up(0x40), Some(0x40));
+    /// assert_eq!(Alignment::new(0x10).align_up(0x0), Some(0x0));
+    /// assert_eq!(Alignment::new(0x10).align_up(u8::MAX), None);
+    /// assert_eq!(Alignment::new(0x100).align_up(0x10u8), None);
+    /// assert_eq!(Alignment::new(0x100).align_up(0x0u8), Some(0x0));
+    /// ```
+    #[inline(always)]
+    pub fn align_up<T>(self, value: T) -> Option<T>
+    where
+        T: TryFrom<usize>
+            + BitAnd<Output = T>
+            + Not<Output = T>
+            + Default
+            + PartialEq
+            + Copy
+            + CheckedAdd,
+    {
+        let aligned_down = self.align_down(value);
+        if value == aligned_down {
+            Some(aligned_down)
+        } else {
+            T::try_from(self.as_usize())
+                .ok()
+                .and_then(|align| aligned_down.checked_add(align))
+        }
+    }
+}

-- 
2.50.1

Re: [PATCH v2 2/4] rust: add `Alignment` type

[Miguel Ojeda]

On Mon, Aug 4, 2025 at 1:45 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> +/// align down/up operations. The alignment operations are done using the [`align_up!`] and
> +/// [`align_down!`] macros.

These intra-doc links don't work (they are not macros in this version at least).

> +    /// Returns the alignment of `T`.
> +    #[inline(always)]
> +    pub const fn of<T>() -> Self {
> +        // INVARIANT: `align_of` always returns a power of 2.
> +        Self(unsafe { NonZero::new_unchecked(align_of::<T>()) })

Missing safety comment (`CLIPPY=1` spots it).

Also, cannot we use `new()` here? i.e. the value will be known at compile-time.

> +        if !self.0.is_power_of_two() {
> +            // SAFETY: per the invariants, `self.0` is always a power of two so this block will
> +            // never be reached.
> +            unsafe { core::hint::unreachable_unchecked() }
> +        }

I guess this one is here to help optimize users after they inline the
cal? Is there a particular case you noticed? i.e. it may be worth
mentioning it.

> +    pub const fn mask(self) -> usize {
> +        // INVARIANT: `self.as_usize()` is guaranteed to be a power of two (i.e. non-zero), thus
> +        // `1` can safely be substracted from it.
> +        self.as_usize() - 1
> +    }

I am not sure why there is `// INVARIANT` here, since we are not
creating a new `Self`.

I guess by "safely" you are trying to say there is no overflow risk --
I would be explicit and avoid "safe", since it is safe to overflow.

Typo: subtracted

Cheers,
Miguel

Re: [PATCH v2 2/4] rust: add `Alignment` type

[Benno Lossin]

On Mon Aug 4, 2025 at 4:17 PM CEST, Miguel Ojeda wrote:
> On Mon, Aug 4, 2025 at 1:45 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>> +        if !self.0.is_power_of_two() {
>> +            // SAFETY: per the invariants, `self.0` is always a power of two so this block will
>> +            // never be reached.
>> +            unsafe { core::hint::unreachable_unchecked() }
>> +        }
>
> I guess this one is here to help optimize users after they inline the
> cal? Is there a particular case you noticed? i.e. it may be worth
> mentioning it.

I suggested this in the previous version [1]. For example, it optimizes
division to only be a left shift.

[1]: https://lore.kernel.org/all/DBL1ZGZCSJF3.29HNS9BSN89C6@kernel.org

---
Cheers,
Benno

Re: [PATCH v2 2/4] rust: add `Alignment` type

[Alexandre Courbot]

On Mon Aug 4, 2025 at 11:17 PM JST, Miguel Ojeda wrote:
> On Mon, Aug 4, 2025 at 1:45 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>>
>> +/// align down/up operations. The alignment operations are done using the [`align_up!`] and
>> +/// [`align_down!`] macros.
>
> These intra-doc links don't work (they are not macros in this version at least).

Oops, these are remnants of some previous attempt at making this work,
which I could swear I removed. That and the sentence's grammar as a
whole is incorrect. Let me rework this.

>
>> +    /// Returns the alignment of `T`.
>> +    #[inline(always)]
>> +    pub const fn of<T>() -> Self {
>> +        // INVARIANT: `align_of` always returns a power of 2.
>> +        Self(unsafe { NonZero::new_unchecked(align_of::<T>()) })
>
> Missing safety comment (`CLIPPY=1` spots it).
>
> Also, cannot we use `new()` here? i.e. the value will be known at compile-time.

We can indeed! Brilliant.

>
>> +        if !self.0.is_power_of_two() {
>> +            // SAFETY: per the invariants, `self.0` is always a power of two so this block will
>> +            // never be reached.
>> +            unsafe { core::hint::unreachable_unchecked() }
>> +        }
>
> I guess this one is here to help optimize users after they inline the
> cal? Is there a particular case you noticed? i.e. it may be worth
> mentioning it.

This was a suggestion from Benno [1], to give more hints to the
compiler. Let me add a comment to justify its presence.

[1] https://lore.kernel.org/rust-for-linux/DBL1ZGZCSJF3.29HNS9BSN89C6@kernel.org/

>
>> +    pub const fn mask(self) -> usize {
>> +        // INVARIANT: `self.as_usize()` is guaranteed to be a power of two (i.e. non-zero), thus
>> +        // `1` can safely be substracted from it.
>> +        self.as_usize() - 1
>> +    }
>
> I am not sure why there is `// INVARIANT` here, since we are not
> creating a new `Self`.

>
> I guess by "safely" you are trying to say there is no overflow risk --
> I would be explicit and avoid "safe", since it is safe to overflow.

I just wanted to justify that we cannot substract from 0. Maybe an
`unchecked_sub` would be better here? The `unsafe` block would also
justify the safety comment.

... mmm actually that would be `checked_sub().unwrap_unchecked()`, since
`unchecked_sub` appeared in Rust 1.79.

Re: [PATCH v2 2/4] rust: add `Alignment` type

[Benno Lossin]

On Tue Aug 5, 2025 at 3:13 PM CEST, Alexandre Courbot wrote:
> On Mon Aug 4, 2025 at 11:17 PM JST, Miguel Ojeda wrote:
>> On Mon, Aug 4, 2025 at 1:45 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>>> +    pub const fn mask(self) -> usize {
>>> +        // INVARIANT: `self.as_usize()` is guaranteed to be a power of two (i.e. non-zero), thus
>>> +        // `1` can safely be substracted from it.
>>> +        self.as_usize() - 1
>>> +    }
>>
>> I am not sure why there is `// INVARIANT` here, since we are not
>> creating a new `Self`.
>
>>
>> I guess by "safely" you are trying to say there is no overflow risk --
>> I would be explicit and avoid "safe", since it is safe to overflow.
>
> I just wanted to justify that we cannot substract from 0. Maybe an
> `unchecked_sub` would be better here? The `unsafe` block would also
> justify the safety comment.
>
> ... mmm actually that would be `checked_sub().unwrap_unchecked()`, since
> `unchecked_sub` appeared in Rust 1.79.

No need to do that, the compiler already knows that there won't be
underflow and optimizes it accordingly (since self.as_usize() converts a
`NonZero<usize>`). [1] (it also works when removing the
`is_power_of_two` check, but if we only stored a `usize`, I bet the
compiler would also optimize this given that check)

I'd just add a normal comment that mentions no underflow can occur. This
shouldn't need unsafe.

[1]: https://godbolt.org/z/M5x1W49nn

---
Cheers,
Benno

Re: [PATCH v2 2/4] rust: add `Alignment` type

[Daniel Almeida]

Hi Alex,

> On 4 Aug 2025, at 08:45, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 
> Alignment operations are very common in the kernel. Since they are
> always performed using a power of two value, enforcing this invariant
> through a dedicated type leads to less bugs and can lead to improved
> generated code.
> 
> Introduce the `Alignment` type, inspired by the nightly Rust feature of
> the same name. It provides the same interface as its upstream namesake,
> while extending it with `align_up` and `align_down` operations that are
> usable on any integer type.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> rust/kernel/lib.rs |   1 +
> rust/kernel/ptr.rs | 213 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 214 insertions(+)
> 
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 2955f65da1278dd4cba1e4272ff178b8211a892c..0e66b55cde66ee1b274862cd78ad465a572dc5d9 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -100,6 +100,7 @@
> pub mod platform;
> pub mod prelude;
> pub mod print;
> +pub mod ptr;
> pub mod rbtree;
> pub mod revocable;
> pub mod security;
> diff --git a/rust/kernel/ptr.rs b/rust/kernel/ptr.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..6d941db58944619ea5b05676af06981a3ceaaca8
> --- /dev/null
> +++ b/rust/kernel/ptr.rs
> @@ -0,0 +1,213 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Types and functions to work with pointers and addresses.
> +
> +use core::fmt::Debug;
> +use core::num::NonZero;
> +use core::ops::{BitAnd, Not};
> +
> +use crate::build_assert;
> +use crate::num::CheckedAdd;
> +
> +/// Type representing an alignment, which is always a power of two.
> +///
> +/// It be used to validate that a given value is a valid alignment, and to perform masking and
> +/// align down/up operations. The alignment operations are done using the [`align_up!`] and

Nit: I’d go with “align up or align down operations” instead of using a slash.

> +/// [`align_down!`] macros.
> +///
> +/// Heavily inspired by the [`Alignment`] nightly feature from the Rust standard library, and
> +/// hopefully to be eventually replaced by it.

It’s a bit hard to parse this.

Also, I wonder if we should standardize some syntax for TODOs so we can parse
them using a script? This way we can actually keep track and perhaps pipe them
to our GitHub page as “good first issues” or just regular issues.

I guess a simple "// TODO: “ here will do, for example.

> +///
> +/// [`Alignment`]: https://github.com/rust-lang/rust/issues/102070
> +///
> +/// # Invariants
> +///
> +/// An alignment is always a power of two.
> +#[repr(transparent)]
> +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
> +pub struct Alignment(NonZero<usize>);
> +
> +impl Alignment {
> +    /// Validates that `align` is a power of two at build-time, and returns an [`Alignment`] of the
> +    /// same value.
> +    ///
> +    /// A build error is triggered if `align` cannot be asserted to be a power of two.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::ptr::Alignment;
> +    ///
> +    /// let v = Alignment::new(16);
> +    /// assert_eq!(v.as_usize(), 16);
> +    /// ```
> +    #[inline(always)]
> +    pub const fn new(align: usize) -> Self {
> +        build_assert!(align.is_power_of_two());
> +
> +        // INVARIANT: `align` is a power of two.
> +        // SAFETY: `align` is a power of two, and thus non-zero.
> +        Self(unsafe { NonZero::new_unchecked(align) })
> +    }
> +
> +    /// Validates that `align` is a power of two at runtime, and returns an
> +    /// [`Alignment`] of the same value.
> +    ///
> +    /// [`None`] is returned if `align` was not a power of two.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::ptr::Alignment;
> +    ///
> +    /// assert_eq!(Alignment::new_checked(16), Some(Alignment::new(16)));
> +    /// assert_eq!(Alignment::new_checked(15), None);
> +    /// assert_eq!(Alignment::new_checked(1), Some(Alignment::new(1)));
> +    /// assert_eq!(Alignment::new_checked(0), None);
> +    /// ```
> +    #[inline(always)]
> +    pub const fn new_checked(align: usize) -> Option<Self> {
> +        if align.is_power_of_two() {
> +            // INVARIANT: `align` is a power of two.
> +            // SAFETY: `align` is a power of two, and thus non-zero.
> +            Some(Self(unsafe { NonZero::new_unchecked(align) }))
> +        } else {
> +            None
> +        }
> +    }
> +
> +    /// Returns the alignment of `T`.
> +    #[inline(always)]
> +    pub const fn of<T>() -> Self {
> +        // INVARIANT: `align_of` always returns a power of 2.
> +        Self(unsafe { NonZero::new_unchecked(align_of::<T>()) })
> +    }

> +
> +    /// Returns the base-2 logarithm of the alignment.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::ptr::Alignment;
> +    ///
> +    /// assert_eq!(Alignment::of::<u8>().log2(), 0);
> +    /// assert_eq!(Alignment::new(16).log2(), 4);
> +    /// ```
> +    #[inline(always)]
> +    pub const fn log2(self) -> u32 {
> +        self.0.ilog2()
> +    }
> +
> +    /// Returns this alignment as a [`NonZero`].
> +    ///
> +    /// It is guaranteed to be a power of two.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::ptr::Alignment;
> +    ///
> +    /// assert_eq!(Alignment::new(16).as_nonzero().get(), 16);
> +    /// ```
> +    #[inline(always)]
> +    pub const fn as_nonzero(self) -> NonZero<usize> {
> +        if !self.0.is_power_of_two() {
> +            // SAFETY: per the invariants, `self.0` is always a power of two so this block will
> +            // never be reached.
> +            unsafe { core::hint::unreachable_unchecked() }
> +        }
> +        self.0
> +    }
> +
> +    /// Returns this alignment as a `usize`.
> +    ///
> +    /// It is guaranteed to be a power of two.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::ptr::Alignment;
> +    ///
> +    /// assert_eq!(Alignment::new(16).as_usize(), 16);
> +    /// ```
> +    #[inline(always)]
> +    pub const fn as_usize(self) -> usize {
> +        self.as_nonzero().get()
> +    }
> +
> +    /// Returns the mask corresponding to `self.as_usize() - 1`.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::ptr::Alignment;
> +    ///
> +    /// assert_eq!(Alignment::new(0x10).mask(), 0xf);
> +    /// ```
> +    #[inline(always)]
> +    pub const fn mask(self) -> usize {
> +        // INVARIANT: `self.as_usize()` is guaranteed to be a power of two (i.e. non-zero), thus
> +        // `1` can safely be substracted from it.
> +        self.as_usize() - 1
> +    }
> +
> +    /// Aligns `value` down to this alignment.
> +    ///
> +    /// If the alignment contained in `self` is too large for `T`, then `0` is returned, which
> +    /// is correct as it is also the result that would have been returned if it did.

I half get this, but still: If it did what?

> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::ptr::Alignment;
> +    ///
> +    /// assert_eq!(Alignment::new(0x10).align_down(0x2f), 0x20);
> +    /// assert_eq!(Alignment::new(0x10).align_down(0x30), 0x30);
> +    /// assert_eq!(Alignment::new(0x1000).align_down(0xf0u8), 0x0);
> +    /// ```
> +    #[inline(always)]
> +    pub fn align_down<T>(self, value: T) -> T
> +    where
> +        T: TryFrom<usize> + BitAnd<Output = T> + Not<Output = T> + Default,
> +    {
> +        T::try_from(self.mask())
> +            .map(|mask| value & !mask)
> +            .unwrap_or(T::default())
> +    }
> +
> +    /// Aligns `value` up to this alignment, returning `None` if aligning pushes the result above
> +    /// the limits of `value`'s type.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::ptr::Alignment;
> +    ///
> +    /// assert_eq!(Alignment::new(0x10).align_up(0x4f), Some(0x50));
> +    /// assert_eq!(Alignment::new(0x10).align_up(0x40), Some(0x40));
> +    /// assert_eq!(Alignment::new(0x10).align_up(0x0), Some(0x0));
> +    /// assert_eq!(Alignment::new(0x10).align_up(u8::MAX), None);
> +    /// assert_eq!(Alignment::new(0x100).align_up(0x10u8), None);
> +    /// assert_eq!(Alignment::new(0x100).align_up(0x0u8), Some(0x0));
> +    /// ```
> +    #[inline(always)]
> +    pub fn align_up<T>(self, value: T) -> Option<T>
> +    where
> +        T: TryFrom<usize>
> +            + BitAnd<Output = T>
> +            + Not<Output = T>
> +            + Default
> +            + PartialEq
> +            + Copy
> +            + CheckedAdd,
> +    {
> +        let aligned_down = self.align_down(value);
> +        if value == aligned_down {
> +            Some(aligned_down)
> +        } else {
> +            T::try_from(self.as_usize())
> +                .ok()
> +                .and_then(|align| aligned_down.checked_add(align))
> +        }
> +    }
> +}
> 
> -- 
> 2.50.1
> 
> 

Everything else looks fine, IMHO.

— Daniel

Re: [PATCH v2 2/4] rust: add `Alignment` type

[Alexandre Courbot]

On Tue Aug 5, 2025 at 12:47 AM JST, Daniel Almeida wrote:
<snip>
>> +/// [`align_down!`] macros.
>> +///
>> +/// Heavily inspired by the [`Alignment`] nightly feature from the Rust standard library, and
>> +/// hopefully to be eventually replaced by it.
>
> It’s a bit hard to parse this.
>
> Also, I wonder if we should standardize some syntax for TODOs so we can parse
> them using a script? This way we can actually keep track and perhaps pipe them
> to our GitHub page as “good first issues” or just regular issues.
>
> I guess a simple "// TODO: “ here will do, for example.

FWIW, in Nova we tag each TODO items with a 4-letter identifier (i.e.
`TODO[ABCD]:` that is defined in our `todo.rst` file. This makes
grepping all the sites relevant to a given item easy.

>
>> +///
>> +/// [`Alignment`]: https://github.com/rust-lang/rust/issues/102070
>> +///
>> +/// # Invariants
>> +///
>> +/// An alignment is always a power of two.
>> +#[repr(transparent)]
>> +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
>> +pub struct Alignment(NonZero<usize>);
>> +
>> +impl Alignment {
>> +    /// Validates that `align` is a power of two at build-time, and returns an [`Alignment`] of the
>> +    /// same value.
>> +    ///
>> +    /// A build error is triggered if `align` cannot be asserted to be a power of two.
>> +    ///
>> +    /// # Examples
>> +    ///
>> +    /// ```
>> +    /// use kernel::ptr::Alignment;
>> +    ///
>> +    /// let v = Alignment::new(16);
>> +    /// assert_eq!(v.as_usize(), 16);
>> +    /// ```
>> +    #[inline(always)]
>> +    pub const fn new(align: usize) -> Self {
>> +        build_assert!(align.is_power_of_two());
>> +
>> +        // INVARIANT: `align` is a power of two.
>> +        // SAFETY: `align` is a power of two, and thus non-zero.
>> +        Self(unsafe { NonZero::new_unchecked(align) })
>> +    }
>> +
>> +    /// Validates that `align` is a power of two at runtime, and returns an
>> +    /// [`Alignment`] of the same value.
>> +    ///
>> +    /// [`None`] is returned if `align` was not a power of two.
>> +    ///
>> +    /// # Examples
>> +    ///
>> +    /// ```
>> +    /// use kernel::ptr::Alignment;
>> +    ///
>> +    /// assert_eq!(Alignment::new_checked(16), Some(Alignment::new(16)));
>> +    /// assert_eq!(Alignment::new_checked(15), None);
>> +    /// assert_eq!(Alignment::new_checked(1), Some(Alignment::new(1)));
>> +    /// assert_eq!(Alignment::new_checked(0), None);
>> +    /// ```
>> +    #[inline(always)]
>> +    pub const fn new_checked(align: usize) -> Option<Self> {
>> +        if align.is_power_of_two() {
>> +            // INVARIANT: `align` is a power of two.
>> +            // SAFETY: `align` is a power of two, and thus non-zero.
>> +            Some(Self(unsafe { NonZero::new_unchecked(align) }))
>> +        } else {
>> +            None
>> +        }
>> +    }
>> +
>> +    /// Returns the alignment of `T`.
>> +    #[inline(always)]
>> +    pub const fn of<T>() -> Self {
>> +        // INVARIANT: `align_of` always returns a power of 2.
>> +        Self(unsafe { NonZero::new_unchecked(align_of::<T>()) })
>> +    }
>
>> +
>> +    /// Returns the base-2 logarithm of the alignment.
>> +    ///
>> +    /// # Examples
>> +    ///
>> +    /// ```
>> +    /// use kernel::ptr::Alignment;
>> +    ///
>> +    /// assert_eq!(Alignment::of::<u8>().log2(), 0);
>> +    /// assert_eq!(Alignment::new(16).log2(), 4);
>> +    /// ```
>> +    #[inline(always)]
>> +    pub const fn log2(self) -> u32 {
>> +        self.0.ilog2()
>> +    }
>> +
>> +    /// Returns this alignment as a [`NonZero`].
>> +    ///
>> +    /// It is guaranteed to be a power of two.
>> +    ///
>> +    /// # Examples
>> +    ///
>> +    /// ```
>> +    /// use kernel::ptr::Alignment;
>> +    ///
>> +    /// assert_eq!(Alignment::new(16).as_nonzero().get(), 16);
>> +    /// ```
>> +    #[inline(always)]
>> +    pub const fn as_nonzero(self) -> NonZero<usize> {
>> +        if !self.0.is_power_of_two() {
>> +            // SAFETY: per the invariants, `self.0` is always a power of two so this block will
>> +            // never be reached.
>> +            unsafe { core::hint::unreachable_unchecked() }
>> +        }
>> +        self.0
>> +    }
>> +
>> +    /// Returns this alignment as a `usize`.
>> +    ///
>> +    /// It is guaranteed to be a power of two.
>> +    ///
>> +    /// # Examples
>> +    ///
>> +    /// ```
>> +    /// use kernel::ptr::Alignment;
>> +    ///
>> +    /// assert_eq!(Alignment::new(16).as_usize(), 16);
>> +    /// ```
>> +    #[inline(always)]
>> +    pub const fn as_usize(self) -> usize {
>> +        self.as_nonzero().get()
>> +    }
>> +
>> +    /// Returns the mask corresponding to `self.as_usize() - 1`.
>> +    ///
>> +    /// # Examples
>> +    ///
>> +    /// ```
>> +    /// use kernel::ptr::Alignment;
>> +    ///
>> +    /// assert_eq!(Alignment::new(0x10).mask(), 0xf);
>> +    /// ```
>> +    #[inline(always)]
>> +    pub const fn mask(self) -> usize {
>> +        // INVARIANT: `self.as_usize()` is guaranteed to be a power of two (i.e. non-zero), thus
>> +        // `1` can safely be substracted from it.
>> +        self.as_usize() - 1
>> +    }
>> +
>> +    /// Aligns `value` down to this alignment.
>> +    ///
>> +    /// If the alignment contained in `self` is too large for `T`, then `0` is returned, which
>> +    /// is correct as it is also the result that would have been returned if it did.
>
> I half get this, but still: If it did what?

I also stumbled while re-reading this sentence. :) Fixed.

[PATCH v2 3/4] gpu: nova-core: use Alignment for alignment-related operations

[Alexandre Courbot]

Make use of the newly-available `Alignment` type and remove the
corresponding TODO item.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 Documentation/gpu/nova/core/todo.rst    |  1 -
 drivers/gpu/nova-core/fb.rs             |  6 +++---
 drivers/gpu/nova-core/firmware/fwsec.rs | 11 +++++------
 drivers/gpu/nova-core/vbios.rs          |  4 ++--
 4 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/Documentation/gpu/nova/core/todo.rst b/Documentation/gpu/nova/core/todo.rst
index 894a1e9c3741a43ad4eb76d24a9486862999874e..8fdb5bced3460a3971699df79ffa2c69f84b2735 100644
--- a/Documentation/gpu/nova/core/todo.rst
+++ b/Documentation/gpu/nova/core/todo.rst
@@ -147,7 +147,6 @@ Numerical operations [NUMM]
 Nova uses integer operations that are not part of the standard library (or not
 implemented in an optimized way for the kernel). These include:
 
-- Aligning up and down to a power of two,
 - The "Find Last Set Bit" (`fls` function of the C part of the kernel)
   operation.
 
diff --git a/drivers/gpu/nova-core/fb.rs b/drivers/gpu/nova-core/fb.rs
index 4a702525fff4f394b75fcf54145ba78e34a1a539..ebeffc1b1feccc5c4dc08135d8447ec185a3068a 100644
--- a/drivers/gpu/nova-core/fb.rs
+++ b/drivers/gpu/nova-core/fb.rs
@@ -3,6 +3,7 @@
 use core::ops::Range;
 
 use kernel::prelude::*;
+use kernel::ptr::Alignment;
 use kernel::sizes::*;
 use kernel::types::ARef;
 use kernel::{dev_warn, device};
@@ -130,10 +131,9 @@ pub(crate) fn new(chipset: Chipset, bar: &Bar0) -> Result<Self> {
         };
 
         let frts = {
-            const FRTS_DOWN_ALIGN: u64 = SZ_128K as u64;
+            const FRTS_DOWN_ALIGN: Alignment = Alignment::new(SZ_128K);
             const FRTS_SIZE: u64 = SZ_1M as u64;
-            // TODO[NUMM]: replace with `align_down` once it lands.
-            let frts_base = (vga_workspace.start & !(FRTS_DOWN_ALIGN - 1)) - FRTS_SIZE;
+            let frts_base = FRTS_DOWN_ALIGN.align_down(vga_workspace.start) - FRTS_SIZE;
 
             frts_base..frts_base + FRTS_SIZE
         };
diff --git a/drivers/gpu/nova-core/firmware/fwsec.rs b/drivers/gpu/nova-core/firmware/fwsec.rs
index 0dff3cfa90afee0cd4c3348023c8bfd7edccdb29..add8cea7bbecc98238d418d0fe6f235c6b850d58 100644
--- a/drivers/gpu/nova-core/firmware/fwsec.rs
+++ b/drivers/gpu/nova-core/firmware/fwsec.rs
@@ -16,6 +16,7 @@
 
 use kernel::device::{self, Device};
 use kernel::prelude::*;
+use kernel::ptr::Alignment;
 use kernel::transmute::FromBytes;
 
 use crate::dma::DmaObject;
@@ -203,7 +204,7 @@ pub(crate) struct FwsecFirmware {
 }
 
 // We need to load full DMEM pages.
-const DMEM_LOAD_SIZE_ALIGN: u32 = 256;
+const DMEM_LOAD_SIZE_ALIGN: usize = 256;
 
 impl FalconLoadParams for FwsecFirmware {
     fn imem_load_params(&self) -> FalconLoadTarget {
@@ -218,11 +219,9 @@ fn dmem_load_params(&self) -> FalconLoadTarget {
         FalconLoadTarget {
             src_start: self.desc.imem_load_size,
             dst_start: self.desc.dmem_phys_base,
-            // TODO[NUMM]: replace with `align_up` once it lands.
-            len: self
-                .desc
-                .dmem_load_size
-                .next_multiple_of(DMEM_LOAD_SIZE_ALIGN),
+            len: Alignment::new(DMEM_LOAD_SIZE_ALIGN)
+                .align_up(self.desc.dmem_load_size)
+                .unwrap_or(u32::MAX),
         }
     }
 
diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 5b5d9f38cbb3a6b1c374c1e0eee2509eb8d5660c..3bc24ce26b9d4d1948313f808986211facb0e8d0 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -10,6 +10,7 @@
 use kernel::error::Result;
 use kernel::pci;
 use kernel::prelude::*;
+use kernel::ptr::Alignment;
 
 /// The offset of the VBIOS ROM in the BAR0 space.
 const ROM_OFFSET: usize = 0x300000;
@@ -177,8 +178,7 @@ fn next(&mut self) -> Option<Self::Item> {
 
         // Advance to next image (aligned to 512 bytes).
         self.current_offset += image_size;
-        // TODO[NUMM]: replace with `align_up` once it lands.
-        self.current_offset = self.current_offset.next_multiple_of(512);
+        self.current_offset = Alignment::new(512).align_up(self.current_offset)?;
 
         Some(Ok(full_image))
     }

-- 
2.50.1

[PATCH v2 4/4] gpu: nova-core: use `checked_ilog2` to emulate `fls`

[Alexandre Courbot]

Rust's `checked_ilog2` is in effect equivalent to the C `fls` operation,
with the exception that its result is zero-indexed. This means we don't
have a good basis to introduce an equivalent of `fls` on our own.

Convert the relevant Nova code to use `checked_ilog2`, and remove the
corresponding TODO item.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 Documentation/gpu/nova/core/todo.rst      | 14 --------------
 drivers/gpu/nova-core/falcon/hal/ga102.rs |  4 ++--
 2 files changed, 2 insertions(+), 16 deletions(-)

diff --git a/Documentation/gpu/nova/core/todo.rst b/Documentation/gpu/nova/core/todo.rst
index 8fdb5bced3460a3971699df79ffa2c69f84b2735..01dfa858d11fe377c345b463742c13c37878e334 100644
--- a/Documentation/gpu/nova/core/todo.rst
+++ b/Documentation/gpu/nova/core/todo.rst
@@ -141,20 +141,6 @@ Features desired before this happens:
 | Complexity: Advanced
 | Contact: Alexandre Courbot
 
-Numerical operations [NUMM]
----------------------------
-
-Nova uses integer operations that are not part of the standard library (or not
-implemented in an optimized way for the kernel). These include:
-
-- The "Find Last Set Bit" (`fls` function of the C part of the kernel)
-  operation.
-
-A `num` core kernel module is being designed to provide these operations.
-
-| Complexity: Intermediate
-| Contact: Alexandre Courbot
-
 Delay / Sleep abstractions [DLAY]
 ---------------------------------
 
diff --git a/drivers/gpu/nova-core/falcon/hal/ga102.rs b/drivers/gpu/nova-core/falcon/hal/ga102.rs
index 52c33d3f22a8e920742b45940c346c47fdc70e93..430a511aa1f85477690e78cdc1104f0e0097b0e4 100644
--- a/drivers/gpu/nova-core/falcon/hal/ga102.rs
+++ b/drivers/gpu/nova-core/falcon/hal/ga102.rs
@@ -69,8 +69,8 @@ fn signature_reg_fuse_version_ga102(
     let reg_fuse_version =
         bar.read32(reg_fuse_base + ((ucode_id - 1) as usize * core::mem::size_of::<u32>()));
 
-    // TODO[NUMM]: replace with `last_set_bit` once it lands.
-    Ok(u32::BITS - reg_fuse_version.leading_zeros())
+    // The original C code performs a `fls`; this is equivalent.
+    Ok(reg_fuse_version.checked_ilog2().map_or(0, |v| v + 1))
 }
 
 fn program_brom_ga102<E: FalconEngine>(bar: &Bar0, params: &FalconBromParams) -> Result {

-- 
2.50.1

Re: [PATCH v2 0/4] rust: add `Alignment` type

[Miguel Ojeda]

On Mon, Aug 4, 2025 at 1:45 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> - The `last_set_bit` function is dropped, with the recommendation to use
>   the standard library's `checked_ilog2` which does essentially the same
>   thing.

Yeah, let's see what people think about this one on the kernel side.

I don't mind either way, i.e. to have a few wrappers with slightly
different semantics if that is more common/understandable.

> The upstream `Alignment` is more constrained than the `PowerOfTwo` of
> the last revision: it uses `usize` internally instead of a generic
> value, and does not provide `align_down` or `align_up` methods.

`PowerOfTwo` seemed fine to me as well (or even implementing one in
terms of the other).

> These two shortcomings come together very nicely to gift us with a nice
> headache: we need to align values potentially larger than `usize`, thus
> need to make `align_down` and `align_up` generic. The generic parameter
> needs to be constrained on the operations used to perform the alignment
> (e.g. `BitAnd`, `Not`, etc) and there is one essential operation for
> which no trait exists in the standard library: `checked_add`. Thus the
> first patch of this series introduces a trait for it in the `num` module
> and implements it for all integer types. I suspect we will need
> something alongside these lines for other purposes anyway, and probably
> other traits too.

This part could be avoided implementing them the other way around,
right? i.e. as an extension trait on the other side.

It may also be also a bit easier to understand on the call site, too,
since value would be first.

> This generic nature also restricts these methods to being non-const,
> unfortunately. I have tried to implement them as macros instead, but
> quickly hit a wall due to the inability to convert `Alignment`'s `usize`
> into the type of the value to align.

I guess we could also just have one per type like for other ones to
have them `const`, like we do for other similar things like
`bit`/`genmask`.

Cheers,
Miguel

Re: [PATCH v2 0/4] rust: add `Alignment` type

[Alexandre Courbot]

On Mon Aug 4, 2025 at 11:16 PM JST, Miguel Ojeda wrote:
> On Mon, Aug 4, 2025 at 1:45 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>>
>> - The `last_set_bit` function is dropped, with the recommendation to use
>>   the standard library's `checked_ilog2` which does essentially the same
>>   thing.
>
> Yeah, let's see what people think about this one on the kernel side.
>
> I don't mind either way, i.e. to have a few wrappers with slightly
> different semantics if that is more common/understandable.
>
>> The upstream `Alignment` is more constrained than the `PowerOfTwo` of
>> the last revision: it uses `usize` internally instead of a generic
>> value, and does not provide `align_down` or `align_up` methods.
>
> `PowerOfTwo` seemed fine to me as well (or even implementing one in
> terms of the other).

`PowerOfTwo` has little prospect of existing upstream, and I think we
should be able to live pretty well with `Alignment` thanks to the
suggestions you make below.

>
>> These two shortcomings come together very nicely to gift us with a nice
>> headache: we need to align values potentially larger than `usize`, thus
>> need to make `align_down` and `align_up` generic. The generic parameter
>> needs to be constrained on the operations used to perform the alignment
>> (e.g. `BitAnd`, `Not`, etc) and there is one essential operation for
>> which no trait exists in the standard library: `checked_add`. Thus the
>> first patch of this series introduces a trait for it in the `num` module
>> and implements it for all integer types. I suspect we will need
>> something alongside these lines for other purposes anyway, and probably
>> other traits too.
>
> This part could be avoided implementing them the other way around,
> right? i.e. as an extension trait on the other side.
>
> It may also be also a bit easier to understand on the call site, too,
> since value would be first.

Yes! This is much better and more intuitive.

>
>> This generic nature also restricts these methods to being non-const,
>> unfortunately. I have tried to implement them as macros instead, but
>> quickly hit a wall due to the inability to convert `Alignment`'s `usize`
>> into the type of the value to align.
>
> I guess we could also just have one per type like for other ones to
> have them `const`, like we do for other similar things like
> `bit`/`genmask`.

This leaves us with two viable solutions: `Alignable` extension trait
with `align_up` and `align_down` operations that take an `Alignment` as
parameter (with the caveat that they could not be const for now), or a
set of per-type functions defined using a macro, similar to bit/genmask.
I am fine with both but don't know which one would be preferred, can the
R4L leadership provide some guidance? :)

Thanks,
Alex.

Re: [PATCH v2 0/4] rust: add `Alignment` type

[John Hubbard]

On 8/5/25 6:26 AM, Alexandre Courbot wrote:
...
> This leaves us with two viable solutions: `Alignable` extension trait
> with `align_up` and `align_down` operations that take an `Alignment` as
> parameter (with the caveat that they could not be const for now), or a

From a readability point of view, this first option sounds nice. It's
clear, concise, and doesn't involve macros.

thanks,
-- 
John Hubbard

> set of per-type functions defined using a macro, similar to bit/genmask.
> I am fine with both but don't know which one would be preferred, can the
> R4L leadership provide some guidance? :)
>