Message-ID: <444ebd64-7a90-46a6-b885-2c114aa59284@nvidia.com>
Date: Wed, 3 Sep 2025 11:15:06 -0400
From: Joel Fernandes <joelagnelf@...dia.com>
To: Alexandre Courbot <acourbot@...dia.com>, linux-kernel@...r.kernel.org,
 Danilo Krummrich <dakr@...nel.org>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>, 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 <lossin@...nel.org>,
 Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>
Cc: John Hubbard <jhubbard@...dia.com>, Alistair Popple <apopple@...dia.com>,
 nouveau@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
 rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 1/2] nova-core: Add a library for bitfields in Rust
 structs

Hi Alex,

On 8/25/2025 7:07 AM, Alexandre Courbot wrote:
> Hi Joel,
> 
> On Sun Aug 24, 2025 at 10:59 PM JST, Joel Fernandes wrote:
>> Add a minimal bitfield library for defining in Rust structures (called
>> bitstruct), similar in concept to bit fields in C structs. This will be used
>> for defining page table entries and other structures in nova-core.
> 
> This is basically a rewrite (with some improvements, and some missing
> features) of the part of the `register!` macro that deals with
> bitfields. We planned to extract it into its own macro, and the split is
> already effective in its internal rules, so I'd suggest to just move the
> relevant rules into a new macro (as it will give you a couple useful
> features, like automatic conversion to enum types), and then apply your
> improvements on top of it. Otherwise we will end up with two
> implementations of the same thing, for no good justification IMHO.
> 
> We were also planning to move the `register!` macro into the kernel
> crate this cycle so Tyr can use it, but this changes the plan a bit.
> Actually it is helpful, since your version implements one thing that we
> needed in the `register!` macro before moving it: visibility specifiers.
> So I would do things in this order:
> 
> 1. Extract the bitfield-related code from the `register!` macro into its
>    own macro (in nova-core), and make `register!` call into it,
> 2. Add support for visibility specifiers and non-u32 types in your macro and
>    `register!`,
> 3. Move both macros to the kernel crate (hopefully in time for the next
>    merge window so Tyr can make use of them).
> 
> A few more comments inline.
> 

Ok, all these sound good to me.

>>
>> Signed-off-by: Joel Fernandes <joelagnelf@...dia.com>
>> ---
>>  drivers/gpu/nova-core/bitstruct.rs | 149 +++++++++++++++++++++++++++++
>>  drivers/gpu/nova-core/nova_core.rs |   1 +
>>  2 files changed, 150 insertions(+)
>>  create mode 100644 drivers/gpu/nova-core/bitstruct.rs
>>
>> diff --git a/drivers/gpu/nova-core/bitstruct.rs b/drivers/gpu/nova-core/bitstruct.rs
>> new file mode 100644
>> index 000000000000..661a75da0a9c
>> --- /dev/null
>> +++ b/drivers/gpu/nova-core/bitstruct.rs
> 
> I wonder whether this should go under the existing `bits.rs`, or be its
> own module?

I'd say its own since it is related to structures and keeps the file smaller.

>> @@ -0,0 +1,149 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +//
>> +// bitstruct.rs — C-style library for bitfield-packed Rust structures
>> +//
>> +// A library that provides support for defining bit fields in Rust
>> +// structures to circumvent lack of native language support for this.
>> +//
>> +// Similar usage syntax to the register! macro.
> 
> Eventually the `register!` macro is the one that should reference this
> (simpler) one, so let's make it the reference. :)

Ah true, already fixed.

> 
>> +
>> +use kernel::prelude::*;
>> +
>> +/// Macro for defining bitfield-packed structures in Rust.
>> +/// The size of the underlying storage type is specified with #[repr(TYPE)].
>> +///
>> +/// # Example (just for illustration)
>> +/// ```rust
>> +/// bitstruct! {
>> +///     #[repr(u64)]
>> +///     pub struct PageTableEntry {
>> +///         0:0       present     as bool,
>> +///         1:1       writable    as bool,
>> +///         11:9      available   as u8,
>> +///         51:12     pfn         as u64,
>> +///         62:52     available2  as u16,
>> +///         63:63     nx          as bool,
> 
> A note on syntax: for nova-core, we may want to use the `H:L` notation,
> as this is what OpenRM uses, but in the larger kernel we might want to
> use inclusive ranges (`L..=H`) as it will look more natural in Rust
> code (and is the notation the `bits` module already uses).

Perhaps future add-on enhancement to have both syntax? I'd like to initially
keep H:L and stabilize the code first, what do you think?

