[PATCH v2 0/8] gpu: nova-core: process and prepare more firmwares to boot GSP

[PATCH v2 0/8] gpu: nova-core: process and prepare more firmwares to boot GSP

[Alexandre Courbot]

This series makes more progress on the GSP boot process for Ampere GPUs.

At the end of the previous series [1], we were left with a WPR memory
region created by the FRTS firmware, but still far from the GSP running.
This series brings us closer to that goal by preparing 3 firmware
packages:

* The Booter firmware, which the driver loads and runs on the SEC2
  falcon;
* The GSP bootloader, which is loaded by Booter onto the GSP RISC-V
  core;
* The GSP firmware itself, which is verified and loaded by the GSP
  bootloader.

There 3 bundles are involved in a rather complex dance that is made
necessary by limitations related to the level of privilege required to
load code onto the GSP (doing so requires a Heavy Secured signed
firmware, which is the role fulfilled by Booter).

The first patch is an addition to `FromBytes` that makes it able to
process the unaligned headers contained in the firmware files by
performing a copy. Second patch adds support for a header commonly used
in NVIDIA firmware files. Both are relatively simple.

The third patch parses the Booter firmware file, queries the hardware
for the right signature to use, and patch it into the firmware blob.
This makes Booter ready to load and run.

Fourth patch takes care of preparing the GSP bootloader for execution
and fifth one does the same for the GSP firmware, which needs to come
with its own device-mapped page table.

Sixth and seventh patches switch to the 570.144 firmware and introduce
the layout for its bindings. The raw bindings are stored in a private
module, and abstracted/reexported by the parent module as needed. This
allows consumer modules to access a safer/nicer form of the bindings
than the raw one, and also makes it easier to switch to a different (and
potentially incompatible) firmware version in the future.

570.144 has been picked because it is the latest firmware currently in
linux-firmware, but as development progresses the plan is to switch to a
newer one that is designed with the constraint of upstream in mind. So
support for 570.144 will be dropped in the future. Support for multiple
firmware versions is not considered at the moment: there is no immediate
need for it as the driver is still unstable, and we can think about this
point once we approach stability (and have better visibility about the
shape of the firmware at that point).

The last patch introduces the first bindings and uses them to compute
more framebuffer layout information needed for booting the GSP. A
separate patch series will pick up from there to use this information
and finally run these firmware blobs.

The base of this series is today's nova-next, but there a few more
unmerged dependencies required:

- BorrowedPage, AsPageIter and VmallocPageIter [2]
- Rust infrastructure for sg_table and scatterlist [3]
- FromBytes [4]
- The Alignment series [5]

[1] https://lore.kernel.org/rust-for-linux/20250619-nova-frts-v6-0-ecf41ef99252@nvidia.com/
[2] https://lore.kernel.org/rust-for-linux/20250820145434.94745-1-dakr@kernel.org/
[3] https://lore.kernel.org/rust-for-linux/20250825132539.122412-1-dakr@kernel.org/
[4] https://lore.kernel.org/rust-for-linux/20250824213134.27079-1-christiansantoslima21@gmail.com/
[5] https://lore.kernel.org/rust-for-linux/20250821-num-v4-0-1f3a425d7244@nvidia.com/

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
Changes in v2:
- Add some GSP bindings and use them to compute more FB layout info
  needed to boot GSP,
- Use PinInit in GspFirmware to avoid several heap allocations,
- Rename `bootloader` to `gsp_bootloader` in `Firmware` to avoid
  confusion with the future Turing falcon bootloader,
- Link to v1: https://lore.kernel.org/r/20250822-nova_firmware-v1-0-ff5633679460@nvidia.com

---
Alexandre Courbot (7):
      rust: transmute: add `from_bytes_copy` method to `FromBytes` trait
      gpu: nova-core: firmware: add support for common firmware header
      gpu: nova-core: firmware: process Booter and patch its signature
      gpu: nova-core: firmware: process the GSP bootloader
      gpu: nova-core: firmware: process and prepare the GSP firmware
      gpu: nova-core: firmware: use 570.144 firmware
      gpu: nova-core: compute layout of more framebuffer regions required for GSP

Alistair Popple (1):
      gpu: nova-core: Add base files for r570.144 firmware bindings

 Documentation/gpu/nova/core/todo.rst            |  17 --
 drivers/gpu/nova-core/falcon.rs                 |   4 +-
 drivers/gpu/nova-core/fb.rs                     | 112 +++++++-
 drivers/gpu/nova-core/fb/hal.rs                 |   4 +
 drivers/gpu/nova-core/fb/hal/ga100.rs           |   6 +-
 drivers/gpu/nova-core/fb/hal/ga102.rs           |  14 +-
 drivers/gpu/nova-core/fb/hal/tu102.rs           |  14 +
 drivers/gpu/nova-core/firmware.rs               | 204 +++++++++++++-
 drivers/gpu/nova-core/firmware/booter.rs        | 356 ++++++++++++++++++++++++
 drivers/gpu/nova-core/firmware/gsp.rs           | 117 ++++++++
 drivers/gpu/nova-core/firmware/riscv.rs         |  89 ++++++
 drivers/gpu/nova-core/gpu.rs                    |  13 +-
 drivers/gpu/nova-core/gsp.rs                    |   7 +
 drivers/gpu/nova-core/nova_core.rs              |   2 +
 drivers/gpu/nova-core/nvfw.rs                   |  42 +++
 drivers/gpu/nova-core/nvfw/r570_144.rs          |  28 ++
 drivers/gpu/nova-core/nvfw/r570_144_bindings.rs | 126 +++++++++
 rust/kernel/transmute.rs                        |  17 ++
 18 files changed, 1135 insertions(+), 37 deletions(-)
---
base-commit: 331c24e6ce814af2af74bac648d1ac1708873e9c
change-id: 20250822-nova_firmware-e0ffb492ba35
prerequisite-message-id: <20250820145434.94745-1-dakr@kernel.org>
prerequisite-patch-id: 0e1b1f9a665317ff569a37df6ff49cd1880b04f8
prerequisite-patch-id: 178b864e6d1b88ee299dcc05d1a7a4c89ec7ed51
prerequisite-patch-id: 7f72c4bfd0e5f50b6d2f8ce96751782894a3ba81
prerequisite-patch-id: 62fa6de7d3ae99dc54c092087bd716e6749545fd
prerequisite-patch-id: 3d14d56ca93b0831837aa26b802100a250adeac6
prerequisite-patch-id: 7a12f4b0e7588874ce589b41b70671dc261b9468
prerequisite-patch-id: c44763ec35c4e4431e769df088b98424cbddf7df
prerequisite-message-id: <20250825132539.122412-1-dakr@kernel.org>
prerequisite-patch-id: a9e008c179b1c2fbe76654a191e5018880383d49
prerequisite-patch-id: 363279599349e5efc7069a63b3f0574639e25418
prerequisite-patch-id: 4dd34d858514b7b982f4ebc85103a756205c3219
prerequisite-patch-id: a747e05834cdb8b8f727e1f7c8b110c636cadab8
prerequisite-patch-id: 24833689bdecd3fc7a604e13bfe203ccd2fca6f0
prerequisite-message-id: <20250824213134.27079-1-christiansantoslima21@gmail.com>
prerequisite-patch-id: 3ddaa5300963f6c069411c6431965a7a48107a8b
prerequisite-change-id: 20250620-num-9420281c02c7:v4
prerequisite-patch-id: 50077433250cad1cf60eb8f85c78e532ac91852e
prerequisite-patch-id: 021a41cd35f09790ec383521ecc9b4c167868732
prerequisite-patch-id: a1ec5698a198d4aad45432b50d42f401e3db6452
prerequisite-patch-id: 8565b054c432bcc9a3a0d0121a934c74ef36d535
prerequisite-patch-id: 19d008deabb88beb441d2398f120ecb426fbdb43
prerequisite-patch-id: 3bc0d2be065a900d224ff8c1bc4450abfe9eb2cc
prerequisite-patch-id: 5b4eb0f71fa2ccf662594819fa79fd932f4f164f
prerequisite-patch-id: 9058ca08cd149444b5f910e4bb4494a890d1a733
prerequisite-patch-id: 8804806f7cc605feddded0804eec8b8362d7b965
prerequisite-patch-id: f999cabde51824432a1bf60817518d1ce189eb76
prerequisite-patch-id: 49e15538e142f2e7dd4f1ba0cf2fd891bd265d36
prerequisite-patch-id: 2ecf9b1e26b5203065bfac4ccf74301b3bb4fbe6
prerequisite-patch-id: 1af6ec7c2ce8503fe476985f59949dcd150ee6bf
prerequisite-patch-id: ac72e72b3affece504bff76b60b88769ff200a2f
prerequisite-patch-id: 7dc0a6da8c9727d27250cf730f8aaf6dd8b3d8c7
prerequisite-patch-id: 31a0a2469de9ac965186098072753dcc749b40fe
prerequisite-patch-id: 7e6d1fc7cf910decf481d135a19b0add38da2b2a
prerequisite-patch-id: c72ab11e9346de71eabfe0e6466636d5ab15a5ba
prerequisite-patch-id: 3f236fdea8c4b33620d0f863fea573b46ab0ded6
prerequisite-patch-id: a8ab42d0c9c3c837bb4cacb02cef585ef163a27e
prerequisite-patch-id: 930a1f26364ed67e0d6b85c96251028fda43c80a
prerequisite-patch-id: f1bc1fd46145a66235ab7475463584e1803882a3
prerequisite-patch-id: 4a2fd7bd8d13dc2feaf68e0dc681546ce2ab3e40
prerequisite-patch-id: dd0df8d299dc0615a88cc0019f38bc09cee31ed7
prerequisite-patch-id: 56a45978f7b956c94ec66eecd4b438706d5174ce
prerequisite-patch-id: e9bceff31bbf653d2641194b97eb1c4e5d8b93ee

Best regards,
-- 
Alexandre Courbot <acourbot@nvidia.com>

[PATCH v2 1/8] rust: transmute: add `from_bytes_copy` method to `FromBytes` trait

[Alexandre Courbot]

`FromBytes::from_bytes` comes with a few practical limitations:

- It requires the bytes slice to have the same alignment as the returned
  type, which might not be guaranteed in the case of a byte stream,
- It returns a reference, requiring the returned type to implement
  `Clone` if one wants to keep the value for longer than the lifetime of
  the slice.

To overcome these when needed, add a `from_bytes_copy` with a default
implementation in the trait. `from_bytes_copy` returns an owned value
that is populated using an unaligned read, removing the lifetime
constraint and making it usable even on non-aligned byte slices.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 rust/kernel/transmute.rs | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/rust/kernel/transmute.rs b/rust/kernel/transmute.rs
index 494bb3b1d059337520efef694fc8952972d44fbf..721dd8254dcedd71ed7c1fc0ee9292950c16c89e 100644
--- a/rust/kernel/transmute.rs
+++ b/rust/kernel/transmute.rs
@@ -78,6 +78,23 @@ fn from_bytes_mut(bytes: &mut [u8]) -> Option<&mut Self>
             None
         }
     }
+
+    /// Creates an owned instance of `Self` by copying `bytes`.
+    ///
+    /// As the data is copied into a properly-aligned location, this method can be used even if
+    /// [`FromBytes::from_bytes`] would return `None` due to incompatible alignment.
+    fn from_bytes_copy(bytes: &[u8]) -> Option<Self>
+    where
+        Self: Sized,
+    {
+        if bytes.len() == size_of::<Self>() {
+            // SAFETY: `bytes` has the same size as `Self`, and per the invariants of `FromBytes`,
+            // any byte sequence is a valid value for `Self`.
+            Some(unsafe { core::ptr::read_unaligned(bytes.as_ptr().cast::<Self>()) })
+        } else {
+            None
+        }
+    }
 }
 
 macro_rules! impl_frombytes {

-- 
2.50.1

[PATCH v2 2/8] gpu: nova-core: firmware: add support for common firmware header

[Alexandre Courbot]

Several firmware files loaded from userspace feature a common header
that describes their payload. Add basic support for it so subsequent
patches can leverage it.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/nova-core/firmware.rs | 62 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
index 2931912ddba0ea1fe6d027ccec70b39cdb40344a..ccb4d19f8fa76b0e844252dede5f50b37c590571 100644
--- a/drivers/gpu/nova-core/firmware.rs
+++ b/drivers/gpu/nova-core/firmware.rs
@@ -4,11 +4,13 @@
 //! to be loaded into a given execution unit.
 
 use core::marker::PhantomData;
+use core::mem::size_of;
 
 use kernel::device;
 use kernel::firmware;
 use kernel::prelude::*;
 use kernel::str::CString;
+use kernel::transmute::FromBytes;
 
 use crate::dma::DmaObject;
 use crate::falcon::FalconFirmware;
@@ -150,6 +152,66 @@ fn no_patch_signature(self) -> FirmwareDmaObject<F, Signed> {
     }
 }
 
