[PATCH 0/8] Transition Nova Core to TLV firmware images

[PATCH 0/8] Transition Nova Core to TLV firmware images

[Timur Tabi]

This patch set transitions nova-core to use the new "TLV" firmware image
files, instead of the ones that Nouveau uses.

The current r570.144 images are a mix of binary headers and ELF files that
are cumbersome to parse in Rust.  There's a significant amount of code
that just reads in a struct, extracts some offset, and uses it to find
another struct, only to have nova-core use just a few fields.

The new format uses a sequence of tag/length/value fields that can be
iterated over.  The script that generates the TLV files,
extract-firmware-nova.py, does the extra work to find the specific metadata
needed by Nova and packages each one separately.

The TLV versions of r570.144 can be found here:

	https://github.com/ttabi/linux-firmware-nova

along with instructions on how to install them.  We are not planning on
submitting these images to linux-firmware.  Rather, if this patchset
is accepted upstream, I expect the small handful of people who are
actually working on Nova to grab and install these images, which needs
to be done only once.

There are still opportunities for improvement.  For example, I would like
to get rid of more GPU-specific code, especially the GA100 quirks.

Timur Tabi (8):
  rust: firmware: add request_into_buf()
  gpu: nova-core: add request_tlv to load TLV images
  gpu: nova-core: add TLV parser for firmware files
  gpu: nova-core: transition booter_load to TLV images
  gpu: nova-core: transition gsp to TLV images
  gpu: nova-core: transition gen_bootloader to TLV images
  gpu: nova-core: transition fsp to TLV images
  gpu: nova-core: update firmware module info for TLV images

 Documentation/gpu/nova/core/tlv.rst           | 172 +++++++
 drivers/gpu/nova-core/firmware.rs             | 451 ++++++++----------
 drivers/gpu/nova-core/firmware/booter.rs      | 328 +++----------
 drivers/gpu/nova-core/firmware/fsp.rs         |  84 ++--
 .../nova-core/firmware/fwsec/bootloader.rs    |  72 +--
 drivers/gpu/nova-core/firmware/gsp.rs         |  56 +--
 drivers/gpu/nova-core/firmware/riscv.rs       |  71 +--
 drivers/gpu/nova-core/gsp/boot.rs             |   7 +-
 drivers/gpu/nova-core/gsp/hal/gh100.rs        |   7 +-
 drivers/gpu/nova-core/gsp/hal/tu102.rs        |  15 +-
 rust/kernel/firmware.rs                       |  45 ++
 11 files changed, 575 insertions(+), 733 deletions(-)
 create mode 100644 Documentation/gpu/nova/core/tlv.rst


base-commit: 9eaff547805f8556992a9474465001c3e128b7bd
prerequisite-patch-id: 0a9ea098bc3576171da9f6293065fdb4db552466
-- 
2.54.0

[PATCH 1/8] rust: firmware: add request_into_buf()

[Timur Tabi]

Add request_into_buf(), a Rust wrapper around the
request_firmware_into_buf() function. This variant loads the firmware
image directly into a caller-provided buffer rather than a
kernel-allocated one.

Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 rust/kernel/firmware.rs | 45 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
index 71168d8004e2..d2ffaacc2d43 100644
--- a/rust/kernel/firmware.rs
+++ b/rust/kernel/firmware.rs
@@ -120,6 +120,51 @@ fn drop(&mut self) {
     }
 }
 
+/// Load firmware directly into the caller-provided `buf`.
+///
+/// On success the firmware image has been copied into `buf`; the number of bytes
+/// written is returned. The caller accesses the data through `buf` itself.
+/// See also `bindings::request_firmware_into_buf`.
+///
+/// This is intentionally a stand-alone function rather than a `Firmware` constructor. For
+/// the `into_buf` path, the firmware data lives in the caller's `buf`, not in a
+/// kernel-owned buffer, so returning a `Firmware` would expose `Firmware::data()` as an
+/// alias of `buf`. Since `buf` is `&mut [u8]` and the returned handle does not borrow it,
+/// the caller could mutate `buf` while reads via `data()` are outstanding (and
+/// `release_firmware()` does not free `buf` anyway). Releasing the bookkeeping here and
+/// returning only the size leaves `buf` as the single owner and accessor of the data.
+pub fn request_into_buf(name: &CStr, dev: &Device, buf: &mut [u8]) -> Result<usize> {
+    let mut fw: *mut bindings::firmware = core::ptr::null_mut();
+    let pfw: *mut *mut bindings::firmware = &mut fw;
+    let pfw: *mut *const bindings::firmware = pfw.cast();
+
+    // SAFETY: `pfw` is a valid pointer to a NULL initialized `bindings::firmware` pointer.
+    // `name` and `dev` are valid as by their type invariants. `buf` is a valid writable
+    // buffer of `buf.len()` bytes.
+    let ret = unsafe {
+        bindings::request_firmware_into_buf(
+            pfw,
+            name.as_char_ptr(),
+            dev.as_raw(),
+            buf.as_mut_ptr().cast(),
+            buf.len(),
+        )
+    };
+    if ret != 0 {
+        return Err(Error::from_errno(ret));
+    }
+
+    // SAFETY: `fw` is a valid pointer returned by `request_firmware_into_buf`.
+    let size = unsafe { (*fw).size };
+
+    // The firmware bytes are now in `buf`, which the caller owns, so we don't need
+    // the kernel to hang on to it any more.
+    // SAFETY: `fw` is a valid pointer returned by `request_firmware_into_buf`.
+    unsafe { bindings::release_firmware(fw) };
+
+    Ok(size)
+}
+
 // SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, which is safe to be used from
 // any thread.
 unsafe impl Send for Firmware {}
-- 
2.54.0

[PATCH 2/8] gpu: nova-core: add request_tlv to load TLV images

[Timur Tabi]

Add function request_tlv() to load TLV firmware images.

TLV (type, length, value) files are the new image format used by Nova
to encapsulate firmware images and their metadata.  Unlike the firmware
files for previous versions of the firmware, TLV filenames are not
versioned, and they have a .tlv suffix.

Also add some #[allow(unused)] statements to two identifiers that will
soon be removed, to keep the patchset clean.

Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 drivers/gpu/nova-core/firmware.rs | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
index 279fbacd0b8e..2749c196416d 100644
--- a/drivers/gpu/nova-core/firmware.rs
+++ b/drivers/gpu/nova-core/firmware.rs
@@ -33,9 +33,11 @@
 pub(crate) mod gsp;
 pub(crate) mod riscv;
 
+#[allow(unused)]
 pub(crate) const FIRMWARE_VERSION: &str = "570.144";
 
 /// Requests the GPU firmware `name` suitable for `chipset`, with version `ver`.
+#[allow(unused)]
 fn request_firmware(
     dev: &device::Device,
     chipset: gpu::Chipset,
@@ -48,6 +50,21 @@ fn request_firmware(
         .and_then(|path| firmware::Firmware::request(&path, dev))
 }
 
+/// Requests the GPU firmware TLV `name` suitable for `chipset`.
+#[allow(unused)]
+fn request_tlv(
+    dev: &device::Device,
+    chipset: gpu::Chipset,
+    name: &str,
+) -> Result<firmware::Firmware> {
+    let chip_name = chipset.name();
+
+    dev_info!(dev, "loading firmware image {}.tlv\n", name);
+
+    CString::try_from_fmt(fmt!("nvidia/{chip_name}/gsp/{name}.tlv"))
+        .and_then(|path| firmware::Firmware::request(&path, dev))
+}
+
 /// Structure used to describe some firmwares, notably FWSEC-FRTS.
 #[repr(C)]
 #[derive(Debug, Clone)]
-- 
2.54.0

[PATCH 3/8] gpu: nova-core: add TLV parser for firmware files

[Timur Tabi]

Add the Tlv struct and supporting types for parsing TLV (type, length,
value) firmware images. TLV files begin with a 4-byte magic header,
which must be "NVFW" for Nvidia firmware files.  This is followed by a
sequence of blocks each containing a 4-byte ASCII tag, a 4-byte
little-endian length, and a payload padded to a 4-byte boundary.

Tlv::new() validates the entire image up front, so that the iterator can
subsequently yield blocks without fallible parsing.

Also add accessor methods for the various encoded types that will be used
by the driver.

Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 Documentation/gpu/nova/core/tlv.rst | 172 ++++++++++++++++++++
 drivers/gpu/nova-core/firmware.rs   | 239 +++++++++++++++++++++++++++-
 2 files changed, 410 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/gpu/nova/core/tlv.rst

diff --git a/Documentation/gpu/nova/core/tlv.rst b/Documentation/gpu/nova/core/tlv.rst
new file mode 100644
index 000000000000..8223605daa6b
--- /dev/null
+++ b/Documentation/gpu/nova/core/tlv.rst
@@ -0,0 +1,172 @@
+.. SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+
+==================================
+TLV Tags in Nova Firmware Images
+==================================
+
+Nova firmware images use a Type-Length-Value (TLV) format to
+encapsulate firmware components and metadata. The TLV file begins
+with a 4-byte "magic" header that contains the string "NVFW".
+Following the header is a sequence of TLV blocks.
+
+Each block consists of a 4-byte tag of ASCII characters, a 4-byte
+length encode as a little-endian non-negative integer, and a
+sequence of bytes, the size of which is equal to the length rounded
+up to the next multiple of 4.
+
+::
+
+    +------+------+------+------+
+    |  'N' |  'V' |  'F' |  'W' |  Magic header
+    +------+------+------+------+
+    |  Tag (4 bytes, ASCII)     |  TLV block 0
+    +---------------------------+
+    |  Length (4 bytes, LE)     |
+    +---------------------------+
+    |                           |
+    |  Value (length bytes,     |
+    |  padded to 4-byte align)  |
+    |                           |
+    +---------------------------+
+    |  Tag (4 bytes, ASCII)     |  TLV block 1
+    +---------------------------+
+    |  Length (4 bytes, LE)     |
+    +---------------------------+
+    |                           |
+    |  Value (length bytes,     |
+    |  padded to 4-byte align)  |
+    |                           |
+    +---------------------------+
+    |         ...               |  More TLV blocks
+    +---------------------------+
+
+Values
+======
+Values are one of three types:
+
+1) Integers, encoded in 32-bit or 64-bit little-endian format.
+2) Strings, encoded as-is and expected to be ASCII only, without a null terminator.
+3) An array of bytes.
+
+
+Common Tags
+===========
+These tags appear across multiple firmware types.
+
+``VERS`` (string)
+    Human-readable firmware version string, indicates the version of
+    the firmware.  Present in all TLV files.
+
+A TLV image contains either a ``BLOB`` tag (firmware embedded
+inline) or a ``SIZE``/``FILE`` pair (firmware stored in a
+separate file).
+
+``BLOB`` (bytes)
+    If the firmware microcode binary is stored in the TLV, this tag contains
+    the actual firmware image bytes.
+
+``FILE`` (string)
+    If the firmware binary is stored as a separate file, this tag contains the
+    name of that file, assumed to be in the same directory as the TLV.
+    This tag is always paired with ``SIZE``, so as to allow the driver to
+    pre-allocate the buffer before loading the file.
+
+``SIZE`` (u32)
+    Total size in bytes of the firmware image to be loaded from
+    the companion file named by ``FILE``.
+
+GSP Firmware Tags
+=================
+Used when loading the GSP (GPU System Processor) firmware
+(``gsp.bin``).
+
+``SIGN`` (bytes)
+    Cryptographic signatures for the GSP firmware, DMA-mapped
+    for the GSP bootloader to verify before execution.
+
+Booter Firmware Tags
+====================
+Used by the Booter firmware (``booter_load.bin``,
+``booter_unload.bin``).
+
+``DAOF`` (u32) - ``os_data_offset``
+    OS data section offset within the firmware image (absolute
+    byte offset). Maps to the DMEM load source.
+
+``DASZ`` (u32) - ``os_data_size``
+    OS data section size in bytes.
+
+``CDOF`` (u32) - ``os_code_offset``
+    OS code section offset within the firmware image (absolute
+    byte offset). Maps to the non-secure IMEM load source.
+
+``CDSZ`` (u32) - ``os_code_size``
+    OS code section size in bytes.
+
+``PLOC`` (u32) - ``patch_loc``
+    Signature patch location -- byte offset within the firmware
+    image where the selected signature should be written.
+
+``FUSE`` (u32) - ``fuse_version``
+    Fuse version of the firmware, used with the hardware fuse
+    register to select the correct signature index.
+
+``ENID`` (u32) - ``engine_id``
+    Engine ID mask identifying the falcon engine this firmware
+    targets.
+
+``UCID`` (u32) - ``ucode_id``
+    Microcode ID used together with the engine ID to query
+    hardware signature fuse registers.
+
+``A0CO`` (u32) - ``app0_code_offset``
+    App0 code offset -- start of the secure code region within
+    the firmware image. Used as the IMEM secure section source.
+
+``A0CS`` (u32) - ``app0_code_size``
+    App0 code size in bytes.
+
+``NSIG`` (u32) - ``num_sigs``
+    Number of signatures included in the ``SIGS`` tag. A value
+    of 0 indicates unsigned firmware.
+
+``SIGS`` (bytes)
+    Concatenated array of firmware signatures. The size of
+    each signature is the total length of the ``SIGS`` value
+    divided by ``NSIG``. The correct signature is selected
+    using the fuse-version-derived index.
+
+Generic Bootloader Tags (FWSEC)
+===============================
+Used by the generic bootloader (``gen_bootloader.bin``) that
+loads the FWSEC firmware via DMA on Turing GPUs. Also includes
+the common ``VERS`` and ``BLOB`` tags.
+
+``CDSZ`` (u32) - ``code_size``
+    Size in bytes of the bootloader code to copy from the
+    ``BLOB`` tag and PIO-load into falcon IMEM.
+
+``STRT`` (u32) - ``start_tag``
+    Start tag identifying the IMEM block where execution begins.
+    The falcon boot address is derived as ``start_tag << 8``.
+
+RISC-V Firmware Tags (GSP Bootloader)
+======================================
+Used by RISC-V-based firmwares such as the GSP bootloader
+(``gsp_bootloader.bin``). Also includes the common ``VERS``
+and ``BLOB`` tags.
+
+``CDOF`` (u32) - ``code_offset``
+    Offset within the firmware image at which the code section
+    starts.
+
+``DAOF`` (u32) - ``data_offset``
+    Offset within the firmware image at which the data section
+    starts.
+
+``MFOF`` (u32) - ``manifest_offset``
+    Offset within the firmware image at which the manifest
+    starts.
+
+``APPV`` (u32) - ``app_version``
+    Application version of the firmware.
diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
index 2749c196416d..888a426b7b41 100644
--- a/drivers/gpu/nova-core/firmware.rs
+++ b/drivers/gpu/nova-core/firmware.rs
@@ -11,7 +11,7 @@
     device,
     firmware,
     prelude::*,
-    str::CString,
+    str::{CStr, CString},
     transmute::FromBytes, //
 };
 
@@ -684,3 +684,240 @@ pub(super) fn elf_section<'a>(elf: &'a [u8], name: &str) -> Option<&'a [u8]> {
         }
     }
 }
+
+pub(crate) struct TlvBlock<'a> {
+    pub(crate) tag: &'a str,
+    pub(crate) value: &'a [u8],
+}
+
+/// On-wire TLV block header: 4-byte ASCII tag + little-endian payload length (bytes, excluding
+/// padding to a 4-byte boundary).
+struct TlvBlockHeader<'a> {
+    tag: &'a str,
+    length: usize,
+}
+
+impl<'a> TlvBlockHeader<'a> {
+    const SIZE: usize = size_of::<[u8; 4]>() + size_of::<u32>();
+
+    /// Parses the first [`Self::SIZE`] bytes of `hdr` (caller may pass a longer slice).
+    fn parse(hdr: &'a [u8]) -> Option<Self> {
+        let hdr = hdr.get(..Self::SIZE)?;
+        let tag_bytes = hdr.get(..4)?;
+        let tag = core::str::from_utf8(tag_bytes).ok()?;
+        if !tag.is_ascii() {
+            return None;
+        }
+        let len_arr = <[u8; 4]>::try_from(hdr.get(4..Self::SIZE)?).ok()?;
+        let length = u32::from_le_bytes(len_arr) as usize;
+        Some(Self { tag, length })
+    }
+}
+
+/// Sequential scan over [`Tlv`]. Unlike [`Tlv`], this type carries a cursor (`pos`) into
+/// the parent blob.  It is not interchangeable with a fresh [`Tlv`] view of the same
+/// bytes.
+///
+/// # Invariants
+///
+/// `pos` is a byte offset into `tlv.data` that always lies on a block boundary (in the sense
+/// of the [`Tlv`] invariant): it is either the start of a well-formed block, or equal to
+/// `tlv.data.len()` (end of iteration).
+struct TlvIter<'tlv, 'a> {
+    tlv: &'tlv Tlv<'a>,
+    pos: usize,
+}
+
+impl<'tlv, 'a> Iterator for TlvIter<'tlv, 'a> {
+    type Item = TlvBlock<'a>;
+
+    /// Returns the block starting at `self.pos` and advances the cursor past it, or [`None`]
+    /// once the cursor reaches the end of the data.
+    ///
+    /// The body relies on a number of `unsafe` operations, because [`Iterator::next`] can
+    /// only return [`Option`], not [`Result`], so there is no way to report a parse error
+    /// from here.  That is why the entire TLV stream is validated up front in the
+    /// constructor.  By this type's invariants, `self.pos` is always on a block boundary
+    /// into already-validated data, so each block is known to be well-formed, and
+    /// therefore the unchecked operations cannot fail.
+    fn next(&mut self) -> Option<Self::Item> {
+        if self.pos >= self.tlv.data.len() {
+            return None;
+        }
+
+        let tail = &self.tlv.data[self.pos..];
+
+        // SAFETY: `self.pos` is on a block boundary (`TlvIter` invariant) and the check
+        // above gives `self.pos < data.len()`, so by the `Tlv` invariant a complete
+        // well-formed block starts at `tail`.
+        let hdr = unsafe { tail.get_unchecked(..TlvBlockHeader::SIZE) };
+
+        // SAFETY: same header bytes as validated in `Tlv::new` for this offset.
+        let header = unsafe { TlvBlockHeader::parse(hdr).unwrap_unchecked() };
+
+        let stored_size = header.length.next_multiple_of(4);
+        let advance = TlvBlockHeader::SIZE + stored_size;
+        let payload_end = TlvBlockHeader::SIZE + header.length;
+
+        // SAFETY: `advance` and `payload_end` are exactly the stored and logical payload extents
+        // `Tlv::new` accepted for this block.
+        let value = unsafe {
+            let block = tail.get_unchecked(..advance);
+            block.get_unchecked(TlvBlockHeader::SIZE..payload_end)
+        };
+
+        // INVARIANT: by the `Tlv` invariant the block at `self.pos` occupies exactly `advance`
+        // bytes, so `self.pos + advance` is the next block boundary (or `data.len()`).
+        self.pos += advance;
+
+        Some(TlvBlock {
+            tag: header.tag,
+            value,
+        })
+    }
+}
+
+/// The payload of a validated TLV (type, length, value) firmware image.
+///
+/// TLV firmware images start with a 4-byte "NVFW" magic header, followed by a sequence of
+/// blocks. Each block has a 4-byte type tag, a 4-byte length field, and a data payload
+/// whose stored size is the length rounded up to the nearest multiple of 4.
+///
+/// [`Self::new`] checks the magic header and walks every block: tags must be ASCII,
+/// lengths and padding must fit without overflow, and the byte stream after `NVFW` must
+/// be exactly partitionable into blocks (no trailing partial header or slack). After
+/// that, [`TlvIter`] only signals end-of-stream via [`None`], not parse failure.
+///
+/// # Invariants
+///
+/// `data` is a validated TLV payload (the bytes *after* the `NVFW` magic): it is the exact
+/// concatenation of zero or more well-formed blocks, with no trailing partial header or slack.
+/// Consequently, any offset `o` into `data` that is a block boundary and satisfies
+/// `o < data.len()` is the start of a complete block whose header parses and whose stored
+/// extent (`TlvBlockHeader::SIZE + header.length.next_multiple_of(4)` bytes) lies within
+/// `data`. `data.len()` is itself a boundary.
+#[allow(dead_code)]
+pub(crate) struct Tlv<'a> {
+    data: &'a [u8],
+}
+
+#[allow(dead_code)]
+impl<'a> Tlv<'a> {
+    const MAGIC: &'static [u8; 4] = b"NVFW";
+
+    /// Parses `data` as a TLV firmware image, returning [`EINVAL`] if the image is malformed.
+    pub(crate) fn new(data: &'a [u8]) -> Result<Self> {
+        // Verify that the magic bytes exist and are the correct value
+        let magic_len = Self::MAGIC.len();
+        if data
+            .get(..magic_len)
+            .is_none_or(|magic| magic != Self::MAGIC)
+        {
+            return Err(EINVAL);
+        }
+
+        // The payload is the contiguous sequence of TLV blocks after the magic.
+        let payload = data.get(magic_len..).ok_or(EINVAL)?;
+
+        let mut pos = 0usize;
+        while pos < payload.len() {
+            // Get the next TLV block.
+            let Some(rest) = payload.get(pos..) else {
+                return Err(EINVAL);
+            };
+            // Validate and extract the header (type, length).
+            let Some(header) = rest
+                .get(..TlvBlockHeader::SIZE)
+                .and_then(TlvBlockHeader::parse)
+            else {
+                return Err(EINVAL);
+            };
+            // The `length` field of a TLV block contains the actual byte length of the
+            // value, but each TLV block is aligned to a 4-byte boundary.
+            let Some(stored_size) = header.length.checked_next_multiple_of(4) else {
+                return Err(EINVAL);
+            };
+            let end = pos
+                .checked_add(TlvBlockHeader::SIZE)
+                .and_then(|p| p.checked_add(stored_size))
+                .ok_or(EINVAL)?;
+            if end > payload.len() {
+                return Err(EINVAL);
+            }
+            pos = end;
+        }
+
+        // INVARIANT: the loop above walked `payload` from offset 0 and only stopped once `pos`
+        // reached `payload.len()` exactly, rejecting any block whose header failed to parse or
+        // whose stored extent overran. So `payload` is an exact concatenation of well-formed
+        // blocks with no partial header or trailing slack.
+        Ok(Self { data: payload })
+    }
+
+    fn iter(&self) -> TlvIter<'_, 'a> {
+        // INVARIANT: 0 is a block boundary, either the start of the first block,
+        // or `data.len()` when `data` is empty.
+        TlvIter { tlv: self, pos: 0 }
+    }
+
+    pub(crate) fn len(&self, tag: &str) -> Result<usize> {
+        let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;
+
+        Ok(tlv.value.len())
+    }
+
+    pub(crate) fn get_bytes(&self, tag: &str) -> Result<&'a [u8]> {
+        let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;
+
+        Ok(tlv.value)
+    }
+
+    pub(crate) fn get_u32(&self, tag: &str) -> Result<u32> {
+        let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;
+
+        tlv.value
+            .try_into()
+            .ok()
+            .map(u32::from_le_bytes)
+            .ok_or(EINVAL)
+    }
+
+    pub(crate) fn get_string(&self, tag: &str) -> Result<&'a str> {
+        let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;
+
+        // Handle the possibility that the value is null-terminated.
+        let bytes = match CStr::from_bytes_until_nul(tlv.value) {
+            Ok(cstr) => cstr.to_bytes(),
+            Err(_) => tlv.value,
+        };
+
+        // But do require it to be all ASCII.
+        if !bytes.is_ascii() {
+            return Err(EINVAL);
+        }
+
+        core::str::from_utf8(bytes).map_err(|_| EINVAL)
+    }
+
+    /// Returns the `n`-th fixed-size chunk from the value of the TLV entry matching
+    /// `tag`.
+    ///
+    /// The value is treated as an array of contiguous, equally-sized chunks, each
+    /// `sig_size` bytes long. `sig_size` is the size of one signature.  The returned
+    /// slice is the `n`-th chunk.
+    ///
+    /// Returns [`EINVAL`] if no entry matches `tag`, if computing the chunk's offset
+    /// overflows, or if the value is too short to contain the requested chunk.
+    pub(crate) fn get_nth_chunk(&self, tag: &str, sig_size: usize, n: usize) -> Result<&'a [u8]> {
+        let sigs = self
+            .iter()
+            .find(|b| b.tag == tag)
+            .map(|b| b.value)
+            .ok_or(EINVAL)?;
+
+        let start = sig_size.checked_mul(n).ok_or(EINVAL)?;
+        let end = start.checked_add(sig_size).ok_or(EINVAL)?;
+
+        sigs.get(start..end).ok_or(EINVAL)
+    }
+}
-- 
2.54.0

[PATCH 4/8] gpu: nova-core: transition booter_load to TLV images

[Timur Tabi]

Switch the booter firmware loader from the legacy binary format to the
TLV format.  This change requires the new TLV versions of the r570.144
firmware images.

The new TLV format has all of the metadata needed by Nova encoded as
separate tags, eliminating the need to parse legacy firmware headers
such as HsHeaderV2 and HsSignatureParams.  All of the structs and
code for parsing those headers is therefore deleted.

Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 drivers/gpu/nova-core/firmware.rs        |  15 +-
 drivers/gpu/nova-core/firmware/booter.rs | 328 ++++-------------------
 drivers/gpu/nova-core/gsp/hal/tu102.rs   |  15 +-
 3 files changed, 63 insertions(+), 295 deletions(-)

diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
index 888a426b7b41..f632db8283f0 100644
--- a/drivers/gpu/nova-core/firmware.rs
+++ b/drivers/gpu/nova-core/firmware.rs
@@ -21,10 +21,7 @@
         FalconFirmware, //
     },
     gpu,
-    num::{
-        FromSafeCast,
-        IntoSafeCast, //
-    },
+    num::IntoSafeCast,
 };
 
 pub(crate) mod booter;
