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

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] =3D 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 =3D Self::MAGIC.len();
>> +        if data
>> +            .get(..magic_len)
>> +            .is_none_or(|magic| magic !=3D Self::MAGIC)
>> +        {
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        // The payload is the contiguous sequence of TLV blocks after t=
he magic.
>> +        let payload =3D 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 em=
pty
> data is kinda invalid.
>
>> +
>> +        // The spec says every TLV must have a VERS tag.
>> +        let mut has_vers =3D false;
>> +
>> +        let mut rest =3D payload;
>> +        while !rest.is_empty() {
>> +            // Validate and extract the header (type, length).
>> +            let Some(header): Option<TlvBlockHeader> =3D rest
>> +                .get(..TlvBlockHeader::SIZE)
>> +                .and_then(TlvBlockHeader::parse)
>> +            else {
>> +                return Err(EINVAL);
>> +            };
>> +
>> +            has_vers |=3D header.tag =3D=3D *b"VERS";
>> +
>> +            // The `length` field of a TLV block contains the actual by=
te length of the
>> +            // value, but each TLV block is aligned to a 4-byte boundar=
y.
>> +            let Some(stored_size) =3D header.length.checked_next_multip=
le_of(4) else {
>> +                return Err(EINVAL);
>> +            };
>> +
>> +            let length =3D TlvBlockHeader::SIZE
>> +                .checked_add(stored_size)
>> +                .ok_or(EINVAL)?;
>> +
>> +            rest =3D 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 =3D=3D *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 =3D self.find(tag)?;
>> +
>> +        // Treat empty value as an error, to avoid trying to parse noth=
ing.
>> +        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 p=
rovides
> 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 wou=
ld 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 t=
wo nits
> in this reply.
>
> Thanks,
> Danilo

[1] https://lore.kernel.org/all/[email protected]/