+/// Header common to most firmware files.
+#[repr(C)]
+#[derive(Debug, Clone)]
+struct BinHdr {
+    /// Magic number, must be `0x10de`.
+    bin_magic: u32,
+    /// Version of the header.
+    bin_ver: u32,
+    /// Size in bytes of the binary (to be ignored).
+    bin_size: u32,
+    /// Offset of the start of the application-specific header.
+    header_offset: u32,
+    /// Offset of the start of the data payload.
+    data_offset: u32,
+    /// Size in bytes of the data payload.
+    data_size: u32,
+}
+
+// SAFETY: all bit patterns are valid for this type, and it doesn't use interior mutability.
+unsafe impl FromBytes for BinHdr {}
+
+// A firmware blob starting with a `BinHdr`.
+struct BinFirmware<'a> {
+    hdr: BinHdr,
+    fw: &'a [u8],
+}
+
+#[expect(dead_code)]
+impl<'a> BinFirmware<'a> {
+    /// Interpret `fw` as a firmware image starting with a [`BinHdr`], and returns the
+    /// corresponding [`BinFirmware`] that can be used to extract its payload.
+    fn new(fw: &'a firmware::Firmware) -> Result<Self> {
+        const BIN_MAGIC: u32 = 0x10de;
+        let fw = fw.data();
+
+        fw.get(0..size_of::<BinHdr>())
+            // Extract header.
+            .and_then(BinHdr::from_bytes_copy)
+            // Validate header.
+            .and_then(|hdr| {
+                if hdr.bin_magic == BIN_MAGIC {
+                    Some(hdr)
+                } else {
+                    None
+                }
+            })
+            .map(|hdr| Self { hdr, fw })
+            .ok_or(EINVAL)
+    }
+
+    /// Returns the data payload of the firmware, or `None` if the data range is out of bounds of
+    /// the firmware image.
+    fn data(&self) -> Option<&[u8]> {
+        let fw_start = self.hdr.data_offset as usize;
+        let fw_size = self.hdr.data_size as usize;
+
+        self.fw.get(fw_start..fw_start + fw_size)
+    }
+}
+
 pub(crate) struct ModInfoBuilder<const N: usize>(firmware::ModInfoBuilder<N>);
 
 impl<const N: usize> ModInfoBuilder<N> {

-- 
2.50.1

[PATCH v2 3/8] gpu: nova-core: firmware: process Booter and patch its signature

[Alexandre Courbot]

The Booter signed firmware is an essential part of bringing up the GSP
on Turing and Ampere. It is loaded on the sec2 falcon core and is
responsible for loading and running the RISC-V GSP bootloader into the
GSP core.

Add support for parsing the Booter firmware loaded from userspace, patch
its signatures, and store it into a form that is ready to be loaded and
executed on the sec2 falcon.

We do not run it yet, as its own payload (the GSP bootloader and
firmware image) still need to be prepared.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/nova-core/falcon.rs          |   4 +-
 drivers/gpu/nova-core/firmware.rs        |  25 ++-
 drivers/gpu/nova-core/firmware/booter.rs | 356 +++++++++++++++++++++++++++++++
 drivers/gpu/nova-core/gpu.rs             |  11 +-
 4 files changed, 386 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
index 2dbcdf26697beb7e52083675fc9ea62a6167fef8..7bd13481a6a37783309c2d2621a6b67b81d55cc5 100644
--- a/drivers/gpu/nova-core/falcon.rs
+++ b/drivers/gpu/nova-core/falcon.rs
@@ -293,7 +293,7 @@ pub(crate) trait FalconEngine:
 }
 
 /// Represents a portion of the firmware to be loaded into a particular memory (e.g. IMEM or DMEM).
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub(crate) struct FalconLoadTarget {
     /// Offset from the start of the source object to copy from.
     pub(crate) src_start: u32,
@@ -304,7 +304,7 @@ pub(crate) struct FalconLoadTarget {
 }
 
 /// Parameters for the falcon boot ROM.
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub(crate) struct FalconBromParams {
     /// Offset in `DMEM`` of the firmware's signature.
     pub(crate) pkc_data_offset: u32,
diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
index ccb4d19f8fa76b0e844252dede5f50b37c590571..be190af1e11aec26c18c85324a185d135a16eabe 100644
--- a/drivers/gpu/nova-core/firmware.rs
+++ b/drivers/gpu/nova-core/firmware.rs
@@ -6,6 +6,7 @@
 use core::marker::PhantomData;
 use core::mem::size_of;
 
+use booter::BooterFirmware;
 use kernel::device;
 use kernel::firmware;
 use kernel::prelude::*;
@@ -13,10 +14,13 @@
 use kernel::transmute::FromBytes;
 
 use crate::dma::DmaObject;
+use crate::driver::Bar0;
 use crate::falcon::FalconFirmware;
+use crate::falcon::{sec2::Sec2, Falcon};
 use crate::gpu;
 use crate::gpu::Chipset;
 
+pub(crate) mod booter;
 pub(crate) mod fwsec;
 
 pub(crate) const FIRMWARE_VERSION: &str = "535.113.01";
@@ -24,14 +28,22 @@
 /// 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,
+    /// Runs on the sec2 falcon engine to load and start the GSP bootloader.
+    booter_loader: BooterFirmware,
+    /// Runs on the sec2 falcon engine to stop and unload a running GSP firmware.
+    booter_unloader: BooterFirmware,
     bootloader: firmware::Firmware,
     gsp: firmware::Firmware,
 }
 
 impl Firmware {
-    pub(crate) fn new(dev: &device::Device, chipset: Chipset, ver: &str) -> Result<Firmware> {
+    pub(crate) fn new(
+        dev: &device::Device<device::Bound>,
+        sec2: &Falcon<Sec2>,
+        bar: &Bar0,
+        chipset: Chipset,
+        ver: &str,
+    ) -> Result<Firmware> {
         let mut chip_name = CString::try_from_fmt(fmt!("{chipset}"))?;
         chip_name.make_ascii_lowercase();
         let chip_name = &*chip_name;
@@ -42,8 +54,10 @@ pub(crate) fn new(dev: &device::Device, chipset: Chipset, ver: &str) -> Result<F
         };
 
         Ok(Firmware {
-            booter_load: request("booter_load")?,
-            booter_unload: request("booter_unload")?,
+            booter_loader: request("booter_load")
+                .and_then(|fw| BooterFirmware::new(dev, &fw, sec2, bar))?,
+            booter_unloader: request("booter_unload")
+                .and_then(|fw| BooterFirmware::new(dev, &fw, sec2, bar))?,
             bootloader: request("bootloader")?,
             gsp: request("gsp")?,
         })
@@ -179,7 +193,6 @@ struct BinFirmware<'a> {
     fw: &'a [u8],
 }
 
-#[expect(dead_code)]
 impl<'a> BinFirmware<'a> {
     /// Interpret `fw` as a firmware image starting with a [`BinHdr`], and returns the
     /// corresponding [`BinFirmware`] that can be used to extract its payload.
diff --git a/drivers/gpu/nova-core/firmware/booter.rs b/drivers/gpu/nova-core/firmware/booter.rs
new file mode 100644
index 0000000000000000000000000000000000000000..108649bdf716eeacaae3098b3c29b2de2813c6ee
--- /dev/null
+++ b/drivers/gpu/nova-core/firmware/booter.rs
@@ -0,0 +1,356 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Support for loading and patching the `Booter` firmware. `Booter` is a Heavy Secured firmware
+//! running on [`Sec2`], that is used on Turing/Ampere to load the GSP firmware into the GSP falcon
+//! (and optionally unload it through a separate firmware image).
+
+use core::marker::PhantomData;
+use core::mem::size_of;
+use core::ops::Deref;
+
+use kernel::device;
+use kernel::firmware::Firmware;
+use kernel::prelude::*;
+use kernel::transmute::FromBytes;
+
+use crate::dma::DmaObject;
+use crate::driver::Bar0;
+use crate::falcon::sec2::Sec2;
+use crate::falcon::{Falcon, FalconBromParams, FalconFirmware, FalconLoadParams, FalconLoadTarget};
+use crate::firmware::{BinFirmware, FirmwareDmaObject, FirmwareSignature, Signed, Unsigned};
+
+/// Local convenience function to return a copy of `S` by reinterpreting the bytes starting at
+/// `offset` in `slice`.
+fn frombytes_at<S: FromBytes + Sized>(slice: &[u8], offset: usize) -> Result<S> {
+    slice
+        .get(offset..offset + size_of::<S>())
+        .and_then(S::from_bytes_copy)
+        .ok_or(EINVAL)
+}
+
+/// Heavy-Secured firmware header.
+///
+/// Such firmwares have an application-specific payload that needs to be patched with a given
+/// signature.
+#[repr(C)]
+#[derive(Debug, Clone)]
+struct HsHeaderV2 {
+    /// Offset to the start of the signatures.
+    sig_prod_offset: u32,
+    /// Size in bytes of the signatures.
+    sig_prod_size: u32,
+    /// Offset to a `u32` containing the location at which to patch the signature in the microcode
+    /// image.
+    patch_loc: u32,
+    /// Offset to a `u32` containing the index of the signature to patch.
+    patch_sig: u32,
+    /// Start offset to the signature metadata.
+    meta_data_offset: u32,
+    /// Size in bytes of the signature metadata.
+    meta_data_size: u32,
+    /// Offset to a `u32` containing the number of signatures in the signatures section.
+    num_sig: u32,
+    /// Offset of the application-specific header.
+    header_offset: u32,
+    /// Size in bytes of the application-specific header.
+    header_size: u32,
+}
+
+// SAFETY: all bit patterns are valid for this type, and it doesn't use interior mutability.
+unsafe impl FromBytes for HsHeaderV2 {}
+
+/// Heavy-Secured Firmware image container.
+///
+/// This provides convenient access to the fields of [`HsHeaderV2`] that are actually indices to
+/// read from in the firmware data.
+struct HsFirmwareV2<'a> {
+    hdr: HsHeaderV2,
+    fw: &'a [u8],
+}
+
+impl<'a> HsFirmwareV2<'a> {
+    /// Interprets the header of `bin_fw` as a [`HsHeaderV2`] and returns an instance of
+    /// `HsFirmwareV2` for further parsing.
+    ///
+    /// Fails if the header pointed at by `bin_fw` is not within the bounds of the firmware image.
+    fn new(bin_fw: &BinFirmware<'a>) -> Result<Self> {
+        frombytes_at::<HsHeaderV2>(bin_fw.fw, bin_fw.hdr.header_offset as usize)
+            .map(|hdr| Self { hdr, fw: bin_fw.fw })
+    }
+
+    /// Returns the location at which the signatures should be patched in the microcode image.
+    ///
+    /// Fails if the offset of the patch location is outside the bounds of the firmware
+    /// image.
+    fn patch_location(&self) -> Result<u32> {
+        frombytes_at::<u32>(self.fw, self.hdr.patch_loc as usize)
+    }
+
+    /// Returns an iterator to the signatures of the firmware. The iterator can be empty if the
+    /// firmware is unsigned.
+    ///
+    /// Fails if the pointed signatures are outside the bounds of the firmware image.
+    fn signatures_iter(&'a self) -> Result<impl Iterator<Item = BooterSignature<'a>>> {
+        let num_sig = frombytes_at::<u32>(self.fw, self.hdr.num_sig as usize)?;
+        let iter = match self.hdr.sig_prod_size.checked_div(num_sig) {
+            // If there are no signatures, return an iterator that will yield zero elements.
+            None => (&[] as &[u8]).chunks_exact(1),
+            Some(sig_size) => {
+                let patch_sig = frombytes_at::<u32>(self.fw, self.hdr.patch_sig as usize)?;
+                let signatures_start = (self.hdr.sig_prod_offset + patch_sig) as usize;
+
+                self.fw
+                    // Get signatures range.
+                    .get(signatures_start..signatures_start + self.hdr.sig_prod_size as usize)
+                    .ok_or(EINVAL)?
+                    .chunks_exact(sig_size as usize)
+            }
+        };
+
+        // Map the byte slices into signatures.
+        Ok(iter.map(BooterSignature))
+    }
+}
+
+/// Signature parameters, as defined in the firmware.
+#[repr(C)]
+struct HsSignatureParams {
+    // Fuse version to use.
+    fuse_ver: u32,
+    // Mask of engine IDs this firmware applies to.
+    engine_id_mask: u32,
+    // ID of the microcode.
+    ucode_id: u32,
+}
+
+// SAFETY: all bit patterns are valid for this type, and it doesn't use interior mutability.
+unsafe impl FromBytes for HsSignatureParams {}
+
+impl HsSignatureParams {
+    /// Returns the signature parameters contained in `hs_fw`.
+    ///
+    /// Fails if the meta data parameter of `hs_fw` is outside the bounds of the firmware image, or
+    /// if its size doesn't match that of [`HsSignatureParams`].
+    fn new(hs_fw: &HsFirmwareV2<'_>) -> Result<Self> {
+        let start = hs_fw.hdr.meta_data_offset as usize;
+        let end = start
+            .checked_add(hs_fw.hdr.meta_data_size as usize)
+            .ok_or(EINVAL)?;
+
+        hs_fw
+            .fw
+            .get(start..end)
+            .and_then(Self::from_bytes_copy)
+            .ok_or(EINVAL)
+    }
+}
+
+/// Header for code and data load offsets.
+#[repr(C)]
+#[derive(Debug, Clone)]
+struct HsLoadHeaderV2 {
+    // Offset at which the code starts.
+    os_code_offset: u32,
+    // Total size of the code, for all apps.
+    os_code_size: u32,
+    // Offset at which the data starts.
+    os_data_offset: u32,
+    // Size of the data.
+    os_data_size: u32,
+    // Number of apps following this header. Each app is described by a [`HsLoadHeaderV2App`].
+    num_apps: u32,
+}
+
+// SAFETY: all bit patterns are valid for this type, and it doesn't use interior mutability.
+unsafe impl FromBytes for HsLoadHeaderV2 {}
+
+impl HsLoadHeaderV2 {
+    /// Returns the load header contained in `hs_fw`.
+    ///
+    /// Fails if the header pointed at by `hs_fw` is not within the bounds of the firmware image.
+    fn new(hs_fw: &HsFirmwareV2<'_>) -> Result<Self> {
+        frombytes_at::<Self>(hs_fw.fw, hs_fw.hdr.header_offset as usize)
+    }
+}
+
+/// Header for app code loader.
+#[repr(C)]
+#[derive(Debug, Clone)]
+struct HsLoadHeaderV2App {
+    /// Offset at which to load the app code.
+    offset: u32,
+    /// Length in bytes of the app code.
+    len: u32,
+}
+
+// SAFETY: all bit patterns are valid for this type, and it doesn't use interior mutability.
+unsafe impl FromBytes for HsLoadHeaderV2App {}
+
+impl HsLoadHeaderV2App {
+    /// Returns the [`HsLoadHeaderV2App`] for app `idx` of `hs_fw`.
+    ///
+    /// Fails if `idx` is larger than the number of apps declared in `hs_fw`, or if the header is
+    /// not within the bounds of the firmware image.
+    fn new(hs_fw: &HsFirmwareV2<'_>, idx: u32) -> Result<Self> {
+        let load_hdr = HsLoadHeaderV2::new(hs_fw)?;
+        if idx >= load_hdr.num_apps {
+            Err(EINVAL)
+        } else {
+            frombytes_at::<Self>(
+                hs_fw.fw,
+                (hs_fw.hdr.header_offset as usize)
+                    // Skip the load header...
+                    .checked_add(size_of::<HsLoadHeaderV2>())
+                    // ... and jump to app header `idx`.
+                    .and_then(|offset| {
+                        offset.checked_add((idx as usize).checked_mul(size_of::<Self>())?)
+                    })
+                    .ok_or(EINVAL)?,
+            )
+        }
+    }
+}
+
+/// Signature for Booter firmware. Their size is encoded into the header and not known a compile
+/// time, so we just wrap a byte slices on which we can implement [`FirmwareSignature`].
+struct BooterSignature<'a>(&'a [u8]);
+
+impl<'a> AsRef<[u8]> for BooterSignature<'a> {
+    fn as_ref(&self) -> &[u8] {
+        self.0
+    }
+}
+
+impl<'a> FirmwareSignature<BooterFirmware> for BooterSignature<'a> {}
+
+/// The `Booter` loader firmware, responsible for loading the GSP.
+pub(crate) struct BooterFirmware {
+    // Load parameters for `IMEM` falcon memory.
+    imem_load_target: FalconLoadTarget,
+    // Load parameters for `DMEM` falcon memory.
+    dmem_load_target: FalconLoadTarget,
+    // BROM falcon parameters.
+    brom_params: FalconBromParams,
+    // Device-mapped firmware image.
+    ucode: FirmwareDmaObject<Self, Signed>,
+}
+
+impl FirmwareDmaObject<BooterFirmware, Unsigned> {
+    fn new_booter(dev: &device::Device<device::Bound>, data: &[u8]) -> Result<Self> {
+        DmaObject::from_data(dev, data).map(|ucode| Self(ucode, PhantomData))
+    }
+}
+
+impl BooterFirmware {
+    /// Parses the Booter firmware contained in `fw`, and patches the correct signature so it is
+    /// ready to be loaded and run on `falcon`.
+    pub(crate) fn new(
+        dev: &device::Device<device::Bound>,
+        fw: &Firmware,
+        falcon: &Falcon<<Self as FalconFirmware>::Target>,
+        bar: &Bar0,
+    ) -> Result<Self> {
+        let bin_fw = BinFirmware::new(fw)?;
+        // The binary firmware embeds a Heavy-Secured firmware.
+        let hs_fw = HsFirmwareV2::new(&bin_fw)?;
+        // The Heavy-Secured firmware embeds a firmware load descriptor.
+        let load_hdr = HsLoadHeaderV2::new(&hs_fw)?;
+        // Offset in `ucode` where to patch the signature.
+        let patch_loc = hs_fw.patch_location()?;
+        let sig_params = HsSignatureParams::new(&hs_fw)?;
+        let brom_params = FalconBromParams {
+            // `load_hdr.os_data_offset` is an absolute index, but `pkc_data_offset` is from the
+            // signature patch location.
+            pkc_data_offset: patch_loc
+                .checked_sub(load_hdr.os_data_offset)
+                .ok_or(EINVAL)?,
+            engine_id_mask: u16::try_from(sig_params.engine_id_mask).map_err(|_| EINVAL)?,
+            ucode_id: u8::try_from(sig_params.ucode_id).map_err(|_| EINVAL)?,
+        };
+        let app0 = HsLoadHeaderV2App::new(&hs_fw, 0)?;
+
+        // Object containing the firmware microcode to be signature-patched.
+        let ucode = bin_fw
+            .data()
+            .ok_or(EINVAL)
+            .and_then(|data| FirmwareDmaObject::<Self, _>::new_booter(dev, data))?;
+
+        let ucode_signed = {
+            let mut signatures = hs_fw.signatures_iter()?.peekable();
+
+            if signatures.peek().is_none() {
+                // If there are no signatures, then the firmware is unsigned.
+                ucode.no_patch_signature()
+            } else {
+                // Obtain the version from the fuse register, and extract the corresponding
+                // signature.
+                let reg_fuse_version = falcon.signature_reg_fuse_version(
+                    bar,
+                    brom_params.engine_id_mask,
+                    brom_params.ucode_id,
+                )?;
+
+                let signature = match reg_fuse_version {
+                    // `0` means the last signature should be used.
+                    0 => signatures.last(),
+                    // Otherwise hardware fuse version needs to be substracted to obtain the index.
+                    reg_fuse_version => {
+                        let Some(idx) = sig_params.fuse_ver.checked_sub(reg_fuse_version) else {
+                            dev_err!(dev, "invalid fuse version for Booter firmware\n");
+                            return Err(EINVAL);
+                        };
+                        signatures.nth(idx as usize)
+                    }
+                }
+                .ok_or(EINVAL)?;
+
+                ucode.patch_signature(&signature, patch_loc as usize)?
+            }
+        };
+
+        Ok(Self {
+            imem_load_target: FalconLoadTarget {
+                src_start: app0.offset,
+                dst_start: 0,
+                len: app0.len,
+            },
+            dmem_load_target: FalconLoadTarget {
+                src_start: load_hdr.os_data_offset,
+                dst_start: 0,
+                len: load_hdr.os_data_size,
+            },
+            brom_params,
+            ucode: ucode_signed,
+        })
+    }
+}
+
+impl FalconLoadParams for BooterFirmware {
+    fn imem_load_params(&self) -> FalconLoadTarget {
+        self.imem_load_target.clone()
+    }
+
+    fn dmem_load_params(&self) -> FalconLoadTarget {
+        self.dmem_load_target.clone()
+    }
+
+    fn brom_params(&self) -> FalconBromParams {
+        self.brom_params.clone()
+    }
+
+    fn boot_addr(&self) -> u32 {
+        self.imem_load_target.src_start
+    }
+}
+
+impl Deref for BooterFirmware {
+    type Target = DmaObject;
+
+    fn deref(&self) -> &Self::Target {
+        &self.ucode.0
+    }
+}
+
+impl FalconFirmware for BooterFirmware {
+    type Target = Sec2;
+}
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 8caecaf7dfb4820a96a568a05653dbdf808a3719..54f0e9fd587ae5c4c045096930c0548fb1ef1b86 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -269,7 +269,6 @@ pub(crate) fn new(
     ) -> Result<impl PinInit<Self>> {
         let bar = devres_bar.access(pdev.as_ref())?;
         let spec = Spec::new(bar)?;
-        let fw = Firmware::new(pdev.as_ref(), spec.chipset, FIRMWARE_VERSION)?;
 
         dev_info!(
             pdev.as_ref(),
@@ -293,7 +292,15 @@ pub(crate) fn new(
         )?;
         gsp_falcon.clear_swgen0_intr(bar);
 
-        let _sec2_falcon = Falcon::<Sec2>::new(pdev.as_ref(), spec.chipset, bar, true)?;
+        let sec2_falcon = Falcon::<Sec2>::new(pdev.as_ref(), spec.chipset, bar, true)?;
+
+        let fw = Firmware::new(
+            pdev.as_ref(),
+            &sec2_falcon,
+            bar,
+            spec.chipset,
+            FIRMWARE_VERSION,
+        )?;
 
         let fb_layout = FbLayout::new(spec.chipset, bar)?;
         dev_dbg!(pdev.as_ref(), "{:#x?}\n", fb_layout);

-- 
2.50.1

[PATCH v2 4/8] gpu: nova-core: firmware: process the GSP bootloader

[Alexandre Courbot]

The GSP bootloader is a small RISC-V firmware that is loaded by Booter
onto the GSP core and is in charge of loading, validating, and starting
the actual GSP firmware.

It is a regular binary firmware file containing a specific header.
Create a type holding the DMA-mapped firmware as well as useful
information extracted from the header, and hook it into our firmware
structure for later use.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/nova-core/firmware.rs       |  7 ++-
 drivers/gpu/nova-core/firmware/riscv.rs | 89 +++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
index be190af1e11aec26c18c85324a185d135a16eabe..9bee0e0a0ab99d10be7e56d366970fdf4c813fc4 100644
--- a/drivers/gpu/nova-core/firmware.rs
+++ b/drivers/gpu/nova-core/firmware.rs
@@ -12,6 +12,7 @@
 use kernel::prelude::*;
 use kernel::str::CString;
 use kernel::transmute::FromBytes;
+use riscv::RiscvFirmware;
 
 use crate::dma::DmaObject;
 use crate::driver::Bar0;
@@ -22,6 +23,7 @@
 
 pub(crate) mod booter;
 pub(crate) mod fwsec;
+pub(crate) mod riscv;
 
 pub(crate) const FIRMWARE_VERSION: &str = "535.113.01";
 
@@ -32,7 +34,8 @@ pub(crate) struct Firmware {
     booter_loader: BooterFirmware,
     /// Runs on the sec2 falcon engine to stop and unload a running GSP firmware.
     booter_unloader: BooterFirmware,
-    bootloader: firmware::Firmware,
+    /// GSP bootloader, verifies the GSP firmware before loading and running it.
+    gsp_bootloader: RiscvFirmware,
     gsp: firmware::Firmware,
 }
 
@@ -58,7 +61,7 @@ pub(crate) fn new(
                 .and_then(|fw| BooterFirmware::new(dev, &fw, sec2, bar))?,
             booter_unloader: request("booter_unload")
                 .and_then(|fw| BooterFirmware::new(dev, &fw, sec2, bar))?,
-            bootloader: request("bootloader")?,
+            gsp_bootloader: request("bootloader").and_then(|fw| RiscvFirmware::new(dev, &fw))?,
             gsp: request("gsp")?,
         })
     }
diff --git a/drivers/gpu/nova-core/firmware/riscv.rs b/drivers/gpu/nova-core/firmware/riscv.rs
new file mode 100644
index 0000000000000000000000000000000000000000..926883230f2fe4e3327713e28b7fae31ebee60bb
--- /dev/null
+++ b/drivers/gpu/nova-core/firmware/riscv.rs
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Support for firmware binaries designed to run on a RISC-V cores. Such firmwares have a
+//! dedicated header.
+
+use kernel::device;
+use kernel::firmware::Firmware;
+use kernel::prelude::*;
+use kernel::transmute::FromBytes;
+
+use crate::dma::DmaObject;
+use crate::firmware::BinFirmware;
+
+/// Descriptor for microcode running on a RISC-V core.
+#[repr(C)]
+#[derive(Debug)]
+struct RmRiscvUCodeDesc {
+    version: u32,
+    bootloader_offset: u32,
+    bootloader_size: u32,
+    bootloader_param_offset: u32,
+    bootloader_param_size: u32,
+    riscv_elf_offset: u32,
+    riscv_elf_size: u32,
+    app_version: u32,
+    manifest_offset: u32,
+    manifest_size: u32,
+    monitor_data_offset: u32,
+    monitor_data_size: u32,
+    monitor_code_offset: u32,
+    monitor_code_size: u32,
+}
+
+// SAFETY: all bit patterns are valid for this type, and it doesn't use interior mutability.
+unsafe impl FromBytes for RmRiscvUCodeDesc {}
+
+impl RmRiscvUCodeDesc {
+    /// Interprets the header of `bin_fw` as a [`RmRiscvUCodeDesc`] and returns it.
+    ///
+    /// Fails if the header pointed at by `bin_fw` is not within the bounds of the firmware image.
+    fn new(bin_fw: &BinFirmware<'_>) -> Result<Self> {
+        let offset = bin_fw.hdr.header_offset as usize;
+
+        bin_fw
+            .fw
+            .get(offset..offset + size_of::<Self>())
+            .and_then(Self::from_bytes_copy)
+            .ok_or(EINVAL)
+    }
+}
+
+/// A parsed firmware for a RISC-V core, ready to be loaded and run.
+#[expect(unused)]
+pub(crate) struct RiscvFirmware {
+    /// Offset at which the code starts in the firmware image.
+    code_offset: u32,
+    /// Offset at which the data starts in the firmware image.
+    data_offset: u32,
+    /// Offset at which the manifest starts in the firmware image.
+    manifest_offset: u32,
+    /// Application version.
+    app_version: u32,
+    /// Device-mapped firmware image.
+    ucode: DmaObject,
+}
+
+impl RiscvFirmware {
+    // Parses the RISC-V firmware image contained in `fw`.
+    pub(crate) fn new(dev: &device::Device<device::Bound>, fw: &Firmware) -> Result<Self> {
+        let bin_fw = BinFirmware::new(fw)?;
+
+        let riscv_desc = RmRiscvUCodeDesc::new(&bin_fw)?;
+
+        let ucode = {
+            let start = bin_fw.hdr.data_offset as usize;
+            let len = bin_fw.hdr.data_size as usize;
+
+            DmaObject::from_data(dev, fw.data().get(start..start + len).ok_or(EINVAL)?)?
+        };
+
+        Ok(Self {
+            ucode,
+            code_offset: riscv_desc.monitor_code_offset,
+            data_offset: riscv_desc.monitor_data_offset,
+            manifest_offset: riscv_desc.manifest_offset,
+            app_version: riscv_desc.app_version,
+        })
+    }
+}

-- 
2.50.1

[PATCH v2 5/8] gpu: nova-core: firmware: process and prepare the GSP firmware

[Alexandre Courbot]

The GSP firmware is a binary blob that is verified, loaded, and run by
the GSP bootloader. Its presentation is a bit peculiar as the GSP
bootloader expects to be given a DMA address to a 3-levels page table
mapping the GSP firmware at address 0 of its own address space.

Prepare such a structure containing the DMA-mapped firmware as well as
the DMA-mapped page tables, and a way to obtain the DMA handle of the
level 0 page table.

As we are performing the required ELF section parsing and radix3 page
table building, remove these items from the TODO file.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 Documentation/gpu/nova/core/todo.rst  |  17 -----
 drivers/gpu/nova-core/firmware.rs     | 110 +++++++++++++++++++++++++++++++-
 drivers/gpu/nova-core/firmware/gsp.rs | 117 ++++++++++++++++++++++++++++++++++
 drivers/gpu/nova-core/gsp.rs          |   4 ++
 drivers/gpu/nova-core/nova_core.rs    |   1 +
 5 files changed, 229 insertions(+), 20 deletions(-)

diff --git a/Documentation/gpu/nova/core/todo.rst b/Documentation/gpu/nova/core/todo.rst
index 89431fec9041b1f35cc55799c91f48dc6bc918eb..0972cb905f7ae64dfbaef4808276757319009e9c 100644
--- a/Documentation/gpu/nova/core/todo.rst
+++ b/Documentation/gpu/nova/core/todo.rst
@@ -229,23 +229,6 @@ Rust abstraction for debugfs APIs.
 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
-
 Initial Devinit support
 -----------------------
 
diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
index 9bee0e0a0ab99d10be7e56d366970fdf4c813fc4..fb751287e938e6a323db185ff8c4ba2781d25285 100644
--- a/drivers/gpu/nova-core/firmware.rs
+++ b/drivers/gpu/nova-core/firmware.rs
@@ -7,6 +7,7 @@
 use core::mem::size_of;
 
 use booter::BooterFirmware;
+use gsp::GspFirmware;
 use kernel::device;
 use kernel::firmware;
 use kernel::prelude::*;
@@ -19,14 +20,100 @@
 use crate::falcon::FalconFirmware;
 use crate::falcon::{sec2::Sec2, Falcon};
 use crate::gpu;
-use crate::gpu::Chipset;
+use crate::gpu::{Architecture, Chipset};
 
 pub(crate) mod booter;
 pub(crate) mod fwsec;
+pub(crate) mod gsp;
 pub(crate) mod riscv;
 
 pub(crate) const FIRMWARE_VERSION: &str = "535.113.01";
 
+/// Ad-hoc and temporary module to extract sections from ELF images.
+///
+/// Some firmware images are currently packaged as ELF files, where sections names are used as keys
+/// to specific and related bits of data. Future firmware versions are scheduled to move away from
+/// that scheme before nova-core becomes stable, which means this module will eventually be
+/// removed.
+mod elf {
+    use kernel::bindings;
+    use kernel::str::CStr;
+    use kernel::transmute::FromBytes;
+
+    /// Newtype to provide a [`FromBytes`] implementation.
+    #[repr(transparent)]
+    struct Elf64Hdr(bindings::elf64_hdr);
+
+    // SAFETY: all bit patterns are valid for this type, and it doesn't use interior mutability.
+    unsafe impl FromBytes for Elf64Hdr {}
+
+    /// Tries to extract section with name `name` from the ELF64 image `elf`, and returns it.
+    pub(super) fn elf64_section<'a, 'b>(elf: &'a [u8], name: &'b str) -> Option<&'a [u8]> {
+        let hdr = &elf
+            .get(0..size_of::<bindings::elf64_hdr>())
+            .and_then(Elf64Hdr::from_bytes)?
+            .0;
+
+        // Get all the section headers.
+        let shdr = {
+            let shdr_num = usize::from(hdr.e_shnum);
+            let shdr_start = usize::try_from(hdr.e_shoff).ok()?;
+            let shdr_end = shdr_num
+                .checked_mul(size_of::<bindings::elf64_shdr>())
+                .and_then(|v| v.checked_add(shdr_start))?;
+
+            elf.get(shdr_start..shdr_end)
+                .map(|slice| slice.as_ptr())
+                .filter(|ptr| ptr.align_offset(align_of::<bindings::elf64_shdr>()) == 0)
+                // `FromBytes::from_bytes` does not support slices yet, so build it manually.
+                //
+                // SAFETY:
+                // * `get` guarantees that the slice is within the bounds of `elf` and of size
+                //   `elf64_shdr * shdr_num`.
+                // * We checked that `ptr` had the correct alignment for `elf64_shdr`.
+                .map(|ptr| unsafe {
+                    core::slice::from_raw_parts(ptr.cast::<bindings::elf64_shdr>(), shdr_num)
+                })?
+        };
+
+        // Get the strings table.
+        let strhdr = shdr.get(usize::from(hdr.e_shstrndx))?;
+
+        // Find the section which name matches `name` and return it.
+        shdr.iter()
+            .find(|sh| {
+                let Some(name_idx) = strhdr
+                    .sh_offset
+                    .checked_add(u64::from(sh.sh_name))
+                    .and_then(|idx| usize::try_from(idx).ok())
+                else {
+                    return false;
+                };
+
+                // Get the start of the name.
+                elf.get(name_idx..)
+                    // Stop at the first `0`.
+                    .and_then(|nstr| nstr.get(0..=nstr.iter().position(|b| *b == 0)?))
+                    // Convert into CStr. This should never fail because of the line above.
+                    .and_then(|nstr| CStr::from_bytes_with_nul(nstr).ok())
+                    // Convert into str.
+                    .and_then(|c_str| c_str.to_str().ok())
+                    // Check that the name matches.
+                    .map(|str| str == name)
+                    .unwrap_or(false)
+            })
+            // Return the slice containing the section.
+            .and_then(|sh| {
+                let start = usize::try_from(sh.sh_offset).ok()?;
+                let end = usize::try_from(sh.sh_size)
+                    .ok()
+                    .and_then(|sh_size| start.checked_add(sh_size))?;
+
+                elf.get(start..end)
+            })
+    }
+}
+
 /// Structure encapsulating the firmware blobs required for the GPU to operate.
 #[expect(dead_code)]
 pub(crate) struct Firmware {
@@ -36,7 +123,10 @@ pub(crate) struct Firmware {
     booter_unloader: BooterFirmware,
     /// GSP bootloader, verifies the GSP firmware before loading and running it.
     gsp_bootloader: RiscvFirmware,
-    gsp: firmware::Firmware,
+    /// GSP firmware.
+    gsp: Pin<KBox<GspFirmware>>,
+    /// GSP signatures, to be passed as parameter to the bootloader for validation.
+    gsp_sigs: DmaObject,
 }
 
 impl Firmware {
@@ -56,13 +146,27 @@ pub(crate) fn new(
                 .and_then(|path| firmware::Firmware::request(&path, dev))
         };
 
+        let gsp_fw = request("gsp")?;
+        let gsp = elf::elf64_section(gsp_fw.data(), ".fwimage")
+            .ok_or(EINVAL)
+            .map(|data| GspFirmware::new(dev, data))?;
+
+        let gsp_sigs_section = match chipset.arch() {
+            Architecture::Ampere => ".fwsignature_ga10x",
+            _ => return Err(ENOTSUPP),
+        };
+        let gsp_sigs = elf::elf64_section(gsp_fw.data(), gsp_sigs_section)
+            .ok_or(EINVAL)
+            .and_then(|data| DmaObject::from_data(dev, data))?;
+
         Ok(Firmware {
             booter_loader: request("booter_load")
                 .and_then(|fw| BooterFirmware::new(dev, &fw, sec2, bar))?,
             booter_unloader: request("booter_unload")
                 .and_then(|fw| BooterFirmware::new(dev, &fw, sec2, bar))?,
             gsp_bootloader: request("bootloader").and_then(|fw| RiscvFirmware::new(dev, &fw))?,
-            gsp: request("gsp")?,
+            gsp: KBox::pin_init(gsp, GFP_KERNEL)?,
+            gsp_sigs,
         })
     }
 }
diff --git a/drivers/gpu/nova-core/firmware/gsp.rs b/drivers/gpu/nova-core/firmware/gsp.rs
new file mode 100644
index 0000000000000000000000000000000000000000..f37bd619bfb71629ed86ee8b7828971bbe4c5916
--- /dev/null
+++ b/drivers/gpu/nova-core/firmware/gsp.rs
@@ -0,0 +1,117 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use kernel::device;
+use kernel::dma::DataDirection;
+use kernel::dma::DmaAddress;
+use kernel::prelude::*;
+use kernel::scatterlist::Owned;
+use kernel::scatterlist::SGTable;
+
+use crate::dma::DmaObject;
+use crate::gsp::GSP_PAGE_SIZE;
+
+/// A device-mapped firmware with a set of (also device-mapped) pages tables mapping the firmware
+/// to the start of their own address space, also known as a `Radix3` firmware.
+#[pin_data]
+pub(crate) struct GspFirmware {
+    /// The GSP firmware inside a [`VVec`], device-mapped via a SG table.
+    #[pin]
+    fw: SGTable<Owned<VVec<u8>>>,
+    /// The level 2 page table, mapping [`Self::fw`] at its beginning.
+    #[pin]
+    lvl2: SGTable<Owned<VVec<u8>>>,
+    /// The level 1 page table, mapping [`Self::lvl2`] at its beginning.
+    #[pin]
+    lvl1: SGTable<Owned<VVec<u8>>>,
+    /// The level 0 page table, mapping [`Self::lvl1`] at its beginning.
+    lvl0: DmaObject,
+    /// Size in bytes of the firmware contained in [`Self::fw`].
+    pub size: usize,
+}
+
+impl GspFirmware {
+    /// Maps the GSP firmware image `fw` into `dev`'s address-space, and creates the page tables
+    /// expected by the GSP bootloader to load it.
+    pub(crate) fn new<'a>(
+        dev: &'a device::Device<device::Bound>,
+        fw: &'a [u8],
+    ) -> impl PinInit<Self, Error> + 'a {
+        try_pin_init!(&this in Self {
+            fw <- {
+                // Move the firmware into a vmalloc'd vector and map it into the device address
+                // space.
+                VVec::with_capacity(fw.len(), GFP_KERNEL)
+                .and_then(|mut v| {
+                    v.extend_from_slice(fw, GFP_KERNEL)?;
+                    Ok(v)
+                })
+                .map_err(|_| ENOMEM)
+                .map(|v| SGTable::new(dev, v, DataDirection::ToDevice, GFP_KERNEL))?
+            },
+            lvl2 <- {
+                // Allocate the level 2 page table, map the firmware onto it, and map it into the
+                // device address space.
+                // SAFETY: `this` is a valid pointer, and `fw` has been initialized.
+                let fw_sg_table = unsafe { &(*this.as_ptr()).fw };
+                VVec::<u8>::with_capacity(
+                    fw_sg_table.iter().count() * core::mem::size_of::<u64>(),
+                    GFP_KERNEL,
+                )
+                .map_err(|_| ENOMEM)
+                .and_then(|lvl2| map_into_lvl(fw_sg_table, lvl2))
+                .map(|lvl2| SGTable::new(dev, lvl2, DataDirection::ToDevice, GFP_KERNEL))?
+            },
+            lvl1 <- {
+                // Allocate the level 1 page table, map the level 2 page table onto it, and map it
+                // into the device address space.
+                // SAFETY: `this` is a valid pointer, and `lvl2` has been initialized.
+                let lvl2_sg_table = unsafe { &(*this.as_ptr()).lvl2 };
+                VVec::<u8>::with_capacity(
+                    lvl2_sg_table.iter().count() * core::mem::size_of::<u64>(),
+                    GFP_KERNEL,
+                )
+                .map_err(|_| ENOMEM)
+                .and_then(|lvl1| map_into_lvl(lvl2_sg_table, lvl1))
+                .map(|lvl1| SGTable::new(dev, lvl1, DataDirection::ToDevice, GFP_KERNEL))?
+            },
+            lvl0: {
+                // Allocate the level 0 page table as a device-visible DMA object, and map the
+                // level 1 page table onto it.
+                // SAFETY: `this` is a valid pointer, and `lvl1` has been initialized.
+                let lvl1_sg_table = unsafe { &(*this.as_ptr()).lvl1 };
+                let mut lvl0 = DmaObject::new(dev, GSP_PAGE_SIZE)?;
+                // SAFETY: we are the only owner of this newly-created object, making races
+                // impossible.
+                let lvl0_slice = unsafe { lvl0.as_slice_mut(0, GSP_PAGE_SIZE) }?;
+                lvl0_slice[0..core::mem::size_of::<u64>()].copy_from_slice(
+                    #[allow(clippy::useless_conversion)]
+                    &(u64::from(lvl1_sg_table.iter().next().unwrap().dma_address())).to_le_bytes(),
+                );
+
+                lvl0
+            },
+            size: fw.len(),
+        })
+    }
+
+    #[expect(unused)]
+    /// Returns the DMA handle of the level 0 page table.
+    pub(crate) fn lvl0_dma_handle(&self) -> DmaAddress {
+        self.lvl0.dma_handle()
+    }
+}
+
+/// Create a linear mapping the device mapping of the buffer described by `sg_table` into `dst`.
+fn map_into_lvl(sg_table: &SGTable<Owned<VVec<u8>>>, mut dst: VVec<u8>) -> Result<VVec<u8>> {
+    for sg_entry in sg_table.iter() {
+        // Number of pages we need to map.
+        let num_pages = (sg_entry.dma_len() as usize).div_ceil(GSP_PAGE_SIZE);
+
+        for i in 0..num_pages {
+            let entry = sg_entry.dma_address() + (i as u64 * GSP_PAGE_SIZE as u64);
+            dst.extend_from_slice(&entry.to_le_bytes(), GFP_KERNEL)?;
+        }
+    }
+
+    Ok(dst)
+}
diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs
new file mode 100644
index 0000000000000000000000000000000000000000..a0e7ec5f6c9c959d57540b3ebf4b782f2e002b08
--- /dev/null
+++ b/drivers/gpu/nova-core/gsp.rs
@@ -0,0 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
+
+pub(crate) const GSP_PAGE_SHIFT: usize = 12;
+pub(crate) const GSP_PAGE_SIZE: usize = 1 << GSP_PAGE_SHIFT;
diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
index cb2bbb30cba142265b354c9acf70349a6e40759e..fffcaee2249fe6cd7f55a7291c1e44be42e791d9 100644
--- a/drivers/gpu/nova-core/nova_core.rs
+++ b/drivers/gpu/nova-core/nova_core.rs
@@ -9,6 +9,7 @@
 mod firmware;
 mod gfw;
 mod gpu;
