Message-Id: <DCBG0345JTPY.3A6MSWZU7AZE5@nvidia.com>
Date: Mon, 25 Aug 2025 19:39:15 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Christian S. Lima" <christiansantoslima21@...il.com>, "Miguel Ojeda"
 <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>, "Boqun Feng"
 <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <benno.lossin@...ton.me>, "Andreas Hindborg" <a.hindborg@...nel.org>,
 "Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
 "Danilo Krummrich" <dakr@...nel.org>, <rust-for-linux@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, <~lkcamp/patches@...ts.sr.ht>,
 <richard120310@...il.com>
Subject: Re: [PATCH v10] rust: transmute: Add methods for FromBytes trait

On Mon Aug 25, 2025 at 6:31 AM JST, Christian S. Lima wrote:
> The two methods added take a slice of bytes and return those bytes in
> a specific type. These methods are useful when we need to transform
> the stream of bytes into specific type.
>
> Since the `is_aligned` method for pointer types has been stabilized in
> `1.79` version and is being used in this patch. I'm enabling the
> feature. In this case, using this method is useful to check the
> alignment and avoid a giant boilerplate, such as `(foo.as_ptr() as
> usize) % core::mem::align_of::<T>() == 0`.
>
> Even enabling in `rust/kernel/lib.rs` when compiling with `make LLVM=1
> CLIPPY=1` a warning is issued, so in order to compile, it was used
> locally the `#[allow(clippy::incompatible_msrv)]`.
>
> Link: https://github.com/Rust-for-Linux/linux/issues/1119
> Suggested-by: Alexandre Courbot <acourbot@...dia.com>
> Signed-off-by: Christian S. Lima <christiansantoslima21@...il.com>

Successfully tested with nova-core, thanks! This is an essential support
piece for the next steps in bringing up our driver.

Tested-by: Alexandre Courbot <acourbot@...dia.com>
Reviewed-by: Alexandre Courbot <acourbot@...dia.com>

If that works with everybody, I would like to take this into the Nova
tree (after a few more days of review) as we will be using it this
cycle. Miguel, is that ok with you?

Minor comments below, which we can address while merging so no need to
send a new version just for these.

