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@nvidia.com>
---
 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);
+
         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),
         })
     }
 

-- 
2.53.0

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@nvidia.com>
> ---
>  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),
>          })
>      }
>  

On Mon Mar 9, 2026 at 9:10 PM JST, Gary Guo wrote:
> 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@nvidia.com>
>> ---
>>  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.

They let us keep this method infallible, and an incorrect value here
will just result in the firmware not booting.

But, maybe we could compute this value at construction time with a
checked operation and return an error there. Actually that would
probably be better. I'll see if I can follow-up with a fix for that.

On Fri Mar 6, 2026 at 1:52 PM JST, 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@nvidia.com>
> ---

Reviewed-by: Eliot Courtney <ecourtney@nvidia.com>