+mod gsp;
 mod regs;
 mod util;
 mod vbios;

-- 
2.50.1

[PATCH v2 6/8] gpu: nova-core: firmware: use 570.144 firmware

[Alexandre Courbot]

570.144 is the latest available into linux-firmware as of this commit,
and the one we will use to start development of nova-core. It should
eventually be dropped for a newer version before the driver becomes able
to do anything useful. The newer firmware is expected to iron out some
of the inelegances of 570.144, notably related to packaging.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/nova-core/firmware.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
index fb751287e938e6a323db185ff8c4ba2781d25285..f296dee224e48b2a4e20d06f8b36d8d1e5f08c53 100644
--- a/drivers/gpu/nova-core/firmware.rs
+++ b/drivers/gpu/nova-core/firmware.rs
@@ -27,7 +27,7 @@
 pub(crate) mod gsp;
 pub(crate) mod riscv;
 
-pub(crate) const FIRMWARE_VERSION: &str = "535.113.01";
+pub(crate) const FIRMWARE_VERSION: &str = "570.144";
 
 /// Ad-hoc and temporary module to extract sections from ELF images.
 ///

-- 
2.50.1

[PATCH v2 7/8] gpu: nova-core: Add base files for r570.144 firmware bindings

[Alexandre Courbot]

From: Alistair Popple <apopple@nvidia.com>

Interacting with the GSP currently requires using definitions from C
header files. Rust definitions for the types needed for Nova core will
be generated using the Rust bindgen tool. This patch adds the base
module to allow inclusion of the generated bindings. The generated
bindings themselves are added by subsequent patches when they are first
used.

Currently we only intend to support a single firmware version, 570.144,
with these bindings. Longer term we intend to move to a more stable GSP
interface that isn't tied to specific firmware versions.

[acourbot@nvidia.com: adapt the bindings module comment a bit.]

Signed-off-by: Alistair Popple <apopple@nvidia.com>
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/nova-core/nova_core.rs              |  1 +
 drivers/gpu/nova-core/nvfw.rs                   |  3 +++
 drivers/gpu/nova-core/nvfw/r570_144.rs          | 29 +++++++++++++++++++++++++
 drivers/gpu/nova-core/nvfw/r570_144_bindings.rs |  1 +
 4 files changed, 34 insertions(+)

diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
index fffcaee2249fe6cd7f55a7291c1e44be42e791d9..db197498b0b7b1ff9234ef6645a4ea5ff44bd285 100644
--- a/drivers/gpu/nova-core/nova_core.rs
+++ b/drivers/gpu/nova-core/nova_core.rs
@@ -10,6 +10,7 @@
 mod gfw;
 mod gpu;
 mod gsp;
+mod nvfw;
 mod regs;
 mod util;
 mod vbios;
diff --git a/drivers/gpu/nova-core/nvfw.rs b/drivers/gpu/nova-core/nvfw.rs
new file mode 100644
index 0000000000000000000000000000000000000000..7c5baccc34a2387c30e51f93d3ae039b14b6b83a
--- /dev/null
+++ b/drivers/gpu/nova-core/nvfw.rs
@@ -0,0 +1,3 @@
+// SPDX-License-Identifier: GPL-2.0
+
+mod r570_144;
diff --git a/drivers/gpu/nova-core/nvfw/r570_144.rs b/drivers/gpu/nova-core/nvfw/r570_144.rs
new file mode 100644
index 0000000000000000000000000000000000000000..2e7bba80fa8b9c5fcb4e26887825d2cca3f7b6b7
--- /dev/null
+++ b/drivers/gpu/nova-core/nvfw/r570_144.rs
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Firmware bindings.
+//!
+//! Imports the generated bindings by `bindgen`.
+//!
+//! This module may not be directly used. Please abstract or re-export the needed symbols in the
+//! parent module instead.
+
+#![cfg_attr(test, allow(deref_nullptr))]
+#![cfg_attr(test, allow(unaligned_references))]
+#![cfg_attr(test, allow(unsafe_op_in_unsafe_fn))]
+#![allow(
+    dead_code,
+    unused_imports,
+    clippy::all,
+    clippy::undocumented_unsafe_blocks,
+    clippy::ptr_as_ptr,
+    clippy::ref_as_ptr,
+    missing_docs,
+    non_camel_case_types,
+    non_upper_case_globals,
+    non_snake_case,
+    improper_ctypes,
+    unreachable_pub,
+    unsafe_op_in_unsafe_fn
+)]
+use kernel::ffi;
+include!("r570_144_bindings.rs");
diff --git a/drivers/gpu/nova-core/nvfw/r570_144_bindings.rs b/drivers/gpu/nova-core/nvfw/r570_144_bindings.rs
new file mode 100644
index 0000000000000000000000000000000000000000..cec5940325151e407aa90128a35cb683afd436d7
--- /dev/null
+++ b/drivers/gpu/nova-core/nvfw/r570_144_bindings.rs
@@ -0,0 +1 @@
+// SPDX-License-Identifier: GPL-2.0

-- 
2.50.1

[PATCH v2 8/8] gpu: nova-core: compute layout of more framebuffer regions required for GSP

[Alexandre Courbot]

Compute more of the required FB layout information to boot the GSP
firmware.

This information is dependent on the firmware itself, so first we need
to import and abstract the required firmware bindings in the `nvfw`
module.

Then, a new FB HAL method is introduced in `fb::hal` that uses these
bindings and hardware information to compute the correct layout
information.

This information is then used in `fb` and the result made visible in
`FbLayout`.

These 3 things are grouped into the same patch to avoid lots of unused
warnings that would be tedious to work around. As they happen in
different files, they should not be too difficult to track separately.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/nova-core/fb.rs                     | 112 ++++++++++++++++++++-
 drivers/gpu/nova-core/fb/hal.rs                 |   4 +
 drivers/gpu/nova-core/fb/hal/ga100.rs           |   6 +-
 drivers/gpu/nova-core/fb/hal/ga102.rs           |  14 ++-
 drivers/gpu/nova-core/fb/hal/tu102.rs           |  14 +++
 drivers/gpu/nova-core/firmware.rs               |   4 +-
 drivers/gpu/nova-core/firmware/riscv.rs         |   2 +-
 drivers/gpu/nova-core/gpu.rs                    |   2 +-
 drivers/gpu/nova-core/gsp.rs                    |   3 +
 drivers/gpu/nova-core/nvfw.rs                   |  39 ++++++++
 drivers/gpu/nova-core/nvfw/r570_144.rs          |   1 -
 drivers/gpu/nova-core/nvfw/r570_144_bindings.rs | 125 ++++++++++++++++++++++++
 12 files changed, 317 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/nova-core/fb.rs b/drivers/gpu/nova-core/fb.rs
index b0e860498b883815b3861b8717f8ee1832d25440..a3eb063f86b3a06a7ad01e684919115abf5e28da 100644
--- a/drivers/gpu/nova-core/fb.rs
+++ b/drivers/gpu/nova-core/fb.rs
@@ -10,7 +10,11 @@
 
 use crate::dma::DmaObject;
 use crate::driver::Bar0;
+use crate::firmware::gsp::GspFirmware;
+use crate::firmware::riscv::RiscvFirmware;
 use crate::gpu::Chipset;
+use crate::gsp::GSP_HEAP_ALIGNMENT;
+use crate::nvfw::{self, LibosParams};
 use crate::regs;
 
 mod hal;
@@ -81,20 +85,80 @@ pub(crate) fn unregister(&self, bar: &Bar0) {
     }
 }
 
