Re: [PATCH v2 1/3] rust: sizes: add DeviceSize trait for device address space constants

["Alexandre Courbot" <acourbot@nvidia.com>]

General note: `make rustfmt` reformats code from the series.

On Thu Mar 12, 2026 at 12:15 PM JST, John Hubbard wrote:
> The SZ_* constants are usize, matching the CPU pointer width. But
> device address spaces have their own widths (32-bit MMIO windows,
> 64-bit GPU framebuffers, etc.), so drivers end up with repeated
> usize-to-u64 or usize-to-u32 conversion calls like
> usize_as_u64(SZ_1M). This adds boilerplate with no safety benefit.

`usize_as_u64` is Nova-only at the moment, so it might not be
well understood in this context.

>
> Add a DeviceSize trait with associated SZ_* constants, implemented for
> u32 and u64. With the trait in scope, callers write u64::SZ_1M or
> u32::SZ_4K to get the constant in their device's native width. All
> SZ_* values fit in a u32, so both implementations are lossless. The
> u32 impl has a const assert to catch any future constant that would
> overflow; the u64 cast from usize is inherently lossless.
>
> Replace the hand-written constant list with a define_sizes! macro that
> generates the usize constants, the trait, and both trait impls from a
> single list of names. Adding a new size or a new target type requires
> changing only one place.
>
> The trait also enables future generic APIs that accept T: DeviceSize,
> so shared infrastructure can work with whatever address width the
> driver declares.
>
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Link: https://lore.kernel.org/all/DGB9G697GSWO.3VBFGU5MKFPMR@kernel.org/
> Link: https://lore.kernel.org/all/DGHI8WRKBQS9.38910L6FIIZTE@kernel.org/
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  rust/kernel/sizes.rs | 132 ++++++++++++++++++++++++++++---------------
>  1 file changed, 88 insertions(+), 44 deletions(-)
>
> diff --git a/rust/kernel/sizes.rs b/rust/kernel/sizes.rs
> index 661e680d9330..6b11ec6d97b3 100644
> --- a/rust/kernel/sizes.rs
> +++ b/rust/kernel/sizes.rs
> @@ -3,48 +3,92 @@
>  //! Commonly used sizes.
>  //!
>  //! C headers: [`include/linux/sizes.h`](srctree/include/linux/sizes.h).
> +//!
> +//! The top-level `SZ_*` constants are [`usize`]-typed, for use in kernel page
> +//! arithmetic and similar CPU-side work.
> +//!
> +//! The [`DeviceSize`] trait provides the same constants as associated constants
> +//! on [`u32`] and [`u64`], for use in device address spaces where the address
> +//! width depends on the hardware. Device drivers frequently need these constants
> +//! as [`u64`] (or [`u32`]) rather than [`usize`], because device address spaces
> +//! are sized independently of the CPU pointer width.
> +//!

There should be a `# Examples` here.

> +//! ```
> +//! use kernel::sizes::{DeviceSize, SZ_1M};
> +//!
> +//! // usize constant (CPU-side)
> +//! let pages: usize = SZ_1M / kernel::page::PAGE_SIZE;

Maybe `num_pages_in_1m` to be more precise.

