Greg KH
2025-Sep-21 12:45 UTC
[PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
On Sun, Sep 21, 2025 at 02:33:56PM +0200, Benno Lossin wrote:> On Sun Sep 21, 2025 at 11:36 AM CEST, Greg KH wrote: > > On Sat, Sep 20, 2025 at 02:22:27PM -0400, Joel Fernandes wrote: > >> The bitfield-specific into new macro. This will be used to define > >> structs with bitfields, similar to C language. > >> > >> Reviewed-by: Elle Rhumsaa <elle at weathered-steel.dev> > >> Signed-off-by: Joel Fernandes <joelagnelf at nvidia.com> > >> --- > >> drivers/gpu/nova-core/bitfield.rs | 314 +++++++++++++++++++++++++++ > >> drivers/gpu/nova-core/nova_core.rs | 3 + > >> drivers/gpu/nova-core/regs/macros.rs | 259 +--------------------- > >> 3 files changed, 327 insertions(+), 249 deletions(-) > >> create mode 100644 drivers/gpu/nova-core/bitfield.rs > >> > >> diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs > >> new file mode 100644 > >> index 000000000000..ba6b7caa05d9 > >> --- /dev/null > >> +++ b/drivers/gpu/nova-core/bitfield.rs > >> @@ -0,0 +1,314 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > >> + > >> +//! Bitfield library for Rust structures > >> +//! > >> +//! Support for defining bitfields in Rust structures. Also used by the [`register!`] macro. > >> +//! > >> +//! # Syntax > >> +//! > >> +//! ```rust > >> +//! #[derive(Debug, Clone, Copy)] > >> +//! enum Mode { > >> +//! Low = 0, > >> +//! High = 1, > >> +//! Auto = 2, > >> +//! } > >> +//! > >> +//! impl TryFrom<u8> for Mode { > >> +//! type Error = u8; > >> +//! fn try_from(value: u8) -> Result<Self, Self::Error> { > >> +//! match value { > >> +//! 0 => Ok(Mode::Low), > >> +//! 1 => Ok(Mode::High), > >> +//! 2 => Ok(Mode::Auto), > >> +//! _ => Err(value), > >> +//! } > >> +//! } > >> +//! } > >> +//! > >> +//! impl From<Mode> for u32 { > >> +//! fn from(mode: Mode) -> u32 { > >> +//! mode as u32 > >> +//! } > >> +//! } > >> +//! > >> +//! #[derive(Debug, Clone, Copy)] > >> +//! enum State { > >> +//! Inactive = 0, > >> +//! Active = 1, > >> +//! } > >> +//! > >> +//! impl From<bool> for State { > >> +//! fn from(value: bool) -> Self { > >> +//! if value { State::Active } else { State::Inactive } > >> +//! } > >> +//! } > >> +//! > >> +//! impl From<State> for u32 { > >> +//! fn from(state: State) -> u32 { > >> +//! state as u32 > >> +//! } > >> +//! } > >> +//! > >> +//! bitfield! { > >> +//! struct ControlReg { > >> +//! 3:0 mode as u8 ?=> Mode; > >> +//! 7 state as bool => State; > >> +//! } > >> +//! } > > > > As discussed at the conference this week, I do object to this as it > > will allow the same mistakes to happen that we used to do in the kernel > > for a long time before the regmap() api happened, along with GENMASK(). > > Have you read the following macro arm of the implementation? > > // Generates the accessor methods for a single field. > ( > @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident > { $process:expr } $to_type:ty => $res_type:ty $(, $comment:literal)?; > ) => { > ::kernel::macros::paste!( > const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi; > const [<$field:upper _MASK>]: u32 = ((((1 << $hi) - 1) << 1) + 1) - ((1 << $lo) - 1); > const [<$field:upper _SHIFT>]: u32 = Self::[<$field:upper _MASK>].trailing_zeros(); > ); > > $( > #[doc="Returns the value of this field:"] > #[doc=$comment] > )? > #[inline(always)] > pub(crate) fn $field(self) -> $res_type { > ::kernel::macros::paste!( > const MASK: u32 = $name::[<$field:upper _MASK>]; > const SHIFT: u32 = $name::[<$field:upper _SHIFT>]; > ); > let field = ((self.0 & MASK) >> SHIFT); > > Here you can see that it's just a mask + shift operation internally to > access the field. > > $process(field) > } > > ::kernel::macros::paste!( > $( > #[doc="Sets the value of this field:"] > #[doc=$comment] > )? > #[inline(always)] > pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self { > const MASK: u32 = $name::[<$field:upper _MASK>]; > const SHIFT: u32 = $name::[<$field:upper _SHIFT>]; > let value = (u32::from(value) << SHIFT) & MASK; > self.0 = (self.0 & !MASK) | value; > > self > } > ); > };Yes, that's great, but that is all done in "native cpu" endian, and might not actually represent what the hardware does at all, which is what I was trying to get at here, sorry for not being more specific.> Now I too would like to see how exactly this will be used to read data > from hardware. But at least in theory if the conversion from hardware > endianness to native endianness is done correctly, this will do the > right thing :)That's great, so we are close, but it's not quite correct. How about something like: 0:7 reg_X as __le32 8:15 reg_y as __le32 and the like. There has to be a way to actually specify the representation here and for C, that is using the "__" types that express the endianness (i.e. __le32, __be32, and so on). Then we have type checks that enforce accesses to those variables to always turn the data into "native" values when the cpu accesses them. Again, regmap handles this all just fine, why not just make bindings to that api here instead? thanks, greg k-h
Danilo Krummrich
2025-Sep-21 13:47 UTC
[PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
On Sun Sep 21, 2025 at 2:45 PM CEST, Greg KH wrote:> Again, regmap handles this all just fine, why not just make bindings to > that api here instead?The idea is to use this for the register!() macro, e.g. register!(NV_PMC_BOOT_0 @ 0x00000000, "Basic revision information about the GPU" { 28:24 architecture_0 as u8, "Lower bits of the architecture"; 23:20 implementation as u8, "Implementation version of the architecture"; 8:8 architecture_1 as u8, "MSB of the architecture"; 7:4 major_revision as u8, "Major revision of the chip"; 3:0 minor_revision as u8, "Minor revision of the chip"; }); (More examples in [1].) This generates a structure with the relevant accessors; we can also implement additional logic, such as: impl NV_PMC_BOOT_0 { /// Combines `architecture_0` and `architecture_1` to obtain the architecture of the chip. pub(crate) fn architecture(self) -> Result<Architecture> { Architecture::try_from( self.architecture_0() | (self.architecture_1() << Self::ARCHITECTURE_0_RANGE.len()), ) } /// Combines `architecture` and `implementation` to obtain a code unique to the chipset. pub(crate) fn chipset(self) -> Result<Chipset> { self.architecture() .map(|arch| { ((arch as u32) << Self::IMPLEMENTATION_RANGE.len()) | u32::from(self.implementation()) }) .and_then(Chipset::try_from) } } This conviniently allows us to read the register with let boot0 = regs::NV_PMC_BOOT_0::read(bar); and obtain an instance of the entire Chipset structure with let chipset = boot0.chipset()?; or pass it to a constructor that creates a Revision instance let rev = Revision::from_boot0(boot0); Analogously it allows us to modify and write registers without having to mess with error prone shifts, masks and casts, because that code is generated by the register!() macro. (Of course, unless we have more complicated cases where multiple fields have to be combined as illustrated above.) Note that bar is of type pci::Bar<BAR0_SIZE> where BAR0_SIZE in our case is SZ_16M. However, the type required by read() as generated by the register!() macro actually only requires something that implements an I/O backend, i.e kernel::io::Io<SIZE>. pci::Bar is a specific implementation of kernel::io::Io. With this we can let the actual I/O backend handle the endianness of the bus. (Actually, we could even implement an I/O backend that uses regmap.) So, I think the register!() stuff is rather orthogonal. - Danilo [1] https://gitlab.freedesktop.org/drm/rust/kernel/-/blob/drm-rust-next/drivers/gpu/nova-core/regs.rs
Joel Fernandes
2025-Sep-23 22:24 UTC
[PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
Hi Greg, On Sun, Sep 21, 2025 at 02:45:27PM +0200, Greg KH wrote:> On Sun, Sep 21, 2025 at 02:33:56PM +0200, Benno Lossin wrote: > > On Sun Sep 21, 2025 at 11:36 AM CEST, Greg KH wrote: > > > On Sat, Sep 20, 2025 at 02:22:27PM -0400, Joel Fernandes wrote: > > >> The bitfield-specific into new macro. This will be used to define > > >> structs with bitfields, similar to C language. > > >> > > >> Reviewed-by: Elle Rhumsaa <elle at weathered-steel.dev> > > >> Signed-off-by: Joel Fernandes <joelagnelf at nvidia.com> > > >> --- > > >> drivers/gpu/nova-core/bitfield.rs | 314 +++++++++++++++++++++++++++ > > >> drivers/gpu/nova-core/nova_core.rs | 3 + > > >> drivers/gpu/nova-core/regs/macros.rs | 259 +--------------------- > > >> 3 files changed, 327 insertions(+), 249 deletions(-) > > >> create mode 100644 drivers/gpu/nova-core/bitfield.rs > > >> > > >> diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs > > >> new file mode 100644 > > >> index 000000000000..ba6b7caa05d9 > > >> --- /dev/null > > >> +++ b/drivers/gpu/nova-core/bitfield.rs > > >> @@ -0,0 +1,314 @@ > > >> +// SPDX-License-Identifier: GPL-2.0 > > >> + > > >> +//! Bitfield library for Rust structures > > >> +//! > > >> +//! Support for defining bitfields in Rust structures. Also used by the [`register!`] macro. > > >> +//! > > >> +//! # Syntax > > >> +//! > > >> +//! ```rust > > >> +//! #[derive(Debug, Clone, Copy)] > > >> +//! enum Mode { > > >> +//! Low = 0, > > >> +//! High = 1, > > >> +//! Auto = 2, > > >> +//! } > > >> +//! > > >> +//! impl TryFrom<u8> for Mode { > > >> +//! type Error = u8; > > >> +//! fn try_from(value: u8) -> Result<Self, Self::Error> { > > >> +//! match value { > > >> +//! 0 => Ok(Mode::Low), > > >> +//! 1 => Ok(Mode::High), > > >> +//! 2 => Ok(Mode::Auto), > > >> +//! _ => Err(value), > > >> +//! } > > >> +//! } > > >> +//! } > > >> +//! > > >> +//! impl From<Mode> for u32 { > > >> +//! fn from(mode: Mode) -> u32 { > > >> +//! mode as u32 > > >> +//! } > > >> +//! } > > >> +//! > > >> +//! #[derive(Debug, Clone, Copy)] > > >> +//! enum State { > > >> +//! Inactive = 0, > > >> +//! Active = 1, > > >> +//! } > > >> +//! > > >> +//! impl From<bool> for State { > > >> +//! fn from(value: bool) -> Self { > > >> +//! if value { State::Active } else { State::Inactive } > > >> +//! } > > >> +//! } > > >> +//! > > >> +//! impl From<State> for u32 { > > >> +//! fn from(state: State) -> u32 { > > >> +//! state as u32 > > >> +//! } > > >> +//! } > > >> +//! > > >> +//! bitfield! { > > >> +//! struct ControlReg { > > >> +//! 3:0 mode as u8 ?=> Mode; > > >> +//! 7 state as bool => State; > > >> +//! } > > >> +//! } > > > > > > As discussed at the conference this week, I do object to this as it > > > will allow the same mistakes to happen that we used to do in the kernel > > > for a long time before the regmap() api happened, along with GENMASK(). > > > > Have you read the following macro arm of the implementation? > > > > // Generates the accessor methods for a single field. > > ( > > @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident > > { $process:expr } $to_type:ty => $res_type:ty $(, $comment:literal)?; > > ) => { > > ::kernel::macros::paste!( > > const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi; > > const [<$field:upper _MASK>]: u32 = ((((1 << $hi) - 1) << 1) + 1) - ((1 << $lo) - 1); > > const [<$field:upper _SHIFT>]: u32 = Self::[<$field:upper _MASK>].trailing_zeros(); > > ); > > > > $( > > #[doc="Returns the value of this field:"] > > #[doc=$comment] > > )? > > #[inline(always)] > > pub(crate) fn $field(self) -> $res_type { > > ::kernel::macros::paste!( > > const MASK: u32 = $name::[<$field:upper _MASK>]; > > const SHIFT: u32 = $name::[<$field:upper _SHIFT>]; > > ); > > let field = ((self.0 & MASK) >> SHIFT); > > > > Here you can see that it's just a mask + shift operation internally to > > access the field. > > > > $process(field) > > } > > > > ::kernel::macros::paste!( > > $( > > #[doc="Sets the value of this field:"] > > #[doc=$comment] > > )? > > #[inline(always)] > > pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self { > > const MASK: u32 = $name::[<$field:upper _MASK>]; > > const SHIFT: u32 = $name::[<$field:upper _SHIFT>]; > > let value = (u32::from(value) << SHIFT) & MASK; > > self.0 = (self.0 & !MASK) | value; > > > > self > > } > > ); > > }; > > Yes, that's great, but that is all done in "native cpu" endian, and > might not actually represent what the hardware does at all, which is > what I was trying to get at here, sorry for not being more specific. > > > Now I too would like to see how exactly this will be used to read data > > from hardware. But at least in theory if the conversion from hardware > > endianness to native endianness is done correctly, this will do the > > right thing :) > > That's great, so we are close, but it's not quite correct. How about > something like: > > 0:7 reg_X as __le32 > 8:15 reg_y as __le32I don't think we should force endianness requirements within specific fields in the bitfield rust library itself, it is upto the user. bitfields are not only for registers even in C. If you see on the C side, we have rcu_special union which uses 'u32' and does not enforce endianness within the fields or bytes of the struct with respect to the fields. Its all native CPU endian and works fine. You're basically saying in terms of C that, the designers of the C bitfield in C standard force the C language to use endianness in the types, no they can't / shouldn't be forced to. For the separate issue of enforcing endianness with respect to (across) multiple fields, I agree with you that if the user's backend (the consumer of the data) is not doing such conversion, say via regmap, then that becomes a problem. But that problem is orthogonal/different and cannot be solved here.> and the like. There has to be a way to actually specify the > representation here and for C, that is using the "__" types that express > the endianness (i.e. __le32, __be32, and so on). Then we have type > checks that enforce accesses to those variables to always turn the data > into "native" values when the cpu accesses them.Sure, it is upto the user, they can decide to use whatever types they want. In fact we also support conversion to enums, not just integer types :) thanks, - Joel
Greg KH
2025-Sep-24 10:40 UTC
[PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro
On Tue, Sep 23, 2025 at 06:24:34PM -0400, Joel Fernandes wrote:> Hi Greg, > > On Sun, Sep 21, 2025 at 02:45:27PM +0200, Greg KH wrote: > > On Sun, Sep 21, 2025 at 02:33:56PM +0200, Benno Lossin wrote: > > > On Sun Sep 21, 2025 at 11:36 AM CEST, Greg KH wrote: > > > > On Sat, Sep 20, 2025 at 02:22:27PM -0400, Joel Fernandes wrote: > > > >> The bitfield-specific into new macro. This will be used to define > > > >> structs with bitfields, similar to C language. > > > >> > > > >> Reviewed-by: Elle Rhumsaa <elle at weathered-steel.dev> > > > >> Signed-off-by: Joel Fernandes <joelagnelf at nvidia.com> > > > >> --- > > > >> drivers/gpu/nova-core/bitfield.rs | 314 +++++++++++++++++++++++++++ > > > >> drivers/gpu/nova-core/nova_core.rs | 3 + > > > >> drivers/gpu/nova-core/regs/macros.rs | 259 +--------------------- > > > >> 3 files changed, 327 insertions(+), 249 deletions(-) > > > >> create mode 100644 drivers/gpu/nova-core/bitfield.rs > > > >> > > > >> diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs > > > >> new file mode 100644 > > > >> index 000000000000..ba6b7caa05d9 > > > >> --- /dev/null > > > >> +++ b/drivers/gpu/nova-core/bitfield.rs > > > >> @@ -0,0 +1,314 @@ > > > >> +// SPDX-License-Identifier: GPL-2.0 > > > >> + > > > >> +//! Bitfield library for Rust structures > > > >> +//! > > > >> +//! Support for defining bitfields in Rust structures. Also used by the [`register!`] macro. > > > >> +//! > > > >> +//! # Syntax > > > >> +//! > > > >> +//! ```rust > > > >> +//! #[derive(Debug, Clone, Copy)] > > > >> +//! enum Mode { > > > >> +//! Low = 0, > > > >> +//! High = 1, > > > >> +//! Auto = 2, > > > >> +//! } > > > >> +//! > > > >> +//! impl TryFrom<u8> for Mode { > > > >> +//! type Error = u8; > > > >> +//! fn try_from(value: u8) -> Result<Self, Self::Error> { > > > >> +//! match value { > > > >> +//! 0 => Ok(Mode::Low), > > > >> +//! 1 => Ok(Mode::High), > > > >> +//! 2 => Ok(Mode::Auto), > > > >> +//! _ => Err(value), > > > >> +//! } > > > >> +//! } > > > >> +//! } > > > >> +//! > > > >> +//! impl From<Mode> for u32 { > > > >> +//! fn from(mode: Mode) -> u32 { > > > >> +//! mode as u32 > > > >> +//! } > > > >> +//! } > > > >> +//! > > > >> +//! #[derive(Debug, Clone, Copy)] > > > >> +//! enum State { > > > >> +//! Inactive = 0, > > > >> +//! Active = 1, > > > >> +//! } > > > >> +//! > > > >> +//! impl From<bool> for State { > > > >> +//! fn from(value: bool) -> Self { > > > >> +//! if value { State::Active } else { State::Inactive } > > > >> +//! } > > > >> +//! } > > > >> +//! > > > >> +//! impl From<State> for u32 { > > > >> +//! fn from(state: State) -> u32 { > > > >> +//! state as u32 > > > >> +//! } > > > >> +//! } > > > >> +//! > > > >> +//! bitfield! { > > > >> +//! struct ControlReg { > > > >> +//! 3:0 mode as u8 ?=> Mode; > > > >> +//! 7 state as bool => State; > > > >> +//! } > > > >> +//! } > > > > > > > > As discussed at the conference this week, I do object to this as it > > > > will allow the same mistakes to happen that we used to do in the kernel > > > > for a long time before the regmap() api happened, along with GENMASK(). > > > > > > Have you read the following macro arm of the implementation? > > > > > > // Generates the accessor methods for a single field. > > > ( > > > @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident > > > { $process:expr } $to_type:ty => $res_type:ty $(, $comment:literal)?; > > > ) => { > > > ::kernel::macros::paste!( > > > const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi; > > > const [<$field:upper _MASK>]: u32 = ((((1 << $hi) - 1) << 1) + 1) - ((1 << $lo) - 1); > > > const [<$field:upper _SHIFT>]: u32 = Self::[<$field:upper _MASK>].trailing_zeros(); > > > ); > > > > > > $( > > > #[doc="Returns the value of this field:"] > > > #[doc=$comment] > > > )? > > > #[inline(always)] > > > pub(crate) fn $field(self) -> $res_type { > > > ::kernel::macros::paste!( > > > const MASK: u32 = $name::[<$field:upper _MASK>]; > > > const SHIFT: u32 = $name::[<$field:upper _SHIFT>]; > > > ); > > > let field = ((self.0 & MASK) >> SHIFT); > > > > > > Here you can see that it's just a mask + shift operation internally to > > > access the field. > > > > > > $process(field) > > > } > > > > > > ::kernel::macros::paste!( > > > $( > > > #[doc="Sets the value of this field:"] > > > #[doc=$comment] > > > )? > > > #[inline(always)] > > > pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self { > > > const MASK: u32 = $name::[<$field:upper _MASK>]; > > > const SHIFT: u32 = $name::[<$field:upper _SHIFT>]; > > > let value = (u32::from(value) << SHIFT) & MASK; > > > self.0 = (self.0 & !MASK) | value; > > > > > > self > > > } > > > ); > > > }; > > > > Yes, that's great, but that is all done in "native cpu" endian, and > > might not actually represent what the hardware does at all, which is > > what I was trying to get at here, sorry for not being more specific. > > > > > Now I too would like to see how exactly this will be used to read data > > > from hardware. But at least in theory if the conversion from hardware > > > endianness to native endianness is done correctly, this will do the > > > right thing :) > > > > That's great, so we are close, but it's not quite correct. How about > > something like: > > > > 0:7 reg_X as __le32 > > 8:15 reg_y as __le32 > > I don't think we should force endianness requirements within specific fields in > the bitfield rust library itself, it is upto the user. bitfields are not only > for registers even in C. If you see on the C side, we have rcu_special union > which uses 'u32' and does not enforce endianness within the fields or bytes > of the struct with respect to the fields. Its all native CPU endian and works > fine. You're basically saying in terms of C that, the designers of the C > bitfield in C standard force the C language to use endianness in the types, no > they can't / shouldn't be forced to.For "cpu native" structures, just use the bit and genmask macros we have today, on top of a normal variable type and you should be fine. The only place you need/want to do stuff like what is being proposed here is when you are trying to match up a data structure that is in hardware to be able to split the values out of it safely. And when dealing with data that goes outside of the kernel (i.e. to/from hardware), you HAVE to specify the endianness of that data as the hardware is the one that defines this, NOT the cpu that the kernel is running on. So you should NEVER see a bitfield structure that is defined just as a "u32" and expect that to work properly when read/written to hardware because while little-endian is what seems to have "won" the recent battles on this topic it's not always the case for many places that Linux runs on today.> For the separate issue of enforcing endianness with respect to (across) > multiple fields, I agree with you that if the user's backend (the consumer of > the data) is not doing such conversion, say via regmap, then that becomes a > problem. But that problem is orthogonal/different and cannot be solved here.But that is exactly what these macros are being defined here for, so to ignore that is going to cause problems :) thanks, greg k-h