> 
>> +///     }
>> +/// }
>> +/// ```
>> +///
>> +/// This generates a struct with methods:
>> +/// - Constructor: `default()` sets all bits to zero.
>> +/// - Field accessors: `present()`, `pfn()`, etc.
>> +/// - Field setters: `set_present()`, `set_pfn()`, etc.
>> +/// - Builder methods: `with_present()`, `with_pfn()`, etc.
>> +/// - Raw conversion: `from_raw()`, `into_raw()`
>> +#[allow(unused_macros)]
>> +macro_rules! bitstruct {
>> +    (
>> +        #[repr($storage:ty)]
>> +        $vis:vis struct $name:ident {
>> +            $(
>> +                $hi:literal : $lo:literal $field:ident as $field_type:tt
>> +            ),* $(,)?
>> +        }
>> +    ) => {
>> +        #[repr(transparent)]
>> +        #[derive(Copy, Clone, Default)]
>> +        $vis struct $name($storage);
>> +
>> +        impl $name {
>> +            /// Create from raw value
>> +            #[inline(always)]
>> +            $vis const fn from_raw(val: $storage) -> Self {
>> +                Self(val)
>> +            }
>> +
>> +            /// Get raw value
>> +            #[inline(always)]
>> +            $vis const fn into_raw(self) -> $storage {
>> +                self.0
>> +            }
>> +        }
>> +
>> +        impl core::fmt::Debug for $name {
>> +            fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
>> +                write!(f, "{}({:#x})", stringify!($name), self.0)
>> +            }
>> +        }
>> +
>> +        // Generate all field methods
>> +        $(
>> +            bitstruct_field_impl!($vis, $name, $storage, $hi, $lo, $field as $field_type);
> 
> Maybe use internal rules [1] instead of a private macro (that cannot be so
> private :))
> 
> [1] https://lukaswirth.dev/tlborm/decl-macros/patterns/internal-rules.html

Sure, done.

> 
>> +        )*
>> +    };
>> +}
>> +
>> +/// Helper to calculate mask for bit fields
>> +#[allow(unused_macros)]
>> +macro_rules! bitstruct_mask {
>> +    ($hi:literal, $lo:literal, $storage:ty) => {{
>> +        let width = ($hi - $lo + 1) as usize;
>> +        let storage_bits = 8 * core::mem::size_of::<$storage>();
>> +        if width >= storage_bits {
>> +            <$storage>::MAX
>> +        } else {
>> +            ((1 as $storage) << width) - 1
>> +        }
>> +    }};
>> +}
> 
> Is there a way to leverage the `genmask_*` functions of the `bits` module?
> 

Maybe, I'll look into it, thanks.

>> +#[allow(unused_macros)]
>> +macro_rules! bitstruct_field_impl {
>> +    ($vis:vis, $struct_name:ident, $storage:ty, $hi:literal, $lo:literal, $field:ident as $field_type:tt) => {
>> +        impl $struct_name {
>> +            #[inline(always)]
>> +            $vis const fn $field(&self) -> $field_type {
>> +                let field_val = (self.0 >> $lo) & bitstruct_mask!($hi, $lo, $storage);
>> +                bitstruct_cast_value!(field_val, $field_type)
>> +            }
>> +        }
>> +        bitstruct_make_setters!($vis, $struct_name, $storage, $hi, $lo, $field, $field_type);
>> +    };
>> +}
>> +
>> +/// Helper macro to convert extracted value to target type
>> +///
>> +/// Special handling for bool types is required because the `as` keyword
>> +/// cannot be used to convert to bool in Rust. For bool fields, we check
>> +/// if the extracted value is non-zero. For all other types, we use the
>> +/// standard `as` conversion.
>> +#[allow(unused_macros)]
>> +macro_rules! bitstruct_cast_value {
>> +    ($field_val:expr, bool) => {
>> +        $field_val != 0
>> +    };
>> +    ($field_val:expr, $field_type:tt) => {
>> +        $field_val as $field_type
>> +    };
>> +}
>> +
>> +#[allow(unused_macros)]
>> +macro_rules! bitstruct_write_bits {
>> +    ($raw:expr, $hi:literal, $lo:literal, $val:expr, $storage:ty) => {{
>> +        let mask = bitstruct_mask!($hi, $lo, $storage);
>> +        ($raw & !(mask << $lo)) | ((($val as $storage) & mask) << $lo)
>> +    }};
>> +}
>> +
>> +#[allow(unused_macros)]
>> +macro_rules! bitstruct_make_setters {
>> +    ($vis:vis, $struct_name:ident, $storage:ty, $hi:literal, $lo:literal, $field:ident, $field_type:tt) => {
>> +        ::kernel::macros::paste! {
>> +            impl $struct_name {
>> +                #[inline(always)]
>> +                #[allow(dead_code)]
>> +                $vis fn [<set_ $field>](&mut self, val: $field_type) {
>> +                    self.0 = bitstruct_write_bits!(self.0, $hi, $lo, val, $storage);
>> +                }
>> +
>> +                #[inline(always)]
>> +                #[allow(dead_code)]
>> +                $vis const fn [<with_ $field>](mut self, val: $field_type) -> Self {
>> +                    self.0 = bitstruct_write_bits!(self.0, $hi, $lo, val, $storage);
>> +                    self
>> +                }
>> +            }
>> +        }
>> +    };
>> +}
> 
> Yep, I think you definitely want to put these into internal rules - see
> how `register!` does it, actually it should be easy to extract these
> rules only and implement your improvements on top.
Ack, done.
 - Joel

Powered by blists - more mailing lists