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

["Alexandre Courbot" <acourbot@nvidia.com>]

On Tue Dec 9, 2025 at 8:17 AM JST, 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@nvidia.com>
> ---
>  drivers/gpu/nova-core/firmware/booter.rs | 33 +++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/firmware/booter.rs b/drivers/gpu/nova-core/firmware/booter.rs
> index 1b98bb47424c..7ceea7cc9a87 100644
> --- a/drivers/gpu/nova-core/firmware/booter.rs
> +++ b/drivers/gpu/nova-core/firmware/booter.rs
> @@ -356,14 +356,27 @@ 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.
> +

This comment begs for some code following it, and I notice that below
the same `if chipset > Chipset::GA100` test is repeated twice.

How about doing the following:

  let (imem_dst_start, imem_ns_load_target) = if chipset <= Chipset::GA100 {
      (app0.offset, Some(FalconLoadTarget {
        ...
      }))
  } else {
      (0, None)
  }

... and using the local variables to initialize the result?

That way the code relevant to the comment immediately follows it, you do
the test only once, and the complex initializer below becomes a bit
simpler.

>          Ok(Self {
> -            imem_sec_load_target: FalconLoadTarget {
> -                src_start: app0.offset,
> -                dst_start: 0,
> -                len: app0.len,
> +            imem_sec_load_target:
> +                FalconLoadTarget {
> +                    src_start: app0.offset,
> +                    dst_start: if chipset > Chipset::GA100 { 0 } else { 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 +406,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

This comment is confusing - the logic is clear and doesn't mention the
chipset, so let's remove it. Ideally we can limit these chipset-specific
things to a single place and a single comment.

> +            ns_target.dst_start
> +        } else {
> +            // GA102 and later - use secure load target

Same here, the code is explicit enough and doesn't mention the chipset.

> +            self.imem_sec_load_target.src_start
> +        }
>      }
>  }
>