Re: [PATCH v3 2/7] gpu: nova-core: add TLV parser for firmware files

On Fri Jul 3, 2026 at 4:27 AM JST, Timur Tabi wrote:
> 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.
>
> Add function request_tlv() to load TLV firmware images.
>
> 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 <[email protected]>
> ---
>  Documentation/gpu/nova/core/tlv.rst   | 182 +++++++++++++++++++++
>  drivers/gpu/nova-core/firmware.rs     |   1 +
>  drivers/gpu/nova-core/firmware/tlv.rs | 225 ++++++++++++++++++++++++++
>  3 files changed, 408 insertions(+)
>  create mode 100644 Documentation/gpu/nova/core/tlv.rst
>  create mode 100644 drivers/gpu/nova-core/firmware/tlv.rs
>
> diff --git a/Documentation/gpu/nova/core/tlv.rst b/Documentation/gpu/nova/core/tlv.rst
> new file mode 100644
> index 000000000000..e4eb6ab6d02f
> --- /dev/null
> +++ b/Documentation/gpu/nova/core/tlv.rst
> @@ -0,0 +1,182 @@
> +.. 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
> +encoded as a little-endian unsigned integer, and a sequence of bytes, the size
> +of which is equal to the length rounded up to the next multiple of 4.
> +
> +The driver code that reads the TLV and uses its contents is called the parser.
> +It is the responsibility of the parser to handle missing or malformed tags,
> +lengths, and values in the TLV.

I guess that last paragraph is kind of expected - spelling it out loud
without being specific as to how to handle missing or malformed tags is
just confusing. I think it can just be dropped?

> +
> +::
> +
> +    +------+------+------+------+
> +    |  '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
> +    +---------------------------+
> +
> +Tags and Length
> +===============
> +TLV tags are always four-character words, with all letters being upper case.
> +Duplicate tags are not allowed.
> +
> +Lengths of zero are allowed and indicate that the tag is a boolean.  That is,
> +presence of the tag indicates ``True`` and absence indicates ``False``.
> +
> +Values
> +======
> +Values are one of three types.  The type is not encoded in the format; rather,
> +the parser expects a given tag to have a value of a given type.
> +
> +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.

"expected" leaves a window of interpretation - let's say "required" or
just "encoded in ASCII without a null terminator".

> +3) An array of bytes, for binary data.
> +
> +Common Tags
> +===========
> +These tags are shared across firmware types and carry the same meaning
> +wherever they appear.  Unlike the firmware-specific tags below, a common tag
> +is reserved: its meaning is fixed and may never be redefined for a particular
> +firmware type.
> +
> +``VERS`` (string)
> +    Human-readable firmware version string, indicates the version of
> +    the firmware.  Present in all TLV files.
> +
> +A TLV image must contain either a single ``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, which is required to be in the same directory as the TLV,
> +    so no paths are allowed in the filename.  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``.  This tag is mandatory if ``FILE`` exists, so the
> +    size of the firmware image must be known when the TLV is created.  If the
> +    firmware image is updated and its size changes, then the TLV must be
> +    updated with it.
> +
> +GSP Firmware Tags
> +=================
> +``SIGN`` (bytes)
> +    Cryptographic signature for the GSP firmware.
> +
> +Booter Firmware Tags
> +====================
> +``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 ``SIGN`` tag.  A value of 0 indicates
> +    unsigned firmware and that there is no ``SIGN`` tag.

Shall we say "A value of 0 or absence of this tag" for coverage? ... Or
not, see my review of patch 3.

> +
> +``SIGN`` (bytes)
> +    Concatenated array of firmware signatures. The size of each signature is
> +    the total length of the ``SIGN`` value divided by ``NSIG``. The correct
> +    signature is selected using the fuse-version-derived index.
> +
> +Generic Bootloader 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``.
> +
> +GSP Bootloader 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.
> +
> +FMC Firmware Tags
> +=================
> +``HASH`` (bytes)
> +    SHA-384 hash of the FMC firmware, exactly 48 bytes long.
> +
> +``PKEY`` (bytes)
> +    Public key used to verify the FMC firmware. At most 384 bytes (RSA-3072),
> +    but may be shorter.
> +
> +``SIGN`` (bytes)
> +    Signature of the FMC firmware. At most 384 bytes (RSA-3072), but may
> +    be shorter.
> \ No newline at end of file
> diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
> index a94820a3b335..c0cd06579643 100644
> --- a/drivers/gpu/nova-core/firmware.rs
> +++ b/drivers/gpu/nova-core/firmware.rs
> @@ -32,6 +32,7 @@
>  pub(crate) mod fwsec;
>  pub(crate) mod gsp;
>  pub(crate) mod riscv;
> +pub(crate) mod tlv;
>  
>  pub(crate) const FIRMWARE_VERSION: &str = "570.144";
>  
> diff --git a/drivers/gpu/nova-core/firmware/tlv.rs b/drivers/gpu/nova-core/firmware/tlv.rs
> new file mode 100644
> index 000000000000..56e0d5cab580
> --- /dev/null
> +++ b/drivers/gpu/nova-core/firmware/tlv.rs
> @@ -0,0 +1,225 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
> +
> +use kernel::{
> +    device,
> +    firmware,
> +    prelude::*,
> +    str::CString, //
> +};
> +
> +use crate::gpu;
> +
> +/// Requests the GPU firmware TLV `name` suitable for `chipset`.
> +#[expect(dead_code)]
> +pub(crate) fn request_tlv(
> +    dev: &device::Device,
> +    chipset: gpu::Chipset,
> +    name: &str,
> +) -> Result<firmware::Firmware> {
> +    let chip_name = chipset.name();
> +
> +    dev_dbg!(
> +        dev,
> +        "loading firmware image {}/gsp/{}.tlv\n",
> +        chip_name,
> +        name
> +    );
> +
> +    CString::try_from_fmt(fmt!("nvidia/{chip_name}/gsp/{name}.tlv"))
> +        .and_then(|path| firmware::Firmware::request(&path, dev))
> +}
> +
> +struct TlvBlock<'a> {
> +    tag: &'a [u8; 4],
> +    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 [u8; 4],

