[PATCH v5 0/5] Initial Nova Core series

[PATCH v5 0/5] Initial Nova Core series

[Danilo Krummrich]

This is the initial series for the nova-core stub driver.

nova-core is intended to serve as a common base for nova-drm (the
corresponding DRM driver) and the vGPU manager VFIO driver, serving as a
hard- and firmware abstraction layer for GSP-based NVIDIA GPUs.

The Nova project, including nova-core and nova-drm, in the long term,
is intended to serve as the successor of Nouveau for all GSP-based GPUs.

The motivation for both, starting a successor project for Nouveau and
doing so using the Rust programming language, is documented in detail
through a previous post on the mailing list [1], an LWN article [2] and a
talk from LPC '24.

In order to avoid the chicken and egg problem to require a user to
upstream Rust abstractions, but at the same time require the Rust
abstractions to implement the driver, nova-core kicks off as a driver
stub and is subsequently developed upstream.

Besides the driver itself and the corresponding documentation, i.e. guidelines,
task list, etc., this series also carries a few more patches to more flexibly
compose firmware path strings for the .modinfo section.

Link: https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u [1]
Link: https://lwn.net/Articles/990736/ [2]
Link: https://youtu.be/3Igmx28B3BQ?si=sBdSEer4tAPKGpOs [3]

Changes in v5:
  - change `ModInfoBuilder::push()` to take a `&str` instead of `&[u8]`
  - drop patch "rust: str: provide const fn as_bytes() for BStr"
  - embedd `impl TryFrom<u32> for Chipset` in `define_chipset!` (Alexandre)
  - switch `Chipset::NAMES` from `[&BStr; N]` to `[&str; N]`
  - introduce `const_bytes_to_str()` helper
  - move `to_lowercase_bytes()` to util.rs

Changes in v4:
  - in `regs::Boot0` take a `&Bar0` and let deref coercion do its thing
    (Alexandre)
  - add missing firmware path string to the .modinfo section (Greg)
  - add some infrastructure to compose firmware path strings more flexibly

Changes in v3:
  - impl TryFrom<u32> for Chipset
  - add Chipset::arch()
  - initialize revision from Boot0
  - in Firmware, eliminate repeating code pattern using a closure (thanks to
    Alexandre)
  - use #[expect(dead_code)] for Firmware
  - Replace some Rust specific rules with links to existing R4L documentation.
  - Link in R4L submit checklist.
  - Update task entry "Page abstraction for foreign pages" with Lina's work.

Changes in v2:
  - Fix module name in Kconfig description. (John)
  - Expand Kconfig description a bit. (John)
  - Expand name for PCI BAR0 region.
  - Do not store / print boot0 raw register value. (John)
  - Rename CardType to Architecture, rename enum names to represent the
    architecture name and adjust enum values according to the register
    definition. (John)
  - Add an abstraction for register accesses.
  - Print chipset, architecture and revision.
  - Load bootloader firmware. (Timur)
  - Add task "Generic register abstraction".
  - Change complexity of "Debugfs abstractions".


Danilo Krummrich (5):
  rust: module: add type `LocalModule`
  rust: firmware: introduce `firmware::ModInfoBuilder`
  rust: firmware: add `module_firmware!` macro
  gpu: nova-core: add initial driver stub
  gpu: nova-core: add initial documentation

 Documentation/gpu/drivers.rst              |   1 +
 Documentation/gpu/nova/core/guidelines.rst |  24 ++
 Documentation/gpu/nova/core/todo.rst       | 446 +++++++++++++++++++++
 Documentation/gpu/nova/guidelines.rst      |  69 ++++
 Documentation/gpu/nova/index.rst           |  30 ++
 MAINTAINERS                                |  11 +
 drivers/gpu/Makefile                       |   1 +
 drivers/gpu/nova-core/Kconfig              |  14 +
 drivers/gpu/nova-core/Makefile             |   3 +
 drivers/gpu/nova-core/driver.rs            |  47 +++
 drivers/gpu/nova-core/firmware.rs          |  45 +++
 drivers/gpu/nova-core/gpu.rs               | 199 +++++++++
 drivers/gpu/nova-core/nova_core.rs         |  20 +
 drivers/gpu/nova-core/regs.rs              |  55 +++
 drivers/gpu/nova-core/util.rs              |  21 +
 drivers/video/Kconfig                      |   1 +
 rust/kernel/firmware.rs                    | 177 ++++++++
 rust/macros/module.rs                      |   2 +
 18 files changed, 1166 insertions(+)
 create mode 100644 Documentation/gpu/nova/core/guidelines.rst
 create mode 100644 Documentation/gpu/nova/core/todo.rst
 create mode 100644 Documentation/gpu/nova/guidelines.rst
 create mode 100644 Documentation/gpu/nova/index.rst
 create mode 100644 drivers/gpu/nova-core/Kconfig
 create mode 100644 drivers/gpu/nova-core/Makefile
 create mode 100644 drivers/gpu/nova-core/driver.rs
 create mode 100644 drivers/gpu/nova-core/firmware.rs
 create mode 100644 drivers/gpu/nova-core/gpu.rs
 create mode 100644 drivers/gpu/nova-core/nova_core.rs
 create mode 100644 drivers/gpu/nova-core/regs.rs
 create mode 100644 drivers/gpu/nova-core/util.rs


base-commit: 99fa936e8e4f117d62f229003c9799686f74cebc
-- 
2.48.1

[PATCH v5 1/5] rust: module: add type `LocalModule`

[Danilo Krummrich]

The `LocalModule` type is the type of the module created by `module!`,
`module_pci_driver!`, `module_platform_driver!`, etc.

Since the exact type of the module is sometimes generated on the fly by
the listed macros, provide an alias.

This is first used by the `module_firmware!` macro introduced in a
subsequent patch.

Suggested-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/macros/module.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index cdf94f4982df..6ba9210677c5 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -228,6 +228,8 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
                 kernel::ThisModule::from_ptr(core::ptr::null_mut())
             }};
 
+            type LocalModule = {type_};
+
             impl kernel::ModuleMetadata for {type_} {{
                 const NAME: &'static kernel::str::CStr = kernel::c_str!(\"{name}\");
             }}
-- 
2.48.1

[PATCH v5 2/5] rust: firmware: introduce `firmware::ModInfoBuilder`

[Danilo Krummrich]

The `firmware` field of the `module!` only accepts literal strings,
which is due to the fact that it is implemented as a proc macro.

Some drivers require a lot of firmware files (such as nova-core) and
hence benefit from more flexibility composing firmware path strings.

The `firmware::ModInfoBuilder` is a helper component to flexibly compose
firmware path strings for the .modinfo section in const context.

It is meant to be used in combination with `kernel::module_firmware!`,
which is introduced in a subsequent patch.

Co-developed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/firmware.rs | 98 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 98 insertions(+)

diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
index c5162fdc95ff..6e6972d94597 100644
--- a/rust/kernel/firmware.rs
+++ b/rust/kernel/firmware.rs
@@ -115,3 +115,101 @@ unsafe impl Send for Firmware {}
 // SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, references to which are safe to
 // be used from any thread.
 unsafe impl Sync for Firmware {}
+
+/// Builder for firmware module info.
+///
+/// [`ModInfoBuilder`] is a helper component to flexibly compose firmware paths strings for the
+/// .modinfo section in const context.
+///
+/// It is meant to be used in combination with [`kernel::module_firmware!`].
+///
+/// For more details and an example, see [`kernel::module_firmware!`].
+pub struct ModInfoBuilder<const N: usize> {
+    buf: [u8; N],
+    n: usize,
+    module_name: &'static CStr,
+}
+
+impl<const N: usize> ModInfoBuilder<N> {
+    /// Create an empty builder instance.
+    pub const fn new(module_name: &'static CStr) -> Self {
+        Self {
+            buf: [0; N],
+            n: 0,
+            module_name,
+        }
+    }
+
+    const fn push_internal(mut self, bytes: &[u8]) -> Self {
+        let mut j = 0;
+
+        if N == 0 {
+            self.n += bytes.len();
+            return self;
+        }
+
+        while j < bytes.len() {
+            if self.n < N {
+                self.buf[self.n] = bytes[j];
+            }
+            self.n += 1;
+            j += 1;
+        }
+        self
+    }
+
+    /// Push an additional path component.
+    ///
+    /// After a new [`ModInfoBuilder`] instance has been created, [`ModInfoBuilder::prepare`] must
+    /// be called before adding path components.
+    pub const fn push(self, s: &str) -> Self {
+        if N != 0 && self.n == 0 {
+            crate::build_error!("Must call prepare() before push().");
+        }
+
+        self.push_internal(s.as_bytes())
+    }
+
+    const fn prepare_module_name(self) -> Self {
+        let mut this = self;
+        let module_name = this.module_name;
+
+        if !this.module_name.is_empty() {
+            this = this.push_internal(module_name.as_bytes_with_nul());
+
+            if N != 0 {
+                // Re-use the space taken by the NULL terminator and swap it with the '.' separator.
+                this.buf[this.n - 1] = b'.';
+            }
+        }
+
+        this.push_internal(b"firmware=")
+    }
+
+    /// Prepare for the next module info entry.
+    ///
+    /// Must be called before [`ModInfoBuilder::push`] can be called.
+    pub const fn prepare(self) -> Self {
+        self.push_internal(b"\0").prepare_module_name()
+    }
+
+    /// Build the byte array.
+    pub const fn build(self) -> [u8; N] {
+        // Add the final NULL terminator.
+        let this = self.push_internal(b"\0");
+
+        if this.n == N {
+            this.buf
+        } else {
+            crate::build_error!("Length mismatch.");
+        }
+    }
+}
+
+impl ModInfoBuilder<0> {
+    /// Return the length of the byte array to build.
+    pub const fn build_length(self) -> usize {
+        // Compensate for the NULL terminator added by `build`.
+        self.n + 1
+    }
+}
-- 
2.48.1

[PATCH v5 3/5] rust: firmware: add `module_firmware!` macro

[Danilo Krummrich]

Analogous to the `module!` macro `module_firmware!` adds additional
firmware path strings to the .modinfo section.

In contrast to `module!`, where path strings need to be string literals,
path strings can be composed with the `firmware::ModInfoBuilder`.

Some drivers require a lot of firmware files (such as nova-core) and
hence benefit from more flexibility composing firmware path strings.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/firmware.rs | 79 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
index 6e6972d94597..5d1ac8287171 100644
--- a/rust/kernel/firmware.rs
+++ b/rust/kernel/firmware.rs
@@ -116,6 +116,85 @@ unsafe impl Send for Firmware {}
 // be used from any thread.
 unsafe impl Sync for Firmware {}
 
+/// Create firmware .modinfo entries.
+///
+/// This macro is the counterpart of the C macro `MODULE_FIRMWARE()`, but instead of taking a
+/// simple string literals, which is already covered by the `firmware` field of
+/// [`crate::prelude::module!`], it allows the caller to pass a builder type (e.g.
+/// [`ModInfoBuilder`]) which can create the firmware modinfo strings in a more flexible way.
+///
+/// Drivers should extend the [`ModInfoBuilder`] with their own driver specific builder type.
+///
+/// The `builder` argument must be a type which implements the following function.
+///
+/// `const fn create(module_name: &'static CStr) -> ModInfoBuilder`
+///
+/// `create` should pass the `module_name` to the [`ModInfoBuilder`] and, with the help of
+/// it construct the corresponding firmware modinfo.
+///
+/// Typically, such contracts would be enforced by a trait, however traits do not (yet) support
+/// const functions.
+///
+/// # Example
+///
+/// ```
+/// # mod module_firmware_test {
+/// # use kernel::firmware;
+/// # use kernel::prelude::*;
+/// #
+/// # struct MyModule;
+/// #
+/// # impl kernel::Module for MyModule {
+/// #     fn init(_module: &'static ThisModule) -> Result<Self> {
+/// #         Ok(Self)
+/// #     }
+/// # }
+/// #
+/// #
+/// struct Builder<const N: usize>;
+///
+/// impl<const N: usize> Builder<N> {
+///     const fn create(module_name: &'static kernel::str::CStr) -> firmware::ModInfoBuilder<N> {
+///         firmware::ModInfoBuilder::new(module_name)
+///             .prepare()
+///             .push("vendor/foo.bin")
+///             .prepare()
+///             .push("vendor/bar.bin")
+///     }
+/// }
+///
+/// module! {
+///    type: MyModule,
+///    name: "module_firmware_test",
+///    author: "Rust for Linux",
+///    description: "module_firmware! test module",
+///    license: "GPL",
+/// }
+///
+/// kernel::module_firmware!(Builder);
+/// # }
+/// ```
+#[macro_export]
+macro_rules! module_firmware {
+    ($($builder:tt)*) => {
+
+        #[cfg(not(MODULE))]
+        const fn __module_name() -> &'static kernel::str::CStr {
+            <LocalModule as kernel::ModuleMetadata>::NAME
+        }
+
+        #[cfg(MODULE)]
+        const fn __module_name() -> &'static kernel::str::CStr {
+            kernel::c_str!("")
+        }
+
+        #[link_section = ".modinfo"]
+        #[used]
+        static __MODULE_FIRMWARE: [u8; $($builder)*::create(__module_name()).build_length()] =
+            $($builder)*::create(__module_name()).build();
+    };
+}
+
 /// Builder for firmware module info.
 ///
 /// [`ModInfoBuilder`] is a helper component to flexibly compose firmware paths strings for the
-- 
2.48.1

[PATCH v5 4/5] gpu: nova-core: add initial driver stub

[Danilo Krummrich]

Add the initial nova-core driver stub.

nova-core is intended to serve as a common base for nova-drm (the
corresponding DRM driver) and the vGPU manager VFIO driver, serving as a
hard- and firmware abstraction layer for GSP-based NVIDIA GPUs.

The Nova project, including nova-core and nova-drm, in the long term,
is intended to serve as the successor of Nouveau for all GSP-based GPUs.

The motivation for both, starting a successor project for Nouveau and
doing so using the Rust programming language, is documented in detail
through a previous post on the mailing list [1], an LWN article [2] and a
talk from LPC '24.

In order to avoid the chicken and egg problem to require a user to
upstream Rust abstractions, but at the same time require the Rust
abstractions to implement the driver, nova-core kicks off as a driver
stub and is subsequently developed upstream.

Link: https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u [1]
Link: https://lwn.net/Articles/990736/ [2]
Link: https://youtu.be/3Igmx28B3BQ?si=sBdSEer4tAPKGpOs [3]
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 MAINTAINERS                        |  10 ++
 drivers/gpu/Makefile               |   1 +
 drivers/gpu/nova-core/Kconfig      |  14 ++
 drivers/gpu/nova-core/Makefile     |   3 +
 drivers/gpu/nova-core/driver.rs    |  47 +++++++
 drivers/gpu/nova-core/firmware.rs  |  45 +++++++
 drivers/gpu/nova-core/gpu.rs       | 199 +++++++++++++++++++++++++++++
 drivers/gpu/nova-core/nova_core.rs |  20 +++
 drivers/gpu/nova-core/regs.rs      |  55 ++++++++
 drivers/gpu/nova-core/util.rs      |  21 +++
 drivers/video/Kconfig              |   1 +
 11 files changed, 416 insertions(+)
 create mode 100644 drivers/gpu/nova-core/Kconfig
 create mode 100644 drivers/gpu/nova-core/Makefile
 create mode 100644 drivers/gpu/nova-core/driver.rs
 create mode 100644 drivers/gpu/nova-core/firmware.rs
 create mode 100644 drivers/gpu/nova-core/gpu.rs
 create mode 100644 drivers/gpu/nova-core/nova_core.rs
 create mode 100644 drivers/gpu/nova-core/regs.rs
 create mode 100644 drivers/gpu/nova-core/util.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index 8e0736dc2ee0..644817ccaa18 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7449,6 +7449,16 @@ T:	git https://gitlab.freedesktop.org/drm/nouveau.git
 F:	drivers/gpu/drm/nouveau/
 F:	include/uapi/drm/nouveau_drm.h
 
+CORE DRIVER FOR NVIDIA GPUS [RUST]
+M:	Danilo Krummrich <dakr@kernel.org>
+L:	nouveau@lists.freedesktop.org
+S:	Supported
+Q:	https://patchwork.freedesktop.org/project/nouveau/
+B:	https://gitlab.freedesktop.org/drm/nova/-/issues
+C:	irc://irc.oftc.net/nouveau
+T:	git https://gitlab.freedesktop.org/drm/nova.git nova-next
+F:	drivers/gpu/nova-core/
+
 DRM DRIVER FOR OLIMEX LCD-OLINUXINO PANELS
 M:	Stefan Mavrodiev <stefan@olimex.com>
 S:	Maintained
diff --git a/drivers/gpu/Makefile b/drivers/gpu/Makefile
index 8997f0096545..36a54d456630 100644
--- a/drivers/gpu/Makefile
+++ b/drivers/gpu/Makefile
@@ -5,3 +5,4 @@
 obj-y			+= host1x/ drm/ vga/
 obj-$(CONFIG_IMX_IPUV3_CORE)	+= ipu-v3/
 obj-$(CONFIG_TRACE_GPU_MEM)		+= trace/
