linux-kernel - Re: [PATCH v7.1 2/4] gpu: nova-core: bitfield: Move bitfield-specific code from register! into new macro

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aPEv_UO4vViOcOvN@yury>
Date: Thu, 16 Oct 2025 13:48:45 -0400
From: Yury Norov <yury.norov@...il.com>
To: Joel Fernandes <joelagnelf@...dia.com>
Cc: 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, Benno Lossin <lossin@...nel.org>,
	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>,
	Daniel Almeida <daniel.almeida@...labora.com>,
	nouveau@...ts.freedesktop.org, Edwin Peer <epeer@...dia.com>
Subject: Re: [PATCH v7.1 2/4] gpu: nova-core: bitfield: Move
 bitfield-specific code from register! into new macro

On Thu, Oct 16, 2025 at 11:13:21AM -0400, Joel Fernandes wrote:
> Move the bitfield-specific code from the register macro into a new macro
> called bitfield. This will be used to define structs with bitfields,
> similar to C language.

Can you please fix line length issues before v8?

$ awk '{print length}' drivers/gpu/nova-core/bitfield.rs | sort -rn | uniq -c
      1 118
      1 116
      1 113
      1 109
      1 105
      1 103
      ...

> Reviewed-by: Elle Rhumsaa <elle@...thered-steel.dev>
> Reviewed-by: Alexandre Courbot <acourbot@...dia.com>
> Reviewed-by: Edwin Peer <epeer@...dia.com>
> Signed-off-by: Joel Fernandes <joelagnelf@...dia.com>
> ---
>  drivers/gpu/nova-core/bitfield.rs    | 319 +++++++++++++++++++++++++++
>  drivers/gpu/nova-core/nova_core.rs   |   3 +
>  drivers/gpu/nova-core/regs/macros.rs | 259 +---------------------
>  3 files changed, 332 insertions(+), 249 deletions(-)
>  create mode 100644 drivers/gpu/nova-core/bitfield.rs

...

> +///
> +/// bitfield! {
> +///     struct ControlReg {
> +///         3:0 mode as u8 ?=> Mode;
> +///         7:7 state as bool => State;
> +///     }
> +/// }

This notation is really unwelcome this days. It may be OK for a random
macro in some local driver, but doesn't really work for a global basic
data type:

https://lore.kernel.org/all/CAHk-=whoOUsqPKb7OQwhQf9H_3=5sXGPJrDbfQfwLB3Bi13tcQ@mail.gmail.com/

I've already shared this link with you, and shared my concern.

I realize that rust/bitfield derives the GENMASK(hi, lo) notation here,
and GENMASK() derives verilog or hardware specs popular notations. But
software people prefer lo:hi. I'm probably OK if you choose C-style
start:nbits, if you prefer. But let's stop this hi:lo early, please.

Let me quote Linus from the link above:

  It does "high, low", which is often very unintuitive, and in fact the
  very commit that introduced this thing from hell had to convert the
  sane "low,high" cases to the other way around.

  See commit 10ef6b0dffe4 ("bitops: Introduce a more generic BITMASK
  macro"), and notice how ALMOST ALL use cases were switched around to
  the illogical "high,low" format by that introductory phase.

  And yes, I understand why that person did it: many datasheets show
  bits in a register graphically, and then you see that "high .. low"
  thing in a rectangle that describes the register, and that ordering
  them makes 100% sense IN THAT CONTEXT.

  But it damn well does not make sense in most other contexts.

  In fact, even in the context of generating mask #defines, it actually
  reads oddly, because you end up having things like

    /* Status register (SR) */
    #define I2C_SR_OP               GENMASK(1, 0)   /* Operation */
    #define I2C_SR_STATUS           GENMASK(3, 2)   /* controller status */
    #define I2C_SR_CAUSE            GENMASK(6, 4)   /* Abort cause */
    #define I2C_SR_TYPE             GENMASK(8, 7)   /* Receive type */
    #define I2C_SR_LENGTH           GENMASK(19, 9)  /* Transfer length */

  ...

Now compare it to what we've got in nova right now:

  register!(NV_PMC_BOOT_0 @ 0x00000000, "Basic revision information about the GPU" {
      3:0     minor_revision as u8, "Minor revision of the chip";
      7:4     major_revision as u8, "Major revision of the chip";
      8:8     architecture_1 as u8, "MSB of the architecture";
      23:20   implementation as u8, "Implementation version of the architecture";
      28:24   architecture_0 as u8, "Lower bits of the architecture";
  });

There's so far 36 thousands of GENMASK()s in the kernel, and only 67
register!()s. It's a separate topic what to do with the GENMASK()
codebase. But now that you do this massive refactoring for the
register!() macro, let's convert those 67 users to the lo:hi notation.

As a side note, for GENMASKs, I tried this trick:

        #define GENMASK(a, b) UNSAFE_GENMASK(MIN(a, b), MAX(a, b))

It works, but bloats defconfig kernel for another 1K. I don't think it
would add to readability on both C and rust sides.

Thanks,
Yury