> ---
> Changes in v2:
> - Rollback the implementation for the macro in the repository and implement
>   methods in trait
> - Link to v2: https://lore.kernel.org/rust-for-linux/20241012070121.110481-1-christiansantoslima21@gmail.com/
>
> Changes in v3:
> - Fix grammar errors
> - Remove repeated tests
> - Fix alignment errors
> - Fix tests not building
> - Link to v3: https://lore.kernel.org/rust-for-linux/20241109055442.85190-1-christiansantoslima21@gmail.com/
>
> Changes in v4:
> - Removed core::simd::ToBytes
> - Changed trait and methods to safe Add
> - Result<&Self, Error> in order to make safe methods
> - Link to v4: https://lore.kernel.org/rust-for-linux/20250314034910.134463-1-christiansantoslima21@gmail.com/
>
> Changes in v5:
> - Changed from Result to Option
> - Removed commentaries
> - Returned trait impl to unsafe
> - Link to v5: https://lore.kernel.org/rust-for-linux/20250320014041.101470-1-christiansantoslima21@gmail.com/
>
> Changes in v6:
> - Add endianess check to doc test and use match to check
> success case
> - Reformulated safety comments
> - Link to v6: https://lore.kernel.org/rust-for-linux/20250330234039.29814-1-christiansantoslima21@gmail.com/
>
> Changes in v7:
> - Add alignment check
> - Link to v7: https://lore.kernel.org/rust-for-linux/20250615072042.133290-1-christiansantoslima21@gmail.com/
>
> Changes in v8:
> - Add the new FromBytesSized trait
> - Change the implementation of FromBytes trait methods
> - Move the cast to pointer earlier and use `is_aligned()` instead manual
> alignment check
> - Link to v8: https://lore.kernel.org/rust-for-linux/20250624042802.105623-1-christiansantoslima21@gmail.com/
>
> Changes in v9:
> - Improve code comments and remove confusing parts.
> - Add a build_assert in the conversion of type `[T]` to check for elements
> inside the slice.
> - Count the elements in the `[T]` conversion instead of using byte
> count.
> - Link to v9: https://lore.kernel.org/rust-for-linux/20250811213851.65644-1-christiansantoslima21@gmail.com/#t
>
> Changes in v10:
> - Remove `FromBytesSized` trait
> - Remove implementation for slice types
> - Fix doctest not compiling because `?` operator outside a function
> that return `Option<()>`
> - Make `FromBytes` trait depend on `Sized`
> - Add `is_aligned` as feature
> ---
>  rust/kernel/lib.rs       |  1 +
>  rust/kernel/transmute.rs | 69 ++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 68 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index ed53169e795c..c859a8984bae 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -18,6 +18,7 @@
>  //
>  // Stable since Rust 1.79.0.
>  #![feature(inline_const)]
> +#![feature(pointer_is_aligned)]
>  //
>  // Stable since Rust 1.81.0.
>  #![feature(lint_reasons)]
> diff --git a/rust/kernel/transmute.rs b/rust/kernel/transmute.rs
> index 1c7d43771a37..7493b84b5474 100644
> --- a/rust/kernel/transmute.rs
> +++ b/rust/kernel/transmute.rs
> @@ -2,6 +2,8 @@
>  
>  //! Traits for transmuting types.
>  
> +use core::mem::size_of;
> +
>  /// Types for which any bit pattern is valid.
>  ///
>  /// Not all types are valid for all values. For example, a `bool` must be either zero or one, so
> @@ -9,10 +11,74 @@
>  ///
>  /// It's okay for the type to have padding, as initializing those bytes has no effect.
>  ///
> +/// # Examples
> +///
> +/// ```
> +/// use kernel::transmute::FromBytes;
> +///
> +/// fn test() -> Option<()> {
> +///    let raw = [1, 2, 3, 4];
> +///
> +///    let result = u32::from_bytes(&raw)?;
> +///
> +///    #[cfg(target_endian = "little")]
> +///    assert_eq!(*result, 0x4030201);
> +///
> +///    #[cfg(target_endian = "big")]
> +///    assert_eq!(*result, 0x1020304);
> +///
> +///    Some(())
> +/// }
> +/// ```
> +///
>  /// # Safety
>  ///
>  /// All bit-patterns must be valid for this type. This type must not have interior mutability.
> -pub unsafe trait FromBytes {}
> +pub unsafe trait FromBytes {
> +    /// Converts a slice of bytes to a reference to `Self`.
> +    ///
> +    /// When the reference is properly aligned and the size of slice is equal to that of `T`
> +    /// and is different from zero.
> +    ///
> +    /// In another case, it will return `None`.

I'd replace "When" by "If" and "In another case" with "Otherwise" to
sound more natural.

> +    #[allow(clippy::incompatible_msrv)]
> +    fn from_bytes(bytes: &[u8]) -> Option<&Self>
> +    where
> +        Self: Sized,
> +    {
> +        let slice_ptr = bytes.as_ptr().cast::<Self>();
> +        let size = size_of::<Self>();
> +        if bytes.len() == size && slice_ptr.is_aligned() {
> +            // SAFETY: Checking for size and alignment ensure
> +            // that the conversion to a type is valid
> +            unsafe { Some(&*slice_ptr) }
> +        } else {
> +            None
> +        }
> +    }
> +
> +    /// Converts a mutable slice of bytes to a reference to `Self`
> +    ///
> +    /// When the reference is properly aligned and the size of slice
> +    /// is equal to that of `T`and is different from zero.
> +    ///
> +    /// In another case, it will return `None`.
> +    #[allow(clippy::incompatible_msrv)]
> +    fn from_bytes_mut(bytes: &mut [u8]) -> Option<&mut Self>
> +    where
> +        Self: AsBytes + Sized,
> +    {
> +        let slice_ptr = bytes.as_mut_ptr().cast::<Self>();
> +        let size = size_of::<Self>();
> +        if bytes.len() == size && slice_ptr.is_aligned() {
> +            // SAFETY: Checking for size and alignment ensure
> +            // that the conversion to a type is valid
> +            unsafe { Some(&mut *slice_ptr) }
> +        } else {
> +            None
> +        }
> +    }
> +}
>  
>  macro_rules! impl_frombytes {
>      ($($({$($generics:tt)*})? $t:ty, )*) => {
> @@ -28,7 +94,6 @@ macro_rules! impl_frombytes {
>  
>      // SAFETY: If all bit patterns are acceptable for individual values in an array, then all bit
>      // patterns are also acceptable for arrays of that type.
> -    {<T: FromBytes>} [T],

As Alice pointed out, there should be no need to remove this - I have
kept it and things build just fine.

Powered by blists - more mailing lists