Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Wed, 17 Dec 2025 15:23:33 +0900
Message-Id: <DF09YET9UFY1.209KXGTYK2485@nvidia.com>
From: "Alexandre Courbot" <acourbot@nvidia.com>
To: "Timur Tabi" <ttabi@nvidia.com>, "Alexandre Courbot"
 <acourbot@nvidia.com>, "Joel Fernandes" <joelagnelf@nvidia.com>, "John
 Hubbard" <jhubbard@nvidia.com>, "Danilo Krummrich" <dakr@kernel.org>,
 "Lyude Paul" <lyude@redhat.com>, <nouveau@lists.freedesktop.org>,
 <rust-for-linux@vger.kernel.org>
Cc: "Nouveau" <nouveau-bounces@lists.freedesktop.org>
Subject: Re: [PATCH v3 03/12] gpu: nova-core: support header parsing on
 Turing/GA100
References: <20251208231801.1786803-1-ttabi@nvidia.com>
 <20251208231801.1786803-4-ttabi@nvidia.com>
In-Reply-To: <20251208231801.1786803-4-ttabi@nvidia.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?Z0tMaVVURnQveW1uVEFOM2xTUG5xMEJsMHNjK3FYRkhzZHBkb1A2bFd4Yzlp?=
 =?utf-8?B?YWxqVFEwL24vODhGNXlMQTlhS0xRLzJ4dU1rQjcxNWNiVkZabDZWYWtHcUhi?=
 =?utf-8?B?RGJOUDN6SWZaR0RDNlhJc2FaY1BsSEJGVzVBdFFjenNqaXB3TSs1SW5xK2xu?=
 =?utf-8?B?UGxLZ0N0SkJzdlVzY1I1YnZYU1pRWFRTUnRodDZmeEtkYmdNYkpocEdxU0Y3?=
 =?utf-8?B?dDdRYktPaFNvY3VWMGJodnhzejJUN2x6cElGNEFmWXl0TThSV2poYjZSRU5H?=
 =?utf-8?B?STdJT2czNnZtOUNoYmgrTngycVdRWFRUT3pHWVVvR3pRQWxNVWpWSzdKNTlN?=
 =?utf-8?B?MXk4SmlqWTNQY08yaHVnam9reGVydGhRWEF5MGtWbHJyNTlOUjVzZ2dFeTM3?=
 =?utf-8?B?MDRiekRLTGFIcGV1K3FVWjJ4SENHRDFJUmNJZmU5ZWJOWjdyRE12eWRvYWZs?=
 =?utf-8?B?N0VZY3dhcjZVcWtXaVd1VGxsUzJGRnlHVGQ0ZG1MT2ZlOFhRNUphNGdLZUYx?=
 =?utf-8?B?aVpIeTNFZHd3Yys0aEVkN2YzeUc0Unc1YmJSUGxsZGtNM2FtUTJCc0VGODZY?=
 =?utf-8?B?WVcwZy9vcFMyUzFMb2RwUTdkVVNuWFpTenRTYXF3M2ZPa1I5MnBESTlOV2Uv?=
 =?utf-8?B?QzdLZ0JsZVFuczB4MExSSmZBdW14cU9tSWExYmJ4cU45a3lZeERPQm5yUjRm?=
 =?utf-8?B?SFcwbjBvcy9qOXBEVktRQ1JxNXdlQTI0UklHYzJkZWdCeW50Um5MLy9ZVDZZ?=
 =?utf-8?B?Z2dZV3NVN3NrTG9zQkdWbHdyV0FzS1lLeU1iTkcvdHRZelREV0ZDa0JNWlJv?=
 =?utf-8?B?R29VcElMV2xoNkNyS0hGVTUrMmkwZmkxL3BNVUlIOGo2d3lCR1BNYkxBWUgx?=
 =?utf-8?B?RjZtWU5ycXZyeTNJRmRSd1FjVFp6US9SRkcrVGJ3V1pUTy9MakxKcUZScFdj?=
 =?utf-8?B?VXFOU1c3QmpQd05HSzVLKy9HN1JMQkx3N0thYjEzdXJ3bHZYUmROSTZQZzJr?=
 =?utf-8?B?cXNZNC82UllVTjBvU3ZuQmdTQnVTWTdMWjN6bHJGNHd0eHZvSGZsbXM0QXNL?=
 =?utf-8?B?TFFpYXdOaWxndW82cG83L3ZYNmJINXM4NWl3ZVlGYXNBUzNYSjVORS9mTEZS?=
 =?utf-8?B?Wm9aR2NPakdvY3FHYkt4ZDVjRDhDRjhVRTIxRFdDUXI1WGRTTnBBRkZGRkZo?=
 =?utf-8?B?TjZGMlBlemEwV3hibFpzMnJpeERHaHdKaTJyakpVdmE2dkU3VXRxQUYzeDBa?=
 =?utf-8?B?VzFTMlVxdkdaVTJuSHJQclkzbC9lNG84SGNBaXh3RVdmcXBwVmNkMXdNbnR5?=
 =?utf-8?B?TTllTFYycmJ0L0tmWjMyUG5JN2o4K2dQSG5CQjJENHQrQTREbHd6SXVGem5U?=
 =?utf-8?B?U1AvbGF6MG1lYnU2T2VrcDZkdUpEcTBmb1JOdm93RUR2MHZYOG84WnN3NzNP?=
 =?utf-8?B?b0QxRGp5d3ZXb211V3ErV2VPTzBqZlozQUorTUFNUU1rcWZhSkVGRTZCZVcr?=
 =?utf-8?B?RlFIQ2ZHaWxFaVNURndQRHpuTGFnOGNwbElvZ0N0SHZQbmkrczA0ZHlndWMv?=
 =?utf-8?B?ZnhsMjA4RFhuYUpjWXZKNnFGWDlyNW9iNEpua1JnOS9IZFR0YXV0TVM2MU8z?=
 =?utf-8?B?OXk3NVMvd1lkK21Sai94UHdWTHBWSlEyL3d3blN2TWhxcERDSzZjZkFtU1ZX?=
 =?utf-8?B?ZHhpSG91a2YxbndvVlZkKzVJQjQrSU1OeXd1TGFGcmVldzR6N1lJU1dKNTJX?=
 =?utf-8?B?MUJ5SlY5UU1Ed2EvTnZ1MkNrZHFrdHB2a0RFbWNMQnRmTHVpeTJSVXN0bXpN?=
 =?utf-8?B?Q1hPUUJNOEhYakszYTUvS3JQSkcyV2RUNjlWYk10K21NOXZjQWQ4LzJOUXZr?=
 =?utf-8?B?cStSVFpodXA0cHBFSmx1MDA0NWZHTUZjODhSbXd6dmNxYUQ1TmFxM20rNUFC?=
 =?utf-8?B?YTFRNmdqeTVQcGlPWjIrZVQrL203TW9TOEVSQkd4REpUaEU5RmI0UkpBcGIr?=
 =?utf-8?B?dVdSdE1CTm9wNG1RMHJFMkhnZnVIWkREMFpPbUhCTTZaaVdXNnR4VStZT0Fx?=
 =?utf-8?B?bWpMTjBrSkhsb1g4S3A1blpRenJTNE41b212SGttNWtyVjY1Y1FoQUVxek81?=
 =?utf-8?B?Yzg2VHNaL0hnT2cybHZoa2VraUZLUFJnQjFZdi95Zk1MTHJJdE5rSWxIdzhK?=
 =?utf-8?Q?9Rx91UIaPayRBiT/9/XhULfGHiF9RhM7nkQlMeMfmH6v?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: af567642-f139-404c-e796-08de3d34cfb7
X-MS-Exchange-CrossTenant-AuthSource: MN2PR12MB3997.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Dec 2025 06:23:37.6943
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: +ZMtXm2olsLeFGvwQLq6ut3DmgL9rKF1BLSuuCRuOuP61hE7YD3brrxlf2x6oaEn4ETk1bpepOT/cuHblekcIA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PPF95ABFC125

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(
>              }
>          };
> =20
> +        // There are two versions of Booter, one for Turing/GA100, and a=
nother for
> +        // GA102+.  The extraction of the IMEM sections differs between =
the two
> +        // versions.  Unfortunately, the file names are the same, and th=
e headers
> +        // don't indicate the versions.  The only way to differentiate i=
s 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) =3D if chipset <=3D Chipset::GA=
100 {
      (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 {
>      }
> =20
>      fn boot_addr(&self) -> u32 {
> -        self.imem_sec_load_target.src_start
> +        if let Some(ns_target) =3D &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
> +        }
>      }
>  }
> =20