+/// Heap memory requirements for the GSP firmware.
+pub(crate) struct GspFwHeapParams {
+    /// Libos parameters in effect.
+    pub libos: &'static LibosParams,
+    /// The amount of heap memory used by GSP-RM boot and initialization, up and including the
+    /// first client subdevice allocation, in bytes.
+    pub base_rm_size: u64,
+}
+
+impl GspFwHeapParams {
+    /// Returns the amount of memory (in bytes) to allocate for the WPR heap for a framebuffer size
+    /// of `fb_size` (in bytes).
+    ///
+    /// Returns `EOVERFLOW` if the computation overflows.
+    pub(crate) fn wpr_heap_size(&self, fb_size: u64) -> Result<u64> {
+        let fb_size_gb = fb_size.div_ceil(SZ_1G as u64);
+
+        // The WPR heap will contain the following:
+        let size =
+            // LIBOS carveout,
+            self.libos.carveout_size
+            // RM boot working memory,
+            + self.base_rm_size
+            // One RM client,
+            + u64::from(nvfw::GSP_FW_HEAP_PARAM_CLIENT_ALLOC_SIZE)
+                .align_up(GSP_HEAP_ALIGNMENT)
+                .ok_or(EOVERFLOW)?
+            // Overhead for memory management.
+            + u64::from(nvfw::GSP_FW_HEAP_PARAM_SIZE_PER_GB_FB)
+                .checked_mul(fb_size_gb)
+                .and_then(|heap_size| heap_size.align_up(GSP_HEAP_ALIGNMENT))
+                .ok_or(EOVERFLOW)?;
+
+        // Clamp to the supported heap sizes.
+        Ok(size.clamp(
+            self.libos.allowed_heap_size.start,
+            self.libos.allowed_heap_size.end - 1,
+        ))
+    }
+}
+
 /// Layout of the GPU framebuffer memory.
 ///
 /// Contains ranges of GPU memory reserved for a given purpose during the GSP boot process.
 #[derive(Debug)]
 #[expect(dead_code)]
 pub(crate) struct FbLayout {
+    /// Range of the framebuffer. Starts at `0`.
     pub(crate) fb: Range<u64>,
+    /// VGA workspace, small area of reserved memory at the end of the framebuffer.
     pub(crate) vga_workspace: Range<u64>,
+    /// FRTS range.
     pub(crate) frts: Range<u64>,
+    /// Memory area containing the GSP bootloader image.
+    pub(crate) boot: Range<u64>,
+    /// Memory area containing the GSP firmware image.
+    pub(crate) elf: Range<u64>,
+    /// WPR2 heap.
+    pub(crate) wpr2_heap: Range<u64>,
+    // WPR2 region range, starting with an instance of `GspFwWprMeta`.
+    pub(crate) wpr2: Range<u64>,
+    pub(crate) heap: Range<u64>,
+    pub(crate) vf_partition_count: u8,
 }
 
 impl FbLayout {
-    /// Computes the FB layout.
-    pub(crate) fn new(chipset: Chipset, bar: &Bar0) -> Result<Self> {
+    /// Computes the FB layout for `chipset`, for running the `bl` GSP bootloader and `gsp` GSP
+    /// firmware.
+    pub(crate) fn new(
+        chipset: Chipset,
+        bar: &Bar0,
+        bl: &RiscvFirmware,
+        gsp: &GspFirmware,
+    ) -> Result<Self> {
         let hal = hal::fb_hal(chipset);
 
         let fb = {
@@ -138,10 +202,54 @@ pub(crate) fn new(chipset: Chipset, bar: &Bar0) -> Result<Self> {
             frts_base..frts_base + FRTS_SIZE
         };
 
+        let boot = {
+            const BOOTLOADER_DOWN_ALIGN: Alignment = Alignment::new(SZ_4K);
+            let bootloader_size = bl.ucode.size() as u64;
+            let bootloader_base = (frts.start - bootloader_size).align_down(BOOTLOADER_DOWN_ALIGN);
+
+            bootloader_base..bootloader_base + bootloader_size
+        };
+
+        let elf = {
+            const ELF_DOWN_ALIGN: Alignment = Alignment::new(SZ_64K);
+            let elf_size = gsp.size as u64;
+            let elf_addr = (boot.start - elf_size).align_down(ELF_DOWN_ALIGN);
+
+            elf_addr..elf_addr + elf_size
+        };
+
+        let wpr2_heap = {
+            const WPR2_HEAP_DOWN_ALIGN: Alignment = Alignment::new(SZ_1M);
+            let wpr2_heap_size = hal.heap_params().wpr_heap_size(fb.end)?;
+            let wpr2_heap_addr = (elf.start - wpr2_heap_size).align_down(WPR2_HEAP_DOWN_ALIGN);
+
+            wpr2_heap_addr..(elf.start).align_down(WPR2_HEAP_DOWN_ALIGN)
+        };
+
+        let wpr2 = {
+            const WPR2_DOWN_ALIGN: Alignment = Alignment::new(SZ_1M);
+            let wpr2_addr = (wpr2_heap.start - size_of::<nvfw::GspFwWprMeta>() as u64)
+                .align_down(WPR2_DOWN_ALIGN);
+
+            wpr2_addr..frts.end
+        };
+
+        let heap = {
+            const HEAP_SIZE: u64 = SZ_1M as u64;
+
+            wpr2.start - HEAP_SIZE..wpr2.start
+        };
+
         Ok(Self {
             fb,
             vga_workspace,
             frts,
+            boot,
+            elf,
+            wpr2_heap,
+            wpr2,
+            heap,
+            vf_partition_count: 0,
         })
     }
 }
diff --git a/drivers/gpu/nova-core/fb/hal.rs b/drivers/gpu/nova-core/fb/hal.rs
index 2f914948bb9a9842fd00a4c6381420b74de81c3f..2bfde29dd3602dd150fb6bdb11072d000a32fec8 100644
--- a/drivers/gpu/nova-core/fb/hal.rs
+++ b/drivers/gpu/nova-core/fb/hal.rs
@@ -3,6 +3,7 @@
 use kernel::prelude::*;
 
 use crate::driver::Bar0;
+use crate::fb::GspFwHeapParams;
 use crate::gpu::Chipset;
 
 mod ga100;
@@ -23,6 +24,9 @@ pub(crate) trait FbHal {
 
     /// Returns the VRAM size, in bytes.
     fn vidmem_size(&self, bar: &Bar0) -> u64;
+
+    /// Returns the heap memory requirements to start the GSP firmware.
+    fn heap_params(&self) -> GspFwHeapParams;
 }
 
 /// Returns the HAL corresponding to `chipset`.
diff --git a/drivers/gpu/nova-core/fb/hal/ga100.rs b/drivers/gpu/nova-core/fb/hal/ga100.rs
index 871c42bf033acd0b9c5735c43d408503075099af..19fc4862f3d88c91d741aa951faa24703aa1d1e9 100644
--- a/drivers/gpu/nova-core/fb/hal/ga100.rs
+++ b/drivers/gpu/nova-core/fb/hal/ga100.rs
@@ -5,7 +5,7 @@
 use kernel::prelude::*;
 
 use crate::driver::Bar0;
-use crate::fb::hal::FbHal;
+use crate::fb::hal::{FbHal, GspFwHeapParams};
 use crate::regs;
 
 use super::tu102::FLUSH_SYSMEM_ADDR_SHIFT;
@@ -51,6 +51,10 @@ fn supports_display(&self, bar: &Bar0) -> bool {
     fn vidmem_size(&self, bar: &Bar0) -> u64 {
         super::tu102::vidmem_size_gp102(bar)
     }
+
+    fn heap_params(&self) -> GspFwHeapParams {
+        super::tu102::heap_params_tu102()
+    }
 }
 
 const GA100: Ga100 = Ga100;
diff --git a/drivers/gpu/nova-core/fb/hal/ga102.rs b/drivers/gpu/nova-core/fb/hal/ga102.rs
index a73b77e3971513d088211a97ad8e50b00a9131f7..4b93fde8357d81c636eb63528750ec600fa77443 100644
--- a/drivers/gpu/nova-core/fb/hal/ga102.rs
+++ b/drivers/gpu/nova-core/fb/hal/ga102.rs
@@ -3,13 +3,21 @@
 use kernel::prelude::*;
 
 use crate::driver::Bar0;
-use crate::fb::hal::FbHal;
+use crate::fb::hal::{FbHal, GspFwHeapParams};
+use crate::nvfw;
 use crate::regs;
 
 fn vidmem_size_ga102(bar: &Bar0) -> u64 {
     regs::NV_USABLE_FB_SIZE_IN_MB::read(bar).usable_fb_size()
 }
 
+fn heap_params_ga102() -> GspFwHeapParams {
+    GspFwHeapParams {
+        libos: &nvfw::LIBOS3_PARAMS,
+        ..super::tu102::heap_params_tu102()
+    }
+}
+
 struct Ga102;
 
 impl FbHal for Ga102 {
@@ -30,6 +38,10 @@ fn supports_display(&self, bar: &Bar0) -> bool {
     fn vidmem_size(&self, bar: &Bar0) -> u64 {
         vidmem_size_ga102(bar)
     }
+
+    fn heap_params(&self) -> GspFwHeapParams {
+        heap_params_ga102()
+    }
 }
 
 const GA102: Ga102 = Ga102;
diff --git a/drivers/gpu/nova-core/fb/hal/tu102.rs b/drivers/gpu/nova-core/fb/hal/tu102.rs
index b022c781caf4514b4060fa2083cdc0ca12573c5b..441f1dc0e5163ea7612b7b950924918cdb6cb5c0 100644
--- a/drivers/gpu/nova-core/fb/hal/tu102.rs
+++ b/drivers/gpu/nova-core/fb/hal/tu102.rs
@@ -2,7 +2,10 @@
 
 use crate::driver::Bar0;
 use crate::fb::hal::FbHal;
+use crate::fb::hal::GspFwHeapParams;
+use crate::nvfw;
 use crate::regs;
+
 use kernel::prelude::*;
 
 /// Shift applied to the sysmem address before it is written into `NV_PFB_NISO_FLUSH_SYSMEM_ADDR`,
@@ -34,6 +37,13 @@ pub(super) fn vidmem_size_gp102(bar: &Bar0) -> u64 {
     regs::NV_PFB_PRI_MMU_LOCAL_MEMORY_RANGE::read(bar).usable_fb_size()
 }
 
+pub(super) fn heap_params_tu102() -> GspFwHeapParams {
+    GspFwHeapParams {
+        libos: &nvfw::LIBOS2_PARAMS,
+        base_rm_size: u64::from(nvfw::GSP_FW_HEAP_PARAM_BASE_RM_SIZE_TU10X),
+    }
+}
+
 struct Tu102;
 
 impl FbHal for Tu102 {
@@ -52,6 +62,10 @@ fn supports_display(&self, bar: &Bar0) -> bool {
     fn vidmem_size(&self, bar: &Bar0) -> u64 {
         vidmem_size_gp102(bar)
     }
+
+    fn heap_params(&self) -> GspFwHeapParams {
+        heap_params_tu102()
+    }
 }
 
 const TU102: Tu102 = Tu102;
diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
index f296dee224e48b2a4e20d06f8b36d8d1e5f08c53..05e57730a3c6fa3d3415c6073de55d1ff1b3b40a 100644
--- a/drivers/gpu/nova-core/firmware.rs
+++ b/drivers/gpu/nova-core/firmware.rs
@@ -122,9 +122,9 @@ pub(crate) struct Firmware {
     /// Runs on the sec2 falcon engine to stop and unload a running GSP firmware.
     booter_unloader: BooterFirmware,
     /// GSP bootloader, verifies the GSP firmware before loading and running it.
-    gsp_bootloader: RiscvFirmware,
+    pub gsp_bootloader: RiscvFirmware,
     /// GSP firmware.
-    gsp: Pin<KBox<GspFirmware>>,
+    pub gsp: Pin<KBox<GspFirmware>>,
     /// GSP signatures, to be passed as parameter to the bootloader for validation.
     gsp_sigs: DmaObject,
 }
diff --git a/drivers/gpu/nova-core/firmware/riscv.rs b/drivers/gpu/nova-core/firmware/riscv.rs
index 926883230f2fe4e3327713e28b7fae31ebee60bb..b2f646c1f02c6d1c5a28e688c6d2d0684b3f31be 100644
--- a/drivers/gpu/nova-core/firmware/riscv.rs
+++ b/drivers/gpu/nova-core/firmware/riscv.rs
@@ -61,7 +61,7 @@ pub(crate) struct RiscvFirmware {
     /// Application version.
     app_version: u32,
     /// Device-mapped firmware image.
-    ucode: DmaObject,
+    pub ucode: DmaObject,
 }
 
 impl RiscvFirmware {
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 54f0e9fd587ae5c4c045096930c0548fb1ef1b86..5c1c88086cb0dae3ae3547aeb0e15332f1d854df 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -302,7 +302,7 @@ pub(crate) fn new(
             FIRMWARE_VERSION,
         )?;
 
-        let fb_layout = FbLayout::new(spec.chipset, bar)?;
+        let fb_layout = FbLayout::new(spec.chipset, bar, &fw.gsp_bootloader, &fw.gsp)?;
         dev_dbg!(pdev.as_ref(), "{:#x?}\n", fb_layout);
 
         let bios = Vbios::new(pdev, bar)?;
diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs
index a0e7ec5f6c9c959d57540b3ebf4b782f2e002b08..ead471746ccad02f1e0d6ec114ab2aa67b1ed733 100644
--- a/drivers/gpu/nova-core/gsp.rs
+++ b/drivers/gpu/nova-core/gsp.rs
@@ -1,4 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
+use kernel::ptr::Alignment;
+
 pub(crate) const GSP_PAGE_SHIFT: usize = 12;
 pub(crate) const GSP_PAGE_SIZE: usize = 1 << GSP_PAGE_SHIFT;
+pub(crate) const GSP_HEAP_ALIGNMENT: Alignment = Alignment::new(1 << 20);
diff --git a/drivers/gpu/nova-core/nvfw.rs b/drivers/gpu/nova-core/nvfw.rs
index 7c5baccc34a2387c30e51f93d3ae039b14b6b83a..11a63c3710b1aa1eec78359c15c101bdf2ad99c8 100644
--- a/drivers/gpu/nova-core/nvfw.rs
+++ b/drivers/gpu/nova-core/nvfw.rs
@@ -1,3 +1,42 @@
 // SPDX-License-Identifier: GPL-2.0
 
 mod r570_144;
+
+use core::ops::Range;
+
+use kernel::sizes::SZ_1M;
+
+/// Heap memory requirements and constraints for a given version of the GSP LIBOS.
+pub(crate) struct LibosParams {
+    /// The base amount of heap required by the GSP operating system, in bytes.
+    pub(crate) carveout_size: u64,
+    /// The minimum and maximum sizes allowed for the GSP FW heap, in bytes.
+    pub(crate) allowed_heap_size: Range<u64>,
+}
+
+/// Version 2 of the GSP LIBOS (Turing and GA100)
+pub(crate) const LIBOS2_PARAMS: LibosParams = LibosParams {
+    carveout_size: r570_144::GSP_FW_HEAP_PARAM_OS_SIZE_LIBOS2 as u64,
+    allowed_heap_size: r570_144::GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS2_MIN_MB as u64 * SZ_1M as u64
+        ..r570_144::GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS2_MAX_MB as u64 * SZ_1M as u64,
+};
+
+/// Version 3 of the GSP LIBOS (GA102+)
+pub(crate) const LIBOS3_PARAMS: LibosParams = LibosParams {
+    carveout_size: r570_144::GSP_FW_HEAP_PARAM_OS_SIZE_LIBOS3_BAREMETAL as u64,
+    allowed_heap_size: r570_144::GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS3_BAREMETAL_MIN_MB as u64
+        * SZ_1M as u64
+        ..r570_144::GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS3_BAREMETAL_MAX_MB as u64 * SZ_1M as u64,
+};
+
+/// Amount of GSP-RM heap memory used during GSP-RM boot and initialization (up to and including
+/// the first client subdevice allocation) on Turing/Ampere/Ada.
+pub(crate) use r570_144::GSP_FW_HEAP_PARAM_BASE_RM_SIZE_TU10X;
+/// WPR heap usage of a single client channel allocation.
+pub(crate) use r570_144::GSP_FW_HEAP_PARAM_CLIENT_ALLOC_SIZE;
+/// Amount of extra WPR heap to reserve per GB of framebuffer memory, in bytes.
+pub(crate) use r570_144::GSP_FW_HEAP_PARAM_SIZE_PER_GB_FB;
+
+/// Structure passed to the GSP bootloader, containing the framebuffer layout as well as the DMA
+/// addresses of the GSP bootloader and firmware.
+pub(crate) use r570_144::GspFwWprMeta;
diff --git a/drivers/gpu/nova-core/nvfw/r570_144.rs b/drivers/gpu/nova-core/nvfw/r570_144.rs
index 2e7bba80fa8b9c5fcb4e26887825d2cca3f7b6b7..bb8074797b550c7976a7432b41841c6bf61bf5f8 100644
--- a/drivers/gpu/nova-core/nvfw/r570_144.rs
+++ b/drivers/gpu/nova-core/nvfw/r570_144.rs
@@ -12,7 +12,6 @@
 #![cfg_attr(test, allow(unsafe_op_in_unsafe_fn))]
 #![allow(
     dead_code,
-    unused_imports,
     clippy::all,
     clippy::undocumented_unsafe_blocks,
     clippy::ptr_as_ptr,
diff --git a/drivers/gpu/nova-core/nvfw/r570_144_bindings.rs b/drivers/gpu/nova-core/nvfw/r570_144_bindings.rs
index cec5940325151e407aa90128a35cb683afd436d7..0407000cca2296e713cc4701b635718fe51488cb 100644
--- a/drivers/gpu/nova-core/nvfw/r570_144_bindings.rs
+++ b/drivers/gpu/nova-core/nvfw/r570_144_bindings.rs
@@ -1 +1,126 @@
 // SPDX-License-Identifier: GPL-2.0
+
+pub const GSP_FW_HEAP_PARAM_OS_SIZE_LIBOS2: u32 = 0;
+pub const GSP_FW_HEAP_PARAM_OS_SIZE_LIBOS3_BAREMETAL: u32 = 23068672;
+pub const GSP_FW_HEAP_PARAM_BASE_RM_SIZE_TU10X: u32 = 8388608;
+pub const GSP_FW_HEAP_PARAM_SIZE_PER_GB_FB: u32 = 98304;
+pub const GSP_FW_HEAP_PARAM_CLIENT_ALLOC_SIZE: u32 = 100663296;
+pub const GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS2_MIN_MB: u32 = 64;
+pub const GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS2_MAX_MB: u32 = 256;
+pub const GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS3_BAREMETAL_MIN_MB: u32 = 88;
+pub const GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS3_BAREMETAL_MAX_MB: u32 = 280;
+pub type __u8 = ffi::c_uchar;
+pub type __u16 = ffi::c_ushort;
+pub type __u32 = ffi::c_uint;
+pub type __u64 = ffi::c_ulonglong;
+pub type u8_ = __u8;
+pub type u16_ = __u16;
+pub type u32_ = __u32;
+pub type u64_ = __u64;
+#[repr(C)]
+#[derive(Copy, Clone)]
+pub struct GspFwWprMeta {
+    pub magic: u64_,
+    pub revision: u64_,
+    pub sysmemAddrOfRadix3Elf: u64_,
+    pub sizeOfRadix3Elf: u64_,
+    pub sysmemAddrOfBootloader: u64_,
+    pub sizeOfBootloader: u64_,
+    pub bootloaderCodeOffset: u64_,
+    pub bootloaderDataOffset: u64_,
+    pub bootloaderManifestOffset: u64_,
+    pub __bindgen_anon_1: GspFwWprMeta__bindgen_ty_1,
+    pub gspFwRsvdStart: u64_,
+    pub nonWprHeapOffset: u64_,
+    pub nonWprHeapSize: u64_,
+    pub gspFwWprStart: u64_,
+    pub gspFwHeapOffset: u64_,
+    pub gspFwHeapSize: u64_,
+    pub gspFwOffset: u64_,
+    pub bootBinOffset: u64_,
+    pub frtsOffset: u64_,
+    pub frtsSize: u64_,
+    pub gspFwWprEnd: u64_,
+    pub fbSize: u64_,
+    pub vgaWorkspaceOffset: u64_,
+    pub vgaWorkspaceSize: u64_,
+    pub bootCount: u64_,
+    pub __bindgen_anon_2: GspFwWprMeta__bindgen_ty_2,
+    pub gspFwHeapVfPartitionCount: u8_,
+    pub flags: u8_,
+    pub padding: [u8_; 2usize],
+    pub pmuReservedSize: u32_,
+    pub verified: u64_,
+}
+#[repr(C)]
+#[derive(Copy, Clone)]
+pub union GspFwWprMeta__bindgen_ty_1 {
+    pub __bindgen_anon_1: GspFwWprMeta__bindgen_ty_1__bindgen_ty_1,
+    pub __bindgen_anon_2: GspFwWprMeta__bindgen_ty_1__bindgen_ty_2,
+}
+#[repr(C)]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct GspFwWprMeta__bindgen_ty_1__bindgen_ty_1 {
+    pub sysmemAddrOfSignature: u64_,
+    pub sizeOfSignature: u64_,
+}
+#[repr(C)]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct GspFwWprMeta__bindgen_ty_1__bindgen_ty_2 {
+    pub gspFwHeapFreeListWprOffset: u32_,
+    pub unused0: u32_,
+    pub unused1: u64_,
+}
+impl Default for GspFwWprMeta__bindgen_ty_1 {
+    fn default() -> Self {
+        let mut s = ::core::mem::MaybeUninit::<Self>::uninit();
+        unsafe {
+            ::core::ptr::write_bytes(s.as_mut_ptr(), 0, 1);
+            s.assume_init()
+        }
+    }
+}
+#[repr(C)]
+#[derive(Copy, Clone)]
+pub union GspFwWprMeta__bindgen_ty_2 {
+    pub __bindgen_anon_1: GspFwWprMeta__bindgen_ty_2__bindgen_ty_1,
+    pub __bindgen_anon_2: GspFwWprMeta__bindgen_ty_2__bindgen_ty_2,
+}
+#[repr(C)]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct GspFwWprMeta__bindgen_ty_2__bindgen_ty_1 {
+    pub partitionRpcAddr: u64_,
+    pub partitionRpcRequestOffset: u16_,
+    pub partitionRpcReplyOffset: u16_,
+    pub elfCodeOffset: u32_,
+    pub elfDataOffset: u32_,
+    pub elfCodeSize: u32_,
+    pub elfDataSize: u32_,
+    pub lsUcodeVersion: u32_,
+}
+#[repr(C)]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct GspFwWprMeta__bindgen_ty_2__bindgen_ty_2 {
+    pub partitionRpcPadding: [u32_; 4usize],
+    pub sysmemAddrOfCrashReportQueue: u64_,
+    pub sizeOfCrashReportQueue: u32_,
+    pub lsUcodeVersionPadding: [u32_; 1usize],
+}
+impl Default for GspFwWprMeta__bindgen_ty_2 {
+    fn default() -> Self {
+        let mut s = ::core::mem::MaybeUninit::<Self>::uninit();
+        unsafe {
+            ::core::ptr::write_bytes(s.as_mut_ptr(), 0, 1);
+            s.assume_init()
+        }
+    }
+}
+impl Default for GspFwWprMeta {
+    fn default() -> Self {
+        let mut s = ::core::mem::MaybeUninit::<Self>::uninit();
+        unsafe {
+            ::core::ptr::write_bytes(s.as_mut_ptr(), 0, 1);
+            s.assume_init()
+        }
+    }
+}

-- 
2.50.1

Re: [PATCH v2 1/8] rust: transmute: add `from_bytes_copy` method to `FromBytes` trait

[Benno Lossin]

On Tue Aug 26, 2025 at 6:07 AM CEST, Alexandre Courbot wrote:
> `FromBytes::from_bytes` comes with a few practical limitations:
>
> - It requires the bytes slice to have the same alignment as the returned
>   type, which might not be guaranteed in the case of a byte stream,
> - It returns a reference, requiring the returned type to implement
>   `Clone` if one wants to keep the value for longer than the lifetime of
>   the slice.

I think that any `Sized` type that can implement `FromBytes` also can
implement `Copy`, so I don't feel like the last point is a strong one.
But the unaligned byte stream is motivation enough :)

> To overcome these when needed, add a `from_bytes_copy` with a default
> implementation in the trait. `from_bytes_copy` returns an owned value
> that is populated using an unaligned read, removing the lifetime
> constraint and making it usable even on non-aligned byte slices.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

Reviewed-by: Benno Lossin <lossin@kernel.org>

---
Cheers,
Benno

> ---
>  rust/kernel/transmute.rs | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)

Re: [PATCH v2 0/8] gpu: nova-core: process and prepare more firmwares to boot GSP

[John Hubbard]

On 8/25/25 9:07 PM, Alexandre Courbot wrote:
...
> The base of this series is today's nova-next, but there a few more
> unmerged dependencies required:
> 
> - BorrowedPage, AsPageIter and VmallocPageIter [2]
> - Rust infrastructure for sg_table and scatterlist [3]
> - FromBytes [4]
> - The Alignment series [5]

Hi Alex!

For future patchsets that have complex and/or lengthy prerequisite
patchsets, let's please please make it easier for people to
locally apply the series. Very important. 

The very nicest experience is by simply providing a link to your git
branch that exactly matches the patchset you just posted.

In this particular case, it would also have worked to just say, "this 
series can be applied by first applying FromBytes [4], to the base commit."

Because that works, and is simpler than hunting down the other three
items, which are not actually necessary for git am (although they
might be for actual testing).

But the point is that the admin can be made simpler for the reviewers--even
those of us who know exactly what you're up to. And we should keep
that in mind, especially because there are many more of these situations
coming soon.


thanks,
-- 
John Hubbard

Re: [PATCH v2 1/8] rust: transmute: add `from_bytes_copy` method to `FromBytes` trait

[John Hubbard]

On 8/25/25 9:07 PM, Alexandre Courbot wrote:
> `FromBytes::from_bytes` comes with a few practical limitations:
> 
> - It requires the bytes slice to have the same alignment as the returned
>   type, which might not be guaranteed in the case of a byte stream,
> - It returns a reference, requiring the returned type to implement
>   `Clone` if one wants to keep the value for longer than the lifetime of
>   the slice.
> 
> To overcome these when needed, add a `from_bytes_copy` with a default
> implementation in the trait. `from_bytes_copy` returns an owned value
> that is populated using an unaligned read, removing the lifetime
> constraint and making it usable even on non-aligned byte slices.
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  rust/kernel/transmute.rs | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/rust/kernel/transmute.rs b/rust/kernel/transmute.rs
> index 494bb3b1d059337520efef694fc8952972d44fbf..721dd8254dcedd71ed7c1fc0ee9292950c16c89e 100644
> --- a/rust/kernel/transmute.rs
> +++ b/rust/kernel/transmute.rs
> @@ -78,6 +78,23 @@ fn from_bytes_mut(bytes: &mut [u8]) -> Option<&mut Self>
>              None
>          }
>      }
> +
> +    /// Creates an owned instance of `Self` by copying `bytes`.
> +    ///
> +    /// As the data is copied into a properly-aligned location, this method can be used even if
> +    /// [`FromBytes::from_bytes`] would return `None` due to incompatible alignment.

Some very minor suggestions:

This wording less precise than it could be: "as the data is copied" can mean
either "while it is being copied", or "because it is copied". Also, there
should not be a hyphen in "properly aligned".

I'd suggest something like this instead:
 
    /// Unlike [`FromBytes::from_bytes`], which requires aligned input, this
    /// method can be used on non-aligned data.

