Re: [PATCH v11 09/12] gpu: nova-core: firmware: fix and explain v2 header offsets computations

[Gary Guo]

On Fri Mar 6, 2026 at 4:52 AM GMT, Alexandre Courbot wrote:
> There are no offsets in `FalconUCodeDescV2` to give the non-secure and
> secure IMEM sections start offsets relative to the beginning of the
> firmware object.
>
> The start offsets for both sections were set to `0`, but that is
> obviously incorrect since two different sections cannot start at the
> same offset. Since these offsets were not used by the bootloader, this
> doesn't prevent proper function but is incorrect nonetheless.
>
> Fix this by computing the start of the secure IMEM section relatively to
> the start of the firmware object and setting it properly. Also add and
> improve comments to explain how the values are obtained.
>
> Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
> ---
>  drivers/gpu/nova-core/firmware.rs | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
> index c2b24906fb7e..5e56c09cc2df 100644
> --- a/drivers/gpu/nova-core/firmware.rs
> +++ b/drivers/gpu/nova-core/firmware.rs
> @@ -63,7 +63,8 @@ pub(crate) struct FalconUCodeDescV2 {
>      pub(crate) interface_offset: u32,
>      /// Base address at which to load the code segment into 'IMEM'.
>      pub(crate) imem_phys_base: u32,
> -    /// Size in bytes of the code to copy into 'IMEM'.
> +    /// Size in bytes of the code to copy into 'IMEM' (includes both secure and non-secure
> +    /// segments).
>      pub(crate) imem_load_size: u32,
>      /// Virtual 'IMEM' address (i.e. 'tag') at which the code should start.
>      pub(crate) imem_virt_base: u32,
> @@ -205,18 +206,25 @@ fn signature_versions(&self) -> u16 {
>      }
>  
>      fn imem_sec_load_params(&self) -> FalconDmaLoadTarget {
> +        // `imem_sec_base` is the *virtual* start address of the secure IMEM segment, so subtract
> +        // `imem_virt_base` to get its physical offset.
> +        let imem_sec_start = self.imem_sec_base.saturating_sub(self.imem_virt_base);

Why is saturating sub used here? I didn't see any explaination on why the
saturating semantics is preferred over checked ones.

Best,
Gary

> +
>          FalconDmaLoadTarget {
> -            src_start: 0,
> -            dst_start: self.imem_sec_base,
> +            src_start: imem_sec_start,
> +            dst_start: self.imem_phys_base.saturating_add(imem_sec_start),
>              len: self.imem_sec_size,
>          }
>      }
>  
>      fn imem_ns_load_params(&self) -> Option<FalconDmaLoadTarget> {
>          Some(FalconDmaLoadTarget {
> +            // Non-secure code always starts at offset 0.
>              src_start: 0,
>              dst_start: self.imem_phys_base,
> -            len: self.imem_load_size.checked_sub(self.imem_sec_size)?,
> +            // `imem_load_size` includes the size of the secure segment, so subtract it to
> +            // get the correct amount of data to copy.
> +            len: self.imem_load_size.saturating_sub(self.imem_sec_size),
>          })
>      }
>