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

On Fri, Nov 14, 2025 at 05:30:36PM -0600, Timur Tabi
wrote:
> The Turing/GA100 version of Booter is slightly different from the
> GA102+ version.  The headers are the same, but different fields of
> the headers are used to identify the IMEM section.  In addition,
> there is an NMEM section on Turing/GA100.
> 
> Signed-off-by: Timur Tabi <ttabi at nvidia.com>
> ---
>  drivers/gpu/nova-core/firmware/booter.rs | 40 +++++++++++++++++++-----
>  1 file changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/firmware/booter.rs
b/drivers/gpu/nova-core/firmware/booter.rs
> index 1b98bb47424c..6ac9593504db 100644
> --- a/drivers/gpu/nova-core/firmware/booter.rs
> +++ b/drivers/gpu/nova-core/firmware/booter.rs
> @@ -356,14 +356,34 @@ pub(crate) fn new(
>              }
>          };
>  
> +        // 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.
> +
>          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,
  },
> +            },
> +            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.

thanks,

 - Joel
> +            self.imem_sec_load_target.src_start
> +        }
>      }
>  }
>  
> -- 
> 2.51.2
>

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.