@@ -411,16 +408,6 @@ fn new(fw: &'a firmware::Firmware) -> Result<Self> {
             .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 = usize::from_safe_cast(self.hdr.data_offset);
-        let fw_size = usize::from_safe_cast(self.hdr.data_size);
-        let fw_end = fw_start.checked_add(fw_size)?;
-
-        self.fw.get(fw_start..fw_end)
-    }
 }
 
 pub(crate) struct ModInfoBuilder<const N: usize>(firmware::ModInfoBuilder<N>);
diff --git a/drivers/gpu/nova-core/firmware/booter.rs b/drivers/gpu/nova-core/firmware/booter.rs
index d9313ac361af..c748aa94cd0e 100644
--- a/drivers/gpu/nova-core/firmware/booter.rs
+++ b/drivers/gpu/nova-core/firmware/booter.rs
@@ -10,8 +10,7 @@
 use kernel::{
     device,
     dma::Coherent,
-    prelude::*,
-    transmute::FromBytes, //
+    prelude::*, //
 };
 
 use crate::{
@@ -25,224 +24,16 @@
         FalconFirmware, //
     },
     firmware::{
-        BinFirmware,
         FirmwareObject,
         FirmwareSignature,
         Signed,
+        Tlv,
         Unsigned, //
     },
     gpu::Chipset,
-    num::{
-        FromSafeCast,
-        IntoSafeCast, //
-    },
+    num::IntoSafeCast,
 };
 
-/// 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> {
-    let end = offset.checked_add(size_of::<S>()).ok_or(EINVAL)?;
-    slice
-        .get(offset..end)
-        .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_offset: u32,
-    /// Offset to a `u32` containing the index of the signature to patch.
-    patch_sig_offset: 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_offset: 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.into_safe_cast())
-            .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_offset.into_safe_cast())
-    }
-
-    /// 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_offset.into_safe_cast())?;
-        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_offset.into_safe_cast())?;
-
-                let signatures_start = self
-                    .hdr
-                    .sig_prod_offset
-                    .checked_add(patch_sig)
-                    .map(usize::from_safe_cast)
-                    .ok_or(EINVAL)?;
-
-                let signatures_end = signatures_start
-                    .checked_add(usize::from_safe_cast(self.hdr.sig_prod_size))
-                    .ok_or(EINVAL)?;
-
-                self.fw
-                    // Get signatures range.
-                    .get(signatures_start..signatures_end)
-                    .ok_or(EINVAL)?
-                    .chunks_exact(sig_size.into_safe_cast())
-            }
-        };
-
-        // 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 = usize::from_safe_cast(hs_fw.hdr.meta_data_offset);
-        let end = start
-            .checked_add(hs_fw.hdr.meta_data_size.into_safe_cast())
-            .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.into_safe_cast())
-    }
-}
-
-/// 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,
-                usize::from_safe_cast(hs_fw.hdr.header_offset)
-                    // Skip the load header...
-                    .checked_add(size_of::<HsLoadHeaderV2>())
-                    // ... and jump to app header `idx`.
-                    .and_then(|offset| {
-                        offset
-                            .checked_add(usize::from_safe_cast(idx).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]);
@@ -292,7 +83,6 @@ pub(crate) fn new(
         dev: &device::Device<device::Bound>,
         kind: BooterKind,
         chipset: Chipset,
-        ver: &str,
         falcon: &Falcon<<Self as FalconFirmware>::Target>,
         bar: Bar0<'_>,
     ) -> Result<Self> {
@@ -300,68 +90,64 @@ pub(crate) fn new(
             BooterKind::Loader => "booter_load",
             BooterKind::Unloader => "booter_unload",
         };
-        let fw = super::request_firmware(dev, chipset, fw_name, ver)?;
-        let bin_fw = BinFirmware::new(&fw)?;
+        let fw = super::request_tlv(dev, chipset, fw_name)?;
+        let tlv = Tlv::new(fw.data())?;
+        dev_info!(dev, "loaded booter firmware v{}\n", tlv.get_string("VERS")?);
+
+        let os_data_offset = tlv.get_u32("DAOF")?;
+        let os_data_size = tlv.get_u32("DASZ")?;
+        let os_code_offset = tlv.get_u32("CDOF")?;
+        let os_code_size = tlv.get_u32("CDSZ")?;
+        let patch_loc = tlv.get_u32("PLOC")?;
+        let fuse_version = tlv.get_u32("FUSE")?;
+        let engine_id = tlv.get_u32("ENID")?;
+        let ucode_id = tlv.get_u32("UCID")?;
+        let app0_code_offset = tlv.get_u32("A0CO")?;
+        let app0_code_size = tlv.get_u32("A0CS")?;
+        let num_sigs = tlv.get_u32("NSIG")?;
 
-        // 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
+            // `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)?,
+            pkc_data_offset: patch_loc.checked_sub(os_data_offset).ok_or(EINVAL)?,
+            engine_id_mask: u16::try_from(engine_id).map_err(|_| EINVAL)?,
+            ucode_id: u8::try_from(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)
+        let ucode = tlv
+            .get_bytes("BLOB")
             .and_then(FirmwareObject::<Self, _>::new_booter)?;
 
-        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 ucode_signed = if num_sigs == 0 {
+            // 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,
+            )?;
+
+            const FUSE_VERSION_USE_LAST_SIG: u32 = 0;
 
+            let index = match reg_fuse_version {
                 // `0` means the last signature should be used.
-                const FUSE_VERSION_USE_LAST_SIG: u32 = 0;
-                let signature = match reg_fuse_version {
-                    FUSE_VERSION_USE_LAST_SIG => signatures.last(),
-                    // Otherwise hardware fuse version needs to be subtracted 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.into_safe_cast())
-                    }
-                }
+                FUSE_VERSION_USE_LAST_SIG => num_sigs - 1,
+                // Otherwise, hardware fuse version needs to be subtracted to obtain the index.
+                _ => fuse_version.checked_sub(reg_fuse_version).ok_or(EINVAL)?,
+            };
+
+            // The size of one signature
+            let sig_size = tlv
+                .len("SIGS")?
+                .checked_div(num_sigs.into_safe_cast())
                 .ok_or(EINVAL)?;
 
-                ucode.patch_signature(&signature, patch_loc.into_safe_cast())?
-            }
+            let signature = BooterSignature(tlv.get_nth_chunk("SIGS", sig_size, index as usize)?);
+
+            ucode.patch_signature(&signature, patch_loc.into_safe_cast())?
         };
 
         // There are two versions of Booter, one for Turing/GA100, and another for
@@ -370,11 +156,11 @@ pub(crate) fn new(
         // don't indicate the versions.  The only way to differentiate is by the Chipset.
         let (imem_sec_dst_start, imem_ns_load_target) = if chipset <= Chipset::GA100 {
             (
-                app0.offset,
+                app0_code_offset,
                 Some(FalconDmaLoadTarget {
                     src_start: 0,
-                    dst_start: load_hdr.os_code_offset,
-                    len: load_hdr.os_code_size,
+                    dst_start: os_code_offset,
+                    len: os_code_size,
                 }),
             )
         } else {
@@ -383,15 +169,15 @@ pub(crate) fn new(
 
         Ok(Self {
             imem_sec_load_target: FalconDmaLoadTarget {
-                src_start: app0.offset,
+                src_start: app0_code_offset,
                 dst_start: imem_sec_dst_start,
-                len: app0.len,
+                len: app0_code_size,
             },
             imem_ns_load_target,
             dmem_load_target: FalconDmaLoadTarget {
-                src_start: load_hdr.os_data_offset,
+                src_start: os_data_offset,
                 dst_start: 0,
-                len: load_hdr.os_data_size,
+                len: os_data_size,
             },
             brom_params,
             ucode: ucode_signed,
diff --git a/drivers/gpu/nova-core/gsp/hal/tu102.rs b/drivers/gpu/nova-core/gsp/hal/tu102.rs
index eb7166148cc9..bd103cf99662 100644
--- a/drivers/gpu/nova-core/gsp/hal/tu102.rs
+++ b/drivers/gpu/nova-core/gsp/hal/tu102.rs
@@ -27,8 +27,7 @@
             FwsecCommand,
             FwsecFirmware, //
         },
-        gsp::GspFirmware,
-        FIRMWARE_VERSION, //
+        gsp::GspFirmware, //
     },
     gpu::Chipset,
     gsp::{
@@ -113,7 +112,6 @@ fn build(
                     dev,
                     BooterKind::Unloader,
                     chipset,
-                    FIRMWARE_VERSION,
                     sec2_falcon,
                     bar,
                 )?,
@@ -319,15 +317,12 @@ fn boot<'a>(
             "Using SEC2 to load and run the booter_load firmware...\n"
         );
 
-        BooterFirmware::new(
+        BooterFirmware::new(dev, BooterKind::Loader, chipset, sec2_falcon, bar)?.run(
             dev,
-            BooterKind::Loader,
-            chipset,
-            FIRMWARE_VERSION,
-            sec2_falcon,
             bar,
-        )?
-        .run(dev, bar, sec2_falcon, wpr_meta)?;
+            sec2_falcon,
+            wpr_meta,
+        )?;
 
         Ok(unload_guard)
     }
-- 
2.54.0

[PATCH 5/8] gpu: nova-core: transition gsp to TLV images

[Timur Tabi]

Switch the GSP firmware loader from the legacy binary format to the
TLV format.  This change requires the new TLV versions of the r570.144
firmware images.

Unlike the other TLV firmware images, gsp.tlv contains a pointer to
the actual GSP firmware file instead of its contents.  This allows
each small gsp.tlv file to contain the distinct metadata for each GPU
while still allowing the very large gsp.bin to be shared by all
GPUs.

One key piece of metadata is the signature.  The legacy GSP firmware
image is an ELF file that contains multiple sections that needed
to be parsed, and the driver needed to determine which section is
relevant for the GPU.  Instead, gsp.tlv contains the pre-processed
metadata, so all the driver needs to do is to extract it.

Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 drivers/gpu/nova-core/firmware.rs       | 22 --------
 drivers/gpu/nova-core/firmware/gsp.rs   | 56 ++++++-------------
 drivers/gpu/nova-core/firmware/riscv.rs | 71 +++++--------------------
 drivers/gpu/nova-core/gsp/boot.rs       |  7 +--
 4 files changed, 30 insertions(+), 126 deletions(-)

diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
index f632db8283f0..8ef7f084f834 100644
--- a/drivers/gpu/nova-core/firmware.rs
+++ b/drivers/gpu/nova-core/firmware.rs
@@ -387,28 +387,6 @@ struct BinHdr {
 // 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],
-}
-
-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.
-            .filter(|hdr| hdr.bin_magic == BIN_MAGIC)
-            .map(|hdr| Self { hdr, fw })
-            .ok_or(EINVAL)
-    }
-}
 
 pub(crate) struct ModInfoBuilder<const N: usize>(firmware::ModInfoBuilder<N>);
 
diff --git a/drivers/gpu/nova-core/firmware/gsp.rs b/drivers/gpu/nova-core/firmware/gsp.rs
index 99a302bae567..382005be8516 100644
--- a/drivers/gpu/nova-core/firmware/gsp.rs
+++ b/drivers/gpu/nova-core/firmware/gsp.rs
@@ -8,6 +8,7 @@
         DataDirection,
         DmaAddress, //
     },
+    firmware,
     prelude::*,
     scatterlist::{
         Owned,
@@ -17,13 +18,11 @@
 
 use crate::{
     firmware::{
-        elf,
         riscv::RiscvFirmware, //
+        CString,
+        Tlv,
     },
-    gpu::{
-        Architecture,
-        Chipset, //
-    },
+    gpu::Chipset,
     gsp::GSP_PAGE_SIZE,
     num::FromSafeCast,
 };
@@ -63,43 +62,26 @@ pub(crate) struct GspFirmware {
 }
 
 impl GspFirmware {
-    fn find_gsp_sigs_section(chipset: Chipset) -> &'static str {
-        match chipset.arch() {
-            Architecture::Turing if matches!(chipset, Chipset::TU116 | Chipset::TU117) => {
-                ".fwsignature_tu11x"
-            }
-            Architecture::Turing => ".fwsignature_tu10x",
-            Architecture::Ampere if chipset == Chipset::GA100 => ".fwsignature_ga100",
-            Architecture::Ampere => ".fwsignature_ga10x",
-            Architecture::Ada => ".fwsignature_ad10x",
-            Architecture::Hopper => ".fwsignature_gh10x",
-            Architecture::BlackwellGB10x => ".fwsignature_gb10x",
-            Architecture::BlackwellGB20x => ".fwsignature_gb20x",
-        }
-    }
-
     /// Loads the GSP firmware binaries, map them 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>,
         chipset: Chipset,
-        ver: &'a str,
     ) -> impl PinInit<Self, Error> + 'a {
         pin_init::pin_init_scope(move || {
-            let firmware = super::request_firmware(dev, chipset, "gsp", ver)?;
+            let firmware = super::request_tlv(dev, chipset, "gsp")?;
+            let tlv = Tlv::new(firmware.data())?;
+            dev_info!(dev, "loaded gsp firmware v{}\n", tlv.get_string("VERS")?);
 
-            let fw_section = elf::elf_section(firmware.data(), ".fwimage").ok_or(EINVAL)?;
+            let size = usize::from_safe_cast(tlv.get_u32("SIZE")?);
+            let mut fw_vvec = VVec::from_elem(0u8, size, GFP_KERNEL).map_err(|_| ENOMEM)?;
 
-            let size = fw_section.len();
+            let chip_name = chipset.name();
+            let file = tlv.get_string("FILE")?;
+            let filename = CString::try_from_fmt(fmt!("nvidia/{chip_name}/gsp/{file}"))?;
+            firmware::request_into_buf(&filename, dev, fw_vvec.as_mut_slice())?;
 
-            // Move the firmware into a vmalloc'd vector and map it into the device address
-            // space.
-            let fw_vvec = VVec::with_capacity(fw_section.len(), GFP_KERNEL)
-                .and_then(|mut v| {
-                    v.extend_from_slice(fw_section, GFP_KERNEL)?;
-                    Ok(v)
-                })
-                .map_err(|_| ENOMEM)?;
+            let signatures = Coherent::from_slice(dev, tlv.get_bytes("SIGN")?, GFP_KERNEL)?;
 
             Ok(try_pin_init!(Self {
                 fw <- SGTable::new(dev, fw_vvec, DataDirection::ToDevice, GFP_KERNEL),
@@ -145,15 +127,9 @@ pub(crate) fn new<'a>(
                     level0.into()
                 },
                 size,
-                signatures: {
-                    let sigs_section = Self::find_gsp_sigs_section(chipset);
-
-                    elf::elf_section(firmware.data(), sigs_section)
-                        .ok_or(EINVAL)
-                        .and_then(|data| Coherent::from_slice(dev, data, GFP_KERNEL))?
-                },
+                signatures,
                 bootloader: {
-                    let bl = super::request_firmware(dev, chipset, "bootloader", ver)?;
+                    let bl = super::request_tlv(dev, chipset, "gsp_bootloader")?;
 
                     RiscvFirmware::new(dev, &bl)?
                 },
diff --git a/drivers/gpu/nova-core/firmware/riscv.rs b/drivers/gpu/nova-core/firmware/riscv.rs
index 2afa7f36404e..52087b6bc0ed 100644
--- a/drivers/gpu/nova-core/firmware/riscv.rs
+++ b/drivers/gpu/nova-core/firmware/riscv.rs
@@ -7,53 +7,10 @@
     device,
     dma::Coherent,
     firmware::Firmware,
-    prelude::*,
-    transmute::FromBytes, //
+    prelude::*, //
 };
 
-use crate::{
-    firmware::BinFirmware,
-    num::FromSafeCast, //
-};
-
-/// 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 = usize::from_safe_cast(bin_fw.hdr.header_offset);
-        let end = offset.checked_add(size_of::<Self>()).ok_or(EINVAL)?;
-
-        bin_fw
-            .fw
-            .get(offset..end)
-            .and_then(Self::from_bytes_copy)
-            .ok_or(EINVAL)
-    }
-}
+use crate::firmware::Tlv;
 
 /// A parsed firmware for a RISC-V core, ready to be loaded and run.
 pub(crate) struct RiscvFirmware {
@@ -70,26 +27,22 @@ pub(crate) struct RiscvFirmware {
 }
 
 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 tlv = Tlv::new(fw.data())?;
 
-        let ucode = {
-            let start = usize::from_safe_cast(bin_fw.hdr.data_offset);
-            let len = usize::from_safe_cast(bin_fw.hdr.data_size);
-            let end = start.checked_add(len).ok_or(EINVAL)?;
+        let code_offset = tlv.get_u32("CDOF")?;
+        let data_offset = tlv.get_u32("DAOF")?;
+        let manifest_offset = tlv.get_u32("MFOF")?;
+        let app_version = tlv.get_u32("APPV")?;
 
-            Coherent::from_slice(dev, fw.data().get(start..end).ok_or(EINVAL)?, GFP_KERNEL)?
-        };
+        let ucode = Coherent::from_slice(dev, tlv.get_bytes("BLOB")?, GFP_KERNEL)?;
 
         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,
+            code_offset,
+            data_offset,
+            manifest_offset,
+            app_version,
         })
     }
 }
diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs
index 8afb62d689cb..84fb426a7113 100644
--- a/drivers/gpu/nova-core/gsp/boot.rs
+++ b/drivers/gpu/nova-core/gsp/boot.rs
@@ -20,10 +20,7 @@
         Falcon, //
     },
     fb::FbLayout,
-    firmware::{
-        gsp::GspFirmware,
-        FIRMWARE_VERSION, //
-    },
+    firmware::gsp::GspFirmware,
     gpu::Chipset,
     gsp::{
         cmdq::Cmdq,
@@ -112,7 +109,7 @@ pub(crate) fn boot(
         let dev = pdev.as_ref();
         let hal = super::hal::gsp_hal(chipset);
 
-        let gsp_fw = KBox::pin_init(GspFirmware::new(dev, chipset, FIRMWARE_VERSION), GFP_KERNEL)?;
+        let gsp_fw = KBox::pin_init(GspFirmware::new(dev, chipset), GFP_KERNEL)?;
 
         let fb_layout = FbLayout::new(chipset, bar, &gsp_fw)?;
         dev_dbg!(dev, "{:#x?}\n", fb_layout);
-- 
2.54.0

[PATCH 6/8] gpu: nova-core: transition gen_bootloader to TLV images

[Timur Tabi]

Switch the generic bootloader firmware loader from the legacy binary
format to the TLV format.  This change requires the new TLV versions
of the r570.144 firmware images.

Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 drivers/gpu/nova-core/firmware.rs             | 22 ------
 .../nova-core/firmware/fwsec/bootloader.rs    | 72 +++++--------------
 2 files changed, 16 insertions(+), 78 deletions(-)

diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
index 8ef7f084f834..fbc9e0cd0021 100644
--- a/drivers/gpu/nova-core/firmware.rs
+++ b/drivers/gpu/nova-core/firmware.rs
@@ -366,28 +366,6 @@ fn no_patch_signature(self) -> FirmwareObject<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 {}
-
-
 pub(crate) struct ModInfoBuilder<const N: usize>(firmware::ModInfoBuilder<N>);
 
 impl<const N: usize> ModInfoBuilder<N> {
diff --git a/drivers/gpu/nova-core/firmware/fwsec/bootloader.rs b/drivers/gpu/nova-core/firmware/fwsec/bootloader.rs
index 039920dc340b..ef80ff19127e 100644
--- a/drivers/gpu/nova-core/firmware/fwsec/bootloader.rs
+++ b/drivers/gpu/nova-core/firmware/fwsec/bootloader.rs
@@ -23,10 +23,7 @@
         Alignment, //
     },
     sizes,
-    transmute::{
-        AsBytes,
-        FromBytes, //
-    },
+    transmute::AsBytes,
 };
 
 use crate::{
@@ -46,38 +43,14 @@
     },
     firmware::{
         fwsec::FwsecFirmware,
-        request_firmware,
-        BinHdr,
-        FIRMWARE_VERSION, //
+        request_tlv,
+        Tlv, //
     },
     gpu::Chipset,
     num::FromSafeCast,
     regs,
 };
 
-/// Descriptor used by RM to figure out the requirements of the boot loader.
-///
-/// Most of its fields appear to be legacy and carry incorrect values, so they are left unused.
-#[repr(C)]
-#[derive(Debug, Clone)]
-struct BootloaderDesc {
-    /// Starting tag of bootloader.
-    start_tag: u32,
-    /// DMEM load offset - unused here as we always load at offset `0`.
-    _dmem_load_off: u32,
-    /// Offset of code section in the image. Unused as there is only one section in the bootloader
-    /// binary.
-    _code_off: u32,
-    /// Size of code section in the image.
-    code_size: u32,
-    /// Offset of data section in the image. Unused as we build the data section ourselves.
-    _data_off: u32,
-    /// Size of data section in the image. Unused as we build the data section ourselves.
-    _data_size: u32,
-}
-// SAFETY: any byte sequence is valid for this struct.
-unsafe impl FromBytes for BootloaderDesc {}
-
 /// Structure used by the boot-loader to load the rest of the code.
 ///
 /// This has to be filled by the GPU driver and copied into DMEM at offset
