* [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-20 18:22 [PATCH v4 0/6] Introduce bitfield and move register macro to rust/kernel/ Joel Fernandes
@ 2025-09-20 18:22 ` Joel Fernandes
2025-09-21 9:36 ` Greg KH
2025-09-29 6:16 ` Alexandre Courbot
2025-09-20 18:22 ` [PATCH v4 2/6] nova-core: bitfield: Add support for different storage widths Joel Fernandes
` (4 subsequent siblings)
5 siblings, 2 replies; 44+ messages in thread
From: Joel Fernandes @ 2025-09-20 18:22 UTC (permalink / raw)
To: linux-kernel, rust-for-linux, dri-devel, dakr, acourbot
Cc: Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Joel Fernandes,
Timur Tabi, joel, Elle Rhumsaa, Yury Norov, Daniel Almeida,
nouveau
The bitfield-specific into new macro. This will be used to define
structs with bitfields, similar to C language.
Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
drivers/gpu/nova-core/bitfield.rs | 314 +++++++++++++++++++++++++++
drivers/gpu/nova-core/nova_core.rs | 3 +
drivers/gpu/nova-core/regs/macros.rs | 259 +---------------------
3 files changed, 327 insertions(+), 249 deletions(-)
create mode 100644 drivers/gpu/nova-core/bitfield.rs
diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs
new file mode 100644
index 000000000000..ba6b7caa05d9
--- /dev/null
+++ b/drivers/gpu/nova-core/bitfield.rs
@@ -0,0 +1,314 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Bitfield library for Rust structures
+//!
+//! Support for defining bitfields in Rust structures. Also used by the [`register!`] macro.
+//!
+//! # Syntax
+//!
+//! ```rust
+//! #[derive(Debug, Clone, Copy)]
+//! enum Mode {
+//! Low = 0,
+//! High = 1,
+//! Auto = 2,
+//! }
+//!
+//! impl TryFrom<u8> for Mode {
+//! type Error = u8;
+//! fn try_from(value: u8) -> Result<Self, Self::Error> {
+//! match value {
+//! 0 => Ok(Mode::Low),
+//! 1 => Ok(Mode::High),
+//! 2 => Ok(Mode::Auto),
+//! _ => Err(value),
+//! }
+//! }
+//! }
+//!
+//! impl From<Mode> for u32 {
+//! fn from(mode: Mode) -> u32 {
+//! mode as u32
+//! }
+//! }
+//!
+//! #[derive(Debug, Clone, Copy)]
+//! enum State {
+//! Inactive = 0,
+//! Active = 1,
+//! }
+//!
+//! impl From<bool> for State {
+//! fn from(value: bool) -> Self {
+//! if value { State::Active } else { State::Inactive }
+//! }
+//! }
+//!
+//! impl From<State> for u32 {
+//! fn from(state: State) -> u32 {
+//! state as u32
+//! }
+//! }
+//!
+//! bitfield! {
+//! struct ControlReg {
+//! 3:0 mode as u8 ?=> Mode;
+//! 7 state as bool => State;
+//! }
+//! }
+//! ```
+//!
+//! This generates a struct with:
+//! - Field accessors: `mode()`, `state()`, etc.
+//! - Field setters: `set_mode()`, `set_state()`, etc. (supports chaining with 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.
+//!
+macro_rules! bitfield {
+ // Main entry point - defines the bitfield struct with fields
+ (struct $name:ident $(, $comment:literal)? { $($fields:tt)* }) => {
+ bitfield!(@core $name $(, $comment)? { $($fields)* });
+ };
+
+ // All rules below are helpers.
+
+ // Defines the wrapper `$name` type, as well as its relevant implementations (`Debug`,
+ // `Default`, `BitOr`, and conversion to the value type) and field accessor methods.
+ (@core $name:ident $(, $comment:literal)? { $($fields:tt)* }) => {
+ $(
+ #[doc=$comment]
+ )?
+ #[repr(transparent)]
+ #[derive(Clone, Copy)]
+ pub(crate) struct $name(u32);
+
+ impl ::core::ops::BitOr for $name {
+ type Output = Self;
+
+ fn bitor(self, rhs: Self) -> Self::Output {
+ Self(self.0 | rhs.0)
+ }
+ }
+
+ impl ::core::convert::From<$name> for u32 {
+ fn from(val: $name) -> u32 {
+ val.0
+ }
+ }
+
+ bitfield!(@fields_dispatcher $name { $($fields)* });
+ };
+
+ // Captures the fields and passes them to all the implementers that require field information.
+ //
+ // Used to simplify the matching rules for implementers, so they don't need to match the entire
+ // complex fields rule even though they only make use of part of it.
+ (@fields_dispatcher $name:ident {
+ $($hi:tt:$lo:tt $field:ident as $type:tt
+ $(?=> $try_into_type:ty)?
+ $(=> $into_type:ty)?
+ $(, $comment:literal)?
+ ;
+ )*
+ }
+ ) => {
+ bitfield!(@field_accessors $name {
+ $(
+ $hi:$lo $field as $type
+ $(?=> $try_into_type)?
+ $(=> $into_type)?
+ $(, $comment)?
+ ;
+ )*
+ });
+ bitfield!(@debug $name { $($field;)* });
+ bitfield!(@default $name { $($field;)* });
+ };
+
+ // Defines all the field getter/setter methods for `$name`.
+ (
+ @field_accessors $name:ident {
+ $($hi:tt:$lo:tt $field:ident as $type:tt
+ $(?=> $try_into_type:ty)?
+ $(=> $into_type:ty)?
+ $(, $comment:literal)?
+ ;
+ )*
+ }
+ ) => {
+ $(
+ bitfield!(@check_field_bounds $hi:$lo $field as $type);
+ )*
+
+ #[allow(dead_code)]
+ impl $name {
+ $(
+ bitfield!(@field_accessor $name $hi:$lo $field as $type
+ $(?=> $try_into_type)?
+ $(=> $into_type)?
+ $(, $comment)?
+ ;
+ );
+ )*
+ }
+ };
+
+ // Boolean fields must have `$hi == $lo`.
+ (@check_field_bounds $hi:tt:$lo:tt $field:ident as bool) => {
+ #[allow(clippy::eq_op)]
+ const _: () = {
+ ::kernel::build_assert!(
+ $hi == $lo,
+ concat!("boolean field `", stringify!($field), "` covers more than one bit")
+ );
+ };
+ };
+
+ // Non-boolean fields must have `$hi >= $lo`.
+ (@check_field_bounds $hi:tt:$lo:tt $field:ident as $type:tt) => {
+ #[allow(clippy::eq_op)]
+ const _: () = {
+ ::kernel::build_assert!(
+ $hi >= $lo,
+ concat!("field `", stringify!($field), "`'s MSB is smaller than its LSB")
+ );
+ };
+ };
+
+ // Catches fields defined as `bool` and convert them into a boolean value.
+ (
+ @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as bool => $into_type:ty
+ $(, $comment:literal)?;
+ ) => {
+ bitfield!(
+ @leaf_accessor $name $hi:$lo $field
+ { |f| <$into_type>::from(if f != 0 { true } else { false }) }
+ $into_type => $into_type $(, $comment)?;
+ );
+ };
+
+ // Shortcut for fields defined as `bool` without the `=>` syntax.
+ (
+ @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as bool $(, $comment:literal)?;
+ ) => {
+ bitfield!(@field_accessor $name $hi:$lo $field as bool => bool $(, $comment)?;);
+ };
+
+ // Catches the `?=>` syntax for non-boolean fields.
+ (
+ @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt ?=> $try_into_type:ty
+ $(, $comment:literal)?;
+ ) => {
+ bitfield!(@leaf_accessor $name $hi:$lo $field
+ { |f| <$try_into_type>::try_from(f as $type) } $try_into_type =>
+ ::core::result::Result<
+ $try_into_type,
+ <$try_into_type as ::core::convert::TryFrom<$type>>::Error
+ >
+ $(, $comment)?;);
+ };
+
+ // Catches the `=>` syntax for non-boolean fields.
+ (
+ @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt => $into_type:ty
+ $(, $comment:literal)?;
+ ) => {
+ bitfield!(@leaf_accessor $name $hi:$lo $field
+ { |f| <$into_type>::from(f as $type) } $into_type => $into_type $(, $comment)?;);
+ };
+
+ // Shortcut for non-boolean fields defined without the `=>` or `?=>` syntax.
+ (
+ @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt
+ $(, $comment:literal)?;
+ ) => {
+ bitfield!(@field_accessor $name $hi:$lo $field as $type => $type $(, $comment)?;);
+ };
+
+ // Generates the accessor methods for a single field.
+ (
+ @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident
+ { $process:expr } $to_type:ty => $res_type:ty $(, $comment:literal)?;
+ ) => {
+ ::kernel::macros::paste!(
+ const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
+ const [<$field:upper _MASK>]: u32 = ((((1 << $hi) - 1) << 1) + 1) - ((1 << $lo) - 1);
+ const [<$field:upper _SHIFT>]: u32 = Self::[<$field:upper _MASK>].trailing_zeros();
+ );
+
+ $(
+ #[doc="Returns the value of this field:"]
+ #[doc=$comment]
+ )?
+ #[inline(always)]
+ pub(crate) fn $field(self) -> $res_type {
+ ::kernel::macros::paste!(
+ const MASK: u32 = $name::[<$field:upper _MASK>];
+ const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
+ );
+ let field = ((self.0 & MASK) >> SHIFT);
+
+ $process(field)
+ }
+
+ ::kernel::macros::paste!(
+ $(
+ #[doc="Sets the value of this field:"]
+ #[doc=$comment]
+ )?
+ #[inline(always)]
+ pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
+ const MASK: u32 = $name::[<$field:upper _MASK>];
+ const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
+ let value = (u32::from(value) << SHIFT) & MASK;
+ self.0 = (self.0 & !MASK) | value;
+
+ self
+ }
+ );
+ };
+
+ // Generates the `Debug` implementation for `$name`.
+ (@debug $name:ident { $($field:ident;)* }) => {
+ impl ::core::fmt::Debug for $name {
+ fn fmt(&self, f: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result {
+ f.debug_struct(stringify!($name))
+ .field("<raw>", &format_args!("{:#x}", &self.0))
+ $(
+ .field(stringify!($field), &self.$field())
+ )*
+ .finish()
+ }
+ }
+ };
+
+ // Generates the `Default` implementation for `$name`.
+ (@default $name:ident { $($field:ident;)* }) => {
+ /// Returns a value for the bitfield where all fields are set to their default value.
+ impl ::core::default::Default for $name {
+ fn default() -> Self {
+ #[allow(unused_mut)]
+ let mut value = Self(Default::default());
+
+ ::kernel::macros::paste!(
+ $(
+ value.[<set_ $field>](Default::default());
+ )*
+ );
+
+ value
+ }
+ }
+ };
+}
diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
index fffcaee2249f..112277c7921e 100644
--- a/drivers/gpu/nova-core/nova_core.rs
+++ b/drivers/gpu/nova-core/nova_core.rs
@@ -2,6 +2,9 @@
//! Nova Core GPU Driver
+#[macro_use]
+mod bitfield;
+
mod dma;
mod driver;
mod falcon;
diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
index 754c14ee7f40..945d15a2c529 100644
--- a/drivers/gpu/nova-core/regs/macros.rs
+++ b/drivers/gpu/nova-core/regs/macros.rs
@@ -8,7 +8,8 @@
//!
//! The `register!` macro in this module provides an intuitive and readable syntax for defining a
//! dedicated type for each register. Each such type comes with its own field accessors that can
-//! return an error if a field's value is invalid.
+//! return an error if a field's value is invalid. Please look at the [`bitfield`] macro for the
+//! complete syntax of fields definitions.
/// Trait providing a base address to be added to the offset of a relative register to obtain
/// its actual offset.
@@ -54,15 +55,6 @@ pub(crate) trait RegisterBase<T> {
/// BOOT_0::alter(&bar, |r| r.set_major_revision(3).set_minor_revision(10));
/// ```
///
-/// 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.
-///
/// The documentation strings are optional. If present, they will be added to the type's
/// definition, or the field getter and setter methods they are attached to.
///
@@ -284,25 +276,25 @@ pub(crate) trait RegisterBase<T> {
macro_rules! register {
// Creates a register at a fixed offset of the MMIO space.
($name:ident @ $offset:literal $(, $comment:literal)? { $($fields:tt)* } ) => {
- register!(@core $name $(, $comment)? { $($fields)* } );
+ bitfield!(struct $name $(, $comment)? { $($fields)* } );
register!(@io_fixed $name @ $offset);
};
// Creates an alias register of fixed offset register `alias` with its own fields.
($name:ident => $alias:ident $(, $comment:literal)? { $($fields:tt)* } ) => {
- register!(@core $name $(, $comment)? { $($fields)* } );
+ bitfield!(struct $name $(, $comment)? { $($fields)* } );
register!(@io_fixed $name @ $alias::OFFSET);
};
// Creates a register at a relative offset from a base address provider.
($name:ident @ $base:ty [ $offset:literal ] $(, $comment:literal)? { $($fields:tt)* } ) => {
- register!(@core $name $(, $comment)? { $($fields)* } );
+ bitfield!(struct $name $(, $comment)? { $($fields)* } );
register!(@io_relative $name @ $base [ $offset ]);
};
// Creates an alias register of relative offset register `alias` with its own fields.
($name:ident => $base:ty [ $alias:ident ] $(, $comment:literal)? { $($fields:tt)* }) => {
- register!(@core $name $(, $comment)? { $($fields)* } );
+ bitfield!(struct $name $(, $comment)? { $($fields)* } );
register!(@io_relative $name @ $base [ $alias::OFFSET ]);
};
@@ -313,7 +305,7 @@ macro_rules! register {
}
) => {
static_assert!(::core::mem::size_of::<u32>() <= $stride);
- register!(@core $name $(, $comment)? { $($fields)* } );
+ bitfield!(struct $name $(, $comment)? { $($fields)* } );
register!(@io_array $name @ $offset [ $size ; $stride ]);
};
@@ -334,7 +326,7 @@ macro_rules! register {
$(, $comment:literal)? { $($fields:tt)* }
) => {
static_assert!(::core::mem::size_of::<u32>() <= $stride);
- register!(@core $name $(, $comment)? { $($fields)* } );
+ bitfield!(struct $name $(, $comment)? { $($fields)* } );
register!(@io_relative_array $name @ $base [ $offset [ $size ; $stride ] ]);
};
@@ -356,7 +348,7 @@ macro_rules! register {
}
) => {
static_assert!($idx < $alias::SIZE);
- register!(@core $name $(, $comment)? { $($fields)* } );
+ bitfield!(struct $name $(, $comment)? { $($fields)* } );
register!(@io_relative $name @ $base [ $alias::OFFSET + $idx * $alias::STRIDE ] );
};
@@ -365,241 +357,10 @@ macro_rules! register {
// to avoid it being interpreted in place of the relative register array alias rule.
($name:ident => $alias:ident [ $idx:expr ] $(, $comment:literal)? { $($fields:tt)* }) => {
static_assert!($idx < $alias::SIZE);
- register!(@core $name $(, $comment)? { $($fields)* } );
+ bitfield!(struct $name $(, $comment)? { $($fields)* } );
register!(@io_fixed $name @ $alias::OFFSET + $idx * $alias::STRIDE );
};
- // All rules below are helpers.
-
- // Defines the wrapper `$name` type, as well as its relevant implementations (`Debug`,
- // `Default`, `BitOr`, and conversion to the value type) and field accessor methods.
- (@core $name:ident $(, $comment:literal)? { $($fields:tt)* }) => {
- $(
- #[doc=$comment]
- )?
- #[repr(transparent)]
- #[derive(Clone, Copy)]
- pub(crate) struct $name(u32);
-
- impl ::core::ops::BitOr for $name {
- type Output = Self;
-
- fn bitor(self, rhs: Self) -> Self::Output {
- Self(self.0 | rhs.0)
- }
- }
-
- impl ::core::convert::From<$name> for u32 {
- fn from(reg: $name) -> u32 {
- reg.0
- }
- }
-
- register!(@fields_dispatcher $name { $($fields)* });
- };
-
- // Captures the fields and passes them to all the implementers that require field information.
- //
- // Used to simplify the matching rules for implementers, so they don't need to match the entire
- // complex fields rule even though they only make use of part of it.
- (@fields_dispatcher $name:ident {
- $($hi:tt:$lo:tt $field:ident as $type:tt
- $(?=> $try_into_type:ty)?
- $(=> $into_type:ty)?
- $(, $comment:literal)?
- ;
- )*
- }
- ) => {
- register!(@field_accessors $name {
- $(
- $hi:$lo $field as $type
- $(?=> $try_into_type)?
- $(=> $into_type)?
- $(, $comment)?
- ;
- )*
- });
- register!(@debug $name { $($field;)* });
- register!(@default $name { $($field;)* });
- };
-
- // Defines all the field getter/methods methods for `$name`.
- (
- @field_accessors $name:ident {
- $($hi:tt:$lo:tt $field:ident as $type:tt
- $(?=> $try_into_type:ty)?
- $(=> $into_type:ty)?
- $(, $comment:literal)?
- ;
- )*
- }
- ) => {
- $(
- register!(@check_field_bounds $hi:$lo $field as $type);
- )*
-
- #[allow(dead_code)]
- impl $name {
- $(
- register!(@field_accessor $name $hi:$lo $field as $type
- $(?=> $try_into_type)?
- $(=> $into_type)?
- $(, $comment)?
- ;
- );
- )*
- }
- };
-
- // Boolean fields must have `$hi == $lo`.
- (@check_field_bounds $hi:tt:$lo:tt $field:ident as bool) => {
- #[allow(clippy::eq_op)]
- const _: () = {
- ::kernel::build_assert!(
- $hi == $lo,
- concat!("boolean field `", stringify!($field), "` covers more than one bit")
- );
- };
- };
-
- // Non-boolean fields must have `$hi >= $lo`.
- (@check_field_bounds $hi:tt:$lo:tt $field:ident as $type:tt) => {
- #[allow(clippy::eq_op)]
- const _: () = {
- ::kernel::build_assert!(
- $hi >= $lo,
- concat!("field `", stringify!($field), "`'s MSB is smaller than its LSB")
- );
- };
- };
-
- // Catches fields defined as `bool` and convert them into a boolean value.
- (
- @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as bool => $into_type:ty
- $(, $comment:literal)?;
- ) => {
- register!(
- @leaf_accessor $name $hi:$lo $field
- { |f| <$into_type>::from(if f != 0 { true } else { false }) }
- $into_type => $into_type $(, $comment)?;
- );
- };
-
- // Shortcut for fields defined as `bool` without the `=>` syntax.
- (
- @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as bool $(, $comment:literal)?;
- ) => {
- register!(@field_accessor $name $hi:$lo $field as bool => bool $(, $comment)?;);
- };
-
- // Catches the `?=>` syntax for non-boolean fields.
- (
- @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt ?=> $try_into_type:ty
- $(, $comment:literal)?;
- ) => {
- register!(@leaf_accessor $name $hi:$lo $field
- { |f| <$try_into_type>::try_from(f as $type) } $try_into_type =>
- ::core::result::Result<
- $try_into_type,
- <$try_into_type as ::core::convert::TryFrom<$type>>::Error
- >
- $(, $comment)?;);
- };
-
- // Catches the `=>` syntax for non-boolean fields.
- (
- @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt => $into_type:ty
- $(, $comment:literal)?;
- ) => {
- register!(@leaf_accessor $name $hi:$lo $field
- { |f| <$into_type>::from(f as $type) } $into_type => $into_type $(, $comment)?;);
- };
-
- // Shortcut for non-boolean fields defined without the `=>` or `?=>` syntax.
- (
- @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt
- $(, $comment:literal)?;
- ) => {
- register!(@field_accessor $name $hi:$lo $field as $type => $type $(, $comment)?;);
- };
-
- // Generates the accessor methods for a single field.
- (
- @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident
- { $process:expr } $to_type:ty => $res_type:ty $(, $comment:literal)?;
- ) => {
- ::kernel::macros::paste!(
- const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
- const [<$field:upper _MASK>]: u32 = ((((1 << $hi) - 1) << 1) + 1) - ((1 << $lo) - 1);
- const [<$field:upper _SHIFT>]: u32 = Self::[<$field:upper _MASK>].trailing_zeros();
- );
-
- $(
- #[doc="Returns the value of this field:"]
- #[doc=$comment]
- )?
- #[inline(always)]
- pub(crate) fn $field(self) -> $res_type {
- ::kernel::macros::paste!(
- const MASK: u32 = $name::[<$field:upper _MASK>];
- const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
- );
- let field = ((self.0 & MASK) >> SHIFT);
-
- $process(field)
- }
-
- ::kernel::macros::paste!(
- $(
- #[doc="Sets the value of this field:"]
- #[doc=$comment]
- )?
- #[inline(always)]
- pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
- const MASK: u32 = $name::[<$field:upper _MASK>];
- const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
- let value = (u32::from(value) << SHIFT) & MASK;
- self.0 = (self.0 & !MASK) | value;
-
- self
- }
- );
- };
-
- // Generates the `Debug` implementation for `$name`.
- (@debug $name:ident { $($field:ident;)* }) => {
- impl ::core::fmt::Debug for $name {
- fn fmt(&self, f: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result {
- f.debug_struct(stringify!($name))
- .field("<raw>", &format_args!("{:#x}", &self.0))
- $(
- .field(stringify!($field), &self.$field())
- )*
- .finish()
- }
- }
- };
-
- // Generates the `Default` implementation for `$name`.
- (@default $name:ident { $($field:ident;)* }) => {
- /// Returns a value for the register where all fields are set to their default value.
- impl ::core::default::Default for $name {
- fn default() -> Self {
- #[allow(unused_mut)]
- let mut value = Self(Default::default());
-
- ::kernel::macros::paste!(
- $(
- value.[<set_ $field>](Default::default());
- )*
- );
-
- value
- }
- }
- };
-
// Generates the IO accessors for a fixed offset register.
(@io_fixed $name:ident @ $offset:expr) => {
#[allow(dead_code)]
--
2.34.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-20 18:22 ` [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro Joel Fernandes
@ 2025-09-21 9:36 ` Greg KH
2025-09-21 9:59 ` Miguel Ojeda
` (2 more replies)
2025-09-29 6:16 ` Alexandre Courbot
1 sibling, 3 replies; 44+ messages in thread
From: Greg KH @ 2025-09-21 9:36 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, rust-for-linux, dri-devel, dakr, acourbot,
Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On Sat, Sep 20, 2025 at 02:22:27PM -0400, Joel Fernandes wrote:
> The bitfield-specific into new macro. This will be used to define
> structs with bitfields, similar to C language.
>
> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> ---
> drivers/gpu/nova-core/bitfield.rs | 314 +++++++++++++++++++++++++++
> drivers/gpu/nova-core/nova_core.rs | 3 +
> drivers/gpu/nova-core/regs/macros.rs | 259 +---------------------
> 3 files changed, 327 insertions(+), 249 deletions(-)
> create mode 100644 drivers/gpu/nova-core/bitfield.rs
>
> diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs
> new file mode 100644
> index 000000000000..ba6b7caa05d9
> --- /dev/null
> +++ b/drivers/gpu/nova-core/bitfield.rs
> @@ -0,0 +1,314 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Bitfield library for Rust structures
> +//!
> +//! Support for defining bitfields in Rust structures. Also used by the [`register!`] macro.
> +//!
> +//! # Syntax
> +//!
> +//! ```rust
> +//! #[derive(Debug, Clone, Copy)]
> +//! enum Mode {
> +//! Low = 0,
> +//! High = 1,
> +//! Auto = 2,
> +//! }
> +//!
> +//! impl TryFrom<u8> for Mode {
> +//! type Error = u8;
> +//! fn try_from(value: u8) -> Result<Self, Self::Error> {
> +//! match value {
> +//! 0 => Ok(Mode::Low),
> +//! 1 => Ok(Mode::High),
> +//! 2 => Ok(Mode::Auto),
> +//! _ => Err(value),
> +//! }
> +//! }
> +//! }
> +//!
> +//! impl From<Mode> for u32 {
> +//! fn from(mode: Mode) -> u32 {
> +//! mode as u32
> +//! }
> +//! }
> +//!
> +//! #[derive(Debug, Clone, Copy)]
> +//! enum State {
> +//! Inactive = 0,
> +//! Active = 1,
> +//! }
> +//!
> +//! impl From<bool> for State {
> +//! fn from(value: bool) -> Self {
> +//! if value { State::Active } else { State::Inactive }
> +//! }
> +//! }
> +//!
> +//! impl From<State> for u32 {
> +//! fn from(state: State) -> u32 {
> +//! state as u32
> +//! }
> +//! }
> +//!
> +//! bitfield! {
> +//! struct ControlReg {
> +//! 3:0 mode as u8 ?=> Mode;
> +//! 7 state as bool => State;
> +//! }
> +//! }
As discussed at the conference this week, I do object to this as it
will allow the same mistakes to happen that we used to do in the kernel
for a long time before the regmap() api happened, along with GENMASK().
The issue is that you are going to want to take these bitfields as part
of a larger structure, and attempt to "lay it over" a chunk of memory
that came from, or is going to, hardware. When that happens, all of the
endian issues of mis-matches between hardware and cpus come into play,
which is not able to be properly expressed here at all, unless you
attempt to either resolve it all later on in something like the regmap
api, or you have #ifdef stuff to attempt to capture all of the possible
combinations and deal with it at build time (which is strongly never
recommended, but is what we used to do in previous decades.)
Your example code using this is nice, and it shows how to set up, and
query these bits, but that's not anything anyone actually does in the
kernel, what they want to do is read/write from hardware with this.
So, how does that work? Where does this "drop down" to the native
bus/memory transactions and swizzle the bits properly to work correctly?
And where does this allow us to define things like BIT(2) for values?
(ok, that's kind of not the point of this patch series, but it will come
up over time...)
Ideally, this would map to our existing regmap api, which does handle
all of this properly, but I know that's not usually used by PCI drivers
like where this code is coming from, as they "just assume" endian
formats are all little and can get away with it due to the limited
nature of different hardware types for their hardware.
Also, a larger meta-comment, why doesn't rust have bit types? Why does
everyone either have to roll their own or rely on an external crate? Is
anyone working to provide native bit support to the language? I'm sure
the embedded people would love it as I imagine it's what they reach for
first when using the language on their hardware.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-21 9:36 ` Greg KH
@ 2025-09-21 9:59 ` Miguel Ojeda
2025-09-21 11:23 ` Greg KH
2025-09-21 12:33 ` Benno Lossin
2025-09-21 13:49 ` Danilo Krummrich
2 siblings, 1 reply; 44+ messages in thread
From: Miguel Ojeda @ 2025-09-21 9:59 UTC (permalink / raw)
To: Greg KH
Cc: Joel Fernandes, linux-kernel, rust-for-linux, dri-devel, dakr,
acourbot, Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On Sun, Sep 21, 2025 at 11:36 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> And where does this allow us to define things like BIT(2) for values?
> (ok, that's kind of not the point of this patch series, but it will come
> up over time...)
We have the `bits` module since 6.17:
https://rust.docs.kernel.org/kernel/bits/index.html
(Or do you mean something else?)
Cheers,
Miguel
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-21 9:59 ` Miguel Ojeda
@ 2025-09-21 11:23 ` Greg KH
0 siblings, 0 replies; 44+ messages in thread
From: Greg KH @ 2025-09-21 11:23 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Joel Fernandes, linux-kernel, rust-for-linux, dri-devel, dakr,
acourbot, Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On Sun, Sep 21, 2025 at 11:59:05AM +0200, Miguel Ojeda wrote:
> On Sun, Sep 21, 2025 at 11:36 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > And where does this allow us to define things like BIT(2) for values?
> > (ok, that's kind of not the point of this patch series, but it will come
> > up over time...)
>
> We have the `bits` module since 6.17:
>
> https://rust.docs.kernel.org/kernel/bits/index.html
>
> (Or do you mean something else?)
No, that's exactly what I was thinking of, sorry, I missed that :)
greg k-h
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-21 9:36 ` Greg KH
2025-09-21 9:59 ` Miguel Ojeda
@ 2025-09-21 12:33 ` Benno Lossin
2025-09-21 12:45 ` Greg KH
2025-09-21 13:49 ` Danilo Krummrich
2 siblings, 1 reply; 44+ messages in thread
From: Benno Lossin @ 2025-09-21 12:33 UTC (permalink / raw)
To: Greg KH, Joel Fernandes
Cc: linux-kernel, rust-for-linux, dri-devel, dakr, acourbot,
Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Andreas Hindborg, Alice Ryhl, Trevor Gross,
David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, John Hubbard, Timur Tabi, joel, Elle Rhumsaa,
Yury Norov, Daniel Almeida, nouveau
On Sun Sep 21, 2025 at 11:36 AM CEST, Greg KH wrote:
> On Sat, Sep 20, 2025 at 02:22:27PM -0400, Joel Fernandes wrote:
>> The bitfield-specific into new macro. This will be used to define
>> structs with bitfields, similar to C language.
>>
>> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>> ---
>> drivers/gpu/nova-core/bitfield.rs | 314 +++++++++++++++++++++++++++
>> drivers/gpu/nova-core/nova_core.rs | 3 +
>> drivers/gpu/nova-core/regs/macros.rs | 259 +---------------------
>> 3 files changed, 327 insertions(+), 249 deletions(-)
>> create mode 100644 drivers/gpu/nova-core/bitfield.rs
>>
>> diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs
>> new file mode 100644
>> index 000000000000..ba6b7caa05d9
>> --- /dev/null
>> +++ b/drivers/gpu/nova-core/bitfield.rs
>> @@ -0,0 +1,314 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Bitfield library for Rust structures
>> +//!
>> +//! Support for defining bitfields in Rust structures. Also used by the [`register!`] macro.
>> +//!
>> +//! # Syntax
>> +//!
>> +//! ```rust
>> +//! #[derive(Debug, Clone, Copy)]
>> +//! enum Mode {
>> +//! Low = 0,
>> +//! High = 1,
>> +//! Auto = 2,
>> +//! }
>> +//!
>> +//! impl TryFrom<u8> for Mode {
>> +//! type Error = u8;
>> +//! fn try_from(value: u8) -> Result<Self, Self::Error> {
>> +//! match value {
>> +//! 0 => Ok(Mode::Low),
>> +//! 1 => Ok(Mode::High),
>> +//! 2 => Ok(Mode::Auto),
>> +//! _ => Err(value),
>> +//! }
>> +//! }
>> +//! }
>> +//!
>> +//! impl From<Mode> for u32 {
>> +//! fn from(mode: Mode) -> u32 {
>> +//! mode as u32
>> +//! }
>> +//! }
>> +//!
>> +//! #[derive(Debug, Clone, Copy)]
>> +//! enum State {
>> +//! Inactive = 0,
>> +//! Active = 1,
>> +//! }
>> +//!
>> +//! impl From<bool> for State {
>> +//! fn from(value: bool) -> Self {
>> +//! if value { State::Active } else { State::Inactive }
>> +//! }
>> +//! }
>> +//!
>> +//! impl From<State> for u32 {
>> +//! fn from(state: State) -> u32 {
>> +//! state as u32
>> +//! }
>> +//! }
>> +//!
>> +//! bitfield! {
>> +//! struct ControlReg {
>> +//! 3:0 mode as u8 ?=> Mode;
>> +//! 7 state as bool => State;
>> +//! }
>> +//! }
>
> As discussed at the conference this week, I do object to this as it
> will allow the same mistakes to happen that we used to do in the kernel
> for a long time before the regmap() api happened, along with GENMASK().
Have you read the following macro arm of the implementation?
// Generates the accessor methods for a single field.
(
@leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident
{ $process:expr } $to_type:ty => $res_type:ty $(, $comment:literal)?;
) => {
::kernel::macros::paste!(
const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
const [<$field:upper _MASK>]: u32 = ((((1 << $hi) - 1) << 1) + 1) - ((1 << $lo) - 1);
const [<$field:upper _SHIFT>]: u32 = Self::[<$field:upper _MASK>].trailing_zeros();
);
$(
#[doc="Returns the value of this field:"]
#[doc=$comment]
)?
#[inline(always)]
pub(crate) fn $field(self) -> $res_type {
::kernel::macros::paste!(
const MASK: u32 = $name::[<$field:upper _MASK>];
const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
);
let field = ((self.0 & MASK) >> SHIFT);
Here you can see that it's just a mask + shift operation internally to
access the field.
$process(field)
}
::kernel::macros::paste!(
$(
#[doc="Sets the value of this field:"]
#[doc=$comment]
)?
#[inline(always)]
pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
const MASK: u32 = $name::[<$field:upper _MASK>];
const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
let value = (u32::from(value) << SHIFT) & MASK;
self.0 = (self.0 & !MASK) | value;
self
}
);
};
Now I too would like to see how exactly this will be used to read data
from hardware. But at least in theory if the conversion from hardware
endianness to native endianness is done correctly, this will do the
right thing :)
> The issue is that you are going to want to take these bitfields as part
> of a larger structure, and attempt to "lay it over" a chunk of memory
> that came from, or is going to, hardware. When that happens, all of the
> endian issues of mis-matches between hardware and cpus come into play,
> which is not able to be properly expressed here at all, unless you
> attempt to either resolve it all later on in something like the regmap
> api, or you have #ifdef stuff to attempt to capture all of the possible
> combinations and deal with it at build time (which is strongly never
> recommended, but is what we used to do in previous decades.)
The "laying over part" requires a cast or transmute in Rust which is
`unsafe`, so I'd say we will definitely notice it in the code if a user
would be trying to do it.
---
Cheers,
Benno
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-21 12:33 ` Benno Lossin
@ 2025-09-21 12:45 ` Greg KH
2025-09-21 13:47 ` Danilo Krummrich
2025-09-23 22:24 ` Joel Fernandes
0 siblings, 2 replies; 44+ messages in thread
From: Greg KH @ 2025-09-21 12:45 UTC (permalink / raw)
To: Benno Lossin
Cc: Joel Fernandes, linux-kernel, rust-for-linux, dri-devel, dakr,
acourbot, Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, bjorn3_gh, Andreas Hindborg, Alice Ryhl, Trevor Gross,
David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, John Hubbard, Timur Tabi, joel, Elle Rhumsaa,
Yury Norov, Daniel Almeida, nouveau
On Sun, Sep 21, 2025 at 02:33:56PM +0200, Benno Lossin wrote:
> On Sun Sep 21, 2025 at 11:36 AM CEST, Greg KH wrote:
> > On Sat, Sep 20, 2025 at 02:22:27PM -0400, Joel Fernandes wrote:
> >> The bitfield-specific into new macro. This will be used to define
> >> structs with bitfields, similar to C language.
> >>
> >> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
> >> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> >> ---
> >> drivers/gpu/nova-core/bitfield.rs | 314 +++++++++++++++++++++++++++
> >> drivers/gpu/nova-core/nova_core.rs | 3 +
> >> drivers/gpu/nova-core/regs/macros.rs | 259 +---------------------
> >> 3 files changed, 327 insertions(+), 249 deletions(-)
> >> create mode 100644 drivers/gpu/nova-core/bitfield.rs
> >>
> >> diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs
> >> new file mode 100644
> >> index 000000000000..ba6b7caa05d9
> >> --- /dev/null
> >> +++ b/drivers/gpu/nova-core/bitfield.rs
> >> @@ -0,0 +1,314 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +//! Bitfield library for Rust structures
> >> +//!
> >> +//! Support for defining bitfields in Rust structures. Also used by the [`register!`] macro.
> >> +//!
> >> +//! # Syntax
> >> +//!
> >> +//! ```rust
> >> +//! #[derive(Debug, Clone, Copy)]
> >> +//! enum Mode {
> >> +//! Low = 0,
> >> +//! High = 1,
> >> +//! Auto = 2,
> >> +//! }
> >> +//!
> >> +//! impl TryFrom<u8> for Mode {
> >> +//! type Error = u8;
> >> +//! fn try_from(value: u8) -> Result<Self, Self::Error> {
> >> +//! match value {
> >> +//! 0 => Ok(Mode::Low),
> >> +//! 1 => Ok(Mode::High),
> >> +//! 2 => Ok(Mode::Auto),
> >> +//! _ => Err(value),
> >> +//! }
> >> +//! }
> >> +//! }
> >> +//!
> >> +//! impl From<Mode> for u32 {
> >> +//! fn from(mode: Mode) -> u32 {
> >> +//! mode as u32
> >> +//! }
> >> +//! }
> >> +//!
> >> +//! #[derive(Debug, Clone, Copy)]
> >> +//! enum State {
> >> +//! Inactive = 0,
> >> +//! Active = 1,
> >> +//! }
> >> +//!
> >> +//! impl From<bool> for State {
> >> +//! fn from(value: bool) -> Self {
> >> +//! if value { State::Active } else { State::Inactive }
> >> +//! }
> >> +//! }
> >> +//!
> >> +//! impl From<State> for u32 {
> >> +//! fn from(state: State) -> u32 {
> >> +//! state as u32
> >> +//! }
> >> +//! }
> >> +//!
> >> +//! bitfield! {
> >> +//! struct ControlReg {
> >> +//! 3:0 mode as u8 ?=> Mode;
> >> +//! 7 state as bool => State;
> >> +//! }
> >> +//! }
> >
> > As discussed at the conference this week, I do object to this as it
> > will allow the same mistakes to happen that we used to do in the kernel
> > for a long time before the regmap() api happened, along with GENMASK().
>
> Have you read the following macro arm of the implementation?
>
> // Generates the accessor methods for a single field.
> (
> @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident
> { $process:expr } $to_type:ty => $res_type:ty $(, $comment:literal)?;
> ) => {
> ::kernel::macros::paste!(
> const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
> const [<$field:upper _MASK>]: u32 = ((((1 << $hi) - 1) << 1) + 1) - ((1 << $lo) - 1);
> const [<$field:upper _SHIFT>]: u32 = Self::[<$field:upper _MASK>].trailing_zeros();
> );
>
> $(
> #[doc="Returns the value of this field:"]
> #[doc=$comment]
> )?
> #[inline(always)]
> pub(crate) fn $field(self) -> $res_type {
> ::kernel::macros::paste!(
> const MASK: u32 = $name::[<$field:upper _MASK>];
> const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
> );
> let field = ((self.0 & MASK) >> SHIFT);
>
> Here you can see that it's just a mask + shift operation internally to
> access the field.
>
> $process(field)
> }
>
> ::kernel::macros::paste!(
> $(
> #[doc="Sets the value of this field:"]
> #[doc=$comment]
> )?
> #[inline(always)]
> pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
> const MASK: u32 = $name::[<$field:upper _MASK>];
> const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
> let value = (u32::from(value) << SHIFT) & MASK;
> self.0 = (self.0 & !MASK) | value;
>
> self
> }
> );
> };
Yes, that's great, but that is all done in "native cpu" endian, and
might not actually represent what the hardware does at all, which is
what I was trying to get at here, sorry for not being more specific.
> Now I too would like to see how exactly this will be used to read data
> from hardware. But at least in theory if the conversion from hardware
> endianness to native endianness is done correctly, this will do the
> right thing :)
That's great, so we are close, but it's not quite correct. How about
something like:
0:7 reg_X as __le32
8:15 reg_y as __le32
and the like. There has to be a way to actually specify the
representation here and for C, that is using the "__" types that express
the endianness (i.e. __le32, __be32, and so on). Then we have type
checks that enforce accesses to those variables to always turn the data
into "native" values when the cpu accesses them.
Again, regmap handles this all just fine, why not just make bindings to
that api here instead?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-21 12:45 ` Greg KH
@ 2025-09-21 13:47 ` Danilo Krummrich
2025-09-23 6:38 ` Behme Dirk (XC-CP/ESD1)
2025-09-24 10:52 ` Greg KH
2025-09-23 22:24 ` Joel Fernandes
1 sibling, 2 replies; 44+ messages in thread
From: Danilo Krummrich @ 2025-09-21 13:47 UTC (permalink / raw)
To: Greg KH
Cc: Benno Lossin, Joel Fernandes, linux-kernel, rust-for-linux,
dri-devel, acourbot, Alistair Popple, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, bjorn3_gh, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On Sun Sep 21, 2025 at 2:45 PM CEST, Greg KH wrote:
> Again, regmap handles this all just fine, why not just make bindings to
> that api here instead?
The idea is to use this for the register!() macro, e.g.
register!(NV_PMC_BOOT_0 @ 0x00000000, "Basic revision information about the GPU" {
28:24 architecture_0 as u8, "Lower bits of the architecture";
23:20 implementation as u8, "Implementation version of the architecture";
8:8 architecture_1 as u8, "MSB of the architecture";
7:4 major_revision as u8, "Major revision of the chip";
3:0 minor_revision as u8, "Minor revision of the chip";
});
(More examples in [1].)
This generates a structure with the relevant accessors; we can also implement
additional logic, such as:
impl NV_PMC_BOOT_0 {
/// Combines `architecture_0` and `architecture_1` to obtain the architecture of the chip.
pub(crate) fn architecture(self) -> Result<Architecture> {
Architecture::try_from(
self.architecture_0() | (self.architecture_1() << Self::ARCHITECTURE_0_RANGE.len()),
)
}
/// Combines `architecture` and `implementation` to obtain a code unique to the chipset.
pub(crate) fn chipset(self) -> Result<Chipset> {
self.architecture()
.map(|arch| {
((arch as u32) << Self::IMPLEMENTATION_RANGE.len())
| u32::from(self.implementation())
})
.and_then(Chipset::try_from)
}
}
This conviniently allows us to read the register with
let boot0 = regs::NV_PMC_BOOT_0::read(bar);
and obtain an instance of the entire Chipset structure with
let chipset = boot0.chipset()?;
or pass it to a constructor that creates a Revision instance
let rev = Revision::from_boot0(boot0);
Analogously it allows us to modify and write registers without having to mess
with error prone shifts, masks and casts, because that code is generated by the
register!() macro. (Of course, unless we have more complicated cases where
multiple fields have to be combined as illustrated above.)
Note that bar is of type pci::Bar<BAR0_SIZE> where BAR0_SIZE in our case is
SZ_16M.
However, the type required by read() as generated by the register!() macro
actually only requires something that implements an I/O backend, i.e
kernel::io::Io<SIZE>.
pci::Bar is a specific implementation of kernel::io::Io.
With this we can let the actual I/O backend handle the endianness of the bus.
(Actually, we could even implement an I/O backend that uses regmap.)
So, I think the register!() stuff is rather orthogonal.
- Danilo
[1] https://gitlab.freedesktop.org/drm/rust/kernel/-/blob/drm-rust-next/drivers/gpu/nova-core/regs.rs
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-21 13:47 ` Danilo Krummrich
@ 2025-09-23 6:38 ` Behme Dirk (XC-CP/ESD1)
2025-09-24 10:52 ` Greg KH
1 sibling, 0 replies; 44+ messages in thread
From: Behme Dirk (XC-CP/ESD1) @ 2025-09-23 6:38 UTC (permalink / raw)
To: Danilo Krummrich, Greg KH
Cc: Benno Lossin, Joel Fernandes, linux-kernel, rust-for-linux,
dri-devel, acourbot, Alistair Popple, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, bjorn3_gh, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
> On Sun Sep 21, 2025 at 2:45 PM CEST, Greg KH wrote:
>> Again, regmap handles this all just fine, why not just make bindings to
>> that api here instead?
>
> The idea is to use this for the register!() macro, e.g.
>
> register!(NV_PMC_BOOT_0 @ 0x00000000, "Basic revision information about the GPU" {
> 28:24 architecture_0 as u8, "Lower bits of the architecture";
> 23:20 implementation as u8, "Implementation version of the architecture";
> 8:8 architecture_1 as u8, "MSB of the architecture";
> 7:4 major_revision as u8, "Major revision of the chip";
> 3:0 minor_revision as u8, "Minor revision of the chip";
> });
>
> (More examples in [1].)
>
> This generates a structure with the relevant accessors; we can also implement
> additional logic, such as:
>
> impl NV_PMC_BOOT_0 {
> /// Combines `architecture_0` and `architecture_1` to obtain the architecture of the chip.
> pub(crate) fn architecture(self) -> Result<Architecture> {
> Architecture::try_from(
> self.architecture_0() | (self.architecture_1() << Self::ARCHITECTURE_0_RANGE.len()),
> )
> }
>
> /// Combines `architecture` and `implementation` to obtain a code unique to the chipset.
> pub(crate) fn chipset(self) -> Result<Chipset> {
> self.architecture()
> .map(|arch| {
> ((arch as u32) << Self::IMPLEMENTATION_RANGE.len())
> | u32::from(self.implementation())
> })
> .and_then(Chipset::try_from)
> }
> }
>
> This conviniently allows us to read the register with
>
> let boot0 = regs::NV_PMC_BOOT_0::read(bar);
>
> and obtain an instance of the entire Chipset structure with
>
> let chipset = boot0.chipset()?;
>
> or pass it to a constructor that creates a Revision instance
>
> let rev = Revision::from_boot0(boot0);
>
> Analogously it allows us to modify and write registers without having to mess
> with error prone shifts, masks and casts, because that code is generated by the
> register!() macro. (Of course, unless we have more complicated cases where
> multiple fields have to be combined as illustrated above.)
>
> Note that bar is of type pci::Bar<BAR0_SIZE> where BAR0_SIZE in our case is
> SZ_16M.
>
> However, the type required by read() as generated by the register!() macro
> actually only requires something that implements an I/O backend, i.e
> kernel::io::Io<SIZE>.
>
> pci::Bar is a specific implementation of kernel::io::Io.
>
> With this we can let the actual I/O backend handle the endianness of the bus.
>
> (Actually, we could even implement an I/O backend that uses regmap.)
>
> So, I think the register!() stuff is rather orthogonal.
>
> - Danilo
>
> [1] https://gitlab.freedesktop.org/drm/rust/kernel/-/blob/drm-rust-next/drivers/gpu/nova-core/regs.rs
I really like this discussion with the thoughts and examples given above!
Some weeks ago I was wondering what might be the correct way for
accessing registers in RfL. There are various different examples
floating around what makes the selection of the correct one even harder.
Most probably I have selected the wrong one ;)
So once this discussion comes to a conclusion (and gets merged?) we
might want to have a verbose documentation with background, examples,
possible options etc. Something like "Howto access registers (hardware?)
in RfL (correctly)". I think this would be quite helpful.
Thanks!
Dirk
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-21 13:47 ` Danilo Krummrich
2025-09-23 6:38 ` Behme Dirk (XC-CP/ESD1)
@ 2025-09-24 10:52 ` Greg KH
2025-09-24 11:28 ` Danilo Krummrich
2025-09-24 14:38 ` Yury Norov
1 sibling, 2 replies; 44+ messages in thread
From: Greg KH @ 2025-09-24 10:52 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Benno Lossin, Joel Fernandes, linux-kernel, rust-for-linux,
dri-devel, acourbot, Alistair Popple, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, bjorn3_gh, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On Sun, Sep 21, 2025 at 03:47:55PM +0200, Danilo Krummrich wrote:
> On Sun Sep 21, 2025 at 2:45 PM CEST, Greg KH wrote:
> > Again, regmap handles this all just fine, why not just make bindings to
> > that api here instead?
>
> The idea is to use this for the register!() macro, e.g.
>
> register!(NV_PMC_BOOT_0 @ 0x00000000, "Basic revision information about the GPU" {
> 28:24 architecture_0 as u8, "Lower bits of the architecture";
> 23:20 implementation as u8, "Implementation version of the architecture";
> 8:8 architecture_1 as u8, "MSB of the architecture";
> 7:4 major_revision as u8, "Major revision of the chip";
> 3:0 minor_revision as u8, "Minor revision of the chip";
> });
>
> (More examples in [1].)
Wonderful, but I fail to see where the endian-ness of this is set
anywhere. Am I just missing that? The regmap api enforces this idea,
and so the
>
> This generates a structure with the relevant accessors; we can also implement
> additional logic, such as:
>
> impl NV_PMC_BOOT_0 {
> /// Combines `architecture_0` and `architecture_1` to obtain the architecture of the chip.
> pub(crate) fn architecture(self) -> Result<Architecture> {
> Architecture::try_from(
> self.architecture_0() | (self.architecture_1() << Self::ARCHITECTURE_0_RANGE.len()),
> )
> }
>
> /// Combines `architecture` and `implementation` to obtain a code unique to the chipset.
> pub(crate) fn chipset(self) -> Result<Chipset> {
> self.architecture()
> .map(|arch| {
> ((arch as u32) << Self::IMPLEMENTATION_RANGE.len())
> | u32::from(self.implementation())
> })
> .and_then(Chipset::try_from)
> }
> }
>
> This conviniently allows us to read the register with
>
> let boot0 = regs::NV_PMC_BOOT_0::read(bar);
>
> and obtain an instance of the entire Chipset structure with
>
> let chipset = boot0.chipset()?;
>
> or pass it to a constructor that creates a Revision instance
>
> let rev = Revision::from_boot0(boot0);
>
> Analogously it allows us to modify and write registers without having to mess
> with error prone shifts, masks and casts, because that code is generated by the
> register!() macro. (Of course, unless we have more complicated cases where
> multiple fields have to be combined as illustrated above.)
>
> Note that bar is of type pci::Bar<BAR0_SIZE> where BAR0_SIZE in our case is
> SZ_16M.
>
> However, the type required by read() as generated by the register!() macro
> actually only requires something that implements an I/O backend, i.e
> kernel::io::Io<SIZE>.
>
> pci::Bar is a specific implementation of kernel::io::Io.
>
> With this we can let the actual I/O backend handle the endianness of the bus.
Ok, great, but right now it's not doing that from what I am seeing when
reading the code. Shouldn't IoMem::new() take that as an argument?
But, that feels odd as our current iomem api in C doesn't care about
endian issues at all because it "assumes" that the caller has already
handle this properly and all that the caller "wants" is to write/read to
some memory chunk and not twiddle bits.
> (Actually, we could even implement an I/O backend that uses regmap.)
That would probably be best to do eventually as most platform drivers
use regmap today as it's the sanest api we have at the moment.
> So, I think the register!() stuff is rather orthogonal.
I think it's very relevant as people seem to just be "assuming" that all
the world (hardware and cpus) are little-endian, while in reality, they
are anything but. As proof, the code that uses this register!() logic
today totally ignores endian issues and just assumes that it is both
running on a little-endian system, AND the hardware is little-endian.
As a crazy example, look at the USB host controllers that at runtime,
have to be queried to determine what endian they are running on and the
kernel drivers have to handle this "on the fly". Yes, one can argue
that the hardware developers who came up with that should be forced to
write the drivers as penance for such sins, but in the end, it's us that
has to deal with it...
So ignoring it will get us quite a ways forward with controlling sane
hardware on sane systems, but when s390 finally realizes they can be
writing their drivers in rust, we are going to have to have these
conversations again :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-24 10:52 ` Greg KH
@ 2025-09-24 11:28 ` Danilo Krummrich
2025-09-24 12:04 ` Greg KH
2025-09-24 14:38 ` Yury Norov
1 sibling, 1 reply; 44+ messages in thread
From: Danilo Krummrich @ 2025-09-24 11:28 UTC (permalink / raw)
To: Greg KH
Cc: Benno Lossin, Joel Fernandes, linux-kernel, rust-for-linux,
dri-devel, acourbot, Alistair Popple, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, bjorn3_gh, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On Wed Sep 24, 2025 at 12:52 PM CEST, Greg KH wrote:
> Ok, great, but right now it's not doing that from what I am seeing when
> reading the code. Shouldn't IoMem::new() take that as an argument?
That's correct, neither IoMem nor pci::Bar do consider it yet; it's on the list
of things that still need to be done.
> But, that feels odd as our current iomem api in C doesn't care about
> endian issues at all because it "assumes" that the caller has already
> handle this properly and all that the caller "wants" is to write/read to
> some memory chunk and not twiddle bits.
Yet it seems to be the correct place to deal with it. As mentioned below, regmap
could just become part of an I/O backend implementation to do exactly that.
>> (Actually, we could even implement an I/O backend that uses regmap.)
>
> That would probably be best to do eventually as most platform drivers
> use regmap today as it's the sanest api we have at the moment.
I agree it's what we should do eventually.
>> So, I think the register!() stuff is rather orthogonal.
>
> I think it's very relevant as people seem to just be "assuming" that all
> the world (hardware and cpus) are little-endian, while in reality, they
> are anything but. As proof, the code that uses this register!() logic
> today totally ignores endian issues and just assumes that it is both
> running on a little-endian system, AND the hardware is little-endian.
>
> As a crazy example, look at the USB host controllers that at runtime,
> have to be queried to determine what endian they are running on and the
> kernel drivers have to handle this "on the fly". Yes, one can argue
> that the hardware developers who came up with that should be forced to
> write the drivers as penance for such sins, but in the end, it's us that
> has to deal with it...
>
> So ignoring it will get us quite a ways forward with controlling sane
> hardware on sane systems, but when s390 finally realizes they can be
> writing their drivers in rust, we are going to have to have these
> conversations again :)
I think it's not really that anyone is ignoring it (intentionally). It's two
different things that should be addressed here; yet they are related:
(1) Implementation of an abstract representation of a register that drivers
can interact with.
(2) The I/O layer that lays out the raw data on the physcial bus.
The register!() macro intends to provide an abstract representation of a
register for drivers to interact with. Think of it as an abstract box, where the
memory layout does not matter at all -- could be anything.
Theoretically, this abstraction could even store every single field of a
register in its own u32 or u64, etc. Of course, that's a waste of memory, which
is why we're using this bitfield thing instead.
The only thing that matters is that there is a contract between the struct
representing a register (generated by the register!() macro) and the I/O backend
layer that lays out the raw value on the bus.
This works attempts to address (1), whereas you are (rightfully) asking for (2).
And I think the answer for (2) simply is, we still have to address it.
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-24 11:28 ` Danilo Krummrich
@ 2025-09-24 12:04 ` Greg KH
0 siblings, 0 replies; 44+ messages in thread
From: Greg KH @ 2025-09-24 12:04 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Benno Lossin, Joel Fernandes, linux-kernel, rust-for-linux,
dri-devel, acourbot, Alistair Popple, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, bjorn3_gh, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On Wed, Sep 24, 2025 at 01:28:18PM +0200, Danilo Krummrich wrote:
> On Wed Sep 24, 2025 at 12:52 PM CEST, Greg KH wrote:
> > Ok, great, but right now it's not doing that from what I am seeing when
> > reading the code. Shouldn't IoMem::new() take that as an argument?
>
> That's correct, neither IoMem nor pci::Bar do consider it yet; it's on the list
> of things that still need to be done.
>
> > But, that feels odd as our current iomem api in C doesn't care about
> > endian issues at all because it "assumes" that the caller has already
> > handle this properly and all that the caller "wants" is to write/read to
> > some memory chunk and not twiddle bits.
>
> Yet it seems to be the correct place to deal with it. As mentioned below, regmap
> could just become part of an I/O backend implementation to do exactly that.
>
> >> (Actually, we could even implement an I/O backend that uses regmap.)
> >
> > That would probably be best to do eventually as most platform drivers
> > use regmap today as it's the sanest api we have at the moment.
>
> I agree it's what we should do eventually.
>
> >> So, I think the register!() stuff is rather orthogonal.
> >
> > I think it's very relevant as people seem to just be "assuming" that all
> > the world (hardware and cpus) are little-endian, while in reality, they
> > are anything but. As proof, the code that uses this register!() logic
> > today totally ignores endian issues and just assumes that it is both
> > running on a little-endian system, AND the hardware is little-endian.
> >
> > As a crazy example, look at the USB host controllers that at runtime,
> > have to be queried to determine what endian they are running on and the
> > kernel drivers have to handle this "on the fly". Yes, one can argue
> > that the hardware developers who came up with that should be forced to
> > write the drivers as penance for such sins, but in the end, it's us that
> > has to deal with it...
> >
> > So ignoring it will get us quite a ways forward with controlling sane
> > hardware on sane systems, but when s390 finally realizes they can be
> > writing their drivers in rust, we are going to have to have these
> > conversations again :)
>
> I think it's not really that anyone is ignoring it (intentionally). It's two
> different things that should be addressed here; yet they are related:
>
> (1) Implementation of an abstract representation of a register that drivers
> can interact with.
>
> (2) The I/O layer that lays out the raw data on the physcial bus.
>
> The register!() macro intends to provide an abstract representation of a
> register for drivers to interact with. Think of it as an abstract box, where the
> memory layout does not matter at all -- could be anything.
>
> Theoretically, this abstraction could even store every single field of a
> register in its own u32 or u64, etc. Of course, that's a waste of memory, which
> is why we're using this bitfield thing instead.
>
> The only thing that matters is that there is a contract between the struct
> representing a register (generated by the register!() macro) and the I/O backend
> layer that lays out the raw value on the bus.
>
> This works attempts to address (1), whereas you are (rightfully) asking for (2).
> And I think the answer for (2) simply is, we still have to address it.
Ok, fair enough, you've convinced me, I'll let you all argue which side
the "0" should be on (left or right) :)
Let's wait for the first platform drivers to start showing up that want
to use regmap, hopefully that will be soon as almost all of the pieces
are there to do so.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-24 10:52 ` Greg KH
2025-09-24 11:28 ` Danilo Krummrich
@ 2025-09-24 14:38 ` Yury Norov
2025-09-24 15:53 ` Danilo Krummrich
2025-09-24 17:46 ` Joel Fernandes
1 sibling, 2 replies; 44+ messages in thread
From: Yury Norov @ 2025-09-24 14:38 UTC (permalink / raw)
To: Greg KH
Cc: Danilo Krummrich, Benno Lossin, Joel Fernandes, linux-kernel,
rust-for-linux, dri-devel, acourbot, Alistair Popple,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, bjorn3_gh,
Andreas Hindborg, Alice Ryhl, Trevor Gross, David Airlie,
Simona Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, John Hubbard, Timur Tabi, joel, Elle Rhumsaa,
Daniel Almeida, nouveau
On Wed, Sep 24, 2025 at 12:52:41PM +0200, Greg KH wrote:
> On Sun, Sep 21, 2025 at 03:47:55PM +0200, Danilo Krummrich wrote:
> > On Sun Sep 21, 2025 at 2:45 PM CEST, Greg KH wrote:
> > > Again, regmap handles this all just fine, why not just make bindings to
> > > that api here instead?
> >
> > The idea is to use this for the register!() macro, e.g.
> >
> > register!(NV_PMC_BOOT_0 @ 0x00000000, "Basic revision information about the GPU" {
> > 28:24 architecture_0 as u8, "Lower bits of the architecture";
> > 23:20 implementation as u8, "Implementation version of the architecture";
> > 8:8 architecture_1 as u8, "MSB of the architecture";
> > 7:4 major_revision as u8, "Major revision of the chip";
> > 3:0 minor_revision as u8, "Minor revision of the chip";
> > });
> >
> > (More examples in [1].)
>
> Wonderful, but I fail to see where the endian-ness of this is set
> anywhere. Am I just missing that? The regmap api enforces this idea,
> and so the
>
> >
> > This generates a structure with the relevant accessors; we can also implement
> > additional logic, such as:
> >
> > impl NV_PMC_BOOT_0 {
> > /// Combines `architecture_0` and `architecture_1` to obtain the architecture of the chip.
> > pub(crate) fn architecture(self) -> Result<Architecture> {
> > Architecture::try_from(
> > self.architecture_0() | (self.architecture_1() << Self::ARCHITECTURE_0_RANGE.len()),
> > )
> > }
> >
> > /// Combines `architecture` and `implementation` to obtain a code unique to the chipset.
> > pub(crate) fn chipset(self) -> Result<Chipset> {
> > self.architecture()
> > .map(|arch| {
> > ((arch as u32) << Self::IMPLEMENTATION_RANGE.len())
> > | u32::from(self.implementation())
> > })
> > .and_then(Chipset::try_from)
> > }
> > }
> >
> > This conviniently allows us to read the register with
> >
> > let boot0 = regs::NV_PMC_BOOT_0::read(bar);
> >
> > and obtain an instance of the entire Chipset structure with
> >
> > let chipset = boot0.chipset()?;
> >
> > or pass it to a constructor that creates a Revision instance
> >
> > let rev = Revision::from_boot0(boot0);
> >
> > Analogously it allows us to modify and write registers without having to mess
> > with error prone shifts, masks and casts, because that code is generated by the
> > register!() macro. (Of course, unless we have more complicated cases where
> > multiple fields have to be combined as illustrated above.)
> >
> > Note that bar is of type pci::Bar<BAR0_SIZE> where BAR0_SIZE in our case is
> > SZ_16M.
> >
> > However, the type required by read() as generated by the register!() macro
> > actually only requires something that implements an I/O backend, i.e
> > kernel::io::Io<SIZE>.
> >
> > pci::Bar is a specific implementation of kernel::io::Io.
> >
> > With this we can let the actual I/O backend handle the endianness of the bus.
>
> Ok, great, but right now it's not doing that from what I am seeing when
> reading the code. Shouldn't IoMem::new() take that as an argument?
>
> But, that feels odd as our current iomem api in C doesn't care about
> endian issues at all because it "assumes" that the caller has already
> handle this properly and all that the caller "wants" is to write/read to
> some memory chunk and not twiddle bits.
>
> > (Actually, we could even implement an I/O backend that uses regmap.)
>
> That would probably be best to do eventually as most platform drivers
> use regmap today as it's the sanest api we have at the moment.
>
> > So, I think the register!() stuff is rather orthogonal.
>
> I think it's very relevant as people seem to just be "assuming" that all
> the world (hardware and cpus) are little-endian, while in reality, they
> are anything but. As proof, the code that uses this register!() logic
> today totally ignores endian issues and just assumes that it is both
> running on a little-endian system, AND the hardware is little-endian.
>
> As a crazy example, look at the USB host controllers that at runtime,
> have to be queried to determine what endian they are running on and the
> kernel drivers have to handle this "on the fly". Yes, one can argue
> that the hardware developers who came up with that should be forced to
> write the drivers as penance for such sins, but in the end, it's us that
> has to deal with it...
>
> So ignoring it will get us quite a ways forward with controlling sane
> hardware on sane systems, but when s390 finally realizes they can be
> writing their drivers in rust, we are going to have to have these
> conversations again :)
Hi Greg, all,
Endianess is not the only problem when dealing with registers mapped
to the memory, right?
I recall some built-in 12-bit ADCs in 8-bit AVR microcontrollers. That
required to read 4-bit LO register before 8-bit HI, if you didn't want to
loose those 4 bits.
Bitfields don't address that issue as well. In my understanding, it's
done on purpose: bitfields encapsulate shifts and masks, and don't
pretend that they are suitable for direct access to a hardware.
Notice another rust bitfield project. It tries to account for endianess
and everything else:
https://docs.rs/bitfield-struct/latest/bitfield_struct/
I didn't ask explicitly, and maybe it's a good time to ask now: Joel,
Danilo and everyone, have you considered adopting this project in
kernel?
The bitfield_struct builds everything into the structure:
use bitfield_struct::bitfield;
#[bitfield(u8, order = Msb)]
struct MyMsbByte {
/// The first field occupies the *most* significant bits
#[bits(4)]
kind: usize,
system: bool,
#[bits(2)]
level: usize,
present: bool
}
let my_byte_msb = MyMsbByte::new()
.with_kind(10)
.with_system(false)
.with_level(2)
.with_present(true);
// .- kind
// | .- system
// | | .- level
// | | | .- present
assert_eq!(my_byte_msb.0, 0b1010_0_10_1);
Here MSB is not BE. For BE you'd specify:
#[bitfield(u16, repr = be16, from = be16::from_ne, into = be16::to_ne)]
struct MyBeBitfield {
#[bits(4)]
first_nibble: u8,
#[bits(12)]
other: u16,
}
The "from = be16::from_ne", is seemingly the same as cpu_to_be32() here.
It looks like bitfield_struct tries to resolve hw access problems
by outsourcing them to 'from' and 'to' callbacks, and that looks
similar to what regmap API does (is that correct?).
Greg, Is that what you're asking about?
This is another bitfield crate with the similar approach
https://crates.io/crates/bitfield
So we're not the first, and we need to discuss what is already done.
As far as I understand, Joel decided to go in the other direction:
bitfields are always native in terms of endianess and not designed to
be mapped on registers directly. Which means they don't specify order
of accesses, number of accesses, access timing, atomicity, alignment,
cacheability and whatever else I/O related.
I discussed with Joel about the hw register access and he confirmed
that the idea of his bitfields is to be a simple wrapper around logical
ops, while the I/O is a matter of 'backbone', which is entirely different
thing:
reg = nova_register(addr, be64,
strong_ordered, lo_first, ...)
reg.read()
bf = reg.bf()
val = bf.field1() | MY_FLAG
bf.set_field1(val)
reg.set_bf()
reg.write()
In this design, .read() and .write() are the only accessors to the
mapped registers memory, they do endianess conversion if needed,
and everything else.
I'm not an expert in regmaps, but from what I can see, the complexity
of the backbone I/O might exceed complexity of bitfields themself; and
what the bitfield_struct (and the other project I've googled) does
looks like dictating to potentially more complex projects about how
their API should look.
Because rust has no out-of-the-box bitfields, like C does, I think
that we should have simple API that resembles C bitfields syntax and
functionality. Joel mentioned rcu_special, and I can guess there's
more examples like flags, where people just need a compact data
structure with a per-bit access capability.
With that, from bitops perspective I think bitfields are anyways useful
addition to rust. How the rust code would address I/O problems is a more
complex and seemingly not immediately related subject.
Does that make sense?
Thanks,
Yury
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-24 14:38 ` Yury Norov
@ 2025-09-24 15:53 ` Danilo Krummrich
2025-09-24 17:46 ` Joel Fernandes
1 sibling, 0 replies; 44+ messages in thread
From: Danilo Krummrich @ 2025-09-24 15:53 UTC (permalink / raw)
To: Yury Norov
Cc: Greg KH, Benno Lossin, Joel Fernandes, linux-kernel,
rust-for-linux, dri-devel, acourbot, Alistair Popple,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, bjorn3_gh,
Andreas Hindborg, Alice Ryhl, Trevor Gross, David Airlie,
Simona Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, John Hubbard, Timur Tabi, joel, Elle Rhumsaa,
Daniel Almeida, nouveau
On Wed Sep 24, 2025 at 4:38 PM CEST, Yury Norov wrote:
> I didn't ask explicitly, and maybe it's a good time to ask now: Joel,
> Danilo and everyone, have you considered adopting this project in
> kernel?
>
> The bitfield_struct builds everything into the structure:
>
> use bitfield_struct::bitfield;
>
> #[bitfield(u8, order = Msb)]
> struct MyMsbByte {
> /// The first field occupies the *most* significant bits
> #[bits(4)]
> kind: usize,
> system: bool,
> #[bits(2)]
> level: usize,
> present: bool
> }
> let my_byte_msb = MyMsbByte::new()
> .with_kind(10)
> .with_system(false)
> .with_level(2)
> .with_present(true);
>
> // .- kind
> // | .- system
> // | | .- level
> // | | | .- present
> assert_eq!(my_byte_msb.0, 0b1010_0_10_1);
>
> Here MSB is not BE. For BE you'd specify:
>
> #[bitfield(u16, repr = be16, from = be16::from_ne, into = be16::to_ne)]
> struct MyBeBitfield {
> #[bits(4)]
> first_nibble: u8,
> #[bits(12)]
> other: u16,
> }
>
> The "from = be16::from_ne", is seemingly the same as cpu_to_be32() here.
>
> It looks like bitfield_struct tries to resolve hw access problems
> by outsourcing them to 'from' and 'to' callbacks, and that looks
> similar to what regmap API does (is that correct?).
>
> Greg, Is that what you're asking about?
>
> This is another bitfield crate with the similar approach
>
> https://crates.io/crates/bitfield
>
> So we're not the first, and we need to discuss what is already done.
>
> As far as I understand, Joel decided to go in the other direction:
> bitfields are always native in terms of endianess and not designed to
> be mapped on registers directly. Which means they don't specify order
> of accesses, number of accesses, access timing, atomicity, alignment,
> cacheability and whatever else I/O related.
>
> I discussed with Joel about the hw register access and he confirmed
> that the idea of his bitfields is to be a simple wrapper around logical
> ops, while the I/O is a matter of 'backbone', which is entirely different
> thing:
When I was working on initial Rust driver support about a year ago, I also
thought about how Rust drivers can deal with registers and added the TODO in
[1]. This was picked up by Alex, who came up with a great implementation for the
register!() macro, which Joel splitted up into separate register!() and bitfield
parts in the context of moving it from a nova internal implementation into a
core kernel API.
As also described in [2], the idea is to have a macro, register!(), that creates
an abstract representation of a register, in order to remove the need for
drivers to manually construct values through shift operations, masks, etc.
At the same time the idea also is to get proper documentation of the hardware
registers in the kernel; the register!() macro encourages that, by its
definition trying to come close to how registers are typically documented in
datasheets, i.e. get rid of thousands of lines of auto-generated #defines for
base addresses, shift and masks with cryptic names and provide something like
register!(NV_PMC_BOOT_0 @ 0x00000000, "Basic revision information about the GPU" {
28:24 architecture_0 as u8, "Lower bits of the architecture";
23:20 implementation as u8, "Implementation version of the architecture";
8:8 architecture_1 as u8, "MSB of the architecture";
7:4 major_revision as u8, "Major revision of the chip";
3:0 minor_revision as u8, "Minor revision of the chip";
});
instead.
(It has quite some more features that also allow you to directly derive complex
types from primitives and define arrays of registers, such as in
register!(NV_PFALCON_FBIF_TRANSCFG @ PFalconBase[0x00000600[8]] {
1:0 target as u8 ?=> FalconFbifTarget;
2:2 mem_type as bool => FalconFbifMemType;
});
which makes dealing with such registers in drivers way less error prone.
Here's one example of how it looks like to alter a single field within a
register:
// `bar` is the `pci::Bar` I/O backend.
regs::NV_PFALCON_FALCON_ENGINE::alter(bar, |v| v.set_reset(true));
Of course you could also alter multiple fields at once by doing more changes
within the closure.)
It intentionally avoids encoding hardware bus specific endianness, because
otherwise we'd need to define this for every single register definition, which
also falls apart when the device can sit on top of multiple different busses.
Instead, the only thing that matters is that there is a contract between the
abstract register representation and the I/O backends, such that the data can be
correctly layed out by the I/O backend, which has to be aware of the actual
hardware bus instead.
As mentioned in another thread, one option for that is to use regmap within the
I/O backends, but that still needs to be addressed.
So, for the register!() macro, I think we should keep it an abstract
representation and deal with endianness in the I/O backend.
However, that's or course orthogonal to the actual feature set of the bitfield
macro itself.
- Danilo
[1] https://docs.kernel.org/gpu/nova/core/todo.html#generic-register-abstraction-rega
[2] https://lore.kernel.org/lkml/DD0ZTZM8S84H.1YDWSY7DF14LM@kernel.org/
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-24 14:38 ` Yury Norov
2025-09-24 15:53 ` Danilo Krummrich
@ 2025-09-24 17:46 ` Joel Fernandes
2025-09-24 20:01 ` Elle Rhumsaa
1 sibling, 1 reply; 44+ messages in thread
From: Joel Fernandes @ 2025-09-24 17:46 UTC (permalink / raw)
To: Yury Norov, Greg KH
Cc: Danilo Krummrich, Benno Lossin, linux-kernel, rust-for-linux,
dri-devel, acourbot, Alistair Popple, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, bjorn3_gh, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Daniel Almeida, nouveau
On 9/24/2025 4:38 PM, Yury Norov wrote:
> On Wed, Sep 24, 2025 at 12:52:41PM +0200, Greg KH wrote:
>> On Sun, Sep 21, 2025 at 03:47:55PM +0200, Danilo Krummrich wrote:
>>> On Sun Sep 21, 2025 at 2:45 PM CEST, Greg KH wrote:
>>>> Again, regmap handles this all just fine, why not just make bindings to
>>>> that api here instead?
>>>
>>> The idea is to use this for the register!() macro, e.g.
>>>
>>> register!(NV_PMC_BOOT_0 @ 0x00000000, "Basic revision information about the GPU" {
>>> 28:24 architecture_0 as u8, "Lower bits of the architecture";
>>> 23:20 implementation as u8, "Implementation version of the architecture";
>>> 8:8 architecture_1 as u8, "MSB of the architecture";
>>> 7:4 major_revision as u8, "Major revision of the chip";
>>> 3:0 minor_revision as u8, "Minor revision of the chip";
>>> });
>>>
>>> (More examples in [1].)
>>
>> Wonderful, but I fail to see where the endian-ness of this is set
>> anywhere. Am I just missing that? The regmap api enforces this idea,
>> and so the
>>
>>>
>>> This generates a structure with the relevant accessors; we can also implement
>>> additional logic, such as:
>>>
>>> impl NV_PMC_BOOT_0 {
>>> /// Combines `architecture_0` and `architecture_1` to obtain the architecture of the chip.
>>> pub(crate) fn architecture(self) -> Result<Architecture> {
>>> Architecture::try_from(
>>> self.architecture_0() | (self.architecture_1() << Self::ARCHITECTURE_0_RANGE.len()),
>>> )
>>> }
>>>
>>> /// Combines `architecture` and `implementation` to obtain a code unique to the chipset.
>>> pub(crate) fn chipset(self) -> Result<Chipset> {
>>> self.architecture()
>>> .map(|arch| {
>>> ((arch as u32) << Self::IMPLEMENTATION_RANGE.len())
>>> | u32::from(self.implementation())
>>> })
>>> .and_then(Chipset::try_from)
>>> }
>>> }
>>>
>>> This conviniently allows us to read the register with
>>>
>>> let boot0 = regs::NV_PMC_BOOT_0::read(bar);
>>>
>>> and obtain an instance of the entire Chipset structure with
>>>
>>> let chipset = boot0.chipset()?;
>>>
>>> or pass it to a constructor that creates a Revision instance
>>>
>>> let rev = Revision::from_boot0(boot0);
>>>
>>> Analogously it allows us to modify and write registers without having to mess
>>> with error prone shifts, masks and casts, because that code is generated by the
>>> register!() macro. (Of course, unless we have more complicated cases where
>>> multiple fields have to be combined as illustrated above.)
>>>
>>> Note that bar is of type pci::Bar<BAR0_SIZE> where BAR0_SIZE in our case is
>>> SZ_16M.
>>>
>>> However, the type required by read() as generated by the register!() macro
>>> actually only requires something that implements an I/O backend, i.e
>>> kernel::io::Io<SIZE>.
>>>
>>> pci::Bar is a specific implementation of kernel::io::Io.
>>>
>>> With this we can let the actual I/O backend handle the endianness of the bus.
>>
>> Ok, great, but right now it's not doing that from what I am seeing when
>> reading the code. Shouldn't IoMem::new() take that as an argument?
>>
>> But, that feels odd as our current iomem api in C doesn't care about
>> endian issues at all because it "assumes" that the caller has already
>> handle this properly and all that the caller "wants" is to write/read to
>> some memory chunk and not twiddle bits.
>>
>>> (Actually, we could even implement an I/O backend that uses regmap.)
>>
>> That would probably be best to do eventually as most platform drivers
>> use regmap today as it's the sanest api we have at the moment.
>>
>>> So, I think the register!() stuff is rather orthogonal.
>>
>> I think it's very relevant as people seem to just be "assuming" that all
>> the world (hardware and cpus) are little-endian, while in reality, they
>> are anything but. As proof, the code that uses this register!() logic
>> today totally ignores endian issues and just assumes that it is both
>> running on a little-endian system, AND the hardware is little-endian.
>>
>> As a crazy example, look at the USB host controllers that at runtime,
>> have to be queried to determine what endian they are running on and the
>> kernel drivers have to handle this "on the fly". Yes, one can argue
>> that the hardware developers who came up with that should be forced to
>> write the drivers as penance for such sins, but in the end, it's us that
>> has to deal with it...
>>
>> So ignoring it will get us quite a ways forward with controlling sane
>> hardware on sane systems, but when s390 finally realizes they can be
>> writing their drivers in rust, we are going to have to have these
>> conversations again :)
>
> Hi Greg, all,
>
> Endianess is not the only problem when dealing with registers mapped
> to the memory, right?
>
> I recall some built-in 12-bit ADCs in 8-bit AVR microcontrollers. That
> required to read 4-bit LO register before 8-bit HI, if you didn't want to
> loose those 4 bits.
>
> Bitfields don't address that issue as well. In my understanding, it's
> done on purpose: bitfields encapsulate shifts and masks, and don't
> pretend that they are suitable for direct access to a hardware.
>
> Notice another rust bitfield project. It tries to account for endianess
> and everything else:
>
> https://docs.rs/bitfield-struct/latest/bitfield_struct/
>
> I didn't ask explicitly, and maybe it's a good time to ask now: Joel,
> Danilo and everyone, have you considered adopting this project in
> kernel?
>
> The bitfield_struct builds everything into the structure:
>
> use bitfield_struct::bitfield;
>
> #[bitfield(u8, order = Msb)]
> struct MyMsbByte {
> /// The first field occupies the *most* significant bits
> #[bits(4)]
> kind: usize,
> system: bool,
> #[bits(2)]
> level: usize,
> present: bool
> }
Thanks for raising this. The syntax seems quite different from what we need, in
particular since register! macro is based on our bitfield! macro, this syntax is
incompatible with the need to specify bit ranges, not just the number of bits.
In other words, it appears the out-of-crate does not satisfy the requirement.
They have to specific 'order' property mainly because they don't have the notion
of bitfield index, just number of bits.
Regarding endianness in that crate, it appears to be configurable based on
user's requirement so we can make it such if needed for any kernel usecases. But
the default in that crate is native-endianness just like our implementation right?
Thanks.
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-24 17:46 ` Joel Fernandes
@ 2025-09-24 20:01 ` Elle Rhumsaa
2025-09-25 7:05 ` Joel Fernandes
0 siblings, 1 reply; 44+ messages in thread
From: Elle Rhumsaa @ 2025-09-24 20:01 UTC (permalink / raw)
To: Joel Fernandes, Yury Norov, Greg KH
Cc: Danilo Krummrich, Benno Lossin, linux-kernel, rust-for-linux,
dri-devel, acourbot, Alistair Popple, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, bjorn3_gh, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Daniel Almeida, nouveau
>
> On 9/24/2025 4:38 PM, Yury Norov wrote:
>> On Wed, Sep 24, 2025 at 12:52:41PM +0200, Greg KH wrote:
>>> On Sun, Sep 21, 2025 at 03:47:55PM +0200, Danilo Krummrich wrote:
>>>> On Sun Sep 21, 2025 at 2:45 PM CEST, Greg KH wrote:
>>>>> Again, regmap handles this all just fine, why not just make bindings to
>>>>> that api here instead?
>>>> The idea is to use this for the register!() macro, e.g.
>>>>
>>>> register!(NV_PMC_BOOT_0 @ 0x00000000, "Basic revision information about the GPU" {
>>>> 28:24 architecture_0 as u8, "Lower bits of the architecture";
>>>> 23:20 implementation as u8, "Implementation version of the architecture";
>>>> 8:8 architecture_1 as u8, "MSB of the architecture";
>>>> 7:4 major_revision as u8, "Major revision of the chip";
>>>> 3:0 minor_revision as u8, "Minor revision of the chip";
>>>> });
>>>>
>>>> (More examples in [1].)
>>> Wonderful, but I fail to see where the endian-ness of this is set
>>> anywhere. Am I just missing that? The regmap api enforces this idea,
>>> and so the
>>>
>>>> This generates a structure with the relevant accessors; we can also implement
>>>> additional logic, such as:
>>>>
>>>> impl NV_PMC_BOOT_0 {
>>>> /// Combines `architecture_0` and `architecture_1` to obtain the architecture of the chip.
>>>> pub(crate) fn architecture(self) -> Result<Architecture> {
>>>> Architecture::try_from(
>>>> self.architecture_0() | (self.architecture_1() << Self::ARCHITECTURE_0_RANGE.len()),
>>>> )
>>>> }
>>>>
>>>> /// Combines `architecture` and `implementation` to obtain a code unique to the chipset.
>>>> pub(crate) fn chipset(self) -> Result<Chipset> {
>>>> self.architecture()
>>>> .map(|arch| {
>>>> ((arch as u32) << Self::IMPLEMENTATION_RANGE.len())
>>>> | u32::from(self.implementation())
>>>> })
>>>> .and_then(Chipset::try_from)
>>>> }
>>>> }
>>>>
>>>> This conviniently allows us to read the register with
>>>>
>>>> let boot0 = regs::NV_PMC_BOOT_0::read(bar);
>>>>
>>>> and obtain an instance of the entire Chipset structure with
>>>>
>>>> let chipset = boot0.chipset()?;
>>>>
>>>> or pass it to a constructor that creates a Revision instance
>>>>
>>>> let rev = Revision::from_boot0(boot0);
>>>>
>>>> Analogously it allows us to modify and write registers without having to mess
>>>> with error prone shifts, masks and casts, because that code is generated by the
>>>> register!() macro. (Of course, unless we have more complicated cases where
>>>> multiple fields have to be combined as illustrated above.)
>>>>
>>>> Note that bar is of type pci::Bar<BAR0_SIZE> where BAR0_SIZE in our case is
>>>> SZ_16M.
>>>>
>>>> However, the type required by read() as generated by the register!() macro
>>>> actually only requires something that implements an I/O backend, i.e
>>>> kernel::io::Io<SIZE>.
>>>>
>>>> pci::Bar is a specific implementation of kernel::io::Io.
>>>>
>>>> With this we can let the actual I/O backend handle the endianness of the bus.
>>> Ok, great, but right now it's not doing that from what I am seeing when
>>> reading the code. Shouldn't IoMem::new() take that as an argument?
>>>
>>> But, that feels odd as our current iomem api in C doesn't care about
>>> endian issues at all because it "assumes" that the caller has already
>>> handle this properly and all that the caller "wants" is to write/read to
>>> some memory chunk and not twiddle bits.
>>>
>>>> (Actually, we could even implement an I/O backend that uses regmap.)
>>> That would probably be best to do eventually as most platform drivers
>>> use regmap today as it's the sanest api we have at the moment.
>>>
>>>> So, I think the register!() stuff is rather orthogonal.
>>> I think it's very relevant as people seem to just be "assuming" that all
>>> the world (hardware and cpus) are little-endian, while in reality, they
>>> are anything but. As proof, the code that uses this register!() logic
>>> today totally ignores endian issues and just assumes that it is both
>>> running on a little-endian system, AND the hardware is little-endian.
>>>
>>> As a crazy example, look at the USB host controllers that at runtime,
>>> have to be queried to determine what endian they are running on and the
>>> kernel drivers have to handle this "on the fly". Yes, one can argue
>>> that the hardware developers who came up with that should be forced to
>>> write the drivers as penance for such sins, but in the end, it's us that
>>> has to deal with it...
>>>
>>> So ignoring it will get us quite a ways forward with controlling sane
>>> hardware on sane systems, but when s390 finally realizes they can be
>>> writing their drivers in rust, we are going to have to have these
>>> conversations again :)
>> Hi Greg, all,
>>
>> Endianess is not the only problem when dealing with registers mapped
>> to the memory, right?
>>
>> I recall some built-in 12-bit ADCs in 8-bit AVR microcontrollers. That
>> required to read 4-bit LO register before 8-bit HI, if you didn't want to
>> loose those 4 bits.
>>
>> Bitfields don't address that issue as well. In my understanding, it's
>> done on purpose: bitfields encapsulate shifts and masks, and don't
>> pretend that they are suitable for direct access to a hardware.
>>
>> Notice another rust bitfield project. It tries to account for endianess
>> and everything else:
>>
>> https://docs.rs/bitfield-struct/latest/bitfield_struct/
>>
>> I didn't ask explicitly, and maybe it's a good time to ask now: Joel,
>> Danilo and everyone, have you considered adopting this project in
>> kernel?
>>
>> The bitfield_struct builds everything into the structure:
>>
>> use bitfield_struct::bitfield;
>>
>> #[bitfield(u8, order = Msb)]
>> struct MyMsbByte {
>> /// The first field occupies the *most* significant bits
>> #[bits(4)]
>> kind: usize,
>> system: bool,
>> #[bits(2)]
>> level: usize,
>> present: bool
>> }
> Thanks for raising this. The syntax seems quite different from what we need, in
> particular since register! macro is based on our bitfield! macro, this syntax is
> incompatible with the need to specify bit ranges, not just the number of bits.
> In other words, it appears the out-of-crate does not satisfy the requirement.
> They have to specific 'order' property mainly because they don't have the notion
> of bitfield index, just number of bits.
>
> Regarding endianness in that crate, it appears to be configurable based on
> user's requirement so we can make it such if needed for any kernel usecases. But
> the default in that crate is native-endianness just like our implementation right?
>
> Thanks.
>
You might be interested in an implementation that we worked on that just
uses `macro_rules`: https://docs.rs/bitfielder/latest/bitfielder/
It's not the same syntax as what you use, but you might be able to find
some inspiration in how we dealt with endianness. We also have
implementations for reading bitfields from byte arrays, if you're
interested.
Either way, thanks for your continued work on this patch series.
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-24 20:01 ` Elle Rhumsaa
@ 2025-09-25 7:05 ` Joel Fernandes
0 siblings, 0 replies; 44+ messages in thread
From: Joel Fernandes @ 2025-09-25 7:05 UTC (permalink / raw)
To: Elle Rhumsaa, Yury Norov, Greg KH
Cc: Danilo Krummrich, Benno Lossin, linux-kernel, rust-for-linux,
dri-devel, acourbot, Alistair Popple, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, bjorn3_gh, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Daniel Almeida, nouveau
On 9/24/2025 10:01 PM, Elle Rhumsaa wrote:
>
>>
>> On 9/24/2025 4:38 PM, Yury Norov wrote:
>>> On Wed, Sep 24, 2025 at 12:52:41PM +0200, Greg KH wrote:
>>>> On Sun, Sep 21, 2025 at 03:47:55PM +0200, Danilo Krummrich wrote:
>>>>> On Sun Sep 21, 2025 at 2:45 PM CEST, Greg KH wrote:
>>>>>> Again, regmap handles this all just fine, why not just make bindings to
>>>>>> that api here instead?
>>>>> The idea is to use this for the register!() macro, e.g.
>>>>>
>>>>> register!(NV_PMC_BOOT_0 @ 0x00000000, "Basic revision information about
>>>>> the GPU" {
>>>>> 28:24 architecture_0 as u8, "Lower bits of the architecture";
>>>>> 23:20 implementation as u8, "Implementation version of the
>>>>> architecture";
>>>>> 8:8 architecture_1 as u8, "MSB of the architecture";
>>>>> 7:4 major_revision as u8, "Major revision of the chip";
>>>>> 3:0 minor_revision as u8, "Minor revision of the chip";
>>>>> });
>>>>>
>>>>> (More examples in [1].)
>>>> Wonderful, but I fail to see where the endian-ness of this is set
>>>> anywhere. Am I just missing that? The regmap api enforces this idea,
>>>> and so the
>>>>
>>>>> This generates a structure with the relevant accessors; we can also implement
>>>>> additional logic, such as:
>>>>>
>>>>> impl NV_PMC_BOOT_0 {
>>>>> /// Combines `architecture_0` and `architecture_1` to obtain the
>>>>> architecture of the chip.
>>>>> pub(crate) fn architecture(self) -> Result<Architecture> {
>>>>> Architecture::try_from(
>>>>> self.architecture_0() | (self.architecture_1() <<
>>>>> Self::ARCHITECTURE_0_RANGE.len()),
>>>>> )
>>>>> }
>>>>>
>>>>> /// Combines `architecture` and `implementation` to obtain a code
>>>>> unique to the chipset.
>>>>> pub(crate) fn chipset(self) -> Result<Chipset> {
>>>>> self.architecture()
>>>>> .map(|arch| {
>>>>> ((arch as u32) << Self::IMPLEMENTATION_RANGE.len())
>>>>> | u32::from(self.implementation())
>>>>> })
>>>>> .and_then(Chipset::try_from)
>>>>> }
>>>>> }
>>>>>
>>>>> This conviniently allows us to read the register with
>>>>>
>>>>> let boot0 = regs::NV_PMC_BOOT_0::read(bar);
>>>>>
>>>>> and obtain an instance of the entire Chipset structure with
>>>>>
>>>>> let chipset = boot0.chipset()?;
>>>>>
>>>>> or pass it to a constructor that creates a Revision instance
>>>>>
>>>>> let rev = Revision::from_boot0(boot0);
>>>>>
>>>>> Analogously it allows us to modify and write registers without having to mess
>>>>> with error prone shifts, masks and casts, because that code is generated by
>>>>> the
>>>>> register!() macro. (Of course, unless we have more complicated cases where
>>>>> multiple fields have to be combined as illustrated above.)
>>>>>
>>>>> Note that bar is of type pci::Bar<BAR0_SIZE> where BAR0_SIZE in our case is
>>>>> SZ_16M.
>>>>>
>>>>> However, the type required by read() as generated by the register!() macro
>>>>> actually only requires something that implements an I/O backend, i.e
>>>>> kernel::io::Io<SIZE>.
>>>>>
>>>>> pci::Bar is a specific implementation of kernel::io::Io.
>>>>>
>>>>> With this we can let the actual I/O backend handle the endianness of the bus.
>>>> Ok, great, but right now it's not doing that from what I am seeing when
>>>> reading the code. Shouldn't IoMem::new() take that as an argument?
>>>>
>>>> But, that feels odd as our current iomem api in C doesn't care about
>>>> endian issues at all because it "assumes" that the caller has already
>>>> handle this properly and all that the caller "wants" is to write/read to
>>>> some memory chunk and not twiddle bits.
>>>>
>>>>> (Actually, we could even implement an I/O backend that uses regmap.)
>>>> That would probably be best to do eventually as most platform drivers
>>>> use regmap today as it's the sanest api we have at the moment.
>>>>
>>>>> So, I think the register!() stuff is rather orthogonal.
>>>> I think it's very relevant as people seem to just be "assuming" that all
>>>> the world (hardware and cpus) are little-endian, while in reality, they
>>>> are anything but. As proof, the code that uses this register!() logic
>>>> today totally ignores endian issues and just assumes that it is both
>>>> running on a little-endian system, AND the hardware is little-endian.
>>>>
>>>> As a crazy example, look at the USB host controllers that at runtime,
>>>> have to be queried to determine what endian they are running on and the
>>>> kernel drivers have to handle this "on the fly". Yes, one can argue
>>>> that the hardware developers who came up with that should be forced to
>>>> write the drivers as penance for such sins, but in the end, it's us that
>>>> has to deal with it...
>>>>
>>>> So ignoring it will get us quite a ways forward with controlling sane
>>>> hardware on sane systems, but when s390 finally realizes they can be
>>>> writing their drivers in rust, we are going to have to have these
>>>> conversations again :)
>>> Hi Greg, all,
>>>
>>> Endianess is not the only problem when dealing with registers mapped
>>> to the memory, right?
>>>
>>> I recall some built-in 12-bit ADCs in 8-bit AVR microcontrollers. That
>>> required to read 4-bit LO register before 8-bit HI, if you didn't want to
>>> loose those 4 bits.
>>>
>>> Bitfields don't address that issue as well. In my understanding, it's
>>> done on purpose: bitfields encapsulate shifts and masks, and don't
>>> pretend that they are suitable for direct access to a hardware.
>>>
>>> Notice another rust bitfield project. It tries to account for endianess
>>> and everything else:
>>>
>>> https://docs.rs/bitfield-struct/latest/bitfield_struct/
>>>
>>> I didn't ask explicitly, and maybe it's a good time to ask now: Joel,
>>> Danilo and everyone, have you considered adopting this project in
>>> kernel?
>>>
>>> The bitfield_struct builds everything into the structure:
>>>
>>> use bitfield_struct::bitfield;
>>> #[bitfield(u8, order = Msb)]
>>> struct MyMsbByte {
>>> /// The first field occupies the *most* significant bits
>>> #[bits(4)]
>>> kind: usize,
>>> system: bool,
>>> #[bits(2)]
>>> level: usize,
>>> present: bool
>>> }
>> Thanks for raising this. The syntax seems quite different from what we need, in
>> particular since register! macro is based on our bitfield! macro, this syntax is
>> incompatible with the need to specify bit ranges, not just the number of bits.
>> In other words, it appears the out-of-crate does not satisfy the requirement.
>> They have to specific 'order' property mainly because they don't have the notion
>> of bitfield index, just number of bits.
>>
>> Regarding endianness in that crate, it appears to be configurable based on
>> user's requirement so we can make it such if needed for any kernel usecases. But
>> the default in that crate is native-endianness just like our implementation
>> right?
>>
>> Thanks.
>>
> You might be interested in an implementation that we worked on that just uses
> `macro_rules`: https://docs.rs/bitfielder/latest/bitfielder/
>
> It's not the same syntax as what you use, but you might be able to find some
> inspiration in how we dealt with endianness. We also have implementations for
> reading bitfields from byte arrays, if you're interested.
Looks similar. About the additional features, in time we can certainly add
support for it if it makes sense. I am sure we will improve support for
additional use cases after it is merged.
>
> Either way, thanks for your continued work on this patch series.
Of course! Will have the v5 out soon, but I am also traveling international to
get back home at the moment :).
- Joel
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-21 12:45 ` Greg KH
2025-09-21 13:47 ` Danilo Krummrich
@ 2025-09-23 22:24 ` Joel Fernandes
2025-09-24 10:40 ` Greg KH
1 sibling, 1 reply; 44+ messages in thread
From: Joel Fernandes @ 2025-09-23 22:24 UTC (permalink / raw)
To: Greg KH
Cc: Benno Lossin, linux-kernel, rust-for-linux, dri-devel, dakr,
acourbot, Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, bjorn3_gh, Andreas Hindborg, Alice Ryhl, Trevor Gross,
David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, John Hubbard, Timur Tabi, joel, Elle Rhumsaa,
Yury Norov, Daniel Almeida, nouveau
Hi Greg,
On Sun, Sep 21, 2025 at 02:45:27PM +0200, Greg KH wrote:
> On Sun, Sep 21, 2025 at 02:33:56PM +0200, Benno Lossin wrote:
> > On Sun Sep 21, 2025 at 11:36 AM CEST, Greg KH wrote:
> > > On Sat, Sep 20, 2025 at 02:22:27PM -0400, Joel Fernandes wrote:
> > >> The bitfield-specific into new macro. This will be used to define
> > >> structs with bitfields, similar to C language.
> > >>
> > >> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
> > >> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> > >> ---
> > >> drivers/gpu/nova-core/bitfield.rs | 314 +++++++++++++++++++++++++++
> > >> drivers/gpu/nova-core/nova_core.rs | 3 +
> > >> drivers/gpu/nova-core/regs/macros.rs | 259 +---------------------
> > >> 3 files changed, 327 insertions(+), 249 deletions(-)
> > >> create mode 100644 drivers/gpu/nova-core/bitfield.rs
> > >>
> > >> diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs
> > >> new file mode 100644
> > >> index 000000000000..ba6b7caa05d9
> > >> --- /dev/null
> > >> +++ b/drivers/gpu/nova-core/bitfield.rs
> > >> @@ -0,0 +1,314 @@
> > >> +// SPDX-License-Identifier: GPL-2.0
> > >> +
> > >> +//! Bitfield library for Rust structures
> > >> +//!
> > >> +//! Support for defining bitfields in Rust structures. Also used by the [`register!`] macro.
> > >> +//!
> > >> +//! # Syntax
> > >> +//!
> > >> +//! ```rust
> > >> +//! #[derive(Debug, Clone, Copy)]
> > >> +//! enum Mode {
> > >> +//! Low = 0,
> > >> +//! High = 1,
> > >> +//! Auto = 2,
> > >> +//! }
> > >> +//!
> > >> +//! impl TryFrom<u8> for Mode {
> > >> +//! type Error = u8;
> > >> +//! fn try_from(value: u8) -> Result<Self, Self::Error> {
> > >> +//! match value {
> > >> +//! 0 => Ok(Mode::Low),
> > >> +//! 1 => Ok(Mode::High),
> > >> +//! 2 => Ok(Mode::Auto),
> > >> +//! _ => Err(value),
> > >> +//! }
> > >> +//! }
> > >> +//! }
> > >> +//!
> > >> +//! impl From<Mode> for u32 {
> > >> +//! fn from(mode: Mode) -> u32 {
> > >> +//! mode as u32
> > >> +//! }
> > >> +//! }
> > >> +//!
> > >> +//! #[derive(Debug, Clone, Copy)]
> > >> +//! enum State {
> > >> +//! Inactive = 0,
> > >> +//! Active = 1,
> > >> +//! }
> > >> +//!
> > >> +//! impl From<bool> for State {
> > >> +//! fn from(value: bool) -> Self {
> > >> +//! if value { State::Active } else { State::Inactive }
> > >> +//! }
> > >> +//! }
> > >> +//!
> > >> +//! impl From<State> for u32 {
> > >> +//! fn from(state: State) -> u32 {
> > >> +//! state as u32
> > >> +//! }
> > >> +//! }
> > >> +//!
> > >> +//! bitfield! {
> > >> +//! struct ControlReg {
> > >> +//! 3:0 mode as u8 ?=> Mode;
> > >> +//! 7 state as bool => State;
> > >> +//! }
> > >> +//! }
> > >
> > > As discussed at the conference this week, I do object to this as it
> > > will allow the same mistakes to happen that we used to do in the kernel
> > > for a long time before the regmap() api happened, along with GENMASK().
> >
> > Have you read the following macro arm of the implementation?
> >
> > // Generates the accessor methods for a single field.
> > (
> > @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident
> > { $process:expr } $to_type:ty => $res_type:ty $(, $comment:literal)?;
> > ) => {
> > ::kernel::macros::paste!(
> > const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
> > const [<$field:upper _MASK>]: u32 = ((((1 << $hi) - 1) << 1) + 1) - ((1 << $lo) - 1);
> > const [<$field:upper _SHIFT>]: u32 = Self::[<$field:upper _MASK>].trailing_zeros();
> > );
> >
> > $(
> > #[doc="Returns the value of this field:"]
> > #[doc=$comment]
> > )?
> > #[inline(always)]
> > pub(crate) fn $field(self) -> $res_type {
> > ::kernel::macros::paste!(
> > const MASK: u32 = $name::[<$field:upper _MASK>];
> > const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
> > );
> > let field = ((self.0 & MASK) >> SHIFT);
> >
> > Here you can see that it's just a mask + shift operation internally to
> > access the field.
> >
> > $process(field)
> > }
> >
> > ::kernel::macros::paste!(
> > $(
> > #[doc="Sets the value of this field:"]
> > #[doc=$comment]
> > )?
> > #[inline(always)]
> > pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
> > const MASK: u32 = $name::[<$field:upper _MASK>];
> > const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
> > let value = (u32::from(value) << SHIFT) & MASK;
> > self.0 = (self.0 & !MASK) | value;
> >
> > self
> > }
> > );
> > };
>
> Yes, that's great, but that is all done in "native cpu" endian, and
> might not actually represent what the hardware does at all, which is
> what I was trying to get at here, sorry for not being more specific.
>
> > Now I too would like to see how exactly this will be used to read data
> > from hardware. But at least in theory if the conversion from hardware
> > endianness to native endianness is done correctly, this will do the
> > right thing :)
>
> That's great, so we are close, but it's not quite correct. How about
> something like:
>
> 0:7 reg_X as __le32
> 8:15 reg_y as __le32
I don't think we should force endianness requirements within specific fields in
the bitfield rust library itself, it is upto the user. bitfields are not only
for registers even in C. If you see on the C side, we have rcu_special union
which uses 'u32' and does not enforce endianness within the fields or bytes
of the struct with respect to the fields. Its all native CPU endian and works
fine. You're basically saying in terms of C that, the designers of the C
bitfield in C standard force the C language to use endianness in the types, no
they can't / shouldn't be forced to.
For the separate issue of enforcing endianness with respect to (across)
multiple fields, I agree with you that if the user's backend (the consumer of
the data) is not doing such conversion, say via regmap, then that becomes a
problem. But that problem is orthogonal/different and cannot be solved here.
> and the like. There has to be a way to actually specify the
> representation here and for C, that is using the "__" types that express
> the endianness (i.e. __le32, __be32, and so on). Then we have type
> checks that enforce accesses to those variables to always turn the data
> into "native" values when the cpu accesses them.
Sure, it is upto the user, they can decide to use whatever types they want.
In fact we also support conversion to enums, not just integer types :)
thanks,
- Joel
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-23 22:24 ` Joel Fernandes
@ 2025-09-24 10:40 ` Greg KH
2025-09-29 19:26 ` Joel Fernandes
0 siblings, 1 reply; 44+ messages in thread
From: Greg KH @ 2025-09-24 10:40 UTC (permalink / raw)
To: Joel Fernandes
Cc: Benno Lossin, linux-kernel, rust-for-linux, dri-devel, dakr,
acourbot, Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, bjorn3_gh, Andreas Hindborg, Alice Ryhl, Trevor Gross,
David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, John Hubbard, Timur Tabi, joel, Elle Rhumsaa,
Yury Norov, Daniel Almeida, nouveau
On Tue, Sep 23, 2025 at 06:24:34PM -0400, Joel Fernandes wrote:
> Hi Greg,
>
> On Sun, Sep 21, 2025 at 02:45:27PM +0200, Greg KH wrote:
> > On Sun, Sep 21, 2025 at 02:33:56PM +0200, Benno Lossin wrote:
> > > On Sun Sep 21, 2025 at 11:36 AM CEST, Greg KH wrote:
> > > > On Sat, Sep 20, 2025 at 02:22:27PM -0400, Joel Fernandes wrote:
> > > >> The bitfield-specific into new macro. This will be used to define
> > > >> structs with bitfields, similar to C language.
> > > >>
> > > >> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
> > > >> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> > > >> ---
> > > >> drivers/gpu/nova-core/bitfield.rs | 314 +++++++++++++++++++++++++++
> > > >> drivers/gpu/nova-core/nova_core.rs | 3 +
> > > >> drivers/gpu/nova-core/regs/macros.rs | 259 +---------------------
> > > >> 3 files changed, 327 insertions(+), 249 deletions(-)
> > > >> create mode 100644 drivers/gpu/nova-core/bitfield.rs
> > > >>
> > > >> diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs
> > > >> new file mode 100644
> > > >> index 000000000000..ba6b7caa05d9
> > > >> --- /dev/null
> > > >> +++ b/drivers/gpu/nova-core/bitfield.rs
> > > >> @@ -0,0 +1,314 @@
> > > >> +// SPDX-License-Identifier: GPL-2.0
> > > >> +
> > > >> +//! Bitfield library for Rust structures
> > > >> +//!
> > > >> +//! Support for defining bitfields in Rust structures. Also used by the [`register!`] macro.
> > > >> +//!
> > > >> +//! # Syntax
> > > >> +//!
> > > >> +//! ```rust
> > > >> +//! #[derive(Debug, Clone, Copy)]
> > > >> +//! enum Mode {
> > > >> +//! Low = 0,
> > > >> +//! High = 1,
> > > >> +//! Auto = 2,
> > > >> +//! }
> > > >> +//!
> > > >> +//! impl TryFrom<u8> for Mode {
> > > >> +//! type Error = u8;
> > > >> +//! fn try_from(value: u8) -> Result<Self, Self::Error> {
> > > >> +//! match value {
> > > >> +//! 0 => Ok(Mode::Low),
> > > >> +//! 1 => Ok(Mode::High),
> > > >> +//! 2 => Ok(Mode::Auto),
> > > >> +//! _ => Err(value),
> > > >> +//! }
> > > >> +//! }
> > > >> +//! }
> > > >> +//!
> > > >> +//! impl From<Mode> for u32 {
> > > >> +//! fn from(mode: Mode) -> u32 {
> > > >> +//! mode as u32
> > > >> +//! }
> > > >> +//! }
> > > >> +//!
> > > >> +//! #[derive(Debug, Clone, Copy)]
> > > >> +//! enum State {
> > > >> +//! Inactive = 0,
> > > >> +//! Active = 1,
> > > >> +//! }
> > > >> +//!
> > > >> +//! impl From<bool> for State {
> > > >> +//! fn from(value: bool) -> Self {
> > > >> +//! if value { State::Active } else { State::Inactive }
> > > >> +//! }
> > > >> +//! }
> > > >> +//!
> > > >> +//! impl From<State> for u32 {
> > > >> +//! fn from(state: State) -> u32 {
> > > >> +//! state as u32
> > > >> +//! }
> > > >> +//! }
> > > >> +//!
> > > >> +//! bitfield! {
> > > >> +//! struct ControlReg {
> > > >> +//! 3:0 mode as u8 ?=> Mode;
> > > >> +//! 7 state as bool => State;
> > > >> +//! }
> > > >> +//! }
> > > >
> > > > As discussed at the conference this week, I do object to this as it
> > > > will allow the same mistakes to happen that we used to do in the kernel
> > > > for a long time before the regmap() api happened, along with GENMASK().
> > >
> > > Have you read the following macro arm of the implementation?
> > >
> > > // Generates the accessor methods for a single field.
> > > (
> > > @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident
> > > { $process:expr } $to_type:ty => $res_type:ty $(, $comment:literal)?;
> > > ) => {
> > > ::kernel::macros::paste!(
> > > const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
> > > const [<$field:upper _MASK>]: u32 = ((((1 << $hi) - 1) << 1) + 1) - ((1 << $lo) - 1);
> > > const [<$field:upper _SHIFT>]: u32 = Self::[<$field:upper _MASK>].trailing_zeros();
> > > );
> > >
> > > $(
> > > #[doc="Returns the value of this field:"]
> > > #[doc=$comment]
> > > )?
> > > #[inline(always)]
> > > pub(crate) fn $field(self) -> $res_type {
> > > ::kernel::macros::paste!(
> > > const MASK: u32 = $name::[<$field:upper _MASK>];
> > > const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
> > > );
> > > let field = ((self.0 & MASK) >> SHIFT);
> > >
> > > Here you can see that it's just a mask + shift operation internally to
> > > access the field.
> > >
> > > $process(field)
> > > }
> > >
> > > ::kernel::macros::paste!(
> > > $(
> > > #[doc="Sets the value of this field:"]
> > > #[doc=$comment]
> > > )?
> > > #[inline(always)]
> > > pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
> > > const MASK: u32 = $name::[<$field:upper _MASK>];
> > > const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
> > > let value = (u32::from(value) << SHIFT) & MASK;
> > > self.0 = (self.0 & !MASK) | value;
> > >
> > > self
> > > }
> > > );
> > > };
> >
> > Yes, that's great, but that is all done in "native cpu" endian, and
> > might not actually represent what the hardware does at all, which is
> > what I was trying to get at here, sorry for not being more specific.
> >
> > > Now I too would like to see how exactly this will be used to read data
> > > from hardware. But at least in theory if the conversion from hardware
> > > endianness to native endianness is done correctly, this will do the
> > > right thing :)
> >
> > That's great, so we are close, but it's not quite correct. How about
> > something like:
> >
> > 0:7 reg_X as __le32
> > 8:15 reg_y as __le32
>
> I don't think we should force endianness requirements within specific fields in
> the bitfield rust library itself, it is upto the user. bitfields are not only
> for registers even in C. If you see on the C side, we have rcu_special union
> which uses 'u32' and does not enforce endianness within the fields or bytes
> of the struct with respect to the fields. Its all native CPU endian and works
> fine. You're basically saying in terms of C that, the designers of the C
> bitfield in C standard force the C language to use endianness in the types, no
> they can't / shouldn't be forced to.
For "cpu native" structures, just use the bit and genmask macros we have
today, on top of a normal variable type and you should be fine. The
only place you need/want to do stuff like what is being proposed here is
when you are trying to match up a data structure that is in hardware to
be able to split the values out of it safely.
And when dealing with data that goes outside of the kernel (i.e. to/from
hardware), you HAVE to specify the endianness of that data as the
hardware is the one that defines this, NOT the cpu that the kernel is
running on.
So you should NEVER see a bitfield structure that is defined just as a
"u32" and expect that to work properly when read/written to hardware
because while little-endian is what seems to have "won" the recent
battles on this topic it's not always the case for many places that
Linux runs on today.
> For the separate issue of enforcing endianness with respect to (across)
> multiple fields, I agree with you that if the user's backend (the consumer of
> the data) is not doing such conversion, say via regmap, then that becomes a
> problem. But that problem is orthogonal/different and cannot be solved here.
But that is exactly what these macros are being defined here for, so to
ignore that is going to cause problems :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-24 10:40 ` Greg KH
@ 2025-09-29 19:26 ` Joel Fernandes
2025-09-29 19:37 ` Greg KH
2025-09-29 20:25 ` Danilo Krummrich
0 siblings, 2 replies; 44+ messages in thread
From: Joel Fernandes @ 2025-09-29 19:26 UTC (permalink / raw)
To: Greg KH
Cc: Benno Lossin, linux-kernel, rust-for-linux, dri-devel, dakr,
acourbot, Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, bjorn3_gh, Andreas Hindborg, Alice Ryhl, Trevor Gross,
David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, John Hubbard, Timur Tabi, joel, Elle Rhumsaa,
Yury Norov, Daniel Almeida, nouveau
On 9/24/2025 12:40 PM, Greg KH wrote:
> On Tue, Sep 23, 2025 at 06:24:34PM -0400, Joel Fernandes wrote:
[..]
>
>> For the separate issue of enforcing endianness with respect to (across)
>> multiple fields, I agree with you that if the user's backend (the consumer of
>> the data) is not doing such conversion, say via regmap, then that becomes a
>> problem. But that problem is orthogonal/different and cannot be solved here.
>
> But that is exactly what these macros are being defined here for, so to
> ignore that is going to cause problems :)
>
If needed, happy to add endianness support as needed by providing additional
options to the macro. Based on this thread, it sounds like we want see if that
is really needed here or can be solved elsewhere (?). The mental model I kind of
have is this macro should only be dealing with CPU native endianness, much like
bitfields in C deal with CPU endianness. Hmm.
Thanks.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-29 19:26 ` Joel Fernandes
@ 2025-09-29 19:37 ` Greg KH
2025-09-29 19:45 ` Joel Fernandes
2025-09-29 20:25 ` Danilo Krummrich
1 sibling, 1 reply; 44+ messages in thread
From: Greg KH @ 2025-09-29 19:37 UTC (permalink / raw)
To: Joel Fernandes
Cc: Benno Lossin, linux-kernel, rust-for-linux, dri-devel, dakr,
acourbot, Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, bjorn3_gh, Andreas Hindborg, Alice Ryhl, Trevor Gross,
David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, John Hubbard, Timur Tabi, joel, Elle Rhumsaa,
Yury Norov, Daniel Almeida, nouveau
On Mon, Sep 29, 2025 at 03:26:57PM -0400, Joel Fernandes wrote:
> On 9/24/2025 12:40 PM, Greg KH wrote:
> > On Tue, Sep 23, 2025 at 06:24:34PM -0400, Joel Fernandes wrote:
> [..]
> >
> >> For the separate issue of enforcing endianness with respect to (across)
> >> multiple fields, I agree with you that if the user's backend (the consumer of
> >> the data) is not doing such conversion, say via regmap, then that becomes a
> >> problem. But that problem is orthogonal/different and cannot be solved here.
> >
> > But that is exactly what these macros are being defined here for, so to
> > ignore that is going to cause problems :)
> >
>
> If needed, happy to add endianness support as needed by providing additional
> options to the macro. Based on this thread, it sounds like we want see if that
> is really needed here or can be solved elsewhere (?). The mental model I kind of
> have is this macro should only be dealing with CPU native endianness, much like
> bitfields in C deal with CPU endianness. Hmm.
Just don't go down the old path like drivers/net/fddi/skfp/h/supern_2.h
does with it's definition of:
union tx_descr {
struct {
#ifdef LITTLE_ENDIAN
unsigned int tx_length:16 ; /* frame length lower/upper byte */
unsigned int tx_res :8 ; /* reserved (bit 16..23) */
unsigned int tx_xmtabt:1 ; /* transmit abort */
unsigned int tx_nfcs :1 ; /* no frame check sequence */
unsigned int tx_xdone :1 ; /* give up token */
unsigned int tx_rpxm :2 ; /* byte offset */
unsigned int tx_pat1 :2 ; /* must be TXP1 */
unsigned int tx_more :1 ; /* more frame in chain */
#else
unsigned int tx_more :1 ; /* more frame in chain */
unsigned int tx_pat1 :2 ; /* must be TXP1 */
unsigned int tx_rpxm :2 ; /* byte offset */
unsigned int tx_xdone :1 ; /* give up token */
unsigned int tx_nfcs :1 ; /* no frame check sequence */
unsigned int tx_xmtabt:1 ; /* transmit abort */
unsigned int tx_res :8 ; /* reserved (bit 16..23) */
unsigned int tx_length:16 ; /* frame length lower/upper byte */
#endif
} t ;
long i ;
} ;
CPU endinness, hah, as if...
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-29 19:37 ` Greg KH
@ 2025-09-29 19:45 ` Joel Fernandes
0 siblings, 0 replies; 44+ messages in thread
From: Joel Fernandes @ 2025-09-29 19:45 UTC (permalink / raw)
To: Greg KH
Cc: Benno Lossin, linux-kernel, rust-for-linux, dri-devel, dakr,
acourbot, Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, bjorn3_gh, Andreas Hindborg, Alice Ryhl, Trevor Gross,
David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, John Hubbard, Timur Tabi, joel, Elle Rhumsaa,
Yury Norov, Daniel Almeida, nouveau
On 9/29/2025 9:37 PM, Greg KH wrote:
> On Mon, Sep 29, 2025 at 03:26:57PM -0400, Joel Fernandes wrote:
>> On 9/24/2025 12:40 PM, Greg KH wrote:
>>> On Tue, Sep 23, 2025 at 06:24:34PM -0400, Joel Fernandes wrote:
>> [..]
>>>
>>>> For the separate issue of enforcing endianness with respect to (across)
>>>> multiple fields, I agree with you that if the user's backend (the consumer of
>>>> the data) is not doing such conversion, say via regmap, then that becomes a
>>>> problem. But that problem is orthogonal/different and cannot be solved here.
>>>
>>> But that is exactly what these macros are being defined here for, so to
>>> ignore that is going to cause problems :)
>>>
>>
>> If needed, happy to add endianness support as needed by providing additional
>> options to the macro. Based on this thread, it sounds like we want see if that
>> is really needed here or can be solved elsewhere (?). The mental model I kind of
>> have is this macro should only be dealing with CPU native endianness, much like
>> bitfields in C deal with CPU endianness. Hmm.
>
> Just don't go down the old path like drivers/net/fddi/skfp/h/supern_2.h
> does with it's definition of:
But this is not a comparable example though because in our macro we specify
individual bit numbers, not just bit width. So tx_length for example would
always start at the lower 2 bytes since we'd specify "tx_length 0:15" regardless
of whether those 2 bytes are higher in memory or lower in memory. Whether it is
higher or lower depends on CPU endianness AFAICS, and if interacting with HW is
needed, the user of the macro would do endianness conversion. Maybe we should
add a comment about such conversion requirements?
Then there is the issue of the byte ordering within a multi-byte field. That
again follows CPU endianness, and we could add a comment for the benefit of
macro user.
>
> union tx_descr {
> struct {
> #ifdef LITTLE_ENDIAN
> unsigned int tx_length:16 ; /* frame length lower/upper byte */
> unsigned int tx_res :8 ; /* reserved (bit 16..23) */
> unsigned int tx_xmtabt:1 ; /* transmit abort */
> unsigned int tx_nfcs :1 ; /* no frame check sequence */
> unsigned int tx_xdone :1 ; /* give up token */
> unsigned int tx_rpxm :2 ; /* byte offset */
> unsigned int tx_pat1 :2 ; /* must be TXP1 */
> unsigned int tx_more :1 ; /* more frame in chain */
> #else
> unsigned int tx_more :1 ; /* more frame in chain */
> unsigned int tx_pat1 :2 ; /* must be TXP1 */
> unsigned int tx_rpxm :2 ; /* byte offset */
> unsigned int tx_xdone :1 ; /* give up token */
> unsigned int tx_nfcs :1 ; /* no frame check sequence */
> unsigned int tx_xmtabt:1 ; /* transmit abort */
> unsigned int tx_res :8 ; /* reserved (bit 16..23) */
> unsigned int tx_length:16 ; /* frame length lower/upper byte */
> #endif
> } t ;
> long i ;
> } ;
>
This is indeed yuck though but afaics not what hopefully we'd be doing ;-)
thanks,
- Joel
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-29 19:26 ` Joel Fernandes
2025-09-29 19:37 ` Greg KH
@ 2025-09-29 20:25 ` Danilo Krummrich
1 sibling, 0 replies; 44+ messages in thread
From: Danilo Krummrich @ 2025-09-29 20:25 UTC (permalink / raw)
To: Joel Fernandes
Cc: Greg KH, Benno Lossin, linux-kernel, rust-for-linux, dri-devel,
acourbot, Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, bjorn3_gh, Andreas Hindborg, Alice Ryhl, Trevor Gross,
David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, John Hubbard, Timur Tabi, joel, Elle Rhumsaa,
Yury Norov, Daniel Almeida, nouveau
On Mon Sep 29, 2025 at 9:26 PM CEST, Joel Fernandes wrote:
> On 9/24/2025 12:40 PM, Greg KH wrote:
>> On Tue, Sep 23, 2025 at 06:24:34PM -0400, Joel Fernandes wrote:
> [..]
>>
>>> For the separate issue of enforcing endianness with respect to (across)
>>> multiple fields, I agree with you that if the user's backend (the consumer of
>>> the data) is not doing such conversion, say via regmap, then that becomes a
>>> problem. But that problem is orthogonal/different and cannot be solved here.
>>
>> But that is exactly what these macros are being defined here for, so to
>> ignore that is going to cause problems :)
>>
>
> If needed, happy to add endianness support as needed by providing additional
> options to the macro. Based on this thread, it sounds like we want see if that
> is really needed here or can be solved elsewhere (?). The mental model I kind of
> have is this macro should only be dealing with CPU native endianness, much like
> bitfields in C deal with CPU endianness. Hmm.
At least for register!() we don't need anything else than CPU endianness. In
fact, as described in [1], any representation is fine as long as it is
consistent.
[1] https://lore.kernel.org/all/DD0ZTZM8S84H.1YDWSY7DF14LM@kernel.org/
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-21 9:36 ` Greg KH
2025-09-21 9:59 ` Miguel Ojeda
2025-09-21 12:33 ` Benno Lossin
@ 2025-09-21 13:49 ` Danilo Krummrich
2 siblings, 0 replies; 44+ messages in thread
From: Danilo Krummrich @ 2025-09-21 13:49 UTC (permalink / raw)
To: Greg KH
Cc: Joel Fernandes, linux-kernel, rust-for-linux, dri-devel, acourbot,
Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On Sun Sep 21, 2025 at 11:36 AM CEST, Greg KH wrote:
> Your example code using this is nice, and it shows how to set up, and
> query these bits, but that's not anything anyone actually does in the
> kernel, what they want to do is read/write from hardware with this.
>
> So, how does that work? Where does this "drop down" to the native
> bus/memory transactions and swizzle the bits properly to work correctly?
Please see reply in [1], let's continue discussing it there. :)
[1] https://lore.kernel.org/lkml/DCYIX8URVIWM.2ZK3GHH3J82XQ@kernel.org/
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-20 18:22 ` [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro Joel Fernandes
2025-09-21 9:36 ` Greg KH
@ 2025-09-29 6:16 ` Alexandre Courbot
2025-09-29 19:36 ` Joel Fernandes
1 sibling, 1 reply; 44+ messages in thread
From: Alexandre Courbot @ 2025-09-29 6:16 UTC (permalink / raw)
To: Joel Fernandes, linux-kernel, rust-for-linux, dri-devel, dakr,
acourbot
Cc: Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On Sun Sep 21, 2025 at 3:22 AM JST, Joel Fernandes wrote:
> The bitfield-specific into new macro. This will be used to define
> structs with bitfields, similar to C language.
>
> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
Very clean. One nit remains below, in any case:
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> drivers/gpu/nova-core/bitfield.rs | 314 +++++++++++++++++++++++++++
> drivers/gpu/nova-core/nova_core.rs | 3 +
> drivers/gpu/nova-core/regs/macros.rs | 259 +---------------------
> 3 files changed, 327 insertions(+), 249 deletions(-)
> create mode 100644 drivers/gpu/nova-core/bitfield.rs
>
> diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs
> new file mode 100644
> index 000000000000..ba6b7caa05d9
> --- /dev/null
> +++ b/drivers/gpu/nova-core/bitfield.rs
> @@ -0,0 +1,314 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Bitfield library for Rust structures
> +//!
> +//! Support for defining bitfields in Rust structures. Also used by the [`register!`] macro.
> +//!
> +//! # Syntax
> +//!
> +//! ```rust
> +//! #[derive(Debug, Clone, Copy)]
> +//! enum Mode {
> +//! Low = 0,
> +//! High = 1,
> +//! Auto = 2,
> +//! }
> +//!
> +//! impl TryFrom<u8> for Mode {
> +//! type Error = u8;
> +//! fn try_from(value: u8) -> Result<Self, Self::Error> {
> +//! match value {
> +//! 0 => Ok(Mode::Low),
> +//! 1 => Ok(Mode::High),
> +//! 2 => Ok(Mode::Auto),
> +//! _ => Err(value),
> +//! }
> +//! }
> +//! }
> +//!
> +//! impl From<Mode> for u32 {
> +//! fn from(mode: Mode) -> u32 {
> +//! mode as u32
> +//! }
> +//! }
Jesung's `TryFrom` and `Into` derive macros [1] would be greatly useful
here, I hope we can merge them soon.
[1] https://lore.kernel.org/all/cover.1755235180.git.y.j3ms.n@gmail.com/
> +//!
> +//! #[derive(Debug, Clone, Copy)]
> +//! enum State {
> +//! Inactive = 0,
> +//! Active = 1,
> +//! }
> +//!
> +//! impl From<bool> for State {
> +//! fn from(value: bool) -> Self {
> +//! if value { State::Active } else { State::Inactive }
> +//! }
> +//! }
> +//!
> +//! impl From<State> for u32 {
> +//! fn from(state: State) -> u32 {
> +//! state as u32
> +//! }
> +//! }
> +//!
> +//! bitfield! {
> +//! struct ControlReg {
> +//! 3:0 mode as u8 ?=> Mode;
> +//! 7 state as bool => State;
> +//! }
> +//! }
> +//! ```
> +//!
> +//! This generates a struct with:
> +//! - Field accessors: `mode()`, `state()`, etc.
> +//! - Field setters: `set_mode()`, `set_state()`, etc. (supports chaining with 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);
Missing code block for the example?
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-29 6:16 ` Alexandre Courbot
@ 2025-09-29 19:36 ` Joel Fernandes
0 siblings, 0 replies; 44+ messages in thread
From: Joel Fernandes @ 2025-09-29 19:36 UTC (permalink / raw)
To: Alexandre Courbot, linux-kernel, rust-for-linux, dri-devel, dakr
Cc: Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On 9/29/2025 8:16 AM, Alexandre Courbot wrote:
> On Sun Sep 21, 2025 at 3:22 AM JST, Joel Fernandes wrote:
>> The bitfield-specific into new macro. This will be used to define
>> structs with bitfields, similar to C language.
>>
>> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>
> Very clean. One nit remains below, in any case:
>
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
Thanks.
>
>> ---
>> drivers/gpu/nova-core/bitfield.rs | 314 +++++++++++++++++++++++++++
>> drivers/gpu/nova-core/nova_core.rs | 3 +
>> drivers/gpu/nova-core/regs/macros.rs | 259 +---------------------
>> 3 files changed, 327 insertions(+), 249 deletions(-)
>> create mode 100644 drivers/gpu/nova-core/bitfield.rs
>>
>> diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs
>> new file mode 100644
>> index 000000000000..ba6b7caa05d9
>> --- /dev/null
>> +++ b/drivers/gpu/nova-core/bitfield.rs
>> @@ -0,0 +1,314 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Bitfield library for Rust structures
>> +//!
>> +//! Support for defining bitfields in Rust structures. Also used by the [`register!`] macro.
>> +//!
>> +//! # Syntax
>> +//!
>> +//! ```rust
>> +//! #[derive(Debug, Clone, Copy)]
>> +//! enum Mode {
>> +//! Low = 0,
>> +//! High = 1,
>> +//! Auto = 2,
>> +//! }
>> +//!
>> +//! impl TryFrom<u8> for Mode {
>> +//! type Error = u8;
>> +//! fn try_from(value: u8) -> Result<Self, Self::Error> {
>> +//! match value {
>> +//! 0 => Ok(Mode::Low),
>> +//! 1 => Ok(Mode::High),
>> +//! 2 => Ok(Mode::Auto),
>> +//! _ => Err(value),
>> +//! }
>> +//! }
>> +//! }
>> +//!
>> +//! impl From<Mode> for u32 {
>> +//! fn from(mode: Mode) -> u32 {
>> +//! mode as u32
>> +//! }
>> +//! }
>
> Jesung's `TryFrom` and `Into` derive macros [1] would be greatly useful
> here, I hope we can merge them soon.
>
> [1] https://lore.kernel.org/all/cover.1755235180.git.y.j3ms.n@gmail.com/
Ah nice, looking forward to it!!
>
>> +//!
>> +//! #[derive(Debug, Clone, Copy)]
>> +//! enum State {
>> +//! Inactive = 0,
>> +//! Active = 1,
>> +//! }
>> +//!
>> +//! impl From<bool> for State {
>> +//! fn from(value: bool) -> Self {
>> +//! if value { State::Active } else { State::Inactive }
>> +//! }
>> +//! }
>> +//!
>> +//! impl From<State> for u32 {
>> +//! fn from(state: State) -> u32 {
>> +//! state as u32
>> +//! }
>> +//! }
>> +//!
>> +//! bitfield! {
>> +//! struct ControlReg {
>> +//! 3:0 mode as u8 ?=> Mode;
>> +//! 7 state as bool => State;
>> +//! }
>> +//! }
>> +//! ```
>> +//!
>> +//! This generates a struct with:
>> +//! - Field accessors: `mode()`, `state()`, etc.
>> +//! - Field setters: `set_mode()`, `set_state()`, etc. (supports chaining with 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);
>
> Missing code block for the example?
>
Ah, sure, I will add the setter into an example as well.
- Joel
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v4 2/6] nova-core: bitfield: Add support for different storage widths
2025-09-20 18:22 [PATCH v4 0/6] Introduce bitfield and move register macro to rust/kernel/ Joel Fernandes
2025-09-20 18:22 ` [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro Joel Fernandes
@ 2025-09-20 18:22 ` Joel Fernandes
2025-09-29 6:22 ` Alexandre Courbot
2025-09-20 18:22 ` [PATCH v4 3/6] nova-core: bitfield: Add support for custom visiblity Joel Fernandes
` (3 subsequent siblings)
5 siblings, 1 reply; 44+ messages in thread
From: Joel Fernandes @ 2025-09-20 18:22 UTC (permalink / raw)
To: linux-kernel, rust-for-linux, dri-devel, dakr, acourbot
Cc: Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Joel Fernandes,
Timur Tabi, joel, Elle Rhumsaa, Yury Norov, Daniel Almeida,
nouveau
Previously, bitfields were hardcoded to use u32 as the underlying
storage type. Add support for different storage types (u8, u16, u32,
u64) to the bitfield macro.
New syntax is: struct Name: <type ex., u32> { ... }
Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
drivers/gpu/nova-core/bitfield.rs | 71 +++++++++++++++++-----------
drivers/gpu/nova-core/regs/macros.rs | 16 +++----
2 files changed, 52 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs
index ba6b7caa05d9..687ef234be75 100644
--- a/drivers/gpu/nova-core/bitfield.rs
+++ b/drivers/gpu/nova-core/bitfield.rs
@@ -51,7 +51,7 @@
//! }
//!
//! bitfield! {
-//! struct ControlReg {
+//! struct ControlReg: u32 {
//! 3:0 mode as u8 ?=> Mode;
//! 7 state as bool => State;
//! }
@@ -61,6 +61,8 @@
//! This generates a struct with:
//! - Field accessors: `mode()`, `state()`, etc.
//! - Field setters: `set_mode()`, `set_state()`, etc. (supports chaining with builder pattern).
+//! Note that the compiler will error out if the size of the setter's arg exceeds the
+//! struct's storage size.
//! - Debug and Default implementations
//!
//! The field setters can be used with the builder pattern, example:
@@ -77,21 +79,21 @@
//!
macro_rules! bitfield {
// Main entry point - defines the bitfield struct with fields
- (struct $name:ident $(, $comment:literal)? { $($fields:tt)* }) => {
- bitfield!(@core $name $(, $comment)? { $($fields)* });
+ (struct $name:ident : $storage:ty $(, $comment:literal)? { $($fields:tt)* }) => {
+ bitfield!(@core $name $storage $(, $comment)? { $($fields)* });
};
// All rules below are helpers.
// Defines the wrapper `$name` type, as well as its relevant implementations (`Debug`,
// `Default`, `BitOr`, and conversion to the value type) and field accessor methods.
- (@core $name:ident $(, $comment:literal)? { $($fields:tt)* }) => {
+ (@core $name:ident $storage:ty $(, $comment:literal)? { $($fields:tt)* }) => {
$(
#[doc=$comment]
)?
#[repr(transparent)]
#[derive(Clone, Copy)]
- pub(crate) struct $name(u32);
+ pub(crate) struct $name($storage);
impl ::core::ops::BitOr for $name {
type Output = Self;
@@ -101,20 +103,26 @@ fn bitor(self, rhs: Self) -> Self::Output {
}
}
- impl ::core::convert::From<$name> for u32 {
- fn from(val: $name) -> u32 {
+ impl ::core::convert::From<$name> for $storage {
+ fn from(val: $name) -> $storage {
val.0
}
}
- bitfield!(@fields_dispatcher $name { $($fields)* });
+ impl ::core::convert::From<$storage> for $name {
+ fn from(val: $storage) -> Self {
+ Self(val)
+ }
+ }
+
+ bitfield!(@fields_dispatcher $name $storage { $($fields)* });
};
// Captures the fields and passes them to all the implementers that require field information.
//
// Used to simplify the matching rules for implementers, so they don't need to match the entire
// complex fields rule even though they only make use of part of it.
- (@fields_dispatcher $name:ident {
+ (@fields_dispatcher $name:ident $storage:ty {
$($hi:tt:$lo:tt $field:ident as $type:tt
$(?=> $try_into_type:ty)?
$(=> $into_type:ty)?
@@ -123,7 +131,7 @@ fn from(val: $name) -> u32 {
)*
}
) => {
- bitfield!(@field_accessors $name {
+ bitfield!(@field_accessors $name $storage {
$(
$hi:$lo $field as $type
$(?=> $try_into_type)?
@@ -138,7 +146,7 @@ fn from(val: $name) -> u32 {
// Defines all the field getter/setter methods for `$name`.
(
- @field_accessors $name:ident {
+ @field_accessors $name:ident $storage:ty {
$($hi:tt:$lo:tt $field:ident as $type:tt
$(?=> $try_into_type:ty)?
$(=> $into_type:ty)?
@@ -154,7 +162,7 @@ fn from(val: $name) -> u32 {
#[allow(dead_code)]
impl $name {
$(
- bitfield!(@field_accessor $name $hi:$lo $field as $type
+ bitfield!(@field_accessor $name $storage, $hi:$lo $field as $type
$(?=> $try_into_type)?
$(=> $into_type)?
$(, $comment)?
@@ -188,11 +196,11 @@ impl $name {
// Catches fields defined as `bool` and convert them into a boolean value.
(
- @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as bool => $into_type:ty
+ @field_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool => $into_type:ty
$(, $comment:literal)?;
) => {
bitfield!(
- @leaf_accessor $name $hi:$lo $field
+ @leaf_accessor $name $storage, $hi:$lo $field
{ |f| <$into_type>::from(if f != 0 { true } else { false }) }
$into_type => $into_type $(, $comment)?;
);
@@ -200,17 +208,17 @@ impl $name {
// Shortcut for fields defined as `bool` without the `=>` syntax.
(
- @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as bool $(, $comment:literal)?;
+ @field_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool $(, $comment:literal)?;
) => {
- bitfield!(@field_accessor $name $hi:$lo $field as bool => bool $(, $comment)?;);
+ bitfield!(@field_accessor $name $storage, $hi:$lo $field as bool => bool $(, $comment)?;);
};
// Catches the `?=>` syntax for non-boolean fields.
(
- @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt ?=> $try_into_type:ty
+ @field_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt ?=> $try_into_type:ty
$(, $comment:literal)?;
) => {
- bitfield!(@leaf_accessor $name $hi:$lo $field
+ bitfield!(@leaf_accessor $name $storage, $hi:$lo $field
{ |f| <$try_into_type>::try_from(f as $type) } $try_into_type =>
::core::result::Result<
$try_into_type,
@@ -221,29 +229,38 @@ impl $name {
// Catches the `=>` syntax for non-boolean fields.
(
- @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt => $into_type:ty
+ @field_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt => $into_type:ty
$(, $comment:literal)?;
) => {
- bitfield!(@leaf_accessor $name $hi:$lo $field
+ bitfield!(@leaf_accessor $name $storage, $hi:$lo $field
{ |f| <$into_type>::from(f as $type) } $into_type => $into_type $(, $comment)?;);
};
// Shortcut for non-boolean fields defined without the `=>` or `?=>` syntax.
(
- @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt
+ @field_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt
$(, $comment:literal)?;
) => {
- bitfield!(@field_accessor $name $hi:$lo $field as $type => $type $(, $comment)?;);
+ bitfield!(@field_accessor $name $storage, $hi:$lo $field as $type => $type $(, $comment)?;);
};
// Generates the accessor methods for a single field.
(
- @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident
+ @leaf_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident
{ $process:expr } $to_type:ty => $res_type:ty $(, $comment:literal)?;
) => {
::kernel::macros::paste!(
const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
- const [<$field:upper _MASK>]: u32 = ((((1 << $hi) - 1) << 1) + 1) - ((1 << $lo) - 1);
+ const [<$field:upper _MASK>]: $storage = {
+ // Generate mask for shifting
+ match ::core::mem::size_of::<$storage>() {
+ 1 => ::kernel::bits::genmask_u8($lo..=$hi) as $storage,
+ 2 => ::kernel::bits::genmask_u16($lo..=$hi) as $storage,
+ 4 => ::kernel::bits::genmask_u32($lo..=$hi) as $storage,
+ 8 => ::kernel::bits::genmask_u64($lo..=$hi) as $storage,
+ _ => ::kernel::build_error!("Unsupported storage type size")
+ }
+ };
const [<$field:upper _SHIFT>]: u32 = Self::[<$field:upper _MASK>].trailing_zeros();
);
@@ -254,7 +271,7 @@ impl $name {
#[inline(always)]
pub(crate) fn $field(self) -> $res_type {
::kernel::macros::paste!(
- const MASK: u32 = $name::[<$field:upper _MASK>];
+ const MASK: $storage = $name::[<$field:upper _MASK>];
const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
);
let field = ((self.0 & MASK) >> SHIFT);
@@ -269,9 +286,9 @@ pub(crate) fn $field(self) -> $res_type {
)?
#[inline(always)]
pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
- const MASK: u32 = $name::[<$field:upper _MASK>];
+ const MASK: $storage = $name::[<$field:upper _MASK>];
const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
- let value = (u32::from(value) << SHIFT) & MASK;
+ let value = (<$storage>::from(value) << SHIFT) & MASK;
self.0 = (self.0 & !MASK) | value;
self
diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
index 945d15a2c529..d34c7f37fb93 100644
--- a/drivers/gpu/nova-core/regs/macros.rs
+++ b/drivers/gpu/nova-core/regs/macros.rs
@@ -276,25 +276,25 @@ pub(crate) trait RegisterBase<T> {
macro_rules! register {
// Creates a register at a fixed offset of the MMIO space.
($name:ident @ $offset:literal $(, $comment:literal)? { $($fields:tt)* } ) => {
- bitfield!(struct $name $(, $comment)? { $($fields)* } );
+ bitfield!(struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_fixed $name @ $offset);
};
// Creates an alias register of fixed offset register `alias` with its own fields.
($name:ident => $alias:ident $(, $comment:literal)? { $($fields:tt)* } ) => {
- bitfield!(struct $name $(, $comment)? { $($fields)* } );
+ bitfield!(struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_fixed $name @ $alias::OFFSET);
};
// Creates a register at a relative offset from a base address provider.
($name:ident @ $base:ty [ $offset:literal ] $(, $comment:literal)? { $($fields:tt)* } ) => {
- bitfield!(struct $name $(, $comment)? { $($fields)* } );
+ bitfield!(struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_relative $name @ $base [ $offset ]);
};
// Creates an alias register of relative offset register `alias` with its own fields.
($name:ident => $base:ty [ $alias:ident ] $(, $comment:literal)? { $($fields:tt)* }) => {
- bitfield!(struct $name $(, $comment)? { $($fields)* } );
+ bitfield!(struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_relative $name @ $base [ $alias::OFFSET ]);
};
@@ -305,7 +305,7 @@ macro_rules! register {
}
) => {
static_assert!(::core::mem::size_of::<u32>() <= $stride);
- bitfield!(struct $name $(, $comment)? { $($fields)* } );
+ bitfield!(struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_array $name @ $offset [ $size ; $stride ]);
};
@@ -326,7 +326,7 @@ macro_rules! register {
$(, $comment:literal)? { $($fields:tt)* }
) => {
static_assert!(::core::mem::size_of::<u32>() <= $stride);
- bitfield!(struct $name $(, $comment)? { $($fields)* } );
+ bitfield!(struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_relative_array $name @ $base [ $offset [ $size ; $stride ] ]);
};
@@ -348,7 +348,7 @@ macro_rules! register {
}
) => {
static_assert!($idx < $alias::SIZE);
- bitfield!(struct $name $(, $comment)? { $($fields)* } );
+ bitfield!(struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_relative $name @ $base [ $alias::OFFSET + $idx * $alias::STRIDE ] );
};
@@ -357,7 +357,7 @@ macro_rules! register {
// to avoid it being interpreted in place of the relative register array alias rule.
($name:ident => $alias:ident [ $idx:expr ] $(, $comment:literal)? { $($fields:tt)* }) => {
static_assert!($idx < $alias::SIZE);
- bitfield!(struct $name $(, $comment)? { $($fields)* } );
+ bitfield!(struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_fixed $name @ $alias::OFFSET + $idx * $alias::STRIDE );
};
--
2.34.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v4 2/6] nova-core: bitfield: Add support for different storage widths
2025-09-20 18:22 ` [PATCH v4 2/6] nova-core: bitfield: Add support for different storage widths Joel Fernandes
@ 2025-09-29 6:22 ` Alexandre Courbot
2025-09-29 19:47 ` Joel Fernandes
0 siblings, 1 reply; 44+ messages in thread
From: Alexandre Courbot @ 2025-09-29 6:22 UTC (permalink / raw)
To: Joel Fernandes, linux-kernel, rust-for-linux, dri-devel, dakr,
acourbot
Cc: Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On Sun Sep 21, 2025 at 3:22 AM JST, Joel Fernandes wrote:
> Previously, bitfields were hardcoded to use u32 as the underlying
> storage type. Add support for different storage types (u8, u16, u32,
> u64) to the bitfield macro.
>
> New syntax is: struct Name: <type ex., u32> { ... }
>
> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> ---
> drivers/gpu/nova-core/bitfield.rs | 71 +++++++++++++++++-----------
> drivers/gpu/nova-core/regs/macros.rs | 16 +++----
> 2 files changed, 52 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs
> index ba6b7caa05d9..687ef234be75 100644
> --- a/drivers/gpu/nova-core/bitfield.rs
> +++ b/drivers/gpu/nova-core/bitfield.rs
> @@ -51,7 +51,7 @@
> //! }
> //!
> //! bitfield! {
> -//! struct ControlReg {
> +//! struct ControlReg: u32 {
Haven't we agreed in [1] to align the type definition syntax to that of
an actual Rust struct? E.g. `struct ControlReg(u32)`?
[1] https://lore.kernel.org/all/3814d6b7-7551-4e8c-b78a-4ac6236eb406@nvidia.com/
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v4 2/6] nova-core: bitfield: Add support for different storage widths
2025-09-29 6:22 ` Alexandre Courbot
@ 2025-09-29 19:47 ` Joel Fernandes
0 siblings, 0 replies; 44+ messages in thread
From: Joel Fernandes @ 2025-09-29 19:47 UTC (permalink / raw)
To: Alexandre Courbot, linux-kernel, rust-for-linux, dri-devel, dakr
Cc: Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On 9/29/2025 8:22 AM, Alexandre Courbot wrote:
> On Sun Sep 21, 2025 at 3:22 AM JST, Joel Fernandes wrote:
>> Previously, bitfields were hardcoded to use u32 as the underlying
>> storage type. Add support for different storage types (u8, u16, u32,
>> u64) to the bitfield macro.
>>
>> New syntax is: struct Name: <type ex., u32> { ... }
>>
>> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>> ---
>> drivers/gpu/nova-core/bitfield.rs | 71 +++++++++++++++++-----------
>> drivers/gpu/nova-core/regs/macros.rs | 16 +++----
>> 2 files changed, 52 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs
>> index ba6b7caa05d9..687ef234be75 100644
>> --- a/drivers/gpu/nova-core/bitfield.rs
>> +++ b/drivers/gpu/nova-core/bitfield.rs
>> @@ -51,7 +51,7 @@
>> //! }
>> //!
>> //! bitfield! {
>> -//! struct ControlReg {
>> +//! struct ControlReg: u32 {
>
> Haven't we agreed in [1] to align the type definition syntax to that of
> an actual Rust struct? E.g. `struct ControlReg(u32)`?
>
> [1] https://lore.kernel.org/all/3814d6b7-7551-4e8c-b78a-4ac6236eb406@nvidia.com/
Sorry this slipped (conference travel and all :P). I will make the change this
time. Btw v5 also has other changes in addition to this, your review is much
appreciated :)
- Joel
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v4 3/6] nova-core: bitfield: Add support for custom visiblity
2025-09-20 18:22 [PATCH v4 0/6] Introduce bitfield and move register macro to rust/kernel/ Joel Fernandes
2025-09-20 18:22 ` [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro Joel Fernandes
2025-09-20 18:22 ` [PATCH v4 2/6] nova-core: bitfield: Add support for different storage widths Joel Fernandes
@ 2025-09-20 18:22 ` Joel Fernandes
2025-09-29 6:28 ` Alexandre Courbot
2025-09-20 18:22 ` [PATCH v4 4/6] rust: Move register and bitfield macros out of Nova Joel Fernandes
` (2 subsequent siblings)
5 siblings, 1 reply; 44+ messages in thread
From: Joel Fernandes @ 2025-09-20 18:22 UTC (permalink / raw)
To: linux-kernel, rust-for-linux, dri-devel, dakr, acourbot
Cc: Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Joel Fernandes,
Timur Tabi, joel, Elle Rhumsaa, Yury Norov, Daniel Almeida,
nouveau
Add support for custom visiblity to allow for users to control visibility
of the structure and helpers.
Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
drivers/gpu/nova-core/bitfield.rs | 55 ++++++++++++++++------------
drivers/gpu/nova-core/regs/macros.rs | 16 ++++----
2 files changed, 40 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs
index 687ef234be75..9c022fc2bd6a 100644
--- a/drivers/gpu/nova-core/bitfield.rs
+++ b/drivers/gpu/nova-core/bitfield.rs
@@ -51,7 +51,7 @@
//! }
//!
//! bitfield! {
-//! struct ControlReg: u32 {
+//! pub struct ControlReg: u32 {
//! 3:0 mode as u8 ?=> Mode;
//! 7 state as bool => State;
//! }
@@ -65,6 +65,9 @@
//! struct's storage size.
//! - Debug and Default implementations
//!
+//! Note: Field accessors and setters inherit the same visibility as the struct itself.
+//! In the example above, both `mode()` and `set_mode()` methods will be `pub`.
+//!
//! The field setters can be used with the builder pattern, example:
//! ControlReg::default().set_mode(mode).set_state(state);
//!
@@ -79,21 +82,21 @@
//!
macro_rules! bitfield {
// Main entry point - defines the bitfield struct with fields
- (struct $name:ident : $storage:ty $(, $comment:literal)? { $($fields:tt)* }) => {
- bitfield!(@core $name $storage $(, $comment)? { $($fields)* });
+ ($vis:vis struct $name:ident : $storage:ty $(, $comment:literal)? { $($fields:tt)* }) => {
+ bitfield!(@core $vis $name $storage $(, $comment)? { $($fields)* });
};
// All rules below are helpers.
// Defines the wrapper `$name` type, as well as its relevant implementations (`Debug`,
// `Default`, `BitOr`, and conversion to the value type) and field accessor methods.
- (@core $name:ident $storage:ty $(, $comment:literal)? { $($fields:tt)* }) => {
+ (@core $vis:vis $name:ident $storage:ty $(, $comment:literal)? { $($fields:tt)* }) => {
$(
#[doc=$comment]
)?
#[repr(transparent)]
#[derive(Clone, Copy)]
- pub(crate) struct $name($storage);
+ $vis struct $name($storage);
impl ::core::ops::BitOr for $name {
type Output = Self;
@@ -115,14 +118,14 @@ fn from(val: $storage) -> Self {
}
}
- bitfield!(@fields_dispatcher $name $storage { $($fields)* });
+ bitfield!(@fields_dispatcher $vis $name $storage { $($fields)* });
};
// Captures the fields and passes them to all the implementers that require field information.
//
// Used to simplify the matching rules for implementers, so they don't need to match the entire
// complex fields rule even though they only make use of part of it.
- (@fields_dispatcher $name:ident $storage:ty {
+ (@fields_dispatcher $vis:vis $name:ident $storage:ty {
$($hi:tt:$lo:tt $field:ident as $type:tt
$(?=> $try_into_type:ty)?
$(=> $into_type:ty)?
@@ -131,7 +134,7 @@ fn from(val: $storage) -> Self {
)*
}
) => {
- bitfield!(@field_accessors $name $storage {
+ bitfield!(@field_accessors $vis $name $storage {
$(
$hi:$lo $field as $type
$(?=> $try_into_type)?
@@ -146,7 +149,7 @@ fn from(val: $storage) -> Self {
// Defines all the field getter/setter methods for `$name`.
(
- @field_accessors $name:ident $storage:ty {
+ @field_accessors $vis:vis $name:ident $storage:ty {
$($hi:tt:$lo:tt $field:ident as $type:tt
$(?=> $try_into_type:ty)?
$(=> $into_type:ty)?
@@ -161,8 +164,14 @@ fn from(val: $storage) -> Self {
#[allow(dead_code)]
impl $name {
+ /// Returns the raw underlying value
+ #[inline(always)]
+ $vis fn raw(&self) -> $storage {
+ self.0
+ }
+
$(
- bitfield!(@field_accessor $name $storage, $hi:$lo $field as $type
+ bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type
$(?=> $try_into_type)?
$(=> $into_type)?
$(, $comment)?
@@ -196,11 +205,11 @@ impl $name {
// Catches fields defined as `bool` and convert them into a boolean value.
(
- @field_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool => $into_type:ty
+ @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool => $into_type:ty
$(, $comment:literal)?;
) => {
bitfield!(
- @leaf_accessor $name $storage, $hi:$lo $field
+ @leaf_accessor $vis $name $storage, $hi:$lo $field
{ |f| <$into_type>::from(if f != 0 { true } else { false }) }
$into_type => $into_type $(, $comment)?;
);
@@ -208,17 +217,17 @@ impl $name {
// Shortcut for fields defined as `bool` without the `=>` syntax.
(
- @field_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool $(, $comment:literal)?;
+ @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool $(, $comment:literal)?;
) => {
- bitfield!(@field_accessor $name $storage, $hi:$lo $field as bool => bool $(, $comment)?;);
+ bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as bool => bool $(, $comment)?;);
};
// Catches the `?=>` syntax for non-boolean fields.
(
- @field_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt ?=> $try_into_type:ty
+ @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt ?=> $try_into_type:ty
$(, $comment:literal)?;
) => {
- bitfield!(@leaf_accessor $name $storage, $hi:$lo $field
+ bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
{ |f| <$try_into_type>::try_from(f as $type) } $try_into_type =>
::core::result::Result<
$try_into_type,
@@ -229,24 +238,24 @@ impl $name {
// Catches the `=>` syntax for non-boolean fields.
(
- @field_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt => $into_type:ty
+ @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt => $into_type:ty
$(, $comment:literal)?;
) => {
- bitfield!(@leaf_accessor $name $storage, $hi:$lo $field
+ bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
{ |f| <$into_type>::from(f as $type) } $into_type => $into_type $(, $comment)?;);
};
// Shortcut for non-boolean fields defined without the `=>` or `?=>` syntax.
(
- @field_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt
+ @field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt
$(, $comment:literal)?;
) => {
- bitfield!(@field_accessor $name $storage, $hi:$lo $field as $type => $type $(, $comment)?;);
+ bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type => $type $(, $comment)?;);
};
// Generates the accessor methods for a single field.
(
- @leaf_accessor $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident
+ @leaf_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident
{ $process:expr } $to_type:ty => $res_type:ty $(, $comment:literal)?;
) => {
::kernel::macros::paste!(
@@ -269,7 +278,7 @@ impl $name {
#[doc=$comment]
)?
#[inline(always)]
- pub(crate) fn $field(self) -> $res_type {
+ $vis fn $field(self) -> $res_type {
::kernel::macros::paste!(
const MASK: $storage = $name::[<$field:upper _MASK>];
const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
@@ -285,7 +294,7 @@ pub(crate) fn $field(self) -> $res_type {
#[doc=$comment]
)?
#[inline(always)]
- pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
+ $vis fn [<set_ $field>](mut self, value: $to_type) -> Self {
const MASK: $storage = $name::[<$field:upper _MASK>];
const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
let value = (<$storage>::from(value) << SHIFT) & MASK;
diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
index d34c7f37fb93..6a4f3271beb3 100644
--- a/drivers/gpu/nova-core/regs/macros.rs
+++ b/drivers/gpu/nova-core/regs/macros.rs
@@ -276,25 +276,25 @@ pub(crate) trait RegisterBase<T> {
macro_rules! register {
// Creates a register at a fixed offset of the MMIO space.
($name:ident @ $offset:literal $(, $comment:literal)? { $($fields:tt)* } ) => {
- bitfield!(struct $name: u32 $(, $comment)? { $($fields)* } );
+ bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_fixed $name @ $offset);
};
// Creates an alias register of fixed offset register `alias` with its own fields.
($name:ident => $alias:ident $(, $comment:literal)? { $($fields:tt)* } ) => {
- bitfield!(struct $name: u32 $(, $comment)? { $($fields)* } );
+ bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_fixed $name @ $alias::OFFSET);
};
// Creates a register at a relative offset from a base address provider.
($name:ident @ $base:ty [ $offset:literal ] $(, $comment:literal)? { $($fields:tt)* } ) => {
- bitfield!(struct $name: u32 $(, $comment)? { $($fields)* } );
+ bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_relative $name @ $base [ $offset ]);
};
// Creates an alias register of relative offset register `alias` with its own fields.
($name:ident => $base:ty [ $alias:ident ] $(, $comment:literal)? { $($fields:tt)* }) => {
- bitfield!(struct $name: u32 $(, $comment)? { $($fields)* } );
+ bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_relative $name @ $base [ $alias::OFFSET ]);
};
@@ -305,7 +305,7 @@ macro_rules! register {
}
) => {
static_assert!(::core::mem::size_of::<u32>() <= $stride);
- bitfield!(struct $name: u32 $(, $comment)? { $($fields)* } );
+ bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_array $name @ $offset [ $size ; $stride ]);
};
@@ -326,7 +326,7 @@ macro_rules! register {
$(, $comment:literal)? { $($fields:tt)* }
) => {
static_assert!(::core::mem::size_of::<u32>() <= $stride);
- bitfield!(struct $name: u32 $(, $comment)? { $($fields)* } );
+ bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_relative_array $name @ $base [ $offset [ $size ; $stride ] ]);
};
@@ -348,7 +348,7 @@ macro_rules! register {
}
) => {
static_assert!($idx < $alias::SIZE);
- bitfield!(struct $name: u32 $(, $comment)? { $($fields)* } );
+ bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_relative $name @ $base [ $alias::OFFSET + $idx * $alias::STRIDE ] );
};
@@ -357,7 +357,7 @@ macro_rules! register {
// to avoid it being interpreted in place of the relative register array alias rule.
($name:ident => $alias:ident [ $idx:expr ] $(, $comment:literal)? { $($fields:tt)* }) => {
static_assert!($idx < $alias::SIZE);
- bitfield!(struct $name: u32 $(, $comment)? { $($fields)* } );
+ bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_fixed $name @ $alias::OFFSET + $idx * $alias::STRIDE );
};
--
2.34.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v4 3/6] nova-core: bitfield: Add support for custom visiblity
2025-09-20 18:22 ` [PATCH v4 3/6] nova-core: bitfield: Add support for custom visiblity Joel Fernandes
@ 2025-09-29 6:28 ` Alexandre Courbot
2025-09-29 20:20 ` Joel Fernandes
0 siblings, 1 reply; 44+ messages in thread
From: Alexandre Courbot @ 2025-09-29 6:28 UTC (permalink / raw)
To: Joel Fernandes, linux-kernel, rust-for-linux, dri-devel, dakr,
acourbot
Cc: Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On Sun Sep 21, 2025 at 3:22 AM JST, Joel Fernandes wrote:
> Add support for custom visiblity to allow for users to control visibility
> of the structure and helpers.
>
> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
Just one comment below; otherwise
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
<snip>
> @@ -161,8 +164,14 @@ fn from(val: $storage) -> Self {
>
> #[allow(dead_code)]
> impl $name {
> + /// Returns the raw underlying value
> + #[inline(always)]
> + $vis fn raw(&self) -> $storage {
> + self.0
> + }
> +
Why does this new method suddenly appears in this patch?
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v4 3/6] nova-core: bitfield: Add support for custom visiblity
2025-09-29 6:28 ` Alexandre Courbot
@ 2025-09-29 20:20 ` Joel Fernandes
0 siblings, 0 replies; 44+ messages in thread
From: Joel Fernandes @ 2025-09-29 20:20 UTC (permalink / raw)
To: Alexandre Courbot, linux-kernel, rust-for-linux, dri-devel, dakr
Cc: Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On 9/29/2025 8:28 AM, Alexandre Courbot wrote:
> On Sun Sep 21, 2025 at 3:22 AM JST, Joel Fernandes wrote:
>> Add support for custom visiblity to allow for users to control visibility
>> of the structure and helpers.
>>
>> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>
> Just one comment below; otherwise
>
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
Thanks.
>> @@ -161,8 +164,14 @@ fn from(val: $storage) -> Self {
>>
>> #[allow(dead_code)]
>> impl $name {
>> + /// Returns the raw underlying value
>> + #[inline(always)]
>> + $vis fn raw(&self) -> $storage {
>> + self.0
>> + }
>> +
>
> Why does this new method suddenly appears in this patch?
Actually I've removed this already in v5, sorry about that.
- Joel
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v4 4/6] rust: Move register and bitfield macros out of Nova
2025-09-20 18:22 [PATCH v4 0/6] Introduce bitfield and move register macro to rust/kernel/ Joel Fernandes
` (2 preceding siblings ...)
2025-09-20 18:22 ` [PATCH v4 3/6] nova-core: bitfield: Add support for custom visiblity Joel Fernandes
@ 2025-09-20 18:22 ` Joel Fernandes
2025-09-29 6:30 ` Alexandre Courbot
2025-09-20 18:22 ` [PATCH v4 5/6] rust: Add KUNIT tests for bitfield Joel Fernandes
2025-09-20 18:22 ` [PATCH v4 6/6] rust: bitfield: Use 'as' operator for setter type conversion Joel Fernandes
5 siblings, 1 reply; 44+ messages in thread
From: Joel Fernandes @ 2025-09-20 18:22 UTC (permalink / raw)
To: linux-kernel, rust-for-linux, dri-devel, dakr, acourbot
Cc: Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Joel Fernandes,
Timur Tabi, joel, Elle Rhumsaa, Yury Norov, Daniel Almeida,
nouveau
Out of broad need for these macros in Rust, move them out. Several folks
have shown interest (Nova, Tyr GPU drivers).
bitfield moved into bits modules - defines bitfields in Rust structs similar to C.
register moved into io module - defines hardware registers and accessors.
Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
MAINTAINERS | 1 +
drivers/gpu/nova-core/falcon.rs | 2 +-
drivers/gpu/nova-core/falcon/gsp.rs | 4 +-
drivers/gpu/nova-core/falcon/sec2.rs | 2 +-
drivers/gpu/nova-core/nova_core.rs | 3 --
drivers/gpu/nova-core/regs.rs | 6 +--
rust/kernel/bits.rs | 2 +
.../kernel/bits}/bitfield.rs | 27 ++++++-----
rust/kernel/io.rs | 1 +
.../macros.rs => rust/kernel/io/register.rs | 46 ++++++++++---------
10 files changed, 51 insertions(+), 43 deletions(-)
rename {drivers/gpu/nova-core => rust/kernel/bits}/bitfield.rs (91%)
rename drivers/gpu/nova-core/regs/macros.rs => rust/kernel/io/register.rs (93%)
diff --git a/MAINTAINERS b/MAINTAINERS
index 841b76234045..a94af8607b6b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4379,6 +4379,7 @@ F: include/asm-generic/bitops.h
F: include/linux/bitops.h
F: lib/test_bitops.c
F: tools/*/bitops*
+F: rust/kernel/bits*
BITOPS API BINDINGS [RUST]
M: Yury Norov <yury.norov@gmail.com>
diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
index 37e6298195e4..a15fa98c8614 100644
--- a/drivers/gpu/nova-core/falcon.rs
+++ b/drivers/gpu/nova-core/falcon.rs
@@ -6,6 +6,7 @@
use hal::FalconHal;
use kernel::device;
use kernel::dma::DmaAddress;
+use kernel::io::register::RegisterBase;
use kernel::prelude::*;
use kernel::sync::aref::ARef;
use kernel::time::Delta;
@@ -14,7 +15,6 @@
use crate::driver::Bar0;
use crate::gpu::Chipset;
use crate::regs;
-use crate::regs::macros::RegisterBase;
use crate::util;
pub(crate) mod gsp;
diff --git a/drivers/gpu/nova-core/falcon/gsp.rs b/drivers/gpu/nova-core/falcon/gsp.rs
index f17599cb49fa..cd4960e997c8 100644
--- a/drivers/gpu/nova-core/falcon/gsp.rs
+++ b/drivers/gpu/nova-core/falcon/gsp.rs
@@ -1,9 +1,11 @@
// SPDX-License-Identifier: GPL-2.0
+use kernel::io::register::RegisterBase;
+
use crate::{
driver::Bar0,
falcon::{Falcon, FalconEngine, PFalcon2Base, PFalconBase},
- regs::{self, macros::RegisterBase},
+ regs::self,
};
/// Type specifying the `Gsp` falcon engine. Cannot be instantiated.
diff --git a/drivers/gpu/nova-core/falcon/sec2.rs b/drivers/gpu/nova-core/falcon/sec2.rs
index 815786c8480d..81717868a8a8 100644
--- a/drivers/gpu/nova-core/falcon/sec2.rs
+++ b/drivers/gpu/nova-core/falcon/sec2.rs
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
use crate::falcon::{FalconEngine, PFalcon2Base, PFalconBase};
-use crate::regs::macros::RegisterBase;
+use kernel::io::register::RegisterBase;
/// Type specifying the `Sec2` falcon engine. Cannot be instantiated.
pub(crate) struct Sec2(());
diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
index 112277c7921e..fffcaee2249f 100644
--- a/drivers/gpu/nova-core/nova_core.rs
+++ b/drivers/gpu/nova-core/nova_core.rs
@@ -2,9 +2,6 @@
//! Nova Core GPU Driver
-#[macro_use]
-mod bitfield;
-
mod dma;
mod driver;
mod falcon;
diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
index 206dab2e1335..1f08e6d4045a 100644
--- a/drivers/gpu/nova-core/regs.rs
+++ b/drivers/gpu/nova-core/regs.rs
@@ -4,15 +4,13 @@
// but are mapped to types.
#![allow(non_camel_case_types)]
-#[macro_use]
-pub(crate) mod macros;
-
use crate::falcon::{
DmaTrfCmdSize, FalconCoreRev, FalconCoreRevSubversion, FalconFbifMemType, FalconFbifTarget,
FalconModSelAlgo, FalconSecurityModel, PFalcon2Base, PFalconBase, PeregrineCoreSelect,
};
use crate::gpu::{Architecture, Chipset};
use kernel::prelude::*;
+use kernel::register;
// PMC
@@ -331,6 +329,7 @@ pub(crate) fn mem_scrubbing_done(self) -> bool {
pub(crate) mod gm107 {
// FUSE
+ use kernel::register;
register!(NV_FUSE_STATUS_OPT_DISPLAY @ 0x00021c04 {
0:0 display_disabled as bool;
@@ -339,6 +338,7 @@ pub(crate) mod gm107 {
pub(crate) mod ga100 {
// FUSE
+ use kernel::register;
register!(NV_FUSE_STATUS_OPT_DISPLAY @ 0x00820c04 {
0:0 display_disabled as bool;
diff --git a/rust/kernel/bits.rs b/rust/kernel/bits.rs
index 553d50265883..590a77d99ad7 100644
--- a/rust/kernel/bits.rs
+++ b/rust/kernel/bits.rs
@@ -201,3 +201,5 @@ pub const fn [<genmask_ $ty>](range: RangeInclusive<u32>) -> $ty {
/// assert_eq!(genmask_u8(0..=7), u8::MAX);
/// ```
);
+
+pub mod bitfield;
diff --git a/drivers/gpu/nova-core/bitfield.rs b/rust/kernel/bits/bitfield.rs
similarity index 91%
rename from drivers/gpu/nova-core/bitfield.rs
rename to rust/kernel/bits/bitfield.rs
index 9c022fc2bd6a..6089f3cdbd1b 100644
--- a/drivers/gpu/nova-core/bitfield.rs
+++ b/rust/kernel/bits/bitfield.rs
@@ -80,10 +80,13 @@
//! - `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.
//!
+
+/// bitfield macro definition
+#[macro_export]
macro_rules! bitfield {
// Main entry point - defines the bitfield struct with fields
($vis:vis struct $name:ident : $storage:ty $(, $comment:literal)? { $($fields:tt)* }) => {
- bitfield!(@core $vis $name $storage $(, $comment)? { $($fields)* });
+ ::kernel::bitfield!(@core $vis $name $storage $(, $comment)? { $($fields)* });
};
// All rules below are helpers.
@@ -118,7 +121,7 @@ fn from(val: $storage) -> Self {
}
}
- bitfield!(@fields_dispatcher $vis $name $storage { $($fields)* });
+ ::kernel::bitfield!(@fields_dispatcher $vis $name $storage { $($fields)* });
};
// Captures the fields and passes them to all the implementers that require field information.
@@ -134,7 +137,7 @@ fn from(val: $storage) -> Self {
)*
}
) => {
- bitfield!(@field_accessors $vis $name $storage {
+ ::kernel::bitfield!(@field_accessors $vis $name $storage {
$(
$hi:$lo $field as $type
$(?=> $try_into_type)?
@@ -143,8 +146,8 @@ fn from(val: $storage) -> Self {
;
)*
});
- bitfield!(@debug $name { $($field;)* });
- bitfield!(@default $name { $($field;)* });
+ ::kernel::bitfield!(@debug $name { $($field;)* });
+ ::kernel::bitfield!(@default $name { $($field;)* });
};
// Defines all the field getter/setter methods for `$name`.
@@ -159,7 +162,7 @@ fn from(val: $storage) -> Self {
}
) => {
$(
- bitfield!(@check_field_bounds $hi:$lo $field as $type);
+ ::kernel::bitfield!(@check_field_bounds $hi:$lo $field as $type);
)*
#[allow(dead_code)]
@@ -171,7 +174,7 @@ impl $name {
}
$(
- bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type
+ ::kernel::bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type
$(?=> $try_into_type)?
$(=> $into_type)?
$(, $comment)?
@@ -208,7 +211,7 @@ impl $name {
@field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool => $into_type:ty
$(, $comment:literal)?;
) => {
- bitfield!(
+ ::kernel::bitfield!(
@leaf_accessor $vis $name $storage, $hi:$lo $field
{ |f| <$into_type>::from(if f != 0 { true } else { false }) }
$into_type => $into_type $(, $comment)?;
@@ -219,7 +222,7 @@ impl $name {
(
@field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as bool $(, $comment:literal)?;
) => {
- bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as bool => bool $(, $comment)?;);
+ ::kernel::bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as bool => bool $(, $comment)?;);
};
// Catches the `?=>` syntax for non-boolean fields.
@@ -227,7 +230,7 @@ impl $name {
@field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt ?=> $try_into_type:ty
$(, $comment:literal)?;
) => {
- bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
+ ::kernel::bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
{ |f| <$try_into_type>::try_from(f as $type) } $try_into_type =>
::core::result::Result<
$try_into_type,
@@ -241,7 +244,7 @@ impl $name {
@field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt => $into_type:ty
$(, $comment:literal)?;
) => {
- bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
+ ::kernel::bitfield!(@leaf_accessor $vis $name $storage, $hi:$lo $field
{ |f| <$into_type>::from(f as $type) } $into_type => $into_type $(, $comment)?;);
};
@@ -250,7 +253,7 @@ impl $name {
@field_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident as $type:tt
$(, $comment:literal)?;
) => {
- bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type => $type $(, $comment)?;);
+ ::kernel::bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type => $type $(, $comment)?;);
};
// Generates the accessor methods for a single field.
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index ee182b0b5452..da1384fd9ab6 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -9,6 +9,7 @@
pub mod mem;
pub mod poll;
+pub mod register;
pub mod resource;
pub use resource::Resource;
diff --git a/drivers/gpu/nova-core/regs/macros.rs b/rust/kernel/io/register.rs
similarity index 93%
rename from drivers/gpu/nova-core/regs/macros.rs
rename to rust/kernel/io/register.rs
index 6a4f3271beb3..088a8590db92 100644
--- a/drivers/gpu/nova-core/regs/macros.rs
+++ b/rust/kernel/io/register.rs
@@ -17,7 +17,8 @@
/// The `T` generic argument is used to distinguish which base to use, in case a type provides
/// several bases. It is given to the `register!` macro to restrict the use of the register to
/// implementors of this particular variant.
-pub(crate) trait RegisterBase<T> {
+pub trait RegisterBase<T> {
+ /// The base address for the register.
const BASE: usize;
}
@@ -273,28 +274,29 @@ pub(crate) trait RegisterBase<T> {
/// # Ok(())
/// # }
/// ```
+#[macro_export]
macro_rules! register {
// Creates a register at a fixed offset of the MMIO space.
($name:ident @ $offset:literal $(, $comment:literal)? { $($fields:tt)* } ) => {
- bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
+ ::kernel::bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_fixed $name @ $offset);
};
// Creates an alias register of fixed offset register `alias` with its own fields.
($name:ident => $alias:ident $(, $comment:literal)? { $($fields:tt)* } ) => {
- bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
+ ::kernel::bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_fixed $name @ $alias::OFFSET);
};
// Creates a register at a relative offset from a base address provider.
($name:ident @ $base:ty [ $offset:literal ] $(, $comment:literal)? { $($fields:tt)* } ) => {
- bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
+ ::kernel::bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_relative $name @ $base [ $offset ]);
};
// Creates an alias register of relative offset register `alias` with its own fields.
($name:ident => $base:ty [ $alias:ident ] $(, $comment:literal)? { $($fields:tt)* }) => {
- bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
+ ::kernel::bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_relative $name @ $base [ $alias::OFFSET ]);
};
@@ -305,7 +307,7 @@ macro_rules! register {
}
) => {
static_assert!(::core::mem::size_of::<u32>() <= $stride);
- bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
+ ::kernel::bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_array $name @ $offset [ $size ; $stride ]);
};
@@ -326,7 +328,7 @@ macro_rules! register {
$(, $comment:literal)? { $($fields:tt)* }
) => {
static_assert!(::core::mem::size_of::<u32>() <= $stride);
- bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
+ ::kernel::bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_relative_array $name @ $base [ $offset [ $size ; $stride ] ]);
};
@@ -348,7 +350,7 @@ macro_rules! register {
}
) => {
static_assert!($idx < $alias::SIZE);
- bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
+ ::kernel::bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_relative $name @ $base [ $alias::OFFSET + $idx * $alias::STRIDE ] );
};
@@ -357,7 +359,7 @@ macro_rules! register {
// to avoid it being interpreted in place of the relative register array alias rule.
($name:ident => $alias:ident [ $idx:expr ] $(, $comment:literal)? { $($fields:tt)* }) => {
static_assert!($idx < $alias::SIZE);
- bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
+ ::kernel::bitfield!(pub(crate) struct $name: u32 $(, $comment)? { $($fields)* } );
register!(@io_fixed $name @ $alias::OFFSET + $idx * $alias::STRIDE );
};
@@ -414,12 +416,12 @@ pub(crate) fn read<const SIZE: usize, T, B>(
base: &B,
) -> Self where
T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
- B: crate::regs::macros::RegisterBase<$base>,
+ B: ::kernel::io::register::RegisterBase<$base>,
{
const OFFSET: usize = $name::OFFSET;
let value = io.read32(
- <B as crate::regs::macros::RegisterBase<$base>>::BASE + OFFSET
+ <B as ::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET
);
Self(value)
@@ -435,13 +437,13 @@ pub(crate) fn write<const SIZE: usize, T, B>(
base: &B,
) where
T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
- B: crate::regs::macros::RegisterBase<$base>,
+ B: ::kernel::io::register::RegisterBase<$base>,
{
const OFFSET: usize = $name::OFFSET;
io.write32(
self.0,
- <B as crate::regs::macros::RegisterBase<$base>>::BASE + OFFSET
+ <B as ::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET
);
}
@@ -455,7 +457,7 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
f: F,
) where
T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
- B: crate::regs::macros::RegisterBase<$base>,
+ B: ::kernel::io::register::RegisterBase<$base>,
F: ::core::ops::FnOnce(Self) -> Self,
{
let reg = f(Self::read(io, base));
@@ -600,11 +602,11 @@ pub(crate) fn read<const SIZE: usize, T, B>(
idx: usize,
) -> Self where
T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
- B: crate::regs::macros::RegisterBase<$base>,
+ B: ::kernel::io::register::RegisterBase<$base>,
{
build_assert!(idx < Self::SIZE);
- let offset = <B as crate::regs::macros::RegisterBase<$base>>::BASE +
+ let offset = <B as ::kernel::io::register::RegisterBase<$base>>::BASE +
Self::OFFSET + (idx * Self::STRIDE);
let value = io.read32(offset);
@@ -622,11 +624,11 @@ pub(crate) fn write<const SIZE: usize, T, B>(
idx: usize
) where
T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
- B: crate::regs::macros::RegisterBase<$base>,
+ B: ::kernel::io::register::RegisterBase<$base>,
{
build_assert!(idx < Self::SIZE);
- let offset = <B as crate::regs::macros::RegisterBase<$base>>::BASE +
+ let offset = <B as ::kernel::io::register::RegisterBase<$base>>::BASE +
Self::OFFSET + (idx * Self::STRIDE);
io.write32(self.0, offset);
@@ -643,7 +645,7 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
f: F,
) where
T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
- B: crate::regs::macros::RegisterBase<$base>,
+ B: ::kernel::io::register::RegisterBase<$base>,
F: ::core::ops::FnOnce(Self) -> Self,
{
let reg = f(Self::read(io, base, idx));
@@ -662,7 +664,7 @@ pub(crate) fn try_read<const SIZE: usize, T, B>(
idx: usize,
) -> ::kernel::error::Result<Self> where
T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
- B: crate::regs::macros::RegisterBase<$base>,
+ B: ::kernel::io::register::RegisterBase<$base>,
{
if idx < Self::SIZE {
Ok(Self::read(io, base, idx))
@@ -684,7 +686,7 @@ pub(crate) fn try_write<const SIZE: usize, T, B>(
idx: usize,
) -> ::kernel::error::Result where
T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
- B: crate::regs::macros::RegisterBase<$base>,
+ B: ::kernel::io::register::RegisterBase<$base>,
{
if idx < Self::SIZE {
Ok(self.write(io, base, idx))
@@ -707,7 +709,7 @@ pub(crate) fn try_alter<const SIZE: usize, T, B, F>(
f: F,
) -> ::kernel::error::Result where
T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
- B: crate::regs::macros::RegisterBase<$base>,
+ B: ::kernel::io::register::RegisterBase<$base>,
F: ::core::ops::FnOnce(Self) -> Self,
{
if idx < Self::SIZE {
--
2.34.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v4 4/6] rust: Move register and bitfield macros out of Nova
2025-09-20 18:22 ` [PATCH v4 4/6] rust: Move register and bitfield macros out of Nova Joel Fernandes
@ 2025-09-29 6:30 ` Alexandre Courbot
0 siblings, 0 replies; 44+ messages in thread
From: Alexandre Courbot @ 2025-09-29 6:30 UTC (permalink / raw)
To: Joel Fernandes, linux-kernel, rust-for-linux, dri-devel, dakr,
acourbot
Cc: Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On Sun Sep 21, 2025 at 3:22 AM JST, Joel Fernandes wrote:
> Out of broad need for these macros in Rust, move them out. Several folks
> have shown interest (Nova, Tyr GPU drivers).
>
> bitfield moved into bits modules - defines bitfields in Rust structs similar to C.
> register moved into io module - defines hardware registers and accessors.
>
> Reviewed-by: Elle Rhumsaa <elle@weathered-steel.dev>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
Very clean move!
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v4 5/6] rust: Add KUNIT tests for bitfield
2025-09-20 18:22 [PATCH v4 0/6] Introduce bitfield and move register macro to rust/kernel/ Joel Fernandes
` (3 preceding siblings ...)
2025-09-20 18:22 ` [PATCH v4 4/6] rust: Move register and bitfield macros out of Nova Joel Fernandes
@ 2025-09-20 18:22 ` Joel Fernandes
2025-09-20 18:22 ` [PATCH v4 6/6] rust: bitfield: Use 'as' operator for setter type conversion Joel Fernandes
5 siblings, 0 replies; 44+ messages in thread
From: Joel Fernandes @ 2025-09-20 18:22 UTC (permalink / raw)
To: linux-kernel, rust-for-linux, dri-devel, dakr, acourbot
Cc: Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Joel Fernandes,
Timur Tabi, joel, Elle Rhumsaa, Yury Norov, Daniel Almeida,
nouveau
Add KUNIT tests to make sure the macro is working correctly.
Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
rust/kernel/bits/bitfield.rs | 349 +++++++++++++++++++++++++++++++++++
1 file changed, 349 insertions(+)
diff --git a/rust/kernel/bits/bitfield.rs b/rust/kernel/bits/bitfield.rs
index 6089f3cdbd1b..99e443580b36 100644
--- a/rust/kernel/bits/bitfield.rs
+++ b/rust/kernel/bits/bitfield.rs
@@ -341,3 +341,352 @@ fn default() -> Self {
}
};
}
+
+#[::kernel::macros::kunit_tests(kernel_bitfield)]
+mod tests {
+ use core::convert::TryFrom;
+
+ // Enum types for testing => and ?=> conversions
+ #[derive(Debug, Clone, Copy, PartialEq)]
+ enum MemoryType {
+ Unmapped = 0,
+ Normal = 1,
+ Device = 2,
+ Reserved = 3,
+ }
+
+ impl Default for MemoryType {
+ fn default() -> Self {
+ MemoryType::Unmapped
+ }
+ }
+
+ impl TryFrom<u8> for MemoryType {
+ type Error = u8;
+ fn try_from(value: u8) -> Result<Self, Self::Error> {
+ match value {
+ 0 => Ok(MemoryType::Unmapped),
+ 1 => Ok(MemoryType::Normal),
+ 2 => Ok(MemoryType::Device),
+ 3 => Ok(MemoryType::Reserved),
+ _ => Err(value),
+ }
+ }
+ }
+
+ impl From<MemoryType> for u64 {
+ fn from(mt: MemoryType) -> u64 {
+ mt as u64
+ }
+ }
+
+ #[derive(Debug, Clone, Copy, PartialEq)]
+ enum Priority {
+ Low = 0,
+ Medium = 1,
+ High = 2,
+ Critical = 3,
+ }
+
+ impl Default for Priority {
+ fn default() -> Self {
+ Priority::Low
+ }
+ }
+
+ impl From<u8> for Priority {
+ fn from(value: u8) -> Self {
+ match value & 0x3 {
+ 0 => Priority::Low,
+ 1 => Priority::Medium,
+ 2 => Priority::High,
+ _ => Priority::Critical,
+ }
+ }
+ }
+
+ impl From<Priority> for u16 {
+ fn from(p: Priority) -> u16 {
+ p as u16
+ }
+ }
+
+ bitfield! {
+ struct TestPageTableEntry: u64 {
+ 0:0 present as bool;
+ 1:1 writable as bool;
+ 11:9 available as u8;
+ 13:12 mem_type as u8 ?=> MemoryType;
+ 17:14 extended_type as u8 ?=> MemoryType; // For testing failures
+ 51:12 pfn as u64;
+ 51:12 pfn_overlap as u64;
+ 61:52 available2 as u16;
+ }
+ }
+
+ bitfield! {
+ struct TestControlRegister: u16 {
+ 0:0 enable as bool;
+ 3:1 mode as u8;
+ 5:4 priority as u8 => Priority;
+ 7:4 priority_nibble as u8;
+ 15:8 channel as u8;
+ }
+ }
+
+ bitfield! {
+ struct TestStatusRegister: u8 {
+ 0:0 ready as bool;
+ 1:1 error as bool;
+ 3:2 state as u8;
+ 7:4 reserved as u8;
+ 7:0 full_byte as u8; // For entire register
+ }
+ }
+
+ bitfield! {
+ struct TestPartialBits: u8 {
+ 0:0 ready as bool;
+ 1:1 error as bool;
+ 3:2 state as u8;
+ }
+ }
+
+ #[test]
+ fn test_single_bits() {
+ let mut pte = TestPageTableEntry::default();
+
+ assert!(!pte.present());
+ assert!(!pte.writable());
+
+ pte = pte.set_present(true);
+ assert!(pte.present());
+
+ pte = pte.set_writable(true);
+ assert!(pte.writable());
+
+ pte = pte.set_writable(false);
+ assert!(!pte.writable());
+
+ assert_eq!(pte.available(), 0);
+ pte = pte.set_available(0x5);
+ assert_eq!(pte.available(), 0x5);
+ }
+
+ #[test]
+ fn test_range_fields() {
+ let mut pte = TestPageTableEntry::default();
+
+ pte = pte.set_pfn(0x123456);
+ assert_eq!(pte.pfn(), 0x123456);
+ // Test overlapping field reads same value
+ assert_eq!(pte.pfn_overlap(), 0x123456);
+
+ pte = pte.set_available(0x7);
+ assert_eq!(pte.available(), 0x7);
+
+ pte = pte.set_available2(0x3FF);
+ assert_eq!(pte.available2(), 0x3FF);
+
+ // Test TryFrom with ?=> for MemoryType
+ pte = pte.set_mem_type(MemoryType::Device);
+ assert_eq!(pte.mem_type(), Ok(MemoryType::Device));
+
+ pte = pte.set_mem_type(MemoryType::Normal);
+ assert_eq!(pte.mem_type(), Ok(MemoryType::Normal));
+
+ // Test all valid values for mem_type
+ pte = pte.set_mem_type(MemoryType::Reserved); // Valid value: 3
+ assert_eq!(pte.mem_type(), Ok(MemoryType::Reserved));
+
+ // Test failure case using extended_type field which has 4 bits (0-15)
+ // MemoryType only handles 0-3, so values 4-15 should return Err
+ let mut raw = pte.raw();
+ // Set bits 17:14 to 7 (invalid for MemoryType)
+ raw = (raw & !::kernel::bits::genmask_u64(14..=17)) | (0x7 << 14);
+ let invalid_pte = TestPageTableEntry::from(raw);
+ // Should return Err with the invalid value
+ assert_eq!(invalid_pte.extended_type(), Err(0x7));
+
+ // Test a valid value after testing invalid to ensure both cases work
+ // Set bits 17:14 to 2 (valid: Device)
+ raw = (raw & !::kernel::bits::genmask_u64(14..=17)) | (0x2 << 14);
+ let valid_pte = TestPageTableEntry::from(raw);
+ assert_eq!(valid_pte.extended_type(), Ok(MemoryType::Device));
+
+ let max_pfn = ::kernel::bits::genmask_u64(0..=39);
+ pte = pte.set_pfn(max_pfn);
+ assert_eq!(pte.pfn(), max_pfn);
+ assert_eq!(pte.pfn_overlap(), max_pfn);
+ }
+
+ #[test]
+ fn test_builder_pattern() {
+ let pte = TestPageTableEntry::default()
+ .set_present(true)
+ .set_writable(true)
+ .set_available(0x7)
+ .set_pfn(0xABCDEF)
+ .set_mem_type(MemoryType::Reserved)
+ .set_available2(0x3FF);
+
+ assert!(pte.present());
+ assert!(pte.writable());
+ assert_eq!(pte.available(), 0x7);
+ assert_eq!(pte.pfn(), 0xABCDEF);
+ assert_eq!(pte.pfn_overlap(), 0xABCDEF);
+ assert_eq!(pte.mem_type(), Ok(MemoryType::Reserved));
+ assert_eq!(pte.available2(), 0x3FF);
+ }
+
+ #[test]
+ fn test_raw_operations() {
+ let raw_value = 0x3FF0000003123E03u64;
+
+ // Test using ::from() syntax
+ let pte = TestPageTableEntry::from(raw_value);
+ assert_eq!(pte.raw(), raw_value);
+
+ assert!(pte.present());
+ assert!(pte.writable());
+ assert_eq!(pte.available(), 0x7);
+ assert_eq!(pte.pfn(), 0x3123);
+ assert_eq!(pte.pfn_overlap(), 0x3123);
+ assert_eq!(pte.mem_type(), Ok(MemoryType::Reserved));
+ assert_eq!(pte.available2(), 0x3FF);
+
+ // Test using direct constructor syntax TestStruct(value)
+ let pte2 = TestPageTableEntry(raw_value);
+ assert_eq!(pte2.raw(), raw_value);
+ }
+
+ #[test]
+ fn test_u16_bitfield() {
+ let mut ctrl = TestControlRegister::default();
+
+ assert!(!ctrl.enable());
+ assert_eq!(ctrl.mode(), 0);
+ assert_eq!(ctrl.priority(), Priority::Low);
+ assert_eq!(ctrl.priority_nibble(), 0);
+ assert_eq!(ctrl.channel(), 0);
+
+ ctrl = ctrl.set_enable(true);
+ assert!(ctrl.enable());
+
+ ctrl = ctrl.set_mode(0x5);
+ assert_eq!(ctrl.mode(), 0x5);
+
+ // Test From conversion with =>
+ ctrl = ctrl.set_priority(Priority::High);
+ assert_eq!(ctrl.priority(), Priority::High);
+ assert_eq!(ctrl.priority_nibble(), 0x2); // High = 2 in bits 5:4
+
+ ctrl = ctrl.set_channel(0xAB);
+ assert_eq!(ctrl.channel(), 0xAB);
+
+ // Test overlapping fields
+ ctrl = ctrl.set_priority_nibble(0xF);
+ assert_eq!(ctrl.priority_nibble(), 0xF);
+ assert_eq!(ctrl.priority(), Priority::Critical); // bits 5:4 = 0x3
+
+ let ctrl2 = TestControlRegister::default()
+ .set_enable(true)
+ .set_mode(0x3)
+ .set_priority(Priority::Medium)
+ .set_channel(0x42);
+
+ assert!(ctrl2.enable());
+ assert_eq!(ctrl2.mode(), 0x3);
+ assert_eq!(ctrl2.priority(), Priority::Medium);
+ assert_eq!(ctrl2.channel(), 0x42);
+
+ let raw_value: u16 = 0x4217;
+ let ctrl3 = TestControlRegister::from(raw_value);
+ assert_eq!(ctrl3.raw(), raw_value);
+ assert!(ctrl3.enable());
+ assert_eq!(ctrl3.priority(), Priority::Medium);
+ assert_eq!(ctrl3.priority_nibble(), 0x1);
+ assert_eq!(ctrl3.channel(), 0x42);
+ }
+
+ #[test]
+ fn test_u8_bitfield() {
+ let mut status = TestStatusRegister::default();
+
+ assert!(!status.ready());
+ assert!(!status.error());
+ assert_eq!(status.state(), 0);
+ assert_eq!(status.reserved(), 0);
+ assert_eq!(status.full_byte(), 0);
+
+ status = status.set_ready(true);
+ assert!(status.ready());
+ assert_eq!(status.full_byte(), 0x01);
+
+ status = status.set_error(true);
+ assert!(status.error());
+ assert_eq!(status.full_byte(), 0x03);
+
+ status = status.set_state(0x3);
+ assert_eq!(status.state(), 0x3);
+ assert_eq!(status.full_byte(), 0x0F);
+
+ status = status.set_reserved(0xA);
+ assert_eq!(status.reserved(), 0xA);
+ assert_eq!(status.full_byte(), 0xAF);
+
+ // Test overlapping field
+ status = status.set_full_byte(0x55);
+ assert_eq!(status.full_byte(), 0x55);
+ assert!(status.ready());
+ assert!(!status.error());
+ assert_eq!(status.state(), 0x1);
+ assert_eq!(status.reserved(), 0x5);
+
+ let status2 = TestStatusRegister::default()
+ .set_ready(true)
+ .set_state(0x2)
+ .set_reserved(0x5);
+
+ assert!(status2.ready());
+ assert!(!status2.error());
+ assert_eq!(status2.state(), 0x2);
+ assert_eq!(status2.reserved(), 0x5);
+ assert_eq!(status2.full_byte(), 0x59);
+
+ let raw_value: u8 = 0x59;
+ let status3 = TestStatusRegister::from(raw_value);
+ assert_eq!(status3.raw(), raw_value);
+ assert!(status3.ready());
+ assert!(!status3.error());
+ assert_eq!(status3.state(), 0x2);
+ assert_eq!(status3.reserved(), 0x5);
+ assert_eq!(status3.full_byte(), 0x59);
+
+ let status4 = TestStatusRegister::from(0xFF);
+ assert!(status4.ready());
+ assert!(status4.error());
+ assert_eq!(status4.state(), 0x3);
+ assert_eq!(status4.reserved(), 0xF);
+ assert_eq!(status4.full_byte(), 0xFF);
+ }
+
+ #[test]
+ fn test_partial_bitfield() {
+ // Test creating a bitfield from a runtime value that has bits set
+ // beyond what the defined fields can handle.
+ let raw_value: u8 = 0xff; // All bits set
+ let bf = TestPartialBits::from(raw_value);
+ assert_eq!(bf.raw(), 0xff);
+
+ // Test individual field extraction from the runtime value
+ assert!(bf.ready());
+ assert!(bf.error());
+ assert_eq!(bf.state(), 3); // bits 3:2 are both set
+
+ // Test overflow of setters
+ let mut bf2 = TestPartialBits::default();
+ bf2 = bf2.set_state(0x55);
+ assert_eq!(bf2.state(), 0x1);
+ }
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v4 6/6] rust: bitfield: Use 'as' operator for setter type conversion
2025-09-20 18:22 [PATCH v4 0/6] Introduce bitfield and move register macro to rust/kernel/ Joel Fernandes
` (4 preceding siblings ...)
2025-09-20 18:22 ` [PATCH v4 5/6] rust: Add KUNIT tests for bitfield Joel Fernandes
@ 2025-09-20 18:22 ` Joel Fernandes
2025-09-29 6:47 ` Alexandre Courbot
2025-09-29 13:59 ` Miguel Ojeda
5 siblings, 2 replies; 44+ messages in thread
From: Joel Fernandes @ 2025-09-20 18:22 UTC (permalink / raw)
To: linux-kernel, rust-for-linux, dri-devel, dakr, acourbot
Cc: Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Joel Fernandes,
Timur Tabi, joel, Elle Rhumsaa, Yury Norov, Daniel Almeida,
nouveau
The bitfield macro's setter currently uses the From trait for type
conversion, which is overly restrictive and prevents use cases such as
narrowing conversions (e.g., u32 storage size to u8 field size) which
aren't supported by From.
Replace 'from' with 'as' in the setter implementation to support this.
Suggested-by: Yury Norov <yury.norov@gmail.com>
Signed-off-by: Joel Fernandes <joel@joelfernandes.org>
---
rust/kernel/bits/bitfield.rs | 46 +++++++++++++++++++++++++++++++++++-
1 file changed, 45 insertions(+), 1 deletion(-)
diff --git a/rust/kernel/bits/bitfield.rs b/rust/kernel/bits/bitfield.rs
index 99e443580b36..6342352891fa 100644
--- a/rust/kernel/bits/bitfield.rs
+++ b/rust/kernel/bits/bitfield.rs
@@ -300,7 +300,7 @@ impl $name {
$vis fn [<set_ $field>](mut self, value: $to_type) -> Self {
const MASK: $storage = $name::[<$field:upper _MASK>];
const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
- let value = (<$storage>::from(value) << SHIFT) & MASK;
+ let value = ((value as $storage) << SHIFT) & MASK;
self.0 = (self.0 & !MASK) | value;
self
@@ -452,6 +452,15 @@ struct TestPartialBits: u8 {
}
}
+ // For testing wide field types on narrow storage
+ bitfield! {
+ struct TestWideFields: u8 {
+ 3:0 nibble as u32;
+ 7:4 high_nibble as u32;
+ 7:0 full as u64;
+ }
+ }
+
#[test]
fn test_single_bits() {
let mut pte = TestPageTableEntry::default();
@@ -689,4 +698,39 @@ fn test_partial_bitfield() {
bf2 = bf2.set_state(0x55);
assert_eq!(bf2.state(), 0x1);
}
+
+ #[test]
+ fn test_wide_field_types() {
+ let mut wf = TestWideFields::default();
+
+ wf = wf.set_nibble(0x0000000F_u32);
+ assert_eq!(wf.nibble(), 0x0000000F_u32);
+
+ wf = wf.set_high_nibble(0x00000007_u32);
+ assert_eq!(wf.high_nibble(), 0x00000007_u32);
+
+ wf = wf.set_nibble(0xDEADBEEF_u32);
+ assert_eq!(wf.nibble(), 0x0000000F_u32);
+
+ wf = wf.set_high_nibble(0xCAFEBABE_u32);
+ assert_eq!(wf.high_nibble(), 0x0000000E_u32);
+
+ wf = wf.set_full(0xDEADBEEFCAFEBABE_u64);
+ assert_eq!(wf.full(), 0xBE_u64);
+
+ assert_eq!(wf.raw(), 0xBE_u8);
+
+ wf = TestWideFields::default()
+ .set_nibble(0x5_u32)
+ .set_high_nibble(0xA_u32);
+ assert_eq!(wf.raw(), 0xA5_u8);
+ assert_eq!(wf.nibble(), 0x5_u32);
+ assert_eq!(wf.high_nibble(), 0xA_u32);
+
+ // Test builder pattern
+ let wf2 = TestWideFields::default()
+ .set_nibble(0x12345678_u32) // truncated to 0x8
+ .set_high_nibble(0x9ABCDEF0_u32); // truncated to 0x0
+ assert_eq!(wf2.raw(), 0x08_u8);
+ }
}
--
2.34.1
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v4 6/6] rust: bitfield: Use 'as' operator for setter type conversion
2025-09-20 18:22 ` [PATCH v4 6/6] rust: bitfield: Use 'as' operator for setter type conversion Joel Fernandes
@ 2025-09-29 6:47 ` Alexandre Courbot
2025-09-29 20:32 ` Joel Fernandes
2025-09-29 13:59 ` Miguel Ojeda
1 sibling, 1 reply; 44+ messages in thread
From: Alexandre Courbot @ 2025-09-29 6:47 UTC (permalink / raw)
To: Joel Fernandes, linux-kernel, rust-for-linux, dri-devel, dakr,
acourbot
Cc: Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On Sun Sep 21, 2025 at 3:22 AM JST, Joel Fernandes wrote:
> The bitfield macro's setter currently uses the From trait for type
> conversion, which is overly restrictive and prevents use cases such as
> narrowing conversions (e.g., u32 storage size to u8 field size) which
> aren't supported by From.
>
> Replace 'from' with 'as' in the setter implementation to support this.
>
> Suggested-by: Yury Norov <yury.norov@gmail.com>
Can you add a `Link: ` tag to the discussion for context?
But I am not really convinced this is needed or desirable at all. Where
would it make sense to define a field that is larger that its containing
type? This looks like it can introduce confusion or errors. It's already
not ideal that we can pass values that would be truncated; but this
makes it worse.
Anyway, if we decide to keep this, I think you want to remove the
+//! Note that the compiler will error out if the size of the setter's
arg exceeds the
+//! struct's storage size.
bit that was introduced in patch 2.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 6/6] rust: bitfield: Use 'as' operator for setter type conversion
2025-09-29 6:47 ` Alexandre Courbot
@ 2025-09-29 20:32 ` Joel Fernandes
0 siblings, 0 replies; 44+ messages in thread
From: Joel Fernandes @ 2025-09-29 20:32 UTC (permalink / raw)
To: Alexandre Courbot
Cc: linux-kernel, rust-for-linux, dri-devel, dakr, Alistair Popple,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo, bjorn3_gh,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, John Hubbard, Timur Tabi, joel, Elle Rhumsaa,
Yury Norov, Daniel Almeida, nouveau
On Mon, Sep 29, 2025 at 03:47:08PM +0900, Alexandre Courbot wrote:
> On Sun Sep 21, 2025 at 3:22 AM JST, Joel Fernandes wrote:
> > The bitfield macro's setter currently uses the From trait for type
> > conversion, which is overly restrictive and prevents use cases such as
> > narrowing conversions (e.g., u32 storage size to u8 field size) which
> > aren't supported by From.
> >
> > Replace 'from' with 'as' in the setter implementation to support this.
> >
> > Suggested-by: Yury Norov <yury.norov@gmail.com>
>
> Can you add a `Link: ` tag to the discussion for context?
>
> But I am not really convinced this is needed or desirable at all. Where
> would it make sense to define a field that is larger that its containing
> type?
The 'as' keyword is not related to the containing struct IMO.
Example:
you can have a
struct Foo(u8) {
0:3 foo as u8;
4:7 bar as u8;
}
Here if you just go by the 'as u8', the total width would be 16. So we should
not conflate the 'as u8' with the '(u8)', they are already 2 separate things
and incompatible. I think the following would also work:
0:3 foo as u8 => u32;
However, directly using 'as u32' is a better shortcut IMO.
Would it help if I added more documentation comments about this?
> This looks like it can introduce confusion or errors. It's already
> not ideal that we can pass values that would be truncated; but this
> makes it worse.
Actually, in new series we're no longer truncating, I will post that shortly
after we conclude feedback on this series.
>
> Anyway, if we decide to keep this, I think you want to remove the
>
> +//! Note that the compiler will error out if the size of the setter's
> arg exceeds the
> +//! struct's storage size.
>
> bit that was introduced in patch 2.
Ah, good catch! Will remove the comment.
thanks,
- Joel
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 6/6] rust: bitfield: Use 'as' operator for setter type conversion
2025-09-20 18:22 ` [PATCH v4 6/6] rust: bitfield: Use 'as' operator for setter type conversion Joel Fernandes
2025-09-29 6:47 ` Alexandre Courbot
@ 2025-09-29 13:59 ` Miguel Ojeda
2025-09-29 14:44 ` Alexandre Courbot
` (2 more replies)
1 sibling, 3 replies; 44+ messages in thread
From: Miguel Ojeda @ 2025-09-29 13:59 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, rust-for-linux, dri-devel, dakr, acourbot,
Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On Sat, Sep 20, 2025 at 8:23 PM Joel Fernandes <joelagnelf@nvidia.com> wrote:
>
> The bitfield macro's setter currently uses the From trait for type
> conversion, which is overly restrictive and prevents use cases such as
> narrowing conversions (e.g., u32 storage size to u8 field size) which
> aren't supported by From.
Being restrictive is a good thing -- it would be nice to know more
context about this change, like Alexandre points out.
In particular, the line:
.set_nibble(0x12345678_u32) // truncated to 0x8
sounds fairly alarming, and not what we usually want. Why cannot the
caller cast on their side, if they really want that?
We avoid `as` for similar reasons and nowadays enable some Clippy
warnings to prevent its use where not needed.
(By the way, please follow our usual coding conventions for comments.)
Thanks!
Cheers,
Miguel
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v4 6/6] rust: bitfield: Use 'as' operator for setter type conversion
2025-09-29 13:59 ` Miguel Ojeda
@ 2025-09-29 14:44 ` Alexandre Courbot
2025-09-29 15:17 ` Miguel Ojeda
2025-09-30 12:03 ` Joel Fernandes
2025-09-29 15:26 ` Yury Norov
2025-09-29 20:46 ` Joel Fernandes
2 siblings, 2 replies; 44+ messages in thread
From: Alexandre Courbot @ 2025-09-29 14:44 UTC (permalink / raw)
To: Miguel Ojeda, Joel Fernandes
Cc: linux-kernel, rust-for-linux, dri-devel, dakr, acourbot,
Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On Mon Sep 29, 2025 at 10:59 PM JST, Miguel Ojeda wrote:
> On Sat, Sep 20, 2025 at 8:23 PM Joel Fernandes <joelagnelf@nvidia.com> wrote:
>>
>> The bitfield macro's setter currently uses the From trait for type
>> conversion, which is overly restrictive and prevents use cases such as
>> narrowing conversions (e.g., u32 storage size to u8 field size) which
>> aren't supported by From.
>
> Being restrictive is a good thing
On that note, I have been wondering whether we should not push the
restriction up to having bounded primitive types with only a set number
of bits valid, e.g. `bound_u8::<2>` is guaranteed to only contain values
in the range `0..=3`.
Getters and setters would use these types depending on the number of
bits of the field, meaning that a caller would have to validate the
value they want to write if it does not implement e.g.
`Into<bound_u8<2>>`.
A bit radical maybe, but correcness ensues. :)
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 6/6] rust: bitfield: Use 'as' operator for setter type conversion
2025-09-29 14:44 ` Alexandre Courbot
@ 2025-09-29 15:17 ` Miguel Ojeda
2025-09-30 12:03 ` Joel Fernandes
1 sibling, 0 replies; 44+ messages in thread
From: Miguel Ojeda @ 2025-09-29 15:17 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Joel Fernandes, linux-kernel, rust-for-linux, dri-devel, dakr,
Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On Mon, Sep 29, 2025 at 4:45 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> A bit radical maybe, but correcness ensues. :)
I am all for Ada/Pascal-like ranges and similar ones as long as we can
make them ergonomic enough with our current Rust and as long as we
still define newtypes over those when appropriate.
Rust may get support for that eventually too (called pattern types),
but we may want to do our own thing meanwhile, just like `Alignment`.
Cheers,
Miguel
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 6/6] rust: bitfield: Use 'as' operator for setter type conversion
2025-09-29 14:44 ` Alexandre Courbot
2025-09-29 15:17 ` Miguel Ojeda
@ 2025-09-30 12:03 ` Joel Fernandes
1 sibling, 0 replies; 44+ messages in thread
From: Joel Fernandes @ 2025-09-30 12:03 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Miguel Ojeda, linux-kernel, rust-for-linux, dri-devel, dakr,
Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On Mon, Sep 29, 2025 at 11:44:56PM +0900, Alexandre Courbot wrote:
> On Mon Sep 29, 2025 at 10:59 PM JST, Miguel Ojeda wrote:
> > On Sat, Sep 20, 2025 at 8:23 PM Joel Fernandes <joelagnelf@nvidia.com> wrote:
> >>
> >> The bitfield macro's setter currently uses the From trait for type
> >> conversion, which is overly restrictive and prevents use cases such as
> >> narrowing conversions (e.g., u32 storage size to u8 field size) which
> >> aren't supported by From.
> >
> > Being restrictive is a good thing
>
> On that note, I have been wondering whether we should not push the
> restriction up to having bounded primitive types with only a set number
> of bits valid, e.g. `bound_u8::<2>` is guaranteed to only contain values
> in the range `0..=3`.
>
> Getters and setters would use these types depending on the number of
> bits of the field, meaning that a caller would have to validate the
> value they want to write if it does not implement e.g.
> `Into<bound_u8<2>>`.
>
> A bit radical maybe, but correcness ensues. :)
In my v5, I will be rejecting setter inputs that are out of range. Do we have a
usecase where we want the inputs to exceed the bit width range? If not, let
us keep the API simple. I should probably post v5 today so we have a full
discussion on the same and get alignment from everyone.
Thanks
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 6/6] rust: bitfield: Use 'as' operator for setter type conversion
2025-09-29 13:59 ` Miguel Ojeda
2025-09-29 14:44 ` Alexandre Courbot
@ 2025-09-29 15:26 ` Yury Norov
2025-09-29 20:46 ` Joel Fernandes
2 siblings, 0 replies; 44+ messages in thread
From: Yury Norov @ 2025-09-29 15:26 UTC (permalink / raw)
To: Miguel Ojeda
Cc: Joel Fernandes, linux-kernel, rust-for-linux, dri-devel, dakr,
acourbot, Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Daniel Almeida, nouveau
On Mon, Sep 29, 2025 at 03:59:32PM +0200, Miguel Ojeda wrote:
> On Sat, Sep 20, 2025 at 8:23 PM Joel Fernandes <joelagnelf@nvidia.com> wrote:
> >
> > The bitfield macro's setter currently uses the From trait for type
> > conversion, which is overly restrictive and prevents use cases such as
> > narrowing conversions (e.g., u32 storage size to u8 field size) which
> > aren't supported by From.
>
> Being restrictive is a good thing -- it would be nice to know more
> context about this change, like Alexandre points out.
>
> In particular, the line:
>
> .set_nibble(0x12345678_u32) // truncated to 0x8
>
> sounds fairly alarming, and not what we usually want. Why cannot the
> caller cast on their side, if they really want that?
It was my suggestion to relax the type requirement. The reasoning is
as follows.
Consider a bitfield bf with bits 5:3 described as field1. The storage
for bf is u8, but the type is u32. This is OK, because storage and
representation are simply different matters. And no matter how you
declare the field inside the bitfield, you can't prevent overflow
followed by silent truncation by just syntax measures.
I suggested to relax the requirement that field representation must
match (not exceed in fact) storage type, and instead bring explicit
check in the setter. With the check, if user tries to overflow the
field, we either throw a warning, or panic if hardening is enabled,
or do nothing in performance-critical builds.
As far as I can say, Joel scheduled this in v5.
Thanks,
Yury
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 6/6] rust: bitfield: Use 'as' operator for setter type conversion
2025-09-29 13:59 ` Miguel Ojeda
2025-09-29 14:44 ` Alexandre Courbot
2025-09-29 15:26 ` Yury Norov
@ 2025-09-29 20:46 ` Joel Fernandes
2 siblings, 0 replies; 44+ messages in thread
From: Joel Fernandes @ 2025-09-29 20:46 UTC (permalink / raw)
To: Miguel Ojeda
Cc: linux-kernel, rust-for-linux, dri-devel, dakr, acourbot,
Alistair Popple, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
bjorn3_gh, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, David Airlie, Simona Vetter, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, John Hubbard, Timur Tabi, joel,
Elle Rhumsaa, Yury Norov, Daniel Almeida, nouveau
On Mon, Sep 29, 2025 at 03:59:32PM +0200, Miguel Ojeda wrote:
> On Sat, Sep 20, 2025 at 8:23 PM Joel Fernandes <joelagnelf@nvidia.com> wrote:
> >
> > The bitfield macro's setter currently uses the From trait for type
> > conversion, which is overly restrictive and prevents use cases such as
> > narrowing conversions (e.g., u32 storage size to u8 field size) which
> > aren't supported by From.
>
> Being restrictive is a good thing -- it would be nice to know more
> context about this change, like Alexandre points out.
Sure, I replied to that thread. Lets discuss there as well about the usecase.
> In particular, the line:
>
> .set_nibble(0x12345678_u32) // truncated to 0x8
>
> sounds fairly alarming, and not what we usually want. Why cannot the
> caller cast on their side, if they really want that?
The setter function generated in this example accepts a u32.
Actually my test case here is not good, I will fix it. In the new v5 series I
am going to post, set_nibble(0x12345678) will actually fail because the value
passed exceeds 4 bit range.
So in reality, there will be no truncation at all, this is just a bad
test case (I developed the failure mode for when the value passed exceeds
the bit range, only by v5).
> We avoid `as` for similar reasons and nowadays enable some Clippy
> warnings to prevent its use where not needed.
Understood, I can add a comment here to explain why we do it. It is just to
make the code compile, in reality we're not really truncating anything. The
issue solved in this patch is the following line wont compile when narrowing
from 32 bit to 8 bit (even if the value passed does not exceed 8 bits):
let val = (<$storage>::from(value) << SHIFT) & MASK;
> (By the way, please follow our usual coding conventions for comments.)
Sorry about that, will do.
Thanks!
- Joel
^ permalink raw reply [flat|nested] 44+ messages in thread