+obj-$(CONFIG_NOVA_CORE)		+= nova-core/
diff --git a/drivers/gpu/nova-core/Kconfig b/drivers/gpu/nova-core/Kconfig
new file mode 100644
index 000000000000..ad0c06756516
--- /dev/null
+++ b/drivers/gpu/nova-core/Kconfig
@@ -0,0 +1,14 @@
+config NOVA_CORE
+	tristate "Nova Core GPU driver"
+	depends on PCI
+	depends on RUST
+	depends on RUST_FW_LOADER_ABSTRACTIONS
+	default n
+	help
+	  Choose this if you want to build the Nova Core driver for Nvidia
+	  GPUs based on the GPU System Processor (GSP). This is true for Turing
+	  and later GPUs.
+
+	  This driver is work in progress and may not be functional.
+
+	  If M is selected, the module will be called nova_core.
diff --git a/drivers/gpu/nova-core/Makefile b/drivers/gpu/nova-core/Makefile
new file mode 100644
index 000000000000..2d78c50126e1
--- /dev/null
+++ b/drivers/gpu/nova-core/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_NOVA_CORE) += nova_core.o
diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
new file mode 100644
index 000000000000..63c19f140fbd
--- /dev/null
+++ b/drivers/gpu/nova-core/driver.rs
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use kernel::{bindings, c_str, pci, prelude::*};
+
+use crate::gpu::Gpu;
+
+#[pin_data]
+pub(crate) struct NovaCore {
+    #[pin]
+    pub(crate) gpu: Gpu,
+}
+
+const BAR0_SIZE: usize = 8;
+pub(crate) type Bar0 = pci::Bar<BAR0_SIZE>;
+
+kernel::pci_device_table!(
+    PCI_TABLE,
+    MODULE_PCI_TABLE,
+    <NovaCore as pci::Driver>::IdInfo,
+    [(
+        pci::DeviceId::from_id(bindings::PCI_VENDOR_ID_NVIDIA, bindings::PCI_ANY_ID as _),
+        ()
+    )]
+);
+
+impl pci::Driver for NovaCore {
+    type IdInfo = ();
+    const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
+
+    fn probe(pdev: &mut pci::Device, _info: &Self::IdInfo) -> Result<Pin<KBox<Self>>> {
+        dev_dbg!(pdev.as_ref(), "Probe Nova Core GPU driver.\n");
+
+        pdev.enable_device_mem()?;
+        pdev.set_master();
+
+        let bar = pdev.iomap_region_sized::<BAR0_SIZE>(0, c_str!("nova-core/bar0"))?;
+
+        let this = KBox::pin_init(
+            try_pin_init!(Self {
+                gpu <- Gpu::new(pdev, bar)?,
+            }),
+            GFP_KERNEL,
+        )?;
+
+        Ok(this)
+    }
+}
diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
new file mode 100644
index 000000000000..9de1399a2a69
--- /dev/null
+++ b/drivers/gpu/nova-core/firmware.rs
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use crate::gpu;
+use kernel::firmware;
+
+pub(crate) struct ModInfoBuilder<const N: usize>(firmware::ModInfoBuilder<N>);
+
+impl<const N: usize> ModInfoBuilder<N> {
+    const fn make_entry_file(self, chipset: &str, fw: &str) -> Self {
+        let version = "535.113.01";
+
+        ModInfoBuilder(
+            self.0
+                .prepare()
+                .push("nvidia/")
+                .push(chipset)
+                .push("/gsp/")
+                .push(fw)
+                .push("-")
+                .push(version)
+                .push(".bin"),
+        )
+    }
+
+    const fn make_entry_chipset(self, chipset: &str) -> Self {
+        self.make_entry_file(chipset, "booter_load")
+            .make_entry_file(chipset, "booter_unload")
+            .make_entry_file(chipset, "bootloader")
+            .make_entry_file(chipset, "gsp")
+    }
+
+    pub(crate) const fn create(
+        module_name: &'static kernel::str::CStr,
+    ) -> firmware::ModInfoBuilder<N> {
+        let mut this = Self(firmware::ModInfoBuilder::new(module_name));
+        let mut i = 0;
+
+        while i < gpu::Chipset::NAMES.len() {
+            this = this.make_entry_chipset(gpu::Chipset::NAMES[i]);
+            i += 1;
+        }
+
+        this.0
+    }
+}
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
new file mode 100644
index 000000000000..f57b7efa10f3
--- /dev/null
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -0,0 +1,199 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use kernel::{
+    device, devres::Devres, error::code::*, firmware, fmt, pci, prelude::*, str::CString,
+};
+
+use crate::driver::Bar0;
+use crate::regs;
+use crate::util;
+use core::fmt;
+
+macro_rules! define_chipset {
+    ({ $($variant:ident = $value:expr),* $(,)* }) =>
+    {
+        /// Enum representation of the GPU chipset.
+        #[derive(fmt::Debug)]
+        pub(crate) enum Chipset {
+            $($variant = $value),*,
+        }
+
+        impl Chipset {
+            pub(crate) const ALL: &'static [Chipset] = &[
+                $( Chipset::$variant, )*
+            ];
+
+            pub(crate) const NAMES: [&str; Self::ALL.len()] = [
+                $( util::const_bytes_to_str(
+                        util::to_lowercase_bytes::<{ stringify!($variant).len() }>(
+                            stringify!($variant)
+                        ).as_slice()
+                ), )*
+            ];
+        }
+
+        // TODO replace with something like derive(FromPrimitive)
+        impl TryFrom<u32> for Chipset {
+            type Error = kernel::error::Error;
+
+            fn try_from(value: u32) -> Result<Self, Self::Error> {
+                match value {
+                    $( $value => Ok(Chipset::$variant), )*
+                    _ => Err(ENODEV),
+                }
+            }
+        }
+    }
+}
+
+define_chipset!({
+    // Turing
+    TU102 = 0x162,
+    TU104 = 0x164,
+    TU106 = 0x166,
+    TU117 = 0x167,
+    TU116 = 0x168,
+    // Ampere
+    GA102 = 0x172,
+    GA103 = 0x173,
+    GA104 = 0x174,
+    GA106 = 0x176,
+    GA107 = 0x177,
+    // Ada
+    AD102 = 0x192,
+    AD103 = 0x193,
+    AD104 = 0x194,
+    AD106 = 0x196,
+    AD107 = 0x197,
+});
+
+impl Chipset {
+    pub(crate) fn arch(&self) -> Architecture {
+        match self {
+            Self::TU102 | Self::TU104 | Self::TU106 | Self::TU117 | Self::TU116 => {
+                Architecture::Turing
+            }
+            Self::GA102 | Self::GA103 | Self::GA104 | Self::GA106 | Self::GA107 => {
+                Architecture::Ampere
+            }
+            Self::AD102 | Self::AD103 | Self::AD104 | Self::AD106 | Self::AD107 => {
+                Architecture::Ada
+            }
+        }
+    }
+}
+
+// TODO
+//
+// The resulting strings are used to generate firmware paths, hence the
+// generated strings have to be stable.
+//
+// Hence, replace with something like strum_macros derive(Display).
+//
+// For now, redirect to fmt::Debug for convenience.
+impl fmt::Display for Chipset {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "{:?}", self)
+    }
+}
+
+/// Enum representation of the GPU generation.
+#[derive(fmt::Debug)]
+pub(crate) enum Architecture {
+    Turing,
+    Ampere,
+    Ada,
+}
+
+pub(crate) struct Revision {
+    major: u8,
+    minor: u8,
+}
+
+impl Revision {
+    fn from_boot0(boot0: regs::Boot0) -> Self {
+        Self {
+            major: boot0.major_rev(),
+            minor: boot0.minor_rev(),
+        }
+    }
+}
+
+impl fmt::Display for Revision {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "{:x}.{:x}", self.major, self.minor)
+    }
+}
+
+/// Structure holding the metadata of the GPU.
+pub(crate) struct Spec {
+    chipset: Chipset,
+    /// The revision of the chipset.
+    revision: Revision,
+}
+
+impl Spec {
+    fn new(bar: &Devres<Bar0>) -> Result<Spec> {
+        let bar = bar.try_access().ok_or(ENXIO)?;
+        let boot0 = regs::Boot0::read(&bar);
+
+        Ok(Self {
+            chipset: boot0.chipset().try_into()?,
+            revision: Revision::from_boot0(boot0),
+        })
+    }
+}
+
+/// Structure encapsulating the firmware blobs required for the GPU to operate.
+#[expect(dead_code)]
+pub(crate) struct Firmware {
+    booter_load: firmware::Firmware,
+    booter_unload: firmware::Firmware,
+    bootloader: firmware::Firmware,
+    gsp: firmware::Firmware,
+}
+
+impl Firmware {
+    fn new(dev: &device::Device, spec: &Spec, ver: &str) -> Result<Firmware> {
+        let mut chip_name = CString::try_from_fmt(fmt!("{}", spec.chipset))?;
+        chip_name.make_ascii_lowercase();
+
+        let request = |name_| {
+            CString::try_from_fmt(fmt!("nvidia/{}/gsp/{}-{}.bin", &*chip_name, name_, ver))
+                .and_then(|path| firmware::Firmware::request(&path, dev))
+        };
+
+        Ok(Firmware {
+            booter_load: request("booter_load")?,
+            booter_unload: request("booter_unload")?,
+            bootloader: request("bootloader")?,
+            gsp: request("gsp")?,
+        })
+    }
+}
+
+/// Structure holding the resources required to operate the GPU.
+#[pin_data]
+pub(crate) struct Gpu {
+    spec: Spec,
+    /// MMIO mapping of PCI BAR 0
+    bar: Devres<Bar0>,
+    fw: Firmware,
+}
+
+impl Gpu {
+    pub(crate) fn new(pdev: &pci::Device, bar: Devres<Bar0>) -> Result<impl PinInit<Self>> {
+        let spec = Spec::new(&bar)?;
+        let fw = Firmware::new(pdev.as_ref(), &spec, "535.113.01")?;
+
+        dev_info!(
+            pdev.as_ref(),
+            "NVIDIA (Chipset: {}, Architecture: {:?}, Revision: {})\n",
+            spec.chipset,
+            spec.chipset.arch(),
+            spec.revision
+        );
+
+        Ok(pin_init!(Self { spec, bar, fw }))
+    }
+}
diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
new file mode 100644
index 000000000000..a91cd924054b
--- /dev/null
+++ b/drivers/gpu/nova-core/nova_core.rs
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Nova Core GPU Driver
+
+mod driver;
+mod firmware;
+mod gpu;
+mod regs;
+mod util;
+
+kernel::module_pci_driver! {
+    type: driver::NovaCore,
+    name: "NovaCore",
+    author: "Danilo Krummrich",
+    description: "Nova Core GPU driver",
+    license: "GPL v2",
+    firmware: [],
+}
+
+kernel::module_firmware!(firmware::ModInfoBuilder);
diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
new file mode 100644
index 000000000000..50aefb150b0b
--- /dev/null
+++ b/drivers/gpu/nova-core/regs.rs
@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use crate::driver::Bar0;
+
+// TODO
+//
+// Create register definitions via generic macros. See task "Generic register
+// abstraction" in Documentation/gpu/nova/core/todo.rst.
+
+const BOOT0_OFFSET: usize = 0x00000000;
+
+// 3:0 - chipset minor revision
+const BOOT0_MINOR_REV_SHIFT: u8 = 0;
+const BOOT0_MINOR_REV_MASK: u32 = 0x0000000f;
+
+// 7:4 - chipset major revision
+const BOOT0_MAJOR_REV_SHIFT: u8 = 4;
+const BOOT0_MAJOR_REV_MASK: u32 = 0x000000f0;
+
+// 23:20 - chipset implementation Identifier (depends on architecture)
+const BOOT0_IMPL_SHIFT: u8 = 20;
+const BOOT0_IMPL_MASK: u32 = 0x00f00000;
+
+// 28:24 - chipset architecture identifier
+const BOOT0_ARCH_MASK: u32 = 0x1f000000;
+
+// 28:20 - chipset identifier (virtual register field combining BOOT0_IMPL and
+//         BOOT0_ARCH)
+const BOOT0_CHIPSET_SHIFT: u8 = BOOT0_IMPL_SHIFT;
+const BOOT0_CHIPSET_MASK: u32 = BOOT0_IMPL_MASK | BOOT0_ARCH_MASK;
+
+#[derive(Copy, Clone)]
+pub(crate) struct Boot0(u32);
+
+impl Boot0 {
+    #[inline]
+    pub(crate) fn read(bar: &Bar0) -> Self {
+        Self(bar.readl(BOOT0_OFFSET))
+    }
+
+    #[inline]
+    pub(crate) fn chipset(&self) -> u32 {
+        (self.0 & BOOT0_CHIPSET_MASK) >> BOOT0_CHIPSET_SHIFT
+    }
+
+    #[inline]
+    pub(crate) fn minor_rev(&self) -> u8 {
+        ((self.0 & BOOT0_MINOR_REV_MASK) >> BOOT0_MINOR_REV_SHIFT) as u8
+    }
+
+    #[inline]
+    pub(crate) fn major_rev(&self) -> u8 {
+        ((self.0 & BOOT0_MAJOR_REV_MASK) >> BOOT0_MAJOR_REV_SHIFT) as u8
+    }
+}
diff --git a/drivers/gpu/nova-core/util.rs b/drivers/gpu/nova-core/util.rs
new file mode 100644
index 000000000000..332a64cfc6a9
--- /dev/null
+++ b/drivers/gpu/nova-core/util.rs
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0
+
+pub(crate) const fn to_lowercase_bytes<const N: usize>(s: &str) -> [u8; N] {
+    let src = s.as_bytes();
+    let mut dst = [0; N];
+    let mut i = 0;
+
+    while i < src.len() && i < N {
+        dst[i] = (src[i] as char).to_ascii_lowercase() as u8;
+        i += 1;
+    }
+
+    dst
+}
+
+pub(crate) const fn const_bytes_to_str(bytes: &[u8]) -> &str {
+    match core::str::from_utf8(bytes) {
+        Ok(string) => string,
+        Err(_) => kernel::build_error!("Bytes are not valid UTF-8."),
+    }
+}
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 44c9ef1435a2..5df981920a94 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -39,6 +39,7 @@ source "drivers/gpu/vga/Kconfig"
 
 source "drivers/gpu/host1x/Kconfig"
 source "drivers/gpu/ipu-v3/Kconfig"
+source "drivers/gpu/nova-core/Kconfig"
 
 source "drivers/gpu/drm/Kconfig"
 
-- 
2.48.1

[PATCH v5 5/5] gpu: nova-core: add initial documentation

[Danilo Krummrich]

Add the initial documentation of the Nova project.

The initial project documentation consists out of a brief introduction
of the project, as well as project guidelines both general and nova-core
specific and a task list for nova-core specifically.

The task list is divided into tasks for general Rust infrastructure
required by the project, tasks regarding GSP enablement and firmware
abstraction, general GPU driver tasks as well as tasks related to
external API design and test infrastructure.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 Documentation/gpu/drivers.rst              |   1 +
 Documentation/gpu/nova/core/guidelines.rst |  24 ++
 Documentation/gpu/nova/core/todo.rst       | 446 +++++++++++++++++++++
 Documentation/gpu/nova/guidelines.rst      |  69 ++++
 Documentation/gpu/nova/index.rst           |  30 ++
 MAINTAINERS                                |   1 +
 6 files changed, 571 insertions(+)
 create mode 100644 Documentation/gpu/nova/core/guidelines.rst
 create mode 100644 Documentation/gpu/nova/core/todo.rst
 create mode 100644 Documentation/gpu/nova/guidelines.rst
 create mode 100644 Documentation/gpu/nova/index.rst

diff --git a/Documentation/gpu/drivers.rst b/Documentation/gpu/drivers.rst
index 1f17ad0790d7..7c2c5dcb5fd4 100644
--- a/Documentation/gpu/drivers.rst
+++ b/Documentation/gpu/drivers.rst
@@ -24,6 +24,7 @@ GPU Driver Documentation
    panfrost
    panthor
    zynqmp
+   nova/index
 
 .. only::  subproject and html
 
