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

["Danilo Krummrich" <dakr@kernel.org>]

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