> +    fn from_bytes_copy(bytes: &[u8]) -> Option<Self>
> +    where
> +        Self: Sized,
> +    {
> +        if bytes.len() == size_of::<Self>() {
> +            // SAFETY: `bytes` has the same size as `Self`, and per the invariants of `FromBytes`,
> +            // any byte sequence is a valid value for `Self`.

More wording suggestions. How about this:

            // SAFETY: we just verified that `bytes` has the same size as `Self`, and per the
            // invariants of `FromBytes`, any byte sequence of the correct length is a valid value
            // for `Self`.

> +            Some(unsafe { core::ptr::read_unaligned(bytes.as_ptr().cast::<Self>()) })
> +        } else {
> +            None
> +        }
> +    }
>  }
>  
>  macro_rules! impl_frombytes {
> 

I'm unable to find anything wrong with the code itself, and the above are just
tiny nits, so whether or not you apply either or both of the above suggestions,
please feel free to add:


Reviewed-by: John Hubbard <jhubbard@nvidia.com>


thanks,
-- 
John Hubbard

Re: [PATCH v2 2/8] gpu: nova-core: firmware: add support for common firmware header

[John Hubbard]

On 8/25/25 9:07 PM, Alexandre Courbot wrote:
> Several firmware files loaded from userspace feature a common header
> that describes their payload. Add basic support for it so subsequent
> patches can leverage it.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drivers/gpu/nova-core/firmware.rs | 62 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
> index 2931912ddba0ea1fe6d027ccec70b39cdb40344a..ccb4d19f8fa76b0e844252dede5f50b37c590571 100644
> --- a/drivers/gpu/nova-core/firmware.rs
> +++ b/drivers/gpu/nova-core/firmware.rs
> @@ -4,11 +4,13 @@
>  //! to be loaded into a given execution unit.
>  
>  use core::marker::PhantomData;
> +use core::mem::size_of;
>  
>  use kernel::device;
>  use kernel::firmware;
>  use kernel::prelude::*;
>  use kernel::str::CString;
> +use kernel::transmute::FromBytes;
>  
>  use crate::dma::DmaObject;
>  use crate::falcon::FalconFirmware;
> @@ -150,6 +152,66 @@ fn no_patch_signature(self) -> FirmwareDmaObject<F, Signed> {
>      }
>  }
>  
> +/// Header common to most firmware files.
> +#[repr(C)]
> +#[derive(Debug, Clone)]
> +struct BinHdr {
> +    /// Magic number, must be `0x10de`.

How about:

       /// Magic number, required to be equal to BIN_MAGIC

...and see below.

> +    bin_magic: u32,
> +    /// Version of the header.
> +    bin_ver: u32,
> +    /// Size in bytes of the binary (to be ignored).
> +    bin_size: u32,
> +    /// Offset of the start of the application-specific header.
> +    header_offset: u32,
> +    /// Offset of the start of the data payload.
> +    data_offset: u32,
> +    /// Size in bytes of the data payload.
> +    data_size: u32,
> +}
> +
> +// SAFETY: all bit patterns are valid for this type, and it doesn't use interior mutability.
> +unsafe impl FromBytes for BinHdr {}
> +
> +// A firmware blob starting with a `BinHdr`.
> +struct BinFirmware<'a> {
> +    hdr: BinHdr,
> +    fw: &'a [u8],
> +}
> +
> +#[expect(dead_code)]
> +impl<'a> BinFirmware<'a> {
> +    /// Interpret `fw` as a firmware image starting with a [`BinHdr`], and returns the
> +    /// corresponding [`BinFirmware`] that can be used to extract its payload.
> +    fn new(fw: &'a firmware::Firmware) -> Result<Self> {
> +        const BIN_MAGIC: u32 = 0x10de;

Let's promote this to approximately file scope and put it just
above the BinHdr definition. Then we end up with one definition
at the ideal scope, and the comment docs take better care of 
themselves.

> +        let fw = fw.data();
> +
> +        fw.get(0..size_of::<BinHdr>())
> +            // Extract header.
> +            .and_then(BinHdr::from_bytes_copy)
> +            // Validate header.
> +            .and_then(|hdr| {
> +                if hdr.bin_magic == BIN_MAGIC {
> +                    Some(hdr)
> +                } else {
> +                    None
> +                }
> +            })
> +            .map(|hdr| Self { hdr, fw })
> +            .ok_or(EINVAL)
> +    }
> +
> +    /// Returns the data payload of the firmware, or `None` if the data range is out of bounds of
> +    /// the firmware image.
> +    fn data(&self) -> Option<&[u8]> {
> +        let fw_start = self.hdr.data_offset as usize;
> +        let fw_size = self.hdr.data_size as usize;
> +
> +        self.fw.get(fw_start..fw_start + fw_size)

This worries me a bit, because we never checked that these bounds
are reasonable: within the range of the firmware, and not overflowing
(.checked_add() for example), that sort of thing.

Thoughts?

> +    }
> +}
> +
>  pub(crate) struct ModInfoBuilder<const N: usize>(firmware::ModInfoBuilder<N>);
>  
>  impl<const N: usize> ModInfoBuilder<N> {
> 

thanks,
-- 
John Hubbard

Re: [PATCH v2 3/8] gpu: nova-core: firmware: process Booter and patch its signature

[John Hubbard]

On 8/25/25 9:07 PM, Alexandre Courbot wrote:
...
> +/// Signature parameters, as defined in the firmware.
> +#[repr(C)]
> +struct HsSignatureParams {
> +    // Fuse version to use.
> +    fuse_ver: u32,
> +    // Mask of engine IDs this firmware applies to.
> +    engine_id_mask: u32,
> +    // ID of the microcode.

Should these three comments use "///" instead of "//" ?

...> +pub(crate) struct BooterFirmware {
> +    // Load parameters for `IMEM` falcon memory.
> +    imem_load_target: FalconLoadTarget,
> +    // Load parameters for `DMEM` falcon memory.
> +    dmem_load_target: FalconLoadTarget,
> +    // BROM falcon parameters.
> +    brom_params: FalconBromParams,
> +    // Device-mapped firmware image.
> +    ucode: FirmwareDmaObject<Self, Signed>,
> +}
> +
> +impl FirmwareDmaObject<BooterFirmware, Unsigned> {
> +    fn new_booter(dev: &device::Device<device::Bound>, data: &[u8]) -> Result<Self> {
> +        DmaObject::from_data(dev, data).map(|ucode| Self(ucode, PhantomData))
> +    }
> +}
> +
> +impl BooterFirmware {
> +    /// Parses the Booter firmware contained in `fw`, and patches the correct signature so it is
> +    /// ready to be loaded and run on `falcon`.
> +    pub(crate) fn new(
> +        dev: &device::Device<device::Bound>,
> +        fw: &Firmware,
> +        falcon: &Falcon<<Self as FalconFirmware>::Target>,
> +        bar: &Bar0,
> +    ) -> Result<Self> {
> +        let bin_fw = BinFirmware::new(fw)?;

A few newlines for a little visual "vertical relief" would be a
welcome break from this wall of text. Maybe one before and after
each comment+line, just for this one time here, if that's not too 
excessive:

here> +        // The binary firmware embeds a Heavy-Secured firmware.
> +        let hs_fw = HsFirmwareV2::new(&bin_fw)?;
here> +        // The Heavy-Secured firmware embeds a firmware load descriptor.
> +        let load_hdr = HsLoadHeaderV2::new(&hs_fw)?;
here> +        // Offset in `ucode` where to patch the signature.
> +        let patch_loc = hs_fw.patch_location()?;
here> +        let sig_params = HsSignatureParams::new(&hs_fw)?;
> +        let brom_params = FalconBromParams {
> +            // `load_hdr.os_data_offset` is an absolute index, but `pkc_data_offset` is from the
> +            // signature patch location.
> +            pkc_data_offset: patch_loc
> +                .checked_sub(load_hdr.os_data_offset)
> +                .ok_or(EINVAL)?,
> +            engine_id_mask: u16::try_from(sig_params.engine_id_mask).map_err(|_| EINVAL)?,
> +            ucode_id: u8::try_from(sig_params.ucode_id).map_err(|_| EINVAL)?,
> +        };
> +        let app0 = HsLoadHeaderV2App::new(&hs_fw, 0)?;
> +
> +        // Object containing the firmware microcode to be signature-patched.
> +        let ucode = bin_fw
> +            .data()
> +            .ok_or(EINVAL)
> +            .and_then(|data| FirmwareDmaObject::<Self, _>::new_booter(dev, data))?;
> +
> +        let ucode_signed = {

This ucode_signed variable is misnamed...

> +            let mut signatures = hs_fw.signatures_iter()?.peekable();
> +
> +            if signatures.peek().is_none() {
> +                // If there are no signatures, then the firmware is unsigned.
> +                ucode.no_patch_signature()

...as we can see here. :)

> +            } else {
> +                // Obtain the version from the fuse register, and extract the corresponding
> +                // signature.
> +                let reg_fuse_version = falcon.signature_reg_fuse_version(

Oh...I don't want to derail this patch review with a pre-existing problem,
but let me mention it anyway so I don't forget: .signature_reg_fuse_version()
appears to be unnecessarily HAL-ified. I think.

SEC2 boot flow only applies to Turing, Ampere, Ada, and so unless Timur
uncovers a Turing-specific signature_reg_fuse_version(), then I think
we'd best delete that entire HAL area and call it directly.

Again, nothing to do with this patch, I'm just looking for a quick
sanity check on my first reading of this situation.

> +                    bar,
> +                    brom_params.engine_id_mask,
> +                    brom_params.ucode_id,
> +                )?;
> +
> +                let signature = match reg_fuse_version {
> +                    // `0` means the last signature should be used.
> +                    0 => signatures.last(),

Should we provide a global const, to make this concept a little more self-documenting?
Approximately: 

const FUSE_VERSION_USE_LAST_SIG: u32 = 0;

> +                    // Otherwise hardware fuse version needs to be substracted to obtain the index.

typo: "s/substracted/subtracted/"

> +                    reg_fuse_version => {
> +                        let Some(idx) = sig_params.fuse_ver.checked_sub(reg_fuse_version) else {
> +                            dev_err!(dev, "invalid fuse version for Booter firmware\n");
> +                            return Err(EINVAL);
> +                        };
> +                        signatures.nth(idx as usize)
> +                    }
> +                }
> +                .ok_or(EINVAL)?;
> +
> +                ucode.patch_signature(&signature, patch_loc as usize)?
> +            }
> +        };
> +
> +        Ok(Self {
> +            imem_load_target: FalconLoadTarget {
> +                src_start: app0.offset,
> +                dst_start: 0,
> +                len: app0.len,

Should we check that app0.offset.checked_add(app0.len) doesn't cause an
out of bounds read?


> +            },
> +            dmem_load_target: FalconLoadTarget {
> +                src_start: load_hdr.os_data_offset,
> +                dst_start: 0,
> +                len: load_hdr.os_data_size,
> +            },
> +            brom_params,
> +            ucode: ucode_signed,
> +        })
> +    }
> +}
> +
> +impl FalconLoadParams for BooterFirmware {
> +    fn imem_load_params(&self) -> FalconLoadTarget {
> +        self.imem_load_target.clone()
> +    }
> +
> +    fn dmem_load_params(&self) -> FalconLoadTarget {
> +        self.dmem_load_target.clone()
> +    }
> +
> +    fn brom_params(&self) -> FalconBromParams {
> +        self.brom_params.clone()
> +    }
> +
> +    fn boot_addr(&self) -> u32 {
> +        self.imem_load_target.src_start
> +    }
> +}
> +
> +impl Deref for BooterFirmware {
> +    type Target = DmaObject;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &self.ucode.0
> +    }
> +}

OK, so this allows &BooterFirmware to be used where &DmaObject is expected,
but it's not immediately obvious that BooterFirmware derefs to its internal
DMA object. It feels too clever...

Could we do something a little more obvious instead? Sort of like this:

impl BooterFirmware {
    pub(crate) fn dma_object(&self) -> &DmaObject {
        &self.ucode.0
    }
}

...

I'm out of time today, will work on the other half of the series tomorrow.

thanks,
-- 
John Hubbard

Re: [PATCH v2 0/8] gpu: nova-core: process and prepare more firmwares to boot GSP

[Alexandre Courbot]

On Wed Aug 27, 2025 at 9:29 AM JST, John Hubbard wrote:
> On 8/25/25 9:07 PM, Alexandre Courbot wrote:
> ...
>> The base of this series is today's nova-next, but there a few more
>> unmerged dependencies required:
>> 
>> - BorrowedPage, AsPageIter and VmallocPageIter [2]
>> - Rust infrastructure for sg_table and scatterlist [3]
>> - FromBytes [4]
>> - The Alignment series [5]
>
> Hi Alex!
>
> For future patchsets that have complex and/or lengthy prerequisite
> patchsets, let's please please make it easier for people to
> locally apply the series. Very important. 
>
> The very nicest experience is by simply providing a link to your git
> branch that exactly matches the patchset you just posted.
>
> In this particular case, it would also have worked to just say, "this 
> series can be applied by first applying FromBytes [4], to the base commit."
>
> Because that works, and is simpler than hunting down the other three
> items, which are not actually necessary for git am (although they
> might be for actual testing).
>
> But the point is that the admin can be made simpler for the reviewers--even
> those of us who know exactly what you're up to. And we should keep
> that in mind, especially because there are many more of these situations
> coming soon.

Right, b4 is supposed to be able to help with this as well, but indeed a
link to a git tree should be the standard with patchsets requiring such
dependencies. :) Will try to keep this in mind.

Re: [PATCH v2 2/8] gpu: nova-core: firmware: add support for common firmware header

[Alexandre Courbot]

On Wed Aug 27, 2025 at 10:34 AM JST, John Hubbard wrote:
<snip>
>> +    /// Returns the data payload of the firmware, or `None` if the data range is out of bounds of
>> +    /// the firmware image.
>> +    fn data(&self) -> Option<&[u8]> {
>> +        let fw_start = self.hdr.data_offset as usize;
>> +        let fw_size = self.hdr.data_size as usize;
>> +
>> +        self.fw.get(fw_start..fw_start + fw_size)
>
> This worries me a bit, because we never checked that these bounds
> are reasonable: within the range of the firmware, and not overflowing
> (.checked_add() for example), that sort of thing.
>
> Thoughts?

`get` returns `None` if the requested slice is out of bounds, so there
should be no risk of panicking here.

However, `fw_start + fw_size` can panic in debug configuration if it
overflows. In a release build I believe it will just happily wrap, and
`get` should consequently return `None` at the invalid range... Although
we can also get unlucky and produce a valid, yet incorrect, one.

This is actually something I've been thinking about while writing this
series and could not really decide upon: how to deal with operands and
functions in Rust that can potentially panic. Using `checked` operands
everywhere is a bit tedious, and even with great care there is no way to
guarantee that no panic occurs in a given function.

Panics are a big no-no in the kernel, yet I don't feel like we have the
proper tools to ensure they do not happen.

User-space has some crates like `no_panic`, but even these feel more
like hacks than anything else. Something at the compiler level would be
nice.

Maybe that would be a good discussion topic for the Plumber
Microconference?

Re: [PATCH v2 2/8] gpu: nova-core: firmware: add support for common firmware header

[John Hubbard]

On 8/27/25 1:47 AM, Alexandre Courbot wrote:
> On Wed Aug 27, 2025 at 10:34 AM JST, John Hubbard wrote:
> <snip>
>>> +    /// Returns the data payload of the firmware, or `None` if the data range is out of bounds of
>>> +    /// the firmware image.
>>> +    fn data(&self) -> Option<&[u8]> {
>>> +        let fw_start = self.hdr.data_offset as usize;
>>> +        let fw_size = self.hdr.data_size as usize;
>>> +
>>> +        self.fw.get(fw_start..fw_start + fw_size)
>>
>> This worries me a bit, because we never checked that these bounds
>> are reasonable: within the range of the firmware, and not overflowing
>> (.checked_add() for example), that sort of thing.
>>
>> Thoughts?
> 
> `get` returns `None` if the requested slice is out of bounds, so there
> should be no risk of panicking here.

I was wondering about the bounds themselves, though. Couldn't they
be wrong? (Do we care?)

> 
> However, `fw_start + fw_size` can panic in debug configuration if it
> overflows. In a release build I believe it will just happily wrap, and
> `get` should consequently return `None` at the invalid range... Although
> we can also get unlucky and produce a valid, yet incorrect, one.
> 
> This is actually something I've been thinking about while writing this
> series and could not really decide upon: how to deal with operands and
> functions in Rust that can potentially panic. Using `checked` operands
> everywhere is a bit tedious, and even with great care there is no way to
> guarantee that no panic occurs in a given function.

Yes, .checked_add() all over the place is just awful, would like to 
avoid that for sure.

> 
> Panics are a big no-no in the kernel, yet I don't feel like we have the
> proper tools to ensure they do not happen.
> 
> User-space has some crates like `no_panic`, but even these feel more
> like hacks than anything else. Something at the compiler level would be
> nice.
> 
> Maybe that would be a good discussion topic for the Plumber
> Microconference?

Yes. And maybe even for Kangrejos.

thanks,
-- 
John Hubbard

Re: [PATCH v2 0/8] gpu: nova-core: process and prepare more firmwares to boot GSP

[John Hubbard]

On 8/27/25 1:39 AM, Alexandre Courbot wrote:
> On Wed Aug 27, 2025 at 9:29 AM JST, John Hubbard wrote:
>> On 8/25/25 9:07 PM, Alexandre Courbot wrote:
>> ...
>> But the point is that the admin can be made simpler for the reviewers--even
>> those of us who know exactly what you're up to. And we should keep
>> that in mind, especially because there are many more of these situations
>> coming soon.
> 
> Right, b4 is supposed to be able to help with this as well, but indeed a

It really doesn't quite, though.

It is true that "base" (git format-patch --base) helps "b4 am" set things
up, but then a subsequent "git am" fails due to missing prerequisites.

b4 isn't set up to go retrieve those, on its own anyway.


thanks,
-- 
John Hubbard

Re: [PATCH v2 4/8] gpu: nova-core: firmware: process the GSP bootloader

[John Hubbard]

On 8/25/25 9:07 PM, Alexandre Courbot wrote:
> The GSP bootloader is a small RISC-V firmware that is loaded by Booter
> onto the GSP core and is in charge of loading, validating, and starting
> the actual GSP firmware.
> 
> It is a regular binary firmware file containing a specific header.
> Create a type holding the DMA-mapped firmware as well as useful
> information extracted from the header, and hook it into our firmware
> structure for later use.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drivers/gpu/nova-core/firmware.rs       |  7 ++-
>  drivers/gpu/nova-core/firmware/riscv.rs | 89 +++++++++++++++++++++++++++++++++
>  2 files changed, 94 insertions(+), 2 deletions(-)
> 

I've once again failed to find any problems with the code itself, and
have instead listed one or two trivial documentation suggestions, 
below. :)

Reviewed-by: John Hubbard <jhubbard@nvidia.com>


> diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
> index be190af1e11aec26c18c85324a185d135a16eabe..9bee0e0a0ab99d10be7e56d366970fdf4c813fc4 100644
> --- a/drivers/gpu/nova-core/firmware.rs
> +++ b/drivers/gpu/nova-core/firmware.rs
> @@ -12,6 +12,7 @@
>  use kernel::prelude::*;
>  use kernel::str::CString;
>  use kernel::transmute::FromBytes;
> +use riscv::RiscvFirmware;
>  
>  use crate::dma::DmaObject;
>  use crate::driver::Bar0;
> @@ -22,6 +23,7 @@
>  
>  pub(crate) mod booter;
>  pub(crate) mod fwsec;
> +pub(crate) mod riscv;
>  
>  pub(crate) const FIRMWARE_VERSION: &str = "535.113.01";
>  
> @@ -32,7 +34,8 @@ pub(crate) struct Firmware {
>      booter_loader: BooterFirmware,
>      /// Runs on the sec2 falcon engine to stop and unload a running GSP firmware.
>      booter_unloader: BooterFirmware,
> -    bootloader: firmware::Firmware,
> +    /// GSP bootloader, verifies the GSP firmware before loading and running it.
> +    gsp_bootloader: RiscvFirmware,
>      gsp: firmware::Firmware,
>  }
>  
> @@ -58,7 +61,7 @@ pub(crate) fn new(
>                  .and_then(|fw| BooterFirmware::new(dev, &fw, sec2, bar))?,
>              booter_unloader: request("booter_unload")
>                  .and_then(|fw| BooterFirmware::new(dev, &fw, sec2, bar))?,
> -            bootloader: request("bootloader")?,
> +            gsp_bootloader: request("bootloader").and_then(|fw| RiscvFirmware::new(dev, &fw))?,
>              gsp: request("gsp")?,
>          })
>      }
> diff --git a/drivers/gpu/nova-core/firmware/riscv.rs b/drivers/gpu/nova-core/firmware/riscv.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..926883230f2fe4e3327713e28b7fae31ebee60bb
> --- /dev/null
> +++ b/drivers/gpu/nova-core/firmware/riscv.rs
> @@ -0,0 +1,89 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Support for firmware binaries designed to run on a RISC-V cores. Such firmwares have a

typo: s/cores/core/

Also, as long as we're here: "Such firmwares" is understandable, but
"Such firmware files" is better.

> +//! dedicated header.
> +
> +use kernel::device;
> +use kernel::firmware::Firmware;
> +use kernel::prelude::*;
> +use kernel::transmute::FromBytes;
> +
> +use crate::dma::DmaObject;
> +use crate::firmware::BinFirmware;
> +
> +/// Descriptor for microcode running on a RISC-V core.
> +#[repr(C)]
> +#[derive(Debug)]
> +struct RmRiscvUCodeDesc {
> +    version: u32,
> +    bootloader_offset: u32,
> +    bootloader_size: u32,
> +    bootloader_param_offset: u32,
> +    bootloader_param_size: u32,
> +    riscv_elf_offset: u32,
> +    riscv_elf_size: u32,
> +    app_version: u32,
> +    manifest_offset: u32,
> +    manifest_size: u32,
> +    monitor_data_offset: u32,
> +    monitor_data_size: u32,
> +    monitor_code_offset: u32,
> +    monitor_code_size: u32,
> +}
> +
> +// SAFETY: all bit patterns are valid for this type, and it doesn't use interior mutability.
> +unsafe impl FromBytes for RmRiscvUCodeDesc {}
> +
> +impl RmRiscvUCodeDesc {
> +    /// Interprets the header of `bin_fw` as a [`RmRiscvUCodeDesc`] and returns it.
> +    ///
> +    /// Fails if the header pointed at by `bin_fw` is not within the bounds of the firmware image.
> +    fn new(bin_fw: &BinFirmware<'_>) -> Result<Self> {
> +        let offset = bin_fw.hdr.header_offset as usize;
> +
> +        bin_fw
> +            .fw
> +            .get(offset..offset + size_of::<Self>())
> +            .and_then(Self::from_bytes_copy)
> +            .ok_or(EINVAL)
> +    }
> +}
> +
> +/// A parsed firmware for a RISC-V core, ready to be loaded and run.
> +#[expect(unused)]
> +pub(crate) struct RiscvFirmware {
> +    /// Offset at which the code starts in the firmware image.
> +    code_offset: u32,
> +    /// Offset at which the data starts in the firmware image.
> +    data_offset: u32,
> +    /// Offset at which the manifest starts in the firmware image.
> +    manifest_offset: u32,
> +    /// Application version.
> +    app_version: u32,
> +    /// Device-mapped firmware image.
> +    ucode: DmaObject,
> +}
> +
> +impl RiscvFirmware {
> +    // Parses the RISC-V firmware image contained in `fw`.

Should this be a "///" comment?

> +    pub(crate) fn new(dev: &device::Device<device::Bound>, fw: &Firmware) -> Result<Self> {
> +        let bin_fw = BinFirmware::new(fw)?;
> +
> +        let riscv_desc = RmRiscvUCodeDesc::new(&bin_fw)?;
> +
> +        let ucode = {
> +            let start = bin_fw.hdr.data_offset as usize;
> +            let len = bin_fw.hdr.data_size as usize;
> +
> +            DmaObject::from_data(dev, fw.data().get(start..start + len).ok_or(EINVAL)?)?
> +        };
> +
> +        Ok(Self {
> +            ucode,
> +            code_offset: riscv_desc.monitor_code_offset,
> +            data_offset: riscv_desc.monitor_data_offset,
> +            manifest_offset: riscv_desc.manifest_offset,
> +            app_version: riscv_desc.app_version,
> +        })
> +    }
> +}
> 

thanks,
-- 
John Hubbard

Re: [PATCH v2 5/8] gpu: nova-core: firmware: process and prepare the GSP firmware

[John Hubbard]

On 8/25/25 9:07 PM, Alexandre Courbot wrote:
> The GSP firmware is a binary blob that is verified, loaded, and run by
> the GSP bootloader. Its presentation is a bit peculiar as the GSP
> bootloader expects to be given a DMA address to a 3-levels page table
> mapping the GSP firmware at address 0 of its own address space.
> 
> Prepare such a structure containing the DMA-mapped firmware as well as
> the DMA-mapped page tables, and a way to obtain the DMA handle of the
> level 0 page table.
> 
> As we are performing the required ELF section parsing and radix3 page
> table building, remove these items from the TODO file.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  Documentation/gpu/nova/core/todo.rst  |  17 -----
>  drivers/gpu/nova-core/firmware.rs     | 110 +++++++++++++++++++++++++++++++-
>  drivers/gpu/nova-core/firmware/gsp.rs | 117 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/nova-core/gsp.rs          |   4 ++
>  drivers/gpu/nova-core/nova_core.rs    |   1 +
>  5 files changed, 229 insertions(+), 20 deletions(-)
 
Code looks good. Or more accurately, it's working on my machine, and
I think I understand it, aside from the SG Table internals.

The documentation on the whole "radix 3" aspect is too light though, so
I've created some that you can add in if you agree with it.

...
> diff --git a/drivers/gpu/nova-core/firmware/gsp.rs b/drivers/gpu/nova-core/firmware/gsp.rs
...
> +/// A device-mapped firmware with a set of (also device-mapped) pages tables mapping the firmware
> +/// to the start of their own address space, also known as a `Radix3` firmware.

I'd like to replace the above two lines with something like this:

/// GSP firmware with 3-level radix page tables for the GSP bootloader.
///
/// The bootloader expects firmware to be mapped starting at address 0 in GSP's virtual address
/// space:
///
/// ```text
/// Level 0:  1 page, 1 entry         -> points to first level 1 page
/// Level 1:  Multiple pages/entries  -> each entry points to a level 2 page
/// Level 2:  Multiple pages/entries  -> each entry points to a firmware page
/// ```
///
/// Each page is 4KB, each entry is 8 bytes (64-bit DMA address).
/// Also known as "Radix3" firmware.