diff --git a/Documentation/gpu/nova/core/guidelines.rst b/Documentation/gpu/nova/core/guidelines.rst
new file mode 100644
index 000000000000..a389d65d7982
--- /dev/null
+++ b/Documentation/gpu/nova/core/guidelines.rst
@@ -0,0 +1,24 @@
+.. SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+
+==========
+Guidelines
+==========
+
+This documents contains the guidelines for nova-core. Additionally, all common
+guidelines of the Nova project do apply.
+
+Driver API
+==========
+
+One main purpose of nova-core is to implement the abstraction around the
+firmware interface of GSP and provide a firmware (version) independent API for
+2nd level drivers, such as nova-drm or the vGPU manager VFIO driver.
+
+Therefore, it is not permitted to leak firmware (version) specifics, through the
+driver API, to 2nd level drivers.
+
+Acceptance Criteria
+===================
+
+- To the extend possible, patches submitted to nova-core must be tested for
+  regressions with all 2nd level drivers.
diff --git a/Documentation/gpu/nova/core/todo.rst b/Documentation/gpu/nova/core/todo.rst
new file mode 100644
index 000000000000..3e8d2125da9d
--- /dev/null
+++ b/Documentation/gpu/nova/core/todo.rst
@@ -0,0 +1,446 @@
+.. SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+
+=========
+Task List
+=========
+
+Tasks may have the following fields:
+
+- ``Complexity``: Describes the required familiarity with Rust and / or the
+  corresponding kernel APIs or subsystems. There are four different complexities,
+  ``Beginner``, ``Intermediate``, ``Advanced`` and ``Expert``.
+- ``Reference``: References to other tasks.
+- ``Link``: Links to external resources.
+- ``Contact``: The person that can be contacted for further information about
+  the task.
+
+Enablement (Rust)
+=================
+
+Tasks that are not directly related to nova-core, but are preconditions in terms
+of required APIs.
+
+FromPrimitive API
+-----------------
+
+Sometimes the need arises to convert a number to a value of an enum or a
+structure.
+
+A good example from nova-core would be the ``Chipset`` enum type, which defines
+the value ``AD102``. When probing the GPU the value ``0x192`` can be read from a
+certain register indication the chipset AD102. Hence, the enum value ``AD102``
+should be derived from the number ``0x192``. Currently, nova-core uses a custom
+implementation (``Chipset::from_u32`` for this.
+
+Instead, it would be desirable to have something like the ``FromPrimitive``
+trait [1] from the num crate.
+
+Having this generalization also helps with implementing a generic macro that
+automatically generates the corresponding mappings between a value and a number.
+
+| Complexity: Beginner
+| Link: https://docs.rs/num/latest/num/trait.FromPrimitive.html
+
+Generic register abstraction
+----------------------------
+
+Work out how register constants and structures can be automatically generated
+through generalized macros.
+
+Example:
+
+.. code-block:: rust
+
+	register!(BOOT0, 0x0, u32, pci::Bar<SIZE>, Fields [
+	   MINOR_REVISION(3:0, RO),
+	   MAJOR_REVISION(7:4, RO),
+	   REVISION(7:0, RO), // Virtual register combining major and minor rev.
+	])
+
+This could expand to something like:
+
+.. code-block:: rust
+
+	const BOOT0_OFFSET: usize = 0x00000000;
+	const BOOT0_MINOR_REVISION_SHIFT: u8 = 0;
+	const BOOT0_MINOR_REVISION_MASK: u32 = 0x0000000f;
+	const BOOT0_MAJOR_REVISION_SHIFT: u8 = 4;
+	const BOOT0_MAJOR_REVISION_MASK: u32 = 0x000000f0;
+	const BOOT0_REVISION_SHIFT: u8 = BOOT0_MINOR_REVISION_SHIFT;
+	const BOOT0_REVISION_MASK: u32 = BOOT0_MINOR_REVISION_MASK | BOOT0_MAJOR_REVISION_MASK;
+
+	struct Boot0(u32);
+
+	impl Boot0 {
+	   #[inline]
+	   fn read(bar: &RevocableGuard<'_, pci::Bar<SIZE>>) -> Self {
+	      Self(bar.readl(BOOT0_OFFSET))
+	   }
+
+	   #[inline]
+	   fn minor_revision(&self) -> u32 {
+	      (self.0 & BOOT0_MINOR_REVISION_MASK) >> BOOT0_MINOR_REVISION_SHIFT
+	   }
+
+	   #[inline]
+	   fn major_revision(&self) -> u32 {
+	      (self.0 & BOOT0_MAJOR_REVISION_MASK) >> BOOT0_MAJOR_REVISION_SHIFT
+	   }
+
+	   #[inline]
+	   fn revision(&self) -> u32 {
+	      (self.0 & BOOT0_REVISION_MASK) >> BOOT0_REVISION_SHIFT
+	   }
+	}
+
+Usage:
+
+.. code-block:: rust
+
+	let bar = bar.try_access().ok_or(ENXIO)?;
+
+	let boot0 = Boot0::read(&bar);
+	pr_info!("Revision: {}\n", boot0.revision());
+
+| Complexity: Advanced
+
+Delay / Sleep abstractions
+--------------------------
+
+Rust abstractions for the kernel's delay() and sleep() functions.
+
+There is some ongoing work from FUJITA Tomonori [1], which has not seen any updates
+since Oct. 24.
+
+| Complexity: Beginner
+| Link: https://lore.kernel.org/netdev/20241001112512.4861-2-fujita.tomonori@gmail.com/ [1]
+
+IRQ abstractions
+----------------
+
+Rust abstractions for IRQ handling.
+
+There is active ongoing work from Daniel Almeida [1] for the "core" abstractions
+to request IRQs.
+
+Besides optional review and testing work, the required ``pci::Device`` code
+around those core abstractions needs to be worked out.
+
+| Complexity: Intermediate
+| Link: https://lore.kernel.org/lkml/20250122163932.46697-1-daniel.almeida@collabora.com/ [1]
+| Contact: Daniel Almeida
+
+Page abstraction for foreign pages
+----------------------------------
+
+Rust abstractions for pages not created by the Rust page abstraction without
+direct ownership.
+
+There is active onging work from Abdiel Janulgue [1] and Lina [2].
+
+| Complexity: Advanced
+| Link: https://lore.kernel.org/linux-mm/20241119112408.779243-1-abdiel.janulgue@gmail.com/ [1]
+| Link: https://lore.kernel.org/rust-for-linux/20250202-rust-page-v1-0-e3170d7fe55e@asahilina.net/ [2]
+
+Scatterlist / sg_table abstractions
+-----------------------------------
+
+Rust abstractions for scatterlist / sg_table.
+
+There is preceding work from Abdiel Janulgue, which hasn't made it to the
+mailing list yet.
+
+| Complexity: Intermediate
+| Contact: Abdiel Janulgue
+
+ELF utils
+---------
+
+Rust implementation of ELF header representation to retrieve section header
+tables, names, and data from an ELF-formatted images.
+
+There is preceding work from Abdiel Janulgue, which hasn't made it to the
+mailing list yet.
+
+| Complexity: Beginner
+| Contact: Abdiel Janulgue
+
+PCI MISC APIs
+-------------
+
+Extend the existing PCI device / driver abstractions by SR-IOV, config space,
+capability, MSI API abstractions.
+
+| Complexity: Beginner
+
+Auxiliary bus abstractions
+--------------------------
+
+Rust abstraction for the auxiliary bus APIs.
+
+This is needed to connect nova-core to the nova-drm driver.
+
+| Complexity: Intermediate
+
+Debugfs abstractions
+--------------------
+
+Rust abstraction for debugfs APIs.
+
+| Reference: Export GSP log buffers
+| Complexity: Intermediate
+
+Vec extensions
+--------------
+
+Implement ``Vec::truncate`` and ``Vec::resize``.
+
+Currently this is used for some experimental code to parse the vBIOS.
+
+| Reference vBIOS support
+| Complexity: Beginner
+
+GPU (general)
+=============
+
+Parse firmware headers
+----------------------
+
+Parse ELF headers from the firmware files loaded from the filesystem.
+
+| Reference: ELF utils
+| Complexity: Beginner
+| Contact: Abdiel Janulgue
+
+Build radix3 page table
+-----------------------
+
+Build the radix3 page table to map the firmware.
+
+| Complexity: Intermediate
+| Contact: Abdiel Janulgue
+
+vBIOS support
+-------------
+
+Parse the vBIOS and probe the structures required for driver initialization.
+
+| Contact: Dave Airlie
+| Reference: Vec extensions
+| Complexity: Intermediate
+
+Initial Devinit support
+-----------------------
+
+Implement BIOS Device Initialization, i.e. memory sizing, waiting, PLL
+configuration.
+
+| Contact: Dave Airlie
+| Complexity: Beginner
+
+Boot Falcon controller
+----------------------
+
+Infrastructure to load and execute falcon (sec2) firmware images; handle the
+GSP falcon processor and fwsec loading.
+
+| Complexity: Advanced
+| Contact: Dave Airlie
+
+GPU Timer support
+-----------------
+
+Support for the GPU's internal timer peripheral.
+
+| Complexity: Beginner
+| Contact: Dave Airlie
+
+MMU / PT management
+-------------------
+
+Work out the architecture for MMU / page table management.
+
+We need to consider that nova-drm will need rather fine-grained control,
+especially in terms of locking, in order to be able to implement asynchronous
+Vulkan queues.
+
+While generally sharing the corresponding code is desirable, it needs to be
+evaluated how (and if at all) sharing the corresponding code is expedient.
+
+| Complexity: Expert
+
+VRAM memory allocator
+---------------------
+
+Investigate options for a VRAM memory allocator.
+
+Some possible options:
+  - Rust abstractions for
+    - RB tree (interval tree) / drm_mm
+    - maple_tree
+  - native Rust collections
+
+| Complexity: Advanced
+
+Instance Memory
+---------------
+
+Implement support for instmem (bar2) used to store page tables.
+
+| Complexity: Intermediate
+| Contact: Dave Airlie
+
+GPU System Processor (GSP)
+==========================
+
+Export GSP log buffers
+----------------------
+
+Recent patches from Timur Tabi [1] added support to expose GSP-RM log buffers
+(even after failure to probe the driver) through debugfs.
+
+This is also an interesting feature for nova-core, especially in the early days.
+
+| Link: https://lore.kernel.org/nouveau/20241030202952.694055-2-ttabi@nvidia.com/ [1]
+| Reference: Debugfs abstractions
+| Complexity: Intermediate
+
+GSP firmware abstraction
+------------------------
+
+The GSP-RM firmware API is unstable and may incompatibly change from version to
+version, in terms of data structures and semantics.
+
+This problem is one of the big motivations for using Rust for nova-core, since
+it turns out that Rust's procedural macro feature provides a rather elegant way
+to address this issue:
+
+1. generate Rust structures from the C headers in a separate namespace per version
+2. build abstraction structures (within a generic namespace) that implement the
+   firmware interfaces; annotate the differences in implementation with version
+   identifiers
+3. use a procedural macro to generate the actual per version implementation out
+   of this abstraction
+4. instantiate the correct version type one on runtime (can be sure that all
+   have the same interface because it's defined by a common trait)
+
+There is a PoC implementation of this pattern, in the context of the nova-core
+PoC driver.
+
+This task aims at refining the feature and ideally generalize it, to be usable
+by other drivers as well.
+
+| Complexity: Expert
+
+GSP message queue
+-----------------
+
+Implement low level GSP message queue (command, status) for communication
+between the kernel driver and GSP.
+
+| Complexity: Advanced
+| Contact: Dave Airlie
+
+Bootstrap GSP
+-------------
+
+Call the boot firmware to boot the GSP processor; execute initial control
+messages.
+
+| Complexity: Intermediate
+| Contact: Dave Airlie
+
+Client / Device APIs
+--------------------
+
+Implement the GSP message interface for client / device allocation and the
+corresponding client and device allocation APIs.
+
+| Complexity: Intermediate
+| Contact: Dave Airlie
+
+Bar PDE handling
+----------------
+
+Synchronize page table handling for BARs between the kernel driver and GSP.
+
+| Complexity: Beginner
+| Contact: Dave Airlie
+
+FIFO engine
+-----------
+
+Implement support for the FIFO engine, i.e. the corresponding GSP message
+interface and provide an API for chid allocation and channel handling.
+
+| Complexity: Advanced
+| Contact: Dave Airlie
+
+GR engine
+---------
+
+Implement support for the graphics engine, i.e. the corresponding GSP message
+interface and provide an API for (golden) context creation and promotion.
+
+| Complexity: Advanced
+| Contact: Dave Airlie
+
+CE engine
+---------
+
+Implement support for the copy engine, i.e. the corresponding GSP message
+interface.
+
+| Complexity: Intermediate
+| Contact: Dave Airlie
+
+VFN IRQ controller
+------------------
+
+Support for the VFN interrupt controller.
+
+| Complexity: Intermediate
+| Contact: Dave Airlie
+
+External APIs
+=============
+
+nova-core base API
+------------------
+
+Work out the common pieces of the API to connect 2nd level drivers, i.e. vGPU
+manager and nova-drm.
+
+| Complexity: Advanced
+
+vGPU manager API
+----------------
+
+Work out the API parts required by the vGPU manager, which are not covered by
+the base API.
+
+| Complexity: Advanced
+
+nova-core C API
+---------------
+
+Implement a C wrapper for the APIs required by the vGPU manager driver.
+
+| Complexity: Intermediate
+
+Testing
+=======
+
+CI pipeline
+-----------
+
+Investigate option for continuous integration testing.
+
+This can go from as simple as running KUnit tests over running (graphics) CTS to
+booting up (multiple) guest VMs to test VFIO use-cases.
+
+It might also be worth to consider the introduction of a new test suite directly
+sitting on top of the uAPI for more targeted testing and debugging. There may be
+options for collaboration / shared code with the Mesa project.
+
+| Complexity: Advanced
diff --git a/Documentation/gpu/nova/guidelines.rst b/Documentation/gpu/nova/guidelines.rst
new file mode 100644
index 000000000000..13ab13984a18
--- /dev/null
+++ b/Documentation/gpu/nova/guidelines.rst
@@ -0,0 +1,69 @@
+.. SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+
+==========
+Guidelines
+==========
+
+This document describes the general project guidelines that apply to nova-core
+and nova-drm.
+
+Language
+========
+
+The Nova project uses the Rust programming language. In this context, all rules
+of the Rust for Linux project as documented in
+:doc:`../../rust/general-information` apply. Additionally, the following rules
+apply.
+
+- Unless technically necessary otherwise (e.g. uAPI), any driver code is written
+  in Rust.
+
+- Unless technically necessary, unsafe Rust code must be avoided. In case of
+  technical necessity, unsafe code should be isolated in a separate component
+  providing a safe API for other driver code to use.
+
+Style
+-----
+
+All rules of the Rust for Linux project as documented in
+:doc:`../../rust/coding-guidelines` apply.
+
+For a submit checklist, please also see the `Rust for Linux Submit checklist
+addendum <https://rust-for-linux.com/contributing#submit-checklist-addendum>`_.
+
+Documentation
+=============
+
+The availability of proper documentation is essential in terms of scalability,
+accessibility for new contributors and maintainability of a project in general,
+but especially for a driver running as complex hardware as Nova is targeting.
+
+Hence, adding documentation of any kind is very much encouraged by the project.
+
+Besides that, there are some minimum requirements.
+
+- Every non-private structure needs at least a brief doc comment explaining the
+  semantical sense of the structure, as well as potential locking and lifetime
+  requirements. It is encouraged to have the same minimum documentation for
+  non-trivial private structures.
+
+- uAPIs must be fully documented with kernel-doc comments; additionally, the
+  semantical behavior must be explained including potential special or corner
+  cases.
+
+- The APIs connecting the 1st level driver (nova-core) with 2nd level drivers
+  must be fully documented. This includes doc comments, potential locking and
+  lifetime requirements, as well as example code if applicable.
+
+- Abbreviations must be explained when introduced; terminology must be uniquely
+  defined.
+
+- Register addresses, layouts, shift values and masks must be defined properly;
+  unless obvious, the semantical sense must be documented. This only applies if
+  the author is able to obtain the corresponding information.
+
+Acceptance Criteria
+===================
+
+- Patches must only be applied if reviewed by at least one other person on the
+  mailing list; this also applies for maintainers.
diff --git a/Documentation/gpu/nova/index.rst b/Documentation/gpu/nova/index.rst
new file mode 100644
index 000000000000..2701b3f4af35
--- /dev/null
+++ b/Documentation/gpu/nova/index.rst
@@ -0,0 +1,30 @@
+.. SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+
+=======================
+nova NVIDIA GPU drivers
+=======================
+
+The nova driver project consists out of two separate drivers nova-core and
+nova-drm and intends to supersede the nouveau driver for NVIDIA GPUs based on
+the GPU System Processor (GSP).
+
+The following documents apply to both nova-core and nova-drm.
+
+.. toctree::
+   :titlesonly:
+
+   guidelines
+
+nova-core
+=========
+
+The nova-core driver is the core driver for NVIDIA GPUs based on GSP. nova-core,
+as the 1st level driver, provides an abstraction around the GPUs hard- and
+firmware interfaces providing a common base for 2nd level drivers, such as the
+vGPU manager VFIO driver and the nova-drm driver.
+
+.. toctree::
+   :titlesonly:
+
+   core/guidelines
+   core/todo
diff --git a/MAINTAINERS b/MAINTAINERS
index 644817ccaa18..f5c7022937a7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7457,6 +7457,7 @@ Q:	https://patchwork.freedesktop.org/project/nouveau/
 B:	https://gitlab.freedesktop.org/drm/nova/-/issues
 C:	irc://irc.oftc.net/nouveau
 T:	git https://gitlab.freedesktop.org/drm/nova.git nova-next
+F:	Documentation/gpu/nova/
 F:	drivers/gpu/nova-core/
 
 DRM DRIVER FOR OLIMEX LCD-OLINUXINO PANELS
-- 
2.48.1

Re: [PATCH v5 1/5] rust: module: add type `LocalModule`

[Jarkko Sakkinen]

On Tue, Mar 04, 2025 at 06:34:48PM +0100, Danilo Krummrich wrote:
> The `LocalModule` type is the type of the module created by `module!`,
> `module_pci_driver!`, `module_platform_driver!`, etc.
> 
> Since the exact type of the module is sometimes generated on the fly by
> the listed macros, provide an alias.
> 
> This is first used by the `module_firmware!` macro introduced in a
> subsequent patch.

So generally speaking for any patches, they are not patches once they
land to the Git and theoretically you cannot presume any order.

So cut out hairs the last sentence should be just:

"The first use for this will be module_firmware!"


> 
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---

You can speak about subsequent patches here but not in the commit
message.

>  rust/macros/module.rs | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> index cdf94f4982df..6ba9210677c5 100644
> --- a/rust/macros/module.rs
> +++ b/rust/macros/module.rs
> @@ -228,6 +228,8 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>                  kernel::ThisModule::from_ptr(core::ptr::null_mut())
>              }};
>  
> +            type LocalModule = {type_};
> +

