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

On Mon, 2026-07-27 at 20:31 +0900, Alexandre Courbot wrote:
> 
> > +Tags and Length
> > +===============
> > +TLV tags are always four-character words, with all letters being upper case.
> > +Duplicate tags are not allowed.
> > +
> > +Lengths of zero are allowed and indicate that the tag is a boolean.  That is,
> 
> Let's mention "Payloads of length zero" to be precise what length this
> is about.

I'm going to remove the stuff about booleans.  We don't use them in Nova, and when I tried to
add support for them it was just messy.  Just because a tag is missing, that doesn't mean that
it's false.  It could be just missing, which would be something to check for.

The upcoming nvoc stuff does have a boolean tag, but it's not in the TLV yet.  I don't have a
real ETA for it, so there will be a time period where we support r615 but some tags will appear
later.  So we need to be able to check for a missing tag vs tag==false.

> > +
> > +GSP Firmware Tags
> > +=================
> > +``SIGN`` (bytes)
> > +    Cryptographic signature for the GSP firmware.
> > +
> > +``BLID`` (bytes)
> > +    The build ID, extracted from the ".note.gnu.build-id" section.
> 
> Should this be a string by any chance?

Yes, thanks.

> 
> > +
> > +Booter Firmware Tags
> > +====================
> > 
...
> > +
> > +``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.
> 
> In patch 4 `booter.rs` says "Booter is always signed", which contradicts
> this definition. We had a non-signed path in the original code; why not
> preserve it? The current firmware files do not make use of it, but the
> point of specifying things here is to make the code future-proof in case
> we introduce or enable unsigned firmwares in the future; so let's make
> the code religiously follow the spec.

Well, booter (both load and unload) always is signed.  The script even enforces that.  The
current unsigned code in booter.rs has never been exercised, so we can't really say it's
correct.

I can change the documentation here to not mention NSIG==0 as a possibility.  When I wrote this
text, I hadn't noticed that NSIG is never 0.

We are never going to introduce unsigned booter firmware.  For one thing, Hopper and later have
already replaced booter with fmc, so this code is already legacy.

If it turns out that some future version of GSP-RM does add an unsigned booter, we would need to
change the script and probably do more than just call no_patch_signature().

> > +    /// Return a string value.
> > +    pub(crate) fn get_string(&self, tag: &[u8; 4]) -> Result<&'a str> {
> > +        let tlv = self.find(tag)?;
> > +
> > +        let bytes = tlv.value;
> > +
> > +        // To make sure the value actually is a string, ensure it's all ASCII.
> > +        if !bytes.is_ascii() {
> > +            return Err(EINVAL);
> > +        }
> 
> The spec also says that NULL characters are invalid; we should test for
> `|| bytes.contains(&0)` as well here.

Hmmm... I wonder if I should expand the definition to exclude all characters less than ASCII 32?

I was debating is_ascii_graphic() but that also excludes blank spaces.  There is value in
insisting that the tag is clearly printable.

Thoughts?

>