@@ -150,38 +123,23 @@ pub(crate) fn new(
         dev: &Device<device::Bound>,
         chipset: Chipset,
     ) -> Result<Self> {
-        let fw = request_firmware(dev, chipset, "gen_bootloader", FIRMWARE_VERSION)?;
-        let hdr = fw
-            .data()
-            .get(0..size_of::<BinHdr>())
-            .and_then(BinHdr::from_bytes_copy)
-            .ok_or(EINVAL)?;
-
-        let desc = {
-            let desc_offset = usize::from_safe_cast(hdr.header_offset);
-
-            fw.data()
-                .get(desc_offset..)
-                .and_then(BootloaderDesc::from_bytes_copy_prefix)
-                .ok_or(EINVAL)?
-                .0
-        };
+        let fw = request_tlv(dev, chipset, "gen_bootloader")?;
+        let tlv = Tlv::new(fw.data())?;
+        dev_info!(
+            dev,
+            "loaded generic bootloader firmware v{}\n",
+            tlv.get_string("VERS")?
+        );
 
         let ucode = {
-            let ucode_start = usize::from_safe_cast(hdr.data_offset);
-            let code_size = usize::from_safe_cast(desc.code_size);
-            // Align to falcon block size (256 bytes).
+            let blob = tlv.get_bytes("BLOB")?;
+            let code_size = usize::from_safe_cast(tlv.get_u32("CDSZ")?);
             let aligned_code_size = code_size
                 .align_up(Alignment::new::<{ falcon::MEM_BLOCK_ALIGNMENT }>())
                 .ok_or(EINVAL)?;
 
             let mut ucode = KVec::with_capacity(aligned_code_size, GFP_KERNEL)?;
-            ucode.extend_from_slice(
-                fw.data()
-                    .get(ucode_start..ucode_start + code_size)
-                    .ok_or(EINVAL)?,
-                GFP_KERNEL,
-            )?;
+            ucode.extend_from_slice(blob.get(..code_size).ok_or(EINVAL)?, GFP_KERNEL)?;
             ucode.resize(aligned_code_size, 0, GFP_KERNEL)?;
 
             ucode
@@ -262,13 +220,15 @@ pub(crate) fn new(
             .checked_sub(ucode.len())
             .ok_or(EOVERFLOW)?;
 
+        let start_tag = u16::try_from(tlv.get_u32("STRT")?)?;
+
         Ok(Self {
             _firmware_dma: firmware_dma,
             ucode,
             dmem_desc,
             brom_params: firmware.brom_params(),
             imem_dst_start: u16::try_from(imem_dst_start)?,
-            start_tag: u16::try_from(desc.start_tag)?,
+            start_tag,
         })
     }
 
-- 
2.54.0

[PATCH 7/8] gpu: nova-core: transition fsp to TLV images

[Timur Tabi]

Switch the FSP firmware loaders from the legacy ELF32 format to the new
TLV format.  This change requires the new TLV versions of the r570.144
firmware images.

Because we are no longer loading ELF images, we can also delete the ELF
parser.

Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 drivers/gpu/nova-core/firmware.rs      | 207 -------------------------
 drivers/gpu/nova-core/firmware/fsp.rs  |  84 +++++-----
 drivers/gpu/nova-core/gsp/hal/gh100.rs |   7 +-
 3 files changed, 39 insertions(+), 259 deletions(-)

diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
index fbc9e0cd0021..354945cafda2 100644
--- a/drivers/gpu/nova-core/firmware.rs
+++ b/drivers/gpu/nova-core/firmware.rs
@@ -421,213 +421,6 @@ pub(crate) const fn create(
     }
 }
 
-/// 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 core::mem::size_of;
-
-    use kernel::{
-        bindings,
-        str::CStr,
-        transmute::FromBytes, //
-    };
-
-    /// Trait to abstract over ELF header differences.
-    trait ElfHeader: FromBytes {
-        fn shnum(&self) -> u16;
-        fn shoff(&self) -> u64;
-        fn shstrndx(&self) -> u16;
-    }
-
-    /// Trait to abstract over ELF section-header differences.
-    trait ElfSectionHeader: FromBytes {
-        fn name(&self) -> u32;
-        fn offset(&self) -> u64;
-        fn size(&self) -> u64;
-    }
-
-    /// Trait describing a matching ELF header and section-header format.
-    trait ElfFormat {
-        type Header: ElfHeader;
-        type SectionHeader: ElfSectionHeader;
-    }
-
-    /// 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 {}
-
-    impl ElfHeader for Elf64Hdr {
-        fn shnum(&self) -> u16 {
-            self.0.e_shnum
-        }
-
-        fn shoff(&self) -> u64 {
-            self.0.e_shoff
-        }
-
-        fn shstrndx(&self) -> u16 {
-            self.0.e_shstrndx
-        }
-    }
-
-    #[repr(transparent)]
-    struct Elf64SHdr(bindings::elf64_shdr);
-    // SAFETY: all bit patterns are valid for this type, and it doesn't use interior mutability.
-    unsafe impl FromBytes for Elf64SHdr {}
-
-    impl ElfSectionHeader for Elf64SHdr {
-        fn name(&self) -> u32 {
-            self.0.sh_name
-        }
-
-        fn offset(&self) -> u64 {
-            self.0.sh_offset
-        }
-
-        fn size(&self) -> u64 {
-            self.0.sh_size
-        }
-    }
-
-    struct Elf64Format;
-
-    impl ElfFormat for Elf64Format {
-        type Header = Elf64Hdr;
-        type SectionHeader = Elf64SHdr;
-    }
-
-    /// Newtype to provide [`FromBytes`] and [`ElfHeader`] implementations for ELF32.
-    #[repr(transparent)]
-    struct Elf32Hdr(bindings::elf32_hdr);
-    // SAFETY: all bit patterns are valid for this type, and it doesn't use interior mutability.
-    unsafe impl FromBytes for Elf32Hdr {}
-
-    impl ElfHeader for Elf32Hdr {
-        fn shnum(&self) -> u16 {
-            self.0.e_shnum
-        }
-
-        fn shoff(&self) -> u64 {
-            u64::from(self.0.e_shoff)
-        }
-
-        fn shstrndx(&self) -> u16 {
-            self.0.e_shstrndx
-        }
-    }
-
-    /// Newtype to provide [`FromBytes`] and [`ElfSectionHeader`] implementations for ELF32.
-    #[repr(transparent)]
-    struct Elf32SHdr(bindings::elf32_shdr);
-    // SAFETY: all bit patterns are valid for this type, and it doesn't use interior mutability.
-    unsafe impl FromBytes for Elf32SHdr {}
-
-    impl ElfSectionHeader for Elf32SHdr {
-        fn name(&self) -> u32 {
-            self.0.sh_name
-        }
-
-        fn offset(&self) -> u64 {
-            u64::from(self.0.sh_offset)
-        }
-
-        fn size(&self) -> u64 {
-            u64::from(self.0.sh_size)
-        }
-    }
-
-    struct Elf32Format;
-
-    impl ElfFormat for Elf32Format {
-        type Header = Elf32Hdr;
-        type SectionHeader = Elf32SHdr;
-    }
-
-    /// Returns a NULL-terminated string from the ELF image at `offset`.
-    fn elf_str(elf: &[u8], offset: u64) -> Option<&str> {
-        let idx = usize::try_from(offset).ok()?;
-        let bytes = elf.get(idx..)?;
-        CStr::from_bytes_until_nul(bytes).ok()?.to_str().ok()
-    }
-
-    fn elf_section_generic<'a, F>(elf: &'a [u8], name: &str) -> Option<&'a [u8]>
-    where
-        F: ElfFormat,
-    {
-        let hdr = F::Header::from_bytes(elf.get(0..size_of::<F::Header>())?)?;
-
-        let shdr_num = usize::from(hdr.shnum());
-        let shdr_start = usize::try_from(hdr.shoff()).ok()?;
-        let shdr_end = shdr_num
-            .checked_mul(size_of::<F::SectionHeader>())
-            .and_then(|v| v.checked_add(shdr_start))?;
-
-        // Get all the section headers as an iterator over byte chunks.
-        let shdr_bytes = elf.get(shdr_start..shdr_end)?;
-        let mut shdr_iter = shdr_bytes.chunks_exact(size_of::<F::SectionHeader>());
-
-        // Get the strings table.
-        let strhdr = shdr_iter
-            .clone()
-            .nth(usize::from(hdr.shstrndx()))
-            .and_then(F::SectionHeader::from_bytes)?;
-
-        // Find the section which name matches `name` and return it.
-        shdr_iter.find_map(|sh_bytes| {
-            let sh = F::SectionHeader::from_bytes(sh_bytes)?;
-            let name_offset = strhdr.offset().checked_add(u64::from(sh.name()))?;
-            let section_name = elf_str(elf, name_offset)?;
-
-            if section_name != name {
-                return None;
-            }
-
-            let start = usize::try_from(sh.offset()).ok()?;
-            let end = usize::try_from(sh.size())
-                .ok()
-                .and_then(|sz| start.checked_add(sz))?;
-
-            elf.get(start..end)
-        })
-    }
-
-    /// Extract the section with name `name` from the ELF64 image `elf`.
-    fn elf64_section<'a>(elf: &'a [u8], name: &str) -> Option<&'a [u8]> {
-        elf_section_generic::<Elf64Format>(elf, name)
-    }
-
-    /// Extract the section with name `name` from the ELF32 image `elf`.
-    fn elf32_section<'a>(elf: &'a [u8], name: &str) -> Option<&'a [u8]> {
-        elf_section_generic::<Elf32Format>(elf, name)
-    }
-
-    /// Automatically detects ELF32 vs ELF64 based on the ELF header.
-    pub(super) fn elf_section<'a>(elf: &'a [u8], name: &str) -> Option<&'a [u8]> {
-        // ELF identification: a 4-byte magic followed by a class byte (32- vs 64-bit).
-        const ELFMAG: &[u8] = b"\x7fELF";
-        const SELFMAG: usize = ELFMAG.len();
-        const EI_CLASS: usize = 4;
-        const ELFCLASS32: u8 = 1;
-        const ELFCLASS64: u8 = 2;
-
-        if elf.get(0..SELFMAG) != Some(ELFMAG) {
-            return None;
-        }
-
-        match *elf.get(EI_CLASS)? {
-            ELFCLASS32 => elf32_section(elf, name),
-            ELFCLASS64 => elf64_section(elf, name),
-            _ => None,
-        }
-    }
-}
-
 pub(crate) struct TlvBlock<'a> {
     pub(crate) tag: &'a str,
     pub(crate) value: &'a [u8],
diff --git a/drivers/gpu/nova-core/firmware/fsp.rs b/drivers/gpu/nova-core/firmware/fsp.rs
index 6eaf1c684b9d..2c212b195ddb 100644
--- a/drivers/gpu/nova-core/firmware/fsp.rs
+++ b/drivers/gpu/nova-core/firmware/fsp.rs
@@ -6,12 +6,11 @@
 use kernel::{
     device,
     dma::Coherent,
-    firmware::Firmware,
     prelude::*, //
 };
 
 use crate::{
-    firmware::elf,
+    firmware::Tlv,
     gpu::Chipset, //
 };
 
@@ -19,11 +18,11 @@
 const FSP_HASH_SIZE: usize = 48;
 /// Maximum size of the FSP public key (RSA-3072), in bytes.
 ///
-/// The FMC ELF `publickey` section may be shorter, so the remaining bytes are zero-padded.
+/// The FMC `PKEY` tag may be shorter, so the remaining bytes are zero-padded.
 const FSP_PKEY_SIZE: usize = 384;
 /// Maximum size of the FSP signature (RSA-3072), in bytes.
 ///
-/// The FMC ELF `signature` section may be shorter, so the remaining bytes are zero-padded.
+/// The FMC `SIGS` tag may be shorter, so the remaining bytes are zero-padded.
 const FSP_SIG_SIZE: usize = 384;
 
 /// Structure to hold FMC signatures.