nit:

I'd document this as:

// The `LocalModule` type is the type of the module created by `module!`,
// `module_pci_driver!`, `module_platform_driver!`, etc.

;-)


>              impl kernel::ModuleMetadata for {type_} {{
>                  const NAME: &'static kernel::str::CStr = kernel::c_str!(\"{name}\");
>              }}
> -- 
> 2.48.1
> 
> 

BR, Jarkko

Re: [PATCH v5 2/5] rust: firmware: introduce `firmware::ModInfoBuilder`

[Jarkko Sakkinen]

On Tue, Mar 04, 2025 at 06:34:49PM +0100, Danilo Krummrich wrote:
> The `firmware` field of the `module!` only accepts literal strings,
> which is due to the fact that it is implemented as a proc macro.
> 
> Some drivers require a lot of firmware files (such as nova-core) and
> hence benefit from more flexibility composing firmware path strings.
> 
> The `firmware::ModInfoBuilder` is a helper component to flexibly compose
> firmware path strings for the .modinfo section in const context.
> 
> It is meant to be used in combination with `kernel::module_firmware!`,
> which is introduced in a subsequent patch.

Ditto.

> 
> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/firmware.rs | 98 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 98 insertions(+)
> 
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> index c5162fdc95ff..6e6972d94597 100644
> --- a/rust/kernel/firmware.rs
> +++ b/rust/kernel/firmware.rs
> @@ -115,3 +115,101 @@ unsafe impl Send for Firmware {}
>  // SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, references to which are safe to
>  // be used from any thread.
>  unsafe impl Sync for Firmware {}
> +
> +/// Builder for firmware module info.
> +///
> +/// [`ModInfoBuilder`] is a helper component to flexibly compose firmware paths strings for the
> +/// .modinfo section in const context.
> +///
> +/// It is meant to be used in combination with [`kernel::module_firmware!`].
> +///
> +/// For more details and an example, see [`kernel::module_firmware!`].
> +pub struct ModInfoBuilder<const N: usize> {
> +    buf: [u8; N],
> +    n: usize,
> +    module_name: &'static CStr,
> +}
> +
> +impl<const N: usize> ModInfoBuilder<N> {
> +    /// Create an empty builder instance.
> +    pub const fn new(module_name: &'static CStr) -> Self {
> +        Self {
> +            buf: [0; N],
> +            n: 0,
> +            module_name,
> +        }
> +    }
> +
> +    const fn push_internal(mut self, bytes: &[u8]) -> Self {
> +        let mut j = 0;
> +
> +        if N == 0 {
> +            self.n += bytes.len();
> +            return self;
> +        }
> +
> +        while j < bytes.len() {
> +            if self.n < N {
> +                self.buf[self.n] = bytes[j];
> +            }
> +            self.n += 1;
> +            j += 1;
> +        }
> +        self
> +    }
> +
> +    /// Push an additional path component.
> +    ///
> +    /// After a new [`ModInfoBuilder`] instance has been created, [`ModInfoBuilder::prepare`] must
> +    /// be called before adding path components.
> +    pub const fn push(self, s: &str) -> Self {
> +        if N != 0 && self.n == 0 {
> +            crate::build_error!("Must call prepare() before push().");
> +        }
> +
> +        self.push_internal(s.as_bytes())
> +    }
> +
> +    const fn prepare_module_name(self) -> Self {
> +        let mut this = self;
> +        let module_name = this.module_name;
> +
> +        if !this.module_name.is_empty() {
> +            this = this.push_internal(module_name.as_bytes_with_nul());
> +
> +            if N != 0 {
> +                // Re-use the space taken by the NULL terminator and swap it with the '.' separator.
> +                this.buf[this.n - 1] = b'.';
> +            }
> +        }
> +
> +        this.push_internal(b"firmware=")
> +    }
> +
> +    /// Prepare for the next module info entry.
> +    ///
> +    /// Must be called before [`ModInfoBuilder::push`] can be called.
> +    pub const fn prepare(self) -> Self {
> +        self.push_internal(b"\0").prepare_module_name()
> +    }
> +
> +    /// Build the byte array.
> +    pub const fn build(self) -> [u8; N] {
> +        // Add the final NULL terminator.
> +        let this = self.push_internal(b"\0");
> +
> +        if this.n == N {
> +            this.buf
> +        } else {
> +            crate::build_error!("Length mismatch.");
> +        }
> +    }
> +}
> +
> +impl ModInfoBuilder<0> {
> +    /// Return the length of the byte array to build.
> +    pub const fn build_length(self) -> usize {
> +        // Compensate for the NULL terminator added by `build`.
> +        self.n + 1
> +    }
> +}
> -- 
> 2.48.1
> 
> 

BR, Jarkko

Re: [PATCH v5 3/5] rust: firmware: add `module_firmware!` macro

[Jarkko Sakkinen]

On Tue, Mar 04, 2025 at 06:34:50PM +0100, Danilo Krummrich wrote:
> Analogous to the `module!` macro `module_firmware!` adds additional
> firmware path strings to the .modinfo section.
> 
> In contrast to `module!`, where path strings need to be string literals,
> path strings can be composed with the `firmware::ModInfoBuilder`.
> 
> Some drivers require a lot of firmware files (such as nova-core) and
> hence benefit from more flexibility composing firmware path strings.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/firmware.rs | 79 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> index 6e6972d94597..5d1ac8287171 100644
> --- a/rust/kernel/firmware.rs
> +++ b/rust/kernel/firmware.rs
> @@ -116,6 +116,85 @@ unsafe impl Send for Firmware {}
>  // be used from any thread.
>  unsafe impl Sync for Firmware {}
>  
> +/// Create firmware .modinfo entries.
> +///
> +/// This macro is the counterpart of the C macro `MODULE_FIRMWARE()`, but instead of taking a
> +/// simple string literals, which is already covered by the `firmware` field of
> +/// [`crate::prelude::module!`], it allows the caller to pass a builder type (e.g.
> +/// [`ModInfoBuilder`]) which can create the firmware modinfo strings in a more flexible way.
> +///
> +/// Drivers should extend the [`ModInfoBuilder`] with their own driver specific builder type.
> +///
> +/// The `builder` argument must be a type which implements the following function.
> +///
> +/// `const fn create(module_name: &'static CStr) -> ModInfoBuilder`
> +///
> +/// `create` should pass the `module_name` to the [`ModInfoBuilder`] and, with the help of
> +/// it construct the corresponding firmware modinfo.
> +///
> +/// Typically, such contracts would be enforced by a trait, however traits do not (yet) support
> +/// const functions.
> +///
> +/// # Example
> +///
> +/// ```
> +/// # mod module_firmware_test {
> +/// # use kernel::firmware;
> +/// # use kernel::prelude::*;
> +/// #
> +/// # struct MyModule;
> +/// #
> +/// # impl kernel::Module for MyModule {
> +/// #     fn init(_module: &'static ThisModule) -> Result<Self> {
> +/// #         Ok(Self)
> +/// #     }
> +/// # }
> +/// #
> +/// #
> +/// struct Builder<const N: usize>;
> +///
> +/// impl<const N: usize> Builder<N> {
> +///     const fn create(module_name: &'static kernel::str::CStr) -> firmware::ModInfoBuilder<N> {
> +///         firmware::ModInfoBuilder::new(module_name)
> +///             .prepare()
> +///             .push("vendor/foo.bin")
> +///             .prepare()
> +///             .push("vendor/bar.bin")
> +///     }
> +/// }
> +///
> +/// module! {
> +///    type: MyModule,
> +///    name: "module_firmware_test",
> +///    author: "Rust for Linux",
> +///    description: "module_firmware! test module",
> +///    license: "GPL",
> +/// }
> +///
> +/// kernel::module_firmware!(Builder);
> +/// # }
> +/// ```
> +#[macro_export]
> +macro_rules! module_firmware {
> +    ($($builder:tt)*) => {
> +
> +        #[cfg(not(MODULE))]
> +        const fn __module_name() -> &'static kernel::str::CStr {
> +            <LocalModule as kernel::ModuleMetadata>::NAME
> +        }
> +
> +        #[cfg(MODULE)]
> +        const fn __module_name() -> &'static kernel::str::CStr {
> +            kernel::c_str!("")
> +        }
> +
> +        #[link_section = ".modinfo"]
> +        #[used]
> +        static __MODULE_FIRMWARE: [u8; $($builder)*::create(__module_name()).build_length()] =
> +            $($builder)*::create(__module_name()).build();
> +    };
> +}
> +
>  /// Builder for firmware module info.
>  ///
>  /// [`ModInfoBuilder`] is a helper component to flexibly compose firmware paths strings for the
> -- 
> 2.48.1
> 
> 
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

Re: [PATCH v5 0/5] Initial Nova Core series

[Danilo Krummrich]

On Tue, Mar 04, 2025 at 06:34:47PM +0100, Danilo Krummrich wrote:
> Danilo Krummrich (5):
>   rust: module: add type `LocalModule`
>   rust: firmware: introduce `firmware::ModInfoBuilder`
>   rust: firmware: add `module_firmware!` macro

Greg, Luis, Russ, any objections on me taking the two firmware patches through
the nova tree?

>   gpu: nova-core: add initial driver stub
>   gpu: nova-core: add initial documentation

Re: [PATCH v5 1/5] rust: module: add type `LocalModule`

[Miguel Ojeda]

On Tue, Mar 4, 2025 at 6:36 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> The `LocalModule` type is the type of the module created by `module!`,
> `module_pci_driver!`, `module_platform_driver!`, etc.
>
> Since the exact type of the module is sometimes generated on the fly by
> the listed macros, provide an alias.
>
> This is first used by the `module_firmware!` macro introduced in a
> subsequent patch.
>
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Acked-by: Miguel Ojeda <ojeda@kernel.org>

Cheers,
Miguel

Re: [PATCH v5 2/5] rust: firmware: introduce `firmware::ModInfoBuilder`

[Benno Lossin]

On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote:
> The `firmware` field of the `module!` only accepts literal strings,
> which is due to the fact that it is implemented as a proc macro.
>
> Some drivers require a lot of firmware files (such as nova-core) and
> hence benefit from more flexibility composing firmware path strings.
>
> The `firmware::ModInfoBuilder` is a helper component to flexibly compose
> firmware path strings for the .modinfo section in const context.
>
> It is meant to be used in combination with `kernel::module_firmware!`,
> which is introduced in a subsequent patch.
>
> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/firmware.rs | 98 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 98 insertions(+)
>
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> index c5162fdc95ff..6e6972d94597 100644
> --- a/rust/kernel/firmware.rs
> +++ b/rust/kernel/firmware.rs
> @@ -115,3 +115,101 @@ unsafe impl Send for Firmware {}
>  // SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, references to which are safe to
>  // be used from any thread.
>  unsafe impl Sync for Firmware {}
> +
> +/// Builder for firmware module info.
> +///
> +/// [`ModInfoBuilder`] is a helper component to flexibly compose firmware paths strings for the
> +/// .modinfo section in const context.
> +///
> +/// It is meant to be used in combination with [`kernel::module_firmware!`].
> +///
> +/// For more details and an example, see [`kernel::module_firmware!`].
> +pub struct ModInfoBuilder<const N: usize> {
> +    buf: [u8; N],
> +    n: usize,
> +    module_name: &'static CStr,
> +}
> +
> +impl<const N: usize> ModInfoBuilder<N> {
> +    /// Create an empty builder instance.
> +    pub const fn new(module_name: &'static CStr) -> Self {
> +        Self {
> +            buf: [0; N],
> +            n: 0,
> +            module_name,
> +        }
> +    }
> +
> +    const fn push_internal(mut self, bytes: &[u8]) -> Self {
> +        let mut j = 0;
> +
> +        if N == 0 {
> +            self.n += bytes.len();
> +            return self;
> +        }
> +
> +        while j < bytes.len() {
> +            if self.n < N {
> +                self.buf[self.n] = bytes[j];
> +            }
> +            self.n += 1;
> +            j += 1;
> +        }
> +        self
> +    }
> +
> +    /// Push an additional path component.
> +    ///
> +    /// After a new [`ModInfoBuilder`] instance has been created, [`ModInfoBuilder::prepare`] must
> +    /// be called before adding path components.
> +    pub const fn push(self, s: &str) -> Self {
> +        if N != 0 && self.n == 0 {
> +            crate::build_error!("Must call prepare() before push().");

This will only prevent the first `prepare` call being missed, right?

> +        }
> +
> +        self.push_internal(s.as_bytes())
> +    }
> +
> +    const fn prepare_module_name(self) -> Self {
> +        let mut this = self;
> +        let module_name = this.module_name;
> +
> +        if !this.module_name.is_empty() {
> +            this = this.push_internal(module_name.as_bytes_with_nul());
> +
> +            if N != 0 {
> +                // Re-use the space taken by the NULL terminator and swap it with the '.' separator.
> +                this.buf[this.n - 1] = b'.';
> +            }
> +        }
> +
> +        this.push_internal(b"firmware=")
> +    }
> +
> +    /// Prepare for the next module info entry.
> +    ///
> +    /// Must be called before [`ModInfoBuilder::push`] can be called.

If you always have to call this before `push`, why not inline it there?

---
Cheers,
Benno

> +    pub const fn prepare(self) -> Self {
> +        self.push_internal(b"\0").prepare_module_name()
> +    }
> +
> +    /// Build the byte array.
> +    pub const fn build(self) -> [u8; N] {
> +        // Add the final NULL terminator.
> +        let this = self.push_internal(b"\0");
> +
> +        if this.n == N {
> +            this.buf
> +        } else {
> +            crate::build_error!("Length mismatch.");
> +        }
> +    }
> +}
> +
> +impl ModInfoBuilder<0> {
> +    /// Return the length of the byte array to build.
> +    pub const fn build_length(self) -> usize {
> +        // Compensate for the NULL terminator added by `build`.
> +        self.n + 1
> +    }
> +}
> --
> 2.48.1

Re: [PATCH v5 2/5] rust: firmware: introduce `firmware::ModInfoBuilder`

[Danilo Krummrich]

