| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2025092125-urban-muppet-1c2f@gregkh>
Date: Sun, 21 Sep 2025 14:45:27 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Benno Lossin <lossin@...nel.org>
Cc: Joel Fernandes <joelagnelf@...dia.com>, linux-kernel@...r.kernel.org,
rust-for-linux@...r.kernel.org, dri-devel@...ts.freedesktop.org,
dakr@...nel.org, acourbot@...dia.com,
Alistair Popple <apopple@...dia.com>,
Miguel Ojeda <ojeda@...nel.org>,
Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
bjorn3_gh@...tonmail.com, Andreas Hindborg <a.hindborg@...nel.org>,
Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
John Hubbard <jhubbard@...dia.com>, Timur Tabi <ttabi@...dia.com>,
joel@...lfernandes.org, Elle Rhumsaa <elle@...thered-steel.dev>,
Yury Norov <yury.norov@...il.com>,
Daniel Almeida <daniel.almeida@...labora.com>,
nouveau@...ts.freedesktop.org
Subject: Re: [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@...thered-steel.dev>
> >> Signed-off-by: Joel Fernandes <joelagnelf@...dia.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
Powered by blists - more mailing lists