> +#[pin_data]
> +pub(crate) struct GspFirmware {

And then a slightly higher-level set of inline comments will help
developers, I think:

> +    /// The GSP firmware inside a [`VVec`], device-mapped via a SG table.
> +    #[pin]
> +    fw: SGTable<Owned<VVec<u8>>>,
> +    /// The level 2 page table, mapping [`Self::fw`] at its beginning.

Instead, how about:

      /// Level 2 page table(s) whose entries contain DMA addresses of firmware pages.

> +    #[pin]
> +    lvl2: SGTable<Owned<VVec<u8>>>,
> +    /// The level 1 page table, mapping [`Self::lvl2`] at its beginning.

       /// Level 1 page table(s) whose entries contain DMA addresses of level 2 pages.

> +    #[pin]
> +    lvl1: SGTable<Owned<VVec<u8>>>,
> +    /// The level 0 page table, mapping [`Self::lvl1`] at its beginning.

       /// Level 0 page table (single 4KB page) with one entry: DMA address of first level 1 page.

> +    lvl0: DmaObject,
> +    /// Size in bytes of the firmware contained in [`Self::fw`].
> +    pub size: usize,
> +}
> +
> +impl GspFirmware {
> +    /// Maps the GSP firmware image `fw` into `dev`'s address-space, and creates the page tables
> +    /// expected by the GSP bootloader to load it.
> +    pub(crate) fn new<'a>(
> +        dev: &'a device::Device<device::Bound>,
> +        fw: &'a [u8],
> +    ) -> impl PinInit<Self, Error> + 'a {
> +        try_pin_init!(&this in Self {
> +            fw <- {
> +                // Move the firmware into a vmalloc'd vector and map it into the device address
> +                // space.
> +                VVec::with_capacity(fw.len(), GFP_KERNEL)
> +                .and_then(|mut v| {
> +                    v.extend_from_slice(fw, GFP_KERNEL)?;
> +                    Ok(v)
> +                })
> +                .map_err(|_| ENOMEM)
> +                .map(|v| SGTable::new(dev, v, DataDirection::ToDevice, GFP_KERNEL))?
> +            },
> +            lvl2 <- {

Why must we use a strange vowel-removal algorithm for these vrbl nms? I'll let you have
a few extra characters and you can spell out "level2"...

> +                // Allocate the level 2 page table, map the firmware onto it, and map it into the
> +                // device address space.
> +                // SAFETY: `this` is a valid pointer, and `fw` has been initialized.
> +                let fw_sg_table = unsafe { &(*this.as_ptr()).fw };
> +                VVec::<u8>::with_capacity(
> +                    fw_sg_table.iter().count() * core::mem::size_of::<u64>(),
> +                    GFP_KERNEL,
> +                )
> +                .map_err(|_| ENOMEM)
> +                .and_then(|lvl2| map_into_lvl(fw_sg_table, lvl2))
> +                .map(|lvl2| SGTable::new(dev, lvl2, DataDirection::ToDevice, GFP_KERNEL))?
> +            },
> +            lvl1 <- {
> +                // Allocate the level 1 page table, map the level 2 page table onto it, and map it
> +                // into the device address space.
> +                // SAFETY: `this` is a valid pointer, and `lvl2` has been initialized.
> +                let lvl2_sg_table = unsafe { &(*this.as_ptr()).lvl2 };
> +                VVec::<u8>::with_capacity(
> +                    lvl2_sg_table.iter().count() * core::mem::size_of::<u64>(),
> +                    GFP_KERNEL,
> +                )
> +                .map_err(|_| ENOMEM)
> +                .and_then(|lvl1| map_into_lvl(lvl2_sg_table, lvl1))
> +                .map(|lvl1| SGTable::new(dev, lvl1, DataDirection::ToDevice, GFP_KERNEL))?
> +            },
> +            lvl0: {
> +                // Allocate the level 0 page table as a device-visible DMA object, and map the
> +                // level 1 page table onto it.
> +                // SAFETY: `this` is a valid pointer, and `lvl1` has been initialized.
> +                let lvl1_sg_table = unsafe { &(*this.as_ptr()).lvl1 };
> +                let mut lvl0 = DmaObject::new(dev, GSP_PAGE_SIZE)?;
> +                // SAFETY: we are the only owner of this newly-created object, making races
> +                // impossible.
> +                let lvl0_slice = unsafe { lvl0.as_slice_mut(0, GSP_PAGE_SIZE) }?;
> +                lvl0_slice[0..core::mem::size_of::<u64>()].copy_from_slice(
> +                    #[allow(clippy::useless_conversion)]
> +                    &(u64::from(lvl1_sg_table.iter().next().unwrap().dma_address())).to_le_bytes(),
> +                );
> +
> +                lvl0
> +            },
> +            size: fw.len(),
> +        })
> +    }
> +
> +    #[expect(unused)]
> +    /// Returns the DMA handle of the level 0 page table.
> +    pub(crate) fn lvl0_dma_handle(&self) -> DmaAddress {
> +        self.lvl0.dma_handle()
> +    }
> +}
> +
> +/// Create a linear mapping the device mapping of the buffer described by `sg_table` into `dst`.

How about this:

/// Build a page table from a scatter-gather list.
///
/// Takes each DMA-mapped region from `sg_table` and writes page table entries
/// for all 4KB pages within that region. For example, a 16KB SG entry becomes
/// 4 consecutive page table entries.

> +fn map_into_lvl(sg_table: &SGTable<Owned<VVec<u8>>>, mut dst: VVec<u8>) -> Result<VVec<u8>> {
> +    for sg_entry in sg_table.iter() {
> +        // Number of pages we need to map.
> +        let num_pages = (sg_entry.dma_len() as usize).div_ceil(GSP_PAGE_SIZE);
> +
> +        for i in 0..num_pages {
> +            let entry = sg_entry.dma_address() + (i as u64 * GSP_PAGE_SIZE as u64);
> +            dst.extend_from_slice(&entry.to_le_bytes(), GFP_KERNEL)?;
> +        }
> +    }
> +
> +    Ok(dst)
> +}
> diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..a0e7ec5f6c9c959d57540b3ebf4b782f2e002b08
> --- /dev/null
> +++ b/drivers/gpu/nova-core/gsp.rs
> @@ -0,0 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +pub(crate) const GSP_PAGE_SHIFT: usize = 12;
> +pub(crate) const GSP_PAGE_SIZE: usize = 1 << GSP_PAGE_SHIFT;
> diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
> index cb2bbb30cba142265b354c9acf70349a6e40759e..fffcaee2249fe6cd7f55a7291c1e44be42e791d9 100644
> --- a/drivers/gpu/nova-core/nova_core.rs
> +++ b/drivers/gpu/nova-core/nova_core.rs
> @@ -9,6 +9,7 @@
>  mod firmware;
>  mod gfw;
>  mod gpu;
> +mod gsp;
>  mod regs;
>  mod util;
>  mod vbios;
> 

thanks,
-- 
John Hubbard

Re: [PATCH v2 6/8] gpu: nova-core: firmware: use 570.144 firmware

[John Hubbard]

On 8/25/25 9:07 PM, Alexandre Courbot wrote:
> 570.144 is the latest available into linux-firmware as of this commit,
> and the one we will use to start development of nova-core. It should
> eventually be dropped for a newer version before the driver becomes able
> to do anything useful. The newer firmware is expected to iron out some
> of the inelegances of 570.144, notably related to packaging.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drivers/gpu/nova-core/firmware.rs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

> 
> diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
> index fb751287e938e6a323db185ff8c4ba2781d25285..f296dee224e48b2a4e20d06f8b36d8d1e5f08c53 100644
> --- a/drivers/gpu/nova-core/firmware.rs
> +++ b/drivers/gpu/nova-core/firmware.rs
> @@ -27,7 +27,7 @@
>  pub(crate) mod gsp;
>  pub(crate) mod riscv;
>  
> -pub(crate) const FIRMWARE_VERSION: &str = "535.113.01";
> +pub(crate) const FIRMWARE_VERSION: &str = "570.144";
>  
>  /// Ad-hoc and temporary module to extract sections from ELF images.
>  ///
> 

thanks,
-- 
John Hubbard

Re: [PATCH v2 7/8] gpu: nova-core: Add base files for r570.144 firmware bindings

[John Hubbard]

On 8/25/25 9:07 PM, Alexandre Courbot wrote:
> From: Alistair Popple <apopple@nvidia.com>
> 
> Interacting with the GSP currently requires using definitions from C
> header files. Rust definitions for the types needed for Nova core will
> be generated using the Rust bindgen tool. This patch adds the base
> module to allow inclusion of the generated bindings. The generated
> bindings themselves are added by subsequent patches when they are first
> used.
> 
> Currently we only intend to support a single firmware version, 570.144,
> with these bindings. Longer term we intend to move to a more stable GSP
> interface that isn't tied to specific firmware versions.
> 
> [acourbot@nvidia.com: adapt the bindings module comment a bit.]
> 
> Signed-off-by: Alistair Popple <apopple@nvidia.com>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drivers/gpu/nova-core/nova_core.rs              |  1 +
>  drivers/gpu/nova-core/nvfw.rs                   |  3 +++
>  drivers/gpu/nova-core/nvfw/r570_144.rs          | 29 +++++++++++++++++++++++++
>  drivers/gpu/nova-core/nvfw/r570_144_bindings.rs |  1 +
>  4 files changed, 34 insertions(+)
> 

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard

> diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
> index fffcaee2249fe6cd7f55a7291c1e44be42e791d9..db197498b0b7b1ff9234ef6645a4ea5ff44bd285 100644
> --- a/drivers/gpu/nova-core/nova_core.rs
> +++ b/drivers/gpu/nova-core/nova_core.rs
> @@ -10,6 +10,7 @@
>  mod gfw;
>  mod gpu;
>  mod gsp;
> +mod nvfw;
>  mod regs;
>  mod util;
>  mod vbios;
> diff --git a/drivers/gpu/nova-core/nvfw.rs b/drivers/gpu/nova-core/nvfw.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..7c5baccc34a2387c30e51f93d3ae039b14b6b83a
> --- /dev/null
> +++ b/drivers/gpu/nova-core/nvfw.rs
> @@ -0,0 +1,3 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +mod r570_144;
> diff --git a/drivers/gpu/nova-core/nvfw/r570_144.rs b/drivers/gpu/nova-core/nvfw/r570_144.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..2e7bba80fa8b9c5fcb4e26887825d2cca3f7b6b7
> --- /dev/null
> +++ b/drivers/gpu/nova-core/nvfw/r570_144.rs
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Firmware bindings.
> +//!
> +//! Imports the generated bindings by `bindgen`.
> +//!
> +//! This module may not be directly used. Please abstract or re-export the needed symbols in the
> +//! parent module instead.
> +
> +#![cfg_attr(test, allow(deref_nullptr))]
> +#![cfg_attr(test, allow(unaligned_references))]
> +#![cfg_attr(test, allow(unsafe_op_in_unsafe_fn))]
> +#![allow(
> +    dead_code,
> +    unused_imports,
> +    clippy::all,
> +    clippy::undocumented_unsafe_blocks,
> +    clippy::ptr_as_ptr,
> +    clippy::ref_as_ptr,
> +    missing_docs,
> +    non_camel_case_types,
> +    non_upper_case_globals,
> +    non_snake_case,
> +    improper_ctypes,
> +    unreachable_pub,
> +    unsafe_op_in_unsafe_fn
> +)]
> +use kernel::ffi;
> +include!("r570_144_bindings.rs");
> diff --git a/drivers/gpu/nova-core/nvfw/r570_144_bindings.rs b/drivers/gpu/nova-core/nvfw/r570_144_bindings.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..cec5940325151e407aa90128a35cb683afd436d7
> --- /dev/null
> +++ b/drivers/gpu/nova-core/nvfw/r570_144_bindings.rs
> @@ -0,0 +1 @@
> +// SPDX-License-Identifier: GPL-2.0
> 

Re: [PATCH v2 1/8] rust: transmute: add `from_bytes_copy` method to `FromBytes` trait

[Alexandre Courbot]

On Wed Aug 27, 2025 at 9:51 AM JST, John Hubbard wrote:
> On 8/25/25 9:07 PM, Alexandre Courbot wrote:
>> `FromBytes::from_bytes` comes with a few practical limitations:
>> 
>> - It requires the bytes slice to have the same alignment as the returned
>>   type, which might not be guaranteed in the case of a byte stream,
>> - It returns a reference, requiring the returned type to implement
>>   `Clone` if one wants to keep the value for longer than the lifetime of
>>   the slice.
>> 
>> To overcome these when needed, add a `from_bytes_copy` with a default
>> implementation in the trait. `from_bytes_copy` returns an owned value
>> that is populated using an unaligned read, removing the lifetime
>> constraint and making it usable even on non-aligned byte slices.
>> 
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>>  rust/kernel/transmute.rs | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>> 
>> diff --git a/rust/kernel/transmute.rs b/rust/kernel/transmute.rs
>> index 494bb3b1d059337520efef694fc8952972d44fbf..721dd8254dcedd71ed7c1fc0ee9292950c16c89e 100644
>> --- a/rust/kernel/transmute.rs
>> +++ b/rust/kernel/transmute.rs
>> @@ -78,6 +78,23 @@ fn from_bytes_mut(bytes: &mut [u8]) -> Option<&mut Self>
>>              None
>>          }
>>      }
>> +
>> +    /// Creates an owned instance of `Self` by copying `bytes`.
>> +    ///
>> +    /// As the data is copied into a properly-aligned location, this method can be used even if
>> +    /// [`FromBytes::from_bytes`] would return `None` due to incompatible alignment.
>
> Some very minor suggestions:
>
> This wording less precise than it could be: "as the data is copied" can mean
> either "while it is being copied", or "because it is copied". Also, there
> should not be a hyphen in "properly aligned".
>
> I'd suggest something like this instead:
>  
>     /// Unlike [`FromBytes::from_bytes`], which requires aligned input, this
>     /// method can be used on non-aligned data.

That's much simpler and better. I'll just add "... at the cost of a
copy." to not lose this information.

>
>> +    fn from_bytes_copy(bytes: &[u8]) -> Option<Self>
>> +    where
>> +        Self: Sized,
>> +    {
>> +        if bytes.len() == size_of::<Self>() {
>> +            // SAFETY: `bytes` has the same size as `Self`, and per the invariants of `FromBytes`,
>> +            // any byte sequence is a valid value for `Self`.
>
> More wording suggestions. How about this:
>
>             // SAFETY: we just verified that `bytes` has the same size as `Self`, and per the
>             // invariants of `FromBytes`, any byte sequence of the correct length is a valid value
>             // for `Self`.

Taken as-is, thanks!

Re: [PATCH v2 2/8] gpu: nova-core: firmware: add support for common firmware header

[Alexandre Courbot]

On Thu Aug 28, 2025 at 6:50 AM JST, John Hubbard wrote:
> On 8/27/25 1:47 AM, Alexandre Courbot wrote:
>> On Wed Aug 27, 2025 at 10:34 AM JST, John Hubbard wrote:
>> <snip>
>>>> +    /// Returns the data payload of the firmware, or `None` if the data range is out of bounds of
>>>> +    /// the firmware image.
>>>> +    fn data(&self) -> Option<&[u8]> {
>>>> +        let fw_start = self.hdr.data_offset as usize;
>>>> +        let fw_size = self.hdr.data_size as usize;
>>>> +
>>>> +        self.fw.get(fw_start..fw_start + fw_size)
>>>
>>> This worries me a bit, because we never checked that these bounds
>>> are reasonable: within the range of the firmware, and not overflowing
>>> (.checked_add() for example), that sort of thing.
>>>
>>> Thoughts?
>> 
>> `get` returns `None` if the requested slice is out of bounds, so there
>> should be no risk of panicking here.
>
> I was wondering about the bounds themselves, though. Couldn't they
> be wrong? (Do we care?)

Not sure what you mean by wrong bounds here? Do you mean what if the
header data is incorrect?

Re: [PATCH v2 3/8] gpu: nova-core: firmware: process Booter and patch its signature

[Alexandre Courbot]

On Wed Aug 27, 2025 at 11:29 AM JST, John Hubbard wrote:
> On 8/25/25 9:07 PM, Alexandre Courbot wrote:
> ...
>> +/// Signature parameters, as defined in the firmware.
>> +#[repr(C)]
>> +struct HsSignatureParams {
>> +    // Fuse version to use.
>> +    fuse_ver: u32,
>> +    // Mask of engine IDs this firmware applies to.
>> +    engine_id_mask: u32,
>> +    // ID of the microcode.
>
> Should these three comments use "///" instead of "//" ?

Absolutely! Thanks.

>
> ...> +pub(crate) struct BooterFirmware {
>> +    // Load parameters for `IMEM` falcon memory.
>> +    imem_load_target: FalconLoadTarget,
>> +    // Load parameters for `DMEM` falcon memory.
>> +    dmem_load_target: FalconLoadTarget,
>> +    // BROM falcon parameters.
>> +    brom_params: FalconBromParams,
>> +    // Device-mapped firmware image.
>> +    ucode: FirmwareDmaObject<Self, Signed>,
>> +}
>> +
>> +impl FirmwareDmaObject<BooterFirmware, Unsigned> {
>> +    fn new_booter(dev: &device::Device<device::Bound>, data: &[u8]) -> Result<Self> {
>> +        DmaObject::from_data(dev, data).map(|ucode| Self(ucode, PhantomData))
>> +    }
>> +}
>> +
>> +impl BooterFirmware {
>> +    /// Parses the Booter firmware contained in `fw`, and patches the correct signature so it is
>> +    /// ready to be loaded and run on `falcon`.
>> +    pub(crate) fn new(
>> +        dev: &device::Device<device::Bound>,
>> +        fw: &Firmware,
>> +        falcon: &Falcon<<Self as FalconFirmware>::Target>,
>> +        bar: &Bar0,
>> +    ) -> Result<Self> {
>> +        let bin_fw = BinFirmware::new(fw)?;
>
> A few newlines for a little visual "vertical relief" would be a
> welcome break from this wall of text. Maybe one before and after
> each comment+line, just for this one time here, if that's not too 
> excessive:

Indeed, that block was a bit too dense. :)

>
> here> +        // The binary firmware embeds a Heavy-Secured firmware.
>> +        let hs_fw = HsFirmwareV2::new(&bin_fw)?;
> here> +        // The Heavy-Secured firmware embeds a firmware load descriptor.
>> +        let load_hdr = HsLoadHeaderV2::new(&hs_fw)?;
> here> +        // Offset in `ucode` where to patch the signature.
>> +        let patch_loc = hs_fw.patch_location()?;
> here> +        let sig_params = HsSignatureParams::new(&hs_fw)?;
>> +        let brom_params = FalconBromParams {
>> +            // `load_hdr.os_data_offset` is an absolute index, but `pkc_data_offset` is from the
>> +            // signature patch location.
>> +            pkc_data_offset: patch_loc
>> +                .checked_sub(load_hdr.os_data_offset)
>> +                .ok_or(EINVAL)?,
>> +            engine_id_mask: u16::try_from(sig_params.engine_id_mask).map_err(|_| EINVAL)?,
>> +            ucode_id: u8::try_from(sig_params.ucode_id).map_err(|_| EINVAL)?,
>> +        };
>> +        let app0 = HsLoadHeaderV2App::new(&hs_fw, 0)?;
>> +
>> +        // Object containing the firmware microcode to be signature-patched.
>> +        let ucode = bin_fw
>> +            .data()
>> +            .ok_or(EINVAL)
>> +            .and_then(|data| FirmwareDmaObject::<Self, _>::new_booter(dev, data))?;
>> +
>> +        let ucode_signed = {
>
> This ucode_signed variable is misnamed...
>
>> +            let mut signatures = hs_fw.signatures_iter()?.peekable();
>> +
>> +            if signatures.peek().is_none() {
>> +                // If there are no signatures, then the firmware is unsigned.
>> +                ucode.no_patch_signature()
>
> ...as we can see here. :)

Technically it is of type `FirmwareDmaObject<Signed>` even if we take to
last branch. The name is to confirm that we have taken care of the
signature step (even if it is unneeded). Not sure what would be a better
name for this.

>
>> +            } else {
>> +                // Obtain the version from the fuse register, and extract the corresponding
>> +                // signature.
>> +                let reg_fuse_version = falcon.signature_reg_fuse_version(
>
> Oh...I don't want to derail this patch review with a pre-existing problem,
> but let me mention it anyway so I don't forget: .signature_reg_fuse_version()
> appears to be unnecessarily HAL-ified. I think.
>
> SEC2 boot flow only applies to Turing, Ampere, Ada, and so unless Timur
> uncovers a Turing-specific signature_reg_fuse_version(), then I think
> we'd best delete that entire HAL area and call it directly.
>
> Again, nothing to do with this patch, I'm just looking for a quick
> sanity check on my first reading of this situation.

Mmm, you're right - on the other hand I don't think I would have added a
HAL method unless I saw at least two different implementations in
OpenRM, but of course I am not 100% sure. Let's keep this in mind and
simplify it if we see it is indeed unneeded down the road.

>
>> +                    bar,
>> +                    brom_params.engine_id_mask,
>> +                    brom_params.ucode_id,
>> +                )?;
>> +
>> +                let signature = match reg_fuse_version {
>> +                    // `0` means the last signature should be used.
>> +                    0 => signatures.last(),
>
> Should we provide a global const, to make this concept a little more self-documenting?
> Approximately: 
>
> const FUSE_VERSION_USE_LAST_SIG: u32 = 0;

Good idea.

>
>> +                    // Otherwise hardware fuse version needs to be substracted to obtain the index.
>
> typo: "s/substracted/subtracted/"
>
>> +                    reg_fuse_version => {
>> +                        let Some(idx) = sig_params.fuse_ver.checked_sub(reg_fuse_version) else {
>> +                            dev_err!(dev, "invalid fuse version for Booter firmware\n");
>> +                            return Err(EINVAL);
>> +                        };
>> +                        signatures.nth(idx as usize)
>> +                    }
>> +                }
>> +                .ok_or(EINVAL)?;
>> +
>> +                ucode.patch_signature(&signature, patch_loc as usize)?
>> +            }
>> +        };
>> +
>> +        Ok(Self {
>> +            imem_load_target: FalconLoadTarget {
>> +                src_start: app0.offset,
>> +                dst_start: 0,
>> +                len: app0.len,
>
> Should we check that app0.offset.checked_add(app0.len) doesn't cause an
> out of bounds read?

If the data is out of bounds, it will be caught when trying to load the
firmware into the falcon engine. I am fine with adding a check here as
well if you think it it better.

>
>
>> +            },
>> +            dmem_load_target: FalconLoadTarget {
>> +                src_start: load_hdr.os_data_offset,
>> +                dst_start: 0,
>> +                len: load_hdr.os_data_size,
>> +            },
>> +            brom_params,
>> +            ucode: ucode_signed,
>> +        })
>> +    }
>> +}
>> +
>> +impl FalconLoadParams for BooterFirmware {
>> +    fn imem_load_params(&self) -> FalconLoadTarget {
>> +        self.imem_load_target.clone()
>> +    }
>> +
>> +    fn dmem_load_params(&self) -> FalconLoadTarget {
>> +        self.dmem_load_target.clone()
>> +    }
>> +
>> +    fn brom_params(&self) -> FalconBromParams {
>> +        self.brom_params.clone()
>> +    }
>> +
>> +    fn boot_addr(&self) -> u32 {
>> +        self.imem_load_target.src_start
>> +    }
>> +}
>> +
>> +impl Deref for BooterFirmware {
>> +    type Target = DmaObject;
>> +
>> +    fn deref(&self) -> &Self::Target {
>> +        &self.ucode.0
>> +    }
>> +}
>
> OK, so this allows &BooterFirmware to be used where &DmaObject is expected,
> but it's not immediately obvious that BooterFirmware derefs to its internal
> DMA object. It feels too clever...
>
> Could we do something a little more obvious instead? Sort of like this:
>
> impl BooterFirmware {
>     pub(crate) fn dma_object(&self) -> &DmaObject {
>         &self.ucode.0
>     }
> }

`Deref<Target = DmaObject>` is a requirement of `FalconFirmware`. That
being said, we could turn that into a `dma_object` method of
`FalconFirmware`, which might be a bit clearer about the intent...

Re: [PATCH v2 5/8] gpu: nova-core: firmware: process and prepare the GSP firmware

[Alexandre Courbot]

On Thu Aug 28, 2025 at 1:01 PM JST, John Hubbard wrote:
> On 8/25/25 9:07 PM, Alexandre Courbot wrote:
>> The GSP firmware is a binary blob that is verified, loaded, and run by
>> the GSP bootloader. Its presentation is a bit peculiar as the GSP
>> bootloader expects to be given a DMA address to a 3-levels page table
>> mapping the GSP firmware at address 0 of its own address space.
>> 
>> Prepare such a structure containing the DMA-mapped firmware as well as
>> the DMA-mapped page tables, and a way to obtain the DMA handle of the
>> level 0 page table.
>> 
>> As we are performing the required ELF section parsing and radix3 page
>> table building, remove these items from the TODO file.
>> 
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>>  Documentation/gpu/nova/core/todo.rst  |  17 -----
>>  drivers/gpu/nova-core/firmware.rs     | 110 +++++++++++++++++++++++++++++++-
>>  drivers/gpu/nova-core/firmware/gsp.rs | 117 ++++++++++++++++++++++++++++++++++
>>  drivers/gpu/nova-core/gsp.rs          |   4 ++
>>  drivers/gpu/nova-core/nova_core.rs    |   1 +
>>  5 files changed, 229 insertions(+), 20 deletions(-)
>  
> Code looks good. Or more accurately, it's working on my machine, and
> I think I understand it, aside from the SG Table internals.
>
> The documentation on the whole "radix 3" aspect is too light though, so
> I've created some that you can add in if you agree with it.
>
> ...
>> diff --git a/drivers/gpu/nova-core/firmware/gsp.rs b/drivers/gpu/nova-core/firmware/gsp.rs
> ...
>> +/// A device-mapped firmware with a set of (also device-mapped) pages tables mapping the firmware
>> +/// to the start of their own address space, also known as a `Radix3` firmware.
>
> I'd like to replace the above two lines with something like this:
>
> /// GSP firmware with 3-level radix page tables for the GSP bootloader.
> ///
> /// The bootloader expects firmware to be mapped starting at address 0 in GSP's virtual address
> /// space:
> ///
> /// ```text
> /// Level 0:  1 page, 1 entry         -> points to first level 1 page
> /// Level 1:  Multiple pages/entries  -> each entry points to a level 2 page
> /// Level 2:  Multiple pages/entries  -> each entry points to a firmware page
> /// ```
> ///
> /// Each page is 4KB, each entry is 8 bytes (64-bit DMA address).
> /// Also known as "Radix3" firmware.

Thanks, this is perfect!

>
>
>> +#[pin_data]
>> +pub(crate) struct GspFirmware {
>
> And then a slightly higher-level set of inline comments will help
> developers, I think:
>
>> +    /// The GSP firmware inside a [`VVec`], device-mapped via a SG table.
>> +    #[pin]
>> +    fw: SGTable<Owned<VVec<u8>>>,
>> +    /// The level 2 page table, mapping [`Self::fw`] at its beginning.
>
> Instead, how about:
>
>       /// Level 2 page table(s) whose entries contain DMA addresses of firmware pages.
>
>> +    #[pin]
>> +    lvl2: SGTable<Owned<VVec<u8>>>,
>> +    /// The level 1 page table, mapping [`Self::lvl2`] at its beginning.
>
>        /// Level 1 page table(s) whose entries contain DMA addresses of level 2 pages.

Looking good. But isn't it singular "table"? We have one table, with
potentialy several pages, but each page is not a table in itself IIUC.

>
>> +    #[pin]
>> +    lvl1: SGTable<Owned<VVec<u8>>>,
>> +    /// The level 0 page table, mapping [`Self::lvl1`] at its beginning.
>
>        /// Level 0 page table (single 4KB page) with one entry: DMA address of first level 1 page.
>
>> +    lvl0: DmaObject,
>> +    /// Size in bytes of the firmware contained in [`Self::fw`].
>> +    pub size: usize,
>> +}
>> +
>> +impl GspFirmware {
>> +    /// Maps the GSP firmware image `fw` into `dev`'s address-space, and creates the page tables
>> +    /// expected by the GSP bootloader to load it.
>> +    pub(crate) fn new<'a>(
>> +        dev: &'a device::Device<device::Bound>,
>> +        fw: &'a [u8],
>> +    ) -> impl PinInit<Self, Error> + 'a {
>> +        try_pin_init!(&this in Self {
>> +            fw <- {
>> +                // Move the firmware into a vmalloc'd vector and map it into the device address
>> +                // space.
>> +                VVec::with_capacity(fw.len(), GFP_KERNEL)
>> +                .and_then(|mut v| {
>> +                    v.extend_from_slice(fw, GFP_KERNEL)?;
>> +                    Ok(v)
>> +                })
>> +                .map_err(|_| ENOMEM)
>> +                .map(|v| SGTable::new(dev, v, DataDirection::ToDevice, GFP_KERNEL))?
>> +            },
>> +            lvl2 <- {
>
> Why must we use a strange vowel-removal algorithm for these vrbl nms? I'll let you have
> a few extra characters and you can spell out "level2"...

Yeah, let me spell these fully.

>
>> +                // Allocate the level 2 page table, map the firmware onto it, and map it into the
>> +                // device address space.
>> +                // SAFETY: `this` is a valid pointer, and `fw` has been initialized.
>> +                let fw_sg_table = unsafe { &(*this.as_ptr()).fw };
>> +                VVec::<u8>::with_capacity(
>> +                    fw_sg_table.iter().count() * core::mem::size_of::<u64>(),
>> +                    GFP_KERNEL,
>> +                )
>> +                .map_err(|_| ENOMEM)
>> +                .and_then(|lvl2| map_into_lvl(fw_sg_table, lvl2))
>> +                .map(|lvl2| SGTable::new(dev, lvl2, DataDirection::ToDevice, GFP_KERNEL))?
>> +            },
>> +            lvl1 <- {
>> +                // Allocate the level 1 page table, map the level 2 page table onto it, and map it
>> +                // into the device address space.
>> +                // SAFETY: `this` is a valid pointer, and `lvl2` has been initialized.
>> +                let lvl2_sg_table = unsafe { &(*this.as_ptr()).lvl2 };
>> +                VVec::<u8>::with_capacity(
>> +                    lvl2_sg_table.iter().count() * core::mem::size_of::<u64>(),
>> +                    GFP_KERNEL,
>> +                )
>> +                .map_err(|_| ENOMEM)
>> +                .and_then(|lvl1| map_into_lvl(lvl2_sg_table, lvl1))
>> +                .map(|lvl1| SGTable::new(dev, lvl1, DataDirection::ToDevice, GFP_KERNEL))?
>> +            },
>> +            lvl0: {
>> +                // Allocate the level 0 page table as a device-visible DMA object, and map the
>> +                // level 1 page table onto it.
>> +                // SAFETY: `this` is a valid pointer, and `lvl1` has been initialized.
>> +                let lvl1_sg_table = unsafe { &(*this.as_ptr()).lvl1 };
>> +                let mut lvl0 = DmaObject::new(dev, GSP_PAGE_SIZE)?;
>> +                // SAFETY: we are the only owner of this newly-created object, making races
>> +                // impossible.
>> +                let lvl0_slice = unsafe { lvl0.as_slice_mut(0, GSP_PAGE_SIZE) }?;
>> +                lvl0_slice[0..core::mem::size_of::<u64>()].copy_from_slice(
>> +                    #[allow(clippy::useless_conversion)]
>> +                    &(u64::from(lvl1_sg_table.iter().next().unwrap().dma_address())).to_le_bytes(),
>> +                );
>> +
>> +                lvl0
>> +            },
>> +            size: fw.len(),
>> +        })
>> +    }
>> +
>> +    #[expect(unused)]
>> +    /// Returns the DMA handle of the level 0 page table.
>> +    pub(crate) fn lvl0_dma_handle(&self) -> DmaAddress {
>> +        self.lvl0.dma_handle()
>> +    }
>> +}
>> +
>> +/// Create a linear mapping the device mapping of the buffer described by `sg_table` into `dst`.
>
> How about this:
>
> /// Build a page table from a scatter-gather list.
> ///
> /// Takes each DMA-mapped region from `sg_table` and writes page table entries
> /// for all 4KB pages within that region. For example, a 16KB SG entry becomes
> /// 4 consecutive page table entries.