On Wed, Mar 05, 2025 at 10:30:31PM +0000, Benno Lossin wrote:
> On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote:
> > The `firmware` field of the `module!` only accepts literal strings,
> > which is due to the fact that it is implemented as a proc macro.
> >
> > Some drivers require a lot of firmware files (such as nova-core) and
> > hence benefit from more flexibility composing firmware path strings.
> >
> > The `firmware::ModInfoBuilder` is a helper component to flexibly compose
> > firmware path strings for the .modinfo section in const context.
> >
> > It is meant to be used in combination with `kernel::module_firmware!`,
> > which is introduced in a subsequent patch.
> >
> > Co-developed-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  rust/kernel/firmware.rs | 98 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 98 insertions(+)
> >
> > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> > index c5162fdc95ff..6e6972d94597 100644
> > --- a/rust/kernel/firmware.rs
> > +++ b/rust/kernel/firmware.rs
> > @@ -115,3 +115,101 @@ unsafe impl Send for Firmware {}
> >  // SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, references to which are safe to
> >  // be used from any thread.
> >  unsafe impl Sync for Firmware {}
> > +
> > +/// Builder for firmware module info.
> > +///
> > +/// [`ModInfoBuilder`] is a helper component to flexibly compose firmware paths strings for the
> > +/// .modinfo section in const context.
> > +///
> > +/// It is meant to be used in combination with [`kernel::module_firmware!`].
> > +///
> > +/// For more details and an example, see [`kernel::module_firmware!`].
> > +pub struct ModInfoBuilder<const N: usize> {
> > +    buf: [u8; N],
> > +    n: usize,
> > +    module_name: &'static CStr,
> > +}
> > +
> > +impl<const N: usize> ModInfoBuilder<N> {
> > +    /// Create an empty builder instance.
> > +    pub const fn new(module_name: &'static CStr) -> Self {
> > +        Self {
> > +            buf: [0; N],
> > +            n: 0,
> > +            module_name,
> > +        }
> > +    }
> > +
> > +    const fn push_internal(mut self, bytes: &[u8]) -> Self {
> > +        let mut j = 0;
> > +
> > +        if N == 0 {
> > +            self.n += bytes.len();
> > +            return self;
> > +        }
> > +
> > +        while j < bytes.len() {
> > +            if self.n < N {
> > +                self.buf[self.n] = bytes[j];
> > +            }
> > +            self.n += 1;
> > +            j += 1;
> > +        }
> > +        self
> > +    }
> > +
> > +    /// Push an additional path component.
> > +    ///
> > +    /// After a new [`ModInfoBuilder`] instance has been created, [`ModInfoBuilder::prepare`] must
> > +    /// be called before adding path components.
> > +    pub const fn push(self, s: &str) -> Self {
> > +        if N != 0 && self.n == 0 {
> > +            crate::build_error!("Must call prepare() before push().");
> 
> This will only prevent the first `prepare` call being missed, right?

Correct, unfortunately there's no way to detect subsequent ones.

> 
> > +        }
> > +
> > +        self.push_internal(s.as_bytes())
> > +    }
> > +
> > +    const fn prepare_module_name(self) -> Self {
> > +        let mut this = self;
> > +        let module_name = this.module_name;
> > +
> > +        if !this.module_name.is_empty() {
> > +            this = this.push_internal(module_name.as_bytes_with_nul());
> > +
> > +            if N != 0 {
> > +                // Re-use the space taken by the NULL terminator and swap it with the '.' separator.
> > +                this.buf[this.n - 1] = b'.';
> > +            }
> > +        }
> > +
> > +        this.push_internal(b"firmware=")
> > +    }
> > +
> > +    /// Prepare for the next module info entry.
> > +    ///
> > +    /// Must be called before [`ModInfoBuilder::push`] can be called.
> 
> If you always have to call this before `push`, why not inline it there?

You can push() multiple times to compose the firmware path string (which is the
whole purpose :).

Example from nova-core:

	pub(crate) struct ModInfoBuilder<const N: usize>(firmware::ModInfoBuilder<N>);
	
	impl<const N: usize> ModInfoBuilder<N> {
	    const fn make_entry_file(self, chipset: &str, fw: &str) -> Self {
	        let version = "535.113.01";
	
	        ModInfoBuilder(
	            self.0
	                .prepare()
	                .push("nvidia/")
	                .push(chipset)
	                .push("/gsp/")
	                .push(fw)
	                .push("-")
	                .push(version)
	                .push(".bin"),
	        )
	    }
	
	    const fn make_entry_chipset(self, chipset: &str) -> Self {
	        self.make_entry_file(chipset, "booter_load")
	            .make_entry_file(chipset, "booter_unload")
	            .make_entry_file(chipset, "bootloader")
	            .make_entry_file(chipset, "gsp")
	    }
	
	    pub(crate) const fn create(
	        module_name: &'static kernel::str::CStr,
	    ) -> firmware::ModInfoBuilder<N> {
	        let mut this = Self(firmware::ModInfoBuilder::new(module_name));
	        let mut i = 0;
	
	        while i < gpu::Chipset::NAMES.len() {
	            this = this.make_entry_chipset(gpu::Chipset::NAMES[i]);
	            i += 1;
	        }
	
	        this.0
	    }
	}

Re: [PATCH v5 0/5] Initial Nova Core series

[Luis Chamberlain]

On Wed, Mar 05, 2025 at 08:56:42PM +0100, Danilo Krummrich wrote:
> On Tue, Mar 04, 2025 at 06:34:47PM +0100, Danilo Krummrich wrote:
> > Danilo Krummrich (5):
> >   rust: module: add type `LocalModule`
> >   rust: firmware: introduce `firmware::ModInfoBuilder`
> >   rust: firmware: add `module_firmware!` macro
> 
> Greg, Luis, Russ, any objections on me taking the two firmware patches through
> the nova tree?

I don't speak Rust so I'd my recommendation would be to add the rust
firmware file under the firmware loader entry for maintainers provided
we get a volunteer from the rust community do help maintain *both* C and
the Rust version of the firmware loader.

  Luis

Re: [PATCH v5 2/5] rust: firmware: introduce `firmware::ModInfoBuilder`

[Benno Lossin]

On Wed Mar 5, 2025 at 11:38 PM CET, Danilo Krummrich wrote:
> On Wed, Mar 05, 2025 at 10:30:31PM +0000, Benno Lossin wrote:
>> On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote:
>> > +    /// Push an additional path component.
>> > +    ///
>> > +    /// After a new [`ModInfoBuilder`] instance has been created, [`ModInfoBuilder::prepare`] must
>> > +    /// be called before adding path components.
>> > +    pub const fn push(self, s: &str) -> Self {
>> > +        if N != 0 && self.n == 0 {
>> > +            crate::build_error!("Must call prepare() before push().");
>>
>> This will only prevent the first `prepare` call being missed, right?
>
> Correct, unfortunately there's no way to detect subsequent ones.

Does it make sense to do that one in the constructor?

(After looking at the example below) Ah maybe you can't do that, since
then you would have two `prepare()` calls for the example below...?

>> > +        }
>> > +
>> > +        self.push_internal(s.as_bytes())
>> > +    }
>> > +
>> > +    const fn prepare_module_name(self) -> Self {
>> > +        let mut this = self;
>> > +        let module_name = this.module_name;
>> > +
>> > +        if !this.module_name.is_empty() {
>> > +            this = this.push_internal(module_name.as_bytes_with_nul());
>> > +
>> > +            if N != 0 {
>> > +                // Re-use the space taken by the NULL terminator and swap it with the '.' separator.
>> > +                this.buf[this.n - 1] = b'.';
>> > +            }
>> > +        }
>> > +
>> > +        this.push_internal(b"firmware=")
>> > +    }
>> > +
>> > +    /// Prepare for the next module info entry.
>> > +    ///
>> > +    /// Must be called before [`ModInfoBuilder::push`] can be called.
>>
>> If you always have to call this before `push`, why not inline it there?
>
> You can push() multiple times to compose the firmware path string (which is the
> whole purpose :).

Ah I see, I only looked at the example you have in the next patch. All
in all, I think this patch could use some better documentation, since I
had to read a lot of the code to understand what everything is supposed
to do...

It might also make sense to make this more generic, since one probably
also needs this in other places? Or do you think this will only be
required for modinfo?

---
Cheers,
Benno

> Example from nova-core:
>
> 	pub(crate) struct ModInfoBuilder<const N: usize>(firmware::ModInfoBuilder<N>);
>
> 	impl<const N: usize> ModInfoBuilder<N> {
> 	    const fn make_entry_file(self, chipset: &str, fw: &str) -> Self {
> 	        let version = "535.113.01";
>
> 	        ModInfoBuilder(
> 	            self.0
> 	                .prepare()
> 	                .push("nvidia/")
> 	                .push(chipset)
> 	                .push("/gsp/")
> 	                .push(fw)
> 	                .push("-")
> 	                .push(version)
> 	                .push(".bin"),
> 	        )
> 	    }
>
> 	    const fn make_entry_chipset(self, chipset: &str) -> Self {
> 	        self.make_entry_file(chipset, "booter_load")
> 	            .make_entry_file(chipset, "booter_unload")
> 	            .make_entry_file(chipset, "bootloader")
> 	            .make_entry_file(chipset, "gsp")
> 	    }
>
> 	    pub(crate) const fn create(
> 	        module_name: &'static kernel::str::CStr,
> 	    ) -> firmware::ModInfoBuilder<N> {
> 	        let mut this = Self(firmware::ModInfoBuilder::new(module_name));
> 	        let mut i = 0;
>
> 	        while i < gpu::Chipset::NAMES.len() {
> 	            this = this.make_entry_chipset(gpu::Chipset::NAMES[i]);
> 	            i += 1;
> 	        }
>
> 	        this.0
> 	    }
> 	}

Re: [PATCH v5 0/5] Initial Nova Core series

[Danilo Krummrich]

On Wed, Mar 05, 2025 at 03:27:13PM -0800, Luis Chamberlain wrote:
> On Wed, Mar 05, 2025 at 08:56:42PM +0100, Danilo Krummrich wrote:
> > On Tue, Mar 04, 2025 at 06:34:47PM +0100, Danilo Krummrich wrote:
> > > Danilo Krummrich (5):
> > >   rust: module: add type `LocalModule`
> > >   rust: firmware: introduce `firmware::ModInfoBuilder`
> > >   rust: firmware: add `module_firmware!` macro
> > 
> > Greg, Luis, Russ, any objections on me taking the two firmware patches through
> > the nova tree?
> 
> I don't speak Rust so I'd my recommendation would be to add the rust
> firmware file under the firmware loader entry for maintainers provided
> we get a volunteer from the rust community do help maintain *both* C and
> the Rust version of the firmware loader.

Yeah, you suggested that when I sent the first firmware loader abstraction more
than half a year ago and since I'm doing exactly that. :-)

Re: [PATCH v5 2/5] rust: firmware: introduce `firmware::ModInfoBuilder`

[Danilo Krummrich]

On Wed, Mar 05, 2025 at 11:36:54PM +0000, Benno Lossin wrote:
> On Wed Mar 5, 2025 at 11:38 PM CET, Danilo Krummrich wrote:
> > On Wed, Mar 05, 2025 at 10:30:31PM +0000, Benno Lossin wrote:
> >> On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote:
> >> > +    /// Push an additional path component.
> >> > +    ///
> >> > +    /// After a new [`ModInfoBuilder`] instance has been created, [`ModInfoBuilder::prepare`] must
> >> > +    /// be called before adding path components.
> >> > +    pub const fn push(self, s: &str) -> Self {
> >> > +        if N != 0 && self.n == 0 {
> >> > +            crate::build_error!("Must call prepare() before push().");
> >>
> >> This will only prevent the first `prepare` call being missed, right?
> >
> > Correct, unfortunately there's no way to detect subsequent ones.
> 
> Does it make sense to do that one in the constructor?
> 
> (After looking at the example below) Ah maybe you can't do that, since
> then you would have two `prepare()` calls for the example below...?

Exactly.

> >> If you always have to call this before `push`, why not inline it there?
> >
> > You can push() multiple times to compose the firmware path string (which is the
> > whole purpose :).
> 
> Ah I see, I only looked at the example you have in the next patch. All
> in all, I think this patch could use some better documentation, since I
> had to read a lot of the code to understand what everything is supposed
> to do...

I can expand the example in module_firmware! to make things a bit more obvious.

Otherwise, what information do you think is missing?

> 
> It might also make sense to make this more generic, since one probably
> also needs this in other places? Or do you think this will only be
> required for modinfo?

Currently, I don't think there's any more need for a generic const string
builder. For now, I think we're good. Let's factor it out, once we have actual
need for that.

Re: [PATCH v5 0/5] Initial Nova Core series

[Luis Chamberlain]

On Thu, Mar 06, 2025 at 12:40:44AM +0100, Danilo Krummrich wrote:
> On Wed, Mar 05, 2025 at 03:27:13PM -0800, Luis Chamberlain wrote:
> > On Wed, Mar 05, 2025 at 08:56:42PM +0100, Danilo Krummrich wrote:
> > > On Tue, Mar 04, 2025 at 06:34:47PM +0100, Danilo Krummrich wrote:
> > > > Danilo Krummrich (5):
> > > >   rust: module: add type `LocalModule`
> > > >   rust: firmware: introduce `firmware::ModInfoBuilder`
> > > >   rust: firmware: add `module_firmware!` macro
> > > 
> > > Greg, Luis, Russ, any objections on me taking the two firmware patches through
> > > the nova tree?
> > 
> > I don't speak Rust so I'd my recommendation would be to add the rust
> > firmware file under the firmware loader entry for maintainers provided
> > we get a volunteer from the rust community do help maintain *both* C and
> > the Rust version of the firmware loader.
> 
> Yeah, you suggested that when I sent the first firmware loader abstraction more
> than half a year ago and since I'm doing exactly that. :-)

Great, it sounds like we have a firmware loader maintainer volunteer!

 Luis

Re: [PATCH v5 2/5] rust: firmware: introduce `firmware::ModInfoBuilder`

[Benno Lossin]

On Thu Mar 6, 2025 at 12:57 AM CET, Danilo Krummrich wrote:
> On Wed, Mar 05, 2025 at 11:36:54PM +0000, Benno Lossin wrote:
>> On Wed Mar 5, 2025 at 11:38 PM CET, Danilo Krummrich wrote:
>> > On Wed, Mar 05, 2025 at 10:30:31PM +0000, Benno Lossin wrote:
>> >> On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote:
>> >> > +    /// Push an additional path component.
>> >> > +    ///
>> >> > +    /// After a new [`ModInfoBuilder`] instance has been created, [`ModInfoBuilder::prepare`] must
>> >> > +    /// be called before adding path components.
>> >> > +    pub const fn push(self, s: &str) -> Self {
>> >> > +        if N != 0 && self.n == 0 {
>> >> > +            crate::build_error!("Must call prepare() before push().");
>> >>
>> >> This will only prevent the first `prepare` call being missed, right?
>> >
>> > Correct, unfortunately there's no way to detect subsequent ones.
>>
>> Does it make sense to do that one in the constructor?
>>
>> (After looking at the example below) Ah maybe you can't do that, since
>> then you would have two `prepare()` calls for the example below...?
>
> Exactly.
>
>> >> If you always have to call this before `push`, why not inline it there?
>> >
>> > You can push() multiple times to compose the firmware path string (which is the
>> > whole purpose :).
>>
>> Ah I see, I only looked at the example you have in the next patch. All
>> in all, I think this patch could use some better documentation, since I
>> had to read a lot of the code to understand what everything is supposed
>> to do...
>
> I can expand the example in module_firmware! to make things a bit more obvious.
>
> Otherwise, what information do you think is missing?

Abstractly: what `ModInfoBuilder` *does*, concretely:
- why the generic constant `N` exists,
- what `prepare()` does,
- what happens with the `module_name` parameter of `new`
- answer the question "I want that the builder outputs the string `???`
  can it do that? If yes, how do I do it?"

>> It might also make sense to make this more generic, since one probably
>> also needs this in other places? Or do you think this will only be
>> required for modinfo?
>
> Currently, I don't think there's any more need for a generic const string
> builder. For now, I think we're good. Let's factor it out, once we have actual
> need for that.

Sounds good.

---
Cheers,
Benno

Re: [PATCH v5 3/5] rust: firmware: add `module_firmware!` macro

[Benno Lossin]

On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote:
> Analogous to the `module!` macro `module_firmware!` adds additional
> firmware path strings to the .modinfo section.
>
> In contrast to `module!`, where path strings need to be string literals,
> path strings can be composed with the `firmware::ModInfoBuilder`.
>
> Some drivers require a lot of firmware files (such as nova-core) and
> hence benefit from more flexibility composing firmware path strings.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/firmware.rs | 79 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
>
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> index 6e6972d94597..5d1ac8287171 100644
> --- a/rust/kernel/firmware.rs
> +++ b/rust/kernel/firmware.rs
> @@ -116,6 +116,85 @@ unsafe impl Send for Firmware {}
>  // be used from any thread.
>  unsafe impl Sync for Firmware {}
>
> +/// Create firmware .modinfo entries.
> +///
> +/// This macro is the counterpart of the C macro `MODULE_FIRMWARE()`, but instead of taking a
> +/// simple string literals, which is already covered by the `firmware` field of
> +/// [`crate::prelude::module!`], it allows the caller to pass a builder type (e.g.
> +/// [`ModInfoBuilder`]) which can create the firmware modinfo strings in a more flexible way.
> +///
> +/// Drivers should extend the [`ModInfoBuilder`] with their own driver specific builder type.
> +///
> +/// The `builder` argument must be a type which implements the following function.
> +///
> +/// `const fn create(module_name: &'static CStr) -> ModInfoBuilder`
> +///
> +/// `create` should pass the `module_name` to the [`ModInfoBuilder`] and, with the help of
> +/// it construct the corresponding firmware modinfo.
> +///
> +/// Typically, such contracts would be enforced by a trait, however traits do not (yet) support
> +/// const functions.
> +///
> +/// # Example
> +///
> +/// ```
> +/// # mod module_firmware_test {
> +/// # use kernel::firmware;
> +/// # use kernel::prelude::*;
> +/// #
> +/// # struct MyModule;
> +/// #
> +/// # impl kernel::Module for MyModule {
> +/// #     fn init(_module: &'static ThisModule) -> Result<Self> {
> +/// #         Ok(Self)
> +/// #     }
> +/// # }
> +/// #
> +/// #
> +/// struct Builder<const N: usize>;
> +///
> +/// impl<const N: usize> Builder<N> {
> +///     const fn create(module_name: &'static kernel::str::CStr) -> firmware::ModInfoBuilder<N> {
> +///         firmware::ModInfoBuilder::new(module_name)
> +///             .prepare()
> +///             .push("vendor/foo.bin")
> +///             .prepare()
> +///             .push("vendor/bar.bin")
> +///     }
> +/// }
> +///
> +/// module! {
> +///    type: MyModule,
> +///    name: "module_firmware_test",
> +///    author: "Rust for Linux",
> +///    description: "module_firmware! test module",
> +///    license: "GPL",
> +/// }
> +///
> +/// kernel::module_firmware!(Builder);
> +/// # }
> +/// ```

Would be nice to see a more complex example here like the one from nova
you sent in the other thread. So with "dynamic" string interpolation and
multiple pushes.

> +#[macro_export]
> +macro_rules! module_firmware {
> +    ($($builder:tt)*) => {

This should probably be `$builder:expr` instead.

> +
> +        #[cfg(not(MODULE))]
> +        const fn __module_name() -> &'static kernel::str::CStr {
> +            <LocalModule as kernel::ModuleMetadata>::NAME

Please either use `::kernel::` or `$crate::` instead of `kernel::`.

Hmm, I am not 100% comfortable with the `LocalModule` way of accessing
the current module for some reason, no idea if there is a rational
argument behind that, but it just doesn't sit right with me.

Essentially you're doing this for convenience, right? So you don't want
to have to repeat the name of the module type every time?

> +        }
> +
> +        #[cfg(MODULE)]
> +        const fn __module_name() -> &'static kernel::str::CStr {
> +            kernel::c_str!("")

Ditto.

> +        }

Are these two functions used outside of the `static` below? If no, then
you can just move them into the static? You can also probably use a
`const` instead of a function, that way you only have 4 lines instead
of 8.

---
Cheers,
Benno

> +
> +        #[link_section = ".modinfo"]
> +        #[used]
> +        static __MODULE_FIRMWARE: [u8; $($builder)*::create(__module_name()).build_length()] =
> +            $($builder)*::create(__module_name()).build();
> +    };
> +}
> +
>  /// Builder for firmware module info.
>  ///
>  /// [`ModInfoBuilder`] is a helper component to flexibly compose firmware paths strings for the
> --
> 2.48.1

Re: [PATCH v5 3/5] rust: firmware: add `module_firmware!` macro

[Danilo Krummrich]

On Thu, Mar 06, 2025 at 12:31:14AM +0000, Benno Lossin wrote:
> On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote:
> 
> > +#[macro_export]
> > +macro_rules! module_firmware {
> > +    ($($builder:tt)*) => {
> 
> This should probably be `$builder:expr` instead.

That doesn't work, the compiler then complains, since it's not an expression:

193  |         static __MODULE_FIRMWARE: [u8; $builder::create(__module_name()).build_length()] =
     |                                                ^^ expected one of `.`, `?`, `]`, or an operator

`ty` doesn't work either, since then the compiler expects the caller to add the
const generic, which we want the macro to figure out instead.

> 
> > +
> > +        #[cfg(not(MODULE))]
> > +        const fn __module_name() -> &'static kernel::str::CStr {
> > +            <LocalModule as kernel::ModuleMetadata>::NAME
> 
> Please either use `::kernel::` or `$crate::` instead of `kernel::`.

Good catch, thanks.

> 
> Hmm, I am not 100% comfortable with the `LocalModule` way of accessing
> the current module for some reason, no idea if there is a rational
> argument behind that, but it just doesn't sit right with me.
> 
> Essentially you're doing this for convenience, right? So you don't want
> to have to repeat the name of the module type every time?

No, it's really that I can't know the type name here, please see the previous
patch commit message that introduces `LocalModule` for explanation.

> 
> > +        }
> > +
> > +        #[cfg(MODULE)]
> > +        const fn __module_name() -> &'static kernel::str::CStr {
> > +            kernel::c_str!("")
> 
> Ditto.
> 
> > +        }
> 
> Are these two functions used outside of the `static` below? If no, then
> you can just move them into the static? You can also probably use a
> `const` instead of a function, that way you only have 4 lines instead
> of 8.

Is this what you're proposing?

	#[macro_export]
	macro_rules! module_firmware {
	    ($($builder:tt)*) => {
	        const __MODULE_FIRMWARE_PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) {
	            $crate::c_str!("")
	        } else {
	            <LocalModule as $crate::ModuleMetadata>::NAME
	        };
	
	        #[link_section = ".modinfo"]
	        #[used]
	        static __MODULE_FIRMWARE: [u8; $($builder)*::create(__MODULE_FIRMWARE_PREFIX)
	            .build_length()] = $($builder)*::create(__MODULE_FIRMWARE_PREFIX).build();
	    };
	}

Re: [PATCH v5 3/5] rust: firmware: add `module_firmware!` macro

[Benno Lossin]

On Thu Mar 6, 2025 at 2:04 AM CET, Danilo Krummrich wrote:
> On Thu, Mar 06, 2025 at 12:31:14AM +0000, Benno Lossin wrote:
>> On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote:
>>
>> > +#[macro_export]
>> > +macro_rules! module_firmware {
>> > +    ($($builder:tt)*) => {
>>
>> This should probably be `$builder:expr` instead.
>
> That doesn't work, the compiler then complains, since it's not an expression:
>
> 193  |         static __MODULE_FIRMWARE: [u8; $builder::create(__module_name()).build_length()] =
>      |                                                ^^ expected one of `.`, `?`, `]`, or an operator

Does `<$builder>::create` work (with the `expr` fragment)?

> `ty` doesn't work either, since then the compiler expects the caller to add the
> const generic, which we want the macro to figure out instead.
>
>>
>> > +
>> > +        #[cfg(not(MODULE))]
>> > +        const fn __module_name() -> &'static kernel::str::CStr {
>> > +            <LocalModule as kernel::ModuleMetadata>::NAME
>>
>> Please either use `::kernel::` or `$crate::` instead of `kernel::`.
>
> Good catch, thanks.
>
>>
>> Hmm, I am not 100% comfortable with the `LocalModule` way of accessing
>> the current module for some reason, no idea if there is a rational
>> argument behind that, but it just doesn't sit right with me.
>>
>> Essentially you're doing this for convenience, right? So you don't want
>> to have to repeat the name of the module type every time?
>
> No, it's really that I can't know the type name here, please see the previous
> patch commit message that introduces `LocalModule` for explanation.

Gotcha.

>> > +        }
>> > +
>> > +        #[cfg(MODULE)]
>> > +        const fn __module_name() -> &'static kernel::str::CStr {
>> > +            kernel::c_str!("")
>>
>> Ditto.
>>
>> > +        }
>>
>> Are these two functions used outside of the `static` below? If no, then
>> you can just move them into the static? You can also probably use a
>> `const` instead of a function, that way you only have 4 lines instead
>> of 8.
>
> Is this what you're proposing?
>
> 	#[macro_export]
> 	macro_rules! module_firmware {
> 	    ($($builder:tt)*) => {
> 	        const __MODULE_FIRMWARE_PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) {
> 	            $crate::c_str!("")
> 	        } else {
> 	            <LocalModule as $crate::ModuleMetadata>::NAME
> 	        };
>
> 	        #[link_section = ".modinfo"]
> 	        #[used]
> 	        static __MODULE_FIRMWARE: [u8; $($builder)*::create(__MODULE_FIRMWARE_PREFIX)
> 	            .build_length()] = $($builder)*::create(__MODULE_FIRMWARE_PREFIX).build();

I meant to also move the `const` into the expression, but I guess that
leads to duplication:

    #[link_section = ".modinfo"]
    #[used]
    static __MODULE_FIRMWARE: [u8; {
        const PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) {
            $crate::c_str!("")
        } else {
            <LocalModule as $crate::ModuleMetadata>::NAME
        };
        <$builder>::create(PREFIX).build_length()
    }] = {
        const PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) {
            $crate::c_str!("")
        } else {
            <LocalModule as $crate::ModuleMetadata>::NAME
        };
        <$builder>::create(PREFIX)
    };

But then the advantage is that only the `__MODULE_FIRMWARE` static will
be in-scope.

Do you think that its useful to have the static be accessible? I.e. do
users need to access it (I would think they don't)? If they don't, then
we could put all of those things into a `const _: () = { /* ... */ };`.
But then people can invoke `module_firmware!` multiple times in the same
module, is that a problem?

---
Cheers,
Benno

> 	    };
> 	}