@@ -38,64 +37,34 @@ pub(crate) struct FmcSignatures {
 }
 
 pub(crate) struct FspFirmware {
-    /// FMC firmware image data (only the "image" ELF section).
+    /// FMC firmware image data
     pub(crate) fmc_image: Coherent<[u8]>,
     /// FMC firmware signatures.
     pub(crate) fmc_sigs: KBox<FmcSignatures>,
 }
 
 impl FspFirmware {
-    pub(crate) fn new(
-        dev: &device::Device<device::Bound>,
-        chipset: Chipset,
-        ver: &str,
-    ) -> Result<Self> {
-        let fw = super::request_firmware(dev, chipset, "fmc", ver)?;
+    pub(crate) fn new(dev: &device::Device<device::Bound>, chipset: Chipset) -> Result<Self> {
+        let fw = super::request_tlv(dev, chipset, "fmc")?;
+        let tlv = Tlv::new(fw.data())?;
 
-        // FSP expects only the "image" section, not the entire ELF file.
-        let fmc_image_data = elf::elf_section(fw.data(), "image").ok_or_else(|| {
-            dev_err!(dev, "FMC ELF file missing 'image' section\n");
-            EINVAL
-        })?;
+        let fmc_image_data = tlv.get_bytes("BLOB")?;
         let fmc_image = Coherent::from_slice(dev, fmc_image_data, GFP_KERNEL)?;
 
         Ok(Self {
             fmc_image,
-            fmc_sigs: Self::extract_fmc_signatures(&fw, dev)?,
+            fmc_sigs: Self::extract_fmc_signatures(&tlv, dev)?,
         })
     }
 
     /// Extract FMC firmware signatures for Chain of Trust verification.
     ///
-    /// Extracts real cryptographic signatures from FMC ELF32 firmware sections.
+    /// Extracts real cryptographic signatures from FMC TLV firmware tags.
     /// Returns signatures in a heap-allocated structure to prevent stack overflow.
-    fn extract_fmc_signatures(
-        fmc_fw: &Firmware,
-        dev: &device::Device,
-    ) -> Result<KBox<FmcSignatures>> {
-        let get_section = |name: &str, max_len: usize| {
-            elf::elf_section(fmc_fw.data(), name)
-                .ok_or(EINVAL)
-                .inspect_err(|_| dev_err!(dev, "FMC firmware missing '{}' section\n", name))
-                .and_then(|section| {
-                    if section.len() > max_len {
-                        dev_err!(
-                            dev,
-                            "FMC {} section size {} > maximum {}\n",
-                            name,
-                            section.len(),
-                            max_len
-                        );
-                        Err(EINVAL)
-                    } else {
-                        Ok(section)
-                    }
-                })
-        };
-
-        let hash_section = get_section("hash", FSP_HASH_SIZE)?;
-        let pkey_section = get_section("publickey", FSP_PKEY_SIZE)?;
-        let sig_section = get_section("signature", FSP_SIG_SIZE)?;
+    fn extract_fmc_signatures(tlv: &Tlv<'_>, dev: &device::Device) -> Result<KBox<FmcSignatures>> {
+        let hash_section = tlv.get_bytes("HASH")?;
+        let pkey_section = tlv.get_bytes("PKEY")?;
+        let sig_section = tlv.get_bytes("SIGS")?;
 
         // The hash section is a SHA-384 output: it must be exactly FSP_HASH_SIZE bytes.
         if hash_section.len() != FSP_HASH_SIZE {
@@ -108,15 +77,36 @@ fn extract_fmc_signatures(
             return Err(EINVAL);
         }
 
+        // The key and signature sections are zero-padded to a fixed maximum, so they may be
+        // shorter, but must not exceed the destination buffers.
+        if pkey_section.len() > FSP_PKEY_SIZE {
+            dev_err!(
+                dev,
+                "FMC public key section size {} > maximum {}\n",
+                pkey_section.len(),
+                FSP_PKEY_SIZE
+            );
+            return Err(EINVAL);
+        }
+        if sig_section.len() > FSP_SIG_SIZE {
+            dev_err!(
+                dev,
+                "FMC signature section size {} > maximum {}\n",
+                sig_section.len(),
+                FSP_SIG_SIZE
+            );
+            return Err(EINVAL);
+        }
+
         // Initialize the signatures in place to avoid building the large `FmcSignatures` on the
         // stack, then fill each section from the firmware.
         let signatures = KBox::init(
             pin_init::init_zeroed::<FmcSignatures>().chain(|sigs| {
                 // PANIC: src and dst lengths are both FSP_HASH_SIZE (verified above).
                 sigs.hash384.copy_from_slice(hash_section);
-                // PANIC: dst is sliced to src.len(); src.len() <= FSP_PKEY_SIZE per `get_section`.
+                // PANIC: dst is sliced to src.len(); src.len() <= FSP_PKEY_SIZE (verified above).
                 sigs.public_key[..pkey_section.len()].copy_from_slice(pkey_section);
-                // PANIC: dst is sliced to src.len(); src.len() <= FSP_SIG_SIZE per `get_section`.
+                // PANIC: dst is sliced to src.len(); src.len() <= FSP_SIG_SIZE (verified above).
                 sigs.signature[..sig_section.len()].copy_from_slice(sig_section);
                 Ok(())
             }),
diff --git a/drivers/gpu/nova-core/gsp/hal/gh100.rs b/drivers/gpu/nova-core/gsp/hal/gh100.rs
index 98f5ce197d13..bb4e191bf1b2 100644
--- a/drivers/gpu/nova-core/gsp/hal/gh100.rs
+++ b/drivers/gpu/nova-core/gsp/hal/gh100.rs
@@ -18,10 +18,7 @@
         Falcon, //
     },
     fb::FbLayout,
-    firmware::{
-        fsp::FspFirmware,
-        FIRMWARE_VERSION, //
-    },
+    firmware::fsp::FspFirmware,
     fsp::{
         FmcBootArgs,
         Fsp, //
@@ -160,7 +157,7 @@ fn boot<'a>(
         gsp_falcon: &'a Falcon<GspEngine>,
         sec2_falcon: &'a Falcon<Sec2>,
     ) -> Result<BootUnloadGuard<'a>> {
-        let fsp_fw = FspFirmware::new(dev, chipset, FIRMWARE_VERSION)?;
+        let fsp_fw = FspFirmware::new(dev, chipset)?;
 
         let unload_bundle = crate::gsp::UnloadBundle(
             KBox::new(FspUnloadBundle, GFP_KERNEL)? as KBox<dyn UnloadBundle>
-- 
2.54.0

[PATCH 8/8] gpu: nova-core: update firmware module info for TLV images

[Timur Tabi]

Now that nova-core loads the TLV firmware images, update the firmware
module info to specify those files.

Also remove FIRMWARE_VERSION and request_firmware() as they are no
longer used.

Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 drivers/gpu/nova-core/firmware.rs | 35 +++++++------------------------
 1 file changed, 8 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
index 354945cafda2..1149ef0730e9 100644
--- a/drivers/gpu/nova-core/firmware.rs
+++ b/drivers/gpu/nova-core/firmware.rs
@@ -30,23 +30,6 @@
 pub(crate) mod gsp;
 pub(crate) mod riscv;
 
-#[allow(unused)]
-pub(crate) const FIRMWARE_VERSION: &str = "570.144";
-
-/// Requests the GPU firmware `name` suitable for `chipset`, with version `ver`.
-#[allow(unused)]
-fn request_firmware(
-    dev: &device::Device,
-    chipset: gpu::Chipset,
-    name: &str,
-    ver: &str,
-) -> Result<firmware::Firmware> {
-    let chip_name = chipset.name();
-
-    CString::try_from_fmt(fmt!("nvidia/{chip_name}/gsp/{name}-{ver}.bin"))
-        .and_then(|path| firmware::Firmware::request(&path, dev))
-}
-
 /// Requests the GPU firmware TLV `name` suitable for `chipset`.
 #[allow(unused)]
 fn request_tlv(
@@ -376,10 +359,7 @@ const fn make_entry_file(self, chipset: &str, fw: &str) -> Self {
                 .push("nvidia/")
                 .push(chipset)
                 .push("/gsp/")
-                .push(fw)
-                .push("-")
-                .push(FIRMWARE_VERSION)
-                .push(".bin"),
+                .push(fw),
         )
     }
 
@@ -387,20 +367,21 @@ const fn make_entry_chipset(self, chipset: gpu::Chipset) -> Self {
         let name = chipset.name();
 
         let this = self
-            .make_entry_file(name, "bootloader")
-            .make_entry_file(name, "gsp");
+            .make_entry_file(name, "booter_load.tlv")
+            .make_entry_file(name, "gsp.tlv")
+            .make_entry_file(name, "gsp.bin");
 
         // FSP-based chipsets (Hopper, Blackwell and later) boot the GSP via the FMC image loaded by
         // FSP. Older chipsets use the SEC2 booter instead.
         let this = if chipset.uses_fsp() {
-            this.make_entry_file(name, "fmc")
+            this.make_entry_file(name, "fmc.tlv")
         } else {
-            this.make_entry_file(name, "booter_load")
-                .make_entry_file(name, "booter_unload")
+            this.make_entry_file(name, "booter_load.tlv")
+                .make_entry_file(name, "booter_unload.tlv")
         };
 
         if chipset.needs_fwsec_bootloader() {
-            this.make_entry_file(name, "gen_bootloader")
+            this.make_entry_file(name, "gen_bootloader.tlv")
         } else {
             this
         }
-- 
2.54.0

Re: [PATCH 1/8] rust: firmware: add request_into_buf()

[Gary Guo]

On Wed Jun 10, 2026 at 6:49 PM BST, Timur Tabi wrote:
> Add request_into_buf(), a Rust wrapper around the
> request_firmware_into_buf() function. This variant loads the firmware
> image directly into a caller-provided buffer rather than a
> kernel-allocated one.
>
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> ---
>  rust/kernel/firmware.rs | 45 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> index 71168d8004e2..d2ffaacc2d43 100644
> --- a/rust/kernel/firmware.rs
> +++ b/rust/kernel/firmware.rs
> @@ -120,6 +120,51 @@ fn drop(&mut self) {
>      }
>  }
>  
> +/// Load firmware directly into the caller-provided `buf`.
> +///
> +/// On success the firmware image has been copied into `buf`; the number of bytes
> +/// written is returned. The caller accesses the data through `buf` itself.
> +/// See also `bindings::request_firmware_into_buf`.
> +///
> +/// This is intentionally a stand-alone function rather than a `Firmware` constructor. For
> +/// the `into_buf` path, the firmware data lives in the caller's `buf`, not in a
> +/// kernel-owned buffer, so returning a `Firmware` would expose `Firmware::data()` as an
> +/// alias of `buf`. Since `buf` is `&mut [u8]` and the returned handle does not borrow it,
> +/// the caller could mutate `buf` while reads via `data()` are outstanding (and
> +/// `release_firmware()` does not free `buf` anyway). Releasing the bookkeeping here and
> +/// returning only the size leaves `buf` as the single owner and accessor of the data.
> +pub fn request_into_buf(name: &CStr, dev: &Device, buf: &mut [u8]) -> Result<usize> {

It looks like this API design is very prone to misuse. Patch 5 of this series
for example does not check for the returned size.

Also, I think people might just write `buf[..size]` and then it'll panic because
`size` can be larger.

Best,
Gary

> +    let mut fw: *mut bindings::firmware = core::ptr::null_mut();
> +    let pfw: *mut *mut bindings::firmware = &mut fw;
> +    let pfw: *mut *const bindings::firmware = pfw.cast();
> +
> +    // SAFETY: `pfw` is a valid pointer to a NULL initialized `bindings::firmware` pointer.
> +    // `name` and `dev` are valid as by their type invariants. `buf` is a valid writable
> +    // buffer of `buf.len()` bytes.
> +    let ret = unsafe {
> +        bindings::request_firmware_into_buf(
> +            pfw,
> +            name.as_char_ptr(),
> +            dev.as_raw(),
> +            buf.as_mut_ptr().cast(),
> +            buf.len(),
> +        )
> +    };
> +    if ret != 0 {
> +        return Err(Error::from_errno(ret));
> +    }
> +
> +    // SAFETY: `fw` is a valid pointer returned by `request_firmware_into_buf`.
> +    let size = unsafe { (*fw).size };
> +
> +    // The firmware bytes are now in `buf`, which the caller owns, so we don't need
> +    // the kernel to hang on to it any more.
> +    // SAFETY: `fw` is a valid pointer returned by `request_firmware_into_buf`.
> +    unsafe { bindings::release_firmware(fw) };
> +
> +    Ok(size)
> +}
> +
>  // SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, which is safe to be used from
>  // any thread.
>  unsafe impl Send for Firmware {}

Re: [PATCH 0/8] Transition Nova Core to TLV firmware images

[John Hubbard]

On 6/10/26 10:49 AM, Timur Tabi wrote:
> This patch set transitions nova-core to use the new "TLV" firmware image
> files, instead of the ones that Nouveau uses.
> 
> The current r570.144 images are a mix of binary headers and ELF files that
> are cumbersome to parse in Rust.  There's a significant amount of code
> that just reads in a struct, extracts some offset, and uses it to find
> another struct, only to have nova-core use just a few fields.
> 
> The new format uses a sequence of tag/length/value fields that can be
> iterated over.  The script that generates the TLV files,
> extract-firmware-nova.py, does the extra work to find the specific metadata
> needed by Nova and packages each one separately.
> 
> The TLV versions of r570.144 can be found here:
> 
> 	https://github.com/ttabi/linux-firmware-nova

Hi Timur,

A quick heads up on this, it works for Ampere and Blackwell, but
it is failing on TU117:

<redforge> ktest $ ./tests/nova_basic_tests.py 
PCIe Probe: PASS: Chipset info: all 3 GPUs OK
  ok   0000:c1:00.0: NVIDIA (Chipset: TU117, Architecture: Turing, Revision: a.1)
  ok   0000:c2:00.0: NVIDIA (Chipset: GB202, Architecture: BlackwellGB20x, Revision: a.1)
  ok   0000:01:00.0: NVIDIA (Chipset: GA104, Architecture: Ampere, Revision: a.1)

Get GPU Name: FAIL: GPU name: 2 of 3 GPUs OK, 1 failed
  FAIL 0000:c1:00.0: probe with driver nova-core failed with error -2
  ok   0000:c2:00.0: NVIDIA RTX PRO 6000 Blackwell Max-Q Workstation Edition
  ok   0000:01:00.0: NVIDIA RTX A4000

dmesg:
nova-core 0000:c1:00.0: Direct firmware load for nvidia/tu117/gsp/gsp_bootloader.tlv failed with error -2
nova-core 0000:c1:00.0: probe with driver nova-core failed with error -2

<redforge> firmware $ ll /lib/firmware/nvidia/tu117/gsp/gsp_bootloader.tlv
/bin/ls: cannot access '/lib/firmware/nvidia/tu117/gsp/gsp_bootloader.tlv': No such file or directory

thanks,
-- 
John Hubbard

> 
> along with instructions on how to install them.  We are not planning on
> submitting these images to linux-firmware.  Rather, if this patchset
> is accepted upstream, I expect the small handful of people who are
> actually working on Nova to grab and install these images, which needs
> to be done only once.
> 
> There are still opportunities for improvement.  For example, I would like
> to get rid of more GPU-specific code, especially the GA100 quirks.
> 
> Timur Tabi (8):
>   rust: firmware: add request_into_buf()
>   gpu: nova-core: add request_tlv to load TLV images
>   gpu: nova-core: add TLV parser for firmware files
>   gpu: nova-core: transition booter_load to TLV images
>   gpu: nova-core: transition gsp to TLV images
>   gpu: nova-core: transition gen_bootloader to TLV images
>   gpu: nova-core: transition fsp to TLV images
>   gpu: nova-core: update firmware module info for TLV images
> 
>  Documentation/gpu/nova/core/tlv.rst           | 172 +++++++
>  drivers/gpu/nova-core/firmware.rs             | 451 ++++++++----------
>  drivers/gpu/nova-core/firmware/booter.rs      | 328 +++----------
>  drivers/gpu/nova-core/firmware/fsp.rs         |  84 ++--
>  .../nova-core/firmware/fwsec/bootloader.rs    |  72 +--
>  drivers/gpu/nova-core/firmware/gsp.rs         |  56 +--
>  drivers/gpu/nova-core/firmware/riscv.rs       |  71 +--
>  drivers/gpu/nova-core/gsp/boot.rs             |   7 +-
>  drivers/gpu/nova-core/gsp/hal/gh100.rs        |   7 +-
>  drivers/gpu/nova-core/gsp/hal/tu102.rs        |  15 +-
>  rust/kernel/firmware.rs                       |  45 ++
>  11 files changed, 575 insertions(+), 733 deletions(-)
>  create mode 100644 Documentation/gpu/nova/core/tlv.rst
> 
> 
> base-commit: 9eaff547805f8556992a9474465001c3e128b7bd
> prerequisite-patch-id: 0a9ea098bc3576171da9f6293065fdb4db552466

Re: [PATCH 0/8] Transition Nova Core to TLV firmware images

[Timur Tabi]

On Wed, 2026-06-10 at 11:16 -0700, John Hubbard wrote:
> Get GPU Name: FAIL: GPU name: 2 of 3 GPUs OK, 1 failed
>   FAIL 0000:c1:00.0: probe with driver nova-core failed with error -2
>   ok   0000:c2:00.0: NVIDIA RTX PRO 6000 Blackwell Max-Q Workstation Edition
>   ok   0000:01:00.0: NVIDIA RTX A4000
> 
> dmesg:
> nova-core 0000:c1:00.0: Direct firmware load for nvidia/tu117/gsp/gsp_bootloader.tlv failed with
> error -2
> nova-core 0000:c1:00.0: probe with driver nova-core failed with error -2
> 
> <redforge> firmware $ ll /lib/firmware/nvidia/tu117/gsp/gsp_bootloader.tlv
> /bin/ls: cannot access '/lib/firmware/nvidia/tu117/gsp/gsp_bootloader.tlv': No such file or
> directory

Yeah, I just noticed yesterday that the symlinks() command in my script is broken.  It needs a
symlink for gsp_bootloader.tlv.

I'll fix it soon and force-push an update to the git repo.

Re: [PATCH 1/8] rust: firmware: add request_into_buf()

[Timur Tabi]

On Wed, 2026-06-10 at 19:03 +0100, Gary Guo wrote:
> > +pub fn request_into_buf(name: &CStr, dev: &Device, buf: &mut [u8]) -> Result<usize> {
> 
> It looks like this API design is very prone to misuse. Patch 5 of this series
> for example does not check for the returned size.

I was debating changing it to use request_partial_firmware_into_buf, which would "encourage" users
to verify the return value.

> Also, I think people might just write `buf[..size]` and then it'll panic because
> `size` can be larger.

Why would it panic if the buffer is larger than the size of the file?  The kernel API requires you
to know the size of the file before using request_firmware_into_buf(). If the file on disk is larger
than the buffer, the kernel returns -EFBIG.  If the file on this is smaller, then it is loader
completely and the returned Result<usize> is the number of bytes actually loaded from disk.

This has always been a problem for Nouveau because the GSP binary is 90MB in size, and
request_firmware() allocates a contiguous block of memory for it, only for us to memcpy it to a
vmalloc'd buffer.

So now, thanks to TLVs, we can finally use request_firmware_into_buf() for gsp.bin.

Re: [PATCH 0/8] Transition Nova Core to TLV firmware images

[Timur Tabi]

On Wed, 2026-06-10 at 13:19 -0500, Timur Tabi wrote:
> Yeah, I just noticed yesterday that the symlinks() command in my script is broken.  It needs a
> symlink for gsp_bootloader.tlv.
> 
> I'll fix it soon and force-push an update to the git repo.

Ok, it should be fixed now:

# ls -l /lib/firmware/nvidia/tu117/gsp/
total 108
-rw-rw-r-- 1 ttabi admin 59064 Jun 10 18:48 booter_load.tlv
-rw-rw-r-- 1 ttabi admin 39096 Jun 10 18:48 booter_unload.tlv
lrwxrwxrwx 1 ttabi admin    34 Jun 10 18:48 gen_bootloader.tlv -> ../../tu102/gsp/gen_bootloader.tlv
lrwxrwxrwx 1 ttabi admin    23 Jun 10 18:48 gsp.bin -> ../../tu102/gsp/gsp.bin
-rw-rw-r-- 1 ttabi admin  4152 Jun 10 18:48 gsp.tlv
lrwxrwxrwx 1 ttabi admin    34 Jun 10 18:48 gsp_bootloader.tlv -> ../../tu102/gsp/gsp_bootloader.tlv

Since I did force-push, you will need to this to actually pull it:

git reset HEAD^
git pull

For some reason, `git pull -f` doesn't overwrite local history.

Alternatively, you can just do this:

cd /lib/firmware/nvidia/tu116/gsp/
ln -s ../../tu102/gsp/gsp_bootloader.tlv gsp_bootloader.tlv

This should fix it for tu116 and tu117.

Re: [PATCH 1/8] rust: firmware: add request_into_buf()

[Gary Guo]

On Wed Jun 10, 2026 at 7:24 PM BST, Timur Tabi wrote:
> On Wed, 2026-06-10 at 19:03 +0100, Gary Guo wrote:
>> > +pub fn request_into_buf(name: &CStr, dev: &Device, buf: &mut [u8]) -> Result<usize> {
>> 
>> It looks like this API design is very prone to misuse. Patch 5 of this series
>> for example does not check for the returned size.
>
> I was debating changing it to use request_partial_firmware_into_buf, which would "encourage" users
> to verify the return value.
>
>> Also, I think people might just write `buf[..size]` and then it'll panic because
>> `size` can be larger.
>
> Why would it panic if the buffer is larger than the size of the file?  The kernel API requires you
> to know the size of the file before using request_firmware_into_buf(). If the file on disk is larger
> than the buffer, the kernel returns -EFBIG.  If the file on this is smaller, then it is loader
> completely and the returned Result<usize> is the number of bytes actually loaded from disk.

Right, this is not the partial version of the API, so this would indeed not
happen.

BTW, this thread should have been cc'ed to the RfL list, as the abstraction is
changed.

Best,
Gary

>
> This has always been a problem for Nouveau because the GSP binary is 90MB in size, and
> request_firmware() allocates a contiguous block of memory for it, only for us to memcpy it to a
> vmalloc'd buffer.
>
> So now, thanks to TLVs, we can finally use request_firmware_into_buf() for gsp.bin.

Re: [PATCH 1/8] rust: firmware: add request_into_buf()

[Timur Tabi]

On Wed, 2026-06-10 at 21:26 +0100, Gary Guo wrote:

> > Why would it panic if the buffer is larger than the size of the file?  The kernel API requires
> you
> > to know the size of the file before using request_firmware_into_buf(). If the file on disk is
> larger
> > than the buffer, the kernel returns -EFBIG.  If the file on this is smaller, then it is loader
> > completely and the returned Result<usize> is the number of bytes actually loaded from disk.
> 
> Right, this is not the partial version of the API, so this would indeed not
> happen.

So you don't want me to switch to request_partial_firmware_into_buf?

> BTW, this thread should have been cc'ed to the RfL list, as the abstraction is
> changed.

Oops.  I will do that with v2.

Re: [PATCH 0/8] Transition Nova Core to TLV firmware images

[John Hubbard]

On 6/10/26 11:59 AM, Timur Tabi wrote:
> On Wed, 2026-06-10 at 13:19 -0500, Timur Tabi wrote:
>> Yeah, I just noticed yesterday that the symlinks() command in my script is broken.  It needs a
>> symlink for gsp_bootloader.tlv.
>>
>> I'll fix it soon and force-push an update to the git repo.
> 
> Ok, it should be fixed now:

Yes, all better now:

<redforge> ktest $ ./tests/nova_basic_tests.py 
PCIe Probe: PASS: Chipset info: all 3 GPUs OK
  ok   0000:c1:00.0: NVIDIA (Chipset: TU117, Architecture: Turing, Revision: a.1)
  ok   0000:c2:00.0: NVIDIA (Chipset: GB202, Architecture: BlackwellGB20x, Revision: a.1)
  ok   0000:01:00.0: NVIDIA (Chipset: GA104, Architecture: Ampere, Revision: a.1)

Get GPU Name: PASS: GPU name: all 3 GPUs OK
  ok   0000:c1:00.0: NVIDIA T400 4GB
  ok   0000:c2:00.0: NVIDIA RTX PRO 6000 Blackwell Max-Q Workstation Edition
  ok   0000:01:00.0: NVIDIA RTX A4000


But a follow-up: do you have a quick "undo" command for this? If
not, that's OK, because it's only needed between now and when this
series gets merged, but at the moment I have to bounce back and
forth between reviewing this, and other things that do *not* yet
use TLV.

Just a minor convenience, nothing to waste any real time on of course.

> 
> # ls -l /lib/firmware/nvidia/tu117/gsp/
> total 108
> -rw-rw-r-- 1 ttabi admin 59064 Jun 10 18:48 booter_load.tlv
> -rw-rw-r-- 1 ttabi admin 39096 Jun 10 18:48 booter_unload.tlv
> lrwxrwxrwx 1 ttabi admin    34 Jun 10 18:48 gen_bootloader.tlv -> ../../tu102/gsp/gen_bootloader.tlv
> lrwxrwxrwx 1 ttabi admin    23 Jun 10 18:48 gsp.bin -> ../../tu102/gsp/gsp.bin
> -rw-rw-r-- 1 ttabi admin  4152 Jun 10 18:48 gsp.tlv
> lrwxrwxrwx 1 ttabi admin    34 Jun 10 18:48 gsp_bootloader.tlv -> ../../tu102/gsp/gsp_bootloader.tlv
> 
> Since I did force-push, you will need to this to actually pull it:
> 
> git reset HEAD^
> git pull
> 
> For some reason, `git pull -f` doesn't overwrite local history.

Eh? "git pull -r" works here, even for force-pushed histories, as it
usually does, no need to invent any new steps:

<blueforge> linux-firmware-nova (main)$ git pull -r
remote: Enumerating objects: 3, done.
remote: Total 3 (delta 0), reused 0 (delta 0), pack-reused 3 (from 1)
Unpacking objects: 100% (3/3), 66.03 MiB | 19.29 MiB/s, done.
From https://github.com/ttabi/linux-firmware-nova
 + c2931d3...60f69ef main       -> origin/main  (forced update)
Successfully rebased and updated refs/heads/main.

<blueforge> linux-firmware-nova (main)$ g
60f69eff16b7 nvidia: add v570.144 TLV tarball <Timur Tabi> (Wed, 10 Jun 2026)
1d3cbf7723eb Add README.md LICENCE.nvidia <Timur Tabi> (Fri, 15 May 2026)

And with that, it is all working.

thanks,
-- 
John Hubbard

Re: [PATCH 0/8] Transition Nova Core to TLV firmware images

[Timur Tabi]

On Wed, 2026-06-10 at 14:21 -0700, John Hubbard wrote:
> On 6/10/26 11:59 AM, Timur Tabi wrote:
> > On Wed, 2026-06-10 at 13:19 -0500, Timur Tabi wrote:
> > > Yeah, I just noticed yesterday that the symlinks() command in my script is broken.  It needs a
> > > symlink for gsp_bootloader.tlv.
> > > 
> > > I'll fix it soon and force-push an update to the git repo.
> > 
> > Ok, it should be fixed now:
> 
> Yes, all better now:
> 
> <redforge> ktest $ ./tests/nova_basic_tests.py 
> PCIe Probe: PASS: Chipset info: all 3 GPUs OK
>   ok   0000:c1:00.0: NVIDIA (Chipset: TU117, Architecture: Turing, Revision: a.1)
>   ok   0000:c2:00.0: NVIDIA (Chipset: GB202, Architecture: BlackwellGB20x, Revision: a.1)
>   ok   0000:01:00.0: NVIDIA (Chipset: GA104, Architecture: Ampere, Revision: a.1)
> 
> Get GPU Name: PASS: GPU name: all 3 GPUs OK
>   ok   0000:c1:00.0: NVIDIA T400 4GB
>   ok   0000:c2:00.0: NVIDIA RTX PRO 6000 Blackwell Max-Q Workstation Edition
>   ok   0000:01:00.0: NVIDIA RTX A4000
> 
> 
> But a follow-up: do you have a quick "undo" command for this? If
> not, that's OK, because it's only needed between now and when this
> series gets merged, but at the moment I have to bounce back and
> forth between reviewing this, and other things that do *not* yet
> use TLV.

I'll see if I can add a clean.sh script, but in the meantime, Gemini has a few ideas:
https://gemini.google.com/share/a4b48287389d

> Just a minor convenience, nothing to waste any real time on of course.

It's bitten me also, so it's a good idea.

> > 
> > Since I did force-push, you will need to this to actually pull it:
> > 
> > git reset HEAD^
> > git pull
> > 
> > For some reason, `git pull -f` doesn't overwrite local history.
> 
> Eh? "git pull -r" works here, even for force-pushed histories, as it
> usually does, no need to invent any new steps:

Learn something new every day, thanks.

Re: [PATCH 1/8] rust: firmware: add request_into_buf()

[Danilo Krummrich]

On Wed Jun 10, 2026 at 10:26 PM CEST, Gary Guo wrote:
> BTW, this thread should have been cc'ed to the RfL list, as the abstraction is
> changed.

FIRMWARE LOADER uses the driver-core list, so please Cc:
driver-core@lists.linux.dev and the other FW loader maintainers.

Re: [PATCH 2/8] gpu: nova-core: add request_tlv to load TLV images

[Danilo Krummrich]

On Wed Jun 10, 2026 at 7:49 PM CEST, Timur Tabi wrote:
> Add function request_tlv() to load TLV firmware images.
>
> TLV (type, length, value) files are the new image format used by Nova
> to encapsulate firmware images and their metadata.  Unlike the firmware
> files for previous versions of the firmware, TLV filenames are not
> versioned, and they have a .tlv suffix.
>
> Also add some #[allow(unused)] statements to two identifiers that will
> soon be removed, to keep the patchset clean.
>
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> ---
>  drivers/gpu/nova-core/firmware.rs | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
> index 279fbacd0b8e..2749c196416d 100644
> --- a/drivers/gpu/nova-core/firmware.rs
> +++ b/drivers/gpu/nova-core/firmware.rs
> @@ -33,9 +33,11 @@
>  pub(crate) mod gsp;
>  pub(crate) mod riscv;
>  
> +#[allow(unused)]

This shouldn't be needed, it can just be removed when once it becomes unused,
no?

>  pub(crate) const FIRMWARE_VERSION: &str = "570.144";
>  
>  /// Requests the GPU firmware `name` suitable for `chipset`, with version `ver`.
> +#[allow(unused)]
>  fn request_firmware(
>      dev: &device::Device,
>      chipset: gpu::Chipset,
> @@ -48,6 +50,21 @@ fn request_firmware(
>          .and_then(|path| firmware::Firmware::request(&path, dev))
>  }
>  
> +/// Requests the GPU firmware TLV `name` suitable for `chipset`.
> +#[allow(unused)]

Please use expect instead.

> +fn request_tlv(
> +    dev: &device::Device,
> +    chipset: gpu::Chipset,
> +    name: &str,
> +) -> Result<firmware::Firmware> {
> +    let chip_name = chipset.name();
> +
> +    dev_info!(dev, "loading firmware image {}.tlv\n", name);

Please use dev_dbg!() instead, this should not be printed under normal
operation.

> +    CString::try_from_fmt(fmt!("nvidia/{chip_name}/gsp/{name}.tlv"))
> +        .and_then(|path| firmware::Firmware::request(&path, dev))
> +}
> +
>  /// Structure used to describe some firmwares, notably FWSEC-FRTS.
>  #[repr(C)]
>  #[derive(Debug, Clone)]
> -- 
> 2.54.0

Re: [PATCH 3/8] gpu: nova-core: add TLV parser for firmware files

[Danilo Krummrich]

On Wed Jun 10, 2026 at 7:49 PM CEST, Timur Tabi wrote:
> diff --git a/Documentation/gpu/nova/core/tlv.rst b/Documentation/gpu/nova/core/tlv.rst
> new file mode 100644
> index 000000000000..8223605daa6b
> --- /dev/null
> +++ b/Documentation/gpu/nova/core/tlv.rst
> @@ -0,0 +1,172 @@
> +.. SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +
> +==================================
> +TLV Tags in Nova Firmware Images
> +==================================

Thanks for adding this!

> diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
> index 2749c196416d..888a426b7b41 100644
> --- a/drivers/gpu/nova-core/firmware.rs
> +++ b/drivers/gpu/nova-core/firmware.rs
> @@ -11,7 +11,7 @@
>      device,
>      firmware,
>      prelude::*,
> -    str::CString,
> +    str::{CStr, CString},

NIT: format

>      transmute::FromBytes, //
>  };
>  
> @@ -684,3 +684,240 @@ pub(super) fn elf_section<'a>(elf: &'a [u8], name: &str) -> Option<&'a [u8]> {
>          }
>      }
>  }
> +
> +pub(crate) struct TlvBlock<'a> {
> +    pub(crate) tag: &'a str,
> +    pub(crate) value: &'a [u8],
> +}
> +
> +/// On-wire TLV block header: 4-byte ASCII tag + little-endian payload length (bytes, excluding
> +/// padding to a 4-byte boundary).
> +struct TlvBlockHeader<'a> {
> +    tag: &'a str,
> +    length: usize,
> +}
> +
> +impl<'a> TlvBlockHeader<'a> {
> +    const SIZE: usize = size_of::<[u8; 4]>() + size_of::<u32>();
> +
> +    /// Parses the first [`Self::SIZE`] bytes of `hdr` (caller may pass a longer slice).
> +    fn parse(hdr: &'a [u8]) -> Option<Self> {
> +        let hdr = hdr.get(..Self::SIZE)?;
> +        let tag_bytes = hdr.get(..4)?;
> +        let tag = core::str::from_utf8(tag_bytes).ok()?;
> +        if !tag.is_ascii() {
> +            return None;
> +        }
> +        let len_arr = <[u8; 4]>::try_from(hdr.get(4..Self::SIZE)?).ok()?;
> +        let length = u32::from_le_bytes(len_arr) as usize;

Here and in a couple other cases, nova-core depends on !CPU_BIG_ENDIAN, so that
shouldn't be necessary.

I'm not concerned to keep it for documentation purposes, but it may be a bit
inconsistent with the rest of the driver.

> +        Some(Self { tag, length })
> +    }
> +}
> +
> +/// Sequential scan over [`Tlv`]. Unlike [`Tlv`], this type carries a cursor (`pos`) into
> +/// the parent blob.  It is not interchangeable with a fresh [`Tlv`] view of the same
> +/// bytes.
> +///
> +/// # Invariants
> +///
> +/// `pos` is a byte offset into `tlv.data` that always lies on a block boundary (in the sense
> +/// of the [`Tlv`] invariant): it is either the start of a well-formed block, or equal to
> +/// `tlv.data.len()` (end of iteration).
> +struct TlvIter<'tlv, 'a> {
> +    tlv: &'tlv Tlv<'a>,
> +    pos: usize,
> +}
> +
> +impl<'tlv, 'a> Iterator for TlvIter<'tlv, 'a> {
> +    type Item = TlvBlock<'a>;
> +
> +    /// Returns the block starting at `self.pos` and advances the cursor past it, or [`None`]
> +    /// once the cursor reaches the end of the data.
> +    ///
> +    /// The body relies on a number of `unsafe` operations, because [`Iterator::next`] can
> +    /// only return [`Option`], not [`Result`], so there is no way to report a parse error
> +    /// from here.  That is why the entire TLV stream is validated up front in the
> +    /// constructor.  By this type's invariants, `self.pos` is always on a block boundary
> +    /// into already-validated data, so each block is known to be well-formed, and
> +    /// therefore the unchecked operations cannot fail.
> +    fn next(&mut self) -> Option<Self::Item> {
> +        if self.pos >= self.tlv.data.len() {
> +            return None;
> +        }
> +
> +        let tail = &self.tlv.data[self.pos..];
> +
> +        // SAFETY: `self.pos` is on a block boundary (`TlvIter` invariant) and the check
> +        // above gives `self.pos < data.len()`, so by the `Tlv` invariant a complete
> +        // well-formed block starts at `tail`.
> +        let hdr = unsafe { tail.get_unchecked(..TlvBlockHeader::SIZE) };
> +
> +        // SAFETY: same header bytes as validated in `Tlv::new` for this offset.
> +        let header = unsafe { TlvBlockHeader::parse(hdr).unwrap_unchecked() };
> +
> +        let stored_size = header.length.next_multiple_of(4);
> +        let advance = TlvBlockHeader::SIZE + stored_size;
> +        let payload_end = TlvBlockHeader::SIZE + header.length;
> +
> +        // SAFETY: `advance` and `payload_end` are exactly the stored and logical payload extents
> +        // `Tlv::new` accepted for this block.
> +        let value = unsafe {
> +            let block = tail.get_unchecked(..advance);
> +            block.get_unchecked(TlvBlockHeader::SIZE..payload_end)
> +        };

Given that this method returns an Option anyways, this could just use the
corresponding safe accessors, e.g.

	let hdr = tail.get(..TlvBlockHeader::SIZE)?;

Alternatively, we can just use panicking accessors with a '// PANIC' comment,
but again, since the function already returns an Option, this isn't a hot path,
and the panic comments aren't obviously trivial, it might not be worth it in
this case.

That said, I'm fine with either safe or panicking, but I don't like the unsafe
accessors.

> +
> +        // INVARIANT: by the `Tlv` invariant the block at `self.pos` occupies exactly `advance`
> +        // bytes, so `self.pos + advance` is the next block boundary (or `data.len()`).
> +        self.pos += advance;
> +
> +        Some(TlvBlock {
> +            tag: header.tag,
> +            value,
> +        })
> +    }
> +}
> +
> +/// The payload of a validated TLV (type, length, value) firmware image.
> +///
> +/// TLV firmware images start with a 4-byte "NVFW" magic header, followed by a sequence of
> +/// blocks. Each block has a 4-byte type tag, a 4-byte length field, and a data payload
> +/// whose stored size is the length rounded up to the nearest multiple of 4.
> +///
> +/// [`Self::new`] checks the magic header and walks every block: tags must be ASCII,
> +/// lengths and padding must fit without overflow, and the byte stream after `NVFW` must
> +/// be exactly partitionable into blocks (no trailing partial header or slack). After
> +/// that, [`TlvIter`] only signals end-of-stream via [`None`], not parse failure.
> +///
> +/// # Invariants
> +///
> +/// `data` is a validated TLV payload (the bytes *after* the `NVFW` magic): it is the exact
> +/// concatenation of zero or more well-formed blocks, with no trailing partial header or slack.
> +/// Consequently, any offset `o` into `data` that is a block boundary and satisfies
> +/// `o < data.len()` is the start of a complete block whose header parses and whose stored
> +/// extent (`TlvBlockHeader::SIZE + header.length.next_multiple_of(4)` bytes) lies within
> +/// `data`. `data.len()` is itself a boundary.
> +#[allow(dead_code)]
> +pub(crate) struct Tlv<'a> {
> +    data: &'a [u8],
> +}
> +
> +#[allow(dead_code)]
> +impl<'a> Tlv<'a> {
> +    const MAGIC: &'static [u8; 4] = b"NVFW";
> +
> +    /// Parses `data` as a TLV firmware image, returning [`EINVAL`] if the image is malformed.
> +    pub(crate) fn new(data: &'a [u8]) -> Result<Self> {
> +        // Verify that the magic bytes exist and are the correct value
> +        let magic_len = Self::MAGIC.len();
> +        if data
> +            .get(..magic_len)
> +            .is_none_or(|magic| magic != Self::MAGIC)
> +        {
> +            return Err(EINVAL);
> +        }
> +
> +        // The payload is the contiguous sequence of TLV blocks after the magic.
> +        let payload = data.get(magic_len..).ok_or(EINVAL)?;
> +
> +        let mut pos = 0usize;
> +        while pos < payload.len() {
> +            // Get the next TLV block.
> +            let Some(rest) = payload.get(pos..) else {

This check doesn't seem to be needed because the loop constraints it directly
above, so I'd use let rest = &payload[pos..] instead in this case.

> +                return Err(EINVAL);
> +            };
> +            // Validate and extract the header (type, length).
> +            let Some(header) = rest
> +                .get(..TlvBlockHeader::SIZE)
> +                .and_then(TlvBlockHeader::parse)
> +            else {
> +                return Err(EINVAL);
> +            };
> +            // The `length` field of a TLV block contains the actual byte length of the
> +            // value, but each TLV block is aligned to a 4-byte boundary.
> +            let Some(stored_size) = header.length.checked_next_multiple_of(4) else {
> +                return Err(EINVAL);
> +            };
> +            let end = pos
> +                .checked_add(TlvBlockHeader::SIZE)
> +                .and_then(|p| p.checked_add(stored_size))
> +                .ok_or(EINVAL)?;
> +            if end > payload.len() {
> +                return Err(EINVAL);
> +            }
> +            pos = end;
> +        }
> +
> +        // INVARIANT: the loop above walked `payload` from offset 0 and only stopped once `pos`
> +        // reached `payload.len()` exactly, rejecting any block whose header failed to parse or
> +        // whose stored extent overran. So `payload` is an exact concatenation of well-formed
> +        // blocks with no partial header or trailing slack.
> +        Ok(Self { data: payload })
> +    }
> +
> +    fn iter(&self) -> TlvIter<'_, 'a> {
> +        // INVARIANT: 0 is a block boundary, either the start of the first block,
> +        // or `data.len()` when `data` is empty.
> +        TlvIter { tlv: self, pos: 0 }
> +    }
> +
> +    pub(crate) fn len(&self, tag: &str) -> Result<usize> {
> +        let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;
> +
> +        Ok(tlv.value.len())
> +    }
> +
> +    pub(crate) fn get_bytes(&self, tag: &str) -> Result<&'a [u8]> {
> +        let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;
> +
> +        Ok(tlv.value)
> +    }
> +
> +    pub(crate) fn get_u32(&self, tag: &str) -> Result<u32> {
> +        let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;
> +
> +        tlv.value
> +            .try_into()
> +            .ok()
> +            .map(u32::from_le_bytes)
> +            .ok_or(EINVAL)
> +    }
> +
> +    pub(crate) fn get_string(&self, tag: &str) -> Result<&'a str> {
> +        let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;
> +
> +        // Handle the possibility that the value is null-terminated.
> +        let bytes = match CStr::from_bytes_until_nul(tlv.value) {
> +            Ok(cstr) => cstr.to_bytes(),
> +            Err(_) => tlv.value,
> +        };
> +
> +        // But do require it to be all ASCII.
> +        if !bytes.is_ascii() {
> +            return Err(EINVAL);
> +        }
> +
> +        core::str::from_utf8(bytes).map_err(|_| EINVAL)
> +    }

Would it make sense to provide those accessors on TlvBlock, so we don't have to
create a fresh TlvIter and scan from the start every time?

> +
> +    /// Returns the `n`-th fixed-size chunk from the value of the TLV entry matching
> +    /// `tag`.
> +    ///
> +    /// The value is treated as an array of contiguous, equally-sized chunks, each
> +    /// `sig_size` bytes long. `sig_size` is the size of one signature.  The returned
> +    /// slice is the `n`-th chunk.
> +    ///
> +    /// Returns [`EINVAL`] if no entry matches `tag`, if computing the chunk's offset
> +    /// overflows, or if the value is too short to contain the requested chunk.
> +    pub(crate) fn get_nth_chunk(&self, tag: &str, sig_size: usize, n: usize) -> Result<&'a [u8]> {
> +        let sigs = self
> +            .iter()
> +            .find(|b| b.tag == tag)
> +            .map(|b| b.value)
> +            .ok_or(EINVAL)?;
> +
> +        let start = sig_size.checked_mul(n).ok_or(EINVAL)?;
> +        let end = start.checked_add(sig_size).ok_or(EINVAL)?;
> +
> +        sigs.get(start..end).ok_or(EINVAL)
> +    }
> +}
> -- 
> 2.54.0

Re: [PATCH 1/8] rust: firmware: add request_into_buf()

[Alexandre Courbot]

