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

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

On Mon Aug 3, 2026 at 11:36 PM CEST, Danilo Krummrich wrote:
> @Miguel: One consideration for you below.
>
> On Fri Jul 31, 2026 at 10:10 PM CEST, Timur Tabi wrote:
>> +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(ENODATA);
>> +        }
>
> This seems redundant, if payload is empty the below loop wouldn't execute and
> the !has_vers check would return EINVAL. Which seems consistent, since empty
> data is kinda invalid.
>
>> +
>> +        // The spec says every TLV must have a VERS tag.
>> +        let mut has_vers = false;
>> +
>> +        let mut rest = payload;
>> +        while !rest.is_empty() {
>> +            // Validate and extract the header (type, length).
>> +            let Some(header): Option<TlvBlockHeader> = rest
>> +                .get(..TlvBlockHeader::SIZE)
>> +                .and_then(TlvBlockHeader::parse)
>> +            else {
>> +                return Err(EINVAL);
>> +            };
>> +
>> +            has_vers |= header.tag == *b"VERS";
>> +
>> +            // 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)?;
>> +
>> +            rest = rest.split_at_checked(length).ok_or(EINVAL)?.1;
>> +        }
>> +
>> +        if !has_vers {
>> +            return Err(EINVAL);
>> +        }
>
> [...]
>
>> +    fn find(&self, tag: &[u8; 4]) -> Result<TlvBlock<'a>> {
>> +        self.iter().find(|b| b.tag == *tag).ok_or(EINVAL)
>> +    }
>
> NIT: Do we really know the tag is invalid just because it wasn't found?
>
>> +    /// Return a slice of bytes.  Returns ENODATA if the value is empty.
>> +    pub(crate) fn get_bytes(&self, tag: &[u8; 4]) -> Result<&'a [u8]> {
>> +        let tlv = self.find(tag)?;
>> +
>> +        // Treat empty value as an error, to avoid trying to parse nothing.
>> +        if tlv.value.is_empty() {
>> +            return Err(ENODATA);
>> +        }
>
> This one seems resonable; but we don't have the error code in place. The commit
> message says the series depends on [1], but this likely goes through the Rust
> tree.
>
> Either we just add this single error code in a separate patch or Miguel provides
> a signed tag for [1].
>
> Since this is a perfectly trivial conflict, I'd go for the former.
>
> Miguel, let me know if you want to provide a signed tag, otherwise it would be
> good if Timur (or myself) could send a patch for ENODATA only.
>
> @Timur: In any case, no need to resend the series for this or the other two nits
> in this reply.
>
> Thanks,
> Danilo

[1] https://lore.kernel.org/all/20260629183022.2709524-1-ttabi@nvidia.com/