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

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.

> > 
> > +
> > +``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".

> > 
> > +
> > +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 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.

> 
> 
> 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.

Yes, I definitely want to do that in the future also.