On Thu Jun 11, 2026 at 3:24 AM JST, Timur Tabi wrote:
> On Wed, 2026-06-10 at 19:03 +0100, Gary Guo wrote:
>> > +pub fn request_into_buf(name: &CStr, dev: &Device, buf: &mut [u8]) -> Result<usize> {
>> 
>> It looks like this API design is very prone to misuse. Patch 5 of this series
>> for example does not check for the returned size.
>
> I was debating changing it to use request_partial_firmware_into_buf, which would "encourage" users
> to verify the return value.
>
>> Also, I think people might just write `buf[..size]` and then it'll panic because
>> `size` can be larger.
>
> Why would it panic if the buffer is larger than the size of the file?  The kernel API requires you
> to know the size of the file before using request_firmware_into_buf(). If the file on disk is larger
> than the buffer, the kernel returns -EFBIG.  If the file on this is smaller, then it is loader
> completely and the returned Result<usize> is the number of bytes actually loaded from disk.
>
> This has always been a problem for Nouveau because the GSP binary is 90MB in size, and
> request_firmware() allocates a contiguous block of memory for it, only for us to memcpy it to a
> vmalloc'd buffer.

IIUC `request_firmware` allocates its memory using `vmalloc`. Unless the
firmware is built-in, in which case it returns a pointer to its
location in `.rodata`. That last option is probably the reason for the
unconditional copy in Nouveau.

Re: [PATCH 1/8] rust: firmware: add request_into_buf()

[Alexandre Courbot]

On Thu Jun 11, 2026 at 2:49 AM JST, Timur Tabi wrote:
> Add request_into_buf(), a Rust wrapper around the
> request_firmware_into_buf() function. This variant loads the firmware
> image directly into a caller-provided buffer rather than a
> kernel-allocated one.
>
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> ---
>  rust/kernel/firmware.rs | 45 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> index 71168d8004e2..d2ffaacc2d43 100644
> --- a/rust/kernel/firmware.rs
> +++ b/rust/kernel/firmware.rs
> @@ -120,6 +120,51 @@ fn drop(&mut self) {
>      }
>  }
>  
> +/// Load firmware directly into the caller-provided `buf`.
> +///
> +/// On success the firmware image has been copied into `buf`; the number of bytes
> +/// written is returned. The caller accesses the data through `buf` itself.
> +/// See also `bindings::request_firmware_into_buf`.
> +///
> +/// This is intentionally a stand-alone function rather than a `Firmware` constructor. For
> +/// the `into_buf` path, the firmware data lives in the caller's `buf`, not in a
> +/// kernel-owned buffer, so returning a `Firmware` would expose `Firmware::data()` as an
> +/// alias of `buf`. Since `buf` is `&mut [u8]` and the returned handle does not borrow it,
> +/// the caller could mutate `buf` while reads via `data()` are outstanding (and
> +/// `release_firmware()` does not free `buf` anyway). Releasing the bookkeeping here and
> +/// returning only the size leaves `buf` as the single owner and accessor of the data.
> +pub fn request_into_buf(name: &CStr, dev: &Device, buf: &mut [u8]) -> Result<usize> {

By adding a lifetime parameter you can return a `Result<&'a mut [u8]>`.
This is more sound as the caller cannot use the unwritten part of `buf`.
It is also more convenient as there is now no need to create a sub-slice
manually.

Re: [PATCH 3/8] gpu: nova-core: add TLV parser for firmware files

[Alexandre Courbot]

On Thu Jun 11, 2026 at 7:37 AM JST, Danilo Krummrich wrote:
<...>
>> +pub(crate) struct TlvBlock<'a> {
>> +    pub(crate) tag: &'a str,
>> +    pub(crate) value: &'a [u8],
>> +}
>> +
>> +/// On-wire TLV block header: 4-byte ASCII tag + little-endian payload length (bytes, excluding
>> +/// padding to a 4-byte boundary).
>> +struct TlvBlockHeader<'a> {
>> +    tag: &'a str,
>> +    length: usize,
>> +}
>> +
>> +impl<'a> TlvBlockHeader<'a> {
>> +    const SIZE: usize = size_of::<[u8; 4]>() + size_of::<u32>();
>> +
>> +    /// Parses the first [`Self::SIZE`] bytes of `hdr` (caller may pass a longer slice).
>> +    fn parse(hdr: &'a [u8]) -> Option<Self> {
>> +        let hdr = hdr.get(..Self::SIZE)?;
>> +        let tag_bytes = hdr.get(..4)?;
>> +        let tag = core::str::from_utf8(tag_bytes).ok()?;
>> +        if !tag.is_ascii() {
>> +            return None;
>> +        }
>> +        let len_arr = <[u8; 4]>::try_from(hdr.get(4..Self::SIZE)?).ok()?;
>> +        let length = u32::from_le_bytes(len_arr) as usize;
>
> Here and in a couple other cases, nova-core depends on !CPU_BIG_ENDIAN, so that
> shouldn't be necessary.
>
> I'm not concerned to keep it for documentation purposes, but it may be a bit
> inconsistent with the rest of the driver.

While relying in the little endianness is useful for things like using
`FromBytes` on data structures, I think it still makes sense to handle
it in cases like this one where it is easy to do so.

Code moves, and in the (remote :)) chance that TLV takes off and other
drivers want to pick it up, this is one less fix to pick.

Re: [PATCH 1/8] rust: firmware: add request_into_buf()

[Timur Tabi]

On Thu, 2026-06-11 at 13:58 +0900, Alexandre Courbot wrote:
> > This has always been a problem for Nouveau because the GSP binary is 90MB in size, and
> > request_firmware() allocates a contiguous block of memory for it, only for us to memcpy it
> > to a
> > vmalloc'd buffer.
> 
> IIUC `request_firmware` allocates its memory using `vmalloc`. Unless the
> firmware is built-in, in which case it returns a pointer to its
> location in `.rodata`. That last option is probably the reason for the
> unconditional copy in Nouveau.

It seems you are correct and I'm mis-remembering how request_firmware works.

Still, avoiding the memcpy and a double vmalloc of 90MB seems like a good idea to me.

Re: [PATCH 1/8] rust: firmware: add request_into_buf()

[Timur Tabi]

On Thu, 2026-06-11 at 15:49 +0900, Alexandre Courbot wrote:
> > +/// the caller could mutate `buf` while reads via `data()` are outstanding (and
> > +/// `release_firmware()` does not free `buf` anyway). Releasing the bookkeeping here and
> > +/// returning only the size leaves `buf` as the single owner and accessor of the data.
> > +pub fn request_into_buf(name: &CStr, dev: &Device, buf: &mut [u8]) -> Result<usize> {
> 
> By adding a lifetime parameter you can return a `Result<&'a mut [u8]>`.
> This is more sound as the caller cannot use the unwritten part of `buf`.
> It is also more convenient as there is now no need to create a sub-slice
> manually.

Sorry, I don't understand.  The caller allocates the buffer any way it wants and passes the
slice.  This function is basically a gloried memcpy, it doesn't care about lifetimes.  Why would
I want to add one?  It even releases the `firmware` object before it exits, so there's no
kernel-allocated data to retain, which is why it's not part of the Firmware struct.

> "the caller cannot use the unwritten part of `buf`"

Sure it can.  It allocated the buffer, and passed a slice of the whole buffer or just a portion.
After the function returns, the caller can do whatever it wants with the buffer, since it owns
it outright.

Re: [PATCH 2/8] gpu: nova-core: add request_tlv to load TLV images

[Timur Tabi]

On Thu, 2026-06-11 at 00:00 +0200, Danilo Krummrich wrote:

> 
> > diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
> > index 279fbacd0b8e..2749c196416d 100644
> > --- a/drivers/gpu/nova-core/firmware.rs
> > +++ b/drivers/gpu/nova-core/firmware.rs
> > @@ -33,9 +33,11 @@
> >  pub(crate) mod gsp;
> >  pub(crate) mod riscv;
> >  
> > +#[allow(unused)]
> 
> This shouldn't be needed, it can just be removed when once it becomes unused,
> no?

Technically, yes.  As a convenience, I removed it here so that I wouldn't have to worry about it
in a later patch.  I find that the patches themselves are cleaner that way.  I don't want to
have a patch where the only change to firmware.rs is adding or removing the `unused` statement.

> 
> >  pub(crate) const FIRMWARE_VERSION: &str = "570.144";
> >  
> >  /// Requests the GPU firmware `name` suitable for `chipset`, with version `ver`.
> > +#[allow(unused)]
> >  fn request_firmware(
> >      dev: &device::Device,
> >      chipset: gpu::Chipset,
> > @@ -48,6 +50,21 @@ fn request_firmware(
> >          .and_then(|path| firmware::Firmware::request(&path, dev))
> >  }
> >  
> > +/// Requests the GPU firmware TLV `name` suitable for `chipset`.
> > +#[allow(unused)]
> 
> Please use expect instead.

Ok, but why should I use `expect` here but `allowed` is for FIRMWARE_VERSION?  As soon as I use
request_tlv() in patch #4, won't get I get a compiler warning and be forced to change it to
`allow`?

> > +fn request_tlv(
> > +    dev: &device::Device,
> > +    chipset: gpu::Chipset,
> > +    name: &str,
> > +) -> Result<firmware::Firmware> {
> > +    let chip_name = chipset.name();
> > +
> > +    dev_info!(dev, "loading firmware image {}.tlv\n", name);
> 
> Please use dev_dbg!() instead, this should not be printed under normal
> operation.

Ok.

Re: [PATCH 2/8] gpu: nova-core: add request_tlv to load TLV images

[Gary Guo]

On Thu Jun 11, 2026 at 5:45 PM BST, Timur Tabi wrote:
> On Thu, 2026-06-11 at 00:00 +0200, Danilo Krummrich wrote:
>
>> 
>> > diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
>> > index 279fbacd0b8e..2749c196416d 100644
>> > --- a/drivers/gpu/nova-core/firmware.rs
>> > +++ b/drivers/gpu/nova-core/firmware.rs
>> > @@ -33,9 +33,11 @@
>> >  pub(crate) mod gsp;
>> >  pub(crate) mod riscv;
>> >  
>> > +#[allow(unused)]
>> 
>> This shouldn't be needed, it can just be removed when once it becomes unused,
>> no?
>
> Technically, yes.  As a convenience, I removed it here so that I wouldn't have to worry about it
> in a later patch.  I find that the patches themselves are cleaner that way.  I don't want to
> have a patch where the only change to firmware.rs is adding or removing the `unused` statement.
>
>> 
>> >  pub(crate) const FIRMWARE_VERSION: &str = "570.144";
>> >  
>> >  /// Requests the GPU firmware `name` suitable for `chipset`, with version `ver`.
>> > +#[allow(unused)]
>> >  fn request_firmware(
>> >      dev: &device::Device,
>> >      chipset: gpu::Chipset,
>> > @@ -48,6 +50,21 @@ fn request_firmware(
>> >          .and_then(|path| firmware::Firmware::request(&path, dev))
>> >  }
>> >  
>> > +/// Requests the GPU firmware TLV `name` suitable for `chipset`.
>> > +#[allow(unused)]
>> 
>> Please use expect instead.
>
> Ok, but why should I use `expect` here but `allowed` is for FIRMWARE_VERSION?  As soon as I use
> request_tlv() in patch #4, won't get I get a compiler warning and be forced to change it to
> `allow`?

The goal is to be able to easily find unused code. If `expect` is used, then
you know for sure the code is unused. If nothing is used then you know for sure
the code is used. If `allow` is used you don't know either way.

`#[allow(unused)]` should only be used when it's used but gated behind some cfg
only.

Best,
Gary

Re: [PATCH 2/8] gpu: nova-core: add request_tlv to load TLV images

[Timur Tabi]

On Thu, 2026-06-11 at 19:17 +0100, Gary Guo wrote:
> The goal is to be able to easily find unused code. If `expect` is used, then
> you know for sure the code is unused. If nothing is used then you know for sure
> the code is used. If `allow` is used you don't know either way.
> 
> `#[allow(unused)]` should only be used when it's used but gated behind some cfg
> only.

This doesn't make any sense to me.  Why would I keep code around if I expect it to be unused? 
May as well just delete the code.

Are are you saying that in patch #1, I add expect(unused), and then in patch #2 I remove the
expect(unused) when I actually use it?  Because that seems dumb to me.

To me, the whole point behind allow(unused) is so that I don't need to remove it right away when
the last user is deleted.  Or conversely, (which is what I'm doing in this case), allow me to
add the function without any callers in one patch, and then add the callers in a subsequent
patch.  Then, after all the callers have been added, I can remove the allow(unused) cleanly.

This gives you a clean sequence of patches, without having to deal with pointless "unusued"
warnings that would happen if you only applied a subset of the patches.

Re: [PATCH 3/8] gpu: nova-core: add TLV parser for firmware files

[John Hubbard]

On 6/10/26 11:51 PM, Alexandre Courbot wrote:
> On Thu Jun 11, 2026 at 7:37 AM JST, Danilo Krummrich wrote:
> <...>
>>> +        let len_arr = <[u8; 4]>::try_from(hdr.get(4..Self::SIZE)?).ok()?;
>>> +        let length = u32::from_le_bytes(len_arr) as usize;
>>
>> Here and in a couple other cases, nova-core depends on !CPU_BIG_ENDIAN, so that
>> shouldn't be necessary.
>>
>> I'm not concerned to keep it for documentation purposes, but it may be a bit
>> inconsistent with the rest of the driver.
> 
> While relying in the little endianness is useful for things like using
> `FromBytes` on data structures, I think it still makes sense to handle
> it in cases like this one where it is easy to do so.
> 

Ah no, we don't want to add code that is not useful, even if it is easy.
We should not have code in Nova that is "aspirational"!

Also, it's actually worse if we are inconsistent about LE/BE throughout
the driver, because then no one knows why the code is (sometimes!) there.

> Code moves, and in the (remote :)) chance that TLV takes off and other

Not all by itself. Some thought is required.

> drivers want to pick it up, this is one less fix to pick.

This sort of reasoning leads to over-engineering, which is of course
a sin. :)

Build things correctly for their intended use, and be clear, clean
and concise. Then if and when it gets promoted to more general use,
that's the time to make any changes.

thanks,
-- 
John Hubbard

Re: [PATCH 2/8] gpu: nova-core: add request_tlv to load TLV images

[Gary Guo]

On Thu Jun 11, 2026 at 7:26 PM BST, Timur Tabi wrote:
> On Thu, 2026-06-11 at 19:17 +0100, Gary Guo wrote:
>> The goal is to be able to easily find unused code. If `expect` is used, then
>> you know for sure the code is unused. If nothing is used then you know for sure
>> the code is used. If `allow` is used you don't know either way.
>> 
>> `#[allow(unused)]` should only be used when it's used but gated behind some cfg
>> only.
>
> This doesn't make any sense to me.  Why would I keep code around if I expect it to be unused? 
> May as well just delete the code.

Which is why `#[allow(unused)]` is discouraged, because it allows people to keep
code around without deleting it.

>
> Are are you saying that in patch #1, I add expect(unused), and then in patch #2 I remove the
> expect(unused) when I actually use it?  Because that seems dumb to me.

Yes, this is what is expected. No, it's not dumb, because the fact a "unused,
but expected to be used" function being actually used is a useful information.

>
> To me, the whole point behind allow(unused) is so that I don't need to remove it right away when
> the last user is deleted.  Or conversely, (which is what I'm doing in this case), allow me to
> add the function without any callers in one patch, and then add the callers in a subsequent
> patch.  Then, after all the callers have been added, I can remove the allow(unused) cleanly.

What prevent you from forgetting to remove the `allow(unused)`?

>
> This gives you a clean sequence of patches, without having to deal with pointless "unusued"
> warnings that would happen if you only applied a subset of the patches.

Same for `#[expect]`

Best,
Gary

Re: [PATCH 2/8] gpu: nova-core: add request_tlv to load TLV images

[Danilo Krummrich]

On Thu Jun 11, 2026 at 6:45 PM CEST, Timur Tabi wrote:
> On Thu, 2026-06-11 at 00:00 +0200, Danilo Krummrich wrote:
>
>> 
>> > diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
>> > index 279fbacd0b8e..2749c196416d 100644
>> > --- a/drivers/gpu/nova-core/firmware.rs
>> > +++ b/drivers/gpu/nova-core/firmware.rs
>> > @@ -33,9 +33,11 @@
>> >  pub(crate) mod gsp;
>> >  pub(crate) mod riscv;
>> >  
>> > +#[allow(unused)]
>> 
>> This shouldn't be needed, it can just be removed when once it becomes unused,
>> no?
>
> Technically, yes.  As a convenience, I removed it here so that I wouldn't have to worry about it
> in a later patch.  I find that the patches themselves are cleaner that way.  I don't want to
> have a patch where the only change to firmware.rs is adding or removing the `unused` statement.

You remove FIRMWARE_VERSION in patch 8 together with its (last) user, so adding
#[allow(unused)] here in unnecessary. Am I missing anything?

>> 
>> >  pub(crate) const FIRMWARE_VERSION: &str = "570.144";
>> >  
>> >  /// Requests the GPU firmware `name` suitable for `chipset`, with version `ver`.
>> > +#[allow(unused)]
>> >  fn request_firmware(
>> >      dev: &device::Device,
>> >      chipset: gpu::Chipset,
>> > @@ -48,6 +50,21 @@ fn request_firmware(
>> >          .and_then(|path| firmware::Firmware::request(&path, dev))
>> >  }
>> >  
>> > +/// Requests the GPU firmware TLV `name` suitable for `chipset`.
>> > +#[allow(unused)]
>> 
>> Please use expect instead.
>
> Ok, but why should I use `expect` here but `allowed` is for FIRMWARE_VERSION?  As soon as I use
> request_tlv() in patch #4, won't get I get a compiler warning and be forced to change it to
> `allow`?

Gary already explained it very well.

Thanks,
Danilo

Re: [PATCH 2/8] gpu: nova-core: add request_tlv to load TLV images

[Timur Tabi]

On Thu, 2026-06-11 at 19:42 +0100, Gary Guo wrote:
> > To me, the whole point behind allow(unused) is so that I don't need to remove it right away
> > when
> > the last user is deleted.  Or conversely, (which is what I'm doing in this case), allow me
> > to
> > add the function without any callers in one patch, and then add the callers in a subsequent
> > patch.  Then, after all the callers have been added, I can remove the allow(unused) cleanly.
> 
> What prevent you from forgetting to remove the `allow(unused)`?

Common sense?  

I can understand if I wrote a patch set that only added request_tlv() without any users, with
the expectation that the users would come in some other patch set.

But this is all in the same patch set, and it's supposed to be applied together.  The only
reason I added allow(unused) at all is in case someone in the future wants to git-bisect and
they land in the middle of my series by coincidence.  It serves no purpose otherwise.

Re: [PATCH 2/8] gpu: nova-core: add request_tlv to load TLV images

[Timur Tabi]

On Thu, 2026-06-11 at 20:53 +0200, Danilo Krummrich wrote:
> 

> > Technically, yes.  As a convenience, I removed it here so that I wouldn't have to worry
> > about it
> > in a later patch.  I find that the patches themselves are cleaner that way.  I don't want to
> > have a patch where the only change to firmware.rs is adding or removing the `unused`
> > statement.
> 
> You remove FIRMWARE_VERSION in patch 8 together with its (last) user, so adding
> #[allow(unused)] here in unnecessary. Am I missing anything?

Ah, you're right.  At some point during development, I had some other patch be the last user.

Thanks for catching that.  I will remove the allow(unused) from FIRMWARE_VERSION.

Re: [PATCH 2/8] gpu: nova-core: add request_tlv to load TLV images

[John Hubbard]

On 6/11/26 11:53 AM, Timur Tabi wrote:
> On Thu, 2026-06-11 at 19:42 +0100, Gary Guo wrote:
>>> To me, the whole point behind allow(unused) is so that I don't need to remove it right away
>>> when
>>> the last user is deleted.  Or conversely, (which is what I'm doing in this case), allow me
>>> to
>>> add the function without any callers in one patch, and then add the callers in a subsequent
>>> patch.  Then, after all the callers have been added, I can remove the allow(unused) cleanly.
>>
>> What prevent you from forgetting to remove the `allow(unused)`?
> 
> Common sense?  
> 
> I can understand if I wrote a patch set that only added request_tlv() without any users, with
> the expectation that the users would come in some other patch set.
> 
> But this is all in the same patch set, and it's supposed to be applied together.  The only
> reason I added allow(unused) at all is in case someone in the future wants to git-bisect and
> they land in the middle of my series by coincidence.  It serves no purpose otherwise.
> 

The series should be git-bisect-friendly, of course.

We've learned that expect(dead_code) is the way to go for this sort of
thing. allow is much worse, because it is too easy for it to linger
(just as here!) when it's not needed.

thanks,
-- 
John Hubbard

Re: [PATCH 1/8] rust: firmware: add request_into_buf()

[Danilo Krummrich]

On Thu Jun 11, 2026 at 6:32 PM CEST, Timur Tabi wrote:
> On Thu, 2026-06-11 at 15:49 +0900, Alexandre Courbot wrote:
>> > +/// the caller could mutate `buf` while reads via `data()` are outstanding (and
>> > +/// `release_firmware()` does not free `buf` anyway). Releasing the bookkeeping here and
>> > +/// returning only the size leaves `buf` as the single owner and accessor of the data.
>> > +pub fn request_into_buf(name: &CStr, dev: &Device, buf: &mut [u8]) -> Result<usize> {
>> 
>> By adding a lifetime parameter you can return a `Result<&'a mut [u8]>`.
>> This is more sound as the caller cannot use the unwritten part of `buf`.
>> It is also more convenient as there is now no need to create a sub-slice
>> manually.
>
> Sorry, I don't understand.  The caller allocates the buffer any way it wants and passes the
> slice.  This function is basically a gloried memcpy, it doesn't care about lifetimes.  Why would
> I want to add one?  It even releases the `firmware` object before it exits, so there's no
> kernel-allocated data to retain, which is why it's not part of the Firmware struct.

It allows you to return a &'a mut [u8], where this is a sub-slice that is
already "trimmed" to the length of the actual firmware image loaded. This way
callers do not need to bother creating a sub-slice themselves.

The lifetime is required to tell the compiler which argument to infer the output
lifetime from.

	 fn request_into_buf<'a>(name: &CStr, dev: &Device, buf: &'a mut [u8]) -> Result<&'a mut [u8]>

This tells the compiler that the returned sub-slice must not outlive buf. Note
that the compiler does only look at the signature, not the function body.


>> "the caller cannot use the unwritten part of `buf`"
>
> Sure it can.  It allocated the buffer, and passed a slice of the whole buffer or just a portion.
> After the function returns, the caller can do whatever it wants with the buffer, since it owns
> it outright.

True, but there is no reason for the caller to create the sub-slice on its own
anymore, i.e. the correct usage becomes the "path of least resistance".

Re: [PATCH 1/8] rust: firmware: add request_into_buf()

[Timur Tabi]

On Thu, 2026-06-11 at 21:18 +0200, Danilo Krummrich wrote:
> > Sure it can.  It allocated the buffer, and passed a slice of the whole buffer or just a
> > portion.
> > After the function returns, the caller can do whatever it wants with the buffer, since it
> > owns
> > it outright.
> 
> True, but there is no reason for the caller to create the sub-slice on its own
> anymore, i.e. the correct usage becomes the "path of least resistance".

Is having the caller create the subslice on its own such a big deal?  Given the returned `size`,
the caller already has all the info it needs.  Most likely, the caller will want to verify
`size` and still use the original buffer.

This just seems like another one of those things that Rust make unnecessarily complicated.

Re: [PATCH 1/8] rust: firmware: add request_into_buf()

[John Hubbard]

On 6/11/26 12:28 PM, Timur Tabi wrote:
> On Thu, 2026-06-11 at 21:18 +0200, Danilo Krummrich wrote:
>>> Sure it can.  It allocated the buffer, and passed a slice of the whole buffer or just a
>>> portion.
>>> After the function returns, the caller can do whatever it wants with the buffer, since it
>>> owns
>>> it outright.
>>
>> True, but there is no reason for the caller to create the sub-slice on its own
>> anymore, i.e. the correct usage becomes the "path of least resistance".
> 
> Is having the caller create the subslice on its own such a big deal?  Given the returned `size`,
> the caller already has all the info it needs.  Most likely, the caller will want to verify
> `size` and still use the original buffer.
> 
> This just seems like another one of those things that Rust make unnecessarily complicated.

Hmmm, interesting. I remain a student of Rust-based design, and am learning
that Rust likes to constrain things early in the call stack (often at
ctor time, even), so that soundness and correctness don't have to be
set up or verified in functions further down.

This is one of those things.

Constraining things so that it is Hard To Do It Wrong is a core theme
of Rust programming, and our experts here are helping guide us into it.

Hope that helps.

thanks,
-- 
John Hubbard

Re: [PATCH 1/8] rust: firmware: add request_into_buf()

[Timur Tabi]

On Thu, 2026-06-11 at 12:31 -0700, John Hubbard wrote:
> Hmmm, interesting. I remain a student of Rust-based design, and am learning
> that Rust likes to constrain things early in the call stack (often at
> ctor time, even), so that soundness and correctness don't have to be
> set up or verified in functions further down.
> 
> This is one of those things.

Ok, I can understand that.  There is just one problem, however.  The code will ignore the returned
slice:

    let mut fw_vvec = VVec::from_elem(0u8, size, GFP_KERNEL).map_err(|_| ENOMEM)?;
...	
    let filename = CString::try_from_fmt(fmt!("nvidia/{chip_name}/gsp/{file}"))?;
    firmware::request_into_buf(&filename, dev, fw_vvec.as_mut_slice())?;
...
    Ok(try_pin_init!(Self {
        fw <- SGTable::new(dev, fw_vvec, DataDirection::ToDevice, GFP_KERNEL),

The returned slice is of no use in this case.  The rest of the code works with the original Vec<u8,
Vmalloc>.  SGTable::new() can't work with a slice.

Re: [PATCH 1/8] rust: firmware: add request_into_buf()

[Danilo Krummrich]

On Thu Jun 11, 2026 at 9:28 PM CEST, Timur Tabi wrote:
> Is having the caller create the subslice on its own such a big deal?  Given
> the returned `size`, the caller already has all the info it needs.  Most
> likely, the caller will want to verify `size` and still use the original
> buffer.

My answer was more about addressing your questions regarding why the approach
would need a lifetime and why it is the better option if you want to do
something with the data before passing it along, and less about expressing an
opinion.

That said, the whole point of the API is that the caller already knows the size
in advance, which is the premise of being able to provide the pre-allocated
buffer in the first place.

For validation the size alone doesn't seem to be overly useful as it is just an
indicator. Either the hardware can report a corrupted image, or you probably
want to do an integrity check in the driver.

Hence, I think this should either return just Result (as nothing else is needed
for now) or a sub-slice. Either is fine with me.

Re: [PATCH 1/8] rust: firmware: add request_into_buf()

[John Hubbard]

On 6/11/26 1:01 PM, Timur Tabi wrote:
> On Thu, 2026-06-11 at 12:31 -0700, John Hubbard wrote:
>> Hmmm, interesting. I remain a student of Rust-based design, and am learning
>> that Rust likes to constrain things early in the call stack (often at
>> ctor time, even), so that soundness and correctness don't have to be
>> set up or verified in functions further down.
>>
>> This is one of those things.
> 
> Ok, I can understand that.  There is just one problem, however.  The code will ignore the returned
> slice:
> 
>     let mut fw_vvec = VVec::from_elem(0u8, size, GFP_KERNEL).map_err(|_| ENOMEM)?;
> ...	
>     let filename = CString::try_from_fmt(fmt!("nvidia/{chip_name}/gsp/{file}"))?;
>     firmware::request_into_buf(&filename, dev, fw_vvec.as_mut_slice())?;
> ...
>     Ok(try_pin_init!(Self {
>         fw <- SGTable::new(dev, fw_vvec, DataDirection::ToDevice, GFP_KERNEL),
> 
> The returned slice is of no use in this case.  The rest of the code works with the original Vec<u8,
> Vmalloc>.  SGTable::new() can't work with a slice.

I see that Danilo has addressed this point, in the nearby thread, so
I think you're all set, and I don't have anything more to add.

thanks,
-- 
John Hubbard

Re: [PATCH 1/8] rust: firmware: add request_into_buf()

[Danilo Krummrich]

On Thu Jun 11, 2026 at 10:01 PM CEST, Timur Tabi wrote:
> The code will ignore the returned slice:

And currently it does ignore the returned usize. :) Please see [1].

[1] https://lore.kernel.org/nova-gpu/DJ6JV2QQY0QD.1ZK1PRRBN5BRP@kernel.org/.

Re: [PATCH 3/8] gpu: nova-core: add TLV parser for firmware files

[Timur Tabi]

On Thu, 2026-06-11 at 00:37 +0200, Danilo Krummrich wrote:
> 
> > +        let len_arr = <[u8; 4]>::try_from(hdr.get(4..Self::SIZE)?).ok()?;
> > +        let length = u32::from_le_bytes(len_arr) as usize;
> 
> Here and in a couple other cases, nova-core depends on !CPU_BIG_ENDIAN, so that
> shouldn't be necessary.
> 
> I'm not concerned to keep it for documentation purposes, but it may be a bit
> inconsistent with the rest of the driver.

Don't we normally need to use endian-specific functions to convert an array of bytes into an
integer?

The only alternative I came up with is this:

        let length = u32::from_bytes_copy(hdr.get(4..Self::SIZE)?)? as usize;

I don't think this is an improvement.

> 
> > +        Some(Self { tag, length })
> > +    }
> > +}
> > +
> > +/// Sequential scan over [`Tlv`]. Unlike [`Tlv`], this type carries a cursor (`pos`) into
> > +/// the parent blob.  It is not interchangeable with a fresh [`Tlv`] view of the same
> > +/// bytes.
> > +///
> > +/// # Invariants
> > +///
> > +/// `pos` is a byte offset into `tlv.data` that always lies on a block boundary (in the sense
> > +/// of the [`Tlv`] invariant): it is either the start of a well-formed block, or equal to
> > +/// `tlv.data.len()` (end of iteration).
> > +struct TlvIter<'tlv, 'a> {
> > +    tlv: &'tlv Tlv<'a>,
> > +    pos: usize,
> > +}
> > +
> > +impl<'tlv, 'a> Iterator for TlvIter<'tlv, 'a> {
> > +    type Item = TlvBlock<'a>;
> > +
> > +    /// Returns the block starting at `self.pos` and advances the cursor past it, or [`None`]
> > +    /// once the cursor reaches the end of the data.
> > +    ///
> > +    /// The body relies on a number of `unsafe` operations, because [`Iterator::next`] can
> > +    /// only return [`Option`], not [`Result`], so there is no way to report a parse error
> > +    /// from here.  That is why the entire TLV stream is validated up front in the
> > +    /// constructor.  By this type's invariants, `self.pos` is always on a block boundary
> > +    /// into already-validated data, so each block is known to be well-formed, and
> > +    /// therefore the unchecked operations cannot fail.
> > +    fn next(&mut self) -> Option<Self::Item> {
> > +        if self.pos >= self.tlv.data.len() {
> > +            return None;
> > +        }
> > +
> > +        let tail = &self.tlv.data[self.pos..];
> > +
> > +        // SAFETY: `self.pos` is on a block boundary (`TlvIter` invariant) and the check
> > +        // above gives `self.pos < data.len()`, so by the `Tlv` invariant a complete
> > +        // well-formed block starts at `tail`.
> > +        let hdr = unsafe { tail.get_unchecked(..TlvBlockHeader::SIZE) };
> > +
> > +        // SAFETY: same header bytes as validated in `Tlv::new` for this offset.
> > +        let header = unsafe { TlvBlockHeader::parse(hdr).unwrap_unchecked() };
> > +
> > +        let stored_size = header.length.next_multiple_of(4);
> > +        let advance = TlvBlockHeader::SIZE + stored_size;
> > +        let payload_end = TlvBlockHeader::SIZE + header.length;
> > +
> > +        // SAFETY: `advance` and `payload_end` are exactly the stored and logical payload
> > extents
> > +        // `Tlv::new` accepted for this block.
> > +        let value = unsafe {
> > +            let block = tail.get_unchecked(..advance);
> > +            block.get_unchecked(TlvBlockHeader::SIZE..payload_end)
> > +        };
> 
> Given that this method returns an Option anyways, this could just use the
> corresponding safe accessors, e.g.
> 
> 	let hdr = tail.get(..TlvBlockHeader::SIZE)?;

Well, I was under the impression that iterators cannot return errors, so I wanted to avoid ?
everywhere.  Wouldn't your code just return None in case of error, falsely indicating that the
iteration was finished?

> Alternatively, we can just use panicking accessors with a '// PANIC' comment,
> but again, since the function already returns an Option, this isn't a hot path,
> and the panic comments aren't obviously trivial, it might not be worth it in
> this case.

Well, the reason the constructor validates the entire TLV is so that the iterator cannot fail.

> That said, I'm fine with either safe or panicking, but I don't like the unsafe
> accessors.

I don't like them either, but I don't know how else to handle this.


> 
> > +
> > +        // INVARIANT: by the `Tlv` invariant the block at `self.pos` occupies exactly `advance`
> > +        // bytes, so `self.pos + advance` is the next block boundary (or `data.len()`).
> > +        self.pos += advance;
> > +
> > +        Some(TlvBlock {
> > +            tag: header.tag,
> > +            value,
> > +        })
> > +    }
> > +}
> > +
> > +/// The payload of a validated TLV (type, length, value) firmware image.
> > +///
> > +/// TLV firmware images start with a 4-byte "NVFW" magic header, followed by a sequence of
> > +/// blocks. Each block has a 4-byte type tag, a 4-byte length field, and a data payload
> > +/// whose stored size is the length rounded up to the nearest multiple of 4.
> > +///
> > +/// [`Self::new`] checks the magic header and walks every block: tags must be ASCII,
> > +/// lengths and padding must fit without overflow, and the byte stream after `NVFW` must
> > +/// be exactly partitionable into blocks (no trailing partial header or slack). After
> > +/// that, [`TlvIter`] only signals end-of-stream via [`None`], not parse failure.
> > +///
> > +/// # Invariants
> > +///
> > +/// `data` is a validated TLV payload (the bytes *after* the `NVFW` magic): it is the exact
> > +/// concatenation of zero or more well-formed blocks, with no trailing partial header or slack.
> > +/// Consequently, any offset `o` into `data` that is a block boundary and satisfies
> > +/// `o < data.len()` is the start of a complete block whose header parses and whose stored
> > +/// extent (`TlvBlockHeader::SIZE + header.length.next_multiple_of(4)` bytes) lies within
> > +/// `data`. `data.len()` is itself a boundary.
> > +#[allow(dead_code)]
> > +pub(crate) struct Tlv<'a> {
> > +    data: &'a [u8],
> > +}
> > +
> > +#[allow(dead_code)]
> > +impl<'a> Tlv<'a> {
> > +    const MAGIC: &'static [u8; 4] = b"NVFW";
> > +
> > +    /// Parses `data` as a TLV firmware image, returning [`EINVAL`] if the image is malformed.
> > +    pub(crate) fn new(data: &'a [u8]) -> Result<Self> {
> > +        // Verify that the magic bytes exist and are the correct value
> > +        let magic_len = Self::MAGIC.len();
> > +        if data
> > +            .get(..magic_len)
> > +            .is_none_or(|magic| magic != Self::MAGIC)
> > +        {
> > +            return Err(EINVAL);
> > +        }
> > +
> > +        // The payload is the contiguous sequence of TLV blocks after the magic.
> > +        let payload = data.get(magic_len..).ok_or(EINVAL)?;
> > +
> > +        let mut pos = 0usize;
> > +        while pos < payload.len() {
> > +            // Get the next TLV block.
> > +            let Some(rest) = payload.get(pos..) else {
> 
> This check doesn't seem to be needed because the loop constraints it directly
> above, so I'd use let rest = &payload[pos..] instead in this case.

Ok.

> > +    pub(crate) fn get_string(&self, tag: &str) -> Result<&'a str> {
> > +        let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;
> > +
> > +        // Handle the possibility that the value is null-terminated.
> > +        let bytes = match CStr::from_bytes_until_nul(tlv.value) {
> > +            Ok(cstr) => cstr.to_bytes(),
> > +            Err(_) => tlv.value,
> > +        };
> > +
> > +        // But do require it to be all ASCII.
> > +        if !bytes.is_ascii() {
> > +            return Err(EINVAL);
> > +        }
> > +
> > +        core::str::from_utf8(bytes).map_err(|_| EINVAL)
> > +    }
> 
> Would it make sense to provide those accessors on TlvBlock, so we don't have to
> create a fresh TlvIter and scan from the start every time?

Sorry, I don't understand.  We have to iterate from the start every time because we have no idea
where in the TLV the requested tag is.  There is no requirement to request tags in order.

Keep in mind that most TLVs only have a few tags in them, so all searches are very quick.
> 

Re: [PATCH 1/8] rust: firmware: add request_into_buf()

[Alexandre Courbot]

On Fri Jun 12, 2026 at 4:28 AM JST, Timur Tabi wrote:
> On Thu, 2026-06-11 at 21:18 +0200, Danilo Krummrich wrote:
>> > Sure it can.  It allocated the buffer, and passed a slice of the whole buffer or just a
>> > portion.
>> > After the function returns, the caller can do whatever it wants with the buffer, since it
>> > owns
>> > it outright.
>> 
>> True, but there is no reason for the caller to create the sub-slice on its own
>> anymore, i.e. the correct usage becomes the "path of least resistance".
>
> Is having the caller create the subslice on its own such a big deal?

It is a potential footgun that we can remove through API design.

For the way we use it in Nova, it is not as useful since we work with
the backing `VVec` to DMA-map the pages, and that `VVec` won't be
automatically resized.

But general users are likely to keep working with the slice if they e.g.
load the firmware using regular I/O. With a returned slice they cannot
forget to check the size.

Re: [PATCH 3/8] gpu: nova-core: add TLV parser for firmware files

[Alexandre Courbot]

On Thu Jun 11, 2026 at 2:49 AM JST, Timur Tabi wrote:
> Add the Tlv struct and supporting types for parsing TLV (type, length,
> value) firmware images. TLV files begin with a 4-byte magic header,
> which must be "NVFW" for Nvidia firmware files.  This is followed by a
> sequence of blocks each containing a 4-byte ASCII tag, a 4-byte
> little-endian length, and a payload padded to a 4-byte boundary.
>
> Tlv::new() validates the entire image up front, so that the iterator can
> subsequently yield blocks without fallible parsing.
>
> Also add accessor methods for the various encoded types that will be used
> by the driver.

Thanks for putting this together, this is quite an improvement.

>
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> ---
>  Documentation/gpu/nova/core/tlv.rst | 172 ++++++++++++++++++++
>  drivers/gpu/nova-core/firmware.rs   | 239 +++++++++++++++++++++++++++-
>  2 files changed, 410 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/gpu/nova/core/tlv.rst
>
> diff --git a/Documentation/gpu/nova/core/tlv.rst b/Documentation/gpu/nova/core/tlv.rst
> new file mode 100644
> index 000000000000..8223605daa6b
> --- /dev/null
> +++ b/Documentation/gpu/nova/core/tlv.rst
> @@ -0,0 +1,172 @@
> +.. SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +
> +==================================
> +TLV Tags in Nova Firmware Images
> +==================================
> +
> +Nova firmware images use a Type-Length-Value (TLV) format to
> +encapsulate firmware components and metadata. The TLV file begins
> +with a 4-byte "magic" header that contains the string "NVFW".
> +Following the header is a sequence of TLV blocks.
> +
> +Each block consists of a 4-byte tag of ASCII characters, a 4-byte
> +length encode as a little-endian non-negative integer, and a

"little-endian unsigned" would be more precise (non-negative can also be
understood as "signed, but >= 0").

> +sequence of bytes, the size of which is equal to the length rounded
> +up to the next multiple of 4.
> +
> +::
> +
> +    +------+------+------+------+
> +    |  'N' |  'V' |  'F' |  'W' |  Magic header
> +    +------+------+------+------+
> +    |  Tag (4 bytes, ASCII)     |  TLV block 0
> +    +---------------------------+
> +    |  Length (4 bytes, LE)     |
> +    +---------------------------+
> +    |                           |
> +    |  Value (length bytes,     |
> +    |  padded to 4-byte align)  |
> +    |                           |
> +    +---------------------------+
> +    |  Tag (4 bytes, ASCII)     |  TLV block 1
> +    +---------------------------+
> +    |  Length (4 bytes, LE)     |
> +    +---------------------------+
> +    |                           |
> +    |  Value (length bytes,     |
> +    |  padded to 4-byte align)  |
> +    |                           |
> +    +---------------------------+
> +    |         ...               |  More TLV blocks
> +    +---------------------------+
> +
> +Values
> +======
> +Values are one of three types:
> +
> +1) Integers, encoded in 32-bit or 64-bit little-endian format.
> +2) Strings, encoded as-is and expected to be ASCII only, without a null terminator.
> +3) An array of bytes.

I think it's worth mentioning that the type is tied to the tag and not
encoded in the format.

> +
> +
> +Common Tags
> +===========
> +These tags appear across multiple firmware types.
> +
> +``VERS`` (string)
> +    Human-readable firmware version string, indicates the version of
> +    the firmware.  Present in all TLV files.
> +
> +A TLV image contains either a ``BLOB`` tag (firmware embedded
> +inline) or a ``SIZE``/``FILE`` pair (firmware stored in a
> +separate file).

We will also want to specify what happens when neither BLOB nor FILE
exist, or when on the contrary both or them are present.

> +
> +``BLOB`` (bytes)
> +    If the firmware microcode binary is stored in the TLV, this tag contains
> +    the actual firmware image bytes.
> +
> +``FILE`` (string)
> +    If the firmware binary is stored as a separate file, this tag contains the
> +    name of that file, assumed to be in the same directory as the TLV.
> +    This tag is always paired with ``SIZE``, so as to allow the driver to
> +    pre-allocate the buffer before loading the file.

Note that `request_firmware` outrights reject paths that contain `..`.
These should probably be excluded from the allowed paths here as well.

> +
> +``SIZE`` (u32)
> +    Total size in bytes of the firmware image to be loaded from
> +    the companion file named by ``FILE``.

We also need to specify what happens if we have a `FILE` tag but no
`SIZE` or vice-versa (presumably, this should be an error?).

Another thing I think we should also formally specify: how are duplicate
tags handled?

> +
> +GSP Firmware Tags
> +=================
> +Used when loading the GSP (GPU System Processor) firmware
> +(``gsp.bin``).

Do you mean `gsp.tlv`? (same for other files).

> +
> +``SIGN`` (bytes)
> +    Cryptographic signatures for the GSP firmware, DMA-mapped

That we DMA-map them is out of scope of the file format imho.

> +    for the GSP bootloader to verify before execution.
> +
> +Booter Firmware Tags
> +====================
> +Used by the Booter firmware (``booter_load.bin``,
> +``booter_unload.bin``).
> +
> +``DAOF`` (u32) - ``os_data_offset``
> +    OS data section offset within the firmware image (absolute
> +    byte offset). Maps to the DMEM load source.
> +
> +``DASZ`` (u32) - ``os_data_size``
> +    OS data section size in bytes.
> +
> +``CDOF`` (u32) - ``os_code_offset``
> +    OS code section offset within the firmware image (absolute
> +    byte offset). Maps to the non-secure IMEM load source.
> +
> +``CDSZ`` (u32) - ``os_code_size``
> +    OS code section size in bytes.
> +
> +``PLOC`` (u32) - ``patch_loc``
> +    Signature patch location -- byte offset within the firmware
> +    image where the selected signature should be written.
> +
> +``FUSE`` (u32) - ``fuse_version``
> +    Fuse version of the firmware, used with the hardware fuse
> +    register to select the correct signature index.
> +
> +``ENID`` (u32) - ``engine_id``
> +    Engine ID mask identifying the falcon engine this firmware
> +    targets.
> +
> +``UCID`` (u32) - ``ucode_id``
> +    Microcode ID used together with the engine ID to query
> +    hardware signature fuse registers.
> +
> +``A0CO`` (u32) - ``app0_code_offset``
> +    App0 code offset -- start of the secure code region within
> +    the firmware image. Used as the IMEM secure section source.
> +
> +``A0CS`` (u32) - ``app0_code_size``
> +    App0 code size in bytes.
> +
> +``NSIG`` (u32) - ``num_sigs``
> +    Number of signatures included in the ``SIGS`` tag. A value
> +    of 0 indicates unsigned firmware.
> +
> +``SIGS`` (bytes)
> +    Concatenated array of firmware signatures. The size of
> +    each signature is the total length of the ``SIGS`` value
> +    divided by ``NSIG``. The correct signature is selected
> +    using the fuse-version-derived index.
> +
> +Generic Bootloader Tags (FWSEC)
> +===============================
> +Used by the generic bootloader (``gen_bootloader.bin``) that
> +loads the FWSEC firmware via DMA on Turing GPUs. Also includes
> +the common ``VERS`` and ``BLOB`` tags.

The common tags are not explicitly mentioned for GSP and Booter - I
think we can remove that last sentence.

> +
> +``CDSZ`` (u32) - ``code_size``
> +    Size in bytes of the bootloader code to copy from the
> +    ``BLOB`` tag and PIO-load into falcon IMEM.
> +
> +``STRT`` (u32) - ``start_tag``
> +    Start tag identifying the IMEM block where execution begins.
> +    The falcon boot address is derived as ``start_tag << 8``.
> +
> +RISC-V Firmware Tags (GSP Bootloader)
> +======================================
> +Used by RISC-V-based firmwares such as the GSP bootloader
> +(``gsp_bootloader.bin``). Also includes the common ``VERS``
> +and ``BLOB`` tags.

Same here.

> +
> +``CDOF`` (u32) - ``code_offset``
> +    Offset within the firmware image at which the code section
> +    starts.
> +
> +``DAOF`` (u32) - ``data_offset``
> +    Offset within the firmware image at which the data section
> +    starts.
> +
> +``MFOF`` (u32) - ``manifest_offset``
> +    Offset within the firmware image at which the manifest
> +    starts.
> +
> +``APPV`` (u32) - ``app_version``
> +    Application version of the firmware.
> diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
> index 2749c196416d..888a426b7b41 100644
> --- a/drivers/gpu/nova-core/firmware.rs
> +++ b/drivers/gpu/nova-core/firmware.rs

Since the code for `Tlv` seems to be nicely self-contained, can you put
it into a sub-module (`firmware/tlv.rs`)? This will reduce the amount of
stuff in `firmware.rs` too.

> @@ -11,7 +11,7 @@
>      device,
>      firmware,
>      prelude::*,
> -    str::CString,
> +    str::{CStr, CString},
>      transmute::FromBytes, //
>  };
>  
> @@ -684,3 +684,240 @@ pub(super) fn elf_section<'a>(elf: &'a [u8], name: &str) -> Option<&'a [u8]> {
>          }
>      }
>  }
> +
> +pub(crate) struct TlvBlock<'a> {

`TlvBlock` and its members can be private IIUC.

> +    pub(crate) tag: &'a str,

This should probably be a `&'a [u8; 4]`? `str` is variable length and
technically UTF-8, using a 4-bytes array is the more accurate type.
Doing this also simplifies `parse` a bit.

> +    pub(crate) value: &'a [u8],
> +}
> +
> +/// On-wire TLV block header: 4-byte ASCII tag + little-endian payload length (bytes, excluding
> +/// padding to a 4-byte boundary).
> +struct TlvBlockHeader<'a> {
> +    tag: &'a str,

Same here.

> +    length: usize,
> +}
> +
> +impl<'a> TlvBlockHeader<'a> {
> +    const SIZE: usize = size_of::<[u8; 4]>() + size_of::<u32>();
> +
> +    /// Parses the first [`Self::SIZE`] bytes of `hdr` (caller may pass a longer slice).
> +    fn parse(hdr: &'a [u8]) -> Option<Self> {
> +        let hdr = hdr.get(..Self::SIZE)?;
> +        let tag_bytes = hdr.get(..4)?;
> +        let tag = core::str::from_utf8(tag_bytes).ok()?;
> +        if !tag.is_ascii() {
> +            return None;
> +        }
> +        let len_arr = <[u8; 4]>::try_from(hdr.get(4..Self::SIZE)?).ok()?;
> +        let length = u32::from_le_bytes(len_arr) as usize;
> +        Some(Self { tag, length })
> +    }
> +}
> +
> +/// Sequential scan over [`Tlv`]. Unlike [`Tlv`], this type carries a cursor (`pos`) into
> +/// the parent blob.  It is not interchangeable with a fresh [`Tlv`] view of the same
> +/// bytes.

The last two sentences are basically implementation details - are they
needed in a public doc?

> +///
> +/// # Invariants
> +///
> +/// `pos` is a byte offset into `tlv.data` that always lies on a block boundary (in the sense
> +/// of the [`Tlv`] invariant): it is either the start of a well-formed block, or equal to
> +/// `tlv.data.len()` (end of iteration).
> +struct TlvIter<'tlv, 'a> {
> +    tlv: &'tlv Tlv<'a>,
> +    pos: usize,
> +}
> +
> +impl<'tlv, 'a> Iterator for TlvIter<'tlv, 'a> {
> +    type Item = TlvBlock<'a>;
> +
> +    /// Returns the block starting at `self.pos` and advances the cursor past it, or [`None`]
> +    /// once the cursor reaches the end of the data.
> +    ///
> +    /// The body relies on a number of `unsafe` operations, because [`Iterator::next`] can
> +    /// only return [`Option`], not [`Result`], so there is no way to report a parse error
> +    /// from here.  That is why the entire TLV stream is validated up front in the
> +    /// constructor.  By this type's invariants, `self.pos` is always on a block boundary
> +    /// into already-validated data, so each block is known to be well-formed, and
> +    /// therefore the unchecked operations cannot fail.

Second paragraph also leaks implementation details and is not relevant
to users.

> +    fn next(&mut self) -> Option<Self::Item> {
> +        if self.pos >= self.tlv.data.len() {
> +            return None;
> +        }
> +
> +        let tail = &self.tlv.data[self.pos..];
> +
> +        // SAFETY: `self.pos` is on a block boundary (`TlvIter` invariant) and the check
> +        // above gives `self.pos < data.len()`, so by the `Tlv` invariant a complete
> +        // well-formed block starts at `tail`.
> +        let hdr = unsafe { tail.get_unchecked(..TlvBlockHeader::SIZE) };
> +
> +        // SAFETY: same header bytes as validated in `Tlv::new` for this offset.
> +        let header = unsafe { TlvBlockHeader::parse(hdr).unwrap_unchecked() };
> +
> +        let stored_size = header.length.next_multiple_of(4);
> +        let advance = TlvBlockHeader::SIZE + stored_size;
> +        let payload_end = TlvBlockHeader::SIZE + header.length;
> +
> +        // SAFETY: `advance` and `payload_end` are exactly the stored and logical payload extents
> +        // `Tlv::new` accepted for this block.
> +        let value = unsafe {
> +            let block = tail.get_unchecked(..advance);
> +            block.get_unchecked(TlvBlockHeader::SIZE..payload_end)
> +        };

As Danilo mentioned we should get rid of these `unsafe` blocks. I think
returning `None` if an error is met is fine: an error is, technically,
the end of the iteration; and we have validated the whole file in the
constructor, so they won't happen anyway.

> +
> +        // INVARIANT: by the `Tlv` invariant the block at `self.pos` occupies exactly `advance`
> +        // bytes, so `self.pos + advance` is the next block boundary (or `data.len()`).
> +        self.pos += advance;
> +
> +        Some(TlvBlock {
> +            tag: header.tag,
> +            value,
> +        })
> +    }
> +}
> +
> +/// The payload of a validated TLV (type, length, value) firmware image.
> +///
> +/// TLV firmware images start with a 4-byte "NVFW" magic header, followed by a sequence of
> +/// blocks. Each block has a 4-byte type tag, a 4-byte length field, and a data payload
> +/// whose stored size is the length rounded up to the nearest multiple of 4.
> +///
> +/// [`Self::new`] checks the magic header and walks every block: tags must be ASCII,
> +/// lengths and padding must fit without overflow, and the byte stream after `NVFW` must
> +/// be exactly partitionable into blocks (no trailing partial header or slack). After
> +/// that, [`TlvIter`] only signals end-of-stream via [`None`], not parse failure.
> +///
> +/// # Invariants
> +///
> +/// `data` is a validated TLV payload (the bytes *after* the `NVFW` magic): it is the exact
> +/// concatenation of zero or more well-formed blocks, with no trailing partial header or slack.
> +/// Consequently, any offset `o` into `data` that is a block boundary and satisfies
> +/// `o < data.len()` is the start of a complete block whose header parses and whose stored
> +/// extent (`TlvBlockHeader::SIZE + header.length.next_multiple_of(4)` bytes) lies within
> +/// `data`. `data.len()` is itself a boundary.
> +#[allow(dead_code)]
> +pub(crate) struct Tlv<'a> {
> +    data: &'a [u8],
> +}
> +
> +#[allow(dead_code)]
> +impl<'a> Tlv<'a> {
> +    const MAGIC: &'static [u8; 4] = b"NVFW";
> +
> +    /// Parses `data` as a TLV firmware image, returning [`EINVAL`] if the image is malformed.
> +    pub(crate) fn new(data: &'a [u8]) -> Result<Self> {
> +        // Verify that the magic bytes exist and are the correct value
> +        let magic_len = Self::MAGIC.len();
> +        if data
> +            .get(..magic_len)
> +            .is_none_or(|magic| magic != Self::MAGIC)
> +        {
> +            return Err(EINVAL);
> +        }
> +
> +        // The payload is the contiguous sequence of TLV blocks after the magic.
> +        let payload = data.get(magic_len..).ok_or(EINVAL)?;
> +
> +        let mut pos = 0usize;

You can work directly with `payload` and avoid storing the position in
its own variable. Just make `payload`'s declaration above `mut`, and
then:

> +        while pos < payload.len() {

    while !payload.is_empty() {

> +            // Get the next TLV block.
> +            let Some(rest) = payload.get(pos..) else {
> +                return Err(EINVAL);
> +            };

This can disappear.

> +            // Validate and extract the header (type, length).
> +            let Some(header) = rest

s/rest/payload

> +                .get(..TlvBlockHeader::SIZE)
> +                .and_then(TlvBlockHeader::parse)
> +            else {
> +                return Err(EINVAL);
> +            };
> +            // The `length` field of a TLV block contains the actual byte length of the
> +            // value, but each TLV block is aligned to a 4-byte boundary.
> +            let Some(stored_size) = header.length.checked_next_multiple_of(4) else {
> +                return Err(EINVAL);
> +            };
> +            let end = pos
> +                .checked_add(TlvBlockHeader::SIZE)
> +                .and_then(|p| p.checked_add(stored_size))
> +                .ok_or(EINVAL)?;

Here you don't need to add `pos` anymore.

> +            if end > payload.len() {
> +                return Err(EINVAL);
> +            }
> +            pos = end;

This becomes:

    payload = payload.split_at_checked(end).ok_or(EINVAL)?.1;

> +        }
> +
> +        // INVARIANT: the loop above walked `payload` from offset 0 and only stopped once `pos`
> +        // reached `payload.len()` exactly, rejecting any block whose header failed to parse or
> +        // whose stored extent overran. So `payload` is an exact concatenation of well-formed
> +        // blocks with no partial header or trailing slack.
> +        Ok(Self { data: payload })
> +    }
> +
> +    fn iter(&self) -> TlvIter<'_, 'a> {
> +        // INVARIANT: 0 is a block boundary, either the start of the first block,
> +        // or `data.len()` when `data` is empty.
> +        TlvIter { tlv: self, pos: 0 }
> +    }
> +
> +    pub(crate) fn len(&self, tag: &str) -> Result<usize> {

This method name usually means something different in Rust - it should
be documented (all methods should be, ideally), or even better renamed
to e.g. `tag_len`.

But maybe we can also just remove it, since it can be expressed as
`get_bytes(tag).len()`, and is only ever used in one place...

> +        let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;
> +
> +        Ok(tlv.value.len())
> +    }
> +
> +    pub(crate) fn get_bytes(&self, tag: &str) -> Result<&'a [u8]> {

For these getter methods as well, `tag` can be `&[u8; 4]`. Callers can
build a literal using the `b` prefix, e.g.

    let fmc_image_data = tlv.get_bytes(b"BLOB")?;

> +        let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;
> +
> +        Ok(tlv.value)

nit: can be collapsed into 

   self.iter().find(|b| b.tag == tag).map(|b| b.value).ok_or(EINVAL)

> +    }
> +
> +    pub(crate) fn get_u32(&self, tag: &str) -> Result<u32> {
> +        let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;

You can reuse `get_bytes` to avoid repeating this snippet (also applies
to the other `get_` methods).

>
> +
> +        tlv.value
> +            .try_into()
> +            .ok()
> +            .map(u32::from_le_bytes)
> +            .ok_or(EINVAL)
> +    }
> +
> +    pub(crate) fn get_string(&self, tag: &str) -> Result<&'a str> {
> +        let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;
> +
> +        // Handle the possibility that the value is null-terminated.
> +        let bytes = match CStr::from_bytes_until_nul(tlv.value) {
> +            Ok(cstr) => cstr.to_bytes(),
> +            Err(_) => tlv.value,
> +        };

The spec says that strings are expected to be ASCII without a NULL
terminator - and the NULL terminator does not carry any meaning in Rust.
So I don't think we should be doing that check.

> +
> +        // But do require it to be all ASCII.
> +        if !bytes.is_ascii() {
> +            return Err(EINVAL);
> +        }

Btw, is it bad if we allow strings to be UTF-8?

> +
> +        core::str::from_utf8(bytes).map_err(|_| EINVAL)
> +    }
> +
> +    /// Returns the `n`-th fixed-size chunk from the value of the TLV entry matching
> +    /// `tag`.
> +    ///
> +    /// The value is treated as an array of contiguous, equally-sized chunks, each
> +    /// `sig_size` bytes long. `sig_size` is the size of one signature.  The returned
> +    /// slice is the `n`-th chunk.
> +    ///
> +    /// Returns [`EINVAL`] if no entry matches `tag`, if computing the chunk's offset
> +    /// overflows, or if the value is too short to contain the requested chunk.

This documentation is confusing - it start by mentioning chunks, then
suddenly signature vocabulary is introduced.

> +    pub(crate) fn get_nth_chunk(&self, tag: &str, sig_size: usize, n: usize) -> Result<&'a [u8]> {
> +        let sigs = self
> +            .iter()
> +            .find(|b| b.tag == tag)
> +            .map(|b| b.value)
> +            .ok_or(EINVAL)?;
> +
> +        let start = sig_size.checked_mul(n).ok_or(EINVAL)?;
> +        let end = start.checked_add(sig_size).ok_or(EINVAL)?;
> +
> +        sigs.get(start..end).ok_or(EINVAL)

This method is single-use Booter-domain logic, and feels a bit too
specialized for a `Tlv` API. The caller in `booter.rs` could do:

    tlv.get_bytes(b"SIGS")?
        .chunks_exact(sig_size)
        .nth(index as usize)
        .ok_or(EINVAL)?;

Re: [PATCH 1/8] rust: firmware: add request_into_buf()

[Alexandre Courbot]

On Thu Jun 11, 2026 at 2:49 AM JST, Timur Tabi wrote:
> Add request_into_buf(), a Rust wrapper around the
> request_firmware_into_buf() function. This variant loads the firmware
> image directly into a caller-provided buffer rather than a
> kernel-allocated one.
>
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> ---
>  rust/kernel/firmware.rs | 45 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> index 71168d8004e2..d2ffaacc2d43 100644
> --- a/rust/kernel/firmware.rs
> +++ b/rust/kernel/firmware.rs
> @@ -120,6 +120,51 @@ fn drop(&mut self) {
>      }
>  }
>  
> +/// Load firmware directly into the caller-provided `buf`.
> +///
> +/// On success the firmware image has been copied into `buf`; the number of bytes
> +/// written is returned. The caller accesses the data through `buf` itself.
> +/// See also `bindings::request_firmware_into_buf`.
> +///
> +/// This is intentionally a stand-alone function rather than a `Firmware` constructor. For
> +/// the `into_buf` path, the firmware data lives in the caller's `buf`, not in a
> +/// kernel-owned buffer, so returning a `Firmware` would expose `Firmware::data()` as an
> +/// alias of `buf`. Since `buf` is `&mut [u8]` and the returned handle does not borrow it,
> +/// the caller could mutate `buf` while reads via `data()` are outstanding (and
> +/// `release_firmware()` does not free `buf` anyway). Releasing the bookkeeping here and
> +/// returning only the size leaves `buf` as the single owner and accessor of the data.

I think this last paragraph can stop after the first sentence - the
stand-alone approach is the correct one here, interested readers can
dive into the code, but this function does not need to justify its
existence to every user.

Re: [PATCH 8/8] gpu: nova-core: update firmware module info for TLV images

[Alexandre Courbot]

On Thu Jun 11, 2026 at 2:49 AM JST, Timur Tabi wrote:
<...>
> @@ -387,20 +367,21 @@ const fn make_entry_chipset(self, chipset: gpu::Chipset) -> Self {
>          let name = chipset.name();
>  
>          let this = self
> -            .make_entry_file(name, "bootloader")
> -            .make_entry_file(name, "gsp");
> +            .make_entry_file(name, "booter_load.tlv")
> +            .make_entry_file(name, "gsp.tlv")
> +            .make_entry_file(name, "gsp.bin");
>  
>          // FSP-based chipsets (Hopper, Blackwell and later) boot the GSP via the FMC image loaded by
>          // FSP. Older chipsets use the SEC2 booter instead.
>          let this = if chipset.uses_fsp() {
> -            this.make_entry_file(name, "fmc")
> +            this.make_entry_file(name, "fmc.tlv")
>          } else {
> -            this.make_entry_file(name, "booter_load")
> -                .make_entry_file(name, "booter_unload")
> +            this.make_entry_file(name, "booter_load.tlv")

`booter_load.tlv` is already unconditionally added above, won't this add
a duplicate entry?

Re: [PATCH 8/8] gpu: nova-core: update firmware module info for TLV images

[Timur Tabi]

On Sat, 2026-06-13 at 00:02 +0900, Alexandre Courbot wrote:
> >           let this = self
> > -            .make_entry_file(name, "bootloader")
> > -            .make_entry_file(name, "gsp");
> > +            .make_entry_file(name, "booter_load.tlv")
> > +            .make_entry_file(name, "gsp.tlv")
> > +            .make_entry_file(name, "gsp.bin");
> >   
> >           // FSP-based chipsets (Hopper, Blackwell and later) boot the GSP via the FMC image
> > loaded by
> >           // FSP. Older chipsets use the SEC2 booter instead.
> >           let this = if chipset.uses_fsp() {
> > -            this.make_entry_file(name, "fmc")
> > +            this.make_entry_file(name, "fmc.tlv")
> >           } else {
> > -            this.make_entry_file(name, "booter_load")
> > -                .make_entry_file(name, "booter_unload")
> > +            this.make_entry_file(name, "booter_load.tlv")
> 
> `booter_load.tlv` is already unconditionally added above, won't this add
> a duplicate entry?

Yes, good catch.  This was a bad merge on top of John's recent patch.

Re: [PATCH 3/8] gpu: nova-core: add TLV parser for firmware files

[Timur Tabi]

On Fri, 2026-06-12 at 23:46 +0900, Alexandre Courbot wrote:
> On Thu Jun 11, 2026 at 2:49 AM JST, Timur Tabi wrote:
> > Add the Tlv struct and supporting types for parsing TLV (type, length,
> > value) firmware images. TLV files begin with a 4-byte magic header,
> > which must be "NVFW" for Nvidia firmware files.  This is followed by a
> > sequence of blocks each containing a 4-byte ASCII tag, a 4-byte
> > little-endian length, and a payload padded to a 4-byte boundary.
> > 
> > Tlv::new() validates the entire image up front, so that the iterator can
> > subsequently yield blocks without fallible parsing.
> > 
> > Also add accessor methods for the various encoded types that will be used
> > by the driver.
> 
> Thanks for putting this together, this is quite an improvement.
> 
> > 
> > Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> > ---
> >  Documentation/gpu/nova/core/tlv.rst | 172 ++++++++++++++++++++
> >  drivers/gpu/nova-core/firmware.rs   | 239 +++++++++++++++++++++++++++-
> >  2 files changed, 410 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/gpu/nova/core/tlv.rst
> > 
> > diff --git a/Documentation/gpu/nova/core/tlv.rst b/Documentation/gpu/nova/core/tlv.rst
> > new file mode 100644
> > index 000000000000..8223605daa6b
> > --- /dev/null
> > +++ b/Documentation/gpu/nova/core/tlv.rst
> > @@ -0,0 +1,172 @@
> > +.. SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +
> > +==================================
> > +TLV Tags in Nova Firmware Images
> > +==================================
> > +
> > +Nova firmware images use a Type-Length-Value (TLV) format to
> > +encapsulate firmware components and metadata. The TLV file begins
> > +with a 4-byte "magic" header that contains the string "NVFW".
> > +Following the header is a sequence of TLV blocks.
> > +
> > +Each block consists of a 4-byte tag of ASCII characters, a 4-byte
> > +length encode as a little-endian non-negative integer, and a
> 
> "little-endian unsigned" would be more precise (non-negative can also be
> understood as "signed, but >= 0").

Ok

> 
> > +sequence of bytes, the size of which is equal to the length rounded
> > +up to the next multiple of 4.
> > +
> > +::
> > +
> > +    +------+------+------+------+
> > +    |  'N' |  'V' |  'F' |  'W' |  Magic header
> > +    +------+------+------+------+
> > +    |  Tag (4 bytes, ASCII)     |  TLV block 0
> > +    +---------------------------+
> > +    |  Length (4 bytes, LE)     |
> > +    +---------------------------+
> > +    |                           |
> > +    |  Value (length bytes,     |
> > +    |  padded to 4-byte align)  |
> > +    |                           |
> > +    +---------------------------+
> > +    |  Tag (4 bytes, ASCII)     |  TLV block 1
> > +    +---------------------------+
> > +    |  Length (4 bytes, LE)     |
> > +    +---------------------------+
> > +    |                           |
> > +    |  Value (length bytes,     |
> > +    |  padded to 4-byte align)  |
> > +    |                           |
> > +    +---------------------------+
> > +    |         ...               |  More TLV blocks
> > +    +---------------------------+
> > +
> > +Values
> > +======
> > +Values are one of three types:
> > +
> > +1) Integers, encoded in 32-bit or 64-bit little-endian format.
> > +2) Strings, encoded as-is and expected to be ASCII only, without a null terminator.
> > +3) An array of bytes.
> 
> I think it's worth mentioning that the type is tied to the tag and not
> encoded in the format.

Ok


> 
> > +
> > +
> > +Common Tags
> > +===========
> > +These tags appear across multiple firmware types.
> > +
> > +``VERS`` (string)
> > +    Human-readable firmware version string, indicates the version of
> > +    the firmware.  Present in all TLV files.
> > +
> > +A TLV image contains either a ``BLOB`` tag (firmware embedded
> > +inline) or a ``SIZE``/``FILE`` pair (firmware stored in a
> > +separate file).
> 
> We will also want to specify what happens when neither BLOB nor FILE
> exist, or when on the contrary both or them are present.

Ok


> 
> > +
> > +``BLOB`` (bytes)
> > +    If the firmware microcode binary is stored in the TLV, this tag contains
> > +    the actual firmware image bytes.
> > +
> > +``FILE`` (string)
> > +    If the firmware binary is stored as a separate file, this tag contains the
> > +    name of that file, assumed to be in the same directory as the TLV.
> > +    This tag is always paired with ``SIZE``, so as to allow the driver to
> > +    pre-allocate the buffer before loading the file.
> 
> Note that `request_firmware` outrights reject paths that contain `..`.
> These should probably be excluded from the allowed paths here as well.

Ok.

I guess that kills the idea of avoiding symlinks.

> We also need to specify what happens if we have a `FILE` tag but no
> `SIZE` or vice-versa (presumably, this should be an error?).

Technically, it's the caller that decides what to do in each case.  There is no central "tag
checker" that verifies any of this.  I felt that this document should be limited to describing
how the tags work if you see one, rather than describing how to actually build a "proper" TLV
file.  No one is expected to build the TLVs outside of the extract_firmware_nova.py script.

So to answer your question, if there's no SIZE tag, then it will need to use request_firmware()
instead of request_firmware_into_buf().  I can add language that says that the SIZE tag should
be created if it's at all possible to know the size when the TLV is built.

I don't really have a strong opinion either way.

> Another thing I think we should also formally specify: how are duplicate
> tags handled?

Well, currently the code will ignore the second copy of the tag, since the iterator always
starts from the beginning.  I'll add some text that says duplicates are not allowed, but if they
exist, only the first tag (in offset order) is used, and the others are ignored.


> 
> > +
> > +GSP Firmware Tags
> > +=================
> > +Used when loading the GSP (GPU System Processor) firmware
> > +(``gsp.bin``).
> 
> Do you mean `gsp.tlv`? (same for other files).

Hmmm.... I don't think so.  But it's obviously so unclear I should remove it.  I think the point
I was trying to make is that this is the TLV that goes with gsp.bin.


> > +``SIGN`` (bytes)
> > +    Cryptographic signatures for the GSP firmware, DMA-mapped
> 
> That we DMA-map them is out of scope of the file format imho.

Ok.  Also, I need to double-check whether this is SIGN or SIGS still.  I should probably
document 

SIGN = one signature
SIGS = an array of same-sized signatures paired with NSIG

> > +    for the GSP bootloader to verify before execution.
> > +
> > +Booter Firmware Tags
> > +====================
> > +Used by the Booter firmware (``booter_load.bin``,
> > +``booter_unload.bin``).
> > +
> > +``DAOF`` (u32) - ``os_data_offset``
> > +    OS data section offset within the firmware image (absolute
> > +    byte offset). Maps to the DMEM load source.
> > +
> > +``DASZ`` (u32) - ``os_data_size``
> > +    OS data section size in bytes.
> > +
> > +``CDOF`` (u32) - ``os_code_offset``
> > +    OS code section offset within the firmware image (absolute
> > +    byte offset). Maps to the non-secure IMEM load source.
> > +
> > +``CDSZ`` (u32) - ``os_code_size``
> > +    OS code section size in bytes.
> > +
> > +``PLOC`` (u32) - ``patch_loc``
> > +    Signature patch location -- byte offset within the firmware
> > +    image where the selected signature should be written.
> > +
> > +``FUSE`` (u32) - ``fuse_version``
> > +    Fuse version of the firmware, used with the hardware fuse
> > +    register to select the correct signature index.
> > +
> > +``ENID`` (u32) - ``engine_id``
> > +    Engine ID mask identifying the falcon engine this firmware
> > +    targets.
> > +
> > +``UCID`` (u32) - ``ucode_id``
> > +    Microcode ID used together with the engine ID to query
> > +    hardware signature fuse registers.
> > +
> > +``A0CO`` (u32) - ``app0_code_offset``
> > +    App0 code offset -- start of the secure code region within
> > +    the firmware image. Used as the IMEM secure section source.
> > +
> > +``A0CS`` (u32) - ``app0_code_size``
> > +    App0 code size in bytes.
> > +
> > +``NSIG`` (u32) - ``num_sigs``
> > +    Number of signatures included in the ``SIGS`` tag. A value
> > +    of 0 indicates unsigned firmware.
> > +
> > +``SIGS`` (bytes)
> > +    Concatenated array of firmware signatures. The size of
> > +    each signature is the total length of the ``SIGS`` value
> > +    divided by ``NSIG``. The correct signature is selected
> > +    using the fuse-version-derived index.
> > +
> > +Generic Bootloader Tags (FWSEC)
> > +===============================
> > +Used by the generic bootloader (``gen_bootloader.bin``) that
> > +loads the FWSEC firmware via DMA on Turing GPUs. Also includes
> > +the common ``VERS`` and ``BLOB`` tags.
> 
> The common tags are not explicitly mentioned for GSP and Booter - I
> think we can remove that last sentence.

Ok

> 
> > +
> > +``CDSZ`` (u32) - ``code_size``
> > +    Size in bytes of the bootloader code to copy from the
> > +    ``BLOB`` tag and PIO-load into falcon IMEM.
> > +
> > +``STRT`` (u32) - ``start_tag``
> > +    Start tag identifying the IMEM block where execution begins.
> > +    The falcon boot address is derived as ``start_tag << 8``.
> > +
> > +RISC-V Firmware Tags (GSP Bootloader)
> > +======================================
> > +Used by RISC-V-based firmwares such as the GSP bootloader
> > +(``gsp_bootloader.bin``). Also includes the common ``VERS``
> > +and ``BLOB`` tags.
> 
> Same here.
> 
> > +
> > +``CDOF`` (u32) - ``code_offset``
> > +    Offset within the firmware image at which the code section
> > +    starts.
> > +
> > +``DAOF`` (u32) - ``data_offset``
> > +    Offset within the firmware image at which the data section
> > +    starts.
> > +
> > +``MFOF`` (u32) - ``manifest_offset``
> > +    Offset within the firmware image at which the manifest
> > +    starts.
> > +
> > +``APPV`` (u32) - ``app_version``
> > +    Application version of the firmware.
> > diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
> > index 2749c196416d..888a426b7b41 100644
> > --- a/drivers/gpu/nova-core/firmware.rs
> > +++ b/drivers/gpu/nova-core/firmware.rs
> 
> Since the code for `Tlv` seems to be nicely self-contained, can you put
> it into a sub-module (`firmware/tlv.rs`)? This will reduce the amount of
> stuff in `firmware.rs` too.

Ok.

I was also thinking of merging request_tlv() into the Tlv::new() constructor.  Do you think
that's a good idea?  We always precede Tlv::new() with request_tlv() anyway.  

> > @@ -11,7 +11,7 @@
> >      device,
> >      firmware,
> >      prelude::*,
> > -    str::CString,
> > +    str::{CStr, CString},
> >      transmute::FromBytes, //
> >  };
> >  
> > @@ -684,3 +684,240 @@ pub(super) fn elf_section<'a>(elf: &'a [u8], name: &str) -> Option<&'a
> > [u8]> {
> >          }
> >      }
> >  }
> > +
> > +pub(crate) struct TlvBlock<'a> {
> 
> `TlvBlock` and its members can be private IIUC.

