Hi Alex, On 8/25/2025 7:07 AM, Alexandre Courbot wrote: > Hi Joel, > > On Sun Aug 24, 2025 at 10:59 PM JST, Joel Fernandes wrote: >> Add a minimal bitfield library for defining in Rust structures (called >> bitstruct), similar in concept to bit fields in C structs. This will be used >> for defining page table entries and other structures in nova-core. > > This is basically a rewrite (with some improvements, and some missing > features) of the part of the `register!` macro that deals with > bitfields. We planned to extract it into its own macro, and the split is > already effective in its internal rules, so I'd suggest to just move the > relevant rules into a new macro (as it will give you a couple useful > features, like automatic conversion to enum types), and then apply your > improvements on top of it. Otherwise we will end up with two > implementations of the same thing, for no good justification IMHO. > > We were also planning to move the `register!` macro into the kernel > crate this cycle so Tyr can use it, but this changes the plan a bit. > Actually it is helpful, since your version implements one thing that we > needed in the `register!` macro before moving it: visibility specifiers. > So I would do things in this order: > > 1. Extract the bitfield-related code from the `register!` macro into its > own macro (in nova-core), and make `register!` call into it, > 2. Add support for visibility specifiers and non-u32 types in your macro and > `register!`, > 3. Move both macros to the kernel crate (hopefully in time for the next > merge window so Tyr can make use of them). > > A few more comments inline. >
Ok, all these sound good to me. >> >> Signed-off-by: Joel Fernandes <joelagn...@nvidia.com> >> --- >> drivers/gpu/nova-core/bitstruct.rs | 149 +++++++++++++++++++++++++++++ >> drivers/gpu/nova-core/nova_core.rs | 1 + >> 2 files changed, 150 insertions(+) >> create mode 100644 drivers/gpu/nova-core/bitstruct.rs >> >> diff --git a/drivers/gpu/nova-core/bitstruct.rs >> b/drivers/gpu/nova-core/bitstruct.rs >> new file mode 100644 >> index 000000000000..661a75da0a9c >> --- /dev/null >> +++ b/drivers/gpu/nova-core/bitstruct.rs > > I wonder whether this should go under the existing `bits.rs`, or be its > own module? I'd say its own since it is related to structures and keeps the file smaller. >> @@ -0,0 +1,149 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// >> +// bitstruct.rs — C-style library for bitfield-packed Rust structures >> +// >> +// A library that provides support for defining bit fields in Rust >> +// structures to circumvent lack of native language support for this. >> +// >> +// Similar usage syntax to the register! macro. > > Eventually the `register!` macro is the one that should reference this > (simpler) one, so let's make it the reference. :) Ah true, already fixed. > >> + >> +use kernel::prelude::*; >> + >> +/// Macro for defining bitfield-packed structures in Rust. >> +/// The size of the underlying storage type is specified with #[repr(TYPE)]. >> +/// >> +/// # Example (just for illustration) >> +/// ```rust >> +/// bitstruct! { >> +/// #[repr(u64)] >> +/// pub struct PageTableEntry { >> +/// 0:0 present as bool, >> +/// 1:1 writable as bool, >> +/// 11:9 available as u8, >> +/// 51:12 pfn as u64, >> +/// 62:52 available2 as u16, >> +/// 63:63 nx as bool, > > A note on syntax: for nova-core, we may want to use the `H:L` notation, > as this is what OpenRM uses, but in the larger kernel we might want to > use inclusive ranges (`L..=H`) as it will look more natural in Rust > code (and is the notation the `bits` module already uses). Perhaps future add-on enhancement to have both syntax? I'd like to initially keep H:L and stabilize the code first, what do you think? > >> +/// } >> +/// } >> +/// ``` >> +/// >> +/// This generates a struct with methods: >> +/// - Constructor: `default()` sets all bits to zero. >> +/// - Field accessors: `present()`, `pfn()`, etc. >> +/// - Field setters: `set_present()`, `set_pfn()`, etc. >> +/// - Builder methods: `with_present()`, `with_pfn()`, etc. >> +/// - Raw conversion: `from_raw()`, `into_raw()` >> +#[allow(unused_macros)] >> +macro_rules! bitstruct { >> + ( >> + #[repr($storage:ty)] >> + $vis:vis struct $name:ident { >> + $( >> + $hi:literal : $lo:literal $field:ident as $field_type:tt >> + ),* $(,)? >> + } >> + ) => { >> + #[repr(transparent)] >> + #[derive(Copy, Clone, Default)] >> + $vis struct $name($storage); >> + >> + impl $name { >> + /// Create from raw value >> + #[inline(always)] >> + $vis const fn from_raw(val: $storage) -> Self { >> + Self(val) >> + } >> + >> + /// Get raw value >> + #[inline(always)] >> + $vis const fn into_raw(self) -> $storage { >> + self.0 >> + } >> + } >> + >> + impl core::fmt::Debug for $name { >> + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> >> core::fmt::Result { >> + write!(f, "{}({:#x})", stringify!($name), self.0) >> + } >> + } >> + >> + // Generate all field methods >> + $( >> + bitstruct_field_impl!($vis, $name, $storage, $hi, $lo, $field >> as $field_type); > > Maybe use internal rules [1] instead of a private macro (that cannot be so > private :)) > > [1] https://lukaswirth.dev/tlborm/decl-macros/patterns/internal-rules.html Sure, done. > >> + )* >> + }; >> +} >> + >> +/// Helper to calculate mask for bit fields >> +#[allow(unused_macros)] >> +macro_rules! bitstruct_mask { >> + ($hi:literal, $lo:literal, $storage:ty) => {{ >> + let width = ($hi - $lo + 1) as usize; >> + let storage_bits = 8 * core::mem::size_of::<$storage>(); >> + if width >= storage_bits { >> + <$storage>::MAX >> + } else { >> + ((1 as $storage) << width) - 1 >> + } >> + }}; >> +} > > Is there a way to leverage the `genmask_*` functions of the `bits` module? > Maybe, I'll look into it, thanks. >> +#[allow(unused_macros)] >> +macro_rules! bitstruct_field_impl { >> + ($vis:vis, $struct_name:ident, $storage:ty, $hi:literal, $lo:literal, >> $field:ident as $field_type:tt) => { >> + impl $struct_name { >> + #[inline(always)] >> + $vis const fn $field(&self) -> $field_type { >> + let field_val = (self.0 >> $lo) & bitstruct_mask!($hi, $lo, >> $storage); >> + bitstruct_cast_value!(field_val, $field_type) >> + } >> + } >> + bitstruct_make_setters!($vis, $struct_name, $storage, $hi, $lo, >> $field, $field_type); >> + }; >> +} >> + >> +/// Helper macro to convert extracted value to target type >> +/// >> +/// Special handling for bool types is required because the `as` keyword >> +/// cannot be used to convert to bool in Rust. For bool fields, we check >> +/// if the extracted value is non-zero. For all other types, we use the >> +/// standard `as` conversion. >> +#[allow(unused_macros)] >> +macro_rules! bitstruct_cast_value { >> + ($field_val:expr, bool) => { >> + $field_val != 0 >> + }; >> + ($field_val:expr, $field_type:tt) => { >> + $field_val as $field_type >> + }; >> +} >> + >> +#[allow(unused_macros)] >> +macro_rules! bitstruct_write_bits { >> + ($raw:expr, $hi:literal, $lo:literal, $val:expr, $storage:ty) => {{ >> + let mask = bitstruct_mask!($hi, $lo, $storage); >> + ($raw & !(mask << $lo)) | ((($val as $storage) & mask) << $lo) >> + }}; >> +} >> + >> +#[allow(unused_macros)] >> +macro_rules! bitstruct_make_setters { >> + ($vis:vis, $struct_name:ident, $storage:ty, $hi:literal, $lo:literal, >> $field:ident, $field_type:tt) => { >> + ::kernel::macros::paste! { >> + impl $struct_name { >> + #[inline(always)] >> + #[allow(dead_code)] >> + $vis fn [<set_ $field>](&mut self, val: $field_type) { >> + self.0 = bitstruct_write_bits!(self.0, $hi, $lo, val, >> $storage); >> + } >> + >> + #[inline(always)] >> + #[allow(dead_code)] >> + $vis const fn [<with_ $field>](mut self, val: $field_type) >> -> Self { >> + self.0 = bitstruct_write_bits!(self.0, $hi, $lo, val, >> $storage); >> + self >> + } >> + } >> + } >> + }; >> +} > > Yep, I think you definitely want to put these into internal rules - see > how `register!` does it, actually it should be easy to extract these > rules only and implement your improvements on top. Ack, done. - Joel