> +//!
> +//! // Device-side constant via the trait
> +//! let heap_size: u64 = 14 * u64::SZ_1M;
> +//! let small: u32 = u32::SZ_4K;
> +//! ```
> +
> +macro_rules! define_sizes {
> +    ($($name:ident),* $(,)?) => {
> +        // `usize` constants, from the C `SZ_*` defines in `include/linux/sizes.h`.
> +        $(
> +            #[doc = concat!("`", stringify!($name), "` as a [`usize`].")]

All the information in this doccomment (the value and the type) is
already in the declaration below, which appears both in the
rust-analyzer and the HTML doc.

The original doccomments OTOH were useful and they showed the
hexadecimal value, allowing the reader to understand at a peek which bit
was affected. Right now we don't have this information as the value is
displayed in decimal in the documentation. So I think we should just
keep the original doccomments without any extra ornament.

> +            pub const $name: usize = bindings::$name as usize;
> +        )*
> +
> +        /// Size constants for device address spaces.
> +        ///
> +        /// Implemented for [`u32`] and [`u64`] so drivers can choose the width
> +        /// that matches their hardware. All `SZ_*` values fit in a [`u32`], so
> +        /// both implementations are lossless.
> +        ///
> +        /// ```
> +        /// use kernel::sizes::DeviceSize;
> +        ///
> +        /// let gpu_heap: u64 = 14 * u64::SZ_1M;
> +        /// let mmio_window: u32 = u32::SZ_16M;
> +        /// ```
> +        pub trait DeviceSize {
> +            $(
> +                #[doc = concat!("`", stringify!($name), "` for this type.")]
> +                const $name: Self;
> +            )*
> +        }
> +
> +        impl DeviceSize for u32 {
> +            $(
> +                const $name: Self = {
> +                    assert!(self::$name <= u32::MAX as usize);
> +                    self::$name as u32
> +                };
> +            )*
> +        }
> +
> +        impl DeviceSize for u64 {
> +            $(
> +                const $name: Self = self::$name as u64;

I know 64-bit is that largest architecture so far, but in order to
protect for an eventual future where larger sizes exist (and for
consistency), let's have the defensive `assert` here as well.

> +            )*
> +        }
> +    };
> +}
>  
> -/// 0x00000400
> -pub const SZ_1K: usize = bindings::SZ_1K as usize;
> -/// 0x00000800
> -pub const SZ_2K: usize = bindings::SZ_2K as usize;
> -/// 0x00001000
> -pub const SZ_4K: usize = bindings::SZ_4K as usize;
> -/// 0x00002000
> -pub const SZ_8K: usize = bindings::SZ_8K as usize;
> -/// 0x00004000
> -pub const SZ_16K: usize = bindings::SZ_16K as usize;
> -/// 0x00008000
> -pub const SZ_32K: usize = bindings::SZ_32K as usize;
> -/// 0x00010000
> -pub const SZ_64K: usize = bindings::SZ_64K as usize;
> -/// 0x00020000
> -pub const SZ_128K: usize = bindings::SZ_128K as usize;
> -/// 0x00040000
> -pub const SZ_256K: usize = bindings::SZ_256K as usize;
> -/// 0x00080000
> -pub const SZ_512K: usize = bindings::SZ_512K as usize;
> -/// 0x00100000
> -pub const SZ_1M: usize = bindings::SZ_1M as usize;
> -/// 0x00200000
> -pub const SZ_2M: usize = bindings::SZ_2M as usize;
> -/// 0x00400000
> -pub const SZ_4M: usize = bindings::SZ_4M as usize;
> -/// 0x00800000
> -pub const SZ_8M: usize = bindings::SZ_8M as usize;
> -/// 0x01000000
> -pub const SZ_16M: usize = bindings::SZ_16M as usize;
> -/// 0x02000000
> -pub const SZ_32M: usize = bindings::SZ_32M as usize;
> -/// 0x04000000
> -pub const SZ_64M: usize = bindings::SZ_64M as usize;
> -/// 0x08000000
> -pub const SZ_128M: usize = bindings::SZ_128M as usize;
> -/// 0x10000000
> -pub const SZ_256M: usize = bindings::SZ_256M as usize;
> -/// 0x20000000
> -pub const SZ_512M: usize = bindings::SZ_512M as usize;
> -/// 0x40000000
> -pub const SZ_1G: usize = bindings::SZ_1G as usize;
> -/// 0x80000000
> -pub const SZ_2G: usize = bindings::SZ_2G as usize;
> +define_sizes! {
> +    SZ_1K,   // 0x0000_0400
> +    SZ_2K,   // 0x0000_0800
> +    SZ_4K,   // 0x0000_1000
> +    SZ_8K,   // 0x0000_2000
> +    SZ_16K,  // 0x0000_4000
> +    SZ_32K,  // 0x0000_8000
> +    SZ_64K,  // 0x0001_0000
> +    SZ_128K, // 0x0002_0000
> +    SZ_256K, // 0x0004_0000
> +    SZ_512K, // 0x0008_0000
> +    SZ_1M,   // 0x0010_0000
> +    SZ_2M,   // 0x0020_0000
> +    SZ_4M,   // 0x0040_0000
> +    SZ_8M,   // 0x0080_0000
> +    SZ_16M,  // 0x0100_0000
> +    SZ_32M,  // 0x0200_0000
> +    SZ_64M,  // 0x0400_0000
> +    SZ_128M, // 0x0800_0000
> +    SZ_256M, // 0x1000_0000
> +    SZ_512M, // 0x2000_0000
> +    SZ_1G,   // 0x4000_0000
> +    SZ_2G,   // 0x8000_0000
> +}

This macro looks like it has its arguments backwards. `SZ_*` is a fixed
list of values from the C headers that is stable and never changes.
OTOH, the types on which we want to implement `DeviceSize` may vary (as
I suggested in patch 3 to also support `usize`). So I think the macro
invocation should be like:

  define_sizes!(usize, u32, u64);

This would make it easier to integrate the doccomment fix I suggested
above. The only drawback is that generating the module-level usize
consts would need an internal macro rule with the SZ_* list, but that
doesn't add much complexity and makes more sense that way IMHO - you
would just be moving the list from the macro call site to the macro
internals.