Much better. And I mixed some words in the original message which didn't
even make sense to begin with.

Re: [PATCH v2 1/8] rust: transmute: add `from_bytes_copy` method to `FromBytes` trait

[Alexandre Courbot]

On Tue Aug 26, 2025 at 1:07 PM JST, Alexandre Courbot wrote:
> `FromBytes::from_bytes` comes with a few practical limitations:
>
> - It requires the bytes slice to have the same alignment as the returned
>   type, which might not be guaranteed in the case of a byte stream,
> - It returns a reference, requiring the returned type to implement
>   `Clone` if one wants to keep the value for longer than the lifetime of
>   the slice.
>
> To overcome these when needed, add a `from_bytes_copy` with a default
> implementation in the trait. `from_bytes_copy` returns an owned value
> that is populated using an unaligned read, removing the lifetime
> constraint and making it usable even on non-aligned byte slices.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

We got 3 Reviewed-by on this patch - Miguel, are you ok if I merge it
together with Christian's `from_bytes` patch, since they are closely
related?

Re: [PATCH v2 5/8] gpu: nova-core: firmware: process and prepare the GSP firmware

[Danilo Krummrich]

On 8/26/25 6:07 AM, Alexandre Courbot wrote:
>   /// Structure encapsulating the firmware blobs required for the GPU to operate.
>   #[expect(dead_code)]
>   pub(crate) struct Firmware {
> @@ -36,7 +123,10 @@ pub(crate) struct Firmware {
>       booter_unloader: BooterFirmware,
>       /// GSP bootloader, verifies the GSP firmware before loading and running it.
>       gsp_bootloader: RiscvFirmware,
> -    gsp: firmware::Firmware,
> +    /// GSP firmware.
> +    gsp: Pin<KBox<GspFirmware>>,

Is there a reason why we don't just propagate it through struct Gpu, which uses 
pin-init already?

You can make Firmware pin_data too and then everything is within the single 
allocation of struct Gpu.

Re: [PATCH v2 0/8] gpu: nova-core: process and prepare more firmwares to boot GSP

[Konstantin Ryabitsev]

On Wed, Aug 27, 2025 at 02:56:26PM -0700, John Hubbard wrote:
> > Right, b4 is supposed to be able to help with this as well, but indeed a
> 
> It really doesn't quite, though.
> 
> It is true that "base" (git format-patch --base) helps "b4 am" set things
> up, but then a subsequent "git am" fails due to missing prerequisites.
> 
> b4 isn't set up to go retrieve those, on its own anyway.

Sure it is. :)

Try `b4 shazam -H` on this series.

-K

Re: [PATCH v2 3/8] gpu: nova-core: firmware: process Booter and patch its signature

[Timur Tabi]

On Tue, 2025-08-26 at 13:07 +0900, Alexandre Courbot wrote:
> +struct HsHeaderV2 {
> +    /// Offset to the start of the signatures.
> +    sig_prod_offset: u32,
> +    /// Size in bytes of the signatures.
> +    sig_prod_size: u32,
> +    /// Offset to a `u32` containing the location at which to patch the signature in the
> microcode
> +    /// image.
> +    patch_loc: u32,
> +    /// Offset to a `u32` containing the index of the signature to patch.
> +    patch_sig: u32,
> +    /// Start offset to the signature metadata.
> +    meta_data_offset: u32,
> +    /// Size in bytes of the signature metadata.
> +    meta_data_size: u32,
> +    /// Offset to a `u32` containing the number of signatures in the signatures section.
> +    num_sig: u32,
> +    /// Offset of the application-specific header.
> +    header_offset: u32,
> +    /// Size in bytes of the application-specific header.
> +    header_size: u32,
> +}

You are inconsistent with the names of offset fields in this struct.

patch_loc should be patch_loc_offset
patch_sig should be patch_sig_offset
num_sig should be num_sig_offset

Re: [PATCH v2 2/8] gpu: nova-core: firmware: add support for common firmware header

[John Hubbard]

On 8/28/25 12:08 AM, Alexandre Courbot wrote:
...
>>>> This worries me a bit, because we never checked that these bounds
>>>> are reasonable: within the range of the firmware, and not overflowing
>>>> (.checked_add() for example), that sort of thing.
>>>>
>>>> Thoughts?
>>>
>>> `get` returns `None` if the requested slice is out of bounds, so there
>>> should be no risk of panicking here.
>>
>> I was wondering about the bounds themselves, though. Couldn't they
>> be wrong? (Do we care?)
> 
> Not sure what you mean by wrong bounds here? Do you mean what if the
> header data is incorrect?

Yes, that's what I meant. And I'm mainly trying to get some perspective
about what kinds of checking we should be doing.

In this case, it seems that we don't actually need anything more than
what you already have, so we're all good here.


thanks,
-- 
John Hubbard

Re: [PATCH v2 3/8] gpu: nova-core: firmware: process Booter and patch its signature

[John Hubbard]

On 8/28/25 12:19 AM, Alexandre Courbot wrote:
> On Wed Aug 27, 2025 at 11:29 AM JST, John Hubbard wrote:
>> On 8/25/25 9:07 PM, Alexandre Courbot wrote:
>> ...
>>> +        let ucode_signed = {
>>
>> This ucode_signed variable is misnamed...
>>
>>> +            let mut signatures = hs_fw.signatures_iter()?.peekable();
>>> +
>>> +            if signatures.peek().is_none() {
>>> +                // If there are no signatures, then the firmware is unsigned.
>>> +                ucode.no_patch_signature()
>>
>> ...as we can see here. :)
> 
> Technically it is of type `FirmwareDmaObject<Signed>` even if we take to
> last branch. The name is to confirm that we have taken care of the
> signature step (even if it is unneeded). Not sure what would be a better
> name for this.

So the <Signed> parameter naming is also awkward, because it refers
to non-signed firmware too, I see. So we need to rename both.

Ideas:

a) ucode_maybe_signed, FirmwareDmaObject<MaybeSigned>

b) ucode_validated, FirmwareDmaObject<Validated>

...
>> Could we do something a little more obvious instead? Sort of like this:
>>
>> impl BooterFirmware {
>>     pub(crate) fn dma_object(&self) -> &DmaObject {
>>         &self.ucode.0
>>     }
>> }
> 
> `Deref<Target = DmaObject>` is a requirement of `FalconFirmware`. That
> being said, we could turn that into a `dma_object` method of
> `FalconFirmware`, which might be a bit clearer about the intent...

That does seem clearer to me.

thanks,
-- 
John Hubbard

Re: [PATCH v2 5/8] gpu: nova-core: firmware: process and prepare the GSP firmware

[John Hubbard]

On 8/28/25 4:13 AM, Alexandre Courbot wrote:
> On Thu Aug 28, 2025 at 1:01 PM JST, John Hubbard wrote:
>> On 8/25/25 9:07 PM, Alexandre Courbot wrote:
...
>>        /// Level 1 page table(s) whose entries contain DMA addresses of level 2 pages.
> 
> Looking good. But isn't it singular "table"? We have one table, with
> potentialy several pages, but each page is not a table in itself IIUC.

Oops, yes, good catch.


thanks,
-- 
John Hubbard

Re: [PATCH v2 0/8] gpu: nova-core: process and prepare more firmwares to boot GSP

[John Hubbard]

On 8/28/25 1:44 PM, Konstantin Ryabitsev wrote:
> On Wed, Aug 27, 2025 at 02:56:26PM -0700, John Hubbard wrote:
>>> Right, b4 is supposed to be able to help with this as well, but indeed a
>>
>> It really doesn't quite, though.
>>
>> It is true that "base" (git format-patch --base) helps "b4 am" set things
>> up, but then a subsequent "git am" fails due to missing prerequisites.
>>
>> b4 isn't set up to go retrieve those, on its own anyway.
> 
> Sure it is. :)
> 
> Try `b4 shazam -H` on this series.

That is nice, I've updated my mental model of what b4 can do now.

It still fails for this particular series, but it at least tries
quite thoroughly to get the dependencies.

I think this demonstrates a case of pushing even a very capable
tool such as b4, a little too hard, though.

<blueforge> linux-people (nova-next)$ b4 shazam -H "<20250826-nova_firmware-v2-0-93566252fe3a@nvidia.com>"
Grabbing thread from lore.kernel.org/all/20250826-nova_firmware-v2-0-93566252fe3a@nvidia.com/t.mbox.gz
Checking for newer revisions
Grabbing search results from lore.kernel.org
Analyzing 35 messages in the thread
Analyzing 10 code-review messages
Checking attestation on all messages, may take a moment...
---
  ✗ [PATCH v2 1/8] rust: transmute: add `from_bytes_copy` method to `FromBytes` trait
    + Acked-by: Miguel Ojeda <ojeda@kernel.org> (✓ DKIM/gmail.com)
    + Reviewed-by: John Hubbard <jhubbard@nvidia.com> (✗ DKIM/nvidia.com)
    + Reviewed-by: Benno Lossin <lossin@kernel.org> (✓ DKIM/kernel.org)
  ✗ [PATCH v2 2/8] gpu: nova-core: firmware: add support for common firmware header
  ✗ [PATCH v2 3/8] gpu: nova-core: firmware: process Booter and patch its signature
  ✗ [PATCH v2 4/8] gpu: nova-core: firmware: process the GSP bootloader
    + Reviewed-by: John Hubbard <jhubbard@nvidia.com> (✗ DKIM/nvidia.com)
  ✗ [PATCH v2 5/8] gpu: nova-core: firmware: process and prepare the GSP firmware
  ✗ [PATCH v2 6/8] gpu: nova-core: firmware: use 570.144 firmware
    + Reviewed-by: John Hubbard <jhubbard@nvidia.com> (✗ DKIM/nvidia.com)
  ✗ [PATCH v2 7/8] gpu: nova-core: Add base files for r570.144 firmware bindings
    + Reviewed-by: John Hubbard <jhubbard@nvidia.com> (✗ DKIM/nvidia.com)
  ✗ [PATCH v2 8/8] gpu: nova-core: compute layout of more framebuffer regions required for GSP
  ---
  ✗ BADSIG: DKIM/nvidia.com
---
Total patches: 8
---
 Base: using specified base-commit 331c24e6ce814af2af74bac648d1ac1708873e9c
 Deps: looking for dependencies matching 39 patch-ids
 Deps: Applying prerequisite patch: [PATCH v5 1/7] rust: page: implement BorrowedPage
 Deps: Applying prerequisite patch: [PATCH v5 2/7] rust: alloc: vmalloc: implement Vmalloc::to_page()
 Deps: Applying prerequisite patch: [PATCH v5 3/7] rust: alloc: implement VmallocPageIter
 Deps: Applying prerequisite patch: [PATCH v5 4/7] rust: page: define trait AsPageIter
 Deps: Applying prerequisite patch: [PATCH v5 5/7] rust: alloc: kbox: implement AsPageIter for VBox
 Deps: Applying prerequisite patch: [PATCH v2 4/6] rust: alloc: layout: implement ArrayLayout::size()
 Deps: Applying prerequisite patch: [PATCH v4 7/7] rust: alloc: kvec: implement AsPageIter for VVec
 Deps: Applying prerequisite patch: [PATCH v3 1/5] rust: dma: implement DataDirection
 Deps: Applying prerequisite patch: [PATCH v3 2/5] rust: dma: add type alias for bindings::dma_addr_t
 Deps: Applying prerequisite patch: [PATCH v3 3/5] rust: scatterlist: Add abstraction for sg_table
 Deps: Applying prerequisite patch: [PATCH v3 4/5] samples: rust: dma: add sample code for SGTable
 Deps: Applying prerequisite patch: [PATCH 4/4] MAINTAINERS: rust: dma: add scatterlist files
 Deps: Applying prerequisite patch: [PATCH v10] rust: transmute: Add methods for FromBytes trait
 Deps: Applying prerequisite patch: [PATCH] MAINTAINERS: Add website of Nova GPU driver
 Deps: Applying prerequisite patch: [PATCH 2/7] rust: gpu: update ARef and AlwaysRefCounted imports from sync::aref
 Deps: Applying prerequisite patch: [PATCH 01/18] gpu: nova-core: register: minor grammar and spelling fixes
 Deps: Applying prerequisite patch: [PATCH v2 02/19] gpu: nova-core: register: fix typo
 Deps: Applying prerequisite patch: [PATCH v2 04/19] gpu: nova-core: register: improve documentation for basic registers
 Deps: Applying prerequisite patch: [PATCH v2 05/19] gpu: nova-core: register: simplify @leaf_accessor rule
 Deps: Applying prerequisite patch: [PATCH v2 06/19] gpu: nova-core: register: remove `try_` accessors for relative registers
 Deps: Applying prerequisite patch: [PATCH 06/18] gpu: nova-core: register: move OFFSET declaration to I/O impl block
 Deps: Applying prerequisite patch: [PATCH 07/18] gpu: nova-core: register: fix documentation and indentation
 Deps: Applying prerequisite patch: [PATCH v2 09/19] gpu: nova-core: register: add missing doccomments for fixed registers I/O accessors
 Deps: Applying prerequisite patch: [PATCH 09/18] gpu: nova-core: register: add fields dispatcher internal rule
 Deps: Applying prerequisite patch: [PATCH v2 11/19] gpu: nova-core: register: improve `Debug` implementation
 Deps: Applying prerequisite patch: [PATCH 11/18] gpu: nova-core: register: generate correct `Default` implementation
 Deps: Applying prerequisite patch: [PATCH 12/18] gpu: nova-core: register: split @io rule into fixed and relative versions
 Deps: Applying prerequisite patch: [PATCH 13/18] gpu: nova-core: register: use #[inline(always)] for all methods
 Deps: Applying prerequisite patch: [PATCH v2 15/19] gpu: nova-core: register: redesign relative registers
 Deps: Applying prerequisite patch: [PATCH v2 16/19] gpu: nova-core: falcon: add distinct base address for PFALCON2
 Deps: Applying prerequisite patch: [PATCH v2 17/19] gpu: nova-core: register: add support for register arrays
 Deps: Applying prerequisite patch: [PATCH v2 18/19] gpu: nova-core: falcon: use register arrays for FUSE registers
 Deps: Applying prerequisite patch: [PATCH v2 19/19] gpu: nova-core: register: add support for relative array registers
 Deps: Applying prerequisite patch: [PATCH v2] gpu: nova-core: falcon: align DMA transfers to 256 bytes
 Deps: Applying prerequisite patch: [PATCH v4 1/2] rust: add `Alignment` type
 Deps: Applying prerequisite patch: [PATCH v3 2/2] gpu: nova-core: use Alignment for alignment-related operations
