linux-kernel - Re: [PATCH v2 1/4] nova-core: bitstruct: Move bitfield-specific code from register! into new macro

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3ef9e2c2-560e-4b58-96f8-a6db4236fe0e@nvidia.com>
Date: Mon, 8 Sep 2025 13:16:01 -0400
From: Joel Fernandes <joelagnelf@...dia.com>
To: Alexandre Courbot <acourbot@...dia.com>, linux-kernel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org, dakr@...nel.org
Cc: Alistair Popple <apopple@...dia.com>, Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>,
 Gary Guo <gary@...yguo.net>, 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>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 John Hubbard <jhubbard@...dia.com>, Timur Tabi <ttabi@...dia.com>,
 joel@...lfernandes.org, Elle Rhumsaa <elle@...thered-steel.dev>,
 Daniel Almeida <daniel.almeida@...labora.com>,
 nouveau@...ts.freedesktop.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2 1/4] nova-core: bitstruct: Move bitfield-specific code
 from register! into new macro

Hi Alex,

On 9/7/2025 11:12 PM, Alexandre Courbot wrote:
> On Thu Sep 4, 2025 at 6:54 AM JST, Joel Fernandes wrote:
>> The bitfield-specific into new macro. This will be used to define
>> structs with bitfields, similar to C language.
>>
>> Signed-off-by: Joel Fernandes <joelagnelf@...dia.com>
>> ---
>>  drivers/gpu/nova-core/bitstruct.rs   | 271 +++++++++++++++++++++++++++
>>  drivers/gpu/nova-core/nova_core.rs   |   3 +
>>  drivers/gpu/nova-core/regs/macros.rs | 247 +-----------------------
>>  3 files changed, 282 insertions(+), 239 deletions(-)
>>  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..1dd9edab7d07
>> --- /dev/null
>> +++ b/drivers/gpu/nova-core/bitstruct.rs
>> @@ -0,0 +1,271 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +//
>> +// bitstruct.rs — Bitfield library for Rust structures
>> +//
>> +// A library that provides support for defining bit fields in Rust
>> +// structures. Also used from things that need bitfields like register macro.
>> +///
>> +/// # Syntax
>> +///
>> +/// ```rust
>> +/// bitstruct! {
>> +///     struct ControlReg {
> 
> The `struct` naming here looks a bit confusing to me - as of this patch,
> this is a u32, right? And eventually these types will be limited to primitive types,
> so why not just `ControlReg: u32 {` ?

This is done in a later patch. This patch is only code movement, in later patch
we add precisely the syntax you're describing when we add storage types, and
update the register! macro. In this patch bitstruct is only u32.

> 
>> +///         3:0       mode        as u8 ?=> Mode;
>> +///         7:4       state       as u8 => State;
>> +///     }
>> +/// }
>> +/// ```
> 
> As this will move to the kernel crate, it is particularly important to
> make sure that this example can compile and run - so please provide
> simple definitions for `Mode` and `State` to make sure the kunit tests
> will pass after patch 4 (in the current state I'm pretty sure they won't).

Good catch. This will blow up the example though. I will change it to no_run
like the register! macro did if that's Ok.

> 
>> +///
>> +/// This generates a struct with:
>> +/// - Field accessors: `mode()`, `state()`, etc.
>> +/// - Field setters: `set_mode()`, `set_state()`, etc. (supports builder pattern).
>> +/// - Debug and Default implementations
>> +///
>> +/// The field setters can be used with the builder pattern, example:
>> +/// ControlReg::default().set_mode(mode).set_state(state);
>> +///
>> +/// Fields are defined as follows:
>> +///
>> +/// - `as <type>` simply returns the field value casted to <type>, typically `u32`, `u16`, `u8` or
>> +///   `bool`. Note that `bool` fields must have a range of 1 bit.
>> +/// - `as <type> => <into_type>` calls `<into_type>`'s `From::<<type>>` implementation and returns
>> +///   the result.
>> +/// - `as <type> ?=> <try_into_type>` calls `<try_into_type>`'s `TryFrom::<<type>>` implementation
>> +///   and returns the result. This is useful with fields for which not all values are valid.
> 
> Can you remove the related documentation from `register!` and replace it
> with a sentence like "Please look at the [`bitstruct`] macro for the
> complete syntax of fields definitions"? This will ensure we don't have
> to update the documentation twice if/when the syntax gets updated.

Sure!

> 
> The rest of the patch is a perfect move (with a few renames) of the
> internal rules from one macro to the other, which makes it really easy
> to review. Thanks for doing this!

My pleasure, thanks for the suggestion!

 - Joel