Re: [PATCH v2 2/4] nova-core: bitstruct: Add support for different storage widths

[Joel Fernandes <joelagnelf@nvidia.com>]

On Mon, Sep 08, 2025 at 12:26:43PM +0900, Alexandre Courbot wrote:
> On Thu Sep 4, 2025 at 6:54 AM JST, Joel Fernandes wrote:
> > Previously, bitstructs were hardcoded to use u32 as the underlying
> > storage type.  Add support for different storage types (u8, u16, u32,
> > u64) to the bitstruct macro.
> >
> > New syntax is: struct Name: <type ex., u32> { ... }
> >
> > Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> <snip>
> >      // 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,
> > +                _ => <$storage>::MAX
> 
> Since this is a const expression, you can use `build_error!` to make
> compilation fail in the unlikely event the `_` is taken due to bug in
> the code. 

Makes sense, changed.

> 
> > +            }
> > +        };
> >          const [<$field:upper _SHIFT>]: u32 = Self::[<$field:upper _MASK>].trailing_zeros();
> >          );
> >  
> > @@ -211,7 +220,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);
> > @@ -226,9 +235,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
> > @@ -237,7 +246,7 @@ pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
> >      };
> >  
> >      // Generates the `Debug` implementation for `$name`.
> > -    (@debug $name:ident { $($field:ident;)* }) => {
> > +    (@debug $name:ident $storage:ty { $($field:ident;)* }) => {
> 
> This rule doesn't make use of the `$storage` argument.

Removed unused arg, thanks.

> >          impl ::core::fmt::Debug for $name {
> >              fn fmt(&self, f: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result {
> >                  f.debug_struct(stringify!($name))
> > @@ -251,7 +260,7 @@ fn fmt(&self, f: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result {
> >      };
> >  
> >      // Generates the `Default` implementation for `$name`.
> > -    (@default $name:ident { $($field:ident;)* }) => {
> > +    (@default $name:ident $storage:ty { $($field:ident;)* }) => {
> 
> Neither does this one.

Removed unused arg, thanks.

thanks,

 - Joel