This struct doesn't need a lifetime - it can store `tag` as `[u8; 4]`.
Same for the `tag` member of `TlvBlock`. Everything above them (`find`,
`get_bytes`, etc) can still take a reference to the array for
convenience as we use the `b"ABCD"` syntax, but for these two the
reference is just inefficient without reason.

> +    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 = <&[u8; 4]>::try_from(hdr.get(..4)?).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;

Let's avoid using the potentially lossy `as` keyword:

    let length = num::u32_as_usize(u32::from_le_bytes(len_arr));

(after importing `crate::num`).

> +        Some(Self { tag, length })
> +    }
> +}
> +
> +/// Iterator over the [`TlvBlock`]s of a [`Tlv`].
> +///
> +/// # 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 or encounters an error.
> +    ///
> +    /// Note that errors cannot actually occur because the data is validated in the constructor.
> +    fn next(&mut self) -> Option<Self::Item> {
> +        if self.pos >= self.tlv.data.len() {
> +            return None;
> +        }
> +
> +        let tail = self.tlv.data.get(self.pos..)?;
> +
> +        let hdr = tail.get(..TlvBlockHeader::SIZE)?;
> +        let header = TlvBlockHeader::parse(hdr)?;
> +
> +        let stored_size = header.length.checked_next_multiple_of(4)?;
> +        let advance = TlvBlockHeader::SIZE.checked_add(stored_size)?;
> +        let payload_end = TlvBlockHeader::SIZE.checked_add(header.length)?;
> +
> +        let value = tail
> +            .get(..advance)?
> +            .get(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;

Maybe use `checked_add` here as well for consistency.

> +
> +        Some(TlvBlock {
> +            tag: header.tag,
> +            value,
> +        })
> +    }
> +}
> +
> +/// The payload of a validated TLV (type, length, value) firmware image.

I think we can just say "A validated TLV", as payload makes it sound
like this is the payload of a given key within the TLV firmware.

> +///
> +/// 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.
> +pub(crate) struct Tlv<'a> {
> +    data: &'a [u8],
> +}
> +
> +#[expect(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)?;
> +
> +        if payload.is_empty() {
> +            // Reject empty TLV files
> +            return Err(EINVAL);
> +        }
> +
> +        let mut rest = payload;
> +        while !rest.is_empty() {
> +            // 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 length = TlvBlockHeader::SIZE
> +                .checked_add(stored_size)
> +                .ok_or(EINVAL)?;
> +
> +            if length > rest.len() {
> +                return Err(EINVAL);
> +            }
> +
> +            // Advance to the next block. `length <= rest.len()` was just checked, so this
> +            // slice is always in bounds and lands on the next block boundary (or empties
> +            // `rest` after the final block).
> +            rest = &rest[length..];

The check on length and slice split can be factored out into this
one-liner:

    rest = rest.split_at_checked(length).ok_or(EINVAL)?.1;

It is preferable not only because it is shorter; the guarantee of
correctness is carried by the split operation itself, and as a result
you also don't need a comment justifying that you just performed the
check.

> +        }
> +
> +        Ok(Self { data: payload })

As mentioned by Sashiko, this needs an `INVARIANT:` comment.

> +    }
> +
> +    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 }
> +    }
> +
> +    fn find(&self, tag: &[u8; 4]) -> Result<TlvBlock<'a>> {
> +        self.iter().find(|b| b.tag == tag).ok_or(EINVAL)
> +    }
> +
> +    pub(crate) fn get_bytes(&self, tag: &[u8; 4]) -> Result<&'a [u8]> {

These methods are simple, but let's grant them a one-line doccomment for
consistency.

One thing I'd like to eventually add (for the record; as a follow-up is
ok) is an abstraction of the `BLOB`/`FILE` distinction: maybe a
`get_payload` method that returns either a reference to the `BLOB`
section, or creates a `VVec` to load the references firmware file. Its
return type would be an `enum` with an `AsRef<[u8]>` implementation and
a `to_owned(self) -> VVec<u8>` method so individual firmware
implementations can get the representation they need without caring
about the underlying storage type.