Ok

> 
> > +    pub(crate) tag: &'a str,
> 
> This should probably be a `&'a [u8; 4]`? `str` is variable length and
> technically UTF-8, using a 4-bytes array is the more accurate type.
> Doing this also simplifies `parse` a bit.
> 
> > +    pub(crate) value: &'a [u8],
> > +}
> > +
> > +/// On-wire TLV block header: 4-byte ASCII tag + little-endian payload length (bytes,
> > excluding
> > +/// padding to a 4-byte boundary).
> > +struct TlvBlockHeader<'a> {
> > +    tag: &'a str,
> 
> Same here.

I made these as strings to simplify the iterator so that it can just compare strings, but I will
take another look at the code.

> > +    length: usize,
> > +}
> > +
> > +impl<'a> TlvBlockHeader<'a> {
> > +    const SIZE: usize = size_of::<[u8; 4]>() + size_of::<u32>();
> > +
> > +    /// Parses the first [`Self::SIZE`] bytes of `hdr` (caller may pass a longer slice).
> > +    fn parse(hdr: &'a [u8]) -> Option<Self> {
> > +        let hdr = hdr.get(..Self::SIZE)?;
> > +        let tag_bytes = hdr.get(..4)?;
> > +        let tag = core::str::from_utf8(tag_bytes).ok()?;
> > +        if !tag.is_ascii() {
> > +            return None;
> > +        }
> > +        let len_arr = <[u8; 4]>::try_from(hdr.get(4..Self::SIZE)?).ok()?;
> > +        let length = u32::from_le_bytes(len_arr) as usize;
> > +        Some(Self { tag, length })
> > +    }
> > +}
> > +
> > +/// Sequential scan over [`Tlv`]. Unlike [`Tlv`], this type carries a cursor (`pos`) into
> > +/// the parent blob.  It is not interchangeable with a fresh [`Tlv`] view of the same
> > +/// bytes.
> 
> The last two sentences are basically implementation details - are they
> needed in a public doc?