Magic: Preparing a sparse worktree
Unable to cleanly apply series, see failure log below
---
Applying: rust: page: implement BorrowedPage
Applying: rust: alloc: vmalloc: implement Vmalloc::to_page()
Applying: rust: alloc: implement VmallocPageIter
Applying: rust: page: define trait AsPageIter
Applying: rust: alloc: kbox: implement AsPageIter for VBox
Applying: rust: alloc: layout: implement ArrayLayout::size()
Applying: rust: alloc: kvec: implement AsPageIter for VVec
Applying: rust: dma: implement DataDirection
Applying: rust: dma: add type alias for bindings::dma_addr_t
Patch failed at 0009 rust: dma: add type alias for bindings::dma_addr_t
error: patch failed: drivers/gpu/nova-core/falcon.rs:4
error: drivers/gpu/nova-core/falcon.rs: patch does not apply
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
---
Not fetching into FETCH_HEAD
<blueforge> linux-people (nova-next)$ git am --show-current-patch=diff
fatal: Resolve operation not in progress, we are not resuming.
<blueforge> linux-people (nova-next)$ 


thanks,
-- 
John Hubbard

Re: [PATCH v2 1/8] rust: transmute: add `from_bytes_copy` method to `FromBytes` trait

[Alexandre Courbot]

On Thu Aug 28, 2025 at 8:45 PM JST, Miguel Ojeda wrote:
> On Thu, Aug 28, 2025 at 1:26 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>>
>> We got 3 Reviewed-by on this patch - Miguel, are you ok if I merge it
>> together with Christian's `from_bytes` patch, since they are closely
>> related?
>
> If you are taking this series this cycle, then sure!
>
> Acked-by: Miguel Ojeda <ojeda@kernel.org>

Thanks Miguel! Pushed this into nova-next, right after Christian's
patch.

Re: [PATCH v2 5/8] gpu: nova-core: firmware: process and prepare the GSP firmware

[Alexandre Courbot]

On Thu Aug 28, 2025 at 8:27 PM JST, Danilo Krummrich wrote:
> On 8/26/25 6:07 AM, Alexandre Courbot wrote:
>>   /// Structure encapsulating the firmware blobs required for the GPU to operate.
>>   #[expect(dead_code)]
>>   pub(crate) struct Firmware {
>> @@ -36,7 +123,10 @@ pub(crate) struct Firmware {
>>       booter_unloader: BooterFirmware,
>>       /// GSP bootloader, verifies the GSP firmware before loading and running it.
>>       gsp_bootloader: RiscvFirmware,
>> -    gsp: firmware::Firmware,
>> +    /// GSP firmware.
>> +    gsp: Pin<KBox<GspFirmware>>,
>
> Is there a reason why we don't just propagate it through struct Gpu, which uses 
> pin-init already?
>
> You can make Firmware pin_data too and then everything is within the single 
> allocation of struct Gpu.

I tried doing that at first, and hit the problem that the `impl PinInit`
returned by `GspFirmware::new` borrows a reference to the GSP firmware
binary loaded by `Firmware::new` - when `Firmware::new` returns, the
firmware gets freed, and the borrow checker complains.

We could move the GSP firmware loading code into the `pin_init!` of
`Firmware::new`, but now we hit another problem: in `Gpu::new` the
following code is executed:

    FbLayout::new(chipset, bar, &fw.gsp_bootloader, &fw.gsp)?

which requires the `Firmware` instance, which doesn't exist yet as the
`Gpu` object isn't initialized until the end of the method.

So we could move `FbLayout`, and everything else created by `Gpu::new`
to become members of the `Gpu` instance. It does make sense actually:
this `new` method is doing a lot of stuff, such as running FRTS, and
with Alistair's series it even runs Booter, the sequencer and so on.
Maybe we should move all firmware execution to a separate method that is
called by `probe` after the `Gpu` is constructed, as right now the `Gpu`
constructor looks like it does a bit more than it should.

... but even when doing that, `Firmware::new` and `FbLayout::new` still
require a reference to the `Bar`, and... you get the idea. :)

So I don't think the current design allows us to do that easily or at
all, and even if it does, it will be at a significant cost in code
clarity. There is also the fact that I am considering making the
firmware member of `Gpu` a trait object: the boot sequence is so
different between pre and post-Hopper that I don't think it makes sense
to share the same `Firmware` structure between the two. I would rather
see `Firmware` as an opaque trait object, which provides high-level
methods such as "start GSP" behind which the specifics of each GPU
family are hidden. If we go with this design, `Firmware` will become a
trait object and so cannot be pinned into `Gpu`.

This doesn't change my observation that `Gpu::new` should not IMHO do
steps like booting the GSP - it should just acquire the resources it
needs, return the pinned GPU object, and then `probe` can continue the
boot sequence. Having the GPU object pinned and constructed early
simplifies things quite a bit as the more we progress with boot, the
harder it becomes to construct everything in place (and the `PinInit`
closure also becomes more and more complex).

I'm still laying down the general design, but I'm pretty convinced that
having `Firmware` as a trait object is the right way to abstract the
differences between GPU families.

Re: [PATCH v2 8/8] gpu: nova-core: compute layout of more framebuffer regions required for GSP

[John Hubbard]

On 8/25/25 9:07 PM, Alexandre Courbot wrote:
> diff --git a/drivers/gpu/nova-core/fb.rs b/drivers/gpu/nova-core/fb.rs
> index b0e860498b883815b3861b8717f8ee1832d25440..a3eb063f86b3a06a7ad01e684919115abf5e28da 100644
...
>          let fb = {
> @@ -138,10 +202,54 @@ pub(crate) fn new(chipset: Chipset, bar: &Bar0) -> Result<Self> {
>              frts_base..frts_base + FRTS_SIZE
>          };
>  
> +        let boot = {

A few lines earlier, not shown in these diffs because it's not part of this patch,
there is a closely related TODO item:

            // TODO[NUMM]: replace with `align_down` once it lands.
            let frts_base = (vga_workspace.start & !(FRTS_DOWN_ALIGN - 1)) - FRTS_SIZE;

...which I think could be optionally fixed now, and added to this patch.

Or it could be done later, in a different patch, but it seems convenient
to merge it in as long as we're here, and using .align_down() in this patch.

...
> diff --git a/drivers/gpu/nova-core/nvfw.rs b/drivers/gpu/nova-core/nvfw.rs
> index 7c5baccc34a2387c30e51f93d3ae039b14b6b83a..11a63c3710b1aa1eec78359c15c101bdf2ad99c8 100644
> --- a/drivers/gpu/nova-core/nvfw.rs
> +++ b/drivers/gpu/nova-core/nvfw.rs
> @@ -1,3 +1,42 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
>  mod r570_144;
> +
> +use core::ops::Range;
> +
> +use kernel::sizes::SZ_1M;
> +
> +/// Heap memory requirements and constraints for a given version of the GSP LIBOS.
> +pub(crate) struct LibosParams {
> +    /// The base amount of heap required by the GSP operating system, in bytes.
> +    pub(crate) carveout_size: u64,
> +    /// The minimum and maximum sizes allowed for the GSP FW heap, in bytes.
> +    pub(crate) allowed_heap_size: Range<u64>,
> +}
> +
> +/// Version 2 of the GSP LIBOS (Turing and GA100)
> +pub(crate) const LIBOS2_PARAMS: LibosParams = LibosParams {
> +    carveout_size: r570_144::GSP_FW_HEAP_PARAM_OS_SIZE_LIBOS2 as u64,
> +    allowed_heap_size: r570_144::GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS2_MIN_MB as u64 * SZ_1M as u64
> +        ..r570_144::GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS2_MAX_MB as u64 * SZ_1M as u64,

We only support one version of the firmware. And in the coming months,
that one version will have a different version number.

Given those constraints, we should simply remove most (all?) of the "r570_144::"
namespace qualifiers in the code, starting here.

That way, we get:

a) A small diff, instead of a huge one, when we update to a new firmware
   version.

b) Shorter, cleaner symbols everywhere: GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS2_MAX_MB
   instead of r570_144::GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS2_MAX_MB, for example.

> +};
> +
> +/// Version 3 of the GSP LIBOS (GA102+)
> +pub(crate) const LIBOS3_PARAMS: LibosParams = LibosParams {
> +    carveout_size: r570_144::GSP_FW_HEAP_PARAM_OS_SIZE_LIBOS3_BAREMETAL as u64,
> +    allowed_heap_size: r570_144::GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS3_BAREMETAL_MIN_MB as u64
> +        * SZ_1M as u64
> +        ..r570_144::GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS3_BAREMETAL_MAX_MB as u64 * SZ_1M as u64,
> +};
> +
> +/// Amount of GSP-RM heap memory used during GSP-RM boot and initialization (up to and including
> +/// the first client subdevice allocation) on Turing/Ampere/Ada.
> +pub(crate) use r570_144::GSP_FW_HEAP_PARAM_BASE_RM_SIZE_TU10X;
> +/// WPR heap usage of a single client channel allocation.
> +pub(crate) use r570_144::GSP_FW_HEAP_PARAM_CLIENT_ALLOC_SIZE;
> +/// Amount of extra WPR heap to reserve per GB of framebuffer memory, in bytes.
> +pub(crate) use r570_144::GSP_FW_HEAP_PARAM_SIZE_PER_GB_FB;
> +
> +/// Structure passed to the GSP bootloader, containing the framebuffer layout as well as the DMA
> +/// addresses of the GSP bootloader and firmware.
> +pub(crate) use r570_144::GspFwWprMeta;

thanks,
-- 
John Hubbard

Re: [PATCH v2 8/8] gpu: nova-core: compute layout of more framebuffer regions required for GSP

[Alexandre Courbot]

On Sat Aug 30, 2025 at 8:30 AM JST, John Hubbard wrote:
> On 8/25/25 9:07 PM, Alexandre Courbot wrote:
>> diff --git a/drivers/gpu/nova-core/fb.rs b/drivers/gpu/nova-core/fb.rs
>> index b0e860498b883815b3861b8717f8ee1832d25440..a3eb063f86b3a06a7ad01e684919115abf5e28da 100644
> ...
>>          let fb = {
>> @@ -138,10 +202,54 @@ pub(crate) fn new(chipset: Chipset, bar: &Bar0) -> Result<Self> {
>>              frts_base..frts_base + FRTS_SIZE
>>          };
>>  
>> +        let boot = {
>
> A few lines earlier, not shown in these diffs because it's not part of this patch,
> there is a closely related TODO item:
>
>             // TODO[NUMM]: replace with `align_down` once it lands.
>             let frts_base = (vga_workspace.start & !(FRTS_DOWN_ALIGN - 1)) - FRTS_SIZE;
>
> ...which I think could be optionally fixed now, and added to this patch.
>
> Or it could be done later, in a different patch, but it seems convenient
> to merge it in as long as we're here, and using .align_down() in this patch.

This is taken care of by the series adding the `Alignment` type:

https://lore.kernel.org/rust-for-linux/20250821-num-v4-2-1f3a425d7244@nvidia.com/

>
> ...
>> diff --git a/drivers/gpu/nova-core/nvfw.rs b/drivers/gpu/nova-core/nvfw.rs
>> index 7c5baccc34a2387c30e51f93d3ae039b14b6b83a..11a63c3710b1aa1eec78359c15c101bdf2ad99c8 100644
>> --- a/drivers/gpu/nova-core/nvfw.rs
>> +++ b/drivers/gpu/nova-core/nvfw.rs
>> @@ -1,3 +1,42 @@
>>  // SPDX-License-Identifier: GPL-2.0
>>  
>>  mod r570_144;
>> +
>> +use core::ops::Range;
>> +
>> +use kernel::sizes::SZ_1M;
>> +
>> +/// Heap memory requirements and constraints for a given version of the GSP LIBOS.
>> +pub(crate) struct LibosParams {
>> +    /// The base amount of heap required by the GSP operating system, in bytes.
>> +    pub(crate) carveout_size: u64,
>> +    /// The minimum and maximum sizes allowed for the GSP FW heap, in bytes.
>> +    pub(crate) allowed_heap_size: Range<u64>,
>> +}
>> +
>> +/// Version 2 of the GSP LIBOS (Turing and GA100)
>> +pub(crate) const LIBOS2_PARAMS: LibosParams = LibosParams {
>> +    carveout_size: r570_144::GSP_FW_HEAP_PARAM_OS_SIZE_LIBOS2 as u64,
>> +    allowed_heap_size: r570_144::GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS2_MIN_MB as u64 * SZ_1M as u64
>> +        ..r570_144::GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS2_MAX_MB as u64 * SZ_1M as u64,
>
> We only support one version of the firmware. And in the coming months,
> that one version will have a different version number.
>
> Given those constraints, we should simply remove most (all?) of the "r570_144::"
> namespace qualifiers in the code, starting here.
>
> That way, we get:
>
> a) A small diff, instead of a huge one, when we update to a new firmware
>    version.
>
> b) Shorter, cleaner symbols everywhere: GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS2_MAX_MB
>    instead of r570_144::GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS2_MAX_MB, for example.

`nvfw` is the module that is supposed to abstract the currently
supported firmware version - but in order to provide this abstraction,
it needs to refer the items in question. :) I don't see how we could
avoid these qualifiers short of having a `use r750_144::*` which could
result into name collisions.

But maybe we can do a module alias to reduce the diff once the version
changes:

    use r570_144 as fwbindings;
    ...

    pub(crate) const LIBOS2_PARAMS: LibosParams = LibosParams {
        carveout_size: fwbindings::GSP_FW_HEAP_PARAM_OS_SIZE_LIBOS2 as u64,

Is that what you had in mind?

Re: [PATCH v2 8/8] gpu: nova-core: compute layout of more framebuffer regions required for GSP

[John Hubbard]

On 8/29/25 5:59 PM, Alexandre Courbot wrote:
> On Sat Aug 30, 2025 at 8:30 AM JST, John Hubbard wrote:
>> On 8/25/25 9:07 PM, Alexandre Courbot wrote:
>> ...
>> We only support one version of the firmware. And in the coming months,
>> that one version will have a different version number.
>>
>> Given those constraints, we should simply remove most (all?) of the "r570_144::"
>> namespace qualifiers in the code, starting here.
>>
>> That way, we get:
>>
>> a) A small diff, instead of a huge one, when we update to a new firmware
>>     version.
>>
>> b) Shorter, cleaner symbols everywhere: GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS2_MAX_MB
>>     instead of r570_144::GSP_FW_HEAP_SIZE_OVERRIDE_LIBOS2_MAX_MB, for example.
> 
> `nvfw` is the module that is supposed to abstract the currently
> supported firmware version - but in order to provide this abstraction,
> it needs to refer the items in question. :) I don't see how we could
> avoid these qualifiers short of having a `use r750_144::*` which could
> result into name collisions.

My idea here was: "what name collisions?". There shouldn't be any.
Because again, there is only one firmware version supported at a time.

> 
> But maybe we can do a module alias to reduce the diff once the version
> changes:
> 
>      use r570_144 as fwbindings;
>      ...
> 
>      pub(crate) const LIBOS2_PARAMS: LibosParams = LibosParams {
>          carveout_size: fwbindings::GSP_FW_HEAP_PARAM_OS_SIZE_LIBOS2 as u64,
> 
> Is that what you had in mind?

Not initially, but it would address most of what bothered me here,
which is having a particular version number all over everywhere.


thanks,
-- 
John Hubbard

Re: [PATCH v2 5/8] gpu: nova-core: firmware: process and prepare the GSP firmware

[Danilo Krummrich]

On 8/29/25 1:16 PM, Alexandre Courbot wrote:
> On Thu Aug 28, 2025 at 8:27 PM JST, Danilo Krummrich wrote:
>> On 8/26/25 6:07 AM, Alexandre Courbot wrote:
>>>    /// Structure encapsulating the firmware blobs required for the GPU to operate.
>>>    #[expect(dead_code)]
>>>    pub(crate) struct Firmware {
>>> @@ -36,7 +123,10 @@ pub(crate) struct Firmware {
>>>        booter_unloader: BooterFirmware,
>>>        /// GSP bootloader, verifies the GSP firmware before loading and running it.
>>>        gsp_bootloader: RiscvFirmware,
>>> -    gsp: firmware::Firmware,
>>> +    /// GSP firmware.
>>> +    gsp: Pin<KBox<GspFirmware>>,
>>
>> Is there a reason why we don't just propagate it through struct Gpu, which uses
>> pin-init already?
>>
>> You can make Firmware pin_data too and then everything is within the single
>> allocation of struct Gpu.

Thanks a lot for the write-up below!

> I tried doing that at first, and hit the problem that the `impl PinInit`
> returned by `GspFirmware::new` borrows a reference to the GSP firmware
> binary loaded by `Firmware::new` - when `Firmware::new` returns, the
> firmware gets freed, and the borrow checker complains.
> 
> We could move the GSP firmware loading code into the `pin_init!` of
> `Firmware::new`, but now we hit another problem: in `Gpu::new` the
> following code is executed:
> 
>      FbLayout::new(chipset, bar, &fw.gsp_bootloader, &fw.gsp)?
> 
> which requires the `Firmware` instance, which doesn't exist yet as the
> `Gpu` object isn't initialized until the end of the method.
> 
> So we could move `FbLayout`, and everything else created by `Gpu::new`
> to become members of the `Gpu` instance. It does make sense actually:
> this `new` method is doing a lot of stuff, such as running FRTS, and
> with Alistair's series it even runs Booter, the sequencer and so on.
> Maybe we should move all firmware execution to a separate method that is
> called by `probe` after the `Gpu` is constructed, as right now the `Gpu`
> constructor looks like it does a bit more than it should.

Absolutely, executing the firmware should be a separate method. Having it in the
constructor makes things more difficult.
> ... but even when doing that, `Firmware::new` and `FbLayout::new` still
> require a reference to the `Bar`, and... you get the idea. :)

Lifetime wise this should be fine, the &Bar out-lives the constructor, since
it's lifetime is bound to the &Device<Bound> which lives for the entire duration
of probe().
> So I don't think the current design allows us to do that easily or at
> all, and even if it does, it will be at a significant cost in code
> clarity. There is also the fact that I am considering making the
> firmware member of `Gpu` a trait object: the boot sequence is so
> different between pre and post-Hopper that I don't think it makes sense
> to share the same `Firmware` structure between the two. I would rather
> see `Firmware` as an opaque trait object, which provides high-level
> methods such as "start GSP" behind which the specifics of each GPU
> family are hidden. If we go with this design, `Firmware` will become a
> trait object and so cannot be pinned into `Gpu`.
> 
> This doesn't change my observation that `Gpu::new` should not IMHO do
> steps like booting the GSP - it should just acquire the resources it
> needs, return the pinned GPU object, and then `probe` can continue the
> boot sequence. Having the GPU object pinned and constructed early
> simplifies things quite a bit as the more we progress with boot, the
> harder it becomes to construct everything in place (and the `PinInit`
> closure also becomes more and more complex).
> 
> I'm still laying down the general design, but I'm pretty convinced that
> having `Firmware` as a trait object is the right way to abstract the
> differences between GPU families.

Makes sense, it's fine with me to keep this in its separate allocation for the
purpose of making Firmware an opaque trait object, which sounds reasonable.

But we should really properly separate construction of the GPU structure from
firmware boot code execution as you say. And actually move the construction of
the GPU object into try_pin_init!().

Re: [PATCH v2 5/8] gpu: nova-core: firmware: process and prepare the GSP firmware

[Alexandre Courbot]

On Sat Aug 30, 2025 at 9:56 PM JST, Danilo Krummrich wrote:
> On 8/29/25 1:16 PM, Alexandre Courbot wrote:
>> On Thu Aug 28, 2025 at 8:27 PM JST, Danilo Krummrich wrote:
>>> On 8/26/25 6:07 AM, Alexandre Courbot wrote:
>>>>    /// Structure encapsulating the firmware blobs required for the GPU to operate.
>>>>    #[expect(dead_code)]
>>>>    pub(crate) struct Firmware {
>>>> @@ -36,7 +123,10 @@ pub(crate) struct Firmware {
>>>>        booter_unloader: BooterFirmware,
>>>>        /// GSP bootloader, verifies the GSP firmware before loading and running it.
>>>>        gsp_bootloader: RiscvFirmware,
>>>> -    gsp: firmware::Firmware,
>>>> +    /// GSP firmware.
>>>> +    gsp: Pin<KBox<GspFirmware>>,
>>>
>>> Is there a reason why we don't just propagate it through struct Gpu, which uses
>>> pin-init already?
>>>
>>> You can make Firmware pin_data too and then everything is within the single
>>> allocation of struct Gpu.
>
> Thanks a lot for the write-up below!
>
>> I tried doing that at first, and hit the problem that the `impl PinInit`
>> returned by `GspFirmware::new` borrows a reference to the GSP firmware
>> binary loaded by `Firmware::new` - when `Firmware::new` returns, the
>> firmware gets freed, and the borrow checker complains.
>> 
>> We could move the GSP firmware loading code into the `pin_init!` of
>> `Firmware::new`, but now we hit another problem: in `Gpu::new` the
>> following code is executed:
>> 
>>      FbLayout::new(chipset, bar, &fw.gsp_bootloader, &fw.gsp)?
>> 
>> which requires the `Firmware` instance, which doesn't exist yet as the
>> `Gpu` object isn't initialized until the end of the method.
>> 
>> So we could move `FbLayout`, and everything else created by `Gpu::new`
>> to become members of the `Gpu` instance. It does make sense actually:
>> this `new` method is doing a lot of stuff, such as running FRTS, and
>> with Alistair's series it even runs Booter, the sequencer and so on.
>> Maybe we should move all firmware execution to a separate method that is
>> called by `probe` after the `Gpu` is constructed, as right now the `Gpu`
>> constructor looks like it does a bit more than it should.
>
> Absolutely, executing the firmware should be a separate method. Having it in the
> constructor makes things more difficult.
>> ... but even when doing that, `Firmware::new` and `FbLayout::new` still
>> require a reference to the `Bar`, and... you get the idea. :)
>
> Lifetime wise this should be fine, the &Bar out-lives the constructor, since
> it's lifetime is bound to the &Device<Bound> which lives for the entire duration
> of probe().

The &Bar is actually obtained inside the constructor (which is passed
the `Arc<Devres<Bar0>>`), so I don't think that would even work without
more clever tricks. :)

>> So I don't think the current design allows us to do that easily or at
>> all, and even if it does, it will be at a significant cost in code
>> clarity. There is also the fact that I am considering making the
>> firmware member of `Gpu` a trait object: the boot sequence is so
>> different between pre and post-Hopper that I don't think it makes sense
>> to share the same `Firmware` structure between the two. I would rather
>> see `Firmware` as an opaque trait object, which provides high-level
>> methods such as "start GSP" behind which the specifics of each GPU
>> family are hidden. If we go with this design, `Firmware` will become a
>> trait object and so cannot be pinned into `Gpu`.
>> 
>> This doesn't change my observation that `Gpu::new` should not IMHO do
>> steps like booting the GSP - it should just acquire the resources it
>> needs, return the pinned GPU object, and then `probe` can continue the
>> boot sequence. Having the GPU object pinned and constructed early
>> simplifies things quite a bit as the more we progress with boot, the
>> harder it becomes to construct everything in place (and the `PinInit`
>> closure also becomes more and more complex).
>> 
>> I'm still laying down the general design, but I'm pretty convinced that
>> having `Firmware` as a trait object is the right way to abstract the
>> differences between GPU families.
>
> Makes sense, it's fine with me to keep this in its separate allocation for the
> purpose of making Firmware an opaque trait object, which sounds reasonable.
>
> But we should really properly separate construction of the GPU structure from
> firmware boot code execution as you say. And actually move the construction of
> the GPU object into try_pin_init!().

Yes, and I'm glad you agree with this idea as the current design of
putting everything inside the GPU is a bit limiting.

Regarding the firmware, I also think this should undergo a redesign -
right now we are putting unrelated stuff inside the `Firmware`
structure, and this won't scale well when we start supporting Hopper+
which use a very different way to start the GSP.

I'll give more details in my review of Alistair's series, and hopefully
send a v3 with some of these ideas implemented soon.