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

Joel Fernandes
2025-09-09T18:26:10Z

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