Uh, I think I wrote that to help me keep track of the code, but now I'm not quite sure what it
means.  I'll take another look, and probably delete it.

> > +///
> > +/// # Invariants
> > +///
> > +/// `pos` is a byte offset into `tlv.data` that always lies on a block boundary (in the
> > sense
> > +/// of the [`Tlv`] invariant): it is either the start of a well-formed block, or equal to
> > +/// `tlv.data.len()` (end of iteration).
> > +struct TlvIter<'tlv, 'a> {
> > +    tlv: &'tlv Tlv<'a>,
> > +    pos: usize,
> > +}
> > +
> > +impl<'tlv, 'a> Iterator for TlvIter<'tlv, 'a> {
> > +    type Item = TlvBlock<'a>;
> > +
> > +    /// Returns the block starting at `self.pos` and advances the cursor past it, or
> > [`None`]
> > +    /// once the cursor reaches the end of the data.
> > +    ///
> > +    /// The body relies on a number of `unsafe` operations, because [`Iterator::next`] can
> > +    /// only return [`Option`], not [`Result`], so there is no way to report a parse error
> > +    /// from here.  That is why the entire TLV stream is validated up front in the
> > +    /// constructor.  By this type's invariants, `self.pos` is always on a block boundary
> > +    /// into already-validated data, so each block is known to be well-formed, and
> > +    /// therefore the unchecked operations cannot fail.
> 
> Second paragraph also leaks implementation details and is not relevant
> to users.

Ok.  I like to add these kinds of comments, because I figure that if it was hard for me to
figure out, then others would probably appreciate it.  I wish the Nouveau code had been
documented like that.

> > +    fn next(&mut self) -> Option<Self::Item> {
> > +        if self.pos >= self.tlv.data.len() {
> > +            return None;
> > +        }
> > +
> > +        let tail = &self.tlv.data[self.pos..];
> > +
> > +        // SAFETY: `self.pos` is on a block boundary (`TlvIter` invariant) and the check
> > +        // above gives `self.pos < data.len()`, so by the `Tlv` invariant a complete
> > +        // well-formed block starts at `tail`.
> > +        let hdr = unsafe { tail.get_unchecked(..TlvBlockHeader::SIZE) };
> > +
> > +        // SAFETY: same header bytes as validated in `Tlv::new` for this offset.
> > +        let header = unsafe { TlvBlockHeader::parse(hdr).unwrap_unchecked() };
> > +
> > +        let stored_size = header.length.next_multiple_of(4);
> > +        let advance = TlvBlockHeader::SIZE + stored_size;
> > +        let payload_end = TlvBlockHeader::SIZE + header.length;
> > +
> > +        // SAFETY: `advance` and `payload_end` are exactly the stored and logical payload
> > extents
> > +        // `Tlv::new` accepted for this block.
> > +        let value = unsafe {
> > +            let block = tail.get_unchecked(..advance);
> > +            block.get_unchecked(TlvBlockHeader::SIZE..payload_end)
> > +        };
> 
> As Danilo mentioned we should get rid of these `unsafe` blocks. I think
> returning `None` if an error is met is fine: an error is, technically,
> the end of the iteration; and we have validated the whole file in the
> constructor, so they won't happen anyway.

Ok.  I really don't like conflating Option with Result, though.  I figured in this case, it
would be better to eliminate any validation code that will never execute, but no one seems to
mind so I'll change it.  I'll just add a comment that technically this iterator will return None
on errors that won't never actually occur because the TLV is validated.

> > +        // INVARIANT: by the `Tlv` invariant the block at `self.pos` occupies exactly
> > `advance`
> > +        // bytes, so `self.pos + advance` is the next block boundary (or `data.len()`).
> > +        self.pos += advance;
> > +
> > +        Some(TlvBlock {
> > +            tag: header.tag,
> > +            value,
> > +        })
> > +    }
> > +}
> > +
> > +/// The payload of a validated TLV (type, length, value) firmware image.
> > +///
> > +/// TLV firmware images start with a 4-byte "NVFW" magic header, followed by a sequence of
> > +/// blocks. Each block has a 4-byte type tag, a 4-byte length field, and a data payload
> > +/// whose stored size is the length rounded up to the nearest multiple of 4.
> > +///
> > +/// [`Self::new`] checks the magic header and walks every block: tags must be ASCII,
> > +/// lengths and padding must fit without overflow, and the byte stream after `NVFW` must
> > +/// be exactly partitionable into blocks (no trailing partial header or slack). After
> > +/// that, [`TlvIter`] only signals end-of-stream via [`None`], not parse failure.
> > +///
> > +/// # Invariants
> > +///
> > +/// `data` is a validated TLV payload (the bytes *after* the `NVFW` magic): it is the exact
> > +/// concatenation of zero or more well-formed blocks, with no trailing partial header or
> > slack.
> > +/// Consequently, any offset `o` into `data` that is a block boundary and satisfies
> > +/// `o < data.len()` is the start of a complete block whose header parses and whose stored
> > +/// extent (`TlvBlockHeader::SIZE + header.length.next_multiple_of(4)` bytes) lies within
> > +/// `data`. `data.len()` is itself a boundary.
> > +#[allow(dead_code)]
> > +pub(crate) struct Tlv<'a> {
> > +    data: &'a [u8],
> > +}
> > +
> > +#[allow(dead_code)]
> > +impl<'a> Tlv<'a> {
> > +    const MAGIC: &'static [u8; 4] = b"NVFW";
> > +
> > +    /// Parses `data` as a TLV firmware image, returning [`EINVAL`] if the image is
> > malformed.
> > +    pub(crate) fn new(data: &'a [u8]) -> Result<Self> {
> > +        // Verify that the magic bytes exist and are the correct value
> > +        let magic_len = Self::MAGIC.len();
> > +        if data
> > +            .get(..magic_len)
> > +            .is_none_or(|magic| magic != Self::MAGIC)
> > +        {
> > +            return Err(EINVAL);
> > +        }
> > +
> > +        // The payload is the contiguous sequence of TLV blocks after the magic.
> > +        let payload = data.get(magic_len..).ok_or(EINVAL)?;
> > +
> > +        let mut pos = 0usize;
> 
> You can work directly with `payload` and avoid storing the position in
> its own variable. Just make `payload`'s declaration above `mut`, and
> then:
> 
> > +        while pos < payload.len() {
> 
>     while !payload.is_empty() {
> 

Ok.  I thought it would be safer to make an integer mut instead of a slice.

> > +            // Get the next TLV block.
> > +            let Some(rest) = payload.get(pos..) else {
> > +                return Err(EINVAL);
> > +            };
> 
> This can disappear.

Ok

> 
> > +            // Validate and extract the header (type, length).
> > +            let Some(header) = rest
> 
> s/rest/payload

Only if I make payload mut as you suggest, right?

> 
> > +                .get(..TlvBlockHeader::SIZE)
> > +                .and_then(TlvBlockHeader::parse)
> > +            else {
> > +                return Err(EINVAL);
> > +            };
> > +            // The `length` field of a TLV block contains the actual byte length of the
> > +            // value, but each TLV block is aligned to a 4-byte boundary.
> > +            let Some(stored_size) = header.length.checked_next_multiple_of(4) else {
> > +                return Err(EINVAL);
> > +            };
> > +            let end = pos
> > +                .checked_add(TlvBlockHeader::SIZE)
> > +                .and_then(|p| p.checked_add(stored_size))
> > +                .ok_or(EINVAL)?;
> 
> Here you don't need to add `pos` anymore.
> 
> > +            if end > payload.len() {
> > +                return Err(EINVAL);
> > +            }
> > +            pos = end;
> 
> This becomes:
> 
>     payload = payload.split_at_checked(end).ok_or(EINVAL)?.1;

Ok


> 
> > +        }
> > +
> > +        // INVARIANT: the loop above walked `payload` from offset 0 and only stopped once
> > `pos`
> > +        // reached `payload.len()` exactly, rejecting any block whose header failed to
> > parse or
> > +        // whose stored extent overran. So `payload` is an exact concatenation of well-
> > formed
> > +        // blocks with no partial header or trailing slack.
> > +        Ok(Self { data: payload })
> > +    }
> > +
> > +    fn iter(&self) -> TlvIter<'_, 'a> {
> > +        // INVARIANT: 0 is a block boundary, either the start of the first block,
> > +        // or `data.len()` when `data` is empty.
> > +        TlvIter { tlv: self, pos: 0 }
> > +    }
> > +
> > +    pub(crate) fn len(&self, tag: &str) -> Result<usize> {
> 
> This method name usually means something different in Rust - it should
> be documented (all methods should be, ideally), or even better renamed
> to e.g. `tag_len`.
> 
> But maybe we can also just remove it, since it can be expressed as
> `get_bytes(tag).len()`, and is only ever used in one place...

Ok

> 
> > +        let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;
> > +
> > +        Ok(tlv.value.len())
> > +    }
> > +
> > +    pub(crate) fn get_bytes(&self, tag: &str) -> Result<&'a [u8]> {
> 
> For these getter methods as well, `tag` can be `&[u8; 4]`. Callers can
> build a literal using the `b` prefix, e.g.
> 
>     let fmc_image_data = tlv.get_bytes(b"BLOB")?;

Ah ok.

> 
> > +        let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;
> > +
> > +        Ok(tlv.value)
> 
> nit: can be collapsed into 
> 
>    self.iter().find(|b| b.tag == tag).map(|b| b.value).ok_or(EINVAL)

I mean, yes it can be, but I don't see how that's an improvement.  That first line

	let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;

exists in each getter, for consistency.  So I can only "collapse it" in get_bytes, not in any of
the others.



> 
> > +    }
> > +
> > +    pub(crate) fn get_u32(&self, tag: &str) -> Result<u32> {
> > +        let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;
> 
> You can reuse `get_bytes` to avoid repeating this snippet (also applies
> to the other `get_` methods).

I would rather have a private "find_tlv" method, so that all the getters are independent.

> > +
> > +        tlv.value
> > +            .try_into()
> > +            .ok()
> > +            .map(u32::from_le_bytes)
> > +            .ok_or(EINVAL)
> > +    }
> > +
> > +    pub(crate) fn get_string(&self, tag: &str) -> Result<&'a str> {
> > +        let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;
> > +
> > +        // Handle the possibility that the value is null-terminated.
> > +        let bytes = match CStr::from_bytes_until_nul(tlv.value) {
> > +            Ok(cstr) => cstr.to_bytes(),
> > +            Err(_) => tlv.value,
> > +        };
> 
> The spec says that strings are expected to be ASCII without a NULL
> terminator - and the NULL terminator does not carry any meaning in Rust.
> So I don't think we should be doing that check.

Ok.  I thought I was being crafty here.


> > +
> > +        // But do require it to be all ASCII.
> > +        if !bytes.is_ascii() {
> > +            return Err(EINVAL);
> > +        }
> 
> Btw, is it bad if we allow strings to be UTF-8?

Depends on your definition of "bad" I guess.  printk doesn't have any real UTF-8 support, it
just kinda works if you're careful.  I added this check because string tags are meant to be
things like file names and version strings, and those things really should not be UTF-8.  Multi-
byte characters cause all sorts of problems.  We're not using TLV to store people's names.

> > +
> > +        core::str::from_utf8(bytes).map_err(|_| EINVAL)
> > +    }
> > +
> > +    /// Returns the `n`-th fixed-size chunk from the value of the TLV entry matching
> > +    /// `tag`.
> > +    ///
> > +    /// The value is treated as an array of contiguous, equally-sized chunks, each
> > +    /// `sig_size` bytes long. `sig_size` is the size of one signature.  The returned
> > +    /// slice is the `n`-th chunk.
> > +    ///
> > +    /// Returns [`EINVAL`] if no entry matches `tag`, if computing the chunk's offset
> > +    /// overflows, or if the value is too short to contain the requested chunk.
> 
> This documentation is confusing - it start by mentioning chunks, then
> suddenly signature vocabulary is introduced.

