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

On 7/29/26 9:37 AM, Timur Tabi wrote:
> On Mon, 2026-07-27 at 20:31 +0900, Alexandre Courbot wrote:
...
>>> +``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.

Yes, that is what I'd recommend here, too.

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

++1, this is not ever going to change.

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

Very true. And IMHO it is wildly unlikely that we would end up with any
sort of unsigned booter thing. So it's best to just remove all mention of it.

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

Agree that there is value in enforcing printable tags. We could
something like this if we want to allow spaces:

   bytes.iter().all(|b| b.is_ascii_graphic() || *b == b' ')

...but is there really-truly a need to support spaces? (I haven't checked
on that, but it surprises me at first.)


thanks,
-- 
John Hubbard