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

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.

On Tue, Nov 18, 2025 at 12:52:47AM +0000, Timur Tabi wrote:
[...] 
> > > +
> > > ???????? 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"?
I don't care about the if/else as much as I care about the opportunity to
just specify FalconLoadTarget once instead twice. I think the match here is
cleaner for this
snippet, but I am Ok with the if/else as well.

Something like:
   imem_sec_load_target: FalconLoadTarget {
       src_start: if chipset > Chipset::GA100  {
          app0.offset
      } else {
           load_hdr.os_code_size
      },

That would be one more line of code, but pretty much the same.

thanks,

 - Joel