Hmmm I'm not sure what happened there, but I'll fix it.

> 
> > +    pub(crate) fn get_nth_chunk(&self, tag: &str, sig_size: usize, n: usize) -> Result<&'a
> > [u8]> {
> > +        let sigs = self
> > +            .iter()
> > +            .find(|b| b.tag == tag)
> > +            .map(|b| b.value)
> > +            .ok_or(EINVAL)?;
> > +
> > +        let start = sig_size.checked_mul(n).ok_or(EINVAL)?;
> > +        let end = start.checked_add(sig_size).ok_or(EINVAL)?;
> > +
> > +        sigs.get(start..end).ok_or(EINVAL)
> 
> This method is single-use Booter-domain logic, and feels a bit too
> specialized for a `Tlv` API. The caller in `booter.rs` could do:
> 
>     tlv.get_bytes(b"SIGS")?
>         .chunks_exact(sig_size)
>         .nth(index as usize)
>         .ok_or(EINVAL)?;

Ok.  When I wrote this, I didn't think there was only one user.  Plus, this lets me get rid of
the awkward documentation.

Re: [PATCH 3/8] gpu: nova-core: add TLV parser for firmware files

[Alexandre Courbot]

On Sat Jun 13, 2026 at 3:04 AM JST, Timur Tabi wrote:
<...>
>> > +
>> > +``BLOB`` (bytes)
>> > +    If the firmware microcode binary is stored in the TLV, this tag contains
>> > +    the actual firmware image bytes.
>> > +
>> > +``FILE`` (string)
>> > +    If the firmware binary is stored as a separate file, this tag contains the
>> > +    name of that file, assumed to be in the same directory as the TLV.
>> > +    This tag is always paired with ``SIZE``, so as to allow the driver to
>> > +    pre-allocate the buffer before loading the file.
>> 
>> Note that `request_firmware` outrights reject paths that contain `..`.
>> These should probably be excluded from the allowed paths here as well.
>
> Ok.
>
> I guess that kills the idea of avoiding symlinks.

Pretty much. We could technically mangle the paths ourselves in the
kernel, but that's likely to be frowned upon.

>
>> We also need to specify what happens if we have a `FILE` tag but no
>> `SIZE` or vice-versa (presumably, this should be an error?).
>
> Technically, it's the caller that decides what to do in each case.  There is no central "tag
> checker" that verifies any of this.  I felt that this document should be limited to describing
> how the tags work if you see one, rather than describing how to actually build a "proper" TLV
> file.  No one is expected to build the TLVs outside of the extract_firmware_nova.py script.

Since this is a file format specification, I'd argue it should on the
contrary try to pin down what is a valid TLV file and what is not.
Essentially so we can rely on it to make changes that have
backward-compatibility implications in the future.

>
> So to answer your question, if there's no SIZE tag, then it will need to use request_firmware()
> instead of request_firmware_into_buf().  I can add language that says that the SIZE tag should
> be created if it's at all possible to know the size when the TLV is built.

Yes, I think that would remove all ambiguity.

>
> I don't really have a strong opinion either way.
>
>> Another thing I think we should also formally specify: how are duplicate
>> tags handled?
>
> Well, currently the code will ignore the second copy of the tag, since the iterator always
> starts from the beginning.  I'll add some text that says duplicates are not allowed, but if they
> exist, only the first tag (in offset order) is used, and the others are ignored.

Sounds good to me.

>
>
>> 
>> > +
>> > +GSP Firmware Tags
>> > +=================
>> > +Used when loading the GSP (GPU System Processor) firmware
>> > +(``gsp.bin``).
>> 
>> Do you mean `gsp.tlv`? (same for other files).
>
> Hmmm.... I don't think so.  But it's obviously so unclear I should remove it.  I think the point
> I was trying to make is that this is the TLV that goes with gsp.bin.

I guess I was confused because `gsp.tlv` could technically point to a
different file through its `FILE` tag. The real anchor for the driver is
`gsp.tlv`.

<...>
>> > diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
>> > index 2749c196416d..888a426b7b41 100644
>> > --- a/drivers/gpu/nova-core/firmware.rs
>> > +++ b/drivers/gpu/nova-core/firmware.rs
>> 
>> Since the code for `Tlv` seems to be nicely self-contained, can you put
>> it into a sub-module (`firmware/tlv.rs`)? This will reduce the amount of
>> stuff in `firmware.rs` too.
>
> Ok.
>
> I was also thinking of merging request_tlv() into the Tlv::new() constructor.  Do you think
> that's a good idea?  We always precede Tlv::new() with request_tlv() anyway.  

I like the `request` wording because it implies more strongly that we
are loading something. Maybe a `Tlv::request` constructor?

Having `Tlv::new` take an array of bytes looks natural and separates
parsing from loading. It also enables things like nested TLV files
should we ever need that.

>
>> > @@ -11,7 +11,7 @@
>> >      device,
>> >      firmware,
>> >      prelude::*,
>> > -    str::CString,
>> > +    str::{CStr, CString},
>> >      transmute::FromBytes, //
>> >  };
>> >  
>> > @@ -684,3 +684,240 @@ pub(super) fn elf_section<'a>(elf: &'a [u8], name: &str) -> Option<&'a
>> > [u8]> {
>> >          }
>> >      }
>> >  }
>> > +
>> > +pub(crate) struct TlvBlock<'a> {
>> 
>> `TlvBlock` and its members can be private IIUC.
>
> Ok
>
>> 
>> > +    pub(crate) tag: &'a str,
>> 
>> This should probably be a `&'a [u8; 4]`? `str` is variable length and
>> technically UTF-8, using a 4-bytes array is the more accurate type.
>> Doing this also simplifies `parse` a bit.
>> 
>> > +    pub(crate) value: &'a [u8],
>> > +}
>> > +
>> > +/// On-wire TLV block header: 4-byte ASCII tag + little-endian payload length (bytes,
>> > excluding
>> > +/// padding to a 4-byte boundary).
>> > +struct TlvBlockHeader<'a> {
>> > +    tag: &'a str,
>> 
>> Same here.
>
> I made these as strings to simplify the iterator so that it can just compare strings, but I will
> take another look at the code.

Comparison of `[u8; 4]`s should be equally trivial to perform.

<...>
>> > +///
>> > +/// # Invariants
>> > +///
>> > +/// `pos` is a byte offset into `tlv.data` that always lies on a block boundary (in the
>> > sense
>> > +/// of the [`Tlv`] invariant): it is either the start of a well-formed block, or equal to
>> > +/// `tlv.data.len()` (end of iteration).
>> > +struct TlvIter<'tlv, 'a> {
>> > +    tlv: &'tlv Tlv<'a>,
>> > +    pos: usize,
>> > +}
>> > +
>> > +impl<'tlv, 'a> Iterator for TlvIter<'tlv, 'a> {
>> > +    type Item = TlvBlock<'a>;
>> > +
>> > +    /// Returns the block starting at `self.pos` and advances the cursor past it, or
>> > [`None`]
>> > +    /// once the cursor reaches the end of the data.
>> > +    ///
>> > +    /// The body relies on a number of `unsafe` operations, because [`Iterator::next`] can
>> > +    /// only return [`Option`], not [`Result`], so there is no way to report a parse error
>> > +    /// from here.  That is why the entire TLV stream is validated up front in the
>> > +    /// constructor.  By this type's invariants, `self.pos` is always on a block boundary
>> > +    /// into already-validated data, so each block is known to be well-formed, and
>> > +    /// therefore the unchecked operations cannot fail.
>> 
>> Second paragraph also leaks implementation details and is not relevant
>> to users.
>
> Ok.  I like to add these kinds of comments, because I figure that if it was hard for me to
> figure out, then others would probably appreciate it.  I wish the Nouveau code had been
> documented like that.

It's fine to turn these into regular code comments if you think they are
useful (although in this case it looks like we are heading towards
removing the unsafe statements anyway). It's just that they don't bring
information that users will find directly useful and would justify them
being doccomments.

>
>> > +    fn next(&mut self) -> Option<Self::Item> {
>> > +        if self.pos >= self.tlv.data.len() {
>> > +            return None;
>> > +        }
>> > +
>> > +        let tail = &self.tlv.data[self.pos..];
>> > +
>> > +        // SAFETY: `self.pos` is on a block boundary (`TlvIter` invariant) and the check
>> > +        // above gives `self.pos < data.len()`, so by the `Tlv` invariant a complete
>> > +        // well-formed block starts at `tail`.
>> > +        let hdr = unsafe { tail.get_unchecked(..TlvBlockHeader::SIZE) };
>> > +
>> > +        // SAFETY: same header bytes as validated in `Tlv::new` for this offset.
>> > +        let header = unsafe { TlvBlockHeader::parse(hdr).unwrap_unchecked() };
>> > +
>> > +        let stored_size = header.length.next_multiple_of(4);
>> > +        let advance = TlvBlockHeader::SIZE + stored_size;
>> > +        let payload_end = TlvBlockHeader::SIZE + header.length;
>> > +
>> > +        // SAFETY: `advance` and `payload_end` are exactly the stored and logical payload
>> > extents
>> > +        // `Tlv::new` accepted for this block.
>> > +        let value = unsafe {
>> > +            let block = tail.get_unchecked(..advance);
>> > +            block.get_unchecked(TlvBlockHeader::SIZE..payload_end)
>> > +        };
>> 
>> As Danilo mentioned we should get rid of these `unsafe` blocks. I think
>> returning `None` if an error is met is fine: an error is, technically,
>> the end of the iteration; and we have validated the whole file in the
>> constructor, so they won't happen anyway.
>
> Ok.  I really don't like conflating Option with Result, though.  I figured in this case, it
> would be better to eliminate any validation code that will never execute, but no one seems to
> mind so I'll change it.  I'll just add a comment that technically this iterator will return None
> on errors that won't never actually occur because the TLV is validated.

Yeah, it's a tradeoff really. Ideally we would be able to represent the
validation done by the constructor using the type system and rely on
that to make the iterator safe and infallible. Here it looks like we
cannot, so the options are doing redundant error handling, panicking, or
unsafe code.

Unsafe code looks out of place in a parser that is not in any critical
path. I am not a fan of panicking either as it adds a landmine just
because nobody is ever supposed to step there and makes it harder to
prove that the code is indeed safe. This leaves error handling as the
least bad of the 3 options.

But since meeting these errors would technically be a bug in the code,
we can at least log them using `pr_err!` so they don't go unnoticed.

<...>
>> > +#[allow(dead_code)]
>> > +impl<'a> Tlv<'a> {
>> > +    const MAGIC: &'static [u8; 4] = b"NVFW";
>> > +
>> > +    /// Parses `data` as a TLV firmware image, returning [`EINVAL`] if the image is
>> > malformed.
>> > +    pub(crate) fn new(data: &'a [u8]) -> Result<Self> {
>> > +        // Verify that the magic bytes exist and are the correct value
>> > +        let magic_len = Self::MAGIC.len();
>> > +        if data
>> > +            .get(..magic_len)
>> > +            .is_none_or(|magic| magic != Self::MAGIC)
>> > +        {
>> > +            return Err(EINVAL);
>> > +        }
>> > +
>> > +        // The payload is the contiguous sequence of TLV blocks after the magic.
>> > +        let payload = data.get(magic_len..).ok_or(EINVAL)?;
>> > +
>> > +        let mut pos = 0usize;
>> 
>> You can work directly with `payload` and avoid storing the position in
>> its own variable. Just make `payload`'s declaration above `mut`, and
>> then:
>> 
>> > +        while pos < payload.len() {
>> 
>>     while !payload.is_empty() {
>> 
>
> Ok.  I thought it would be safer to make an integer mut instead of a slice.

The slice should be the safer option as it has less state to keep track
of.

Note that the `mut` keyword applies to the slice itself, not what it
points to - you still cannot modify the content of `payload`, you can
just assign a different slice to the `payload` variable.

>
>> > +            // Get the next TLV block.
>> > +            let Some(rest) = payload.get(pos..) else {
>> > +                return Err(EINVAL);
>> > +            };
>> 
>> This can disappear.
>
> Ok
>
>> 
>> > +            // Validate and extract the header (type, length).
>> > +            let Some(header) = rest
>> 
>> s/rest/payload
>
> Only if I make payload mut as you suggest, right?

Oh yes these comments are still about dropping `pos`.

<...>
>> > +        let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;
>> > +
>> > +        Ok(tlv.value)
>> 
>> nit: can be collapsed into 
>> 
>>    self.iter().find(|b| b.tag == tag).map(|b| b.value).ok_or(EINVAL)
>
> I mean, yes it can be, but I don't see how that's an improvement.  That first line
>
> 	let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;
>
> exists in each getter, for consistency.  So I can only "collapse it" in get_bytes, not in any of
> the others.

You can also chain more `map` and other operators. This gives a
functional vibe to the method which feels slightly more idiomatic in
Rust, but it is not critical if you prefer to keep it procedural. Both
read fine for something that short.

>
>
>
>> 
>> > +    }
>> > +
>> > +    pub(crate) fn get_u32(&self, tag: &str) -> Result<u32> {
>> > +        let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;
>> 
>> You can reuse `get_bytes` to avoid repeating this snippet (also applies
>> to the other `get_` methods).
>
> I would rather have a private "find_tlv" method, so that all the getters are independent.

That would work as well.

>
>> > +
>> > +        tlv.value
>> > +            .try_into()
>> > +            .ok()
>> > +            .map(u32::from_le_bytes)
>> > +            .ok_or(EINVAL)
>> > +    }
>> > +
>> > +    pub(crate) fn get_string(&self, tag: &str) -> Result<&'a str> {
>> > +        let tlv = self.iter().find(|b| b.tag == tag).ok_or(EINVAL)?;
>> > +
>> > +        // Handle the possibility that the value is null-terminated.
>> > +        let bytes = match CStr::from_bytes_until_nul(tlv.value) {
>> > +            Ok(cstr) => cstr.to_bytes(),
>> > +            Err(_) => tlv.value,
>> > +        };
>> 
>> The spec says that strings are expected to be ASCII without a NULL
>> terminator - and the NULL terminator does not carry any meaning in Rust.
>> So I don't think we should be doing that check.
>
> Ok.  I thought I was being crafty here.
>
>
>> > +
>> > +        // But do require it to be all ASCII.
>> > +        if !bytes.is_ascii() {
>> > +            return Err(EINVAL);
>> > +        }
>> 
>> Btw, is it bad if we allow strings to be UTF-8?
>
> Depends on your definition of "bad" I guess.  printk doesn't have any real UTF-8 support, it
> just kinda works if you're careful.  I added this check because string tags are meant to be
> things like file names and version strings, and those things really should not be UTF-8.  Multi-
> byte characters cause all sorts of problems.  We're not using TLV to store people's names.

Mmm indeed there is probably no real benefit here.

Re: [PATCH 3/8] gpu: nova-core: add TLV parser for firmware files

[John Hubbard]

On 6/12/26 7:46 AM, Alexandre Courbot wrote:
> On Thu Jun 11, 2026 at 2:49 AM JST, Timur Tabi wrote:
...
> The spec says that strings are expected to be ASCII without a NULL
> terminator - and the NULL terminator does not carry any meaning in Rust.
> So I don't think we should be doing that check.
> 
>> +
>> +        // But do require it to be all ASCII.
>> +        if !bytes.is_ascii() {
>> +            return Err(EINVAL);
>> +        }
> 
> Btw, is it bad if we allow strings to be UTF-8?

Yes, it is bad. :) This is a tightly constrained file format,
rather than a general purpose filesystem. As such, sharply
constraining it makes the code simpler and easier to confirm
correct, whether via reading the code on the screen, or
setting up test coverage.

thanks,
-- 
John Hubbard