Re: [PATCH v8 02/13] gpu: nova-core: add ImemNonSecure section infrastructure

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

On Sat Jan 24, 2026 at 3:07 AM JST, Gary Guo wrote:
> On Thu Jan 22, 2026 at 10:28 PM GMT, Timur Tabi wrote:
>> The GSP booter firmware in Turing and GA100 includes a third memory
>> section called ImemNonSecure, which is non-secure IMEM.  This section
>> must be loaded separately from DMEM and secure IMEM, but only if it
>> actually exists.
>>
>> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
>> Reviewed-by: John Hubbard <jhubbard@nvidia.com>
>> ---
>>  drivers/gpu/nova-core/falcon.rs          | 19 +++++++++++++++++--
>>  drivers/gpu/nova-core/firmware/booter.rs |  9 +++++++++
>>  drivers/gpu/nova-core/firmware/fwsec.rs  |  5 +++++
>>  3 files changed, 31 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
>> index 310d4e75bad3..0d1a0f86d83c 100644
>> --- a/drivers/gpu/nova-core/falcon.rs
>> +++ b/drivers/gpu/nova-core/falcon.rs
>> @@ -242,6 +242,9 @@ fn from(value: PeregrineCoreSelect) -> Self {
>>  pub(crate) enum FalconMem {
>>      /// Secure Instruction Memory.
>>      ImemSecure,
>> +    /// Non-Secure Instruction Memory.
>> +    #[expect(unused)]
>> +    ImemNonSecure,
>>      /// Data Memory.
>>      Dmem,
>>  }
>> @@ -351,6 +354,10 @@ pub(crate) trait FalconLoadParams {
>>      /// Returns the load parameters for Secure `IMEM`.
>>      fn imem_sec_load_params(&self) -> FalconLoadTarget;
>>  
>> +    /// Returns the load parameters for Non-Secure `IMEM`,
>> +    /// used only on Turing and GA100.
>> +    fn imem_ns_load_params(&self) -> Option<FalconLoadTarget>;
>> +
>>      /// Returns the load parameters for `DMEM`.
>>      fn dmem_load_params(&self) -> FalconLoadTarget;
>>  
>> @@ -460,7 +467,9 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
>>          //
>>          // For DMEM we can fold the start offset into the DMA handle.
>>          let (src_start, dma_start) = match target_mem {
>> -            FalconMem::ImemSecure => (load_offsets.src_start, fw.dma_handle()),
>> +            FalconMem::ImemSecure | FalconMem::ImemNonSecure => {
>> +                (load_offsets.src_start, fw.dma_handle())
>> +            }
>>              FalconMem::Dmem => (
>>                  0,
>>                  fw.dma_handle_with_offset(load_offsets.src_start.into_safe_cast())?,
>> @@ -517,7 +526,7 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
>>  
>>          let cmd = regs::NV_PFALCON_FALCON_DMATRFCMD::default()
>>              .set_size(DmaTrfCmdSize::Size256B)
>> -            .set_imem(target_mem == FalconMem::ImemSecure)
>> +            .set_imem(target_mem != FalconMem::Dmem)
>>              .set_sec(if sec { 1 } else { 0 });
>>  
>>          for pos in (0..num_transfers).map(|i| i * DMA_LEN) {
>> @@ -546,6 +555,12 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
>>  
>>      /// Perform a DMA load into `IMEM` and `DMEM` of `fw`, and prepare the falcon to run it.
>>      pub(crate) fn dma_load<F: FalconFirmware<Target = E>>(&self, bar: &Bar0, fw: &F) -> Result {
>> +        // The Non-Secure section only exists on firmware used by Turing and GA100, and
>> +        // those platforms do not use DMA.
>> +        if fw.imem_ns_load_params().is_some() {
>> +            return Err(EINVAL);
>> +        }
>
> As pointed out in last round of review this is unreachable code.
>
> I am not too against in leaving it in as is, but for checks that assist
> debugging only, I would probably use `debug_assert` myself.

I expect this bit of code to go away when we address the remaining issues
with the last 2 patches, but let me add a `debug_assert` for now.