* [PATCH v3 1/5] nova-core: bitfield: Move bitfield-specific code from register! into new macro
2025-09-09 21:20 [PATCH v3 0/5] Introduce bitfield and move register macro to rust/kernel/ Joel Fernandes
@ 2025-09-09 21:20 ` Joel Fernandes
2025-09-09 21:20 ` [PATCH v3 2/5] nova-core: bitfield: Add support for different storage widths Joel Fernandes
` (3 subsequent siblings)
4 siblings, 0 replies; 19+ messages in thread
From: Joel Fernandes @ 2025-09-09 21:20 UTC (permalink / raw)
To: linux-kernel, 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 4dbc7e5daae3..eaba6ad22f7a 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] 19+ messages in thread* [PATCH v3 2/5] nova-core: bitfield: Add support for different storage widths
2025-09-09 21:20 [PATCH v3 0/5] Introduce bitfield and move register macro to rust/kernel/ Joel Fernandes
2025-09-09 21:20 ` [PATCH v3 1/5] nova-core: bitfield: Move bitfield-specific code from register! into new macro Joel Fernandes
@ 2025-09-09 21:20 ` Joel Fernandes
2025-09-09 21:20 ` [PATCH v3 3/5] nova-core: bitfield: Add support for custom visiblity Joel Fernandes
` (2 subsequent siblings)
4 siblings, 0 replies; 19+ messages in thread
From: Joel Fernandes @ 2025-09-09 21:20 UTC (permalink / raw)
To: linux-kernel, 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 | 69 +++++++++++++++++-----------
drivers/gpu/nova-core/regs/macros.rs | 16 +++----
2 files changed, 50 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs
index ba6b7caa05d9..824559c3462b 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;
//! }
@@ -77,21 +77,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 +101,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 +129,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 +144,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 +160,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 +194,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 +206,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 +227,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 +269,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 +284,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] 19+ messages in thread* [PATCH v3 3/5] nova-core: bitfield: Add support for custom visiblity
2025-09-09 21:20 [PATCH v3 0/5] Introduce bitfield and move register macro to rust/kernel/ Joel Fernandes
2025-09-09 21:20 ` [PATCH v3 1/5] nova-core: bitfield: Move bitfield-specific code from register! into new macro Joel Fernandes
2025-09-09 21:20 ` [PATCH v3 2/5] nova-core: bitfield: Add support for different storage widths Joel Fernandes
@ 2025-09-09 21:20 ` Joel Fernandes
2025-09-09 21:20 ` [PATCH v3 4/5] rust: Move register and bitfield macros out of Nova Joel Fernandes
2025-09-09 21:20 ` [PATCH v3 5/5] rust: Add KUNIT tests for bitfield Joel Fernandes
4 siblings, 0 replies; 19+ messages in thread
From: Joel Fernandes @ 2025-09-09 21:20 UTC (permalink / raw)
To: linux-kernel, 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 824559c3462b..39354e60360c 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;
//! }
@@ -63,6 +63,9 @@
//! - Field setters: `set_mode()`, `set_state()`, etc. (supports chaining with builder pattern).
//! - 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);
//!
@@ -77,21 +80,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;
@@ -113,14 +116,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)?
@@ -129,7 +132,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)?
@@ -144,7 +147,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)?
@@ -159,8 +162,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)?
@@ -194,11 +203,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)?;
);
@@ -206,17 +215,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,
@@ -227,24 +236,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!(
@@ -267,7 +276,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>];
@@ -283,7 +292,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] 19+ messages in thread* [PATCH v3 4/5] rust: Move register and bitfield macros out of Nova
2025-09-09 21:20 [PATCH v3 0/5] Introduce bitfield and move register macro to rust/kernel/ Joel Fernandes
` (2 preceding siblings ...)
2025-09-09 21:20 ` [PATCH v3 3/5] nova-core: bitfield: Add support for custom visiblity Joel Fernandes
@ 2025-09-09 21:20 ` Joel Fernandes
2025-09-09 21:36 ` Miguel Ojeda
2025-09-09 21:20 ` [PATCH v3 5/5] rust: Add KUNIT tests for bitfield Joel Fernandes
4 siblings, 1 reply; 19+ messages in thread
From: Joel Fernandes @ 2025-09-09 21:20 UTC (permalink / raw)
To: linux-kernel, 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.
[Added F: record to MAINTAINERS file entry as suggested by Yury.]
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 | 3 +-
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, 50 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 b97760467f09..ca9132fa4055 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4313,6 +4313,7 @@ F: include/asm-generic/bitops.h
F: include/linux/bitops.h
F: lib/test_bitops.c
F: tools/*/bitops*
+F: rust/kernel/bits*
BLINKM RGB LED DRIVER
M: Jan-Simon Moeller <jansimon.moeller@gmx.de>
diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
index 938f25b556a8..55f03f435138 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 c9ab375fd8a1..04920a619246 100644
--- a/drivers/gpu/nova-core/falcon/gsp.rs
+++ b/drivers/gpu/nova-core/falcon/gsp.rs
@@ -1,12 +1,13 @@
// SPDX-License-Identifier: GPL-2.0
+use kernel::io::register::RegisterBase;
use kernel::prelude::*;
use kernel::time::Delta;
use crate::{
driver::Bar0,
falcon::{Falcon, FalconEngine, PFalcon2Base, PFalconBase},
- regs::{self, macros::RegisterBase},
+ regs,
util::wait_on,
};
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 eaba6ad22f7a..4dbc7e5daae3 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 c214f8056d6e..07533eb6f64e 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
@@ -352,6 +350,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;
@@ -360,6 +359,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 39354e60360c..0837fefc270f 100644
--- a/drivers/gpu/nova-core/bitfield.rs
+++ b/rust/kernel/bits/bitfield.rs
@@ -78,10 +78,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.
@@ -116,7 +119,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.
@@ -132,7 +135,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)?
@@ -141,8 +144,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`.
@@ -157,7 +160,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)]
@@ -169,7 +172,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)?
@@ -206,7 +209,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)?;
@@ -217,7 +220,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.
@@ -225,7 +228,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,
@@ -239,7 +242,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)?;);
};
@@ -248,7 +251,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 03b467722b86..a79b603604b1 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -8,6 +8,7 @@
use crate::{bindings, build_assert, ffi::c_void};
pub mod mem;
+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] 19+ messages in thread* Re: [PATCH v3 4/5] rust: Move register and bitfield macros out of Nova
2025-09-09 21:20 ` [PATCH v3 4/5] rust: Move register and bitfield macros out of Nova Joel Fernandes
@ 2025-09-09 21:36 ` Miguel Ojeda
2025-09-10 0:15 ` Joel Fernandes
0 siblings, 1 reply; 19+ messages in thread
From: Miguel Ojeda @ 2025-09-09 21:36 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, 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, rust-for-linux
On Tue, Sep 9, 2025 at 11:21 PM Joel Fernandes <joelagnelf@nvidia.com> wrote:
>
> Out of broad need for these macros in Rust, move them out. Several folks
> have shown interest (Nova, Tyr GPU drivers).
Please Cc the rust-for-Linux mailing list, especially so for patches
that add things to the core infrastructure.
I notice easily because I tag the ones that are in my client :)
> [Added F: record to MAINTAINERS file entry as suggested by Yury.]
Please don't use [ ... ] nor the past tense -- for normal changes,
please use the imperative instead.
(I guess you picked this up from other [ ... ] notation, but that is
normally only done for modifications of a patch by someone else, e.g.
by maintainers.)
Thanks!
Cheers,
Miguel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 4/5] rust: Move register and bitfield macros out of Nova
2025-09-09 21:36 ` Miguel Ojeda
@ 2025-09-10 0:15 ` Joel Fernandes
0 siblings, 0 replies; 19+ messages in thread
From: Joel Fernandes @ 2025-09-10 0:15 UTC (permalink / raw)
To: Miguel Ojeda
Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
dakr@kernel.org, Alexandre Courbot, Alistair Popple, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, bjorn3_gh@protonmail.com,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, John Hubbard, Timur Tabi,
joel@joelfernandes.org, Elle Rhumsaa, Yury Norov, Daniel Almeida,
nouveau@lists.freedesktop.org, rust-for-linux
> On Sep 9, 2025, at 5:36 PM, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Tue, Sep 9, 2025 at 11:21 PM Joel Fernandes <joelagnelf@nvidia.com> wrote:
>>
>> Out of broad need for these macros in Rust, move them out. Several folks
>> have shown interest (Nova, Tyr GPU drivers).
>
> Please Cc the rust-for-Linux mailing list, especially so for patches
> that add things to the core infrastructure.
>
> I notice easily because I tag the ones that are in my client :)
Oops my bad. I shall do so, will await other comments if any before reposting.
>
>> [Added F: record to MAINTAINERS file entry as suggested by Yury.]
>
> Please don't use [ ... ] nor the past tense -- for normal changes,
> please use the imperative instead.
Sure, will do.
thanks,
- Joel
>
> (I guess you picked this up from other [ ... ] notation, but that is
> normally only done for modifications of a patch by someone else, e.g.
> by maintainers.)
>
> Thanks!
>
> Cheers,
> Miguel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 5/5] rust: Add KUNIT tests for bitfield
2025-09-09 21:20 [PATCH v3 0/5] Introduce bitfield and move register macro to rust/kernel/ Joel Fernandes
` (3 preceding siblings ...)
2025-09-09 21:20 ` [PATCH v3 4/5] rust: Move register and bitfield macros out of Nova Joel Fernandes
@ 2025-09-09 21:20 ` Joel Fernandes
2025-09-10 3:04 ` Yury Norov
4 siblings, 1 reply; 19+ messages in thread
From: Joel Fernandes @ 2025-09-09 21:20 UTC (permalink / raw)
To: linux-kernel, 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.
[Added range overlap tests suggested by Yury].
Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
rust/kernel/bits/bitfield.rs | 320 +++++++++++++++++++++++++++++++++++
1 file changed, 320 insertions(+)
diff --git a/rust/kernel/bits/bitfield.rs b/rust/kernel/bits/bitfield.rs
index 0837fefc270f..f3134f2ffd08 100644
--- a/rust/kernel/bits/bitfield.rs
+++ b/rust/kernel/bits/bitfield.rs
@@ -339,3 +339,323 @@ 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; // 4-bit field for testing failures
+ 51:12 pfn as u64;
+ 51:12 pfn_overlap as u64; // Overlapping field
+ 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; // Overlapping field
+ 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; // Overlapping field for entire register
+ }
+ }
+
+ #[test]
+ fn test_single_bits() {
+ let mut pte = TestPageTableEntry::default();
+
+ // Test bool field
+ 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();
+ raw = (raw & !(0xF << 14)) | (0x7 << 14); // Set bits 17:14 to 7 (invalid for MemoryType)
+ let invalid_pte = TestPageTableEntry::from(raw);
+ assert_eq!(invalid_pte.extended_type(), Err(0x7)); // Should return Err with the invalid value
+
+ // Test a valid value after testing invalid to ensure both cases work
+ raw = (raw & !(0xF << 14)) | (0x2 << 14); // Set bits 17:14 to 2 (valid: Device)
+ let valid_pte = TestPageTableEntry::from(raw);
+ assert_eq!(valid_pte.extended_type(), Ok(MemoryType::Device)); // Should return Ok with Device
+
+ let max_pfn = (1u64 << 40) - 1;
+ 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);
+ }
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 5/5] rust: Add KUNIT tests for bitfield
2025-09-09 21:20 ` [PATCH v3 5/5] rust: Add KUNIT tests for bitfield Joel Fernandes
@ 2025-09-10 3:04 ` Yury Norov
2025-09-10 23:08 ` Joel Fernandes
2025-09-10 23:22 ` Joel Fernandes
0 siblings, 2 replies; 19+ messages in thread
From: Yury Norov @ 2025-09-10 3:04 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, 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 Tue, Sep 09, 2025 at 05:20:39PM -0400, Joel Fernandes wrote:
> Add KUNIT tests to make sure the macro is working correctly.
>
> [Added range overlap tests suggested by Yury].
Please no brackets and personal references in commit messages.
If you believe it's worth mentioning my suggestion, then:
Suggested-by: Yury Norov (NVIDIA) <yury.norov@gmail.com> # For overlapped tests
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
Some comments inline.
Thanks,
Yury
> ---
> rust/kernel/bits/bitfield.rs | 320 +++++++++++++++++++++++++++++++++++
> 1 file changed, 320 insertions(+)
>
> diff --git a/rust/kernel/bits/bitfield.rs b/rust/kernel/bits/bitfield.rs
> index 0837fefc270f..f3134f2ffd08 100644
> --- a/rust/kernel/bits/bitfield.rs
> +++ b/rust/kernel/bits/bitfield.rs
> @@ -339,3 +339,323 @@ 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; // 4-bit field for testing failures
> + 51:12 pfn as u64;
> + 51:12 pfn_overlap as u64; // Overlapping field
> + 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; // Overlapping field
> + 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; // Overlapping field for entire register
> + }
> + }
> +
> + #[test]
> + fn test_single_bits() {
> + let mut pte = TestPageTableEntry::default();
> +
> + // Test bool field
> + 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();
> + raw = (raw & !(0xF << 14)) | (0x7 << 14); // Set bits 17:14 to 7 (invalid for MemoryType)
> + let invalid_pte = TestPageTableEntry::from(raw);
> + assert_eq!(invalid_pte.extended_type(), Err(0x7)); // Should return Err with the invalid value
Please make sure your lines don't exceed 100 chars, preferably less
than 80.
> +
> + // Test a valid value after testing invalid to ensure both cases work
> + raw = (raw & !(0xF << 14)) | (0x2 << 14); // Set bits 17:14 to 2 (valid: Device)
Can you use genmask!() here and everywhere else?
> + let valid_pte = TestPageTableEntry::from(raw);
> + assert_eq!(valid_pte.extended_type(), Ok(MemoryType::Device)); // Should return Ok with Device
> +
> + let max_pfn = (1u64 << 40) - 1;
> + 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);
You've got only one negative test that covers the .from() method.
Can you add more?
What if I create a bitfield from a runtime value that exceeds
the capacity?
bitfield! {
struct bf: u8 {
0:0 ready as bool;
1:1 error as bool;
3:2 state as u32;
}
}
let raw_value: u8 = 0xff;
let bf = bf::from(raw_value);
I guess you'd return None or similar. Can you add such a test?
The same question for the setters. What would happen for this:
let bf = bf::default()
.set_state(0xf)
.set_ready(true);
I think that after the first out-of-boundary in set_state(), you
should abort the call chain, make sure you're not touching memory
in set_ready() and returning some type of error.
And for this:
let ret: u32 = -EINVAL;
bf = bf::default();
bf = bf.set_state(ret);
For compile-time initializes, it should be a compile-time error, right?
Can you drop a comment on that?
(In C we've got FIELD_{GET,MODIFY,PREP}. They cover all the static
cases.)
> + 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);
> + }
> +}
> --
> 2.34.1
I tried to apply your series on top of master, but it failed. So
my apologies for not finding the answers to some questions above
by myself.
For the next version, can you make sure your series is applicable
on top of master or -next?
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 5/5] rust: Add KUNIT tests for bitfield
2025-09-10 3:04 ` Yury Norov
@ 2025-09-10 23:08 ` Joel Fernandes
2025-09-11 1:47 ` Yury Norov
2025-09-10 23:22 ` Joel Fernandes
1 sibling, 1 reply; 19+ messages in thread
From: Joel Fernandes @ 2025-09-10 23:08 UTC (permalink / raw)
To: Yury Norov
Cc: linux-kernel, 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 9/9/2025 11:04 PM, Yury Norov wrote:
> On Tue, Sep 09, 2025 at 05:20:39PM -0400, Joel Fernandes wrote:
>> Add KUNIT tests to make sure the macro is working correctly.
>>
>> [Added range overlap tests suggested by Yury].
>
> Please no brackets and personal references in commit messages.
> If you believe it's worth mentioning my suggestion, then:
>
> Suggested-by: Yury Norov (NVIDIA) <yury.norov@gmail.com> # For overlapped tests
>
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>
> Some comments inline.
>
> Thanks,
> Yury
>
>> ---
>> rust/kernel/bits/bitfield.rs | 320 +++++++++++++++++++++++++++++++++++
>> 1 file changed, 320 insertions(+)
>>
>> diff --git a/rust/kernel/bits/bitfield.rs b/rust/kernel/bits/bitfield.rs
>> index 0837fefc270f..f3134f2ffd08 100644
>> --- a/rust/kernel/bits/bitfield.rs
>> +++ b/rust/kernel/bits/bitfield.rs
>> @@ -339,3 +339,323 @@ 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; // 4-bit field for testing failures
>> + 51:12 pfn as u64;
>> + 51:12 pfn_overlap as u64; // Overlapping field
>> + 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; // Overlapping field
>> + 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; // Overlapping field for entire register
>> + }
>> + }
>> +
>> + #[test]
>> + fn test_single_bits() {
>> + let mut pte = TestPageTableEntry::default();
>> +
>> + // Test bool field
>> + 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();
>> + raw = (raw & !(0xF << 14)) | (0x7 << 14); // Set bits 17:14 to 7 (invalid for MemoryType)
>> + let invalid_pte = TestPageTableEntry::from(raw);
>> + assert_eq!(invalid_pte.extended_type(), Err(0x7)); // Should return Err with the invalid value
>
> Please make sure your lines don't exceed 100 chars, preferably less
> than 80.
>
>> +
>> + // Test a valid value after testing invalid to ensure both cases work
>> + raw = (raw & !(0xF << 14)) | (0x2 << 14); // Set bits 17:14 to 2 (valid: Device)
>
> Can you use genmask!() here and everywhere else?
>
>> + let valid_pte = TestPageTableEntry::from(raw);
>> + assert_eq!(valid_pte.extended_type(), Ok(MemoryType::Device)); // Should return Ok with Device
>> +
>> + let max_pfn = (1u64 << 40) - 1;
>> + 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);
>
> You've got only one negative test that covers the .from() method.
> Can you add more?
Sure, but note that we can only add negative tests if there is a chance of
failure, which at runtime can mainly happen with the fallible usage (?=>
pattern). Also just to note, we already at ~300 lines of test code now :)
>
> What if I create a bitfield from a runtime value that exceeds
> the capacity?
>
> bitfield! {
> struct bf: u8 {
> 0:0 ready as bool;
> 1:1 error as bool;
> 3:2 state as u32;
Here you mean 'as u8', otherwise it wont compile.
> }
> }
>
> let raw_value: u8 = 0xff;
> let bf = bf::from(raw_value);
>
> I guess you'd return None or similar.
No, we would ignore the extra bits sent. There is a .raw() method and 'bf' is
8-bits, bf.raw() will return 0xff. So it is perfectly valid to do so. I don't
think we should return None here, this is also valid in C.
> Can you add such a test?
Sure, I added such a test.
> The same question for the setters. What would happen for this:
>
> let bf = bf::default()
> .set_state(0xf)
> .set_ready(true);
>
> I think that after the first out-of-boundary in set_state(), you
> should abort the call chain, make sure you're not touching memory
> in set_ready() and returning some type of error.
Here, on out of boundary, we just ignore the extra bits passed to set_state. I
think it would be odd if we errored out honestly. We are using 'as u8' in the
struct so we would accept any u8 as input, but then if we complained that extra
bits were sent, that would be odd. In C also this is valid. If you passed a
higher value than what the bitfield can hold, the compiler will still just use
the bits that it needs and ignore the rest.
Now, I am not opposed to error'ing out on that, but that's not what we currently
do and it is also not easy to do. The setters in the patch return Self, not
Result<Self>, so they are infallible, which is what allows them to be chained as
well (builder pattern).
I added another test here as well, to ensure the behavior is as I describe.
>
> And for this:
>
> let ret: u32 = -EINVAL;
> bf = bf::default();
> bf = bf.set_state(ret);
>
> For compile-time initializes, it should be a compile-time error, right?
Yes, since the struct in this example is u8, this wont compile. Yes, I will add
a comment.
> Can you drop a comment on that?
Yes, I will do so.
>
> I tried to apply your series on top of master, but it failed. So
> my apologies for not finding the answers to some questions above
> by myself.
Oh ok, I applied it on top of drm-rust-next. I will rebase on -next for the next
revision, thanks.
> For the next version, can you make sure your series is applicable
> on top of master or -next?
Sure, thanks.
- Joel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 5/5] rust: Add KUNIT tests for bitfield
2025-09-10 23:08 ` Joel Fernandes
@ 2025-09-11 1:47 ` Yury Norov
2025-09-16 9:59 ` Joel Fernandes
0 siblings, 1 reply; 19+ messages in thread
From: Yury Norov @ 2025-09-11 1:47 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, 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 Wed, Sep 10, 2025 at 07:08:43PM -0400, Joel Fernandes wrote:
> > You've got only one negative test that covers the .from() method.
> > Can you add more?
>
> Sure, but note that we can only add negative tests if there is a chance of
> failure, which at runtime can mainly happen with the fallible usage (?=>
> pattern). Also just to note, we already at ~300 lines of test code now :)
>
> >
> > What if I create a bitfield from a runtime value that exceeds
> > the capacity?
> >
> > bitfield! {
> > struct bf: u8 {
> > 0:0 ready as bool;
> > 1:1 error as bool;
> > 3:2 state as u32;
> Here you mean 'as u8', otherwise it wont compile.
No, I meant u32. Can you explain this limitation in docs please? From
a user perspective, the 'state' is a 2-bit variable, so any type wider
than that should be OK to hold the data. If it's just an implementation
limitation, maybe it's worth to relax it?
> > }
> > }
> >
> > let raw_value: u8 = 0xff;
> > let bf = bf::from(raw_value);
> >
> > I guess you'd return None or similar.
>
> No, we would ignore the extra bits sent. There is a .raw() method and 'bf' is
> 8-bits, bf.raw() will return 0xff. So it is perfectly valid to do so.
So I'm lost. Do you ignore or keep untouched?
Imagine a code relying on the behavior you've just described. So, I
create a 5-bit bitfield residing in a u8 storage, and my user one
day starts using that 3-bit tail for his own purposes.
Is that OK? Can you guarantee that any possible combination of methods
that you've implemented or will implement in future will keep the tail
untouched?
In bitmaps, even for a single-bit bitmap the API reserves the whole word,
thus we have a similar problem. And we state clearly that any bit beyond
the requested area is 'don't care'. It's OK for C. Is it OK for rust?
(And even that, we have a couple of functions that take care of tails
for some good reasons.)
So the question is: do you
- provide the same minimal guarantees as C does (undefined behavior); or
- preserve tails untouched, so user can play with them; or
- clean the tails for user; or
- reject such requests?
Or something else? Whichever option you choose, please describe
it explicitly.
> I don't
> think we should return None here, this is also valid in C.
This doesn't sound like an argument in the rust world, isn't? :) I've
been told many times that the main purpose of rust is the bullet-proof
way of coding. Particularly: "rust is free of undefined behavior gray
zone".
> > Can you add such a test?
>
> Sure, I added such a test.
>
> > The same question for the setters. What would happen for this:
> >
> > let bf = bf::default()
> > .set_state(0xf)
> > .set_ready(true);
> >
> > I think that after the first out-of-boundary in set_state(), you
> > should abort the call chain, make sure you're not touching memory
> > in set_ready() and returning some type of error.
>
> Here, on out of boundary, we just ignore the extra bits passed to set_state. I
> think it would be odd if we errored out honestly. We are using 'as u8' in the
> struct so we would accept any u8 as input, but then if we complained that extra
> bits were sent, that would be odd.
That really depends on your purpose. If your end goal is the safest API
in the world, and you're ready to sacrifice some performance (which is
exactly opposite to the C case), then you'd return to your user with a
simple question: are you sure you can fit this 8-bit number into a 3-bit
storage?
> In C also this is valid. If you passed a
> higher value than what the bitfield can hold, the compiler will still just use
> the bits that it needs and ignore the rest.
In C we've got FIELD_{PREP,GET,MODIFY}, implementing the checks.
So those who want to stay on safe side have a choice.
> Now, I am not opposed to error'ing out on that, but that's not what we currently
> do and it is also not easy to do. The setters in the patch return Self, not
> Result<Self>, so they are infallible, which is what allows them to be chained as
> well (builder pattern).
That 'chainability' looks optional to me, not saying weird, anyways.
> I added another test here as well, to ensure the behavior is as I describe.
>
> >
> > And for this:
> >
> > let ret: u32 = -EINVAL;
> > bf = bf::default();
> > bf = bf.set_state(ret);
> >
> > For compile-time initializes, it should be a compile-time error, right?
>
> Yes, since the struct in this example is u8, this wont compile. Yes, I will add
> a comment.
So, the following would work?
bitfield! {
struct bf: u32 {
0:0 ready as bool;
1:1 error as bool;
3:2 state as u32;
...
}
}
let state: u32 = some_C_wrapper(); // returns 0..3 or -EINVAL
bf = bf::default();
bf = bf.set_state(state);
That doesn't look right...
> > Can you drop a comment on that?
>
> Yes, I will do so.
>
> >
> > I tried to apply your series on top of master, but it failed. So
> > my apologies for not finding the answers to some questions above
> > by myself.
>
> Oh ok, I applied it on top of drm-rust-next. I will rebase on -next for the next
> revision, thanks.
>
> > For the next version, can you make sure your series is applicable
> > on top of master or -next?
> Sure, thanks.
> - Joel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 5/5] rust: Add KUNIT tests for bitfield
2025-09-11 1:47 ` Yury Norov
@ 2025-09-16 9:59 ` Joel Fernandes
2025-09-16 10:10 ` Joel Fernandes
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Joel Fernandes @ 2025-09-16 9:59 UTC (permalink / raw)
To: Yury Norov
Cc: linux-kernel, 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
Hi Jury,
Sorry for late reply, I was busy with conference travel. Now I found a 3 hour
break before my train journey. :-)
On Wed, Sep 10, 2025 at 09:47:04PM -0400, Yury Norov wrote:
> On Wed, Sep 10, 2025 at 07:08:43PM -0400, Joel Fernandes wrote:
> > > You've got only one negative test that covers the .from() method.
> > > Can you add more?
> >
> > Sure, but note that we can only add negative tests if there is a chance of
> > failure, which at runtime can mainly happen with the fallible usage (?=>
> > pattern). Also just to note, we already at ~300 lines of test code now :)
> >
> > >
> > > What if I create a bitfield from a runtime value that exceeds
> > > the capacity?
> > >
> > > bitfield! {
> > > struct bf: u8 {
> > > 0:0 ready as bool;
> > > 1:1 error as bool;
> > > 3:2 state as u32;
> > Here you mean 'as u8', otherwise it wont compile.
>
> No, I meant u32. Can you explain this limitation in docs please? From
> a user perspective, the 'state' is a 2-bit variable, so any type wider
> than that should be OK to hold the data. If it's just an implementation
> limitation, maybe it's worth to relax it?
Yes it is a limitation because of the way the code does mask and shifts, it
requires the width to not exceed the width of the struct itself. Yes, I can
add a comment.
I think to do what you want, you have to write it as 'as u8 => u32'.
Otherwise it wont compile.
Just to note, the bitfield code reused the code in the register macro, so it
is existing code in nova-core. We can improve it, but I just did a code
movement with few features on top (sizes other than u32 for the overall
struct, visibility etc) so I look at such changes as an improvement on top
(which will be other patches in this series or later). We can certainly
improve the bitfield support now and as we go.
> > > }
> > > }
> > >
> > > let raw_value: u8 = 0xff;
> > > let bf = bf::from(raw_value);
> > >
> > > I guess you'd return None or similar.
> >
> > No, we would ignore the extra bits sent. There is a .raw() method and 'bf' is
> > 8-bits, bf.raw() will return 0xff. So it is perfectly valid to do so.
>
> So I'm lost. Do you ignore or keep untouched?
It would be ignored for the field, but kept in the struct. So .raw() will
return the full 8 bits, and the field will return a subset.
> Imagine a code relying on the behavior you've just described. So, I
> create a 5-bit bitfield residing in a u8 storage, and my user one
> day starts using that 3-bit tail for his own purposes.
>
> Is that OK? Can you guarantee that any possible combination of methods
> that you've implemented or will implement in future will keep the tail
> untouched?
>
> In bitmaps, even for a single-bit bitmap the API reserves the whole word,
> thus we have a similar problem. And we state clearly that any bit beyond
> the requested area is 'don't care'. It's OK for C. Is it OK for rust?
>
> (And even that, we have a couple of functions that take care of tails
> for some good reasons.)
>
> So the question is: do you
> - provide the same minimal guarantees as C does (undefined behavior); or
> - preserve tails untouched, so user can play with them; or
> - clean the tails for user; or
> - reject such requests?
>
> Or something else? Whichever option you choose, please describe
> it explicitly.
I feel this is macro-user's policy, if the user wants to use hidden bits,
they should document it in their struct. They could mention it is 'dont care'
or 'do not touch' in code comments. Obviously if they decide not to expose it
in the fields, that would be one way to deter users of the struct from
touching it without knowing what they are done. In that sense it is
undefined behavior, it is up to the user I'd say. Does that sound reasonable?
> > I don't
> > think we should return None here, this is also valid in C.
>
> This doesn't sound like an argument in the rust world, isn't? :) I've
> been told many times that the main purpose of rust is the bullet-proof
> way of coding. Particularly: "rust is free of undefined behavior gray
> zone".
>
Since we only partially quoted my reply, lets take a step back and paste the
code snip here again. The following should not return None IMO:
bitfield! {
struct bf: u8 {
0:0 ready as bool;
1:1 error as bool;
3:2 state as u32;
}
}
let raw_value: u8 = 0xff;
let b = bf::from(raw_value);
Maybe I used 'ignore' incorrectly in my last reply. The above code snip is
perfectly valid code IMO. Because b.raw() will return 0xff. The fact that we
don't have defined bitfields for the value should not prevent us from
accessing the entire raw value right? If we want, we can set it up as policy
that there are really no undefined bits, everything is defined even if only a
few of them have accessors, and '.raw()' is the ultimate catch-all. Does that
sound reasonable?
> > Sure, I added such a test.
> >
> > > The same question for the setters. What would happen for this:
> > >
> > > let bf = bf::default()
> > > .set_state(0xf)
> > > .set_ready(true);
> > >
> > > I think that after the first out-of-boundary in set_state(), you
> > > should abort the call chain, make sure you're not touching memory
> > > in set_ready() and returning some type of error.
> >
> > Here, on out of boundary, we just ignore the extra bits passed to set_state. I
> > think it would be odd if we errored out honestly. We are using 'as u8' in the
> > struct so we would accept any u8 as input, but then if we complained that extra
> > bits were sent, that would be odd.
>
> That really depends on your purpose. If your end goal is the safest API
> in the world, and you're ready to sacrifice some performance (which is
> exactly opposite to the C case), then you'd return to your user with a
> simple question: are you sure you can fit this 8-bit number into a 3-bit
> storage?
I think personally I am OK with rejecting requests about this, so we can
agree on this.
> > In C also this is valid. If you passed a
> > higher value than what the bitfield can hold, the compiler will still just use
> > the bits that it needs and ignore the rest.
>
> In C we've got FIELD_{PREP,GET,MODIFY}, implementing the checks.
> So those who want to stay on safe side have a choice.
Ah ok. We can add these checks then for the accessors, I will do so in v4.
> > I added another test here as well, to ensure the behavior is as I describe.
> >
> > >
> > > And for this:
> > >
> > > let ret: u32 = -EINVAL;
> > > bf = bf::default();
> > > bf = bf.set_state(ret);
> > >
> > > For compile-time initializes, it should be a compile-time error, right?
> >
> > Yes, since the struct in this example is u8, this wont compile. Yes, I will add
> > a comment.
>
> So, the following would work?
>
> bitfield! {
> struct bf: u32 {
> 0:0 ready as bool;
> 1:1 error as bool;
> 3:2 state as u32;
> ...
> }
> }
>
> let state: u32 = some_C_wrapper(); // returns 0..3 or -EINVAL
> bf = bf::default();
> bf = bf.set_state(state);
>
> That doesn't look right...
I agree with you, a better approach is to reject anything great than 2 bits.
We do agree on that, and I can make that change. *Currently* what happens is
we mask and shift ignoring all extra bits passed, instead of rejecting.
I hope we're on the same page now, but let me know any other concerns. Just
to emphasize again, I moved *existing* code out of the register macro related
to bitfields, so this has been mostly a code move. That said, we can
certainly improve it incrementally.
Thanks!
- Joel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 5/5] rust: Add KUNIT tests for bitfield
2025-09-16 9:59 ` Joel Fernandes
@ 2025-09-16 10:10 ` Joel Fernandes
2025-09-19 23:02 ` Joel Fernandes
2025-09-20 0:39 ` Joel Fernandes
2 siblings, 0 replies; 19+ messages in thread
From: Joel Fernandes @ 2025-09-16 10:10 UTC (permalink / raw)
To: Yury Norov
Cc: linux-kernel, 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 Tue, Sep 16, 2025 at 05:59:18AM -0400, Joel Fernandes wrote:
> Hi Jury,
> Sorry for late reply, I was busy with conference travel. Now I found a 3 hour
> break before my train journey. :-)
>
> On Wed, Sep 10, 2025 at 09:47:04PM -0400, Yury Norov wrote:
> > On Wed, Sep 10, 2025 at 07:08:43PM -0400, Joel Fernandes wrote:
> > > > You've got only one negative test that covers the .from() method.
> > > > Can you add more?
> > >
> > > Sure, but note that we can only add negative tests if there is a chance of
> > > failure, which at runtime can mainly happen with the fallible usage (?=>
> > > pattern). Also just to note, we already at ~300 lines of test code now :)
> > >
> > > >
> > > > What if I create a bitfield from a runtime value that exceeds
> > > > the capacity?
> > > >
> > > > bitfield! {
> > > > struct bf: u8 {
> > > > 0:0 ready as bool;
> > > > 1:1 error as bool;
> > > > 3:2 state as u32;
> > > Here you mean 'as u8', otherwise it wont compile.
> >
> > No, I meant u32. Can you explain this limitation in docs please? From
> > a user perspective, the 'state' is a 2-bit variable, so any type wider
> > than that should be OK to hold the data. If it's just an implementation
> > limitation, maybe it's worth to relax it?
>
> Yes it is a limitation because of the way the code does mask and shifts, it
> requires the width to not exceed the width of the struct itself. Yes, I can
> add a comment.
>
> I think to do what you want, you have to write it as 'as u8 => u32'.
> Otherwise it wont compile.
I think I am convinced we should fix this, it is too much of a burden to the
user otherwise. 'as u32' should just work out of the box.. I'll add support
for this in v4 but if anyone has any objections, please let me know, thanks!
- Joel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 5/5] rust: Add KUNIT tests for bitfield
2025-09-16 9:59 ` Joel Fernandes
2025-09-16 10:10 ` Joel Fernandes
@ 2025-09-19 23:02 ` Joel Fernandes
2025-09-20 0:39 ` Joel Fernandes
2 siblings, 0 replies; 19+ messages in thread
From: Joel Fernandes @ 2025-09-19 23:02 UTC (permalink / raw)
To: Yury Norov, acourbot
Cc: linux-kernel, 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,
Daniel Almeida, nouveau
On Tue, Sep 16, 2025 at 05:59:18AM -0400, Joel Fernandes wrote:
[...]
> > > > The same question for the setters. What would happen for this:
> > > >
> > > > let bf = bf::default()
> > > > .set_state(0xf)
> > > > .set_ready(true);
> > > >
> > > > I think that after the first out-of-boundary in set_state(), you
> > > > should abort the call chain, make sure you're not touching memory
> > > > in set_ready() and returning some type of error.
> > >
> > > Here, on out of boundary, we just ignore the extra bits passed to
> > > set_state. I think it would be odd if we errored out honestly. We are
> > > using 'as u8' in the struct so we would accept any u8 as input, but
> > > then if we complained that extra bits were sent, that would be odd.
> >
> > That really depends on your purpose. If your end goal is the safest API
> > in the world, and you're ready to sacrifice some performance (which is
> > exactly opposite to the C case), then you'd return to your user with a
> > simple question: are you sure you can fit this 8-bit number into a 3-bit
> > storage?
>
> I think personally I am OK with rejecting requests about this, so we can
> agree on this.
It is not possible to reject values passed to set, because it returns Self
and follows the builder-pattern, to do this we will need to return Result,
and have the caller unwrap it, and have to rename it to try_set().
Instead of that, I would just extract the bits from the value the user passed
and ignore the rest (example if 0xff is passed for a 4-bit bitfield, then the
field is 0xf.)
Alex what do you think, should set_ be fallible?
thanks,
- Joel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 5/5] rust: Add KUNIT tests for bitfield
2025-09-16 9:59 ` Joel Fernandes
2025-09-16 10:10 ` Joel Fernandes
2025-09-19 23:02 ` Joel Fernandes
@ 2025-09-20 0:39 ` Joel Fernandes
2025-09-20 0:52 ` John Hubbard
2025-09-21 11:41 ` Miguel Ojeda
2 siblings, 2 replies; 19+ messages in thread
From: Joel Fernandes @ 2025-09-20 0:39 UTC (permalink / raw)
To: Yury Norov
Cc: linux-kernel, 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 Tue, Sep 16, 2025 at 05:59:18AM -0400, Joel Fernandes wrote:
[...]
> > > In C also this is valid. If you passed a higher value than what the
> > > bitfield can hold, the compiler will still just use the bits that it
> > > needs and ignore the rest.
> >
> > In C we've got FIELD_{PREP,GET,MODIFY}, implementing the checks.
> > So those who want to stay on safe side have a choice.
>
> Ah ok. We can add these checks then for the accessors, I will do so in v4.
The C checks use BUILD_BUG_ON, in rust-for-linux we have build_assert but it
is fragile and depends on the value being a constant. Since the setter API
accepts a run-time value and not a constant, we cannot use this.
Or, we can fail at runtime, but that requires changing the set_* to try_set_*
and returning a Result instead of Self. Alternatively, we can have a debug
option that panics if the setter API is misued.
Thoughts?
Or for the moment, we can keep it simple and filter out / ignore extra bits
of the larger value passed (which is what nova-core's register macro bitfield
implementation currently does anyway).
thanks,
- Joel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 5/5] rust: Add KUNIT tests for bitfield
2025-09-20 0:39 ` Joel Fernandes
@ 2025-09-20 0:52 ` John Hubbard
2025-09-20 9:33 ` Joel Fernandes
2025-09-21 11:41 ` Miguel Ojeda
1 sibling, 1 reply; 19+ messages in thread
From: John Hubbard @ 2025-09-20 0:52 UTC (permalink / raw)
To: Joel Fernandes, Yury Norov
Cc: linux-kernel, 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, Timur Tabi, joel, Elle Rhumsaa, Daniel Almeida,
nouveau
On 9/19/25 5:39 PM, Joel Fernandes wrote:
> On Tue, Sep 16, 2025 at 05:59:18AM -0400, Joel Fernandes wrote:
> [...]
>>>> In C also this is valid. If you passed a higher value than what the
>>>> bitfield can hold, the compiler will still just use the bits that it
>>>> needs and ignore the rest.
>>>
>>> In C we've got FIELD_{PREP,GET,MODIFY}, implementing the checks.
>>> So those who want to stay on safe side have a choice.
>>
>> Ah ok. We can add these checks then for the accessors, I will do so in v4.
>
> The C checks use BUILD_BUG_ON, in rust-for-linux we have build_assert but it
> is fragile and depends on the value being a constant. Since the setter API
> accepts a run-time value and not a constant, we cannot use this.
>
> Or, we can fail at runtime, but that requires changing the set_* to try_set_*
> and returning a Result instead of Self. Alternatively, we can have a debug
> option that panics if the setter API is misued.
Please no...
>
> Thoughts?
>
> Or for the moment, we can keep it simple and filter out / ignore extra bits
> of the larger value passed (which is what nova-core's register macro bitfield
> implementation currently does anyway).
>
Yes. Assuming that I'm not completely lost here, you are proposing to
simply truncate to the size of the bitfield--no panics, no warnings. And
that's perfectly fine here IMHO.
thanks,
--
John Hubbard
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 5/5] rust: Add KUNIT tests for bitfield
2025-09-20 0:52 ` John Hubbard
@ 2025-09-20 9:33 ` Joel Fernandes
0 siblings, 0 replies; 19+ messages in thread
From: Joel Fernandes @ 2025-09-20 9:33 UTC (permalink / raw)
To: John Hubbard
Cc: Yury Norov, linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org, dakr@kernel.org,
Alexandre Courbot, Alistair Popple, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, bjorn3_gh@protonmail.com, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, David Airlie,
Simona Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, Timur Tabi, joel@joelfernandes.org,
Elle Rhumsaa, Daniel Almeida, nouveau@lists.freedesktop.org
Hello, John,
> On Sep 20, 2025, at 2:52 AM, John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 9/19/25 5:39 PM, Joel Fernandes wrote:
>> On Tue, Sep 16, 2025 at 05:59:18AM -0400, Joel Fernandes wrote:
>> [...]
>>>>> In C also this is valid. If you passed a higher value than what the
>>>>> bitfield can hold, the compiler will still just use the bits that it
>>>>> needs and ignore the rest.
>>>>
>>>> In C we've got FIELD_{PREP,GET,MODIFY}, implementing the checks.
>>>> So those who want to stay on safe side have a choice.
>>>
>>> Ah ok. We can add these checks then for the accessors, I will do so in v4.
>>
>> The C checks use BUILD_BUG_ON, in rust-for-linux we have build_assert but it
>> is fragile and depends on the value being a constant. Since the setter API
>> accepts a run-time value and not a constant, we cannot use this.
>>
>> Or, we can fail at runtime, but that requires changing the set_* to try_set_*
>> and returning a Result instead of Self. Alternatively, we can have a debug
>> option that panics if the setter API is misued.
>
> Please no...
True, it requires significant complication.
>
>>
>> Thoughts?
>>
>> Or for the moment, we can keep it simple and filter out / ignore extra bits
>> of the larger value passed (which is what nova-core's register macro bitfield
>> implementation currently does anyway).
>>
>
> Yes. Assuming that I'm not completely lost here, you are proposing to
> simply truncate to the size of the bitfield--no panics, no warnings. And
> that's perfectly fine here IMHO.
Ok, thanks. Yeah truncate. This is what register macro also currently does for its bitfields. To the point Jury is making though, the C equivalents do have build checks. We could do that once he have a better build_assert in rust but until then.. hopefully this suffices.
I am currently also research better ways of implementing build_assert.
- Joel
>
> thanks,
> --
> John Hubbard
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 5/5] rust: Add KUNIT tests for bitfield
2025-09-20 0:39 ` Joel Fernandes
2025-09-20 0:52 ` John Hubbard
@ 2025-09-21 11:41 ` Miguel Ojeda
1 sibling, 0 replies; 19+ messages in thread
From: Miguel Ojeda @ 2025-09-21 11:41 UTC (permalink / raw)
To: Joel Fernandes
Cc: Yury Norov, linux-kernel, 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 Sat, Sep 20, 2025 at 2:39 AM Joel Fernandes <joelagnelf@nvidia.com> wrote:
>
> The C checks use BUILD_BUG_ON, in rust-for-linux we have build_assert but it
> is fragile and depends on the value being a constant.
What do you mean?
`build_assert!` works essentially like `BUILD_BUG_ON`, i.e. after the
optimizer, and does not depend on the value being a constant.
You may be thinking of `static_assert!`, which is the compiler-based one.
Cheers,
Miguel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 5/5] rust: Add KUNIT tests for bitfield
2025-09-10 3:04 ` Yury Norov
2025-09-10 23:08 ` Joel Fernandes
@ 2025-09-10 23:22 ` Joel Fernandes
1 sibling, 0 replies; 19+ messages in thread
From: Joel Fernandes @ 2025-09-10 23:22 UTC (permalink / raw)
To: Yury Norov
Cc: linux-kernel, 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
Hi Jury,
Some reason I messed up with my email client while trimming, so I am re-sending.
Sorry :)
On 9/9/2025 11:04 PM, Yury Norov wrote:
[...]
>> + #[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();
>> + raw = (raw & !(0xF << 14)) | (0x7 << 14); // Set bits 17:14 to 7 (invalid for MemoryType)
>> + let invalid_pte = TestPageTableEntry::from(raw);
>> + assert_eq!(invalid_pte.extended_type(), Err(0x7)); // Should return Err with the invalid value
>
> Please make sure your lines don't exceed 100 chars, preferably less
> than 80.
Ok.
>> +
>> + // Test a valid value after testing invalid to ensure both cases work
>> + raw = (raw & !(0xF << 14)) | (0x2 << 14); // Set bits 17:14 to 2 (valid: Device)
>
> Can you use genmask!() here and everywhere else?
Ok.
>> + #[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);
>
> You've got only one negative test that covers the .from() method.
> Can you add more?
>
Sure, by negative you mean the test that returned an error code? If so, just to
note, we can only add negative tests if there is a chance of
runtime failure, which at runtime can mainly happen with the fallible usage (?=>
pattern). Also just to note, we already at ~300 lines of test code now :)
> What if I create a bitfield from a runtime value that exceeds
> the capacity?
>
> bitfield! {
> struct bf: u8 {
> 0:0 ready as bool;
> 1:1 error as bool;
> 3:2 state as u32;
Here you mean 'as u8', otherwise it wont compile.
> }
> }
>
> let raw_value: u8 = 0xff;
> let bf = bf::from(raw_value);
>
> I guess you'd return None or similar.
No, we would ignore the extra bits sent. There is a .raw() method and 'bf' is
8-bits, bf.raw() will return 0xff. So it is perfectly valid to do so. I don't
think we should return None here, this is also valid in C.
> Can you add such a test?
Sure, I added such a test.
> The same question for the setters. What would happen for this:
>
> let bf = bf::default()
> .set_state(0xf)
> .set_ready(true);
>
> I think that after the first out-of-boundary in set_state(), you
> should abort the call chain, make sure you're not touching memory
> in set_ready() and returning some type of error.
Here, on out of boundary, we just ignore the extra bits passed to set_state. I
think it would be odd if we errored out honestly. We are using 'as u8' in the
struct so we would accept any u8 as input, but then if we complained that extra
bits were sent, that would be odd. In C also this is valid. If you passed a
higher value than what the bitfield can hold, the compiler will still just use
the bits that it needs and ignore the rest.
Now, I am not opposed to error'ing out on that, but that's not what we currently
do and it is also not easy to do. The setters in the patch return Self, not
Result<Self>, so they are infallible, which is what allows them to be chained as
well (builder pattern).
I added another test here as well, to ensure the behavior is as I describe.
>
> And for this:
>
> let ret: u32 = -EINVAL;
> bf = bf::default();
> bf = bf.set_state(ret);
>
> For compile-time initializes, it should be a compile-time error, right?
Yes, since the struct in this example is u8, this wont compile. Yes, I will add
a comment.
> Can you drop a comment on that?
Yes, I will do so.
> (In C we've got FIELD_{GET,MODIFY,PREP}. They cover all the static
> cases.)
>
>> + 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);
>> + }
>> +}
>> --
>> 2.34.1
>
> I tried to apply your series on top of master, but it failed. So
> my apologies for not finding the answers to some questions above
> by myself.
Oh ok, I applied it on top of drm-rust-next. I will rebase on -next for the next
revision, thanks.
> For the next version, can you make sure your series is applicable
> on top of master or -next?
Sure, thanks,
- Joel
^ permalink raw reply [flat|nested] 19+ messages in thread