Re: [PATCH v5 2/5] rust: firmware: introduce `firmware::ModInfoBuilder`

[Danilo Krummrich]

On Thu, Mar 06, 2025 at 12:24:21AM +0000, Benno Lossin wrote:
> On Thu Mar 6, 2025 at 12:57 AM CET, Danilo Krummrich wrote:
> > On Wed, Mar 05, 2025 at 11:36:54PM +0000, Benno Lossin wrote:
> >> On Wed Mar 5, 2025 at 11:38 PM CET, Danilo Krummrich wrote:
> >> > On Wed, Mar 05, 2025 at 10:30:31PM +0000, Benno Lossin wrote:
> >> >> On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote:
> >> >> > +    /// Push an additional path component.
> >> >> > +    ///
> >> >> > +    /// After a new [`ModInfoBuilder`] instance has been created, [`ModInfoBuilder::prepare`] must
> >> >> > +    /// be called before adding path components.
> >> >> > +    pub const fn push(self, s: &str) -> Self {
> >> >> > +        if N != 0 && self.n == 0 {
> >> >> > +            crate::build_error!("Must call prepare() before push().");
> >> >>
> >> >> This will only prevent the first `prepare` call being missed, right?
> >> >
> >> > Correct, unfortunately there's no way to detect subsequent ones.
> >>
> >> Does it make sense to do that one in the constructor?
> >>
> >> (After looking at the example below) Ah maybe you can't do that, since
> >> then you would have two `prepare()` calls for the example below...?
> >
> > Exactly.
> >
> >> >> If you always have to call this before `push`, why not inline it there?
> >> >
> >> > You can push() multiple times to compose the firmware path string (which is the
> >> > whole purpose :).
> >>
> >> Ah I see, I only looked at the example you have in the next patch. All
> >> in all, I think this patch could use some better documentation, since I
> >> had to read a lot of the code to understand what everything is supposed
> >> to do...
> >
> > I can expand the example in module_firmware! to make things a bit more obvious.
> >
> > Otherwise, what information do you think is missing?
> 
> Abstractly: what `ModInfoBuilder` *does*, concretely:
> - why the generic constant `N` exists,

It doesn't really matter to the user, since the user never needs to supply it.
That happens in the module_firmware! macro.

I agree it not good to not mention anything about it at all, but I wouldn't want
to bother the user with all implemention details.

We can probably just mention that it's used internally and is supplied by
module_firmware!. (That module_firmware! does that by doing a dry run of the
builder itself, isn't necessary to know for the user I think.)

> - what `prepare()` does,

Same here, it's an implementation detail not relevant to the user. All the user
needs to know is that prepare() acts as a separator to be able to supply the
next firmware path.

> - what happens with the `module_name` parameter of `new`

Should probably just mention it's supplied by module_firmware! and used
internally.

> - answer the question "I want that the builder outputs the string `???`
>   can it do that? If yes, how do I do it?"

All it does is concatenating multiple &str in const context, which I thought is
clear since there are only push() and prepare() as public methods.

May it be that your request is more about can we add more hints on the
implementation details rather than user focused documentation?

Re: [PATCH v5 2/5] rust: firmware: introduce `firmware::ModInfoBuilder`

[Benno Lossin]

On Thu Mar 6, 2025 at 2:29 AM CET, Danilo Krummrich wrote:
> On Thu, Mar 06, 2025 at 12:24:21AM +0000, Benno Lossin wrote:
>> On Thu Mar 6, 2025 at 12:57 AM CET, Danilo Krummrich wrote:
>> > On Wed, Mar 05, 2025 at 11:36:54PM +0000, Benno Lossin wrote:
>> >> On Wed Mar 5, 2025 at 11:38 PM CET, Danilo Krummrich wrote:
>> >> > On Wed, Mar 05, 2025 at 10:30:31PM +0000, Benno Lossin wrote:
>> >> >> On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote:
>> >> >> > +    /// Push an additional path component.
>> >> >> > +    ///
>> >> >> > +    /// After a new [`ModInfoBuilder`] instance has been created, [`ModInfoBuilder::prepare`] must
>> >> >> > +    /// be called before adding path components.
>> >> >> > +    pub const fn push(self, s: &str) -> Self {
>> >> >> > +        if N != 0 && self.n == 0 {
>> >> >> > +            crate::build_error!("Must call prepare() before push().");
>> >> >>
>> >> >> This will only prevent the first `prepare` call being missed, right?
>> >> >
>> >> > Correct, unfortunately there's no way to detect subsequent ones.
>> >>
>> >> Does it make sense to do that one in the constructor?
>> >>
>> >> (After looking at the example below) Ah maybe you can't do that, since
>> >> then you would have two `prepare()` calls for the example below...?
>> >
>> > Exactly.
>> >
>> >> >> If you always have to call this before `push`, why not inline it there?
>> >> >
>> >> > You can push() multiple times to compose the firmware path string (which is the
>> >> > whole purpose :).
>> >>
>> >> Ah I see, I only looked at the example you have in the next patch. All
>> >> in all, I think this patch could use some better documentation, since I
>> >> had to read a lot of the code to understand what everything is supposed
>> >> to do...
>> >
>> > I can expand the example in module_firmware! to make things a bit more obvious.
>> >
>> > Otherwise, what information do you think is missing?
>>
>> Abstractly: what `ModInfoBuilder` *does*, concretely:
>> - why the generic constant `N` exists,
>
> It doesn't really matter to the user, since the user never needs to supply it.
> That happens in the module_firmware! macro.
>
> I agree it not good to not mention anything about it at all, but I wouldn't want
> to bother the user with all implemention details.
>
> We can probably just mention that it's used internally and is supplied by
> module_firmware!. (That module_firmware! does that by doing a dry run of the
> builder itself, isn't necessary to know for the user I think.)
>
>> - what `prepare()` does,
>
> Same here, it's an implementation detail not relevant to the user. All the user
> needs to know is that prepare() acts as a separator to be able to supply the
> next firmware path.

How about calling it `new_path`/`new_entry` or similar?

>> - what happens with the `module_name` parameter of `new`
>
> Should probably just mention it's supplied by module_firmware! and used
> internally.

IIUC, that's not the case, the `module_firmware!` macro will call the
`create` function with the name and you're supposed to just pass it onto
the builder.

>> - answer the question "I want that the builder outputs the string `???`
>>   can it do that? If yes, how do I do it?"
>
> All it does is concatenating multiple &str in const context, which I thought is
> clear since there are only push() and prepare() as public methods.
>
> May it be that your request is more about can we add more hints on the
> implementation details rather than user focused documentation?

I am not familiar with MODULE_FIRMWARE in C, and I'd think that someone
that uses this API would know what to put into the `.modinfo` section,
so like "foo\0bar\0\0baz" (no idea if that makes sense, but just add
`firmware` or whatever is needed to make it make sense). And then the
question would be how to translate that into the builder.

I wouldn't be able to piece it together without looking at the
implementation.

---
Cheers,
Benno

Re: [PATCH v5 3/5] rust: firmware: add `module_firmware!` macro

[Danilo Krummrich]

On Thu, Mar 06, 2025 at 01:27:19AM +0000, Benno Lossin wrote:
> On Thu Mar 6, 2025 at 2:04 AM CET, Danilo Krummrich wrote:
> > On Thu, Mar 06, 2025 at 12:31:14AM +0000, Benno Lossin wrote:
> >> On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote:
> >>
> >> > +#[macro_export]
> >> > +macro_rules! module_firmware {
> >> > +    ($($builder:tt)*) => {
> >>
> >> This should probably be `$builder:expr` instead.
> >
> > That doesn't work, the compiler then complains, since it's not an expression:
> >
> > 193  |         static __MODULE_FIRMWARE: [u8; $builder::create(__module_name()).build_length()] =
> >      |                                                ^^ expected one of `.`, `?`, `]`, or an operator
> 
> Does `<$builder>::create` work (with the `expr` fragment)?

No, the compiler then explicitly complains that it expects a type.

> 
> > `ty` doesn't work either, since then the compiler expects the caller to add the
> > const generic, which we want the macro to figure out instead.
> >
> >>
> >> > +
> >> > +        #[cfg(not(MODULE))]
> >> > +        const fn __module_name() -> &'static kernel::str::CStr {
> >> > +            <LocalModule as kernel::ModuleMetadata>::NAME
> >>
> >> Please either use `::kernel::` or `$crate::` instead of `kernel::`.
> >
> > Good catch, thanks.
> >
> >>
> >> Hmm, I am not 100% comfortable with the `LocalModule` way of accessing
> >> the current module for some reason, no idea if there is a rational
> >> argument behind that, but it just doesn't sit right with me.
> >>
> >> Essentially you're doing this for convenience, right? So you don't want
> >> to have to repeat the name of the module type every time?
> >
> > No, it's really that I can't know the type name here, please see the previous
> > patch commit message that introduces `LocalModule` for explanation.
> 
> Gotcha.
> 
> >> > +        }
> >> > +
> >> > +        #[cfg(MODULE)]
> >> > +        const fn __module_name() -> &'static kernel::str::CStr {
> >> > +            kernel::c_str!("")
> >>
> >> Ditto.
> >>
> >> > +        }
> >>
> >> Are these two functions used outside of the `static` below? If no, then
> >> you can just move them into the static? You can also probably use a
> >> `const` instead of a function, that way you only have 4 lines instead
> >> of 8.
> >
> > Is this what you're proposing?
> >
> > 	#[macro_export]
> > 	macro_rules! module_firmware {
> > 	    ($($builder:tt)*) => {
> > 	        const __MODULE_FIRMWARE_PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) {
> > 	            $crate::c_str!("")
> > 	        } else {
> > 	            <LocalModule as $crate::ModuleMetadata>::NAME
> > 	        };
> >
> > 	        #[link_section = ".modinfo"]
> > 	        #[used]
> > 	        static __MODULE_FIRMWARE: [u8; $($builder)*::create(__MODULE_FIRMWARE_PREFIX)
> > 	            .build_length()] = $($builder)*::create(__MODULE_FIRMWARE_PREFIX).build();
> 
> I meant to also move the `const` into the expression, but I guess that
> leads to duplication:
> 
>     #[link_section = ".modinfo"]
>     #[used]
>     static __MODULE_FIRMWARE: [u8; {
>         const PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) {
>             $crate::c_str!("")
>         } else {
>             <LocalModule as $crate::ModuleMetadata>::NAME
>         };
>         <$builder>::create(PREFIX).build_length()
>     }] = {
>         const PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) {
>             $crate::c_str!("")
>         } else {
>             <LocalModule as $crate::ModuleMetadata>::NAME
>         };
>         <$builder>::create(PREFIX)
>     };
> 
> But then the advantage is that only the `__MODULE_FIRMWARE` static will
> be in-scope.
> 
> Do you think that its useful to have the static be accessible? I.e. do
> users need to access it (I would think they don't)? If they don't, then
> we could put all of those things into a `const _: () = { /* ... */ };`.
> But then people can invoke `module_firmware!` multiple times in the same
> module, is that a problem?

Didn't know that's possible (const _; () = { ... };). That's pretty nice, I will
go with my above proposal wrapped into the anonymous const. Thanks.

Re: [PATCH v5 3/5] rust: firmware: add `module_firmware!` macro

[Benno Lossin]

On Thu Mar 6, 2025 at 2:38 AM CET, Danilo Krummrich wrote:
> On Thu, Mar 06, 2025 at 01:27:19AM +0000, Benno Lossin wrote:
>> On Thu Mar 6, 2025 at 2:04 AM CET, Danilo Krummrich wrote:
>> > On Thu, Mar 06, 2025 at 12:31:14AM +0000, Benno Lossin wrote:
>> >> On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote:
>> >>
>> >> > +#[macro_export]
>> >> > +macro_rules! module_firmware {
>> >> > +    ($($builder:tt)*) => {
>> >>
>> >> This should probably be `$builder:expr` instead.
>> >
>> > That doesn't work, the compiler then complains, since it's not an expression:
>> >
>> > 193  |         static __MODULE_FIRMWARE: [u8; $builder::create(__module_name()).build_length()] =
>> >      |                                                ^^ expected one of `.`, `?`, `]`, or an operator
>>
>> Does `<$builder>::create` work (with the `expr` fragment)?
>
> No, the compiler then explicitly complains that it expects a type.

Aw well, can't have em all... Probably would be useful if you add a
comment saying that `expr` and `ty` can't be used.

>> > `ty` doesn't work either, since then the compiler expects the caller to add the
>> > const generic, which we want the macro to figure out instead.
>> >
>> >>
>> >> > +
>> >> > +        #[cfg(not(MODULE))]
>> >> > +        const fn __module_name() -> &'static kernel::str::CStr {
>> >> > +            <LocalModule as kernel::ModuleMetadata>::NAME
>> >>
>> >> Please either use `::kernel::` or `$crate::` instead of `kernel::`.
>> >
>> > Good catch, thanks.
>> >
>> >>
>> >> Hmm, I am not 100% comfortable with the `LocalModule` way of accessing
>> >> the current module for some reason, no idea if there is a rational
>> >> argument behind that, but it just doesn't sit right with me.
>> >>
>> >> Essentially you're doing this for convenience, right? So you don't want
>> >> to have to repeat the name of the module type every time?
>> >
>> > No, it's really that I can't know the type name here, please see the previous
>> > patch commit message that introduces `LocalModule` for explanation.
>>
>> Gotcha.
>>
>> >> > +        }
>> >> > +
>> >> > +        #[cfg(MODULE)]
>> >> > +        const fn __module_name() -> &'static kernel::str::CStr {
>> >> > +            kernel::c_str!("")
>> >>
>> >> Ditto.
>> >>
>> >> > +        }
>> >>
>> >> Are these two functions used outside of the `static` below? If no, then
>> >> you can just move them into the static? You can also probably use a
>> >> `const` instead of a function, that way you only have 4 lines instead
>> >> of 8.
>> >
>> > Is this what you're proposing?
>> >
>> > 	#[macro_export]
>> > 	macro_rules! module_firmware {
>> > 	    ($($builder:tt)*) => {
>> > 	        const __MODULE_FIRMWARE_PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) {
>> > 	            $crate::c_str!("")
>> > 	        } else {
>> > 	            <LocalModule as $crate::ModuleMetadata>::NAME
>> > 	        };
>> >
>> > 	        #[link_section = ".modinfo"]
>> > 	        #[used]
>> > 	        static __MODULE_FIRMWARE: [u8; $($builder)*::create(__MODULE_FIRMWARE_PREFIX)
>> > 	            .build_length()] = $($builder)*::create(__MODULE_FIRMWARE_PREFIX).build();
>>
>> I meant to also move the `const` into the expression, but I guess that
>> leads to duplication:
>>
>>     #[link_section = ".modinfo"]
>>     #[used]
>>     static __MODULE_FIRMWARE: [u8; {
>>         const PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) {
>>             $crate::c_str!("")
>>         } else {
>>             <LocalModule as $crate::ModuleMetadata>::NAME
>>         };
>>         <$builder>::create(PREFIX).build_length()
>>     }] = {
>>         const PREFIX: &'static $crate::str::CStr = if cfg!(MODULE) {
>>             $crate::c_str!("")
>>         } else {
>>             <LocalModule as $crate::ModuleMetadata>::NAME
>>         };
>>         <$builder>::create(PREFIX)
>>     };
>>
>> But then the advantage is that only the `__MODULE_FIRMWARE` static will
>> be in-scope.
>>
>> Do you think that its useful to have the static be accessible? I.e. do
>> users need to access it (I would think they don't)? If they don't, then
>> we could put all of those things into a `const _: () = { /* ... */ };`.
>> But then people can invoke `module_firmware!` multiple times in the same
>> module, is that a problem?
>
> Didn't know that's possible (const _; () = { ... };). That's pretty nice, I will
> go with my above proposal wrapped into the anonymous const. Thanks.

Sounds good.

---
Cheers,
Benno

Re: [PATCH v5 2/5] rust: firmware: introduce `firmware::ModInfoBuilder`

[Danilo Krummrich]

On Thu, Mar 06, 2025 at 01:35:52AM +0000, Benno Lossin wrote:
> On Thu Mar 6, 2025 at 2:29 AM CET, Danilo Krummrich wrote:
> > On Thu, Mar 06, 2025 at 12:24:21AM +0000, Benno Lossin wrote:
> >> On Thu Mar 6, 2025 at 12:57 AM CET, Danilo Krummrich wrote:
> >> > On Wed, Mar 05, 2025 at 11:36:54PM +0000, Benno Lossin wrote:
> >> >> On Wed Mar 5, 2025 at 11:38 PM CET, Danilo Krummrich wrote:
> >> >> > On Wed, Mar 05, 2025 at 10:30:31PM +0000, Benno Lossin wrote:
> >> >> >> On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote:
> >> >> >> > +    /// Push an additional path component.
> >> >> >> > +    ///
> >> >> >> > +    /// After a new [`ModInfoBuilder`] instance has been created, [`ModInfoBuilder::prepare`] must
> >> >> >> > +    /// be called before adding path components.
> >> >> >> > +    pub const fn push(self, s: &str) -> Self {
> >> >> >> > +        if N != 0 && self.n == 0 {
> >> >> >> > +            crate::build_error!("Must call prepare() before push().");
> >> >> >>
> >> >> >> This will only prevent the first `prepare` call being missed, right?
> >> >> >
> >> >> > Correct, unfortunately there's no way to detect subsequent ones.
> >> >>
> >> >> Does it make sense to do that one in the constructor?
> >> >>
> >> >> (After looking at the example below) Ah maybe you can't do that, since
> >> >> then you would have two `prepare()` calls for the example below...?
> >> >
> >> > Exactly.
> >> >
> >> >> >> If you always have to call this before `push`, why not inline it there?
> >> >> >
> >> >> > You can push() multiple times to compose the firmware path string (which is the
> >> >> > whole purpose :).
> >> >>
> >> >> Ah I see, I only looked at the example you have in the next patch. All
> >> >> in all, I think this patch could use some better documentation, since I
> >> >> had to read a lot of the code to understand what everything is supposed
> >> >> to do...
> >> >
> >> > I can expand the example in module_firmware! to make things a bit more obvious.
> >> >
> >> > Otherwise, what information do you think is missing?
> >>
> >> Abstractly: what `ModInfoBuilder` *does*, concretely:
> >> - why the generic constant `N` exists,
> >
> > It doesn't really matter to the user, since the user never needs to supply it.
> > That happens in the module_firmware! macro.
> >
> > I agree it not good to not mention anything about it at all, but I wouldn't want
> > to bother the user with all implemention details.
> >
> > We can probably just mention that it's used internally and is supplied by
> > module_firmware!. (That module_firmware! does that by doing a dry run of the
> > builder itself, isn't necessary to know for the user I think.)
> >
> >> - what `prepare()` does,
> >
> > Same here, it's an implementation detail not relevant to the user. All the user
> > needs to know is that prepare() acts as a separator to be able to supply the
> > next firmware path.
> 
> How about calling it `new_path`/`new_entry` or similar?

Sure, new_entry() sounds good!

> 
> >> - what happens with the `module_name` parameter of `new`
> >
> > Should probably just mention it's supplied by module_firmware! and used
> > internally.
> 
> IIUC, that's not the case, the `module_firmware!` macro will call the
> `create` function with the name and you're supposed to just pass it onto
> the builder.

Yes, but this part is documented by module_firmware!, which I think is the
correct place.

> 
> >> - answer the question "I want that the builder outputs the string `???`
> >>   can it do that? If yes, how do I do it?"
> >
> > All it does is concatenating multiple &str in const context, which I thought is
> > clear since there are only push() and prepare() as public methods.
> >
> > May it be that your request is more about can we add more hints on the
> > implementation details rather than user focused documentation?
> 
> I am not familiar with MODULE_FIRMWARE in C, and I'd think that someone
> that uses this API would know what to put into the `.modinfo` section,
> so like "foo\0bar\0\0baz" (no idea if that makes sense, but just add
> `firmware` or whatever is needed to make it make sense). And then the
> question would be how to translate that into the builder.
> 
> I wouldn't be able to piece it together without looking at the
> implementation.

I believe if you come from the perspective of writing a driver, you reach
module_firmware! first and then the subsequent stuff makes sense.

But I recognize your feedback and will try to make things a bit more obvious by
expanding the example of module_firmware! and expanding a few comments here and
there. I also think that s/prepare/new_entry/ will help a lot.

Re: [PATCH v5 0/5] Initial Nova Core series

[Greg KH]

On Wed, Mar 05, 2025 at 08:56:42PM +0100, Danilo Krummrich wrote:
> On Tue, Mar 04, 2025 at 06:34:47PM +0100, Danilo Krummrich wrote:
> > Danilo Krummrich (5):
> >   rust: module: add type `LocalModule`
> >   rust: firmware: introduce `firmware::ModInfoBuilder`
> >   rust: firmware: add `module_firmware!` macro
> 
> Greg, Luis, Russ, any objections on me taking the two firmware patches through
> the nova tree?

None from me!

Re: [PATCH v5 4/5] gpu: nova-core: add initial driver stub

[Alexandre Courbot]

Hi Danilo,

On Wed Mar 5, 2025 at 2:34 AM JST, Danilo Krummrich wrote:
> Add the initial nova-core driver stub.
>
> nova-core is intended to serve as a common base for nova-drm (the
> corresponding DRM driver) and the vGPU manager VFIO driver, serving as a
> hard- and firmware abstraction layer for GSP-based NVIDIA GPUs.
>
> The Nova project, including nova-core and nova-drm, in the long term,
> is intended to serve as the successor of Nouveau for all GSP-based GPUs.
>
> The motivation for both, starting a successor project for Nouveau and
> doing so using the Rust programming language, is documented in detail
> through a previous post on the mailing list [1], an LWN article [2] and a
> talk from LPC '24.
>
> In order to avoid the chicken and egg problem to require a user to
> upstream Rust abstractions, but at the same time require the Rust
> abstractions to implement the driver, nova-core kicks off as a driver
> stub and is subsequently developed upstream.
>
> Link: https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u [1]
> Link: https://lwn.net/Articles/990736/ [2]
> Link: https://youtu.be/3Igmx28B3BQ?si=sBdSEer4tAPKGpOs [3]
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

A couple of nits inline below, but feel free to add my

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

> ---
>  MAINTAINERS                        |  10 ++
>  drivers/gpu/Makefile               |   1 +
>  drivers/gpu/nova-core/Kconfig      |  14 ++
>  drivers/gpu/nova-core/Makefile     |   3 +
>  drivers/gpu/nova-core/driver.rs    |  47 +++++++
>  drivers/gpu/nova-core/firmware.rs  |  45 +++++++
>  drivers/gpu/nova-core/gpu.rs       | 199 +++++++++++++++++++++++++++++
>  drivers/gpu/nova-core/nova_core.rs |  20 +++
>  drivers/gpu/nova-core/regs.rs      |  55 ++++++++
>  drivers/gpu/nova-core/util.rs      |  21 +++
>  drivers/video/Kconfig              |   1 +
>  11 files changed, 416 insertions(+)
>  create mode 100644 drivers/gpu/nova-core/Kconfig
>  create mode 100644 drivers/gpu/nova-core/Makefile
>  create mode 100644 drivers/gpu/nova-core/driver.rs
>  create mode 100644 drivers/gpu/nova-core/firmware.rs
>  create mode 100644 drivers/gpu/nova-core/gpu.rs
>  create mode 100644 drivers/gpu/nova-core/nova_core.rs
>  create mode 100644 drivers/gpu/nova-core/regs.rs
>  create mode 100644 drivers/gpu/nova-core/util.rs
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8e0736dc2ee0..644817ccaa18 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7449,6 +7449,16 @@ T:	git https://gitlab.freedesktop.org/drm/nouveau.git
>  F:	drivers/gpu/drm/nouveau/
>  F:	include/uapi/drm/nouveau_drm.h
>  
> +CORE DRIVER FOR NVIDIA GPUS [RUST]
> +M:	Danilo Krummrich <dakr@kernel.org>
> +L:	nouveau@lists.freedesktop.org
> +S:	Supported
> +Q:	https://patchwork.freedesktop.org/project/nouveau/
> +B:	https://gitlab.freedesktop.org/drm/nova/-/issues
> +C:	irc://irc.oftc.net/nouveau
> +T:	git https://gitlab.freedesktop.org/drm/nova.git nova-next
> +F:	drivers/gpu/nova-core/
> +
>  DRM DRIVER FOR OLIMEX LCD-OLINUXINO PANELS
>  M:	Stefan Mavrodiev <stefan@olimex.com>
>  S:	Maintained
> diff --git a/drivers/gpu/Makefile b/drivers/gpu/Makefile
> index 8997f0096545..36a54d456630 100644
> --- a/drivers/gpu/Makefile
> +++ b/drivers/gpu/Makefile
> @@ -5,3 +5,4 @@
>  obj-y			+= host1x/ drm/ vga/
>  obj-$(CONFIG_IMX_IPUV3_CORE)	+= ipu-v3/
>  obj-$(CONFIG_TRACE_GPU_MEM)		+= trace/
> +obj-$(CONFIG_NOVA_CORE)		+= nova-core/
> diff --git a/drivers/gpu/nova-core/Kconfig b/drivers/gpu/nova-core/Kconfig
> new file mode 100644
> index 000000000000..ad0c06756516
> --- /dev/null
> +++ b/drivers/gpu/nova-core/Kconfig
> @@ -0,0 +1,14 @@
> +config NOVA_CORE
> +	tristate "Nova Core GPU driver"
> +	depends on PCI
> +	depends on RUST
> +	depends on RUST_FW_LOADER_ABSTRACTIONS
> +	default n
> +	help
> +	  Choose this if you want to build the Nova Core driver for Nvidia
> +	  GPUs based on the GPU System Processor (GSP). This is true for Turing
> +	  and later GPUs.
> +
> +	  This driver is work in progress and may not be functional.
> +
> +	  If M is selected, the module will be called nova_core.
> diff --git a/drivers/gpu/nova-core/Makefile b/drivers/gpu/nova-core/Makefile
> new file mode 100644
> index 000000000000..2d78c50126e1
> --- /dev/null
> +++ b/drivers/gpu/nova-core/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_NOVA_CORE) += nova_core.o
> diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
> new file mode 100644
> index 000000000000..63c19f140fbd
> --- /dev/null
> +++ b/drivers/gpu/nova-core/driver.rs
> @@ -0,0 +1,47 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use kernel::{bindings, c_str, pci, prelude::*};
> +
> +use crate::gpu::Gpu;
> +
> +#[pin_data]
> +pub(crate) struct NovaCore {
> +    #[pin]
> +    pub(crate) gpu: Gpu,
> +}
> +
> +const BAR0_SIZE: usize = 8;
> +pub(crate) type Bar0 = pci::Bar<BAR0_SIZE>;
> +
> +kernel::pci_device_table!(
> +    PCI_TABLE,
> +    MODULE_PCI_TABLE,
> +    <NovaCore as pci::Driver>::IdInfo,
> +    [(
> +        pci::DeviceId::from_id(bindings::PCI_VENDOR_ID_NVIDIA, bindings::PCI_ANY_ID as _),
> +        ()
> +    )]
> +);
> +
> +impl pci::Driver for NovaCore {
> +    type IdInfo = ();
> +    const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
> +
> +    fn probe(pdev: &mut pci::Device, _info: &Self::IdInfo) -> Result<Pin<KBox<Self>>> {
> +        dev_dbg!(pdev.as_ref(), "Probe Nova Core GPU driver.\n");
> +
> +        pdev.enable_device_mem()?;
> +        pdev.set_master();
> +
> +        let bar = pdev.iomap_region_sized::<BAR0_SIZE>(0, c_str!("nova-core/bar0"))?;
> +
> +        let this = KBox::pin_init(
> +            try_pin_init!(Self {
> +                gpu <- Gpu::new(pdev, bar)?,
> +            }),
> +            GFP_KERNEL,
> +        )?;
> +
> +        Ok(this)
> +    }
> +}
> diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
> new file mode 100644
> index 000000000000..9de1399a2a69
> --- /dev/null
> +++ b/drivers/gpu/nova-core/firmware.rs
> @@ -0,0 +1,45 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use crate::gpu;
> +use kernel::firmware;
> +
> +pub(crate) struct ModInfoBuilder<const N: usize>(firmware::ModInfoBuilder<N>);
> +
> +impl<const N: usize> ModInfoBuilder<N> {
> +    const fn make_entry_file(self, chipset: &str, fw: &str) -> Self {
> +        let version = "535.113.01";

This should probably be a constant.

> +
> +        ModInfoBuilder(
> +            self.0
> +                .prepare()
> +                .push("nvidia/")
> +                .push(chipset)
> +                .push("/gsp/")
> +                .push(fw)
> +                .push("-")
> +                .push(version)
> +                .push(".bin"),
> +        )
> +    }
> +
> +    const fn make_entry_chipset(self, chipset: &str) -> Self {
> +        self.make_entry_file(chipset, "booter_load")
> +            .make_entry_file(chipset, "booter_unload")
> +            .make_entry_file(chipset, "bootloader")
> +            .make_entry_file(chipset, "gsp")
> +    }
> +
> +    pub(crate) const fn create(
> +        module_name: &'static kernel::str::CStr,
> +    ) -> firmware::ModInfoBuilder<N> {
> +        let mut this = Self(firmware::ModInfoBuilder::new(module_name));
> +        let mut i = 0;
> +
> +        while i < gpu::Chipset::NAMES.len() {
> +            this = this.make_entry_chipset(gpu::Chipset::NAMES[i]);
> +            i += 1;
> +        }
> +
> +        this.0
> +    }
> +}
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> new file mode 100644
> index 000000000000..f57b7efa10f3
> --- /dev/null
> +++ b/drivers/gpu/nova-core/gpu.rs
> @@ -0,0 +1,199 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use kernel::{
> +    device, devres::Devres, error::code::*, firmware, fmt, pci, prelude::*, str::CString,
> +};
> +
> +use crate::driver::Bar0;
> +use crate::regs;
> +use crate::util;
> +use core::fmt;
> +
> +macro_rules! define_chipset {
> +    ({ $($variant:ident = $value:expr),* $(,)* }) =>
> +    {
> +        /// Enum representation of the GPU chipset.
> +        #[derive(fmt::Debug)]
> +        pub(crate) enum Chipset {
> +            $($variant = $value),*,
> +        }
> +
> +        impl Chipset {
> +            pub(crate) const ALL: &'static [Chipset] = &[
> +                $( Chipset::$variant, )*
> +            ];
> +
> +            pub(crate) const NAMES: [&str; Self::ALL.len()] = [
> +                $( util::const_bytes_to_str(
> +                        util::to_lowercase_bytes::<{ stringify!($variant).len() }>(
> +                            stringify!($variant)
> +                        ).as_slice()
> +                ), )*
> +            ];
> +        }
> +
> +        // TODO replace with something like derive(FromPrimitive)
> +        impl TryFrom<u32> for Chipset {
> +            type Error = kernel::error::Error;
> +
> +            fn try_from(value: u32) -> Result<Self, Self::Error> {
> +                match value {
> +                    $( $value => Ok(Chipset::$variant), )*
> +                    _ => Err(ENODEV),
> +                }
> +            }
> +        }
> +    }
> +}
> +
> +define_chipset!({
> +    // Turing
> +    TU102 = 0x162,
> +    TU104 = 0x164,
> +    TU106 = 0x166,
> +    TU117 = 0x167,
> +    TU116 = 0x168,
> +    // Ampere
> +    GA102 = 0x172,
> +    GA103 = 0x173,
> +    GA104 = 0x174,
> +    GA106 = 0x176,
> +    GA107 = 0x177,
> +    // Ada
> +    AD102 = 0x192,
> +    AD103 = 0x193,
> +    AD104 = 0x194,
> +    AD106 = 0x196,
> +    AD107 = 0x197,
> +});
> +
> +impl Chipset {
> +    pub(crate) fn arch(&self) -> Architecture {
> +        match self {
> +            Self::TU102 | Self::TU104 | Self::TU106 | Self::TU117 | Self::TU116 => {
> +                Architecture::Turing
> +            }
> +            Self::GA102 | Self::GA103 | Self::GA104 | Self::GA106 | Self::GA107 => {
> +                Architecture::Ampere
> +            }
> +            Self::AD102 | Self::AD103 | Self::AD104 | Self::AD106 | Self::AD107 => {
> +                Architecture::Ada
> +            }
> +        }
> +    }
> +}
> +
> +// TODO
> +//
> +// The resulting strings are used to generate firmware paths, hence the
> +// generated strings have to be stable.
> +//
> +// Hence, replace with something like strum_macros derive(Display).
> +//
> +// For now, redirect to fmt::Debug for convenience.
> +impl fmt::Display for Chipset {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        write!(f, "{:?}", self)
> +    }
> +}
> +
> +/// Enum representation of the GPU generation.
> +#[derive(fmt::Debug)]
> +pub(crate) enum Architecture {
> +    Turing,
> +    Ampere,
> +    Ada,
> +}
> +
> +pub(crate) struct Revision {
> +    major: u8,
> +    minor: u8,
> +}
> +
> +impl Revision {
> +    fn from_boot0(boot0: regs::Boot0) -> Self {
> +        Self {
> +            major: boot0.major_rev(),
> +            minor: boot0.minor_rev(),
> +        }
> +    }
> +}
> +
> +impl fmt::Display for Revision {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        write!(f, "{:x}.{:x}", self.major, self.minor)
> +    }
> +}
> +
> +/// Structure holding the metadata of the GPU.
> +pub(crate) struct Spec {
> +    chipset: Chipset,
> +    /// The revision of the chipset.
> +    revision: Revision,
> +}
> +
> +impl Spec {
> +    fn new(bar: &Devres<Bar0>) -> Result<Spec> {
> +        let bar = bar.try_access().ok_or(ENXIO)?;
> +        let boot0 = regs::Boot0::read(&bar);
> +
> +        Ok(Self {
> +            chipset: boot0.chipset().try_into()?,
> +            revision: Revision::from_boot0(boot0),
> +        })
> +    }
> +}
> +
> +/// Structure encapsulating the firmware blobs required for the GPU to operate.
> +#[expect(dead_code)]
> +pub(crate) struct Firmware {
> +    booter_load: firmware::Firmware,
> +    booter_unload: firmware::Firmware,
> +    bootloader: firmware::Firmware,
> +    gsp: firmware::Firmware,
> +}
> +
> +impl Firmware {
> +    fn new(dev: &device::Device, spec: &Spec, ver: &str) -> Result<Firmware> {
> +        let mut chip_name = CString::try_from_fmt(fmt!("{}", spec.chipset))?;
> +        chip_name.make_ascii_lowercase();
> +
> +        let request = |name_| {
> +            CString::try_from_fmt(fmt!("nvidia/{}/gsp/{}-{}.bin", &*chip_name, name_, ver))
> +                .and_then(|path| firmware::Firmware::request(&path, dev))
> +        };
> +
> +        Ok(Firmware {
> +            booter_load: request("booter_load")?,
> +            booter_unload: request("booter_unload")?,
> +            bootloader: request("bootloader")?,
> +            gsp: request("gsp")?,
> +        })
> +    }
> +}
> +
> +/// Structure holding the resources required to operate the GPU.
> +#[pin_data]
> +pub(crate) struct Gpu {
> +    spec: Spec,
> +    /// MMIO mapping of PCI BAR 0
> +    bar: Devres<Bar0>,
> +    fw: Firmware,
> +}
> +
> +impl Gpu {
> +    pub(crate) fn new(pdev: &pci::Device, bar: Devres<Bar0>) -> Result<impl PinInit<Self>> {
> +        let spec = Spec::new(&bar)?;
> +        let fw = Firmware::new(pdev.as_ref(), &spec, "535.113.01")?;
> +
> +        dev_info!(
> +            pdev.as_ref(),
> +            "NVIDIA (Chipset: {}, Architecture: {:?}, Revision: {})\n",
> +            spec.chipset,
> +            spec.chipset.arch(),
> +            spec.revision
> +        );
> +
> +        Ok(pin_init!(Self { spec, bar, fw }))
> +    }
> +}
> diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
> new file mode 100644
> index 000000000000..a91cd924054b
> --- /dev/null
> +++ b/drivers/gpu/nova-core/nova_core.rs
> @@ -0,0 +1,20 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Nova Core GPU Driver
> +
> +mod driver;
> +mod firmware;
> +mod gpu;
> +mod regs;
> +mod util;
> +
> +kernel::module_pci_driver! {
> +    type: driver::NovaCore,
> +    name: "NovaCore",
> +    author: "Danilo Krummrich",
> +    description: "Nova Core GPU driver",
> +    license: "GPL v2",
> +    firmware: [],
> +}
> +
> +kernel::module_firmware!(firmware::ModInfoBuilder);
> diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
> new file mode 100644
> index 000000000000..50aefb150b0b
> --- /dev/null
> +++ b/drivers/gpu/nova-core/regs.rs
> @@ -0,0 +1,55 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use crate::driver::Bar0;
> +
> +// TODO
> +//
> +// Create register definitions via generic macros. See task "Generic register
> +// abstraction" in Documentation/gpu/nova/core/todo.rst.
> +
> +const BOOT0_OFFSET: usize = 0x00000000;
> +
> +// 3:0 - chipset minor revision
> +const BOOT0_MINOR_REV_SHIFT: u8 = 0;
> +const BOOT0_MINOR_REV_MASK: u32 = 0x0000000f;
> +
> +// 7:4 - chipset major revision
> +const BOOT0_MAJOR_REV_SHIFT: u8 = 4;
> +const BOOT0_MAJOR_REV_MASK: u32 = 0x000000f0;
> +
> +// 23:20 - chipset implementation Identifier (depends on architecture)
> +const BOOT0_IMPL_SHIFT: u8 = 20;
> +const BOOT0_IMPL_MASK: u32 = 0x00f00000;
> +
> +// 28:24 - chipset architecture identifier
> +const BOOT0_ARCH_MASK: u32 = 0x1f000000;
> +
> +// 28:20 - chipset identifier (virtual register field combining BOOT0_IMPL and
> +//         BOOT0_ARCH)
> +const BOOT0_CHIPSET_SHIFT: u8 = BOOT0_IMPL_SHIFT;
> +const BOOT0_CHIPSET_MASK: u32 = BOOT0_IMPL_MASK | BOOT0_ARCH_MASK;
> +
> +#[derive(Copy, Clone)]
> +pub(crate) struct Boot0(u32);
> +
> +impl Boot0 {
> +    #[inline]
> +    pub(crate) fn read(bar: &Bar0) -> Self {
> +        Self(bar.readl(BOOT0_OFFSET))
> +    }
> +
> +    #[inline]
> +    pub(crate) fn chipset(&self) -> u32 {
> +        (self.0 & BOOT0_CHIPSET_MASK) >> BOOT0_CHIPSET_SHIFT
> +    }
> +
> +    #[inline]
> +    pub(crate) fn minor_rev(&self) -> u8 {
> +        ((self.0 & BOOT0_MINOR_REV_MASK) >> BOOT0_MINOR_REV_SHIFT) as u8
> +    }
> +
> +    #[inline]
> +    pub(crate) fn major_rev(&self) -> u8 {
> +        ((self.0 & BOOT0_MAJOR_REV_MASK) >> BOOT0_MAJOR_REV_SHIFT) as u8
> +    }
> +}
> diff --git a/drivers/gpu/nova-core/util.rs b/drivers/gpu/nova-core/util.rs
> new file mode 100644
> index 000000000000..332a64cfc6a9
> --- /dev/null
> +++ b/drivers/gpu/nova-core/util.rs
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +pub(crate) const fn to_lowercase_bytes<const N: usize>(s: &str) -> [u8; N] {
> +    let src = s.as_bytes();
> +    let mut dst = [0; N];
> +    let mut i = 0;
> +
> +    while i < src.len() && i < N {
> +        dst[i] = (src[i] as char).to_ascii_lowercase() as u8;
> +        i += 1;
> +    }
> +
> +    dst
> +}
> +
> +pub(crate) const fn const_bytes_to_str(bytes: &[u8]) -> &str {
> +    match core::str::from_utf8(bytes) {
> +        Ok(string) => string,
> +        Err(_) => kernel::build_error!("Bytes are not valid UTF-8."),
> +    }
> +}

I guess these functions could be useful to other drivers and can maybe
be moved to a string utility module, but this can be done later too.

Thanks!
Alex.

Re: [PATCH v5 5/5] gpu: nova-core: add initial documentation

[Alexandre Courbot]

On Wed Mar 5, 2025 at 2:34 AM JST, Danilo Krummrich wrote:
> Add the initial documentation of the Nova project.
>
> The initial project documentation consists out of a brief introduction
> of the project, as well as project guidelines both general and nova-core
> specific and a task list for nova-core specifically.
>
> The task list is divided into tasks for general Rust infrastructure
> required by the project, tasks regarding GSP enablement and firmware
> abstraction, general GPU driver tasks as well as tasks related to
> external API design and test infrastructure.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

Thanks for this - the todo list in particular is super helpful to
understand which components outside of the driver we need to include or
drive to completion.

Re: [PATCH v5 5/5] gpu: nova-core: add initial documentation

[FUJITA Tomonori]

On Tue,  4 Mar 2025 18:34:52 +0100
Danilo Krummrich <dakr@kernel.org> wrote:

> +Delay / Sleep abstractions
> +--------------------------
> +
> +Rust abstractions for the kernel's delay() and sleep() functions.
> +
> +There is some ongoing work from FUJITA Tomonori [1], which has not seen any updates
> +since Oct. 24.
> +
> +| Complexity: Beginner
> +| Link: https://lore.kernel.org/netdev/20241001112512.4861-2-fujita.tomonori@gmail.com/ [1]

I posted v11 last month.

https://lore.kernel.org/netdev/20250220070611.214262-1-fujita.tomonori@gmail.com/

Re: [PATCH v5 5/5] gpu: nova-core: add initial documentation

[Danilo Krummrich]

On Thu, Mar 06, 2025 at 09:56:38PM +0900, FUJITA Tomonori wrote:
> On Tue,  4 Mar 2025 18:34:52 +0100
> Danilo Krummrich <dakr@kernel.org> wrote:
> 
> > +Delay / Sleep abstractions
> > +--------------------------
> > +
> > +Rust abstractions for the kernel's delay() and sleep() functions.
> > +
> > +There is some ongoing work from FUJITA Tomonori [1], which has not seen any updates
> > +since Oct. 24.
> > +
> > +| Complexity: Beginner
> > +| Link: https://lore.kernel.org/netdev/20241001112512.4861-2-fujita.tomonori@gmail.com/ [1]
> 
> I posted v11 last month.
> 
> https://lore.kernel.org/netdev/20250220070611.214262-1-fujita.tomonori@gmail.com/

Thanks for letting me know.

I think I lost track of this because in v1 the series was named "add delay
abstraction (sleep functions)" and with v2 it was switched to "rust: Add IO
polling" and I was searching for subsequent patch series with the "delay"
keyword.

Anyways, AFAICS you ended up with adding fsleep(). I think nova-core will need
control over having a busy loop or actually re-schedule.

I will update this entry accordingly.

Re: [PATCH v5 5/5] gpu: nova-core: add initial documentation

[FUJITA Tomonori]

On Thu, 6 Mar 2025 14:45:05 +0100
Danilo Krummrich <dakr@kernel.org> wrote:

> On Thu, Mar 06, 2025 at 09:56:38PM +0900, FUJITA Tomonori wrote:
>> On Tue,  4 Mar 2025 18:34:52 +0100
>> Danilo Krummrich <dakr@kernel.org> wrote:
>> 
>> > +Delay / Sleep abstractions
>> > +--------------------------
>> > +
>> > +Rust abstractions for the kernel's delay() and sleep() functions.
>> > +
>> > +There is some ongoing work from FUJITA Tomonori [1], which has not seen any updates
>> > +since Oct. 24.
>> > +
>> > +| Complexity: Beginner
>> > +| Link: https://lore.kernel.org/netdev/20241001112512.4861-2-fujita.tomonori@gmail.com/ [1]
>> 
>> I posted v11 last month.
>> 
>> https://lore.kernel.org/netdev/20250220070611.214262-1-fujita.tomonori@gmail.com/
> 
> Thanks for letting me know.
> 
> I think I lost track of this because in v1 the series was named "add delay
> abstraction (sleep functions)" and with v2 it was switched to "rust: Add IO
> polling" and I was searching for subsequent patch series with the "delay"
> keyword.

I see.

During the review process, I changed the subject due to the consensus
that, in most cases, device drivers should use read_poll_timeout
instead of calling the sleep function directly.

> Anyways, AFAICS you ended up with adding fsleep(). I think nova-core will need
> control over having a busy loop or actually re-schedule.

I plan to add read_poll_timeout_atomic() with delay functions:

https://lore.kernel.org/netdev/20250228.080550.354359820929821928.fujita.tomonori@gmail.com/

delay functions need Delta struct in the patchset so the patchset
needs to be merged first.

Re: [PATCH v5 0/5] Initial Nova Core series

[Russ Weight]

On Wed, Mar 05, 2025 at 08:56:42PM +0100, Danilo Krummrich wrote:
> On Tue, Mar 04, 2025 at 06:34:47PM +0100, Danilo Krummrich wrote:
> > Danilo Krummrich (5):
> >   rust: module: add type `LocalModule`
> >   rust: firmware: introduce `firmware::ModInfoBuilder`
> >   rust: firmware: add `module_firmware!` macro
> 
> Greg, Luis, Russ, any objections on me taking the two firmware patches through
> the nova tree?
> 
> >   gpu: nova-core: add initial driver stub
> >   gpu: nova-core: add initial documentation

No objections here.

- Russ