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

On Thu Jul 9, 2026 at 5:12 AM JST, Timur Tabi wrote:
> On Mon, 2026-07-06 at 15:31 +0900, Alexandre Courbot wrote:
>> > +==================================
>> > +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?
>
> I think the docs need a statement somewhere that "parsers" should handle missing or invalid tags,
> and that that there is no expectation that when a TLV is created, that all the tags are there or
> that they are even the correct format or anything like that.

No issue with keeping that paragraph if you prefer.

>
>> > 
>> > +
>> > +``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.
>
> Hmmm... I'm not really crazy about that idea, because then there is incentive to make all u32 tags
> act that way.  Sometimes 0 is just another number, and not a synonym for "None".

Yup, that would make the absence of `NSIG` an error, but that's not a
problem.

>
>> > 
>> > +
>> > +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.
>
> Ok.  This change resulted in a cascading effect and I had to change a lot of &[u8; 4] into [u8; 4],
> but I didn't want to change all of them.  Which is better:
>
>     pub(crate) fn get_bytes(&self, tag: &[u8; 4]) -> Result<&'a [u8]> {
> ...
>     tlv.get_bytes(b"BLOB")?
>
> or
>
>     pub(crate) fn get_bytes(&self, tag: [u8; 4]) -> Result<&'a [u8]> {
> ...
>     tlv.get_bytes(*b"BLOB")?

The first one is fine - and better arguably, as the callers don't have
to dereference the `b"ABCD"` tags. It should also limit the cascading
effect.

>
>
>> > +
>> > +/// 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.
>
> Well, it's the part after the "NVFW" header.  The "payload" of an actual get is the "value".
>
> I'll change it to:
>
> /// The post-header part of a validated TLV (type, length, value) firmware image.

Sounds good!