Re: [PATCH 03/11] gpu: nova-core: support header parsing on Turing/GA100

[Timur Tabi <ttabi@nvidia.com>]

On Mon, 2025-11-17 at 17:33 -0500, Joel Fernandes wrote:
> >  
> > +        // There are two versions of Booter, one for Turing/GA100, and another for
> > +        // GA102+.  The extraction of the IMEM sections differs between the two
> > +        // versions.  Unfortunately, the file names are the same, and the headers
> > +        // don't indicate the versions.  The only way to differentiate is by the Chipset.
> 
> Some more doc comments and documentation explaining the header structure
> would be great.

The header structure is the same, it's just that on pre-GA102 platforms, some header fields are
used, and on GA102+ platforms other header fields are used.  I can't explain why -- that's just how
the images were built.

I'm not sure what kind of documentation I could add.  The headers are already all documented, and it
just seems somewhat arbitrary how the fields are used.

> > +
> >          Ok(Self {
> > -            imem_sec_load_target: FalconLoadTarget {
> > -                src_start: app0.offset,
> > -                dst_start: 0,
> > -                len: app0.len,
> > +            imem_sec_load_target: if chipset > Chipset::GA100 {
> > +                FalconLoadTarget {
> > +                    src_start: app0.offset,
> > +                    dst_start: 0,
> > +                    len: app0.len,
> > +                }
> > +            } else {
> > +                FalconLoadTarget {
> > +                    src_start: load_hdr.os_code_size,
> > +                    dst_start: app0.offset,
> > +                    len: app0.len,
> > +                }
> 
> Can write more succinctly:
> 
>   imem_sec_load_target: FalconLoadTarget {
>       src_start: match chipset > Chipset::GA100  {
>           true => app0.offset,
>           false => load_hdr.os_code_size,
>       },
>       dst_start: match chipset > Chipset::GA100 {
>           true => 0,
>           false => app0.offset,
>       
>       len: app0.len,                                                                            
>   },

Do we really want to use "match" instead of "if", just because we don't need "else"?

> 
> > +            },
> > +            imem_ns_load_target: if chipset > Chipset::GA100 {
> > +                None
> > +            } else {
> > +                Some(FalconLoadTarget {
> > +                    src_start: 0,
> > +                    dst_start: load_hdr.os_code_offset,
> > +                    len: load_hdr.os_code_size,
> > +                })
> >              },
> > -            // Exists only in the booter image for Turing and GA100
> > -            imem_ns_load_target: None,
> >              dmem_load_target: FalconLoadTarget {
> >                  src_start: load_hdr.os_data_offset,
> >                  dst_start: 0,
> > @@ -393,7 +413,13 @@ fn brom_params(&self) -> FalconBromParams {
> >      }
> >  
> >      fn boot_addr(&self) -> u32 {
> > -        self.imem_sec_load_target.src_start
> > +        if let Some(ns_target) = &self.imem_ns_load_target {
> > +            // Turing and GA100 - use non-secure load target
> > +            ns_target.dst_start
> > +        } else {
> > +            // GA102+ (Ampere) - use secure load target
> 
> s/Ampere/Ampere and later/ ? Also missing period at end of comment, here and
> elsewhere.

Sure, I'll clean those up.  I'll just remove the (Ampere) though.

Please keep in mind that I've been working on these patches on-and-off over several weeks and
through multiple rebases.  There will be a lot of nits.